Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/sql/shared/ConnectionFlags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ bitflags! {
const HAS_BACKPRESSURE = 1 << 4;
/// `ref()` was called; `on_data` must not unref the idle connection.
const KEEP_ALIVE_REQUESTED = 1 << 5;
/// maxLifetime expired mid-query; retire at the next drain boundary (#30646).
const LIFETIME_EXCEEDED = 1 << 6;
}
}

Expand Down
24 changes: 24 additions & 0 deletions src/sql_jsc/mysql/JSMySQLConnection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,19 @@ impl JSMySQLConnection {
if self.connection.get().status == my_sql_connection::Status::Failed {
return;
}

// Don't kill an in-flight query (#30646): retire at the next drain boundary.
if self.connection.get().status == my_sql_connection::Status::Connected
&& !self.connection.get().is_idle()
{
self.connection_mut().set_lifetime_exceeded();
return;
}
Comment thread
claude[bot] marked this conversation as resolved.

self.fail_lifetime_timeout();
}

fn fail_lifetime_timeout(&self) {
use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout};
self.fail_fmt(
AnyMySQLErrorT::LifetimeTimeout,
Expand All @@ -329,6 +342,17 @@ impl JSMySQLConnection {
);
}

/// Fails the connection if maxLifetime expired mid-query; returns true when it did.
pub(crate) fn retire_if_lifetime_exceeded(&self) -> bool {
if self.connection.get().status != my_sql_connection::Status::Connected
|| !self.connection.get().is_lifetime_exceeded()
{
return false;
}
self.fail_lifetime_timeout();
true
}

fn setup_max_lifetime_timer_if_necessary(&self) {
if self.max_lifetime_interval_ms == 0 {
return;
Expand Down
10 changes: 10 additions & 0 deletions src/sql_jsc/mysql/MySQLConnection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,16 @@ impl MySQLConnection {
self.queue.current().is_none() && self.write_buffer.len() == 0
}

#[inline]
pub(crate) fn set_lifetime_exceeded(&mut self) {
self.flags.insert(ConnectionFlags::LIFETIME_EXCEEDED);
}

#[inline]
pub(crate) fn is_lifetime_exceeded(&self) -> bool {
self.flags.contains(ConnectionFlags::LIFETIME_EXCEEDED)
}

#[inline]
pub(crate) fn enqueue_request(&mut self, request: *mut JSMySQLQuery) {
self.queue.add(request);
Expand Down
11 changes: 11 additions & 0 deletions src/sql_jsc/mysql/MySQLRequestQueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,17 @@ impl MySQLRequestQueue {
// momentary `Deref` lifetime. All queue mutation below goes through
// `Cell`/`JsCell` interior mutability — `&Self` is sufficient.
let queue_ref: ParentRef<Self> = ParentRef::new(&conn_ref.connection.get().queue);

// maxLifetime expired mid-query (#30646): retire before dispatching more
// work, once the head finished (a prepare in flight bumps neither counter).
Comment thread
robobun marked this conversation as resolved.
if queue_ref.pipelined_requests.get() == 0
&& queue_ref.nonpipelinable_requests.get() == 0
&& queue_ref.current_ref().is_none_or(|r| r.is_completed())
&& conn_ref.retire_if_lifetime_exceeded()
{
return;
}
Comment thread
claude[bot] marked this conversation as resolved.

// reshaped for borrowck — the cleanup that must run at function exit
// became a post-block pass; early returns become
// `break 'advance` so cleanup always runs at function exit.
Expand Down
35 changes: 35 additions & 0 deletions src/sql_jsc/postgres/PostgresSQLConnection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,8 @@ impl PostgresSQLConnection {

fn get_timeout_interval(&self) -> u32 {
match self.status.get() {
// Never arm the idle timer while a query is outstanding (#30646).
Status::Connected if self.has_query_running() => 0,
Comment thread
robobun marked this conversation as resolved.
Status::Connected => self.idle_timeout_interval_ms,
Status::Failed => 0,
_ => self.connection_timeout_ms,
Expand Down Expand Up @@ -538,6 +540,8 @@ impl PostgresSQLConnection {
return;
}

// `Connected` here implies idle: `get_timeout_interval()` returns 0
// for a busy connection, taking the early return above.
Comment thread
robobun marked this conversation as resolved.
use bun_core::fmt::{ConnTimeoutKind::*, fmt_conn_timeout};
let (code, kind, ms, sfx): (&[u8], _, _, _) = match self.status.get() {
Status::Connected => (
Expand Down Expand Up @@ -569,6 +573,18 @@ impl PostgresSQLConnection {
if self.status.get() == Status::Failed {
return;
}

// Don't kill a healthy in-flight query (#30646): retire at the next
// queue-drain boundary instead (the ReadyForQuery arm).
Comment thread
robobun marked this conversation as resolved.
if self.status.get() == Status::Connected && self.has_query_running() {
self.update_flags(|f| f.insert(ConnectionFlags::LIFETIME_EXCEEDED));
return;
}

self.fail_lifetime_timeout();
Comment thread
claude[bot] marked this conversation as resolved.
}

fn fail_lifetime_timeout(&self) {
use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout};
self.fail_fmt(
b"ERR_POSTGRES_LIFETIME_TIMEOUT",
Expand Down Expand Up @@ -2530,6 +2546,25 @@ impl PostgresSQLConnection {
);
}
}

// maxLifetime expired mid-query (#30646): retire only once the head is
// finished; a named statement's Parse+Describe+Sync RFQs first.
Comment thread
robobun marked this conversation as resolved.
if self
.flags
.get()
.contains(ConnectionFlags::LIFETIME_EXCEEDED)
&& self.current().is_none_or(|request| {
matches!(
request.status.get(),
QueryStatus::Success | QueryStatus::Fail
)
})
{
self.fail_lifetime_timeout();
Comment thread
robobun marked this conversation as resolved.
self.update_ref();
return Ok(());
}

self.advance();

self.register_auto_flusher();
Expand Down
4 changes: 3 additions & 1 deletion src/sql_jsc/postgres/PostgresSQLQuery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -864,10 +864,12 @@ impl PostgresSQLQuery {
if did_write {
connection.flush_data_and_reset_timeout();
} else {
connection.reset_connection_timeout();
// For unnamed prepared statements with params, we skip writeQuery+Sync
// in the enqueue path and let advance() handle it atomically.
connection.advance_and_flush();
// After advance(): it can discard the request synchronously, and the
// idle timer must re-arm for a connection that just became idle.
Comment thread
robobun marked this conversation as resolved.
connection.reset_connection_timeout();
}
Ok(JSValue::UNDEFINED)
}
Expand Down
83 changes: 67 additions & 16 deletions test/js/sql/sql-mysql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ if (isDockerEnabled()) {
expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_IDLE_TIMEOUT`);
});

test("Max lifetime works", async () => {
test("Max lifetime closes an idle connection and the pool reconnects", async () => {
const onClosePromise = Promise.withResolvers();
const onclose = mock(err => {
onClosePromise.resolve(err);
Expand All @@ -326,28 +326,79 @@ if (isDockerEnabled()) {
onclose,
max: 1,
});
let error: unknown;

try {
expect<[{ x: number }]>(await sql`select 1 as x`).toEqual([{ x: 1 }]);

while (true) {
for (let i = 0; i < 100; i++) {
await sql`select SLEEP(1)`;
}
}
} catch (e) {
error = e;
}
// Get the server-side connection id before maxLifetime fires.
const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`;

// maxLifetime fires while the connection is idle and retires it with
// the documented error code.
const err = await onClosePromise.promise;
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err).toBeInstanceOf(SQL.MySQLError);
expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`);
expect(onclose).toHaveBeenCalledTimes(1);
expect(onconnect).toHaveBeenCalledTimes(1);

expect(error).toBeInstanceOf(SQL.SQLError);
expect(error).toBeInstanceOf(SQL.MySQLError);
expect((error as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`);
// Pool reconnects on a different connection id.
const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`;
expect(connAfter).not.toBe(connBefore);
});

test("Max lifetime does not kill an in-flight query (#30646)", async () => {
const onClosePromise = Promise.withResolvers();
const onclose = mock(err => {
onClosePromise.resolve(err);
});
const onconnect = mock();
await using sql = new SQL({
...getOptions(),
max_lifetime: 1,
onconnect,
onclose,
max: 1,
});

const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`;

// Query longer than max_lifetime must complete normally.
// Before the fix this rejected with ERR_MYSQL_LIFETIME_TIMEOUT.
const result = await sql`select SLEEP(3) as s, 42 as x`;
expect(result[0].x).toBe(42);

// The connection is then retired at the drain boundary with the
// documented error code, and the pool reconnects.
const err = await onClosePromise.promise;
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err).toBeInstanceOf(SQL.MySQLError);
expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`);
expect(onclose).toHaveBeenCalledTimes(1);

const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`;
expect(connAfter).not.toBe(connBefore);
}, 30_000);

test("Max lifetime does not kill an in-flight parameterized query (#30646)", async () => {
const onClosePromise = Promise.withResolvers();
const onclose = mock(err => {
onClosePromise.resolve(err);
});
await using sql = new SQL({
...getOptions(),
max_lifetime: 1,
onclose,
max: 1,
});

// A parameterized query goes through COM_STMT_PREPARE before
// execute; cover that path end to end.
const result = await sql`select SLEEP(2) as s, ${42} as x`;
expect(result[0].x).toBe(42);

const err = await onClosePromise.promise;
expect(err).toBeInstanceOf(SQL.MySQLError);
expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`);
}, 30_000);

// Last one wins.
test("Handles duplicate string column names", async () => {
const result = await sql`select 1 as x, 2 as x, 3 as x`;
Expand Down
Loading