fix(api_server): avoid duplicating history when preflight compression rewrites transcript - #41700
Conversation
|
Positive verification — history duplication regression fix. The The static method is intentionally content-only (ignoring metadata like Test coverage is thorough: the regression test simulates the exact chained-turn scenario (3 turns with compression between turns 2 and 3) and verifies |
|
I had this problem too |
|
@alt-glitch friendly ping — the CI workflow is still awaiting |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the stale-prefix failure: current main still prepends the old history after _response_messages_turn_start_index() returns zero (gateway/platforms/api_server.py:3873-3885), so the proposed transcript guard addresses a real part of the bug.
Problems
- Compression can rotate the agent session.
_run_agentreturns that effective ID inresult["session_id"](gateway/platforms/api_server.py:4128-4134), but Responses snapshots still persist the original requestsession_id(gateway/platforms/api_server.py:2760-2765; non-streaming storage at:3509-3514). This patch does not update either path, so the nextprevious_response_idrequest still uses the old session (:3324-3350). - The added test asserts only transcript replacement; it does not cover rotated-session continuation or streaming snapshots.
Suggested changes
- Persist and return the effective result session ID for both Responses completion paths, and add chained rotated-session tests for streaming and non-streaming behavior.
Automated hermes-sweeper review.
| # would double-count history and grow _response_store uncapped | ||
| # across chained turns (994 → 1119 → 1358 … bug). Fall through | ||
| # to the legacy prepend only for true turn-only deltas. | ||
| if APIServerAdapter._messages_contain_user_turn( |
There was a problem hiding this comment.
This fixes the duplicated transcript branch, but compression can also rotate agent.session_id. _run_agent already returns that value in result["session_id"], while Responses snapshots still store the pre-run session_id; propagate the effective ID through both streaming and non-streaming persistence so the next previous_response_id call does not resume the old session.
… rewrites transcript When the AIAgent runs preflight compression mid-turn, the returned result["messages"] is the *post-compression* full transcript, which no longer starts with the uncompressed prior history prefix that _build_response_conversation_history uses to detect transcript-shaped agent output. The prefix match fails, the fallback branch fires, and the adapter stores `prior + current_user + compressed_messages` in _response_store — double-counting every chained turn. Stored history grows uncapped (observed 994 -> 1119 -> 1358 messages across three follow-ups) and preflight compression re-triggers on every subsequent request because the bloated history keeps breaching the model context limit. Add a secondary detection rule: if the agent-returned messages already contain the current user turn (content-based match, robust to metadata like timestamp/name), trust them as a full transcript and return as-is. Only true turn-only deltas (no current-user marker) still fall through to the legacy prepend path, preserving existing behaviour for callers that did not run compression. Regression covered by TestResponsesEndpoint.test_previous_response_id_with_compressed_transcript_does_not_duplicate which asserts the stored history is exactly the compressed transcript (3 items) rather than the bloated prior(40) + current(1) + compressed(3) that the buggy fallback produced.
Follow-up to teknium1's review on PR NousResearch#41700. The transcript-fallback fix in this PR closed the visible bug (double history growth), but the underlying compression path can also *rotate* the agent's session_id mid-run. _run_agent surfaces the effective, post-rotation value via `result["session_id"]`, yet the two Responses completion paths still persisted the pre-run session id into `_response_store`. Every subsequent request that chained via `previous_response_id` therefore resumed the abandoned pre-compression session — re-triggering compression on every turn. Persist and return the effective result session id from both Responses completion paths: - Non-streaming (`_handle_responses`): read `result["session_id"]`, write it into the stored snapshot and the `X-Hermes-Session-Id` response header. - Streaming (`_write_sse_responses`): track `effective_session_id` starting from the pre-run value; after `agent_task` completes, adopt `result["session_id"]` if present, so the terminal `response.completed` / `response.failed` snapshot records the rotated id. The initial `response.created` snapshot intentionally keeps the pre-run value — rotation hasn't happened yet at that point. Add two regression tests that chain a third turn after a simulated rotation and assert: - The stored snapshot's `session_id` is the post-rotation value. - The non-streaming response returns it via `X-Hermes-Session-Id`. - The next `previous_response_id` request passes the post-rotation session id to `_run_agent`, so the agent resumes the same underlying session instead of resurrecting the abandoned pre-rotation row. All 199 tests in `tests/gateway/test_api_server.py` pass under the canonical `scripts/run_tests.sh` runner.
b485d80 to
38e0774
Compare
|
Thanks for the deep dive @teknium1 — you're right that persisting the pre-run session id on the Responses side was papering over the compression rotation path. Pushed Non-streaming (
Streaming ( Tests
Full |
|
Fixed via #69306 (merged), which salvaged #58133. You identified and fixed this root cause first (June 8) — first-submitter credit is yours in the merged PR body. The later branch was used as the merge vehicle only because it was already rebased onto the refactored _run_agent structure. Thanks for the original diagnosis! |
Summary
/v1/responseswas storing duplicated history in_response_storewhenever theAIAgent ran preflight compression mid-turn. Stored history grew uncapped across
chained turns (observed
994 → 1119 → 1358messages over three follow-ups),which in turn re-triggered preflight compression on every subsequent request —
preflight compression effectively never "stuck".
Root cause
APIServerAdapter._build_response_conversation_historydecides whetherresult["messages"]is a turn-only delta or a transcript-shaped full history bychecking whether it starts with
prior + current_user(or justprior).When the agent runs preflight compression,
result["messages"]is thepost-compression full transcript. It does NOT start with the uncompressed
priorprefix, so the prefix match fails, the fallback branch fires, and theadapter stores
prior + current_user + compressed_messages— double-countingevery chained turn.
Fix
Add a secondary detection rule: if the agent-returned messages already contain
the current user turn (content-based match, robust to metadata like
timestamp/
name), trust them as a full transcript and return as-is. Only trueturn-only deltas (no current-user marker) still fall through to the legacy
prepend path, preserving existing behaviour for callers that did not run
compression.
Test plan
New regression test
TestResponsesEndpoint.test_previous_response_id_with_compressed_transcript_does_not_duplicateasserts the stored history is exactly the compressed transcript (3 items)
rather than the bloated
prior(40) + current(1) + compressed(3) = 44itemsthat the buggy fallback produced.
python -m pytest tests/gateway/test_api_server.py -q→ 158 passed