Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ jobs:
run: |
go test ./internal/adapter/managedtemp -count=1
go test ./internal/adapter/osfs -run '^TestADR_0281_.*' -count=1
go test ./internal/app -run '^TestADR_0281_ManagedModeUnixAdmission$' -count=1
go test ./internal/app -run '^Test(ADR_0281_ManagedModeUnixAdmission$|ManagedTempRecovery|LocalPlacementPropagatesRunnerErrors)' -count=1
GOOS=freebsd GOARCH=amd64 go test -c -o "$RUNNER_TEMP/osfs-freebsd.test" ./internal/adapter/osfs
GOOS=freebsd GOARCH=amd64 go test -c -o "$RUNNER_TEMP/app-freebsd.test" ./internal/app

Expand Down
2 changes: 1 addition & 1 deletion docs/adr/0027-cloud-native.md
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ durable artifact survives and is reloaded), or **lost** (gone, possibly leaking)
| 67 | Embedded mecatui perf admin listener (ADR 0254) | `cmd/mecatui/embed.Server` | process instance, only with `--perf` | ordinary perf binds owner-private `admin.sock` beside the private gRPC socket; explicit `--perf-addr` and addressless `--perf-mcp` bind loopback TCP (the latter ephemeral). `Server.Close` shuts down the HTTP server and removes the shared private runtime directory. Each instance has its own directory, so defaults do not collide | **reconstructible/reset-by-design**: a restart creates a fresh private directory/socket or ephemeral MCP URL; metrics/profile state is live process data, not session state. No List 2 row | `cmd/mecatui/embed/embed.go` (`setupPerf`, `listenPrivateUnix`, `Close`) |
| 68 | mecak8s plaintext drain-only listener | `cmd/mecak8s` (`serve`) | process | bound before serving; `boundedShutdown` shuts down its `http.Server`, and deferred listener close releases it on every later startup failure | **reconstructible/reset-by-design**: listener state is process-local and recreated from `--drain-addr` (default `0.0.0.0:8082`); no session state exists | `cmd/mecak8s/serve.go` (`drainHTTPMux`, `serve`, `boundedShutdown`); ADR 0290 |
| 69 | Worktree selector HMAC key (ADR 0291) | `app.Build` | process lifetime | no goroutine, registry, map, cache, or external handle; the random key dies with Build/process and needs no explicit cleanup | **reset-by-design**: the key and selectors are not persisted. Restart invalidates every selector, clients relist worktrees, and the server re-enumerates current choices for constant-time matching. There is no selector registry/map to reattach | `internal/app/build.go` (`Build`); `internal/adapter/server/worktree_selector.go` (`WorktreeSelectorIssuer`) |
| 70 | Managed temporary command/job reaper worker, root GC lock/completion record, and per-lease locks (ADR 0281) | `app.Build` / `managedtemp.Namespace` | process worker; durable private managed root | `Built.Close` cancels the worker and waits only `shutdown_reap_timeout`; each startup/periodic sweep is bounded by `reap_timeout`. The root lock is non-blocking, lease locks are acquired non-blockingly per candidate, and only validated unlocked leases past TTL are removed. System or unsupported modes construct no worker and make no mutation | **mixed**: the ticker/worker reset on restart; owner-only completion record and lease manifests persist. A crashed holder releases advisory locks, so a later interval-gated Build sweep can reclaim eligible residue; incomplete/cancelled scans publish no completion record. No List 2 row: this is adapter-owned cleanup coordination, not session/run state | `internal/app/managed_temp_worker.go` (`startManagedTempWorker`); `internal/adapter/managedtemp/reaper_unix.go` (`Sweep`) |
| 70 | Managed temporary namespace/workspace handles, command/job reaper worker, root GC lock/completion record, and per-lease locks (ADR 0281) | `app.Build` / `managedtemp.Namespace` | process handles and worker; durable private managed root | `Built.Close` cancels the worker and waits only `shutdown_reap_timeout`; each startup/periodic sweep is bounded by `reap_timeout`. Namespace recovery and operation-handle pinning share a mutex: reopening the exact configured private root retires the stale cached handle, while per-operation pins close on return and independently owned workspace/lease handles are never closed or rehomed beneath active users. Runner construction opens and immediately closes a validation workspace; each command/job allocation reopens through namespace recovery and its lease owns the workspace until `Lease.Close` (including failed removal). There is no Build-lifetime workspace registry. Writable child provider guards borrow the existing Config/provider without new handles or session state, revalidating configured shell construction before model invocation; fork builders propagate construction errors before driving a child. The root GC lock is non-blocking, lease locks are acquired non-blockingly per candidate, and only validated unlocked leases past TTL are removed. System or unsupported modes construct no worker and make no mutation | **mixed**: the ticker/worker reset on restart; owner-only completion record and lease manifests persist. Missing/replaced namespaces are revalidated and safely reopened or recreated on later opens/sweeps; unsafe replacements fail closed, and configured local Bash placement cannot silently become shell-less. Old allocations whose visible workspace was replaced fail rather than being adopted into the new namespace. A crashed holder releases advisory locks, so a later interval-gated Build sweep can reclaim eligible residue; incomplete/cancelled scans publish no completion record. No List 2 row: this is adapter-owned cleanup coordination, not session/run state | `internal/app/managed_temp_worker.go` (`startManagedTempWorker`); `internal/app/temporary_storage.go` (`managedTemporaryStorage`); `internal/adapter/managedtemp/namespace_unix.go` (`openRoot`, `refreshLocked`, `Close`); `internal/adapter/managedtemp/reaper_unix.go` (`Sweep`) |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] No acceptance plan for what reads as Bounded-at-minimum work. This PR changes security/trust behavior (a configured shell that previously degraded silently to shell-less now hard-fails placement instead), adds cross-subsystem seams (forker.WithRunnerError, Namespace.AllocateWorkspace, osfs.WithManagedTemporaryAllocator, a new port.LLMProvider decorator), and changes operator-visible failure text — yet ships with no docs/acceptance/<slug>.md. Per docs/development-process.md, the only two carve-outs are a human-authorized Spike or an explicit human waiver ("skip the spine for this") — the PR body discloses neither. (Anchoring this comment on the ADR-0027 edit line since there's no plan file to anchor it to.) Could you add the plan retroactively, or confirm a waiver was given so it's on the record?


**Remote-login resource re-audit (List 1 / List 2 — ADR 0277).** Remote mecatui adds rows 64–66: the durable public registry plus per-registry/per-target flock domains, the encrypted credential source plus root-scoped keyring key/root flock, and the connected-target refresh/validator transport. These are adapter-owned credential or connection resources, not session/run state, so List 2 gains no row. All flocks are held per operation and process death releases them; their sentinel files persist. There is no cross-store journal, so the documented partial-state crash residual is not rehydratable session state. Registry and credential records are reattached from disk/keyring; validator and access-token state are derived again on the next connect. `mecatui` closes the refresh source, validator, and store before a target-switch restart completes.

Expand Down
19 changes: 17 additions & 2 deletions internal/adapter/forker/forker.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ type Forker struct {
// or nil when the child namespace has no shell. nil (the field) means the
// forker was constructed without a runner builder, so EVERY forked child is
// shell-less (the composition root wires the builder only when Bash is on).
newRunner childRunner
newRunner func(string) (tool.CommandRunner, error)
// tmpBase is the parent directory under which child directories are created.
// Empty means os.MkdirTemp's default (os.TempDir()).
tmpBase string
Expand Down Expand Up @@ -224,6 +224,17 @@ func WithDirtyOverlay() Option {
// called once per Fork with the isolated child directory; it returns nil when the
// child namespace should have no shell (e.g. the trust gate withheld it).
func WithRunner(r childRunner) Option {
return func(f *Forker) {
f.newRunner = nil
if r != nil {
f.newRunner = func(root string) (tool.CommandRunner, error) { return r(root), nil }
}
}
}

// WithRunnerError preserves configured-shell construction failures as fork errors.
// A nil runner with no error still means intentionally shell-less.
func WithRunnerError(r func(string) (tool.CommandRunner, error)) Option {
return func(f *Forker) { f.newRunner = r }
}

Expand Down Expand Up @@ -349,7 +360,11 @@ func (f *Forker) childEnv(base tool.Environment, ws tool.Workspace) (tool.Enviro
root := ws.Root()
var runner tool.CommandRunner
if f.newRunner != nil {
runner = f.newRunner(root)
var err error
runner, err = f.newRunner(root)
if err != nil {
return tool.Environment{}, fmt.Errorf("forker: configured child command runner: %w", err)
}
}
ref := session.EnvironmentRef{Kind: base.Ref().Kind, ID: root, Revision: base.Ref().Revision}
return tool.NewEnvironment(ref, ws, memledger.New(), runner)
Expand Down
85 changes: 75 additions & 10 deletions internal/adapter/managedtemp/namespace_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"os"
"path/filepath"
"strings"
"sync"
"syscall"
)

Expand All @@ -29,6 +30,7 @@ const (

// Namespace is a private managed root. Every mutation is relative to root.
type Namespace struct {
mu sync.Mutex // serializes root recovery, handle pinning, and Close
path string
root *os.Root
}
Expand All @@ -54,10 +56,20 @@ func Open(path string) (*Namespace, error) {
if err := ensurePrivateDir(parent, name); err != nil {
return nil, fmt.Errorf("managedtemp: managed root: %w", err)
}
info, err := parent.Lstat(name)
if err != nil {
return nil, err
}
root, err := parent.OpenRoot(name)
if err != nil {
return nil, fmt.Errorf("managedtemp: open managed root: %w", err)
}
opened, statErr := root.Stat(".")
visible, visibleErr := parent.Lstat(name)
if statErr != nil || visibleErr != nil || !os.SameFile(info, opened) || !os.SameFile(opened, visible) {
_ = root.Close()
return nil, errors.New("managedtemp: managed root replaced while opening")
}
if err := validatePrivateDir(root, "."); err != nil {
_ = root.Close()
return nil, fmt.Errorf("managedtemp: validate managed root: %w", err)
Expand All @@ -71,15 +83,61 @@ func Open(path string) (*Namespace, error) {
}

func (n *Namespace) initialize() error {
// Validate existing protocol objects before creating anything in an adopted root.
if err := validatePrivateDir(n.root, "workspaces"); err != nil && !errors.Is(err, fs.ErrNotExist) {
return err
}
if err := validatePrivateFile(n.root, "gc.lock"); err != nil && !errors.Is(err, fs.ErrNotExist) {
return err
}
if err := ensurePrivateDir(n.root, "workspaces"); err != nil {
return err
}
return ensurePrivateFile(n.root, "gc.lock")
}

// openRoot pins an independent handle for one operation. Recovery is serialized,
// but a long sweep must not block workspace opens. Existing workspace and lease
// handles remain independently owned, never closed or rehomed by recovery.
func (n *Namespace) openRoot() (*os.Root, error) {
if n == nil {
return nil, errors.New("managedtemp: namespace is closed")
}
n.mu.Lock()
defer n.mu.Unlock()
if err := n.refreshLocked(); err != nil {
return nil, err
}
return n.root.OpenRoot(".")
}

// refreshLocked reopens the exact configured path using the same checks as Open.
func (n *Namespace) refreshLocked() error {
if n.root == nil {
return errors.New("managedtemp: namespace is closed")
}
current, err := Open(n.path)
if err != nil {
return fmt.Errorf("managedtemp: reopen namespace: %w", err)
}
oldInfo, oldErr := n.root.Stat(".")
newInfo, newErr := current.root.Stat(".")
if oldErr == nil && newErr == nil && os.SameFile(oldInfo, newInfo) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ADVISORY — resolved cross-agent disagreement, documenting the resolution] go-architect flagged this SameFile comparison as a blocker, hypothesizing that after an external rm -rf && mkdir, the filesystem could hand the freed inode back to the recreated directory, making this comparison silently report "unchanged" and retain the stale handle — reproducing the original incident.

That can't happen as long as n.root's fd stays open across the delete+recreate: POSIX unlink-while-open semantics keep the old inode alive/pinned (nlink may drop to 0, but the kernel won't recycle that inode number while a descriptor still references it) until the last close — which here is current.Close() a few lines below, i.e. after this comparison already ran. secure-code-reviewer's read is correct.

The invariant is real but implicit and load-bearing: if this code is ever refactored to cache (dev, ino) instead of holding the fd open, this protection silently breaks and reintroduces the exact incident #1337 describes. Worth one comment line here saying so, for the next person who touches this function.

return current.Close()
}
old := n.root
n.root = current.root
return old.Close()
}

// Close releases the root handle. It does not delete managed data.
func (n *Namespace) Close() error {
if n == nil || n.root == nil {
if n == nil {
return nil
}
n.mu.Lock()
defer n.mu.Unlock()
if n.root == nil {
return nil
}
err := n.root.Close()
Expand All @@ -94,20 +152,22 @@ func (n *Namespace) Path() string { return n.path }
// identity. The digest key, never the raw identity, is used in the path. canonicalPath
// is retained only in the owner-readable manifest for operator diagnosis.
func (n *Namespace) OpenWorkspace(backend, identity, canonicalPath string) (*Workspace, error) {
if n == nil || n.root == nil {
return nil, errors.New("managedtemp: namespace is closed")
}
if backend == "" || identity == "" {
return nil, errors.New("managedtemp: workspace backend and identity are required")
}
if canonicalPath == "" || !filepath.IsAbs(canonicalPath) || filepath.Clean(canonicalPath) != canonicalPath {
return nil, errors.New("managedtemp: canonical workspace path is required")
}
key := workspaceKey(backend, identity)
if err := validatePrivateDir(n.root, "workspaces"); err != nil {
namespaceRoot, err := n.openRoot()
if err != nil {
return nil, err
}
defer func() { _ = namespaceRoot.Close() }()
if err := validatePrivateDir(namespaceRoot, "workspaces"); err != nil {
return nil, fmt.Errorf("managedtemp: workspaces: %w", err)
}
workspaces, err := n.root.OpenRoot("workspaces")
workspaces, err := namespaceRoot.OpenRoot("workspaces")
if err != nil {
return nil, fmt.Errorf("managedtemp: open workspaces: %w", err)
}
Expand Down Expand Up @@ -148,7 +208,7 @@ func (n *Namespace) OpenWorkspace(backend, identity, canonicalPath string) (*Wor
_ = root.Close()
return nil, err
}
if err := validateWorkspaceManifest(root, key); err != nil {
if err := validateWorkspace(w); err != nil {
_ = root.Close()
return nil, err
}
Expand Down Expand Up @@ -192,17 +252,22 @@ func (w *Workspace) WritePrivateFile(name string, data []byte) error {

// WritePrivateFile creates a private root-level protocol record atomically.
func (n *Namespace) WritePrivateFile(name string, data []byte) error {
if n == nil || n.root == nil {
return errors.New("managedtemp: namespace is closed")
root, err := n.openRoot()
if err != nil {
return err
}
return writePrivateFile(n.root, name, data)
defer func() { _ = root.Close() }()
return writePrivateFile(root, name, data)
}

func workspaceManifestJSON(key, canonicalPath string) ([]byte, error) {
return json.Marshal(workspaceManifest{Version: manifestVersion, Key: key, CurrentPath: canonicalPath})
}

func (w *Workspace) writeWorkspaceManifest(manifest []byte, canonicalPath string) error {
if err := validateVisibleWorkspace(w); err != nil {
return err
}
info, err := w.root.Lstat("workspace.manifest")
if errors.Is(err, fs.ErrNotExist) {
return writePrivateFile(w.root, "workspace.manifest", manifest)
Expand Down
3 changes: 3 additions & 0 deletions internal/adapter/managedtemp/protocol_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ func (*Namespace) Close() error { return nil }
func (*Namespace) OpenWorkspace(string, string, string) (*Workspace, error) {
return nil, errUnsupported
}
func (*Namespace) AllocateWorkspace(string, string, string, string) (*Lease, error) {
return nil, errUnsupported
}
func (*Workspace) Close() error { return nil }
func (*Workspace) Allocate(string) (*Lease, error) { return nil, errUnsupported }
func (*Lease) Path() string { return "" }
Expand Down
77 changes: 67 additions & 10 deletions internal/adapter/managedtemp/protocol_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ type sweepCompletion struct {

// Lease is one locked command or job allocation. Its path is adapter-internal.
type Lease struct {
path string
name string
id string
kind string
root *os.Root
lock *os.File
parent *Workspace
path string
name string
id string
kind string
root *os.Root
lock *os.File
parent *Workspace
ownsParent bool
}

// ID returns the opaque 96-bit random allocation ID.
Expand Down Expand Up @@ -167,9 +168,28 @@ func (l *Lease) Close() error {
err = closeErr
}
}
if l.ownsParent {
err = errors.Join(err, l.parent.Close())
}
return err
}

// AllocateWorkspace recovers the namespace at allocation time. The lease owns
// its independent workspace handle and releases it on Close, not at process exit.
func (n *Namespace) AllocateWorkspace(backend, identity, canonicalPath, kind string) (*Lease, error) {
w, err := n.OpenWorkspace(backend, identity, canonicalPath)
if err != nil {
return nil, err
}
lease, err := w.Allocate(kind)
if err != nil {
_ = w.Close()
return nil, err
}
lease.ownsParent = true
return lease, nil
}

// Allocate creates and locks a private cmd or job lease before publishing its manifest.
func (w *Workspace) Allocate(kind string) (*Lease, error) {
if w == nil || w.root == nil {
Expand Down Expand Up @@ -249,10 +269,16 @@ func (l *Lease) Remove() error {

// ReadSweepCompletion reads the durable sweep record without changing it.
func (n *Namespace) ReadSweepCompletion() (time.Time, error) {
if n == nil || n.root == nil {
return time.Time{}, errors.New("managedtemp: namespace is closed")
root, err := n.openRoot()
if err != nil {
return time.Time{}, err
}
data, err := readPrivateFile(n.root, "last-successful-sweep.json")
defer func() { _ = root.Close() }()
return readSweepCompletion(root)
}

func readSweepCompletion(root *os.Root) (time.Time, error) {
data, err := readPrivateFile(root, "last-successful-sweep.json")
if err != nil {
return time.Time{}, err
}
Expand Down Expand Up @@ -305,12 +331,43 @@ func replacePrivateFile(root *os.Root, name string, data []byte) error {
}

func validateWorkspace(w *Workspace) error {
if err := validateVisibleWorkspace(w); err != nil {
return err
}
if err := validatePrivateDir(w.root, "."); err != nil {
return err
}
return validateWorkspaceManifest(w.root, filepath.Base(w.path))
}

func validateVisibleWorkspace(w *Workspace) error {
// Reject a replaced namespace even for runners holding an older workspace.
// These are exactly the three private components: root/workspaces/key.
path := w.path
for range 3 {
info, err := os.Lstat(path)
if err != nil {
return err
}
if err := validatePrivateDirInfo(info); err != nil {
return err
}
path = filepath.Dir(path)
}
visible, err := os.Lstat(w.path)
if err != nil {
return err
}
opened, err := w.root.Stat(".")
if err != nil {
return err
}
if !os.SameFile(visible, opened) {
return errors.New("managedtemp: workspace replaced")
}
return nil
}

func validateLease(l *Lease) error {
if err := validateWorkspace(l.parent); err != nil {
return err
Expand Down
Loading
Loading