Conversation
Measure the RSS delta in a child process through expectRssDeltaBelow, which runs with the ASAN quarantine off. Amplify the per-query leak with a 500-column table and 64-byte column names, so 300 measured queries replace 5,000. Assert that SELECT * returns the full row in object and values form, and check every result in the fixture. Drop the file-level isDockerEnabled() gate. describeWithContainer skips on its own, and the gate blocked the BUN_TEST_SERVICE_mysql_plain override.
|
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)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe PR adds a 500-column MySQL prepared-statement result regression test. It also moves issue 28632 memory-growth measurement into an asynchronous fixture and updates the regression test to use that fixture. ChangesMySQL regression coverage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed problem, fix, background, verification results, performance data, and test-impact information. It does not use the template headings exactly, but it contains the required information and is substantially complete. Comment |
|
Status: ready for review. Test-only change, green on the lanes that run it. The red CI lanes so far were unrelated files (reported to main-break triage). Verification on this branch, against a local MariaDB 11.8 through
CI (build 106902, the final push): Self-review note: the ASAN lane's LeakSanitizer also catches the original leak class in any MySQL test, so the RSS guard is kept only for growth that stays reachable. The PR body has the details and the alternative (drop the regression file, keep the sql-mysql case) if you prefer it. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the afterAll guard on url addresses the earlier note, and the rest is a test-only refactor.
What was reviewed:
expectRssDeltaBelowcall shape matches the harness signature (args-after-bunExe,{release, debug}bounds); fixture prints{"deltaMiB"}as its last line as the helper expects.- Fixture checks every result and throws on shape mismatch, so a broken query path fails rather than measuring an idle process.
await usingon everySQLhandle; setup/teardown ordering withafterAllnow guarded for the beforeAll-throws case..fixture.tsnaming matches the dominant pattern already intest/regression/issue/(11793, 14135, 20092, etc.).
Extended reasoning...
Overview
This PR refactors the existing test/regression/issue/28632.test.ts MySQL memory-leak regression test and adds a spawned child fixture 28632.fixture.ts. No production code is touched. The refactor moves RSS measurement into a child process via the existing expectRssDeltaBelow harness helper (which disables ASAN quarantine), amplifies the leak signal with 500 64-byte column names, adds a separate correctness test asserting the wide-row shape in both object and .values() modes, and moves table setup/teardown into beforeAll/afterAll with await using connections. The one issue raised in the previous review — afterAll constructing new SQL({ url: undefined }) when beforeAll failed early — was fixed in the follow-up commit with an if (!url) return; guard.
Security risks
None. This is test code only, running against a local describeWithContainer MySQL instance. The generated column names are locally constructed from a fixed template and passed through sql.unsafe for DDL only; the SELECTs use parameterized tagged templates. No credentials, no external network, no production surface.
Level of scrutiny
Low-to-moderate. Test-only changes to an existing regression test, using established harness helpers (describeWithContainer, expectRssDeltaBelow, bunExe/bunEnv via the helper). The leak-test conventions from REVIEW.md are followed: bounds branch on debug vs release via the harness helper, measurement happens after a warmup window, the bound (5/8 MiB) sits well below the stated unfixed leak (9.6–17 MiB), and results are asserted so the test cannot pass on a no-op query path. No sleep/setTimeout waits remain (the old version's Bun.sleep(50) calls are gone). Resources use await using registered before assertions.
Other factors
The .fixture.ts (dot) naming matches the majority of sibling fixtures in test/regression/issue/, so the -fixture.ts guidance in test/CLAUDE.md is not enforced in practice here. No CODEOWNERS entry covers these paths. The bug-hunt exit reason was dry_streak, and no third-party CHANGES_REQUESTED reviews are outstanding in the timeline. Given the test-only scope, addressed prior feedback, and adherence to the leak-test conventions, approving is appropriate.
|
Updated 8:54 AM PT - Aug 27th, 2026
❌ @robobun, your commit 8439f35 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40633That installs a local version of the PR into your bun-40633 --bun |
…ow check to sql-mysql.test.ts The fixture creates a CREATE TEMPORARY TABLE on its pinned connection, so the test needs no shared table, no beforeAll/afterAll, and no url guard. The wide-result assertion moves to sql-mysql.test.ts, where it runs on all three MySQL images. It covers a 500-column prepared-statement row in object and .values() form, a path the existing >64-column tests do not reach because they go through sql.unsafe() and the text protocol.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the latest push addresses the earlier afterAll/undefined-url concern by dropping the hook entirely in favor of a TEMPORARY TABLE on a max: 1 pool, which is cleaner than the guard.
Checked the new sql-mysql.test.ts wide-result test: TINYINT values top out at 71 (within range), table name is uniqued via randomUUIDv7, and both object and .values() shapes are asserted. The fixture's expectRssDeltaBelow path uses bunExe()/bunEnv and branches the bound on isASAN || isDebug, and the .fixture.ts naming matches the existing convention in test/regression/issue/.
Extended reasoning...
Overview
Test-only change across three files. test/regression/issue/28632.test.ts is rewritten from an in-process 5,500-query RSS measurement to a subprocess-based leak test that delegates to the harness expectRssDeltaBelow helper with { release: 5, debug: 8 } bounds. The new 28632.fixture.ts child creates a 500-column TEMPORARY table on a pinned connection, runs 100 warm-up + 300 measured queries in batches of 50 with Bun.gc(true) between batches, validates every result shape, and prints { deltaMiB }. test/js/sql/sql-mysql.test.ts gains a companion correctness test asserting a 500-column parameterized SELECT returns every column in both object and .values() form.
Security risks
None. Test-only; no production code touched. The fixture connects to the container-provided MySQL URL passed as argv, uses parameterized queries or sql.unsafe() on locally-constructed DDL strings, and never touches external networks.
Level of scrutiny
Low-to-medium. This is a test-quality improvement with detailed measurement data in the PR description (fixed build 0-3 MiB vs. bounds of 5/8 MiB; leak-restored 15-17 MiB). The change follows the leak-test guidance in REVIEW.md exactly: RSS bounds branch on isASAN/isDebug (via the harness helper), the bound sits well below the unfixed leak, measurement happens after a warm-up window, and ASAN quarantine is disabled to remove freed-block noise. The harness helper (test/harness.ts:346-366) drains stdout/stderr concurrently with proc.exited and asserts stderr/output before exit code, matching the subprocess-test convention.
Other factors
My earlier inline comment about afterAll constructing new SQL({ url: undefined }) when beforeAll fails is now moot: the latest commit removed beforeAll/afterAll entirely — the fixture uses CREATE TEMPORARY TABLE on a max: 1 pool with await using, so cleanup is automatic when the connection closes, and the parent test has no cleanup to guard. The .fixture.ts naming matches 14 existing files in test/regression/issue/. The new sql-mysql.test.ts test lands in the right module file per test-organization rules, uses a uniqued temporary table name, and the inserted TINYINT values (0-71, from 72 filled columns at stride 7 over 500) stay within range. No outstanding CHANGES_REQUESTED from other reviewers.
This branch has not been deployed
1 participant
Problem
test/regression/issue/28632.test.tsruns 5,500 sequential 50-column queries in the test process. A debug build needs 11.1 s, past the 5 s default timeout, sobun bd testfails on it.Fix
expectRssDeltaBelow, which turns the ASAN quarantine off. The fixture owns aCREATE TEMPORARY TABLEon its pinned connection and checks every result.test/js/sql/sql-mysql.test.ts: a 500-column row with a mixed NULL pattern, asserted in object and.values()form on all three MySQL images. The existing wide-row tests there usesql.unsafe(), the text protocol, so the binary row path had no coverage past 64 columns.bun bd test test/regression/issue/28632.test.ts: 3.2 s (was 11.1 s), file 5.4 s (was 13.7 s). Release: file 0.35 s (was 0.78 s). With the leak restored, the test fails at 16.0 MiB.Background
COM_STMT_EXECUTEresponse carries one column definition per result column. The adapter re-decodes them into the cachedMySQLStatementand keeps an owned copy of each name asname_or_index. Issuebun:sqlMySQL adapter causes RSS memory leak on Linux (RSS grows unbounded until OOM crash) #28632 was that copy leaking on each re-decode.expectRssDeltaBelow(test/harness.ts) spawnsbunExe()on a fixture that prints{"deltaMiB"}, withASAN_OPTIONS=quarantine_size_mb=0. The quarantine delays reuse of freed memory, so freed bytes count as RSS growth.detect_leaks=1, and LeakSanitizer reports the original leak after a few queries in any MySQL test (verified with the leak restored). The RSS guard remains the only check for growth that stays reachable. If you would rather rely on LSAN alone, drop the regression file and keep the sql-mysql case.isDockerEnabled()gate is gone:describeWithContainerskips on its own, and the gate hid the test from theBUN_TEST_SERVICE_mysql_plainoverride. test(sql): drop redundant isDockerEnabled() gates that block BUN_TEST_SERVICE overrides #37123 drops the same gate in ten files.Notes
The reported 101 s on darwin x64 is a measurement artifact. In build #106656 the darwin x64 shard printed
Ran 0 tests across 1 file. [52.00ms]for this file (no docker on darwin). The slow-test parser charged the625 files in parallelbatch that followed to this file. #35418 fixes that parser and lists this file as an example. Real CI times from the same build: debian 13 x64 0.74 s, debian 13 x64-asan 4.0 s, debian 13 aarch64 skipped (26 ms). On this branch (builds 106802, 106813) the test body takes 0.3 s (x64) and 0.6 s (x64-asan) once the container is ready.Why not just fewer queries. The leak signal is proportional to column decodes (columns x queries), and so is the debug-build cost: about 6 ms per 500-column response (roughly 11 us per column definition under ASAN) plus about 1 ms per query. Pipelining does not apply: the MySQL adapter keeps one command in flight per connection (
MySQLRequestQueue::can_pipelinerequiresis_ready_for_query, whichadvanceclears before the check). Batching 50 queries throughPromise.allonly removes the per-query event-loop turnaround (12% in debug, 2.8x in release). The gains come from 64-byte names (2.7x the leaked bytes per column against 20-byte names), no quarantine noise (so the bound drops from 36 to 8 MiB), and a 100-query warm-up (release noise drops from 2 to 4 MiB at 20 warm-up queries to 0 to 1 MiB at 100).Measurements, fixed debug build, quarantine off, 500 columns, 300 measured queries: -0.1, 3.0, 0.9 MiB (100 warm-up), 1.9, 2.0, 2.0 MiB (50 warm-up). Release: 0, 1, 0 MiB (100 warm-up). Leak restored (debug): 15.0, 16.0, 17.0 MiB. Leak restored, old test shape (debug): 38.4 MiB against the 36 MiB bound.
Restoring the leak for the check: in
ColumnDefinition41::decode_internal, rebuildname_or_indexon every decode andstd::mem::forget(std::mem::replace(&mut self.name_or_index, rebuilt)), which is the original bug's overwrite-without-free. WithASAN_OPTIONS=detect_leaks=1:abort_on_error=1 BUN_DESTRUCT_VM_ON_EXIT=1, five executions of a 4-column SELECT on that build exit 134 withLeakSanitizer: detected memory leaks ... ColumnIdentifier::init ... ColumnDefinition41.rs:202. Without those variables LSAN does not run at exit.Location. The file stays in
test/regression/issue/to keep this change small. The leak existed from the adapter's first commit, so bytest/CLAUDE.mda move totest/js/sql/next tosql-mysql-query-string-leak.test.tswould fit the convention better. That move also needs theregression/issue/28632entry intest/docker/prestart-map.mjsupdated, so it is left for a follow-up or for this PR if you prefer it here.Two things found on the way, not changed here. (1) #40634: when two or more queued queries share a prepared statement whose prepare fails (for example the table does not exist), only the first rejects. The others never settle and
sql.close()hangs. The fixture runs one lone query before the batches so a bad table fails fast instead of hanging. (2) Under a debug build, the same script is about 30% slower throughbun -ethan from a file (1.9 s against 1.2 s for the warm-up phase). Release shows no difference. The fixture is a file for that reason.