fix(ai): best-effort per-conversation send serialization (advisory lock) - #2071
Conversation
Adds startGenerationExclusive (apps/web/src/lib/ai/core/start-generation-exclusive.ts), wrapping takeover + stream-lifecycle create in a per-conversation Postgres advisory try-lock (ai-send:<conversationId>, withAdvisoryLock/getAdvisoryLockPool). Closes the check-then-act race documented at chat/route.ts:1193-1211 between takeOverConversationStreams's SELECT and createStreamLifecycle's INSERT. Best-effort, not an invariant: on lock_busy, retries 3x300ms then proceeds unlocked (availability over serialization), emitting a named metric (logPerformance) and a structured warn — never silent. The lock-busy retry/degrade decision is a pure function (decideOnLockBusy), 100% branch tested. Wired into both POST /api/ai/chat and POST /api/ai/global/[id]/messages. Existing route-level mock-DB tests updated with a lock-always-free advisory pool mock so they exercise the same takeover+lifecycle-create path as before. Leaves a seam note on PR 2's board page (assistant message row at stream start): the placeholder insert slots in as one more await inside startGenerationExclusive's run closure, no changes needed to the primitive itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197HJSWMLvTu8qe75V7Dkvs
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ 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 |
![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: b21061c129
ℹ️ 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".
|
|
||
| let attemptsMade = 0; | ||
| for (;;) { | ||
| const attempt = await withAdvisoryLock(pool, lockKey, run); |
There was a problem hiding this comment.
When the advisory-lock pool cannot connect or pg_try_advisory_lock throws (for example after adding this new 10-connection pool in a deployment that is near Postgres max_connections while the main DB pool is still usable), withAdvisoryLock rejects here and both chat routes fall into their 500 handlers before run executes. That makes the best-effort serialization lock a hard availability dependency and regresses sends that previously continued through takeover/lifecycle creation; lock acquisition errors should take the same logged unlocked fallback as lock_busy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26a03fe: startGenerationExclusive now wraps the withAdvisoryLock(...) call in try/catch. On a lock-machinery failure (pool.connect() or the try-lock query throwing — pool exhaustion, connection reset), it degrades to unlocked immediately (no retry, since a broken connection is not "busy" and wont resolve on a 300ms cadence) rather than propagating and 500ing the send. Both degrade paths (lock_busy after retry exhaustion, and this lock_error path) now share one telemetry helper (degradeToUnlocked), tagged with reason so they stay distinguishable in logs/metrics. 5 new tests cover pool.connect() rejection, a poisoned try-lock query, no-retry-on-lock_error, and telemetry content — 15/15 passing at 100% branch coverage. Leaving this thread open for verification against the new commit.
… just lock_busy
startGenerationExclusive only handled the resolved lock_busy outcome — a
pool.connect() or try-lock query failure (pool exhaustion, connection reset on
the dedicated advisory-lock pool) threw straight out of withAdvisoryLock
uncaught, turning the PR's own "a send must never block on this lock"
guarantee into a hard 500. The PR page's own verification checklist already
required this exact case ("Lock-pool exhaustion simulation: proceeds
unlocked... degraded = today"), which the implementation didn't satisfy.
Wraps the withAdvisoryLock call in try/catch: on a lock-machinery failure,
degrades to unlocked immediately (no retry — a broken connection isn't
"busy" and won't resolve on a 300ms cadence). Both degrade paths now share
one telemetry helper (degradeToUnlocked), tagged with reason: 'lock_busy' |
'lock_error' so they stay distinguishable in logs/metrics.
5 new RED-then-GREEN tests; 15/15 passing at 100% line/branch/function
coverage.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0197HJSWMLvTu8qe75V7Dkvs
CI (web:lint via next build) failed: 'error' is defined but never used (@typescript-eslint/no-unused-vars) in the catch block added for the lock-connection-failure fix. My local `bun run typecheck` only ran `tsc --noEmit`, which doesn't catch ESLint rules — `bun run lint` does, and I hadn't run it before pushing. Fixes it by actually using the error: degradeToUnlocked now takes an optional `error` field, stringified into the structured warn (not the metric — kept lean/structured) so a lock_error degrade is diagnosable in logs, matching the pattern withAdvisoryLock itself uses for its own unlock-failure logging. 2 new tests cover the error-message plumbing (Error instance and a thrown non-Error value) — 16/16 passing at 100% branch coverage. Confirmed `bun run lint` exits 0 and `bun run typecheck` is clean before this push. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197HJSWMLvTu8qe75V7Dkvs
|
@coderabbitai review |
✅ Action performedReview finished.
|
…-run tail
Proactive self-review (/simplify) on the degrade path added in the previous
fix commit surfaced two low-risk cleanups, both applied:
- degradeToUnlocked's destructure-then-conditionally-respread for the error
field was a double negative (strip error out, then maybe splice a
differently-shaped one back in via a ternary yielding {} or {error}).
Flattened to one computed errorMessage plus a single flat object literal.
toEqual (used in the tests) ignores undefined-valued keys, so this is
behaviorally identical — confirmed by the unchanged test assertions.
- The two call sites (lock_busy exhaustion, lock_error catch) each repeated
the same "telemeter, run(), wrap in {outcome:'degraded'}" tail.
degradeToUnlocked now does the run()+wrap itself and returns the outcome
directly, so each call site is one line.
Also evaluated and declined two other candidates from the review:
- A discriminated-union restructuring of the try/catch (removing the `let
attempt`) — read cleaner in isolation but doesn't fix the underlying
ambiguity (withAdvisoryLock can't structurally distinguish its own
connection failures from `run` throwing); filed as a D task instead of
a cosmetic-only local change.
- Passing the error through the codebase's PII-scrubbing logger.error() path
instead of a plain string field — logger.warn() has no Error-aware channel
at all in this codebase, and passing error.message as a string metadata
field matches 45+ existing call sites; not a defect unique to this PR.
Filed a D task at the epic level (Server Stream Durability & Rejoin,
lmlmfg9ooagf88dzonqt4yph, task fmfmzw4g4gh6u6q9cjt7ylne) for the deeper
structural fix: withAdvisoryLock should resolve a connection_error outcome
instead of throwing for pool.connect()/try-lock-query failures, so a future
addition to `run` (PR 2's message insert) can't cause misclassified
telemetry + a double-run if it ever throws. Out of scope for this PR's leaf
(4.1-4.3 already satisfied) since it touches a shared, tested primitive with
another live caller (reconcileMachineStorageSerialized).
16/16 tests passing, 100% branch/line/function coverage maintained.
bun run lint / typecheck clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0197HJSWMLvTu8qe75V7Dkvs
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 40 minutes. |
1 participant
Summary
PR 4 of the "Server Stream Durability & Rejoin" epic — closes the check-then-act race
between
takeOverConversationStreams's SELECT andcreateStreamLifecycle's INSERT(documented at
apps/web/src/app/api/ai/chat/route.ts:1193-1211), which allowed twonear-simultaneous sends on the same conversation to both see zero in-flight rows and
both proceed — two generations, two sets of tool calls, two bills.
startGenerationExclusive(apps/web/src/lib/ai/core/start-generation-exclusive.ts),wrapping takeover + stream-lifecycle-create in a per-conversation Postgres advisory
try-lock (
ai-send:<conversationId>, viawithAdvisoryLock/getAdvisoryLockPool—packages/db/src/advisory-lock.ts).lock_busy, retries3×300ms then proceeds unlocked — availability wins over serialization, a send must
never block on this lock.
lock_busy— the lock is held by a concurrent send on the SAME conversation; retriesexhausted, proceeds unlocked.
lock_error— the lock connection itself failed (pool.connect()or the try-lock querythrew: pool exhaustion, connection reset); proceeds unlocked immediately, no retry (a
broken connection isn't "busy" and won't resolve on a 300ms cadence). Added after
self-review and an independent Codex review both caught that the initial implementation
only handled the resolved
lock_busyoutcome — a connection failure propagateduncaught and turned "never block a send" into a hard 500.
logPerformance) + structured warn, tagged withreasonso they stay distinguishable in logs/metrics — never silent.decideOnLockBusy) is a pure function, 100%branch tested.
POST /api/ai/chatandPOST /api/ai/global/[id]/messages.so they continue exercising the same takeover+lifecycle-create path as before.
placeholder insert slots in as one more
awaitinsidestartGenerationExclusive'srunclosure — no change to the primitive itself needed.Test plan
bun run typecheck(web) — cleanbun run lint(web) — clean (0 errors; pre-existing unrelated warnings only)bun vitest run src/lib/ai/core/__tests__/start-generation-exclusive.test.ts— 16/16, 100% line/branch/function coveragebun vitest run src/lib/ai/core/__tests__— 895+/895+ passingbun vitest run src/app/api/ai— 473+/473+ passing (+3 in the gate-callsites guard)master(verified viagit merge-treeafter master advanced)🤖 Generated with Claude Code
https://claude.ai/code/session_0197HJSWMLvTu8qe75V7Dkvs