Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 20 additions & 9 deletions src/sql_jsc/postgres/PostgresSQLQuery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,15 +479,6 @@
};
let connection: &PostgresSQLConnection = &connection;

// `KeepAlive::ref_` takes an `EventLoopCtx` (manual vtable in `bun_io`), not a
// `*mut VirtualMachine`. `global_object.bun_vm()` and `get_vm_ctx(.Js)` both
// resolve to the same singleton JS VM, so route through the global hook —
// identical to `PostgresSQLConnection::vm_ctx`.
connection.poll_ref.with_mut(|r| {
r.ref_(bun_io::posix_event_loop::get_vm_ctx(
bun_io::AllocatorType::Js,
))
});
let query = arguments[1];

if !query.is_object() {
Expand Down Expand Up @@ -561,6 +552,16 @@
return Err(global_object.throw_out_of_memory());
}

// Request is enqueued: keep the event loop alive until the server
// responds. KeepAlive is a flag (not a count), so taking this any
// earlier would leave it stuck Active on the synchronous-error
// returns above.
connection.poll_ref.with_mut(|r| {
r.ref_(bun_io::posix_event_loop::get_vm_ctx(
bun_io::AllocatorType::Js,
))
});

this.this_value.with_mut(|r| r.upgrade(global_object));
js::target_set_cached(this_value, global_object, query);
if this.status.get() == Status::Running {
Expand Down Expand Up @@ -853,9 +854,19 @@
.requests
.with_mut(|q| q.write_item(this_ptr))
.is_err()
{
return Err(global_object.throw_out_of_memory());
}

Check notice on line 859 in src/sql_jsc/postgres/PostgresSQLQuery.rs

View check run for this annotation

Claude / Claude Code Review

Pre-existing: three do_run error paths still leak the speculative this.ref_()

Pre-existing: three error paths in the prepared-statement branch still leak the speculative `this.ref_()` taken near the top of `do_run` — the `statements.get_or_put` Err arm (line 632), the `writer.write(&protocol::SYNC)` Err arm (line 783), and this final `requests.write_item` OOM (which also leaks the just-allocated `this.statement`). Every sibling error path explicitly calls `Self::deref(this_ptr)`; the PR description's claim that "every synchronous error return … undoes the speculative `thi
Comment thread
robobun marked this conversation as resolved.

// Request is enqueued: keep the event loop alive until the server
// responds. See the matching call in the simple-query branch above
// for why this must come after every fallible step.
connection.poll_ref.with_mut(|r| {
r.ref_(bun_io::posix_event_loop::get_vm_ctx(
bun_io::AllocatorType::Js,
))
});

this.this_value.with_mut(|r| r.upgrade(global_object));

js::target_set_cached(this_value, global_object, query);
Expand Down
58 changes: 58 additions & 0 deletions test/js/sql/sql-postgres-run-error-pollref-fixture.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions test/js/sql/sql-postgres-run-error-pollref.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// PostgresSQLQuery.do_run refs the connection's poll_ref KeepAlive. KeepAlive
// is a two-state flag, not a counter, so when this query is the only in-flight
// work the call flips Inactive -> Active. When do_run then returns early with
// a synchronous error (bad binding, signature-generation failure, OOM during
// enqueue, ...) the poll_ref must not be left Active: nothing else on the
// connection will touch it until the next server message, so the event loop
// stays pinned and the process never exits.
//
// The fixture connects to a mock server, lets the connection go idle, then
// issues a query whose binding is rejected synchronously before anything is
// written. It must print the rejection and exit on its own.

import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
import path from "node:path";

test("postgres: synchronous do_run failure does not pin the event loop", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "sql-postgres-run-error-pollref-fixture.ts")],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect({ stdout, stderr }).toEqual({ stdout: "rejected:ERR_INVALID_ARG_TYPE\n", stderr: "" });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
// exited on its own, not killed by the runner's timeout
expect(proc.signalCode).toBeNull();
expect(exitCode).toBe(0);
});
Loading