Skip to content

fix(watch): make close reentrant in event callbacks#9904

Merged
hyf0 merged 1 commit into
rolldown:mainfrom
hyf0:fix/watcher-close-event-deadlock
Jun 23, 2026
Merged

fix(watch): make close reentrant in event callbacks#9904
hyf0 merged 1 commit into
rolldown:mainfrom
hyf0:fix/watcher-close-event-deadlock

Conversation

@hyf0

@hyf0 hyf0 commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

  • add an out-of-band close notification so the coordinator remains responsive while awaiting consumer event callbacks
  • stop waiting for an in-flight event, change, or restart callback when that callback closes its own watcher, then run the normal complete close sequence
  • preserve sequential async listener behavior and the contract that await watcher.close() settles only after hooks, bundler cleanup, and the close event finish
  • document the re-entrant close lifecycle and add an end-to-end regression test

Root cause

The coordinator awaited the JavaScript event callback, while a callback awaiting watcher.close() waited for that same coordinator to finish:

coordinator -> JS callback -> watcher.close() -> coordinator

The queued WatcherMsg::Close could not be processed until the callback returned, so neither side could make progress.

Design

Watcher::close() now both queues the normal state-machine message and wakes a dedicated close notification. Consumer callback dispatch waits on the callback and that notification together. If close wins, only the Rust-side wait for the callback is dropped; the JavaScript promise continues running while the coordinator executes the existing close hooks, bundler cleanup, and close event. The original close promise then resolves and the callback resumes.

This differs from the request-only approach in #9475: a re-entrant close() does not resolve early before cleanup has completed.

Fixes #9462.

Tests

  • just build-rolldown
  • just build-rolldown-test-dev-server
  • RUST_BACKTRACE=1 ROLLDOWN_TEST=1 pnpm --filter rolldown-tests test:watcher (36 passed)
  • cargo test -p rolldown_watcher --lib (17 passed)
  • cargo clippy -p rolldown_watcher --all-targets -- --deny warnings
  • just lint-repo
  • vp check

@netlify

netlify Bot commented Jun 20, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit a2bec8f
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a39497bf451bf00089697e9

@hyf0 hyf0 force-pushed the fix/watcher-close-event-deadlock branch 2 times, most recently from bd4c4e1 to 868bb28 Compare June 20, 2026 12:16
@hyf0 hyf0 marked this pull request as ready for review June 21, 2026 16:04
@codspeed-hq

codspeed-hq Bot commented Jun 21, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing hyf0:fix/watcher-close-event-deadlock (a2bec8f) with main (e0beeb8)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

hyf0 commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

Merge activity

  • Jun 22, 8:31 AM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 22, 8:31 AM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jun 22, 8:32 AM UTC: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).
  • Jun 22, 1:45 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 22, 1:48 PM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jun 22, 1:50 PM UTC: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).
  • Jun 22, 1:56 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 22, 1:58 PM UTC: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).
  • Jun 22, 2:15 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 22, 2:15 PM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jun 22, 2:23 PM UTC: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).

@hyf0 hyf0 force-pushed the fix/watcher-close-event-deadlock branch from baf4e77 to a2bec8f Compare June 22, 2026 14:40
@hyf0 hyf0 merged commit 9f960eb into rolldown:main Jun 23, 2026
34 checks passed
@rolldown-guard rolldown-guard Bot mentioned this pull request Jun 24, 2026
shulaoda added a commit that referenced this pull request Jun 24, 2026
## [1.1.3] - 2026-06-24

### 🐛 Bug Fixes

- `defer_drop` crashes the browser main thread (#9942) by @shulaoda
- camel-case: correct camel case for nested values (#9933) by @kb019
- cli: display --help options in camelCase (#9941) by @IWANABETHATGUY
- preserve used re-exports under preserveModules (#9122) (#9934) by @IWANABETHATGUY
- watch: make close reentrant in event callbacks (#9904) by @hyf0
- git for windows treats symlink files as regular files (#9915) by @AliceLanniste
- dev: cancel pending full reload on build error (#9903) by @h-a-n-a
- chunking: pass plugin meta to codeSplitting groups name function (#9267) by @Kyujenius
- dev: serve assets emitted during HMR/lazy compile (vite#22596) (#9815) by @h-a-n-a
- release: dry-run step no longer publishes binding packages (#9866) by @Boshen

### 🚜 Refactor

- rolldown_common: model ModuleId as a classified Path/Virtual/Bare enum (#9927) by @Boshen
- remove unused LegacyModuleIdx (#9872) by @shulaoda
- remove unused StmtInfos::get_namespace_stmt_info (#9870) by @shulaoda
- remove unused Module::as_external_mut (#9871) by @shulaoda
- remove unused EcmaAst::is_body_empty (#9869) by @shulaoda
- drop dead is_css_module handling in resolve_dependencies (#9867) by @shulaoda
- drop redundant with_commonjs on cjs source type (#9868) by @shulaoda

### 📚 Documentation

- clarify on drafting PRs (#9952) by @h-a-n-a
- update contribution guidelines (#9944) by @fubhy
- note Rust crates don't follow semver in AGENTS.md (#9905) by @IWANABETHATGUY
- add feedback form (#9159) by @TheAlexLichter

### ⚡ Performance

- utils: avoid allocation in default_sanitize_file_name for clean names (#9928) by @Boshen
- binding: box once-per-build futures before spawn_future (#9864) by @Boshen
- utils: avoid wasted allocation in legitimize_identifier_name (#9926) by @Boshen
- rolldown: fuse the canonical-name dedup and insert in the renamer (#9900) by @Boshen
- rolldown: probe the name map once in ConflictResolver::resolve (#9899) by @Boshen
- cut two heap allocations from wrapped ESM init finalize (#9901) by @Boshen
- rolldown_plugin_vite_reporter: hoist invariant out_dir prefix out of reporter loop (#9873) by @shulaoda
- drop throwaway Vec in wrapped esm init stmt (#9878) by @shulaoda
- borrow owner_filename in build-import-analysis AddDeps (#9874) by @shulaoda

### 🧪 Testing

- cover preserveModules named export via namespace re-export (#6010) (#9937) by @IWANABETHATGUY

### ⚙️ Miscellaneous Tasks

- deps: update napi to v3.9.4 (#9954) by @shulaoda
- reduce noise from CODEOWNERS for trival changes (#9953) by @h-a-n-a
- deps: update mimalloc-safe to 0.1.64 (#9950) by @shulaoda
- deps: update rollup submodule for tests to v4.62.2 (#9931) by @rolldown-guard[bot]
- deps: test mimalloc-safe upstream-mimalloc switch in CI (#9930) by @shulaoda
- rolldown_plugin_vite_build_import_analysis: remove unused v2 code path (#9917) by @shulaoda
- rolldown_plugin_vite_manifest: remove unused is_enable_v2 code path (#9916) by @shulaoda
- rolldown_plugin_vite_asset_import_meta_url: remove unexposed native vite plugin (#9896) by @shulaoda
- rolldown_plugin_vite_asset: remove unexposed native vite plugin (#9895) by @shulaoda
- rolldown_plugin_vite_css_post: remove unexposed native vite plugin (#9894) by @shulaoda
- rolldown_plugin_vite_css: remove unexposed native vite plugin (#9893) by @shulaoda
- rolldown_plugin_vite_html_inline_proxy: remove unexposed native vite plugin (#9892) by @shulaoda
- rolldown_plugin_vite_html: remove unexposed native vite plugin (#9891) by @shulaoda
- deps: update github actions (#9909) by @renovate[bot]
- deps: update rust crate oxc_sourcemap to v8.0.2 (#9910) by @renovate[bot]
- deps: update npm packages (#9912) by @renovate[bot]
- deps: update github actions to v7 (#9913) by @renovate[bot]
- deps: update rolldown-plugin-dts to ^0.26.0 (#9897) by @renovate[bot]
- remove rolldown_filter_analyzer crate (#9865) by @Boshen

### ❤️ New Contributors

* @fubhy made their first contribution in [#9944](#9944)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: watcher.close() deadlocks when called inside its event callback

2 participants