Skip to content

feat(registry): add Registar::deregister - #2

Merged
josecelano merged 1 commit into
mainfrom
2450-registar-deregister
Oct 26, 2026
Merged

josecelano merged 1 commit into
mainfrom
2450-registar-deregister

Conversation

@josecelano

Copy link
Copy Markdown
Member

Summary

  • Add Registar::deregister(&ServiceBinding) -> Option<RegisteredService<M>>, an additive async method that removes one registration and returns it, or returns None when the binding is not registered.
  • A stopped service can then register the same final binding again. Without it, a stop-then-start on a fixed port fails with RegistrationError::DuplicateBinding.
  • The registry is keyed by binding only, so the method removes whichever registration holds that binding. The doc comment tells callers to pass only a binding they registered themselves. A receipt-based API that removes only its own entry was considered and left out as too large for now; it can be built on this method later.
  • Bump torrust-server-lib from 0.3.0 to 0.4.0 for the additive public API, as the previous additive API did (0.2.0 to 0.3.0).

Validation

  • cargo test: 10 passed, including 4 new tests (remove and return, register again after removal, None for an unknown binding, other registrations kept). The last was mutation-proven: clearing the whole map made it fail.
  • cargo fmt --check, cargo clippy --all-targets -- -D warnings, linter all
  • cargo publish --dry-run

Related to torrust/torrust-tracker#2450.

Add an additive async deregister(&ServiceBinding) that removes one registration and returns it, so a stopped service can register its final binding again. Bump torrust-server-lib from 0.3.0 to 0.4.0 for the additive public API, as the previous additive API did (0.2.0 to 0.3.0).

Related to torrust/torrust-tracker#2450.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 26, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The mutex-protected removal matches its documented contract, focused tests cover the intended behavior, and no blocking issues were identified.

No open issues 🎉

What changed in this PR

Adds Registar::deregister so stopped services can release their registrations and later register the same binding again.

Changes:

  • Adds an async removal method that returns the removed service or None, with documented caller ownership expectations.
  • Adds four tests covering removal, re-registration, unknown bindings, and preservation of other registrations.
  • Bumps the crate version from 0.3.0 to 0.4.0.
File Description
src/​registar.rs Adds deregistration, API documentation, and regression tests.
Cargo.toml Updates the crate version to 0.4.0.
Cargo.lock Synchronizes the crate version with the manifest.

Review effort: Balanced

@josecelano
josecelano merged commit bb13375 into main Oct 26, 2026
5 checks passed
josecelano added a commit to torrust/torrust-tracker that referenced this pull request Oct 26, 2026
…the health-check API test environment to the token lifecycle

6c0d9cd docs(pr-reviews): [#2450] record the review audit of PR 2652 (Jose Celano)
8ed7da2 docs(issues): [#2450] record the independent review of the final tree (Jose Celano)
6c21b1d docs(issues): [#2450] record the prose-first comparison of the new tests and fix the review findings (Jose Celano)
0042a18 docs(issues): [#2450] record the registry deregistration and update the spec to the final design (Jose Celano)
cb8c34f fix(axum-health-check-api-server): [#2450] deregister the health-check test environment on stop (Jose Celano)
c554b24 chore(deps): [#2450] bump torrust-server-lib to 0.4.0 (Jose Celano)
70b2141 docs(issues): [#2450] correct the independent review entry after the fixed-port finding (Jose Celano)
be0acb8 docs(issues): [#2450] record the fixed-port restart limitation and its retrospective (Jose Celano)
e810655 test(axum-health-check-api-server): [#2450] document and pin the fixed-port restart limitation (Jose Celano)
f0656bc docs(issues): [#2450] record the independent review, resolve its findings and update the spec status (Jose Celano)
aa28b3f test(axum-health-check-api-server): [#2450] assert the stop result in the binding-release test (Jose Celano)
8a09fe5 docs(issues): [#2450] record the prose-first comparison and the post-change checks (Jose Celano)
6cf6534 docs(shutdown-process): [#2450] record the health-check test environment token lifecycle (Jose Celano)
a381a86 docs(issues): [#2450] record T0 baseline, T1 mutation proofs and M1/M2 evidence (Jose Celano)
4830852 refactor(axum-health-check-api-server): [#2450] migrate the health-check API test environment to the token lifecycle (Jose Celano)

Pull request description:

  Closes #2450

  Migrates the health-check API test `Environment` to the token lifecycle (SI-24, EPIC #1488 roadmap row 17), following the #2449 pattern.

  What the change does: private `state`, `Stopped` and `Running` structs, one cancellation token held as one `DropGuard`, and a `stop()` that disarms and cancels before its first await, joins both tasks and returns an error naming every failing task. A new `bind_address()` accessor replaces the 8 field reads in the contract tests.

  Review round: Copilot and da2ce7 found that a stop-then-start on a fixed port panicked with `DuplicateBinding`, because the token-aware start registers the service and `Registar` could not remove it. This PR now fixes it: `stop()` removes the environment's own registration with `Registar::deregister`, which is new in `torrust-server-lib` 0.4.0 (torrust/torrust-server-lib#2, merged and published). The decision to first document the limitation was reversed; see the evidence file, section V7. The audit is `docs/pr-reviews/pr-2652-review/PR-REVIEW.md`.

  Evidence and reviews: `manual-verification-evidence.md` (V0 to V7, mutation proofs), `agent-review-reports.md` (an earlier review entry, its correction, and a review of the final tree) and `implementation-retrospective.md`. Acceptance criteria AC1 to AC8 are left for the maintainer to tick.

  Head to merge: the current head of this branch, `docs(pr-reviews): [#2450] record the review audit of PR 2652`. Any later push will be announced in a comment on this pull request.

  Commits, oldest first:

  - `refactor(axum-health-check-api-server): [#2450] migrate the health-check API test environment to the token lifecycle`
  - `docs(issues): [#2450] record T0 baseline, T1 mutation proofs and M1/M2 evidence`
  - `docs(shutdown-process): [#2450] record the health-check test environment token lifecycle`
  - `docs(issues): [#2450] record the prose-first comparison and the post-change checks`
  - `test(axum-health-check-api-server): [#2450] assert the stop result in the binding-release test`
  - `docs(issues): [#2450] record the independent review, resolve its findings and update the spec status`
  - `test(axum-health-check-api-server): [#2450] document and pin the fixed-port restart limitation`
  - `docs(issues): [#2450] record the fixed-port restart limitation and its retrospective`
  - `docs(issues): [#2450] correct the independent review entry after the fixed-port finding`
  - `chore(deps): [#2450] bump torrust-server-lib to 0.4.0`
  - `fix(axum-health-check-api-server): [#2450] deregister the health-check test environment on stop`
  - `docs(issues): [#2450] record the registry deregistration and update the spec to the final design`
  - `docs(issues): [#2450] record the prose-first comparison of the new tests and fix the review findings`
  - `docs(issues): [#2450] record the independent review of the final tree`
  - `docs(pr-reviews): [#2450] record the review audit of PR 2652`

  Files touched:

  - `Cargo.lock`
  - `Cargo.toml`
  - `docs/features/shutdown-process/task-inventory.md`
  - `docs/issues/open/2450-1488-si-24-migrate-health-check-api-test-environment/ISSUE.md`
  - `docs/issues/open/2450-1488-si-24-migrate-health-check-api-test-environment/agent-review-reports.md`
  - `docs/issues/open/2450-1488-si-24-migrate-health-check-api-test-environment/implementation-retrospective.md`
  - `docs/issues/open/2450-1488-si-24-migrate-health-check-api-test-environment/manual-verification-evidence.md`
  - `docs/pr-reviews/pr-2652-review/PR-REVIEW.md`
  - `packages/axum-health-check-api-server/Cargo.toml`
  - `packages/axum-health-check-api-server/src/environment.rs`
  - `packages/axum-health-check-api-server/src/server.rs`
  - `packages/axum-health-check-api-server/tests/server/contract.rs`
  - `packages/axum-http-server/Cargo.toml`
  - `packages/axum-rest-api-server/Cargo.toml`
  - `packages/axum-server/Cargo.toml`
  - `packages/udp-server/Cargo.toml`

ACKs for top commit:
  josecelano:
    ACK 6c0d9cd

Tree-SHA512: 9ebf7837f949641d98f98580e509c1a2a044ef8119b38d9379426fc47ace445a3f9955541a77a729fcc16ec5048b0f8b230d6299850706bae8eac430b9bc69b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants