test(udp-server): [#2149] add focused UDP server tests

[josecelano]

Summary

Completes Issue #2149's focused UDP-server package-test review. The branch adds or refactors deterministic package-local tests for active-request capacity/eviction/drop cleanup, UDP event and parse-error classification, socket metadata, packet dispatch, launcher admission, container event publication, received-datagram adaptation, banning/error/response-metric behavior, server-state mappings, and the portable processor port-zero guard. It records completed file-local plans, separate aggregate/global, unit-only, and integration-only coverage evidence, a bounded mutation assessment, a real local tracker/client UDP announce, and final acceptance/retrospective evidence.

Scope

• packages/udp-server focused unit tests and selected real-loopback contracts
• Issue-local coverage, mutation, manual-verification, and retrospective evidence
• Package-local request-buffer eviction-policy ADR and test-design guidance
• No production behavior changes, raw-socket port-zero test, or shutdown/lifecycle redesign

Final Evidence

• Aggregate/global coverage: 5,560 / 5,682 lines (97.85%)
• Unit-only coverage: 5,465 / 5,682 lines (96.18%)
• Integration-only coverage: 1,118 / 1,469 lines (76.11%)
• Bounded Processor::process_request mutation sample caught the port-zero guard inversion; the only other mutant was unviable
• Built local tracker accepted a unified client UDP announce at 127.0.0.1:16969 and returned a valid IPv4 announce response

Validation

• cargo test --tests --benches --examples --workspace --all-targets --all-features
• cargo test -p torrust-tracker-udp-server — 170 unit tests, 11 integration tests, and 1 documentation test passed
• cargo test -p torrust-tracker-udp-server --test integration
• linter all
• TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh
• Independent final acceptance review passed

Closes #2149

[codecov]

Codecov Report

❌ Patch coverage is 99.86559% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.50%. Comparing base (f3eb9aa) to head (40b1a6f).

Files with missing lines	Patch %	Lines
packages/udp-server/src/handlers/error.rs	97.95%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2174      +/-   ##
===========================================
+ Coverage    85.09%   85.50%   +0.40%     
===========================================
  Files          353      353              
  Lines        30626    31240     +614     
  Branches     30626    31240     +614     
===========================================
+ Hits         26062    26712     +650     
+ Misses        4189     4150      -39     
- Partials       375      378       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds deterministic, focused test coverage and supporting documentation for the udp-server package as part of #2149, including clarified request-buffer eviction policy and improved test-design guidance.

Changes:

• Added unit tests for request-buffer capacity/eviction/drop cleanup, bound-socket metadata invariants, handler parse-failure routing, and event/error classification adapters.
• Documented the request-buffer eviction policy via a package-local ADR and expanded issue-local coverage/performance evidence.
• Updated repository test-writing/review guidance to require prose-first AAA verification and clearer test-level coverage attribution.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
packages/udp-server/src/server/request_buffer.rs	Clarifies eviction-policy docs and adds deterministic unit tests for capacity, eviction, and drop cleanup.
packages/udp-server/src/server/bound_socket.rs	Adds unit tests for port-zero binding and URL/service-binding consistency.
packages/udp-server/src/handlers/mod.rs	Adds a focused dispatcher test asserting parse-error routing keeps transaction_id and returns no request kind.
packages/udp-server/src/event.rs	Adds unit tests for stable ErrorKind classification and request-kind metric/display mappings.
packages/udp-server/src/error.rs	Adds unit tests for sendable/unsendable parse-error metadata conversion and invalid-request wrapping.
packages/udp-server/docs/adrs/index.md	Introduces package-local ADR index for udp-server decisions.
packages/udp-server/docs/adrs/README.md	Documents scope/ownership of package-local ADRs.
packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md	Records rationale/contract for bounded oldest-first request eviction.
docs/testing/refactoring-patterns/prose-first-arrange-act-assert-verification.md	Adds a documented test-refactoring pattern used by this work.
docs/testing/refactoring-patterns/README.md	Registers the new prose-first AAA refactoring pattern.
docs/templates/ISSUE.md	Strengthens test-producing workflow requirements (prose-first AAA + explicit design review).
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/request-buffer-tests.md	Records request-buffer test plan, decisions, and validation evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/handler-dispatch-tests.md	Records handler-dispatch test plan and its completion evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/event-tests.md	Records event classification/mapping test plan and completion evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-tests.md	Records parse-error adapter test plan and completion evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/bound-socket-tests.md	Records bound-socket test plan and completion evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md	Introduces two-phase (cleanup then add tests) plan structure and links to plans.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md	Defines baseline performance measurement policy for hot-path changes.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md	Adds test-level coverage attribution policy and interim per-file evidence.
docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md	Updates scope, links, acceptance criteria, and progress log to reflect current increments.
docs/issues/open/1347-overhaul-packages-testing/EPIC.md	Tightens EPIC guidance on unit vs integration coverage attribution and boundary selection.
.vscode/settings.json	Disables format-on-save for Markdown in workspace settings.
.github/skills/dev/testing/write-unit-test/SKILL.md	Makes prose-first AAA verification mandatory and adds checklist item.
.github/skills/dev/task-reviews/review-task/SKILL.md	Requires reviewers to apply a concrete test-design checklist to changed tests.
.github/skills/dev/planning/create-issue/SKILL.md	Updates issue-planning guidance for test increments and prose-first AAA evidence.
.github/agents/task-reviewer.agent.md	Updates task-review agent instructions to enforce prose-first AAA evidence for changed tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[da2ce7]

Reviewed 35294e1 (44 commits on develop 7abc30b). Gated on a dedicated build host at that exact head: cargo clippy --workspace --all-targets --all-features -- -D warnings clean (12.0s), cargo test -p torrust-tracker-udp-server --all-targets --all-features green (33.1s, 154 unit + 11 integration), cargo test --workspace --all-targets --all-features green (166.8s), doc tests green. Determinism is good: 180 repeated runs of the new tests (including 40 request-buffer runs, 20 of them under a 64-way CPU load, and 20 single-threaded) produced 0 failures.

The test design is thoughtful and the increments are readable. Two things need to change before this leaves draft.

Blocker — the branch does not pass the mandatory pre-commit gate. linter all exits 1 at this head, failing only on rustfmt, in two files this PR adds lines to. CI agrees: the Testing workflow's Unit (nightly) job fails at step Run Formatting-Checks (run 34354718522), which cancelled Unit (stable). I reproduced both diffs independently with the PATH-resolved nightly cargo-fmt --all -- --check (0.7s). Bisecting the series with rustfmt against rustfmt.toml (group_imports = "StdExternalCrate", imports_granularity = "Module") shows request_buffer.rs has been unformatted since 5fc5f916 — the 6th of 44 commits — and handlers/mod.rs since 2fecd514; develop itself is clean, so both were introduced here. Several review replies on this PR state that "the mandatory pre-commit gate passed", and the PR description lists linter all under Validation; that has not been true for most of the series. Worth checking why the local gate is reporting green — a cargo fmt that resolves through a $CARGO_HOME/bin shim can silently run a different rustfmt than the one CI uses. Both fixes are one-liners; inline comments below.

Major — the headline eviction test does not pin the policy the new ADR defines. it_should_evict_the_oldest_pending_task_when_the_buffer_is_full asserts that the oldest task was aborted and that the others were aborted after the buffer is dropped — but ActiveRequests::drop aborts everything it still holds, so "aborted during cleanup" is satisfied whether force_push evicted one task or fifty. I verified this rather than assuming it: mutating force_push so the eviction branch continues instead of breaks — i.e. aborting every still-active handle, destroying the oldest-first, exactly-one policy the ADR is being added to protect and killing up to 50 in-flight requests per admission — leaves the entire torrust-tracker-udp-server suite green, all three request-buffer tests included (worktree restored and re-verified clean afterwards). Since this PR adds an ADR specifically to record that this eviction shape is intentional, the accompanying test should be the thing that stops it from silently changing. Suggested fix: capture the retained tasks' liveness immediately after the Act and before dropping the buffer — assert they are still un-aborted at that point — so the test distinguishes "the oldest was evicted" from "the buffer was emptied".

Major — the branch no longer merges into develop. develop has advanced to f6b73e2 (PR #2178 merged). A merge of this head into that tip conflicts in .github/agents/task-reviewer.agent.md; .github/skills/dev/planning/create-issue/SKILL.md and .github/skills/dev/testing/write-unit-test/SKILL.md auto-merge but are now edited by both sides, so the combined result of the two rule sets is worth re-reading after the rebase rather than trusting the auto-merge.

Minor — the PR is wider than its title. Under a test(udp-server): title the diff also changes repository-wide process rules (.github/agents/task-reviewer.agent.md, three .github/skills/** files, docs/templates/ISSUE.md) and an editor setting (.vscode/settings.json). The individual commits are correctly typed (docs(agents), docs(issues), docs(testing)), so this is a PR-level scoping point: those rule changes affect every contributor and are easy to miss when reviewing something announced as UDP server tests. Splitting them out, or at least widening the title, would help. Relatedly, the .vscode change (disabling markdown format-on-save) is committed inside f20d39e3 docs(udp-server): approve request buffer reclamation test, where its subject gives no hint it is there.

Verified good, for the record: no executable production code changes — the request_buffer.rs non-test diff is entirely comments and doc comments, and the rewritten doc comment now matches what the code actually does, which the previous wording did not. Every new test drives real production code (From<RequestParseError> conversions, From<Error> for ErrorKind, UdpRequestKind display/label conversion, handle_packet, BoundSocket::bind/url/service_binding, ActiveRequests::force_push and its Drop); none of them merely exercise a test helper. Ports are all ephemeral (:0), the 1s TASK_COMPLETION_TIMEOUT is a failure bound rather than a sleep, and the probes above found no raciness. All 44 commits are signed with conventional subjects, the package-local ADR follows the existing console/tracker-client/docs/adrs/ layout, and every path its front matter references resolves.

Happy to re-review once the formatting gate is green and the eviction test pins the policy.

[da2ce7]

Re-reviewed f7b355d (46 commits on develop 7abc30b), covering the two commits pushed since my review at 35294e1: 550cf7af test(udp-server): clarify launcher startup cleanup and f7b355d7 docs(udp-server): approve port zero admission test. Gated on a dedicated build host at both heads.

The good news first, and it is real. The launcher refactor in 550cf7af is behaviour-preserving — every Arrange statement moved into UdpLauncherDependencies::new() unchanged and in the same order, the five values reach run_with_graceful_shutdown in the same argument positions, the policy is still Strict, and the visible Act is still the direct launcher call with no wrapper. More importantly the refactored test still has teeth, which I checked rather than assumed: removing running.abort(); drop(running.await); from the production BrokenPipe branch — so the spawned server task keeps holding the socket — makes it_should_release_the_socket_when_the_startup_notification_receiver_is_dropped fail (panic at launcher.rs:403), and the package goes 153 passed / 1 failed. The rename of tx_start/rx_halt to startup_notification_sender/halt_receiver genuinely reads better, and the dropped receiver stays visible in the test body where the causal state belongs. f7b355d7 is internally consistent too: R2 is marked IN_PROGRESS, not done, and no source-port-zero test exists in the tree yet — a grep of packages/udp-server/src finds only the production should_discard_request at launcher.rs:182/:247.

But the blocker is untouched, so this stays at changes-requested. Neither commit touches either rustfmt-failing file. At this head, cargo-fmt --all -- --check still exits 1 (0.7s) with byte-identical diffs in packages/udp-server/src/handlers/mod.rs:268 and packages/udp-server/src/server/request_buffer.rs:141, and linter all still exits 1 (23.7s) with rustfmt as the only failing linter — markdown, lychee, yaml, toml, cspell, clippy and shellcheck all pass. The two inline threads on those lines carry the exact one-line fixes and are still open.

New — a recorded validation result the gate contradicts. 550cf7af adds this row to the R1 plan: | R1 | DONE | `cargo fmt --all -- --check`, `cargo test …`, and `git diff --check` passed. |. That is not true at the commit that records it, nor here. I am flagging it as more than a stale document because the recorded command is itself the likely cause: the space form cargo fmt resolves through a $CARGO_HOME/bin shim before PATH, so it can silently run a different rustfmt than the binary form cargo-fmt that linter all and CI's Run Formatting-Checks both use. My round-1 note guessed at a shim; this row is evidence for it. Try cargo-fmt --all -- --check (with the dash) locally and see whether it disagrees with cargo fmt. Worth resolving before R2, because otherwise the same false green recurs — and f7b355d7 opens R2 by rewriting the row directly beneath this one, 31 minutes after the review, while the plan's own rule is to complete one increment's validation before starting the next. Inline comment below.

The other two round-1 items are also unchanged, and I re-verified both rather than inferring them from the diff:

• Eviction test. request_buffer.rs is byte-identical to 35294e1, but I re-ran the mutation at this head anyway: force_push's finished == 0 branch continue-ing instead of break-ing — aborting every still-active handle rather than only the oldest, discarding up to 50 in-flight requests per admission — still leaves all three request-buffer tests and the whole torrust-tracker-udp-server suite green (154 unit + 11 integration). The mutation the new ADR exists to forbid is still invisible to the test written to protect it. Thread on request_buffer.rs:252 has the suggested fix.
• Mergeability. git merge-tree --write-tree f6b73e29 f7b355d7 still exits 1, conflicting in .github/agents/task-reviewer.agent.md; create-issue/SKILL.md, write-unit-test/SKILL.md and docs/templates/ISSUE.md still auto-merge while edited on both sides, so the combined rule text deserves a read after the rebase.

Everything else gates clean at this head, for the record: clippy --workspace --all-targets --all-features -D warnings (4.2s), cargo test -p torrust-tracker-udp-server --all-targets --all-features (12.7s, 154 unit + 11 integration), doc tests (1.0s), and the full workspace suite (123.5s). The changed launcher test is not flaky: 20 consecutive runs clean, and 20 more clean under a 64-way CPU load.

One thing to check that is not a finding against the change: no CI has run on either new head. gh run list returns nothing for 550cf7af or f7b355d7, and the commit has zero check suites — polled ~13 minutes after the first push and ~5 after the second, where the two previous pushes on this branch started their five workflows within about ten seconds. The newest CI evidence on the branch is still run 34354718522 at 35294e14: Unit (nightly) failed at step Run Formatting-Checks, cancelling Unit (stable) at Run All Linters. A workflow that never starts emits no check runs at all, so the checks page can look clean when nothing ran — worth a glance given this PR's history of a gate believed green while it was red.

Also still standing from round 1, unchanged and unreplied: the // Act and Assert nit at event.rs:344 and the .vscode/settings.json format-on-save minor. The PR description still lists linter all under Validation.

Happy to re-review as soon as the formatting gate is actually green — that one change closes the blocker and the plan's validation record together.

[da2ce7]

Round-3 review at 954d4d2054186272a123a0fbde943aefa124ebc3.

REQUEST_CHANGES — forced by the round-1 Blocker, which is now four commits old and untouched. Everything else in this round is good news: the new port-zero test is the strongest test on this branch.

Standing items

Item	Where	Status	Evidence at this head
Blocker — rustfmt	handlers/mod.rs:273	OPEN	cargo-fmt --all -- --check exits 1, byte-identical diff; file untouched since 35294e14
Blocker — rustfmt	request_buffer.rs:147	OPEN	same run, second diff; file untouched since 35294e14
Major — eviction policy not pinned	request_buffer.rs:252	OPEN	mutation M1 (break -> continue, abort every in-flight handle) re-run at 9d966925: package still 155 passed; 0 failed
Major — conflicts with develop	.github/agents/task-reviewer.agent.md	OPEN	git merge-tree --write-tree f6b73e29 954d4d20 exits 1, all three stages present; GitHub CONFLICTING/DIRTY
Major — false validation record	launcher-tests.md R1 row	OPEN and repeated	see the inline comment on the R2 row below
Nit — // Act and Assert	event.rs:344	OPEN	file untouched since 35294e14
Nit — fixture named for construction	launcher.rs:338	OPEN	UdpLauncherDependencies was restructured in 9d966925 (socket binding moved out, bind_address added) and now serves two tests, without a reply on the thread
Minor — markdown format-on-save	.vscode/settings.json:6	OPEN	file untouched since 35294e14

linter all at this head: rustfmt is the only failing linter (markdown, lychee, yaml, toml, cspell, clippy, shellcheck all pass, 23.6s). Package suite passes: 155 unit + 11 integration. Workspace suite, clippy -D warnings and doc tests pass. No CI has run on the last four heads (550cf7af, f7b355d7, 9d966925, 954d4d20 — checkSuites.totalCount 0); the newest CI on the branch is still the round-1 Testing failure at 35294e14.

9d966925 — the port-zero admission test

This one is right, and I checked it adversarially rather than by reading.

It drives the production path. The Act is a direct Launcher::should_discard_request(...) call — the same private associated function the receive loop calls at launcher.rs:181, with the same argument shape. No wrapper, no reimplementation. The production region of launcher.rs (lines 1-313) is byte-identical to the previous head, so the plan's non-goal of not changing admission behaviour holds.

The assertions are on observable effects, and all four have teeth. Four mutations were applied to the production code in a scratch worktree, each reverted, tree verified clean:

Mutation	Test result
M3a — delete the port-zero branch (if client_socket_addr.port() == 0 -> if false)	caught: assertion failed: should_discard
M3b — keep the discard decision, drop the UdpRequestDiscarded publication	caught: request-discarded event should be published before the test deadline: Elapsed(())
M3d — publish UdpRequestBanned instead of UdpRequestDiscarded	caught: variant mismatch
M3e — publish the discard event carrying a different client address	caught: client_socket_addr: 203.0.113.1:1 vs 203.0.113.1:0

So the test pins the decision, the publication, the exact variant, and the exact context payload — not internal state. The event assertion is genuine end-to-end: UdpTrackerServerContainer builds stats_event_sender from the same EventBus the test subscribes to via event_bus.receiver(), and the receiver is created before the Act. The one-second tokio::time::timeout is the right call and its comment earns its place: M3b fails diagnostically in 1.2s instead of hanging.

No flakes. 20 runs of the new test: 0 failures (2s). 20 runs under 64-way CPU contention: 0 failures (3s). 20 runs of the whole server::launcher::tests module, to catch interference with its neighbour: 0 failures (3s).

The R1 test survived the refactor with its teeth. 9d966925 moved socket binding out of the fixture and into the R1 test body. Mutation M2a (remove running.abort(); drop(running.await); from the production BrokenPipe branch) still fails that test, so the restructuring did not hollow it out.

Naming and placement are right. it_should_discard_a_request_when_its_source_port_is_zero follows the package's it_should_* convention, sample_udp_service_binding follows the established sample_* helper convention (13 such helpers in this package), and the test sits next to the seam it covers.

f7b355d7 and 954d4d20 — plan bookkeeping

954d4d20 moves R3 to IN_PROGRESS and records approval of a banned-IP admission test. That is consistent with the tree: no such test exists yet (launcher.rs has exactly two tests, neither about banning), the checklist leaves "R3 assessment completed" unchecked, and markdown/link/spell checks pass on the file. The R2 bookkeeping is consistent too — R2 DONE, the test exists — except for the validation row, which is the inline Major below.

Two nits on the new test follow. Neither blocks; the blocker is the formatting.

[da2ce7]

Round 5 — the substance of rounds 1-4 is closed; two record-accuracy defects remain

This round is a large and genuine step forward. I re-derived every claim against the tree, the gates and the API rather than reading the consolidated response as evidence, and the code-level findings all hold up.

Reviewed at 2c53718256eb6a784cbb9aca413c34cbe6ee3f6b. The branch was force-pushed twice while this review was in progress; the second push is a pure rebase onto the new develop (f3eb9aa8, carrying the #2177 merge) and changes none of this PR's content — the packages/udp-server tree oid (c9c748e5), the 2149-… issue-directory tree oid (1aa3cee6) and the review-feedback blob (665c0648) are identical across it, and the 119 commits map 1:1 by git patch-id --stable with zero additions, changes or drops. Every gate below was re-run at the new head anyway, because the base moved.

Confirmed closed

The formatting blocker is gone, and the root-cause diagnosis is correct. I reproduced the divergence directly:

cargo +nightly fmt --all -- --check   -> exit 0 (0.8s)
cargo +stable  fmt --all -- --check   -> exit 0 (0.9s), emitting per crate:
  Warning: can't set `imports_granularity = Module`, unstable features are only available in nightly channel.
  Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.

That is exactly the mechanism described: stable rustfmt downgrades both unstable options to warnings and still exits 0, so a stable-toolchain gate cannot see an import-grouping violation. .github/workflows/testing.yaml confirms the enforcing side — the format step (Run Formatting-Checks, cargo fmt --check) is guarded by matrix.run_format, which is true only on the nightly leg of the Unit matrix. The job name in the narrative survives the workflow rewrite. Good diagnosis, and the plans README is the right home for it.

The eviction test now has teeth. I re-applied the mutation rather than taking the reply's word for it. Changing packages/udp-server/src/server/request_buffer.rs:110 from break; to continue; makes it_should_evict_the_oldest_pending_task_when_the_buffer_is_full fail at request_buffer.rs:256 with only the oldest task should be evicted when capacity is exhausted (167 passed; 1 failed, 7.6s); reverting restores 168 passed; 0 failed (7.5s), with the worktree proven clean between the runs. Moving assert_retained_tasks_are_still_active() ahead of the buffer drop is what makes the difference.

Also verified in the tree: the parameterless sample_udp_service_binding() (launcher.rs:400, four call sites updated); the guard-ahead-of-policy comments at both port-zero Acts (launcher.rs:447, launcher.rs:541); the table-form rationale recorded at event-tests.md:110; the .vscode/settings.json comment explaining the Markdown opt-out.

Gates are green. Package-scoped, nightly, on a shared build host: cargo clippy -p torrust-tracker-udp-server --all-targets -- -D warnings exit 0 (3.9s); cargo test -p torrust-tracker-udp-server 168 unit + 11 integration + 1 doc, 0 failures (12.1s).

No AI co-author trailers across all 119 commits; every subject conventional; and no production behaviour change — the pre-#[cfg(test)] regions of processor.rs, launcher.rs and request_buffer.rs are byte-identical to base apart from comments.

Why this is still CHANGES_REQUESTED

Two findings, both in the class these four rounds have been about: a record that says something the tree does not.

1. Major — every commit id cited as an Independent fix commit in pr-2174-review-feedback.md, and in all twenty thread replies, is unreachable from this branch. Details inline.
2. Major — thread PRRT_kwDOGp2yqc6hkqaD was resolved on a reply stating all six bare-named tests were renamed. One was not. Details inline.

Both are small edits. Neither touches the code, which is in good shape.

Thread state

20 threads, 20 resolved, 0 unresolved. Every reply names a fix commit; not one of those commits is reachable from head — see finding 1.

Validation rows — both readings of "25"

• Rows across the plan documents recording a cargo fmt --all -- --check … passed result: 29. 4 carry an in-place **Corrected** marker (launcher-tests.md R1 and R2, error-metric-tests.md R2, handler-error-tests.md R1/R2) — exactly the four the review threads named. The other 25 still read as a plain pass, which matches the round-4 count.
• Rows the correction commit actually rewrote: 4, across three plan files, plus a 13-line blanket note in test-refactor-plans/README.md.

So "25 false validation rows" is the right diagnosis of the scope, but the correction is 4 rows in place plus one blanket note covering the rest. That is a defensible and openly stated choice — README.md:30-41 says so plainly — so I am not blocking on it. One Suggestion inline about making those rows self-describing.

CI

At the previous head every workflow completed green (18 check-runs, no startup_failure, Docker E2E skipped for the known feature-branch trigger gap). At 2c537182 the five workflows are still running: Layer Boundary Bans and the new Documented Clippy Allows have passed, Docker E2E is skipped again, and the rest are in progress. Nothing has failed. The green needs re-reading once the runs finish, particularly the Unit (nightly) formatting step against the new base.

[josecelano]

Consolidated response to review 5200096977 (round 5, @da2ce7)

Both Majors and both advisory items are addressed. All five threads are replied to and resolved. Fixes were pushed as a fast-forward (no rebase), so the cited commits stay reachable.

Finding	Fix (cited by unique commit subject)
Major: audit commit ids unreachable after the two rebases	docs(review): cite fix commits by stable subject — all eight SHAs in pr-2174-review-feedback.md replaced by their unique Conventional Commit subjects, plus a Commit Citation Note recording that the twenty pre-rebase inline replies carry invalidated ids and giving the git log --fixed-strings --grep lookup recipe
Major: sixth bare-named test missed; round-4 thread resolved on a false claim	test(udp-server): rename bare receiver test prefix — it_should_yield_a_raw_request_with_the_received_datagram_and_sender_address, plan row updated. The round-4 reply miscounted by swapping receiver for response-sent; acknowledged in the thread
Suggestion: uncorrected rows not self-describing	docs(udp-server): mark stable-rustfmt validation rows — every affected plan (16 files) now carries a one-line stable-rustfmt pointer directly under its Validation Evidence heading
Nit: RawRequest PartialEq/Eq derive unmentioned	docs(udp-server): record RawRequest derive decision — receiver plan now records it as the issue's one deliberate production-surface change

On the structural point in the first Major: agreed. Citing branch SHAs from inside a rebasable branch is the trap, and it has now been folded into the drafted process-improvement issue for the review workflow — fix commits will be cited by stable subject or reply URL until merge pins the history.

Validation for this batch: cargo +nightly fmt --all -- --check clean, full pre-commit gate (now including the new Documented Clippy Allows step) passed, focused receiver test green, linter all passed.

[da2ce7]

Round 6 — verification of the round-5 fixes

All four round-5 findings are closed, recomputed from the tree rather than read off the replies. The push is a clean fast-forward and every package gate is green, including linter all, which is fully green here for the first time in this pull request's history.

N1 (Major) — rebase-stable citations

CLOSED. docs/pr-review-feedback/pr-2174-review-feedback.md carries no fix-commit SHA. The provenance column and the processing log now cite twelve distinct Conventional Commit subjects, and each one resolves to exactly one commit in f3eb9aa8..40b1a6fd — verified subject by subject with git log --format=%s --fixed-strings --grep=<subject> | grep -cxF <subject>, count 1 for all twelve, no near-miss and no prefix collision:

style(udp-server): fix rustfmt import grouping, docs(udp-server): correct formatting validation records, test(udp-server): assert retained tasks survive eviction, test(udp-server): clarify inert admission test inputs, test(udp-server): apply it_should naming convention, docs(udp-server): record table-form test rationale, chore(vscode): explain markdown format-on-save opt-out, docs(issues): repair archived issue links in EPIC, docs(review): cite fix commits by stable subject, test(udp-server): rename bare receiver test prefix, docs(udp-server): mark stable-rustfmt validation rows, docs(udp-server): record RawRequest derive decision.

The Commit Citation Note at docs/pr-review-feedback/pr-2174-review-feedback.md:31-38 records the invalidation and its cause (two rebases after the replies were written), and its recipe was run verbatim, not just read: git log --oneline --fixed-strings --grep='style(udp-server): fix rustfmt import grouping' returns the single line 8901e686 style(udp-server): fix rustfmt import grouping.

Five hex ids survive in the document, all in the Reviewed commit column of the Reviews table (:44-48). That column is a record of the head each review was taken at, so a historical id is the correct content there, and four of the five (35294e14, f7b355d7, 954d4d20, 598e5f57) are unreachable from the branch exactly as the Citation Note says. 2c537182 is an ancestor of head. Nothing to change.

N2 (Major) — the missed bare test name

CLOSED. packages/udp-server/src/server/receiver.rs:107 now reads async fn it_should_yield_a_raw_request_with_the_received_datagram_and_sender_address(), and the plan's R2/R3 evidence row was updated with it in the same commit. Two independent checks:

• grep -n -E 'fn (it_)?should_' packages/udp-server/src/server/receiver.rs → one match, prefixed; zero bare.
• Added-line scan over the whole range f3eb9aa8..40b1a6fd: 0 added fn should_ lines against 33 added fn it_should_ lines. The convention now holds across every test this pull request introduces, not just the receiver one.

The round-4 miscount is confirmed by the tree. test(udp-server): apply it_should naming convention renamed six tests, the sixth being should_update_the_connect_processing_time_average_for_a_successful_connect_response in the response-sent handler — so the reply's claim of six was accurate in count and wrong in membership, precisely the swap acknowledged in the thread. packages/udp-server/src/statistics/event/handler/response_sent.rs does still hold two bare-named tests at :117 and :149, but those lines are unchanged from develop (f3eb9aa8:…/response_sent.rs:84,116) and the file's diff here is +33/−0. Pre-existing, not this pull request's to carry.

N3 (Suggestion) — self-describing validation rows

CLOSED, and the complement holds. Sixteen plans carry the pointer, and in all sixteen it sits as the first content directly under the ### Validation Evidence heading, ahead of the table it qualifies. Checked by parsing the line after the heading in each file, not by a bare grep:

banning-event-handler, bound-socket, container, contract, error-metric, error, event, handler-dispatch, handler-error, launcher, processor, receiver, request-buffer, response-sent-handler, server-states, statistics-event-dispatch.

The closure test that matters is the other direction: no plan carrying a formatting-validation row lacks the pointer. The only two plans without it, spawner-tests.md and statistics-module-tests.md, contain zero cargo fmt --all -- --check rows — no occurrence of fmt or format at all. The pointer's link target resolves: README.md#formatting-validation-correction-2026-09-14 matches ## Formatting Validation Correction (2026-09-14) at README.md:30.

N4 (Nit) — the RawRequest derive

CLOSED. …/test-refactor-plans/receiver-tests.md:115-118 records all three things: that the derive is the one production-surface change in the issue, that it is deliberate with the reason given (payload bytes plus sender address are meaningful structural value equality callers may rely on, and it enables the whole-value assertion), and that no behavioural code path changed. That matches the tree — packages/udp-server/src/lib.rs:669 is a derive widening and nothing else.

Series integrity

2c537182 is an ancestor of 40b1a6fd; five new commits, no rebase. The 119 earlier commits are byte-identical work: git patch-id --stable over f3eb9aa8..2c537182 and over the first 119 of f3eb9aa8..40b1a6fd gives 119 ids each, 0 only-in-old, 0 only-in-new. Across all 124 commits: zero co-author or AI-attribution trailers, all subjects conventional, zero occurrences of the banned five-character token in added lines, and zero added sleeps, #[ignore], #[allow] or #[expect]. The only non-documentation change in the five new commits is the one-line test rename.

Gates — nightly, package-scoped, minimum priority

gate	exit	wall
cargo +nightly fmt --all -- --check	0	0.7 s
cargo +nightly clippy -p torrust-tracker-udp-server --all-targets -- -D warnings	0	8.5 s
cargo +nightly test -p torrust-tracker-udp-server	0	25.1 s — 168 unit, 11 integration, 1 doc, 0 failures
linter all	0	29.0 s — markdown, lychee, yaml, toml, cspell, clippy, rustfmt, shellcheck all pass

cargo 1.100.0-nightly (7941be6fb 2026-09-11), rustc 1.100.0-nightly (809936eac 2026-09-12). Clippy's only output is the pre-existing package.readme manifest advice in unrelated crates, none of it from this package's code. The spell-check step that could not complete in round 5 for want of its binary now runs and passes in 9.4 s, so linter all is green end to end at this head.

CI

Read six minutes after the push, so it is necessarily partial: 14 check-runs, 3 success (Docs Lint, Documented Clippy Allows, Layer Boundary Bans), 1 skipped (Docker E2E, the known feature-branch trigger defect), 10 in progress. Nothing failed and nothing is startup_failure. The complete run at 2c537182 — the same tree apart from four documentation commits and a test rename — was clean on all 18 check-runs, and the formatting step CI enforces is the one that passes locally above.

Verdict

Approving. Every finding from rounds 1 through 6 is now closed, several of them proved by mechanism rather than by inspection. The test work itself was already sound; what the last two rounds fixed was the record, and the record now resolves.

[da2ce7]

ACK 40b1a6f — round 6 verified: 12 cited commit subjects resolving 1:1 in f3eb9aa..40b1a6f, 0 bare fn should_ in added lines against 33 it_should_, 16/16 plans carrying the stable-rustfmt pointer under Validation Evidence with the two pointer-less plans holding no formatting rows, clean fast-forward over 119 unchanged patch-ids, and nightly gates green (fmt 0.7s, clippy 8.5s, 168+11+1 tests 25.1s, linter all 8/8 29.0s).

[josecelano]

ACK 40b1a6f