Skip to content

fix: stop the compaction loop that destroys long multimodal sessions - #18

Open
Michael J. Jabbour (michaeljabbour) wants to merge 8 commits into
mainfrom
fix/compaction-token-estimate
Open

fix: stop the compaction loop that destroys long multimodal sessions#18
Michael J. Jabbour (michaeljabbour) wants to merge 8 commits into
mainfrom
fix/compaction-token-estimate

Conversation

@michaeljabbour

@michaeljabbour Michael J. Jabbour (michaeljabbour) commented Aug 19, 2026

Copy link
Copy Markdown

Summary

Four fixes to the compaction loop, from a real incident: session eec9ae98 spent 21 hours working in a project, then could not find that project on disk. The cause was a compaction cycle that ran on 88% of all model calls — 235 times, pinned at maximum strategy — deleting 663 of 686 messages and permanently stubbing the user instructions that carried the project path.

Sibling PRs from the same incident:

901494c — make token estimation content-aware so images stop forcing compaction

Token estimation was sum(len(str(msg)) // 4), which stringifies the whole message dict. A base64 screenshot persisted in the transcript was therefore measured as if it were prose. Two pasted images read as ~2.46M tokens against a 978,720 budget, while the provider's own usage accounting showed the model receiving 9,187–58,428 input tokens.

The old estimate for the first screenshot (1,795,756) matches the observed stuck compaction floor (1,797,300) to within 0.09%. That correspondence is what confirms the right defect was fixed.

Because an image-bearing message was structurally protected from shrinking, the target became unreachable, the predicate never cleared, and compaction re-ran on every subsequent call.

56fecd6 — stub oversized user messages by cost, not by content shape

Stubbing guarded on isinstance(content, str) and returned anything else unchanged, so multimodal (list-shaped) messages were structurally exempt from the only mechanism that could shrink them — while small text-only messages carrying the user's actual instructions were stubbed on all 235 passes.

Protection ran by TYPE when it should run by COST. Non-text blocks are now preserved verbatim while their text is compacted.

ad7936a — do compaction delta arithmetic in the same unit as the baseline

A defect introduced by 901494c itself, found by adversarial review and confirmed by measurement. The estimator fix landed in _estimate_tokens but not in the two hot paths doing delta arithmetic on top of it, which kept the old formula.

Removing one image-bearing message credited 100,031 tokens against a baseline that had counted the whole list at 1,621, driving the running total to −98,410 so the removal loop exited on its first candidate. Silent UNDER-compaction on exactly the conversations the fix was written for.

The reported final_tokens looked correct throughout because it is re-measured independently of the loop it summarizes.

b5a351c — stop chasing a compaction target that arithmetic cannot reach

System messages are never compacted, so once their share alone exceeds the target, no level can reach it. Escalating anyway deletes conversation on every request to chase a number that cannot come down.

Reproduced live on HEAD with a 12,153-token system prompt against a 40,000 budget and no images: level pinned at 8, after_tokens frozen at ~14,776, _removed_seqs ratcheting to 54 of 58 messages, the view sawtoothing as history regrew and was destroyed again — and silently, because the existing over-budget warning was gated on final_tokens > budget while the damage begins at > target_tokens, and that state sits at 37% of budget.

This is a deliberate feasibility PRE-check rather than a stuckness detector with an epsilon and a pass counter:

  • deterministic instead of heuristic
  • fires on the first call instead of after N passes of damage
  • prefix-neutral by construction — it decides whether to escalate, never what the view contains, so prompt-cache prefix stability is untouched

Test plan

  • uv run ruff check . → All checks passed!
  • uv run ruff format --check . → 14 files already formatted
  • uv run pytest -q → 76 passed in 2.18s (baseline 58; 18 new)
  • 18 new tests across four regression files, carrying the incident's real arithmetic so the numbers stay falsifiable:
    • tests/test_multimodal_token_estimate.py
    • tests/test_stub_protection_by_cost.py
    • tests/test_compaction_unit_consistency.py
    • tests/test_infeasible_target_guard.py
  • b5a351c reproduced live on HEAD before the fix (12,153-token system prompt, 40,000 budget, no images) — see commit body

Generated with Amplifier

@michaeljabbour @microsoft-amplifier
fix: make token estimation content-aware so images stop forcing compa…
901494c 
…ction

`_estimate_tokens` was `sum(len(str(msg)) // 4 ...)`, which stringifies
the whole message dict -- so a base64 screenshot persisted in the
transcript was measured as if it were prose. This destroyed a real
session (eec9ae98).

Two pasted images read as ~2.46M tokens against a 978,720 budget, while
the provider's own usage accounting showed the model receiving
9,187-58,428 input tokens. Because an image-bearing message is
structurally protected from shrinking, the compaction target became
unreachable, its predicate never cleared, and it re-ran on 88% of all
model calls -- 235 times, pinned at maximum strategy, deleting 663 of 686
messages and permanently stubbing the user instructions carrying the
project path. The session then could not find the project it had worked
on for hours.

Estimation is now content-aware: it descends into structured content
blocks, counts text at chars/4, and charges non-text blocks a flat
per-block cost rather than their payload length, recursing into
`tool_result` content. On the report's actual first screenshot the
estimate goes from 1,795,756 to 1,612 tokens -- and the old figure matches
the observed stuck compaction floor of 1,797,300 to within 0.09%, which is
what confirms the right defect was fixed.

Also in this commit: `tests/.../_estimate` was a hand-copied "mirror of
SimpleContextManager._estimate_tokens" that went stale the moment
production changed and produced two false failures. It now delegates to
the real estimator.

Verified: ruff clean, ruff format clean, pytest 63 passed (baseline 58;
5 new regression tests carrying the incident's real arithmetic).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: stub oversized user messages by cost, not by content shape
56fecd6 
`_stub_user_message` guarded on `isinstance(content, str)` and returned the
message unchanged for anything else. A message whose content is a LIST of
blocks -- the multimodal shape -- therefore could not be stubbed at all.

In the incident behind this fix, that meant the two largest messages in the
context were structurally exempt from the only mechanism that could shrink
them, while small text-only messages carrying the user's actual instructions
were stubbed on all 235 compaction passes. Protection ran by TYPE when it
should run by COST: the user's project path was destroyed while two
multi-megabyte messages sat untouched.

The fix handles both content shapes. For block content the text blocks are
compacted into a single stub block and non-text blocks are preserved
verbatim -- those are counted at a flat cost by the estimator, so dropping
one buys almost nothing and loses the attachment. The 50-character preview
logic is now shared by both paths via a small `_stub_text` helper.
Unrecognised content shapes pass through untouched rather than being guessed
at.

Six tests in tests/test_stub_protection_by_cost.py cover the string path,
short strings, block content with long text (the defect), attachment
survival, block content with only a short caption, and unknown shapes.

Verified: ruff check clean, ruff format clean, 69 passed (was 63).

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: do compaction delta arithmetic in the same unit as the baseline
ad7936a 
901494c made token estimation content-aware so a base64 image payload would
stop being measured as prose. That fix landed in `_estimate_tokens` but not in
the two hot paths that do delta arithmetic on top of it, which kept the old
`len(str(msg)) // 4`:

- `_remove_messages_with_protection` seeded its running total from the new
  content-aware estimator but computed per-message deltas with the old formula
- `_truncate_tool_wave` did the same on its before/after pair

Measured on a conversation with one image-bearing message:

    baseline (content-aware, whole list) :     1,621
    per-message delta (old formula)      :   100,031   <- what the loop subtracted
    running total after one removal      :   -98,410   <- hard negative

The removal loop exits as soon as `current_tokens <= target_tokens`, so a
single image-bearing removal drove the total negative and the loop stopped on
its first candidate. Compaction silently UNDER-shot on exactly the
conversations the estimator fix was written for -- the opposite failure to the
one being fixed. And `final_tokens` is honestly re-measured at the end, so the
reported stats looked correct while the loop that produced them had been
running on garbage.

Both sites now use `_estimate_message_tokens`, so deltas and baseline come from
one estimator by construction. The arithmetic closes exactly: the whole-list
estimate equals the sum of the per-message estimates, and removing any message
leaves a non-negative total that matches a fresh estimate of the shortened list.

Three new tests pin the invariant rather than the incident: per-message figures
must sum to the whole-list figure, removing any message must leave the running
total sane and equal to a fresh re-estimate, and truncation's before/after delta
must predict the re-measured total.

901494c was tested and green -- its tests exercised the estimator, not the
mixed-unit delta paths built on top of it. A measurement fix that leaves
arithmetic-on-measurements behind is only half a fix.

Verified: ruff clean, ruff format clean, 72 passed (was 69).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: stop chasing a compaction target that arithmetic cannot reach
b5a351c 
System messages are never compacted. Once their share alone exceeds the
compaction target, no escalation level can reach that target -- the predicate
never clears, so compaction re-decides on every request, pinned at maximum
level, deleting real conversation to chase a number that cannot come down.

Reproduced on this module with a 12,153-token system prompt against a 40,000
budget (target 10,000) and no images anywhere, so this is not the
image-estimation bug fixed earlier on this branch:

    call  level  after_tokens  removed  view
       5      8        14,475        6     4
      13      8        14,776       18     8
      29      8        14,776       54     4   <- 54 of 58 messages ever added

`after_tokens` never moved. The returned view sawtoothed as history regrew and
was destroyed again. And it was silent: the existing over-budget warning was
gated on `final_tokens > budget`, while this state sits at 37% of budget.

Two changes:

1. Feasibility pre-check before the escalation ladder. If the system share
   exceeds the target AND the view still fits the ACTUAL budget, do not
   escalate -- return the already-decided sticky view and log once, naming the
   knob that actually moves. Over budget, escalation proceeds as before,
   because a partial reduction beats none.

2. The over-budget warning gate now fires on the destructive condition, not
   only when over budget.

This is deliberately a feasibility PRE-check rather than the stuckness detector
with an epsilon and a pass counter that was originally proposed. It is
deterministic rather than heuristic, it fires on the first call instead of
after N passes of damage, and it is prefix-neutral by construction -- it
decides whether to escalate, never what the returned view contains, so
prompt-cache prefix stability is untouched.

Effect on the repro: removals drop from 54 of 58 to 42 of 58, and every
remaining removal is legitimate work to get back inside the real budget rather
than chasing an unreachable target. Two clear warnings replace total silence.

Four new tests: an unreachable target does not destroy a context that fits, the
warning is emitted exactly once and names the knob, going genuinely over budget
still escalates, and a reachable target is completely unaffected.

Verified: ruff clean, ruff format clean, 76 passed (was 72).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: stop deleting every eligible message when the target is unreachable
f8853d3 
`_remove_messages_with_protection` exits its removal loop when
`current_tokens <= target_tokens`. When the target is unreachable that
condition never becomes true, so the loop runs to exhaustion and removes every
eligible candidate -- permanently, because `_removed_seqs` is re-applied on
every later rebuild -- for no gain at all.

The previous commit on this branch (b5a351c) guards one way the target can be
unreachable: the system-message floor. It is not the only way. The last user
message and the last `protected_tool_results` tool results are equally
un-compactable. One `read_file` on a large file puts an enormous tool result
inside that protected window and arms this -- with a 16-token system prompt and
no images anywhere, so the existing guard cannot fire.

Measured on this branch, before the fix:

    call  hist  view   tokens  removed  lvl
       1    64     6   35,273       58    8
       2    66     5      229       61    8   <- 229 tokens against a 40,000 budget

A 64-message conversation collapsed to 6 on the FIRST call, at maximum level,
with 58 messages deleted permanently. The condition causing it is TRANSIENT --
the blob leaves the protected tool window within a few turns and becomes
truncatable -- while the removals are not.

After:

       1    64    62   39,777        2
       2    66    62   39,745        4

Permanent removals across ten calls drop from 61 to 32, and the view never
collapses.

The fix threads the real `budget` into the function (three call sites, levels
3, 5 and 7) and, before the loop, computes the achievable floor: if removing
every eligible candidate still cannot reach the target, aim at the budget
instead. `target_usage` is a hysteresis preference; fitting the budget is the
contract. If even the budget is unreachable this changes nothing and the loop
behaves exactly as before.

Same shape as b5a351c -- decide WHETHER TO KEEP GOING, never WHAT THE VIEW
CONTAINS -- applied one layer down, so it is prefix-neutral by construction.

Five new tests: one large tool result must not delete the conversation; the
view must never fall to a tenth of the budget; removals stay bounded across ten
calls; a reachable target is still pursued (the clamp must not become an excuse
to stop compacting); and `budget=0` leaves the old behaviour exactly as it was.

Verified: ruff clean, ruff format clean, 81 passed (was 76).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: stop the stub call sites re-imposing the shape exemption
fe6508e 
`56fecd6` made `_stub_user_message` shape-agnostic so a message whose content
is a list of blocks could be compacted, with non-text blocks preserved. Both
callers -- the Level 8 first-user-message site and the stub-candidate loop --
kept an `isinstance(content, str)` guard, so that branch of the helper was
never reachable from production. The test added with `56fecd6` exercised the
helper in isolation, so the suite stayed green while the behaviour was
unchanged: the fix was live in the unit under test and dead in the code path.

Both sites also derived savings as `(len(content) - 70) // 4`. On block content
`len()` is a BLOCK COUNT, not a character count, so lifting the guard without
fixing the arithmetic would have subtracted a large negative number from the
running token total -- inflating it, and driving further compaction. Same class
as `ad7936a` on this branch, where the per-message deltas and the baseline
disagreed.

Both sites now defer to the helper, which already returns the message unchanged
when there is nothing worth compacting, and measure savings as
`_estimate_message_tokens` before minus after.

SCOPE -- this is a latent-defect fix. An adversarial review reported a measured
end-to-end difference between the two content shapes. Reproducing that was
attempted four times and failed: in every fixture built, `stub_candidates`
excludes the first and last user message at levels 1-7, and removal reached the
target before the stub stage ran at all, so total stubbed was 0 for both shapes
-- before and after this change. The inconsistency and the unit error are real
and are verified by the new tests; a user-visible symptom was NOT demonstrated.
The reviewer's numbers are not confirmed here.

Five new tests in tests/test_stub_call_sites.py pin the call sites directly
rather than driving them through the level ladder: both content shapes are
stubbable from the call site (parametrized), a stubbed block message actually
gets smaller, the reported post-compaction token figure agrees with a fresh
estimate of the returned view (the units assertion), and a short block message
is left alone -- deferring to the helper does not mean stubbing everything in
sight. An earlier draft of these tests asserted on `kept[2]` and was silently
reading whichever message happened to land at that index after removals shifted
them; they now locate the subject by an explicit marker.

Verified: ruff check clean, ruff format clean, 86 passed (was 81).

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
fix: stop compaction escalating forever when it cannot reduce the result
4baf87b 
The incident ran 235 compactions across 266 model calls -- 88% of every
request preceded by a full compaction, all pinned at maximum strategy, every
one finishing at roughly 1.84x the budget. Nothing anywhere counted the
repetition. The only signal was an INFO line that fired 235 times and nobody
saw.

Add a breaker that stops escalation after 10 consecutive ineffective passes,
logs one ERROR naming the knobs that actually move (system prompt size, token
budget, any single message larger than the target), and returns the sticky
view already decided. Freezing is prefix-safe by construction -- re-applying
the existing decisions is strictly more stable for prompt caching than
re-deriving them. Same shape as the two guards already on this branch: decide
*whether* to escalate, never *what the view contains*.

Defining "ineffective" took three attempts, and every wrong definition was
caught by an existing test on this branch,
`test_going_over_budget_still_escalates`, rather than by reasoning:

1. "escalated N times in a row" punishes a session that is genuinely over
   budget and must compact on every call. Under it the view grew to 61,190
   tokens against a 40,000 budget: the breaker converted "destroying
   conversation" into "guaranteed provider rejection", which is worse than the
   bug.

2. "did not reduce versus the previous pass" punishes compaction that is
   correctly holding the line while new turns arrive. A result that plateaus
   just under budget is compaction working, not failing. This one still failed
   the same test, at 58,601 tokens.

3. "finished still over the real budget" -- shipped. The view being returned
   will not fit, so the pass did not accomplish the one thing it exists to do.
   Matches the incident (1.84x budget, 235 times) and clears a healthy session.

The counter also re-arms on the no-compaction-needed path in
get_messages_for_request, not only inside _compact_ephemeral. Compaction not
being needed at all is the strongest evidence pressure is relieved, and leaving
a stale count there could trip the breaker on an unrelated later burst.

Six new tests: the breaker trips when every pass lands over budget; it says so
exactly once (235 identical lines is why nobody saw the incident); a session
that compacts successfully never trips; one successful pass re-arms it; the
count is reported in stats for observability; and tripping freezes rather than
re-derives -- asserting _removed_seqs and _sticky_level are unchanged after the
trip, which is the prefix-safety claim.

Verified: ruff clean, ruff format clean, 92 passed (was 86).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@michaeljabbour @microsoft-amplifier
refactor: record why compaction removed each message, not just that i…
c12c87b 
…t did

`_removed_seqs` was a bare `set[int]`. It could answer "was this message
removed?" but never "why". Every diagnosis during this investigation had to
reconstruct intent from logs that no longer existed -- the incident's 663
removals arrived as a single number with no story attached.

It is now `dict[int, str]`, mapping seq to a short reason recorded at the point
of decision: which strategy level made the call, and what target it was chasing.
Membership tests, `len()`, and iteration are identical on a dict, and nothing in
the compaction path reads the reason. This is prefix-neutral by construction --
it cannot change any decision, only explain one after the fact.

Two adversarial reviewers independently landed on this as worth doing on its own
merits, and it is the prerequisite for any future work on reversibility: you
cannot decide whether a removal should be undone until you know what condition
caused it and whether that condition still holds.

One test needed updating, and the reason is worth stating.
`test_tripping_freezes_rather_than_re_deriving` captured
`set(context._removed_seqs)` and then compared the live `_removed_seqs` against
it. With a dict on one side and a set on the other, that comparison is always
False -- so had the invariant actually broken, the test would have failed open,
not closed. It now compares key sets explicitly, which is what the invariant
("no new removals after the breaker trips") actually means. The reason strings
are deliberately not part of it: they are diagnostics, not contract.

Verified: ruff clean, ruff format clean, 92 passed (unchanged -- this commit
adds diagnostics, not behaviour).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Michael J. Jabbour (michaeljabbour) added a commit to michaeljabbour/amplifier-runtime that referenced this pull request Aug 19, 2026
@michaeljabbour @microsoft-amplifier
fix: keep a pasted screenshot from being rewritten into the transcrip…
7955aea 
…t forever

`ClipboardImageInjector` writes a multimodal message into the stored context and
`_save_transcript` serialized its base64 straight into `transcript.jsonl`. That
is the storage enabler behind the session `eec9ae98` loss: the token estimator
bug that turned two screenshots into a 235-pass compaction spiral is fixed
separately (microsoft/amplifier-module-context-simple#18), but the bytes on disk
are this repo's problem.

Measured on real sessions on this machine before the fix -- 7 of 5,323
transcripts carry `"type": "base64"`, and the worst is not close:

    transcript.jsonl   3,113,952 base64 chars in ONE message   (4.9 MB file)
    metadata           {"source": "tui-clipboard", "attachment_count": 1}

`source: "tui-clipboard"` is the exact stamp `build_image_message` writes, so
that is a causal fingerprint rather than a coincidence. Two costs the estimator
fix does nothing about: `_write_with_backup` keeps a `.backup` copy, so it is
~10 MB on disk per screenshot rather than 5; and `IncrementalSaver` fires on
every `tool:post`, so that file is fully re-serialized, rewritten and fsynced
hundreds of times in a session.

The fix externalizes at the persistence sink. `_save_transcript` writes image
bytes to content-addressed blobs under `sessions/<id>/blobs/sha256-<hex>.<ext>`
and stores a `{"type":"ref","id":"sha256:..."}` reference; `_load_transcript`
rehydrates them back to inline base64. The in-memory shape is byte-identical, so
the injector, orchestrator, estimator, rewind and provider all see exactly what
they saw before. This is a storage change, not a behaviour change.

Three deliberate choices:

- Blobs are written BEFORE the transcript that references them, so a crash
  between the two leaves an orphan blob rather than a dangling reference.
- A blob that cannot be written falls back to inline base64. Degraded storage is
  recoverable; a silently dropped attachment is not -- and rewind reads
  attachments back out of stored history.
- A missing blob on read leaves the reference in place rather than raising, so a
  session copied without its `blobs/` directory degrades instead of failing to
  load.

Scope, stated plainly: this fixes disk and I/O, not the in-memory multiplier.
The orchestrator sources messages exclusively from `get_messages_for_request()`,
so resolving references lazily would have to land in the context module. That is
deliberately not attempted here.

Six new tests, written to fail first -- three of them did, at 2.8 million
characters. A pasted image does not land in the transcript verbatim (the bound
this repo accepts, written down where it can be checked); the save/load round
trip is byte-identical; several images in one message all survive; identical
images are stored once; a transcript written before this change still loads, so
migration is "do nothing"; and ordinary text messages are untouched, creating no
`blobs/` directory at all. Zero tests touched this path before this file, which
is the condition that let a 21-hour session destroy itself.

Verified: ruff clean, ruff format clean, pyright 0 errors, 301 passed (was 295).
`persistence.py` is inside the `kernel/` tree that `amplifier-app-tui`
`__path__`-shims to execute from this distribution, so per AGENTS.md the client
suite ran against this working tree (not the pinned SHA): 4915 passed,
13 skipped.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@michaeljabbour