fix(fs): crash-safe atomic writes for auth vault and shell profile files - #175
Conversation
auth.rs's activate_with/activate_into write the agent's LIVE credential file (~/.claude/.credentials.json etc.) via plain fs::write, and alias.rs's write_managed_block writes directly into the user's real shell rc file. Neither is crash-atomic: a process killed mid-write (OOM kill, SIGKILL, power loss) leaves a truncated or zero-byte file in place of the original — for auth.rs that's a corrupted credential file breaking the agent's login; for alias.rs it's data loss in the user's own .bashrc/.zshrc. Adds src/atomic_fs.rs (ported from lean-ctx's core/atomic_fs.rs): same-directory temp file + rename for crash-atomicity, with an in-place-overwrite fallback for read-only-directory/writable-inode cases. Wires it into both write sites. libc added as a unix-only dependency for the O_NOFOLLOW/errno pieces of the fallback path (both already conditionally compiled per-platform; inert on Windows).
📝 WalkthroughWalkthroughAdds crash-atomic file writing with an in-place fallback for read-only parent directories, then uses it for managed alias updates and encrypted or plaintext vault restoration. ChangesAtomic write integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Activation
participant write_bytes_with_fallback
participant try_atomic_write
participant in_place_overwrite
Activation->>write_bytes_with_fallback: restore file bytes
write_bytes_with_fallback->>try_atomic_write: write temp file and rename
try_atomic_write-->>write_bytes_with_fallback: read-only directory error
write_bytes_with_fallback->>in_place_overwrite: overwrite existing target
in_place_overwrite-->>Activation: return write result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
src/atomic_fs.rs (2)
205-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead-only-directory fallback test may silently pass without exercising the fallback when CI runs as root.
std::fs::set_permissions(dir.path(), 0o500)relies on the OS enforcing DAC permission checks. Root (common for containerized CI runners) bypasses this, sotry_atomic_writewould still succeed via the primary path,write_bytes_with_fallbackwould never take the fallback branch, andres.expect(...)would still pass — the test would give a false sense of coverage.Consider skipping (or asserting the fallback was actually exercised) when running as root, e.g. via
unsafe { libc::geteuid() == 0 }.🤖 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/atomic_fs.rs` around lines 205 - 219, Update fallback_overwrites_when_parent_dir_is_readonly to avoid false coverage when running as root: detect a zero effective UID and skip the test before changing permissions, or otherwise assert that the fallback path was exercised. Preserve the existing permission restoration and content assertions for non-root runs.
52-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win"Durable" guarantee isn't fully delivered: sync errors are swallowed and the parent directory is never fsynced.
f.sync_all()errors are discarded at Line 54 and Line 100, so a failed fsync (e.g. disk full) is treated as success and the code proceeds torenameanyway. Separately, even a successfulsync_all()on the temp file only durably persists file content/metadata — it does not make therename's directory-entry update durable. This is the well-known ext4 pattern; as one source puts it, "You should fsync the containing directory in addition to the file itself whenever you need the directory-level change (creation, removal, renaming, linking) to be durable across crashes or power loss." Right now a crash right after a "successful"rename()can still roll back to the previous file on some filesystems/configurations.🛡️ Proposed fix: fsync the parent directory after rename, propagate sync errors
if let Err(e) = std::fs::rename(&tmp, path) { let _ = std::fs::remove_file(&tmp); return Err(e); } + if let Ok(dir) = std::fs::File::open(parent) { + let _ = dir.sync_all(); + } Ok(())Also applies to: 68-73, 99-101
🤖 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/atomic_fs.rs` around lines 52 - 55, Update both atomic write paths around the file sync and rename operations to propagate errors from f.sync_all() instead of discarding them, preventing rename after an unsuccessful file sync. After each successful rename, open the destination’s parent directory and fsync it, propagating that directory sync error as well so the durable guarantee includes the directory entry update.
🤖 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/atomic_fs.rs`:
- Around line 61-66: Remove the Windows-only pre-removal block that checks
path.exists() and calls std::fs::remove_file(path) before the atomic rename. Let
the existing std::fs::rename flow replace the destination directly, preserving
the file throughout the operation.
- Around line 61-75: Update the atomic-write flow surrounding the final
std::fs::rename call to detect when path is a symlink before removing or
replacing it. Preserve symlink-managed files by resolving the link target and
performing the atomic replacement at that target, or reject symlink paths with a
consistent error; do not allow rename to replace the symlink itself with a
regular file.
- Around line 47-59: Update try_atomic_write to capture the existing target file
permissions before creating the temporary inode, and use those permissions when
the permissions argument is None. Preserve explicitly supplied permissions,
while ensuring atomic replacement of an existing file retains its current mode
bits instead of relying on default creation permissions.
---
Nitpick comments:
In `@src/atomic_fs.rs`:
- Around line 205-219: Update fallback_overwrites_when_parent_dir_is_readonly to
avoid false coverage when running as root: detect a zero effective UID and skip
the test before changing permissions, or otherwise assert that the fallback path
was exercised. Preserve the existing permission restoration and content
assertions for non-root runs.
- Around line 52-55: Update both atomic write paths around the file sync and
rename operations to propagate errors from f.sync_all() instead of discarding
them, preventing rename after an unsuccessful file sync. After each successful
rename, open the destination’s parent directory and fsync it, propagating that
directory sync error as well so the durable guarantee includes the directory
entry update.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a082013-474a-4bea-9d6d-e39cb4e861a1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/alias.rssrc/atomic_fs.rssrc/auth.rssrc/main.rs
…ndows rename) Three issues from review of the crash-safe write module: - try_atomic_write created the replacement inode with default (umask-masked) permissions when permissions=None, which every call site passes — a 0600 credential file being rewritten would silently become world-readable. Now preserves the existing file's permissions when none are explicitly given. - rename() replaces a symlink *path* with a regular file rather than writing through it, unlike the plain fs::write this module replaced. A symlinked .bashrc or credential file (common for dotfiles setups) would silently lose its symlink. Now resolves one level of symlink indirection first and performs the atomic write at the resolved target, matching the original write-through behavior while keeping the symlink itself intact. - Removed the Windows pre-removal-before-rename step: std::fs::rename already replaces an existing destination on Windows (MoveFileExW with MOVEFILE_REPLACE_EXISTING), so the explicit remove_file only opened a window where neither the old nor new file existed. New tests: permission preservation on replace, write-through-a-symlink.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomic_fs.rs (1)
90-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate
sync_all()failure instead of discarding it.
let _ = f.sync_all();swallows fsync errors (e.g.ENOSPC,EIO).flush()onFileis a documented no-op so ignoring it is harmless, but ignoringsync_all()'s result means the function can returnOk(())(after a subsequent successfulrename) even though the new content was never durably written — defeating the crash-safety guarantee this module explicitly documents.🔒 Proposed fix
f.write_all(bytes)?; - let _ = f.flush(); - let _ = f.sync_all(); + f.sync_all()?;Note: like the existing
write_allfailure path, an error here still leavestmpbehind (no cleanup happens before the?returns); consider unifying cleanup across all failure points inside this block if you want a stronger guarantee, though that's a separate, lower-priority concern.🤖 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/atomic_fs.rs` around lines 90 - 98, In the temporary-file write block, update the sync_all call on the OpenOptions-created file to propagate failures with the existing Result flow instead of discarding them. Preserve the current flush handling and ensure a sync_all error returns before rename, leaving the temporary file behavior consistent with write_all failures.
🤖 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/atomic_fs.rs`:
- Around line 15-45: Update write_bytes_with_fallback to resolve the input path
through resolve_symlink_target before calling in_place_overwrite. Pass the
resolved target to the fallback write while preserving the existing behavior for
non-symlinks and other write paths.
---
Outside diff comments:
In `@src/atomic_fs.rs`:
- Around line 90-98: In the temporary-file write block, update the sync_all call
on the OpenOptions-created file to propagate failures with the existing Result
flow instead of discarding them. Preserve the current flush handling and ensure
a sync_all error returns before rename, leaving the temporary file behavior
consistent with write_all failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8f0ab24-0329-41c5-a84c-06378053ecca
📒 Files selected for processing (1)
src/atomic_fs.rs
| use std::borrow::Cow; | ||
| use std::path::Path; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| fn invalid_input(msg: &'static str) -> std::io::Error { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidInput, msg) | ||
| } | ||
|
|
||
| /// Resolves one level of symlink indirection so an atomic write lands on the | ||
| /// real file `path` points at, not on a new plain file replacing the symlink | ||
| /// itself — `rename()` does not follow a symlink destination, it unlinks it | ||
| /// and puts the new file in its place. A relative link target is resolved | ||
| /// against the symlink's own parent directory. Non-symlinks (and symlinks | ||
| /// this can't stat, e.g. a dangling one) pass through unchanged. | ||
| fn resolve_symlink_target(path: &Path) -> Cow<'_, Path> { | ||
| let Ok(meta) = std::fs::symlink_metadata(path) else { | ||
| return Cow::Borrowed(path); | ||
| }; | ||
| if !meta.file_type().is_symlink() { | ||
| return Cow::Borrowed(path); | ||
| } | ||
| let Ok(target) = std::fs::read_link(path) else { | ||
| return Cow::Borrowed(path); | ||
| }; | ||
| if target.is_absolute() { | ||
| Cow::Owned(target) | ||
| } else { | ||
| let parent = path.parent().unwrap_or_else(|| Path::new(".")); | ||
| Cow::Owned(parent.join(target)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect in_place_overwrite / write_bytes_with_fallback for symlink handling.
sed -n '117,192p' src/atomic_fs.rsRepository: getappz/agentflare
Length of output: 2662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '47,116p' src/atomic_fs.rsRepository: getappz/agentflare
Length of output: 2651
Resolve the symlink before the fallback write
write_bytes_with_fallback still passes the original path into in_place_overwrite, and O_NOFOLLOW will reject symlinks on Unix. Because path.is_file() follows symlinks, writable targets behind a symlink can still reach this branch and fail; resolve the link here too.
🤖 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/atomic_fs.rs` around lines 15 - 45, Update write_bytes_with_fallback to
resolve the input path through resolve_symlink_target before calling
in_place_overwrite. Pass the resolved target to the fallback write while
preserving the existing behavior for non-symlinks and other write paths.
1 participant
Summary
auth.rs'sactivate_with/activate_intowrite the agent's live credential file (e.g.~/.claude/.credentials.json) via plainfs::write, andalias.rs'swrite_managed_blockwrites directly into the user's real shell rc file. Neither is crash-atomic: a process killed mid-write (OOM kill, SIGKILL, power loss) leaves a truncated or zero-byte file in place of the original.auth.rs: a truncated write corrupts the credential file the agent is actively using — breaks login immediately.alias.rs: a truncated write is data loss in the user's own.bashrc/.zshrc.Fix
Adds
src/atomic_fs.rs(ported from lean-ctx'score/atomic_fs.rs): a same-directory temp file +renamefor crash-atomicity, with an in-place-overwrite fallback for the read-only-directory/writable-inode case. Wires it into both write sites (activate_with,activate_into,write_managed_block).libcadded as a[target.'cfg(unix)'.dependencies]entry for theO_NOFOLLOW/errno pieces of the fallback path — both already conditionally compiled per-platform in the ported module, inert on Windows.Testing
cargo test --workspace— 368 passed, 0 failed (includes atomic_fs's own unit tests: atomic replace, in-place-overwrite, read-only-dir fallback, no-leftover-temp-file)cargo clippy --workspace --all-targets -- -D warnings -A unsafe_code -A clippy::pedantic— cleancargo fmt --check— cleanSummary by CodeRabbit