perf: cut fixed per-request round trips and add preamble Server-Timing - #1376
Conversation
Five latency fixes from the 2026-07-28 audit (docs/audit/latency-audit-2026-07-28.md). Rebased fresh on main; the index migration that sank PR #1312 is split out. RAG impact: no retrieval behaviour change — the only src/lib/rag/** edit defers a process-local cache write off the response path. No scoring, ordering, selection, alias, or citation logic is touched, and the mid-request staleness guard that discards the write when the corpus moves is preserved. - Server-Timing: /api/answer/stream emitted none at all, and stream/route.ts:262 confirms it is the route the UI actually uses. Adds auth/ratelimit there and to /api/search, plus scope on /api/answer. On a streaming route only pre-flush stages can reach a header; routing in-stream stages through the SSE contract would put instrumentation inside a governed clinical payload. - Shared-cache hits no longer await the cache write. setCachedAnswer forces an uncached documents read, so the fastest path in the system paid a round trip before responding. Deferred, not dropped. - /api/answer overlaps scope resolution with the rate-limit RPC and aborts it on deny, so a throttled caller still costs nothing. Threading the signal also fixes scope queries never receiving .abortSignal(). - document_table_facts: three select("*") narrowed to explicit projections, keeping the generated search_tsv and owner_id off the wire. The PATCH response is unchanged — the projection matches the TableFactRow DTO field for field. - 10 of 11 ssr:false dashboard surfaces had no loading fallback and rendered nothing between HTML arrival and chunk execution; all now use the shared LoadingPanel. Adds preconnect/dns-prefetch for the Supabase origin, which AuthProvider contacts on mount with no connection warm-up. Ledger #98-#105 record the 19 findings not addressed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 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 (9)
📝 WalkthroughWalkthroughThe PR adds a dated latency audit and related issue records, instruments timing for answer and search APIs, narrows table-facts projections, protects deferred RAG cache writes, and adds Supabase connection hints plus loading fallbacks for client-only surfaces. ChangesLatency audit and performance changes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AnswerRoute
participant RateLimiter
participant ScopeResolution
participant ServerTiming
Client->>AnswerRoute: Submit answer request
AnswerRoute->>RateLimiter: Authenticate and consume rate limit
RateLimiter-->>AnswerRoute: Return admission result and duration
AnswerRoute->>ScopeResolution: Resolve scope with request signal
ScopeResolution-->>AnswerRoute: Return scope or cancellation
AnswerRoute->>ServerTiming: Build timing header
ServerTiming-->>Client: Return response with Server-Timing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![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: c0b4844e6a
ℹ️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b4844e6a
ℹ️ 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".
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #6447 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
@cursoragent please resolve failing CI testing and do bugbot and resolve comments and merge conflicts |
|
Taking a look!   |
|
@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers. |
|
PR #1376 is mergeable with 0 unresolved review threads. Required static CI is green; only Production UI (Playwright) was still in progress at last poll, which keeps the aggregate Root causes found
Changes made
Validation
Remaining blockers
RAG impact: cache timing/invalidation only — no retrieval scoring, ordering, or selection changes. |
|
@codex review PR |
|
@CodeRabbit review PR |
|
✅ Action performedReview finished.
|
Keep resolveSearchScope behind the answer rate limiter so denied callers cannot enumerate documents/labels, and discard deferred shared-cache promotions that resume after invalidateRagCachesForOwner via an invalidation epoch. Drop the outstanding-issues reference to the unauthored L2-3 migration path that broke docs:check-links. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b4844e6a
ℹ️ 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".
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/rag-cache-invalidation.test.ts (1)
89-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the shared-cache promotion is discarded too.
This test only calls
getCachedAnswer, which exercises the process-local cache. It can pass while the deferred promotion still repopulatesrag_response_cache; capture the shared write/delete calls or perform a shared-cache read and assert that no stale row remains.🤖 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-cache-invalidation.test.ts` around lines 89 - 157, The test around setCachedAnswer and invalidateRagCachesForOwner must also verify the shared rag_response_cache is not repopulated with the stale answer. Capture the mocked shared-cache insert/delete calls or read the shared cache after awaiting promotion, and assert that no stale row remains in addition to the existing getCachedAnswer assertion.src/lib/rag/rag-cache.ts (1)
33-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScope the invalidation epoch by owner.
ragCacheInvalidationEpochis global whileinvalidateRagCachesForOwner(ownerId)is owner-scoped. A mutation for one tenant cancels unrelated tenants’ deferred promotions, causing avoidable cache misses under concurrent traffic. Use owner-specific generations plus a separate all-owner generation.Also applies to: 676-678
🤖 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/lib/rag/rag-cache.ts` around lines 33 - 40, Replace the global ragCacheInvalidationEpoch with owner-scoped invalidation generations and a separate all-owner generation. Update invalidateRagCachesForOwner and all deferred setCachedAnswer staleness checks to capture and compare the requesting owner’s generation together with the all-owner generation, so invalidating one owner does not cancel unrelated owners’ promotions.
🤖 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 `@docs/audit/latency-audit-2026-07-28.md`:
- Line 309: Remove the leading and trailing spaces inside the inline code span
in the RAG impact statement, while preserving the displayed text and surrounding
Markdown.
In `@docs/outstanding-issues.md`:
- Line 143: Update the `#099` row in docs/outstanding-issues.md to state that
admission via the rate-limit RPC occurs before scope resolution, with scope
resolution using the request abort signal. Remove the claim that scope
resolution overlapped with the rate-limit RPC, while preserving the remaining
status and cross-references.
In `@src/app/api/answer/route.ts`:
- Around line 100-108: Update the empty-scope success return in the answer route
to construct and attach the timing headers for auth, rate limit, scope
resolution, and total duration before returning. Keep the same headers for the
existing non-empty response path, reusing the timing values from the surrounding
scope and request timing logic.
In `@src/app/api/answer/stream/route.ts`:
- Around line 337-343: Update the scope-resolution flow around
resolveSearchScope to accept and forward streamSignal as its abort signal,
including through resolveRetrievalAccessScope if that wrapper is used before
streamAnswer. Preserve the existing scope behavior while ensuring client
cancellation aborts paginated scope queries.
In `@src/lib/rag/rag-cache.ts`:
- Around line 186-204: Close the race between the epoch check and shared-cache
commit in the cache-write flow around setSharedCachedAnswer and
invalidateRagCachesForOwner. Coordinate invalidation with the shared write using
a shared invalidation generation, or perform conditional post-write cleanup, so
any write overlapping invalidation cannot leave stale data. Add a race test that
interleaves invalidation with the shared write and verifies the stale answer is
absent.
In `@tests/private-rag-access.test.ts`:
- Around line 477-479: Update the rate-limit test around resolveSearchScope and
consumeSubjectApiRateLimit so the mock resolves a limiterStarted promise when
limiter entry begins; await that promise before asserting resolveSearchScope has
not been called, replacing the unreliable single Promise.resolve
synchronization.
---
Nitpick comments:
In `@src/lib/rag/rag-cache.ts`:
- Around line 33-40: Replace the global ragCacheInvalidationEpoch with
owner-scoped invalidation generations and a separate all-owner generation.
Update invalidateRagCachesForOwner and all deferred setCachedAnswer staleness
checks to capture and compare the requesting owner’s generation together with
the all-owner generation, so invalidating one owner does not cancel unrelated
owners’ promotions.
In `@tests/rag-cache-invalidation.test.ts`:
- Around line 89-157: The test around setCachedAnswer and
invalidateRagCachesForOwner must also verify the shared rag_response_cache is
not repopulated with the stale answer. Capture the mocked shared-cache
insert/delete calls or read the shared cache after awaiting promotion, and
assert that no stale row remains in addition to the existing getCachedAnswer
assertion.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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
Run ID: 46c593e2-2908-44ab-b4fa-cb0c044d1c47
📒 Files selected for processing (18)
docs/README.mddocs/audit/latency-audit-2026-07-28.mddocs/branch-review-ledger.mddocs/codebase-index.mddocs/outstanding-issues.mdsrc/app/api/answer/route.tssrc/app/api/answer/stream/route.tssrc/app/api/documents/[id]/table-facts/route.tssrc/app/api/search/route.tssrc/app/layout.tsxsrc/components/clinical-dashboard/clinical-dashboard-lazy.tsxsrc/components/clinical-dashboard/dashboard-nav.tsxsrc/lib/document-detail.tssrc/lib/rag/rag-cache.tssrc/lib/rag/rag.tssrc/lib/server-timing.tstests/private-rag-access.test.tstests/rag-cache-invalidation.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6a30aedf6
ℹ️ 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".
Scope invalidation epochs per owner, await shared-cache promotion with post-write cleanup on raced invalidation, thread the stream abort signal into resolveSearchScope, emit Server-Timing on empty-scope answers, and tighten the rate-limit gating test synchronization. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…BigSimmo/Database into claude/latency-fixes-2026-07-29 Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…ation Corrections found while planning the remaining work — each was verified at the cited line, and each REDUCES the work the audit implied: - L3-3 was wrong twice: the Therapy Compass index and full payloads are mutually exclusive (mode-home first paint is 707 KB, not 3.16 MB), and the index split the finding implied was missing already ships via build-therapies-index.mjs with its own gate. The real residual is only the missing /public Cache-Control and the proxy matcher not excluding .json. - L3-6(b) overstated a 3-hop first-paint waterfall. The 4-way fan-out is gated on drawer visibility, and readLocalProjectIdentity is a guaranteed no-op off localhost. Real production win is ~1 hop. - L3-6(c) proposed batching as new work, but /api/images/signed-urls already exists with zero call sites. Records the hard constraint that signed URLs must never reach SearchResult.images[].signed_url, because rag-cache persists results into a shared cross-owner cache table. - The measurement plan named test-cache-path.mjs as an offline fixture; it is the vitest/tsc cache-directory helper. Names the real offline surface and the vi.doMock seam instead. Also adds Refutation 6 to docs/rag-behaviour/refuted-approaches.md: token streaming as a latency fix. Unlike Refutations 1-5 this was not re-attempted — it is recorded because the defect it would "fix" is real and visible, so the wrong fix is the one a future task reaches for first. answer-stream-contract.ts removed token/revising as a clinical-safety control; raw tokens bypass the numeric-faithfulness gate. Only admissible shape is progressive disclosure of already-verified sections over the existing progress event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Awaiting replaceSharedCacheRow inside setCachedAnswer blocked every generation path on a Sydney DB round trip. Restore fire-and-forget shared promote while keeping owner-scoped epoch cleanup after commit. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…BigSimmo/Database into claude/latency-fixes-2026-07-29 Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9510a05d95
ℹ️ 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".
…2026-07-29 Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Following up my own reply on the offline-harness thread: restoring the script names was only half the fix. #1376 had added real implementation guidance to measurement-plan item 2 on main, and re-landing this audit overwrote it. That detail is what makes #98 buildable rather than merely described. Restored, with every reference verified rather than copied: - scripts/fixtures/rag-offline-contract-tests.json — exists. - The seam: vi.doMock("@/lib/supabase/admin", ...) before importing rag.ts. main cited tests/rag-offline-answer.test.ts:79-108; the admin doMock is actually at :102 (a second doMock at :108), so this cites :102. - Why it is the only zero-production-edit seam: rag.ts and rag-cache.ts construct their client inline per phase rather than accepting an injected one — verified, 4 and 8 createAdminClient() call sites respectively. - Ship as a vitest suite, not an npm script, so it rides npm run test instead of forcing a ci.yml edit via check-gate-manifest.mjs. Kept from this branch: the admission-cost direction guard, which main's copy lacks — any generalisation must assert scope starts only after the limiter admits and a denial dispatches zero scope queries, never the reverse. Also records why test-cache-path.mjs / check-rag-fixtures.mjs were the wrong reference, so the substitution is not repeated. Verification: verify:cheap exit 0 (427 files, 4386 passed / 4 skipped). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
2 participants
Summary
Five latency fixes from the 2026-07-28 audit (
docs/audit/latency-audit-2026-07-28.md), rebased fresh onmain. Supersedes #1312, which was closed because it bundled an index migration without synchronizedschema.sql/drift proof — that review was right, and the migration is split out of this change (tracked as#102, to land in one operator window that does apply → mirror → register → regenerate together).RAG impact: no retrieval behaviour change — the only
src/lib/rag/**edit defers a process-local cache write off the response path. No scoring, ordering, selection, alias, or citation logic is touched, and the mid-request staleness guard that discards the write when the corpus moves is preserved.Server-Timingwhere it was missing./api/answer/streamemitted none at all, andstream/route.ts:262confirms it is the route the UI actually uses — the largest measurement gap in the repo. Addsauth/ratelimitthere and to/api/search, plusscopeon/api/answer. On a streaming route only pre-flush stages can reach a header; routing in-stream stages through the SSE contract would put instrumentation inside a governed clinical payload, so they are deliberately omitted.rag.ts:3234).setCachedAnswerforces an uncacheddocumentsread, so the fastest path in the system paid a round trip before responding. Deferred, not dropped./api/answeroverlaps scope resolution with the rate-limit RPC and aborts it on deny, so a throttled caller still costs nothing. Threading the signal also fixes scope queries never receiving.abortSignal().document_table_facts: threeselect("*")narrowed to explicit projections, keeping the generatedsearch_tsvandowner_idoff the wire. The PATCH response shape is unchanged — the projection matches theTableFactRowDTO field for field.ssr:falsedashboard surfaces had noloadingfallback and rendered nothing between HTML arrival and chunk execution; all now use the sharedLoadingPanel(role="status"+ accessible name). Addspreconnect/dns-prefetchfor the Supabase origin, whichAuthProvidercontacts on mount with no connection warm-up.Four planned fixes were retired during verification rather than shipped — an in-process auth memo (would dedupe nothing), a batched rate-limit RPC (needs a migration first), a
differential_recordsbound (push-down is a no-op against the check constraint), and 8 further cache-write deferrals (change abort semantics and widen a post-awaitclone window). Each is recorded in the report with its evidence.Ledger
#098–#105record the 19 findings not addressed here.Verification
npm run verify:pr-local—Test Files 422 passed (422)·Tests 4272 passed | 3 skipped (4275), on a freshnpm ciagainst this basenpm run verify:ui— UI verification not run: the cross-worktree heavy lock was contended throughout; the client changes are aloadingfallback and two resource hints, both zero-payload. Worth running before merge.Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy) — unchanged, no Supabase config touchedprocess.env.NEXT_PUBLIC_SUPABASE_URLdirectly rather than theserver-onlyenv contract, keeping it out of the client module graph (tests/client-secret-surface.test.tsguards this)Notes
Server-Timingemits durations only, never query text or identifiers — see the trust-boundary constraint insrc/lib/server-timing.ts.Summary by CodeRabbit
New Features
Bug Fixes
Documentation