src: disable calls to mimalloc when asan is enabled [v2]#24741
Draft
nektro wants to merge 19 commits into
Draft
Conversation
Collaborator
This comment was marked as duplicate.
This comment was marked as duplicate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.buildkite/ci.mjs (1)
567-597: Align LSAN--stepargument with ASAN build step to avoid missing artifactsFor
profile === "lsan",depends_onnow correctly points at the ASAN build step:const build_platform = profile === "lsan" ? { ...platform, profile: "asan" } : platform; depends.push(`${getTargetKey(build_platform)}-build-bun`);However,
argsstill usesgetTargetKey(platform)(LSAN) when forming--step:const args = [`--step=${getTargetKey(platform)}-build-bun`];If
scripts/runner.node.mjsuses--stepto resolve the build step key for artifacts, LSAN tests will look for a non-existent*-lsan-build-bunstep while actually depending on*-asan-build-bun. That mismatch is very likely to break LSAN runs.Refactor to reuse
build_platformfor both--stepanddepends_on:function getTestBunStep(platform, options, testOptions = {}) { const { os, profile } = platform; const { buildId, testFiles } = testOptions; - const args = [`--step=${getTargetKey(platform)}-build-bun`]; + const build_platform = profile === "lsan" ? { ...platform, profile: "asan" } : platform; + const args = [`--step=${getTargetKey(build_platform)}-build-bun`]; if (buildId) { args.push(`--build-id=${buildId}`); } - const depends = []; - if (!buildId) { - const build_platform = profile === "lsan" ? { ...platform, profile: "asan" } : platform; - depends.push(`${getTargetKey(build_platform)}-build-bun`); - } + const depends = []; + if (!buildId) { + depends.push(`${getTargetKey(build_platform)}-build-bun`); + }Optionally, if LSAN runs are expected to be as heavy as ASAN, consider giving them the extended timeout too:
- timeout_in_minutes: profile === "asan" || os === "windows" ? 60 : 30, + timeout_in_minutes: profile === "asan" || profile === "lsan" || os === "windows" ? 60 : 30,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.buildkite/ci.mjs(4 hunks)test/cli/bun.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:778-782
Timestamp: 2025-09-02T17:09:17.391Z
Learning: In Zig's bun codebase, the `isDefault` function in `src/allocators.zig` only needs to compare `allocator.vtable == c_allocator.vtable` rather than also checking the `ptr` field, because: (1) the codebase never creates multiple allocators that use `c_allocator.vtable` but have different `ptr`s, and (2) the default allocator vtable ignores the `ptr` field anyway, so any allocators sharing the same vtable would function identically.
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-16T02:17:35.237Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-19T02:52:37.412Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-30T21:52:04.707Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24212
File: src/cli/publish_command.zig:782-788
Timestamp: 2025-10-30T21:52:04.707Z
Learning: In the Bun codebase (oven-sh/bun), `enable_ansi_colors` flags are used to gate both ANSI color codes and Unicode box-drawing characters/emoji. This is the established pattern across the codebase.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-10-04T21:17:53.040Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23253
File: test/js/valkey/valkey.failing-subscriber-no-ipc.ts:40-59
Timestamp: 2025-10-04T21:17:53.040Z
Learning: In Bun runtime, the global `console` object is an AsyncIterable that yields lines from stdin. The pattern `for await (const line of console)` is valid and documented in Bun for reading input line-by-line from the child process stdin.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-09-12T22:30:48.490Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22613
File: src/cli/package_manager_command.zig:0-0
Timestamp: 2025-09-12T22:30:48.490Z
Learning: Commands like `bun info` and `bun whoami` that are promoted from pm-only subcommands to top-level commands use manual `bun.argv` parsing to detect direct invocation vs. pm invocation. This is different from regular commands that should use `ctx.positionals`.
Applied to files:
test/cli/bun.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
.buildkite/ci.mjs
🧬 Code graph analysis (2)
test/cli/bun.test.ts (1)
scripts/runner.node.mjs (1)
bunEnv(1132-1151)
.buildkite/ci.mjs (2)
scripts/utils.mjs (9)
profile(1683-1683)os(1679-1679)options(2240-2240)options(3089-3089)i(145-145)i(858-858)i(2244-2244)i(2504-2504)i(3058-3058)scripts/runner.node.mjs (8)
os(358-358)options(92-171)i(203-203)i(426-426)i(1698-1698)i(1870-1870)i(1877-1877)i(1970-1970)
🔇 Additional comments (3)
test/cli/bun.test.ts (2)
14-15: LGTM! Consistent environment handling across spawn calls.Spreading
bunEnvensures all spawn calls have the full standard test environment (FORCE_COLOR, BUN_DEBUG_QUIET_LOGS, etc.), improving test reliability and consistency.Also applies to: 34-35, 84-84
29-32: Past review comment addressed correctly.The undefined NO_COLOR case now explicitly sets
NO_COLOR: undefinedafter spreadingbunEnv, which properly unsets the environment variable and allows testing the "NO_COLOR unset" scenario (once TTY constraints are resolved)..buildkite/ci.mjs (1)
1138-1150: Test gating andtestFileswiring look consistentDestructuring
{ skipTests, forceTests, testFiles }and gating regular test steps with:if (!isMainBranch()) { if (!skipTests || forceTests) { // schedule tests... } }matches the expected semantics for commit/option flags and correctly passes
testFilesthrough togetTestBunStep. No issues from this change.
taylordotfish
suggested changes
Nov 18, 2025
taylordotfish
suggested changes
Nov 18, 2025
markovejnovic
left a comment
markovejnovic
left a comment
Contributor
There was a problem hiding this comment.
Approving for C++ code, would urge to rename bun_free to BUN_FREE, though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pulled out of #21663
realized I was fighting CI a bit more than i wanted to, so i decided to pull out just this change and then will pr the rest of the leak fixes i found along the way separately