sql(postgres): make finish_request's per-class counter decrement idempotent - #34756
Conversation
…potent PostgresSQLConnection::finish_request decremented nonpipelinable_requests or pipelined_requests based on the request's flags at fire time, from three call sites (the ReadyForQuery handler, the ErrorResponse handler, and connection-close cleanup) with no per-request idempotence guard. Under connection-failure timing the same request can be finished twice, driving the u32 past zero: debug builds panic with 'attempt to subtract with overflow'; release builds silently wrap to u32::MAX, after which advance()'s 'nonpipelinable_requests > 0' guard returns early forever and the connection stops dispatching queued queries. Record a per-request 'counted' bit alongside the simple/pipelined flags, set at the five sites that increment the counter and cleared by finish_request when it consumes that contribution. The decrement now no-ops once the bit is cleared. A debug_assert on the counter value and saturating_sub keep the release path safe even if a new call site is added without setting the bit.
|
Updated 8:05 PM PT - Jul 19th, 2026
❌ @robobun, your commit d96d396 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34756That installs a local version of the PR into your bun-34756 --bun |
|
Warning Review limit reached
Next review available in: 21 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
The diff is ready; the included test is a regression guard, not a fail-before test. The underflow only reproduces under syscall fault injection on the client socket (roughly 3% of 400-iteration runs, per the internal fault-plan lane); a zero-fault server-driven reproduction does not exist on current main because #33989's Honest multi-iteration probe on current main (debug+ASAN, 30-round #32004 workload against local Postgres, no fault injection): 0/5 The PR body has the full stack trace and the server-sequence analysis. Needs a maintainer merge on the analysis. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this touches the Postgres connection state machine (per-request counter bookkeeping across five increment sites and three finish_request callers) and the test is explicitly a regression guard rather than a fail-before repro — worth a human look.
What was reviewed:
- Verified all five
nonpipelinable_requests/pipelined_requestsincrement sites now setcounted = true; no other writers exist. - Checked that the early
returnon!countedin the Running/Binding/PartialResponse arm is behavior-preserving — every path reaching those states withsimple/pipelinedset also setscounted. - Confirmed the fixture's
wire-frameshelpers exist and the subprocess test drains stdout/stderr/exited concurrently and asserts on a combined object per harness conventions.
Extended reasoning...
Overview
The PR adds a counted: bool flag to PostgresSQLQuery::Flags and threads it through five counter-increment sites (two in PostgresSQLQuery::do_run, three in PostgresSQLConnection::advance). finish_request now gates its decrement on that flag, clears it, and replaces the raw - 1 with debug_assert!(n > 0) + saturating_sub(1). A new mock-server fixture drives one connection through the Z, E, and E+late-C+Z paths and asserts a follow-up query still dispatches.
Security risks
None. The change is internal bookkeeping on per-connection in-flight counters; no user input reaches the new code paths beyond what already flowed through them. saturating_sub degrades a hypothetical future accounting bug from a u32::MAX wrap (permanent stall) to a leaked-high counter, which is strictly safer.
Level of scrutiny
Medium-high. This is not a mechanical change: it modifies the Postgres connection state machine's request-accounting invariant across multiple call sites, and getting one increment site wrong would leak the counter high (stalling a pool connection). I grepped for every read/write of nonpipelinable_requests and pipelined_requests across src/sql_jsc/postgres/ and confirmed every increment is paired with counted = true in this diff, and confirmed the early return in the Running/Binding/PartialResponse arm cannot skip a decrement that the old code would have performed (any request in those states with simple or pipelined set necessarily came through an increment site that now also sets counted). Still, the PR's own analysis is intricate (fault-injection-only repro, interaction with #33989 and #32006) and a maintainer familiar with this state machine should confirm the design choice of per-request idempotence over, e.g., a terminal status transition.
Other factors
The author is transparent that the new test is a regression guard, not a fail-before test — the underlying race only reproduces under syscall fault injection and the deterministic server-driven path was already closed by #33989. REVIEW.md's "a test that passes both ways is worse than no test" is relevant here; the test does guard the new debug_assert and the follow-up-query-dispatches property, so it's not vacuous, but it doesn't prove the fix. The existing Postgres wire-frame suites are reported passing. Given the state-machine complexity and the non-fail-before test, I'm deferring rather than approving.
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
2 participants
What
PostgresSQLConnection::finish_requestdecrementednonpipelinable_requests/pipelined_requestsbased on the request'ssimple/pipelinedflags at fire time, from three call sites: theReadyForQueryhandler, theErrorResponsehandler, and connection-close cleanup. None of them record that a given request's contribution has been consumed, so under connection-failure timing the same request can be finished twice and theu32counter goes past zero.Debug builds panic:
Release builds silently wrap the counter to
u32::MAX, after whichadvance()'snonpipelinable_requests.get() > 0guard returns early forever and that pool connection stops dispatching queued queries. This is one of the mechanisms behind #32004's permanent pool stall (the other is the JS-side reserved-connection handoff, tracked in #32006).Fix
Add a per-request
countedbit to the query flags. Each of the five increment sites (two in the enqueue fast paths inPostgresSQLQuery.rs, three inadvance()inPostgresSQLConnection.rs) sets it alongside the increment.finish_requestonly decrements when the bit is set and clears it afterwards, so a second call is a no-op. The decrement also gains adebug_assert!(n > 0)and usessaturating_subso a future call site that forgets to set the bit trips the assert in debug and degrades to a counter leak (not a wrap) in release.Reproduction and fail-before
The underflow was caught by syscall fault injection on the client's Postgres socket against a real loopback server: the panic above reproduces at roughly 3% of 400-iteration runs under injection, always with a
PostgresError: Connection closedrejection printed in the same tick immediately before the panic. I could not reduce it to a zero-fault reproduction. The known server-driven path (ErrorResponse, then a late CommandComplete flipping the failed request back to PartialResponse so ReadyForQuery decrements again) was closed by #33989'sstatus == Failskip in the result-message handlers, and theon_datadispatch loop bails oncefail_with_js_valuehas run, which rules out the straightforward re-entrancy windows.multi-iteration probe on current main (debug+ASAN, no fault injection)
30-round #32004 workload (
sql.begin()interleaved with pooled parameterized queries,max: 4) against a local Postgres 17:The wedge persists with this fix applied (it is the #32006 JS-side pool bug). No
attempt to subtract with overflowpanic in any of 5 runs, with or without this fix, so the remaining trigger is fault-gated.The same workload on a debug build that predates #33989 panics 3/3 at
finish_request, which is what #33989's commit message documented; the server-driven path is what that PR closed, the fault-timed one is what this PR closes.A directed hostile-server probe (CommandComplete before the first ReadyForQuery, to push a never-dispatched Pending query through
on_result(false)into PartialResponse) does not reach the decrement either: the JS adapter parks queries until after startup and the resultingERR_POSTGRES_EXPECTED_REQUESTfails the connection beforefinish_requestruns. The decrement is on an internal per-request counter, not a wire field, so there is no single server byte sequence that reaches it twice.Given the above, the race is not fail-before-provable without instrumenting
src/(the gate stripssrc/for fail-before).test/js/sql/postgres-finish-request-underflow.test.tsis therefore a regression guard rather than a fail-before test: it drives a single connection through theZ,E, andE-then-late-C+Zpaths back-to-back against a scripted mock server and asserts a follow-up query still dispatches afterwards. A leaked-high or wrapped counter would stall that follow-up query until the fixture's watchdog fires, and a violated invariant trips the newdebug_assert.Verification
bun bd test test/js/sql/postgres-finish-request-underflow.test.ts: passestest/js/sql/Postgres suites (postgres-error-then-datarow,postgres-simple-query-pipeline,postgres-prepared-pipeline-reorder,postgres-split-prepare-reorder,postgres-multi-statement-fields,postgres-failed-connection-resurrection,postgres-datarow-overrun,wire-frames) still pass with the fix.cargo clippy -p bun_sql_jsc: clean