Skip to content

fix(core): guard compression request admission - #13072

Closed
AaronZ345 wants to merge 11 commits into
QwenLM:mainfrom
AaronZ345:aaron/fix-compression-context-admission-v2
Closed

AaronZ345 wants to merge 11 commits into
QwenLM:mainfrom
AaronZ345:aaron/fix-compression-context-admission-v2

Conversation

@AaronZ345

Copy link
Copy Markdown
Contributor

What this PR does

Rebuilds the compression-admission fix on the current main without rebasing or force-pushing the old PR branch.

The change applies complete request admission to shared-cache and cold compression requests. It combines provider anchors with current-route estimates, includes thought signatures, runs bounded microcompaction before cold requests, and rejects requests locally when they cannot leave a usable output budget.

It also preserves current-main model routing, including endpoint-pinned compaction model selection, while retaining the admission safeguards from #9541.

Why it's needed

Issue #9455 captured compression requests whose estimated input plus reserved output exceeded the receiving model window. Those requests were knowingly sent to the provider and failed with context-window errors instead of being reduced or rejected locally. Additional review found that multilingual inputs, side-query failures, and mixed exact/estimated accounting could still bypass or corrupt that safety boundary.

Reviewer Test Plan

Verify the focused core suites around chat compression, compaction slimming, microcompaction, turn handling, and llm-chat, plus the normal CI build/lint/typecheck gates.

The regression matrix covers same-model cache admission, route overhead, thought signatures, bounded microcompaction, multilingual dense text, compaction-model budget boundaries, side-query exceptions, truncation telemetry, estimated token deltas, and failure rendering.

Risk & Scope

  • Token estimates remain heuristic when provider-comparable usage is unavailable.
  • Provider-reported prompt counts bound the local history heuristic when authoritative.
  • Current-main endpoint-pinned compaction model routing is preserved.
  • No migration or breaking API change.

Linked Issues

Fixes #9455.
Supersedes #9541, whose branch no longer merged cleanly with current main.

中文说明

本 PR 在最新 main 上重新构建压缩准入修复,不对旧分支 rebase 或 force-push。保留 #9541 的压缩准入保护,同时保留当前 main 已新增的 endpoint-pinned 压缩模型路由。

关联并修复 #9455;由于 #9541 已与当前 main 产生真实合并冲突,本 PR 作为其干净替代。

Copy link
Copy Markdown
Contributor Author

Current head: 2459defd980ece3f9c0c5f343fd385e81de91b1a

Fresh current-main replacement for #9541. The compression admission changes were ported onto current model-routing code, including endpoint-pinned compaction model selection.

@qwen-code /review

chore: sync current upstream main into aaron/fix-compression-context-admission-v2

Copy link
Copy Markdown
Contributor Author

Current head: 490b993ef5816ac82e0540168e26d1c4f25e3dbb

Current-main replacement for #9541 after syncing the latest upstream commit; preserves endpoint-pinned model routing and compression admission safeguards.

@qwen-code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review at 490b993 by a maintainer's assistant. This PR appears to have no automated review (the precheck withheld triage), so this is the first review on the thread. Scope checked: ChatCompressionService.compress is the sole LLM-compression entry, so all five callers are covered; the guard has no queue/slot/shared counter, so no check-then-act or leak concerns apply; the rejection path returns newHistory: null without touching history and the breaker latches. Two findings:

cache_sharing_used: usedCacheSharing,
}),
);
const newTokenCountIsEstimated =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The new provenance stamp ignores outputCountIsEstimated, so a locally-estimated output count can be reported as authoritative.

const newTokenCountIsEstimated =
  usedEstimatedVisibleDelta ||
  Boolean(opts.originalTokenCountIsEstimated) ||
  restorationChars > 0;

One of the two "provider" counts consumed by the branch above (lines 1535-1566) can be a local estimate: when the provider reports promptTokenCount but omits candidatesTokenCount/totalTokenCount, the code sets outputCountIsEstimated = true and compressionOutputTokenCount = estimateSummaryOutputTokens(...) (lines 1296-1300), and the branch only requires typeof compressionOutputTokenCount === 'number' && > 0. newTokenCountIsEstimated then omits that third estimated source, so with an authoritative baseline and no restoration attachments it computes false and stamps a partly-estimated newTokenCount as exact.

The merge base hardcoded newTokenCountIsEstimated: true on this return, so this is a regression rather than a pre-existing gap. Consequence: tryCompress stores the value as non-estimated (llm-chat.ts:2772-2775), the banner drops the ~ prefix, and the threshold/session-limit gates later trust a number that contains an estimate.

Fix: fold the flag in — usedEstimatedVisibleDelta || outputCountIsEstimated || Boolean(opts.originalTokenCountIsEstimated) || restorationChars > 0; — and add a test with usage: { promptTokenCount: N } only, asserting newTokenCountIsEstimated === true.

}).length / CHARS_PER_TOKEN,
);
sharedCurrentRouteTokenEstimate =
estimateContentTokens(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The cache-sharing admission estimate uses the raw char/4 estimator, not the UTF-8-adjusted one the cold path uses.

sharedCurrentRouteTokenEstimate is built from estimateContentTokens(...), while the cold path's sibling term is estimateUtf8AdjustedContentTokens(...) at line 663 (and the pre-hook shared estimate at line 639 has the same raw shape). This is the only local term protecting the unslimmed, largest request when the provider anchor understates the current route — the scenario the new "stale provider anchor" test targets — yet for non-ASCII content it is roughly 5× below the calibrated estimate (0.25 tok/char vs 1.25). That is the multilingual-bypass class this PR set out to fix, and the only test for this term uses ASCII.

Failure is bounded (the shared request 400s, is caught, and the slimmed cold path runs), hence a Suggestion: use estimateUtf8AdjustedContentTokens(sideQueryHistory, slimmingConfig.imageTokenEstimate) for the current-route term at both 639 and 1095.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): chat compression can exceed its target model context window

3 participants