Repository navigation
fix(core): retry truncated compaction with less history - #57236
surlymochan wants to merge 2 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
With the PR, the new regression tests pass, and 5 of the 8 fail on the base (I ran them against both). The packages/core typecheck passes. I did not drive the TUI end to end, because the tests' local SSE server already covers the behaviour. The public API is unchanged.
Main concern: the length retry fires even when shrinking the input cannot give the summary more room (see the inline comment). In the PR's own "retries once" test, both requests ask for the same max_tokens: 30000. The retry only drops the oldest exchange, which then ends up in neither the summary nor the retained tail, and the user is not told. That helps the #57187 case, where the output limit was cut down to fit the window. For a summary that simply hit the fixed cap, it trades a visible failure for lost context. Please limit the retry to requests whose output limit fitOutput actually lowered.
Related: #57220 (draft) also handles finish: "length" in compaction. It resends the same input with a request for a shorter summary and lower reasoning, and it edits the same case "length" / deliver code. The two PRs will conflict and need reconciling.
Tests: the new file has 281 lines and starts a real local server for a 60-line change. Two or three cases would cover the fix: one truncated retry that succeeds, one second truncation that fails, and one mixed overflow/length sequence. Please trim the rest.
Related: #56862
| const cause = attempt.failure | ||
| if (!(cause instanceof AIError)) return yield* Effect.fail(cause) | ||
| if (!(cause instanceof AIError)) { | ||
| if (!cause.outputLimit || lengthFailure) return yield* Effect.fail(cause) |
There was a problem hiding this comment.
This shrinks on any outputLimit failure. A smaller request only gets more output room when the first request's maxTokens was cut down by fitOutput (the window was nearly full, as in #57187). Otherwise it stays at the same cap: in a truncated summary retries once… both bodies send max_tokens: 30000, and the retry just drops the oldest exchange. That exchange then ends up in neither the summary nor the tail, and the user is not told. Could you only take this path when the truncated request's output limit was below the prepared one (for example, by recording that in the Failure), and keep the visible failure otherwise?
There was a problem hiding this comment.
Addressed in 8dbb566. Fixed-cap exhaustion and missing/incomplete window evidence now preserve the visible failure and original history. Shrinking requires a client window clamp or inclusive provider usage reaching the configured window while output remains below the actual requested cap. The source-service probe demonstrated the latter: requested cap 16,000; input 62,295 + output 3,497 = window 65,792. Forty focused tests pass, including fixed-cap refusal, cancellation and mixed-error budgets. Related #57220 is acknowledged in the body; this patch does not adopt its shorter-summary/reasoning policy. Please re-review the updated head.
Issue for this PR
Fixes #57187
Related: #56862 (length-limited summaries; the empty-stop/subagent variant is not covered).
Type of change
What does this PR do?
Retry truncated compaction once with less history only when evidence shows its context window constrained output: the client lowered the prepared request cap for available window room, or inclusive provider usage reaches the configured window while output remains below the actual requested cap. Fixed-cap exhaustion and missing/incomplete usage alone do not trigger a shrink; the visible failure and original history remain.
Recovery uses the existing reducer and shared three-reduction input-overflow budget. Mixed overflow/length failures keep shrinking instead of resending a rejected request. Partial summaries stay rejected; cancellation and retained-tail behavior remain unchanged.
This covers the older-only summary request independently of #54107's cache-prefix fallback. Related Draft #57220 requests a shorter summary/lower reasoning; this patch is limited to evidenced window pressure and does not adopt that policy.
How did you verify your code works?
Tested in managed cloud with a synthetic provider, not live LM Studio. The full-repo check was attempted earlier but Turbo crashed during typecheck (SIGSEGV, exit 139); it was not rerun for this correction and is not reported as passing.
Screenshots / recordings
N/A — compaction recovery behavior.
Checklist
AI-assisted implementation; final diff reviewed before publication.
Added by the opencode bot (AI-generated).
Related: #56862