Repository navigation
fix(api): make atomic renames durable - #6942
Conversation
Per CLAUDE.md prose-wrapping convention, since this paragraph was directly edited by this PR.
PR #6942 fixed atomic_write's crash-durability gap but shipped without a changelog.d/ fragment per the repo's fragment-per-entry convention.
houko
left a comment
There was a problem hiding this comment.
Automated daily review. Durability logic (fsync temp file → rename → fsync parent dir on Unix) is correctly ordered and I found no crash-consistency/data-loss bug in the core fix. Pushed one mechanical fix (missing changelog fragment) and left two inline notes needing human judgment: a commit with Claude <noreply@anthropic.com> author identity that I can't fix without a disallowed force-push, and a question about the new error-propagation behavior after a successful rename.
Generated by Claude Code
| /// per-process monotonic counter so concurrent writers never share a staging | ||
| /// file. The file is `sync_all`-ed before the rename so a power loss between | ||
| /// the two syscalls does not leave a zero-byte file in place of the original. | ||
| /// The temp file receives a unique name derived from the process ID and a per-process monotonic counter so concurrent writers never share a staging file. |
There was a problem hiding this comment.
Commit 099e7cd ("docs(api): rewrap atomic_write doc comment to one sentence per line") has its author identity set to Claude <noreply@anthropic.com>. CLAUDE.md's commit-msg hook is specifically designed to reject commits whose author identity (GIT_AUTHOR_IDENT) resolves to Claude/Anthropic even when the message text itself is clean — this looks like it slipped past that check (possibly pushed through a path that bypasses local hooks). The message body has no attribution text, so this wasn't caught by a text scan, only by author identity.
I can't fix this myself: correcting it means rewriting this commit (changing its author and therefore its SHA), which would require a force-push to update the already-pushed branch — disallowed for me per the repo's automation rules. Recommend the PR author reword/re-author this commit (git commit --amend --author=... or squash it into the other commit) before merge.
Generated by Claude Code
| let parent = path.parent().ok_or_else(|| { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidInput, "missing parent directory") | ||
| })?; | ||
| std::fs::File::open(parent)?.sync_all()?; |
There was a problem hiding this comment.
Durability logic itself looks correct: sync the temp file's data before rename, rename, then sync the parent directory after rename — that's the right order for surviving a crash.
One judgment call worth a second look: std::fs::File::open(parent)?.sync_all()? runs after std::fs::rename has already succeeded, so a failure here (e.g. EIO, a restrictive sandbox/container fs, or a filesystem where directory fsync isn't supported) now surfaces as an Err from atomic_write even though the data was already durably renamed into place — just not guaranteed to survive a crash. Previously this function never failed after a successful rename. That's arguably the right tradeoff (durability is the whole point of this PR), but it's a behavior change for every one of atomic_write's many callers (config writes, skill/vault persistence, upload metadata, etc.) — worth confirming none of them treat this Err as "the write was lost" and do something destructive like re-attempting from stale in-memory state, versus just logging/retrying the sync.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Reviewed the durability fix. Ordering (temp fsync → rename → parent-dir fsync) is correct and matches the PR's stated intent. Left two comments: one on whether the same fix should extend to the independent atomic_write in sidecar_toml.rs (explicitly declared out of scope, but same defect class in the same crate), and one on test coverage — the added test only covers the pre-existing happy path, not the new directory-fsync/error-propagation behavior this PR introduces.
Separately, flagging for the PR author: two commits already on this branch (099e7cd, 46c3967) have author identity Claude <noreply@anthropic.com>, which trips this repo's own commit-msg attribution check on author identity. That's already called out in an existing unresolved thread on this PR, so not re-raising it as new — just noting it's still outstanding and blocks a clean merge as-is.
Generated by Claude Code
| /// The temp file receives a unique name derived from the process ID and a per-process monotonic counter so concurrent writers never share a staging file. | ||
| /// The file is `sync_all`-ed before the rename. | ||
| /// On Unix, the parent directory is synced after the rename so the new directory entry is durable. | ||
| pub(crate) fn atomic_write(path: &std::path::Path, content: &[u8]) -> std::io::Result<()> { |
There was a problem hiding this comment.
Scope check: crates/librefang-api/src/routes/sidecar_toml.rs has its own private atomic_write (temp-file write + rename, no post-rename directory fsync) used for sidecar-channel config writes. It has the identical durability gap this PR fixes here, and the doc comment on that function even says "Same defect class as secrets_env::upsert_secret (T3.1)" — so this class of bug is already tracked as recurring in more than one spot. The PR body lists this under "Out of scope: independent atomic-write helpers in other modules/crates", which is a reasonable call for keeping this PR tight, but worth confirming explicitly: is there a follow-up planned for sidecar_toml.rs (and secrets_env::upsert_secret), or should they be pulled into this PR since they live in the same crate and same durability-guarantee class the changelog entry is about to advertise?
Generated by Claude Code
| atomic_write(&path, b"new content").expect("atomic write"); | ||
|
|
||
| assert_eq!(std::fs::read(&path).expect("read result"), b"new content"); | ||
| let entries: Vec<_> = std::fs::read_dir(dir.path()) |
There was a problem hiding this comment.
The new test only covers the happy path (content replaced, no stray tmp file left behind) — it doesn't exercise anything this PR actually added. It would still pass unmodified if the #[cfg(unix)] block were deleted entirely.
Two gaps worth closing before merge, in order of value:
- No test asserts that a failure in the new directory-open/sync step actually propagates as
Err(the PR body explicitly calls out "propagate directory-open and directory-sync failures instead of reporting a durable write prematurely" as a goal, but nothing pins that behavior down). - No test asserts call ordering (temp
sync_all→rename→ parentsync_all), which is the actual property being fixed.
A caveat on (1): the obvious way to force the directory-open to fail is chmod'ing the parent directory unreadable/unsearchable and asserting Err, but that only works when the test runs as a non-root user — root (and some CI containers run tests as root, per this repo's own Dockerfile.rust-dev verification path) bypasses DAC permission checks entirely, so the same test would silently take the success branch there and the assertion would fail for an unrelated reason. If you go this route, gate it with a runtime uid check (skip/log rather than assert when running as root) rather than assuming non-root, or find a failure mode that doesn't depend on the test's own privilege level.
Generated by Claude Code
atomic_write's post-rename directory-fsync step used path.parent() to
find the directory to open, but parent() returns Some("") rather than
None for a bare relative filename with no directory component (e.g.
"config.toml"), so the intended None fallback never ran and
File::open("") failed with ENOENT after the rename had already
succeeded.
Treat an empty-but-present parent the same as the current directory
so the fsync targets the real containing directory instead of an
empty path.
houko
left a comment
There was a problem hiding this comment.
Reviewed the durable-write mechanism carefully since this is called out as the base helper for #6944/#6945/#6947/#6948. Core mechanism (same-directory temp file, sync_all before rename, atomic rename, then directory fsync on Unix) is correct — that's the standard POSIX durable-rename shape, and no stray temp file is left on any error path I traced. Two inline notes on the error-propagation trade-off and test coverage.
One more finding not tied to a code line: two commits on this branch (46c3967b9, 099e7cd5a) have author identity Claude <noreply@anthropic.com> (git log --format='%an <%ae>'). Per this repo's own CLAUDE.md, the commit-msg git hook is specifically designed to reject "a commit whose author identity … resolves to Claude / Anthropic even when the message itself is clean" — these commit messages are clean, but the identity match is exactly what that hook exists to catch, and no GitHub-side check in this PR's check-run list currently enforces it (CHANGELOG Attribution only checks the changelog fragment's (@user) suffix, which is correctly @houko here). Worth a maintainer decision on whether to rewrite those two commits' authorship before merge — not something I'm fixing directly since that requires a history rewrite/force-push to someone else's branch.
Generated by Claude Code
| Some(p) if !p.as_os_str().is_empty() => p, | ||
| _ => std::path::Path::new("."), | ||
| }; | ||
| std::fs::File::open(parent)?.sync_all()?; |
There was a problem hiding this comment.
Mechanism itself looks right (temp file in the same directory for a same-filesystem rename, sync_all() on the staged file before rename, then rename, then this directory fsync on Unix) — that's the correct POSIX durable-rename pattern.
One thing worth a maintainer decision rather than a drive-by fix: File::open(parent)?.sync_all()? runs after std::fs::rename has already succeeded, so a failure here (directory not readable, fsync returning EINVAL on a filesystem/overlay that doesn't support directory fsync, etc.) causes atomic_write to return Err even though the new content is already live at path. I checked the callers (routes/budget.rs, routes/config/system.rs, routes/config/manage.rs, routes/users.rs, routes/providers.rs, webhook_store.rs, routes/agents/uploads.rs) and every one of them treats any Err from atomic_write as a hard failure — several return 500 Internal Server Error to the operator. Before this PR, atomic_write returning Ok meant "the rename committed"; now it can return Err for a step that runs strictly after the rename committed, so an operator can see "write failed" for an edit that actually saved.
The PR body says this is intentional ("propagate directory-open and directory-sync failures instead of reporting a durable write prematurely"), which I think is the right call for durability correctness, but I don't see evidence the callers were audited for the new "succeeded-but-reported-as-failed" UX (e.g. the config-save 500 handler logs and discards the io error entirely, so the operator has no way to tell "your config is fine, but full crash-durability wasn't confirmed" apart from a server log line). Given this is the base helper 3-4 sibling PRs are said to build on, worth confirming explicitly one way or the other before it propagates.
Generated by Claude Code
| // succeeded. | ||
| let parent = match path.parent() { | ||
| Some(p) if !p.as_os_str().is_empty() => p, | ||
| _ => std::path::Path::new("."), |
There was a problem hiding this comment.
The only test added exercises the happy path with an absolute tempdir path (dir.path().join("config.toml")), so path.parent() is always Some(<non-empty>) here — it never hits the Path::new(".") fallback on line 122. That fallback is exactly the branch that commit 366dc36 had to add on top of this PR's initial version, to fix a real ENOENT-after-a-successful-rename bug for bare relative filenames. As written, a regression back to the pre-366dc361 state (path.parent() mishandled for a single-component relative path) would not be caught by this test suite.
Since exercising that branch through atomic_write directly would require mutating the process's current directory (flaky under parallel test execution), consider pulling the parent-resolution logic out into its own small, pure function, e.g.:
fn dir_to_fsync(path: &std::path::Path) -> &std::path::Path {
match path.parent() {
Some(p) if !p.as_os_str().is_empty() => p,
_ => std::path::Path::new("."),
}
}That can be unit-tested directly against Path::new("config.toml") → Path::new("."), Path::new("/config.toml") → Path::new("/"), and Path::new("sub/config.toml") → Path::new("sub") with zero filesystem I/O, and it would have caught the original bug this PR's own follow-up commit had to fix.
Separately (not this PR's fault, just flagging since the task explicitly asked me to look for it): none of the tests here simulate a crash/partial-write — only the happy path is covered. That's understandable given true crash-durability isn't really unit-testable, but worth having in mind for CLAUDE.md's "MANDATORY: Integration Testing" bar on the sibling PRs that build on this helper.
Generated by Claude Code
* fix(api): durably persist registry content * docs(changelog): add fragment for durable registry content write fix PR #6984 fixes a real TOCTOU race and adds rollback-on-reject behavior for registry content writes but was missing the changelog.d fragment other PRs in this durable-write series (#6942, #6944, #6945, #6947, #6948) already carry. --------- Co-authored-by: Evan <tonymo2048@gmail.com>
Summary
This shared helper backs config, provider, webhook, budget, user, and upload-metadata writes.
Verification
cargo test -p librefang-api atomic_write_tests --libcargo clippy -p librefang-api --lib -- -D warningscargo fmt --checkgit diff --checkOut of scope