Repository navigation
review: whole-repo audit after #447 — eleven bugs fixed with hardware negative controls - #455
Merged
Merged
Conversation
Without archives Release went straight to releaseResolved, which never checked the move: the source dropped the claim and answered 204 while the move still committed, so the released sandbox ran and billed on the target.
settleCapture marked the claim failed when the guest did not resume but returned nil, so a wake handed out the dead guest's socket and a hibernate captured the failed claim; a later restartFailed then left a running VM recorded as hibernated.
The goroutine a failed capture leaves queues on the claim's transition; when a wake or hibernate settled first, it still logged an interrupted capture and cleared ReaderSafe on the new hibernate, so reads woke the VM.
…at start Reconcile adopted the intent as the wake image without knowing the snapshot existed, so a crash before cocoon persisted it left a claim every wake failed to restore; the background settle now reads the list with retry first.
Only the source checked its cocoon version, so a move to a node still on an older cocoon streamed every round and paused the guest before the target's vm migrate receive failed.
Admission refused a draining target, but a drain or evacuation that began during the rounds still took the claim after evacuation had listed the node's claims, so taking the node down lost it.
…irst, tighten comments Two reconcile sweeps logged as pool.Reconcile; godocs that restated code or contradicted it (atomic hibernate, a received-VM-only Start, the v0 package summary, m.mu-only runningShaped, the move-concurrency fallback, AtCapacity) now match the code; a stale cocoon v0.5.8 note is gone.
MigrateUnsupported had no errors.As consumer outside a test; operator wake and replicaHeld forwarded once; finalPause guarded a zero it never receives; the tenant cap skipped an empty-map range that costs nothing.
archive checked for a wake image before it took the claim's transition, so a wake that landed first made the export snapshot the running guest: a pause the client saw and a full upload, then a discarded checkpoint. The archive now passes the wake image it saw and the export refuses any other source.
failMoveOut wrapped every settle error in ErrMoveStoreDown and logged that the abort waited for the registry, so a vm migrate abort failure with PG healthy answered 503 "move registry down" and sent the operator to the wrong place.
A target that restarted between the commit and the start ran vm start on a VMM the host crash had killed, so cocoon cold-booted the guest and the claim went live as if moved: no restart recorded, no guest env rewritten, a claim probe budget for a cold boot. Restart now inspects the VM and leaves a dead one to liveness, which restarts it like any other. The test engine now models a received VM as cocoon does: a live VMM with the guest paused.
Column names, downtime_ms, what the 200 promises, the 403/503 answers, target admission and the draining commit, the move registry's table and pool, the live-move capture and receive budgets, the v0.7.7 qualifier on an adopted hibernate, and sentences that described cocoon internals instead of its CLI.
…laim; drop assertions that cannot fail Tests that could not fail or did not test their name: a config fixture that set the defaults it checked, a compile-time interface assertion, a liveness check read before maintenance quiesced, a nil dereference on the failure path, an unreachable dialer branch, a dead token column and a dead env var.
errors.Join already classifies a store-down abort as ErrMoveStoreDown, a settle with no intent returns before touching ReaderSafe, and the move-in restart inspects the VM only on the branch that starts it.
A failed capture leaves a settle goroutine that commits claims.json moments later; tests that ended first failed their TempDir cleanup with "directory not empty" (3 of 10 full runs). The cold move-in test now also waits for its liveness pass to finish its usage write.
…tion The move journal kept only the epoch and series, so a source that restarted mid-move aborted without telling the target, whose reservation then held a move_concurrency slot until the 2-minute idle drop (hardware: a following move to that target answered 429 for about 2 minutes).
Reconcile resumes a journaled move, whose abort now drops the target's reservation through the move transport, but main installed that transport only after the mesh joined; a restart whose abort finished first called a nil transport and crashed.
…ntent wake test The background settle a failed hibernate leaves reads them concurrently; the race detector flagged it once in a full run.
CMGS
changed the title
…ting it warm An inspect error read as a live VMM, so a target restarted after a host crash could still cold-boot the received guest through the move path, skipping the restart record and guest env rewrite. The restart now retries like a registry outage until inspect answers.
CMGS
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Verdict
Whole-repo audit of
mainat 88e6aa5 (live move #445 and interrupted-capture settle #447 included). Eleven bugs found and fixed, each in its own commit with a regression test that fails on the old code. Every fixed path ran on hardware against the old binary as a negative control, and the old binary showed each bad outcome; the final head passed the whole hardware regression. Codex: four rounds, one P1 and one P2 (from a separate review) found and fixed, then no findings for88e6aa5d..c4b161bb(963b2fc after it only renames a variable for govet). Gates are green on both platforms. The hygiene ledger is closed: 0 files due in every lens.Bugs fixed
archive_failover, a release during a live move answered 204 while the move still committed: the released sandbox ran and billed on the target with its old tokenvm hibernateon a failed claim; new 409, no hibernateReaderSafe, so reads woke a reader-safe claimvm restore; new served by the reader VMsnapshot listfailed at start, reconcile adopted a hibernate intent whose snapshot never landed: every wake failedvm migrate abortwas reported as 503 "move registry down"vmm_restart, env backvm inspectfailedAlso fixed:
e2e/go.modleft untidy by the dependency bump turned main's CI red (#454, merged first). Test-only: a background capture settle outliving a test failed 3 of 10 full runs with "directory not empty" (d64aab2); a data race in a test (f063d74); assertions that could not fail, missing wake-verb and tenant-read coverage, evacuation of a moving claim (a009823).What ran
boot/Dockerfile. Flow readers: 8; five new flows declared and read in full (settle-capture, hibernate-wake, live-move, cocoon-cli, restart-reconcile), six existing flows re-read for the windows sandboxd: move a running claim to a named peer with cocoon live migration #445/pool: settle a capture interrupted by sandboxd's death #447 changed. Docs reader: 127 sentences checked. A fresh style reader (36/36 touched files) and flow reader (21 windows, 0 bugs) on the final diff.88e6aa5d..c4b161bb. Rounds 1–3 ran on one resumed thread; round 4's resume opened a new thread.make go-lint10/100 issues(5 modules × linux/darwin) at 963b2fc;make go-test(race) 29 ok, 0 fail at c4b161b;asl0 findings on all modules both GOOS; Python ruff format/check clean, pytest 278 passed; Rust silkd and boot/init fmt/clippy clean, tests pass on mac (161/15) and in a linux/arm64 container (162/17) — Rust unchanged in this round.Final-head regression (4aef153; M5i on c4b161b), new binary: e2e pass; R1, R2 (both orders), R3 (3/3 ticks), R4 (100 attempts, 1 reached the race order, served by the reader); M0 (332 / 217 ms), M1c, M2, M3, M4, M5, M6a–e, M7, M5i all pass; #447 interrupted capture: TERM 1 s in, TERM mid-dump, SIGKILL 1 s in (guest resumed) and SIGKILL mid-dump (snapshot adopted) all settle with the memory marker intact and nothing left after release.
Numbers
vm inspect. No timeout, budget or default changed.Coverage
snapshot listfailing after a host-crash-style killKnown issues kept without code
Each is bounded, loud and converges; all are in the hygiene ledger with the reason:
TestRetryResolvesDanglingIntenttakes 5 s of real time (synctest hangs on the settle mutex).boot/Dockerfile: a possibly stale cdn comment with a duplicate mirror entry, and rustfmt/clippy downloaded on every uncached boot build — touching it rebuilds the boot image (decision below).Decisions for CMGS
boot/Dockerfilecleanup (comment, duplicate mirror, toolchain components): costs a boot image rebuild. Recommendation: fold it into the next change that rebuilds the image anyway.