Skip to content

perf(core): clear tool results to a low watermark to preserve prompt cache - #8464

Merged
doudouOUC merged 9 commits into
QwenLM:mainfrom
doudouOUC:perf/microcompact-low-watermark
Aug 4, 2026
Merged

perf(core): clear tool results to a low watermark to preserve prompt cache#8464
doudouOUC merged 9 commits into
QwenLM:mainfrom
doudouOUC:perf/microcompact-low-watermark

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

When the cumulative size of compactable tool results crosses the configured threshold, the client now clears oldest results down to a low watermark at half the threshold, instead of stopping as soon as the total is just below the threshold. The trigger condition, the -1 disable semantics, and the legacy idle-disable compatibility are unchanged; only the clearing depth changes, so between clearings the conversation prefix stays byte-stable and provider prompt caches keep matching.

It also fixes protection-accounting defects that deeper clearing would have amplified. The recent-result budget was computed over history plus the pending, about-to-be-sent tool results, so a ToolResult turn carrying enough pending results left zero committed results protected; it also counted error responses, previously cleared placeholders, and empty outputs — entries that can never be cleared — letting them absorb protection slots from real recent outputs, while media-only results (image/PDF reads whose bytes live outside the text output) stay protected. The budget now selects from committed results with actual clearable content. File-residency vouching in eviction reporting is now strictly conservative: pending results and kept read or edit results cannot prove a file's complete bytes remain in history — only a kept write result can, since its call carries the full content — so the file-read fast path is disarmed in every ambiguous case, at worst costing one redundant re-read.

For observability, the size-cleanup metadata and its debug log line now report the watermark target, with a soft-exceeded marker when protected results keep the total above it. Settings descriptions are updated accordingly.

Why it's needed

Production traffic analysis over a two-day window showed long active sessions stuck at a ~5.43% cache rate (only a fixed ~37,840-token head kept matching) while comparable large-context sessions without history rewrites cached at 99.90%; this pattern accounted for about 63% of all cache loss in the window. The cause is the "just below the threshold" stop: once the budget rides the limit, nearly every turn blanks one more old result and invalidates the cache from that position on. With the watermark, the same #5101-shaped workload (167 results of ~25.5K chars) goes from ~148 rewrites to 14 — about one per 11 average-size results. For workloads where the watermark is reachable — protected results fit under it — the trigger fires no more often than before and each batch break lands no earlier in the prefix. When protections alone pin the total above the threshold (for example five protected 100K results against a 500K threshold), the trigger can fire on consecutive checkpoints, matching the pre-existing rolling regime rather than improving on it; a regression test pins this corner.

Reviewer Test Plan

How to verify

Drive a session past the size threshold with repeated large compactable tool outputs and watch the size-cleanup debug log: it should fire once, report clearing down to about half the threshold with a target value, and then stay silent while subsequent turns reuse the now-stable prefix, instead of logging one cleanup per turn. Confirm the totals-exceeded log still appears with cleared 0 and a soft-exceeded marker when everything above the watermark is protected. Confirm a ToolResult turn that batches five or more pending results still leaves the five most recent committed results intact. Confirm -1 (and the legacy negative idle threshold without the new setting) still disables the size trigger entirely.

Commands run locally: cd packages/core && npx vitest run src/services/microcompaction/microcompact.test.ts src/core/client.test.ts src/services/fileReadCache.integration.test.ts src/services/memoryPressureMonitor.test.ts (442 passed); cd packages/cli && npx vitest run src/config/settingsSchema.test.ts (34 passed); npm run build; npm run typecheck; npm run generate:settings-schema; git diff --check.

Evidence (Before & After)

N/A. Non-UI behavior covered by focused unit tests, including a workload simulation asserting the rewrite count drops to exactly 14 for 167 sequential 25.5K-char results, a steady-state test asserting the history object is returned untouched between clearings, and review-driven regressions: trailing zero-char results cannot absorb protection slots, a pending cache-hit placeholder cannot suppress fast-path disarming, and the protected-saturation corner where consecutive checkpoints re-trigger.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node.js v24; local repository checkout; unit tests only, no sandbox-specific runtime required.

Risk & Scope

  • Main risk or tradeoff: each clearing now removes more old tool output at once — retention at clearing time drops from roughly the threshold to roughly half of it — so old outputs become placeholders earlier than before. The recent-result floor (now selected from clearable committed results), pending results' exemption from clearing, and the managed-memory preservation remain in place. If a single turn adds more than half the threshold of new output, clearing degrades to firing every turn; when protected results alone exceed the threshold it can fire on consecutive checkpoints — both match the pre-existing rolling regime. A pending same-file re-read no longer suppresses fast-path disarming, which can cost one redundant re-read.
  • Not validated / out of scope: the projected 80–90% token-weighted cache-rate recovery is an estimate pending replay validation against real traffic; the watermark remains a best-effort target that protected results may keep the total above; this mechanism is still a character budget for successful compactable tool outputs, not a total prompt bound — token estimation, automatic compression, and context-overflow protection are untouched.
  • Breaking changes / migration notes: none. No new settings; the watermark is derived as half of toolResultsTotalCharsThreshold, and -1 still disables the size trigger.

Linked Issues

Fixes #8452
Fixes #8463

中文说明

What this PR does

当可压缩工具结果的累计大小越过配置阈值时,客户端现在会按最老优先清理到阈值一半的低水位,而不是清到刚好低于阈值就停止。触发条件、-1 禁用语义、legacy idle 禁用兼容都不变;只有清理深度变化,因此两次清理之间会话前缀保持字节级稳定,provider 的 prompt cache 能持续命中。

同时修复了会被更深清理放大的保护配额缺陷。最近结果保护配额此前按“历史 + 待发送结果”计算,携带足够多待发送结果的 ToolResult 轮次会让已提交历史完全失去保护;它还把错误响应、既有占位符、空输出这些永远不会被清理的条目计入配额,任由它们挤占真实近期输出的保护名额;而字节在文本输出之外的纯媒体结果(图片/PDF 读取)仍保持受保护。现在配额只从具有实际可清理内容的已提交结果中选取。驱逐上报中的文件驻留背书改为严格保守:待发送结果以及保留的 read/edit 结果都无法证明文件完整字节仍在历史中——只有保留的 write 结果可以(其调用参数携带完整 content)——因此所有模糊情形下文件读取快速路径一律解除,最多多付出一次冗余重读。

为了可观测性,size 清理的元数据与调试日志现在会报告水位目标,并在受保护结果使总量高于水位时标注软超限。配置描述已同步更新。

Why it's needed

两天窗口的生产流量分析显示,长活跃会话的缓存率被钉在约 5.43%(只有固定的约 37,840 token 头部还能命中),而同样大上下文但没有历史改写的会话缓存率为 99.90%;该模式占窗口内全部缓存损失的约 63%。原因就是“清到刚好低于阈值”:预算一旦贴着上限,几乎每一轮都会再抹掉一条旧结果,并使缓存从该位置起失效。改为低水位后,与 #5101 复现器同形态的负载(167 条、每条约 25.5K 字符)从约 148 次改写降到 14 次——约每 11 条平均大小的结果一次。在水位可达的负载下(受保护结果能落在水位之下),触发不会比以前更频繁,每次批量断点在前缀中的位置也不会更靠前;当保护项本身就把总量钉在阈值之上时(例如五条受保护的 100K 结果对 500K 阈值),触发可能在连续 checkpoint 上重复出现,退回既有的滚动机制而非改善它;已有回归测试钉住该角落。

Reviewer Test Plan

How to verify

用重复的大型可压缩工具输出把会话推过阈值,然后观察 size 清理调试日志:应当只触发一次、报告清理到约一半阈值并带 target 值,随后保持静默,后续轮次复用已稳定的前缀,而不是每轮都出现一条清理日志。确认当水位以上全部为受保护结果时,超限日志仍会出现且带 cleared 0 与软超限标注。确认一次携带五条及以上待发送结果的 ToolResult 轮次仍会保留最近五条已提交结果。确认 -1(以及未设置新配置时的 legacy 负值 idle 阈值)仍会完全禁用 size 触发。

