| 1 | # Astra (codex, GPT, reasoning high) scoped re-review of the zdd plan 2 final fix wave, round 2 |
| 2 | |
| 3 | Fix range `cab0dbd4..5263503b`. Brief: ~/.cache/zdd-impl/plan2/brief-r2.md. Run read-only, 2026-10-08. |
| 4 | |
| 5 | **ACCEPT WITH FIXES** |
| 6 | |
| 7 | 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. |
| 8 | |
| 9 | ### Finding verdicts |
| 10 | |
| 11 | | Finding | Verdict | Evidence | |
| 12 | |---|---|---| |
| 13 | | D1 | **ADDRESSED** | [cleanup_linux.go:251](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/cgroup/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. | |
| 14 | | D2 | **ADDRESSED** | [gen_kernel_linux_test.go:215](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/cgroup/gen_kernel_linux_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. | |
| 15 | | 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](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/backend_exec.go:1534). The existing wake caller releases the lease for that result. Details below. | |
| 16 | | L2 | **ADDRESSED** | [containment.go:166](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/containment.go:166) refuses a missing manager with `cgroup.ErrNoDelegation`. The manager’s legacy contract is unchanged. | |
| 17 | | L3 | **ADDRESSED** | [unitproc.go:156](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/unitproc.go:156) finalises proven natural and foreign-invocation exits; [backend_exec.go:1536](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/backend_exec.go:1536) removes launch files after proven failed-start cleanup; [gen.go:279](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/instancectl/gen.go:279) recovers inactive, empty generation leftovers. | |
| 18 | | T8 | **ADDRESSED** | [exec_test.go:185](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/instanceexec/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. | |
| 19 | | T12 | **ADDRESSED** | [claimspawn.go:25](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/claimspawn.go:25) and the corresponding test comment accurately describe the closed helper and the missing isolated root protocol. | |
| 20 | |
| 21 | **L1 remains High, latent behind `GenContainment`; fix before merge.** |
| 22 | |
| 23 | 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](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/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. |
| 24 | |
| 25 | 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. |
| 26 | |
| 27 | **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](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/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. |
| 28 | |
| 29 | ### New breakage in the fix diff (severity, file:line, failing sequence, smallest fix, fix-before-merge or defer) |
| 30 | |
| 31 | No separate new regression established. |
| 32 | |
| 33 | - **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. |
| 34 | - **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. |
| 35 | - **Unproven termination:** the new failed-start paths retain files and listeners when kill proof fails. Ownership release remains the unresolved L1 finding above. |
| 36 | |
| 37 | ### Residual concerns |
| 38 | |
| 39 | 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. |
| 40 | |
| 41 | 2. **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. |
| 42 | |
| 43 | 3. **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. |
| 44 | |
| 45 | 4. **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. |
| 46 | |
| 47 | 5. **Repeated stops from observer/Stop races — agree.** Under the fresh-generation-only contract, repeated stops target the same retired generation and are harmless. |
| 48 | |
| 49 | ### Out-of-scope |
| 50 | |
| 51 | 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. |
| 52 | --- |
| 53 | |
| 54 | ## Controller adjudication (no second fix wave, per process) |
| 55 | |
| 56 | - **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. |
| 57 | - **Exe-layout-only leftovers:** deferred, as Astra agrees. |
| 58 | - **Dormancy wording:** the flag-off compatibility exceptions Astra lists in rounds 1 and 2 are accepted. They belong in the index's dormancy text. |
| 59 | |
amadan