Skip to content

Deduplicate auto_tick, IPC drain loops, async module dispatch, and entry-point waits - #37365

Open
robobun wants to merge 11 commits into
claude/split/install-clifrom
farm/df8b29f2/split-event-loop
Open

Deduplicate auto_tick, IPC drain loops, async module dispatch, and entry-point waits#37365
robobun wants to merge 11 commits into
claude/split/install-clifrom
farm/df8b29f2/split-event-loop

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What this does

jsc_hooks.rs: auto_tick and auto_tick_active differed only in whether the GC timer and rejected-promise pass run; merged into auto_tick<const ACTIVE: bool> with both tails preserved. ipc.rs: the three decode/dispatch loops shared their framing and error handling; extracted. AsyncModule.rs: the fulfill/reject paths for the three module kinds shared their resolve-and-drain body; extracted. VirtualMachine.rs: load_entry_point and load_entry_point_for_test_runner shared the HMR-aware wait loop; it becomes wait_for_entry_point_promise, with each caller keeping its own tail (the test-runner variant's trailing auto_tick + unwrap).

Split 7 of 8 from #32023, which alii asked to be broken up. Pure code motion and deletion; no intended behavior change. Stacked on claude/split/install-cli (merge order: foundations → install-cli → these, in any order among themselves).

Notes from rebasing against main

Return types follow main's per-crate error enums (#33909): crate::CrateResult.

Verification

  • Compiles in isolation against the base (cargo check), and the union of all eight splits is byte-identical to Deduplicate jsc bindings, runtime api, test matchers, and shell builtins #32023's head f29ce8fe4, which CI ran at 285/286 green (the one red was the fleet-wide napi.test.ts GC-timing flake on windows x64-baseline).
  • Locally on the parent head: spawn.ipc.test.ts (8 pass), spawn-streaming-stdout (pass), bun_test / expect suites (exercise the test-runner entry-point path), bake dev-server html.test.ts (10 pass, exercises the HMR wait loop).
  • No new tests: this adds no behavior, so the existing suites above are the regression coverage.

alii and others added 11 commits June 9, 2026 13:19
Resolved src/io/posix_event_loop.rs: #32594 added PollMemoryPressure to
is_registered(), which this branch moved into impl_file_poll_flag_methods;
carried the flag into the macro body.

Restored ArrayHashMap::get_index_mut: gained a caller in
react_compiler/collections.rs via #32504.
Resolved src/bun_core/string/mod.rs: #33909 switched return types to
crate::CrateResult<()> in write_pre_quoted_string and quote_for_json,
which this branch restructures into the const-generic
write_pre_quoted_string_inner; carried CrateResult<()> into both the
adapter and _inner signatures.

Resolved src/router/lib.rs and src/sourcemap/Chunk.rs: #33909 touched
return types inside blocks this branch deletes (orphaned router test
scaffolding, uncalled print_source_map_contents); kept the deletions
after re-verifying zero callers on current main.

Restored Node::find_last in src/collections/pool.rs: gained a test
caller via #33311.

@claude claude Bot left a comment

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.

I reviewed this PR and found no bugs — the extractions are line-for-line behavior-preserving. Because it touches the core event-loop tick (auto_tick), IPC message framing, and entry-point loading, a human look would still be worthwhile.

What was reviewed:

  • auto_tick<ACTIVE>: confirmed the deleted auto_tick_active body matches the surviving body with the three !ACTIVE gates skipped; the "no debug sleep-timer logging" note in the deleted doc comment was already stale (no such logging exists in either variant).
  • wait_for_entry_point_promise: the bool return correctly maps to only the non-watcher Rejected early-return; both callers' tails (unwrap_or vs auto_tick() + unwrap()) are preserved.
  • ipc.rs: the Err(err) catch-all is equivalent to the old exhaustive InvalidFormat | JSError | JSTerminated / OutOfMemory arms (5-variant enum, NotEnoughBytes handled above); the Windows advanced path's captured total_len equals adv_buf.len() throughout the loop (buffer never grows), so [slice_start..] is equivalent.
  • AsyncModule.rs: property-put order on the error object is preserved in both resolve_error and download_error (the range_data computation moved after the specifier put but is pure — the observable put sequence is unchanged).
Extended reasoning...

Overview

Four-file dedup refactor: (1) jsc_hooks.rs merges auto_tick + auto_tick_active into auto_tick<const ACTIVE: bool> with three if !ACTIVE gates around run_imminent_gc_timer and both handle_rejected_promises tails; (2) ipc.rs extracts three near-identical decode/dispatch loops into drain_json_messages, drain_advanced_messages, and a shared close_socket_on_decode_failure error tail; (3) AsyncModule.rs extracts the shared error-object construction / promise-rejection body of resolve_error and download_error into four helpers; (4) VirtualMachine.rs extracts the HMR-aware wait loop shared by load_entry_point and load_entry_point_for_test_runner.

I compared each extraction against the deleted inline code line by line. The only observable deltas are debug log-string labels (NotEnoughBytes3NotEnoughBytes/NotEnoughBytes2), which are dev-only. The IPC error catch-all (Err(err) vs the old exhaustive match) is equivalent because IPCDecodeError has exactly five variants and NotEnoughBytes is matched first. The Windows advanced-mode drain previously captured total_len once and sliced [slice_start..total_len]; the shared helper slices [slice_start..] — equivalent because the buffer never grows during the loop (documented, and the deleted comment already asserted total_len == adv_buf.len()). The AsyncModule helpers preserve the exact property-insertion order on the error instance in both call sites; the range_data computation moved relative to one put but is pure.

Security risks

None. No new inputs are parsed, no bounds/validation logic changes, no auth/crypto/permissions surface.

Level of scrutiny

High — auto_tick is the core per-tick event-loop path, IPC framing sits on the parent↔child message boundary, and load_entry_point gates every module execution and bun test run. A subtle divergence here would be broadly visible. That said, this is split 7 of 8 from a parent PR (#32023) whose union is byte-identical to a head that already ran 285/286 green in CI, and the diff is mechanical extraction with no control-flow reshaping beyond the const-generic gate.

Other factors

The PR ships no new tests (correct for pure code motion; existing suites named in the description exercise every changed path). No prior review comments on the timeline. The bug-hunting pass found nothing. I'm deferring rather than approving solely because of how central auto_tick and the entry-point wait loop are — a second pair of eyes on the const-generic gate placement is cheap insurance.

@robobun
robobun force-pushed the claude/split/install-cli branch from 2f5dd36 to 9ae3ff5 Compare August 11, 2026 02:09
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:05 AM PT - Aug 11th, 2026

@alii, your commit 801ded4 has some failures in Build #91739 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37365

That installs a local version of the PR into your bun-37365 executable, so you can run:

bun-37365 --bun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants