Skip to content

fix(update): wait for resumed Windows gateway before failing fleet check - #94580

Closed
AlexGabbia wants to merge 1 commit into
NousResearch:mainfrom
AlexGabbia:fix/update-fleet-check-windows-resume-race
Closed

AlexGabbia wants to merge 1 commit into
NousResearch:mainfrom
AlexGabbia:fix/update-fleet-check-windows-resume-race

Conversation

@AlexGabbia

Copy link
Copy Markdown
Contributor

Problem

On Windows, hermes update reports "Update failed (exit 1)" even though the code update completes successfully. The desktop surfaces the failure, and the handoff script (scripts/desktop-update/windows.ps1) retries the entire update.

Root cause

The post-update fleet version check (_cmd_update_impl) sleeps 2s and probes once. On Windows the resume path relaunches the gateway detached, and it needs ~10s to boot (the Telegram polling reconnect) before it stamps gateway_state.json or answers the control socket. The probe therefore finds zero rows for a healthy resume, _fleet_probe_expected_runtimes() reports "verification incomplete", and the update exits 1.

The retry then pauses the gateway the first attempt had just started, killing it, and repeats the cycle — leaving the user with a stopped gateway and a spurious failure message.

Fix

Poll a bounded window (up to 30s) for the resumed gateway to publish its identity instead of probing once. Only a persistently empty snapshot is treated as verification failure, so the #93406 fail-closed contract is preserved (a gateway that genuinely never comes back still exits 1).

Test plan

  • ests/hermes_cli/test_update_fleet_check_fail_closed.py — 11 passed
  • ests/hermes_cli/test_update_receipt.py, est_fleet_matrix_down_state.py, est_update_desktop_stale_warning.py, est_update_version_report.py — 42 passed

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists 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 25, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

This PR fixes a real Windows-specific race in the update command's fleet check. The old single 2-second sleep was too short for a DETACHED gateway process to boot and publish its identity, causing a false "no rows" result that triggered a destructive full retry (re-killing the gateway the first attempt just started). Replacing the fixed sleep with a bounded 30-second polling loop is the right approach. The pre-restart PID snapshot is correctly preserved through the polling loop so DOWN rows still surface for gateways that were stopped without replacement.

Two concerns:

  1. Loop exits on first non-empty snapshot, not on all-expected (hermes_cli/update_cmd.py:8337): The while True loop breaks as soon as _fleet_snapshot is truthy — meaning at least one row exists. If multiple gateways are expected and the first publishes in 2s while the second takes 12s, the loop exits after the first gateway appears. The second gateway would show as DOWN (via pre_restart_pids), setting gateway_fleet_restart_incomplete = True and triggering the retry loop the PR is trying to avoid. Consider checking that the snapshot contains all _fleet_rows_expected rows (or at least that no expected gateway is in a "not yet published" state distinct from "confirmed DOWN") before breaking, rather than just checking for non-emptiness.

  2. 30-second deadline is hardcoded (hermes_cli/update_cmd.py:8333): The comment notes ~10s for a Telegram gateway, and 30s gives 3x headroom. But on systems with aggressive antivirus scanning or slow disk I/O, a freshly launched DETACHED process could take longer. Since this is an update command where the user is already waiting, consider making the deadline configurable via an environment variable (e.g., HERMES_FLEET_SETTLE_TIMEOUT) with 30s as the default, so operators on slow systems can extend it without code changes.

The else branch for the no-expected-gateways case correctly skips the polling loop. Minor style note: the collect_fleet_versions call is duplicated in both branches — a single call after the conditional polling block would reduce duplication, but the current structure is readable as-is.

The post-update fleet version check slept 2s and probed once. On Windows the
resume path relaunches the gateway detached, and it needs ~10s to boot (the
Telegram polling reconnect) before it stamps gateway_state.json or answers the
control socket. That race reported "no rows" for a healthy resume, exited 1,
and triggered a full retry that re-killed the gateway the first attempt had
just started — leaving it down and surfacing "Update failed (exit 1)".

Poll a bounded window (up to 30s) for the resumed gateway to publish its
identity, and only treat a persistently empty snapshot as verification
failure. The fail-closed contract from NousResearch#93406 is preserved: a gateway that
genuinely never comes back still exits 1.
@AlexGabbia
AlexGabbia force-pushed the fix/update-fleet-check-windows-resume-race branch from 7d12570 to ea2669c Compare August 25, 2026 13:19
@AlexGabbia

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I've addressed the first point — the poll now keeps waiting while any row is still down (the stale pre-restart record of a gateway whose detached replacement is still booting), so a slow second gateway can't be misread as down and re-trigger the retry. I've left the 30s deadline hardcoded for now to keep the change minimal; happy to make it configurable if maintainers prefer.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #95859 (rebase merge, commit b3e477f) — your commit cherry-picked onto current main with your authorship preserved in git log, including the down-rows-keep-polling refinement from your review round, which landed exactly as you wrote it.

Your root-cause read matched both independent repros on #95589 precisely: the detached Windows relaunch needs ~10s to stamp its identity while the check gave it 2, and the false exit-1 cascaded into the desktop hand-off re-killing the gateway the resume had just started. We verified with an A/B replay of that timeline — merge-base's single poll returns zero rows, your poll settles on current at 10s with the worst case unchanged.

#95589 stays open for its separate silent-stall half (the #95625 watchdog, under review). Thanks for a precise fix with the review feedback already folded in.

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 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.

4 participants