Skip to content

Deduplicate cron job state machines and DNS pending-cache drains - #37363

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

Deduplicate cron job state machines and DNS pending-cache drains#37363
robobun wants to merge 11 commits into
claude/split/install-clifrom
farm/df8b29f2/split-cron-dns

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What this does

cron: CronRegisterJob and CronRemoveJob were ~600-line near-copies (same process spawn, stdout/stderr readers, exit handling, promise settlement). The shared state moves into CronJobCommon, and the two jobs implement a small trait supplying the per-job differences: the state sequence and accepts_nonzero_exit (remove tolerates crontab -l exit 1 on an empty crontab, and on Windows a missing task). The raw-pointer receiver discipline (this: *mut Self, local reborrows ended before any call that may free the job) is unchanged. dns: four PendingCacheKey types and five drain_pending_* functions collapse into one generic keyed by a NameKeyed/HasTail trait pair and two drain helpers (drain_chain_ok / drain_chain_err) that keep the original ensure_still_alive bracketing and per-global re-materialization.

Split 6 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

Main renamed bun_core::immutable::trim to bun_core::strings::trim (#33035); applied in the shared maybe_finished.

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: cron/ (155 pass). DNS suites need outbound network; they fail identically on the debug and release binaries in the container and pass on CI lanes (no DNS annotations on any of this PR's builds).
  • 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 the bug-hunting pass found no issues. Because it restructures a large amount of memory-safety-critical unsafe Rust — raw-pointer receivers with Stacked Borrows reborrow discipline in cron, intrusive-list walks with GC ensure_still_alive bracketing and heap::take ordering in DNS — across three platform cfg arms, a human pass would still be worthwhile.

What was reviewed:

  • cron: base()/base_mut() reborrows still end before every finish/maybe_finished call; Drop for CronJobCommon covers the process detach + tmp-file unlink the per-job Drop impls used to.
  • cron: accepts_nonzero_exit preserves the per-job asymmetry (remove tolerates Windows InstallingCrontab nonzero, register does not); spawn_bootout replacing CronRemoveJob::start_mac sets the same state.
  • dns: drain_chain_ok/drain_chain_err snapshot next before consuming each node and keep the original ensure_still_alive bracketing and per-global re-materialization; consume_head runs at the same point heap::take(key.lookup) did.
  • dns: PendingCacheKey<GetAddrInfoRequest> uses init_query (port+options+name hash) — the NameKeyed bound on plain init excludes it, so addr-info can't accidentally hash name-only.
Extended reasoning...

Overview

This PR deduplicates two independent subsystems as split 6/8 of a larger refactor:

  • src/runtime/api/cron.rs (~500 lines net): CronRegisterJob and CronRemoveJob had near-identical state machines, process-spawn plumbing, stdout/stderr readers, exit handling, and promise settlement. Shared fields move into CronJobCommon; the shared logic (maybe_finished, finish, spawn_cmd, start_linux, install_crontab, spawn_bootout) moves onto the CronJobBase trait as default methods over base()/base_mut() accessors. Per-job differences reduce to advance_state, accepts_nonzero_exit, and a Windows exit_err_override hook. The separate SpawnCmdTarget trait and its two impls are absorbed into CronJobBase.
  • src/runtime/dns_jsc/dns.rs (~300 lines net): four copy-pasted PendingCacheKey structs collapse into one generic PendingCacheKey<Req: HasTail>; five drain_pending_* functions collapse their intrusive-list walk into two helpers drain_chain_err / drain_chain_ok; get_or_put_into_pending_cache and get_or_put_into_resolve_pending_cache merge into one generic function.
  • src/runtime/dns_jsc/mod.rs: drops re-exports of the removed per-type PendingCacheKey modules.

Security risks

None identified. This is internal refactoring with no new user-facing surface, input parsing, or trust-boundary changes. The cron code already single-quotes/XML-escapes user-supplied paths and titles; those call sites are moved, not altered.

Level of scrutiny

High. The change is behavior-preserving in intent, but the code being moved is delicate:

  • Cron uses raw *mut Self receivers because finish() frees this mid-call-stack; every path relies on local reborrows ending (under NLL) before the freeing call. Moving field access from s.field to s.base_mut().field / unsafe { &mut *this }.base_mut() changes the borrow shape and needs re-verification per site.
  • DNS drain helpers walk intrusive singly-linked lists whose head is embedded in a heap request freed mid-walk, while holding a JSC JSValue alive across callbacks via ensure_still_alive and re-materializing it per JSGlobalObject. Getting the next-snapshot / heap::take / ensure_still_alive ordering wrong is a UAF or a GC-collected value.
  • Three-way platform cfg gating (linux / macOS / windows) means a mistake can compile on two targets and break the third.

Other factors

  • No bugs surfaced from the automated bug-hunting pass.
  • I spot-checked several behavior-preservation questions: the merged CronJobState enum drops the per-variant cfg gates that RegisterState had, which is harmless (unused variants); Drop for CronJobCommon subsumes both removed per-job Drop impls (extra CronRegisterJob fields like abs_path/schedule drop via field Drop); take_filtered_crontab changes the remove-path OOM message from "Out of memory" to "Out of memory building crontab" — cosmetic.
  • The PR adds no tests, relying on existing cron (155 tests) and DNS suites; the description notes DNS suites need network and were verified on CI lanes.
  • This is part of a stacked series that a maintainer (alii) asked to be split from #32023; the union is stated to be byte-identical to the already-CI'd parent head.

Given the volume of unsafe code being restructured across platform arms, this exceeds the "simple, mechanical, or obvious" bar for auto-approval even with a clean automated pass. Deferring to a human reviewer.

@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 5:05 AM PT - Aug 11th, 2026

@alii, your commit c5e7fe3 has some failures in Build #91738 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37363

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

bun-37363 --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