Skip to content

test: avoid TLA self-cycle in bun-main dynamic-import test#29738

Merged
dylan-conway merged 2 commits into
mainfrom
claude/bun-main-tla-cycle-test-fix
Apr 26, 2026
Merged

test: avoid TLA self-cycle in bun-main dynamic-import test#29738
dylan-conway merged 2 commits into
mainfrom
claude/bun-main-tla-cycle-test-fix

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

Follow-up to #29719.

bun:main statically imports the entry file, so await import("bun:main") at the entry's top level is a TLA self-cycle: bun:main waits for entry.mjs (async dep), and entry.mjs waits for bun:main's evaluation promise. Per spec that promise never settles.

The old JSC module loader broke these cycles early, which is why #29719 passed locally (tested against 892042c2, pre-#29393). After #29393 (WebKit module-loader rewrite) the loader correctly leaves the promise unsettled, so the test now hangs and times out at 90s — see build 48023 (debian-asan, win x64, win x64-baseline).

Fix: drop the top-level await and use import("bun:main").then(...) so entry.mjs finishes synchronously, bun:main finishes, and the import resolves on the next microtask. The preload and --hot tests are unaffected (preload isn't in bun:main's dep graph).

Note: this also surfaced that Bun now hangs forever on any unsettled-TLA cycle instead of exiting 13 like Node — separate PR coming for that.

How did you verify your code works?

  • bun bd test test/js/bun/resolve/bun-main-entry-point.test.ts — 3 pass, 20 consecutive runs on Windows
  • USE_SYSTEM_BUN=1 bun test ... — 2 fail (still catches the alias bug on unfixed bun)

bun:main statically imports the entry, so awaiting import("bun:main")
at the entry's top level is a TLA self-cycle that never resolves under
the new JSC module loader (#29393). Prior to that rewrite the loader
broke the cycle early, which is why this passed when #29719 landed.

Switch the entry from `await import("bun:main")` to
`import("bun:main").then(...)` so the entry finishes synchronously,
bun:main finishes, and the import promise resolves on the next tick.
The preload and --hot tests are unaffected.
@robobun

robobun commented Apr 26, 2026

Copy link
Copy Markdown
Collaborator
Updated 11:20 PM PT - Apr 25th, 2026

@dylan-conway, your commit ca81de4 has 3 failures in Build #48048 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29738

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

bun-29738 --bun

@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 deflake, well-reasoned fix for the TLA self-cycle.

Extended reasoning...

Overview

This PR modifies a single test fixture in test/js/bun/resolve/bun-main-entry-point.test.ts. It rewrites the embedded entry.mjs script in the "dynamic import('bun:main')" test case to use import("bun:main").then(...) instead of top-level await import("bun:main"), avoiding a TLA self-cycle deadlock that surfaced after the WebKit module-loader rewrite (#29393). The assertions are unchanged; an explicit rejection handler was added that prints the error and exits 1.

Security risks

None. This is a test-only change to an embedded fixture string. No production/runtime code, no auth/crypto/permissions, no new dependencies.

Level of scrutiny

Low. The change is small (~15 lines net), mechanical, and confined to test fixture code. The reasoning is clearly documented in both the PR description and an inline comment, and the author verified 20 consecutive passes on Windows plus confirmed the test still catches the original alias regression under USE_SYSTEM_BUN=1.

Other factors

  • The assertion semantics are preserved: the same checks run inside the .then() fulfillment handler, and the outer test still asserts stdout: "OK\n", empty stderr, and exitCode: 0. If an assertion throws inside the handler it becomes an unhandled rejection, which will produce non-empty stderr / non-zero exit and correctly fail the outer expect.
  • The other two tests in the file (preload and --hot) are untouched, matching the PR description's note that they aren't in bun:main's dep graph.
  • No prior reviewer comments or outstanding feedback on the timeline; bug-hunting system found no issues.

@coderabbitai

coderabbitai Bot commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 21caa34f-8be7-493c-bb14-872399f29a98

📥 Commits

Reviewing files that changed from the base of the PR and between 34d86be and ca81de4.

📒 Files selected for processing (1)
  • test/js/bun/resolve/bun-main-entry-point.test.ts

Walkthrough

Replaced a top-level await import("bun:main") in the entry.mjs test fixture with a promise-based import("bun:main").then(...) wrapper to avoid a TLA self-cycle deadlock. Moved module-namespace and empty-exports assertions into the then callback and added .catch(...) to log errors and exit with code 1.

Changes

Cohort / File(s) Summary
Test fixture refactor
test/js/bun/resolve/bun-main-entry-point.test.ts
Updated the entry.mjs test fixture: replaced top-level await import("bun:main") with import("bun:main").then(...); moved Symbol.toStringTag and empty-export assertions into the then callback; added .catch(...) to print errors and call process.exit(1).
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: avoid TLA self-cycle in bun-main dynamic-import test' directly describes the main change: fixing a test hang caused by a top-level-await self-cycle.
Description check ✅ Passed The description provides detailed context about the TLA self-cycle issue, references related PRs, explains the fix, and includes verification steps. Both required template sections are addressed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@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 the current code and only fix it if needed.

Inline comments:
In `@test/js/bun/resolve/bun-main-entry-point.test.ts`:
- Around line 29-42: The import("bun:main").then(...) call currently passes a
rejection handler as the second .then argument but assertion errors thrown
inside the fulfillment callback (the function that inspects m using
Symbol.toStringTag and Object.keys) will bypass that and won't trigger the
process.exit(1) path; fix by chaining a .catch() after the .then to log the
error with console.error(String(e)) and call process.exit(1) so any thrown
errors from the fulfillment handler are handled and cause the test to fail.
🪄 Autofix (Beta)

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: 3097c256-f9ef-4f30-9575-3b097705514e

📥 Commits

Reviewing files that changed from the base of the PR and between 30478d2 and 34d86be.

📒 Files selected for processing (1)
  • test/js/bun/resolve/bun-main-entry-point.test.ts

Comment thread test/js/bun/resolve/bun-main-entry-point.test.ts Outdated

@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 test-fixture deflake, and the .catch() feedback is already addressed.

Extended reasoning...

Overview

This PR touches a single test file, test/js/bun/resolve/bun-main-entry-point.test.ts, and only modifies the inline entry.mjs fixture string for one test case. It replaces const m = await import("bun:main") at the entry's top level with import("bun:main").then(...).catch(...), plus an explanatory comment. The assertion logic inside is byte-for-byte identical to before; only the control-flow wrapper changed. The other two tests in the file (preload and --hot) are untouched.

Security risks

None. This is a test-only change to a fixture string that runs in a spawned subprocess inside a temp dir. No runtime, resolver, or production code is modified.

Level of scrutiny

Low. The PR description gives a precise and correct explanation: bun:main statically imports the entry, so top-level await import("bun:main") from the entry creates a TLA self-cycle that per spec never settles, and the post-#29393 module loader now (correctly) honors that. Dropping the top-level await lets entry.mjs finish synchronously so bun:main can settle and the dynamic import resolves on the next microtask. The .then().catch() shape ensures both import rejection and assertion throws inside the fulfillment handler hit process.exit(1), so regressions still fail the outer exitCode: 0 / stdout: "OK\n" assertion. The change is mechanical and self-contained.

Other factors

  • No bugs flagged by the bug-hunting system.
  • CodeRabbit's one nit (use chained .catch() instead of the second .then arg so assertion throws are caught) was addressed in ca81de4 and the final diff reflects it; that thread is resolved.
  • Author verified 3/3 pass over 20 consecutive Windows runs, and confirmed the test still fails on an unfixed bun (so it hasn't been neutered).
  • The robobun ❌ on build #48033 is the generic CI status comment; given this PR only edits one test fixture string, any unrelated failures there are not attributable to this change.

@dylan-conway dylan-conway merged commit ad5d333 into main Apr 26, 2026
56 of 60 checks passed
@dylan-conway dylan-conway deleted the claude/bun-main-tla-cycle-test-fix branch April 26, 2026 06:07
xhjkl pushed a commit to xhjkl/bun that referenced this pull request May 14, 2026
…9738)

Follow-up to oven-sh#29719.

`bun:main` statically imports the entry file, so `await
import("bun:main")` at the entry's top level is a TLA self-cycle:
`bun:main` waits for `entry.mjs` (async dep), and `entry.mjs` waits for
`bun:main`'s evaluation promise. Per spec that promise never settles.

The old JSC module loader broke these cycles early, which is why oven-sh#29719
passed locally (tested against `892042c2`, pre-oven-sh#29393). After oven-sh#29393
(WebKit module-loader rewrite) the loader correctly leaves the promise
unsettled, so the test now hangs and times out at 90s — see build 48023
(debian-asan, win x64, win x64-baseline).

Fix: drop the top-level `await` and use `import("bun:main").then(...)`
so `entry.mjs` finishes synchronously, `bun:main` finishes, and the
import resolves on the next microtask. The preload and `--hot` tests are
unaffected (preload isn't in `bun:main`'s dep graph).

Note: this also surfaced that Bun now hangs forever on any unsettled-TLA
cycle instead of exiting 13 like Node — separate PR coming for that.

## How did you verify your code works?

- `bun bd test test/js/bun/resolve/bun-main-entry-point.test.ts` — 3
pass, 20 consecutive runs on Windows
- `USE_SYSTEM_BUN=1 bun test ...` — 2 fail (still catches the alias bug
on unfixed bun)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants