From 975b62663a2a554eb06d33b098104f9b4d5ec7ea Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 9 Oct 2026 03:29:14 -0700 Subject: [PATCH] fix(telemetry): a Desktop hand-off's update refused by its own claim counts as failed The hand-off quits the Desktop and launches `hermes update` as the marker's delegate. When the update lock then refuses that claim (the pre-a28a5d03a9f whole-second creation-time bug on macOS, still present on installs that cannot pull the fix through the button), no other updater exists: the Desktop shows "Hermes couldn't finish updating" and the user retries. The metrics row read outcome `refused`, which the rollup treats as neither attempt nor failure, so ~320 Desktop update failures a fortnight vanished from the failure rate. Record that one case (marker names this process as its delegate) as `failed`; a refusal under another update's claim stays `refused`. --- hermes_cli/update_receipt.py | 18 ++++++++++----- ...test_shared_metrics_update_stop_reasons.py | 23 +++++++++++++++++++ .../developer-guide/relay-shared-metrics.md | 2 +- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/hermes_cli/update_receipt.py b/hermes_cli/update_receipt.py index 85c3310417..b044f4117a 100644 --- a/hermes_cli/update_receipt.py +++ b/hermes_cli/update_receipt.py @@ -563,8 +563,9 @@ def git_output_stop_class(output: str, argv: list[str], returncode: Optional[int return None -def _started_by_handoff_partner() -> bool: - """True when the update marker names the Desktop hand-off this process runs under. +def _started_by_handoff_partner() -> str | None: + """``delegate`` (the hand-off launched this process as its update child) or ``partner`` when + the update marker names the Desktop hand-off this process runs under, else None. For an exit before the receipt opens, where this process may NOT hold the lock: at the update-lock refusal the marker belongs to whichever update holds it, so "a pid other than @@ -579,12 +580,12 @@ def _started_by_handoff_partner() -> bool: marker = _parse_marker(update_marker_path().read_bytes()) if marker.started_at is None: - return False + return None pid = os.getpid() if marker.delegate_pid == pid: - return True + return "delegate" partners = {_handoff_pid(), os.getppid()} - {None, 0, 1, pid} - return bool(partners & {marker.pid, marker.delegate_pid}) + return "partner" if partners & {marker.pid, marker.delegate_pid} else None def record_stop_without_receipt(reason: str, outcome: str) -> None: @@ -605,8 +606,13 @@ def record_stop_without_receipt(reason: str, outcome: str) -> None: "stop_class": reason, "pre_update": {}, "stages": [], "steps": [], "fleet": [], } with suppress(Exception): # no marker, or an unreadable one: the CLI default - if _started_by_handoff_partner(): + claim = _started_by_handoff_partner() + if claim: data["initiator"] = "desktop" + if claim == "delegate": + # The Desktop checked the marker, handed this run off and quit; its result dialog + # reports the refusal as a failed update, so the row must not read a benign one. + data["outcome"] = "failed" from hermes_cli.observability.shared_metrics_update import record_update_receipt record_update_receipt(data) diff --git a/tests/hermes_cli/test_shared_metrics_update_stop_reasons.py b/tests/hermes_cli/test_shared_metrics_update_stop_reasons.py index 1150434a11..d10c28ce91 100644 --- a/tests/hermes_cli/test_shared_metrics_update_stop_reasons.py +++ b/tests/hermes_cli/test_shared_metrics_update_stop_reasons.py @@ -152,6 +152,29 @@ def test_an_exit_before_the_receipt_reads_desktop_only_under_its_own_hand_off(ro ("lock_held", "desktop"), ("git_in_progress", "desktop"), ("lock_held", "cli"), ("managed_install", "cli")] +def test_a_lock_refusal_of_the_hand_offs_own_update_child_is_a_failed_run(rows, monkeypatch): + """Invariant: when the lock refuses the claim whose delegate line names this very process, the + Desktop quit to run this update and reports the refusal as a failed update, so the row reads + ``failed``; a refusal under someone else's claim (Tauri partner, a CLI run) stays ``refused``.""" + import os + + from hermes_cli.update_cmd_common import _record_stop + + monkeypatch.delenv("HERMES_UPDATE_HANDOFF_PID", raising=False) + pid, other = os.getpid(), 999_999 + _marker(rows.home, other, 1, "ct:1.000", f"delegate:{pid} ct:2.000") # our own hand-off's claim + _record_stop("lock_held", without_receipt="refused") + monkeypatch.setenv("HERMES_UPDATE_HANDOFF_PID", str(other)) + _marker(rows.home, other, 1, "ct:1.000") + _record_stop("lock_held", without_receipt="refused") + monkeypatch.delenv("HERMES_UPDATE_HANDOFF_PID") + _record_stop("lock_held", without_receipt="refused") + + assert [(r["outcome"], r["kind"], r["failure_class"]) for r in rows.runs()] == [ + ("failed", "desktop", "lock_held"), ("refused", "desktop", "lock_held"), ("refused", "cli", "lock_held")] + assert all(contract.counter_dimensions_are_valid(contract.UPDATE_RUN_METRIC, r) for r in rows.runs()) + + @pytest.mark.skipif(sys.platform == "win32" or os.geteuid() == 0, reason="POSIX permissions; root ignores them") def test_a_checkout_move_names_a_held_index_lock_apart_from_a_permission_error(rows, tmp_path, monkeypatch): """Invariant: the fast-forward's git error goes through the shared classifier: an index.lock that diff --git a/website/docs/developer-guide/relay-shared-metrics.md b/website/docs/developer-guide/relay-shared-metrics.md index 4400753708..f92656b9ce 100644 --- a/website/docs/developer-guide/relay-shared-metrics.md +++ b/website/docs/developer-guide/relay-shared-metrics.md @@ -506,7 +506,7 @@ they failed. Rows recorded before the change keep their old field set and still | Metric | Dimensions added | Question it answers | |---|---|---| | `hermes.extension.install.count` | failure class, registry | Why skill, plugin and MCP installs fail, and which skills-hub source fails. `failure_class` is `none` on success, else a closed name for the failure exit that stopped the install. Skills: `ambiguous`, `auth_rejected`, `fetch_failed`, `invalid_bundle`, `invalid_name`, `not_found`, `rate_limited`, `scan_blocked`, `stale_index`. Plugins: `already_installed`, `clone_failed`, `deps_declined`, `deps_failed`, `git_missing`, `incompatible`, `invalid_source`, `manifest_invalid`, `non_interactive`, `removed_from_catalog`, `scan_blocked`. MCP servers: `auth_required`, `bootstrap_failed`, `clone_failed`, `config_invalid`, `config_rejected`, `connect_failed`, `git_missing`, `missing_credentials`, `server_start_failed`. Every kind may also read `exception`, `filesystem_error`, `network`, `other`, `permission`. A raised error without a named class is classified by its Python type only (`PermissionError` reads `permission`, connection and timeout errors `network`, other `OSError`s `filesystem_error`, anything else `exception`, its type name dropped). `registry` is set on skill rows only: the skills-hub adapter that resolved or served the skill, one of `browse-sh`, `clawhub`, `github`, `hermes-index`, `lobehub`, `official`, `skills-sh`, `url`, `well-known` (the adapter ids `tools/skills_hub_search.py::create_source_router` builds), `unresolved` when no adapter answered, `none` for built-in restores and every plugin/MCP row, `other` otherwise. A row whose `failure_class` is outside its kind's set, is `none` on a failure or set on a success, or that names a registry on a plugin/MCP row is invalid. Never the identifier, URL, error text or path; `name` keeps its existing catalog-or-`custom` rule. | -| `hermes.update.run` | failure class | Why `hermes update` fails, next to where (`failed_stage`). `none` unless the outcome is `failed`, `refused` or `partial`. Derived from the final update receipt only: `build_failed`, `deps_failed`, `exception`, `fleet_stale`, `fleet_unverified`, `git_failed`, `interrupted`, `lock_held`, `managed_install`, `os_error`, `disk_full`, `permission_denied`, `restart_failed`, `subprocess_failed`, else `other`; Desktop packaged self-updates report `unknown` (their RPC carries a stage, never a reason). An exception that ended the run is read by its type name alone, never its message (a `PermissionError` reads `permission_denied`, an `OSError` whose errno is ENOSPC reads `disk_full`); a failed Windows gateway resume after the update reads `restart_failed`. Every exit that stops a run before its apply stage mark records one closed token at the exit itself, and the run reads it (a few of these, `syntax_rollback`, `stash_restore_rejected`, `unexpected_branch`, `head_moved`, fire after git moved the tree and put it back or left it for the next launch to restore): `fetch_failed`, `git_timeout` (a network git call hit the updater's time limit), `channel_unresolved`, `git_in_progress` (a merge/rebase/... was already running), `git_index_locked`, `merge_conflict` (local commits on a custom branch), `checkout_move_failed`, `branch_missing`, `parked_branch_blocked`, `detached_head`, `unexpected_branch`, `head_moved` (a ref moved mid-update), `target_unresolved`, `target_syntax_error` (refused before the move), `syntax_rollback` (moved, failed the syntax check, rolled back), `commit_point_refused` (the commit point could not be armed; nothing moved), `venv_foreign_owner`, `local_changes_blocked` (local changes git could not stash, or a dirty tree the ZIP fallback will not overwrite), `stash_restore_rejected`, `not_git_checkout`, `old_version_handoff` (an older updater's hand-off could not finish), `branch_unsupported`, `download_failed`, `zip_failed`, `disk_full`, `lock_held`, `managed_install`. `aborted_before_apply` is now only the fallback for such an exit with no recorded token. The update-lock refusal, the Git-operation refusal and the managed-install refusal fire before the run's receipt opens; they still write no receipt (the receipt store belongs to the update holding the lock) and record one row with that token, with `kind` `desktop` when the update marker names this run's Desktop hand-off (as its delegate or its hand-off partner), never merely because another update holds the marker. A git error that ended the checkout move reads `git_index_locked` only when git reports the index lock already exists; git refused permission to write it (or any file) reads `permission_denied`. A token is read only while the run has no apply mark, so a reason can never outlive the exit that recorded it. A run that committed and then was interrupted, or whose stashed local changes could not be re-applied, reads outcome `partial` (`interrupted` / `local_changes_parked`, `failed_stage` `apply` for the latter), never `failed`; owed follow-ups alone keep the run a `success` (contract C3). A run the pre-update interpreter finishes is parked with the token, the exit code, the stop reason's leading exception type (or one of Hermes' two fixed phrases) and the restart/user-action flags (never argv, paths or messages), so it classifies exactly as the same run finished in-process. A failed run whose every stage mark passed now reads `failed_stage` `verify` (the post-restart verification wrote `partial`) or `restart` (a skipped restart left the fleet owing one) instead of `other`; a `partial` run with a failed build or restart mark keeps that stage, and its verify stage row is failed only when the fleet has a stale or down row. | +| `hermes.update.run` | failure class | Why `hermes update` fails, next to where (`failed_stage`). `none` unless the outcome is `failed`, `refused` or `partial`. Derived from the final update receipt only: `build_failed`, `deps_failed`, `exception`, `fleet_stale`, `fleet_unverified`, `git_failed`, `interrupted`, `lock_held`, `managed_install`, `os_error`, `disk_full`, `permission_denied`, `restart_failed`, `subprocess_failed`, else `other`; Desktop packaged self-updates report `unknown` (their RPC carries a stage, never a reason). An exception that ended the run is read by its type name alone, never its message (a `PermissionError` reads `permission_denied`, an `OSError` whose errno is ENOSPC reads `disk_full`); a failed Windows gateway resume after the update reads `restart_failed`. Every exit that stops a run before its apply stage mark records one closed token at the exit itself, and the run reads it (a few of these, `syntax_rollback`, `stash_restore_rejected`, `unexpected_branch`, `head_moved`, fire after git moved the tree and put it back or left it for the next launch to restore): `fetch_failed`, `git_timeout` (a network git call hit the updater's time limit), `channel_unresolved`, `git_in_progress` (a merge/rebase/... was already running), `git_index_locked`, `merge_conflict` (local commits on a custom branch), `checkout_move_failed`, `branch_missing`, `parked_branch_blocked`, `detached_head`, `unexpected_branch`, `head_moved` (a ref moved mid-update), `target_unresolved`, `target_syntax_error` (refused before the move), `syntax_rollback` (moved, failed the syntax check, rolled back), `commit_point_refused` (the commit point could not be armed; nothing moved), `venv_foreign_owner`, `local_changes_blocked` (local changes git could not stash, or a dirty tree the ZIP fallback will not overwrite), `stash_restore_rejected`, `not_git_checkout`, `old_version_handoff` (an older updater's hand-off could not finish), `branch_unsupported`, `download_failed`, `zip_failed`, `disk_full`, `lock_held`, `managed_install`. `aborted_before_apply` is now only the fallback for such an exit with no recorded token. The update-lock refusal, the Git-operation refusal and the managed-install refusal fire before the run's receipt opens; they still write no receipt (the receipt store belongs to the update holding the lock) and record one row with that token, with `kind` `desktop` when the update marker names this run's Desktop hand-off (as its delegate or its hand-off partner), never merely because another update holds the marker. A lock refusal of the update child a Desktop hand-off launched (the marker names it as the delegate) reads outcome `failed`, not `refused`: the Desktop already quit to run that update and reports it as a failed one. A git error that ended the checkout move reads `git_index_locked` only when git reports the index lock already exists; git refused permission to write it (or any file) reads `permission_denied`. A token is read only while the run has no apply mark, so a reason can never outlive the exit that recorded it. A run that committed and then was interrupted, or whose stashed local changes could not be re-applied, reads outcome `partial` (`interrupted` / `local_changes_parked`, `failed_stage` `apply` for the latter), never `failed`; owed follow-ups alone keep the run a `success` (contract C3). A run the pre-update interpreter finishes is parked with the token, the exit code, the stop reason's leading exception type (or one of Hermes' two fixed phrases) and the restart/user-action flags (never argv, paths or messages), so it classifies exactly as the same run finished in-process. A failed run whose every stage mark passed now reads `failed_stage` `verify` (the post-restart verification wrote `partial`) or `restart` (a skipped restart left the fleet owing one) instead of `other`; a `partial` run with a failed build or restart mark keeps that stage, and its verify stage row is failed only when the fleet has a stale or down row. | #### Fresh installs