Simplify watcher run_tasks shutdown flow#760
Simplify watcher run_tasks shutdown flow#760ok300 wants to merge 4 commits intopubky:fix/task-panicfrom
run_tasks shutdown flow#760Conversation
Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com> Inline watcher loops and add panic non-hang tests Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com> test: consolidate panicking runners in missed_tick_skip tests Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com> test: consolidate panicking service test runners Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com> test: merge duplicate panicking runners in watcher tests Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com>
Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com>
ok300
force-pushed
the
copilot/investigate-nexuswatcher-avoid-spawn-loop
branch
from
3a0b240 to
7589c2b
Compare
There was a problem hiding this comment.
Pull request overview
Simplifies the watcher task shutdown/error-handling flow by inlining the processing loops in NexusWatcher::run_tasks (removing the spawn_processing_loop helper) and updating test utilities to exercise panic paths with a single configurable runner.
Changes:
- Inline the default/external homeserver processing loops directly in
NexusWatcher::run_tasksand removespawn_processing_loop. - Replace separate panicking runner test helpers with a single
PanickingRunner+PanicTarget, and add a timeout-based non-hanging assertion test. - Update existing shutdown-guard tests and utility exports to use the new runner.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
nexus-watcher/src/service/mod.rs |
Refactors run_tasks to spawn two explicit loops and removes the helper function. |
nexus-watcher/tests/service/utils/panicking_runner.rs |
Introduces PanicTarget/PanickingRunner and adds a timeout-based test to ensure panics don’t hang run_tasks. |
nexus-watcher/tests/service/utils/mod.rs |
Updates re-exports to the new panicking runner types. |
nexus-watcher/tests/service/shutdown_guard.rs |
Updates tests to use PanickingRunner and adjusts test documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -85,53 +84,67 @@ impl NexusWatcher { | |||
| /// Three parallel tasks are spawned: | |||
There was a problem hiding this comment.
The run_tasks doc comment says "Three parallel tasks are spawned", but the implementation now spawns only two tasks (default + external) and the shutdown-forwarder task has been removed. Update the comment to match the current behavior so callers/test readers aren’t misled.
| /// Three parallel tasks are spawned: | |
| /// Two parallel tasks are spawned: |
| } | ||
| /// Ensures `run_tasks` returns promptly with `Err` if a HS-processing task panics. | ||
| #[tokio_shared_rt::test(shared)] | ||
| async fn test_panicking_external_task_does_not_hang_run_tasks() -> Result<()> { |
There was a problem hiding this comment.
This test name implies only the external task panic case, but the body also asserts the default-homeserver panic path. Rename the test (or split into two tests) so the name matches what’s being verified.
| async fn test_panicking_external_task_does_not_hang_run_tasks() -> Result<()> { | |
| async fn test_panicking_tasks_do_not_hang_run_tasks() -> Result<()> { |
|
Superseded by #761 |
This PR attempts to simplify #757 by removing the need to use
spawn_processing_loop.