Skip to content

fix(plugins): bound lifecycle hook execution on hot paths (#10048) - #39751

Open
rodboev wants to merge 3 commits into
NousResearch:mainfrom
rodboev:pr/plugin-hook-timeouts
Open

fix(plugins): bound lifecycle hook execution on hot paths (#10048)#39751
rodboev wants to merge 3 commits into
NousResearch:mainfrom
rodboev:pr/plugin-hook-timeouts

Conversation

@rodboev

@rodboev rodboev commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Plugin lifecycle hooks run on hot user-turn and tool-dispatch paths, and the earlier timeout salvage still left three holes: on_session_start and on_session_end were unbounded, pre_tool_call timeouts still failed open, and the shared four-worker pool could be starved by distinct hung callbacks.

This moves the guarantee into the shared Python callback seam. Bounded hooks now run in isolated per-callback daemon workers with context propagation, session-boundary hooks are covered, pre_tool_call timeouts fail closed through the existing resolve_pre_tool_block() path, and observer or transform hooks still fail open after a warning. Hooks that require caller-thread delivery, such as subagent_stop, keep that contract. Shell-hook subprocess timeout behavior is unchanged.

Closes #10048.
Reported by @audioh.

Changes

  • hermes_cli/plugins.py: replace the shared executor path with isolated bounded callback workers, add explicit hook policy groups, cover on_session_start and on_session_end, and return a fail-closed directive for timed or still-running pre_tool_call callbacks.
  • tests/hermes_cli/test_plugins.py: add policy-table, saturation, bounded session-hook, and fail-closed pre_tool_call regressions.
  • tests/agent/test_system_prompt_restore.py: cover the real first-turn on_session_start path under timeout.
  • tests/run_agent/test_run_agent.py: add an end-to-end timed pre_tool_call block before tool dispatch.

Validation

  • Focused suites:
    • python -m pytest tests\hermes_cli\test_plugins.py tests\agent\test_system_prompt_restore.py tests\run_agent\test_run_agent.py tests\agent\test_plugin_llm.py tests\agent\test_subagent_stop_hook.py -v --timeout=0 --timeout-method=thread — 199 passed
  • ruff on changed files — passed
  • Invariant enumeration assert — passed
  • git diff --check — passed

Test plan

  • Run python -m pytest tests\hermes_cli\test_plugins.py tests\agent\test_system_prompt_restore.py tests\run_agent\test_run_agent.py tests\agent\test_plugin_llm.py tests\agent\test_subagent_stop_hook.py -v --timeout=0 --timeout-method=thread

Not in scope

The unrelated .github/workflows/docker.yml residue is excluded from this PR.

@daimon-nous daimon-nous Bot added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins labels Jun 5, 2026
@rodboev
rodboev force-pushed the pr/plugin-hook-timeouts branch from 4ce428c to f332798 Compare June 12, 2026 11:38
@rodboev
rodboev force-pushed the pr/plugin-hook-timeouts branch from 09b4fcd to 6f0b383 Compare June 28, 2026 19:57
@rodboev
rodboev force-pushed the pr/plugin-hook-timeouts branch 3 times, most recently from 6f0b383 to ff9a6e7 Compare July 7, 2026 04:28

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling a real hot-path reliability issue. The inline dispatcher is still present on current main at hermes_cli/plugins.py:1913-1925, so the premise is valid, but this implementation needs rework before it can provide the stated guarantee.

Problems

  • hermes_cli/plugins.py:225 excludes on_session_start and on_session_end; current agent/conversation_loop.py:368-373 calls on_session_start inline. That leaves #10048's concrete lifecycle-hook repro unresolved.
  • hermes_cli/plugins.py:226 applies fail-open timeout handling to pre_tool_call. Current hermes_cli/plugins.py:2109-2128 documents it as a security/approval policy gate, and resolve_pre_tool_block() is intentionally fail-closed for approval failures at :2244-2268. A timeout must not silently permit the tool call.
  • hermes_cli/plugins.py:2008-2015 submits into a four-worker pool and calls future.cancel() after timeout. Running futures cannot be cancelled, so several distinct hung callbacks can occupy all workers and starve healthy callbacks; tests/hermes_cli/test_plugins.py:753-795 exercises only one hung callback.

Suggested changes

  • Add an explicit bounded/background policy for session-boundary hooks and cover the real on_session_start path.
  • Make pre_tool_call timeout fail closed, with an end-to-end dispatch test.
  • Add a saturation test and ensure timed-out running hooks cannot consume every worker.

Automated hermes-sweeper review.

Comment thread hermes_cli/plugins.py
Comment thread hermes_cli/plugins.py Outdated
Comment thread hermes_cli/plugins.py Outdated
@rodboev
rodboev force-pushed the pr/plugin-hook-timeouts branch from ff9a6e7 to 7c97a2b Compare July 14, 2026 03:22
@rodboev

rodboev commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, these findings did point to one incomplete execution-policy seam rather than three unrelated call-site fixes.

  1. hermes_cli/plugins.py now includes on_session_start and on_session_end in the bounded callback policy, and the first-turn path is covered through _restore_or_build_system_prompt() in tests/agent/test_system_prompt_restore.py.
  2. pre_tool_call timeouts now fail closed through the current main directive path, so a timed or still-running callback blocks dispatch through the existing resolve_pre_tool_block() callers instead of silently permitting the tool call.
  3. The shared four-worker executor and future.cancel() path are gone. Bounded callbacks now run in isolated per-callback daemon workers with at most one active worker per callback key, so distinct hung callbacks cannot consume capacity needed by a healthy callback.
  4. The focused regression set now covers policy classification, saturation, bounded session hooks, and an end-to-end blocked tool-dispatch case.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Plugin lifecycle hooks run synchronously on the turn hot path with no deadline

2 participants