fix(plugins): per-session hook single-flight + matcher gate before the flight window - #109441
emirsaffar-collab wants to merge 1 commit into
Conversation
…e flight window Cross-session contention fix for upstream NousResearch#105223: one session's in-flight pre_tool_call callback skipped every other session's identical callback process-wide; because pre_tool_call fails closed, parallel webui sessions blocked each other's tool calls with 'plugin callback timed out or is still running' despite zero real hook timeouts (RCA 2026-09-12). - invoke_hook honors the shell-hook tool matcher BEFORE the single-flight window: a non-matching callback never acquires (nor is skipped for) the flight token. ~17 hooks/tool-call collapse to the 1-3 matching ones. - flight + suppression keys scoped per (hook, callback, session_id) instead of per callback process-wide. Same-session single-flight (hang protection, NousResearch#6622/NousResearch#76821) fully preserved; parallel sessions get their own tokens. Polarity-tested in TestHookSingleFlightContentionUpstream105223 (5 tests: non-matching never contends; same-session skip preserved; parallel sessions independent; matching hooks still dispatched; real shell-hook e2e).
gaoanze888
left a comment
There was a problem hiding this comment.
Moving the shell matcher before admission is sound, but the new per-session key removes the original process-wide resource bound at exact head 32ccc9b18225e597a0c607af6c5d9611ffa60b86.
session_id is request/session controlled. A hung callback can now create one abandoned daemon worker for every distinct session, whereas (hook_name, id(cb)) limited a callback to one worker process-wide. I reproduced one hung callback with 100 unique session IDs producing 100 simultaneously running hook workers. This contradicts the existing invariant that a stuck policy hook cannot spawn a new abandoned thread on every fire. Keep session-level contention isolation, but add a bounded global per-callback worker/admission budget so distinct sessions cannot create unbounded abandoned threads; add a many-session hung-callback regression.
The timeout suppression map has the same cardinality problem: each timed-out (hook, callback, session) remains until that exact session fires again or the manager unloads. Continually new API/gateway session IDs can grow it indefinitely. Please prune expired entries and/or use a bounded cache, with a many-session timeout cleanup test.
Matcher placement itself is correct and retains the callback-side defensive match check; manager scoping also keeps identical session IDs in different profiles separate. Focused plugin/shell tests pass 130/130 with one platform skip; Ruff and diff checks are clean. The current tests cover same-session and cross-session polarity but not bounded global worker/cache behavior. The old (hook_name, id(cb)) wording in plugins.py should also be updated.
|
Partial overlap note: the single-flight half of this (concurrent same-tool calls collapsing on the per-callback key) landed on main via #111177 ( |
|
Closing as superseded by #118844 (#118844), commit Thanks for the contribution — the underlying problem this PR addresses has been resolved on current main. If you believe this was closed in error, please comment and we'll reopen. |
Problem (upstream #105223)
pre_tool_callis a fail-closed policy hook: when its callback is skipped (still running for another fire, or in the 60s post-timeout suppression window), the tool call is blocked. Because single-flight is keyed per(hook_name, callback)process-wide, parallel sessions sharing one process (e.g. an API/webui server that imports AIAgent directly and runs many concurrent sessions) block each other's tool calls whenever their hook runs overlap — even though no callback ever actually hangs.Two amplifiers make it routine rather than rare:
pre_tool_callhook contends on every tool call of every session, matching or not.Observed on a 10+ parallel-session host: 333 skip events in two days, all "skipped ... or while still running" — zero real timeouts. Sessions hit
same_tool_failure_halttool-guardrail halts from the resulting blocked-call streaks.Fix (two minimal, independent changes)
hermes_cli/plugins_dispatch.py): if a callback exposes_hermes_matches_tool(shell-hook callbacks do,agent/shell_hooks.py), non-matching toolscontinuebefore acquiring the flight token. A non-matching hook can never contend (nor be skipped because of) another session's in-flight run.hermes_cli/plugins_dispatch.py):callback_key = (hook_name, id(cb), session_id)— same composition as the shell-hook payload (falls back to""when identity kwargs are absent). Parallel sessions each get their own flight token + suppression window; same-session single-flight (hang protection, one stacked worker per session) is fully preserved.Tests
New
TestHookSingleFlightContentionUpstream105223(5 polarity tests) + synthetic load test:tests/hermes_cli/test_plugins.py83/83 green on this branch (incl. existing timeout/fail-closed/suppression regression tests untouched).Notes
pre_tool_callfail-closed semantics on actual timeout are unchanged — only cross-session false contention is removed._hermes_matches_toolbehave exactly as before (the gate is opt-in via the attribute).