fix: auto-compaction fires on fresh cached token counts (#66520)#66716
fix: auto-compaction fires on fresh cached token counts (#66520)#66716KeWang0622 wants to merge 2 commits into
Conversation
…reshold When a provider like Anthropic has a high prompt cache hit rate, totalTokens on the session entry includes cached tokens (input + cacheRead + cacheWrite) and is marked as fresh. Previously, runPreflightCompactionIfNeeded returned early when totalTokensFresh was true WITHOUT checking the compaction threshold, so auto-compaction never triggered — even at 153% of the context window. This change restructures the token resolution so that fresh persisted totals are checked against the compaction threshold (contextWindow - reserveTokens - softThreshold) before deciding whether to compact. The stale/transcript fallback path is preserved unchanged. Fixes openclaw#66520 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Greptile SummaryThis PR fixes a real bug where Confidence Score: 5/5
Reviews (1): Last reviewed commit: "fix: preflight compaction now fires when..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86352d78cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!shouldUseTranscriptFallback) { | ||
| // Fresh persisted tokens available — project forward with prompt estimate | ||
| const projectedFreshTokens = resolveEffectivePromptTokens( | ||
| freshPersistedTokens, |
There was a problem hiding this comment.
The new fresh-token branch now runs preflight compaction when shouldUseTranscriptFallback is false, but that predicate still treats totalTokensFresh: undefined as fresh. For legacy/unknown session entries (where freshness is not guaranteed), this branch uses freshPersistedTokens directly and can trigger compaction from stale totals, causing unnecessary summarization on the next turn. This behavior is introduced by the new branch because these sessions previously returned early without compacting; please gate this path on totalTokensFresh === true (or fall back to transcript estimation when freshness is unknown).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c149fe4. Three changes to runPreflightCompactionIfNeeded:
-
Replaced
shouldUseTranscriptFallbackwithhasFreshPersistedTokensthat requirestotalTokensFresh === trueexplicitly. The previous predicate (entry.totalTokensFresh === false || !hasPersistedTotalTokens) treatedundefinedas "not needing fallback" becauseundefined === falseisfalse. Now whentotalTokensFreshisundefined(legacy) orfalse(known-stale), the function falls through to transcript estimation. -
Always run transcript estimation in the fallback branch. The old guard (
typeof freshPersistedTokens === "number") skipped transcript estimation for legacy sessions becauseresolveFreshSessionTotalTokenstreatstotalTokensFresh: undefinedas fresh and returns a number. Now the else branch unconditionally estimates from the transcript. -
Bail out before
shouldRunPreflightCompactionwhen freshness is unconfirmed. Even with the above fixes, if transcript estimation returnsundefined(e.g. no session file),shouldRunPreflightCompactionwould fall back toresolveFreshSessionTotalTokensinternally, which again treatsundefinedas fresh. Added an explicit guard: iftokenCountForCompactionis not a number andtotalTokensFresh !== true, return early.
Added a test case for totalTokensFresh: undefined (legacy sessions) that verifies compaction is not triggered from stale persisted totals. All 9 tests in the test suite pass, along with the related test files (followup-runner, commands-compact, agent-runner-direct-runtime-config, reply-state).
The fresh-token branch in runPreflightCompactionIfNeeded ran when shouldUseTranscriptFallback was false, but that predicate treated totalTokensFresh: undefined as NOT needing fallback (undefined === false evaluates to false). For legacy sessions where totalTokensFresh was never set, this caused the fresh-token path to use potentially stale persisted totals, triggering unnecessary compaction. Three changes: 1. Replace shouldUseTranscriptFallback with hasFreshPersistedTokens that requires totalTokensFresh === true explicitly. When undefined (legacy) or false (known-stale), falls through to transcript estimation. 2. In the transcript fallback branch, always run transcript estimation instead of skipping when resolveFreshSessionTotalTokens returns a value (that function also treats undefined freshness as fresh). 3. Add bail-out before shouldRunPreflightCompaction when transcript estimation returns no count and freshness is unconfirmed, preventing the gate function from falling back to stale totals via resolveFreshSessionTotalTokens. Resolves Codex P2 review on PR openclaw#66716. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Related work from PRtags group Title: Preflight compaction skips fresh token totals
|
|
Thanks for the context here. I did a careful shell check against current Close this stale PR: current main and the latest release already cover the central fresh-token preflight compaction problem with a broader token-accounting path, while the remaining branch-specific diff is conflicting and no longer a useful landing candidate. So I’m closing this older PR as already covered on Review detailsBest possible solution: Keep the current main implementation and close this stale branch; any remaining compaction edge case should be opened as a narrow follow-up against the current token-accounting path. Do we have a high-confidence way to reproduce the issue? Yes. The old source and the PR diff show a high-confidence source reproduction: when persisted total tokens were treated as fresh, runPreflightCompactionIfNeeded returned before the threshold check. Is this the best way to solve the issue? No. This PR was a plausible April fix, but current main now solves the central behavior through a broader compaction accounting path, and the stale branch would be the wrong landing vehicle. Security review: Security review cleared: The diff only changes compaction runtime logic and tests; it does not add dependency, workflow, secret, permission, network, or package-resolution surface. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against d28691da97fc; fix evidence: release v2026.6.6, commit 8c802aa68351. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
P3
|
ClawSweeper applied the proposed close for this PR.
|
2 participants
Summary
runPreflightCompactionIfNeededreturned early whentotalTokensFresh === truewithout checking the compaction threshold, so auto-compaction never triggered for sessions with fresh token counts — even at 153% of the context windowcacheReadfrom Anthropic prompt caching) are projected forward and checked againstcontextWindow - reserveTokens - softThresholdbefore deciding whether to compactDetails
When Anthropic's prompt cache absorbs nearly all tokens (100% hit rate),
derivePromptTokenscorrectly computesinput + cacheRead + cacheWrite(e.g. 99 + 305,000 + 0 = 305,099), and this is persisted astotalTokenswithtotalTokensFresh: true. However,runPreflightCompactionIfNeededhad this logic:The fix moves the threshold check into both branches (fresh and stale), using the fresh persisted value directly when available.
Edge cases verified
cacheRead=0, sototalTokensis justinput + cacheWrite, same as beforeTest plan
shouldRunPreflightCompactionunit tests: 100% cache hit triggers, below threshold skips, partial cache boundary correctrunPreflightCompactionIfNeededintegration tests: fresh tokens above threshold trigger compaction, below threshold skip, stale tokens use transcript fallback, heartbeat skipsFixes #66520
🤖 Generated with Claude Code