Skip to content

test: quarantine test-http2-reset-flood.js on ASAN - #34848

Closed
robobun wants to merge 2 commits into
mainfrom
farm/71a9ddaf/quarantine-http2-reset-flood-asan
Closed

test: quarantine test-http2-reset-flood.js on ASAN#34848
robobun wants to merge 2 commits into
mainfrom
farm/71a9ddaf/quarantine-http2-reset-flood-asan

Conversation

@robobun

@robobun robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Fixes #34846.

Failure

test/js/node/test/parallel/test-http2-reset-flood.js intermittently dies with SIGABRT on debian-13 x64-asan:

ASSERTION FAILED: (null)
!exception()
vendor/WebKit/Source/JavaScriptCore/runtime/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Seen across 11+ unrelated branches (builds 75493, 75670, 75930, 75949, 75996, 75998, 76117, 76129, 76248, 76250, 76342), including a pure deps-bot bump, so not branch-specific.

Cause

The issue body says "with no worker involvement", but that's not the case: this test runs the http2 server inside a Worker and the main thread calls worker.terminate() when the client connection errors:

const worker = new Worker(__filename).on('message', common.mustCall((port) => {
  // ...
  conn.once('error', common.mustCall(() => {
    gotError = true;
    worker.terminate();   // races with a socket handler on the worker's VM
    conn.destroy();
  }));
}));

So this is the same ExceptionScope::assertNoException during-worker-terminate race as #34690, triggered via a different test. Root-cause fix is #34414 (socket/websocket error-handler dispatch guarding against re-entering JS with a pending termination exception).

Change

Adds an [ ASAN ] ... [ CRASH ] entry next to the existing #34095/#34690 entries. The test continues to run on all non-ASAN lanes. Entry verified to parse as {modifiers: ['ASAN'], filename: 'test/js/node/test/parallel/test-http2-reset-flood.js', expectations: ['CRASH']} via the getTestExpectations logic in scripts/runner.node.mjs.

Remove with #34690 / #34414.


no test proof · iteration 1 · docs-only change; test-proof not applicable

The test runs the http2 server inside a Worker and calls
worker.terminate() on conn error, so terminate() races with a socket
handler on the worker's VM and trips the same
ExceptionScope::assertNoException at ExceptionScope.h:61 as #34690.
Seen on debian-13 x64-asan across 11+ unrelated branches.

Root-cause fix is #34414; remove this entry with #34690/#34414.
@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:06 PM PT - Jul 20th, 2026

@robobun, your commit 32f6da2 is building: #76451

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds an ASAN quarantine entry for test-http2-reset-flood.js in test/expectations.txt, with documentation linking the crash to ExceptionScope::assertNoException failures and related tracking issues.

Changes

HTTP/2 ASAN crash quarantine

Layer / File(s) Summary
Add ASAN crash expectation
test/expectations.txt
Adds an [ ASAN ] [ CRASH ] annotation for test-http2-reset-flood.js and documents the related exception and termination-race context.

Possibly related issues

Possibly related PRs

  • oven-sh/bun#33952 — Updates related HTTP/2 quarantine expectations in test/expectations.txt.
  • oven-sh/bun#34414 — Fixes an exception re-entrancy issue associated with the quarantined crash.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR adds the requested [ASAN]...[CRASH] quarantine for test-http2-reset-flood.js, matching #34846's core objective.
Out of Scope Changes check ✅ Passed The only change is a targeted test expectation update and related notes, with no unrelated code changes.
Title check ✅ Passed The title clearly and concisely summarizes the main change: quarantining test-http2-reset-flood.js on ASAN.
Description check ✅ Passed The description covers what changed, why, and a verification note, though it doesn't use the template's exact headings.

Comment @coderabbitai help to get the list of available commands.

@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 — single ASAN quarantine entry for a known worker.terminate() assertion crash.

What was reviewed:

  • Entry format matches neighboring [ ASAN ] ... [ CRASH ] lines and parses per getTestExpectations.
  • Verified test-http2-reset-flood.js exists and does spawn a Worker + call worker.terminate() (lines 8/29/82), so the #34690 linkage holds.
  • Path is specific enough that substring matching won't quarantine sibling tests; non-ASAN lanes still run it.
