Skip to content

fix(cli): print exit summary before cleanup so watchdog can't swallow it - #72164

Open
adurham wants to merge 2 commits into
NousResearch:mainfrom
adurham:upstream-pr/exit-summary-before-cleanup
Open

fix(cli): print exit summary before cleanup so watchdog can't swallow it#72164
adurham wants to merge 2 commits into
NousResearch:mainfrom
adurham:upstream-pr/exit-summary-before-cleanup

Conversation

@adurham

@adurham adurham commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a bug where the CLI's exit summary (cost report + --resume <id>
hint) can be silently swallowed if cleanup takes long enough to trip the
exit watchdog.

Root Cause

_run_cleanup() was called BEFORE self._print_exit_summary() on both
interactive-exit paths in cli.py's run(). _run_cleanup() includes a
memory provider's on_session_end hook (can be a network-bound call for
external memory providers) and shutdown_mcp_servers() (can block during
MCP server teardown), both guarded by a 30s exit watchdog
(_arm_exit_watchdog) that calls os._exit(0) if cleanup hasn't returned
in time. If the watchdog fires while still inside _run_cleanup(), the
process is killed before any code written AFTER _run_cleanup() ever
runs — silently swallowing the cost report and resume hint with zero
user-visible error.

Changes Made

  • Extracted a shared HermesCLI._finish_interactive_exit(*, release_session=False)
    helper (print summary → cleanup → optionally release the session) and
    wired both interactive-exit call sites in run() to it — the
    stdin-unavailable early return and the main finally-block exit path —
    so the ordering can't drift out of sync between the two sites again.
  • Bumped the exit watchdog's default timeout from 30s to 60s for
    headroom against realistic worst-case cleanup time.
  • Fixed a second, separately-hardcoded 30s default in
    _arm_exit_watchdog_on_shutdown_signal (the SIGTERM/SIGHUP-armed
    backstop) that would otherwise leave its "2x headroom over the normal
    cleanup timeout" computed from a stale base — caught by the new
    regression test's explicit assertion on the doubled value (120s, not
    60s).
  • Replaced a pre-existing regression test that read cli.py's raw source
    text and pattern-matched call order via regex with a real behavioral
    test exercising _finish_interactive_exit() directly (mocking
    _print_exit_summary/_run_cleanup/_release_active_session and
    asserting invocation order), matching the established pattern already
    used in test_exit_watchdog_signal_arm.py in the same directory.

Related Issue

No existing issue found (searched gh search issues for "exit watchdog
fired cost report missing", "resume hint swallowed exit", "cli exit cost
summary missing" — no hits).

Type of Change

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

How to Test

  1. Set HERMES_EXIT_WATCHDOG_S=1 and configure a memory provider whose
    on_session_end hook takes longer than 1s (or just observe the
    ordering change directly).
  2. Before this fix: a slow-enough cleanup step could trip the watchdog
    before the cost report ever printed.
  3. After this fix: the cost report and resume hint always print first,
    regardless of how long cleanup takes afterward.
  4. pytest tests/cli/test_exit_summary_before_cleanup_ordering.py tests/cli/test_exit_watchdog_signal_arm.py -q — 10 passed.
  5. Also ran adjacent exit/cleanup test files to check for regressions:
    test_cli_active_agent_ref_wiring.py, test_cli_new_session.py,
    test_cli_shutdown_memory_messages.py, test_session_boundary_hooks.py,
    test_single_query_session_finalize.py,
    test_tui_terminal_reset_on_exit.py — 51 passed, 0 failed.

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs/issues (none found)
  • My PR contains only changes related to this fix (the
    _finish_interactive_exit extraction is coupled to this fix
    because it's what makes the ordering invariant testable without a
    source-regex test — flagging this explicitly since it's a small
    refactor riding along with the bugfix, not unrelated scope)
  • I've run the relevant test suite and all tests pass
  • I've added tests for my changes
  • Tested on macOS

adurham added a commit to adurham/hermes-agent that referenced this pull request Jul 26, 2026
…Research#72164), close out Bucket A

Documents the final Bucket A item: split the 3-entry "exit-summary/cost-
accounting" cluster into a portable piece (watchdog ordering, submitted
as PR NousResearch#72164) and two entirely fork-only pieces (memory-confirm/curator
cost tracking, which touch files that don't exist upstream at all).

The fix required extracting a shared _finish_interactive_exit() helper
to de-duplicate two call sites, which also made the ordering invariant
unit-testable and let the original fix's source-text-regex test (banned
per this project's own testing rules) be replaced with a real behavioral
one. That extraction caught a genuine second bug the original fork
commit missed: a separately-hardcoded 30s default in
_arm_exit_watchdog_on_shutdown_signal left out of the 30->60s bump.

This closes out Bucket A -- every item from the original 2026-07-25 audit
has now been checked, with a mix of PRs filed, items ruled out with
documented reasoning, and a couple flagged for later hand-reconciliation
where the desktop app's fast churn made a clean patch impractical today.

Saved verified patch to .upstream-candidates/ for reference.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels Jul 26, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused exit-path fix. The premise remains valid on current main: both interactive paths invoke cleanup before printing the summary (cli.py:17157-17158, cli.py:17318-17320), while cleanup arms a watchdog (cli.py:1169-1172) whose timer can terminate the process with os._exit(0) (cli.py:1103-1108).

The shared helper in PR head 6c46afa0b3dd changes both affected paths and its behavioral tests cover summary-before-cleanup, optional lease release, and print failures. The single-query sibling already prints its summary before final cleanup (cli.py:17936-17938), so no matching unaddressed path was found.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
adurham added a commit to adurham/hermes-agent that referenced this pull request Jul 31, 2026
…s on all 7 PRs

Documents the real external engagement on the 7 upstream PRs filed
2026-07-26, and the 3 substantive follow-up fixes pushed in response:

- NousResearch#72054 closed as superseded, but merged anyway via NousResearch#74139 (contributor
  CrowLoki's reconciliation with NousResearch#62026, credited via Co-authored-by).
- NousResearch#72087, NousResearch#72151, NousResearch#72152, NousResearch#72153, NousResearch#72155, NousResearch#72164 all reviewed by the
  repo's automated sweeper -- keep_open/high on all 6.
- Fixed NousResearch#72087 (payload-proportional test assertions, catching a future
  allowlist-regression risk flagged by both the sweeper and an
  independent contributor who measured it precisely on their own fork).
