Skip to content

fix(desktop): don't dead-end the update on ledgered manual serve blockers - #98350

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98336
Closed

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98336

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

On Windows, the Desktop update preflight (hermes_cli._scan_venv_blockers) reports every hermes serve/dashboard holder as a venv blocker, so the hand-off aborts with venv-blocked before hermes-setup ever spawns the updater — even though the CLI updater's venv guard has a rung (#63206) that stops exactly the MANUAL serve/dashboard backends (spawn-ledger positive identity for this install, spawner provably not alive) and relaunches them on their recorded endpoints after the update. This is the same dead-end shape the gateway exemption (_is_pausable_gateway) already fixed for gateway run; this PR closes the gap for manual serves (#98336).

The fix mirrors the gateway exemption: holders vouched for by the canonical matcher update_cmd._ledger_manual_serve_holders are exempted from the blocker list (counted in a new diagnostic ledgered_manual_serves field). Delegating to that matcher — rather than a cmdline regex — keeps the preflight exemption, the updater's stop rung, and the relauncher on one parser, and inherits its safety properties: install-scoped ledger identity ((pid, create_time) for THIS install), spawner-liveness check, and structured relaunch.

Deliberately NOT exempted (fail-closed, unchanged behavior):

  • Desktop-owned serves (live spawner) — the app would respawn what the update kills;
  • unledgered serves (no positive identity → still a blocker, listed with PID/cmdline);
  • foreign installs (ledger entries are install-id scoped);
  • any matcher import failure reads as "no manual serves" → pre-exemption behavior.

Related Issue

Fixes #98336

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/_scan_venv_blockers.py: added _ledgered_manual_serve_pids(), which delegates to the canonical ledger matcher (import failure → empty set, i.e. fail-closed); main() now skips those PIDs after the gateway exemption and reports a ledgered_manual_serves diagnostic count (mirroring pausable_gateways).
  • tests/hermes_cli/test_scan_venv_blockers.py: three regression tests — ledgered manual serve scans clear; a ledgered serve alongside an unledgered/Desktop-owned serve still blocks on the latter only; a matcher failure keeps the serve blocking (fail-closed).

How to Test

  1. Run the scanner suite → 34 passed (31 pre-existing + 3 new).
  2. Related ledger/guard suites (process identity, MCP helper ledger, serve runtime inventory, Windows gateway cold-start desktop lifecycle) → 50 passed.
  3. python -m hermes_cli._scan_venv_blockers → Observed result: {"ok": true, "blocked": false, "processes": [], "pausable_gateways": 0, "ledgered_manual_serves": 0}, exit 0 — new field present, import chain intact.
  4. ruff check on both changed files → all checks passed.

The TS consumer (apps/desktop/electron/venv-blocker-scan.ts) needs no change: parseVenvBlockerScanOutput ignores unknown top-level fields, and blocked/processes consistency is produced by the Python side.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.6 (arm64); the Windows behavior is covered by the patched-detector unit tests, which feed real Windows cmdline shapes

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (docstring on the new helper documents the contract)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…kers

The Desktop Windows preflight reported every 'hermes serve'/'dashboard'
holder as a venv blocker, aborting the hand-off before hermes-setup could
spawn the updater — yet the updater's venv guard has a rung (NousResearch#63206) that
stops exactly the MANUAL serve/dashboard backends (spawn-ledger positive
identity for this install, spawner provably not alive) and relaunches them
on their recorded endpoints after the update. The same dead-end shape the
gateway exemption fixed, left unfixed for serves (NousResearch#98336).

Mirror the gateway exemption: exempt holders the canonical matcher
update_cmd._ledger_manual_serve_holders vouches for, so the preflight,
the stop rung, and the relauncher share one parser. Desktop-owned serves
(live spawner), unledgered serves, and foreign installs keep blocking;
an import failure reads as no manual serves (pre-exemption fail-closed).
@liyangbing

Copy link
Copy Markdown

Review

Delegating to the canonical ledger matcher is the right direction: the scanner, updater stop rung, and relauncher should share one install-scoped identity rather than maintain independent command-line heuristics. The fail-closed behavior for matcher import failure also preserves the safer pre-exemption path.

Suggested contract

  • Treat the ledger identity as a tuple such as PID plus process creation time, and revalidate it immediately before exemption and again before termination; PID reuse must never exempt a foreign process.
  • Keep the install/profile identity in the match, not only the command role, so a ledger entry from another Hermes installation cannot suppress a blocker.
  • Make the scan result explain the decision without leaking command-line secrets: report counts and sanitized role/path evidence for exempted and still-blocking processes.
  • Preserve the fail-closed rule for malformed, stale, missing, or unreadable ledger entries, including a ledger race where the spawner exits or a service restarts between scan and stop.
  • Ensure the updater stop and relaunch path are idempotent. If termination is denied or the process disappears, the hand-off should retain the old runnable package and surface a resumable state rather than claiming success.
  • Verify that the desktop consumer tolerates the diagnostic field while still enforcing the authoritative blocked/processes fields; add a contract fixture so a future parser change cannot silently ignore a new safety state.

Regression matrix

  • one ledgered manual serve, one unledgered serve, and a Desktop-owned serve together
  • PID reuse, changed creation time, stale ledger, malformed ledger, and foreign install/profile
  • matcher import failure, spawner still alive, spawner exit during scan, and a new process appearing before rename
  • serve and dashboard variants with Windows command-line quoting and path aliases
  • graceful stop, timeout, access denial, already-exited process, and relaunch with the recorded endpoint and profile
  • interrupted hand-off, rollback after failed update, and post-update health verification
  • scanner JSON consumed by the TypeScript updater with blocked/processes consistency preserved

The handbook's Windows update-boundary guide covers the same process-identity, lock-evidence, and rollback requirements.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 30, 2026
…fixture

Address review on NousResearch#98350: report structured ledger identity (pid/purpose/
port, never argv) for exempted manual serves so the scan result explains
its decision without echoing command lines, and pin in the TypeScript
parser tests that the diagnostic fields stay non-authoritative while
blocked/processes consistency is still enforced.
@liuhao1024

Copy link
Copy Markdown
Author

Thanks @liyangbing for the thorough review. Pushed fa76c0a with the scanner-side items; the identity contracts are satisfied by the canonical delegation. Point by point:

Done in this push (scanner scope)

  • Sanitized evidence: the scan result now carries exempted_manual_serves — structured ledger identity (pid, purpose, recorded port) only, never argv — alongside the existing counts (ledgered_manual_serves, pausable_gateways) and the still-blocking processes list (which keeps the redacted, 120-char-truncated cmdline). Both sides of the decision now explain themselves without echoing command lines.
  • Desktop contract fixture: added a case in venv-blocker-scan.test.ts pinning that the TypeScript parser tolerates the diagnostic fields while still enforcing the authoritative blocked/processes consistency — a future parser change that either chokes on the diagnostics or reinterprets an exemption as a blocker fails the fixture.

Satisfied by the canonical delegation

  • (pid, create_time) revalidation: _ledger_manual_serve_holders reads through process_identity.ledger_entries(), which verifies each entry's (pid, create_time) pair against the live process table at scan time (_pid_alive_matches, 2s tolerance) — PID reuse reads as dead and can never exempt a foreign process. Covered by tests/hermes_cli/test_process_identity.py.
  • Install scoping: ledger_entries() filters on install_id(project_root) (path hash), so a ledger entry from another Hermes installation cannot suppress a blocker in this scan.
  • Fail-closed ledger states: a corrupt ledger is quarantined and read as empty, and an import/matcher failure counts as no manual serves (regression-tested here) — both land on "keep blocking", the pre-exemption behavior. The spawner-exits-between-scan-and-stop race is owned by the updater's stop rung (it re-reads the ledger when it actually runs); this scanner never terminates anything, so it cannot widen that window.

Out of scope for this scanner mirror (owned by the #63206 rung and its tests): stop/relaunch idempotency and resumable-state semantics, plus the Windows-quoting/rollback rows of the matrix — those live in the updater (test_update_*, test_process_identity.py), not in the Desktop preflight view.

@teknium1

Copy link
Copy Markdown
Collaborator

Status note (not a verdict): this PR is the scanner-side mirror of the updater's #63206 ledgered-manual-serve rung, targeting #98336. That serve/gateway run classification-and-exemption work in _scan_venv_blockers.py is being consolidated right now in a sibling workstream for #98336/#81774 — please hold off rebasing until that lands so the two don't double-implement the exemption. The design direction here (delegate to the canonical _ledger_manual_serve_holders matcher, fail closed on import failure, sanitized structured evidence instead of argv) matches where that consolidation is headed and will be credited if absorbed. Leaving open for the consolidating maintainer.

Unrelated defects in the same family were resolved separately: scan timeout/perf via #99674 (salvage of #75570), truncated-cmdline classification already on main (0b33ee88e4).

@liuhao1024

Copy link
Copy Markdown
Author

Acknowledged — holding off on any rebase until the #98336/#81774 consolidation lands. Keeping this PR open as-is for the consolidating maintainer; happy to rework on top of the consolidated _scan_venv_blockers.py if the exemption shape changes there. Thanks also for the pointers on #99674 and 0b33ee88e4 — neither overlaps with the ledgered-manual-serve exemption itself, but I'll re-verify the fail-closed path against the truncated-cmdline classification once the sibling workstream is in.

@alt-glitch alt-glitch added blocked Waiting on external dependency or decision and removed blocked Waiting on external dependency or decision labels Aug 31, 2026
@teknium1

teknium1 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Wave-3 salvage status: the surviving half of this PR now rides in #100124 with your authorship preserved (commit authored as liuhao1024).

What survived vs. main: the exemption itself landed independently via #99724 (_is_updater_owned_backend — ledger-verified serve/dashboard holders are deferred to the updater's stop+relaunch rungs, including the this-handoff-Desktop ancestor case). What #100124 carries from here is your sanitized-evidence design from fa76c0a: the scan output now explains WHICH holders the deferral consumed via structured ledger identity (deferred_backend_evidence: pid/purpose/port — never argv), plus your desktop parser contract fixture, adapted to main's merged field names (deferred_backends instead of ledgered_manual_serves).

This PR will be closed with credit once #100124 merges. Thanks for the careful canonical-matcher delegation design — it shaped the consolidated implementation.

teknium1 pushed a commit that referenced this pull request Sep 1, 2026
…lockers

The Desktop venv-blocker scan (since #99724) defers ledger-verified
serve/dashboard holders to the CLI updater's stop+relaunch rungs, but the
scan output only carried an opaque deferred_backends count — nothing
explained WHICH holders the deferral consumed or why they vanished from
processes.

Add sanitized decision evidence (#98350): deferred_backend_evidence lists
structured ledger identity only (pid, purpose, recorded port) — never the
command line, which can carry tokens or private endpoints. Adds a desktop
parser contract fixture proving the consumer tolerates the diagnostics
while keeping blocked/processes authoritative.

Salvaged from PR #98350; the exemption half of that PR was independently
consolidated on main via #99724 (_is_updater_owned_backend).
@teknium1

teknium1 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks @liuhao1024 — landed on main via #100124. The exemption half of this was absorbed earlier by #99724 (updater-owned backend classification incl. manual serves), and #100124 carried your surviving evidence half (deferred_backend_evidence: pid/purpose/port, never argv) authored under your name. Closing as superseded by the merged salvages.

@teknium1 teknium1 closed this Sep 1, 2026
royalaid added a commit to royalaid/hermes-agent that referenced this pull request Sep 2, 2026
Desktop runs the bundled carrier copy of the venv-blocker scanner with
`python -I`, never hermes_cli/_scan_venv_blockers.py. The upstream rebase
added `deferred_backend_evidence` (NousResearch#98350) to the module and to the exact-key
parser, the carrier never got it, and every "Update now" click aborted with
"Desktop could not verify the Hermes installation is free" while zero holders
existed (7 aborts on 2026-09-02, desktop.log: `scanner envelope fields are
invalid`).

A second, latent drift hid behind the pre-scan kill-all: both scanner copies
attach `parent_pid` to pausable-gateway records, but the parser only accepted
it on generic records, so any scan taken while a gateway was alive was
rejected with `pausable gateway identity is invalid`.

- carrier emits `deferred_backend_evidence: []`
- parser accepts `parent_pid` on gateway records (still a positive integer)
- the carrier test now round-trips real scanner stdout through
  parseVenvBlockerScanOutput, the contract the preflight consumes
- parser tests pin the gateway parent_pid case

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
royalaid added a commit to royalaid/hermes-agent that referenced this pull request Sep 3, 2026
Desktop runs the bundled carrier copy of the venv-blocker scanner with
`python -I`, never hermes_cli/_scan_venv_blockers.py. The upstream rebase
added `deferred_backend_evidence` (NousResearch#98350) to the module and to the exact-key
parser, the carrier never got it, and every "Update now" click aborted with
"Desktop could not verify the Hermes installation is free" while zero holders
existed (7 aborts on 2026-09-02, desktop.log: `scanner envelope fields are
invalid`).

A second, latent drift hid behind the pre-scan kill-all: both scanner copies
attach `parent_pid` to pausable-gateway records, but the parser only accepted
it on generic records, so any scan taken while a gateway was alive was
rejected with `pausable gateway identity is invalid`.

- carrier emits `deferred_backend_evidence: []`
- parser accepts `parent_pid` on gateway records (still a positive integer)
- the carrier test now round-trips real scanner stdout through
  parseVenvBlockerScanOutput, the contract the preflight consumes
- parser tests pin the gateway parent_pid case

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…lockers

The Desktop venv-blocker scan (since NousResearch#99724) defers ledger-verified
serve/dashboard holders to the CLI updater's stop+relaunch rungs, but the
scan output only carried an opaque deferred_backends count — nothing
explained WHICH holders the deferral consumed or why they vanished from
processes.

Add sanitized decision evidence (NousResearch#98350): deferred_backend_evidence lists
structured ledger identity only (pid, purpose, recorded port) — never the
command line, which can carry tokens or private endpoints. Adds a desktop
parser contract fixture proving the consumer tolerates the diagnostics
while keeping blocked/processes authoritative.

Salvaged from PR NousResearch#98350; the exemption half of that PR was independently
consolidated on main via NousResearch#99724 (_is_updater_owned_backend).
royalaid added a commit to royalaid/hermes-agent that referenced this pull request Sep 4, 2026
Desktop runs the bundled carrier copy of the venv-blocker scanner with
`python -I`, never hermes_cli/_scan_venv_blockers.py. The upstream rebase
added `deferred_backend_evidence` (NousResearch#98350) to the module and to the exact-key
parser, the carrier never got it, and every "Update now" click aborted with
"Desktop could not verify the Hermes installation is free" while zero holders
existed (7 aborts on 2026-09-02, desktop.log: `scanner envelope fields are
invalid`).

A second, latent drift hid behind the pre-scan kill-all: both scanner copies
attach `parent_pid` to pausable-gateway records, but the parser only accepted
it on generic records, so any scan taken while a gateway was alive was
rejected with `pausable gateway identity is invalid`.

- carrier emits `deferred_backend_evidence: []`
- parser accepts `parent_pid` on gateway records (still a positive integer)
- the carrier test now round-trips real scanner stdout through
  parseVenvBlockerScanOutput, the contract the preflight consumes
- parser tests pin the gateway parent_pid case

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop auto-update fails on Windows when hermes serve / gateway child processes hold hermes.exe (os error 32)

4 participants