sql: reject close() timeouts that overflow setTimeout's millisecond range#32100
sql: reject close() timeouts that overflow setTimeout's millisecond range#32100robobun wants to merge 2 commits into
Conversation
|
Updated 3:45 PM PT - Jun 12th, 2026
❌ @robobun, your commit 7fadfe2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32100That installs a local version of the PR into your bun-32100 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughIntroduces validateCloseTimeout and MAX_CLOSE_TIMEOUT, centralizes timeout coercion/validation for SQL close() flows, applies it in pool/transaction/reserved close paths, and adds tests asserting overflow and edge-case rejection while preserving connection usability. ChangesSQL close() timeout overflow fix
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 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 `@test/js/sql/sql-close-timeout-overflow.test.ts`:
- Around line 103-112: The test leaks the reserved connection if an assertion
fails because reserved.release() is called after assertions; wrap the test body
that performs assertions and queries in a try/finally (or register cleanup) so
reserved.release() runs on every exit path. Specifically, after obtaining the
connection via sql.reserve() and performing the reserved.close() call/assertions
and the subsequent reserved`select 1 as x` check, ensure reserved.release() is
placed inside a finally block (or registered before assertions) so it always
executes even when expect() throws.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 73ecfc54-3067-42f2-8da5-d1957fe3d6ac
📒 Files selected for processing (5)
src/js/bun/sql.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tstest/js/sql/sql-close-timeout-overflow.test.ts
robobun
force-pushed
the
farm/32148b3c/sql-close-timeout-overflow
branch
from
d5e4b90 to
3b87de4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/js/internal/sql/shared.ts`:
- Around line 1222-1248: The timeout branch is incorrectly gated by truthiness
so a provided timeout of 0 is treated as "no timeout"; update the check to
detect presence (e.g., options?.timeout !== undefined) so that timeout =
validateCloseTimeout(timeout) is used for zero and the immediate-close behavior
(this.closed = true; if (timeout === 0 || !this.hasPendingQueries()) { await
this.#close(); return; }) runs; ensure the rest of the timeout flow
(Promise.withResolvers, setTimeout using timeout * 1000, timer.unref(),
this.onAllQueriesFinished clearing timer and calling
this.#close().finally(resolve)) remains unchanged.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: b8d90858-952d-473d-8e46-8e1d73956e0a
📒 Files selected for processing (3)
src/js/bun/sql.tssrc/js/internal/sql/shared.tstest/js/sql/sql-close-timeout-overflow.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@src/js/internal/sql/shared.ts`:
- Around line 1222-1248: The timeout branch is incorrectly gated by truthiness
so a provided timeout of 0 is treated as "no timeout"; update the check to
detect presence (e.g., options?.timeout !== undefined) so that timeout =
validateCloseTimeout(timeout) is used for zero and the immediate-close behavior
(this.closed = true; if (timeout === 0 || !this.hasPendingQueries()) { await
this.#close(); return; }) runs; ensure the rest of the timeout flow
(Promise.withResolvers, setTimeout using timeout * 1000, timer.unref(),
this.onAllQueriesFinished clearing timer and calling
this.#close().finally(resolve)) remains unchanged.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: b8d90858-952d-473d-8e46-8e1d73956e0a
📒 Files selected for processing (3)
src/js/bun/sql.tssrc/js/internal/sql/shared.tstest/js/sql/sql-close-timeout-overflow.test.ts
🛑 Comments failed to post (1)
src/js/internal/sql/shared.ts (1)
1222-1248:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
timeout: 0by presence, not truthiness.Line 1223 and Line 1247 both gate on
if (timeout), sosql.close({ timeout: 0 })never enters the timeout path and does not take the intended immediate-close branch. That leaves0behaving like “no timeout provided” instead of “force close now”.💡 Suggested fix
async close(options?: { timeout?: number }): Promise<void> { if (this.closed) { return; } let timeout = options?.timeout; - if (timeout) { + const hasTimeout = timeout === 0 || !!timeout; + if (hasTimeout) { timeout = validateCloseTimeout(timeout); this.closed = true; if (timeout === 0 || !this.hasPendingQueries()) { // close immediately @@ return promise; } else { this.closed = true; if (!this.hasPendingQueries()) {🤖 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/js/internal/sql/shared.ts` around lines 1222 - 1248, The timeout branch is incorrectly gated by truthiness so a provided timeout of 0 is treated as "no timeout"; update the check to detect presence (e.g., options?.timeout !== undefined) so that timeout = validateCloseTimeout(timeout) is used for zero and the immediate-close behavior (this.closed = true; if (timeout === 0 || !this.hasPendingQueries()) { await this.#close(); return; }) runs; ensure the rest of the timeout flow (Promise.withResolvers, setTimeout using timeout * 1000, timer.unref(), this.onAllQueriesFinished clearing timer and calling this.#close().finally(resolve)) remains unchanged.
|
Re the latest CodeRabbit finding on the timeout truthiness gate in shared.ts close(): that behavior is pre-existing, this PR only replaces the bound check inside the branch. close({ timeout: 0 }) being treated as "no timeout" is tracked in #32038 and has a dedicated fix open in #32039, so changing it here would be an out of scope behavior change that conflicts with that PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused validation fix with thorough test coverage.
Extended reasoning...
Overview
This PR fixes a timeout-overflow bug in sql.close({ timeout }): the option is in seconds, but the deadline timer is armed in milliseconds, and the old upper-bound check (> 2 ** 31 seconds) allowed values whose millisecond product overflows setTimeout's 32-bit limit and gets clamped to 1 ms. The fix consolidates three identical inline checks into a shared validateCloseTimeout helper bounded at (2 ** 31 - 1) / 1000 seconds, and reorders validation in reserved_sql.close / transaction_sql.close to run before clearing acceptQueries, so a rejected close no longer poisons the handle. A new test file covers rejection of overflowing values, acceptance of the exact boundary, continued rejection of previously-invalid inputs, and the validate-before-mutate ordering across postgres/mysql/sqlite/reserved paths.
Security risks
None. This is input validation tightening on a numeric timeout option; no auth, crypto, network, or untrusted-input parsing is involved.
Level of scrutiny
Low-to-moderate. The change is small (~30 source lines), mechanical, and follows the existing validation pattern used for idleTimeout / connectionTimeout / maxLifetime in the same file. The only behavior changes are (1) rejecting timeouts in (2147483.647, 2^31] seconds that were previously accepted-but-broken, and (2) a more accurate error message — both strict improvements with no realistic compatibility impact.
Other factors
- The bug-hunting system found no issues.
- CodeRabbit's only comment (release the reserved connection on assertion failure) was addressed via
await usingand confirmed resolved. - No CODEOWNERS apply to these paths.
- The reordering preserves semantics:
validateCloseTimeoutthrows inside async functions, so callers still see a rejected promise; the splitif (timeout) { validate } … if (timeout) { use }is equivalent to the prior single block for all inputs (0, falsy, truthy, string-coercible). - Test coverage is well-designed: pool tests need no server (lazy connect), the sqlite transaction path runs in-process, and only the reserved-connection test is docker-gated.
…ange The timeout option is in seconds but the deadline timer is armed in milliseconds. The upper bound was checked against the seconds value (2 ** 31), so timeouts in (2147483.647, 2 ** 31] passed validation and setTimeout clamped the overflowing delay to 1 ms, force-closing the pool and cancelling in-flight queries. All four close() sites now share one validator bounded at (2 ** 31 - 1) / 1000 seconds. On reserved connections and transactions, validation now runs before acceptQueries is cleared so a rejected close leaves the handle usable. Fixes #32096
robobun
force-pushed
the
farm/32148b3c/sql-close-timeout-overflow
branch
from
3b87de4 to
7fadfe2
Compare
There was a problem hiding this comment.
LGTM — focused fix that corrects the seconds-vs-milliseconds bound and moves validation ahead of state mutation, with thorough test coverage.
Extended reasoning...
Overview
This PR fixes #32096: sql.close({ timeout }) validated the upper bound against the seconds value (2 ** 31) but then passed timeout * 1000 to setTimeout, so values in (2147483.647, 2 ** 31] seconds overflowed the 32-bit ms range and were clamped to 1 ms, force-closing immediately. The fix introduces a shared validateCloseTimeout in internal/sql/shared.ts bounded at (2 ** 31 - 1) / 1000 seconds and replaces the three inline checks (pool close in shared.ts, reserved_sql.close and transaction_sql.close in sql.ts). It also reorders the reserved/transaction close paths so validation runs before clearing acceptQueries, so a rejected close no longer leaves the handle unusable. A new test file covers overflow rejection, boundary acceptance, the error message, and post-rejection usability for sqlite transactions and (docker-gated) postgres reserved connections.
Security risks
None. This is pure input-validation tightening on a numeric option in JS-land; no auth, crypto, parsing of untrusted data, or native code is touched. The only behavior change is that previously-accepted absurdly large timeouts now reject with ERR_INVALID_ARG_VALUE instead of silently misbehaving.
Level of scrutiny
Low-to-moderate. The production diff is ~30 lines: a 10-line helper plus three call-site substitutions and two statement reorderings. The bound math ((2 ** 31 - 1) / 1000) is correct for setTimeout's documented limit, and the reordering is a clear improvement (validate-then-mutate). The split into two if (timeout) blocks preserves the existing control flow exactly aside from where the throw lands relative to the state flip.
Other factors
- No CODEOWNERS for these paths.
- The bug-hunting system found nothing.
- The one CodeRabbit finding (reserved-connection cleanup on assertion failure) was addressed via
await usingand confirmed resolved. - The author explicitly scoped out the pre-existing
timeout: 0truthiness quirk (#32038/#32039) and the unrelated reserved-close hang (#32099), keeping this change minimal. - Tests verify both the new rejection boundary and that previously-rejected inputs (
-1, non-numeric) stay rejected, and were reported passing on the fixed build and failing on the unfixed one.
|
CI status: every lane that ran is green (284 jobs, including the ASAN lane). The only outstanding check is darwin-14-aarch64-test-bun, which has expired twice waiting for an agent and is requeued; the red check is queue starvation on that lane, not a test failure. The diff is ready for review. |
1 participant
Fixes #32096
Repro
Cause
close({ timeout })takes seconds, but every close site (the pool close shared by the postgres and mysql adapters,reserved_sql.close,transaction_sql.close) validated the upper bound against the seconds value and then armed the timer in milliseconds:setTimeoutclamps delays above2 ** 31 - 1ms to 1 ms, so any timeout in(2147483.647, 2 ** 31]seconds passed validation and force-closed after about 1 ms, cancelling in-flight queries instead of waiting.Fix
validateCloseTimeoutininternal/sql/shared.ts, bounded at(2 ** 31 - 1) / 1000seconds, replaces the inline checks. The error message now states the real bound:must be a non-negative number no greater than 2147483.647 seconds. After the rebase onto sql: consolidate JS pool/connection plumbing into BasePooledConnection in shared.ts #32145 (which consolidated the postgres and mysql adapters' pool plumbing into shared.ts), that means the consolidated poolclose()inshared.tsplusreserved_sql.closeandtransaction_sql.closeinsql.ts.reserved_sql.closeandtransaction_sql.close, validation ran afteracceptQuerieswas already cleared, so a rejected close left the handle refusing all further queries. Validation now runs before the state mutation; the pool close already had the right order.While verifying the reserved path I hit an unrelated pre-existing hang (
reserved.close()thensql.close()never resolves, with no timeout option at all); filed as #32099, not touched here.Verification
test/js/sql/sql-close-timeout-overflow.test.ts:close()reject2 ** 31,2147483.648, andInfinitywithERR_INVALID_ARG_VALUE, accept2147483.647(the exact max),60, and0.5, and still reject-1and non-numeric input. Pool connections are lazy, so these run without a server.tx.close({ timeout: 2 ** 31 })rejects and the transaction still runs queries and commits.The server-free tests fail on the unfixed build (6 fail: the close resolves instead of rejecting, and the sqlite transaction gets rolled back with
ERR_SQLITE_CONNECTION_CLOSED) and pass with the fix (18 pass). Re-verified after rebasing onto #32145.The new test file is separate from
sql.test.tsbecause that file is wrapped inisDockerEnabled()and skips entirely without docker; these tests need no server.