fix(updater): a probe that cannot be spawned stays advisory instead of failing the update

bounded_probe_run collapses spawn failure and timeout into one None, so a venv
interpreter that exists but cannot be executed (PermissionError, ENOEXEC, fork
failure) was reported as "timed out before reporting import health" -- a bogus
warning on the git path and sys.exit(1) on the ZIP path -- and the
`except OSError: return {}` branch beneath it was dead. Let the Popen error
propagate (opt-in flag, default unchanged for the other probes) so "we could not
run our own probe" says nothing about the checkout and no longer blocks the
update, while a spawned child that hangs is still a verdict.

Restores the non-fatal test the earlier rewrite dropped, now driving the real
bounded_probe_run/Popen: red on the previous head, green here.

Review finding: spawn failure of the import probe reported as a fatal timeout.
This commit is contained in:
teknium1
2026-09-14 21:04:14 -07:00
committed by Teknium
parent 7020a0081a
commit 37e4cd5faa
3 changed files with 22 additions and 4 deletions
+6 -1
View File
@@ -498,11 +498,14 @@ def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None:
def bounded_probe_run(
argv: Sequence[str], *, timeout: float, errors: str = "replace",
env: "Mapping[str, str] | None" = None, cwd: "str | os.PathLike[str] | None" = None,
raise_on_spawn_failure: bool = False,
) -> "subprocess.CompletedProcess[str] | None":
"""Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=…)`` for fail-open probes.
Returns a ``CompletedProcess`` when the child finished within *timeout* (any exit code), or
``None`` on spawn failure or timeout.
``None`` on spawn failure or timeout. With ``raise_on_spawn_failure=True`` the ``Popen``
exception propagates instead, so callers that treat a *timeout* as a verdict can still tell
"our own probe never started" apart from "the child hung".
Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup calls an *unbounded*
``communicate()`` after killing the direct child. Killing it can leave a descendant (``git.exe`` under a
@@ -518,6 +521,8 @@ def bounded_probe_run(
text=True, encoding="utf-8", errors=errors,
env=dict(env) if env is not None else None, cwd=cwd, **_popen_kwargs)
except Exception:
if raise_on_spawn_failure:
raise
return None
try:
stdout, stderr = proc.communicate(timeout=timeout)
+4 -3
View File
@@ -89,11 +89,12 @@ def _critical_module_import_failures(
if venv_python.exists():
interpreter = str(venv_python)
result = bounded_probe_run(
[interpreter, "-c", probe], timeout=120, cwd=str(root),
[interpreter, "-c", probe], timeout=120, cwd=str(root), raise_on_spawn_failure=True,
)
except (OSError, subprocess.SubprocessError):
# Keep this guard advisory: a broken probe runner cannot make an otherwise
# successful update fail.
# Keep this guard advisory: a probe we could not even spawn (unreadable venv
# interpreter, fork failure) says nothing about the checkout, so it must not
# fail an otherwise successful update. A spawned child that hangs does.
return {}
if result is None:
return _probe_failure("TimeoutExpired", "timed out before reporting import health")
@@ -16,6 +16,7 @@ and the syntax guard reported the update as successful.
from __future__ import annotations
import sys
from pathlib import Path
import pytest
@@ -249,6 +250,17 @@ def test_import_guard_reports_bounded_probe_failure(monkeypatch, tmp_path):
)
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX exec-bit PermissionError")
def test_import_guard_is_non_fatal_when_probe_cannot_run(tmp_path):
"""A venv interpreter that exists but cannot be executed must not read as a hung probe:
spawn failure stays advisory (real ``bounded_probe_run``, real ``Popen``)."""
venv_python = tmp_path / "venv" / "bin" / "python"
venv_python.parent.mkdir(parents=True)
venv_python.write_text("#!/bin/sh\nexit 0\n")
venv_python.chmod(0o644) # present, not executable -> Popen raises PermissionError
assert update_cmd._validate_critical_modules_import(tmp_path) == (True, None, None)
# ---------------------------------------------------------------------------
# partial_update_hint
# ---------------------------------------------------------------------------