Skip to content

fix(plugins): stop the hook slot from latching off for the life of the process - #107894

Closed
deadczarvc wants to merge 2 commits into
NousResearch:mainfrom
deadczarvc:fix/hook-slot-latch
Closed

deadczarvc wants to merge 2 commits into
NousResearch:mainfrom
deadczarvc:fix/hook-slot-latch

Conversation

@deadczarvc

Copy link
Copy Markdown

Problem

The hook enforcement layer can latch itself off for the life of the process.

  1. _run_callback_with_timeout waits timeout for the worker; on expiry it logs, records
    _hook_timeout_suppressed_until[callback_key] and returns without freeing the slot.
    The abandoned worker never reaches _release_token, so callback_key stays in
    _hook_running_callbacks forever. Every later call of that hook fails closed — not for the
    suppression window, but until the process restarts.
  2. The running/suppression key is (hook_name, id(cb)) — it ignores the tool. A slow shell
    hook that fired for tool A therefore rejects unrelated tool B whose own matcher never ran.

Observed in the field: one stalled callback produced 459 hook skips and 15 blocked tool
calls over 2 hours
, including calls that never matched the stalled hook's own matcher.

Repro (hermetic)

tests/hermes_cli/test_plugins.py::test_slow_hook_for_one_tool_does_not_block_an_unrelated_tool
registers a hook that never returns for one tool and asserts an unrelated tool still passes.

Fix

  • free the slot on the timeout path (_release_token(callback_key)) so the latch is bounded by
    the suppression window instead of the process lifetime;
  • scope the key by tool_name for tool-bearing events (tool-less events keep the old key).

Fail-closed behaviour and "same tool does not run two workers" are preserved.

Verification

  • new regression test passes; existing tests/hermes_cli/test_plugins.py suite passes;
  • live environment, after backend restart: 0 blocked tool calls and 0 skips in the
    observation window, against 478/hour before the fix;
  • a separate invariant check now asserts that a shell hook's own timeout is strictly below
    plugins.hook_callback_timeout, so the abandoned-worker path is not reachable in normal
    operation (our_max * 1.2 <= kernel, with threaded path disabled as an explicit escape).

Two halves of one outage in the enforcement layer:
- an abandoned timeout worker never reached _release_token, so its key latched
  "still running" for the life of the process and every later call of that hook
  failed closed (observed: 100 skips and 15 blocked tool calls over 2h)
- the running/suppression key ignored the tool, so a slow hook for one tool
  rejected unrelated tools whose matcher never ran

Regression: tests/hermes_cli/test_plugins.py::test_slow_hook_for_one_tool_does_not_block_an_unrelated_tool
Live check after backend restart: 0 blocked tool calls.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins comp/cli CLI entry point, hermes_cli/, setup wizard labels Sep 11, 2026
@deadczarvc

Copy link
Copy Markdown
Author

Related, already merged upstream: #94339 (un-invert the stdio children liveness check) fixed the polarity half of the same family, and #96452 salvaged the reconnect-on-fast-fail half of our previous report #95626 (our commit was cherry-picked there with authorship preserved).

This PR is the next defect in that same lifecycle: the hook enforcement layer latches itself off. Unlike the MCP path, here the abandoned worker never releases its slot, so the first timeout poisons every later call of that hook for the life of the process — including tools whose matcher never ran.

Both halves are covered by a regression test; the invariant check (our hook timeout * 1.2 <= kernel timeout) is stated in the body because it makes the abandoned-worker path unreachable in normal operation.

kshitijk4poor pushed a commit that referenced this pull request Sep 15, 2026
…alone

Concurrent invocations of the same tool in one session collapsed into a single
busy key (hook_name, id(cb)): the second invocation was reported as 'still
running' and dropped. For pre_tool_call a drop is a fail-closed block, so the
gate silenced itself on an ordinary, healthy callback.

Measured on a busy profile: 3574 skip lines and 0 timeout lines in one hour —
every skip was the 'while still running' branch, i.e. pure key collision, not
slowness.

The gate now keys on the call identity that is already in the payload
(tool_call_id, else turn_id, else none — the last case behaves exactly as
before). Suppression stays keyed coarsely on (hook_name, id(cb)): a hung
callback is a fact about the callback, so its back-off must not be diluted
per call.

Refs #98382. Independent of #107894 (that one releases the slot on timeout;
this one stops healthy concurrency from colliding).

(cherry picked from commit 53b3dac)
karljohannisson pushed a commit to karljohannisson/hermes-agent that referenced this pull request Sep 15, 2026
…alone

Concurrent invocations of the same tool in one session collapsed into a single
busy key (hook_name, id(cb)): the second invocation was reported as 'still
running' and dropped. For pre_tool_call a drop is a fail-closed block, so the
gate silenced itself on an ordinary, healthy callback.

Measured on a busy profile: 3574 skip lines and 0 timeout lines in one hour —
every skip was the 'while still running' branch, i.e. pure key collision, not
slowness.

The gate now keys on the call identity that is already in the payload
(tool_call_id, else turn_id, else none — the last case behaves exactly as
before). Suppression stays keyed coarsely on (hook_name, id(cb)): a hung
callback is a fact about the callback, so its back-off must not be diluted
per call.

Refs NousResearch#98382. Independent of NousResearch#107894 (that one releases the slot on timeout;
this one stops healthy concurrency from colliding).

(cherry picked from commit 53b3dac)
@deadczarvc

Copy link
Copy Markdown
Author

Closing as superseded by upstream work.

The latch half is fixed in main: _release_token() now clears the running slot and _hook_abandoned tracks a timed-out worker, so one stalled callback no longer latches "still running" for the life of the process (see fix(plugins): keep one worker per callback while a timed-out worker is still running).

The scoping half is covered too, by a different mechanism: the gate key is now (hook, callback, _hook_call_identity(kwargs)), where the identity is the tool_call_id (falling back to turn_id). Two concurrent calls — including calls of different tools — no longer collapse into one key, which is the outcome this PR argued for; the per-tool-name key I proposed is a narrower variant of the same idea.

Thanks for landing it. I am closing this PR rather than rebasing a narrower duplicate.

@deadczarvc deadczarvc closed this Sep 17, 2026
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 comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants