fix(compaction): later provider timeouts discard completed chunks#92899
fix(compaction): later provider timeouts discard completed chunks#92899mushuiyu886 wants to merge 14 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 10:03 PM ET / July 18, 2026, 02:03 UTC. Summary PR surface: Source -4, Tests +1. Total -3 across 2 files. Reproducibility: yes. from the supplied production-path loopback HTTP/SSE proof and focused regression fixture: complete one summary chunk, then return a provider timeout for the next chunk while the caller signal remains unaborted. I did not independently execute that path in this read-only review. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow partial-summary recovery only if the rebased current-main result retains terminal caller cancellation and safety-timeout handling while preserving completed chunks for non-caller provider failures. Do we have a high-confidence way to reproduce the issue? Yes, from the supplied production-path loopback HTTP/SSE proof and focused regression fixture: complete one summary chunk, then return a provider timeout for the next chunk while the caller signal remains unaborted. I did not independently execute that path in this read-only review. Is this the best way to solve the issue? Yes for this bounded failure mode: it reuses the existing partial-summary path and preserves the established terminal boundary for an actually aborted caller signal. Cross-attempt persistence and timeout-budget policy remain separate work in #92043. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3d1b77103511. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -4, Tests +1. Total -3 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (27 earlier review cycles; latest 8 shown)
|
5013dda to
cdacf1e
Compare
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
Security-sensitive guard clearedThis PR no longer has blocked security-sensitive file changes. A future security-sensitive change requires a fresh
|
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Updated the branch to resolve the current
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
Related: #92043
What Problem This Solves
Fixes an issue where multi-chunk compaction would discard summaries from completed chunks when a later provider request timed out, replacing useful progress with the generic "Summary unavailable due to size limits" fallback.
Why This Change Was Made
Non-caller provider timeouts now use the existing partial-summary recovery path after at least one chunk has completed. Caller cancellation and wrapper safety-timeout aborts remain terminal, so this no longer changes post-timeout AgentSession persistence, lifecycle, or stored-state policy.
This update intentionally narrows the previous 20-file proposal to the primary contributor-owned invariant. The bounded post-timeout commit policy, session lifecycle drain, retained-entry remapping, timeout-helper cleanup, and failure-copy changes are no longer part of this PR.
User Impact
When an early compaction chunk succeeds and a later provider request times out, OpenClaw retains the completed summary instead of losing all progress. Explicit cancellation behavior and the outer compaction safety timeout are unchanged.
Evidence
The narrowed behavior is covered by a red/green regression test, adjacent compaction tests, and an actual loopback OpenAI-compatible HTTP/SSE before/after run.
Real behavior proof
A loopback OpenAI-compatible HTTP/SSE provider exercised the production summary transport with two chunks: the first request returned a streamed summary and the second returned HTTP 504 with a provider timeout error.
origin/main(277f009cc902aecf09ee0cdd8345223186c16fa9): 2 provider requests; first chunk completed; later request timed out;retainedCompletedChunk=false;partialMarkerPresent=false; generic fallback returned.1dc4de07426a218efe07d436d5db92bd1c7c37b8: 2 provider requests; first chunk completed; later request timed out;retainedCompletedChunk=true;partialMarkerPresent=true; generic fallback absent.Tests and checks
node scripts/run-vitest.mjs src/agents/compaction-partial-summary.test.tsfailed because a later timeout returned the generic fallback.compaction-partial-summary.test.tspluscompaction.summarize-fallback.test.ts, 13/13 tests passed.packages/net-policy/src/ip.tserrors; the identical command reproduced the same errors on a cleanorigin/maincontrol worktree with the same dependency tree.summarizeWithFallbackResult, caller-abort behavior, timeout classification, and adjacent partial-summary tests.