From 20dcca5e1c421dcbd1129618b875b16145a03381 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 27 Oct 2026 10:13:58 -0700 Subject: [PATCH] fix(desktop): look for app-folder holders only in the folder being renamed The swap renames release/win-unpacked alone, but discovery, the re-check before closing and the close itself looked for holders anywhere under release/. A program whose working folder is a sibling (a stale win-arm64-unpacked) was reported as blocking and offered for closing, though it never blocked the rename. All three steps now take the folder being renamed. close_holders compares start times with _START_TIME_TOLERANCE_S instead of a literal 0.5, and take_consent decodes the consent file after removing it and treats a non-UTF-8 file as no consent instead of raising out of a swap documented never to raise. Review folds 1, 2 and 4 (kshitijk4poor) on #144890. --- hermes_cli/desktop_release_holders.py | 27 +++++++------- hermes_cli/main_desktop.py | 12 ++++--- ...est_desktop_release_holder_windows_live.py | 36 +++++++++++++++++++ .../test_desktop_release_holders.py | 11 ++++++ 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/hermes_cli/desktop_release_holders.py b/hermes_cli/desktop_release_holders.py index 1b9cf24efc..bdc16b4703 100644 --- a/hermes_cli/desktop_release_holders.py +++ b/hermes_cli/desktop_release_holders.py @@ -93,13 +93,14 @@ def _exe_or_empty(proc) -> str: return "" -def find_release_cwd_holders(release_dir: Path) -> list[ReleaseHolder]: - """Every other process whose working directory is ``release_dir`` or below it. +def find_release_cwd_holders(folder: Path) -> list[ReleaseHolder]: + """Every other process whose working directory is ``folder`` (the app folder being renamed, + e.g. ``release/win-unpacked``) or below it. A process whose cwd cannot be read (another user's, access denied) is skipped: it cannot be shown to hold the tree. Never raises. """ - root = _release_root(release_dir) + root = _release_root(folder) try: import psutil procs = psutil.process_iter(["pid", "name"]) @@ -130,13 +131,13 @@ def take_consent(release_dir: Path, run_id: Optional[str]) -> HolderConsent: or not it applies, and it applies only to the hand-off run that the Desktop started for it.""" path = release_dir / CONSENT_FILE_NAME try: - raw = path.read_text(encoding="utf-8-sig") + raw = path.read_bytes() except OSError: return NO_CONSENT with contextlib.suppress(OSError): path.unlink() try: - data = json.loads(raw) + data = json.loads(raw.decode("utf-8-sig")) if not run_id or data.get("run_id") != run_id: return NO_CONSENT holders = frozenset( @@ -160,10 +161,10 @@ def _ask_at_terminal(holders: list[ReleaseHolder], input_fn: Callable[[str], str return answer.strip().lower() in ("y", "yes") -def close_holders(holders: list[ReleaseHolder], release_dir: Path) -> list[int]: +def close_holders(holders: list[ReleaseHolder], folder: Path) -> list[int]: """Terminate exactly these holders, each re-checked right before: same pid AND creation - time (no recycled pid), cwd still inside ``release_dir``, and not one of our ancestors.""" - root = _release_root(release_dir) + time (no recycled pid), cwd still inside ``folder``, and not one of our ancestors.""" + root = _release_root(folder) try: import psutil ancestors = {p.pid for p in psutil.Process(os.getpid()).parents()} @@ -173,7 +174,7 @@ def close_holders(holders: list[ReleaseHolder], release_dir: Path) -> list[int]: for holder in holders: try: proc = psutil.Process(holder.pid) - if proc.pid in ancestors or abs(proc.create_time() - holder.create_time) > 0.5: + if proc.pid in ancestors or abs(proc.create_time() - holder.create_time) > _START_TIME_TOLERANCE_S: continue if root is None or not _inside(_norm(proc.cwd()), root): continue @@ -193,12 +194,12 @@ def close_holders(holders: list[ReleaseHolder], release_dir: Path) -> list[int]: def release_after_refused_rename( - release_dir: Path, *, consent: HolderConsent = NO_CONSENT, input_fn: Callable[[str], str] = input, + folder: Path, *, consent: HolderConsent = NO_CONSENT, input_fn: Callable[[str], str] = input, ) -> tuple[list[ReleaseHolder], list[int]]: - """After Windows refused a rename under ``release_dir``: name every cwd holder, then close + """After Windows refused to rename ``folder``: name every cwd holder inside it, then close the ones covered by ``consent`` (taken at the start of this swap attempt) or agreed to at the terminal. Returns (holders found, pids closed).""" - holders = find_release_cwd_holders(release_dir) + holders = find_release_cwd_holders(folder) if not holders: return [], [] # Human lines only: the machine lines go out once, for the holders still in the way when the @@ -216,7 +217,7 @@ def release_after_refused_rename( chosen = closable if not chosen: return holders, [] - closed = close_holders(chosen, release_dir) + closed = close_holders(chosen, folder) if closed: print(f" ✓ Closed with your consent: {', '.join(h.describe() for h in chosen if h.pid in closed)}") return holders, closed diff --git a/hermes_cli/main_desktop.py b/hermes_cli/main_desktop.py index f243d22b56..729d5f3d28 100644 --- a/hermes_cli/main_desktop.py +++ b/hermes_cli/main_desktop.py @@ -254,9 +254,11 @@ def _recover_interrupted_swap(live: Path, aside: Path) -> None: shutil.rmtree(aside, ignore_errors=True) -def _move_live_app_aside(live_root: Path, previous: Path, release_dir: Path, held_by: Optional[list], consent) -> None: +def _move_live_app_aside(live_root: Path, previous: Path, held_by: Optional[list], consent) -> None: """First swap rename. On Windows a refusal names the programs whose working folder is inside - ``release/`` (#101789) and retries once only after the user consented to closing them.""" + ``live_root``, the folder being renamed (#101789), and retries once only after the user + consented to closing them. A program in a sibling folder (``win-arm64-unpacked``) does not + block this rename, so it is neither named nor offered for closing.""" try: _rename_riding_out_file_lock(live_root, previous) return @@ -264,7 +266,7 @@ def _move_live_app_aside(live_root: Path, previous: Path, release_dir: Path, hel if sys.platform != "win32": raise from hermes_cli.desktop_release_holders import find_release_cwd_holders, release_after_refused_rename - holders, closed = release_after_refused_rename(release_dir, consent=consent) + holders, closed = release_after_refused_rename(live_root, consent=consent) if not closed: if held_by is not None: held_by.extend(holders) @@ -273,7 +275,7 @@ def _move_live_app_aside(live_root: Path, previous: Path, release_dir: Path, hel _rename_riding_out_file_lock(live_root, previous) except PermissionError: if held_by is not None: - held_by.extend(find_release_cwd_holders(release_dir)) + held_by.extend(find_release_cwd_holders(live_root)) raise @@ -308,7 +310,7 @@ def _swap_staged_desktop_app( stopped = _stop_desktop_processes_locking_build(desktop_dir, also_posix=True) if stopped: logger.info("stopped desktop processes before staged app promotion: %s", stopped) - _move_live_app_aside(live_root, previous, release_dir, held_by, consent) + _move_live_app_aside(live_root, previous, held_by, consent) try: _rename_riding_out_file_lock(staged_root, live_root) except OSError: diff --git a/tests/hermes_cli/test_desktop_release_holder_windows_live.py b/tests/hermes_cli/test_desktop_release_holder_windows_live.py index 43083a81de..d0877c5473 100644 --- a/tests/hermes_cli/test_desktop_release_holder_windows_live.py +++ b/tests/hermes_cli/test_desktop_release_holder_windows_live.py @@ -9,6 +9,7 @@ and finish the swap. from __future__ import annotations +import contextlib import json import subprocess import sys @@ -144,3 +145,38 @@ def test_a_partial_close_reports_only_the_holder_still_in_the_way(swap_fixture, for proc in [*psutil.Process(protected.pid).children(recursive=True), psutil.Process(protected.pid)]: proc.kill() protected.wait(10) + + +def test_a_program_in_a_sibling_unpacked_folder_is_neither_named_nor_closed(swap_fixture, tmp_path, capsys, monkeypatch): + """Review fold 1: only release/win-unpacked is renamed. A program whose working folder is a + sibling (a stale win-arm64-unpacked) does not block it, so even a consent naming it must not + close it, and the failure lines must not offer it.""" + desktop_dir, staging, live_root, helper = swap_fixture + sibling_dir = live_root.parent / "win-arm64-unpacked" + sibling_dir.mkdir() + sibling_tree: list = [] + sibling = subprocess.Popen( + [sys.executable, str(tmp_path / "foreign_helper.py")], cwd=sibling_dir, stdout=subprocess.PIPE, + text=True, encoding="utf-8", creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), + ) + try: + assert sibling.stdout.readline().strip() == "ready" + sibling_tree.extend([psutil.Process(sibling.pid), *psutil.Process(sibling.pid).children(recursive=True)]) + consent, tree = _consent_for_run(desktop_dir, helper, "desktop-retry-1") + seen = json.loads(consent.read_text(encoding="utf-8")) + seen["holders"] += [{"pid": p.pid, "start_time": p.create_time(), "exe": p.exe()} for p in sibling_tree] + consent.write_text(json.dumps(seen), encoding="utf-8") + monkeypatch.setenv("HERMES_UPDATE_CORRELATION_ID", "desktop-retry-1") + + promoted, error = _promote(desktop_dir, staging) + + assert error == "" and promoted == live_root / "Hermes.exe" + assert helper.wait(10) is not None + assert not [p.pid for p in tree if p.is_running()] + assert all(p.is_running() for p in sibling_tree), "closed a program that never held the renamed folder" + assert f"pid {sibling.pid}" not in capsys.readouterr().out + finally: + for proc in sibling_tree: + with contextlib.suppress(psutil.NoSuchProcess): + proc.kill() + sibling.wait(10) diff --git a/tests/hermes_cli/test_desktop_release_holders.py b/tests/hermes_cli/test_desktop_release_holders.py index 2df46c8e01..ad19692451 100644 --- a/tests/hermes_cli/test_desktop_release_holders.py +++ b/tests/hermes_cli/test_desktop_release_holders.py @@ -118,3 +118,14 @@ def test_a_holder_whose_program_path_is_unreadable_is_left_for_the_user(holder, line = next(ln for ln in desktop_release_holders.describe_block(holders, release).splitlines() if f"(pid {proc.pid}, started " in ln) assert line.endswith("path unreadable)") + + +def test_a_consent_file_that_is_not_utf8_is_consumed_without_raising(tmp_path): + """Review fold 2: the swap that reads it is documented never to raise, and a consent file + left behind would fail every later swap.""" + release = tmp_path / "release" + release.mkdir() + (release / CONSENT_FILE_NAME).write_bytes(b'{"run_id": "\xff\xfe"}') + + assert take_consent(release, "desktop-retry-1") is desktop_release_holders.NO_CONSENT + assert not (release / CONSENT_FILE_NAME).exists()