fix(runtime): atomically persist MCP migration by houko · Pull Request #6975 · librefang/librefang

houko · 2026-08-12T12:28:37Z

Summary

replace the MCP migrator's truncating config write with a unique sibling staging file
sync staged bytes before publishing and sync the parent directory on Unix
preserve existing Unix permissions and create new config files with mode 0600
use MoveFileExW for atomic replacement of existing files on Windows

Tests

cargo test -p librefang-runtime mcp_migrate
cargo test -p librefang-runtime mcp_migrate::tests::durable_atomic_write --lib
cargo clippy -p librefang-runtime --lib --tests -- -D warnings
cargo fmt --all -- --check
git diff --check

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).

houko · 2026-08-12T12:37:52Z

    Ok(())
 }

+fn durable_atomic_write(path: &Path, content: &[u8]) -> std::io::Result<()> {


This is the sixth near-identical hand-rolled implementation of the same durable-write recipe (stage to a sibling tempfile with create_new, fsync, atomic rename, fsync the parent dir on Unix, clean up the staging file on any failure) after sidecar_toml.rs (#6945), cron_script.rs (#6948), and the other durable-write fixes referenced in this PR's own changelog neighbors (#6942/#6944/#6947). It even duplicates apply_patch::atomic_write in this same crate (weaker: no fsync, no permission preservation), so the crate now carries two versions of this logic side by side.

Per CLAUDE.md's "fix what you found" rule, this would normally be in-scope to consolidate, but a shared helper here would need to live somewhere common to librefang-runtime, librefang-api, librefang-kernel, librefang-skills, librefang-memory-wiki, librefang-llm-drivers, librefang-import, and librefang-hands (e.g. librefang-http or librefang-types) — a genuine cross-crate architecture call, not a mechanical fix. Flagging for a maintainer decision on whether it's worth extracting durable_atomic_write (this PR is also the first to add the Windows MoveFileExW/MOVEFILE_WRITE_THROUGH path and Unix permission-preservation, both of which the earlier copies lack) into a shared utility that the other five/six call sites migrate to, rather than growing a seventh copy the next time this bug class shows up in a new file.

Generated by Claude Code

houko · 2026-08-12T18:01:06Z

Reviewed the atomic-write implementation against CLAUDE.md's checklist (temp-file + fsync + rename, permission preservation, changelog fragment, new dependency wiring).

The core durability mechanism holds up: unique sibling staging file, sync_all() before publish, atomic rename with a parent-directory fsync on Unix, and MoveFileExW+MOVEFILE_WRITE_THROUGH on Windows all match the changelog's claims.

One correctness gap I found: OpenOptionsExt::mode() alone doesn't guarantee the "existing permissions are preserved" claim, because POSIX open() still ANDs the requested mode with the process umask — under a restrictive umask (e.g. 0o077), a 0o640 config could silently become 0o600 after migration, i.e. narrower than before. That's already been fixed on this branch in commit fdd18f2 (explicit set_permissions() after opening the staging file, bypassing umask, plus a regression test using a mode that actually conflicts with a default umask) — no action needed there.

Flagging one thing about that commit itself for the maintainer's attention rather than fixing unilaterally (would require rewriting a commit that's not mine): its author identity is Claude <noreply@anthropic.com>, which conflicts with this repo's own "No AI / Claude attribution in commit messages... the commit-msg hook enforces it server-side too" policy (CLAUDE.md, Git Conventions) — that policy also covers author identity, not just message text. Might be worth amending the author before merge.

No other issues found (changelog fragment present and correctly formatted, no new config fields requiring #[serde(default)], windows-sys dependency correctly scoped under [target.'cfg(windows)'.dependencies]).

Generated by Claude Code

houko

Reviewed the atomic-write implementation in detail. The main structural concern (six near-identical durable-write implementations across the crate, including this new one) is already flagged in the existing unresolved thread on this PR — not duplicating it. One additional narrow finding below on error semantics after a partial success. No mechanical fixes applied; changelog fragment, Cargo.toml/Cargo.lock windows-sys addition, and permission-preservation logic all check out.

Generated by Claude Code

houko · 2026-08-12T18:01:31Z

+        file.sync_all()?;
+        replace_file(&staging, path)?;
+        #[cfg(unix)]
+        std::fs::File::open(parent)?.sync_all()?;


If this parent-directory fsync fails, it does so after replace_file on the previous line has already renamed the staging file onto path — the new content is live on disk. But the function still returns Err here, and the caller (upsert_mcp_server_from_template → migrate_if_needed's loop) treats that as "could not upsert", logs a warn!, and bumps skipped_count instead of synth_count.

Concretely: integrations.toml still gets renamed to integrations.toml.bak.<ts> unconditionally afterward (this branch isn't gated on skipped_count == 0), so this migration never runs again for that install record, yet config.toml already has the correct [[mcp_servers]] entry. The operator sees a misleading "skipped" count even though nothing is actually missing.

Not proposing a fix here since it's a real design question (should a durability-only fsync failure after a successful rename be surfaced as "the write failed", or should it be a best-effort warning that doesn't affect the success/skip bookkeeping?) — flagging for a maintainer call rather than guessing at intent.

Generated by Claude Code

houko

Automated CLAUDE.md compliance pass. Code correctness looks solid (verified with cargo check -p librefang-runtime --lib, cargo test -p librefang-runtime --lib mcp_migrate — 8/8 passing, and cargo clippy -p librefang-runtime --lib --tests -- -D warnings — clean), and the changelog fragment is present and correctly formatted. Two findings left inline for maintainer judgment: an AI-attributed commit already on the branch, and cross-PR helper duplication with #6974. No mechanical fixes applied.

Generated by Claude Code

houko · 2026-08-12T18:11:51Z

+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt as _;
+            file.set_permissions(std::fs::Permissions::from_mode(mode & 0o7777))?;


Head commit fdd18f24 (which introduced this set_permissions call) is authored as Claude <noreply@anthropic.com>:

commit fdd18f24aa58f77ce6df7157e4f97712d0c3d1e6 Author: Claude <noreply@anthropic.com>

That's AI attribution in the commit history, which CLAUDE.md's "No AI / Claude attribution" rule and the commit-msg hook are meant to prevent — the hook checks author identity via git var GIT_AUTHOR_IDENT, so this commit should have been rejected locally, but it's already pushed to this branch.

This is exactly the pattern that's been leaking into main on recent squash-merges (e.g. #6968, #6965 carry a Co-authored-by: Claude <noreply@anthropic.com> trailer picked up from a similarly-authored commit in the PR) — if this branch is squash-merged as-is, the same trailer will likely get added here too.

Since the commit is already on the shared branch, fixing it needs a rebase + author reset (git commit --amend --reset-author) and a force-push, which is outside what an automated review pass should do unilaterally. Flagging for a maintainer to either re-author this commit before merge, or strip the Co-authored-by: Claude trailer when squash-merging.

(The fix itself — forcing set_permissions past the umask — is correct and covered by the added regression test; no concerns with the code.)

Generated by Claude Code

houko · 2026-08-12T18:12:01Z

    Ok(())
 }

+fn durable_atomic_write(path: &Path, content: &[u8]) -> std::io::Result<()> {


This is a fourth independent implementation of the same staged-write + fsync + atomic-rename pattern in the codebase: librefang-api::lib::atomic_write, librefang-api::routes::sidecar_toml::atomic_write, the durable_atomic_write landing in #6974 for librefang-cli, and now this one in librefang-runtime.

Since #6974 (which bills itself as "add a shared durable atomic-write helper") isn't merged yet, this PR isn't literally duplicating an already-landed helper — but the two are open at the same time and doing the same work independently, including the same umask-preservation subtlety this PR's last commit specifically had to patch in. Worth checking with whoever owns #6974 whether the helper should live somewhere shared (e.g. librefang-types or a small new util crate) that both librefang-cli and librefang-runtime depend on, rather than landing two hand-copies of the same ~60 lines that will drift independently over time.

Not blocking — the implementation here is correct (mode preservation, rollback-on-failure via staging-file cleanup, and parent-dir fsync all match the bar set by the existing helpers) — just flagging the cross-PR duplication for a maintainer call.

Generated by Claude Code

durable_atomic_write() only passed the preserved/new mode to OpenOptionsExt::mode(), which the kernel masks against the process umask at file-creation time. A config file with permission bits that overlap the umask (e.g. group/other write) would silently lose those bits on rewrite, contradicting the "preserve existing permissions" guarantee this helper exists to provide. Force the exact mode via an explicit set_permissions() call after opening the staging file, bypassing the umask. Adds a regression test pinning a mode (0o666) that conflicts with a typical 0o022 umask.

The MCP migration atomic writer had tests for the happy path, permission preservation, and the umask edge case, but nothing exercising a failed publish step. That is the exact scenario a crash or kill mid-migration produces: a staging file lands successfully, then the rename (or on Windows, MoveFileExW) does not complete. Add a regression test that forces the rename to fail by pointing the target at an existing directory, mirroring the equivalent test already established at librefang_api::routes::sidecar_toml::atomic_write_removes_staging_file_when_rename_fails, and assert the original target is left untouched and no staging file remains.

houko · 2026-08-12T22:52:17Z

+            let mode = std::fs::metadata(path)
+                .ok()
+                .map(|metadata| metadata.permissions().mode())
+                .unwrap_or(0o600);


Ambiguous behavior change worth a maintainer call, not something I'm fixing directly: previously std::fs::write created a first-time config.toml with the default umask-derived mode (typically 0o644), but this unwrap_or(0o600) now hardcodes 0o600 whenever the target doesn't already exist.
The changelog fragment (changelog.d/fixed/6975-mcp-migration-atomic-config.md) only says "existing Unix permissions are preserved" — it doesn't mention that new files now get a more restrictive default than before.
If tightening the new-file default is intentional (reasonable, since MCP server entries can carry OAuth config), please say so in the fragment per CLAUDE.md's changelog-fragment rule ("write the entry as prose that explains why"), since this is a behavior change a config.toml-permission-sensitive deployment could reasonably want called out rather than discovered.

Generated by Claude Code

… per line Per CLAUDE.md's prose-wrapping convention, since these comments were newly added by this PR.

houko · 2026-08-14T00:49:18Z

Superseded by #7168, which is now merged. The replacement persists MCP migration atomically and retains the migration-focused test coverage without changing unrelated behavior.

Evan and others added 2 commits August 12, 2026 21:28

fix(runtime): atomically persist MCP migration

c181cc2

docs(runtime): add changelog fragment for mcp migration atomic write

4ede81d

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).

houko commented Aug 12, 2026

View reviewed changes

github-actions Bot added area/runtime Agent loop, LLM drivers, WASM sandbox size/M 50-249 lines changed labels Aug 12, 2026

houko commented Aug 12, 2026

View reviewed changes

houko force-pushed the fix/mcp-migration-atomic-config branch from fdd18f2 to bb5d3a9 Compare August 12, 2026 18:13

houko commented Aug 12, 2026

View reviewed changes

docs(runtime): rewrap mcp_migrate durability comments to one sentence…

412a35b

… per line Per CLAUDE.md's prose-wrapping convention, since these comments were newly added by this PR.

houko mentioned this pull request Aug 14, 2026

fix(runtime): atomically persist MCP migration #7168

Merged

houko closed this Aug 14, 2026

Navigation Menu

Repository navigation

fix(runtime): atomically persist MCP migration - #6975

fix(runtime): atomically persist MCP migration#6975
houko wants to merge 5 commits into
mainfrom
fix/mcp-migration-atomic-config

houko commented Aug 12, 2026

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 Aug 12, 2026

Uh oh!

houko Aug 12, 2026

Uh oh!

houko commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

houko commented Aug 12, 2026

Summary

Tests

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 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 commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants