From 26aae7896da5cf86b45ecc3c3f9a1621e9a28584 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 27 Jul 2026 12:42:14 +0100 Subject: [PATCH 1/9] docs(issues): add specification for parallel integration test infrastructure Issue spec for #1419 covering: - Three problems: logging, port conflicts, config isolation - Temp directory pattern for complete test isolation - 8-task implementation plan with prove-then-fix strategy - 9 acceptance criteria and verification plan --- ...ple-integration-tests-at-main-app-level.md | 268 ++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md new file mode 100644 index 000000000..05c60c06d --- /dev/null +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md @@ -0,0 +1,268 @@ +--- +doc-type: issue +issue-type: enhancement +status: open +priority: p3 +github-issue: 1419 +spec-path: docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md +branch: 1419-allow-multiple-integration-tests +related-pr: null +last-updated-utc: 2026-07-27 08:00 +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - tests/integration.rs + - tests/servers/ + - src/app.rs + - packages/test-helpers/ +--- + +# Issue #1419 - Allow multiple integration tests at the main app level + +## Goal + +Enable running multiple independent integration tests at the main application level (`tests/integration.rs`) +in parallel without port conflicts, configuration collisions, or logging initialization errors. + +## Background + +Currently, there is one integration test for global metrics at the main app level +(`tests/servers/api/contract/stats/mod.rs`). This test verifies behavior that can only be tested at +the main app level, specifically that multiple tracker instances (HTTP and UDP) running on different +socket addresses contribute to global metrics aggregation. + +Most tests are correctly located inside the `packages/` directory, testing individual components in +isolation. Integration tests at the main app level should be reserved for testing application-level +concerns: + +- Multiple tracker instances running simultaneously +- Global metrics aggregation across services +- Application container initialization and lifecycle +- Job manager orchestration +- Cross-service coordination +- Bootstrap and configuration integration + +Integration tests at this level offer advantages over E2E tests: + +- **Faster execution**: No docker container overhead +- **Flexible context**: Easy to modify configuration per test +- **Portable**: Run anywhere (including inside docker image builds) +- **Better debugging**: Direct access to application state + +### Current Problems + +When attempting to add a second integration test, three problems arise: + +#### ~~Problem 1: Logging initialization fails with multiple tests~~ [RESOLVED] + +**Update**: This issue can no longer be reproduced. Initial investigation showed that calling +`app::run()` with `logging.threshold = "info"` in multiple tests would fail with: + +```text +Unable to install global subscriber: SetGlobalDefaultError("a global default trace dispatcher has already been set") +``` + +However, testing with two concurrent tests using identical configuration (including +`logging.threshold = "info"`) now runs cleanly. The logger appears to handle reinitialization +gracefully, likely due to internal guards in the tracing infrastructure. + +This problem is considered resolved and requires no further action. + +#### Problem 2: Port conflicts when tests run in parallel + +Tests run concurrently by default (`cargo test` uses multiple threads). If multiple tests use the +same hard-coded ports, they fail with: + +```text +Could not bind tcp_listener to address.: Os { code: 98, kind: AddrInUse, message: "Address already in use" } +``` + +The current test uses fixed ports: + +```toml +[[http_trackers]] +bind_address = "0.0.0.0:7272" + +[[http_trackers]] +bind_address = "0.0.0.0:7373" + +[http_api] +bind_address = "0.0.0.0:1414" +``` + +**Solution**: Use port `0` for all bind addresses. The OS assigns a free ephemeral port, eliminating +conflicts: + +```toml +[[http_trackers]] +bind_address = "0.0.0.0:0" + +[http_api] +bind_address = "0.0.0.0:0" +``` + +After binding, the actual assigned ports must be retrieved from the running services to construct +request URLs for test assertions. The application already provides access to bound addresses through +the `Registar` component in `AppContainer`. + +#### Problem 3: Environment variable configuration conflicts and storage isolation + +Tests run in parallel within the same process share the same environment. If tests inject +configuration via `std::env::set_var("TORRUST_TRACKER_CONFIG_TOML", ...)`, concurrent tests +overwrite each other's configuration, causing non-deterministic failures. + +Additionally, trackers need isolated storage directories for their databases and runtime state. +Using a shared `storage/` directory or relying on default paths causes conflicts when multiple +trackers run concurrently. + +The current test uses `unsafe { env::set_var(...) }` with a safety comment acknowledging this +limitation. + +**Note**: The E2E runner ([`src/console/ci/e2e/runner.rs`](../../../src/console/ci/e2e/runner.rs)) +demonstrates a pattern where CLI arguments (`--config-toml-path`, `--config-toml`) map to these +same environment variables (`TORRUST_TRACKER_CONFIG_TOML_PATH`, `TORRUST_TRACKER_CONFIG_TOML`). +However, the main tracker binary ([`src/main.rs`](../../../src/main.rs)) does not currently +accept CLI arguments - it only reads configuration from environment variables. Adding CLI argument +support to the main binary would be a future improvement, but is out of scope for this issue. + +**Solution**: Use temporary directories (not just temp files) for complete test isolation: + +1. Create a unique temporary directory per test using `tempfile::TempDir` +2. Within the temp directory, create subdirectories for: + - Config file (e.g., `tracker-config.toml`) + - Storage directory (e.g., `tracker-storage/` for database and runtime data) +3. Configure the tracker to use these isolated paths +4. Set `TORRUST_TRACKER_CONFIG_TOML_PATH` to point to the temp config file +5. The entire temp directory and its contents are automatically cleaned up when the `TempDir` + handle is dropped + +This pattern matches the approach used in qBittorrent E2E tests +([`src/console/ci/qbittorrent_e2e/filesystem_setup.rs`](../../../src/console/ci/qbittorrent_e2e/filesystem_setup.rs)), +which creates isolated workspaces with separate config and storage directories for each test run. + +### Related work + +- E2E tests ([`src/console/ci/e2e/runner.rs`](../../../src/console/ci/e2e/runner.rs)) parse tracker + output to extract bound ports, but they run the tracker as an external process +- qBittorrent E2E tests + ([`src/console/ci/qbittorrent_e2e/filesystem_setup.rs`](../../../src/console/ci/qbittorrent_e2e/filesystem_setup.rs)) + create isolated temporary workspaces with subdirectories for config, storage, and shared fixtures + using `tempfile::TempDir` +- Package-level tests already use similar patterns (port 0, temp files) in various + `testing/environment.rs` modules + +## Scope + +### In Scope + +- Enable multiple integration tests to run concurrently without port conflicts +- Provide test utilities for managing temporary test workspaces (config + storage directories) +- Extract bound port information from `AppContainer` or `JobManager` for test assertions +- Update existing integration test to use port 0 and isolated temp workspace +- Expand global stats test coverage to verify multiple metrics +- Document patterns for writing integration tests at the main app level +- Create `tests/AGENTS.md` with guidelines for AI agents and TODO list of future integration tests + +### Out of Scope + +- Changing E2E test infrastructure +- Modifying package-level test infrastructure +- Changing logging infrastructure or tracing initialization +- Adding extensive integration test coverage (focus is on infrastructure, not coverage) +- Modifying `Registar` API (use existing capabilities only) + +## Implementation Plan + +**Strategy**: First prove the scaffolding is broken by adding a second test case that would conflict, +then fix the scaffolding infrastructure, then expand coverage. + +| ID | Status | Task | Notes | +| --- | ------ | ----------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Create `tests/AGENTS.md` with guidelines and TODO list | Document what belongs in main-level integration tests vs package tests; include prioritized TODO list of future valuable integration tests | +| T2 | TODO | Add second assertion to existing stats test | Check another global stat field (e.g., `tcp4_scrapes_handled` or `tcp6_announces_handled`) to prove need for parallel test capability | +| T3 | TODO | Create test utilities module | `tests/helpers.rs` with utilities for temp workspace creation (config + storage dirs) and port extraction | +| T4 | TODO | Add utility to create isolated temp workspace | Returns `TempDir` with subdirectories for config and storage; writes TOML config; sets `TORRUST_TRACKER_CONFIG_TOML_PATH` env var | +| T5 | TODO | Add utility to extract bound addresses from `AppContainer` | Query `Registar` or job handles to get actual bound `SocketAddr` for HTTP API, trackers, etc. | +| T6 | TODO | Update existing stats test to use port 0 and temp workspace | Replace `env::set_var` with isolated temp workspace, use port 0, extract actual ports for requests; fixes scaffolding to support parallelism | +| T7 | TODO | Expand global stats test coverage | Add tests for more global stat metrics now that scaffolding supports it (scrape counters, different IP families, etc.) | +| T8 | TODO | Run automatic verification | `cargo test --test integration` must pass with all tests running concurrently | + +## Progress Tracking + +### Workflow Checkpoints + +- [ ] Specification drafted and approved by user/maintainer +- [ ] GitHub issue #1419 already exists (created by maintainer) +- [ ] Implementation completed +- [ ] Automatic verification completed (`cargo test --test integration`) +- [ ] Acceptance criteria reviewed after implementation +- [ ] Issue closed and specification moved to `docs/issues/closed/` + +### Progress Log + +- 2025-03-27 16:40 UTC - josecelano - Created GitHub issue #1419 +- 2025-04-04 10:04 UTC - josecelano - Added comment noting Problem 1 can no longer be reproduced +- 2026-07-27 08:00 UTC - agent - Drafted issue specification +- 2026-07-27 08:37 UTC - agent - Created `tests/AGENTS.md` with guidelines and TODO list +- 2026-07-27 08:38 UTC - agent - Updated implementation plan to use prove-then-fix strategy +- 2026-07-27 13:00 UTC - agent - Updated Problem 3 and implementation plan to use temp directory + pattern (not just temp files) for complete test isolation, matching qBittorrent E2E approach + +## Acceptance Criteria + +- [ ] AC1: `tests/AGENTS.md` exists and documents guidelines for what belongs at main-level vs + package-level, with a TODO list of future valuable integration tests. +- [ ] AC2: Multiple integration tests can run concurrently with `cargo test --test integration` + without port conflicts. +- [ ] AC3: Each test uses an isolated temporary workspace with separate config and storage + directories (no shared environment variables or storage paths). +- [ ] AC4: Tests using port 0 can extract the actual bound ports from `AppContainer` to construct + request URLs. +- [ ] AC5: The existing stats integration test is updated to use an isolated temp workspace and + port 0. +- [ ] AC6: Global stats test coverage includes multiple metrics (not just `tcp4_announces_handled`). +- [ ] AC7: Test utilities for temp workspace creation (config + storage) and port extraction are + available and documented. +- [ ] AC8: `cargo test --test integration` passes cleanly with expanded test coverage. +- [ ] AC9: `linter all` passes. + +## Verification Plan + +### Automatic Checks + +- `cargo test --test integration` — Must pass with all integration tests running concurrently +- `cargo test --test integration -- --test-threads=1` — Verify tests also work in serial mode +- `linter all` — Standard quality gate + +### Manual Verification Scenarios + +| ID | Scenario | Expected Result | Status | Evidence | +| --- | ----------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------ | -------- | +| M1 | Run `cargo test --test integration` with default parallelism | All tests pass; no port conflicts or configuration collisions | TODO | | +| M2 | Run `cargo test --test integration -- --nocapture` to see logs | Each test shows unique bound ports; no env var configuration warnings | TODO | | +| M3 | Add a third integration test temporarily and run the suite | All three tests run concurrently without interference | TODO | | +| M4 | Verify temp workspaces are cleaned up after tests complete | No leftover temporary directories in `/tmp` or system temp directory | TODO | | +| M5 | Run tests in serial mode: `cargo test --test integration -- --test-threads=1` | Tests pass in serial mode (no implicit dependency on parallelism for correctness) | TODO | | + +## Risks and Trade-offs + +- **Temp workspace cleanup**: If tests panic or are interrupted, temporary directories may not be + cleaned up. This is standard behavior for integration tests and acceptable. +- **Port 0 complexity**: Tests must extract actual bound ports from the application, adding a layer + of indirection. This is necessary for parallel execution and mirrors real-world deployment scenarios. +- **Scope creep risk**: It's tempting to add many integration tests at this level. Maintain + discipline: most tests belong in `packages/`, only application-level concerns should be tested here. +- **Registar API surface**: If `Registar` doesn't expose bound addresses in a convenient form, + alternative extraction methods (e.g., parsing job handles) may be needed. Investigate existing + capabilities first. + +## Notes + +- The issue description mentions that the `Registar` type now includes "listen url" information, + making it easier to extract bound addresses. Confirm this during implementation. +- The existing test has a safety comment about `std::env::set_var` being unsafe in Rust 2024 due to + concurrent access. The temp file approach eliminates this concern entirely. +- Consider whether test utilities should live in `tests/helpers.rs` or be added to the existing + `packages/test-helpers/` package. Decision: prefer `tests/helpers.rs` to keep test-specific + utilities close to the tests and avoid polluting the shared `test-helpers` package. From 4dd469d2ea291501916aa6e770261f1468756fe7 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 27 Jul 2026 13:05:29 +0100 Subject: [PATCH 2/9] docs(issues): add draft issue for integration test coverage expansion Tracking issue for expanding main application-level integration tests. Documents the three-layer testing strategy (unit/integration/E2E) and lists 14 prioritized tests that require full application context. --- ...ease-main-app-integration-test-coverage.md | 257 ++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 docs/issues/drafts/increase-main-app-integration-test-coverage.md diff --git a/docs/issues/drafts/increase-main-app-integration-test-coverage.md b/docs/issues/drafts/increase-main-app-integration-test-coverage.md new file mode 100644 index 000000000..a199f8773 --- /dev/null +++ b/docs/issues/drafts/increase-main-app-integration-test-coverage.md @@ -0,0 +1,257 @@ +--- +doc-type: issue +issue-type: enhancement +status: draft +priority: p3 +github-issue: null +spec-path: docs/issues/drafts/increase-main-app-integration-test-coverage.md +branch: null +related-pr: null +last-updated-utc: 2026-07-27 12:00 +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - tests/integration.rs + - tests/AGENTS.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md + related-issues: + - 1347 + - 1419 +--- + +# Draft Issue - Increase Main Application-Level Integration Test Coverage + +## Goal + +Systematically expand integration test coverage at the main application level (`tests/`) to verify +application-level behaviors that can only be tested with the complete Torrust Tracker application +and multiple coordinated services. + +## Background + +The Torrust Tracker project uses a three-layer testing strategy: + +1. **Unit tests** (`packages/*/tests/`) — Fast, isolated tests for individual components +2. **Integration tests** (`tests/`) — Main application-level tests with full app context +3. **E2E tests** (`packages/e2e-tools/`, `src/console/ci/e2e/`, `src/console/ci/qbittorrent_e2e/`) + — Container-based tests with Docker Compose + +After implementing issue #1419 (parallel integration test infrastructure), the project has a +foundation for writing independent, concurrent integration tests at the main application level. +Currently, only one test suite exists (`tests/servers/api/contract/stats/`), which verifies global +metrics aggregation across multiple tracker instances. + +This issue tracks the expansion of **integration test coverage** (layer 2) for application-level +concerns that cannot be tested at the package level: + +- Multiple tracker instances running simultaneously +- Cross-service coordination and metrics aggregation +- Application container lifecycle and job orchestration +- Health check aggregation across all services +- Bootstrap and configuration integration +- Graceful shutdown coordination + +### Relationship to EPIC #1347 and Testing Layers + +This issue complements [EPIC #1347 - Increase unit testing for workspace +packages](https://github.com/torrust/torrust-tracker/issues/1347). + +| Layer | Location | Focus | EPIC/Issue | +| --------------- | ---------------------------------------------------------------- | ---------------------------------------- | ---------- | +| **Unit tests** | `packages/*/tests/` | Individual component behavior | EPIC #1347 | +| **Integration** | `tests/` (main app-level) | Application-level coordination | This issue | +| **E2E tests** | `packages/e2e-tools/`, `src/console/ci/e2e/`, `qbittorrent_e2e/` | Container-based cross-process validation | (separate) | + +All three layers are part of a broader effort to improve overall test coverage and reliability. + +## Scope + +### In Scope + +- Integration tests that require the full application context (`app::run()`) +- Tests that verify behavior across multiple coordinated services +- Tests that verify application container initialization and lifecycle +- Tests that verify job manager orchestration and background tasks +- Tests for global metrics, health checks, and cross-service coordination +- Tests that run in parallel without port conflicts (using port `0` and temp config) + +### Out of Scope + +- **Package-level unit tests** — belongs in `packages/*/tests/` (covered by EPIC #1347) +- **E2E tests using Docker Compose** — belongs in `packages/e2e-tools/`, `src/console/ci/e2e/`, + and `src/console/ci/qbittorrent_e2e/` (runs against containerized tracker with external clients) +- **Protocol parsing tests** — belongs in `packages/http-protocol/tests/` or + `packages/udp-protocol/tests/` +- **Single-service behavior tests** — belongs in corresponding server package tests +- **Database-only tests** — belongs in `packages/swarm-coordination-registry/tests/` + +**Guideline**: If a test can be written at the package level, it should be. Only add integration +tests when the full application context is genuinely required. If a test requires Docker Compose +orchestration or external BitTorrent clients, it belongs in the E2E layer. + +## Prioritized Test List + +### High Priority + +1. **Multiple trackers with different protocols** + Verify HTTP and UDP trackers run simultaneously, handle announces independently, and contribute + to separate metrics. + +2. **Health check aggregates all services** + Verify health check API returns status for all registered services (HTTP API, HTTP trackers, UDP + trackers). + +3. **Torrent cleanup job with active trackers** + Run the cleanup job while trackers are handling announces; verify it removes inactive peers + without interfering with active announces. + +4. **Global scrape across multiple trackers** + Send scrape requests to multiple HTTP tracker instances and verify the responses reflect the + correct swarm state. + +5. **Metrics counters across HTTP and UDP** + Verify that announce counters aggregate correctly when requests come to both HTTP and UDP + trackers. + +### Medium Priority + +1. **Graceful shutdown coordination** + Start all services, send requests, trigger shutdown, verify all services stop cleanly without + dropping active connections. + +2. **Job manager handles job failures** + Trigger a job failure; verify the job manager restarts or reports the failure without crashing + the application. + +3. **Concurrent announce load across multiple trackers** + Send simultaneous announces to multiple tracker instances; verify correct peer aggregation and + no race conditions. + +4. **Activity metrics updater job** + Verify the activity metrics updater job correctly processes peer activity and updates global + stats across all running services. + +5. **Event listener coordination** + Verify event listeners for different services process events without interference when multiple + services emit events simultaneously. + +### Low Priority + +1. **Container dependency validation** + Verify the application refuses to start with invalid service combinations or detects + configuration conflicts at bootstrap. + +2. **Application bootstrap with minimal configuration** + Start the application with minimal required config; verify all default services initialize + correctly. + +3. **Multiple database backends** + Start the application with SQLite, MySQL, and PostgreSQL configurations; verify the bootstrap + process correctly initializes each database backend and all services start without errors. + +4. **Service registration completeness** + Verify all configured services register correctly in the Registrar with their actual bound + addresses and metadata. + +## Implementation Plan + +This is a tracking issue. Each test case should be implemented as a subtask or separate small issue. + +Suggested approach: + +1. Start with high-priority tests (tests 1-5) +2. Implement one test per PR to keep changes reviewable +3. Follow the test pattern established in issue #1419 +4. Use test utilities from `tests/helpers.rs` (temp config, port extraction) +5. Ensure all tests use port `0` and temporary configuration files +6. Document test purpose with clear doc comments + +## Acceptance Criteria + +- [ ] AC1: All high-priority tests (tests 1-5) are implemented and passing +- [ ] AC2: Test utilities in `tests/helpers.rs` are expanded as needed for common patterns +- [ ] AC3: All new tests run in parallel without conflicts (port `0`, temp config) +- [ ] AC4: Each test has clear documentation explaining what application-level behavior is verified +- [ ] AC5: `linter all` passes +- [ ] AC6: All tests pass in CI + +## Verification Plan + +### Automatic Checks + +- `linter all` exits with code `0` +- `cargo test --test integration` passes all new integration tests +- `cargo test --workspace` passes (no regressions) +- CI pipeline passes with new tests running in parallel + +### Manual Checks + +| ID | Check | Expected Outcome | +| --- | ----------------------------------- | ------------------------------------------------------------------ | +| M1 | Run `cargo test --test integration` | All integration tests pass, no port conflicts or config collisions | +| M2 | Run with `RUST_LOG=debug` | Verify multiple services log startup without errors | +| M3 | Review test execution time | Integration tests complete faster than equivalent E2E tests | + +## Dependencies + +- Issue #1419 must be completed (infrastructure for parallel integration tests) + +## Related Issues + +- [Issue #1419 - Allow multiple integration tests at the main app + level](../open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure + foundation +- [EPIC #1347 - Increase unit testing for workspace + packages](https://github.com/torrust/torrust-tracker/issues/1347) - Package-level unit test + coverage + +## References + +### Integration Test Infrastructure + +- [tests/AGENTS.md](../../../tests/AGENTS.md) - Guidelines for main-level vs package-level tests +- [tests/integration.rs](../../../tests/integration.rs) - Integration test scaffolding +- [tests/servers/api/contract/stats/](../../../tests/servers/api/contract/stats/) - Current global + stats test example + +### Testing Strategy Documentation + +- [.github/skills/dev/testing/write-unit-test/SKILL.md](../../../.github/skills/dev/testing/write-unit-test/SKILL.md) + \- Unit testing conventions and Test Desiderata principles +- [docs/adrs/20260603000000_keep_unit_tests_inside_container_build.md](../../adrs/20260603000000_keep_unit_tests_inside_container_build.md) + \- ADR documenting the three-layer testing strategy (GHA unit tests, in-container unit tests, + E2E tests) +- [packages/e2e-tools/README.md](../../../packages/e2e-tools/README.md) - E2E test runners + (`e2e_tests_runner`, `qbittorrent_e2e_runner`) + +**Note**: There is currently no comprehensive testing strategy document in `docs/`. Testing +guidance is distributed across skills, ADRs, and package README files. A future improvement +could consolidate this into a canonical `docs/testing.md` document. + +## Progress Tracking + +### Completion Checklist + +High-priority tests: + +- [ ] Test 1: Multiple trackers with different protocols +- [ ] Test 2: Health check aggregates all services +- [ ] Test 3: Torrent cleanup job with active trackers +- [ ] Test 4: Global scrape across multiple trackers +- [ ] Test 5: Metrics counters across HTTP and UDP + +Medium-priority tests: + +- [ ] Test 6: Graceful shutdown coordination +- [ ] Test 7: Job manager handles job failures +- [ ] Test 8: Concurrent announce load across multiple trackers +- [ ] Test 9: Activity metrics updater job +- [ ] Test 10: Event listener coordination + +Low-priority tests tracked separately when high/medium priorities are complete. + +### Progress Log + +- 2026-07-27 12:00 UTC - agent - Created draft issue to track integration test coverage expansion + after #1419 infrastructure implementation. From 700e0aa7ca1763913ccc49f23ab0f2f19762f962 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 27 Jul 2026 13:06:18 +0100 Subject: [PATCH 3/9] docs(tests): add AI agent guidelines for integration tests Documents what belongs in main-level vs package-level tests, infrastructure requirements (port 0, temp workspaces), and references the draft issue for future test coverage expansion. --- tests/AGENTS.md | 102 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 tests/AGENTS.md diff --git a/tests/AGENTS.md b/tests/AGENTS.md new file mode 100644 index 000000000..8dc21479f --- /dev/null +++ b/tests/AGENTS.md @@ -0,0 +1,102 @@ +--- +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - tests/integration.rs + - tests/servers/ + - src/app.rs + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md + issue-spec: docs/issues/drafts/increase-main-app-integration-test-coverage.md +--- + +# Integration Tests — AI Agent Guidelines + +## Purpose + +This directory contains **main application-level integration tests**. These tests verify behavior +that can only be tested by running the complete Torrust Tracker application with multiple services +coordinated through the application container. + +## What Belongs Here + +Integration tests at this level should focus on **application-level concerns**: + +- **Multiple tracker instances**: Running HTTP and UDP trackers simultaneously on different ports +- **Global metrics aggregation**: Metrics that aggregate data across all running tracker instances +- **Application container lifecycle**: Container initialization, service registration, shutdown coordination +- **Job manager orchestration**: Background jobs interacting with multiple services +- **Cross-service coordination**: Interactions between HTTP API, trackers, and core services +- **Bootstrap and configuration**: Application startup with complex multi-service configurations +- **Health check aggregation**: Health status across all registered services + +## What Does NOT Belong Here + +Most tests should be in the corresponding `packages/*/tests/` directories: + +- **Single-service behavior**: Test HTTP tracker logic in `packages/axum-http-server/tests/` +- **Protocol parsing**: Test in `packages/http-protocol/tests/` or `packages/udp-protocol/tests/` +- **Core tracker logic**: Test in `packages/tracker-core/tests/` +- **Database operations**: Test in `packages/swarm-coordination-registry/tests/` +- **API endpoints**: Test in `packages/axum-rest-api-server/tests/` +- **Individual component behavior**: Always prefer package-level tests for isolated components + +**Guideline**: If the test can be written at the package level, it should be. Only use main-level +integration tests when you genuinely need the full application context. + +## Test Infrastructure Requirements + +All integration tests at this level must: + +1. **Use port `0` for all bind addresses**: The OS assigns free ephemeral ports, preventing conflicts + when tests run in parallel +2. **Use isolated temporary workspaces**: Never use `std::env::set_var()` for configuration — + tests run concurrently and would overwrite each other's config. Use `tempfile::TempDir` to create + isolated directories with separate config files and storage subdirectories +3. **Extract actual bound ports**: Query `AppContainer` or `Registar` to get the OS-assigned ports + for making requests +4. **Be independent**: Each test must be able to run in isolation or concurrently with others +5. **Clean up resources**: Use RAII patterns (temp dirs, handles) for automatic cleanup + +## Current Test Structure + +```text +tests/ +├── AGENTS.md # This file +├── integration.rs # Test scaffolding and module declarations +├── helpers.rs # Shared test utilities (temp config, port extraction) +└── servers/ + └── api/ + └── contract/ + └── stats/ + └── mod.rs # Global statistics tests +``` + +## Future Test Coverage + +For a prioritized list of valuable integration tests to add at the main application level, see the +[draft issue for increasing integration test +coverage](../docs/issues/drafts/increase-main-app-integration-test-coverage.md). + +This draft issue tracks 14 planned integration tests organized by priority (high/medium/low) and +complements [EPIC #1347](https://github.com/torrust/torrust-tracker/issues/1347) which focuses on +unit test coverage for workspace packages. + +## Adding a New Integration Test + +1. **Confirm it belongs here**: Can this test be written at the package level? If yes, write it there. +2. **Use test utilities**: Import helpers from `tests/helpers.rs` for temp config and port extraction +3. **Use port `0`**: All services must bind to port `0` in test configurations +4. **Extract bound ports**: Query `AppContainer.registar` or job handles to get actual socket addresses +5. **Make it independent**: Test must not depend on execution order or side effects from other tests +6. **Document the purpose**: Add a clear doc comment explaining what application-level behavior is + being tested + +For concrete examples, see the existing tests in `tests/servers/` — they serve as the canonical +reference for the integration test pattern. + +## References + +- [Issue #1419](../../docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure for parallel integration tests +- [Integration test scaffolding](integration.rs) +- [Test helpers](helpers.rs) From c5b2b89c2db2adc0c15aaf045127bf120f6e5ff1 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 27 Jul 2026 18:46:05 +0100 Subject: [PATCH 4/9] docs(issues): revise integration test execution model --- ...ple-integration-tests-at-main-app-level.md | 94 ++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md index 05c60c06d..69328caab 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md @@ -7,7 +7,7 @@ github-issue: 1419 spec-path: docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md branch: 1419-allow-multiple-integration-tests related-pr: null -last-updated-utc: 2026-07-27 08:00 +last-updated-utc: 2026-07-27 17:35 semantic-links: skill-links: - write-unit-test @@ -208,6 +208,9 @@ then fix the scaffolding infrastructure, then expand coverage. - 2026-07-27 08:38 UTC - agent - Updated implementation plan to use prove-then-fix strategy - 2026-07-27 13:00 UTC - agent - Updated Problem 3 and implementation plan to use temp directory pattern (not just temp files) for complete test isolation, matching qBittorrent E2E approach +- 2026-07-27 17:35 UTC - agent - Recorded the decision to use one tracker application per Cargo + integration-test executable, with sequential scenarios per suite and a non-Docker process runner + deferred unless in-process lifecycle control proves insufficient ## Acceptance Criteria @@ -266,3 +269,92 @@ then fix the scaffolding infrastructure, then expand coverage. - Consider whether test utilities should live in `tests/helpers.rs` or be added to the existing `packages/test-helpers/` package. Decision: prefer `tests/helpers.rs` to keep test-specific utilities close to the tests and avoid polluting the shared `test-helpers` package. + +## Decision Pivot: One Application per Integration-Test Binary + +**Decision date:** 2026-07-27 + +The preceding specification describes the initial proposal: multiple independent test functions in +`tests/integration.rs`, each bootstrapping a tracker application and running concurrently. That +proposal is superseded by this section. The historical content remains above to preserve the +reasoning and investigation that led to the decision. + +### Why the Initial Proposal Is Unsuitable + +Calling `app::run()` from an integration test starts the application inside the integration-test +executable, not in an independent tracker process. Application bootstrap initializes process-wide +state through `initialize_global_services`, including clock state, UDP cryptographic state, and +logging. The application also starts a collection of long-lived servers and background jobs. + +Consequently, a test function cannot safely own a fully isolated tracker lifecycle in a shared +test process. Temporary workspaces and port `0` solve filesystem and listener conflicts, but they +do not isolate process-global state, environment-variable configuration, or background task +lifecycle. Supporting multiple complete application instances per test executable would require a +larger application lifecycle redesign and is out of scope for this issue. + +### Chosen Execution Model + +Each top-level Rust source file in `tests/` is a separate Cargo integration-test executable and +therefore a separate operating-system process. The project will use one tracker configuration and +one tracker application instance per such executable. + +Within an integration-test executable, a single suite test will: + +1. Create an isolated temporary workspace containing the tracker configuration and storage. +2. Start one tracker application with the suite's fixed initial configuration. +3. Execute its scenario functions sequentially against that running application. +4. Shut down the application and wait for its jobs before releasing the temporary workspace. + +Scenario functions are not independently scheduled `#[tokio::test]` functions. They share the +suite's runtime and data lifecycle, so they must use distinct test data or make assertions that +explicitly account for accumulated state. + +The existing global-statistics scenarios belong in the same suite because they require the same +public-tracker configuration. A concern requiring a different initial configuration or process +lifecycle will be placed in another top-level file, such as `tests/bootstrap.rs`. Cargo may run +such executables concurrently; each suite must therefore still use a unique `TempDir` workspace, +its own database and storage paths, and port `0` for listeners. + +`TORRUST_TRACKER_CONFIG_TOML_PATH` remains process-local under this model, so configuration +injection through the environment is safe between separate test executables. It must not be +modified concurrently by separate scenarios in one executable. + +### Relationship to E2E Tests + +This is not a replacement for container E2E tests. Main-application integration suites provide +faster application-composition coverage without Docker, while E2E tests continue to validate +container images, mounts, network setup, and external-client workflows. + +### Deferred Alternative: Non-Docker Process Runner + +If an integration suite needs to verify the real tracker executable's startup, signal handling, +logging, or exit behavior, or if reliable in-process shutdown cannot be implemented, introduce a +non-Docker process runner. That runner would launch the built tracker binary as a child process +with an isolated workspace, wait for readiness, capture diagnostics, and terminate it cleanly. + +Do not create a new package solely to obtain separate test executables: Cargo already provides +that isolation through separate top-level files in `tests/`. A dedicated package becomes justified +only when the child-process runner is reusable enough to warrant its own lifecycle, readiness, +diagnostic, and cleanup abstractions. + +### Superseded Scope, Plan, Acceptance Criteria, and Verification + +The original scope, implementation plan, acceptance criteria, and verification plan above are +superseded where they require parallel tracker application instances or multiple independently +bootstrapped test functions in `tests/integration.rs`. + +This issue now covers the following: + +- Convert the existing global-statistics integration test into one sequential suite using one + public-tracker application instance. +- Provide test-local helpers for an isolated temporary workspace, tracker startup, readiness, and + deterministic shutdown. +- Use port `0` and discover resolved listener addresses for suite requests. +- Add further global-statistics scenarios only when they share the suite's initial configuration. +- Establish the convention that a different initial tracker configuration belongs to another + top-level integration-test executable. + +Verification must show that the suite starts one application, runs all its scenarios sequentially, +shuts it down cleanly, and leaves no shared database, storage, or port dependency. Cross-suite +parallel execution is supported through process isolation, but it is not a requirement for +parallel full-application instances inside a single executable. From 8e42f8e96dd20f583145fdcd3d558f85783cd21a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 28 Jul 2026 09:38:18 +0100 Subject: [PATCH 5/9] feat(tests): add parallel integration test infrastructure - Replace tests/integration.rs with tests/stats.rs (rename) - Create tests/common/mod.rs with shared helpers (EphemeralTrackerWorkspace, port discovery via registar IP-based filtering) - Create tests/scaffold.rs as a demo binary for future contributors - Convert stats tests to single-suite with cross-instance aggregation focus - Update tests/AGENTS.md with the new execution model - Remove unused dev-dependencies (torrust-info-hash, torrust-tracker-client, torrust-tracker-http-protocol) - Remove obsolete tests/helpers.rs --- Cargo.lock | 4 +- Cargo.toml | 4 +- ...ease-main-app-integration-test-coverage.md | 16 +- ...ple-integration-tests-at-main-app-level.md | 30 ++-- tests/AGENTS.md | 77 ++++++---- tests/common/mod.rs | 137 +++++++++++++++++ tests/scaffold.rs | 145 ++++++++++++++++++ tests/servers/api/contract/stats/mod.rs | 103 ++++++------- tests/{integration.rs => stats.rs} | 3 +- 9 files changed, 407 insertions(+), 112 deletions(-) create mode 100644 tests/common/mod.rs create mode 100644 tests/scaffold.rs rename tests/{integration.rs => stats.rs} (93%) diff --git a/Cargo.lock b/Cargo.lock index 81a66b83c..53bb91460 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4821,17 +4821,15 @@ dependencies = [ "tokio-util", "toml 1.1.3+spec-1.1.0", "torrust-clock", - "torrust-info-hash", + "torrust-net-primitives", "torrust-server-lib", "torrust-tracker-axum-health-check-api-server", "torrust-tracker-axum-http-server", "torrust-tracker-axum-rest-api-server", "torrust-tracker-axum-server", - "torrust-tracker-client-lib", "torrust-tracker-configuration", "torrust-tracker-core", "torrust-tracker-http-core", - "torrust-tracker-http-protocol", "torrust-tracker-primitives", "torrust-tracker-rest-api-client", "torrust-tracker-rest-api-protocol", diff --git a/Cargo.toml b/Cargo.toml index c9252ef8d..d424d82e2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,9 +73,7 @@ tracing = "0" tracing-subscriber = { version = "0", features = [ "json" ] } [dev-dependencies] -torrust-info-hash = "=0.2.0" -torrust-tracker-client = { package = "torrust-tracker-client-lib", version = "0.1.0", path = "packages/tracker-client" } -torrust-tracker-http-protocol = { version = "0.1.0", path = "packages/http-protocol" } +torrust-net-primitives = "0.1.0" torrust-tracker-test-helpers = { version = "3.0.0", path = "packages/test-helpers" } [workspace] diff --git a/docs/issues/drafts/increase-main-app-integration-test-coverage.md b/docs/issues/drafts/increase-main-app-integration-test-coverage.md index a199f8773..379953f9f 100644 --- a/docs/issues/drafts/increase-main-app-integration-test-coverage.md +++ b/docs/issues/drafts/increase-main-app-integration-test-coverage.md @@ -12,7 +12,7 @@ semantic-links: skill-links: - write-unit-test related-artifacts: - - tests/integration.rs + - tests/stats.rs - tests/AGENTS.md - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md related-issues: @@ -181,17 +181,17 @@ Suggested approach: ### Automatic Checks - `linter all` exits with code `0` -- `cargo test --test integration` passes all new integration tests +- `cargo test --test stats` passes all new integration tests - `cargo test --workspace` passes (no regressions) - CI pipeline passes with new tests running in parallel ### Manual Checks -| ID | Check | Expected Outcome | -| --- | ----------------------------------- | ------------------------------------------------------------------ | -| M1 | Run `cargo test --test integration` | All integration tests pass, no port conflicts or config collisions | -| M2 | Run with `RUST_LOG=debug` | Verify multiple services log startup without errors | -| M3 | Review test execution time | Integration tests complete faster than equivalent E2E tests | +| ID | Check | Expected Outcome | +| --- | ----------------------------- | ------------------------------------------------------------------ | +| M1 | Run `cargo test --test stats` | All integration tests pass, no port conflicts or config collisions | +| M2 | Run with `RUST_LOG=debug` | Verify multiple services log startup without errors | +| M3 | Review test execution time | Integration tests complete faster than equivalent E2E tests | ## Dependencies @@ -211,7 +211,7 @@ Suggested approach: ### Integration Test Infrastructure - [tests/AGENTS.md](../../../tests/AGENTS.md) - Guidelines for main-level vs package-level tests -- [tests/integration.rs](../../../tests/integration.rs) - Integration test scaffolding +- [tests/stats.rs](../../../tests/stats.rs) - Integration test scaffolding - [tests/servers/api/contract/stats/](../../../tests/servers/api/contract/stats/) - Current global stats test example diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md index 69328caab..b8ca6f43d 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md @@ -12,7 +12,7 @@ semantic-links: skill-links: - write-unit-test related-artifacts: - - tests/integration.rs + - tests/stats.rs - tests/servers/ - src/app.rs - packages/test-helpers/ @@ -22,7 +22,7 @@ semantic-links: ## Goal -Enable running multiple independent integration tests at the main application level (`tests/integration.rs`) +Enable running multiple independent integration tests at the main application level (`tests/stats.rs`) in parallel without port conflicts, configuration collisions, or logging initialization errors. ## Background @@ -186,7 +186,7 @@ then fix the scaffolding infrastructure, then expand coverage. | T5 | TODO | Add utility to extract bound addresses from `AppContainer` | Query `Registar` or job handles to get actual bound `SocketAddr` for HTTP API, trackers, etc. | | T6 | TODO | Update existing stats test to use port 0 and temp workspace | Replace `env::set_var` with isolated temp workspace, use port 0, extract actual ports for requests; fixes scaffolding to support parallelism | | T7 | TODO | Expand global stats test coverage | Add tests for more global stat metrics now that scaffolding supports it (scrape counters, different IP families, etc.) | -| T8 | TODO | Run automatic verification | `cargo test --test integration` must pass with all tests running concurrently | +| T8 | TODO | Run automatic verification | `cargo test --test stats` must pass with all tests running concurrently | ## Progress Tracking @@ -195,7 +195,7 @@ then fix the scaffolding infrastructure, then expand coverage. - [ ] Specification drafted and approved by user/maintainer - [ ] GitHub issue #1419 already exists (created by maintainer) - [ ] Implementation completed -- [ ] Automatic verification completed (`cargo test --test integration`) +- [ ] Automatic verification completed (`cargo test --test stats`) - [ ] Acceptance criteria reviewed after implementation - [ ] Issue closed and specification moved to `docs/issues/closed/` @@ -216,7 +216,7 @@ then fix the scaffolding infrastructure, then expand coverage. - [ ] AC1: `tests/AGENTS.md` exists and documents guidelines for what belongs at main-level vs package-level, with a TODO list of future valuable integration tests. -- [ ] AC2: Multiple integration tests can run concurrently with `cargo test --test integration` +- [ ] AC2: Multiple integration tests can run concurrently with `cargo test --test stats` without port conflicts. - [ ] AC3: Each test uses an isolated temporary workspace with separate config and storage directories (no shared environment variables or storage paths). @@ -227,26 +227,26 @@ then fix the scaffolding infrastructure, then expand coverage. - [ ] AC6: Global stats test coverage includes multiple metrics (not just `tcp4_announces_handled`). - [ ] AC7: Test utilities for temp workspace creation (config + storage) and port extraction are available and documented. -- [ ] AC8: `cargo test --test integration` passes cleanly with expanded test coverage. +- [ ] AC8: `cargo test --test stats` passes cleanly with expanded test coverage. - [ ] AC9: `linter all` passes. ## Verification Plan ### Automatic Checks -- `cargo test --test integration` — Must pass with all integration tests running concurrently -- `cargo test --test integration -- --test-threads=1` — Verify tests also work in serial mode +- `cargo test --test stats` — Must pass with all integration tests running concurrently +- `cargo test --test stats -- --test-threads=1` — Verify tests also work in serial mode - `linter all` — Standard quality gate ### Manual Verification Scenarios -| ID | Scenario | Expected Result | Status | Evidence | -| --- | ----------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------ | -------- | -| M1 | Run `cargo test --test integration` with default parallelism | All tests pass; no port conflicts or configuration collisions | TODO | | -| M2 | Run `cargo test --test integration -- --nocapture` to see logs | Each test shows unique bound ports; no env var configuration warnings | TODO | | -| M3 | Add a third integration test temporarily and run the suite | All three tests run concurrently without interference | TODO | | -| M4 | Verify temp workspaces are cleaned up after tests complete | No leftover temporary directories in `/tmp` or system temp directory | TODO | | -| M5 | Run tests in serial mode: `cargo test --test integration -- --test-threads=1` | Tests pass in serial mode (no implicit dependency on parallelism for correctness) | TODO | | +| ID | Scenario | Expected Result | Status | Evidence | +| --- | ----------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------ | -------- | +| M1 | Run `cargo test --test stats` with default parallelism | All tests pass; no port conflicts or configuration collisions | TODO | | +| M2 | Run `cargo test --test stats -- --nocapture` to see logs | Each test shows unique bound ports; no env var configuration warnings | TODO | | +| M3 | Add a third integration test temporarily and run the suite | All three tests run concurrently without interference | TODO | | +| M4 | Verify temp workspaces are cleaned up after tests complete | No leftover temporary directories in `/tmp` or system temp directory | TODO | | +| M5 | Run tests in serial mode: `cargo test --test stats -- --test-threads=1` | Tests pass in serial mode (no implicit dependency on parallelism for correctness) | TODO | | ## Risks and Trade-offs diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 8dc21479f..d14632ef0 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -3,7 +3,7 @@ semantic-links: skill-links: - write-unit-test related-artifacts: - - tests/integration.rs + - tests/stats.rs - tests/servers/ - src/app.rs - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md @@ -44,18 +44,37 @@ Most tests should be in the corresponding `packages/*/tests/` directories: **Guideline**: If the test can be written at the package level, it should be. Only use main-level integration tests when you genuinely need the full application context. +## Execution Model + +Each top-level Rust source file in `tests/` is a **separate Cargo integration-test +executable** (and therefore a separate operating-system process). A single test +executable manages **one tracker application instance** with a fixed initial +configuration. Scenario functions run sequentially against that instance. + +A different initial configuration requires a separate top-level file. For example: + +| File | Purpose | +| -------------------- | -------------------------------------------------------- | +| `tests/stats.rs` | Global statistics suite (public tracker, two HTTP nodes) | +| `tests/scaffold.rs` | Scaffolding demo — same pattern, isolated process | +| `tests/bootstrap.rs` | _(future)_ Bootstrap/shutdown lifecycle scenarios | + +Cargo may run these binaries in parallel. Each binary binds to port `0` (OS-assigned +ephemeral ports), uses its own `TempDir` workspace, and sets +`TORRUST_TRACKER_CONFIG_TOML_PATH` only in its own process, so no conflict occurs. + ## Test Infrastructure Requirements All integration tests at this level must: -1. **Use port `0` for all bind addresses**: The OS assigns free ephemeral ports, preventing conflicts - when tests run in parallel -2. **Use isolated temporary workspaces**: Never use `std::env::set_var()` for configuration — - tests run concurrently and would overwrite each other's config. Use `tempfile::TempDir` to create +1. **Use port `0` for all bind addresses**: The OS assigns free ephemeral ports, preventing + conflicts when tests run in parallel +2. **Use isolated temporary workspaces**: Use `tempfile::TempDir` to create isolated directories with separate config files and storage subdirectories -3. **Extract actual bound ports**: Query `AppContainer` or `Registar` to get the OS-assigned ports +3. **Extract actual bound ports**: Query `AppContainer`'s `Registar` to get the OS-assigned ports for making requests -4. **Be independent**: Each test must be able to run in isolation or concurrently with others +4. **Be independent**: Each top-level test binary must be able to run in isolation or concurrently + with others (it is the binary, not the function, that is the unit of isolation) 5. **Clean up resources**: Use RAII patterns (temp dirs, handles) for automatic cleanup ## Current Test Structure @@ -63,40 +82,38 @@ All integration tests at this level must: ```text tests/ ├── AGENTS.md # This file -├── integration.rs # Test scaffolding and module declarations -├── helpers.rs # Shared test utilities (temp config, port extraction) +├── common/ +│ └── mod.rs # Shared test utilities (temp config, port extraction) +├── integration.rs # Global statistics suite (main integration tests) +├── scaffold.rs # Scaffolding demo — pattern reference for new binaries └── servers/ └── api/ └── contract/ └── stats/ - └── mod.rs # Global statistics tests + └── mod.rs # Global statistics test scenarios ``` -## Future Test Coverage - -For a prioritized list of valuable integration tests to add at the main application level, see the -[draft issue for increasing integration test -coverage](../docs/issues/drafts/increase-main-app-integration-test-coverage.md). - -This draft issue tracks 14 planned integration tests organized by priority (high/medium/low) and -complements [EPIC #1347](https://github.com/torrust/torrust-tracker/issues/1347) which focuses on -unit test coverage for workspace packages. - -## Adding a New Integration Test +## Adding a New Integration-Test Binary 1. **Confirm it belongs here**: Can this test be written at the package level? If yes, write it there. -2. **Use test utilities**: Import helpers from `tests/helpers.rs` for temp config and port extraction -3. **Use port `0`**: All services must bind to port `0` in test configurations -4. **Extract bound ports**: Query `AppContainer.registar` or job handles to get actual socket addresses -5. **Make it independent**: Test must not depend on execution order or side effects from other tests -6. **Document the purpose**: Add a clear doc comment explaining what application-level behavior is - being tested +2. **Determine the initial configuration**: If your scenarios need a different tracker + configuration than the existing suite, create a new top-level file (e.g., `tests/bootstrap.rs`). + If they share the same configuration, add scenarios to the existing suite. +3. **Reuse shared utilities**: Import `mod common;` and use the helpers in `tests/common/mod.rs` + for workspace setup, tracker startup, and port discovery. +4. **Use port `0`**: All services must bind to port `0` in test configurations. +5. **Extract bound ports**: Query the registar or `AppContainer` to discover actual socket addresses. +6. **Document the purpose**: Add clear doc comments explaining what application-level behavior is + being tested. +7. **Reference existing code**: See `tests/scaffold.rs` for a minimal working example of a new + integration-test binary, or `tests/servers/api/contract/stats/mod.rs` for the scenario pattern. For concrete examples, see the existing tests in `tests/servers/` — they serve as the canonical reference for the integration test pattern. ## References -- [Issue #1419](../../docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure for parallel integration tests -- [Integration test scaffolding](integration.rs) -- [Test helpers](helpers.rs) +- [Issue #1419](../../docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure for parallel integration tests (execution model decision) +- [Integration test scaffolding](stats.rs) +- [Shared test utilities](common/mod.rs) +- [Scaffolding demo](scaffold.rs) diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 000000000..f0087b9a5 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,137 @@ +//! Shared test utilities for integration tests. +//! +//! This module is shared across multiple integration-test binaries via +//! `mod common;`. Each top-level file under `tests/` is a separate Cargo +//! integration-test executable. Common helpers belong here rather than in +//! a top-level file, so all test binaries can reach them. +//! +//! # Architecture +//! +//! Each integration-test binary manages **one** tracker application instance +//! with a fixed initial configuration. Scenario functions run sequentially +//! against that instance. A different initial configuration belongs to +//! another top-level binary, which Cargo may run concurrently. +//! +//! See `docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md` +//! for the full decision record. +use std::net::SocketAddr; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use tempfile::TempDir; +use torrust_tracker_lib::app; +use torrust_tracker_lib::bootstrap::jobs::manager::JobManager; +use torrust_tracker_lib::container::AppContainer; + +/// A temporary workspace for an integration test. +/// +/// Creates an isolated directory with config file and storage directory. +/// The `{STORAGE_PATH}` placeholder in the config TOML is replaced with +/// the absolute path to the temp storage directory. +pub struct EphemeralTrackerWorkspace { + _temp_dir: TempDir, + config_path: PathBuf, +} + +impl EphemeralTrackerWorkspace { + #[must_use] + pub fn new(config_toml: &str) -> Self { + let temp_dir = TempDir::new().expect("failed to create temp dir"); + let storage_path = temp_dir.path().join("tracker-storage"); + std::fs::create_dir_all(&storage_path).expect("failed to create storage dir"); + + let config_path = temp_dir.path().join("tracker-config.toml"); + let resolved = config_toml.replace("{STORAGE_PATH}", &storage_path.to_string_lossy()); + std::fs::write(&config_path, resolved).expect("failed to write config file"); + + Self { + _temp_dir: temp_dir, + config_path, + } + } + + #[must_use] + pub fn config_path(&self) -> &Path { + &self.config_path + } +} + +/// Starts the tracker application with the given workspace config. +/// +/// Since the application reads its configuration from the +/// `TORRUST_TRACKER_CONFIG_TOML_PATH` environment variable, +/// tests in this binary must not run concurrently with other tests +/// that modify the same variable. +/// +/// A short delay is added after startup to allow services to register +/// in the registar and bind to OS-assigned ports. +pub async fn start_tracker_with_config(workspace: &EphemeralTrackerWorkspace) -> (Arc, JobManager) { + // SAFETY: This binary must be the only test executable setting + // `TORRUST_TRACKER_CONFIG_TOML_PATH`. Cargo may run different + // integration-test binaries in parallel, but each binary is a + // separate OS process with its own environment. + #[allow(unsafe_code)] + unsafe { + std::env::set_var( + "TORRUST_TRACKER_CONFIG_TOML_PATH", + workspace.config_path().to_str().expect("config path must be valid UTF-8"), + ); + } + let (container, jobs) = app::run().await; + // Allow spawned tasks (registar insertions, listener binds) to + // complete before we attempt to read the registar or make requests. + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + (container, jobs) +} + +/// Returns the HTTP tracker URLs from the registar. +/// +/// HTTP trackers bind to `0.0.0.0` (unspecified). The REST API and health +/// check bind to `127.0.0.1` (loopback). We identify trackers by their +/// unspecified IP, which is deterministic regardless of hash-map ordering. +/// Wildcard addresses are converted to `127.0.0.1` for client requests. +pub async fn http_tracker_urls(container: &AppContainer) -> Vec { + let reg = container.registar.entries(); + let map = reg.lock().await; + map.keys() + .filter(|b| { + b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_unspecified() + }) + .map(|b| loopback_url(b.bind_address())) + .collect() +} + +/// Returns the HTTP API URL from the registar. +/// +/// The REST API binds to `127.0.0.1` (loopback), unlike the HTTP trackers +/// which bind to `0.0.0.0`. +pub async fn http_api_url(container: &AppContainer) -> Option { + let reg = container.registar.entries(); + let map = reg.lock().await; + map.keys() + .find(|b| b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_loopback()) + .map(|b| format!("http://{}", b.bind_address())) +} + +/// Returns all registered service bindings for diagnostic purposes. +#[allow(dead_code)] +pub async fn all_service_bindings(container: &AppContainer) -> Vec<(String, String)> { + let reg = container.registar.entries(); + let map = reg.lock().await; + map.keys() + .map(|b| (b.protocol().to_string(), loopback_url(b.bind_address()))) + .collect() +} + +/// Convert a socket address to a connectable loopback URL. +/// +/// Tracker services bind to `0.0.0.0` (all interfaces), but clients must +/// connect to a reachable address. This replaces wildcard IPv4 with the +/// loopback address `127.0.0.1`, preserving the OS-assigned port. +fn loopback_url(addr: SocketAddr) -> String { + if addr.ip().is_unspecified() { + format!("http://127.0.0.1:{port}", port = addr.port()) + } else { + format!("http://{addr}") + } +} diff --git a/tests/scaffold.rs b/tests/scaffold.rs new file mode 100644 index 000000000..6ca27d7b9 --- /dev/null +++ b/tests/scaffold.rs @@ -0,0 +1,145 @@ +//! Scaffolding integration test — demo and sample. +//! +//! This file is a **scaffolding sample** that demonstrates the integration-test +//! pattern adopted by this project. It is not intended to provide unique test +//! coverage. Instead, its purpose is to: +//! +//! - Verify that multiple top-level integration-test binaries can run +//! concurrently without port or configuration conflicts. +//! - Show future contributors how to add a new integration-test binary for +//! a different tracker configuration or lifecycle scenario. +//! +//! # Architecture +//! +//! Each top-level `tests/*.rs` file is a **separate OS process** (Cargo +//! integration-test binary). A binary runs **one tracker application +//! instance** with a fixed initial configuration. Scenario functions run +//! sequentially against that instance. +//! +//! A different initial configuration belongs in another binary. +//! For example, `tests/bootstrap.rs` would exercise the startup/shutdown +//! lifecycle, while `tests/stats.rs` exercises the global statistics API +//! under one configuration. +//! +//! ## Shared Helpers +//! +//! Common utilities live in [`tests/common/`](../common/index.html). +//! Import with `mod common;`. +//! +//! ## Requirements +//! +//! - Port `0` for all service bind addresses. +//! - Isolated temporary workspace per suite (`EphemeralTrackerWorkspace`). +//! - A small startup delay to allow async service registration. +//! - Sequential scenarios that account for accumulated state. +//! +//! # Example: Running this test +//! +//! ```text +//! cargo test --test scaffold +//! ``` +//! +//! Both `stats` and `scaffold` binaries can run in parallel: +//! +//! ```text +//! cargo test --test stats --test scaffold +//! ``` +mod common; + +use serde::Deserialize; +use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; +use torrust_tracker_rest_api_client::v1::client::ApiHttpClient as TrackerApiClient; + +use crate::common::EphemeralTrackerWorkspace; + +/// Demo: the stats API should aggregate announces across multiple trackers. +/// +/// This is a scaffolding sample that reproduces the global-stats scenario +/// to demonstrate that a second integration-test binary can boot its own +/// tracker application without conflicting with the main suite. +#[tokio::test] +async fn the_stats_api_endpoint_should_aggregate_announces_across_multiple_trackers() { + // ── 1. Configuration ────────────────────────────────────────────── + let config_toml = r#" + [metadata] + app = "torrust-tracker" + purpose = "configuration" + schema_version = "2.0.0" + + [logging] + threshold = "off" + + [core] + listed = false + private = false + + [core.database] + driver = "sqlite3" + path = "{STORAGE_PATH}/sqlite3.db" + + [[http_trackers]] + bind_address = "0.0.0.0:0" + tracker_usage_statistics = true + + [[http_trackers]] + bind_address = "0.0.0.0:0" + tracker_usage_statistics = true + + [http_api] + bind_address = "127.0.0.1:0" + + [http_api.access_tokens] + admin = "MyAccessToken" + + [health_check_api] + bind_address = "127.0.0.2:0" + "#; + + // ── 2. Start tracker on isolated workspace ─────────────────────── + let workspace = EphemeralTrackerWorkspace::new(config_toml); + let (app_container, _jobs) = common::start_tracker_with_config(&workspace).await; + + // ── 3. Discover bound addresses ────────────────────────────────── + let tracker_urls = common::http_tracker_urls(&app_container).await; + assert_eq!(tracker_urls.len(), 2, "expected two HTTP trackers"); + + let api_url = common::http_api_url(&app_container).await.expect("expected an HTTP API URL"); + + // ── 4. Scenario: announce to both trackers ─────────────────────── + let client = reqwest::Client::new(); + for url in &tracker_urls { + let announce_url = format!( + "{}/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0", + url.trim_end_matches('/') + ); + let resp = client.get(&announce_url).send().await.unwrap(); + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + panic!("announce to {url} failed: status {status}, body: {body}"); + } + } + + // ── 5. Scenario: verify global stats ───────────────────────────── + let stats = get_stats(&api_url, "MyAccessToken").await; + assert_eq!(stats.tcp4_announces_handled, 2, "two announces should be aggregated"); + + // The tracker application and its temporary workspace are cleaned up + // when `workspace` and `_jobs` are dropped at the end of this scope. +} + +/// Statistics subset relevant to this demo. +#[derive(Deserialize)] +struct DemoStats { + tcp4_announces_handled: u64, +} + +async fn get_stats(api_url: &str, token: &str) -> DemoStats { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url).unwrap(), token)) + .unwrap() + .get_tracker_statistics(None) + .await + .expect("failed to get tracker statistics"); + + response.json::().await.expect("failed to parse JSON response") +} diff --git a/tests/servers/api/contract/stats/mod.rs b/tests/servers/api/contract/stats/mod.rs index 202cf31f0..506247a63 100644 --- a/tests/servers/api/contract/stats/mod.rs +++ b/tests/servers/api/contract/stats/mod.rs @@ -1,23 +1,23 @@ -use std::env; -use std::str::FromStr as _; - -use reqwest::Url; use serde::Deserialize; -use tokio::time::Duration; -use torrust_info_hash::InfoHash; -use torrust_tracker_client::http::client::Client as HttpTrackerClient; -use torrust_tracker_http_protocol::v1::requests::announce::AnnounceBuilder; -use torrust_tracker_lib::app; use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; use torrust_tracker_rest_api_client::v1::client::ApiHttpClient as TrackerApiClient; +use crate::common::{self, EphemeralTrackerWorkspace}; + +/// The stats API endpoint should aggregate announces across multiple HTTP tracker instances. +/// +/// This is an application-level integration test. It verifies that announces +/// sent to two separate HTTP tracker instances are both counted in the global +/// tracker statistics. This behavior cannot be tested at the package level +/// because it requires the full application container coordinating multiple +/// HTTP tracker instances. +/// +/// Single-instance announce and scrape behavior is tested in the +/// `axum-http-server` package. #[tokio::test] async fn the_stats_api_endpoint_should_return_the_global_stats() { - // Logging must be OFF otherwise your will get the following error: - // `Unable to install global subscriber: SetGlobalDefaultError("a global default trace dispatcher has already been set")` - // That's because we can't initialize the logger twice. - // You can enable it if you run only this test. - let config_with_two_http_trackers = r#" + // ── 1. Configuration ────────────────────────────────────────────── + let config_toml = r#" [metadata] app = "torrust-tracker" purpose = "configuration" @@ -32,57 +32,56 @@ async fn the_stats_api_endpoint_should_return_the_global_stats() { [core.database] driver = "sqlite3" - path = "./integration_tests_sqlite3.db" + path = "{STORAGE_PATH}/sqlite3.db" [[http_trackers]] - bind_address = "0.0.0.0:7272" + bind_address = "0.0.0.0:0" tracker_usage_statistics = true [[http_trackers]] - bind_address = "0.0.0.0:7373" + bind_address = "0.0.0.0:0" tracker_usage_statistics = true [http_api] - bind_address = "0.0.0.0:1414" + bind_address = "127.0.0.1:0" [http_api.access_tokens] admin = "MyAccessToken" - "#; - - // SAFETY: `std::env::set_var` is unsafe in Rust 2024 because concurrent reads from - // other threads in the same process are undefined behaviour. This test is the only - // function in this integration binary that writes `TORRUST_TRACKER_CONFIG_TOML`, and - // each test in this file binds to unique fixed ports, making parallel execution - // impossible (port conflicts). In practice the tests therefore run serially, but the - // safety guarantee is not formally enforced by the test runner. For strict soundness, - // run the integration suite with `RUST_TEST_THREADS=1`. - #[allow(unsafe_code)] - unsafe { - env::set_var("TORRUST_TRACKER_CONFIG_TOML", config_with_two_http_trackers); - } - - let (_app_container, _jobs) = app::run().await; - - announce_to_tracker("http://127.0.0.1:7272").await; - announce_to_tracker("http://127.0.0.1:7373").await; - let global_stats = get_tracker_statistics("http://127.0.0.1:1414", "MyAccessToken").await; + [health_check_api] + bind_address = "127.0.0.2:0" + "#; + + // ── 2. Start tracker on isolated workspace ─────────────────────── + let workspace = EphemeralTrackerWorkspace::new(config_toml); + let (app_container, _jobs) = common::start_tracker_with_config(&workspace).await; + + let tracker_urls = common::http_tracker_urls(&app_container).await; + assert_eq!(tracker_urls.len(), 2, "expected two HTTP trackers"); + + let api_url = common::http_api_url(&app_container).await.expect("expected an HTTP API URL"); + + // ── 3. Announce to both tracker instances ──────────────────────── + let client = reqwest::Client::new(); + for url in &tracker_urls { + let announce_url = format!( + "{}/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0", + url.trim_end_matches('/') + ); + let resp = client.get(&announce_url).send().await.unwrap(); + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + panic!("announce to {url} failed: status {status}, body: {body}"); + } + } + // ── 4. Verify both announces are aggregated ────────────────────── + let global_stats = get_tracker_statistics(&api_url, "MyAccessToken").await; assert_eq!(global_stats.tcp4_announces_handled, 2); -} -/// Make a sample announce request to the tracker. -async fn announce_to_tracker(tracker_url: &str) { - let response = HttpTrackerClient::new(Url::parse(tracker_url).unwrap(), Duration::from_secs(1)) - .unwrap() - .announce( - &AnnounceBuilder::with_default_values() - .with_info_hash(&InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap()) // DevSkim: ignore DS173237 - .query(), - ) - .await; - - assert!(response.is_ok()); + // The tracker application and its temporary workspace are cleaned up + // when `workspace` and `_jobs` are dropped at the end of this scope. } /// Global statistics with only metrics relevant to the test. @@ -91,8 +90,8 @@ struct PartialGlobalStatistics { tcp4_announces_handled: u64, } -async fn get_tracker_statistics(aip_url: &str, token: &str) -> PartialGlobalStatistics { - let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(aip_url).unwrap(), token)) +async fn get_tracker_statistics(api_url: &str, token: &str) -> PartialGlobalStatistics { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url).unwrap(), token)) .unwrap() .get_tracker_statistics(None) .await diff --git a/tests/integration.rs b/tests/stats.rs similarity index 93% rename from tests/integration.rs rename to tests/stats.rs index c0af43b87..4329f9929 100644 --- a/tests/integration.rs +++ b/tests/stats.rs @@ -5,8 +5,9 @@ //! the corresponding package. //! //! ```text -//! cargo test --test integration +//! cargo test --test stats //! ``` +mod common; mod servers; use torrust_clock::clock; From 935ceb98e441dd3f4e97b136a69b922816562696 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 28 Jul 2026 10:37:40 +0100 Subject: [PATCH 6/9] refactor(tests): use Url type instead of String for URL values --- Cargo.lock | 1 + Cargo.toml | 1 + tests/common/mod.rs | 16 +++++++++------- tests/scaffold.rs | 14 +++++++------- tests/servers/api/contract/stats/mod.rs | 14 +++++++------- 5 files changed, 25 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 53bb91460..e49aac631 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4840,6 +4840,7 @@ dependencies = [ "torrust-tracker-udp-server", "tracing", "tracing-subscriber", + "url", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d424d82e2..ada409b88 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,6 +75,7 @@ tracing-subscriber = { version = "0", features = [ "json" ] } [dev-dependencies] torrust-net-primitives = "0.1.0" torrust-tracker-test-helpers = { version = "3.0.0", path = "packages/test-helpers" } +url = { version = "2", features = [ "serde" ] } [workspace] members = [ diff --git a/tests/common/mod.rs b/tests/common/mod.rs index f0087b9a5..a498a2cd5 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -22,6 +22,7 @@ use tempfile::TempDir; use torrust_tracker_lib::app; use torrust_tracker_lib::bootstrap::jobs::manager::JobManager; use torrust_tracker_lib::container::AppContainer; +use url::Url; /// A temporary workspace for an integration test. /// @@ -90,7 +91,7 @@ pub async fn start_tracker_with_config(workspace: &EphemeralTrackerWorkspace) -> /// check bind to `127.0.0.1` (loopback). We identify trackers by their /// unspecified IP, which is deterministic regardless of hash-map ordering. /// Wildcard addresses are converted to `127.0.0.1` for client requests. -pub async fn http_tracker_urls(container: &AppContainer) -> Vec { +pub async fn http_tracker_urls(container: &AppContainer) -> Vec { let reg = container.registar.entries(); let map = reg.lock().await; map.keys() @@ -105,17 +106,17 @@ pub async fn http_tracker_urls(container: &AppContainer) -> Vec { /// /// The REST API binds to `127.0.0.1` (loopback), unlike the HTTP trackers /// which bind to `0.0.0.0`. -pub async fn http_api_url(container: &AppContainer) -> Option { +pub async fn http_api_url(container: &AppContainer) -> Option { let reg = container.registar.entries(); let map = reg.lock().await; map.keys() .find(|b| b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_loopback()) - .map(|b| format!("http://{}", b.bind_address())) + .map(|b| loopback_url(b.bind_address())) } /// Returns all registered service bindings for diagnostic purposes. #[allow(dead_code)] -pub async fn all_service_bindings(container: &AppContainer) -> Vec<(String, String)> { +pub async fn all_service_bindings(container: &AppContainer) -> Vec<(String, Url)> { let reg = container.registar.entries(); let map = reg.lock().await; map.keys() @@ -128,10 +129,11 @@ pub async fn all_service_bindings(container: &AppContainer) -> Vec<(String, Stri /// Tracker services bind to `0.0.0.0` (all interfaces), but clients must /// connect to a reachable address. This replaces wildcard IPv4 with the /// loopback address `127.0.0.1`, preserving the OS-assigned port. -fn loopback_url(addr: SocketAddr) -> String { +fn loopback_url(addr: SocketAddr) -> Url { if addr.ip().is_unspecified() { - format!("http://127.0.0.1:{port}", port = addr.port()) + Url::parse(&format!("http://127.0.0.1:{port}", port = addr.port())) } else { - format!("http://{addr}") + Url::parse(&format!("http://{addr}")) } + .expect("loopback URL should always be valid") } diff --git a/tests/scaffold.rs b/tests/scaffold.rs index 6ca27d7b9..4b80a6caf 100644 --- a/tests/scaffold.rs +++ b/tests/scaffold.rs @@ -49,6 +49,7 @@ mod common; use serde::Deserialize; use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; use torrust_tracker_rest_api_client::v1::client::ApiHttpClient as TrackerApiClient; +use url::Url; use crate::common::EphemeralTrackerWorkspace; @@ -108,11 +109,10 @@ async fn the_stats_api_endpoint_should_aggregate_announces_across_multiple_track // ── 4. Scenario: announce to both trackers ─────────────────────── let client = reqwest::Client::new(); for url in &tracker_urls { - let announce_url = format!( - "{}/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0", - url.trim_end_matches('/') - ); - let resp = client.get(&announce_url).send().await.unwrap(); + let announce_url = url + .join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0") + .expect("announce URL should be valid"); + let resp = client.get(announce_url.as_str()).send().await.unwrap(); let status = resp.status(); if !status.is_success() { let body = resp.text().await.unwrap_or_default(); @@ -134,8 +134,8 @@ struct DemoStats { tcp4_announces_handled: u64, } -async fn get_stats(api_url: &str, token: &str) -> DemoStats { - let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url).unwrap(), token)) +async fn get_stats(api_url: &Url, token: &str) -> DemoStats { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url.as_str()).unwrap(), token)) .unwrap() .get_tracker_statistics(None) .await diff --git a/tests/servers/api/contract/stats/mod.rs b/tests/servers/api/contract/stats/mod.rs index 506247a63..36e355bda 100644 --- a/tests/servers/api/contract/stats/mod.rs +++ b/tests/servers/api/contract/stats/mod.rs @@ -1,6 +1,7 @@ use serde::Deserialize; use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; use torrust_tracker_rest_api_client::v1::client::ApiHttpClient as TrackerApiClient; +use url::Url; use crate::common::{self, EphemeralTrackerWorkspace}; @@ -64,11 +65,10 @@ async fn the_stats_api_endpoint_should_return_the_global_stats() { // ── 3. Announce to both tracker instances ──────────────────────── let client = reqwest::Client::new(); for url in &tracker_urls { - let announce_url = format!( - "{}/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0", - url.trim_end_matches('/') - ); - let resp = client.get(&announce_url).send().await.unwrap(); + let announce_url = url + .join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0") + .expect("announce URL should be valid"); + let resp = client.get(announce_url.as_str()).send().await.unwrap(); let status = resp.status(); if !status.is_success() { let body = resp.text().await.unwrap_or_default(); @@ -90,8 +90,8 @@ struct PartialGlobalStatistics { tcp4_announces_handled: u64, } -async fn get_tracker_statistics(api_url: &str, token: &str) -> PartialGlobalStatistics { - let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url).unwrap(), token)) +async fn get_tracker_statistics(api_url: &Url, token: &str) -> PartialGlobalStatistics { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url.as_str()).unwrap(), token)) .unwrap() .get_tracker_statistics(None) .await From a2186a12c63708d86e29d23a12b86bb483336075 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 28 Jul 2026 13:16:42 +0100 Subject: [PATCH 7/9] docs(1419): plan runtime service registry refactor --- ...ne_registar_as_runtime_service_registry.md | 107 ++++++++++ docs/adrs/index.md | 1 + ...ease-main-app-integration-test-coverage.md | 4 +- .../ISSUE.md} | 18 +- ...investigation-registar-and-health-check.md | 182 ++++++++++++++++++ .../1419-runtime-service-registry-refactor.md | 175 +++++++++++++++++ tests/AGENTS.md | 4 +- 7 files changed, 485 insertions(+), 6 deletions(-) create mode 100644 docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md rename docs/issues/open/{1419-allow-multiple-integration-tests-at-main-app-level.md => 1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md} (95%) create mode 100644 docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md create mode 100644 docs/refactor-plans/open/1419-runtime-service-registry-refactor.md diff --git a/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md b/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md new file mode 100644 index 000000000..9ac86ccec --- /dev/null +++ b/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md @@ -0,0 +1,107 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md + - docs/refactor-plans/open/1419-runtime-service-registry-refactor.md + - src/container.rs + - tests/common/mod.rs + - packages/axum-health-check-api-server/src/handlers.rs +--- + +# Define Registar as the runtime service registry + +## Description + +Services configured with port zero receive their final listener address only after the operating +system binds their socket. The tracker needs a single, side-effect-free way for internal consumers +to discover those running services. The health check API already receives such information through +`Registar`, but the current registration record only contains a `ServiceBinding` and a function to +run a health check. Service role metadata is instead constructed while a health check runs. + +This forces consumers that need endpoint discovery, including main-application integration tests, +to infer service identity from bind IP conventions, registry iteration order, logs, or health-check +side effects. None is a valid application contract. + +## Agreement + +`Registar` is the authoritative internal registry of services that have successfully started. A +registration contains immutable local runtime metadata: + +- `ServiceBinding`: the listener protocol and final socket address; +- service role: the application role implemented by the listener; and +- optional health-check behavior. + +`ServiceBinding`, its socket address, and service role describe different facts. The binding is +derivable from `ServiceBinding` and must not be stored separately in the registration. Service role +cannot reconstruct `ServiceBinding`, because, for example, an HTTP tracker role may listen using +either HTTP or HTTPS. + +The role set is owned by the tracker, not by generic network primitives or `torrust-server-lib`. +Tracker packages use a shared `ServiceRole` enum to define canonical role names. The standalone +server library stores the resulting opaque role name and remains usable by applications with other +role sets. + +`AppContainer` retains ownership of application composition and boot-time configuration. +`JobManager` retains ownership of task lifecycle, cancellation, and shutdown. Neither replaces the +runtime registry. `ServiceRegistrationForm` remains the sole service-to-parent reporting channel; +no parallel registry or reporting type is introduced. + +The registry exposes local process listener data only. It does not represent public URLs, reverse +proxy routes, DNS names, load balancers, or other deployment topology. + +The health check API is a registry consumer. It obtains immutable identity metadata from the +registration and executes only optional health-check behavior. A metadata query must not itself +perform a health check. + +## Alternatives Considered + +### Infer service identity from listener IP address or protocol + +Rejected because multiple valid services can share HTTP, HTTPS, or the same bind IP. Deployment +configuration must not become a service-identity contract. + +### Derive identity from `AppContainer` service counts + +Rejected because configuration containers and runtime registrations have no stable identity mapping, +and `HashMap` iteration order is unspecified. + +### Use a health check to retrieve service metadata + +Rejected because it performs network I/O, reports health rather than identity, and fails exactly +when metadata is most useful for diagnosing an unhealthy service. + +### Add a separate runtime registry + +Rejected because `Registar` and `ServiceRegistrationForm` already provide the required runtime +service-to-parent reporting flow. A second registry would duplicate state and create consistency +risk. + +### Put tracker service roles in `torrust-net-primitives` or `torrust-server-lib` + +Rejected because `http_tracker`, `tracker_rest_api`, and `udp_tracker` are tracker application +roles, not generic network or server-library concepts. + +## Consequences + +- Internal consumers can discover final runtime bindings by role without log parsing or IP-based + conventions. +- Main-application integration tests can use port zero for all listeners and reliably target the + intended service. +- Health-check reporting has one source of truth for stable metadata. +- The change requires coordinated versioning: `torrust-server-lib` must release the registry API + before the tracker updates its dependency and migrates its server packages. + +## Date + +2026-07-28 + +## References + +- Issue #1419: [main-application integration tests](../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) +- [Runtime service registry refactor plan](../refactor-plans/open/1419-runtime-service-registry-refactor.md) +- [Investigation: runtime service registration and health check API](../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md) +- [App container](../../src/container.rs) +- [Integration-test helpers](../../tests/common/mod.rs) +- [Health-check handler](../../packages/axum-health-check-api-server/src/handlers.rs) diff --git a/docs/adrs/index.md b/docs/adrs/index.md index b77e79b56..e1c83c078 100644 --- a/docs/adrs/index.md +++ b/docs/adrs/index.md @@ -29,6 +29,7 @@ semantic-links: | [20260723184019](20260723184019_separate_configuration_value_invariants_from_consistency_validation.md) | 2026-07-23 | Separate configuration value invariants from consistency validation | Validate a single constrained value with a typed newtype; reserve `Validator` for multi-option consistency and bootstrap checks for environment-dependent validity. | | [20260727000000](20260727000000_events_are_objective_facts.md) | 2026-07-27 | Events are objective facts | Event variants must describe _what happened_ — a neutral, observable fact. Policy and mode decisions belong in the consumer or the enforcement point, never in the event definition. | | [20260727180000](20260727180000_shared_services_across_tracker_instances.md) | 2026-07-27 | Shared services across tracker instances | Peer repository and ban service are shared across all listener instances. Per-listener settings that affect shared services must be global to avoid inconsistency. | +| [20260728115400](20260728115400_define_registar_as_runtime_service_registry.md) | 2026-07-28 | Define Registar as the runtime service registry | `Registar` is the authoritative internal registry of started local services, their final bindings, and stable roles; health checks are one consumer of that metadata. | ## ADR Lifecycle diff --git a/docs/issues/drafts/increase-main-app-integration-test-coverage.md b/docs/issues/drafts/increase-main-app-integration-test-coverage.md index 379953f9f..a508cacdf 100644 --- a/docs/issues/drafts/increase-main-app-integration-test-coverage.md +++ b/docs/issues/drafts/increase-main-app-integration-test-coverage.md @@ -14,7 +14,7 @@ semantic-links: related-artifacts: - tests/stats.rs - tests/AGENTS.md - - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md related-issues: - 1347 - 1419 @@ -200,7 +200,7 @@ Suggested approach: ## Related Issues - [Issue #1419 - Allow multiple integration tests at the main app - level](../open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure + level](../open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) - Infrastructure foundation - [EPIC #1347 - Increase unit testing for workspace packages](https://github.com/torrust/torrust-tracker/issues/1347) - Package-level unit test diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md similarity index 95% rename from docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md rename to docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md index b8ca6f43d..a739ebf81 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md @@ -4,14 +4,16 @@ issue-type: enhancement status: open priority: p3 github-issue: 1419 -spec-path: docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md +spec-path: docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md branch: 1419-allow-multiple-integration-tests related-pr: null -last-updated-utc: 2026-07-27 17:35 +last-updated-utc: 2026-07-28 11:54 semantic-links: skill-links: - write-unit-test related-artifacts: + - docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md + - docs/refactor-plans/open/1419-runtime-service-registry-refactor.md - tests/stats.rs - tests/servers/ - src/app.rs @@ -358,3 +360,15 @@ Verification must show that the suite starts one application, runs all its scena shuts it down cleanly, and leaves no shared database, storage, or port dependency. Cross-suite parallel execution is supported through process isolation, but it is not a requirement for parallel full-application instances inside a single executable. + +## Runtime Registry Prerequisite + +The current test helper identifies services through test-only bind-IP conventions. That is not a +valid solution because tracker services may legitimately share the same listener address class. +Completing this issue therefore requires the runtime-service-registry refactor defined in +[the active refactor plan](../../../refactor-plans/open/1419-runtime-service-registry-refactor.md). + +The refactor is tracked on this issue branch because it is required to discover port-zero listener +addresses reliably. It includes a coordinated `torrust-server-lib` release, tracker-side migration, +and role-based endpoint discovery in the main-application test helpers. The architectural boundary +is recorded in [ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md). diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md new file mode 100644 index 000000000..6ecbaf97d --- /dev/null +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md @@ -0,0 +1,182 @@ +# Investigation: Runtime Service Registration and Health Check API + +## Goal + +Determine how the application can expose runtime-discovered service identity and final bindings to its internal consumers. This is needed to identify services reliably in integration tests when configured with port zero, without parsing logs or inferring identity from configured IP addresses. + +## Running tracker + +Started with config: all services on `0.0.0.0:0` (port zero). + +```text +HTTP TRACKER: Started on: http://0.0.0.0:33303 +HTTP TRACKER: Started on: http://0.0.0.0:52633 +API: Started on: http://0.0.0.0:46715 +HEALTH CHECK: Started on: http://0.0.0.0:46199 +``` + +## Health check API response + +The health check API endpoint returns service type information: + +```json +{ + "status": "Ok", + "message": "", + "details": [ + { + "service_binding": "http://0.0.0.0:33303/", + "binding": "0.0.0.0:33303", + "service_type": "http_tracker", + "info": "checking http tracker health check at: http://0.0.0.0:33303/health_check", + "result": { "Ok": "200 OK" } + }, + { + "service_binding": "http://0.0.0.0:52633/", + "binding": "0.0.0.0:52633", + "service_type": "http_tracker", + "info": "checking http tracker health check at: http://0.0.0.0:52633/health_check", + "result": { "Ok": "200 OK" } + }, + { + "service_binding": "http://0.0.0.0:46715/", + "binding": "0.0.0.0:46715", + "service_type": "tracker_rest_api", + "info": "checking api health check at: http://0.0.0.0:46715/api/health_check", + "result": { "Ok": "200 OK" } + } + ] +} +``` + +### Key observations + +- `service_type` values: `"http_tracker"`, `"tracker_rest_api"`, `"udp_tracker"` +- `info` strings contain the health check URL path, which differs by service type +- The health check API itself is NOT in the registar (it is the thing that queries the registar) + +## Independent runtime metadata + +The three identity-related fields in a health check report are not redundant. They describe separate facts about a service known to the tracker: + +| Field | Meaning | Example | +| ----------------- | ----------------------------------------------------- | ----------------------- | +| `binding` | The socket address on which the process is listening. | `0.0.0.0:33303` | +| `service_binding` | The local listener protocol plus its socket address. | `http://0.0.0.0:33303/` | +| `service_type` | The tracker role implemented by that listener. | `http_tracker` | + +`binding` alone does not identify a service role or protocol. `service_type` cannot reconstruct `service_binding`: an HTTP tracker may use either HTTP or HTTPS, and the role is intentionally independent of the transport protocol. The combination of `service_binding` and `service_type` is therefore required to identify both how the process listens and what it serves. + +These values describe only local runtime state managed by the tracker. They do not claim to be public client endpoints. A deployment may place a reverse proxy, load balancer, domain name, different public IP address, or path routing in front of the process. Public endpoint configuration is outside this registry's scope, including any optional public URL configuration introduced elsewhere. The registry records only the mandatory data needed to run and inspect the local services. + +## Current Registar structure + +`ServiceRegistration` stores only: + +- `service_binding: ServiceBinding` — the protocol + address +- `check_fn: FnSpawnServiceHeathCheck` — a function pointer to spawn health checks + +The `service_type` and `info` fields are only in `ServiceHealthCheckJob`, which is created by calling `spawn_check()` on the registration. This makes an HTTP request as a side effect. + +The registar uses `HashMap`. There is no service type information on the key or value. + +## Service type constants + +Each server package defines its own type string constant: + +| Package | Constant | Value | +| ---------------------- | ------------- | -------------------- | +| `axum-http-server` | `TYPE_STRING` | `"http_tracker"` | +| `axum-rest-api-server` | `TYPE_STRING` | `"tracker_rest_api"` | +| `udp-server` | `TYPE_STRING` | `"udp_tracker"` | + +These are passed to `ServiceHealthCheckJob::new()` but **not** stored in `ServiceRegistration`. + +### Candidate tracker-owned service role enum + +The duplicated constants should become one tracker-owned enum, tentatively named `ServiceRole` to distinguish it from the network `Protocol` stored in `ServiceBinding`: + +```rust +pub enum ServiceRole { + HttpTracker, + TrackerRestApi, + UdpTracker, +} + +impl ServiceRole { + pub const fn as_str(&self) -> &'static str { + match self { + Self::HttpTracker => "http_tracker", + Self::TrackerRestApi => "tracker_rest_api", + Self::UdpTracker => "udp_tracker", + } + } +} +``` + +`torrust-tracker-primitives` is a viable home because all three registering server packages already depend on it. `torrust-net-primitives` must not own this enum because the role values are tracker-specific, not generic network concepts. + +`torrust-server-lib` must remain decoupled from this closed tracker role set. The tracker packages should convert `ServiceRole` to its canonical string when constructing a `ServiceRegistration`; the generic registry stores that opaque role name and exposes it to its consumers. This preserves a single source of truth for tracker role names without making the standalone server library depend on tracker packages. + +## Architectural reassessment + +`Registar` was introduced for the health check API, but its data flow represents a broader responsibility: it receives information from services only after those services have started and therefore knows their final runtime bindings. In particular, it is the existing parent-side destination for bindings selected by the operating system when a service is configured with port zero. + +The health check API is one consumer of that runtime service information. Integration tests are another legitimate internal consumer: after `app::run()` has started services, a test needs to discover the concrete tracker and REST API endpoints in order to exercise them. Requiring either consumer to parse application logs, perform a health check merely to obtain metadata, or assume a particular bind IP is an API design gap. + +The responsibilities should remain distinct: + +- `AppContainer` owns application composition and boot-time configuration. It can describe which services are intended to run, but cannot by itself provide runtime facts that only exist after binding, such as an OS-assigned port. +- `Registar` owns the registry of runtime-discovered services and their stable descriptive metadata. +- `JobManager` owns task lifecycle management, including cancellation and shutdown. It must not become a service-information registry. +- Service-specific parent-child command channels remain appropriate where their behavior is specific to that service. They do not need to be generalized into the registry. + +The existing `ServiceRegistrationForm` is the appropriate child-to-parent channel for this information. Introducing a second registry or a new, parallel reporting type would duplicate that established startup flow without adding a useful separation of responsibilities. + +## Rejected approaches + +### Infer identity from the bind IP address + +This is the current test-only workaround: HTTP trackers use an unspecified address while the REST API and health check API use different loopback addresses. It is invalid as a production contract because users may legitimately configure any of these services with the same valid bind address, including `0.0.0.0`. + +### Derive service identity from `AppContainer` counts + +For example, selecting the first HTTP registrations based on `AppContainer.http_tracker_instance_containers.len()` is fragile. The registry contains multiple HTTP services, `HashMap` ordering is unspecified, and the relationship between configured service containers and runtime registrations is not an identity contract. + +### Reuse health checks as metadata queries + +Calling `spawn_check()` merely to obtain `service_type` makes a network request and couples a metadata query to service health. A registry lookup must be side-effect free and usable even when a service is unhealthy. + +### Add a separate runtime registry + +This would duplicate the registration form and service-to-parent reporting path that already delivers the final runtime binding. The existing `Registar` should be evolved instead. + +## Design direction + +`ServiceRegistration` should represent a running service's registration record, not only the data required to spawn a health check. It needs enough immutable metadata for consumers to identify the service and use its final binding without executing the health-check function. + +The tracker-owned `ServiceRole` enum should be the source of truth for currently supported tracker roles. `ServiceRegistration` in `torrust-server-lib` should store its canonical string form, keeping the generic crate independent from tracker packages. The health API can serialize that stored value using its existing `service_type: String` response field. + +The desired consumer outcome is conceptually: + +```rust +let tracker_services = container.registar.services_of_type(ServiceType::HttpTracker).await; +``` + +Each returned entry must provide the final `ServiceBinding`. Tests can then translate an unspecified listener address to a loopback client URL while retaining its runtime-assigned port. This removes assumptions about IP addresses, registry iteration order, protocol-only matching, and health-check side effects. + +## Questions for implementation design + +1. Should the generic role name in `torrust-server-lib` remain a `String` or become a generic validated newtype around `String`? +2. Should `ServiceRegistration` expose its own immutable metadata, or should `Registar` provide read-only query methods and hide the storage representation? +3. Which metadata is registration-time data versus health-check-execution data? Role and `ServiceBinding` are registration-time data; `info` and the result may remain health-check data. +4. Should `ServiceHealthCheckJob` stop carrying `service_binding` and `service_type`, so the health API obtains immutable identity metadata directly from the registration record? +5. How will the tracker update its dependency on the standalone `torrust-server-lib` crate once the registry API is finalized? + +## Decision and Implementation Handoff + +This investigation remains the record of observed current behavior, the discovered limitation, and +the reasoning that led to the change. The approved architectural boundary is defined by +[ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md). +The ordered implementation and validation work is defined by the +[runtime service registry refactor plan](../../../refactor-plans/open/1419-runtime-service-registry-refactor.md). diff --git a/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md b/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md new file mode 100644 index 000000000..bee01c4a4 --- /dev/null +++ b/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md @@ -0,0 +1,175 @@ +--- +doc-type: refactor-plan +status: open +related-issue: 1419 +spec-path: docs/refactor-plans/open/1419-runtime-service-registry-refactor.md +last-updated-utc: 2026-07-28 11:54 +semantic-links: + skill-links: + - create-refactor-plan + related-artifacts: + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md + - docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md +--- + + + +# Refactor Plan - Runtime Service Registry for #1419 + +## Goal + +Make runtime service discovery a reliable application capability by evolving `Registar` from a +health-check registration mechanism into the authoritative registry of started local services. +This removes the main-application integration tests' bind-IP heuristic and makes port-zero listener +configuration safe for all local services. + +Related artifact: [issue #1419](../../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) + +## Constraints + +- Preserve `ServiceBinding` as the local protocol-plus-socket-address representation; do not model + public or proxy-facing URLs in this work. +- Keep tracker service roles out of `torrust-net-primitives` and `torrust-server-lib`. +- Use the existing `ServiceRegistrationForm` reporting path; do not add a parallel registry. +- Keep `JobManager` responsible for lifecycle only. +- Preserve the health API response's `service_binding`, `binding`, and `service_type` fields. +- Complete the standalone `torrust-server-lib` release before updating the tracker dependency. + +## Items + +### 1. [x] Record the runtime registry architectural boundary [High impact / Low effort] + +**Problem**: `Registar` currently appears health-check-specific, leaving its relationship to +`AppContainer`, `JobManager`, and integration-test endpoint discovery implicit. + +**Files**: + +- `docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md` +- `docs/adrs/index.md` + +**Change**: Record `Registar` as the authoritative local runtime service registry, its metadata +scope, ownership boundaries, and rejected alternatives. + +--- + +### 2. [x] Add tracker-owned service role values [High impact / Low effort] + +**Problem**: HTTP tracker, REST API, and UDP tracker each define a duplicated `TYPE_STRING` +constant. The generic registry needs a role name, but must not own tracker-specific variants. + +**Files**: + +- `packages/primitives/src/service_role.rs` +- `packages/primitives/src/lib.rs` +- `packages/axum-http-server/src/server.rs` +- `packages/axum-rest-api-server/src/server.rs` +- `packages/udp-server/src/server/launcher.rs` + +**Change**: Add `ServiceRole` in `torrust-tracker-primitives`, with canonical role names exposed +through `as_str()`. Replace the three local constants with the appropriate enum variant. Decide and +document whether a non-self-checkable health-check API role is needed in the first migration. + +--- + +### 3. [ ] Extend the standalone registry registration record [High impact / Medium effort] + +**Problem**: `ServiceRegistration` stores only a binding and health-check function. Its consumers +cannot identify a service without executing network I/O through `spawn_check()`. + +**Files**: + +- `torrust-server-lib/src/registar.rs` (standalone repository) +- `torrust-server-lib` unit tests (standalone repository) + +**Change**: Store the canonical tracker-supplied role name in `ServiceRegistration`, provide +read-only access to registration metadata, and expose a snapshot or role-query API from `Registar` +without exposing `HashMap` ordering as a contract. Separate immutable registration metadata from +per-health-check execution data. Release a new compatible `torrust-server-lib` version. + +--- + +### 4. [ ] Migrate tracker registrations and health reporting [High impact / Medium effort] + +**Problem**: Server packages construct role strings only inside health-check jobs, so the registry +and health API have duplicated, indirectly related metadata paths. + +**Files**: + +- `packages/axum-http-server/src/server.rs` +- `packages/axum-rest-api-server/src/server.rs` +- `packages/udp-server/src/server/states.rs` +- `packages/udp-server/src/server/launcher.rs` +- `packages/axum-health-check-api-server/src/handlers.rs` +- `packages/axum-health-check-api-server/src/resources.rs` +- root `Cargo.toml` +- `Cargo.lock` + +**Change**: Upgrade to the released server-library version. Pass canonical role names through each +existing registration form. Build health reports from registration metadata plus health-check +execution results, keeping the external JSON field names and values stable. Add focused tests for +HTTP, HTTPS, REST API, and UDP registrations. + +--- + +### 5. [ ] Make registration visibility an application readiness guarantee [High impact / Medium effort] + +**Problem**: `tests/common/mod.rs` sleeps for 500 milliseconds after `app::run()` because +registration insertion happens asynchronously after a service reports through its form. + +**Files**: + +- `torrust-server-lib/src/registar.rs` (standalone repository) +- `src/app.rs` +- `tests/common/mod.rs` + +**Change**: Make the registration protocol acknowledge insertion or otherwise provide a deterministic +readiness boundary. Remove the fixed test delay only after `app::run()` or the relevant bootstrap +step guarantees registrations are visible. + +--- + +### 6. [ ] Replace IP-based endpoint discovery in main-application tests [High impact / Low effort] + +**Problem**: The integration helpers classify services by wildcard versus loopback bind IP. This +breaks for valid configurations that use the same bind address for multiple HTTP services. + +**Files**: + +- `tests/common/mod.rs` +- `tests/stats.rs` +- `tests/scaffold.rs` +- `tests/servers/api/contract/stats/mod.rs` + +**Change**: Query registrations by canonical role and use their final `ServiceBinding` values. Keep +only the client-side wildcard-to-loopback conversion needed to connect to a local listener. Configure +HTTP trackers, REST API, and health API with port zero without identity-by-address conventions. + +--- + +### 7. [ ] Validate the cross-repository migration [High impact / Medium effort] + +**Problem**: This change modifies a published dependency and every service-registration consumer. +Focused tests are needed to prove the new contract before broader workspace validation. + +**Files**: + +- `torrust-server-lib` test suite (standalone repository) +- `packages/axum-health-check-api-server/tests/` +- `tests/` + +**Change**: Add or update unit and contract tests for registration metadata, role-based lookups, +health-report compatibility, and port-zero main-application suites. Run the focused tests after each +slice, then the repository quality gates and full required checks. + +## Order of Execution + +| Order | Status | Item | Impact | Effort | +| ----- | ------ | --------------------------------------------------------------- | ------ | ------ | +| 1 | [x] | Record the runtime registry architectural boundary | High | Low | +| 2 | [x] | Add tracker-owned service role values | High | Low | +| 3 | [ ] | Extend the standalone registry registration record | High | Medium | +| 4 | [ ] | Migrate tracker registrations and health reporting | High | Medium | +| 5 | [ ] | Make registration visibility an application readiness guarantee | High | Medium | +| 6 | [ ] | Replace IP-based endpoint discovery in main-application tests | High | Low | +| 7 | [ ] | Validate the cross-repository migration | High | Medium | diff --git a/tests/AGENTS.md b/tests/AGENTS.md index d14632ef0..b6d12ca1d 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -6,7 +6,7 @@ semantic-links: - tests/stats.rs - tests/servers/ - src/app.rs - - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md issue-spec: docs/issues/drafts/increase-main-app-integration-test-coverage.md --- @@ -113,7 +113,7 @@ reference for the integration test pattern. ## References -- [Issue #1419](../../docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md) - Infrastructure for parallel integration tests (execution model decision) +- [Issue #1419](../../docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) - Infrastructure for parallel integration tests (execution model decision) - [Integration test scaffolding](stats.rs) - [Shared test utilities](common/mod.rs) - [Scaffolding demo](scaffold.rs) From a1f7a7508d3a6e8dc384c9e49490302685e5e6b9 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 28 Jul 2026 16:33:42 +0100 Subject: [PATCH 8/9] docs(issues): add specs for #2035 and #2036 --- ...ne_registar_as_runtime_service_registry.md | 4 +- .../ISSUE.md | 37 ++-- ...investigation-registar-and-health-check.md | 23 +- .../ISSUE.md | 121 +++++++++++ .../evidence.md | 199 ++++++++++++++++++ .../ISSUE.md | 119 +++++++++++ .../1419-runtime-service-registry-refactor.md | 175 --------------- tests/common/mod.rs | 8 + tests/scaffold.rs | 8 + tests/servers/api/contract/stats/mod.rs | 73 +++++++ 10 files changed, 577 insertions(+), 190 deletions(-) create mode 100644 docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md create mode 100644 docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/evidence.md create mode 100644 docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md delete mode 100644 docs/refactor-plans/open/1419-runtime-service-registry-refactor.md diff --git a/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md b/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md index 9ac86ccec..732f557cb 100644 --- a/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md +++ b/docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md @@ -4,7 +4,7 @@ semantic-links: - create-adr related-artifacts: - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md - - docs/refactor-plans/open/1419-runtime-service-registry-refactor.md + - docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md - src/container.rs - tests/common/mod.rs - packages/axum-health-check-api-server/src/handlers.rs @@ -100,7 +100,7 @@ roles, not generic network or server-library concepts. ## References - Issue #1419: [main-application integration tests](../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) -- [Runtime service registry refactor plan](../refactor-plans/open/1419-runtime-service-registry-refactor.md) +- Feature #2036: [add runtime service registry metadata](../issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md) - [Investigation: runtime service registration and health check API](../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md) - [App container](../../src/container.rs) - [Integration-test helpers](../../tests/common/mod.rs) diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md index a739ebf81..0ca089761 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md @@ -13,7 +13,8 @@ semantic-links: - write-unit-test related-artifacts: - docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md - - docs/refactor-plans/open/1419-runtime-service-registry-refactor.md + - docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md + - docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md - tests/stats.rs - tests/servers/ - src/app.rs @@ -361,14 +362,26 @@ shuts it down cleanly, and leaves no shared database, storage, or port dependenc parallel execution is supported through process isolation, but it is not a requirement for parallel full-application instances inside a single executable. -## Runtime Registry Prerequisite - -The current test helper identifies services through test-only bind-IP conventions. That is not a -valid solution because tracker services may legitimately share the same listener address class. -Completing this issue therefore requires the runtime-service-registry refactor defined in -[the active refactor plan](../../../refactor-plans/open/1419-runtime-service-registry-refactor.md). - -The refactor is tracked on this issue branch because it is required to discover port-zero listener -addresses reliably. It includes a coordinated `torrust-server-lib` release, tracker-side migration, -and role-based endpoint discovery in the main-application test helpers. The architectural boundary -is recorded in [ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md). +## Implementation Pause and Prerequisites + +The current integration suite correctly verifies aggregate statistics across two started HTTP +listeners. Its endpoint discovery is intentionally temporary: `tests/common/mod.rs` identifies +HTTP trackers and the REST API through distinct bind-IP conventions. If discovery is incorrect, +the test fails rather than producing a false aggregate-stats success, but the convention is not a +valid application contract and must not be extended to more integration suites. + +During implementation, two prerequisite defects were discovered. Work on this issue stops after +the current, working scaffold is documented and merged. The prerequisites will be implemented and +merged independently; #1419 remains open and resumes on that clean base. + +1. Bug #2035: [fix duplicate port-zero tracker instance bootstrap](../../open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md) + — `AppContainer` stores HTTP and UDP per-instance containers in `HashMap`. + Repeated `0.0.0.0:0` configuration blocks overwrite each other before startup, so distinct + per-instance configuration can be silently lost. +2. Feature #2036: [add runtime service registry metadata](../../open/2036-add-runtime-service-registry-metadata/ISSUE.md) + — `Registar` cannot expose stable service role or configuration-instance identity without + health-check side effects. This requires a coordinated `torrust-server-lib` change and release. + +The runtime registry boundary remains recorded in +[ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md). +The dedicated prerequisite specifications above are the implementation records for that boundary. diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md index 6ecbaf97d..714bb1b38 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md @@ -80,6 +80,27 @@ The `service_type` and `info` fields are only in `ServiceHealthCheckJob`, which The registar uses `HashMap`. There is no service type information on the key or value. +## Bootstrap collision discovered during implementation + +The endpoint-discovery limitation revealed a separate bootstrap correctness defect. `AppContainer` +stores HTTP and UDP per-instance containers in `HashMap`, keyed by the configured +`bind_address`. Two same-protocol configuration blocks using `0.0.0.0:0` therefore collide before +either service starts: the later insertion overwrites the earlier container. + +Bootstrap then iterates both configuration blocks but retrieves the surviving container by the same +`0.0.0.0:0` key for each. Two listeners can start with distinct OS-assigned ports, yet both use the +later configuration block's settings. This silently loses per-instance configuration such as +`tracker_usage_statistics` and affects HTTP and UDP trackers alike. + +A final `ServiceBinding` uniquely identifies a running listener, but it cannot retrospectively +identify which repeated configuration block created that listener. Bootstrap must first preserve +configuration-instance identity, for example through an ordered collection aligned with the +configuration entries. The runtime registry can then carry that identity with the final binding. + +This is tracked separately in Bug #2035: [fix duplicate port-zero tracker instance bootstrap](../../open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md). +The external-crate registry work is tracked separately in +Feature #2036: [add runtime service registry metadata](../../open/2036-add-runtime-service-registry-metadata/ISSUE.md). + ## Service type constants Each server package defines its own type string constant: @@ -179,4 +200,4 @@ This investigation remains the record of observed current behavior, the discover the reasoning that led to the change. The approved architectural boundary is defined by [ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md). The ordered implementation and validation work is defined by the -[runtime service registry refactor plan](../../../refactor-plans/open/1419-runtime-service-registry-refactor.md). +[runtime service registry metadata feature](../../open/2036-add-runtime-service-registry-metadata/ISSUE.md). diff --git a/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md b/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md new file mode 100644 index 000000000..1e45b3bf2 --- /dev/null +++ b/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md @@ -0,0 +1,121 @@ +--- +doc-type: issue +issue-type: bug +status: open +priority: p1 +github-issue: 2035 +spec-path: docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md +branch: 2035-fix-duplicate-port-zero-tracker-instance-bootstrap +related-pr: null +last-updated-utc: 2026-07-28 13:06 +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - src/container.rs + - src/app.rs + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md + - evidence.md + related-issues: + - 1419 +--- + +# Issue #2035 - Fix Duplicate Port-Zero Tracker Instance Bootstrap + +## Goal + +Start every configured HTTP and UDP tracker instance with its own configuration, including when +multiple same-protocol blocks use the same configured port-zero bind address. + +## Background + +`AppContainer` stores HTTP and UDP instance containers in `HashMap`, keyed by each +configuration block's `bind_address`. `HashMap::insert` replaces the previous value for an equal +key. Consequently, two HTTP tracker blocks both configured as `0.0.0.0:0` leave only the later +container in the map. + +Application startup then iterates both configuration blocks and looks up a container using the +same configured address. Both services start using the surviving later configuration, even though +the operating system gives each listener a distinct final port. The same defect exists for UDP +trackers. This can silently apply the wrong per-instance behavior, for example +`tracker_usage_statistics`, TLS, or network settings. + +The local reproduction is recorded in [evidence.md](evidence.md). + +## Scope + +### In Scope + +- Preserve each configured HTTP and UDP tracker instance even when configured bind addresses are equal. +- Replace address-keyed instance-container storage with an order-preserving representation aligned + with configuration entries, or an equivalent stable configuration-instance identifier. +- Start each configured HTTP and UDP instance with its matching container. +- Include the configuration instance index in HTTP and UDP bootstrap lifecycle logs, including + events that report configured and final bound addresses. +- Add regressions with repeated `0.0.0.0:0` blocks whose configuration differs. + +### Out of Scope + +- Runtime registry metadata or health-check API changes. +- Public endpoint, proxy, or DNS configuration. +- User-supplied persistent service IDs in configuration. + +## Implementation Plan + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | -------------------------------------- | ------------------------------------------------------------------------------------------------------ | +| T1 | DONE | Add failing HTTP bootstrap regression | Ignored stats-contract regression records the current `2 != 1` failure. | +| T2 | TODO | Add failing UDP bootstrap regression | Same identity preservation for UDP instances. | +| T3 | TODO | Replace address-keyed container lookup | Startup aligns each configuration entry with its own initialized container. | +| T4 | TODO | Remove obsolete address lookup API | No bootstrap path relies on configured `SocketAddr` uniqueness. | +| T5 | TODO | Correlate bootstrap lifecycle logs | Every HTTP and UDP lifecycle event that emits a configured or final binding includes `instance_index`. | +| T6 | TODO | Run focused and workspace validation | Record before/after evidence in this issue folder. | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Specification drafted and approved by user/maintainer +- [x] GitHub issue created: #2035 +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests) +- [ ] Manual verification scenarios executed and recorded +- [ ] Acceptance criteria reviewed after implementation +- [ ] Issue closed and specification moved to `docs/issues/closed/` + +### Progress Log + +- 2026-07-28 14:51 UTC - agent - User-approved specification promoted to GitHub issue #2035; + the ignored HTTP stats-contract regression and its current `2 != 1` failure are recorded in + [evidence.md](evidence.md). + +## Acceptance Criteria + +- [ ] AC1: Two HTTP tracker blocks with the same `0.0.0.0:0` binding each start with their own configuration. +- [ ] AC2: Two UDP tracker blocks with the same `0.0.0.0:0` binding each start with their own configuration. +- [ ] AC3: Bootstrap does not use configured `SocketAddr` as a unique instance identity. +- [ ] AC4: HTTP and UDP startup logs include the configuration `instance_index`, allowing logs + with duplicate configured addresses to be correlated with their source configuration block. +- [ ] AC5: Focused HTTP, UDP, and application bootstrap tests pass. +- [ ] AC6: `linter all` exits with code `0`. + +## Verification Plan + +### Automatic Checks + +- Focused regression tests for `AppContainer` and startup jobs. +- `cargo test --test stats --test scaffold` after the runtime-registry prerequisite lands. +- `linter all`. + +### Manual Verification Scenarios + +| ID | Scenario | Expected Result | Status | Evidence | +| --- | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------ | ------ | -------------------------- | +| M1 | Start two HTTP trackers with identical `0.0.0.0:0` bindings and different settings. | Each listener retains the settings from its own configuration block. | TODO | [evidence.md](evidence.md) | +| M2 | Repeat M1 for UDP trackers. | Each UDP listener retains the settings from its own configuration block. | TODO | | + +## References + +- Issue #1419: [main-application integration tests](../../open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) +- [Runtime registry investigation](../../open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md) +- Feature #2036: [add runtime service registry metadata](../2036-add-runtime-service-registry-metadata/ISSUE.md) diff --git a/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/evidence.md b/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/evidence.md new file mode 100644 index 000000000..9b2a0738f --- /dev/null +++ b/docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/evidence.md @@ -0,0 +1,199 @@ +# Bootstrap Collision Evidence + +## Purpose + +Demonstrate the current HTTP bootstrap defect before implementing the fix: duplicate configured +`0.0.0.0:0` bindings overwrite the first instance container, so both started listeners use the +second configuration block. + +## Environment + +- Repository: `torrust/torrust-tracker` +- Working branch: `1419-allow-multiple-integration-tests` +- Execution date: `2026-07-28` +- Required tools: Rust/Cargo and a writable `/tmp` directory + +No network access, external tracker, generated certificate, or source-file change was retained +after this reproduction. + +## Reproduction Configuration + +The following complete configuration was written to +`/tmp/torrust-1419-bootstrap-evidence/tracker.toml`: + +```toml +[metadata] +app = "torrust-tracker" +purpose = "configuration" +schema_version = "2.0.0" + +[logging] +threshold = "debug" + +[core] +listed = false +private = false + +[core.database] +driver = "sqlite3" +path = "/tmp/torrust-1419-bootstrap-evidence/storage/sqlite3.db" + +[[http_trackers]] +bind_address = "0.0.0.0:0" +tracker_usage_statistics = true + +[[http_trackers]] +bind_address = "0.0.0.0:0" +tracker_usage_statistics = false + +[http_api] +bind_address = "127.0.0.1:0" + +[http_api.access_tokens] +admin = "evidence-token" + +[health_check_api] +bind_address = "127.0.0.2:0" +``` + +## Temporary Instrumentation + +The following temporary debug events were added solely for this reproduction. They were removed +immediately after recording the output and are not part of the working tree. + +In `src/container.rs`, the HTTP configuration loop was temporarily changed to enumerate entries, +capture the return value from `HashMap::insert`, and emit: + +```rust +tracing::debug!( + index, + bind_address = %http_tracker_config.bind_address, + tracker_usage_statistics = http_tracker_config.tracker_usage_statistics, + replaced = replaced.is_some(), + "Initialized HTTP tracker instance container" +); +``` + +In `src/app.rs`, immediately after retrieving the HTTP container for a configuration entry, this +temporary event was emitted: + +```rust +tracing::debug!( + index = idx, + bind_address = %http_tracker_config.bind_address, + configured_tracker_usage_statistics = http_tracker_config.tracker_usage_statistics, + container_tracker_usage_statistics = http_tracker_container.http_tracker_config.tracker_usage_statistics, + "Starting HTTP tracker instance" +); +``` + +## Commands Executed + +From the repository root, the configuration directory and file were created, then the tracker was +started with that file: + +```sh +mkdir -p /tmp/torrust-1419-bootstrap-evidence/storage +printf '%s\n' \ + '[metadata]' \ + 'app = "torrust-tracker"' \ + 'purpose = "configuration"' \ + 'schema_version = "2.0.0"' \ + '' \ + '[logging]' \ + 'threshold = "debug"' \ + '' \ + '[core]' \ + 'listed = false' \ + 'private = false' \ + '' \ + '[core.database]' \ + 'driver = "sqlite3"' \ + 'path = "/tmp/torrust-1419-bootstrap-evidence/storage/sqlite3.db"' \ + '' \ + '[[http_trackers]]' \ + 'bind_address = "0.0.0.0:0"' \ + 'tracker_usage_statistics = true' \ + '' \ + '[[http_trackers]]' \ + 'bind_address = "0.0.0.0:0"' \ + 'tracker_usage_statistics = false' \ + '' \ + '[http_api]' \ + 'bind_address = "127.0.0.1:0"' \ + '' \ + '[http_api.access_tokens]' \ + 'admin = "evidence-token"' \ + '' \ + '[health_check_api]' \ + 'bind_address = "127.0.0.2:0"' \ + > /tmp/torrust-1419-bootstrap-evidence/tracker.toml + +TORRUST_TRACKER_CONFIG_TOML_PATH=/tmp/torrust-1419-bootstrap-evidence/tracker.toml cargo run +``` + +After recording the output, the tracker process was terminated and both temporary source edits +were removed. The final verification command was: + +```sh +git diff -- src/app.rs src/container.rs +``` + +It produced no output, confirming the probe did not remain in production code. + +## Observed Output + +Cargo rebuilt the tracker successfully and started `target/debug/torrust-tracker`. The tracker +loaded both HTTP blocks exactly as configured. The following complete set of discriminator lines +was emitted during bootstrap and startup: + +```text +Initialized HTTP tracker instance container index=0 bind_address=0.0.0.0:0 tracker_usage_statistics=true replaced=false +Initialized HTTP tracker instance container index=1 bind_address=0.0.0.0:0 tracker_usage_statistics=false replaced=true +Starting HTTP tracker instance index=0 bind_address=0.0.0.0:0 configured_tracker_usage_statistics=true container_tracker_usage_statistics=false +HTTP TRACKER: Started on: http://0.0.0.0:33439 +Starting HTTP tracker instance index=1 bind_address=0.0.0.0:0 configured_tracker_usage_statistics=false container_tracker_usage_statistics=false +HTTP TRACKER: Started on: http://0.0.0.0:33983 +``` + +The normal tracker output also showed that the REST API and health check API started successfully; +their output is not relevant to this defect and is omitted above. The compile progress, metrics, +database migration diagnostics, and unrelated service logs are likewise omitted because they do +not affect the configuration-collision result. + +## Result + +The `replaced=true` result proves that the second configuration entry overwrote the first in the +address-keyed map. The first startup record proves that configuration index `0` was started using +the surviving container from index `1`. Distinct runtime ports do not preserve the lost +configuration-instance identity. + +This run used temporary instrumentation only. No production debug statements remain after the +evidence capture. + +## Automated Regression Evidence + +The application-level regression +`the_stats_api_endpoint_should_exclude_announces_from_a_tracker_with_statistics_disabled` now +captures the same defect without temporary production instrumentation. It configures two HTTP +trackers with `0.0.0.0:0`: the first disables usage statistics and the second enables them. It +announces once to each listener and expects the global `tcp4_announces_handled` counter to be `1`. + +The regression is intentionally ignored until this issue is implemented so the regular integration +suite remains green. It was run explicitly from the repository root with: + +```sh +cargo test --test stats the_stats_api_endpoint_should_exclude_announces_from_a_tracker_with_statistics_disabled -- --ignored +``` + +The command compiled successfully, started the isolated application, and failed with: + +```text +assertion `left == right` failed + left: 2 + right: 1 +``` + +The observed `2` shows that both listeners inherited the second configuration block's enabled +statistics setting. After the bootstrap fix, remove the `#[ignore]` attribute and the same test +must pass with the expected count of `1`. diff --git a/docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md b/docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md new file mode 100644 index 000000000..d5f2fec3d --- /dev/null +++ b/docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md @@ -0,0 +1,119 @@ +--- +doc-type: issue +issue-type: enhancement +status: open +priority: p1 +github-issue: 2036 +spec-path: docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md +branch: 2036-add-runtime-service-registry-metadata +related-pr: null +last-updated-utc: 2026-07-28 12:30 +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md + - docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md + related-issues: + - 1419 +--- + +# Issue #2036 - Add Runtime Service Registry Metadata + +## Goal + +Evolve `Registar` into a side-effect-free internal source of final local listener bindings, tracker +service roles, and configuration-instance correlation metadata. + +## Background + +`ServiceRegistration` in the standalone `torrust-server-lib` crate stores a final +`ServiceBinding` and a health-check function. The health-check function constructs the service role +only when it executes network I/O. Consumers cannot discover a running HTTP tracker versus REST +API through the registry without relying on bind-IP conventions, `HashMap` iteration order, logs, +or an unnecessary health check. + +The bootstrap prerequisite must land first. A final listener binding identifies a running listener, +but repeated `0.0.0.0:0` configuration blocks require bootstrap to preserve configuration-instance +identity before that identity can be reported to the registry. + +## Scope + +### In Scope + +- Extend the standalone `torrust-server-lib` registration record and read-only query API. +- Define tracker-owned canonical service-role values without coupling the generic library to them. +- Carry bootstrap-provided configuration-instance identity with each registration. +- Make registration visibility a deterministic application-readiness boundary. +- Build health-check reports from registration metadata and health-check execution results. +- Release the standalone crate and upgrade the tracker dependency. + +### Out of Scope + +- Fixing duplicate port-zero bootstrap storage; owned by the prerequisite issue. +- Public URLs, proxies, domain names, and deployment topology. +- Dynamic service restart, deregistration, or configuration reload. + +## Implementation Plan + +| ID | Status | Task | Notes / Expected Output | +| --- | ------- | ------------------------------------------------------------- | --------------------------------------------------------------------------- | +| T1 | BLOCKED | Merge bootstrap identity prerequisite | Registration needs a stable configuration-instance identity to carry. | +| T2 | TODO | Define tracker-owned service role and instance identity types | Keep tracker semantics out of generic network and server crates. | +| T3 | TODO | Extend `ServiceRegistration` in `torrust-server-lib` | Store immutable metadata and make health-check behavior optional. | +| T4 | TODO | Add side-effect-free registry query API | Do not expose `HashMap` ordering as a contract. | +| T5 | TODO | Establish registration readiness | Acknowledge insertion or provide an equivalent readiness boundary. | +| T6 | TODO | Release the standalone crate | Publish a compatible version before tracker migration. | +| T7 | TODO | Migrate tracker registrations and health reporting | Preserve the health API JSON contract. | +| T8 | TODO | Replace #1419 bind-IP helper | Query runtime metadata by role and instance identity without a fixed delay. | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Specification drafted and approved by user/maintainer +- [x] GitHub issue created: #2036 +- [ ] Prerequisite #2035 completed +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests in both repositories) +- [ ] Manual verification scenarios executed and recorded +- [ ] Acceptance criteria reviewed after implementation +- [ ] Issue closed and specification moved to `docs/issues/closed/` + +### Progress Log + +- 2026-07-28 14:51 UTC - agent - User-approved specification promoted to GitHub feature #2036; + implementation remains blocked on the configuration-instance identity fix in #2035. + +## Acceptance Criteria + +- [ ] AC1: Internal consumers discover final local bindings without running health checks. +- [ ] AC2: Registrations expose tracker role and configuration-instance correlation metadata. +- [ ] AC3: The health-check response preserves `service_binding`, `binding`, and `service_type`. +- [ ] AC4: The generic server library remains independent of tracker-specific role variants. +- [ ] AC5: #1419 removes its bind-IP endpoint classification and fixed registration delay. +- [ ] AC6: Focused tests pass in both repositories and `linter all` exits with code `0`. + +## Verification Plan + +### Automatic Checks + +- `torrust-server-lib` unit tests for registration metadata and query behavior. +- Health-check API contract tests. +- `cargo test --test stats --test scaffold` in the tracker repository. +- `linter all` in both repositories. + +### Manual Verification Scenarios + +| ID | Scenario | Expected Result | Status | Evidence | +| --- | ------------------------------------------------------------------------ | ------------------------------------------------------------------------------- | ------ | -------- | +| M1 | Start HTTP, HTTPS, REST API, and UDP services with port zero. | Registry distinguishes local protocol, role, and final binding. | TODO | | +| M2 | Start repeated HTTP tracker configuration instances after bootstrap fix. | Registry correlates each final listener to the intended configuration instance. | TODO | | + +## References + +- [ADR 20260728115400](../../../adrs/20260728115400_define_registar_as_runtime_service_registry.md) +- Prerequisite #2035: [fix duplicate port-zero tracker instance bootstrap](../2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md) +- Issue #1419: [main-application integration tests](../../open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) +- [Runtime registry investigation](../../open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md) diff --git a/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md b/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md deleted file mode 100644 index bee01c4a4..000000000 --- a/docs/refactor-plans/open/1419-runtime-service-registry-refactor.md +++ /dev/null @@ -1,175 +0,0 @@ ---- -doc-type: refactor-plan -status: open -related-issue: 1419 -spec-path: docs/refactor-plans/open/1419-runtime-service-registry-refactor.md -last-updated-utc: 2026-07-28 11:54 -semantic-links: - skill-links: - - create-refactor-plan - related-artifacts: - - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md - - docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md - - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md ---- - - - -# Refactor Plan - Runtime Service Registry for #1419 - -## Goal - -Make runtime service discovery a reliable application capability by evolving `Registar` from a -health-check registration mechanism into the authoritative registry of started local services. -This removes the main-application integration tests' bind-IP heuristic and makes port-zero listener -configuration safe for all local services. - -Related artifact: [issue #1419](../../issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md) - -## Constraints - -- Preserve `ServiceBinding` as the local protocol-plus-socket-address representation; do not model - public or proxy-facing URLs in this work. -- Keep tracker service roles out of `torrust-net-primitives` and `torrust-server-lib`. -- Use the existing `ServiceRegistrationForm` reporting path; do not add a parallel registry. -- Keep `JobManager` responsible for lifecycle only. -- Preserve the health API response's `service_binding`, `binding`, and `service_type` fields. -- Complete the standalone `torrust-server-lib` release before updating the tracker dependency. - -## Items - -### 1. [x] Record the runtime registry architectural boundary [High impact / Low effort] - -**Problem**: `Registar` currently appears health-check-specific, leaving its relationship to -`AppContainer`, `JobManager`, and integration-test endpoint discovery implicit. - -**Files**: - -- `docs/adrs/20260728115400_define_registar_as_runtime_service_registry.md` -- `docs/adrs/index.md` - -**Change**: Record `Registar` as the authoritative local runtime service registry, its metadata -scope, ownership boundaries, and rejected alternatives. - ---- - -### 2. [x] Add tracker-owned service role values [High impact / Low effort] - -**Problem**: HTTP tracker, REST API, and UDP tracker each define a duplicated `TYPE_STRING` -constant. The generic registry needs a role name, but must not own tracker-specific variants. - -**Files**: - -- `packages/primitives/src/service_role.rs` -- `packages/primitives/src/lib.rs` -- `packages/axum-http-server/src/server.rs` -- `packages/axum-rest-api-server/src/server.rs` -- `packages/udp-server/src/server/launcher.rs` - -**Change**: Add `ServiceRole` in `torrust-tracker-primitives`, with canonical role names exposed -through `as_str()`. Replace the three local constants with the appropriate enum variant. Decide and -document whether a non-self-checkable health-check API role is needed in the first migration. - ---- - -### 3. [ ] Extend the standalone registry registration record [High impact / Medium effort] - -**Problem**: `ServiceRegistration` stores only a binding and health-check function. Its consumers -cannot identify a service without executing network I/O through `spawn_check()`. - -**Files**: - -- `torrust-server-lib/src/registar.rs` (standalone repository) -- `torrust-server-lib` unit tests (standalone repository) - -**Change**: Store the canonical tracker-supplied role name in `ServiceRegistration`, provide -read-only access to registration metadata, and expose a snapshot or role-query API from `Registar` -without exposing `HashMap` ordering as a contract. Separate immutable registration metadata from -per-health-check execution data. Release a new compatible `torrust-server-lib` version. - ---- - -### 4. [ ] Migrate tracker registrations and health reporting [High impact / Medium effort] - -**Problem**: Server packages construct role strings only inside health-check jobs, so the registry -and health API have duplicated, indirectly related metadata paths. - -**Files**: - -- `packages/axum-http-server/src/server.rs` -- `packages/axum-rest-api-server/src/server.rs` -- `packages/udp-server/src/server/states.rs` -- `packages/udp-server/src/server/launcher.rs` -- `packages/axum-health-check-api-server/src/handlers.rs` -- `packages/axum-health-check-api-server/src/resources.rs` -- root `Cargo.toml` -- `Cargo.lock` - -**Change**: Upgrade to the released server-library version. Pass canonical role names through each -existing registration form. Build health reports from registration metadata plus health-check -execution results, keeping the external JSON field names and values stable. Add focused tests for -HTTP, HTTPS, REST API, and UDP registrations. - ---- - -### 5. [ ] Make registration visibility an application readiness guarantee [High impact / Medium effort] - -**Problem**: `tests/common/mod.rs` sleeps for 500 milliseconds after `app::run()` because -registration insertion happens asynchronously after a service reports through its form. - -**Files**: - -- `torrust-server-lib/src/registar.rs` (standalone repository) -- `src/app.rs` -- `tests/common/mod.rs` - -**Change**: Make the registration protocol acknowledge insertion or otherwise provide a deterministic -readiness boundary. Remove the fixed test delay only after `app::run()` or the relevant bootstrap -step guarantees registrations are visible. - ---- - -### 6. [ ] Replace IP-based endpoint discovery in main-application tests [High impact / Low effort] - -**Problem**: The integration helpers classify services by wildcard versus loopback bind IP. This -breaks for valid configurations that use the same bind address for multiple HTTP services. - -**Files**: - -- `tests/common/mod.rs` -- `tests/stats.rs` -- `tests/scaffold.rs` -- `tests/servers/api/contract/stats/mod.rs` - -**Change**: Query registrations by canonical role and use their final `ServiceBinding` values. Keep -only the client-side wildcard-to-loopback conversion needed to connect to a local listener. Configure -HTTP trackers, REST API, and health API with port zero without identity-by-address conventions. - ---- - -### 7. [ ] Validate the cross-repository migration [High impact / Medium effort] - -**Problem**: This change modifies a published dependency and every service-registration consumer. -Focused tests are needed to prove the new contract before broader workspace validation. - -**Files**: - -- `torrust-server-lib` test suite (standalone repository) -- `packages/axum-health-check-api-server/tests/` -- `tests/` - -**Change**: Add or update unit and contract tests for registration metadata, role-based lookups, -health-report compatibility, and port-zero main-application suites. Run the focused tests after each -slice, then the repository quality gates and full required checks. - -## Order of Execution - -| Order | Status | Item | Impact | Effort | -| ----- | ------ | --------------------------------------------------------------- | ------ | ------ | -| 1 | [x] | Record the runtime registry architectural boundary | High | Low | -| 2 | [x] | Add tracker-owned service role values | High | Low | -| 3 | [ ] | Extend the standalone registry registration record | High | Medium | -| 4 | [ ] | Migrate tracker registrations and health reporting | High | Medium | -| 5 | [ ] | Make registration visibility an application readiness guarantee | High | Medium | -| 6 | [ ] | Replace IP-based endpoint discovery in main-application tests | High | Low | -| 7 | [ ] | Validate the cross-repository migration | High | Medium | diff --git a/tests/common/mod.rs b/tests/common/mod.rs index a498a2cd5..39538c10d 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -87,6 +87,11 @@ pub async fn start_tracker_with_config(workspace: &EphemeralTrackerWorkspace) -> /// Returns the HTTP tracker URLs from the registar. /// +/// TODO: Replace this temporary bind-IP classification after +/// `fix-duplicate-port-zero-tracker-instance-bootstrap` and +/// `add-runtime-service-registry-metadata` are implemented. Those issues establish stable +/// configuration-instance identity and role-based runtime discovery. +/// /// HTTP trackers bind to `0.0.0.0` (unspecified). The REST API and health /// check bind to `127.0.0.1` (loopback). We identify trackers by their /// unspecified IP, which is deterministic regardless of hash-map ordering. @@ -104,6 +109,9 @@ pub async fn http_tracker_urls(container: &AppContainer) -> Vec { /// Returns the HTTP API URL from the registar. /// +/// TODO: Replace this temporary bind-IP classification with role-based runtime discovery. See +/// `docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md`. +/// /// The REST API binds to `127.0.0.1` (loopback), unlike the HTTP trackers /// which bind to `0.0.0.0`. pub async fn http_api_url(container: &AppContainer) -> Option { diff --git a/tests/scaffold.rs b/tests/scaffold.rs index 4b80a6caf..28fd03885 100644 --- a/tests/scaffold.rs +++ b/tests/scaffold.rs @@ -33,6 +33,14 @@ //! - A small startup delay to allow async service registration. //! - Sequential scenarios that account for accumulated state. //! +//! ## Temporary Limitation +//! +//! Endpoint discovery currently distinguishes services by test-only bind-IP conventions. This +//! sample must not be copied as a general service-discovery pattern until the bootstrap and runtime +//! registry prerequisite issues are implemented. See +//! `docs/issues/open/2035-fix-duplicate-port-zero-tracker-instance-bootstrap/ISSUE.md` and +//! `docs/issues/open/2036-add-runtime-service-registry-metadata/ISSUE.md`. +//! //! # Example: Running this test //! //! ```text diff --git a/tests/servers/api/contract/stats/mod.rs b/tests/servers/api/contract/stats/mod.rs index 36e355bda..78d49822d 100644 --- a/tests/servers/api/contract/stats/mod.rs +++ b/tests/servers/api/contract/stats/mod.rs @@ -15,6 +15,10 @@ use crate::common::{self, EphemeralTrackerWorkspace}; /// /// Single-instance announce and scrape behavior is tested in the /// `axum-http-server` package. +/// +/// TODO: Replace the temporary bind-IP endpoint discovery used by this suite after +/// `fix-duplicate-port-zero-tracker-instance-bootstrap` and +/// `add-runtime-service-registry-metadata` are implemented. #[tokio::test] async fn the_stats_api_endpoint_should_return_the_global_stats() { // ── 1. Configuration ────────────────────────────────────────────── @@ -84,6 +88,75 @@ async fn the_stats_api_endpoint_should_return_the_global_stats() { // when `workspace` and `_jobs` are dropped at the end of this scope. } +/// A disabled tracker must not contribute to global statistics. +/// +/// This regression is ignored until +/// `fix-duplicate-port-zero-tracker-instance-bootstrap` preserves an individual container for +/// every repeated `0.0.0.0:0` configuration entry. At present, the address-keyed bootstrap map +/// makes both listeners use the later enabled configuration, so both announces are counted. +#[ignore = "blocked by fix-duplicate-port-zero-tracker-instance-bootstrap"] +#[tokio::test] +async fn the_stats_api_endpoint_should_exclude_announces_from_a_tracker_with_statistics_disabled() { + let config_toml = r#" + [metadata] + app = "torrust-tracker" + purpose = "configuration" + schema_version = "2.0.0" + + [logging] + threshold = "off" + + [core] + listed = false + private = false + + [core.database] + driver = "sqlite3" + path = "{STORAGE_PATH}/sqlite3.db" + + [[http_trackers]] + bind_address = "0.0.0.0:0" + tracker_usage_statistics = false + + [[http_trackers]] + bind_address = "0.0.0.0:0" + tracker_usage_statistics = true + + [http_api] + bind_address = "127.0.0.1:0" + + [http_api.access_tokens] + admin = "MyAccessToken" + + [health_check_api] + bind_address = "127.0.0.2:0" + "#; + + let workspace = EphemeralTrackerWorkspace::new(config_toml); + let (app_container, _jobs) = common::start_tracker_with_config(&workspace).await; + + let tracker_urls = common::http_tracker_urls(&app_container).await; + assert_eq!(tracker_urls.len(), 2, "expected two HTTP trackers"); + + let api_url = common::http_api_url(&app_container).await.expect("expected an HTTP API URL"); + + let client = reqwest::Client::new(); + for url in &tracker_urls { + let announce_url = url + .join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0") + .expect("announce URL should be valid"); + let response = client.get(announce_url.as_str()).send().await.unwrap(); + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + panic!("announce to {url} failed: status {status}, body: {body}"); + } + } + + let global_stats = get_tracker_statistics(&api_url, "MyAccessToken").await; + assert_eq!(global_stats.tcp4_announces_handled, 1); +} + /// Global statistics with only metrics relevant to the test. #[derive(Deserialize)] struct PartialGlobalStatistics { From e3a4cd6d1a4673144e06b80c7e75e721a8e23a06 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 28 Jul 2026 16:47:06 +0100 Subject: [PATCH 9/9] docs(tests): reference stats integration test binary --- .../ISSUE.md | 4 ++-- tests/AGENTS.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md index 0ca089761..0ec43fd39 100644 --- a/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md @@ -182,7 +182,7 @@ then fix the scaffolding infrastructure, then expand coverage. | ID | Status | Task | Notes | | --- | ------ | ----------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | TODO | Create `tests/AGENTS.md` with guidelines and TODO list | Document what belongs in main-level integration tests vs package tests; include prioritized TODO list of future valuable integration tests | +| T1 | DONE | Create `tests/AGENTS.md` with guidelines and TODO list | Document what belongs in main-level integration tests vs package tests; include prioritized TODO list of future valuable integration tests | | T2 | TODO | Add second assertion to existing stats test | Check another global stat field (e.g., `tcp4_scrapes_handled` or `tcp6_announces_handled`) to prove need for parallel test capability | | T3 | TODO | Create test utilities module | `tests/helpers.rs` with utilities for temp workspace creation (config + storage dirs) and port extraction | | T4 | TODO | Add utility to create isolated temp workspace | Returns `TempDir` with subdirectories for config and storage; writes TOML config; sets `TORRUST_TRACKER_CONFIG_TOML_PATH` env var | @@ -217,7 +217,7 @@ then fix the scaffolding infrastructure, then expand coverage. ## Acceptance Criteria -- [ ] AC1: `tests/AGENTS.md` exists and documents guidelines for what belongs at main-level vs +- [x] AC1: `tests/AGENTS.md` exists and documents guidelines for what belongs at main-level vs package-level, with a TODO list of future valuable integration tests. - [ ] AC2: Multiple integration tests can run concurrently with `cargo test --test stats` without port conflicts. diff --git a/tests/AGENTS.md b/tests/AGENTS.md index b6d12ca1d..044398d7a 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -84,7 +84,7 @@ tests/ ├── AGENTS.md # This file ├── common/ │ └── mod.rs # Shared test utilities (temp config, port extraction) -├── integration.rs # Global statistics suite (main integration tests) +├── stats.rs # Global statistics suite (main integration tests) ├── scaffold.rs # Scaffolding demo — pattern reference for new binaries └── servers/ └── api/