KAP-672: close active env root GC race

[milymarkovic]

Summary

• close daemon GC TOCTOU between lifecycle decision and whole-root removal
• atomically quarantine inactive roots before delete so partial removal cannot be reused
• keep final-gate skip/error statistics accurate

Root cause

shouldCleanTaskDir sampled activeEnvRoots, then released its mutex. gcWorkspace later ran whole-root deletion. A follow-up task reusing a stale terminal issue root could claim it after the decision snapshot but before removal.

Initial reservation fixed active-first ordering, but review found a second failure path: partial RemoveAll failure could release a waiting claimant onto a damaged PriorWorkDir.

Fix

• per-root reservation linearizes claim vs deletion without holding global mutex during filesystem I/O
• delete-first path atomically renames root under verified non-symlink .gc-trash before removal
• after partial removal failure, original PriorWorkDir is absent and execenv.Reuse returns nil; caller falls back to fresh Prepare
• failed quarantines are retried by later GC cycles
• rename/quarantine failure is fail-closed

Regression coverage

• decision → follow-up active mark → final removal skip
• deterministic waiter-at-reservation handshake; no negative timing assertion
• partial removal error leaves original path non-reusable and Reuse == nil
• gcWorkspace active/error paths increment skipped only; no cleaned/orphaned/bytes count
• symlinked trash root fails closed

Verification

• focused GC/failure/race suite: -race -count=10 PASS
• go test ./internal/daemon -race -count=1 PASS
• go vet ./internal/daemon PASS
• git diff --check PASS

Commit / rollback

• commit: 0351d4991
• rollback: revert this single daemon commit

Release gate

Fresh 3-point code/security re-review, separate merge approval, test-profile canary, controlled runtime rollout, fresh long-running task plus forced test-GC smoke, then monitoring. No merge/release/deploy without approval.

Tracking: KAP-672

[vercel]

@milymarkovic is attempting to deploy a commit to the IndexLabs Team on Vercel.

A member of the Team first needs to authorize it.

[milymarkovic]

Fresh code/security review at 0d94b1dd2.

The per-root reservation correctly linearizes the normal claim-vs-delete order, uses the condition predicate safely, releases the mutex during filesystem I/O, and does not block unrelated roots. CI is green, and I reproduced the targeted race tests with -race -count=20 plus the full daemon race suite.

Blocking issue:

• cleanTaskDir always releases the reservation after os.RemoveAll, including when removal fails after deleting only part of the root. A waiting claim then resumes. execenv.Reuse accepts the prior environment whenever PriorWorkDir still exists, so a partial removal can lead to reuse of a damaged workdir rather than the documented fresh Prepare. This is a data-integrity/liveness risk (especially with locked files or permission errors). The deletion outcome must be propagated to waiters so a failed/partial delete cannot reuse that root; force a fresh cleanup/Prepare or fail the claim explicitly.

Required regression hardening:

• Replace the 25 ms negative timing assertion in TestEnvRootDeletionReservationBlocksConcurrentClaim with a deterministic handshake/test hook proving the goroutine reached the reservation gate before release.
• Exercise the gcWorkspace stats branch: active-at-final-gate and removal error must increment skipped without incrementing cleaned, orphaned, or bytesReclaimed.

No new remote attack surface, path traversal, privilege escalation, global-lock I/O, or ordinary reservation leak was found.

[milymarkovic]

CTO fresh code/security review verdict at 0d94b1d: APPROVED, no blocking findings. Verified per-root reservation linearizes final RemoveAll against markActiveEnvRoot without holding the mutex during filesystem I/O; deferred release prevents reservation leaks on RemoveAll errors; active-first deletion skips with accurate stats; delete-first claims wait and Reuse safely falls back to fresh Prepare if needed. No path-traversal, secret exposure, cross-root blocking, or deadlock issue found. Independent PASS: targeted race tests x10, full internal/daemon race suite, go vet, git diff --check, and GitHub CI. Residual limitation: in-process guard only; test-profile canary and forced-GC smoke remain mandatory before rollout. GitHub cannot record APPROVE because platform roles share the PR author's GitHub account; this COMMENT is the review artifact, with merge approval recorded separately in KAP-672.

[milymarkovic]

CORRECTION — this supersedes my preceding approval comment at the same SHA. Verdict: CHANGES REQUIRED; do not merge.

After the initial review, a concurrent review surfaced a valid failure-path blocker that changes the verdict: os.RemoveAll may partially delete a root and return an error. cleanTaskDir then releases the reservation, the waiting same-root claim resumes, and execenv.Reuse accepts the prior environment whenever PriorWorkDir still exists. The claim can therefore continue inside a partially deleted/corrupted root instead of reliably falling back to fresh Prepare.

Required before re-review:

1. Make the delete-first/error path explicit and safe: a waiting claim must never reuse a root after failed or partial removal. Prefer an atomic quarantine/rename-before-delete design where feasible, or propagate invalidation so the claim forces fresh Prepare/fails closed.
2. Replace the 25 ms negative timing assertion with a deterministic handshake/test hook proving the claimant reached the reservation gate before release.
3. Add gcWorkspace regression coverage showing active-at-final-gate and removal failure increment skipped without incrementing cleaned, orphaned, or bytesReclaimed.

The active-first linearization and ordinary success path remain sound. No remote security issue was found, but the partial-removal path is a data-integrity/liveness blocker.

[milymarkovic]

CTO fresh 3-point re-review at SHA 0351d49: APPROVED (posted as comment — same GitHub account as PR author, formal approve blocked).

1. Partial-delete blocker — closed. quarantineTaskDir atomically renames the task root into .gc-trash/<sha256(rel)> before RemoveAll; a partial removal failure leaves the damaged tree only in quarantine, PriorWorkDir is gone and execenv.Reuse falls back to fresh Prepare (asserted in TestGcWorkspace_PartialRemovalFailureQuarantinesAndOnlyIncrementsSkipped). Quarantine path is traversal-guarded (filepath.Rel + .. checks) and ensureGCTrashRoot fails closed on symlinked/non-dir trash root (TestCleanTaskDir_TrashSymlinkFailsClosed). Trash retried each GC cycle.
2. Deterministic reservation handshake — closed. reserveEnvRootForDeletion atomic with markActiveEnvRoot under activeEnvRootsMu + cond-var wait; TestEnvRootDeletionReservationBlocksConcurrentClaim uses activeEnvRootWaitHook gate, no negative-timing proof.
3. Stats coverage — closed. Both failure paths (active at final gate, partial removal) increment only stats.skipped, no bytesReclaimed; both asserted.

Independently verified at this SHA: focused tests -race -count=10 PASS, full ./internal/daemon -race PASS, go vet clean, diff confined to daemon.go/gc.go/gc_test.go, GitHub CI all green.

[multica-eve]

Changes requested at 0351d499193228d5305abb573e1b3dfb14e662ec.

There is one remaining blocking TOCTOU in the artifact-cleanup path. shouldCleanTaskDir snapshots isActiveEnvRoot(taskDir) before performing the server GC-check, but gcWorkspace later handles gcActionCleanArtifacts by calling cleanTaskArtifacts directly, without acquiring the new per-root deletion reservation.

A follow-up can therefore interleave as follows:

1. GC observes the reusable root as inactive.
2. During the GC-check request, a follow-up marks that prior root active and starts using it.
3. The earlier decision returns gcActionCleanArtifacts based on stale completion metadata.
4. GC removes matching directories (for example dependencies or build output) from the live task.

The whole-root clean/orphan paths are protected because they go through cleanTaskDir and reserveEnvRootForDeletion; the artifact branch at server/internal/daemon/gc.go:150-151 bypasses that protocol. This leaves the active-root contract incomplete and can still break a running reused task.

Please run artifact cleanup under the same per-root reservation (active task wins -> skip; GC wins -> claimant waits until cleanup completes), and add a deterministic regression test plus stats assertions for this interleaving.

The whole-root quarantine design, partial-removal fallback to fresh Prepare, symlink fail-closed behavior, and existing stats changes otherwise look sound. Focused race tests (-count=10), the full daemon race suite, and go vet ./internal/daemon passed locally; the missing interleaving is not covered by those tests.

[milymarkovic]

Addressed the artifact-path TOCTOU in 3a64edf.

Fix: gcActionCleanArtifacts no longer calls cleanTaskArtifacts directly. New cleanTaskArtifactsReserved routes artifact cleanup through the same per-root deletion reservation as whole-root removal:

• Active task wins: if a follow-up marked the root active after the shouldCleanTaskDir snapshot (e.g. during the GC-check round-trip), reserveEnvRootForDeletion fails and cleanup is skipped — only stats.skipped increments, no artifact stats.
• GC wins: once the reservation is held, a concurrent markActiveEnvRoot waits on the existing deletingEnvRoots gate until cleanup completes, so a claimant never starts inside a root whose artifacts are being deleted underneath it.

Regression tests (both deterministic):

• TestGcWorkspace_ArtifactCleanupActiveAfterDecisionOnlyIncrementsSkipped — reproduces the exact interleaving from the review (claim marks root active inside the GC-check handler); asserts artifact dir survives and skipped==1, artifactDirs==0, artifactRemoved==0, bytesReclaimed==0, empty byPattern.
• TestCleanTaskArtifactsReserved_BlocksConcurrentClaimUntilCleanupDone — uses a test-only artifactCleanupHook barrier plus the existing activeEnvRootWaitHook gate to deterministically assert the claim blocks mid-cleanup and only resumes after cleanup finishes, with the artifact removed and the task root preserved.

Verification: go vet ./internal/daemon clean; new tests -race -count=20 PASS; targeted GC/reservation tests -race -count=5 PASS; full go test ./internal/daemon -race PASS.

[milymarkovic]

@multica-eve Fresh re-review requested on rebased head 37cb9089c. Base is current 28969a849; conflicts with batch issue reconciliation (#5534) resolved while preserving batch path, whole-root quarantine/fail-closed semantics, and artifact-cleanup reservation. Local full daemon -race, go vet, diff check and GitHub CI run 29571093473 pass. Please review this exact SHA; merge/release remain out of scope.