fix(core): clear FileReadCache on every history rewrite path by wenshao · Pull Request #3810 · QwenLM/qwen-code

wenshao · 2026-05-03T14:16:51Z

Summary

Fixes #3805 — "read tool returns no content in long-running sessions."

The FileReadCache (#3717) short-circuits repeated full Reads of an unchanged file by returning a file_unchanged placeholder that points the model at the prior tool result in conversation history. This optimization is correct only as long as that prior tool result is still in history. Anything that rewrites history without invalidating the cache produces a Read that succeeds at the tool layer but delivers no usable content to the model.

#3717 wired the invalidation into two paths (tryCompressChat, startNewSession) but missed five others. After a multi-round audit:

Path	Trigger	Was clearing?
`tryCompressChat`	auto compaction	yes
`Config.startNewSession`	/clear, session resume	yes
`microcompactHistory` call site	idle cleanup (>= 60min)	no -> yes
`GeminiClient.setHistory`	/restore, /load_history	no -> yes
`GeminiClient.truncateHistory`	rewind	no -> yes
`GeminiClient.resetChat`	public API	no -> yes
`stripOrphanedUserEntriesFromHistory`	retry path	no -> yes

Each fix is a single getFileReadCache().clear() at the call site (matching the pre-existing pattern), with a focused regression test in client.test.ts.

Proof the bug is real

fileReadCache.integration.test.ts runs the real ReadFileTool against a real on-disk file, calls the real microcompactHistory, and uses the real FileReadCache — no mocks. The first test deliberately omits the cache clear (i.e. reproduces the pre-fix code path) and asserts the failure:

// STEP 1 — first Read populates the cache and emits real bytes.
const r1 = await tool.createInvocation({ file_path: filePath })
                    .execute(signal);
expect(r1.llmContent).toContain('export function hello');  // ✓

// STEP 2 — build a 6-entry history of read_file results, then run
//          microcompactHistory (idle 90min, keepRecent=1).
const mc = microcompactHistory(history, Date.now() - 90 * 60_000, {
  toolResultsThresholdMinutes: 60,
  toolResultsNumToKeep: 1,
});
expect(mc.meta.toolsCleared).toBe(5);  // ✓ — 5 entries replaced with placeholder

// STEP 3 — pre-fix path: cache is NOT cleared. Read foo.ts again.
const r2 = await tool.createInvocation({ file_path: filePath })
                    .execute(signal);

// THE BUG: the tool returns the placeholder, not the file content.
expect(r2.llmContent).toContain('unchanged since last read in this session');  // ✓
expect(r2.llmContent).not.toContain('export function hello');                  // ✓

The test passes — meaning without the fix, a follow-up Read of an unchanged file after microcompact really does return the placeholder while history no longer holds the bytes. This is the exact failure mode in the issue.

A worst-case test (toolsCleared = 19, kept = 1, surviving entry is a different file) further shows the model is left with zero recoverable bytes anywhere in its reachable context.

Proof the fix works

Same setup, with cache.clear() between the two reads:

const r1 = await tool.createInvocation({ file_path: filePath }).execute(signal);
expect(r1.llmContent).toContain('export function hello');  // ✓

cache.clear();  // the fix

const r2 = await tool.createInvocation({ file_path: filePath }).execute(signal);
expect(r2.llmContent).toContain('export function hello');                       // ✓ — real bytes
expect(r2.llmContent).not.toContain('unchanged since last read in this session'); // ✓

After the fix, the second Read re-emits the real file content instead of the placeholder.

Test output

✓ FileReadCache integration: read after history rewrite
  ✓ returns the file_unchanged placeholder on a follow-up Read after microcompact,
    exposing why the cache must be cleared on history rewrite (6ms)
  ✓ after cache.clear(), a follow-up Read of the same unchanged file re-emits
    the real bytes (1ms)
  ✓ worst case: when microcompact removes every prior read of a file, the
    placeholder leaves zero recoverable bytes for the model (1ms)

Test Files  1 passed (1)
     Tests  3 passed (3)

Reverse-tested

Every new clear() in client.ts was temporarily commented out and the matching unit test in client.test.ts was confirmed to fail. So each clear() is exercised by — and required by — the test suite.

Verification

npm run test --workspace packages/core: 6610 tests pass (6607 prior + 3 new integration)
tsc --noEmit: clean
eslint: clean

Notes

The user's second comment ("Edit tool errors with filenames containing spaces") is a separate report and not addressed here. Edit uses path.resolve / fs APIs that handle spaces correctly; that report needs a concrete repro before it can be triaged.

Microcompaction (the idle-cleanup pass that runs at the start of every new user/cron message) replaces old read_file / shell / glob / grep / edit / write_file tool outputs with a `[Old tool result content cleared]` placeholder. The FileReadCache, however, still records the prior full Reads as "seen in this conversation" — so the next ReadFile of an unchanged file returns the file_unchanged placeholder pointing at bytes the model can no longer retrieve from history. The result is a Read that succeeds at the tool layer but delivers no usable content to the model, which is the failure mode reported in QwenLM#3805 ("read tool returns no content in long-running sessions"). This mirrors the existing post-compaction clear in tryCompressChat — microcompaction has the same "history rewrite invalidates the cache's 'model has seen this' assumption" property, it was just missed when the cache was wired in.

PR1 only patched microcompaction, but a multi-round audit found four more entry points that rewrite history without clearing the cache, producing the same `file_unchanged` placeholder vs. missing-content mismatch. Each is fixed in the same minimal way (clear() at the call site) and covered by a regression test: - GeminiClient.setHistory — /restore checkpoint, /load_history - GeminiClient.truncateHistory — rewind in AppContainer - GeminiClient.resetChat — public API; clearCommand happens to clear the cache via startNewSession beforehand, but other callers have no such guarantee - stripOrphanedUserEntriesFromHistory — Retry path drops trailing user entries that may include read_file functionResponses Also tightened the microcompaction comment ("compactable tool outputs" instead of an enumerated list, since the source of truth is microcompact.COMPACTABLE_TOOLS) and removed caller references per the codebase comment style. Reverse-tested every new clear() by commenting it out and confirming the matching regression test fails.

End-to-end tests using the real ReadFileTool, real FileReadCache, real microcompactHistory, and a real on-disk file. Three cases: 1. Without a cache clear after microcompact, the second Read of an unchanged file returns the file_unchanged placeholder while the prior content has already been wiped from history. Demonstrates the failure mode this PR fixes. 2. After an explicit cache.clear(), the second Read re-emits the real bytes. Demonstrates that the fix works. 3. When microcompact removes every prior read of a file, the placeholder leaves zero recoverable bytes — the model literally cannot find the content anywhere it can reach. These complement the existing unit tests in client.test.ts (which verify the call-site wiring) by proving the end-to-end behaviour through the real code paths, without mocks.

wenshao

⚠️ 审查已从 Approve 降级为 Comment（原因：self-PR — GitHub 不允许对自己 PR 提交 Approve）。审查结论仍为 Approve（无 Critical 问题）。

Per review feedback: the new clear() call sites were silent, leaving no breadcrumb in production debug streams when the cache is dropped. Adds a `[FILE_READ_CACHE] clear after <reason>` log at every clear site (5 new + 1 pre-existing in tryCompressChat) so operators can grep one prefix and see why the cache was invalidated.

wenshao

⚠️ Downgraded from Approve to Comment: self-PR. No Critical issues found — 3 Suggestions below.

测试通过 (72/72)，tsc + eslint 无发现。修复正确且必要。

Per review feedback (deepseek-v4-pro): 1. truncateHistory now skips the cache clear when keepCount >= prevLen, since a no-op truncate leaves the cache valid against the unchanged history. Adds a regression test covering both keepCount==prevLen and keepCount>prevLen. 2. The 6 cache-spy test cases each repeated the same 4-line mock setup. Extract a `mockFileReadCacheClear()` helper so future changes to the FileReadCache mock surface only need one edit. Both are quality-of-implementation tweaks; the underlying fix is unchanged.

Copilot

Pull request overview

This PR fixes a long-running-session failure mode where ReadFileTool can return the file_unchanged placeholder after conversation history is rewritten (compaction/truncation/reset), leaving the model without any reachable copy of the original file bytes.

Changes:

Clear FileReadCache on additional GeminiClient history-rewrite paths (microcompaction, setHistory, truncateHistory, resetChat, retry orphan-strip).
Add unit tests in client.test.ts to assert cache invalidation on these history mutations.
Add an integration test that reproduces the bug end-to-end with real filesystem + real ReadFileTool + real microcompactHistory.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/core/src/core/client.ts	Clears `FileReadCache` on multiple history rewrite paths to prevent stale placeholder reads.
packages/core/src/core/client.test.ts	Adds regression tests ensuring cache invalidation happens on reset/history mutation/microcompaction/retry cleanup.
packages/core/src/services/fileReadCache.integration.test.ts	Adds real-FS integration coverage demonstrating the placeholder failure mode and why cache clears are required.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wenshao

No review findings. Downgraded from Approve to Comment: self-PR (GitHub rejects APPROVE on own PR). — deepseek-v4-pro via Qwen Code /review

Per Copilot review feedback: the previous commit's no-op detection in truncateHistory called this.getChat().getHistory().length, but GeminiChat.getHistory() does a structuredClone of the entire history on every call (line 770 of geminiChat.ts) — paying an O(history) clone purely to read .length. In long-running sessions with hundreds of entries this is a meaningful regression. Adds GeminiChat.getHistoryLength(): O(1), no clone. truncateHistory switches to it. The behaviour (skip clear when keepCount >= prevLen) is unchanged. Also adds: - Unit tests for GeminiChat.getHistoryLength (empty, after addHistory, parity with getHistory().length). - A regression test asserting truncateHistory calls getHistoryLength and NOT getHistory, locking in the perf fix against future drift.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Two issues from copilot review: 1. NaN edge case in truncateHistory cache invalidation. The "did anything actually change?" check was `keepCount < prevLen`, but `Array.slice(0, NaN)` returns [] (history wiped) while `NaN < prevLen` is false. That sequence would wipe the chat but leave the FileReadCache claiming the model has seen the prior reads — exactly the file_unchanged placeholder bug this PR is closing. Switched the check to compare actual post-truncate length (`newLen < prevLen`), which correctly invalidates whenever entries were removed regardless of how `keepCount` was malformed. Added a NaN regression test. 2. The integration test cast `tool` to `unknown` to reach the protected `createInvocation()` method. Switched to the public `tool.buildAndExecute(params, signal)` API so the test exercises the same surface real callers use, including build-time schema validation.

yiliang114

Reviewed the latest head against issue #3805 and the FileReadCache/history-rewrite interaction. The fix covers the relevant history mutation paths without changing the cache semantics outside those invalidation boundaries, and the integration test demonstrates the real stale-placeholder failure mode. Local targeted core tests and typecheck passed, and CI is green. Approving.

…3810)

