fix(api): make sidecar config writes durable by houko · Pull Request #6945 · librefang/librefang

houko · 2026-08-12T08:51:30Z

Summary

fsync sidecar config staging files before atomically renaming them
fsync the parent directory on Unix so the renamed config entry is durable
clean up staging files after write or rename failures and avoid truncating an existing staging path

Testing

cargo test -p librefang-api sidecar_toml::tests --lib
cargo clippy -p librefang-api --lib -- -D warnings
cargo fmt --all -- --check
git diff --check

houko

Automated review: two findings left as inline comments (missing changelog.d fragment; possible duplication with the #6942 shared atomic_write helper and #6944's secrets_env writer). No commits pushed to this branch.

Generated by Claude Code

houko · 2026-08-12T09:07:03Z

@@ -150,8 +151,33 @@ static TMP_SEQ: AtomicU64 = AtomicU64::new(0);
 fn atomic_write(path: &Path, contents: &str) -> Result<(), String> {


Missing changelog fragment: CLAUDE.md requires a new file under changelog.d/ per PR (not an edit to CHANGELOG.md), and this PR's diff has no fragment, unlike sibling PRs #6942 and #6944 which each added one under changelog.d/fixed/. Suggest adding changelog.d/fixed/6945-sidecar-config-durable-write.md, ending with (#6945) (@houko).

Generated by Claude Code

houko · 2026-08-12T09:07:08Z

@@ -150,8 +151,33 @@ static TMP_SEQ: AtomicU64 = AtomicU64::new(0);
 fn atomic_write(path: &Path, contents: &str) -> Result<(), String> {


This atomic_write (open+create_new, write, sync_all, rename, fsync parent dir, cleanup on failure) duplicates the pattern that #6942 is extracting into a shared pub(crate) fn atomic_write in crates/librefang-api/src/lib.rs ("This shared helper backs config, provider, webhook, budget, user, and upload-metadata writes"), and is structurally identical to the inline copy in #6944's secrets_env::upsert_secret — this file's own doc comment even says "Same defect class as secrets_env::upsert_secret (T3.1)". Flagging rather than changing myself because: (1) #6942 is unmerged and its PR body explicitly scopes out "independent atomic-write helpers in other modules/crates", so it's unclear whether unifying sidecar's writer is in scope yet; (2) this module's atomic_write returns Result<(), String> and shares TMP_SEQ across both upsert_sidecar_block and remove_sidecar_block (see the comment on TMP_SEQ), whereas the lib.rs helper is io::Result-typed with its own private counter — reconciling those isn't a pure call-site swap. Worth a maintainer call on whether to converge onto the shared helper now or track it as a follow-up once #6942 lands.

Generated by Claude Code

houko

One additional finding on top of the two threads already open on this PR (missing changelog fragment — since resolved by a follow-up push; atomic_write duplication with #6942/#6944). Everything else in the diff checks out: fsync-before-rename and fsync-parent-dir-after-rename is the correct durability ordering, cleanup on both write and rename failure paths is correct, the new unit tests are sound, rustfmt is clean, and cargo check -p librefang-api --lib compiles without errors.

Generated by Claude Code

houko · 2026-08-12T09:19:50Z

+    #[cfg(unix)]
+    fs::File::open(parent)
+        .and_then(|dir| dir.sync_all())
+        .map_err(|e| format!("sync parent directory {parent:?}: {e}"))?;


Once fs::rename on line 171 succeeds, the on-disk config.toml already reflects the new sidecar block — that part of the write is done and visible to any subsequent read. If the parent-directory sync_all() here then fails (e.g. an unusual filesystem, a permissions hiccup, or an FS that rejects directory fsync), atomic_write still returns Err, and both callers (configure_channel / delete_sidecar_channel in routes/channels.rs) turn that into a 500 via ApiErrorResponse::internal_scrub. The operator sees "internal server error" and will likely retry, even though the config change already landed — only the crash-durability confirmation failed, not the write itself.

For upsert_sidecar_block this is low-risk since retries are idempotent by name, but for remove_sidecar_block a caller could see a false failure on a removal that already took effect, which is more confusing (dashboard says "failed to remove", GET shows it gone). Might be worth distinguishing "durability unconfirmed" from "write failed" in the error, or at minimum logging the directory-fsync failure as a WARN and still returning Ok(()) since the rename — the operation that matters for correctness — already succeeded. Flagging for a maintainer call rather than changing the behavior myself, since picking between "fail loud" and "warn and proceed" is a product/ops decision, not just a bug fix.

Generated by Claude Code

houko

Reviewed the durability pattern and consistency against sibling PRs #6942/#6944/#6947/#6948; one issue flagged inline.

Generated by Claude Code

houko · 2026-08-12T09:30:34Z

Commit 346cd4b ("docs: add missing changelog fragment for #6945") has author and committer identity Claude <noreply@anthropic.com> (git log --format='%an|%ae|%cn|%ce' confirms both). That's the exact pattern the commit-msg hook's author-identity check exists to reject (CLAUDE.md: "separately rejects a commit whose author identity ... resolves to Claude / Anthropic even when the message itself is clean"), and the project rule is no AI/Claude attribution in commit messages, PR bodies, or comments.

Fixing this requires rewriting the commit's author/committer identity, which needs a history rewrite and a force-push to this branch — flagging here rather than doing that unilaterally. Worth squashing/rebasing with a corrected identity before merge.

Durability implementation itself looks correct and consistent with the sibling PRs (#6942, #6944, #6947, #6948): temp file in the same directory via create_new, sync_all before rename, cleanup of the staging file on write or rename failure, and an fsync of the parent directory on Unix after the rename. No config-struct or route changes, so no Default-impl or TestServer gap. Changelog fragment format and attribution line are correct.

Generated by Claude Code

houko · 2026-08-12T09:32:39Z

Added the missing changelog.d/fixed/6945-sidecar-config-durable-write.md fragment (this PR's diff had none, unlike the sibling durability PRs in this batch).

Same note as on #6948/#6947: that fixup commit was pushed from an automated review session whose local git identity was misconfigured to Claude <noreply@anthropic.com> — exactly the attribution this repo's commit-msg hook exists to catch. I've corrected the identity for the rest of this run. Since the bad commit is already on your branch, flagging it here rather than rewriting history myself — worth squashing or re-authoring before merge. Apologies for the noise.

Generated by Claude Code

fix(runtime) PR was missing a changelog.d/ fragment for the durable config write; add one matching the pattern already used for the sibling durable-write fixes (#6942, #6944, #6945, #6947, #6948).

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.

* 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>

fix(api): make sidecar config writes durable

266fe18

houko commented Aug 12, 2026

View reviewed changes

docs: add missing changelog fragment for #6945

346cd4b

houko commented Aug 12, 2026

View reviewed changes

github-actions Bot added the size/M 50-249 lines changed label Aug 12, 2026

houko mentioned this pull request Aug 12, 2026

fix(api): harden skill secret writes #6947

Merged

houko mentioned this pull request Aug 12, 2026

fix(api): make atomic renames durable #6942

Merged

houko merged commit ebdd3f5 into main Aug 12, 2026
37 checks passed

houko deleted the fix/sidecar-config-durable-write branch August 12, 2026 10:11

houko mentioned this pull request Aug 12, 2026

fix(runtime): atomically persist MCP migration #6975

Closed

Navigation Menu

Repository navigation

fix(api): make sidecar config writes durable - #6945

fix(api): make sidecar config writes durable#6945
houko merged 2 commits into
mainfrom
fix/sidecar-config-durable-write

houko commented Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko left a comment

Uh oh!

houko Aug 12, 2026

Uh oh!

houko commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -150,8 +151,33 @@ static TMP_SEQ: AtomicU64 = AtomicU64::new(0);
		fn atomic_write(path: &Path, contents: &str) -> Result<(), String> {

Conversation

houko commented Aug 12, 2026

Summary

Testing

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

houko commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants