Skip to content

fix(gateway): bound SIGTERM post-interrupt grace - #65084

Closed
embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/bound-sigterm-interrupt-grace
Closed

embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/bound-sigterm-interrupt-grace

Conversation

@embwl0x

@embwl0x embwl0x commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the fixed five-second post-interrupt wait with a one-second default for unexpected SIGTERM shutdowns
  • expose the finite, non-negative value as gateway.signal_interrupt_grace_timeout
  • preserve the historical five-second unwind grace for planned stops and restarts
  • keep compatibility with duck-typed runners used by integrations and tests

Current-main behavior

A stuck entry in GatewayRunner._running_agents can consume a fixed five seconds after the gateway has already decided to interrupt remaining work, delaying adapter teardown and contributing to systemd timeout kills. The signal-only grace is independently bounded; planned lifecycle paths retain their existing behavior.

Sibling-path audit

  • unexpected signal, planned stop, and restart paths are distinguished explicitly
  • zero, malformed, and non-finite values are covered
  • session resume, clean-shutdown markers, adapter teardown, and restart draining remain covered

Verification

  • rebased onto exact current main 8b09a9df8476010a78e86d6c32254b4ac14a8c4f
  • 90 focused gateway shutdown, restart-drain, and config tests passed; 2 Windows-only cases skipped on macOS
  • Ruff, lock validation, git diff --check, public identity/privacy checks, gitleaks, and the existing-PR publish gate passed

Current head: 2a18a1705a1711de109368148ecea3ffec16d809

Closes #64155

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused shutdown-path fix. The premise remains present on current main: agent.restart_drain_timeout defaults to 0 (hermes_cli/config.py:1011), _drain_active_agents() immediately returns timed out for active work at that value (gateway/run.py:5810-5811), and GatewayRunner.stop() then waits a fixed five seconds before adapter teardown (gateway/run.py:8361-8364).

The proposed conditional grace matches the existing signal classification: only non-planned signals set runner._signal_initiated_shutdown (gateway/run.py:21165-21172), so planned stop and takeover behavior retains its existing path. The new setting is in config.yaml, not a new user-facing environment variable, and the PR's required GitHub checks are passing.

No blocking defects identified in this read-only review. This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 16, 2026
@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@embwl0x

embwl0x commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the current-main conflict at 4b3693b1a956dd792c36f532c5b08dce2936c13e.

  • Preserved the bounded unexpected-SIGTERM grace and replayed its default into the new canonical hermes_cli/config_defaults.py owner after the config extraction.
  • Preserved current main's gateway shutdown, session, and MCP lifecycle changes.
  • Local: 64 focused tests passed on the final head; the expanded shutdown/config/session matrix had 566 passes and one macOS forensic-helper failure that reproduced identically on untouched current main (29 pass, same one fail).
  • Ruff, Windows-footgun scan, lock check, publish gate, diff check, and gitleaks passed.
  • GitHub reports the PR mergeable, and All required checks pass is green.

@embwl0x

embwl0x commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the new current-main conflict at 297a028e81b5a026ede8de58ffd61513a2bb3aaa.

  • Merged current origin/main (8eb06e75b9dbf29dfad90683a8efef546e0058e0) and preserved its intentional gateway-test pruning; the four signal-grace regressions now live in focused tests/gateway/test_signal_interrupt_grace.py coverage instead of restoring removed legacy blocks.
  • Preserved the bounded unexpected-SIGTERM grace and current main's shutdown/session behavior.
  • Final-head checks: 98 focused shutdown/config tests passed; Ruff, Windows-footgun scan, lock check, diff check, publish gate, and gitleaks passed.
  • The earlier expanded lifecycle matrix had 151 passes and one macOS forensic subprocess failure; the then-current exact main reproduced the identical failure. Current main now includes the unrelated test-baseline corrections.
  • GitHub reports the PR mergeable. Replacement CI run 30514260159 failed during workflow initialization with zero jobs and no logs; contemporaneous runs 30514262881, 30514279821, and 30514231055 show the same repo-wide zero-job failure, so there is no branch test failure to correct.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from 297a028 to e384b6c Compare August 2, 2026 09:37
@embwl0x

embwl0x commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed onto exact current main 0a62610f1 at head e384b6ca2, including the current config_defaults.py ownership introduced since the original branch.

Post-rebase verification is green: 29 focused signal/shutdown/restart tests and Ruff passed. A wider macOS run also exposed the unchanged current-main shutdown_forensics test's dependency on GNU timeout; this branch does not touch that module or test. Replacement CI is running.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from e384b6c to 987cb1f Compare August 3, 2026 07:44
@embwl0x

