refactor(jobs): adopt JoinSet supervision - #2164
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2164 +/- ##
===========================================
- Coverage 84.98% 84.69% -0.29%
===========================================
Files 351 351
Lines 29139 29657 +518
Branches 29139 29657 +518
===========================================
+ Hits 24764 25119 +355
- Misses 4020 4186 +166
+ Partials 355 352 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Several component shutdown paths treat an already-dropped halt receiver as a hard failure (and the health-check completion path can skip joining the shutdown controller), which can misreport normal shutdown and reduce shutdown reliability/observability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the application shutdown supervisor by migrating JobManager from a sequential Vec<JoinHandle> model to direct, named tokio::task::JoinSet ownership with a single process-wide shutdown deadline, and updates component/job launchers to return unspawned runners that report explicit completion vs cooperative cancellation.
Changes:
- Replaces
JobManagerhandle collection withJoinSetsupervision, named outcomes, and a shared shutdown deadline (plus a narrow legacy-handle registry). - Updates server/job starters and event listeners to return unspawned futures and report explicit
Completion/ComponentResultoutcomes. - Updates docs/specs and adds focused tests (including timed profiling shutdown regression) to validate cancellation, deadline escalation, and naming.
File summaries
| File | Description |
|---|---|
src/bootstrap/jobs/manager.rs |
Implements JoinSet-based supervision, named outcomes, and legacy-handle compatibility registry. |
src/app.rs |
Migrates wiring to JobManager::spawn and register_legacy; adds component_runner adapter for listener completion. |
src/console/profiling.rs |
Ensures timed profiling shutdown cancels and joins jobs; adds regression test. |
src/bootstrap/jobs/udp_tracker.rs |
Returns an unspawned component runner and uses NestedServerTask for cancellation-safe nested ownership. |
src/bootstrap/jobs/http_tracker.rs |
Returns an unspawned component runner; integrates cancellation-safe nested server ownership. |
src/bootstrap/jobs/tracker_apis.rs |
Returns unspawned REST API runner; integrates nested server ownership with explicit component outcomes. |
src/bootstrap/jobs/health_check_api.rs |
Returns unspawned health-check runner and supervises both runtime and shutdown-controller tasks. |
src/bootstrap/jobs/tracker_core.rs |
Converts tracker-core listeners to unspawned runners returning Completion. |
src/bootstrap/jobs/torrent_repository.rs |
Converts registry listener to an unspawned runner returning Completion. |
src/bootstrap/jobs/http_tracker_core.rs |
Converts HTTP core listener to an unspawned runner returning Completion. |
src/bootstrap/jobs/udp_tracker_core.rs |
Converts UDP core listener to an unspawned runner returning Completion. |
src/bootstrap/jobs/udp_tracker_server.rs |
Converts UDP server stats/banning listeners to unspawned runners returning Completion. |
packages/events/src/shutdown.rs |
Adds cross-package Completion enum for explicit listener termination state. |
packages/events/src/lib.rs |
Exposes the new shutdown module. |
packages/*/src/**/listener.rs (http-core, udp-core, udp-server, tracker-core, swarm-coordination-registry) |
Adds run_*_unspawned APIs and returns Completion from dispatch loops. |
packages/axum-health-check-api-server/src/server.rs |
Returns RunningServer { running, shutdown_controller } so callers own/abort/join both. |
packages/axum-health-check-api-server/src/environment.rs |
Updates environment runner to await both runtime and shutdown controller. |
src/AGENTS.md |
Updates architecture notes to reflect JoinSet supervision and the legacy boundary. |
docs/application-jobs.md |
Updates terminology and shutdown deadline semantics to match new supervision model. |
docs/issues/open/1586-use-joinset-in-jobmanager.md |
Removes superseded flat spec in favor of folder-style spec. |
docs/issues/open/1586-evaluate-job-manager-join-set/ISSUE.md |
Updates issue metadata and acceptance criteria to reflect completed work. |
docs/issues/open/1586-evaluate-job-manager-join-set/verification.md |
Records validation evidence and test coverage for the change. |
docs/issues/open/1586-evaluate-job-manager-join-set/implementation-retrospective.md |
Adds implementation retrospective and rationale. |
project-words.txt |
Adds “unspawned” for spell-checking. |
Cargo.toml / Cargo.lock |
Adds torrust-tracker-events dependency and enables Tokio test-util for tests. |
Review details
Suppressed comments (1)
src/bootstrap/jobs/health_check_api.rs:133
- When the health-check runtime future finishes first, the shutdown controller is currently neither joined nor checked for panics; it will be aborted in
Drop, which can silently discard failures and leave shutdown semantics inconsistent with the explicit ownership contract. Consider signalling shutdown and joining the controller before returningCompleted.
result = server_task.join() => {
let result = result
.map_err(|error| ComponentError::new(format!("health check API runtime task failed: {error}")))?;
result.map_err(|error| ComponentError::new(format!("health check API runtime task failed: {error}")))?;
Ok(ComponentCompletion::Completed)
- Files reviewed: 30/31 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5ddc501 to
8b62f4b
Compare
|
ACK 8b62f4b |
Summary
Adopt direct
tokio::task::JoinSetsupervision forJobManager, replacing the sequentialVec<Job>shutdown loop with named, concurrent outcomes under one process-wide deadline.Closes #1586
Changes
JobManager::spawn(name, future)without wrapper tasks around existing handles.Validation
./contrib/dev-tools/git/hooks/pre-commit.shlinter all