fix: reap stale background processes to prevent gateway starvation (#76115) - #76172
fix: reap stale background processes to prevent gateway starvation (#76115)#76172JonthanaHanh wants to merge 1 commit into
Conversation
…ousResearch#76115) Add a background sweep thread to ProcessRegistry that kills running background processes exceeding MAX_ACTIVE_PROCESS_AGE (default 24h). Without this, abandoned or stuck tool subprocesses (e.g. pnpm build) run indefinitely, consuming unbounded memory until the gateway cgroup hits MemoryHigh — starving the asyncio event loop and causing all platforms and crons to time out. Changes: - Add STALE_SWEEP_INTERVAL constant (300s / 5 min) - Add _stale_sweep_loop() daemon thread started at registry init - Add _reap_stale_running() method using existing kill_process() which handles process-tree teardown (SIGKILL/SIGTERM POSIX, taskkill /T /F Windows) Fixes NousResearch#76115
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real process-lifecycle gap. The sweep needs policy and lifecycle changes before it can safely land.
Problems
tools/process_registry.py:62currently supplies the reset guard's default only;gateway/config.py:506-511andwebsite/docs/user-guide/messaging/index.md:274-281explicitly permit0or larger values for legitimate long-lived processes. The new hard 24-hour sweep ignores that contract and would kill those processes anyway.- #76115 reports gateway starvation after an approximately 11-hour build, while this diff waits 24 hours before reaping.
- The added
ProcessRegistry.__init__()worker has no stop path._stale_sweep_loop()only exits when its event is set, but the diff never sets it;tests/tools/test_process_registry.py:23-26constructs a registry per test. - No regression tests cover reaping, policy overrides, or worker lifecycle.
Suggested changes
- Define a separate, documented lifecycle policy (or wire the existing configured policy deliberately), make it cover the abandoned-turn failure path, and add a shutdown-safe execution mechanism plus focused regression tests.
Automated hermes-sweeper review.
| target=self._stale_sweep_loop, | ||
| daemon=True, | ||
| name="process-stale-sweep", | ||
| ) |
There was a problem hiding this comment.
This starts one daemon thread per ProcessRegistry instance, but _stale_sweep_loop() only exits when _stale_sweep_stop is set and this diff adds no shutdown path. The per-test ProcessRegistry fixture in tests/tools/test_process_registry.py:23-26 would accumulate retained workers; please use an existing gateway lifecycle scheduler or add explicit stop/join ownership.
|
Closing as duplicate of #76188 (@JoaoMarcos44) — that PR implements turn-scoped reaping (only kills processes the abandoned turn created) with 4 test files and gateway integration, vs the age-based blind sweep here which would kill legitimate long-running background processes older than 24h. You were first to submit (by ~18 min) but #76183 was an identical submission. Thanks for the contribution! |
Summary
Add a background sweep thread to
ProcessRegistrythat kills running background processes exceedingMAX_ACTIVE_PROCESS_AGE(default 24h).Problem
When an agent turn spawns a background tool subprocess (e.g.
pnpm build) and that turn gets stuck or abandoned, the subprocess is never reaped and has no lifetime cap. It keeps running indefinitely. A single runaway build can push the gateway cgroup past itsMemoryHighlimit, starving the asyncio event loop — causing every messaging platform to disconnect and every cron to time out (see #76115 for full analysis).Root Cause
MAX_ACTIVE_PROCESS_AGE(86400s / 24h) is defined inprocess_registry.pyand used byhas_active_for_session()to decide whether a process blocks session reset, but the process itself is never killed — only ignored by the reset guard.Changes
STALE_SWEEP_INTERVALconstant (300s)tools/process_registry.py:63_stale_sweep_loop()daemon threadProcessRegistry.__init__()_reap_stale_running()methodkill_process()for process-tree teardownMAX_ACTIVE_PROCESS_AGEkill_process()infrastructure which handles tree teardown (SIGKILL/SIGTERM on POSIX,taskkill /T /Fon Windows) and moves the session to the finished dictTest Plan
Fixes #76115