carlos / platform Public

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 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 then reinstalls that fence without rereading the record. Legacy aborts similarly delete staging before their live CAS at 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.

1. 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. The new removal confirmation checks only region and pool at 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.

1. P1 — An index GET can indefinitely block reconciliation and its backstop. RecordView.Refresh:120 and rebuildLocked: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 leaves the previous observation usable; 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.

1. P1 — Live removal confirmation omits protections provided by the original walk. confirmRecordGone: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. 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.

Required: preserve the live walk’s validity and host-wide protection rules before retirement. Add regressions for both deferred cases.

1. P2 — Telemetry changes behavior while disarmed. With no watch, currentTelemetryReads:60 still selects the new implementation. Its per-pass memo at 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.

Required: retain the original resolution path when disarmed; restrict memoization to armed readers.

1. P2 — One canary channel can suppress another channel’s restart retry. 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. 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.

1. P3 — Unchanged certificate polls skip failed local-copy retries. certsync.go:756 returns immediately even when the preceding local reconciliation failed at 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, census overlay:27).

Verdict: reject. P1 findings 1–4 block merge.