diff --git a/.github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md b/.github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md index a89e0ff8d..58a3d12ae 100644 --- a/.github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md +++ b/.github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md @@ -103,6 +103,13 @@ fn it_should_parse_valid_config() { } ``` +## When Public APIs Return `Result` + +Return `Result` only when an operation can fail today, crosses an I/O boundary, or is a trait or port +with an existing or planned backend that can fail. Otherwise return a plain value; a real failure later is a +semver-signalled breaking change. Never use `Infallible` or an empty error enum as a placeholder, and mark +existing public error enums `#[non_exhaustive]` before first publish. See [ADR 20261005145329](../../../../../docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md). + ## Quick Checklist - [ ] Error type uses `thiserror::Error` derive diff --git a/docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md b/docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md new file mode 100644 index 000000000..3ea44be22 --- /dev/null +++ b/docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md @@ -0,0 +1,142 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - .github/skills/dev/planning/create-adr/SKILL.md + - .github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md + - "issue #2435" + - docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md + - packages/swarm-coordination-registry/src/swarm/registry.rs + - packages/tracker-core/src/torrent/repository/in_memory.rs +--- + + + +# Return `Result` Only for Concretely Fallible Public APIs + +## Scope + +This is a repository-level decision. It sets an API convention for every workspace package, +because every package will be published on crates.io and consumed independently of the tracker +(EPIC #1669, [independent package versioning](20260629000000_adopt_independent_package_versioning.md)). +The first application is the boundary between `swarm-coordination-registry` and `tracker-core`. + +## Description + +The swarm coordination `Registry` returned `Result<_, Error>` from ten methods with +`pub type Error = Infallible;`. No method could fail. Its main consumer, +`InMemoryTorrentRepository`, called `.expect(...)` on every result and documented a `# Panics` +section for a panic that could not happen. Reviewers read that as a reliability problem, and every +new method copied the pattern. + +The `Result` had been kept on purpose, to absorb future failures without a breaking change. The +first implementation of issue #2435 followed that intent through: it replaced `Infallible` with a +crate-owned, empty `#[non_exhaustive]` error enum and propagated it to every delivery layer. That +work showed the cost of the approach: + +- `SwarmRegistry` variants in `AnnounceError` and `ScrapeError`, a REST `StatsError` with a `500` + path, error handling in the torrent cleanup job, and 37 new `.unwrap()` calls in tests; +- all of it for an error that cannot occur, so none of those paths could be tested; +- and readers once again inferred a failure mode that does not exist, which was the original + problem. + +The deciding question was simple: can counting the swarms (`Registry::len`) ever fail? Nobody could +name a case. The counting methods that returned `Result` had an equally weak case: a different +backend would affect `len()` as much as `count_peers()`, and a backend that different would most +likely be a new abstraction with its own error type anyway. + +## Agreement + +A public API returns `Result` when at least one of these holds: + +1. **A failure can happen today**: I/O, parsing, validation, resource limits. +2. **The operation crosses an I/O boundary**: database, network, filesystem. +3. **It is a trait or port with an existing or planned backend that can fail.** The database + driver traits are an example. A "port" is a trait that defines a boundary other + implementations can back, such as the REST application port traits in the + [contract-first REST API architecture](20260623200526_adopt_contract-first_architecture_for_rest_api.md). + Being swappable is not enough: some backend that is real or planned must be able to fail. For + example, the auth-key and whitelist ports return `Result` because their implementation does + database I/O (conditions 1 and 2). The stats and torrent query ports return plain values, + because no backend that can fail exists or is planned; they change to `Result` when one is. + +Otherwise, it returns a plain value. If a real failure appears later, the signature changes to +`Result` as a semver-signalled breaking change: a `0.x` minor bump or a major bump after 1.0. The +compiler then lists every caller that must handle the error. + +Related rules: + +- **Never use `Result` or an empty error enum to reserve room for future failures.** + `Infallible` means "the error type for errors that can never happen". If an operation cannot + fail, it returns `T`. +- **Do not `expect` a workspace API's `Result` whose error cannot occur.** Change that API to + return a plain value instead, so the signature states the truth and no false `# Panics` section + is needed. A call-site `expect` on an API that can fail in general but not for this input, often + one we do not own (for example serializing plain data with `serde_json`), stays under the + `handle-errors-in-code` skill's Unwrap and Expect Policy, with the `# Panics` section that + `clippy::missing_panics_doc` requires. +- **Mark existing public error enums `#[non_exhaustive]` before their first publish.** Enums with + real variants gain variants as features grow (the reverted first attempt at #2435 had to add one + to two enums). With + `#[non_exhaustive]`, adding a variant is not a breaking change. This is the cheap, standard tool; + RFC 2008 names error types as its most common use. Derive only traits every future variant can + keep, because removing a derive is a breaking change. + +### Why Breaking Changes Are Acceptable + +The two options spread their costs differently: + +- A speculative `Result` costs every consumer, from the first release and forever: propagation code + and untestable error paths for an error that does not exist. +- A breaking change costs a one-time migration, only if the failure ever appears, and only for + callers of the changed method. Cargo treats `0.1` to `0.2` (or `1.x` to `2.0`) as incompatible, + so consumers adopt the change deliberately rather than being broken silently. + +Before a crate reaches 1.0, its public API should be reviewed against this rule method by method. + +### Alternatives Considered + +- **Keep `Result`.** Rejected: it states that failure is impossible while keeping a + `Result`, so consumers `expect` it, and replacing `Infallible` later changes the public API + anyway. +- **An empty `#[non_exhaustive]` error enum with full propagation.** Implemented first in #2435, + then reverted. Consumers cannot treat the type as uninhabited, so later variants are + non-breaking, but every consumer pays for the plumbing up front for a failure that may never + exist. See the Description. +- **A generic error type** such as `Box`. Rejected: it carries + the same speculative cost and also hides which failures are possible. + +### Consequences + +Positive: + +- Signatures state the truth: callers see `Result` only where something can fail. +- No error plumbing, `expect` calls, or `# Panics` sections exist for impossible failures. +- Error paths that do exist correspond to real failures and can be tested. + +Negative: + +- Introducing the first failure into an operation is a breaking change for its callers. +- The rule needs judgement. "Can this fail today, does it do I/O, or does a real or planned backend + fail?" is + usually clear, but borderline cases should be decided in review. + +## Affected Code + +- [`packages/swarm-coordination-registry/src/swarm/registry.rs`](../../packages/swarm-coordination-registry/src/swarm/registry.rs) +- [`packages/tracker-core/src/torrent/repository/in_memory.rs`](../../packages/tracker-core/src/torrent/repository/in_memory.rs) + +## Date + +2026-10-05 + +## References + +- Issue #2435 and its [specification](../issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md) +- Found during PR #2423 (issue #2406) review +- EPIC #1669 (package overhaul and publishing) +- [ADR: adopt independent package versioning](20260629000000_adopt_independent_package_versioning.md) +- [RFC 2008: `#[non_exhaustive]`](https://rust-lang.github.io/rfcs/2008-non-exhaustive.html) +- [`std::convert::Infallible`](https://doc.rust-lang.org/std/convert/enum.Infallible.html) +- [Cargo Book: SemVer compatibility](https://doc.rust-lang.org/cargo/reference/semver.html) diff --git a/docs/adrs/index.md b/docs/adrs/index.md index 55d7fb510..55677c0a6 100644 --- a/docs/adrs/index.md +++ b/docs/adrs/index.md @@ -46,6 +46,7 @@ supersession rules. | [20260926142648](20260926142648_adopt_self_hosted_hetzner_runner_for_container_tests.md) | 2026-09-26 | Adopt a self-hosted Hetzner runner for the container test job | Run the container test and Docker E2E jobs on one persistent Hetzner runner, accepting the fork-PR exposure under approval, 2FA, Dependabot and `main` routing, and cache-free publish builds. | | [20260929183441](20260929183441_build_container_from_positive_lists_with_external_only_dependency_cache.md) | 2026-09-29 | Build the container from positive lists with an external-only dependency cache | Use Cargo Chef's canonical `COPY . .` recipe stage (measured to cache identically to hand-maintained manifest/stub lists), keep the `--external-only` three-layer cook, make `.dockerignore` a default-deny allow-list, and select container test scope with `[workspace] default-members` instead of `--exclude` lists. | | [20261002173716](20261002173716_load_persisted_scrape_downloads_with_a_batched_uncached_lookup.md) | 2026-10-02 | Load persisted scrape downloads with a batched, uncached lookup | Announce and scrape share one persisted-downloads lookup; scrape loads all torrents absent from memory with one deduplicated `IN (...)` query, bounded at 100 info-hashes independently of protocol request limits, with no cache until metrics show a need. | +| [20261005145329](20261005145329_return_result_only_for_concretely_fallible_public_apis.md) | 2026-10-05 | Return `Result` only for concretely fallible public APIs | A public API returns `Result` only when it can fail today, crosses an I/O boundary, or is a trait or port with an existing or planned backend that can fail; otherwise it returns a plain value and a real failure later is a semver-signalled breaking change. No `Infallible` or empty error enums as placeholders; existing public error enums get `#[non_exhaustive]` before first publish. | ## ADR Lifecycle diff --git a/docs/issues/open/1669-overhaul-packages/EPIC.md b/docs/issues/open/1669-overhaul-packages/EPIC.md index 0446b4290..ae5d7b675 100644 --- a/docs/issues/open/1669-overhaul-packages/EPIC.md +++ b/docs/issues/open/1669-overhaul-packages/EPIC.md @@ -6,7 +6,7 @@ epic: null github-issue: 1669 spec-path: docs/issues/open/1669-overhaul-packages/EPIC.md epic-owner: josecelano -last-updated-utc: "2026-10-06 09:12" +last-updated-utc: "2026-10-06 10:40" semantic-links: skill-links: - create-issue @@ -18,6 +18,7 @@ semantic-links: - docs/issues/closed/1926-1669-si-32-define-package-versioning-strategy/ISSUE.md - docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md - docs/adrs/20260629000000_adopt_independent_package_versioning.md + - docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md - docs/adrs/index.md - docs/issues/open/1669-overhaul-packages/DECISIONS.md - AGENTS.md @@ -780,6 +781,17 @@ Each subsequent cycle produces one or more of: There is no predetermined end date or total subissue count. +### Pre-publish API checklist + +Before the first crates.io publish of each package, audit its public error API +([ADR 20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md)): + +1. Mark every public error enum that has real variants `#[non_exhaustive]`, so that adding a + variant later is not a breaking change. +2. Derive only the traits every future variant can keep; removing a derive is a breaking change. +3. Confirm that no public API returns `Result<_, Infallible>` or an empty error enum as a + placeholder for future failures. + ## Open Questions These questions do not block starting work, but need answers before specific subissues can @@ -932,12 +944,16 @@ Previously referenced tools (screenshots from CodeScene already in the issue com #1860, #1861, #1864, SI-23 to SI-28, SI-34 and SI-35 as done; replaced `rest-api-core` with the #1938 REST API packages; regenerated the dependency lists from `cargo metadata`; recorded that the baseline analysis is largely done but not yet tracked in a GitHub issue. +- 2026-10-05 21:07 UTC - Copilot - Added the Pre-publish API checklist (`#[non_exhaustive]` + audit of public error enums) from issue #2435 and its ADR, with maintainer approval. - 2026-10-06 08:41 UTC - GitHub Copilot - Applied PR #2441 review feedback: added Details rows for the newly listed subissues and SI-29; replaced the publication Yes/No column with the latest crates.io version and added the published root `torrust-tracker` crate; marked `bittorrent-primitives` as superseded and archived. - 2026-10-06 09:12 UTC - GitHub Copilot - Stated that the published count excludes `bittorrent-primitives`, and marked the repository as archived in the desired-state notes. +- 2026-10-06 10:40 UTC - Copilot - Moved the Pre-publish API checklist from Open Questions to + Delivery Strategy, because it is a decided procedure (PR #2445 review finding F7). ## Acceptance Criteria diff --git a/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md index 08e1c8fa7..a2c5d7492 100644 --- a/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md +++ b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md @@ -2,14 +2,14 @@ schema-version: 1 doc-type: issue issue-type: task -status: planned +status: in-progress priority: p3 epic: null github-issue: 2435 spec-path: docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md -branch: "2435-remove-misleading-panics-in-in-memory-torrent-repository-spec" -related-pr: null -last-updated-utc: "2026-10-05 08:43" +branch: "2435-remove-misleading-panics-in-in-memory-torrent-repository" +related-pr: 2445 +last-updated-utc: "2026-10-06 11:20" semantic-links: skill-links: - create-issue @@ -49,16 +49,70 @@ Found during PR #2423 (issue #2406) review. - **B.** Remove `Result` from the infallible registry methods so they return values directly. - **C.** Introduce a real registry error and propagate it through `InMemoryTorrentRepository` to its callers. - Implement the chosen option and update the affected doc comments. +- Fix the registry's own misleading `# Errors` sections (they claim a panic when a lock "cannot be acquired"; `tokio::sync::Mutex::lock` cannot fail). +- Write an ADR for the error-signature policy of public packages. +- Undo the option C work (T4 to T6) after the switch to option B (see [Decision Revision (T7)](#decision-revision-t7)). ### Out of Scope -- Other `expect`/`unwrap` uses outside `InMemoryTorrentRepository` and the registry methods it calls. +- Other `expect`/`unwrap` uses outside `InMemoryTorrentRepository` and the registry methods it calls (for example the `MetricCollection::merge` `expect` calls in the REST labeled-stats adapter). +- `.unwrap()`/`.expect()` on registry or repository results in test code, test-support modules (`src/testing/`), examples, and benchmarks. +- Adding `#[non_exhaustive]` to existing public error enums before the first crates.io publish. This belongs to the package-publishing work in EPIC #1669; this issue only adds the checklist item to the EPIC (T11, maintainer-approved). - Changing swarm-coordination behavior. ## Architectural Decisions -- Related ADRs: none known. -- ADRs to create: only if option C introduces a new error-propagation contract between `swarm-coordination-registry` and `tracker-core`. +- Related ADRs: [independent package versioning](../../../adrs/20260629000000_adopt_independent_package_versioning.md). +- ADR created: [20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md), recording when public package APIs return `Result`. It is root-scoped because every workspace package will be published and consumed independently (EPIC #1669). + +## Decision Revision (T7) + +Maintainer decision, 2026-10-05 (supersedes [Decision (T2)](#decision-t2)): **option B**. The registry methods return plain values, and the `Error` type is deleted. + +Why the decision changed: after T4 to T6 were implemented, the maintainer asked whether counting the swarms (`Registry::len`) could ever fail, and neither of us could imagine a case. The argument for the counting methods returning `Result` was just as weak: a different backend would affect `len()` equally, and the only remaining candidate, a future lock-acquisition timeout, is speculative. The maintainer prefers a breaking change over filling the code with `Result` "just in case". + +The cost of option C was already visible on the branch: `SwarmRegistry` variants in `AnnounceError` and `ScrapeError`, a `StatsError` and a REST `500` path, error handling in the cleanup job, and 37 new `.unwrap()` calls in tests, all for an error that cannot occur and cannot be tested. The plumbing also recreated the original problem: readers infer a failure mode that does not exist. + +Publishing context: every workspace package will be published on crates.io within weeks (EPIC #1669), with new crates starting at `0.x`. A breaking change then costs consumers a semver-signalled migration (for example `0.1` to `0.2`), paid once and only if a real failure ever appears. A speculative `Result` costs every consumer from day one. + +Policy (recorded in the ADR): a public API returns `Result` when at least one of these holds: + +1. a failure can happen today (I/O, parsing, validation, limits); +2. the operation crosses an I/O boundary (database, network, filesystem); +3. it is a trait or port designed for swappable backends, and a realistic backend can fail. + +Otherwise it returns a plain value, and a real failure is introduced later as a semver-signalled breaking change. Existing public error enums that have real variants should get `#[non_exhaustive]` before their first publish, so that adding variants is not a breaking change. That is a #1669 checklist item. + +Shape: + +- `Registry`: the 10 methods return `T` directly; `pub enum Error` and its `compile_fail` doctest are deleted; the `# Errors` sections go away. +- `InMemoryTorrentRepository`: plain delegation with no `expect` and no `# Panics`. +- Revert T6 (propagation) and T5 (REST `StatsError` and `500` path) with `git revert` commits, keeping the history visible. + +## Decision (T2) + +Superseded by [Decision Revision (T7)](#decision-revision-t7). Kept as history. + +Maintainer decision, 2026-10-05: **option C, with an uninhabited `#[non_exhaustive]` error type and full propagation**. + +Rationale (maintainer): every package in this workspace is public and usable independently of the tracker. Implementations often change and suddenly need to return an error. Unless an operation can never fail, it should return `Result`, so consumers are ready to handle the error case when it appears. Consumers who investigate can see that no errors happen today. The decision must be documented and honest: if the API returns `Result`, callers propagate it instead of hiding it behind `expect`. + +Why not `Infallible`: `Result<_, Infallible>` does not give that forward compatibility. Consumers can write `let Ok(v) = ...;`, `match e {}`, or `impl From for MyError`, and all of these break when the alias becomes a real type. Consumers that `.unwrap()` keep compiling and silently start panicking. + +Chosen shape: + +- `swarm-coordination-registry` replaces `pub type Error = Infallible` with `#[non_exhaustive] pub enum Error {}` (implementing `Debug`, `Clone`, `Display`, and `std::error::Error`). Inside the registry it is uninhabited, so internal code stays trivial. Other crates cannot treat it as uninhabited (they cannot use exhaustive patterns to prove that `Err` is impossible), so their code stays valid when variants are added, and adding variants later is not a breaking change. Verified on 2026-10-05 with a two-crate scratch build: `let Ok(v) = lib::count();` compiles in the defining crate and fails with `E0005: pattern Err(_) not covered` in the consumer crate. +- `InMemoryTorrentRepository` returns `Result` from the 10 registry-backed methods; no `expect`, no `# Panics`. +- Propagation boundaries (all the way to delivery layers): + - Announce: `AnnounceError` gains a `SwarmRegistry` variant. HTTP maps it through the existing `TrackerCoreError` → `failure_reason` path. UDP maps it to `ErrorKind::InternalServer` in `udp-server/src/event.rs`. + - Scrape: `ScrapeError` gains a `SwarmRegistry` variant, mapped the same way as announce. + - Torrent cleanup: `TorrentsManager::cleanup_torrents` returns `Result`. The cleanup job runner logs the error and keeps running on the next tick (`Completion` has no error variant, and one failed pass must not stop future cleanups). + - REST stats: `StatsQueryPort::get_stats` and `StatsApiService::get_stats` return `Result` with a `rest-api-application`-owned port error. The `get_stats_handler` responds `500` through the existing `unhandled_rejection_response` pattern. + - `udp-core` and `udp-server` `statistics::services::get_metrics` return `Result` (public functions with no production caller; their callers are their own tests). + +Rejected: option A (keeps the misleading `Infallible` and hides the result in one consumer) and option B (removes `Result`, contrary to the forward-compatibility policy above). + +Test consequence: the registry error has no values today, so registry-originated error paths cannot be exercised at runtime. AC3 is guarded by a `compile_fail` doctest on the registry error. The REST `500` mapping is tested with a stub `StatsQueryPort` returning the constructible port error. ## Design and Ownership Review @@ -70,7 +124,7 @@ Not applicable. The bug rule in the `create-issue` and `fix-bug` skills covers o ## Regression Test Strategy -Not applicable. Options A and B are compile-time guarantees; option C needs tests for the propagated error path. +Option B was chosen (T7). It is a compile-time guarantee: if a registry operation ever becomes fallible, its signature changes to `Result` and every caller fails to compile until it handles the error. No runtime test can exercise a failure that does not exist. ## Implementation Plan @@ -78,16 +132,41 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | ID | Status | Task | Notes / Expected Output | | --- | --- | --- | --- | -| T1 | TODO | Inventory fallible registry methods and repository callers | List of methods, callers, and whether any can fail | -| T2 | TODO | Choose option A, B, or C | Record the decision, rationale, and maintainer approval here before implementation | -| T3 | TODO | Implement the chosen option | No `expect` on registry results in `in_memory.rs`; doc comments match behavior | +| T1 | DONE | Inventory fallible registry methods and repository callers | See [T1 Inventory](#t1-inventory) | +| T2 | DONE | Choose option A, B, or C | Option C; see [Decision (T2)](#decision-t2) (superseded by T7) | +| T3 | DONE | Write the ADR | Written for option C, then renamed and rewritten in T8; the final file is `docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md`, plus its index row and `handle-errors-in-code` skill link | +| T4 | DONE | Registry error type | `#[non_exhaustive] pub enum Error {}`, honest `# Errors` docs, `compile_fail` doctest | +| T5 | DONE | REST stats port returns `Result` | `StatsError` in `rest-api-protocol` (same pattern as `WhitelistError`); `StatsQueryPort`/`StatsApiService::get_stats` return `Result`; handler responds `500` via `failed_to_get_stats_response`; stub-port handler test | +| T6 | DONE | Propagate through `tracker-core` and delivery layers | Repository returns `Result`; `AnnounceError`/`ScrapeError::SwarmRegistry`; `TorrentsManager::cleanup_torrents` returns `Result` and the job logs `tracing::error!`; UDP `ErrorKind::InternalServer`; `udp-core`/`udp-server` `get_metrics` return `Result`; REST adapter maps to `StatsError` | +| T7 | DONE | Revise the decision | Option B; see [Decision Revision (T7)](#decision-revision-t7) | +| T8 | DONE | Rewrite the ADR for the revised policy | `docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md` (same timestamp, new slug; not merged yet); index row and `handle-errors-in-code` skill updated | +| T9 | DONE | Revert T6 and T5 | Two `git revert` commits (T6, then T5); spec edits from those commits kept; each revert compiled with `cargo check --workspace --all-targets --all-features` | +| T10 | DONE | Registry returns plain values | `Error`, its `Display` impl, the doctest, and all `# Errors` sections deleted; 62 test `.unwrap()` calls in `registry.rs`, plus one each in `statistics/mod.rs` and `tracker-core/tests/common/test_env.rs`, removed; `in_memory.rs` is plain delegation | +| T11 | DONE | Draft the #1669 pre-publish checklist item | Maintainer approved; added as the "Pre-publish API checklist" section of EPIC #1669 | + +### T1 Inventory + +Registry methods returning `Result<_, Error>` (all infallible today): `handle_announcement`, `get_swarm_metadata`, `get_swarm_metadata_or_default`, `get_peers_peers_excluding`, `get_swarm_peers`, `remove_inactive_peers`, `remove_peerless_torrents`, `get_aggregate_swarm_metadata`, `count_peerless_torrents`, `count_peers`. + +- `tracker-core`: `in_memory.rs` is the only caller, with one `expect` per method (10 in total) and a matching `# Panics` section. +- Other direct callers: about 63 `.unwrap()` calls in the registry's own tests and `statistics/mod.rs` tests. +- Production callers of the repository methods: `AnnounceHandler`, `ScrapeHandler`, `TorrentsManager` (cleanup and metadata logging), `TrackerStatsAdapter::get_stats` (REST), and `udp-core`/`udp-server` `statistics::services::get_metrics`. +- None of these calls can fail today: every registry method returns `Ok`, and `tokio::sync::Mutex::lock` is infallible. ## Commit Points | Task | Coherent change set | Commit policy | | --- | --- | --- | | T2 | Decision recorded in the spec | Commit after maintainer approval | -| T3 | Production change and doc updates | Commit after focused validation and review | +| T3 | ADR and index row | One `docs(adrs)` commit | +| T4 | Registry error type and docs | One commit; the workspace still compiles because callers only need `Debug` for `expect` | +| T5 | REST stats port `Result` | One commit; the adapter returns `Ok` until T6 | +| T6 | Propagation through `tracker-core` and delivery layers | One commit (signature changes must land together to compile) | +| T7 | Revised decision in the spec | One `docs(issues)` commit | +| T8 | Rewritten ADR, index row, and skill | One `docs(adrs)` commit | +| T9 | One revert commit per reverted task (T6, then T5) | Each revert compiles on its own | +| T10 | Registry plain values and all callers | One commit | +| T11 | EPIC #1669 pre-publish checklist | One `docs(issues)` commit after maintainer approval | ## Progress Tracking @@ -96,12 +175,13 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - [x] Folder-style spec drafted in `docs/issues/drafts/remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md` - [x] Spec reviewed and approved by user/maintainer - [x] GitHub issue created and issue number added to this spec -- [ ] Implementation completed -- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) -- [ ] Manual verification scenarios executed and recorded in issue-local `manual-verification-evidence.md` -- [ ] Acceptance criteria reviewed after implementation and updated with evidence -- [ ] Evidence-based implementation completion review recorded: issue-local retrospective created for material discoveries, or progress log states why none was needed -- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [x] Implementation completed +- [x] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks): pre-commit (`linter all`, doc tests), the full stable test suite, and the pre-push hook (nightly format/check/docs and all tests) passed before the first PR #2445 push +- [x] Manual verification scenarios: not applicable (maintainer decision, 2026-10-05; compile-time and automated tests are sufficient) +- [x] Acceptance criteria reviewed after implementation and updated with evidence +- [x] Evidence-based implementation completion review recorded: issue-local retrospective created for material discoveries, or progress log states why none was needed +- [x] Reviewer validated acceptance criteria and updated checkboxes +- [x] Independent reviewer reports recorded in issue-local `agent-review-reports.md` when reviewers received this folder-style specification - [ ] Committer verified spec progress is up to date before commit - [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` @@ -111,33 +191,43 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - 2026-10-03 07:58 UTC - Copilot - Maintainer approved the draft; the A/B/C decision is deferred until the draft moves to `docs/issues/open/`. Committed as a draft in PR #2423; no GitHub issue yet. - 2026-10-05 07:05 UTC - Copilot - Maintainer confirmed the task classification (no runtime defect). Created GitHub issue #2435 and moved the spec to `docs/issues/open/` on a spec-only branch; the A/B/C decision remains open for T2. - 2026-10-05 08:43 UTC - Copilot - Reworded the Bug-Fix Process paragraph after PR #2436 review (F1): the classification is recorded as outside the bug rule's scope (no observable behavior), not as an exception to it, and AC3 is named as the regression protection. +- 2026-10-05 14:26 UTC - Copilot - Created implementation branch `2435-remove-misleading-panics-in-in-memory-torrent-repository`. T1 inventory recorded. Maintainer chose option C (keep `Result` for forward compatibility of public packages) with a `#[non_exhaustive]` uninhabited error instead of `Infallible`, full propagation to delivery layers, a root ADR, and registry `# Errors` doc fixes. Manual scenario M1 dropped. Work stops at local commits (no push or PR). +- 2026-10-05 15:01 UTC - Copilot - T3 committed (ADR). T4: replaced the `Infallible` alias with `#[non_exhaustive] pub enum Error {}`. `Display` uses the same `match *self {}` as `std`'s `Display for Infallible`, with a documented `expect` for `clippy::uninhabited_references`. The registry crate does not depend on `thiserror`, and adding it for one empty enum was not justified. +- 2026-10-05 15:09 UTC - Copilot - T5: the port error lives in `rest-api-protocol` (`StatsError::TorrentRepository(String)`), following the existing `WhitelistError` boundary pattern, so the application layer does not depend on tracker internals. Added `async-trait = "0.1"` as an `axum-rest-api-server` dev-dependency (already in the lockfile and used by sibling crates) for the stub port. Mutation proof: returning `ok_response()` from the error branch made the test fail (`left: 200, right: 500`); restored by hand. +- 2026-10-05 15:49 UTC - Copilot - T6: propagated the registry error through `tracker-core` and every production caller (see the T6 row). Test seeding helpers in `axum-http-server`/`axum-rest-api-server` `src/testing/environment.rs` use a documented `expect` (test support, out of scope). Test code uses `.unwrap()`. Verified that all 37 added `unwrap`/`expect` lines sit in `#[cfg(test)]` modules or test targets, with none in production. `cargo clippy --workspace --all-targets --all-features` is clean; `cargo test --tests --benches --examples --workspace --all-targets --all-features` passed 2976 tests with 0 failures. Completion review: no retrospective needed. The one material discovery (that `Infallible` defeats forward compatibility) changed the design before implementation and is recorded in the Decision (T2) section and the ADR; the `clippy::uninhabited_references` workaround is recorded in the T4 entry. +- 2026-10-05 18:03 UTC - Copilot - Revised the ADR after an external AI review; the decision is unchanged. Renamed it to "Use Crate-Owned Non-Exhaustive Errors for Potentially Fallible Public APIs". Revisions: separated the abstraction's semantics from the current implementation's capabilities; explained that `Infallible` states the wrong contract; replaced "forced to handle `Err`" with "cannot treat the error as uninhabited" (consumers can still `unwrap`); limited the scope to independently consumed API boundaries; presented the empty-enum pattern as a repository convention built on established practice rather than a standard idiom; added a generic-error alternative and expanded consequences; cited RFC 2008, C-GOOD-ERR, the Reference, and `std::convert::Infallible` (all checked against the live pages). Following the new derive guidance, dropped `PartialEq, Eq` from the registry `Error` because no caller used them and a future variant may not support them. Added an intent comment to the `compile_fail` doctest. +- 2026-10-05 18:42 UTC - Copilot - Maintainer reversed the decision to option B (T7); see [Decision Revision (T7)](#decision-revision-t7). Trigger: no plausible failure for `Registry::len`, and an equally weak case for the counting methods. Packages will be published within weeks (EPIC #1669), so the trade-off was re-assessed in that context: a semver-signalled breaking change, paid once if a failure ever appears, is cheaper than a speculative `Result` every consumer pays for. Reopened AC3, AC5, and AC6 and reworded them for option B. Plan: rewrite the ADR (T8), revert T6 and T5 with `git revert` (T9), make the registry methods return plain values (T10), and draft the #1669 checklist item (T11). +- 2026-10-05 19:56 UTC - Copilot - T8 to T10 done. The ADR was rewritten, T6 and T5 were reverted with `git revert` (the spec conflicts were resolved by keeping the current spec), and the registry methods now return plain values. A subagent did the mechanical removal of 62 test `.unwrap()` calls in `registry.rs`, using file-edit tools and verified by the compiler. Also removed the false `# Returns ... true` sections on both `handle_announcement` methods, which return `()`. Net code diff against `develop`: four files, +96/-263. `cargo clippy --workspace --all-targets --all-features` is clean; `cargo test --tests --benches --examples --workspace --all-targets --all-features` passed 2975 tests with 0 failures (one fewer than T6 because the reverted REST stub-port test is gone). Completion review: the reversal is the material discovery, and its lesson (do not reserve `Result` for speculative failures) is recorded permanently in the ADR's Description and Alternatives and in the T7 decision, so no separate retrospective file was created. +- 2026-10-06 09:16 UTC - Copilot - Rebased onto `torrust/develop` (38 commits, no conflicts). The two `revert(...)` commit messages cite pre-rebase commit ids that no longer exist; the reverted commits are "propagate swarm registry errors instead of expecting them" (T6) and "return a 500 when the tracker stats cannot be collected" (T5). A Task Reviewer review returned PASS WITH FINDINGS. Fixes: added `#[must_use]` to the seven registry query methods, which lost the unused-value warning when they stopped returning `Result`; linked the ADR from the Architectural Decisions and References sections; marked T2 as superseded; added the T11 commit point; removed `examples/bench_peers.rs` from the T1 inventory (it uses only `Coordinator`); and reworded the ADR's mention of the reverted enum variants. +- 2026-10-06 09:20 UTC - Copilot - Following the task review's major finding and the maintainer's request, added [`implementation-retrospective.md`](implementation-retrospective.md) covering the reversal from option C to option B. It supersedes the earlier "no separate retrospective" note in the 2026-10-05 19:56 UTC entry. +- 2026-10-06 10:40 UTC - Copilot - Opened PR #2445. Processing its first review round (da2ce7 F1 to F7 and four Copilot findings), with the maintainer approving the dispositions. The ADR's no-`expect` rule is scoped to workspace APIs whose error cannot occur (F1). Condition 3 now requires an existing or planned backend that can fail, and "port" is defined (F4); the T7 policy wording above is kept as the decision record, and the ADR is authoritative. Added ADR back-links in the code (F2), removed three false `# Panics` sections from `torrent/services.rs` (F5), moved the EPIC checklist to Delivery Strategy (F7), recorded the task review in [`agent-review-reports.md`](agent-review-reports.md) with its checkpoint (F3), scoped the AC5 command (F6), named the final ADR file in the T3 row, marked the pre-push checkpoint done, and set `related-pr`. No `#[must_use]` on the two `remove_*` counts: every caller discards them on purpose. The PR audit record is `docs/pr-reviews/pr-2445-review/PR-REVIEW.md`. +- 2026-10-06 11:20 UTC - Copilot - Round 2 of the PR #2445 review (F12 to F14). Restored three EPIC #1669 log lines that the rebase conflict resolution had dropped (F12). Corrected the AC5 and retrospective file lists, which became stale when the F5 fix touched `torrent/services.rs` (F13). Scoped F10's verification grep in the audit (F14). ## Acceptance Criteria -- [ ] AC1: No method of `InMemoryTorrentRepository` documents a panic that cannot occur. -- [ ] AC2: No method of `InMemoryTorrentRepository` calls `expect` or `unwrap` on a registry result. -- [ ] AC3: If the registry gains a real error variant, the repository fails to compile or propagates the error, rather than panicking. -- [ ] `linter all` exits with code `0` -- [ ] Relevant tests pass -- [ ] Manual verification scenarios are executed and documented in issue-local `manual-verification-evidence.md` -- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior -- [ ] Documentation is updated when behavior/workflow changes +- [x] AC1: No method of `InMemoryTorrentRepository` documents a panic that cannot occur. +- [x] AC2: No method of `InMemoryTorrentRepository` calls `expect` or `unwrap` on a registry result. +- [x] AC3: If the registry gains a real error variant, the repository fails to compile or propagates the error, rather than panicking. +- [x] `linter all` exits with code `0` +- [x] AC4: The registry `# Errors` docs no longer claim a lock-acquisition failure. +- [x] AC5: The infallible registry methods return plain values; no `Result`, error variant, or error-response path exists for an error that cannot occur. +- [x] AC6: The ADR records when public package APIs return `Result`. +- [x] Relevant tests pass +- [x] Acceptance criteria are re-reviewed after implementation and reflect actual behavior +- [x] Documentation is updated when behavior/workflow changes ## Verification Plan ### Automatic Checks - `linter all` -- `cargo test -p torrust-tracker-core -p torrust-tracker-swarm-coordination-registry` +- `cargo test --tests --benches --examples --workspace --all-targets --all-features` (signatures change across several packages) +- `cargo test --doc --workspace` - Pre-push checks ### Manual Verification Scenarios -Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. - -| ID | Scenario | Human-oriented command/steps | Expected Result | Status | Evidence | -| --- | --- | --- | --- | --- | --- | -| M1 | Tracker smoke test | Start a local tracker, announce and scrape one torrent over UDP and HTTP with `tracker_client` | Normal announce and scrape responses; no panics in the logs | TODO | `manual-verification-evidence.md` section V1 | +None. Maintainer decision on 2026-10-05: the change alters types and error plumbing only; compile-time checks and automated tests are sufficient. ### Disposable Verification Scripts @@ -147,14 +237,17 @@ None planned. | AC ID | Status (`TODO`/`DONE`) | Evidence | | --- | --- | --- | -| AC1 | TODO | Doc comments in `in_memory.rs` | -| AC2 | TODO | `grep` of `in_memory.rs` | -| AC3 | TODO | Chosen option and its compile-time or test evidence | +| AC1 | DONE | `grep -nE 'expect\(\|unwrap\(\|# Panics\|# Errors\|Result' in_memory.rs` finds nothing | +| AC2 | DONE | Same `grep`; every method is plain delegation | +| AC3 | DONE | Option B: the registry signatures are plain values, so introducing an error changes them and every caller fails to compile | +| AC4 | DONE | Corrected in T4, then removed entirely with the `# Errors` sections in T10 | +| AC5 | DONE | `rg 'registry::Error\|SwarmRegistry\|StatsError' -- packages src` finds nothing (unscoped, it also matches prose about the reverted work in the ADR, this spec, and the retrospective); the net code diff against `develop` touches only `registry.rs`, `in_memory.rs`, `statistics/mod.rs`, `tracker-core/tests/common/test_env.rs`, and `tracker-core/src/torrent/services.rs` (doc-only: three false `# Panics` sections removed) (`git diff --stat torrust/develop -- packages src`) | +| AC6 | DONE | [ADR 20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md) and its index row | ## Risks and Trade-offs -- Option B or C changes public signatures of `swarm-coordination-registry`; check downstream callers and benchmarks. -- Option A keeps the `Result` wrapper; it is the smallest change but leaves an unusual API shape. +- Option B changes public `swarm-coordination-registry` signatures (`Result` to `T`) and deletes `Error`. The crate is unpublished, and every caller is in this workspace. +- If a registry operation becomes fallible later, its signature changes again. That is accepted as a semver-signalled breaking change (see the ADR). ## Implementation Completion Review @@ -162,7 +255,7 @@ After implementation, compare the result with this specification. Record invalidated assumptions, material design changes, unexpected validation findings, and reusable lessons. -- Retrospective: `Not yet assessed` +- Retrospective: [`implementation-retrospective.md`](implementation-retrospective.md) - If needed, create `implementation-retrospective.md` from the repository template at `docs/templates/IMPLEMENTATION-RETROSPECTIVE.md` in this issue specification's directory. @@ -173,4 +266,4 @@ findings, and reusable lessons. - Related issues: #2406 - Related PRs: #2423 -- Related ADRs: none +- Related ADRs: [20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md), [20260629000000](../../../adrs/20260629000000_adopt_independent_package_versioning.md) diff --git a/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/agent-review-reports.md b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/agent-review-reports.md new file mode 100644 index 000000000..2edfb82a7 --- /dev/null +++ b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/agent-review-reports.md @@ -0,0 +1,53 @@ +--- +semantic-links: + related-artifacts: + - .github/agents/task-reviewer.agent.md + - docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md +--- + +# Agent Review Reports - Remove Misleading Panics From the In-Memory Torrent Repository + +> Append one completed independent-review entry at a time. Do not modify, reorder, or remove +> earlier entries. A correction is a new entry that names the earlier conclusion. + +## Reports + +### 2026-10-06 09:16 UTC - Task Reviewer + +- Invocation scope: AC1 to AC6 and the generic acceptance criteria of issue #2435 (option B final + state), the net code diff against `torrust/develop`, the ADR with its index row and skill link, + the EPIC #1669 checklist, and repository conventions. Recorded after the fact, following PR #2445 + review finding F3; the report text below summarizes the reviewer's returned result. +- Inputs: `ISSUE.md`, `git diff torrust/develop...HEAD`, `git log torrust/develop..HEAD`, + `docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md`, + `docs/adrs/index.md`, `.github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md`, + `docs/issues/open/1669-overhaul-packages/EPIC.md`. +- Evidence: + - Net code diff: four files (`registry.rs`, `statistics/mod.rs`, `in_memory.rs`, + `tracker-core/tests/common/test_env.rs`). + - AC1/AC2: searching `in_memory.rs` for `expect(`, `unwrap(`, `# Panics`, `# Errors`, and + `Result` found nothing. + - AC4/AC5: no `Error`, `Infallible`, `Result`, `# Errors`, or `# Panics` remains in `registry.rs`; + no `registry::Error`, `SwarmRegistry`, or `StatsError` in workspace Rust files. + - `linter all`: exit code 0. + - `cargo test -p torrust-tracker-swarm-coordination-registry -p torrust-tracker-core`: 151 + 9 + + 110 + 15 doc tests passed, 0 failed. + - Every commit is a signed Conventional Commit. +- Findings: + - Major: no `implementation-retrospective.md`, although the progress log called the option C to + option B reversal the material discovery. + - Minor: the two `revert(...)` commit messages cite pre-rebase commit ids. + - Minor: the spec said "Related ADRs: none" and "ADRs to create" after the ADR existed. + - Minor: the seven registry query methods lost the unused-value warning when they stopped + returning `Result` (no `#[must_use]`). + - Nit: the T1 inventory listed `examples/bench_peers.rs`, which uses only `Coordinator`. + - Nit: the T2 row did not point to its supersession by T7, and the T11 commit point was missing. + - Nit: the ADR's mention of the enum variants added in the reverted attempt read as if they + existed in the final code. +- Verdict: REVIEW PASSED (returned as PASS WITH FINDINGS) +- Follow-up actions: + - All findings addressed: `fix(swarm-coordination-registry): mark registry query methods must_use`, + `docs(issues): address the #2435 task review findings`, + `docs(issues): add the #2435 implementation retrospective`, and + `docs(issues): cite #2435 branch commits by subject instead of id`. The revert-message ids are + recorded in the spec's progress log instead of rewriting history. diff --git a/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/implementation-retrospective.md b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/implementation-retrospective.md new file mode 100644 index 000000000..c2f563bef --- /dev/null +++ b/docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/implementation-retrospective.md @@ -0,0 +1,116 @@ +--- +semantic-links: + skill-links: + - write-markdown-docs + related-artifacts: + - docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md + - docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md + - .github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md + - packages/swarm-coordination-registry/src/swarm/registry.rs +--- + +# Implementation Retrospective — Remove Misleading Panics From the In-Memory Torrent Repository + +## Purpose + +Record evidence-based process improvements discovered while implementing issue #2435. This is a +blameless review of the implementation approach; it does not replace acceptance-criteria +verification. + +## Outcome + +The ten swarm registry methods that could never fail now return plain values, and the registry +`Error` type is gone. `InMemoryTorrentRepository` passes calls straight through, with no `expect` +and no `# Panics` sections. The seven read-only registry queries are `#[must_use]`. The net code +diff against `develop` is five files, mostly deletions; one of them, `torrent/services.rs`, only +loses three false `# Panics` doc sections (PR #2445 review). + +The policy is recorded in +[ADR 20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md): +return `Result` only when an operation can fail today, crosses an I/O boundary, or is a trait or +port with an existing or planned backend that can fail. EPIC #1669 gained a pre-publish checklist to audit +public error enums for `#[non_exhaustive]`. + +Validation: `cargo clippy --workspace --all-targets --all-features` is clean, the full stable test +suite passes (2975 tests), `linter all` passes, and an independent task review returned PASS WITH +FINDINGS. All findings are addressed or decided. + +## What Went Well + +1. **Small, coherent commits made the reversal cheap.** Each option C step (ADR, error type, REST + path, propagation) was its own commit, so undoing it took two `git revert` commits instead of + hand edits. The history keeps both the attempt and the correction. +2. **Claims were verified before they were recorded.** A two-crate scratch build confirmed how + `#[non_exhaustive]` behaves across crates; the cited references were checked against the live + pages; the new REST test was mutation-proven. +3. **Independent review caught a real regression.** Removing `Result` also removed the unused-value + warning on the query methods. The task review flagged it, and `#[must_use]` restored it. +4. **The spec kept the reasoning.** The superseded T2 decision, the T7 revision, and the progress + log show why the design changed, not just that it did. + +## What Changed During Implementation + +The decision changed from option C (keep `Result` with an uninhabited `#[non_exhaustive]` error and +propagate it to every delivery layer) to option B (return plain values). + +- Option C was fully implemented in three commits ("replace the Infallible error alias with a + non-exhaustive empty enum", "return a 500 when the tracker stats cannot be collected", and + "propagate swarm registry errors instead of expecting them"): `SwarmRegistry` + variants in `AnnounceError` and `ScrapeError`, a REST `StatsError` with a `500` path, error + handling in the cleanup job, and 37 new `.unwrap()` calls in tests. +- The ADR was then refined after an external AI review ("sharpen the non-exhaustive error ADR + around abstraction semantics"). The review sharpened the wording but did not question the + premise. +- The premise broke on one concrete question from the maintainer: can counting the swarms + (`Registry::len`) ever fail? Nobody could name a case, and the case for the counting methods that + returned `Result` was just as weak. The maintainer reversed the decision (T7). The option C + commits were reverted with two `revert(...)` commits, and the registry was changed to return + plain values ("return plain values from infallible registry methods"). + +## Root Cause + +The T2 decision was framed as "how should the code keep `Result` for future errors?" (which error +type, how far to propagate) before testing the premise that each operation could plausibly fail. +No step asked for a concrete, plausible failure scenario per method. + +Three things let the premise through: + +- **The general argument was persuasive in the abstract.** "Implementations change; public crates + should be ready" is true in general, so it was accepted without being applied to a single method. +- **The cost was not put next to each option.** The propagation inventory (every production caller + and delivery boundary) was gathered before implementation but not set against option B's cost. + The plumbing cost only became concrete once it was on the branch. +- **The reviews checked how the decision was argued, not whether it held.** The external ADR review + improved precision without testing the decision against a real method. + +## Improvements for Future Work + +1. **Before keeping `Result` for future failures, name one plausible failure per operation.** If + none can be named (the `len()` test), the operation returns a plain value. The ADR's three + conditions now encode this; reviewers should ask the question explicitly when a public signature + returns `Result` without a visible failure source. +2. **Put each option's concrete cost next to it when asking for a decision.** List the signatures + that change, the new error variants, the untestable paths, and the test churn per option, so the + trade-off is visible before implementation rather than after. +3. **Check an abstract design argument against one concrete case before agreeing.** When a + maintainer or reviewer offers a general rationale, try it on a specific method first; a + one-question check would have saved the option C implementation and its reverts. + +## Avoiding Overcorrection + +- Do not ban `Result` on infallible implementations of ports or traits. A port with an existing or + planned backend that can fail (for example the database driver traits) correctly returns `Result` + even when one implementation cannot fail. Being swappable alone does not qualify (ADR condition 3). +- Do not require a prototype or a retrospective for every design decision. This one was warranted + because the decision was fully implemented, then reversed, and produced a repository-wide ADR. +- Do not treat breaking changes as free. They are acceptable because they are semver-signalled and + paid only when a failure becomes real, and existing public error enums still get + `#[non_exhaustive]` before their first publish (EPIC #1669 checklist). + +## Evidence + +- [Issue specification](ISSUE.md): Decision (T2), Decision Revision (T7), and the progress log +- [ADR 20261005145329](../../../adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md) +- The PR for issue #2435: the option C commits, the ADR refinement, the decision switch, the two + `revert(...)` commits, the option B commit, and the `must_use` review fix, cited above by their + Conventional Commit subjects diff --git a/docs/pr-reviews/pr-2445-review/PR-REVIEW.md b/docs/pr-reviews/pr-2445-review/PR-REVIEW.md new file mode 100644 index 000000000..29b5c4908 --- /dev/null +++ b/docs/pr-reviews/pr-2445-review/PR-REVIEW.md @@ -0,0 +1,329 @@ +--- +semantic-links: + skill-links: + - process-pr-review + related-artifacts: + - .github/skills/dev/pr-reviews/process-pr-review/SKILL.md + - docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md +--- + + + +# PR #2445 Review Audit + +Source: pull-request reviews and inline review threads for . + +## Ownership + +The PR author owns this tracked audit record. Reviewers, including repository review agents, +deliver findings through GitHub and have no repository-artifact obligation. + +- Post-merge workflow approval: N/A + +## Status Values + +- Relationship: `ORIGINAL`, `RE_RAISE_OF:` +- Disposition: `FIXED`, `NO_ACTION`, `SUPERSEDED`, `FOLLOW_UP` +- Thread state: `OPEN`, `RESOLVED`, `NON_RESOLVABLE`, `SUPERSEDED`. `OPEN` applies prospectively + to audits created or updated for approved follow-up work; historical audits remain valid without + bulk migration. +- Severity: `Blocker`, `Major`, `Minor`, `Nit`, `Suggestion`; append `(inferred)` when derived + from free prose. +- Author class: `Copilot`, `Human`, `Unknown` +- Category: `link-integrity`, `formatting`, `metadata`, `testing`, `correctness`, `documentation`, + `maintainability`, `security`, `other` +- An outdated thread whose concern was fixed is `FIXED`/`RESOLVED`, even when GitHub marks the + original thread outdated after the push. For in-PR feedback, use `NO_ACTION`/`SUPERSEDED` only + for a duplicate, superseded, or no-change concern. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. + +## Findings + +Round 1 had two reviews. Human review 5426703978 (da2ce7) supplied finding IDs F1 to F7, which are +kept. Copilot review 5426586537 supplied none, so its four findings take the next audit-local IDs, +F8 to F11, in source order. Copilot severities are inferred from its overview badges: `Medium` +maps to Minor and `Low` to Nit. Neither review body adds an assertion beyond its inline threads. +The maintainer approved the dispositions before any change. + +| Finding ID | Review finding reference | Author class | Severity | Category | Relationship | Disposition | Thread state | +| ---------- | ------------------------ | ------------ | -------- | -------- | ------------ | ----------- | ------------ | +| F1 | `review-finding:pr-2445-f1` | Human | Major | documentation | ORIGINAL | FIXED | RESOLVED | +| F2 | `review-finding:pr-2445-f2` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F3 | `review-finding:pr-2445-f3` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F4 | `review-finding:pr-2445-f4` | Human | Suggestion | documentation | ORIGINAL | FIXED | RESOLVED | +| F5 | `review-finding:pr-2445-f5` | Human | Suggestion | documentation | ORIGINAL | FIXED | RESOLVED | +| F6 | `review-finding:pr-2445-f6` | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | +| F7 | `review-finding:pr-2445-f7` | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | +| F8 | `review-finding:pr-2445-f8` | Copilot | Minor (inferred) | link-integrity | ORIGINAL | FIXED | RESOLVED | +| F9 | `review-finding:pr-2445-f9` | Copilot | Minor (inferred) | metadata | ORIGINAL | FIXED | RESOLVED | +| F10 | `review-finding:pr-2445-f10` | Copilot | Nit (inferred) | maintainability | ORIGINAL | NO_ACTION | SUPERSEDED | +| F11 | `review-finding:pr-2445-f11` | Copilot | Nit (inferred) | maintainability | RE_RAISE_OF:F10 | NO_ACTION | SUPERSEDED | +| F12 | `review-finding:pr-2445-f12` | Human | Major | correctness | ORIGINAL | FIXED | RESOLVED | +| F13 | `review-finding:pr-2445-f13` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F14 | `review-finding:pr-2445-f14` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | + +## Finding Details + +### F1 - ADR no-expect rule contradicted the expect policy + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F1 +- Source URL: +- Concern: The ADR bullet "Never `expect` or document `# Panics` for failures that cannot occur" + contradicted the `handle-errors-in-code` skill, which allows production `expect` when failure is + logically impossible. Together with `clippy::missing_panics_doc`, it forbade legitimate call-site + `expect` uses on APIs the workspace does not own. +- Solution: Scoped the bullet. It now forbids `expect`ing a workspace API's impossible error (change + that API instead), while a call-site `expect` on an API that can fail in general but not for this + input stays under the skill's policy, with the required `# Panics` section. +- Current-tree verification: re-read the ADR's Related rules and the skill's Unwrap and Expect + Policy table; they no longer conflict. +- Resolution reference: `docs(adrs): scope the no-expect rule to workspace APIs that cannot fail` +- Follow-up PR URL: N/A +- Reply URL: + +### F2 - Affected code did not link back to the ADR + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F2 +- Source URL: +- Concern: `create-adr` Step 3.5 asks for module-level doc links from the affected code back to the + ADR; neither `registry.rs` nor `in_memory.rs` had one. +- Solution: Added a module doc link and an `// adr:` marker to both files, matching + `persisted_downloads.rs`. +- Current-tree verification: `grep -n 'adr' packages/swarm-coordination-registry/src/swarm/registry.rs packages/tracker-core/src/torrent/repository/in_memory.rs` + shows the link and marker in both; clippy and nightly fmt are clean. +- Resolution reference: `docs(tracker-core): link the registry and in-memory repository to their ADR` +- Follow-up PR URL: N/A +- Reply URL: + +### F3 - Independent task review was not persisted + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F3 +- Source URL: +- Concern: The spec recorded a task review outcome but the folder had no `agent-review-reports.md`, + and the template's checkpoint for it was missing. +- Solution: Added `agent-review-reports.md` from the template with the review's scope, evidence, + findings, verdict, and follow-up, noting it was recorded after the fact; added and ticked the + checkpoint. +- Current-tree verification: the spec folder lists `ISSUE.md`, `agent-review-reports.md`, and + `implementation-retrospective.md`; the checkpoint line is present. +- Resolution reference: `docs(issues): record the #2435 task review and fix spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F4 - "Port" undefined and REST query ports not placed + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F4 +- Source URL: +- Concern: The ADR used "port" without a definition. Read with the contract-first REST API ADR, + `StatsQueryPort` is a swappable port, so condition 3 and the retrospective seemed to require the + `Result` that this PR reverted. +- Solution: The maintainer chose to tighten condition 3: a trait or port qualifies only with an + existing or planned backend that can fail; being swappable alone does not. The ADR links the + port definition and places the REST ports (auth-key and whitelist return `Result` through + database I/O, stats and torrent query ports return plain values). The retrospective, skill, and + index use the same wording. +- Current-tree verification: re-read ADR condition 3, the retrospective's Avoiding Overcorrection + section, the skill rule, and the index row. +- Resolution reference: `docs(adrs): define ports and require a real or planned failing backend` +- Follow-up PR URL: N/A +- Reply URL: + +### F5 - False lock-panic docs in torrent services + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F5 +- Source URL: +- Concern: `get_torrent_info`, `get_torrents_page`, and `get_torrents` documented a panic when a + lock cannot be obtained, but they only await `tokio::sync::Mutex::lock`, which cannot fail. +- Solution: Deleted the three `# Panics` sections (docs-only change). +- Current-tree verification: `grep -n 'panics if the lock' packages/tracker-core/src/torrent/services.rs` + returns nothing; the function bodies contain no `expect`, `unwrap`, or indexing; clippy is clean. +- Resolution reference: `docs(tracker-core): remove false lock-panic docs from torrent services` +- Follow-up PR URL: N/A +- Reply URL: + +### F6 - AC5 evidence command was not reproducible + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F6 +- Source URL: +- Concern: Run from the repository root, the AC5 `rg` command matched prose about the reverted work + in three docs files, so the "finds nothing" claim did not reproduce. +- Solution: Scoped the command to `-- packages src` and noted what the unscoped run matches. +- Current-tree verification: `rg 'registry::Error|SwarmRegistry|StatsError' -- packages src` exits + 1 with no matches. +- Resolution reference: `docs(issues): record the #2435 task review and fix spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F7 - EPIC checklist placed under Open Questions + +- PR number: 2445 +- Source review ID: 5426703978 +- Reviewer finding ID: F7 +- Source URL: +- Concern: The Pre-publish API checklist is a decided procedure but sat under "Open Questions". +- Solution: Moved it to a subsection of Delivery Strategy with an EPIC progress-log entry. The + rebase onto `develop` after #2441 merged resolved the EPIC stamp and log conflicts, keeping both + log histories in chronological order. +- Current-tree verification: `grep -nE '^#{2,3} ' docs/issues/open/1669-overhaul-packages/EPIC.md` + shows the checklist between "Subsequent cycles" and "Open Questions". +- Resolution reference: `docs(issues): move the EPIC #1669 pre-publish checklist to Delivery Strategy` +- Follow-up PR URL: N/A +- Reply URL: + +### F8 - T3 row cited a superseded ADR filename + +- PR number: 2445 +- Source review ID: 5426586537 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The spec's T3 row named an ADR file that does not exist in this PR. +- Solution: The T3 row now explains the rename history and names the final ADR file. +- Current-tree verification: re-read the T3 row; `git ls-files docs/adrs/ | grep 20261005145329` + lists only the final file. +- Resolution reference: `docs(issues): record the #2435 task review and fix spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F9 - Spec said pre-push checks had not run + +- PR number: 2445 +- Source review ID: 5426586537 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The PR body said pre-push checks passed, but the spec checkpoint said they had not run. +- Solution: The PR body was accurate; the spec line was stale. The checkpoint is now ticked and + records the pre-commit, full test suite, and pre-push results. +- Current-tree verification: re-read the Workflow Checkpoints in `ISSUE.md`; the pre-push hook + passed again on this round's push. +- Resolution reference: `docs(issues): record the #2435 task review and fix spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F10 - Remove-method counts lack `#[must_use]` + +- PR number: 2445 +- Source review ID: 5426586537 +- Reviewer finding ID: N/A +- Source URL: +- Concern: `remove_inactive_peers` and `remove_peerless_torrents` return counts without + `#[must_use]`; either mark them or return `()`. +- Solution: No change, approved by the maintainer. The counts are informational: the registry + logs them, and every caller discards them on purpose. `#[must_use]` would force unused bindings, + and `()` would drop a count a caller may legitimately want. Review 5426703978 independently found + the split matches the callers. +- Current-tree verification: `grep -rnE 'swarms\s*\.\s*remove_(inactive_peers|peerless_torrents)\(' packages --include=*.rs` + (scoped to the registry receiver; corrected per F14) matches eight lines: `in_memory.rs` (two) + and six registry test lines, each discarding the count. +- Resolution reference: +- Follow-up PR URL: N/A +- Reply URL: + +### F11 - Same `#[must_use]` concern on the second remove method + +- PR number: 2445 +- Source review ID: 5426586537 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The same "These methods..." comment as F10, anchored on `remove_peerless_torrents`. +- Solution: No change; F10's reasoning covers both methods. +- Current-tree verification: same as F10. +- Resolution reference: +- Follow-up PR URL: N/A +- Reply URL: + +### F12 - Rebase dropped three EPIC log lines from #2441 + +- PR number: 2445 +- Source review ID: 5427427892 +- Reviewer finding ID: F12 +- Source URL: +- Concern: Resolving the EPIC conflict while rebasing onto `develop` deleted the first three lines + of the 2026-10-05 15:53 UTC Progress Log entry merged with #2441, so the F7 reply and Solution + ("both log histories kept in chronological order") were false. +- Solution: Restored the three base lines from `d0c97baef`, so the entry is whole and precedes the + 21:07 entry. +- Current-tree verification: `git diff -U0 d0c97baef -- docs/issues/open/1669-overhaul-packages/EPIC.md` + deletes only the intended `last-updated-utc` line. +- Resolution reference: `docs(issues): restore the EPIC #1669 log lines lost in the #2445 rebase` +- Follow-up PR URL: N/A +- Reply URL: + +### F13 - AC5 file list stale after the F5 fix + +- PR number: 2445 +- Source review ID: 5427427892 +- Reviewer finding ID: F13 +- Source URL: +- Concern: The F5 fix made `tracker-core/src/torrent/services.rs` a fifth file in the net code diff, + but AC5, the retrospective, and the PR body still said four; the PR body also still described + #2441 as open. +- Solution: AC5 and the retrospective now list five files with `services.rs` marked doc-only; the + PR body was updated with `gh pr edit` and its #2441 note replaced. The 2026-10-05 19:56 UTC + progress-log entry stays, since it was true when written. +- Current-tree verification: `git diff --stat torrust/develop -- packages src` reports 5 files; the + live PR body no longer contains "four files" or "Open PR #2441". +- Resolution reference: `docs(issues): list all five files in the #2435 net code diff` +- Follow-up PR URL: N/A +- Reply URL: + +### F14 - F10 verification grep did not reproduce + +- PR number: 2445 +- Source review ID: 5427427892 +- Reviewer finding ID: F14 +- Source URL: +- Concern: F10's verification grep matched 106 lines in 25 files, not only the callers it claimed. +- Solution: Scoped the pattern to the registry receiver and recorded its exact result; F11 inherits + it through "same as F10". +- Current-tree verification: `grep -rnE 'swarms\s*\.\s*remove_(inactive_peers|peerless_torrents)\(' packages --include=*.rs` + prints eight lines (`in_memory.rs:65,78` and six registry test lines). +- Resolution reference: `docs(pr-reviews): scope the F10 verification grep in the #2445 audit` +- Follow-up PR URL: N/A +- Reply URL: + +## Processing Log + +- 2026-10-06 10:06 UTC - Started audit. Fetched 11 threads (all unresolved) and both round-1 + review bodies with `github-review-threads` (time from the thread file). +- 2026-10-06 10:33 UTC - First fix committed after the maintainer approved the dispositions in + chat: F1 scope the ADR bullet, F4 tighten condition 3, F10 and F11 no change, all others fixed as + proposed. The approval preceded this commit; its exact time was not recorded. +- 2026-10-06 10:47 UTC - Rebased all fix commits (authored 10:33 to 10:43 UTC) onto `develop`, + resolving the EPIC conflicts with #2441, and pushed; the pre-push hook passed. +- 2026-10-06 10:51 UTC - Replied to all 11 threads; recorded reply URLs. +- 2026-10-06 11:08 UTC - Round 2: review 5427427892 (da2ce7), at the head carrying this audit's + first commit, raised + F12 (Major), F13 and F14 (Minor), all blocking, and asked to hold the F7 thread until F12 was + fixed. No new Copilot review. +- 2026-10-06 11:30 UTC - Committed the F12, F13, and F14 fixes (authored 11:19 to 11:25 UTC) and + pushed; the pre-push hook passed. Updated the PR body for F13. +- 2026-10-06 11:33 UTC - Replied to the F12, F13, and F14 threads; recorded reply URLs. + +## Completion Rules + +- Re-derive the reply claim against the current tree before replying or resolving a thread. +- Reply on every resolvable thread before resolving it. +- For an outdated thread whose concern was fixed, record `Disposition=FIXED` and + `Thread state=RESOLVED`, even if GitHub marks the thread outdated after the push. For a + duplicate, superseded, or no-change in-PR thread, reply exactly + `Superseded by : .`, record `Disposition=NO_ACTION` and + `Thread state=SUPERSEDED`, then resolve it. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. +- A consolidated PR conversation response may cover multiple review rounds only when it names + every review ID and every finding ID with its disposition and resolution reference. Record its + durable URL in each related row. +- Cite a fix by its unique Conventional Commit subject or durable reply URL, never by a branch SHA + that can change after a rebase. +- Refresh review threads using GraphQL and confirm that no unresolved actionable thread remains. diff --git a/packages/swarm-coordination-registry/src/statistics/mod.rs b/packages/swarm-coordination-registry/src/statistics/mod.rs index 470ff6ea2..396b59d77 100644 --- a/packages/swarm-coordination-registry/src/statistics/mod.rs +++ b/packages/swarm-coordination-registry/src/statistics/mod.rs @@ -173,7 +173,7 @@ mod tests { let mut peer = sample_peer(); peer.updated = startup_time; - swarms.handle_announcement(&sample_info_hash(), &peer, None).await.unwrap(); + swarms.handle_announcement(&sample_info_hash(), &peer, None).await; Self { startup_time, diff --git a/packages/swarm-coordination-registry/src/swarm/registry.rs b/packages/swarm-coordination-registry/src/swarm/registry.rs index 75f5c4db7..fdfd0e246 100644 --- a/packages/swarm-coordination-registry/src/swarm/registry.rs +++ b/packages/swarm-coordination-registry/src/swarm/registry.rs @@ -1,4 +1,8 @@ -use std::convert::Infallible; +//! Swarm coordination registry. +//! +//! Methods that cannot fail return plain values, per +//! [ADR-20261005145329](https://github.com/torrust/torrust-tracker/blob/develop/docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md). +// adr: docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md use std::sync::Arc; use crossbeam_skiplist::SkipMap; @@ -41,21 +45,12 @@ impl Registry { /// * `peer` - The peer to upsert. /// * `opt_persistent_torrent` - The optional persisted data about a torrent /// (number of downloads for the torrent). - /// - /// # Returns - /// - /// Returns `true` if the number of downloads was increased because the peer - /// completed the download. - /// - /// # Errors - /// - /// This function panics if the lock for the swarm handle cannot be acquired. pub async fn handle_announcement( &self, info_hash: &InfoHash, peer: &peer::Peer, opt_persistent_torrent: Option, - ) -> Result<(), Error> { + ) { let swarm_handle = match self.swarms.get(info_hash) { None => { let number_of_downloads = opt_persistent_torrent.unwrap_or_default(); @@ -80,8 +75,6 @@ impl Registry { }; swarm_handle.value().lock().await.handle_announcement(peer).await; - - Ok(()) } /// Inserts a new swarm. Only used for testing purposes. @@ -161,16 +154,13 @@ impl Registry { /// # Returns /// /// A `SwarmMetadata` struct containing the aggregated torrent data if found. - /// - /// # Errors - /// - /// This function panics if the lock for the swarm handle cannot be acquired. - pub async fn get_swarm_metadata(&self, info_hash: &InfoHash) -> Result, Error> { + #[must_use] + pub async fn get_swarm_metadata(&self, info_hash: &InfoHash) -> Option { match self.swarms.get(info_hash) { - None => Ok(None), + None => None, Some(swarm_handle) => { let swarm = swarm_handle.value().lock().await; - Ok(Some(swarm.metadata())) + Some(swarm.metadata()) } } } @@ -181,16 +171,9 @@ impl Registry { /// /// A `SwarmMetadata` struct containing the aggregated torrent data if it's /// found or a zeroed metadata struct if not. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for the - /// swarm handle. - pub async fn get_swarm_metadata_or_default(&self, info_hash: &InfoHash) -> Result { - match self.get_swarm_metadata(info_hash).await { - Ok(Some(swarm_metadata)) => Ok(swarm_metadata), - Ok(None) => Ok(SwarmMetadata::zeroed()), - } + #[must_use] + pub async fn get_swarm_metadata_or_default(&self, info_hash: &InfoHash) -> SwarmMetadata { + self.get_swarm_metadata(info_hash).await.unwrap_or_else(SwarmMetadata::zeroed) } /// Retrieves torrent peers for a given torrent and client, excluding the @@ -203,22 +186,13 @@ impl Registry { /// /// A vector of peers (wrapped in `Arc`) representing the active peers for /// the torrent, excluding the requesting client. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for the - /// swarm handle. - pub async fn get_peers_peers_excluding( - &self, - info_hash: &InfoHash, - peer: &peer::Peer, - limit: usize, - ) -> Result>, Error> { + #[must_use] + pub async fn get_peers_peers_excluding(&self, info_hash: &InfoHash, peer: &peer::Peer, limit: usize) -> Vec> { match self.get(info_hash) { - None => Ok(vec![]), + None => vec![], Some(swarm_handle) => { let swarm = swarm_handle.lock().await; - Ok(swarm.peers_excluding(&peer.peer_addr, Some(limit))) + swarm.peers_excluding(&peer.peer_addr, Some(limit)) } } } @@ -232,17 +206,13 @@ impl Registry { /// /// A vector of peers (wrapped in `Arc`) representing the active peers for /// the torrent. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for the - /// swarm handle. - pub async fn get_swarm_peers(&self, info_hash: &InfoHash, limit: usize) -> Result>, Error> { + #[must_use] + pub async fn get_swarm_peers(&self, info_hash: &InfoHash, limit: usize) -> Vec> { match self.get(info_hash) { - None => Ok(vec![]), + None => vec![], Some(swarm_handle) => { let swarm = swarm_handle.lock().await; - Ok(swarm.peers(Some(limit))) + swarm.peers(Some(limit)) } } } @@ -287,12 +257,7 @@ impl Registry { /// /// A peer is considered inactive if its last update timestamp is older than /// the provided cutoff time. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for any - /// swarm handle. - pub async fn remove_inactive_peers(&self, current_cutoff: DurationSinceUnixEpoch) -> Result { + pub async fn remove_inactive_peers(&self, current_cutoff: DurationSinceUnixEpoch) -> usize { tracing::info!( "Removing inactive peers since: {:?} ...", convert_from_timestamp_to_datetime_utc(current_cutoff) @@ -307,19 +272,14 @@ impl Registry { tracing::info!(inactive_peers_removed = inactive_peers_removed); - Ok(inactive_peers_removed) + inactive_peers_removed } /// Removes torrent entries that have no active peers. /// /// Depending on the tracker policy, torrents without any peers may be /// removed to conserve memory. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for any - /// swarm handle. - pub async fn remove_peerless_torrents(&self, policy: &TrackerPolicy) -> Result { + pub async fn remove_peerless_torrents(&self, policy: &TrackerPolicy) -> u64 { tracing::info!("Removing peerless torrents ..."); let mut peerless_torrents_removed = 0; @@ -342,7 +302,7 @@ impl Registry { tracing::info!(peerless_torrents_removed = peerless_torrents_removed); - Ok(peerless_torrents_removed) + peerless_torrents_removed } /// Imports persistent torrent data into the in-memory repository. @@ -383,12 +343,8 @@ impl Registry { /// # Returns /// /// An [`AggregateActiveSwarmMetadata`] struct with the aggregated metrics. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for any - /// swarm handle. - pub async fn get_aggregate_swarm_metadata(&self) -> Result { + #[must_use] + pub async fn get_aggregate_swarm_metadata(&self) -> AggregateActiveSwarmMetadata { let mut metrics = AggregateActiveSwarmMetadata::default(); for swarm_handle in &self.swarms { @@ -400,7 +356,7 @@ impl Registry { metrics.total_torrents += 1; } - Ok(metrics) + metrics } /// Counts the number of torrents that are peerless (i.e., have no active @@ -409,12 +365,8 @@ impl Registry { /// # Returns /// /// A `usize` representing the number of peerless torrents. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for any - /// swarm handle. - pub async fn count_peerless_torrents(&self) -> Result { + #[must_use] + pub async fn count_peerless_torrents(&self) -> usize { let mut peerless_torrents = 0; for swarm_handle in &self.swarms { @@ -425,7 +377,7 @@ impl Registry { } } - Ok(peerless_torrents) + peerless_torrents } /// Counts the total number of peers across all torrents. @@ -433,12 +385,8 @@ impl Registry { /// # Returns /// /// A `usize` representing the total number of peers. - /// - /// # Errors - /// - /// This function returns an error if it fails to acquire the lock for any - /// swarm handle. - pub async fn count_peers(&self) -> Result { + #[must_use] + pub async fn count_peers(&self) -> usize { let mut peers = 0; for swarm_handle in &self.swarms { @@ -447,7 +395,7 @@ impl Registry { peers += swarm.len(); } - Ok(peers) + peers } #[must_use] @@ -465,9 +413,6 @@ impl Registry { } } -/// The registry currently exposes no recoverable error cases. -pub type Error = Infallible; - #[derive(Clone, Debug, Default)] pub struct AggregateActivityMetadata { /// The number of active peers in all swarms. @@ -544,7 +489,7 @@ mod tests { let swarms = Arc::new(Registry::default()); let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; assert_eq!(swarms.len(), 1); } @@ -555,7 +500,7 @@ mod tests { let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; assert!(!swarms.is_empty()); } @@ -564,7 +509,7 @@ mod tests { let swarms = Arc::new(Registry::default()); let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; assert!(!swarms.is_empty()); } @@ -582,7 +527,7 @@ mod tests { let info_hash = sample_info_hash(); - swarms.handle_announcement(&info_hash, &sample_peer(), None).await.unwrap(); + swarms.handle_announcement(&info_hash, &sample_peer(), None).await; assert!(swarms.get(&info_hash).is_some()); } @@ -593,8 +538,8 @@ mod tests { let info_hash = sample_info_hash(); - swarms.handle_announcement(&info_hash, &sample_peer(), None).await.unwrap(); - swarms.handle_announcement(&info_hash, &sample_peer(), None).await.unwrap(); + swarms.handle_announcement(&info_hash, &sample_peer(), None).await; + swarms.handle_announcement(&info_hash, &sample_peer(), None).await; assert!(swarms.get(&info_hash).is_some()); } @@ -620,9 +565,9 @@ mod tests { let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; - let peers = swarms.get_swarm_peers(&info_hash, 74).await.unwrap(); + let peers = swarms.get_swarm_peers(&info_hash, 74).await; assert_eq!(peers, vec![Arc::new(peer)]); } @@ -631,7 +576,7 @@ mod tests { async fn it_should_return_an_empty_list_or_peers_for_a_non_existing_torrent() { let swarms = Arc::new(Registry::default()); - let peers = swarms.get_swarm_peers(&sample_info_hash(), 74).await.unwrap(); + let peers = swarms.get_swarm_peers(&sample_info_hash(), 74).await; assert_eq!(peers, Vec::new()); } @@ -653,10 +598,10 @@ mod tests { event: AnnounceEvent::Completed, }; - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; } - let peers = swarms.get_swarm_peers(&info_hash, 74).await.unwrap(); + let peers = swarms.get_swarm_peers(&info_hash, 74).await; assert_eq!(peers.len(), 74); } @@ -682,8 +627,7 @@ mod tests { let peers = swarms .get_peers_peers_excluding(&sample_info_hash(), &sample_peer(), MAX_PEERS) - .await - .unwrap(); + .await; assert_eq!(peers, vec![]); } @@ -695,9 +639,9 @@ mod tests { let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; - let peers = swarms.get_peers_peers_excluding(&info_hash, &peer, MAX_PEERS).await.unwrap(); + let peers = swarms.get_peers_peers_excluding(&info_hash, &peer, MAX_PEERS).await; assert_eq!(peers, vec![]); } @@ -710,7 +654,7 @@ mod tests { let excluded_peer = sample_peer(); - swarms.handle_announcement(&info_hash, &excluded_peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &excluded_peer, None).await; // Add 74 peers for idx in 2..=75 { @@ -724,13 +668,10 @@ mod tests { event: AnnounceEvent::Completed, }; - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; } - let peers = swarms - .get_peers_peers_excluding(&info_hash, &excluded_peer, MAX_PEERS) - .await - .unwrap(); + let peers = swarms.get_peers_peers_excluding(&info_hash, &excluded_peer, MAX_PEERS).await; assert_eq!(peers.len(), 74); } @@ -755,7 +696,7 @@ mod tests { let swarms = Arc::new(Registry::default()); let info_hash = sample_info_hash(); - swarms.handle_announcement(&info_hash, &sample_peer(), None).await.unwrap(); + swarms.handle_announcement(&info_hash, &sample_peer(), None).await; let _unused = swarms.remove(&info_hash).await; @@ -774,7 +715,7 @@ mod tests { let mut peer = sample_peer(); peer.updated = DurationSinceUnixEpoch::new(0, 0); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; // Cut off time is 1 second after the peer was updated let inactive_peers_total = swarms.count_inactive_peers(peer.updated.add(Duration::from_secs(1))).await; @@ -790,21 +731,12 @@ mod tests { let mut peer = sample_peer(); peer.updated = DurationSinceUnixEpoch::new(0, 0); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; // Cut off time is 1 second after the peer was updated - swarms - .remove_inactive_peers(peer.updated.add(Duration::from_secs(1))) - .await - .unwrap(); - - assert!( - !swarms - .get_swarm_peers(&info_hash, 74) - .await - .unwrap() - .contains(&Arc::new(peer)) - ); + swarms.remove_inactive_peers(peer.updated.add(Duration::from_secs(1))).await; + + assert!(!swarms.get_swarm_peers(&info_hash, 74).await.contains(&Arc::new(peer))); } async fn initialize_repository_with_one_torrent_without_peers(info_hash: &InfoHash) -> Arc { @@ -813,13 +745,10 @@ mod tests { // Insert a sample peer for the torrent to force adding the torrent entry let mut peer = sample_peer(); peer.updated = DurationSinceUnixEpoch::new(0, 0); - swarms.handle_announcement(info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(info_hash, &peer, None).await; // Remove the peer - swarms - .remove_inactive_peers(peer.updated.add(Duration::from_secs(1))) - .await - .unwrap(); + swarms.remove_inactive_peers(peer.updated.add(Duration::from_secs(1))).await; swarms } @@ -835,7 +764,7 @@ mod tests { ..Default::default() }; - swarms.remove_peerless_torrents(&tracker_policy).await.unwrap(); + swarms.remove_peerless_torrents(&tracker_policy).await; assert!(swarms.get(&info_hash).is_none()); } @@ -883,7 +812,7 @@ mod tests { let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; let torrent_entry_info = torrent_entry_info(swarms.get(&info_hash).unwrap()).await; @@ -918,7 +847,7 @@ mod tests { let info_hash = sample_info_hash(); let peer = sample_peer(); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; let torrent_entries = swarms.get_paginated(None); @@ -962,12 +891,12 @@ mod tests { // Insert one torrent entry let info_hash_one = sample_info_hash_one(); let peer_one = sample_peer_one(); - swarms.handle_announcement(&info_hash_one, &peer_one, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_one, None).await; // Insert another torrent entry let info_hash_one = sample_info_hash_alphabetically_ordered_after_sample_info_hash_one(); let peer_two = sample_peer_two(); - swarms.handle_announcement(&info_hash_one, &peer_two, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_two, None).await; // Get only the first page where page size is 1 let torrent_entries = swarms.get_paginated(Some(&Pagination { offset: 0, limit: 1 })); @@ -997,12 +926,12 @@ mod tests { // Insert one torrent entry let info_hash_one = sample_info_hash_one(); let peer_one = sample_peer_one(); - swarms.handle_announcement(&info_hash_one, &peer_one, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_one, None).await; // Insert another torrent entry let info_hash_one = sample_info_hash_alphabetically_ordered_after_sample_info_hash_one(); let peer_two = sample_peer_two(); - swarms.handle_announcement(&info_hash_one, &peer_two, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_two, None).await; // Get only the first page where page size is 1 let torrent_entries = swarms.get_paginated(Some(&Pagination { offset: 1, limit: 1 })); @@ -1032,12 +961,12 @@ mod tests { // Insert one torrent entry let info_hash_one = sample_info_hash_one(); let peer_one = sample_peer_one(); - swarms.handle_announcement(&info_hash_one, &peer_one, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_one, None).await; // Insert another torrent entry let info_hash_one = sample_info_hash_alphabetically_ordered_after_sample_info_hash_one(); let peer_two = sample_peer_two(); - swarms.handle_announcement(&info_hash_one, &peer_two, None).await.unwrap(); + swarms.handle_announcement(&info_hash_one, &peer_two, None).await; // Get only the first page where page size is 1 let torrent_entries = swarms.get_paginated(Some(&Pagination { offset: 1, limit: 1 })); @@ -1064,7 +993,7 @@ mod tests { async fn it_should_get_empty_aggregate_swarm_metadata_when_there_are_no_torrents() { let swarms = Arc::new(Registry::default()); - let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await.unwrap(); + let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await; assert_eq!( aggregate_swarm_metadata, @@ -1081,12 +1010,9 @@ mod tests { async fn it_should_return_the_aggregate_swarm_metadata_when_there_is_a_leecher() { let swarms = Arc::new(Registry::default()); - swarms - .handle_announcement(&sample_info_hash(), &leecher(), None) - .await - .unwrap(); + swarms.handle_announcement(&sample_info_hash(), &leecher(), None).await; - let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await.unwrap(); + let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await; assert_eq!( aggregate_swarm_metadata, @@ -1103,12 +1029,9 @@ mod tests { async fn it_should_return_the_aggregate_swarm_metadata_when_there_is_a_seeder() { let swarms = Arc::new(Registry::default()); - swarms - .handle_announcement(&sample_info_hash(), &seeder(), None) - .await - .unwrap(); + swarms.handle_announcement(&sample_info_hash(), &seeder(), None).await; - let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await.unwrap(); + let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await; assert_eq!( aggregate_swarm_metadata, @@ -1125,12 +1048,9 @@ mod tests { async fn it_should_return_the_aggregate_swarm_metadata_when_there_is_a_completed_peer() { let swarms = Arc::new(Registry::default()); - swarms - .handle_announcement(&sample_info_hash(), &complete_peer(), None) - .await - .unwrap(); + swarms.handle_announcement(&sample_info_hash(), &complete_peer(), None).await; - let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await.unwrap(); + let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await; assert_eq!( aggregate_swarm_metadata, @@ -1149,15 +1069,12 @@ mod tests { let start_time = std::time::Instant::now(); for i in 0..1_000_000 { - swarms - .handle_announcement(&gen_seeded_infohash(i), &leecher(), None) - .await - .unwrap(); + swarms.handle_announcement(&gen_seeded_infohash(i), &leecher(), None).await; } let result_a = start_time.elapsed(); let start_time = std::time::Instant::now(); - let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await.unwrap(); + let aggregate_swarm_metadata = swarms.get_aggregate_swarm_metadata().await; let result_b = start_time.elapsed(); assert_eq!( @@ -1183,7 +1100,7 @@ mod tests { #[tokio::test] async fn no_peerless_torrents() { let swarms = Arc::new(Registry::default()); - assert_eq!(swarms.count_peerless_torrents().await.unwrap(), 0); + assert_eq!(swarms.count_peerless_torrents().await, 0); } #[tokio::test] @@ -1192,12 +1109,12 @@ mod tests { let peer = sample_peer(); let swarms = Arc::new(Registry::default()); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; let current_cutoff = peer.updated + DurationSinceUnixEpoch::from_secs(1); - swarms.remove_inactive_peers(current_cutoff).await.unwrap(); + swarms.remove_inactive_peers(current_cutoff).await; - assert_eq!(swarms.count_peerless_torrents().await.unwrap(), 1); + assert_eq!(swarms.count_peerless_torrents().await, 1); } } @@ -1210,7 +1127,7 @@ mod tests { #[tokio::test] async fn no_peers() { let swarms = Arc::new(Registry::default()); - assert_eq!(swarms.count_peers().await.unwrap(), 0); + assert_eq!(swarms.count_peers().await, 0); } #[tokio::test] @@ -1219,9 +1136,9 @@ mod tests { let peer = sample_peer(); let swarms = Arc::new(Registry::default()); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; - assert_eq!(swarms.count_peers().await.unwrap(), 1); + assert_eq!(swarms.count_peers().await, 1); } } } @@ -1241,9 +1158,9 @@ mod tests { let infohash = sample_info_hash(); - swarms.handle_announcement(&infohash, &leecher(), None).await.unwrap(); + swarms.handle_announcement(&infohash, &leecher(), None).await; - let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await.unwrap(); + let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await; assert_eq!( swarm_metadata, @@ -1259,7 +1176,7 @@ mod tests { async fn it_should_return_zeroed_swarm_metadata_for_a_non_existing_torrent() { let swarms = Arc::new(Registry::default()); - let swarm_metadata = swarms.get_swarm_metadata_or_default(&sample_info_hash()).await.unwrap(); + let swarm_metadata = swarms.get_swarm_metadata_or_default(&sample_info_hash()).await; assert_eq!(swarm_metadata, SwarmMetadata::zeroed()); } @@ -1286,7 +1203,7 @@ mod tests { swarms.import_persistent(&persistent_torrents); - let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await.unwrap(); + let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await; // Only the number of downloads is persisted. assert_eq!(swarm_metadata.downloaded, 1); @@ -1307,7 +1224,7 @@ mod tests { swarms.import_persistent(&persistent_torrents); - let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await.unwrap(); + let swarm_metadata = swarms.get_swarm_metadata_or_default(&infohash).await; // It takes the last value assert_eq!(swarm_metadata.downloaded, 2); @@ -1320,8 +1237,8 @@ mod tests { let infohash = sample_info_hash(); // Insert a new the torrent entry - swarms.handle_announcement(&infohash, &leecher(), None).await.unwrap(); - let initial_number_of_downloads = swarms.get_swarm_metadata_or_default(&infohash).await.unwrap().downloaded; + swarms.handle_announcement(&infohash, &leecher(), None).await; + let initial_number_of_downloads = swarms.get_swarm_metadata_or_default(&infohash).await.downloaded; // Try to import the torrent entry let new_number_of_downloads = initial_number_of_downloads + 1; @@ -1331,7 +1248,7 @@ mod tests { // The number of downloads should not be changed assert_eq!( - swarms.get_swarm_metadata_or_default(&infohash).await.unwrap().downloaded, + swarms.get_swarm_metadata_or_default(&infohash).await.downloaded, initial_number_of_downloads ); } @@ -1370,7 +1287,7 @@ mod tests { let swarms = Registry::new(Some(Arc::new(event_sender_mock))); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; } #[tokio::test] @@ -1394,7 +1311,7 @@ mod tests { let swarms = Registry::new(Some(Arc::new(event_sender_mock))); - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; swarms.remove(&info_hash).await.unwrap(); } @@ -1422,11 +1339,11 @@ mod tests { let swarms = Registry::new(Some(Arc::new(event_sender_mock))); // Add the new torrent - swarms.handle_announcement(&info_hash, &peer, None).await.unwrap(); + swarms.handle_announcement(&info_hash, &peer, None).await; // Remove the peer let current_cutoff = peer.updated + DurationSinceUnixEpoch::from_secs(1); - swarms.remove_inactive_peers(current_cutoff).await.unwrap(); + swarms.remove_inactive_peers(current_cutoff).await; // Remove peerless torrents @@ -1435,7 +1352,7 @@ mod tests { ..Default::default() }; - swarms.remove_peerless_torrents(&tracker_policy).await.unwrap(); + swarms.remove_peerless_torrents(&tracker_policy).await; } } } diff --git a/packages/tracker-core/src/torrent/repository/in_memory.rs b/packages/tracker-core/src/torrent/repository/in_memory.rs index ec0b0716d..764f71add 100644 --- a/packages/tracker-core/src/torrent/repository/in_memory.rs +++ b/packages/tracker-core/src/torrent/repository/in_memory.rs @@ -1,4 +1,8 @@ //! In-memory torrents repository. +//! +//! The swarm registry it wraps cannot fail, so these methods return plain values, per +//! [ADR-20261005145329](https://github.com/torrust/torrust-tracker/blob/develop/docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md). +// adr: docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md use std::sync::Arc; use torrust_clock::DurationSinceUnixEpoch; @@ -39,24 +43,13 @@ impl InMemoryTorrentRepository { /// /// * `info_hash` - The unique identifier of the torrent. /// * `peer` - The peer to insert or update in the torrent entry. - /// - /// # Returns - /// - /// `true` if the peer stats were updated. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. pub async fn handle_announcement( &self, info_hash: &InfoHash, peer: &peer::Peer, opt_persistent_torrent: Option, ) { - self.swarms - .handle_announcement(info_hash, peer, opt_persistent_torrent) - .await - .expect("Failed to upsert the peer in swarms"); + self.swarms.handle_announcement(info_hash, peer, opt_persistent_torrent).await; } /// Removes inactive peers from all torrent entries. @@ -68,15 +61,8 @@ impl InMemoryTorrentRepository { /// /// * `current_cutoff` - The cutoff timestamp; peers not updated since this /// time will be removed. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. pub(crate) async fn remove_inactive_peers(&self, current_cutoff: DurationSinceUnixEpoch) { - self.swarms - .remove_inactive_peers(current_cutoff) - .await - .expect("Failed to remove inactive peers from swarms"); + self.swarms.remove_inactive_peers(current_cutoff).await; } /// Removes torrent entries that have no active peers. @@ -88,15 +74,8 @@ impl InMemoryTorrentRepository { /// /// * `policy` - The tracker policy containing the configuration for /// removing peerless torrents. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. pub(crate) async fn remove_peerless_torrents(&self, policy: &TrackerPolicy) { - self.swarms - .remove_peerless_torrents(policy) - .await - .expect("Failed to remove peerless torrents from swarms"); + self.swarms.remove_peerless_torrents(policy).await; } /// Retrieves a torrent entry by its infohash. @@ -136,16 +115,9 @@ impl InMemoryTorrentRepository { /// This method returns the swarm metadata (aggregate information such as /// peer counts) for the torrent specified by the infohash, or `None` if the /// torrent is not in memory. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub(crate) async fn get_swarm_metadata(&self, info_hash: &InfoHash) -> Option { - self.swarms - .get_swarm_metadata(info_hash) - .await - .expect("Failed to get swarm metadata") + self.swarms.get_swarm_metadata(info_hash).await } /// Retrieves swarm metadata for a given torrent. @@ -161,16 +133,9 @@ impl InMemoryTorrentRepository { /// # Returns /// /// A `SwarmMetadata` struct containing the aggregated torrent data. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub(crate) async fn get_swarm_metadata_or_default(&self, info_hash: &InfoHash) -> SwarmMetadata { - self.swarms - .get_swarm_metadata_or_default(info_hash) - .await - .expect("Failed to get swarm metadata") + self.swarms.get_swarm_metadata_or_default(info_hash).await } /// Retrieves torrent peers for a given torrent and client, excluding the @@ -189,16 +154,9 @@ impl InMemoryTorrentRepository { /// /// A vector of peers (wrapped in `Arc`) representing the active peers for /// the torrent, excluding the requesting client. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub(crate) async fn get_peers_for(&self, info_hash: &InfoHash, peer: &peer::Peer, limit: usize) -> Vec> { - self.swarms - .get_peers_peers_excluding(info_hash, peer, limit) - .await - .expect("Failed to get other peers in swarm") + self.swarms.get_peers_peers_excluding(info_hash, peer, limit).await } /// Retrieves the list of peers for a given torrent. @@ -215,16 +173,9 @@ impl InMemoryTorrentRepository { /// /// A vector of peers (wrapped in `Arc`) representing the active peers for /// the torrent. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub async fn get_torrent_peers(&self, info_hash: &InfoHash, max_peers: usize) -> Vec> { - self.swarms - .get_swarm_peers(info_hash, max_peers) - .await - .expect("Failed to get other peers in swarm") + self.swarms.get_swarm_peers(info_hash, max_peers).await } /// Calculates and returns overall torrent metrics. @@ -236,39 +187,21 @@ impl InMemoryTorrentRepository { /// # Returns /// /// A [`AggregateActiveSwarmMetadata`] struct with the aggregated metrics. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub async fn get_aggregate_swarm_metadata(&self) -> AggregateActiveSwarmMetadata { - self.swarms - .get_aggregate_swarm_metadata() - .await - .expect("Failed to get aggregate swarm metadata") + self.swarms.get_aggregate_swarm_metadata().await } /// Counts the number of peerless torrents in the repository. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub async fn count_peerless_torrents(&self) -> usize { - self.swarms - .count_peerless_torrents() - .await - .expect("Failed to count peerless torrents") + self.swarms.count_peerless_torrents().await } /// Counts the number of peers in the repository. - /// - /// # Panics - /// - /// This function panics if the underlying swarms return an error. #[must_use] pub async fn count_peers(&self) -> usize { - self.swarms.count_peers().await.expect("Failed to count peers") + self.swarms.count_peers().await } /// Imports persistent torrent data into the in-memory repository. diff --git a/packages/tracker-core/src/torrent/services.rs b/packages/tracker-core/src/torrent/services.rs index 3e0470564..eecf2d3f6 100644 --- a/packages/tracker-core/src/torrent/services.rs +++ b/packages/tracker-core/src/torrent/services.rs @@ -88,10 +88,6 @@ pub struct BasicInfo { /// An [`Option`] which is: /// - `Some(Info)` if the torrent exists in the repository. /// - `None` if the torrent is not found. -/// -/// # Panics -/// -/// This function panics if the lock for the torrent entry cannot be obtained. #[must_use] pub async fn get_torrent_info( in_memory_torrent_repository: &Arc, @@ -133,10 +129,6 @@ pub async fn get_torrent_info( /// /// A vector of [`BasicInfo`] structs representing the summarized data of the /// torrents. -/// -/// # Panics -/// -/// This function panics if the lock for the torrent entry cannot be obtained. #[must_use] pub async fn get_torrents_page( in_memory_torrent_repository: &Arc, @@ -175,10 +167,6 @@ pub async fn get_torrents_page( /// # Returns /// /// A vector of [`BasicInfo`] structs for the requested torrents. -/// -/// # Panics -/// -/// This function panics if the lock for the torrent entry cannot be obtained. #[must_use] pub async fn get_torrents( in_memory_torrent_repository: &Arc, diff --git a/packages/tracker-core/tests/common/test_env.rs b/packages/tracker-core/tests/common/test_env.rs index 1780d9583..5cb368b1b 100644 --- a/packages/tracker-core/tests/common/test_env.rs +++ b/packages/tracker-core/tests/common/test_env.rs @@ -174,7 +174,6 @@ impl TestEnv { .swarms .get_swarm_metadata(info_hash) .await - .unwrap() } /// Waits until the global download count in the database reaches `expected`, with a 5-second