amadan
docs: Astra's whole-branch review of readiness round 2, round 2
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+79
−0
docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2-R2.md+79 −0
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2-R2.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2-R2.md| new file mode 100644 |
| index 000000000..9cc496bdf |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-READINESS2-R2.md |
| @@ -0,0 +1,79 @@ |
| +Dormant merge: **ACCEPT WITH FIXES** |
| +Flag-on for boxes without host claims: **NOT YET SAFE** |
| + |
| +Read-only review at `bcdf4fe9`. No files changed or tests run. The remaining fixes identified here concern instructions and recovery advice; I found no new Critical, High or Medium runtime defect in the fix diff. |
| + |
| +**1. Disposition of round-1 findings and accepted triage** |
| + |
| +Repository citations below refer to `bcdf4fe9`. “Roll notes” means `docs/superpowers/plans/2026-10-07-zero-downtime-deploys-6-roll-notes.md`. |
| + |
| +| Item | Disposition | Evidence | |
| +|---|---|---| |
| +| 1. Returning sidecar lost during stop | **ADDRESSED** | Cleanup retains the join until the returned sidecar is stopped or retained as unproven (`internal/activator/activator.go:749`, `:4498`). Both returned and co-stopped sidecars block completion (`:3707`, `:3839`). Pending stops retry their termination proof (`internal/activator/opset.go:691`). Deterministic cases cover the missed join and both nested-sidecar cases (`internal/activator/stop_returned_sidecar_test.go:55`, `:115`, `:175`). | |
| +| 2. Seed-obligation errors release lease | **ADDRESSED** | Failed discharge after an unproved success attempts termination proof and returns `ErrSeedUnproven` if it fails (`internal/activator/backend_exec.go:1106`). Unreadable obligations return that sentinel too (`:1307`). Restore and Seed callers leave the lease to its TTL (`internal/activator/activator.go:2045`, `:2130`). Tests cover both errors and successful proof (`internal/activator/containment_test.go:696`). | |
| +| 3. I9 issuance masking and temporal exclusions | **ADDRESSED** | Independent transition numbers replace execution-start ordering (`internal/rollout/sim_test.go:764`). Grants capture that number (`:1997`); acceptance checks precede registering the resulting clear (`:2029`). The masking, late-outcome and same-step witnesses, plus a positive control, are at `internal/rollout/sim_i9_witness_test.go:83`, `:102`, `:124`, `:137`. Final sweep remains outstanding. | |
| +| 4. Manual recovery races wakes | **ADDRESSED** | Agent shutdown now brackets proof, database deletion, directory sync and obligation removal; restart comes last (roll notes `:459`). Nested units bind their lifetime to the edge (`internal/host/units/carlos-instance-seed@.service:14`). | |
| +| 5. Sequence overflow | **ADDRESSED** | `Snapshot` returns zero at the boundary, allocation cannot wrap, and exhausted saves fail (`internal/rollout/failures.go:768`, `:787`, `:729`). Boundary/reopen coverage is at `internal/rollout/failures_epoch_test.go:495`. Recovery advice has a separate defect below. | |
| +| Cancelled-observation flake | **ADDRESSED** | Flushes acknowledgements and rejects publication of the observed build while allowing the scheduler’s legitimate v1 acknowledgement (`internal/rollout/wakeplan_test.go:259`). | |
| +| Pointer-clear ordering flake | **ADDRESSED** | Counts the pass’s pointer reads, excluding the retry attempt’s independent read (`internal/rollout/manager_test.go:1165`). The ordering assertion remains. | |
| +| Retained-copy cleanup flake | **ADDRESSED** | Cleanup waits for the wake goroutine’s final write (`internal/activator/wakecheck_test.go:254`; `internal/activator/activator.go:1533`). | |
| +| Data-directory copy audit | **PARTIAL** | The two-repository audit is recorded at roll notes `:387`. Inspected move paths carry host directories and environment files, not the root-level book (`internal/host/movespass.go:337`). Two deployment repositories and externally configured backups remain explicitly unaudited (roll notes `:404`). | |
| +| Plan environment instructions | **PARTIAL** | The explanation follows the controller’s fix brief, but still contradicts the supplied default-cache rule: `docs/superpowers/plans/2026-10-12-zero-downtime-flag-readiness-2.md:27` overrides `GOCACHE`. | |
| + |
| +**2. New defects and checks of the critical paths** |
| + |
| +1. **Low / Merge — Exhaustion recovery advice is incorrect and offers a destructive alternative.** |
| + |
| + `internal/rollout/failures.go:200` recommends dropping the `book` key, or deleting the file. Dropping only the identity does not recover an exhausted sequence: reopen retains `highestSaved()` (`:524`), while identity rotation changes only identity and dirty state (`:533`). The next allocation exhausts again. |
| + |
| + Deleting the whole file also removes failure records and hold-ran restrictions—the reason roll notes `:375` explicitly warns against deletion. |
| + |
| + **Smallest fix:** remove both recovery recommendations. Document that identity rotation alone does not reset exhaustion; any supported reset must preserve restrictions and establish fresh sequence values under a new identity. Add boundary recovery coverage if implementing that reset. |
| + |
| +2. **Low / Merge — The environment correction still overrides the required cache.** |
| + |
| + Plan `:27` prescribes `GOCACHE=$TMPDIR/gocache`. The supplied machine rules explicitly require the default cache even when project documentation says otherwise. Being on disk does not resolve that conflict. |
| + |
| + **Smallest fix:** leave `GOCACHE` unset. Use a short directory on disk for socket-test scratch rather than prescribing `/tmp/k` at `:28`. |
| + |
| +**Item 1, liveness:** I found no new unrecoverable pending-stop state in the changed path. Once the main process exits and the restart join completes, the sweeper retries retained sidecars outside `Service.mu`; a failed proof leaves the entry available for another pass (`activator.go:3923`; `opset.go:696`). Nested `unitProc.Exited()` is driven by its observer, and subsequent `Stop` calls retry control requests (`unitproc.go:100`, `:106`, `:186`). A process that remains alive correctly keeps the host stopping. This remains subject to real-unit verification. |
| + |
| +**Item 2, unreadable obligations:** Refusing wakes until readability or operator recovery is appropriate. The code does not interpret uncertainty as permission to use the database. The agent-stopped procedure supplies recovery without racing another wake (roll notes `:459`). Retention here means leaving the lease to its TTL, not renewing it indefinitely. |
| + |
| +**Item 3, I9:** The revised check closes the identified oracle defects. A later grant’s issuance cannot discharge anything; only a matching applied clear after a fact can do so (`sim_test.go:790`). Same-step ordering is explicit, and late outcomes are ordered by their restriction transition. I found no new false-positive path under correct code. Forgetting clears across restart is conservative; durable generation advancement prevents an older grant from matching a later persisted restriction (`:2180`; `failures.go:524`). |
| + |
| +The M13–M17 routes remain meaningful: |
| + |
| +| Mutant | Required invariant and route | |
| +|---|---| |
| +| M13 | **I10:** failed-save publication differs from the landed file (`sim_test.go:2111`). | |
| +| M14 | **I9:** a stopped rewake renews the marker after grant capture (`:1910`, `:1966`). | |
| +| M15 | **I9:** reopening with a lowered sequence reuses a published generation (`mutate.sh:116`). | |
| +| M16 | **I9:** a rebuilt book accepts a matching old-book epoch (`mutate.sh:123`; `sim_test.go:2167`). | |
| +| M17 | **I9:** a sibling’s first-wake failure creates an app-wide restriction after capture (`sim_test.go:1949`, `:709`). | |
| + |
| +Here `mutate.sh` is `internal/rollout/testdata/rolloutsim/mutate.sh`. Its runner requires the named invariant (`:298`). The reported 300-seed kills on `929dfc39` support these paths; I did not rerun them. The inspected `sweep-bcdf4fe9.log` contained only its revision/seed header, with no completed baseline or mutant verdicts. |
| + |
| +**Item 5, callers:** I found no remaining sequence increment that can wrap. Exhaustion keeps the book dirty, preventing subsequent publication and grant acceptance (`failures.go:1131`, `:1170`). The grant that first exhausts the counter may already have cleared matching restrictions in memory; both callers explicitly handle that existing save-error contract (`internal/rollout/manager.go:844`; `internal/host/rolloutdeclined.go:289`). Further first-wake admission refuses failed intent saves (`internal/rollout/wakeplan.go:312`). |
| + |
| +**3. New “Final review: Ruling:” entries** |
| + |
| +Lines refer to `/home/paulca/.cache/zdd-impl/readiness2/ledger.md`. |
| + |
| +| Line | Position | |
| +|---|---| |
| +| 101 | **Agree with the scope.** Completion remains conditional: environment reconciliation is partial, and the deployment audit is unfinished. | |
| +| 103 | **Agree.** Including co-stopped sidecars closes the same lease-safety defect and fits the authorized normal-stop change. Retry and completion paths exist. | |
| +| 104 | **Agree.** An unreadable obligation cannot establish termination. Recovery must exclude wakes throughout. | |
| +| 105 | **Agree with conditions.** The explicitly bounded audit is useful. Unaudited deployment and backup paths remain flag gates for the boxes concerned; documentation alone does not verify them. | |
| + |
| +**4. Explicit flag-on answer** |
| + |
| +**No: `CARLOS_SWAP` is not yet ready to enable on boxes without host claims.** I found no additional runtime fix required in this diff, but the remaining gates are: |
| + |
| +- **This branch:** correct the two instructions above; obtain completed baseline and M1–M17 results from the final 3000-seed sweep; complete the repository’s required final build/test/vet/format/cross-build and reviewed, squashed-tip CI gates. Reported rollout/activator suites and darwin/windows builds do not establish every gate. |
| +- **Paul:** rev-18 sign-off and the outstanding copy approval, including the declined-box hold/Retry surface. Neither requirement is waived (`docs/superpowers/specs/2026-10-12-zero-downtime-deploys-rev18-amendment.md:3`). |
| +- **Deployment investigation:** finish the copy/restore audit for each intended deployment, including its backup configuration; any copied book must receive a new identity before use (roll notes `:404`). |
| +- **Real boxes:** delegated placement and sibling limits; legacy-parent recovery; live-generation sidecar respawn; unsupported-clone3 refusal; flat/nested seed and flag-off-stop helper termination before lease release; real-unit control and sidecar fallback; mixed-version retries and unsaved-book recovery; copy/downgrade behavior; outstanding round-1/plan-2 kernel checks and flagship gates E–G (roll notes `:501`). |
| + |
| +Claims-on boxes must continue to decline arming. |
| \ No newline at end of file |