- Fixed NousResearch#72152 (extracted ProfileRail's focus/visibilitychange wiring
  into a tested hook, matching the directory's own established
  use-profile-prewarm.ts pattern).
- Rebased NousResearch#72155 past a real merge conflict (an unrelated upstream
  test-pruning pass removed 3 tests my diff's context touched).

All fixes verified by simulating the exact regression each review was
warning about and confirming the new tests catch it, then restoring the
real fix.

Also noted a real environment issue found this session: the `upstream`
remote's SSH URL intermittently fails to connect from this network; a
one-off HTTPS fetch into a separate ref works around it without touching
the configured remote.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adurham
adurham force-pushed the upstream-pr/exit-summary-before-cleanup branch from 6c46afa to 002f5ec Compare August 20, 2026 23:48
adurham and others added 2 commits August 20, 2026 18:53
_run_cleanup() was called before self._print_exit_summary() on both
interactive-exit paths. _run_cleanup() includes a memory provider's
on_session_end hook (can be a network-bound call for external providers)
and shutdown_mcp_servers() (can block during MCP server teardown), both
guarded by a 30s exit watchdog that os._exit(0)s the process if cleanup
hasn't returned in time. If the watchdog fires mid-cleanup, the process
is killed before any code written after _run_cleanup() ever runs --
silently swallowing the cost report / --resume hint with zero
user-visible error.

Fix: extract a shared HermesCLI._finish_interactive_exit() helper
(print summary, then cleanup, then optionally release the session) and
call it from both interactive-exit sites in run() -- the stdin-unavailable
early return and the main finally-block exit path -- so the ordering
can't drift out of sync between them. Also bumps the exit watchdog's
default timeout from 30s to 60s for headroom, and fixes a second,
separately-hardcoded 30s default in _arm_exit_watchdog_on_shutdown_signal
that would otherwise leave the signal-armed backstop computing its "2x
headroom" from a stale base -- caught by the new regression test's
explicit assertion on the doubled value.

Replaced the pre-existing regression test (a source-text regex reading
cli.py and pattern-matching call order across the file) with a real
behavioral test that exercises _finish_interactive_exit() directly via
mocking, matching this file's own established pattern in
test_exit_watchdog_signal_arm.py. Source-regex tests test the shape of
the code, not its behavior, and can't survive a refactor with unchanged
behavior -- exactly what the extraction here would have broken.

Tests: tests/cli/test_exit_summary_before_cleanup_ordering.py (4 tests,
rewritten) + tests/cli/test_exit_watchdog_signal_arm.py (1 assertion
updated for the new default) -- 10 passed. Also ran adjacent exit/cleanup
test files to check for regressions: test_cli_active_agent_ref_wiring.py,
test_cli_new_session.py, test_cli_shutdown_memory_messages.py,
test_session_boundary_hooks.py, test_single_query_session_finalize.py,
test_tui_terminal_reset_on_exit.py -- 51 passed, 0 failed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
External review (Fable) caught a real regression in the original commit:
_finish_interactive_exit() called _print_exit_summary() then _run_cleanup()
with no exception handling between them. If _print_exit_summary() raised
(a bare print() can raise BrokenPipeError on a broken stdout pipe, e.g.
piping to `head`), _run_cleanup() -- and the exit watchdog arm inside it
-- would never run at all. That trades the original "summary silently
swallowed" bug for a worse "cleanup and watchdog never run" one.

Wrap the print step in try/finally so _run_cleanup() (and session release)
always run regardless of what happens during the print, while the
original exception still propagates after cleanup completes.

Added 2 regression tests confirming cleanup and session release still run
when _print_exit_summary() raises -- both fail against the pre-fix (no
try/finally) code, confirmed via a scripted revert.

Tests: 12 passed (10 existing + 2 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adurham
adurham force-pushed the upstream-pr/exit-summary-before-cleanup branch from 002f5ec to 67a55e7 Compare August 20, 2026 23:53
adurham pushed a commit to adurham/hermes-agent that referenced this pull request Aug 21, 2026
…ate for all 8 open PRs

Found NousResearch#72164 had also gone conflicting (same class of drift as NousResearch#82095):
upstream independently added a from_signal kwarg but left one existing
test assertion stale. Fixed that assertion plus a second occurrence in
this PR's own new test file, verified 64/64 relevant tests, rebased,
pushed. Diagnosed a CI e2e flake as environment noise (not a real
regression -- byte-identical file, 5/5 passes locally) rather than
chasing it blindly; retriggered CI via a fresh rebase+push since we
lack gh run rerun admin rights. All 8 open PRs now confirmed
mergeable/CI-clean -- genuinely review-ready, waiting only on a
NousResearch maintainer (confirmed we have no push/merge rights).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants