Conversation
* Bumped versions * Added full doc blocks
Power2All
added
documentation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR bumps the project to 4.2.15, adds security validation helpers and API user/whitelist behavior changes, refactors tracker announce/peer/RTC/user-index handling, propagates several database commit failures, and adds rustdoc across the API, HTTP, UDP, WebSocket, SSL, cache, config, common, and tracker code. Changesv4.2.15 release
Functional changes
Documentation sweep
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UdpServer
participant TorrentTracker
participant TorrentSharding
Client->>UdpServer: announce request
UdpServer->>TorrentTracker: handle_announce(query, user_key)
TorrentTracker->>TorrentSharding: add_torrent_peer / get_rtctorrent_peers
TorrentSharding-->>TorrentTracker: AnnounceEntry snapshot
TorrentTracker->>TorrentTracker: update_user_on_announce(user_id)
TorrentTracker-->>UdpServer: AnnounceEntry
UdpServer-->>Client: encoded response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/api/api_whitelists.rs (2)
55-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBatch endpoints silently drop entries whose length isn't 40.
The single-hash handlers reject a bad-length
info_hashwith400 "bad info_hash", but all three batch handlers (whitelists_get/_post/_delete) skip non-40-char entries with no output row and no error. Callers get a success response that silently omits those hashes. Consider returning an error or an explicit per-hash status for length mismatches to keep the contract consistent with the single-hash routes.🤖 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/api/api_whitelists.rs` around lines 55 - 67, The batch whitelist handlers in whitelists_get/whitelists_post/whitelists_delete are silently skipping entries whose info_hash length is not 40, unlike the single-hash routes that return a bad request. Update the loop in api_whitelists to validate every whitelist entry before processing, and either return HttpResponse::BadRequest on a length mismatch or include an explicit per-hash error status in the response so callers do not get a misleading success. Use the existing hex2bin/InfoHash/check_whitelist flow as the reference point when wiring the validation.
111-131: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate batch whitelist entries before mutating The batch POST/DELETE handlers return
400on a later invalid hash after earlier entries have already been applied and, when persistent, queued for update. Validate the full list up front or return per-item errors so the response matches the actual state.🤖 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/api/api_whitelists.rs` around lines 111 - 131, The batch whitelist handling in the whitelist loop currently mutates state as it goes, so a later invalid entry can still leave earlier entries added and queued via add_whitelist_update/add_whitelist before the 400 is returned. Update the api_whitelists flow to validate all incoming whitelists up front in the handler (before calling add_whitelist, add_whitelist_update, or building the response), or otherwise return per-item results from the same loop so the response reflects partial success accurately. Use the existing add_whitelist, add_whitelist_update, and hex2bin path to locate the affected logic.src/security/security.rs (1)
63-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a boundary-safe slice here.
path[1..]can panic on non-ASCII input because&strslicing must stay on UTF-8 boundaries. Preferpath.get(1..3) == Some(":\\" )or equivalent.🤖 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/security/security.rs` at line 63, The path check in security::security uses an unsafe string slice that can panic on non-ASCII input; update the logic around the path prefix test to avoid `path[1..]` and use a boundary-safe check such as a bounded `get` slice or equivalent. Keep the existing intent in the same conditional, and preserve the `starts_with('/')` and Windows drive-letter detection behavior while making the slice UTF-8 safe.src/http/http.rs (1)
467-546: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse each peer’s own port in compact replies
The compact branches appendannounce_unwrapped.portfor every peer, so clients receive the announcer’s port instead of each peer’speer_addr.port(). Usetorrent_peer.peer_addr.port().to_be_bytes()in all compact branches.🤖 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/http/http.rs` around lines 467 - 546, The compact peer response in the HTTP announce handler is using the announcer’s port for every peer instead of each peer’s own port. Update the compact reply logic in the `http` response branches to use `torrent_peer.peer_addr.port().to_be_bytes()` when extending `peers_list`, including both the IPv4 and IPv6 paths inside the announce handling code.src/tracker/impls/torrent_tracker_handlers.rs (1)
256-290: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSkip enqueuing the zeroed
StoppedsnapshotAnnounceEntry::default()here becomes anUpdatesAction::Addwith zero counts, and the flush path writes those values to the DB/cache. Only queue the update whenremove_torrent_peerreturns a real snapshot.🤖 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_handlers.rs` around lines 256 - 290, The AnnounceEvent::Stopped branch in torrent_tracker_handlers::handle_announce is enqueueing a default AnnounceEntry when remove_torrent_peer does not return a real snapshot, which later gets persisted as an Add update with zeroed counts. Update the handling so add_torrent_update is only called when remove_torrent_peer returns a valid new_torrent snapshot, and skip queuing any update for the default case. Keep the fix localized to the Stopped match arm and the existing remove_torrent_peer / add_torrent_update flow.
🟡 Minor comments (21)
src/security/security.rs-14-24 (1)
14-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the API key docs
BASE64_URL_SAFE_NO_PADover 32 random bytes produces a 43-character URL-safe Base64 string, not a 32-character alphanumeric key. The strength check at line 22 also classifies any non-alphanumeric character as “special,” so the wording should describe the actual character-class check instead of “mixes letters and digits.”🤖 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/security/security.rs` around lines 14 - 24, Update the documentation for generate_secure_api_key so it accurately says the function returns a 43-character URL-safe Base64 string from 32 random bytes, not a 32-character alphanumeric key. Also revise the startup warning comments/docs near the API key strength check to match what the logic actually does in the security helpers: it classifies characters by alphanumeric vs special, rather than checking that the key “mixes letters and digits.” Refer to generate_secure_api_key and the API key strength-check helper to keep the wording aligned with the implementation.src/tracker/impls/torrent_tracker_handlers.rs-291-328 (1)
291-328: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
needs_updateforCompletedannounces
save_torrent_updatesis what propagates peer-count changes to the cache, so skippingadd_torrent_updatehere leaves cache-only deployments with staleCompletedstate. This branch should follow the sameneeds_updategate asStarted/Stopped.🤖 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_handlers.rs` around lines 291 - 328, The Completed announce branch in torrent_tracker_handlers.rs is bypassing the same update flow used by Started/Stopped, which can leave cache-only deployments stale. Update the AnnounceEvent::Completed handling in the announce handler to use the existing needs_update gate before calling data.add_torrent_update, following the same pattern already used for other announce events. Keep the add_torrent_peer and data.get_rtctorrent_peers logic intact, but ensure the Completed path only skips the cache update when needs_update is false.src/api/api_certificate.rs-101-101 (1)
101-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the expiry-data claim.
The shown response serializes
cert_path,key_path, andloaded_at; it does not return expiry data here. Please align the doc with the payload or add the missing field.🤖 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/api/api_certificate.rs` at line 101, The `GET /api/certificate/status` doc comment in `api_certificate.rs` currently claims it reports expiry data, but the actual status payload only includes `cert_path`, `key_path`, and `loaded_at`. Update the documentation tied to the certificate status response to remove the expiry-data claim, or if expiry is intended, add the missing field to the response and the associated serializer in the certificate status handler.src/api/api_certificate.rs-21-22 (1)
21-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNarrow the reload description.
This handler accepts optional
server_type/bind_addressfilters, so it does not always reload “all SSL-enabled listeners.” Please update the doc to match the filtered behavior.🤖 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/api/api_certificate.rs` around lines 21 - 22, The documentation for the certificate reload handler is too broad because the reload endpoint supports optional server_type and bind_address filters. Update the doc comment on the reload handler in api_certificate so it describes reloading only the matching TLS certificates/listeners rather than “all SSL-enabled listeners,” and make the wording reflect the filtered behavior exposed by the handler.src/api/api.rs-410-412 (1)
410-412: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
api_validationdoes not validate the API token.This helper only resolves the client IP and records the IP-family stat; token authentication still happens in the callers. Please update the comment so the contract matches the implementation.
🤖 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/api/api.rs` around lines 410 - 412, The doc comment on api_validation currently claims it validates both client IP and the API token, but the function only resolves the client IP and records the IP-family stat. Update the comment above api_validation in api.rs so it accurately describes the implementation and caller contract, removing any mention of token validation and keeping the “Returns Some(response) / None” behavior description aligned with what the helper actually does.src/udp/udp.rs-57-61 (1)
57-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the parsing docs to match actual error handling.
These helpers currently do not return an error for trailing bytes; they only process full chunks and silently ignore the remainder. Please either update the docs or add an explicit length check so the contract is true.
Also applies to: 82-86, 107-111
🤖 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/udp/udp.rs` around lines 57 - 61, The parsing docs for the UDP peer entry helpers are inconsistent with the actual behavior in the parsing functions that handle packed 6-byte entries. Update the documentation on the affected helper(s) to match the implementation, or add an explicit length validation in the relevant parsing routine so it באמת returns an error on trailing bytes. Make sure the contract is aligned for the peer parsing helpers referenced by the docs in the UDP module.src/api/api_users.rs-126-126 (1)
126-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the actual batch payload shape.
The handler deserializes
Vec<(String, String, u64, u64, u64, u64, u8)>, so callers must send a JSON array of arrays/tuples here, not objects.Suggested fix
-/// `POST /api/users` — creates or replaces multiple users from a JSON array of user objects. +/// `POST /api/users` — creates or replaces multiple users from a JSON array of `[id, key, uploaded, downloaded, completed, updated, active]` tuples.🤖 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/api/api_users.rs` at line 126, The `POST /api/users` doc comment in `api_users.rs` currently says the endpoint accepts a JSON array of user objects, but the `Vec<(String, String, u64, u64, u64, u64, u8)>` deserialization means the actual payload is an array of tuples/arrays. Update the documentation near the `POST /api/users` handler to explicitly describe the tuple-shaped batch body and clarify the field order expected by `Vec<(String, String, u64, u64, u64, u64, u8)>`.src/cache/impls/cache_engine.rs-14-14 (1)
14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the full scheme strings.
url_scheme()returnsredis://andmemcache://, so the current wording is slightly misleading.♻️ Suggested fix
- /// Returns the URL scheme used to connect to this cache engine (`redis` or `memcache`). + /// Returns the URL scheme used to connect to this cache engine (`redis://` or `memcache://`).🤖 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/cache/impls/cache_engine.rs` at line 14, The documentation for url_scheme() is misleading because it implies bare scheme names, but the method returns full URL prefixes. Update the comment on cache_engine::url_scheme() to state the complete scheme strings returned by the cache engines, using the existing symbols url_scheme(), redis, and memcache as the reference point.src/config/impls/configuration.rs-704-711 (1)
704-711: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
loadcontract explicit.
String::from_utf8_lossyrewrites malformed bytes before TOML parsing, so this does not actually reject invalid UTF-8 input. That can parse a corrupted file into a different config; either document the UTF-8 requirement or switch to a fallible decode.🤖 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/config/impls/configuration.rs` around lines 704 - 711, The Configuration::load contract is currently misleading because String::from_utf8_lossy will silently rewrite invalid UTF-8 before toml::from_str runs. Update load to either use a fallible UTF-8 decode and return an error on malformed bytes, or explicitly document in the load docs that invalid UTF-8 is accepted and transformed before parsing; keep the behavior and documentation aligned for Configuration::load and toml::from_str.src/udp/impls/response.rs-130-135 (1)
130-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the
from_bytescontract.Unknown actions are not surfaced as
io::Errorhere; they become anErrorResponsewith"Invalid action". Please align the doc with the implementation, or change the branch to return an error if that is the intended API.🤖 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/udp/impls/response.rs` around lines 130 - 135, The from_bytes contract in response parsing is inconsistent with the implementation: unknown actions are currently converted into an ErrorResponse with "Invalid action" rather than an io::Error. Update the documentation on Response::from_bytes and its surrounding parse logic to match the actual behavior, or change the unknown-action branch in from_bytes to return an error if that is the intended API; make sure the wording stays aligned with the Response and ErrorResponse types.src/database/impls/database_connector.rs-30-34 (1)
30-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
Error::RowNotFoundpath too.These wrappers don't only bubble
sqlxerrors; they also synthesizeRowNotFoundwhen the selected backend slot is missing or uninitialized. Please reflect that here and in the repeatedload_*/save_*docs below.🤖 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.rs` around lines 30 - 34, The docs for the database wrapper methods are incomplete because they only mention bubbling `sqlx` errors and omit the synthesized `Error::RowNotFound` case. Update the `DatabaseConnector` method docs for the affected `load_*` and `save_*` wrappers to explicitly mention that they can return `Error::RowNotFound` when the selected backend slot is missing or uninitialized, alongside the existing `sqlx` error path. Keep the wording consistent across the repeated doc comments so the behavior is described the same way in each wrapper.src/tracker/impls/torrent_tracker_keys.rs-46-48 (1)
46-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
timeout = 0is not permanent here.
add_key()storesnow + timeout, andcheck_key()only accepts future timestamps, so0expires immediately. Please update the doc or add a real permanent-key path.🤖 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_keys.rs` around lines 46 - 48, The add_key/check_key behavior in TorrentTrackerKeys does not make timeout = 0 permanent, since the stored expiry is based on now + timeout and check_key only accepts future timestamps. Update the doc comment on add_key to describe the actual expiration behavior, or change the TorrentTrackerKeys implementation to support a true permanent-key path by handling zero-timeout entries distinctly in add_key and check_key.src/database/impls/database_connector_pgsql.rs-56-61 (1)
56-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
create()is only a connection opener.This doc says the function creates the PG schema, but the body only builds
PgConnectOptionsand opens a pool. Please move the schema note todatabase_connector()or reword this 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_pgsql.rs` around lines 56 - 61, The doc comment on create() is inaccurate because this function only builds PgConnectOptions and opens a pool, while the schema creation happens elsewhere. Update the documentation on create() to describe only the connection-opening behavior, and move the schema-creation note to database_connector() if that is the function that actually creates the PostgreSQL schema.src/tracker/impls/torrent_tracker_keys_updates.rs-59-63 (1)
59-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the snapshot/remove-after-success flow.
This method clones the queue first, then removes entries only after a successful flush; it doesn't actually drain the queue up front. Please reword the doc to match the implemented failure semantics.
🤖 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_keys_updates.rs` around lines 59 - 63, The doc comment for TorrentTrackerKeysUpdates::flush_queue currently says it drains the key-update queue up front, but the implementation actually clones the queue and only removes entries after a successful flush. Reword the snapshot/remove-after-success description to match the real behavior and make the failure semantics explicit: on error, the original queued updates remain intact, and only successful flushes clear them.src/tracker/impls/torrent_tracker_users_updates.rs-61-66 (1)
61-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the snapshot/remove-after-success flow.
This method clones the queue first, then removes entries only after a successful flush; it doesn't actually drain the queue up front. Please reword the doc to match the implemented failure semantics.
🤖 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_users_updates.rs` around lines 61 - 66, Update the doc comment for the queue-flush method in torrent_tracker_users_updates.rs to match the actual snapshot/remove-after-success behavior: describe that the method clones the current user-update queue, attempts the flush, and only removes the queued entries after a successful write. Make sure the error section reflects that on failure the queue is preserved rather than drained and restored, and keep the wording aligned with the method that performs the flush/dedup flow.src/tracker/impls/torrent_tracker_torrents_whitelist_updates.rs-75-79 (1)
75-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the snapshot/remove-after-success flow.
This method clones the queue first, then removes entries only after a successful flush; it doesn't actually drain the queue up front. Please reword the doc to match the implemented failure semantics.
🤖 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_whitelist_updates.rs` around lines 75 - 79, The doc comment for the whitelist-update flush method currently says it “drains” the queue, but the implementation actually snapshots the queue, flushes that snapshot, and only removes entries after a successful flush. Reword the documentation on the relevant queue-flush method in torrent_tracker_torrents_whitelist_updates to describe the snapshot/remove-after-success flow and the failure behavior where entries remain queued if the flush returns Err(()).src/tracker/impls/torrent_tracker_export.rs-12-13 (1)
12-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExport in-memory state, not the database.
The function serializes the tracker’s current in-memory maps/sets, not a fresh DB read. Please say “current tracker state” if that’s the intended source of truth.
✏️ Suggested wording
- /// Handles the `--export` CLI command: dumps torrents, whitelist, blacklist, keys and users - /// from the database to JSON files. + /// Handles the `--export` CLI command: dumps the current tracker state (torrents, + /// whitelist, blacklist, keys and users) to JSON files.🤖 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_export.rs` around lines 12 - 13, The export documentation in the torrent tracker export implementation is misleading because it says the command dumps data from the database, but the source of truth is the tracker’s current in-memory state. Update the doc comment in the export path around the tracker export function so it says “current tracker state” and reflects that it serializes the in-memory maps/sets rather than reading fresh data from the DB.src/database/impls/database_connector_sqlite.rs-56-61 (1)
56-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDon't claim schema creation here.
create()only opens the SQLite pool; the table bootstrap happens indatabase_connector()whencreate_databaseis enabled. Please narrow the doc to the function's actual responsibility.✏️ Suggested wording
- /// Creates the SQLite 3 database schema (tables for torrents, whitelist, blacklist, - /// keys and users) and returns a connection pool. + /// Opens the SQLite connection pool for the configured DSN. + /// Schema creation is handled separately when requested.🤖 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 56 - 61, The doc comment for `create()` is overstating its behavior by claiming it creates the SQLite schema, but this function only opens the SQLite pool. Update the documentation on `create()` in `DatabaseConnectorSqlite` to describe only pool initialization and its error behavior, and leave schema/bootstrap creation to `database_connector()` when `create_database` is enabled.src/rtctorrent_bridge/impls/rtc_torrent_bridge.rs-12-16 (1)
12-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the file-not-found error path.
create_torrent,seed_torrent, and the file-backeddownload_torrentbranch also returnFileNotFoundErrorbefore Node starts. Please mention that in the# Errorssections so the public contract matches the implementation.Suggested wording
- /// Returns an [`RtcTorrentBridgeError`] when the script fails or its output cannot be parsed. + /// Returns an [`RtcTorrentBridgeError`] when the script fails, its output cannot be parsed, + /// or the input file/path does not exist.Also applies to: 72-76, 127-131
🤖 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/rtctorrent_bridge/impls/rtc_torrent_bridge.rs` around lines 12 - 16, Update the doc comments for create_torrent, seed_torrent, and the file-backed download_torrent branch in RtcTorrentBridge so the # Errors sections explicitly mention FileNotFoundError when the input file is missing before Node starts. Keep the existing RtcTorrentBridgeError/script-failure and parse-error descriptions, and expand the public contract wording near those method comments to match the implementation.src/ssl/impls/certificate_store.rs-63-69 (1)
63-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse bundle terminology in the accessor docs.
get_certificate()andget_all_certificates()returnArc<CertificateBundle>, not a key-only value. The current wording is misleading, and callers already use bundle fields such asloaded_at.Suggested wording
- /// Returns the currently loaded certified key for a server, if any. + /// Returns the currently loaded certificate bundle for a server, if any. - /// Returns the loaded certified keys of all registered servers. + /// Returns the loaded certificate bundles of all registered servers.Also applies to: 132-140
🤖 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/ssl/impls/certificate_store.rs` around lines 63 - 69, The accessor docs for get_certificate() and get_all_certificates() use “certificate”/“key” wording that is misleading because both methods return Arc<CertificateBundle> values. Update the doc comments on these accessors in CertificateStore to refer to the loaded bundle terminology, matching the returned type and the bundle fields callers use such as loaded_at, while keeping the method names unchanged.src/ssl/impls/dynamic_certificate_resolver.rs-19-23 (1)
19-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument signing-key conversion failures here too.
new()andrefresh_cache()can also returnCertificateError::CertifiedKeyErrorwhenbundle_to_certified_key()can't build a usable rustls signing key, not just when the store is empty.🤖 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/ssl/impls/dynamic_certificate_resolver.rs` around lines 19 - 23, The documentation for DynamicCertificateResolver::new and refresh_cache currently only mentions the empty-store case, but these methods can also fail with CertificateError::CertifiedKeyError when bundle_to_certified_key() cannot produce a usable rustls signing key. Update the doc comments on the relevant methods in dynamic_certificate_resolver.rs to mention this signing-key conversion failure alongside the existing no-certificate-loaded error, using the bundle_to_certified_key and CertifiedKeyError symbols as the reference points.
🧹 Nitpick comments (1)
src/tracker/impls/torrent_tracker.rs (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a plain
AHashMap::default()for consistency.Other structs in this cohort (e.g.
TorrentEntry::default,AnnounceEntry::default) constructAHashMap::default()via a direct import rather than the fully-qualifiedcrate::tracker::types::ahash_map::AHashMap::default()path used here. Purely cosmetic, but worth aligning for readability.♻️ Optional cleanup
- users_key_index: Arc::new(RwLock::new(crate::tracker::types::ahash_map::AHashMap::default())), + users_key_index: Arc::new(RwLock::new(AHashMap::default())),(requires
use crate::tracker::types::ahash_map::AHashMap;at the top of the file if not already imported)🤖 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.rs` at line 121, Use the plain AHashMap::default() style for the users_key_index initialization in TorrentTracker to match TorrentEntry::default and AnnounceEntry::default. Add the direct AHashMap import at the top of torrent_tracker.rs if needed, then replace the fully qualified crate::tracker::types::ahash_map::AHashMap::default() reference in TorrentTracker’s default/constructor path with AHashMap::default() for consistency and readability.
🤖 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.
Inline comments:
In `@src/api/api_users.rs`:
- Line 189: The delete endpoints are still using the internal hash instead of
the advertised id/UUID, so align them with the read/create flow. Update the
delete handlers in api_users (including the batch delete loop) to use hash_id on
the incoming user id/UUID, or else rename the exposed route/body field to
user_hash if you intend to keep deleting by hash. Make sure both the
single-delete and bulk-delete paths use the same identifier conversion as the
get path.
- Line 50: The `GET /api/users` handler is documented as accepting raw
ids/UUIDs, but the `len() == 40` check in the `api_users` path blocks those
inputs and `api_service_users_return_json` hashes them again. Remove the length
gate so the handler forwards raw ids/UUIDs to `api_service_users_return_json`,
or alternatively update the `api_users` docstring to state it only accepts
pre-hashed inputs.
In `@src/database/impls/database_connector_pgsql.rs`:
- Around line 388-392: The transaction save helpers are reporting success
without checking the result of `self.commit(transaction).await`, so failures can
be silently dropped. Update `save_whitelist`, `save_blacklist`, `save_keys`,
`save_users`, and `reset_seeds_peers` to await and propagate the commit result,
then only return success after a successful commit. Make sure the affected
helper methods keep their `# Errors` docs and any queue-flush guarantees in sync
with the actual `DatabaseConnectorPgsql` behavior.
In `@src/database/impls/database_connector_sqlite.rs`:
- Around line 413-417: The transaction helpers in DatabaseConnectorSqlite are
swallowing failures from self.commit(...), so callers can see success even after
a rollback. Update the whitelist helper shown here and the matching blacklist,
key, and reset helpers to return the result of self.commit(...) instead of
discarding it, preserving any commit error in the function’s Result. This keeps
the documented sqlx error behavior accurate and ensures queue-flush callers
observe commit failures.
In `@src/tracker/impls/torrent_tracker_torrents_blacklist.rs`:
- Around line 12-18: The load_blacklist flow in TorrentTrackerTorrentsBlacklist
currently ignores errors from self.sqlx.load_blacklist(tracker).await, which can
let startup continue with an empty blacklist. Update load_blacklist to handle
the Err case explicitly by logging the failure with context or returning the
error so the caller can fail startup closed, and keep the success path for the
Loaded {blacklist} message.
In `@src/tracker/impls/torrent_tracker_torrents_whitelist.rs`:
- Around line 12-18: The whitelist startup path in load_whitelist currently
ignores errors from self.sqlx.load_whitelist(tracker), which lets the tracker
continue with an empty whitelist. Update load_whitelist in
TorrentTrackerTorrentsWhitelist so failures are not swallowed: either propagate
the error to the caller or log it and return a failure so startup can fail
closed. Keep the success path unchanged, but make sure the error case is handled
explicitly instead of using if let Ok(...).
---
Outside diff comments:
In `@src/api/api_whitelists.rs`:
- Around line 55-67: The batch whitelist handlers in
whitelists_get/whitelists_post/whitelists_delete are silently skipping entries
whose info_hash length is not 40, unlike the single-hash routes that return a
bad request. Update the loop in api_whitelists to validate every whitelist entry
before processing, and either return HttpResponse::BadRequest on a length
mismatch or include an explicit per-hash error status in the response so callers
do not get a misleading success. Use the existing
hex2bin/InfoHash/check_whitelist flow as the reference point when wiring the
validation.
- Around line 111-131: The batch whitelist handling in the whitelist loop
currently mutates state as it goes, so a later invalid entry can still leave
earlier entries added and queued via add_whitelist_update/add_whitelist before
the 400 is returned. Update the api_whitelists flow to validate all incoming
whitelists up front in the handler (before calling add_whitelist,
add_whitelist_update, or building the response), or otherwise return per-item
results from the same loop so the response reflects partial success accurately.
Use the existing add_whitelist, add_whitelist_update, and hex2bin path to locate
the affected logic.
In `@src/http/http.rs`:
- Around line 467-546: The compact peer response in the HTTP announce handler is
using the announcer’s port for every peer instead of each peer’s own port.
Update the compact reply logic in the `http` response branches to use
`torrent_peer.peer_addr.port().to_be_bytes()` when extending `peers_list`,
including both the IPv4 and IPv6 paths inside the announce handling code.
In `@src/security/security.rs`:
- Line 63: The path check in security::security uses an unsafe string slice that
can panic on non-ASCII input; update the logic around the path prefix test to
avoid `path[1..]` and use a boundary-safe check such as a bounded `get` slice or
equivalent. Keep the existing intent in the same conditional, and preserve the
`starts_with('/')` and Windows drive-letter detection behavior while making the
slice UTF-8 safe.
In `@src/tracker/impls/torrent_tracker_handlers.rs`:
- Around line 256-290: The AnnounceEvent::Stopped branch in
torrent_tracker_handlers::handle_announce is enqueueing a default AnnounceEntry
when remove_torrent_peer does not return a real snapshot, which later gets
persisted as an Add update with zeroed counts. Update the handling so
add_torrent_update is only called when remove_torrent_peer returns a valid
new_torrent snapshot, and skip queuing any update for the default case. Keep the
fix localized to the Stopped match arm and the existing remove_torrent_peer /
add_torrent_update flow.
---
Minor comments:
In `@src/api/api_certificate.rs`:
- Line 101: The `GET /api/certificate/status` doc comment in
`api_certificate.rs` currently claims it reports expiry data, but the actual
status payload only includes `cert_path`, `key_path`, and `loaded_at`. Update
the documentation tied to the certificate status response to remove the
expiry-data claim, or if expiry is intended, add the missing field to the
response and the associated serializer in the certificate status handler.
- Around line 21-22: The documentation for the certificate reload handler is too
broad because the reload endpoint supports optional server_type and bind_address
filters. Update the doc comment on the reload handler in api_certificate so it
describes reloading only the matching TLS certificates/listeners rather than
“all SSL-enabled listeners,” and make the wording reflect the filtered behavior
exposed by the handler.
In `@src/api/api_users.rs`:
- Line 126: The `POST /api/users` doc comment in `api_users.rs` currently says
the endpoint accepts a JSON array of user objects, but the `Vec<(String, String,
u64, u64, u64, u64, u8)>` deserialization means the actual payload is an array
of tuples/arrays. Update the documentation near the `POST /api/users` handler to
explicitly describe the tuple-shaped batch body and clarify the field order
expected by `Vec<(String, String, u64, u64, u64, u64, u8)>`.
In `@src/api/api.rs`:
- Around line 410-412: The doc comment on api_validation currently claims it
validates both client IP and the API token, but the function only resolves the
client IP and records the IP-family stat. Update the comment above
api_validation in api.rs so it accurately describes the implementation and
caller contract, removing any mention of token validation and keeping the
“Returns Some(response) / None” behavior description aligned with what the
helper actually does.
In `@src/cache/impls/cache_engine.rs`:
- Line 14: The documentation for url_scheme() is misleading because it implies
bare scheme names, but the method returns full URL prefixes. Update the comment
on cache_engine::url_scheme() to state the complete scheme strings returned by
the cache engines, using the existing symbols url_scheme(), redis, and memcache
as the reference point.
In `@src/config/impls/configuration.rs`:
- Around line 704-711: The Configuration::load contract is currently misleading
because String::from_utf8_lossy will silently rewrite invalid UTF-8 before
toml::from_str runs. Update load to either use a fallible UTF-8 decode and
return an error on malformed bytes, or explicitly document in the load docs that
invalid UTF-8 is accepted and transformed before parsing; keep the behavior and
documentation aligned for Configuration::load and toml::from_str.
In `@src/database/impls/database_connector_pgsql.rs`:
- Around line 56-61: The doc comment on create() is inaccurate because this
function only builds PgConnectOptions and opens a pool, while the schema
creation happens elsewhere. Update the documentation on create() to describe
only the connection-opening behavior, and move the schema-creation note to
database_connector() if that is the function that actually creates the
PostgreSQL schema.
In `@src/database/impls/database_connector_sqlite.rs`:
- Around line 56-61: The doc comment for `create()` is overstating its behavior
by claiming it creates the SQLite schema, but this function only opens the
SQLite pool. Update the documentation on `create()` in `DatabaseConnectorSqlite`
to describe only pool initialization and its error behavior, and leave
schema/bootstrap creation to `database_connector()` when `create_database` is
enabled.
In `@src/database/impls/database_connector.rs`:
- Around line 30-34: The docs for the database wrapper methods are incomplete
because they only mention bubbling `sqlx` errors and omit the synthesized
`Error::RowNotFound` case. Update the `DatabaseConnector` method docs for the
affected `load_*` and `save_*` wrappers to explicitly mention that they can
return `Error::RowNotFound` when the selected backend slot is missing or
uninitialized, alongside the existing `sqlx` error path. Keep the wording
consistent across the repeated doc comments so the behavior is described the
same way in each wrapper.
In `@src/rtctorrent_bridge/impls/rtc_torrent_bridge.rs`:
- Around line 12-16: Update the doc comments for create_torrent, seed_torrent,
and the file-backed download_torrent branch in RtcTorrentBridge so the # Errors
sections explicitly mention FileNotFoundError when the input file is missing
before Node starts. Keep the existing RtcTorrentBridgeError/script-failure and
parse-error descriptions, and expand the public contract wording near those
method comments to match the implementation.
In `@src/security/security.rs`:
- Around line 14-24: Update the documentation for generate_secure_api_key so it
accurately says the function returns a 43-character URL-safe Base64 string from
32 random bytes, not a 32-character alphanumeric key. Also revise the startup
warning comments/docs near the API key strength check to match what the logic
actually does in the security helpers: it classifies characters by alphanumeric
vs special, rather than checking that the key “mixes letters and digits.” Refer
to generate_secure_api_key and the API key strength-check helper to keep the
wording aligned with the implementation.
In `@src/ssl/impls/certificate_store.rs`:
- Around line 63-69: The accessor docs for get_certificate() and
get_all_certificates() use “certificate”/“key” wording that is misleading
because both methods return Arc<CertificateBundle> values. Update the doc
comments on these accessors in CertificateStore to refer to the loaded bundle
terminology, matching the returned type and the bundle fields callers use such
as loaded_at, while keeping the method names unchanged.
In `@src/ssl/impls/dynamic_certificate_resolver.rs`:
- Around line 19-23: The documentation for DynamicCertificateResolver::new and
refresh_cache currently only mentions the empty-store case, but these methods
can also fail with CertificateError::CertifiedKeyError when
bundle_to_certified_key() cannot produce a usable rustls signing key. Update the
doc comments on the relevant methods in dynamic_certificate_resolver.rs to
mention this signing-key conversion failure alongside the existing
no-certificate-loaded error, using the bundle_to_certified_key and
CertifiedKeyError symbols as the reference points.
In `@src/tracker/impls/torrent_tracker_export.rs`:
- Around line 12-13: The export documentation in the torrent tracker export
implementation is misleading because it says the command dumps data from the
database, but the source of truth is the tracker’s current in-memory state.
Update the doc comment in the export path around the tracker export function so
it says “current tracker state” and reflects that it serializes the in-memory
maps/sets rather than reading fresh data from the DB.
In `@src/tracker/impls/torrent_tracker_handlers.rs`:
- Around line 291-328: The Completed announce branch in
torrent_tracker_handlers.rs is bypassing the same update flow used by
Started/Stopped, which can leave cache-only deployments stale. Update the
AnnounceEvent::Completed handling in the announce handler to use the existing
needs_update gate before calling data.add_torrent_update, following the same
pattern already used for other announce events. Keep the add_torrent_peer and
data.get_rtctorrent_peers logic intact, but ensure the Completed path only skips
the cache update when needs_update is false.
In `@src/tracker/impls/torrent_tracker_keys_updates.rs`:
- Around line 59-63: The doc comment for TorrentTrackerKeysUpdates::flush_queue
currently says it drains the key-update queue up front, but the implementation
actually clones the queue and only removes entries after a successful flush.
Reword the snapshot/remove-after-success description to match the real behavior
and make the failure semantics explicit: on error, the original queued updates
remain intact, and only successful flushes clear them.
In `@src/tracker/impls/torrent_tracker_keys.rs`:
- Around line 46-48: The add_key/check_key behavior in TorrentTrackerKeys does
not make timeout = 0 permanent, since the stored expiry is based on now +
timeout and check_key only accepts future timestamps. Update the doc comment on
add_key to describe the actual expiration behavior, or change the
TorrentTrackerKeys implementation to support a true permanent-key path by
handling zero-timeout entries distinctly in add_key and check_key.
In `@src/tracker/impls/torrent_tracker_torrents_whitelist_updates.rs`:
- Around line 75-79: The doc comment for the whitelist-update flush method
currently says it “drains” the queue, but the implementation actually snapshots
the queue, flushes that snapshot, and only removes entries after a successful
flush. Reword the documentation on the relevant queue-flush method in
torrent_tracker_torrents_whitelist_updates to describe the
snapshot/remove-after-success flow and the failure behavior where entries remain
queued if the flush returns Err(()).
In `@src/tracker/impls/torrent_tracker_users_updates.rs`:
- Around line 61-66: Update the doc comment for the queue-flush method in
torrent_tracker_users_updates.rs to match the actual
snapshot/remove-after-success behavior: describe that the method clones the
current user-update queue, attempts the flush, and only removes the queued
entries after a successful write. Make sure the error section reflects that on
failure the queue is preserved rather than drained and restored, and keep the
wording aligned with the method that performs the flush/dedup flow.
In `@src/udp/impls/response.rs`:
- Around line 130-135: The from_bytes contract in response parsing is
inconsistent with the implementation: unknown actions are currently converted
into an ErrorResponse with "Invalid action" rather than an io::Error. Update the
documentation on Response::from_bytes and its surrounding parse logic to match
the actual behavior, or change the unknown-action branch in from_bytes to return
an error if that is the intended API; make sure the wording stays aligned with
the Response and ErrorResponse types.
In `@src/udp/udp.rs`:
- Around line 57-61: The parsing docs for the UDP peer entry helpers are
inconsistent with the actual behavior in the parsing functions that handle
packed 6-byte entries. Update the documentation on the affected helper(s) to
match the implementation, or add an explicit length validation in the relevant
parsing routine so it באמת returns an error on trailing bytes. Make sure the
contract is aligned for the peer parsing helpers referenced by the docs in the
UDP module.
---
Nitpick comments:
In `@src/tracker/impls/torrent_tracker.rs`:
- Line 121: Use the plain AHashMap::default() style for the users_key_index
initialization in TorrentTracker to match TorrentEntry::default and
AnnounceEntry::default. Add the direct AHashMap import at the top of
torrent_tracker.rs if needed, then replace the fully qualified
crate::tracker::types::ahash_map::AHashMap::default() reference in
TorrentTracker’s default/constructor path with AHashMap::default() for
consistency and readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e44e9985-196f-4a3a-a61a-e73169cb38ec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
Cargo.tomlREADME.mddocker/Dockerfiledocker/build.batsrc/api/api.rssrc/api/api_blacklists.rssrc/api/api_certificate.rssrc/api/api_keys.rssrc/api/api_stats.rssrc/api/api_torrents.rssrc/api/api_users.rssrc/api/api_whitelists.rssrc/cache/impls/cache_connector_connect.rssrc/cache/impls/cache_connector_memcache_connect.rssrc/cache/impls/cache_connector_redis_connect.rssrc/cache/impls/cache_engine.rssrc/cache/impls/torrent_peer_counts.rssrc/common/common.rssrc/common/impls/compressed_bytes.rssrc/common/impls/custom_error.rssrc/common/structs/compressed_bytes.rssrc/config/config.rssrc/config/impls/cluster_mode.rssrc/config/impls/configuration.rssrc/database/database.rssrc/database/impls/database_connector.rssrc/database/impls/database_connector_mysql.rssrc/database/impls/database_connector_pgsql.rssrc/database/impls/database_connector_sqlite.rssrc/database/impls/query_builder.rssrc/http/http.rssrc/rtctorrent_bridge/impls/rtc_torrent_bridge.rssrc/security/security.rssrc/ssl/impls/certificate_store.rssrc/ssl/impls/dynamic_certificate_resolver.rssrc/ssl/impls/server_identifier.rssrc/ssl/ssl.rssrc/stats/impls/torrent_tracker.rssrc/tracker/impls/announce_entry.rssrc/tracker/impls/cleanup_stats.rssrc/tracker/impls/peer_id.rssrc/tracker/impls/rtc_data.rssrc/tracker/impls/torrent_counts.rssrc/tracker/impls/torrent_entry.rssrc/tracker/impls/torrent_peer.rssrc/tracker/impls/torrent_sharding.rssrc/tracker/impls/torrent_tracker.rssrc/tracker/impls/torrent_tracker_cert_gen.rssrc/tracker/impls/torrent_tracker_export.rssrc/tracker/impls/torrent_tracker_handlers.rssrc/tracker/impls/torrent_tracker_import.rssrc/tracker/impls/torrent_tracker_keys.rssrc/tracker/impls/torrent_tracker_keys_updates.rssrc/tracker/impls/torrent_tracker_peers.rssrc/tracker/impls/torrent_tracker_rtctorrent.rssrc/tracker/impls/torrent_tracker_torrents.rssrc/tracker/impls/torrent_tracker_torrents_blacklist.rssrc/tracker/impls/torrent_tracker_torrents_blacklist_updates.rssrc/tracker/impls/torrent_tracker_torrents_updates.rssrc/tracker/impls/torrent_tracker_torrents_whitelist.rssrc/tracker/impls/torrent_tracker_torrents_whitelist_updates.rssrc/tracker/impls/torrent_tracker_users.rssrc/tracker/impls/torrent_tracker_users_updates.rssrc/tracker/structs/torrent_tracker.rssrc/udp/impls/batch_recv.rssrc/udp/impls/io_uring_recv.rssrc/udp/impls/parse_pool.rssrc/udp/impls/request.rssrc/udp/impls/request_parse_error.rssrc/udp/impls/response.rssrc/udp/impls/rio_recv.rssrc/udp/impls/simple_proxy_protocol.rssrc/udp/impls/udp_server.rssrc/udp/udp.rssrc/utils/sentry_tracing.rssrc/websocket/impls/cluster_connection.rssrc/websocket/impls/cluster_request.rssrc/websocket/impls/cluster_response.rssrc/websocket/impls/handshake_request.rssrc/websocket/impls/handshake_response.rssrc/websocket/impls/slave_client_state.rssrc/websocket/websocket.rstests/tracker_tests.rstorrust-actix.desktop
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/tracker/impls/torrent_tracker_torrents_whitelist.rs (1)
12-25: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLoad failure is now logged but startup still doesn't fail closed.
This addresses half of the earlier request (logging), but the tracker still proceeds with an empty in-memory whitelist on DB failure — as the new doc comment itself states. With
whitelist_enabled,check_whitelistwill then reject every legitimate announce, silently degrading the tracker into rejecting all traffic instead of failing fast at boot. The original ask was to make startup fail closed on this error.🩺 Proposed fix to fail closed on load failure
pub async fn load_whitelist(&self, tracker: Arc<TorrentTracker>) { match self.sqlx.load_whitelist(tracker).await { Ok(whitelist) => { info!("Loaded {whitelist} whitelists"); } Err(e) => { error!("Unable to load the whitelist from the database: {e}"); + std::process::exit(1); } } }Please verify how
load_whitelistis invoked at startup (likely inmain.rs, not in this file set) to confirm whether anexit(1)here or a propagatedResultis the preferred fail-closed mechanism.🤖 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_whitelist.rs` around lines 12 - 25, load_whitelist currently logs database errors but lets startup continue with an empty whitelist, which causes whitelist_enabled to reject valid announces instead of failing closed. Update TorrentTrackerTorrentsWhitelist::load_whitelist so the database load failure is propagated or otherwise causes startup to abort, and adjust the startup caller that invokes load_whitelist to stop the process on error rather than proceeding with an empty in-memory whitelist. Keep the existing logging, but ensure the failure path is fatal instead of silently continuing.src/api/api_users.rs (1)
188-189: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDELETE endpoints still diverge from GET/POST identifier semantics.
This only documents the existing behavior — GET/POST accept a raw id/UUID and hash it internally, while DELETE still expects the caller to supply the pre-computed 40-character hex hash. That asymmetry (and the underlying implementation) was flagged previously as confusing/inconsistent API design; documenting it doesn't resolve the underlying mismatch, it only records it.
Also applies to: 222-223
🤖 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/api/api_users.rs` around lines 188 - 189, The DELETE user route still uses a different identifier contract than the GET/POST paths, so align the DELETE handler’s lookup logic with the same raw id/UUID handling used elsewhere instead of requiring callers to pass a precomputed 40-character hash. Update the DELETE route implementation and any related docs/comments around the user delete endpoint so the identifier semantics are consistent across the API, using the same hashing behavior already applied in the GET/POST flow.
🤖 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.
Duplicate comments:
In `@src/api/api_users.rs`:
- Around line 188-189: The DELETE user route still uses a different identifier
contract than the GET/POST paths, so align the DELETE handler’s lookup logic
with the same raw id/UUID handling used elsewhere instead of requiring callers
to pass a precomputed 40-character hash. Update the DELETE route implementation
and any related docs/comments around the user delete endpoint so the identifier
semantics are consistent across the API, using the same hashing behavior already
applied in the GET/POST flow.
In `@src/tracker/impls/torrent_tracker_torrents_whitelist.rs`:
- Around line 12-25: load_whitelist currently logs database errors but lets
startup continue with an empty whitelist, which causes whitelist_enabled to
reject valid announces instead of failing closed. Update
TorrentTrackerTorrentsWhitelist::load_whitelist so the database load failure is
propagated or otherwise causes startup to abort, and adjust the startup caller
that invokes load_whitelist to stop the process on error rather than proceeding
with an empty in-memory whitelist. Keep the existing logging, but ensure the
failure path is fatal instead of silently continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2138cacd-33f2-489b-8f49-21052af80355
📒 Files selected for processing (27)
src/api/api.rssrc/api/api_certificate.rssrc/api/api_users.rssrc/api/api_whitelists.rssrc/cache/impls/cache_engine.rssrc/config/impls/configuration.rssrc/database/impls/database_connector.rssrc/database/impls/database_connector_mysql.rssrc/database/impls/database_connector_pgsql.rssrc/database/impls/database_connector_sqlite.rssrc/http/http.rssrc/rtctorrent_bridge/impls/rtc_torrent_bridge.rssrc/security/security.rssrc/ssl/impls/certificate_store.rssrc/ssl/impls/dynamic_certificate_resolver.rssrc/tracker/impls/torrent_tracker.rssrc/tracker/impls/torrent_tracker_export.rssrc/tracker/impls/torrent_tracker_handlers.rssrc/tracker/impls/torrent_tracker_keys.rssrc/tracker/impls/torrent_tracker_keys_updates.rssrc/tracker/impls/torrent_tracker_torrents_blacklist.rssrc/tracker/impls/torrent_tracker_torrents_blacklist_updates.rssrc/tracker/impls/torrent_tracker_torrents_whitelist.rssrc/tracker/impls/torrent_tracker_torrents_whitelist_updates.rssrc/tracker/impls/torrent_tracker_users_updates.rssrc/udp/impls/response.rssrc/udp/udp.rs
✅ Files skipped from review due to trivial changes (17)
- src/api/api_certificate.rs
- src/cache/impls/cache_engine.rs
- src/udp/impls/response.rs
- src/ssl/impls/dynamic_certificate_resolver.rs
- src/rtctorrent_bridge/impls/rtc_torrent_bridge.rs
- src/ssl/impls/certificate_store.rs
- src/udp/udp.rs
- src/tracker/impls/torrent_tracker_torrents_blacklist_updates.rs
- src/tracker/impls/torrent_tracker_export.rs
- src/tracker/impls/torrent_tracker_torrents_blacklist.rs
- src/tracker/impls/torrent_tracker_torrents_whitelist_updates.rs
- src/tracker/impls/torrent_tracker_users_updates.rs
- src/api/api.rs
- src/tracker/impls/torrent_tracker_keys.rs
- src/config/impls/configuration.rs
- src/tracker/impls/torrent_tracker_keys_updates.rs
- src/database/impls/database_connector.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tracker/impls/torrent_tracker.rs
- src/tracker/impls/torrent_tracker_handlers.rs
- src/security/security.rs
- src/http/http.rs
- src/api/api_whitelists.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/api_users.rs (1)
231-246: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist loop-invariant work out of the per-id loop.
The persistent-mode boolean on Line 233 and the
empty_usertemplate (Lines 234-244) are identical for every id, yet they are recomputed on each iteration. Evaluating the flag once before the loop and building the template with.clone()inside keeps the batch path cheaper and clearer.♻️ Suggested hoist
let mut users_output = HashMap::with_capacity(ids.len()); + let persistent = data.torrent_tracker.config.database_structure.users.persistent + .unwrap_or(data.torrent_tracker.config.database.persistent); for id in ids { let id_hash = UserId(hash_id(&id)); - if data.torrent_tracker.config.database_structure.users.persistent.unwrap_or(data.torrent_tracker.config.database.persistent) { + if persistent { let empty_user = UserEntryItem {🤖 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/api/api_users.rs` around lines 231 - 246, Hoist the loop-invariant persistent-mode check and the empty user template out of the per-id loop in the user deletion path. In the loop that iterates over ids in api_users.rs, compute the config boolean once before entering the loop, build the UserEntryItem template once, and then reuse it inside the loop with a clone when calling data.torrent_tracker.add_user_update with UpdatesAction::Remove. Keep the existing behavior in the add_user_update flow, but avoid recomputing the same values for every id.
🤖 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/api/api_users.rs`:
- Around line 231-246: Hoist the loop-invariant persistent-mode check and the
empty user template out of the per-id loop in the user deletion path. In the
loop that iterates over ids in api_users.rs, compute the config boolean once
before entering the loop, build the UserEntryItem template once, and then reuse
it inside the loop with a clone when calling
data.torrent_tracker.add_user_update with UpdatesAction::Remove. Keep the
existing behavior in the add_user_update flow, but avoid recomputing the same
values for every id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffb644c4-a613-4097-8bc0-847bca9eb190
📒 Files selected for processing (2)
src/api/api_users.rssrc/tracker/impls/torrent_tracker_torrents_whitelist.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
GET /api/usersincludes all provided identifiers;DELETEoperations now hash inputs consistently without enforcing hex formatting rules.Chores