fix(ai): guard placeholder INSERT + harden run-failure classification - #2080
Merged
Conversation
PR #2076 added an unguarded db.insert() for the assistant placeholder row directly inside startGenerationExclusive's `run` closure in both chat routes, with no try/catch unlike its sibling operations. The closure's caller assumed `run` never throws: if the insert threw while the advisory lock was held, the exception was misclassified as lock-machinery failure and `run` was invoked a SECOND time, unlocked — double generation, double billing. Fix has two layers: - Both routes: wrap the placeholder INSERT in its own try/catch (log-and- continue), matching takeOverConversationStreams/createStreamLifecycle in the same closure. - start-generation-exclusive.ts: guardedRun wrapper tags whether `run` itself threw (runThrew flag) so the outer catch can tell a run-failure apart from a lock-machinery failure and rethrow it instead of degrading to an unlocked retry. This is a structural guarantee, not just documentation — `run` can never be invoked twice regardless of what throws inside it. RED test mirrors the reviewer's exact repro (run rejects once with the lock genuinely acquired): pre-fix, run was invoked twice and the call silently "succeeded" as a degraded outcome; post-fix, the error propagates once and run is called exactly once. 100% branch coverage on the classification path.
Contributor
|
Warning Review limit reached
Next review available in: 20 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 (6)
✨ 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: cb2a0e7fa4
ℹ️ 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".
Comment on lines
+126
to
+131
| let runThrew = false; | ||
| const guardedRun = async (): Promise<T> => { | ||
| try { | ||
| return await run(); | ||
| } catch (error) { | ||
| runThrew = true; |
There was a problem hiding this comment.
If the lock is acquired and run() completes, withAdvisoryLock still executes its finally unlock/release path afterward; if that cleanup/release throws after generation has already started, runThrew remains false and this catch treats the error as lock_error, calling degradeToUnlocked(..., run) and starting the same generation a second time. The guard needs to record that run was entered, not only that it threw, before allowing the unlocked fallback.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Confirmed and fixed in 270bcb6. You're right — runThrew only covered run throwing, not the case where run succeeds but withAdvisoryLock's post-run lock release then throws (finally overriding the successful return). Broadened the flag to runSettled, set on both the success and failure paths of guardedRun, so any error surfacing after run has settled at all propagates as a genuine failure instead of triggering the unlocked retry. Added a regression test that reproduces exactly this (unlock query rejects + destroy-on-release fallback also throws) and verified it was RED against the runThrew-only version before the fix, GREEN after. 100% branch coverage maintained.
…-review Self-review pass on the double-generation hotfix (PR #2080): - Corrected route.ts comments that overstated the placeholder-INSERT guarantee ("a second send can never observe a stream row without its placeholder") — the new try/catch makes that best-effort, not an invariant. Corrected the error-cleanup comments that still cited "the placeholder INSERT itself failing" as a reason `lifecycle` never gets assigned — no longer possible now that the insert can't throw. - start-generation-exclusive.ts: hoisted the `guardedRun`/`runThrew` guard above the retry loop instead of recreating it every iteration — a `runThrew` catch always throws out of the function immediately, so there is no cross-iteration state to reset. - Both stream-socket-events.test.ts route tests already exercise the real startGenerationExclusive (only the advisory-lock pool is mocked), so they are genuine regression tests for this bug. Fixed their comments, which described the old degrade-and-retry behavior, and added an explicit toHaveBeenCalledTimes(1) assertion on the mocked run-closure operation as a direct regression guard. - start-generation-exclusive.test.ts: replaced a mislabeled test (claimed to test "the same conversation" while using two different conversation ids) with one that actually exercises cross-invocation isolation on the same conversationId; switched an internal-implementation-detail assertion (raw query count) to the actual release() contract. No behavior change. 100% branch coverage maintained on start-generation-exclusive.ts; all touched test files green; typecheck and lint clean.
… failure Codex review on PR #2080 (thread PRRT_kwDOPhnPxc6Q_IVP) found a real gap in the runThrew-only guard: withAdvisoryLock releases the Postgres session in a `finally` AFTER `fn()` resolves (packages/db/src/advisory-lock.ts:76-103). If the unlock query rejects AND the destroy-on-release fallback (`client.release(err)`) also throws, that exception escapes the finally block uncaught, overriding a successful `run()` return — so withAdvisoryLock's promise rejects despite `run` having already completed. `runThrew` (set only in guardedRun's catch) stayed false in that case, so the outer catch still misclassified it as `lock_error` and invoked `run` a second time, unlocked — the exact double-generation bug this PR exists to close, just via a different trigger. Renamed the flag to `runSettled` and set it on BOTH the success and failure paths of guardedRun: once `run` has settled at all (resolved or rejected), any later exception from withAdvisoryLock must propagate as a genuine failure, never trigger the unlocked retry. Verified RED against the runThrew-only version (git stash) before applying this fix, then GREEN after. 100% branch coverage maintained; full route + core test suites (129 tests) green; typecheck and lint clean.
2witstudios
added a commit
that referenced
this pull request
Jul 15, 2026
….ts conflict PR #2080 (guard placeholder INSERT + harden run-failure classification) merged into master and conflicted with this branch's leaf 5.6 change to start-generation-exclusive.ts. Resolved in favor of this branch's design: withAdvisoryLock's structural connection_error outcome (leaf 5.6) means startGenerationExclusive never catches withAdvisoryLock's rejection at all, only branches on its resolved outcome. That makes #2080's runSettled flag — built to disambiguate a caught rejection's cause before deciding whether re-invoking run unlocked was safe — provably redundant here: with no catch site, there is no reclassification step left for it to guard, for any rejection cause including the "post-run lock release throws" edge case #2080 added a test for. Verified directly: all 21 tests in start-generation-exclusive.test.ts, including that edge case, pass unchanged against the simpler shape. Updated the file's docstring and inline comments to explain the reconciliation for future readers of either PR's history. The rest of the merge (both chat routes' try/catch around the placeholder INSERT, CI config, coverage-ratchet additions) applied cleanly with no conflicts and is orthogonal to this branch's changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WDuPiAGSLXE3vxC59is3Ro
2witstudios
added a commit
that referenced
this pull request
Jul 15, 2026
…t conflict PR #2077 (materialize interrupted streams on dead-row reap) merged into master right after the previous #2080 merge landed, and touched the same GET /api/ai/chat/active-streams route this branch's leaf 5.1 added the scope=user discovery mode to. route.ts itself auto-merged cleanly: leaf 5.1's scope=user branch is untouched, and #2077's lazy-reap loop (materializeInterruptedStream on provably-dead rows) is correctly scoped to the pre-existing channelId branch only — the two modes don't interact. Only the test file conflicted, on the vi.hoisted() mock declaration: leaf 5.1's tests need mockWhere (asserting the scope=user WHERE clause), #2077's tests need mockMaterializeInterruptedStream (asserting the reap side effect). Both are exercised elsewhere in the now-merged file, so both are kept in one combined vi.hoisted() block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WDuPiAGSLXE3vxC59is3Ro
6 tasks
2witstudios
added a commit
that referenced
this pull request
Jul 17, 2026
…throws release contract CI caught a cross-file regression from 518876d: withAdvisoryLock's release machinery (unlockAndRelease/releaseQuietly) now never throws, so the #2080 double-failure scenario (unlock query rejects + destroy release(err) throws) no longer escapes the finally and reject an already-successful run. The test pinning that old contract failed. The property the test guards is unchanged and still asserted: run() executes exactly once and is never re-invoked unlocked. Updated to also assert the new, stronger outcome — the caller keeps the successful locked result instead of losing an already-started generation to lock-cleanup noise, the destroy is still attempted, and no lock_error degrade telemetry fires. Stale docblock references to the old finally-can-throw behavior refreshed. Audited every other real-advisory-lock consumer: machine-storage-billing branches on the resolved connection_error outcome (23/23 pass); the processor workers use their own pre-helper raw-pg copies (out of scope, unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Jul 17, 2026
…e, poll-fallback) + notify mentions on interrupted replies (#2097) * test(ai): RED — materializeInterruptedStream must fire mention notifications like the finalize path D task st3pyh9q4zwnmae00j195o97: an interrupted reply that @mentions a user currently vanishes without notifying anyone — the normal finalize path (saveMessageToDatabase) fires notifyMentionedUsers behind the route's gate (page.driveId present, conversation explicitly shared, non-empty content), but a materialized interrupted reply never does. Spec: exactly one notification per actually-written materialization, never a duplicate when the CAS upsert skipped (the row's own onFinish already notified), fail closed on missing page/conversation, best-effort + warn on lookup failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(ai): GREEN — close R.4 coverage gaps + notify mentions on interrupted replies Four items from the E2 PR5 R.4 audit (D.1/D.2/D.3 + the epic-level mention D task): 1. materializeInterruptedStream now fires notifyMentionedUsers for a page-chat reply it materializes, mirroring the finalize path's gate (page.driveId, conversation isShared, non-empty content; stream owner as triggeredBy, page title as mentioner). The upsert gained .returning so the notification only fires when the CAS actually wrote — a row already terminalized by its own onFinish (which already notified) can never double-page the mentioned user. Best-effort: lookup/notify failures warn and never un-succeed the materialization. Module stays at 100% branch (36/36). 2. advisory-lock.ts 90% -> 100% branch: the non-Error unlock-rejection wrap (pg release(err) destroy contract + operator-visible log) is now tested. The last uncovered branch was a v8 artifact — the catch->finally junction of a catch that exits abruptly on every path, unreachable by any input — so the function was flattened per the epic rail ("a branch you didn't design; fix the design"): try-lock gets its own catch, the clientPoisoned flag is gone, unlock/destroy lives in a named helper. All 7 pre-existing behavior tests pass unchanged. 3. checkpoint-serialize.ts 93.33% -> 100% branch: mismatched/truncated originRawIndex falls back to survivingFromRawIndex 0 — the under-skip (self-correcting duplicate) direction, never over-skip (content loss). 4. stream-join-poll-fallback.ts 78.94% -> 100% branch: abort landing at each await point inside an in-flight tick discards the stale snapshot and never fires a bogus onNotFound; an abort before the clearInterval listener registers is caught by the per-tick entry guard; AbortError rejections with a live signal stay silent but keep polling; a body with no streams field is terminal, not a crash. Thresholds are ENFORCED in config: per-glob 100% entries for all three modules. packages/db's vitest.config.ts gained the #2081 whole-line ratchet sentinel first — scripts/coverage-ratchet.mjs covers that config, and its plain-regex fallback would truncate a thresholds block containing per-glob sub-objects. Verified with coverage-ratchet --dry-run on both configs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(ai): RED — every terminal write must carry the mention gate exactly once (Codex P2) Codex review on PR #2097: the route has three writes that can flip the assistant placeholder out of 'streaming' (execute-end, onFinish, outer-catch cleanup) but only onFinish carried mentionNotify. execute-end's own docblock says "when onFinish never runs, this write stands as the sole record" — in that documented gap the @mention notification was permanently lost, and materialize-interrupted-stream's CAS-gated notify (which assumes "the route flipped it => the route notified") could never recover it. Spec: whichever terminal write lands FIRST carries mentionNotify, exactly once per request; the once-flag latches only on a successful save so a failed execute-end persist still lets onFinish notify; the private- conversation gate holds on every path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ai): GREEN — mention gate travels with whichever terminal write lands first (Codex P2) Route-scoped mentionNotifyFor helper + once-flag, hoisted out of the try so the outer-catch cleanup can use it too. All three writes that can flip the assistant placeholder out of 'streaming' (execute-end, onFinish, cleanup) now attach mentionNotify when they are the request's first successful terminal write; the flag latches only after a successful save so a failed execute-end persist still lets onFinish notify. The gate is unchanged (page.driveId + requester + explicitly-shared conversation) plus saveMessageToDatabase's own content.trim() firing condition, so the flag can only latch when a notification would actually have been dispatched. This makes materialize-interrupted-stream's CAS-gated notify premise true: any row the route flipped out of 'streaming' WAS notified by the route, so skipping notification on a CAS miss can no longer lose an @mention in the execute-end-then-death window Codex flagged. Comment there updated to cite the real contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(db): CodeQL — lock key never reaches the format-string position; review-pass cleanups CodeQL flagged the unlock-failure console.error (js/tainted-format-string high, js/log-injection medium): lock keys can be derived from request- supplied ids, so interpolating one into console.error's first argument would let a crafted key smuggle %-directives or forge log lines. Now a constant format string with the key newline-stripped as a %s argument, pinned by a test (evil\nkey\r%s -> 'evil key %s', never in the format). Review-pass cleanups (self-review finders, behavior pinned by existing tests throughout): - advisory-lock: fn-error/success unlock duplication collapsed to one try/finally (unlockAndRelease never throws, so it cannot mask fn's rejection); still 100% branch. - route.ts: the gate+attach+latch trio, hand-copied at all three terminal writes, now lives in one saveTerminalAssistantMessage helper — a fourth terminal write can't get the exactly-once protocol wrong. Dead page.driveId ternary removed (schema NOT NULL). - materialize-interrupted-stream: the two inline gate selects replaced with the readers the live paths already use — pageRepository.findById (also closes a real gap: a page trashed between stream death and reap no longer pages drive members) and conversationRepository.getConversation (the route's own isConversationShared source, so the two gates can't drift). Test mocks moved to the repository boundary; redundant inner beforeEach deleted. Still 100% branch. - Cross-route mention-gate extraction (v1 completions still builds its gate inline) filed as an epic-level D task (class 4) rather than done as a drive-by. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(db): unlockAndRelease honors its never-throws contract even when release() throws Second self-review pass (finders A/B/C on the final shape; the two correctness finders independently re-confirmed the cross-process duplicate already filed on the epic board): - advisory-lock: if the success-path client.release() throws (release hook, pool shut down), the catch's destroy-release then throws pg's synchronous double-release error, which escaped the catch and replaced a successful fn() result with a rejection inside withAdvisoryLock's finally. The destroy-release is now guarded and logged; two new tests pin the contract (Error and non-Error release throws). Still 100% branch (10 tests). - route.ts: the exactly-once mention latch documented honestly as best-effort — the latch flips only after the save resolves, so overlapping same-process terminal writers and the cross-instance materializer race both resolve to a DUPLICATE ping, never a lost one (the epic's chosen direction; idempotent-notification fix filed as an epic D task). Execute-end comment also names the content source caveat (buffered snapshot vs onFinish's refined message), filed likewise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(db): releaseQuietly on every advisory-lock exit; JSON.stringify the logged key CodeRabbit review + CodeQL alert 262 on PR #2097: - Every exit of withAdvisoryLock now funnels release() through a shared never-throw releaseQuietly helper — the lock_busy and try-lock-failure paths were still calling release() bare, so a throwing release there replaced the promised resolved outcome with a rejection (inconsistent with the contract the previous commit hardened on the acquired path). Two new tests pin lock_busy and connection_error resolving through a throwing release. 12 tests, still 100% branch. - The logged lock key switches from replace(/[\r\n]/g,' ') to JSON.stringify: CodeQL's log-injection model did not recognize the replace form (alert 262 fired on the sanitized line itself), and JSON escaping is strictly stronger — newlines become literal \n, the constant format string keeps %-directives inert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(ai): re-pin start-generation-exclusive to advisory-lock's never-throws release contract CI caught a cross-file regression from 518876d: withAdvisoryLock's release machinery (unlockAndRelease/releaseQuietly) now never throws, so the #2080 double-failure scenario (unlock query rejects + destroy release(err) throws) no longer escapes the finally and reject an already-successful run. The test pinning that old contract failed. The property the test guards is unchanged and still asserted: run() executes exactly once and is never re-invoked unlocked. Updated to also assert the new, stronger outcome — the caller keeps the successful locked result instead of losing an already-started generation to lock-cleanup noise, the destroy is still attempted, and no lock_error degrade telemetry fires. Stale docblock references to the old finally-can-throw behavior refreshed. Audited every other real-advisory-lock consumer: machine-storage-billing branches on the resolved connection_error outcome (23/23 pass); the processor workers use their own pre-helper raw-pg copies (out of scope, unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
URGENT HOTFIX for a double-generation/double-billing race reopened by PR #2076.
PR #2076 added an unguarded
db.insert()for the assistant placeholder row directly insidestartGenerationExclusive'srunclosure, in bothapps/web/src/app/api/ai/chat/route.tsandapps/web/src/app/api/ai/global/[id]/messages/route.ts— no try/catch, unlike the sibling operations in that closure (takeOverConversationStreams,createStreamLifecycle). The caller (start-generation-exclusive.ts) assumesrunnever throws: if the insert threw while the advisory lock was genuinely held, the exception was misclassified as lock-machinery failure anddegradeToUnlockedinvokedruna second time, unlocked — double generation, double billing.Reviewer-verified and reproduced (board:
nxkkhcv8yjnuvm9ake60jg4r, task D.1 ontza273pegokkzpd7ut5qvxky).start-generation-exclusive.ts: aguardedRunwrapper tags whetherrunalready settled (resolved or rejected) —runSettled— so the outer catch rethrows any error surfacing after that point as a genuine failure instead of degrading to an unlocked retry. This is a structural guarantee —runcan never double-invoke regardless of what throws inside it, not just documentation of an assumed contract.withAdvisoryLockreleases the Postgres session in afinallyafterrun()resolves — if that release itself throws (unlock query fails + the destroy-on-release fallback also throws), the exception overrides a successfulrun()return, and arunThrew-only flag (set only on the failure path) would still misclassify it aslock_errorand double-invoke an already-succeededrun. Broadened the flag torunSettled, set on both the success and failure paths — closes that gap too. Verified RED against therunThrew-only version before applying, GREEN after.Test plan
runrejects once then would resolve on a second call) — pre-fix,runwas invoked twice and the call "succeeded" as a silent degraded outcome.runis called exactly once.runsucceeds, then the unlock query + destroy-on-release fallback both throw — pre-fix this degraded and double-invokedrun; post-fix it propagates the release error once.start-generation-exclusive.ts.chat/global messagesroute test suites green (129 tests, including two now-strengthened regression tests that exercise the realstartGenerationExclusiveend-to-end).apps/web+ full monorepotypecheckgreen.🤖 Generated with Claude Code
https://claude.ai/code/session_01BFRRMrLvWqEjUhYHTRebRy