fix(update): bound the seven unbounded git network calls in hermes update - #79192
fix(update): bound the seven unbounded git network calls in hermes update#79192briandevans wants to merge 2 commits into
Conversation
…date `hermes update` and `hermes update --check` shell out to git for seven operations that talk to a remote, and not one of them passed `timeout=`. On a network that blackholes packets — captive-portal Wi-Fi, a half-up VPN, a filtering corporate proxy, a throttled forge — the remote completes the TCP handshake and then goes silent, and the CLI prints "-> Fetching updates..." and blocks forever. No timeout, no error, no way out but Ctrl-C. `hermes update` is a universal command and the fork-sync path fires automatically inside it, so this reaches any user on a bad link. Bound all seven with a single module constant and handle `subprocess.TimeoutExpired` at each site: - `_cmd_update_check`: upstream fetch, origin fallback fetch, non-default branch fetch. - `_cmd_update_impl`: the apply-path fetch. - `_sync_with_upstream_if_needed`: the upstream fetch and the ff-only pull. - `_sync_fork_with_upstream`: the `git push` back to the fork. The error handling is deliberately asymmetric. The user-initiated paths (`update`, `--check`) print a diagnostic and exit non-zero, because a stalled origin means there is nothing to update against. The upstream/fork sync degrades and returns, mirroring its existing `CalledProcessError` branches, because it is a convenience layered on top of the user's actual update and must not block it. A stalled upstream fetch in `--check` falls through to the existing origin fallback rather than aborting, matching how a failed upstream fetch is already handled there. The bound is 300s, not the 10s used by the background probe in `hermes_cli/banner.py`. That probe is non-blocking and its result is discardable; these are foreground operations where a first fetch over a slow link can legitimately run for minutes. The defect being fixed is the unbounded wait, so a generous finite ceiling closes it without regressing any setup that works today.
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing hang in hermes update / hermes update --check by ensuring every git subprocess that can block on a remote network operation is executed with a finite timeout, and by handling subprocess.TimeoutExpired in a user-friendly way.
Changes:
- Add a single
_GIT_NETWORK_TIMEOUT_SECONDS = 300ceiling and apply it to all remote-talking git operations in the update flow (fetch/pull/push), with explicitTimeoutExpiredhandling per path. - Add regression tests that assert (a) all remote-talking invocations are bounded and (b) timeouts are handled without propagating exceptions/tracebacks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/update_cmd.py |
Introduces a shared git network timeout constant + helper, and applies bounded timeouts + timeout handling across update check/apply and upstream fork-sync paths. |
tests/hermes_cli/test_update_network_timeout.py |
Adds coverage to ensure all remote git calls in hermes update are bounded and timeouts are handled (exit/degrade behavior). |
Suppressed comments (4)
tests/hermes_cli/test_update_network_timeout.py:170
- This test only checks stdout (
capsys.readouterr().out). To ensure no traceback / exception details leak to the user, capture and assert against both stdout and stderr.
out = capsys.readouterr().out
tests/hermes_cli/test_update_network_timeout.py:216
- The test currently inspects only stdout, but tracebacks are written to stderr. Combine stdout+stderr when checking that no traceback/exception details were printed.
out = capsys.readouterr().out
tests/hermes_cli/test_update_network_timeout.py:282
- This assertion block reads only stdout from capsys; stderr should be included as well so the test reliably fails if a traceback is printed.
out = capsys.readouterr().out
tests/hermes_cli/test_update_network_timeout.py:295
- This test checks only stdout for traceback text. Since tracebacks are emitted to stderr, combine both streams to accurately enforce the “no traceback” contract.
out = capsys.readouterr().out
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| cmd_update(SimpleNamespace(check=True, branch=None)) | ||
|
|
||
| out = capsys.readouterr().out |
There was a problem hiding this comment.
Good catch — fixed in commit 3069653cc.
You are right that the assertion was false confidence: capsys.readouterr().out is stdout only, and a leaked traceback or unhandled-exception output surfaces on stderr, so the check could never have caught the failure mode it was written to guard.
All five sites now go through a _drain(capsys) helper in tests/hermes_cli/test_update_network_timeout.py (defined just above _check_side_effect) that returns (stdout, stdout + stderr). The no-traceback assertions run against the combined stream, while the positive content assertions ("Timed out" in out, "Skipping upstream sync" in out) stay on stdout, which is where those diagnostics are actually printed. I also extended the same combined-stream coverage to "TimeoutExpired" not in ... at every site — previously only two of the five checked for it at all.
Affected tests (by name, so this survives a rebase): TestUpdateCheckTimeoutHandling::test_upstream_timeout_falls_back_to_origin, TestUpdateCheckTimeoutHandling::test_origin_timeout_exits_cleanly, TestUpdateApplyFetch::test_apply_fetch_timeout_exits_cleanly, TestUpstreamSyncDegradesOnTimeout::test_upstream_fetch_timeout_skips_sync, TestUpstreamSyncDegradesOnTimeout::test_upstream_pull_timeout_skips_sync. All 12 still pass; ruff clean.
The timeout tests asserted `"Traceback" not in capsys.readouterr().out`, but tracebacks and unhandled-exception output go to stderr, so a leak would have slipped past. Route every no-traceback assertion through a `_drain` helper that checks stdout+stderr combined, and extend the same coverage to "TimeoutExpired" at all five sites.
alt-glitch
added
type/bug
What does this PR do?
Supersedes #49181 (rblume32120), which started this fix in June and has been CONFLICTING/DIRTY ever since — no force-push, no commit since 2026-06-19, and it patches
hermes_cli/main.py, a file the entire update implementation has since been mechanically extracted out of (intohermes_cli/update_cmd.py). #50391 (Morad37) is a strictly narrower duplicate of the same idea — the apply-path fetch only, no test — and is also CONFLICTING with no commit since 2026-06-21. Both have to be rewritten against currentmainregardless of who does it; this is that rewrite, plus the coverage the maintainer asked for on #49181.The user symptom. Run
hermes updateorhermes update --checkon a network that blackholes packets — captive-portal Wi-Fi, a half-up VPN, a filtering corporate proxy, a throttled forge — and the remote completes the TCP handshake and then goes silent. The CLI prints→ Fetching updates...(or→ Fetching from upstream...) and blocks forever: no timeout, no error, no way out but Ctrl-C.hermes updateis a universal command and the fork-sync path fires automatically inside it, so any user on a bad link is exposed.Root cause. Every git subprocess in the update flow that talks to a remote is called without
timeout=, sosubprocess.runwaits indefinitely on a stalled socket.The maintainer's acceptance checklist on #49181
_cmd_update_checkupstream fetch — bounded_cmd_update_checkorigin fallback fetch — bounded_cmd_update_checknon-default-branch origin fetch — bounded_cmd_update_impl) — bounded_sync_with_upstream_if_needed) — boundedSibling sweep — a seventh site the original review did not list
Grepping every
subprocess.runin thehermes updateflow whose argv carries a remote-talking verb turns up seven unbounded calls, not six. The extra one is_sync_fork_with_upstream'sgit push origin main --force-with-lease, which runs at the end of the same fork-sync path — a stalled push hangs the update just as hard as a stalled fetch. It is included here.The sweep's boundary is deliberate, and these are explicitly not touched:
hermes_cli/mcp_catalog.pyandhermes_cli/profile_distribution.pyalso run unboundedgit clone. Different commands, different user flows, different concern — bundling them would make this PR two ideas instead of one.update_cmd.py'sgit stash pushandgit merge --ff-only origin/<branch>match on the wordspush/pullbut never leave the machine. Correctly excluded; the tests encode that exclusion so a future refactor cannot quietly widen it.Why 300s and not
banner.py's 10shermes_cli/banner.pybounds its update probe attimeout=10. That probe is a background, non-blocking check whose result is discardable, so a tight bound costs nothing. These seven are foreground, user-initiated operations, and a first fetch over a slow or metered link can legitimately run for minutes — a 10s bound would break setups that work today. The defect here is the unbounded wait, so a generous finite ceiling is a complete fix with no regression risk.No new config surface:
update_cmd.pyhas no env-override convention for tunables, so this adds a single module constant and does not invent one.The error handling is deliberately asymmetric
TimeoutExpired_cmd_update_implapply fetchsys.exit(1)_cmd_update_checkorigin fetchessys.exit(1)_cmd_update_checkupstream fetch_sync_with_upstream_if_neededfetch/pullreturn(degrade)CalledProcessErrorbranches — the upstream sync is a convenience on top of the user's actual update and must not block it_sync_fork_with_upstreampushreturn FalsePreserving that asymmetry is the point: unifying it would either abort updates over a slow upstream or silently swallow a dead origin.
Related Issue
No filed issue — the demand is stated directly in the maintainer review on #49181, quoted above.
Supersedes #49181. Narrower duplicate: #50391.
Type of Change
Changes Made
hermes_cli/update_cmd.py_GIT_NETWORK_TIMEOUT_SECONDS = 300, with the rationale for 300-vs-10 in the comment._print_git_network_timeout(remote)— one diagnostic, in the same style as the existing✗ Network error — cannot reach the remote repository.branch, used by the three fatal sites._sync_fork_with_upstream— bound thegit push origin main --force-with-lease._sync_with_upstream_if_needed— boundgit fetch upstream mainandgit pull --ff-only upstream main; added aTimeoutExpiredbranch to each that degrades and returns._cmd_update_check— bound all threegit fetchcalls; upstream stall falls back to origin, origin stall exits 1._cmd_update_impl— boundgit fetch origin <branch>; stall exits 1.tests/hermes_cli/test_update_network_timeout.py(new, 12 tests)timeoutkwarg, on all three entry paths.TimeoutExpirednever propagates: fatal paths exit 1 with a diagnostic and no traceback; the upstream-sync path prints and returns without aborting; the upstream stall in--checkstill reaches the origin fallback.git stashand localmerge, so the tests fail if a future change bounds only some sites.How to Test
originat an address that accepts connections and never answers, e.g.git remote set-url origin https://10.255.255.1/x.git(an unroutable host behind a stateful firewall), then runhermes update --check. Before this change the CLI prints→ Fetching from origin...and hangs indefinitely. After it, the fetch is bounded and you get:subprocess.runis mocked):hermes_cli/update_cmd.pyfrommainand re-running the new file turns 11 of the 12 red. The twelfth,test_fork_push_timeout_degrades_to_false, is labelled in its docstring as a contract test rather than a regression test —_sync_fork_with_upstreamalready had a catch-allexcept, so it is green either way; its companiontest_fork_push_is_boundedis the one that fails without the fix.pytest tests/hermes_cli/ -q -p no:randomlyproduces a byte-identical failure set on this branch and on cleanmain(82c6aca) — 142 pre-existing failures on both, zero new and zero fixed. Adjacent update suites (test_cmd_update.py,test_update_check.py,test_update_yes_flag.py,test_update_hangup_protection.py,test_update_fleet_restart_timeout.py,test_update_autostash.py) are all green: 55 passed.ruff check hermes_cli/update_cmd.py tests/hermes_cli/test_update_network_timeout.py— clean. Source parses underast.parse(..., feature_version=(3, 9)).Related / Positioning
#73751 (Frowtek) touches the same functions but fixes a different hang: it adds
stdin=DEVNULL+ a non-interactive git env so a credential prompt can't block. That is orthogonal and complementary to bounding the network wait — neither change subsumes the other. It is also CONFLICTING againsthermes_cli/main.py. This PR does not touch stdin or the git env, so the two remain independently applicable.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A