fix(macos): harden anchor alias failures — warn, unique staging, marker-last
Follow-up to the #95605 salvage, closing the review findings: - _copy_alias no longer swallows OSError silently: it warns (a leftover alias symlink is the exact #95541 crash shape) and reports failure. - Alias staging uses mkstemp (unique names) so concurrent ensures (update + doctor --fix) can never promote a truncated interim copy. - The anchor marker is written LAST and atomically (write-then-rename): it now asserts the whole layout (anchor + aliases) is complete, so a partially-materialized alias set can never read 'active' in doctor — the next ensure retries the install instead. - /.hermes-runtime/python/ store marker is derived from managed_uv._RUNTIME_DIR_NAME instead of a hardcoded string. 5 new regression tests.
This commit is contained in:
kshitijk4poor
@@ -29,8 +29,9 @@ stays on the stable venv path) and closes both holes:
|
||||
points at ``@executable_path/../lib`` — no rewrite.
|
||||
3. A pre-install boot gate actually launches the staged copy and demands
|
||||
``import encodings`` plus ``sys.prefix == <venv>``. Failure rolls the
|
||||
staging file back and leaves the live venv untouched, so a bad anchor
|
||||
can never brick update/doctor again.
|
||||
staging file back and leaves the live interpreter untouched (a surplus
|
||||
provisioned dylib in ``venv/lib/`` may remain — harmless), so a bad
|
||||
anchor can never brick update/doctor again.
|
||||
|
||||
All functions are no-ops on non-macOS and for interpreters that are not
|
||||
uv-managed. Best-effort: never raises to callers.
|
||||
@@ -49,13 +50,16 @@ import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
from hermes_constants import venv_python_path
|
||||
from hermes_cli.managed_uv import _RUNTIME_DIR_NAME
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
_MARKER_NAME = ".tcc-anchor-source"
|
||||
|
||||
_STORE_COMMON_MARKERS = ("cpython-", "-macos-")
|
||||
_STORE_ROOT_MARKERS = ("/uv/python/", "/.hermes-runtime/python/")
|
||||
# The runtime-store marker is derived from managed_uv so a rename of the
|
||||
# repair-generation directory cannot silently stop the anchor from matching.
|
||||
_STORE_ROOT_MARKERS = ("/uv/python/", f"/{_RUNTIME_DIR_NAME}/python/")
|
||||
|
||||
|
||||
class _BootGateFailed(Exception):
|
||||
@@ -159,6 +163,26 @@ def _anchor_marker(venv_bin: Path) -> Path:
|
||||
return venv_bin / _MARKER_NAME
|
||||
|
||||
|
||||
def _write_marker(venv_bin: Path, source_file: Path) -> None:
|
||||
"""Write the anchor marker atomically (write-then-rename).
|
||||
|
||||
A concurrent ensure (update + doctor --fix) must never observe a
|
||||
partially-written marker: a torn read would compare unequal and trigger
|
||||
a spurious reinstall, and ``write_text`` alone is not atomic.
|
||||
"""
|
||||
fd, tmp_name = tempfile.mkstemp(prefix=f"{_MARKER_NAME}.", dir=str(venv_bin))
|
||||
try:
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as fh:
|
||||
fh.write(_marker_value(source_file))
|
||||
os.replace(tmp_name, _anchor_marker(venv_bin))
|
||||
except OSError:
|
||||
try:
|
||||
os.unlink(tmp_name)
|
||||
except OSError:
|
||||
pass
|
||||
raise
|
||||
|
||||
|
||||
def _store_root(source_file: Path) -> Path:
|
||||
# .../cpython-<ver>-macos-*/bin/python3.N → store root
|
||||
return source_file.resolve(strict=False).parent.parent
|
||||
@@ -200,24 +224,41 @@ def _provision_libpython(
|
||||
logger.debug("libpython provision skipped", exc_info=True)
|
||||
|
||||
|
||||
def _copy_alias(venv_bin: Path, name: str, anchor: Path) -> None:
|
||||
"""Materialize *name* as a real-file copy of *anchor* (atomic rename)."""
|
||||
tmp = venv_bin / f".{name}.tcc-tmp"
|
||||
def _copy_alias(venv_bin: Path, name: str, anchor: Path) -> bool:
|
||||
"""Materialize *name* as a real-file copy of *anchor* (atomic rename).
|
||||
|
||||
Returns False (and warns) on failure: a leftover alias *symlink* to the
|
||||
anchor is the exact #95541 crash shape, so callers must know when the
|
||||
alias set is incomplete. The staging name is unique (mkstemp) so a
|
||||
concurrent ensure (update + doctor --fix) cannot promote a truncated
|
||||
interim copy.
|
||||
"""
|
||||
tmp_path: Path | None = None
|
||||
try:
|
||||
shutil.copy2(anchor, tmp)
|
||||
os.chmod(tmp, anchor.stat().st_mode | 0o111)
|
||||
os.replace(tmp, venv_bin / name)
|
||||
except OSError:
|
||||
try:
|
||||
tmp.unlink(missing_ok=True)
|
||||
except OSError:
|
||||
pass
|
||||
fd, tmp_name = tempfile.mkstemp(prefix=f".{name}.tcc-", dir=str(venv_bin))
|
||||
os.close(fd)
|
||||
tmp_path = Path(tmp_name)
|
||||
shutil.copy2(anchor, tmp_path)
|
||||
os.chmod(tmp_path, anchor.stat().st_mode | 0o111)
|
||||
os.replace(tmp_path, venv_bin / name)
|
||||
return True
|
||||
except OSError as exc:
|
||||
logger.warning("TCC anchor alias %s not materialized: %s", name, exc)
|
||||
if tmp_path is not None:
|
||||
try:
|
||||
tmp_path.unlink(missing_ok=True)
|
||||
except OSError:
|
||||
pass
|
||||
return False
|
||||
|
||||
|
||||
def _materialize_aliases(
|
||||
venv_bin: Path, anchor: Path, *, refresh: bool = False
|
||||
) -> None:
|
||||
"""Materialize uv alias names as real-file copies of the anchor."""
|
||||
) -> bool:
|
||||
"""Materialize uv alias names as real-file copies of the anchor.
|
||||
|
||||
Returns True only when every alias that needed materializing succeeded.
|
||||
"""
|
||||
names = set(_sibling_names())
|
||||
try:
|
||||
names.update(
|
||||
@@ -227,13 +268,16 @@ def _materialize_aliases(
|
||||
)
|
||||
except OSError:
|
||||
pass
|
||||
ok = True
|
||||
for name in sorted(names):
|
||||
alias = venv_bin / name
|
||||
try:
|
||||
if refresh or alias.is_symlink() or not alias.exists():
|
||||
_copy_alias(venv_bin, name, anchor)
|
||||
ok = _copy_alias(venv_bin, name, anchor) and ok
|
||||
except OSError:
|
||||
ok = False
|
||||
continue
|
||||
return ok
|
||||
|
||||
|
||||
def _passes_boot_gate(staged: Path, venv_dir: Path) -> bool:
|
||||
@@ -305,8 +349,19 @@ def _install_anchor(venv_dir: Path, source_file: Path) -> None:
|
||||
f"staged copy at {tmp_path} failed encodings/prefix probe"
|
||||
)
|
||||
os.replace(tmp_path, venv_py)
|
||||
_anchor_marker(venv_bin).write_text(_marker_value(source_file), encoding="utf-8")
|
||||
_materialize_aliases(venv_bin, venv_py, refresh=True)
|
||||
aliases_ok = _materialize_aliases(venv_bin, venv_py, refresh=True)
|
||||
if aliases_ok:
|
||||
# Marker last, atomically: it asserts the WHOLE layout (anchor +
|
||||
# aliases) is complete. A partially-materialized alias set (the
|
||||
# #95541 crash shape when an alias stays a symlink) must not read
|
||||
# "active" in doctor — leaving the marker absent makes the next
|
||||
# ensure retry the install.
|
||||
_write_marker(venv_bin, source_file)
|
||||
else:
|
||||
logger.warning(
|
||||
"TCC anchor installed but alias materialization was "
|
||||
"incomplete; leaving anchor unmarked so the next run retries"
|
||||
)
|
||||
except Exception:
|
||||
try:
|
||||
tmp_path.unlink(missing_ok=True)
|
||||
|
||||
@@ -297,6 +297,75 @@ class TestEnsureTccAnchor:
|
||||
assert venv_py.is_symlink()
|
||||
assert not (venv_py.parent / ".tcc-anchor-source").exists()
|
||||
|
||||
def test_alias_failure_leaves_anchor_unmarked(self, tmp_path, monkeypatch, caplog):
|
||||
# If an alias copy fails (e.g. ENOSPC), the marker must NOT be
|
||||
# written: a symlink alias to the anchored copy is the #95541 crash
|
||||
# shape, and a marker would make doctor report "active" over it.
|
||||
# The next ensure retries the whole install.
|
||||
_darwin(monkeypatch)
|
||||
store_bin = _build_store(tmp_path)
|
||||
root = _build_checkout(tmp_path, store_bin=store_bin)
|
||||
venv_py = venv_python_path(root / ".venv")
|
||||
monkeypatch.setattr(tcc, "_copy_alias", lambda *a, **k: False)
|
||||
|
||||
import logging
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger=tcc.__name__):
|
||||
tcc.ensure_tcc_anchor(root)
|
||||
|
||||
assert not (venv_py.parent / ".tcc-anchor-source").exists()
|
||||
status, _ = tcc.tcc_anchor_state(root)
|
||||
assert status != "active"
|
||||
assert any("alias" in r.message for r in caplog.records)
|
||||
|
||||
# Recovery: with alias copies working again the retry completes.
|
||||
monkeypatch.undo()
|
||||
monkeypatch.setattr(tcc.platform, "system", lambda: "Darwin")
|
||||
assert tcc.ensure_tcc_anchor(root) is not None
|
||||
assert tcc.tcc_anchor_state(root)[0] == "active"
|
||||
|
||||
def test_copy_alias_failure_warns_and_cleans_staging(self, tmp_path, monkeypatch, caplog):
|
||||
# _copy_alias must log the failure (silent skips hid the #95541
|
||||
# shape) and never leave a staging file behind.
|
||||
anchor = tmp_path / "anchor"
|
||||
anchor.write_bytes(b"#!anchor")
|
||||
anchor.chmod(0o755)
|
||||
|
||||
def boom(src, dst, **kw):
|
||||
raise OSError(28, "No space left on device")
|
||||
|
||||
monkeypatch.setattr(tcc.shutil, "copy2", boom)
|
||||
|
||||
import logging
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger=tcc.__name__):
|
||||
assert tcc._copy_alias(tmp_path, "python3", anchor) is False
|
||||
|
||||
assert any("python3" in r.message for r in caplog.records)
|
||||
assert not list(tmp_path.glob(".python3.tcc-*"))
|
||||
|
||||
def test_marker_written_atomically(self, tmp_path):
|
||||
# Marker goes through write-then-rename; no torn intermediate name
|
||||
# survives and the content matches the resolved source path.
|
||||
source = tmp_path / "store" / "python3.11"
|
||||
source.parent.mkdir(parents=True)
|
||||
source.write_bytes(b"#!store")
|
||||
venv_bin = tmp_path / "bin"
|
||||
venv_bin.mkdir()
|
||||
|
||||
tcc._write_marker(venv_bin, source)
|
||||
|
||||
marker = venv_bin / ".tcc-anchor-source"
|
||||
assert marker.read_text(encoding="utf-8") == tcc._marker_value(source)
|
||||
assert not list(venv_bin.glob(".tcc-anchor-source.*"))
|
||||
|
||||
def test_store_root_marker_tracks_managed_uv_constant(self):
|
||||
# The repair-generation store marker must stay derived from
|
||||
# managed_uv's directory constant, not drift as a hardcoded string.
|
||||
from hermes_cli.managed_uv import _RUNTIME_DIR_NAME
|
||||
|
||||
assert f"/{_RUNTIME_DIR_NAME}/python/" in tcc._STORE_ROOT_MARKERS
|
||||
|
||||
|
||||
class TestBootGate:
|
||||
"""Direct branch coverage for _passes_boot_gate.
|
||||
|
||||
Reference in New Issue
Block a user