Skip to content

Fix Windows update and reset recovery reporting - #280

Merged
Salil Das (sadlilas) merged 2 commits into
microsoft:mainfrom
sadlilas:fix/windows-update-reset-cache-recovery
Aug 27, 2026
Merged

Fix Windows update and reset recovery reporting#280
Salil Das (sadlilas) merged 2 commits into
microsoft:mainfrom
sadlilas:fix/windows-update-reset-cache-recovery

Conversation

@sadlilas

@sadlilas Salil Das (sadlilas) commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hit on a real native-Windows machine while running amplifier update. Fixing the
reporting there led to the same class of bug in reset, which shares the deferred
tool-swap machinery.

  • report a detached Windows self-update as staged, not completed
  • stop reset and exit nonzero when requested cache or registry cleanup is incomplete
  • exit nonzero when reset's reinstall fails, instead of reporting success with no
    Amplifier installed
  • prevent Windows reset from staging a reinstall after a failed cleanup
  • fully qualify the generated finisher's Windows utilities under System32

Root cause

Windows self-update reported the wrong outcome. On Windows, execute_self_update
hands the install to a deferred .cmd and returns immediately. It returned
updated=["amplifier"], so the CLI printed ✓ Update complete — for an install that
had not started yet. A messages line did say the update would finish in the new
window, so the information was present, but the headline contradicted it.

Reset reported success after a partial cleanup. A locked file makes
shutil.rmtree fail partway. rmtree_robust re-raises, clear_download_cache
catches OSError and returns (0, False) — and _remove_amplifier_dir had no else
branch on that result, so it counted nothing and still returned True. reset()
discarded the return value entirely and printed Reset complete! with exit 0, then
staged the reinstall.

To be precise about the blast radius: the half-deleted cache directory is not
left permanently broken. amplifier-foundation validates a cached clone before using
it (sources/git.py _verify_clone_integrity) and has no incremental-update path —
it deletes and re-clones on any integrity failure. So a cache directory that lost its
.git self-heals on the next load. The defect being fixed here is that reset lies
about having succeeded
, which is invisible to the user and undetectable by any
script or CI step driving it — not that it corrupts state permanently.

Reset also exited 0 when the reinstall failed. On the POSIX path
_uninstall_amplifier runs first, so a failed _install_amplifier leaves the user
with no Amplifier at all — and the command returned normally, reporting that as
success. This is the same honest-exit-code defect as above, with a worse outcome.

The deferred script relied on PATH for its utilities. Two independent problems.
Git for Windows ships a GNU find.exe in usr\bin, which its installer will place on
PATH; GNU find reads "<pid>" as a path rather than a pattern and exits 1, so
if not errorlevel 1 goes false and the PID wait loop is skipped. Separately,
cmd.exe resolves a bare command name from the current directory before
consulting PATH, and the finisher inherits Amplifier's cwd — typically a user's
project directory — so a stray ping.exe or tasklist.exe there would be executed.
Both are closed by spelling out %SystemRoot%\System32\...; the second reason is why
ping and tasklist are qualified and not just find.

Skipping the wait loop is partly masked today by the script's own retry-with-backoff
(the required step retries 10 times at ~3s), so this is a correctness fix rather than
a hard failure in the common case.

Verification

  • uv run pytest -q — 1449 passed, 13 deselected, 1 xfailed
  • uv run pytest -q tests/test_windows_update_reset.py — 15 passed
  • reproduced the partial-cleanup case against a real filesystem (a genuinely
    undeletable directory, not a mocked failure): before, amplifier reset -y deleted
    .git/config and .git/objects/pack/*, left the locked file, printed
    Reset complete! and exited 0; after, it exits 1 and names the failure
  • reproduced the reinstall-failure case: uninstall succeeds, cleanup succeeds,
    reinstall fails — exit code goes 0 → 1
  • native Windows isolated probe: locked-cache reset exited nonzero and launched no
    finisher
  • native Windows isolated probe: generated finisher waited correctly with Git
    usr/bin first on PATH
  • native Windows isolated probe: parent update reported Update staged, and a
    lock-release reset retry completed successfully

Breaking changes

amplifier reset now exits nonzero when cleanup or reinstall fails, where it
previously always exited 0. Anything scripting reset that assumed a 0 exit will now
correctly see the failure.

Salil Das (sadlilas) and others added 2 commits August 26, 2026 13:40
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
On POSIX, when uninstall succeeds but reinstall fails, reset() was
returning successfully (exit 0) even though the user was left with no
Amplifier installed. Changed to raise ClickException to report the
failure correctly.

Also enhanced the uv_utils comment to document both reasons the batch
script fully-qualifies Windows utilities: (a) Git for Windows shadowing
GNU find on PATH, and (b) cmd.exe resolution order checking the current
directory before PATH.

Extended test coverage for Windows batching and added explicit test for
POSIX reset failure on reinstall error.
@sadlilas
Salil Das (sadlilas) marked this pull request as ready for review August 27, 2026 02:38
@sadlilas
Salil Das (sadlilas) merged commit 6f2ad04 into microsoft:main Aug 27, 2026
9 checks passed
@sadlilas
Salil Das (sadlilas) deleted the fix/windows-update-reset-cache-recovery branch August 27, 2026 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant