feat: rework persistence for async sqlx backends and PostgreSQL support#1695
feat: rework persistence for async sqlx backends and PostgreSQL support#1695DamnCrab wants to merge 8 commits intotorrust:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Reworks tracker persistence to be async-native via sqlx, adds PostgreSQL as a first-class backend, and widens persisted download counters to 64-bit while clamping at protocol encoding boundaries.
Changes:
- Replaces the monolithic database abstraction with async, domain-focused store traits behind a
Persistencefacade. - Migrates SQLite/MySQL to
sqlx, adds a new PostgreSQL driver, and centralizes schema ownership in SQL migrations. - Widens persisted download counters to 64-bit and updates UDP/HTTP scrape encoding to saturate where protocol fields are narrower.
Reviewed changes
Copilot reviewed 68 out of 70 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| share/default/config/tracker.container.postgresql.toml | Adds a default container config for PostgreSQL deployments. |
| share/container/entry_script_sh | Adds PostgreSQL driver option and selects the PostgreSQL default config. |
| packages/udp-tracker-server/src/handlers/scrape.rs | Clamps scrape counters at the UDP protocol boundary; adds saturation tests. |
| packages/udp-tracker-server/src/handlers/mod.rs | Updates tests to use the new torrent-metrics store handle. |
| packages/udp-tracker-server/src/handlers/announce.rs | Updates tests to use the new torrent-metrics store handle. |
| packages/tracker-core/tests/integration.rs | Awaits the now-async torrent load call in integration coverage. |
| packages/tracker-core/tests/common/test_env.rs | Adds an eventual-consistency wait helper for persisted download counters. |
| packages/tracker-core/src/whitelist/test_helpers.rs | Updates whitelist test wiring to use whitelist_store(). |
| packages/tracker-core/src/whitelist/setup.rs | Refactors whitelist setup to accept a WhitelistStore handle. |
| packages/tracker-core/src/whitelist/repository/persisted.rs | Makes persisted whitelist operations async and store-trait based; updates tests. |
| packages/tracker-core/src/whitelist/manager.rs | Awaits persisted whitelist operations; updates related tests. |
| packages/tracker-core/src/torrent/manager.rs | Makes torrent loading from persistence async and store-backed. |
| packages/tracker-core/src/test_helpers.rs | Updates helper wiring to use the torrent-metrics store handle. |
| packages/tracker-core/src/statistics/persisted/mod.rs | Loads persisted global downloads asynchronously and treats them as u64. |
| packages/tracker-core/src/statistics/persisted/downloads.rs | Refactors downloads repository to async + TorrentMetricsStore; updates tests for u64. |
| packages/tracker-core/src/statistics/event/handler.rs | Awaits async persistence updates when handling completion events. |
| packages/tracker-core/src/databases/setup.rs | Makes database initialization return Persistence and adds PostgreSQL driver selection. |
| packages/tracker-core/src/databases/mod.rs | Introduces Persistence and async store traits (SchemaMigrator, TorrentMetricsStore, WhitelistStore, AuthKeyStore). |
| packages/tracker-core/src/databases/error.rs | Reworks persistence error type to support sqlx and migration errors. |
| packages/tracker-core/src/databases/driver/sqlite.rs | Reimplements SQLite backend using sqlx + migrations and async store traits. |
| packages/tracker-core/src/databases/driver/postgres.rs | Adds a new PostgreSQL backend using sqlx + migrations and async store traits. |
| packages/tracker-core/src/databases/driver/mysql.rs | Reimplements MySQL backend using sqlx + migrations and async store traits. |
| packages/tracker-core/src/databases/driver/mod.rs | Updates driver factory to return Persistence and refactors driver integration tests to async. |
| packages/tracker-core/src/container.rs | Wires TrackerCoreContainer to the new Persistence facade and store handles. |
| packages/tracker-core/src/authentication/mod.rs | Updates auth wiring to use auth_key_store(). |
| packages/tracker-core/src/authentication/key/repository/persisted.rs | Refactors persisted key repository to async + AuthKeyStore; updates tests. |
| packages/tracker-core/src/authentication/key/mod.rs | Updates key error conversion to map from the new persistence error type. |
| packages/tracker-core/src/authentication/handler.rs | Awaits async persisted key operations. |
| packages/tracker-core/src/announce_handler.rs | Makes DB download-metric loading async in announce path. |
| packages/tracker-core/migrations/sqlite/20260409120000_torrust_tracker_widen_download_counters.sql | Adds a no-op SQLite migration to keep migration history aligned. |
| packages/tracker-core/migrations/sqlite/20240730183000_torrust_tracker_create_all_tables.sql | Fixes comment syntax to SQL comment style in SQLite migration file. |
| packages/tracker-core/migrations/postgresql/20260409120000_torrust_tracker_widen_download_counters.sql | Widens PostgreSQL counter columns to BIGINT. |
| packages/tracker-core/migrations/postgresql/20250527093000_torrust_tracker_new_torrent_aggregate_metrics_table.sql | Adds PostgreSQL torrent aggregate metrics table migration. |
| packages/tracker-core/migrations/postgresql/20240730183500_torrust_tracker_keys_valid_until_nullable.sql | Makes PostgreSQL valid_until nullable. |
| packages/tracker-core/migrations/postgresql/20240730183000_torrust_tracker_create_all_tables.sql | Adds PostgreSQL base schema migration. |
| packages/tracker-core/migrations/mysql/20260409120000_torrust_tracker_widen_download_counters.sql | Widens MySQL counter columns to BIGINT. |
| packages/tracker-core/migrations/mysql/20240730183000_torrust_tracker_create_all_tables.sql | Fixes comment syntax to SQL comment style in MySQL migration file. |
| packages/tracker-core/migrations/README.md | Updates documentation to reflect automatic migrations and backend split. |
| packages/tracker-core/Cargo.toml | Switches persistence dependencies to sqlx + async-trait and removes r2d2 drivers. |
| packages/tracker-client/src/udp/client.rs | Removes a panic path in UDP health-check by handling receive errors. |
| packages/torrent-repository-benchmarking/tests/repository/mod.rs | Updates metrics/download count types to u64. |
| packages/torrent-repository-benchmarking/src/repository/skip_map_mutex_std.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_tokio_mutex_tokio.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_tokio_mutex_std.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_tokio.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_std_mutex_tokio.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_std_mutex_std.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/rw_lock_std.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/repository/dash_map_mutex_std.rs | Updates metrics/download count aggregation to use u64 directly. |
| packages/torrent-repository-benchmarking/src/entry/mod.rs | Widens torrent entry downloaded field to NumberOfDownloads. |
| packages/swarm-coordination-registry/src/swarm/registry.rs | Updates aggregate downloaded metric math to use u64 directly. |
| packages/swarm-coordination-registry/src/swarm/coordinator.rs | Widens coordinator constructor downloaded parameter to NumberOfDownloads. |
| packages/primitives/src/swarm_metadata.rs | Widens downloaded in SwarmMetadata to NumberOfDownloads and documents rationale. |
| packages/primitives/src/lib.rs | Widens NumberOfDownloads from u32 to u64. |
| packages/http-tracker-core/src/services/scrape.rs | Updates tests to use the torrent-metrics store handle. |
| packages/http-tracker-core/src/services/announce.rs | Updates tests to use the torrent-metrics store handle. |
| packages/http-tracker-core/benches/helpers/util.rs | Updates benchmark setup to use the torrent-metrics store handle. |
| packages/http-protocol/src/v1/responses/scrape.rs | Saturates large download counts when bencoding scrape responses; adds tests. |
| packages/configuration/src/v2_0_0/database.rs | Adds PostgreSQL to config driver enum and URL secret masking coverage. |
| packages/axum-rest-tracker-api-server/tests/server/v1/contract/context/whitelist.rs | Updates fault-injection helper usage to be async. |
| packages/axum-rest-tracker-api-server/tests/server/v1/contract/context/auth_key.rs | Updates fault-injection helper usage to be async. |
| packages/axum-rest-tracker-api-server/tests/server/mod.rs | Makes force_database_error async and uses schema_migrator() handles. |
| packages/axum-http-tracker-server/src/v1/handlers/announce.rs | Updates tests to use the torrent-metrics store handle. |
| packages/axum-health-check-api-server/tests/server/contract.rs | Makes UDP health-check assertions tolerant of non-timeout socket errors. |
| docs/postgresql-adaptation-rework.md | Adds design/benchmark write-up for the PostgreSQL + persistence redesign. |
| contrib/dev-tools/qa/run-qbittorrent-e2e.py | Adds an end-to-end qBittorrent QA runner for HTTP/UDP across DB backends. |
| contrib/dev-tools/qa/run-db-compatibility-matrix.sh | Adds a script to run a DB version compatibility matrix across MySQL/PostgreSQL. |
| contrib/dev-tools/qa/run-before-after-db-benchmark.py | Adds a before/after benchmark harness for persistence-related workloads. |
| .gitignore | Ignores Python bytecode caches for the new QA scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // code-review: should we return a friendly error instead of the DB | ||
| // constrain error when the key already exist? For now, it's returning | ||
| // the specif error for each DB driver when a UNIQUE constrain fails. |
There was a problem hiding this comment.
Spelling/grammar in this comment makes it harder to read (e.g. “constrain”→“constraint”, “exist”→“exists”, “specif”→“specific”). Consider correcting it since this area was touched in the refactor.
| pub type DurationSinceUnixEpoch = Duration; | ||
|
|
||
| pub type NumberOfDownloads = u32; | ||
| pub type NumberOfDownloads = u64; |
There was a problem hiding this comment.
NumberOfDownloads is defined as u64, but SQL backends store counters in signed 64-bit columns (BIGINT) and the drivers encode via i64::try_from(...). This means values > i64::MAX cannot be persisted even though the public type suggests they can. Consider switching NumberOfDownloads to i64 (or a newtype wrapping i64) across the codebase, or explicitly documenting and saturating/clamping at i64::MAX when persisting/loading to keep the effective range consistent across SQLite/MySQL/PostgreSQL.
| pub type NumberOfDownloads = u64; | |
| pub type NumberOfDownloads = i64; |
| fn decode_counter_i64(&self, value: i64) -> Result<NumberOfDownloads, Error> { | ||
| u64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) | ||
| } | ||
|
|
||
| Ok(()) | ||
| fn encode_counter(&self, value: NumberOfDownloads) -> Result<i64, Error> { | ||
| i64::try_from(value).map_err(|err| Error::invalid_query(DRIVER, err)) | ||
| } |
There was a problem hiding this comment.
encode_counter() converts NumberOfDownloads (u64) to i64 and maps overflow to an InvalidQuery error. With the counters widened to u64, this makes persistence fail once the value exceeds i64::MAX (the actual range of the BIGINT columns). Either clamp/saturate before converting (and treat BIGINT as the true max) or change the counter type to a signed 64-bit representation so the type-level contract matches what the DB can store.
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
Hi @DamnCrab, thank you for your PR. It looks good, and I think it's a very valuable feature for the community.
I'm going to start a detailed review (I will write comments on code blocks and maybe another set of questions). These are my first impressions:
- Things to fix before merging:
- Sign commits
- Pass all PR checks
- Things I would recommend doing:
This is a big PR. As you mentioned, it would have been better to implement it progressively. However, this PR give us a good understanding of all things we need to change to support PostgreSQL. Now that we have learnt a lot I think it would be convenient to do an implementation plan with two main parts:
- Refactor plan: Refactor the current state to make the new feature easy to implement.
- Add support for PostgreSQL
“Make the change easy, then make the easy change”.
Steps could be like something like this:
- Add more E2E tests (as you did). For example using a real client like you do. Everything that add tests should be included as soon as possible.
- Align Rust types with DB types (fix problems with misalignment between max u64 and the representation in the DB)
- Split the Database trait
- Migrate current drivers to sqlx crate (without migrations)
- Introduce automatic migrations
- Add new driver for postgres
Notice that all steps could be merged independently from the new feature.
I understand that doing that now can be a big effort for you, so if you prefer to keep it all in one I will try to merge it as it's because It's a nice feature to have, but it can take me longer to carefully review it.
- I have some initial questions (I haven't checked the code yet)
A) Are all migrations safe to execute in old databases?
B) Why the postgresql driver has more migrations? Should not all the drivers have the same?
C) I think it's a good idea to use a real client. I have to review that code. However I would prefer to use Rust instead of python. We are already using container for E2E testing in other parts. On the other hand, I wonder if the tracker client would be enough for the tests you are doing (I have not checked them yet). I'm planning tho change the tracker client to accept more arguments so we can simulate to be a bittorrent client. That should be simpler and faster for testing.
That's it for now. I may add some more comments.
|
Hi @DamnCrab, I prefer not to have python code:
Those are valuable tests but:
|
Summary
This PR reworks the tracker persistence layer to make PostgreSQL support fit the current async architecture, instead of adding PostgreSQL as a special case on top of the previous synchronous database abstraction.
The main goals were:
This branch also includes the follow-up fixes from review and the regressions found while running the full local test suite.
Why
The previous PostgreSQL attempt worked around the sync/async mismatch with a blocking execution model that is not a good fit for the rest of the project.
This rework addresses that first:
Main Changes
Persistence redesign
Databasetrait with narrower async traits:SchemaMigratorTorrentMetricsStoreWhitelistStoreAuthKeyStorePersistencefacade so the rest of the codebase can move to the new structure without introducing a single “god object” againAsync SQL backends
sqlxSchema and migrations
Counter widening and protocol boundaries
u32tou64Follow-up fixes included in this branch
unwrap()drop_database_tables()contract used by testsValidation
I ran the following locally:
cargo test --workspace --all-targets3.51.08.0,8.414,15,16,17I also ran real qBittorrent end-to-end tests against the tracker:
Those runs completed real seeder/leecher transfers and ended with the expected tracker scrape state:
complete = 2downloaded = 1incomplete = 0Benchmark Notes
I also compared the previous implementation and this branch with the same black-box workloads.
High-level results:
Representative PostgreSQL results from the before/after comparison:
whitelist_add_seq: about+61%auth_key_add_seq: about+30%auth_key_reload: about+38%Notes
This PR is intentionally broader than “just add a PostgreSQL driver”, because the required PostgreSQL support is tied to the persistence redesign itself.
If you would prefer, I can split this into smaller follow-up PRs after initial review, but I kept the branch together because the persistence trait split, async SQL migration, and PostgreSQL backend are tightly connected.