Repository navigation
fix: prevent stale session detail and 60s update lag - #183
Conversation
DataCache now stores an optional file-state fingerprint (mtimeMs+size) alongside each entry. handleGetSessionDetail stats the JSONL file before the cache lookup and passes the fingerprint to get()/set(); a mismatch invalidates the entry. This is a safety net for FileWatcher events dropped by macOS FSEvents — without it, a missed event left a stale cache entry that survived manual refresh for up to the 10-minute TTL. Also caps the renderer's adaptive session-refresh debounce at 5s (previously up to 60s for sessions with >1000 AI groups), so the UI never appears frozen while a session is actively streaming.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a file-state fingerprinting mechanism to the session data cache, using file modification time and size to detect stale entries when file system events are missed. It also optimizes the renderer's adaptive debounce logic by capping the refresh interval at 5 seconds to maintain UI responsiveness during active streaming. A review comment suggests explicitly invalidating the cache entry if the file status check fails to ensure that deleted or inaccessible sessions do not persist in the cache until the TTL expires.
| } catch { | ||
| // Stat failure is non-fatal — fall through to the existence check below. | ||
| } |
There was a problem hiding this comment.
If stat fails (e.g., the file was deleted or is inaccessible), fingerprint remains undefined. In this case, the subsequent dataCache.get(cacheKey, fingerprint) call will skip the fingerprint check and return a cached entry if one exists. This means stale data for a deleted session could persist in the UI until the 10-minute TTL expires, even after a manual refresh.
Invalidating the cache entry in the catch block ensures that a failed stat correctly leads to a cache miss and a fresh existence check, which is consistent with the goal of preventing stale data from surviving manual refreshes.
| } catch { | |
| // Stat failure is non-fatal — fall through to the existence check below. | |
| } | |
| } catch { | |
| // Stat failure is non-fatal — invalidate cache to ensure we don't serve stale data if the file was deleted. | |
| dataCache.invalidate(cacheKey); | |
| } |
There was a problem hiding this comment.
Pull request overview
Prevents stale session-detail caching when file watcher events are missed, and reduces perceived UI “freeze” by capping adaptive refresh debounce for large/streaming sessions.
Changes:
- Add optional file-state fingerprinting (mtimeMs+size) to
DataCacheentries and invalidate on mismatch. - Update
handleGetSessionDetailtostat()the session JSONL file, pass a fingerprint to cacheget()/set(), and re-parse when stale. - Cap renderer adaptive session-refresh debounce to a 5s maximum for large sessions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/renderer/store/index.ts | Caps adaptive refresh debounce to avoid long (up to 60s) UI update lag on large sessions. |
| src/main/services/infrastructure/DataCache.ts | Stores optional fingerprints on cache entries and invalidates entries when the provided fingerprint differs. |
| src/main/ipc/sessions.ts | Computes a session file fingerprint via stat() and uses it to avoid returning stale cached session detail. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check fingerprint mismatch (file changed since cached) | ||
| if (fingerprint !== undefined && entry.fingerprint !== fingerprint) { | ||
| this.cache.delete(key); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The fingerprint invalidation behavior doesn’t match the new doc comment: the comment says a mismatch invalidates the entry “when present on both set() and get()”, but the code invalidates whenever a caller provides a fingerprint and the cached entry’s fingerprint is different (including undefined). Either tighten the check to only compare when entry.fingerprint is defined, or update the comment to reflect that entries without a fingerprint are treated as stale when a fingerprinted get() occurs.
| try { | ||
| const filePath = projectScanner.getSessionPath(safeProjectId, safeSessionId); | ||
| const stats = await projectScanner.getFileSystemProvider().stat(filePath); | ||
| fingerprint = `${stats.mtimeMs}-${stats.size}`; | ||
| } catch { | ||
| // Stat failure is non-fatal — fall through to the existence check below. | ||
| } |
There was a problem hiding this comment.
The comment in the stat() catch block says it will “fall through to the existence check below”, but on a cache hit the function returns immediately and never performs the later getSessionWithOptions null check. This can still serve a stale cached SessionDetail (or one for a deleted/missing file) for the full TTL when stat() fails. Consider bypassing the cache when fingerprinting fails (or explicitly verifying existence even on cache hits), and/or update the comment to reflect the actual control flow.
📝 WalkthroughWalkthroughImplemented fingerprint-based cache validation for session data using file modification time and size. Updated Changes
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/sessions.ts (1)
340-354:⚠️ Potential issue | 🟡 Minor
handleGetSessionMetricsbypasses the new fingerprint check.This handler reads the same
cacheKeyashandleGetSessionDetailbut callsdataCache.get(cacheKey)without a fingerprint, so an entry left stale by a dropped FileWatcher event will still be served here for up to the 10‑minute TTL — exactly the failure mode the PR is closing for session detail. If session-metrics callers are user-visible, please mirror the stat+fingerprint pattern; otherwise consider routing metrics throughhandleGetSessionDetail(or via a shared helper) so both stay consistent.♻️ Suggested fix
- const { sessionParser, dataCache } = registry.getActive(); + const { projectScanner, sessionParser, dataCache } = registry.getActive(); const safeProjectId = validatedProject.value!; const safeSessionId = validatedSession.value!; - // Try to get from cache first const cacheKey = DataCache.buildKey(safeProjectId, safeSessionId); - const cached = dataCache.get(cacheKey); + + let fingerprint: string | undefined; + try { + const filePath = projectScanner.getSessionPath(safeProjectId, safeSessionId); + const stats = await projectScanner.getFileSystemProvider().stat(filePath); + fingerprint = `${stats.mtimeMs}-${stats.size}`; + } catch { + dataCache.invalidate(cacheKey); + } + + const cached = dataCache.get(cacheKey, fingerprint); if (cached) { return cached.metrics; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/sessions.ts` around lines 340 - 354, handleGetSessionMetrics currently calls dataCache.get(cacheKey) directly and can return stale entries; change it to mirror the stat+fingerprint pattern used in handleGetSessionDetail: call dataCache.stat(cacheKey) to obtain the current fingerprint (and check existence), then call dataCache.get(cacheKey, fingerprint) (or re-parse via sessionParser.parseSession and update the cache when stat indicates miss) so returned metrics respect the same fingerprint validation as handleGetSessionDetail; reference functions/objects: handleGetSessionMetrics, handleGetSessionDetail, DataCache.buildKey, dataCache.stat, dataCache.get, and sessionParser.parseSession.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/sessions.ts`:
- Around line 220-233: The catch block after
projectScanner.getFileSystemProvider().stat can leave fingerprint undefined
which causes dataCache.get(cacheKey, fingerprint) to skip validation and return
stale data; update the catch to explicitly invalidate the cache entry (e.g.,
call the DataCache invalidation method for cacheKey or otherwise remove the
entry) before proceeding so a stale sessionDetail cannot be returned, and keep
the comment in sync; locate the logic around projectScanner.getSessionPath /
projectScanner.getFileSystemProvider().stat, the fingerprint variable, and the
subsequent dataCache.get call to implement the invalidation.
---
Outside diff comments:
In `@src/main/ipc/sessions.ts`:
- Around line 340-354: handleGetSessionMetrics currently calls
dataCache.get(cacheKey) directly and can return stale entries; change it to
mirror the stat+fingerprint pattern used in handleGetSessionDetail: call
dataCache.stat(cacheKey) to obtain the current fingerprint (and check
existence), then call dataCache.get(cacheKey, fingerprint) (or re-parse via
sessionParser.parseSession and update the cache when stat indicates miss) so
returned metrics respect the same fingerprint validation as
handleGetSessionDetail; reference functions/objects: handleGetSessionMetrics,
handleGetSessionDetail, DataCache.buildKey, dataCache.stat, dataCache.get, and
sessionParser.parseSession.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65eabd7a-aae6-48a9-aaf6-31fe20418038
📒 Files selected for processing (3)
src/main/ipc/sessions.tssrc/main/services/infrastructure/DataCache.tssrc/renderer/store/index.ts
| // Stat the JSONL file so we can fingerprint the cache entry. | ||
| // Without this, a missed FileWatcher event leaves a stale cache entry | ||
| // that survives manual refresh for up to 10 minutes (the TTL). | ||
| let fingerprint: string | undefined; | ||
| try { | ||
| const filePath = projectScanner.getSessionPath(safeProjectId, safeSessionId); | ||
| const stats = await projectScanner.getFileSystemProvider().stat(filePath); | ||
| fingerprint = `${stats.mtimeMs}-${stats.size}`; | ||
| } catch { | ||
| // Stat failure is non-fatal — fall through to the existence check below. | ||
| } | ||
|
|
||
| // Check cache first (returns undefined if fingerprint mismatches) | ||
| let sessionDetail = dataCache.get(cacheKey, fingerprint); |
There was a problem hiding this comment.
Misleading comment — there is no “existence check below.”
The catch comment claims the code falls through to an existence check, but the very next statement is dataCache.get(cacheKey, fingerprint) with fingerprint === undefined. Per DataCache.get, an undefined fingerprint skips validation, so on stat failure a stale cached entry will still be returned. That's the original bug this PR is meant to mitigate, just narrowed to the rare stat-failure window.
Either tighten the comment to reflect actual behavior, or invalidate the cache entry explicitly when stat fails so the safety net still applies.
♻️ Suggested clarification
let fingerprint: string | undefined;
try {
const filePath = projectScanner.getSessionPath(safeProjectId, safeSessionId);
const stats = await projectScanner.getFileSystemProvider().stat(filePath);
fingerprint = `${stats.mtimeMs}-${stats.size}`;
} catch {
- // Stat failure is non-fatal — fall through to the existence check below.
+ // Stat failure is non-fatal. Without a fingerprint we can't validate the
+ // cached entry, so invalidate it to avoid serving a stale read; the
+ // downstream parse path will repopulate the cache.
+ dataCache.invalidate(cacheKey);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/sessions.ts` around lines 220 - 233, The catch block after
projectScanner.getFileSystemProvider().stat can leave fingerprint undefined
which causes dataCache.get(cacheKey, fingerprint) to skip validation and return
stale data; update the catch to explicitly invalidate the cache entry (e.g.,
call the DataCache invalidation method for cacheKey or otherwise remove the
entry) before proceeding so a stale sessionDetail cannot be returned, and keep
the comment in sync; locate the logic around projectScanner.getSessionPath /
projectScanner.getFileSystemProvider().stat, the fingerprint variable, and the
subsequent dataCache.get call to implement the invalidation.
Fixes matt1398#186. Long sessions ran at ~10fps after v0.4.13 because PR matt1398#183 shrank the refresh debounce ceiling from 60s to 5s. The shorter window exposed an existing architectural flaw: every `getSessionDetail` IPC returned the full payload, with the renderer-side fingerprint check happening *after* the expensive serialization. Frequent refreshes paid near-real-refresh cost even when nothing on disk had changed. Move the fingerprint check to the IPC boundary. The renderer caches the opaque file-state fingerprint (mtimeMs+size) returned by main and passes it back on the next call. When it matches, the handler short-circuits to a tiny `{ unchanged: true }` sentinel before cache lookup or parsing. The renderer detects the sentinel and exits without transforming or re-rendering — bounded to one stat() + a sub-100B IPC payload. With no-op cost effectively eliminated, the adaptive debounce can be restored to sane values (1s ceiling for >500 groups, 500ms for >200, 300ms for >100), preserving the streaming-responsiveness intent of PR matt1398#183 without the perf cost. Backwards compatible: knownFingerprint is optional, so initial loads and export still receive a full SessionDetail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DataCache now stores an optional file-state fingerprint (mtimeMs+size) alongside each entry. handleGetSessionDetail stats the JSONL file before the cache lookup and passes the fingerprint to get()/set(); a mismatch invalidates the entry. This is a safety net for FileWatcher events dropped by macOS FSEvents — without it, a missed event left a stale cache entry that survived manual refresh for up to the 10-minute TTL.
Also caps the renderer's adaptive session-refresh debounce at 5s (previously up to 60s for sessions with >1000 AI groups), so the UI never appears frozen while a session is actively streaming.
Summary by CodeRabbit
Bug Fixes
Performance