embwl0x commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the new current-main conflict at 987cb1f23498e71a5caed80e6b49c5c848442745.

  • Replayed the single authored fix onto exact origin/main d1afa16053a3777849c2b5465d59a0147b2172f9.
  • Preserved current main's in-band restart_after_turn_timeout lifecycle and off-loop delivery-ledger work while retaining the bounded unexpected-signal post-interrupt grace.
  • Local validation: 139 focused/adjacent shutdown, restart, signal, async-session, resume-pending, transcript-flush, cache-cleanup, and cron-drain tests passed; the final pre-push focused rerun added 30 passes.
  • Ruff, uv lock --check, diff checks, the existing-PR publish gate, and gitleaks passed.
  • GitHub reports MERGEABLE; all eight Python slices, E2E, required lint/security/attribution checks, and All required checks pass are green. The two non-required Docker builds are still running.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Three PRs address or reference #64155. #65084 targets the gateway-level fixed post-interrupt delay before teardown, #64352 adds bounded WhatsApp bridge reaping after teardown begins, and #13270 covers Windows-specific bridge process-tree termination.

Related pull requests

  • fix(whatsapp): kill bridge process tree on Windows disconnect #13270 [merged] related — (+60/-10) — n/a: The diff changes Windows bridge shutdown to terminate the full process tree while leaving the POSIX path unchanged; although already merged, it remains relevant as the Windows reference implementation for bridge cleanup rather than a fix for the reported Debian pre-teardown delay.
  • fix(whatsapp): reap bridge process on disconnect #64352 [closed] partial — (+150/-7) — n/a: The diff adds bounded wait/force-terminate/wait handling to reap the managed WhatsApp child after adapter teardown starts, but does not change the gateway's preceding post-interrupt grace. Although closed, it remains relevant as a complementary subprocess-reaping approach superseded for this issue's primary cause by fix(gateway): bound SIGTERM post-interrupt grace #65084.
  • fix(gateway): bound SIGTERM post-interrupt grace #65084 best fix — (+196/-4) — n/a; recorded best fix: The diff replaces the fixed five-second post-interrupt grace with a configurable one-second default only for unexpected signal shutdowns, allowing adapter, bridge, and database teardown to start sooner while preserving the five-second planned stop/restart path. This supports the visible keep_open automated review, which found the current-main premise intact and no blocking defect.

Suggested consolidation

