[rominf]

Fixes #605. Of #605's three problems, this fixes the non-unique temp name and the window where the marker is missing. The third, that no lock serialises activations, is tracked in #618 so that it survives this PR closing #605.

Background

#424 already fixed #605's temp-name collision and missing-marker window on main. This PR moves the marker, and every other write_file_atomically caller, onto one shared rocm_core::atomic_write, and deletes rocmd's drifted copy. apps/rocm and rocmd both drop their now-unused windows-sys dependency.

What this changes

• rocmd no longer declares windows-sys: ReplaceFileW was its only use, and that moved into rocm_core::atomic_write.

• One shared writer. write_file_atomically and its stage/publish helpers move from apps/rocm/src/therock.rs into a new rocm_core::atomic_write. The copy in apps/rocmd/src/sandbox.rs is deleted, so both binaries share one implementation. It creates a unique temp file exclusively ({name}.tmp-{pid}-{millis}-{attempt}), retries on collision, cleans up on failure, and uses ReplaceFileW on Windows.

• The two copies had drifted. The therock one is kept because it has the full-disk message and the stage/publish split, plus rocmd's "failed to create " message. rocmd's errors gain the full-disk message and "failed to publish ".

• The marker. write_active_runtime_marker writes through the shared helper, so the marker is replaced in one step and never removed first. Its error now reads "failed to write active runtime marker ". Nothing in tests, docs or scenarios referenced the old "failed to move" text.

• Tests moved with the helper. The five rocmd tests that duplicated therock's are dropped and its one unique test is kept: 15 tests become 10, with no assertion lost.

Coverage floor

coverage-floors.toml lowers rocmd's floor from 75.36 to 75.02, and changes only that line. The drop is legitimate: nearly fully covered code, and its tests, moved out of rocmd into rocm-core, and no test was weakened. I measured it with cargo llvm-cov --no-cfg-coverage -p rocm -p rocmd, which is how the workspace run measures rocmd, and got 75.30% → 75.02%. That is a drop of 0.28 points, past the 0.2 tolerance.

I edited the one line instead of running --bless, because a local bless would rewrite every crate's floor with this machine's numbers. If the required coverage job measures rocmd differently, I'll set the floor to its figure.

Deliberately not in this PR

• A lock around activation. activate_runtime/rollback_runtime receive a config their callers already loaded. config set-default-runtime, config clear-default-runtime and runtime uninstall also write config and marker without a lock. So a FileLock inside the two functions could not keep the pair consistent; it has to span load→save at every writer. Tracked in runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618.
• RocmCliConfig::save via the atomic writer. Replacing by rename would turn a symlinked config.json (common with dotfile managers) into a regular file, and drop custom permissions on it. That behaviour change deserves its own review. Also tracked in runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618.

Tests

• active_runtime_marker_write_never_reuses_another_writers_temp_file (all platforms, deterministic): pre-creates a file at every name the old scheme could pick during the test. It asserts the write succeeds, the marker parses, and none of those files was written through or moved.
• concurrent_active_runtime_marker_writes_keep_a_complete_marker_published (Unix): two writers run in lock-step for 200 rounds while a reader watches. It asserts every write succeeds, the reader always finds a complete marker, and no temp files are left behind. It is Unix-only because the Windows publish goes through ReplaceFileW, and two concurrent replacements of one file have not been run there.

Mutation check:

• With the old function body, both tests fail; in one run, 190 of 400 writes failed with "failed to move active runtime marker".
• With only the remove_file put back, the concurrent test fails because the reader finds no marker.
• With the fix, both pass in 10 of 10 runs.

No Gherkin scenario: the changed behaviour only shows when two activations race in the same millisecond, which the CLI-level harness cannot drive deterministically. On the single-writer path a user sees the same active.json content and the same commands, which the existing activation scenarios cover.

Run locally:

• cargo fmt --all --check

• cargo test --locked for -p rocm --bin rocm, -p rocmd --all-targets, -p rocm-core --lib, -p xtask and -p e2e-cucumber --lib

• cargo xtask check-crate-edges

• both clippy invocations with -D warnings

• Windows: a cargo check --target x86_64-pc-windows-gnu cross-check only; nothing was run on Windows.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No rows reference runtimes: the active-runtime marker is written non-atomically and without a lock #605 or the marker.)

• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (No new subcommand; the shared writer is its own rocm-core module.)

• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state. (The only changed message is the marker write error, which describes a failed write; the tests assert the marker state, not the wording.)

Since the reviews of e03fc77b and 46f283cb

• Rebase: onto 10345e7e. The marker code now lives in runtimes_cmd.rs.
• The writer's doc: gives the real reason for ReplaceFileW (a sharing violation, not rename semantics), says "atomic, not durable", and says concurrent Windows publishes are untested.
• Test seams: they are #[doc(hidden)], and write_file_atomically_with is private.
• The full-disk test: it now drives the real staging path.
• The collision test: its window is sized from measured timing.
• The scenario this relies on: @id:runtime-lifecycle-activate-records-previous.
• Coverage, measured locally: rocmd 75.24% (floor 75.02) and rocm-core 86.68% (floor 82.14).

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 8ae618f

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change (6 commits, db11e4742689...8ae618f47a4d). This change moves a duplicated "atomic file write" implementation (previously separately maintained in apps/rocm/src/therock.rs and apps/rocmd/src/sandbox.rs) into a new shared rocm_core::atomic_write module, and uses it to fix a real race condition in write_active_runtime_marker (non-unique temp names, a truncating write, and a remove-then-rename window that could leave the active-runtime marker briefly absent or corrupted when two activations raced). Fixes #605, but only part of it — see Blocking below.

Blocking

• "Fixes #605" will auto-close an issue whose own title and problem list this change does not fully resolve — PR title/body vs. issue #605.
  Issue #605's title is "the active-runtime marker is written non-atomically and without a lock," and its "Problems" section lists three items: (1) non-unique temp name, (2) marker briefly absent during remove+rename, (3) "No lock serialises activations... the two can end up disagreeing about the active runtime." This PR fixes (1) and (2) correctly (verified by three independent passes below). Problem (3) is explicitly not addressed: the PR's own "Deliberately not in this PR" section says "A lock around activation... That is a follow-up," with no follow-up issue number given. I confirmed no such follow-up issue exists (gh issue list search came up empty) and #605 is still open. Using GitHub's "Fixes #605" keyword will close that issue on merge even though its own stated lock concern remains untracked anywhere.
  Confidence 85 · architectural · Fix: either stop using the closing keyword (e.g. "Addresses #605" / "Part of #605") and keep #605 open for the lock follow-up, or file the follow-up issue now and reference it in the PR body before merging.

• Dead windows-sys dependency left in apps/rocmd/Cargo.toml — apps/rocmd/Cargo.toml:25-26.

  [target.'cfg(windows)'.dependencies]
  windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem"] }

  This file has zero diff in this PR (confirmed via git diff prw-base...HEAD -- apps/rocmd/Cargo.toml, empty), but the PR deletes apps/rocmd/src/sandbox.rs's replace_file_windows function, which was the only caller of windows_sys::...::ReplaceFileW in the whole rocmd source tree (verified: grep -rn "windows_sys" apps/rocmd/src/ now returns zero hits; it returned one hit, at the old sandbox.rs:778, before this PR). The Windows FFI call moved into rocm-core, which already declares its own windows-sys dependency. rocmd's copy is now unused dead weight on every Windows build, and there is no cargo-machete/unused-dependency check in this repo's CI to catch it.
  Confidence 90 · mechanical · Fix: remove the [target.'cfg(windows)'.dependencies] block (or the windows-sys line) from apps/rocmd/Cargo.toml.

Non-blocking

• A regression test's name/doc-comment overclaims what it exercises — apps/rocm/src/main.rs, active_runtime_marker_write_never_reuses_another_writers_temp_file (~line 35183).
  The test pre-creates foreign files named active.json.tmp-<millis> — exactly the old, pre-fix naming scheme — and asserts the new write never touches them. But the current production scheme (crates/rocm-core/src/atomic_write.rs) names temp files {name}.tmp-{pid}-{millis}-{attempt}, which is structurally different, so given create_new, the real write can never land on any of those pre-created names regardless of timing — the assertion is true by construction. It's a legitimate regression test against reverting to the literal old scheme (and does fail against the pre-fix code), just narrower than "never reuses another writer's temp file" suggests. The actual current-scheme collision safety is exercised elsewhere (the companion concurrent_active_runtime_marker_writes_keep_a_complete_marker_published test, and rocm-core's own concurrent_atomic_writes_do_not_remove_a_published_destination).
  Confidence 75 · mechanical · Fix: tighten the doc comment to say it guards against reverting to the old millis-only scheme specifically.

• Similar non-atomic state-writers exist elsewhere, untouched by this consolidation — engines/lemonade/src/state.rs:205, engines/vllm/src/state.rs:190, engines/lemonade/src/backend_alignment.rs:463,483.
  These all use a plain truncating fs::write over an existing JSON/state file — the same crash/race shape as the bug this PR fixes — though none are literal copies of the deleted atomic-write helper, so they're not part of "the two copies" this PR explicitly set out to consolidate. Pre-existing, pattern-relevant to this change's own stated purpose; worth a future pass.
  Confidence 70 · architectural

Decisions for the author

• Coverage-floor file hand-edited despite its own "do not edit by hand" header — tradeoff.
  coverage-floors.toml's header says "Generated by cargo xtask coverage --bless; do not edit by hand," but commit 8ae618f hand-edits exactly the rocmd line instead of running --bless. The commit message explains why: a full bless would rewrite every other crate's floor using this one machine's measured numbers, which could be noisy/host-dependent, and the author offers to reconcile against CI's own figure if it differs. I independently confirmed CI's "Coverage (rocm-dash crates, ratcheted)" job runs cargo xtask coverage over the whole workspace (not just rocm-dash crates, per that job's own comment) and is green on this PR — so rocm and rocm-core's unchanged floor entries, despite both crates' line counts changing, are validated as still above their committed floors by that same CI run, not stale. Both the deviation from the stated mechanism and the resolution are reasonable; flagging so the author can confirm this is still the intended final state.

• The new concurrent-write regression test is Unix-only — tradeoff.
  concurrent_active_runtime_marker_writes_keep_a_complete_marker_published is #[cfg(unix)], with an honest doc comment stating the Windows publish path (ReplaceFileW) has never been run under this specific concurrent-replacement scenario. Windows is a fully supported platform here (AGENTS.md §6), so this specific property is unverified there — the gap is disclosed, not hidden, and writing a deterministic equivalent against ReplaceFileW may be harder to make reliable in CI. Author's call whether to pursue it now or as a follow-up.

Positive signals

• The regression tests use real concurrent threads with Barrier, not mocks, and assert on-disk state (parseability, no leftover temp files, which payload survived) — genuine race reproduction rather than tautological tests.
• The PR's own claim about "keeping the better bits from each of the two drifted copies" (full-disk-aware error mapping, stage/publish split, parent-dir-creation message) was independently verified against both pre-PR implementations and checks out exactly, not just asserted.
• docs/architecture.md was updated precisely in both affected paragraphs (rocmd loses the "atomic-write helpers" mention, rocm-core gains an accurate one-line pointer), with no stale reference to the old location left anywhere else in the file.

Deployment notes

None — internal CLI-only change, no schema/migration, no public API. The marker-write failure error text changed ("failed to move active runtime marker ..." → "failed to write active runtime marker ..."); confirmed by grep that nothing in docs/, README.md, or e2e fixtures asserts on the old wording.

What this covered

