[volen-silo]

Problem

E2E tests (Strix Halo, Windows, current) fails in its prewarm step, before any scenario runs: rocm engines install lemonade exits 1 with HTTP error 403 downloading therock-dist-windows-gfx1151-7.14.1.tar.gz.

The 403 is not the cause. The runner's shared Lemonade tree keeps a patched resources/backend_versions.json from an earlier successful ROCm alignment (therock.version=7.14.1 and a newer llama.cpp tag). The TheRock tarball that pin names is not served, so the install fails. Alignment never runs again because the pin already equals the active ROCm version, and the alignment revert is the only other thing that rewrites the pin. The message tells the user to run --reinstall, but the prewarm step does not pass it, so the failure repeats on every run.

Evidence, from the logs of 8 runs of that lane: the runs that start from a stale first-attempt tag fail, and the runs that start from the packaged tag hit the same 403 and 404 responses, revert to the packaged 7.13.0 defaults and pass. That is why the lane looks flaky. It has failed on main pushes as well as on PRs, depending on which runner state it landed on.

Change

When installing the already-pinned backend fails, the install restores the two pins that backend alignment writes (therock.version and llamacpp.rocm-stable) from the digest-verified cached embeddable archive, and installs once more, forced. docs/engine-plugins.md is updated, with short notes in README.md, docs/testing.md and docs/manual-testing.md; docs/architecture.md's write_atomic entry now says the function is pub and has a Lemonade caller.

This is a change to what a plain rocm engines install lemonade does: on a failed install of a kept pin it can print a warning, rewrite those two pins and replace the selected backend, where it used to report the failure and leave both alone.

• The restore never downloads. It runs after an install already failed (often because the network is down), so it reads the cached archive and nothing else. The same install has just verified that archive into downloads/ (every install does, before the backend step), so the cache is normally there. If it is gone or does not match its digest, the restore is skipped, a warning says why, and the original install error is reported.
• Only those two pins are restored. Every other entry of backend_versions.json is kept, so a hand edit to another entry survives. If the file is missing or is not JSON, the packaged file is written whole. If the two pins already hold the packaged values, nothing is rewritten. The pin file is read after the archive work, immediately before the rewrite, and that read is also what the backup holds, so a pin written by another process in between is not overwritten blindly.
• The retry is a forced reinstall into the shared tree, so it is wrapped the way alignment's own revert is. The existing ROCm backend directories are set aside first, and put back if the retry fails, so a failed retry does not leave the host without a ROCm backend. A retry that succeeds does not leave two builds side by side for rocm serve to choose between. The ROCm backend entries are listed before the first attempt, and after a failed retry anything that was not in that list (for example a partial build the first attempt left) is removed once the directories are back. The directories are not set aside before the first attempt, because that would move a working backend on every plain install.
• What is reported when it does not recover.
  • If the pins already hold the packaged values, the cache is missing, the pin file exists but cannot be read, or the restore itself fails, there is no second attempt and the original install error is reported.
  • If the second attempt fails too, its error is reported (the first error is printed in the warning), the backend directories are put back, and the previous pin file is put back. Any install failure starts the restore, including a transient network or disk error, so a pin that was fine must not stay discarded by a retry that failed for another reason.
  • The warning names the root cause of the first failure, not the whole chain. The chain carries the advice to run --reinstall, which the CLI is already doing.
• Writes are atomic. The restore and the put-back go through rocm_core::write_atomic, which is now pub for this.

ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT disables this path too. The existing alignment code's own edit of the pin file is unchanged.

Tradeoffs and known limitations, stated rather than hidden:

• The restore starts after any install failure, including a transient one, and the first attempt has already retried once. If the restored retry then succeeds, the kept aligned pin is gone, and the next plain install sees a pin that differs from the target and runs alignment again. The alternative is a stale pin blocking every install until someone runs --reinstall.
• The restore resets only the two pins alignment writes. If a user edited those two by hand, they are reset too; ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT is the supported way to keep them.
• Kill window. Between setting the backend directories aside and putting them back, the working backend exists only under a .pre-alignment name; a process killed there leaves it there. The previous pin file is held in memory only, so a kill during the retry also leaves the packaged pins on disk (the next install runs alignment against them again). Alignment's own revert path has the same window. It is not recovered automatically because the runtime tree is shared between processes and a leftover aside cannot be told from another process's install in progress without a lock. It is documented in docs/engine-plugins.md; --reinstall rebuilds the tree.
• Scope. Only the branch where the kept pin equals the active ROCm version restores. The other places that install what the file pins (no active SDK, an unreadable pin file, a failed backend probe) do not.

Verification

Reproduced on a Windows Strix Halo host on the code of the previous head 620a872e (the amend since then only changes how the pin file is read for the backup and adds tests; the reproduction was not repeated on it) (debug builds of rocm and rocm-engine-lemonade; the base binary is origin/main at ea86403c), each in its own isolated config/data/cache directory:

1. rocm engines install lemonade once, cleanly (no managed SDK in that data dir): exit 0, packaged pins 7.13.0 / b9752, backend installed.
2. A hand-written registry record makes 7.14.1 the active managed ROCm version (this is a synthetic record, not a real SDK install; it only feeds the version lookup). Then backend_versions.json is patched to therock.version=7.14.1, llamacpp.rocm-stable=b11783 plus an unrelated key hand-edited-unrelated, and the installed ROCm backend directory is removed.
3. rocm engines install lemonade again:
   • base binary: HTTP 403 for therock-dist-windows-gfx1151-7.14.1.tar.gz, Lemonade backend installation failed again; run rocm engines install lemonade --reinstall ..., exit 1; the pins are left at 7.14.1 / b11783 and no backend is installed.
   • this change: the same 403 on the first attempt, then Warning: the llama.cpp backend pinned in resources/backend_versions.json could not be installed (...); restored the pins Lemonade ships and trying again., exit 0, pins back to 7.13.0 / b9752, the b9752 backend installed, and the unrelated key still present.

Unit tests in backend_alignment.rs and install.rs, each asserting the message together with the real state it claims:

• the restored warning, with the pin file holding the packaged pins;
• a failed retry: the previous pin file is back, the backend directories are back, a stray directory from the failed install is gone, and the message says so;
• a failed retry after the first attempt left a partial build and a new directory: the old build is back and neither leftover is;
• a failed restore: the warning names the error, and the pin file and backend directory are untouched;
• a failed put-back: the warning says so, and the restored pins are still what is on disk;
• the retry sets the backend directories aside and a success discards them (no two builds side by side);
• the warning names the root cause and not the --reinstall advice;
• the restore with no cached archive, and with a cached archive that fails its digest: nothing is downloaded, the error says so, the pin file is untouched, and no download or scratch directory is created;
• the restore reads the pin file after the archive work: a pin written in between is what the backup holds and what the rewrite preserves;
• a pin file that exists but cannot be read is not treated as absent: the restore errors before writing anything, the original install error is reported and the file is left as it is (a failed retry can no longer delete it);
• a packaged file without llamacpp.rocm-stable removes that key from the current file, and a drift in only that key triggers the restore;
• the restore keeps entries a user edited, is a no-op when only other entries differ, replaces a file that is not JSON, and the backup puts the previous bytes back and leaves no temp file.

I changed the production code under each new test and it fails: removing the prune after a failed retry, removing the missing-cache check (the restore then reports a digest mismatch instead of the not-cached error), moving the pin-file read back before the archive work, treating every read error as an absent file, making the remove-a-pin arm a no-op, and making the drift check ignore llamacpp.rocm-stable; the equal-pin restore-failure test now asserts the restore was attempted and fails when the restore step is skipped. The earlier tests were mutation-checked on the previous version of this change. Each mutation was reverted and the file compared byte-identical.

On Windows: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo clippy -p e2e-cucumber --test e2e -- -D warnings, cargo xtask check-crate-edges are clean, and cargo nextest run -p rocm-core -p rocm-engine-lemonade passes 922 tests (one reported as leaky: rocm-core process::tests::a_timed_out_child_has_its_output_quoted_within_a_bound, which this change does not touch). Under WSL2, cargo test -p rocm-engine-lemonade --lib -- restore pin backup retry passes (33 tests).

Not covered / gaps (AGENTS.md section 3)

• No new Gherkin scenario. The behaviour needs a runtime tree that already holds a stale pin and a live backend download, so it could only be a gated scenario. The lane that exercises it is E2E tests (Strix Halo, Windows, current): its prewarm step runs rocm engines install lemonade against the runner's shared tree, and it is the step that fails today. It only exercises this change on a runner that still holds the stale state. That lane is not in this head's check list, so no CI hardware run has shown the fix; the manual reproduction above is the evidence, and the lane's result after merge is what confirms it.
• The runner already holds the stale state, so the lane only turns green once this change is on the branch the lane runs. Clearing the stale prewarm directory on the runners would also unblock it in the meantime.
• TheRock 7.14.x tarballs are not served from the download host at the time of writing, so clean installs still print noisy 403 and 404 lines before they fall back to 7.13.0. Whether they will be published is a question for the TheRock owners and is outside this change.
• Not run: the full cargo test --workspace (single process) and scripts/smoke_local.py outside the pre-push hook; cargo xtask e2e; the missing-cache branch against a real install (every install refills the cache first, so it is covered by the unit tests only); the reproduction used debug builds and a synthetic registry record, not a real 7.14.1 SDK install.

[nowycondro]

🔴 Automated review · pr-review-watcher · bd917f4

Review — needs work

Full review of the whole change (base efb9bc37…head bd917f4d, 3 files, 367/-2 lines). No ticket named; the PR body is a self-contained CI-failure writeup, not a tracker reference.

Blocking

• Description and docs claim the original error survives a failed retry; the code reports the retry's error instead — docs/engine-plugins.md:77, engines/lemonade/src/backend_alignment.rs:259-268
  The PR description and docs/engine-plugins.md:76-77 both state: "If the file already holds the packaged values, or the second attempt fails too, the original install error is reported unchanged." Tracing install_pinned_backend_or_restore_packaged_pins: when restore_packaged_pins returns Ok(true) and the forced retry (fallback_install(manifest, true)? at line 267) itself fails, the ? propagates that error, not the captured install_error. The PR's own test equal_pin_still_failing_after_the_restore_reports_the_retry_failure (backend_alignment.rs:1705) confirms this directly: it asserts the surfaced error contains "packaged pins" (the retry failure's text) and that it does not contain the first failure's text. Only the Ok(false) and Err(restore_error) branches actually report the original error unchanged; the third documented case does not. The code's behavior (surfacing the retry failure, which is what's actually actionable once packaged defaults are in play) looks like the right choice — the description and doc are what should change to match it.
  Confidence 90 · logic · Fix: correct the wording in the PR description and docs/engine-plugins.md to say the retry's own error is reported when the forced reinstall fails after a successful restore.

• The new forced reinstall skips this file's own established half-install safety net — engines/lemonade/src/backend_alignment.rs:267 vs. :398-407, :433-448, :1047-1097
  Every other forced reinstall in this file (align_llamacpp_backend_to_version's revert fallback at :398-407, try_llamacpp_backend_alignment at :433-448) wraps the install in set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside (:1047, :1087), whose own doc comments explain why: Lemonade extracts each build into its own build-numbered directory rather than replacing one in place, so a rejected/failed attempt can leave a stray directory that find_binary_in's priority-ordered lookup later picks up instead of (or alongside) the intended build — or leave the host with no working backend at all. install_pinned_backend_or_restore_packaged_pins's own comment names the identical hazard ("the failed attempt may have left a half-installed backend for the withdrawn pin") but calls fallback_install(manifest, true)? directly, with no aside/resolve wrapping. A retry that fails after a successful restore can leave exactly the stale/shadowing directory state this machinery exists to prevent.
  Confidence 80 · architectural · Fix: wrap the forced retry (and arguably the first attempt too) with the same set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside pair used elsewhere in this file.

• No Gherkin scenario for user-observable behavior this change introduces, and the stated justification doesn't fit AGENTS.md's own exceptions — AGENTS.md §3, PR description "Not covered / gaps"
  AGENTS.md §3 requires: "If the change alters what a user of the CLI can observe — command output, exit codes, files or paths the CLI creates, or which runtime/engine it selects — then that behavior must be covered by a Gherkin scenario in tests/e2e-cucumber/features/," with only three outs: an existing scenario (cited by @id), a new scenario gated to @requires-gpu/@nightly (named in the PR text), or a purely-internal change. This change adds new stderr warnings, rewrites a file the CLI manages, and turns a previously-failing install into a succeeding one — squarely in scope, and not purely internal. The PR's own gap note reasons that such a scenario "can only be a gated GPU or nightly scenario" and points at "the Windows lane's prewarm step" as "the live guard" — but that's an existing CI job, not a tagged Gherkin scenario, so even on the author's own reasoning the gated-lane exception's required action (add the scenario, tag it, name the lane) was never taken. Coverage is unit tests over injected closures plus one archive-round-trip test, which prove the decision logic and the file-rewrite mechanics but not the CLI-observable outcome end to end.
  Confidence 80 · architectural · Fix: either add a @requires-gpu/@nightly-gated scenario exercising the restore-and-retry path, or make the case explicitly for why this is exempt and get that judgment call confirmed rather than asserted.

Non-blocking

• Archive-extraction/root-finding sequence duplicated — engines/lemonade/src/install.rs:171-188 (prepare_embeddable_with) and :291-306 (restore_packaged_backend_versions_with)
  Both functions independently do "create a timestamped extract dir, extract_archive, find_embeddable_root, clean up the extract dir" with only the follow-up step (copy the whole tree vs. read one file) differing. Not a correctness issue — reuses already-hardened extraction code — but a reasonable candidate for a small shared helper.
  Confidence 70 · mechanical · Fix: factor the extract-into-temp-dir-and-find-root steps into one shared helper returning the embeddable root path.

• The restore function's own failure branches, and the new warning text, are untested — engines/lemonade/src/install.rs:273-311, engines/lemonade/src/backend_alignment.rs:259-268
restore_packaged_backend_versions_with's own new logic (digest mismatch via ensure_cached_archive, a packaged archive missing the resource entry) has no direct test — only the happy-path round trip (restore_packaged_backend_versions_replaces_only_a_pin_file_that_drifted, install.rs:1767) is exercised; the underlying ensure_cached_archive/verify_sha256 primitives are separately well-tested elsewhere, which limits the real exposure here. Separately, the new warning strings ("restored the pins Lemonade ships and trying again", "could not restore the pins Lemonade ships") are never referenced by any test (zero matches repo-wide), so AGENTS.md §3's "a message about the CLI's own behavior is asserted together with the behavior" guidance isn't exercised either way for this string — not a false-pinning risk since nothing pins it at all, just an uncovered edge.
  Confidence 70 · test-coverage.

• Doc's new recovery paragraph isn't cross-referenced from the earlier summary it extends — docs/engine-plugins.md:46 vs. :69-79
  The pre-existing sentence "skipped when the pin already matches the active version" (line 46) is still accurate (it describes the alignment rewrite being skipped, which this PR doesn't change) but a reader stopping there would not know the install in that case now has a recovery path, described eleven lines further down with no forward pointer. Not a contradiction, just a completeness/discoverability gap.
  Confidence 70 · mechanical.

Decisions for the author

• Full archive re-extraction just to read one JSON file — tradeoff
  restore_packaged_backend_versions_with extracts the entire embeddable archive to a temp directory solely to read resources/backend_versions.json out of it, rather than reading just that one archive entry directly. The current approach reuses the same hardened, tested extraction path prepare_embeddable_with uses (consistent traversal/safety guarantees) at the cost of needless I/O on a failure-recovery path that should be rare. A leaner single-entry read would avoid the extra work but would duplicate (or need to factor out) the archive-format-specific entry-reading logic instead. Confirm this tradeoff was made deliberately rather than by default.

Positive signals

• Digest verification is enforced before any restored byte is trusted (ensure_cached_archive's existing lock/verify path is reused, not re-implemented) — matches the archive-handling discipline already established in this file.
• Comparing the pin file as parsed JSON rather than raw bytes (install.rs:308-312) correctly avoids a spurious "restore" when a prior write only re-serialized equivalent content — a detail that's easy to get wrong and is handled carefully here, with a test for it.
• The five new backend_alignment.rs unit tests exhaustively drive the equal-pin branch's four outcomes (clean install, restorable drift, already-packaged, restore-fails, retry-fails-after-restore) through injected closures, without needing a real lemond spawn or network access.

Deployment notes

• This only changes behavior on the "pin already equals active ROCm version but install fails" path; ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT continues to bypass it exactly as before (verified: the disabled-check in prepare_llamacpp_backend_for_active_rocm_impl returns before this branch is ever reached). No migration, no new dependency, no config surface added.
• At review time the two CI lanes this PR directly targets — E2E tests (Strix Halo, Windows, current) and E2E tests (Strix Halo, WSL2, current) — had just finished and both report SUCCESS on this exact head commit, consistent with the fix working against the real failure mode described in the PR. The GPU-hardware lanes (MI300X, W7900, Strix Halo Ubuntu release/nightly) were still QUEUED at review time; confirm they finished green before merge.

What this covered

Read in full: engines/lemonade/src/backend_alignment.rs, engines/lemonade/src/install.rs, docs/engine-plugins.md at the reviewed commit, plus the unchanged call sites and helpers they depend on (ensure_cached_archive, extract_archive, find_embeddable_root, ensure_best_llamacpp_backend, install_response, set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside) and AGENTS.md in full. Diff reviewed: efb9bc3767…bd917f4d72 (the PR's merge-base to its head). Two independent blind passes were run in addition to this driving pass — one a code-correctness pass over the two Rust files, one an agent-instruction-adherence/impact-analysis pass over AGENTS.md and the doc — and their candidate findings were independently re-verified against the source before inclusion above; every finding reported here was confirmed directly against the code, not taken on their word.
Did not run: cargo test/clippy/fmt locally (no Rust toolchain available in this review environment) — relied on the PR's own stated local verification and on live CI status instead, both checked immediately before publishing. History (git log/blame) on the touched files showed no prior related work conflicting with this change. No prior reviews existed on this PR to reconcile against.

[juhovainio]

The fix does what it says: when a kept alignment pin stops installing, the packaged pins are restored and the install is retried once, forced. I found nothing that breaks that path, but a few things should change before merge.

• The docs and PR description say that if the second attempt also fails, the original install error is reported. The code returns the retry's error, and a test pins that. Reporting the retry error is fine; the docs and PR body should say so.
• Any install failure triggers the restore, including transient network or disk errors. If the retry then fails too, backend_versions.json stays rewritten and a good kept pin is lost. It self-heals on the next run, but it is a surprising side effect. Inline comment below.
• The restore can re-download the embeddable archive when the cache is gone. The docs should mention that.
• The pin-file write is not atomic.
• The new warning says the packaged pins were restored, but no test ties that message to the resulting state, and there is no Gherkin scenario. The PR text justifies the missing scenario and names the Windows prewarm lane. It would help to state which gated lane exercises it, and to assert the message together with the pin-file state in one test.

Inline comments cover the first three code and docs points.

[juhovainio]

The code returns the retry's error here (fallback_install(manifest, true)?, and a test pins that), not the original one. Please reword, e.g. "the second attempt's error is reported", and fix the same claim in the PR body. Also worth noting that the restore may re-download the embeddable archive if it is no longer cached.

[juhovainio]

If this retry fails, the pin file stays rewritten even when the first failure was transient (network or disk), discarding a kept pin that may have been fine. Consider restoring the previous bytes when the retry fails, or only treating pin-specific failures as restorable.

[juhovainio]

Non-atomic write of the pin file: a crash mid-write leaves invalid JSON that the next run reads. Writing to a temp file and renaming is cheap.

[volen-silo]

Thanks for the careful review. The branch is rebased onto current main and force-pushed (4dfbb9a6). Point by point:

1. Original error vs the retry's error. You were right: the code returns the retry's error, and a test pins that. I reworded docs/engine-plugins.md and the PR body instead of changing the code. When the second attempt fails, its error is reported and the first one is in the warning. When the file already holds the packaged values, or the restore itself fails, there is no second attempt and the original error is reported.

2. A failed retry left the pin file rewritten. Fixed. The restore now hands back a backup of the bytes it replaced (or of "no file"), and a failed retry puts them back and says so. I did not try to limit the restore to pin-specific failures, because telling those apart from transient ones would mean matching error text, which is fragile; putting the previous file back removes the side effect either way.

3. Re-download. The docs now say the restore reads the cached archive and downloads it again if the cache no longer has it.

4. Non-atomic write. The restore and the put-back both write a sibling temp file and rename it over the pin file. The existing alignment code's own edit of that file is unchanged.

5. The message and the state. Two new tests assert the printed message together with the real pin-file content: a successful retry reports the pins as restored and the file holds the packaged values; a failed retry reports that the previous file was put back and the file holds the kept pin. I removed the put-back and the second test failed, then restored it. To make that testable the message goes through a small callback instead of a direct print. On the lane: the PR text now names E2E tests (Strix Halo, Windows, current), whose prewarm step runs rocm engines install lemonade against the runner's shared tree. It only exercises this change on a runner that still holds the stale pin, so I did not add a scenario that would need to fake that state.

[nowycondro]

🔴 Automated review · pr-review-watcher · 4dfbb9a

Review

Read docs/engine-plugins.md, engines/lemonade/src/backend_alignment.rs, and engines/lemonade/src/install.rs in full at the current head (merge-base fe1dbabe…head 4dfbb9a6), plus the unchanged helpers the new code calls (ensure_cached_archive, extract_archive, find_embeddable_root, set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside). This head is a force-pushed rebase that answers the two earlier review rounds on this PR (one automated, one human) at commit bd917f4d: the doc now matches the code on which error is reported after a failed retry, the restore takes a backup and puts it back when the retry fails, the pin file is written atomically, and two new tests tie each new warning message to the resulting pin-file state. Two points from those earlier rounds are still open at this head, detailed below.

Blocking

• The forced retry bypasses this file's own half-install safety net — engines/lemonade/src/backend_alignment.rs:280 vs. :427-435, :1075-1115
install_pinned_backend_or_restore_packaged_pins calls fallback_install(manifest, true) directly after a successful restore. Its own comment at line 278-279 names the hazard ("the failed attempt may have left a half-installed backend for the withdrawn pin"), but every other forced reinstall in this file — the revert fallback inside align_llamacpp_backend_to_version (lines 427-435) and try_llamacpp_backend_alignment (line 461) — wraps that same call in set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside (defined lines 1075-1115), whose doc comments explain why: each build extracts into its own build-numbered directory, so a failed attempt can leave a stray directory that the binary lookup later picks up instead of, or alongside, the intended build. The new retry path recognizes the same risk in prose but doesn't apply the existing mitigation.
  Confidence 80 · architectural · Fix: wrap the forced retry (and arguably the first attempt) with the same aside/resolve pair used elsewhere in this file.

• No Gherkin scenario for the user-observable behavior this change introduces — AGENTS.md §3, PR description "Not covered / gaps"
  The project's own contribution guide requires a Gherkin scenario under tests/e2e-cucumber/features/ for any change that alters CLI-observable output, exit codes, or files/paths the CLI creates — with only three outs: an existing scenario, a new scenario gated to @requires-gpu/@nightly and named in the PR text, or a change that's purely internal. This adds new stderr warnings and turns a previously-failing install into a succeeding one, so it isn't purely internal. The PR names the CI lane whose prewarm step exercises this path, and the author's follow-up comment explains why a scenario would need to fake runner state that can't easily be reproduced in a normal test run. That's a reasonable practical constraint, but naming an existing CI job is not the same as adding and tagging a scenario, so the project's own rule isn't satisfied on its own terms yet. Coverage today is unit tests over injected closures plus one archive round-trip test, which exercise the decision logic but not the CLI-observable outcome end to end.
  Confidence 75 · architectural · Fix: either add a gated scenario, or get explicit maintainer sign-off that the CI-lane argument is accepted as the exception here.

Non-blocking

• Archive-extraction/root-finding logic is duplicated — engines/lemonade/src/install.rs:135-188 (prepare_embeddable_with) and :275-311 (restore_packaged_backend_versions_with)
  Both independently extract the embeddable archive to a timestamped temp dir, call find_embeddable_root, then clean up — differing only in what they do with the result (copy the tree vs. read one file). Not a correctness problem, just a candidate for a shared helper.

• Some of the new warning strings and restore failure branches remain untested — engines/lemonade/src/backend_alignment.rs:301 and :290, engines/lemonade/src/install.rs:275-311
  The new tests assert the "restored the pins" and "put the previous" messages against real pin-file state, which is good, but the "could not restore the pins Lemonade ships" and "could not be put back" messages have no test referencing them, and restore_packaged_backend_versions_with's own failure branches (digest mismatch via ensure_cached_archive, a packaged archive missing the resource entry) are only indirectly covered through already-tested lower-level primitives.

• The new recovery paragraph isn't cross-referenced from the summary it extends — docs/engine-plugins.md:46 vs. :69-83
  Line 46 still reads "skipped when the pin already matches the active version" (still accurate for the alignment rewrite), but a reader stopping there won't know the install in that case now has a recovery path described 23 lines later, with no forward pointer.

Decisions for the author

• Full archive re-extraction just to read one JSON file — tradeoff
  restore_packaged_backend_versions_with extracts the whole embeddable archive to read a single entry out of it, reusing the same hardened extraction path prepare_embeddable_with uses rather than reading one archive entry directly. That trades some I/O on a failure-recovery path that should be rare for reuse of already-tested code. Worth confirming this was a deliberate choice.

Positive signals

• The restore takes a verified-JSON-equal check before deciding anything changed (install.rs:290-296), so a prior write that only re-serialized equivalent content doesn't trigger a spurious restore — handled carefully, with a test for it.
• The pin file write now goes through a temp-file-and-rename (write_file_atomic), and the backup/put-back path is tested against a real file on disk, including the "no file existed" case.
• Digest verification is enforced before any restored byte is trusted, reusing the existing ensure_cached_archive lock/verify path rather than re-implementing it.

Deployment notes

• At review time, all required checks on this head pass except E2E tests and windows-build-and-test, both still pending/running — these are exactly the lanes this change targets, so confirm they finish green before merge.
• This only changes behavior on the "pin already equals the active version but install fails" path; ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT continues to bypass it, unchanged. No migration or new configuration surface.

What this covered

Read in full at head 4dfbb9a6: docs/engine-plugins.md, engines/lemonade/src/backend_alignment.rs, engines/lemonade/src/install.rs, diffed against merge-base fe1dbabe, plus the unchanged call sites/helpers named above and the project's AGENTS.md. Did not run cargo test/clippy/fmt locally — no Rust toolchain available in this review environment; relied on GitHub's reported CI status instead, re-checked immediately before publishing. Checked git log on the touched files and found no other recent related work. This review did read the PR description and the two earlier review rounds on this PR (an automated review and a human review, both at the prior head bd917f4d) and the author's reply to them, specifically to establish which of their points this force-push already addresses versus what's still open — the two blocking items above are the ones that remain.

[rominf]

🔴 Automated review · pr-review-watcher · 4dfbb9a

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.
No ticket or issue named. The description matches the diff and justifies the missing Gherkin scenario under its AGENTS.md §3 gaps section.

All findings below are new (first automated round on this PR).

Blocking

• Two of the four new user-facing warnings are not tested, and one of them claims an outcome (new) — engines/lemonade/src/backend_alignment.rs (install_pinned_backend_or_restore_packaged_pins: the Err(restore_error) arm's "could not restore the pins Lemonade ships…", and the Err(put_back_error) arm's "…could not be put back: …")
  AGENTS.md §3 says a line the CLI prints about what it did must be asserted together with the resulting state. equal_pin_whose_restore_fails_reports_the_original_failure goes through prepare_with_equal_pin, which sends warn to eprintln!. So it checks the returned error, but neither the message nor the pin file.
  Two mutations ran on this head, and the lemonade suite stayed green on both (146/146):

  • replacing the restore-error warn(format!(…)) with drop(format!(…));
  • changing the put-back-error message text.

  The other two warnings are properly covered: removing put_back() fails a_failed_retry_puts_the_previous_pin_file_back_and_says_so.
  Confidence 100 · logic · Fix: drive both arms through install_pinned_backend_or_restore_packaged_pins with a captured warn. Assert the message, plus the pin file being unchanged (restore failed) or still holding the packaged bytes (put-back failed, e.g. the target made unwritable).

• The new forced retry skips the backend-directory set-aside that its twin, the revert path, uses for the same operation (new) — engines/lemonade/src/backend_alignment.rs (fallback_install(manifest, true) in install_pinned_backend_or_restore_packaged_pins); twin at align_llamacpp_backend_to_version ~L426-437; lookup in engines/lemonade/src/direct_llama.rs find_binary_in

  • What the twin does: after rewriting the pins, it wraps its forced reinstall in set_aside_rocm_llamacpp_backend_dirs / resolve_rocm_llamacpp_backend_dirs_aside.
  • Why, per the twin's own doc comment: installing a different therock.version/llama.cpp tag "can otherwise leave two builds side by side". find_binary_in takes the first read_dir subdirectory, so rocm serve "may resolve to whichever build" is found first. Also, "a failed fallback install must not leave the host with no ROCm backend at all".
  • Why the new path matches that case: it rewrites the pins and then force-installs a different build. The new comment ("the failed attempt may have left a half-installed backend") admits there may be leftovers. But forcing the install does not remove a leftover in another build-numbered directory.
  • What happens on failure: if the retry fails, nothing restores a backend that the forced install removed.

  Not run on hardware. Traced from the code.
  Confidence 70 · logic · Fix: wrap the retry the same way as the revert path. Pass true/false to resolve_rocm_llamacpp_backend_dirs_aside according to the retry result.

Non-blocking

• The new atomic-write helper is a third copy, and weaker than the existing one (new) — engines/lemonade/src/install.rs write_file_atomic; existing helpers at crates/rocm-core/src/lib.rs write_atomic and apps/rocmd/src/sandbox.rs write_file_atomically

  • The new helper always uses the fixed temp name <path>.rocm-tmp, and does not remove the temp file when fs::write fails.
  • rocm_core::write_atomic adds a millis+pid suffix. Its doc says this exists because two processes writing at the same moment would otherwise overwrite each other's temp file. The runtime tree here is documented as shared.
  • Lemonade already depends on rocm-core, but write_atomic is pub(crate) there.

  Confidence 85 · mechanical · Fix: export and reuse rocm_core::write_atomic, or at least copy its unique temp name and its cleanup when the write fails.

• A doc comment the change made stale (new) — engines/lemonade/src/backend_alignment.rs doc on prepare_llamacpp_backend_for_active_rocm (~L69-79)
  It still says that when "the pinned version already matches", this "falls back to … today's unmodified pinned-version install — never a regression". That branch can now rewrite backend_versions.json and force a second install. The code governs here, so the comment is the side to fix.
  Confidence 85 · mechanical · Fix: mention the restore-and-retry on that branch.

• The new warning wraps advice that contradicts it (new) — engines/lemonade/src/backend_alignment.rs ({install_error:#} in the "restored the pins…" warning); source of the advice at install_llamacpp_backend_with_retry ~L903-906
  install_error carries the context "run rocm engines install lemonade --reinstall and then retry rocm serve". The same line then says the CLI restored the pins and is retrying by itself.
  Confidence 70 · mechanical · Fix: print the root cause (install_error.root_cause(), or the chain without the outer context) in this warning.

• "Installed backends are untouched" reads as true of the whole recovery, but the retry is a forced reinstall (new) — docs/engine-plugins.md new paragraph; restore doc in engines/lemonade/src/install.rs (restore_packaged_backend_versions); the PR description's "Change" section
  The statement is true of the restore step alone. The next step, fallback_install(manifest, true), reinstalls the selected backend into the shared tree.
  Confidence 70 · mechanical · Fix: limit the claim to the restore step, and say that the retry forces a reinstall.

Decisions for the author

• Any install failure, including a transient one, can permanently replace a pin that was working — tradeoff
  • What happens: the inner install_llamacpp_backend_with_retry has already retried once. If the restored retry then succeeds, the kept aligned pin is gone. The next plain install sees pinned ≠ target and runs alignment again (forced multi-GB reinstalls).
  • Where the user hears about it: only the stderr warning. Nothing goes into InstallResponse.warnings.
  • The other side: a stale pin otherwise blocks every install until someone runs --reinstall. The docs do say that any failure starts the restore.
• The restore rewrites the whole file, so hand edits to keys alignment never touches are also dropped — tradeoff
  • Before this change: with therock.version equal to the active SDK, the file was never rewritten without --reinstall.
  • Now: any failure there can replace hand pins for other recipes.
  • The other side: ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT is checked before this branch (verified: the disabled early return comes first), and the docs point users who edit by hand at it. Restoring only the therock/llamacpp.rocm-stable keys would keep other edits, at the cost of a pin file that is not exactly what Lemonade ships.

Positive signals

• Recovery is limited to the equal-pin branch, and a no-op when the file already holds the packaged values (compared as JSON, not bytes). So a genuinely bad packaged pin still reports its original error and is not hidden behind a retry.
• The archive is read through ensure_cached_archive, so the bytes are digest-checked before they are trusted. A backup is put back when the retry fails, so a transient error cannot throw away a pin that was fine.

Verdicts for every test the change adds (mutations run on this head, lemonade lib suite, 146 tests):

Test	Fails with the fix hunk reverted?	Branch mutations it catches
equal_pin_kept_from_an_earlier_alignment_is_restored_when_it_no_longer_installs	yes	retry not forced
equal_pin_still_failing_after_the_restore_reports_the_retry_failure	yes	retry error replaced by the first error
equal_pin_that_is_already_the_packaged_pin_reports_the_original_failure	yes	Ok(None) arm turned into a retry
equal_pin_whose_restore_fails_reports_the_original_failure	no (pins pre-existing behaviour)	restore error returned instead of the original; not its warning (see Blocking)
equal_pin_that_installs_never_touches_the_pin_file	no (no-op path)	restore count asserted
the_restored_message_is_true_of_the_pin_file_when_the_retry_succeeds	depends on the new helper	retry not forced
a_failed_retry_puts_the_previous_pin_file_back_and_says_so	depends on the new helper	put-back removed; first error reported
restore_packaged_backend_versions_replaces_only_a_pin_file_that_drifted	depends on the new helper	JSON-equality skip removed; extract cleanup removed
a_backup_of_a_missing_pin_file_removes_the_file_again	depends on the new helper	removal skipped

Mutations that survived:

• the restore-error warning dropped (Blocking above);
• the put-back-error message changed (Blocking above);
• write_file_atomic made non-atomic — acceptable, as a crash in mid-write is not practical to test.

Deployment notes

• The lane the description names as exercising this, E2E tests (Strix Halo, Windows, current), does not appear in this head's check rollup. So no run on hardware has shown the fix on a runner holding the stale state. The description says the lane goes green only once the change is on the branch it runs.

What this covered

The full diff 24e405a7…4dfbb9a6 (one commit), plus the callers and twins it touches:

• the prepare_llamacpp_backend_for_active_rocm{,_impl} gate chain;
• the alignment revert and set-aside code;
• install_llamacpp_backend_with_retry and ensure_best_llamacpp_backend;
• ensure_cached_archive, prepare_embeddable_with and find_binary_in;
• the only production caller, in engines/lemonade/src/lib.rs.

What was run:

• cargo test -p rocm-engine-lemonade --lib: 146 tests passed, including all 9 new ones by name.
• 12 single-branch mutations, each rebuilt and run separately.

Fan-out ran with 2 workers: a code/design pass, and an agent-instruction/history/comments pass. The design questions (Step 7) were folded into the first worker because of the 2-worker cap, not run as separate workers.

Checks and sign-off:

• Sign-off: checked with check_signoff.py, which exited 0 (1 commit checked, none missing).
• CI: for this head, all reported checks succeeded. "Skill checks (skillscope)" and "Sphinx docs build (-W)" were SKIPPED. No GPU lane appears in the rollup.

Did not run:

• Prior-changes pass: earlier PR reviews were not consulted, by design.
• Workspace-wide gates: cargo test --workspace, clippy, the Windows cross-check and smoke_local.py were not run locally. CI's clippy and Windows build-and-test passed.

No prompt-injection content found. Not reconciled against any PR discussion here.

Why this requests changes: both blocking findings are defects with a consequence once merged — an outcome-claiming warning whose removal no test notices (the repo's AGENTS.md §3 rule), and a forced reinstall that can leave two backend builds side by side or no ROCm backend after a failed retry, unlike its twin. Resolving them: add the two captured-warn tests, and wrap the retry in the same set-aside/resolve as the revert path (or say why it is not needed here). This automation will withdraw the request once a later round confirms both.

[volen-silo]

Thanks for the thorough review. Both blocking findings were right. The branch is rebased onto current main and force-pushed (d9796e48). Point by point:

Blocking 1: two warnings were untested. Added tests that drive both arms through install_pinned_backend_or_restore_packaged_pins with a captured warn. A failed restore asserts the message and that the pin file and the backend directory are untouched. A failed put-back asserts the message and that the restored pins are still what is on disk (the backup points at a directory that does not exist, so the write fails). I reran your mutations on production code only: changing the restore warning's text, and the put-back warning's text, each fail a test now.

Blocking 2: the forced retry skipped the set-aside its twin uses. Agreed. The retry now sets the ROCm backend directories aside before the forced install, discards them on success, and puts them back on failure, using the same two functions as the revert path. Tests: a success leaves only the new build (no two builds side by side); a failed retry puts the old build back, removes a stray directory the failed install left under another ROCm name, and puts the previous pin file back. Removing the set-aside fails both.

Non-blocking.

• Third atomic-write copy. Removed. The restore and the put-back use rocm_core::write_atomic, which is now pub (the only API change outside lemonade). It has the unique temp name and the cleanup. The existing test checks that no temp file is left.
• Stale doc comment. The doc on prepare_llamacpp_backend_for_active_rocm now says that the matching-pin branch can rewrite backend_versions.json and force a second install.
• Warning contradicting the advice. The warning now prints the root cause. A test gives the first error the --reinstall advice as context and checks the warning has the cause and not the advice.
• "Installed backends are untouched". Limited to what is true. The docs, the commit message and the PR body now say the retry is a forced reinstall of the selected backend, with the set-aside above.

Decisions.

• A transient failure can replace a working pin. I kept this behaviour and wrote the tradeoff into the PR text. The alternative is a stale pin blocking every install until someone runs --reinstall. The put-back covers a retry that fails. When the retry succeeds, the kept aligned pin is gone and the next plain install aligns again. The message goes to stderr like the other warnings in that function; routing it into the install response would change that function's return type, so I left it.
• The restore rewrites the whole file. Changed. Only the two entries alignment writes (therock.version, llamacpp.rocm-stable) are restored, and all other entries are kept. A file whose two pins already match is not rewritten, even if other entries differ. Tests cover a hand-edited entry surviving, the no-op, and a file that is not JSON.

On the lane: E2E tests (Strix Halo, Windows, current) is the one that exercises this, and the PR text says it only does so on a runner that still holds the stale state. I did not re-run the Windows reproduction after these changes, and the PR text says so.

Addressed in d9796e4; point-by-point reply: #754 (comment)

[juhovainio]

I went through this at d9796e4. The fix itself matches the PR description, and the tests assert each warning together with the real pin-file and backend-directory state, which I appreciate. I'm requesting changes for the items below; the first three are inline.

1. The backend directories are set aside only after the first install attempt has already failed. A partial rocm-* directory left by that attempt is what would be "put back" if the retry also fails. Setting aside before attempt one, or a test where attempt one leaves a partial directory, would close this.
2. Any install failure now triggers the restore path, which reads or re-downloads the embeddable archive even when the pins are already the packaged ones. On the common failure (for example offline) that adds a download attempt and a second warning before the original error. A cheap short-circuit would avoid it.
3. In install.rs, the current pin file is read before the archive is verified and extracted, so a concurrent writer in between would be overwritten and the backup would hold stale bytes.
4. The retry is forced on any first failure, including transient ones, and moves the working ROCm backend aside. A kill between set-aside and put-back leaves only a .pre-alignment directory, which is now reachable from a plain install.
5. Docs: only docs/engine-plugins.md was updated, but rocm engines install lemonade now prints a new warning and rewrites pins. Please grep README.md, docs/manual-testing.md and docs/testing.md for the repeated "alignment" claim and update them in this change.
6. The Strix Halo Windows lane this fix targets is not in the check list for this head, and the Windows repro was done on an earlier version of the change. Please re-run it on the current head, or say which lane will exercise it.

Not asking for a change: hand-edited pins being lost on a successful retry is disclosed in the PR and the env var is the escape hatch.

[juhovainio]

The backend dirs are set aside only after the first install already failed, so a half-installed rocm-* dir left by that attempt is what gets put back if the retry fails too. Consider capturing before the first attempt, or add a test where attempt 1 leaves a partial dir.

[juhovainio]

Any failure (including a plain network error) now triggers restore, which reads or downloads the embeddable archive even when the pins are already packaged. Consider short-circuiting before the archive work when the pins already match, or limiting this to failures that look like a withdrawn asset.

[juhovainio]

current is read before the archive is verified and extracted. If alignment in another process writes between this read and the write_atomic below, its write is overwritten and the backup holds stale bytes.

[nowycondro]

🔴 Automated review · pr-review-watcher · d9796e4

Review

Restores the two backend-alignment pins (therock.version, llamacpp.rocm-stable) in resources/backend_versions.json from the packaged embeddable archive and retries once, forced, when the already-pinned Lemonade backend fails to install — recovering from a stale pin an earlier alignment kept that now names a withdrawn build. The core restore/retry logic, its error-reporting branches, and the backend-directory aside/resolve wrapping were traced through every branch and are correct and well tested against real files and directories. One real documentation-accuracy defect blocks; one narrow data-loss edge case and the stated AGENTS.md scenario gap are not.

Blocking

• write_atomic's visibility bump contradicts docs/architecture.md, which still calls it crate-private and slated for deletion — crates/rocm-core/src/lib.rs:362, docs/architecture.md (the write_atomic bullet, ~line 110), new consumer at engines/lemonade/src/install.rs:332 and :407.
  docs/architecture.md reads, unchanged by this PR: "write_atomic — not permanent. It is the crate-private temp-then-rename writer config.rs, automation.rs, and service_record.rs use for their atomic replaces. ... Once #610 lands, the automation and service-record callers follow and write_atomic is deleted (ROCMAI-724)." This PR changes the function from pub(crate) to pub and gives it a brand-new, permanent cross-crate consumer (rocm_core::write_atomic from engines/lemonade), which the doc's deletion plan never accounted for. The doc is now wrong on two counts: the function is no longer crate-private, and deleting it per ROCMAI-724 would break this new caller. AGENTS.md §5 requires keeping docs and behavior claims in sync while editing.
  Confidence 85 · architectural · Fix: update the write_atomic bullet in docs/architecture.md to drop "crate-private", name the lemonade consumer, and note how this affects (or supersedes) the #610/ROCMAI-724 deletion plan.

Non-blocking

• A process kill during the forced retry window can permanently lose a hand-edited pin file — engines/lemonade/src/backend_alignment.rs:282-314, engines/lemonade/src/install.rs:398-411 (PinFileBackup::put_back).
  restore_packaged_pins atomically overwrites backend_versions.json with the packaged values and hands back a backup whose previous bytes live only in an in-process Vec<u8>. The code then runs a full forced reinstall (spawns lemond, waits up to 30s, network-bound backend install) before put_back() ever runs, and only on the Err arm. A SIGKILL, OOM-kill, or power loss during that window leaves the packaged values on disk with no way to recover whatever was there before — a window that did not exist before this change, since the equal-pin branch previously never touched the file. Narrow (needs an actual process kill, not an ordinary failure) and partly mitigated by the PR's own stated design: these two keys are already meant to be reset if a user's hand-edit of them causes an install failure, crash or not.
  Confidence 70 · architectural · Fix: either persist the backup to a sibling file before the retry (cleaned up after put_back), or note the accepted risk explicitly in the function's doc comment.

Decisions for the author

• No Gherkin scenario covers the new restore-and-retry path — answered-by-intent. engines/lemonade/src/backend_alignment.rs:280-311 adds a new stderr warning, rewrites backend_versions.json, and can change which llama.cpp build gets installed — all three AGENTS.md §3 triggers for "needs a scenario, not only a unit test," and no feature file in tests/e2e-cucumber/features/ exercises this path (only the pre-existing Tier1/Tier2/revert alignment is covered there). The PR description's "Not covered / gaps (AGENTS.md section 3)" section answers this directly: "The behaviour needs a runtime tree that already holds a stale pin and a live backend download, so it could only be a gated scenario. The lane that exercises it is E2E tests (Strix Halo, Windows, current) ... That lane did not appear in this head's check list at the last look, so no hardware run has shown the fix on a runner holding the stale state." AGENTS.md §3 explicitly permits exactly this: state the gap and name the gated lane. Would otherwise have been blocking. Confirm that lane is still the intended coverage path once this lands, and that someone will check it actually goes green there (it isn't in this PR's own check list).
• The restore-and-retry recovery only triggers from the pinned_version == target_version branch — tradeoff. Other fallback_install(manifest, false) call sites in the same function (no active managed SDK version, an unreadable pin, a selected_backend_lookup error) don't get the same recovery, so an identically stale pin reached via one of those rarer paths stays unrecovered. Plausible but speculative — confirm this narrower scope (the one failure mode actually seen in CI) was the deliberate choice rather than an oversight.

Positive signals

• The restore/retry control flow (install_pinned_backend_or_restore_packaged_pins) was traced through every branch — install succeeds; fails then restore+retry succeeds; fails then restore succeeds but retry fails (put-back succeeds or itself fails); restore is a no-op because the pins already match packaged; restore itself errors — and each is correct, with the ROCm backend-directory aside/resolve pairing (set_aside_rocm_llamacpp_backend_dirs / resolve_rocm_llamacpp_backend_dirs_aside) never leaked or double-resolved in any outcome.
• Tests exercise realistic failure scenarios against real pin files and real backend directories (not just mocked closures), including a simulated stray directory from a failed install and the exact wording/root-cause-vs-chain distinction in the warning messages, each asserted together with the resulting on-disk state per AGENTS.md §3's message-assertion rule.
• docs/engine-plugins.md's new paragraph was checked clause-by-clause against the code and is accurate on every point, including the subtle ones (original error reported unchanged when there's nothing to restore, the warning names only the root cause and not the chain's --reinstall advice, the retry is forced, aside/resolve pairing correct in every branch).

Deployment notes

None — no new dependency, no migration, no config change. write_atomic's visibility change is additive (widens crate-private to crate-public) and doesn't alter its existing callers' behavior.

What this covered

Full review of the whole change: engines/lemonade/src/install.rs, engines/lemonade/src/backend_alignment.rs, crates/rocm-core/src/lib.rs, and docs/engine-plugins.md, each read in full (not just the diff hunks), plus AGENTS.md, docs/architecture.md, relevant git log/git blame on the touched regions, and prior commits touching the backend-directory aside/resolve and archive-cache-locking helpers these functions build on. Commit range: merge-base of origin/main and pr-754 (24e405a7) through d9796e48, the single commit on this PR. CI re-checked live via gh pr checks 754: all checks pass or are intentionally skipped; no E2E Strix Halo (gated, hardware) run appears in this head's check list, consistent with the PR's own description. Did not run cargo test/cargo clippy locally — relied on tracing the code plus the PR's own stated local verification and CI's green result. Not reconciled against this PR's existing review threads or its prior nowycondro review (at an older commit) — this is a fresh, blind pass against the current head.

[volen-silo]

Thanks, these were good catches. The branch is rebased onto current main and amended into the same single commit (620a872e; local until pushed). Point by point, including where I did something other than what you suggested:

1. Partial directory from the first attempt. Real, fixed, but not by setting the directories aside before the first attempt: that would move a working backend on every plain install, including the ones that succeed. Instead the ROCm backend entries (the rocm-stable / rocm-nightly / rocm directories and what sits directly inside them) are listed before the first attempt, and after a failed retry, once the directories are put back, anything not in that list is removed. New test: the first attempt leaves a partial build next to the existing one and a whole new directory, the retry fails; the old build is back and both leftovers are gone. I changed the code (removed the prune) and the test fails. One limit: a first attempt that modifies a file inside an existing entry in place is not detected.

2. Restore re-downloading the archive. Done, and stricter than a short-circuit: the restore is now cache-only. It never downloads. If the archive is not cached or its digest does not verify, the restore is skipped with a warning and the original install error is reported. I did not short-circuit "before the archive work when the pins already match", because the packaged values come from the archive; reading the cached archive is a local extraction, so on the common offline failure the cost is now a local read, not a download. docs/engine-plugins.md no longer says it downloads again. Assumption, stated plainly: this still heals the stale-slot case because every rocm engines install lemonade verifies the archive into downloads/ before the backend step, so the cache is there when the backend install fails. It would not heal if something removes the archive between those two steps. Tests: no cache and a cache that fails its digest each report the not-downloading error, leave the pin file alone and create no download or scratch directory. Since the function no longer takes a download step at all, I could not inject a panicking download closure as suggested; there is nothing to inject. I mutation-checked the missing-cache test instead.

3. Pin file read before the archive work. Fixed: the file is read after the archive work, immediately before computing and writing, and that read is what the backup holds. Test: a writer changes the pin file between the archive work and the read (through a test-only hook that runs at that point); the backup holds the writer's bytes, the writer's other entries survive, and put-back restores them. Moving the read back to the top makes it fail.

4. Kill between set-aside and put-back. I did not implement orphan recovery. The runtime tree is shared between processes, so a leftover .pre-alignment directory cannot be told from another process's install in progress without a lock, and guessing wrong would delete a live install. Instead it is documented as a known limitation, shared with alignment's own revert path, in docs/engine-plugins.md, the function's doc comment and the PR text, with --reinstall as the way to rebuild the tree. I also documented that the previous pin file is held in memory only, so a kill during the retry leaves the packaged pins on disk.

5. Docs. I grepped README.md, docs/*.md, tests/e2e-cucumber/README.md and skills/ for alignment, backend_versions, pinned and reinstall claims. Nothing else claimed that a plain install never rewrites the pins or leaves backends untouched (the alignment text in docs/engine-plugins.md was the only one, and the Gherkin comments for the alignment scenarios stay accurate). Because the observable behaviour changed, I added a short note to README.md, focused test commands to docs/testing.md, and a manual check to docs/manual-testing.md. I removed a manual step I had drafted about deleting the cached archive, because every install refills the cache first, so that state cannot be reached by hand.

6. Windows lane and reproduction. I re-ran the reproduction on this Windows Strix Halo host with the final code, with an origin/main binary and a binary of this branch in separate worktrees and isolated config/data/cache directories. Clean install, then pins patched to 7.14.1 / b11783 plus an unrelated key, backend removed, install again: the base binary fails with HTTP 403 for therock-dist-windows-gfx1151-7.14.1.tar.gz and exits 1; this branch prints the warning, restores 7.13.0 / b9752, installs the 7.13.0 backend, exits 0, and the unrelated key survives. Caveats: debug builds, and the active 7.14.1 SDK is a hand-written registry record, not a real SDK install. The E2E tests (Strix Halo, Windows, current) lane is still not in this head's check list, so CI has not shown it; the PR text says so and names that lane as the one that exercises it.

The second automated review on the same head, for completeness:

• docs/architecture.md still called write_atomic crate-private: valid, fixed. It now says the function is pub because the Lemonade engine uses it, and that the planned deletion has to move that caller too.
• Pin file lost if the process is killed during the retry: accepted and documented rather than persisted. A backup file in the shared tree would bring back the orphan problem from point 4, and the two keys are Lemonade's own defaults that the next install re-aligns.
• No Gherkin scenario: unchanged answer; it needs a stale pin plus a live download, so only the gated Strix Halo Windows lane can exercise it, and it is named in the PR text.
• Restore only in the equal-pin branch: deliberate, since that is the one failure seen in CI; the other install sites do not restore, and the docs now say so.

Fixed 620a872

[nowycondro]

🔴 Automated review · pr-review-watcher · 620a872

Review — needs work

Full review of the whole change (single commit 620a872e, base ea86403c). No ticket named for this change; the PR description's own account of scope matches what this review found — no overclaim or undisclosed work detected.

Blocking

• PinFileBackup's backup conflates "pin file absent" with "pin file present but unreadable," so a failed recovery can delete a real file instead of restoring it — engines/lemonade/src/install.rs:324 (let current = fs::read(&target).ok();), :346 (None => packaged), :352 (previous: current), :406-409 (doc comment: `None` when the file did not exist, so putting it back removes it), :421-432 (PinFileBackup::put_back).
  fs::read(&target).ok() discards the specific error kind, so any read failure other than "file doesn't exist" (e.g. a Windows sharing violation from a concurrent writer — a scenario this very function's own neighboring doc comment says it's designed around, and which read_manifest/alignment's own revert path treat as a real possibility) is treated identically to "no file was ever there." The PinFileBackup built from that read then carries previous: None, directly contradicting its own doc comment's contract ( `None` when the file did not exist). If the subsequent forced retry then also fails, put_back() takes the None branch and calls fs::remove_file, deleting resources/backend_versions.json outright, rather than restoring the real (merely unread) content that was there. No test exercises this through the real restore_packaged_backend_versions_with function — the only "missing file" test constructs PinFileBackup::new(.., None) directly, bypassing this code path entirely.
  Confidence 85 · architectural · Fix: match on io::ErrorKind — treat NotFound as "no previous file", and propagate (bail!/?) any other read error instead of silently folding it into "nothing to restore."

• Two self-hosted E2E hardware checks are failing on this head — CI: E2E tests (Strix Halo, Ubuntu, release, current) and E2E tests (Strix Halo, Ubuntu, nightly, current) both report FAILURE as of this review (statusCheckRollup for 620a872e). The directly-relevant lane for this fix, E2E tests (Strix Halo, Windows, current), passed. Job logs were not retrievable from this review environment (403 on the Actions API), so whether the two Ubuntu failures are a regression from this change or pre-existing/runner flakiness (the PR's own description documents this general class of self-hosted-lane flakiness) could not be determined here.
  Confidence 100 (directly read from live CI status) · correctness/CI · Fix: inspect the two failing jobs' logs before merge and confirm they're unrelated to this change (or fix/rerun if not).

Non-blocking

• Pre-existing, pattern-relevant: three sibling pin-writer functions in the same file still use non-atomic fs::write for the same backend_versions.json this change newly treats atomically — engines/lemonade/src/backend_alignment.rs:614 (write_backend_versions_llamacpp_tag), :631 (remove_backend_versions_llamacpp_tag), :686 (write_backend_versions_therock_version). Not introduced by this diff, and not wrong on its own, but this change's own framing emphasizes "writes are atomic" for this exact file and makes rocm_core::write_atomic pub specifically to reach it from here — the three older writers to the same file are the obvious next candidates and are left as-is.
  Confidence 85 · mechanical · Fix: consider migrating these three to write_atomic in a follow-up, or note why they're excluded.

Decisions for the author

None

Positive signals

• restore_reads_the_pin_file_after_the_archive_work_not_before in install.rs is a genuinely rigorous test: it simulates a concurrent writer mutating the pin file mid-restore and asserts the backup holds exactly what was on disk at write time, not a stale earlier read — a real race condition, actually exercised.
• The new forced-retry path reuses the existing set_aside_rocm_llamacpp_backend_dirs/resolve_rocm_llamacpp_backend_dirs_aside machinery rather than reimplementing directory set-aside/restore, keeping the new recovery consistent with alignment's existing revert semantics instead of introducing a second, subtly different implementation.
• The warning text surfaces install_error.root_cause() rather than the full context chain, specifically avoiding repeating the confusing --reinstall advice the CLI is already acting on — verified by a dedicated test (the_restored_warning_names_the_root_cause_not_the_reinstall_advice).

Deployment notes

• Behavior change on the happy-path-adjacent failure case: a rocm engines install lemonade that previously failed loudly and left backend_versions.json/the installed backend untouched can now, on a kept-but-stale pin, print a warning, rewrite the two alignment-owned pins, and replace the selected backend — exit 0 instead of exit 1. Operators or scripts depending on the old "hard fail, no mutation" behavior should account for this.
• ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT disables this new recovery path along with alignment itself (verified in code: the disabled branch returns before the new logic is ever reached).
• See the two failing self-hosted E2E checks under Blocking before merging.

What this covered

Every file in the change read completely at 620a872e against base ea86403c (engines/lemonade/src/backend_alignment.rs, engines/lemonade/src/install.rs, crates/rocm-core/src/lib.rs, README.md, docs/architecture.md, docs/engine-plugins.md, docs/manual-testing.md, docs/testing.md), plus the pre-existing sibling helpers each new function calls (directory set-aside/restore, the pin read/write helpers, the cached-archive digest/lock machinery, write_atomic and its other callers) and cross-file consistency checks (pin-key literals, public-item usage, crate-edge/dependency direction). Run as two independent fan-out workers (one per primary Rust file, each also covering the docs most tied to its file) plus direct verification of both files and all five doc files by the driver; candidates were consolidated and re-verified against source before scoring, and several sub-70-confidence candidates from the workers (a bare-backtick doc citation matching existing file convention; a shared-constant suggestion for pin-key names; a speculative one-level-deep pruning gap; an already-self-documented warning-noise tradeoff; an already-self-healing kill-window overlap) were checked and dropped as not meeting the keep bar.

Did not run: cargo test/cargo clippy directly (read-only clone, PR head never checked out per method; relied on the PR's own CI instead). Prior-changes pass did not run — this review has no access to this PR's own discussion thread or any earlier review round by design, so could not check whether earlier feedback was addressed. The PR description's mutation-testing claims (reverting specific lines makes the new tests fail) were not independently reproduced. Not reconciled against any discussion on this PR.

[juhovainio]

No blocking issues from my read of the diff. The fix matches the stated root cause (a kept pin equal to the active ROCm version means alignment never reruns), and the tests assert each message together with the state it describes. A few non-blocking notes, with the first three also inline:

1. fs::read(&target).ok() treats any read error as a missing file. Only NotFound should.
2. The scratch extract dir name is millisecond-only; a pid suffix would avoid a collision.
3. Question on Windows: what happens when the set-aside rename fails because lemond holds the backend dir?
4. The PR is large for one recovery branch. A lighter option would be to restore the two pins and call the existing forced reinstall, without the prune step. Not a request, just something to weigh.
5. CI: Strix Halo Ubuntu release and nightly are red at bench-06 "a model is being served on GPU" (device state not drained after 120s), although the backend install itself succeeded. It looks like runner contention rather than this diff, but a rerun to confirm would be good. MI300X and W7900 had not reported when I looked.

This reflects a read of the diff only, not a local build or run.

[juhovainio]

fs::read(&target).ok() collapses any read error (permission, Windows sharing violation) into "file missing". In that case the code writes the whole packaged file with a None backup, and a later put-back deletes it. Consider treating only NotFound as missing and returning an error for other kinds, so the caller reports the original install error and the pin file is left alone.

[juhovainio]

Nit: the scratch dir is keyed by milliseconds only, so two processes sharing the runtime tree could pick the same directory. Unlikely, but a pid suffix is free.

[juhovainio]

Question: on Windows, if a running lemond or llama-server holds one of these backend dirs open, does set_aside_rocm_llamacpp_backend_dirs fail the rename, and if so what does the forced reinstall do next? The new tests don't seem to cover that case.

[rominf]

🔴 Automated review · pr-review-watcher · d9796e4

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.

Superseded — informational only. This report describes commit d9796e48. The PR head moved to 620a872e (a force-push with a different tree) while the review was running. It files no change request and does not gate. Its blocking findings are not gating between this comment and the next round, which will review 620a872e and settle them. Findings are not attached inline because they are anchored to a diff the PR has moved past. Line numbers refer to d9796e48.

Standing objections from the change request at 4dfbb9a6 (since dismissed by the author). This review did not re-find either of them blind at d9796e48, and its mutation checks hit both directly. Suppressing the restore-error warning, dropping the put-back, and removing the set-aside around the forced retry each now fail a test. Neither objection was put to the reviewer separately in this round. No change request from this automation is standing on this PR.

Review — needs work

Full review of the whole change at d9796e4. The PR head has since moved to 620a872, so this review is of an older commit.
No ticket named. The PR description was written for the newer head 620a872 and does not describe d9796e4 (see What this covered).

Blocking

• Two branches of the pin-file restore can break without any test failing — engines/lemonade/src/install.rs:345-347 (alignment_pins), engines/lemonade/src/install.rs:351-380 (set_or_remove_pin)
  I ran two mutations, and the 14 restore tests stayed green under each (exit 0):

  • The None arm of set_or_remove_pin made a no-op. This arm runs when the packaged file has no llamacpp.rocm-stable key. The existing revert code handles that case explicitly (remove_backend_versions_llamacpp_tag), so it is a real shape.
  • alignment_pins changed to ignore the llamacpp.rocm-stable key. The restore would then call a pin file "already packaged" when only the llama.cpp tag has drifted. That state is reachable: Tier 1 can keep a Tier-2 tag after the SDK is switched back to the packaged ROCm version.

  In every test fixture, both keys are present and both drift together. The non-object normalisation arms (!value.is_object(), a section that is not an object) are also never reached.
  Confidence 100 · logic · Fix: add restore cases for (a) a packaged file without rocm-stable against a current file that has it, asserting the key is gone afterwards; (b) only rocm-stable drifted, asserting the restore happens.

• equal_pin_whose_restore_fails_reports_the_original_failure passes with the production change reverted — engines/lemonade/src/backend_alignment.rs (test added at about line 1750)
  I put the old fallback_install(manifest, false)?; return Ok(None); back in the pinned_version == target_version branch:

  • this test and equal_pin_that_installs_never_touches_the_pin_file still pass;
  • the other three equal_pin_* tests fail.

  The second one is a legitimate guard. This one discards _restores, so it cannot tell that a restore was attempted, and its name claims a path it cannot fail on.
  Confidence 100 · logic · Fix: assert restores == 1.

• docs/architecture.md still calls write_atomic crate-private and slated for deletion, but this change makes it pub and adds the first caller outside the crate — crates/rocm-core/src/lib.rs:362, docs/architecture.md:110, engines/lemonade/src/install.rs:332,407
docs/architecture.md:110: "It is the crate-private temp-then-rename writer config.rs, automation.rs, and service_record.rs use … write_atomic is deleted (ROCMAI-724)."

  • The code is what governs: the function is now pub and lemonade calls it.
  • The doc is now false, and the planned deletion gains a caller in another crate that has to be migrated first.
  • AGENTS.md §5 requires docs to be kept in sync in the same change.

  Confidence 85 · mechanical · Fix: either update the architecture entry (visibility, the lemonade caller, what the ROCMAI-724 migration must cover), or keep write_atomic crate-private and use tempfile persist inside lemonade, which already depends on it.

Non-blocking

• The alignment revert path can still get stuck on the same stale pin — engines/lemonade/src/backend_alignment.rs:418-454 vs :199-211
  When pinned != target (for example the SDK was upgraded after a pin was kept) and both tiers fail, the revert writes pinned_version/pinned_tag back. Those are the stale kept pins, so its forced fallback_install fails the same way, and nothing restores the packaged pins. The branches with no SDK, a nightly SDK, an unreadable pin, a probe error, or a vulkan backend also install the kept pin with no recovery.
  The two paths that do the same job also follow different policies:

  • Pin writes: the revert writes non-atomically through fs::write; the new path uses write_atomic.
  • Failed fallback: the revert leaves the reverted pins on disk; the new path puts the previous pins back.

  Confidence 75 · logic · Fix: send the revert's fallback through install_pinned_backend_or_restore_packaged_pins, or revert to the packaged values. At minimum, add the revert path to the documented scope limits.

• A warning names alignment during the recovery path — engines/lemonade/src/backend_alignment.rs:1112
set_aside_rocm_llamacpp_backend_dirs prints "could not move … aside before backend alignment" when the recovery retry calls it, and no alignment is running then. When that rename fails (for example Windows DLLs in use), the doc comment's claim that a successful retry "does not leave two builds side by side" is also untrue.
  Confidence 70 · mechanical · Fix: pass a context label into the warning, and make the doc claim conditional on the set-aside succeeding.

Decisions for the author

• Any failure of the first install starts a restore and a forced reinstall, even when the pin cannot be the cause — tradeoff
  • When the selected backend is already installed, ensure_best_llamacpp_backend prints "Using installed…" and downloads nothing. A failure on that path comes from starting lemond or listing backends, never from the pin.
  • Even so, the recovery restores the packaged pins and sets the verified aligned backend aside. If the retry succeeds, it deletes that backend and replaces it with the packaged build, and the next install pays for alignment again.
  • The other side: limiting the trigger needs a way to tell install-step errors apart (a typed error or marker from install_llamacpp_backend_with_retry). The broad trigger guarantees that a stale pin never blocks an install.
  • One narrow option: skip the restore when the backend was already installed.
• The whole embeddable archive is extracted to read one small JSON file, while the archive lock is held — non-blocking-improvement
  • Cost: disk churn on a shared tree, and other installs of the same archive are blocked for the duration.
  • Alternative: stream only embeddable/resources/backend_versions.json out of the tar or zip, reusing the existing entry validation.

Positive signals

• The recovery's user messages go through an injected warn, and each test asserts the message together with the resulting pin-file and backend-directory state (for example a_failed_put_back_says_so_and_leaves_the_restored_pins_in_place). That is the AGENTS.md §3 "message and behaviour together" rule followed exactly.
• The restore reads the archive through ensure_cached_archive, so the bytes are digest-verified and held under the same cross-process lock before they are trusted.
• Pins are compared as JSON and only the two keys alignment writes are compared. A re-serialised but identical file is not treated as drift, and unrelated hand edits survive.

Deployment notes

• A plain rocm engines install lemonade can now rewrite two entries of resources/backend_versions.json and force-reinstall the ROCm backend in the shared runtime tree after a failed install. ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT skips this; I traced the early return at backend_alignment.rs:160-171.
• rocm_core::write_atomic is now public API of rocm-core.

What this covered

• Scope read: every file in prw-base…d9796e48 (ea86403…d9796e48), read completely, plus the surrounding alignment state machine, the set-aside/resolve helpers, install_best_llamacpp_backend/ensure_best_llamacpp_backend, ensure_cached_archive and its locks, prepare_embeddable_with, and the caller in lib.rs. Fan-out: two independent read-only workers (alignment flow + twin path; restore + docs + history + AGENTS adherence), consolidated and re-verified here.

• Tests run: cargo test -p rocm-engine-lemonade --lib on the checkout's own build passed 154 tests, all 21 new ones included.

• Mutation checks run: put-back dropped, set-aside removed, retry error swapped for the original, restore-error warning suppressed (each caught by a test); the set_or_remove_pin removal arm, single-key alignment_pins, and the semantic revert of the wiring (these three survived; the results are above).

• Sign-off: the sign-off check covered 1 commit and found none missing.

• CI: hosted CI for d9796e4 is green; no GPU lane reported a result for it.

• Head moved: the PR head has moved to 620a872. The description and the current check rollup describe that head, not d9796e4. Several claims in the description do not hold at d9796e4:

  • "the restore never downloads" (here it downloads the archive again);
  • the README/testing/manual-testing notes and the architecture.md update (absent here);
  • the post-retry prune;
  • the pin file read after the archive work (here it is read before).

  I did not file these as findings against the author, because they are probably true of the newer head. The Step 9 comparison is therefore not meaningful for this commit, and 620a872 needs its own review. Some findings above (the architecture.md one in particular) may already be addressed there; nothing here establishes that.

• AGENTS.md §3 scenario: the description argues why there is no Gherkin scenario (it needs a stale shared tree plus a live download, and the lane that would exercise it is named). I accepted that justification and filed no finding.

• Did not run: the prior-changes pass, because earlier review comments are out of bounds for this run; the e2e harness and workspace-wide gates.

• Discussion: not reconciled against any PR discussion here.

[volen-silo]

Thanks. Both blocking points were checked against the code; the fix is in 4a45d1b9 (still one commit).

Unreadable pin file read as absent. Confirmed. fs::read(..).ok() folded every read error into "no file", so a failed retry could have deleted the real backend_versions.json. Now only NotFound means "no previous file"; any other read error makes the restore fail before it writes anything, the original install error is reported (same path as the missing-cache and digest-mismatch skips), and the file is left as it is. New test restore_does_not_treat_an_unreadable_pin_file_as_absent goes through the real restore_packaged_backend_versions_with (a directory sits where the file should be) and asserts the error, that the path is untouched and that no temp file remains. It fails when the guard is changed back to Err(_) => None.

Ubuntu Strix Halo lanes. Not caused by this change. Both lanes report 8 failed scenarios: 7 are expected failures with rows in expectations.toml (bench-06, chat-06, chat-07, runtime-02, serve-01, serve-02, serve-07, all Lemonade serving) and 1 unexpected one, dash-37. main at d59f596f fails its own Ubuntu release lane with the same 8 scenarios and the same "7 xfail, 1 unexpected" line, so the lane is red without this branch. This change does not touch serving or the dashboard. As you noted, the Windows lane, which this change targets, passed on 620a872e.

I also took the older review's two test gaps, which were still true: restore tests for a packaged file without llamacpp.rocm-stable (the key must be removed) and for drift in only that key (the restore must happen), and restores == 1 in the equal-pin restore-failure test. Each fails under the matching change to production code.

Not changed: the three older fs::write pin writers in backend_alignment.rs stay as they are; moving them to write_atomic is a reasonable follow-up, but they are outside this change and I did not check how their existing tests expect write failures to behave.

Not run: scripts/smoke_local.py and the e2e lanes locally. Two symlink tests in the e2e crate fail on this machine with os error 1314 (the symlink privilege is not held); they are untouched by this change.

[rominf]

🔴 Automated review · pr-review-watcher · 620a872

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.

Superseded — informational only. This report describes commit 620a872e. The PR head moved to 4a45d1b9 (a different tree) while the review was running, so it files no change request and does not gate. Its blocking findings are not gating between this comment and the next round, which will review 4a45d1b9 and settle them. Findings are not attached inline because they are anchored to a diff the PR has moved past. Line numbers refer to 620a872e.

Standing objections from the previous round (posted at d9796e48). Each is marked below:

• Untested pin-restore branches — the None arm of set_or_remove_pin was re-found blind at 620a872e (Blocking 3, marked standing). The other half of that objection (alignment_pins ignoring a drift of llamacpp.rocm-stable alone) was not re-found and was not put to the reviewer: unverified this round, appended below.
• equal_pin_whose_restore_fails_reports_the_original_failure passes with the fix reverted — not re-found blind; settled by putting it to the reviewer after its report was frozen. Verdict (anchored, since the reviewer was told where to look): still holds at 620a872e. The test asserts the 403 text, the absence of "digest mismatch" and attempts == [false], all of which still hold with the old fallback_install(manifest, false)?; return Ok(None); restored, and it discards the restore count. Appended below as standing.
• docs/architecture.md calls write_atomic crate-private — not re-found blind; put to the reviewer, verdict (anchored) does not hold: the entry now says it is pub because lemonade writes backend_versions.json through it, and that it is deleted only after that caller migrates. Confirmed against the checkout. Dropped.

Per the PR description, 4a45d1b9 adds the remove-a-pin test, the restores == 1 assertion and an error for an unreadable pin file. None of that was read here.

Review — needs work

Full review of the whole change at 620a872. That commit is no longer the PR head: the PR now points at 4a45d1b, which this review did not read.
No ticket named. The description reports a reproduction on 620a872 and describes a later amend (it says that amend adds a few things, listed under What this covered).

Blocking

• [new] The manual-test recipe cannot reach the restore path — docs/manual-testing.md:296-308
  The restore runs only on the branch where the pinned version equals the active ROCm version (engines/lemonade/src/backend_alignment.rs:199). With no active managed SDK, fallback_install runs without a restore and the install simply fails. With an active SDK whose version is not 7.14.1, alignment's Tier 1/Tier 2/revert path runs instead. In both cases the documented result (the "restored the pins Lemonade ships" warning and exit 0) never appears. The PR's own reproduction needed a synthetic registry record that made 7.14.1 the active version, and the recipe leaves that step out. The code's branch condition governs here, so the doc should follow it.
  Confidence 80 · logic · Fix: list an active managed ROCm SDK as a prerequisite, and tell the tester to set therock.version to that SDK's version, not to an arbitrary nonexistent version.

• [new] docs/testing.md claims CI coverage of the restore that no lane provides — docs/testing.md:1073-1078
  The doc says the Windows Strix Halo lane's prewarm step "is what exercises it". That prewarm (xtask/src/e2e_prewarm.rs → engines install lemonade --yes) reaches the new code only when the first install fails while a kept pin equals the active SDK. On a healthy runner it takes the early Ok(None) return in install_pinned_backend_or_restore_packaged_pins, so the restore, the set-aside, the retry and the put-back never run. The PR description is accurate on this point: it says the lane "only exercises this change on a runner that still holds the stale state". The doc drops that qualifier. AGENTS.md §3 requires the reason for a missing scenario to be true, and an overstated coverage claim stops the next reader from looking.
  Confidence 75 · architectural · Fix: carry the description's qualifier into the doc, or add a scenario using the simulated-host/fake-tools e2e pattern.

• [standing, re-found blind] The restore's remove-a-pin branch has no test — engines/lemonade/src/install.rs, the None => arm of set_or_remove_pin, plus its two non-object coercions
  Every new test's packaged JSON contains both therock.version and llamacpp.rocm-stable, so the arm that removes a key the packaged file lacks never runs. That state is real: alignment's own revert removes llamacpp.rocm-stable when no prior tag existed (remove_backend_versions_llamacpp_tag). If the arm were turned into a no-op, every test would stay green, a stale Tier-2 tag would survive the restore, and the drift check would fire on every later run. The description says 4a45d1b adds this test; at 620a872 it is absent.
  Confidence 80 · logic · Fix: add a restore_over case where the packaged JSON has no llamacpp.rocm-stable, and assert the key is removed and the other entries survive.

• [standing, settled by putting it to the reviewer — still holds] equal_pin_whose_restore_fails_reports_the_original_failure passes with the production change reverted — engines/lemonade/src/backend_alignment.rs (tests)
  Wording from the previous round: with the old fallback_install(manifest, false)?; return Ok(None); restored in the pinned_version == target_version branch, this test still passes because it discards _restores and cannot tell that a restore was attempted; its name claims a path it cannot fail on. Fix: assert restores == 1.

• [standing, unverified this round] alignment_pins ignoring llamacpp.rocm-stable would go unnoticed — engines/lemonade/src/install.rs (alignment_pins)
  Wording from the previous round: changing alignment_pins to ignore the llamacpp.rocm-stable key makes the restore call a pin file "already packaged" when only the llama.cpp tag has drifted, and no test fails, because every fixture drifts both keys together. Not re-found blind at 620a872 and not put to the reviewer. Fix: a restore case where only rocm-stable drifted, asserting the restore happens.

Non-blocking

• [new] A pin file that exists but cannot be read is treated as absent, so a failed retry deletes it — engines/lemonade/src/install.rs, let current = fs::read(&target).ok();
  Every read error, not only NotFound, becomes previous: None. The file is then overwritten with the packaged bytes, and if the retry fails put_back removes it. This silently loses a user file in an edge case. The description says 4a45d1b fixes this.
  Confidence 75 · logic · Fix: map NotFound to None and bail on any other error.

• [new] A failed set-aside followed by a failed retry deletes a working backend — engines/lemonade/src/backend_alignment.rs, install_pinned_backend_or_restore_packaged_pins → resolve_rocm_llamacpp_backend_dirs_aside(.., false)
set_aside_rocm_llamacpp_backend_dirs only warns when a rename fails, which happens on Windows when a running server holds DLLs. resolve(false) then runs remove_dir_all on every ROCm-family name, including a directory that was never moved aside. The new doc comment's promise that "a failed retry does not leave the host without a ROCm backend" fails on this path. The weakness already exists in alignment's revert; this change adds a new and more frequent trigger for it.
  Confidence 70 · architectural · Fix: on failure, clear only names that were moved aside or did not exist before the set-aside; or skip the retry when any set-aside failed.

• [new] The README's trigger condition is broader than the code — README.md:753-758
  "If installing an already-pinned backend fails, it restores…" also covers the no-SDK, unreadable-pin and failed-probe paths, and none of those restore. docs/engine-plugins.md states the scope correctly.
  Confidence 75 · mechanical · Fix: qualify it as "when the kept pin matches the active ROCm SDK".

• [new] Undocumented: the restore deletes a cached archive that fails its digest — docs/engine-plugins.md:79-85; install.rs ensure_cached_archive → ensure_cached_archive_after_invalid (fs::remove_file(archive))
  The user doc says only that the restore "is skipped". It does not say the shared downloads/ copy is removed. The code comment does say so, and the bad-digest test does not assert either way.
  Confidence 70 · mechanical · Fix: say so in the doc, and assert it in restore_does_not_replace_a_cached_archive_that_fails_its_digest.

• [new] The focused-test commands miss two new tests — docs/testing.md:1066-1070
the_retry_sets_the_backend_directories_aside_and_a_success_discards_them and a_backup_of_a_missing_pin_file_removes_the_file_again match none of the filters restore, equal_pin or failed_retry.
  Confidence 80 · mechanical · Fix: add filters for them or rename the tests.

• [new] A repo file is cited as a bare backtick path — docs/testing.md:1062-1064
`docs/engine-plugins.md`, section "…" should be a relative link (AGENTS.md §5): [engine-plugins.md](engine-plugins.md#lemonade-backend-alignment-on-engine-install).
  Confidence 85 · mechanical.

• [new] The restore_over test helper never removes its scratch directory — engines/lemonade/src/install.rs tests
  Every sibling helper calls remove_dir_all; this one does not, and three tests use it.
  Confidence 85 · mechanical · Fix: remove the directory before returning.

• [standing from the previous round's non-blocking list] The set-aside warning names the wrong operation on the new path — backend_alignment.rs:1139
  It prints "could not move … aside before backend alignment", but the restore retry now calls it too.
  Confidence 70 · mechanical · Fix: use neutral wording.

• [new] The pin-key list is defined in two places — install.rs ALIGNMENT_PIN_KEYS and the hard-coded therock/version and llamacpp/rocm-stable in backend_alignment.rs's read/write helpers
  If alignment ever pins another key, the restore will silently stop resetting it.
  Confidence 70 · architectural · Fix: define the keys once and use them from both files.

• [new] A doc comment reads as if the whole recovery leaves backends alone — install.rs, the doc comment of restore_packaged_backend_versions
  "Backends already installed in the tree, and any server running from it, are left alone" is true of this function. Its only caller then sets the backend directories aside and force-reinstalls.
  Confidence 70 · mechanical · Fix: mention the caller's forced retry.

Decisions for the author

• Every install failure triggers the restore — tradeoff
  A deterministic failure unrelated to the pin also costs an archive extract, a set-aside, a forced retry and a put-back: for example no supported GPU backend, or a lemond status timeout. A transient failure followed by a successful retry discards a working aligned pin. Against that, a stale pin otherwise blocks every install until --reinstall. The description acknowledges this. Narrowing the trigger to download-class errors is the alternative.
• The successful retry is neither re-aligned nor verified — non-blocking-improvement
  After the restore, the retry installs the packaged backend without rocm_backend_resolves. The next run sees the pin differ from the target and runs alignment again, which is another forced install. Running align against the restored pins, rather than a plain forced install, would converge in one run.

Positive signals

• The after_archive seam in restore_packaged_backend_versions_with makes the read-after-archive ordering testable against a simulated concurrent writer, not just asserted in prose.
• The retry reuses alignment's existing set-aside and resolve helpers instead of copying them, so the two revert paths cannot drift apart.
• Every warning test asserts the resulting pin-file or directory state alongside the message (AGENTS.md §3). The root-cause-only warning test pins down that the --reinstall advice is not repeated.

Deployment notes

• A plain rocm engines install lemonade can now rewrite therock.version and llamacpp.rocm-stable in a shared runtime tree and replace the installed ROCm backend. ROCM_CLI_DISABLE_LEMONADE_BACKEND_ALIGNMENT opts out.
• rocm_core::write_atomic is now pub API. The rocm-engine-lemonade → rocm-core edge already existed, so no new crate edge was added.

What this covered

I read the whole diff prw-base…620a872e (f923273…620a872e, one commit, 8 files) completely. I also read the surrounding code:

• in backend_alignment.rs: the prepare_llamacpp_backend_for_active_rocm* functions, the alignment revert, and the set-aside and resolve helpers;
• in install.rs: prepare_embeddable_with and ensure_cached_archive*;
• the install_response caller in lib.rs;
• xtask/src/e2e_prewarm.rs and xtask/src/crate_edges.rs, as they relate to this change.

Two read-only workers ran: one on the coupled Rust files, and one covering agent-instruction adherence, doc accuracy, history and code comments. I ran consolidation, the design questions (not fanned out further, because of the two-worker cap) and Step 9 myself.

The reviewed commit is stale. The live PR head is 4a45d1b. According to its description, that head adds the remove-a-pin test, makes an unreadable pin file an error, and changes how the backup read works. This report does not cover 4a45d1b, and the description's claims about it were not checked against code. Re-run the review on the current head.

Did not run:

• No build, test or clippy ran locally, because the disk is at its floor. Every mutation verdict comes from reading, so no confidence is above 85.
• The prior-changes pass did not run, because earlier PR review comments are out of scope here.
• CI evidence for 620a872: all hosted checks passed. The Strix Halo Ubuntu lanes fail identically on main. The MI300X and W7900 lanes were still queued, so there is no evidence from them.

The commit sign-off check ran and passed (1 commit, none missing).

This report was not reconciled against any PR discussion.

[volen-silo]

Follow-up on the review of 620a872e. Two of its points are fixed in d71fab48 (docs only, still one commit); the three standing test points were already addressed in 4a45d1b9.

• Manual recipe could not reach the restore. Correct: the recovery only runs when the pinned therock.version equals the active SDK's version (backend_alignment.rs, the pinned_version == target_version branch). docs/manual-testing.md now lists an active managed ROCm SDK as a prerequisite, tells the tester to set therock.version to that SDK's version, and says what happens otherwise. I did not re-run the recipe against a real SDK; the earlier reproduction used a synthetic registry record.
• Overstated CI coverage. Correct: on a healthy runner the prewarm install succeeds and the restore never runs. docs/testing.md now says the lane reaches it only on a runner that still holds a stale pin, so a green lane does not prove the restore works.
• Remove-a-pin branch, alignment_pins drift on llamacpp.rocm-stable, and the restores == 1 assertion: added in 4a45d1b9 (restore_removes_a_pin_the_packaged_file_does_not_have, restore_happens_when_only_the_rocm_stable_pin_drifted, and the assertion in equal_pin_whose_restore_fails_reports_the_original_failure). Each fails when the matching production line is changed.