本地运行过的命令:cd packages/core && npx vitest run src/services/microcompaction/microcompact.test.ts src/core/client.test.ts src/services/fileReadCache.integration.test.ts src/services/memoryPressureMonitor.test.ts(442 通过);cd packages/cli && npx vitest run src/config/settingsSchema.test.ts(34 通过);npm run buildnpm run typechecknpm run generate:settings-schemagit diff --check

Evidence (Before & After)

N/A。非 UI 行为,由聚焦单测覆盖,包括断言 167 条连续 25.5K 字符结果的改写次数恰好降为 14 次的负载仿真、断言两次清理之间 history 对象原样返回的稳态测试,以及评审驱动的回归:尾随零字符结果不得挤占保护名额、待发送 cache-hit 占位符不得压制快速路径解除、保护饱和角落的连续 checkpoint 重复触发。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node.js v24;本地仓库 checkout;仅单元测试,无需额外 sandbox 运行时。

Risk & Scope

  • Main risk or tradeoff: 每次清理会一次性移除更多旧工具输出——清理时刻的保留量从约一个阈值降到约半个阈值——旧输出会比以前更早变成占位符。最近结果下限(现改为从可清理的已提交结果中选取)、待发送结果的不可清理豁免、managed-memory 保留均继续生效。若单轮新增输出超过半个阈值,清理会退化为每轮触发;当受保护结果本身超过阈值时也可能在连续 checkpoint 上触发——两者均等同于既有滚动机制。待发送的同文件重读不再压制快速路径解除,可能多付出一次冗余重读。
  • Not validated / out of scope: 预计的 80–90% token 加权缓存率恢复是待真实流量 replay 验证的估计值;水位仍是 best-effort 软目标,受保护结果可使总量停留其上;该机制仍只是成功可压缩工具输出的字符预算,不是总 prompt 上限——token 估算、自动压缩、上下文超限保护均未改动。
  • Breaking changes / migration notes: 无。不新增配置;水位由 toolResultsTotalCharsThreshold 的一半推导,-1 仍可禁用 size 触发。

Linked Issues

Fixes #8452
Fixes #8463

…cache

Size-triggered microcompaction now clears oldest compactable tool results down to half the threshold instead of stopping just below it, so the conversation prefix stays stable between clearings and provider prompt caches keep matching. The recent-result budget now protects committed results only; pending results no longer consume protection slots but stay counted, uncleared, and live for file-read-cache resolution. Adds the watermark to cleanup metadata and the debug log.

Fixes QwenLM#8463
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /verify

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 39 passed · 0 failed · 39 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:39 通过 · 0 失败 · 39 总计

Verification report

Verification report — PR #8464 (perf(core): clear tool results to a low watermark to preserve prompt cache)

Verdict: merge-ready — 39/39 scripted assertions passed, 0 unexpected outcomes. Verified head: a66751b8c2535a9c8984495f28ebdedc488627a5 (single commit; git rev-list HEAD^1..HEAD^2 = 1, matching the snapshot's commits array, so per-commit attribution holds). One non-blocking Suggestion (see Findings S-1).

中文摘要
  • 结论merge-ready。断言计数见 verdict 行;1 条非阻塞 Suggestion(软超限日志标记无测试钉住,见 S-1)。
  • A/B 结论:见 "Central claim + A/B" 表与 01-ab-workload-base-vs-head.png。负载仿真中 base 贴着阈值每轮改写、head 按水位批量清理(改写次数见 W 行),且 head 未触发轮次全部原样返回同一 history 对象(前缀字节稳定,见 W prefix stability 行)。保护配额修复经 P1 行证明:待发送批单独超过阈值时 base 抹掉全部已提交结果,head 保留最近 5 条。
  • Findings:S-1 (soft-exceeded) 标记字符串无测试钉住(M4 突变存活,见 02-mutation-matrix.png)——纯观测性缺口,不阻塞;S-2 描述中测试计数陈旧,无实质影响。
  • 未覆盖范围:真实 provider 缓存命中率与流量 replay 估计(需生产流量,PR 自身也列为 out of scope);全仓 lint/测试未跑(仅受影响 workspace 的测试与 typecheck);base worktree 已按规程删除,harness 的 base 侧不可复跑(日志与截图已留存)。

Central claim + A/B

Central claim: clearing oldest compactable tool results down to floor(threshold/2) (instead of stopping just below the threshold) keeps the conversation prefix byte-stable between clearings, amortizing history rewrites; the PR cites ~148 → 14 rewrites for 167 sequential 25.5K-char results. Secondary claims: (1) pending results no longer consume keepRecent protection slots; (2) cleanup metadata/log report the watermark with a soft-exceeded marker.

Environment: head = prebuilt dist/ at merge head; base = scratch worktree at HEAD^1 (07ba18ee8) with only packages/core recompiled (23 s, clean), wired to the root node_modules (PR leaves package.json/lockfile untouched, so the install is a clean control). The harness imports each tree's dist/src/services/microcompaction/microcompact.js by direct path — no @qwen-code/* workspace symlink is crossed; capture 01 prints the realpath of both loaded modules, both inside their own tree. Base worktree removed after capture; raw logs in logs/.

Witness: evidence/01-ab-workload-base-vs-head.png (live re-run of harness/ab-driver.mjs, 30/30).

Cell Oracle base (07ba18ee8) head (a66751b8c)
W: 167 turns × 25.5K pending result, threshold 500K, keep 5 compactions / actual rewrites 148 / 148 (rides the limit, rewrites every turn) 14 / 14
W prefix stability no-meta turns returning the same history object 19/19 (only pre-threshold turns) 153/153
P1: 10×30K committed + 11×50K pending (pending alone > threshold) cleared / kept / last-5 committed 10 / 0 / wiped to placeholders 5 / 5 / intact
P2: PR fixture 12×25.5K + 5×50K pending cleared / kept / charsAfter 3 / 0 / 229,500 (bug masked by threshold-stop) 7 / 5 / 127,500
B1 -1 threshold; B2 legacy negative idle; B3 exactly-at-threshold meta absent absent / absent / absent absent / absent / absent
B4 one char over, single protected result meta with cleared 0 yes / 0 yes / 0, watermark 250,000
B5 hostile: 12 turns × 300K (> half threshold) compactions 11 11 (never fires more often)
B6 threshold 101 watermark / cleared / after n/a / 1 / 70 50 / 2 / 30
FR: pending re-read of /proj/same.ts, keep slot absorbed by older other-file read (#4239 seam) cleared / evictedReadPaths 2 / ['/proj/other.ts'] 1 / []

The W cell reproduces the PR's claimed numbers exactly (148 and 14), and the P1 cell shows the protection-accounting fix is load-bearing in the regime the threshold-stop previously masked: with pending alone over the threshold, base blanked all committed results including the five most recent (toolsKept=0), head keeps exactly five.

Reviewer Test Plan walk (per step): ① size debug log fires once then stays silent — W cell (14 metas, 153 silent turns) ✔; ② totals-exceeded log with cleared 0 + soft-exceeded marker — B4 proves the cleared 0 shape and the target value; the marker string itself is not pinned by any test (S-1), verified manually-reachable via B4/P1 (virtualAfter 500,001 / 700,015 > 250,000); ③ ToolResult turn with ≥5 pending keeps 5 most recent committed — P2 ✔; ④ -1 and legacy negative idle disable — B1/B2 ✔.

Findings

S-1 (Suggestion, non-blocking): the (soft-exceeded) marker is the one guard no test pins. Repro: in packages/core/src/core/client.ts, replace the targetNote ternary's (virtualAfter > m.toolResultsLowWatermark ? ' (soft-exceeded)' : '') suffix with '', then cd packages/core && npx vitest run src/core/client.test.ts src/services/microcompaction/microcompact.test.ts → 385/385 green (logs/m4-marker-only-removed.log, witness evidence/02-mutation-matrix.png). Classification: coverage gap, not dead code — the branch is reachable (B4 and P1 head cells both have virtualAfter > watermark, so the marker would print today) but nothing asserts it; a future edit could silently drop the marker. The coarser M3 (remove the whole target note) is killed by exactly one test, so the target half is pinned and only the marker half is not. The PR's own test plan covers the marker manually. Suggested fix, if desired: extend the existing client log test with a fixture where protected results keep the virtual total above the watermark (the B4 shape) and assert stringContaining('(soft-exceeded)').

S-2 (cosmetic): stale test counts in the description. The body cites 439 core / 34 cli tests; at the verified head the same commands run 460 / 40 (all passing). The numbers were evidently written mid-development; no action needed beyond awareness.

Not covered

  • Projected 80–90% token-weighted cache-rate recovery — requires replay against real provider traffic; untestable in this container and explicitly out of scope in the PR. The prefix-stability oracle (same history object returned between clearings) is the local proxy for it.
  • Real provider prompt-cache measurement — same reason; the A/B measures rewrites, the cause side of cache loss, not provider-side hits. This reproduces the rewrite mechanism, not provider behavior.
  • Full-repo gates — only the affected workspaces were tested/typechecked (core 460 + cli 40 tests, tsc --noEmit in both). Repo-wide lint/test/preflight were not run; the PR's own CI covers them.
  • Base-side re-runs — the scratch base worktree was removed after capture per protocol, so harness/ab-driver.mjs can only re-run the head arm now; base numbers are preserved in logs/ and evidence/01-*.png.
  • Interactive/daemon E2E — the changed surface is pure history-rewriting logic driven identically by the client path exercised in client.test.ts (315 tests green); a live-daemon run would add nothing to the measured cells.

Methodology

Node v22.23.2 in the CI verify container; merge-ref checkout (HEAD = 7c8af4e84, base 07ba18ee8, head a66751b8c). Head side = prebuilt dist/; base side = git worktree add tmp/base-tree HEAD^1 + npm run build -w equivalent (packages/core only, logs/base-core-build.log), node_modules symlinked from the root install (lockfile unchanged by the PR). harness/run-tree.mjs drives the compiled microcompactHistory of each tree through 9 cells (workload simulation, pending-protection, six boundary cells, #4239 disarm seam); harness/ab-driver.mjs encodes all expectations as scripted comparisons (30 checks) and prints the table captured in evidence/01-ab-workload-base-vs-head.png. Vacuity: four interface-preserving mutants applied one at a time to head source, suites re-run, source restored and re-verified clean after each (logs/m1..m4*.log, rendered in evidence/02-mutation-matrix.png); unmutated control green (460/40, evidence/03-targeted-gates-green.png). Committed settings.schema.json re-generated with npm run generate:settings-schema and diffed byte-identical. Raw logs in logs/, harnesses in harness/.

Evidence images

01-ab-workload-base-vs-head

02-mutation-matrix

03-targeted-gates-green

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on a66751b and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— a66751b 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical. Linked issue #8463 carries quantitative production evidence: long sessions pinned at a ~5.43% cache rate vs 99.90% for comparable sessions without history rewrites, ~63% of all cache loss in the analysis window — plus a concrete second defect (pending tool results consuming the keepRecent protection budget). Maintainers triaged the issue as a P2 bug on the roadmap/context-performance track.

Direction: aligned. The size trigger shipped in #5111 achieves its bounding goal but rewrites the prompt prefix every turn once the budget rides the threshold, defeating provider prompt caches. Prompt-cache preservation is a recurring, explicit focus in this CLI category (Claude Code's changelog has several entries fixing cache invalidation from mid-session changes). No new settings, no public contract change.

Size: core paths touched (packages/core/src/**, packages/cli/src/config/**). ~43 production logic lines (microcompact.ts +28, client.ts +13, settingsSchema.ts +2), 216 test lines, 32 docs/generated-schema lines — well under any threshold.

Approach: low-watermark (hysteresis) clearing is the standard fix for threshold flapping and matches what I'd propose independently. Scope is minimal: the trigger condition, -1 disable semantics, and legacy idle-disable compatibility are untouched — only the clearing depth changes. The keepRecent accounting fix belongs in the same PR, since deeper clearing would have made the pending-slot bug reachable in practice. One honest tradeoff, stated plainly in the PR: retention at clearing time drops from ~threshold to ~threshold/2.

Risk: no elevated risk signals — no changed file matches the revert-correlated high-risk paths.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的问题,不是理论性的。关联 issue #8463 带有定量的生产证据:长会话缓存率被钉在约 5.43%(对比无历史改写会话的 99.90%),占分析窗口内缓存损失约 63%——外加第二个具体缺陷(待发送工具结果占用 keepRecent 保护配额)。维护者已把该 issue 定为 P2 bug 并列入 roadmap/context-performance

方向:对齐。#5111 引入的 size 触发达成了预算上限目标,但预算贴着阈值时每轮都会改写 prompt 前缀,破坏 provider prompt cache。prompt cache 保护在这一类 CLI 中是反复出现的明确重点(Claude Code changelog 有多条修复中途变更导致缓存失效的条目)。无新配置项,无公共契约变更。

规模:触及核心路径。约 43 行生产逻辑(microcompact.ts +28、client.ts +13、settingsSchema.ts +2),216 行测试,32 行文档/生成 schema——远低于任何阈值。

方案:低水位(迟滞)清理是阈值抖动的标准修法,与我的独立提议一致。范围最小:触发条件、-1 禁用语义、legacy idle 禁用兼容都不变——只有清理深度变化。keepRecent 配额修复应放在同一个 PR,因为更深的清理会使"待发送结果占用保护位"的缺陷在实际中可达。PR 也如实说明了一个取舍:清理时的保留量从约阈值降到约阈值一半。

风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at a66751b8c2535a9c8984495f28ebdedc488627a5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal before reading the diff: threshold flapping calls for hysteresis — when the total crosses the threshold, clear down to a low watermark (~half the threshold) instead of "just below", and fix the keepRecent budget to count committed results only. The PR does exactly this, and does it minimally.

What I verified by reading the code at all three microcompactHistory call sites:

  • Only the size-trigger path changes. The watermark and the new keep-set live in planSizeBasedClearing. The idle/force paths (compressFast in geminiChat.ts, the memory-pressure monitor's compact_history step) are structurally untouched, and the sizeOnly ToolResult-turn path in client.ts is the only consumer of the changed behavior.
  • The keep-set union is correct. keepToolRefs now protects the most recent keepRecent committed results plus all pending refs. Pending refs were never clearable (the clearing loop skips contentIndex >= history.length), but joining the set matters for buildKeptFilePaths: a pending re-read of a file keeps that file's read fast-path armed even when the committed keep slots go to other files — the updated issue-Assistant is forced to re-read files it already read, after the session has been idle #4239 disarm test pins exactly this.
  • The accounting stays sound. remainingChars never subtracts pending chars (pending refs are skipped in the loop), so toolResultCharsAfter = remainingChars - pendingChars cannot go negative. When protected results keep the total above the watermark, the loop simply exhausts and the client log marks (soft-exceeded) — covered by the best-effort test, and the cleared 0 totals-exceeded log path is preserved by the size-path exemption from the two early returns.
  • Byte-stability between clearings holds. The trigger condition is unchanged (totalChars <= threshold returns the same history array reference), and the new steady-state test asserts history identity across a below-threshold checkpoint — that is the property the prompt cache actually depends on.
  • The amortization oracle is concrete. The 167 × 25.5K-char simulation asserts exactly 14 rewrites vs the ~148 the old stop condition produced — the central claim is pinned by a test, not just prose.

No correctness, security, or convention issues found. The MicrocompactMeta change is additive, and the settings description is kept in sync across the schema source, the generated VS Code schema, and the docs page. No drive-by changes in the diff.

Test evidence

At the reviewed commit, the fork-approval gate leaves only the Linux unit suite running — macOS/Windows and integration jobs are skipped, and nothing has failed:

Final CI results for a66751b (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The main unit suite is still running, so there is no result to quote yet — not verified: final unit-suite outcome on this commit. The author reports 439 + 34 focused unit tests passing locally; that is the author's claim, not independently re-run here (this review never executes PR code).

Sandboxed verification would settle the one claim static review and unit CI cannot: that the watermark actually reduces provider-visible history rewrites on a live session. A @qwen-code /verify run is already in flight on this PR (see the verification comment) — its A/B comparison against the base build is the evidence to read before merge. The projected 80–90% cache-rate recovery is the author's estimate pending replay validation, as the PR itself states; the rewrite-count drop (148 → 14) is the part the suite does pin.

中文说明

代码审查:读 diff 之前我的独立提议就是迟滞式低水位清理——越过阈值后清到低水位(约阈值一半)而非"刚好低于阈值",并让 keepRecent 配额只数已提交结果。PR 与之一致且改动最小。在三个 microcompactHistory 调用点逐一核对:

  • 只有 size 触发路径变化:水位与新的 keep 集合都在 planSizeBasedClearing 内;geminiChat 的 compressFast 与内存压力监控的 compact_history 走 idle/force 路径,结构上不受影响。
  • keep 集合并集正确:待发送结果本就不可清理(清理循环跳过 pending),加入集合的意义在于 buildKeptFilePaths 把其文件读取视为存活——即使已提交的保留位被其他文件占用,待发送的重新读取仍保持该文件的快速路径 armed(更新后的 Assistant is forced to re-read files it already read, after the session has been idle #4239 测试钉住了这一点)。
  • 账目自洽:pending 字符不会从 remainingChars 中扣除,toolResultCharsAfter 不会为负;受保护结果使总量高于水位时循环自然耗尽并在日志标注软超限,cleared 0 的超限日志路径由 size 路径对两个 early-return 的豁免保留。
  • 两次清理之间的字节级稳定成立:触发条件未变(不超阈值返回同一数组引用),新的稳态测试断言了 history 对象同一性——这正是 prompt cache 依赖的性质。
  • 摊销预言是具体的:167 条 25.5K 字符的模拟断言恰好 14 次改写(旧停止条件约 148 次)——核心主张由测试而非文字背书。

未发现正确性、安全性或规范问题;元数据字段为纯增量,配置描述在 schema 源、生成的 VS Code schema、文档三处同步;diff 无夹带改动。

测试证据:受 fork 审批门限制,当前仅 Linux 单元测试在运行(macOS/Windows/集成测试被跳过),暂无失败。主套件尚未出结果——本提交上的最终单测结果未验证。作者本地报告 439+34 个相关单测通过——这是作者声明,非本审查独立复跑(本审查从不执行 PR 代码)。

沙箱验证将解决静态审查与单测 CI 无法覆盖的核心主张:水位是否真的减少了真实会话中 provider 可见的历史改写。本 PR 已有一个进行中的 @qwen-code /verify 运行(见验证评论)——其与 base 构建的 A/B 对比是合并前应读的证据。80–90% 缓存率恢复是作者待回放验证的估算(PR 已如实说明);改写次数下降(148 → 14)是测试已钉住的部分。

Qwen Code · qwen3.8-max

Reviewed at a66751b8c2535a9c8984495f28ebdedc488627a5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean review throughout; what's left unverified is procedural (unit suite still running, live A/B verify run still in flight), not substantive doubt.

This PR does one thing, minimally. When the tool-result budget crosses the threshold it now clears down to half the threshold instead of stopping at "just below" — the standard hysteresis fix for threshold flapping — and it fixes the protection-accounting bug that deeper clearing would otherwise have exposed. That matches the proposal I wrote down before reading the diff; I found no simpler path. Every line in the diff earns its place: the behavior change, the tests pinning it (including the 167-result workload simulation asserting exactly 14 rewrites), and the settings description kept in sync across three places. No drive-bys, no new settings, no public contract change.

The motivating problem is real and measured — the linked issue carries production cache-rate data and was maintainer-triaged as P2 on the context-performance roadmap — not hypothetical. The honest cost, old outputs becoming placeholders earlier (retention at clearing drops from ~500K chars to ~250K), is disclosed in the PR itself.

Two reservations, neither blocking: the headline 80–90% cache-rate recovery is an estimate pending replay validation (the mechanism it rests on — rewrite count dropping from ~148 to 14 — is the part the tests pin), and the unit suite for this commit hasn't finished. The in-flight @qwen-code /verify A/B run should settle the live-behavior question before merge.

Verdict: approve. Approval is deferred until CI lands green on a66751b8c2535a9c8984495f28ebdedc488627a5.

中文说明

反思:这个 PR 只做一件事且改动最小——工具结果预算越过阈值时清理到阈值一半(低水位迟滞,阈值抖动的标准修法),而不是清到"刚好低于阈值",同时修复了更深清理会暴露的保护配额缺陷。这与我读 diff 前写下的独立提议一致,没有找到更简路径。diff 每一行都必要:行为变更、钉住它的测试(包括断言恰好 14 次改写的 167 条结果负载模拟)、三处同步的配置描述。无夹带改动、无新配置项、无公共契约变更。

解决的问题真实且经过测量——关联 issue 带有生产缓存率数据,已被维护者定为 P2 并列入 context-performance roadmap——不是假设性问题。诚实的代价——旧输出更早变成占位符(清理时保留量从约 500K 字符降到约 250K)——PR 本身已明确披露。

两点保留,均不构成阻塞:80–90% 缓存率恢复是待回放验证的估算(其依赖的机制——改写次数从约 148 降到 14——正是测试钉住的部分);本提交的单元测试套件尚未跑完。进行中的 @qwen-code /verify A/B 运行应能在合并前解决真实行为问题。

结论为 approve;因 CI 未完成,正式批准推迟到被审提交上 CI 全绿后自动执行。

Qwen Code · qwen3.8-max

Reviewed at a66751b8c2535a9c8984495f28ebdedc488627a5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot has neither a verdict nor a deferral on a66751b8c2535a9c8984495f28ebdedc488627a5 — no APPROVED, CHANGES_REQUESTED, or COMMENTED review of its own. A DISMISSED one does not count: dismiss_stale_reviews voids the bot's approval on every push, which is exactly when a fresh one is needed. If this re-run was meant to review or approve, it did not, and an approval left by another account is a separate vote that does not count as the bot's own.

⚠️ 机器人在 a66751b8c2535a9c8984495f28ebdedc488627a5既没有裁决也没有 defer —— 没有属于它自己的 APPROVEDCHANGES_REQUESTEDCOMMENTED 评审。DISMISSED 不算:dismiss_stale_reviews 会在每次推送时作废机器人的批准,而那恰恰是需要一次新批准的时刻。如果这次重跑本应评审或批准,那么它没有做到;而其他账号留下的批准是另一张票,不能算作机器人自己的。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 82 passed · 0 failed · 82 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:82 通过 · 0 失败 · 82 总计

Verification report

Verification report — PR #8464 (perf(core): clear tool results to a low watermark to preserve prompt cache) — follow-up round

Verdict: merge-ready — 82/82 scripted assertions passed, 0 unexpected outcomes. Verified head: a66751b8c2535a9c8984495f28ebdedc488627a5 (single commit; the snapshot's commits array carries exactly that OID, and the shallow git rev-list HEAD^1..HEAD^2 agrees, so per-commit attribution holds). The verified head and base are byte-identical commits to the previous round (a66751b8c… / 07ba18ee8…) — the strongest possible input-closure proof — and every carried measurement was nonetheless rebuilt and re-run from scratch in this round. Two non-blocking findings carried forward, both still standing (S-1, S-2).

中文 — 判定:✅ 通过 · 可合入(agent 判定)· 跟进轮
  • 结论merge-ready,82/82 脚本断言通过。本轮验证的 head/base 与上一轮为同一对提交(git 对象级完全一致),但所有测量均在本轮重新构建、重新执行,未沿用旧报告数字。
  • A/B 结论:见 "Central claim + A/B" 表与 01-ab-workload-base-vs-head.png。负载仿真 base 每轮改写(148 次),head 按水位批量清理(14 次),未触发轮全部原样返回同一 history 对象(153/153,前缀字节稳定)。保护配额修复在 P1/P2/C2 三格证明为 load-bearing;新增探针 C1/C2 验证了生产实际的"单个 user Content 批量携带多个 functionResponse"形状,与数组形状行为完全一致。
  • 既往 findings 状态:S-1((soft-exceeded) 标记无测试钉住)与 S-2(描述中测试计数陈旧)均复测为仍存在,均为非阻塞;S-1 本轮额外完成了反向突变证明(钉住该标记的 fixture 在 head 上绿、在突变体上红)。
  • 未覆盖范围:真实 provider 缓存命中率与流量 replay(PR 自身列为 out of scope);全仓门禁未跑(仅受影响 workspace);两臂的完整 npm run build 在本容器中因一个与 PR 无关的既有依赖类型错误(@lydell/node-pty TS7016)无法跑通——经 head 侧同条件重建 A/A 对照证实为环境问题,两臂表现完全一致,改用已完整产出的 dist 模块做 A/B(见 Methodology)。

Previous-finding status (follow-up round)

# Finding (previous round) Severity Status at this head (a66751b8c, unchanged)
S-1 The (soft-exceeded) log marker is the one guard no test pins Suggestion, non-blocking Stands — re-measured: mutant M4 (marker-only removed) survives 385/385 (logs/m4-marker-only-removed.log, 02-mutation-matrix.png). New this round: the reverse-mutation proof — adding expect(stringContaining('target 250000 (soft-exceeded)')) to the existing 'logs size overages when protected results leave nothing to clear' test is green on head (logs/marker-fixture-on-head.log) and red on the M4 mutant, exactly that test (logs/marker-fixture-on-m4.log). The pinning fixture is one added expect; classification unchanged: coverage gap, branch reachable (B4/P1/C2 cells all have virtualAfter > watermark).
S-2 Stale test counts in the description (439 core / 34 cli) Cosmetic Stands — the same commands run 460 / 40 at this head (all passing); the description's numbers are unchanged and still mid-development values. No action needed.

Central claim + A/B

Central claim: clearing oldest compactable tool results down to floor(threshold/2) (instead of stopping just below the threshold) keeps the conversation prefix byte-stable between clearings, amortizing history rewrites (PR cites ~148 → 14 for 167 sequential 25.5K-char results). Secondary claims: (1) pending results no longer consume keepRecent protection slots (committed-only budget) while still counting toward the size decision, staying uncleared, and keeping their file reads live; (2) cleanup metadata/log report the watermark with a soft-exceeded marker.

Environment: head = prebuilt dist/ at the merge head; base = scratch worktree at HEAD^1 (07ba18ee8), packages/core recompiled there (see Methodology for the one environmental build error and the A/A control). The harness imports each tree's dist/src/services/microcompaction/microcompact.js by direct path; all of that module's runtime imports are relative paths inside packages/core, so no @qwen-code/* workspace symlink is crossed — capture 04 prints the realpath of both loaded modules, both inside their own tree. Witness: evidence/01-ab-workload-base-vs-head.png (live run, 54/54).

Cell Oracle base (07ba18ee8) head (a66751b8c)
W: 167 turns × 25.5K pending result, threshold 500K, keep 5 compactions / actual rewrites / prefix-stable turns 148 / 148 / 19 (rides the limit) 14 / 14 / 153 (same history object returned)
P1: 10×30K committed + 11×50K pending (pending alone > threshold) cleared / kept / charsAfter 10 / 0 / 0 (committed wiped) 5 / 5 / 150,000
P2: PR fixture 12×25.5K + 5×50K pending (array form) cleared / kept / charsAfter 3 / 0 / 229,500 (bug masked by threshold-stop) 7 / 5 / 127,500
C1 (new): P2 as ONE user Content batching 5 functionResponse parts — the production shape createUserContent(requestToSend) sends cleared / kept / charsAfter 3 / 0 / 229,500 (≡ array form) 7 / 5 / 127,500 (≡ array form)
C2 (new): batched 7×70K pending (> keepRecent) + 2×30K committed cleared / kept / charsAfter / meta 2 / 0 / 0 0 / 2 / 60,000 / present
B1 -1 threshold; B2 legacy negative idle; B3 exactly-at-threshold meta absent absent / absent / absent absent / absent / absent (+ same object)
B4 one char over, everything protected meta / cleared / watermark present / 0 / n-a present / 0 / 250,000
B5 hostile: 12 turns × 300K (> half threshold) compactions 11 11 (never fires more often)
B6 threshold 101 watermark / cleared / after n-a / 1 / 70 50 / 2 / 30
FR: pending re-read of /proj/same.ts, keep slot absorbed by older other-file read (#4239 seam) cleared / evictedReadPaths 2 / ['/proj/other.ts'] 1 / []

The W cell reproduces the PR's claimed numbers exactly (148 and 14). The protection fix is load-bearing in the regime the threshold-stop previously masked: with pending alone over the threshold, base blanked all committed results including the five most recent; head keeps exactly five (P1). The two new cells cover the shape production actually sends — one user Content carrying the whole ToolResult batch — and behave identically to the array fixtures on both arms, including the batched-sibling case C2 where base wipes both committed results while head clears nothing.

Reviewer Test Plan walk (per step): ① size debug log fires once then stays silent — W cell (14 metas, 153 silent same-object turns) ✔; the target N log shape is pinned by the client test (mutant M3 removing it is killed by exactly one test); ② totals-exceeded log with cleared 0 + soft-exceeded marker — B4 proves the cleared 0 + watermark shape and virtualAfter (500,001) > watermark (250,000); the marker string remains unpinned (S-1), now with the pinning fixture identified; ③ ToolResult turn with ≥5 pending keeps the 5 most recent committed — P1/P2/C1/C2 ✔, including the batched single-Content shape; ④ -1 and legacy negative idle disable — B1/B2 ✔.

Corrections

None this round — no inaccurate descriptions of the code found in prior review material.

Findings

S-1 (carried, Suggestion, non-blocking): the (soft-exceeded) marker is still the one guard no test pins. Re-measured at the unchanged head: removing only the marker (mutant M4, target value left intact) leaves both suites green at 385/385, while removing the whole target note (M3) is killed by exactly one test — so the target half is pinned and the marker half is not. New evidence this round (reverse mutation): the fixture that would pin it is one added assertion on the existing 'logs size overages when protected results leave nothing to clear' test — expect(mockClientDebugLogger.info).toHaveBeenCalledWith(expect.stringContaining('target 250000 (soft-exceeded)')); it passes on head and fails (exactly that test) when the marker is removed. Classification unchanged: coverage gap, not dead code — the branch prints today in the B4/P1/C2 shapes. Suggested fix unchanged from the previous round: add that one expect.

S-2 (carried, cosmetic): stale test counts in the description. 439 core / 34 cli cited vs 460 / 40 measured at this head (all passing). No action needed beyond awareness.

N-1 (note, environmental, not attributable to the PR): a from-scratch npm run build -w packages/core fails in this container with exactly one pre-existing error — TS7016 on @lydell/node-pty in shellExecutionService.ts (a file the PR does not touch; the dependency's exports string hides its top-level types). Proven environmental by an A/A control: an identical from-scratch rebuild of the head tree fails with the byte-same single error (logs/aa-head-rebuild.log vs logs/base-core-build.log), while the prebuilt head dist/ from the workflow's build step works. Since noEmitOnError is unset, both builds still emitted complete JS, which the A/B drives by direct path. This differs from the previous round's "base rebuilt cleanly (23 s)" — the container's dependency state evidently moved between rounds; the discrepancy is recorded rather than papered over.

Not covered

  • Projected 80–90% token-weighted cache-rate recovery — requires replay against real provider traffic; untestable in this container and explicitly out of scope in the PR. The prefix-stability oracle (same history object returned between clearings, 153/153) is the local proxy.
  • Real provider prompt-cache measurement — the A/B measures rewrites (the cause side of cache loss), not provider-side hits. This reproduces the rewrite mechanism, not provider behavior.
  • Full-repo gates — only the affected workspaces were tested/typechecked (core 460 + cli 40 tests, tsc --noEmit in both, schema regen parity; witness evidence/03-targeted-gates-green.png). Repo-wide lint/test/preflight were not run; the PR's own CI covers them.
  • Full npm run build on either arm — see N-1: impossible in this container for a reason unrelated to the PR (A/A-proven); the A/B used the emitted dist of the module under test, whose runtime import closure is four relative files.
  • Base-side re-runs — the scratch base worktree was removed after capture per protocol; base numbers are preserved in logs/ and evidence/01-*.png. harness/ab-driver.mjs re-runs the head arm only from this point.
  • Interactive/daemon E2E — the changed surface is pure history-rewriting logic driven identically by the client path exercised in client.test.ts (315 tests green); a live-daemon run would add nothing to the measured cells.
  • Per-commit attribution beyond the aggregate — single-commit PR, so nothing to decompose.

Methodology

Node v22.23.2 in the CI verify container; merge-ref checkout (HEAD = 7c8af4e84, base 07ba18ee8, head a66751b8c); snapshot OIDs match local HEAD^1/HEAD^2 exactly and the head equals the previous round's verified head (input closure identical at the git-object level). Head side = prebuilt dist/; base side = git worktree add tmp/base-tree HEAD^1 + tsc rebuild of packages/core wired to the root node_modules (lockfile untouched by the PR) with the nested conflict-pin packages/core/node_modules symlinked in (base and head lockfiles byte-identical). Both from-scratch builds exit 1 on the same single pre-existing TS7016 (N-1, A/A-proven) and both emit complete JS; harness/ab-driver.mjs drives the compiled microcompactHistory of each tree through 12 cells (workload, two pending-protection shapes, two new batched-shape probes, six boundary cells, #4239 disarm seam) with 54 scripted comparisons. Mutations M1–M4 were applied one at a time to head source, suites re-run, source restored via git checkout and re-verified green (385/385) after each; M3 is the positive control, M4 the survivor, and the S-1 fixture proof ran the added assertion on both head and mutant. Gates: harness/gates.sh (both suites, both tsc --noEmit, schema regeneration parity via sha256). harness/final-assertions.mjs consolidates every expectation into one scripted run (82 checks, witness evidence/04-consolidated-assertions-82-0.png), re-invoking the A/B driver live; raw logs in logs/, harnesses in harness/, captures in evidence/.

Evidence images

01-ab-workload-base-vs-head

02-mutation-matrix

03-targeted-gates-green

04-consolidated-assertions-82-0

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot qwen-code-ci-bot 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.

LGTM, looks ready to ship — CI landed green after the review. ✅

@doudouOUC
doudouOUC enabled auto-merge August 3, 2026 14:40

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Blocking review findings at a66751b8c2535a9c8984495f28ebdedc488627a5: the high/low-watermark direction is sound for the reported uniform workload, but the current implementation has two correctness regressions described inline. I also found a counterexample to the PR body's universal "never worse / trigger fires no more often" claim: with H=500K, L=250K, K=5, committed outputs of 1 + 5×100K chars and a 1-char pending result, the parent clears two results in the first checkpoint and is stable on the next one, while this head clears the 1-char result first, remains at 500001 > H, and rewrites another old result at the next checkpoint. Please narrow that guarantee (or add state/logic for an unreachable watermark) and add a cross-checkpoint regression test. The 167×25.5K 148 → 14 result remains valid for that workload, but it does not establish monotonic behavior for all workloads or the projected provider cache-hit recovery.

Comment thread packages/core/src/services/microcompaction/microcompact.ts Outdated
Comment thread packages/core/src/services/microcompaction/microcompact.ts Outdated
…ng refs

Review follow-up for the low-watermark change: keepRecent now selects from committed results that are actually clearable (positive, successful, uncleared output), so trailing errors, prior placeholders, and empty outputs no longer absorb protection slots. Pending refs are dropped from the keep set entirely — a pending read may be a cache-hit placeholder rather than file bytes, so it must not suppress eviction reporting; over-disarming only costs a redundant re-read. Adds regression tests for both plus the protected-saturation consecutive-trigger corner.
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Re the review-body counterexample (1 + 5×100K committed, 1-char pending, K=5): confirmed and adopted in 86db1b7. The universal "never worse / trigger fires no more often" claim was wrong — it holds only when the watermark is reachable (protected results fit under it). The PR body now states the narrowed guarantee, and a cross-checkpoint regression test pins the protected-saturation corner: the trigger fires on two consecutive checkpoints (clearing the 1-char result, then the 100K result that rotated out of the protection window) and stabilizes once the total drops back under the threshold — matching the pre-watermark rolling regime rather than improving on it. No extra state machinery was added for the unreachable-watermark case, per the option you offered. The 167×25.5K result (148 → 14) is unchanged and now sits alongside these bounds in the body. All three findings from this review round are addressed: keep-slot selection from clearable committed results, conservative disarm for pending same-path reads, and the narrowed monotonicity claim.

Pin the (soft-exceeded) log marker with the one-line assertion suggested by the sandboxed verification report (finding S-1): the all-protected overage test now asserts 'target 250000 (soft-exceeded)', killing the surviving mutant M4.

@qwen-code-ci-bot qwen-code-ci-bot 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.

Test Plan (not a blocker): 442 passed — this review observed 15492, 17835, 473 passed; 34 passed — this review observed 15492, 17835, 473 passed.

中文说明

Test Plan(非阻断):442 passed — this review observed 15492, 17835, 473 passed; 34 passed — this review observed 15492, 17835, 473 passed

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment thread packages/core/src/services/microcompaction/microcompact.ts
Comment thread packages/core/src/core/client.test.ts

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): 442 passed — this review observed 473 passed; 34 passed — this review observed 473 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):442 passed — this review observed 473 passed; 34 passed — this review observed 473 passed

— qwen3.8-max via Qwen Code /review (v0.21.3)

Comment thread packages/core/src/services/microcompaction/microcompact.ts
Comment thread packages/core/src/services/microcompaction/microcompact.ts Outdated
Comment thread packages/core/src/core/client.ts

@yiliang114 yiliang114 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.

The low-watermark direction and the current committed/pending protection fixes look sound. I found two blocking context-integrity regressions in the current head: media-only tool results can fall out of keepRecent, and a kept edit can suppress eviction after the full read is removed. Details inline.

Comment thread packages/core/src/services/microcompaction/microcompact.ts Outdated
Comment thread packages/core/src/services/microcompaction/microcompact.ts Outdated
Two P1 context-integrity fixes from review: (1) media-only tool results (image/PDF reads with empty text output and bytes on functionResponse.parts) stay in the idle-path keepRecent candidates instead of being dropped by the zero-char filter; (2) only write_file results vouch for file residency in kept-path accounting — edit calls carry just old/new snippets while still setting the cache's sticky full-read flags, so a kept edit can no longer suppress eviction reporting after the full read is blanked. Regression tests for both.
DragonnZhang
DragonnZhang previously approved these changes Aug 4, 2026

@DragonnZhang DragonnZhang 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.

Reviewed exact head e596dc182db2a08641b8d4b1ffa6538f694d4b59 and the current GitHub merge result.

No blocking findings. The high/low-watermark implementation addresses #8452 as intended: the 167 × 25.5K workload drops from 148 history rewrites on the merge base to exactly 14, and non-compaction checkpoints return the history unchanged. I also checked the committed-vs-pending keep budget, zero-char and media-only protection, and conservative file-read-cache disarming. The three production consumers remain consistent: GeminiClient pre-send checkpoints, GeminiChat /compress-fast, and MemoryPressureMonitor history compaction.

Verification on the current merge result: 466 focused core tests passed, 40 CLI settings-schema tests passed, core typecheck passed, and git diff --check passed.

Non-blocking follow-up, intentionally deferred after the existing review rounds: on idle/force cleanup, an error response carrying nested media can still consume a keepRecent slot. This reproduces identically on the merge base, so it is pre-existing rather than introduced by this PR and should not hold this cache fix.

Pin the absence of the (soft-exceeded) marker at the exact watermark boundary: clearing that lands the virtual total exactly on the watermark must not be flagged. Kills the >= and always-true mutants of the marker condition that previously survived the suite.
DragonnZhang
DragonnZhang previously approved these changes Aug 4, 2026

@qwen-code-ci-bot qwen-code-ci-bot 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.

Test Plan (not a blocker): 442 passed — this review observed 15398, 17831, 473 passed; 34 passed — this review observed 15398, 17831, 473 passed.

中文说明

Test Plan(非阻断):442 passed — this review observed 15398, 17831, 473 passed; 34 passed — this review observed 15398, 17831, 473 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment thread packages/core/src/services/microcompaction/microcompact.ts
Comment thread packages/core/src/services/microcompaction/microcompact.test.ts
Comment thread packages/core/src/services/microcompaction/microcompact.ts
doudouOUC added a commit to doudouOUC/qwen-code that referenced this pull request Aug 4, 2026
@doudouOUC
doudouOUC force-pushed the perf/microcompact-low-watermark branch from ebf7ae9 to 0c5842f Compare August 4, 2026 11:14
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — real-stack A/B (macOS, local build)

I built both sides from source and ran the real qwen CLI (headless and TUI) against a local OpenAI-compatible provider that simulates an actual prefix cache, so the cache numbers below are end-to-end, not model-internal assertions.

  • base = merge-base 06cc41ee3f (main), head = efb8e398c0 (this PR)
  • Each side: npm ci + npm run build, then node packages/cli/dist/index.js
  • Provider: local HTTP server recording every /v1/chat/completions body; a trie over all prompt prefixes seen so far, quantized to 128-token blocks, returned as usage.prompt_tokens_details.cached_tokens — which the CLI surfaces as cachedContentTokenCount / /stats → Cached
  • Workload: 60 sequential read_file calls, ~12.6K chars each; toolResultsTotalCharsThreshold=200000, toolResultsNumToKeep=5, idle trigger disabled (-1) so only the size trigger is in play
  • A "history rewrite" is measured externally: request N's prompt diverges from request N-1's before the previous prompt ended, i.e. the provider's cached prefix is invalidated

1. Core claim: the watermark holds the prefix stable

Metric (60 tool calls, identical workload) main PR #8464
provider requests 61 61
history rewrites (prompt-cache prefix breaks) 44 5
[TOOL-RESULT MC] size-cleanup log lines 44 5
prompt tokens sent 3,260,551 2,710,245 (−16.9%)
prompt tokens served from cache 979,328 2,394,880
cache hit rate 30.04% 88.36%

main rewrites history on every single turn from request 18 onward. The PR rewrites at 18, 27, 36, 45, 54 — one per ~9 results, matching the "half the threshold / average result size" prediction. The PR's break set is a strict subset of main's, and the first break lands at the same request (18) on both, so the claim that the trigger fires no more often and no earlier holds on this workload.

real-stack A/B timeline

The same run through the interactive TUI, read off the app's own /stats:

main — Cached: 971,520 (29.4%) PR #8464Cached: 2,386,944 (86.8%)
main /stats PR /stats

2. The protection-accounting fix is a real data-loss fix, not just bookkeeping

Steady-state debug line on the main workload already shows it — main reports kept 4 because the single pending result eats one of the five protection slots; the PR reports kept 5:

main      [TOOL-RESULT MC] ... cleared 1 ... history now 180660 (+12044 pending), kept 4 tool result(s)
PR 8464   [TOOL-RESULT MC] ... cleared 9 ... history now 84308 (+12044 pending), target 100000, kept 5 tool result(s)

Pushed harder — 5 parallel read_file calls per turn against a 60000 threshold, so one turn's batch alone is a full threshold — main loses everything:

protection accounting

main      ... history now 0     (+60220 pending), kept 0 tool result(s)
PR 8464   ... history now 60220 (+60220 pending), target 30000 (soft-exceeded), kept 5 tool result(s)

On main the only full outputs the model still sees are the 5 pending ones; every committed result is a placeholder. The PR keeps the 5 most recent committed results as documented. This is the strongest reason to merge, independent of the cache win.

3. Disable semantics unchanged (byte-identical)

Case main PR #8464
toolResultsTotalCharsThreshold: -1 0 cleanups 0 cleanups
legacy: toolResultsThresholdMinutes: -1, new key unset 0 cleanups 0 cleanups

In both cases the recorded provider request streams are byte-identical between main and the PR (diff clean over all 41 / 58 requests). The legacy case runs until the context-overflow guard fires at the same request with the same estimate (178600 tokens) on both sides — the size trigger genuinely stays off.

4. Soft-exceeded reporting

With threshold=100000 and 30K-char results, five protected results (150K) can never fit under the 50K watermark. All 27 cleanup lines on the PR side carry target 50000 (soft-exceeded), including the cleared 0 ones; the watermark-reachable run has zero soft-exceeded markers. The marker correctly accounts for pending chars (it fires in the parallel case above where history alone is under the watermark).

5. Tests

Unit suites + necessity check + gates
  • PR head, packages/core: microcompact.test.ts, client.test.ts, fileReadCache.integration.test.ts, memoryPressureMonitor.test.ts468 passed / 0 failed
  • Necessity check: copying the PR's microcompact.test.ts onto the base implementation → 15 failed / 62 passed. The failures are exactly the behaviours this PR introduces (watermark derivation, 167-result amortization = 14, pending-vs-keepRecent, zero-char slots, idle-path zero-char guard, read/edit vouching, pending cache-hit placeholder). The new tests genuinely pin new behaviour rather than restating the old.
  • npm run typecheck → exit 0; npm run generate:settings-schema → no diff (schema in sync); git diff --check clean.

Notes / caveats for reviewers

  • The saturated corner is real and the PR is honest about it. With threshold=100000 + 30K results (protections alone pin the total above the threshold), main = 25 rewrites / 29.1% cached, PR = 24 rewrites / 27.9% cached. No improvement, and the PR sends slightly more prompt tokens there because it correctly protects 5 results where main protected 4. This matches the "matching the pre-existing rolling regime" wording in the description; it is a retention trade, not a new failure mode.
  • Deeper clearing means old outputs become placeholders earlier. Visible in §2 (C cells appear one request sooner on the PR side). Expected and documented.
  • The cache simulator quantizes a character-level prefix into 128-token blocks; absolute percentages are approximate, the base-vs-PR comparison is what carries weight. Both sides ran the identical workload against the identical simulator.

Verdict: verified, supports merge. The cache claim reproduces end-to-end on a real build, the disable paths are byte-identical, and the protection fix prevents a case where main blanks 100% of committed tool output.

中文版本

维护者验证 —— 真实环境 A/B(macOS,本地构建)

我把两侧都从源码构建,用真实 qwen CLI(headless 与 TUI 两种方式)跑在一个本地 OpenAI 兼容 provider 上,该 provider 模拟真实前缀缓存,所以下面的缓存数字是端到端实测,而不是对模型行为的断言。

  • base = merge-base 06cc41ee3f(main),head = efb8e398c0(本 PR)
  • 两侧各自 npm ci + npm run build,然后 node packages/cli/dist/index.js
  • Provider:本地 HTTP 服务,记录每个 /v1/chat/completions 请求体;对历史所有 prompt 前缀建 trie,按 128 token 分块量化,通过 usage.prompt_tokens_details.cached_tokens 回传,CLI 会把它显示为 /stats → Cached
  • 负载:60 次连续 read_file,每次约 12.6K 字符;toolResultsTotalCharsThreshold=200000toolResultsNumToKeep=5、idle 触发用 -1 关掉,只留 size 触发
  • "历史改写"是从外部测的:第 N 个请求的 prompt 在上一个 prompt 结束之前就发生分叉,即 provider 缓存前缀失效

1. 核心结论:低水位确实稳住了前缀

指标(60 次工具调用,负载完全相同) main PR #8464
provider 请求数 61 61
历史改写次数(缓存前缀断裂) 44 5
[TOOL-RESULT MC] size 清理日志条数 44 5
发送的 prompt token 3,260,551 2,710,245(−16.9%
命中缓存的 prompt token 979,328 2,394,880
缓存命中率 30.04% 88.36%

main 从第 18 个请求起每一轮都改写历史。PR 只在 18、27、36、45、54 改写——约每 9 条结果一次,与"半阈值 / 平均结果大小"的预测吻合。PR 的断裂集合是 main 的严格子集,且两侧首次断裂都落在第 18 个请求,因此"触发不会更频繁、断点不会更靠前"这一说法在该负载下成立。

交互式 TUI 里跑同样负载,直接看 app 自己的 /stats:main 是 Cached: 971,520 (29.4%),PR 是 Cached: 2,386,944 (86.8%)(截图见英文部分)。

2. 保护配额修复是真实的数据丢失修复,不只是记账问题

主负载的稳态日志已经能看出来——main 报 kept 4,因为那一条待发送结果占掉了 5 个保护名额之一;PR 报 kept 5

再加压——每轮 5 个并行 read_file、阈值 60000,即单轮批次本身就等于一整个阈值——main 全军覆没

main      ... history now 0     (+60220 pending), kept 0 tool result(s)
PR 8464   ... history now 60220 (+60220 pending), target 30000 (soft-exceeded), kept 5 tool result(s)

在 main 上,模型能看到的完整输出只剩那 5 条待发送结果,所有已提交结果都是占位符。PR 按文档保留了最近 5 条已提交结果。抛开缓存收益不谈,这是最值得合并的理由。

3. 禁用语义未变(字节级一致)

场景 main PR #8464
toolResultsTotalCharsThreshold: -1 0 次清理 0 次清理
legacy:toolResultsThresholdMinutes: -1,新配置未设置 0 次清理 0 次清理

两种场景下,main 与 PR 记录到的 provider 请求流逐字节一致(全部 41 / 58 个请求 diff 无差异)。legacy 场景两侧都在同一个请求上触发上下文超限保护、估算值同为 178600 tokens——说明 size 触发确实被完全关闭。

4. 软超限上报

threshold=100000 配 30K 字符结果时,5 条受保护结果(150K)永远不可能落到 50K 水位以下。PR 侧 27 条清理日志全部target 50000 (soft-exceeded),包括 cleared 0 的那些;而水位可达的那次运行里 soft-exceeded 标记数为 0。该标记也正确计入了 pending 字符(上面并行场景里 history 本身低于水位,标记仍然出现)。

5. 测试

  • PR head,packages/core 四个测试文件 → 468 通过 / 0 失败
  • 必要性验证:把 PR 的 microcompact.test.ts 拷到 base 实现上跑 → 15 失败 / 62 通过,失败项正好是本 PR 引入的行为(水位推导、167 条结果摊薄到 14 次、pending 与 keepRecent、零字符名额、idle 路径零字符保护、read/edit 背书、待发送 cache-hit 占位符)。说明新测试确实钉住了新行为,而不是重述旧行为。
  • npm run typecheck → exit 0;npm run generate:settings-schema → 无 diff;git diff --check 干净。

注意事项 / 保留意见

  • 保护饱和角落真实存在,PR 的描述是诚实的。 threshold=100000 + 30K 结果(受保护项本身就把总量钉在阈值之上)时:main = 25 次改写 / 29.1% 命中,PR = 24 次改写 / 27.9% 命中。没有改善,而且 PR 在这里发送的 prompt token 反而略多——因为它正确保护了 5 条而 main 只保护了 4 条。这与描述里"退回既有滚动机制"的说法一致,属于保留量换取正确性的取舍,不是新的失效模式。
  • 更深的清理意味着旧输出更早变占位符。第 2 节截图里能看到 PR 侧的 C 单元格比 main 早一个请求出现。符合预期且已在描述中说明。
  • 缓存模拟器把字符级前缀按 128 token 分块量化,绝对百分比是近似值,真正有意义的是 base 与 PR 的相对对比;两侧跑的是同一份负载、同一个模拟器。

结论:验证通过,支持合并。 缓存收益在真实构建上端到端复现,禁用路径字节级一致,保护配额修复堵住了 main 会把 100% 已提交工具输出抹成占位符的场景。

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline. Test Plan (not a blocker): 442 passed — this review observed 17101, 475 passed; 34 passed — this review observed 17101, 475 passed.

中文说明

已审查。 建议见行内评论。 Test Plan(非阻断):442 passed — this review observed 17101, 475 passed; 34 passed — this review observed 17101, 475 passed

— qwen3.8-max via Qwen Code /review (v0.21.5)

expect(cleared.parts).toBeUndefined();
});

it('keeps a media-only tool result in the recent-result budget (idle path)', () => {

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 new idle-path media-only handling is only half-tested: this test pins "protected when recent", but "cleared when old" has no test. Failure scenario: a future refactor simplifying the idle zero-char guard to match the size path's unconditional skip — dropping the !(part && hasNestedMedia(part)) term at microcompact.ts:623 — would make media-only image/PDF results (empty text output, bytes on functionResponse.parts) permanently unblankable by idle/force clearing, and the size path never clears zero-char results, so the idle path is the only reclaim mechanism. Nothing would catch it: that mutant survives the entire shipped suite (verified at this commit — 77/77 microcompact + 24/24 client microcompaction tests stay green). Suggested fix: add the companion regression — a media-only read_file result (empty output, parts: [{ inlineData }]) positioned outside the keep window (one newer normal tool result, toolResultsNumToKeep: 1, idle trigger fired), asserting its output becomes MICROCOMPACT_CLEARED_MESSAGE, its functionResponse.parts is stripped, and meta.toolsCleared counts it. That exact test was run against this commit: it passes on the PR code and kills the mutant.

中文说明

[Suggestion] 新增的 idle 路径纯媒体结果处理只测了一半:本测试钉住了"较新时受保护",但"较旧时被清理"没有测试。失败场景:未来若把 idle 的零字符守护简化为与 size 路径一致的无条件跳过——即去掉 microcompact.ts:623 处的 !(part && hasNestedMedia(part)) 项——纯媒体的图片/PDF 结果(文本输出为空、字节在 functionResponse.parts 上)将永远无法被 idle/force 清理抹除,而 size 路径从不清理零字符结果,因此 idle 路径是唯一的回收机制。且现有测试无法发现该回归:该变异体在整个现有测试套件下存活(已在本提交上验证——microcompact 77/77 与 client microcompaction 24/24 全部保持通过)。建议修复:补充配套回归测试——将一个纯媒体 read_file 结果(output 为空、parts: [{ inlineData }])放在保留窗口之外(另有 1 条更新的普通工具结果,toolResultsNumToKeep: 1,触发 idle 清理),断言其输出变为 MICROCOMPACT_CLEARED_MESSAGEfunctionResponse.parts 被剥离、meta.toolsCleared 计入该条。该测试已在本提交上实际运行:在 PR 代码上通过,并能杀死上述变异体。

— qwen3.8-max via Qwen Code /review (v0.21.5)

@yiliang114 yiliang114 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.

LGTM. Watermark math is correct (trigger > threshold, clear oldest-first to floor(threshold/2)), and the vouching tightening to write_file is a genuine dangling-placeholder safety fix. Verified tests have teeth (18-failure mutation check) and cache preservation is pinned by the amortization test. Non-blocking notes: (1) release note should mention the halved steady-state retention (~250K-500K oscillation) for users who tuned the threshold; (2) the soft-exceeded log can repeat per-turn when protections pin total above threshold — optional dedupe.

@doudouOUC
doudouOUC added this pull request to the merge queue Aug 4, 2026
Merged via the queue into QwenLM:main with commit 32e2741 Aug 4, 2026
93 of 94 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.6.

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

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

5 participants