fix(persist): fall back to a direct write when session.json rename crosses a file system by vidhanio · Pull Request #2335 · herdrdev/herdr

vidhanio · 2026-08-05T00:02:56Z

Herdr saves session.json by writing a temp file next to it, then renaming the temp file onto the target. When session.json is a mounted file from another drive (e.g. a bind mount), the rename fails with ResourceBusy or CrossesDevices and the save is silently dropped (only a trace-log error, no user-visible message).

Fall back to a direct, non-atomic write when the rename fails for either of these reasons.

Verified with a real bind-mounted tmpfs file reproducing the issue, plus new unit tests.

…osses a file system

coderabbitai · 2026-08-05T00:03:06Z

Warning

Review limit reached

@vidhanio, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 63b57421-fd24-4c4e-98b3-f69a74fe9a30

📥 Commits

Reviewing files that changed from the base of the PR and between aceeafa and edeeb83.

📒 Files selected for processing (1)

src/persist/io.rs

📝 Walkthrough

Walkthrough

save_json_to_path now handles selected rename failures with direct writes. Session and history loading retry from temporary recovery files when primary files are missing or invalid. Tests cover error handling, cleanup, fallback retention, and snapshot recovery.

Changes

JSON persistence recovery

Layer / File(s)	Summary
Rename fallback and cleanup validation `src/persist/io.rs`	The save path handles `ResourceBusy` and `CrossesDevices` with direct writes. Other rename errors remain propagated. Tests verify temporary-file cleanup and retention.
Session and history recovery loading `src/persist/io.rs`	Session and history loading retry from `.json.tmp` files when primary files cannot be loaded. Tests verify recovery from invalid session JSON.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main persistence fix for rename failures across filesystems.
Description check	✅ Passed	The description directly explains the rename failure, direct-write fallback, recovery behavior, and tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-08-05T00:05:18Z

Greptile Summary

The PR adds a direct-write fallback for cross-filesystem session saves and recovery from retained temporary snapshots.

Retains a complete temporary snapshot when the direct fallback fails.
Loads temporary snapshots when the main snapshot is unavailable, while excluding unsupported newer versions.
Clears temporary recovery files alongside session and history files.

Confidence Score: 4/5

The PR is not yet safe to merge because a failed recovery-file deletion can still restore session state that the user explicitly cleared.

clear_path_and_tmp removes the main snapshot first, and if removing the temporary snapshot then fails, load_with_recovery treats that retained file as authoritative because the main file is absent.

Files Needing Attention: src/persist/io.rs

Important Files Changed

Filename	Overview
src/persist/io.rs	Adds direct-write and temporary-file recovery behavior, but clear_path_and_tmp can delete the main snapshot and then fail to delete the recovery snapshot, allowing explicitly cleared state to return.

_{Reviews (4): Last reviewed commit: "fix(persist): stop stale .tmp recovery f..." | Re-trigger Greptile}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 025fbb33-28ba-4266-a646-dd5dce717005

📥 Commits

Reviewing files that changed from the base of the PR and between 1f1e434 and 6dac644.

📒 Files selected for processing (1)

src/persist/io.rs

Only remove the temp file after the direct-write fallback actually succeeds, so a partial or failed overwrite leaves a recovery copy instead of losing the session. Also loosen the propagate-unrelated- errors test comment to not assume a specific platform error kind.

…le is invalid If the direct-write fallback truncates or corrupts session.json (or session-history.json) part way through, the good copy left behind in the matching .tmp file was never read back on load. load() and load_history() now fall back to the .tmp file when the main file is missing or fails to parse.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/persist/io.rs (1)

436-450: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise the public recovery branches in the tests.

The test calls try_load_snapshot directly. It does not execute load's primary-to-.tmp selection, and it provides no coverage for load_history's equivalent branch. Add tests for missing and invalid primary files through load() and load_history(), or extract a path-injected loader and test that selection logic directly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d0b255a7-56e4-4af9-ba98-ac7f7b9c6b8b

📥 Commits

Reviewing files that changed from the base of the PR and between 2abc0a9 and aceeafa.

📒 Files selected for processing (1)

src/persist/io.rs

…d or unsupported sessions clear() and clear_history() only removed the main file, leaving a stale .tmp behind that a later load() would treat as a recoverable snapshot and restore, even though the user had explicitly cleared the session. Fix by also removing the .tmp file on clear (and when a save disables history). Also stop treating a main file from a newer, unsupported herdr version as a recoverable failure: it is valid, just not understood yet, so falling back to a stale .tmp file for it could silently replace real data. load()/load_history() now go through a shared load_with_recovery() that only tries the .tmp file when the main file is genuinely missing or invalid.

greptile-apps · 2026-08-05T00:38:12Z

+fn clear_path_and_tmp(path: &Path) -> std::io::Result<()> {
+    clear_path(path)?;
+    clear_path(&path.with_extension("json.tmp"))
+}


Failed clear retains recoverable state

When removing the .json.tmp recovery file fails after the main snapshot has been deleted, clear_path_and_tmp leaves the temporary snapshot intact, causing the next load to restore session or history state that the user explicitly cleared.

Suggested change

fn clear_path_and_tmp(path: &Path) -> std::io::Result<()> {

clear_path(path)?;

clear_path(&path.with_extension("json.tmp"))

}

fn clear_path_and_tmp(path: &Path) -> std::io::Result<()> {

clear_path(&path.with_extension("json.tmp"))?;

clear_path(path)

}

Knowledge Base Used: Server Process and Headless Mode

ogulcancelik · 2026-08-05T02:20:39Z

this feels like a very niche weird edge case that become a 220 line of round fixes, i don't see why you would only mount the session-file but not the whole dir, so sorry but i'm going to close it.

vidhanio · 2026-08-05T09:38:16Z

I use impermanence with nix (wipe on reboot) and would prefer to only persist what is absolutely required, excluding other files in the herdr config. I have just realized that impermanence has a mount method = "symlink" which seems to be natively supported by herdr so this isn't actually needed to be fixed in herdr, sorry about that.

fix(persist): fall back to a direct write when session.json rename cr…

6dac644

…osses a file system

kangal-bot added the ai-review Trigger automated AI reviews for pull requests admitted by the PR gate label Aug 5, 2026

greptile-apps Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/persist/io.rs Outdated

coderabbitai Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/persist/io.rs Outdated

greptile-apps Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/persist/io.rs

greptile-apps Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/persist/io.rs Outdated

coderabbitai Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread src/persist/io.rs Outdated

greptile-apps Bot reviewed Aug 5, 2026

View reviewed changes

ogulcancelik closed this Aug 5, 2026

coderabbitai Bot mentioned this pull request Aug 5, 2026

fix: use process-unique temp names for session saves #2376

Closed

Navigation Menu

Uh oh!

fix(persist): fall back to a direct write when session.json rename crosses a file system - #2335

fix(persist): fall back to a direct write when session.json rename crosses a file system#2335
vidhanio wants to merge 4 commits into
herdrdev:masterfrom
vidhanio:fix/session-json-cross-device-rename

vidhanio commented Aug 5, 2026

Uh oh!

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

greptile-apps Bot commented Aug 5, 2026 •

edited

Loading

Important Files Changed

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

greptile-apps Bot Aug 5, 2026

Uh oh!

ogulcancelik commented Aug 5, 2026

Uh oh!

vidhanio commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vidhanio commented Aug 5, 2026

Uh oh!

coderabbitai Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

greptile-apps Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

ogulcancelik commented Aug 5, 2026

Uh oh!

vidhanio commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

greptile-apps Bot commented Aug 5, 2026 •

edited

Loading