amadan
docs: Astra's whole-branch review of readiness round 2, round 1
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+141
−0
docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2.md+141 −0
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2.md| new file mode 100644 |
| index 000000000..9e6146f9b |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2.md |
| @@ -0,0 +1,141 @@ |
| +Dormant merge: **ACCEPT WITH FIXES** |
| +Flag-on for boxes without host claims: **NOT YET SAFE** |
| + |
| +Read-only review of `650e6152..4c9bc525`. No files changed or tests run. All repository anchors below refer to `4c9bc525`. |
| + |
| +1. **High / Merge — A stopping instance can lose track of its returning sidecar.** |
| + |
| + `internal/activator/activator.go:4437` clears `sidecarStartDone` before deferred cleanup runs. Sequence: `beginStopLocked` marks stopping; the restart returns and clears the channel; `stop()` reads nil at `:3593`; the returned sidecar is still draining through `stopSidecarNow`. |
| + |
| + Nested instances are particularly exposed: `internal/activator/opset.go:390` skips host-cgroup proof for their layout, while the main process’s stop proves only its own role (`internal/activator/unitproc.go:89`). The lease can therefore be released at `activator.go:3973` while the sidecar remains. |
| + |
| + Even when the join is captured, `stopSidecarNow` merely logs a failed stop (`activator.go:746`), then forgets the returned process. Closing the join is not termination proof. |
| + |
| + **Smallest fix:** retain the join through cleanup, retain any unproven returned sidecar, and require its termination before releasing the lease. Add deterministic tests for the missed-join window and nested `ErrStillRunning`. |
| + |
| +2. **High / Merge — Two seed-obligation errors release the lease without proving surviving processes gone.** |
| + |
| + A successful flag-off seed may leave a helper running: `internal/activator/backend_exec.go:1072` deliberately skips descendant cleanup. If obligation discharge fails, `:1104` returns an ordinary error; `internal/activator/activator.go:2061` consequently releases the lease. No installed instance exists whose terminal stop will prove that helper gone. |
| + |
| + Likewise, an obligation read error returns without `ErrSeedUnproven` at `backend_exec.go:1286`, causing Restore’s error path to release at `activator.go:1987`. Nested wakes skip the preceding host proof (`internal/activator/opset.go:429`). |
| + |
| + **Smallest fix:** preserve the lease whenever seed termination remains unknown. Attempt proof after discharge failure; classify unreadable obligations conservatively. Test both paths with a surviving process. A subsequent wake’s proof cannot justify an earlier release. |
| + |
| +3. **High / Flag — I9 does not prove the required stale-grant guarantee.** |
| + |
| + `internal/rollout/sim_test.go:747` considers a failure “answered” by another grant’s **issuance**, recorded at `:1967`, regardless of whether that grant is applied. |
| + |
| + Counterexample: issue G0; begin another hold execution; issue G1; renew the marker with M14’s missing generation bump; deliver G0 first. `answered` sees G1 and suppresses the violation even though G0 cleared the later marker. Delivery is explicitly oldest-first (`:1975`). |
| + |
| + I9 also excludes facts by execution-start time rather than restriction-change order, including all same-step facts (`:2025`, `:2033`). That leaves late outcomes and same-step changes outside the asserted guarantee. |
| + |
| + **Smallest fix:** give the independent model ordered restriction transitions and capture their identity at publication. Check each accepted grant against that state; issuance alone must never discharge a fact. Add deterministic masking, late-outcome and same-step witnesses, then rerun the mutants. |
| + |
| +4. **Medium / Merge — Manual obligation recovery is not serialized against wakes.** |
| + |
| + The procedure in `docs/superpowers/plans/2026-10-07-zero-downtime-deploys-6-roll-notes.md:424` proves emptiness, deletes database files, then removes the obligation, but never excludes concurrent wakes. |
| + |
| + Between proof and deletion, Restore can discharge the obligation and begin another seed (`internal/activator/backend_exec.go:1281`). The operator can then delete that new seed’s database. |
| + |
| + **Smallest fix:** require exclusive maintenance across proof, database deletion, directory sync and obligation removal; resume wakes afterward. Correct deletion order alone is insufficient. |
| + |
| +5. **Low / Flag — The literal generation non-repetition proof omits counter exhaustion.** |
| + |
| + `internal/rollout/failures.go:751` and `:756` increment `uint64` without checking overflow. Thus “every subsequent generation is larger” is not an unconditional property. |
| + |
| + This is not a plausible operational exhaustion scenario, but it contradicts the stated hard limit. |
| + |
| + **Smallest fix:** fail closed before exhaustion, including `Snapshot`, and test the boundary. Do not silently wrap under the same book identity. |
| + |
| +**1. Plan-review findings 1–12** |
| + |
| +| Finding | Disposition | Evidence | |
| +|---|---|---| |
| +| 1. Replacement/removal generations | ADDRESSED | Per-app generations; marker replacement and pruning touch them: `internal/rollout/failures.go:898`, `:901`, `:1242`. | |
| +| 2. Upgrade identity rotation | ADDRESSED | Schema/durability migration rotates identity: `internal/rollout/failures.go:520`. | |
| +| 3. Failed seed’s partial database | ADDRESSED | Write-ahead obligation and Restore settlement: `internal/activator/backend_exec.go:1198`, `:1281`. Lease errors remain finding 2. | |
| +| 4. Required placement fallback | ADDRESSED | Required starts refuse fallback; final defensive guard: `internal/cgroup/cgroup.go:449`, `:500`, `:538`. | |
| +| 5. In-flight sidecar start | PARTIAL | Join added at `internal/activator/activator.go:3593`, but finding 1 defeats it. | |
| +| 6. Successful seed descendants | ADDRESSED | Flag-on successful seeds require descendant proof: `internal/activator/backend_exec.go:1072`. | |
| +| 7. Independent I9 facts | PARTIAL | Model facts exist, but exemptions weaken the oracle: `internal/rollout/sim_test.go:747`, `:2033`. | |
| +| 8. Mutation witnesses | ADDRESSED | Real-path mutations and named invariant checks: `internal/rollout/testdata/rolloutsim/mutate.sh:104`, `:160`, `:298`. | |
| +| 9. Retry transport/restart | ADDRESSED | Separate signed log; legacy restart processing retained: `internal/release/hostretrylog.go:36`; `internal/host/restart.go:944`. | |
| +| 10. Independent save retries | ADDRESSED | Startup and periodic retry: `internal/host/rolloutdeclined.go:109`, `:324`. | |
| +| 11. Test committed revision | ADDRESSED | Checkpoint-before-mutation requirement: `docs/superpowers/plans/2026-10-12-zero-downtime-flag-readiness-2.md:29`. Ledger reports checkpointed runs. | |
| +| 12. Factual/environment corrections | PARTIAL | Forwarder fixed at `internal/console/instancepage.go:67`; plan `:26` still contradicts supplied scratch/cache rules. | |
| + |
| +**Hard-limit assessment:** within the non-overflow range and stated file assumptions, I found no additional production mutation missing its generation change. Publication refuses dirty state; acceptance retries the save and refuses remaining dirtiness (`internal/rollout/failures.go:1095`, `:1134`). Reopen uses the highest saved sequence; migration rotates identity (`:511`, `:520`). These are substantial improvements. Certification remains incomplete because of findings 3 and 5 and the unaudited restore/clone assumption (`:349`; roll notes `:379`). |
| + |
| +The M13–M17 mutations exercise real paths: failed-save publication→I10; marker renewal, reopened sequence reuse, old-book grants and sibling-created records→I9. The runner requires the named invariant, not just any failure (`mutate.sh:104`, `:160`, `:298`). Their reported kills demonstrate sensitivity, not completeness of I9. I did not rerun them. The last inspected 3000-seed log showed the baseline passed and mutation checks still progressing. |
| + |
| +Tasks 3–7 close their identified implementation gaps, subject to real-box evidence: |
| + |
| +- **3–4:** capability/unsaved refusals and shared console eligibility agree (`internal/console/apiretry.go:88`; `instancepage.go:49`). |
| +- **5:** declined execution pins identity and tier; production installs `BeforeExec` (`internal/activator/backend_exec.go:1685`, `:2952`; `internal/host/agent.go:451`). |
| +- **6:** success requires the matching clean park, no fence/shutdown, and current scheduler revision (`internal/activator/workerswap.go:195`). |
| +- **7:** required placement cannot fall through to PID movement (`internal/cgroup/cgroup.go:538`). |
| +- **8:** remains incomplete through findings 1–2. |
| + |
| +I found no additional lock-order inversion or direct disk I/O under `Service.mu` in the changed cleanup paths; host proof drops the mutex before I/O (`internal/activator/opset.go:382`). The schema-3 refusal is deliberate local downgrade protection, not an incompatible external wire change (`internal/rollout/failures.go:501`). All seven new member-facing literals carry TODO-COPY markers (`internal/console/apiretry.go:42`, `:45`; `browserretry.go:22`, `:23`; `swapcopy.go:67`, `:72`, `:81`). |
| + |
| +**5. Every ledger ruling** |
| + |
| +Line numbers identify rulings in `/home/paulca/.cache/zdd-impl/readiness2/ledger.md`. |
| + |
| +| Lines | Position | Reason / condition | |
| +|---|---|---| |
| +| 9 | Agree with conditions | Hard rule is correct; findings 3 and 5 prevent certification. | |
| +| 10 | Agree | Capability-gated signed transport: `internal/console/apiretry.go:88`. | |
| +| 11 | Agree | Separate rev-18 amendment preserves the binding specification. | |
| +| 12 | Agree | Own-build recovery is appropriate: `internal/activator/activator.go:2523`. | |
| +| 13 | Agree with conditions | Authorized stop proof must include finding 1. | |
| +| 14 | Agree with conditions | Durable obligation is right; close finding 2. | |
| +| 15 | Agree | Required-placement refusal: `internal/cgroup/cgroup.go:449`. | |
| +| 16 | Agree | Restart must apply checked grants, not seed-skip them: `internal/host/restart.go:944`. | |
| +| 17 | Agree | Clean-park qualification: `internal/activator/workerswap.go:195`. | |
| +| 18 | Agree | Real-box work remains external: roll notes `:455`. | |
| +| 22 | Agree | Per-app generation matches record scope: `internal/rollout/failures.go:836`. | |
| +| 23 | Disagree | Calling conflicting cache commands “house rules” does not resolve plan `:26`. | |
| +| 40 | Agree with conditions | Migration and downgrade protection are sound; restore-path audit remains required (`failures.go:349`, `:520`). | |
| +| 46 | Agree | Renewal witness matches M14’s actual mutation: `mutate.sh:107`. | |
| +| 48 | Disagree in part | App-wide facts are right; issuance-as-answer and skipped same-step facts are not sufficient: finding 3. | |
| +| 50 | Agree | M17’s zero-only bump isolates stale-grant safety from initial-generation liveness (`mutate.sh:131`). | |
| +| 51 | Agree | Forced sibling health failure is a legitimate witness (`mutate.sh:139`). | |
| +| 57 | Agree | Historic legacy retries remain quiet while eligible signed retries are processed (`internal/host/restart.go:129`, `:143`). | |
| +| 64 | Agree | Declined-down wording and shared refusal match behavior (`instancepage.go:49`, `:77`). | |
| +| 68–69 | Agree | Pinned tier and recorded-build sidecar fallback agree (`backend_exec.go:2952`; `activator.go:2523`). | |
| +| 74 | Agree | Abandoning non-clean exits preserves the clean-park condition (`workerswap.go:195`). | |
| +| 80 | Agree with conditions | Placement classification is heuristic; the probe does not prove ancestor permissions (`internal/cgroup/cgroup_linux.go:78`). | |
| +| 81 | Agree | Single latch decision plus final fallback guard closes the identified race (`internal/cgroup/cgroup.go:445`, `:538`). | |
| +| 86 | Disagree in part | Whole-host proof/reseeding are sound; calling the lost-sidecar scenario impossible is unsupported by finding 1. | |
| +| 88 | Agree with conditions | Write-ahead/nested obligations improve recovery; finding 2 remains. | |
| +| 94 | Agree with conditions | Database-first order is necessary; exclusive recovery is also necessary: finding 4. | |
| + |
| +**6. Every deferred minor** |
| + |
| +Grouped by ledger entry; every item receives a disposition. |
| + |
| +| Ledger entry | Triage | |
| +|---|---| |
| +| Task 1, line 41 | **Defer:** randomness failure refusing starts is conservative (`internal/rollout/failures.go:524`). Memory-only simulation concern is superseded by disk-backed seeds (`sim_test.go:504`). “Grants never expire” is already corrected by verification (`internal/release/hostretry.go:75`). **Merge:** resolve the reported `TestACancelledObservationRecordsNothing` and `TestAPointerClearRequeuesBeforeTheNextChannelIsRead` flakes; do not dismiss known failures as scheduling noise. | |
| +| Task 1, line 43 | **Flag:** audit every data-directory copy/restore path and rotate identity. **Defer:** deliberate schema-3 downgrade refusal, with upgrade recovery (roll notes `:370`, `:379`). | |
| +| Task 2, line 52 | **Flag:** obtain final M14/M16/M17 evidence after fixing I9. **Defer:** duplicate reconciled facts are redundant rather than permissive (`sim_test.go:2031`). | |
| +| Task 3, lines 58, 60 | **Defer:** double-counted read errors; missing nil-handler/no-GET test; missing legacy lost-position test; missing history lost-position test; census key-specific coverage. These concern diagnostics or coverage around the checked delivery path (`internal/host/restart.go:129`, `:301`, `:857`). **Merge:** squash intermediate ungreen commits and validate the final tip. | |
| +| Task 5, line 71 | **Defer:** long comment; silent version-mismatch refusal; unreachable nested identity omission; unreachable flat unversioned-link arm; missing real-backend fallback test; refused-seed coverage limited to absent databases. Production hook installation and pinning support this disposition (`internal/host/agent.go:451`; `backend_exec.go:1685`; `activator.go:2523`). | |
| +| Task 6, line 76 | **Defer:** untested shutdown clause, positive control not asserting park, overstated timing comment. The production conjunction is explicit (`internal/activator/workerswap.go:195`). | |
| +| Task 7, line 83 | **Defer:** fixture-only placement misclassification, common-ancestor EACCES diagnostic gap, linking `testing`. None opens required-placement fallback (`internal/cgroup/cgroup_linux.go:66`; `cgroup.go:538`). | |
| +| Task 8, line 89 | **Defer:** claims-path cleared-Seed/ColdRestore gap; pre-existing undelegated flag-off sidecar limitation; approximately 35-second shutdown wait. These do not excuse the separately identified nested-sidecar defect (`internal/activator/opset.go:385`, `:402`; `activator.go:3596`). | |
| +| Task 8, line 91 | **Merge:** unreadable-obligation lease release and successful-seed discharge lease release—finding 2. **Defer:** planted-directory recovery, existing-obligation refusal, pre-existing older-kernel worker/uncontained proof behavior, stale `reapUncontainedStop` plan name (`backend_exec.go:1200`, `:1281`; roll notes `:436`). **Merge:** resolve `TestARetainedCopyIsPublishedBeforeItIsRecorded` flakiness; repeated failures are not “noise.” | |
| +| Task 9, line 96 | **Defer:** successful-but-unproven reseed wording, cosmetic wrapping, preamble omitting separately listed real-box items. Recovery and outstanding checks are described nearby (roll notes `:416`, `:455`). | |
| + |
| +**7. Explicit flag-on answer** |
| + |
| +**No—even without host claims, this branch is not yet ready to enable `CARLOS_SWAP`.** |
| + |
| +The remaining gates are distinct: |
| + |
| +- **Branch fixes:** findings 1–5; correction of the conflicting verification instructions; resolution of reported flaky tests; required final build/test/cross-build gates; and a completed mutation sweep using the repaired I9 on the resulting revision. |
| +- **Contract/operational gates:** Paul’s rev-18 sign-off; the separate copy-review gate; and the restore/clone audit required by the identity proof (`failures.go:349`; roll notes `:379`). Claims-on arming remains excluded as instructed. |
| +- **Real-box proof:** delegated cgroup placement, sibling limits, legacy-parent recovery, live-generation sidecar respawn, unsupported-clone3 refusal, seed/normal-stop helper termination before lease release, nested/root unit control, sidecar fallback, mixed-version retries, unsaved-book recovery, downgrade/clone behavior, outstanding round-1 checks and flagship gates E–G (roll notes `:455` onward). |
| + |
| +Real-box success cannot substitute for repairing the cleanup paths or the simulation oracle. |
| \ No newline at end of file |