Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion scripts/build/deps/webkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b";
export const WEBKIT_VERSION = "preview-pr-228-3a85cc44";

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.

⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoff

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify the WebKit preview release and upstream PR exist

# Check if the release tag exists and list its assets
echo "=== Checking release tag ==="
gh api repos/oven-sh/WebKit/releases/tags/autobuild-preview-pr-228-3a85cc44 \
  --jq '{name: .name, tag: .tag_name, draft: .draft, prerelease: .prerelease, assets: [.assets[].name]}' \
  || echo "ERROR: Release tag not found"

# Check if PR `#228` exists
echo -e "\n=== Checking upstream PR `#228` ==="
gh api repos/oven-sh/WebKit/pulls/228 \
  --jq '{number: .number, title: .title, state: .state, merged: .merged}' \
  || echo "ERROR: PR not found"

Repository: oven-sh/bun

Length of output: 445


The release tag autobuild-preview-pr-228-3a85cc44 does not exist in oven-sh/WebKit.

The WEBKIT_VERSION points to a preview release that cannot be found (HTTP 404). When the build attempts to download prebuilt artifacts, it will fail. The upstream PR #228 exists and is open (not yet merged), which explains why the preview release doesn't exist.

Either create the release tag in oven-sh/WebKit with the required platform artifacts, or point WEBKIT_VERSION to an existing stable release and track the upstream PR separately.

🤖 Prompt for 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.

In `@scripts/build/deps/webkit.ts` at line 6, The WEBKIT_VERSION constant
currently points to a non-existent preview release ("preview-pr-228-3a85cc44")
causing 404s when downloading artifacts; fix by either creating the matching
release/tag and uploading the required platform artifacts in the oven-sh/WebKit
repo, or update the WEBKIT_VERSION value to a known existing release tag
(replace the string in the WEBKIT_VERSION export) that contains the prebuilt
artifacts you need; ensure the chosen tag is reachable and contains assets for
all target platforms so the build can download successfully.

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.

🔴 WEBKIT_VERSION is set to the temporary preview tag preview-pr-228-3a85cc44 instead of a commit SHA. prebuiltUrl() (lines 76-78) only special-cases the autobuild- prefix, so the download URL becomes .../releases/download/autobuild-preview-pr-228-3a85cc44/... and prebuilt-mode builds will 404; it also corrupts process.versions.webkit and the cache key (prebuiltDestDir() slices to preview-pr-228-3). This needs to be replaced with the merged commit hash from oven-sh/WebKit#228 before landing.

Extended reasoning...

What the bug is

WEBKIT_VERSION at scripts/build/deps/webkit.ts:6 is changed from a 40-char commit SHA (5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b) to "preview-pr-228-3a85cc44", a temporary preview-build tag for the still-open oven-sh/WebKit#228. The doc comment immediately above (lines 1-5) explicitly documents this constant as a "WebKit commit — determines prebuilt download URL + what to checkout for local mode", and git log -p on this file shows every prior value has been a full commit SHA. Nothing in scripts/ handles a preview-pr- prefix (grep finds no other occurrence).

Code path that triggers it

config.ts:56 wires WEBKIT_VERSION directly into cfg.webkitVersion with no normalization. prebuiltUrl() at webkit.ts:76-78 then computes the release tag as:

const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;

There is no branch for preview-pr-. Since the default cfg.webkit mode is "prebuilt" (used by CI and most local builds), this URL is the one fetched whenever the WebKit dep is resolved.

Why existing code doesn't prevent it

The only special case in prebuiltUrl() is the autobuild- prefix check, intended for users who pass --webkit-version=autobuild-<sha> verbatim. A preview-pr-... value falls into the else branch and gets autobuild- prepended. The override-detection warning at config.ts:1073-1076 ("you're not using the default WebKit version") only fires when the runtime value differs from the default — here the bad value is the default, so no warning fires.

Step-by-step proof

  1. cfg.webkitVersion = "preview-pr-228-3a85cc44" (default from WEBKIT_VERSION).
  2. prebuiltUrl(): "preview-pr-228-3a85cc44".startsWith("autobuild-")false.
  3. tag = "autobuild-preview-pr-228-3a85cc44".
  4. URL = https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-228-3a85cc44/bun-webkit-linux-amd64-lto.tar.gz.
  5. oven-sh/WebKit publishes preview builds under preview-pr-<n>-<sha> tags, not autobuild-preview-pr-..., so this asset does not exist → the WebKit fetch step 404s and the build fails.
  6. Even if such a release happened to exist, preview-PR tags are ephemeral and are GC'd once the WebKit PR merges, so any commit on main pinned to it would rot.

Secondary impact

  • depVersionsHeader.ts:59 emits cfg.webkitVersion verbatim into the generated header, so process.versions.webkit would report "preview-pr-228-3a85cc44" instead of a commit SHA.
  • prebuiltDestDir() at webkit.ts:86 keys the cache on .slice(0, 16) = "preview-pr-228-3", dropping the trailing commit-ish entirely; a future preview of the same PR would collide with this cache entry.
  • Local mode ("what to checkout") would attempt to check out a ref that isn't a commit in the WebKit repo.

Fix

Replace "preview-pr-228-3a85cc44" with the 40-char commit SHA of the merged oven-sh/WebKit#228 (i.e. wait for that PR to land, then pin to its merge commit) before merging this PR. This is the standard cross-repo workflow placeholder that needs to be swapped before landing.


/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
142 changes: 72 additions & 70 deletions test/js/bun/resolve/dynamic-import-tla-cycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,77 +2,23 @@ import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";

