Skip to content

test: make 12042.test.ts exit cleanly under LeakSanitizer and tighten its assertions - #37479

Open
robobun wants to merge 1 commit into
mainfrom
farm/3d5ba043/speed-up-12042-test
Open

robobun wants to merge 1 commit into
mainfrom
farm/3d5ba043/speed-up-12042-test

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Test-only change. test/regression/issue/12042.test.ts took 16.0s on the debian 13 x64-asan lane (builds 91926 and 91993) and 0.03s on every other lane. One test, one child process, one loopback request.

What the 15 seconds were

Not the child lingering: under the release build the whole test takes ~14ms, and the child exits as soon as await server.stop() resolves (graceful stop() already closes idle keep-alive connections).

The ASAN lane runs tests and their children with ASAN_OPTIONS=detect_leaks=1, BUN_DESTRUCT_VM_ON_EXIT=1 and LSAN_OPTIONS=suppressions=test/leaksan.supp (scripts/runner.node.mjs). After a graceful stop() of a server that served a request, VM teardown leaves the server and its uWS app allocated: Server::finalize() only frees inline at shutdown when the server is TERMINATED, which a graceful stop never sets. LeakSanitizer finds that graph (158 allocations, 13.6KB, all indirect) and ends up suppressing all of it (leak:uws_create_app matches the app and context, and LSAN ignores whatever is reachable from a suppressed block), so the child still exits 0. Matching the suppressions needs symbolized stacks though, so LSAN launches llvm-symbolizer against the bun binary and the child sits in a pipe read until it has loaded the debug info: ~5.5s with a local debug build, ~15s with CI's release ASAN binary. The unfixed test never noticed because it only awaited the stdio pipes and did a single toContain.

Minimal form, under the same environment the runner uses (BUN_DESTRUCT_VM_ON_EXIT=1 ASAN_OPTIONS=detect_leaks=1 LSAN_OPTIONS=suppressions=$PWD/test/leaksan.supp, debug build):

const server = Bun.serve({ port: 0, fetch: () => new Response("x") });
await fetch(server.url);
await server.stop();       // 5.8s, LSAN reports "Suppressions used: uws_create_app"
// await server.stop(true) // 0.37s, nothing for LSAN to symbolize

The teardown side of this (a drained, gracefully stopped server not being freed at exit) is reported separately; this PR only fixes the test.

Changes to the test

  • The child calls server.stop(true), so the server is freed at exit and LSAN has nothing to symbolize. The verbose fetch output this test exists for is produced before that point, so coverage is unchanged.
  • The script is passed with -e instead of a temp dir.
  • The server echoes back the Content-Type it received and the request body; the test asserts both, so the body is known to have arrived intact for each request.
  • Two requests: one with an explicit Content-Type: application/x-www-form-urlencoded (the case from curl log with BUN_CONFIG_VERBOSE_FETCH does not print request body for "Content-Type": "application/x-www-form-urlencoded" #12042) and one with no header, where fetch derives application/x-www-form-urlencoded;charset=UTF-8 from the URLSearchParams body. The fix in fix(fetch): print request body for application/x-www-form-urlencoded in curl logs #22849 is a prefix match, and the second request covers the ;charset= form of it.
  • Both curl lines from stderr are pinned with an inline snapshot (port and Bun version normalized), including the Content-Length and the full --data-raw "..." argument, instead of one toContain.
  • proc.exited is awaited and the exit code is asserted (last). The old test would have passed even if the child had aborted after printing the line, which is exactly what it would do under LSAN if any of those allocations were not suppressed.

Timing

Local, bun bd (debug + ASAN) build. bun bd test alone does not set the LSAN environment, so the slowdown only shows up with the runner's variables set:

before after
bun bd test test/regression/issue/12042.test.ts (test / file) 373-385ms / 2.8s 374-408ms / 3.0s
same, with the ASAN lane's env vars (BUN_DESTRUCT_VM_ON_EXIT=1, detect_leaks=1, suppressions) 6372ms / 9.18s 498ms / 3.22s
CI, debian 13 x64-asan, build 91993 16.04s

The ~2.7s of file time that remains locally is debug-build test runner startup and is paid by every file. The remaining ~0.4s of test time is the debug child's own startup. Also passes with the release binary (USE_SYSTEM_BUN=1, 10ms), which checks that the snapshot is stable across version strings.

The child gracefully stopped a Bun.serve server after serving a request,
which leaves the server and its uWS app allocated at exit. On the ASAN
lane LeakSanitizer then symbolizes those (suppressed) allocations, which
costs about 15 seconds per run of this file. stop(true) frees the server
so the child exits immediately.

Also run the script via -e instead of a temp dir, echo the request body
back from the server and assert it, pin both curl lines (explicit and
derived content type) with an inline snapshot, and check the exit code.
@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:46 AM PT - Aug 11th, 2026

✅ @robobun, your commit beb4a9c9507e49195dff073361edae4a6e04fde2 passed in Build #92199! 🎉


🧪   To try this PR locally:

bunx bun-pr 37479

That installs a local version of the PR into your bun-37479 executable, so you can run:

bun-37479 --bun

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced locally with a bun bd (debug + ASAN) build by setting the variables the ASAN lane's runner sets (BUN_DESTRUCT_VM_ON_EXIT=1, ASAN_OPTIONS=detect_leaks=1, LSAN_OPTIONS=suppressions=test/leaksan.supp): the test went from 6.4s to 0.5s with this change (16.0s on the CI lane before, see the PR body). Plain bun bd test and USE_SYSTEM_BUN=1 both pass.

The underlying teardown behaviour (a gracefully stopped server is not freed at exit under BUN_DESTRUCT_VM_ON_EXIT) is tracked separately; this PR is test-only.

@coderabbitai coderabbitai Bot left a comment

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.

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/regression/issue/12042.test.ts`:
- Around line 14-41: Wrap the fetch and response-parsing sequence in the
regression test’s server setup flow with try/finally, and move await
server.stop(true) into the finally block. Ensure the server is stopped whether
either fetch or JSON parsing succeeds or rejects, without changing the existing
request assertions or payloads.
🪄 Autofix

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: 7d69892c-d22a-489a-809d-9a6f94e8304b

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and beb4a9c.

📒 Files selected for processing (1)
  • test/regression/issue/12042.test.ts

Comment thread test/regression/issue/12042.test.ts

@claude claude Bot left a comment

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.

LGTM — test-only tightening that also removes ~15s of LSAN symbolization on the ASAN lane.

What was reviewed:

  • Checked normalizeBunSnapshot replaces Bun.version so User-Agent: Bun/<bun-version> is stable across debug/release; server.url defaults to localhost (server_body.rs:2785) so Host: and the URL are platform-stable.
  • Verified Content-Length: 61 matches the encoded body length and the derived ;charset=UTF-8 variant covers the prefix-match path from #22849.
  • Confirmed harness conventions: pipes drained concurrently with proc.exited, exit code asserted last, bunEnv spread, port: 0, -e for single-file child.
Extended reasoning...

Overview

Single-file test-only change to test/regression/issue/12042.test.ts. The child script switches from server.stop() to server.stop(true) so the uWS app is freed at exit and LeakSanitizer has nothing to symbolize (the ~15s stall on the debian x64-asan lane). Along the way the test is tightened: it now sends two requests (explicit Content-Type and the fetch-derived ;charset=UTF-8 variant), the server echoes back the received header and body which are asserted exactly, both curl lines from BUN_CONFIG_VERBOSE_FETCH=curl are pinned with an inline snapshot (port and version normalized), and the child's exit code is awaited and asserted last. The temp dir is dropped in favour of -e.

Security risks

None. Test-only; no production code, no auth/crypto/permissions surface, no external network (loopback port: 0 only), no untrusted input.

Level of scrutiny

Low. This is a regression test refactor with no runtime code changes. The snapshot's stability was the only real question — I verified normalizeBunSnapshot in test/harness.ts does .replaceAll(Bun.version, "<bun-version>") (parent and child use the same bunExe() binary so versions match), and that Bun.serve({ port: 0 }) with no hostname always yields localhost in server.url (src/runtime/server/server_body.rs:2785), so the Host: header and URL in the snapshot are platform-independent. Content-Length: 61 matches the 61-byte body string. The header set/order in the curl line is generated by Bun's fetch client, not the OS, so it is deterministic.

Other factors

The change follows the repo's test guidance closely: stdio pipes and proc.exited are awaited together in one Promise.all, the exit code is asserted after the content assertions, bunEnv is spread when adding BUN_CONFIG_VERBOSE_FETCH, the strongest invariant is asserted (exact echoed body/header + full snapshot) instead of a single toContain, and a second variant is added alongside rather than replacing the original case. The three-line comment above server.stop(true) names why no observable signal exists for the LSAN stall, which is what REVIEW.md asks for. The PR body reports it passes under bun bd test, under the ASAN lane's env vars (6.4s → 0.5s), and under USE_SYSTEM_BUN=1. No prior reviewer comments to address.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant