The timeout branch of _run_hook_callback_bounded unconditionally added
gate_key to _hook_abandoned. A worker that finishes between done.wait()
returning False and the caller taking the lock has already popped its
token via _release_token, so nothing would ever clear that entry: the
callback stayed blocked for every later call id until reload with no
thread behind it. Guard the insert on the worker still being registered.
The new test makes the race deterministic by swapping the module's
threading.Event for one whose wait() lets the worker finish and then
reports a timeout, and asserts a fresh call id still runs.
Also pass tool_call_id inline from terminal_tool_result instead of the
conditional dict plumbing: an empty id is already treated as "no
identity" by _hook_call_identity and unknown fields are withheld from
narrow-signature callbacks (same shape as _fire_approval_hook). Update
the stale "(hook_name, id(cb))" comment above _hook_running_callbacks.
Gating hook callbacks by call identity lets two concurrent calls of the
same tool both run their hooks, but it also let a fresh tool_call_id pass
the gate once the 60 s suppression window lapsed even though the previous
worker for that callback never returned. A hung plugin then leaked one
daemon thread per minute for the life of the process; on the old
coarse-keyed gate it leaked exactly one.
Track abandoned-but-running workers per callback: the timeout branch
records the gate key, the worker's own release discards it, and the gate
treats any non-empty abandoned set as "still running" for that callback.
Healthy callbacks keep distinct-id concurrency; hung ones are back to
at most one outstanding worker.
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 53b3dacd008418fcdf5fa6dfcadde575a35a776e)
Slash-command handlers gained loop-safe awaiting in ca9a61ae38, but
`PluginManager.invoke_hook` still called `async def` hook callbacks directly:
the coroutine object was appended to the results (so `pre_llm_call` context
injection silently did nothing) and Python warned "coroutine was never
awaited". `_invoke_hook_callback` now routes every return through
`resolve_plugin_command_result`, which covers both the direct and the
timeout-bounded paths and is safe under the gateway's running loop.
Fixes#12449 (remaining hook half). Salvage of #63240 by @Bartok9, applied
one layer down so the bounded-worker path is covered too.
Co-authored-by: Bartok9 <Bartok9@users.noreply.github.com>
The identity-guarded token pop appeared twice (the runner's finally and the
new worker-start except); a future edit to one copy would silently reintroduce
the sticky-token class. Hoist into a local closure called from both sites, and
extend the _run_hook_callback_bounded docstring with the new skip reason.
Behavior-preserving follow-up on #104651.
For each issue anchor present in BASE 63279301bc non-test .py and absent on HEAD, the BASE comment/docstring block was re-attached at the HEAD location of the code it explained (matched by the distinctive code line / enclosing def). Sentences already covered by an existing HEAD comment were deduped; the issue number always survives. Insert-only: no code lines changed.
