Repository navigation
Stop AI Vault cold scans from re-reading the entire transcript corpus on every launch - #9474
Conversation
Live re-verification evidence (dev build from this branch, real local corpus)Method: launched the dev build with a fresh temp profile via CDP, triggered Run 1 — fresh profile (no cache file; equals pre-fix behavior on every launch): Cache file written by the debounced save after the scan: Run 2 — first scan after quitting and relaunching the app (the #9210 scenario): Restart cold scan went from 3.7 s / 1.79 GB / Baseline HEAD repro for comparison (same rig, before the fix): cold Full-window screenshots of both runs captured at |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 2a13b28734e2bb2bb8b20582fb8c7a131621b632 and efb686c. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds persistent JSON snapshots for the AI Vault session parse cache, including strict validation, restart loading, capped seeding, debounced persistence, atomic replacement, and temporary-file cleanup. Session scanning now loads cached entries before parsing and schedules saves afterward. Application startup initializes persistence using the canonical user data path and app version. Tests cover cache reuse, invalid snapshots, scan integration, capacity limits, and filesystem failure handling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/ai-vault/session-parse-cache-persistence.test.ts (1)
254-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally assert
readdiris not called to fully cover the "zero disk IO" claim.
sweepOrphanedTempFiles(invoked fromensureSessionParseCacheLoadedwhen initialized) reads the directory viareaddir. Since this test relies on the uninitialized early-return path, adding areaddirassertion guards against a regression where the sweep runs before theoptions === nullcheck.♻️ Optional: add readdir assertion
expect(fsPromises.readFile).not.toHaveBeenCalled() expect(fsPromises.writeFile).not.toHaveBeenCalled() expect(fsPromises.mkdir).not.toHaveBeenCalled() expect(fsPromises.rename).not.toHaveBeenCalled() expect(fsPromises.rm).not.toHaveBeenCalled() + expect(fsPromises.readdir).not.toHaveBeenCalled()src/main/ai-vault/session-scanner-parse-cache.ts (1)
122-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnsure correct LRU order if the cache is already populated.
If the cache is already partially populated, the current logic inadvertently inverts the LRU eviction order and drops the newest seeded entries:
- Because
Mapiteration order is insertion order, appending seeded entries after existing in-process entries makes the seeded items appear newer than the in-process items.- Stopping when
cache.size >= MAX_CACHE_ENTRIESdiscards the newest seeded entries instead of the oldest.While the current startup sequence likely guarantees an empty cache during seeding, rebuilding the map defensively ensures correct LRU behavior if this is ever called concurrently or if the initialization order changes.
♻️ Proposed refactor to rebuild the map
export function seedSessionParseCache( entries: Iterable<[string, PersistedSessionParseCacheEntry]> ): void { const list = [...entries] // Snapshot order is oldest→newest (LRU); an over-cap list keeps the newest // tail rather than seeding the oldest entries and dropping the tail. - for (const [path, entry] of list.slice(Math.max(0, list.length - MAX_CACHE_ENTRIES))) { - if (cache.size >= MAX_CACHE_ENTRIES) { - return - } - // In-process entries are always fresher than persisted ones; never clobber. - if (cache.has(path)) { - continue - } + + const existing = new Map(cache) + cache.clear() + + const newEntries = list.filter(([path]) => !existing.has(path)) + const sliceStart = Math.max(0, newEntries.length - (MAX_CACHE_ENTRIES - existing.size)) + + for (const [path, entry] of newEntries.slice(sliceStart)) { cache.set(path, { mtimeMs: entry.mtimeMs, sizeBytes: entry.sizeBytes, platform: entry.platform, session: entry.session, resume: null }) } + + // Re-insert existing entries so they remain the newest (freshest) + for (const [path, entry] of existing) { + cache.set(path, entry) + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7830e843-6915-4823-addd-2ac8e22a4283
📥 Commits
Reviewing files that changed from the base of the PR and between 624c8d4 and 2a13b28734e2bb2bb8b20582fb8c7a131621b632.
📒 Files selected for processing (5)
src/main/ai-vault/session-parse-cache-persistence.test.tssrc/main/ai-vault/session-parse-cache-persistence.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner.tssrc/main/index.ts
|
Responding to the two optional nitpicks:
|
Cold launches re-parsed the entire agent-transcript corpus (measured 6.7 GB / 109 s upstream; 1.79 GB / 3.7 s locally) because the parse cache was an in-memory Map. Persist the reusable portion (mtime+size gated session entries, resume states dropped) to one JSON file under the canonical userData dir: lazy load before the first scan, debounced atomic save after scans that parsed anything. Restart scans now reuse unchanged files (measured 328 ms / 3 MB, reused=1036).
2a13b28 to
efb686c
Compare
Cold launches re-parsed the entire agent-transcript corpus (measured 6.7 GB / 109 s upstream; 1.79 GB / 3.7 s locally) because the parse cache was an in-memory Map. Persist the reusable portion (mtime+size gated session entries, resume states dropped) to one JSON file under the canonical userData dir: lazy load before the first scan, debounced atomic save after scans that parsed anything. Restart scans now reuse unchanged files (measured 328 ms / 3 MB, reused=1036).
Fixes #9210
What changes
On a fresh launch, the AI Vault (Agent Session History) scan re-parsed the user's entire transcript corpus — the reporter measured 6.75 GB read over 109 s with
reused=0, and every launch paid it again. The parse cache already made in-process rescans nearly free, but it was an in-memory Map that died with the process.This PR persists the cache's reusable portion (
{mtimeMs, sizeBytes, platform, session}per transcript, ≤4096 entries, resume parser states dropped) to one JSON file under the canonical userData dir (ai-vault/session-parse-cache.json, dir0o700/file0o600). It loads lazily before the first scan and saves via a debounced (1.5 s) atomic temp+rename write after scans that parsed anything. Unchanged files (mtime+size+platform exact match — the same reuse gate as before, no new invalidation logic) become cache hits; files changed while the app was closed pay one full re-parse.Measured on this machine (real corpus, dev build, fresh profile):
What does NOT change
appVersionmismatch, by design) still pay one full parse. Offloading to a worker/utilityProcess and the issue's watchdog suggestion are deferred follow-ups.remote-session-scanner) don't use this cache and are untouched. Serve-mode/remote-runtime hosts gain persistence on their own main process (init runs in both desktop and--servemodes).Design & review process (Brennan's PR process)
schemaVersion/appVersionmismatch or any malformed entry.flushSessionParseCachePersistForTests) for deterministic debounce testing; snapshot/seed tests live in the new test file rather than extending the old one..tmpsweep on load, serialized save chain, debounce-collapse test).Risks / accepted trades
sessionobjects are envelope-validated (per-field types on the entry, whole-file discard on any anomaly) but the session interior is trusted — same trust domain as the transcripts themselves (user-only file under userData); reviewers traced every consumer and found no crash path. Cache-file strings bypass the normalization real transcripts get.appVersiondiscard is inert across dev parser changes (matches the existing in-process staleness trade).Validation
reused=1, bytesRead=0with the transcript deleted; corrupt/schema/appVersion discard; seed-no-clobber; changed-while-closed full parse; zero disk IO uninitialized; failing mkdir and failing rename both swallowed with temp cleanup; debounce collapse; orphan sweep; over-cap newest-tail; scanner-hook integration test proven to fail if either hook is dropped).pnpm typecheck:nodeand oxlint clean.Follow-up review: live restart validation (2026-07-19)
Revalidated after rebasing onto current
main(including the OpenCode SQLite worker change) with a fresh isolated dev profile and the real local transcript corpus. The panel populated with 500 sessions on both launches; the persisted cache was 1,327,760 bytes with mode0600.The seven restart parses were transcripts that changed while the app was closed; unchanged entries came from disk as intended.
First launch — populated Agent Session History after creating the cache
Restart — the same panel restored after disk-backed cache reuse