amadan
S3 change index: Astra whole-branch review round 1 findings (#336)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+42
−0
docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md+42 −0
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md b/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md| new file mode 100644 |
| index 000000000..e926bcce9 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md |
| @@ -0,0 +1,42 @@ |
| +Reviewed `5f341f33..26b9e850` at `7d662671`. Tests could not start because the read-only sandbox prevented Go from creating its build directory. Findings are based on code and test inspection. |
| + |
| +1. **P1 — Moves can act on an older operation after a newer live pass.** |
| + [movespass.go:1074](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movespass.go:1074) substitutes cached records; its epoch check orders snapshots, but not snapshots against live fallback walks. If an abort misses its bump and an unrelated refresh fails, a live fallback can observe the abort and release the source fence. Once the unrelated refresh recovers, the complete snapshot can still contain the old stopping state. [offlineSourceStop:722](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movesoffline.go:722) then reinstalls that fence without rereading the record. Legacy aborts similarly delete staging before their live CAS at [movespass.go:1438](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/movespass.go:1438). |
| + |
| + **Required:** validate the current live operation before move side effects, and prevent participant/hold state from regressing after a live fallback. Add a failed-bump → fallback → snapshot-recovery regression. |
| + |
| +2. **P1 — Sidecar placement changes cannot evacuate existing workloads.** |
| + The ordinary scan excludes a sidecar whose placement becomes `no-inbound` on an inbound box at [sidecar_reconcile.go:336](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/sidecar_reconcile.go:336). The new removal confirmation checks only region and pool at [sidecar_reconcile.go:484](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/sidecar_reconcile.go:484). Because the live record still matches those two fields, confirmation refuses removal forever—even with successful bumps and repeated backstop rebuilds. |
| + |
| + **Required:** confirm the complete live eligibility decision, including the record’s placement and live sidecar manifest. Unreadable evidence must retain the row. Test both record-level and manifest-level placement changes. |
| + |
| +3. **P1 — An index GET can indefinitely block reconciliation and its backstop.** |
| + [RecordView.Refresh:120](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/recordview.go:120) and [rebuildLocked:193](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/recordview.go:193) poll under the caller’s context while holding the refresh mutex. Host callers supply long-lived contexts. A stalled index response therefore holds both the view mutex and shared poll slot. Other polls time out, but [changewatch.go:76](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/release/changewatch.go:76) leaves the previous observation usable; [agent.go:2732](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/agent.go:2732) still installs the canary gate after that tick’s failed observation. |
| + |
| + **Required:** bound both RecordView polls with `ChangeIndexPollTimeout`. Ensure poll failure cannot leave gated readers trusting a frozen observation. Test a stalled index response while ordinary bucket reads remain healthy. |
| + |
| +4. **P1 — Live removal confirmation omits protections provided by the original walk.** |
| + [confirmRecordGone:346](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/recordview_store.go:346) checks only the row’s old record key and permits removal for an out-of-scope record. `GetRecord` validates decoding and identity, whereas the original walk protects invalid shapes and empty channels *before* checking scope at [reconcile.go:901](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/reconcile.go:901). Consequently, a stale snapshot can cause deletion where the live walk would preserve the row. Checking only the old key also misses a same-host record moved to another unbumped key; the original walk collects records by host at [reconcile.go:936](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/reconcile.go:936). |
| + |
| + **Required:** preserve the live walk’s validity and host-wide protection rules before retirement. Add regressions for both deferred cases. |
| + |
| +5. **P2 — Telemetry changes behavior while disarmed.** |
| + With no watch, [currentTelemetryReads:60](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/telemetry_pass.go:60) still selects the new implementation. Its per-pass memo at [telemetry_pass.go:134](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/telemetry_pass.go:134) reuses account reads, including failures, across apps. Previously each app resolved both files independently. A transient account-read failure now disables telemetry for every subsequent app in that account during the pass. The new test explicitly pins this behavior with a nil watch at [readgate_test.go:299](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/readgate_test.go:299). |
| + |
| + **Required:** retain the original resolution path when disarmed; restrict memoization to armed readers. |
| + |
| +6. **P2 — One canary channel can suppress another channel’s restart retry.** |
| + [agent.go:2783](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/agent.go:2783) tracks freshness per app, while heads are per channel. After channel A’s promotion fails to restart and rolls back, its route is temporarily excluded by [fastMembers:2327](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/host/agent.go:2327). Channel B can then mark their shared app generation successfully. When A’s hold expires, its old cached head still matches its rolled-back live version, so the gate incorrectly considers A settled. Retry waits for another full/app pass or the backstop. |
| + |
| + **Required:** store generation and successful-read age per channel group alongside its head. Test two channels with a failed restart on one. This violates the fixed ruling that unsettled canaries retain their existing retry behavior. |
| + |
| +7. **P3 — Unchanged certificate polls skip failed local-copy retries.** |
| + [certsync.go:756](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edge/certsync.go:756) returns immediately even when the preceding local reconciliation failed at [certsync.go:1440](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/edge/certsync.go:1440). Recovery waits for another scope change or backstop. Certificates remain served from memory, so this is not a demonstrated outage. |
| + |
| + **Required change:** retry local reconciliation against the existing snapshot on unchanged polls; no additional S3 reads are needed. |
| + |
| +**Deferred-minor triage:** promote the unbounded RecordView poll/frozen-watch issue, malformed/out-of-scope and rehomed-key removal issues, and shared canary freshness issue to must-fix—findings 3, 4 and 6. The local certificate-copy retry can remain deferred. The other inspected ledger minors do not establish additional merge blockers. |
| + |
| +The census classifications and root’s added index read/CAS are acceptable: the bump operates on the fixed hint object and supplies no authorization decision ([changeindex.go:127](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/release/changeindex.go:127), [census overlay:27](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/s3-polling-336/internal/provision/testdata/plan14-root-reads-s3-change-index.tsv:27)). |
| + |
| +**Verdict: reject. P1 findings 1–4 block merge.** |
| \ No newline at end of file |