* fix(core): clear FileReadCache after microcompaction Microcompaction (the idle-cleanup pass that runs at the start of every new user/cron message) replaces old read_file / shell / glob / grep / edit / write_file tool outputs with a `[Old tool result content cleared]` placeholder. The FileReadCache, however, still records the prior full Reads as "seen in this conversation" — so the next ReadFile of an unchanged file returns the file_unchanged placeholder pointing at bytes the model can no longer retrieve from history. The result is a Read that succeeds at the tool layer but delivers no usable content to the model, which is the failure mode reported in #3805 ("read tool returns no content in long-running sessions"). This mirrors the existing post-compaction clear in tryCompressChat — microcompaction has the same "history rewrite invalidates the cache's 'model has seen this' assumption" property, it was just missed when the cache was wired in. * fix(core): clear FileReadCache on every history rewrite path PR1 only patched microcompaction, but a multi-round audit found four more entry points that rewrite history without clearing the cache, producing the same `file_unchanged` placeholder vs. missing-content mismatch. Each is fixed in the same minimal way (clear() at the call site) and covered by a regression test: - GeminiClient.setHistory — /restore checkpoint, /load_history - GeminiClient.truncateHistory — rewind in AppContainer - GeminiClient.resetChat — public API; clearCommand happens to clear the cache via startNewSession beforehand, but other callers have no such guarantee - stripOrphanedUserEntriesFromHistory — Retry path drops trailing user entries that may include read_file functionResponses Also tightened the microcompaction comment ("compactable tool outputs" instead of an enumerated list, since the source of truth is microcompact.COMPACTABLE_TOOLS) and removed caller references per the codebase comment style. Reverse-tested every new clear() by commenting it out and confirming the matching regression test fails. * test(core): integration test for FileReadCache + history rewrite End-to-end tests using the real ReadFileTool, real FileReadCache, real microcompactHistory, and a real on-disk file. Three cases: 1. Without a cache clear after microcompact, the second Read of an unchanged file returns the file_unchanged placeholder while the prior content has already been wiped from history. Demonstrates the failure mode this PR fixes. 2. After an explicit cache.clear(), the second Read re-emits the real bytes. Demonstrates that the fix works. 3. When microcompact removes every prior read of a file, the placeholder leaves zero recoverable bytes — the model literally cannot find the content anywhere it can reach. These complement the existing unit tests in client.test.ts (which verify the call-site wiring) by proving the end-to-end behaviour through the real code paths, without mocks. * chore(core): add traceable debug log for every FileReadCache clear Per review feedback: the new clear() call sites were silent, leaving no breadcrumb in production debug streams when the cache is dropped. Adds a `[FILE_READ_CACHE] clear after <reason>` log at every clear site (5 new + 1 pre-existing in tryCompressChat) so operators can grep one prefix and see why the cache was invalidated. * chore(core): refine truncateHistory cache clear + extract test helper Per review feedback (deepseek-v4-pro): 1. truncateHistory now skips the cache clear when keepCount >= prevLen, since a no-op truncate leaves the cache valid against the unchanged history. Adds a regression test covering both keepCount==prevLen and keepCount>prevLen. 2. The 6 cache-spy test cases each repeated the same 4-line mock setup. Extract a `mockFileReadCacheClear()` helper so future changes to the FileReadCache mock surface only need one edit. Both are quality-of-implementation tweaks; the underlying fix is unchanged. * perf(core): use O(1) getHistoryLength in truncateHistory Per Copilot review feedback: the previous commit's no-op detection in truncateHistory called this.getChat().getHistory().length, but GeminiChat.getHistory() does a structuredClone of the entire history on every call (line 770 of geminiChat.ts) — paying an O(history) clone purely to read .length. In long-running sessions with hundreds of entries this is a meaningful regression. Adds GeminiChat.getHistoryLength(): O(1), no clone. truncateHistory switches to it. The behaviour (skip clear when keepCount >= prevLen) is unchanged. Also adds: - Unit tests for GeminiChat.getHistoryLength (empty, after addHistory, parity with getHistory().length). - A regression test asserting truncateHistory calls getHistoryLength and NOT getHistory, locking in the perf fix against future drift. * fix(core): close NaN hole + use public ReadFileTool API in tests Two issues from copilot review: 1. NaN edge case in truncateHistory cache invalidation. The "did anything actually change?" check was `keepCount < prevLen`, but `Array.slice(0, NaN)` returns [] (history wiped) while `NaN < prevLen` is false. That sequence would wipe the chat but leave the FileReadCache claiming the model has seen the prior reads — exactly the file_unchanged placeholder bug this PR is closing. Switched the check to compare actual post-truncate length (`newLen < prevLen`), which correctly invalidates whenever entries were removed regardless of how `keepCount` was malformed. Added a NaN regression test. 2. The integration test cast `tool` to `unknown` to reach the protected `createInvocation()` method. Switched to the public `tool.buildAndExecute(params, signal)` API so the test exercises the same surface real callers use, including build-time schema validation.

…3810) * fix(core): clear FileReadCache after microcompaction Microcompaction (the idle-cleanup pass that runs at the start of every new user/cron message) replaces old read_file / shell / glob / grep / edit / write_file tool outputs with a `[Old tool result content cleared]` placeholder. The FileReadCache, however, still records the prior full Reads as "seen in this conversation" — so the next ReadFile of an unchanged file returns the file_unchanged placeholder pointing at bytes the model can no longer retrieve from history. The result is a Read that succeeds at the tool layer but delivers no usable content to the model, which is the failure mode reported in QwenLM#3805 ("read tool returns no content in long-running sessions"). This mirrors the existing post-compaction clear in tryCompressChat — microcompaction has the same "history rewrite invalidates the cache's 'model has seen this' assumption" property, it was just missed when the cache was wired in. * fix(core): clear FileReadCache on every history rewrite path PR1 only patched microcompaction, but a multi-round audit found four more entry points that rewrite history without clearing the cache, producing the same `file_unchanged` placeholder vs. missing-content mismatch. Each is fixed in the same minimal way (clear() at the call site) and covered by a regression test: - GeminiClient.setHistory — /restore checkpoint, /load_history - GeminiClient.truncateHistory — rewind in AppContainer - GeminiClient.resetChat — public API; clearCommand happens to clear the cache via startNewSession beforehand, but other callers have no such guarantee - stripOrphanedUserEntriesFromHistory — Retry path drops trailing user entries that may include read_file functionResponses Also tightened the microcompaction comment ("compactable tool outputs" instead of an enumerated list, since the source of truth is microcompact.COMPACTABLE_TOOLS) and removed caller references per the codebase comment style. Reverse-tested every new clear() by commenting it out and confirming the matching regression test fails. * test(core): integration test for FileReadCache + history rewrite End-to-end tests using the real ReadFileTool, real FileReadCache, real microcompactHistory, and a real on-disk file. Three cases: 1. Without a cache clear after microcompact, the second Read of an unchanged file returns the file_unchanged placeholder while the prior content has already been wiped from history. Demonstrates the failure mode this PR fixes. 2. After an explicit cache.clear(), the second Read re-emits the real bytes. Demonstrates that the fix works. 3. When microcompact removes every prior read of a file, the placeholder leaves zero recoverable bytes — the model literally cannot find the content anywhere it can reach. These complement the existing unit tests in client.test.ts (which verify the call-site wiring) by proving the end-to-end behaviour through the real code paths, without mocks. * chore(core): add traceable debug log for every FileReadCache clear Per review feedback: the new clear() call sites were silent, leaving no breadcrumb in production debug streams when the cache is dropped. Adds a `[FILE_READ_CACHE] clear after <reason>` log at every clear site (5 new + 1 pre-existing in tryCompressChat) so operators can grep one prefix and see why the cache was invalidated. * chore(core): refine truncateHistory cache clear + extract test helper Per review feedback (deepseek-v4-pro): 1. truncateHistory now skips the cache clear when keepCount >= prevLen, since a no-op truncate leaves the cache valid against the unchanged history. Adds a regression test covering both keepCount==prevLen and keepCount>prevLen. 2. The 6 cache-spy test cases each repeated the same 4-line mock setup. Extract a `mockFileReadCacheClear()` helper so future changes to the FileReadCache mock surface only need one edit. Both are quality-of-implementation tweaks; the underlying fix is unchanged. * perf(core): use O(1) getHistoryLength in truncateHistory Per Copilot review feedback: the previous commit's no-op detection in truncateHistory called this.getChat().getHistory().length, but GeminiChat.getHistory() does a structuredClone of the entire history on every call (line 770 of geminiChat.ts) — paying an O(history) clone purely to read .length. In long-running sessions with hundreds of entries this is a meaningful regression. Adds GeminiChat.getHistoryLength(): O(1), no clone. truncateHistory switches to it. The behaviour (skip clear when keepCount >= prevLen) is unchanged. Also adds: - Unit tests for GeminiChat.getHistoryLength (empty, after addHistory, parity with getHistory().length). - A regression test asserting truncateHistory calls getHistoryLength and NOT getHistory, locking in the perf fix against future drift. * fix(core): close NaN hole + use public ReadFileTool API in tests Two issues from copilot review: 1. NaN edge case in truncateHistory cache invalidation. The "did anything actually change?" check was `keepCount < prevLen`, but `Array.slice(0, NaN)` returns [] (history wiped) while `NaN < prevLen` is false. That sequence would wipe the chat but leave the FileReadCache claiming the model has seen the prior reads — exactly the file_unchanged placeholder bug this PR is closing. Switched the check to compare actual post-truncate length (`newLen < prevLen`), which correctly invalidates whenever entries were removed regardless of how `keepCount` was malformed. Added a NaN regression test. 2. The integration test cast `tool` to `unknown` to reach the protected `createInvocation()` method. Switched to the public `tool.buildAndExecute(params, signal)` API so the test exercises the same surface real callers use, including build-time schema validation.

wenshao added 2 commits May 3, 2026 22:15

wenshao changed the title ~~fix(core): clear FileReadCache after microcompaction~~ fix(core): clear FileReadCache on every history rewrite path May 3, 2026

wenshao commented May 3, 2026

View reviewed changes

Comment thread packages/core/src/core/client.ts

wenshao commented May 3, 2026

View reviewed changes

Comment thread packages/core/src/core/client.ts

Comment thread packages/core/src/core/client.ts Outdated

Comment thread packages/core/src/core/client.test.ts Outdated

wenshao requested a review from Copilot May 3, 2026 22:36

Copilot started reviewing on behalf of wenshao May 3, 2026 22:36 View session

Copilot AI reviewed May 3, 2026

View reviewed changes

Comment thread packages/core/src/core/client.ts Outdated

wenshao commented May 3, 2026

View reviewed changes

This was referenced May 4, 2026

📊 AI CLI 工具社区动态日报 2026-05-04 gsscsd/big_model_radar#290

Open

📊 AI CLI 工具社区动态日报 2026-05-04 borq168/radar-forge#128

Open

wenshao requested a review from Copilot May 4, 2026 08:22

Copilot started reviewing on behalf of wenshao May 4, 2026 08:22 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

Comment thread packages/core/src/core/client.ts Outdated

