Conversation
📝 WalkthroughWalkthroughVersion 4.2.9 release hardening supply chain security through dependency pinning, refactoring database initialization to exit from main rather than connectors, fixing test reliability for SQLite remove and concurrent operations, and updating version metadata across build and desktop entry files. Changesv4.2.9 Release and Supply Chain Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Cargo.toml`:
- Around line 36-91: Change all caret-style version requirements in the
Cargo.toml dependency lists to exact pins by prefixing with '=' (e.g., change
actix = "0.13.5" to actix = "=0.13.5") for every dependency shown (actix,
actix-cors, actix-web, actix-web-actors, tokio-tungstenite, rmp-serde,
once_cell, uuid, async-std, binascii, bip_bencode, byteorder, chrono, clap,
fern, futures-util, hex, log, parking_lot, percent-encoding, rcgen, regex,
rustls, rustls-pemfile, sentry, sentry-actix, serde, serde_json, serde_millis,
sha1, socket2, sqlx, async-trait, redis, memcache, thiserror, tokio, rand,
base64, tokio-shutdown, toml, tracing, utoipa-swagger-ui, lazy_static,
crossbeam, smallvec, ahash, lz4_flex, zstd and the dev-dependencies tempfile,
criterion, mockall, proptest, reqwest) so versions are exact; also update any
feature tuples unchanged and ensure CI/release jobs are configured to use the
checked-in Cargo.lock for reproducible builds.
In `@docker/Dockerfile`:
- Line 6: The Dockerfile currently forces a git checkout of the remote tag via
the RUN line "RUN cd /app/torrust-actix && git checkout tags/v4.2.9", which
causes the image to package the remote tag instead of local sources; remove that
RUN checkout and instead rely on the build context (COPY the local repo into
/app/torrust-actix before build) or, if you must use git inside the image,
replace the checkout with a command that checks out the current HEAD/commit
(e.g., checkout the repo's provided branch or commit hash) so PR/local changes
are included; update the Dockerfile to remove the hardcoded tag checkout and
ensure COPY/ADD of local code happens prior to any build steps that use
/app/torrust-actix.
In `@README.md`:
- Around line 569-571: Update the v4.2.9 changelog entry: replace the awkward
phrase "cause of chain attacks" with a clearer supply-chain security phrase and
replace the vague '"safe" libs' wording with a concrete description (e.g.,
"locking dependency versions to mitigate supply‑chain risks" and
"updating/locking vulnerable third‑party libraries"). Edit the two bullets under
"#### v4.2.9" to read something like "Locking dependency versions to mitigate
supply‑chain risks" and "Bumping/locking third‑party library versions to address
known vulnerabilities" so the purpose is explicit.
In `@src/main.rs`:
- Around line 80-82: The code currently calls std::process::exit(0) inside the
async block when args.create_database is true, which bypasses Drop and prevents
cleanup (e.g. _sentry_guard); replace the exit(...) call with an early return
from the async block (return Ok(()) ) so the function can unwind normally after
awaiting task::sleep(Duration::from_secs(1)). Locate the branch checking
args.create_database and change the control flow to return Ok(()) instead of
calling exit(0), preserving the sleep await and allowing outer scope destructors
to run.
🪄 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: 4a7a249c-82f2-41d8-b418-a05d4a812f93
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdbenches/tracker_benchmarks.rsdocker/Dockerfiledocker/build.batsrc/database/impls/database_connector_mysql.rssrc/database/impls/database_connector_pgsql.rssrc/database/impls/database_connector_sqlite.rssrc/main.rstests/database_tests.rstorrust-actix.desktop
💤 Files with no reviewable changes (3)
- src/database/impls/database_connector_mysql.rs
- src/database/impls/database_connector_sqlite.rs
- src/database/impls/database_connector_pgsql.rs
Comment on lines
+36
to
+91
| actix = "0.13.5" | ||
| actix-cors = "0.7.1" | ||
| actix-web = { version = "4.13.0", features = ["rustls-0_23", "compress-gzip"] } | ||
| actix-web-actors = "4.3.1" | ||
| tokio-tungstenite = { version = "0.29.0", features = ["rustls-tls-webpki-roots"] } | ||
| rmp-serde = "1.3.1" | ||
| once_cell = "1.21.4" | ||
| uuid = { version = "1.23.1", features = ["v4"] } | ||
| async-std = "1.13.2" | ||
| binascii = "0.1.4" | ||
| bip_bencode = "0.4.4" | ||
| byteorder = "1.5.0" | ||
| chrono = "0.4.44" | ||
| clap = { version = "4.6.1", features = ["derive"] } | ||
| fern = { version = "0.7.1", features = ["colored"] } | ||
| futures-util = "0.3.32" | ||
| hex = "0.4.3" | ||
| log = "0.4.29" | ||
| parking_lot = { version = "0.12.5", features = ["arc_lock", "hardware-lock-elision", "serde", "deadlock_detection"] } | ||
| percent-encoding = "2.3.2" | ||
| rcgen = "0.14.7" | ||
| regex = "1.12.3" | ||
| rustls = { version = "0.23.37", default-features = false, features = ["std", "ring"] } | ||
| rustls-pemfile = "2.2.0" | ||
| sentry = { version = "0.48.1", default-features = false, features = ["rustls", "backtrace", "contexts", "panic", "transport", "debug-images", "reqwest"] } | ||
| sentry-actix = "0.48.1" | ||
| serde = { version = "1.0.228", features = ["derive"] } | ||
| serde_json = { version = "1.0.149", features = ["preserve_order"] } | ||
| serde_millis = "0.1.1" | ||
| sha1 = "0.11.0" | ||
| socket2 = "0.6.3" | ||
| sqlx = { version = "0.8.6", features = ["mysql", "postgres", "sqlite", "runtime-tokio-rustls"] } | ||
| async-trait = "0.1.89" | ||
| redis = { version = "1.2.1", features = ["tokio-comp", "connection-manager"] } | ||
| memcache = { version = "0.19.0", default-features = false } | ||
| thiserror = "2.0.18" | ||
| tokio = { version = "1.52.2", features = ["full"] } | ||
| rand = "0.10.1" | ||
| base64 = "0.22.1" | ||
| tokio-shutdown = "0.1.5" | ||
| toml = "1.1.2" | ||
| tracing = "0.1.44" | ||
| utoipa-swagger-ui = { version = "9.0.2", features = ["actix-web"] } | ||
| lazy_static = "1.5.0" | ||
| crossbeam = "0.8.4" | ||
| smallvec = { version = "1.15.1", features = ["union", "const_generics"] } | ||
| ahash = "0.8.12" | ||
| lz4_flex = { version = "0.13.0", default-features = false } | ||
| zstd = { version = "0.13.3", default-features = false } | ||
|
|
||
| [dev-dependencies] | ||
| tempfile = "^3.26" | ||
| criterion = { version = "^0.8", features = ["async_tokio"] } | ||
| mockall = "^0.14" | ||
| proptest = "^1.10" | ||
| reqwest = { version = "^0.13", default-features = false, features = ["rustls", "json"] } | ||
| tempfile = "3.27.0" | ||
| criterion = { version = "0.8.2", features = ["async_tokio"] } | ||
| mockall = "0.14.0" | ||
| proptest = "1.11.0" | ||
| reqwest = { version = "0.13.3", default-features = false, features = ["rustls", "json"] } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cargo.toml, does version = "1.2.3"pin exactly1.2.3, or is it a caret requirement equivalent to ^1.2.3? How do you express an exact version pin in Cargo?
💡 Result:
In Cargo.toml, version = "1.2.3" does not pin exactly to 1.2.3; it is a caret requirement equivalent to ^1.2.3, allowing any SemVer-compatible version >=1.2.3 and <2.0.0 [1][2]. To express an exact version pin, use the equals operator: version = "=1.2.3" [3][1].
Citations:
- 1: https://doc.rust-lang.org/1.69.0/cargo/reference/resolver.html
- 2: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
- 3: https://stackoverflow.com/questions/45224563/how-to-specify-the-exact-version-of-a-dependency
Use exact version pins if the goal is true dependency locking.
In Cargo, version = "1.2.3" is a caret requirement (equivalent to ^1.2.3), which allows SemVer-compatible updates up to the next major version. This does not pin exact versions. To lock dependencies precisely, use version = "=1.2.3" instead. Additionally, enforce reproducible builds by ensuring release builds use the checked-in lockfile.
Fix pattern
-actix = "0.13.5"
+actix = "=0.13.5"
-tokio = { version = "1.52.2", features = ["full"] }
+tokio = { version = "=1.52.2", features = ["full"] }
-serde = { version = "1.0.228", features = ["derive"] }
+serde = { version = "=1.0.228", features = ["derive"] }Apply across all modified dependencies.
🤖 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 `@Cargo.toml` around lines 36 - 91, Change all caret-style version requirements
in the Cargo.toml dependency lists to exact pins by prefixing with '=' (e.g.,
change actix = "0.13.5" to actix = "=0.13.5") for every dependency shown (actix,
actix-cors, actix-web, actix-web-actors, tokio-tungstenite, rmp-serde,
once_cell, uuid, async-std, binascii, bip_bencode, byteorder, chrono, clap,
fern, futures-util, hex, log, parking_lot, percent-encoding, rcgen, regex,
rustls, rustls-pemfile, sentry, sentry-actix, serde, serde_json, serde_millis,
sha1, socket2, sqlx, async-trait, redis, memcache, thiserror, tokio, rand,
base64, tokio-shutdown, toml, tracing, utoipa-swagger-ui, lazy_static,
crossbeam, smallvec, ahash, lz4_flex, zstd and the dev-dependencies tempfile,
criterion, mockall, proptest, reqwest) so versions are exact; also update any
feature tuples unchanged and ensure CI/release jobs are configured to use the
checked-in Cargo.lock for reproducible builds.
| RUN apk add git musl-dev curl pkgconfig openssl-dev openssl-libs-static --no-interactive | ||
| RUN git clone https://github.com/Power2All/torrust-actix.git /app/torrust-actix | ||
| RUN cd /app/torrust-actix && git checkout tags/v4.2.8 | ||
| RUN cd /app/torrust-actix && git checkout tags/v4.2.9 |
There was a problem hiding this comment.
Build the checked-out source instead of a remote tag.
This image no longer packages the code in the current checkout; it packages whatever v4.2.9 points to on GitHub. That makes PR builds depend on the tag existing and can silently miss local changes.
📦 Proposed fix
-RUN git clone https://github.com/Power2All/torrust-actix.git /app/torrust-actix
-RUN cd /app/torrust-actix && git checkout tags/v4.2.9
WORKDIR /app/torrust-actix
-RUN cd /app/torrust-actix
-RUN cargo build --release && rm -Rf target/release/.fingerprint target/release/build target/release/deps target/release/examples target/release/incremental
+COPY . .
+RUN cargo build --release --locked && rm -Rf target/release/.fingerprint target/release/build target/release/deps target/release/examples target/release/incremental🤖 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 `@docker/Dockerfile` at line 6, The Dockerfile currently forces a git checkout
of the remote tag via the RUN line "RUN cd /app/torrust-actix && git checkout
tags/v4.2.9", which causes the image to package the remote tag instead of local
sources; remove that RUN checkout and instead rely on the build context (COPY
the local repo into /app/torrust-actix before build) or, if you must use git
inside the image, replace the checkout with a command that checks out the
current HEAD/commit (e.g., checkout the repo's provided branch or commit hash)
so PR/local changes are included; update the Dockerfile to remove the hardcoded
tag checkout and ensure COPY/ADD of local code happens prior to any build steps
that use /app/torrust-actix.
Comment on lines
+569
to
+571
| #### v4.2.9 | ||
| * Bumping versions | ||
| * Locking versions, cause of chain attacks, making sure we use "safe" libs |
There was a problem hiding this comment.
Reword this changelog entry.
cause of chain attacks reads awkwardly, and safe libs is too vague for release notes. A clearer supply-chain phrasing would be better here.
✏️ Suggested wording
#### v4.2.9
* Bumping versions
-* Locking versions, cause of chain attacks, making sure we use "safe" libs
+* Locked dependency versions to reduce supply-chain risk and standardize on vetted library releases🧰 Tools
🪛 LanguageTool
[grammar] ~571-~571: Ensure spelling is correct
Context: ....9 * Bumping versions * Locking versions, cause of chain attacks, making sure we use "s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@README.md` around lines 569 - 571, Update the v4.2.9 changelog entry: replace
the awkward phrase "cause of chain attacks" with a clearer supply-chain security
phrase and replace the vague '"safe" libs' wording with a concrete description
(e.g., "locking dependency versions to mitigate supply‑chain risks" and
"updating/locking vulnerable third‑party libraries"). Edit the two bullets under
"#### v4.2.9" to read something like "Locking dependency versions to mitigate
supply‑chain risks" and "Bumping/locking third‑party library versions to address
known vulnerabilities" so the purpose is explicit.
Comment on lines
+80
to
+82
| if args.create_database { | ||
| task::sleep(Duration::from_secs(1)).await; | ||
| exit(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Rust, does std::process::exit(0)runDropdestructors, and is returning frommain/an async block the recommended way to exit after work is complete?
💡 Result:
No, std::process::exit(0) does not run Drop destructors. It immediately terminates the process without running destructors on the current stack or any other thread's stack [1][2][3]. The official Rust documentation explicitly states this and recommends avoiding it for clean shutdowns [1].
Citations:
- 1: https://doc.rust-lang.org/stable/std/process/fn.exit.html
- 2: https://doc.rust-lang.org/1.94.1/std/process/fn.exit.html
- 3: https://doc.rust-lang.org/std/process/fn.exit.html
🏁 Script executed:
cat -n src/main.rs | sed -n '60,95p'Repository: Power2All/torrust-actix
Length of output: 2231
🏁 Script executed:
cat -n src/main.rs | sed -n '1,80p'Repository: Power2All/torrust-actix
Length of output: 3329
🏁 Script executed:
cat -n src/main.rs | sed -n '150,220p'Repository: Power2All/torrust-actix
Length of output: 3433
🏁 Script executed:
cat -n src/main.rs | tail -50Repository: Power2All/torrust-actix
Length of output: 2547
Return from the async block instead of calling exit(0) here.
std::process::exit(0) immediately terminates the process without running any Drop destructors. This means _sentry_guard and other resources in the outer scope won't be cleaned up properly. Since this code is already inside the async block that returns Ok(()), use return Ok(()); instead to enable normal cleanup.
♻️ Proposed fix
if args.create_database {
task::sleep(Duration::from_secs(1)).await;
- exit(0);
+ return Ok(());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if args.create_database { | |
| task::sleep(Duration::from_secs(1)).await; | |
| exit(0); | |
| if args.create_database { | |
| task::sleep(Duration::from_secs(1)).await; | |
| return Ok(()); | |
| } |
🤖 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/main.rs` around lines 80 - 82, The code currently calls
std::process::exit(0) inside the async block when args.create_database is true,
which bypasses Drop and prevents cleanup (e.g. _sentry_guard); replace the
exit(...) call with an early return from the async block (return Ok(()) ) so the
function can unwind normally after awaiting task::sleep(Duration::from_secs(1)).
Locate the branch checking args.create_database and change the control flow to
return Ok(()) instead of calling exit(0), preserving the sleep await and
allowing outer scope destructors to run.
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.
v4.2.9
Summary by CodeRabbit
Chores
Security
Improvements