diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 4edd96bd2..88fd46a30 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -25,6 +25,8 @@ For the full project context see the [root AGENTS.md](../AGENTS.md). | `profiling.md` | CPU and memory profiling with Valgrind / kcachegrind | | `release_process.md` | Branch strategy, versioning, and the release pipeline | | `adrs/` | Architectural Decision Records (ADRs) | +| `analysis/` | In-depth analysis of features, components, or aspects of the app | +| `research/` | External research on technologies, patterns, and best practices | | `issues/` | Issue specification documents linked to GitHub issues | | `refactor-plans/` | Refactor plan specifications (same lifecycle as issue specs) | | `pr-reviews/` | Notable PR review records and Copilot suggestion threads | diff --git a/docs/analysis/20260716-shutdown-process/README.md b/docs/analysis/20260716-shutdown-process/README.md new file mode 100644 index 000000000..3571008f6 --- /dev/null +++ b/docs/analysis/20260716-shutdown-process/README.md @@ -0,0 +1,447 @@ +--- +doc-type: analysis +status: draft +last-updated-utc: 2026-07-16 +semantic-links: + related-artifacts: + - src/main.rs + - src/app.rs + - src/container.rs + - src/bootstrap/jobs/manager.rs + - src/bootstrap/jobs/torrent_cleanup.rs + - src/bootstrap/jobs/activity_metrics_updater.rs + - src/bootstrap/jobs/health_check_api.rs + - src/bootstrap/jobs/http_tracker.rs + - src/bootstrap/jobs/udp_tracker.rs + - src/bootstrap/jobs/tracker_apis.rs + - src/bootstrap/jobs/torrent_repository.rs + - src/bootstrap/jobs/tracker_core.rs + - src/bootstrap/jobs/udp_tracker_core.rs + - src/bootstrap/jobs/udp_tracker_server.rs + - src/bootstrap/jobs/http_tracker_core.rs + - packages/axum-server/src/signals.rs + - packages/axum-health-check-api-server/src/server.rs + - packages/axum-http-server/src/server.rs + - packages/axum-rest-api-server/src/server.rs + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/src/server/states.rs + - packages/udp-server/src/server/mod.rs + - packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs + - packages/axum-server/src/custom_axum_server.rs + - docs/features/shutdown-process/README.md + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md + - docs/research/20260716-console-shutdown-patterns/README.md + related-issues: + - "https://github.com/torrust/torrust-tracker/issues/1488" + - "https://github.com/torrust/torrust-tracker/issues/1588" + - "https://github.com/torrust/torrust-tracker/issues/1477" + - "https://github.com/torrust/torrust-tracker/issues/1405" +--- + +# Shutdown Process Analysis + +## Overview + +This document analyses the current shutdown process of the Torrust Tracker application. +The tracker is a multi-service BitTorrent tracker composed of several concurrent jobs +(UDP servers, HTTP servers, REST API, Health Check API, event listeners, periodic cleanup +tasks). The shutdown process must coordinate stopping all these jobs cleanly. + +## 1. Entry Point + +The main entry point is in `src/main.rs`: + +```rust +#[tokio::main] +async fn main() { + let (_app_container, jobs) = app::run().await; + + tokio::select! { + _ = tokio::signal::ctrl_c() => { + tracing::info!("Torrust tracker shutting down ..."); + + jobs.cancel(); + + jobs.wait_for_all(Duration::from_secs(10)).await; + + tracing::info!("Torrust tracker successfully shutdown."); + } + } +} +``` + +**Key observations:** + +- Only `SIGINT` (Ctrl+C) is handled at the top level. +- `SIGTERM` is **not** handled in `main.rs` (though it is handled internally by servers — see §3). +- The shutdown sequence is: `cancel()` → `wait_for_all(Duration::from_secs(10))`. +- The 10-second grace period is a **hardcoded magic number**. +- Jobs are waited **sequentially** (one by one), each with the same timeout. + +## 2. The `JobManager` (`src/bootstrap/jobs/manager.rs`) + +The `JobManager` is a central coordinator that holds: + +- A `Vec` — each job has a `name` and a `JoinHandle<()>`. +- A shared `CancellationToken`. + +### 2.1 Job Cancellation + +```rust +pub fn cancel(&self) { + self.cancellation_token.cancel(); +} +``` + +Cancelling the `CancellationToken` signals all jobs that were registered with a +`new_cancellation_token()`. However, not all jobs use this token (see §2.2.2). + +### 2.2 Waiting for All Jobs + +```rust +pub async fn wait_for_all(mut self, grace_period: Duration) { + for job in self.jobs.drain(..) { + // ... waited sequentially with timeout(grace_period, job.handle) + } +} +``` + +**Key observations:** + +- Jobs are waited **sequentially**, not concurrently. +- Each job gets the **same** grace period timeout. +- If a job times out, only a warning is logged — no forced abort. +- The order of waiting is the order jobs were pushed (currently: event listeners first, + then servers, then periodic tasks, then API servers). + +## 3. Three Shutdown Mechanisms (Inconsistent) + +The tracker uses **three different mechanisms** to signal shutdown to its various jobs. +This inconsistency is a primary area for improvement. + +### 3.1 `CancellationToken` (used by event listeners) + +Used by statistics event listeners: + +- `swarm_coordination_registry` event listener +- `tracker_core` event listener +- `http_core` event listener +- `udp_core` event listener +- `udp_server` stats event listener +- `udp_server` banning event listener + +These jobs receive a `CancellationToken` from the `JobManager` and check `token.cancelled()` +in their main loop. They respond to `jobs.cancel()`. + +### 3.2 Direct `tokio::signal::ctrl_c()` (used by periodic jobs) + +Used by: + +- **Torrent cleanup** (`src/bootstrap/jobs/torrent_cleanup.rs`) +- **Activity metrics updater** (`packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs`) + +These jobs listen for `tokio::signal::ctrl_c()` directly inside a `tokio::select!` in their +own loop. They **do not** respond to `jobs.cancel()` — they have no connection to the +`CancellationToken`. However, they will still stop when Ctrl+C is pressed because the signal +fires globally. + +### 3.3 Oneshot Channel `Halted` (used by server instances) + +Used by all server types: + +- **UDP tracker** (`packages/udp-server/src/server/launcher.rs`) +- **HTTP tracker** (`packages/axum-http-server/src/server.rs`) +- **REST API** (`packages/axum-rest-api-server/src/server.rs`) +- **Health Check API** (`packages/axum-health-check-api-server/src/server.rs`) + +Each server is started with a `oneshot::Receiver`. The server task awaits +`shutdown_signal_with_message(rx_halt)` which internally calls `shutdown_signal(rx_halt)`. + +The `shutdown_signal()` function (from `torrust_server_lib::signals`) is a `tokio::select!` +between: + +1. The halt channel (receiving `Halted::Normal` from the main process) +2. The `global_shutdown_signal()` (Ctrl+C or SIGTERM) + +This means **all servers will also shut down if any other server's halt signal fires** — +because the `global_shutdown_signal()` is shared across all tokio tasks and resolves once +for all of them. + +## 4. The `global_shutdown_signal()` (`torrust_server_lib::signals`) + +```rust +pub async fn global_shutdown_signal() { + let ctrl_c = async { + tokio::signal::ctrl_c().await.expect("..."); + }; + + #[cfg(unix)] + let terminate = async { + tokio::signal::unix::signal(SignalKind::terminate()) + .expect("...") + .recv() + .await; + }; + + #[cfg(not(unix))] + let terminate = std::future::pending::<()>(); + + tokio::select! { + () = ctrl_c => { ... }, + () = terminate => { ... } + } +} +``` + +**Key observations:** + +- Handles both `SIGINT` (Ctrl+C) and `SIGTERM` (Unix) — but only inside servers. +- The `global_shutdown_signal()` is used **inside** each server's `shutdown_signal()`. +- This means that when the user presses Ctrl+C: + 1. `main.rs` catches it and calls `jobs.cancel()` + `jobs.wait_for_all()`. + 2. Each server ALSO catches it independently via `global_shutdown_signal()`. + 3. This creates a **double-signal** scenario — servers react to Ctrl+C both via the + halt channel **and** via the global signal. +- The `global_shutdown_signal()` is **not** used in `main.rs` — only `ctrl_c()` is. + +## 5. Graceful Shutdown Per Server + +### 5.1 Axum Servers (HTTP Tracker, REST API, Health Check API) + +All three Axum-based servers use the same `graceful_shutdown` function from +`packages/axum-server/src/signals.rs`: + +```rust +pub async fn graceful_shutdown(handle, rx_halt, message, address) { + shutdown_signal_with_message(rx_halt, message).await; + + let grace_period = Duration::from_secs(90); + let max_wait = Duration::from_secs(95); + + handle.graceful_shutdown(Some(grace_period)); + + loop { + // Poll connection count every second + // Break when: connections == 0 OR max_wait elapsed + } +} +``` + +**Key observations:** + +- Grace period is **90 seconds** with a **95-second** upper bound. +- The 10-second delta allows for the `graceful_shutdown` to complete before the loop times out. +- Connections are drained actively — the server waits for active HTTP connections to finish. +- **BUT**: `main.rs` only waits **10 seconds** per job. The Axum 90s grace period is never + reached because `JobManager` times out after 10s. The Axum shutdown runs in a spawned task + and will complete independently, but the main process will have already exited by then. + +### 5.2 UDP Server + +The UDP server (`packages/udp-server/src/server/launcher.rs`) has a different approach: + +```rust +// The halt task waits for shutdown signal +let halt_task = tokio::task::spawn(shutdown_signal_with_message(rx_halt, ...)); + +select! { + _ = running => { ... }, + _ = halt_task => { ... } +} +stop.abort(); // Force-abort the main loop +``` + +**Key observations:** + +- The UDP server **cannot** drain connections gracefully — it simply aborts the main loop. +- There is no connection draining mechanism for UDP. +- After abort, `tokio::task::yield_now().await` gives other tasks a chance to complete. + +## 6. Startup and Shutdown Architecture + +The overall startup sequence in `src/app.rs`: + +```rust +pub async fn start(config, app_container) -> JobManager { + warn_if_no_services_enabled(config); + load_data_from_database(config, app_container).await; + start_jobs(config, app_container).await +} +``` + +Jobs are started in this order: + +1. Event listeners (swarm, core, http-core, udp-core, udp-server stats, udp-server banning) +2. UDP tracker instances +3. HTTP tracker instances +4. Torrent cleanup (periodic) +5. Peers inactivity update (periodic) +6. REST API +7. Health Check API + +The shutdown (waiting) order follows the push order — jobs pushed first are +waited first: + +1. Event listeners (swarm, core, http-core, udp-core, udp-server stats, banning) +2. UDP tracker instances +3. HTTP tracker instances +4. Torrent cleanup +5. Peers inactivity update +6. REST API +7. Health Check API (waited last) + +> This was confirmed experimentally in §8.2. + +## 7. Identified Issues + +### 7.1 Inconsistent Shutdown Mechanisms + +Jobs use three different mechanisms (`CancellationToken`, direct `ctrl_c`, halt channel). +This makes it hard to reason about the shutdown process and hard to add new job types. + +### 7.2 Torrent Cleanup and Activity Metrics Ignore `CancellationToken` + +These two jobs listen for `ctrl_c` directly instead of using the shared `CancellationToken`. +They will still stop on Ctrl+C because the signal fires globally, but they won't respond +to `jobs.cancel()`. + +### 7.3 Grace Period Mismatch + +The `JobManager` waits **10 seconds per job sequentially**, while Axum servers have a +**90-second grace period**. This means: + +- The main process will likely timeout before the Axum servers finish draining connections. +- The Axum graceful shutdown spawned task continues running after the main process exits. + +### 7.4 No SIGTERM in `main.rs` + +Only `SIGINT` (Ctrl+C) is handled at the top level. SIGTERM (used by container orchestrators +like Docker/Podman) is only handled inside each server via `global_shutdown_signal()`. If +a container runtime sends SIGTERM, the servers will react, but `jobs.cancel()` will never +be called, and `jobs.wait_for_all()` will never execute. + +### 7.5 Sequential Job Waiting + +Jobs are waited one by one. If the first job (e.g., Health Check API) takes the full 10 +seconds, all subsequent jobs get less time. This could be improved by waiting concurrently +with a shared timeout. + +### 7.6 Hardcoded Grace Periods + +Both the `JobManager`'s 10-second timeout and the Axum's 90-second grace period are +hardcoded magic numbers. They are not configurable. + +### 7.7 Double-Signal on Ctrl+C + +When Ctrl+C is pressed: + +1. `main.rs` catches it and starts the shutdown sequence. +2. Each server's `shutdown_signal()` also catches it via `global_shutdown_signal()`. +3. This creates a race: the main process calls `jobs.cancel()` and sends `Halted` via the + channel, but the servers may already be shutting down from the global signal. + +### 7.8 UDP Server Has No Graceful Shutdown + +The UDP server simply aborts its main loop. There is no mechanism to wait for in-flight +UDP requests to complete before stopping. + +### 7.9 Profiling Binary Shutdown Difference + +The profiling binary in `src/console/profiling.rs` has a different shutdown path: + +- It does not call `jobs.cancel()` before `jobs.wait_for_all()`. +- It uses a timed shutdown instead of Ctrl+C. +- This means the `CancellationToken` is never triggered for the profiling binary. + +> **Note**: The profiling binary is a developer-only tool for profiling +> (valgrind/callgrind), not a user-facing entry point. It is out of scope for +> the shutdown process feature (EPIC #1488) and can be updated independently +> as needed. + +## 8. Experimental Validation + +The findings in this analysis were validated by running the tracker locally on +2026-07-16 using the default development configuration (`cargo run`). + +### 8.1 SIGTERM Test + +**Command**: `kill ` (sends `SIGTERM` by default) + +**Result**: The tracker **kept running**. No shutdown sequence was initiated. +The logs continued normally with periodic metrics output and torrent cleanup +tasks. The process had to be force-killed with `kill -9`. + +**Conclusion**: Confirms that `SIGTERM` is not handled at the top level. The +`main.rs` entry point only listens for `SIGINT`. This is the most critical gap +for container orchestration and AI agents. + +### 8.2 SIGINT Test (Ctrl+C simulation) + +**Command**: `kill -INT ` (sends `SIGINT`, same as Ctrl+C) + +**Result**: The tracker shut down gracefully. The logs showed: + +```text +1. `main.rs` caught SIGINT and called `jobs.cancel()` + `jobs.wait_for_all()`. +2. JobManager waited for each job sequentially with a 10s timeout. +3. All jobs completed gracefully within the timeout. +4. Final message: "Torrust tracker successfully shutdown." +``` + +**Key observation — double-signal confirmed**: The logs also showed that each +server's `global_shutdown_signal()` caught the same SIGINT independently: + +```text +WARN torrust_server_lib::signals: caught interrupt signal (ctrl-c), halting... +``` + +This confirms the **double-signal problem** identified in §7.7: both `main.rs` +and each server's internal signal handler catch the same Ctrl+C. + +**Key observation — shutdown order**: The JobManager waited jobs in this order: + +1. Event listeners (swarm, tracker-core, http-core, udp-core, udp-server stats, + udp-server banning) +2. UDP instances (6868, 6969) +3. HTTP instances (7070, 7171) +4. Torrent cleanup +5. Peers inactivity update +6. HTTP API +7. Health Check API + +All completed within the 10s timeout. + +### 8.3 Graceful shutdown works + +Despite the signal-handling issues, the internal shutdown mechanism is solid: + +- Axum servers drain connections (the log shows "All connections closed"). +- Event listeners respect the `CancellationToken`. +- The `JobManager` reports each job's status during shutdown. + +### 8.4 `cargo run` vs actual binary PID + +When starting with `cargo run`, the PID of `cargo` itself is different from the +PID of the final `torrust-tracker` binary. This means: + +- `kill ` sends the signal to `cargo`, not the tracker. +- The tracker binary runs as a child process. +- If `cargo` is killed, the tracker becomes orphaned but continues running. + +This is relevant for development workflows but not for production deployments +(where the binary runs directly or via a container entrypoint). + +## 9. Summary Table + +| Aspect | Current Implementation | Status | +| ---------------------------- | ----------------------------------------------------- | ---------------------------------- | +| Top-level signal handling | Only `SIGINT` in `main.rs` | ⚠️ Missing `SIGTERM` | +| Server shutdown mechanism | Oneshot channel `Halted` + `global_shutdown_signal()` | ✅ Functional | +| Event listener shutdown | `CancellationToken` from `JobManager` | ✅ Functional | +| Periodic job shutdown | Direct `tokio::signal::ctrl_c()` | ⚠️ Inconsistent | +| Axum connection draining | 90s grace period, polls connection count | ✅ Functional but timeout mismatch | +| UDP connection draining | None — `abort()` | ❌ Not graceful | +| Job waiting strategy | Sequential per-job timeout | ⚠️ Sequential + timeout mismatch | +| Grace period configurability | Hardcoded everywhere | ❌ Not configurable | +| Double-signal on Ctrl+C | Both `main.rs` and servers catch it | ⚠️ Potential race | diff --git a/docs/analysis/AGENTS.md b/docs/analysis/AGENTS.md new file mode 100644 index 000000000..3bf3a16c7 --- /dev/null +++ b/docs/analysis/AGENTS.md @@ -0,0 +1,48 @@ +# `docs/analysis/` — Analysis Documents + +This directory contains analysis documents that study a concrete feature, component, or +aspect of the application in depth. Analyses are typically produced **before** defining a +refactoring plan, introducing a new feature, or making architectural decisions. + +## Purpose + +An analysis document answers questions like: + +- How does this part of the system work today? +- What are the pain points, risks, and gaps? +- What options exist for improvement? +- What is the current state of the code? + +Analyses are the **input** to further work: they feed into feature definitions, EPIC +specifications, issue specs, and ADRs. + +## Timestamp Prefix Convention + +Analysis folders use a **timestamp prefix** like ADRs to make it clear when the analysis +was written: + +```text +docs/analysis/ +├── AGENTS.md +├── 20260716-shutdown-process/ +│ └── README.md +└── ... +``` + +## Lifecycle + +- **Analyses may become outdated** if the object of their analysis has changed since they + were written. Always check the timestamp and verify against the current code before + relying on an old analysis. +- **Analyses may stay relevant** if the code has not changed in the area they study. +- **Old analyses can be cleaned up** when they are no longer relevant. The timestamp + prefix makes it easy to identify which ones are older. +- **Consider reviewing** analyses older than 6 months before using them as a basis for + decisions. + +## Related + +- [Research documents](../research/) — external investigations of technologies and patterns +- [Feature definitions](../features/) — product-oriented descriptions of desired features +- [Issue specs](../issues/) — concrete task breakdowns linked to GitHub issues +- [ADRs](../adrs/) — architectural decision records diff --git a/docs/features/shutdown-process/README.md b/docs/features/shutdown-process/README.md new file mode 100644 index 000000000..ace815ec1 --- /dev/null +++ b/docs/features/shutdown-process/README.md @@ -0,0 +1,382 @@ +--- +doc-type: feature +status: draft +last-updated-utc: 2026-07-16 +semantic-links: + related-artifacts: + - docs/analysis/20260716-shutdown-process/README.md + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/research/20260716-console-shutdown-patterns/README.md +--- + +# Feature: Shutdown Process + +## Status + +Draft — under analysis. No implementation started. + +## Summary + +Make the Torrust Tracker conform to the **Unix and container process lifecycle +contracts** — the well-proven, widely-adopted standards that govern how a +long-running service is expected to stop. The tracker should respond correctly +to every conventional shutdown mechanism (`kill`, Ctrl+C, `docker stop`, +`systemctl stop`, Kubernetes pod termination) and behave predictably for human +operators, container runtimes, and automated agents alike. + +This is not about adding new features. It is about **not surprising the operator** +— implementing the behavior they already expect based on decades of Unix and +container conventions. + +## The Problem in One Sentence + +`kill ` — the most basic Unix way to ask a process to stop — silently does +nothing to the Torrust Tracker today. The tracker ignores `SIGTERM`. + +> **A note on `kill`**: Despite its name, `kill` does not force-terminate a +> process by default. It sends `SIGTERM` (signal 15), which is simply a polite +> request to stop gracefully. The word "kill" sounds brutal, but the mechanism +> is not — it is the standard Unix way of asking a process to exit. The truly +> forceful command is `kill -9` (SIGKILL), which cannot be caught or ignored. +> The tracker currently treats `kill ` as if it were silent — surprising +> and wrong. + +## Motivation + +The current shutdown process has several pain points: + +1. **Container orchestration**: Docker/Podman send `SIGTERM` by default, but the + main entry point only handles `SIGINT` (Ctrl+C). Containers may be forcefully + killed after the orchestrator's own timeout. +2. **Silent hangs**: When a job does not finish in time, only a generic warning + is logged — operators cannot tell which job is blocking shutdown. +3. **Inconsistent behavior**: Different jobs use different shutdown mechanisms + (`CancellationToken`, direct `ctrl_c` listener, oneshot channel). Some jobs + ignore the central `JobManager` entirely. +4. **Timeout mismatch**: The `JobManager` waits 10 seconds per job sequentially, + while Axum servers have a 90-second graceful shutdown. The main process may + exit before servers finish draining connections. +5. **No graceful UDP shutdown**: The UDP server simply aborts its main loop. + In-flight requests are dropped without notice. +6. **Hardcoded timeouts**: Grace periods are magic numbers scattered across the + codebase with no configuration surface. + +## The Contracts We Are Implementing + +These are established, well-proven standards. We are not inventing anything new — +we are bringing the tracker into compliance with what every process manager, +container runtime, and operator already expects. + +### Unix Process Signal Contract + +> A well-behaved Unix process catches `SIGTERM` and shuts down gracefully. +> `SIGKILL` is the last resort when a process refuses to stop. + +| Signal | Meaning | Expected tracker behavior | +| -------------- | ----------------------------- | -------------------------------------------------------- | +| `SIGTERM` (15) | "Please stop gracefully" | Start shutdown sequence, drain connections, exit cleanly | +| `SIGINT` (2) | "User pressed Ctrl+C" | Same as `SIGTERM` — start graceful shutdown | +| `SIGKILL` (9) | "Stop immediately, no choice" | Immediate termination by OS — cannot be caught | + +**Currently**: `SIGTERM` is ignored. `kill ` has no effect. +**After fix**: `kill ` triggers the same graceful shutdown as Ctrl+C. + +### Docker / Podman Container Stop Contract + +> `docker stop ` sends `SIGTERM` and waits up to 10 seconds (the +> `--time` grace period). If the process has not exited, it sends `SIGKILL`. + +```bash +# What docker stop does internally: +kill -TERM # sends SIGTERM, waits up to 10s +kill -KILL # sends SIGKILL if process is still running +``` + +**Currently**: `docker stop torrust-tracker` sends `SIGTERM`, which is ignored. +After the 10s timeout, Docker force-kills the container with `SIGKILL`. +**After fix**: `docker stop torrust-tracker` triggers graceful shutdown and the +container exits cleanly within the grace period — no force kill needed. + +### Kubernetes Pod Termination Contract + +> When a pod is deleted or evicted, Kubernetes sends `SIGTERM` and waits for +> `terminationGracePeriodSeconds` (default 30s) before sending `SIGKILL`. + +**Currently**: Pod termination force-kills the tracker every time. +**After fix**: The tracker drains active connections and exits cleanly. + +### Systemd / Init System Contract + +> `systemctl stop ` sends `SIGTERM` and waits for `TimeoutStopSec` +> (default 90s on most distros) before sending `SIGKILL`. + +**Currently**: `systemctl stop torrust-tracker` has no effect — the tracker +keeps running. After `TimeoutStopSec`, systemd force-kills it. +**After fix**: `systemctl stop torrust-tracker` gracefully shuts down the +tracker as expected. + +### The Principle of Least Surprise + +Any operator, developer, or automated agent interacting with the tracker will +try the most natural stop mechanisms first. They should not be surprised: + +```bash +# These all SHOULD work — and currently DON'T (except Ctrl+C): +kill # sends SIGTERM — currently ignored ❌ +kill -TERM # sends SIGTERM — currently ignored ❌ +docker stop # sends SIGTERM — currently ignored ❌ +podman stop # sends SIGTERM — currently ignored ❌ +systemctl stop # sends SIGTERM — currently ignored ❌ + +# This works but is less standard: +kill -INT # sends SIGINT — works ✅ + +# This is the last resort and should never be needed: +kill -9 # SIGKILL — force kill, no cleanup ❌ +``` + +After implementing this feature, all the lines marked ❌ above will work. + +## User Value + +| Stakeholder | Value | +| ----------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Operators / DevOps** | Every standard stop mechanism works as expected. No more needing to know the tracker's quirks. Container orchestrators can rely on `SIGTERM` working correctly. | +| **Developers** | Single, consistent shutdown pattern for all job types. Easy to add new jobs with correct shutdown behavior. | +| **AI agents / scripts** | `kill ` and `docker stop` work without special-casing the tracker. No need for `kill -9`. | +| **End users** | Fewer dropped connections during restarts/deployments. Active HTTP requests are drained before the process exits. | + +## Production Scenarios + +The shutdown process must work correctly in all production scenarios where the +tracker runs: + +### 1. Docker / Podman Containers + +Container orchestrators send `SIGTERM` when stopping a container, followed by +`SIGKILL` after a grace period (default 10s). The tracker must: + +- Catch `SIGTERM` and start the shutdown sequence. +- Drain active connections within the orchestrator's grace period. +- Exit cleanly before `SIGKILL` is sent. + +### 2. Cloud Providers (Kubernetes, ECS, Nomad, etc.) + +Cloud platforms add a pre-stop hook or a configurable termination grace period +(e.g., `terminationGracePeriodSeconds` in Kubernetes, typically 30s). The +tracker must: + +- Respond to `SIGTERM` (sent by the platform before killing the pod). +- Respect the configured grace period and exit promptly. +- Allow the platform to collect logs before the pod is removed. + +### 3. Systemd / Init System Managed Services + +When a service manager stops the tracker, it sends `SIGTERM` and waits for the +process to exit. The tracker must: + +- Handle `SIGTERM` correctly. +- Log shutdown progress so the service manager can capture it. +- Respect `TimeoutStopSec` (or equivalent) in the service unit. + +### 4. Human User Running and Stopping the Service + +A developer or operator starts the tracker from a terminal and presses Ctrl+C. +The tracker must: + +- Catch `SIGINT` (Ctrl+C) and start the shutdown sequence. +- Show clear progress feedback in the terminal. +- Exit cleanly within a reasonable time. + +### 5. AI Agents Running and Stopping the Service + +Automated agents (CI/CD pipelines, deployment scripts, monitoring systems) start +and stop the tracker programmatically. They typically send signals via process +management (e.g., `kill`, `docker stop`, systemd). The tracker must: + +- Behave predictably regardless of how the stop signal is sent. +- Not hang indefinitely. +- Exit with a consistent exit code so the agent can detect success/failure. + +## Shutdown Triggers (Options Considered) + +The tracker needs multiple ways to trigger shutdown. Below are the options +considered, with the recommended combination. + +### Current Situation + +`main.rs` only listens for `SIGINT` (Ctrl+C) via `tokio::signal::ctrl_c()`. +The `kill` command sends `SIGTERM` by default, which is not handled. AI agents +are forced to use `kill -INT ` or `kill ` (which does nothing) and +then fall back to `kill -9 `. + +### Option 1: SIGTERM Handler (Minimum Recommendation) + +Add a `SIGTERM` handler alongside the existing `SIGINT` handler. `kill ` +would then trigger graceful shutdown automatically. + +```rust +tokio::select! { + _ = tokio::signal::ctrl_c() => { /* SIGINT */ } + _ = tokio::signal::unix::signal( + tokio::signal::unix::SignalKind::terminate() + ).expect("...").recv() => { /* SIGTERM */ } +}; +``` + +**Pros:** + +- Minimal code change. +- Unix standard: any well-behaved process responds to `SIGTERM`. +- Compatible with `docker stop`, systemd, Kubernetes, and all process managers. +- AI agents can use `kill ` (no `-9` needed). + +**Cons:** + +- AI agents still need to find the PID. + +### Option 2: Unix Domain Socket Command Channel + +Create a Unix socket (e.g. `/tmp/torrust-tracker.sock`) where commands can be +sent. + +```rust +let listener = UnixListener::bind("/tmp/torrust-tracker.sock")?; +``` + +Usage: `echo "shutdown" | nc -U /tmp/torrust-tracker.sock` + +**Pros:** + +- Full control over commands (shutdown, status, metrics, etc.). +- No need to know the PID. +- Can be authenticated/authorized. + +**Cons:** + +- More code to maintain. +- Socket management (cleanup on exit, avoid collisions between instances). +- Only works on Unix. + +### Option 3: HTTP Shutdown Endpoint + +The tracker already exposes a REST API. Add a shutdown endpoint: + +```text +POST /api/shutdown +``` + +Which internally triggers the `CancellationToken` from `JobManager`. + +Usage: `curl -X POST http://localhost:1212/api/shutdown` + +**Pros:** + +- Very natural for AI agents. +- No need for PID or filesystem access. +- Integrates with existing API authentication. +- Works over network (remote shutdown). + +**Cons:** + +- Only works if the REST API is enabled and reachable. +- Security considerations (who can call this endpoint). + +### Option 4: Custom Signals (SIGUSR1 and SIGUSR2) + +Use Unix user-defined signals for custom actions: + +```rust +tokio::signal::unix::signal(SignalKind::user_defined1())? +``` + +**Pros:** + +- No new infrastructure needed. +- Can differentiate between shutdown (SIGUSR1) and other actions (SIGUSR2). + +**Cons:** + +- Not standard — operators must remember custom signal numbers. +- Only works on Unix. + +### Conclusion and Current Priority + +Adding `SIGTERM` is the only change needed to satisfy the Unix, Docker, +Kubernetes, and systemd contracts. It is a small code change with very high +value. The HTTP endpoint is a useful future enhancement for remote/API-driven +management, but it is not needed to meet the fundamental standards. + +| Trigger | Priority | Rationale | +| -------------------------- | -------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | +| **SIGTERM handler** | ✅ Now | Implements the Unix/Docker/K8s/systemd contract. Makes `kill `, `docker stop`, `systemctl stop`, and Kubernetes pod termination work correctly. | +| **HTTP shutdown endpoint** | 🔜 Future | Useful for API-driven management and remote shutdown. Not needed for contract compliance. | +| Unix domain socket | ❌ Not planned | SIGTERM covers the use case. Adds complexity without proportional value. | +| Custom signals | ❌ Not needed | Non-standard. SIGTERM is sufficient. | + +## Scope + +### In Scope + +- Centralize signal handling in `main.rs` (both `SIGINT` and `SIGTERM`). +- Consistent shutdown mechanism for all jobs (prefer `CancellationToken`). +- Configurable grace periods. +- Observable shutdown progress (which jobs are still running). +- Proper UDP server shutdown (drain or at least log in-flight work). +- Grace period alignment between `JobManager` and server-level shutdown. + +### Out of Scope + +- Hot-reload / restart without process exit. +- Dynamic job lifecycle (start/stop jobs at runtime via admin API). +- Windows-specific signal handling beyond what Tokio provides. +- The **profiling binary** (`src/console/profiling.rs`) — it is a developer-only + tool for profiling (valgrind/callgrind), not a user-facing entry point. It can + be updated independently as needed. + +## Design Ideas + +### Centralized Signal Handling + +Only `main.rs` captures OS signals. On signal receipt: + +1. Log "Shutting down..." +2. Cancel all jobs via `CancellationToken`. +3. Send halt signal to all servers via oneshot channels. +4. Wait for all jobs concurrently with a shared grace period. +5. Log "Shutdown complete" or "N jobs did not finish in time". + +### Consistent Job Interface + +Every job should accept a `CancellationToken` and check it in its main loop. +Jobs that need a two-phase shutdown (e.g., Axum servers) can additionally +receive a halt channel. + +### Observable Shutdown + +The `JobManager` should periodically log which jobs are still running during +shutdown, e.g.: + +```text +Waiting for jobs to finish (timeout: 30s)... + ✅ Health Check API — done + ⏳ HTTP tracker (127.0.0.1:7070) — still running (5 active connections) + ⏳ Torrent cleanup — still running + ❌ Activity metrics updater — timed out +``` + +### Configurable Grace Periods + +Add configuration options: + +```toml +[shutdown] +grace_period_secs = 30 +force_timeout_secs = 35 +``` + +## Related Documents + +- [Analysis: Shutdown Process](../../analysis/20260716-shutdown-process/README.md) — detailed code-level analysis +- [Research: Console Shutdown Patterns](../../research/20260716-console-shutdown-patterns/README.md) — SIGINT vs SIGTERM and real-world patterns +- [EPIC: Overhaul Tracker Shutdown](../../issues/open/1488-overhaul-tracker-shutdown/ISSUE.md) — concrete task breakdown diff --git a/docs/features/shutdown-process/open-questions.md b/docs/features/shutdown-process/open-questions.md new file mode 100644 index 000000000..3edda4fa4 --- /dev/null +++ b/docs/features/shutdown-process/open-questions.md @@ -0,0 +1,669 @@ +--- +doc-type: open-questions +status: open +last-updated-utc: 2026-07-16 (Q1, Q2 resolved) +semantic-links: + related-artifacts: + - docs/features/shutdown-process/README.md + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md + - docs/research/20260716-console-shutdown-patterns/README.md +--- + +# Open Questions: Shutdown Process Feature + +This document tracks open questions, risks, and gaps identified during the +specification of the shutdown process feature. All items must be addressed +(answered, resolved, or explicitly deferred with a rationale) before the +feature spec (`README.md`) can be considered complete and the EPIC sub-issues +can be created and scheduled for implementation. + +## Progress + +| # | Severity | Status | Title | +| ----------- | ------------ | ----------- | ------------------------------------------------------------------- | +| [Q1](#q1) | 🔴 Critical | ✅ Resolved | `global_shutdown_signal()` removal not tracked | +| [Q2](#q2) | 🔴 Critical | ✅ Resolved | Halt-sender wiring unspecified in design | +| [Q3](#q3) | 🔴 Critical | Open | Exit codes on shutdown not defined | +| [Q4](#q4) | 🟡 Important | Open | Docker 10s default vs. tracker grace period | +| [Q5](#q5) | 🟡 Important | Open | Orphan risk if `main.rs` crashes before sending halts | +| [Q6](#q6) | 🟡 Important | Open | Two-phase shutdown not discussed for Kubernetes rolling deployments | +| [Q7](#q7) | 🟡 Important | Open | `#[cfg(unix)]` asymmetry on Windows not noted | +| [Q8](#q8) | 🟢 Minor | Open | `SIGHUP` / config reload not explicitly deferred | +| [Q9](#q9) | 🟢 Minor | Open | Docker experimental validation missing | +| [Q10](#q10) | 🟢 Minor | Open | Option 4 heading/body mismatch after signal rename | + +## Question → Sub-issue Impact + +This table shows which sub-issues each question affects and what the current +readiness is. Update it whenever a question is resolved. + +| Question | Affected sub-issues | Impact | +| -------- | --------------------------- | -------------------------------------------------------- | +| Q1 ✅ | SI-2 (partial), SI-3 (full) | SI-3 fully unblocked. SI-2 still needs Q5. | +| Q2 | SI-2, SI-4, SI-5 | Wiring design decision needed before SI-2 implementation | +| Q3 | SI-1 (exit code AC), SI-8 | Exit code contract must be defined | +| Q4 | SI-6, SI-8 | Grace period target values must be agreed | +| Q5 | SI-2 | Orphan risk strategy must be decided before SI-2 lands | +| Q6 | SI-6 | Advisory note — no hard block | +| Q7 | SI-1 | Windows note to add — no hard block | +| Q8 | feature doc only | Out-of-scope note to add — no hard block | +| Q9 | SI-1 verification | Docker test for Phase 2 — no hard block | +| Q10 | feature doc only | Cosmetic fix — no hard block | + +## Sub-issue Readiness + +| Sub-issue | Can start? | Waiting on | +| --------- | ---------- | ------------------------------- | +| SI-1 | ✅ Yes | Nothing | +| SI-3 | ✅ Yes | Nothing (Q1 resolved) | +| SI-4 | ✅ Yes | Nothing | +| SI-5 | ✅ Yes | Nothing | +| SI-7 | ✅ Yes | Nothing | +| SI-9 | ✅ Yes | Nothing | +| SI-2 | ❌ No | Q5 (orphan risk strategy) | +| SI-6 | ❌ No | Q4 (grace period target values) | +| SI-8 | ❌ No | Q3 (exit codes), Q4 | + +--- + +## Q1 + +**Severity**: 🔴 Critical +**Status**: ✅ Resolved (2026-07-16) +**Title**: `global_shutdown_signal()` removal not tracked; standalone binaries have a different contract + +### Description + +#### The double-signal problem in the main tracker binary + +The analysis (§7.7) and research (§5.2) both identify a **double-signal problem** +in `src/main.rs`: when `SIGINT` or `SIGTERM` is received, both `main.rs` and each +server's internal `shutdown_signal()` (via `global_shutdown_signal()`) catch the +same signal independently. This creates a race condition where servers may begin +shutting themselves down before `main.rs` has called `jobs.cancel()` and +`jobs.wait_for_all()`. + +Without removing `global_shutdown_signal()`, adding `SIGTERM` to `main.rs` creates +a **triple-signal** scenario for SIGTERM: + +1. `main.rs` catches SIGTERM and starts the ordered shutdown. +2. Each server's `shutdown_signal()` also catches it via `global_shutdown_signal()`. +3. `main.rs` then also sends a `Halted` message via the oneshot halt channel. + +This is **not tracked** as a sub-issue in the EPIC. + +#### The standalone binary examples have a completely different shutdown contract + +The tracker is intentionally designed as a set of composable packages. There are +two example standalone binaries that show how library users can build their own +trackers: + +- `packages/axum-http-server/examples/http_only_public_tracker.rs` +- `packages/udp-server/examples/udp_only_public_tracker.rs` + +Both examples use the same pattern — they **do not use `JobManager`** at all. +Instead they rely directly on `global_shutdown_signal()` (via `ctrl_c()`) and +the server's `Environment::stop()` method: + +```rust +// Both examples look like this: +tokio::signal::ctrl_c().await.expect("failed to install Ctrl-C handler"); +env.stop().await; +``` + +Looking at what `env.stop()` does in each case: + +**HTTP example** (`Environment::stop()`): + +```rust +pub async fn stop(self) -> Environment { + // Stop the event listener — NOTE: uses abort(), not graceful cancellation + if let Some(event_listener_job) = self.event_listener_job { + // todo: send a message to the event listener to stop and wait for it to finish + event_listener_job.abort(); + } + // Stop the server — sends Halted::Normal via oneshot channel + let server = self.server.stop().await.expect("..."); + ... +} +``` + +**UDP example** (`Environment::stop()`): + +```rust +pub async fn stop(self) -> Environment { + // Abort all three event listener jobs — NOTE: abort(), not graceful cancellation + udp_core_event_listener_job.abort(); + udp_server_stats_event_listener_job.abort(); + udp_server_banning_event_listener_job.abort(); + // Stop the server — sends Halted::Normal via oneshot channel + let server = self.server.stop().await.expect("..."); + ... +} +``` + +This reveals **two more issues specific to the standalone examples**: + +1. **Both examples only handle `SIGINT`** — they call `tokio::signal::ctrl_c()`, + which is SIGINT only. SIGTERM is **not** handled, just like the main binary. + `docker stop` or `kill` will be ignored. + +2. **Event listeners are `abort()`ed, not gracefully stopped** — the `TODO` + comments in the code explicitly call this out. The `CancellationToken` in + `Environment` is created but **never cancelled** — `cancel()` is never called + on it. This means event listeners are abruptly killed rather than given time + to drain their event queues. Any in-flight statistics events are lost. + +#### The architecture implies a contract question + +The tracker is designed as a library. The shutdown contract for library users +(standalone binaries) is: + +- Currently: "call `env.stop()` after `ctrl_c()`" +- Problem: `env.stop()` aborts event listeners rather than cancelling them + +If we fix `global_shutdown_signal()` in the main tracker binary, the standalone +examples remain broken in different ways. The fix strategy must consider both +consumers. + +### Question to answer + +1. Can we add `SIGTERM` to `main.rs` (as a standalone sub-issue) without + removing `global_shutdown_signal()` from the servers, and without breaking + the standalone binary contract? + +2. Should `Environment::stop()` in both `axum-http-server` and `udp-server` + use `cancellation_token.cancel()` instead of `abort()` for event listeners? + The `CancellationToken` is already in the `Environment` struct but unused + during shutdown. + +3. Should the standalone example binaries also be updated to handle `SIGTERM`? + They are documentation/examples but they model the intended usage pattern. + +### Proposed approach + +**For the main tracker binary (`src/main.rs`):** + +Adding `SIGTERM` and removing `global_shutdown_signal()` are logically coupled but +can be landed as two sequential sub-issues if done carefully: + +- Sub-issue A: Add `SIGTERM` to `main.rs` (the `global_shutdown_signal()` double-signal + becomes a triple-signal for SIGTERM, but the behavior is still correct — just redundant). +- Sub-issue B: Remove `global_shutdown_signal()` from `shutdown_signal()` in + `torrust_server_lib` (requires coordination with the standalone binary consumers). + +Sub-issue B touches an external standalone package (`torrust-server-lib`) which +is no longer part of this workspace. That must be factored into planning. + +**For the standalone binary examples:** + +- Fix `Environment::stop()` to call `cancellation_token.cancel()` and await + the event listener jobs instead of `abort()`ing them. +- Update both examples to handle `SIGTERM` alongside `SIGINT`. + +### Decision + +**1. SI-1 (SIGTERM in `main.rs`) can and should land before SI-2.** + +The Phase 1 verification evidence confirms the sequence is safe: after SIGTERM, +the servers' `global_shutdown_signal()` reacts independently (they start draining +their connections), while `main.rs` does nothing. After SI-1 lands, `main.rs` +catches SIGTERM first at the top-level `tokio::select!`, calls `jobs.cancel()`, +and sends halt messages to the servers. The servers' own `global_shutdown_signal()` +fires afterward as a redundant no-op. The behavior is correct and the logs will +show duplicate "caught interrupt signal (terminate)" messages — noisy but +harmless. SI-2 will clean this up later. + +**2. SI-1 and SI-2 are separate sub-issues landed sequentially.** + +- SI-1: Add `SIGTERM` to `main.rs` — no prerequisites, safe to land now. +- SI-2: Remove `global_shutdown_signal()` from `shutdown_signal()` in + `torrust_server_lib` — requires a coordinated release of `torrust-server-lib`. + Must not land before the orphan risk in Q5 is also resolved. + +**3. `Environment::stop()` should use `cancel()` instead of `abort()` for event +listeners.** + +The `CancellationToken` is already wired into `Environment` but never cancelled. +The `TODO` comments in the code call this out explicitly. This is a pre-existing +bug in the standalone library API. The fix is tracked in SI-3. + +**4. Both standalone example binaries should be updated to handle `SIGTERM`.** + +They model the intended library usage pattern. If a user copies the example as +a starting point, their binary will have the same SIGTERM gap. SI-3 covers this. + +### Actions Taken + +- [x] Decision recorded: SI-1 and SI-2 are sequential, SI-1 is safe to land first. +- [x] SI-2 already exists in the EPIC sub-issue table with the `torrust-server-lib` + external dependency noted. +- [x] SI-3 already exists covering `Environment::stop()` abort-vs-cancel and + SIGTERM for standalone examples. +- [x] Q5 (orphan risk) remains open — it must be resolved before SI-2 lands, + not before SI-1. + +--- + +## Q2 + +**Severity**: 🔴 Critical +**Status**: ✅ Resolved (2026-07-16) +**Title**: Halt-sender wiring is unspecified in the design + +### Description + +The feature doc "Design Ideas" section says: + +> "Send halt signal to all servers via oneshot channels." + +This is architecturally vague. Currently the `halt_task` (the `Sender`) +for each server is owned inside the job closure that was spawned in the bootstrap +job starters (`start_job` functions). The `JobManager` only holds `JoinHandle<()>` +values — it has no reference to the halt senders. + +To implement centralized halt dispatch, one of these approaches is needed: + +1. **Wrap halt senders in the `JobManager`** — add a `Vec>` to + `JobManager` alongside `Vec`. `cancel()` would then both cancel the + `CancellationToken` and send `Halted::Normal` to all registered halt senders. +2. **Move server shutdown into `CancellationToken`** — remove the oneshot `Halted` + channel entirely and make servers watch the `CancellationToken` directly, using + the Axum `Handle` for connection draining triggered by the token. +3. **Leave halt senders inside the spawned jobs** — each job handles its own halt + when the `CancellationToken` fires (but this requires the Axum servers to grow + a `CancellationToken` watch loop, which they currently lack). + +The design doc mentions none of these options. Without a concrete decision here, +developers implementing the sub-issues will face an unguided architectural choice +mid-implementation. + +### Question to answer + +Which wiring approach should be used? This decision affects the scope and +complexity of at least three sub-issues: + +- "Centralize signal handling in `main.rs`" +- "Migrate torrent cleanup to `CancellationToken`" +- Any sub-issue touching Axum server shutdown + +### Why Option 1 and 2 are not suitable right now + +**Option 1** (wrap halt senders in `JobManager`): the halt sender (`tx_halt`) is +currently consumed inside the `async move` closure of each `start_job` function. +For example, in `src/bootstrap/jobs/udp_tracker.rs`, the `server.state.halt_task` +is moved into the spawned task and dropped when the task ends. There is no way to +extract it without a breaking API change to all job starters — returning +`(JoinHandle<()>, Option>)` from every `start_job`. Significant +refactor touching many files. + +**Option 2** (move servers to watch `CancellationToken` directly): removes the +oneshot `Halted` channel from Axum servers and requires changes to +`torrust_server_lib` and `torrust_tracker_axum_server` — both are external +standalone packages. Touches more packages than necessary for the current goal. + +### Decision + +**Use Option 3: each server job watches the `CancellationToken` and when +cancelled, sends `Halted::Normal` to its own halt channel internally.** + +The `JobManager` calls `cancel()` as it does now. Each server job needs a small +addition: a `CancellationToken` parameter and a `tokio::select!` that either +waits for the existing `server.state.task` to finish or for the token to be +cancelled — at which point it sends `Halted::Normal` to its own halt sender +and then waits for the task. + +This approach: + +- Requires no API change to `JobManager` — it still only holds `JoinHandle<()>`. +- Requires no changes to external packages (`torrust_server_lib`, + `torrust_tracker_axum_server`). +- Is consistent with how event listener jobs already work (they accept a + `CancellationToken` and check `token.cancelled()` in their loops). +- Keeps the halt channel as the internal per-server shutdown mechanism; the + `CancellationToken` becomes the external coordinator trigger. + +**Concrete change per server job starter** (e.g., `src/bootstrap/jobs/udp_tracker.rs`): + +```rust +// Before: +tokio::spawn(async move { + server.state.task.await.expect("..."); +}) + +// After: +tokio::spawn(async move { + tokio::select! { + _ = cancellation_token.cancelled() => { + // JobManager signalled shutdown — forward to the server via halt channel + let _ = server.state.halt_task.send(Halted::Normal); + server.state.task.await.expect("..."); + } + _ = &mut server.state.task => { + // Server finished on its own (e.g. via global_shutdown_signal) + } + } +}) +``` + +The Health Check API job is wired differently (it creates the halt channel +itself), but the same pattern applies: watch the token, send halt, await task. + +**Impact on sub-issues:** + +- SI-1 (SIGTERM to `main.rs`): unchanged — SI-1 only adds signal handling to + `main.rs`, it does not touch job starters. +- SI-2 (remove `global_shutdown_signal()`): this decision means SI-2 must also + update each job starter to pass and use the `CancellationToken` as described + above — otherwise removing `global_shutdown_signal()` would leave servers with + no way to receive the halt signal from `main.rs`. +- SI-4, SI-5 (torrent cleanup, activity metrics): already use `CancellationToken` + directly — no change needed from this decision. + +### Actions Taken + +- [x] Option 3 selected and documented. +- [x] Concrete code pattern documented above. +- [x] No changes needed to `JobManager` API or external packages. +- [x] SI-2 scope note updated: must include job-starter `CancellationToken` + wiring alongside the `global_shutdown_signal()` removal. + +--- + +## Q3 + +**Severity**: 🔴 Critical +**Status**: Open +**Title**: Exit codes on shutdown not defined + +### Description + +The feature doc states (AI agent scenario): + +> "Exit with a consistent exit code so the agent can detect success/failure." + +But neither the feature doc nor the EPIC spec defines what exit codes the tracker +should return. The current code exits 0 after `wait_for_all()` regardless of +whether jobs timed out. This is observed in `src/main.rs`: + +```rust +jobs.wait_for_all(Duration::from_secs(10)).await; +tracing::info!("Torrust tracker successfully shutdown."); +// implicit exit 0 +``` + +Open questions: + +- Should a **graceful shutdown** (all jobs completed within the grace period) → exit 0? +- Should a **timeout shutdown** (some jobs did not finish in time) → exit 0 or a + different code (e.g., 1 or `exit_code::UNAVAILABLE`)? +- Should **startup failure** (e.g., port already in use) → exit 1 or a specific code? +- Should systemd's `SuccessExitStatus` be documented for graceful timeouts? + +This matters for: + +- CI/CD pipelines checking the process exit code. +- Systemd deciding whether to restart the service (`Restart=on-failure`). +- Container orchestrators deciding whether the container exited cleanly. +- AI agents deciding whether to retry or escalate. + +### Action + +- [ ] Define the exit code contract for the tracker (at minimum: success=0, + timeout-during-shutdown=?, startup-failure=?). +- [ ] Add an "Exit Codes" section to the feature doc. +- [ ] Check whether Vector's exit code crate pattern is appropriate here. + +--- + +## Q4 + +**Severity**: 🟡 Important +**Status**: Open +**Title**: Docker's 10s default grace period may be shorter than the tracker needs + +### Description + +The feature doc describes the Docker contract correctly: + +> `docker stop` sends SIGTERM and waits up to 10 seconds before SIGKILL. + +However, it does not acknowledge the tension between Docker's **10s default** and +the tracker's **10s per-job sequential wait** in `JobManager`. In a worst case: + +```text +t=0 Docker sends SIGTERM +t=0 main.rs catches SIGTERM, starts shutdown +t=0 JobManager starts waiting for job 1 (up to 10s) +t=5 job 1 finishes +t=5 JobManager starts waiting for job 2 (up to 10s) +t=10 Docker sends SIGKILL (tracker is killed mid-shutdown) +t=10 jobs 2..N are force-killed with no cleanup +``` + +The "after fix" description implies the tracker will exit cleanly within Docker's +grace period, but that is only guaranteed if either: + +1. All jobs complete well within 10s total (likely in practice, but not guaranteed). +2. Operators configure Docker's `stop_grace_period` to a higher value. + +The feature doc should explicitly state the **minimum recommended Docker/Compose +`stop_grace_period`** and warn operators to configure it appropriately. + +### Question to answer + +What is the minimum recommended `stop_grace_period` for the tracker, given its +current sequential 10s-per-job waiting? Should this be documented in the feature +spec or in a deployment guide? + +### Action + +- [ ] Add a note to the feature doc warning that Docker's 10s default may be + insufficient and recommending a `stop_grace_period` ≥ 30s in `compose.yml`. +- [ ] Cross-reference `docs/containers.md` as the place to document deployment + configuration. + +--- + +## Q5 + +**Severity**: 🟡 Important +**Status**: Open +**Title**: Orphan risk if `main.rs` crashes before sending halt messages + +### Description + +If we remove `global_shutdown_signal()` from the servers (as recommended in Q1), +the servers will **only stop** when they receive a `Halted` message via their +oneshot channel — which is sent by `main.rs` during the shutdown sequence. + +If `main.rs` crashes, panics, or is killed with `SIGKILL` before sending those +halt messages: + +- The Tokio tasks running the servers continue running as orphans. +- They hold the TCP/UDP ports. +- The process appears to be gone (from the OS perspective the parent is dead) but + the ports are not freed. +- A restart attempt will fail with "address already in use". + +This was actually observed during experimental validation: when SIGTERM was sent +to the `cargo run` process (not the actual binary), the actual binary became +orphaned and held the ports. + +### Question to answer + +Should the servers retain a fallback `global_shutdown_signal()` as a safety net +for SIGKILL/crash scenarios, or should the deployment environment handle this +(e.g., container restart policy, systemd `KillMode=control-group`)? + +### Action + +- [ ] Decide on the safety net strategy. +- [ ] If retaining `global_shutdown_signal()` as a fallback: document this as + intentional in the server signal design. +- [ ] If relying on the OS/container environment: document the deployment + requirement (e.g., "always run in a container with a restart policy"). +- [ ] Add a note to the feature doc about this risk. + +--- + +## Q6 + +**Severity**: 🟡 Important +**Status**: Open +**Title**: Two-phase shutdown not discussed for Kubernetes rolling deployments + +### Description + +The research doc (§4.5) describes a pattern used by Vector and other production +services: before draining connections, **first mark the service as unhealthy**. +This causes load balancers and Kubernetes readiness probes to stop routing new +traffic to this instance during the drain window. + +Without this, during a rolling deployment: + +1. Kubernetes sends `SIGTERM` to the old pod. +2. The tracker starts shutting down but is still accepting new connections. +3. New BitTorrent clients connect and their announce/scrape requests are + immediately dropped when the tracker exits. +4. Clients see unexplained errors during the deployment window. + +For a BitTorrent tracker, the impact depends on client behavior: + +- HTTP clients get a connection error or incomplete response. +- UDP clients get no response (UDP is fire-and-forget anyway). + +The feature doc's K8s section says "The tracker drains active connections and +exits cleanly" but does not address whether the tracker stops accepting **new** +connections during the drain period. + +### Question to answer + +Should the tracker implement a pre-drain "unhealthy" phase? If so: + +- Should the Health Check API start returning 503 immediately on shutdown signal? +- Is this needed for the initial SIGTERM sub-issue or is it a separate concern? + +### Action + +- [ ] Decide whether the Health Check API should return 503 during shutdown. +- [ ] If yes: add a sub-issue or note in the EPIC for "Mark Health Check as + unhealthy during shutdown". +- [ ] If no: add a note to the feature doc explaining why this is acceptable + (e.g., UDP clients retry anyway, HTTP clients are rare in BitTorrent usage). + +--- + +## Q7 + +**Severity**: 🟡 Important +**Status**: Open +**Title**: `#[cfg(unix)]` asymmetry on Windows not noted + +### Description + +The recommended `SIGTERM` implementation (research doc §5.1 and feature doc design) +requires `#[cfg(unix)]` because `SIGTERM` does not exist on Windows: + +```rust +#[cfg(unix)] +let mut sigterm = signal(SignalKind::terminate()) + .expect("failed to install SIGTERM handler"); + +tokio::select! { + _ = ctrl_c => { ... } + #[cfg(unix)] + _ = sigterm.recv() => { ... } +} +``` + +On Windows, the `select!` only has `ctrl_c`. This is correct behavior — `kill` +(in the Windows sense, via Task Manager or `taskkill.exe`) sends `WM_CLOSE` or +terminates directly, and `ctrl_c()` already catches Ctrl+C, Ctrl+Break, and +console close events on Windows. + +However, the feature doc and EPIC spec make no mention of Windows behavior. The +EPIC "Out of Scope" section says "Windows-specific signal handling beyond what +Tokio provides" which is fine, but neither document explains what that means +concretely for Windows users of the tracker. + +### Question to answer + +Is the current Windows behavior (only `ctrl_c()`) acceptable and documented +enough? Or should the feature doc at least note what happens on Windows? + +### Action + +- [ ] Add a brief Windows note to the feature doc explaining that `SIGTERM` + handling is Unix-only and that `ctrl_c()` covers the relevant Windows + termination events. + +--- + +## Q8 + +**Severity**: 🟢 Minor +**Status**: Open +**Title**: `SIGHUP` / config reload not explicitly deferred + +### Description + +The research doc (§4.2) lists `SIGHUP` as commonly used for configuration reload +in daemons. The feature doc and EPIC make no decision about it — it is neither +in scope nor explicitly out of scope. Operators familiar with Unix daemons may +expect `SIGHUP` to trigger a config reload. + +### Question to answer + +Should `SIGHUP` handling (config reload without restart) be explicitly deferred +to a separate future feature, or explicitly excluded from this tracker's roadmap? + +### Action + +- [ ] Add `SIGHUP` to the feature doc "Out of Scope" section with a note: + either "deferred to a future 'hot reload' feature" or "not planned — restart + the tracker to apply configuration changes". + +--- + +## Q9 + +**Severity**: 🟢 Minor +**Status**: Open +**Title**: Docker experimental validation missing + +### Description + +The analysis doc (§8) validates SIGTERM and SIGINT by sending signals directly to +the binary. However, the most common production scenario — `docker stop` — was +not tested. It is possible (as noted in Q4) that Docker's 10s default would +SIGKILL the tracker before shutdown completes, even after the SIGTERM fix. + +### Action + +- [ ] After implementing the SIGTERM handler, run `docker stop` against the + containerized tracker and verify it exits cleanly within the default 10s. +- [ ] Document the result in the analysis doc (§8) as a new experiment. +- [ ] If the default 10s is insufficient, update Q4's recommendation accordingly. + +--- + +## Q10 + +**Severity**: 🟢 Minor +**Status**: Open +**Title**: Option 4 heading/body mismatch after signal rename + +### Description + +In the feature doc "Shutdown Triggers" section, Option 4 was renamed from +"Custom Signals (SIGUSR1 / SIGUSR2)" to "Custom Signals" to avoid cspell issues. +However, the body still refers to `SIGUSR1` and `SIGUSR2` by name. The heading +no longer tells the reader which signals are being discussed. + +### Action + +- [ ] Restore the signal names to the heading: "Option 4: Custom Signals + (`SIGUSR1` / `SIGUSR2`)" — the cspell issue is solved by the `SIGUSR` entry + in `project-words.txt`, so the full name can now be used in the heading. diff --git a/docs/index.md b/docs/index.md index 0acd6e775..979197a5e 100644 --- a/docs/index.md +++ b/docs/index.md @@ -4,6 +4,8 @@ semantic-links: - write-markdown-docs related-artifacts: - docs/AGENTS.md + - docs/analysis/AGENTS.md + - docs/research/AGENTS.md - docs/benchmarking.md - docs/containers.md - docs/packages.md @@ -44,6 +46,27 @@ Records of significant architectural decisions, including context and consequenc | [adrs/README.md](adrs/README.md) | Index of all ADRs and guidance on writing new ones | | [adrs/index.md](adrs/index.md) | Quick-reference table of every ADR | +## Analysis Documents + +In-depth studies of concrete features, components, or aspects of the application, +typically produced before defining a refactoring plan, introducing a new feature, +or making architectural decisions. + +| Location | Description | +| -------------------------------------------------------------------------- | --------------------------------------------------- | +| [analysis/AGENTS.md](analysis/AGENTS.md) | Overview of the analysis folder and its conventions | +| [analysis/20260716-shutdown-process/](analysis/20260716-shutdown-process/) | Analysis of the tracker shutdown process | + +## Research Documents + +Investigations of external topics, technologies, or patterns relevant to the +project. Research looks outward — at how other projects solve similar problems. + +| Location | Description | +| -------------------------------------------------------------------------------------------- | -------------------------------------------------------------- | +| [research/AGENTS.md](research/AGENTS.md) | Overview of the research folder and its conventions | +| [research/20260716-console-shutdown-patterns/](research/20260716-console-shutdown-patterns/) | How console apps handle SIGINT, SIGTERM, and graceful shutdown | + ## Issue Specifications Structured specification documents linked to GitHub issues. Used for planning and tracking diff --git a/docs/issues/drafts/1488-si-1-add-sigterm-to-main/ISSUE.md b/docs/issues/drafts/1488-si-1-add-sigterm-to-main/ISSUE.md new file mode 100644 index 000000000..fb0bdad0f --- /dev/null +++ b/docs/issues/drafts/1488-si-1-add-sigterm-to-main/ISSUE.md @@ -0,0 +1,215 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p1 +github-issue: null +spec-path: docs/issues/drafts/1488-si-1-add-sigterm-to-main/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - src/main.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/features/shutdown-process/README.md + - docs/features/shutdown-process/open-questions.md + - docs/analysis/20260716-shutdown-process/README.md + - docs/research/20260716-console-shutdown-patterns/README.md +--- + + + +# Draft SI-1 — Add `SIGTERM` Handler to `main.rs` + +> **EPIC position**: SI-1 of #1488. Highest priority. No prerequisites. + +## Goal + +Handle `SIGTERM` in `src/main.rs` alongside the existing `SIGINT` handler so that +`kill `, `docker stop`, `systemctl stop`, and Kubernetes pod termination all +trigger the same graceful shutdown as Ctrl+C. + +This is the single most impactful change in the EPIC: a few lines of code that fix +the tracker's compliance with the Unix, Docker, Kubernetes, and systemd process +lifecycle contracts. + +## Background + +`main.rs` currently only listens for `SIGINT` (Ctrl+C) via +`tokio::signal::ctrl_c()`. `SIGTERM` (signal 15) — sent by default by `kill`, +`docker stop`, `systemctl stop`, and Kubernetes — is silently ignored by `main.rs`. + +This was confirmed experimentally on 2026-07-16 (see Phase 1 evidence in +[verification.md](./verification.md)). + +**Exact behaviour observed (commit 49d8117f)**: + +- `kill ` sent SIGTERM to the binary (PID 955797). +- Each server's internal `global_shutdown_signal()` **did** catch SIGTERM and + began draining its own connections — this is the per-server signal handler + inside `torrust_server_lib::signals`, **not** `main.rs`. +- `main.rs`'s `tokio::select!` did **not** fire. `jobs.cancel()` and + `jobs.wait_for_all()` were never called. +- After the servers shut themselves down, the swarm coordination registry + continued emitting periodic metrics, proving `main.rs` was still running. +- The process had to be killed with `kill -9` (exit code 137). +- The log contained **none** of the normal graceful shutdown messages + (`Torrust tracker shutting down`, `Job completed gracefully`, + `Torrust tracker successfully shutdown.`). + +See also [analysis §7.4 and §8.1](../../analysis/20260716-shutdown-process/README.md) +and [research §4.2](../../research/20260716-console-shutdown-patterns/README.md). + +## Implementation + +Change `src/main.rs` from: + +```rust +tokio::select! { + _ = tokio::signal::ctrl_c() => { + tracing::info!("Torrust tracker shutting down ..."); + jobs.cancel(); + jobs.wait_for_all(Duration::from_secs(10)).await; + tracing::info!("Torrust tracker successfully shutdown."); + } +} +``` + +To: + +```rust +#[cfg(unix)] +let mut sigterm = tokio::signal::unix::signal( + tokio::signal::unix::SignalKind::terminate() +).expect("failed to install SIGTERM handler"); + +tokio::select! { + _ = tokio::signal::ctrl_c() => { + tracing::info!("Torrust tracker shutting down (SIGINT) ..."); + } + #[cfg(unix)] + _ = sigterm.recv() => { + tracing::info!("Torrust tracker shutting down (SIGTERM) ..."); + } +} + +jobs.cancel(); +jobs.wait_for_all(Duration::from_secs(10)).await; +tracing::info!("Torrust tracker successfully shutdown."); +``` + +**Important nuance from the experimental baseline**: after this change there +will be a **redundant double-signal** for SIGTERM: `main.rs` catches it _and_ +each server's `global_shutdown_signal()` also catches it. In practice, `main.rs` +reacts first (since it is the top-level `tokio::select!`), calls `jobs.cancel()`, +and sends halt messages to all servers. The servers' own SIGTERM handlers then +fire as a redundant no-op. This is harmless but produces extra log noise +(duplicate `caught interrupt signal (terminate)` messages from each server). +The clean removal of `global_shutdown_signal()` is tracked in SI-2. + +## Acceptance Criteria + +- [ ] `kill ` against the tracker binary starts a graceful shutdown. +- [ ] `kill -TERM ` starts a graceful shutdown. +- [ ] Ctrl+C still works as before. +- [ ] The tracker logs distinguish the signal source: `(SIGINT)` vs `(SIGTERM)`. +- [ ] After SIGTERM, `main.rs` logs `Torrust tracker shutting down (SIGTERM) ...`. +- [ ] `JobManager` logs `Waiting for job to finish` for each job. +- [ ] All jobs report `Job completed gracefully`. +- [ ] Final log line is `Torrust tracker successfully shutdown.` +- [ ] No periodic metrics log lines appear after the final shutdown message. +- [ ] Process exits with code 0 (not 137). +- [ ] `cargo test` passes. +- [ ] `linter all` passes. +- [ ] Phase 2 of [verification.md](./verification.md) is fully completed. + +## Open Questions Affecting This Sub-issue + +- [Q1](../../features/shutdown-process/open-questions.md#q1): The + double-signal for SIGTERM after this change is harmless but should be noted. + SI-2 must follow to clean it up. + +## Dependencies + +- No hard prerequisites. Can land independently. +- SI-2 should follow to remove the `global_shutdown_signal()` redundancy. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +```bash +# Build the release binary +cargo build --release + +# Start the tracker in a terminal +RUST_LOG=info ./target/release/torrust-tracker +``` + +Wait for all services to report "Started on" in the log output. + +### Test 1: `kill ` triggers graceful shutdown (SIGTERM) + +```bash +# In a second terminal, get the binary PID (not the cargo PID) +pgrep -x torrust-tracker +kill +``` + +**Expected**: + +- Log shows: `Torrust tracker shutting down (SIGTERM) ...` +- Log shows each job completing gracefully: `Job completed gracefully job=...` +- Log shows: `Torrust tracker successfully shutdown.` +- Process exits with code 0. +- No `SIGKILL` is needed. +- Verify exit code: `echo $?` in the terminal running the tracker (should be 0). + +**Record in `verification.md`**: full log output from shutdown start to exit. + +### Test 2: `kill -TERM ` (explicit SIGTERM) + +Repeat Test 1 using `kill -TERM `. Expected outcome is identical. + +### Test 3: Ctrl+C still works (SIGINT) + +```bash +# Start the tracker, then press Ctrl+C in its terminal +``` + +**Expected**: + +- Log shows: `Torrust tracker shutting down (SIGINT) ...` +- Same graceful shutdown sequence as Test 1. +- Log **does not** say `(SIGTERM)`. + +### Test 4: Signal source is distinguishable in logs + +Confirm that the log message text differs between SIGINT and SIGTERM shutdowns +(i.e., the log says "SIGINT" vs "SIGTERM" respectively). + +### Test 5: `docker stop` triggers graceful shutdown + +```bash +# Run tracker in a container +docker run -d --name torrust-test torrust/tracker:dev +docker stop torrust-test +docker logs torrust-test +``` + +**Expected**: + +- Logs show the graceful shutdown sequence ending with `successfully shutdown`. +- `docker stop` returns within 10 seconds (Docker default). +- No "Killed" or forced exit message from Docker. + +### Test 6: `kill -9 ` still works (SIGKILL, unchanged behavior) + +Verify that `kill -9` still terminates the process immediately (this is OS behavior +and cannot be changed, but should be confirmed as still functional). diff --git a/docs/issues/drafts/1488-si-1-add-sigterm-to-main/verification.md b/docs/issues/drafts/1488-si-1-add-sigterm-to-main/verification.md new file mode 100644 index 000000000..bbaf8a54d --- /dev/null +++ b/docs/issues/drafts/1488-si-1-add-sigterm-to-main/verification.md @@ -0,0 +1,240 @@ +# Verification Evidence — SI-1: Add `SIGTERM` Handler to `main.rs` + +This document contains two phases of verification: + +- **Phase 1 (Pre-implementation)**: evidence that the current behaviour is broken. +- **Phase 2 (Post-implementation)**: evidence that the fix works correctly. + +Both phases must be completed. Phase 1 was run on the `before` baseline. Phase 2 +must be run after the implementation is merged. + +> Copy-paste actual terminal output. Do not summarize or paraphrase. Raw output +> is the evidence. + +--- + +## Environment + +- **Date**: 2026-07-16 +- **OS**: Linux josecelano-desktop 7.0.0-27-generic #27-Ubuntu SMP PREEMPT_DYNAMIC + Thu Jun 18 19:13:49 UTC 2026 x86_64 GNU/Linux +- **Rust version**: rustc 1.99.0-nightly (da80ed070 2026-07-14) +- **Tracker git commit**: 49d8117f +- **Branch**: 1488-overhaul-tracker-shutdown-docs + +--- + +## Phase 1 — Pre-Implementation (Baseline: Broken Behaviour) + +> **Purpose**: prove that `SIGTERM` is currently ignored by `main.rs` and that +> the tracker must be force-killed with `SIGKILL` to stop it. +> +> **Status**: Completed on 2026-07-16. + +### P1 — Preparation + +Binary built and confirmed: + +```bash +cargo build --release +ls -lh target/release/torrust-tracker +``` + +```text +-rwxrwxr-x 2 josecelano josecelano 127M Jul 16 17:54 target/release/torrust-tracker +``` + +### P1 — Test 1: `kill ` is ignored by `main.rs` (SIGTERM ignored) + +#### Procedure + +```bash +RUST_LOG=info ./target/release/torrust-tracker > /tmp/tracker-si1-before-test1.log 2>&1 & +TRACKER_PID=$(pgrep -x torrust-tracker) +echo "Binary PID: $TRACKER_PID" +kill "$TRACKER_PID" +sleep 3 +if kill -0 "$TRACKER_PID" 2>/dev/null; then + echo "RESULT: Process IS STILL RUNNING — SIGTERM was IGNORED" +else + echo "RESULT: Process has exited" +fi +``` + +#### Terminal Output + +```text +Binary PID: 955797 +RESULT: Process IS STILL RUNNING after SIGTERM — SIGTERM was IGNORED +``` + +#### Interpretation + +The binary PID is 955797. After `kill 955797` (SIGTERM), the process was still +alive 3 seconds later. `main.rs` does not handle SIGTERM. + +#### Key Finding: servers DID react, but `main.rs` did NOT + +Each server's `global_shutdown_signal()` caught SIGTERM and began shutting down +its own connections — but `main.rs`'s `tokio::select!` never fired, so +`jobs.cancel()` and `jobs.wait_for_all()` were never called. + +After the servers shut themselves down, the swarm coordination registry kept +emitting periodic metrics, proving the main process was still alive: + +```text +2026-07-16T16:57:30.727509Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727530Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727535Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727553Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727596Z INFO graceful_shutdown{address=0.0.0.0:7070}: !! Shutting down HTTP server ... in 90 seconds !! +2026-07-16T16:57:30.727608Z INFO graceful_shutdown{address=0.0.0.0:7171}: !! Shutting down HTTP server ... in 90 seconds !! +2026-07-16T16:57:30.727613Z INFO graceful_shutdown{address=0.0.0.0:1212}: All connections closed, shutting down server +2026-07-16T16:57:30.727621Z INFO graceful_shutdown{address=0.0.0.0:7070}: All connections closed, shutting down server +2026-07-16T16:57:30.727615Z INFO graceful_shutdown{address=0.0.0.0:7171}: All connections closed, shutting down server +2026-07-16T16:57:30.727648Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727664Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727679Z WARN ...global_shutdown_signal: caught interrupt signal (terminate), halting... +2026-07-16T16:57:30.727731Z INFO HEALTH CHECK API: Stopped server running on: http://127.0.0.1:1313 +--- servers have stopped, but main.rs is still running: --- +2026-07-16T16:57:44.991005Z INFO torrust_tracker_swarm_coordination_registry: active_peers_total=0 ... +2026-07-16T16:57:59.991645Z INFO torrust_tracker_swarm_coordination_registry: active_peers_total=0 ... +``` + +#### What is absent from the log (critical evidence) + +The log does **not** contain any of these lines that would appear if `main.rs` +had reacted: + +- `Torrust tracker shutting down ...` +- `Waiting for job to finish` +- `Job completed gracefully` +- `Torrust tracker successfully shutdown.` + +#### P1 Test 1 Result: CONFIRMED BUG + +- [x] CONFIRMED: Process still running 3 seconds after SIGTERM +- [x] CONFIRMED: Servers reacted via `global_shutdown_signal()` but `main.rs` did not +- [x] CONFIRMED: `jobs.cancel()` and `jobs.wait_for_all()` were never called +- [x] CONFIRMED: No graceful shutdown message from `main.rs` + +### P1 — Test 2: Force-killing with SIGKILL is required to stop the process + +After SIGTERM was ignored, SIGKILL was required: + +```bash +kill -9 955797 +``` + +```text +Exit 137 ./target/release/torrust-tracker > /tmp/tracker-si1-before-test1.log 2>&1 +``` + +Exit code 137 (= 128 + 9) confirms SIGKILL was used. After the fix, a graceful +shutdown should produce exit code 0. + +### P1 — Test 3: Ports freed after SIGKILL + +```bash +lsof -i :7070,6969,1212,1313 2>/dev/null | grep LISTEN || echo "All ports are now free" +``` + +```text +All ports are now free +``` + +Ports freed correctly when the OS killed the process. + +--- + +## Phase 2 — Post-Implementation (After the Fix) + +> **Status**: Not started — to be completed after SI-1 is implemented and merged. + +Rebuild the binary from the branch with the SIGTERM fix applied, then run all +tests below. The environment block must be updated with the new commit hash. + +### P2 Environment + +- **Date**: +- **Tracker git commit** (`git rev-parse --short HEAD`): +- **Branch** (`git branch --show-current`): + +### P2 — Test 1: `kill ` triggers graceful shutdown (SIGTERM handled) + +Same procedure as P1 Test 1 using the fixed binary. + +```text +(paste terminal output — must show "Process has exited") +``` + +```text +(paste /tmp/tracker-si1-after-test1.log) +``` + +#### P2 Test 1 Pass/Fail + +- [ ] PASS: Process exits without a second signal +- [ ] PASS: Log contains `shutting down (SIGTERM)` +- [ ] PASS: Log contains `successfully shutdown.` +- [ ] PASS: Log contains `Job completed gracefully` for all jobs +- [ ] PASS: No periodic metrics log lines after `successfully shutdown.` +- [ ] PASS: Exit code is 0 (not 137) + +### P2 — Test 2: `kill -TERM ` — same outcome as P2 Test 1 + +```text +(paste log output) +``` + +- [ ] PASS: Outcome identical to P2 Test 1 + +### P2 — Test 3: Ctrl+C — log says SIGINT not SIGTERM + +```text +(paste log output) +``` + +- [ ] PASS: Log contains `shutting down (SIGINT)` +- [ ] PASS: Log does NOT contain `shutting down (SIGTERM)` + +### P2 — Test 4: SIGKILL — still force-terminates immediately (exit 137) + +```bash +kill -9 +echo "Exit code: $?" +``` + +```text +(paste output) +``` + +- [ ] PASS: Exit code is 137 +- [ ] PASS: No graceful shutdown log lines after the kill + +### P2 — Test 5: `docker stop` — graceful shutdown within 10s (optional) + +```text +(paste `time docker stop` output and container log tail — or mark as SKIPPED) +``` + +- [ ] PASS: `docker stop` returns in < 10s +- [ ] PASS: Container log ends with `successfully shutdown.` +- [ ] SKIPPED (reason: ___) + +--- + +## Final Summary + +| Phase | Test | Description | Result | +| ----- | ---- | ----------------------------------- | ------------ | +| P1 | T1 | SIGTERM ignored — process survives | Confirmed | +| P1 | T2 | SIGKILL required — exit code 137 | Confirmed | +| P1 | T3 | Ports freed after SIGKILL | Confirmed | +| P2 | T1 | SIGTERM handled — graceful exit | Pending | +| P2 | T2 | `kill -TERM` — same as T1 | Pending | +| P2 | T3 | Ctrl+C — log says SIGINT | Pending | +| P2 | T4 | SIGKILL — exit 137, no shutdown log | Pending | +| P2 | T5 | `docker stop` — within 10s | Pending | + +All P2 tests must PASS (or T5 SKIPPED with reason) before this issue can be closed. diff --git a/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/ISSUE.md b/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/ISSUE.md new file mode 100644 index 000000000..f9bb2269e --- /dev/null +++ b/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/ISSUE.md @@ -0,0 +1,201 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/axum-server/src/signals.rs + - packages/axum-health-check-api-server/src/server.rs + - packages/axum-http-server/src/server.rs + - packages/axum-rest-api-server/src/server.rs + - packages/udp-server/src/server/launcher.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/features/shutdown-process/open-questions.md +--- + + + +# Draft SI-2 — Remove `global_shutdown_signal()` from Per-Server Shutdown + +> **EPIC position**: SI-2 of #1488. Depends on SI-1. +> **Blocked**: Q5 must be resolved before implementing (Q1 and Q2 are resolved). + +## Goal + +Remove the `global_shutdown_signal()` call from `shutdown_signal()` in +`torrust_server_lib::signals` so that servers only stop when explicitly told to +by `main.rs` via the halt channel — not by independently catching OS signals. + +Also wire the `CancellationToken` from `JobManager` into each server job starter +so that when `main.rs` calls `jobs.cancel()`, the halt message is forwarded to +each server automatically. + +After SI-1 (`main.rs` catches `SIGTERM`), both `main.rs` and each server still +catch SIGINT/SIGTERM independently. This sub-issue cleans that up so that: + +- `main.rs` owns all signal handling. +- Servers shut down only when they receive a `Halted::Normal` message via + their oneshot channel, triggered by the `CancellationToken`. + +## Background + +`torrust_server_lib::signals::shutdown_signal()` currently contains: + +```rust +pub async fn shutdown_signal(rx_halt: tokio::sync::oneshot::Receiver) { + tokio::select! { + signal = halt => { ... }, + () = global_shutdown_signal() => { ... } // <-- catches SIGINT/SIGTERM directly + } +} +``` + +This means every server independently catches Ctrl+C and SIGTERM. The servers +do not wait for `main.rs` to coordinate the shutdown order. + +## Implementation + +Change `shutdown_signal()` in `torrust_server_lib` to remove the +`global_shutdown_signal()` branch: + +```rust +pub async fn shutdown_signal(rx_halt: tokio::sync::oneshot::Receiver) { + match rx_halt.await { + Ok(signal) => tracing::debug!("Halt signal processed: {}", signal), + Err(err) => panic!("Failed to install stop signal: {err}"), + } +} +``` + +Servers then only stop when `main.rs` explicitly sends `Halted::Normal` via +the halt channel. + +**Also update each server job starter** to watch the `CancellationToken` and +forward the halt signal internally (Q2 decision, Option 3): + +```rust +// e.g. src/bootstrap/jobs/udp_tracker.rs — after this change: +tokio::spawn(async move { + tokio::select! { + _ = cancellation_token.cancelled() => { + // JobManager signalled shutdown — forward to server via halt channel + let _ = server.state.halt_task.send(Halted::Normal); + server.state.task.await.expect("..."); + } + _ = &mut server.state.task => { + // Server finished on its own + } + } +}) +``` + +The same pattern applies to HTTP tracker, REST API, and Health Check API job +starters. This requires adding a `CancellationToken` parameter to each +`start_job` function (or threading it through from `app.rs`). + +See [Q2 decision](../../features/shutdown-process/open-questions.md#q2) for full +rationale. + +## Important Considerations + +### External package dependency + +`torrust_server_lib` is an external standalone crate, not part of this workspace. +Changes to `shutdown_signal()` require a coordinated release of `torrust-server-lib` +and a version bump in this workspace's `Cargo.toml`. + +### Orphan risk on `main.rs` crash + +If `main.rs` crashes or is SIGKILL'd before sending halt messages, servers +become orphaned (ports stay open, process appears dead). See Q5 for the +strategy decision on this risk. + +### Impact on standalone binary consumers + +The examples `http_only_public_tracker.rs` and `udp_only_public_tracker.rs` use +`global_shutdown_signal()` indirectly via the server's `shutdown_signal()`. After +this change, those examples must be updated to send halt signals explicitly — or +they can be updated independently in SI-3. + +## Acceptance Criteria + +- [ ] Q1, Q2, and Q5 are resolved before implementation begins. +- [ ] `shutdown_signal()` in `torrust-server-lib` no longer calls + `global_shutdown_signal()`. +- [ ] Each server job starter accepts and uses a `CancellationToken` to + forward the halt signal to its server when `jobs.cancel()` is called. +- [ ] Servers only stop when the halt channel receives `Halted::Normal`. +- [ ] `main.rs` does not need to hold or send halt senders directly — + `jobs.cancel()` is the only call needed. +- [ ] All existing server start/stop tests pass. +- [ ] Experimental validation: `kill ` (after SI-1) still shuts down + cleanly with a single shutdown sequence in the logs (no duplicate + "halting" messages per server). + +## Dependencies + +- SI-1 must land first. +- Requires `torrust-server-lib` to be updated and released. +- Q1 is resolved: SI-1 and SI-2 are sequential; SI-1 lands first. +- Q5 (orphan risk) must be resolved before this sub-issue can land. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +```bash +cargo build --release +RUST_LOG=info ./target/release/torrust-tracker +``` + +### Test 1: Single shutdown sequence in the logs (no duplicate "halting" messages) + +Send `kill ` (SIGTERM) to the tracker. + +**Expected**: Each server logs exactly **one** halt message, not two: + +```text +# CORRECT (one message per server): +INFO Shutting down HTTP server on socket address: 0.0.0.0:7070 + +# WRONG (two messages per server — indicates double-signal still present): +WARN caught interrupt signal (ctrl-c), halting... +INFO Shutting down HTTP server on socket address: 0.0.0.0:7070 +``` + +**Record in `verification.md`**: full shutdown log showing absence of +`global_shutdown_signal` messages. + +### Test 2: Ctrl+C also produces a single shutdown sequence + +Send SIGINT (Ctrl+C). Confirm the same — no duplicate halt messages per server. + +### Test 3: Orphan risk validation + +Send `kill -9 ` to force-kill the main process. + +**Expected**: All server ports (`6868`, `6969`, `7070`, `7171`, `1212`, `1313`) +are freed within a few seconds (the OS reclaims them when the process group exits). + +```bash +sleep 2 && lsof -i :7070,6969,1212 | grep LISTEN +``` + +Output should be empty. **Record any ports that remain open.** + +### Test 4: Restart succeeds immediately after clean shutdown + +After a graceful shutdown (SIGTERM or SIGINT), restart the tracker immediately. + +**Expected**: All services bind to their ports without "address already in use" errors. diff --git a/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/verification.md b/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-2-remove-global-shutdown-signal/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-3-fix-environment-stop/ISSUE.md b/docs/issues/drafts/1488-si-3-fix-environment-stop/ISSUE.md new file mode 100644 index 000000000..482a7f8a3 --- /dev/null +++ b/docs/issues/drafts/1488-si-3-fix-environment-stop/ISSUE.md @@ -0,0 +1,173 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/1488-si-3-fix-environment-stop/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/axum-http-server/src/testing/environment.rs + - packages/udp-server/src/testing/environment.rs + - packages/axum-http-server/examples/http_only_public_tracker.rs + - packages/udp-server/examples/udp_only_public_tracker.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/features/shutdown-process/open-questions.md +--- + + + +# Draft SI-3 — Fix `Environment::stop()` in Standalone Library Examples + +> **EPIC position**: SI-3 of #1488. Depends on Q1 resolution. +> ✅ **Unblocked** — Q1 is resolved. See the [Q1 decision](../../features/shutdown-process/open-questions.md#q1): +> SI-3 and SI-2 are sequential; this sub-issue can land independently of SI-2. + +## Goal + +Fix two problems in the standalone library usage examples: + +1. **Event listeners are `abort()`ed instead of gracefully stopped** — the + `CancellationToken` in `Environment` exists but is never `cancel()`led during + shutdown. In-flight statistics events are silently lost. +2. **Only `SIGINT` is handled** — both examples only call `ctrl_c()`, not + `SIGTERM`. `docker stop` and `kill` are ignored. + +## Background + +Both example binaries follow this pattern: + +```rust +tokio::signal::ctrl_c().await.expect("failed to install Ctrl-C handler"); +env.stop().await; +``` + +`Environment::stop()` currently aborts event listeners: + +```rust +// todo: send a message to the event listener to stop and wait for it to finish +event_listener_job.abort(); +``` + +The `CancellationToken` stored in `Environment` is created but `cancel()` is +never called on it. This is an explicit known issue (the `TODO` comments call it out). + +The affected files are: + +- `packages/axum-http-server/src/testing/environment.rs` +- `packages/udp-server/src/testing/environment.rs` +- `packages/axum-http-server/examples/http_only_public_tracker.rs` +- `packages/udp-server/examples/udp_only_public_tracker.rs` + +## Implementation + +### Fix 1: Use `CancellationToken` in `Environment::stop()` + +Change event listener shutdown from `abort()` to graceful cancel + await: + +```rust +pub async fn stop(self) -> Environment { + // Cancel all event listeners via the shared token + self.cancellation_token.cancel(); + + // Wait for each listener to finish (instead of abort) + if let Some(job) = self.event_listener_job { + let _ = job.await; + } + + let server = self.server.stop().await.expect("..."); + ... +} +``` + +### Fix 2: Handle `SIGTERM` in the example binaries + +```rust +#[cfg(unix)] +let mut sigterm = tokio::signal::unix::signal( + tokio::signal::unix::SignalKind::terminate() +).expect("failed to install SIGTERM handler"); + +tokio::select! { + _ = tokio::signal::ctrl_c() => {} + #[cfg(unix)] + _ = sigterm.recv() => {} +} + +env.stop().await; +``` + +## Acceptance Criteria + +- [ ] `event_listener_job.abort()` is replaced with `cancel()` + `await` in both + `axum-http-server` and `udp-server` environment `stop()` methods. +- [ ] The `TODO` comments about graceful event listener shutdown are resolved. +- [ ] Both example binaries handle `SIGTERM` in addition to `SIGINT`. +- [ ] `kill ` against a running example binary shuts it down cleanly. +- [ ] `linter all` passes. + +## Dependencies + +- Q1 is resolved: this sub-issue can land independently of SI-1 and SI-2. + The standalone examples use `env.stop()` directly — they do not depend on + the `global_shutdown_signal()` removal in SI-2. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +Build and run each example binary: + +```bash +# HTTP example +cargo run -p torrust-tracker-axum-http-server --example http_only_public_tracker + +# UDP example +cargo run -p torrust-tracker-udp-server --example udp_only_public_tracker +``` + +Note the PID of the **example binary** (not cargo). + +### Test 1: `kill ` shuts down HTTP example gracefully (SIGTERM) + +Run the HTTP example and send `kill `. + +**Expected**: + +- The example logs a shutdown message. +- The process exits cleanly (exit code 0). +- No "Killed" message from the OS. + +**Record in `verification.md`**: full output including shutdown messages. + +### Test 2: `kill ` shuts down UDP example gracefully (SIGTERM) + +Repeat Test 1 for the UDP example. + +### Test 3: Event listeners are cancelled, not aborted + +Verify that the statistics event listener shutdown message is logged (not just +silently killed). If event listeners log a shutdown message, they were cancelled +gracefully. If there is no log message, they were aborted. + +**Expected**: a log line like `Stopping ... event listener` or `... receiver closed` +appears during `env.stop()`. + +### Test 4: `TODO` comments are removed + +Search the codebase for the `TODO` comments about graceful event listener shutdown +and confirm they are removed: + +```bash +grep -rn 'todo: send a message to the event listener' packages/ +``` + +Output should be empty. diff --git a/docs/issues/drafts/1488-si-3-fix-environment-stop/verification.md b/docs/issues/drafts/1488-si-3-fix-environment-stop/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-3-fix-environment-stop/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/ISSUE.md b/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/ISSUE.md new file mode 100644 index 000000000..9a7ab98f3 --- /dev/null +++ b/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/ISSUE.md @@ -0,0 +1,141 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - src/bootstrap/jobs/torrent_cleanup.rs + - src/bootstrap/jobs/manager.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md +--- + + + +# Draft SI-4 — Migrate Torrent Cleanup Job to `CancellationToken` + +> **EPIC position**: SI-4 of #1488. Independent — no blockers. + +## Goal + +Replace the direct `tokio::signal::ctrl_c()` listener in the torrent cleanup job +with the shared `CancellationToken` from `JobManager`. This makes the job respond +to `jobs.cancel()` and removes a direct signal dependency that bypasses the +centralized shutdown coordinator. + +## Background + +`src/bootstrap/jobs/torrent_cleanup.rs` currently uses: + +```rust +tokio::select! { + _ = tokio::signal::ctrl_c() => { + tracing::info!("Stopping torrent cleanup job ..."); + break; + } + _ = interval.tick() => { ... } +} +``` + +This job does **not** respond to `jobs.cancel()` — it only stops when Ctrl+C is +pressed. After SI-1 adds `SIGTERM` to `main.rs`, this job will still not stop +on SIGTERM because it listens for Ctrl+C directly. + +See [analysis §3.2 and §7.2](../../analysis/20260716-shutdown-process/README.md). + +## Implementation + +Pass the `CancellationToken` into `start_job` and use it in the loop: + +```rust +pub fn start_job( + config: &Core, + torrents_manager: &Arc, + cancellation_token: CancellationToken, // new parameter +) -> JoinHandle<()> { + tokio::spawn(async move { + loop { + tokio::select! { + _ = cancellation_token.cancelled() => { + tracing::info!("Stopping torrent cleanup job ..."); + break; + } + _ = interval.tick() => { ... } + } + } + }) +} +``` + +In `src/app.rs`, pass `job_manager.new_cancellation_token()` when calling +`start_torrent_cleanup`. + +## Acceptance Criteria + +- [ ] The `ctrl_c()` call is removed from `torrent_cleanup.rs`. +- [ ] The job stops when `jobs.cancel()` is called (i.e., responds to SIGTERM + after SI-1, not just SIGINT). +- [ ] The job receives the `CancellationToken` as a parameter. +- [ ] `src/app.rs` passes the token when starting the job. +- [ ] `cargo test` passes. +- [ ] `linter all` passes. + +## Dependencies + +- No hard prerequisites. Can land independently. +- Benefits from SI-1 being landed first (SIGTERM will then also stop this job). + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +```bash +cargo build --release +RUST_LOG=info ./target/release/torrust-tracker +``` + +### Test 1: No direct `ctrl_c()` call in source + +```bash +grep -rn 'ctrl_c' src/bootstrap/jobs/torrent_cleanup.rs +``` + +**Expected**: no matches. + +### Test 2: Torrent cleanup job stops on graceful shutdown + +Send `kill -INT ` (or Ctrl+C if SI-1 is not yet landed) to the tracker. + +**Expected log** (in RUST_LOG=info output): + +```text +INFO Stopping torrent cleanup job ... +``` + +This message confirms the job's cancellation path ran. If it is absent, the job +was aborted rather than cancelled. + +**Record in `verification.md`**: the log line showing the torrent cleanup job +stopped gracefully. + +### Test 3: Torrent cleanup stops on SIGTERM (requires SI-1) + +If SI-1 has been merged, send `kill ` (SIGTERM). + +**Expected**: same `Stopping torrent cleanup job ...` message appears. + +### Test 4: `ctrl_c()` removal does not break other jobs + +After Ctrl+C, confirm all other jobs also shut down as expected (no regressions). +Verify that the `JobManager` reports all jobs completing gracefully. diff --git a/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/verification.md b/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-4-migrate-torrent-cleanup/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/ISSUE.md b/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/ISSUE.md new file mode 100644 index 000000000..bcba05276 --- /dev/null +++ b/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/ISSUE.md @@ -0,0 +1,159 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs + - src/bootstrap/jobs/activity_metrics_updater.rs + - src/bootstrap/jobs/manager.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md +--- + + + +# Draft SI-5 — Migrate Activity Metrics Updater to `CancellationToken` + +> **EPIC position**: SI-5 of #1488. Independent — no blockers. + +## Goal + +Replace the direct `tokio::signal::ctrl_c()` listener in the peers activity +metrics updater with the shared `CancellationToken` from `JobManager`. This makes +the job respond to `jobs.cancel()` and removes a direct signal dependency. + +## Background + +`packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs` +currently uses: + +```rust +tokio::select! { + _ = tokio::signal::ctrl_c() => { + tracing::info!("Stopping peers activity metrics update job (ctrl-c signal received) ..."); + break; + } + _ = interval.tick() => { ... } +} +``` + +This job does **not** respond to `jobs.cancel()`. The interval is also hardcoded +at 15 seconds (a separate known issue noted in the code with a `todo:`). + +See [analysis §3.2 and §7.2](../../analysis/20260716-shutdown-process/README.md). + +## Implementation + +The `start_job` function in the `swarm-coordination-registry` package currently +has this signature: + +```rust +pub fn start_job( + swarms: &Arc, + stats_repository: &Arc, + inactivity_cutoff: DurationSinceUnixEpoch, +) -> JoinHandle<()> +``` + +Add a `CancellationToken` parameter: + +```rust +pub fn start_job( + swarms: &Arc, + stats_repository: &Arc, + inactivity_cutoff: DurationSinceUnixEpoch, + cancellation_token: CancellationToken, // new parameter +) -> JoinHandle<()> +``` + +In the loop, replace `ctrl_c()` with: + +```rust +tokio::select! { + _ = cancellation_token.cancelled() => { + tracing::info!("Stopping peers activity metrics update job ..."); + break; + } + _ = interval.tick() => { ... } +} +``` + +Update the call site in `src/bootstrap/jobs/activity_metrics_updater.rs` to +pass `job_manager.new_cancellation_token()`. + +## Acceptance Criteria + +- [ ] The `ctrl_c()` call is removed from `activity_metrics_updater.rs`. +- [ ] The job stops when `jobs.cancel()` is called. +- [ ] The `CancellationToken` is passed from `JobManager` through + `src/bootstrap/jobs/activity_metrics_updater.rs`. +- [ ] `cargo test` passes. +- [ ] `linter all` passes. + +## Dependencies + +- No hard prerequisites. Can land independently. +- Benefits from SI-1 being landed first (SIGTERM will then also stop this job). +- Note: the hardcoded 15s interval has a `TODO` comment — that is a separate + concern and not in scope for this sub-issue. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +```bash +cargo build --release +RUST_LOG=info ./target/release/torrust-tracker +``` + +### Test 1: No direct `ctrl_c()` call in source + +```bash +grep -rn 'ctrl_c' packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs +``` + +**Expected**: no matches. + +### Test 2: Activity metrics updater stops on graceful shutdown + +Send Ctrl+C (or `kill -INT `) to the tracker. Look for the updater's stop +message in the logs. + +**Expected log**: + +```text +INFO Stopping peers activity metrics update job ... +``` + +If this message does not appear, the job was aborted rather than cancelled. + +**Record in `verification.md`**: the log line confirming graceful stop. + +### Test 3: Activity metrics updater stops on SIGTERM (requires SI-1) + +If SI-1 has been merged, send `kill ` (SIGTERM). + +**Expected**: same stop message appears. + +### Test 4: Activity metrics are still collected during normal operation + +Run the tracker for at least 30 seconds (two 15s intervals) and confirm that +metrics update log messages appear: + +```bash +RUST_LOG=debug ./target/release/torrust-tracker 2>&1 | grep 'activity_metrics' +``` + +Verify that activity metrics are still being computed and logged before shutdown. diff --git a/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/verification.md b/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-5-migrate-activity-metrics-updater/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-6-align-grace-periods/ISSUE.md b/docs/issues/drafts/1488-si-6-align-grace-periods/ISSUE.md new file mode 100644 index 000000000..db8e511e2 --- /dev/null +++ b/docs/issues/drafts/1488-si-6-align-grace-periods/ISSUE.md @@ -0,0 +1,187 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/1488-si-6-align-grace-periods/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - src/main.rs + - src/bootstrap/jobs/manager.rs + - packages/axum-server/src/signals.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md + - docs/features/shutdown-process/open-questions.md +--- + + + +# Draft SI-6 — Align `JobManager` Grace Period with Axum Server Timeout + +> **EPIC position**: SI-6 of #1488. +> **Blocked**: Q4 must be resolved — the correct target grace period depends +> on the Docker/Kubernetes deployment context decision. + +## Goal + +Fix the mismatch between the `JobManager`'s per-job timeout (10s) and the Axum +server's graceful shutdown grace period (90s) so that the main process actually +waits long enough for HTTP connections to drain before exiting. + +## Background + +**Current state** (confirmed experimentally): + +- `main.rs` calls `jobs.wait_for_all(Duration::from_secs(10))` — 10 seconds per + job, sequentially. +- Axum servers (HTTP tracker, REST API, Health Check API) have an internal grace + period of 90 seconds (`graceful_shutdown(Some(Duration::from_secs(90)))`). + +Because the `JobManager` times out after 10s per job, the main process exits +before the Axum 90s drain period completes. The Axum drain task keeps running +as an orphan but the process has already exited — connections are force-dropped. + +See [analysis §5.1 and §7.3](../../analysis/20260716-shutdown-process/README.md). + +**The tension with Docker**: + +Docker's default `stop_grace_period` is 10s. If we raise the `JobManager` timeout +to match the Axum 90s, Docker will SIGKILL the container before the tracker +finishes draining. Operators must configure `stop_grace_period` appropriately. +See Q4 in open-questions.md. + +## Implementation + +**Step 1**: Reduce the Axum server grace period to a value shorter than the +`JobManager` total timeout. A good target: + +```rust +// packages/axum-server/src/signals.rs +let grace_period = Duration::from_secs(25); // was 90s +let max_wait = Duration::from_secs(30); // was 95s +``` + +**Step 2**: Raise the `JobManager` grace period to give enough time for all jobs +(including the Axum drain): + +```rust +// src/main.rs +jobs.wait_for_all(Duration::from_secs(30)).await; // was 10s +``` + +**Step 3**: Change `wait_for_all` to wait concurrently rather than sequentially +(each job currently gets 10s regardless of what others are doing): + +```rust +// src/bootstrap/jobs/manager.rs +pub async fn wait_for_all(mut self, grace_period: Duration) { + let handles: Vec<_> = self.jobs.drain(..).collect(); + let futures = handles.into_iter().map(|job| { + let name = job.name.clone(); + async move { + match timeout(grace_period, job.handle).await { + Ok(Ok(())) => info!(job = %name, "Job completed gracefully"), + Ok(Err(e)) => warn!(job = %name, "Job returned an error: {:?}", e), + Err(_) => warn!(job = %name, "Job did not complete in time"), + } + } + }); + futures::future::join_all(futures).await; +} +``` + +## Acceptance Criteria + +- [ ] Q4 is resolved and the target grace period is agreed. +- [ ] `jobs.wait_for_all()` waits concurrently, not sequentially. +- [ ] The Axum internal grace period is ≤ `JobManager` total timeout. +- [ ] `main.rs` uses the agreed total timeout. +- [ ] When `kill -INT ` is sent, all Axum servers drain their connections + (log shows "All connections closed") before `main.rs` exits. +- [ ] Deployment documentation (`docs/containers.md`) notes the recommended + Docker `stop_grace_period` configuration. + +## Dependencies + +- Q4 (grace period decision) must be resolved. +- Can land after SI-1 or independently; the grace period alignment is correct + behavior regardless of which signal triggered the shutdown. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Setup + +```bash +cargo build --release +RUST_LOG=info ./target/release/torrust-tracker +``` + +### Test 1: All Axum servers drain connections before main exits + +With the tracker running, open one or more idle HTTP connections (e.g., with +`curl` or `telnet`) and then send `kill -INT `. + +```bash +# Keep a connection alive in background +curl -v --no-progress-meter http://127.0.0.1:7070/health_check & + +# Shutdown +kill -INT +``` + +**Expected log sequence**: + +```text +INFO graceful_shutdown: !! Shutting down HTTP server ... in 25 seconds !! +INFO graceful_shutdown: All connections closed, shutting down server in address ... +INFO Job completed gracefully job=http_instance_0_0.0.0.0:7070 +INFO Torrust tracker successfully shutdown. +``` + +The key requirement is that `All connections closed` appears **before** +`successfully shutdown` — the main process waited for HTTP drain to complete. + +**Record in `verification.md`**: timestamped log output showing the sequence. + +### Test 2: Jobs are waited concurrently + +Confirm that the shutdown log does **not** show sequential one-by-one waits +but instead shows jobs completing in parallel. Compare timestamps: + +```text +# Sequential (WRONG): timestamps are separated by ~10s each +2026-07-16T10:00:00 INFO Waiting for job ... job=health_check_api +2026-07-16T10:00:10 INFO Waiting for job ... job=http_api + +# Concurrent (CORRECT): timestamps are clustered together +2026-07-16T10:00:00 INFO Waiting for 9 jobs to finish (timeout: 30s) +2026-07-16T10:00:01 INFO Job completed gracefully job=health_check_api +2026-07-16T10:00:01 INFO Job completed gracefully job=http_api +``` + +**Record in `verification.md`**: log output with timestamps showing concurrent completion. + +### Test 3: Docker stop completes without SIGKILL + +```bash +docker run -d --name torrust-test torrust/tracker:dev +# Make one HTTP request to ensure a connection is open +curl http://localhost:7070/health_check & +docker stop torrust-test # default 10s timeout +docker logs torrust-test | tail -5 +``` + +**Expected**: `docker stop` returns before the timeout. The last log line shows +`successfully shutdown`, not a killed/aborted message. + +**Note**: If the default 10s is too short, document the observed timing and the +recommended `stop_grace_period` in `verification.md`. diff --git a/docs/issues/drafts/1488-si-6-align-grace-periods/verification.md b/docs/issues/drafts/1488-si-6-align-grace-periods/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-6-align-grace-periods/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-7-observable-shutdown-progress/ISSUE.md b/docs/issues/drafts/1488-si-7-observable-shutdown-progress/ISSUE.md new file mode 100644 index 000000000..2b60ccad8 --- /dev/null +++ b/docs/issues/drafts/1488-si-7-observable-shutdown-progress/ISSUE.md @@ -0,0 +1,145 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p3 +github-issue: null +spec-path: docs/issues/drafts/1488-si-7-observable-shutdown-progress/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - src/bootstrap/jobs/manager.rs + - src/main.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md +--- + + + +# Draft SI-7 — Implement Observable Shutdown Progress in `JobManager` + +> **EPIC position**: SI-7 of #1488. Independent — no blockers. +> Pairs naturally with SI-6 (concurrent waiting). + +## Goal + +During shutdown, log which jobs are still running so operators and developers can +tell what is blocking the tracker from exiting. Currently only a single `WARN` is +logged if a job times out, with no periodic progress reporting. + +## Background + +`src/bootstrap/jobs/manager.rs` currently logs: + +```text +INFO Waiting for job to finish (timeout of 10 seconds) ... job=http_tracker_0 +WARN Job did not complete in time job=http_tracker_0 +``` + +There is no visibility into what is happening while waiting — no active connection +count, no periodic "still waiting" message, no final summary. This makes it hard +to diagnose shutdown hangs in production or CI. + +## Desired Output + +```text +INFO Torrust tracker shutting down (SIGTERM) ... +INFO Waiting for 9 jobs to finish (timeout: 30s) ... +INFO Waiting for jobs: http_tracker_0, http_tracker_1, udp_tracker_0 ... (6 others done) +INFO All jobs finished. Shutdown complete. +``` + +Or when a job times out: + +```text +WARN Shutdown timeout reached. Jobs still running: http_tracker_0 (3 active connections) +INFO Exiting. +``` + +## Implementation + +Options: + +1. **Periodic log loop** — spawn a task during `wait_for_all` that logs + remaining job names every N seconds until all are done. +2. **Final summary** — after `join_all` completes, log which jobs finished vs + timed out. +3. **Both** — periodic progress + final summary. + +The minimal viable implementation is option 2 (final summary). Option 1 requires +SI-6's concurrent waiting to be useful. + +## Acceptance Criteria + +- [ ] On graceful shutdown, a final summary log lists each job and its outcome + (completed / timed out). +- [ ] If any job times out, the log message includes the job name. +- [ ] The shutdown start message logs the total number of jobs and the timeout. +- [ ] `linter all` passes. + +## Dependencies + +- No hard prerequisites. Can land independently. +- Most useful after SI-6 (concurrent waiting), since sequential waiting makes + progress harder to interpret. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Test 1: Shutdown start message includes job count and timeout + +Start the tracker and send Ctrl+C. Confirm the first shutdown log line includes +the number of jobs and the grace period: + +**Expected** (exact wording may differ): + +```text +INFO Torrust tracker shutting down ... +INFO Waiting for 9 jobs to finish (timeout: 30s) ... +``` + +**Record in `verification.md`**: the exact log output. + +### Test 2: Final summary lists all jobs + +After all jobs complete, confirm a summary is logged: + +**Expected** (clean shutdown): + +```text +INFO All jobs finished. Shutdown complete. +``` + +Or with timed-out jobs: + +```text +WARN Job did not complete in time job=http_instance_0_0.0.0.0:7070 +INFO Shutdown complete (1 job timed out). +``` + +### Test 3: Timed-out job is named + +To trigger a timeout, artificially hold an HTTP connection open during shutdown +(while using a short grace period). Confirm the timeout warning names the job. + +**Expected**: + +```text +WARN Job did not complete in time job=http_instance_0_0.0.0.0:7070 +``` + +**Not acceptable**: + +```text +WARN Job did not complete in time +``` + +(job name missing) + +**Record in `verification.md`**: the warning log line including the job name. diff --git a/docs/issues/drafts/1488-si-7-observable-shutdown-progress/verification.md b/docs/issues/drafts/1488-si-7-observable-shutdown-progress/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-7-observable-shutdown-progress/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-8-configurable-grace-periods/ISSUE.md b/docs/issues/drafts/1488-si-8-configurable-grace-periods/ISSUE.md new file mode 100644 index 000000000..df7d0408a --- /dev/null +++ b/docs/issues/drafts/1488-si-8-configurable-grace-periods/ISSUE.md @@ -0,0 +1,151 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p3 +github-issue: null +spec-path: docs/issues/drafts/1488-si-8-configurable-grace-periods/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/configuration/src/v3_0_0/ + - src/main.rs + - src/bootstrap/jobs/manager.rs + - packages/axum-server/src/signals.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/features/shutdown-process/open-questions.md +--- + + + +# Draft SI-8 — Add Configurable Grace Periods (`[shutdown]` Config Section) + +> **EPIC position**: SI-8 of #1488. +> **Blocked**: Q3 (exit codes) and Q4 (grace period alignment decision) must +> be resolved before the config schema can be finalized. + +## Goal + +Replace the hardcoded grace period constants with a `[shutdown]` configuration +section so operators can tune the shutdown timeout to match their deployment +environment (Docker, Kubernetes, systemd, etc.). + +## Background + +Grace periods are currently hardcoded in two places: + +- `src/main.rs`: `jobs.wait_for_all(Duration::from_secs(10))` +- `packages/axum-server/src/signals.rs`: + `let grace_period = Duration::from_secs(90);` + `let max_wait = Duration::from_secs(95);` + +These magic numbers cannot be tuned by operators. For example: + +- Docker's default `stop_grace_period` is 10s — operators who want graceful + drain must increase it, but they also need to increase the tracker's own + timeout to match. +- Kubernetes `terminationGracePeriodSeconds` defaults to 30s. +- Systemd `TimeoutStopSec` defaults to 90s on most distros. + +## Proposed Configuration Schema + +```toml +[shutdown] +# Total time the tracker will wait for all jobs to finish before forcing exit. +# Set this lower than your container/service manager's own grace period. +# Default: 30s +grace_period_secs = 30 + +# Time each Axum server waits for active HTTP connections to finish. +# Must be < grace_period_secs to ensure the process exits within the total budget. +# Default: 25s +connection_drain_secs = 25 +``` + +## Implementation + +1. Add `Shutdown` struct to `packages/configuration/src/v3_0_0/`. +2. Add optional `shutdown: Option` field to `Configuration`. +3. Pass the config through the bootstrap to `start_job` calls and `wait_for_all`. +4. Use the config values in `main.rs` and `packages/axum-server/src/signals.rs`. + +## Acceptance Criteria + +- [ ] Q3 and Q4 are resolved. +- [ ] A `[shutdown]` section is added to the configuration schema (v3.0.0). +- [ ] `grace_period_secs` controls the `JobManager` timeout. +- [ ] `connection_drain_secs` controls the Axum server drain timeout. +- [ ] Default values produce the same behavior as the current hardcoded values + (or the improved values agreed in SI-6). +- [ ] Config documentation is updated. +- [ ] `linter all` passes. + +## Dependencies + +- Q3 (exit codes) and Q4 (grace period target values) must be resolved. +- Should land after SI-6 (which sets the correct non-configurable defaults). +- Coordinates with the Configuration Overhaul EPIC (#1978) for schema placement. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Test 1: Default values produce correct behavior + +Run the tracker **without** a `[shutdown]` section in the config. Confirm that +default values are used and the shutdown behavior matches the behavior from SI-6. + +**Record in `verification.md`**: log output confirming the default timeout value +is logged at startup (e.g., `INFO Shutdown grace period: 30s`). + +### Test 2: Custom `grace_period_secs` is respected + +Add to the config: + +```toml +[shutdown] +grace_period_secs = 5 +``` + +Start the tracker and hold open an HTTP connection during shutdown. The tracker +should time out the job after 5 seconds. + +**Expected**: + +```text +WARN Job did not complete in time job=http_instance_0_... +``` + +Time the shutdown from SIGINT to process exit — it should be approximately 5s. + +**Record in `verification.md`**: timing evidence (timestamps from log). + +### Test 3: Custom `connection_drain_secs` is respected + +Add to the config: + +```toml +[shutdown] +connection_drain_secs = 3 +``` + +Hold an HTTP connection open and send Ctrl+C. The Axum server should close +connections after 3 seconds and report its drain timeout. + +### Test 4: Invalid config values are rejected at startup + +Set `connection_drain_secs` > `grace_period_secs` (logically invalid). Confirm +the tracker refuses to start with a clear error message. + +**Expected**: startup error mentioning the invalid relationship. + +### Test 5: Config documentation is accurate + +Read `share/default/config/tracker.development.sqlite3.toml` and any +documentation added for `[shutdown]`. Confirm the documented defaults match +the actual behavior observed in Tests 1–3. diff --git a/docs/issues/drafts/1488-si-8-configurable-grace-periods/verification.md b/docs/issues/drafts/1488-si-8-configurable-grace-periods/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-8-configurable-grace-periods/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/drafts/1488-si-9-improve-udp-shutdown/ISSUE.md b/docs/issues/drafts/1488-si-9-improve-udp-shutdown/ISSUE.md new file mode 100644 index 000000000..8ecfce2e1 --- /dev/null +++ b/docs/issues/drafts/1488-si-9-improve-udp-shutdown/ISSUE.md @@ -0,0 +1,157 @@ +--- +doc-type: issue +issue-type: task +status: draft +priority: p3 +github-issue: null +spec-path: docs/issues/drafts/1488-si-9-improve-udp-shutdown/ISSUE.md +branch: null +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/src/server/states.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/analysis/20260716-shutdown-process/README.md +--- + + + +# Draft SI-9 — Improve UDP Server Shutdown + +> **EPIC position**: SI-9 of #1488. Lowest priority. Independent. + +## Goal + +Improve the UDP server shutdown to be more observable and, where practical, to +avoid dropping in-flight UDP requests during shutdown. + +## Background + +The UDP server (`packages/udp-server/src/server/launcher.rs`) shuts down by +aborting its main loop: + +```rust +select! { + _ = running => { ... }, + _ = halt_task => { ... } +} +stop.abort(); // Force-abort the main loop task +tokio::task::yield_now().await; // Give other tasks a chance to run +``` + +There is no connection draining mechanism — in-flight UDP requests are dropped +silently. + +See [analysis §5.2 and §7.8](../../analysis/20260716-shutdown-process/README.md). + +## Important Context: UDP is Stateless + +UDP is fire-and-forget at the protocol level. BitTorrent UDP clients: + +- Expect responses on a best-effort basis. +- Retry automatically if no response arrives. +- Do not hold long-lived connections. + +This means a dropped UDP request during shutdown is **much less severe** than +a dropped HTTP connection. The client will retry on the next tracker (if multiple +trackers are configured) or on the next announce interval. + +The actual improvement is therefore primarily about **observability**, not about +preventing data loss. + +## Implementation Options + +### Option A: Log in-flight requests on shutdown (minimal) + +Before aborting the main loop, log the number of requests in the buffer: + +```rust +tracing::info!( + "UDP server shutting down with {} requests in flight", + active_requests.len() +); +stop.abort(); +``` + +### Option B: Drain the request buffer before stopping (more complete) + +Process all requests already in the socket buffer before stopping. This requires: + +1. Stop accepting new UDP packets (close the socket for reads). +2. Process any already-buffered packets. +3. Then stop. + +This is more complex and may not be worth the effort given UDP retry behavior. + +## Recommendation + +Implement **Option A** first as it is low risk and adds observability. Revisit +Option B if operator feedback indicates that in-flight UDP request drops are +causing measurable peer-tracking inconsistencies. + +## Acceptance Criteria + +- [ ] On shutdown, the UDP server logs the number of in-flight requests (if any). +- [ ] The abort approach is documented as intentional with a note about UDP + retry semantics. +- [ ] `linter all` passes. + +## Dependencies + +- No hard prerequisites. Can land independently. +- Lower priority than SI-1 through SI-6. + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Test 1: In-flight request count is logged on shutdown + +Start the tracker with UDP enabled. Use the tracker client to send a burst of +UDP requests, then immediately send Ctrl+C. + +```bash +# Send several UDP announce requests rapidly +for i in $(seq 1 10); do + cargo run -p torrust-tracker-client -- udp announce \ + udp://127.0.0.1:6969 aabbccddeeff00112233445566778899aabbccdd & +done + +# Immediately shut down +kill -INT +``` + +**Expected**: a log line like: + +```text +INFO UDP server shutting down with N requests in flight +``` + +(even if N is 0 in practice, the log line must be present) + +**Record in `verification.md`**: the log line. + +### Test 2: Abort is documented as intentional + +Search the code for the `stop.abort()` call and confirm there is a comment +explaining that UDP abort is intentional due to protocol retry semantics: + +```bash +grep -n 'abort' packages/udp-server/src/server/launcher.rs +``` + +**Expected**: the `abort()` call has an adjacent comment explaining the decision. + +### Test 3: UDP clients retry after tracker restart + +Shut down the tracker during active UDP traffic and confirm that clients +(BitTorrent peers) reconnect and resume normal operation after the tracker +restarts. Use the checker/monitor tool if available, or observe from logs +after restart. + +**Note**: This is a best-effort test. UDP retry behavior is client-dependent. diff --git a/docs/issues/drafts/1488-si-9-improve-udp-shutdown/verification.md b/docs/issues/drafts/1488-si-9-improve-udp-shutdown/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/drafts/1488-si-9-improve-udp-shutdown/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md b/docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md new file mode 100644 index 000000000..7401ebecd --- /dev/null +++ b/docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md @@ -0,0 +1,152 @@ +--- +doc-type: epic +status: open +github-issue: 1488 +spec-path: docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md +epic-owner: josecelano +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/analysis/20260716-shutdown-process/README.md + - docs/features/shutdown-process/README.md + - src/main.rs + - src/bootstrap/jobs/manager.rs + - src/bootstrap/jobs/torrent_cleanup.rs + - src/bootstrap/jobs/activity_metrics_updater.rs + - packages/axum-server/src/signals.rs + - packages/udp-server/src/server/launcher.rs + - packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs + - docs/research/20260716-console-shutdown-patterns/README.md +--- + + + +# EPIC #1488 - Overhaul: Tracker Shutdown + +## Goal + +Bring the Torrust Tracker into compliance with the **Unix and container process +lifecycle contracts** — the well-proven standards that govern how a long-running +service is expected to stop. Then, as a second step, normalize and clean up the +internal shutdown implementation so all jobs follow a single consistent pattern. + +The primary goal is **correctness and lack of surprise**: every standard stop +mechanism (`kill`, `docker stop`, `systemctl stop`, Kubernetes pod termination) +should trigger a graceful shutdown, exactly as any operator or automation tool +would expect. + +The secondary goal is **internal consistency**: standardize all jobs to use the +`CancellationToken` from `JobManager`, remove direct `ctrl_c` listeners from +individual jobs, and align timeouts so shutdown actually completes cleanly. + +## Why This Is Needed + +The current shutdown process has several problems identified in the +[shutdown analysis](../../analysis/20260716-shutdown-process/README.md): + +1. **No `SIGTERM` in `main.rs`** — only `SIGINT` (Ctrl+C) is handled at the top + level. Container orchestrators (Docker/Podman) send `SIGTERM` by default, + which means `jobs.cancel()` and `jobs.wait_for_all()` are never called. +2. **Three inconsistent shutdown mechanisms** — jobs use `CancellationToken`, + direct `tokio::signal::ctrl_c()`, or oneshot `Halted` channels. Some jobs + ignore the central `JobManager` entirely. +3. **Torrent cleanup and activity metrics ignore `CancellationToken`** — they + listen for `ctrl_c` directly instead of using the shared token. +4. **Grace period mismatch** — `JobManager` waits 10s per job sequentially, + while Axum servers have a 90s graceful shutdown. The main process may exit + before servers finish draining connections. +5. **No graceful UDP shutdown** — the UDP server simply aborts its main loop. +6. **Hardcoded timeouts** — grace periods are magic numbers with no configuration + surface. +7. **No observable shutdown progress** — operators cannot tell which job is + blocking shutdown. +8. **Double-signal on Ctrl+C** — both `main.rs` and each server's + `global_shutdown_signal()` catch the same signal, creating a potential race. + +## The Contracts Being Implemented + +These are not new features — they are standard behaviors that every process +manager, container runtime, and operator already expects: + +```bash +# These all SHOULD work — and currently DON'T (except Ctrl+C): +kill # SIGTERM — currently ignored ❌ +docker stop # SIGTERM — currently ignored ❌ +systemctl stop # SIGTERM — currently ignored ❌ +# Kubernetes pod delete # SIGTERM — currently ignored ❌ + +# This works but is non-standard: +kill -INT # SIGINT — works ✅ + +# This should be the last resort, never needed in normal operation: +kill -9 # SIGKILL — force kill ❌ +``` + +Adding a `SIGTERM` handler in `main.rs` is the single most impactful change in +this EPIC — it fixes all four broken cases above with a few lines of code. + +## Background + +This EPIC was originally created after closing issue #1477 ("Fix shutdown message +and improve it"), which introduced the `JobManager` type and centralized job +management. The current EPIC builds on that foundation to complete the +centralization and address remaining gaps. + +Issue #1588 ("Review shutdown process for all tasks/jobs") is the first sub-issue +and identified the remaining jobs that still handle `ctrl_c` directly. + +## Scope + +### In Scope + +- Centralize signal handling in `main.rs` (both `SIGINT` and `SIGTERM`). +- Consistent shutdown mechanism for all jobs (prefer `CancellationToken`). +- Migrate torrent cleanup and activity metrics updater to use `CancellationToken`. +- Configurable grace periods (add `[shutdown]` configuration section). +- Observable shutdown progress (which jobs are still running). +- Grace period alignment between `JobManager` and server-level shutdown. +- Review and align the Axum `graceful_shutdown` timeout with the `JobManager` timeout. +- UDP server shutdown improvements (drain or at least log in-flight work). + +### Out of Scope + +- Hot-reload / restart without process exit. +- Dynamic job lifecycle (start/stop jobs at runtime via admin API). +- Windows-specific signal handling beyond what Tokio provides. +- The **profiling binary** (`src/console/profiling.rs`) — it is a developer-only + tool for profiling (valgrind/callgrind), not a user-facing entry point. It can + be updated independently as needed. + +## Sub-issues + +Items marked **Blocked** depend on an open question in +[open-questions.md](../../features/shutdown-process/open-questions.md). +Spec files for draft items live under `docs/issues/drafts/`. + +| # | Title | Spec | Status | Notes | +| ----- | ------------------------------------------------------------ | --------------------------------------------------------------------------------- | ------ | ---------------------------------------------- | +| #1588 | Review shutdown process for all tasks/jobs | [1588/ISSUE.md](../1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md) | Open | Pre-existing; inventory and gap analysis | +| SI-1 | Add `SIGTERM` handler to `main.rs` | [SI-1/ISSUE.md](../../drafts/1488-si-1-add-sigterm-to-main/ISSUE.md) | Draft | Highest priority; fixes the Unix contract | +| SI-2 | Remove `global_shutdown_signal()` from per-server shutdown | [SI-2/ISSUE.md](../../drafts/1488-si-2-remove-global-shutdown-signal/ISSUE.md) | Draft | Blocked: Q5 only; touches `torrust-server-lib` | +| SI-3 | Fix `Environment::stop()` in standalone library examples | [SI-3/ISSUE.md](../../drafts/1488-si-3-fix-environment-stop/ISSUE.md) | Draft | ✅ Unblocked (Q1 resolved) | +| SI-4 | Migrate torrent cleanup to `CancellationToken` | [SI-4/ISSUE.md](../../drafts/1488-si-4-migrate-torrent-cleanup/ISSUE.md) | Draft | Removes direct `ctrl_c` listener | +| SI-5 | Migrate activity metrics updater to `CancellationToken` | [SI-5/ISSUE.md](../../drafts/1488-si-5-migrate-activity-metrics-updater/ISSUE.md) | Draft | Removes direct `ctrl_c` listener | +| SI-6 | Align `JobManager` grace period with Axum server timeout | [SI-6/ISSUE.md](../../drafts/1488-si-6-align-grace-periods/ISSUE.md) | Draft | Blocked: Q4; fixes the 10s vs 90s mismatch | +| SI-7 | Implement observable shutdown progress in `JobManager` | [SI-7/ISSUE.md](../../drafts/1488-si-7-observable-shutdown-progress/ISSUE.md) | Draft | | +| SI-8 | Add configurable grace periods (`[shutdown]` config section) | [SI-8/ISSUE.md](../../drafts/1488-si-8-configurable-grace-periods/ISSUE.md) | Draft | Blocked: Q3, Q4 | +| SI-9 | Improve UDP server shutdown | [SI-9/ISSUE.md](../../drafts/1488-si-9-improve-udp-shutdown/ISSUE.md) | Draft | Low priority; UDP is stateless by nature | + +## Dependencies + +- **#1405** (Overhaul stats: graceful shutdown for broadcast channels) — ✅ Closed. + Implemented with `CancellationToken`, which is the foundation for this EPIC. +- **#1477** (Fix shutdown message and improve it) — ✅ Closed. + Introduced the `JobManager` type. + +## Related Documents + +- [Analysis: Shutdown Process](../../analysis/20260716-shutdown-process/README.md) — detailed code-level analysis +- [Feature: Shutdown Process](../../features/shutdown-process/README.md) — product-oriented feature description +- [Open Questions](../../features/shutdown-process/open-questions.md) — gaps and risks to resolve before implementation diff --git a/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md b/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md new file mode 100644 index 000000000..2bad9c499 --- /dev/null +++ b/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md @@ -0,0 +1,147 @@ +--- +doc-type: issue +issue-type: task +status: open +priority: p2 +github-issue: 1588 +spec-path: docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/ISSUE.md +branch: "1588-review-shutdown-process" +related-pr: null +last-updated-utc: 2026-07-16 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/analysis/20260716-shutdown-process/README.md + - docs/features/shutdown-process/README.md + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - src/bootstrap/jobs/manager.rs + - src/bootstrap/jobs/torrent_cleanup.rs + - src/bootstrap/jobs/activity_metrics_updater.rs + - packages/axum-server/src/signals.rs + - packages/udp-server/src/server/launcher.rs + - packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs +--- + + + +# Issue #1588 - Review shutdown process for all tasks/jobs + +> **EPIC position**: Subissue #1 of #1488. First step — inventory and analysis before any implementation. + +## Goal + +Review all jobs and tasks in the Torrust Tracker application to identify which ones still handle the Ctrl+C signal directly instead of using the centralized `CancellationToken` from the `JobManager`. Document the current state and produce a list of remaining jobs that need migration. + +## Background + +The `JobManager` type was introduced in PR #1587 to centralize job management. It provides a shared `CancellationToken` that can be used to signal all jobs to stop. However, some jobs were not updated to use it: + +- **HTTP servers (Axum)**: Use `torrust_server_lib::signals::global_shutdown_signal` inside the `shutdown_signal()` function, which listens for `ctrl_c` and `SIGTERM` directly. +- **Activity metrics updater**: Uses `tokio::signal::ctrl_c()` directly in its loop. +- **Torrent cleanup job**: Uses `tokio::signal::ctrl_c()` directly in its loop. + +Additionally, the `main.rs` entry point only handles `SIGINT` (Ctrl+C), not `SIGTERM`. + +## Tasks + +### Task 1: Inventory all jobs and their shutdown mechanism + +Create a complete inventory of all jobs spawned by the application, including: + +- Event listeners (swarm, core, http-core, udp-core, udp-server stats, udp-server banning) +- UDP tracker instances +- HTTP tracker instances +- REST API server +- Health Check API server +- Torrent cleanup +- Activity metrics updater + +For each job, document: + +- What shutdown mechanism it uses (`CancellationToken`, direct `ctrl_c`, halt channel) +- Whether it responds to `jobs.cancel()` +- Whether it would stop on `SIGTERM` + +### Task 2: Identify gaps + +From the inventory, produce a list of jobs that: + +- Do not respond to the `CancellationToken` +- Do not respond to `SIGTERM` +- Have inconsistent shutdown behavior + +### Task 3: Propose migration plan + +For each gap, propose a concrete migration: + +- Which jobs can be migrated to `CancellationToken` directly +- Which jobs need a two-phase shutdown (e.g., Axum servers) +- Whether the `global_shutdown_signal` in servers should be removed or kept + +## Acceptance Criteria + +- [ ] Complete inventory documented in a table +- [ ] Gaps identified and categorized +- [ ] Migration plan reviewed and approved +- [ ] New sub-issues created for each migration task + +## References + +- [PR #1587](https://github.com/torrust/torrust-tracker/pull/1587) — introduced centralized shutdown for event listeners +- [Shutdown Analysis](../../analysis/20260716-shutdown-process/README.md) — detailed code-level analysis +- [Feature: Shutdown Process](../../features/shutdown-process/README.md) — product-oriented feature description + +## Manual Verification + +Evidence of these steps must be recorded in `verification.md` in this folder +before the issue can be closed. + +### Test 1: Complete inventory table exists + +After completing Task 1, confirm the inventory table in this issue (or in a +linked document) covers all of the following jobs: + +- [ ] swarm coordination registry event listener +- [ ] tracker core event listener +- [ ] HTTP core event listener +- [ ] UDP core event listener +- [ ] UDP server stats event listener +- [ ] UDP server banning event listener +- [ ] UDP tracker instances (one per configured port) +- [ ] HTTP tracker instances (one per configured port) +- [ ] REST API server +- [ ] Health Check API server +- [ ] Torrent cleanup +- [ ] Activity metrics updater (peers inactivity update) + +For each job, the inventory must document all three columns: + +- Shutdown mechanism (`CancellationToken` / direct `ctrl_c` / halt channel) +- Whether it responds to `jobs.cancel()` +- Whether it would stop on `SIGTERM` + +**Record in `verification.md`**: a copy of or link to the completed inventory table. + +### Test 2: Gaps identified match the analysis + +Confirm the gaps identified in Task 2 are consistent with the findings in the +[shutdown analysis §7](../../analysis/20260716-shutdown-process/README.md). + +Specifically, at minimum these gaps must be identified: + +- [ ] Torrent cleanup uses direct `ctrl_c` — does not respond to `jobs.cancel()` +- [ ] Activity metrics updater uses direct `ctrl_c` — does not respond to `jobs.cancel()` +- [ ] HTTP/REST API/Health Check servers use `global_shutdown_signal()` independently +- [ ] `main.rs` does not handle `SIGTERM` + +**Record in `verification.md`**: the gap list, confirming it matches or extends +the analysis findings. + +### Test 3: Migration plan covers all gaps + +Confirm Task 3 produces a migration entry for every gap found in Test 2, and +that each entry maps to an existing draft sub-issue (SI-1 through SI-9) or +proposes a new one. + +**Record in `verification.md`**: the migration mapping table (gap → sub-issue). diff --git a/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/verification.md b/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/verification.md new file mode 100644 index 000000000..3b2c2c0fa --- /dev/null +++ b/docs/issues/open/1588-review-shutdown-process-for-all-tasks-jobs/verification.md @@ -0,0 +1,14 @@ +# Verification Evidence + +> **Status**: Not started — to be filled in when implementing the issue. + +## Environment + +- Date: +- OS: +- Rust version (`rustc --version`): +- Tracker commit/branch: + +## Test Results + + diff --git a/docs/research/20260716-console-shutdown-patterns/README.md b/docs/research/20260716-console-shutdown-patterns/README.md new file mode 100644 index 000000000..0784d36f9 --- /dev/null +++ b/docs/research/20260716-console-shutdown-patterns/README.md @@ -0,0 +1,482 @@ +--- +doc-type: research +status: draft +last-updated-utc: 2026-07-16 +semantic-links: + related-artifacts: + - docs/analysis/20260716-shutdown-process/README.md + - docs/features/shutdown-process/README.md + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md +--- + +# Console Shutdown Patterns: SIGINT vs SIGTERM + +## Status + +Draft — research conducted on 2026-07-16. + +## Summary + +This document investigates how console applications, particularly long-running +network services written in Rust, handle OS signals for graceful shutdown. It +focuses on the differences between `SIGINT` (Ctrl+C) and `SIGTERM` (default +`kill` signal), and how real-world projects like Vector (Datadog) implement +their shutdown logic. + +## 1. OS Signals Overview + +### 1.1 SIGINT (Signal Interrupt) + +| Property | Value | +| ------------------------- | ------------------------------------------------------ | +| **Signal number** | 2 | +| **Default action** | Terminate process | +| **Can be caught/ignored** | Yes | +| **How sent** | Ctrl+C in terminal, `kill -2 `, `kill -INT ` | +| **Typical meaning** | "User requested interrupt" | + +**Characteristics:** + +- Typically sent by the user from a terminal (Ctrl+C). +- The process is expected to stop **promptly** but can clean up. +- Some programs treat it as a "soft" shutdown (print status and continue). +- When caught by a Tokio runtime, it can be received by **multiple** tasks if + they all call `tokio::signal::ctrl_c()`. However, only **one** task will + actually receive it — the signal is consumed by the first listener. + +### 1.2 SIGTERM (Signal Terminate) + +| Property | Value | +| ------------------------- | ------------------------------------------------------------------------------------------------------------------ | +| **Signal number** | 15 | +| **Default action** | Terminate process | +| **Can be caught/ignored** | Yes | +| **How sent** | `kill `, `kill -15 `, `kill -TERM `, Docker/Podman `stop`, Kubernetes pre-stop hook, systemd `stop` | +| **Typical meaning** | "Please terminate gracefully" | + +**Characteristics:** + +- This is the **default** signal sent by `kill`, Docker/Podman `stop`, + Kubernetes, systemd, and most process managers. +- The process is expected to perform a **graceful shutdown** (drain connections, + flush data, close files) and then exit. +- If the process does not exit within a grace period, a `SIGKILL` (signal 9) is + sent, which cannot be caught and force-terminates the process. +- The `tokio::signal::ctrl_c()` function does **not** handle SIGTERM. You must + use `tokio::signal::unix::signal(SignalKind::terminate())` on Unix. + +### 1.3 SIGQUIT (Signal Quit) + +| Property | Value | +| ------------------------- | ------------------------------------------------------- | +| **Signal number** | 3 | +| **Default action** | Terminate with core dump | +| **Can be caught/ignored** | Yes | +| **How sent** | Ctrl+\ in terminal, `kill -3 `, `kill -QUIT ` | +| **Typical meaning** | "Quit and dump core" | + +Used by Vector to trigger a **quick/forced quit** (no graceful shutdown). + +### 1.4 SIGHUP (Signal Hangup) + +| Property | Value | +| ------------------------- | ---------------------------------------------------- | +| **Signal number** | 1 | +| **Default action** | Terminate | +| **Can be caught/ignored** | Yes | +| **How sent** | Closing terminal, `kill -1 `, `kill -HUP ` | +| **Typical meaning** | Traditionally "hang up" — reload configuration | + +Used by Vector (and many daemons) to trigger a **configuration reload**. + +## 2. Signal Handling in Tokio + +### 2.1 `tokio::signal::ctrl_c()` + +```rust +pub async fn ctrl_c() -> Result<()> +``` + +- Only handles `SIGINT` (signal 2). +- Available on all platforms (Unix + Windows). +- On Windows, it uses `SetConsoleCtrlHandler` to catch Ctrl+C, Ctrl+Break, + and console close events. +- **Important**: Only one task can successfully wait for `ctrl_c()`. If multiple + tasks call `ctrl_c()`, only one receives the signal. The others will hang + indefinitely. + +### 2.2 `tokio::signal::unix::signal()` + +```rust +pub fn signal(kind: SignalKind) -> Result +``` + +- Unix-only. +- Can handle any signal: `SIGTERM`, `SIGINT`, `SIGHUP`, `SIGQUIT`, and user-defined signals, etc. +- Returns a `Signal` stream that yields `()` each time the signal is received. +- **Important**: Like `ctrl_c()`, only one instance of a particular signal + handler can be created. Creating a second handler for the same signal will + overwrite the first. + +### 2.3 `tokio_util::sync::CancellationToken` + +```rust +pub struct CancellationToken { ... } +impl CancellationToken { + pub fn new() -> Self; + pub fn cancel(&self); + pub fn cancelled(&self) -> WaitForCancellationFuture; + pub fn child_token(&self) -> Self; + pub fn drop_guard(&self) -> DropGuard; +} +``` + +- Not a signal mechanism, but a **coordination** mechanism. +- Used to propagate shutdown signals from a central coordinator to many tasks. +- Tasks check `token.cancelled()` or await `token.cancelled()` in their loops. +- Child tokens inherit parent cancellation. +- `DropGuard` auto-cancels on drop (useful for RAII-style shutdown). + +## 3. How Real-World Projects Handle Shutdown + +### 3.1 Vector (Datadog) — `src/signal.rs` + +Vector is a high-performance observability data pipeline written in Rust. It has +a sophisticated signal handling system: + +**Unix signal handling** (`src/signal.rs`): + +```rust +#[cfg(unix)] +fn os_signals(runtime: &Runtime) -> impl Stream + use<> { + runtime.block_on(async { + let mut sigint = signal(SignalKind::interrupt()).expect("..."); + let mut sigterm = signal(SignalKind::terminate()).expect("..."); + let mut sigquit = signal(SignalKind::quit()).expect("..."); + let mut sighup = signal(SignalKind::hangup()).expect("..."); + + async_stream::stream! { + loop { + let signal = tokio::select! { + _ = sigint.recv() => { + info!(message = "Signal received.", signal = "SIGINT"); + SignalTo::Shutdown(None) + }, + _ = sigterm.recv() => { + info!(message = "Signal received.", signal = "SIGTERM"); + SignalTo::Shutdown(None) + }, + _ = sigquit.recv() => { + info!(message = "Signal received.", signal = "SIGQUIT"); + SignalTo::Quit + }, + _ = sighup.recv() => { + info!(message = "Signal received.", signal = "SIGHUP"); + SignalTo::ReloadFromDisk + }, + }; + yield signal; + } + } + }) +} +``` + +**Windows signal handling**: + +```rust +#[cfg(windows)] +fn os_signals() -> impl Stream { + async_stream::stream! { + loop { + let signal = tokio::signal::ctrl_c().map(|_| SignalTo::Shutdown(None)).await; + yield signal; + } + } +} +``` + +**Key observations from Vector:** + +- Both `SIGINT` and `SIGTERM` produce the **same** `SignalTo::Shutdown` action. +- `SIGQUIT` produces a **different** action (`SignalTo::Quit`) — immediate exit + without graceful shutdown. +- `SIGHUP` triggers a **configuration reload** (`SignalTo::ReloadFromDisk`). +- Vector uses a **broadcast channel** (`SignalTx`/`SignalRx`) to propagate + signals from the handler to all interested components. +- The shutdown flow is: `Application::start()` → `StartedApplication::main()` + (event loop) → `FinishedApplication::shutdown()`. +- Graceful shutdown has a configurable timeout (`--graceful-shutdown-limit-secs`, + default 60s). After the timeout, force shutdown occurs. +- The `stop()` method has a **two-phase shutdown**: first mark the API as + unavailable (for Kubernetes readiness probes), then drain the topology. + +### 3.2 Axum (Tokio) — `Handle::graceful_shutdown()` + +Axum provides a built-in mechanism for graceful HTTP shutdown: + +```rust +use axum_server::Handle; + +let handle = Handle::new(); + +// Spawn the graceful shutdown watcher +tokio::spawn(async move { + // Wait for signal + signal::ctrl_c().await.unwrap(); + // Start graceful shutdown + handle.graceful_shutdown(Some(Duration::from_secs(30))); +}); + +// Pass the handle to the server +axum_server::from_tcp(listener) + .handle(handle) + .serve(app.into_make_service()) + .await + .unwrap(); +``` + +**Key observations:** + +- `graceful_shutdown(Some(duration))` stops accepting new connections and waits + for existing connections to finish, up to the given duration. +- After the grace period, remaining connections are forcibly closed. +- The `Handle` also provides `connection_count()` to monitor active connections. + +### 3.3 Torrust Tracker (Current Implementation) + +The current implementation is covered in detail in the +[shutdown analysis](../../analysis/20260716-shutdown-process/README.md). Key points: + +- `main.rs` only handles `SIGINT` via `tokio::signal::ctrl_c()`. +- `SIGTERM` is not handled at the top level — only inside each server via + `torrust_server_lib::signals::global_shutdown_signal()`. +- The `global_shutdown_signal()` handles both `SIGINT` and `SIGTERM` via + `tokio::signal::unix::signal(SignalKind::terminate())`. +- This creates a **double-signal** problem on Ctrl+C: both `main.rs` and each + server's `global_shutdown_signal()` catch the same signal independently. + +## 4. Common Patterns and Best Practices + +### 4.1 Centralized Signal Handling (Recommended) + +```text +┌─────────────────────────────────────────────────┐ +│ main.rs │ +│ │ +│ tokio::select! { │ +│ _ = sigint() => shutdown().await, │ +│ _ = sigterm() => shutdown().await, │ +│ _ = sigquit() => quit().await, │ +│ _ = sighup() => reload().await, │ +│ } │ +│ │ +│ async fn shutdown() { │ +│ token.cancel(); │ +│ send_halt_to_all_servers().await; │ +│ wait_for_all_jobs(timeout).await; │ +│ } │ +└─────────────────────────────────────────────────┘ + │ │ │ + ▼ ▼ ▼ + ┌─────────┐ ┌─────────┐ ┌─────────┐ + │ Job 1 │ │ Job 2 │ │ Job 3 │ + │(token) │ │(token) │ │(channel)│ + └─────────┘ └─────────┘ └─────────┘ +``` + +**Benefits:** + +- Single source of truth for shutdown decisions. +- Predictable shutdown order. +- Can differentiate between signals (e.g., SIGQUIT → immediate exit). +- No double-signal problem. + +### 4.2 Signal Differentiation + +Most projects treat `SIGINT` and `SIGTERM` the same way: graceful shutdown. +However, some projects differentiate: + +| Signal | Typical Action | Notes | +| --------------------- | ----------------------------------- | ------------------------ | +| `SIGINT` | Graceful shutdown | User pressed Ctrl+C | +| `SIGTERM` | Graceful shutdown (possibly faster) | Container orchestrator | +| `SIGQUIT` | Immediate/dirty shutdown | User wants to force quit | +| `SIGHUP` | Reload configuration | Reload without restart | +| `SIGUSR1` / `SIGUSR2` | Toggle debug/log level | Custom behavior | + +For the Torrust Tracker, there is likely **no need to differentiate** between +`SIGINT` and `SIGTERM` — both should trigger the same graceful shutdown +sequence. The key missing piece is simply that `SIGTERM` is not handled at the +top level. + +### 4.3 Grace Period Configuration + +Production services should make the shutdown grace period configurable: + +```toml +[shutdown] +# Maximum time to wait for jobs to finish before force-exiting. +# Kubernetes terminationGracePeriodSeconds should be set higher than this. +grace_period_secs = 30 + +# How long each Axum server waits for connections to drain. +# This must be <= grace_period_secs. +connection_drain_secs = 25 +``` + +**Reference values from real projects:** + +| Project | Grace Period | Configurable? | +| ------------------------- | ------------- | -------------------------------------- | +| Vector | 60s | Yes (`--graceful-shutdown-limit-secs`) | +| Kubernetes pod | 30s (default) | Yes (`terminationGracePeriodSeconds`) | +| Docker/Podman stop | 10s (default) | Yes (`--time`) | +| systemd | 90s (default) | Yes (`TimeoutStopSec`) | +| Torrust Tracker (current) | 10s per job | No (hardcoded) | + +### 4.4 Observable Shutdown + +During shutdown, the application should log which jobs are still running: + +```text +2026-07-16T12:00:00Z INFO Shutting down ... +2026-07-16T12:00:00Z INFO Waiting for jobs to finish (timeout: 30s)... +2026-07-16T12:00:05Z INFO Still waiting for: HTTP tracker (0.0.0.0:7070), Torrent cleanup +2026-07-16T12:00:10Z INFO Still waiting for: HTTP tracker (0.0.0.0:7070) — 3 active connections +2026-07-16T12:00:12Z INFO HTTP tracker (0.0.0.0:7070) — done +2026-07-16T12:00:12Z INFO All jobs finished. Shutdown complete. +``` + +Vector does this by printing which components won't shutdown gracefully when +the deadline is reached: + +```rust +if let Some(deadline) = deadline { + let mut check_handles2 = check_handles.clone(); + Box::pin(async move { + sleep_until(deadline).await; + check_handles2.retain(|_key, handles| { + retain(handles, |handle| handle.peek().is_none()); + !handles.is_empty() + }); + // Log remaining handles that haven't finished + if !check_handles2.is_empty() { + warn!(...); + } + }) +} +``` + +### 4.5 Two-Phase Shutdown for Network Services + +Vector implements a two-phase shutdown pattern that is useful for services +behind load balancers: + +1. **Phase 1**: Mark the service as unhealthy (Kubernetes readiness probe fails). + This stops new traffic from being routed to this instance. +2. **Phase 2**: Drain existing connections gracefully within the timeout. + +```rust +impl TopologyController { + pub async fn stop(mut self) { + // Phase 1: Mark the API as unavailable + #[cfg(feature = "api")] + if let Some(server) = self.api_server.as_mut() { + server.set_not_serving().await; + } + + // Phase 2: Drain the topology + self.topology.stop().await; + } +} +``` + +### 4.6 Windows Considerations + +On Windows, `tokio::signal::ctrl_c()` handles Ctrl+C, Ctrl+Break, and console +close events. There is no equivalent of `SIGTERM` on Windows. The standard +approach is: + +```rust +#[cfg(windows)] +let terminate = std::future::pending::<()>(); + +#[cfg(unix)] +let terminate = async { + tokio::signal::unix::signal(SignalKind::terminate()) + .expect("...") + .recv() + .await; +}; +``` + +This is exactly what the current `torrust_server_lib::signals::global_shutdown_signal()` +does. + +## 5. Recommendations for the Torrust Tracker + +### 5.1 Handle SIGTERM in `main.rs` + +Add `SIGTERM` handling alongside the existing `SIGINT` handler: + +```rust +#[cfg(unix)] +use tokio::signal::unix::{SignalKind, signal}; + +#[tokio::main] +async fn main() { + let (_app_container, jobs) = app::run().await; + + let ctrl_c = tokio::signal::ctrl_c(); + + #[cfg(unix)] + let mut sigterm = signal(SignalKind::terminate()).expect("failed to install SIGTERM handler"); + + tokio::select! { + _ = ctrl_c => { + tracing::info!("Torrust tracker shutting down (SIGINT) ..."); + } + #[cfg(unix)] + _ = sigterm.recv() => { + tracing::info!("Torrust tracker shutting down (SIGTERM) ..."); + } + } + + jobs.cancel(); + jobs.wait_for_all(Duration::from_secs(30)).await; + tracing::info!("Torrust tracker successfully shutdown."); +} +``` + +### 5.2 Remove `global_shutdown_signal()` from Servers + +Once `main.rs` handles both signals, the duplicate `global_shutdown_signal()` +inside each server's `shutdown_signal()` should be removed. The halt channel +alone is sufficient — `main.rs` sends the halt signal to all servers during +shutdown. + +### 5.3 Make Grace Periods Configurable + +Add a `[shutdown]` configuration section (tracked in the EPIC as a draft +sub-issue). + +### 5.4 Consider Concurrent Job Waiting + +Change `JobManager::wait_for_all()` to wait for all jobs **concurrently** +with a shared timeout, rather than sequentially. + +### 5.5 Consider SIGQUIT for Immediate Exit + +Optionally, add `SIGQUIT` handling for an immediate, non-graceful exit (useful +for developers who want to force-stop the tracker without waiting). + +## 6. References + +- [Tokio Signal Documentation](https://docs.rs/tokio/latest/tokio/signal/index.html) +- [Tokio Signal Unix](https://docs.rs/tokio/latest/tokio/signal/unix/index.html) +- [Tokio Util CancellationToken](https://docs.rs/tokio-util/latest/tokio_util/sync/struct.CancellationToken.html) +- [Vector Signal Handling](https://github.com/vectordotdev/vector/blob/master/src/signal.rs) +- [Vector Graceful Shutdown CLI Options](https://github.com/vectordotdev/vector/blob/master/src/cli.rs) +- [Axum Server Graceful Shutdown](https://docs.rs/axum-server/latest/axum_server/struct.Handle.html#method.graceful_shutdown) +- [Torrust Tracker Shutdown Analysis](../../analysis/20260716-shutdown-process/README.md) diff --git a/docs/research/AGENTS.md b/docs/research/AGENTS.md new file mode 100644 index 000000000..d8bc9b63b --- /dev/null +++ b/docs/research/AGENTS.md @@ -0,0 +1,45 @@ +# `docs/research/` — Research Documents + +This directory contains research documents that investigate external topics, technologies, +or patterns relevant to the project. Unlike **analysis** documents (which study the project's +own code), research documents look outward — at how other projects solve similar problems, +what the ecosystem offers, or what best practices exist. + +## Purpose + +A research document answers questions like: + +- How do other projects (Rust or otherwise) handle this problem? +- What are the standard patterns, libraries, or approaches? +- What are the trade-offs between different options? +- What does the ecosystem recommend? + +Research is the **input** to design decisions: it feeds into feature definitions, ADRs, +and implementation plans. + +## Timestamp Prefix Convention + +Like analysis folders, research folders use a **timestamp prefix** to make it clear when +the research was conducted: + +```text +docs/research/ +├── AGENTS.md +├── 20260716-console-shutdown-patterns/ +│ └── README.md +└── ... +``` + +## Lifecycle + +- **Research may become outdated** as the ecosystem evolves. Always check the timestamp + before relying on old research. +- **Research may stay relevant** if the technologies and patterns it covers have not + changed significantly. +- **Old research can be cleaned up** when no longer relevant. + +## Related + +- [Analysis documents](../analysis/) — studies of the project's own code +- [Feature definitions](../features/) — product-oriented descriptions of desired features +- [ADRs](../adrs/) — architectural decision records diff --git a/project-words.txt b/project-words.txt index 9966f4645..c983ae53b 100644 --- a/project-words.txt +++ b/project-words.txt @@ -361,7 +361,11 @@ sharktorrent shellcheck SHLVL Signedness +pgrep +rwxrwxr +SIGUSR skiplist +taskkill slowloris socat socketaddr