Skip to content

refactor(udp-core, udp-server, udp-protocol, tracker-client): consolidate duplicate UDP types (#1966) #1991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
josecelano merged 3 commits into from
Jul 16, 2026
Merged

refactor(udp-core, udp-server, udp-protocol, tracker-client): consolidate duplicate UDP types (#1966) #1991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3209d2
refactor(udp-core, udp-server, udp-protocol, tracker-client): consoli…
josecelano Jul 16, 2026
1dbf405
docs(issue): update spec with PR #1991 link
josecelano Jul 16, 2026
8265e01
chore(review): fix outdated InfoHash comment referenced by Copilot (#…
josecelano Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 40 additions & 38 deletions docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
---
doc-type: issue
issue-type: task
status: planned
status: in-review
priority: p2
github-issue: 1966
spec-path: docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md
branch: "1966-1669-si-35-consolidate-duplicate-udp-types"
related-pr: null
last-updated-utc: 2026-06-30 12:00
related-pr: 1991
last-updated-utc: 2026-07-16 12:00
semantic-links:
skill-links:
- create-issue
- write-markdown-docs
related-artifacts:
- docs/issues/open/1669-overhaul-packages/EPIC.md
- docs/issues/open/1669-overhaul-packages/DECISIONS.md
Expand Down Expand Up @@ -126,45 +127,46 @@ future contributors understand the architectural reasoning and do not accidental

Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`.

| ID | Status | Task | Notes / Expected Output |
| --- | ------ | ------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| T1 | TODO | Consolidate `ConnectionContext` into `udp-core` | Make `udp-server` import from `udp-core` instead of defining its own copy |
| T2 | TODO | Move `MAX_PACKET_SIZE` to `udp-protocol` | Add `pub const MAX_PACKET_SIZE: usize = 1496;` to `udp-protocol`; update imports |
| T3 | TODO | Remove dead `PROTOCOL_ID` from `tracker-client` | Delete the unused constant |
| T4 | TODO | Add `adr:` comments for intentional duplications | Annotate `udp-protocol/src/common.rs`, `http-protocol/src/v1/requests/announce.rs`, `http-protocol/src/v1/responses/announce.rs`, `http-protocol/src/v1/responses/scrape.rs`, and `primitives/src/announce.rs` with `// adr: docs/adrs/20260527175600...` comments |
| T5 | TODO | Run full verification | `linter all`, `cargo test --workspace`, pre-commit, pre-push |
| ID | Status | Task | Notes / Expected Output |
| --- | ------ | ------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------- |
| T1 | DONE | Consolidate `ConnectionContext` into `udp-core` | Made fields private in `udp-core`, removed duplicate from `udp-server`, updated all imports to `torrust_tracker_udp_core::event::ConnectionContext` |
| T2 | DONE | Move `MAX_PACKET_SIZE` to `udp-protocol` | Added to `udp-protocol/src/common.rs`, removed from `udp-server/src/lib.rs` and `tracker-client/src/udp/mod.rs`, updated all imports |
| T3 | DONE | Remove dead `PROTOCOL_ID` from `tracker-client` | Deleted the unused constant |
| T4 | DONE | Add `adr:` comments for intentional duplications | Annotated all 5 locations with `// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md` |
| T5 | DONE | Run full verification | `cargo test --workspace --all-targets` all pass, `cargo machete` clean, no duplicate definitions remain |

## Progress Tracking

### Workflow Checkpoints

- [x] Spec drafted in `docs/issues/drafts/`
- [ ] Spec reviewed and approved by user/maintainer
- [x] Spec reviewed and approved by user/maintainer
- [ ] GitHub issue created and issue number added to this spec
- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation
- [ ] Implementation completed
- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks)
- [x] Implementation completed
- [x] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks)
- [ ] Manual verification scenarios executed and recorded (status + evidence)
- [ ] Acceptance criteria reviewed after implementation and updated with evidence
- [x] Acceptance criteria reviewed after implementation and updated with evidence
- [ ] Reviewer validated acceptance criteria and updated checkboxes
- [ ] Committer verified spec progress is up to date before commit
- [x] Committer verified spec progress is up to date before commit
- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/`

### Progress Log

- 2026-07-16 12:00 UTC - Copilot - Implementation completed. All T1-T5 done. All ACs verified. 24 files modified.
- 2026-06-30 12:00 UTC - Copilot - Spec draft created

## Acceptance Criteria

- [ ] AC1: `ConnectionContext` is defined in exactly one location (imported by the other)
- [ ] AC2: `MAX_PACKET_SIZE` is defined in `udp-protocol` and imported by both `udp-server` and `tracker-client`
- [ ] AC3: `PROTOCOL_ID` no longer exists in `tracker-client`
- [ ] AC4: Each location listed in the "Intentional duplications" section has an `adr:` comment referencing the ADR
- [ ] AC5: All existing tests pass (`cargo test --workspace`)
- [ ] AC6: `linter all` exits with code `0`
- [ ] AC7: Pre-commit and pre-push checks pass
- [x] AC1: `ConnectionContext` is defined in exactly one location (imported by the other)
- [x] AC2: `MAX_PACKET_SIZE` is defined in `udp-protocol` and imported by both `udp-server` and `tracker-client`
- [x] AC3: `PROTOCOL_ID` no longer exists in `tracker-client`
- [x] AC4: Each location listed in the "Intentional duplications" section has an `adr:` comment referencing the ADR
- [x] AC5: All existing tests pass (`cargo test --workspace`)
- [x] AC6: `linter all` exits with code `0`
- [x] AC7: Pre-commit and pre-push checks pass
- [ ] Manual verification scenarios are executed and documented (status + evidence)
- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior
- [x] Acceptance criteria are re-reviewed after implementation and reflect actual behavior

## Verification Plan

Expand All @@ -180,24 +182,24 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`.

Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`.

| ID | Scenario | Command/Steps | Expected Result | Status | Evidence |
| --- | ---------------------------------------------- | --------------------------------------------------------------------------------------- | ----------------------------------------- | ------ | ---------------------------- |
| M1 | UDP tracker announces work with tracker-client | Run `tracker_client udp announce` against a local tracker; verify request/response flow | Same behavior as before the consolidation | TODO | {log/output/screenshot/path} |
| M2 | UDP scrape works with tracker-client | Run `tracker_client udp scrape` against a local tracker | Same behavior as before | TODO | {log/output/screenshot/path} |
| M3 | udp-server tests pass | `cargo test -p torrust-tracker-udp-server` | All tests pass | TODO | {log/output/screenshot/path} |
| M4 | No duplicate definitions remain | `grep` for `ConnectionContext` and `MAX_PACKET_SIZE` across workspace | Only one definition each | TODO | {log/output/screenshot/path} |
| ID | Scenario | Command/Steps | Expected Result | Status | Evidence |
| --- | ---------------------------------------------- | --------------------------------------------------------------------------------------- | ----------------------------------------- | ------ | ------------------------- |
| M1 | UDP tracker announces work with tracker-client | Run `tracker_client udp announce` against a local tracker; verify request/response flow | Same behavior as before the consolidation | TODO | Pending — manual E2E test |
| M2 | UDP scrape works with tracker-client | Run `tracker_client udp scrape` against a local tracker | Same behavior as before | TODO | Pending — manual E2E test |
| M3 | udp-server tests pass | `cargo test -p torrust-tracker-udp-server` | All tests pass | DONE | 122 unit + 7 integration |
| M4 | No duplicate definitions remain | `grep` for `ConnectionContext` and `MAX_PACKET_SIZE` across workspace | Only one definition each | DONE | Verified via grep output |

### Acceptance Verification

| AC ID | Status (`TODO`/`DONE`) | Evidence |
| ----- | ---------------------- | ------------------ |
| AC1 | TODO | {test/log/PR link} |
| AC2 | TODO | {test/log/PR link} |
| AC3 | TODO | {test/log/PR link} |
| AC4 | TODO | {test/log/PR link} |
| AC5 | TODO | {test/log/PR link} |
| AC6 | TODO | {test/log/PR link} |
| AC7 | TODO | {test/log/PR link} |
| AC ID | Status (`TODO`/`DONE`) | Evidence |
| ----- | ---------------------- | ------------------------------------------------------------------------------- |
| AC1 | DONE | grep output: single `pub struct ConnectionContext` in `udp-core/src/event.rs` |
| AC2 | DONE | grep output: single `pub const MAX_PACKET_SIZE` in `udp-protocol/src/common.rs` |
| AC3 | DONE | grep output: zero references to `PROTOCOL_ID` in `tracker-client` |
| AC4 | DONE | `adr:` comments added to all 5 locations |
| AC5 | DONE | `cargo test --workspace --all-targets` — all pass |
| AC6 | DONE | `linter all` — exit code 0 |
| AC7 | DONE | Pre-commit and pre-push checks pass |

## Risks and Trade-offs

Expand Down
46 changes: 46 additions & 0 deletions docs/pr-reviews/pr-1991-copilot-suggestions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
semantic-links:
skill-links:
- process-copilot-suggestions
related-artifacts:
- .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md
---

<!-- skill-link: process-copilot-suggestions -->

# PR #1991 Copilot Suggestions Tracking

Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/1991

Status legend:

- `action`: code/docs change applied
- `no-action`: suggestion reviewed; no code change needed
- `resolved`: thread resolved in PR

## Workflow

1. Download all review threads (including resolved/outdated state and thread IDs).
2. Add one row per thread in the Suggestions table.
3. Process suggestions one by one:
- decide `action` or `no-action`
- if `action`, apply change and validate
- if needed, commit changes
- resolve the PR thread
4. Set `Thread State` to `resolved` once resolved in PR.

## Processing Log

- 2026-07-16: Started processing suggestions.
- 2026-07-16: Completed processing suggestions.

## Suggestions

| # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State |
| --- | --------------------- | ------------------------------------- | -------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- | -------- | ------ | ------------ |
| 1 | PRRT_kwDOGp2yqc6Rb5YB | `packages/udp-protocol/src/common.rs` | [comment](https://github.com/torrust/torrust-tracker/pull/1991#discussion_r3595317028) | `InfoHash` comment references deprecated `bittorrent-primitives` instead of `torrust_info_hash` | action | DONE | resolved |

## Notes

- The suggestion is valid: the comment in `common.rs` on `InfoHash` references `bittorrent-primitives::InfoHash` which is a deprecated crate path. Updated to `torrust_info_hash::InfoHash`.
- No other suggestions were found in the review.
1 change: 1 addition & 0 deletions packages/http-protocol/src/v1/requests/announce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const PEER_ADDR: &str = "peer_addr";
// `NumberOfBytes` concept and domain byte counters, but it is kept local so
// HTTP wire semantics can evolve independently without forcing cross-protocol
// or domain-wide refactors.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)]
pub struct NumberOfBytes(pub i64);

Expand Down
1 change: 1 addition & 0 deletions packages/http-protocol/src/v1/responses/announce/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use torrust_peer_id::PeerId;
// Protocol-local announce response DTOs intentionally duplicate some domain
// field shapes. This keeps protocol crates decoupled from tracker domain types
// and centralizes conversions in boundary adapters.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
#[derive(Clone, Debug, PartialEq, Constructor, Default)]
pub struct AnnounceData {
pub peers: Vec<Peer>,
Expand Down
2 changes: 2 additions & 0 deletions packages/http-protocol/src/v1/responses/scrape/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use torrust_info_hash::InfoHash;

// Intentional boundary duplication: this represents scrape response payload
// semantics for the HTTP protocol crate, not tracker-domain semantics.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
#[derive(Clone, Copy, Debug, PartialEq, Eq, Default)]
pub struct SwarmMetadata {
pub complete: u32,
Expand All @@ -18,6 +19,7 @@ pub struct SwarmMetadata {

// Intentional boundary duplication: this represents scrape response payload
// semantics for the HTTP protocol crate, not tracker-domain semantics.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
#[derive(Clone, Debug, PartialEq, Default)]
pub struct ScrapeData {
pub files: BTreeMap<InfoHash, SwarmMetadata>,
Expand Down
5 changes: 5 additions & 0 deletions packages/primitives/src/announce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ pub struct AnnounceData {
pub policy: AnnouncePolicy,
}

/// Intentional boundary duplication: this domain type mirrors
/// protocol-level `AnnounceEvent` definitions in `udp-protocol` and
/// `http-protocol`, but is kept here so domain logic does not depend on
/// protocol wire formats.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)]
pub enum AnnounceEvent {
Started,
Expand Down
3 changes: 1 addition & 2 deletions packages/tracker-client/src/udp/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@ use std::time::Duration;
use tokio::net::UdpSocket;
use tokio::time;
use torrust_net_primitives::service_binding::ServiceBinding;
use torrust_tracker_udp_protocol::{ConnectRequest, Request, Response, TransactionId};
use torrust_tracker_udp_protocol::{ConnectRequest, MAX_PACKET_SIZE, Request, Response, TransactionId};
use zerocopy::byteorder::network_endian::I32;

use super::Error;
use crate::udp::MAX_PACKET_SIZE;

pub const UDP_CLIENT_LOG_TARGET: &str = "UDP CLIENT";

Expand Down
6 changes: 0 additions & 6 deletions packages/tracker-client/src/udp/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@ use torrust_tracker_udp_protocol::Request;

pub mod client;

/// The maximum number of bytes in a UDP packet.
pub const MAX_PACKET_SIZE: usize = 1496;
/// A magic 64-bit integer constant defined in the protocol that is used to
/// identify the protocol.
pub const PROTOCOL_ID: i64 = 0x0417_2710_1980;

#[derive(Debug, Clone, Error)]
pub enum Error {
#[error("Timeout while waiting for socket to bind: {addr:?}")]
Expand Down
4 changes: 2 additions & 2 deletions packages/udp-core/src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ pub enum Event {

#[derive(Debug, PartialEq, Eq, Clone)]
pub struct ConnectionContext {
pub client_socket_addr: SocketAddr,
pub server_service_binding: ServiceBinding,
client_socket_addr: SocketAddr,
server_service_binding: ServiceBinding,
}

impl ConnectionContext {
Expand Down
8 changes: 7 additions & 1 deletion packages/udp-protocol/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ use zerocopy::{FromBytes, Immutable, IntoBytes};

pub trait Ip: Clone + Copy + Debug + PartialEq + Eq + std::hash::Hash + IntoBytes + Immutable {}

/// The maximum number of bytes in a UDP packet.
pub const MAX_PACKET_SIZE: usize = 1496;

#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, IntoBytes, FromBytes, Immutable)]
#[repr(transparent)]
// Intentionally kept in `common`: this protocol-level wire type mirrors
// `bittorrent-primitives::InfoHash` and may be unified across packages later.
// `torrust_info_hash::InfoHash` but is kept protocol-local so that wire
// representations can evolve independently of domain types.
Comment thread
josecelano marked this conversation as resolved.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
pub struct InfoHash(pub [u8; 20]);

#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, IntoBytes, FromBytes, Immutable)]
Expand Down Expand Up @@ -47,6 +52,7 @@ impl TransactionId {
// `packages/primitives/src/number_of_bytes.rs` and HTTP protocol byte counters,
// but remains UDP-local so protocol wire representations can evolve
// independently per protocol.
// adr: docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md
pub struct NumberOfBytes(pub I64);

impl NumberOfBytes {
Expand Down
Loading
Loading