Keep #65084 open with a salvage path: retain its bounded unexpected-signal grace, typed configuration parsing, and focused shutdown regressions as the recorded best fix for the gateway-level cause, consistent with the automated keep_open verdict. Keep #64352 closed as a complementary but superseded partial approach rather than a duplicate, and retain merged #13270 only as the Windows process-tree cleanup reference.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I64155(["issue #64155 (open)"])
    P65084["PR #65084 (open)"]
    P65084 -->|best fix| I64155
    class I64155 open
    class P65084 open
    class P65084 best
    class P65084 target
    click I64155 "https://github.com/NousResearch/hermes-agent/issues/64155"
    click P65084 "https://github.com/NousResearch/hermes-agent/pull/65084"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 27 kB of PR diffs, 9 kB of issue/PR text, 4 kB of discussion (6 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from 987cb1f to a66976e Compare August 7, 2026 22:42
@embwl0x

embwl0x commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the current-main conflict at a66976ee96ef07b0bcb0d531510d63c7bfbda4b7.

  • Replayed the single authored fix onto exact origin/main b3aa561faffd64f05436e429a6415d175e534ec9.
  • Preserved current main's API-server settle-window accounting and late-work re-interrupt while applying the configurable short grace only to unexpected signal shutdowns.
  • Local validation: 205 focused/adjacent gateway lifecycle and config tests passed; the final pre-push shutdown/API boundary rerun added 47 passes.
  • Ruff, byte compilation, uv lock --check, Windows-footgun scan, diff checks, the existing-PR publish gate, and gitleaks passed.
  • GitHub reports MERGEABLE; replacement CI run 31224739342 is fully green, including all 12 Python slices, E2E, required lint/security/attribution gates, and both Docker architectures.

@embwl0x

embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict in d5c6178dbe4153541aef09f9956a4bd15e26d61d.

The single authored fix is replayed onto exact upstream main at c0106e50e7ecedb3ce34e785d949725dc4e0e457. The conflict resolution preserves main’s new gateway.multiplex_profile_allowlist default alongside this PR’s bounded unexpected-signal interrupt grace; current shutdown, API settle-window, restart, and adapter teardown behavior remains intact.

Local verification on the pushed head: 157 tests passed across gateway shutdown, restart drain/after-turn/resume, API active-work drain, bounded adapter teardown, shutdown cache/flush, async session storage, and config; two Windows-only cases were skipped locally for their CI lane. Ruff, uv lock --check, diff checks, the existing-PR publish gate, and gitleaks passed. Replacement CI is running.

@embwl0x

embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

The current CI failure is an upstream-main baseline regression, not a failure in this PR's shutdown change.

  • Slice 5 fails only tests/gateway/test_multiplex_busy_input_mode.py::test_profile_route_and_nonmultiplexed_resolution_preserve_boundaries: the test expects steer, but the new served-profile gate correctly rejects research because the fixture does not mark it served.
  • The same 1-failure/14-pass result reproduces in a clean detached worktree at this PR's exact base, c0106e50e7ecedb3ce34e785d949725dc4e0e457; main's own CI run 31470991542 is red on that base.
  • Focused correction fix(tests): mark routed profile as served in busy-mode test #83745 updates only that stale fixture, is mergeable, and its full required CI is green.

I am keeping #65084 scoped rather than importing the unrelated baseline test repair. Once #83745 lands, this branch can be refreshed onto the corrected main to trigger replacement CI.

@embwl0x

embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

CI classification for the replacement head d5c6178dbe41: slice 5 has one failure, test_profile_route_and_nonmultiplexed_resolution_preserve_boundaries. I reproduced the identical assertion failure on the exact base (c0106e50e7ec) and on this PR head; both reject the fixture route because the test never marks research as a served profile.

This PR does not change that profile-routing contract. The focused upstream test correction is #83745, whose required CI is green. I am leaving this branch scoped to SIGTERM shutdown behavior rather than copying an unrelated test repair; once #83745 lands, rebasing/rerunning will clear this inherited failure.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from d5c6178 to d52f2c6 Compare August 12, 2026 13:20
@embwl0x

embwl0x commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed onto exact current main (222465d84) now that the shared busy-mode fixture correction is upstream.

  • New head: d52f2c6fc
  • Local shutdown/config/session matrix: 98 passed, 2 Windows-only skipped
  • Ruff, diff check, public identity, publish gate, and gitleaks: passed
  • Replacement GitHub CI: 33 passed checks, 12 intentional skips, zero failures or pending checks

The SIGTERM-only grace behavior remains scoped from planned stop/restart handling, and GitHub reports the PR mergeable.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from d52f2c6 to d68a958 Compare August 14, 2026 10:53
@embwl0x

embwl0x commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict at d68a95816642657332b7b1459cc86a0ff12165ce.

  • Replayed the single authored fix onto exact upstream main at 56a41715dc3b8bf6f50a740ff9416c4036ef4259.
  • Preserved current main’s completion-batch shutdown ordering regression while retaining this PR’s bounded unexpected-SIGTERM grace and its focused tests.
  • Local validation: 211 gateway lifecycle/config/session tests passed with 2 Windows-only skips; the final pre-push boundary rerun added 29 passes. Ruff, byte compilation, uv lock --check, Windows-footgun scan, diff checks, the existing-PR publish gate, and gitleaks passed.
  • GitHub now reports the PR mergeable. Eleven Python slices, E2E, both platform lanes, lint/security/attribution gates, and both Docker architectures passed.
  • Slice 3 fails only tests/tui_gateway/test_slash_worker_profile_home.py::test_slash_worker_accepts_profile_home; the identical failure reproduces on this PR’s untouched exact base because its get_hermes_home() mock returns str instead of the production Path. Focused test-only correction fix(tests): return Path from get_hermes_home mock in slash_worker profile test #85998 fixes that baseline regression, so this branch remains scoped to SIGTERM shutdown behavior.

@alt-glitch alt-glitch removed the sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state label Aug 14, 2026
@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from d68a958 to c2f49e0 Compare August 14, 2026 18:36
@embwl0x

embwl0x commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict at c2f49e03fa076549db94f2e08e6c88cd4f10706b.

  • Replayed the single authored fix onto exact upstream main at a90d5369f76c87c98547d2e283aa26d5cfabf322.
  • Preserved current main’s wedged-turn restart exclusion, worker-stack diagnostics, and their focused regressions while retaining this PR’s bounded unexpected-SIGTERM grace. The earlier slash-worker Path baseline correction is now upstream.
  • Local validation: 163 adjacent gateway shutdown/restart/session tests passed, including the former CI failure; the final pre-push boundary rerun added 28 passes. Ruff, byte compilation, uv lock --check, Windows-footgun scan, diff checks, the existing-PR publish gate, and gitleaks passed.
  • Replacement GitHub CI is fully green: all 12 Python slices, E2E, Windows/macOS, lint/type/security/attribution gates, aggregate required checks, and both Docker architectures passed (32 passes, 13 intentional skips, zero failures or pending checks).
  • GitHub reports the PR mergeable and clean.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from c2f49e0 to 337d596 Compare August 15, 2026 08:17
@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict at 337d596debef572a0b0bc9ff83e8e4610cf833fc.

  • Replayed the single authored fix onto exact upstream main at 77be513de1da24610ebe8d1d4848228578c6bdf3.
  • Preserved current main's cron-only drain floor and its shutdown-test double contract while retaining this PR's independently configured, bounded unexpected-SIGTERM post-interrupt grace.
  • Local validation: 177 adjacent gateway shutdown/restart/session/cron tests passed with 2 Windows-only skips; the final focused boundary rerun added 44 passes with the same 2 platform skips. Ruff, byte compilation, uv lock --check, Windows-footgun scan, diff checks, the existing-PR publish gate, and gitleaks passed.
  • Replacement GitHub CI is fully green: all 12 Python slices, E2E, Windows/macOS, lint/type/security/attribution gates, aggregate required checks, and both Docker architectures passed (32 passes, 13 intentional skips, zero failures or pending checks).
  • GitHub reports the PR mergeable and clean.

@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict at 337d596debef572a0b0bc9ff83e8e4610cf833fc.

  • Replayed the single authored fix onto exact upstream main at 77be513de1da24610ebe8d1d4848228578c6bdf3.
  • Preserved current main's cron-only drain floor and its test-double contract while retaining this PR's independent bounded unexpected-SIGTERM post-interrupt grace.
  • Local validation: 177 adjacent gateway shutdown/restart/session/API/cron tests passed; the final pre-push boundary rerun added 28 passes. Ruff, byte compilation, Windows-footgun scan, uv lock --check, diff checks, the existing-PR publish gate, and gitleaks passed.
  • Replacement GitHub CI is fully green: all 12 Python slices, E2E, Windows/macOS, lint/security/attribution gates, aggregate required checks, CI timing, and both Docker architectures passed (32 successes, 12 intentional skips, one neutral check, zero failures or pending checks).
  • GitHub reports the PR mergeable; the conflict signal is cleared.

@Enough1122

Copy link
Copy Markdown
Contributor

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

fix(gateway): bound SIGTERM post-interrupt grace

  1. gateway/run.py — the new default of 1s for the unexpected-SIGTERM grace is aggressive: _interrupt_running_agents is cooperative, and an agent mid-way through a DB/file operation may not unwind within 1s, so teardown then races that operation. It's configurable and documented to stay under the service-manager stop budget, but consider whether a slightly larger default (or a warning log when _running_agents is still non-empty at the deadline) would be safer for the common self-hosted case. The new info log of the chosen grace is a good start.

  2. gateway/run.py _post_interrupt_grace_timeout — the logic (signal-initiated and not restart-requested → configured grace, otherwise 5.0) is clear and the parametrized test covers all three branches, including the restart-requested signal case keeping the old 5.0. Good.

  3. Minor: parse_signal_interrupt_grace_timeout parses the value, and _load_signal_interrupt_grace_timeout re-parses it (float(raw)) purely to decide whether to log a warning — the parse could return a sentinel or the loader could log inside the parser to avoid the double parse and the duplicated except (TypeError, ValueError).

  4. Minor: DEFAULT_GATEWAY_POST_INTERRUPT_GRACE_TIMEOUT = 5.0 now names the previously inline constant; consider deriving both the planned-stop and signal paths from a single source so they can't drift.

No blocking issues found.

@embwl0x
embwl0x force-pushed the agent/bound-sigterm-interrupt-grace branch from 337d596 to 2a18a17 Compare August 23, 2026 06:16
@embwl0x

embwl0x commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the renewed current-main conflict at 2a18a1705a1711de109368148ecea3ffec16d809.

  • Replayed the single authored fix onto exact upstream main 8b09a9df8476010a78e86d6c32254b4ac14a8c4f.
  • Preserved main's planned service-restart regression alongside this PR's unexpected-signal grace coverage.
  • Local verification: 90 focused shutdown, restart-drain, and config tests passed with 2 Windows-only skips; Ruff, lock/diff checks, publish gate, and gitleaks passed.
  • Replacement CI is running.

@alt-glitch alt-glitch added the area/config Config system, migrations, profiles label Aug 23, 2026
@embwl0x

embwl0x commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Replacement CI is fully green on 2a18a1705a1711de109368148ecea3ffec16d809: Python and E2E tests, native Windows/macOS lanes, Ruff/ty and Windows-footgun enforcement, security/attribution/history gates, both Docker architectures, Nix, and All required checks pass all succeeded.

@alt-glitch alt-glitch added P1 High — major feature broken, no workaround and removed P2 Medium — degraded but workaround exists labels Aug 23, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via salvage PR #99663 (commit 2213128 on main) — your commit was cherry-picked onto current main with your authorship preserved in git log. This covers the gateway half of #64155; the other half (the blocking MCP shutdown wait, #82874) is in flight as PR #99675. Thanks for the clean, well-tested fix and for keeping the branch rebased!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes-gateway: slow/hanging graceful shutdown on SIGTERM causes systemd timeout

5 participants