Skip to content

fix(core): preserve read-before-write state across idle microcompaction #4243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 3 commits into from
May 18, 2026
Merged

fix(core): preserve read-before-write state across idle microcompaction #4243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b3af20
fix(core): preserve read-before-write state across idle microcompacti…
tanzhenxin May 17, 2026
ed0b668
Merge remote-tracking branch 'origin/main' into fix/microcompaction-r…
tanzhenxin May 18, 2026
f602477
refactor(core): harden microcompaction fast-path eviction (PR #4243 r…
tanzhenxin May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
222 changes: 208 additions & 14 deletions packages/core/src/core/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<Config['getFileReadCache']>);
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<typeof vi.fn>;
markReadEvictedFromHistory: ReturnType<typeof vi.fn>;
} {
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<Config['getFileReadCache']>);
return { clear, markReadEvictedFromHistory };
}

describe('thinking block idle cleanup and latch', () => {
let mockChat: Partial<GeminiChat>;

Expand Down Expand Up @@ -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 },
},
},
],
Expand All @@ -1514,32 +1554,39 @@ describe('Gemini Client (client.ts)', () => {
parts: [
{
functionResponse: {
id: callId,
name: 'read_file',
response: { output: `content of ${i}` },
},
},
],
});
}
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' };
})(),
);
});

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(),
Expand All @@ -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),
Expand All @@ -1584,7 +1777,8 @@ describe('Gemini Client (client.ts)', () => {
/* drain */
}

expect(cacheClear).not.toHaveBeenCalled();
expect(clear).not.toHaveBeenCalled();
expect(markReadEvictedFromHistory).not.toHaveBeenCalled();
});
});

Expand Down
56 changes: 45 additions & 11 deletions packages/core/src/core/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();

@wenshao wenshao May 18, 2026

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 .catch(() => undefined) swallows all errors indiscriminately. ENOENT is expected (file deleted since read), but EMFILE, EACCES, ENOMEM, etc. are not — they silently produce undefined, triggering the blanket wipe with zero diagnostic info.

Consider logging unexpected errors before falling through:

fsPromises.stat(p).catch((err: NodeJS.ErrnoException) => {
  if (err.code !== 'ENOENT') {
    debugLogger.debug(`[FILE_READ_CACHE] stat failed for ${p}: ${err.code ?? err.message}`);
  }
  return undefined;
})

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

@wenshao wenshao May 18, 2026

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: This log says "an evicted path was unresolvable" without naming which file(s) failed, how many were attempted, or the failure mode (stat error vs inode mismatch). When this fires in production it is very difficult to diagnose why the cache was wiped.

The data is all in scope — consider including it:

const failedPaths = m.evictedReadPaths.filter((_, i) => !statResults[i]);
debugLogger.debug(
  "[FILE_READ_CACHE] clear after microcompaction " +
    `(${failedPaths.length}/${m.evictedReadPaths.length} path(s) unresolvable: ${failedPaths.join(", ")})`,
);

// 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)',

@wenshao wenshao May 18, 2026

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.

💡 Nice to have: Consider a targeted clear instead of blanket clear(). The entries already disarmed in the loop above have their fast-path disabled and are safe. Only the unresolvable entries (stat failed or inode mismatch) need invalidation.

// After the loop, selectively clear only entries that weren't disarmed:
const unresolvedPaths = m.evictedReadPaths.filter(
  (_, i) => !statResults[i] || !fileReadCache.markReadEvictedFromHistory(statResults[i]!),
);
for (const p of unresolvedPaths) fileReadCache.invalidateByPath(p);

This preserves the PR's core benefit (read-before-write state survives) even in the edge case where one evicted file was deleted during the idle gap. The current blanket wipe is safe but destroys state for non-evicted files and already-disarmed entries.

);
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), ` +
Expand Down
Loading
Loading