Conversation
…ic the thread (or freeze), this should prevent it from freezing and locking up
📝 WalkthroughWalkthroughThe release is updated to v4.2.18. Database operations gain transient-error retries, transactions are reopened after chunk commits, logging reports failures more clearly, and torrent cache synchronization proceeds independently of database flush results. ChangesDatabase reliability and release updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TorrentTracker
participant DatabaseConnector
participant SQLxDatabase
participant PeerCountCache
TorrentTracker->>DatabaseConnector: flush torrent updates
DatabaseConnector->>SQLxDatabase: execute persistence operation
SQLxDatabase-->>DatabaseConnector: transient connection error
DatabaseConnector->>SQLxDatabase: retry operation once
SQLxDatabase-->>DatabaseConnector: database result
DatabaseConnector-->>TorrentTracker: return flush result
TorrentTracker->>PeerCountCache: write counts and delete removed hashes
TorrentTracker-->>TorrentTracker: requeue failed database batch
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/database/impls/database_connector_pgsql.rs (1)
885-898: 📐 Maintainability & Code Quality | 🔵 TrivialCorrect fix; logic sound. Same commit-before-reopen pattern as the MySQL/SQLite variants, correctly avoiding double pool-connection holding.
This implementation is identical to the MySQL and SQLite
commit_chunk(modulo the generic DB type) — see consolidated comment for a proposed dedup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/impls/database_connector_pgsql.rs` around lines 885 - 898, Deduplicate the PostgreSQL commit_chunk implementation with the corresponding MySQL and SQLite variants while preserving its commit-before-reopen ordering, chunk_size handling, transaction reset, and error logging behavior. Reuse the shared abstraction or helper identified by the existing commit_chunk implementations rather than maintaining another identical method.src/database/impls/database_connector_sqlite.rs (1)
902-915: 📐 Maintainability & Code Quality | 🔵 TrivialCorrect fix; logic sound. Matches the MySQL/PgSQL
commit_chunkpattern exactly (commit old transaction before reopening).Third verbatim copy of this helper (mysql/pgsql/sqlite) — see consolidated comment for a proposed dedup via a shared trait/generic helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/impls/database_connector_sqlite.rs` around lines 902 - 915, The SQLite commit_chunk helper duplicates the equivalent MySQL and PostgreSQL implementations. Extract the shared commit-and-reopen logic into a common trait or generic helper, then update the SQLite commit_chunk (and corresponding backend helpers as needed) to reuse it while preserving the existing chunk-size behavior and commit-before-begin ordering.src/database/impls/database_connector_mysql.rs (1)
877-890: 📐 Maintainability & Code Quality | 🔵 TrivialCorrect fix; logic sound. Commit-before-reopen ordering correctly avoids holding two pool connections simultaneously, and error propagation via
?/inspect_erris proper.Note: this exact
commit_chunkimplementation is duplicated verbatim (modulo the DB generic type) indatabase_connector_pgsql.rsanddatabase_connector_sqlite.rs— see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/impls/database_connector_mysql.rs` around lines 877 - 890, Apply the same commit-before-reopen commit_chunk implementation and error propagation in the duplicated commit_chunk methods of database_connector_pgsql.rs and database_connector_sqlite.rs, preserving their respective database transaction types and pool APIs.src/tracker/impls/torrent_tracker_torrents_updates.rs (1)
105-147: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache decoupling from
db_resultis a sound fix. Writing absolute peer counts idempotently, independent of DB flush success, correctly addresses stale-cache-during-outage without risking corruption on retry — matches the PR's goal of preventing freezes/lockups on DB connectivity issues.Minor: the per-hash
cache.delete_torrent(hash).awaitloop (lines 141-146) issues one round trip per removed torrent, unlike the batched write path just above. If the cache client exposes (or can expose) a multi-key delete, batching would reduce latency for large removal batches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tracker/impls/torrent_tracker_torrents_updates.rs` around lines 105 - 147, Batch cache deletions for removed torrents instead of issuing one delete_torrent call per hash in the cache flush logic. Update the cache client API if necessary to expose a multi-key deletion operation, then use it from the loop over torrents_to_save while preserving the existing removal filtering and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/database/impls/database_connector_mysql.rs`:
- Around line 877-890: Apply the same commit-before-reopen commit_chunk
implementation and error propagation in the duplicated commit_chunk methods of
database_connector_pgsql.rs and database_connector_sqlite.rs, preserving their
respective database transaction types and pool APIs.
In `@src/database/impls/database_connector_pgsql.rs`:
- Around line 885-898: Deduplicate the PostgreSQL commit_chunk implementation
with the corresponding MySQL and SQLite variants while preserving its
commit-before-reopen ordering, chunk_size handling, transaction reset, and error
logging behavior. Reuse the shared abstraction or helper identified by the
existing commit_chunk implementations rather than maintaining another identical
method.
In `@src/database/impls/database_connector_sqlite.rs`:
- Around line 902-915: The SQLite commit_chunk helper duplicates the equivalent
MySQL and PostgreSQL implementations. Extract the shared commit-and-reopen logic
into a common trait or generic helper, then update the SQLite commit_chunk (and
corresponding backend helpers as needed) to reuse it while preserving the
existing chunk-size behavior and commit-before-begin ordering.
In `@src/tracker/impls/torrent_tracker_torrents_updates.rs`:
- Around line 105-147: Batch cache deletions for removed torrents instead of
issuing one delete_torrent call per hash in the cache flush logic. Update the
cache client API if necessary to expose a multi-key deletion operation, then use
it from the loop over torrents_to_save while preserving the existing removal
filtering and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a35c8f3b-dc9d-4956-b5fa-b8721d6426fa
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mddocker/Dockerfiledocker/build.batsrc/common/common.rssrc/database/impls/database_connector.rssrc/database/impls/database_connector_mysql.rssrc/database/impls/database_connector_pgsql.rssrc/database/impls/database_connector_sqlite.rssrc/tracker/impls/torrent_tracker_torrents.rssrc/tracker/impls/torrent_tracker_torrents_updates.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tracker/impls/torrent_tracker_torrents_updates.rs (1)
131-149: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve failed cache work for retry after a successful database flush.
When
set_torrent_peers_batchordelete_torrentsfails, this code only logs a warning. Ifdb_resultisOk(()),drained_updatesis discarded and the failed cache operation is never retried; peer counts or deleted keys can remain stale. Keep the database result independent, but enqueue failed cache payloads in a cache-specific retry path rather than dropping them.Also applies to: 151-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tracker/impls/torrent_tracker_torrents_updates.rs` around lines 131 - 149, The cache update flow around set_torrent_peers_batch and delete_torrents must preserve failed payloads for retry after a successful database flush. Keep db_result handling independent, but enqueue cache_data when peer-count updates fail and removals when delete_torrents fails through the existing cache-specific retry mechanism, while retaining the current warning logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tracker/impls/torrent_tracker_torrents_updates.rs`:
- Around line 131-149: The cache update flow around set_torrent_peers_batch and
delete_torrents must preserve failed payloads for retry after a successful
database flush. Keep db_result handling independent, but enqueue cache_data when
peer-count updates fail and removals when delete_torrents fails through the
existing cache-specific retry mechanism, while retaining the current warning
logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c50e42d-428c-4e37-ace6-b8aca5ab3fa9
📒 Files selected for processing (4)
src/cache/impls/cache_connector_cache_backend.rssrc/cache/impls/cache_connector_redis_cache_backend.rssrc/cache/traits/cache_backend.rssrc/tracker/impls/torrent_tracker_torrents_updates.rs
1 participant
Bugfix: When connection with DB has a problem, it could literally panic the thread (or freeze), this should prevent it from freezing and locking up
Summary by CodeRabbit