From 8c3a35b69d820beb349637c36e7aa96d9c092998 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:45:36 +0530 Subject: [PATCH] fix(gateway-windows): bind the attestation to process birth, not the ledger claim Review follow-ups on the identity binding: - The sentinel's `start_time` is `time.time()` at `record_startup`, seconds after the process was born once imports finish, so comparing it with psutil's create_time within 2 s would have read every real gateway as undecidable and silently stopped the #109538 cold-start. `record_startup` now stamps `create_time` (psutil birth via the existing `process_identity._process_create_time`), `mark_exited` carries it, and the attestation compares birth to birth. A sentinel from a gateway older than the stamp falls back to the PID-only rule. - A resume token written by pre-generation code and resumed by this code probes the marker again instead of skipping the spawn. - Horizon allows a 60 s backwards clock step; the unused `now` parameter is gone; the create-time tolerance is a named constant; the read-then-unlink in `_consume_start_attestation` is documented as best-effort. --- gateway/lifecycle_ledger.py | 12 +++++++-- hermes_cli/gateway_windows.py | 18 ++++++++----- hermes_cli/update_cmd_windows.py | 7 ++++- tests/gateway/test_lifecycle_ledger.py | 11 +++++--- .../test_gateway_start_attestation.py | 26 +++++++++++-------- ...ws_gateway_cold_start_desktop_lifecycle.py | 5 +++- 6 files changed, 54 insertions(+), 25 deletions(-) diff --git a/gateway/lifecycle_ledger.py b/gateway/lifecycle_ledger.py index 33d80c4a21..47f2bbbbbc 100644 --- a/gateway/lifecycle_ledger.py +++ b/gateway/lifecycle_ledger.py @@ -214,6 +214,13 @@ def record_startup(home: Optional[Path] = None) -> Optional[Dict[str, Any]]: logger.debug("Unclean-exit detection failed", exc_info=True) try: claim: Dict[str, Any] = {"phase": "running", "pid": os.getpid(), "start_time": time.time(), "started_at": _now_iso()} + # Process birth (psutil), distinct from ``start_time`` (the ledger claim, seconds later once + # imports finish): the Windows start attestation binds PIDs to birth time (#110020 review). + from hermes_cli.process_identity import _process_create_time + + create_time = _process_create_time(os.getpid()) + if create_time is not None: + claim["create_time"] = create_time # Carry the verdict on the PREVIOUS life on the new sentinel: it is the only # machine-readable copy (/api/status reads it to report an OOM restart). # Scoped to this life — the next clean exit or boot rewrites the sentinel. @@ -242,8 +249,9 @@ def mark_exited(exit_code: Optional[int] = None, reason: str = "graceful_shutdow "exited_at": _now_iso()} # Carry the incarnation identity: the Windows start attestation matches a clean exit by # PID *and* start time so a reused PID's exit cannot vouch for a different life (#110020). - if sentinel is not None and sentinel.get("start_time") is not None: - exited["start_time"] = sentinel["start_time"] + for key in ("start_time", "create_time"): + if sentinel is not None and sentinel.get(key) is not None: + exited[key] = sentinel[key] _write_sentinel(exited, home) except Exception: logger.debug("Failed to mark lifecycle sentinel exited", exc_info=True) diff --git a/hermes_cli/gateway_windows.py b/hermes_cli/gateway_windows.py index fcc2c51262..5d11faa04a 100644 --- a/hermes_cli/gateway_windows.py +++ b/hermes_cli/gateway_windows.py @@ -907,9 +907,13 @@ def _clear_start_attestation() -> None: # meant to bridge the seconds between a ✓ and the next ``hermes gateway status``/``update``; a # historical marker must never later override Desktop ownership into a duplicate gateway (#76129). START_ATTESTATION_MAX_AGE_S = 24 * 3600 +# Same slack process_identity uses for psutil create_time comparisons (PID reuse disambiguation). +_CREATE_TIME_TOLERANCE_S = 2.0 +# A backwards clock step (NTP) between write and read must not kill a fresh marker. +_ATTESTATION_CLOCK_SLACK_S = 60.0 -def _attestation_within_horizon(data: object, now: float | None = None) -> bool: +def _attestation_within_horizon(data: object) -> bool: """False for a marker whose ``ts`` is missing, unparsable or older than the horizon (fail closed).""" try: ts = datetime.fromisoformat(str(data["ts"])) if isinstance(data, dict) else None @@ -917,8 +921,8 @@ def _attestation_within_horizon(data: object, now: float | None = None) -> bool: return False if ts.tzinfo is None: ts = ts.replace(tzinfo=timezone.utc) - age = (time.time() if now is None else now) - ts.timestamp() - return 0 <= age <= START_ATTESTATION_MAX_AGE_S + age = time.time() - ts.timestamp() + return -_ATTESTATION_CLOCK_SLACK_S <= age <= START_ATTESTATION_MAX_AGE_S except Exception: return False @@ -931,6 +935,7 @@ def _attestation_generation(data: object) -> str | None: def _consume_start_attestation(generation: str) -> None: """Clear the marker only while it is still the ``generation`` that was acted on; a newer marker belongs to a gateway start this caller knows nothing about and keeps its own report.""" + # Best-effort read-then-unlink: a marker written in between loses one post-start report, never authority. if _attestation_generation(_read_start_attestation()) == generation: _clear_start_attestation() @@ -980,10 +985,11 @@ def _attested_pid_exited_cleanly(pid: int, create_time: float | None = None) -> if not isinstance(data, dict): return create_time is not None if create_time is not None: - start_time = data.get("start_time") - if data.get("pid") != pid or type(start_time) not in (int, float): + if data.get("pid") != pid: return True - if abs(float(start_time) - create_time) > 2.0: + sentinel_birth = data.get("create_time") + # A sentinel from a gateway older than the identity stamp cannot be told apart: PID-only rule. + if type(sentinel_birth) in (int, float) and abs(float(sentinel_birth) - create_time) > _CREATE_TIME_TOLERANCE_S: return True return data.get("phase") == "exited" and data.get("pid") == pid diff --git a/hermes_cli/update_cmd_windows.py b/hermes_cli/update_cmd_windows.py index 115ab4fe64..d1c190a404 100644 --- a/hermes_cli/update_cmd_windows.py +++ b/hermes_cli/update_cmd_windows.py @@ -950,7 +950,12 @@ def _cold_start_windows_gateway_after_update(token: dict | None = None) -> bool: with _abort_on_error("Could not re-check gateway liveness before cold-start"): if list(find_gateway_pids(all_profiles=True)): return True - generation = (token or {}).get("attested_generation") + token = token or {} + generation = token.get("attested_generation") + if generation is None and "attested_generation" not in token: + # Token written by pre-generation code and resumed across this very update: probe the marker. + with _abort_on_error("Could not re-read the start attestation before cold-start"): + generation = gateway_windows.attested_death_generation(current_pids=[]) with _abort_on_error("Could not re-check Desktop gateway-lifecycle ownership before cold-start"): if _desktop_owns_gateway_lifecycle() and not generation: logger.debug("Skipping Windows gateway cold-start: Desktop owns gateway lifecycle") diff --git a/tests/gateway/test_lifecycle_ledger.py b/tests/gateway/test_lifecycle_ledger.py index c824b33a78..d3e39a1497 100644 --- a/tests/gateway/test_lifecycle_ledger.py +++ b/tests/gateway/test_lifecycle_ledger.py @@ -216,12 +216,15 @@ def test_prior_exit_label_survives_corrupt_sentinel(tmp_path: Path) -> None: assert read_prior_exit_label(tmp_path) == "unknown" -def test_mark_exited_carries_start_time_onto_exited_sentinel(tmp_path: Path) -> None: - """The exited sentinel keeps the life's ``start_time`` so the Windows start attestation can - match a clean exit by incarnation, not by reusable PID (#110020).""" +def test_sentinel_carries_process_birth_through_exit(tmp_path: Path, monkeypatch) -> None: + """The running sentinel stamps the process ``create_time`` (psutil birth, not the later ledger + claim) and the exited sentinel keeps it, so the Windows start attestation can match a clean + exit by incarnation, not by reusable PID (#110020).""" + monkeypatch.setattr("hermes_cli.process_identity._process_create_time", lambda pid=None: 1234.5) record_startup(home=tmp_path) running = json.loads(get_lifecycle_sentinel_path(tmp_path).read_text(encoding="utf-8")) + assert running["create_time"] == 1234.5 mark_exited(0, reason="graceful_shutdown", home=tmp_path) exited = json.loads(get_lifecycle_sentinel_path(tmp_path).read_text(encoding="utf-8")) assert exited["phase"] == "exited" - assert exited["start_time"] == running["start_time"] + assert (exited["start_time"], exited["create_time"]) == (running["start_time"], 1234.5) diff --git a/tests/hermes_cli/test_gateway_start_attestation.py b/tests/hermes_cli/test_gateway_start_attestation.py index 15c6bd60a7..6a13884398 100644 --- a/tests/hermes_cli/test_gateway_start_attestation.py +++ b/tests/hermes_cli/test_gateway_start_attestation.py @@ -254,17 +254,17 @@ def _sentinel(attest_home, **fields): def test_attestation_bound_to_create_time_is_no_authority_once_the_sentinel_moved_on(monkeypatch, attest_home): """#110020 review (gateway_windows.py:937): the sentinel used to be matched by numeric PID only, so a stale marker for PID 111 flipped from clean to crash once an unrelated PID 222 lifecycle overwrote - the sentinel. A marker bound to 111's create time fails closed: another PID, another start time, - or a sentinel without a start time all read as undecidable, never as dead.""" + the sentinel. A marker bound to 111's process birth fails closed: another PID or another birth + time reads as undecidable, never as dead. (Birth, not the ledger's ``start_time``: that is stamped + seconds later, once imports finish.)""" monkeypatch.setattr("hermes_cli.process_identity._process_create_time", lambda pid=None: 1000.0) gateway_windows._write_start_attestation([111], "direct spawn (PID 111)") marker = json.loads((attest_home / "state" / "gateway.start-attestation.json").read_text(encoding="utf-8")) assert marker["create_times"] == {"111": 1000.0} for fields in ( - {"phase": "exited", "pid": 222, "start_time": 5000.0}, # unrelated lifecycle overwrote it - {"phase": "running", "pid": 222, "start_time": 5000.0}, - {"phase": "running", "pid": 111, "start_time": 1003.0}, # PID reuse: different incarnation - {"phase": "running", "pid": 111}, # sentinel carries no identity + {"phase": "exited", "pid": 222, "create_time": 5000.0}, # unrelated lifecycle overwrote it + {"phase": "running", "pid": 222, "create_time": 5000.0}, + {"phase": "running", "pid": 111, "create_time": 1003.0}, # PID reuse: different incarnation ): _sentinel(attest_home, **fields) assert gateway_windows.attested_death_generation(current_pids=[]) is None, fields @@ -274,14 +274,18 @@ def test_attestation_bound_to_create_time_is_no_authority_once_the_sentinel_move def test_attestation_bound_to_create_time_keeps_authority_for_its_own_incarnation(monkeypatch, attest_home): """A running sentinel for the same PID within 2s of the bound create time is the attested - incarnation: gone with no clean exit → dead. Its own clean exit (start_time carried by - ``mark_exited``) → planned stop. Older markers without ``create_times`` keep PID-only matching.""" + incarnation: gone with no clean exit → dead. Its own clean exit (create_time carried by + ``mark_exited``) → planned stop. A sentinel from a gateway older than the identity stamp, and + older markers without ``create_times``, keep PID-only matching.""" monkeypatch.setattr("hermes_cli.process_identity._process_create_time", lambda pid=None: 1000.0) gateway_windows._write_start_attestation([111], "direct spawn (PID 111)") - _sentinel(attest_home, phase="running", pid=111, start_time=1001.5) + _sentinel(attest_home, phase="running", pid=111, create_time=1001.5) assert gateway_windows.attested_death_generation(current_pids=[]) is not None - _sentinel(attest_home, phase="exited", pid=111, start_time=1001.5, exit_reason="graceful_shutdown") + _sentinel(attest_home, phase="exited", pid=111, create_time=1001.5, exit_reason="graceful_shutdown") assert gateway_windows.attested_death_generation(current_pids=[]) is None + # Pre-identity sentinel (no create_time, start_time is the later ledger claim): PID-only rule. + _sentinel(attest_home, phase="running", pid=111, start_time=1007.0) + assert gateway_windows.attested_death_generation(current_pids=[]) is not None # No sentinel at all: the process never booted far enough to claim one → dead. (attest_home / "state" / "gateway.lifecycle.json").unlink() assert gateway_windows.attested_death_generation(current_pids=[]) is not None @@ -289,5 +293,5 @@ def test_attestation_bound_to_create_time_keeps_authority_for_its_own_incarnatio path = attest_home / "state" / "gateway.start-attestation.json" legacy = {k: v for k, v in json.loads(path.read_text(encoding="utf-8")).items() if k != "create_times"} path.write_text(json.dumps(legacy), encoding="utf-8") - _sentinel(attest_home, phase="running", pid=111, start_time=1003.0) + _sentinel(attest_home, phase="running", pid=111, create_time=1003.0) assert gateway_windows.attested_death_generation(current_pids=[]) is not None diff --git a/tests/hermes_cli/test_windows_gateway_cold_start_desktop_lifecycle.py b/tests/hermes_cli/test_windows_gateway_cold_start_desktop_lifecycle.py index d7ec3023e6..db1ff57dcb 100644 --- a/tests/hermes_cli/test_windows_gateway_cold_start_desktop_lifecycle.py +++ b/tests/hermes_cli/test_windows_gateway_cold_start_desktop_lifecycle.py @@ -187,7 +187,10 @@ def test_attested_dead_gateway_survives_desktop_ownership_and_marker_is_consumed monkeypatch.setattr(gateway_windows, "_wait_for_gateway_ready", lambda *a, **k: [4242]) monkeypatch.setattr(gateway_windows, "_write_start_attestation", lambda *a, **k: None) - assert update_cmd._cold_start_windows_gateway_after_update(token) is True + # A token written by pre-generation code and resumed across this very update carries no + # ``attested_generation`` key: the marker is probed again rather than the spawn skipped. + legacy_token = {k: v for k, v in token.items() if k != "attested_generation"} + assert update_cmd._cold_start_windows_gateway_after_update(legacy_token) is True assert spawned == [1] assert "Gateway started via cold-start after update (PID: 4242)" in capsys.readouterr().out assert not marker.exists() # consumed by the spawn