Repository navigation
refactor(swarm-coordination-registry): [#2435] return plain values from infallible registry methods - #2445
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
Refactors the swarm coordination registry and its consumers to remove Result<_, Infallible> APIs, eliminating impossible error handling and aligning docs/tests with the new signatures.
Changes:
- Change 10 registry methods from
Result-returning to plain values and remove the registryErrortype. - Update
tracker-corerepository and tests to call the infallible APIs directly (remove.expect()/.unwrap()and# Panicsdocs). - Add ADR + supporting documentation to codify when public APIs should return
Result.
| File | Description |
|---|---|
| packages/tracker-core/tests/common/test_env.rs | Removes .unwrap() now that the underlying call no longer returns Result. |
| packages/tracker-core/src/torrent/repository/in_memory.rs | Removes expect/# Panics and delegates directly to infallible registry methods. |
| packages/swarm-coordination-registry/src/swarm/registry.rs | Updates public registry API to return plain values; deletes Error; updates tests accordingly; adds #[must_use] to queries. |
| packages/swarm-coordination-registry/src/statistics/mod.rs | Adjusts tests after registry API became infallible. |
| docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/implementation-retrospective.md | Adds retrospective documenting the decision reversal and lessons learned. |
| docs/issues/open/2435-remove-misleading-panics-in-in-memory-torrent-repository/ISSUE.md | Updates issue spec/status/plan and records decision revision + verification notes. |
| docs/issues/open/1669-overhaul-packages/EPIC.md | Adds a pre-publish API checklist referencing the new ADR. |
| docs/adrs/index.md | Adds the new ADR to the ADR index. |
| docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md | Adds repository-level ADR defining the Result policy for public APIs. |
| .github/skills/dev/rust-code-quality/handle-errors-in-code/SKILL.md | Adds a rule summary and links to the ADR. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at d2c00ea5bfc52585adcd554778ccbb7e9e9e612b (round 1). Recomputed from the bytes at this head, against develop at 632f8b177.
The PR carries 16 commits over 632f8b177 (the merge base is the develop tip) and touches 10 files, +500/−302: four Rust files in swarm-coordination-registry and tracker-core, six docs/skill files, no manifest or lockfile change. The ten registry methods that returned Result<_, Error> with pub type Error = Infallible now return plain values, Error is deleted, and InMemoryTorrentRepository drops its ten expect calls and # Panics sections; a new root ADR and a handle-errors-in-code rule state the policy. Option C (an empty #[non_exhaustive] enum plus propagation) was built first and then reverted to option B; the history keeps both.
Registry API. Ten return types change in registry.rs (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). At the base every return path was Ok(..) of an Infallible error (16 Ok( and 10 # Errors → 0). No Infallible, pub type Error or registry::Error remains in the workspace. #[must_use] is on the seven queries only; handle_announcement and the two remove_* counts stay unmarked, which matches the callers that discard the counts (in_memory.rs:61,74). The eight dependents reach the crate only through containers, events and statistics; the only callers of the ten methods outside the crate are in_memory.rs and tests/common/test_env.rs:172-177.
tracker-core and the reverts. 10 expect and 10 # Panics go, one per method; none could fire, so no input changes behaviour. Both revert(...) commits are byte-exact inverses on every code file, and every file they touch except in_memory.rs is identical to 632f8b177 at this head, Cargo.lock included. No option-C trace (non_exhaustive, SwarmRegistry, StatsError, the doctest) remains in code.
ADR and skill rule. The ADR follows docs/templates/ADR.md and the index-row form, and no text from the two superseded drafts carries their stance. One related-rules bullet contradicts the existing expect policy (F1); the code back-links create-adr Step 3.5 asks for are missing (F2); "port" is used without a definition (F4).
Spec, retrospective, EPIC. The counts reproduce (62 + 1 + 1 test unwrap, 10 expect), the AC1/AC2 grep returns nothing, the retrospective has every template section, and the six docs files cite no branch commit ids (only ADR timestamps). The independent review is not persisted (F3), three false # Panics remain in the same module (F5), AC5's command needs a path scope (F6), and the EPIC checklist sits under "Open Questions" (F7). Open PR #2441 edits the same EPIC stamp line and Progress Log, and a git merge-tree of the two heads reports a content conflict in EPIC.md, so whichever merges second needs a rebase.
Hygiene. 16 Conventional subjects (docs, refactor, feat, fix, revert). AGENTS.md:382 requires the [#<issue>] tag in the PR title, which has it; no skill requires it in commit subjects. Author and committer times are ordered (2026-10-05 14:52Z to 2026-10-06 09:22Z). No attribution trailer and no banned token. The two revert bodies cite pre-rebase ids; ISSUE.md:200 and the PR body say so.
Findings
- F1 Major (blocking): the ADR's "never
expect/# Panics" bullet contradictshandle-errors-in-code/SKILL.md:86. - F2 Minor: the ADR is not linked back from the affected code.
- F3 Minor: the independent review report is not persisted.
- F4 Suggestion: define "port" and place the REST query ports.
- F5 Suggestion: three false
# Panicssections intracker-core/src/torrent/services.rs. - F6 Nit: AC5 evidence command scope.
- F7 Nit: EPIC checklist placement.
Checked, no finding
statistics/mod.rs:176andtest_env.rs:172-177drop anunwrapof a value that could not fail; the helper still returnsOption.get_swarm_metadata_or_default(registry.rs:170-172) is equivalent to the removedmatch.- The removed
# Returns ... truedocs were false: bothhandle_announcementmethods return(). last-updated-utc09:20 equals the last spec log entry; the EPIC stamp 21:07 equals its new entry.- Progress-log actors are agent names, which the template's
{Role/Agent}cell allows. - Manual verification is waived by recorded maintainer decision (
ISSUE.md:180,227);create-issueoffers no waiver path, which is a skill gap rather than a defect of this PR. - PR body has the sections
open-pull-request/SKILL.md:105-110asks for andCloses #2435; its Changes list matches the diff, and its verification claims are reported, not recomputed here. The title scope names the package whose API changed.
Checks
On the loop's compute hub at this head, base develop 632f8b177 (receipt server-gates-krkavec-75): pre-commit profile gate exit 0 (146 s), clippy-allow-reasons --base-ref 632f8b177 exit 0, tree clean after the gate; linter markdown, linter cspell, linter lychee exit 0; cargo +stable test --workspace --all-targets --all-features: 2988 passed, 0 failed (83 s); cargo +nightly clippy --workspace --all-targets --all-features -- -D warnings: 0 diagnostics. GitHub checks at this head at posting time: 3 completed/skipped; 27 completed/success. At posting, develop is ed3cea204 (the merges of #2441 and #2442) and GitHub reports this head as dirty against it (the EPIC.md overlap with the merged #2441), so a rebase is needed before merge; the review stands for the bytes at this head.
…on plan Keep Result on the registry API for forward compatibility of public packages, replace the Infallible alias with an uninhabited non_exhaustive error, and propagate it to the delivery layers. Adds the T1 inventory, splits implementation into T3-T6, adds an ADR task, and drops manual scenario M1 per maintainer decision.
…ble public APIs Records the torrust#2435 policy: public package operations that may become fallible keep Result, using a crate-owned uninhabited non_exhaustive error instead of Infallible, and consumers propagate it. Links the policy from the handle-errors-in-code skill.
…lias with a non-exhaustive empty enum Consumer crates must now handle Err, so adding the first real failure is not a breaking change (torrust#2435). The # Errors docs no longer claim a lock-acquisition panic. A compile_fail doctest guards the property.
StatsQueryPort::get_stats and StatsApiService::get_stats now return Result with a protocol-level StatsError, and the stats handler maps Err to a 500 response. The tracker adapter returns Ok until the torrent repository propagates registry errors (torrust#2435).
…pecting them InMemoryTorrentRepository now returns the registry Result from its ten registry-backed methods instead of calling expect and documenting impossible panics (torrust#2435). The error propagates to the delivery layers: AnnounceError/ScrapeError gain a SwarmRegistry variant (HTTP failure_reason, UDP InternalServer error kind), TorrentsManager::cleanup_torrents returns Result and the cleanup job logs failures, udp-core/udp-server get_metrics return Result, and the REST stats adapter maps it to StatsError (500).
…emantics Renames the ADR to 'Use Crate-Owned Non-Exhaustive Errors for Potentially Fallible Public APIs' and revises it after review without changing the decision: it separates abstraction semantics from current implementation capabilities, explains that Infallible states the wrong contract, replaces 'forced to handle Err' with 'cannot treat the error as uninhabited', limits the scope to independently consumed APIs, frames the empty-enum pattern as a repository convention, and cites RFC 2008, C-GOOD-ERR, the Reference, and std::convert::Infallible. Drops PartialEq/Eq from the registry Error per the new derive guidance and documents the compile_fail doctest's intent (torrust#2435).
…ulative Results Records the maintainer's reversal: the registry methods will return plain values instead of an uninhabited error, because no plausible failure exists and a semver-signalled breaking change is cheaper than plumbing every consumer pays for, even with crates.io publication weeks away (EPIC torrust#1669). Keeps the option C decision as history and plans the ADR rewrite, reverts, and registry change.
Rewrites the unmerged torrust#2435 ADR for the revised decision: a public API returns Result only when it can fail today, crosses an I/O boundary, or is a swappable port with a realistically fallible backend. Otherwise it returns a plain value and a later failure is a semver-signalled breaking change. Infallible or empty error enums are not used as placeholders, and existing public error enums get non_exhaustive before first publish. Records the reverted empty-enum approach as an alternative.
… error This reverts commit 6c4066e69 (option C propagation) after the torrust#2435 decision moved to option B. The spec changes from that commit are kept. The repository temporarily returns to expect calls on the registry results; the next commits remove the registry Result entirely.
This reverts commit bb769d1e9 (StatsError, Result-returning stats port, and its stub-port test) after the torrust#2435 decision moved to option B: tracker stats aggregation has no failure mode. The spec changes from that commit are kept; T9 is marked done.
…lible registry methods The ten registry methods that could never fail now return T instead of Result<T, Error>, and the empty Error type is deleted (torrust#2435, option B). InMemoryTorrentRepository becomes plain delegation with no expect calls and no # Panics sections. Also removes the false '# Returns true' docs on both handle_announcement methods, which return ().
Before a package's first crates.io publish, mark public error enums with real variants non_exhaustive, derive only traits every future variant can keep, and confirm no Result<_, Infallible> or empty error enum placeholders remain (ADR 20261005145329, issue torrust#2435). Marks torrust#2435 T11 done.
When the registry query methods stopped returning Result (torrust#2435), ignoring their value no longer produced a warning. Restore that warning on the seven read-only queries; the remove_* commands are left unmarked because callers may ignore their counts.
Links the ADR from the spec, marks the option C decision as superseded, adds the T11 commit point, corrects the T1 inventory, records the post-rebase hashes of the reverted commits, ticks the reviewer checkpoint, and clarifies the ADR's mention of the reverted enum variants.
Records the reversal from option C (uninhabited non_exhaustive error with full propagation) to option B (plain values): what went well, the root cause (the premise that each operation could plausibly fail was never tested against a concrete method), reusable improvements, and what not to overcorrect.
Rebases rewrite PR-branch commit ids, so the spec and retrospective now cite the Conventional Commit subjects, as the open-pull-request skill requires.
The rule no longer contradicts the handle-errors-in-code Unwrap and Expect Policy: it forbids expecting a workspace API's impossible error (change the API instead), while a call-site expect on an API that can fail in general but not for this input keeps the skill's policy and the # Panics section clippy requires. Addresses PR torrust#2445 review finding F1.
Condition 3 now requires an existing or planned backend that can fail, not mere swappability, links the repository's port definition (contract-first REST API ADR), 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. Addresses PR torrust#2445 review finding F4.
…ir ADR Adds module-level doc links and adr markers to registry.rs and in_memory.rs pointing to ADR 20261005145329, as create-adr Step 3.5 requires. Addresses PR torrust#2445 review finding F2.
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, and contain no expect or unwrap. Addresses PR torrust#2445 review finding F5.
…ivery Strategy The checklist is a decided procedure, not an open question. Addresses PR torrust#2445 review finding F7.
… findings Adds agent-review-reports.md with the independent task review and its checkpoint (F3), scopes the AC5 evidence command to code paths (F6), names the final ADR file in the T3 row, marks the pre-push checkpoint done, and sets related-pr to 2445. Addresses PR torrust#2445 review findings F3 and F6 and two Copilot findings.
d2c00ea to
571b816
Compare
Records the eleven round-1 findings (da2ce7 F1 to F7, Copilot F8 to F11) with dispositions, verification, resolution references, and reply URLs.
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at 1ea8054cf7d7ce64768a672db1fa058847c03bd1 (round 2). Recomputed from the bytes at this head, against develop at d0c97baef.
A pass at the previous tip was superseded when the audit record was pushed. This head is the round-1 commits rebased onto the moved develop, six commits answering F1–F7 and Copilot's F8/F9, and the audit record. A rebase gets its own round because conflict resolution changes bytes the earlier review never saw.
Rebase. git range-diff shows fifteen of the sixteen round-1 commits unchanged. The EPIC checklist commit is the one that changed: it drops its stamp edit, keeps the checklist and related-artifact hunks, and moves its 2026-10-05 21:07 log entry into date order. It also deletes the first three lines of the 2026-10-05 15:53 entry merged with #2441, leaving that entry's last line attached to the 2026-06-09 entry (F12). EPIC.md is the only file both the develop delta and this PR touch, and no manifest or lockfile changes.
F1–F7 at the bytes.
- F1 fixed: ADR
:73-78forbidsexpectonly on a workspace API'sResultwhose error cannot occur and sends other call sites to the Unwrap and Expect Policy (handle-errors-in-code/SKILL.md:86, "Only when failure is logically impossible"), so the two agree. - F2 fixed:
registry.rs:1-5andin_memory.rs:1-5link the ADR in thepersisted_downloads.rsform. - F3 fixed:
agent-review-reports.mdfollows the template, andISSUE.md:184adds its checkpoint verbatim. - F4 fixed: condition 3 defines a port, links the contract-first ADR, and places the REST ports as the code has them.
- F5 fixed:
services.rsloses exactly the three# Panicssections (−12). - F6 fixed: AC5's command is scoped
-- packages src; the same cell's file list is now stale (F13). - F7 placement fixed: the checklist is at
EPIC.md:784under Delivery Strategy, and stamp 10:40 equals its log entry. The reply's "both log histories kept" does not hold (F12).
Copilot's F8–F11. F8 and F9 hold. The F10/F11 reasoning holds: both counts are logged (registry.rs:273,303) and discarded by every caller of the registry methods. Both replies use the Superseded by <FindingId>: <reason>. form.
Audit record. PR-REVIEW.md (+268, the only change since the previous tip) has eleven rows with matching detail entries.
- Every Source URL, Source review ID and Reply URL matches the thread capture.
- Every Resolution reference is a branch subject, or the reply URL for F10/F11.
- F1–F7 severities equal the posted brackets (
process-pr-review/SKILL.md:226), and Copilot's rows carry(inferred), mapped from its Medium/Low badges. link-integrity,metadata,maintainabilityandRE_RAISE_OF:F10are template values.- The log is in order (10:06, 10:33, 10:47, 10:51, all before the record commit at 10:55). Unlike the #2440/#2441 records it does not log the two reviews themselves; the template does not require that.
- The
RESOLVED/SUPERSEDEDstates are forward-looking, since all eleven threads are still open. - The approval of the dispositions "in chat" cannot be verified from the bytes.
Two record sentences are false at this head: F7's "keeping both log histories in chronological order" (F12) and F10's Current-tree verification (F14). Once this review posts, the record owes rows for F12–F14 and a log entry.
Carried forward. The code, EPIC and spec bytes are identical to the previous tip, so the greps, the ten plain return types and the byte-exact reverts stand, and F12/F13 anchor on the same lines. The branch has 23 Conventional commits; the new docs(pr-reviews) commit (10:55:07Z, after 10:43:38Z) has the same author and committer, no trailers and no manifest change.
Findings
- F12 Major (blocking): the rebase deleted three lines of the EPIC Progress Log entry merged with #2441.
- F13 Minor (blocking, false evidence): AC5 says the net code diff touches four files; it touches five.
- F14 Minor (blocking, false evidence): F10's Current-tree verification grep matches 106 lines in 25 files, not only
in_memory.rsand the registry tests.
Threads
Resolve F1–F6 (verified at this head). Hold F7 until F12 is fixed. Copilot's four threads are the author's to close.
Checks
On the loop's compute hub at this head, base develop d0c97baef (receipt server-gates-krkavec-78): pre-commit profile gate exit 0 (73 s), clippy-allow-reasons --base-ref d0c97baef exit 0, tree clean after the gate; linter markdown, linter cspell, linter lychee exit 0; cargo +stable test --workspace --all-targets --all-features: 2988 passed, 0 failed (72 s); cargo +nightly clippy --workspace --all-targets --all-features -- -D warnings: 0 diagnostics; validate-audit-record.py --pr-number 2445 over the live review comments captured after the eleven replies: 11 rows, 4 log entries, 0 failures (it checks recorded rows only; F12–F14 are new at this review). The previous tip 571b816d5 was gated the same way (receipt 77: tests 2988/0, clippy 0). GitHub checks at this head at posting time: 1 completed/skipped; 23 completed/success; 3 in_progress/null.
…rust#2445 rebase Resolving the rebase conflict with torrust#2441 dropped the first three lines of the 2026-10-05 15:53 UTC Progress Log entry, leaving its last line attached to the 2026-06-09 entry. Restores them so the entry is whole and precedes the 21:07 entry. Addresses PR torrust#2445 review finding F12.
The F5 fix made torrent/services.rs (doc-only) a fifth file in the net code diff against develop, so the AC5 evidence cell and the retrospective's four-file claim were stale. Addresses PR torrust#2445 review finding F13.
…audit The unscoped pattern matched 106 lines in 25 files. Scoping it to the registry receiver reproduces the claim: eight call sites, in_memory.rs and registry tests, each discarding the count. F11 reuses this verification. Addresses PR torrust#2445 review finding F14.
Records round-2 findings F12 to F14 (da2ce7) with dispositions, verification, resolution references, and reply URLs, plus the round's log entries.
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at 657e7b5c8be031dc24bdb84616beba869787073b (round 3). Recomputed from the bytes at this head, against develop at d0c97baef.
A pass at the previous tip was superseded when the round-2 audit record was pushed. This head is the round-2 tip plus three fix commits and that record (+60 in PR-REVIEW.md, the only change).
Audit record. It now has fourteen rows and fourteen detail entries, one for every posted thread.
- F12 (
Major), F13 (Minor) and F14 (Minor) equal the posted brackets (process-pr-review/SKILL.md:226). Their categories (correctness,documentation,documentation) are template values. - Each cites review 5427427892, its source (
#discussion_r4194514062/…077/…089), its reply on the same thread (#discussion_r4194736590/…6857/…7072), and one fix subject that exists on the branch. - All three Current-tree verifications reproduce at this head: the EPIC diff deletes only the old stamp,
packages srcholds 5 files, and the scoped grep prints its eight lines. The F13 claim about the PR body matches the captured body. - The log is in order and matches the events: 11:08 is the review (submitted 11:08:06Z), 11:30 is the fixes (authored 11:19:38Z–11:25:26Z) and the push, and 11:33 is the replies (11:33:36Z–11:33:40Z). The record commit at 11:34:53Z follows its last entry.
- Unlike the round-1 entries, the record now logs a review submission itself; that is a scope change within one record, not an error.
RESOLVEDis the forward state for F12–F14. The capture already shows all fourteen threads resolved.
The validator sees fourteen rows covering every thread. Only this round's log entry is owed.
Carried forward. Nothing outside the record changed since the round-3 pass, so its results stand. F12 is fixed (the restored lines are byte-equal to develop's, and the EPIC delta is intent only), F13 is fixed (five files in AC5, the retrospective and the PR body), and F14 is fixed. F7's history claim is true. The round-2 results (rebase, F1–F11, the clean greps) stand. Hygiene covers 27 Conventional commits; the new docs(pr-reviews) commit has the same author and committer, 11:34:53Z after 11:25:26Z, no trailers and no manifest change. CI at capture has no failures.
Findings
None new.
Threads
Resolve F12, F13 and F14 (fixed at the bytes); the capture shows them already resolved.
Checks
On the loop's compute hub at this head, base develop d0c97baef (receipt server-gates-krkavec-83): pre-commit profile gate exit 0 (75 s), clippy-allow-reasons --base-ref d0c97baef exit 0, tree clean after the gate; linter markdown, linter cspell, linter lychee exit 0; cargo +stable test --workspace --all-targets --all-features: 2988 passed, 0 failed (126 s); cargo +nightly clippy --workspace --all-targets --all-features -- -D warnings: 0 diagnostics; validate-audit-record.py --pr-number 2445 over the live review comments at this head: 14 rows, 7 log entries, 0 failures. The previous tip ee35bbf7a was gated the same way (receipt 82: tests 2988/0, clippy 0, validator 11/4/0). GitHub checks at this head at posting time: 1 completed/skipped; 18 completed/success; 7 in_progress/null.
|
ACK 657e7b5 — F12–F14 fixes verified at the bytes (EPIC log lines restored byte-equal to develop, five code files named in AC5 and the retrospective, F10's scoped grep prints its eight lines) and the audit record verified complete at fourteen rows for fourteen threads; only this round's log entry owed |
|
ACK 657e7b5 |
…closed Moves the torrust#2435 spec folder (ISSUE.md, agent-review-reports.md, implementation-retrospective.md) to docs/issues/closed/ after PR torrust#2445 merged and the issue closed. Sets status done and the new spec-path, ticks the archive checkpoint, adds PR torrust#2445 to Related PRs, and updates the live references in ADR 20261005145329 and the PR torrust#2445 audit front matter. The PR torrust#2436 audit keeps the old path as a historical record.
…closed Moves the torrust#2435 spec folder (ISSUE.md, agent-review-reports.md, implementation-retrospective.md) to docs/issues/closed/ after PR torrust#2445 merged and the issue closed. Sets status done and the new spec-path, ticks the archive checkpoint, adds PR torrust#2445 to Related PRs, and updates the live references in ADR 20261005145329 and the PR torrust#2445 audit front matter. The PR torrust#2436 audit keeps the old path as a historical record.
…closed Moves the torrust#2435 spec folder (ISSUE.md, agent-review-reports.md, implementation-retrospective.md) to docs/issues/closed/ after PR torrust#2445 merged and the issue closed. Sets status done and the new spec-path, ticks the archive checkpoint, adds PR torrust#2445 to Related PRs, and updates the live references in ADR 20261005145329 and the PR torrust#2445 audit front matter. The PR torrust#2436 audit keeps the old path as a historical record.
33938bc docs(pr-reviews): audit the review of #2453 (Jose Celano) 2e64602 chore(issues): archive closed issue #2435 spec to docs/issues/closed (Jose Celano) Pull request description: Archives the spec for issue #2435 (closed on GitHub after PR #2445 merged) from `docs/issues/open/` to `docs/issues/closed/`. Related to #2435 - Verified issue #2435 is `CLOSED` on GitHub - Moved the whole spec folder: `ISSUE.md`, `agent-review-reports.md`, `implementation-retrospective.md` - Updated `ISSUE.md` front matter (`status: done`, `spec-path`, `last-updated-utc`), ticked the archive checkpoint, added PR #2445 to Related PRs, and added a progress-log entry - Updated the moved supplementary files' `related-artifacts` paths - Repaired live references to the old path: ADR `20261005145329` (front matter and the References link) and the PR #2445 audit front matter - Kept the PR #2436 audit's evidence sentence unchanged: it is a historical record of the path at that time - Pre-commit and pre-push hooks passed ACKs for top commit: josecelano: ACK 33938bc Tree-SHA512: e201e25c7bcf754397bc1cccc25967ee948274d64ef35d1de5690ace61902c14659c45de0015f05d6c70f54db02a6135d8d7a445f86b0f3f4d7a165356e31cdb
Closes #2435
Summary
InMemoryTorrentRepositorycalled.expect(...)on every swarm registry result and documented# Panicsfor failures that could not happen: the registry's error type waspub type Error = Infallible. The ten registry methods now return plain values, the registryErrortype is deleted, and the repository passes calls straight through with noexpectand no# Panics. The seven read-only registry queries are#[must_use], which keeps the unused-value warning they had while they returnedResult.Decision and policy
The issue offered three options. Option C (keep
Result, replaceInfalliblewith an empty#[non_exhaustive]error enum, and propagate it to every delivery layer) was implemented first, then reversed to option B (return plain values). The branch keeps both on purpose: the option C commits, tworevert(...)commits, and the option B commit. The net code diff againstdevelopis five files, mostly deletions; one of them,tracker-core/src/torrent/services.rs, only loses three false# Panicsdoc sections (review finding F5).Why the reversal: nobody could name a plausible failure for counting the swarms, and the case for the other methods was just as weak. Meanwhile option C had added error variants, a REST
500path, cleanup-job error handling, and test churn, all for an error that cannot occur and cannot be tested. The full reasoning is in the spec's "Decision Revision (T7)" section and the implementation retrospective.The policy is recorded in a new root ADR,
docs/adrs/20261005145329_return_result_only_for_concretely_fallible_public_apis.md: a public API returnsResultonly when it can fail today, crosses an I/O boundary, or is a swappable port with a realistically fallible backend. Otherwise it returns a plain value, and a later real failure is a semver-signalled breaking change.Infallibleand empty error enums are not used as placeholders. Because every package will be published soon, EPIC #1669 gains a "Pre-publish API checklist" to mark existing public error enums#[non_exhaustive]before their first publish.Because this ADR sets an API convention for every workspace package, it deserves specific review attention.
Changes
swarm-coordination-registry: plain return values,Errordeleted, false# Errorsand# Returnsdocs removed,#[must_use]on the queriestracker-core:InMemoryTorrentRepositorypasses registry calls through with noexpect.unwrap()removed where results are no longerResulthandle-errors-in-codeskill rule, EPIC Overhaul: Packages #1669 checklist, issue spec, and implementation retrospectiveVerification
cargo clippy --workspace --all-targets --all-features: cleancargo test --tests --benches --examples --workspace --all-targets --all-features: 2975 passed, 0 failedlinter all, doc tests) and pre-push (nightly checks, docs, all tests): passedNotes
revert(...)commit messages cite commit ids from before the rebase; the spec's progress log names the reverted commits by subject.developafter docs(issues): [#1669] sync EPIC spec with current workspace state #2441 merged; review finding F12 restored EPIC log lines the conflict resolution had dropped.