fix(responses): stop previous_response_id replay from compounding history - #1698
fix(responses): stop previous_response_id replay from compounding history#1698lidge-jun wants to merge 1 commit into
Conversation
…tory expandPreviousResponseInput concatenated stored items in front of whatever the client sent, unconditionally. When the client already carries that history -- stateless providers replaying full context alongside a previous_response_id -- the turn doubles, and the doubled turn is stored, so the next one triples. #1412 watched 127k of real context reach 1.3M tokens that way. The hard part is not detecting equality, it is proving occurrence. A client may legitimately repeat itself: stored history of one message "repeat" and a genuine delta that also begins with "repeat" produce an identical run, and skipping there would silently delete a real turn. Since stored state flattens request input and provider output into one array, position cannot settle it either -- an id-less assistant message sits in the output region without the provider having authored anything identifiable. So rememberResponseState now records where response.output begins, and a skip requires three things: the run covers the whole stored entry, it reaches that boundary, and some matched item past it carries a provider-issued id. A client echoing provider output WITH the provider's id is replaying that exact occurrence, which is what we want to detect; a client echoing itself cannot manufacture one. Entries whose output carries no id never skip. Comparison is bounded during the walk rather than serialize-then-measure, because a tool result can be megabytes and this is the request path. The cap applies to every item: an id is extra evidence, never a substitute for content equality, so an over-cap tool item is non-comparable exactly like an over-cap message. Any non-comparable item aborts the whole check -- skipping just that item could align two different occurrences. previous_response_id is deliberately preserved. Kiro and Cursor recover their conversation ids from it, so stripping it would start a new upstream conversation to fix a memory bug. The replay prefix length is still recorded, because the boundary is real whoever supplied the history: without it the parser re-acknowledges historical compaction markers and guidance gets injected twice. The anchor is threaded through resident entries, all four spill writes, the spill payload, materialization, and snapshot load, where a malformed value degrades to never-skip rather than to a bad index. Spill compatibility is forward-only: validPayload is a strict key allowlist, so rolling back across this commit invalidates spilled entries, degrading to the already-handled replay-miss path. Two provenance contracts in types.ts said "the proxy expanded"; they now say the history is present however it arrived, which is what every consumer actually reads them for. Known gap, recorded as FU-2: sessions where the proxy injected guidance into stored history, or repaired ids after recording, do not match and expand as before. M0-1 does not bound that -- admission runs after expansion and parsing and fails open on unknown ceilings -- so it stays real remaining work. Verified: bun test tests/continuation-dedup.test.ts (16 pass), plus responses-state and memory-watchdog suites green (130 total), typecheck clean. Two byte-accounting tests mirror the measured envelope by hand and were updated for the new field.
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Landed on dev as d30ab74 (cherry-picked from 33e2b70). Confirmed the defect on dev before landing: src/responses/state.ts:895 unconditionally built input as [...materialized.state.items, ...inputItems(request.input)] with no overlap comparison, so client-carried history could be prepended again on every continuation. The landed change adds guarded overlap detection requiring a full stored-prefix match, a provider-output boundary, and provider-issued identity before skipping the prepend. Verification on the landed commit: bun run typecheck clean, 222 pass / 0 fail across the regression slice. Closing in favour of the direct landing. 다음 배포에 포함됩니다. |
1 participant
Summary
Layer 2 of the usage/memory stack. Stops
previous_response_idreplay from compounding conversation history. Design doc:devlog/_plan/260814_usage_memory_roadmap/020_m0_2_continuation_dedup.md.expandPreviousResponseInputconcatenated stored items in front of whatever the client sent, unconditionally (src/responses/state.ts). When the client already carries that history — stateless providers replaying full context alongside aprevious_response_id— the turn doubles, the doubled turn is stored, and the next one triples. #1412 watched 127k of real context reach 1.3M tokens that way.The hard part is not equality, it is occurrence. A client may legitimately repeat itself: stored history of one message
"repeat"and a genuine delta that also begins with"repeat"produce an identical run, and skipping there would silently delete a real turn. Position cannot settle it either — stored state flattens request input and provider output into one array, so an id-less assistant message sits in the output region without the provider having authored anything identifiable.So
rememberResponseStaterecords whereresponse.outputbegins, and a skip requires three things:id/call_id.A client echoing provider output with the provider's id is replaying that exact occurrence — which is what we want to detect. A client echoing itself cannot manufacture one. Entries whose output carries no id never skip.
Bounded comparison. Canonicalization counts bytes during the walk rather than serialize-then-measure, because a tool result can be megabytes and this is the request path. The cap applies to every item: an id is extra evidence, never a substitute for content equality, so an over-cap tool item is non-comparable exactly like an over-cap message. Any non-comparable item aborts the whole check — skipping just that item could align two different occurrences.
previous_response_idis deliberately preserved. Kiro and Cursor recover their conversation ids from it, so stripping it would start a new upstream conversation in order to fix a memory bug. The replay prefix length is still recorded, because the boundary is real whoever supplied the history: without it the parser re-acknowledges historical compaction markers and guidance gets injected twice.The anchor is threaded through resident entries, all four spill writes, the spill payload, materialization, and snapshot load, where a malformed value degrades to never-skip rather than to a bad index. Spill compatibility is forward-only:
validPayloadis a strict key allowlist, so rolling back across this commit invalidates spilled entries, degrading to the already-handled replay-miss path.Two provenance contracts in
src/types.tssaid "the proxy expanded"; they now say the history is present however it arrived, which is what every consumer actually reads them for.Known gap (FU-2, recorded in the doc): sessions where the proxy injected guidance into stored history, or repaired ids after recording, do not match and expand as before. M0-1 does not bound that — admission runs after expansion and parsing and fails open on unknown ceilings — so it stays real remaining work rather than something already covered.
Verification
bun test tests/continuation-dedup.test.ts— 16 pass, 0 failbun test tests/responses-state.test.ts tests/memory-watchdog.test.ts tests/continuation-dedup.test.ts— 130 pass, 0 failbun run typecheck— cleanTwo existing byte-accounting tests mirror the measured envelope by hand and were updated for the new field; they still assert exact sizes.
Notable cases: the one-message repeat does not skip; id-less provider output does not skip; a spilled entry keeps its anchor and still deduplicates (a real bug caught during implementation —
writeResponseSpillDurablyrebuilds the payload field by field and was dropping it); an over-cap or deeply nested item is non-comparable and expands.Stack
Depends on #1696 → #1693. Review this PR's diff only.
Checklist