Every file in the diff, completely, against db11e4742689a8a54e5ed9816f4d202cd3e6cab9...8ae618f47a4dff3093ae73f6293df948ce397655: apps/rocm/src/main.rs, apps/rocm/src/therock.rs, apps/rocmd/src/sandbox.rs, coverage-floors.toml, crates/rocm-core/Cargo.toml, crates/rocm-core/src/atomic_write.rs (new), crates/rocm-core/src/lib.rs, docs/architecture.md — plus surrounding code (all other call sites of the marker path, all remaining fs::/windows_sys/OsStr/io:: usages in touched files, prior git history on the deleted sections, engines/lemonade and engines/vllm for duplicate or related patterns). Four independent passes ran in parallel (rocm-core's new module; main.rs+therock.rs; sandbox.rs; a cross-cutting AGENTS.md/history/docs/impact pass), each blind to the PR's description, plus my own consolidation, verification of two concrete claims (the dead dependency, the coverage-CI-job scope), a leak scan, and a DCO/sign-off check.

Verified by running, not just reading: cargo check/cargo clippy -D warnings/cargo test for rocm-core, rocm, and rocmd (all clean, all tests pass); a genuine cargo check -p rocmd --target x86_64-pc-windows-gnu cross-compile (clean, after installing mingw-w64 locally); cargo xtask check-crate-edges (clean); live gh pr view --json statusCheckRollup (all 27 non-skipped checks SUCCESS, including the full-workspace coverage-floor gate and commit-signatures+sign-off); gh issue view 605 and a search for a lock-related follow-up issue (none found); a leak scan (internal|confidential|proprietary|private|jira|confluence|.corp|.internal — clean) and a conflict-marker grep (clean).

Did not run / could not verify: execution of the new regression tests against the reverted (pre-fix) code to empirically reproduce the PR's claimed 190/400-failure mutation-testing result (read-only reviewer constraint; verified structurally/by static reasoning instead); actual Windows runtime behavior of ReplaceFileW/the ambient TOCTOU window in publish_temp_file (cross-compiled only, not run on real Windows); re-running cargo llvm-cov myself to re-derive the exact 75.30%→75.02% rocmd coverage numbers in commit 8ae618f (took CI's green full-workspace coverage gate as the authoritative check instead, per this method's own instruction to trust what CI already reports). Skill checks (skillscope) and Sphinx docs build were SKIPPED by CI itself (not relevant to this diff) — not re-run here.

Not reconciled against any discussion, review comments, or prior rounds on this PR — none were read, per this review's instructions.

[jussielo-amd]

Non-blocking: this test's name and doc comment claim it proves the write "never reuses another writer's temp file," but the foreign files it pre-creates use the old pre-fix naming scheme (active.json.tmp-<millis>), while the current scheme in rocm_core::atomic_write is {name}.tmp-{pid}-{millis}-{attempt}. Given create_new, the real write structurally cannot land on any of these pre-created names regardless of timing, so the assertion holds by construction rather than exercising a genuine race against the current naming scheme. It's still a legitimate regression test against reverting to the literal old scheme. Suggest tightening the doc comment to say that explicitly — the current-scheme collision property is already covered by the companion concurrent_active_runtime_marker_writes_keep_a_complete_marker_published test and rocm-core's own concurrent_atomic_writes_do_not_remove_a_published_destination.

[rominf]

Agreed, and narrowed in b3a5710. The doc comment now says the test guards against a return to the millis-only scheme specifically. The current scheme ({name}.tmp-{pid}-{millis}-{attempt}) cannot match those names by construction. Collision safety under the current scheme is covered by concurrent_active_runtime_marker_writes_keep_a_complete_marker_published and by rocm_core::atomic_write's own concurrency tests.

[rominf]

Addressing the review above. Pushed b3a57102.

Blocking:

• "Fixes runtimes: the active-runtime marker is written non-atomically and without a lock #605" would leave the lock concern untracked: runtimes: the active-runtime marker is written non-atomically and without a lock #605's third problem (no lock serialises activations) is now its own issue, runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618, and the description says this PR fixes runtimes: the active-runtime marker is written non-atomically and without a lock #605's first two problems and points at runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618. Closing runtimes: the active-runtime marker is written non-atomically and without a lock #605 on merge now loses nothing. runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618 also carries the RocmCliConfig::save atomicity question.
• Dead windows-sys dependency in apps/rocmd/Cargo.toml: removed in b3a57102. ReplaceFileW was its only use, and that moved into rocm_core::atomic_write. The lock file only drops rocmd's edge; the package stays through rocm-core. cargo xtask manifest --check and cargo xtask tpn --check are unchanged, and a Windows-target cargo check of rocmd and rocm-core is clean.

Non-blocking:

• The test name and doc overclaimed: narrowed in b3a57102; reply inline.
• Other non-atomic state writers in the engines: tracked in engines: write engine state files atomically through rocm_core::atomic_write #622. They are not copies of the deleted helper, so they are outside this consolidation.

Decisions:

• Hand-edited coverage floor: yes, that is the intended final state. CI's required coverage job, which you confirmed measures the whole workspace, is green on this PR with rocmd at 75.02.
• The concurrent test is Unix-only: a deliberate, disclosed gap. Two simultaneous ReplaceFileW calls on one file have never been run here, and a deterministic Windows equivalent needs Windows CI time to make reliable. Worst case there is a failed write with an error, not a corrupt marker.

Stale (left on 8ae618f; head is b3a5710). Both blocking items are addressed. #605 problem 3 (the activation lock) is now tracked in #618, and the description says so. The dead windows-sys dependency is removed in b3a5710. Non-blocking: the test doc is narrowed in b3a5710, and the engine state writers are tracked in #622. Point-by-point reply is in the PR conversation.

[rominf]

Rebased onto main 34fbdfe9; the head is now e03fc77b. #584 added a command_failure_detail import to apps/rocmd/src/sandbox.rs next to this PR's atomic_write import, and I kept both. Every other line of the diff is unchanged.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1029), rocm-core (608), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations
• a Windows-target cargo check -D warnings

#541 also moves the atomic writer into rocm-core, so whichever of the two merges second will need a rebase.

[siloteemu]

🔴 Automated review · pr-review-watcher · e03fc77

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
States "Fixes #605". It fixes two of the issue's three problems: the non-unique temp name and the moment when the marker is missing. The third, a lock around activation, is explicitly deferred to #618. The issue listed that lock as optional, and #618 is open, so the closing claim holds.

Blocking

• [new] apps/rocm still declares the Windows windows-sys dependency the move left unused — apps/rocm/Cargo.toml:54
  The only windows_sys use in apps/rocm at the base was ReplaceFileW in therock.rs, which this PR moves into rocm_core::atomic_write. grep -rn windows_sys apps/rocm/ now finds nothing. The PR removes the same dependency from apps/rocmd (commit e03fc77, "drop the windows-sys dependency the move left unused") but leaves the rocm one.
  Confidence 85 · mechanical · Fix: delete the windows-sys line from apps/rocm/Cargo.toml and keep windows-native-keyring-store.

Non-blocking

• [new] Test-only seams are now public API of rocm-core — crates/rocm-core/src/atomic_write.rs:70, :94, :187/:194, :111-113

  • write_file_atomically_with has no caller outside its own module.
  • write_file_atomically_with_publish and publish_temp_file are pub only because therock.rs imports them under #[cfg(test)].
  • The doc on stage_file_for_atomic_publish says the caller "either publishes it or removes it". The only publish helper that cleans up after itself, publish_staged_file_with, is private, so an outside caller cannot publish safely.

  Confidence 75 · architectural · Fix: make write_file_atomically_with private, put the other seams behind #[doc(hidden)] or a test-support feature, and either expose a cleaning publish or narrow that doc.

• [new] "Crash-safe" claims more than the code does — crates/rocm-core/src/atomic_write.rs:5, docs/architecture.md:38
  The staging path does write_all, drop, then rename, with no sync_all/sync_data. The writer survives a process crash and races, but not necessarily a power loss on a filesystem that does not order data before rename. The code is what governs here: the new wording overstates it.
  Confidence 70 · mechanical · Fix: say "process-crash- and race-safe", or add sync_all before publishing.

• [new] docs/architecture.md calls this "the one" atomic replacement, but another copy remains — docs/architecture.md:38, apps/rocm/src/remote/session.rs:108-130
RemoteSession::write has its own unique-staging write followed by fs::rename. That copy is pre-existing; calling atomic_write.rs "the one" is new in this PR.
  Confidence 70 · mechanical · Fix: soften the wording, or name session.rs as a copy not yet migrated.

• [new] The moved full-disk test never touches the write path, and rocmd now relies on that path — crates/rocm-core/src/atomic_write.rs:257, :162
write_file_atomically_reports_a_full_disk_clearly calls only disk_space::map_write_error, so it stays green if the stager stops mapping errors. The only real ENOSPC test (:487) is #[ignore] and does not check the message. The PR description says rocmd errors now gain the full-disk message, and no test that runs by default reaches that path.
  Confidence 70 · logic · Fix: rename the test to what it checks, or drive a real write failure through stage_file_for_atomic_publish_with and assert the mapped message.

• [new] The history carries fixups for breakage from the first commit — e8d19c8, fixed by 1efeff1, 7a357fe, 1ee40d0 and e03fc77
  Taken alone, e8d19c8 fails clippy -D warnings, leaves an unused import on Windows, leaves a dead dependency, and breaks the rocmd coverage floor. That makes it unbisectable under the repo's gates.
  Confidence 70 · mechanical · Fix: fold the four fixups into e8d19c8 and keep 4491fef (the fix) as its own commit.

• [new] The MI300X E2E check has not reported for this head — CI status rollup
  "E2E tests (MI300X)" is still QUEUED. The Strix Halo Ubuntu, Windows and WSL2 E2E lanes passed. "Sphinx docs build", "Skill checks" and "Regenerate manifests" are SKIPPED by their path filters; the docs file this PR touches is not part of the Sphinx source.
  Confidence 70 · mechanical · Fix: let the MI300X lane finish before merging.

Decisions for the author

• RocmCliConfig::save is still a plain truncating write in the same activation path — answered-by-intent
  Would otherwise have been a non-blocking finding: activate_runtime/rollback_runtime call it just before the marker write (crates/rocm-core/src/lib.rs:2648-2658). The PR description says: "RocmCliConfig::save via the atomic writer. Replacing by rename would turn a symlinked config.json … into a regular file … Also tracked in #618." Confirm that is still the plan.
• The Windows concurrent publish is not exercised, but the function doc states the guarantee without qualification — tradeoff
  • write_active_runtime_marker's doc (apps/rocm/src/main.rs:9216-9222) says "a reader always finds a complete marker".
  • The stress test is #[cfg(unix)], and its comment and the PR text say ReplaceFileW with two replacements in flight has not been run.
  • One option keeps the doc as written and accepts the untested Windows claim. The other qualifies the doc for Windows until a Windows lane runs the test.
• The rocm-core coverage floor was not raised after it received the moved tests — tradeoff
  • docs/testing.md says "After adding tests, ratchet the floors up". If the floor stays where it is, the moved tests could be deleted inside the tolerance.
  • The PR explains why it hand-edited only rocmd: a local --bless would rewrite every crate's floor with one host's numbers. Raising rocm-core from the coverage job's own measurement would close the gap.

Positive signals

• Both new marker tests were mutation-checked in this review and fail on the regressions they target:
  • With the prw-base function body restored, both fail, 3 runs of 3.
  • With the shared writer replaced by a plain fs::write, the concurrent test fails, 3 runs of 3.
• The consolidation keeps the stricter therock copy, including the full-disk mapping and the stage/publish split. It carries forward every invariant earlier reviews required: an exclusive temp file reserved with bounded retries, cleanup of its own temp file on any failure, and the previous file kept when a replace fails.

Deployment notes

None

What this covered

Read completely: every file in the change (prw-base...e03fc77b), the deleted helpers and tests in therock.rs and sandbox.rs at prw-base, every caller of the moved helpers and of the marker writer and reader, and the checkout's AGENTS.md, docs/testing.md and docs/architecture.md.

What was run:

• cargo test -p rocm-core --lib atomic_write: 9 passed, 1 ignored.
• cargo test -p rocm --bin rocm active_runtime_marker: 2 passed.
• The two mutation runs above.

Verdicts on the added tests:

• active_runtime_marker_write_never_reuses_another_writers_temp_file:
  • It fails on revert.
  • It stays green when the writer is a plain fs::write. It guards only against a return to the millisecond-only temp name, which its doc comment says.
• concurrent_active_runtime_marker_writes_keep_a_complete_marker_published:
  • It fails on revert.
  • It fails with a plain fs::write.
• The atomic_write.rs tests were moved, not newly written, and pass.

Not run, and how the gaps were handled:

• Nothing was run on Windows.
• Coverage was not measured.
• The Step 7 design questions (config atomicity, Windows publish, API surface) were settled from the fan-out workers' results and #605/#618, not by separate design workers.
• The prior-changes pass read review comments on earlier PRs #165 and #483 among others, not on this PR.

Not reconciled against any discussion here.

[rominf]

Rebased onto main 7a062ad7; the head is now 46f283cb. #624 reformatted docs/architecture.md into per-module lists, so this PR's two edits are re-applied in that form: sandbox.rs no longer claims atomic-write helpers, and atomic_write.rs has its own entry. The code diff is unchanged, and it now sits on top of #510.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1030), rocm-core (612), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

[siloteemu]

🔴 Automated review · pr-review-watcher · 46f283c

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Claims to fix #605. Its first two problems, the non-unique temp name and the missing-marker window, were already fixed on main by #424. What this change still adds is the move to a shared rocm-core writer. The third problem (locking) is deferred to #618.

Blocking

• [new] The branch conflicts with main, which already fixed the marker writer and added a second caller of the helper this change deletes — apps/rocm/src/main.rs, apps/rocm/src/therock.rs, crates/rocm-core/src/lib.rs, docs/architecture.md
  • git merge-tree --write-tree prw-base HEAD reports content conflicts in those four files, and GitHub reports the PR as CONFLICTING / DIRTY.
  • On main, #424 already sends write_active_runtime_marker through therock::write_file_atomically, which is pub(crate). It also calls that helper from ActivationSnapshot::restore (prw-base:apps/rocm/src/main.rs:5158).
  • This change deletes that function from therock.rs and only brings it back in through a private use rocm_core::atomic_write::{…}. So therock::write_file_atomically no longer resolves from main.rs, and the rebase has to repoint both callers.
  • The description's "What was wrong" section and the "Regression for #605" test comments describe code that is no longer on main. After a rebase the marker hunk is a refactor, not the fix. The two new tests still guard the property, but they no longer catch a regression in this change.
  • Confidence 100 · architectural · Fix: rebase onto main, and point the restore path and the marker writer at rocm_core::atomic_write. Narrow the description and the test comments to what the change does once rebased: one shared writer, and the rocmd copy removed.
• [standing] apps/rocm still declares windows-sys although the code that used it was moved out — apps/rocm/Cargo.toml:58
  • Its only use was ReplaceFileW in therock.rs (prw-base:apps/rocm/src/therock.rs:4793), which this change moves to rocm-core.
  • grep -rn windows_sys apps/rocm now finds nothing.
  • Commit 46f283c removed the same leftover from rocmd for the same reason. apps/rocm was missed.
  • Confidence 85 · mechanical · Fix: delete the windows-sys line under [target.'cfg(windows)'.dependencies] in apps/rocm/Cargo.toml.

Non-blocking

• [standing] The new module doc gives the wrong reason for ReplaceFileW, and its "crash-safe" claim is new — crates/rocm-core/src/atomic_write.rs:5,15-18, docs/architecture.md:71
  • The comment says rename onto an existing file "is not a replace" on Windows. Rust's std::fs::rename does replace an existing file on Windows.
  • The real reason is the one #424 wrote on main beside the marker writer: a sharing violation while another process holds the destination open.
  • "Crash-safe" holds for a process crash. Neither the temp file nor the directory is fsynced, though, so a power loss can leave a published but empty or stale file.
  • Neither removed copy made either claim; both are new text.
  • The code governs here, since it behaves correctly, so the comments are what should change.
  • Confidence 70 · mechanical · Fix: state the sharing-violation reason, and say "atomic replace, not durable" (or add sync_all before the publish).
• [standing] "The one … file replacement" and "add a caller rather than another copy" overstate the consolidation — docs/architecture.md:71
  • Two hand-rolled temp-then-rename writers remain:
    • apps/rocm/src/remote/session.rs:117-132 has no create_new, no ReplaceFileW, and does not clean up when the write fails.
    • RocmCliConfig::save in rocm-core writes its own temp file and renames it.
  • The description defers save to #618 on purpose (symlink and permission semantics), which is a fair reason.
  • The architecture line, though, claims the consolidation is complete.
  • Confidence 70 · mechanical · Fix: say "the shared writer" and name the known exceptions, or point at #618.
• [new] The collision test hard-fails when its 2 s window runs out — apps/rocm/src/main.rs (active_runtime_marker_write_never_reuses_another_writers_temp_file, the finished < start + WINDOW_MS assert)
  • It creates 2000 files and then requires the write to finish inside 2 s. It is not platform-gated.
  • Locally the whole test took under 0.1 s, and the Windows CI lane passed.
  • On a slow or antivirus-scanned filesystem, though, an "inconclusive" run fails as a test failure, even though the code is correct.
  • Confidence 70 · logic · Fix: make the window proportional to the time the pre-creation took, or skip with a message instead of failing.
• [new] The description does not name the existing scenario it relies on — AGENTS.md §3
  • It says "the existing activation scenarios cover" the single-writer path, but names no scenario.
  • §3 asks for the specific @id:, here @id:runtime-lifecycle-activate-records-previous in tests/e2e-cucumber/features/runtime_lifecycle.feature.
  • Confidence 70 · mechanical · Fix: name the @id in the PR text.
• [standing] Fix-up commits inside the PR — 420cb47, 46f283c
  • "gate std::fs import to the targets that still use it" and "drop the windows-sys dependency the move left unused" repair 928d367 from this same PR.
  • Confidence 70 · mechanical · Fix: fold them into 928d367 during the rebase.

Decisions for the author

• Test seams are now public API of rocm-core — tradeoff
  • write_file_atomically_with, write_file_atomically_with_publish and publish_temp_file have no production caller. Their only users outside the crate are #[cfg(test)] code in therock.rs.
  • They have to be pub for those cross-crate tests.
  • The alternatives are #[doc(hidden)] (precedent: crates/rocm-dash-collectors/src/amd_smi.rs:84), or dropping the therock.rs tests that duplicate the ones now in atomic_write.rs and making the seams private.
• Windows replacement under contention is untested and has no fallback — tradeoff
  • The concurrent test is #[cfg(unix)], and the description says so.
  • If ReplaceFileW fails (a held handle, or a concurrent replacement), the error is returned with no retry and no fallback to rename.
  • Unique temp names and never removing the destination are a clear improvement over the old code. Whether to add a fallback is the author's call.

Positive signals

• Both new tests do catch the old code. With the old write_active_runtime_marker body restored, both failed: 81 of 400 concurrent writes failed with "failed to move active runtime marker", and the collision test reported a foreign temp file written through.
• The consolidation kept the stronger of the two drifted copies (full-disk mapping, the stage/publish split), and every property the removed tests covered has an equivalent in rocm-core.

Deployment notes

None

What this covered

• Read: every file in prw-base...HEAD completely, plus the code paths around the changes:
  • the callers of the moved helpers in apps/rocm and apps/rocmd;
  • the readers and writers of active.json;
  • the prior versions of both removed copies at the merge base 7a062ad;
  • #424's version on prw-base.
• Commits: 7a062ad…46f283cb, with prw-base at 8e2e8904.
• Run:
  • cargo test -p rocm-core --lib atomic_write: 9 passed, 1 ignored.
  • cargo test -p rocm --bin rocm active_runtime_marker: 2 passed. The revert mutation above made both fail.
  • git merge-tree against prw-base.
  • Leak scan of the added lines: clean.
• CI on 46f283c: all checks green; three were skipped.
• Not run:
  • The suite on Windows.
  • Branch-level mutation of the rocm-core writer.
  • A build of the merged result, which conflicts and cannot be built without resolving it.
• Not reconciled against any PR discussion here.

[rominf]

Rebased onto 10345e7e; the head is now 71e6bf39. #653 moved the marker code into runtimes_cmd.rs, and the hunks are re-applied there. The PR's added and removed lines are identical before and after the rebase. This answers both automated reviews, point by point.

Review on 46f283cb

• Conflicts / repoint both callers: done. ActivationSnapshot::restore and write_active_runtime_marker in runtimes_cmd.rs both call rocm_core::atomic_write::write_file_atomically. A plain fs::write at the restore site fails restoring_a_marker_replaces_the_file_rather_than_rewriting_it_in_place.
• The test comments about runtimes: the active-runtime marker is written non-atomically and without a lock #605: narrowed in 50d1075e. They now say EAI-7404: make runtime activation and rollback transactional #424 fixed the original bugs, and that these tests pin the marker keeping those properties through the one shared writer.
• windows-sys in apps/rocm: removed in cb3fbec8, with Cargo.lock updated. Windows clippy is clean.
• The ReplaceFileW reason and "crash-safe": fixed in a091153b. The doc now gives the real reason (a sharing violation while another process holds the destination open; std::fs::rename itself does replace), and says "atomic, not durable", since nothing is fsynced.
• "The one" writer: fixed in 71e6bf39. The doc now says "the shared writer" and names the exceptions: remote/session.rs, and RocmCliConfig::save, which fix(config): never replace an unreadable config.json with defaults (ROCMAI-532) #541 moves onto this writer (runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618).
• The collision test's fixed 2 s window: a miss now retries with a window sized from the measured pre-creation and write time. Where creating names is slower than one per millisecond, the test reports itself inconclusive and passes. With the old body restored, it still fails.
• The scenario @id: @id:runtime-lifecycle-activate-records-previous is now named in the description.
• Fix-up commits: left as they are, since the PR squash-merges.

Review on e03fc77b

• Test seams in the public API: write_file_atomically_with is now private. write_file_atomically_with_publish and publish_temp_file stay pub, because therock.rs's cross-crate #[cfg(test)] cache tests call them. They are now #[doc(hidden)] and documented as test seams.
• The full-disk test never touched the write path: replaced by staging_into_a_full_disk_reports_it_clearly_and_removes_the_temp_file. It drives the real staging path with only the byte write injected. Dropping map_write_error, or the temp-file cleanup, fails it.
• The Windows concurrency claim: the module doc and write_active_runtime_marker's doc now say concurrent Windows publishes are not tested.
• The rocm-core coverage floor: measured locally, rocm-core is at 86.68% (floor 82.14) and rocmd at 75.24% (floor 75.02). A local --bless would rewrite every crate's floor from this host's numbers, so I'd rather raise rocm-core from the coverage job's own figure. Happy to do that in this PR if you prefer.
• RocmCliConfig::save: still fix(config): never replace an unreadable config.json with defaults (ROCMAI-532) #541's, tracked in runtimes: serialise active-runtime changes so config.json and active.json cannot disagree #618.

Verified locally

• fmt
• tests for rocm --bin (1087), rocm-core --features e2e-test-hooks (631), rocmd (126), xtask (252) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e, check-crate-edges and xtask manifest --check
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu
• cargo xtask coverage

The one local failure was the known spinner timing flake, which passed on re-run.

Addressed on 71e6bf3 (rebased onto 10345e7): the PR comment of 2026-10-13 18:35 UTC answers both automated reviews point by point. All required checks pass on that head.

[rominf]

Rebased onto main 07166450 (#526); the head is now c6e2d959.

• Conflict: one, in the imports of apps/rocmd/src/sandbox.rs. fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 replaced sandbox's run_process_with_timeout with rocm_core::process::run_with_timeout and dropped the command_failure_detail import. This PR imports rocm_core::atomic_write::write_file_atomically instead of the local writer. The resolution keeps both changes.
• What fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 means for this PR: it adds no new atomic-write call sites and does not use windows-sys in rocmd or rocm, so both removals stand. The PR's added and removed lines are line-for-line unchanged.
• Verified locally:
  • fmt;
  • tests: rocmd, rocm-core, rocm, xtask, e2e-cucumber lib and feature_naming;
  • manifest --check and tpn --check;
  • cargo check --test e2e;
  • clippy, both invocations, on Linux and the Windows target.

[rominf]

Rebased onto fcc646e9 (#666, #683); the head is now 46033600. It still merges cleanly with current main. On that merge, cargo test -p xtask passes (313, including the ASCII-output contract), and rocm, rocmd and rocm-core build.

• Moved code: ROCMAI-151: extract config/automation/service_record/binary_paths from rocm-core lib.rs #666 moved RocmCliConfig into config.rs. pub mod atomic_write now sits next to its new siblings in lib.rs, and the doc-comment edit moved with its test into config.rs.
• A new second writer: ROCMAI-151: extract config/automation/service_record/binary_paths from rocm-core lib.rs #666 also added a crate-private write_atomic in lib.rs. The config save, the automation stores and the service-record manifests use it. The architecture note now names it as a writer still outside atomic_write, rather than calling atomic_write the only one. Moving those callers over is a follow-up.

Verified locally, all green:

• fmt;
• tests: rocm-core (755), rocmd (148), rocm (1233), xtask;
• manifest --check and tpn --check;
• e2e-cucumber lib, feature_naming and check --test e2e;
• clippy on Linux and the Windows target.

[nowycondro]

🔴 Automated review · pr-review-watcher · 4603360

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (prw-base d4e7c75…46033600, 11 files). Consolidates a duplicated atomic-file-write implementation (previously separately maintained in apps/rocm/src/therock.rs and apps/rocmd/src/sandbox.rs) into a new shared rocm_core::atomic_write module. States it fixes #605's non-unique-temp-name and missing-marker-window problems, and explicitly defers #605's third problem (a lock around activation) to #618 — consistent with #618 being open. This PR has several earlier automated review rounds on it from a different account, all at older commits (e03fc77b, 46f283cb); this is a fresh review at the current head, with nothing assumed to still hold from those rounds.

Blocking

• A documented Windows failure mode of ReplaceFileW can delete the only remaining copy of the data, contradicting this module's and its caller's own guarantees — crates/rocm-core/src/atomic_write.rs:212-221 (publish_staged_file_with), :241-291 (publish_temp_file/replace_file_windows). Microsoft's own documentation (cross-checked against LLVM and Tahoe-LAFS Magic Folder, which both had to special-case this) states ReplaceFileW called without a backup filename can fail after already moving the destination off its name, returning ERROR_UNABLE_TO_MOVE_REPLACEMENT/_2. replace_file_windows returns the raw OS error unconditionally on any failure, and publish_staged_file_with's cleanup then deletes the temp file — the one remaining copy of the new bytes — regardless of whether the destination still exists. In this specific failure mode the destination is already gone and the temp file is deleted too, so the path ends up absent entirely: neither the old content nor the new content. This directly contradicts the module's own doc ("the destination is never absent while it is being replaced") and the caller's comment at apps/rocm/src/runtimes_cmd.rs:1786 ("a reader always finds a complete marker"). No test in the file exercises this path. A true sibling branch in this same repo (origin/fix/unreadable-config-wipe, commit 8342be1, authored ~2.5h after this PR's last commit, common ancestor fcc646e9 — not a later rebase of this PR) independently fixes exactly this bug in the same function, with a commit message describing this precise production failure.
  Confidence 85 · architectural · Fix: on a ReplaceFileW failure, retry once as a plain rename (as the sibling branch does), and only delete the temp file if the pre-existing destination is confirmed still present/restored; otherwise keep it and name it in the error so the caller can recover the data.
• CI is not green at this head — E2E tests (Strix Halo, Windows, current) reports failure (job 117421900594, run 39041524839); every other check, including E2E tests (MI300X, current) and E2E tests (W7900, current), is green. The job's own log shows it ran to completion and uploaded a normal 6-file report rather than dying early, so — unlike a known infra-download failure seen on sibling PRs around the same time — this looks like an actual scenario failure rather than a runner/setup problem, though the log tail available here does not identify which scenario or why. Given this PR specifically changes Windows file-replacement behavior, the two are plausibly connected; this should be triaged before being waved off as unrelated.
  Confidence 60 that this is connected to the change (the job log excerpt available did not show the actual scenario failure) · logic · Fix: open the job's full report artifact or logs and determine which scenario failed and why before merging.

Non-blocking

• A weaker, duplicate atomic-write path remains in rocm-core itself, on three more call sites — crates/rocm-core/src/lib.rs:502-519 (write_atomic), used by config.rs:559, and per docs/architecture.md by automation.rs/service_record.rs. It names its temp file with a single fixed {pid}-{millis} string and uses plain fs::write — the same collision class (#605/#424) this PR's module exists to fix, left running elsewhere. This is explicitly disclosed in this PR's own docs/architecture.md update and deferred to #541/#618, so it's a visible, intentional scope boundary rather than a silent gap.
  Confidence 85 · architectural.
• No fsync before publish, and no permission-bit/symlink-target preservation — the module's own doc is upfront that it is "atomic, not durable" (no sync_all/sync_data, so a power loss can leave a stale/empty destination), and a sibling branch (predating this PR) already closes that gap for the config.json case. Permission/symlink preservation isn't needed by any of this PR's current callers (none writes a restrictive-permission file or symlinked path today) but will matter once config.rs::save migrates onto this module per the tracked #541.
  Confidence 75 · architectural.
• coverage-floors.toml was re-blessed only for rocmd; rocm (77.13) and rocm-core (82.14) floors/line-counts are unchanged despite rocm-core gaining a ~596-line new module and rocm's therock.rs/runtimes_cmd.rs shrinking by roughly 200 lines. Likely harmless given the new tests, but unconfirmed — no Rust toolchain was available in this sandbox to run cargo xtask coverage and check.
  Confidence 60 · mechanical.
• One low-severity leftover from an incomplete cleanup pass: ActivationSnapshot::restore (apps/rocm/src/runtimes_cmd.rs:440-449) still does its own fs::create_dir_all(parent) immediately before calling the shared writer, even though the shared writer already creates the parent directory internally (atomic_write.rs:169) and the sibling call site (write_active_runtime_marker) had its own redundant create_dir_all removed for exactly that reason. Harmless (create_dir_all on an existing dir is a no-op), just inconsistent.
  Confidence 75 · mechanical.

Decisions for the author

• Deferring lib.rs::write_atomic's weaker guarantees and durability (fsync) to #541/#618 — tradeoff. Both gaps are openly disclosed in this PR's own docs/architecture.md update, so this is a conscious scoping decision rather than an oversight; worth confirming the sequencing is still intended given a sibling branch has already implemented fixes for the first blocking item above and for durability.

Positive signals

• The exclusive-create retry loop (atomic_write.rs:168-205) uses true create_new with no TOCTOU window, bounds retries at 128 attempts, and fails with a clear, actionable error rather than looping forever.
• The full-disk test exercises the real production staging path via a write-injection seam, not an isolated unit test of the error-mapping function alone — it proves the claim it makes.
• active_runtime_marker_write_never_reuses_another_writers_temp_file is an honest, well-scoped regression guard against reintroducing the specific millis-only temp-name bug from #605/#424 (its own doc comment correctly disclaims testing the current scheme's general collision safety, which is instead covered by the companion concurrent test) — exactly the kind of precisely-scoped test documentation that's easy to get wrong and was gotten right here.
• write_file_atomically_with_publish/publish_temp_file are pub only with #[doc(hidden)] plus a doc comment explaining the cross-crate test-seam reason and steering production code elsewhere — a narrow, well-justified API surface.
• Staging the temp file as a sibling of the destination (not a system temp dir) avoids cross-filesystem EXDEV rename failures entirely.
• Repo-wide grep confirms no dangling references to the deleted per-binary copies, and both binaries' now-unused windows-sys dependency was correctly dropped (verified: its only use, ReplaceFileW, now lives solely in rocm-core, which already declares the same dependency).

Deployment notes

• Windows deployments carry a latent, untested data-loss window on the active-runtime-marker and metadata-cache files (see first Blocking item) whenever ReplaceFileW hits a partial-move failure (e.g. antivirus/indexer holding a handle open).
• config.json and the automation/service-record stores do not yet benefit from this consolidation; they remain on the weaker lib.rs::write_atomic pending #541/#618.

What this covered

Read completely: the new crates/rocm-core/src/atomic_write.rs, the lib.rs/config.rs diffs and their surrounding functions, crates/rocm-core/src/disk_space.rs, every caller of the new module's public API in apps/rocm/src/therock.rs, apps/rocm/src/runtimes_cmd.rs, apps/rocmd/src/sandbox.rs, coverage-floors.toml, crates/rocm-core/Cargo.toml/apps/rocm/Cargo.toml/apps/rocmd/Cargo.toml's Windows dependency lines, docs/architecture.md, and root AGENTS.md. Diff prw-base (d4e7c75) … HEAD (4603360), 11 commits. Four independent passes covered: the new rocm-core module itself (plus a cross-check against an independent sibling branch's later fix for the same bug and public Windows API documentation); the apps/rocmd removal and its behavior-preservation; the apps/rocm call-site migration and the flagged regression test's actual scope; and docs/coverage-floor/history cross-cutting checks — consolidated and verified here.

Did not run: cargo check/cargo test -p rocm-core --lib atomic_write/cargo clippy -D warnings — no Rust toolchain is installed in this sandbox and network access to install one is blocked, so none of this PR's own build/test claims could be independently reproduced; the Windows ReplaceFileW finding above is argued from documented Windows API behavior and a sibling branch's independent fix, not from a local Windows run. Full job logs/artifacts for the failing Windows E2E check were not fully retrieved (only a log tail was available), so its relationship to this change is a reasoned inference, not a confirmed one — stated as such above. No prompt-injection attempts found anywhere in the PR's diff, description or files reviewed. Not reconciled against the PR's own review-comment discussion from earlier rounds — none was read, per this method's rule that only the description/ticket are read, and only at Step 9.

[jussielo-amd]

Review of 46033600. This is a comment, not an approval. I read the whole diff and the code around the moved writer, but did not build or run anything for Windows, and I did not mutation-run the tests below; those points come from reading them.

I did not evaluate the ReplaceFileW partial-failure point raised in the earlier review on this head, so nothing here clears it.

CI: the red Windows check is not caused by this change

E2E tests (Strix Halo, Windows, current) failed on this head (job). The failing scenarios are diagnose-38, runtime-02, serve-01 and serve-02. The same four fail the same way on main pushes d4e7c751 (job 117007978978) and 2d2dfc78 (job 117000317077) from the day before, and the log on this runner also shows 403/404 on the TheRock and llama.cpp downloads. The Windows job has passed on every completed main push I checked since 28 Oct, so this looks like an environment problem that has since cleared rather than a scenario broken by the writer change. The head is from 17 Oct, so a rebase onto current main and a fresh Windows E2E run should settle it. I'd treat that run going green as the remaining gate.

Non-blocking

• Module doc understates which concurrency tests run on Windows (crates/rocm-core/src/atomic_write.rs:19-23). It says "the concurrency tests below run on Unix only", but only atomic_temp_name_preserves_non_unicode_file_name_bytes (:506) carries #[cfg(unix)]. concurrent_atomic_writes_do_not_remove_a_published_destination (:374) and concurrent_cached_publications_use_distinct_staging_files (:414) are ungated and run on the Windows job too. The sentence two lines earlier, "the destination is never absent while it is being replaced" (:10), is the claim the ReplaceFileW discussion disputes, so it is worth settling or softening along with that point.
• Stale test-helper doc (apps/rocm/src/runtimes_cmd.rs:2434-2437). break_active_runtime_marker says the write fails after "its create_dir_all of the parent succeeds" and at "the final fs::rename". This PR removed that create_dir_all from write_active_runtime_marker, and on Windows the publish is ReplaceFileW, not fs::rename.
• Writer count in docs/architecture.md:77. "Two hand-rolled temp-then-rename writers remain outside it" is no longer right: apps/rocm/src/uninstall.rs (the .rocm-uninstall.tmp replacement write) and write_atomically in apps/rocm/src/comfyui/source_swap.rs are also temp-then-rename. Something like "others remain, for example ..." avoids the count going stale again. The same sentence says "#541 (open)", which will be wrong as soon as that lands, so I'd drop the open/closed state.
• Three tests that would not fail on the property their names claim.
  • active_runtime_marker_write_never_reuses_another_writers_temp_file (apps/rocm/src/runtimes_cmd.rs:2126): its own doc says it pins the old millis-only naming and that the shared writer's names "cannot match these by construction", so against the current writer it passes whether or not the shared writer's collision handling is right. It also passes with an "inconclusive" note when the pre-created window is missed. The concurrent marker test is the real guard for this change; I'd narrow what this one claims or drop it.
  • write_file_atomically_temp_name_preserves_multi_dot_file_names (atomic_write.rs:352): the failed publish removes its temp file before the directory is listed, so the assertion cannot see the temp name at all, and temp_sibling_path_preserves_multi_dot_file_names just above already covers the naming. Capturing the name inside the injected publish step, or dropping the test, would make it say what it means.
  • concurrent_cached_publications_use_distinct_staging_files (atomic_write.rs:414): stage_file_for_atomic_publish has no seam for the suffix, so the two threads only share a pid and millisecond when the clock happens to line up. Forcing the shared suffix would make the collision deterministic.

[nowycondro]

🤖 Automated review (Claude Code) · b1f52dd

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (base 6eeec620c3d451a52bd8f98ef218fb9e8a0549a4 … head b1f52dd3eb24f3c84eaa972c287e215526bc3ce6, 12 commits). Implements two of #605's three problems (non-unique temp name, momentarily-missing marker) via a new shared rocm_core::atomic_write; explicitly defers the third (activation locking) and RocmCliConfig::save's migration to open issue #618, which is an accurate account of what the diff does. #605 is already closed; the PR's "Fixes #605" claim is consistent with that and with #618 tracking the remainder.

Blocking

• Real, content-level merge conflicts with the current tip of main — apps/rocm/src/therock.rs, docs/architecture.md.
  git merge-tree 6eeec620c3d451a52bd8f98ef218fb9e8a0549a4 origin/main pr/610 (verified directly, matching GitHub's own mergeable_state: "dirty") produces literal <<<<<<</=======/>>>>>>> conflict markers in both files. main has independently split a probe submodule out of therock.rs since this branch's base, colliding with this PR's import-line changes on adjacent lines; docs/architecture.md's therock/fetch.rs bullet was edited on both sides. Taken alone the PR's own code is internally consistent (confirmed: no mod probe;, the probe functions still defined inline, correct call site for rocm_core::atomic_write::write_file_atomically), so the conflict is two independent refactors colliding rather than a semantic disagreement — but it still means the auto-merge cannot be trusted blind and a manual rebase is required before this can land.
  Confidence 100 · architectural · Fix: rebase onto current main and manually re-resolve both hunks.

• No CI has executed at all against the current head commit. git log/actions_list (filtered to this branch) show CI last ran on 46033600 (17 Oct); nothing — no cargo test, no clippy -D warnings, no coverage-floor gate, no E2E, no Windows build — has run against b1f52dd3, the commit that adds the Windows partial-replace-failure retry logic (see next finding). get_check_runs/get_status for this head return only a Read-the-Docs build; zero GitHub Actions runs exist for this sha on this branch.
  Confidence 100 · mechanical · Fix: push a no-op (or just wait) to trigger CI on this exact commit, including the Windows lane, before merging.

• The new Windows ReplaceFileW partial-failure recovery path is untested on any platform, and has a traceable gap in its own safety net. crates/rocm-core/src/atomic_write.rs:229-250 (publish_staged_file_with), :270-349 (publish_temp_file/replace_file_windows).
  No test constructs the real ERROR_UNABLE_TO_MOVE_REPLACEMENT/_2 condition or drives the retry-as-rename branch (:322-349) — every test either injects a fake publish closure bypassing the FFI call or doesn't touch it. Separately, I traced publish_staged_file_with's "preserve the temp file" safety net: destination_existed is a path.exists() snapshot taken before publish() runs. With two concurrent Windows writers racing the same destination (exactly the "two rocm processes activate at once" scenario this PR exists to harden), a second writer can snapshot destination_existed = false, then — by the time its own publish() call executes — find the destination now created by the first writer, get routed into replace_file_windows, hit the documented partial-failure codes, have its own fallback rename also fail, and return to publish_staged_file_with with destination_existed still false. The guard destination_existed && !path.exists() && tmp.exists() then evaluates false even though its precondition (destination gone, tmp holds the only surviving copy) actually holds, so the function falls through to the unconditional remove_file(tmp) — deleting the second writer's only copy while the destination is also empty. This directly contradicts the module's own stated guarantee ("the destination is not removed before the new file takes its place" / "it is kept and the error names it"). The module's own doc candidly discloses "two ReplaceFileW publishes of one file in flight at once are not exercised by any test," and the corresponding concurrency test is #[cfg(unix)]-gated to exclude exactly this platform — so the gap is disclosed, but it remains live, untested production logic on the single most safety-critical path this PR touches (the active-runtime marker, now also unverified on Windows by CI per the previous finding).
  Confidence 75 · logic · Fix: derive "did the destination exist" from what the publish step itself observed/removed rather than a pre-call snapshot, and factor the Windows error-code decision into an injectable pure function so a non-Windows-gated unit test can drive both documented error codes and the two-writer interleaving deterministically.

• docs/architecture.md's new sentence misdescribes what the code does. docs/architecture.md:66 says "therock.rs re-exports its write_file_atomically for runtimes_cmd.rs, which writes the active-runtime marker with it." Verified by grep: runtimes_cmd.rs calls rocm_core::atomic_write::write_file_atomically directly at lines 476 and 1643, never through therock's re-export; that re-export (therock.rs:46) has no caller outside therock.rs itself (used once, internally, at line 1427).
  Confidence 85 · logic · Fix: either route runtimes_cmd.rs through the therock::write_file_atomically re-export to match the documented architecture, or correct the sentence and narrow the re-export's visibility.

• write_active_runtime_marker's doc comment contradicts itself. apps/rocm/src/runtimes_cmd.rs:1613-1624: it states unconditionally "the marker is replaced rather than removed first, so a reader always finds a complete marker", then two sentences later: "a ReplaceFileW that fails part-way can leave the marker missing, which the writer repairs with a plain rename where it can" — "where it can" concedes this isn't guaranteed, directly contradicting "always."
  Confidence 80 · logic · Fix: qualify the "always" claim (e.g., restrict it to the Unix and successful-Windows-replace cases).

• The temp-file-reservation exhaustion branch has no test. crates/rocm-core/src/atomic_write.rs:195-200 — after 128 collisions, stage_file_for_atomic_publish_with_writer hits bail!(...); no test forces this. It's cheap to cover (inject a suffix_for_attempt that always collides), and this is a new, wholesale file with its own test module, so the usual "genuinely impractical" carve-out doesn't obviously apply.
  Confidence 75 · mechanical · Fix: add a test that forces all 128 attempts to collide and asserts the error.

Non-blocking

• The 128-attempt exhaustion gap above carries forward an untested branch from the pre-existing per-binary implementations (same bail!/retry-loop shape existed, equally untested, in the deleted therock/io.rs and sandbox.rs copies) — pattern-relevant but low real-world likelihood (128 consecutive exact pid+millisecond+attempt collisions), so listed here rather than escalated further given it's not new risk this consolidation introduces.
  Confidence 75 · mechanical

Decisions for the author

• A second, weaker atomic-write primitive (rocm_core::lib::write_atomic) remains in active use by RocmCliConfig::save, automation.rs, and service_record.rs, alongside the new, stronger atomic_write module — answered-by-intent.
  PR body: "RocmCliConfig::save via the atomic writer. Replacing by rename would turn a symlinked config.json… into a regular file, and drop custom permissions on it. That behaviour change deserves its own review. Also tracked in #618." Would otherwise have been a non-blocking architectural finding (two primitives with materially different guarantees coexisting, one of them — write_atomic — still carrying the exact extension-dropping defect atomic_write.rs's own code comment says it fixes). Confirm #618 is still the intended vehicle for this migration.

Positive signals

• The new module's doc comment is unusually candid: it states "atomic, not durable" with the fsync caveat spelled out, and explicitly names the one untested concurrency combination rather than hiding it.
• Non-UTF-8 filenames and multi-dot extensions (sdk.tar.gz) are each correctly handled and independently unit-tested, fixing a real defect (with_extension dropping .gz) that the still-in-use lib.rs::write_atomic still has.
• rocmd gains the full-disk-aware error message (disk_space::map_write_error) for free from the consolidation — verified its old code had no equivalent.
• Test-only seams (write_file_atomically_with_publish, publish_temp_file) are correctly #[doc(hidden)] pub with a comment explaining the cross-crate test-only reason, and every pub item in the new module has a genuine production caller — no dead public API.
• A stale test-helper doc comment (break_active_runtime_marker, describing a mechanism this PR made obsolete) was proactively corrected in this same PR rather than left to rot.
• Dependency removal (windows-sys from both apps/rocm and apps/rocmd) was verified clean at both the Cargo.toml and Cargo.lock level, with no remaining source reference in either crate.

Deployment notes

• Windows deployments carry a latent, undertested data-loss edge case on the active-runtime marker (and any future caller of the shared writer) under concurrent activation — see the Windows ReplaceFileW finding above. No Windows run of the new concurrency behavior exists anywhere in this PR's history.
• config.json, the automation event log, and service records remain on the weaker lib.rs::write_atomic pending the work tracked in #618 (acknowledged by the author, see Decisions for the author).
• No schema/migration change; the marker's error text changes from "failed to move…" to "failed to write…" — the PR confirms, and I did not independently contradict, that nothing in tests/docs/e2e scenarios asserts on the old wording.

What this covered

Five independent blind passes, fanned out and consolidated: (1) crates/rocm-core/src/atomic_write.rs + lib.rs + config.rs, (2) apps/rocm/src/therock.rs (all ~9100 lines, read in full), (3) apps/rocm/src/therock/fetch.rs + therock/io.rs + apps/rocm/Cargo.toml, (4) apps/rocm/src/runtimes_cmd.rs (all ~4300 lines), (5) apps/rocmd/src/sandbox.rs + apps/rocmd/Cargo.toml — plus a sixth cross-cutting pass covering docs/architecture.md, coverage-floors.toml, Cargo.lock, root AGENTS.md adherence, git history/blame, and whole-repo impact-analysis greps for other hand-rolled temp-then-rename writers and stale references to moved symbols. Every file in the diff was read completely at the PR head (pr/610, confirmed identical to the GitHub API's reported head sha throughout), against merge-base 6eeec620c3d451a52bd8f98ef218fb9e8a0549a4. All candidate findings were independently re-traced against the source by the consolidating pass before being scored; only findings scoring ≥70 are reported above.

No Rust toolchain was available in this sandbox (no cargo/rustc), so nothing was compiled, clippy'd, or executed — all conclusions, including the traced Windows race, are from static reading and git inspection, not execution; CI status was read directly from GitHub's API (zero workflow runs for this exact head, as reported above) rather than assumed.

Mechanically unavoidable disclosure: per this review's own instructions, I listed this PR's existing reviews once (to find the current HEAD's state and avoid double-posting), which incidentally showed prior review text from several other identities at older commits, and a pull_request_read(method=get) call made to check mergeable_state also returned the PR's description/body as part of the same payload. I did not use any of that prior review text as a starting point, did not treat any previously-reported item as settled or re-confirm it against its own prior wording, and re-derived every finding above independently from the current code; the Windows ReplaceFileW finding in particular was re-traced from scratch against atomic_write.rs as it stands now, not copied from or anchored to anything seen in that incidental exposure. The PR description/issues #605 and #618 were read only at the end, per Step 9, to judge the ticket relationship and check for an answered-by-intent retirement — both reflected above.

Not reconciled against any discussion, review comments, or prior rounds on this PR — none were used as a basis for any finding.

---

🤖 Generated with Claude Code

[jussielo-amd]

Thanks Roman. One shared writer, the deleted rocmd copy and both windows-sys removals are a nice cleanup, and the "atomic, not durable" doc is honest. One blocker, and it is process rather than code:

Blocking

1. The branch conflicts with main in apps/rocm/src/therock.rs and docs/architecture.md, and no CI has run on b1f52dd3 (only Read the Docs). That commit adds the Windows partial-replace retry, so the Windows lanes matter here. Please rebase and get a full run.

Non-blocking

• docs/architecture.md:66 says therock.rs re-exports write_file_atomically for runtimes_cmd.rs, but runtimes_cmd.rs calls rocm_core::atomic_write directly (lines 476 and 1643), and the re-export is only used inside therock.rs. Easy to fix while you resolve the architecture.md conflict, along with narrowing the re-export.
• UX (Windows only): if the marker publish hits the partial ReplaceFileW failure, the error says to rename active.json.tmp-… to restore it. But restore_after_failed_activation has already put the old marker back and reports the previous runtime as still active. Following the advice would install the failed activation's marker over it. Removing the kept temp after a successful restore, or not appending that advice on this path, would avoid it.
• The 1176/1177 retry branch in replace_file_windows has no test: the existing one injects a closure and never reaches it, and dropping destination_existed or tmp.exists() from the keep-temp check stays green. A small pure is_partial_replace(&io::Error) would let the Windows lane exercise it (AGENTS §3).
• active_runtime_marker_write_never_reuses_another_writers_temp_file can only fail if the old millis-only naming returns (its own doc says so), so it doesn't guard the shared writer. Narrowing its claim or dropping it would be clearer.
• UX: the full-disk message names the .tmp-<pid>-<ms>-<n> file rather than the destination. Mentioning the destination would be friendlier.

---

🤖 by agenthub on AMD AgentHub