Skip to content

fix(hermes_cli): bound plugin hook callbacks without blocking on timeout (#85125 2d, salvage of #76822) - #93824

Merged
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/76822-hook-deadline
Aug 27, 2026
Merged

kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/76822-hook-deadline

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Contributor

#85125 Phase 2d. Python plugin hook callbacks ran synchronously on the agent turn hot path with no bound — one wedged callback (network call in an observer, deadlocked lock in a policy hook) froze the conversation loop indefinitely (#76821, #10048). Hook callbacks on the hot-path allowlist are now bounded by plugins.hook_callback_timeout (default 30s, 0 disables, clamped at 600), with the worker abandoned, never joined — joining is exactly the #6622 ThreadPoolExecutor-shutdown hang this design avoids.

Based on #76822 by @fangliquanflq — cherry-picked to preserve authorship. The PR was a 4-commit stack ~2,600 commits behind main; the tip commit (which contains the final shape of all four) was cherry-picked and the content of the earlier commits that the tip didn't carry (config default, 4 test functions) was restored from the PR diff during conflict resolution, all under his authorship.

Design (his, preserved)

  • Allowlist, not blanket: only hot-path observers (post_tool_call, pre_llm_call, transform_*, pre/post_api_request, session-boundary hooks, …) and the policy hook pre_tool_call are bounded. Teardown/flush hooks (on_session_finalize), policy gates where abandonment is unsafe either way (pre_gateway_dispatch), and observer-only approval/kanban hooks stay unbounded — each exclusion documented in the code.
  • pre_tool_call fails CLOSED: a timed-out or still-running policy callback blocks the tool rather than letting it run without a policy decision.
  • Timeout suppression: a hung callback is suppressed for 60s (and while still running) so a repeatedly-fired stuck hook cannot accumulate abandoned daemon threads.
  • Caller-thread contract preserved: subagent_stop (documented parent-thread serialization) is never moved onto a timeout worker.
  • contextvars propagated to the worker so callbacks see the caller's context.

Salvage-round conflict resolution (main drifted heavily)

  • invoke_hook on main now routes callbacks through _invoke_hook_callback (additive-payload signature filtering for narrow legacy callbacks, feat(plugins): gateway UX observer hooks + capability-gated platform actions #64176-adjacent). The timeout worker routes through the same helper, so signature filtering applies on both the bounded and unbounded paths — the PR's original cb(**kwargs) would have regressed narrow-signature callbacks.
  • Main's multi-profile registration-ledger __init__ and force-rediscovery restructure kept intact; the PR's hook-timeout state cleanly added alongside.
  • gateway_platform_event's telemetry-marker exemption (feat(plugins): gateway UX observer hooks + capability-gated platform actions #64176) preserved.
  • Docs merged into the current hooks.md structure (catalog/correlation bullets kept; timeout + fail-closed paragraphs added).

Verification

  • tests/hermes_cli/test_plugins.py + tests/agent/test_subagent_stop_hook.py: 80 passed — includes his 8 new tests: caller-not-blocked (the fix: error context preservation, WAL checkpoint, hook timeout #6622 regression shape), within-timeout value, exception isolation under the timeout path, config read, caller-thread contract, hung-callback suppression, pre_tool_call fail-closed, and an E2E proving a timed-out pre_tool_call blocks handle_function_call before dispatch
  • Live wedged-hook probe: hung observer + healthy sibling → caller returns in 0.30s, hung result skipped (not fabricated), healthy result kept; hung pre_tool_call → block directive in 0.30s; on_session_finalize confirmed synchronous on the caller thread
  • ruff clean; ty diagnostic classes identical to base; config key in DEFAULT_CONFIG + web-schema surfaced (folded into the agent tab, no orphan category)
  • No new env vars (config.yaml only, per policy); no cache impact

Closes #76822. Fixes #76821. Fixes #10048.
Part of #85125 (Phase 2d).

@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 24, 2026 11:29
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 24, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

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

This is the right architecture for #85125's constraints: abandon-without-join avoids the #6622 shutdown hang, the bounded/fail-closed/caller-thread hook taxonomy is explicitly reasoned rather than accidental, and the token-based cleanup means a late-finishing worker can't clobber a newer invocation's bookkeeping. Points:

  1. hermes_cli/plugins.py:5402 — keys are (hook_name, id(cb)). CPython recycles id() after object collection, so a reloaded plugin's new callback can inherit a dead callback's suppression entry (silently skipped up to 60s) or "still running" state. Transient and low-harm, but this exact id-recycling class was just fixed for event-loop semaphores (fix(agent): scope auxiliary semaphores to event loops #93880 territory); a weakref.WeakKeyDictionary-style key (or (hook_name, id, weakref) pair) closes it cleanly.

  2. hermes_cli/plugins.py:5444-5447 — the worker captures only Exception. A callback raising BaseException (SystemExit from misbehaving plugin code is realistic) leaves outcome/failure empty, so the parent treats it as a silent None return — no log, no suppression, every fire spawns a doomed thread. Capture BaseException into failure (re-raising non-Exception in the parent is fine given the outer handler).

  3. Per-invocation threading.Thread spawn on hot-path hooks (post_tool_call, transforms) costs ~100µs plus GIL churn under high tool-call rates. Acceptable versus the hang it prevents, but a tiny reusable executor per manager (with explicit non-join semantics on timeout) would remove the cost without reintroducing fix: error context preservation, WAL checkpoint, hook timeout #6622 — worth an issue, not a blocker.

  4. hermes_cli/plugins.py:3468-3505_resolve_hook_callback_timeout runs on every invoke_hook call; correct only while load_config_readonly stays cached. A one-word comment pinning that assumption prevents a future "read config fresh" refactor from turning every hook dispatch into disk I/O.

Docs (hooks.md) and the negative-value disable/clamp behavior are well covered. The suppression-window design is a nice touch against daemon-thread accumulation.

auto-merge was automatically disabled August 27, 2026 08:51

Pull request was closed

@kshitijk4poor kshitijk4poor reopened this Aug 27, 2026
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 27, 2026 08:52
Allowlist hot-path hooks for abandon-on-timeout, keep subagent_stop on the caller thread, suppress re-fires of hung callbacks, and block tools when pre_tool_call times out.
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/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

4 participants