fix(compaction): add circuit breaker to stop token burn when summarizer unavailable#86900
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 3:14 AM ET / 07:14 UTC. Summary PR surface: Source +52, Tests +142. Total +194 across 4 files. Reproducibility: yes. at source level: persistent summarizer failures traverse each staged split's retry and generic-fallback path, and the focused tests deterministically exercise that condition. A failing live current-main run was not performed during this review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Keep the typed internal result and incomplete-compaction cancellation design, add current-head runtime or compiled fault-injection output proving that later calls stop and the original transcript remains intact, then merge after exact-head checks pass. Do we have a high-confidence way to reproduce the issue? Yes at source level: persistent summarizer failures traverse each staged split's retry and generic-fallback path, and the focused tests deterministically exercise that condition. A failing live current-main run was not performed during this review. Is this the best way to solve the issue? Yes. Typed internal outcomes plus cancellation of an incomplete compaction is narrower and safer than string matching, committing partial coverage, or adding a process-wide time-based breaker. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b64242a4a99a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +52, Tests +142. Total +194 across 4 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 (1 earlier review cycle)
|
|
ClawSweeper PR egg: 🎁 locked until real behavior proof passes. Details
|
martingarramon
left a comment
There was a problem hiding this comment.
Resetting consecutiveChunkFailures on each successful chunk is correct — the breaker targets a persistently unavailable summarizer, not a single hard-to-summarize chunk.
One accuracy issue in the circuit-breaker catch block. When the breaker trips at the second consecutive failure (chunk at zero-based index N), completedChunks = chunks.indexOf(chunk) equals N. But the preceding chunk (index N-1) also failed and was not summarized, so the annotation:
chunks 1-${completedChunks} of ${chunks.length} were summarized
overcounts by one — it should read chunks 1-${completedChunks - 1}. The accumulated summary content is correct (it only updates on success); the annotation text is what's off.
The previousSummary ?? DEFAULT_SUMMARY_FALLBACK path when no chunk has succeeded handles the cold-start unavailable case cleanly.
This comment was marked as low quality.
This comment was marked as low quality.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
This comment was marked as low quality.
This comment was marked as low quality.
|
Heads up: this PR needs to be updated against current |
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
|
🦞🧹 I asked ClawSweeper to review this item again. |
This comment was marked as low quality.
This comment was marked as low quality.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
This comment was marked as low quality.
This comment was marked as low quality.
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
This comment was marked as low quality.
This comment was marked as low quality.
|
This pull request has been automatically marked as stale due to inactivity. |
Co-authored-by: tanshanshan <[email protected]>
|
Merged via squash.
|
Summary
Fixes #58838. Staged compaction no longer burns requests across every split when summarization repeatedly falls back generically.
The maintainer rewrite:
What Problem This Solves
When the summarizer is persistently unavailable, staged compaction can exhaust the retry/fallback path for every split. That multiplies token and latency cost. Stopping the loop by returning the summaries collected so far is unsafe: later transcript chunks were never processed, yet compaction would commit and drop them. This fix bounds the storm and cancels the incomplete compaction so the transcript survives.
Evidence
node scripts/run-vitest.mjs src/agents/compaction.circuit-breaker.test.ts src/agents/compaction.identifier-preservation.test.ts src/agents/compaction.summarize-fallback.test.ts src/agents/compaction-partial-summary.test.ts src/agents/agent-hooks/compaction-safeguard.test.tspassed 5 files and 133 tests. The breaker test proves exactly two calls before circuit-open and rejection; the intermittent test proves counter reset plus degraded merge state; safeguard coverage proves prior-summary retention. Fresh full-diff autoreview is clean.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Motivation
A persistently unavailable summarizer could make every staged split exhaust its retry/fallback path, multiplying token and latency cost. Returning an incomplete partial summary would also risk committing away splits that were never processed. The breaker must stop the request storm and preserve the source transcript.
Real Behavior Proof
Focused regression command:
Result: 5 test files passed, 133 tests passed.
The circuit-breaker regression proves two consecutive generic fallbacks make only two summarization calls and reject the incomplete compaction. The intermittent sequence proves a successful split resets the consecutive counter, the merge still runs, and the typed result remains degraded. Safeguard coverage proves a prior summary survives degraded redistillation.
Scoped changed-gate command:
Conflict markers, max-lines ratchet, changelog attribution, wildcard export guards, duplicate coverage, dependency pins, and formatting passed. The delegated run then encountered pre-existing Plugin SDK API baseline drift on current main; this PR changes no Plugin SDK path or export. Exact-head hosted CI remains the landing gate.
Fresh full-diff autoreview result: clean, no accepted/actionable findings.
Root Cause
The staged loop treated a generic fallback string like a usable summary and had no typed signal for repeated failure. That made it impossible to bound repeated calls without brittle string matching, and an early partial return could silently discard unprocessed transcript chunks.
Regression Test Plan
User-visible / Behavior Changes
During a sustained summarizer outage, OpenClaw stops staged summarization after two consecutive generic fallbacks and cancels that compaction attempt. The session transcript remains available for a later retry instead of being replaced by an incomplete placeholder summary.
Release-note context: compaction now stops repeated staged fallback storms while preserving session history when the summarizer is unavailable. Thanks @tanshanshan.
Security Impact (required)
Human Verification (required)
Review Conversations
Compatibility / Migration
Risks and Mitigations
Risk: stopping early could lose current transcript context if an incomplete summary were committed. Mitigation: circuit-open throws into the safeguard's existing cancel path, which retains the original transcript. No config, storage schema, or public API changes.