sql: don't kill in-flight queries when idleTimeout/maxLifetime fires - #30648
sql: don't kill in-flight queries when idleTimeout/maxLifetime fires#30648robobun wants to merge 10 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (8)
WalkthroughMySQL and PostgreSQL connection handlers now reschedule idle and max-lifetime timeout timers when in-flight queries are present, allowing queries to complete before connection closure. Regression tests across both databases verify idle-aware behavior closes idle connections while preserving in-flight query execution. ChangesIdle-aware connection timeout and max-lifetime closure
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:57 PM PT - Aug 14th, 2026
❌ @robobun, your commit d76bf17 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30648That installs a local version of the PR into your bun-30648 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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
🤖 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-timer-drain.test.ts`:
- Around line 104-116: The test schedules a response on every "data" chunk which
can cause duplicate responses when a query is TCP-chunked; update the
socket.on("data") handler (referencing socket.on("data"), gotStartup, HANDSHAKE,
QUERY_RESPONSE, queryDelayMs, timers) to accumulate incoming bytes into a buffer
and only schedule/send a mock reply once a full query boundary is detected (e.g.
parse until the protocol's message delimiter or length-prefixed frame is
complete), deduplicate by clearing the buffer or marking the query as handled,
remove/avoid the flaky setTimeout-based delay (and timers collection) and
instead send the reply immediately when the full request is received and have
the test await the expected condition rather than sleeping.
In `@test/js/sql/sql.test.ts`:
- Around line 802-829: The test "Max lifetime does not kill an in-flight query
(`#30646`)" and the similar block at 831-859 are flaky because they rely on an
unbounded wait for a 3s sleep; wrap the awaited operations with explicit
timeouts instead of using setTimeout so the test fails fast when the condition
isn't met. Concretely, when awaiting the long-running query (sql`select
pg_sleep(3), 42 as x`) and when awaiting onClosePromise.promise, use a helper
timeout pattern (e.g. Promise.race between the target promise and a short
timeout-rejecting promise) so the test will reject with a clear timeout if the
condition isn't met; apply the same pattern to the other test block referenced
(lines 831-859) and keep the unique symbols: onClosePromise, onclose, onconnect,
and the sql query calls to locate where to wrap the awaits.
🪄 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: a9a64679-c967-42d8-bed2-87cb6ca70ea2
📒 Files selected for processing (5)
src/sql_jsc/mysql/JSMySQLConnection.zigsrc/sql_jsc/postgres/PostgresSQLConnection.zigtest/js/sql/sql-mysql.test.tstest/js/sql/sql-timer-drain.test.tstest/js/sql/sql.test.ts
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.test.ts`:
- Around line 744-748: After each awaited long-running query (e.g. the
sql`select pg_sleep(2)` call) add a negative assertion that the connection close
handler has not been invoked yet (assert the mocked onclose callback has not
been called) before awaiting onClosePromise.promise; specifically, insert
expect(onclose).not.toHaveBeenCalled() (or the actual mock name used for the
close callback) immediately after the pg_sleep result to ensure the timer didn't
trigger onclose while the query was still running; repeat the same insertion for
the other similar blocks referenced (around the other pg_sleep assertions).
🪄 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: 3a547255-69f3-4810-ac21-ee417e93f864
📒 Files selected for processing (3)
test/js/sql/sql-mysql.test.tstest/js/sql/sql-timer-drain.test.tstest/js/sql/sql.test.ts
|
Local verification:
CI red lanes are on shards my diff doesn't touch:
Used my one ci-retrigger (79c0a6d). Diff itself is green. Needs a maintainer to merge. |
|
Rebased onto main (post-Rust-rewrite) and ported to:
Gate verified with the mock-server test (no Docker needed): without the |
79c0a6d to
bbbd7fb
Compare
|
CI failure on build 54337 is pre-existing, not caused by this PR. The failing test is Reproduced on main directly: Main's CI jobs are build-only ( This PR's SQL tests pass locally (bun bd): 3/3 in Already used my one |
bbbd7fb to
e73248a
Compare
|
Rebased onto current main and force-pushed ( The conflicts were non-trivial because main completed the Zig→Rust transition and deleted the
Collapsed the old 7-commit history (which included the now-obsolete Verified locally: fail-before |
|
CI build 65506: 47/48 jobs green. The one red lane ( The runner couldn't download the build artifact, so the test suite never started — unrelated to this diff. Every lane that actually ran tests passed. A re-run of that single job (or a merge) should clear it; I've already spent my one ci-retrigger on this PR so I'm not pushing another. |
|
Same darwin-lane infra flake recurred on build 65524 (45/46 jobs green). The suite never ran — it's a BuildKite artifact-transfer timeout to the darwin test agent, not a test failure, and independent of this diff (which only touches |
103bcad to
adad059
Compare
|
Rebased onto current main and force-pushed ( The conflict was in The Verified locally: |
|
CI build 70304: 48/49 jobs green. The one red lane ( These tests connect to a local mock TCP server ( This PR's own regression test ( |
There was a problem hiding this comment.
All prior feedback is addressed and no new issues found, but this changes native connection-lifecycle/refcount paths and user-facing timeout semantics (existing tests rewritten, ERR_*_LIFETIME_TIMEOUT no longer emitted, 1s-poll retirement trade-off deferred as a follow-up), so it warrants a maintainer sign-off.
Extended reasoning...
Overview
This PR changes on_max_lifetime_timeout / get_timeout_interval in both src/sql_jsc/postgres/PostgresSQLConnection.rs and src/sql_jsc/mysql/JSMySQLConnection.rs so that idle/maxLifetime timers no longer kill in-flight queries — they reschedule (1s poll) until the connection is idle, then close gracefully via disconnect()/close(). It rewrites the existing "Max lifetime works" / "Idle timeout works at start" tests in sql.test.ts and sql-mysql.test.ts (which previously asserted the old kill-on-timer behavior) and adds a Docker-free mock-server regression test (sql-timer-drain.test.ts).
Security risks
None identified. No auth, crypto, or untrusted-input parsing is touched; the change is client-side timer scheduling and connection teardown ordering.
Level of scrutiny
This deserves maintainer review rather than bot approval:
- Native lifecycle / memory safety: the Postgres path now calls
disconnect()from an unrooted timer callback and brackets it withself.ref_()…Self::deref(self.as_ctx_ptr())to survive synchronouson_close→ JSonclosere-entry. The reasoning is sound and mirrorsfail_with_js_value, but refcount discipline around GC-eligible wrappers is exactly the class of change CLAUDE.md flags as most-blocked. - User-facing behavior change:
ERR_POSTGRES_LIFETIME_TIMEOUT/ERR_MYSQL_LIFETIME_TIMEOUTare no longer emitted (graceful close instead), and existing tests that asserted those codes were deleted/rewritten. That's the right outcome for the bug, but it's an observable API change a maintainer should ratify. - Design trade-off explicitly deferred: the author acknowledged that the 1s-poll approach can, in theory, delay retirement of a continuously-busy connection indefinitely, and chose it over a deterministic retire-at-next-RFQ flag (which would need JS-pool coordination). That's a reasonable call matching #28591's shape, but it's a design decision worth a human nod.
Other factors
The PR has been through several review rounds; every inline comment (mine and CodeRabbit's) is resolved in the current head (e20031a), including the has_query_running() helper reuse, the ref-guard around disconnect(), await using pool disposal, and the wire-frames.ts migration. The bug-hunting pass found nothing new. Test coverage is solid (fail-before/pass-after verified against a mock server plus real-DB integration tests). CI has been green on all lanes that actually ran; remaining red lanes were unrelated infra/artifact-download flakes.
|
I independently arrived at a fix for the same maxLifetime in-flight kill and pushed it as Difference in approach: instead of re-arming the lifetime timer on a 1s poll, that branch sets a It also keeps the retirement going through Verified against Postgres 17 and MariaDB: |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Requesting changes. The idle half (the has_query_running gate) is the right fix. The max_lifetime half should be the same busy gate in front of the existing fail_fmt, not a switch to disconnect()/close().
- onclose now gets ERR_CONNECTION_CLOSED instead of ERR_LIFETIME_TIMEOUT in both drivers, the body's table says this row is unchanged, and the rewritten tests dropped the one assertion that would have caught it
- the 1s re-poll never retires a connection that has steady traffic
- the new gate disarms the idle timer on enqueue and nothing re-arms it when the request fails synchronously
- sql-timer-drain.test.ts should go, the container tests already cover it
Also drop #25405 from the get_timeout_interval comment and the same-root-cause line in the body: MySQL's idle gate has been there since 1.2.22 and this PR does not touch the MySQL idle path, so whatever that issue is, this does not address it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1e668d4 to
7d6b4bf
Compare
|
@alii round 2 addressed, rebased onto current main (c8be23f):
Verified locally against Postgres and MariaDB: the lifetime/idle suites in |
|
CI on 5fcae38 is red only on lanes unrelated to this diff (a bake deinit segfault on Windows 2019, asan leak-test timeouts, and tests the harness itself marks flaky/passed-alone); both sql suites pass. Diff is ready for review. |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Both remaining points are addressed on 5fcae38: the MySQL re-poll is gone and the connection retires from MySQLRequestQueue::advance() once both counters are 0 and the head is finished, and the postgres ReadyForQuery retire is gated on the head being Success/Fail so the Parse+Describe+Sync round trip of a named statement falls through to advance(); the body now carries the pipelined-behind-finished-head note. Read the follow-up commits and the surrounding queue and event-loop code (retire runs inside the on_data enter scope, so the microtask that hands the connection its next query runs after it is already closed), tests not run here; nothing left from this side, looks ready to merge.
|
Thanks for the careful reads. Correct on the ordering: the retire runs inside the on_data ref scope, so onclose and the queue rejection happen before the JS pool's release microtask can hand this connection another query; the pool sees a closed connection and reconnects. Nothing further from my side. |
|
@robobun rebase fix conflicts |
Postgres and MySQL rejected any in-flight query when idleTimeout or
maxLifetime fired, because onConnectionTimeout/onMaxLifetimeTimeout failed
the connection unconditionally. The query itself was healthy; only a
client-side timer raced it.
Postgres (src/sql_jsc/postgres/PostgresSQLConnection.rs):
- get_timeout_interval returns 0 when requests are queued or
IS_READY_FOR_QUERY is clear, so the idle timer never arms while a
query is outstanding (mirrors MySQL's is_idle gate).
- on_connection_timeout reschedules on the .connected branch if a
request slipped in between arming and firing.
- on_max_lifetime_timeout disconnect()s when idle, otherwise reschedules
for 1s and retries until the connection returns to idle.
MySQL (src/sql_jsc/mysql/JSMySQLConnection.rs):
- on_max_lifetime_timeout close()s when idle, otherwise reschedules 1s.
Tests: rewrite the idle/maxLifetime tests to the drained behavior (query
completes, then the connection retires and the pool reconnects), add a
Docker-free mock-server regression in sql-timer-drain.test.ts.
Fixes #30646. Related: #25405 (MySQL idle).
…_query_running() - on_max_lifetime_timeout: hold an intrinsic ref across disconnect(), whose socket.close() can synchronously run the JS onclose callback and make the wrapper GC-eligible before ref_and_close's clean_up_requests touches self. Mirrors fail_with_js_value's ref/deref discipline (the pre-rewrite path took this ref; the MySQL side already guards via ref_guard()). - Replace the inlined busy predicate with the existing has_query_running() helper in get_timeout_interval and on_max_lifetime_timeout. - Drop the dead idle guard in on_connection_timeout: get_timeout_interval() already returns 0 for a busy .connected connection, so the early return above covers it.
…n boundary Address review feedback: - Postgres: when max_lifetime fires with a query in flight, set a LIFETIME_EXCEEDED flag instead of polling; the ReadyForQuery arm acts on it before advance() dispatches more work, so max_lifetime stays a hard bound under steady traffic and onclose still reports ERR_POSTGRES_LIFETIME_TIMEOUT (no disconnect()/CONNECTION_CLOSED, no TLS close_notify window, no ref_/deref bracket needed). - MySQL: reschedule 1s when busy, otherwise fall through to the original fail_fmt(LifetimeTimeout), so ERR_MYSQL_LIFETIME_TIMEOUT is preserved. - Postgres do_run: move reset_connection_timeout after advance_and_flush so a synchronously-discarded request can't leave an idle connection with no timer armed. - Tests: drop the mock-server sql-timer-drain.test.ts (container tests cover the scenarios); restore ERR_*_LIFETIME_TIMEOUT assertions in the idle and in-flight container tests. - Drop the stale #25405 reference.
…ndary; gate Postgres retirement on the head request finishing
… request is mid-prepare
5fcae38 to
8730e98
Compare
|
Rebased onto main at 8730e98. One conflict: main added KEEP_ALIVE_REQUESTED at bit 5 in ConnectionFlags, so LIFETIME_EXCEEDED moved to bit 6. Built and re-ran the lifetime/idle suites for both drivers locally, all green; PR is mergeable again. |
The container suites are skipped entirely where docker and the test services are unavailable, leaving #30646 unprovable there. The wire mock delays the query response past the client-side timer; on main both tests reject with ERR_POSTGRES_IDLE_TIMEOUT / ERR_POSTGRES_LIFETIME_TIMEOUT.
|
Added serverless twins of the two in-flight timer tests at the bottom of sql.test.ts (d76bf17). The container suites are skipped entirely in environments without docker, which left the regression unprovable there; the mock only delays the query response past the client timer and carries a header pointing at the real-server twins. Verified: with src/ reverted to main both reject with the timeout codes at ~1.5s, with the fix both pass. |
2 participants
Fixes #30646
Problem
PostgresError: Max lifetime timeout reached after 30m(ERR_POSTGRES_LIFETIME_TIMEOUT) andPostgresError: Idle timeout reached after 2m(ERR_POSTGRES_IDLE_TIMEOUT), same shape for MySQL.on_connection_timeout/on_max_lifetime_timeoutfailed the connection unconditionally; for Postgres the idle timer was even armed while a query was outstanding. Reporter saw 137 occurrences/24h in production.Fix
Postgres (
src/sql_jsc/postgres/PostgresSQLConnection.rs):get_timeout_intervalreturns 0 whilehas_query_running(), so the idle timer never arms with a query outstanding (mirrors MySQL's existingis_idlegate; that gate already existed on the MySQL side, so only Postgres needed this).on_max_lifetime_timeout: if a query is in flight, set aLIFETIME_EXCEEDEDflag instead of failing; theReadyForQueryarm acts on it beforeadvance()dispatches more work. If idle, fail immediately as before.ReadyForQueryretirement only happens once the head request is finished (Success/Fail). A named statement with parameters sends Parse+Describe+Sync first and gets its own ReadyForQuery before Bind+Execute; retiring on that one would reject the head query before it ran, so that ReadyForQuery falls through toadvance()and the next one retires.fail_fmt(ERR_POSTGRES_LIFETIME_TIMEOUT).onclosestill reports the documented code, andmax_lifetimestays a hard bound under steady traffic (retires at the first query-completion after expiry; verified 799 back-to-back queries retire at ~2.1s withmax_lifetime: 2).do_runnow callsreset_connection_timeoutafteradvance_and_flush, so a request thatadvance()discards synchronously can't leave an idle connection with no timer armed.MySQL (
src/sql_jsc/mysql/JSMySQLConnection.rs,MySQLRequestQueue.rs):on_max_lifetime_timeoutsetsLIFETIME_EXCEEDEDwhile a query is in flight instead of failing. Every completion path ends inMySQLRequestQueue::advance(), which retires the connection via the originalfail_fmt(ERR_MYSQL_LIFETIME_TIMEOUT)oncepipelined_requestsandnonpipelinable_requestsare both 0, before dispatching the next request. (Replaces the earlier 1s re-poll, which never retired under steady traffic.)Scope notes:
has_query_running()is false between the statements ofsql.begin()/ on areserve()d connection that is momentarily idle, so a lifetime expiry in that window still retires the connection mid-transaction — same as current main, where the timer killed it in that window too.Verification (local Postgres/MariaDB, debug build)
max_lifetime=1,pg_sleep(3)in flight42; then onclose fires withERR_POSTGRES_LIFETIME_TIMEOUT; pool reconnects on new pidmax_lifetime=1, idleERR_POSTGRES_LIFETIME_TIMEOUTidle_timeout=1,pg_sleep(3)in flight42; then oncloseERR_POSTGRES_IDLE_TIMEOUTmax_lifetime=2(Postgres)max_lifetime=2(MySQL)ERR_MYSQL_LIFETIME_TIMEOUTmax_lifetime=1,SLEEP(3)in flightERR_MYSQL_LIFETIME_TIMEOUT42; then oncloseERR_MYSQL_LIFETIME_TIMEOUTTests
test/js/sql/sql.test.ts/sql-mysql.test.ts(container): the oldMax lifetime works/Idle timeout works at starttests asserted the in-flight kill; rewritten to assert the query completes, then onclose fires with the documentedERR_*_LIFETIME_TIMEOUT/ERR_*_IDLE_TIMEOUTcode and the pool reconnects (pid /CONNECTION_ID()changes). A parameterized (named statement) in-flight variant covers the prepared-statement path end to end; the head-finished guard itself protects a few-ms prepare window a real server cannot stretch on demand, so it is verified by reading. The in-flight tests fail on main (the awaited query rejects).Background
IS_READY_FOR_QUERYtracks the Postgres ReadyForQuery protocol state;requestsis the native FIFO of dispatched/queued queries.has_query_running()= either is active.fail_fmtmarks the connection failed, rejects queued requests, runs the JSonclose, and closes the socket; the JS pool then reconnects on demand.[review] gate passed · iteration 15 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 15
evidence per changed file
root cause · written by the author bot
The root cause was that the idleTimeout and maxLifetime timers fired unconditionally, immediately failing the connection and rejecting any queries that happened to be in flight, which surfaced as spurious ERR_POSTGRES_IDLE_TIMEOUT and ERR_POSTGRES_LIFETIME_TIMEOUT errors under normal traffic. The fix makes both timers idle-aware in the Postgres and MySQL adapters: when a timer fires on a connection with in-flight queries, it reschedules itself and retries until the connection returns to idle, and the Postgres idle timer is additionally disarmed while requests are pending. As a result, conne…