Skip to content

test(sql): bound spawned fixture lifetime so a test timeout cannot leave orphans #32207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alii merged 1 commit into from
Jun 12, 2026
Merged

test(sql): bound spawned fixture lifetime so a test timeout cannot leave orphans #32207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-bind-blob-borrow.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ async function runFixture(url: string, caPath = "") {
env: { ...bunEnv, MYSQL_URL: url, CA_PATH: caPath },
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-bind-oob.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ async function runFixture(url: string, caPath = "") {
env: { ...bunEnv, MYSQL_URL: url, CA_PATH: caPath },
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-clean-reentry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ test.skipIf(!isDebug && !isASAN)(
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
Expand Down
2 changes: 2 additions & 0 deletions test/js/sql/sql-mysql-column-name-digits.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
env: { ...bunEnv, MYSQL_URL: url },
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
Expand Down Expand Up @@ -87,6 +88,7 @@
stdout: "ignore",
stderr: "ignore",
stdin: "ignore",
timeout: 60_000,

Check failure on line 91 in test/js/sql/sql-mysql-column-name-digits.test.ts

View check run for this annotation

Claude / Claude Code Review

60s timeout on mysqld_safe daemon may kill the database server mid-test 

Unlike the other 8 spawns in this PR (short-lived bun fixture clients), `mysqld_safe` is the long-running MariaDB server daemon — it is supposed to stay alive for the duration of the test, and `.unref()` does not cancel the spawn timeout timer. With `waitForSocket` budgeted up to 30s for cold start, plus `provisionTcpUser` and a debug/ASAN fixture run, the 60s timer can fire mid-test and SIGTERM the database out from under the fixture, causing a spurious failure. The PR's rationale (fixture clie

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Unlike the other 8 spawns in this PR (short-lived bun fixture clients), mysqld_safe is the long-running MariaDB server daemon — it is supposed to stay alive for the duration of the test, and .unref() does not cancel the spawn timeout timer. With waitForSocket budgeted up to 30s for cold start, plus provisionTcpUser and a debug/ASAN fixture run, the 60s timer can fire mid-test and SIGTERM the database out from under the fixture, causing a spurious failure. The PR's rationale (fixture clients hanging on pool drain) doesn't apply to the database daemon itself; suggest dropping this hunk or using a much larger bound (e.g. 300_000+).

Extended reasoning...

What the bug is

This PR adds timeout: 60_000 to nine Bun.spawn calls. Eight of them spawn short-lived bun fixture subprocesses that connect to a database, run a query, and exit — exactly the case the PR description targets. But the ninth, at test/js/sql/sql-mysql-column-name-digits.test.ts:91, spawns mysqld_safe — the MariaDB server daemon itself. That process is intended to outlive the test body: it is .unref()'d, its stdio is ignored, and ensureServerStarted() early-returns if the socket already exists from a prior run. Putting a 60s hard kill on it means the database server can be terminated while the test is still using it.

Why .unref() doesn't help

I verified in src/runtime/api/bun/subprocess.zig that jsUnref() only calls process.disableKeepingEventLoopAlive() and unrefs stdin/stdout/stderr — it does not touch event_loop_timer. Meanwhile computeHasPendingActivity() returns true while !process.hasExited(), which keeps the JS wrapper Strong-referenced, so GC will not collect the Subprocess and cancel the timer via finalize() either. Therefore, 60 seconds after ensureServerStarted() runs, timeoutCallback() unconditionally calls tryKill(killSignal) on the still-running mysqld_safe process — regardless of .unref().

Step-by-step trigger

  1. Test runs in the non-Docker branch (sandboxed dev/CI-gate container with native MariaDB, isDockerEnabled() === false).
  2. beforeAll → ensureServerStarted() spawns mysqld_safe at T=0 with timeout: 60_000 and calls .unref().
  3. waitForSocket(30_000) polls every 250ms. On a cold container start, MariaDB initialization can take a substantial fraction of that 30s budget — say T≈25s when the socket appears.
  4. provisionTcpUser() opens a root socket connection and runs 6 sequential SQL statements — a few more seconds.
  5. The test body calls runFixture(url), which spawns a debug/ASAN bun subprocess (the file's other hunk), connects over TCP, and runs the query. Debug+ASAN bun startup alone is routinely 10–20s.
  6. At T=60s the timeout timer fires and SIGTERMs mysqld_safe. MariaDB shuts down.
  7. The fixture's connection drops mid-query; it never prints CONNECTED; the test throws could not connect to mysql://bun_sql_test@127.0.0.1:3306/... — a spurious failure unrelated to the column-name-digits regression under test.

Why the PR rationale doesn't apply here

The PR description says: "the sql fixture subprocesses connect to a database and await the pool draining; under a regression that hangs the pool, the fixture never exits". mysqld_safe is not a bun SQL fixture, has no JS pool to drain, and is not subject to a #32145-style hang. The timeout also doesn't achieve the orphan-prevention goal for this spawn: because the process is .unref()'d and never awaited, when the test-runner process exits the timer dies with it and mysqld_safe is orphaned regardless. So this hunk adds a flake risk without providing the intended safety.

Fix

Either drop this one hunk (the mysqld_safe spawn), or use a bound comfortably larger than the worst-case test duration — e.g. timeout: 300_000 or more — so the daemon is reaped only when the whole test/js/sql/ run is clearly stuck, not while a single slow cold-start test is still in flight.

}).unref();
return waitForSocket(30_000);
}
Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-columns-realloc-oom.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ test("MySQL: OOM reallocating statement.columns does not leave a dangling slice"
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-datetime-roundtrip.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe.each(TIMEZONES)("text protocol via mock server, TZ=%s", TZ => {
env: { ...bunEnv, TZ },
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-mysql-query-string-leak.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ test("MySQL: query string is not leaked across query lifecycle", async () => {
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
timeout: 60_000,
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
Expand Down
1 change: 1 addition & 0 deletions test/js/sql/sql-onconnect-onclose-throw.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ async function runFixture(code: string, env: Record<string, string> = {}) {
env: { ...bunEnv, ...env },
cwd: String(dir),
stderr: "pipe",
timeout: 60_000,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
Expand Down
Loading