fix(core): preserve read-before-write state across idle microcompaction#4243
Merged
Conversation
…on (#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.
Collaborator
Author
E2E test reportScenario: read a file, let the session go idle past the cleanup threshold, then ask the assistant to edit that unchanged file without re-reading it.
Verification:
|
Contributor
📋 Review SummaryThis PR fixes issue #4239 where idle microcompaction was incorrectly clearing the entire 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. — glm-5.1 via Qwen Code /review
wenshao
previously approved these changes
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
PR #4243 — Review by mimo-v2.5-pro
Verdict: ✅ Looks Good To Me With Suggestions
264 tests pass, 0 deterministic findings (tsc + eslint clean). The readResidentInHistory flag correctly scopes the fast-path guard, the surgical disarm logic has proper fallback paths, and the test coverage is strong.
Finding 1: [Suggestion] — Duplicate functionCall.id causes disarm of wrong file
microcompact.ts:65 — buildCallIdToFilePath uses map.set(call.id, filePath) which silently overwrites if history contains duplicate functionCall.id values (malformed/resumed session). When a functionResponse with that id is blanked, the lookup returns the later file's path; the earlier file (whose result was actually blanked) stays armed, and the fast-path serves a placeholder the model cannot retrieve.
Suggested fix: Use Map<string, string[]> to accumulate all paths per callId:
function buildCallIdToFilePath(history: Content[]): Map<string, string[]> {
const map = new Map<string, string[]>();
for (const content of history) {
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) {
const existing = map.get(call.id);
if (existing) existing.push(filePath);
else map.set(call.id, [filePath]);
}
}
}
return map;
}Then iterate all mapped paths for each evicted callId and add each to evictedReadPaths.
Finding 2: [Suggestion] — FILE_PATH_TOOLS / COMPACTABLE_TOOLS split-brain risk
microcompact.ts:32 — FILE_PATH_TOOLS is a hand-maintained subset of COMPACTABLE_TOOLS. If a new file-touching tool is added to COMPACTABLE_TOOLS but not FILE_PATH_TOOLS, microcompaction will blank its output without reporting the eviction — the exact bug this PR fixes, silently reintroduced.
Suggested fix: Add a runtime assertion at module scope:
for (const tool of FILE_PATH_TOOLS) {
if (!COMPACTABLE_TOOLS.has(tool)) {
throw new Error(
`FILE_PATH_TOOLS contains "${tool}" not in COMPACTABLE_TOOLS — must stay in sync (issue #4239)`,
);
}
}And add a warning comment on COMPACTABLE_TOOLS:
// IMPORTANT: any new file-touching tool added here MUST also be added
// to FILE_PATH_TOOLS below so its eviction is tracked for the
// FileReadCache fast-path (issue #4239).Finding 3: [Suggestion] — Silent blanket wipe with no per-file diagnostics
client.ts:1248 — When the blanket wipe fires, the log says only "an evicted path was unresolvable" — no indication of which path failed, why (stat error vs inode mismatch), or how many were correctly disarmed. At 3 AM debugging a cache incident, this is the only breadcrumb trail.
Suggested fix: Log per-path results in the disarm loop:
for (let i = 0; i < statResults.length; i++) {
const stats = statResults[i];
const path = m.evictedReadPaths[i];
if (!stats) {
debugLogger.debug(`[FILE_READ_CACHE] stat failed for evicted path: ${path}`);
fullyDisarmed = false;
} else if (!fileReadCache.markReadEvictedFromHistory(stats)) {
debugLogger.debug(
`[FILE_READ_CACHE] inode mismatch for evicted path: ${path} (dev=${stats.dev}, ino=${stats.ino})`,
);
fullyDisarmed = false;
}
}Finding 4: [Suggestion] — All-or-nothing cache fallback defeats surgical disarm goal
client.ts:1240-1255 — If microcompaction evicts 10 files and 1 was deleted from disk, the blanket clear() wipes read-before-write state for ALL tracked files — including the 9 never evicted. The more files evicted in a batch, the higher the probability one is unresolvable, making the surgical path fragile. This is the exact regression issue #4239 was filed to prevent, reappearing as a fallback.
Suggested fix: Consider per-entry invalidation instead of blanket wipe. For unresolvable paths, call fileReadCache.invalidate() on the inode-mismatched entries rather than wiping the entire cache. At minimum, log which path failed (see Finding 3) so the blast radius is diagnosable.
Finding 5: [Suggestion] — No logging in markReadEvictedFromHistory
fileReadCache.ts:293 — The method performs a silent boolean check. If a specific file intermittently fails disarm (NFS inode race, symlink retarget), there's no per-file breadcrumb trail.
Finding 6: [Suggestion] — recordWrite unconditionally sets readResidentInHistory = true with no safety net
fileReadCache.ts:246 — If recordWrite succeeds but the write result is later blanked by microcompaction, the code relies on evictedReadPaths to disarm. If the file was deleted/recreated (new inode), markReadEvictedFromHistory returns false and the blanket wipe fires. Safe today due to the blanket wipe fallback, but the gap would surface if the fallback is ever removed.
Finding 7: [Suggestion] — buildCallIdToFilePath eagerly builds full Map
microcompact.ts:313 — Iterates ALL history entries to build the map, but only a handful will ever be looked up (the evicted ones). In long sessions this is unnecessary churn on every idle-microcompaction trigger.
Suggested fix: Collect functionResponse.id values of parts scheduled for clearing into a Set<string> neededCallIds, then replace the full-history scan with a targeted pass. This makes the map O(evicted) instead of O(all file-tool calls).
Finding 8: [Suggestion] — markReadEvictedFromHistory is a naming outlier
fileReadCache.ts:293 — Every other public method on FileReadCache is short and concise (recordRead, check, invalidate, clear, size). At 28 characters, this method reads more like a sentence than an API name. Consider renaming to markEvicted or markContentEvicted.
Finding 9: [Suggestion] — Missing test for mixed success/failure in stat loop
client.test.ts — Existing tests cover all-success, all-fail (ghost files), and all-fail (inode misses). No test has a mix where one path succeeds and another fails — the most realistic production scenario (e.g., 3 files evicted, 2 on disk, 1 deleted). Add a test where evictedReadPaths has 2 entries, one on disk and one ghost, asserting clear() is called.
Finding 10: [Nice to have] — buildCallIdToFilePath silently skips non-string file_path args
microcompact.ts:58 — A FILE_PATH_TOOLS call with an id but a non-string file_path arg silently increments unresolvedEvictedReads, triggering a blanket wipe. Consider adding a debug log when this happens.
Finding 11: [Nice to have] — fsPromises import placement breaks import grouping
client.ts:107 — The file groups imports by domain. A lone node: built-in in the middle of local module imports breaks convention. Move to the top alongside external deps.
Finding 12: [Nice to have] — Double traversal of history array
microcompact.ts:135,45 — collectCompactablePartRefs (user-role) and buildCallIdToFilePath (model-role) each do a full pass. Could be merged into one loop since they scan disjoint roles.
Finding 13: [Nice to have] — fsPromises.stat() on history-sourced paths bypasses permission gates
client.ts:1228 — evictedReadPaths come from functionCall.args.file_path in history, passed directly to stat() without re-validating against the tool's permission system. Low risk: paths were user-approved, and only metadata (not content) is disclosed. The clear() fallback on stat failure is the correct safety net.
Finding 14: [Nice to have] — readResidentInHistory not reset on fingerprint drift
fileReadCache.ts:197 — When fingerprint drifts, recordRead resets lastReadWasFull and lastReadCacheable but NOT readResidentInHistory. This is correct (fast-path content is orthogonal to on-disk fingerprint), but the invariant is only documented on the field definition, not in the drift branch itself. Consider adding an inline comment.
| Severity | Count |
|---|---|
| Critical | 0 |
| Suggestion | 9 |
| Nice to have | 5 |
Reviewed by mimo-v2.5-pro | Qwen Code v3.29.16
wenshao
reviewed
May 17, 2026
Collaborator
|
@tanzhenxin 麻烦 rebase 一下 main 解决一下冲突再合：
另外 14:24 那条 [Suggestion] 关于 microcompact.ts:340 同文件多次读时的 over-disarm（只是一次多余的 re-read，非 correctness），你顺手收也可以、留到 follow-up 也行——你判断。本地 264 tests + tsc clean，CI 全绿，rebase 完就 merge。 |
Collaborator
|
@copilot resolve the merge conflicts in this pull request |
Copilot stopped work on behalf of
wenshao due to an error
May 17, 2026 16:44
…ead-before-write-4239 # Conflicts: # packages/core/src/services/fileReadCache.test.ts
…eview) - buildCallIdToFilePath now accumulates paths per callId (Map<string, string[]>) 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.
Collaborator
Author
|
@wenshao thanks for the review — status update. Merge conflict resolved ✅Merged
|
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Second-opinion review (mimo-v2.5-pro)
Deterministic analysis: tsc=0, eslint=0, 18 test files / 806 tests all pass.
Overall assessment: The core surgical-disarm logic is well-designed. The three-tier fallback (unresolved IDs → blanket wipe, stat/inode check → surgical disarm, stat failure → blanket wipe) is correct and conservative. The test coverage is thorough — happy path, id-less providers, missing files, inode mismatches, and partial reads are all covered.
Finding 1 (Nice to have): Binary all-or-nothing cache fallback
File: packages/core/src/core/client.ts:1249-1252
When microcompaction blanks N file-tool results, the code attempts surgical disarm via concurrent fs.stat + markReadEvictedFromHistory. If any single stat fails or inode doesn't match, fileReadCache.clear() wipes the entire cache — including entries that were already successfully disarmed and entries for files that were never evicted.
The disarmed entries are safe (fast-path disabled), so a targeted clear of only unresolvable entries would preserve the PR's core benefit in the edge case where one evicted file is deleted/replaced during the idle gap.
// 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 is a refinement, not a correctness issue — the blanket wipe is safe, just overly aggressive. The current tests confirm the blanket-wipe fallback works correctly.
Positive observations:
- The
buildCallIdToFilePathfunction correctly handles missingfunctionCall.id(falls back to blanket wipe) - The
readResidentInHistoryflag's sticky-on-true fingerprint interaction is well-thought-out - Test coverage for the
fsPromises.statfallback path (Codex P2 regression) is thorough - The
execFilemigration insystemInfo.tseliminates shell injection risk
| } else { | ||
| debugLogger.debug( | ||
| '[FILE_READ_CACHE] clear after microcompaction ' + | ||
| '(an evicted path was unresolvable)', |
Collaborator
There was a problem hiding this comment.
💡 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.
wenshao
reviewed
May 18, 2026
| // 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(); |
Collaborator
There was a problem hiding this comment.
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;
})
wenshao
reviewed
May 18, 2026
| ), | ||
| ); | ||
| // A path is surgically disarmed only if it stats AND its | ||
| // inode matches the recorded entry. A failed stat or inode |
Collaborator
There was a problem hiding this comment.
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(", ")})`,
);
wenshao
reviewed
May 18, 2026
|
|
||
| // 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. |
Collaborator
There was a problem hiding this comment.
Nice to have: FILE_PATH_TOOLS is a strict subset of COMPACTABLE_TOOLS but the two sets are defined independently. A developer adding a new file-path tool to COMPACTABLE_TOOLS could easily miss updating FILE_PATH_TOOLS — the JSDoc here mentions the relationship but doesn't reference COMPACTABLE_TOOLS by name.
Consider adding a one-liner linking them:
// Must be a subset of COMPACTABLE_TOOLS above.
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary: PR #4243
Verdict: Looks good with minor suggestions
Build & Tests
- ✅
tscclean,eslintclean - ✅ 267/267 tests pass (fileReadCache + microcompact + client + read-file)
Correctness
The surgical disarm design is sound:
markReadEvictedFromHistorycorrectly disarms only blanked files' fast-path while preservingreadBeforeWritestatebuildCallIdToFilePathuses id linkage correctly; duplicate ids safely over-report paths- Fallback to blanket
clear()on any failure is conservative and correct readResidentInHistoryis only checked behindstatus.state === 'fresh', so stale fingerprint drift independently prevents dangling placeholders
No bugs found. Reverse audit confirmed the primary findings cover all real issues.
Findings
| # | Severity | File | Description |
|---|---|---|---|
| 1 | Suggestion | client.ts:1217 |
.catch(() => undefined) on fsPromises.stat() swallows all errors (EMFILE, EACCES, etc.), not just ENOENT. Consider logging unexpected errors. |
| 2 | Suggestion | client.ts:1233 |
Blanket wipe fallback log says "an evicted path was unresolvable" without naming which file(s) failed. The data is all in scope — include it for debuggability. |
| 3 | Nice to have | microcompact.ts:18 |
FILE_PATH_TOOLS is a strict subset of COMPACTABLE_TOOLS but defined independently. Add a cross-reference comment to prevent drift. |
No Critical findings.
wenshao
approved these changes
May 18, 2026
Collaborator
There was a problem hiding this comment.
本地实测 HEAD f602477b：269 tests passed（含 f602477 新加的 mimo F1 / F9 两条），tsc --build packages/core clean，CI 全绿。
f602477b 收得很到位：
buildCallIdToFilePath改Map<string, string[]>让 reused functionCall.id 把候选路径全 disarm，保守优于错放COMPACTABLE_TOOLS上加 guard-rail 注释，防未来加文件工具时漏掉FILE_PATH_TOOLS同步readResidentInHistory在 fingerprint-drift 分支为何不重置的设计意图也写进注释了
我 14:24 的 [Suggestion] 你 defer 的论证我认（fail-safe + 修复有引回 #4239 不对称风险 + mimo 给的 snippet 编译不过）。我 01:00 那条 Finding 1（client.ts:1249-1252 blanket clear）同源逻辑，一并 defer，开了 follow-up issue 跟进两项：#4259
LGTM。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
a.txt, then readb.txt; let the session go idle past the cleanup threshold; then "Append a line toa.txtusing the edit tool, do not read it again first."client.test.tsproves idle cleanup no longer performs the wholesale reset on the common path.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Closes #4239