// A top-level-awaited dynamic import whose target statically imports the
// awaiting module back. The spec's innerModuleEvaluation 11.c.v would have the
// awaiting module back. The spec's innerModuleEvaluation 11.c.v makes the
// chunk wait on the entry's async-evaluation order, but the entry can only
// finish once the chunk's evaluate() promise settles — a self-deadlock. Bun
// matches the pre-rewrite loader and lets the chunk evaluate immediately
// against the entry's already-initialised bindings.
test("dynamic import inside TLA whose target imports the awaiter back does not deadlock", async () => {
using dir = tempDir("dyn-tla-cycle", {
"index.mjs": `
import fs from "node:fs";
export const x = 42;
const chunk = await import("./chunks/stream.mjs");
console.log("chunk loaded:", chunk.handler());
`,
"chunks/stream.mjs": `
import { x } from "../index.mjs";
import fs from "node:fs";
export const handler = () => x + (fs.existsSync("/") ? 1 : 0);
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "index.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe("chunk loaded: 43");
expect(exitCode).toBe(0);
});

// Same self-deadlock pattern, but the awaiting module is not the Evaluate()
// entry — it's a static dependency of the entry. The cycle root re-entered by
// the chunk has no TopLevelCapability of its own, so the discriminator must
// be "has its body started" (pendingAsyncDependencies == 0), not "is it the
// Evaluate() entry".
test("dynamic import inside TLA of a non-entry module whose target imports it back does not deadlock", async () => {
using dir = tempDir("dyn-tla-cycle-nonentry", {
"entry.mjs": `
import { result } from "./mid.mjs";
console.log("result:", result);
`,
"mid.mjs": `
export const x = 42;
const chunk = await import("./chunk.mjs");
export const result = chunk.handler();
`,
"chunk.mjs": `
import { x } from "./mid.mjs";
export const handler = () => x + 1;
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "entry.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe("result: 43");
expect(exitCode).toBe(0);
});
// finish once the chunk's evaluate() promise settles — a self-deadlock, and
// Node prints "unsettled top-level await". Bun used to divert from spec at
// 11.c.v to match the pre-rewrite loader behaviour (let the chunk evaluate
// immediately against the entry's already-initialised bindings), but that
// custom skip also fired for unrelated sibling dynamic imports and left
// their importers reading post-`await` exports while they were still in
// TDZ (#30634 — breaks @lexical/react and other packages that dispatch
// dev/prod via `await import()` in a wrapper module). The skip was dropped;
// this pattern now matches spec/Node behaviour (deadlock). Reinstating a
// narrower skip that distinguishes the self-deadlock case from the
// sibling-race case requires threading the dynamic-import referrer from
// Bun's moduleLoaderImportModule hook through ModuleLoaderPayload to the
// evaluate path — tracked for follow-up.
test.todo("dynamic import inside TLA whose target imports the awaiter back does not deadlock");
test.todo("dynamic import inside TLA of a non-entry module whose target imports it back does not deadlock");
Comment on lines +20 to +21

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.

🔴 Converting the two reduced self-deadlock tests to .todo is correct, but the real Nitro-output fixture they were reduced from was not updated: test/js/bun/http/bun-server.test.ts:405 ("should be able to parse source map and fetch small stream") spawns js-sink-sourmap-fixture/index.mjs, whose top-level await fetch(/stream) triggers import('./chunks/stream.mjs'), and chunks/stream.mjs:6 statically imports back ../index.mjs. With the 11.c.v skip dropped that self-deadlocks, Bun.serve keeps the loop alive so there's no unsettled-TLA exit, and the test calls Bun.spawnSync with no timeout — so the entire bun-server.test.ts file will hang in CI. That test needs the same .todo treatment (or at minimum a spawnSync timeout) before this lands.

Extended reasoning...

What the bug is

The WebKit change (oven-sh/WebKit#228) drops the Bun-specific deadlock-avoidance skip at innerModuleEvaluation step 11.c.v. The PR description acknowledges that the "Nitro-style await import(./chunk)-loops-back" pattern now matches spec/Node behaviour (deadlock), and accordingly converts the two reduced reproductions in dynamic-import-tla-cycle.test.ts to test.todo.

However, those reduced tests were minimised from a real Nitro output fixture that still exists in the test suite: test/js/bun/http/js-sink-sourmap-fixture/. The now-deleted reduction even uses the identical file naming (index.mjs + chunks/stream.mjs). The original fixture was not given the same .todo treatment, and with the skip removed it will hang.

Code path

  1. bun-server.test.ts:405-415 runs Bun.spawnSync({ cmd: [bunExe(), "js-sink-sourmap-fixture/index.mjs"], ... }) — synchronous, no timeout option.
  2. js-sink-sourmap-fixture/index.mjs:5577-5599 is module-top-level code: it calls Bun.serve(...), then const result = await fetch(${server.url}/stream). This top-level await makes index.mjs HasTLA; the module suspends here with status EvaluatingAsync.
  3. The /stream route is wired to _lazy_VB431L = () => import('./chunks/stream.mjs') (line 5442/5447) via h3's defineLazyEventHandler (line 2146-2171), which does Promise.resolve(factory()).then(...) on first request. So serving the request awaits the import() promise.
  4. chunks/stream.mjs:6 has import { e as eventHandler } from '../index.mjs'; — a static import back to the still-EvaluatingAsync entry.

Why existing code doesn't prevent it

With the custom skip removed, spec step 11.c applies when stream.mjs's fresh Evaluate() DFS visits index.mjs (status evaluating-async, AsyncEvaluation true): stream.mjs.[[PendingAsyncDependencies]] is incremented to 1 and stream.mjs is appended to index.mjs.[[AsyncParentModules]]. stream.mjs's body is therefore not executed; the import() promise stays pending until index.mjs finishes async evaluation. But index.mjs is suspended at await fetch(...), which can only resolve once the /stream handler responds, which requires stream.mjs to evaluate — circular wait.

Bun.serve's listening socket keeps the event loop alive, so the "unsettled top-level await" exit path does not fire. The child process hangs indefinitely. Even if idleTimeout eventually rejects the fetch (→ catchprocess.exit(1)), the test still goes from PASS → FAIL on expect(exitCode).toBe(0).

This wasn't caught locally because, per the robobun comment, only dynamic-import-tla-cycle.test.ts was run with the patched JSC; full Bun CI is still blocked on the WebKit preview tarball publishing.

Step-by-step proof

  1. Test runner thread enters Bun.spawnSync at bun-server.test.ts:406 and blocks in native code waiting for the child.
  2. Child evaluates index.mjs → starts Bun.serve → reaches await fetch('/stream') at line 5599 → module status becomes EvaluatingAsync, JS stack unwinds.
  3. Server receives the request → lazyEventHandler calls import('./chunks/stream.mjs').
  4. innerModuleEvaluation(stream.mjs) recurses into ../index.mjs → finds AsyncEvaluation == true → sets stream.[[PendingAsyncDependencies]] = 1, returns. Back in stream.mjs: pendingAsyncDependencies > 0 → body NOT executed. import() returns an unsettled promise.
  5. Lazy handler's .then(r => r.default ...) never fires → no Response → fetch at 5599 never settles → index.mjs never finishes → stream.mjs is never released. Server socket keeps the loop alive → no process exit.
  6. spawnSync blocks the parent's JS thread; bun:test's async per-test timer cannot interrupt a thread blocked in a native syscall. The whole bun-server.test.ts file stalls.

Impact

CI-breaking regression: bun-server.test.ts will hang once a build links the patched JSC. Best case (if something eventually times the connection out) the test goes PASS → FAIL.

Fix

Mark bun-server.test.ts:405 as test.todo alongside the two reduced cases (referencing the same follow-up tracking), or add a timeout to the Bun.spawnSync call and convert the assertion to expect the deadlock until the narrower referrer-aware skip lands. Longer term the fixture could be restructured so chunks/stream.mjs no longer statically re-imports index.mjs.


// The deadlock-avoidance above must NOT fire for sibling static imports in the
// same Evaluate() pass. Here `entry` first imports `a` (in an SCC {a,c} with
Expand Down Expand Up @@ -199,3 +145,59 @@ test("static sibling import waits for an indirectly-shared TLA dep in the same E
expect(stdout.trim()).toBe("456");
expect(exitCode).toBe(0);
});

// #30634: sibling dynamic imports from the event loop (Promise.all of two
// top-level import() calls) that share a TLA wrapper dep. Each import() is
// its own Evaluate() at the top of the stack, so by the time consumer2's DFS
// visits wrapper, wrapper is EvaluatingAsync (popped at the end of consumer1's
// DFS), its asyncEvaluationOrder is below the new watermark, and its
// pendingAsyncDependencies is 0 — matching every earlier discriminator.
// But wrapper's post-`await` `export const` assignments have not run yet
// (its continuation is queued in the microtask queue, not on the C++ stack),
// so skipping the spec wait runs consumer2 with wrapper's exports in TDZ.
// The discriminator must additionally require the dep's body to be actively
// executing on the JS call stack (Field::State == Executing) — true for
// require(esm)/dynamic-import re-entry from inside wrapper's continuation,
// false for a sibling import racing in from a fresh event-loop turn.
test("sibling dynamic imports in Promise.all wait for a shared TLA wrapper", async () => {
using dir = tempDir("sibling-dynamic-tla", {
"entry.mjs": `
await Promise.all([import("./consumer1.mjs"), import("./consumer2.mjs")]);
console.log("ok");
`,
"wrapper.mjs": `
const mod = await import("./inner.mjs");
export const FOO = mod.FOO;
export const BAR = mod.BAR;
`,
"inner.mjs": `
export const FOO = "hello";
export const BAR = "world";
`,
"consumer1.mjs": `
import { FOO } from "./wrapper.mjs";
console.log("c1:", FOO);
`,
"consumer2.mjs": `
import { BAR } from "./wrapper.mjs";
console.log("c2:", BAR);
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "entry.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
// Order of c1/c2 isn't part of the contract — both must appear with their
// correct bindings, and the final "ok" must print.
const lines = stdout.trim().split("\n").sort();
expect(lines).toEqual(["c1: hello", "c2: world", "ok"]);
expect(exitCode).toBe(0);
});
Loading