diff --git a/.github/agents/task-reviewer.agent.md b/.github/agents/task-reviewer.agent.md index 7a87956b5..77f221cf2 100644 --- a/.github/agents/task-reviewer.agent.md +++ b/.github/agents/task-reviewer.agent.md @@ -38,18 +38,28 @@ pull request is opened. - Acceptance criteria list - Claimed implementation scope 2. Inspect relevant diffs/files and run focused checks as needed. -3. Validate each acceptance criterion explicitly as one of: +3. When changed tests are in scope, apply the `Test Design` checklist from + `.github/skills/dev/task-reviews/review-task/SKILL.md` to every changed test. Report each + violated item as a repository-convention finding with concrete remediation; do not pass the + review while a test fixture is a parameter bag or hides the causal state, production Act, or + independently specified expected result. Require recorded evidence of the mandatory prose-first + Arrange-Act-Assert comparison; do not pass a review when a changed test's code has not been + compared against its temporary prose specification, or when redundant prose remains without an + irreducible-context rationale. Assess helper quality by whether it gives a coherent action, + capability, or state a meaningful name and aligns the caller's abstraction level; do not flag a + helper solely because it has a single caller. +4. Validate each acceptance criterion explicitly as one of: - `PASS` - implemented and verified - `FAIL` - not implemented or incorrect - `PENDING` - partial/unclear or missing evidence -4. If the issue spec contains checklist items, mark only verified `PASS` items as done. -5. Review the completion-review evidence. Require an issue-local +5. If the issue spec contains checklist items, mark only verified `PASS` items as done. +6. Review the completion-review evidence. Require an issue-local `implementation-retrospective.md` when implementation revealed reusable lessons, material design changes, or meaningful deviations from the original plan. Otherwise require a concise issue progress-log entry explaining why no retrospective was needed. -6. Report findings with concrete remediation guidance for all `FAIL` or `PENDING` items. -7. Return an overall status: +7. Report findings with concrete remediation guidance for all `FAIL` or `PENDING` items. +8. Return an overall status: - `REVIEW PASSED` when all required criteria pass and no blocking issues remain. - `REVIEW FAILED` when any required criterion fails or blocking issues remain. diff --git a/.github/skills/dev/planning/create-issue/SKILL.md b/.github/skills/dev/planning/create-issue/SKILL.md index 02e5856ed..67b844273 100644 --- a/.github/skills/dev/planning/create-issue/SKILL.md +++ b/.github/skills/dev/planning/create-issue/SKILL.md @@ -142,16 +142,36 @@ For testing or coverage-focused issue specs, also require: - an issue-local, human-readable coverage-evidence document when coverage is measured; - the exact reproducible coverage command and a statement of what paths and code types it includes; -- aggregate baseline/current values **and** per-file coverage plus prioritized uncovered functions, - regions, or behavior gaps; and +- separate aggregate/global and unit-only baseline/current tables, plus per-file coverage and + prioritized uncovered functions, regions, or behavior gaps. Aggregate/global coverage tracks all + selected test levels; unit-only coverage tracks the primary package-local objective. Do not infer + sufficient unit coverage from aggregate, integration, example, or end-to-end results. Record + integration-only results separately when they inform an ownership decision; and - a policy to retain concise Markdown evidence rather than raw generated JSON, LCOV, or HTML artifacts unless the artifact itself has a documented human-review purpose. -When the plan adds or changes tests, include a progressive test-development loop: make the smallest -behavior-focused increment, review its design and focused validation before the next test-producing -task, and stop for maintainer review after the final increment before final verification, commit, or -pull request. Direct test authors to the `write-unit-test` skill and the test refactoring-pattern -catalog when applicable. +For package-testing work, require a feasible focused unit test to be assessed before accepting +higher-level coverage as sufficient. Integration, example, root, or end-to-end coverage may retain +a distinct contract, but must not be used to decline a package-owned unit test that is deterministic +and readable at the unit boundary. A documented no-unit-test decision must state why the behavior +cannot be protected appropriately by a unit test or why the higher-level boundary is demonstrably +clearer and more maintainable. + +When the plan adds or changes tests, include a progressive test-development loop: use the +`write-unit-test` skill; make the smallest behavior-focused increment; and, after it passes focused +validation, perform and record an explicit design review before maintainer review and commit. The +review must confirm the test exposes the one causal initial-state difference, its fixture owns only +incidental mechanics, and the production Act plus independently specified expected result remain +visible. Make the review enforceable with the mandatory prose-first Arrange-Act-Assert comparison: +write temporary prose for each section, refactor until the code expresses it, remove redundant prose, +and record the result in task evidence or a file-local plan. Complete this review for every +test-producing subtask before starting the next one. Stop for maintainer review after the final +increment before final verification, commit, or pull request. Direct test authors to the test +refactoring-pattern catalog when applicable. Require the test-design review to judge helper +boundaries by meaningful named actions and abstraction-level alignment, not caller count; a +single-use helper is valid when it hides only incidental mechanics. Use the independent Task +Reviewer for the final pre-PR review of the completed issue, not as a mandatory reviewer for every +subtask. During implementation, create an ADR when an important architectural decision emerges, even if the issue draft did not anticipate it. Link the ADR from the diff --git a/.github/skills/dev/task-reviews/review-task/SKILL.md b/.github/skills/dev/task-reviews/review-task/SKILL.md index 526195f6b..442b6c1c5 100644 --- a/.github/skills/dev/task-reviews/review-task/SKILL.md +++ b/.github/skills/dev/task-reviews/review-task/SKILL.md @@ -51,6 +51,29 @@ an issue/task is complete and ready to be pushed. - [ ] Docs updates are present when behavior changed. - [ ] New terms are added to `project-words.txt` when needed. +### Test Design + +When the reviewed changes add or modify tests, inspect each changed test against +`.github/skills/dev/testing/write-unit-test/SKILL.md` and report a finding for every unchecked +item below: + +- [ ] The test's name states one observable behavior and relevant condition. +- [ ] Arrange makes the causal initial-state difference visible. +- [ ] Any builder or scenario fixture is named for that state and owns only incidental mechanics; + it is not a parameter bag mirroring the production call. +- [ ] Every helper names a coherent action, capability, or state and keeps the caller at one + abstraction level. Do not treat a single-use helper as a defect solely because it has one + caller; flag it only when it is vague, hides behavior, or mixes responsibilities. +- [ ] The production Act remains visible in the test body. +- [ ] Expected results are independently specified and assertions remain visible. +- [ ] The test does not duplicate a better-owned protocol, domain, integration, or end-to-end + contract. +- [ ] Execution is deterministic: no uncontrolled I/O, wall-clock dependency, sleep, polling, or + shared mutable state is introduced. +- [ ] The test evidence records a prose-first Arrange-Act-Assert comparison, or the reviewer + records why it was not applicable. The final code expresses the temporary prose; redundant + comments were removed and retained comments provide irreducible context. + ### Spec Hygiene - [ ] Only verified checklist items are marked done. diff --git a/.github/skills/dev/testing/write-unit-test/SKILL.md b/.github/skills/dev/testing/write-unit-test/SKILL.md index c3ae4bf69..2299f49e3 100644 --- a/.github/skills/dev/testing/write-unit-test/SKILL.md +++ b/.github/skills/dev/testing/write-unit-test/SKILL.md @@ -65,6 +65,19 @@ Acceptable reasons to defer or avoid direct unit tests include: If a feature is hard to test, treat that as design feedback first and improve testability when practical. +### Coverage Attribution Is Unit-First + +For package-owned behavior, treat unit-only coverage as the primary measurement and aggregate/global +coverage as a separate broad-progress measurement. An aggregate report can include unit, +integration, example, or end-to-end binaries; it cannot prove that a source seam has adequate unit +protection. Record unit-only and integration-only measurements separately when coverage informs a +test-boundary decision. + +Do not reject a feasible focused unit test because an integration, example, or end-to-end test +already executes the behavior. Decline a unit test only when it cannot protect the behavior at an +appropriate boundary, or when a higher-level contract is demonstrably clearer and more maintainable; +record that rationale in the issue-local evidence. + ### Lifecycle Fixture Design Review When a test fixture manages a child process, asynchronous I/O, network @@ -117,6 +130,65 @@ components, or derive an expected outcome using production code under test. For constraints and example, see [Scenario fixtures for causal initial state](../../../../../docs/testing/refactoring-patterns/scenario-fixtures-for-causal-initial-state.md). +### Name Coherent Actions at One Abstraction Level + +Use a helper when it gives a coherent sequence of setup or transport actions a meaningful name and +keeps the caller at one readable abstraction level. A helper does **not** require multiple callers: +`start_ephemeral_udp_tracker()` can be justified by naming one complete ordinary setup action even +when one contract test currently uses it. + +Judge a helper by semantic value, not reuse count. Keep it when its name expresses a capability or +state relevant to the test and it hides only incidental mechanics. Reject it when it merely moves +code away behind a vague name such as `setup()`, becomes a parameter bag, hides the causal state, +production Act, or expected result, or mixes unrelated responsibilities. See +[Named helpers for abstraction-level alignment](../../../../../docs/testing/refactoring-patterns/named-helpers-for-abstraction-level-alignment.md) +for selection criteria and examples. + +### Anti-Pattern: Duplicated Fixture-Derived Expectations + +Do not extract a second helper that manually reconstructs a representation already derived from a +fixture when that representation is not independently under test. For example, a test that passes a +`ConnectionContext` to production code should not separately hard-code every metric label expected +from that context merely to add one causal label such as `request_kind=connect`. The fixture and +expectation become coupled by hidden duplication: an unrelated fixture change makes the test fail +with stale expected details. + +Instead, derive fixture-owned details from the exact fixture value used by the Act, and specify only +the test's causal input or independently asserted result in the test body. In the metric example, +create `LabelSet::from(connection_context.clone())` and visibly add `request_kind=connect`. Add a +separate focused test when conversion of the fixture into its derived representation is itself the +behavior under test. + +During prose-first review, ask: **“If this fixture changes, should this test fail?”** If no, derive +the incidental expectation from the fixture. If yes, keep the relevant fixture value and its +assertion visibly connected in the test prose; use a scenario or builder if several coordinated +values establish that causal state. + +### Verify Intent with Prose-First AAA + +Before considering any new or materially refactored test ready for maintainer review, make its +intent explicit and verify that the final code communicates it. This is mandatory for every +test-producing increment: + +1. Write temporary normal-prose **Arrange**, **Act**, and **Assert** paragraphs above the test. + State the causal initial state, the production action, and independently specified observable + result; do not describe implementation mechanics without explaining their behavioral purpose. +2. Repeat each paragraph above the corresponding `// Arrange`, `// Act`, or `// Assert` code + section. +3. Compare the code with each paragraph. Refactor names, setup, builders, scenario fixtures, the + visible Act, or assertions until the code itself expresses the paragraph. +4. Remove prose that is redundant once the code communicates the intent. Retain only essential + context that cannot be expressed clearly in code without disproportionate complexity or a + misleading abstraction. +5. Record the completed prose-first comparison in the task evidence or file-local test plan before + maintainer review and commit. + +The temporary prose is the test's specification, not permanent commentary. A parameter bag, an +opaque fixture, a hidden Act, or an assertion derived through production code is evidence that the +code has not yet expressed its specification. See +[Prose-first Arrange-Act-Assert verification](../../../../../docs/testing/refactoring-patterns/prose-first-arrange-act-assert-verification.md) +for a repository example. + ## Phase 1: Basic Unit Test ### Naming Convention @@ -297,6 +369,7 @@ establishes a reusable pattern for future tests. - [ ] Test name uses `it_should_` prefix - [ ] Test follows AAA pattern with comments (`// Arrange`, `// Act`, `// Assert`) +- [ ] Temporary prose-first AAA specification was compared with the code; redundant prose was removed - [ ] No `std::time::SystemTime::now()` in production code — use the `CurrentClock` type alias instead - [ ] No shared mutable state between tests - [ ] Behaviour coverage is maximized with maintainable tests diff --git a/.vscode/settings.json b/.vscode/settings.json index d27d562e8..fe3f82b90 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -2,6 +2,9 @@ "[rust]": { "editor.formatOnSave": true }, + "[markdown]": { + "editor.formatOnSave": false + }, "[ignore]": { "rust-analyzer.cargo.extraEnv": { "RUSTFLAGS": "-Z profile -C codegen-units=1 -C inline-threshold=0 -C link-dead-code -C overflow-checks=off -C panic=abort -Z panic_abort_tests", diff --git a/docs/copilot-pr-reviews/pr-2174-copilot-suggestions.md b/docs/copilot-pr-reviews/pr-2174-copilot-suggestions.md new file mode 100644 index 000000000..e7361b49a --- /dev/null +++ b/docs/copilot-pr-reviews/pr-2174-copilot-suggestions.md @@ -0,0 +1,48 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md + - packages/udp-server/src/server/request_buffer.rs +--- + + + + + +# PR #2174 Copilot Suggestions Tracking + +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/2174 + +Status legend: + +- `action`: code/docs change applied +- `no-action`: suggestion reviewed; no code change needed +- `resolved`: thread resolved in PR + +## Processing Log + +- 2026-09-09: Started processing six Copilot suggestions after rebasing the draft PR. +- 2026-09-09: Completed all six suggestions. Three received focused action commits, and three + were resolved as already addressed or intentionally declined with a documented rationale. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | +| 1 | `PRRT_kwDOGp2yqc6goHu9` | `packages/udp-server/src/server/request_buffer.rs` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967777870) | Bound task-completion waits so a cleanup regression cannot hang CI. | action: added one-second absolute cleanup bound in `1ef8589b`. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968614576) | DONE | RESOLVED | +| 2 | `PRRT_kwDOGp2yqc6goHvj` | `docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967777918) | Update stale request-buffer implementation status. | no-action: duplicate suggestion addressed in `01cde544`. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968226077) | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6goHv9` | `docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967777950) | Update stale request-buffer implementation status. | action: corrected test-only completion/deferred benchmark status in `01cde544`. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968216396) | DONE | RESOLVED | +| 4 | `PRRT_kwDOGp2yqc6goHwW` | `docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967777992) | Mark completed handler-dispatch plan as complete. | no-action: already addressed in `9c05e359`. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968220747) | DONE | RESOLVED | +| 5 | `PRRT_kwDOGp2yqc6goHwn` | `packages/udp-server/src/server/request_buffer.rs` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967778018) | Avoid direct ring-buffer mutation in tests where public behavior can express setup. | no-action: `force_push` is the test Act; direct insertion remains controlled Arrange mechanics. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968617981) | DONE | RESOLVED | +| 6 | `PRRT_kwDOGp2yqc6goHwz` | `packages/udp-server/src/server/request_buffer.rs` | [thread](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3967778042) | Avoid hard-coded active-request capacity in test setup. | action: derive the retained count from actual buffer capacity in `1ef8589b`. | [reply](https://github.com/torrust/torrust-tracker/pull/2174#discussion_r3968621167) | DONE | RESOLVED | + +## Notes + +- Process every thread individually: reply before resolving it. +- Record the action/no-action decision, rationale, and reply URL in this audit log. +- R6 in `test-refactor-plans/request-buffer-tests.md` records the approved rationale and prose-first + design review for request-buffer suggestions 1, 5, and 6. diff --git a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md index 661ef428b..c557994a4 100644 --- a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md +++ b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md @@ -23,7 +23,9 @@ semantic-links: ## Goal -Improve maintainable automated test coverage across the current Torrust Tracker workspace packages, prioritizing critical behavior and making the published crates robust and reliable for consumers. +Improve maintainable automated test coverage across the current Torrust Tracker workspace packages, +prioritizing critical behavior and increasing the proportion of fast package-local unit coverage so +published crates are robust and reliable for consumers. ## Why This Is Needed @@ -33,9 +35,18 @@ The repository was reorganized through package refactoring and extraction work. ### In Scope -- Establish and record a coverage baseline for each package addressed by a subissue, then aim to increase it by testing critical behavior. Record an issue-local, human-readable coverage-evidence document with the command, measurement scope, aggregate comparison, per-file results, and prioritized uncovered areas. +- Establish and record separate aggregate and unit-only coverage baselines for each package addressed + by a subissue, then aim to increase both through critical behavior tests, prioritizing unit-only + improvement. Record an issue-local, human-readable coverage-evidence document with the command, + measurement scope, per-file results, and prioritized uncovered areas. Aggregate/global coverage + includes all selected test binaries and shows broad progress; it must not be used to infer that a + source seam has sufficient unit coverage. When aggregate coverage includes multiple test binaries, + measure and record unit-only and integration-only contributions separately. - Add maintainable, fast, responsibility-oriented unit tests close to the code they protect, using Arrange, Act, Assert (AAA) structure where appropriate. -- Add integration tests, runnable examples, or end-to-end tests when they provide valuable package-level regression protection. +- Add integration tests, runnable examples, or end-to-end tests only when a unit test cannot protect + the behavior at an appropriate boundary or the higher-level test gives a clearer, more maintainable + behavioral contract. Existing or newly added higher-level coverage never justifies declining a + feasible focused unit test for a package-owned responsibility. - When a package behavior is impractical to cover with a unit test, select the narrowest stable test boundary that can cover it: package-local integration or end-to-end tests first, then root `tests/` integration tests or `packages/e2e-tools/` only when the behavior is necessarily composed at that level. Record the chosen boundary and its rationale in the subissue evidence. - For every package subissue, assess the applicability and current evidence for unit tests, package-local integration tests, runnable examples, package/root/end-to-end tests, mutation @@ -75,19 +86,42 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. Add a row only when work begins on a package subissue. Record its baseline before adding tests and its latest measurement after implementation. Link each row to the subissue's issue-local `coverage-evidence.md`, which remains the source of truth for measurement scope, per-file detail, -and prioritized gaps. These aggregate values show progress across the EPIC; they do not determine -whether a subissue has adequately covered critical behavior. +and prioritized gaps. Keep aggregate/global and unit-only results in separate tables: they have +different denominators and answer different questions. Neither aggregate nor integration coverage +can establish that unit-test coverage is sufficient. + +### Aggregate Coverage (All Selected Test Levels) + +Aggregate values show broad package progress only; they do not attribute a source seam to a test +level or determine whether unit coverage is adequate. | Package | Subissue | Baseline | Latest | Change | Evidence | | ---------------------------------- | ------------------------------------------------------------------- | ------------------------------------------------- | ------------------------------------------------- | ------------------------------------------------------- | ------------------------------------------------------------------------------------------- | | `torrust-tracker-axum-http-server` | [#2136](../2136-1347-add-tests-axum-http-server/ISSUE.md) | Lines: 93.82%; regions: 91.66%; functions: 89.54% | Lines: 95.07%; regions: 92.99%; functions: 90.86% | Lines: +1.25 pp; regions: +1.33 pp; functions: +1.32 pp | [Coverage evidence](../2136-1347-add-tests-axum-http-server/coverage-evidence.md) | | `torrust-tracker-udp-server` | [#2149](../2149-1347-add-focused-udp-server-package-tests/ISSUE.md) | Lines: 96.96%; regions: 95.79%; functions: 97.19% | Not yet measured | Not yet measured | [Coverage evidence](../2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md) | +### Unit-Only Coverage + +Unit-only values track the primary package-local testing objective. Do not replace a missing or +weak unit-only result with aggregate, integration, example, or end-to-end coverage. + +| Package | Subissue | Baseline | Latest | Change | Evidence | +| --- | --- | --- | --- | --- | --- | +| `torrust-tracker-axum-http-server` | [#2136](../2136-1347-add-tests-axum-http-server/ISSUE.md) | See issue-local evidence | See issue-local evidence | See issue-local evidence | [Coverage evidence](../2136-1347-add-tests-axum-http-server/coverage-evidence.md) | +| `torrust-tracker-udp-server` | [#2149](../2149-1347-add-focused-udp-server-package-tests/ISSUE.md) | Not measured before #2149 increments | Pending final measurement | Pending final measurement | [Coverage evidence](../2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md) | + ## Delivery Strategy Implement independently reviewable, package-scoped subissues. When work begins on a package, add it to the Package Coverage Tracking table and record its starting coverage before adding tests. After implementation, update the row with the latest measurement and percentage-point change. Each subissue records its starting coverage, the critical responsibilities assessed, the coverage increase achieved where practical, verification evidence, and any explicitly justified exclusions. Store the coverage evidence in an issue-local human-readable document, rather than committing large raw coverage artifacts. State which source paths and code types the measurement includes, because test-inclusive totals are not production-only coverage. Use aggregate percentages only for navigation; prioritize behavior by examining per-file coverage and uncovered functions or regions. -Prioritize fast unit tests close to the code being changed, while retaining or adding integration, runnable-example, and end-to-end tests when they provide valuable regression protection. Coverage percentage informs the work but does not replace testing critical behavior. Record reusable test-design refactors in the [testing refactoring-pattern catalog](../../../testing/refactoring-patterns/README.md) so later subissues can apply proven patterns without restating their rationale. +Prioritize fast unit tests close to the code being changed. Use package integration tests only when a +unit test cannot protect the behavior at an appropriate boundary or the real package boundary +produces a clearer, more maintainable contract; retain runnable-example and end-to-end coverage +where they add distinct regression value. A passing higher-level test is not evidence that an +available unit seam needs no test. Coverage percentage informs the work but does not replace testing +critical behavior. + +When an aggregate coverage command runs unit and integration binaries together, it must not be used as proof that either boundary is adequately covered. For each selected source seam, issue-local evidence must state which test level protects it and, when the aggregate report could conceal that distinction, record separate unit-only and integration-only measurements using the relevant Cargo target selection. Compare results only within the same measurement scope because test-support code may produce different denominators. Record reusable test-design refactors in the [testing refactoring-pattern catalog](../../../testing/refactoring-patterns/README.md) so later subissues can apply proven patterns without restating their rationale. When a package behavior is covered outside its package, add a high-signal semantic link from the subissue specification to the external test artifact using the @@ -105,6 +139,12 @@ For every test-producing task, apply this development loop: 4. After the final test-producing task, stop and request maintainer review before final verification, committing, or opening a pull request. 5. Address review feedback, then complete verification and acceptance review. +A helper is justified by a meaningful name for one coherent action, capability, or state and by +keeping the caller at a consistent abstraction level—not by a minimum number of callers. A +single-use helper is appropriate when it hides only incidental mechanics and leaves causal state, +the production Act, and independently specified expected results visible. Reject vague helpers, +parameter bags, and helpers that conceal behavior or mix responsibilities. + For multi-input protocol behavior, scenarios should own every related artifact that describes the example, including selector request fields, domain input, and independently specified expected output. Builders may hide irrelevant fields of an individual artifact. Do not derive expected values by calling production mapping or serialization code under test. Keep the production-boundary invocation, concrete expected representation, and final actual-versus-expected assertion visible; helpers may encapsulate only repeated mechanics such as successful-response decoding. For each subissue implementation, the completion policy is: @@ -154,6 +194,10 @@ For each subissue implementation, the completion policy is: testing. Its spec-only PR records the 96.96% line, 95.79% region, and 97.19% function baseline, then requires per-file test-refactor plans and small, reviewed commit points before implementation. - https://github.com/torrust/torrust-tracker/issues/2149 +- 2026-09-09 - User/maintainer - Clarified that package-testing subissues must assess both + unit-test and integration-test coverage separately. Unit tests are the default priority; an + integration test requires evidence that a unit test is unsuitable or less readable at the chosen + package boundary. Combined coverage reports must not be treated as proof of unit coverage. ## Acceptance Criteria @@ -184,6 +228,7 @@ For each subissue implementation, the completion policy is: ## Risks and Trade-offs - Coverage percentage can conceal critical low-coverage files behind strong aggregate results; mitigate it by maintaining per-file and uncovered-area evidence, then selecting behavior by risk rather than pursuing a percentage target. +- Combined coverage can conceal whether a unit or integration binary executed a source seam; mitigate it by recording separate test-level measurements whenever aggregate coverage is used for a coverage decision. - Raw coverage formats can be too large or tool-oriented for code review; mitigate this by committing a concise, human-readable issue-local evidence document and retaining the reproducible command instead. - Testing may expose design seams that are difficult to isolate; make small testability refactorings only when justified and keep unrelated refactoring out of scope. - New packages or package extractions can change the inventory during the EPIC; add concrete subissues as needs are identified and record deferrals explicitly before closing the EPIC. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md index 2ca827bd6..833e4ccfb 100644 --- a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md @@ -6,9 +6,9 @@ priority: p2 epic: 1347 github-issue: 2149 spec-path: docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md -branch: "2149-add-focused-udp-server-package-tests-spec" +branch: "2149-add-focused-udp-server-package-tests" related-pr: 2152 -last-updated-utc: 2026-09-07 09:42 +last-updated-utc: 2026-09-10 semantic-links: skill-links: - create-issue @@ -26,7 +26,18 @@ semantic-links: - packages/udp-server/src/server/launcher.rs - packages/udp-server/tests/server/contract.rs - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/request-buffer-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/event-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/bound-socket-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/handler-dispatch-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/launcher-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/contract-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-metric-tests.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/container-tests.md + - packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md --- @@ -59,21 +70,31 @@ must protect current normal-operation behavior without preempting that design. ### In Scope -- Establish package-source coverage baseline and final evidence using a reproducible - `cargo llvm-cov` command, aggregate comparison, per-file results, and prioritized uncovered - behavior. +- Establish separate aggregate/global and unit-only package-source coverage baselines and final + evidence using reproducible `cargo llvm-cov` commands. Aggregate/global coverage tracks all + selected test binaries; unit-only coverage tracks the primary package-local objective. Keep their + results in separate evidence tables and do not infer unit coverage from aggregate execution. Where + aggregate coverage could hide the selected boundary, also record integration-only coverage and + each test level's contribution. - Inventory current unit, real-loopback package integration, example, root integration, and relevant historical coverage before selecting new tests. - Add focused, deterministic tests for package-owned transport and dispatch seams where they protect observable behavior: socket binding metadata, packet/error conversion, event/error classification, container composition, and normal-operation request-buffer capacity/cleanup. +- Establish and record a reproducible release-performance baseline before an approved production + change to a UDP hot-path file. Compare equivalent repeated measurements after the change; do not + require throughput measurements for test-only changes. - Assess launcher admission behavior only where it can be tested without timing dependence, production refactoring, or a competing lifecycle design. - Review every test-bearing file selected by the evidence inventory. Create one file-local refactor plan for each concrete opportunity, then improve test readability, maintainability, expressiveness, or behavior coverage without reducing valuable existing protection. -- Review `tests/server/contract.rs` and add only approved real-socket contracts that cover a - stable package transport behavior not already protected at a better boundary. +- Do not decline a feasible deterministic package unit test because integration, example, root, or + end-to-end coverage already executes the behavior. Higher-level coverage may retain a distinct + contract but is not a substitute for the unit-first objective. +- Review `tests/server/contract.rs` and add an approved real-socket contract only when a unit test + cannot protect the behavior at an appropriate boundary or the real-loopback contract is clearer + and more maintainable. Record why the integration boundary is preferred. - Perform a bounded mutation-testing assessment after the evidence and incremental test plan are approved; retain only behavior-relevant survivors as a follow-up queue. @@ -95,8 +116,9 @@ must protect current normal-operation behavior without preempting that design. ## Architectural Decisions - Related ADRs: `docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md` +- Package-local ADR: `packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md` - Related shutdown governance: `docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md` -- ADRs to create: None expected. Create one if this work identifies a durable package or +- ADRs to create: None known. Create one if this work identifies another durable package or cross-package ownership/design decision. ## Design and Ownership Review @@ -120,17 +142,17 @@ without evidence of a shared capability. Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`, `DEFERRED`. -| ID | Status | Task | Notes / Expected Output | -| --- | ------ | ------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | DONE | Record baseline and test-boundary inventory | [coverage-evidence.md](coverage-evidence.md) records the exact command, package-source scope, aggregate baseline, per-file detail, priority gaps, and external-coverage/deferral decisions. | -| T2 | TODO | Review and approve test design | Inventory test-bearing files and create one file-local plan per concrete opportunity in [test-refactor-plans/](test-refactor-plans/README.md). Each plan identifies strengths, problems, ordered improvements, scope guardrails, and focused validation. Assess unit, package integration, example, root/E2E, mutation, property, and fuzz techniques before adding tests. | -| T3 | TODO | Improve request-buffer tests | Implement the approved `server/request_buffer.rs` plan increment for current normal-operation capacity, finished-task removal, eviction, or drop cleanup. Explicitly exclude shutdown drain/deadline policy. **Commit point:** one reviewed request-buffer plan increment plus its focused validation. | -| T4 | TODO | Improve dispatch and classification tests | Implement the approved plan increment(s) for `event.rs`, `error.rs`, or `handlers/mod.rs`. Keep event/error classification and packet-dispatch behavior separate from handler business rules. **Commit point:** one reviewed, coherent classification or dispatch increment plus focused validation. | -| T5 | TODO | Improve socket-adapter tests | Implement the approved `server/bound_socket.rs` or `server/receiver.rs` plan increment for stable socket metadata, port-zero allocation, or receive adaptation. Do not assert platform-specific dual-stack defaults. **Commit point:** one reviewed socket-adapter increment plus focused validation. | -| T6 | TODO | Improve container-composition tests | Implement a `container.rs` test-plan increment only if review identifies a package-owned composition regression not already proven indirectly. A justified no-change decision completes this task without a commit. **Commit point:** one reviewed composition increment plus focused validation, if code changes are warranted. | -| T7 | TODO | Improve admission or UDP contracts | Implement one approved `server/launcher.rs` or `tests/server/contract.rs` increment only when the package integration boundary adds unique stable value. Record an infeasible seam rather than forcing a production refactor. **Commit point:** one reviewed admission or real-loopback contract increment plus focused validation. | -| T8 | TODO | Perform bounded mutation assessment | Run a time-bounded sample against the completed changed/high-risk seam. Record configuration, duration, limitations, and behavior-relevant surviving mutants; do not create a score target or CI gate. **Commit point:** documentation-only commit if the evidence materially changes the tracked review queue. | -| T9 | TODO | Review, verify, and complete evidence | Stop for maintainer review after the final test increment, then run checks, manual scenarios, refreshed coverage, acceptance review, and completion review. **Commit point:** final documentation/evidence commit only after the required review and verification. | +| ID | Status | Task | Notes / Expected Output | +| --- | ----------- | ------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | DONE | Record baseline and test-boundary inventory | [coverage-evidence.md](coverage-evidence.md) records the exact command, package-source scope, aggregate baseline, per-file detail, priority gaps, and external-coverage/deferral decisions. | +| T2 | DONE | Review and approve test design | The reviewed file-local plans cover request-buffer, event, parse-error adapter, bound socket, handler dispatch, launcher, real-loopback contract, and error-metric seams. The proposed `container.rs` plan is the remaining package-composition assessment and follows the clarified unit-first policy. | +| T3 | DONE | Improve request-buffer tests | Completed the reviewed request-buffer plan: capacity-available, oldest-first eviction, and buffer-drop cleanup contracts are covered; R2 documents the intentional bounded policy and R5 defers the scheduler-dependent race guard. The current per-file comparison is recorded in [coverage-evidence.md](coverage-evidence.md). **Commit point:** completed through focused reviewed increments. | +| T4 | DONE | Improve dispatch and classification tests | Completed reviewed `event.rs` classification/metric representations, `error.rs` parse-error adapter coverage, `handlers/mod.rs` packet dispatch coverage, and statistics error-metric routing coverage. **Commit point:** completed through focused reviewed increments. | +| T5 | DONE | Improve socket-adapter tests | Completed the reviewed `server/bound_socket.rs` plan with stable IPv4 loopback port-zero allocation and endpoint-metadata contracts. Platform-specific dual-stack defaults remain intentionally outside the test contract. **Commit point:** completed through focused reviewed increments. | +| T6 | IN_PROGRESS | Improve container-composition tests | The proposed `container.rs` plan identifies one feasible deterministic package unit contract for the explicitly enabled server event-publication path. Existing integration coverage is not a reason to decline it; implementation awaits maintainer approval. **Commit point:** one reviewed composition increment plus focused validation, if code changes are warranted. | +| T7 | DONE | Improve admission or UDP contracts | Completed reviewed `server/launcher.rs` admission/event increments and `tests/server/contract.rs` real-loopback contract increments. The contract plan records the justified no-change boundary for further transport expansion. **Commit point:** completed through focused reviewed increments. | +| T8 | TODO | Perform bounded mutation assessment | Run a time-bounded sample against the completed changed/high-risk seam. Record configuration, duration, limitations, and behavior-relevant surviving mutants; do not create a score target or CI gate. **Commit point:** documentation-only commit if the evidence materially changes the tracked review queue. | +| T9 | TODO | Review, verify, and complete evidence | Stop for maintainer review after the final test increment, then run checks, manual scenarios, refreshed coverage, acceptance review, and completion review. **Commit point:** final documentation/evidence commit only after the required review and verification. | ## Commit Points @@ -229,6 +251,49 @@ responsibility. - 2026-09-07 10:08 UTC - GitHub Copilot - Opened spec-only PR #2152 against `develop` from the fork branch `josecelano:2149-add-focused-udp-server-package-tests-spec`. The PR uses `Related to #2149` and does not close the implementation issue. +- 2026-09-07 11:10 UTC - GitHub Copilot - Spec-only PR #2152 was merged into `develop`. Created + the implementation branch from the merged commit and began T2 with the proposed + [request-buffer test refactor plan](test-refactor-plans/request-buffer-tests.md). No test or + production change has been made; implementation awaits maintainer approval of R1. +- 2026-09-07 11:10 UTC - User/maintainer - Identified `ActiveRequests` as a UDP hot-path concern. + Added a performance-evidence policy requiring equivalent release throughput baseline and after + measurements before any approved hot-path production change, while keeping focused test-only + changes free from unnecessary benchmark work. +- 2026-09-07 11:27 UTC - User/maintainer - Approved the request-buffer test refactor plan. Commit + all accumulated #2149 planning and performance-evidence changes before beginning the R1 + test-only implementation increment. +- 2026-09-07 15:12 UTC - GitHub Copilot - Preserved the failed R2 experiment in an ignored handoff + while investigating whether its full-scan expectation represented a production defect or an + intentional policy. No production change was made. +- 2026-09-07 15:27 UTC - User/maintainer - After reviewing the request-buffer history, confirmed + that R2's observed oldest-first eviction behavior is an intentional performance trade-off, not a + defect. Approved a package-local ADR and source-comment clarification as an independent + documentation commit. The unsupported bug-handoff conclusion is withdrawn. +- 2026-09-07 17:03 UTC - User/maintainer - Reviewed and approved completion of the request-buffer + plan. Its current package-source measurement is 92.31% lines, 87.89% regions, and 95.65% + functions for `server/request_buffer.rs`; the issue-local evidence records the baseline comparison. +- 2026-09-08 08:13 UTC - GitHub Copilot - Began the next file-local planning step after the + completed request-buffer review. The proposed [event test plan](test-refactor-plans/event-tests.md) + targets deterministic internal-error classification and request-kind metric representations without + duplicating event emission, consumer, protocol, or tracker-core coverage. +- 2026-09-08 11:31 UTC - GitHub Copilot - Began the next file-local planning step after completing + the event plan. The proposed [parse-error adapter plan](test-refactor-plans/error-tests.md) targets + `RequestParseError` to server response-routing-metadata conversion without duplicating protocol + parsing, event classification, or error-response serialization. +- 2026-09-08 11:38 UTC - User/maintainer - Required all subsequent file-local plans to first clean + existing test code, then add missing behavior tests one at a time with a post-test design review. + The shared plan guidance and proposed [bound-socket plan](test-refactor-plans/bound-socket-tests.md) + now record this two-phase sequence. +- 2026-09-08 16:13 UTC - GitHub Copilot - Reconciled this implementation specification with the + completed event, parse-error adapter, and bound-socket plans. The next file-local planning step is + `handlers/mod.rs`; no additional test behavior is authorized until its two-phase plan is reviewed. +- 2026-09-10 - User/maintainer - Clarified that #2149 must increase package testing while + increasing the proportion of unit tests. Aggregate/global, unit-only, and integration-only + coverage are separate evidence streams. Higher-level coverage cannot justify declining a feasible + focused package unit test. +- 2026-09-10 - GitHub Copilot - Completed and pushed the error-metric handler plan. Created the + proposed `container.rs` plan for the next feasible deterministic package unit contract; no test or + production change is authorized until maintainer approval. ## Acceptance Criteria @@ -236,6 +301,8 @@ responsibility. aggregate comparison, per-file detail, and prioritized gaps. - [ ] The current unit, package integration, example, root/E2E, mutation, property, and fuzz evidence is assessed, with selected, deferred, and inapplicable levels justified. +- [ ] Coverage evidence distinguishes unit-only and integration-only contributions for every + selected seam where aggregate package coverage could conceal the responsible test boundary. - [ ] Every selected test-bearing file has a reviewed file-local refactor plan that records strengths, concrete problems, ordered improvements, guardrails, validation, and justified no-change decisions where applicable. @@ -246,6 +313,8 @@ responsibility. output in generic helpers. - [ ] Request-buffer tests distinguish current normal-operation capacity/cleanup behavior from the shutdown policy owned by SI-15. +- [ ] Any approved production change to a UDP hot-path file has reproducible before/after release + performance evidence with equivalent workload and environment details. - [ ] Any asynchronous fixture or lifecycle test change completes the Design and Ownership Review, uses bounded absolute deadlines, and has a post-vertical-slice review. - [ ] Package integration tests are added only when the actual loopback UDP boundary provides @@ -261,6 +330,8 @@ responsibility. ### Automatic Checks - `cargo llvm-cov -p torrust-tracker-udp-server --all-features --json` +- `cargo llvm-cov -p torrust-tracker-udp-server --all-features --lib --json` +- `cargo llvm-cov -p torrust-tracker-udp-server --all-features --test integration --json` - `cargo test -p torrust-tracker-udp-server` - `cargo test -p torrust-tracker-udp-server --test integration` - `linter all` @@ -286,13 +357,14 @@ Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. | AC4 | TODO | Approved refactor increments and focused validation | | AC5 | TODO | Focused test paths and test output | | AC6 | TODO | Request-buffer tests and SI-15 deferral record | -| AC7 | TODO | Design and Ownership Review or explicit non-applicability | -| AC8 | TODO | Approved real-loopback contract evidence | -| AC9 | TODO | `linter all` output | -| AC10 | TODO | Package test output | -| AC11 | TODO | Manual-verification table | -| AC12 | TODO | Post-implementation acceptance review | -| AC13 | TODO | Documentation diff and completion review | +| AC7 | TODO | `performance-evidence.md` and any required result report | +| AC8 | TODO | Design and Ownership Review or explicit non-applicability | +| AC9 | TODO | Approved real-loopback contract evidence | +| AC10 | TODO | `linter all` output | +| AC11 | TODO | Package test output | +| AC12 | TODO | Manual-verification table | +| AC13 | TODO | Post-implementation acceptance review | +| AC14 | TODO | Documentation diff and completion review | ## Risks and Trade-offs @@ -301,11 +373,21 @@ Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. subissues rather than encoding accidental behavior in a test. - Package coverage includes test code and can hide low-value framework or fixture coverage. Use it to navigate per-file gaps, while selecting tests by observable risk and ownership. +- Aggregate package coverage can also hide whether a unit-test or integration-test binary executes + a seam. Treat unit tests as the default; add an integration test only when the unit boundary is + unsuitable or the real-loopback contract is clearer and more maintainable. Record separate + unit-only and integration-only evidence when aggregate coverage informs a decision. - Socket behavior varies by host IPv6 and dual-stack support. Test port-zero and endpoint metadata invariants, and retain existing availability guards rather than asserting a universal dual-stack default. - Mutation testing can be slow and generate a tool-specific backlog. Keep it bounded and use only behavior-relevant surviving mutants to challenge assertions. +- A production hot-path refactor can cause a throughput regression even when its tests pass. + Mitigate this with the conditional, reproducible baseline policy in + [performance-evidence.md](performance-evidence.md), not with a single noisy benchmark run. +- A coverage increment can uncover a production defect outside its intended delivery scope. + Mitigate this by preserving a reproducible handoff, fixing the defect on an independent branch, + then rebasing this branch before resuming dependent coverage work. ## Implementation Completion Review @@ -325,6 +407,8 @@ material design changes, unexpected verification results, and reusable test-desi - Completed package-testing predecessors: #2136 and #2140 - Package: `packages/udp-server/` - Current real-loopback contracts: `packages/udp-server/tests/server/contract.rs` +- Performance measurement policy: [performance-evidence.md](performance-evidence.md) +- Canonical benchmarking guide: `docs/benchmarking.md` - Shutdown EPIC: `docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md` - UDP receive-loop lifecycle draft: `docs/issues/drafts/1488-si-14-migrate-udp-receive-reset-token-lifecycle/ISSUE.md` - Active-request policy draft: `docs/issues/drafts/1488-si-15-define-udp-active-request-policy/ISSUE.md` diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md index 9b35fe976..97679de9b 100644 --- a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md @@ -11,7 +11,7 @@ measured-utc: 2026-09-07 This document records the package-source coverage baseline before Issue #2149 adds or changes tests. -## Measurement Method +## Aggregate/Global Coverage Measurement ```text cargo llvm-cov clean --workspace @@ -21,16 +21,90 @@ cargo llvm-cov -p torrust-tracker-udp-server --all-features --json The raw JSON report was generated at commit `2054d494` and filtered by files below `packages/udp-server/src/`. Its 62 MB generated output is deliberately retained only in ignored local temporary storage, not committed. The table below sums its file `summary` objects. It -includes package test and test-support code, so it is navigation evidence rather than a -production-only coverage measure or proof of behavioral completeness. +includes all selected package test binaries and test-support code, so it is broad navigation +evidence rather than a production-only coverage measure, proof of behavioral completeness, or +evidence that unit coverage is sufficient. -## Baseline Package Coverage +## Unit-First Test-Level Coverage Policy + +Aggregate package reports can combine unit and integration test binaries, hiding which boundary +executed a source seam. Unit tests are the default for package-owned behavior because they are fast, +deterministic, and close to the responsibility under test. Add or retain a package integration test +only when a unit test cannot protect the behavior at an appropriate boundary or the real-loopback +contract is clearer and more maintainable. + +Do not decline a feasible deterministic package unit test because integration, example, root, or +end-to-end coverage already executes the behavior. When aggregate coverage informs a selected-seam +decision, record separate reports before claiming coverage ownership: + +```text +cargo llvm-cov clean --workspace +cargo llvm-cov -p torrust-tracker-udp-server --all-features --lib --json +cargo llvm-cov clean --workspace +cargo llvm-cov -p torrust-tracker-udp-server --all-features --test integration --json +``` + +Do not compare percentages across those reports as a single total: unit reports include unit test +and test-support code while integration reports compile only the exercised package production slice. +Use them to identify the test level that protects each selected behavior. + +### Test-Level Reporting Tables + +Update aggregate/global and unit-only tables independently. Aggregate/global totals show broad +package progress; unit-only totals show whether the primary package-local objective is improving. +Integration-only evidence identifies distinct real-boundary protection and must never substitute for +a unit-only result. + +### Selected-Seam Test-Level Evidence + +At commit `9eb74c23`, the separate reports for `packages/udp-server/src/handlers/mod.rs` show: + +| Measurement scope | Lines | Regions | Functions | Interpretation | +| --- | ---: | ---: | ---: | --- | +| Unit-only (`--lib`) | 184 / 214 (85.98%) | 224 / 249 (89.96%) | 32 / 37 (86.49%) | The direct `handle_packet` test executes the selected sendable parse-error routing seam. No executable source-line entries are uncovered in this report. | +| Integration-only (`--test integration`) | 31 / 31 (100.00%) | 18 / 18 (100.00%) | 5 / 5 (100.00%) | The real-loopback suite executes a separate compiled production slice; its smaller denominator excludes unit-test and test-support code. | +| Combined package report | 205 / 214 (95.79%) | 239 / 249 (95.98%) | 35 / 37 (94.59%) | Navigation-only aggregate; it must not be used to attribute the selected dispatcher coverage to unit or integration tests. | + +The unit test is the appropriate primary boundary for sendable parse-error routing: it makes the +raw packet, dispatcher Act, returned request kind, and response transaction ID directly readable +without socket lifecycle or client/server mechanics. Integration tests remain valuable for actual +loopback transport behavior, but are neither needed nor used as evidence for this internal dispatch +contract. + +## Aggregate/Global Package Coverage | Measurement | Lines | Regions | Functions | | ----------------------------- | ---------------------: | ---------------------: | -----------------: | | Baseline before issue changes | 4,814 / 4,965 (96.96%) | 6,326 / 6,604 (95.79%) | 485 / 499 (97.19%) | | Latest | Not yet measured | Not yet measured | Not yet measured | +## Unit-Only Package Coverage + +The #2149 baseline predates the separated measurement policy, so no unit-only baseline exists. Do +not derive one from the aggregate baseline. Record the final unit-only package measurement here and +compare future unit-only measurements only with an equivalent unit-only command. + +| Measurement | Lines | Regions | Functions | +| --- | ---: | ---: | ---: | +| Baseline before issue changes | Not measured separately | Not measured separately | Not measured separately | +| Latest | Pending final measurement | Pending final measurement | Pending final measurement | + +## Current Increment Coverage + +The following measurement was taken after the completed request-buffer plan at commit `796e2a9e`. +It is an interim comparison, not the final Issue #2149 measurement; later file-plan increments can +change package totals and source-file denominators. + +| Source file | Baseline lines | Current lines | Change | Baseline regions | Current regions | Change | Baseline functions | Current functions | Change | +| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | +| `server/request_buffer.rs` | 24 / 45 (53.33%) | 144 / 156 (92.31%) | +38.98 pp | 36 / 74 (48.65%) | 196 / 223 (87.89%) | +39.24 pp | 3 / 4 (75.00%) | 22 / 23 (95.65%) | +20.65 pp | + +The added test code increases the measured denominator because package-source coverage includes +`#[cfg(test)]` code. The meaningful result is that the capacity-available, oldest-first eviction, +and buffer-drop cleanup contracts now execute deterministically. The remaining uncovered areas are +the intentionally untested scheduler-dependent incoming-task race guard and implementation details +not selected by the approved plan. + ## Baseline Detailed File Report Files are ordered by ascending line coverage so the table highlights the review queue. The issue diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md new file mode 100644 index 000000000..ba4fffe22 --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md @@ -0,0 +1,88 @@ +--- +doc-type: performance-evidence +issue: 2149 +package: torrust-tracker-udp-server +status: planned +--- + +# UDP Server Performance Evidence + +This document defines the reproducible performance baseline required before changing a UDP server +hot-path production file for Issue #2149. It contains no benchmark result yet: no production code +has changed. The completed request-buffer plan added tests only, so its performance baseline remains +deferred until an approved non-test change affects the hot path. + +## Policy + +`server/request_buffer.rs` is invoked by `Launcher::run_udp_server_main` for every accepted UDP +request. Any change to its non-test production code requires a baseline before implementation and +an equivalent after measurement before the related commit or pull request. + +Test-only changes do not alter the release artifact. They still require focused tests and normal +quality checks, but do not require a throughput measurement unless they change production code, +benchmark configuration, release dependencies, or the runtime workload. + +If testing requires a production refactor, stop the current test increment. Record the proposed +production change, obtain maintainer approval, establish the baseline described below, and only +then resume the increment. A main-loop, task-spawning, event-publication, or shutdown-policy change +is outside Issue #2149 and must be coordinated with the relevant UDP lifecycle/main-loop work. + +## Measurement Levels + +| Level | When required | Tool and result | +| ----------------------------- | ------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Whole tracker UDP throughput | Every approved hot-path production change | Run Aquatic's `aquatic_udp_load_test` against the release-built tracker. Record response rate, response classes, errors, peers per announce, workload config, tracker config, host details, and median of repeated equivalent runs. | +| Request-buffer microbenchmark | An approved change affects `ActiveRequests` algorithm, allocation, capacity, or eviction behavior | Add or use a focused release microbenchmark for capacity available, completed-handle reclamation, and active-handle eviction. Do not infer whole-tracker throughput from it. | +| Comparative tracker benchmark | Only when assessing tracker competitiveness or a material performance regression | Optionally run Aquatic's `aquatic_bencher`; it is not a gate for focused tests because it is expensive and depends on external tracker setup. | + +The current repository has no `udp-server`/`ActiveRequests` microbenchmark. Do not add one merely +because a test changes. Add one only when an approved production algorithm change requires a direct +measurement. + +## Reproducible Whole-Tracker Baseline + +Use the current repository guidance as the source of truth: + +1. Build the tracker with `cargo build --release`. +2. Start that artifact with `share/default/config/tracker.udp.benchmarking.toml` through + `TORRUST_TRACKER_CONFIG_TOML_PATH`. +3. Build the current Aquatic source's `aquatic_udp_load_test` release binary. +4. Generate its configuration with `aquatic_udp_load_test -p`; record the complete workload file + with the evidence. +5. Run at least three equivalent, fixed-duration iterations after confirming no unrelated local + workload dominates the host. Record every run and compare medians, not a single observation. +6. Use the same tracker commit/worktree state, Aquatic revision, release profile, host/kernel, + tracker config, load-test config, CPU-affinity policy, and measurement window for before/after. + +The benchmark configuration disables verbose logging and binds UDP to port 3000. Do not compare a +run using a different configuration, logging level, client workload, or host condition as if it +were an A/B result. + +## Interpretation Rules + +- Treat the expected non-dedicated-host variance of approximately 5–10% as measurement noise until + repeated median results show otherwise. +- Report before/after response-rate differences as observations, including response and error mix; + do not declare causation from throughput alone. +- A functional test or coverage increase is not evidence of unchanged performance. +- Historical website articles are background only. Their 2024 commands, environment, tool versions, + configuration-variable names, and results may be outdated; verify every command against the + current repository guide and the checked-out Aquatic revision. + +## Planned Evidence Table + +| Measurement | Baseline | Latest | Status | Evidence | +| --------------------------------------- | --------------------------------------------------------- | ------------ | -------- | ----------------------------------------------------------------------------- | +| Aquatic UDP load test, release tracker | Not required until an approved hot-path production change | Not measured | DEFERRED | Completed request-buffer work is test-only; no hot-path production change was approved or implemented. | +| `ActiveRequests` focused microbenchmark | Not applicable; no algorithm change proposed | Not measured | DEFERRED | Add only after approval of a production algorithm/allocation/capacity change. | + +## References + +- Canonical guide: `docs/benchmarking.md` +- Current benchmark tracker configuration: `share/default/config/tracker.udp.benchmarking.toml` +- Detailed historical repository guide: `docs/issues/closed/1505-optimize-peer-ip-list-from-swarm/aquatic-benchmarking-guide.md` +- Historical baseline format: `docs/issues/closed/1505-optimize-peer-ip-list-from-swarm/baseline-performance.md` +- Historical website background (verify before use): + +- Historical website background (operational packet-path context, not a code benchmark): + diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md index 6ba31bf29..ce14e2c5d 100644 --- a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/README.md @@ -10,14 +10,39 @@ a cross-file extraction unless maintainer review establishes a cohesive common r ## Plans -- No file plans have been created. T2 will inventory each test-bearing file and create a plan only - where review identifies a concrete maintainability or behavior-coverage opportunity. +- [Request-buffer tests](request-buffer-tests.md) — complete. +- [Event tests](event-tests.md) — complete. +- [Parse-error adapter tests](error-tests.md) — complete. +- [Bound-socket tests](bound-socket-tests.md) — complete. +- [Handler-dispatch tests](handler-dispatch-tests.md) — complete. +- [Launcher tests](launcher-tests.md) — complete. +- [Contract tests](contract-tests.md) — complete. +- [Error-metric handler tests](error-metric-tests.md) — complete. +- [Container tests](container-tests.md) — proposed; awaiting maintainer approval. ## Shared Purpose Each plan improves test code without changing production behavior. It applies only to its target file and must be reviewed and approved before any proposed item is implemented. +## Required Two-Phase Sequence + +Every file-local plan follows these phases in order: + +1. **Clean current tests first.** Review existing test code for readability, expressiveness, + sustainability, duplication, deterministic execution, causal initial state, and visible + Arrange–Act–Assert structure. Implement and review approved cleanup increments before adding a + behavior test. Record a no-change decision when the file has no current tests or no concrete + cleanup opportunity. +2. **Add missing behavior tests second.** Add one approved behavior-focused test increment at a + time. After each added test, stop to review its design: remove accidental duplication, select an + inline value, builder, or scenario fixture that best exposes causal state, and keep the + production Act and independently specified assertion visible before starting the next test. + +Follow `.github/skills/dev/testing/write-unit-test/SKILL.md` and the test refactoring-pattern +catalog for both phases. Do not use the second phase as a reason to postpone obvious cleanup in the +first phase or to create speculative shared test infrastructure. + ## Shared Quality Goals The refactoring must improve or preserve: diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/bound-socket-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/bound-socket-tests.md new file mode 100644 index 000000000..ffc382cec --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/bound-socket-tests.md @@ -0,0 +1,211 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/server/bound_socket.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/server/bound_socket.rs + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/tests/server/contract.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md + - .github/skills/dev/testing/write-unit-test/SKILL.md + - docs/testing/refactoring-patterns/README.md +--- + +# UDP Bound Socket Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to `packages/udp-server/src/server/bound_socket.rs`. + +## Phase 1 — Clean Current Tests + +### Current state + +`bound_socket.rs` has no colocated test module. Existing tests only exercise it incidentally through +the launcher, processor, and real-loopback package contracts. + +### Decision + +No refactoring increment is needed before new tests. There is no test code in this target file to +clean, and changing distant consumer tests would obscure the wrapper's own socket/metadata +contract. Record this no-change decision before Phase 2 begins. + +## Phase 2 — Add Missing Behavior Tests + +### Strengths to preserve + +1. `BoundSocket::bind` establishes the package-owned invariant that every returned local port is + non-zero, including port-zero requests delegated to the OS. +2. `address`, `url`, and `service_binding` are small public metadata adapters derived from the same + bound socket. +3. `create_socket` deliberately leaves `IPV6_V6ONLY` unset when `ipv6_v6only` is false, preserving + OS defaults rather than claiming a cross-platform dual-stack contract. + +### Problems and opportunities + +#### P1 — The port-zero invariant has no direct contract + +**Problem.** No test proves that `BoundSocket::bind` returns a non-zero port when asked to bind +IPv4 loopback port zero. + +**Opportunity.** Bind `127.0.0.1:0` and assert `address().port() != 0`. + +#### P2 — Metadata adapters have no direct consistency contract + +**Problem.** No test proves that `address`, `url`, and `service_binding` describe the same +successfully bound endpoint. + +**Opportunity.** From one IPv4 loopback port-zero binding, independently assert UDP protocol, the +same bind address, and the expected `udp://
` URL representation. + +#### P3 — Platform-specific dual-stack behavior must not be inferred + +**Problem.** An IPv6 socket with `ipv6_v6only = false` has OS-dependent behavior. + +**Decision.** Do not add a dual-stack reachability test. Existing package integration covers an +IPv6-only listener where supported. A future explicit IPv6-only metadata test needs a portability +review and availability guard. + +## Proposed Refactorings + +### R1 — Record the Phase 1 no-change decision + +- **Status:** DONE +- **Priority:** High impact / trivial effort +- **Change:** Confirm this file has no existing tests to refactor and that adjacent tests remain at + their established integration/consumer boundaries. +- **Done when:** Phase 1 is recorded complete with no cleanup code change. + +### R2 — Cover port-zero binding + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Change:** Add one direct test that binds IPv4 loopback port zero and asserts the resulting port + is non-zero. +- **Guardrails:** Use an OS-assigned port; do not reserve/release a port, sleep, retry, or make a + real client request. +- **Done when:** the non-zero port invariant is asserted at the wrapper boundary. + +### R3 — Cover endpoint metadata consistency + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Change:** Add one direct test from a bound IPv4 loopback socket asserting its address, + `ServiceBinding` UDP protocol/address, and URL are consistent. +- **Guardrails:** Keep expected protocol/address/URL values independent and visible. Do not derive + expected values using `BoundSocket::url` or `service_binding`, and do not test dual-stack policy. +- **Done when:** all public endpoint representations agree for one bound socket. + +### R4 — Review Phase 2 test design + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Change:** After each added test, review Arrange–Act–Assert structure, fixture choice, and + portability. Record no-change or an approved focused cleanup before beginning the next test. +- **Decision:** No change. Each test has one visible causal state: a requested IPv4 loopback + port-zero bind. The bind/metadata Act and independently constructed assertions remain visible. + Repeating the single requested-address expression and bind call is clearer than a helper; a + scenario fixture or builder would hide ordinary valid input without expressing a new state. +- **Done when:** the final added test remains direct, deterministic, and free of unnecessary helper + abstractions. + +### R5 — Assess residual coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Change:** Measure residual coverage and document why debug formatting, `Deref`, impossible + OS-port-zero failure injection, or platform-specific dual-stack branches are covered elsewhere or + intentionally deferred. +- **Decision:** No test added. Current `bound_socket.rs` coverage is 56/65 lines (86.15%), 113/135 + regions (83.70%), and 10/11 functions (90.91%) at commit `a561e7b0`. The remaining + `create_socket` IPv6 option branch has OS-dependent dual-stack behavior and is covered at the + guarded real-listener integration boundary. `Deref` is a thin standard trait implementation; + debug output has no stable operator contract; and the post-bind port-zero error requires an + impossible OS behavior or production-only injection seam. No direct portable wrapper test would + add unique regression value. +- **Done when:** remaining gaps have an ownership/portability rationale. + +## Progress Tracking + +### Plan Checklist + +- [x] Target source and adjacent integration coverage reviewed. +- [x] Two-phase sequence applied; Phase 1 has no test code to refactor. +- [x] Maintainer approved R1. +- [x] R1 no-change decision recorded and committed. +- [x] Maintainer approved R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R3. +- [x] R3 implemented, reviewed, validated, and committed. +- [x] R4 design reviews completed and recorded. +- [x] R5 assessment completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-08 11:38 UTC - GitHub Copilot - Created this two-phase proposed plan from + `bound_socket.rs`, its launcher/processor consumers, and existing real-loopback integration + coverage. Phase 1 has no target-file tests to clean; no test or production change has been made. +- 2026-09-08 11:42 UTC - User/maintainer - Reviewed and approved the Phase 1 no-change decision. + `bound_socket.rs` has no existing colocated tests to refactor, so the next work may proceed to + the separately approved Phase 2 port-zero behavior test. +- 2026-09-08 11:49 UTC - User/maintainer - Approved R2. Bind IPv4 loopback on port zero and assert + only the non-zero returned port invariant; commit this plan update before test implementation. +- 2026-09-08 11:55 UTC - User/maintainer - Reviewed and approved R2. The direct test uses a Tokio + runtime only because `BoundSocket::bind` constructs a Tokio UDP socket; it retains the narrow + IPv4 loopback port-zero contract without client traffic, retries, sleeps, or dual-stack behavior. +- 2026-09-08 12:16 UTC - User/maintainer - Approved R3. Bind IPv4 loopback on port zero, retain the + bound address as the independently observed endpoint, and assert that public URL and service + binding representations use that same UDP endpoint. Commit the plan update before implementation. +- 2026-09-08 12:22 UTC - User/maintainer - Reviewed and approved R3. The direct test observes one + bound IPv4 endpoint and independently verifies its URL and UDP service-binding representations, + without client traffic, dual-stack assumptions, or production changes. +- 2026-09-08 12:31 UTC - GitHub Copilot - Completed R4 design review. Retained direct inline + requested-address and bind setup because it exposes the sole causal state more clearly than a + helper, builder, or scenario fixture. Both Phase 2 tests remain deterministic and portable. +- 2026-09-08 12:35 UTC - GitHub Copilot - Completed R5 assessment. The refreshed package-source + report gives `bound_socket.rs` 86.15% lines, 83.70% regions, and 90.91% function coverage. + Remaining IPv6 option, `Deref`, debug, and impossible OS-port-zero paths have no additional + stable portable wrapper contract; no test is added. +- 2026-09-08 12:39 UTC - User/maintainer - Reviewed and approved the completed bound-socket plan. + Phase 1 records the no-test cleanup decision; Phase 2 adds direct port-zero and endpoint-metadata + contracts; R4/R5 document their design and portability decisions. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after plan review changes. | +| R1 | DONE | Maintainer approved the explicit no-change decision: there is no target-file test code to clean before Phase 2. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server bound_socket::tests`, and `git diff --check` passed. One Tokio-bound direct test covers the non-zero port invariant. | +| R3 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server bound_socket::tests`, and `git diff --check` passed. One direct test covers URL and UDP service-binding endpoint consistency. | +| R4 | DONE | No change: direct inline setup keeps the port-zero IPv4 causal state, production Act, and independently specified endpoint assertions visible. | +| R5 | DONE | No change: 86.15% lines, 83.70% regions, and 90.91% functions. Remaining platform-dependent or trait/debug/impossible-injection paths lack a unique portable wrapper contract. | +| Plan completion | DONE | Maintainer reviewed all approved increments and decisions before the next file plan begins. | + +## Non-Goals + +- Do not test UDP packet reception, processing, listener lifecycle, or application registration. +- Do not assert platform-default dual-stack reachability, add port handoff/retry logic, or inject an + impossible OS-assigned port-zero error. +- Do not change `BoundSocket` production behavior or create a generic socket-test factory. + +## Validation Per Approved Increment + +- Run focused `bound_socket` unit tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- After each Phase 2 behavior test, review causal state, visible Act, independent expected value, + and portability before the next increment. + +## Completion Criteria + +- Phase 1 no-change decision is explicit and justified. +- Phase 2 tests protect stable wrapper invariants without crossing into listener or dual-stack + integration behavior. +- Every behavior test has a recorded post-test design review. +- The maintainer reviews all approved increments before the next file plan begins. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/container-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/container-tests.md new file mode 100644 index 000000000..4b54b07c3 --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/container-tests.md @@ -0,0 +1,202 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/container.rs +status: proposed +semantic-links: + related-artifacts: + - packages/udp-server/src/container.rs + - packages/udp-server/src/event.rs + - packages/events/src/bus.rs + - packages/udp-server/src/server/launcher.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Server Container Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to `packages/udp-server/src/container.rs`. + +## Phase 1 - Clean Current Tests + +### Current state + +`container.rs` has no direct tests. `UdpTrackerServerServices::initialize` constructs the package +`Broadcaster`, an explicitly enabled server `EventBus`, its optional event sender, and a statistics +repository. `UdpTrackerServerContainer::initialize` exposes cloned handles from those services. + +The aggregate package baseline reports `container.rs` as 19/19 lines, 29/29 regions, and 2/2 +functions covered, but that global result does not show which test level provides the coverage. +Existing launcher unit tests construct the real container and observe server events, while root +integration tests own multi-listener metrics and banning policy. + +### Decision + +No cleanup increment is proposed because no local test code exists. Preserve the concise explicit +container composition. Do not treat indirect aggregate or integration coverage as a reason to +skip a feasible focused unit test: the issue's unit-only coverage objective requires this package +composition decision to be assessed at the unit boundary. + +## Phase 2 - Add Missing Behavior Tests + +### Strengths to preserve + +1. `UdpTrackerServerServices::initialize` owns selection of an enabled UDP-server event-publication + path. +2. The container owns package-local coherence between its `event_bus` and `stats_event_sender`. +3. `packages/events` owns generic enabled/disabled event-bus behavior. +4. Launcher tests own server admission facts, and root integration tests own multi-listener metrics + filtering and banning outcomes. + +### Problems and opportunities + +#### P1 - The package-selected enabled publication path has no direct unit contract + +**Problem.** Indirect launcher coverage proves that the container is exercised, but a failure does +not isolate the container's own composition decision. Aggregate/global coverage cannot establish +that this package responsibility has focused unit protection. + +**Why it matters.** A future change can disable the server event bus, omit its sender, or wire the +sender to a different bus while higher-level tests fail less locally or only under a specific +listener configuration. + +**Opportunity.** Add one deterministic asynchronous unit test that initializes +`UdpTrackerServerServices`, creates a receiver from its `event_bus`, publishes one representative +UDP-server event through `stats_event_sender`, and asserts that exact event is received. Use one +absolute timeout solely as a diagnostic failure bound; do not use a delay, socket, spawned server, +or lifecycle fixture. + +The event and its `ConnectionContext` are setup mechanics for observing the container-owned +publication path. Keep the sender, receiver, exact event, production publication Act, and received +event assertion visible. Do not derive the expected event through production code. + +#### P2 - Multi-listener policy is not a container-unit responsibility + +**Decision.** Do not test metrics-enabled/disabled listener policy, root statistics aggregation, +REST exposure, or banning outcomes here. Those require root composition and retain their existing +higher-level ownership. This direct unit test protects the package's unconditional event +publication, not a consumer's policy. + +#### P3 - Generic event-bus variants are not this package's responsibility + +**Decision.** Do not add a disabled-sender test or a generic `EventBus` matrix. The events package +owns that implementation. This package needs one contract proving its explicit selection of the +enabled mode is observable through its own composed services. + +## Proposed Refactorings + +Apply items in order. Complete one approved increment—including prose-first comparison, focused +validation, review, and its mapped commit point—before beginning the next item. + +### R1 - Record the Phase 1 no-change decision + +- **Status:** DONE +- **Priority:** High impact / trivial effort +- **Addresses:** Phase 1 +- **Change:** Confirm `container.rs` has no direct tests to clean and that direct unit coverage, + not indirect global or integration coverage, is required for the package-owned enabled publication + decision. +- **Guardrails:** Do not move launcher or root tests, change production composition, or introduce a + fixture before a specific test requires it. +- **Decision:** `container.rs` has no direct tests to clean. Its current composition is concise and + explicit, so no test-code refactor applies. Indirect aggregate/global and higher-level coverage + do not substitute for assessing the feasible focused unit contract in R2. +- **Done when:** The no-cleanup decision is recorded before adding a test. + +### R2 - Cover the enabled server event-publication path + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1 +- **Change:** Add one direct asynchronous unit test for `UdpTrackerServerServices::initialize`. + Publish a representative event through its available sender and assert that its own event-bus + receiver obtains that exact event. +- **Guardrails:** Keep the causal enabled sender, publication Act, and expected event visible. Use + only an absolute diagnostic timeout. Do not assert optional-sender implementation details, test + generic disabled behavior, add sockets/tasks, or assert metrics/banning/root policy. +- **Prose-first review:** The temporary prose specified that newly initialized services publish a + server event through their enabled sender to their own event-bus receiver. The final code makes + the initialized services, available sender, exact representative event, sender publication Act, + and received-event assertion visible. `sample_udp_request_received_event` names only incidental + valid event construction; no fixture derives the expected event. The timeout is an absolute + diagnostic failure bound. Temporary prose is redundant and removed. +- **Done when:** Disabling or disconnecting the package-composed publication path has one direct, + deterministic unit-test failure. + +### R3 - Review residual composition coverage and ownership + +- **Status:** TODO +- **Priority:** Low impact / low effort +- **Change:** Measure unit-only coverage for `container.rs`, separately retain aggregate/global and + integration-only evidence, and record ownership for residual paths. +- **Guardrails:** Do not add tests merely to increase percentages. Do not claim unit coverage from + aggregate/global or integration-only results. +- **Done when:** Unit-only measurement and each residual ownership decision are recorded. + +## Progress Tracking + +### Plan Checklist + +- [x] Container source, event-bus responsibility, indirect package coverage, and root-policy + boundaries reviewed. +- [x] Maintainer approved R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [ ] R3 coverage/ownership review completed and decision recorded. +- [ ] Maintainer reviewed all approved changes. +- [ ] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-10 - GitHub Copilot - Created this proposed plan after reviewing package container + composition, event-bus ownership, existing launcher unit tests, root policy tests, and the + clarified unit-first coverage objective. No test or production change has been made. +- 2026-09-11 - User/maintainer - Approved R1. Record that `container.rs` has no direct test code + to clean; do not use indirect aggregate/global or higher-level coverage to avoid the R2 unit-test + assessment. +- 2026-09-11 - User/maintainer - Approved R2. Add the direct deterministic services event-bus + publication test only, retaining the visible sender, event, publication Act, and received-event + assertion. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after maintainer review changes. | +| R1 | DONE | The reviewed source has no direct test code or concrete cleanup opportunity. The explicit no-cleanup decision preserves the feasible R2 unit-test assessment under the unit-first coverage policy. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server container::tests::should_publish_events_through_the_enabled_server_event_bus`, and `git diff --check` passed. Prose-first review keeps the enabled sender, exact event, publication Act, and received-event assertion visible; the timeout is diagnostic only. | +| R3 | TODO | Awaiting approved increments. | + +## Non-Goals + +- Do not change container production code, event-bus implementation, metrics aggregation, banning, + REST exposure, or multi-listener policy. +- Do not duplicate generic enabled/disabled `EventBus` tests owned by `packages/events`. +- Do not start sockets, listeners, server tasks, cancellation, shutdown, sleeps, polling, or a + lifecycle fixture; those concerns remain owned by #1488. +- Do not replace root integration tests or use their coverage to claim this direct unit contract. + +## Validation Per Approved Increment + +- Apply the mandatory prose-first Arrange-Act-Assert comparison before maintainer review. +- Run the focused `container` unit test and then the package `--lib` target when the increment is + approved for broader validation. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Measure and record aggregate/global, unit-only, and integration-only coverage separately whenever + coverage informs a decision. + +## Completion Criteria + +- The package-selected enabled event-publication path has one direct, deterministic unit contract. +- The test keeps its causal enabled sender, production publication Act, and exact received event + visible without a generic fixture. +- Aggregate/global, unit-only, and integration-only coverage are recorded in separate tables and + used only for their respective claims. +- Generic event-bus behavior, root consumer policy, and lifecycle concerns remain at their existing + ownership boundaries. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/contract-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/contract-tests.md new file mode 100644 index 000000000..789f8348b --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/contract-tests.md @@ -0,0 +1,231 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/tests/server/contract.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/tests/server/contract.rs + - packages/udp-server/tests/server/asserts.rs + - packages/udp-server/src/server/receiver.rs + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/src/server/processor.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Server Contract Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to `packages/udp-server/tests/server/contract.rs`. + +## Phase 1 - Clean Current Tests + +### Current state + +`contract.rs` contains the package's real-loopback UDP contracts. Those tests are the appropriate +boundary for wire behavior that a unit test cannot express, including datagram serialization, +response decoding, and listener configuration. They currently repeat environment/client bootstrap, +manual `match`-and-`panic!` error handling, and teardown calls. The first empty-request contract +also constructs and decodes the transport exchange inline, so its intended BEP 15 error-response +contract is less prominent than its mechanics. + +`src/server/receiver.rs` is exercised by this real-loopback suite. Its only direct wrapper behavior +is converting a bound socket receive into `RawRequest`; a new direct test would require the same +UDP I/O boundary and would be less readable than the existing integration coverage. No receiver +plan is proposed unless this contract review exposes a receiver-specific regression gap. + +### Decision + +Begin with one prose-first refactor of +`should_return_a_bad_request_response_when_the_client_sends_an_empty_request`. Its temporary prose +must distinguish the causal empty UDP datagram, the real loopback exchange, and the independently +specified error response. Extract only repeated, non-behavioral mechanics that remain useful to an +adjacent contract; do not introduce a general integration-test framework or refactor the entire +file in one increment. + +## Phase 2 - Add Missing Behavior Tests + +### Strengths to preserve + +1. Real-loopback contracts cover actual UDP client/server serialization and the receive/send + transport boundary. +2. Unit tests own internal adapter, admission, event, and normal-operation buffer behavior first. +3. Existing contract tests exercise connect, announce, scrape, invalid packet, IPv6, and selected + connection-ID-validation behaviors. + +### Problems and opportunities + +#### P1 - The empty-datagram wire contract has a readability opportunity + +**Problem.** The test's Arrange and Act mix server bootstrap, client bootstrap, datagram send, +response receive, and protocol decoding. + +**Opportunity.** Use prose-first AAA verification to expose “an empty datagram receives the +protocol error response” while retaining the actual client/server transport call and independently +specified error response assertion. + +#### P2 - Integration behavior must remain distinct from unit seams + +**Decision.** Do not add an integration test for the launcher admission decisions, parse-error +routing, or event classification covered by #2149 unit tests. Add a contract only when real UDP +datagram transport gives clearer or unique regression value. + +#### P3 - Broad integration-fixture extraction is premature + +**Decision.** Do not introduce a configurable server/client builder or shared lifecycle abstraction. +Extract one helper only after the first prose-first refactor demonstrates repeated non-behavioral +mechanics and keeps each test's causal state, Act, and expected result visible. + +## Proposed Refactorings + +Apply items in order. Complete one approved increment—including prose-first comparison, focused +validation, review, and its mapped commit point—before beginning the next item. + +### R1 - Clarify the empty-datagram error contract + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1, P3 +- **Change:** Write temporary Arrange-Act-Assert prose for the empty-request contract. Refactor + only enough to make the empty datagram, direct UDP exchange, and expected error response clearly + visible. Keep server lifecycle setup and teardown correct. +- **Guardrails:** Do not assert logging, internal parser implementation, event delivery, or + statistics. Do not use sleeps, polling, or a new generic fixture. Keep response parsing and the + error assertion in the test or a narrowly named decoding helper that does not derive expectations. +- **Prose-first review:** The temporary Arrange prose was “a running UDP tracker and a real + loopback client send an empty datagram.” `start_ephemeral_udp_tracker` names the coherent + non-behavioral lifecycle setup, even with one caller, because it keeps the test at the same + abstraction level as its real UDP interaction. `empty_udp_datagram` makes the causal input + visible. The Act retains send, receive, and protocol decode steps; the Assert independently + specifies the missing-protocol-identifier error. Temporary prose is redundant and removed. +- **Done when:** the code expresses the wire contract without redundant prose and has one clear + behavioral reason to fail. + +### R2 - Assess one adjacent real-loopback contract improvement + +- **Status:** IN_PROGRESS +- **Priority:** Medium impact / low effort +- **Addresses:** P1-P3 +- **Change:** After R1, inspect the nearby connect-response contract. Record whether a small + repeated transport helper improves both tests without hiding their causal input, real UDP Act, or + expected response. Do not add behavior merely to increase integration coverage. +- **Guardrails:** Preserve distinct unit-test ownership. A no-change decision is preferred to a + broad fixture extraction. +- **Assessment:** A narrow cleanup is justified. The adjacent connect contract repeats the complete + ephemeral tracker bootstrap that R1 moved into `start_ephemeral_udp_tracker`, proving the helper + names a coherent shared lifecycle action rather than hiding one caller's mechanics. Reuse that + helper and replace the manual client `match` branches with expectation messages. Keep the causal + `ConnectRequest`, direct client send/receive Act, expected transaction ID, and explicit tracker + shutdown visible. Do not extract a generic send/receive helper because the connect request and + response assertion are the contract's relevant behavior. +- **Prose-first review:** The temporary Arrange prose was “a running ephemeral tracker and a + loopback client have a connect request with transaction ID 123.” The final code names tracker + startup, client connection, and the transaction ID/request directly. The Act retains the real + client send/receive exchange and the Assert independently specifies the response transaction ID. + `start_ephemeral_udp_tracker` owns only coherent ordinary lifecycle setup; no generic transport + helper hides the contract. The temporary prose is redundant and removed. +- **Done when:** the next contract cleanup or no-change boundary decision is recorded. + +### R3 - Review residual integration coverage and ownership + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Change:** Measure integration-only coverage for selected production seams and compare it with + separate unit-only evidence. Record only unique loopback behavior; assign internal logic to its + existing unit boundary or lifecycle work to #1488. +- **Guardrails:** Do not use combined coverage to claim either boundary and do not add + percentage-only tests. +- **Decision:** No test added. At commit `87bf6b73`, integration-only coverage gives + `server/receiver.rs` 21/22 lines (95.45%), 29/31 regions (93.55%), and 3/3 functions (100%); + `server/processor.rs` 34/34 lines, 20/20 regions, and 7/7 functions (all 100%); and + `handlers/mod.rs` 31/31 lines, 18/18 regions, and 5/5 functions (all 100%) for their compiled + integration slices. The suite already retains unique real-loopback contracts for malformed + packets, connect, announce, scrape, high request volume, IPv6-only binding, strict-mode banning, + and disabled connection-ID validation. Unit-only evidence remains the primary proof for internal + adapters and admission decisions. The remaining receiver error/pending branches require controlled + socket readiness or I/O fault injection with no clearer user-visible contract; receive-loop and + teardown lifecycle behavior belongs to #1488. No additional integration contract is justified. +- **Done when:** the plan identifies whether another real-loopback contract has unique value. + +## Progress Tracking + +### Plan Checklist + +- [x] Existing real-loopback contracts, receiver boundary, unit-first policy, and candidate seams reviewed. +- [x] Maintainer approved R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] R2 assessment completed and proposed cleanup recorded. +- [x] Maintainer approved R2 cleanup. +- [x] R2 cleanup implemented, reviewed, validated, and committed. +- [x] R3 coverage/ownership review completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-09 - GitHub Copilot - Created this proposed plan after completing the launcher plan and + reviewing `contract.rs`, `receiver.rs`, and the issue's unit-first coverage policy. No contract + test or production change has been made. +- 2026-09-10 - User/maintainer - Approved R1. Apply the prose-first Arrange-Act-Assert cleanup to + the empty-datagram contract only; commit this plan update before modifying the integration test. +- 2026-09-10 - User/maintainer - Confirmed that a helper is justified by its meaningful name and + coherent abstraction level, not by having multiple callers. Retained + `start_ephemeral_udp_tracker` because it names a cohesive setup action and keeps the contract test + focused on real UDP behavior. +- 2026-09-10 - GitHub Copilot - Completed R2 assessment. The adjacent connect contract repeats + R1's tracker bootstrap, so `start_ephemeral_udp_tracker` is a justified shared named action. A + narrow cleanup is proposed; it retains the connect request, client exchange, expected transaction + ID, and tracker shutdown in the test rather than introducing a generic transport helper. +- 2026-09-10 - User/maintainer - Approved the R2 cleanup. Reuse the named ephemeral-tracker + setup, improve client error messages, and retain the visible connect request, UDP exchange, + transaction-ID assertion, and explicit shutdown. +- 2026-09-10 - User/maintainer - Reviewed and approved R2. The shared tracker-start helper keeps + both adjacent loopback contracts at one abstraction level; the visible connect request, transport + Act, expected transaction ID, and shutdown preserve the test's behavior-specific contract. +- 2026-09-10 - GitHub Copilot - Completed R3. Separate measurements confirm integration tests own + the real socket receive/send and packet-path slice, while unit tests own internal adapters and + admission decisions. The existing suite covers every selected loopback category; receiver + fault/pending paths lack a clearer portable user-visible contract, and lifecycle behavior belongs + to #1488. No additional integration test is added. +- 2026-09-10 - User/maintainer - Reviewed and approved the completed contract-test plan. R1/R2 + clarify the empty-datagram and connect real-loopback contracts; R3 records separate unit versus + integration coverage and the justified no-change decision for further transport expansion. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after maintainer review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server --test integration should_return_a_bad_request_response_when_the_client_sends_an_empty_request`, and `git diff --check` passed. Prose-first review retains named tracker setup, causal empty datagram, visible UDP exchange, and independent protocol-error assertion. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server --test integration receiving_a_connection_request::should_return_a_connect_response`, and `git diff --check` passed. Prose-first review retains named tracker setup, visible connect request/exchange, independent transaction-ID assertion, and explicit shutdown. | +| R3 | DONE | No change: integration-only coverage is 95.45% receiver lines and 100% compiled processor/dispatcher slices; unit tests own internal adapters/admission. Existing loopback contracts cover selected transport behavior, while receiver fault/pending and lifecycle paths lack a clearer contract or belong to #1488. | +| Plan completion | DONE | Maintainer reviewed all approved increments and evidence before the next file plan begins. | + +## Non-Goals + +- Do not replace package integration tests with unit tests or duplicate unit-owned behavior at the + wire boundary. +- Do not redesign server shutdown, receive-loop ownership, listener teardown, client timeout, or + task lifecycle; those are governed by #1488. +- Do not add sleeps, polling, uncontrolled external networking, a generic integration fixture, or + a percentage-only test. + +## Validation Per Approved Increment + +- Apply the mandatory prose-first Arrange-Act-Assert comparison before maintainer review. +- Run only the selected contract test, then the package integration target when the increment is + approved for broader validation. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Record unit-only and integration-only coverage separately whenever coverage informs a decision. + +## Completion Criteria + +- Each retained integration test has a real UDP boundary reason that makes it more appropriate or + clearer than a unit test. +- Refactors expose causal state, real transport Act, and independent expected response without + hiding them in broad helpers. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-metric-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-metric-tests.md new file mode 100644 index 000000000..541aca8fb --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-metric-tests.md @@ -0,0 +1,235 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/statistics/event/handler/error.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/statistics/event/handler/error.rs + - packages/udp-server/src/statistics/metrics.rs + - packages/udp-server/src/event.rs + - packages/udp-server/src/statistics/event/handler/mod.rs + - packages/udp-server/src/handlers/announce.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Error-Metric Handler Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to +`packages/udp-server/src/statistics/event/handler/error.rs`. + +## Phase 1 - Clean Current Tests + +### Current state + +The handler has one direct asynchronous test. It verifies that an IPv4 UDP error event increments +the aggregate IPv4 error metric, but it mixes a full inline connection context, event construction, +repository setup, event handling, and metric assertion without Arrange-Act-Assert headings or +named ordinary setup. At commit `23889a84`, unit-only coverage is 71/106 lines (66.98%), 70/173 +regions (40.46%), and 10/11 functions (90.91%). + +### Decision + +Start with a mandatory prose-first Arrange-Act-Assert cleanup of the existing general-error metric +test. Use helpers only when they name coherent ordinary event/context setup and maintain a single +abstraction level. Keep the causal error/request-kind input, direct `handle_event` Act, and one +metric assertion visible. Do not create a general metrics fixture or derive expected metric values +through production code. + +## Phase 2 - Add Missing Behavior Tests + +### Strengths to preserve + +1. `error::handle_event` owns routing one `Event::UdpError` payload into general and conditional + connection-ID metric updates. +2. `event.rs` owns conversion of internal errors into `ErrorKind`; these tests must construct the + classification directly rather than reproduce conversion behavior. +3. `statistics/event/handler/mod.rs` owns dispatch from the event enum; these tests call the + local error handler directly. +4. `statistics/metrics.rs` and `statistics/repository.rs` own metric aggregation/query behavior. +5. `torrust-peer-id` owns peer-client classification. A fixed QBitTorrent-style peer ID may select + an already-known client label, but tests must not reproduce the parser's variant matrix. + +### Problems and opportunities + +#### P1 - General-error request-kind label routing is not directly protected + +**Problem.** The existing test covers an IPv4 event without a request kind, but not the handler's +`request_kind` label insertion for parsed requests. + +**Opportunity.** Add one direct error event with `UdpRequestKind::Connect` and assert only the +general error metric query for the `connect` request-kind label. Do not duplicate event +classification or metric collection arithmetic. + +#### P2 - Announce connection-cookie errors have an untested client-software metric route + +**Problem.** The conditional branch increments the connection-ID-error counter only when a +connection-cookie error belongs to an announce request, labelling it by client software name and +version. + +**Opportunity.** Add one direct announce `UdpRequestKind` with a fixed QBitTorrent-style peer ID +and `ErrorKind::ConnectionCookie`. Assert only the connection-ID-error metric associated with its +independently specified client-software labels. Do not test non-announce suppression, peer-ID +parsing, or the general-error counter in the same test. + +#### P3 - Peer-client mapping variants are not this handler's responsibility + +**Decision.** Do not create a table for every `PeerClient` variant. The handler's metric-routing +contract needs one representative known client and can defer unknown/other classification to the +peer-ID library and a future targeted observability need. + +## Proposed Refactorings + +Apply items in order. Complete one approved increment—including prose-first comparison, focused +validation, review, and its mapped commit point—before beginning the next item. + +### R1 - Clarify the general IPv4 error metric contract + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** Phase 1 +- **Change:** Write temporary Arrange-Act-Assert prose for the existing IPv4 error metric test. + Refactor until a named ordinary IPv4 connection context, direct error-handler Act, and one + aggregate IPv4 error assertion express that prose. +- **Guardrails:** Do not add a behavior case, listener, socket, clock abstraction, or broad fixture. + Keep the independently constructed request-parse error visible. +- **Prose-first review:** The temporary Arrange prose was “an IPv4 request-parse error has no + parsed request kind and uses an empty metrics repository.” + `sample_ipv4_connection_context` names ordinary context construction, while the test retains the + direct request-parse classification. The Act now calls this file's local `error::handle_event`, + rather than the parent event router, and the Assert has one aggregate IPv4 error-metric fact. + Temporary prose is redundant and removed. +- **Done when:** redundant prose can be removed and the test has one metric assertion. + +### R2 - Cover general-error request-kind metric routing + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1 +- **Change:** Add one unit test for a connect-kind error event and assert only its general-error + metric route labelled `request_kind=connect`. +- **Guardrails:** Do not also assert aggregate IPv4/IPv6 totals, connection-ID-error metrics, event + conversion, listener dispatch, or metric arithmetic. +- **Prose-first review:** The temporary prose specified that a parsed connect request increments the + general-error metric series labelled `request_kind=connect`. The test derives ordinary connection + labels from the exact `ConnectionContext` passed to the handler, so fixture-owned labels cannot + become a duplicated expectation. It specifies only the causal `request_kind=connect` label + independently, calls the local handler directly, and asserts one labelled metric-series value. + Temporary prose is redundant and removed. +- **Done when:** a regression in request-kind label routing has one direct, deterministic failure. + +### R3 - Cover announce cookie-error client-software metric routing + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P2, P3 +- **Change:** Add one unit test with a direct `ConnectionCookie` classification and minimal announce + request using a fixed QBitTorrent-style peer ID. Assert only the client-software-labelled + connection-ID-error metric. +- **Guardrails:** Keep the selected client label/version independently specified. Do not test the + peer-ID parser, general error metric, ban counter, or event emission. +- **Prose-first review:** The temporary prose specified that a connection-cookie error for an + announce request with the visible QBitTorrent peer ID increments the connection-ID-error series + labelled `QBitTorrent` and `0.0.0`. `AnnounceRequestBuilder` supplies only incidental valid + request fields; the peer ID remains visible because it selects the handler-owned client-label + route. The test calls the local handler directly and asserts one connection-ID metric series. + Temporary prose is redundant and removed. +- **Done when:** the conditional announce-cookie route has one readable contract. + +### R4 - Review residual metric-routing coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Change:** Apply prose-first review after each test and measure unit-only coverage. Record why + unselected peer-client variants, repository failures, or metric aggregation remain at their + existing ownership boundaries. +- **Guardrails:** Do not add percentage-only cases or broaden the peer-client variant matrix. +- **Decision:** Unit-only coverage after R2 and R3 is 131/151 lines (86.75%), 191/259 regions + (73.75%), and 16/16 functions (100%). Do not add a coverage-only test for residual branches: + the `PeerClient` variant matrix is peer-ID classification behavior, while R3 protects this + handler's representative known-client route. `Repository::increase_counter` failure paths are + repository/observability infrastructure behavior and would require artificial failure injection. + Connect, announce, and scrape general-error routes share R2's request-kind label insertion; + testing other kinds would duplicate that contract. Metric aggregation and query arithmetic belong + to the repository and metric-collection test boundaries. +- **Done when:** each residual branch has an ownership decision. + +## Progress Tracking + +### Plan Checklist + +- [x] Handler, current test, metric ownership, event classification, and unit-only coverage reviewed. +- [x] Maintainer approved R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R3. +- [x] R3 implemented, reviewed, validated, and committed. +- [x] R4 coverage/ownership review completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-10 - GitHub Copilot - Created this proposed plan after reviewing the error-metric + handler, current local test, event/router ownership, metric query boundaries, representative + announce fixture support, and unit-only coverage. No test or production change has been made. +- 2026-09-10 - User/maintainer - Approved R1. Apply the prose-first cleanup to the existing IPv4 + general-error metric test only; commit this plan update before modifying the test. +- 2026-09-10 - User/maintainer - Reviewed and approved R1. The cleaned test directly exercises the + error-metric handler with a visible request-parse classification and one IPv4 aggregate error + metric assertion; ordinary connection context setup is named locally. +- 2026-09-10 - User/maintainer - Approved R2. Add one direct unit test for a connect-kind + request-parse error and assert only the general error metric labelled `request_kind=connect`. + Do not assert aggregate totals, client-software metrics, conversion, routing, or metric arithmetic. +- 2026-09-10 - User/maintainer - Reviewed and approved R2. The test derives ordinary metric labels + from its `ConnectionContext`, explicitly adds only `request_kind=connect`, directly invokes the + error-metric handler, and asserts one general-error metric series. +- 2026-09-10 - User/maintainer - Approved R3. Add one direct unit test for the announce + connection-cookie route with a visible QBitTorrent peer ID and independently specified + client-software labels only. +- 2026-09-10 - User/maintainer - Approved R4. Record the unit-only coverage evidence and retain + residual peer-client classification, repository failure, request-kind duplication, and metric + aggregation behavior at their existing ownership boundaries. +- 2026-09-10 - User/maintainer - Reviewed and approved the completed error-metric plan. The R1-R3 + tests protect distinct handler-owned routes, and R4 records the residual ownership decisions. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | DONE | Markdown and spelling checks passed after all maintainer review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server statistics::event::handler::error::tests::should_increase_the_udp4_errors_counter_when_it_receives_a_udp4_error_event`, and `git diff --check` passed. Prose-first review keeps request-parse classification, local handler Act, and one aggregate IPv4 metric assertion visible. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server statistics::event::handler::error::tests::should_label_a_general_error_metric_with_connect_request_kind`, and `git diff --check` passed. Prose-first review derives fixture-owned connection labels from the context under test and specifies only `request_kind=connect` independently. | +| R3 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server statistics::event::handler::error::tests::should_label_a_connection_id_error_metric_with_qbittorrent_client_software`, and `git diff --check` passed. Prose-first review keeps the QBitTorrent peer ID and independently specified client labels visible while `AnnounceRequestBuilder` owns incidental request setup. | +| R4 | DONE | Unit-only `cargo llvm-cov -p torrust-tracker-udp-server --all-features --lib --json` passed all 160 package unit tests. `error.rs` coverage is 131/151 lines (86.75%), 191/259 regions (73.75%), and 16/16 functions (100%). Residual branches have recorded ownership decisions; no coverage-only tests added. | +| Plan completion | DONE | Maintainer reviewed all approved increments and evidence before the next file plan begins. | + +## Non-Goals + +- Do not change event classification, listener dispatch, metrics repository behavior, peer-ID + parsing, ban policy, or production error-metric logic. +- Do not create sockets, event buses, listeners, databases, sleeps, polling, or generic fixtures. +- Do not test every client-software variant or combine general-error and connection-ID-error + assertions in one test. + +## Validation Per Approved Increment + +- Apply the mandatory prose-first Arrange-Act-Assert comparison before maintainer review. +- Run focused `statistics::event::handler::error` tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Measure unit-only coverage when coverage informs a decision. + +## Completion Criteria + +- The existing aggregate-error test has a clear causal input, direct handler Act, and one metric + assertion. +- Each new test protects exactly one handler-owned metric-routing decision. +- Event classification, peer-client parsing, metric aggregation, and event dispatch remain at their + existing ownership boundaries. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-tests.md new file mode 100644 index 000000000..65a6d844e --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/error-tests.md @@ -0,0 +1,197 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/error.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/error.rs + - packages/udp-server/src/event.rs + - packages/udp-server/src/handlers/mod.rs + - packages/udp-server/src/handlers/error.rs + - packages/udp-protocol/src/request.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Server Parse-Error Adapter Test Plan + +Follow the shared [purpose, quality goals, and plan structure](README.md). This plan applies only +to `packages/udp-server/src/error.rs`. + +## Phase 1 — Identify Problems + +### Strengths to preserve + +1. `SendableRequestParseError` preserves connection and transaction identifiers only when a + malformed UDP packet can receive an error response. +2. The adapter cleanly separates protocol parsing ownership in `udp-protocol` from UDP server + response-routing metadata. +3. `Error::from(RequestParseError)` consistently wraps the converted server representation as + `Error::InvalidRequest`. +4. `event.rs` and `handlers/error.rs` already consume this server error at their appropriate + classification and wire-response boundaries. + +### Problems and opportunities + +#### P1 — Parse-error routing metadata has no direct contract + +**Problem.** `From for SendableRequestParseError` is not tested directly. + +**Why it matters.** Losing a sendable error's connection or transaction identifier prevents the UDP +server from addressing its error response correctly. Conversely, preserving invented identifiers on +an unsendable parse error would be incorrect. + +**Opportunity.** Add two direct deterministic tests: one sendable protocol error must preserve both +identifiers and message; one unsendable protocol error must preserve its message while clearing both +optional identifiers. + +#### P2 — Outer error wrapping is not directly asserted + +**Problem.** `From for Error` is only covered incidentally through later handler +and event behavior. + +**Why it matters.** The wrapper is the explicit server boundary used by `handlers/mod.rs` before +constructing a UDP error response and event fact. + +**Opportunity.** Add one focused test that converts a sendable parse error through `Error` and +asserts its `Error::InvalidRequest` payload retains the converted identifiers and message. + +#### P3 — Display formatting is not an independent behavior target + +**Problem.** `SendableRequestParseError::fmt` is used by the error event classification, but a +separate formatting test could duplicate the event-plan request-parse test. + +**Decision.** Do not add a standalone `Display` test unless a consumer requires a distinct stable +operator-facing message. `event.rs` already verifies the resulting request-parse classification +contains the full adapter display representation. + +#### P4 — Protocol parser variants are out of scope + +**Problem.** It would be easy to use parser byte inputs to obtain source errors. + +**Why it matters.** That would duplicate `udp-protocol` parser tests and obscure the UDP server +adapter contract. + +**Opportunity.** Construct `RequestParseError::sendable_text` and `RequestParseError::unsendable_text` +directly. Use fixed protocol identifier values; do not invoke `Request::parse_bytes`. + +## Phase 2 — Proposed Refactorings + +Apply items in order. Complete one approved increment—including review, focused validation, and the +mapped commit point—before beginning the next item. + +### R1 — Cover sendable and unsendable parse-error conversion + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1, P4 +- **Change:** Add one test for `RequestParseError::sendable_text` and one test for + `RequestParseError::unsendable_text`. Assert the message and both optional identifiers explicitly. +- **Guardrails:** Keep source errors, numeric identifiers, and expected adapter values visible. + Do not parse bytes, create sockets, or add generic error fixtures. +- **Done when:** sendable errors retain both response-routing identifiers and unsendable errors have + no identifiers. + +### R2 — Cover outer invalid-request wrapping + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** P2 +- **Change:** Convert one sendable `RequestParseError` directly into `Error` and assert the + `InvalidRequest` payload preserves its message and identifiers. +- **Guardrails:** Assert the typed `Error::InvalidRequest` variant. Do not test final response + serialization or event classification here. +- **Done when:** the outer UDP server error boundary has one readable typed conversion contract. + +### R3 — Assess residual adapter coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Addresses:** P3 +- **Change:** Review remaining coverage after R1/R2. Record a no-change decision for wrapper + conversions already exercised by handlers, `ConnectionCookie` display, or display formatting + already protected at a distinct consumer boundary. +- **Guardrails:** Do not add percentage-only tests or reproduce `udp-protocol` parser matrices. +- **Decision:** No test added. Current `error.rs` coverage is 57/62 lines (91.94%), 79/82 regions + (96.34%), and 7/8 functions (87.50%) at commit `441a7512`. The direct parse-error conversion + boundary is now covered by R1 and R2. `handlers/announce.rs` and `handlers/scrape.rs` cover + wrapping UDP-core service errors at their handler boundary; `handlers/mod.rs` owns protocol parse + failure routing; `handlers/error.rs` owns wire-response and emitted-event behavior; and `event.rs` + owns stable event classification. `ConnectionCookie` display and residual wrapper conversions do + not expose a distinct UDP-server behavior beyond those boundaries. +- **Done when:** each residual direct gap has a documented ownership/boundary decision. + +## Progress Tracking + +### Plan Checklist + +- [x] Phase 1 findings reviewed against the adapter, protocol source error, and downstream consumers. +- [x] Phase 2 refactorings ordered by impact and effort. +- [x] Maintainer approved implementation of R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved implementation of R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] R3 assessment completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-08 11:31 UTC - GitHub Copilot - Created this proposed plan from `error.rs`, the direct + `RequestParseError` source variants in `packages/udp-protocol/src/request.rs`, and existing event + and handler boundaries. No test or production change has been made. +- 2026-09-08 10:35 UTC - User/maintainer - Approved R1. Construct sendable and unsendable protocol + errors directly, then assert the message and response-routing identifier presence or absence. + Commit the plan update before test implementation. +- 2026-09-08 10:45 UTC - User/maintainer - Reviewed and approved R1. The two direct adapter tests + preserve the protocol/server boundary and explicitly assert response-routing identifier retention + or absence without parsing packets or constructing handlers. +- 2026-09-08 11:13 UTC - User/maintainer - Approved R2. Convert one fixed sendable protocol error + through `Error` and assert its typed `InvalidRequest` payload retains the message and both + response-routing identifiers. Commit the plan update before test implementation. +- 2026-09-08 11:20 UTC - User/maintainer - Reviewed and approved R2. The direct typed conversion + test retains the source error, conversion Act, and `Error::InvalidRequest` message/identifier + assertions without crossing into response serialization or event classification. +- 2026-09-08 11:30 UTC - GitHub Copilot - Completed R3 assessment. The refreshed package-source + report gives `error.rs` 91.94% lines, 96.34% regions, and 87.50% function coverage. The remaining + direct paths are `ConnectionCookie` display or wrapper conversions already protected by their + handler, error-response, or event-classification boundaries; no additional adapter test is + justified. +- 2026-09-08 11:34 UTC - User/maintainer - Reviewed and approved the completed parse-error adapter + plan. R1/R2 cover the server-owned response-routing conversion and typed wrapper, while R3 records + the justified no-change decision for residual display and downstream-boundary paths. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after plan review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server error::tests`, and `git diff --check` passed. Two direct adapter tests cover sendable and unsendable parse-error conversion. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server error::tests`, and `git diff --check` passed. One typed conversion test covers `Error::InvalidRequest` wrapping. | +| R3 | DONE | No change: 91.94% lines, 96.34% regions, and 87.50% functions. Residual display/wrapper paths are covered at handler, response, or event boundaries. | +| Plan completion | DONE | Maintainer reviewed all approved increments and decisions before the next file plan begins. | + +## Non-Goals + +- Do not change UDP server errors, protocol errors, response serialization, event classification, or + event payloads. +- Do not test protocol byte parsing, connection-cookie validation, socket behavior, handler services, + event buses, or logging. +- Do not add an error builder or generic fixture; direct protocol error construction is clearer. + +## Validation Per Approved Increment + +- Run focused `error::tests`. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Review that each test keeps source error, conversion Act, and exact typed adapter result visible. + +## Completion Criteria + +- Every approved test is deterministic and adapter-focused. +- Tests preserve the boundary: `udp-protocol` owns parsing and `udp-server` owns response-routing + metadata conversion. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/event-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/event-tests.md new file mode 100644 index 000000000..ee1fa36f2 --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/event-tests.md @@ -0,0 +1,210 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/event.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/event.rs + - packages/udp-server/src/error.rs + - packages/udp-server/src/handlers/error.rs + - packages/udp-server/src/statistics/event/handler/error.rs + - docs/adrs/20260727000000_events_are_objective_facts.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Server Event Test Refactor Plan + +Follow the shared [purpose, quality goals, and plan structure](README.md). This plan applies only +to `packages/udp-server/src/event.rs`. + +## Phase 1 — Identify Problems + +### Strengths to preserve + +1. The module explains the objective-fact event policy and links + `docs/adrs/20260727000000_events_are_objective_facts.md` where a reader encounters the event + schema. +2. `ErrorKind::from(Error)` is a narrow deterministic adapter from internal/server/domain errors to + a stable event classification used by statistics and banning consumers. +3. `UdpRequestKind` keeps its wire/request data while mapping independently to stable metric labels + and display values. +4. Existing handler tests already prove selected emitted `Event::UdpError` values. Direct tests here + can verify the classification adapter without requiring socket, service, listener, or event-bus + setup. + +### Problems and opportunities + +#### P1 — Error classification has no direct behavioral tests + +**Problem.** `ErrorKind::from(Error)` has no local test module despite handling parsing, cookie, +whitelist, database, internal-server, and authentication error families. + +**Why it matters.** The mapping determines both the error facts emitted by `handlers/error.rs` and +which `Event::UdpError` values allow the statistics and banning consumers to classify a failure. +A change can silently turn a connection-cookie error into a non-cookie classification or collapse a +stable consumer-facing category. + +**Opportunity.** Add table-oriented unit cases using independently constructed source errors and +expected `ErrorKind` values. Use each test case only where it represents a distinct output category; +do not duplicate every wrapper path that maps to the same variant. + +#### P2 — Request-kind metric representations have no local contract + +**Problem.** The conversion and display implementations for `UdpRequestKind::{Connect, Announce, +Scrape}` have no direct tests. + +**Why it matters.** These values become request-kind labels in server metrics. An accidental spelling +or mapping change affects observability without necessarily breaking protocol behavior. + +**Opportunity.** Add a small table-driven unit test that independently expects `connect`, `announce`, +and `scrape` for `LabelValue` and `Display`. Construct only the minimum valid announce request +fixture required by the enum variant; do not test announce protocol parsing here. + +#### P3 — The test boundary must not duplicate adjacent ownership + +**Problem.** Error construction can tempt tests to reproduce UDP-core cookie validation, tracker-core +whitelist/database logic, or the event consumers' metric increments. + +**Why it matters.** Those tests would be slower, more coupled, and duplicate coverage at a lower or +later boundary. + +**Opportunity.** Keep all new cases synchronous and adapter-focused. Assert the exact `ErrorKind` +variant plus an independently specified stable message or message fragment. Retain +`handlers/error.rs` for emitted-event/wire-error behavior and +`statistics/event/handler/error.rs` for counter-consumption behavior. + +## Phase 2 — Proposed Refactorings + +Apply items in order. Complete one approved increment—including review, focused validation, and the +mapped commit point—before beginning the next item. + +### R1 — Cover distinct `ErrorKind` classifications + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1, P3 +- **Change:** Add direct, deterministic unit tests for one representative of each distinct output: + request parse, connection cookie, whitelist, database, internal server, and tracker + authentication. Group table cases only when their source setup remains readable and each expected + classification is visible. +- **Guardrails:** Use concrete error values and independently specified expected `ErrorKind` values. + Do not assert log text, create event-bus fixtures, invoke handlers, or re-test UDP-core/tracker-core + behavior. Keep exact expected values adjacent to their assertions, rather than placing them in + Arrange. Do not add a generic error factory with optional unrelated error families. +- **Done when:** each stable event error category has one readable adapter contract, and equivalent + announce/scrape wrapper paths are covered only where they produce a distinct classification. + +### R2 — Cover request-kind label and display mappings + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** P2 +- **Change:** Add table-driven test cases for `Connect`, `Announce`, and `Scrape` label/display + values. Use a local minimal `AnnounceRequest` fixture only for the `Announce` variant; an + inline case table must retain each concrete request-kind input and exact string value visibly. +- **Guardrails:** The announce fixture must be minimal and local. Do not derive expected labels by + calling production conversion code or make metric-repository assertions. +- **Done when:** all three request kinds have exact independently specified `LabelValue` and display + contracts. + +### R3 — Assess residual event-schema coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Addresses:** P1–P3 +- **Change:** Review uncovered lines/functions after R1 and R2 against existing handler and consumer + tests. Record a no-change decision for enum derives, sender/receiver type aliases, event-bus + aliases, or paths already covered at the event-emission/consumption boundary. +- **Guardrails:** Do not add tests solely to increase a percentage and do not modify the event schema + or ADR-defined objective-fact policy. +- **Decision:** No test added. Current `event.rs` coverage is 125/131 lines (95.42%), 139/151 + regions (92.05%), and 11/11 functions (100.00%) at commit `c8e20c19`. The remaining uncovered + regions are type/alias and enum-construction paths with no independent observable contract. + `handlers/announce.rs`, `handlers/connect.rs`, `handlers/scrape.rs`, `handlers/error.rs`, + `server/launcher.rs`, and `server/processor.rs` construct the event facts; their focused tests + cover the relevant emitted-event behavior. `statistics/event/handler/mod.rs`, + `statistics/event/handler/error.rs`, and `banning/event/handler.rs` cover consumer routing and + effects. Duplicating those boundaries in `event.rs` would test derives, aliases, or construction + mechanics rather than a distinct event-schema behavior. +- **Done when:** remaining direct coverage gaps have a documented ownership/boundary decision. + +## Progress Tracking + +### Plan Checklist + +- [x] Phase 1 findings reviewed against current source, event ADR, handlers, and consumers. +- [x] Phase 2 refactorings ordered by impact and effort. +- [x] Maintainer approved implementation of R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved implementation of R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] R3 assessment completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-08 08:13 UTC - GitHub Copilot - Created this proposed plan from `event.rs`, its direct + error sources, the existing error-event emitter/consumer tests, and + `docs/adrs/20260727000000_events_are_objective_facts.md`. No test or production change has been + made. +- 2026-09-08 08:19 UTC - User/maintainer - Approved R1 and requested repository-relative paths + whenever an ADR is mentioned in this plan. Commit the approved plan update before implementing + event-classification tests. +- 2026-09-08 08:53 UTC - User/maintainer - Approved a readability refinement for R1: keep source + error construction in Arrange and place exact expected `ErrorKind` values directly beside their + assertions. Commit the plan update before applying the test-only refactor. +- 2026-09-08 09:01 UTC - User/maintainer - Reviewed and approved R1. The six direct classification + tests retain concrete source errors in Arrange, exact expected values beside assertions, and a + stable-fragment whitelist assertion because its message includes source-location information. +- 2026-09-08 09:13 UTC - User/maintainer - Approved R2. Use a small local announce fixture and + table cases that retain the request kind and independent expected `connect`, `announce`, or + `scrape` representation visibly. Commit the plan update before test implementation. +- 2026-09-08 09:25 UTC - User/maintainer - Reviewed and approved R2. The one table-driven test + directly verifies all three request-kind label/display representations using a minimal local + announce fixture, without protocol parsing, event emission, or metric-repository setup. +- 2026-09-08 10:52 UTC - GitHub Copilot - Completed R3 assessment. The refreshed package-source + report gives `event.rs` 95.42% lines, 92.05% regions, and 100% function coverage. Residual + regions are aliases, derives, or event-construction paths already covered at the emitter or + consumer boundary; no additional event-module test is justified. +- 2026-09-08 10:56 UTC - User/maintainer - Reviewed and approved the completed event plan. R1 + covers every distinct stable error classification, R2 covers request-kind metric representations, + and R3 records the justified no-change decision for residual event-schema coverage. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after plan review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server event::tests`, and `git diff --check` passed. Six classification tests are deterministic and test-only; the public test info hash has a narrow DevSkim suppression. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server event::tests`, and `git diff --check` passed. One table-driven mapping test covers all request-kind label/display representations. | +| R3 | DONE | No change: 95.42% lines, 92.05% regions, and 100% functions. Remaining aliases, derives, and emitter/consumer construction paths have no distinct event-module contract. | +| Plan completion | DONE | Maintainer reviewed all approved increments and decisions before the next file plan begins. | + +## Non-Goals + +- Do not change event variants, error classifications, event payloads, or the objective-fact policy. +- Do not duplicate `handlers/error.rs` response/event-emission tests, error-counter consumer tests, + UDP-core cookie validation, tracker-core whitelist/database behavior, or UDP-protocol parsing. +- Do not add listener, socket, database, clock, or random-data setup for this adapter-level work. +- Do not create a broad cross-module error fixture or builder. + +## Validation Per Approved Increment + +- Run focused `event` unit tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Review that each test's source error and independently specified `ErrorKind` expectation remain + visible before beginning the next increment. + +## Completion Criteria + +- Every approved test is deterministic and adapter-focused. +- Error classifications preserve the objective-fact event contract without duplicating lower-layer + error behavior or later event-consumer behavior. +- Metric label/display tests specify expected values independently of the production conversion. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/handler-dispatch-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/handler-dispatch-tests.md new file mode 100644 index 000000000..eebc192cd --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/handler-dispatch-tests.md @@ -0,0 +1,242 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/handlers/mod.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/handlers/mod.rs + - packages/udp-server/src/handlers/error.rs + - packages/udp-server/src/server/processor.rs + - packages/udp-server/src/error.rs + - packages/udp-protocol/src/request.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Handler Dispatch Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to `packages/udp-server/src/handlers/mod.rs`. + +## Phase 1 - Clean Current Tests + +### Current state + +`handlers/mod.rs` has no direct test cases. Its `#[cfg(test)]` module provides narrowly scoped +service construction, sample network values, and mock sender types used by the individual handler +modules. The test-bearing handler modules already exercise their own business rules at the service +boundary. + +### Decision + +No cleanup increment is proposed. Moving or generalizing the existing support would create a +cross-module fixture change without an observed readability, duplication, or determinism problem. +The proposed direct tests must use only the smallest existing support required by the orchestration +boundary; they must not turn this support module into a generic server fixture. + +## Phase 2 - Add Missing Behavior Tests + +### Strengths to preserve + +1. `handle_packet` owns the package boundary between raw datagrams, protocol parsing, request + dispatch, and error response routing. +2. `handle_request` dispatches `Connect`, `Announce`, and `Scrape` requests, while each concrete + handler owns its respective tracker behavior. +3. `error.rs` directly covers parse-error metadata conversion and `handlers/error.rs` directly + covers error-response serialization and error-event emission. +4. `server/processor.rs` owns the source-port-zero defensive guard before it delegates to + `handle_packet`. + +### Problems and opportunities + +#### P1 - Parse-failure routing has no direct orchestration contract + +**Problem.** No test calls `handle_packet` with a malformed raw payload. The direct adapter and +error-response tests prove their individual behavior, but neither proves that the dispatcher +preserves a sendable parse error's transaction identifier while reporting that no request kind was +parsed. + +**Why it matters.** A refactor can accidentally discard the transaction identifier before error +routing, or report an invented request kind to the caller. Either regression breaks the UDP server +response/metrics boundary without being a protocol-parser or error-serializer defect. + +**Opportunity.** Construct one minimal malformed payload that produces a sendable +`RequestParseError` with a fixed transaction identifier. Call `handle_packet` with deterministic +containers and assert an error response carries that identifier and the returned request kind is +`None`. + +#### P2 - Handler-error routing is adjacent but risks duplicating handler behavior + +**Problem.** The successful-parse / failed-handler branch is untested directly here. + +**Why it matters.** This branch must preserve the parsed request kind for the caller while routing +the handler's error through `handle_error`. + +**Opportunity.** Assess whether an existing deterministic invalid request can exercise this branch +without testing connection-cookie validation, whitelist policy, database behavior, or final +error-event serialization. Add a test only if its fixture makes the dispatch/routing distinction +clearer than the existing handler and error tests. + +#### P3 - Success dispatch belongs primarily to individual handlers + +**Decision.** Do not add connect, announce, or scrape success-dispatch matrices. The individual +handler tests own those behavioral outcomes, and a dispatcher matrix would only repeat their +service setup and protocol response assertions. + +## Proposed Refactorings + +Apply items in order. Complete one approved increment, including its review and focused validation, +before beginning the next item. + +### R1 - Record the Phase 1 no-change decision + +- **Status:** TODO +- **Priority:** High impact / trivial effort +- **Change:** Confirm that `handlers/mod.rs` has no direct tests to clean and that its existing + support remains local to the handler modules. +- **Done when:** Phase 1 is explicitly complete without a cross-module fixture refactor. + +### R2 - Cover sendable parse-failure routing + +- **Status:** TODO +- **Priority:** High impact / low effort +- **Addresses:** P1 +- **Change:** Add one direct asynchronous `handle_packet` test using a fixed malformed payload with + a sendable transaction identifier. Assert the independently specified `Response::Error` + transaction identifier and `None` request-kind result. +- **Guardrails:** Construct no parser matrix and do not call `Error::from` to derive expected + metadata. Use disabled/non-listening event infrastructure unless the exact dispatch contract + requires observing an event. Do not assert error response text, logging, latency, UUIDs, or + handler service effects. +- **Done when:** the raw-payload to error-response-routing contract is protected while protocol + parsing and final error serialization remain owned by their existing tests. + +### R3 - Assess failed-handler routing without duplicate business behavior + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** P2, P3 +- **Change:** Review the parsed-request error branch after R2. Record a no-change decision unless + one existing deterministic request produces a handler error with a visible request-kind routing + distinction and no duplicated handler-policy assertion. +- **Guardrails:** Do not introduce mocks or production dependency injection solely for this test. + Do not use clocks, retries, sockets, databases, or lifecycle fixtures beyond existing minimal + test support. +- **Decision:** No test added. `handle_announce` and `handle_scrape` construct the + `(Error, TransactionId, UdpRequestKind)` tuple from their parsed request at the handler boundary. + `handle_error` directly verifies that a supplied transaction ID and request kind become the error + response/event routing result. The real-loopback contract suite exercises invalid-cookie request + behavior at the outer boundary. A direct `handle_packet` failure case would need to configure an + invalid cookie, whitelist, or other tracker policy merely to reproduce that tuple and call + `handle_error`; its assertions would duplicate the handler cause or the direct error-routing + contract rather than reveal a distinct dispatcher behavior. +- **Done when:** the branch either has one distinct dispatcher contract or a documented reason it + remains protected at the handler/error boundaries. + +### R4 - Review Phase 2 test design and residual coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Change:** After each added test, review Arrange-Act-Assert visibility, fixture scope, and + ownership. Measure residual coverage only to decide whether another distinct orchestration + behavior exists. +- **Guardrails:** Do not add percentage-only tests or change production dispatch behavior. +- **Decision:** No test added. At commit `9eb74c23`, the unit-only report gives `handlers/mod.rs` + 184/214 lines (85.98%), 224/249 regions (89.96%), and 32/37 functions (86.49%), with no + uncovered executable source-line entries. The integration-only report gives 31/31 lines (100%), + 18/18 regions (100%), and 5/5 functions (100%) for its smaller compiled production slice; the + combined report is navigation-only and cannot attribute coverage to either test level. R2 is the + appropriate primary unit boundary because its prose-first Arrange-Act-Assert comparison makes the + causal raw packet, dispatcher Act, returned request kind, and response transaction ID readable + without transport lifecycle mechanics. R3 assigns failed-handler routing to its handler, + error-routing, and loopback boundaries. No residual direct dispatcher contract justifies another + test. +- **Done when:** every remaining gap is assigned to the dispatcher, a concrete handler, the + protocol parser, error serializer, or processor boundary. + +## Progress Tracking + +### Plan Checklist + +- [x] Phase 1 and Phase 2 boundaries reviewed against current handler, error, parser, and processor tests. +- [x] Maintainer approved R1. +- [x] R1 no-change decision recorded and committed. +- [x] Maintainer approved R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] R3 assessment completed and decision recorded. +- [x] R4 design/coverage review completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-08 16:32 UTC - GitHub Copilot - Created this proposed two-phase plan after reviewing + `handlers/mod.rs`, its current shared test support, the processor guard tests, direct + parse-error adapter tests, and direct error-response tests. No test or production change has + been made. +- 2026-09-09 - User/maintainer - Approved R1's Phase 1 no-change decision and R2's focused + sendable parse-failure routing test. Commit this approval record and the proposed plan before + implementing the test. +- 2026-09-09 - User/maintainer - Reviewed and approved R2 after its Arrange section was reduced + to `SendableParseErrorPacketScenario`. The scenario names the causal sendable-parse-error state + and the test keeps `handle_packet` plus its independently specified transaction-ID/request-kind + contract visible. +- 2026-09-09 - User/maintainer - Requested a further simplification because the scenario still + moved complexity rather than making the initial state directly readable. Applied a prose-first + Arrange-Act-Assert loop: temporarily state each section in normal prose, then refactor until the + code expresses that prose and remove redundant comments. The final test separates ordinary + `initialize_udp_handler_environment` mechanics from the causal + `scrape_request_without_info_hashes(transaction_id)` input; its transaction ID, dispatcher Act, + and expected outputs remain directly visible. +- 2026-09-09 - GitHub Copilot - Completed R3 assessment. No failed-handler routing test is added: + handler tests own construction of error/request-kind metadata, `handlers/error.rs` owns its + routing to a response/event, and real-loopback contracts own invalid-cookie behavior. A direct + dispatcher case would duplicate one of those boundaries to reach the same call. +- 2026-09-09 - GitHub Copilot - Completed R4. The package-source measurement gives + `handlers/mod.rs` 85.98% unit-only lines, 89.96% unit-only regions, and 86.49% unit-only + functions, with no uncovered executable source-line entries. The separate integration-only report + covers a smaller production slice and is not used to claim unit coverage. The R2 prose-first + comparison confirms code now expresses the causal input, ordinary environment, dispatcher Act, + and independent assertions; R3 owns the only remaining routing assessment. No further direct + dispatcher test is justified. +- 2026-09-09 - User/maintainer - Reviewed and approved the completed handler-dispatch plan. R1 + records the no-cleanup decision; R2 adds the unit-first sendable parse-error routing contract; + R3/R4 record the no-duplication and separate test-level coverage decisions. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after maintainer review changes. | +| R1 | DONE | No change: `handlers/mod.rs` has no direct test cases to clean. Its existing local support remains focused on individual handler modules, so a cross-module fixture refactor is not justified. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server handlers::tests::it_should_preserve_the_transaction_id_for_a_sendable_parse_error_without_a_request_kind`, and `git diff --check` passed. A prose-first Arrange-Act-Assert comparison replaced the scenario with named ordinary-environment and causal-empty-scrape helpers; the transaction ID, dispatcher Act, and expected outputs remain visible. | +| R3 | DONE | No change: handler-error metadata is created and covered at the announce/scrape boundary, `handlers/error.rs` directly covers supplied error routing, and real-loopback contracts cover invalid-cookie behavior. A `handle_packet` failure test would duplicate one of those boundaries. | +| R4 | DONE | No change: unit-only coverage is 85.98% lines, 89.96% regions, and 86.49% functions, with no uncovered executable source-line entries. The separate integration-only report is not used to claim unit coverage. The prose-first review confirms R2 expresses its intent; R3 assigns failed-handler routing to its established boundaries. | +| Plan completion | DONE | Maintainer reviewed all approved increments and evidence before the next file plan begins. | + +## Non-Goals + +- Do not change dispatcher, parser, handler, error-response, event, or processor production behavior. +- Do not duplicate `udp-protocol` parsing matrices, concrete handler business rules, or + `handlers/error.rs` error serialization/event tests. +- Do not create a generic test container, mock a concrete handler, or introduce a socket, retry, + sleep, clock, database, or shutdown-lifecycle test. + +## Validation Per Approved Increment + +- Run focused handler-dispatch tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- After each Phase 2 behavior test, review causal state, visible Act, independently specified + expected result, and ownership before the next increment. + +## Completion Criteria + +- Phase 1 makes an explicit no-change or cleanup decision for existing target-file test code. +- Each approved direct test protects a unique raw-packet dispatch contract. +- Individual handler, protocol parser, error serializer, and processor contracts remain at their + existing ownership boundaries. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/launcher-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/launcher-tests.md new file mode 100644 index 000000000..9ab238eeb --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/launcher-tests.md @@ -0,0 +1,298 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/server/launcher.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/server/launcher.rs + - packages/udp-server/src/server/processor.rs + - packages/udp-server/src/server/request_buffer.rs + - packages/udp-server/tests/server/contract.rs + - docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md + - docs/issues/drafts/1488-si-14-migrate-udp-receive-reset-token-lifecycle/ISSUE.md + - docs/issues/drafts/1488-si-15-define-udp-active-request-policy/ISSUE.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md +--- + +# UDP Launcher Test Refactor Plan + +Follow the shared [purpose, quality goals, plan structure, and required two-phase +sequence](README.md). This plan applies only to `packages/udp-server/src/server/launcher.rs`. + +## Phase 1 - Clean Current Tests + +### Current state + +`launcher.rs` has one direct test: the startup-notification receiver is dropped, so +`run_with_graceful_shutdown` must return `BrokenPipe` and release its socket. The test protects a +valuable failure cleanup contract, but its Arrange block manually composes configuration, clocks, +logging, UDP-core services, server services, a bound socket, and two oneshot channels. The causal +state—the startup receiver is absent—is difficult to see among ordinary infrastructure. + +The separate coverage reports at commit `81f5edbc` show 72/135 lines (53.33%), 83/144 regions +(57.64%), and 7/13 functions (53.85%) for unit-only `--lib`; integration-only execution gives +68/91 lines (74.73%), 46/75 regions (61.33%), and 9/11 functions (81.82%) for its smaller +production-only slice. Neither report identifies an uncovered source line through the generic +line-entry data, so the measurements are navigation evidence, not a reason to force tests into +lifecycle-owned branches. + +### Decision + +Start with a mandatory prose-first Arrange-Act-Assert comparison of the existing test. Its temporary +prose must distinguish ordinary valid launcher dependencies from the causal dropped startup receiver +and the independently observed socket address. Refactor only to make those concepts visible. A +focused scenario fixture may own ordinary launcher construction and the dropped receiver condition, +but it must not run the launcher, receive its outcome, or assert socket release. + +## Phase 2 - Add Missing Behavior Tests + +### Strengths to preserve + +1. `run_with_graceful_shutdown` owns startup notification and releases the listener when startup + reporting fails. +2. `should_discard_request` owns deterministic pre-processing admission decisions for source port + zero and currently banned source IPs. +3. `server/processor.rs` already protects source-port-zero defense in depth, while + `statistics/event/handler` modules own the corresponding counter effects. +4. The #1488 shutdown EPIC and SI-14/SI-15 own receive-loop cancellation, child-task joining, + request-abort behavior, and active-request shutdown policy. + +### Problems and opportunities + +#### P1 - Startup-receiver failure setup is harder to read than the contract + +**Problem.** The one existing test makes readers reconstruct the causal dropped-receiver state from +the last lines of a long setup sequence. + +**Opportunity.** Apply the Phase 1 prose-first refactor before considering any behavior additions. + +#### P2 - Admission decisions may have direct deterministic unit seams + +**Problem.** The source-port-zero and banned-IP paths are package-owned decisions before processing, +but direct evidence at this boundary is limited. + +**Opportunity.** After Phase 1, assess one direct `should_discard_request` contract at a time only +if it can observe the Boolean admission decision and its immediate event without starting a receive +loop, spawning request tasks, using sleeps/polling, or duplicating processor/statistics tests. + +#### P3 - Lifecycle and active-request behavior is not owned by this issue + +**Decision.** Do not add tests for receive-loop completion, `None`/I/O receiver outcomes, spawned +request-task lifecycle, shutdown aborts, task joining, or active-request eviction. These are owned +by #1488 SI-14 and SI-15 and require their approved cancellation and deadline policy. + +## Proposed Refactorings + +Apply items in order. Complete one approved increment—including prose-first comparison, focused +validation, review, and its mapped commit point—before beginning the next item. + +### R1 - Express startup-receiver failure causally + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1 +- **Change:** Write temporary prose for the existing test's Arrange, Act, and Assert sections. Then + refactor its setup until the code visibly states a valid launcher with a dropped startup receiver, + the `run_with_graceful_shutdown` Act, and the independent `BrokenPipe`/rebind assertions. +- **Guardrails:** Keep the launcher call and both observable assertions in the test body. Do not + generalize a fixture for future shutdown cases or change production lifecycle behavior. +- **Prose-first review:** The temporary Arrange prose was “a valid UDP launcher has a dropped + startup-notification receiver.” `UdpLauncherDependencies::new()` now names ordinary valid + construction, while the test visibly creates and drops only the startup receiver. The Act remains + the direct `run_with_graceful_shutdown` call with strict validation, and the Assert retains + independent `BrokenPipe` and rebind results. The temporary prose is redundant and removed. +- **Done when:** redundant prose can be removed because names and structure express the causal + state and contract. + +### R2 - Assess source-port-zero admission at the launcher boundary + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** P2 +- **Change:** Determine whether a direct test can call `should_discard_request` with a source-port- + zero raw request and observe only its Boolean decision plus immediate `UdpRequestDiscarded` fact. + Add one unit test only if it adds a clearer contract than `Processor::process_request` and the + existing statistics handler tests. +- **Guardrails:** Do not start `run_udp_server_main`, receive real UDP traffic, spawn tasks, use a + listener, sleep, poll, or assert later counter consumption. Do not test source-port-zero wire + transport, which standard sockets cannot produce. +- **Prose-first review:** The temporary Arrange prose was “a valid launcher evaluates a request + whose source port is zero.” The final code makes the port-zero client address and raw request + visible, while `UdpLauncherDependencies`, `sample_udp_service_binding`, and `TEST_LOG_TARGET` + own ordinary setup. The direct `should_discard_request` Act and strict-policy input remain + visible. The Assert independently specifies both discard decision and exact immediate event; + only the event-await comment remains because its deadline failure-bound rationale is not evident + from syntax alone. The temporary prose is redundant and removed. +- **Done when:** the admission seam has either one unique direct contract or a documented + no-change decision assigning it to processor/statistics boundaries. + +### R3 - Assess banned-IP admission at the launcher boundary + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** P2, P3 +- **Change:** Determine whether one deterministic unit test can seed a banned IP, call + `should_discard_request`, and assert only the Boolean decision plus immediate `UdpRequestBanned` + fact. Add it only if it does not duplicate ban-service policy or listener counter behavior. +- **Guardrails:** Keep validation-policy choice visible. Do not cover ban threshold accumulation, + receive-loop lifecycle, or disabled-mode tracker behavior unless the direct admission choice is + uniquely obscured elsewhere. +- **Prose-first review:** The temporary Arrange prose was “a strict launcher receives a nonzero- + port request from an already-banned IP.” The final `ban_client_ip` setup operation expresses the + causal state while deriving its counter increments from the configured threshold; it does not + assert or test the UDP-core ban algorithm. The direct strict-policy admission Act and the + independent discard/exact-event assertions remain visible. The shared event-publication deadline + retains its concise failure-bound rationale; the temporary prose is redundant and removed. +- **Done when:** the strict-mode admission choice has a unique direct contract or a documented + no-change ownership decision. + +### R3a - Split admission decision and event contracts + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** R2/R3 assertion specificity +- **Change:** Refactor each admission condition into two tests with one observable reason to fail: + one asserts only `should_discard_request`'s Boolean decision; the other asserts only its immediate + event. Implement and review the source-port-zero pair first. Assess the already-banned-IP pair + only after that review. +- **Guardrails:** Each test must retain the direct `should_discard_request` Act. Decision tests do + not subscribe to or assert events. Event tests do not assert the Boolean decision. Preserve direct + event-bus observation and its absolute deadline in event tests. Do not change production behavior, + start a receive loop, or duplicate processor/statistics behavior. +- **Prose-first review:** The source-port-zero and banned-IP tests initially combined their decision + and event assertions, giving each two unrelated failure causes. The final four test names state + either `require_discarding` or `publish` and retain one assertion accordingly. Each Arrange keeps + its causal source-port-zero or `with_banned_client_ip` state visible; each Act is the direct + `should_discard_request` call. Event tests retain only the bounded direct event receive, while + decision tests do not subscribe. The temporary prose is redundant and removed. +- **Done when:** a failing decision assertion identifies admission-policy behavior, and a failing + event assertion identifies immediate observability behavior without conflating the two. + +### R4 - Review design and residual test-level coverage + +- **Status:** DONE +- **Priority:** Low impact / low effort +- **Change:** After each approved test, apply and record prose-first AAA verification. Measure + unit-only and integration-only coverage separately; assign every remaining relevant branch to the + launcher, processor, request buffer, integration contract, or #1488 shutdown work. +- **Guardrails:** Do not use combined coverage to claim either boundary, and do not add + percentage-only tests. +- **Decision:** No test added. At commit `2f7643ae`, `launcher.rs` unit-only coverage is 278/290 + lines (95.86%), 274/297 regions (92.26%), and 24/26 functions (92.31%); the report has no + uncovered executable line or region entries. The integration-only report gives 68/91 lines + (74.73%), 46/75 regions (61.33%), and 9/11 functions (81.82%) for its smaller production-only + slice, so it is not used to claim unit coverage. R1 and R3a give each test one visible causal + state, direct Act, and single observable assertion. Remaining receive-loop completion, receiver + I/O, spawned task lifecycle, request-buffer eviction, and shutdown cancellation/join behavior + belong to #1488 SI-14/SI-15; no further launcher test is justified in this issue. +- **Done when:** remaining lifecycle-sensitive gaps have explicit ownership and all approved tests + are readable, deterministic, and unit-first where appropriate. + +## Progress Tracking + +### Plan Checklist + +- [x] Existing launcher test, admission branches, separate coverage, and #1488 ownership reviewed. +- [x] Maintainer approved R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R2. +- [x] R2 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R3. +- [x] R3 implemented, reviewed, validated, and committed. +- [x] Maintainer approved R3a source-port-zero split. +- [x] R3a source-port-zero split implemented, reviewed, validated, and committed. +- [x] Maintainer approved R3a banned-IP split. +- [x] R3a banned-IP split implemented, reviewed, validated, and committed. +- [x] R4 design/coverage review completed and decision recorded. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-09 - GitHub Copilot - Created this proposed two-phase plan after reviewing the existing + launcher test, `should_discard_request`, processor and request-buffer boundaries, separate + unit-only/integration-only coverage, and #1488 shutdown ownership. No test or production change + has been made. +- 2026-09-09 - User/maintainer - Reviewed and approved R1. The final test retains the direct + launcher Act and observable failure/rebind assertions, while `UdpLauncherDependencies` owns only + ordinary construction and the visible dropped startup receiver identifies the causal state. +- 2026-09-09 - User/maintainer - Approved R2. Add one direct unit test proving the launcher + rejects a source-port-zero raw request and immediately emits `UdpRequestDiscarded`, without + starting the receive loop, spawning a processor, or asserting later metrics consumption. +- 2026-09-09 - User/maintainer - Reviewed and approved R2. The unit-first direct admission test + makes the source-port-zero causal state, strict policy, dispatcher-independent Act, and exact + immediate event visible; helpers hide only tracing/server metadata and ordinary dependencies. +- 2026-09-09 - User/maintainer - Approved R3. Add one direct strict-mode unit test for an + already-banned nonzero-port client IP. Seed the existing ban service past its configured limit, + then assert only the launcher discard decision and exact immediate `UdpRequestBanned` event. +- 2026-09-09 - User/maintainer - Reviewed and approved R3. The named banned-client setup, visible + strict-policy admission Act, and exact immediate event retain the launcher boundary without + duplicating UDP-core threshold behavior or integration-level network handling. +- 2026-09-09 - User/maintainer - Identified that the R2/R3 tests assert both admission decision + and event publication, giving each test two unrelated reasons to fail. R3a splits each condition + into a decision contract and an immediate-event contract, starting with the source-port-zero pair. +- 2026-09-09 - User/maintainer - Approved the R3a source-port-zero split. Commit this plan update + before replacing the combined test with separate decision and event contracts. +- 2026-09-09 - User/maintainer - Reviewed and approved the source-port-zero split: one test asserts + only the discard decision and the other only the immediate discard event. Also approved applying + the same single-fact split to the banned-IP admission test. +- 2026-09-09 - User/maintainer - Reviewed and approved R3a. Both admission conditions now have a + decision-only contract and an event-only contract, preserving one behavioral reason to fail per + test without receive-loop, processor, or metrics-listener setup. +- 2026-09-09 - GitHub Copilot - Completed R4. Separate measurements give 95.86% unit-only line + coverage and 92.26% unit-only region coverage for `launcher.rs`, with no uncovered executable + line or region entries. Integration coverage is recorded separately and has a different smaller + denominator. The remaining lifecycle-sensitive paths belong to #1488 SI-14/SI-15, so no further + launcher test is added. +- 2026-09-09 - User/maintainer - Reviewed and approved the completed launcher plan. R1 makes the + startup-receiver failure state visible; R2/R3 cover immediate source-port-zero and strict banned- + IP admission; R3a gives each decision/event fact its own test; R4 records separate test-level + coverage and lifecycle ownership decisions. +- 2026-09-09 - User/maintainer - Approved the final naming refinement. The test context is named + `UdpLauncherTestContext`, its variable is `launcher`, and + `with_banned_client_ip(client_socket_addr.ip())` states the causal already-banned-client state + directly in Arrange. The shared event-publication deadline records its failure-bound rationale. + +### Validation Evidence + +| Increment | Status | Evidence | +| --- | --- | --- | +| Plan documentation | TODO | Run Markdown and spelling checks after maintainer review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server launcher::tests::it_should_release_the_socket_when_the_startup_notification_receiver_is_dropped`, and `git diff --check` passed. The prose-first review separates ordinary launcher construction from the visible dropped receiver state. | +| R2 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server launcher::tests::it_should_discard_a_request_when_its_source_port_is_zero`, and `git diff --check` passed. The prose-first review retains the direct admission Act, causal source port, strict policy, and exact immediate event. | +| R3 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server launcher::tests::it_should_discard_a_request`, and `git diff --check` passed. The prose-first review uses `UdpLauncherTestContext::with_banned_client_ip` to keep the causal state, strict Act, and independent discard/event assertions visible. | +| R3a | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server launcher::tests`, and `git diff --check` passed. Source-port-zero and banned-IP behavior are each split into one decision-only and one event-only test after prose-first review. | +| R4 | DONE | No change: unit-only coverage is 95.86% lines, 92.26% regions, and 92.31% functions, with no uncovered executable line or region entries. The integration-only report has a separate smaller production-only denominator. Remaining lifecycle paths belong to #1488 SI-14/SI-15. | +| Plan completion | DONE | Maintainer reviewed all approved increments and evidence before the next file plan begins. | + +## Non-Goals + +- Do not change UDP launcher, admission, request-buffer, processor, or shutdown production behavior. +- Do not test receive-loop termination, task lifecycle, cancellation, joining, request draining, or + active-request shutdown policy owned by #1488 SI-14/SI-15. +- Do not add a real-loopback integration test when a deterministic unit contract can express the + selected behavior more directly. +- Do not duplicate ban-service policy, processor source-port-zero defense, event-listener counter + consumption, or protocol transport constraints. + +## Validation Per Approved Increment + +- Apply the mandatory prose-first Arrange-Act-Assert comparison before maintainer review. +- Run focused launcher tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- Record unit-only and integration-only coverage separately when coverage informs a decision. + +## Completion Criteria + +- The existing startup-receiver failure test expresses its causal state without obscuring the Act or + independent assertions. +- Any new admission test is deterministic, unit-first, and protects a unique immediate launcher + decision. +- Lifecycle-sensitive gaps remain assigned to #1488 until its cancellation and active-request + policies are implemented. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/request-buffer-tests.md b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/request-buffer-tests.md new file mode 100644 index 000000000..614b5aa0e --- /dev/null +++ b/docs/issues/open/2149-1347-add-focused-udp-server-package-tests/test-refactor-plans/request-buffer-tests.md @@ -0,0 +1,366 @@ +--- +doc-type: test-refactor-plan +issue: 2149 +package: torrust-tracker-udp-server +target-file: packages/udp-server/src/server/request_buffer.rs +status: completed +semantic-links: + related-artifacts: + - packages/udp-server/src/server/request_buffer.rs + - packages/udp-server/src/server/launcher.rs + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/ISSUE.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/coverage-evidence.md + - docs/issues/open/2149-1347-add-focused-udp-server-package-tests/performance-evidence.md + - packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md + - docs/issues/drafts/1488-si-15-define-udp-active-request-policy/ISSUE.md +--- + +# UDP Request Buffer Test Refactor Plan + +Follow the shared [purpose, quality goals, and plan structure](README.md). This plan applies only +to `packages/udp-server/src/server/request_buffer.rs`. + +## Phase 1 — Identify Problems + +### Strengths to preserve + +1. `ActiveRequests::force_push` has a documented normal-operation overload policy: retain up to + 50 processor-task abort handles, reclaim completed handles encountered before the first + still-active task, otherwise abort that oldest active task to make space. +2. `Drop` explicitly aborts remaining unfinished processor tasks, avoiding detached work when the + normal-operation buffer is released. +3. The implementation retains single-owner buffer invariants and does not use shared mutable + state. + +### Problems and opportunities + +#### P1 — Normal capacity behavior has no direct contract + +**Problem.** No test proves that inserting a pending task while capacity is available retains the +task and returns `false`. + +**Why it matters.** `Launcher::run_udp_server_main` uses the return value to decide whether to +publish `UdpRequestAborted`. A regression could emit an abort fact without an eviction. + +**Opportunity.** Create a pending task with a deterministic synchronization channel, insert its +abort handle, and assert no eviction occurred while preserving the production buffer behavior. + +#### P2 — Oldest-first bounded eviction is unprotected + +**Problem.** The full-buffer path has no test for its intentional oldest-first decision: it does +not scan newer completed handles before evicting the first oldest task that remains active after a +scheduler yield. + +**Why it matters.** A future refactor could mistake this intentional performance trade-off for a +bug, introduce a slower full-buffer scan, or change the eviction/event result without review. + +**Opportunity.** Fill the buffer with one oldest pending task followed by completed handles. Insert +a new pending task and assert that the oldest task is evicted and `force_push` reports the eviction. + +#### P3 — Active-task eviction is unprotected + +**Problem.** When all tracked handles remain active, `force_push` yields once and aborts the oldest +observed unfinished task. No test proves the eviction or its `true` result. + +**Why it matters.** This is the buffer's material overload behavior and the only condition that +causes the launcher to publish an aborted-request fact. + +**Opportunity.** Fill the buffer with pending tasks controlled by deterministic cancellation +observers, push one additional pending task, then verify the selected oldest handle was aborted +and the other tracked work remains active. + +#### P4 — Drop cleanup is unprotected + +**Problem.** `Drop::drop` aborts unfinished handles and skips finished handles, but no regression +test protects that distinction. + +**Why it matters.** Leaving pending processor tasks alive after the normal-operation buffer drops +would leak work; aborting an already finished task is unnecessary but harmless. + +**Opportunity.** Drop a buffer containing one confirmed completed task and one pending task, then +assert the pending task observes cancellation without relying on elapsed time. + +#### P5 — Scheduler coupling must remain constrained + +**Problem.** The implementation calls `tokio::task::yield_now()` before deciding an old task is +still unfinished. + +**Why it matters.** Tests based on sleeps, polling, or task scheduling order would be flaky and +would make an implementation detail look like a shutdown contract. + +**Opportunity.** Use channels and bounded awaits solely to establish task completion or abort +observation. Do not specify drain, deadline, join, or shutdown behavior. + +## Phase 2 — Proposed Refactorings + +Apply items in order. Complete one approved increment—including review, focused validation, and the +mapped commit point—before beginning the next item. + +### R1 — Cover insertion while capacity is available + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** P1 +- **Change:** Add one deterministic unit test that inserts a pending task handle into an empty + `ActiveRequests` buffer and asserts `force_push` returns `false`. +- **Guardrails:** The task must remain pending until the test cleans it up. Do not inspect private + ring-buffer internals or add production observation APIs. If a production change becomes + necessary, stop this increment and follow the baseline policy in + [performance-evidence.md](../performance-evidence.md) before changing the hot path. +- **Done when:** the test names the capacity-available causal state and proves no task was evicted. + +### R2 — Assess and document oldest-first bounded eviction + +- **Status:** DONE +- **Priority:** High impact / medium effort +- **Addresses:** P2, P5 +- **Change:** Use the deterministic oldest-pending/later-completed scenario to assess the current + behavior. Document its historical performance rationale in a package-local ADR and clarify the + production comments. Defer a behavior test until the ADR and source wording receive maintainer + review. +- **Guardrails:** Do not reinterpret the historic comment as a full-buffer reclamation guarantee. + Do not change hot-path production behavior or add a benchmark for documentation-only work. +- **Decision:** The current oldest-first behavior is intentional. PR #921 documents the starvation + concern and one-yield opportunity; PR #922 records that a refactor separating removal from + cleaning all completed tasks regressed performance. The initial failing R2 test asserted the + rejected full-scan alternative, not a production defect. +- **Done when:** the ADR and production comment clarify the policy, and the unsupported bug handoff + and failing test evidence are removed. + +### R3 — Cover active-task eviction at capacity + +- **Status:** DONE +- **Priority:** High impact / medium effort +- **Addresses:** P3, P5 +- **Change:** Add a deterministic full-buffer test in which every tracked task remains pending; + insert one more pending task, assert `force_push` returns `true`, and observe cancellation of the + oldest selected task. Use a file-local `FullBufferWithPendingTasks` scenario fixture so the + Arrange section names the causal full-buffer state while the test retains the visible `force_push` + Act and eviction assertion. +- **Guardrails:** Assert only the normal-operation eviction contract. Do not establish a task + drain, deadline, join, shutdown metric, or graceful-shutdown policy. The fixture may create and + clean up tasks, but it must not call `force_push`, decide the expected result, or hide the + eviction assertion. Keep it specialized to this full-pending-buffer scenario; do not generalize + it into a builder or shared test factory. +- **Done when:** the test demonstrates exactly one required capacity eviction, names the full + pending-buffer state in Arrange, and keeps the Act and eviction assertion visible. + +### R3a — Clarify full-buffer scenario construction + +- **Status:** DONE +- **Priority:** Medium impact / low effort +- **Addresses:** R3 Arrange readability +- **Change:** Add a file-local `PendingTask::insert_into` helper that creates a pending task, + inserts its abort handle into the scenario buffer, and returns the task for deterministic + cleanup. Rename `new_task` to `incoming_task` because it represents the request arriving after + capacity is exhausted. +- **Guardrails:** The helper owns only Arrange mechanics and must not invoke `force_push`, decide + an expected result, or assert eviction behavior. Keep it private to this module; do not create a + general builder or shared test factory. +- **Done when:** `FullBufferWithPendingTasks::new` visibly constructs the oldest task, the + remaining 49 tasks, and the incoming task without duplicating buffer-insertion mechanics. + +### R4 — Cover drop cleanup for active work + +- **Status:** DONE +- **Priority:** Medium impact / medium effort +- **Addresses:** P4, P5 +- **Change:** Add a deterministic test that drops a buffer containing a completed and a pending + task handle, then observes pending-task cancellation. Keep the completed task inline because its + only causal role is to establish mixed buffer state; use the local `PendingTask` helper for the + pending task's controlled lifetime and cancellation assertion. +- **Guardrails:** Do not use this test to define server shutdown behavior. `ActiveRequests` is a + normal-operation capacity buffer; shutdown task policy belongs to SI-15. +- **Done when:** the test proves unfinished retained work is aborted by buffer drop without timing + dependence. + +### R5 — Assess finished incoming-task behavior + +- **Status:** DONE +- **Priority:** Low impact / medium effort +- **Addresses:** P5 +- **Change:** After R1–R4, decide whether a task that completes before reinsertion has a stable, + independently stated behavior worth asserting. +- **Guardrails:** Record a no-change decision if the behavior is scheduler-dependent or has no + observable package contract. Do not create a test merely to cover the `new_task.is_finished()` + branch. +- **Decision:** No test added. `Launcher::run_udp_server_main` checks `abort_handle.is_finished()` + immediately after spawning a processor and does not call `force_push` for an already completed + task. The `new_task.is_finished()` check is therefore a defensive race guard only for completion + between that caller check and buffer admission. A direct test would need to control scheduler + timing rather than prove an observable UDP-server contract. +- **Done when:** the plan records a justified no-change decision. + +### R6 — Address Copilot review feedback on test mechanics + +- **Status:** DONE +- **Priority:** High impact / low effort +- **Addresses:** bounded cleanup waits, public-behavior setup, and capacity-independent full-buffer + setup. +- **Change:** Assess the three Copilot review suggestions from draft PR #2174 before changing the + completed test suite: (1) bound task joins so a cleanup regression fails rather than hangs; + (2) use `force_push` where it can establish setup without obscuring the intended full-buffer + state; and (3) derive the full-buffer fill count from the buffer capacity rather than hard-coding + 49 retained tasks. Apply the mandatory prose-first Arrange-Act-Assert comparison before any + approved refactor. +- **Guardrails:** Preserve deterministic synchronization and the oldest-first eviction contract. + Do not use polling or arbitrary sleeps as a timeout substitute. Keep the `force_push` Act and + eviction assertion visible in the behavior test. Do not change hot-path production code, capacity, + eviction policy, or shutdown semantics. +- **Decision:** Accept bounded task joins and capacity-derived setup. A timeout is an absolute + failure bound for an awaited cleanup outcome, not a delay or polling mechanism. Derive the count + of retained pending tasks from the buffer's actual capacity so the full-buffer scenario remains + correct if that policy changes. Decline public-API-only setup: `force_push` is the behavior under + test, so repeatedly calling it during Arrange would make the initial full-buffer state depend on + the Act and obscure which task is oldest. Keep the private `rb.try_push` operation only inside the + narrowly named setup helper, with a comment recording this reason. +- **Prose-first review:** The temporary Arrange prose was “a request buffer is full of controlled + pending tasks, with a separately retained oldest task”; the final + `FullBufferWithPendingTasks` constructor expresses this with an oldest task, a count derived from + `rb.capacity()`, and retained pending tasks. The Act remains one visible `force_push` call. The + temporary Assert prose was “the oldest task is aborted and all retained work is cleaned up”; the + named assertions express it, while `TASK_COMPLETION_TIMEOUT` documents the irreducible + test-process failure bound. The direct-insertion comment remains because it records why a + superficially attractive public-API setup would incorrectly execute the Act during Arrange. +- **Done when:** each suggestion has either a reviewed test-only change or a documented no-action + rationale, the affected tests remain readable/deterministic, and the PR threads have replies + before resolution. + +## Progress Tracking + +### Plan Checklist + +- [x] Phase 1 findings reviewed against current code and issue coverage evidence. +- [x] Phase 2 refactorings ordered by impact and effort. +- [x] Maintainer approved implementation of R1. +- [x] R1 implemented, reviewed, validated, and committed. +- [x] Maintainer approved implementation of R2. +- [x] R2 assessment, ADR, and source-comment clarification committed independently. +- [x] Maintainer approved implementation of R3. +- [x] R3 implemented, reviewed, validated, and committed. +- [x] Maintainer approved implementation of R3a. +- [x] R3a implemented, reviewed, validated, and committed. +- [x] Maintainer approved implementation of R4. +- [x] R4 implemented, reviewed, validated, and committed. +- [x] R5 assessment completed and decision recorded. +- [x] Maintainer approved R6. +- [x] R6 assessment and approved test refactor completed. +- [x] Maintainer reviewed all approved changes. +- [x] Plan completed and ready for final verification. + +### Progress Log + +- 2026-09-07 11:10 UTC - GitHub Copilot - Created this proposed plan from the + `request_buffer.rs` implementation, the issue baseline evidence, and the SI-15 shutdown-policy + boundary. No test or production change has been made. +- 2026-09-07 11:10 UTC - User/maintainer - Required performance protection for this hot-path + component. Test-only changes retain focused validation; any approved production change must first + establish the issue-local release-performance baseline and later record an equivalent after + measurement. +- 2026-09-07 11:27 UTC - User/maintainer - Approved R1 as a test-only increment and required the + accumulated issue planning, test-plan, and performance-evidence changes to be committed before + test implementation begins. +- 2026-09-07 11:43 UTC - User/maintainer - Reviewed and approved R1. The increment adds one + deterministic capacity-available contract without production changes, sleeps, polling, or a + performance-baseline requirement. +- 2026-09-07 13:08 UTC - User/maintainer - Approved R2. The test must establish an oldest pending + handle followed by completed handles, then prove whether finished work is reclaimed before active + work is aborted. If the expected contract fails, stop before changing the hot-path implementation + and follow the issue performance-baseline policy. +- 2026-09-07 15:12 UTC - GitHub Copilot - The initial R2 test assumed that all later completed + handles must be reclaimed before an oldest pending task can be evicted. The test failed as + expected against the implementation. +- 2026-09-07 15:27 UTC - GitHub Copilot - History review found this is intentional, not a defect: + PR #921 documents the starvation/fairness rationale, and PR #922 records a rejected + finished-handle-cleanup refactor due to a performance regression. A package-local ADR and source + comment clarification record this decision. The unsupported bug handoff and failing test snapshot + were removed. +- 2026-09-07 15:32 UTC - User/maintainer - Approved a deterministic R3 test for the documented + oldest-first eviction policy. The test must fill the buffer with pending tasks, prove that one + oldest task is aborted to admit the new task, and clean up every retained task explicitly. +- 2026-09-07 15:35 UTC - User/maintainer - Approved refactoring R3's complex Arrange section into + a file-local `FullBufferWithPendingTasks` scenario fixture before completing the test increment. + The fixture may own setup and cleanup mechanics only; the test retains the `force_push` Act and + observable eviction assertion. +- 2026-09-07 16:05 UTC - User/maintainer - Requested a further simplification of + `FullBufferWithPendingTasks::new`. Replaced duplicated channel/task mechanics with the file-local + `PendingTask` helper; the scenario constructor now directly states construction of the oldest task, + the remaining 49 pending tasks, and the incoming task. +- 2026-09-07 16:25 UTC - User/maintainer - Approved an R3a readability refinement: factor only + repeated pending-task insertion into `PendingTask::insert_into` and rename the incoming request + task. Commit the plan update before changing test code. +- 2026-09-07 16:30 UTC - User/maintainer - Reviewed and approved R3a. The local helper now owns + only pending-task construction/insertion mechanics, while the scenario constructor and test Act + remain readable and behavior-specific. +- 2026-09-07 16:37 UTC - User/maintainer - Approved R4. Use an inline completed task and the + focused `PendingTask` helper to make the mixed completion state visible without adding another + scenario fixture. +- 2026-09-07 16:42 UTC - User/maintainer - Reviewed and approved R4. The inline Arrange retains + the causal mixed completion state without a premature builder or scenario fixture; the test-only + increment leaves production behavior and the hot path unchanged. +- 2026-09-07 16:55 UTC - GitHub Copilot - Completed R5 assessment. No test is added: the launcher + filters already finished processor handles before buffer admission, and the remaining + `new_task.is_finished()` branch is a scheduler-dependent defensive race guard rather than a + stable observable contract. +- 2026-09-07 17:03 UTC - User/maintainer - Reviewed and approved the completed request-buffer + plan. R1–R4 add deterministic coverage, R2 documents the intentional performance trade-off, and + R5 is a justified no-change decision. The next test plan may begin. +- 2026-09-09 - GitHub Copilot - Draft PR #2174 received three Copilot suggestions on the completed + request-buffer tests: bound task-completion waits, avoid direct ring-buffer insertion where + public behavior can express setup, and avoid hard-coded capacity assumptions. R6 records them as + a new approval-gated test-maintainability assessment; no test change has been made. +- 2026-09-09 - User/maintainer - Approved R6 decisions: use bounded task joins and derive + full-buffer setup from capacity; retain private direct insertion only as controlled Arrange + mechanics because calling `force_push` during Arrange would test the Act before the test Act. + Make this rationale discoverable in the test code for future reviewers. +- 2026-09-09 - GitHub Copilot - Completed R6. All task joins now have the one-second absolute + cleanup bound; full-buffer construction derives retained-task count from `rb.capacity()`; and + comments document why direct insertion is controlled Arrange mechanics rather than a call to the + `force_push` Act. Focused request-buffer tests pass. + +### Validation Evidence + +| Increment | Status | Evidence | +| ------------------ | ------ | ----------------------------------------------------------- | +| Plan documentation | TODO | Run Markdown and spelling checks after plan review changes. | +| R1 | DONE | `cargo fmt --all -- --check`, focused request-buffer test, and `git diff --check` passed. | +| R2 | DONE | History review, package ADR, and production comments record the intentional oldest-first bounded policy; committed in `208f1d70`. | +| R3 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server server::request_buffer::tests`, and `git diff --check` passed. The reviewed `FullBufferWithPendingTasks` scenario uses a local `PendingTask` helper for setup/cleanup mechanics. | +| R3a | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server server::request_buffer::tests`, and `git diff --check` passed. | +| R4 | DONE | `cargo fmt --all -- --check`, `cargo test -p torrust-tracker-udp-server server::request_buffer::tests`, and `git diff --check` passed. | +| R5 | DONE | No change: the launcher filters already finished handles; the remaining defensive race guard has no stable observable contract. | +| R6 | DONE | Bounded cleanup waits and capacity-derived setup implemented. Public-API-only setup declined because `force_push` is the visible Act; the controlled direct-insertion rationale is documented in code. | +| Plan completion | DONE | Maintainer reviewed all approved increments and decisions before the next file plan begins. | + +## Non-Goals + +- Do not change the fixed capacity, ring-buffer implementation, or production control flow merely + to expose test internals. +- Do not make a production hot-path change without first recording the required baseline in + [performance-evidence.md](../performance-evidence.md). +- Do not define active-request draining, deadlines, joins, outcomes, or shutdown metrics; SI-15 + owns that policy. +- Do not test `Launcher` event publication here; this plan protects only the buffer's own contract. +- Do not add sleeps, polling loops, unbounded awaits, or log assertions. +- Do not replace the oldest-first policy with a full-buffer scan without a separately approved + production change, direct benchmark evidence, and ADR review. + +## Validation Per Approved Increment + +- Run the focused `ActiveRequests` unit tests. +- Run `cargo fmt --all -- --check` and `git diff --check`. +- Run `linter markdown` and `linter cspell` when this plan changes. +- For an approved production change, complete the applicable release throughput and, when needed, + focused microbenchmark evidence before committing the production increment. +- Review the changed test's causal state, visible Act, independently specified expected outcome, + and explicit task cleanup before the next increment. + +## Completion Criteria + +- Each approved test is deterministic, behavior-focused, and limited to current normal-operation + buffer semantics. +- Task completion and abort observation use explicit bounded synchronization rather than elapsed + time. +- No test changes the shutdown boundary owned by SI-15. +- The maintainer reviews every approved increment before the next increment and before final + verification. diff --git a/docs/templates/ISSUE.md b/docs/templates/ISSUE.md index 6b6486706..fb36111d3 100644 --- a/docs/templates/ISSUE.md +++ b/docs/templates/ISSUE.md @@ -89,10 +89,18 @@ refactor, or evidence increment; do not group unrelated changes merely to reduce | T2 | {Narrow, independently reviewable change} | Commit after focused validation and required review. | Record a justified no-change decision in the task's evidence without creating an empty commit. For -test-producing work, commit each reviewed test-design increment before starting the next planned -file or behavior area. Keep final verification and completion evidence separate when it improves -reviewability. Use a Conventional Commit message with the narrow affected scope, and sign every -commit with GPG. +test-producing work, use the `write-unit-test` skill and complete an explicit design review after +each passing test increment, before maintainer review and commit. Confirm that the test exposes the +one causal initial-state difference; its fixture owns only incidental mechanics; and the production +Act plus independently specified expected result remain visible. The review must use the mandatory +prose-first Arrange-Act-Assert comparison: write temporary prose for each section, refactor until +the code expresses it, remove redundant prose, and retain only irreducible context. Record this +review in task evidence or a file-local test plan. Assess helper boundaries by meaningful named +actions and abstraction-level alignment, not caller count: a single-use helper is valid when it +keeps the test readable and hides only incidental mechanics. Commit each reviewed test-design +increment before starting the next planned file or behavior area. Keep final verification and +completion evidence separate when it improves reviewability. Use a Conventional Commit message +with the narrow affected scope, and sign every commit with GPG. ## Progress Tracking diff --git a/docs/testing/refactoring-patterns/README.md b/docs/testing/refactoring-patterns/README.md index ef1882879..40ceba804 100644 --- a/docs/testing/refactoring-patterns/README.md +++ b/docs/testing/refactoring-patterns/README.md @@ -19,6 +19,8 @@ the mandatory conventions in the [unit-test skill](../../../.github/skills/dev/t | ------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------- | ------------------------------------------------------- | | [Scenario fixture with independent expected outputs](scenario-fixture-independent-expected-outputs.md) | One domain input must be verified through multiple independently decoded response representations. | `packages/axum-http-server/src/v1/handlers/announce.rs` | | [Scenario fixtures for causal initial state](scenario-fixtures-for-causal-initial-state.md) | Several setup operations establish the one state that makes the Act behave differently. | `packages/axum-http-server/src/server.rs` | +| [Prose-first Arrange-Act-Assert verification](prose-first-arrange-act-assert-verification.md) | A correct test is hard to read because its code does not yet express its behavioral intent. | `packages/udp-server/src/handlers/mod.rs` | +| [Named helpers for abstraction-level alignment](named-helpers-for-abstraction-level-alignment.md) | A coherent setup action is obscured by low-level mechanics or rejected only because it has one caller. | `packages/udp-server/tests/server/contract.rs` | ## Entry Requirements diff --git a/docs/testing/refactoring-patterns/named-helpers-for-abstraction-level-alignment.md b/docs/testing/refactoring-patterns/named-helpers-for-abstraction-level-alignment.md new file mode 100644 index 000000000..4c4a92910 --- /dev/null +++ b/docs/testing/refactoring-patterns/named-helpers-for-abstraction-level-alignment.md @@ -0,0 +1,62 @@ +--- +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - .github/skills/dev/testing/write-unit-test/SKILL.md + - packages/udp-server/tests/server/contract.rs + - docs/testing/refactoring-patterns/README.md +--- + +# Named Helpers for Abstraction-Level Alignment + +## Problem + +Test code can mix domain-relevant behavior with low-level setup mechanics. A reader then has to +reconstruct one coherent operation from configuration extraction, dependency construction, or +transport bootstrapping. The opposite mistake is rejecting a useful helper solely because it has one +caller, leaving callers at an inconsistent and noisy abstraction level. + +## Pattern + +Extract a helper when it gives a coherent sequence of actions a specific meaningful name and keeps +the calling test focused on its behavioral scenario. The helper owns incidental mechanics; the test +retains causal state, the production Act, and independently specified assertions. + +For example, an integration test that exercises a UDP datagram exchange can call: + +```rust +let tracker = start_ephemeral_udp_tracker().await; +``` + +This is justified even with one caller because it names one complete ordinary setup action. The test +can then remain at a consistent level: start tracker, connect client, send datagram, receive/decode +response, assert behavior, stop tracker. + +## Selection Criteria + +Keep a helper when all of the following are true: + +1. Its name describes an action, capability, or state rather than a vague implementation detail. +2. Its body performs one coherent responsibility. +3. It hides only incidental mechanics from the caller. +4. The caller retains the causal state, production Act, and expected result. +5. The helper makes the caller's abstraction level more consistent. + +Caller count is not a selection criterion. Reuse may later confirm a helper's value, but it is not +a prerequisite. + +## Do Not Use When + +- The inline code already expresses the state or action more clearly. +- The helper has a vague name such as `setup`, `prepare`, or `make_test_data`. +- It becomes a parameter bag or accumulates unrelated optional behavior. +- It hides the production call, derives expected outputs, or conceals the causal state. + +## Repository Example + +[`packages/udp-server/tests/server/contract.rs`](../../../packages/udp-server/tests/server/contract.rs) +uses `start_ephemeral_udp_tracker()` for the empty-datagram real-loopback contract. The helper +contains configuration and server-start mechanics; the test visibly provides the empty datagram, +performs the UDP exchange, decodes the response, and asserts the expected protocol error. The helper +was introduced during package-testing EPIC issue #1347, subissue #2149. diff --git a/docs/testing/refactoring-patterns/prose-first-arrange-act-assert-verification.md b/docs/testing/refactoring-patterns/prose-first-arrange-act-assert-verification.md new file mode 100644 index 000000000..fe2836b8a --- /dev/null +++ b/docs/testing/refactoring-patterns/prose-first-arrange-act-assert-verification.md @@ -0,0 +1,79 @@ +--- +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - .github/skills/dev/testing/write-unit-test/SKILL.md + - packages/udp-server/src/handlers/mod.rs + - docs/testing/refactoring-patterns/README.md +--- + +# Prose-First Arrange-Act-Assert Verification + +## Problem + +A test can pass while its behavioral intent remains implicit. Large Arrange blocks, parameter-bag +fixtures, opaque helpers, hidden production calls, and derived expected values make a test harder to +review and maintain. Conventional `Arrange`, `Act`, and `Assert` headings alone do not prove that +the code under each heading communicates what it is meant to establish. + +## Pattern + +Use temporary normal prose as the test specification, then make the code replace that prose: + +1. Write one **Arrange** paragraph identifying the causal initial-state difference, one **Act** + paragraph naming the production behavior, and one **Assert** paragraph stating the independently + specified observable result. +2. Place the complete prose specification above the test and repeat each paragraph directly above + its `// Arrange`, `// Act`, or `// Assert` section. +3. Compare each code section with its paragraph. Refactor names, setup, helper boundaries, + builders, scenario fixtures, the production call, or assertions until the code communicates the + same meaning. +4. Remove prose that the code now communicates. Retain a comment only when it supplies essential + domain, portability, ownership, or safety context that code cannot express without a misleading + or disproportionate abstraction. +5. Record the comparison in the test's task evidence before maintainer review and commit. + +The prose constrains refactoring: simplify implementation mechanics, but do not weaken the stated +behavior merely to make the test shorter. + +## Why This Works + +- **Readable and expressive:** reviewers can first agree on behavior in plain language, then see + that names and structure make the final code self-explanatory. +- **Maintainable:** a helper survives only when it has a specific, behavior-revealing responsibility. +- **Specific and behavioral:** the Act and independently specified outcome remain visible, preventing + implementation-detail assertions or expectations derived from production code. +- **Deterministic:** the temporary prose makes hidden clock, I/O, retry, sleep, and shared-state + dependencies easier to notice before they become flaky tests. +- **Structure-insensitive:** tests describe observable behavior, so internal refactoring need not + require changing an opaque fixture or commentary. + +## Use When + +- Adding a new test or materially refactoring an existing test. +- An Arrange block needs multiple setup lines and the causal state is difficult to identify. +- A proposed helper or fixture might merely move complexity outside the test body. +- A passing test is difficult to explain in a concise review. + +## Do Not Use When + +- Never skip the process because a test looks small; the comparison may confirm that inline code is + already the clearest design. +- Do not retain prose as permanent duplicate documentation once the code says the same thing. +- Do not force every domain explanation into code. Keep concise comments for irreducible facts, such + as a protocol constraint or platform-specific limitation. +- Do not use prose to conceal an unclear test. Refactor until the code can express the intended + behavior, or record why a direct test is not appropriate. + +## Repository Example + +The UDP handler-dispatch test in +[`packages/udp-server/src/handlers/mod.rs`](../../../packages/udp-server/src/handlers/mod.rs) +initially used a `SendableParseErrorPacketScenario` that combined the raw packet, environment, and +several ordinary `handle_packet` arguments. Its temporary prose distinguished the ordinary handler +environment from the causal raw scrape request containing no info hashes. The final code expresses +those responsibilities as `initialize_udp_handler_environment()` and +`scrape_request_without_info_hashes(transaction_id)`, while retaining the dispatcher Act and the +independent transaction-ID and request-kind assertions visibly in the test. This was reviewed under +package-testing EPIC issue #1347, subissue #2149. diff --git a/packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md b/packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md new file mode 100644 index 000000000..ea5aa2312 --- /dev/null +++ b/packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md @@ -0,0 +1,109 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - packages/udp-server/src/server/request_buffer.rs + - packages/udp-server/src/server/launcher.rs + - issue #2149 + - docs/issues/drafts/1488-si-15-define-udp-active-request-policy/ISSUE.md +--- + + + +# Keep Oldest-First UDP Request Eviction + +## Scope + +This is a package-local decision in `packages/udp-server/docs/adrs/`. It governs only the +extractable UDP server's bounded normal-operation request buffer. It does not define cross-package +protocol behavior, tracker-domain rules, or shutdown policy. + +## Description + +`ActiveRequests` stores up to 50 `AbortHandle` values for UDP request processor tasks. When a new +request arrives while the buffer is full, the server must make room quickly on the request hot path. + +A literal reading of the historic comments suggested scanning every retained handle and reclaiming +all completed tasks before aborting a live task. A deterministic test explored an ordering where the +oldest task was pending and later tasks were already completed. The implementation instead yields +once to the oldest pending task and aborts it when no older completed task has made space. + +The original active-request-buffer review established that a design which decoupled removal from +cleaning all completed tasks caused a performance regression. It also recorded that the oldest task +cannot be assumed to be the next task to complete. The current bounded traversal is therefore an +intentional normal-operation overload policy, not evidence of a defect. + +## Agreement + +When `ActiveRequests` is full, preserve the following oldest-first, bounded decision: + +1. Traverse handles from oldest to newest. +2. Discard completed handles encountered before the first handle that remains active after one + scheduler yield. +3. If such an active handle is encountered before any completed handle has created capacity, abort + that oldest active handle and stop scanning. +4. Otherwise continue its bounded traversal after capacity has been created. The current + implementation retains at most one subsequently encountered active handle for re-entry; any + broader change to that tracking behavior needs separate analysis and performance evidence. + +This policy favors prompt, bounded overload handling over a full-buffer scan that would preserve a +live oldest task when newer completed handles exist. Its work is bounded by the fixed capacity of +50, and it must not add dynamic dispatch, per-request heap allocation, or additional asynchronous +coordination. + +The `yield_now` call is a fairness opportunity for the oldest task to complete; it is not a +shutdown deadline, task-joining mechanism, or guarantee that every completed handle is reclaimed on +each insertion. + +## Alternatives Considered + +### Scan every retained handle before selecting an eviction + +This would preserve the oldest active task whenever any newer handle has completed. It was rejected: +the historical #922 experiment that separated removal from cleaning completed tasks regressed +performance, and the request path must remain bounded and inexpensive under load. + +### Replace the ring buffer or change its capacity + +Rejected. The issue is policy clarification, not a demonstrated data-structure or capacity defect. +Any future capacity or algorithm change requires separate evidence, review, and performance +measurement. + +### Treat this as shutdown behavior + +Rejected. This ADR governs normal-operation overload. Shutdown-time processor ownership, deadlines, +joining, and outcome reporting are separately owned by the planned SI-15 work. + +## Consequences + +- A later completed task may remain in the ring buffer when an older task is aborted under pressure. +- This ADR does not broaden the existing policy for tracking multiple active handles after an + earlier completed handle has created capacity; that behavior requires separate analysis before + it is changed or treated as a contract. +- Request-buffer tests must assert the documented oldest-first policy rather than a full-scan + reclamation policy. +- Any production change to this path requires the equivalent before/after performance evidence + described in Issue #2149. +- Future contributors have an explicit rationale for retaining this non-obvious trade-off. + +## Affected Code + +- `packages/udp-server/src/server/request_buffer.rs`: buffer traversal, completed-handle removal, + and oldest-active-task eviction. +- `packages/udp-server/src/server/launcher.rs`: calls `force_push` and publishes an aborted-request + fact only when the buffer reports an eviction. + +## Date + +2026-09-07 + +## References + +- Issue #2149: https://github.com/torrust/torrust-tracker/issues/2149 +- Original implementation: commit `89bb73576` +- Original review clarification: PR #921 + () +- Rejected performance-regression experiment: PR #922 + () +- Planned shutdown policy: `docs/issues/drafts/1488-si-15-define-udp-active-request-policy/ISSUE.md` diff --git a/packages/udp-server/docs/adrs/README.md b/packages/udp-server/docs/adrs/README.md new file mode 100644 index 000000000..4a00992d1 --- /dev/null +++ b/packages/udp-server/docs/adrs/README.md @@ -0,0 +1,16 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - packages/udp-server/docs/adrs/index.md + - .github/skills/dev/planning/create-adr/SKILL.md +--- + +# UDP Server Architectural Decision Records + +This directory contains architectural decision records owned solely by the extractable +`udp-server` package. See [index.md](index.md) for the record list. + +Use the repository root [`docs/adrs/`](../../../../docs/adrs/README.md) collection for decisions +that affect more than this package. diff --git a/packages/udp-server/docs/adrs/index.md b/packages/udp-server/docs/adrs/index.md new file mode 100644 index 000000000..045c7fd72 --- /dev/null +++ b/packages/udp-server/docs/adrs/index.md @@ -0,0 +1,14 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - packages/udp-server/docs/adrs/README.md + - .github/skills/dev/planning/create-adr/SKILL.md +--- + +# UDP Server ADR Index + +| ADR | Date | Title | Short Description | +| --- | --- | --- | --- | +| [20260907152707](20260907152707_keep_oldest_first_udp_request_eviction.md) | 2026-09-07 | Keep oldest-first UDP request eviction | Preserve the bounded, oldest-first overload decision instead of scanning all request handles before evicting active work. | diff --git a/packages/udp-server/src/container.rs b/packages/udp-server/src/container.rs index 173c04d24..55ea23a49 100644 --- a/packages/udp-server/src/container.rs +++ b/packages/udp-server/src/container.rs @@ -56,3 +56,60 @@ impl UdpTrackerServerServices { }) } } + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; + use std::time::Duration; + + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; + use torrust_tracker_primitives::{ConfigurationInstanceId, ServiceRole}; + use torrust_tracker_udp_core::event::ConnectionContext; + + use super::UdpTrackerServerServices; + use crate::event::Event; + + const EVENT_PUBLICATION_TIMEOUT: Duration = Duration::from_secs(1); + + fn sample_udp_request_received_event() -> Event { + Event::UdpRequestReceived { + context: ConnectionContext::new( + ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0), + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 8080), + ServiceBinding::new( + Protocol::UDP, + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 196)), 6969), + ) + .expect("sample UDP service binding should be valid"), + ), + } + } + + #[tokio::test] + async fn should_publish_events_through_the_enabled_server_event_bus() { + // Arrange + let services = UdpTrackerServerServices::initialize(); + let event = sample_udp_request_received_event(); + let mut event_receiver = services.event_bus.receiver(); + let event_sender = services + .stats_event_sender + .as_deref() + .expect("UDP-server services should enable event publication"); + + // Act + event_sender + .send(event.clone()) + .await + .expect("event sender should be active") + .expect("event should be delivered to the connected receiver"); + + // Assert + assert_eq!( + tokio::time::timeout(EVENT_PUBLICATION_TIMEOUT, event_receiver.recv()) + .await + .expect("event should be received before the test deadline") + .expect("event receiver should remain connected"), + event + ); + } +} diff --git a/packages/udp-server/src/error.rs b/packages/udp-server/src/error.rs index 9f1a53181..36a231842 100644 --- a/packages/udp-server/src/error.rs +++ b/packages/udp-server/src/error.rs @@ -102,3 +102,66 @@ impl From for SendableRequestParseError { } } } + +#[cfg(test)] +mod tests { + use torrust_tracker_udp_protocol::{ConnectionId, RequestParseError, TransactionId}; + use zerocopy::byteorder::network_endian::{I32, I64}; + + use super::{Error, SendableRequestParseError}; + + #[test] + fn it_should_preserve_response_routing_identifiers_for_a_sendable_parse_error() { + // Arrange + let connection_id = ConnectionId(I64::new(12)); + let transaction_id = TransactionId(I32::new(34)); + let parse_error = RequestParseError::sendable_text("invalid announce request", connection_id, transaction_id); + + // Act + let actual = SendableRequestParseError::from(parse_error); + + // Assert + assert_eq!(actual.message, "invalid announce request"); + assert_eq!(actual.opt_connection_id, Some(connection_id)); + assert_eq!(actual.opt_transaction_id, Some(transaction_id)); + } + + #[test] + fn it_should_clear_response_routing_identifiers_for_an_unsendable_parse_error() { + // Arrange + let parse_error = RequestParseError::unsendable_text("invalid request action"); + + // Act + let actual = SendableRequestParseError::from(parse_error); + + // Assert + assert_eq!(actual.message, "invalid request action"); + assert_eq!(actual.opt_connection_id, None); + assert_eq!(actual.opt_transaction_id, None); + } + + #[test] + fn it_should_wrap_a_sendable_parse_error_as_an_invalid_request() { + // Arrange + let connection_id = ConnectionId(I64::new(12)); + let transaction_id = TransactionId(I32::new(34)); + let parse_error = RequestParseError::sendable_text("invalid scrape request", connection_id, transaction_id); + + // Act + let actual = Error::from(parse_error); + + // Assert + assert!(matches!( + actual, + Error::InvalidRequest { + request_parse_error: SendableRequestParseError { + message, + opt_connection_id: Some(actual_connection_id), + opt_transaction_id: Some(actual_transaction_id), + }, + } if message == "invalid scrape request" + && actual_connection_id == connection_id + && actual_transaction_id == transaction_id + )); + } +} diff --git a/packages/udp-server/src/event.rs b/packages/udp-server/src/event.rs index 92685509a..ae10fee37 100644 --- a/packages/udp-server/src/event.rs +++ b/packages/udp-server/src/event.rs @@ -166,3 +166,185 @@ pub mod bus { pub type EventBus = torrust_tracker_events::bus::EventBus; } + +#[cfg(test)] +mod tests { + use std::net::Ipv4Addr; + use std::num::NonZeroU16; + use std::panic::Location; + use std::str::FromStr; + + use torrust_info_hash::InfoHash; + use torrust_metrics::label::LabelValue; + use torrust_peer_id::PeerId; + use torrust_tracker_core::databases::error::Error as DatabaseError; + use torrust_tracker_core::error::{AnnounceError, WhitelistError}; + use torrust_tracker_primitives::Driver; + use torrust_tracker_udp_core::connection_cookie::ConnectionCookieError; + use torrust_tracker_udp_core::services::announce::UdpAnnounceError; + use torrust_tracker_udp_protocol::{ + AnnounceActionPlaceholder, AnnounceEvent, AnnounceRequest, ConnectionId, InfoHash as UdpInfoHash, NumberOfBytes, + NumberOfPeers, PeerKey, Port, TransactionId, + }; + use zerocopy::byteorder::network_endian::I32; + + use super::{ErrorKind, UdpRequestKind}; + use crate::error::{Error, SendableRequestParseError}; + + fn announce_request() -> AnnounceRequest { + AnnounceRequest { + connection_id: ConnectionId(I32::new(0).into()), + action_placeholder: AnnounceActionPlaceholder::default(), + transaction_id: TransactionId(I32::new(0)), + info_hash: UdpInfoHash([0; 20]), + peer_id: PeerId([0; 20]), + bytes_downloaded: NumberOfBytes(I32::new(0).into()), + bytes_left: NumberOfBytes(I32::new(0).into()), + bytes_uploaded: NumberOfBytes(I32::new(0).into()), + event: AnnounceEvent::None.into(), + ip_address: Ipv4Addr::UNSPECIFIED.into(), + key: PeerKey::new(0), + peers_wanted: NumberOfPeers::new(0), + port: Port::new(NonZeroU16::MIN), + } + } + + #[test] + fn it_should_classify_an_invalid_request_as_a_request_parse_error() { + // Arrange + let error = Error::InvalidRequest { + request_parse_error: SendableRequestParseError { + message: "invalid request".to_string(), + opt_connection_id: None, + opt_transaction_id: None, + }, + }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert_eq!( + actual, + ErrorKind::RequestParse( + "SendableRequestParseError: message: invalid request, connection_id: None, transaction_id: None".to_string(), + ) + ); + } + + #[test] + fn it_should_classify_a_connection_cookie_error() { + // Arrange + let error = Error::AnnounceFailed { + source: UdpAnnounceError::ConnectionCookieError { + source: ConnectionCookieError::ValueExpired { + expired_value: 1.0, + min_value: 2.0, + }, + }, + }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert_eq!( + actual, + ErrorKind::ConnectionCookie("cookie value is expired: 1, expected > 2".to_string()) + ); + } + + #[test] + fn it_should_classify_a_whitelist_error() { + // Arrange + let info_hash = InfoHash::from_str("3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0") // DevSkim: ignore DS173237 + .expect("test info hash should be valid"); + let error = Error::AnnounceFailed { + source: UdpAnnounceError::TrackerCoreWhitelistError { + source: WhitelistError::TorrentNotWhitelisted { + info_hash, + location: Location::caller(), + }, + }, + }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert!( + matches!(actual, ErrorKind::Whitelist(message) if message.contains("The torrent: 3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0, is not whitelisted")) + ); + } + + #[test] + fn it_should_classify_a_database_error() { + // Arrange + let error = Error::AnnounceFailed { + source: UdpAnnounceError::TrackerCoreAnnounceError { + source: AnnounceError::Database(DatabaseError::MalformedDatabaseRecord { + message: "corrupt record".to_string(), + driver: Driver::Sqlite3, + }), + }, + }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert_eq!( + actual, + ErrorKind::Database("Malformed Sqlite3 database record: corrupt record".to_string()) + ); + } + + #[test] + fn it_should_classify_an_internal_error() { + // Arrange + let error = Error::Internal { + location: Location::caller(), + message: "internal failure".to_string(), + }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert_eq!(actual, ErrorKind::InternalServer("internal failure".to_string())); + } + + #[test] + fn it_should_classify_an_authentication_error() { + // Arrange + let location = Location::caller(); + let error = Error::AuthRequired { location }; + + // Act + let actual = ErrorKind::from(error); + + // Assert + assert_eq!(actual, ErrorKind::TrackerAuthentication(location.to_string())); + } + + #[test] + fn it_should_convert_request_kinds_to_metric_labels_and_display_values() { + // Arrange + let cases = [ + (UdpRequestKind::Connect, "connect"), + ( + UdpRequestKind::Announce { + announce_request: announce_request(), + }, + "announce", + ), + (UdpRequestKind::Scrape, "scrape"), + ]; + + // Act and Assert + for (request_kind, expected) in cases { + assert_eq!(request_kind.to_string(), expected); + assert_eq!(LabelValue::from(request_kind), LabelValue::new(expected)); + } + } +} diff --git a/packages/udp-server/src/handlers/mod.rs b/packages/udp-server/src/handlers/mod.rs index afea685c4..a52d3aa70 100644 --- a/packages/udp-server/src/handlers/mod.rs +++ b/packages/udp-server/src/handlers/mod.rs @@ -244,6 +244,7 @@ pub(crate) mod tests { use futures::future::BoxFuture; use mockall::mock; + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; use torrust_tracker_configuration::v3_0_0::Configuration; use torrust_tracker_configuration::v3_0_0::core::Core; use torrust_tracker_core::announce_handler::AnnounceHandler; @@ -264,8 +265,12 @@ pub(crate) mod tests { use torrust_tracker_udp_core::services::announce::AnnounceService; use torrust_tracker_udp_core::services::scrape::ScrapeService; use torrust_tracker_udp_core::{self, event as core_event}; + use torrust_tracker_udp_protocol::{ConnectionId, ErrorResponse, Request, Response, ScrapeRequest, TransactionId}; + use zerocopy::byteorder::network_endian::{I32, I64}; use crate::event as server_event; + use crate::testing::environment::EnvContainer; + use crate::{RawRequest, handlers::handle_packet}; pub struct CoreTrackerServices { pub core_config: Arc, @@ -462,4 +467,63 @@ pub(crate) mod tests { fn send(&self, event: server_event::Event) -> BoxFuture<'static,Option > > > ; } } + + async fn initialize_udp_handler_environment() -> EnvContainer { + let configuration = configuration::ephemeral(); + let core_config = Arc::new(configuration.core.clone()); + let udp_tracker_config = Arc::new(configuration.udp_trackers.as_ref().expect("UDP tracker configuration")[0].clone()); + EnvContainer::initialize( + &core_config, + &udp_tracker_config, + configuration.udp_tracker_server.max_connection_id_errors_per_ip, + ) + .await + } + + fn scrape_request_without_info_hashes(transaction_id: TransactionId) -> RawRequest { + let request = Request::Scrape(ScrapeRequest { + connection_id: ConnectionId(I64::new(7)), + transaction_id, + info_hashes: Vec::new(), + }); + let mut payload = Vec::new(); + request.write_bytes(&mut payload).expect("scrape request should serialize"); + + RawRequest { + payload, + from: sample_ipv4_remote_addr(), + } + } + + #[tokio::test] + async fn it_should_preserve_the_transaction_id_for_a_sendable_parse_error_without_a_request_kind() { + // Arrange + let environment = initialize_udp_handler_environment().await; + let transaction_id = TransactionId(I32::new(42)); + let raw_request = scrape_request_without_info_hashes(transaction_id); + + // Act + let (response, request_kind) = handle_packet( + raw_request, + environment.udp_tracker_core_container, + environment.udp_tracker_server_container, + ServiceBinding::new(Protocol::UDP, sample_ipv4_socket_address()).expect("UDP service binding should be valid"), + super::CookieTimeValues { + issue_time: sample_issue_time(), + valid_range: sample_cookie_valid_range(), + }, + torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, + ) + .await; + + // Assert + assert!(matches!( + response, + Response::Error(ErrorResponse { + transaction_id: actual_transaction_id, + .. + }) if actual_transaction_id == transaction_id + )); + assert_eq!(request_kind, None); + } } diff --git a/packages/udp-server/src/server/bound_socket.rs b/packages/udp-server/src/server/bound_socket.rs index 80e21f23c..88f83319a 100644 --- a/packages/udp-server/src/server/bound_socket.rs +++ b/packages/udp-server/src/server/bound_socket.rs @@ -138,3 +138,47 @@ impl Debug for BoundSocket { f.debug_struct("UdpSocket").field("addr", &local_addr).finish_non_exhaustive() } } + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; + + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; + use url::Url; + + use super::BoundSocket; + + #[tokio::test] + async fn it_should_bind_to_a_non_zero_port_when_port_zero_is_requested() { + // Arrange + let requested_address = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0); + + // Act + let bound_socket = BoundSocket::bind(requested_address, false).expect("IPv4 loopback socket should bind"); + + // Assert + assert_ne!(bound_socket.address().port(), 0); + } + + #[tokio::test] + async fn it_should_report_consistent_udp_endpoint_metadata() { + // Arrange + let requested_address = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0); + let bound_socket = BoundSocket::bind(requested_address, false).expect("IPv4 loopback socket should bind"); + let expected_address = bound_socket.address(); + + // Act + let actual_url = bound_socket.url(); + let actual_service_binding = bound_socket.service_binding(); + + // Assert + assert_eq!( + actual_url, + Url::parse(&format!("udp://{expected_address}")).expect("bound UDP address should form a URL") + ); + assert_eq!( + actual_service_binding, + ServiceBinding::new(Protocol::UDP, expected_address).expect("bound UDP address should form a service binding") + ); + } +} diff --git a/packages/udp-server/src/server/launcher.rs b/packages/udp-server/src/server/launcher.rs index 899e03bac..789dead38 100644 --- a/packages/udp-server/src/server/launcher.rs +++ b/packages/udp-server/src/server/launcher.rs @@ -314,61 +314,111 @@ async fn publish_event_if_sender_available(sender: &Sender, event: Event) { #[cfg(test)] mod tests { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::sync::Arc; + use std::time::Duration; use tokio::sync::oneshot; + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; use torrust_server_lib::signals::{Halted, Started}; use torrust_tracker_configuration::v3_0_0::logging; use torrust_tracker_primitives::{ConfigurationInstanceId, ServiceRole}; use torrust_tracker_test_helpers::configuration::ephemeral_public; use torrust_tracker_udp_core::container::UdpTrackerCoreContainer; + use torrust_tracker_udp_core::event::ConnectionContext; use super::Launcher; + use crate::RawRequest; use crate::container::UdpTrackerServerContainer; + use crate::event::Event; use crate::server::bound_socket::BoundSocket; + const TEST_LOG_TARGET: &str = "udp://test"; + // This is an absolute failure bound, not a scheduling delay. Event-publication regressions + // must fail diagnostically instead of leaving the test process waiting indefinitely. + const EVENT_PUBLICATION_TIMEOUT: Duration = Duration::from_secs(1); + + struct UdpLauncherTestContext { + udp_tracker_core_container: Arc, + udp_tracker_server_container: Arc, + cookie_lifetime: Duration, + bind_address: SocketAddr, + max_connection_id_errors_per_ip: u32, + } + + impl UdpLauncherTestContext { + async fn new() -> Self { + let configuration = Arc::new(ephemeral_public()); + let core_config = Arc::new(configuration.core.clone()); + let udp_tracker_config = Arc::new( + configuration + .udp_trackers + .clone() + .expect("UDP test configuration should include a tracker") + .into_iter() + .next() + .expect("UDP test configuration should include one tracker"), + ); + torrust_clock::initialize_static(); + torrust_tracker_udp_core::initialize_static(); + logging::setup(&configuration.logging); + + let configuration_instance_id = ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0); + let udp_tracker_core_container = UdpTrackerCoreContainer::initialize( + &core_config, + &udp_tracker_config, + configuration.udp_tracker_server.max_connection_id_errors_per_ip, + configuration_instance_id, + ) + .await; + let udp_tracker_server_container = UdpTrackerServerContainer::initialize(&core_config); + + Self { + udp_tracker_core_container, + udp_tracker_server_container, + cookie_lifetime: udp_tracker_config.cookie_lifetime, + bind_address: udp_tracker_config.bind_address, + max_connection_id_errors_per_ip: configuration.udp_tracker_server.max_connection_id_errors_per_ip, + } + } + + async fn with_banned_client_ip(client_ip: IpAddr) -> Self { + let context = Self::new().await; + let mut ban_service = context.udp_tracker_core_container.ban_service.write().await; + + for _ in 0..=context.max_connection_id_errors_per_ip { + ban_service.increase_counter(&client_ip); + } + + drop(ban_service); + context + } + } + + fn sample_udp_service_binding(bind_address: SocketAddr) -> ServiceBinding { + ServiceBinding::new(Protocol::UDP, SocketAddr::new(bind_address.ip(), 6969)) + .expect("sample UDP service binding should be valid") + } + #[tokio::test] async fn it_should_release_the_socket_when_the_startup_notification_receiver_is_dropped() { // Arrange - let configuration = Arc::new(ephemeral_public()); - let core_config = Arc::new(configuration.core.clone()); - let udp_tracker_config = Arc::new( - configuration - .udp_trackers - .clone() - .expect("UDP test configuration should include a tracker") - .into_iter() - .next() - .expect("UDP test configuration should include one tracker"), - ); - torrust_clock::initialize_static(); - torrust_tracker_udp_core::initialize_static(); - logging::setup(&configuration.logging); - - let configuration_instance_id = ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0); - let udp_tracker_core_container = UdpTrackerCoreContainer::initialize( - &core_config, - &udp_tracker_config, - configuration.udp_tracker_server.max_connection_id_errors_per_ip, - configuration_instance_id, - ) - .await; - let udp_tracker_server_container = UdpTrackerServerContainer::initialize(&core_config); - let bound_socket = BoundSocket::bind(udp_tracker_config.bind_address, false).expect("UDP socket should bind"); - let address = bound_socket.address(); - let (tx_start, rx_start) = oneshot::channel::(); - let (_tx_halt, rx_halt) = oneshot::channel::(); - drop(rx_start); + let launcher = UdpLauncherTestContext::new().await; + let bound_socket = BoundSocket::bind(launcher.bind_address, false).expect("UDP socket should bind"); + let bound_address = bound_socket.address(); + let (startup_notification_sender, startup_notification_receiver) = oneshot::channel::(); + let (_halt_sender, halt_receiver) = oneshot::channel::(); + drop(startup_notification_receiver); // Act let result = Launcher::run_with_graceful_shutdown( - udp_tracker_core_container, - udp_tracker_server_container, + launcher.udp_tracker_core_container, + launcher.udp_tracker_server_container, bound_socket, - udp_tracker_config.cookie_lifetime, + launcher.cookie_lifetime, torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, - tx_start, - rx_halt, + startup_notification_sender, + halt_receiver, ) .await; @@ -377,6 +427,136 @@ mod tests { result.expect_err("startup notification should fail").kind(), std::io::ErrorKind::BrokenPipe ); - BoundSocket::bind(address, false).expect("UDP socket should be released after startup notification failure"); + BoundSocket::bind(bound_address, false).expect("UDP socket should be released after startup notification failure"); + } + + #[tokio::test] + async fn it_should_require_discarding_a_request_when_its_source_port_is_zero() { + // Arrange + let launcher = UdpLauncherTestContext::new().await; + let client_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); + let request = RawRequest { + payload: Vec::new(), + from: client_socket_addr, + }; + let server_service_binding = sample_udp_service_binding(launcher.bind_address); + + // Act + let should_discard = Launcher::should_discard_request( + &request, + &launcher.udp_tracker_core_container, + &launcher.udp_tracker_server_container, + &server_service_binding, + TEST_LOG_TARGET, + torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, + ) + .await; + + // Assert + assert!(should_discard); + } + + #[tokio::test] + async fn it_should_require_discarding_a_request_when_its_client_ip_is_banned_in_strict_mode() { + // Arrange + let client_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 8080); + let launcher = UdpLauncherTestContext::with_banned_client_ip(client_socket_addr.ip()).await; + let request = RawRequest { + payload: Vec::new(), + from: client_socket_addr, + }; + let server_service_binding = sample_udp_service_binding(launcher.bind_address); + + // Act + let should_discard = Launcher::should_discard_request( + &request, + &launcher.udp_tracker_core_container, + &launcher.udp_tracker_server_container, + &server_service_binding, + TEST_LOG_TARGET, + torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, + ) + .await; + + // Assert + assert!(should_discard); + } + + #[tokio::test] + async fn it_should_publish_a_request_banned_event_when_its_client_ip_is_banned_in_strict_mode() { + // Arrange + let client_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 8080); + let launcher = UdpLauncherTestContext::with_banned_client_ip(client_socket_addr.ip()).await; + let request = RawRequest { + payload: Vec::new(), + from: client_socket_addr, + }; + let server_service_binding = sample_udp_service_binding(launcher.bind_address); + let mut event_receiver = launcher.udp_tracker_server_container.event_bus.receiver(); + + // Act + let _ = Launcher::should_discard_request( + &request, + &launcher.udp_tracker_core_container, + &launcher.udp_tracker_server_container, + &server_service_binding, + TEST_LOG_TARGET, + torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, + ) + .await; + + // Assert + assert_eq!( + tokio::time::timeout(EVENT_PUBLICATION_TIMEOUT, event_receiver.recv()) + .await + .expect("request-banned event should be published before the test deadline") + .expect("request-banned event receiver should remain connected"), + Event::UdpRequestBanned { + context: ConnectionContext::new( + launcher.udp_tracker_core_container.configuration_instance_id, + client_socket_addr, + server_service_binding, + ), + } + ); + } + + #[tokio::test] + async fn it_should_publish_a_request_discarded_event_when_its_source_port_is_zero() { + // Arrange + let launcher = UdpLauncherTestContext::new().await; + let client_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)), 0); + let request = RawRequest { + payload: Vec::new(), + from: client_socket_addr, + }; + let server_service_binding = sample_udp_service_binding(launcher.bind_address); + let mut event_receiver = launcher.udp_tracker_server_container.event_bus.receiver(); + + // Act + let _ = Launcher::should_discard_request( + &request, + &launcher.udp_tracker_core_container, + &launcher.udp_tracker_server_container, + &server_service_binding, + TEST_LOG_TARGET, + torrust_tracker_udp_core::ConnectionIdValidationPolicy::Strict, + ) + .await; + + // Assert + assert_eq!( + tokio::time::timeout(EVENT_PUBLICATION_TIMEOUT, event_receiver.recv()) + .await + .expect("request-discarded event should be published before the test deadline") + .expect("request-discarded event receiver should remain connected"), + Event::UdpRequestDiscarded { + context: ConnectionContext::new( + launcher.udp_tracker_core_container.configuration_instance_id, + client_socket_addr, + server_service_binding, + ), + } + ); } } diff --git a/packages/udp-server/src/server/request_buffer.rs b/packages/udp-server/src/server/request_buffer.rs index fa2861987..4bff353c0 100644 --- a/packages/udp-server/src/server/request_buffer.rs +++ b/packages/udp-server/src/server/request_buffer.rs @@ -3,7 +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 +// ADR: packages/udp-server/docs/adrs/20260907152707_keep_oldest_first_udp_request_eviction.md /// A ring buffer for managing active UDP request abort handles. /// /// The `ActiveRequests` struct maintains a fixed-size ring buffer of abort @@ -36,10 +36,16 @@ impl Drop for ActiveRequests { impl ActiveRequests { /// Inserts an abort handle for a UDP request processor task. /// - /// If the buffer is full, this method attempts to make space by: + /// If the buffer is full, this method traverses handles from oldest to newest. It: /// - /// 1. Removing finished tasks. - /// 2. Removing the oldest unfinished task if no finished tasks are found. + /// 1. Removes completed handles encountered before the first still-active handle. + /// 2. Gives that oldest active task one scheduler yield to finish. + /// 3. Aborts that task when no earlier completed handle created capacity; otherwise it + /// continues the bounded traversal. It retains at most one subsequently encountered active + /// handle for re-entry. + /// + /// It intentionally does not scan all newer handles before selecting this eviction. See the + /// module ADR for the request-hot-path performance rationale. /// /// Returns `true` if a task was removed, `false` otherwise. /// @@ -66,28 +72,22 @@ impl ActiveRequests { let mut old_task_aborted = false; for old_task in self.rb.pop_iter() { - // We found a finished tasks ... increase the counter and - // continue searching for more and ... + // A completed task before the first still-active task frees capacity. if old_task.is_finished() { finished += 1; continue; } - // The current removed tasks is not finished. - - // Give it a second chance to finish. + // Give the oldest still-active task one opportunity to finish. tokio::task::yield_now().await; - // Recheck if it finished ... increase the counter and - // continue searching for more and ... + // If it completed while yielded, it also frees capacity. if old_task.is_finished() { finished += 1; continue; } - // At this point we found a "definitive" unfinished task. - - // Log unfinished task. + // This is the first task that remains active after yielding. tracing::debug!( target: UDP_TRACKER_LOG_TARGET, local_addr, @@ -95,8 +95,7 @@ impl ActiveRequests { "Udp::run_udp_server::loop (got unfinished task)" ); - // If no finished tasks were found, abort the current - // unfinished task. + // No older completed task created capacity, so evict this oldest active task. if finished == 0 { // We make place aborting this task. old_task.abort(); @@ -111,11 +110,7 @@ impl ActiveRequests { break; } - // At this point we found at least one finished task, but the - // current one is not finished and it was removed from the - // buffer, so we need to re-insert in in the buffer. - - // Save the unfinished task for re-entry. + // Earlier completed tasks created capacity; retain this active task for re-entry. unfinished_task = Some(old_task); } @@ -124,18 +119,14 @@ impl ActiveRequests { // buffer to be full again. That means the "expects" should // never happen. - // Reinsert the unfinished task if any. + // Reinsert the active task that followed at least one completed task, if any. if let Some(h) = unfinished_task { self.rb.try_push(h).expect("it was previously inserted"); } // Insert the new task. // - // Notice that space has already been made for this new task in - // the buffer. One or many old task have already been finished - // or yielded, freeing space in the buffer. Or a single - // unfinished task has been aborted to make space for this new - // task. + // Earlier completed tasks, or one oldest active task eviction, made capacity. if !new_task.is_finished() { self.rb.try_push(new_task).expect("it should have space for this new task."); } @@ -145,3 +136,190 @@ impl ActiveRequests { } } } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use tokio::sync::oneshot; + use tokio::task::JoinHandle; + + use ringbuf::traits::{Observer, Producer}; + + use super::ActiveRequests; + + // This is an absolute failure bound, not a scheduling delay. A cleanup regression must fail + // the test with a useful message rather than leave the test process waiting forever. + const TASK_COMPLETION_TIMEOUT: Duration = Duration::from_secs(1); + + struct PendingTask { + completion_sender: oneshot::Sender<()>, + join_handle: JoinHandle<()>, + } + + impl PendingTask { + fn new() -> Self { + let (completion_sender, completion_receiver) = oneshot::channel::<()>(); + let join_handle = tokio::spawn(async move { + drop(completion_receiver.await); + }); + + Self { + completion_sender, + join_handle, + } + } + + fn abort_handle(&self) -> tokio::task::AbortHandle { + self.join_handle.abort_handle() + } + + fn insert_into(self, active_requests: &mut ActiveRequests) -> Self { + // `force_push` is the Act being tested. Direct insertion here establishes the full + // pending-buffer state without executing that behavior during Arrange, keeping the + // oldest task and capacity-exhausted condition independently controlled. + active_requests + .rb + .try_push(self.abort_handle()) + .expect("a request buffer with available capacity should accept the pending task"); + self + } + + async fn assert_was_aborted(self, message: &str) { + let join_result = tokio::time::timeout(TASK_COMPLETION_TIMEOUT, self.join_handle) + .await + .expect("pending task should complete or abort before the cleanup deadline"); + + join_result.expect_err(message); + } + + async fn assert_completed_after_cleanup(self) { + drop(self.completion_sender); + + let join_result = tokio::time::timeout(TASK_COMPLETION_TIMEOUT, self.join_handle) + .await + .expect("pending task should complete before the cleanup deadline"); + + join_result.expect("pending task should complete after test cleanup"); + } + } + + struct FullBufferWithPendingTasks { + active_requests: ActiveRequests, + oldest_task: Option, + retained_tasks: Vec, + incoming_task: PendingTask, + } + + impl FullBufferWithPendingTasks { + fn new() -> Self { + let mut active_requests = ActiveRequests::default(); + let oldest_task = PendingTask::new().insert_into(&mut active_requests); + + let retained_task_count = active_requests + .rb + .capacity() + .get() + .checked_sub(1) + .expect("the active request buffer should have capacity for an oldest task"); + let mut retained_tasks = Vec::with_capacity(retained_task_count); + for _ in 0..retained_task_count { + retained_tasks.push(PendingTask::new().insert_into(&mut active_requests)); + } + + let incoming_task = PendingTask::new(); + + Self { + active_requests, + oldest_task: Some(oldest_task), + retained_tasks, + incoming_task, + } + } + + fn incoming_task_abort_handle(&self) -> tokio::task::AbortHandle { + self.incoming_task.abort_handle() + } + + async fn assert_oldest_task_was_aborted(&mut self) { + self.oldest_task + .take() + .expect("scenario should retain the oldest task") + .assert_was_aborted("oldest pending task should be evicted when capacity is exhausted") + .await; + } + + async fn abort_and_join_retained_tasks(self) { + drop(self.active_requests); + + for task in self.retained_tasks { + task.assert_was_aborted("retained task should be aborted during test cleanup") + .await; + } + self.incoming_task + .assert_was_aborted("incoming task should be aborted during test cleanup") + .await; + } + } + + #[tokio::test] + async fn it_should_not_evict_a_pending_task_when_the_buffer_has_available_capacity() { + // Arrange + let task = PendingTask::new(); + let mut active_requests = ActiveRequests::default(); + + // Act + let task_was_evicted = active_requests.force_push(task.abort_handle(), "127.0.0.1:6969").await; + + // Assert + assert!(!task_was_evicted); + assert!(!task.join_handle.is_finished()); + + task.assert_completed_after_cleanup().await; + } + + #[tokio::test] + async fn it_should_evict_the_oldest_pending_task_when_the_buffer_is_full() { + // Arrange + let mut scenario = FullBufferWithPendingTasks::new(); + + // Act + let task_was_evicted = scenario + .active_requests + .force_push(scenario.incoming_task_abort_handle(), "127.0.0.1:6969") + .await; + + // Assert + assert!(task_was_evicted); + scenario.assert_oldest_task_was_aborted().await; + scenario.abort_and_join_retained_tasks().await; + } + + #[tokio::test] + async fn it_should_abort_a_pending_task_when_the_request_buffer_is_dropped() { + // Arrange + let completed_task = tokio::spawn(async {}); + let completed_task_abort_handle = completed_task.abort_handle(); + completed_task + .await + .expect("completed task should finish before the buffer is dropped"); + + let pending_task = PendingTask::new(); + let mut active_requests = ActiveRequests::default(); + // The completed handle establishes mixed buffer state. `force_push` is not used here + // because this test's Act is dropping the buffer, not admitting a request. + active_requests + .rb + .try_push(completed_task_abort_handle) + .expect("an empty request buffer should accept the completed task"); + let pending_task = pending_task.insert_into(&mut active_requests); + + // Act + drop(active_requests); + + // Assert + pending_task + .assert_was_aborted("pending task should be aborted when the request buffer is dropped") + .await; + } +} diff --git a/packages/udp-server/src/statistics/event/handler/error.rs b/packages/udp-server/src/statistics/event/handler/error.rs index fffa2c44e..ad8f48da0 100644 --- a/packages/udp-server/src/statistics/event/handler/error.rs +++ b/packages/udp-server/src/statistics/event/handler/error.rs @@ -106,41 +106,111 @@ mod tests { use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use torrust_clock::clock::Time; + use torrust_metrics::label::LabelSet; + use torrust_metrics::metric_collection::aggregate::sum::Sum; + use torrust_metrics::{label_name, metric_name}; use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; + use torrust_peer_id::PeerId; use torrust_tracker_primitives::{ConfigurationInstanceId, ServiceRole}; use torrust_tracker_udp_core::event::ConnectionContext; + use super::handle_event; use crate::CurrentClock; - use crate::event::Event; - use crate::statistics::event::handler::error::ErrorKind; - use crate::statistics::event::handler::handle_event; - use crate::statistics::repository::Repository; + use crate::event::ErrorKind; + use crate::event::UdpRequestKind; + use crate::handlers::announce::tests::announce_request::AnnounceRequestBuilder; + use crate::statistics::{ + UDP_TRACKER_SERVER_CONNECTION_ID_ERRORS_TOTAL, UDP_TRACKER_SERVER_ERRORS_TOTAL, repository::Repository, + }; + + fn sample_ipv4_connection_context() -> ConnectionContext { + ConnectionContext::new( + ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0), + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 8080), + ServiceBinding::new( + Protocol::UDP, + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 196)), 6969), + ) + .expect("sample UDP service binding should be valid"), + ) + } #[tokio::test] async fn should_increase_the_udp4_errors_counter_when_it_receives_a_udp4_error_event() { + // Arrange + let stats_repository = Repository::new(); + let connection_context = sample_ipv4_connection_context(); + let error_kind = ErrorKind::RequestParse("Invalid request format".to_string()); + + // Act + handle_event(connection_context, None, error_kind, &stats_repository, CurrentClock::now()).await; + + // Assert + let stats = stats_repository.get_stats().await; + + assert_eq!(stats.udp4_errors_total(), 1); + } + + #[tokio::test] + async fn should_label_a_general_error_metric_with_connect_request_kind() { + // Arrange let stats_repository = Repository::new(); + let connection_context = sample_ipv4_connection_context(); + let error_kind = ErrorKind::RequestParse("Invalid request format".to_string()); + let mut expected_labels = LabelSet::from(connection_context.clone()); + expected_labels.upsert(label_name!("request_kind"), "connect".to_string().into()); + // Act handle_event( - Event::UdpError { - context: ConnectionContext::new( - ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0), - SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 8080), - ServiceBinding::new( - Protocol::UDP, - SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 196)), 6969), - ) - .unwrap(), - ), - kind: None, - error: ErrorKind::RequestParse("Invalid request format".to_string()), - }, + connection_context, + Some(UdpRequestKind::Connect), + error_kind, &stats_repository, CurrentClock::now(), ) .await; - let stats = stats_repository.get_stats().await; + // Assert + let counter_value = { + let stats = stats_repository.get_stats().await; + stats + .metric_collection + .sum(&metric_name!(UDP_TRACKER_SERVER_ERRORS_TOTAL), &expected_labels) + .expect("connect-labelled general error metric should exist") + }; + assert!((counter_value - 1.0).abs() < f64::EPSILON); + } - assert_eq!(stats.udp4_errors_total(), 1); + #[tokio::test] + async fn should_label_a_connection_id_error_metric_with_qbittorrent_client_software() { + // Arrange + let stats_repository = Repository::new(); + let announce_request = AnnounceRequestBuilder::default() + .with_peer_id(PeerId(*b"-qB00000000000000001")) + .into(); + let expected_labels = LabelSet::from([ + (label_name!("client_software_name"), "QBitTorrent".to_string().into()), + (label_name!("client_software_version"), "0.0.0".to_string().into()), + ]); + + // Act + handle_event( + sample_ipv4_connection_context(), + Some(UdpRequestKind::Announce { announce_request }), + ErrorKind::ConnectionCookie("connection ID is invalid".to_string()), + &stats_repository, + CurrentClock::now(), + ) + .await; + + // Assert + let counter_value = { + let stats = stats_repository.get_stats().await; + stats + .metric_collection + .sum(&metric_name!(UDP_TRACKER_SERVER_CONNECTION_ID_ERRORS_TOTAL), &expected_labels) + .expect("QBitTorrent connection-ID-error metric should exist") + }; + assert!((counter_value - 1.0).abs() < f64::EPSILON); } } diff --git a/packages/udp-server/tests/server/contract.rs b/packages/udp-server/tests/server/contract.rs index 748538981..3c3054e65 100644 --- a/packages/udp-server/tests/server/contract.rs +++ b/packages/udp-server/tests/server/contract.rs @@ -15,10 +15,23 @@ use crate::server::asserts::get_error_response_message; const DEFAULT_UDP_TIMEOUT: Duration = Duration::from_secs(5); -const fn empty_udp_request() -> [u8; MAX_PACKET_SIZE] { +const fn empty_udp_datagram() -> [u8; MAX_PACKET_SIZE] { [0; MAX_PACKET_SIZE] } +async fn start_ephemeral_udp_tracker() -> torrust_tracker_udp_server::testing::environment::Started { + let configuration = configuration::ephemeral(); + let core_config = Arc::new(configuration.core.clone()); + let udp_tracker_config = Arc::new( + configuration + .udp_trackers + .expect("UDP test configuration should include a tracker")[0] + .clone(), + ); + + torrust_tracker_udp_server::testing::environment::Started::new(&core_config, &udp_tracker_config).await +} + async fn send_connection_request(transaction_id: TransactionId, client: &UdpTrackerClient) -> ConnectionId { let connect_request = ConnectRequest { transaction_id }; @@ -42,78 +55,70 @@ async fn send_connection_request(transaction_id: TransactionId, client: &UdpTrac async fn should_return_a_bad_request_response_when_the_client_sends_an_empty_request() { logging::setup(); - 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 env = torrust_tracker_udp_server::testing::environment::Started::new(&core_config, &udp_tracker_config).await; - - let client = match UdpTrackerClient::new(env.bind_address(), DEFAULT_UDP_TIMEOUT).await { - Ok(udp_client) => udp_client, - Err(err) => panic!("{err}"), - }; - - match client.client.send(&empty_udp_request()).await { - Ok(_) => (), - Err(err) => panic!("{err}"), - } - - let response = match client.client.receive().await { - Ok(response) => response, - Err(err) => panic!("{err}"), - }; - - let response = Response::parse_bytes(&response, true).unwrap(); - + // Arrange + let tracker = start_ephemeral_udp_tracker().await; + let client = UdpTrackerClient::new(tracker.bind_address(), DEFAULT_UDP_TIMEOUT) + .await + .expect("UDP client should connect to the ephemeral tracker"); + + // Act + client + .client + .send(&empty_udp_datagram()) + .await + .expect("UDP client should send the empty datagram"); + let response_bytes = client + .client + .receive() + .await + .expect("UDP tracker should respond to the empty datagram"); + let response = Response::parse_bytes(&response_bytes, true).expect("UDP tracker response should be valid"); + + // Assert assert!( get_error_response_message(&response) .unwrap() .contains("Protocol identifier missing") ); - env.stop().await; + tracker.stop().await; } mod receiving_a_connection_request { - use std::sync::Arc; - use torrust_tracker_client::udp::client::UdpTrackerClient; - use torrust_tracker_test_helpers::{configuration, logging}; + use torrust_tracker_test_helpers::logging; use torrust_tracker_udp_protocol::{ConnectRequest, TransactionId}; - use super::DEFAULT_UDP_TIMEOUT; + use super::{DEFAULT_UDP_TIMEOUT, start_ephemeral_udp_tracker}; use crate::server::asserts::is_connect_response; #[tokio::test] async fn should_return_a_connect_response() { logging::setup(); - 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 env = torrust_tracker_udp_server::testing::environment::Started::new(&core_config, &udp_tracker_config).await; - - let client = match UdpTrackerClient::new(env.bind_address(), DEFAULT_UDP_TIMEOUT).await { - Ok(udp_tracker_client) => udp_tracker_client, - Err(err) => panic!("{err}"), - }; - - let connect_request = ConnectRequest { - transaction_id: TransactionId::new(123), - }; + // Arrange + let tracker = start_ephemeral_udp_tracker().await; + let client = UdpTrackerClient::new(tracker.bind_address(), DEFAULT_UDP_TIMEOUT) + .await + .expect("UDP client should connect to the ephemeral tracker"); + let transaction_id = TransactionId::new(123); + let connect_request = ConnectRequest { transaction_id }; - match client.send(connect_request.into()).await { - Ok(_) => (), - Err(err) => panic!("{err}"), - } + // Act + client + .send(connect_request.into()) + .await + .expect("UDP client should send the connect request"); - let response = match client.receive().await { - Ok(response) => response, - Err(err) => panic!("{err}"), - }; + let response = client + .receive() + .await + .expect("UDP tracker should respond to the connect request"); - assert!(is_connect_response(&response, TransactionId::new(123))); + // Assert + assert!(is_connect_response(&response, transaction_id)); - env.stop().await; + tracker.stop().await; } }