Extended reasoning...

Overview

This PR adds one line (plus a comment block) to test/expectations.txt, quarantining test/js/node/test/parallel/test-http2-reset-flood.js on ASAN lanes only with a [ CRASH ] expectation. It is placed alongside the existing #34095/#34690 entries for the same ExceptionScope::assertNoException failure class and documents the tracking issue (#34846), root-cause fix (#34414), and removal condition.

Security risks

None. This is a test-runner metadata file; no runtime or build code is touched.

Level of scrutiny

Low. The expectations file header explicitly documents that entries here remove a file from the run on matching platforms — this entry is scoped to [ ASAN ] only, so coverage on release/debug lanes is preserved. I confirmed the target file exists and actually uses Worker + worker.terminate() (test-http2-reset-flood.js:8,29,82), validating the PR's claim that this is the same worker-terminate race as #34690 rather than an unrelated http2 bug. The entry path is a full filename, so the substring match won't accidentally quarantine other test-http2-* files.

Other factors

The comment is thorough (build numbers across 11 unrelated branches, cross-references to #34690/#34414 for removal), which is exactly what the file's header asks for to prevent stale entries. No prior reviews on this PR; the bug-hunting system found nothing.

@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 — straightforward ASAN quarantine entry.

What was reviewed:

  • Entry format matches neighboring [ ASAN ] ... [ CRASH ] lines and the file exists at the given path.
  • Confirmed test-http2-reset-flood.js does spawn a Worker and call worker.terminate(), so the #34690 attribution holds.
  • No substring collision with other paths in expectations.txt; scoped to ASAN only, non-ASAN lanes still run it.
Extended reasoning...

Overview

Single-line addition to test/expectations.txt quarantining test/js/node/test/parallel/test-http2-reset-flood.js on ASAN builds only, plus a 7-line comment documenting the failure mode, affected builds, tracking issue (#34846), root cause (#34690), fix PR (#34414), and removal condition. Placed directly beneath the related worker-transfer-terminate-stress.test.ts entry that shares the same ExceptionScope::assertNoException failure.

Security risks

None. This is a test-infrastructure metadata change; no runtime, build, or dependency code is touched.

Level of scrutiny

Low. The change is mechanical and follows the file's established format exactly. I verified the test file exists at the stated path, that it does in fact use new Worker(__filename) + worker.terminate() (lines 29/82) as claimed in the PR description, and that the entry doesn't substring-collide with any other test path in the file. The [ ASAN ] modifier keeps the test running on all non-ASAN lanes, so coverage loss is minimal and scoped to a known-flaky configuration.

Other factors

The comment names the tracking issue, the shared root cause, the fix PR, and an explicit removal condition ("Remove with #34690/#34414") — exactly what the file header asks for to prevent stale entries. The PR description states the entry was verified against getTestExpectations in scripts/runner.node.mjs. No outstanding reviewer comments and no prior review from me.

@robobun

robobun commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: this diff touches only test/expectations.txt (one ASAN quarantine line + comment).

  • Build 76405: 74/75 jobs passed; the one failure was alpine-3-dot-23-x64-baseline-test-bun expiring before an agent picked it up.
  • Build 76451 (empty retrigger): 72/74 passed; the only hard failures are four test/bake/ dev-server tests on darwin-14-x64 (agent darwin-flatbread-x64-1), all ConnectionRefused / timeout connecting to localhost. That same lane passed on 76405 with identical code. All other annotations are flakes that passed on retry.

None of the failures are related to http2, worker termination, ASAN, or expectations.txt parsing. Ready for review/merge.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: the crash this quarantined was fixed at the root by #34414 (merged 2026-07-21), and #34846 was closed after test-http2-reset-flood.js passed 25/25 consecutive runs on a debug+ASAN build of main. The related tracking issue #34690 has since been closed as well. On current main the test is not in test/expectations.txt and runs on the ASAN lanes, so this entry is no longer needed.

@robobun robobun closed this Aug 13, 2026
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.

CI: test-http2-reset-flood.js SIGABRT (assertNoException) on debian-13-x64-asan across unrelated branches

1 participant