perf(rag): parallelise independent retrieval hydration - #1474
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMemory artifact loading and application are extracted into reusable functions. RAG retrieval now overlaps memory loading with document metadata hydration across three branches, with tests verifying concurrency and call-site coverage. ChangesRAG memory hydration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Retrieval as RAG retrieval path
participant Metadata as attachDocumentRankingMetadata
participant Memory as loadMemoryBoostArtifacts
participant Applier as applyMemoryBoostArtifacts
par Metadata hydration
Retrieval->>Metadata: hydrate candidate metadata
Metadata-->>Retrieval: metadataCandidates
and Memory artifact loading
Retrieval->>Memory: load memory artifacts
Memory-->>Retrieval: memory artifacts
end
Retrieval->>Applier: merge and apply memory boosts
Applier-->>Retrieval: boosted candidates
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d19f013d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #7178 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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/rag-retrieval-parallelism.test.ts (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-text assertions don't actually verify concurrency or correctness.
Both tests parse
rag.tsas a string and check for literal substrings/regex matches rather than exercising the function. This provides weak guarantees:
expect(helper).toContain("await Promise.all([")passes even if that code is dead/unreachable, or ifattachDocumentRankingMetadata/loadMemoryBoostArtifactsare actually awaited sequentially elsewhere while an unusedPromise.all([...])literal merely exists in the sliced text.- The slice end marker
"/** Attach document ranking metadata. */"is a hardcoded comment string; if that docstring is ever reworded (e.g. by a formatter or doc pass),indexOfreturns-1andslice(start, -1)silently truncates to almost the entire file instead of failing loudly, defeating the purpose of the test.callCountregex/await hydrateCandidatesWithMetadataAndMemory\(\{/gis brittle to formatting (e.g. object argument on its own line) and doesn't confirm the calls are on the branches they're meant to be on.Consider replacing with a behavioral test that mocks
attachDocumentRankingMetadataandloadMemoryBoostArtifacts(e.g. viavi.mock) with deferred promises, and asserts that both are invoked before either resolves — this actually protects the perf/correctness invariant this PR introduces, rather than the source layout.🤖 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/rag-retrieval-parallelism.test.ts` around lines 1 - 25, Replace the source-text assertions in the retrieval hydration parallelism tests with a behavioral test of hydrateCandidatesWithMetadataAndMemory. Mock attachDocumentRankingMetadata and loadMemoryBoostArtifacts with deferred promises, invoke the helper, and assert both operations are started before either promise resolves; retain coverage that the helper is used across the intended retrieval branches without brittle formatting or comment-string matching.
🤖 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/lib/rag/rag.ts`:
- Around line 1496-1531: Update hydrateCandidatesWithMetadataAndMemory so the
concurrently executed metadata and memory reads are timed as one combined
wall-clock phase rather than wrapping each branch with measureSearchPhase.
Preserve the existing parallel Promise.all behavior and result assembly, and
record the single elapsed duration under an appropriate shared phase while
avoiding separate metadata_hydration and memory_hydration latency charges.
---
Nitpick comments:
In `@tests/rag-retrieval-parallelism.test.ts`:
- Around line 1-25: Replace the source-text assertions in the retrieval
hydration parallelism tests with a behavioral test of
hydrateCandidatesWithMetadataAndMemory. Mock attachDocumentRankingMetadata and
loadMemoryBoostArtifacts with deferred promises, invoke the helper, and assert
both operations are started before either promise resolves; retain coverage that
the helper is used across the intended retrieval branches without brittle
formatting or comment-string matching.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32043fc7-ce75-4772-aa1c-06bbffa7da04
📒 Files selected for processing (3)
src/lib/rag/rag-candidate-sources.tssrc/lib/rag/rag.tstests/rag-retrieval-parallelism.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
# Conflicts: # docs/outstanding-issues.md # src/lib/rag/rag.ts
BigSimmo
added
the
skip-branch-sync
There was a problem hiding this comment.
Pull request overview
This PR reduces RAG retrieval latency by overlapping two independent hydration reads—document ranking metadata and memory-card evidence—while keeping candidate assembly deterministic (memory boosts applied only after both reads complete) and maintaining existing caching semantics.
Changes:
- Refactors memory-boost hydration to split “load artifacts” vs “apply artifacts”, enabling independent execution from metadata hydration (
rag-candidate-sources.ts). - Introduces
hydrateCandidatesWithMetadataAndMemoryto run metadata + memory hydration concurrently and applies memory boosts deterministically afterward (rag-hydration.ts), then migrates three post-gate retrieval branches to use it (rag.ts). - Adds a focused contract test to assert the concurrency boundary and call-site adoption, and updates an eval test fixture to reflect the new phase key (
tests/*).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/rag-retrieval-parallelism.test.ts | Adds a contract test asserting parallel hydration and verifying the helper is used at three call sites. |
| tests/eval-retrieval.test.ts | Updates the latency telemetry fixture to use the new combined hydration phase key. |
| src/lib/rag/rag.ts | Replaces three serial metadata→memory hydration sequences with the new parallel helper while preserving deterministic ordering. |
| src/lib/rag/rag-hydration.ts | Adds hydrateCandidatesWithMetadataAndMemory, running metadata + memory reads concurrently and applying memory boosts after both settle. |
| src/lib/rag/rag-candidate-sources.ts | Splits memory hydration into loadMemoryBoostArtifacts + applyMemoryBoostArtifacts, keeping withMemoryBoostedCandidates as a wrapper. |
| docs/branch-review-ledger.md | Appends a review ledger entry for this PR’s review/run evidence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { readFileSync } from "node:fs"; | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| describe("retrieval hydration parallelism", () => { | ||
| it("overlaps only independent metadata and memory reads before deterministic assembly", () => { | ||
| const source = readFileSync("src/lib/rag/rag-hydration.ts", "utf8"); | ||
| const helper = source.slice( | ||
| source.indexOf("export async function hydrateCandidatesWithMetadataAndMemory"), | ||
| source.indexOf("/** Attach document ranking metadata. */"), | ||
| ); | ||
|
|
||
| expect(helper).toContain("Promise.all(["); | ||
| expect(helper).toContain("attachDocumentRankingMetadata("); | ||
| expect(helper).toContain("loadMemoryBoostArtifacts({"); | ||
| expect(helper.indexOf("applyMemoryBoostArtifacts(")).toBeGreaterThan(helper.indexOf("Promise.all([")); | ||
| }); | ||
|
|
||
| it("uses the parallel helper on all post-gate vector and document-lookup branches", () => { | ||
| const source = readFileSync("src/lib/rag/rag.ts", "utf8"); | ||
| const callCount = source.match(/await hydrateCandidatesWithMetadataAndMemory\(\{/g)?.length ?? 0; | ||
|
|
||
| expect(callCount).toBe(3); | ||
| }); | ||
| }); |
fix #186 archive - Restore #156 (read-modify-write race + Update-branch corruption) from archive back to the open issues section with its original full text; the underlying allocation race is not resolved by the conservative architecture decision described in the previous commit. - Re-add #156 to the recommendations priority table as row 51 (between #101 and #172, consistent with its original ordering). - Correct #101 to credit only metadata and memory parallelisation (hydrateCandidatesWithMetadataAndMemory) from PR #1474; visual hydration (attachPageVisualEvidence) is still called serially after hydration on all six call sites and is explicitly listed as a remaining candidate. - Update #186 archive text to reflect that only metadata+memory were updated in #101, not visual hydration. Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>
Motivation
Description
withMemoryBoostedCandidatesintoloadMemoryBoostArtifactsandapplyMemoryBoostArtifactsinsrc/lib/rag/rag-candidate-sources.tsso memory reads can run independently of metadata reads.hydrateCandidatesWithMetadataAndMemoryinsrc/lib/rag/rag-hydration.tswhich runs metadata and memory reads concurrently withPromise.alland then applies memory boosts deterministically after both settle.tests/rag-retrieval-parallelism.test.tsthat asserts the concurrency boundary and that the helper is invoked at the three migrated call sites.Testing
npm run formatandnpm run format:changed(passed).npm run typecheck(passed).node scripts/run-vitest.mjs run tests/rag-retrieval-parallelism.test.ts tests/rag-round-trip-budget.test.tsand full unit suite vianpm run testas part ofverify:pr-local(all executed tests passed).npm run eval:rag:offline(36 golden cases, 22 suites; 572 tests passed).Files changed (high level):
src/lib/rag/rag-candidate-sources.ts,src/lib/rag/rag.ts, and new testtests/rag-retrieval-parallelism.test.ts.Codex Task
Summary by CodeRabbit
RAG impact: behaviour change — canary pair 30578169116 -> 30579534353
Risk and rollout
Retrieval candidate ordering is protected by the measured baseline/post pair. Roll back the squash commit if production latency or retrieval telemetry regresses; no provider configuration or production data is changed.
Clinical Governance Preflight