diff --git a/docs/issues/1525-01-persistence-test-coverage.md b/docs/issues/1525-01-persistence-test-coverage.md new file mode 100644 index 000000000..9baf1102e --- /dev/null +++ b/docs/issues/1525-01-persistence-test-coverage.md @@ -0,0 +1,155 @@ +# Subissue Draft for #1525-01: Add DB Compatibility Matrix + +## Goal + +Establish a compatibility matrix that exercises persistence-layer tests across supported database +versions before any refactoring begins. + +## Why First + +The later refactors change persistence architecture, async behavior, schema setup, and backend +implementations. Running the tests against multiple database versions first gives a baseline to +detect regressions early and narrows review scope to behavior rather than guesswork. + +## Scope + +- Bash is acceptable for low-complexity orchestration. +- Focus only on the database compatibility matrix; end-to-end real-client testing is covered by + subissue #1525-02. + +## Testing Principles + +The implementation must follow these quality rules for all new and modified tests. + +- **Isolation**: Each test run must be independent. Tests that spin up database containers via + `testcontainers` already get their own ephemeral container; the bash matrix script achieves + isolation by running one matrix cell at a time in a fresh process, each with an exclusively + allocated container. +- **Independent system resources**: Tests must not hard-code host ports. `testcontainers` binds + containers to random free host ports automatically — do not override this with fixed bindings. + Temporary files or directories, if needed, must be created under a `tempfile`-managed path so + they are always removed on exit. +- **Cleanup**: After each test (success or failure) all containers, volumes, and temporary files + must be released. `testcontainers` handles containers automatically when the handle is dropped; + ensure `Drop` is not suppressed. +- **Behavior, not implementation**: Tests must assert observable outcomes (e.g. the driver + correctly inserts and retrieves a torrent entry) rather than internal state (e.g. a specific SQL + query was issued). +- **Verified before done**: No test is considered complete until it has been executed and passes + in a clean environment. Include confirmation of a passing run in the PR description. + +## Reference QA Workflow + +The PR #1695 review branch includes a QA script that defines the expected behavior: + +- `run-db-compatibility-matrix.sh`: + executes a compatibility matrix across SQLite, multiple MySQL versions, and multiple PostgreSQL + versions. + +This should be treated as a reference prototype, not a production artifact. The goal is to +re-implement it in a form that integrates with the repository's normal test strategy. + +## Dependency Note + +PostgreSQL is not implemented yet, so this subissue cannot require successful execution against +PostgreSQL. The structure should make it easy to add PostgreSQL combinations in subissue +`#1525-08` once the driver exists. + +## Proposed Branch + +- `1525-01-db-compatibility-matrix` + +## Tasks + +### 1) Port the compatibility matrix workflow + +Add a low-complexity bash compatibility-matrix runner that exercises persistence-related tests +across supported database versions. + +Tests to orchestrate: + +- `cargo check --workspace --all-targets` +- configuration coverage for PostgreSQL connection settings +- large-download counter saturation tests in the HTTP protocol layer +- large-download counter saturation tests in the UDP protocol layer +- SQLite driver tests +- MySQL driver tests across selected MySQL versions + +Note: PostgreSQL version-matrix execution is deferred to subissue #1525-08, once the +PostgreSQL driver exists. + +Steps: + +- Modify current DB driver tests so the DB image version can be injected through environment + variables: + - MySQL: `TORRUST_TRACKER_CORE_MYSQL_DRIVER_IMAGE_TAG` + - PostgreSQL (reserved for subissue #1525-08): `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE_TAG` + + When `TORRUST_TRACKER_CORE_MYSQL_DRIVER_IMAGE_TAG` is not set, the test falls back to the + current hardcoded default (e.g. `8.0`), preserving existing behavior. The matrix script sets + this variable explicitly for each version in the loop, so unset means "run as today" and the + matrix just expands that into multiple combinations. + +- Add `contrib/dev-tools/qa/run-db-compatibility-matrix.sh` modeled after the PR prototype: + - `set -euo pipefail` + - define default version sets from env vars: + - `MYSQL_VERSIONS` defaulting to at least `8.0 8.4` + - `POSTGRES_VERSIONS` reserved for subissue #1525-08 + - run pre-checks once (`cargo check --workspace --all-targets`) + - run protocol/configuration tests once + - run SQLite driver tests once + - loop MySQL versions: `docker pull mysql:`, then run MySQL driver tests with + `TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=1` and + `TORRUST_TRACKER_CORE_MYSQL_DRIVER_IMAGE_TAG=` + - print a clear heading for each backend/version before executing tests + - fail fast on first failure with the failing backend/version visible in logs + - keep script complexity intentionally low; avoid re-implementing test logic already in test + functions +- Replace the current single MySQL `database` step in `.github/workflows/testing.yaml` with + execution of the new script. + +Acceptance criteria: + +- [ ] DB image version injection is supported via `TORRUST_TRACKER_CORE_MYSQL_DRIVER_IMAGE_TAG` + (and a reserved `POSTGRES` equivalent for subissue #1525-08). +- [ ] `contrib/dev-tools/qa/run-db-compatibility-matrix.sh` exists and runs successfully. +- [ ] The script exercises SQLite and at least two MySQL versions by default. +- [ ] Failures identify the backend/version combination that broke. +- [ ] The `database` job step in `.github/workflows/testing.yaml` runs the matrix script instead + of a single-version MySQL command. +- [ ] The script structure allows PostgreSQL to be added in subissue #1525-08 without a redesign. +- [ ] Tests do not hard-code host ports; `testcontainers` assigns random ports automatically. +- [ ] All containers started by tests are removed unconditionally on test completion or failure. + +### 2) Document the workflow + +Steps: + +- Document the local invocation command for the matrix script. +- Document that the CI `database` step runs the same script. + +Acceptance criteria: + +- [ ] The matrix script is documented and runnable without ad hoc manual steps. + +## Out of Scope + +- qBittorrent end-to-end testing (covered by subissue #1525-02). +- Adding PostgreSQL support itself. +- Refactoring the production persistence interfaces. +- Performance benchmarking, before/after comparison, and benchmark reporting. + +## Definition of Done + +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. +- [ ] The matrix script has been executed successfully in a clean environment; a passing run log + is included in the PR description. + +## References + +- EPIC: #1525 +- Reference PR: #1695 +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference script: `contrib/dev-tools/qa/run-db-compatibility-matrix.sh` diff --git a/docs/issues/1525-02-qbittorrent-e2e.md b/docs/issues/1525-02-qbittorrent-e2e.md new file mode 100644 index 000000000..447b4ecc9 --- /dev/null +++ b/docs/issues/1525-02-qbittorrent-e2e.md @@ -0,0 +1,184 @@ +# Subissue Draft for #1525-02: Add qBittorrent End-to-End Test + +## Goal + +Add a high-level end-to-end test that validates tracker behavior through a complete torrent-sharing +scenario using real containerized BitTorrent clients, covering scenarios that lower-level unit and +integration tests cannot reach. + +## Why Before the Refactor + +The persistence refactor changes storage behavior underneath the tracker. Having a real-client +scenario that exercises a full download cycle (seeder uploads → leecher downloads → tracker +records completion) gives a regression backstop that is not possible with protocol-level tests +alone. + +## Scope + +- Follow the same pattern as the existing `e2e_tests_runner` binary + (`src/console/ci/e2e/runner.rs`): a Rust binary that drives the whole scenario using + `std::process::Command` to invoke `docker compose` and any container-side commands. +- Use SQLite as the database backend; database compatibility across multiple versions is already + covered by subissue #1525-01. +- Cover one complete scenario: a seeder sharing a torrent that a leecher downloads in full. +- The binary is responsible for scaffolding (generating a temporary config and torrent file), + starting the services, sending commands into the qBittorrent containers (via their WebUI API + or `docker exec`), polling for completion, asserting the result, and tearing down. +- Do not re-test things already covered at a lower level: announce parsing, scrape format, + whitelist/key logic, or multi-database compatibility. + +## Testing Principles + +The implementation must follow these quality rules. + +- **Isolation**: Each run of the E2E binary must be isolated from any other concurrently running + instance. Achieve this by using a unique Docker Compose project name per run (e.g. + `--project-name qbt-e2e-`) so container names, networks, and volumes never + collide with a parallel run. +- **Independent system resources**: Do not bind services to fixed host ports. Let Docker assign + ephemeral host ports and discover them from the compose output, so two simultaneous runs cannot + conflict. Place all temporary files (tracker config, payload, `.torrent` file) in a + `tempfile`-managed directory created at runner start and deleted on exit. +- **Cleanup**: `docker compose down --volumes` must be called unconditionally — on success, on + assertion failure, and on panic. Use a Rust `Drop` guard or equivalent to guarantee teardown + even when the runner exits unexpectedly. +- **Mock time when possible**: Use a configurable timeout (CLI argument or env var) for the + leecher-completion poll rather than a hard-coded sleep. If any logic depends on wall-clock time + (e.g. stale peer detection), inject a mockable clock consistent with the `clock` package used + elsewhere in the codebase. +- **Behavior, not implementation**: Assert the outcome the user cares about — the leecher holds a + complete, byte-identical copy of the payload — not which internal tracker counters changed or + which announce endpoints were called. +- **Verified before done**: The binary must be executed end-to-end and produce a passing result in + a clean environment before the subissue is closed. Include a run log in the PR description. + +## Reference QA Workflow + +`contrib/dev-tools/qa/run-qbittorrent-e2e.py` in the PR #1695 review branch demonstrates the +scenario (seeder + leecher + tracker via Python subprocess). Treat it as a behavioral reference +only; the implementation here will use `docker compose` instead of manual container management. + +## Proposed Branch + +- `1525-02-qbittorrent-e2e` + +## Tasks + +### 1) Add a docker compose file for the E2E scenario + +Add a compose file (e.g., `compose.qbittorrent-e2e.yaml`) that defines: + +- the tracker service configured with SQLite +- a qbittorrent-seeder container +- a qbittorrent-leecher container + +Steps: + +- Define a tracker service mounting a SQLite config file (generated by the runner). +- Define seeder and leecher services using a suitable qBittorrent image. +- Configure a shared network so all containers can reach each other and the tracker. +- Define any volumes needed to mount the payload and torrent file into each client container. +- Ensure `docker compose up --wait` exits cleanly when services are healthy. +- Ensure `docker compose down --volumes` removes all containers and volumes. + +Acceptance criteria: + +- [ ] `docker compose -f compose.qbittorrent-e2e.yaml up --wait` starts all services without error. +- [ ] `docker compose -f compose.qbittorrent-e2e.yaml down --volumes` leaves no orphaned resources. + +### 2) Implement the Rust runner binary + +Add a new binary (e.g., `src/bin/qbittorrent_e2e_runner.rs`) that follows the same structure as +`src/console/ci/e2e/runner.rs`: + +- Parses CLI arguments or environment variables (compose file path, payload size, timeout). +- Generates scaffolding: a temporary tracker config (SQLite) and a small deterministic payload + with its `.torrent` file. +- Calls `docker compose up` via `std::process::Command`. +- Seeds the payload: injects the torrent and payload into the seeder container via the qBittorrent + WebUI REST API (or `docker exec` as a fallback) and starts seeding. +- Leaches the payload: injects the `.torrent` file into the leecher container and starts + downloading. +- Polls for completion: queries the leecher's WebUI API until the torrent state reaches + `uploading` (100 % downloaded) or a timeout expires. +- Asserts payload integrity: compares the downloaded file against the original (hash or byte + comparison). +- Calls `docker compose down --volumes` unconditionally (even on assertion failure), mirroring + the cleanup pattern in `tracker_container.rs`. + +Steps: + +- Add a shared `docker compose` wrapper at `src/console/ci/compose.rs` (see below). This + module is not specific to qBittorrent and is reused by the benchmark runner in subissue + `#1525-03`. +- Add a `qbittorrent` module under `src/console/ci/` (parallel to `e2e/`) containing: + - `runner.rs` — main orchestration logic + - `qbittorrent_client.rs` — HTTP calls to the qBittorrent WebUI API +- **`src/console/ci/compose.rs` wrapper** — mirrors `docker.rs` but targets `docker compose` + subcommands. Design it around a `DockerCompose` struct that holds the compose file path and + project name: + - `DockerCompose::new(file: &Path, project: &str) -> Self` + - `up(&self) -> io::Result<()>` — runs `docker compose -f -p up --wait --detach` + - `down(&self) -> io::Result<()>` — runs `docker compose -f -p down --volumes` + - `port(&self, service: &str, container_port: u16) -> io::Result` — runs + `docker compose -f -p port ` and parses the host port so + the runner never hard-codes ports + - `exec(&self, service: &str, cmd: &[&str]) -> io::Result` — wraps + `docker compose -f -p exec ` for injecting commands into + running containers + - Implement `Drop` on a `RunningCompose` guard returned by `up` that calls `down` + unconditionally, matching the `RunningContainer::drop` pattern in `docker.rs` + - Use `tracing` for progress output consistent with the rest of the runner +- Generate a fixed small payload (e.g., 1 MiB of deterministic bytes) at runtime; store the + `.torrent` file in a `tempfile` directory so it is cleaned up automatically. +- Re-use `tracing` for progress output, consistent with the existing runner. + +Acceptance criteria: + +- [ ] The runner completes a full seeder → leecher download using the containerized tracker. +- [ ] Payload integrity is verified after download (hash or byte comparison). +- [ ] The runner can be executed repeatedly without manual setup or teardown. +- [ ] No orphaned containers or volumes remain on success or failure. +- [ ] The binary is documented in the top-level module doc comment with an example invocation. +- [ ] Each invocation uses a unique compose project name so parallel runs do not conflict. +- [ ] All temporary files are placed in a managed temp directory and deleted on exit. +- [ ] No fixed host ports are used; ports are discovered dynamically from the compose output. +- [ ] `docker compose down --volumes` is called unconditionally via a `Drop` guard. + +### 3) Document the E2E workflow + +Steps: + +- Document the local invocation command (e.g., `cargo run --bin qbittorrent_e2e_runner`). +- Document any prerequisites (Docker, image availability, open ports). +- Clarify that this test is not run in the standard `cargo test` suite due to resource + requirements and describe how it is triggered in CI (opt-in env var or separate job). + +Acceptance criteria: + +- [ ] The test is documented and runnable without ad hoc manual steps. + +## Out of Scope + +- Testing multiple database backends (covered by subissue #1525-01). +- Testing announce or scrape protocol correctness at the protocol level. +- UDP tracker E2E (can be added later without redesigning the compose setup). + +## Definition of Done + +- [ ] `cargo test --workspace --all-targets` passes (or the E2E test is explicitly excluded with a + documented opt-in flag). +- [ ] `linter all` exits with code `0`. +- [ ] The E2E runner has been executed successfully in a clean environment; a passing run log is + included in the PR description. + +## References + +- EPIC: #1525 +- Reference PR: #1695 +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference script: `contrib/dev-tools/qa/run-qbittorrent-e2e.py` +- Existing runner pattern: `src/console/ci/e2e/runner.rs` +- Docker command wrapper: `src/console/ci/e2e/docker.rs` +- Existing container wrapper patterns: `src/console/ci/e2e/tracker_container.rs` diff --git a/docs/issues/1525-03-persistence-benchmarking.md b/docs/issues/1525-03-persistence-benchmarking.md new file mode 100644 index 000000000..d1b3ec32b --- /dev/null +++ b/docs/issues/1525-03-persistence-benchmarking.md @@ -0,0 +1,251 @@ +# Subissue Draft for #1525-03: Add Persistence Benchmarking + +## Goal + +Establish reproducible before/after persistence benchmarks so later refactors can be evaluated +against a concrete performance baseline. + +## Why After Testing + +Correctness comes first. Benchmarking is useful only after the core persistence behaviors are +already covered by tests, otherwise performance comparisons risk masking regressions in behavior. + +## Scope + +- Implement the benchmark runner in Rust (a new binary, consistent with the `e2e_tests_runner` + pattern), following the same docker compose approach used in subissue #1525-02. +- Use one docker compose file per database backend. Each compose file defines the database + container and the tracker container together. The runner launches the compose stack, + discovers the ports, runs the workloads, and tears down. No manual `docker run` calls. +- Run the benchmark against SQLite and MySQL only. PostgreSQL is not available yet; the runner + must be designed so PostgreSQL can be added in subissue #1525-08 without redesign. +- The benchmark compares two tracker Docker images: a `bench-before` image and a `bench-after` + image. The tracker image tag is passed to compose via an environment variable so the runner + can swap it per variant. This allows the same compose files and runner to be re-used after + each subsequent subissue. +- On the first run (this subissue), before and after use the same image built from the current + `develop` HEAD, giving an identical-baseline comparison. The committed report records this. +- Commit the first benchmark report into `docs/benchmarks/` as a baseline reference. Re-run + and update the report in each subsequent subissue that changes persistence behavior. + +## Measurement Tool Rationale + +**Why not Criterion?** `criterion` is a micro-benchmark framework: it runs the same in-process +function thousands of times in a tight loop, applies warm-up phases, and performs statistical +outlier detection for nanosecond-to-millisecond measurements. It is the right tool for the +existing `torrent-repository-benchmarking` crate (in-memory data structures). It is the wrong +tool here because: + +- Each operation involves a real HTTP round-trip to a containerized tracker talking to a real + database. The overhead dwarfs what criterion's sampling model expects. +- We need _aggregate_ metrics across N concurrent workers (ops/sec, p95 latency), not per-call + statistics from a single thread. +- The before/after comparison is across two different Docker images, not across two functions + in the same process — criterion has no model for that. + +**What to use instead**: `std::time::Instant` per-call timing, collected into a `Vec`, +then sorted for percentile extraction. This is exactly what the Python reference script does. +For concurrency, spawn N OS threads via `std::thread::spawn` (one per worker up to +`--concurrency`), each running blocking `reqwest` calls in a loop. Join all threads and +collect their `Duration` measurements into a shared `Vec` for percentile computation. Do +not use `rayon` — its work-stealing pool is designed for CPU-bound tasks and will stall +under I/O-bound HTTP workloads. Output is written as JSON (via `serde_json`) and Markdown. + +## Reference Workflow + +The PR #1695 review branch includes a Python reference: + +- `contrib/dev-tools/qa/run-before-after-db-benchmark.py` + +That script defines the full benchmark approach: it starts a real tracker binary, starts +database containers with free ports, sends HTTP workloads concurrently, collects latency +percentiles and throughput, and prints a before/after comparison. The Rust implementation +must replicate this approach. + +### What the Python script measures + +- **Startup time** — how long the tracker takes to reach `200 OK` on the health endpoint, + measured for both an empty database and a populated database (after the workloads have run). +- **Workloads** (each run sequentially and concurrently): + - `announce_lifecycle` — HTTP `started` announce followed by `completed` announce for each + unique infohash + - `whitelist_add` — REST API `POST /api/v1/whitelist/{info_hash}` + - `whitelist_reload` — REST API `GET /api/v1/whitelist/reload` + - `auth_key_add` — REST API `POST /api/v1/keys` + - `auth_key_reload` — REST API `GET /api/v1/keys/reload` +- **Metrics per workload**: count, total time, ops/sec, mean latency, median latency, p95 + latency, min/max latency. +- **Comparison output**: startup speedup (after/before), ops/s speedup, p95 latency improvement + ratio for each workload × driver combination. + +## Proposed Branch + +- `1525-03-persistence-benchmarking` + +## Testing Principles + +- **Isolation**: Each run uses a unique compose project name (e.g. + `torrust-bench---`) so container names, networks, and volumes + never collide with a parallel invocation. This mirrors the isolation strategy in + subissue #1525-02. +- **Independent system resources**: Do not bind to fixed host ports. Discover the ports + assigned by compose using `docker compose port`. Place all temporary files (SQLite database + file, tracker config, logs) in a `tempfile`-managed directory that is removed on exit. +- **Cleanup**: Use a `RunningCompose` `Drop` guard (from the `DockerCompose` wrapper in + subissue #1525-02) to call `docker compose down --volumes` unconditionally on success, + failure, and panic. +- **Verified before done**: Run the benchmark in a clean environment and include the output in + the PR description alongside the committed report. + +## Tasks + +### 1) Add docker compose files for each database backend + +Add one compose file per database under `contrib/dev-tools/bench/`: + +- `compose.bench-sqlite3.yaml` — tracker service + a volume for the SQLite database file. +- `compose.bench-mysql.yaml` — tracker service + MySQL service. + +Design notes: + +- Parameterize the tracker image tag with an env var (e.g. + `TORRUST_TRACKER_BENCH_IMAGE`, defaulting to `torrust-tracker:bench`) so the runner can + swap before/after images without editing the file. +- Set `TORRUST_TRACKER_CONFIG_TOML` via the compose `environment` key so the runner can inject + a generated config without mounting a file. +- Do not expose fixed host ports in the compose files; expose only the container ports and let + Docker assign ephemeral host ports. The runner discovers them with `docker compose port`. +- Ensure `healthcheck` is defined for each service so `docker compose up --wait` blocks until + everything is ready. + +Acceptance criteria: + +- [ ] `docker compose -f compose.bench-sqlite3.yaml up --wait` starts successfully. +- [ ] `docker compose -f compose.bench-mysql.yaml up --wait` starts successfully. +- [ ] `docker compose -f down --volumes` leaves no orphaned resources. + +### 2) Implement the Rust benchmark runner binary + +Add a new binary `src/bin/persistence_benchmark_runner.rs` following the `e2e_tests_runner` +pattern. Reuse the `DockerCompose` wrapper introduced in subissue #1525-02 at +`src/console/ci/compose.rs`. + +**Dependencies** — add to the workspace `Cargo.toml` and the binary's crate: + +```toml +reqwest = { version = "...", features = ["blocking"] } +serde_json = { version = "..." } +``` + +`rayon` is not needed (see the concurrent workloads approach below). Run `cargo machete` +after to verify no unused dependencies remain. + +**Architecture** — add a module `src/console/ci/bench/` containing: + +- `runner.rs` — main orchestration and CLI argument parsing +- `workloads.rs` — HTTP client calls for each workload (announce, whitelist, auth key) +- `metrics.rs` — `Instant`-based latency collection, sorting, percentile and throughput + computation (no external stats crate needed) +- `report.rs` — JSON (`serde_json`) and Markdown formatting + +**CLI arguments** (mirroring the Python script): + +- `--before-image ` — tracker Docker image for the "before" variant + (default: `torrust-tracker:bench`) +- `--after-image ` — tracker Docker image for the "after" variant + (default: same as `--before-image`) +- `--dbs ` — space/comma-separated list of drivers (default: `sqlite3 mysql`) +- `--mysql-version ` — MySQL Docker image tag (default `8.4`) +- `--ops ` — number of operations per workload (default `200`) +- `--reload-iterations ` — iterations for reload workloads (default `30`) +- `--concurrency ` — worker threads for concurrent workloads (default `16`) +- `--json-output ` — write machine-readable JSON to this path +- `--report-output ` — write the human-readable Markdown report to this path + +**Per-suite lifecycle** (one suite = one `(driver, variant)` pair): + +1. Select the compose file for the driver. +2. Build or tag the tracker image as `TORRUST_TRACKER_BENCH_IMAGE` for this variant. +3. Create a unique compose project name. +4. `DockerCompose::up()` — blocks until all services are healthy. +5. Discover the tracker HTTP, REST API, and health check host ports via + `DockerCompose::port()`. +6. Record `startup_empty_ms` (time from `up` call to first successful health check response). +7. Run a warm-up iteration. +8. Run each workload sequentially then concurrently; collect per-operation `Duration` values. +9. Restart the tracker service only (or call `down` then `up` again) to measure + `startup_populated_ms` against the now-populated database. +10. `DockerCompose::down()` — unconditional, via `Drop` guard. + +**HTTP client**: use `reqwest` (blocking feature) for workload calls. + +**Concurrent workloads**: spawn `--concurrency` OS threads via `std::thread::spawn`, each +running blocking `reqwest` calls in a loop; collect per-thread `Duration` measurements into +a shared `Vec` (via `Arc>>` or join handles). Do not use `rayon` — +its work-stealing pool blocks under I/O-bound workloads. + +Acceptance criteria: + +- [ ] The binary runs successfully against SQLite and MySQL. +- [ ] Startup times (empty and populated) are recorded for each driver. +- [ ] All five workload families are measured sequentially and concurrently. +- [ ] JSON output schema matches the Python reference (`results`, `comparisons` keys). +- [ ] Human-readable Markdown report is produced. +- [ ] All compose stacks are cleaned up unconditionally via `Drop` guards. +- [ ] No hard-coded host ports; all ports are discovered via `docker compose port`. + +### 3) Commit the baseline benchmark report + +After the binary is working: + +- Build a Docker image from the current `develop` HEAD: + `docker build -t torrust-tracker:bench .` +- Run the benchmark with `--before-image torrust-tracker:bench` and + `--after-image torrust-tracker:bench` (both pointing to the same freshly built image, + producing an identical-baseline comparison). +- Save the JSON output to `docs/benchmarks/baseline.json`. +- Save the Markdown report to `docs/benchmarks/baseline.md`. +- Commit both files as part of this subissue's PR. + +Acceptance criteria: + +- [ ] `docs/benchmarks/baseline.json` and `docs/benchmarks/baseline.md` are committed. +- [ ] The Markdown report is readable without tooling and identifies the git revision used. + +### 4) Document the workflow + +Steps: + +- Document how to invoke the benchmark locally. +- Document how to produce an updated report after each subsequent subissue. +- Note that PostgreSQL support will be added to the benchmark in subissue #1525-08. + +Acceptance criteria: + +- [ ] The benchmark is documented and runnable without ad hoc manual steps. + +## Out of Scope + +- PostgreSQL support (reserved for subissue #1525-08). +- Defining hard performance gates for CI. +- Replacing correctness-focused tests. +- The existing `torrent-repository-benchmarking` criterion micro-benchmarks (those measure + in-memory data structures, not the full persistence stack). + +## Definition of Done + +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. +- [ ] The benchmark has been executed successfully; `docs/benchmarks/baseline.md` and + `docs/benchmarks/baseline.json` are committed. +- [ ] A passing run log is included in the PR description. + +## References + +- EPIC: #1525 +- Reference PR: #1695 +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference script: `contrib/dev-tools/qa/run-before-after-db-benchmark.py` +- Docker compose wrapper: `src/console/ci/e2e/docker.rs` (pattern reused for compose wrapper) +- Subissue #1525-02 compose wrapper: `src/console/ci/compose.rs` diff --git a/docs/issues/1525-04-split-persistence-traits.md b/docs/issues/1525-04-split-persistence-traits.md new file mode 100644 index 000000000..284127643 --- /dev/null +++ b/docs/issues/1525-04-split-persistence-traits.md @@ -0,0 +1,281 @@ +# Subissue Draft for #1525-04: Split Persistence Traits by Context + +## Goal + +Decompose the monolithic `Database` trait into four focused context traits while +keeping `Database` as the unified driver contract, and write an ADR to record the +decision. + +## Background + +`packages/tracker-core/src/databases/mod.rs` defines a single `Database` trait with +19 methods covering four unrelated concerns: schema management, torrent metrics, +whitelist, and authentication keys. This makes the trait long and conflates distinct +responsibilities in one place. + +Two options were considered: + +1. **Replace `Database` with four independent traits** — consumers hold + `Arc` etc. directly. Clean interface segregation, but it loses + the single place that tells a new driver implementor exactly what to build, and it + changes every consumer at once. + +2. **Keep `Database` as an aggregate supertrait** (chosen) — the four narrow traits + exist independently; `Database` is defined as: + + ```rust + pub trait Database: + Sync + Send + SchemaMigrator + TorrentMetricsStore + WhitelistStore + AuthKeyStore {} + ``` + + A blanket impl means any type that implements all four narrow traits automatically + satisfies `Database`. Existing consumers (`Arc>`) are untouched. + +This preserves both goals: + +- **One place to discover the full driver contract**: `Database` and its four supertrait + bounds tell a new implementor exactly what to write. +- **Compiler-enforced completeness**: adding a fifth supertrait later causes a compile + error in every driver that does not yet implement it. +- **Interface segregation at the consumer level**: the four narrow traits can be used + directly in tests (`MockWhitelistStore` etc.) and optionally as dependency types once + the MSRV allows trait-object upcasting (stabilised in Rust 1.76; current MSRV is 1.72). + +## Proposed Branch + +- `1525-04-split-persistence-traits` + +## Current State + +The starting point (before this subissue): + +```text +packages/tracker-core/src/databases/ + mod.rs ← Database trait (19 methods, all concerns in one block) + driver/ + mod.rs + sqlite.rs ← impl Database for Sqlite { ... 19 methods ... } + mysql.rs ← impl Database for Mysql { ... 19 methods ... } + error.rs + setup.rs +``` + +The four context groups already exist as doc-comment markers inside the trait +(`# Context: Schema`, `# Context: Torrent Metrics`, etc.) — this subissue makes those +boundaries structural. + +## Target State + +```text +packages/tracker-core/src/databases/ + mod.rs ← module declarations, re-exports + database.rs ← Database aggregate trait + blanket impl + schema.rs ← SchemaMigrator trait + torrent_metrics.rs ← TorrentMetricsStore trait + whitelist.rs ← WhitelistStore trait + auth_keys.rs ← AuthKeyStore trait + driver/ + mod.rs + sqlite.rs ← impl SchemaMigrator + TorrentMetricsStore + + WhitelistStore + AuthKeyStore for Sqlite + mysql.rs ← same for Mysql + error.rs + setup.rs +``` + +## Tasks + +### 1) Write the ADR + +Create `docs/adrs/_keep_database_as_aggregate_supertrait.md` recording: + +- The problem (19-method monolith, unclear per-context boundaries). +- The two options considered (independent traits vs. aggregate supertrait). +- The decision and rationale (aggregate supertrait — see Background above). +- The known constraint: trait-object upcasting from `dyn Database` to a narrow + `dyn XxxStore` requires Rust ≥ 1.76; the MSRV today is 1.72, so consumer wiring + stays as `Arc>` for now. + +Add a row to `docs/adrs/index.md`. + +### 2) Introduce the four narrow traits + +Create one file per trait. Each file contains only that trait's methods, moved verbatim +from `Database` (doc-comments included), plus `#[automock]` for mockall. + +**`databases/schema.rs`** — `SchemaMigrator`: + +```rust +#[automock] +pub trait SchemaMigrator: Sync + Send { + fn create_database_tables(&self) -> Result<(), Error>; + fn drop_database_tables(&self) -> Result<(), Error>; +} +``` + +**`databases/torrent_metrics.rs`** — `TorrentMetricsStore`: + +```rust +#[automock] +pub trait TorrentMetricsStore: Sync + Send { + fn load_all_torrents_downloads(&self) -> Result; + fn load_torrent_downloads(&self, info_hash: &InfoHash) -> Result, Error>; + fn save_torrent_downloads(&self, info_hash: &InfoHash, downloaded: NumberOfDownloads) -> Result<(), Error>; + fn increase_downloads_for_torrent(&self, info_hash: &InfoHash) -> Result<(), Error>; + fn load_global_downloads(&self) -> Result, Error>; + fn save_global_downloads(&self, downloaded: NumberOfDownloads) -> Result<(), Error>; + fn increase_global_downloads(&self) -> Result<(), Error>; +} +``` + +**`databases/whitelist.rs`** — `WhitelistStore`: + +```rust +#[automock] +pub trait WhitelistStore: Sync + Send { + fn load_whitelist(&self) -> Result, Error>; + fn get_info_hash_from_whitelist(&self, info_hash: InfoHash) -> Result, Error>; + fn add_info_hash_to_whitelist(&self, info_hash: InfoHash) -> Result; + fn remove_info_hash_from_whitelist(&self, info_hash: InfoHash) -> Result; + fn is_info_hash_whitelisted(&self, info_hash: InfoHash) -> Result { + Ok(self.get_info_hash_from_whitelist(info_hash)?.is_some()) + } +} +``` + +**`databases/auth_keys.rs`** — `AuthKeyStore`: + +```rust +#[automock] +pub trait AuthKeyStore: Sync + Send { + fn load_keys(&self) -> Result, Error>; + fn get_key_from_keys(&self, key: &Key) -> Result, Error>; + fn add_key_to_keys(&self, auth_key: &authentication::PeerKey) -> Result; + fn remove_key_from_keys(&self, key: &Key) -> Result; +} +``` + +### 3) Introduce the `Database` aggregate trait + +Create `databases/database.rs`: + +```rust +use super::{AuthKeyStore, SchemaMigrator, TorrentMetricsStore, WhitelistStore}; + +/// The full driver contract. +/// +/// A new database driver must implement all four supertrait bounds. The blanket +/// impl below means that any type satisfying all four automatically satisfies +/// `Database` — no separate `impl Database for MyDriver {}` is needed. +/// +/// `Arc>` continues to be the wiring type used by driver +/// setup and consumer repositories. Direct use of the narrow traits as +/// dependency types will become practical once the MSRV reaches 1.76 +/// (trait-object upcasting). +pub trait Database: + Sync + Send + SchemaMigrator + TorrentMetricsStore + WhitelistStore + AuthKeyStore +{ +} + +impl Database for T where + T: Sync + Send + SchemaMigrator + TorrentMetricsStore + WhitelistStore + AuthKeyStore +{ +} +``` + +Remove the `#[automock]` from the old `Database` trait definition — mocking now happens +through the four narrow traits. + +### 4) Update the drivers + +In `driver/sqlite.rs` and `driver/mysql.rs`: + +- Remove `impl Database for { ... }` (the blanket impl replaces it). +- Add four separate `impl` blocks — one per narrow trait — containing the same method + bodies that were previously in the single `impl Database` block. +- No logic changes. This is a mechanical redistribution of existing code. + +Example structure after the change: + +```rust +impl SchemaMigrator for Sqlite { + fn create_database_tables(&self) -> Result<(), Error> { ... } + fn drop_database_tables(&self) -> Result<(), Error> { ... } +} + +impl TorrentMetricsStore for Sqlite { + fn load_all_torrents_downloads(&self) -> Result { ... } + // ... remaining 6 methods +} + +impl WhitelistStore for Sqlite { + // ... 5 methods +} + +impl AuthKeyStore for Sqlite { + // ... 4 methods +} +``` + +If the driver file becomes unwieldy, the four `impl` blocks can be moved into a +`driver/sqlite/` submodule — but that is optional and not required by this subissue. + +### 5) Update `mod.rs` + +- Declare the four new submodules. +- Re-export the traits and the `MockXxx` types so existing `use +crate::databases::Database` imports continue to work. +- Remove the method bodies and imports that were previously inlined in `mod.rs`. + +After the change, `mod.rs` should be a thin index: + +```rust +pub mod auth_keys; +pub mod database; +pub mod driver; +pub mod error; +pub mod schema; +pub mod setup; +pub mod torrent_metrics; +pub mod whitelist; + +pub use auth_keys::{AuthKeyStore, MockAuthKeyStore}; +pub use database::Database; +pub use schema::{MockSchemaMigrator, SchemaMigrator}; +pub use torrent_metrics::{MockTorrentMetricsStore, TorrentMetricsStore}; +pub use whitelist::{MockWhitelistStore, WhitelistStore}; +``` + +## Out of Scope + +- Changing consumer wiring from `Arc>` to narrow trait objects. + That is blocked by the MSRV constraint and is deferred. +- Async trait methods. That is subissue #1525-05. +- Schema migrations. That is subissue #1525-06. +- PostgreSQL support. That is subissue #1525-08. + +## Acceptance Criteria + +- [ ] ADR is written and added to `docs/adrs/index.md`. +- [ ] Four narrow traits exist in separate files under `databases/`. +- [ ] `Database` is an empty aggregate supertrait with a blanket impl. +- [ ] Both drivers (`Sqlite`, `Mysql`) compile through the blanket impl with no manual + `impl Database for ` block. +- [ ] No existing consumer file (`persisted.rs`, `downloads.rs`, etc.) is changed. +- [ ] `#[automock]` is on the four narrow traits; `MockDatabase` is removed. +- [ ] No behavior change — existing tests pass without modification. +- [ ] Persistence benchmarking (see subissue #1525-03) shows no regression against the + committed baseline. +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. + +## References + +- EPIC: #1525 +- Reference PR: #1695 +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- `packages/tracker-core/src/databases/mod.rs` — current monolithic `Database` trait +- `packages/tracker-core/src/whitelist/repository/persisted.rs` — example consumer +- `packages/tracker-core/src/statistics/persisted/downloads.rs` — example consumer +- `packages/tracker-core/src/authentication/key/repository/persisted.rs` — example consumer diff --git a/docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md b/docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md new file mode 100644 index 000000000..5b49a72cb --- /dev/null +++ b/docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md @@ -0,0 +1,280 @@ +# Subissue Draft for #1525-05: Migrate SQLite and MySQL Drivers to sqlx + +## Goal + +Move the existing SQL backends to a shared async `sqlx` substrate before adding PostgreSQL. + +## Why + +PostgreSQL should not be added as a special case. The existing SQL backends need to follow the same +async persistence model first so PostgreSQL can land on a common foundation. + +## Proposed Branch + +- `1525-05-migrate-sqlite-and-mysql-to-sqlx` + +## Background + +### Starting point + +By the time this subissue is implemented, subissue `1525-04` will have split the monolithic +`Database` trait into four narrow sync traits (`SchemaMigrator`, `TorrentMetricsStore`, +`WhitelistStore`, `AuthKeyStore`) plus a `Database` aggregate supertrait with a blanket impl. +Consumers still hold `Arc>`. + +The existing drivers (`Sqlite` in `driver/sqlite.rs`, `Mysql` in `driver/mysql.rs`) use +synchronous connection pools (`r2d2_sqlite`/`r2d2` for SQLite, the `mysql` crate for MySQL). +`build()` in `driver/mod.rs` calls `create_database_tables()` eagerly on startup. + +### Migration strategy: green parallel → single switch commit + +Rewriting both drivers at once while simultaneously making all four traits async would keep the +branch in a broken ("red") state for an extended period. Instead, this subissue uses a +**green parallel approach**: + +1. Build the async infrastructure and new driver implementations alongside the existing sync code + (Tasks 1–3). The branch compiles and all tests pass throughout these tasks. +2. Wire everything up and remove the old code in a single focused switch commit (Task 4). The + branch is briefly in a red state only during this commit. + +The technique is to put the async traits and new drivers in a temporary `databases/sqlx/` +submodule during Tasks 1–3. Task 4 moves them into place, updates consumers, and removes the sync +code. + +### What changes in the drivers + +The current drivers use blocking I/O and create the schema eagerly on construction. The new +`sqlx`-backed drivers: + +- Use `SqlitePool` / `MySqlPool` with lazy `connect_lazy_with()`. +- Manage the schema with raw `sqlx::query()` DDL statements (`CREATE TABLE IF NOT EXISTS ...`), + exactly mirroring what the current sync drivers do. `sqlx::migrate!()` and migration files are + **not** introduced here — that is subissue `1525-06`. +- Run `create_database_tables()` lazily the first time any operation is called, protected by an + `AtomicBool` + `Mutex` double-checked latch (`ensure_schema()`). +- All trait methods become `async fn` (via `async_trait`). + +## Tasks + +### Task 1 — Add sqlx infrastructure (no behavior change, stays green) + +Add the async substrate without touching the existing drivers or traits. + +#### Dependencies + +In `packages/tracker-core/Cargo.toml`, add: + +```toml +async-trait = "..." +sqlx = { version = "...", features = ["sqlite", "mysql", "runtime-tokio-native-tls"] } +tokio = { version = "...", features = ["full"] } # if not already present with needed features +``` + +Keep `r2d2`, `r2d2_sqlite`, `rusqlite`, and the `mysql` crate — they are still needed by the old +drivers until Task 4. + +#### Error handling + +Update `databases/error.rs` so that `sqlx::Error` can be converted into the existing `Error` type. +Add the following constructor methods and their corresponding enum variants. Do not add +`Error::migration_error()` — that belongs to `1525-06`: + +- `Error::connection_error()` — wraps connection failures (`sqlx::Error::Io`, pool errors, + etc.). Introduce the `ConnectionError` variant. +- `Error::invalid_query()` — wraps type-decoding and encoding failures. Used by + `decode_info_hash`, `decode_key`, `decode_valid_until`, and counter conversion helpers in + the async drivers. Also used by the `decode_counter`/`encode_counter` helpers introduced in + `1525-07` — introduce the variant here so `1525-07` requires no additional `error.rs` + changes. Introduce the `InvalidQuery` variant. +- `Error::query_returned_no_rows()` — for `sqlx::Error::RowNotFound`. Introduce the + `QueryReturnedNoRows` variant. +- `From<(sqlx::Error, Driver)>` — maps `sqlx::Error` variants to `ConnectionError`, + `QueryReturnedNoRows`, or `InvalidQuery` based on error kind (see reference `error.rs`). + +Do not change existing variants. + +**Outcome**: `cargo test --workspace --all-targets` still passes. No behavior change. + +### Task 2 — Implement async SQLite driver (stays green) + +Create a new async SQLite driver in a parallel `databases/sqlx/` submodule without touching the +existing `databases/driver/sqlite.rs`. + +#### New files + +```text +packages/tracker-core/src/databases/sqlx/mod.rs ← async trait definitions + AsyncDatabase aggregate +packages/tracker-core/src/databases/sqlx/sqlite.rs ← SqliteSqlx struct +``` + +#### Async trait definitions (`databases/sqlx/mod.rs`) + +Define async versions of the four narrow traits. Use `async_trait` for object safety: + +```rust +use async_trait::async_trait; + +#[async_trait] +pub trait AsyncSchemaMigrator: Send + Sync { + async fn create_database_tables(&self) -> Result<(), Error>; + async fn drop_database_tables(&self) -> Result<(), Error>; +} + +// ... AsyncTorrentMetricsStore, AsyncWhitelistStore, AsyncAuthKeyStore (same method +// signatures as their sync counterparts but with async fn) + +pub trait AsyncDatabase: + AsyncSchemaMigrator + AsyncTorrentMetricsStore + AsyncWhitelistStore + AsyncAuthKeyStore +{ +} + +impl AsyncDatabase for T where + T: AsyncSchemaMigrator + AsyncTorrentMetricsStore + AsyncWhitelistStore + AsyncAuthKeyStore +{ +} +``` + +#### `SqliteSqlx` struct (`databases/sqlx/sqlite.rs`) + +Mirrors the reference `Sqlite` in `driver/sqlite.rs` (PR branch): + +```rust +use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; +use sqlx::SqlitePool; +use std::sync::atomic::{AtomicBool, Ordering}; +use tokio::sync::Mutex; + +pub(crate) struct SqliteSqlx { + pool: SqlitePool, + schema_ready: AtomicBool, + schema_lock: Mutex<()>, +} +``` + +Implement `AsyncSchemaMigrator`, `AsyncTorrentMetricsStore`, `AsyncWhitelistStore`, and +`AsyncAuthKeyStore` for `SqliteSqlx`. All SQL queries use `sqlx::query(...)`. Schema +initialization in `create_database_tables()` executes raw `CREATE TABLE IF NOT EXISTS ...` +statements via `sqlx::query()` — no `sqlx::migrate!()` in this step. + +#### Tests + +Add an inline `#[cfg(test)]` module in `databases/sqlx/sqlite.rs`. Use the shared +`databases/driver/tests::run_tests()` helper (or a new async equivalent) to run all behavioral +tests against `SqliteSqlx`. Use `torrust_tracker_test_helpers::configuration::ephemeral_sqlite_database` +for the in-memory/temp-file path. + +**Outcome**: `cargo test --workspace --all-targets` still passes. Old sync `Sqlite` driver +untouched. + +### Task 3 — Implement async MySQL driver (stays green) + +Create `packages/tracker-core/src/databases/sqlx/mysql.rs` with a `MysqlSqlx` struct mirroring +the same structure as `SqliteSqlx` but using `MySqlPool`. Schema initialization uses raw +`sqlx::query()` DDL — no `sqlx::migrate!()` in this step. + +Implement the same four async traits. Add an inline `#[cfg(test)]` module that runs the shared +behavioral test suite against a real MySQL instance (via environment variable guard +`TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=true`, consistent with existing MySQL test gating). + +**Outcome**: `cargo test --workspace --all-targets` still passes. Old sync `Mysql` driver +untouched. + +### Task 4 — Switch: replace sync traits with async, update consumers (brief red) + +This task is a single focused commit. Steps within the commit: + +1. **Rename async traits to canonical names**: rename `AsyncSchemaMigrator` → `SchemaMigrator`, + `AsyncTorrentMetricsStore` → `TorrentMetricsStore`, etc. in `databases/sqlx/mod.rs`. Rename + `AsyncDatabase` → `Database`. Move the trait definitions from `databases/sqlx/mod.rs` into + `databases/mod.rs` (replacing the sync trait definitions). Move the driver files into the + existing driver directory, overwriting the old sync drivers: + `databases/sqlx/sqlite.rs` → `databases/driver/sqlite.rs` and + `databases/sqlx/mysql.rs` → `databases/driver/mysql.rs`. Remove the now-empty + `databases/sqlx/` submodule. + +2. **Rename driver structs**: rename `SqliteSqlx` → `Sqlite`, `MysqlSqlx` → `Mysql`. + +3. **Clean up old driver module helpers**: remove the sync test helpers from + `databases/driver/mod.rs` that reference `Arc>` with sync methods; replace + with async equivalents. (The old sync driver files at `databases/driver/sqlite.rs` and + `databases/driver/mysql.rs` were already overwritten by the async drivers in step 1.) + +4. **Update `databases/driver/mod.rs` `build()`**: the function no longer calls + `create_database_tables()` eagerly (schema is now lazy). Update the return type if needed. + +5. **Update `databases/setup.rs`**: `initialize_database()` constructs the new async `Sqlite` or + `Mysql` and wraps in `Arc>` (type stays the same, traits are now async). + +6. **Add `.await` at all consumer call sites**: every location that called a `Database` method + synchronously now needs `.await`. The affected files are: + - `statistics/persisted/downloads.rs` (`DatabaseDownloadsMetricRepository`) + - `whitelist/repository/persisted.rs` (`DatabaseWhitelist`) + - `whitelist/setup.rs` + - `authentication/key/repository/persisted.rs` (`DatabaseKeyRepository`) + - `authentication/handler.rs` (test helpers) + - Any integration tests in `tests/` + +7. **Remove unused dependencies**: remove `r2d2`, `r2d2_sqlite`, `rusqlite`, and the `mysql` crate + from `tracker-core/Cargo.toml`. Run `cargo machete` to verify. + +8. **Update mock usage**: `#[automock]` on the narrow traits generates async mocks via `mockall`. + Note that `MockDatabase` was already removed in `1525-04` (the aggregate supertrait has no + methods). The actual breakage surface in this switch commit is the four narrow-trait mocks: + `MockSchemaMigrator`, `MockTorrentMetricsStore`, `MockWhitelistStore`, and `MockAuthKeyStore`. + Any tests written against the **sync** versions of these mocks (from `1525-04`) will fail to + compile after the switch because async `mockall` mocks use + `.returning(|| Box::pin(async { Ok(()) }))` rather than `.returning(|| Ok(()))`. Find and + update all such tests before declaring this task complete. + +**Outcome**: `cargo test --workspace --all-targets` passes. `linter all` exits `0`. Sync drivers +and all `r2d2`/`rusqlite`/`mysql` dependencies are gone. + +## Constraints + +- Do not add PostgreSQL in this step. +- Do not introduce `sqlx::migrate!()`, migration files, or the `sqlx` `macros` feature — those + are introduced in subissue `1525-06`. +- Do not change the SQL schema in this step (schema evolution is `1525-06`). +- Keep `Arc>` as the consumer-facing type; do not introduce the `Persistence` + struct from the reference implementation (that is a separate concern). +- The lazy `ensure_schema()` latch must be correct under concurrent async access: use + `AtomicBool` (Acquire/Release) + `Mutex` double-checked pattern as in the reference. + +## Acceptance Criteria + +- [ ] SQLite and MySQL drivers use `sqlx` with async trait methods. +- [ ] Schema initialization is lazy (`ensure_schema()` pattern) — no eager call in `build()`. +- [ ] Schema management uses raw `sqlx::query()` DDL; `sqlx::migrate!()` is not used. +- [ ] `r2d2`, `r2d2_sqlite`, `rusqlite`, and the `mysql` crate are removed from + `tracker-core/Cargo.toml`. +- [ ] Existing behavior is preserved end-to-end. +- [ ] The branch compiles and all tests pass after each of Tasks 1–3 individually (verified by CI + or manual `cargo test` run after each task). +- [ ] Persistence benchmarking (see subissue `1525-03`) shows no regression against the committed + baseline. +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. +- [ ] `cargo machete` reports no unused dependencies. + +## Out of Scope + +- PostgreSQL driver — that is subissue `1525-08`. +- `sqlx::migrate!()` and migration files — that is subissue `1525-06`. +- `async_trait` removal — the `async_trait` crate is required at MSRV 1.72 because + async-fn-in-traits was stabilized in Rust 1.75. When the MSRV is raised to 1.75+, remove + `async_trait` and replace `#[async_trait]` attribute usage with native async trait syntax. + Track this as a follow-up when the MSRV is next bumped. + +## References + +- EPIC: `#1525` +- Subissue `1525-04`: `docs/issues/1525-04-split-persistence-traits.md` — must be completed first +- Subissue `1525-03`: `docs/issues/1525-03-persistence-benchmarking.md` — benchmark baseline +- Reference PR: `#1695` +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference files (async driver implementations — note: the reference uses `sqlx::migrate!()` + which is not adopted in this step; use raw DDL instead): + - `packages/tracker-core/src/databases/driver/sqlite.rs` + - `packages/tracker-core/src/databases/driver/mysql.rs` + - `packages/tracker-core/src/databases/driver/mod.rs` diff --git a/docs/issues/1525-06-introduce-schema-migrations.md b/docs/issues/1525-06-introduce-schema-migrations.md new file mode 100644 index 000000000..b04ab7942 --- /dev/null +++ b/docs/issues/1525-06-introduce-schema-migrations.md @@ -0,0 +1,429 @@ +# Subissue Draft for #1525-06: Introduce Schema Migrations + +## Goal + +Replace the raw DDL calls in the async drivers with `sqlx`'s versioned migration framework, +making schema evolution explicit, reproducible, and aligned across all SQL backends. + +## Why + +After subissue `1525-05` the drivers still manage their schema through hand-written +`CREATE TABLE IF NOT EXISTS ...` statements executed by `create_database_tables()`. That approach +has no history, no ordering guarantees, and no way to apply incremental schema changes safely to +an existing database. `sqlx::migrate!()` gives us versioned SQL files, automatic up-migration on +startup, and a `_sqlx_migrations` tracking table — a foundation required before PostgreSQL can +be added (subissue `1525-08`). + +## Proposed Branch + +- `1525-06-introduce-schema-migrations` + +## Background + +### Starting point + +By the time this subissue is implemented, subissue `1525-05` will have delivered async SQLite +and MySQL drivers backed by `sqlx`. Each driver has an `ensure_schema()` latch that calls +`create_database_tables()` lazily. That method currently issues raw `sqlx::query()` DDL. This +subissue replaces that raw DDL path with `sqlx::migrate!()`. + +There are already 3 migration files under `packages/tracker-core/migrations/` (both `sqlite/` +and `mysql/` subdirectories) that capture the schema history: + +```text +20240730183000_torrust_tracker_create_all_tables.sql +20240730183500_torrust_tracker_keys_valid_until_nullable.sql +20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql +``` + +These files were written for users to run manually. The tracker has never executed them +automatically. This subissue is the first time they are wired into the application startup path. + +### Current code behavior + +The current `create_database_tables()` method issues `CREATE TABLE IF NOT EXISTS` for all four +tables (`whitelist`, `torrents`, `torrent_aggregate_metrics`, `keys`) using hardcoded DDL that +already reflects the final schema state (nullable `valid_until`, all four tables present). The +current `drop_database_tables()` drops `whitelist`, `torrents`, and `keys` but **not** +`torrent_aggregate_metrics`, which leaks across test drop/create cycles. + +This gives two distinct behaviors today: + +- **New (empty) database**: all four tables are created in the final schema state — equivalent + to having run all three migrations in sequence. The database is immediately usable. +- **Existing database (no `_sqlx_migrations` table)**: `IF NOT EXISTS` silently skips tables + that already exist. Migration 2's `ALTER TABLE` (making `valid_until` nullable) never runs, + so an old `keys` table with `valid_until NOT NULL` stays broken. Migration 3's + `torrent_aggregate_metrics` table is created if absent (it did not exist before migration 3). + The user is expected to run the missing migrations manually, as documented in + `packages/tracker-core/migrations/README.md`. + +### How sqlx migrations work + +`sqlx::migrate!("path/to/migrations")` is a compile-time macro that embeds all `.sql` files +found under the given directory into the binary. At runtime, calling `MIGRATOR.run(&pool)` +applies any unapplied migrations in timestamp order and records them in the `_sqlx_migrations` +tracking table. Each migration is applied exactly once; on subsequent runs its checksum is +verified but it is not re-applied. Migrations are irreversible by default (no down migrations). + +The `macros` feature of `sqlx` is required for the `sqlx::migrate!()` macro. + +Because the migration files are embedded at compile time, the running binary carries all +migrations and does not need the `.sql` files on disk at runtime. No special deployment +packaging is required beyond distributing the binary. + +### Migration file layout + +```text +packages/tracker-core/migrations/ + sqlite/ + 20240730183000_torrust_tracker_create_all_tables.sql + 20240730183500_torrust_tracker_keys_valid_until_nullable.sql + 20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql + mysql/ + 20240730183000_torrust_tracker_create_all_tables.sql + 20240730183500_torrust_tracker_keys_valid_until_nullable.sql + 20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql + postgresql/ ← added in subissue 1525-08; see "PostgreSQL migration alignment" below + ... +``` + +Each backend has its own directory because SQL dialects differ. + +### History-alignment pattern + +All backends must have the **same set of migration filenames** with the same timestamps. When a +schema change is not needed for a specific backend (e.g., a column-type widening that the +backend's native type system already handles), the migration file still exists for that backend +but contains only a comment: + +```sql +-- This migration is intentionally a no-op for this backend. +-- The migration file exists to keep the version history aligned +-- with the other backends. +``` + +This keeps the `_sqlx_migrations` version history identical across backends, which simplifies +reasoning about compatibility and avoids gaps in the timestamp sequence. + +### PostgreSQL migration alignment + +When subissue `1525-08` adds the PostgreSQL driver, its migration directory must contain the +**same set of migration filenames** as SQLite and MySQL, starting from migration 1 — treating +PostgreSQL as if it existed in the project from the beginning. This keeps the +`_sqlx_migrations` version history identical across all three backends. + +Concretely, PostgreSQL's migration 1 creates the original schema (same initial table definitions +as SQLite and MySQL migration 1), and the subsequent migrations apply the same schema changes in +order. Any migration that is a no-op for PostgreSQL follows the history-alignment pattern +(comment-only file) rather than being omitted. + +This means no additional "catch-up" migration is needed when PostgreSQL is added: the full +history starts from migration 1, identical to the other backends. + +### Legacy upgrade path + +When a v4 tracker starts against a database that was managed by an older tracker version, the +`_sqlx_migrations` table will not yet exist. Calling `MIGRATOR.run(&pool)` blindly on such a +database would try to re-apply migration 1 (`CREATE TABLE IF NOT EXISTS ...`) which is harmless +for `whitelist` and `torrents`, but migration 2's `ALTER TABLE` would fail because the +columns it targets are already in their expected state (on a fully-updated old schema) or in an +inconsistent state (on a partially-updated one). + +**Decision: legacy bootstrap with a v4 upgrade pre-condition.** + +The v4 changelog requires that users running an older tracker must apply all three existing +manual migrations before upgrading to v4. Once that pre-condition is met, the driver can +safely detect the legacy state and bootstrap the tracking table automatically: + +1. If `_sqlx_migrations` does **not** exist and the schema tables (`whitelist`, `torrents`, + `keys`, `torrent_aggregate_metrics`) do exist → **legacy bootstrap path**: + - Create the `_sqlx_migrations` table (via `MIGRATOR.ensure_migrations_table(&pool)`). + - Insert fake-applied rows for the three pre-existing migrations (correct versions and + checksums from the embedded `MIGRATOR`), marking them as already executed. + - Call `MIGRATOR.run(&pool)` to apply any migrations added after those three. +2. If `_sqlx_migrations` exists → **normal path**: call `MIGRATOR.run(&pool)` directly; sqlx + skips already-applied migrations. +3. If no tables exist at all → **fresh database path**: `MIGRATOR.run(&pool)` creates + `_sqlx_migrations` and applies all migrations from scratch. + +This logic lives in a helper function called before `MIGRATOR.run(&pool)` inside +`create_database_tables()`. + +### Effect on `ensure_schema()` / `create_database_tables()` + +After this subissue, `SchemaMigrator::create_database_tables()` calls the legacy-bootstrap +helper and then `MIGRATOR.run(&pool)` instead of issuing raw DDL. `drop_database_tables()` +(used only in tests) must also drop the `_sqlx_migrations` and `torrent_aggregate_metrics` +tables (fixing the pre-existing omission) so that the drop/create cycle used in the test suite +works correctly. + +## Tasks + +### Task 1 — Verify existing migration files + +The three migration files already exist under `packages/tracker-core/migrations/`. Verify that +their SQL content is correct and consistent with the current schema produced by the hardcoded +DDL in `1525-05`. Do not change existing file timestamps or names. Fix content only if a +discrepancy is found. + +**Outcome**: all three migration files are verified correct; nothing else changes yet. + +### Task 2 — Enable `sqlx` `macros` feature and add `MIGRATOR` statics + +In `packages/tracker-core/Cargo.toml`, add the `macros` feature to the existing `sqlx` +dependency: + +```toml +sqlx = { version = "...", features = ["sqlite", "mysql", "macros", "runtime-tokio-native-tls"] } +``` + +In each driver file add a static migrator: + +```rust +use sqlx::migrate::Migrator; + +// SQLite driver +static MIGRATOR: Migrator = sqlx::migrate!("migrations/sqlite"); + +// MySQL driver +static MIGRATOR: Migrator = sqlx::migrate!("migrations/mysql"); +``` + +Add `Error::migration_error()` to `databases/error.rs` to wrap `sqlx::migrate::MigrateError`. + +**Outcome**: project compiles with migration statics defined but not yet called. + +### Task 3 — Wire migrations into `create_database_tables()` and `drop_database_tables()` + +#### Legacy bootstrap helper + +Add a private async helper function `bootstrap_legacy_schema` to each driver. This function +detects whether the database is in the legacy state (user-managed schema, no +`_sqlx_migrations` table) and, if so, fake-applies the three pre-existing migrations so that +`MIGRATOR.run()` can continue with only the new ones: + +```rust +async fn bootstrap_legacy_schema(pool: &Pool) -> Result<(), Error> { + // Check whether _sqlx_migrations already exists. + let migrations_table_exists: bool = /* backend-appropriate query */; + if migrations_table_exists { + return Ok(()); // normal path — nothing to do here + } + + // Check whether the legacy tables exist (whitelist is a reliable sentinel). + let legacy_tables_exist: bool = /* backend-appropriate query */; + if !legacy_tables_exist { + return Ok(()); // fresh database — MIGRATOR.run() will handle it + } + + // PRECONDITION GUARD: before fake-applying, verify that migration 2 (nullable + // valid_until) and migration 3 (torrent_aggregate_metrics table) were applied. + // If not, return a descriptive error rather than silently bootstrapping a broken schema. + // SQLite: use `PRAGMA table_info(keys)` and `sqlite_master`. + // MySQL: use `information_schema.columns` and `information_schema.tables`. + let migration_2_applied: bool = /* check keys.valid_until is nullable */; + let migration_3_applied: bool = /* check torrent_aggregate_metrics table exists */; + if !migration_2_applied || !migration_3_applied { + return Err(Error::migration_error( + DRIVER, + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Legacy database is not fully migrated. Apply all three manual migrations \ + listed in packages/tracker-core/migrations/README.md before upgrading to v4.", + ), + )); + } + + // PRECONDITION: all three manual migrations have been verified as applied: + // (1) whitelist/torrents/keys tables exist (whitelist sentinel check above) + // (2) keys.valid_until is nullable (verified above) + // (3) torrent_aggregate_metrics table exists (verified above) + // The v4 upgrade guide requires the user to have applied all three manual migrations + // before upgrading to v4. + MIGRATOR + .ensure_migrations_table(pool) + .await + .map_err(|e| Error::migration_error(DRIVER, e))?; + for migration in MIGRATOR.iter() { + if migration.version <= 20_250_527_093_000 { + // sqlx 0.8 does not expose a public `apply_fake()` API on `Migrator`. + // Fake-apply by inserting directly into `_sqlx_migrations`. The `checksum` + // field MUST equal the value embedded in the compiled binary (from + // `migration.checksum`) so that subsequent `MIGRATOR.run()` calls pass the + // checksum-verification step and do not raise a mismatch error. + // + // The INSERT uses `?` placeholders, valid for both SQLite and MySQL (this + // function lives in the driver-specific file, not in shared code). + sqlx::query( + "INSERT INTO _sqlx_migrations \ + (version, description, installed_on, success, checksum, execution_time) \ + VALUES (?, ?, CURRENT_TIMESTAMP, TRUE, ?, 0)", + ) + .bind(migration.version) + .bind(migration.description.as_ref()) + .bind(migration.checksum.as_ref()) + .execute(pool) + .await + .map_err(|e| Error::migration_error(DRIVER, e))?; + } + } + Ok(()) +} +``` + +#### Updated `create_database_tables()` + +```rust +async fn create_database_tables(&self) -> Result<(), Error> { + bootstrap_legacy_schema(&self.pool).await?; + MIGRATOR.run(&self.pool).await.map_err(|e| Error::migration_error(DRIVER, e))?; + Ok(()) +} +``` + +#### Updated `drop_database_tables()` + +Fix the pre-existing omission: drop `torrent_aggregate_metrics` and `_sqlx_migrations` in +addition to the existing drops so that the test setup cycle (drop → create) works correctly. + +Use `DROP TABLE IF EXISTS` for all five drops. This matches the reference implementation and +is the safer choice for test teardown (avoids errors on a partially torn-down database). + +```rust +// Example using DROP TABLE IF EXISTS for all five drops: +sqlx::query("DROP TABLE IF EXISTS _sqlx_migrations").execute(&self.pool).await...?; +sqlx::query("DROP TABLE IF EXISTS torrent_aggregate_metrics").execute(&self.pool).await...?; +sqlx::query("DROP TABLE IF EXISTS whitelist").execute(&self.pool).await...?; +sqlx::query("DROP TABLE IF EXISTS torrents").execute(&self.pool).await...?; +sqlx::query("DROP TABLE IF EXISTS keys").execute(&self.pool).await...?; +``` + +#### Legacy bootstrap precondition guard + +The `bootstrap_legacy_schema()` helper must verify the critical schema elements before +fake-applying migrations. If any element is absent, it must return an error rather than +silently bootstrapping a broken schema. Add the precondition checks described in the code +block above (migration 2 nullable check and migration 3 table existence check) and document +the verified state with a comment: + +```rust +// PRECONDITION: all three manual migrations have been verified as applied: +// (1) whitelist/torrents/keys tables exist (whitelist sentinel check above) +// (2) keys.valid_until is nullable (verified above) +// (3) torrent_aggregate_metrics table exists (verified above) +// The v4 upgrade guide requires the user to have applied all three manual migrations +// before upgrading to v4. +``` + +#### Update `migrations/README.md` + +Update `packages/tracker-core/migrations/README.md` to replace the stale content (currently: +"We don't support automatic migrations yet") with accurate documentation covering: + +- Migrations are now applied automatically on startup via `sqlx::migrate!()`. +- The `_sqlx_migrations` table tracks which migrations have run. +- To add a new migration: create a `.sql` file with the next timestamp in all applicable backend + directories, following the history-alignment pattern. +- v4 upgrade requirement: users on a pre-v4 tracker must apply all three manual migrations before + upgrading to v4. The automatic bootstrap handles the rest. +- **Migration file immutability**: once a migration file has been deployed, it must never be + modified. `sqlx` records each migration's checksum in `_sqlx_migrations`; editing a committed + migration file causes a checksum-mismatch error on the next startup for any database that has + already applied that migration. + +The `ensure_schema()` latch remains in place — it now guards the +`bootstrap_legacy_schema()` + `MIGRATOR.run()` sequence. + +**Outcome**: `cargo test --workspace --all-targets` passes. Schema is owned by migration files. +The README accurately reflects the new automatic migration behavior. + +### Task 4 — Validate migration behavior + +Add or extend tests that verify: + +- **Fresh database**: a single `create_database_tables()` call runs all migrations and + leaves the database in the correct final schema state. +- **Idempotency**: calling `create_database_tables()` a second time on an already-migrated + database is a no-op (all migrations already recorded in `_sqlx_migrations`). +- **Drop/create cycle**: `drop_database_tables()` followed by `create_database_tables()` + produces a clean schema (all tables including `_sqlx_migrations` and + `torrent_aggregate_metrics` are dropped and recreated). +- **Legacy bootstrap**: a database that has the pre-existing three tables (created without + `_sqlx_migrations`) is correctly bootstrapped — `_sqlx_migrations` is created, the three + migrations are marked fake-applied, and any new migrations are applied. +- **Partial-migration guard**: a database that has the schema tables but is missing + `torrent_aggregate_metrics` (migration 3 not applied) must cause `bootstrap_legacy_schema()` + to return an error, not silently proceed. + +These tests can live alongside the existing behavioral tests in the driver `#[cfg(test)]` +modules. + +## Out of Scope + +- PostgreSQL migration files — those are added in subissue `1525-08`. The + [PostgreSQL migration alignment](#postgresql-migration-alignment) section above specifies + the history-alignment requirement: PostgreSQL must start from migration 1 (not a catch-up + migration) to keep version history identical across all backends. +- Down migrations (rollback) — not needed at this stage. +- Handling legacy databases where not all three manual migrations were applied — the v4 + changelog must state that all three migrations must be applied before upgrading to v4. + The legacy bootstrap path verifies this precondition and returns an error if it is not met + (see the precondition guard above). +- **Migration file integrity check in CI** — `sqlx migrate check` (or an equivalent + step that connects to a fresh database and verifies checksums) can detect if a deployed + migration file has been edited after deployment. This requires a live database in CI and + is a follow-up improvement. It is out of scope here but worth adding once a database + service is reliably available in the CI pipeline (e.g., after subissue `1525-08` wires in + the PostgreSQL service). + +## Acceptance Criteria + +- [ ] The three existing migration files under `migrations/sqlite/` and `migrations/mysql/` are + confirmed correct and match the final schema produced by the hardcoded DDL in `1525-05`. +- [ ] `sqlx::migrate!()` (`macros` feature) is used in both drivers; no raw DDL remains in + `create_database_tables()`. +- [ ] `drop_database_tables()` drops `_sqlx_migrations` **and** `torrent_aggregate_metrics` + (fixing the pre-existing omission) so the test cycle works. All five drops use + `DROP TABLE IF EXISTS`. +- [ ] `bootstrap_legacy_schema()` verifies that migrations 2 and 3 were applied before + fake-applying, and returns a descriptive error if the precondition is not met. +- [ ] `Error::migration_error()` wraps `sqlx::migrate::MigrateError`. +- [ ] `packages/tracker-core/migrations/README.md` is updated to document automatic migration + behavior and the v4 upgrade requirement. +- [ ] Guidance for `1525-08`: PostgreSQL migration files start from migration 1 following the + history-alignment pattern, with the same filenames/timestamps as SQLite and MySQL. +- [ ] Legacy bootstrap: a database with the pre-existing tables but no `_sqlx_migrations` is + correctly detected; the three pre-existing migrations are fake-applied; new migrations + run normally. +- [ ] Fresh database: `create_database_tables()` runs all migrations from scratch via + `MIGRATOR.run()`. +- [ ] Migration idempotency is verified by tests (second call is a no-op). +- [ ] Drop/create cycle is verified by tests (all tables cleaned up and recreated). +- [ ] Legacy bootstrap scenario is verified by a test (fully-migrated legacy database is + bootstrapped correctly). +- [ ] Partial-migration guard is verified by a test (database missing `torrent_aggregate_metrics` + causes an error rather than silent bootstrap). +- [ ] Existing behavioral tests continue to pass. +- [ ] The v4 changelog or upgrade guide documents the pre-upgrade requirement: apply all three + manual migrations before upgrading to v4. +- [ ] Persistence benchmarking (see subissue `1525-03`) shows no regression against the committed + baseline. +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. + +## References + +- EPIC: `#1525` +- Subissue `1525-05`: `docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md` — must be + completed first +- Subissue `1525-03`: `docs/issues/1525-03-persistence-benchmarking.md` — benchmark baseline +- Reference PR: `#1695` +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference files (migration files and driver wiring): + - `packages/tracker-core/migrations/sqlite/` + - `packages/tracker-core/migrations/mysql/` + - `packages/tracker-core/src/databases/driver/sqlite.rs` + - `packages/tracker-core/src/databases/driver/mysql.rs` +- Existing migration README: `packages/tracker-core/migrations/README.md` diff --git a/docs/issues/1525-07-align-rust-and-db-types.md b/docs/issues/1525-07-align-rust-and-db-types.md new file mode 100644 index 000000000..9b869af34 --- /dev/null +++ b/docs/issues/1525-07-align-rust-and-db-types.md @@ -0,0 +1,228 @@ +# Subissue 1525-07: Align Rust and Database Types + +## Goal + +Widen the download-counter type in Rust from `u32` to `u64` and widen the corresponding +database columns from `INTEGER` (32-bit, MySQL) to `BIGINT` (64-bit), delivered as a versioned +`sqlx` migration so the change is explicit, testable, and tracked as a forward schema change. + +## Background + +### Current state + +By the time this subissue is implemented, subissue `1525-06` will have wired `sqlx::migrate!()` +into both drivers. The schema at that point contains: + +- `torrents.completed` — `INTEGER` in MySQL (32-bit signed, max ≈ 2.1 billion), `INTEGER` in + SQLite (storage is already 64-bit for any integer value). +- `torrent_aggregate_metrics.value` — same types as above. + +The Rust type alias is `NumberOfDownloads = u32` in +`packages/primitives/src/lib.rs`. The `SwarmMetadata.downloaded` field also uses this type. +The drivers read the column as `i64` (sqlx always returns integer columns as `i64`) and +immediately narrow-cast to `u32`. + +### Why this is a problem + +The MySQL `INT` column type is **signed 32-bit** (max 2,147,483,647). Writing a `u32` value +above that limit silently overflows or errors. Practically, the counter saturates at the same +point as the UDP scrape wire format (`completed` is `i32` in BEP 15), but the correct fix is +to widen the storage type rather than rely on implicit saturation in the driver. + +`u32::MAX` (4,294,967,295) is already higher than the `i32::MAX` wire limit, so protocol +saturation happens before storage overflow today. However, aligning storage to `BIGINT` and the +Rust type to `u64` makes the storage contract explicit and decoupled from any particular +protocol encoding. Future protocol changes or a direct-database query tool cannot accidentally +exceed a silently-constrained column. + +**Protocol encoding** (read-only, no changes needed in this subissue): + +- UDP scrape response (`i32` wire field): the existing conversion from `NumberOfDownloads` to + `i32` already saturates at `i32::MAX`. This remains unchanged. +- HTTP scrape response (bencoded `i64`): `bencode_download_count()` saturates at `i64::MAX`. + This remains unchanged. + +### Why migrations first (1525-06 before 1525-07) + +The column-widening change must be delivered as a versioned migration rather than an ad hoc DDL +update. Having the migration framework from `1525-06` in place ensures the change is tracked in +`_sqlx_migrations`, tested like any other migration, and can be reasoned about in production +upgrade scenarios. + +## Proposed Branch + +- `1525-07-align-rust-and-db-types` + +## What Changes + +### Migration files + +Add the fourth migration to both existing backends: + +```text +packages/tracker-core/migrations/sqlite/20260409120000_torrust_tracker_widen_download_counters.sql +packages/tracker-core/migrations/mysql/20260409120000_torrust_tracker_widen_download_counters.sql +``` + +**SQLite** — no-op (SQLite already stores any `INTEGER` value as a 64-bit signed integer): + +```sql +-- SQLite stores INTEGER values as signed 64-bit integers already. +-- This migration is intentionally a no-op so the migration history stays +-- aligned with the MySQL backend. +``` + +**MySQL** — widen both download-counter columns: + +```sql +ALTER TABLE torrents + MODIFY completed BIGINT NOT NULL DEFAULT 0; + +ALTER TABLE torrent_aggregate_metrics + MODIFY value BIGINT NOT NULL DEFAULT 0; +``` + +PostgreSQL migration files are not created here. They will be added in subissue `1525-08` when +the PostgreSQL driver is introduced. Following the +[history-alignment pattern](1525-06-introduce-schema-migrations.md#history-alignment-pattern) +established in `1525-06`, subissue `1525-08` creates **all four** migration files for +PostgreSQL starting from migration 1. PostgreSQL's migration 1 creates the columns as +`INTEGER` (matching the original schema from the other backends), and migration 4 widens them +to `BIGINT` using PostgreSQL-specific `ALTER COLUMN ... TYPE BIGINT` syntax. Migration 4 is +not a no-op for PostgreSQL. + +### Rust type changes + +**`packages/primitives/src/lib.rs`** — widen the type alias: + +```rust +// Before +pub type NumberOfDownloads = u32; + +// After +pub type NumberOfDownloads = u64; +``` + +**`packages/primitives/src/swarm_metadata.rs`** — `downloaded` field currently uses the bare +`u32`. Update it to use `NumberOfDownloads` explicitly: + +```rust +// Before +pub downloaded: u32, + +// After +pub downloaded: NumberOfDownloads, +``` + +Also update the `downloads()` method return type to `NumberOfDownloads`. + +### Driver conversion changes + +After `1525-05`, the sqlx drivers read counter columns as `i64`. With `NumberOfDownloads = u32` +the read path does `u32::try_from(i64_value)`. After this subissue it becomes +`u64::try_from(i64_value)`. + +Because the database column type is `BIGINT` (signed), the **write path** must also encode +`u64 → i64`. Values above `i64::MAX` (≈ 9.2 × 10¹⁸) cannot be stored and must return an +error rather than silently truncate. Add named helper methods to each driver to make the +conversion explicit and consistent: + +```rust +fn decode_counter(value: i64) -> Result { + u64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) +} + +fn encode_counter(value: NumberOfDownloads) -> Result { + i64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) +} +``` + +Use these helpers in every place a counter column is read from or written to the database. + +### Cascade compilation fixes + +Widening `NumberOfDownloads` from `u32` to `u64` will produce compilation errors wherever the +old `u32` range was assumed. Fix all errors; do not add `as u32` casts or `allow` attributes +to suppress them. + +## Tasks + +### Task 1 — Add migration files + +Create the two new migration files listed above. Do not modify any existing migration file. + +**Outcome**: `packages/tracker-core/migrations/` has four files in each of `sqlite/` and +`mysql/`. The fourth file is verified by running the migration against a fresh test database +of each type. + +### Task 2 — Widen `NumberOfDownloads` and fix cascade + +Change `NumberOfDownloads = u32 → u64` in `packages/primitives/src/lib.rs` and update +`SwarmMetadata.downloaded` to use the alias. Fix all resulting compilation errors across the +workspace (driver conversion logic, scrape response encoding, announce handler arithmetic, +etc.). + +Add `decode_counter` / `encode_counter` helpers to both driver files as described above. + +**Outcome**: `cargo build --workspace` succeeds with no warnings or errors. + +### Task 3 — Validate migration and type alignment + +Add or extend tests that verify: + +- **MySQL migration**: running the migration on a database with the pre-migration `INT` column + produces a `BIGINT` column, and writing and reading a value larger than `2^31 − 1` round-trips + correctly. +- **SQLite no-op**: the migration applies cleanly (recorded in `_sqlx_migrations`) and the + column already accepts large values. +- **Boundary encode**: writing a `u64` counter value of exactly `i64::MAX` succeeds; writing + `i64::MAX + 1` returns an appropriate error rather than panicking or wrapping. + +These tests extend the existing driver `#[cfg(test)]` modules. + +**Outcome**: `cargo test --workspace --all-targets` passes. + +## Out of Scope + +- PostgreSQL migration files — added in subissue `1525-08`. +- Down migrations (rollback) — not needed at this stage. +- Trait splitting or other structural refactoring. +- Other numeric types beyond `NumberOfDownloads` / download counters. + +## Acceptance Criteria + +- [ ] `packages/tracker-core/migrations/sqlite/20260409120000_torrust_tracker_widen_download_counters.sql` + exists and is a comment-only no-op. +- [ ] `packages/tracker-core/migrations/mysql/20260409120000_torrust_tracker_widen_download_counters.sql` + exists and widens `torrents.completed` and `torrent_aggregate_metrics.value` to `BIGINT`. +- [ ] `NumberOfDownloads = u64` in `packages/primitives/src/lib.rs`. +- [ ] `SwarmMetadata.downloaded` uses `NumberOfDownloads`; bare `u32` is removed from that field. +- [ ] Both driver files use explicit `decode_counter` / `encode_counter` helpers for all + counter-column reads and writes. +- [ ] `encode_counter` returns an error (not a panic, not silent truncation) for values + above `i64::MAX`. +- [ ] A test verifies round-trip of a value larger than `u32::MAX` for each backend. +- [ ] A test verifies the encode error path for values above `i64::MAX`. +- [ ] No `as u32` casts or compiler-suppression attributes introduced by this subissue. +- [ ] Persistence benchmarking (see subissue `1525-03`) shows no regression against the + committed baseline. +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `linter all` exits with code `0`. + +## References + +- EPIC: `#1525` +- Subissue `1525-06`: `docs/issues/1525-06-introduce-schema-migrations.md` — must be completed + first (provides the migration framework) +- Subissue `1525-08`: `docs/issues/1525-08-add-postgresql-driver.md` — adds PostgreSQL + migration files including the history-aligned no-op for this migration +- Subissue `1525-03`: `docs/issues/1525-03-persistence-benchmarking.md` — benchmark baseline +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions (`docs/issues/1525-overhaul-persistence.md`) +- Reference files: + - `packages/tracker-core/migrations/sqlite/20260409120000_torrust_tracker_widen_download_counters.sql` + - `packages/tracker-core/migrations/mysql/20260409120000_torrust_tracker_widen_download_counters.sql` + - `packages/primitives/src/lib.rs` (type alias change) + - `packages/primitives/src/swarm_metadata.rs` (field type change) + - `packages/tracker-core/src/databases/driver/sqlite.rs` (decode/encode helpers) + - `packages/tracker-core/src/databases/driver/mysql.rs` (decode/encode helpers) diff --git a/docs/issues/1525-08-add-postgresql-driver.md b/docs/issues/1525-08-add-postgresql-driver.md new file mode 100644 index 000000000..4b2123564 --- /dev/null +++ b/docs/issues/1525-08-add-postgresql-driver.md @@ -0,0 +1,790 @@ +# Subissue 1525-08: Add PostgreSQL Driver + +## Goal + +Add PostgreSQL as a third production SQL backend by implementing an async `sqlx`-backed +driver, wiring it into the configuration and factory, creating all four migration files +(starting from migration 1, history-aligned with SQLite and MySQL), and extending the +existing QA harnesses so PostgreSQL receives the same test coverage as the other backends. + +## Why Last + +PostgreSQL is the feature goal of the EPIC, but adding it first would have meant building on +an ad hoc, sync, pre-migration foundation. By the time this subissue is implemented, the +persistence layer is async (`1525-05`), schema-managed (`1525-06`), and correctly typed +(`1525-07`). PostgreSQL can now land as a first-class backend with no special-casing. + +## Proposed Branch + +- `1525-08-add-postgresql-driver` + +## Background + +### Starting point + +By the time this subissue is implemented: + +- **1525-04** has split the monolithic `Database` trait into four narrow context traits + (`SchemaMigrator`, `TorrentMetricsStore`, `WhitelistStore`, `AuthKeyStore`) plus a blanket + `Database` aggregate supertrait. Both existing drivers (`Sqlite`, `Mysql`) satisfy `Database` + through the blanket impl. Consumers hold `Arc>`. + +- **1525-05** has moved SQLite and MySQL to async `sqlx` connection pools. `r2d2`, `r2d2_sqlite`, + `rusqlite`, and the `mysql` crate are gone. The `sqlx` dependency has `sqlite` and `mysql` + features but not yet `postgres`. + +- **1525-06** has replaced the raw DDL in `create_database_tables()` with `sqlx::migrate!()`. + Each driver has a `static MIGRATOR` pointing to its backend-specific migration directory and + a `bootstrap_legacy_schema()` helper for upgrading pre-v4 databases. Both backends have three + migration files. + +- **1525-07** has widened `NumberOfDownloads` from `u32` to `u64`, added a fourth migration to + SQLite and MySQL, and added `decode_counter`/`encode_counter` helpers to both drivers. The + migration file layout at the end of `1525-07` is: + + ```text + packages/tracker-core/migrations/ + sqlite/ + 20240730183000_torrust_tracker_create_all_tables.sql + 20240730183500_torrust_tracker_keys_valid_until_nullable.sql + 20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql + 20260409120000_torrust_tracker_widen_download_counters.sql + mysql/ + 20240730183000_torrust_tracker_create_all_tables.sql + 20240730183500_torrust_tracker_keys_valid_until_nullable.sql + 20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql + 20260409120000_torrust_tracker_widen_download_counters.sql + ``` + + No `postgresql/` directory exists yet. + +### Driver enum locations + +Two separate `Driver` enums exist and both must be extended: + +- **Configuration** — `packages/configuration/src/v2_0_0/database.rs`: user-facing config + file value. Holds `Sqlite3`, `MySQL`. Used by the tracker to select which driver to build. +- **Databases factory** — `packages/tracker-core/src/databases/driver/mod.rs`: internal + dispatch enum. Holds `Sqlite3`, `MySQL`. `build()` matches on this to construct the driver. + `databases/setup.rs` converts from the configuration enum to this internal enum. + +### No legacy bootstrap for PostgreSQL + +The `bootstrap_legacy_schema()` helper introduced in `1525-06` exists to upgrade databases +that were managed manually before v4. PostgreSQL was never supported before this subissue, so +no pre-existing PostgreSQL tracker databases exist. The PostgreSQL `create_database_tables()` +implementation skips the legacy bootstrap and calls `MIGRATOR.run()` directly. + +### Connection string format + +PostgreSQL uses the same `path` field as MySQL in the configuration — a single URL string: + +```toml +[core.database] +driver = "postgresql" +path = "postgresql://user:password@host:port/dbname" +``` + +The `mask_secrets()` function in the configuration package must be extended to parse and +redact the password from this URL, mirroring the existing MySQL URL masking logic. + +### Database pre-creation requirement + +Unlike SQLite (which creates its file on first connection), PostgreSQL requires the target +database to already exist before `sqlx` can connect. The `torrust_tracker` database referenced +in the connection URL must be created before the tracker starts: + +```sql +CREATE DATABASE torrust_tracker; +``` + +**Test containers**: the `PostgresConfiguration.database` field (`torrust_tracker_test` by +default) is passed as the `POSTGRES_DB` env var to the PostgreSQL container. The official +`postgres` Docker image creates this database automatically — no manual `CREATE DATABASE` +call is needed in test code. + +**Container config** (`tracker.container.postgresql.toml`): the URL points to +`postgresql://postgres:postgres@postgres:5432/torrust_tracker`. The accompanying compose file +or deployment guide must ensure the `torrust_tracker` database exists — either by setting +`POSTGRES_DB=torrust_tracker` on the PostgreSQL service, or by running a setup step before the +tracker starts. Without it, the tracker will exit on startup with a `sqlx` connection error +that does not clearly identify the missing database as the cause. + +## What Changes + +### Migration files + +Create a `postgresql/` directory under `packages/tracker-core/migrations/` with all four +migration files. The timestamps are shared with the SQLite and MySQL backends, keeping the +`_sqlx_migrations` version history identical across all three backends. Migration 4 is **not** +a no-op for PostgreSQL — PostgreSQL's migration 1 creates the columns as `INTEGER` (matching +the other backends at their migration-1 state), and migration 4 widens them to `BIGINT` using +PostgreSQL-specific `ALTER COLUMN` syntax. + +**`20240730183000_torrust_tracker_create_all_tables.sql`**: + +```sql +CREATE TABLE IF NOT EXISTS whitelist ( + id SERIAL PRIMARY KEY, + info_hash VARCHAR(40) NOT NULL UNIQUE +); + +CREATE TABLE IF NOT EXISTS torrents ( + id SERIAL PRIMARY KEY, + info_hash VARCHAR(40) NOT NULL UNIQUE, + completed INTEGER DEFAULT 0 NOT NULL +); + +CREATE TABLE IF NOT EXISTS keys ( + id SERIAL PRIMARY KEY, + key VARCHAR(32) NOT NULL UNIQUE, + valid_until INTEGER NOT NULL +); +``` + +PostgreSQL differences from MySQL and SQLite: `SERIAL` instead of `AUTO_INCREMENT` or +`INTEGER PRIMARY KEY AUTOINCREMENT`; no backtick quoting; parameter placeholders are `$1`, +`$2`, … in DML queries (not `?`). + +**`20240730183500_torrust_tracker_keys_valid_until_nullable.sql`**: + +```sql +ALTER TABLE keys ALTER COLUMN valid_until DROP NOT NULL; +``` + +**`20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql`**: + +```sql +CREATE TABLE IF NOT EXISTS torrent_aggregate_metrics ( + id SERIAL PRIMARY KEY, + metric_name VARCHAR(50) NOT NULL UNIQUE, + value INTEGER DEFAULT 0 NOT NULL +); +``` + +**`20260409120000_torrust_tracker_widen_download_counters.sql`**: + +```sql +ALTER TABLE torrents + ALTER COLUMN completed TYPE BIGINT, + ALTER COLUMN completed SET DEFAULT 0, + ALTER COLUMN completed SET NOT NULL; + +ALTER TABLE torrent_aggregate_metrics + ALTER COLUMN value TYPE BIGINT, + ALTER COLUMN value SET DEFAULT 0, + ALTER COLUMN value SET NOT NULL; +``` + +### Configuration package + +In `packages/configuration/src/v2_0_0/database.rs`: + +- Add `PostgreSQL` variant to the `Driver` enum: + + ```rust + #[derive(Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Debug, Hash, Clone)] + #[serde(rename_all = "lowercase")] + pub enum Driver { + Sqlite3, + MySQL, + PostgreSQL, // new + } + ``` + +- Extend `mask_secrets()` to handle the PostgreSQL URL. MySQL and PostgreSQL both use a URL + `path`; the masking code can share a branch: + + ```rust + Driver::MySQL | Driver::PostgreSQL => { + let mut url = Url::parse(&self.path)?; + url.set_password(Some("***")).ok(); + self.path = url.to_string(); + } + ``` + +- Add a test: + + ```rust + fn it_should_allow_masking_the_postgresql_user_password() + ``` + +### `tracker-core` Cargo.toml + +Add `"postgres"` to the `sqlx` features list: + +```toml +sqlx = { version = "...", features = [ + "sqlite", "mysql", "postgres", "macros", "runtime-tokio-native-tls" +] } +``` + +### PostgreSQL driver + +New file: `packages/tracker-core/src/databases/driver/postgres.rs`. + +**Driver struct and constructor**: + +```rust +use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; +use sqlx::{ConnectOptions, PgPool, Row}; +use std::sync::atomic::{AtomicBool, Ordering}; +use tokio::sync::Mutex; + +const DRIVER: &str = "postgresql"; + +static MIGRATOR: Migrator = sqlx::migrate!("migrations/postgresql"); + +pub(crate) struct Postgres { + pool: PgPool, + schema_ready: AtomicBool, + schema_lock: Mutex<()>, +} + +impl Postgres { + pub fn new(db_path: &str) -> Result { + let options = db_path + .parse::() + .map_err(|e| Error::connection_error(DRIVER, e))? + .disable_statement_logging(); + let pool = PgPoolOptions::new().connect_lazy_with(options); + Ok(Self { + pool, + schema_ready: AtomicBool::new(false), + schema_lock: Mutex::new(()), + }) + } +} +``` + +**Lazy migration latch** (same double-checked pattern as SQLite and MySQL): + +```rust +async fn ensure_schema(&self) -> Result<(), Error> { + if self.schema_ready.load(Ordering::Acquire) { + return Ok(()); + } + let _guard = self.schema_lock.lock().await; + if self.schema_ready.load(Ordering::Acquire) { + return Ok(()); + } + self.create_database_tables().await?; + self.schema_ready.store(true, Ordering::Release); + Ok(()) +} +``` + +**`SchemaMigrator` implementation**: + +`create_database_tables()` skips the legacy bootstrap (PostgreSQL has no pre-v4 databases) +and calls `MIGRATOR.run()` directly: + +```rust +async fn create_database_tables(&self) -> Result<(), Error> { + // PostgreSQL is a new backend — no legacy databases exist without _sqlx_migrations. + // MIGRATOR.run() always takes the fresh-database path. + MIGRATOR + .run(&self.pool) + .await + .map_err(|e| Error::migration_error(DRIVER, e))?; + Ok(()) +} +``` + +`drop_database_tables()` drops all five tables including `_sqlx_migrations` so the +drop/create cycle used in the test suite works correctly. Use `DROP TABLE IF EXISTS` +consistently for all drops, matching the style established in `1525-06`: + +```rust +async fn drop_database_tables(&self) -> Result<(), Error> { + sqlx::query("DROP TABLE IF EXISTS _sqlx_migrations") + .execute(&self.pool).await?; + sqlx::query("DROP TABLE IF EXISTS torrent_aggregate_metrics") + .execute(&self.pool).await?; + sqlx::query("DROP TABLE IF EXISTS whitelist") + .execute(&self.pool).await?; + sqlx::query("DROP TABLE IF EXISTS torrents") + .execute(&self.pool).await?; + sqlx::query("DROP TABLE IF EXISTS keys") + .execute(&self.pool).await?; + Ok(()) +} +``` + +**SQL syntax differences from SQLite and MySQL**: + +| Aspect | SQLite / MySQL | PostgreSQL | +| --------------------- | ----------------------------------------------------------------- | ---------------------------------------------------- | +| Parameter placeholder | `?` | `$1`, `$2`, … | +| Upsert | `ON DUPLICATE KEY UPDATE` (MySQL) or `INSERT OR REPLACE` (SQLite) | `ON CONFLICT (col) DO UPDATE SET col = EXCLUDED.col` | +| Auto-increment (DDL) | `AUTO_INCREMENT` / `AUTOINCREMENT` | `SERIAL` (in migration files only) | + +**Counter encode/decode helpers** (identical contract to SQLite and MySQL): + +```rust +fn decode_counter(value: i64) -> Result { + u64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) +} + +fn encode_counter(value: NumberOfDownloads) -> Result { + i64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) +} +``` + +Use these helpers in every place a counter column is read from or written to the database. +Do not use bare `as i64` casts or `as u64` casts. + +**`TorrentMetricsStore`, `WhitelistStore`, `AuthKeyStore` implementations**: Follow the same +structure as the SQLite and MySQL drivers, substituting `$1`/`$2` placeholders and the +PostgreSQL upsert syntax. There are no behavior differences relative to the other backends. + +### Driver factory + +In `packages/tracker-core/src/databases/driver/mod.rs`: + +- Add `PostgreSQL` variant to the `Driver` enum. +- Add a `pub mod postgres;` declaration. +- Add a match arm in `build()`: + + ```rust + Driver::PostgreSQL => { + let backend = Postgres::new(db_path)?; + Ok(Arc::new(Box::new(backend) as Box)) + } + ``` + +### Database setup + +In `packages/tracker-core/src/databases/setup.rs`, extend the configuration-to-internal +driver enum conversion: + +```rust +torrust_tracker_configuration::Driver::PostgreSQL => Driver::PostgreSQL, +``` + +### Default configuration file + +Add `share/default/config/tracker.container.postgresql.toml` modelled on the existing MySQL +container config. The PostgreSQL connection string points to a service named `postgres`: + +```toml +[core.database] +driver = "postgresql" +path = "postgresql://postgres:postgres@postgres:5432/torrust_tracker" +``` + +All other sections remain the same as the existing container configs. + +### Driver tests + +Add an inline `#[cfg(test)]` module in `postgres.rs`. The test is guarded by an environment +variable to avoid requiring a PostgreSQL container in every `cargo test` run. + +**Environment variables**: + +| Variable | Purpose | Default | +| ------------------------------------------------ | ------------------------------------------ | ------------------------- | +| `TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST` | Enable the test (must be set to any value) | unset → test is skipped | +| `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_URL` | Use an already-running PostgreSQL instance | unset → start a container | +| `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE` | PostgreSQL Docker image name | `postgres` | +| `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE_TAG` | PostgreSQL Docker image tag | `16` | + +**Test container defaults** (when no URL is provided): + +```text +internal port: 5432 +database: torrust_tracker_test +user: postgres +password: test +``` + +Start the container using `testcontainers::GenericImage` (already a dev-dependency from +MySQL tests). Set container env vars `POSTGRES_PASSWORD`, `POSTGRES_USER`, `POSTGRES_DB`. + +**Test function skeleton**: + +```rust +#[tokio::test] +async fn run_postgres_driver_tests() -> Result<(), Box> { + if std::env::var("TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST").is_err() { + return Ok(()); + } + let db_url = /* resolve from env or start container */; + let driver = Postgres::new(&db_url)?; + super::tests::run_tests(&driver).await; + Ok(()) +} +``` + +**Shared test suite**: reuse the `tests::run_tests()` function already used by the SQLite and +MySQL test modules. All three backends must pass the same set of behavioral scenarios (torrent +CRUD, whitelist CRUD, auth key CRUD, schema drop/create cycle). + +## Tasks + +### Task 1 — Add `Driver::PostgreSQL` to the configuration package + +Steps: + +- Add `PostgreSQL` variant to the `Driver` enum in + `packages/configuration/src/v2_0_0/database.rs`. +- Extend `mask_secrets()` to handle the PostgreSQL URL (share a branch with the MySQL case). +- Add test `it_should_allow_masking_the_postgresql_user_password`. + +Acceptance criteria: + +- [ ] `Driver::PostgreSQL` serializes as `"postgresql"` in TOML. +- [ ] `mask_secrets()` correctly redacts the password in a PostgreSQL URL. +- [ ] The new test passes. + +### Task 2 — Add sqlx `postgres` feature and create PostgreSQL migration files + +Steps: + +- Add `"postgres"` to the `sqlx` features in `packages/tracker-core/Cargo.toml`. +- Create `packages/tracker-core/migrations/postgresql/` with the four migration files listed + in the "What Changes" section above. +- Verify the SQL content is correct by running each migration in sequence against a temporary + PostgreSQL database and confirming the expected schema is produced. + +Acceptance criteria: + +- [ ] `packages/tracker-core/migrations/postgresql/` contains exactly four files with the + same timestamps as the SQLite and MySQL directories. +- [ ] Migration 1 creates `whitelist`, `torrents`, and `keys` with PostgreSQL DDL (`SERIAL`, + no backtick quoting, `$1`/`$2` placeholders in DML). +- [ ] Migration 2 makes `keys.valid_until` nullable. +- [ ] Migration 3 creates `torrent_aggregate_metrics`. +- [ ] Migration 4 widens `torrents.completed` and `torrent_aggregate_metrics.value` to + `BIGINT` using `ALTER COLUMN ... TYPE BIGINT` syntax. +- [ ] Running all four migrations in sequence produces a schema consistent with the SQLite + and MySQL schemas after their four migrations. + +### Task 3 — Implement the PostgreSQL driver + +Create `packages/tracker-core/src/databases/driver/postgres.rs` with: + +- `Postgres` struct (pool, `schema_ready` latch, `schema_lock` mutex). +- `Postgres::new(db_path: &str) -> Result` using `PgConnectOptions` and + `PgPoolOptions::connect_lazy_with()`. +- `static MIGRATOR: Migrator = sqlx::migrate!("migrations/postgresql");` +- `ensure_schema()` latch — same double-checked pattern as SQLite and MySQL. +- `SchemaMigrator` impl: `create_database_tables()` (MIGRATOR.run() only, no legacy + bootstrap) and `drop_database_tables()` (all five tables with `DROP TABLE IF EXISTS`). +- `TorrentMetricsStore`, `WhitelistStore`, `AuthKeyStore` impls — same semantics as the + other backends, using `$1`/`$2` placeholders and PostgreSQL upsert syntax. +- `decode_counter`/`encode_counter` helpers. + +Acceptance criteria: + +- [ ] `Postgres` satisfies the `Database` aggregate supertrait through the blanket impl + (no manual `impl Database for Postgres {}` block). +- [ ] `create_database_tables()` calls `MIGRATOR.run()` with no legacy bootstrap. +- [ ] `drop_database_tables()` drops all five tables including `_sqlx_migrations`. +- [ ] All counter reads use `decode_counter`; all counter writes use `encode_counter`. +- [ ] No bare `as i64` or `as u64` casts in the driver. + +### Task 4 — Wire the PostgreSQL driver into the factory and setup + +Steps: + +- In `packages/tracker-core/src/databases/driver/mod.rs`: + - Add `PostgreSQL` to the `Driver` enum. + - Add `pub mod postgres;`. + - Add the `Driver::PostgreSQL` arm in `build()`. +- In `packages/tracker-core/src/databases/setup.rs`: + - Add `torrust_tracker_configuration::Driver::PostgreSQL => Driver::PostgreSQL`. + +Acceptance criteria: + +- [ ] `cargo build --workspace` succeeds with `driver = "postgresql"` in a config file. +- [ ] `databases/setup.rs` correctly dispatches to the PostgreSQL driver when the + configuration specifies `driver = "postgresql"`. + +### Task 5 — Add the PostgreSQL driver tests + +Add an inline `#[cfg(test)]` module to `postgres.rs` as described in the "Driver tests" +section above. + +Steps: + +- Implement `run_postgres_driver_tests` guarded by + `TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST`. +- Support both a pre-existing PostgreSQL instance (via + `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_URL`) and a `testcontainers` container started + on demand. +- Default container tag: `16`. Image tag injection via + `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE_TAG` (enables the compatibility matrix loop + in Task 6). +- Call `tests::run_tests(&driver).await` — the shared test suite used by all backends. + +Acceptance criteria: + +- [ ] `TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST` is unset → test returns immediately + without error. +- [ ] When the env var is set, the test starts a PostgreSQL container (or connects to the + provided URL), runs the shared test suite, and passes. +- [ ] The container started by the test is removed unconditionally on completion or failure. + +### Task 6 — Extend the compatibility matrix (completing subissue 1525-01) + +Steps: + +- In `contrib/dev-tools/qa/run-db-compatibility-matrix.sh`, add: + - A test for the PostgreSQL configuration URL masking (after the existing protocol tests): + + ```bash + cargo test -p torrust-tracker-configuration postgresql_user_password -- --nocapture + ``` + + - A PostgreSQL versions loop after the MySQL loop: + + ```bash + POSTGRES_VERSIONS_STRING="${POSTGRES_VERSIONS:-14 15 16 17}" + read -r -a POSTGRES_VERSIONS <<< "$POSTGRES_VERSIONS_STRING" + + for version in "${POSTGRES_VERSIONS[@]}"; do + print_heading "PostgreSQL ${version}" + docker pull "postgres:${version}" + TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST=1 \ + TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE_TAG="${version}" \ + cargo test -p bittorrent-tracker-core run_postgres_driver_tests -- --nocapture + done + ``` + + - `POSTGRES_VERSIONS` defaults to `14 15 16 17`; override via env var. + +- The script already has `set -euo pipefail`; failures in the PostgreSQL loop will abort + the script with the failing version visible in the output. + +Acceptance criteria: + +- [ ] The script runs the PostgreSQL driver test for each version in `POSTGRES_VERSIONS`. +- [ ] The `POSTGRES_VERSIONS` set is overridable via env var. +- [ ] The script fails fast on the first failing backend/version combination. +- [ ] The script runs successfully end-to-end in a clean environment; a passing run log is + included in the PR description. +- [ ] The compatibility matrix exercises PostgreSQL 14, 15, 16, and 17 by default. + +### Task 7 — Extend the qBittorrent E2E runner with PostgreSQL (completing subissue 1525-02) + +The qBittorrent E2E runner introduced in subissue `1525-02` uses SQLite only. This task +extends it to support PostgreSQL and MySQL. MySQL E2E support (`--db-driver mysql`) is new +work introduced here — it was explicitly out of scope in `1525-02`. It is included here to +avoid a fourth subissue for a minor change and to keep all three backends consistent. + +Steps: + +- Add a `--db-driver` CLI argument to the E2E runner binary. Accept `sqlite3`, `mysql`, and + `postgresql`. Default: `sqlite3` (preserving existing behavior). +- When `--db-driver postgresql` is specified: + - Start a PostgreSQL container via `testcontainers::GenericImage` (or a `DockerCompose` + stack if a compose file is preferred). Wait for the container to be ready before starting + the tracker. Readiness can be checked by attempting a database connection or by running + `pg_isready` inside the container via `docker exec`. + - Generate a tracker config with `driver = "postgresql"` and the appropriate connection URL. + - Run the rest of the E2E scenario unchanged (seeder → tracker → leecher flow is + database-agnostic). +- Reuse the `Drop` guard pattern from the existing runner for unconditional PostgreSQL + container cleanup. +- Add a CI step (or extend the existing E2E step) that exercises `--db-driver postgresql`. +- Document the `--db-driver` argument in the binary's module doc comment. + +Acceptance criteria: + +- [ ] The E2E runner completes a full seeder → leecher download with PostgreSQL as the + backend. +- [ ] No orphaned containers remain on success or failure. +- [ ] The `--db-driver` argument is documented in the binary's module doc comment. + +### Task 8 — Extend the benchmark runner with PostgreSQL (completing subissue 1525-03) + +The benchmark runner introduced in subissue `1525-03` supports SQLite and MySQL. Extend it to +also benchmark PostgreSQL. + +Steps: + +- Add `postgresql` as an accepted value for `--dbs` in the benchmark runner CLI. +- Add `contrib/dev-tools/bench/compose.bench-postgresql.yaml` following the same structure as + the MySQL compose file: tracker service + PostgreSQL service, parameterized tracker image tag + via env var, no fixed host ports, `healthcheck` defined for each service. +- Wire the PostgreSQL compose file into the runner's per-suite lifecycle (same as MySQL/SQLite: + `DockerCompose::up()`, port discovery, workloads, `DockerCompose::down()` via `Drop` guard). +- Re-run the benchmark with both SQLite, MySQL, and PostgreSQL and update + `docs/benchmarks/baseline.md` and `docs/benchmarks/baseline.json` with the new results. + +Acceptance criteria: + +- [ ] `--dbs postgresql` produces benchmark results. +- [ ] `compose.bench-postgresql.yaml` starts and stops cleanly with no orphaned resources. +- [ ] `docs/benchmarks/baseline.md` is updated and includes PostgreSQL results. + +### Task 9 — Add the default PostgreSQL container config, update docs, and fix spell-check + +Steps: + +- Add `share/default/config/tracker.container.postgresql.toml` as described in the + "What Changes" section. + +- Update `share/container/entry_script_sh` to handle `postgresql` alongside the existing + `sqlite3` and `mysql` branches. Add an `elif` branch immediately after the `mysql` branch: + + ```sh + elif cmp_lc "$TORRUST_TRACKER_CONFIG_OVERRIDE_CORE__DATABASE__DRIVER" "postgresql"; then + + # (no database file needed for PostgreSQL) + + # Select default PostgreSQL configuration + default_config="/usr/share/torrust/default/config/tracker.container.postgresql.toml" + ``` + + Also update the error message in the `else` branch to list all three supported backends: + + ```sh + echo "Please Note: Supported Database Types: \"sqlite3\", \"mysql\", \"postgresql\"." + ``` + + The `Containerfile` already copies this file via + `COPY --chmod=0555 ./share/container/entry_script_sh /usr/local/bin/entry.sh`; no + `Containerfile` changes are needed. + +- Update `compose.yaml` to support the PostgreSQL backend alongside the existing MySQL + service: + - Add a `postgres` service using `image: postgres:16`: + + ```yaml + postgres: + image: postgres:16 + healthcheck: + test: ["CMD-SHELL", "pg_isready -U postgres"] + interval: 3s + retries: 5 + start_period: 30s + environment: + - POSTGRES_PASSWORD=postgres + - POSTGRES_USER=postgres + - POSTGRES_DB=torrust_tracker + networks: + - server_side + volumes: + - postgres_data:/var/lib/postgresql/data + ``` + + - Add `postgres` to the tracker service's `depends_on` list (alongside `mysql`) so the + tracker waits for whichever backend is healthy. Both DB services start; the tracker + connects to whichever backend the `TORRUST_TRACKER_CONFIG_OVERRIDE_CORE__DATABASE__DRIVER` + env var selects. This is acceptable for a demo / developer compose file. + + - Add a `postgres_data` named volume to the `volumes:` section. + +- Update user-facing documentation to document PostgreSQL as a supported backend: + - `README.md` — add `postgresql` to the list of supported database backends. + - `docs/containers.md` — add a section (or extend the existing database section) describing + how to run the tracker with PostgreSQL, including the `POSTGRES_DB` pre-creation + requirement and a reference to the new container config file. + +- Run `linter cspell` and add any new technical terms to `project-words.txt` in alphabetical + order. Terms likely to be flagged: `postgresql` (lowercase), `isready`, and any other + identifiers used in scripts or code comments. + +Acceptance criteria: + +- [ ] `share/default/config/tracker.container.postgresql.toml` exists and is valid TOML. +- [ ] `share/container/entry_script_sh` has a `postgresql` branch that selects + `tracker.container.postgresql.toml`; the `else` error message lists all three supported + backends. +- [ ] `compose.yaml` has a `postgres` service; the tracker service's `depends_on` includes + both `mysql` and `postgres`; a `postgres_data` volume is declared. +- [ ] `docker compose up` with + `TORRUST_TRACKER_CONFIG_OVERRIDE_CORE__DATABASE__DRIVER=postgresql` starts the tracker + successfully against the PostgreSQL container. +- [ ] The container configuration or its companion documentation (compose file or README) + creates the `torrust_tracker` database (via `POSTGRES_DB` env var or equivalent) before + the tracker is started. +- [ ] The tracker starts successfully when pointed at this config with a running PostgreSQL + container named `postgres`. +- [ ] `README.md` lists PostgreSQL as a supported database backend. +- [ ] `docs/containers.md` documents how to run the tracker with PostgreSQL and states the + database pre-creation requirement. +- [ ] `linter cspell` reports no new failures. + +## Out of Scope + +- Changing consumer wiring from `Arc>` to narrow trait objects. Deferred + until the MSRV reaches 1.76 (trait-object upcasting). +- PostgreSQL-specific performance tuning or connection pool size configuration beyond the + default `PgPoolOptions` settings. +- Down migrations (rollback support). +- TLS configuration for the PostgreSQL connection (can be expressed in the URL without code + changes). +- Any persistence redesign not required for the driver to work. +- UDP E2E testing against PostgreSQL (can be added later without redesigning the E2E setup). + +## Acceptance Criteria + +- [ ] `Driver::PostgreSQL` serializes as `"postgresql"` in TOML; the configuration package + compiles cleanly. +- [ ] `mask_secrets()` redacts the password from a PostgreSQL URL. +- [ ] `packages/tracker-core/migrations/postgresql/` contains four migration files with the + same timestamps as SQLite and MySQL. +- [ ] Migration 1 creates the tables with PostgreSQL DDL (`SERIAL`, no backtick quoting). +- [ ] Migration 4 widens `torrents.completed` and `torrent_aggregate_metrics.value` to + `BIGINT` using `ALTER COLUMN ... TYPE BIGINT` syntax. +- [ ] `packages/tracker-core/src/databases/driver/postgres.rs` exists and satisfies + `Database` through the blanket impl (no manual `impl Database for Postgres {}`). +- [ ] `create_database_tables()` calls `MIGRATOR.run()` with no legacy bootstrap. +- [ ] `drop_database_tables()` drops all five tables including `_sqlx_migrations`. +- [ ] All counter reads/writes use `decode_counter`/`encode_counter`; no bare truncating + casts. +- [ ] The shared driver test suite passes against PostgreSQL when + `TORRUST_TRACKER_CORE_RUN_POSTGRES_DRIVER_TEST` is set. +- [ ] `TORRUST_TRACKER_CORE_POSTGRES_DRIVER_IMAGE_TAG` controls the PostgreSQL version used + in tests, enabling the compatibility matrix loop. +- [ ] `run-db-compatibility-matrix.sh` loops over `POSTGRES_VERSIONS` (default: + `14 15 16 17`). +- [ ] The qBittorrent E2E runner completes a full download cycle with PostgreSQL. +- [ ] The benchmark runner produces results for PostgreSQL; `docs/benchmarks/baseline.md` + is updated. +- [ ] `share/default/config/tracker.container.postgresql.toml` exists and is valid TOML. +- [ ] `share/container/entry_script_sh` has a `postgresql` branch; the `else` error message + lists all three supported backends. +- [ ] `compose.yaml` has a `postgres` service; the tracker service's `depends_on` includes + both `mysql` and `postgres`; `docker compose up` with + `TORRUST_TRACKER_CONFIG_OVERRIDE_CORE__DATABASE__DRIVER=postgresql` starts the tracker + successfully. +- [ ] `project-words.txt` is up to date; `linter cspell` reports no failures. +- [ ] `README.md` lists PostgreSQL as a supported database backend. +- [ ] `docs/containers.md` documents how to run the tracker with PostgreSQL and states the + database pre-creation requirement. +- [ ] Persistence benchmarking shows no regression for SQLite or MySQL against the committed + baseline. +- [ ] `cargo test --workspace --all-targets` passes. +- [ ] `cargo machete` reports no unused dependencies. +- [ ] `linter all` exits with code `0`. + +## References + +- EPIC: `#1525` — `docs/issues/1525-overhaul-persistence.md` +- Subissue `1525-01`: `docs/issues/1525-01-persistence-test-coverage.md` — compatibility + matrix structure (PostgreSQL loop deferred here) +- Subissue `1525-02`: `docs/issues/1525-02-qbittorrent-e2e.md` — E2E runner (PostgreSQL + deferred here) +- Subissue `1525-03`: `docs/issues/1525-03-persistence-benchmarking.md` — benchmark runner + (PostgreSQL deferred here) +- Subissue `1525-06`: `docs/issues/1525-06-introduce-schema-migrations.md` — migration + framework and history-alignment pattern +- Subissue `1525-07`: `docs/issues/1525-07-align-rust-and-db-types.md` — fourth migration + and `NumberOfDownloads = u64` +- Reference PR: `#1695` +- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout + instructions +- Reference files: + - `packages/configuration/src/v2_0_0/database.rs` (`Driver::PostgreSQL`, URL masking) + - `packages/tracker-core/src/databases/driver/postgres.rs` (full driver) + - `packages/tracker-core/src/databases/driver/mod.rs` (`Driver::PostgreSQL` in `build()`) + - `packages/tracker-core/src/databases/setup.rs` (PostgreSQL dispatch) + - `packages/tracker-core/migrations/postgresql/` (all four migration files) + - `share/default/config/tracker.container.postgresql.toml` + - `contrib/dev-tools/qa/run-db-compatibility-matrix.sh` (PostgreSQL versions loop) + - `contrib/dev-tools/qa/run-qbittorrent-e2e.py` (E2E reference with PostgreSQL) + - `contrib/dev-tools/qa/run-before-after-db-benchmark.py` (benchmark with PostgreSQL) diff --git a/docs/issues/1525-overhaul-persistence.md b/docs/issues/1525-overhaul-persistence.md new file mode 100644 index 000000000..f1b3e623b --- /dev/null +++ b/docs/issues/1525-overhaul-persistence.md @@ -0,0 +1,150 @@ +# Issue #1525 Implementation Plan (Overhaul Persistence) + +## Goal + +Redesign the persistence layer progressively so PostgreSQL support can be added safely, with each step independently reviewable and mergeable. + +## Scope + +- Target issue: https://github.com/torrust/torrust-tracker/issues/1525 +- Reference PR: https://github.com/torrust/torrust-tracker/pull/1695 +- Review record PR: https://github.com/torrust/torrust-tracker/pull/1700 +- Key review comment: https://github.com/torrust/torrust-tracker/pull/1695#pullrequestreview-4127741472 +- Reference branch for existing implementation work: `review/pr-1695` + +## Context + +This EPIC was created in May 2025, almost a year before the current implementation effort. The problems it describes were identified early, and the opening of PR #1695 (PostgreSQL support) is what turned the plan into an active priority — but PostgreSQL is not the only driver. + +### Original motivations (from issue #1525) + +- **No migrations**: The tracker has no schema migration mechanism. As more tables are planned (e.g. extended metrics from issue #1437), the absence of migrations becomes increasingly risky. +- **Wrong crate for the job**: `r2d2` is a synchronous connection-pool library. It is not clear it is still the best fit; `sqlx` is already used in the Index project and supports async natively. The issue references SeaORM as an alternative worth researching. +- **Adding a new driver is too hard**: The `Database` trait is too wide. Adding PostgreSQL support (issue #462) was confirmed to be tricky with the current `r2d2`-based abstraction — the trait must be split before new backends can be added cleanly. + +### Immediate trigger + +PR #1695 demonstrates that the PostgreSQL work is feasible, but bundled the entire redesign into one large diff. This plan re-delivers that work incrementally so every step is independently reviewable and mergeable. + +### Why now + +The PostgreSQL PR created momentum and a concrete reference implementation. Leaving the redesign for later would mean adding more complexity on top of a layer that is already known to be the wrong shape. + +## Delivery Strategy + +Apply the redesign in small steps that can be merged independently into `develop`. + +### Phase 1: Make the change easy + +1. Add a DB compatibility matrix across supported database versions. +2. Add an end-to-end test with a real BitTorrent client. +3. Add before/after persistence benchmarking so later changes can be compared against a concrete baseline. +4. Split the persistence traits to reduce coupling. +5. Migrate existing SQL backends to the new async `sqlx` substrate without introducing PostgreSQL yet. +6. Introduce schema migrations and align schema ownership with migrations. +7. Align Rust types with the actual SQL storage model. This step may require schema changes (e.g. widening 32-bit counter columns to 64-bit), so it belongs after migrations are in place. + +### Phase 2: Make the easy change + +1. Add PostgreSQL as a first-class backend on top of the refactored persistence layer. + +## Working Rules + +- Treat `review/pr-1695` as a read-only reference branch. +- Do not try to preserve the original PR commit structure. +- Port useful code selectively from the reference branch into clean subissue branches. +- New QA and tooling code should be written in Rust unless there is a strong reason not to. +- Every subissue should produce a PR that is reviewable on its own and safe to merge before PostgreSQL support is complete. + +## Reference Implementation + +PR #1695 was authored on the fork `josecelano/torrust-tracker`, branch `pr-1684-review`. +The reference implementation lives at: + +```text +https://github.com/josecelano/torrust-tracker/tree/pr-1684-review +``` + +This branch should be treated as a **read-only reference** — a prototype that demonstrates +feasibility. Implementation work is done in dedicated subissue branches cut from `develop`. + +### Checking out the reference branch locally + +To inspect the reference implementation without affecting your current checkout, clone the +fork into a separate directory: + +```bash +git clone --branch pr-1684-review \ + https://github.com/josecelano/torrust-tracker.git \ + /path/to/torrust-tracker-pr-1700 +``` + +Replace `/path/to/torrust-tracker-pr-1700` with any directory outside your main checkout. +You can then browse or search it while working in the main repository. + +## Proposed Subissues + +### 1) Add DB compatibility matrix + +- Spec file: `docs/issues/1525-01-persistence-test-coverage.md` +- Outcome: compatibility matrix exercises SQLite and multiple MySQL versions; PostgreSQL slot + reserved for subissue 8 + +### 2) Add qBittorrent end-to-end test + +- Spec file: `docs/issues/1525-02-qbittorrent-e2e.md` +- Outcome: one complete seeder/leecher torrent-sharing scenario using real containerized clients + and docker compose, with SQLite as the backend + +### 3) Add persistence benchmarking + +- Spec file: `docs/issues/1525-03-persistence-benchmarking.md` +- Outcome: reproducible before/after performance measurements across supported backends + +### 4) Split the persistence traits by context + +- Spec file: `docs/issues/1525-04-split-persistence-traits.md` +- Outcome: smaller interfaces with lower coupling and clearer responsibilities + +### 5) Migrate SQLite and MySQL drivers to async `sqlx` + +- Spec file: `docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md` +- Outcome: shared async persistence substrate without adding PostgreSQL yet + +### 6) Introduce schema migrations + +- Spec file: `docs/issues/1525-06-introduce-schema-migrations.md` +- Outcome: schema changes become explicit, versioned, and testable + +### 7) Align persisted counters and Rust/SQL type boundaries + +- Spec file: `docs/issues/1525-07-align-rust-and-db-types.md` +- Outcome: explicit contract for persisted counters and numeric ranges, with any needed schema + changes delivered through migrations + +### 8) Add PostgreSQL driver support + +- Spec file: `docs/issues/1525-08-add-postgresql-driver.md` +- Outcome: PostgreSQL support lands on top of the refactored and migration-backed persistence + layer; PostgreSQL is added to the compatibility matrix (subissue 1) and qBittorrent E2E + (subissue 2) test harnesses + +## PR Strategy + +- Current branch for the planning docs: `1525-persistence-plan` +- Merge this planning PR into `develop` first. +- After the planning PR is merged, create one branch per subissue from `develop`. +- Keep the PRs narrow and link them back to this EPIC. + +## Acceptance Criteria + +- [ ] The EPIC plan is merged into `develop`. +- [ ] Each subissue has its own specification file in `docs/issues/`. +- [ ] The implementation order is explicit and justified. +- [ ] The plan references PR #1695 and PR #1700 as historical context, not as the delivery vehicle. + +## References + +- Related issue: #1525 +- Related PRs: #1695, #1700 +- Related discussion: PostgreSQL support request #462 diff --git a/project-words.txt b/project-words.txt index 9458ebbf3..0f5990a32 100644 --- a/project-words.txt +++ b/project-words.txt @@ -53,6 +53,7 @@ Cyberneering dashmap datagram datetime +dbname debuginfo Deque Dijke @@ -88,6 +89,7 @@ infohashes infoschema Intermodal intervali +isready Joakim kallsyms Karatay @@ -195,6 +197,7 @@ uroot usize Vagaa valgrind +VARCHAR Vitaly vmlinux Vuze @@ -268,4 +271,14 @@ Agentic agentskills frontmatter MSRV +newtypes +pipefail +qbittorrent rustup +sqlx +stabilised +subissue +Subissue +Subissues +supertrait +upcasting