Repository navigation
test(branch): pin tick's branch check after the chunk klaxon's await - #246
Merged
nissa-seru merged 1 commit intoOct 19, 2026
Conversation
tick checks its branch right after the chunk klaxon's await, before the merge klaxon, a line anima-research#138 added, and no other test needs it: with it taken out, this is the only test in the suite that fails. Without it, a host that switches the chronicle directly, without ContextManager, before a tick it didn't await resumes from that await (in the same synchronous run, or a promise continuation of the same turn) lets the merge klaxon's sweep run on the old branch's mirrors and persist that branch's merge quarantine onto the new one. The test in test/branch-lifecycle-regression.test.ts replays that interleaving: on main, a merge-quarantine record whose sources are gone, which the sweep clears and persists; on a fork, a record over two live L1s, which the sweep keeps; then a tick on main that isn't awaited, and a direct switch of the store to the fork. The fork keeps its record and the tick returns with no error. Back on main, main's record is still there, so the switch landed before the merge klaxon, and as a control the same sweep then clears main's record and persists. With the check taken out, the fork's merge quarantine becomes main's and the test fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Required label not found on this PR. |
nissa-seru
pushed a commit
to nissa-seru/context-manager
that referenced
this pull request
Oct 19, 2026
main now holds anima-research#246, anima-research#248 and anima-research#247. anima-research#248 conflicts with this branch in src/strategies/autobiographical.ts, in 2 places, and anima-research#246 and anima-research#247 merge cleanly. - Each conflict keeps main's change, with this.diag where main has console, and this.ns as the last argument of logCompressionCall. - Two stderr lines merged cleanly but named no namespace, so they go through this.diag too: anima-research#247's speculative-drain error from the drain's timer, and anima-research#248's warning that merge discovery after a failed compression attempt failed. One assertion changes for the suffix: anima-research#247's test of a throw from the drain's next tick now expects ns=default at the end of its line (drain-timer). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 task done
nissa-seru
pushed a commit
to ian-de-marcellus/context-manager
that referenced
this pull request
Oct 19, 2026
… retried, and merge around the hole at once Builds on anima-research#117's hole (mergeHeldSourceIds): a quarantined group's sources, and operator holds, are left out of the merge candidates so they split the frontier, and enqueueMerge refuses any group holding one. Holding every record's sources went further than main in four cases, which a probe on anima-research#117 merged with main showed: a topology refusal, or a record whose group is no longer adjacent, kept its sources out of merges until an operator cleared it, and a record with a source gone or parented did the same until the next sweep, where main merged those sources in other groups. A record now holds its sources while its group could be retried as it stands (mergeRecordHolds): its merge failed at the provider, not on topology, every source is present and unparented, and the group is still a valid merge in store order (mergeTopologyProblem). mergeHeldSourceIds gives the operator holds and the sources of the records that hold. - contiguousMergeCandidates keeps main's rule that threshold selection forms no merge at a level below mergeThreshold unmerged summaries, counting held ones, so a hole doesn't keep a level under the threshold; anima-research#117 had dropped that rule for every store, holes of any kind included. It leaves held summaries out after the unresolved-position check, as it leaves out a summary in the wide-span quarantine. A run that ends before a held summary can't grow past it, so it consolidates as an interior run does, at two. A run waits for a retry (anima-research#138's rule for a hole a retry will fill) only while the retry is pending before the first thing it can't grow past: the next run or the first held summary after it. The newest run waits for none unless a held summary follows it. - enqueueMergeForRange leaves held summaries out and takes the longest run of what remains, as it does for any hole in the range (issue anima-research#95), where anima-research#117 took the first run of two or more. - enqueueMerge lets a topology refusal's own group through once it is a valid merge of present, unparented sources, and drops its record. - The merge step skips a queued entry holding a held source, as it skips one whose source has a parent, resolving false as its other skips do; it drops the entry and runs the merge check, since the check left the entry's other sources out while it was queued. That covers a queue from an older version and an entry that becomes held while it waits, so the load's drop of such entries in sanitizePersistedMergeQueue is gone. - quarantineMerge and refuseCrossedMerge run checkMergeThreshold once the record is written, so the runs beside a new hole are offered without waiting for another L1 or merge. - clearMergeQuarantine queues a held group again as it stands, at the queue's tail with a fresh attempt budget, before its merge check: a group short of the threshold, alone at its level, isn't offered by threshold selection alone. An operator hold or another record still holding one of its sources refuses it. A group overlapping one already queued, as two records an older version wrote over overlapping groups can, isn't queued: it would be skipped as stale once the other merged, with no merge check for its other sources then, which the merge check after the clear offers. - scripts/drain-autobiographical.ts passes mergeHoldSummaryIds through from a recipe, as it does the compression options, so an offline drain keeps the operator's holds. anima-research#117's load sweep stays: a stale record holds nothing now, but getCompressionDebt lists every record's key, and the alarm-driven sweep may not run for a long time. The klaxon's line counts the records that hold. The comments on MergeQuarantineRecord, enqueueMerge, refuseCrossedMerge, the sweep, the klaxon, the merge walk, speculativeCapHolds, tick's check that the head is still its merge, executeMerge, and mergeHoldSummaryIds' option now say the hole as it is, operator holds included. The option's doc says that no summary above a held one forms, where it said held summaries render at their own level, which the adaptive picker doesn't keep them to, and that they count toward maxSpeculativeL1s and a level's mergeThreshold. It and anima-research#117's .added fragment now say that lifting a hold doesn't undo the merges formed around the summary: it can then merge only with an unmerged summary of its level beside it, and one with none beside it stays at its level. clearMergeQuarantine's doc and the fix's fragment say the same of a cleared group: it merges into one summary, which stays below its neighbours' level when nothing beside it was left unmerged. The two changelog fragments for the fix are one, anima-research#117's, rewritten to match; it names the greedy solvers and kv-unified's latent demand as the users of the produce path, where it named kv-stable, whose produce requests are all L1s. anima-research#117's tests: the quarantined-oldest-group test seeds stored L1s, so its record could be retried and holds; the produce-op test expects the longest run past the hold; the test of the load's drop goes with the drop, and the skip at the head is tested in test/merge-quarantine-hole.test.ts; the load sweep's test says a stale record holds nothing, though the debt report lists its key, where it said such a record holds every source. Main's test of tick's branch check after the chunk klaxon (anima-research#246) now writes main's stale record after the load: anima-research#117's sweep at load clears a record already stale then, so the race needs one that goes stale while the store is open, as the klaxon's sweep finds them; with the check taken out it still fails. Tests (test/merge-quarantine-hole.test.ts): the L1s after a quarantined merge merge while its group stays unmerged, on both merge checks, and the clear merges the group; the same under a maxSpeculativeL1s the held L1s and a run short of the threshold pass; the oldest run beside a new hole is offered with no new L1; a run that ends before a trailing hole merges at two, on both merge checks, while a newest run with no held summary after it waits; two records both hold, and clearing one by its key retries its group alone; a record holds nothing once a source is gone, gains a parent, its group is no longer adjacent, or its group is no valid merge, and then its own group is still refused and the clear queues nothing for it; the clear queues the held group before the merge check offers anything else; of two records over overlapping groups, the clear queues the first and not the second, and the merge check groups the second's last source with the L1s after it, which merge; a clear of a record whose group holds an operator hold lifts the record, queues nothing for its group, and the sources after the hold merge; the hole survives a reopen; a persisted entry holding a held source is skipped at the head without a provider call or a landing, and its other sources are offered at once with no new L1; the klaxon's line counts the records that hold, not a topology refusal or a provider failure whose group is no longer adjacent; a topology refusal holds nothing, its sources merge, the clear queues nothing for it, its own group queues again once valid while its record goes for good, and is refused while a source is parented or gone; enqueueMerge refuses a group holding a held source; the L2s after the hole merge into an L3 that leaves out the L2 before it; and a produce op over a range holding the group queues a merge without it; on the same arrivals with the fourth L1 or the fifth held while the L1s around it merge, the fourth, whose neighbours both merged, stays unmerged after the hold lifts and 40 more batches, and the fifth, beside an L1 left unmerged, merges; and the oldest group, quarantined while the history after it merged up to L3, merges after the clear into one L2 over the group that no summary above takes, while the L2 after it is under an L3. merge-disposition's quarantine test now finds the clear's queued retry, with a fresh attempt budget. In test/compression-transient-ladder.test.ts, on anima-research#138's layout of summaries over messages 0-3 and 8-9 with a pending chunk over 4-7: with a held pair after the chunk, the run before it merges at two with no retry pending and waits with one; with a held pair before the chunk, a retry past it doesn't hold the run before it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: a test for
tick's branch check right after the chunk-quarantine klaxon's await, a line #138 added that no test needed. Without that check, a host that switches the chronicle directly before atickit didn't await resumes from that await lets the merge-quarantine klaxon's sweep write the old branch's merge quarantine onto the new branch.What it pins
On
0547be76,tickawaits the chunk-quarantine klaxon (src/strategies/autobiographical.ts:6429), checks that its branch is still current (:6430), and then runs the merge-quarantine klaxon (:6432). That klaxon's sweep (:5997) reads the strategy's in-memory mirrors and persists the merge quarantine throughpersistMergeQuarantine(:5956), with no branch check of its own.setStateJsonwrites to whatever branch the store has selected.So without the check at
:6430, a directstore.switchBranchmade before atick()that isn't awaited resumes from the chunk klaxon, in the same synchronous run or in a promise continuation of the same turn, lets the sweep run on the old branch's mirrors and write its merge quarantine onto the new branch.tickthen throws atcheckMergeThreshold's branch guard, after the write.ContextManager.switchBranchdoesn't do this, sinceinitializereloads the mirrors beforetickresumes, and neither does a switch that arrives as its own task.The test
In
test/branch-lifecycle-regression.test.ts:main, a merge-quarantine record whose sources are gone, which the sweep clears and persists;tick()onmainthat isn't awaited, then a direct switch of the store to the fork;main,main's record is still there, so the switch landed before the merge klaxon;mainthe same sweep then clearsmain's record and persists.What each part is for
:6430taken out, it is the only test that fails: the fork's merge quarantine reads[],main's, instead of['live'].Checks
On Linux, with Node 24.22.0:
Not verified
switchBranchwaits for ticks in flight, and its undo and redo switch from operator actions, which arrive as their own tasks; connectome-host switches throughContextManager.switchBranch. The lock the chunk klaxon awaits is only ever held for synchronous work, so that await ends within the turn. So the window is open only to a caller that switches before atickit didn't await resumes: in the same synchronous run, or in a promise continuation of the same turn.Test-only, so no changelog fragment (CONTRIBUTING).
— Beth-2389 (Claude Opus 5.5). Oscar-1005 (Claude Opus 5.5) placed it, from an internal review's finding on a related change, and an internal review and Ezra-2423 (Claude Opus 5.5) read it. Description checked and posted by Fiona-2757 (Claude Opus 5.5) through the shared
nissa-seruaccount.🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling.