Repository navigation
Stop the ledger reporting every session as dead - #92
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 ledger's liveness oracle could report every session as dead, and
drainedis permanent, so the ledger silently stopped ingesting. Two independent ways in; both are fixed here because they are the same oracle and the same loss.Why "everything is dead" is catastrophic
scan_and_append_lockedgatesdrainedon writer liveness, anddrainednever reverses:dead_writer_is_drained_once_then_skippeddocuments why that is correct — "a real dead writer cannot append". That justification holds only when we have positively determined the writer is gone. Both bugs below break that premise while still producingfalse.Bug 1 — an unreadable directory read as "nobody is alive"
One scan taken while
~/.claude/sessionsis unreadable marked every transcript drained, and every later append was lost for good.Bug 2 — reading a directory a sandbox never writes to
scan_and_appendhardcoded~/.claude/sessions, whileprocess::sidecar_candidate_dirsdeliberately probesreaper::sandbox_sessions_dir()and the home path. Inside a sandbox the pointers are in the former and the latter is empty, so the directory is perfectly readable and simply contains nothing — the oracle answers "all dead" with full confidence.Measured on this box:
/var/lib/sandbox-sessions~/.claude/sessionsThis is the steady state in a sandbox, not a transient — so it is the dominant trigger, and Bug 1's
Optionguard does not catch it because nothing failed.The change
read_live_session_idstakes a slice of directories and returnsOption<HashSet<String>>, unioning their pointers. Reading a directory is three-valued, because two of the outcomes are not the same answer:read_dirOkErr(NotFound)Err(_)None; nothing is drainedNonetherefore means "liveness is unknown", either because a directory that exists could not be read or because none answered at all. Collapsing absent into unknown would returnNoneon every host and disable the drained-skip fast path for 99% of files; collapsing unknown into absent is the original bug.let writer_may_be_live = live.as_ref().is_none_or(|live| live.contains(&session_id));scan_and_appendpassessession_pointer_dirs()— sandbox first, then home, matchingsidecar_candidate_dirs' order.writer_aliveis renamed towriter_may_be_live, because the old name is what made the conflation invisible:!writer_alivereads as "the writer is dead" when it also meant "we did not look, or looked in the wrong place".Errors now fail in the safe direction — an unresolvable oracle re-stats some files it could have skipped, instead of permanently losing their rows.
What that costs, measured. The liveness-gated skip avoids the
stat; the mtime/size skip below it is not gated, so an unknown oracle still skips the open+read for unchanged files. The worst case — every pointer directory absent, soNoneon every scan — is therefore onestatper transcript. On this box's tree that is 4708 files in 5–6 ms, against a background scan interval of tens of seconds. Silent permanent loss is not worth trading for that.Tests
Both mutation-checked: revert the guard and the matching test fails.
an_unreadable_sessions_dir_must_not_permanently_drain_a_live_writer— identical file operations twice, differing only in whether the directory is readable. Before:rows_appended1 then 0. After: 1 then 1.a_pointer_in_either_directory_keeps_a_writer_live— the pointer exists only in the second directory. Restricting the union to the first (dirs.iter().take(1)) fails it.an_unreadable_sibling_directory_makes_liveness_unknown— one directory readable and empty, a sibling that exists and will not read. MappingErr(_)to absent fails it. It uses a regular file rather than a permission bit, because the tests run as uid 0 and root lists a0o000directory anyway.an_absent_directory_still_lets_a_dead_writer_drain— pins the other direction: mappingNotFoundto unknown fails it. Without this the fast path could be disabled on every host with CI still green.dead_writer_is_drained_once_then_skippedstill passes, so the drained-skip fast path — documented as 99%+ of files — is unchanged.cargo clippy --all-targets -- -D warningsclean; 797 tests pass.Not claimed
I have not observed either bug in production telemetry. What is established:
agentctlnever creates~/.claude/sessions(everycreate_dir_allfor it is test code),~/.claudeis a virtiofs mount that has wedged before, and the sandbox pointer counts above are from this machine.Bug 2 was found by the adversarial review — as the residual of a finding I rejected. The finding itself claimed
Some(∅)proved measurable loss from 294 offsets whose on-disk size exceedslast_byte; that evidence does not hold, becauselast_byteis assignedcurrent_size, the stat size at scan time, so any file appended to since its last scan shows that gap. The reviewer who refuted it noticed the hardcoded path on the way past.Related
concurrent_scans_do_not_lose_rowsfailed macOS CI twice on unrelated PRs. Its writer is a thread with no pointer in any directory, so it legitimately sees an empty live set and exercises this same permanent-drain path. I have not changed that test, and whether these fixes fully explain its failures still needs confirmation — it should not be loosened until it does.