diff --git a/docs/copilot-pr-reviews/pr-2123-copilot-suggestions.md b/docs/copilot-pr-reviews/pr-2123-copilot-suggestions.md new file mode 100644 index 000000000..71296f20d --- /dev/null +++ b/docs/copilot-pr-reviews/pr-2123-copilot-suggestions.md @@ -0,0 +1,52 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +--- + + + + + +# PR #2123 Copilot Suggestions Tracking + +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/2123 + +Status legend: + +- `action`: code/docs change applied +- `no-action`: suggestion reviewed; no code change needed +- `resolved`: thread resolved in PR + +## Workflow + +1. Download all review threads (including resolved/outdated state and thread IDs). +2. Add one row per thread in the Suggestions table. +3. Process suggestions one by one: + - decide `action` or `no-action` + - if `action`, apply change and validate + - if needed, commit changes + - reply on the PR thread with the fix commit and outcome, or the no-action rationale + - resolve the PR thread + +4. Set `Thread State` to `resolved` once resolved in PR. + +## Processing Log + +- 2026-08-31: Started processing suggestions. +- 2026-08-31: Completed processing suggestions. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ---------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------------- | -------- | --------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6d0DdR | docs/issues/open/2122-expose-unambiguous-download-counter-semantics/ISSUE.md | https://github.com/torrust/torrust-tracker/pull/2123#discussion_r3896790783 | Replace inconsistent "non-ambiguous" wording with "unambiguous". | action | https://github.com/torrust/torrust-tracker/pull/2123#discussion_r3898753179 | DONE | RESOLVED | + +## Notes + +- Keep this file as an audit log of review handling for the PR. +- Prefer concise decisions with explicit rationale. +- If no code changes are needed, explain why in `Decision`. +- Reply on every PR suggestion thread before resolving it so the decision is visible to reviewers. diff --git a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md b/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md index 063474e8b..289734bad 100644 --- a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md +++ b/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md @@ -27,7 +27,7 @@ semantic-links: - packages/tracker-core/src/container.rs - share/container/entry_script_sh - contrib/dev-tools/containers/tests/test-mounted-no-persistence-configuration.sh - - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/bootstrap-error-propagation-draft.md + - docs/issues/open/2121-propagate-bootstrap-startup-errors/ISSUE.md - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/manual-t2-rest-route-contract.md - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/manual-t3-persistence-free-runtime.md - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/manual-m2-persistence-requirements.md @@ -150,15 +150,15 @@ manual/documentation evidence. Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. -| ID | Status | Task | Notes / Expected Output | -| --- | ----------- | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | DONE | Activate persistence validation | Active v3 bootstrap invokes the centralized check after configuration validation and before globals or containers are built. | -| T2 | DONE | Compose capability-aware REST API | Key and whitelist routes retain registration but short-circuit to `409`/`ActionStatus::Err` when their capability is disabled; their adapters are not constructed. | -| T3 | DONE | Build persistence-free core graph | Tracker-core now groups database stores and persistence-only services in optional `PersistenceServices`; public HTTP/UDP and REST composition has no database fallback. | -| T4 | DONE | Preserve persistence-enabled composition | SQLite, MySQL, and PostgreSQL configured-driver lifecycle suites passed, including complete-migration and idempotency coverage. | -| T5 | DONE | Adapt supported container startup | A packaged v3 public default omits persistence; no override, SQLite seed, or persistence-only directory is used unless SQLite is explicitly selected. | -| T6 | DONE | Add regression and transition tests | Covered mounted-configuration precedence and non-destructive SQLite disable, target-change, and reuse transitions; existing configuration coverage preserves the v2 rejection boundary. | -| T7 | DONE | Execute manual evidence and documentation | M1-M6, #999 evidence, migration guidance, final acceptance review, and quality gates are complete. | +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | DONE | Activate persistence validation | Active v3 bootstrap invokes the centralized check after configuration validation and before globals or containers are built. | +| T2 | DONE | Compose capability-aware REST API | Key and whitelist routes retain registration but short-circuit to `409`/`ActionStatus::Err` when their capability is disabled; their adapters are not constructed. | +| T3 | DONE | Build persistence-free core graph | Tracker-core now groups database stores and persistence-only services in optional `PersistenceServices`; public HTTP/UDP and REST composition has no database fallback. | +| T4 | DONE | Preserve persistence-enabled composition | SQLite, MySQL, and PostgreSQL configured-driver lifecycle suites passed, including complete-migration and idempotency coverage. | +| T5 | DONE | Adapt supported container startup | A packaged v3 public default omits persistence; no override, SQLite seed, or persistence-only directory is used unless SQLite is explicitly selected. | +| T6 | DONE | Add regression and transition tests | Covered mounted-configuration precedence and non-destructive SQLite disable, target-change, and reuse transitions; existing configuration coverage preserves the v2 rejection boundary. | +| T7 | DONE | Execute manual evidence and documentation | M1-M6, #999 evidence, migration guidance, final acceptance review, and quality gates are complete. | ## Progress Tracking @@ -303,9 +303,9 @@ mysql:8.0` succeeded. The canonical MySQL compatibility suite passed, so release-image transition regression checks checksums across persistence disable, target change, and original-target reuse. - [x] AC13: #999 manual evidence and acceptance verification are updated - truthfully, including API-disabled-capability evidence. + truthfully, including API-disabled-capability evidence. - [x] AC14: `linter all` exits with code `0`, relevant automated tests pass, - and acceptance criteria are re-reviewed against observed evidence. + and acceptance criteria are re-reviewed against observed evidence. ## Verification Plan @@ -356,7 +356,7 @@ Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`, `DEFERRED`. | AC10 | DONE | Local REST torrent query returned the in-memory swarm; disabled capability routes returned `409`; `manual-t3-persistence-free-runtime.md`. | | AC11 | DONE | Release-image M6 evidence: `manual-m6-container-no-persistence.md`. | | AC12 | DONE | `test-mounted-no-persistence-configuration.sh` checks old/new SQLite checksums across persistence disable, target change, and original-target reuse. | -| AC13 | DONE | #999 M1-M6 scenario and acceptance records now link the #2107 runtime, REST, container, and transition evidence. | +| AC13 | DONE | #999 M1-M6 scenario and acceptance records now link the #2107 runtime, REST, container, and transition evidence. | | AC14 | DONE | Focused shipped-template/v2-boundary tests, release-image transition regression, `linter all`, and the prior applicable pre-push suite passed; acceptance criteria re-reviewed. | ## Risks and Trade-offs diff --git a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/bootstrap-error-propagation-draft.md b/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/bootstrap-error-propagation-draft.md deleted file mode 100644 index 1c9c3d4af..000000000 --- a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/bootstrap-error-propagation-draft.md +++ /dev/null @@ -1,159 +0,0 @@ ---- -doc-type: issue -issue-type: task -status: draft -priority: p2 -epic: null -github-issue: null -spec-path: docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/bootstrap-error-propagation-draft.md -branch: "{issue-number}-propagate-bootstrap-errors" -related-pr: null -depends-on: - - 2107 -last-updated-utc: 2026-08-28 11:58 -semantic-links: - skill-links: - - create-issue - related-artifacts: - - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md - - src/AGENTS.md - - src/bootstrap/app.rs - - src/bootstrap/config.rs - - src/bootstrap/persistence.rs - - src/container.rs - - src/app.rs - - src/main.rs ---- - -# Draft - Propagate bootstrap startup errors - -> **Deferred follow-up:** Do not create this GitHub issue until #2107 is -> complete. #2107's persistence-free composition work may change the final -> application-container error boundary and should supply the concrete failures -> this refactor must represent. - -## Goal - -Make expected startup, configuration, and composition failures explicit typed -errors that bubble from bootstrap to the executable boundary, where they are -reported with context and terminate the process with a nonzero exit status. - -## Background - -The configuration source APIs already expose fallible operations: -`Info::new`, `Configuration::load`, semantic validation, and bootstrap -persistence-requirement validation each return errors. Current startup code -converts these expected operator-facing failures to `expect` or `panic` in -`initialize_configuration` and `setup`. Application-container composition also -assumes successful initialization. - -Fail-fast startup remains the intended operational behavior, but panicking -hides the typed cause at intermediate boundaries and makes individual failure -paths harder to test. A `Result`-based bootstrap boundary will show what can -fail, preserve error context, and give executable entrypoints one consistent -way to report startup failure. - -## Scope - -### In Scope - -- Return typed errors from configuration source creation and loading instead of - panicking in `initialize_configuration`. -- Define a focused bootstrap error type that retains source errors from - configuration loading, semantic validation, persistence-requirement - validation, and application-container composition. -- Change bootstrap `setup()` to return a `Result` and propagate it through - `app::run()` and executable entrypoints, including profiling or test helpers - where their startup contracts require it. -- Refactor application-container initialization to return a contextual typed - error once #2107 establishes the persistence-enabled and persistence-free - composition paths. -- Report startup failures at the executable boundary with useful context and a - nonzero exit status. -- Add focused tests for representative configuration, validation, and - composition failures without binding listener ports or creating unrelated - runtime services. - -### Out of Scope - -- Changing `check_seed()` from its assertion-based internal cryptographic - invariant. It is not operator configuration input. -- Treating asynchronous failures after successful startup as bootstrap results. -- Reclassifying operational database failures as configuration validation - errors. -- Making every server bind, TLS, job lifecycle, or shutdown path transactional - unless the final typed startup boundary requires a narrowly scoped cleanup. -- Implementing this refactor as part of #2107. - -## Proposed Design - -1. Let `initialize_configuration()` return a configuration-load `Result`. -2. Introduce `bootstrap::app::Error` with variants that preserve configuration - source, semantic validation, persistence requirement, and composition - causes. -3. Let `setup()` return `Result<(Configuration, AppContainer), Error>`. -4. Propagate the error through `app::run()` to each executable boundary. -5. At the executable boundary, format the error for operators and exit - unsuccessfully while leaving internal invariants as assertions. - -The exact error types and whether `run()` has a distinct wrapper error must be -decided after #2107's final container composition is known. Avoid converting -errors to strings early: callers and tests need access to the original error -category. - -## Implementation Plan - -Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - -| ID | Status | Task | Notes / Expected Output | -| --- | ------ | ------------------------------- | ---------------------------------------------------------------------------------- | -| T1 | TODO | Reconcile after #2107 | Identify final fallible container and startup boundaries from #2107's merged code. | -| T2 | TODO | Define typed bootstrap errors | Preserve original sources and stable contextual categories. | -| T3 | TODO | Propagate setup and run results | Update startup callers and executable error reporting. | -| T4 | TODO | Add focused failure-path tests | Exercise source, semantic, requirement, and composition failures. | -| T5 | TODO | Document final startup contract | Update `src/AGENTS.md` and operator-facing docs if the policy changes. | - -## Acceptance Criteria - -- [ ] Expected configuration, validation, and composition failures are returned - as typed errors through bootstrap rather than panicking. -- [ ] Error chains retain their original source category and meaningful - bootstrap context. -- [ ] The executable boundary reports startup failure and exits nonzero. -- [ ] `check_seed()` remains an assertion for its internal invariant. -- [ ] Focused tests cover representative failures and prove no unrelated - services begin after bootstrap failure. -- [ ] Documentation describes the resulting startup-error contract truthfully. - -## Verification Plan - -- Focused bootstrap and application tests for each error category. -- Entry-point tests for contextual reporting and nonzero termination where the - test harness permits it. -- `cargo fmt`, relevant package tests, `linter all`, and the required - repository quality gates. - -## Progress Tracking - -### Workflow Checkpoints - -- [x] Deferred draft recorded while implementing #2107. -- [ ] #2107 completed and final composition error boundaries reviewed. -- [ ] Draft refined and approved for GitHub issue creation. -- [ ] GitHub issue created and linked to its final parent or related work. - -### Progress Log - -- 2026-08-28 11:58 UTC - GitHub Copilot/User - Recorded this deferred draft - after observing expected startup failures converted to `expect` or `panic`. - The draft must be reconciled after #2107; it is not current implementation - scope. - -## References - -- Current implementation issue: #2107 -- Startup policy: `src/AGENTS.md` -- Configuration bootstrap: `src/bootstrap/config.rs` -- Bootstrap composition: `src/bootstrap/app.rs` -- Application startup: `src/app.rs` -- Executable entrypoint: `src/main.rs` diff --git a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/download-counter-semantics-investigation.md b/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/download-counter-semantics-investigation.md deleted file mode 100644 index 0f782e5d5..000000000 --- a/docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/download-counter-semantics-investigation.md +++ /dev/null @@ -1,142 +0,0 @@ ---- -doc-type: defect-investigation -issue: 2107 -status: confirmed -last-updated-utc: 2026-08-28 ---- - -# Download Counter Semantics Investigation - -## Confirmed Defect - -`tracker_core_persistent_torrents_downloads_total` is an in-memory metrics -repository counter whose name and description claim that its value is -persisted. Its event handler increments it for every `PeerDownloadCompleted` -event whenever the tracker-core event listener runs. The listener is started -when either tracker usage statistics or persistent completed statistics is -enabled. - -When persistent completed statistics are enabled with tracker usage statistics, -the intended startup path loads the database aggregate into this in-memory -counter and the persistent listener updates the database aggregate. That -represents a total retained across process restarts. - -When persistence is disabled but tracker usage statistics is enabled, the same -in-memory counter is incremented but cannot survive restart. If the counter is -exported as the persisted total, its current name and description are false. - -The retention behavior is intentional. Commit `b0e74439` records that the -tracker-core metric is available regardless of persistence: it contains the -session download count without database persistence and a restored historical -count when persistence is enabled. The defect is the metric name, its -description, and the REST `completed` field documentation, which do not state -that distinction. - -## Known Evidence - -- `packages/tracker-core/src/statistics/mod.rs` defines the counter as - `tracker_core_persistent_torrents_downloads_total` and describes it as "The - total number of torrent downloads (persisted)." -- `packages/tracker-core/src/statistics/persisted/mod.rs` loads the database - aggregate into that in-memory counter at persistence-enabled startup. -- `packages/tracker-core/src/statistics/event/handler.rs` currently increments - that in-memory counter for every completed-download event before conditional - database writes. -- `src/bootstrap/jobs/tracker_core.rs` currently starts one event listener when - either `tracker_usage_statistics` or - `tracker_policy.persistent_torrent_completed_stat` is enabled. -- `packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs` exposes this - counter as `Stats.completed` for `GET /api/v1/stats`. -- `packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs` - describes `completed` as the total number of peers that have ever completed - downloading, without a persistence-free qualification. -- `GET /api/v1/metrics` exposes the labeled counter, including its current - `persistent` name and persisted description. -- Commit `b0e74439` states that the counter is deliberately session-scoped - when download persistence is disabled and persistent when it is enabled. - -## Questions To Confirm - -1. [x] Which REST API responses, metrics exporters, or internal consumers - expose `Repository::get_torrents_downloads_total`? `GET /api/v1/stats` - maps it to `completed`; `GET /api/v1/metrics` exports the labeled - tracker-core metric. -2. [x] Does a no-persistence tracker intentionally expose a session-only - completed total? Yes; the behavior is recorded in `b0e74439`. -3. [ ] Add a regression test proving that a restart without persistence resets - the exposed total to zero. -4. [x] Does a restart with persistence enabled restore the exposed total? - Yes; tracker-core integration coverage verifies restoration. -5. [x] Does tracker usage statistics require event-derived completed-download - metrics? Yes; the listener's only observed in-memory output is the - session completed-download counter used by the stats endpoints. - -## Candidate Resolution - -Retain one exported in-memory counter whose value is session-scoped without -persistence and restored from database persistence when enabled. Give its -description and REST field documentation an explicit retention contract. - -Renaming the current labeled metric would change a public Prometheus-style -identifier and must be treated as a compatibility decision. First establish -whether v3 permits that rename or whether the existing name must remain while -its description is corrected. Do not expose a second counter or change REST -response shapes in this work. The #2107 scope already permits process-local -completed counts in persistence-free operation. - -## Listener Design Decision - -The existing listener appears to have two responsibilities: - -- update the in-memory completed-download counter for tracker usage statistics; -- update per-torrent and global database counters when persistent completed - statistics are enabled. - -After confirming actual consumers, compare these options: - -| Option | Benefits | Costs | Status | -| ---------------------------------------------------- | ---------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | -------- | -| Retain one listener with explicit persistence branch | One receiver and task; preserves event order in one handler. | Persistence branch remains in a usage-statistics listener and must receive an optional dependency. | Rejected | -| Split in-memory and persistence listeners | Each listener receives only its concrete dependencies; no database work/task when persistence is disabled. | Two receivers/tasks when both features are enabled; duplicated event filtering and more bootstrap lifecycle management. | Selected | -| Start only a persistence listener | Avoids the listener without persistence. | Incorrect because tracker usage statistics needs the session counter. | Rejected | - -## Plan - -Status values: `TODO`, `IN_PROGRESS`, `DONE`, `BLOCKED`, `NOT_APPLICABLE`. - -| ID | Status | Task | Evidence / Completion Condition | -| --- | ------ | -------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | -| D1 | DONE | Record the potential counter semantics defect separately from the persistence-capability refactor. | This document. | -| D2 | DONE | Trace the counter's API, exporter, internal consumers, and historical intent. | `GET /api/v1/stats` and `GET /api/v1/metrics` expose it; `b0e74439` confirms session-versus-persistent semantics. | -| D3 | TODO | Add a focused persistence-free restart regression test. | Demonstrate that the exposed count resets to zero. | -| D4 | DONE | Confirm whether tracker usage statistics requires event-derived completed-download metrics. | The listener maintains the session counter exposed by stats endpoints. | -| D5 | DONE | Decide listener topology from D2-D4. | Selected independent in-memory and persistence listeners; implementation is tracked by P2-P4. | -| D6 | TODO | Decide the labeled metric compatibility strategy. | Approve a v3 rename or retain the identifier with corrected descriptions. | -| D7 | TODO | Implement the confirmed counter naming/documentation fix and listener refactor, if required. | Focused tests and updated documentation. | -| D8 | TODO | Run applicable checks and record results. | Relevant tests, formatting, linting, and manual evidence when necessary. | - -## Non-Goals - -- Do not merge this investigation with the persistence-capability refactor. -- Do not change REST response shapes or add historical-data provenance in this - task. -- Do not assume the current metric behavior is a bug until D2-D4 are complete. - -## Progress Log - -- 2026-08-28 - Created after discovering that the metric named and described - as persisted is incremented in memory even for a no-persistence runtime. The - behavior may correctly represent a session total, but its naming or exposed - semantics may be defective. Investigation precedes implementation. -- 2026-08-28 - Confirmed the retention behavior through public API/export - tracing and commit `b0e74439`: session total without persistence, restored - historical total with it. The defect is the inaccurate metric name and - descriptions. The listener must remain active for tracker usage statistics; - splitting database writes into a second listener remains a performance and - complexity trade-off, not a prerequisite for correcting the documentation. -- 2026-08-28 - Selected two independent listeners for the target runtime: - tracker usage statistics starts the mandatory in-memory listener; persistent - completed statistics starts an additional listener with a concrete database - repository. Statistics disabled starts neither listener. Implementation is - intentionally deferred until the committed persistence-capability plan is - reviewed. diff --git a/docs/issues/open/2121-propagate-bootstrap-startup-errors/ISSUE.md b/docs/issues/open/2121-propagate-bootstrap-startup-errors/ISSUE.md new file mode 100644 index 000000000..b2617444f --- /dev/null +++ b/docs/issues/open/2121-propagate-bootstrap-startup-errors/ISSUE.md @@ -0,0 +1,255 @@ +--- +doc-type: issue +issue-type: task +status: open +priority: p2 +epic: null +github-issue: 2121 +spec-path: docs/issues/open/2121-propagate-bootstrap-startup-errors/ISSUE.md +branch: "2121-propagate-bootstrap-errors" +related-pr: 2123 +depends-on: + - 2107 +last-updated-utc: 2026-08-31 17:13 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - .github/skills/dev/planning/create-issue/SKILL.md + - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md + - src/AGENTS.md + - src/bootstrap/app.rs + - src/bootstrap/config.rs + - src/bootstrap/persistence.rs + - src/container.rs + - src/app.rs + - src/main.rs +--- + + + +# Issue #2121 - Propagate bootstrap startup errors + +## Goal + +Make every expected failure during initial tracker startup an explicit typed +error from `app::run()` through the tracker executable boundary. The executable +must report the failure with context and exit unsuccessfully, cancelling any +jobs already started during the failed startup attempt. + +## Background + +The configuration source APIs already expose fallible operations: +`Info::new`, `Configuration::load`, semantic validation, and bootstrap +persistence-requirement validation each return errors. After #2107, tracker +core composition also has a persistence-enabled branch that can fail while +constructing the configured database driver and applying migrations. + +Current startup code converts these categories to `expect` or `panic` in +`initialize_configuration`, `setup`, and `AppContainer::initialize`. +`app::start()` and its job-starter call stack also panic for expected +database-load, TLS-material, registration, and listener-start failures. These +are expected, operator-facing startup failures, but panic messages discard +their typed source and make failure paths difficult to test directly. + +Fail-fast startup remains the intended operational behavior, but panicking +hides the typed cause at intermediate boundaries and makes individual failure +paths harder to test. A `Result`-based bootstrap boundary will show what can +fail, preserve error context, clean up partially started jobs, and give +executable entrypoints one consistent way to report startup failure. + +## Scope + +### In Scope + +- Return typed errors from configuration source creation and loading instead of + panicking in `initialize_configuration`. +- Define typed startup errors that retain source errors from configuration + loading, semantic validation, persistence-requirement validation, + application-container composition, initial persistence data loading, and + configured service startup. +- Change `setup()`, `start()`, each fallible startup helper, and `app::run()` + to return and propagate typed `Result` values. Update executable callers, + including profiling and integration test helpers that start the complete + application. +- Refactor application-container and tracker-core initialization so expected + configured-driver and migration failures return a contextual typed error + rather than being converted to `expect` or an ambiguous `Option`. +- Report startup failures at the executable boundary with useful context and a + nonzero exit status. +- Cancel and join jobs that were started before a subsequent initial startup + failure, without treating post-start task failures as startup results. +- Add focused tests for representative configuration, composition, + persistence-load, and listener-start failures without starting unrelated + runtime services. + +### Out of Scope + +- Changing `check_seed()` from its assertion-based internal cryptographic + invariant. It is not operator configuration input. +- Treating asynchronous task failures that occur after that task successfully + starts as initial startup results. +- Reclassifying operational database failures as configuration validation + errors. +- Changing graceful shutdown behavior after successful startup. + +## Architectural Decisions + +- Related ADR: `docs/adrs/20260825193119_make_persistence_an_optional_application_composition_capability.md`. +- Related completed work: #2107 established the persistence-free and + persistence-enabled composition branches this task must preserve. +- `app::run()` owns the typed startup boundary: it returns `Ok` only after + `setup()`, initial persistence loading, and configured job startup succeed. + Error variants retain their source categories instead of being flattened to + strings. +- If a configured service fails after another job has started, `run()` cancels + and joins the already-started jobs before returning that startup error. +- `check_seed()` remains an assertion because it guards an internal + cryptographic invariant, not an operator-controlled configuration failure. +- ADRs to create: None known. Create one during implementation if the error + boundary changes a repository-wide error-handling policy or package contract. + +## Known Refactoring Targets + +These targets reflect the current startup path and are subject to T1 +reconciliation; they are not an exhaustive implementation inventory. + +- `src/bootstrap/config.rs`: make `initialize_configuration()` return its + configuration-source or load error. +- `src/bootstrap/app.rs`: replace expected validation panics and return a + source-preserving bootstrap `Result` from `setup()`. +- `packages/tracker-core/src/container.rs` and `src/container.rs`: return + typed configured-driver, migration, and application-composition errors rather + than `Option` or `expect`. +- `src/app.rs`: make `start()`, initial persistence loaders, service starters, + and `run()` propagate expected startup errors. Cancel already-started jobs + when a later startup operation fails. +- `src/bootstrap/jobs/health_check_api.rs`, + `src/bootstrap/jobs/http_tracker.rs`, `src/bootstrap/jobs/tracker_apis.rs`, + and `src/bootstrap/jobs/udp_tracker.rs`: return typed TLS, registration, and + listener-start errors rather than panicking. +- `src/bootstrap/jobs/tracker_core.rs`: replace the persistence assumption in + the persistent-statistics listener startup path with a typed startup error. +- `src/main.rs`, `src/console/profiling.rs`, and + `tests/common/workspace.rs`: handle or surface `run()` failures according to + their executable and test contracts. +- `src/AGENTS.md`: replace the stated startup-panic policy with the final + documented startup-error contract. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| T1 | TODO | Map expected startup failures | Recursively inspect every call from `app::run()` through `setup()`, `start()`, data loading, and initial job startup; classify expected sources, invariants, and post-start task failures. | +| T2 | TODO | Make composition fallible | Return typed errors from tracker-core and application-container initialization; replace expected failure `expect`/`Option` paths without changing valid persistence-free composition. | +| T3 | TODO | Establish bootstrap boundary | Make `initialize_configuration()` and `setup()` return typed `Result` values with source-preserving bootstrap context. | +| T4 | TODO | Propagate the complete startup result | Make `start()`, its loaders, and its configured service starters return typed errors; have `run()` cancel and join partial startup jobs before it returns an error. | +| T5 | TODO | Report at executable callers | Report `app::run()` errors consistently from the tracker and profiling entrypoints and adapt full-application test helpers. | +| T6 | TODO | Prove failure behavior | Add focused failure-path tests and document the final startup-error contract in `src/AGENTS.md` and operator-facing documentation when it changes. | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Deferred draft recorded while implementing #2107. +- [x] #2107 completed and final composition error boundaries reviewed. +- [x] Spec drafted in `docs/issues/drafts/`. +- [x] Spec reviewed and approved by user/maintainer. +- [x] GitHub issue #2121 created and issue number added to this spec. +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation. +- [ ] Implementation completed. +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks). +- [ ] Manual verification scenarios executed and recorded (status + evidence). +- [ ] Acceptance criteria reviewed after implementation and updated with evidence. +- [ ] Reviewer validated acceptance criteria and updated checkboxes. +- [ ] Committer verified spec progress is up to date before commit. +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/`. + +### Progress Log + +- 2026-08-28 11:58 UTC - GitHub Copilot/User - Recorded a deferred draft after observing expected startup failures converted to `expect` or `panic` during #2107. +- 2026-08-31 15:46 UTC - GitHub Copilot - Reconciled the draft with merged and closed #2107. The formal issue draft defines typed configuration and composition errors through `setup()` and `app::run()` while retaining post-setup runtime failures and `check_seed()` outside scope. +- 2026-08-31 15:46 UTC - GitHub Copilot/User - Added a concrete, non-exhaustive list of current refactoring targets. T1 remains responsible for reconciling it with the exact error types and callers before implementation. +- 2026-08-31 16:03 UTC - GitHub Copilot/User - Expanded the intended boundary from `setup()` to complete initial startup. `app::run()` must propagate expected failures from `setup()`, `start()`, initial persistence loading, and configured job startup to `main()`, cancelling partial startup jobs before returning an error. +- 2026-08-31 16:09 UTC - GitHub Copilot/User - Approved the specification. Created GitHub issue #2121 with the `task` label and moved this document into `docs/issues/open/`. +- 2026-08-31 16:45 UTC - GitHub Copilot/User - Converted this specification to folder-style layout so issue-local implementation evidence can be added without a later layout migration. +- 2026-08-31 17:13 UTC - GitHub Copilot/User - Opened spec-only PR #2123 for this specification and #2122. It is related to, not an implementation that closes, either issue. + +## Acceptance Criteria + +- [ ] AC1: Configuration-source creation and loading failures return typed errors from `initialize_configuration()` instead of panicking. +- [ ] AC2: Semantic configuration and persistence-requirement validation failures return source-preserving startup errors before global services or application containers are initialized. +- [ ] AC3: Expected configured-driver, migration, and application-container composition failures return contextual typed errors rather than `expect` or an ambiguous `Option`. +- [ ] AC4: Initial persistence-data loading and configured TLS, registration, and listener-start failures return source-preserving startup errors instead of panicking. +- [ ] AC5: `setup()`, `start()`, and `app::run()` propagate typed startup errors; `run()` returns `Ok` only after all configured initial startup work succeeds. +- [ ] AC6: A failure after another initial job has started cancels and joins the partial startup jobs before `run()` returns the error. +- [ ] AC7: The tracker executable and profiling executable report startup failures with context and exit nonzero. +- [ ] AC8: Valid persistence-free and configured-persistence composition behavior from #2107 remains unchanged. +- [ ] AC9: `check_seed()` remains an assertion for its internal invariant, and asynchronous task failures after successful task startup remain outside this task's contract. +- [ ] AC10: Focused tests cover representative source, semantic, requirement, composition, persistence-load, and listener-start failures without starting unrelated services. +- [ ] AC11: `linter all` exits with code `0`, relevant tests pass, manual verification scenarios are executed and documented, and acceptance criteria are re-reviewed against actual behavior. + +## Verification Plan + +Define verification before implementation starts and execute it before closing the issue. + +### Automatic Checks + +- Focused unit tests for configuration loading, bootstrap validation, and fallible container composition. +- Focused application tests that prove a `setup()` failure prevents job startup and listener binding and that a later failure cleans up already-started jobs. +- Focused loader and job-starter tests for database-load, TLS, registration, and listener-start errors. +- Entrypoint/subprocess tests for contextual stderr output and nonzero status where the test harness permits them. +- Regression tests for both persistence-free and configured-persistence composition. +- `cargo fmt`, `linter all`, relevant package tests, and pre-push checks when applicable. + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | -------------------------------- | ----------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- | ------ | --------------------------------------------------------- | +| M1 | Invalid configuration source | Run the tracker with `TORRUST_TRACKER_CONFIG_TOML_PATH` set to a nonexistent file. | The executable reports a contextual configuration-source failure, exits nonzero, and creates no listener. | TODO | {log path and exit status} | +| M2 | Invalid persistence requirements | Run the tracker with a v3 configuration that enables `core.private = true` and omits `core.database`. | The executable reports the typed requirement failure before application composition and exits nonzero. | TODO | {configuration, log path, and exit status} | +| M3 | Unavailable configured listener | Run the tracker with a valid configuration whose configured HTTP or UDP listener cannot bind. | The executable reports the listener-start error, exits nonzero, and stops any previously started jobs. | TODO | {configuration, log path, exit status, and port evidence} | +| M4 | Valid startup regression | Run one documented persistence-free v3 configuration and one configured SQLite v3 configuration. | Both configurations retain #2107's successful startup behavior. | TODO | {commands, logs, and health-check evidence} | + +Notes: + +- Manual verification is mandatory even when automated tests pass. +- If a scenario fails, record the failure and diagnosis in the progress log before proceeding. + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------ | +| AC1 | TODO | {test/log/PR link} | +| AC2 | TODO | {test/log/PR link} | +| AC3 | TODO | {test/log/PR link} | +| AC4 | TODO | {test/log/PR link} | +| AC5 | TODO | {test/log/PR link} | +| AC6 | TODO | {test/log/PR link} | +| AC7 | TODO | {test/log/PR link} | +| AC8 | TODO | {test/log/PR link} | +| AC9 | TODO | {test/log/PR link} | +| AC10 | TODO | {test/log/PR link} | +| AC11 | TODO | {test/log/PR link} | + +## Risks and Trade-offs + +- **Partial startup:** A listener can fail after other jobs have started. Mitigation: make `run()` own partial-startup cancellation and joining before it returns the source-preserving error. +- **Error-boundary breadth:** Recursive startup propagation can accidentally include asynchronous task supervision. Mitigation: the boundary ends when each configured startup operation returns successfully; later task failures remain outside this task. +- **Source fidelity:** Converting lower-layer errors to strings would make callers and tests unable to distinguish source categories. Mitigation: preserve error chains in typed variants through bootstrap and delay formatting until executable reporting. +- **Persistence regression:** Refactoring container initialization can accidentally make valid persistence-free composition fallible. Mitigation: retain #2107 regression coverage for both composition branches. + +## References + +- GitHub issue: #2121 +- Completed prerequisite: #2107 +- Parent EPIC of completed prerequisite: #1978 +- Startup policy: `src/AGENTS.md` +- Configuration bootstrap: `src/bootstrap/config.rs` +- Bootstrap composition: `src/bootstrap/app.rs` +- Application startup: `src/app.rs` +- Executable entrypoint: `src/main.rs` diff --git a/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/ISSUE.md b/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/ISSUE.md new file mode 100644 index 000000000..c58bf3c5b --- /dev/null +++ b/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/ISSUE.md @@ -0,0 +1,266 @@ +--- +doc-type: issue +issue-type: bug +status: open +priority: p2 +epic: null +github-issue: 2122 +spec-path: docs/issues/open/2122-expose-unambiguous-download-counter-semantics/ISSUE.md +branch: "2122-expose-unambiguous-download-counter-semantics" +related-pr: 2123 +depends-on: + - 2107 +last-updated-utc: 2026-08-31 17:13 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - .github/skills/dev/planning/create-issue/SKILL.md + - docs/issues/open/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md + - packages/tracker-core/src/statistics/mod.rs + - packages/tracker-core/src/statistics/repository.rs + - packages/tracker-core/src/statistics/persisted/mod.rs + - packages/tracker-core/src/statistics/event/handler.rs + - packages/tracker-core/tests/integration.rs + - packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs + - packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs + - packages/axum-rest-api-server/src/v1/routes.rs + - packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs + - tests/scaffold.rs + - tests/common/statistics.rs + - docs/issues/open/999-1978-optional-database-configuration/ISSUE.md + - docs/issues/open/2122-expose-unambiguous-download-counter-semantics/manual-verification.md +--- + + + +# Issue #2122 - Expose unambiguous download counter semantics + +## Goal + +Expose separate session and persisted completed-download totals without breaking +v1 consumers. Establish the `in_session` and `persisted` metric naming +convention that API v2 will use as its unambiguous completed-count contract. + +## Background + +`tracker_core_persistent_torrents_downloads_total` is an in-memory counter. It increments for every `PeerDownloadCompleted` event when tracker usage statistics are enabled, including a persistence-free runtime. In that mode it resets when the tracker restarts. + +When persistent completed statistics are enabled, startup restores the global database aggregate into the same counter and the persistent listener updates the database aggregate. It then represents a historical total across restart. + +The session-versus-persistent behavior is intentional and recorded in commit `b0e74439`. The defect is the public metric identifier and description, the repository documentation, and the REST `Stats.completed` documentation: they claim or imply an always-persisted lifetime. The REST response shape does not identify the counter's retention mode. + +The v1 contract is additive-only: existing fields cannot be renamed or removed +before API v2. The v4 tracker release can nevertheless add the unambiguous +fields now, allowing consumers to migrate before v2 removes the legacy +ambiguous field. + +## Scope + +### In Scope + +- Retain the legacy `completed` REST field and + `tracker_core_persistent_torrents_downloads_total` metric identifier and + conditional value semantics for compatibility. Correct their descriptions + and document their deprecation in favor of the new explicit fields/metrics. +- Add v1 REST fields `completed_in_session: u64`, + `completed_persisted: u64`, and `completed_persisted_enabled: bool`. + When persistence is disabled, `completed_persisted` is zero and + `completed_persisted_enabled` is false; clients must use the boolean to + distinguish disabled persistence from an enabled zero count. +- Publish separate `in_session` and `persisted` tracker-core metrics. The + in-session metric has the same availability as tracker usage statistics; the + persisted metric is exposed only when persistent completed statistics are + enabled. Do not expose zero as a Prometheus disabled-state sentinel. +- Record an ADR defining `in_session` for process-lifetime metrics and + `persisted` for metrics restored and maintained in persistent storage. +- Add focused regressions that prove a persistence-free restart resets the exposed total and configured persistence restores it. +- Preserve #2107's independent in-memory and persistence listener topology. + +### Out of Scope + +- Removing, renaming, or changing the value semantics of the legacy v1 + `completed` field. +- Removing, renaming, or changing the conditional value semantics of the + legacy public `tracker_core_persistent_torrents_downloads_total` metric. +- Implementing API v2 or removing deprecated legacy fields and metrics. +- Reworking listener topology, event delivery, database schema, migrations, or persistence configuration. + +## Architectural Decisions + +- Related ADR: `docs/adrs/20260825193119_make_persistence_an_optional_application_composition_capability.md`. +- Related completed work: #2107 established persistence-free runtime behavior and split in-memory completed-count updates from database persistence updates. +- The legacy v1 `completed` field and existing metric retain their current + conditional value to preserve consumers. They are deprecated through their + descriptions and migration documentation in favor of explicit views and are + removed only in API v2. +- `completed_in_session` starts at zero for each tracker process and increments + for every completed-download event processed by the in-memory listener. + `completed_persisted` starts at zero when disabled; when enabled, it is + seeded from the database aggregate and advances after successful persistent + updates. `completed_persisted_enabled` is the authoritative availability + indicator. Consumers must never infer availability from a numeric zero. +- The REST composition root derives `completed_persisted_enabled` from the + validated `persistent_torrent_completed_stat` configuration, rather than + inferring it from a metric value or a composed database service. +- Prometheus uses distinct `in_session` and `persisted` metric identifiers. The + persisted metric is omitted from the exported metric collection when disabled, + so zero remains an unambiguous observed historical count. The legacy metric + stays exported with its current conditional value. +- Create a repository-wide ADR in `docs/adrs/` that defines retention names, + legacy deprecation communication, update ordering, and this additive v1 + bridge. It explicitly refines #999's deferral: a zero persisted field is + permitted only with the separate authoritative availability boolean. + +## Known Refactoring Targets + +These targets are confirmed current behavior and are subject to T1 reconciliation; they are not an exhaustive implementation inventory. + +- `packages/tracker-core/src/statistics/mod.rs`: declare legacy, in-session, + and persisted counter views, with accurate descriptions. +- `packages/tracker-core/src/statistics/repository.rs`: expose named queries + and a capability-aware metric collection for all three views. +- `packages/tracker-core/src/statistics/event/handler.rs` and + `packages/tracker-core/src/statistics/persisted/mod.rs`: update the + independent in-session and persisted views in the defined order. +- `packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs`: add + the three additive v1 fields, document legacy deprecation, and preserve + deserialization compatibility for callers that consume older payloads. +- `packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs` and + `packages/axum-rest-api-server/src/v1/routes.rs`: map the separate values + and inject validated persistence capability at adapter composition. +- `packages/tracker-core/tests/common/test_env.rs` and + `packages/tracker-core/tests/integration.rs`: support persistence-free test + environments and prove both restart contracts. +- `packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs`: + review and extend the existing authenticated `GET /api/v1/stats` contract + coverage for every new field. +- `tests/` and `tests/common/statistics.rs`: review the application-level REST + test harness. Add a focused integration test when endpoint values and metric + presence/absence across both persistence modes cannot be proven by + package-local contract tests. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ---------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Record retention ADR | Add the repository-wide ADR and reconcile it with #999's API-v2 deferral, legacy compatibility, availability, update ordering, and API-v2 removal. | +| T2 | TODO | Separate counter views | Implement legacy, in-session, and persisted tracker-core views; retain listener topology and make persisted metric export capability-aware. | +| T3 | TODO | Extend the v1 stats contract | Add the three fields with backward-compatible deserialization; inject validated persistence capability and map all values through the REST adapter. | +| T4 | TODO | Prove retention regressions | Make the test harness support no persistence; prove the reset, restoration, disabled metric omission, and enabled zero-value cases. | +| T5 | TODO | Review and extend API tests | Review the existing `GET /api/v1/stats` contract test and add direct `GET /api/v1/metrics` endpoint coverage. Add focused `tests/` integration coverage if package-local tests cannot prove configuration, restart, and exported REST behavior together. | +| T6 | TODO | Verify public contract | Run focused tracker-core, REST contract, and any new application integration tests; inspect legacy/new REST fields and legacy/new metrics for enabled and disabled persistence. | +| T7 | TODO | Record local manual evidence | Run the tracker locally for M1-M3 and record exact commands, HTTP requests, redacted responses, configuration, and outcome in `manual-verification.md`. | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Deferred investigation recorded while implementing #2107. +- [x] #2107 completed and the resulting listener topology reviewed. +- [x] Spec drafted in `docs/issues/drafts/`. +- [x] Spec reviewed and approved by user/maintainer. +- [x] GitHub issue #2122 created and issue number added to this spec. +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation. +- [ ] Implementation completed. +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks). +- [ ] Manual verification scenarios executed and recorded (status + evidence). +- [ ] Acceptance criteria reviewed after implementation and updated with evidence. +- [ ] Reviewer validated acceptance criteria and updated checkboxes. +- [ ] Committer verified spec progress is up to date before commit. +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/`. + +### Progress Log + +- 2026-08-28 00:00 UTC - GitHub Copilot - Recorded the counter-semantics defect independently from #2107. +- 2026-08-28 00:00 UTC - GitHub Copilot - Confirmed through public API/export tracing and commit `b0e74439` that the counter is session-scoped without persistence and historical with persistence. The defect is inaccurate public naming/documentation, not retention behavior. +- 2026-08-28 00:00 UTC - GitHub Copilot - Confirmed that tracker usage statistics needs the in-memory counter and selected independent in-memory and persistence listeners. #2107 delivered that topology. +- 2026-08-31 16:13 UTC - GitHub Copilot - Reconciled the investigation with merged #2107 and converted it into a formal bug draft. The explicit metric-identifier compatibility decision precedes implementation. +- 2026-08-31 16:20 UTC - GitHub Copilot/User - Added mandatory local manual verification. The folder-style specification owns `manual-verification.md`, which records commands, requests, redacted responses, configuration, and outcome for M1-M3. +- 2026-08-31 16:45 UTC - GitHub Copilot/User - Reconciled the approved additive v1 bridge with the current counter, REST-adapter, metrics-export, and test-harness boundaries. The final draft defines distinct views and capability behavior, and records the required ADR refinement of #999. +- 2026-08-31 16:48 UTC - GitHub Copilot/User - Created GitHub issue #2122 with the `bug` label and promoted this folder-style specification into `docs/issues/open/`. +- 2026-08-31 16:49 UTC - GitHub Copilot/User - Located the REST API test boundary. The implementation plan requires review of the existing stats endpoint contract coverage and direct metrics endpoint coverage, with a focused `tests/` integration test when package-local coverage cannot prove the configuration-to-endpoint contract. +- 2026-08-31 17:13 UTC - GitHub Copilot/User - Opened spec-only PR #2123 for this specification and #2121. It is related to, not an implementation that closes, either issue. + +## Acceptance Criteria + +- [ ] AC1: The legacy `completed` field and legacy metric identifier retain their conditional value semantics, have accurate descriptions, and are documented as deprecated migration paths to explicit views. +- [ ] AC2: `completed_in_session` resets to zero for every tracker process and increments with every in-memory completed-download event. +- [ ] AC3: With persistent completed statistics enabled, `completed_persisted` is seeded from the database aggregate and advances only after successful database persistence; `completed_persisted_enabled` is true. +- [ ] AC4: With persistent completed statistics disabled, `completed_persisted` is zero, `completed_persisted_enabled` is false, and clients can distinguish this from an enabled zero count only through the boolean. +- [ ] AC5: The in-session metric has the tracker-usage-statistics availability contract; the persisted metric is exported only when persistent completed statistics are enabled; the legacy metric remains exported with legacy semantics. +- [ ] AC6: The REST composition root supplies persistence capability from validated configuration, and the v1 protocol remains backward-compatible for clients deserializing older payloads. +- [ ] AC7: REST server contract tests cover the additive `GET /api/v1/stats` fields and direct `GET /api/v1/metrics` behavior for both persistence modes. +- [ ] AC8: Focused tests prove a persistence-free restart reset, persistence-enabled restoration, enabled zero-value behavior, and persisted-metric omission when disabled without changing #2107's listener topology. Add a `tests/` application integration test when package-local tests cannot prove the configuration-to-endpoint contract. +- [ ] AC9: A repository-wide ADR records the names, lifecycle, compatibility/deprecation policy, and API-v2 migration; it explicitly refines #999's session-versus-historical deferral. +- [ ] AC10: `linter all` exits with code `0`, relevant tests pass, manual verification is documented, and acceptance criteria are re-reviewed against actual behavior. + +## Verification Plan + +Define verification before implementation starts and execute it before closing the issue. + +### Automatic Checks + +- Focused tracker-core tests for independent legacy, in-session, and persisted values and metric descriptions. +- Focused tracker-core integration tests for no-persistence reset, persistence-enabled restoration, and an enabled persisted zero count across a simulated restart. +- Focused export tests proving the persisted metric is absent when disabled and present when enabled. +- Focused REST protocol/runtime-adapter tests for the additive v1 fields, configuration-derived availability, and backward-compatible deserialization. +- Review and extend `packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs` for the authenticated `GET /api/v1/stats` endpoint contract, and add direct authenticated `GET /api/v1/metrics` coverage for JSON and Prometheus output as applicable. +- Add a focused integration test under `tests/` using `TrackerApplicationFixture` when the package-local server environment cannot prove the configuration-to-endpoint behavior across persistence modes and restart. +- `cargo fmt`, `linter all`, relevant package tests, and pre-push checks when applicable. + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ---------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | ------ | --------------------------- | +| M1 | Inspect disabled persistence | Start documented v3 no-persistence configuration, complete a download, inspect stats/metrics, then restart. | `completed_in_session` resets on restart; `completed_persisted` is zero with its boolean false; no persisted metric is exported. | TODO | `manual-verification.md` M1 | +| M2 | Inspect enabled persistence | Start configured SQLite v3 tracker with persistent completed statistics, complete a download, restart using the same database, and inspect endpoints. | The persisted value survives restart with its boolean true; the persisted metric is exported, including when its observed value is zero. | TODO | `manual-verification.md` M2 | +| M3 | Verify legacy migration | Inspect `GET /api/v1/stats` and `GET /api/v1/metrics` in both modes. | Legacy and new names, descriptions, values, and availability match the ADR; legacy values remain compatible. | TODO | `manual-verification.md` M3 | + +Notes: + +- Manual verification is mandatory even when automated tests pass. +- Record exact local commands, HTTP requests, redacted response bodies, HTTP + statuses, configuration, and outcome for M1-M3 in + `manual-verification.md`; do not record tokens, credentials, or other + secrets. +- If a scenario fails, record the failure and diagnosis in the progress log before proceeding. + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------ | +| AC1 | TODO | {test/log/PR link} | +| AC2 | TODO | {test/log/PR link} | +| AC3 | TODO | {test/log/PR link} | +| AC4 | TODO | {test/log/PR link} | +| AC5 | TODO | {test/log/PR link} | +| AC6 | TODO | {test/log/PR link} | +| AC7 | TODO | {test/log/PR link} | +| AC8 | TODO | {test/log/PR link} | +| AC9 | TODO | {test/log/PR link} | +| AC10 | TODO | {test/log/PR link} | + +## Risks and Trade-offs + +- **Metric compatibility:** Existing dashboards may treat the legacy metric as historical. Mitigation: retain its identifier/value semantics, correct its description, and publish a documented migration window before API v2 removal. +- **Cross-view consistency:** Events and database writes are asynchronous. Mitigation: define the persisted-view update after successful database persistence and test eventual values with bounded waits. +- **Disabled-state ambiguity:** A numeric zero can mean no completed downloads or unavailable persistence. Mitigation: REST uses the explicit boolean and Prometheus omits the persisted metric when disabled. +- **REST compatibility:** New required DTO fields can break deserializers of stored or fixture JSON. Mitigation: preserve v1 deserialization compatibility with defaults and test both payload shapes. +- **Test isolation:** Existing tracker-core integration fixtures assume persistence. Mitigation: make their persistence setup conditional before adding no-persistence restart coverage. +- **Endpoint regression:** Repository tests cover the stats endpoint but not the metrics endpoint directly. Mitigation: review that contract suite and require direct metrics coverage plus a top-level integration test when composition-level behavior is not otherwise observable. + +## References + +- Completed prerequisite: #2107 +- Parent EPIC of completed prerequisite: #1978 +- Historical behavior: commit `b0e74439` (`fix: [#1543] return always in API the downloads number from tracker-core`) +- Persistence capability ADR: `docs/adrs/20260825193119_make_persistence_an_optional_application_composition_capability.md` +- Earlier API-v2 deferral: `docs/issues/open/999-1978-optional-database-configuration/ISSUE.md` +- Tracker metric: `packages/tracker-core/src/statistics/mod.rs` +- REST stats adapter: `packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs` +- REST stats protocol: `packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs` diff --git a/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/manual-verification.md b/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/manual-verification.md new file mode 100644 index 000000000..d902aa5bb --- /dev/null +++ b/docs/issues/open/2122-expose-unambiguous-download-counter-semantics/manual-verification.md @@ -0,0 +1,84 @@ +# Manual Verification Evidence + +**Date:** {YYYY-MM-DD HH:MM UTC} +**Tracker revision:** {commit SHA} +**Issue:** #2122 + +## Safety + +Do not record API tokens, passwords, private keys, connection strings, or other +secrets. Replace each secret with `{REDACTED}` in commands, configuration, and +HTTP requests. + +## Local Environment + +- Operating system: {value} +- Tracker command: {exact command} +- Working directory: {path} +- Configuration source: {environment variable or file path} +- Configuration: {redacted TOML or link to an issue-local redacted fixture} + +## M1 - Disabled Persistence + +**Status:** `TODO` + +### Commands + +```text +{exact tracker start, client, HTTP request, and restart commands} +``` + +### Requests And Responses + +```text +{exact HTTP method, redacted URL, request body, HTTP status, and response body} +``` + +### Result + +{Record legacy `completed`, `completed_in_session`, `completed_persisted`, and +`completed_persisted_enabled` before and after restart. Confirm the persisted +Prometheus metric is absent.} + +## M2 - Enabled Persistence + +**Status:** `TODO` + +### Commands + +```text +{exact tracker start, client, HTTP request, and restart commands} +``` + +### Requests And Responses + +```text +{exact HTTP method, redacted URL, request body, HTTP status, and response body} +``` + +### Result + +{Record all legacy and new REST values before and after restart using the same +database. Confirm the persisted metric is exported and document an enabled +zero-value observation when feasible.} + +## M3 - Legacy Migration + +**Status:** `TODO` + +### Commands + +```text +{exact metrics request command} +``` + +### Requests And Responses + +```text +{exact HTTP method, redacted URL, HTTP status, and relevant response body} +``` + +### Result + +{Confirm the observed legacy and new REST fields and metric identifiers, +descriptions, values, and availability match the approved ADR.}