Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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 = "2e2aa2290fac856d6f451ceacb58f7f5b44dd057";
export const WEBKIT_VERSION = "autobuild-preview-pr-567-e62d118e";

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 pinned to autobuild-preview-pr-567-e62d118e, an ephemeral preview tag; if merged as-is, clean builds break once oven-sh/WebKit#567 merges or closes and the tag is garbage-collected. Fix: before merge, swap the pin to the merged upstream oven-sh/WebKit main SHA and verify prebuilt artifacts exist for every platform × flavor (per .claude/docs/landing-prs.md §Dependencies & vendoring). The PR description already commits to this; noting it as a hard merge blocker.

Extended reasoning...

prebuiltUrl() (scripts/build/deps/webkit.ts:73) resolves the download URL from WEBKIT_VERSION; autobuild-preview-pr-* release tags on oven-sh/WebKit are ephemeral and are deleted once the associated PR is merged or closed. On the base branch the pin is a merged main SHA (2e2aa2290fac…) whose release is permanent. After this change merges, once WebKit#567 lands and the preview tag is pruned, every fresh checkout / CI cache miss fails at the WebKit prebuilt fetch step with a 404, blocking all builds until someone repins. .claude/docs/landing-prs.md explicitly forbids merging pins to preview tags or unmerged-PR builds. The author states the pin will move before landing; this comment tracks that requirement.

Verification: normal — acknowledged in diff: the PR description states "Before landing, oven-sh/WebKit#567 has to merge and the pin has to move to the resulting main sha", which is accurate and matches the repository rule; the finding correctly restates it as a hard merge blocker rather than resolved. scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from "2e2aa2290fac856d6f451ceacb58f7f5b44dd057"…

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and the PR body says the same: this does not land on the preview tag. Once oven-sh/WebKit#567 merges, the pin moves to the resulting main sha and CI runs again. The thread stays open until then.


/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
143 changes: 143 additions & 0 deletions test/js/bun/wasm/compile-rss-fixture.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 34 additions & 0 deletions test/js/bun/wasm/compile-rss.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isDebug } from "harness";
import path from "node:path";

// Unfixed, the 8 idle wasm compiler threads keep 17 to 25 MB of freed compile temporaries until
// they exit after 10 s. Fixed, they release it about 100 ms after the last compile and RSS returns
// to where it started.
const idleTargetMiB = 10;

// Debug and ASAN builds link a JavaScriptCore that does not allocate through mimalloc, so the
// per-thread retention this test checks for does not exist there.
test.skipIf(isDebug || isASAN)(
"WebAssembly.compile does not retain memory in idle compiler threads (#41438)",
async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "compile-rss-fixture.mjs"), String(idleTargetMiB)],
env: {
...bunEnv,
// The retained amount scales with the compiler thread count. Pin it so the test does not
// depend on the core count of the machine.
BUN_JSC_numberOfWasmCompilerThreads: "8",
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
const result = JSON.parse(stdout.trim().split("\n").at(-1)!);
// The compiles have to grow RSS well past the target first, or the check below means nothing.
expect(result.peakDeltaMiB).toBeGreaterThan(idleTargetMiB * 2);
expect(result.idleDeltaMiB).toBeLessThan(idleTargetMiB);
Comment thread
robobun marked this conversation as resolved.
Resolved
expect(exitCode).toBe(0);
},
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Loading