From d04f331002a173dffbbb1d428aa7140d9b4c130c Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 18:20:15 +0100 Subject: [PATCH 01/18] docs(issue-1450): add issue spec with manual verification evidence Adds the issue spec and pre-fix manual verification evidence for issue #1450: discard UDP requests from clients with source port 0. The evidence captures the WARN log produced by the original tracker when a crafted UDP datagram with source port 0 is received: WARN process_request:send_response{...}: failed to send bytes_count=16 error=Invalid argument (os error 22) The spec explains why port 0 is possible in UDP (RFC 768, connectionless protocol), the design decision (early discard + stats counter, no per- request log), and how to reproduce the bug manually using a raw Python socket script. --- .../ISSUE.md | 203 ++++++++++++++++++ .../before-fix-manual-verification.md | 95 ++++++++ project-words.txt | 8 + 3 files changed, 306 insertions(+) create mode 100644 docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md create mode 100644 docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/before-fix-manual-verification.md diff --git a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md new file mode 100644 index 000000000..5c6e19f31 --- /dev/null +++ b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md @@ -0,0 +1,203 @@ +--- +doc-type: issue +issue-type: bug +status: open +priority: p2 +github-issue: 1450 +spec-path: docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md +branch: "1450-discard-udp-requests-from-clients-with-port-0" +related-pr: null +last-updated-utc: 2026-07-21 00:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/udp-server/src/server/processor.rs + - packages/udp-server/src/event.rs + - packages/udp-server/src/statistics/mod.rs + - packages/udp-server/src/statistics/event/handler/ +--- + +# Issue #1450 - Discard UDP requests from clients with port 0 + +## Goal + +Prevent the UDP tracker from processing requests that arrive from a client address +where the source port is 0. Such requests produce an OS-level error when the tracker +tries to send the response, and the error is currently surfaced as a noisy `WARN` log. + +## Background + +### Why can a UDP client have port 0? + +Unlike TCP, UDP is a **connectionless protocol**. The tracker never establishes a +handshake — it simply calls `recvfrom()` and reads whatever datagram arrives. The +"client port" is whatever value happens to be in the **source port field of the +incoming UDP header**, a 16-bit number entirely under the sender's control. + +RFC 768 (the UDP specification) explicitly permits port 0: + +> _"Source Port is an optional field, when meaningful, it indicates the port of the +> sending process... If not used, a value of zero is inserted."_ + +In practice, port 0 in a UDP source can originate from: + +- A **buggy BitTorrent client** that fails to bind before sending. +- A **raw-socket tool or scanner** that crafts datagrams with an intentionally zeroed + source port (e.g., to probe the tracker without revealing a real port). +- A **broken middlebox** (NAT/firewall) that strips or zeroes the source port. + +The tracker has no way to prevent these datagrams from arriving — the OS delivers +them just like any other UDP packet. + +### Current behaviour + +The tracker received UDP packets from clients whose source port is `0` in the UDP +header. This is an invalid socket address — no response can ever be delivered to +`:0`. The current code processes the request fully (parses it, executes the +handler, serializes the response) and only discovers the problem when it calls +`send_to`, which returns `EINVAL` (OS error 22). The failure is then logged as a +`WARN`, polluting production logs. + +Example from the demo tracker logs: + +```text +tracker | 2025-04-14T08:52:42.491940Z WARN process_request:send_response{client_socket_addr=*.*.*.*:0 response=Connect(...) ...}: torrust_udp_tracker_server::server::processor: failed to send bytes_count=16 error=Invalid argument (os error 22) payload=[...] +``` + +This happens at least for `Connect` requests and could in theory happen for any +request type. It has been observed multiple times in the demo tracker logs: + +- 2025-04-14 (first observation) +- 2025-06-18 (two additional occurrences) + +Whether these are malformed clients, scanner tools, or deliberate abuse (port-0 +spam) is unknown. Regardless, the tracker should not waste resources processing +them and should not fill logs with OS-level errors caused by user-space input. + +## Design + +### Detection point + +The check happens at the very start of `Processor::process_request`, before any +packet parsing or handler invocation. If `client_socket_addr.port() == 0`, the +request is **discarded immediately**: + +```rust +pub async fn process_request(self, request: RawRequest) { + let client_socket_addr = request.from; + + if client_socket_addr.port() == 0 { + // Discard: cannot send a response to port 0. + // Emit a stats event so operators can detect abuse / misconfigured clients. + ... + return; + } + ... +} +``` + +### Logging + +**No per-request `WARN` log.** The existing `WARN` log is removed (it came from the +send failure, which no longer occurs). A per-request log for bad-user traffic would +add uncontrollable noise to production logs. Operators who want visibility should +use the metrics/stats endpoint. + +A single `tracing::trace!` line may be emitted for debugging purposes (enabled only +at trace level, never in default production configurations). + +### Statistics + +A new stats event `UdpRequestDiscarded` is introduced (not reusing +`UdpRequestAborted`, which represents a different lifecycle stage). A matching +metric counter is added: + +```text +udp_tracker_server_requests_discarded_total +``` + +This counter increments for every discarded request, providing operators with +a signal to detect scanner activity or abuse without exposing it in logs. + +## Acceptance Criteria + +- [ ] Requests with client port 0 are discarded before any handler is invoked. +- [ ] The existing `WARN` log ("failed to send ... error=Invalid argument (os error 22)") + no longer appears for this case. +- [ ] A new `UdpRequestDiscarded` event is defined in `event.rs`. +- [ ] The event is emitted from `process_request` when the client port is 0. +- [ ] A new metric counter `udp_tracker_server_requests_discarded_total` is described + and handled by the statistics event handler. +- [ ] Unit tests cover: + - The handler for `UdpRequestDiscarded` increments the counter. + - The processor discards the request (no response sent, counter incremented). + +## Verification + +### Automated tests + +The unit tests in `packages/udp-server/src/server/processor.rs` are the deepest +automated coverage possible for this scenario. They work by injecting a `RawRequest` +with `from = :0` directly into `Processor::process_request`, bypassing the +network layer entirely. + +**Why a network-level integration test is not feasible:** + +When a process opens a normal UDP socket and binds to port 0, the OS always assigns +a real ephemeral source port (e.g., `54321`). It is impossible to make the kernel +send a datagram with source port 0 through a normal socket API. The only way to +produce such a datagram on the wire is to use a **raw socket**, which requires +`CAP_NET_RAW` / `root` privileges — not acceptable in standard CI environments. + +Therefore: + +- Unit tests cover the discard logic and the stats counter increment. +- No separate integration or E2E test is added for this path. + +### Manual verification + +To verify the fix end-to-end on a running tracker, you need a tool that can craft +raw UDP packets with an explicit source port. Two options: + +**Option A — `nping` (from the nmap suite)** + +```sh +sudo nping --udp --dest-port 6969 --source-port 0 +``` + +**Option B — `scapy` (Python)** + +```sh +sudo python3 - <<'EOF' +from scapy.all import * +# BEP 15 connect request (magic + action=0 + transaction_id) +payload = b'\x00\x00\x04\x17\x27\x10\x19\x80\x00\x00\x00\x00\xde\xad\xbe\xef' +send(IP(dst="") / UDP(sport=0, dport=6969) / Raw(load=payload)) +EOF +``` + +Both require root / `sudo` because they use raw sockets. + +**What to check after sending the packet:** + +1. **No `WARN` log** — the line + `"failed to send ... error=Invalid argument (os error 22)"` must not appear. +2. **Counter increment** — query the REST API stats endpoint and confirm + `udp_requests_discarded` has increased by 1: + + ```sh + curl -s http://localhost:1212/api/v1/stats | jq .udp_requests_discarded + ``` + +3. **No response sent to the client** — `nping` or `scapy` should report no reply. + +## Implementation Notes + +- `ConnectionContext::new(client_addr_with_port_0, server_service_binding)` is valid; + only the server binding is required to have a non-zero port. +- The `process_request` function already has `self.server_service_binding` available + to construct the `ConnectionContext` for the stats event. +- Follow the existing pattern in + `packages/udp-server/src/statistics/event/handler/request_aborted.rs` for the new + handler file. diff --git a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/before-fix-manual-verification.md b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/before-fix-manual-verification.md new file mode 100644 index 000000000..387593bea --- /dev/null +++ b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/before-fix-manual-verification.md @@ -0,0 +1,95 @@ +# Evidence: Original (Pre-Fix) Behaviour — Manual Verification + +**Date**: 2026-07-21 +**Tracker version**: 3.0.0-develop, commit `c0fb3895` +**Branch**: `develop` (before the fix branch was applied) +**Environment**: Local development machine, Linux + +## Purpose + +This document captures the evidence that confirms the buggy behaviour described in +issue #1450: the tracker processes a UDP connect request from a client with source +port 0 and then emits a `WARN` log when it fails to send the response back. + +## Steps to Reproduce + +### 1. Build the tracker from the pre-fix commit + +```sh +git checkout c0fb3895 +cargo build --bin torrust-tracker +``` + +### 2. Start the tracker with the development config + +```sh +TORRUST_TRACKER_CONFIG_TOML_FILE=share/default/config/tracker.development.sqlite3.toml \ + ./target/debug/torrust-tracker > /tmp/tracker.log 2>&1 & +``` + +### 3. Send a UDP datagram with source port 0 using a raw socket + +The script below constructs a BEP 15 connect request with source port 0 +and sends it via a raw IP socket (requires root): + +```sh +sudo python3 .tmp/send_port0_udp.py +``` + +The script content (`send_port0_udp.py`): + +```python +import socket, struct + +DST_IP, DST_PORT, SRC_PORT = "127.0.0.1", 6969, 0 +PAYLOAD = struct.pack("!qII", 0x0000041727101980, 0, 0xDEADBEEF) + +def checksum(data): + if len(data) % 2: data += b"\x00" + s = sum((data[i] << 8) + data[i+1] for i in range(0, len(data), 2)) + s = (s >> 16) + (s & 0xFFFF); s += s >> 16 + return ~s & 0xFFFF + +def build_udp(sp, dp, payload, sip, dip): + ln = 8 + len(payload) + pseudo = socket.inet_aton(sip) + socket.inet_aton(dip) + struct.pack("!BBH", 0, socket.IPPROTO_UDP, ln) + raw = struct.pack("!HHHH", sp, dp, ln, 0) + payload + return struct.pack("!HHHH", sp, dp, ln, checksum(pseudo + raw)) + payload + +def build_ip(sip, dip, udp): + tl = 20 + len(udp) + return struct.pack("!BBHHHBBH4s4s", 0x45, 0, tl, 0xABCD, 0, 64, # cspell:disable-line + socket.IPPROTO_UDP, 0, socket.inet_aton(sip), socket.inet_aton(dip)) + udp + +pkt = build_ip(DST_IP, DST_IP, build_udp(SRC_PORT, DST_PORT, PAYLOAD, DST_IP, DST_IP)) +with socket.socket(socket.AF_INET, socket.SOCK_RAW, socket.IPPROTO_RAW) as s: + s.setsockopt(socket.IPPROTO_IP, socket.IP_HDRINCL, 1) + s.sendto(pkt, (DST_IP, 0)) +print(f"Sent BEP 15 connect request src={DST_IP}:{SRC_PORT} dst={DST_IP}:{DST_PORT}") +``` + +## Observed Behaviour (Bug Confirmed) + +The tracker received the request, processed it fully (parsed the connect request, +generated a connect response), then tried to send the response back to `127.0.0.1:0` +and received `EINVAL` (OS error 22). The failure was surfaced as a `WARN` log: + +```text +2026-07-21T16:58:50.032701Z WARN process_request:send_response{client_socket_addr=127.0.0.1:0 response=Connect(ConnectResponse { transaction_id: TransactionId(I32(-559038737)), connection_id: ConnectionId(I64(-4357419529092936579)) }) opt_req_kind=Some(Connect) req_processing_time=54.673µs}: torrust_tracker_udp_server::server::processor: failed to send bytes_count=16 error=Invalid argument (os error 22) payload=[0, 0, 0, 0, 222, 173, 190, 239, 195, 135, 86, 6, 95, 16, 204, 125] +``` + +### Key observations from the log line + +| Field | Value | Meaning | +| --------------------- | ---------------------------------- | -------------------------------------------------------- | +| `client_socket_addr` | `127.0.0.1:0` | Source port is 0 — undeliverable | +| `response` | `Connect(ConnectResponse { ... })` | Request was fully processed before the error | +| `req_processing_time` | `54.673µs` | CPU was spent on a request that can never be answered | +| `error` | `Invalid argument (os error 22)` | `EINVAL` from `sendto(2)` — OS refuses to send to port 0 | +| `bytes_count` | `16` | Full 16-byte connect response was serialized | + +### What should happen instead (after the fix) + +The request should be discarded **before** any parsing or processing. No response +is serialized, no `WARN` is emitted. The `udp_requests_discarded` statistics +counter increments by 1. diff --git a/project-words.txt b/project-words.txt index e15744d05..7d4af6df4 100644 --- a/project-words.txt +++ b/project-words.txt @@ -86,7 +86,9 @@ cves Cyberneering cyclomatic dashmap +dport datagram +datagrams datetime dbip dbname @@ -166,6 +168,7 @@ hotspot hotspots httpclientpeerid Hydranode +HDRINCL hyperium hyperthread Icelake @@ -230,6 +233,7 @@ metainfo microbenchmark microbenchmarks middlewares +middlebox millis miniz miniz_oxide @@ -248,6 +252,8 @@ mysqld ñaca Naim nanos +nmap +nping newkey newtrackon newtype @@ -316,6 +322,7 @@ readelf realpath reannounce recaches +recvfrom recognised recompiles recvspace @@ -351,6 +358,7 @@ Rustls rustup Ryzen sarif +sendto savepath scanf sccache From 15e24e213220b920aec044c85904c98fc4430164 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 18:25:29 +0100 Subject: [PATCH 02/18] feat(udp-server): discard requests from clients with source port 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1450. UDP datagrams with source port 0 are undeliverable — sending a response to :0 always fails with EINVAL (os error 22). Previously the tracker processed the full request and only discovered the problem at send_to time, producing a noisy WARN log in production. Changes: - Add early guard in Processor::process_request: if client port == 0, emit a UdpRequestDiscarded stats event and return immediately before any parsing or handler invocation. - Add Event::UdpRequestDiscarded to event.rs. - Add udp_tracker_server_requests_discarded_total metric counter. - Add statistics event handler request_discarded.rs (follows the pattern of request_aborted.rs). - Register the new handler in the event handler dispatch table. - Add unit tests: - handler increments the discarded counter on UdpRequestDiscarded. - processor discards the request (no response, counter incremented) when client source port is 0. --- packages/udp-server/src/event.rs | 3 + packages/udp-server/src/server/processor.rs | 96 +++++++++++++++++++ .../src/statistics/event/handler/mod.rs | 4 + .../event/handler/request_discarded.rs | 60 ++++++++++++ packages/udp-server/src/statistics/metrics.rs | 15 ++- packages/udp-server/src/statistics/mod.rs | 9 ++ .../udp-server/src/statistics/repository.rs | 5 + 7 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 packages/udp-server/src/statistics/event/handler/request_discarded.rs diff --git a/packages/udp-server/src/event.rs b/packages/udp-server/src/event.rs index 8e93105cd..456cfb6ab 100644 --- a/packages/udp-server/src/event.rs +++ b/packages/udp-server/src/event.rs @@ -16,6 +16,9 @@ pub enum Event { UdpRequestReceived { context: ConnectionContext, }, + UdpRequestDiscarded { + context: ConnectionContext, + }, UdpRequestAborted { context: ConnectionContext, }, diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index cd0dbb1cd..4a95f8e13 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -52,6 +52,26 @@ impl Processor { pub async fn process_request(self, request: RawRequest) { let client_socket_addr = request.from; + // Guard: discard requests from clients with port 0. + // + // Sending a UDP response to port 0 is rejected by the OS with EINVAL. + // We discard such requests immediately and record them in statistics so + // operators can detect scanner activity or misconfigured clients without + // filling the log with noise. + if client_socket_addr.port() == 0 { + tracing::trace!(%client_socket_addr, "discarding request: client source port is 0"); + + if let Some(sender) = self.udp_tracker_server_container.stats_event_sender.as_deref() { + sender + .send(Event::UdpRequestDiscarded { + context: ConnectionContext::new(client_socket_addr, self.server_service_binding), + }) + .await; + } + + return; + } + let start_time = Instant::now(); let (response, opt_req_kind) = handlers::handle_packet( @@ -143,3 +163,79 @@ impl Processor { self.socket.send_to(payload, target).await } } + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; + use std::sync::Arc; + use std::time::Duration; + + use tokio_util::sync::CancellationToken; + use torrust_tracker_test_helpers::configuration; + + use crate::RawRequest; + use crate::server::bound_socket::BoundSocket; + use crate::server::processor::Processor; + use crate::statistics::event::listener; + use crate::testing::environment::EnvContainer; + + fn request_from(addr: SocketAddr) -> RawRequest { + RawRequest { + payload: vec![], + from: addr, + } + } + + /// Scenario: the tracker receives a UDP request whose source port is 0. + /// + /// A source port of 0 is invalid — the OS rejects `send_to` with EINVAL, so + /// there is no point in processing or responding to the request. The tracker + /// should: + /// + /// 1. Discard the request immediately (no handlers invoked, no response sent). + /// 2. Count the discarded request in statistics so operators can detect + /// scanner activity or abuse without relying on log noise. + #[tokio::test] + async fn udp_tracker_discards_requests_from_clients_with_port_0_and_counts_them_in_statistics() { + let cfg = configuration::ephemeral(); + let core_config = Arc::new(cfg.core.clone()); + let udp_tracker_config = Arc::new(cfg.udp_trackers.unwrap()[0].clone()); + + let container = Arc::new(EnvContainer::initialize(&core_config, &udp_tracker_config).await); + + // Start the stats event listener so that emitted events update the repository. + let cancellation_token = CancellationToken::new(); + let _event_listener_job = listener::run_event_listener( + container.udp_tracker_server_container.event_bus.receiver(), + cancellation_token.clone(), + &container.udp_tracker_server_container.stats_repository, + ); + + // Create a processor backed by an ephemeral socket. + let socket = Arc::new(BoundSocket::new("0.0.0.0:0".parse().unwrap(), false).expect("Failed to bind socket")); + let processor = Processor::new( + socket, + container.udp_tracker_core_container.clone(), + container.udp_tracker_server_container.clone(), + udp_tracker_config.cookie_lifetime.as_secs_f64(), + ); + + // Submit a request from a client whose source port is 0. + let client_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); + processor.process_request(request_from(client_addr)).await; + + // Give the async event listener time to process the emitted event. + tokio::time::sleep(Duration::from_millis(50)).await; + + // The discarded counter must be 1; all other counters must stay at 0. + let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; + assert_eq!(stats.udp_requests_discarded_total(), 1, "expected 1 discarded request"); + assert_eq!( + stats.udp4_requests_received_total(), + 0, + "a port-0 request must not count as received" + ); + + cancellation_token.cancel(); + } +} diff --git a/packages/udp-server/src/statistics/event/handler/mod.rs b/packages/udp-server/src/statistics/event/handler/mod.rs index 34f1ddc60..f357a2cee 100644 --- a/packages/udp-server/src/statistics/event/handler/mod.rs +++ b/packages/udp-server/src/statistics/event/handler/mod.rs @@ -2,6 +2,7 @@ mod error; mod request_aborted; mod request_accepted; mod request_banned; +mod request_discarded; mod request_received; mod response_sent; @@ -15,6 +16,9 @@ pub async fn handle_event(event: Event, stats_repository: &Repository, now: Dura Event::UdpRequestAborted { context } => { request_aborted::handle_event(context, stats_repository, now).await; } + Event::UdpRequestDiscarded { context } => { + request_discarded::handle_event(context, stats_repository, now).await; + } Event::UdpRequestBanned { context } => { request_banned::handle_event(context, stats_repository, now).await; } diff --git a/packages/udp-server/src/statistics/event/handler/request_discarded.rs b/packages/udp-server/src/statistics/event/handler/request_discarded.rs new file mode 100644 index 000000000..6e2b8bdd0 --- /dev/null +++ b/packages/udp-server/src/statistics/event/handler/request_discarded.rs @@ -0,0 +1,60 @@ +use torrust_clock::DurationSinceUnixEpoch; +use torrust_metrics::label::LabelSet; +use torrust_metrics::metric_name; +use torrust_tracker_udp_core::event::ConnectionContext; + +use crate::statistics::UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL; +use crate::statistics::repository::Repository; + +pub async fn handle_event(context: ConnectionContext, stats_repository: &Repository, now: DurationSinceUnixEpoch) { + match stats_repository + .increase_counter( + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL), + &LabelSet::from(context), + now, + ) + .await + { + Ok(()) => {} + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + } +} + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; + + use torrust_clock::clock::Time; + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; + use torrust_tracker_udp_core::event::ConnectionContext; + + use crate::CurrentClock; + use crate::event::Event; + use crate::statistics::event::handler::handle_event; + use crate::statistics::repository::Repository; + + #[tokio::test] + async fn it_should_increase_the_number_of_discarded_requests_when_it_receives_a_udp_request_discarded_event() { + let stats_repository = Repository::new(); + + handle_event( + Event::UdpRequestDiscarded { + context: ConnectionContext::new( + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 0), + ServiceBinding::new( + Protocol::UDP, + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 196)), 6969), + ) + .unwrap(), + ), + }, + &stats_repository, + CurrentClock::now(), + ) + .await; + + let stats = stats_repository.get_stats().await; + + assert_eq!(stats.udp_requests_discarded_total(), 1); + } +} diff --git a/packages/udp-server/src/statistics/metrics.rs b/packages/udp-server/src/statistics/metrics.rs index ab674cc40..350fd39b3 100644 --- a/packages/udp-server/src/statistics/metrics.rs +++ b/packages/udp-server/src/statistics/metrics.rs @@ -13,8 +13,8 @@ use crate::statistics::{ UDP_TRACKER_SERVER_ERRORS_TOTAL, UDP_TRACKER_SERVER_IPS_BANNED_TOTAL, UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL, UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS, UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL, UDP_TRACKER_SERVER_REQUESTS_ACCEPTED_TOTAL, - UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL, UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL, - UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL, + UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL, UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL, + UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL, UDP_TRACKER_SERVER_RESPONSES_SENT_TOTAL, }; /// Metrics collected by the UDP tracker server. @@ -157,6 +157,17 @@ impl Metrics { .unwrap_or_default() as u64 } + /// Total number of UDP (UDP tracker) requests discarded before processing + /// (e.g. because the client source port is 0). + #[must_use] + #[allow(clippy::cast_sign_loss)] + #[allow(clippy::cast_possible_truncation)] + pub fn udp_requests_discarded_total(&self) -> u64 { + self.metric_collection + .sum(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL), &LabelSet::empty()) + .unwrap_or_default() as u64 + } + /// Total number of UDP (UDP tracker) requests banned. #[must_use] #[allow(clippy::cast_sign_loss)] diff --git a/packages/udp-server/src/statistics/mod.rs b/packages/udp-server/src/statistics/mod.rs index 7dc5b4a00..5b4f61d15 100644 --- a/packages/udp-server/src/statistics/mod.rs +++ b/packages/udp-server/src/statistics/mod.rs @@ -10,6 +10,7 @@ use torrust_metrics::unit::Unit; pub const UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL: &str = "udp_tracker_server_requests_aborted_total"; pub const UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL: &str = "udp_tracker_server_requests_banned_total"; +pub const UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL: &str = "udp_tracker_server_requests_discarded_total"; pub const UDP_TRACKER_SERVER_IPS_BANNED_TOTAL: &str = "udp_tracker_server_ips_banned_total"; pub const UDP_TRACKER_SERVER_CONNECTION_ID_ERRORS_TOTAL: &str = "udp_tracker_server_connection_id_errors_total"; pub const UDP_TRACKER_SERVER_REQUESTS_RECEIVED_TOTAL: &str = "udp_tracker_server_requests_received_total"; @@ -30,6 +31,14 @@ pub fn describe_metrics() -> Metrics { Some(MetricDescription::new("Total number of UDP requests aborted")), ); + metrics.metric_collection.describe_counter( + &metric_name!(UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL), + Some(Unit::Count), + Some(MetricDescription::new( + "Total number of UDP requests discarded before processing (e.g. client source port is 0)", + )), + ); + metrics.metric_collection.describe_counter( &metric_name!(UDP_TRACKER_SERVER_REQUESTS_BANNED_TOTAL), Some(Unit::Count), diff --git a/packages/udp-server/src/statistics/repository.rs b/packages/udp-server/src/statistics/repository.rs index cdc942e64..c38e5cd5e 100644 --- a/packages/udp-server/src/statistics/repository.rs +++ b/packages/udp-server/src/statistics/repository.rs @@ -140,6 +140,11 @@ mod tests { .metric_collection .contains_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_ABORTED_TOTAL)) ); + assert!( + stats + .metric_collection + .contains_counter(&metric_name!(UDP_TRACKER_SERVER_REQUESTS_DISCARDED_TOTAL)) + ); assert!( stats .metric_collection From 9b647b4604f07dac9225c95f1206b86e1d4b5bd6 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 18:33:25 +0100 Subject: [PATCH 03/18] feat(rest-api): expose udp_requests_discarded in stats endpoint Wires the new UdpRequestDiscarded counter into the REST API stats response so operators can observe discarded port-0 requests via the /api/v1/stats endpoint alongside the existing aborted/banned counters. Changes: - Add udp_requests_discarded field to Stats resource struct. - Map udp_requests_discarded_total() from UDP server metrics in the stats adapter. - Add plain-text line to the stats response formatter. - Update the contract test fixture with the new field (value 0). - Add after-fix manual verification evidence for issue #1450. Verified end-to-end: udp_requests_discarded: 1 (after one crafted port-0 datagram) udp4_responses: 0 (no response sent) WARN log: absent (no more 'os error 22' noise) --- .../evidence/after-fix-manual-verification.md | 76 +++++++++++++++++++ .../src/v1/context/stats/responses.rs | 1 + .../tests/server/v1/contract/context/stats.rs | 1 + .../src/v1/context/stats/resources/stats.rs | 2 + .../src/v1/adapters/stats.rs | 1 + 5 files changed, 81 insertions(+) create mode 100644 docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/after-fix-manual-verification.md diff --git a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/after-fix-manual-verification.md b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/after-fix-manual-verification.md new file mode 100644 index 000000000..4a3779440 --- /dev/null +++ b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/after-fix-manual-verification.md @@ -0,0 +1,76 @@ +# After-Fix Manual Verification — Issue #1450 + +**Date**: 2026-07-21 +**Branch**: `1450-discard-udp-requests-from-clients-with-port-0` +**Tracker version**: `3.0.0-develop` (commit `86bb083b`) +**Config**: `share/default/config/tracker.development.sqlite3.toml` + +## Setup + +```sh +cargo build --bin torrust-tracker +TORRUST_TRACKER_CONFIG_TOML_FILE=share/default/config/tracker.development.sqlite3.toml \ + ./target/debug/torrust-tracker > .tmp/tracker-run-fixed.log 2>&1 & +``` + +## Packet sent + +```sh +sudo python3 .tmp/send_port0_udp.py +# Sent BEP 15 connect request src=127.0.0.1:0 dst=127.0.0.1:6969 +``` + +The script crafts a raw IP/UDP datagram with source port 0 using `socket.IPPROTO_RAW` +and `IP_HDRINCL`, bypassing the OS socket API which would otherwise assign a non-zero +ephemeral port. + +## Result + +### No WARN log (fixed) + +```sh +grep -i "warn\|error 22\|failed to send" .tmp/tracker-run-fixed.log +# (no output — WARN is gone) +``` + +Before the fix the following line appeared in the tracker log every time a port-0 +datagram arrived: + +```text +WARN process_request:send_response{...}: torrust_udp_tracker_server::server::processor: +failed to send bytes_count=16 error=Invalid argument (os error 22) +``` + +After the fix, no such line appears. + +### Stats counter incremented + +```sh +curl -s "http://localhost:1212/api/v1/stats?token=MyAccessToken" | python3 -m json.tool +``` + +Relevant fields from the response after one port-0 datagram: + +```json +{ + "udp_requests_discarded": 1, + "udp_requests_aborted": 0, + "udp4_requests": 1, + "udp4_responses": 0 +} +``` + +| Field | Value | Meaning | +| ------------------------ | ----- | -------------------------------------------------------- | +| `udp_requests_discarded` | **1** | Request was counted and discarded | +| `udp4_requests` | 1 | Datagram was received by the socket | +| `udp4_responses` | **0** | No response was sent (correct — port 0 is undeliverable) | +| `udp_requests_aborted` | 0 | Not aborted — discarded before any processing | + +## Summary + +The fix works as designed: + +- The WARN log no longer pollutes production logs. +- The request is discarded silently before any parsing or handler invocation. +- The `udp_requests_discarded` counter gives operators a clean signal via the stats API. diff --git a/packages/axum-rest-api-server/src/v1/context/stats/responses.rs b/packages/axum-rest-api-server/src/v1/context/stats/responses.rs index 92be842d7..927f35436 100644 --- a/packages/axum-rest-api-server/src/v1/context/stats/responses.rs +++ b/packages/axum-rest-api-server/src/v1/context/stats/responses.rs @@ -41,6 +41,7 @@ pub fn metrics_response(stats: &Stats) -> Response { lines.push(format!("tcp6_scrapes_handled {}", stats.tcp6_scrapes_handled)); // UDP + lines.push(format!("udp_requests_discarded {}", stats.udp_requests_discarded)); lines.push(format!("udp_requests_aborted {}", stats.udp_requests_aborted)); lines.push(format!("udp_requests_banned {}", stats.udp_requests_banned)); lines.push(format!("udp_banned_ips_total {}", stats.udp_banned_ips_total)); diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs b/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs index 33dd439eb..20278530c 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs @@ -47,6 +47,7 @@ async fn should_allow_getting_tracker_statistics() { tcp6_announces_handled: 0, tcp6_scrapes_handled: 0, // UDP + udp_requests_discarded: 0, udp_requests_aborted: 0, udp_requests_banned: 0, udp_banned_ips_total: 0, diff --git a/packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs b/packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs index 12f7f1aa2..dc58d6102 100644 --- a/packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs +++ b/packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs @@ -33,6 +33,8 @@ pub struct Stats { pub tcp6_scrapes_handled: u64, // UDP + /// Total number of UDP (UDP tracker) requests discarded before processing (e.g. client source port is 0). + pub udp_requests_discarded: u64, /// Total number of UDP (UDP tracker) requests aborted. pub udp_requests_aborted: u64, /// Total number of UDP (UDP tracker) requests banned. diff --git a/packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs b/packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs index 83ddac976..69ba50d17 100644 --- a/packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs +++ b/packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs @@ -74,6 +74,7 @@ impl StatsQueryPort for TrackerStatsAdapter { tcp6_scrapes_handled: http_stats.tcp6_scrapes_handled(), // UDP + udp_requests_discarded: udp_server_stats.udp_requests_discarded_total(), udp_requests_aborted: udp_server_stats.udp_requests_aborted_total(), udp_requests_banned: udp_server_stats.udp_requests_banned_total(), udp_banned_ips_total: udp_server_stats.udp_banned_ips_total(), From 62cb3d27aa024c06a6fa2c61df02f8eba4064dfd Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 19:11:07 +0100 Subject: [PATCH 04/18] fix(review): fix word ordering and improve flaky test - project-words.txt: fix alphabetical ordering of dport, HDRINCL, middlebox, and sendto (all were placed out of case-insensitive order) - processor.rs: replace fixed sleep(50ms) with bounded timeout+poll to avoid flaky behaviour on slow CI runners - processor.rs: clarify assertion message for udp4_requests_received_total to reflect that UdpRequestReceived is emitted by the launcher, not the processor, so the counter is always 0 in unit tests that bypass the launcher --- packages/udp-server/src/server/processor.rs | 19 ++++++++++++++++--- project-words.txt | 8 ++++---- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index 4a95f8e13..5754a54e2 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -224,16 +224,29 @@ mod tests { let client_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); processor.process_request(request_from(client_addr)).await; - // Give the async event listener time to process the emitted event. - tokio::time::sleep(Duration::from_millis(50)).await; + // Wait (bounded) for the async event listener to process the discarded event. + tokio::time::timeout(Duration::from_secs(1), async { + loop { + let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; + if stats.udp_requests_discarded_total() >= 1 { + break; + } + tokio::time::sleep(Duration::from_millis(1)).await; + } + }) + .await + .expect("timed out waiting for the stats event listener to record the discarded event"); // The discarded counter must be 1; all other counters must stay at 0. let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; assert_eq!(stats.udp_requests_discarded_total(), 1, "expected 1 discarded request"); + // Note: `UdpRequestReceived` is emitted by the launcher before `process_request` is + // called, so this counter is always 0 in tests that invoke the processor directly. + // This assertion confirms the processor itself does not emit that event. assert_eq!( stats.udp4_requests_received_total(), 0, - "a port-0 request must not count as received" + "the processor does not emit UdpRequestReceived; that is the launcher's responsibility" ); cancellation_token.cancel(); diff --git a/project-words.txt b/project-words.txt index 7d4af6df4..3dc24a56d 100644 --- a/project-words.txt +++ b/project-words.txt @@ -86,9 +86,9 @@ cves Cyberneering cyclomatic dashmap -dport datagram datagrams +dport datetime dbip dbname @@ -156,6 +156,7 @@ Glrg Graphviz Grcov hasher +HDRINCL healthcheck heaptrack hexdigit @@ -168,7 +169,6 @@ hotspot hotspots httpclientpeerid Hydranode -HDRINCL hyperium hyperthread Icelake @@ -232,8 +232,8 @@ Mebibytes metainfo microbenchmark microbenchmarks -middlewares middlebox +middlewares millis miniz miniz_oxide @@ -358,11 +358,11 @@ Rustls rustup Ryzen sarif -sendto savepath scanf sccache Seedable +sendto serde serialisation setgroups From b1ebbb795c4df59a16deb14ec6fd3ce846bb6355 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 19:25:47 +0100 Subject: [PATCH 05/18] fix(review): fix additional word ordering and clarify doc comment - project-words.txt: fix case-insensitive sort order for nmap/nping (moved after new*/nextest/nghttp/ngtcp/nocapture/nologin) and for recvfrom (moved after recognised and recompiles) - processor.rs: replace 'source port 0 is invalid' with more accurate wording noting RFC 768 does not forbid port 0; the real issue is the OS rejects send_to to port 0 with EINVAL --- packages/udp-server/src/server/processor.rs | 5 +++-- project-words.txt | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index 5754a54e2..027eca498 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -188,8 +188,9 @@ mod tests { /// Scenario: the tracker receives a UDP request whose source port is 0. /// - /// A source port of 0 is invalid — the OS rejects `send_to` with EINVAL, so - /// there is no point in processing or responding to the request. The tracker + /// Although RFC 768 does not forbid a source port of 0, the OS rejects any + /// `send_to` directed at port 0 with EINVAL, so there is no point in + /// processing or responding to such requests. The tracker /// should: /// /// 1. Discard the request immediately (no handlers invoked, no response sent). diff --git a/project-words.txt b/project-words.txt index 3dc24a56d..dff6e29f8 100644 --- a/project-words.txt +++ b/project-words.txt @@ -252,8 +252,6 @@ mysqld ñaca Naim nanos -nmap -nping newkey newtrackon newtype @@ -263,6 +261,8 @@ nghttp ngtcp nocapture nologin +nmap +nping nonblocking nonroot Norberg @@ -322,9 +322,9 @@ readelf realpath reannounce recaches -recvfrom recognised recompiles +recvfrom recvspace referer Registar From 571d801edee5cc1e82510e9f13ed7c15c4e51887 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 20:45:48 +0100 Subject: [PATCH 06/18] docs(review): document PR #2017 copilot suggestions audit 9 threads processed (all action): 6 in first round, 3 after re-review on push. All threads replied and resolved. --- .../pr-reviews/pr-2017-copilot-suggestions.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 docs/pr-reviews/pr-2017-copilot-suggestions.md diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md new file mode 100644 index 000000000..b75e9bae7 --- /dev/null +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -0,0 +1,60 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +--- + + + + + +# PR #2017 Copilot Suggestions Tracking + +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/2017 + +Status legend: + +- `action`: code/docs change applied +- `no-action`: suggestion reviewed; no code change needed +- `resolved`: thread resolved in PR + +## Workflow + +1. Download all review threads (including resolved/outdated state and thread IDs). +2. Add one row per thread in the Suggestions table. +3. Process suggestions one by one: + - decide `action` or `no-action` + - if `action`, apply change and validate + - if needed, commit changes + - reply on the PR thread with the fix commit and outcome, or the no-action rationale + - resolve the PR thread + +4. Set `Thread State` to `resolved` once resolved in PR. + +## Processing Log + +- 2026-07-21: Started processing suggestions (9 threads across 2 pushes). +- 2026-07-21: Completed processing all suggestions. All 9 threads resolved. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ------------------------------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | -------- | --------------------------------------------------------------------------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | +| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | +| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | +| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | +| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | +| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | +| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | +| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | +| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | + +## Notes + +- Keep this file as an audit log of review handling for the PR. +- Prefer concise decisions with explicit rationale. +- If no code changes are needed, explain why in `Decision`. +- Reply on every PR suggestion thread before resolving it so the decision is visible to reviewers. From 509b78c8a62d4e8aa751047d94e3712536acc673 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 21:37:56 +0100 Subject: [PATCH 07/18] test(udp-server): split processor port-0 test into focused AAA tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single combined test with two focused tests, each covering one responsibility of Processor::process_request for the port-0 case: - processor_does_not_send_a_response_when_client_port_is_0 Asserts that udp4_responses_sent_total and udp6_responses_sent_total remain 0 after processing a port-0 request (early return, no send_response call). - processor_emits_discard_event_when_client_port_is_0 Asserts that udp_requests_discarded_total == 1, confirming that Event::UdpRequestDiscarded was emitted to the stats bus. Also extract two shared test helpers to make each test body a clear three-section Arrange / Act / Assert: - setup_processor_with_stats_listener() — all environment boilerplate - wait_for_discarded_count() — bounded async poll The removed assertion (udp4_requests_received_total == 0) tested the launcher's responsibility, not the processor's, so it was dropped. --- packages/udp-server/src/server/processor.rs | 101 +++++++++++++++----- 1 file changed, 75 insertions(+), 26 deletions(-) diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index 027eca498..d80dfb98c 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -179,6 +179,10 @@ mod tests { use crate::statistics::event::listener; use crate::testing::environment::EnvContainer; + // ----------------------------------------------------------------------- + // Test helpers + // ----------------------------------------------------------------------- + fn request_from(addr: SocketAddr) -> RawRequest { RawRequest { payload: vec![], @@ -186,33 +190,27 @@ mod tests { } } - /// Scenario: the tracker receives a UDP request whose source port is 0. + /// Creates an ephemeral tracker environment, wires up the stats event + /// listener, and returns a ready-to-use `Processor`. /// - /// Although RFC 768 does not forbid a source port of 0, the OS rejects any - /// `send_to` directed at port 0 with EINVAL, so there is no point in - /// processing or responding to such requests. The tracker - /// should: - /// - /// 1. Discard the request immediately (no handlers invoked, no response sent). - /// 2. Count the discarded request in statistics so operators can detect - /// scanner activity or abuse without relying on log noise. - #[tokio::test] - async fn udp_tracker_discards_requests_from_clients_with_port_0_and_counts_them_in_statistics() { + /// The caller receives: + /// - `processor` — consumes itself in `process_request`. + /// - `container` — holds the stats repository for later assertions. + /// - `cancellation_token` — cancel it after the test to stop the listener. + async fn setup_processor_with_stats_listener() -> (Processor, Arc, CancellationToken) { let cfg = configuration::ephemeral(); let core_config = Arc::new(cfg.core.clone()); let udp_tracker_config = Arc::new(cfg.udp_trackers.unwrap()[0].clone()); let container = Arc::new(EnvContainer::initialize(&core_config, &udp_tracker_config).await); - // Start the stats event listener so that emitted events update the repository. let cancellation_token = CancellationToken::new(); - let _event_listener_job = listener::run_event_listener( + let _listener_job = listener::run_event_listener( container.udp_tracker_server_container.event_bus.receiver(), cancellation_token.clone(), &container.udp_tracker_server_container.stats_repository, ); - // Create a processor backed by an ephemeral socket. let socket = Arc::new(BoundSocket::new("0.0.0.0:0".parse().unwrap(), false).expect("Failed to bind socket")); let processor = Processor::new( socket, @@ -221,15 +219,16 @@ mod tests { udp_tracker_config.cookie_lifetime.as_secs_f64(), ); - // Submit a request from a client whose source port is 0. - let client_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); - processor.process_request(request_from(client_addr)).await; + (processor, container, cancellation_token) + } - // Wait (bounded) for the async event listener to process the discarded event. + /// Polls the stats repository until `udp_requests_discarded_total` reaches + /// `expected`, or panics after one second. + async fn wait_for_discarded_count(container: &Arc, expected: u64) { tokio::time::timeout(Duration::from_secs(1), async { loop { let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; - if stats.udp_requests_discarded_total() >= 1 { + if stats.udp_requests_discarded_total() >= expected { break; } tokio::time::sleep(Duration::from_millis(1)).await; @@ -237,17 +236,67 @@ mod tests { }) .await .expect("timed out waiting for the stats event listener to record the discarded event"); + } + + // ----------------------------------------------------------------------- + // Tests + // ----------------------------------------------------------------------- - // The discarded counter must be 1; all other counters must stay at 0. + /// Scenario: the tracker receives a UDP request whose source port is 0. + /// + /// The processor must return immediately without calling `send_response`. + /// Sending to port 0 would be rejected by the OS with EINVAL; the early + /// exit avoids the wasted work and the resulting WARN log noise. + #[tokio::test] + async fn processor_does_not_send_a_response_when_client_port_is_0() { + // Arrange + let (processor, container, cancellation_token) = setup_processor_with_stats_listener().await; + let client_with_port_0 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); + + // Act + processor.process_request(request_from(client_with_port_0)).await; + // Sync: wait until the discard event is processed so the stats + // are settled before we assert on the response counters. + wait_for_discarded_count(&container, 1).await; + + // Assert: no response was sent (neither IPv4 nor IPv6 channel). let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; - assert_eq!(stats.udp_requests_discarded_total(), 1, "expected 1 discarded request"); - // Note: `UdpRequestReceived` is emitted by the launcher before `process_request` is - // called, so this counter is always 0 in tests that invoke the processor directly. - // This assertion confirms the processor itself does not emit that event. assert_eq!( - stats.udp4_requests_received_total(), + stats.udp4_responses_sent_total(), + 0, + "no IPv4 response should be sent to port 0" + ); + assert_eq!( + stats.udp6_responses_sent_total(), 0, - "the processor does not emit UdpRequestReceived; that is the launcher's responsibility" + "no IPv6 response should be sent to port 0" + ); + + cancellation_token.cancel(); + } + + /// Scenario: the tracker receives a UDP request whose source port is 0. + /// + /// The processor must emit `Event::UdpRequestDiscarded` so that the stats + /// counter increments. This gives operators a clean signal (via the REST + /// stats endpoint) to detect scanner activity or abuse without relying on + /// log noise. + #[tokio::test] + async fn processor_emits_discard_event_when_client_port_is_0() { + // Arrange + let (processor, container, cancellation_token) = setup_processor_with_stats_listener().await; + let client_with_port_0 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); + + // Act + processor.process_request(request_from(client_with_port_0)).await; + + // Assert: the discard event was emitted and the counter reflects it. + wait_for_discarded_count(&container, 1).await; + let stats = container.udp_tracker_server_container.stats_repository.get_stats().await; + assert_eq!( + stats.udp_requests_discarded_total(), + 1, + "expected exactly 1 discarded request" ); cancellation_token.cancel(); From 6f15d60f8510014d9c0df60a45cdbd6ad1b5cd52 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 21:48:53 +0100 Subject: [PATCH 08/18] =?UTF-8?q?docs(issue-1450):=20clarify=20port-0=20wo?= =?UTF-8?q?rding=20=E2=80=94=20RFC=20768=20does=20not=20forbid=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace 'This is an invalid socket address' with 'Although RFC 768 does not forbid source port 0, no response can ever be delivered to :0' in the issue spec Background section. Consistent with the identical wording fix applied to processor.rs in the previous Copilot review round (thread #9). Addresses Copilot PR review thread #10 on PR #2017. --- .../ISSUE.md | 10 ++++---- .../pr-reviews/pr-2017-copilot-suggestions.md | 24 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md index 5c6e19f31..7d55bbfdf 100644 --- a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md +++ b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md @@ -53,11 +53,11 @@ them just like any other UDP packet. ### Current behaviour The tracker received UDP packets from clients whose source port is `0` in the UDP -header. This is an invalid socket address — no response can ever be delivered to -`:0`. The current code processes the request fully (parses it, executes the -handler, serializes the response) and only discovers the problem when it calls -`send_to`, which returns `EINVAL` (OS error 22). The failure is then logged as a -`WARN`, polluting production logs. +header. Although RFC 768 does not forbid source port 0, no response can ever be +delivered to `:0`. The current code processes the request fully (parses it, +executes the handler, serializes the response) and only discovers the problem when +it calls `send_to`, which returns `EINVAL` (OS error 22). The failure is then +logged as a `WARN`, polluting production logs. Example from the demo tracker logs: diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md index b75e9bae7..3880954b1 100644 --- a/docs/pr-reviews/pr-2017-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -37,20 +37,22 @@ Status legend: - 2026-07-21: Started processing suggestions (9 threads across 2 pushes). - 2026-07-21: Completed processing all suggestions. All 9 threads resolved. +- 2026-07-21: New thread (PRRT_kwDOGp2yqc6StQu4) found on re-check after push. Applied fix and resolved. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | --------------------- | ------------------------------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | -------- | --------------------------------------------------------------------------- | ------ | ------------ | -| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | -| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | -| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | -| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | -| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | -| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | -| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | -| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | -| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------ | -------- | --------------------------------------------------------------------------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | +| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | +| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | +| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | +| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | +| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | +| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | +| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | +| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | +| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | TBD | DONE | RESOLVED | ## Notes From 5c1f8f326269fbc830f36ca3420a2a9da6e09a34 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 07:25:53 +0100 Subject: [PATCH 09/18] refactor(udp-server): enforce non-zero port invariant in BoundSocket BoundSocket now documents and enforces the invariant that the bound port is always non-zero: - Rename BoundSocket::new to BoundSocket::bind, making the action explicit (named constructor that performs binding, not a plain allocator). - Add a post-bind check: if the OS somehow assigns port 0 after a successful bind, BoundSocket::bind returns Err instead of silently producing an invalid socket. In practice this cannot happen, but making it explicit removes the ambiguity. - Update the type-level doc comment to state the invariant. Callers updated: - Processor::new no longer duplicates ServiceBinding::new with a fragile expect(); it delegates to socket.service_binding() which is already guaranteed safe by the BoundSocket invariant. The 'Panics' section is removed from the doc comment because the condition is now prevented at the type boundary. - launcher.rs and the test helper updated to call bind(). --- .../udp-server/src/server/bound_socket.rs | 30 +++++++++++++++---- packages/udp-server/src/server/launcher.rs | 2 +- packages/udp-server/src/server/processor.rs | 13 ++++---- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/packages/udp-server/src/server/bound_socket.rs b/packages/udp-server/src/server/bound_socket.rs index 3e3d90cb9..80e21f23c 100644 --- a/packages/udp-server/src/server/bound_socket.rs +++ b/packages/udp-server/src/server/bound_socket.rs @@ -7,24 +7,42 @@ use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; use torrust_tracker_udp_core::UDP_TRACKER_LOG_TARGET; use url::Url; -/// Wrapper for Tokio [`UdpSocket`][`tokio::net::UdpSocket`] that is bound to a particular socket. +/// A UDP socket that has been successfully bound to a local address with a non-zero port. +/// +/// # Invariant +/// +/// The bound port is always non-zero. If port 0 is passed to [`BoundSocket::bind`], the OS +/// assigns an ephemeral port before construction completes, and the resulting address is +/// verified to have a non-zero port before the value is returned. pub struct BoundSocket { socket: tokio::net::UdpSocket, } impl BoundSocket { + /// Binds a UDP socket to `addr` and returns the bound socket. + /// + /// If `addr.port()` is 0 the OS assigns an ephemeral port; the resulting + /// socket always has a non-zero port (see [`BoundSocket`] invariant). + /// /// # Errors /// - /// Will return an error if the socket can't be bound to the provided address. - pub fn new(addr: SocketAddr, ipv6_v6only: bool) -> Result> { + /// Returns an error if the socket cannot be created or bound, or if the + /// OS unexpectedly assigns port 0 after a successful bind. + pub fn bind(addr: SocketAddr, ipv6_v6only: bool) -> Result> { let bind_addr = format!("udp://{addr}"); - tracing::debug!(target: UDP_TRACKER_LOG_TARGET, bind_addr, "UdpSocket::new (binding)"); + tracing::debug!(target: UDP_TRACKER_LOG_TARGET, bind_addr, "UdpSocket::bind (binding)"); let socket = Self::create_socket(addr, ipv6_v6only)?; let tokio_socket = tokio::net::UdpSocket::from_std(socket)?; - let local_addr = format!("udp://{}", tokio_socket.local_addr()?); - tracing::debug!(target: UDP_TRACKER_LOG_TARGET, local_addr, "UdpSocket::new (bound)"); + let local_addr = tokio_socket.local_addr()?; + tracing::debug!(target: UDP_TRACKER_LOG_TARGET, local_addr = %format!("udp://{local_addr}"), "UdpSocket::bind (bound)"); + + if local_addr.port() == 0 { + return Err(Box::new(std::io::Error::other( + "bound socket has port 0 — OS did not assign an ephemeral port", + ))); + } Ok(Self { socket: tokio_socket }) } diff --git a/packages/udp-server/src/server/launcher.rs b/packages/udp-server/src/server/launcher.rs index 3f05b95c3..0f741cca9 100644 --- a/packages/udp-server/src/server/launcher.rs +++ b/packages/udp-server/src/server/launcher.rs @@ -55,7 +55,7 @@ impl Launcher { panic!("it should not use udp if using authentication"); } - let socket = BoundSocket::new(bind_to, udp_tracker_core_container.udp_tracker_config.ipv6_v6only); + let socket = BoundSocket::bind(bind_to, udp_tracker_core_container.udp_tracker_config.ipv6_v6only); let bound_socket = match socket { Ok(socket) => socket, diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index d80dfb98c..624e67b3b 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use std::time::Duration; use tokio::time::Instant; -use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; +use torrust_net_primitives::service_binding::ServiceBinding; use torrust_tracker_udp_core::container::UdpTrackerCoreContainer; use torrust_tracker_udp_core::event::ConnectionContext; use torrust_tracker_udp_core::{self}; @@ -26,18 +26,15 @@ pub struct Processor { } impl Processor { - /// # Panics - /// - /// It will panic if a bound socket address port is 0. It should never - /// happen. pub fn new( socket: Arc, udp_tracker_core_container: Arc, udp_tracker_server_container: Arc, cookie_lifetime: f64, ) -> Self { - let server_service_binding = - ServiceBinding::new(Protocol::UDP, socket.address()).expect("Bound socket port should't be 0"); + // BoundSocket guarantees a non-zero port by construction, so + // service_binding() cannot fail. + let server_service_binding = socket.service_binding(); Self { socket, @@ -211,7 +208,7 @@ mod tests { &container.udp_tracker_server_container.stats_repository, ); - let socket = Arc::new(BoundSocket::new("0.0.0.0:0".parse().unwrap(), false).expect("Failed to bind socket")); + let socket = Arc::new(BoundSocket::bind("0.0.0.0:0".parse().unwrap(), false).expect("Failed to bind socket")); let processor = Processor::new( socket, container.udp_tracker_core_container.clone(), From 06294354264559e3452207c154e5f346818b7654 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 07:55:13 +0100 Subject: [PATCH 10/18] docs(pr-review): update PR-2017 Copilot suggestions tracking table - Thread #10: fill in Reply URL (discussion_r3628033936) that was left as TBD after the thread was resolved without a posted reply. - Thread #11: add new entry for PRRT_kwDOGp2yqc6SuPys (about the TBD reply URL for thread #10); mark DONE/RESOLVED after posting reply discussion_r3628037362 and resolving the thread. --- docs/pr-reviews/pr-2017-copilot-suggestions.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md index 3880954b1..28f89374c 100644 --- a/docs/pr-reviews/pr-2017-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -52,7 +52,8 @@ Status legend: | 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | | 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | | 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | -| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | TBD | DONE | RESOLVED | +| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | +| 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | ## Notes From 8e55070697e51b511584826d611ff4a2ed82abde Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 07:57:19 +0100 Subject: [PATCH 11/18] docs(pr-review): add thread #12 to PR-2017 Copilot suggestions table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread #12 (PRRT_kwDOGp2yqc6S0eYH) flagged that the processing log said 'All 9 threads resolved' while the table already listed 10 entries. Reworded the log line to 'initial batch' and appended entries for threads #10–#12 individually. --- docs/pr-reviews/pr-2017-copilot-suggestions.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md index 28f89374c..e47558292 100644 --- a/docs/pr-reviews/pr-2017-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -36,8 +36,10 @@ Status legend: ## Processing Log - 2026-07-21: Started processing suggestions (9 threads across 2 pushes). -- 2026-07-21: Completed processing all suggestions. All 9 threads resolved. -- 2026-07-21: New thread (PRRT_kwDOGp2yqc6StQu4) found on re-check after push. Applied fix and resolved. +- 2026-07-21: Completed processing initial batch. All 9 threads resolved. +- 2026-07-21: New thread (PRRT_kwDOGp2yqc6StQu4, thread #10) found on re-check after push. Applied fix and resolved. +- 2026-07-22: New thread (PRRT_kwDOGp2yqc6SuPys, thread #11) found: flagged TBD reply URL for thread #10. Posted reply and resolved. +- 2026-07-22: New thread (PRRT_kwDOGp2yqc6S0eYH, thread #12) found: processing log said "All 9 threads resolved" while table had 10 entries. Reworded log entry to say "initial batch". Applied fix and resolved. ## Suggestions @@ -54,6 +56,7 @@ Status legend: | 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | | 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | | 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | +| 12 | PRRT_kwDOGp2yqc6S0eYH | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3627977934 | Processing log says "All 9 threads resolved" but table lists 10; reword to "initial batch" | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628134459 | DONE | RESOLVED | ## Notes From b4fb60ce7a5ecb8813a567521619623daea18e80 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 08:50:01 +0100 Subject: [PATCH 12/18] feat(udp-server): discard port-0 requests in launcher before spawning Move the primary port-0 discard to the launcher loop, before a processing task is spawned and force-pushed into the active-requests buffer. Previously the guard only ran inside Processor::process_request, which meant a port-0 flood could still churn the active-requests buffer and evict legitimate in-flight requests, even though each discarded request returned immediately once scheduled. The launcher check mirrors the existing banned-IP pattern (check -> emit UdpRequestDiscarded -> continue). The guard in Processor::process_request is kept as defense-in-depth for any other caller; in production it never fires, so each discarded request is counted exactly once. ISSUE.md design section updated to document the two-layer detection. Suggested by Copilot review (thread PRRT_kwDOGp2yqc6S0_RT). --- .../ISSUE.md | 18 +++++++++++++--- packages/udp-server/src/server/launcher.rs | 21 +++++++++++++++++++ packages/udp-server/src/server/processor.rs | 5 +++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md index 7d55bbfdf..50c630e45 100644 --- a/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md +++ b/docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md @@ -79,9 +79,18 @@ them and should not fill logs with OS-level errors caused by user-space input. ### Detection point -The check happens at the very start of `Processor::process_request`, before any -packet parsing or handler invocation. If `client_socket_addr.port() == 0`, the -request is **discarded immediately**: +Detection happens at two layers: + +1. **Launcher loop (production path)**: the check runs in + `Launcher::run_udp_server_main`, right after the `UdpRequestReceived` event is + emitted and **before** a processing task is spawned and pushed into the + active-requests buffer. This means port-0 requests never consume a task slot + and can never evict legitimate in-flight requests under a port-0 flood. This + mirrors the existing banned-IP check (`check → emit event → continue`). + +2. **`Processor::process_request` (defense-in-depth)**: the same check is kept at + the very start of `process_request`, before any packet parsing or handler + invocation, protecting any other caller of the processor: ```rust pub async fn process_request(self, request: RawRequest) { @@ -97,6 +106,9 @@ pub async fn process_request(self, request: RawRequest) { } ``` +In production only the launcher-level check fires (the processor is never invoked +with a port-0 request), so each discarded request is counted exactly once. + ### Logging **No per-request `WARN` log.** The existing `WARN` log is removed (it came from the diff --git a/packages/udp-server/src/server/launcher.rs b/packages/udp-server/src/server/launcher.rs index 0f741cca9..b29445ae7 100644 --- a/packages/udp-server/src/server/launcher.rs +++ b/packages/udp-server/src/server/launcher.rs @@ -189,6 +189,27 @@ impl Launcher { .await; } + // Discard requests from clients with source port 0 before + // spawning a processing task. Responses to port 0 are rejected + // by the OS with EINVAL, so processing them wastes resources + // and — worse — pushing them into the active-requests buffer + // could evict legitimate in-flight requests under a port-0 + // flood. See also the defensive guard in + // `Processor::process_request`. + if client_socket_addr.port() == 0 { + tracing::trace!(target: UDP_TRACKER_LOG_TARGET, local_addr, %client_socket_addr, "Udp::run_udp_server::loop continue: (discarded: client source port is 0)"); + + if let Some(udp_server_stats_event_sender) = udp_tracker_server_container.stats_event_sender.as_deref() { + udp_server_stats_event_sender + .send(Event::UdpRequestDiscarded { + context: ConnectionContext::new(client_socket_addr, server_service_binding.clone()), + }) + .await; + } + + continue; + } + if udp_tracker_core_container.ban_service.read().await.is_banned(&req.from.ip()) { tracing::debug!(target: UDP_TRACKER_LOG_TARGET, local_addr, "Udp::run_udp_server::loop continue: (banned ip)"); diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index 624e67b3b..05fbdffdc 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -55,6 +55,11 @@ impl Processor { // We discard such requests immediately and record them in statistics so // operators can detect scanner activity or misconfigured clients without // filling the log with noise. + // + // In production the launcher loop already discards port-0 requests + // before spawning a processing task (so they never enter the + // active-requests buffer); this guard is kept as defense-in-depth for + // any other caller of `process_request`. if client_socket_addr.port() == 0 { tracing::trace!(%client_socket_addr, "discarding request: client source port is 0"); From 3023af3bbffeeac23cafbdf852593b4429489a91 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 08:54:45 +0100 Subject: [PATCH 13/18] docs(pr-review): add thread #13 to PR-2017 Copilot suggestions table Thread #13 (PRRT_kwDOGp2yqc6S0_RT) suggested discarding port-0 requests in the launcher loop before spawning/pushing into active_requests. Applied in b4fb60ce, replied and resolved. --- .../pr-reviews/pr-2017-copilot-suggestions.md | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md index e47558292..6d442921d 100644 --- a/docs/pr-reviews/pr-2017-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -40,23 +40,25 @@ Status legend: - 2026-07-21: New thread (PRRT_kwDOGp2yqc6StQu4, thread #10) found on re-check after push. Applied fix and resolved. - 2026-07-22: New thread (PRRT_kwDOGp2yqc6SuPys, thread #11) found: flagged TBD reply URL for thread #10. Posted reply and resolved. - 2026-07-22: New thread (PRRT_kwDOGp2yqc6S0eYH, thread #12) found: processing log said "All 9 threads resolved" while table had 10 entries. Reworded log entry to say "initial batch". Applied fix and resolved. +- 2026-07-22: New thread (PRRT_kwDOGp2yqc6S0_RT, thread #13) found: port-0 guard ran only inside the processor, after spawn/push into active_requests. Moved primary discard to the launcher loop (before spawning); processor guard kept as defense-in-depth. Fixed in b4fb60ce and resolved. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------ | -------- | --------------------------------------------------------------------------- | ------ | ------------ | -| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | -| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | -| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | -| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | -| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | -| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | -| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | -| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | -| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | -| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | -| 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | -| 12 | PRRT_kwDOGp2yqc6S0eYH | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3627977934 | Processing log says "All 9 threads resolved" but table lists 10; reword to "initial batch" | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628134459 | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | -------- | --------------------------------------------------------------------------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | +| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | +| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | +| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | +| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | +| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | +| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | +| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | +| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | +| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | +| 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | +| 12 | PRRT_kwDOGp2yqc6S0eYH | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3627977934 | Processing log says "All 9 threads resolved" but table lists 10; reword to "initial batch" | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628134459 | DONE | RESOLVED | +| 13 | PRRT_kwDOGp2yqc6S0_RT | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628171246 | Port-0 guard runs after spawn/push into active_requests; flood can evict legit requests; discard in launcher | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628457230 | DONE | RESOLVED | ## Notes From b362d26c52972a995364634e4daec69a17d89a79 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 08:59:30 +0100 Subject: [PATCH 14/18] chore(cspell): alphabetize project dictionary --- project-words.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/project-words.txt b/project-words.txt index dff6e29f8..64b0ae2fb 100644 --- a/project-words.txt +++ b/project-words.txt @@ -88,7 +88,6 @@ cyclomatic dashmap datagram datagrams -dport datetime dbip dbname @@ -109,6 +108,7 @@ dockerhub doctest downloadedi dpkg +dport dtolnay dylib EADDRINUSE @@ -259,14 +259,14 @@ newtypes nextest nghttp ngtcp +nmap nocapture nologin -nmap -nping nonblocking nonroot Norberg notnull +nping nquery numwant nvCFlJCq7fz7Qx6KoKTDiMZvns8l5Kw7 From c128a23e320538e65e0a5277fb17f39ae91c7d6b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 09:12:26 +0100 Subject: [PATCH 15/18] docs(issues): add draft spec to simplify UDP server main loop The port-0 discard added in this PR increased the complexity of Launcher::run_udp_server_main, which now mixes setup, receive/error handling, a per-request policy pipeline, and four copies of the stats-event emission boilerplate. It also recreates ServiceBinding on every loop iteration (a per-request allocation with no benefit). The draft spec plans a follow-up refactor constrained to static dispatch and zero new per-request allocations, with benchmark verification, to be implemented in a separate PR. --- .../drafts/simplify-udp-server-main-loop.md | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 docs/issues/drafts/simplify-udp-server-main-loop.md diff --git a/docs/issues/drafts/simplify-udp-server-main-loop.md b/docs/issues/drafts/simplify-udp-server-main-loop.md new file mode 100644 index 000000000..16684fad1 --- /dev/null +++ b/docs/issues/drafts/simplify-udp-server-main-loop.md @@ -0,0 +1,200 @@ +--- +doc-type: issue +issue-type: enhancement +status: draft +priority: p2 +github-issue: null +spec-path: docs/issues/drafts/simplify-udp-server-main-loop.md +branch: "{issue-number}-simplify-udp-server-main-loop" +related-pr: null +last-updated-utc: 2026-07-22 00:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/src/server/processor.rs + - packages/udp-server/src/server/request_buffer.rs +--- + + + +# Issue #[To be assigned] - Simplify the UDP server main request loop + +## Goal + +Reduce the complexity of `Launcher::run_udp_server_main` in +`packages/udp-server/src/server/launcher.rs` without degrading the performance of +the UDP request hot path, and remove a per-request allocation that serves no +purpose. + +## Background + +`run_udp_server_main` is the core request loop of the UDP tracker server. It has +grown to ~120 lines mixing several concerns: + +1. **Setup**: active-requests buffer, server service binding, ban-cleaner task spawn. +2. **Receive loop**: `receiver.next().await`, I/O error classification + (`Interrupted` → return, other → break, `None` → break). +3. **Per-request policy pipeline**: emit `UdpRequestReceived` → discard port-0 + requests → discard banned IPs → spawn `Processor::process_request` → + `force_push` into `ActiveRequests` → emit `UdpRequestAborted` on eviction. +4. **Event-emission boilerplate**: the pattern + `if let Some(sender) = container.stats_event_sender.as_deref() { sender.send(Event::X { ... }).await }` + is repeated four times (`UdpRequestReceived`, `UdpRequestDiscarded`, + `UdpRequestBanned`, `UdpRequestAborted`). + +There is also a genuine hot-path inefficiency: `server_service_binding` is +recreated with `ServiceBinding::new(...).expect(...)` **on every loop iteration**, +even though an identical value is already constructed once before the loop. The +per-iteration copy only exists so the last event-emission arm can consume it by +value. This is a per-request allocation + validation with no benefit. + +### Performance constraints (why this needs care) + +This function is a critical hot path: the UDP server handles thousands of +requests per second. Per-request costs today are dominated by the `recvfrom` +syscall (~1–2 µs), `tokio::task::spawn` (allocation + ~100s of ns), and event +channel sends. A statically-dispatched function call is ~1 ns and is typically +inlined to zero cost. Therefore: + +- Extracting private `async fn`s with **static dispatch** is free: the compiler + merges them into the same state machine and expands them inline. +- What must **not** be introduced: dynamic dispatch (`Box`, trait + objects), extra `Arc` clones, per-request heap allocations, or additional + channel hops. +- Hoisting the per-iteration `ServiceBinding::new` **improves** performance. + +### Test coverage caveat + +`run_udp_server_main` has no unit tests; it is exercised only by integration +tests (`tests/integration.rs`, `packages/udp-server` contract tests) and E2E +suites. The refactoring must be mechanical and reviewed carefully against the +existing behavior, and should be validated with the full integration suite plus +the UDP load-test benchmarks where practical. + +## Scope + +### In Scope + +- Hoist `server_service_binding` creation out of the request loop (create once, + clone only where an event is actually emitted). +- Extract the ban-cleaner task spawn into a private helper (setup noise). +- Extract the per-request policy pipeline into a private `handle_request` + helper, keeping the receive/error-classification concern in the main loop. +- Introduce a small private context struct (e.g. `RequestDispatchContext`) + holding the per-server loop state built once before the loop: + containers, service binding, socket, `local_addr`, `cookie_lifetime`. +- Collapse the 4× event-emission boilerplate into a single + `send_event(&self, event: Event)` helper on the context struct. +- Verify no performance regression (see Verification Plan). + +### Out of Scope + +- Any change to request-processing semantics (event order, discard/ban + behavior, force-push/eviction policy). +- Restructuring `Processor`, `ActiveRequests`, or the stats event system. +- Middleware/trait-based request pipeline abstractions (speculative generality, + dynamic-dispatch risk). +- Changes to `run_with_graceful_shutdown`. +- Adding unit tests for the loop itself (would require raw-socket tooling; the + existing integration/E2E coverage remains the safety net). + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | +| T1 | TODO | Hoist per-iteration `ServiceBinding` recreation | `ServiceBinding::new` called once before the loop; per-request allocation removed; behavior identical | +| T2 | TODO | Extract ban-cleaner spawn helper | `spawn_ban_cleaner(...)` private fn; `run_udp_server_main` setup section shrinks | +| T3 | TODO | Introduce `RequestDispatchContext` + `send_event` | Private struct built once; 4× event boilerplate collapsed; static dispatch only; no new `Arc` clones per req | +| T4 | TODO | Extract `handle_request` policy pipeline | Main loop reduced to receive + error classification + `handle_request` call; diff reviewed line-by-line | +| T5 | TODO | Run full test suite + benchmarks | All unit/integration/E2E tests pass; UDP benchmark comparison shows no regression | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-07-22 00:00 UTC - Copilot - Draft spec created from design discussion in PR #2017 (port-0 discard work highlighted the loop's complexity) - https://github.com/torrust/torrust-tracker/pull/2017 + +## Acceptance Criteria + +- [ ] AC1: `run_udp_server_main` contains only setup, the receive loop, and I/O + error classification; the per-request policy pipeline lives in a private + helper. +- [ ] AC2: `ServiceBinding` is constructed once per server (not once per + request); no new per-request heap allocations or `Arc` clones are + introduced relative to the current code. +- [ ] AC3: Event-emission boilerplate is expressed once (single `send_event` + helper); event order and payloads are unchanged for all four events. +- [ ] AC4: No dynamic dispatch (`dyn`) is introduced anywhere in the request + hot path. +- [ ] AC5: UDP benchmark comparison (before/after) shows no measurable + throughput/latency regression. +- [ ] `linter all` exits with code `0` +- [ ] Relevant tests pass +- [ ] Manual verification scenarios are executed and documented (status + evidence) +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior +- [ ] Documentation is updated when behavior/workflow changes + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --package torrust-tracker-udp-server` +- `cargo test --test integration` +- Pre-push checks + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ------------------------------------- | -------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | ------ | -------- | +| M1 | UDP benchmark before/after comparison | Run the UDP load-test benchmarks (see `docs/benchmarking.md`) on `develop` and on the branch | No measurable throughput/latency regression | TODO | | +| M2 | Stats events still emitted correctly | Start tracker, send normal / port-0 / banned-IP traffic, check REST API stats counters | `received`, `discarded`, `banned`, `aborted` counters unchanged | TODO | | +| M3 | Graceful shutdown still works | Start tracker, send SIGINT while under light load | Clean shutdown, no panics, halt log lines present | TODO | | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | -------- | +| AC1 | TODO | | +| AC2 | TODO | | +| AC3 | TODO | | +| AC4 | TODO | | +| AC5 | TODO | | + +## Risks and Trade-offs + +- **Hot-path regression risk**: mitigated by restricting the refactor to static + dispatch and zero new allocations, plus benchmark comparison (M1/AC5). +- **Behavioral drift in a function with no unit tests**: mitigated by a + mechanical, step-per-commit refactor (T1–T4 as separate commits), careful + diff review, and the full integration/E2E suite. +- **`#[instrument]` spans**: extracting helpers may change tracing span + structure; keep `#[instrument]` placement equivalent or deliberately document + the change. + +## References + +- Related PRs: [#2017](https://github.com/torrust/torrust-tracker/pull/2017) + (port-0 discard work that surfaced this complexity) +- Related file: `packages/udp-server/src/server/launcher.rs` + (`Launcher::run_udp_server_main`) +- Benchmarking guide: `docs/benchmarking.md` From 27b9cd40d3a0f2be8c6cbda9f5e0a072efbc134a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 09:31:37 +0100 Subject: [PATCH 16/18] test(udp-server): use valid connect payload in port-0 discard tests Copilot review threads on PR #2017 pointed out that the processor port-0 tests used an empty payload, so they could not detect a regression where the discard guard moved after parsing/handler work. - Build a valid UDP connect request payload in the test helper. - Assert udp4_connect_requests_accepted_total() == 0 so the tests fail if the connect handler ever runs for a port-0 request. --- packages/udp-server/src/server/processor.rs | 41 +++++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index 05fbdffdc..ea5714368 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -174,6 +174,7 @@ mod tests { use tokio_util::sync::CancellationToken; use torrust_tracker_test_helpers::configuration; + use torrust_tracker_udp_protocol::{ConnectRequest, Request, TransactionId}; use crate::RawRequest; use crate::server::bound_socket::BoundSocket; @@ -185,11 +186,23 @@ mod tests { // Test helpers // ----------------------------------------------------------------------- - fn request_from(addr: SocketAddr) -> RawRequest { - RawRequest { - payload: vec![], - from: addr, - } + /// Builds a raw request carrying a valid UDP connect payload. + /// + /// The port-0 tests use a parsable payload on purpose: if the discard + /// guard regressed (e.g. it was moved after parsing or handler + /// invocation), the connect handler would run and increment the + /// accepted-connect counter, so the tests would catch it. + fn connect_request_from(addr: SocketAddr) -> RawRequest { + let connect_request = Request::from(ConnectRequest { + transaction_id: TransactionId(0i32.into()), + }); + + let mut payload = Vec::new(); + connect_request + .write_bytes(&mut payload) + .expect("a valid connect request should serialize"); + + RawRequest { payload, from: addr } } /// Creates an ephemeral tracker environment, wires up the stats event @@ -256,7 +269,7 @@ mod tests { let client_with_port_0 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); // Act - processor.process_request(request_from(client_with_port_0)).await; + processor.process_request(connect_request_from(client_with_port_0)).await; // Sync: wait until the discard event is processed so the stats // are settled before we assert on the response counters. wait_for_discarded_count(&container, 1).await; @@ -273,6 +286,13 @@ mod tests { 0, "no IPv6 response should be sent to port 0" ); + // Assert: the request was discarded before any handler work, so the + // (valid) connect payload must never reach the connect handler. + assert_eq!( + stats.udp4_connect_requests_accepted_total(), + 0, + "the connect handler should never run for port-0 requests" + ); cancellation_token.cancel(); } @@ -290,7 +310,7 @@ mod tests { let client_with_port_0 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); // Act - processor.process_request(request_from(client_with_port_0)).await; + processor.process_request(connect_request_from(client_with_port_0)).await; // Assert: the discard event was emitted and the counter reflects it. wait_for_discarded_count(&container, 1).await; @@ -300,6 +320,13 @@ mod tests { 1, "expected exactly 1 discarded request" ); + // Assert: the request was discarded before any handler work, so the + // (valid) connect payload must never reach the connect handler. + assert_eq!( + stats.udp4_connect_requests_accepted_total(), + 0, + "the connect handler should never run for port-0 requests" + ); cancellation_token.cancel(); } From 2584c0ffad9cb473b6d52dbbb1ffe4c4e2b401ca Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 09:36:06 +0100 Subject: [PATCH 17/18] docs(pr-review): add threads #14-#16 to PR-2017 Copilot suggestions table --- .../pr-reviews/pr-2017-copilot-suggestions.md | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/docs/pr-reviews/pr-2017-copilot-suggestions.md b/docs/pr-reviews/pr-2017-copilot-suggestions.md index 6d442921d..727990f40 100644 --- a/docs/pr-reviews/pr-2017-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2017-copilot-suggestions.md @@ -41,24 +41,28 @@ Status legend: - 2026-07-22: New thread (PRRT_kwDOGp2yqc6SuPys, thread #11) found: flagged TBD reply URL for thread #10. Posted reply and resolved. - 2026-07-22: New thread (PRRT_kwDOGp2yqc6S0eYH, thread #12) found: processing log said "All 9 threads resolved" while table had 10 entries. Reworded log entry to say "initial batch". Applied fix and resolved. - 2026-07-22: New thread (PRRT_kwDOGp2yqc6S0_RT, thread #13) found: port-0 guard ran only inside the processor, after spawn/push into active_requests. Moved primary discard to the launcher loop (before spawning); processor guard kept as defense-in-depth. Fixed in b4fb60ce and resolved. +- 2026-07-22: Three new threads found on re-check after push. Thread #14 (word ordering) already fixed by the full re-sort in b362d26c; replied no-action and resolved. Threads #15 and #16 (port-0 processor tests: empty payload and missing accepted-connect assertion) fixed together in 27b9cd40 and resolved. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | -------- | --------------------------------------------------------------------------- | ------ | ------------ | -| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | -| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | -| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | -| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | -| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | -| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | -| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | -| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | -| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | -| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | -| 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | -| 12 | PRRT_kwDOGp2yqc6S0eYH | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3627977934 | Processing log says "All 9 threads resolved" but table lists 10; reword to "initial batch" | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628134459 | DONE | RESOLVED | -| 13 | PRRT_kwDOGp2yqc6S0_RT | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628171246 | Port-0 guard runs after spawn/push into active_requests; flood can evict legit requests; discard in launcher | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628457230 | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | --------- | --------------------------------------------------------------------------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6Sq55a | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391767 | `dport` placed before `datagram`; breaks alphabetical order | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624598630 | DONE | RESOLVED | +| 2 | PRRT_kwDOGp2yqc6Sq55o | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391788 | `HDRINCL` placed after `Hydranode`; should be after `hasher` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624605903 | DONE | RESOLVED | +| 3 | PRRT_kwDOGp2yqc6Sq56B | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391826 | `middlewares` before `middlebox`; b < w | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624636696 | DONE | RESOLVED | +| 4 | PRRT_kwDOGp2yqc6Sq56P | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391850 | `sendto` before `savepath`; should be after `Seedable` | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624638336 | DONE | RESOLVED | +| 5 | PRRT_kwDOGp2yqc6Sq56p | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391885 | Fixed `sleep(50ms)` can be flaky; use bounded wait | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624647118 | DONE | RESOLVED | +| 6 | PRRT_kwDOGp2yqc6Sq561 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624391900 | Assertion message misleading; received counter always 0 in unit test (launcher bypassed) | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624659670 | DONE | RESOLVED | +| 7 | PRRT_kwDOGp2yqc6SrCCM | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624438710 | `recvfrom` before `recognised`; outdated thread but issue persisted | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624698072 | DONE | RESOLVED | +| 8 | PRRT_kwDOGp2yqc6Srh5b | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629672 | `nmap`/`nping` before `new*` words; e < m < p | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624711012 | DONE | RESOLVED | +| 9 | PRRT_kwDOGp2yqc6Srh5u | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624629708 | Doc comment says port 0 "invalid"; RFC 768 permits it; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3624712519 | DONE | RESOLVED | +| 10 | PRRT_kwDOGp2yqc6StQu4 | docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625275177 | "invalid socket address" in Current Behaviour section; RFC 768 permits port 0; real issue is OS EINVAL | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628033936 | DONE | RESOLVED | +| 11 | PRRT_kwDOGp2yqc6SuPys | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3625638747 | Thread #10 marked DONE/RESOLVED but Reply URL left as TBD | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628037362 | DONE | RESOLVED | +| 12 | PRRT_kwDOGp2yqc6S0eYH | docs/pr-reviews/pr-2017-copilot-suggestions.md | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3627977934 | Processing log says "All 9 threads resolved" but table lists 10; reword to "initial batch" | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628134459 | DONE | RESOLVED | +| 13 | PRRT_kwDOGp2yqc6S0_RT | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628171246 | Port-0 guard runs after spawn/push into active_requests; flood can evict legit requests; discard in launcher | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628457230 | DONE | RESOLVED | +| 14 | PRRT_kwDOGp2yqc6S15Op | project-words.txt | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628506537 | `n*` entries out of order (`nmap`, `nping`); already fixed by full re-sort in b362d26c; thread outdated | no-action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628733576 | DONE | RESOLVED | +| 15 | PRRT_kwDOGp2yqc6S2URd | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628661730 | Port-0 tests used empty payload; use valid connect payload so guard regression is detectable | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628735417 | DONE | RESOLVED | +| 16 | PRRT_kwDOGp2yqc6S2UR8 | packages/udp-server/src/server/processor.rs | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628661767 | Assert `udp4_connect_requests_accepted_total() == 0` so tests guard against handler work for port-0 | action | https://github.com/torrust/torrust-tracker/pull/2017#discussion_r3628746238 | DONE | RESOLVED | ## Notes From 99858e234b5a80f514206ce91bfd1bae4d416082 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 22 Jul 2026 09:45:14 +0100 Subject: [PATCH 18/18] docs(skills): define semantic issue-link markers --- .../dev/planning/write-markdown-docs/SKILL.md | 6 +++ docs/skills/semantic-skill-link-convention.md | 47 ++++++++++++++++--- packages/udp-server/src/server/launcher.rs | 1 + packages/udp-server/src/server/processor.rs | 1 + .../udp-server/src/server/request_buffer.rs | 1 + 5 files changed, 49 insertions(+), 7 deletions(-) diff --git a/.github/skills/dev/planning/write-markdown-docs/SKILL.md b/.github/skills/dev/planning/write-markdown-docs/SKILL.md index 181e929fd..b03d1bfa0 100644 --- a/.github/skills/dev/planning/write-markdown-docs/SKILL.md +++ b/.github/skills/dev/planning/write-markdown-docs/SKILL.md @@ -72,6 +72,12 @@ Follow the frontmatter convention defined in which specifies the required fields for each document type and the shape of `semantic-links` entries. +When a draft issue spec identifies source artifacts it will change, add an +`issue-spec: ` marker to those artifacts when the link +is high-signal. Once the GitHub issue is created, replace the draft-path marker +with `issue: #`; do not keep paths that will become stale when the spec +moves from `drafts/` to `open/` or `closed/`. + ## Repo Markdown vs. GitHub Markdown The `.markdownlint.json` configuration at the repository root applies **only to `.md` files diff --git a/docs/skills/semantic-skill-link-convention.md b/docs/skills/semantic-skill-link-convention.md index ef962956f..13abd38b3 100644 --- a/docs/skills/semantic-skill-link-convention.md +++ b/docs/skills/semantic-skill-link-convention.md @@ -21,12 +21,33 @@ The repository keeps a small catalog of marker definitions. Current markers: -| Marker | Value | Meaning | -| ------------ | -------------- | -------------------------------------------------------------------------------------- | -| `skill-link` | `` | This artifact affects the linked skill and should trigger a skill review when changed. | +| Marker | Value | Meaning | +| ------------ | ---------------------- | -------------------------------------------------------------------------------------- | +| `skill-link` | `` | This artifact affects the linked skill and should trigger a skill review when changed. | +| `issue-spec` | `` | This artifact is affected by a draft issue specification at the given temporary path. | +| `issue` | `#` | This artifact is affected by the GitHub issue with the given number. | Add new markers only when there is a concrete recurring maintenance problem that the current marker set cannot represent. +### Issue-spec lifecycle + +Use `issue-spec` only while an issue specification is still a draft. The value must be +the repository-relative path to the draft spec: + +```text +issue-spec: docs/issues/drafts/simplify-udp-server-main-loop.md +``` + +When the draft becomes a GitHub issue, replace every corresponding `issue-spec` +marker with the stable issue-number marker: + +```text +issue: #1234 +``` + +Do not retain the draft file path after the issue is created: issue specs move from +`drafts/` to `open/` and later to `closed/`, while the issue number remains stable. + ## Marker Format Use this marker in comments or documentation text close to behavior-defining lines: @@ -170,21 +191,33 @@ Use language-appropriate syntax: - TOML: `# skill-link: ` - Markdown: `` +Use the same language-appropriate comment syntax for issue references: + +- Rust: `// issue-spec: docs/issues/drafts/.md` or `// issue: #` +- TOML: `# issue-spec: docs/issues/drafts/.md` or `# issue: #` +- Markdown: `` or `` + For Markdown files with frontmatter `semantic-links.skill-links`, top-of-file inline markers are redundant and need not be added. Inline markers placed near specific workflow-defining sections within the body remain useful for navigation but are not required when frontmatter links are present. -Place the marker near: +Place a `skill-link`, `issue-spec`, or `issue` marker near: - constants that encode default behavior, - configuration blocks consumed by the workflow, - documentation sections that define the operational procedure. +For issue references in source code, prefer the declaration of the function, type, +or module whose behavior the issue plans to change. Keep these links high-signal: +do not add a marker merely because a file is mentioned incidentally in an issue. + ## Maintenance Workflow -1. Add or update `skill-link` markers in touched artifacts. -2. Update the skill instructions if semantics changed. -3. Validate links and markers. +1. Add or update `skill-link`, `issue-spec`, or `issue` markers in touched artifacts. +2. When moving a draft spec to an issue, replace all of its `issue-spec` markers + with `issue: #` markers. +3. Update the skill instructions if semantics changed. +4. Validate links and markers. ## Ontology-Lite Categories diff --git a/packages/udp-server/src/server/launcher.rs b/packages/udp-server/src/server/launcher.rs index b29445ae7..02b9d3b44 100644 --- a/packages/udp-server/src/server/launcher.rs +++ b/packages/udp-server/src/server/launcher.rs @@ -126,6 +126,7 @@ impl Launcher { ServiceHealthCheckJob::new(service_binding.clone(), info, TYPE_STRING.to_string(), job) } + // issue-spec: docs/issues/drafts/simplify-udp-server-main-loop.md #[instrument(skip(receiver, udp_tracker_core_container, udp_tracker_server_container))] async fn run_udp_server_main( mut receiver: Receiver, diff --git a/packages/udp-server/src/server/processor.rs b/packages/udp-server/src/server/processor.rs index ea5714368..478fedbf7 100644 --- a/packages/udp-server/src/server/processor.rs +++ b/packages/udp-server/src/server/processor.rs @@ -45,6 +45,7 @@ impl Processor { } } + // issue-spec: docs/issues/drafts/simplify-udp-server-main-loop.md #[instrument(skip(self, request))] pub async fn process_request(self, request: RawRequest) { let client_socket_addr = request.from; diff --git a/packages/udp-server/src/server/request_buffer.rs b/packages/udp-server/src/server/request_buffer.rs index 85a374b36..fa2861987 100644 --- a/packages/udp-server/src/server/request_buffer.rs +++ b/packages/udp-server/src/server/request_buffer.rs @@ -3,6 +3,7 @@ use ringbuf::traits::{Consumer, Observer, Producer}; use tokio::task::AbortHandle; use torrust_tracker_udp_core::UDP_TRACKER_LOG_TARGET; +// issue-spec: docs/issues/drafts/simplify-udp-server-main-loop.md /// A ring buffer for managing active UDP request abort handles. /// /// The `ActiveRequests` struct maintains a fixed-size ring buffer of abort