-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kody code-review Cross File high
Resource cleanup logic fragmented across two methods: globalExit() (lines 838-839) and deinit() (lines 1916-1933). The forever_timer deinitialization and gc_controller cleanup were moved from globalExit() to deinit(), but transpiler.deinit() and auto_killer.deinit() are now conditionally executed only on main thread in deinit(). This creates inconsistent cleanup semantics: globalExit() calls Zig__GlobalObject__destructOnExit() before deinit(), but critical resources are now cleaned up in deinit() after that call, potentially leaving dangling references during the global object destruction phase.
// Consolidate cleanup order in deinit() to ensure consistent semantics: pub fn deinit(this: *VirtualMachine) void { // Clean up event loop resources first if (this.eventLoop().forever_timer) |t| t.deinit(true); // Clean up garbage collector this.gc_controller.deinit(); // Main thread-specific cleanup if (this.is_main_thread) { this.transpiler.deinit(); this.auto_killer.deinit(); } // ... rest of cleanup ... } // In globalExit(), ensure cleanup order respects deinit() semantics: pub fn globalExit(this: *VirtualMachine) noreturn { bun.assert(this.isShuttingDown()); if (this.shouldDestructMainThreadOnExit()) { // Perform global object destruction AFTER resource cleanup this.deinit(); Zig__GlobalObject__destructOnExit(this.global); } bun.Global.exit(this.exit_handler.exit_code); }
Prompt for LLM
File src/bun.js/VirtualMachine.zig:
Line 1916:
In a Zig virtual machine implementation, resource cleanup logic has been refactored across two methods: globalExit() and deinit(). The original code in globalExit() performed cleanup in this order: (1) forever_timer deinit, (2) global object destruction, (3) transpiler deinit, (4) gc_controller deinit, (5) call deinit(). The new code moves forever_timer and gc_controller cleanup into deinit(), and conditionally moves transpiler and auto_killer cleanup to only execute on the main thread within deinit(). However, globalExit() still calls Zig__GlobalObject__destructOnExit() before deinit(), which means the global object is destroyed before these critical resources are cleaned up. This could leave dangling references or cause use-after-free issues if the global object destruction process accesses resources that haven't been properly deinitialized yet. Analyze whether the cleanup order should be reversed in globalExit() to ensure deinit() completes before global object destruction, or if there's a dependency that requires the current order.
Suggested Code:
// Consolidate cleanup order in deinit() to ensure consistent semantics:
pub fn deinit(this: *VirtualMachine) void {
// Clean up event loop resources first
if (this.eventLoop().forever_timer) |t| t.deinit(true);
// Clean up garbage collector
this.gc_controller.deinit();
// Main thread-specific cleanup
if (this.is_main_thread) {
this.transpiler.deinit();
this.auto_killer.deinit();
}
// ... rest of cleanup ...
}
// In globalExit(), ensure cleanup order respects deinit() semantics:
pub fn globalExit(this: *VirtualMachine) noreturn {
bun.assert(this.isShuttingDown());
if (this.shouldDestructMainThreadOnExit()) {
// Perform global object destruction AFTER resource cleanup
this.deinit();
Zig__GlobalObject__destructOnExit(this.global);
}
bun.Global.exit(this.exit_handler.exit_code);
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
valdis
commented
Mar 11, 2026
Claudes Review:
⏺ ---
Code Review: jarred/fix-small-leak
Branch: jarred/fix-small-leak vs main
Commits: 2 commits touching src/bun.js/VirtualMachine.zig
Overview
This PR fixes two related issues in VirtualMachine.deinit():
- Commit 1 (b7d3f5a) — Fixed a 72-byte memory leak in worker destruction by adding gc_controller.deinit() to deinit(), which was previously
only being called for the main thread path in globalExit(). - Commit 2 (008a99) — Refactored globalExit() to remove duplicate/inline cleanup calls, centralizing them inside deinit(). Added a
is_main_thread guard for transpiler.deinit() and auto_killer.deinit() since these should only run for the main thread VM.
Analysis
Correctness
- The refactor is sound. Previously globalExit() called forever_timer.deinit(), transpiler.deinit(), and gc_controller.deinit() explicitly
before calling this.deinit() — meaning workers that went through deinit() directly missed gc_controller.deinit() (the original 72-byte leak). - Moving cleanup into deinit() ensures workers get the same teardown path.
- The is_main_thread guard for transpiler.deinit() and auto_killer.deinit() makes sense — auto_killer docs show enabled: false by default and
is only meaningful for the main thread; transpiler is initialized per-VM but the guard suggests workers have different ownership semantics.
Potential concern: transpiler.deinit() gated on is_main_thread
- transpiler is initialized in both the main-thread and worker init paths (both init() overloads call Transpiler.init()). If workers need
their transpiler freed to avoid leaks, this guard would cause a worker transpiler leak. - The commit message says the original bug was 72 bytes (gc_controller), which is now fixed. But the transpiler gate needs verification — is
it safe to skip transpiler.deinit() for workers, or does the worker path deinit the transpiler elsewhere?
forever_timer moved from globalExit() to deinit()
- This is correct — the timer should be cleaned up whenever deinit() is called, not just from the globalExit() path. Workers hitting deinit()
directly now get timer cleanup too.
has_terminated = true set before transpiler.deinit()
- Minor ordering note: this.has_terminated = true is set at line 1928, then transpiler.deinit() happens after (line 1931). This seems
intentional to prevent re-entry, but worth confirming transpiler.deinit() doesn't rely on has_terminated being false.
Suggestions
- Verify worker transpiler lifecycle — Confirm that worker VMs have their transpiler cleaned up elsewhere (e.g., in the worker thread's own
teardown path), or add a comment explaining why transpiler.deinit() is main-thread-only. The current code is correct only if workers don't
leak transpiler. - Consider a comment on the is_main_thread guard — The guard is non-obvious. A short comment like // transpiler is owned by the main thread;
workers share/inherit it (or whatever the actual reason is) would improve maintainability. - Test coverage — There's no new test for the leak fix. Since this targets worker destruction, a regression test in test/js/bun/ that spawns
workers and verifies clean teardown (e.g., with BUN_DESTRUCT_VM_ON_EXIT) would be valuable, especially to prevent reintroduction.
Summary
The core fix (moving gc_controller.deinit() into deinit()) is correct and addresses the reported leak. The second commit's refactor is a
clean improvement. The main open question is whether worker VMs leak their transpiler under the new is_main_thread guard — that should be
verified before merging.
valdis
commented
Mar 11, 2026
Claude's code-review plugin
Code Review
Found 1 issue:
gc_controller.deinit() is called twice for every worker VM, causing a double-free of its uws timers.
web_worker.zig explicitly calls vm.gc_controller.deinit() on line 615 before calling vm.deinit() on line 616. The PR added an unconditional this.gc_controller.deinit() inside VirtualMachine.deinit() (line 1917), so
every worker teardown now double-frees gc_timer and gc_repeating_timer.
Relevant locations:
- src/bun.js/web_worker.zig L614–617 (existing explicit call before deinit())
- src/bun.js/VirtualMachine.zig L1915–1918 (new unconditional call inside deinit())
Fix: Either remove the explicit vm.gc_controller.deinit() call from web_worker.zig:615 (since deinit() now owns it), or add an is_main_thread guard around this.gc_controller.deinit() in VirtualMachine.deinit() to
match the pattern used for transpiler and auto_killer. The former is simpler.
Uh oh!
There was an error while loading. Please reload this page.
Pull Request Summary
Overview
This PR fixes a memory leak in the VirtualMachine deinitialization process by reorganizing the cleanup order and scope of resource deallocation.
Changes Made
Problem
The original code was deallocating certain resources (
forever_timer,transpiler, andgc_controller) in theglobalExit()function before callingdeinit(), which could lead to incomplete cleanup or double-deallocation issues.Solution
Reorganized the deinitialization sequence in the
deinit()method:Moved resource cleanup into
deinit():forever_timer.deinit()fromglobalExit()to the beginning ofdeinit()gc_controller.deinit()fromglobalExit()todeinit()Added thread-safety check:
transpiler.deinit()andauto_killer.deinit()in a conditional that only executes on the main thread (if (this.is_main_thread))Removed premature cleanup from
globalExit():globalExit()to avoid double-deallocationglobalExit()now only callsZig__GlobalObject__destructOnExit()anddeinit()Impact
This change ensures proper resource cleanup order and prevents memory leaks by:
deinit()method