diff --git a/Cargo.lock b/Cargo.lock index 81a66b83c..e49aac631 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", @@ -4842,6 +4840,7 @@ dependencies = [ "torrust-tracker-udp-server", "tracing", "tracing-subscriber", + "url", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index c9252ef8d..ada409b88 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,10 +73,9 @@ 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" } +url = { version = "2", features = [ "serde" ] } [workspace] members = [ 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..732f557cb --- /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/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 +--- + +# 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) +- 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) +- [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 new file mode 100644 index 000000000..a508cacdf --- /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/stats.rs + - tests/AGENTS.md + - docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.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 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 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 + +- 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/ISSUE.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/stats.rs](../../../tests/stats.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. 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 new file mode 100644 index 000000000..0ec43fd39 --- /dev/null +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/ISSUE.md @@ -0,0 +1,387 @@ +--- +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/ISSUE.md +branch: 1419-allow-multiple-integration-tests +related-pr: null +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/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 + - 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/stats.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 | 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 | +| 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 stats` 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 stats`) +- [ ] 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 +- 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 + +- [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. +- [ ] 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 stats` passes cleanly with expanded test coverage. +- [ ] AC9: `linter all` passes. + +## Verification Plan + +### Automatic Checks + +- `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 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 + +- **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. + +## 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. + +## 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 new file mode 100644 index 000000000..714bb1b38 --- /dev/null +++ b/docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level/investigation-registar-and-health-check.md @@ -0,0 +1,203 @@ +# 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. + +## 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: + +| 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 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/tests/AGENTS.md b/tests/AGENTS.md new file mode 100644 index 000000000..044398d7a --- /dev/null +++ b/tests/AGENTS.md @@ -0,0 +1,119 @@ +--- +semantic-links: + skill-links: + - write-unit-test + related-artifacts: + - tests/stats.rs + - tests/servers/ + - src/app.rs + - 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 +--- + +# 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. + +## 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**: Use `tempfile::TempDir` to create + isolated directories with separate config files and storage subdirectories +3. **Extract actual bound ports**: Query `AppContainer`'s `Registar` to get the OS-assigned ports + for making requests +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 + +```text +tests/ +├── AGENTS.md # This file +├── common/ +│ └── mod.rs # Shared test utilities (temp config, port extraction) +├── stats.rs # Global statistics suite (main integration tests) +├── scaffold.rs # Scaffolding demo — pattern reference for new binaries +└── servers/ + └── api/ + └── contract/ + └── stats/ + └── mod.rs # Global statistics test scenarios +``` + +## 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. **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/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) diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 000000000..39538c10d --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,147 @@ +//! 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; +use url::Url; + +/// 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. +/// +/// 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. +/// 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. +/// +/// 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 { + 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| 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, Url)> { + 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) -> Url { + if addr.ip().is_unspecified() { + Url::parse(&format!("http://127.0.0.1:{port}", port = addr.port())) + } else { + Url::parse(&format!("http://{addr}")) + } + .expect("loopback URL should always be valid") +} diff --git a/tests/scaffold.rs b/tests/scaffold.rs new file mode 100644 index 000000000..28fd03885 --- /dev/null +++ b/tests/scaffold.rs @@ -0,0 +1,153 @@ +//! 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. +//! +//! ## 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 +//! 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 url::Url; + +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 = 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(); + 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: &Url, token: &str) -> DemoStats { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url.as_str()).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..78d49822d 100644 --- a/tests/servers/api/contract/stats/mod.rs +++ b/tests/servers/api/contract/stats/mod.rs @@ -1,23 +1,28 @@ -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 url::Url; + +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. +/// +/// 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() { - // 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 +37,124 @@ 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); + + [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 = 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(); + panic!("announce to {url} failed: status {status}, body: {body}"); + } } - let (_app_container, _jobs) = app::run().await; + // ── 4. Verify both announces are aggregated ────────────────────── + let global_stats = get_tracker_statistics(&api_url, "MyAccessToken").await; + assert_eq!(global_stats.tcp4_announces_handled, 2); - announce_to_tracker("http://127.0.0.1:7272").await; - announce_to_tracker("http://127.0.0.1:7373").await; + // The tracker application and its temporary workspace are cleaned up + // when `workspace` and `_jobs` are dropped at the end of this scope. +} - let global_stats = get_tracker_statistics("http://127.0.0.1:1414", "MyAccessToken").await; +/// 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" - assert_eq!(global_stats.tcp4_announces_handled, 2); -} + [logging] + threshold = "off" -/// 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()); + [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. @@ -91,8 +163,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: &Url, token: &str) -> PartialGlobalStatistics { + let response = TrackerApiClient::new(ConnectionInfo::authenticated(Origin::new(api_url.as_str()).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;