Repository navigation
fix: never build a registry write on a read that failed - #59
Merged
Merged
Conversation
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.
The half of the 18:22 wipe that #58 did not fix
#58 stopped the snapshot (
sandboxes.json) from publishing an observation it could not place. It did nothing for the registry (sandbox-sessions.json), which is the file that actually lost 41 sessions — the wholelinera-agent-09bfb4b7c635key vanished at 18:22:26Z, one minute before the snapshot went empty.Root cause
Both failure modes are indistinguishable from "nothing is recorded", and
replace_sandbox_slicebuilds a whole-file write on top of that read (sandbox_registry.rs:686). So one unreadable read, while any hook saves its own slice, serialises that slice as the entire registry and deletes every other sandbox from disk.The read failing is not itself surprising and cannot be prevented here: the file is rewritten in place by the host collector while in-sandbox hooks read it over virtiofs, where fresh metadata over stale pages is a known hazard on this setup, and half of a 20 KB JSON does not parse. Publishing a write built on that read is the bug.
Change
try_load()keeps the difference between the two cases:Ok(empty)— a first run, and empty is the honest answerErr— we do not know what is recordedload()stays as-is for read-only callers (renderers, tests) and is nowtry_load().unwrap_or_default(). All three read-modify-write paths —replace_sandbox_slice,forget_session,mark_session_departed— usetry_load()?and abandon the write rather than publish one built on a base they never read.Tests
regression_an_unreadable_registry_never_deletes_another_sandboxreplays the incident: two sandboxes recorded, the file truncated to half its bytes (a torn read), then a write for sandbox A. The write must fail, the file must be byte-identical afterwards, and once the file is readable again sandbox B must still be there.Verified it fails without the fix, not merely that it passes with it — with
try_load()?reverted toload()inreplace_sandbox_slicethe assertion trips at the surviving-key check. A test that passes against the unfixed code is how this class of bug keeps shipping, so it was checked both ways.a_registry_that_has_never_been_written_is_legitimately_emptypins the other half: absence is a real answer, and treating a first run as a failure would mean no sandbox could ever record its first session.cargo test725/726. The one failure,perf_do_refresh_io_does_not_explode_with_repeated_calls, fails identically on unmodifiedorigin/mainin this same worktree (a full-suite control run: main 722 passed / 2 failed, branch 725 passed / 1 failed) — it reads the real~/.claude, which on this box holds 36 live sessions with multi-MB transcripts on virtiofs, against a 5 s bound.usage_ledgerhas zero references tosandbox_registry.cargo clippy --all-targets -- -D warningsandcargo fmt --checkclean.The pattern, for the third time today
Idlewhile blocked on the userEvery one is not observed recorded as not there.
live_claude_procs()already states the rule forps— "callers must degrade to their previous liveness source, never to 'no sessions'" — and it wants applying to every reader in this crate, not one.