Repository navigation
ROCMAI-724: retire rocm-core's write_atomic onto the shared atomic_write module - #811
Draft
jussielo-amd wants to merge 14 commits into
Draft
jussielo-amd wants to merge 14 commits into
jussielo-amd wants to merge 14 commits into
Conversation
`apps/rocm/src/therock.rs` and `apps/rocmd/src/sandbox.rs` each carried their own copy of `write_file_atomically` and its staging/publish helpers. Move the `therock` copy, the more complete of the two, into `rocm_core::atomic_write` and call it from both binaries, so the next caller (the active-runtime marker) gets the same implementation rather than a third copy. The move is mechanical apart from the error text. rocmd gets two extra pieces of context it was missing: the full-disk mapping through `disk_space::map_write_error`, and "failed to publish <path>" when the rename fails. The `failed to create <parent>` context that only rocmd had is kept. The helper tests move with the code. The five rocmd tests that duplicated therock tests are dropped. The one rocmd test that drove the temp-name check through a real write is kept. Count: 15 tests before (two of them ignored), 10 now (one ignored), with no assertion lost. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`write_active_runtime_marker` used to stage `runtimes/active.json` at `active.json.tmp-<millis>` with a truncating write, remove the marker, then rename the temp file into place (ROCm#605). Two activations against one data dir in the same millisecond therefore wrote through one shared temp file. One of them failed with "failed to move active runtime marker". The other could publish its payload followed by the tail of a longer one, which later readers such as `rocm runtimes uninstall` could not parse. Between the remove and the rename, the marker did not exist at all. write itself does not change here beyond going through `rocm_core::atomic_write::write_file_atomically` (the one writer the previous commit moved into rocm-core) for both the forward write and the failed-activation restore. Each writer stages into its own `create_new` temp file named with pid, millis and attempt, and the marker is replaced in one step rather than removed first. The doc comment now says what that does and does not guarantee for two concurrent activations. Two regression tests pin it: - a deterministic one pre-creates a file at every name the old scheme could pick during the test and asserts none is written through or moved; - a Unix-only stress test runs two writers in lock-step plus a reader, and asserts every write succeeds and the reader always finds a complete marker. Against the pre-ROCm#424 body, 190 of 400 writes failed. Re-adding only the remove before the publish made the reader miss the marker thousands of times. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The rocmd sandbox module no longer carries its own atomic-write helpers; record where the shared one lives so the next caller reuses it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
clippy's too_long_first_doc_paragraph rejects the two seam doc comments carried over from therock, where the private functions were not linted for it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
With the atomic writer moved to rocm-core, sandbox.rs uses std::fs only in its Linux bubblewrap path and its tests, so the Windows build warned about an unused import. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Moving the atomic writer out of rocmd's sandbox.rs took its helper code and six tests with it. Those lines were close to fully covered, and the line count includes test modules, so rocmd's percentage drops although nothing it still owns lost coverage. The code and its tests are now measured under rocm-core. Only rocm's tests reach rocmd, so `cargo llvm-cov --no-cfg-coverage -p rocm -p rocmd` measures rocmd exactly as the workspace run does. Measured locally that way, rocmd went from 75.30% (7776 lines) on the base to 75.02% (7549 lines) here. That is a 0.28pp drop, past the 0.2pp tolerance. The floor is set to the branch measurement. Only this one line is changed. A full `--bless` from a local run would also rewrite every other crate's floor from a different host's numbers. rocm moved by -0.04pp, inside tolerance, and is left as is. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`ReplaceFileW` was rocmd's only use of `windows-sys`; it moved into `rocm_core::atomic_write` with the rest of the atomic writer, and rocm-core declares its own dependency. Also narrow the marker regression test's doc comment: it guards against a return to the millis-only temp names, while collision safety under the current names is covered by the concurrent tests. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
ReplaceFileW was apps/rocm's only use of windows-sys, and it moved into rocm_core::atomic_write with the rest of the writer; rocm-core declares its own dependency. Same leftover as the rocmd one removed earlier. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
… disk The module doc now gives the real reason for ReplaceFileW (a sharing violation while another process holds the destination open; std rename does replace), says Windows concurrent publishes are untested, and says atomic but not durable, since nothing is fsynced. The test seams no external production code calls are narrowed: write_file_atomically_with is private, and the two that apps/rocm's cross-crate tests drive stay pub but #[doc(hidden)] and say they are seams. The staging doc no longer implies an outside caller can publish and clean up on its own. The full-disk test called disk_space::map_write_error directly, so it stayed green if staging stopped mapping errors. It now drives the real staging path with the byte write swapped for one that fails like a full disk, and checks the message and that no temp file is left. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…#424 ROCm#424 already fixed the temp-name collision and the missing-marker window on main. The two tests now say they pin that the marker keeps those properties through the shared writer, not that they are the regression for ROCm#605. The collision test no longer fails when the write lands after its pre-created names: it retries with a window sized from how long the pre-creation and write took, and where the filesystem is too slow for any window to cover the write it reports itself inconclusive and passes. The marker writer's doc says the Windows replace is untested under concurrency. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…ite module The remaining callers of write_atomic (the automation event and proposal store rewrites, the service-record manifests, the config save, and the Lemonade pins restore and put-back) now publish through rocm_core::atomic_write::write_file_atomically. write_atomic and its two unit tests are deleted: the atomic_write module's own tests and the config save tests already cover what they asserted. Temp files now get a unique, exclusively created name instead of a deterministic clock-and-pid one, so two writers can no longer share a temp file and publish each other's bytes. The shared writer also creates a missing parent directory, so the Lemonade failed-put-back test now forces the failure with a non-empty directory at the target instead of a missing directory. Six per-caller tests pin that a writer never reuses another writer's temp file. Each was shown to fail with the old behaviour restored. docs/architecture.md no longer lists write_atomic as a remaining hand-rolled writer. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The foreign-temp-file probe that backs the six per-caller tests could pass without proving anything: on a host that creates files at about a millisecond each, pre-creating the names took as long as the window they covered, and the probe then printed a notice and returned Ok. On the machine this was measured on, that was the normal case, not a corner. The window now starts a lead after the names are created and the write only runs once it has begun, so slow file creation no longer decides the outcome; a retry lengthens the lead or the window, and a host too slow for any of them fails the test off Windows instead of passing. On Windows the probe still tolerates an inconclusive run, since the cost of creating files there has not been measured; that is stated in its doc comment. The probe now has its own tests: it fails a writer that reuses a name fixed by the clock, spares one that picks its own, reports an inconclusive run, and turns that into a failure off Windows. The probe was duplicated in rocm-core and in the Lemonade engine, and the duplicate would have needed the same tests. It now lives once in rocm_core::foreign_temp_probe, compiled for rocm-core's own tests and under a new test-support feature that the Lemonade engine enables only from its dev-dependencies, so no shipped build contains it. Only retired_pin_temp_path stays in the engine. The atomic_write module header said the writer is shared by rocm and rocmd; it now names the rocm-core and Lemonade callers too. The Windows publish step is untouched: ReplaceFileW can leave the destination name briefly absent, which the lock-free automation readers read as an empty store. That property belongs to the shared writer, not to this change, and is left for a separate decision. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…wo stale doc claims The foreign-temp-file probe's inconclusive handling now takes the Windows tolerance as a parameter, so both the tolerated and the failing branch are tested on every platform instead of only the failing one off Windows. The two self-tests that drive the probe against a writer that reuses a name and one that picks its own now rerun when a loaded host leaves the probe inconclusive, while still failing if the probe reports a clobbered name as spared or blames a clean writer. architecture.md no longer states an exact count of hand-rolled temp-then-rename writers outside atomic_write (the shell-profile rewrite in uninstall.rs is one more), and lists the probe module with the other rocm-core modules. The doc on the config save cleanup test now names the module the cleanup lives in. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
The Windows replacement can briefly hide the destination, violating the migrated stores’ no-gap reader contract and causing readers to return defaults or an empty store instead of the old or new contents.
1 open issue
What changed in this PR
Retires rocm-core::write_atomic by moving its remaining callers to the shared atomic writer introduced in #610. The diff also includes that dependency’s writer consolidation.
Changes:
- Migrates config, automation, service manifests, and Lemonade pin writes to the shared implementation.
- Adds per-caller regression tests and a shared test-only probe for foreign temporary-file preservation.
- Removes duplicate writers and tests, updates dependencies and coverage metadata, and documents the architecture.
Verification here was limited to source inspection; native Windows behavior was not tested.
| File | Description |
|---|---|
| engines/lemonade/src/test_support.rs | Adds retired pin-temp naming helper. |
| engines/lemonade/src/install.rs | Migrates pin writes and adds regression tests. |
| engines/lemonade/src/backend_alignment.rs | Updates the publish-failure fixture. |
| engines/lemonade/Cargo.toml | Enables shared test support for tests. |
| docs/architecture.md | Documents the shared writer and remaining implementations. |
| crates/rocm-core/src/test_support.rs | Adds retired temp-name helper. |
| crates/rocm-core/src/service_record.rs | Migrates manifest writes and tests temp-file isolation. |
| crates/rocm-core/src/lib.rs | Exports new modules and removes the retired writer. |
| crates/rocm-core/src/foreign_temp_probe.rs | Adds shared foreign-temp preservation probe and self-tests. |
| crates/rocm-core/src/config.rs | Migrates config saves and adds regression coverage. |
| crates/rocm-core/src/automation.rs | Migrates store rewrites and torn-tail repair. |
| crates/rocm-core/src/atomic_write.rs | Centralizes staging, publication, cleanup, and tests. |
| crates/rocm-core/Cargo.toml | Declares the test-support feature. |
| coverage-floors.toml | Adjusts rocmd’s floor after writer extraction. |
| Cargo.lock | Removes unused application dependency entries. |
| apps/rocmd/src/sandbox.rs | Uses the shared writer and removes its duplicate. |
| apps/rocmd/Cargo.toml | Removes the direct Windows API dependency. |
| apps/rocm/src/therock.rs | Uses shared helpers and relocates writer tests. |
| apps/rocm/src/runtimes_cmd.rs | Migrates marker writes and adds regression tests. |
| apps/rocm/Cargo.toml | Removes the direct Windows API dependency. |
Review effort: Balanced
Comment on lines
+245
to
+246
| if path.try_exists()? { | ||
| return replace_file_windows(path, tmp); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #610, which introduces the shared
atomic_writemodule this change migrates onto. Until it merges, this diff also shows #610's commits.Why
rocm-core still carries a crate-root
write_atomicalongside the sharedatomic_writemodule, so atomic file replacement has two implementations with different guarantees. The clock-and-pid temp name the old one uses lets two writers share a temp file and publish each other's bytes.What changes
The last callers of
write_atomicmove ontorocm_core::atomic_write::write_file_atomically, andwrite_atomicand its two unit tests are deleted. The callers are the automation event and proposal store rewrites (including the torn-tail repair), the service-record manifests,RocmCliConfig::save, and the Lemonade pins restore and put-back. The two deleted tests asserted that success leaves no temp file and that a failed rename leaves the destination and directory untouched; theatomic_writetests and the config save tests already assert both, so no assertion is lost.Behaviour changes, deliberately:
RocmCliConfig::savealready replaced via rename, so migrating it changes nothing for symlinks or modes. If a stronger shared writer lands, behaviour for these callers (fsync, mode and symlink handling) changes with it.Verification
Six per-caller tests pre-create foreign files at the retired temp names and assert the write leaves them byte-identical and the destination correct. Each fails when the old behaviour is restored. They share a probe in a new rocm-core module behind a
test-supportfeature that only the Lemonade engine's dev-dependencies enable, so no shipped build contains it; the probe has its own self-tests. Off Windows an inconclusive probe fails the test; on Windows it is tolerated and reported on stderr.No new Gherkin scenario is needed: command output, exit codes and retained files do not change. The existing scenarios config-01, config-02, automations-08, service-stop-01/02 and service-cleanup-01 to 07 exercise the touched writers and passed locally. Torn-tail repair and the Lemonade pin restore have no scenario and are covered by unit tests.
Not verified: nothing Windows-specific was run, including the publish step's behaviour and the probe's Windows tolerance, because no Windows host was available; the Windows CI job is the only check. Coverage floors for rocm-core and Lemonade were not re-measured after the probe moved.
Scope
docs/architecture.mdno longer listswrite_atomicas a remaining writer and namesremote/session.rs,comfyui/source_swap.rsanduninstall.rsas the remaining hand-rolled writers. Theruntimes_cmd.rsmarker test from #610 could later reuse the shared probe.