Astra (codex, GPT, reasoning high) scoped re-review of the zdd plan 2 final fix wave, round 2

Fix range cab0dbd4..5263503b. Brief: ~/.cache/zdd-impl/plan2/brief-r2.md. Run read-only, 2026-10-08.

ACCEPT WITH FIXES

Reviewed cab0dbd4..5263503b against spec rev 17. Read-only: no builds or tests run. git diff --check passed; test results below refer to the supplied report.

Finding verdicts

| Finding | Verdict | Evidence | |---|---|---| | D1 | ADDRESSED | cleanup_linux.go:251 reads 64 entries at a time and enforces the shared group budget during enumeration. Tests cover 1,025 children refusing and 1,024 succeeding. | | D2 | ADDRESSED | genkernellinux_test.go:215 registers cleanup with direct-process kill, pipe closure and bounded reaping. Spawns receive cleanup immediately; confinement errors return through it. Independent kill budgets replace the shared context. | | L1 | NOT ADDRESSED | Server cleanup and backend resource retention are implemented, but the failed-kill path still returns a nil process at backend_exec.go:1534. The existing wake caller releases the lease for that result. Details below. | | L2 | ADDRESSED | containment.go:166 refuses a missing manager with cgroup.ErrNoDelegation. The manager’s legacy contract is unchanged. | | L3 | ADDRESSED | unitproc.go:156 finalises proven natural and foreign-invocation exits; backend_exec.go:1536 removes launch files after proven failed-start cleanup; gen.go:279 recovers inactive, empty generation leftovers. | | T8 | ADDRESSED | exec_test.go:185 requires parser refusal and zero executions, includes traversal and extra-colon cases, and verifies a valid launch succeeds. Missing files or host mismatches can no longer satisfy the malformed-case assertions. | | T12 | ADDRESSED | claimspawn.go:25 and the corresponding test comment accurately describe the closed helper and the missing isolated root protocol. |

L1 remains High, latent behind GenContainment; fix before merge.

Failing sequence: the generation starts, invocation lookup fails, and neither cleanup attempt proves termination. The backend keeps listeners and files but returns nil, errGenUnproven. At activator.go:1326, that nil process selects lease.Release(ctx) without inspecting the sentinel. With a still-valid caller context, the lease can be released while the generation continues writing.

The server’s 500-to-instancectl.ErrGenUnproven mapping works. The backend’s separate sentinel also works. Neither establishes ownership retention in the current caller. This is the original failing sequence, so implementing the controller’s sentinel suggestion alone does not close L1.

Smallest complete fix: retain the failed generation in a cleanup handle and make failed-wake cleanup retain ownership and the lease until termination is proven. Returning a handle alone is insufficient: maintenance.go:252 currently releases the lease even when Proc.Stop fails. Add a service-level regression asserting no lease release after an unproven kill, followed by eventual cleanup and release after proof.

New breakage in the fix diff (severity, file:line, failing sequence, smallest fix, fix-before-merge or defer)

No separate new regression established.

• Flag off: replacing removeLaunch() with finaliseGen() preserves legacy behavior: both skip generation zero. The additional drop-in scan is a compatibility change, not byte-identical dormancy.
• Sweep/start race: the new leftover pass holds genMu across state checks and removal; startGen marks the unit before creating files. This protects against a concurrent fresh start through this server. Duplicate starts of the same generation remain outside the established fresh-generation-only contract.
• Unproven termination: the new failed-start paths retain files and listeners when kill proof fails. Ownership release remains the unresolved L1 finding above.

Residual concerns

1. Unproven start leaves listeners without a handle — agree; disagree with deferring the complete problem to plan 3. The consequence includes lease release, not just leaked descriptors. L1 remains a merge blocker.

1. Server cleanup can take 60 seconds while the client waits 30 — agree. Every start error, including transport timeout, triggers the backend’s independent kill attempt. Thus timeout does not bypass cleanup. A failed second attempt still reaches unresolved L1; the timeout handling cannot be described as safe end to end yet.

1. Flag-off sweep only lists the directory and finds nothing — qualified agreement. That holds on a box that has never created generation drop-ins. After rollback, leftovers may exist and be removed after inactive/empty checks. Document that compatibility behavior.

1. Drop-in-only recovery and no daemon-reload — agree with deferral. Executable-only leftovers are missed. They can also arise when drop-in removal succeeds but executable removal fails, since removal errors are ignored. Skipping reload is acceptable for cleanup of retired names; subsequent generation starts reload before execution.

1. Repeated stops from observer/Stop races — agree. Under the fresh-generation-only contract, repeated stops target the same retired generation and are harmless.

Out-of-scope

Unchanged lifecycle code was inspected only to trace L1’s existing caller; this was not a broader lifecycle re-review. Other round-1 deferrals, plan-3 integration, activation prerequisites, cache-policy compliance and deployment validation remain outside this fix review. The supplied package, kernel and build results were not independently rerun. ---

Controller adjudication (no second fix wave, per process)

• L1 parked to plan 3 Task 2, and flagged to Paul as Astra's merge blocker. Real and High, but latent: ExecBackend.GenContainment is false everywhere outside tests until plan 5 sets it. Plan 3 Task 2 already owns the mechanism: cleanupFailedWake keeps the lease of a failed wake whose generation is unproven (unprovenWakes, errWakeUnproven, releasable). Plan 3 must (a) return or keep a handle for the unproven generation from startUnitAt/StartGen instead of a nil Proc; (b) have cleanupFailedWake treat errGenUnproven / instancectl.ErrGenUnproven as unproven; (c) stop maintenance.go's release after a failed Stop for generations. It needs a service-level test: no release after an unproven kill, and a release after a later proof. Paul decides whether plan 2 merges before plan 3 lands this.
• Exe-layout-only leftovers: deferred, as Astra agrees.
• Dormancy wording: the flag-off compatibility exceptions Astra lists in rounds 1 and 2 are accepted. They belong in the index's dormancy text.