fix(reads): skip lazy SELECTs for absent tables via listTables pre-check#209
Merged
Conversation
SessionStart reads hivemind_rules / hivemind_goals (context renderer) and
skills (auto-pull) on every start, relying on catching "relation does not
exist". The client handled it, but the SELECT still failed server-side, so
pg-deeplake logged a 42P01 per read on every fresh/empty workspace — the
single largest cluster in the prod error digest.
Gate these reads on a trusted table list:
- DeeplakeApi.knownTablesOrNull() returns the cached table list, or null
when the lookup is untrusted (so callers tell "empty workspace" from
"fetch failed").
- renderContextBlock and runPull take an optional tableExists predicate;
when it reports a table absent we skip the SELECT entirely.
- The existing isMissingTableError catch stays as a fallback: when the
list is unavailable (predicate omitted) behavior is unchanged, so a
transient lookup blip never drops a read of a table that exists.
listTables() is cached and already warmed by the write paths each session,
so this is typically zero extra HTTP calls.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDeeplakeApi adds a ChangesLazy table existence checks via cached known-tables predicate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 — 8 files changed
Generated for commit 1d2262e. |
![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
🤖 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/skillify/auto-pull.ts`:
- Around line 108-109: The call to api.knownTablesOrNull() happens outside the
withTimeout(...) block causing SessionStart to potentially exceed timeoutMs;
move the table discovery into the same timeout boundary by invoking
knownTablesOrNull via withTimeout(timeoutMs, () => api.knownTablesOrNull()) (or
call withTimeout separately and treat failures/timeouts as null), then set
tableExists = (name: string) => known.includes(name) only if known is returned;
apply the same change to the other discovery usage around lines 116-128 so both
discovery paths degrade to null on timeout and don’t block SessionStart.
🪄 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 Plus
Run ID: 030b2931-4f87-4734-ad76-aea78ac5eb05
📒 Files selected for processing (15)
src/commands/context.tssrc/deeplake-api.tssrc/hooks/cursor/session-start.tssrc/hooks/hermes/session-start.tssrc/hooks/session-start.tssrc/hooks/shared/context-renderer.tssrc/skillify/auto-pull.tssrc/skillify/pull.tstests/claude-code/cli-context.test.tstests/claude-code/session-start-hook.test.tstests/claude-code/skillify-auto-pull.test.tstests/claude-code/skillify-pull.test.tstests/cursor/cursor-session-start-hook.test.tstests/hermes/hermes-session-start-hook.test.tstests/shared/context-renderer.test.ts
Comment on lines
+108
to
+109
| const known = await api.knownTablesOrNull(); | ||
| if (known) tableExists = (name: string) => known.includes(name); |
There was a problem hiding this comment.
Bound table discovery inside the same timeout budget.
Line 108 calls await api.knownTablesOrNull() before withTimeout(...), so SessionStart can still block past timeoutMs when table discovery is slow. Keep table discovery inside the timed block (or timeout it separately and degrade to null).
⏱️ Proposed fix
- let query: QueryFn;
+ let query: QueryFn;
+ let api: DeeplakeApi | null = null;
// Existence predicate from a trusted table list — lets runPull skip the
// SELECT (and the server-side 42P01) when `skills` doesn't exist yet on a
// fresh workspace. Undefined when the list can't be fetched or a test
// injects its own query, in which case runPull falls back to its catch.
let tableExists: ((name: string) => boolean) | undefined;
if (deps.queryFn) {
query = deps.queryFn;
} else {
- const api = new DeeplakeApi(
+ api = new DeeplakeApi(
config.token,
config.apiUrl,
config.orgId,
config.workspaceId,
config.skillsTableName,
);
query = (sql: string) => api.query(sql) as Promise<Record<string, unknown>[]>;
- const known = await api.knownTablesOrNull();
- if (known) tableExists = (name: string) => known.includes(name);
}
@@
- try {
- const summary = await withTimeout(
- runPull({
+ try {
+ const summary = await withTimeout((async () => {
+ let resolvedTableExists = tableExists;
+ if (api) {
+ const known = await api.knownTablesOrNull();
+ if (known) {
+ const knownSet = new Set(known);
+ resolvedTableExists = (name: string) => knownSet.has(name);
+ }
+ }
+ return runPull({
query,
tableName: config.skillsTableName,
install,
cwd: install === "project" ? (deps.cwd ?? process.cwd()) : undefined,
users: [],
dryRun: false,
force: false,
- tableExists,
- }),
- timeoutMs,
- );
+ tableExists: resolvedTableExists,
+ });
+ })(), timeoutMs);Also applies to: 116-128
🤖 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/skillify/auto-pull.ts` around lines 108 - 109, The call to
api.knownTablesOrNull() happens outside the withTimeout(...) block causing
SessionStart to potentially exceed timeoutMs; move the table discovery into the
same timeout boundary by invoking knownTablesOrNull via withTimeout(timeoutMs,
() => api.knownTablesOrNull()) (or call withTimeout separately and treat
failures/timeouts as null), then set tableExists = (name: string) =>
known.includes(name) only if known is returned; apply the same change to the
other discovery usage around lines 116-128 so both discovery paths degrade to
null on timeout and don’t block SessionStart.
…dget knownTablesOrNull() (a live GET /tables on a cold cache) was awaited before withTimeout(), so a slow table lookup could block SessionStart past timeoutMs — defeating the timeout's purpose. Defer discovery into the timed block so the table fetch + pull share one budget and degrade together on timeout.
kaghni
approved these changes
May 27, 2026
…e thresholds The fix/skip-missing-table-reads changes added DeeplakeApi.knownTablesOrNull and a tableExists gating predicate in session-start.ts and context.ts that the existing tests never exercised (mocks returned null, so the predicate closure was never constructed), dropping three per-file coverage thresholds: deeplake-api branches 86.36% < 87%, session-start functions 83.33% < 90%, context functions 66.66% < 90%. - deeplake-api.test.ts: add knownTablesOrNull block covering all branch arms (cached hit, fresh [] / populated success, non-retryable + network-exhaust failures returning null without caching). - cli-context + session-start-hook tests: make the knownTablesOrNull mock configurable and add cases with a trusted table list so the tableExists predicate runs (both-present -> SELECTs fire; empty -> SELECTs skipped). - standalone-embed-client.test.ts: fix a test-isolation bug surfaced by the added tests shifting parallel scheduling. The skip-guard probed SHARED_DAEMON_PATH (the daemon entry .js), but a live daemon answers on a per-user socket whose liveness is independent of that file, so a dev box with embeddings installed returned a vector instead of null. Assert the result shape (null or numeric vector) per the test's documented intent instead of probing the filesystem.
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
SessionStart reads
hivemind_rules/hivemind_goals(context renderer) andskills(auto-pull) on every start, relying on catchingrelation does not exist. The client handled it, but the SELECT still failed server-side, so pg-deeplake logged a42P01per read on every fresh/empty workspace — the single largest cluster in the prod error digest (~1,060/24h, dominated byhivemind_goals/hivemind_rules/skills).This gates those reads on a trusted table list so we never issue the SELECT for a table that isn't there.
DeeplakeApi.knownTablesOrNull()— returns the cached table list, ornullwhen the lookup is untrusted (lets callers tell an empty workspace apart from a failed fetch).renderContextBlockandrunPulltake an optionaltableExistspredicate; when it reports a table absent, the SELECT is skipped (section/rows empty).hooks/session-start.ts(claude),hooks/hermes,hooks/cursor,commands/context.ts, andskillify/auto-pull.ts. (codex doesn't render the block; it only auto-pulls, covered centrally.)Fallback preserved: the existing
isMissingTableErrorcatch stays. When the list is unavailable (predicate omitted), behavior is unchanged — a transient lookup blip never drops a read of a table that exists. The pre-check only fast-skips when we have a confident list showing the table is absent.listTables()is cached and already warmed by the write paths each session, so this is typically zero extra HTTP calls.Scope: kills the bare-name reads (
hivemind_rules/hivemind_goals/skills). Schema-qualified stragglers (default.sessions, etc.) come from SDK/shell paths and are out of scope.Test plan
npm run typecheckcleantableExistsreports absent, and still SELECT when presentDeeplakeApitest mocks withknownTablesOrNull()cli-index/install-consentinstall-consent flow,standalone-embed-clientdefaults — none import these modules)Summary by CodeRabbit
New Features
Bug Fixes
Tests