From ea665a911d67b76c7677d4a29d808df63dda71e2 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 4 Oct 2026 02:20:48 -0700 Subject: [PATCH] fix(update): one custody policy for every updater git and Node child R2: hermes_cli/update_custody.run_git is THE updater git runner; update_cmd._git_run, update_cmd_git._git_run, update_cmd_stash, update_cmd_check._git, gitlock, update_cmd_zip and the fleet ancestry probe all go through it. - every updater git call carries -c gc.autoDetach=false -c maintenance.auto=false: no detached gc/maintenance child ever forks under (POSIX) or outlives (Windows) the checkout lock; - POSIX: only local mutators (reset, checkout, merge, stash, rebase, update-ref, clean, apply, commit, ...) inherit the lock fd, with core.fsmonitor=false; fetch/ls-remote/credential and readers run without it, so a credential-cache daemon never holds the checkout (reviewer: up to 2 h of installer waits). On Linux those fd-less children die with the updater (PR_SET_PDEATHSIG) instead of rewriting refs under the next owner; - Windows: inside an update every child is created suspended, assigned to the owner's kill-on-close job and only then resumed; the Node build (pm.progress.run_contained) joins it through a stdlib launcher. The fork sync fast-forwards locally (merge --ff-only refs/remotes/upstream/main after its fetch) instead of a networked pull under the mutator fd. --- hermes_cli/gitlock.py | 45 +-- hermes_cli/source_build.py | 23 +- hermes_cli/update_cmd.py | 17 +- hermes_cli/update_cmd_check.py | 6 +- hermes_cli/update_cmd_fleet_checkout.py | 6 +- hermes_cli/update_cmd_git.py | 29 +- hermes_cli/update_cmd_stash.py | 18 +- hermes_cli/update_cmd_zip.py | 10 +- hermes_cli/update_custody.py | 257 ++++++++++++++++++ tests/hermes_cli/test_update_custody.py | 166 +++++++++++ .../test_update_fetch_failure_classifier.py | 8 +- .../test_update_fleet_checkout_build_stamp.py | 5 +- .../test_update_lock_windows_live.py | 106 ++++++++ .../hermes_cli/test_update_target_identity.py | 13 +- 14 files changed, 638 insertions(+), 71 deletions(-) create mode 100644 hermes_cli/update_custody.py create mode 100644 tests/hermes_cli/test_update_custody.py diff --git a/hermes_cli/gitlock.py b/hermes_cli/gitlock.py index 3258729bec..22504b38b3 100644 --- a/hermes_cli/gitlock.py +++ b/hermes_cli/gitlock.py @@ -19,6 +19,7 @@ from hermes_cli._subprocess_compat import ( noninteractive_git_env, windows_hide_flags, ) +from hermes_cli.update_custody import git_argv, run_git logger = logging.getLogger(__name__) @@ -161,16 +162,16 @@ def disable_tree0_auto_maintenance(repo_root: Path) -> None: _migrate_earlier_maintenance_keys(repo_root) for key, value in _TREE0_MAINTENANCE_OFF: try: - current = subprocess.run( - ["git", "config", "--local", "--get", key], + current = run_git( + ["git"], ["config", "--local", "--get", key], cwd=str(repo_root), capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, creationflags=windows_hide_flags(), ).stdout.strip() if current == value: continue - subprocess.run( - ["git", "config", "--local", key, value], + run_git( + ["git"], ["config", "--local", key, value], cwd=str(repo_root), check=True, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, @@ -198,8 +199,8 @@ def _migrate_earlier_maintenance_keys(repo_root: Path) -> None: or "maintenance.commit-graph.enabled" in local): return for key in ("maintenance.auto", "gc.auto") if local.get("gc.auto") == "0" else ("maintenance.auto",): - subprocess.run( - ["git", "config", "--local", "--unset", key], + run_git( + ["git"], ["config", "--local", "--unset", key], cwd=str(repo_root), check=True, capture_output=True, timeout=30, creationflags=windows_hide_flags(), ) @@ -231,8 +232,8 @@ def clear_stale_tmp_packs(repo_root: Path, *, min_age_seconds: Optional[int] = N def _git_stdout_lines(repo_root: Path, args: List[str]) -> List[str]: """Run a read-only git query in ``repo_root``; [] on any failure.""" try: - result = subprocess.run( - ["git", *args], cwd=str(repo_root), + result = run_git( + ["git"], [*args], cwd=str(repo_root), capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10, creationflags=windows_hide_flags(), ) @@ -257,8 +258,8 @@ def _batch_missing_parents(repo_root: Path, candidates: List[str]) -> set[str]: parents_by_commit = {} parents = set() request = "\n".join(candidates) + "\n" - result = subprocess.run( - ["git", "cat-file", "--batch"], + result = run_git( + ["git"], ["cat-file", "--batch"], cwd=str(repo_root), input=request.encode(), capture_output=True, @@ -295,8 +296,8 @@ def _batch_missing_parents(repo_root: Path, candidates: List[str]) -> set[str]: return set() if not parents: return set() - check = subprocess.run( - ["git", "cat-file", "--batch-check"], + check = run_git( + ["git"], ["cat-file", "--batch-check"], cwd=str(repo_root), input=("\n".join(sorted(parents)) + "\n").encode(), capture_output=True, @@ -379,8 +380,8 @@ def repair_broken_shallow_boundaries(repo_root: Path) -> int: if shallow_path is None: return 0 # Cheap gate: repair only when the walk the corruption breaks already fails. - probe = subprocess.run( - ["git", "rev-list", "--count", "--all", "--reflog"], + probe = run_git( + ["git"], ["rev-list", "--count", "--all", "--reflog"], cwd=str(repo_root), capture_output=True, timeout=10, creationflags=windows_hide_flags(), ) @@ -473,8 +474,8 @@ def prune_stale_shallow_grafts(repo_root: Path) -> int: ): entries = _git_stdout_lines(repo_root, ["reflog", "show", "--format=%H", ref]) if set(entries) & dropped: - subprocess.run( - ["git", "reflog", "expire", "--expire=now", ref], + run_git( + ["git"], ["reflog", "expire", "--expire=now", ref], cwd=str(repo_root), capture_output=True, timeout=10, creationflags=windows_hide_flags(), ) @@ -499,15 +500,15 @@ def prune_stale_shallow_grafts(repo_root: Path) -> int: def _partial_clone_filter(repo_root: Path, **run_kwargs) -> "str | None": """The checkout's own ``remote.origin.partialclonefilter``, or None for a non-partial clone.""" - result = subprocess.run( - ["git", "config", "--get", "remote.origin.promisor"], + result = run_git( + ["git"], ["config", "--get", "remote.origin.promisor"], cwd=str(repo_root), capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, **run_kwargs, ) if result.returncode != 0 or result.stdout.strip().lower() != "true": return None - configured = subprocess.run( - ["git", "config", "--get", "remote.origin.partialclonefilter"], + configured = run_git( + ["git"], ["config", "--get", "remote.origin.partialclonefilter"], cwd=str(repo_root), capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, **run_kwargs, ) @@ -553,8 +554,8 @@ def fetch_full_commit_graph(repo_root: Path, *extra_refspecs: str, **run_kwargs) if converts: fetch_filter = "blob:none" try: - subprocess.run( - ["git", "fetch", "--quiet", *(["--unshallow"] if shallow else []), + run_git( + ["git"], ["fetch", "--quiet", *(["--unshallow"] if shallow else []), *([f"--filter={fetch_filter}"] if fetch_filter else []), "--no-tags", "origin", "refs/tags/v*:refs/tags/v*", *extra_refspecs], cwd=str(repo_root), check=True, capture_output=True, text=True, diff --git a/hermes_cli/source_build.py b/hermes_cli/source_build.py index f7846d38fe..eef127eda9 100644 --- a/hermes_cli/source_build.py +++ b/hermes_cli/source_build.py @@ -48,18 +48,23 @@ def source_build_env(base_env: dict | None = None, *, explicit: bool = False) -> def run_source_script(project_root: Path, script: str, *args: str, env: dict, label: str) -> None: from pm.progress import run_contained + from hermes_cli.update_custody import contained_command from hermes_cli.update_lock import checkout_lock_fds - # The build writes the checkout: node (and npm/esbuild under it) inherits the update's - # checkout lock fd, so the checkout is never handed to a contender while it still writes. + # The build writes the checkout: node (and npm/esbuild under it) stays in the update's custody + # (POSIX: the checkout lock fd; Windows: the owner's kill-on-close job), so the checkout is + # never handed to a contender while it still writes. fds = checkout_lock_fds(project_root) - # npm's deprecation warnings are the loudest lines and never actionable - # here; they still land in the failure tail. - run_contained( - [shutil.which("node", path=env["PATH"]), str(project_root / script), *args], - label, hide=lambda line: line.lower().startswith("npm warn"), indent=" ", - cwd=project_root, env=env, **({"pass_fds": fds} if fds else {}), - ) + command = [shutil.which("node", path=env["PATH"]), str(project_root / script), *args] + with contained_command(command) as (argv, custody): + if fds and "pass_fds" not in custody: # an inherited lock fd this process did not take + custody = {**custody, "pass_fds": fds} + # npm's deprecation warnings are the loudest lines and never actionable + # here; they still land in the failure tail. + run_contained( + argv, label, hide=lambda line: line.lower().startswith("npm warn"), indent=" ", + cwd=project_root, env=env, **custody, + ) def prepare_source_dependencies(project_root: Path, workspaces: tuple[str, ...], *, env: dict, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 4b45e5f3be..ffffa799d1 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -260,14 +260,13 @@ def _git_run(git_cmd, args, cwd=None, *, check=False, network=False): # calls, so layer them instead of passing the keyword twice. spawn_kwargs = {"timeout": NETWORK_GIT_TIMEOUT_SECONDS, **_no_prompt_git_kwargs()} if network else {} spawn_kwargs.setdefault("creationflags", windows_hide_flags()) - from hermes_cli.update_lock import custody_spawn_kwargs + from hermes_cli.update_custody import run_git - # Git (and the hooks it runs) mutates the checkout: it keeps the update's checkout lock fd, - # so a killed updater cannot hand the checkout to a contender while git still writes (R2). - spawn_kwargs.update(custody_spawn_kwargs()) + # The one custody policy (R2): local mutators keep the update's checkout lock fd, network + # git runs without it and dies with us, no git forks a detached gc/maintenance child. try: - return subprocess.run( - git_cmd + args, cwd=_m().PROJECT_ROOT if cwd is None else cwd, capture_output=True, + return run_git( + git_cmd, args, cwd=_m().PROJECT_ROOT if cwd is None else cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", check=check, **spawn_kwargs) except subprocess.TimeoutExpired as exc: @@ -298,8 +297,10 @@ def _heal_stale_shallow_checkout(repo_root: Path, branch: str) -> None: def _capture_head_sha(git_cmd, cwd) -> str | None: """Return the current HEAD SHA, or None if it can't be resolved.""" try: - result = subprocess.run( - git_cmd + ["rev-parse", "HEAD"], + from hermes_cli.update_custody import run_git + + result = run_git( + git_cmd, ["rev-parse", "HEAD"], cwd=cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", diff --git a/hermes_cli/update_cmd_check.py b/hermes_cli/update_cmd_check.py index b3a6b9cd88..d599b473f5 100644 --- a/hermes_cli/update_cmd_check.py +++ b/hermes_cli/update_cmd_check.py @@ -18,8 +18,10 @@ def _git(git_cmd: list[str], root: Path, args: list[str], **kwargs: Any) -> subp # Callers pass **_no_prompt_git_kwargs() which already carries creationflags; # OR the hide flag into the shared kwargs instead of passing the keyword twice. kwargs["creationflags"] = kwargs.get("creationflags", 0) | windows_hide_flags() - return subprocess.run( - git_cmd + args, cwd=root, capture_output=True, text=True, encoding="utf-8", errors="replace", + from hermes_cli.update_custody import run_git + + return run_git( + git_cmd, args, cwd=root, capture_output=True, text=True, encoding="utf-8", errors="replace", **kwargs, ) diff --git a/hermes_cli/update_cmd_fleet_checkout.py b/hermes_cli/update_cmd_fleet_checkout.py index e613b5a4b4..0448b191a9 100644 --- a/hermes_cli/update_cmd_fleet_checkout.py +++ b/hermes_cli/update_cmd_fleet_checkout.py @@ -34,8 +34,10 @@ def checkout_contains(sha: str) -> bool: stamped = str(identity.get("sha") or "") return bool(stamped) and (stamped == sha or stamped.startswith(sha) or sha.startswith(stamped)) try: - result = subprocess.run( - ["git", "merge-base", "--is-ancestor", sha, "HEAD"], + from hermes_cli.update_custody import run_git + + result = run_git( + ["git"], ["merge-base", "--is-ancestor", sha, "HEAD"], cwd=_m().PROJECT_ROOT, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10, # A SHA this checkout lacks is not contained; asking the promisor remote for it would # download that commit's entire history (on-demand fetches send no "have" lines). diff --git a/hermes_cli/update_cmd_git.py b/hermes_cli/update_cmd_git.py index ee7453aa0e..d8295ed3a1 100644 --- a/hermes_cli/update_cmd_git.py +++ b/hermes_cli/update_cmd_git.py @@ -41,8 +41,11 @@ def _git_run(git_cmd, args, cwd=None, *, check=False, env=None): Windows ANSI code page (#52649). ``check=True`` raises on non-zero exit. The spawn always hides its console window (#117781). ``env`` replaces the child's environment. """ - return subprocess.run( - git_cmd + list(args), + from hermes_cli.update_custody import run_git + + # THE custody policy (R2): stash push / reset --hard keep the update's lock fd, fetch does not. + return run_git( + git_cmd, list(args), cwd=cwd, capture_output=True, text=True, encoding="utf-8", errors="replace", @@ -145,12 +148,13 @@ def _branch_head_label(git_cmd=None, cwd=None) -> str | None: Appended to summary lines so a checkout parked on a stale branch is visible.""" from hermes_cli.update_cmd import _m + from hermes_cli.update_custody import run_git try: cmd = list(git_cmd) if git_cmd else ["git"] root = cwd if cwd is not None else _m().PROJECT_ROOT def _rev_parse(*args): - return subprocess.run(cmd + ["rev-parse", *args], cwd=root, **_GIT_TEXT_KW) + return run_git(cmd, ["rev-parse", *args], cwd=root, **_GIT_TEXT_KW) branch, sha = _rev_parse("--abbrev-ref", "HEAD"), _rev_parse("--short", "HEAD") branch_name, sha_text = branch.stdout.strip(), sha.stdout.strip() @@ -370,13 +374,14 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path, *, assume_yes: """ from hermes_cli.update_cmd import _count_commits_between, _has_upstream_remote, _no_prompt_git_kwargs, _should_skip_upstream_prompt from hermes_cli.update_cmd_check import tracking_refspec + from hermes_cli.update_custody import run_git if not _has_upstream_remote(git_cmd, cwd) and ( _should_skip_upstream_prompt() or not _offer_upstream_remote(git_cmd, cwd, assume_yes=assume_yes, input_fn=input_fn) ): return False print("\n→ Fetching upstream...") try: - subprocess.run(git_cmd + ["fetch", "upstream", tracking_refspec("upstream", "main"), "--quiet"], cwd=cwd, capture_output=True, check=True, **_no_prompt_git_kwargs()) + run_git(git_cmd, ["fetch", "upstream", tracking_refspec("upstream", "main"), "--quiet"], cwd=cwd, capture_output=True, check=True, **_no_prompt_git_kwargs()) except subprocess.CalledProcessError: print(" ✗ Failed to fetch upstream. Skipping upstream sync.") return False @@ -397,7 +402,10 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path, *, assume_yes: return True print(f"\n→ Fork is {upstream_ahead} commit(s) behind upstream\n→ Pulling from upstream...") try: - subprocess.run(git_cmd + ["pull", "--ff-only", "upstream", "main"], cwd=cwd, check=True, **_no_prompt_git_kwargs()) + # The fetch above already brought upstream/main: a local fast-forward (no network, so no + # credential helper is started under the checkout lock fd a mutator inherits). + run_git(git_cmd, ["merge", "--ff-only", "refs/remotes/upstream/main"], cwd=cwd, check=True, + **_no_prompt_git_kwargs()) except subprocess.CalledProcessError: print(" ✗ Failed to pull from upstream. You may need to resolve conflicts manually.") return False @@ -460,7 +468,9 @@ def _print_fetch_failure(stderr: str) -> None: def _probe_fork_bomb(argv: list) -> Optional[bool]: """Run `` --version``; True/False = guard message seen/absent, None = probe itself failed.""" try: - result = subprocess.run(argv + ["--version"], timeout=15, **_GIT_TEXT_KW) + from hermes_cli.update_custody import run_git + + result = run_git(argv, ["--version"], timeout=15, **_GIT_TEXT_KW) except Exception: return None return "fork bomb" in ((result.stdout or "") + (result.stderr or "")).lower() @@ -596,11 +606,12 @@ def _normalize_managed_eol(git_cmd, repo_root): itself, is the only path left that can fix them. See #67730. """ from hermes_cli.update_cmd_git import _git_run + from hermes_cli.update_custody import run_git # -c, not config: evaluate the tree as it WOULD look pinned, persisting nothing. probe = git_cmd + ["-c", "core.autocrlf=false"] def _probe_run(*args, **kw): - return subprocess.run(probe + list(args), cwd=repo_root, **_GIT_TEXT_KW, **kw) + return run_git(probe, list(args), cwd=repo_root, **_GIT_TEXT_KW, **kw) def _eol_only(): """Dirty paths whose ONLY change is CRLF; None when either probe fails.""" @@ -630,5 +641,5 @@ def _normalize_managed_eol(git_cmd, repo_root): if _eol_only(): # still dirty: pinning would only surface churn we failed to clear return print(f"→ Normalized line-ending churn ({len(eol_only)} file(s))") - subprocess.run(git_cmd + ["config", "core.autocrlf", "false"], cwd=repo_root, capture_output=True, check=False, - creationflags=windows_hide_flags()) + run_git(git_cmd, ["config", "core.autocrlf", "false"], cwd=repo_root, capture_output=True, check=False, + creationflags=windows_hide_flags()) diff --git a/hermes_cli/update_cmd_stash.py b/hermes_cli/update_cmd_stash.py index ae55fbfa1b..b2164955ed 100644 --- a/hermes_cli/update_cmd_stash.py +++ b/hermes_cli/update_cmd_stash.py @@ -35,8 +35,10 @@ _pending_autostash: Optional[tuple[str, int]] = None def _git_quiet(git_cmd: list[str], args: list[str], cwd: Path, **kwargs): """``subprocess.run`` of a git command with captured output; None when git cannot run.""" + from hermes_cli.update_custody import run_git + try: - return subprocess.run(git_cmd + args, cwd=cwd, capture_output=True, **kwargs) + return run_git(git_cmd, args, cwd=cwd, capture_output=True, **kwargs) except (OSError, subprocess.SubprocessError): return None @@ -66,7 +68,9 @@ def _git_paths_z(git_cmd: list[str], args: list[str], cwd: Path): def _reset_hard(git_cmd: list[str], cwd: Path) -> None: - subprocess.run(git_cmd + ["reset", "--hard", "HEAD"], cwd=cwd, capture_output=True) + from hermes_cli.update_custody import run_git + + run_git(git_cmd, ["reset", "--hard", "HEAD"], cwd=cwd, capture_output=True) def _print_nonempty(text: str, prefix: str = "") -> None: @@ -90,7 +94,7 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st # "needs merge"; `git reset` drops only the index conflict state, not the tree. if _git_run(git_cmd, ["ls-files", "--unmerged"], cwd).stdout.strip(): print("→ Clearing unmerged index entries from a previous conflict...") - subprocess.run(git_cmd + ["reset"], cwd=cwd, capture_output=True) + _git_quiet(git_cmd, ["reset"], cwd) # Intent-to-add entries are the other index state `git stash push` refuses (see # _intent_to_add_paths). Promoting them to real staged adds keeps their content and is lossless @@ -264,6 +268,8 @@ def _untracked_files_replaced_by_update(git_cmd: list[str], cwd: Path, stash_ref now tracks its own file there (``HEAD:``) and the tree does not hold the stash's untracked copy (``^3``). An untracked occupant HEAD does not track is the #70127 file that could not be deleted at stash time, whatever its content now; it is never the update's.""" + from hermes_cli.update_custody import run_git + replaced = [] suffix = " already exists, no checkout" for ln in (stderr or "").splitlines(): @@ -271,12 +277,10 @@ def _untracked_files_replaced_by_update(git_cmd: list[str], cwd: Path, stash_ref if not ln.endswith(suffix): continue rel = ln[: -len(suffix)] - tracked = subprocess.run([*git_cmd, "cat-file", "-e", f"HEAD:{rel}"], cwd=cwd, capture_output=True, check=False) + tracked = run_git(git_cmd, ["cat-file", "-e", f"HEAD:{rel}"], cwd=cwd, capture_output=True, check=False) if tracked.returncode != 0: continue - stashed = subprocess.run( - [*git_cmd, "show", f"{stash_ref}^3:{rel}"], cwd=cwd, capture_output=True, check=False, - ) + stashed = run_git(git_cmd, ["show", f"{stash_ref}^3:{rel}"], cwd=cwd, capture_output=True, check=False) try: current = (Path(cwd) / rel).read_bytes() except OSError: diff --git a/hermes_cli/update_cmd_zip.py b/hermes_cli/update_cmd_zip.py index ce8db1ca96..c64d6f7269 100644 --- a/hermes_cli/update_cmd_zip.py +++ b/hermes_cli/update_cmd_zip.py @@ -142,21 +142,23 @@ def _zip_overlay_block_reason( """ if not (root / ".git").exists(): return None + from hermes_cli.update_custody import run_git + git_cmd = ["git", "-c", "windows.appendAtomically=false"] if sys.platform == "win32" else ["git"] - result = subprocess.run( + result = run_git( # -uall: a user-level ``status.showUntrackedFiles = no`` must not blind this guard. --ignored=matching: # gitignored files are still USER DATA the overlay would delete; ``matching`` reports an ignored dir # as one ``dir/`` line. ``--ignored=all`` is NOT a valid git mode (exits 128, would fail-close every update). # ``matching`` reports an ignored directory as one ``dir/`` line instead of enumerating its contents # (cheaper, same verdict for the top-level filter below). See #87392. - git_cmd + ["status", "--porcelain", "--untracked-files=all", "--ignored=matching"], + git_cmd, ["status", "--porcelain", "--untracked-files=all", "--ignored=matching"], cwd=root, capture_output=True, text=True, encoding="utf-8", errors="replace", ) if result.returncode == 0 and shipped is None: # Before the download the ZIP's entry set is unknown; the tracked root entries stand in for it (the # pre-swap re-check gets the real set), so an ignored root entry the swap never touches cannot refuse. - tracked = subprocess.run( - git_cmd + ["ls-tree", "--name-only", "HEAD"], + tracked = run_git( + git_cmd, ["ls-tree", "--name-only", "HEAD"], cwd=root, capture_output=True, text=True, encoding="utf-8", errors="replace", ) shipped = set(tracked.stdout.splitlines()) if tracked.returncode == 0 else None diff --git a/hermes_cli/update_custody.py b/hermes_cli/update_custody.py new file mode 100644 index 0000000000..1160f2f83c --- /dev/null +++ b/hermes_cli/update_custody.py @@ -0,0 +1,257 @@ +"""Checkout custody for the updater's children (R2): the ONE spawn policy for every git (and the +Node build) an update starts. + +The checkout kernel lock (``update_lock``) must stay held while any process of the update tree +can still write the checkout, and must NOT leak into processes that outlive the update: + +* Every updater git call carries ``-c gc.autoDetach=false -c maintenance.auto=false``: git never + forks a detached gc/maintenance child that would inherit (POSIX) or outlive (Windows) the lock. +* POSIX: the lock fd is inherited ONLY by git commands that mutate the worktree, index or refs + locally (:data:`LOCAL_MUTATORS`, run with ``core.fsmonitor=false`` so no fsmonitor daemon + starts under them). Network/credential commands (fetch, ls-remote, credential) and readers run + without it: a ``git credential-cache--daemon`` they start never holds the checkout. A killed + fetch that leaves ``*.lock`` ref files is recovered by the stale-lock rules + (``gitlock.clear_stale_git_locks``). +* Windows has no fd inheritance: while this process holds (or joined) the checkout lock, every + child started here is created SUSPENDED, assigned to the update's kill-on-close job and only + then resumed, so it and everything it spawns die with the lock owner. The Node build goes + through ``pm.progress.run_contained`` (whose Popen it never sees): :func:`contained_command` + wraps it in a stdlib launcher that joins the job before it starts node. + +Every updater git runner (``update_cmd._git_run``, ``update_cmd_git._git_run``, +``update_cmd_stash``, ``update_cmd_check``, ``gitlock``, ``update_cmd_commit``) calls +:func:`run_git`; nothing else spawns updater git. +""" + +from __future__ import annotations + +import contextlib +import logging +import subprocess +import sys +from collections.abc import Sequence + +logger = logging.getLogger(__name__) + +# No detached child: `git gc --auto` / `maintenance run --auto --detach` would otherwise fork a +# daemonized repack after any command that writes objects (commit, merge, fetch, stash). +GIT_NO_DETACH = ("-c", "gc.autoDetach=false", "-c", "maintenance.auto=false") +# Local mutators only (they hold the lock fd): never start an fsmonitor daemon under it. +_MUTATOR_CONFIG = ("-c", "core.fsmonitor=false") + +# git subcommands that write the worktree, the index or refs on THIS machine. Only these inherit +# the checkout lock fd: if the updater dies mid-command, the checkout stays locked until git exits. +LOCAL_MUTATORS = frozenset({ + "add", "am", "apply", "checkout", "checkout-index", "cherry-pick", "clean", "commit", "merge", + "mv", "pull", "read-tree", "rebase", "reset", "restore", "revert", "rm", "stash", "switch", + "update-index", "update-ref", "symbolic-ref", "tag", "branch", "worktree", +}) + +# Global options before the subcommand that take a separate value argument. +_GLOBAL_WITH_VALUE = frozenset({"-c", "-C", "--git-dir", "--work-tree", "--namespace", "--exec-path", + "--config-env", "--super-prefix", "--attr-source", "--list-cmds"}) + +_CREATE_SUSPENDED = 0x00000004 + + +def git_subcommand(args: Sequence[str]) -> str | None: + """The subcommand in ``args`` (everything after the git executable), skipping global options.""" + it = iter(args) + for arg in it: + if arg in _GLOBAL_WITH_VALUE: + next(it, None) + elif not arg.startswith("-"): + return arg + return None + + +def is_local_mutator(args: Sequence[str]) -> bool: + return git_subcommand(args) in LOCAL_MUTATORS + + +def git_argv(git_cmd: Sequence[str], args: Sequence[str]) -> list[str]: + """``git_cmd + args`` with the custody config inserted before the subcommand.""" + git_cmd, args = list(git_cmd), list(args) + extra = GIT_NO_DETACH + (_MUTATOR_CONFIG if is_local_mutator(git_cmd[1:] + args) else ()) + return [*git_cmd, *extra, *args] + + +def _held() -> dict | None: + from hermes_cli import update_lock + + return update_lock._HELD + + +def _death_signal_preexec(): + """Linux: a ``preexec_fn`` making the child die (SIGKILL) with the updater thread that waits + on it. For the git children that do NOT hold the lock fd (fetch, ls-remote, readers): a + killed owner must not leave a fetch rewriting refs under the next lock owner. The ruling's + stale-lock rules recover a fetch killed mid-write. ``None`` elsewhere (Windows: the job; + macOS: no parent-death signal — see NOT_COVERED).""" + if not sys.platform.startswith("linux"): + return None + import ctypes + import os + + prctl = ctypes.CDLL(None, use_errno=True).prctl # resolved before fork: the child only calls + parent = os.getpid() + + def _arm(): + prctl(1, 9, 0, 0, 0) # PR_SET_PDEATHSIG, SIGKILL + if os.getppid() != parent: # the owner died between fork and prctl + os._exit(137) + + return _arm + + +def _custody_kwargs(inherit_lock: bool, kwargs: dict) -> dict: + fds = _lock_fds(inherit_lock) + if fds: + kwargs["pass_fds"] = tuple(dict.fromkeys((*kwargs.get("pass_fds", ()), *fds))) + elif not inherit_lock and _held() is not None and "preexec_fn" not in kwargs: + arm = _death_signal_preexec() + if arm is not None: + kwargs["preexec_fn"] = arm + return kwargs + + +def _lock_fds(inherit_lock: bool) -> tuple[int, ...]: + if not inherit_lock or sys.platform == "win32": + return () + from hermes_cli.update_lock import custody_spawn_kwargs + + return tuple(custody_spawn_kwargs().get("pass_fds", ())) + + +def _bind_suspended(proc: subprocess.Popen) -> None: + """Assign a CREATE_SUSPENDED child to the update's kill-on-close job, then resume it. + + Suspended until bound, so nothing it spawns can escape the job. A failed bind still resumes + (the update must not hang on a weaker lock); a failed resume kills the child.""" + import ctypes + + from hermes_cli.update_lock import bind_child_to_update_tree + + try: + bind_child_to_update_tree(proc) + finally: + ntdll = ctypes.WinDLL("ntdll") + ntdll.NtResumeProcess.argtypes = [ctypes.c_void_p] + ntdll.NtResumeProcess.restype = ctypes.c_long + if ntdll.NtResumeProcess(int(proc._handle)) != 0: + proc.kill() + raise OSError(f"could not resume update child {proc.pid}") + + +def popen(argv: Sequence[str], *, inherit_lock: bool = False, **kwargs) -> subprocess.Popen: + """``subprocess.Popen`` in the update tree's custody (see the module doc).""" + if sys.platform == "win32" and _held() is not None: + kwargs["creationflags"] = kwargs.get("creationflags", 0) | _CREATE_SUSPENDED + proc = subprocess.Popen(list(argv), **kwargs) + try: + _bind_suspended(proc) + except BaseException: + with contextlib.suppress(OSError): + proc.kill() + raise + return proc + return subprocess.Popen(list(argv), **_custody_kwargs(inherit_lock, kwargs)) + + +def run(argv: Sequence[str], *, inherit_lock: bool = False, **kwargs) -> subprocess.CompletedProcess: + """``subprocess.run`` in the update tree's custody. Off Windows (or outside an update) it IS + ``subprocess.run`` with the caller's kwargs untouched, plus the lock fd for mutators (or the + parent-death signal for the rest); on Windows inside an update it is the same contract over + :func:`popen`, so the child is job-bound before it runs.""" + if not (sys.platform == "win32" and _held() is not None): + return subprocess.run(list(argv), **_custody_kwargs(inherit_lock, kwargs)) + input, timeout = kwargs.pop("input", None), kwargs.pop("timeout", None) + check = kwargs.pop("check", False) + if input is not None: + kwargs["stdin"] = subprocess.PIPE + if kwargs.pop("capture_output", False): + kwargs["stdout"] = kwargs["stderr"] = subprocess.PIPE + with popen(argv, **kwargs) as proc: + try: + stdout, stderr = proc.communicate(input, timeout=timeout) + except subprocess.TimeoutExpired as exc: + proc.kill() + exc.stdout, exc.stderr = proc.communicate() + raise + except BaseException: + proc.kill() + raise + code = proc.poll() + if check and code: + raise subprocess.CalledProcessError(code, proc.args, output=stdout, stderr=stderr) + return subprocess.CompletedProcess(proc.args, code, stdout, stderr) + + +def run_git(git_cmd: Sequence[str], args: Sequence[str], **kwargs) -> subprocess.CompletedProcess: + """THE updater git runner: custody config in argv, the lock fd only into local mutators, + Windows job binding inside an update. ``kwargs`` are ``subprocess.run``'s.""" + argv = git_argv(git_cmd, args) + return run(argv, inherit_lock=is_local_mutator(argv[1:]), **kwargs) + + +# A stdlib launcher for children whose Popen the updater never sees (``run_contained``): it +# joins the job named by an inherited handle, drops that handle (only the owner's handle may +# keep the job open) and runs the real command with the same stdio. +_JOIN_JOB = ( + "import ctypes, subprocess, sys\n" + "k = ctypes.WinDLL('kernel32', use_last_error=True)\n" + "k.AssignProcessToJobObject.argtypes = [ctypes.c_void_p, ctypes.c_void_p]\n" + "k.GetCurrentProcess.restype = ctypes.c_void_p\n" + "h = ctypes.c_void_p(int(sys.argv[1]))\n" + "if not k.AssignProcessToJobObject(h, k.GetCurrentProcess()):\n" + " sys.exit('could not join the update job: %d' % ctypes.get_last_error())\n" + "k.CloseHandle(h)\n" + "sys.exit(subprocess.call(sys.argv[2:], stdin=subprocess.DEVNULL))\n" +) + + +@contextlib.contextmanager +def contained_command(argv: Sequence[str], *, inherit_lock: bool = True): + """``(argv, kwargs)`` for a checkout writer started by a runner that hides its Popen (the Node + build in ``pm.progress.run_contained``). POSIX: the lock fd. Windows inside an update: the + command runs under a launcher that joins the update's kill-on-close job first.""" + argv = list(argv) + if not (sys.platform == "win32" and _held() is not None): + fds = _lock_fds(inherit_lock) + yield argv, ({"pass_fds": fds} if fds else {}) + return + handle = _inheritable_job_handle() + if handle is None: + yield argv, {} + return + try: + info = subprocess.STARTUPINFO() + info.lpAttributeList = {"handle_list": [handle]} + yield [sys.executable, "-I", "-S", "-c", _JOIN_JOB, str(handle), *argv], {"startupinfo": info} + finally: + import ctypes + + ctypes.WinDLL("kernel32").CloseHandle(ctypes.c_void_p(handle)) + + +def _inheritable_job_handle() -> int | None: + """An inheritable duplicate of the update's kill-on-close job handle (Windows), or None.""" + import ctypes + + from hermes_cli.update_lock import update_tree_job + + try: + job = update_tree_job() + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.GetCurrentProcess.restype = ctypes.c_void_p + kernel32.DuplicateHandle.argtypes = [ctypes.c_void_p, ctypes.c_void_p, ctypes.c_void_p, + ctypes.POINTER(ctypes.c_void_p), ctypes.c_ulong, ctypes.c_int, + ctypes.c_ulong] + me, dup = kernel32.GetCurrentProcess(), ctypes.c_void_p() + # DUPLICATE_SAME_ACCESS, inheritable + if not kernel32.DuplicateHandle(me, job, me, ctypes.byref(dup), 0, True, 0x2): + raise ctypes.WinError(ctypes.get_last_error()) + return int(dup.value) + except OSError as exc: + logger.warning("Could not hand the update's job to a build child: %s", exc) + return None diff --git a/tests/hermes_cli/test_update_custody.py b/tests/hermes_cli/test_update_custody.py new file mode 100644 index 0000000000..57d49fd327 --- /dev/null +++ b/tests/hermes_cli/test_update_custody.py @@ -0,0 +1,166 @@ +"""R2: every updater git runner shares ONE custody policy (hermes_cli.update_custody). + +Real git, real processes, no mocks: +* every runner passes ``-c gc.autoDetach=false -c maintenance.auto=false`` (git reports the + effective config it was started with), so no detached gc/maintenance child is ever forked; +* POSIX: the checkout lock fd reaches only local mutators — a ``git fetch``'s upload-pack (where a + credential-cache daemon would hang) never holds it, a ``git stash push``'s clean filter does. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import textwrap +import time +from pathlib import Path + +import pytest + +from hermes_cli import update_lock as ul + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def _git(cwd: Path, *args: str) -> str: + return subprocess.run(["git", "-C", str(cwd), *args], check=True, capture_output=True, + text=True, encoding="utf-8", env=_env(cwd)).stdout.strip() + + +def _env(base: Path) -> dict: + return dict(os.environ, GIT_CONFIG_NOSYSTEM="1", GIT_CONFIG_GLOBAL=os.devnull, + GIT_AUTHOR_NAME="t", GIT_AUTHOR_EMAIL="t@e", GIT_COMMITTER_NAME="t", GIT_COMMITTER_EMAIL="t@e") + + +@pytest.fixture +def repo(tmp_path, monkeypatch): + for key, value in _env(tmp_path).items(): + monkeypatch.setenv(key, value) + root = tmp_path / "repo" + root.mkdir() + _git(root, "init", "-q", "-b", "main") + (root / "f.txt").write_text("one\n", encoding="utf-8") + _git(root, "add", "f.txt") + _git(root, "commit", "-qm", "one") + return root + + +def _runners(repo: Path): + """Every updater git runner, as ``args -> stdout``.""" + from hermes_cli import gitlock, update_cmd, update_cmd_check, update_cmd_git, update_cmd_stash + + yield "update_cmd._git_run", lambda a: update_cmd._git_run(["git"], a, cwd=repo).stdout + yield "update_cmd_git._git_run", lambda a: update_cmd_git._git_run(["git"], a, cwd=repo).stdout + yield "update_cmd_stash._git_quiet", lambda a: update_cmd_stash._git_quiet(["git"], a, repo, text=True).stdout + yield "update_cmd_check._git", lambda a: update_cmd_check._git(["git"], repo, a).stdout + yield "gitlock._git_stdout_lines", lambda a: "\n".join(gitlock._git_stdout_lines(repo, a)) + + +def test_every_updater_git_runner_forbids_detached_children(repo): + missing = {} + for name, run in _runners(repo): + seen = {key: run(["config", "--get", key]).strip() for key in ("gc.autoDetach", "maintenance.auto")} + if seen != {"gc.autoDetach": "false", "maintenance.auto": "false"}: + missing[name] = seen + assert not missing, f"updater git runners that may fork a detached gc/maintenance child: {missing}" + + +# Records whether the checkout lock file is among this process's open fds, then hands over. +_RECORDER = textwrap.dedent("""\ + #!/bin/sh + lock="$HERMES_TEST_LOCK"; out="$HERMES_TEST_OUT" + held=no + for fd in /proc/$$/fd/*; do + [ "$(readlink "$fd")" = "$lock" ] && held=yes + done + echo "$held" >> "$out" + exec "$@" +""") + + +@pytest.mark.skipif(not Path("/proc/self/fd").is_dir(), reason="needs /proc fd listing") +def test_lock_fd_reaches_local_mutators_only(repo, tmp_path, monkeypatch): + from hermes_cli import update_cmd + + recorder = tmp_path / "recorder.sh" + recorder.write_text(_RECORDER, encoding="utf-8") + recorder.chmod(0o755) + remote = tmp_path / "remote.git" + subprocess.run(["git", "clone", "-q", "--bare", str(repo), str(remote)], check=True, env=_env(tmp_path)) + _git(repo, "remote", "add", "origin", str(remote)) + _git(repo, "config", "filter.rec.clean", f"{recorder} cat") + (repo / ".git" / "info" / "attributes").write_text("f.txt filter=rec\n", encoding="utf-8") + out_fetch, out_stash = tmp_path / "fetch.out", tmp_path / "stash.out" + + lock = ul.UpdateLock(path=tmp_path / "marker", install_root=repo) + assert lock.acquire() + try: + monkeypatch.setenv("HERMES_TEST_LOCK", os.path.realpath(ul.checkout_lock_path(repo))) + monkeypatch.setenv("HERMES_TEST_OUT", str(out_fetch)) + fetched = update_cmd._git_run(["git"], ["fetch", f"--upload-pack={recorder} git-upload-pack", "origin"], + cwd=repo, network=True) + assert fetched.returncode == 0, fetched.stderr + monkeypatch.setenv("HERMES_TEST_OUT", str(out_stash)) + (repo / "f.txt").write_text("two\n", encoding="utf-8") + stashed = update_cmd._git_run(["git"], ["stash", "push", "-m", "custody"], cwd=repo) + assert stashed.returncode == 0, stashed.stderr + finally: + lock.release() + assert out_fetch.read_text(encoding="utf-8-sig").split() == ["no"], "a fetch child (credential/daemon territory) held the lock" + assert "yes" in out_stash.read_text(encoding="utf-8-sig").split(), "a local mutator's child lost the checkout lock" + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="parent-death signal is Linux-only") +def test_network_git_dies_with_its_killed_owner(repo, tmp_path): + """The fd-less fetch must not keep rewriting refs after the owner died (the next owner's lease).""" + script = textwrap.dedent(f""" + import sys + sys.path.insert(0, {str(REPO_ROOT)!r}) + from pathlib import Path + from hermes_cli import update_lock as ul, update_cmd + lock = ul.UpdateLock(path=Path(sys.argv[2]), install_root=sys.argv[1]) + assert lock.acquire() + update_cmd._git_run(["git"], ["fetch", "--upload-pack=" + sys.argv[3], "origin"], cwd=sys.argv[1], network=True) + """) + remote = tmp_path / "remote.git" + subprocess.run(["git", "clone", "-q", "--bare", str(repo), str(remote)], check=True, env=_env(tmp_path)) + _git(repo, "remote", "add", "origin", str(remote)) + started = tmp_path / "upload-pack.pid" + blocker = tmp_path / "block.sh" + blocker.write_text(f"#!/bin/sh\necho $$ > {started}.self\necho $PPID > {started}\nsleep 60\nexec git-upload-pack \"$@\"\n", encoding="utf-8") + blocker.chmod(0o755) + owner = subprocess.Popen([sys.executable, "-c", script, str(repo), str(tmp_path / "marker"), str(blocker)], + env=_env(tmp_path)) + try: + for _ in range(300): + if started.exists() and started.read_text(encoding="utf-8-sig").strip(): + break + time.sleep(0.05) + # git runs the upload-pack command through `sh -c`: the fetch is the shell's parent. + shell = int(started.read_text(encoding="utf-8-sig")) + fetch_pid = next(int(line.split()[1]) for line in Path(f"/proc/{shell}/status").read_text(encoding="utf-8-sig").splitlines() + if line.startswith("PPid:")) + assert "git" in Path(f"/proc/{fetch_pid}/cmdline").read_text(encoding="utf-8-sig", errors="replace") + owner.kill() + owner.wait(timeout=10) + for _ in range(100): + if not Path(f"/proc/{fetch_pid}").exists() or "Z" in _state(fetch_pid): + break + time.sleep(0.05) + assert not Path(f"/proc/{fetch_pid}").exists() or "Z" in _state(fetch_pid), \ + "git fetch outlived its killed update owner" + finally: + if owner.poll() is None: + owner.kill() + blocker_pid = Path(f"{started}.self") + if blocker_pid.exists(): + subprocess.run(["kill", "-9", blocker_pid.read_text(encoding="utf-8-sig").strip()], capture_output=True) + + +def _state(pid: int) -> str: + try: + stat = Path(f"/proc/{pid}/stat").read_text(encoding="utf-8-sig") + except OSError: + return "Z" + return stat[stat.rfind(")") + 2:][:1] diff --git a/tests/hermes_cli/test_update_fetch_failure_classifier.py b/tests/hermes_cli/test_update_fetch_failure_classifier.py index 39395133ea..d1ca74d9f3 100644 --- a/tests/hermes_cli/test_update_fetch_failure_classifier.py +++ b/tests/hermes_cli/test_update_fetch_failure_classifier.py @@ -124,7 +124,7 @@ def test_update_network_git_calls_never_prompt_for_credentials(): def test_update_and_upstream_network_calls_disable_terminal_prompts(monkeypatch, tmp_path): - """Exercise origin fetch and fork fetch/pull/push, not their source spelling.""" + """Exercise origin fetch and fork fetch/merge/push, not their source spelling.""" import subprocess from hermes_cli import update_cmd_git @@ -146,7 +146,11 @@ def test_update_and_upstream_network_calls_disable_terminal_prompts(monkeypatch, monkeypatch.setattr(subprocess, "run", run) update_cmd._git_run(["git"], ["fetch", "origin", "main"], cwd=tmp_path, network=True, check=True) assert update_cmd_git._sync_with_upstream_if_needed(["git"], tmp_path, assume_yes=True) - assert [args[0] for args, _ in calls] == ["fetch", "fetch", "pull", "push"] + from hermes_cli.update_custody import git_subcommand + + # The fork's fast-forward is local (`merge --ff-only upstream/main` after the fetch): no + # credential helper ever runs under the checkout lock fd a mutator inherits (R2). + assert [git_subcommand(args) for args, _ in calls] == ["fetch", "fetch", "merge", "push"] for args, kwargs in calls: assert kwargs["stdin"] is subprocess.DEVNULL, args env = kwargs["env"] diff --git a/tests/hermes_cli/test_update_fleet_checkout_build_stamp.py b/tests/hermes_cli/test_update_fleet_checkout_build_stamp.py index 4eb60f8b7a..3c52ebf388 100644 --- a/tests/hermes_cli/test_update_fleet_checkout_build_stamp.py +++ b/tests/hermes_cli/test_update_fleet_checkout_build_stamp.py @@ -33,7 +33,10 @@ def test_git_checkout_still_walks_ancestry(monkeypatch): monkeypatch.setattr(chk.subprocess, "run", lambda cmd, **k: calls.append(cmd) or _R()) assert chk.checkout_contains("cafebabe" * 5) is True - assert calls and calls[0][:3] == ["git", "merge-base", "--is-ancestor"] + from hermes_cli.update_custody import git_subcommand + + assert calls and calls[0][0] == "git" and git_subcommand(calls[0][1:]) == "merge-base" + assert calls[0][-3:] == ["--is-ancestor", "cafebabe" * 5, "HEAD"] def test_partial_clone_never_downloads_history_for_a_sha_it_lacks(monkeypatch, tmp_path): diff --git a/tests/hermes_cli/test_update_lock_windows_live.py b/tests/hermes_cli/test_update_lock_windows_live.py index 42cc52654d..bce79602d1 100644 --- a/tests/hermes_cli/test_update_lock_windows_live.py +++ b/tests/hermes_cli/test_update_lock_windows_live.py @@ -7,6 +7,8 @@ into a kill-on-close job: killing the owner (taskkill /F) kills the child and fr from __future__ import annotations +import contextlib +import os import subprocess import sys import time @@ -66,3 +68,107 @@ def test_killed_owner_takes_its_tree_down_and_frees_the_lock(tmp_path): finally: if owner.poll() is None: owner.kill() + + +# --- R2 on Windows: the updater's git and Node children join the owner's kill-on-close job ----- + +_GIT_OWNER = r""" +import sys +from pathlib import Path +sys.path.insert(0, sys.argv[1]) +from hermes_cli.update_lock import UpdateLock +from hermes_cli.update_cmd import _git_run +root = Path(sys.argv[2]) +assert UpdateLock(path=Path(sys.argv[3]), install_root=root).acquire() +_git_run(["git"], ["stash", "push", "-m", "custody"], cwd=root) +""" + +_BUILD_OWNER = r""" +import os, sys +from pathlib import Path +sys.path.insert(0, sys.argv[1]) +from hermes_cli.update_lock import UpdateLock +from hermes_cli.source_build import run_source_script +root = Path(sys.argv[2]) +assert UpdateLock(path=Path(sys.argv[3]), install_root=root).acquire() +env = {**os.environ, "PATH": sys.argv[4] + os.pathsep + os.environ["PATH"]} +run_source_script(root, "build.mjs", env=env, label="probe build") +""" + + +def _blocker(tmp_path: Path) -> str: + """Python source that records its pid and blocks until ``go`` exists.""" + pid_file, go = (tmp_path / "blocker.pid").as_posix(), (tmp_path / "go").as_posix() + return (f"import os, time\nopen({pid_file!r}, 'w').write(str(os.getpid()))\n" + f"while not os.path.exists({go!r}): time.sleep(0.05)\nprint('cleaned')\n") + + +def _git(root: Path, *args: str) -> None: + subprocess.run(["git", "-C", str(root), *args], check=True, capture_output=True, + env={**os.environ, "GIT_CONFIG_NOSYSTEM": "1"}) + + +def _tree_dies_with_its_owner(tmp_path: Path, install: Path, owner_args: list[str]) -> None: + import psutil + + owner = subprocess.Popen([sys.executable, "-c", *owner_args], stdin=subprocess.DEVNULL) + pid_file, tree = tmp_path / "blocker.pid", [] + try: + deadline = time.time() + 60 + while not (pid_file.exists() and pid_file.read_text(encoding="utf-8-sig").strip()): + assert owner.poll() is None, f"owner exited {owner.returncode} before its child blocked" + assert time.time() < deadline, "the blocking child never started" + time.sleep(0.1) + blocker = psutil.Process(int(pid_file.read_text(encoding="utf-8-sig"))) + tree = [blocker, *(p for p in blocker.parents() if p.pid != owner.pid and owner.pid in + {q.pid for q in p.parents()})] + assert UpdateLock(path=tmp_path / "other-marker", install_root=install).acquire() is False + + subprocess.run(["taskkill", "/F", "/PID", str(owner.pid)], capture_output=True, check=False) + owner.wait(timeout=30) + deadline = time.time() + 15 + while any(p.is_running() for p in tree) and time.time() < deadline: + time.sleep(0.2) + survivors = [f"{p.pid}:{p.name()}" for p in tree if p.is_running()] + assert not survivors, f"update children outlived their killed owner: {survivors}" + fresh = UpdateLock(path=tmp_path / "other-marker", install_root=install) + assert fresh.acquire() is True, "the checkout stayed locked after the whole tree died" + fresh.release() + finally: + (tmp_path / "go").touch() + for proc in tree: + with contextlib.suppress(Exception): + proc.kill() + if owner.poll() is None: + owner.kill() + + +def test_killed_owner_takes_its_git_child_down(tmp_path): + exe = Path(sys.executable).as_posix() + if any(ch in exe for ch in " =~%#'\"&;|<>()$`*?["): + pytest.skip("the clean filter must run without a shell: interpreter path has shell metacharacters") + install = tmp_path / "checkout" + install.mkdir() + _git(install, "init", "-q") + _git(install, "config", "user.name", "probe") + _git(install, "config", "user.email", "probe@invalid.local") + (install / "f.txt").write_text("before\n", encoding="utf-8") + _git(install, "add", "f.txt") + _git(install, "commit", "-qm", "before") + # The clean filter is the bare interpreter: it runs the file's content (stdin) as a program. + _git(install, "config", "filter.block.clean", exe) + (install / ".git" / "info" / "attributes").write_text("f.txt filter=block\n", encoding="utf-8") + (install / "f.txt").write_text(_blocker(tmp_path), encoding="utf-8") + _tree_dies_with_its_owner(tmp_path, install, [_GIT_OWNER, str(REPO_ROOT), str(install), str(tmp_path / "m")]) + + +def test_killed_owner_takes_its_node_build_down(tmp_path): + install = tmp_path / "checkout" + install.mkdir() + (install / "build.py").write_text(_blocker(tmp_path), encoding="utf-8") + (install / "build.mjs").write_text("// stands in for a build script\n", encoding="utf-8") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + (bin_dir / "node.bat").write_text(f'@"{sys.executable}" "{install / "build.py"}"\r\n', encoding="utf-8") + _tree_dies_with_its_owner(tmp_path, install, + [_BUILD_OWNER, str(REPO_ROOT), str(install), str(tmp_path / "m"), str(bin_dir)]) diff --git a/tests/hermes_cli/test_update_target_identity.py b/tests/hermes_cli/test_update_target_identity.py index 31a885cc84..2533f6658a 100644 --- a/tests/hermes_cli/test_update_target_identity.py +++ b/tests/hermes_cli/test_update_target_identity.py @@ -126,6 +126,14 @@ def test_branch_update_uses_real_refs_and_completion_request(update_tree, monkey def fault(command, *args, **kwargs): assert Path(command[0]).name.lower() in {'git', 'git.exe'} or command[0] == sys.executable, command assert Path(kwargs['cwd']).resolve() in {t.clone, t.origin}, command + if 'refs/remotes/upstream/main' in command: # the fork sync's local fast-forward + result = run(command, *args, **kwargs) + if case.startswith('fork-late'): + if case.endswith('wrong-branch'): + run(['git', 'checkout', '-qb', 'wrong'], cwd=t.clone, check=True, capture_output=True) + if case.endswith('reverted'): + run(['git', 'reset', '--hard', t.base], cwd=t.clone, check=True, capture_output=True) + return result if 'merge' in command and '--ff-only' in command: if case == 'no-move': return subprocess.CompletedProcess(command, 0, stdout='', stderr='') @@ -136,11 +144,6 @@ def test_branch_update_uses_real_refs_and_completion_request(update_tree, monkey run(['git', 'reset', '--hard', t.base], cwd=t.clone, check=True, capture_output=True) return result result = run(command, *args, **kwargs) - if 'pull' in command and case.startswith('fork-late'): - if case.endswith('wrong-branch'): - run(['git', 'checkout', '-qb', 'wrong'], cwd=t.clone, check=True, capture_output=True) - if case.endswith('reverted'): - run(['git', 'reset', '--hard', t.base], cwd=t.clone, check=True, capture_output=True) if 'push' in command and 'origin' in command: pushes.append(result.returncode) return result