fix(file watcher): watchlist threadsafety#23682
Conversation
WalkthroughAdds explicit shutdown APIs for platform watchers, introduces optional mutex-based locking in WindowsWatcher batch processing and a shutdown method, changes Watcher.deinit to accept DeinitOpts (close_descriptors and join_thread), updates call sites, adds ImportWatcher.deinitAndJoin, and adds a concurrent hot-reload stress test. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/watcher/WindowsWatcher.zig (2)
246-255: In-loop flush dispatches under the outer lock; consider minimizing hold time
processWatchEventBatch(this, event_id, .dont_lock)avoids double-locking, but it still executesonFileUpdatewhile the outer lock is held. IfonFileUpdatecan indirectly cause watchlist ops on other threads, this increases contention and could deadlock if any path tries to acquire the same mutex.Consider:
- Flushing after finishing the current event’s
item_pathsscan (i.e., move the flush outside the inner loop), so you can release the outer lock before dispatching, and then callprocessWatchEventBatch(..., .lock).- Or split processing into “accumulate/merge” vs. “dispatch” phases: do the former while locked, then unlock and dispatch with
.lock.If re-entrancy is impossible by design, documenting that invariant would help future maintainers.
274-305: Document the locking contract and tighten internalsThe new
lockparameter and conditional guard are sensible. Two follow-ups:
- Add a brief contract comment: when
.dont_lockis passed, the caller must already holdthis.mutex. When.lockis passed,processWatchEventBatchwill acquire/release it aroundwriteTraceEvents/onFileUpdate.- Minor cleanup: after the
event_count == 0early-return,if (all_events.len == 0)is redundant onceall_eventsis derived directly fromevent_count.Example doc addition:
fn processWatchEventBatch( this: *bun.Watcher, event_count: usize, lock: enum { lock, dont_lock }, ) bun.sys.Maybe(void) { + // Locking contract: + // - .dont_lock: caller must already hold this.mutex for the duration of this call. + // - .lock: this function acquires/releases this.mutex around dispatch.And removing redundant check:
- if (all_events.len == 0) return .success;
📜 Review details
Configuration used: CodeRabbit UI
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)
src/watcher/WindowsWatcher.zig(5 hunks)test/cli/hot/hot.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/watcher/WindowsWatcher.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")
Files:
src/watcher/WindowsWatcher.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/hot/hot.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/hot/hot.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/hot/hot.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/hot/hot.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/hot/hot.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/hot/hot.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/hot/hot.test.ts
🧠 Learnings (10)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Do not set explicit test timeouts; Bun already has timeouts
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Use `tempDirWithFiles` (or `tempDir`) from `harness` for temporary directories/files
Applied to files:
test/cli/hot/hot.test.ts
🧬 Code graph analysis (1)
test/cli/hot/hot.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/watcher/WindowsWatcher.zig (2)
218-220: Correct: guard watchlist slice access with mutexTaking the lock before calling
this.watchlist.items(.file_path)fixes the TOCTOU on the multiarraylist slice. This addresses the invalid pointer risk described in the PR.Please confirm
onFileUpdate(or any code it calls) does not attempt to re-acquirethis.mutexon the same thread when invoked from the in-loop path, since the mutex is still held during event scanning. If it might, we should adjust to avoid re-entrancy deadlocks.
265-269: Final batch guarded with.lockGood: the final dispatch occurs with an internal lock when no outer lock is held, keeping the locking contract consistent across paths.
| it("does not crash under stress", () => { | ||
| // 1 second test, crashes about 50% of the time | ||
| using testDir = tempDir("watcher-stress-test", { | ||
| "index.js": ` | ||
| const TEST_DIR = "./crash-test"; | ||
|
|
||
| // Create a deeply nested module structure | ||
| for (let i = 0; i < 100; i++) { | ||
| for (let j = 0; j < 10; j++) { | ||
| const dir = \`\${TEST_DIR}/dir\${i}\`; | ||
| await Bun.write(\`\${dir}/module\${j}.ts\`, \`export default \${i * 10 + j};\`); | ||
| } | ||
| } | ||
|
|
||
| // Spawn multiple async tasks to stress the watcher | ||
| for (let worker = 0; worker < 10; worker++) { | ||
| (async () => { | ||
| while (true) { | ||
| // Dynamically import (adds to watchlist via ModuleLoader.zig) | ||
| const i = Math.floor(Math.random() * 100); | ||
| const j = Math.floor(Math.random() * 10); | ||
| try { | ||
| await import(\`\${TEST_DIR}/dir\${i}/module\${j}.ts\`); | ||
| } catch {} | ||
|
|
||
| // Modify files to trigger WindowsWatcher.watchLoopCycle | ||
| await Bun.write(\`\${TEST_DIR}/dir\${i}/module\${j}.ts\`, \`export default \${Date.now()};\`); | ||
| } | ||
| })(); | ||
| } | ||
|
|
||
| // Keep process alive | ||
| setTimeout(() => { | ||
| process.exit(0); | ||
| }, 1000); | ||
| `, | ||
| }); | ||
|
|
||
| const { stdout, stderr, exitCode } = Bun.spawnSync({ | ||
| cmd: [bunExe(), "--hot", "index.js"], | ||
| cwd: testDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| stdin: "ignore", | ||
| }); | ||
| expect(stdout.toString()).toBeEmpty(); | ||
| expect(stderr.toString().replace(/DEBUG: Reloading...\n/g, "")).toBeEmpty(); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Useful stress coverage; align with test guidelines and portability
- This is effective at reproducing the race. Nice use of
tempDir,bunExe, andbunEnv. - Two asks:
- The assertion pattern (“no output”,
exitCode === 0) is effectively an “absence of crash” assertion, which our test guidelines discourage. Consider rephrasing to assert positive invariants (e.g., bounded reload messages) or marking/skipping on non‑Windows where the bug isn’t relevant. Based on learnings. - Ensure parents are created before the
Bun.write(\${dir}/module${j}.ts`, ...)calls. IfBun.writedoes not guarantee recursive directory creation, addawait Bun.mkdir(dir, { recursive: true })` in the setup loop.
- The assertion pattern (“no output”,
Example tweak inside the generated index.js:
for (let i = 0; i < 100; i++) {
for (let j = 0; j < 10; j++) {
const dir = `${TEST_DIR}/dir${i}`;
+ await Bun.mkdir(dir, { recursive: true });
await Bun.write(`${dir}/module${j}.ts`, `export default ${i * 10 + j};`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("does not crash under stress", () => { | |
| // 1 second test, crashes about 50% of the time | |
| using testDir = tempDir("watcher-stress-test", { | |
| "index.js": ` | |
| const TEST_DIR = "./crash-test"; | |
| // Create a deeply nested module structure | |
| for (let i = 0; i < 100; i++) { | |
| for (let j = 0; j < 10; j++) { | |
| const dir = \`\${TEST_DIR}/dir\${i}\`; | |
| await Bun.write(\`\${dir}/module\${j}.ts\`, \`export default \${i * 10 + j};\`); | |
| } | |
| } | |
| // Spawn multiple async tasks to stress the watcher | |
| for (let worker = 0; worker < 10; worker++) { | |
| (async () => { | |
| while (true) { | |
| // Dynamically import (adds to watchlist via ModuleLoader.zig) | |
| const i = Math.floor(Math.random() * 100); | |
| const j = Math.floor(Math.random() * 10); | |
| try { | |
| await import(\`\${TEST_DIR}/dir\${i}/module\${j}.ts\`); | |
| } catch {} | |
| // Modify files to trigger WindowsWatcher.watchLoopCycle | |
| await Bun.write(\`\${TEST_DIR}/dir\${i}/module\${j}.ts\`, \`export default \${Date.now()};\`); | |
| } | |
| })(); | |
| } | |
| // Keep process alive | |
| setTimeout(() => { | |
| process.exit(0); | |
| }, 1000); | |
| `, | |
| }); | |
| const { stdout, stderr, exitCode } = Bun.spawnSync({ | |
| cmd: [bunExe(), "--hot", "index.js"], | |
| cwd: testDir, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| stdin: "ignore", | |
| }); | |
| expect(stdout.toString()).toBeEmpty(); | |
| expect(stderr.toString().replace(/DEBUG: Reloading...\n/g, "")).toBeEmpty(); | |
| expect(exitCode).toBe(0); | |
| }); | |
| // In index.js (inside the tempDir setup) | |
| for (let i = 0; i < 100; i++) { | |
| for (let j = 0; j < 10; j++) { | |
| const dir = `${TEST_DIR}/dir${i}`; | |
| await Bun.mkdir(dir, { recursive: true }); | |
| await Bun.write(`${dir}/module${j}.ts`, `export default ${i * 10 + j};`); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/cli/hot/hot.test.ts (2)
84-86: Test assertions align with crash regression testing.While the coding guidelines generally discourage "absence of crash" assertions, this test is a legitimate regression test for a specific Windows crash bug (issue #20916) that occurred ~50% of the time before the fix. The empty output and exit code 0 assertions are appropriate here to verify the crash no longer occurs.
Note: As suggested in a previous review, consider adding positive assertions (e.g., counting reload messages) or platform-specific skipping if you want to make the test more robust beyond "didn't crash."
42-47: Ensure parent directories exist before writing module files.As flagged in a previous review,
Bun.writemay not recursively create parent directories on all platforms. Explicitly create eachdir${i}directory before writing module files to prevent potential failures.Apply this diff inside the generated
index.jscontent to fix:for (let i = 0; i < 100; i++) { for (let j = 0; j < 10; j++) { const dir = `${TEST_DIR}/dir${i}`; + await Bun.mkdir(dir, { recursive: true }); await Bun.write(`${dir}/module${j}.ts`, `export default ${i * 10 + j};`); } }
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
test/cli/hot/hot.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/hot/hot.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/hot/hot.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/hot/hot.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/hot/hot.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/hot/hot.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/hot/hot.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/hot/hot.test.ts
🧠 Learnings (13)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Use `tempDirWithFiles` (or `tempDir`) from `harness` for temporary directories/files
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/cli/hot/hot.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Do not set explicit test timeouts; Bun already has timeouts
Applied to files:
test/cli/hot/hot.test.ts
🧬 Code graph analysis (1)
test/cli/hot/hot.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
| }); | ||
| const out = stdout.toString(); | ||
| const err = stderr.toString().replace(/DEBUG: Reloading...\n/g, ""); | ||
| console.log({ out, err }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statement.
The console.log({ out, err }) on Line 83 appears to be leftover debug output and should be removed before merging.
- console.log({ out, err });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log({ out, err }); |
🤖 Prompt for AI Agents
In test/cli/hot/hot.test.ts at line 83, there is a leftover debug console.log
statement "console.log({ out, err })"; remove that line so tests don't emit
unnecessary output and commit the change.
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 (4)
src/Watcher.zig (1)
120-147: Critical: Use-after-free vulnerability whenopts.join_threadis true.When
opts.join_threadis true andwatchloop_handle != null, the watcher thread callsallocator.destroy(this)at line 277 afterthreadMaincompletes. However,deinitreturns to the caller without destroyingthisin this code path, leaving the caller with a dangling pointer. Any subsequent access tothisresults in use-after-free.Required fixes:
Document the ownership transfer behavior in a function comment clearly stating that when
opts.join_threadis true, the caller must not accessthisafterdeinitreturns.Verify all platform implementations support
shutdown()— confirmed present in:
src/watcher/WindowsWatcher.zig:200src/watcher/KEventWatcher.zig:24src/watcher/INotifyWatcher.zig:228Consider whether this ownership model is intentional or if the design should be revised to prevent accidental use-after-free.
src/watcher/INotifyWatcher.zig (1)
325-366: Factor-out to processINotifyEventBatch is solid; consider wd→index mapSorting/merging then dispatching under lock is correct. For large watchlists, replace per-event indexOfScalar over eventlist_index with a small hash map (wd → index) maintained on watch/unwatch to cut batching cost.
src/watcher/WindowsWatcher.zig (2)
283-317: processWatchEventBatch: conditional locking + merge-before-dispatch is rightNo functional issues spotted. Keep the lock contract documented (caller locked vs. not).
5-5: Remove unused field to avoid confusionWindowsWatcher.mutex is never used; watchLoopCycle uses bun.Watcher.mutex. Please remove the unused struct field.
- mutex: Mutex = .{},
📜 Review details
Configuration used: CodeRabbit UI
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 (8)
src/Watcher.zig(2 hunks)src/bake/DevServer.zig(2 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/hot_reloader.zig(1 hunks)src/bun.js/node/path_watcher.zig(1 hunks)src/watcher/INotifyWatcher.zig(1 hunks)src/watcher/KEventWatcher.zig(1 hunks)src/watcher/WindowsWatcher.zig(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/watcher/KEventWatcher.zigsrc/watcher/INotifyWatcher.zigsrc/watcher/WindowsWatcher.zigsrc/bun.js/hot_reloader.zigsrc/bun.js/VirtualMachine.zigsrc/bake/DevServer.zigsrc/Watcher.zigsrc/bun.js/node/path_watcher.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/watcher/KEventWatcher.zigsrc/watcher/INotifyWatcher.zigsrc/watcher/WindowsWatcher.zigsrc/bun.js/hot_reloader.zigsrc/bun.js/VirtualMachine.zigsrc/bake/DevServer.zigsrc/Watcher.zigsrc/bun.js/node/path_watcher.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/hot_reloader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/node/path_watcher.zig
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/VirtualMachine.zigsrc/bake/DevServer.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (11)
src/Watcher.zig (2)
115-119: LGTM: Clear API improvement.The new
DeinitOptsstruct makes the deinitialization behavior explicit and allows for future extensibility without changing the function signature.
259-261: LGTM: Proper thread exit signaling.Clearing
watchloop_handleon normal completion is correct and allowsdeinitto determine if the thread has already exited. This prevents double-cleanup scenarios.src/bun.js/VirtualMachine.zig (1)
855-855: Verified:deinitAndJoin()correctly exists and handles allImportWatcherunion variants.The method implementation properly handles all three union cases (.hot, .watch, .none) with an exhaustive switch statement. It calls
deinit()withjoin_thread=truefor active watchers, ensuring the watcher thread is joined before process exit. The call inVirtualMachine.zig:855is correct.src/bake/DevServer.zig (1)
386-388: deinit opts on init failure: confirm no FD/resource leakUsing close_descriptors=false on the errdefer path is fine if init didn’t open platform descriptors yet; otherwise it risks leaking. Please confirm Watcher.init can’t leave open descriptors before start() succeeds, or flip close_descriptors to true here.
src/watcher/KEventWatcher.zig (1)
24-26: Straightforward shutdown aliasshutdown() delegating to stop() is correct and consistent with the new API surface.
src/watcher/INotifyWatcher.zig (2)
228-231: Linux shutdown path wakes waitersGood: stop() plus Futex.wake(max) ensures threads in Futex.waitForever() unblock promptly.
253-319: Bounded batching and safe name handlingNice defensive batching: checks against watch_events capacity and temp_name_list, and processes partial batches before continuing. Keeps indices consistent and avoids OOB.
Please run your new stress test with higher event rates to validate no drops when temp_name_list fills repeatedly.src/bun.js/hot_reloader.zig (1)
14-22: Add deinitAndJoin helper — good APIJoining the watcher thread via DeinitOpts is correct; this centralizes shutdown behavior for hot/watch modes.
src/bun.js/node/path_watcher.zig (1)
670-671: Update to struct-based deinit optsCallsite now matches the new API; looks good.
src/watcher/WindowsWatcher.zig (2)
200-207: Windows shutdown notification is correctPosting a completion with the watcher’s OVERLAPPED cleanly wakes the IOCP loop to exit. Ensure Watcher.deinit calls shutdown before joining.
227-229: Locking strategy fixes the watchlist raceTaking Watcher.mutex while building/dispatching the batch ensures the watchlist slice stays stable; using .dont_lock for in-loop flush and .lock for the final flush avoids double-lock. Good fix for the TOCTOU.
Also applies to: 255-264, 272-278
| .memory_visualizer_timer = if (dev.memory_visualizer_timer.state == .ACTIVE) | ||
| dev.vm.timer.remove(&dev.memory_visualizer_timer), | ||
| .graph_safety_lock = dev.graph_safety_lock.lock(), | ||
| .bun_watcher = dev.bun_watcher.deinit(true), | ||
| .bun_watcher = dev.bun_watcher.deinit(.{ .close_descriptors = true, .join_thread = false }), | ||
| .dump_dir = if (bun.FeatureFlags.bake_debugging_features) if (dev.dump_dir) |*dir| dir.close(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Keep join_thread=false here (avoids deadlock) — add a brief comment
Deinit is called while holding graph_safety_lock; join_thread=true could deadlock if the watcher thread is inside onFileUpdate and trying to lock the same. Current opts look correct; please add a short comment documenting this constraint to prevent future changes.
🤖 Prompt for AI Agents
In src/bake/DevServer.zig around lines 604 to 608, the deinit call sets
join_thread = false to avoid deadlock because deinit is run while holding
graph_safety_lock and the watcher thread may try to acquire that same lock
inside onFileUpdate; add a short inline comment next to the
.bun_watcher.deinit(.{ .close_descriptors = true, .join_thread = false })
explaining that join_thread must remain false to prevent deadlock when deinit is
invoked while holding graph_safety_lock.
2 participants
What does this PR do?
The main and worker threads append to
Watcher.watchlist, and they lock when doing it. Watcher thread was missing this lock before copying the pointer to the multiarraylist slice which would likely change causing 0xFFFFFFFFFFFFFFFF.fixes #20916
fixes ENG-21291
How did you verify your code works?
Manually, and added a test that crashes 50% of the time.