Repository navigation
fix(Bun.SQL) fix postgres error handling when pipelining and state reset - #22505
Conversation
|
Updated 9:18 PM PT - Sep 8th, 2025
❌ @cirospaciari, your commit 085a565 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22505That installs a local version of the PR into your bun-22505 --bun |
cirospaciari
changed the title
cirospaciari
changed the title
WalkthroughReworks Postgres connection into a statement-centric lifecycle with explicit statuses, centralizes per-request finalization via Changes
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLConnection.zig (2)
1149-1165: Pipeline bookkeeping during Bind/Execute is sound; add defensive checksThe transition to .binding/.running, setting pipelined flag, and bumping pipelined_requests align with the new flow. Add the same underflow assert pattern here to guard increments/decrements symmetry over time.
req.status = .binding; req.flags.pipelined = true; -this.pipelined_requests += 1; +bun.assert(this.flags.use_unnamed_prepared_statements == false); +this.pipelined_requests += 1;Also applies to: 1180-1205, 1206-1212
1749-1762: Reject subsequent pipelined requests after an ErrorResponseAfter writing Sync (per PostgreSQL protocol), walk your pending request queue and immediately call onError(...) for any pipelined requests sent after the one that failed. The server will discard those commands until it sees Sync/ReadyForQuery, so proactively failing them prevents promises from hanging indefinitely.
Optionally, add an integration test: pipeline two extended queries where the first errors and assert the second is rejected immediately (before ReadyForQuery).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/sql/postgres/PostgresSQLConnection.zig(10 hunks)test/js/sql/sql.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sql.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/sql/sql.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/sql/sql.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/sql/sql.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/sql/sql.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zig
🧬 Code graph analysis (1)
test/js/sql/sql.test.ts (3)
src/js/internal-for-testing.ts (1)
SQL(17-17)src/js/internal/sql/query.ts (1)
reject(196-211)src/js/internal/sql/postgres.ts (2)
err(366-395)err(397-426)
🔇 Additional comments (7)
src/sql/postgres/PostgresSQLConnection.zig (7)
932-933: Good call: centralize state accounting via finishRequest()Switching cleanup to finishRequest() avoids counter skew and keeps one source of truth for decrements. Consider adding asserts in finishRequest() to prevent accidental underflow if it’s ever called twice for the same item (see next comment).
1086-1088: canPrepareQuery guard looks rightBlocking prepares while not ready, while waiting_to_prepare, or while any pipeline is active prevents out-of-sync states.
1129-1144: Write-path error routing via tryTakeException() is correctFalling back from JS exception to onWriteFail() is the right split and fixes misclassified write failures.
1215-1220: Prepare path + immediate Sync is a solid resync boundary
- Waiting for pipeline to drain before preparing avoids the prior stuck state.
- Emitting Sync after writeQuery() guarantees a ReadyForQuery to bracket the parse phase.
Minor: prefer consistent error handling when writeQuery() or Sync writes fail (you already do this—nice).
Also applies to: 1221-1304
1315-1323: Offset jump over running items: verify off-by-one behaviortotal_requests_running = pipelined + nonpipelinable is correct; the conditional offset jump keeps scan O(1). Just ensure a request transitioning to .fail/.success mid-loop can’t be skipped indefinitely—your defer cleanup at Lines 1094–1112 covers this.
Would you like me to craft a focused unit test that enqueues N pipelined + 1 pending and asserts the scanner advances correctly after completions?
1458-1461: Finish partial on ReadyForQuery to release countersTying finalization to ReadyForQuery for partial_response is correct and prevents the post-error hang.
1773-1774: Standardizing onResult() for CloseComplete/EmptyQueryResponseUnifying through onResult(..., ready=false) keeps state transitions consistent.
Also applies to: 1789-1790
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1070-1085: Prevent counter underflow in finishRequest() (add assertions + safe decrement)nonpipelinable_requests/pipelined_requests are u32; double-finalization or unexpected states will wrap to 2^32−1. Add debug assertions and guard the decrements.
Apply:
fn finishRequest(this: *@This(), item: *PostgresSQLQuery) void { switch (item.status) { .running, .binding, .partial_response => { if (item.flags.simple) { - this.nonpipelinable_requests -= 1; + bun.assert(this.nonpipelinable_requests > 0); + if (this.nonpipelinable_requests > 0) this.nonpipelinable_requests -= 1; } else if (item.flags.pipelined) { - this.pipelined_requests -= 1; + bun.assert(this.pipelined_requests > 0); + if (this.pipelined_requests > 0) this.pipelined_requests -= 1; } }, .success, .fail, .pending => { if (this.flags.waiting_to_prepare) { this.flags.waiting_to_prepare = false; } }, } }
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLConnection.zig (2)
1086-1088: Make canPrepareQuery receiver constThis doesn’t mutate; take *const to communicate intent and enable calls on const receivers.
-pub fn canPrepareQuery(this: *@This()) bool { +pub fn canPrepareQuery(this: *const @This()) bool { return this.flags.is_ready_for_query and !this.flags.waiting_to_prepare and this.pipelined_requests == 0; }
1215-1304: Good: gated prepare with Sync resynchronization fixes pipeline-stuck states
- canPrepareQuery() blocks new prepares until the pipeline fully drains.
- Writing Parse/Describe (via writeQuery) followed by Sync, then flipping waiting_to_prepare=true, ensures server-side recovery after errors before proceeding.
One small suggestion: consider logging the prepared_statement_name and pipelined_requests at the decision point to aid field debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/postgres/PostgresSQLConnection.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zig
🔇 Additional comments (8)
src/sql/postgres/PostgresSQLConnection.zig (8)
932-933: Good: decrement counters when aborting in-flight requests during cleanupCalling finishRequest() here prevents leaked pipelined/non-pipelined counters when we tear down mid-flight. Looks correct.
1129-1134: Nice: distinguish JS exceptions from write failuresUsing tryTakeException() cleanly routes to onJSError vs onWriteFail, preventing swallowed protocol errors.
1149-1165: Early fail on statement.failed is correctShort-circuiting to onError() when the statement is already failed avoids spurious writes and keeps queue state consistent.
1180-1213: Correct lifecycle for prepared execution + counter bookkeepingSetting binding→running, marking req.flags.pipelined = true, and incrementing pipelined_requests ensures ReadyForQuery drives completion. The canPipeline gate is applied at the right time.
1316-1322: Efficient skip over running regionUsing global counters to jump past the currently running window avoids O(n) scans per tick. Looks good.
1750-1752: Good: finishRequest() on ErrorResponse avoids pipeline deadlocksDecrementing counters and then calling onError() ensures we don’t remain “blocked” until the server’s ReadyForQuery arrives.
1764-1765: Uniform result signaling for CloseComplete/EmptyQueryResponseDeferring finalization until ReadyForQuery via onResult(..., completed=false) aligns with the extended-query flow.
Also applies to: 1779-1781
1456-1461: No action needed: status transitions are correct
onResult(..., false)setsstatusto.partial_responseandonResult(..., true)sets it to.success.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1070-1081: Prevent counter underflow in finishRequest() and add diagnosticsDecrementing u32 counters without guards risks wrap-under on double-finalization or misclassified requests. Add debug asserts and trace the transition. This also makes cleanUpRequests() safe.
fn finishRequest(this: *@This(), item: *PostgresSQLQuery) void { switch (item.status) { .running, .binding, .partial_response => { - if (item.flags.simple) { - this.nonpipelinable_requests -= 1; - } else if (item.flags.pipelined) { - this.pipelined_requests -= 1; - } + const was_simple = item.flags.simple; + const was_pipelined = item.flags.pipelined; + if (was_simple) { + bun.assertf(this.nonpipelinable_requests > 0, + "finishRequest underflow: nonpipelinable_requests={d} (item={*}, status={s})", + .{ this.nonpipelinable_requests, item, @tagName(item.status) }); + this.nonpipelinable_requests -= 1; + } else if (was_pipelined) { + bun.assertf(this.pipelined_requests > 0, + "finishRequest underflow: pipelined_requests={d} (item={*}, status={s})", + .{ this.pipelined_requests, item, @tagName(item.status) }); + this.pipelined_requests -= 1; + } + debug("finishRequest: simple={} pipelined={} -> nonpipe={d} pipe={d}", + .{ was_simple, was_pipelined, this.nonpipelinable_requests, this.pipelined_requests }); }, .success, .fail, .pending => {}, } }
🧹 Nitpick comments (2)
src/sql/postgres/PostgresSQLConnection.zig (2)
1082-1084: Tighten canPrepareQuery() gate with status checkGuard on being connected to avoid accidental prepares during transient states.
-pub fn canPrepareQuery(this: *@This()) bool { - return this.flags.is_ready_for_query and !this.flags.waiting_to_prepare and this.pipelined_requests == 0; +pub fn canPrepareQuery(this: *@This()) bool { + return this.status == .connected + and this.flags.is_ready_for_query + and !this.flags.waiting_to_prepare + and this.pipelined_requests == 0; }
1312-1322: Queue-walk logic is subtle; add a brief comment to justify the “jump”The offset jump by pipelined_requests is correct but non-obvious. A short comment will prevent future regressions during maintenance.
- const total_requests_running = this.pipelined_requests; + // Skip over the currently pipelined window; we only consider enqueuing after it. + const total_requests_running = this.pipelined_requests;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/postgres/PostgresSQLConnection.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zig
🔇 Additional comments (9)
src/sql/postgres/PostgresSQLConnection.zig (9)
932-940: Good: centralize counter cleanup via finishRequest() on shutdown pathsCalling finishRequest() here avoids leaking in-flight counters before propagating errors.
1125-1130: Good error classification: JS exception vs write failureUsing tryTakeException() to route onJSError vs onWriteFail improves diagnostics and correctness on synchronous throw paths.
1145-1161: Correct: early-surface statement failures while pendingShort-circuiting pending requests when their statement is already failed prevents futile retries and keeps the queue consistent.
1176-1201: Solid: bind+execute pipeline accounting and gatingSetting binary/result flags, transitioning to .binding, flipping pipelined=true, and incrementing pipelined_requests are in the right order. The canPipeline() check avoids overfilling the buffer.
Also applies to: 1202-1208
1211-1306: Nice: prepare path writes Sync to resynchronize after pipeline errorsWriting Query then Sync, flipping waiting_to_prepare, and deferring flush via flushDataAndResetTimeout() aligns with PG protocol recovery best practices.
1451-1451: Good: clear waiting_to_prepare on ReadyForQueryPrevents deadlocks where prepares never re-open the pipeline.
1458-1461: Good: finalize partial responses on ReadyForQueryfinishRequest() before signaling completion ensures counters reflect the true terminal state.
1749-1751: Good: decrement counters on ErrorResponse before surfacing errorThis keeps pipeline/non-pipeline counts accurate across error paths.
1764-1764: Verify onResult(..., false) semantics for Close/EmptyQueryThese paths now pass false; ensure onResult’s finalization relies on ReadyForQuery to close out the request and that tests cover both message types.
Would you like me to add a focused test asserting no counter leaks and no hangs on CloseComplete/EmptyQueryResponse?
Also applies to: 1779-1779
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1064-1074: Prevent counter underflow in finishRequest()Decrementing u32 counters without guards can underflow and wedge pipelining logic. Add debug assertions before each decrement to catch double-finalization in dev builds.
fn finishRequest(this: *@This(), item: *PostgresSQLQuery) void { switch (item.status) { .running, .binding, .partial_response => { if (item.flags.simple) { - this.nonpipelinable_requests -= 1; + bun.assert(this.nonpipelinable_requests > 0); + this.nonpipelinable_requests -= 1; } else if (item.flags.pipelined) { - this.pipelined_requests -= 1; + bun.assert(this.pipelined_requests > 0); + this.pipelined_requests -= 1; } }, .success, .fail, .pending => {}, } }
🧹 Nitpick comments (3)
src/sql/postgres/PostgresSQLConnection.zig (3)
1076-1078: canPrepareQuery(): signature and gating look goodNice use of noalias and clear readiness gates.
Optional: assert the invariant during development that ready implies no outstanding requests:
bun.assert(!this.flags.is_ready_for_query or (this.pipelined_requests == 0 and this.nonpipelinable_requests == 0));
1119-1123: Deduplicate JS-exception vs write-failure routingThe tryTakeException/onWriteFail pattern repeats in several places. Centralize it to reduce drift.
Apply within the selected ranges:
- if (this.globalObject.tryTakeException()) |err_| { - req.onJSError(err_, this.globalObject); - } else { - req.onWriteFail(err, this.globalObject, this.getQueriesArray()); - } + this.routeWriteError(req, err);Add this helper near other small fns:
fn routeWriteError(this: *PostgresSQLConnection, req: *PostgresSQLQuery, err: AnyPostgresError) void { if (this.globalObject.tryTakeException()) |js_err| { req.onJSError(js_err, this.globalObject); } else { req.onWriteFail(err, this.globalObject, this.getQueriesArray()); } }Also applies to: 1174-1178, 1232-1240, 1263-1268, 1276-1282
1761-1761: Consistent completion signaling for CLOSE/EMPTYRouting these via onResult (non-final) aligns with the ReadyForQuery-based finalization.
Consider a brief comment where onResult(false) is used to clarify it marks partial_response and that ReadyForQuery will finalize.
Also applies to: 1776-1777
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sql/postgres/PostgresSQLConnection.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (6)
src/sql/postgres/PostgresSQLConnection.zig (6)
926-934: Good: call finishRequest() during cleanupEnsures counters stay consistent when we tear down mid-flight requests.
1141-1155: Early-exit on failed statements is correctPropagating the statement’s error to the request and continuing the scan avoids blocking the queue.
1170-1199: Pipeline bookkeeping and gating
- Setting binary based on fields and marking requests pipelined before incrementing the counter is right.
- Gating further pipelining when unnamed or canPipeline() fails prevents unbounded buffering.
Please run a quick stress to confirm counters reconcile under mixed load (N pipelined prepared execs followed by a simple query), i.e., nonpipelinable_requests remains 0 until ReadyForQuery and pipelined_requests drops to 0 at the corresponding ReadyForQuery boundary.
Also applies to: 1310-1318
1448-1456: ReadyForQuery cleanup ordering is rightClearing waiting_to_prepare and finishing partial before signaling completion avoids stuck counters.
1746-1748: Finish before onError on ErrorResponseGreat: counters and refs are reconciled prior to surfacing the error.
1205-1294: Extended-query batches already end with Flush+Sync in both prepareAndQueryWithSignature and bindAndExecute; no further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1070-1080: Add underflow guards when decrementing request countersDouble-finalization or mismatched flags can underflow u32 counters and break scheduling. Add debug assertions before decrements.
fn finishRequest(this: *@This(), item: *PostgresSQLQuery) void { switch (item.status) { .running, .binding, .partial_response => { if (item.flags.simple) { - this.nonpipelinable_requests -= 1; + bun.assert(this.nonpipelinable_requests > 0); + this.nonpipelinable_requests -= 1; } else if (item.flags.pipelined) { - this.pipelined_requests -= 1; + bun.assert(this.pipelined_requests > 0); + this.pipelined_requests -= 1; } }, .success, .fail, .pending => {}, } }
🧹 Nitpick comments (3)
src/sql/postgres/PostgresSQLConnection.zig (3)
1201-1204: Defensive check before toggling pipelined flag and incrementing counterAvoids accidental double-increment if the state machine re-enters unexpectedly.
- req.status = .binding; - req.flags.pipelined = true; - this.pipelined_requests += 1; + req.status = .binding; + bun.assert(!req.flags.pipelined); + req.flags.pipelined = true; + this.pipelined_requests += 1;
1315-1326: Minor: clarify offset jump logic with a short commentThe skip by pipelined_requests is subtle; a brief note will help future readers.
- const total_requests_running = this.pipelined_requests; + // Jump past the contiguous block of running pipelined requests to find the next schedulable item. + const total_requests_running = this.pipelined_requests;
487-491: Optional aggressive pipelining during prepare
Verified thatthis.flags.waiting_to_prepareandthis.pipelined_requestsare defined and toggled around query‐prepare/parse, so the new condition is syntactically valid. Apply to keep the pipeline full between parse and ReadyForQuery:- if (!this.flags.has_backpressure and this.flags.is_ready_for_query) { + if ( + !this.flags.has_backpressure and + (this.flags.is_ready_for_query or (this.pipelined_requests > 0 and !this.flags.waiting_to_prepare)) + ) { // no backpressure yet so pipeline more if possible and flush again this.advance(); this.flushData();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/sql/postgres/AnyPostgresError.zig(1 hunks)src/sql/postgres/PostgresSQLConnection.zig(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/sql/postgres/AnyPostgresError.zig
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLConnection.zig
🔇 Additional comments (6)
src/sql/postgres/PostgresSQLConnection.zig (6)
932-933: Good: centralizing per-status cleanup via finishRequest() before error propagationEnsures counters are reconciled before signaling errors.
1083-1085: noalias on canPrepareQuery() is correct and welcomeSignature and gating conditions look right.
1126-1131: Consistent JS-exception-aware error routing is solidPreferring onJSError when tryTakeException() succeeds avoids masking JS exceptions behind protocol errors.
Also applies to: 1180-1185, 1239-1245, 1269-1275, 1283-1289
1212-1301: Sync after writeQuery() is the right resync for extended-protocol errorsWriting protocol.Sync post-Parse to force ReadyForQuery realigns state after server-side errors during pipelined prepare. Good fix for the hang.
1455-1463: Good: R4Q clears waiting_to_prepare and finishes partial responses before signaling completionThis ordering avoids late counter updates and matches PG readiness semantics.
1767-1768: OK: unify completion via onResult() for CloseComplete/EmptyQueryResponseAssuming finalization happens on R4Q, this keeps the state machine consistent.
Also applies to: 1783-1784
| this.finishRequest(request); | ||
| this.updateRef(); | ||
| request.onError(.{ .protocol = err }, this.globalObject); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Finish current request on ErrorResponse before propagating error
Early counter reconciliation plus updateRef prevents leaked keep-alives. Looks correct given R4Q will follow.
To ensure all success paths also reconcile counters, please confirm either finishRequest() is invoked on successful completion or requests transition to partial_response and are finalized on R4Q:
🏁 Script executed:
#!/bin/bash
rg -nC2 'CommandComplete|EmptyQueryResponse|CloseComplete|finishRequest\(' src/sql/postgresLength of output: 6514
🏁 Script executed:
#!/bin/bash
rg -nC3 'ReadyForQuery' src/sql/postgres/PostgresSQLConnection.zigLength of output: 378
🏁 Script executed:
#!/bin/bash
nl -ba src/sql/postgres/PostgresSQLConnection.zig | sed -n '1450,1500p'Length of output: 40
Call finishRequest before updateRef and onResult in all full-response branches
In the .CommandComplete, .CloseComplete, and .EmptyQueryResponse handlers, invoke this.finishRequest(request); immediately before this.updateRef(); and the subsequent request.onResult(…) call so that counters are reconciled on every successful completion.
🤖 Prompt for AI Agents
In src/sql/postgres/PostgresSQLConnection.zig around lines 1753 to 1755, the
handlers for full-response cases (.CommandComplete, .CloseComplete,
.EmptyQueryResponse) must call this.finishRequest(request) immediately before
calling this.updateRef() and request.onResult(…) (or request.onError/onResult
equivalents). Modify those handlers so the order is:
this.finishRequest(request); this.updateRef(); then the request callback,
ensuring finishRequest is invoked on every successful completion branch to
reconcile counters.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1472-1483: Finish pipelined request counters in .CommandComplete
pipelined_requests never decrements since finishRequest(request) isn’t called; insert it before updateRef:.CommandComplete => { var request = this.current() orelse return error.ExpectedRequest; var cmd: protocol.CommandComplete = undefined; try cmd.decodeInternal(Context, reader); defer { cmd.deinit(); } debug("-> {s}", .{cmd.command_tag.slice()}); - defer this.updateRef(); - request.onResult(cmd.command_tag.slice(), this.globalObject, this.js_value, false); + this.finishRequest(request); + defer this.updateRef(); + request.onResult(cmd.command_tag.slice(), this.globalObject, this.js_value, false); },Add a focused test issuing two prepared SELECTs back-to-back without awaiting, asserting both resolve and pipelined_requests returns to zero after the final ReadyForQuery.
♻️ Duplicate comments (2)
src/sql/postgres/PostgresSQLConnection.zig (2)
1780-1784: And for EmptyQueryResponseTreat this as a full-response boundary for the head request and reconcile counters.
Apply:
.EmptyQueryResponse => { try reader.eatMessage(protocol.EmptyQueryResponse); var request = this.current() orelse return error.ExpectedRequest; - defer this.updateRef(); + this.finishRequest(request); + defer this.updateRef(); request.onResult("", this.globalObject, this.js_value, false); },
1764-1768: Call finishRequest in CloseComplete before updateRefAdd
this.finishRequest(request);immediately beforedefer this.updateRef();in the.CloseCompletecase to properly conclude the request cycle and avoid pending-activity leaks:@@ src/sql/postgres/PostgresSQLConnection.zig:1763 - defer this.updateRef(); + this.finishRequest(request); + defer this.updateRef();
🧹 Nitpick comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1830-1833: Nit: debug vs. log helperGuidelines mention
const log = bun.Output.scoped(.<Scope>, false);while this file usesdebug = bun.Output.scoped(.Postgres, .visible). If.visibleis intentional for this subsystem, ignore. Otherwise consider aligning to the guideline-specified verbosity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/sql/postgres/PostgresSQLConnection.zig(12 hunks)src/sql/postgres/PostgresSQLQuery.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/sql/postgres/PostgresSQLQuery.zigsrc/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/sql/postgres/PostgresSQLQuery.zigsrc/sql/postgres/PostgresSQLConnection.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (7)
src/sql/postgres/PostgresSQLQuery.zig (1)
118-120: Normalize JS exceptions via toError() when rejecting callbacks — good changePassing a real Error object improves parity with other error paths and tooling. No further concerns here.
src/sql/postgres/PostgresSQLConnection.zig (6)
329-331: Also normalize connection-close errors to JS Error — goodUsing
value.toError() orelse valuebefore invokingoncloseensures consistent error shapes for consumers.
487-492: Drain gating on is_ready_for_query looks correctOnly advancing when not backpressured and ready avoids writing into an extended-protocol window incorrectly. Looks good.
932-940: Cleanup path now reconciles counters before rejecting — goodCalling
finishRequest(request)in connection teardown prevents counter leaks on mid-flight queries.
1083-1086: noalias canPrepareQuery signature — good
noaliason the receiver is appropriate here and the readiness predicate matches the intent.
1454-1466: Guard against double-finalization on ReadyForQuery
Add a check onrequest.flags.is_donebefore callingfinishRequest:if (this.current()) |request| { - if (request.status == .partial_response) { - this.finishRequest(request); - // if is a partial response, just signal that the query is now complete - request.onResult("", this.globalObject, this.js_value, true); - } + if (request.status == .partial_response) { + if (!request.flags.is_done) { + this.finishRequest(request); + } + // signal completion + request.onResult("", this.globalObject, this.js_value, true); + } }Re-run regression #22395 to confirm recovery after a server-side error isn’t broken.
1115-1213: Only Sync emits ReadyForQuery, not Execute
PostgreSQL’s extended protocol only sends ReadyForQuery upon processing a Sync (or at the end of a simple Query cycle); Execute alone does not emit R4Q. Relying on CommandComplete to close each request prevents rows for N+1 from being misattributed to N. Confirm your implementation expects ReadyForQuery only after Sync, not after Execute.
What does this PR do?
Fixes: #22395
How did you verify your code works?
Test