fix: explicitly close DevEngine to prevent coordinator task leaks under shared runtime#8568
Conversation
…ntime Co-authored-by: hyf0 <49502170+hyf0@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the HMR/dev-mode integration test harness to explicitly shut down DevEngine so that the background BundleCoordinator task doesn’t leak across tests when running under a shared Tokio runtime.
Changes:
- Replace
drop(dev_engine)with an awaiteddev_engine.close()call after each dev test run. - Add inline comments explaining why explicit shutdown is required under a shared runtime.
| // Explicitly close the dev engine to shut down the background coordinator task. | ||
| // Without this, the coordinator task would persist across tests under the shared runtime. | ||
| let _ = dev_engine.close().await; |
There was a problem hiding this comment.
dev_engine.close().await is only reached on the happy path; earlier unwrap()/assert! calls in this loop can panic and skip this cleanup, which can still leak the coordinator task into subsequent tests under a shared runtime. Consider structuring the loop so the close is run in a guaranteed cleanup path (e.g., a guard/"finally"-style pattern) so it executes even when the test logic panics.
| drop(dev_engine); | ||
| // Explicitly close the dev engine to shut down the background coordinator task. | ||
| // Without this, the coordinator task would persist across tests under the shared runtime. | ||
| let _ = dev_engine.close().await; |
There was a problem hiding this comment.
The cleanup error from dev_engine.close().await is fully suppressed here. Since close() also runs the closeBundle plugin hook and waits for the coordinator to finish, swallowing errors can make it hard to diagnose cleanup regressions (and could leave background tasks running if Close couldn’t be sent). Consider at least emitting the error to stdout/stderr (without failing the test) so CI logs capture unexpected close failures.
| let _ = dev_engine.close().await; | |
| if let Err(err) = dev_engine.close().await { | |
| eprintln!("dev_engine.close() failed during test cleanup: {err:?}"); | |
| } |
Benchmarks Rust |
With a shared Tokio runtime across fixture tests,
drop(dev_engine)no longer cleans up background tasks — theBundleCoordinatortask spawned byDevEngine::run()persists into subsequent tests since it requires an explicit shutdown signal.Change
drop(dev_engine)withdev_engine.close().awaitinrun_multiple_for_dev(crates/rolldown_testing/src/integration_test.rs)close()atomically marks the engine as closed, sendsCoordinatorMsg::Close, invokes thecloseBundleplugin hook, and awaits the coordinator handlelet _ = ...) since the coordinator may have already exited in error paths — cleanup failures should not obscure the original test failure💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.