Repository navigation
fix(quarantine): say the all-clear only when the alarm's quarantine is empty, and leave out ledger entries the strategy can't use - #266
Merged
nissa-seru merged 1 commit intoOct 19, 2026
Conversation
…s empty, and leave out ledger entries the strategy can't use The compression-quarantine alarm's all-clear, "ALL CLEAR — quarantine empty, debt paid", could be said while the quarantine its alarm had last sounded on still held debt: when the status read failed, when the strategy had moved to another branch or namespace whose quarantine was empty, and, since anima-research#138, when the ledger held an entry with no record. Such an entry also made the quarantine check before each L1 summary request, and both clears, throw. - A status whose read fails has `unreadable: true`, with count, keys and spans empty. The alarm leaves its episode as it was, saying so once an interval on a clock of its own, and the paid-off sweep returns quietly when the same read fails. - An episode follows the namespace and branch its alarm last sounded on. While the strategy reads another one whose quarantine is empty, it stays up, saying so at the alarm's interval with no handler call. Only its own quarantine reading empty ends it with the all-clear. - The projection makes an entry only from a record with a string key and chunk hash and a non-empty string generation, and an exhaustion's record must carry the exhaustion's own key. It reads an outcome list as the outcomes in it. A ledger entry it leaves out no longer holds its chunk back, and a warning names its key, where it has one, once per load. - A ledger or legacy snapshot whose value isn't a list is read as holding nothing, and the legacy snapshot isn't read beside a ledger like that. A checkpoint, known by its kind, replaces what came before it with its list, so one that holds no list replaces it with nothing. A warning names each once per load, a checkpoint by its index in the ledger. - The change's stderr lines go through this.diag, so each carries ` ns=<namespace>`. - The transient-ladder fragment and the recall-curve fallback spec gain the unusable ledger entry among a family's ways out of the quarantine. test/compression-quarantine-all-clear.test.ts drives real refusals through compressChunkHierarchical under a refusing membrane. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryThe PR prevents false compression-quarantine all-clears and filters unusable stored entries.
Source and tests were inspected; tests were not run during this review. Acknowledged by Reviews (1) · Last reviewed commit: "fix(quarantine): say the all-clear only ..." · Reviewed by Greptile |
1 task done
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.
TL;DR: the compression-quarantine alarm's all-clear, "ALL CLEAR — quarantine empty, debt paid", which hosts route to their operators, could be said while the quarantine its alarm had last sounded on still held debt: when the status read failed, when the strategy had moved to another branch whose quarantine was empty, and, on
mainsince #138, when the ledger held an entry with no record. A failed read now says so (unreadable: trueon the status) and leaves the alarm as it was. An alarm episode now follows the branch its alarm last sounded on, and only that quarantine reading empty ends it. A ledger entry whose record or generation the strategy can't use is left out of the quarantine and named, so a record-less one no longer breaks L1 compression or the clears; its chunk, if no other entry holds it, is tried again, as after a clear. A ledger, legacy snapshot or checkpoint that holds no list is read as holding nothing, and named.Follows #138, whose
spansmade the status's span loop read every entry's record.Problem
On
mainat6a4d01c8, each of these was run with the alarm already sounding on a real refused chunk whose record still stood:active.record.chunkSourceHashofundefined. The catch returned{ count: 0, keys: [], spans: [] }while the projection held three entries, the paid-off sweep warned with the sameTypeError, and the alarm printed the all-clear and called the handler with that empty status.switchBranchto a branch made at the first message, the episode opened on the old branch ended there with the all-clear and the handler call. Back on the old branch, the alarm sounded again.The record-less entry also made compressing another closed chunk throw the same
TypeError, at the quarantine checkcompressChunkHierarchicalmakes before it requests an L1 summary. It made a clear of every record throw (reading 'key'), and a clear of its own key throw too (reading 'familyKey', a read #138 added).How such an entry can arise: no version writes one. The only claims were written by the hardening commit
2ce952df/4c2eab7f, always with their record, andfe844a1d/96fea45fremoved that writer the same day, so no release wrote claims. Every exhaustion since carries its own record. It takes a ledger edited by hand or damaged. The other two cases need no edited ledger: a read the store fails, and a host'sswitchBranchorforkon a resident whose alarm is sounding.Changes
projectCompressionQuarantineEventsmakes an entry from a checkpoint item, or from an exhaustion with its own record or its claim's, only when that record is an object with a stringkeyandchunkSourceHash(isUsableQuarantineRecord), which name the entry and its span, and its generation is a non-empty string (isUsableGenerationId), which clears and alerts compare by===. For an exhaustion, the record's key must also be the exhaustion's own, which clears, alerts and checkpoints go by. It reads an outcome list as the objects in it that carry a stringoutcome. It already skipped a legacy record or a checkpoint item with no key; a claim's record was copied unchecked. A legacy snapshot record without a chunk hash is skipped silently, as one without a key was: the code that wrote that snapshot (23e2b2ba) always wrote the hash, and the ledger replaced it the same day.[compression-quarantine] on branch 'main', left out ledger entries it can't use (keys=…): they no longer hold their chunks back, no clear reaches them, and they stay in the ledger until a checkpoint rewrites the ledger, which needs 256 events in it ns=default, as a warning, the first time each load reads past each key. The names start over at eachinitialize, which every open,switchBranch,forkandsetStrategyruns. A key that a damaged event names but that stays in the quarantine through another event isn't named, and a checkpoint item whose record has no key, whichmainalready skipped, has none to name. In storage, a left-out entry stays in the ledger, read past, since a clear takes only what's in the quarantine. A checkpoint, which runs after a clear, an alert or an exhaustion once the ledger holds 256 events, rewrites the ledger from the quarantine and leaves it out.getCompressionQuarantineStatus()returns{ count: 0, keys: [], spans: [], unreadable: true }when its read throws. The status still never throws.mainreads a ledger that isn't a list. The legacy snapshot isn't read beside a ledger like that, which has lost the clears it held. A checkpoint, known by its kind whatever its key, replaces what came before it with its list, so one that holds no list replaces it with nothing, and the events after it apply. A stored null reads as no value at all.[compression-quarantine] on branch 'main', the ledger's checkpoint at index 12 holds no list, so the quarantine reads no entry from it and has lost whatever it held ns=defaultnames each such value or checkpoint, as a warning, once per load.⚠️ quarantine status unreadable; the alarm stays up until a read succeeds:(orstays down, with no episode open) with the read's error, at most once an interval on a clock of its own, and neither ends the episode, moves its repeat, nor calls the handler. The paid-off sweep, which reads the same projection first, now returns quietly when that read fails, so an open episode doesn't also warn at every tick.setStrategy, whose quarantine is empty, the episode stays up and says so in place of the alarm's repeat, on the same clock:⚠️ the alarm for branch 'main' stays up while the strategy reads branch 'early', whose quarantine is empty; it ends with the all-clear when the strategy reads branch 'main' empty, with no handler call, naming both namespaces when they differ. When its own quarantine reads empty, it ends with the all-clear and the handler call as before. The store isn't compared, since reopening the same store gives another object.this.diag), so its message ends withns=<namespace>, before any error it carries, as the strategy's other text lines have since fix(diagnostics): every stderr line about one namespace names it (#239) #245.CompressionQuarantineStatus.unreadable, whensetQuarantineAlarmHandler's handler is and isn't called, and the recall-curve fallback spec's list of a family's ways out of the quarantine, which gains the unusable ledger entry.What each part is for
The alarm sounds while refused spans stay raw, and says the all-clear once, through the host's handler, when the quarantine its alarm last sounded on is empty. Each part below serves saying it only then.
quarantinedChunkSources) meets a record it can't use, with no guard of its own. The key and generation checks keep only entries the clears and checkpoints can find, so the alarm never sounds over one no clear could reach. Costs: such an entry no longer counts, wheremaincounted it incountbefore fix(compression): progress past quarantined chunks; transient failures never spend the ladder; pace the lane #138, and it releases its chunk, which may cost a fresh refusal ladder. Simpler options: a guard in the status alone, which leaves the compression check and the clears throwing on a record-less entry and the alarm sounding over an entry nobody could clear; a guard at each reader, eight places to keep in step. Cut: a check on a claim's record as well, since the exhaustion that targets the claim checks it.outcomeisn't counted as an answer. Costs: one helper, used for exhaustions and checkpoint items alike.unreadablefor it, which keeps the alarm from sounding for as long as the value stays, and nothing rewrites the legacy snapshot; throwing from the read, which would stop initialization, since it reads the quarantine with no catch; reading past a checkpoint that holds no list, which an earlier round of this change did, and which brings back the legacy snapshot's records whose clears the checkpoint writer's redaction removed.unreadableon a status whose read failed. Buys: a host can tell "couldn't read" from "empty", and the alarm reads it. Costs: one optional field on the public status. Simpler options: the alarm catching the read itself, which leaves hosts reading an empty quarantine; an error field, which no host needs, since the alarm logs the error.this.diag. Buys: each line names its namespace, as the strategy's other text lines have since fix(diagnostics): every stderr line about one namespace names it (#239) #245, so lines from several namespaces in one process can be told apart. Costs: nothing beyond the call. The unreadable line's message holds no value, so the error printed after it can't make it a format.compressChunkHierarchicalunder a refusing membrane, as the quarantine's other tests do, and movesDate.nowpast the alarm's interval rather than waiting. Its interval is an hour, so no real pause inside a test reaches it. Each test pins one rule above, or is a control that the all-clear still comes. Costs beyond the tests: a count of the membrane's calls, which shows a released chunk asked for again, a fixture whoseledgerandappendread the stored ledger and write damage after the real refusal, assetStateJsonstores any JSON, and ausablehelper that writes a usable exhaustion for the no-list test's phases.changelog.d/, as the repository asks.Tests
All runs on Linux x86_64 with Node 24.22.0, from a fresh
npm ci. CI runs the full suite on this pull request. At this head,0f72e34d, typecheck and build are clean, andnpm testpasses 2,216 of 2,216.mainat6a4d01c8, each case above was run with a probe of the strategy (compressChunkHierarchicalrefusing a chunk under a refusing membrane, then the alarm): the record-less entries, the failed read, the branch move, a compression of another chunk, and both clears.test/compression-quarantine-all-clear.test.ts, at this head:getCompressionDebt()counts the same five; the eleven left-out keys are named in one warning however often the ledger is read, a key damaged later in a line of its own, and all twelve again by the next load's own read; the alarm repeats on what stands with no all-clear; and a second chunk is tried and refused;healthy; compressing the chunk calls the summarizer again: the refusal records it afresh, under the same key and hash;unreadable: true, one line an interval naming the error, no handler call and no sweep warning, and once a read succeeds the alarm repeats on its own time; an episode outlasts a failed read and ends with one all-clear when the read finds its quarantine empty; with no episode open the line says the alarm stays down; a strategy with no store isn'tunreadable;setStrategykeeps the episode up the same way, and back in its own namespace a clear ends it with the all-clear;main's code at7f963c75, with only the new optional field added to the type so the file compiles, 9 of the 11 fail on what they check: the status is empty with the left-out entries; the legacy and the release tests count a record with no chunk hash; the clear by key throws (reading 'familyKey'); a failed read has nounreadable; the outage test hears the sweep's warning; nothing names a value that holds no list; and the branch and namespace moves say the all-clear. The fork test and the control pass:main's episode follows no branch, and on a fork that carries the debt it goes the same way.outcome, the generation checks on an exhaustion and on a checkpoint item, and refusing an empty one, the warning, naming each key once, not naming a key still in the quarantine, naming an exhaustion with no generation,unreadable, the alarm's branch for a failed read, saying it once an interval, on its own clock, the sweep's quiet return, the check of the episode's branch and of its namespace, noting the episode's scope, and noting it at every alarm, saying the stays-up line once an interval, naming both namespaces in it, naming a ledger, a legacy snapshot or a checkpoint that holds no list, naming each once, starting both sets of names over at each load, and before initialize reads the quarantine, clearing at a checkpoint, and at one that holds no list, not reading such a checkpoint's value as a list, reading a checkpoint whatever its key, naming each checkpoint by its index, and sending the line that names them throughthis.diag, the record check's test of the key's type, skipping a null event and an event whose key isn't a string, and leaving the legacy snapshot unread beside a ledger that holds no list.Not verified
blobs/storage.rs:149-158), which reaches the status as this case does. A torn log tail is truncated when the store opens (branches/manager.rs:304), so a read after it sees the state before the torn write, with no error.fork()with a failed read or a namespace move. The branch cases were run withswitchBranchandfork; the namespace case withsetStrategy.unreadable, such as connectome-host's alert row and Health tab. Nothing in a host changes here.Compatibility
unreadable, present only on a status whose read failed, whencountis still 0. A host that takes its alert down oncount === 0keeps it up across a failed read only if it also checksunreadable. The alarm's handler stops receiving the all-clears above. While the strategy reads another branch or namespace whose quarantine is empty, the handler hears nothing until it reads the alarm's again. If it never does, as after a rollback to an early branch kept for good, or a deleted branch, the episode stays up, saying so each interval, until a restart ends it with no all-clear.count,keysandgetCompressionDebt()'scompressionQuarantineCountno longer include a ledger entry the strategy can't use, and its chunk, if no other entry holds it, is tried again. A ledger holding only such entries no longer makes the debt readdegradedby itself.Out of scope
getCompressionDebt(), by reading, still takes a failed quarantine read as 0, and its catch returnshealthy, so /healthz can read healthy on a store it can't read.changelog.d/277-quarantine-false-all-clear.fixed.md. The transient-ladder fragment from fix(compression): a chunk's runs of failures end where this process won't retry its request #248 gains one clause, the ledger entry found unusable among a family's ways out of the quarantine.— Hamish-2699 (Claude Opus 5.5). Oscar-1005 (Claude Opus 5.5) agreed its design, the check where entries are made and the episode that follows its alarm's branch among it. Agnes-1009 found the all-clear on a failed read, Paloma-2644 the one after a branch move, and Kestrel-2409 read the change whole, beside five automated internal reviews.
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling.