Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a file-based cross-process locking mechanism and integrates it into package installation; updates error types and exports, adjusts install logic to acquire locks with retry and re-checks to avoid double-installation, and adds a nix Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Installer
participant Lock as FileLock
participant FS as File System
participant DB as CoreRepository
participant Worker as Install Task
Client->>Lock: acquire(package_name) (block, retry 500ms)
activate Lock
Lock->>FS: open/create .lock file
FS-->>Lock: file handle
Lock->>FS: flock exclusive
FS-->>Lock: lock acquired
deactivate Lock
Client->>DB: is_installed(package_name)?
activate DB
DB-->>Client: installed? (yes/no)
deactivate DB
alt not installed
Client->>Worker: perform installation
activate Worker
Worker-->>Client: install_result (install_dir)
deactivate Worker
else already installed
Client-->>Client: log & skip installation
end
Client->>Lock: drop FileLock (unlock on drop)
activate Lock
Lock->>FS: release lock
deactivate Lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Fix all issues with AI agents
In `@crates/soar-cli/src/install.rs`:
- Around line 1094-1116: The loop in install_single_package uses
std::thread::sleep which blocks the tokio worker thread; replace that blocking
call with an awaitable tokio::time::sleep and make the surrounding
lock-acquisition loop async-aware so you can .await the sleep. Locate the loop
that calls FileLock::try_acquire and change
std::thread::sleep(Duration::from_millis(500)) to
tokio::time::sleep(Duration::from_millis(500)).await, ensuring
install_single_package remains async and any error mapping around the loop (the
.map_err and SoarError::Custom handling) is preserved.
- Around line 1108-1116: The code double-wraps the same message: the Err(err)
arm constructs SoarError::Custom("Failed to acquire package lock: {err}") and
then the chained .map_err(...) wraps it again. Remove the trailing .map_err(|e|
SoarError::Custom(format!("Failed to acquire package lock: {}", e))) so the Err
arm’s SoarError::Custom is returned directly (or alternatively, change the Err
arm to propagate the original error and keep the map_err—pick one); reference
the Err(err) arm and the .map_err call around the package-lock acquisition and
ensure only a single SoarError::Custom formatting is performed.
In `@crates/soar-utils/src/lock.rs`:
- Around line 46-57: The current sanitize closure in lock.rs maps all non
[a-zA-Z0-9._-] chars to '_' which can cause distinct package names (e.g.,
"pkg/foo" vs "pkg@foo") to collide; update the lock filename generation to
append or replace the sanitized name with a short collision-resistant digest
(e.g., a hex or base32 hash of the original package name) so filenames remain
filesystem-safe but unique — locate the sanitize closure and the code that
builds the lock filename and change it to include a hash of the original string
(or use the hash as the filename with the sanitized name as a prefix) to avoid
collisions while preserving readability.
- Around line 135-138: The Drop implementation for FileLock currently unlinks
the lock file (fs::remove_file) which creates a race where a new path can be
created while another process still holds the inode lock; remove the unlink from
FileLock::drop so the lock file is not deleted on drop (leave the file on disk
after releasing the flock), or implement the alternative verification: after
acquiring the flock in the FileLock constructor/lock method, compare the locked
fd's inode+device via fstat with the path's stat and retry if they differ;
update references to FileLock, Drop::drop, and any lock-acquisition code paths
used by install.rs accordingly.
- Around line 25-40: The fallback in lock_dir() uses temp_dir which is
world-shared; change lock_dir() so that when XDG_RUNTIME_DIR is None you append
a per-user component (e.g., "soar-{uid}") to the temp_dir before joining "locks"
to isolate users; obtain the current UID (via libc::getuid() or a crate helper)
and incorporate it into the base path used when std::env::var("XDG_RUNTIME_DIR")
is None, then create that directory as before (fs::create_dir_all) so locks are
placed under a user-specific temp subdirectory.
🧹 Nitpick comments (3)
crates/soar-utils/src/lock.rs (1)
74-91:acquire()blocks indefinitely with no timeout.
Flock::lockwithLockExclusivewill block forever if the holding process hangs or crashes without releasing. Consider documenting this behavior, or providing an optional timeout (e.g., converting to atry_acquireloop with a maximum attempt count, similar to howinstall.rsalready retries).crates/soar-cli/src/install.rs (2)
1094-1116: No upper bound on lock retry attempts — can spin indefinitely.If a lock is held indefinitely (e.g., a crashed process that left a stale
flock— though flock is released on process exit, edge cases exist), this loop never terminates. Consider adding a maximum attempt count or total timeout, and returning an error if exceeded.
1042-1050: Using an emptyPathBufas a sentinel for "skipped" is fragile.Returning
PathBuf::new()to signal "not actually installed" couples the caller to checkingis_empty()on the path. A more explicit approach would be to wrap the return type in anOptionor a dedicated enum, making the "skipped" case unambiguous.
| fn lock_dir() -> LockResult<PathBuf> { | ||
| let xdg_runtime = std::env::var("XDG_RUNTIME_DIR").ok(); | ||
| let base = if let Some(ref runtime) = xdg_runtime { | ||
| PathBuf::from(runtime) | ||
| } else { | ||
| std::env::temp_dir() | ||
| }; | ||
|
|
||
| let lock_dir = base.join("soar").join("locks"); | ||
|
|
||
| if !lock_dir.exists() { | ||
| fs::create_dir_all(&lock_dir)?; | ||
| } | ||
|
|
||
| Ok(lock_dir) | ||
| } |
There was a problem hiding this comment.
Fallback lock directory in /tmp is world-writable — potential for cross-user interference.
When XDG_RUNTIME_DIR is unset, the lock directory falls back to <temp_dir>/soar/locks (typically /tmp/soar/locks). Any user can create or hold lock files there, which could cause:
- A denial-of-service where another user holds the lock indefinitely
- Permission errors if the directory was created by a different user
Since soar is a per-user package manager, consider including the UID in the fallback path (e.g., /tmp/soar-{uid}/locks) to isolate users.
Proposed fix
fn lock_dir() -> LockResult<PathBuf> {
let xdg_runtime = std::env::var("XDG_RUNTIME_DIR").ok();
let base = if let Some(ref runtime) = xdg_runtime {
PathBuf::from(runtime)
} else {
- std::env::temp_dir()
+ let uid = nix::unistd::getuid();
+ std::env::temp_dir().join(format!("soar-{}", uid))
};
- let lock_dir = base.join("soar").join("locks");
+ let lock_dir = base.join("soar").join("locks");🤖 Prompt for AI Agents
In `@crates/soar-utils/src/lock.rs` around lines 25 - 40, The fallback in
lock_dir() uses temp_dir which is world-shared; change lock_dir() so that when
XDG_RUNTIME_DIR is None you append a per-user component (e.g., "soar-{uid}") to
the temp_dir before joining "locks" to isolate users; obtain the current UID
(via libc::getuid() or a crate helper) and incorporate it into the base path
used when std::env::var("XDG_RUNTIME_DIR") is None, then create that directory
as before (fs::create_dir_all) so locks are placed under a user-specific temp
subdirectory.
There was a problem hiding this comment.
@QaidVoid, understood. I'll defer to your judgment on this.
(\_/)
( •_•)
/ > ✓
| // Sanitize the package name to ensure a valid filename | ||
| let sanitize = |s: &str| { | ||
| s.chars() | ||
| .map(|c| { | ||
| if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' { | ||
| c | ||
| } else { | ||
| '_' | ||
| } | ||
| }) | ||
| .collect::<String>() | ||
| }; |
There was a problem hiding this comment.
Sanitization could cause lock collisions between distinct package names.
Characters outside [a-zA-Z0-9._-] are all mapped to _, so e.g. "pkg/foo" and "pkg@foo" would both produce "pkg_foo.lock". If package names can contain such characters, consider using a hash-based filename or a more collision-resistant encoding.
🤖 Prompt for AI Agents
In `@crates/soar-utils/src/lock.rs` around lines 46 - 57, The current sanitize
closure in lock.rs maps all non [a-zA-Z0-9._-] chars to '_' which can cause
distinct package names (e.g., "pkg/foo" vs "pkg@foo") to collide; update the
lock filename generation to append or replace the sanitized name with a short
collision-resistant digest (e.g., a hex or base32 hash of the original package
name) so filenames remain filesystem-safe but unique — locate the sanitize
closure and the code that builds the lock filename and change it to include a
hash of the original string (or use the hash as the filename with the sanitized
name as a prefix) to avoid collisions while preserving readability.
There was a problem hiding this comment.
It's fine. Package names shouldn't usually contain those characters.
There was a problem hiding this comment.
@QaidVoid, understood! I've noted that for future reference.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/soar-cli/src/install.rs`:
- Line 1096: The lock currently calls
FileLock::try_acquire(&target.package.pkg_name) which only uses pkg_name; change
it to use a composite key built from the package identity used elsewhere
(repo_name, pkg_name, pkg_id) so locks are unique per package instance. Locate
the call to FileLock::try_acquire in install.rs (the line with
FileLock::try_acquire(&target.package.pkg_name)) and replace the argument with a
formatted composite string like format!("{repo}:{name}:{id}") using
target.repository.repo_name (or equivalent), target.package.pkg_name, and
target.package.pkg_id to construct the lock key. Ensure the composite string is
deterministic and used wherever package-level locking is expected so different
repos with the same pkg_name do not contend on the same lock.
🧹 Nitpick comments (2)
crates/soar-utils/src/lock.rs (1)
74-91:acquire()blocks indefinitely with no timeout.The blocking
Flock::lockcall has no timeout mechanism. If a peer process stalls (not crashes — the kernel reclaims on exit), callers will hang forever. The install path ininstall.rswisely usestry_acquirein a polling loop, butacquireis apubmethod. Consider either documenting the no-timeout semantics prominently or providing aacquire_timeout(Duration)variant.crates/soar-cli/src/install.rs (1)
1042-1050: Using an emptyPathBufas a sentinel for "skipped" is fragile.The skip signal from
install_single_packagereturning(PathBuf::new(), Vec::new())(line 1143) is detected here viainstall_dir.as_os_str().is_empty(). This works but couples two distant code sites via an implicit contract on an empty path. A more explicit approach would be to change the return type (e.g.,Option<(PathBuf, Vec<…>)>whereNone= skipped), which would make the skip case self-documenting and compiler-enforced.
|
|
||
| let mut lock_attempts = 0; | ||
| let _package_lock = loop { | ||
| match FileLock::try_acquire(&target.package.pkg_name) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the FileLock implementation
find . -name "lock.rs" -o -name "*lock*" | grep -E "\.rs$" | head -10Repository: pkgforge/soar
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the install.rs context around line 1096
sed -n '1085,1110p' crates/soar-cli/src/install.rsRepository: pkgforge/soar
Length of output: 968
🏁 Script executed:
#!/bin/bash
# Search for FileLock::try_acquire usage to understand the pattern
rg -n 'FileLock::try_acquire' --type rust -B2 -A2Repository: pkgforge/soar
Length of output: 1125
🏁 Script executed:
#!/bin/bash
# Check if pkg_name is unique across repos or if composite keys are used elsewhere
rg -n '(repo_name|pkg_name|pkg_id)' crates/soar-utils/src/ --type rust | head -30Repository: pkgforge/soar
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Read the FileLock implementation
cat -n crates/soar-utils/src/lock.rsRepository: pkgforge/soar
Length of output: 6806
🏁 Script executed:
#!/bin/bash
# Search for package structures and how repo_name is associated with pkg_name
rg -n 'pub struct Package|struct Package' --type rust -A10 crates/soar-utils/src/Repository: pkgforge/soar
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if pkg_name is guaranteed unique across repos or if there's documentation
rg -n 'pkg_name.*unique|unique.*pkg_name' --type rust -iRepository: pkgforge/soar
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for how packages are stored/queried to understand if pkg_name is unique within repos only
rg -n 'repo_name.*pkg_name|pkg_name.*repo_name' --type rust -B2 -A2 | head -50Repository: pkgforge/soar
Length of output: 3306
🏁 Script executed:
#!/bin/bash
# Check the apply.rs file around the context mentioned in the learning
sed -n '140,160p' crates/soar-cli/src/apply.rsRepository: pkgforge/soar
Length of output: 698
🏁 Script executed:
#!/bin/bash
# Search for package query patterns to see how uniqueness is handled
rg -n 'WHERE|SELECT' --type rust -B1 -A1 | grep -E 'repo_name|pkg_name|pkg_id' | head -20Repository: pkgforge/soar
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or tests showing packages with same name from different repos
rg -n 'different.*repo|multiple.*repo' --type rust -i -C2 | head -30Repository: pkgforge/soar
Length of output: 1896
Consider using a composite lock key if same package names can exist across different repositories.
The lock uses only pkg_name, but packages throughout the codebase are identified by the composite tuple (repo_name, pkg_name, pkg_id). Database queries and deduplication logic all use this composite key pattern. If the same pkg_name exists in different repositories, they'll unnecessarily serialize on a shared lock file. If this scenario is rare/acceptable, it's fine—but for consistency and to prevent unnecessary contention, consider using a composite key like format!("{}:{}:{}", repo_name, pkg_name, pkg_id) for the lock.
🤖 Prompt for AI Agents
In `@crates/soar-cli/src/install.rs` at line 1096, The lock currently calls
FileLock::try_acquire(&target.package.pkg_name) which only uses pkg_name; change
it to use a composite key built from the package identity used elsewhere
(repo_name, pkg_name, pkg_id) so locks are unique per package instance. Locate
the call to FileLock::try_acquire in install.rs (the line with
FileLock::try_acquire(&target.package.pkg_name)) and replace the argument with a
formatted composite string like format!("{repo}:{name}:{id}") using
target.repository.repo_name (or equivalent), target.package.pkg_name, and
target.package.pkg_id to construct the lock key. Ensure the composite string is
deterministic and used wherever package-level locking is expected so different
repos with the same pkg_name do not contend on the same lock.
There was a problem hiding this comment.
Even though the different pkg_id or repo_name is used, they might symlink as a same binary resulting in conflicts.
There was a problem hiding this comment.
@QaidVoid, that's a great point! You're absolutely right. Using pkg_name as the lock key makes sense because different package variants (different pkg_id or repo_name) that share the same pkg_name will ultimately create symlinks with the same binary names in the bin directory. If they installed concurrently with a more granular lock, they'd race on those symlink creations.
The current implementation correctly serializes installations at the pkg_name level to prevent these symlink conflicts. Thank you for the clarification!
✏️ Learnings added
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 154
File: crates/soar-cli/src/install.rs:1096-1096
Timestamp: 2026-02-11T16:23:54.342Z
Learning: In crates/soar-cli/src/install.rs, FileLock uses only pkg_name (not a composite key with repo_name/pkg_id) intentionally. Different package variants with the same pkg_name but different pkg_id or repo_name will symlink to the same binary names in the bin directory. Locking at the pkg_name level prevents symlink race conditions during concurrent installations of different variants.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 137
File: crates/soar-cli/src/apply.rs:149-153
Timestamp: 2026-01-09T16:14:21.516Z
Learning: In crates/soar-cli/src/apply.rs, for URL packages, passing non-installed records (is_installed=false) as existing_install is intentional. The query filters by repo_name, pkg_name, and pkg_id, ensuring the record is for the same package. This allows resuming partial installs and preserving settings (portable paths, profile) from previous installs.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores