| 1 | # Final whole-branch review, fix wave R1: report |
| 2 | |
| 3 | Branch `s3-polling-336`, base of this wave `4512018a` (the review findings |
| 4 | commit). Findings: `docs/superpowers/specs/CODEX-FINDINGS-S3-CHANGE-INDEX-CODE-R1.md`. |
| 5 | All seven findings are fixed under the controller's binding rulings. Nothing |
| 6 | pushed. |
| 7 | |
| 8 | ## Commits |
| 9 | |
| 10 | | Commit | Finding(s) | Subject | |
| 11 | |---|---|---| |
| 12 | | `cd418ebf` | F3 | release, provision, host: any failed index poll leaves the watch unknown; record view polls are bounded; no canary gate on a tick that could not read the index | |
| 13 | | `c1d29378` | F2, F4 | provision: a snapshot pass decides removals from the bucket's own walk | |
| 14 | | `0c3e3ebc` | F1 | host: the moves pass acts only on its own walk of the bucket; the record view only says whether there is move work | |
| 15 | | `46a738be` | F5 | host: with no change index armed the telemetry pass is main's, one resolve per app | |
| 16 | | `6ca3bf3c` | F6 | host: a canary group is settled only by its own pointer read, at the generation and age the gate asks | |
| 17 | | `5e0af511` | F7 | edge: an unchanged cert poll retries a failed local copy from the snapshot it holds | |
| 18 | | `25eac78c` | F2, F4, census | provision: Reconcile and ReconcileSidecars stay whole units (scan and retire closures); census overlays re-pinned for the review fixes | |
| 19 | |
| 20 | `25eac78c` reshapes `c1d29378`: the first version split the record walk and |
| 21 | the rest of each pass into new functions, which renamed legacy-accepted |
| 22 | census units (B8 and plan14) and would have made the census owner re-rule |
| 23 | them. The final version keeps `Reconcile` and `ReconcileSidecars` whole, with |
| 24 | the walk and the removal phase as closures; behaviour and tests are |
| 25 | identical, and only digests and basis move in the census. |
| 26 | |
| 27 | ## F1 (P1): the moves pass acts only on live data |
| 28 | |
| 29 | Ruling applied as written. The snapshot is used only to decide whether there |
| 30 | is any move work. |
| 31 | |
| 32 | - `internal/host/movespass.go:1077` `idle := w.movesIdle(ctx, s)`, computed |
| 33 | inside `w.pass` before the outbox flushes; `:1092` returns after the |
| 34 | flushes (they still run every pass, as on main) when idle; otherwise |
| 35 | `:1097-1098` walks `provision.ListAllRecordsStrict(ctx, s)` over live, and |
| 36 | the pass runs exactly as on main. |
| 37 | - `:1296` `walkedIdle` is set at the end of a walked pass: no record with a |
| 38 | move, nothing undecodable, and `localMoveState()` false. Every early return |
| 39 | leaves it false (set false before the walk). |
| 40 | - `:1307 movesIdle`: idle only when the view vouches (`viewStore` ok), the |
| 41 | last pass walked and was idle, the snapshot's strict listing has no record |
| 42 | with `Move` set and no undecodable record, and the worker holds no local |
| 43 | state. |
| 44 | - `:1324 localMoveState`: wake fences (`fenced`), the strict set, the hold |
| 45 | view (unbuilt, or any claim/unknown), the offline keep view (unbuilt, |
| 46 | failed, or any move/unreadable host), T5 archive verdicts, landing leases, |
| 47 | and `:1356 movesDirHolds` on disk: anything under `<DataDir>/.moves/<x>/` |
| 48 | (op journals; parked journals leave the tree) or the page outbox. An |
| 49 | unreadable directory counts as holding something. Not counted, on purpose: |
| 50 | the move-log memo, `nudged`, `trouble`, `notices` (dedup and log |
| 51 | throttles; they never need a walk to be correct). |
| 52 | - Epoch guard: **removed** (field `lastEpoch` and its refusal), since no move |
| 53 | step reads the snapshot any more. The view's monotonic publication is |
| 54 | still asserted by the simulation (invariant c). |
| 55 | - `refreshOfflineView` always reads live: `internal/host/agent.go:1636` now |
| 56 | passes `s`, not the view store (start-up call at `agent.go:2985` was |
| 57 | already live). |
| 58 | - Spec §4.3 "Moves" bullet rewritten to match; `viewStore`'s doc comment in |
| 59 | `internal/host/changeindex.go` updated. |
| 60 | |
| 61 | Tests (`internal/host/changeindex_test.go`): |
| 62 | - `TestMovesPassNeverActsOnAnOlderSnapshot` (the regression Astra asked for, |
| 63 | legacy-abort flavour): an abort-complete write whose bump fails; an |
| 64 | unrelated scope fails to re-read so the pass walks live (fallback, sees no |
| 65 | move); a new move of the host is requested with its bump failing and |
| 66 | staging fills; the unrelated scope recovers, so the view is Complete again |
| 67 | holding the old `aborting` state (asserted). The pass must not delete the |
| 68 | new move's staging. RED on HEAD's moves pass: `the pass acted on the |
| 69 | snapshot's old abort and deleted the new move's staging: [delete |
| 70 | data-uk/moves/a.go.example.com/generations/g/snapshot delete |
| 71 | data-ie/move-receipts/a.go.example.com.json]`. GREEN after. |
| 72 | - `TestMovesPassSkipsTheWalkOnlyWithNoMoveWork`: an idle armed pass makes no |
| 73 | record-family listing; a worker holding a wake fence walks. RED on HEAD: |
| 74 | `a worker holding a wake fence skipped its walk`. GREEN after. |
| 75 | - `TestMovesPassDecidesFromItsOwnRefresh` replaces |
| 76 | `TestMovesPassTakesSnapshotUnderPass` (it asserted the removed epoch): pass |
| 77 | A paused after its reconcile, a move requested, A's moves leg sees it from |
| 78 | its own refresh under `w.pass` and walks. GREEN. |
| 79 | - `TestMovesPassRefusesOlderEpoch` deleted with the guard. |
| 80 | - `TestReconcileUsesViewWhenArmed` updated: the unbuilt offline keep view now |
| 81 | lists the bucket once (from live), and two later passes list nothing. |
| 82 | |
| 83 | ## F2 + F4 (P1): removals over a snapshot are decided by the bucket's walk |
| 84 | |
| 85 | Ruling applied: per-row confirmations replaced by one live view of the facts |
| 86 | per pass, built on the first removal candidate, by the walk's own logic. |
| 87 | |
| 88 | - `internal/provision/reconcile.go:835` `scan := func(st release.Store)`: |
| 89 | the record-collection phase, unchanged, over a store argument. `:1120` |
| 90 | `trashOver(st, pre)`: the trash lookup over a store (the pass's own over |
| 91 | `s` with its prefetch; the live one over the bucket with none). `:1542` |
| 92 | `retire(recs, protect, trashFor, stopAtFirst)`: the removal phase, |
| 93 | unchanged except that with `stopAtFirst` it removes nothing and reports |
| 94 | whether any row would go. Over a snapshot (`onSnapshot(s)`) the pass calls |
| 95 | `retire(..., true)`; on the first candidate it runs `scan(freshStore(s))` |
| 96 | (`:1614`) and, if that walk read cleanly, `retire` over the live facts and |
| 97 | `trashOver(live, nil)`, which decides every removal that pass (including |
| 98 | rows that were not candidates on the snapshot). A live walk that errs or |
| 99 | sets `readFailed` keeps every row that pass. |
| 100 | - `internal/provision/sidecar_reconcile.go:210` `scan`, which also returns |
| 101 | the per-app sidecars manifest lookup (the mint loop keeps using the |
| 102 | snapshot's), so live placement uses the bucket's record and manifest |
| 103 | against `cfg.Inbound`. `:499 retire`, `:538` the live re-scan. |
| 104 | - Snapshot passes with no candidate read nothing more; non-snapshot stores |
| 105 | walk once, as before. |
| 106 | - `confirmRecordGone`, `confirmSidecarGone`, `confirmTrashed` deleted from |
| 107 | `internal/provision/recordview_store.go`; their reasoning now sits on |
| 108 | `onSnapshot` (`:331`); `walkErrSuffix` (`:339`) formats the log line. |
| 109 | - Spec §4.4 paragraph "Removals are confirmed in the bucket" rewritten as |
| 110 | "Removals are decided by the bucket's walk". |
| 111 | |
| 112 | Tests (`internal/provision/recordview_test.go`, new `removalWorld` fixture |
| 113 | over a bumping store plus a non-bumping handle on the same directory): |
| 114 | - `TestSnapshotSidecarPlacementChangeEvacuates/record` and `/manifest`: |
| 115 | placement becomes no-inbound in the record or in the sidecars manifest, |
| 116 | bumps succeed, inbound box. RED: `a no-inbound sidecar stayed on an inbound |
| 117 | box over the snapshot` (both). GREEN. The same test asserts a snapshot pass |
| 118 | with nothing to remove makes no bucket listing. |
| 119 | - `TestSnapshotRemovalProtectsAnInvalidOutOfScopeRecord`: the live record |
| 120 | (unbumped) has an empty channel and an out-of-scope region. RED: `route |
| 121 | one.example.com missing`. GREEN (kept, and the plain walk agrees). |
| 122 | - `TestSnapshotRemovalKeepsARehomedHost`: the same host moves to another |
| 123 | app's key without a bump. RED: `route one.example.com missing`. GREEN; the |
| 124 | same test then deletes it everywhere and the row is removed (really gone |
| 125 | still removed). |
| 126 | - `TestRemovalOverTheBucketWalksOnce`: disarmed path unchanged, one |
| 127 | `List accounts/` per pass and the removal happens. GREEN before and after |
| 128 | (a guard). |
| 129 | - Existing `TestSnapshotAbsenceDoesNotRetireALiveRecord`, |
| 130 | `TestSnapshotTrashMarkerIsConfirmedBeforeRemoval`, |
| 131 | `TestSnapshotAbsenceDoesNotRetireALiveSidecar` pass unchanged. |
| 132 | |
| 133 | ## F3 (P1): stalled index GET, frozen observation |
| 134 | |
| 135 | - `internal/release/changewatch.go:76-81`: a Poll whose ctx has already ended |
| 136 | or that gives up waiting for the poll slot now calls `w.fail`, so the watch |
| 137 | is unknown (`Gen`/`Observed` not ok, `Usable` false) until the next good |
| 138 | poll. ETag and entries are kept, so recovery is a 304. |
| 139 | - `internal/provision/recordview.go:201` `RecordView.poll` bounds every index |
| 140 | poll with `pollTimeout` (`release.ChangeIndexPollTimeout`, set in |
| 141 | `NewRecordView` at `:62`, a field so provision tests can shorten it), |
| 142 | derived from the caller's ctx. Refresh's failed poll walks without polling |
| 143 | again (`rebuildLocked(ctx, false)`, `:126`, `:130`). |
| 144 | - Agent: **both** protections. The watch is unknown after any failed poll |
| 145 | (so the canary gate finds every group due), and `fastTick` installs the |
| 146 | canary pointer gate only when its own index observation succeeded |
| 147 | (`internal/host/agent.go:2749`, `d.canaryGate != nil && indexOK`), which |
| 148 | also covers another caller's poll succeeding in between. |
| 149 | |
| 150 | Tests: |
| 151 | - `TestChangeWatchPollLockWaitHonoursContext` updated to the new rule (the |
| 152 | give-up makes the watch unknown; after the stuck poll answers, the |
| 153 | generation is back). RED: `a give-up on the lock left the watch usable: its |
| 154 | observation is frozen`. New `TestChangeWatchCancelledPollIsUnknownThenA304`. |
| 155 | RED: `a cancelled Poll left the watch usable`. Both GREEN. |
| 156 | - `TestViewStalledIndexIsBounded` (provision): index GET hangs while the |
| 157 | bucket answers; Refresh and Rebuild each return, walk, publish a Complete |
| 158 | snapshot with the new record, poll the index exactly once each, the watch |
| 159 | is unknown; after the stall, a refresh recovers and the next quiet refresh |
| 160 | is one conditional GET. RED: `Refresh over a stalled index did not return: |
| 161 | its poll is unbounded`. GREEN. |
| 162 | - `TestStalledIndexGetKeepsReconcileWalking` (host): armed agent, a stalled |
| 163 | index GET; `reconcileSerialized` returns within the bound and mints the new |
| 164 | record (it walked), the watch is unknown, a gate marked before the stall is |
| 165 | due again, and the next good poll recovers the watch. RED against HEAD's |
| 166 | RecordView: `a reconcile pass hung behind a stalled index GET`. GREEN. |
| 167 | - `TestCanaryPointerReadWhenTheTicksIndexPollFails` (host): another caller's |
| 168 | index GET hangs holding the slot; the tick gives up and must read the |
| 169 | canary pointer; after the stall, the quiet tick is gated again. RED against |
| 170 | HEAD's ChangeWatch: `a tick whose index poll gave up skipped the pointer |
| 171 | read (1 reads, want 2)`. GREEN. |
| 172 | - Edge tests re-checked unchanged: `TestHomeMapStampsRefreshWhileTheWatchIsStuck`, |
| 173 | `TestCertMirrorPollsPastAStuckWatch` and the rest of |
| 174 | `-run 'Cert|HomeMap|Mirror|Mesh|Health'` pass; they assert the poll and the |
| 175 | heartbeat finish, which still holds. |
| 176 | |
| 177 | ## F5 (P2): telemetry unchanged while disarmed |
| 178 | |
| 179 | - `internal/host/telemetry_pass.go:87`: `telemetrySwitchesWith` over a nil |
| 180 | watch (`r == nil || r.w == nil`) runs `telemetrySwitchesResolve` (`:136`), |
| 181 | main's `telemetrySwitches` body verbatim (one `telemetry.Resolve` per |
| 182 | account/app). Memo and cross-pass values apply only when armed. |
| 183 | - `TestTelemetryAccountReadOncePerPass` now runs over a polled watch (armed). |
| 184 | - New `TestTelemetryDisarmedResolvesEachAppOnItsOwn`: three apps of one |
| 185 | account, the first account read fails once; disarmed, the account file is |
| 186 | read three times and two apps stay on. RED: `three apps read the account's |
| 187 | telemetry.json 1 time(s) disarmed, want 3`. GREEN. |
| 188 | |
| 189 | ## F6 (P2): one canary channel suppressing another's retry |
| 190 | |
| 191 | - `internal/host/agent.go:2201` `canaryRead{head, gen, at}` stored per |
| 192 | (account, app, channel) in `canaryHeads`; the read callback records the |
| 193 | generation `Due` returned and `now`. `:2823 canarySettled(k, routes, gen, |
| 194 | now)` requires the group's own `gen` to equal the one `Due` returned and |
| 195 | its own read to be younger than `ReadGate.MaxAge()` (new, |
| 196 | `internal/release/changewatch.go:204`), then every route live on its head. |
| 197 | - New `TestCanaryGroupFreshnessIsPerChannel`: canary/dev and canary/beta of |
| 198 | one app; dev's promote is held out by a restart hold while beta reads and |
| 199 | marks the app scope; when dev's hold ends it reads again and adopts v2. |
| 200 | RED: `dev's pointer read 1 time(s) after its hold ended, want 2: beta's |
| 201 | read settled dev's group`. GREEN. |
| 202 | |
| 203 | ## F7 (P3): cert local-copy retry |
| 204 | |
| 205 | - `internal/edge/certsync.go`: `syncLocal` returns whether any entry failed; |
| 206 | `publish` stores it in `localPending` (`:932`); the armed poll's unchanged |
| 207 | branch retries `syncLocal` against `m.snapshot()` when pending (`:763-764`). |
| 208 | No bucket read. |
| 209 | - New `TestCertMirrorRetriesAFailedLocalCopyOnAnUnchangedPoll`: local cache |
| 210 | refuses Puts on the first poll; the next unchanged poll writes it with zero |
| 211 | bucket reads; a settled unchanged poll writes nothing. RED: `an unchanged |
| 212 | poll after a failed local copy left the local cache without it: |
| 213 | acme/autocert: certificate cache miss`. GREEN. |
| 214 | |
| 215 | ## Census changes (for the #280 request amendment) |
| 216 | |
| 217 | All in this branch's own unratified overlays, still `PROPOSED-s3-change-index`. |
| 218 | |
| 219 | `internal/provision/testdata/plan14-root-reads-s3-change-index.tsv` |
| 220 | (seal in `plan14_root_reads_test.go` moves from `7d4b63ec…` to |
| 221 | `1eccb15a9cc0a215c4c3addb818b6310f9dd76fc6f8aba38038254b6037dfee6`): |
| 222 | - Re-pinned (class unchanged, `legacy`/`accepted-legacy`; new digests, basis |
| 223 | rewritten for what they now do): `internal/provision/reconcile.go:Reconcile` |
| 224 | (scan and retire closures; a snapshot removal is decided by a second walk |
| 225 | over the bucket; relative to main a removal can only be withheld), |
| 226 | `internal/provision/sidecar_reconcile.go:ReconcileSidecars` (same, with live |
| 227 | placement), `internal/host/agent.go:fastTick` (canary gate only on a tick |
| 228 | whose index observation succeeded), `fastDeps.canaryPointerGate` and |
| 229 | `fastDeps.canarySettled` (per-channel generation and age), |
| 230 | `internal/host/telemetry_pass.go:telemetrySwitchesWith` (hands the |
| 231 | disarmed pass to main's body). |
| 232 | - Removed (branch-own rows, functions deleted): |
| 233 | `internal/provision/recordview_store.go:confirmRecordGone`, |
| 234 | `confirmSidecarGone` (both the `@legacy` declaration and the entry). |
| 235 | - Added: `internal/host/movespass.go:movesDirHolds`, `legacy`/ |
| 236 | `accepted-legacy`, basis root-input (J4, as |
| 237 | `movesWorker.sweepFinalJournals`): two `file:ReadDir` sites listing |
| 238 | `<DataDir>/.moves` and its first level, only to decide whether to walk. |
| 239 | `internal/host/telemetry_pass.go:telemetrySwitchesResolve`, `legacy`/ |
| 240 | `accepted-legacy`, basis J1 root-route: main's `telemetrySwitches` body |
| 241 | under a new name, same six sites at the same relative offsets. |
| 242 | - Reworded: the `@removed` H3 row for `telemetrySwitches` (still declared, |
| 243 | still reads nothing; it now reaches main's body through |
| 244 | `telemetrySwitchesWith` when disarmed). |
| 245 | - Envelopes moved: `internal/edge`, `internal/host`, `internal/provision`, |
| 246 | `internal/release`. Header line "What changed" updated. |
| 247 | |
| 248 | `internal/provision/s3_change_index_census_test.go` (B8 record writes): |
| 249 | re-pins `Reconcile` (`b93306e1…`) and `ReconcileSidecars` (`cc2aa4db…`), |
| 250 | class `key-site` and kinds unchanged; the comment describes the closures and |
| 251 | the live re-walk instead of the confirmations. |
| 252 | |
| 253 | Console authority and invariant census: no change needed (passes). |
| 254 | |
| 255 | ## Covering runs |
| 256 | |
| 257 | All with the sandbox off, GOCACHE default, serial and niced, at `25eac78c`. |
| 258 | |
| 259 | - `go test ./internal/release/ -run 'ChangeWatch|ReadGate|ChangeIndex|PassFilter|Adopt'`: ok (3.5s). |
| 260 | - `go test ./internal/provision/ -run 'TestView|TestSnapshot|TestRemoval|Reconcile|Sidecar|RecordView|Census|Plan14|Plan9'` |
| 261 | (includes `TestRecordViewSimulation` at its default 50 seeds, the plan14 |
| 262 | root-read census and the B8 record-write census): ok (31.1s). |
| 263 | - `go test ./internal/host/ -run 'Moves|Offline|ChangeIndex|Canary|ConfigGen|Telemetry|FastTick|Scoped|Backstop|Reconcile|Instances|Stalled|Gates'`: ok (145.3s). |
| 264 | - `go test ./internal/edge/ -run 'Cert|HomeMap|Mirror|Mesh|Health'`: ok (9.8s). |
| 265 | - `go test ./internal/console/ -run 'RunSweep|SeedSweep|Census|Authority|Invariant'`: ok (195.6s), run after every code change. |
| 266 | - `nice -n 19 go test ./internal/provision/ -run TestRecordViewSimulation -seeds 3000 -count=1`: ok (286s), one sweep for this revision. |
| 267 | - `go vet ./...`: clean. `gofmt -l internal cmd`: empty. |
| 268 | - `GOOS=darwin GOARCH=arm64 go build ./...` and `GOOS=windows GOARCH=amd64 go build ./...`: both build. |
| 269 | |
| 270 | ## Concerns |
| 271 | |
| 272 | 1. F1 cost and limit, as the ruling accepts: a move started by a write |
| 273 | whose bump failed is not seen until the backstop rebuild (or any pass |
| 274 | that walks for another reason). A box that keeps local move state on |
| 275 | disk (an op journal that never reaches its stamp, a root-owned page |
| 276 | entry) walks every pass, which is main's cost, not worse. The disk check |
| 277 | counts any non-empty first-level directory under `.moves`, so unrelated |
| 278 | leftovers there would also keep the walk on; I judged over-walking the |
| 279 | safe direction. |
| 280 | 2. F1 offline flows: skipping the walk also skips the per-pass refresh of |
| 281 | the offline keep view, which keeps the last walked state (no moves, |
| 282 | nothing undecodable, by the idle rule). A new offline move whose request |
| 283 | bump failed cannot flip without the source box's report, and the source |
| 284 | only reports from a pass that walked, so I see no path to a row being |
| 285 | removed early; worth a reviewer's second look. |
| 286 | 3. F2/F4: on a snapshot pass with a candidate, log lines from the removal |
| 287 | phase ("not removing X: participant ...", "protected") can appear twice, |
| 288 | once from the snapshot facts and once from the live ones. Cosmetic. |
| 289 | 4. F2/F4 granularity: the live re-walk is a whole-deployment walk, once per |
| 290 | pass that has any removal candidate (the ruling's cost). Candidates that |
| 291 | persist (a row whose live walk keeps it, e.g. a protected record) make |
| 292 | every snapshot pass walk until the record is fixed; that is main's cost |
| 293 | for those passes. |
| 294 | 5. F3: `TestStalledIndexGetKeepsReconcileWalking` and |
| 295 | `TestCanaryPointerReadWhenTheTicksIndexPollFails` each wait out the real |
| 296 | 5s `ChangeIndexPollTimeout` (host serial and parallel phases, about 10s |
| 297 | added in total); the provision test shortens the bound through the new |
| 298 | unexported `RecordView.pollTimeout` field. |
| 299 | 6. Census: all changes are in this branch's own PROPOSED overlays and need |
| 300 | the #280 request amended (list above). New rows `movesDirHolds` and |
| 301 | `telemetrySwitchesResolve` are legacy/accepted-legacy like their siblings |
| 302 | in this overlay; the census owner may prefer different bases. |
| 303 | 7. Pre-existing, not touched: the branch's earlier tests in |
| 304 | `recordview_test.go` (`TestSnapshotAbsenceDoesNotRetireALiveRecord`, |
| 305 | `TestSnapshotTrashMarkerIsConfirmedBeforeRemoval`, |
| 306 | `TestSnapshotHoldRereadReadsTheBucket`) reuse the old `setup` fixture, |
| 307 | which uses tenant-like names (`tito`, `go.tito.io`) against the "no |
| 308 | tenant names" rule. My new tests use `shop`/`example.com`; the move |
| 309 | regression reuses the moves harness's `a.go.example.com` and account |
| 310 | `bab`. |
| 311 | |
amadan