perf(deeplake-fs): serve whole-session reads from the local event cache#329
Merged
Merged
Conversation
The VFS whole-session read (readFile/readFileBuffer) issued an unbounded SELECT message FROM sessions WHERE path = ... ORDER BY creation_date ASC, re-materializing the entire fat message column on every fresh process. On mega-sessions (thousands of rows, tens of MB) that is a multi-second cold read, and it is the dominant driver of fat-payload p95 for orgs whose sessions are re-read repeatedly (observed 6+ s per read). The capture hook already appends every event to a local per-session cache (session-event-cache.ts, row-for-row identical to the message column); the wiki-workers already prefer it. Wire the same cache-first strategy into the VFS read: recover the sessionId from the path and read the local cache first, falling back to the full ordered DB read on any miss (session captured elsewhere, cache disabled/pruned, unreadable). Both readFile and readFileBuffer now share one loadSessionMessages helper. Content is provably identical: both cache lines and DB rows flow through joinSessionMessages -> normalizeContent, which JSON.parses each row and rebuilds output from parsed fields, so jsonb re-serialization vs the cached string cannot diverge. The DB fallback stays unbounded because the VFS needs the whole session (cat/grep), unlike the wiki-worker's bounded tail read.
📝 WalkthroughWalkthroughSession file reads now use a cache-first loader with ordered database fallback, shared reconstruction, and ChangesSession read caching
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 1 file changed
Generated for commit be6f7f2. |
![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
🧹 Nitpick comments (1)
tests/claude-code/deeplake-fs.test.ts (1)
879-887: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the expected SQL exactly.
includes("SELECT message FROM")accepts a query with a missing path predicate, ordering, or wrong table, allowing fallback tests to pass despite a broken DB-read contract. Match the expected bootstrap and session-read SQL strings and reject unexpected queries.As per path instructions,
tests/**should “Prefer asserting on specific values (paths, messages) over generic substrings.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-code/deeplake-fs.test.ts` around lines 879 - 887, Update the mock query handler in the affected test to compare the complete expected bootstrap and session-read SQL strings, including the correct table, path predicate, and ordering, instead of using the generic `includes("SELECT message FROM")` check. Return the configured rows only for those exact queries and reject unexpected SQL; assert specific paths and messages where the test validates results.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shell/deeplake-fs.ts`:
- Around line 96-108: Update sessionIdFromVfsPath to accept only canonical
/sessions/<user>/<user>_<org>_<workspace>_<sessionId>.jsonl paths with a valid
UUID session ID, returning null for all other paths before cache lookup. Add a
test covering a noncanonical path that ends with an existing session ID and
verify it falls back to database rows rather than local cached content.
---
Nitpick comments:
In `@tests/claude-code/deeplake-fs.test.ts`:
- Around line 879-887: Update the mock query handler in the affected test to
compare the complete expected bootstrap and session-read SQL strings, including
the correct table, path predicate, and ordering, instead of using the generic
`includes("SELECT message FROM")` check. Return the configured rows only for
those exact queries and reject unexpected SQL; assert specific paths and
messages where the test validates results.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35bd2f8c-b445-46bd-b8bd-c91130a897dd
📒 Files selected for processing (2)
src/shell/deeplake-fs.tstests/claude-code/deeplake-fs.test.ts
Comment on lines
+96
to
+108
| // Recover the bare capture sessionId from a VFS session path so a whole-session | ||
| // read can hit the local event cache. Path shape (see buildSessionPath): | ||
| // /sessions/<user>/<user>_<org>_<ws>_<sessionId>.jsonl | ||
| // The sessionId is a UUID (no underscores), so it is the last `_`-delimited | ||
| // segment of the filename stem — recoverable even when user/org/ws contain | ||
| // underscores. A non-matching path yields a value that simply misses the cache | ||
| // (→ DB fallback), so this is best-effort and never load-bearing. | ||
| function sessionIdFromVfsPath(p: string): string | null { | ||
| const file = p.split("/").pop() ?? ""; | ||
| if (!file.endsWith(".jsonl")) return null; | ||
| const sid = file.slice(0, -".jsonl".length).split("_").pop() ?? ""; | ||
| return sid.length > 0 ? sid : null; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject non-session paths before consulting the cache.
The helper claims non-matching paths fall back, but any .jsonl filename ending in _<existing-session-id> resolves to that ID. A noncanonical session-table row could therefore render another session’s local cached content instead of its DB rows. Validate the /sessions/<user>/..._<UUID>.jsonl shape and UUID before returning an ID; add a DB-fallback test for a noncanonical path.
Proposed fix
function sessionIdFromVfsPath(p: string): string | null {
- const file = p.split("/").pop() ?? "";
- if (!file.endsWith(".jsonl")) return null;
- const sid = file.slice(0, -".jsonl".length).split("_").pop() ?? "";
- return sid.length > 0 ? sid : null;
+ const match = /^\/sessions\/[^/]+\/.+_([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\.jsonl$/i.exec(p);
+ return match?.[1] ?? null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Recover the bare capture sessionId from a VFS session path so a whole-session | |
| // read can hit the local event cache. Path shape (see buildSessionPath): | |
| // /sessions/<user>/<user>_<org>_<ws>_<sessionId>.jsonl | |
| // The sessionId is a UUID (no underscores), so it is the last `_`-delimited | |
| // segment of the filename stem — recoverable even when user/org/ws contain | |
| // underscores. A non-matching path yields a value that simply misses the cache | |
| // (→ DB fallback), so this is best-effort and never load-bearing. | |
| function sessionIdFromVfsPath(p: string): string | null { | |
| const file = p.split("/").pop() ?? ""; | |
| if (!file.endsWith(".jsonl")) return null; | |
| const sid = file.slice(0, -".jsonl".length).split("_").pop() ?? ""; | |
| return sid.length > 0 ? sid : null; | |
| } | |
| // Recover the bare capture sessionId from a VFS session path so a whole-session | |
| // read can hit the local event cache. Path shape (see buildSessionPath): | |
| // /sessions/<user>/<user>_<org>_<ws>_<sessionId>.jsonl | |
| // The sessionId is a UUID (no underscores), so it is the last `_`-delimited | |
| // segment of the filename stem — recoverable even when user/org/ws contain | |
| // underscores. A non-matching path yields a value that simply misses the cache | |
| // (→ DB fallback), so this is best-effort and never load-bearing. | |
| function sessionIdFromVfsPath(p: string): string | null { | |
| const match = /^\/sessions\/[^/]+\/.+_([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\.jsonl$/i.exec(p); | |
| return match?.[1] ?? null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shell/deeplake-fs.ts` around lines 96 - 108, Update sessionIdFromVfsPath
to accept only canonical
/sessions/<user>/<user>_<org>_<workspace>_<sessionId>.jsonl paths with a valid
UUID session ID, returning null for all other paths before cache lookup. Add a
test covering a noncanonical path that ends with an existing session ID and
verify it falls back to database rows rather than local cached content.
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
1 participant
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.
Problem
The VFS whole-session read (
readFile/readFileBufferinsrc/shell/deeplake-fs.ts) issued an unboundedSELECT message FROM sessions WHERE path = … ORDER BY creation_date ASC, re-materializing the entire fatmessagecolumn on every fresh process. On mega-sessions (thousands of rows, tens of MB) that's a multi-second cold read.Observed in prod: an org whose sessions are re-read repeatedly showed 6+ s per whole-session read (one 2,204-msg / 8.6 MB session), dominating its p95. This is the path
grep-interceptor/catand skillify hit, and it was not covered by the earlier wiki-worker DB-fallback bounding — that fix bounded a different path.Fix
The capture hook already appends every event to a local per-session cache (
session-event-cache.ts,~/.claude/hooks/session-cache/<sessionId>.jsonl, row-for-row identical to themessagecolumn). The wiki-workers already prefer it. This wires the same cache-first strategy into the VFS read:sessionIdfrom the VFS path (last_-segment of the filename stem — a UUID, so unambiguous even when user/org/ws contain underscores).readFileandreadFileBuffernow share oneloadSessionMessageshelper.The DB fallback stays unbounded on purpose — the VFS needs the whole session (cat/grep), unlike the wiki-worker's bounded tail read.
Why it's safe
joinSessionMessages→normalizeContent, whichJSON.parses each row and rebuilds output from parsed fields — so jsonb re-serialization (reordered keys/whitespace) vs the cached string cannot diverge.HIVEMIND_SESSION_EVENT_CACHEopt-out and 14-day TTL prune.Tests
6 new cases in
tests/claude-code/deeplake-fs.test.ts: cache-hit avoids the fat read, cache/DB content equivalence, null-miss and empty-cache fall back to DB,readFileBufferalso prefers cache, and sessionId recovery with underscores. Full suite: 90/90 in the deeplake-fs suite,tsc --noEmitclean. (The twograph-on-stop-bundlefailures are pre-existing onorigin/main— a missing built-bundle artifact, unrelated.)Summary by CodeRabbit
Performance
Bug Fixes