fix: preserve cross-tenant search id collisions#364
Conversation
📝 WalkthroughWalkthroughFixes cross-tenant deduplication in ChangesCross-tenant search fan-out fix
Test expectation updates
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libravdb-client.ts (1)
450-470: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep tenant context off shared client state. The runtime reuses a single
LibravDBClientacross sessions, andbefore_prompt_buildmutatesclient.setTenantKey(...)on that same instance. InsearchTextCollections, swappingthis.tenantKeyacrossawaitboundaries lets overlapping RPCs pick up the wrong tenant header, so reads/writes can be routed to the wrong tenant. Pass tenant context per request or serialize tenant swaps.🤖 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/libravdb-client.ts` around lines 450 - 470, `searchTextCollections` is mutating shared tenant state on `LibravDBClient`, which can leak the wrong tenant across overlapping calls. Update the method so it does not swap `this.tenantKey` around each `await`; instead pass the tenant context per request in `client.searchTextCollections` or otherwise isolate each tenant call. Keep the dedupe loop and `readTenants` iteration in place, but ensure tenant selection is local to each RPC and not stored on the shared client instance.
🧹 Nitpick comments (2)
test/unit/libravdb-client.test.ts (1)
360-388: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the happy path; consider covering the error-path restoration.
The
try/finallyaddition specifically guards againsttenantKeyleaking when a per-tenant call throws, but this test only exercises the success path. A stub that rejects for one tenant would directly validate thefinallyrestoration this PR introduces.Optional: add error-path test
+test("tenantKey is restored even if a tenant read throws", async () => { + const client = new LibravDBClient({ endpoint: "tcp:127.0.0.1:9" }); + (client as any).client = { + searchTextCollections: async () => { + if ((client as any).tenantKey === "tenant-a") throw new Error("boom"); + return { results: [] }; + }, + }; + client.setTenantKey("primary"); + client.setReadTenants(["tenant-a", "tenant-b"]); + await client.searchTextCollections({ collections: ["global"], text: "x", k: 5 }); + assert.equal((client as any).tenantKey, "primary"); +});🤖 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 `@test/unit/libravdb-client.test.ts` around lines 360 - 388, Add an error-path test for LibravDBClient.searchTextCollections to verify the new try/finally restoration logic. In the existing cross-tenant test area, stub the underlying client.searchTextCollections to reject for one tenant (using the tenantKey/tenant loop behavior) and assert that tenantKey is restored to its original value even after the failure. Keep the focus on the searchTextCollections method and the tenantKey state transition.src/libravdb-client.ts (1)
466-466: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePer-tenant read failures are silently swallowed with no observability.
If a tenant's
searchTextCollectionscall fails (auth error, network issue, etc.), it's dropped without any logging, making cross-tenant search failures invisible in production.Optional: add lightweight logging on skipped tenant reads
- } catch { /* skip failed tenant reads */ } + } catch (err) { + // best-effort fan-out — log but don't fail the aggregate search + console.warn(`searchTextCollections: skipped tenant ${t}`, err); + }🤖 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/libravdb-client.ts` at line 466, The per-tenant read failure in the `searchTextCollections` flow is being swallowed by the empty `catch` block, so add lightweight logging there before skipping the tenant. Update the retry/tenant iteration path in `src/libravdb-client.ts` to capture the thrown error and emit a minimal warning with tenant/context details, while still continuing to the next tenant. Keep the existing behavior of skipping failed reads, but make the failure observable through the relevant search helper that wraps `searchTextCollections`.
🤖 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.
Outside diff comments:
In `@src/libravdb-client.ts`:
- Around line 450-470: `searchTextCollections` is mutating shared tenant state
on `LibravDBClient`, which can leak the wrong tenant across overlapping calls.
Update the method so it does not swap `this.tenantKey` around each `await`;
instead pass the tenant context per request in `client.searchTextCollections` or
otherwise isolate each tenant call. Keep the dedupe loop and `readTenants`
iteration in place, but ensure tenant selection is local to each RPC and not
stored on the shared client instance.
---
Nitpick comments:
In `@src/libravdb-client.ts`:
- Line 466: The per-tenant read failure in the `searchTextCollections` flow is
being swallowed by the empty `catch` block, so add lightweight logging there
before skipping the tenant. Update the retry/tenant iteration path in
`src/libravdb-client.ts` to capture the thrown error and emit a minimal warning
with tenant/context details, while still continuing to the next tenant. Keep the
existing behavior of skipping failed reads, but make the failure observable
through the relevant search helper that wraps `searchTextCollections`.
In `@test/unit/libravdb-client.test.ts`:
- Around line 360-388: Add an error-path test for
LibravDBClient.searchTextCollections to verify the new try/finally restoration
logic. In the existing cross-tenant test area, stub the underlying
client.searchTextCollections to reject for one tenant (using the
tenantKey/tenant loop behavior) and assert that tenantKey is restored to its
original value even after the failure. Keep the focus on the
searchTextCollections method and the tenantKey state transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a0549cb-81f4-4e39-afe4-ea6cf0659004
📒 Files selected for processing (5)
src/libravdb-client.tstest/integration/checklist-validation.test.tstest/integration/host-flow.test.tstest/unit/libravdb-client.test.tstest/unit/slot-conflict.test.ts
Vale Review — PR #364Quality: Q4/5 — sharp Findings:
Proof gaps: No local gate run; finding is from current head source and requires a concurrency regression test to pin down. Verdict: request-changes — the dedupe fix is good, but tenant fan-out still routes through shared mutable state across await boundaries. – Vale |
1 participant
Summary
searchTextCollectionshits when different readable tenants return the same record id by deduping on tenant + id.finallyafter fan-out.Quality
Quality: Q5/5
Name: cross-tenant search drops same-id hits from later readable tenants
Evidence: focused regression failed before the fix, preserving only
tenant-awhentenant-aandtenant-bboth returnedshared-id.Repro: the focused
libravdb-clientunit test failed before the patch and passes after it.Impact: agents with cross-tenant read access can silently miss relevant memories from other tenants when daemon-local record ids collide.
Fix confidence: high; dedupe scope now matches the tenant isolation boundary.
Risk: low; single-tenant path is unchanged, and multi-tenant fan-out still dedupes duplicate ids within the same tenant.
Testing
pnpm run clean:test && pnpm exec tsc -p tsconfig.tests.json && node --test .ts-build/test/unit/libravdb-client.test.jspnpm run test:tspnpm run test:integrationpnpm check– Vale
What changed
searchTextCollectionsfan-out deduping to preserve same-record hits across tenants by keying ontenant + idinstead ofidalone.finallyblock after multi-tenant fan-out.Tests updated
Complexity review