carlos / platform Public

Review of b014d57d (Claude Opus 5.5 + Astra via codex, both read against the code). Not merging yet: four data-loss paths. CI is green and the db tests pass; these are interleavings the tests don't reach.

P1, confirmed in code

1. The push deletes the lease along with the replica. replace calls lease.DeletePrefix on the live prefix, and DeletePrefix deletes lock.json too (mirror.go:110, "the lease lock included"). Nothing claims the backed-up→replaced step exclusively, so if two boxes in one region both own the route (dev has two eu-west-1 boxes; replica placements make this more likely), B can delete A's lock and data mid-replicate. Fix: hold one lease across delete, replicate and purge, and keep lock.json out of the delete.
2. A move that finished before the first wake can quietly undo a push. promoteStagedReplica (backend_exec.go:1572) makes staging win whenever moves/<host>/ exists, and that staging copy stays until the destination's first wake. A push in that window backs up only instances/<host>/ (possibly "nothing to back up"), replaces it, and reports done. The next wake then mirrors the old moved database over it. Fix: resolve or remove move staging before the backup, and back up whichever copy would actually be restored.
3. A failed warm-cache purge counts as success. PurgeLocalDBArtifacts ignores every os.Remove error, so a purge that fails still leads to Step "replaced" and the route being released. ExecBackend trusts an existing local file, so the next wake serves the old database and replicates it over the pushed one. Fix: an error-returning purge, check the file is gone, and keep the route held until it is.
4. An ambiguous record write can delete the upload recovery depends on (apiinstancedb.go:231). If the CAS write lands but its response is lost, the console deletes push.db. The box may already be past DeletePrefix. On retry it reads staging as missing, which is a dbFailure, so it marks the push failed and releases the route over an empty replica. Fix: after a failed write, re-read the record and delete push.db only if the request is definitely not there. Once the box is past DeletePrefix, missing staging has to keep the route held, never release it.

P2

1. releaseRoute only logs a failed SetStatus("ready"). finishPush then marks the request done, and nothing ever retries, so the route sits in "maintenance" for good. The same thing happens after an agent restart if a push record is cleared: releaseStale works from the in-memory fenced map, which a restart empties.
2. The 256 MiB cap is checked after the restore. A larger replica gets restored in full onto the shared /data volume first, and because a restore error is transient, that repeats every pass. Shared /data filling up is today's bdn/ips outage.
3. dbPass adds a second ListAllRecords (a LIST plus a GET per record) on every converge pass on every box, on top of the moves pass's identical one. Share one read.

Policy question for Paul: pulls and pre-push backups are copied into the control bucket, which is not necessarily in the instance's region. Customer data then leaves its region, and staged pulls stay until the next request or a clear.

The proof test (internal/lease/replace_test.go) uses a fresh fake lock store each time and never deletes a live lock. On retry it reuses the changed database rather than rebuilding scratch from the upload. So it shows that one replacement works in sequence, not that it is safe under a crash retry or a competing box.

Also: shipping this means rolling the box agent to every box before the console, plus a CLI release, and the docs/site/cli.md section and CLI output still need Paul's copy review.