feat: improve concurrency safety, cross-platform support and dynamic versioning#6
feat: improve concurrency safety, cross-platform support and dynamic versioning#6Fahl-Design wants to merge 1 commit intomainfrom
Conversation
…versioning This change introduces: - File locking for shared state files using fs4 - Dynamic versioning with commit ID and build time via build.rs - Cross-platform support for macOS and ARM architectures - Session-specific environment updates using PVM_SHELL_PID - Safety guards and silent removal of temp files in shell wrappers - Centralized constants and semantic version sorting logic - Bug fixes in interactive menu and shell file discovery
Fahl-Design
force-pushed
the
feat/concurrency-and-versioning
branch
from
9d28092 to
e213c57
Compare
📝 WalkthroughWalkthroughThis PR refactors environment variable and file handling across the codebase by introducing centralized constants, implementing file locking for concurrent access, switching from HTML to JSON-based remote version listing, and adding a compile-time version tracking system via a new build script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
4-4:⚠️ Potential issue | 🟡 MinorEnsure CI and local toolchains support Rust 1.85.0 or later.
The
edition = "2024"requires rustc 1.85.0 (stable) or later. Verify your CI pipeline and development environment are configured to use this minimum version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 4, The Cargo.toml now sets edition = "2024" which requires rustc 1.85.0+; update your toolchain configuration and CI to pin/use Rust 1.85.0 or later (e.g., update any rust-toolchain, GitHub Actions workflow steps, CI images, and Dockerfiles) so local and CI builds match the new edition; verify rustup/toolchain settings and CI matrix entries reference 1.85.0+ and rerun the pipeline to confirm compatibility.
🧹 Nitpick comments (3)
build.rs (2)
20-20: CI detection is limited to GitHub Actions.The
GITHUB_ACTIONScheck excludes other CI environments (GitLab CI, CircleCI, Jenkins, etc.). Consider using the more universalCIenvironment variable, which is set by most CI systems.💡 Proposed broader CI detection
- let is_ci = std::env::var("GITHUB_ACTIONS").is_ok(); + // CI env var is set by most CI systems (GitHub Actions, GitLab CI, CircleCI, Travis, etc.) + let is_ci = std::env::var("CI").is_ok() || std::env::var("GITHUB_ACTIONS").is_ok();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` at line 20, The CI detection currently sets is_ci by checking GITHUB_ACTIONS only; update it to detect a wider range of CI environments by checking the generic "CI" env var (and optionally fall back to other CI-specific vars) instead of or in addition to "GITHUB_ACTIONS" so is_ci becomes true for most CI systems; locate the declaration of the is_ci variable and replace the std::env::var("GITHUB_ACTIONS").is_ok() expression with a check for std::env::var("CI").is_ok() (or a small OR chain of known CI vars) to broaden detection.
3-41: Addcargo:rerun-if-changeddirectives for build efficiency.Without rerun directives, this build script executes on every compilation, invoking
gitanddatecommands unnecessarily. Adding directives improves incremental build times.⚡ Proposed fix to add rerun directives
fn main() { + // Only rerun if git state or Cargo.toml changes + println!("cargo:rerun-if-changed=.git/HEAD"); + println!("cargo:rerun-if-changed=.git/refs/heads/"); + println!("cargo:rerun-if-changed=Cargo.toml"); + let commit_hash = Command::new("git")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` around lines 3 - 41, In main(), add cargo rerun directives so the build script only reruns when relevant inputs change: emit println!("cargo:rerun-if-changed=.git/HEAD") and println!("cargo:rerun-if-changed=.git/refs/heads") (or other git files you rely on) to avoid invoking git/date on every compile, and emit environment-change directives like println!("cargo:rerun-if-env-changed=GITHUB_ACTIONS") and println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION") so changes to those vars or package version trigger a rerun; place these near the top of main() before computing commit_hash, build_time, and version.src/utils.rs (1)
10-18: Mixed semver/non-semver comparison may produce inconsistent ordering.When one string is valid semver and the other is not, both are compared using the numeric fallback, discarding valid semver metadata. For example, comparing
"8.2.0-alpha"(valid semver with prerelease) against"8.2"(invalid semver) uses the fallback, yielding[8,2,0]vs[8,2], treating the prerelease as greater—contradicting semver semantics where8.2.0-alpha < 8.2.0.Consider handling the mixed case explicitly or documenting this limitation.
💡 Proposed fix to handle mixed case
match (a_sem, b_sem) { (Ok(av), Ok(bv)) => av.cmp(&bv), + (Ok(_), Err(_)) => std::cmp::Ordering::Greater, // valid semver sorts after non-semver + (Err(_), Ok(_)) => std::cmp::Ordering::Less, _ => { // Fallback for non-semver strings (e.g., "8.2") let a_parts: Vec<u32> = a.split('.').filter_map(|s| s.parse().ok()).collect(); let b_parts: Vec<u32> = b.split('.').filter_map(|s| s.parse().ok()).collect(); a_parts.cmp(&b_parts) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.rs` around lines 10 - 18, The current match over (a_sem, b_sem) falls through to the numeric fallback for mixed semver/non-semver cases and loses semver prerelease semantics; update the match to handle mixed cases explicitly by adding arms for (Ok(av), Err(_)) and (Err(_), Ok(bv)): when one side parsed as semver, split the non-semver string into numeric parts and compare major/minor/patch from av (use av.major,av.minor,av.patch) to those numeric parts; if numeric parts are equal but av has a prerelease, treat the semver prerelease as less than the non-semver “release” (i.e., return Less), and mirror the logic for the reversed case using bv, ensuring a and b variables and the existing match block are used to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.php-version:
- Line 1: The repository includes a committed .php-version file which is
machine-specific and can confuse contributors or cause merge conflicts; remove
the tracked .php-version from the repo (git rm --cached .php-version or delete
and commit), add ".php-version" to .gitignore to prevent re-committing, and if
keeping it intentionally, add a short note in the project README explaining why
this specific PHP version is required; reference the .php-version file when
making these changes.
In `@build.rs`:
- Around line 12-18: The current build_time computation uses
Command::new("date") (see build_time variable in build.rs) which is POSIX-only
and breaks on Windows; replace this shell call with a cross-platform timestamp
from Rust (add chrono as a build-dependency and compute build_time via
chrono::Utc::now().format(...).to_string()) so the timestamp is produced
reliably on all platforms and remove the Command::new("date") logic.
In `@src/commands/install.rs`:
- Around line 81-94: The current install path in install.rs swallows all I/O
errors when writing the env-update file (using .ok() on lock_exclusive,
writeln!, writer.flush, and unlock), causing a reported "successful switch" even
when the write fails; change this block to treat the write as fallible like
src/commands/use_cmd.rs: use crate::fs::get_env_update_path() as you already do,
but propagate errors (replace the .ok() calls with proper Result handling and
use ? to return the error or map it into the function's error type) for
file.open(), file.lock_exclusive(), writeln!(...), writer.flush(), and
file.unlock() so the install command returns an error if the env-update write
fails.
In `@src/commands/use_cmd.rs`:
- Around line 92-98: The code currently calls std::fs::write(PHP_VERSION_FILE,
&version).ok() and then unconditionally prints a success eprintln; change this
to check the Result from std::fs::write (e.g., match or if let Err(e)) so you
only print the success message when write returns Ok, and print or warn
(eprintln) with the error and mention PHP_VERSION_FILE when it fails so the user
knows the pin update was not written; update the write call handling and the
eprintln block around PHP_VERSION_FILE and version.bold() accordingly.
In `@src/fs.rs`:
- Around line 64-72: get_env_update_path currently rebuilds the .env_update
filename from PVM_SHELL_PID which still collides for concurrent backgrounded
invocations; instead accept a per-invocation path from the caller. Change the
signature of get_env_update_path (and any callers in src/shell.rs) to accept an
Option<PathBuf> or &Path (e.g., get_env_update_path(override: Option<&Path>))
and if provided return that path immediately, otherwise fall back to the
existing pvm_dir + PVM_SHELL_PID logic; update the shell wrapper code that
spawns each invocation to create a unique temp filename and pass that PathBuf
into get_env_update_path (or set and pass through an explicit env var like
PVM_ENV_UPDATE_PATH) so each invocation uses a unique .env_update file instead
of reconstructing from PVM_SHELL_PID.
In `@src/network.rs`:
- Around line 109-116: The cache file is currently truncated by File::create
before acquiring the exclusive lock, creating a race; replace the
File::create(&cache_path) call with opening via std::fs::OpenOptions
(create=true, read=true, write=true) so the file is not truncated on open, then
call file.lock_exclusive(), call file.set_len(0) to clear the file while holding
the lock, then create the std::io::BufWriter from the locked file and
write_all/flush, and finally call file.unlock(); keep references to
serde_json::to_string(&versions), pvm_dir, cache_path, lock_exclusive, set_len,
BufWriter, write_all, flush, and unlock when making the change.
In `@src/update.rs`:
- Around line 18-45: The guard file is being truncated at open which makes
read_to_string() always return empty; remove truncate(true) from the OpenOptions
chain used to open the file (the let file =
std::fs::OpenOptions::new()...open(&guard_file)? block) so the existing
timestamp can be read, then after deciding to update, call file.set_len(0) and
rewind before writing the new timestamp (the current set_len(0),
BufWriter::new(&file), writeln!(writer, "{}", now) and writer.flush() sequence)
and keep the existing lock_exclusive()/unlock() usage around the
read-and-possibly-write sequence.
---
Outside diff comments:
In `@Cargo.toml`:
- Line 4: The Cargo.toml now sets edition = "2024" which requires rustc 1.85.0+;
update your toolchain configuration and CI to pin/use Rust 1.85.0 or later
(e.g., update any rust-toolchain, GitHub Actions workflow steps, CI images, and
Dockerfiles) so local and CI builds match the new edition; verify
rustup/toolchain settings and CI matrix entries reference 1.85.0+ and rerun the
pipeline to confirm compatibility.
---
Nitpick comments:
In `@build.rs`:
- Line 20: The CI detection currently sets is_ci by checking GITHUB_ACTIONS
only; update it to detect a wider range of CI environments by checking the
generic "CI" env var (and optionally fall back to other CI-specific vars)
instead of or in addition to "GITHUB_ACTIONS" so is_ci becomes true for most CI
systems; locate the declaration of the is_ci variable and replace the
std::env::var("GITHUB_ACTIONS").is_ok() expression with a check for
std::env::var("CI").is_ok() (or a small OR chain of known CI vars) to broaden
detection.
- Around line 3-41: In main(), add cargo rerun directives so the build script
only reruns when relevant inputs change: emit
println!("cargo:rerun-if-changed=.git/HEAD") and
println!("cargo:rerun-if-changed=.git/refs/heads") (or other git files you rely
on) to avoid invoking git/date on every compile, and emit environment-change
directives like println!("cargo:rerun-if-env-changed=GITHUB_ACTIONS") and
println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION") so changes to those
vars or package version trigger a rerun; place these near the top of main()
before computing commit_hash, build_time, and version.
In `@src/utils.rs`:
- Around line 10-18: The current match over (a_sem, b_sem) falls through to the
numeric fallback for mixed semver/non-semver cases and loses semver prerelease
semantics; update the match to handle mixed cases explicitly by adding arms for
(Ok(av), Err(_)) and (Err(_), Ok(bv)): when one side parsed as semver, split the
non-semver string into numeric parts and compare major/minor/patch from av (use
av.major,av.minor,av.patch) to those numeric parts; if numeric parts are equal
but av has a prerelease, treat the semver prerelease as less than the non-semver
“release” (i.e., return Less), and mirror the logic for the reversed case using
bv, ensuring a and b variables and the existing match block are used to locate
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 632cbd06-1e14-494d-8cb9-2e803f4dbca3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.php-versionCargo.tomlbuild.rssrc/cli.rssrc/commands/env.rssrc/commands/install.rssrc/commands/use_cmd.rssrc/constants.rssrc/fs.rssrc/main.rssrc/network.rssrc/shell.rssrc/update.rssrc/utils.rstests/cli.rs
| @@ -0,0 +1 @@ | |||
| 8.4.18 No newline at end of file | |||
There was a problem hiding this comment.
Consider removing .php-version from the repository.
This file is typically generated per-project by the pvm init or pvm use commands. Committing it to the version manager's own repository may:
- Confuse users who clone the repo, as
pvmmight try to use PHP 8.4.18 when working on the tool itself. - Create merge conflicts if developers have different local PHP versions.
Consider adding .php-version to .gitignore instead, or document why it's intentionally included.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.php-version at line 1, The repository includes a committed .php-version
file which is machine-specific and can confuse contributors or cause merge
conflicts; remove the tracked .php-version from the repo (git rm --cached
.php-version or delete and commit), add ".php-version" to .gitignore to prevent
re-committing, and if keeping it intentionally, add a short note in the project
README explaining why this specific PHP version is required; reference the
.php-version file when making these changes.
| let build_time = Command::new("date") | ||
| .args(["-u", "+%Y-%m-%dT%H:%M:%SZ"]) | ||
| .output() | ||
| .ok() | ||
| .and_then(|output| String::from_utf8(output.stdout).ok()) | ||
| .map(|s| s.trim().to_string()) | ||
| .unwrap_or_else(|| "unknown".to_string()); |
There was a problem hiding this comment.
date command is not cross-platform (Windows incompatible).
The date -u +%Y-%m-%dT%H:%M:%SZ command is POSIX-specific and will fail on Windows, falling back to "unknown". Since the PR aims to improve cross-platform support, consider using Rust's standard library for timestamp generation.
🛠️ Proposed cross-platform fix
- let build_time = Command::new("date")
- .args(["-u", "+%Y-%m-%dT%H:%M:%SZ"])
- .output()
- .ok()
- .and_then(|output| String::from_utf8(output.stdout).ok())
- .map(|s| s.trim().to_string())
- .unwrap_or_else(|| "unknown".to_string());
+ // Cross-platform timestamp using chrono or manual formatting
+ let build_time = {
+ use std::time::SystemTime;
+ SystemTime::now()
+ .duration_since(SystemTime::UNIX_EPOCH)
+ .map(|d| {
+ let secs = d.as_secs();
+ // Simple UTC timestamp (consider using chrono for proper formatting)
+ format!("{}", secs)
+ })
+ .unwrap_or_else(|_| "unknown".to_string())
+ };Alternatively, add the chrono crate as a build dependency for proper ISO 8601 formatting:
[build-dependencies]
chrono = "0.4"let build_time = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string();📝 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.
| let build_time = Command::new("date") | |
| .args(["-u", "+%Y-%m-%dT%H:%M:%SZ"]) | |
| .output() | |
| .ok() | |
| .and_then(|output| String::from_utf8(output.stdout).ok()) | |
| .map(|s| s.trim().to_string()) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| // Cross-platform timestamp using chrono or manual formatting | |
| let build_time = { | |
| use std::time::SystemTime; | |
| SystemTime::now() | |
| .duration_since(SystemTime::UNIX_EPOCH) | |
| .map(|d| { | |
| let secs = d.as_secs(); | |
| // Simple UTC timestamp (consider using chrono for proper formatting) | |
| format!("{}", secs) | |
| }) | |
| .unwrap_or_else(|_| "unknown".to_string()) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.rs` around lines 12 - 18, The current build_time computation uses
Command::new("date") (see build_time variable in build.rs) which is POSIX-only
and breaks on Windows; replace this shell call with a cross-platform timestamp
from Rust (add chrono as a build-dependency and compute build_time via
chrono::Utc::now().format(...).to_string()) so the timestamp is produced
reliably on all platforms and remove the Command::new("date") logic.
| if let Ok(env_file) = crate::fs::get_env_update_path() { | ||
| // Atomic write with advisory lock | ||
| if let Ok(file) = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .write(true) | ||
| .truncate(true) | ||
| .open(&env_file) | ||
| { | ||
| file.lock_exclusive().ok(); | ||
| let mut writer = std::io::BufWriter::new(&file); | ||
| writeln!(writer, "{}\n{}", export_str1, export_str2).ok(); | ||
| writer.flush().ok(); | ||
| file.unlock().ok(); | ||
| } |
There was a problem hiding this comment.
Don't swallow env-update I/O failures.
If this block fails, the install still reports a successful switch, but the wrapper never receives the exports and the parent shell stays on the old PHP. src/commands/use_cmd.rs already treats the same write as fallible; this path should too.
Suggested fix
- if let Ok(env_file) = crate::fs::get_env_update_path() {
- // Atomic write with advisory lock
- if let Ok(file) = std::fs::OpenOptions::new()
- .create(true)
- .write(true)
- .truncate(true)
- .open(&env_file)
- {
- file.lock_exclusive().ok();
- let mut writer = std::io::BufWriter::new(&file);
- writeln!(writer, "{}\n{}", export_str1, export_str2).ok();
- writer.flush().ok();
- file.unlock().ok();
- }
- }
+ let env_file = crate::fs::get_env_update_path()?;
+ let file = std::fs::OpenOptions::new()
+ .create(true)
+ .write(true)
+ .truncate(true)
+ .open(&env_file)?;
+ file.lock_exclusive()?;
+ let mut writer = std::io::BufWriter::new(&file);
+ writeln!(writer, "{}", export_str1)?;
+ writeln!(writer, "{}", export_str2)?;
+ writer.flush()?;
+ file.unlock()?;📝 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.
| if let Ok(env_file) = crate::fs::get_env_update_path() { | |
| // Atomic write with advisory lock | |
| if let Ok(file) = std::fs::OpenOptions::new() | |
| .create(true) | |
| .write(true) | |
| .truncate(true) | |
| .open(&env_file) | |
| { | |
| file.lock_exclusive().ok(); | |
| let mut writer = std::io::BufWriter::new(&file); | |
| writeln!(writer, "{}\n{}", export_str1, export_str2).ok(); | |
| writer.flush().ok(); | |
| file.unlock().ok(); | |
| } | |
| let env_file = crate::fs::get_env_update_path()?; | |
| let file = std::fs::OpenOptions::new() | |
| .create(true) | |
| .write(true) | |
| .truncate(true) | |
| .open(&env_file)?; | |
| file.lock_exclusive()?; | |
| let mut writer = std::io::BufWriter::new(&file); | |
| writeln!(writer, "{}", export_str1)?; | |
| writeln!(writer, "{}", export_str2)?; | |
| writer.flush()?; | |
| file.unlock()?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/install.rs` around lines 81 - 94, The current install path in
install.rs swallows all I/O errors when writing the env-update file (using .ok()
on lock_exclusive, writeln!, writer.flush, and unlock), causing a reported
"successful switch" even when the write fails; change this block to treat the
write as fallible like src/commands/use_cmd.rs: use
crate::fs::get_env_update_path() as you already do, but propagate errors
(replace the .ok() calls with proper Result handling and use ? to return the
error or map it into the function's error type) for file.open(),
file.lock_exclusive(), writeln!(...), writer.flush(), and file.unlock() so the
install command returns an error if the env-update write fails.
| std::fs::write(PHP_VERSION_FILE, &version).ok(); | ||
| eprintln!( | ||
| "{} Updated {} to {}", | ||
| "✓".green(), | ||
| PHP_VERSION_FILE, | ||
| version.bold() | ||
| ); |
There was a problem hiding this comment.
Don't print a successful pin update when the write is ignored.
If .php-version cannot be written, this still prints a success message and the directory pin silently stays stale. At least warn on failure instead of unconditionally reporting success.
Suggested fix
- std::fs::write(PHP_VERSION_FILE, &version).ok();
- eprintln!(
- "{} Updated {} to {}",
- "✓".green(),
- PHP_VERSION_FILE,
- version.bold()
- );
+ match std::fs::write(PHP_VERSION_FILE, &version) {
+ Ok(()) => eprintln!(
+ "{} Updated {} to {}",
+ "✓".green(),
+ PHP_VERSION_FILE,
+ version.bold()
+ ),
+ Err(e) => eprintln!(
+ "{} Failed to update {}: {}",
+ "✗".red(),
+ PHP_VERSION_FILE,
+ e
+ ),
+ }📝 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.
| std::fs::write(PHP_VERSION_FILE, &version).ok(); | |
| eprintln!( | |
| "{} Updated {} to {}", | |
| "✓".green(), | |
| PHP_VERSION_FILE, | |
| version.bold() | |
| ); | |
| match std::fs::write(PHP_VERSION_FILE, &version) { | |
| Ok(()) => eprintln!( | |
| "{} Updated {} to {}", | |
| "✓".green(), | |
| PHP_VERSION_FILE, | |
| version.bold() | |
| ), | |
| Err(e) => eprintln!( | |
| "{} Failed to update {}: {}", | |
| "✗".red(), | |
| PHP_VERSION_FILE, | |
| e | |
| ), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/use_cmd.rs` around lines 92 - 98, The code currently calls
std::fs::write(PHP_VERSION_FILE, &version).ok() and then unconditionally prints
a success eprintln; change this to check the Result from std::fs::write (e.g.,
match or if let Err(e)) so you only print the success message when write returns
Ok, and print or warn (eprintln) with the error and mention PHP_VERSION_FILE
when it fails so the user knows the pin update was not written; update the write
call handling and the eprintln block around PHP_VERSION_FILE and version.bold()
accordingly.
| pub fn get_env_update_path() -> Result<PathBuf> { | ||
| let pvm_dir = get_pvm_dir()?; | ||
| let shell_pid = std::env::var("PVM_SHELL_PID").unwrap_or_default(); | ||
| let filename = if shell_pid.is_empty() { | ||
| crate::constants::ENV_UPDATE_FILE.to_string() | ||
| } else { | ||
| format!("{}_{}", crate::constants::ENV_UPDATE_FILE, shell_pid) | ||
| }; | ||
| Ok(pvm_dir.join(filename)) |
There was a problem hiding this comment.
PVM_SHELL_PID still collides across concurrent commands in the same shell.
Every backgrounded pvm invocation from one shell shares the same PID, so both src/fs.rs::get_env_update_path() and the wrappers in src/shell.rs still target the same .env_update_<pid> file. The later command can overwrite or remove the first command's update before it is sourced. Pass a per-invocation path from the wrapper instead of reconstructing it from the shell PID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fs.rs` around lines 64 - 72, get_env_update_path currently rebuilds the
.env_update filename from PVM_SHELL_PID which still collides for concurrent
backgrounded invocations; instead accept a per-invocation path from the caller.
Change the signature of get_env_update_path (and any callers in src/shell.rs) to
accept an Option<PathBuf> or &Path (e.g., get_env_update_path(override:
Option<&Path>)) and if provided return that path immediately, otherwise fall
back to the existing pvm_dir + PVM_SHELL_PID logic; update the shell wrapper
code that spawns each invocation to create a unique temp filename and pass that
PathBuf into get_env_update_path (or set and pass through an explicit env var
like PVM_ENV_UPDATE_PATH) so each invocation uses a unique .env_update file
instead of reconstructing from PVM_SHELL_PID.
| if let Ok(json) = serde_json::to_string(&versions) { | ||
| std::fs::create_dir_all(&pvm_dir).ok(); | ||
| if let Ok(mut file) = File::create(&cache_path) { | ||
| file.write_all(json.as_bytes()).ok(); | ||
| if let Ok(file) = File::create(&cache_path) { | ||
| file.lock_exclusive().ok(); | ||
| let mut writer = std::io::BufWriter::new(&file); | ||
| writer.write_all(json.as_bytes()).ok(); | ||
| writer.flush().ok(); | ||
| file.unlock().ok(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, does std::fs::File::create(path) truncate an existing file immediately on open?
💡 Result:
Yes. std::fs::File::create(path) creates the file if it doesn’t exist, and if it does exist it truncates it to length 0 as part of successfully opening it (i.e., immediately on open, before you write anything). This is equivalent to opening with write access plus create(true) and truncate(true). [1] [2]
Sources:
[1] Rust std docs: std::fs::File::create (“will truncate it if it does”) (doc.rust-lang.org)
[2] Rust std docs: std::fs::OpenOptions::truncate (“If a file is successfully opened … it will truncate the file to 0 length”) (doc.rust-lang.org)
Citations:
- 1: https://doc.rust-lang.org/std/fs/struct.File.html?utm_source=openai
- 2: https://doc.rust-lang.org/std/fs/struct.OpenOptions.html?utm_source=openai
The cache write truncates before the lock is acquired.
File::create zeroes remote_cache.json before lock_exclusive() runs, leaving a window where concurrent readers/writers observe an empty or partial cache. The lock does not protect the destructive truncation. Open the file without truncation, acquire the lock, then clear and rewrite the contents.
Suggested fix
- if let Ok(file) = File::create(&cache_path) {
+ if let Ok(file) = std::fs::OpenOptions::new()
+ .create(true)
+ .write(true)
+ .open(&cache_path)
+ {
file.lock_exclusive().ok();
+ file.set_len(0).ok();
let mut writer = std::io::BufWriter::new(&file);
writer.write_all(json.as_bytes()).ok();
writer.flush().ok();
file.unlock().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.
| if let Ok(json) = serde_json::to_string(&versions) { | |
| std::fs::create_dir_all(&pvm_dir).ok(); | |
| if let Ok(mut file) = File::create(&cache_path) { | |
| file.write_all(json.as_bytes()).ok(); | |
| if let Ok(file) = File::create(&cache_path) { | |
| file.lock_exclusive().ok(); | |
| let mut writer = std::io::BufWriter::new(&file); | |
| writer.write_all(json.as_bytes()).ok(); | |
| writer.flush().ok(); | |
| file.unlock().ok(); | |
| if let Ok(json) = serde_json::to_string(&versions) { | |
| std::fs::create_dir_all(&pvm_dir).ok(); | |
| if let Ok(file) = std::fs::OpenOptions::new() | |
| .create(true) | |
| .write(true) | |
| .open(&cache_path) | |
| { | |
| file.lock_exclusive().ok(); | |
| file.set_len(0).ok(); | |
| let mut writer = std::io::BufWriter::new(&file); | |
| writer.write_all(json.as_bytes()).ok(); | |
| writer.flush().ok(); | |
| file.unlock().ok(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/network.rs` around lines 109 - 116, The cache file is currently truncated
by File::create before acquiring the exclusive lock, creating a race; replace
the File::create(&cache_path) call with opening via std::fs::OpenOptions
(create=true, read=true, write=true) so the file is not truncated on open, then
call file.lock_exclusive(), call file.set_len(0) to clear the file while holding
the lock, then create the std::io::BufWriter from the locked file and
write_all/flush, and finally call file.unlock(); keep references to
serde_json::to_string(&versions), pvm_dir, cache_path, lock_exclusive, set_len,
BufWriter, write_all, flush, and unlock when making the change.
| let file = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .read(true) | ||
| .write(true) | ||
| .truncate(true) | ||
| .open(&guard_file)?; | ||
|
|
||
| file.lock_exclusive()?; | ||
|
|
||
| let mut contents = String::new(); | ||
| let mut f = &file; | ||
| f.read_to_string(&mut contents).ok(); | ||
|
|
||
| // Check if 24 hours have passed | ||
| let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); | ||
| if guard_file.exists() | ||
| && let Ok(contents) = std::fs::read_to_string(&guard_file) | ||
| if !contents.is_empty() | ||
| && let Ok(last_check) = contents.trim().parse::<u64>() | ||
| && now - last_check < 86400 | ||
| { | ||
| file.unlock().ok(); | ||
| return Ok(None); | ||
| } | ||
|
|
||
| // Write the new timestamp to prevent spam on next commands | ||
| std::fs::write(&guard_file, now.to_string()).ok(); | ||
| file.set_len(0).ok(); | ||
| let mut writer = std::io::BufWriter::new(&file); | ||
| writeln!(writer, "{}", now).ok(); | ||
| writer.flush().ok(); | ||
| file.unlock().ok(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, does std::fs::OpenOptions::new().truncate(true).open(path) truncate the file immediately before any reads from that handle?
💡 Result:
Yes—if the open succeeds with truncation enabled, the file is truncated as part of the open() operation (before you ever get the File handle back), so any reads from that handle will see an empty file (EOF). Rust’s docs say: “If a file is successfully opened with this option set to true, it will truncate the file to 0 length if it already exists.” [1]. This matches the usual OS semantics of O_TRUNC, which truncates “upon successful completion” of the open call [2].
Two important details for your exact code:
truncate(true)only works when opened with write access (“The file must be opened with write access for truncate to work.”) [1].- Your snippet
OpenOptions::new().truncate(true).open(path)doesn’t request read or write access at all, so it won’t do what you intend.
If you do:
OpenOptions::new()
.read(true)
.write(true)
.truncate(true)
.open(path)then the truncation happens during open, and you cannot read the old contents afterward. [1]
Sources: [1] Rust std::fs::OpenOptions::truncate docs, [2] Open Group/POSIX open (O_TRUNC) semantics.
The update guard never throttles because the file is truncated before it is read.
OpenOptions::truncate(true) clears the file as part of the open() call, so subsequent read_to_string() always reads an empty file. The 24-hour short-circuit check cannot activate. Remove truncate(true) from the open call, then truncate and rewind the file only after deciding to persist the new timestamp.
Suggested fix
-use std::io::{Read, Write};
+use std::io::{Read, Seek, Write};
...
let file = std::fs::OpenOptions::new()
.create(true)
.read(true)
.write(true)
- .truncate(true)
.open(&guard_file)?;
...
- f.read_to_string(&mut contents).ok();
+ f.read_to_string(&mut contents)?;
...
- file.set_len(0).ok();
- let mut writer = std::io::BufWriter::new(&file);
- writeln!(writer, "{}", now).ok();
- writer.flush().ok();
- file.unlock().ok();
+ file.set_len(0)?;
+ let mut f = &file;
+ f.rewind()?;
+ let mut writer = std::io::BufWriter::new(f);
+ writeln!(writer, "{}", now)?;
+ writer.flush()?;
+ file.unlock()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/update.rs` around lines 18 - 45, The guard file is being truncated at
open which makes read_to_string() always return empty; remove truncate(true)
from the OpenOptions chain used to open the file (the let file =
std::fs::OpenOptions::new()...open(&guard_file)? block) so the existing
timestamp can be read, then after deciding to update, call file.set_len(0) and
rewind before writing the new timestamp (the current set_len(0),
BufWriter::new(&file), writeln!(writer, "{}", now) and writer.flush() sequence)
and keep the existing lock_exclusive()/unlock() usage around the
read-and-possibly-write sequence.
1 participant
Summary of Changes
1. Shell Update & Session Isolation (Root Cause Fix)
pvm envcommand now explicitly exportsPVM_DIR, ensuring the shell wrapper can correctly locate its configuration files..env_update_1234). This prevents race conditions and "version bleeding" between different terminal tabs.rmorcatoperations on invalid paths.command rmand redirected all temporary file cleanup output to/dev/null.2. Concurrency & Reliability
fs4crate to implement advisory file locking on shared state files (.env_update,.update_check_guard,remote_cache.json). This ensures data integrity when multiple PVM instances run simultaneously.network.rsto use the remote JSON API instead of HTML scraping. This is faster, more robust, and allowed for the removal of thescraperdependency.3. Cross-Platform Support
std::env::consts). PVM now natively supports macOS (Intel/Silicon) and ARM64 Linux (e.g., Raspberry Pi).4. Modernization & Maintainability
build.rsscript that injects the current Git commit ID and build timestamp into the version string.semvercrate for standard-compliant version parsing and sorting, replacing brittle manual string splitting.src/constants.rsfile.scraper,regex,thiserror,log).Summary by CodeRabbit
Release Notes
New Features
Improvements