fix(cli): add timeout to git stash during update - #57134
Conversation
On Windows, git stash push --include-untracked can hang indefinitely when files are locked by other processes (antivirus, gateway, etc.), causing hermes update to appear stuck for hours. Add a 120-second timeout. When reached, skip stashing and print guidance for manual stash/reset so the update can proceed. Fixes NousResearch#57081
Related: #42862 adds timeouts to all 11 git subprocess calls in the stash-management flow; this PR is a narrower subset targeting just the |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real Windows update hang. Current main still has the unbounded stash call at hermes_cli/main.py:6485-6489.
Problems
- Returning
Noneon timeout makescmd_updatetreat the operation as if there were no stash, while the local changes are still present. A later nonzerogit pull --ff-onlyis handled as divergence and reachesgit reset --hard origin/<branch>athermes_cli/main.py:9900-9912; that fallback’s own comment assumes changes were already stashed. This can discard unstashed tracked edits. - The new test covers the helper return only. It does not cover the subsequent update path or prevent the reset fallback after a timeout.
Suggested changes
- Abort the update through a distinct safe failure path after
TimeoutExpired; do not use the no-stash sentinel while the tree remains dirty. - Add a
cmd_updateregression test proving a timeout cannot reach pull/reset with unstashed changes. The #57081 discussion and PR #57212 document the concrete reserved-nulWindows case and a fail-fast alternative.
Automated hermes-sweeper review.
| " git stash push --include-untracked\n" | ||
| " git reset --hard HEAD" | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Returning None says no stash exists, but local changes remain after the timeout. If the subsequent pull is nonzero, current cmd_update falls back to git reset --hard origin/<branch> at hermes_cli/main.py:9900-9912 under the assumption changes were stashed. Please abort through a distinct safe path instead of continuing with this sentinel.
What does this PR do?
Add a 120-second timeout to
git stash push --include-untrackedin the update flow. On Windows, this command can hang indefinitely when files are locked by other processes (antivirus, gateway, etc.), causinghermes updateto appear stuck for hours.When the timeout is reached, the update proceeds without stashing and prints guidance for manual stash/reset.
Related Issue
Fixes #57081
Type of Change
Changes Made
hermes_cli/main.py: Addedtimeout=120to thegit stash push --include-untrackedsubprocess call in_stash_local_changes_if_needed(). Catchessubprocess.TimeoutExpiredand returnsNone(skip stashing) with actionable guidance.tests/hermes_cli/test_update_autostash.py: Addedtest_stash_local_changes_if_needed_returns_none_on_timeoutverifying the timeout behavior.How to Test
pytest tests/hermes_cli/test_update_autostash.py -q— all 34 tests should pass.hermes updateshould no longer hang indefinitely. After 120s, it prints a warning and proceeds.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