fix(compression): stop an aborted rotation from growing the parent it could not publish - #88227
Closed
jackulau wants to merge 1 commit into
Closed
Conversation
… could not publish The rotation path flushes its un-persisted transcript to the parent (NousResearch#47202) and only then calls publish_compression_child. The abort handler rolls back the in-memory transcript and keeps agent.session_id on the parent - its own comment says "keep the parent live and discard the stale compacted snapshot" - but the rows the flush just wrote are not part of what it discards. Every failed rotation therefore leaves the parent transcript longer than it found it, whatever the failure was. That is survivable for a one-off failure and pathological for a sticky one. A parent row carrying ended_at fails the publish on every attempt and nothing in this path clears it, so each auto-compaction appends another copy of the current turn to the transcript it was supposed to shrink. Worse, the growth then satisfies conversation_compression's own len(durable_parent) > len(messages) check, so the next attempt adopts the inflated snapshot as if it were genuine concurrent activity and the in-memory transcript doubles too. Check that one precondition before writing. It is a plain read of the row the publish is about to read anyway, and it raises the publish's own message, so split_status=aborted, failure_class=session_split_failed and the rollback path are all unchanged; a live parent reaches the flush exactly as before. Deliberately not extended to the compression lease, which is re-acquirable - a transient miss there would abort a rotation that would otherwise have committed. old_session_id moves above the flush so a failure raised from here takes the same in-memory rollback as any other pre-publish failure. Scope: this fixes the amplification for every abort cause. It does not fix what marks a live session as ended in the first place (NousResearch#88197 Bug 1), which needs a maintainer decision on end-reason taxonomy and is tracked on the issue; an affected session still aborts every attempt, it just stops making itself larger while it does. Refs NousResearch#88197
kshitijk4poor
enabled auto-merge
August 17, 2026 09:32
auto-merge was automatically disabled
August 17, 2026 11:56
Pull request was closed
kshitijk4poor
enabled auto-merge
August 17, 2026 11:56
Collaborator
|
Merged via #88411. Your commit was cherry-picked onto the latest Great find — the mutation table and the fail-open/fail-closed analysis were thorough. The fix is correct and well-tested. |
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.
What does this PR do?
The compression rotation path flushes its un-persisted transcript to the parent (#47202) and then publishes:
The abort handler rolls back the in-memory transcript and keeps
agent.session_idon the parent. Its own comment says "keep the parent live and discard the stale compacted snapshot" — but the rows the flush just committed are not part of what it discards. Every failed rotation leaves the parent transcript longer than it found it, whatever the failure was.Survivable once. Pathological when the failure is sticky:
publish_compression_childrefuses any parent whose row carriesended_at, nothing in this path clears it, so each auto-compaction appends another copy of the current turn to the transcript it was supposed to shrink. And the growth is then self-feeding — it satisfies this same file'sso the next attempt adopts the inflated snapshot as if it were genuine concurrent activity, and the in-memory transcript doubles too.
Three consecutive auto-compactions on such a parent, from the regression test's own log:
This checks that precondition before writing. A live parent reaches the flush exactly as before.
Why this is a safe place to check. It is a plain read of the row
publish_compression_childis about to read anyway, and it raises the publish's own message, sosplit_status=aborted,failure_class=session_split_failed, the"Compression rotation aborted and rolled back to the parent session"warning and the in-memory rollback are byte-identical to today's post-publish abort.old_session_idmoves above the flush for that reason: theexcepthandler keys its rollback off that name, so hoisting it means a failure raised from here rolls the transcript back instead of leaving the failed attempt's compacted snapshot in place.Deliberately not extended to the compression lease. A lease is re-acquirable and its loss is transient, so pre-checking it would abort rotations that would otherwise have committed. The ended-parent check is the opposite shape — it is sticky by construction, which is exactly what makes it worth paying for up front.
Related Issue
Refs #88197
Reported by @mayqhw: a live session went from ~52% to 166% of context in ~15 minutes and hit
400 invalid_request_error, with 303 unique messages stored as 2,611 rows (88% duplicate) after 7 aborted compression attempts. Theirended_atcame fromtui_gateway/server.py::_shutdown_sessions()stampingend_reason="tui_shutdown"on a still-active session when the TUI server auto-reloaded.Scope — this is one of the two halves, and not the one in the title of the issue. It does not fix what marks a live session as ended. An affected session still aborts every attempt; it just stops making itself larger while it does. That half needs a maintainer decision (should
_shutdown_sessions()stamp at all, should the publish guard readend_reasonthe way_check_transcript_write_guardsdoes, or should the attach path reopen the way all eightreopen_sessioncall sites do?) and the analysis is on the issue rather than in this PR.Relationship to #85141, which is open against the same two files: it introduces
_LINEAGE_CLOSED_END_REASONSand makes the write guard use it, deliberately leaving hygiene ends writable.tui_shutdownis a hygiene end by that taxonomy and #85141 does not touch the publish guard, so the two do not overlap — but #85141 reports this same amplification independently (25k → 126k messages on a/newparent), which is the second production sighting of an aborted rotation inflating the row it aborted on. That is the part this PR generalises: it holds for any abort cause, including the ones #85141 is fixing.Type of Change
Changes Made
agent/conversation_compression.py— in the rotation branch: hoistold_session_id = agent.session_idabove the Context compression silently loses unflushed messages (end_session without flush) #47202 flush, and read the parent row before flushing. If it already carriesended_at, raise the publish's ownRuntimeErrorso nothing durable has been written. Fails open on an unreadable row — a cheap guard must not become a new way to lose compression.tests/agent/test_compression_rotation_state.py—TestAbortedRotationDoesNotGrowParent, 3 tests, driving the real_compress_contextagainst a realSessionDB.No behavior change for in-place compaction, for a rotation whose parent is live, or for any other publish failure (lease loss, busy, ambiguous lineage) — those still abort after the flush exactly as before.
How to Test
3 passed.
Mutation check, one rule at a time:
if False and _parent_already_ended:) — i.e. today's behaviorattempt 3 appended to the parent it could not publish, and the log shows the adoption firing at20 -> 40 msgsif True:) — skip the flush unconditionallyexcept Exception: _parent_already_ended = True)Each mutation kills exactly the test that owns the rule, and no others.
Neighbouring suites, all on this branch:
The one failure is
test_compression_review_76354.py::TestF6ExecutorSaturation::test_cancelled_fence_skips_summary_work_before_start, and it is pre-existing and unrelated — verified by stashing both of my files and re-running it on pristinemain(b20229312), where it fails identically withfailure_class=commit_fence_cancelled.ruff checkon both files: clean.Checklist
Code
fix(scope):,feat(scope):, etc.)88197andpublish_compression_child, and a topic sweep of all 21,600 open PR titles for compaction/abort/duplicate/rotation terms. The nearest hits are fix(state): stop compressing /new parents after session_reset #85141 (different guard, different end-reason class — relationship spelled out above), fix(agent): adopt live compression continuation when flushing to a closed session #86409 and fix(agent): adopt live compression child on flush #79763 (both about adopting a live child when a flush hits a closed session, not about the rotation's own pre-publish write), and fix(compression): stop archive_and_compact replay duplicates and mark iteration-limit summaries terminal #87561 (archive_and_compactreplay duplicates — the in-place path, which this PR does not touch).pytest tests/ -qand all tests pass — ran the compression/rotation subset rather than the whole suite; results and the one pre-existing failure are in How to TestDocumentation & Housekeeping
docs/, docstrings) — the guard carries its own reasoning inline, including why the lease is excludedcli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ASessionDBwith no path, process or filesystem behavior. The reported incident is macOS and I verified the DB-level chain on Windows 11, so the fix is platform-neutral in both directionsScreenshots / Logs
From the issue,
agent.logon the affected session:After this change those seven attempts still abort — the parent is still marked ended — but the row count stays at 303 and the session stays usable instead of bricking at the context limit.