ci: add linux aarch64 ASAN build and test lanes to PR pipelines - #31699
ci: add linux aarch64 ASAN build and test lanes to PR pipelines#31699robobun wants to merge 9 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughRefactors cached-bytecode ownership between Rust and C++, fixes VM/server shutdown and resource-drop ordering, adds Linux aarch64 ASAN CI lanes (skipping baseline verification for ASAN), and bumps the WebKit prebuilt version. ChangesBytecode Ownership & Shutdown Cleanup
CI Infrastructure & Dependencies
HTTP response metadata change
Minor comments and tests
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 4:46 PM PT - Jun 3rd, 2026
❌ @robobun, your commit 212daf8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31699That installs a local version of the PR into your bun-31699 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues with the code — it cleanly mirrors the x64 ASAN lane and the needsBaselineVerification guard is correct — but adding a 20-shard r8g.2xlarge test lane plus build steps to every PR is a CI capacity/cost decision worth a human sign-off.
Extended reasoning...
Overview
This PR adds a linux-aarch64 ASAN build+test lane to the Buildkite pipeline by appending one entry each to buildPlatforms and testPlatforms in .buildkite/ci.mjs, and adds an early profile === "asan" return to needsBaselineVerification(). The new entries are exact mirrors of the existing linux-x64 ASAN entries with arch swapped to aarch64.
Security risks
None. This is CI pipeline configuration only — no auth, secrets handling, or release-artifact paths are touched. ASAN lanes are already filtered out on main and excluded from release/binary-size steps, so the new lane cannot affect shipped artifacts.
Level of scrutiny
The code change itself is low-risk and mechanically correct: the supporting plumbing (getTestAgent aarch64+asan → r8g.2xlarge, getRustAgent asan → r8g.4xlarge, main-branch ASAN filtering, release/binary-size exclusion) was already in place, and the needsBaselineVerification guard is necessary and correctly scoped — linux-aarch64 unconditionally triggered verify-baseline, which would download a -profile.zip that ASAN link steps don't produce. The previous x64 ASAN lane never hit this because linux-x64 only verifies when baseline is set.
Other factors
The reason I'm deferring rather than approving is not correctness but scope: this adds ~20 r8g.2xlarge test shards (45 min timeout) plus c8g.4xlarge/r8g.4xlarge/r8g.2xlarge build agents to every PR build. That's a meaningful recurring infrastructure cost and fleet-capacity commitment that a maintainer should explicitly green-light, even though the implementation is sound and self-verifying (the lane runs on this PR's own CI).
|
Status: blocked on oven-sh/WebKit#247 — marking draft until a fixed WebKit prebuilt is pinned. The first run of the new lane (build 59814) proved the pipeline itself: Root cause (a latent JSC bug, not a pipeline bug): on Linux ARM64, Fix: oven-sh/WebKit#247 bounds the assert by |
62a8f6e to
2edbb2a
Compare
|
✅ All review feedback addressed, conflicts resolved, all review threads resolved. Ready for a maintainer.
CI status (build 60069): every lane that runs this diff is green — including the new linux-aarch64-asan lane (the PR's purpose) and all server test shards. The sole failure is |
Mirrors the existing linux x64 ASAN lane: release-asan build on
amazonlinux 2023 (aarch64) plus a 20-shard test step on debian 13.
Like x64-asan, the lane is PR-only — filtered out on the main branch.
Skip baseline verification for asan profiles: asan link steps upload
${triplet}-asan.zip, not the ${triplet}-profile.zip that step downloads,
and asan artifacts never ship in releases.
Picks up oven-sh/WebKit#247: FreeList::forEach asserted a hardcoded 16 KB interval bound, but MarkedBlock::blockSize is 64 KB on Linux ARM64 (CeilingOnPageSize), so every ASSERT_ENABLED arm64-linux artifact (-asan, -debug, -debug-asan) crashed at the first GC stop. The new linux-aarch64-asan lane needs the fixed -asan prebuilt. Also picks up from oven-sh/WebKit main: xwin 0.9.0, UB fix in double-to-int conversions, JIT disassembler compiled out of release, windows amd64-baseline ThinLTO variant.
01c1d50 to
daf118d
Compare
Covers four leak groups across the runtime: - Bytecode cache (transpiler/module loader): the sidecar .jsc buffer read for '// @Bun @bytecode' modules was never freed. The CachedBytecode destructor was selected off ResolvedSource.needsDeref, which tracks the source_code string and is cleared (and externally mutated) before the selection ran, so the no-op destructor was always chosen. Add a dedicated bytecode_cache_needs_deref flag (Rust + C++ struct mirror), set it at the two heap::into_raw producer sites, key the destructor off it, clear it once CachedBytecode adopts the buffer, and free the buffer in OwnedResolvedSource::drop when ownership never crossed FFI. Also fixes the async RuntimeTranspilerStore path passing a pointer borrowed from a local parse_result (dangling by the time the JS thread used it) by transferring ownership instead. Embedded bun build --compile bytecode keeps the no-op destructor (flag stays false). - Bun.serve teardown: a server finalized during VM shutdown enqueued its App.close + deinit task pair on an event loop that never ticks again; EventLoop::deinit freed the tasks unrun and the entire server graph (NewServer box, ServerConfig strings, routes, HTMLBundle routes) leaked. Run close + deinit synchronously when the VM is shutting down, preserving close-before-destroy ordering. Guard JSNodeHTTPServerSocket::onData against allocating JS cells during shutdown, mirroring onClose. - Dev server route bundles: DevServer::drop released client_bundle and cached_response but not the intrusive ref RouteBundle holds on its html_bundle route (raw pointer, so Vec drop could not release it), leaking one HTMLBundleRoute per bundled route. Mirror Zig's RouteBundle.deinit and deref it. - Watcher: WatchItem.file_path is an owning Cow column in a MultiArrayList whose Drop is slab-only, so owned paths leaked at watcher teardown and on every eviction (swap_remove is a bitwise overwrite). Drop the rows before freeing the slab and take the evicted path out before swap_remove. - Cron teardown ordering: VirtualMachine::destroy took rare_data before calling cron_clear_all_teardown, which early-returns when rare_data is None - the hook was a no-op, leaking still-registered CronJobs and tripping the cron_jobs.is_empty() debug assert when a job survived finalization (process.exit from inside a cron callback under --hot). Call the hook before taking rare_data.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@scripts/build/deps/webkit.ts`:
- Line 10: Update the WEBKIT_VERSION bump verification by confirming the
prebuilt artifacts exist for the new tag referenced by WEBKIT_VERSION
("f18cf9c267c72ff3d1ca31eec864ebcbeefd29b5"): check the GitHub release URL for
the tag (autobuild-<hash>) and verify presence of linux (amd64/arm64) with
default/lto/asan, macos (amd64/arm64) with default/lto, and windows (amd64) with
default/lto; if any artifacts are missing either revert or pick a tag that has
full coverage or add a short inline comment next to WEBKIT_VERSION noting which
platforms/variants were manually verified and any remaining gaps (e.g.,
baseline/debug/musl/android) so reviewers know what was checked.
🪄 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: 0bbce492-d645-45b6-bfd9-26dacca6db2c
📒 Files selected for processing (12)
.buildkite/ci.mjsscripts/build/deps/webkit.tssrc/jsc/ResolvedSource.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/ZigSourceProvider.cppsrc/jsc/bindings/headers-handwritten.hsrc/jsc/bindings/node/JSNodeHTTPServerSocket.cppsrc/runtime/bake/DevServer.rssrc/runtime/jsc_hooks.rssrc/runtime/server/mod.rssrc/watcher/Watcher.rs
- http: send_sync returned a picohttp::Response that borrowed an intentionally leaked HTTPResponseMetadata (status text, header buffer, and the boxed headers slice leaked once per sync CLI request). Return the owning HTTPResponseMetadata instead; a Deref to the inner Response keeps all call sites unchanged and Drop reclaims the buffers. Also stop copying the now-droppable Response back into the caller's AsyncHTTP in the sync handoff so no stale alias survives the metadata's drop. - server: Route::init takes a +1 ref on its HTMLBundle, but Route's Drop never released it (RefPtr has no Drop glue), stranding every bundle attached to a route past VM teardown. Release it explicitly, matching the original deinit order. - test: html-rewriter-leak's RSS workload exceeds its 15s timeout under ASAN instrumentation; raise the timeout to 90s on ASAN builds only, keeping the workload and threshold unchanged. - verify-baseline: the strpbrk flag on windows x64-baseline is a data-in-text artifact (an in-text switch jump-table entry decoding as xabort after a layout shift), not a real RTM instruction; widen the existing ceiling to [AVX, AVX2, RTM] with a comment documenting the evidence.
There was a problem hiding this comment.
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 `@src/http/lib.rs`:
- Around line 136-151: The public Deref impl and pub field
HTTPResponseMetadata.response currently expose a Copy Response<'static> that can
outlive the backing owned_buf and cause UB; remove the core::ops::Deref impl,
make the response field private, and stop using a fake 'static. Add safe
accessors on HTTPResponseMetadata (e.g., methods like status(&self) ->
StatusType, headers(&self) -> &[Header], body_slice(&self) -> &[u8] or a method
that returns a bun_picohttp::Response<'_> tied to &self) that return references
with lifetimes tied to &HTTPResponseMetadata so callers cannot copy a 'static
Response or outlive owned_buf; update any call sites that relied on Deref to use
these accessors and ensure Drop still frees owned_buf safely.
🪄 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: cad1df10-f406-4046-8ef4-16b8e0bcf717
📒 Files selected for processing (6)
scripts/verify-baseline-static/allowlist-x64-windows.txtsrc/http/AsyncHTTP.rssrc/http/lib.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/server_body.rstest/js/workerd/html-rewriter-leak.test.ts
`schedule_deinit`'s shutdown branch frees the server synchronously, which is UB under Stacked Borrows while any `&mut self` argument up the stack is still protected. Convert `deinit_if_we_can`, `schedule_deinit`, `on_request_complete`, `on_static_request_complete` (and the `ServerLike` trait method / `AnyServer` dispatch) to the `borrow = ptr` shape: raw `*mut Self` parameters with scoped reborrows that end before the freeing call, matching `Watcher::thread_main` and `NewServer::deinit`.
hasWebKitChanges() still looked for SetupWebKit.cmake, which no longer exists — the WEBKIT_VERSION pin moved to scripts/build/deps/webkit.ts — so WebKit bumps never got --jit-stress or the longer verify-baseline timeout.
`AnyServer::{on_request_complete, on_static_request_complete}` reach
`deinit_if_we_can`, which drops the server-owned `Box<DevServer>`; the
DevServer call sites passed `&mut self` pointing into that same box
(`dev.server.as_mut()`), leaving a protected reference live across the
free. `AnyServer` is `Copy`, so take `self` by value and copy the
handle out at the call sites.
2 participants
What
Adds a linux aarch64 ASAN (glibc) build + test lane to Buildkite, mirroring the existing linux x64 ASAN lane. PRs now get ASAN coverage on both Linux architectures.
buildPlatforms+={ os: linux, arch: aarch64, profile: asan }(amazonlinux 2023, same image as the other linux build lanes)testPlatforms+={ os: linux, arch: aarch64, profile: asan }(debian 13, 20 shards, 45 min timeout, sameASAN_OPTIONSas x64-asan)needsBaselineVerification()now skipsprofile === "asan"Like x64-asan, the new lane is PR-only: both
getPipelineOptions()and theincludeASANcheck ingetPipeline()already filter asan profiles on the main branch, and asan artifacts are already excluded from the release and binary-size steps.Why the
needsBaselineVerificationchangeWithout it, the new lane would inherit a
verify-baselinestep (the check matches all linux-aarch64 targets). That step downloads${triplet}-profile.zip, but asan link steps upload${triplet}-asan.zip(see the zip contract inscripts/build/ci.ts), so the step would fail on artifact download — and it would also try to run an ASAN-instrumented binary under qemu-user. ASAN builds are PR-only test artifacts that never ship; instruction-policy verification stays on the release lanes.What was already in place
The rest of the plumbing is profile/arch-generic and needed no changes:
getTestAgent()already had an aarch64+asan branch (r8g.2xlarge for ASAN shadow-memory headroom)getRustAgent()already sizes up to r8g.4xlarge for asanscripts/build/ci.ts) derivesbun-linux-aarch64-asan.zipfrom configrunner.node.mjskeys its ASAN timeout bump off-asan-in the step name, and the test harness detects ASAN via a runtime probebun-webkit-linux-arm64-asan.tar.gz) exists at the pinnedWEBKIT_VERSIONVerification
linux-aarch64-asan: build-cpp → build-rust → build-bun → test-bun) appears; full-content diff against the pre-change pipeline shows no other step changed, and all existing verify-baseline steps remain.mainbranch: zero asan steps (both arches filtered).--profile=ci-rust-only --os=linux --arch=aarch64 --abi=gnu --asan=on(the same cross-compile CI performs): builds clean; the resultinglibbun_rust.aobjects areELF 64-bit ARM aarch64with__asan_init/__asan_globals_registeredsymbols, confirming instrumented std via-Zsanitizer=address.ci.mjs— the new lane runs on this very PR.