amadan
zdd plan 1: Astra final review findings (r1 ACCEPT WITH FIXES, r2 ACCEPT)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+75
−0
docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1-R2.md+15 −0docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1.md+60 −0
CI passed — run details
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1-R2.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1-R2.md| new file mode 100644 |
| index 000000000..855a4cd6e |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1-R2.md |
| @@ -0,0 +1,15 @@ |
| +**Overall verdict: ACCEPT** — reviewed `c8dbe3de..eb942159` against spec §7.2. |
| + |
| +**Finding 1: ADDRESSED.** [internal/edge/genproxy.go:158](../../../internal/edge/genproxy.go:158) validates the sampled generation while holding `GenPools.mu`, before returning or creating a pool. `Retire` takes that same lock at line 179. Retirement therefore either removes the published pool or precedes validation that refuses to recreate it. |
| + |
| +**New findings: none.** |
| + |
| +- **Lock order and deadlock:** `ServingGen` takes only the activator lock. Retirement releases that lock before invoking hooks ([activator.go:2423](../../../internal/activator/activator.go:2423)); hook dispatch also releases `retiredMu` first. No reverse lock order found. |
| +- **Bounded reselection:** [genproxy.go:269](../../../internal/edge/genproxy.go:269) permits three reselections and exits on the fourth refusal. These do not consume the separate two dial retries. Exhaustion returns the current generation in `genMovedError`, or `errNoGeneration` when absent. Earlier disappearance enters the existing context-aware, bounded wait. |
| +- **Body release:** the new exhaustion path releases the held body before either error return. Reselection itself neither reads nor closes it; the existing no-generation timeout also releases it. |
| +- **Flag-off behavior:** unchanged. Both edge and scheduled delivery still gate generation proxies on `GenerationsOn`; the fix changes neither gate. |
| +- **Test determinism:** [genproxy_test.go:446](../../../internal/edge/genproxy_test.go:446) forces each retirement synchronously during the initial sample. `sync.Once` prevents retirement from firing again inside the pool lock. The flapping source deterministically changes its number on every call; its test verifies termination, zero retained pools and one body close. |
| + |
| +`git diff --check` and formatting checks passed. I reviewed the supplied RED/GREEN evidence but did not rerun builds or tests, preserving the read-only scope. No files changed. |
| + |
| +**Out of scope:** no additional observations. |
| \ No newline at end of file |
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1.md| new file mode 100644 |
| index 000000000..158c8f654 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-1.md |
| @@ -0,0 +1,60 @@ |
| +**ACCEPT WITH FIXES** |
| + |
| +Reviewed `4f26f15c..c8dbe3de`. No flag-off traffic regression found for supported routes with canonical real sockets. Production never calls `activator.SetGenerations`; the required cross-plan interfaces are present. |
| + |
| +**Findings affecting the dormant branch** |
| + |
| +None found. |
| + |
| +**Findings latent until generations are enabled** |
| + |
| +1. **Medium — retired generation pools can accumulate without bound.** |
| + [internal/edge/genproxy.go:145](../../../internal/edge/genproxy.go:145), also the selection-to-pool interval at line 233. |
| + |
| + Sequence: a request samples generation N → execution pauses → the activator switches generations and `Retire` deletes N’s pool → the request resumes and `pool` recreates N’s entry → `dialGen` rejects the obsolete generation and the request retries successfully. Nothing removes the recreated entry. Repeating this across generations retains arbitrarily many transports in both edge and delivery pool maps. `IdleConnTimeout` does not remove map entries. |
| + |
| + **Smallest fix:** while holding `GenPools.mu`, revalidate the sampled generation against `ServingGen` before finding or creating its pool. If it changed or disappeared, return to generation selection. Because retirement takes the same pool lock, it then either removes an already-published pool or precedes a validation that refuses publication. Add a deterministic repeated-retirement test asserting pool count remains bounded. |
| + |
| + **Fix before merge.** The ledger’s description as a “bounded leak” is incorrect. |
| + |
| +`git diff --check` and formatting checks on changed Go files passed. I did not execute builds or tests because they write files; runtime results remain those reported in the ledger. |
| + |
| +**Plan’s spec questions** |
| + |
| +1. **Foreign layouts: confirm.** Rev 17 §2.9 explicitly requires canonical sockets and `hold`. |
| +2. **Canonical links: confirm.** Publishing/removing links and skipping the advisory’s checked dial are consistent. Flag-off removal of a leftover symlink is the intentional rollback exception; regular files and real sockets remain untouched. |
| +3. **Cache while asleep: confirm.** Retaining the last serving generation preserves cache-before-wake. |
| +4. **Request generation: confirm.** Attribution to the selected connection’s generation handles first wakes correctly. |
| +5. **Version headers: confirm.** Successful managed responses use the serving build; dial failures have no serving-build header. |
| +6. **Claims-on wakes: confirm as staged work.** Canonical handles are acceptable here while dormant; generation-aware claim spawning remains a plan-3 prerequisite. |
| +7. **Version probes: confirm unchanged here.** There are no production callers requiring migration on this branch. Future generation health checks must use handles. |
| +8. **Workers: confirm as staged work.** Socketless generation identity remains later-plan work, not a completed guarantee of plan 1. |
| + |
| +**Ledger deferred-minor triage** |
| + |
| +| Item | Disposition | |
| +|---|---| |
| +| Task 1: global-switch tripwire | Defer. No demonstrated defect. | |
| +| Task 2: missing socket-sandbox guards, forwarding/refusal tests | Defer. These are coverage and test-portability gaps. | |
| +| Task 2: stale comments | Nonblocking. | |
| +| Task 3: duplicate guards | Nonblocking. | |
| +| Task 3: unconditional `noteCurrent` store | Confirm current implementation. Calls are serialized at publication; generation allocation order is not itself publication order. Later switches must maintain that serialization. | |
| +| Task 3: claim-path and empty-socket coverage | Defer; claim integration needs coverage in plan 3. | |
| +| Task 4: advisory race and cleanup-helper coverage | Defer. The described advisory race produces diagnostic noise, not a false stray warning. | |
| +| Task 5: recreated retired pool | **Must fix before merge: finding 1.** | |
| +| Task 5: delayed retirement closes a replacement’s shared-path pool | Defer. It causes reconnection without interrupting active requests or misattributing responses. | |
| +| Task 5: in-flight connection retained for 90 seconds after retirement | Reject as a general defect. Go’s `CloseIdleConnections` sets `closeIdle`, closing connections when they subsequently become idle. | |
| +| Task 5: dead branch, body mutation, retry/timeout coverage, unused assignments | Nonblocking. The proxy supplies its outgoing request; the listed coverage gaps do not establish another defect. | |
| +| Task 6: hook test only counts registration | Defer. Production wiring directly registers `genPools.Retire`. | |
| +| Task 7: incomplete integration coverage | Defer. No flag-off tagging or unmanaged-route defect found. | |
| +| Task 7: stale cache entries occupy variant slots | Defer. Existing cache limits bound retention; generation checks prevent stale delivery. | |
| +| Task 8: status, exclusions and delivery-retirement coverage | Defer. No additional implementation defect found. | |
| + |
| +**Each controller “Ruling:”** |
| + |
| +1. **Worktree location — confirm.** It does not change the branch’s behavior or reviewability. |
| +2. **Go cache and socket-test scratch — partially overrule.** An existing on-disk cache fallback is reasonable when the default is unavailable. `/tmp/k` conflicts with the supplied machine rules; future scratch must use the permitted on-disk location. The consequence is not merely build speed. |
| +3. **Model allocation — partially overrule.** Mechanical implementation assignments fit the repository policy. Sonnet reviews alone do not satisfy its production-path review requirement; this explicitly requested whole-branch Astra review provides the final assessment. |
| +4. **Explicit listener closure in the Task-5 fixture — confirm.** Closing the listener synchronously addresses the `Serve` registration race and prevents its later close from unlinking a replacement socket. |
| +5. **Controller performs Task-9 verification — confirm.** Delegation adds no necessary independence to inventory and gate execution. This ruling does not establish that the complete gate has passed; the supplied ledger does not record its completion. |
| +6. **`LaunchEnvironment` remains canonical — confirm.** It describes launch composition rather than dialing a process. More decisively, `moves.LayoutVars` classifies `CARLOS_CONTROL_SOCKET` as a presence-only comparison: different generation paths do not produce the hypothesized value-mismatch failure. |
| \ No newline at end of file |