| 1 | # Astra (codex, GPT, reasoning high) final review of zdd plan 2 (containment), round 1 |
| 2 | |
| 3 | Range `98d09867..1f75ae65` (origin/zdd-1-generations..zdd-2-containment). Brief: ~/.cache/zdd-impl/plan2/brief-r1.md. Run read-only, 2026-10-08. |
| 4 | |
| 5 | **ACCEPT WITH FIXES** |
| 6 | |
| 7 | Reviewed `98d09867..1f75ae65` against spec rev 17, the plan, index and execution ledger. Read-only review; no builds or tests run. `git diff --check` passed. |
| 8 | |
| 9 | **Findings affecting the dormant branch (flag off)** |
| 10 | |
| 11 | 1. **Medium — subtree enumeration is not actually bounded.** [cleanup_linux.go:212](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/cgroup/cleanup_linux.go:212). A flat tenant can leave many empty sub-cgroups. After termination, `Members()` reads every directory entry into memory before checking `maxCleanupGroups`. The 1,024-group limit therefore does not bound allocation or enumeration work, and this path runs without `GenContainment`. **Smallest fix:** read directory entries in bounded batches and enforce the group budget during enumeration; add a width-bound regression test. **Fix before merge.** |
| 12 | |
| 13 | 2. **Medium — kernel-test cleanup can hang on the failure it should contain.** [gen_kernel_linux_test.go:209](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/cgroup/gen_kernel_linux_test.go:209). If `OpenCleanupChild` fails—or `KillAndEmpty` fails while the incumbent remains alive—the cleanup calls `incumbent.Wait()` without killing it or closing its input. The echo child remains blocked, so the test hangs. The strict kernel test also lacks failure-path cleanup after starting children. These opt-in tests do not depend on the backend flag. **Smallest fix:** register cleanup immediately after each spawn, provide direct-process termination when cgroup cleanup fails, close pipes, and bound reaping. **Fix before merge.** |
| 14 | |
| 15 | Dormancy is otherwise preserved for ordinary spawn selection, names, launch files and ordering. However, **literal byte-identical behaviour is not true across the whole branch**: flag-off `/sweep` adds the generation glob; startup sweeping removes recognised child groups; `Members()` descends and sorts; the sampler includes populated subtrees; installation adds the template and widens the control unit’s writable paths. These are requested compatibility changes, including accepted D1/D5 decisions. Retain them, but qualify the blanket dormancy claim. |
| 16 | |
| 17 | **Findings latent until the flag is on** |
| 18 | |
| 19 | 1. **High — a failed generation-start response can discard a live process.** [gen.go:157](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/instancectl/gen.go:157), [backend_exec.go:1527](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/backend_exec.go:1527). `systemctl start` succeeds; the subsequent invocation lookup fails or its context expires; the server returns 502 without stopping the generation. The activator closes its listeners and returns a nil process. The generation can continue writing its database while wake failure releases ownership. The earlier start-error branch also removes files after a stop whose failure is only logged. |
| 20 | |
| 21 | **Smallest fix:** handle uncertain generation starts in this branch: stop and prove the named generation empty using an independent bounded context, or retain and return an unresolved cleanup handle. Do not discard ownership or files while termination is uncertain. Add successful-start/failed-show and failed-cleanup tests. The ledger’s “future caller contract” is insufficient because the caller already exists here. **Fix before merge.** |
| 22 | |
| 23 | 2. **High — `ParentLimit` reports success when containment is unavailable.** [containment.go:158](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/containment.go:158). When cgroup setup returns nil, flag-on `ParentLimit` returns nil through the manager’s nil-safe method. A caller can therefore proceed past its overlap admission check, and `StartChild` starts the successor without a generation cgroup. `KillGen` subsequently returns `ErrNoDelegation`. No parent was raised and the promised per-generation termination mechanism does not exist. |
| 24 | |
| 25 | **Smallest fix:** make the backend’s flat/foreign `ParentLimit` refuse a nil manager with `ErrNoDelegation`; preserve the legacy manager contract if other callers require it. Test that refusal before admitting a successor. **Fix before merge.** |
| 26 | |
| 27 | 3. **Medium — generation files accumulate on ordinary exits and failed starts.** [unitproc.go:102](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/unitproc.go:102), [backend_exec.go:1511](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-2-containment/internal/activator/backend_exec.go:1511). A generation exits naturally; observation marks it exited; later `Stop()` skips `StopGen`, leaving its runtime drop-in and executable directory. Clean inactive units are also skipped by `/sweep`. Separately, a refused start after `writeLaunchFor` leaves `launch.gen.N.json`, and the foreign-invocation exit branch omits launch cleanup. Successive generation numbers turn these into accumulating files rather than overwritten leftovers. |
| 28 | |
| 29 | **Smallest fix:** finalise generation files after proven termination, including natural and foreign-invocation exits; remove launch files on proven pre-execution failure. Give inactive generation leftovers a recovery cleanup path. Coordinate uncertain starts with finding 1. **Fix before merge.** |
| 30 | |
| 31 | **Rulings** |
| 32 | |
| 33 | - **Branch from plan 1 — agree.** This matches the controller’s explicit sequencing and the dependency being reviewed. Reconcile with landed main before merge. |
| 34 | - **Override `GOCACHE` — disagree as documented.** The supplied machine instructions expressly require the default cache and say it is writable. The ledger’s contrary memory-based rule does not substantiate this exception. This does not itself invalidate the code review. |
| 35 | - **Defer the dev-box kernel run — agree.** Scoped production approval is absent. DOCS-T1 results do not discharge the later dev requirement. |
| 36 | - **Push and describe after final review — agree.** This follows the controller’s requested order. |
| 37 | - **Extract shared `pinChild` — agree.** The final implementation preserves the role path’s intent-before-mkdir ordering and its refusal of `EEXIST`, including the intervening-directory case. |
| 38 | - **Add `SuccessExitStatus` — agree.** The generation template uses the same signal-translating trampoline as the existing templates. |
| 39 | - **Return the settlement callback from `spawnInCgroupAt` — agree.** Dropping it would break cleanup obligations inherited from the legacy strict path. |
| 40 | - **Preserve strict-host/owner gates and strict-aware cleanup — agree.** Generation placement must not bypass existing ownership restrictions. |
| 41 | - **Keep `spawnClaimedGen` closed — agree.** Reintroducing in-process tenant execution would reverse the isolation decision. However, `RoleLaunch.Start` is also closed at this tip: routing to it alone does not implement claimed generation spawning. |
| 42 | |
| 43 | The burst-allocation kernel-test adjustment is reasonable. It proves containment under that workload, not that gradual growth promptly produces an OOM kill. |
| 44 | |
| 45 | **Cross-plan consequences** |
| 46 | |
| 47 | - **Plan 3 Task 1:** change `spawnInCgroupAtTier`, forwarding helpers, callers and tests to return `(*exec.Cmd, func(), error)`. Preserve the strict gate and call settlement after reaping. |
| 48 | - **Plan 3 Task 10:** replace the assumed working `spawnClaimedGen` body with an explicitly designed isolated root launch protocol carrying reservations and the final authority check. `RoleLaunch.Start` currently refuses execution too. |
| 49 | - **Plan 5:** keep claimed generation activation unavailable until that protocol exists. Require usable containment before overlap, route instance wakes through nonzero `StartGen`, and prevent switching a populated legacy parent directly into the generation layout. |
| 50 | - **Plans 3 and 5:** preserve cleanup ownership after ambiguous start errors. Never retry a running generation’s start as recovery. |
| 51 | - **Index:** correct the three-result signature, document the closed claimed-spawn contract and missing root integration, record the activation prerequisites, and qualify dormancy’s shared compatibility exceptions. |
| 52 | - Keep the accepted dev kernel run and D1 reload-cost measurement as prerequisites to enabling the flag. |
| 53 | |
| 54 | **Deferred-minor triage** |
| 55 | |
| 56 | “Defer” below means it does not block this dormant merge; any stated activation prerequisite still applies. |
| 57 | |
| 58 | | Item | Disposition | |
| 59 | |---|---| |
| 60 | | T1: initially unused `isGenName` | Defer/closed: `CloseRemove` consumes it. | |
| 61 | | T1: failed spawn leaves empty parent | Defer: sampler excludes it; later cold wake resets it. | |
| 62 | | T1: fallback placement lacks a direct assertion | Defer; add when exercising generation spawn integration. | |
| 63 | | T1: populated legacy parent skips delegation | Defer to plan 5; migration/drain is required before activation. | |
| 64 | | T1/T3/T6: reconstructed or placeholder RED evidence | **Fix before merge:** correct the evidence record; do not present reconstructed text as captured output. | |
| 65 | | T2: deterministic member sorting | Defer: harmless ordering change; acknowledge it in dormancy wording. | |
| 66 | | T2: missing width-cap test | **Fix before merge**, with dormant finding 1. | |
| 67 | | T2: missing ENOENT, replaced-child identity and public-wrapper tests | Defer: no demonstrated incorrect result in those paths. | |
| 68 | | T2: non-atomic member walk | Defer: it remains supplementary evidence; retain the fence and kernel population checks. | |
| 69 | | T2/T5: attribution and trailer formatting | Defer: correct the final squash attribution. | |
| 70 | | T3: strict-parent success branch lacks fixture coverage | Defer/closed for success: covered by the reported kernel run; refusal coverage can follow. | |
| 71 | | T3: write-only `Prepared.tier` | Defer. | |
| 72 | | T3: invalid `CloseRemove` drains a parent | Defer: fail-closed API misuse; validate handle kind before mutation in a follow-up. | |
| 73 | | T3: `_ = g1` and inaccurate report line count | Defer; remove cosmetic debris and correct reporting. | |
| 74 | | T4: missing g2/strict-test cleanup; potentially hanging incumbent cleanup | **Fix before merge**, with dormant finding 2. | |
| 75 | | T4: swallowed mmap error | Defer/closed: tip checks the error and exits nonzero. | |
| 76 | | T4: discarded strict-child wait statuses | Defer: emptiness and surviving-sibling checks establish the containment result. | |
| 77 | | T4: shared three-second cleanup context | **Fix before merge** with cleanup work; give independent cleanup operations fresh budgets. | |
| 78 | | T4: swap-dependent OOM timeout; gradual-growth observation | Defer to rollout notes; accurately describe the burst-allocation evidence. | |
| 79 | | T4: ignored `StdinPipe` error and `cgroupOf` fatal bypass | **Fix before merge** with unconditional child cleanup. | |
| 80 | | T4: g2 swap limit not asserted | Defer; strengthen the next kernel validation. | |
| 81 | | T5: stale sampler comment | Defer. | |
| 82 | | T5: substring population match; missing-events coverage | Defer: kernel-generated values are constrained; exact field parsing would be clearer. | |
| 83 | | T6: escaped input passed to `ParseGenInstance` | Defer: filesystem cleanup separately unescapes and rejects unsafe components; trampoline parsing uses unescaped input. | |
| 84 | | T6: three-sample key-collision test | Defer: hostname validation and the identifier construction establish `@` exclusion. | |
| 85 | | T6: generation sweep glob | Defer/closed: present. | |
| 86 | | T7: permissive template-specifier test | Defer: inspected template has the intended split. | |
| 87 | | T7: stale template count | Defer/closed: changed hardening comment now says four templates plus probe. | |
| 88 | | T8: malformed-generation test passes for unrelated reasons | **Fix before merge:** plant otherwise-valid launch files and assert parser refusals, including traversal and extra-colon cases. | |
| 89 | | T8: missing malformed exit-record cases | Defer; shared parser already has direct validation coverage. | |
| 90 | | T9: starting an already-running generation | Defer as an explicit fresh-generation-only contract; plans 3/5 must enforce it. | |
| 91 | | T9: successful start followed by failed invocation lookup | **Fix before merge**, latent finding 1. | |
| 92 | | T9: naturally exited generation retains files | **Fix before merge**, latent finding 3. | |
| 93 | | T9: missing write/reload-failure cleanup tests | **Fix before merge** alongside start-failure cleanup. | |
| 94 | | T9: long-host cleanup round trip; empty test-only `ExeRoot` | Defer. | |
| 95 | | T10: duplicated strict-gate mapping and different error wrapping | Defer: refusal remains intact. | |
| 96 | | T10: missing end-to-end flag-on spawn cleanup coverage | Defer to integration before activation. | |
| 97 | | T10: redundant removal survives mutation | Defer: redundant cleanup need not be independently observable. | |
| 98 | | T10: nil-manager `ParentLimit` success | **Fix before merge**, latent finding 2. | |
| 99 | | T10: weak no-cgroup error assertion; stale seed comment | Defer. | |
| 100 | | T11: record-read and legacy-log non-interference assertions; missing comment | Defer. | |
| 101 | | T11: foreign-invocation launch-file leak | **Fix before merge**, latent finding 3. | |
| 102 | | T11: old `exit.run.*` records retained | Defer to migration cleanup; generation preparation must not delete another live role’s records. | |
| 103 | | T11: flag-on `Start` rejects generation zero | Defer/intentional: plan 5 must use `StartGen`. | |
| 104 | | T12: stale commit subject and helper/test descriptions | **Fix before merge:** describe a closed helper accurately, particularly in the index and final squash. | |
| 105 | | T13: literal replacement coupling in `claimUnitContent` | Defer: current replacement and its test agree; an explicit no-match failure would strengthen future edits. | |
amadan