Skip to content

fix(merge): merge around quarantined sources; add operator merge holds - #117

Open
ian-de-marcellus wants to merge 2 commits into
anima-research:mainfrom
ian-de-marcellus:fix/merge-around-quarantine
Open

ian-de-marcellus wants to merge 2 commits into
anima-research:mainfrom
ian-de-marcellus:fix/merge-around-quarantine

Conversation

@ian-de-marcellus

@ian-de-marcellus ian-de-marcellus commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A single quarantined merge can freeze the whole summary pyramid.

contiguousMergeCandidates always offers the oldest contiguous run of unmerged summaries. If that exact source set is in the merge quarantine, enqueueMerge declines 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. getCompressionDebt reported degraded the whole time, but nothing pointed at the scheduler.

Fix

  • Sources of every quarantined merge are removed from candidacy before contiguity is computed, so they split the frontier into runs like any other hole. Later history merges around them, and the existing interior-run rule consolidates stranded runs of ≥2.
  • New 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 sweepPaidOffMergeQuarantine behaves as before.

Tests

merge-contiguity.test.ts:

  • a quarantined oldest group no longer blocks the next contiguous run;
  • an operator hold splits the frontier like a hole (the interior run consolidates, the held id is never offered).

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

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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 apply threshold only to the newest run.
  2. 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. With quarantineAlarmIntervalMs: 0 they last forever. Fix: also run sweepPaidOffMergeQuarantine() once after load/repair.
  3. Diagnostics. (Sol) getCompressionDebt reports mergeQuarantineCount but 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 + 1 after filtering, and the merged range comes from contiguous sources (:7465-7469).
  • Both new tests fail on main (they select q0..q5 and h0..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).

ian-de-marcellus and others added 2 commits September 25, 2026 17:41
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>
@ian-de-marcellus

Copy link
Copy Markdown
Contributor Author

Thanks. The adaptive-path finding was the important one. Addressed in d82affa, with the branch rebased onto current main (d1d17b0).

1. One eligibility gate. enqueueMerge now refuses any group containing a held or quarantined source, not just an exact quarantined set. enqueueMergeForRange splits its candidates around such sources, and sanitizePersistedMergeQueue drops entries that contain one. On your question: yes, the freeze persisted on adaptive stores. Checked on the previous head: three produce requests over a range whose oldest six L1s were quarantined each asked for exactly that group, were hash-declined, and left the queue empty, so the newer six never merged. With the gate they merge. I can't reconstruct which scheduler the offline 47-merge run went through, so I'd rather lean on that test than on the claim in the body. I've corrected the body.

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 threshold summaries are eligible overall ([h0, h1, H, h3, h4, h5] → [h0, h1]). Threshold-1 callers are unaffected.

4. Paid-off quarantine records are swept at load. The test uses quarantineAlarmIntervalMs: 0 and no tick.

5. getCompressionDebt() now reports mergeQuarantineKeys and mergeHeldIds. Holds don't change state, since they're deliberate, but they're visible.

Six new tests, all failing on the previous head. Full suite: 854/854 (node 22).

🤖 Generated with Claude Code

ian-de-marcellus added a commit to ian-de-marcellus/context-manager that referenced this pull request Sep 25, 2026
…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>
@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5 Tier: apex

[High risk] Changes merge scheduling logic and adds operator-controlled merge holds.

The PR should not merge until adaptive range selection cannot form a parent across a held summary.

Findings

  1. P1 Held summary loses its position ▶
Fix with agent prompt
### Issue 1
src/strategies/autobiographical.ts:4070
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.

Summary

This PR makes merge scheduling work around quarantined sources, adds operator summary holds, sweeps stale quarantine on load, and exposes hold and quarantine information in compression debt.

  • Threshold and adaptive schedulers now exclude held sources from merge groups.
  • New tests cover candidate selection, enqueue gating, persisted-queue filtering, and load-time sweeping.

Reviews (1) · Last reviewed commit: "fix(merge): one eligibility gate for eve..."

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants