From 0b3af202df0e43bb33416ac994bddfbf4753d91d Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Sun, 17 May 2026 21:00:57 +0800 Subject: [PATCH 1/2] fix(core): preserve read-before-write state across idle microcompaction (#4239) Idle microcompaction blanked old tool outputs and then wiped the entire file-read tracking cache to keep the read_file "unchanged" fast path from pointing at content no longer in history. That wipe also destroyed the read-before-write record, so after any idle break the model was falsely told a file it had already read "has not been read in this session" and forced into a redundant re-read of an unchanged file before it could edit it. Replace the blanket wipe with per-file targeting: when microcompaction blanks a read/edit/write result, only that file's fast-path eligibility is disarmed; the on-disk fingerprint and "seen this session" markers are preserved, so edits/overwrites of an unchanged file proceed without a re-read while a file that actually changed on disk is still correctly caught. If a blanked result's file path cannot be resolved back to its cache entry, fall back to the old blanket wipe so a stale "unchanged" placeholder can never be served. --- packages/core/src/core/client.test.ts | 222 ++++++++++++++++-- packages/core/src/core/client.ts | 56 ++++- .../core/src/services/fileReadCache.test.ts | 125 ++++++++++ packages/core/src/services/fileReadCache.ts | 58 +++++ .../microcompaction/microcompact.test.ts | 144 ++++++++++++ .../services/microcompaction/microcompact.ts | 68 ++++++ packages/core/src/tools/read-file.test.ts | 44 ++++ packages/core/src/tools/read-file.ts | 3 + 8 files changed, 695 insertions(+), 25 deletions(-) diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 57b268ce7ca..977978db9b0 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -14,6 +14,9 @@ import { type Mock, } from 'vitest'; +import { mkdtemp, writeFile, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import type { Content, GenerateContentResponse, Part } from '@google/genai'; import { GeminiClient, SendMessageType } from './client.js'; import { findCompressSplitPoint } from '../services/chatCompressionService.js'; @@ -1438,10 +1441,33 @@ describe('Gemini Client (client.ts)', () => { const clearMock = vi.fn(); vi.mocked(mockConfig.getFileReadCache).mockReturnValue({ clear: clearMock, + // Returns true = "entry found and disarmed" (the common case). + markReadEvictedFromHistory: vi.fn().mockReturnValue(true), } as unknown as ReturnType); return clearMock; } + /** + * Like {@link mockFileReadCacheClear} but also exposes the + * `markReadEvictedFromHistory` spy — the surgical per-file fast-path + * disarm that microcompaction now uses instead of a blanket wipe + * (issue #4239). + */ + function mockFileReadCacheStub(): { + clear: ReturnType; + markReadEvictedFromHistory: ReturnType; + } { + const clear = vi.fn(); + // Default: every disarm matches an entry (true). Tests that need + // the inode-miss fallback override the return value per-call. + const markReadEvictedFromHistory = vi.fn().mockReturnValue(true); + vi.mocked(mockConfig.getFileReadCache).mockReturnValue({ + clear, + markReadEvictedFromHistory, + } as unknown as ReturnType); + return { clear, markReadEvictedFromHistory }; + } + describe('thinking block idle cleanup and latch', () => { let mockChat: Partial; @@ -1495,16 +1521,30 @@ describe('Gemini Client (client.ts)', () => { }); describe('microcompaction FileReadCache invalidation', () => { - function makeReadFileResponses(count: number): Content[] { + let mcTmpDir: string; + + // Real on-disk files so client.ts's `fsPromises.stat(filePath)` (used + // to resolve a blanked path to its inode) succeeds. `node:fs` is + // mocked in this suite but `node:fs/promises` is not. + async function makeReadFileResponses(count: number): Promise<{ + history: Content[]; + paths: string[]; + }> { const out: Content[] = []; + const paths: string[] = []; for (let i = 0; i < count; i++) { + const p = join(mcTmpDir, `${i}.ts`); + await writeFile(p, `content of ${i}`); + paths.push(p); + const callId = `mc-call-${i}`; out.push({ role: 'model', parts: [ { functionCall: { + id: callId, name: 'read_file', - args: { file_path: `/x/${i}.ts` }, + args: { file_path: p }, }, }, ], @@ -1514,6 +1554,7 @@ describe('Gemini Client (client.ts)', () => { parts: [ { functionResponse: { + id: callId, name: 'read_file', response: { output: `content of ${i}` }, }, @@ -1521,10 +1562,11 @@ describe('Gemini Client (client.ts)', () => { ], }); } - return out; + return { history: out, paths }; } - beforeEach(() => { + beforeEach(async () => { + mcTmpDir = await mkdtemp(join(tmpdir(), 'qwen-mc-cache-')); mockTurnRunFn.mockReturnValue( (async function* () { yield { type: GeminiEventType.Content, value: 'response' }; @@ -1532,14 +1574,19 @@ describe('Gemini Client (client.ts)', () => { ); }); - it('clears the cache after microcompaction strips old read_file results', async () => { + afterEach(async () => { + await rm(mcTmpDir, { recursive: true, force: true }); + }); + + it('disarms the fast-path for blanked files instead of wiping the cache (issue #4239)', async () => { // Default test fixture: toolResultsThresholdMinutes = 60, // toolResultsNumToKeep = 5. Six read_file results + a 90-minute - // idle gap means the oldest one gets cleared, so the if-meta - // branch in sendMessageStream fires and must invalidate the cache. - const cacheClear = mockFileReadCacheClear(); + // idle gap means the oldest one gets blanked. The read-before-write + // state must survive (no clear()); only the one blanked file's + // fast-path is disarmed via markReadEvictedFromHistory. + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); - const history = makeReadFileResponses(6); + const { history } = await makeReadFileResponses(6); const setHistory = vi.fn(); client['chat'] = { addHistory: vi.fn(), @@ -1559,13 +1606,159 @@ describe('Gemini Client (client.ts)', () => { } expect(setHistory).toHaveBeenCalled(); - expect(cacheClear).toHaveBeenCalled(); + // The blanket wipe is gone — read-before-write state is preserved. + expect(clear).not.toHaveBeenCalled(); + // Exactly the one blanked file (oldest of 6, keepRecent=5) had its + // fast-path disarmed. + expect(markReadEvictedFromHistory).toHaveBeenCalledTimes(1); }); - it('does not clear the cache when the idle gap is below the threshold', async () => { - const cacheClear = mockFileReadCacheClear(); + it('falls back to a blanket clear when blanked reads cannot be linked to a path (id-less provider)', async () => { + // Provider did not populate functionCall.id, so microcompaction + // cannot recover the blanked reads' file paths. Leaving their + // fast-path armed would serve a dangling placeholder, so the + // client must fall back to the old safe blanket wipe. + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); + + const idless: Content[] = []; + for (let i = 0; i < 6; i++) { + idless.push({ + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: join(mcTmpDir, `${i}.ts`) }, + }, + }, + ], + }); + idless.push({ + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: `content of ${i}` }, + }, + }, + ], + }); + } + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(idless), + setHistory: vi.fn(), + } as unknown as GeminiChat; + client['lastApiCompletionTimestamp'] = Date.now() - 90 * 60_000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-3', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + expect(clear).toHaveBeenCalled(); + expect(markReadEvictedFromHistory).not.toHaveBeenCalled(); + }); + + it('falls back to a blanket clear when an evicted path cannot be stat’d (Codex P2)', async () => { + // Path is recovered (id linkage present) so it lands in + // evictedReadPaths, but the file does not exist on disk, so the + // client's stat fails. Leaving the entry armed would risk a + // dangling placeholder, so it must fall back to the safe wipe. + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); + + const history: Content[] = []; + for (let i = 0; i < 6; i++) { + const callId = `mc-missing-${i}`; + // Path inside mcTmpDir that is never created. + const p = join(mcTmpDir, `ghost-${i}.ts`); + history.push({ + role: 'model', + parts: [ + { + functionCall: { + id: callId, + name: 'read_file', + args: { file_path: p }, + }, + }, + ], + }); + history.push({ + role: 'user', + parts: [ + { + functionResponse: { + id: callId, + name: 'read_file', + response: { output: `content of ${i}` }, + }, + }, + ], + }); + } + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(history), + setHistory: vi.fn(), + } as unknown as GeminiChat; + client['lastApiCompletionTimestamp'] = Date.now() - 90 * 60_000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-4', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + expect(clear).toHaveBeenCalled(); + expect(markReadEvictedFromHistory).not.toHaveBeenCalled(); + }); + + it('falls back to a blanket clear when an evicted path stats to a different inode (Codex P2)', async () => { + // Path stats fine, but resolves to an inode the cache never + // recorded (file replaced / symlink retargeted since the read), + // so markReadEvictedFromHistory finds no entry and returns false. + // A stale entry could stay armed, so fall back to the safe wipe. + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); + markReadEvictedFromHistory.mockReturnValue(false); - const history = makeReadFileResponses(6); + const { history } = await makeReadFileResponses(6); + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(history), + setHistory: vi.fn(), + } as unknown as GeminiChat; + client['lastApiCompletionTimestamp'] = Date.now() - 90 * 60_000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-5', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + // It attempted the surgical disarm, found no entry, then wiped. + expect(markReadEvictedFromHistory).toHaveBeenCalled(); + expect(clear).toHaveBeenCalled(); + }); + + it('does not touch the cache when the idle gap is below the threshold', async () => { + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); + + const { history } = await makeReadFileResponses(6); client['chat'] = { addHistory: vi.fn(), getHistory: vi.fn().mockReturnValue(history), @@ -1584,7 +1777,8 @@ describe('Gemini Client (client.ts)', () => { /* drain */ } - expect(cacheClear).not.toHaveBeenCalled(); + expect(clear).not.toHaveBeenCalled(); + expect(markReadEvictedFromHistory).not.toHaveBeenCalled(); }); }); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 93610264d2b..b50e5aa411d 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -104,6 +104,7 @@ import { } from '../confirmation-bus/types.js'; import { partToString } from '../utils/partUtils.js'; import { createHookOutput, SessionStartSource } from '../hooks/types.js'; +import fsPromises from 'node:fs/promises'; // IDE integration import { ideContextStore } from '../ide/ideContext.js'; @@ -1206,18 +1207,51 @@ export class GeminiClient { this.config.getClearContextOnIdle(), ); if (mcResult.meta) { - this.getChat().setHistory(mcResult.history); - // Microcompaction replaces old compactable tool outputs - // (including read_file) with a placeholder, but the - // FileReadCache still records the prior full Reads as "seen in - // this conversation". A follow-up Read of an unchanged file - // would then return the file_unchanged placeholder pointing at - // bytes the model can no longer retrieve from history. Drop the - // cache so post-microcompaction Reads re-emit the bytes, - // mirroring the post-compaction clear in tryCompressChat. - debugLogger.debug('[FILE_READ_CACHE] clear after microcompaction'); - this.config.getFileReadCache().clear(); const m = mcResult.meta; + this.getChat().setHistory(mcResult.history); + // Disarm only the blanked files' fast-path, keeping + // read-before-write state intact (issue #4239; rationale on + // FileReadEntry.readResidentInHistory). Any blanked read we + // can't disarm surgically forces the old blanket wipe so a + // later Read can't get a dangling file_unchanged placeholder. + const fileReadCache = this.config.getFileReadCache(); + if (m.unresolvedEvictedReads > 0) { + debugLogger.debug( + `[FILE_READ_CACHE] clear after microcompaction ` + + `(${m.unresolvedEvictedReads} unresolved blanked read(s))`, + ); + fileReadCache.clear(); + } else { + // Concurrent stats — don't serialize N FS round-trips + // before the next turn. + const statResults = await Promise.all( + m.evictedReadPaths.map((p) => + fsPromises.stat(p).catch(() => undefined), + ), + ); + // A path is surgically disarmed only if it stats AND its + // inode matches the recorded entry. A failed stat or inode + // miss could leave a stale entry armed, so fall back to the + // blanket wipe if any path is unresolvable. + let fullyDisarmed = true; + for (const stats of statResults) { + if (!stats || !fileReadCache.markReadEvictedFromHistory(stats)) { + fullyDisarmed = false; + } + } + if (fullyDisarmed) { + debugLogger.debug( + `[FILE_READ_CACHE] disarmed fast-path for ` + + `${m.evictedReadPaths.length} file(s) after microcompaction`, + ); + } else { + debugLogger.debug( + '[FILE_READ_CACHE] clear after microcompaction ' + + '(an evicted path was unresolvable)', + ); + fileReadCache.clear(); + } + } debugLogger.debug( `[TIME-BASED MC] gap ${m.gapMinutes}min > ${m.thresholdMinutes}min, ` + `cleared ${m.toolsCleared} tool result(s) + ${m.mediaCleared} media (~${m.tokensSaved} tokens), ` + diff --git a/packages/core/src/services/fileReadCache.test.ts b/packages/core/src/services/fileReadCache.test.ts index 491e4d8923a..e6018cf0eb0 100644 --- a/packages/core/src/services/fileReadCache.test.ts +++ b/packages/core/src/services/fileReadCache.test.ts @@ -505,4 +505,129 @@ describe('FileReadCache', () => { expect(cache.check(fs.statSync(file)).state).toBe('stale'); }); }); + + describe('readResidentInHistory / markReadEvictedFromHistory (issue #4239)', () => { + it('a fresh recordRead is resident in history', () => { + const cache = new FileReadCache(); + const stats = makeStats(); + const entry = cache.recordRead('/x/foo.ts', stats, { + full: true, + cacheable: true, + }); + expect(entry.readResidentInHistory).toBe(true); + }); + + it('a fresh recordWrite is resident in history', () => { + const cache = new FileReadCache(); + const entry = cache.recordWrite('/x/foo.ts', makeStats()); + expect(entry.readResidentInHistory).toBe(true); + }); + + it('markReadEvictedFromHistory disarms only the fast-path, preserving read-before-write state', () => { + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + + expect(cache.markReadEvictedFromHistory(stats)).toBe(true); + + const result = cache.check(stats); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + // Fast-path disarmed... + expect(result.entry.readResidentInHistory).toBe(false); + // ...but everything read-before-write depends on is intact. + expect(result.entry.lastReadAt).toBeDefined(); + expect(result.entry.lastReadWasFull).toBe(true); + expect(result.entry.lastReadCacheable).toBe(true); + } + }); + + it('returns false (caller must fall back to clear) when there is no entry for the stats', () => { + const cache = new FileReadCache(); + // No entry, or stats resolved to a different inode than recorded + // — the caller treats this like an unstattable path. + expect(cache.markReadEvictedFromHistory(makeStats())).toBe(false); + expect(cache.check(makeStats()).state).toBe('unknown'); + + // Entry exists under inode A; a stat for inode B must not match. + cache.recordRead('/x/foo.ts', makeStats({ ino: 1 }), { + full: true, + cacheable: true, + }); + expect(cache.markReadEvictedFromHistory(makeStats({ ino: 2 }))).toBe( + false, + ); + }); + + it('a subsequent real read re-arms the fast-path (resident again)', () => { + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + cache.markReadEvictedFromHistory(stats); + + // The model voluntarily re-read the file: its bytes are back in + // history, so the fast-path is honest again. + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + + const result = cache.check(stats); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + expect(result.entry.readResidentInHistory).toBe(true); + } + }); + + it('a subsequent write re-arms the fast-path (resident again)', () => { + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + cache.markReadEvictedFromHistory(stats); + + cache.recordWrite('/x/foo.ts', makeStats({ mtimeMs: 2000 })); + + const result = cache.check(makeStats({ mtimeMs: 2000 })); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + expect(result.entry.readResidentInHistory).toBe(true); + } + }); + + it('a PARTIAL read does NOT re-arm an evicted full read', () => { + // Regression for the Codex P2: after microcompaction blanks a + // full read, a later partial read of the unchanged file used to + // unconditionally re-arm readResidentInHistory while + // lastReadWasFull stayed sticky-true — so a follow-up full Read + // would get a file_unchanged placeholder pointing at bytes no + // longer in history (only a slice is resident). + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + cache.markReadEvictedFromHistory(stats); + + // Same unchanged bytes, but only a slice read this time. + cache.recordRead('/x/foo.ts', stats, { full: false, cacheable: true }); + + const result = cache.check(stats); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + // Still disarmed — the full bytes are NOT back in history. + expect(result.entry.readResidentInHistory).toBe(false); + // lastReadWasFull stays sticky-true (read-rights preserved). + expect(result.entry.lastReadWasFull).toBe(true); + } + }); + + it('a partial read leaves a still-resident full read armed', () => { + const cache = new FileReadCache(); + const stats = makeStats(); + cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true }); + // No eviction — the full read is still in history. + cache.recordRead('/x/foo.ts', stats, { full: false, cacheable: true }); + + const result = cache.check(stats); + expect(result.state).toBe('fresh'); + if (result.state === 'fresh') { + expect(result.entry.readResidentInHistory).toBe(true); + } + }); + }); }); diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index 4bac8fb19c5..d5e6dbeebc2 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -105,6 +105,25 @@ export interface FileReadEntry { * model to use a different mechanism rather than re-read). */ lastReadCacheable: boolean; + /** + * True iff the read/write that the fast-path would point at is still + * quotable from conversation history — i.e. it has NOT been blanked + * by idle microcompaction. + * + * Sole consumer is the ReadFile fast-path: the `file_unchanged` + * placeholder ("you already have this earlier in the conversation") + * is only honest while that content is still in history. Set `true` + * only by a full {@link recordRead} / {@link recordWrite} (a partial + * read does not make the whole file resident); flipped to `false` by + * {@link markReadEvictedFromHistory} when microcompaction blanks it. + * + * `priorReadEnforcement.ts` does NOT consult this flag and must not + * start: read-before-write only needs that the model saw the file + * and the on-disk fingerprint is current, neither of which history + * blanking invalidates. Wiping read-rights on idle cleanup was the + * issue #4239 false-block this whole marker exists to avoid. + */ + readResidentInHistory: boolean; } /** Result of {@link FileReadCache.check}. */ @@ -172,6 +191,14 @@ export class FileReadCache { existing.sizeBytes === stats.size; const entry = this.upsert(absPath, stats); entry.lastReadAt = Date.now(); + if (opts.full) { + // Only a full read re-arms: a partial read leaves a mere slice + // in history while sticky `lastReadWasFull` stays true, so + // re-arming on it would resurrect a dangling placeholder for an + // evicted full read. Leaving it untouched for partial reads is + // correct either way (a still-resident full read stays true). + entry.readResidentInHistory = true; + } if (sameFingerprint) { // Same bytes the entry already described — sticky-on-true // preserves prior `true` flags from full reads or writes. @@ -214,6 +241,9 @@ export class FileReadCache { entry.lastReadAt = now; entry.lastReadWasFull = true; entry.lastReadCacheable = true; + // The model authored the current bytes and that result is in + // history, so the fast-path may serve a placeholder again. + entry.readResidentInHistory = true; return entry; } @@ -242,6 +272,33 @@ export class FileReadCache { return { state: 'fresh', entry }; } + /** + * Mark the entry for `stats` as no longer quotable from conversation + * history — its read/edit/write output was blanked by idle + * microcompaction. + * + * Surgical alternative to {@link clear} for microcompaction: only + * {@link FileReadEntry.readResidentInHistory} is disarmed; the + * fingerprint / `lastReadAt` / `lastReadCacheable` that + * read-before-write depends on are preserved (that is the issue + * #4239 fix). + * + * Returns `true` if a matching entry was found and disarmed; `false` + * if there is no entry for `stats` (never tracked, or `stats` + * resolved to a different inode than recorded — file replaced / + * symlink retargeted since the read). A `false` is NOT harmless: the + * stale entry stays armed, so the caller must fall back to + * {@link clear} just as it does for an unstattable path. + */ + markReadEvictedFromHistory(stats: Stats): boolean { + const entry = this.byInode.get(FileReadCache.inodeKey(stats)); + if (entry) { + entry.readResidentInHistory = false; + return true; + } + return false; + } + /** Remove the entry for the given Stats, if any. */ invalidate(stats: Stats): void { this.byInode.delete(FileReadCache.inodeKey(stats)); @@ -273,6 +330,7 @@ export class FileReadCache { sizeBytes: stats.size, lastReadWasFull: false, lastReadCacheable: false, + readResidentInHistory: false, }; this.byInode.set(key, entry); return entry; diff --git a/packages/core/src/services/microcompaction/microcompact.test.ts b/packages/core/src/services/microcompaction/microcompact.test.ts index 13b1f1770a6..428f51ade48 100644 --- a/packages/core/src/services/microcompaction/microcompact.test.ts +++ b/packages/core/src/services/microcompaction/microcompact.test.ts @@ -662,3 +662,147 @@ describe('microcompactHistory', () => { expect(cleared.parts).toBeUndefined(); }); }); + +describe('microcompactHistory evictedReadPaths (issue #4239)', () => { + const TWO_HOURS_AGO = Date.now() - 2 * 60 * 60 * 1000; + + function fileCall(id: string, name: string, filePath: string): Content { + return { + role: 'model', + parts: [{ functionCall: { id, name, args: { file_path: filePath } } }], + }; + } + + function fileResult(id: string, name: string, output: string): Content { + return { + role: 'user', + parts: [{ functionResponse: { id, name, response: { output } } }], + }; + } + + it('reports the file path of a blanked read_file result', () => { + const history: Content[] = [ + fileCall('c0', 'read_file', '/proj/old.ts'), + fileResult('c0', 'read_file', 'old long content '.repeat(50)), + fileCall('c1', 'read_file', '/proj/recent.ts'), + fileResult('c1', 'read_file', 'recent content'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta).toBeDefined(); + expect(result.meta!.toolsCleared).toBe(1); + // Only the blanked (oldest) file is reported; the kept one is not. + expect(result.meta!.evictedReadPaths).toEqual(['/proj/old.ts']); + expect(result.meta!.unresolvedEvictedReads).toBe(0); + }); + + it('reports edit and write_file paths too, deduplicated', () => { + const history: Content[] = [ + fileCall('c0', 'edit', '/proj/a.ts'), + fileResult('c0', 'edit', 'edit output '.repeat(50)), + fileCall('c1', 'write_file', '/proj/a.ts'), + fileResult('c1', 'write_file', 'write output '.repeat(50)), + fileCall('c2', 'read_file', '/proj/keep.ts'), + fileResult('c2', 'read_file', 'kept'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta!.toolsCleared).toBe(2); + // /proj/a.ts blanked via both edit and write_file → reported once. + expect(result.meta!.evictedReadPaths).toEqual(['/proj/a.ts']); + expect(result.meta!.unresolvedEvictedReads).toBe(0); + }); + + it('counts a blanked read it cannot link back as unresolved (forces safe fallback)', () => { + const history: Content[] = [ + // functionResponse without an id: cannot be linked to a call. + // This is the id-less-provider case — must NOT be silently + // skipped, or its fast-path stays armed and serves a dangling + // placeholder. It is counted so the caller falls back to the + // blanket wipe. + { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: 'orphan content '.repeat(50) }, + }, + }, + ], + }, + fileCall('c1', 'read_file', '/proj/recent.ts'), + fileResult('c1', 'read_file', 'recent'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta!.toolsCleared).toBe(1); + expect(result.meta!.evictedReadPaths).toEqual([]); + expect(result.meta!.unresolvedEvictedReads).toBe(1); + }); + + it('counts a blanked file call whose id has no mapped file_path as unresolved', () => { + const history: Content[] = [ + // functionResponse has an id, but no functionCall carries that + // id with a file_path (synthetic-id / mismatch case). + fileResult('orphan-id', 'read_file', 'orphan content '.repeat(50)), + fileCall('c1', 'read_file', '/proj/recent.ts'), + fileResult('c1', 'read_file', 'recent'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta!.toolsCleared).toBe(1); + expect(result.meta!.evictedReadPaths).toEqual([]); + expect(result.meta!.unresolvedEvictedReads).toBe(1); + }); + + it('does not report non-file tools (shell/grep) as evicted reads', () => { + const history: Content[] = [ + fileCall('c0', 'run_shell_command', 'unused'), + fileResult('c0', 'run_shell_command', 'shell output '.repeat(50)), + fileCall('c1', 'read_file', '/proj/recent.ts'), + fileResult('c1', 'read_file', 'recent'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta!.toolsCleared).toBe(1); + expect(result.meta!.evictedReadPaths).toEqual([]); + // Shell is not a file tool — not counted as an unresolved read. + expect(result.meta!.unresolvedEvictedReads).toBe(0); + }); + + it('returns no evictedReadPaths when nothing fires (no idle trigger)', () => { + const history: Content[] = [ + fileCall('c0', 'read_file', '/proj/a.ts'), + fileResult('c0', 'read_file', 'content'), + ]; + + const result = microcompactHistory(history, Date.now(), { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + // No trigger → no meta at all (and therefore no eviction data). + expect(result.meta).toBeUndefined(); + }); +}); diff --git a/packages/core/src/services/microcompaction/microcompact.ts b/packages/core/src/services/microcompaction/microcompact.ts index c67cfc8cc7b..6a0ac1f87ee 100644 --- a/packages/core/src/services/microcompaction/microcompact.ts +++ b/packages/core/src/services/microcompaction/microcompact.ts @@ -23,6 +23,46 @@ const COMPACTABLE_TOOLS = new Set([ ToolNames.WRITE_FILE, ]); +/** + * Tools whose blanked output drops a file's bytes from history. We + * report their path so the caller can disarm just that file's + * fast-path (issue #4239) instead of wiping the whole cache. All three + * take the target as a `file_path` arg. + */ +const FILE_PATH_TOOLS = new Set([ + ToolNames.READ_FILE, + ToolNames.EDIT, + ToolNames.WRITE_FILE, +]); + +/** + * Build a `callId → file_path` map for every file-tool call. The path + * lives on the request-side `functionCall.args`, not on the + * `functionResponse` microcompaction blanks, so this is the only way + * to recover which file a cleared result referred to. Calls missing an + * id or file_path are absent (the caller treats that as unresolvable). + */ +function buildCallIdToFilePath(history: Content[]): Map { + const map = new Map(); + for (const content of history) { + // functionCall parts are always model-role; skip user/system turns + // (mirrors collectCompactablePartRefs' role short-circuit). + if (content.role !== 'model' || !content.parts) continue; + for (const part of content.parts) { + const call = part.functionCall; + if (!call?.id || !call.name || !FILE_PATH_TOOLS.has(call.name)) { + continue; + } + const filePath = (call.args as { file_path?: unknown } | undefined) + ?.file_path; + if (typeof filePath === 'string' && filePath.length > 0) { + map.set(call.id, filePath); + } + } + } + return map; +} + // --- Trigger evaluation --- /** @@ -200,6 +240,15 @@ export interface MicrocompactMeta { mediaKept: number; keepRecent: number; tokensSaved: number; + /** Recovered paths of files whose read/edit/write result was blanked; the caller disarms their fast-path (issue #4239). */ + evictedReadPaths: string[]; + /** + * Count of blanked file results whose path could NOT be recovered + * (e.g. provider didn't populate `functionCall.id`). Non-zero means + * the caller MUST fall back to the blanket wipe — an unrecovered + * armed entry would serve a dangling placeholder. + */ + unresolvedEvictedReads: number; } /** @@ -261,6 +310,10 @@ export function microcompactHistory( parts.set(ref.partIndex, ref.kind); } + const callIdToFilePath = buildCallIdToFilePath(history); + const evictedReadPaths = new Set(); + let unresolvedEvictedReads = 0; + let tokensSaved = 0; let toolsCleared = 0; let mediaCleared = 0; @@ -284,6 +337,19 @@ export function microcompactHistory( tokensSaved += estimatePartTokens(part); toolsCleared++; touched = true; + // Record the blanked file's path so the caller disarms its + // fast-path; if unrecoverable, count it so the caller falls + // back to the blanket wipe (issue #4239). + if (FILE_PATH_TOOLS.has(part.functionResponse.name)) { + const filePath = part.functionResponse.id + ? callIdToFilePath.get(part.functionResponse.id) + : undefined; + if (filePath) { + evictedReadPaths.add(filePath); + } else { + unresolvedEvictedReads++; + } + } return { functionResponse: { ...stripNestedMedia(part.functionResponse), @@ -346,6 +412,8 @@ export function microcompactHistory( mediaKept, keepRecent, tokensSaved, + evictedReadPaths: [...evictedReadPaths], + unresolvedEvictedReads, }, }; } diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index fae35b5f541..ab59c30290d 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -645,6 +645,50 @@ describe('ReadFileTool', () => { expect(second.returnDisplay).toMatch(/^Unchanged: /); }); + it('re-emits bytes (no placeholder) after the read was evicted from history by microcompaction (issue #4239)', async () => { + const filePath = path.join(tempRootDir, 'evicted.txt'); + await fsp.writeFile(filePath, 'hello world', 'utf-8'); + + const first = await read({ file_path: filePath }); + expect(first.llmContent).toBe('hello world'); + + // Simulate idle microcompaction blanking this read's output: + // the bytes are no longer quotable from history. + fileReadCache.markReadEvictedFromHistory(fs.statSync(filePath)); + + // The fast-path must NOT serve a placeholder pointing at content + // the model can no longer retrieve — it must re-emit real bytes. + const second = await read({ file_path: filePath }); + expect(second.llmContent).toBe('hello world'); + expect(second.llmContent).not.toMatch(/unchanged since/); + + // And a re-read re-arms the fast-path (bytes are back in history). + const third = await read({ file_path: filePath }); + expect(third.llmContent).toMatch( + /unchanged since last read in this session/, + ); + }); + + it('a partial read after eviction does NOT re-arm the placeholder (Codex P2)', async () => { + const filePath = path.join(tempRootDir, 'evicted-partial.txt'); + await fsp.writeFile(filePath, 'line1\nline2\nline3\n', 'utf-8'); + + // Full read, then microcompaction blanks it. + await read({ file_path: filePath }); + fileReadCache.markReadEvictedFromHistory(fs.statSync(filePath)); + + // A partial (ranged) read of the unchanged file: only a slice + // is now resident, not the full bytes. + const partial = await read({ file_path: filePath, limit: 1 }); + expect(partial.llmContent).not.toMatch(/unchanged since/); + + // A follow-up full Read must STILL re-emit real bytes — the + // full content is not in history, only the slice is. + const full = await read({ file_path: filePath }); + expect(full.llmContent).toContain('line3'); + expect(full.llmContent).not.toMatch(/unchanged since/); + }); + it('serves a fresh full Read after an external modification (stale)', async () => { const filePath = path.join(tempRootDir, 'mut.txt'); await fsp.writeFile(filePath, 'one', 'utf-8'); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index c528ba634d8..4e8528c80b8 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -186,6 +186,9 @@ class ReadFileToolInvocation extends BaseToolInvocation< status.entry.lastReadAt !== undefined && status.entry.lastReadWasFull && status.entry.lastReadCacheable && + // Only quote-back if that read is still in history (issue + // #4239: idle microcompaction flips this off when it blanks it). + status.entry.readResidentInHistory && (status.entry.lastWriteAt === undefined || status.entry.lastReadAt > status.entry.lastWriteAt) ) { From f602477b134a986bb8c8fa06a608be5e72e948e2 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Mon, 18 May 2026 08:44:15 +0800 Subject: [PATCH 2/2] refactor(core): harden microcompaction fast-path eviction (PR #4243 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - buildCallIdToFilePath now accumulates paths per callId (Map) so a reused/duplicate functionCall.id disarms ALL candidate files instead of silently dropping one — over-disarming costs at most a redundant re-read, whereas keeping the wrong file armed would resurrect the issue #4239 dangling-placeholder hazard. - Add a guard-rail comment tying COMPACTABLE_TOOLS to FILE_PATH_TOOLS. - Document why readResidentInHistory is intentionally not reset on the fingerprint-drift branch. - Add tests: duplicate-id disarm, and a mixed on-disk/ghost evicted batch forcing the safe blanket wipe. --- packages/core/src/core/client.test.ts | 70 +++++++++++++++++++ packages/core/src/services/fileReadCache.ts | 4 ++ .../microcompaction/microcompact.test.ts | 27 +++++++ .../services/microcompaction/microcompact.ts | 25 +++++-- 4 files changed, 119 insertions(+), 7 deletions(-) diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 977978db9b0..7e968b1326e 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -1724,6 +1724,76 @@ describe('Gemini Client (client.ts)', () => { expect(markReadEvictedFromHistory).not.toHaveBeenCalled(); }); + it('falls back to a blanket clear on a MIXED batch — one path on disk, one a ghost (mimo F9)', async () => { + // Most realistic production case: several files evicted, most on + // disk, one deleted since. A single unresolvable path must still + // force the safe blanket wipe rather than a partial disarm. + const { clear, markReadEvictedFromHistory } = mockFileReadCacheStub(); + + // keepRecent = 5 in this suite, so 7 results blank the 2 oldest: + // index 0 (real, stats OK) and index 1 (ghost, stat fails). + const realPath = join(mcTmpDir, 'mixed-real.ts'); + await writeFile(realPath, 'real content'); + const ghostPath = join(mcTmpDir, 'mixed-ghost.ts'); // never created + + const history: Content[] = []; + for (let i = 0; i < 7; i++) { + const callId = `mc-mixed-${i}`; + const p = + i === 0 + ? realPath + : i === 1 + ? ghostPath + : join(mcTmpDir, `mixed-keep-${i}.ts`); + history.push({ + role: 'model', + parts: [ + { + functionCall: { + id: callId, + name: 'read_file', + args: { file_path: p }, + }, + }, + ], + }); + history.push({ + role: 'user', + parts: [ + { + functionResponse: { + id: callId, + name: 'read_file', + response: { output: `content of ${i}` }, + }, + }, + ], + }); + } + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(history), + setHistory: vi.fn(), + } as unknown as GeminiChat; + client['lastApiCompletionTimestamp'] = Date.now() - 90 * 60_000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-mixed', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + // The ghost path makes the batch not-fully-disarmed → safe wipe. + expect(clear).toHaveBeenCalled(); + // The on-disk path was still attempted before the batch was + // deemed unresolvable. + expect(markReadEvictedFromHistory).toHaveBeenCalled(); + }); + it('falls back to a blanket clear when an evicted path stats to a different inode (Codex P2)', async () => { // Path stats fine, but resolves to an inode the cache never // recorded (file replaced / symlink retargeted since the read), diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index 862adf0a042..cae6f7ff302 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -212,6 +212,10 @@ export class FileReadCache { } else { // Drift detected (or fresh entry): the prior flags described // different bytes. Reset to what this read actually produced. + // `readResidentInHistory` is intentionally NOT reset here — it + // tracks whether the read is still quotable from history, which + // is orthogonal to the on-disk fingerprint and already handled + // by the `opts.full` branch above. entry.lastReadWasFull = opts.full; entry.lastReadCacheable = opts.cacheable; } diff --git a/packages/core/src/services/microcompaction/microcompact.test.ts b/packages/core/src/services/microcompaction/microcompact.test.ts index 428f51ade48..72f62c1901d 100644 --- a/packages/core/src/services/microcompaction/microcompact.test.ts +++ b/packages/core/src/services/microcompaction/microcompact.test.ts @@ -700,6 +700,33 @@ describe('microcompactHistory evictedReadPaths (issue #4239)', () => { expect(result.meta!.unresolvedEvictedReads).toBe(0); }); + it('disarms ALL paths sharing a reused functionCall.id (mimo F1)', () => { + // Pathological/resumed history reuses one id across two files. + // The blanked result must disarm BOTH candidate paths — keeping + // the wrong one armed would resurrect the dangling-placeholder + // hazard. Over-disarming only costs a redundant re-read. + const history: Content[] = [ + fileCall('dup', 'read_file', '/proj/first.ts'), + fileResult('dup', 'read_file', 'first old content '.repeat(50)), + fileCall('dup', 'read_file', '/proj/second.ts'), + fileResult('dup', 'read_file', 'second old content '.repeat(50)), + fileCall('c2', 'read_file', '/proj/keep.ts'), + fileResult('c2', 'read_file', 'kept'), + ]; + + const result = microcompactHistory(history, TWO_HOURS_AGO, { + toolResultsThresholdMinutes: 5, + toolResultsNumToKeep: 1, + }); + + expect(result.meta!.toolsCleared).toBe(2); + expect([...result.meta!.evictedReadPaths].sort()).toEqual([ + '/proj/first.ts', + '/proj/second.ts', + ]); + expect(result.meta!.unresolvedEvictedReads).toBe(0); + }); + it('reports edit and write_file paths too, deduplicated', () => { const history: Content[] = [ fileCall('c0', 'edit', '/proj/a.ts'), diff --git a/packages/core/src/services/microcompaction/microcompact.ts b/packages/core/src/services/microcompaction/microcompact.ts index 6a0ac1f87ee..77f91d30dea 100644 --- a/packages/core/src/services/microcompaction/microcompact.ts +++ b/packages/core/src/services/microcompaction/microcompact.ts @@ -13,6 +13,9 @@ import { ToolNames } from '../../tools/tool-names.js'; export const MICROCOMPACT_CLEARED_MESSAGE = '[Old tool result content cleared]'; export const MICROCOMPACT_CLEARED_IMAGE_PREFIX = '[Old inline media cleared:'; +// IMPORTANT: any new file-touching tool added here MUST also be added +// to FILE_PATH_TOOLS below, or microcompaction will blank its output +// without reporting the eviction — silently reintroducing issue #4239. const COMPACTABLE_TOOLS = new Set([ ToolNames.READ_FILE, ToolNames.SHELL, @@ -36,14 +39,20 @@ const FILE_PATH_TOOLS = new Set([ ]); /** - * Build a `callId → file_path` map for every file-tool call. The path + * Build a `callId → file_path[]` map for every file-tool call. The path * lives on the request-side `functionCall.args`, not on the * `functionResponse` microcompaction blanks, so this is the only way * to recover which file a cleared result referred to. Calls missing an * id or file_path are absent (the caller treats that as unresolvable). + * + * Paths accumulate per id rather than overwrite: if a (malformed or + * resumed) history reuses a `functionCall.id` across different files, + * disarming *all* candidate paths is the safe choice — over-disarming + * costs at most a redundant re-read, whereas keeping the wrong file + * armed would resurrect the dangling-placeholder hazard (issue #4239). */ -function buildCallIdToFilePath(history: Content[]): Map { - const map = new Map(); +function buildCallIdToFilePath(history: Content[]): Map { + const map = new Map(); for (const content of history) { // functionCall parts are always model-role; skip user/system turns // (mirrors collectCompactablePartRefs' role short-circuit). @@ -56,7 +65,9 @@ function buildCallIdToFilePath(history: Content[]): Map { const filePath = (call.args as { file_path?: unknown } | undefined) ?.file_path; if (typeof filePath === 'string' && filePath.length > 0) { - map.set(call.id, filePath); + const existing = map.get(call.id); + if (existing) existing.push(filePath); + else map.set(call.id, [filePath]); } } } @@ -341,11 +352,11 @@ export function microcompactHistory( // fast-path; if unrecoverable, count it so the caller falls // back to the blanket wipe (issue #4239). if (FILE_PATH_TOOLS.has(part.functionResponse.name)) { - const filePath = part.functionResponse.id + const filePaths = part.functionResponse.id ? callIdToFilePath.get(part.functionResponse.id) : undefined; - if (filePath) { - evictedReadPaths.add(filePath); + if (filePaths && filePaths.length > 0) { + for (const p of filePaths) evictedReadPaths.add(p); } else { unresolvedEvictedReads++; }