Comment thread packages/core/src/services/fileReadCache.integration.test.ts Outdated

yiliang114 approved these changes May 4, 2026

View reviewed changes

wenshao merged commit fcefab6 into QwenLM:main May 4, 2026
13 checks passed

TaimoorSiddiquiOfficial pushed a commit to TaimoorSiddiquiOfficial/HopCode that referenced this pull request May 4, 2026

fix(core): clear FileReadCache on all history mutation paths (QwenLM#…

2f81941

…3810)

wenshao mentioned this pull request May 6, 2026

Daemon mode (qwen serve): proposal & open decisions #3803

Open

46 tasks

tanzhenxin mentioned this pull request May 9, 2026

feat(core): refuse Edit/WriteFile when the file changed since last read #3840

Closed

4 tasks

Navigation Menu

Uh oh!

fix(core): clear FileReadCache on every history rewrite path - #3810

fix(core): clear FileReadCache on every history rewrite path#3810
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:fix/microcompact-clear-file-read-cache

wenshao commented May 3, 2026 •

edited

Loading

Uh oh!

wenshao left a comment

Uh oh!

Uh oh!

wenshao left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

wenshao left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

yiliang114 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wenshao commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Proof the bug is real

Proof the fix works

Test output

Reverse-tested

Verification

Notes

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

yiliang114 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wenshao commented May 3, 2026 •

edited

Loading