fix(merge): merge around quarantined sources; add operator merge holds - #117
ian-de-marcellus wants to merge 2 commits into
Conversation
Anarchid
left a comment
There was a problem hiding this comment.
Verdict: the quarantine fix is good; the merge-hold option needs rework or its own PR
Reviewed at e5667c8 against main eca70da (0.11.0).
Removing quarantined sources before contiguity is computed is the right repair for the freeze, and it can't produce a merge that spans a hole. But mergeHoldSummaryIds is honoured by only one of the two ways merges get scheduled, and nothing in connectome-host can set it. Either finish it or split it out; shipping an operator control that fails silently is worse than not having one.
Major
1. The adaptive produce path ignores holds, and so do persisted queues. (Sol) The hold filter lives only in contiguousMergeCandidates (autobiographical.ts:6656-6670). enqueueMergeForRange (:4048-4063) picks sources.slice(0, N) from in-range unmerged summaries without checking holds, and that's what handleProducedOps calls from the live pick and the production shadow pick (:7919-7940). Persisted-queue validation (:3701-3724) doesn't check holds either, and enqueueMerge (:3681) rejects only an exact quarantine-set hash. So on adaptive stores (kv-stable / kv-unified), a held summary still gets merged whenever the picker asks for that range.
Plausible but not verified: the same path can re-request the exact quarantined set, which enqueueMerge would hash-decline silently. In that case the freeze this PR fixes may still exist on adaptive stores. Could the body say which scheduler the offline 47-merge simulation exercised?
Fix: enforce eligibility in one place, inside enqueueMerge: reject any group containing a held or quarantined id. Split enqueueMergeForRange candidates around held ids, drop persisted queue entries that contain one, and add a handleProducedOps test.
2. mergeHoldSummaryIds can't be set from connectome-host. buildFrameworkStrategy copies only allowlisted keys (framework-strategy.ts:11-55 on connectome-host main c5f1389), and RecipeStrategy has no field for it (recipe.ts:22-113). If an operator adds it to a recipe, it's dropped without a word. Fix: a companion connectome-host PR adding the recipe field, validation (array of non-empty ids) and a PASSTHROUGH_KEYS entry. Or leave the option out of this PR and land the quarantine fix alone.
Minor
- A held source can hide a stranded run for a while. (Sol)
if (unmerged.length < threshold) return null(:6670) runs before runs are built. With threshold 6 and[h0, h1, H, h3, h4, h5], five ids are eligible, so the function returns null and[h0, h1]waits until a sixth eligible summary exists. The guard already exists on main (:6648), so this is a delay rather than a freeze. But the fix relies on the interior-run rule, and holds make the guard fire more often. Fix: build runs first, and applythresholdonly to the newest run. - A stale quarantine record now holds its remaining sources until the next sweep. (Sol) On main, a stale record could block only its own exact set; now it holds every source. The sweep runs at the top of
tick()via the alarm, and AF ticks only when!isReady(), so stale holds last through idle periods. WithquarantineAlarmIntervalMs: 0they last forever. Fix: also runsweepPaidOffMergeQuarantine()once after load/repair. - Diagnostics. (Sol)
getCompressionDebtreportsmergeQuarantineCountbut no held ids and no quarantine keys, and a store with only holds reports healthy. That's the same "degraded, and nothing points at the scheduler" blind spot the body describes.
Verified fine
- There's no merge across a hole: runs split on
x.first !== runEnd + 1after filtering, and the merged range comes from contiguous sources (:7465-7469). - Both new tests fail on main (they select
q0..q5andh0..h5) and pass here. - The quarantine itself,
sweepPaidOffMergeQuarantine, and manual clears are unchanged, and nothing is mutated in place. - main + #114–#118 merged together builds and passes 852/852 (node 22).
Method
Two reviewers in parallel. Sol (Codex gpt-5.6-sol, xhigh, read-only, no network; ~10 min, 112 commands, 3.5M input tokens (3.3M cached), 25k output) and Claude (the connectome-host wiring, the combined-tree build and run, and a check of every Sol citation). Both: #2. Sol only: #1, #3, #4, #5. Sol rated #3 and #4 major; they were lowered after checking main (the guard predates this PR) and AF's tick gating (the sweep runs every tick).
contiguousMergeCandidates always offered the oldest contiguous run. When that exact group was quarantined, enqueueMerge silently declined it on every threshold pass and no later group was ever considered: one refused L2 merge left 322 newer L1s unmerged for 16 days with zero merge calls. Sources of a quarantined merge, plus new operator holds (mergeHoldSummaryIds), are now removed before contiguity is computed, so they split the frontier like a hole and later history merges around them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rantine at load Review follow-up (anima-research#117): - enqueueMerge refuses any group containing a held or quarantined source (was: only an exact quarantined set). enqueueMergeForRange splits candidates around them; on the previous head the adaptive path re-requested the exact quarantined group every pick and enqueued nothing (the freeze persisted on kv-stable/kv-unified stores). - sanitizePersistedMergeQueue drops entries containing a held/quarantined id. - Sweep paid-off merge quarantine at load (alarm may never fire). - contiguousMergeCandidates: drop the total-count guard that ran before runs, so interior runs consolidate at 2 as documented. - getCompressionDebt reports mergeQuarantineKeys and mergeHeldIds. - Tests for each; all six fail on the previous head. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
e5667c8 to
d82affa
Compare
|
Thanks. The adaptive-path finding was the important one. Addressed in d82affa, with the branch rebased onto current main (d1d17b0). 1. One eligibility gate. 2. Host wiring: companion PR anima-research/connectome-host#150 adds the recipe field, load-time validation and the passthrough entry. Both of its tests fail on host main. 3. Removed the total-count guard, so an interior run consolidates at 2 even when fewer than 4. Paid-off quarantine records are swept at load. The test uses 5. Six new tests, all failing on the previous head. Full suite: 854/854 (node 22). 🤖 Generated with Claude Code |
…rantine at load Review follow-up (anima-research#117): - enqueueMerge refuses any group containing a held or quarantined source (was: only an exact quarantined set). enqueueMergeForRange splits candidates around them; on the previous head the adaptive path re-requested the exact quarantined group every pick and enqueued nothing (the freeze persisted on kv-stable/kv-unified stores). - sanitizePersistedMergeQueue drops entries containing a held/quarantined id. - Sweep paid-off merge quarantine at load (alarm may never fire). - contiguousMergeCandidates: drop the total-count guard that ran before runs, so interior runs consolidate at 2 as documented. - getCompressionDebt reports mergeQuarantineKeys and mergeHeldIds. - Tests for each; all six fail on the previous head. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
| if (!inRange(s.sourceRange.first) && !inRange(s.sourceRange.last)) continue; | ||
| inRangeCandidates.push(s); | ||
| } | ||
| const pos = (s: SummaryEntry) => messageOrder.get(s.sourceRange.first) ?? Number.MAX_SAFE_INTEGER; |
There was a problem hiding this comment.
Held summary loses its position When a held or quarantined summary’s first source message has been removed but its last remains, the selector admits it by the last message but sorts it after later summaries. It can then enqueue a merge across the held summary.
executeMerge does not check that gap, so the new parent spans history that was meant to remain at its own level.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/strategies/autobiographical.ts
Line: 4070
Comment:
**Held summary loses its position** When a held or quarantined summary’s first source message has been removed but its last remains, the selector admits it by the last message but sorts it after later summaries. It can then enqueue a merge across the held summary. `executeMerge` does not check that gap, so the new parent spans history that was meant to remain at its own level.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Problem
A single quarantined merge can freeze the whole summary pyramid.
contiguousMergeCandidatesalways offers the oldest contiguous run of unmerged summaries. If that exact source set is in the merge quarantine,enqueueMergedeclines it silently. On the next threshold pass the selector offers the same group again, so no later group is ever considered.Observed on a production resident: one L2 merge over its six oldest live L1s was refused 5× and quarantined. For the following 16 days the store made zero merge calls. 322 newer L1s were never attempted, and the 2 live L2s and 2 live L3s could never gather siblings. The context grew until the resident sat at its hard budget with almost nothing folded above L1.
getCompressionDebtreporteddegradedthe whole time, but nothing pointed at the scheduler.Fix
mergeHoldSummaryIds(operator merge holds): listed summaries are held the same way. It's useful for keeping specific summaries at their own level while they await review, without stalling everything after them.The quarantine itself is unchanged: its sources stay unmerged until an operator clears it, and
sweepPaidOffMergeQuarantinebehaves as before.Tests
merge-contiguity.test.ts:Full suite 820/820. Changelog fragments included (
.fixed+.added).Scheduler coverage (revised after review): eligibility is enforced for every scheduler in
enqueueMerge. An earlier version of this description cited an offline simulation of the affected store; I can't confirm which scheduler it exercised, so the evidence is now the tests: on the previous head the adaptive path re-requested the exact quarantined group on every pick and enqueued nothing; with the gate the later group merges (merge-contiguity.test.ts).🤖 Generated with Claude Code