Skip to content

fix(plugins): isolate timed-out hook worker state - #96841

Closed
bgrablin wants to merge 1 commit into
NousResearch:mainfrom
bgrablin:fix/plugins-isolate-timed-out-hook-state
Closed

bgrablin wants to merge 1 commit into
NousResearch:mainfrom
bgrablin:fix/plugins-isolate-timed-out-hook-state

Conversation

@bgrablin

@bgrablin bgrablin commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

A timed-out plugin hook can currently publish its late result into a later callback invocation. This can return the wrong plugin value, surface the wrong exception, or complete a successor callback before that callback finishes.

The cause is late-bound closure state in PluginManager.invoke_hook(): the timeout worker closes over loop-local context, result, failure, and completion objects that are reassigned for the next callback. Each worker now owns its invocation state for its full lifetime.

Changes

  • Bind callback execution context, result, failure, and completion state to each timeout worker.
  • Keep callback identity and timeout tokens invocation-specific so an abandoned worker cannot clear or replace a successor's running state.
  • Add deterministic overlap coverage for a late first callback, a blocked second callback, and a healthy third callback.
  • Preserve abandoned-worker timeout behavior, timeout suppression, fail-closed pre_tool_call behavior, caller-thread-only hooks, and context propagation.

Validation

Check Result
Before fix: timed-out first callback overlaps a blocked successor Failed as expected: ['late-first', 'third'] instead of ['third']
After fix: same deterministic overlap reproduction Passed: ['third']; the late first result cannot complete or populate the successor
Targeted plugin-hook test suite 75 passed, 0 failed
Targeted subagent-stop-hook test suite 7 passed, 0 failed
Ruff B023 check for the plugin module Passed: all checks passed
Gateway E2E suite on current main 61 passed, 7 skipped
Diff whitespace check Passed

The existing timeout test missed this overlap because it exercised one blocked callback followed by a healthy callback. It did not allow the timed-out worker to finish after the invocation loop advanced while a successor worker was also active. Therefore, it verified caller non-blocking behavior but not isolation of late completion state.

Timeout duration and configuration defaults are unchanged. Timeout suppression, timeout abandonment without joining, pre_tool_call fail-closed semantics, caller-thread-only hooks, and contextvars propagation are unchanged.

Duplicate search across current source and all issue/PR states found no equivalent report or change for late timed-out hook state. Related results were distinct: open #6622 covers the earlier hook-timeout/WAL work; merged #93824 established the current bounded-callback design; closed #39751 is an older lifecycle-timeout attempt; open #61656 requests opt-in strict policy hooks; merged #84901 covers shell-hook process-tree cleanup; and open #24557 is a broad plugin-isolation proposal. Exact searches for timed-out hook next callback, stale hook result, hook worker closure, bind per-callback hook state, and B023 plugins module found no equivalent issue or PR.

Infographic

Industrial relay schematic comparing plugin hook workers before and after per-worker state isolation: a late first completion corrupts shared closure state before the fix, while after the fix it cannot publish and the third callback returns alone.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins labels Aug 28, 2026
@bgrablin
bgrablin force-pushed the fix/plugins-isolate-timed-out-hook-state branch from 380fef6 to 233a951 Compare August 28, 2026 03:23
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

fix(plugins): isolate timed-out hook worker state — prevents a late first worker from completing a successor.

  • hermes_cli/plugins.py:PluginManager.invoke_hook_runner closure now captures context/outcome/failure/done/token/key as default-arg bindings (_context, _outcome, etc.) instead of closing over the loop-mutated locals. Previously Python's late binding meant a timed-out first worker's _runner would resolve outcome/done/context to the second callback's objects (the loop had advanced), so its late return "late-first" could done.set() and publish into the successor's outcome, wedging or corrupting it.
  • Test test_late_timed_out_hook_cannot_complete_successor — first callback blocks on second_started, second blocks on release_second past the 0.15s timeout, third is the only result. Asserts results==["third"] and second_completed not set until release_second fires, then waits for clean exit. Directly pins the late-binding bug.
  • Fix is four default-arg captures + three uses; existing timeout semantics and _invoke_hook_callback routing unchanged. Minimal blast radius.

@bgrablin

bgrablin commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded. On base 13e72fb205b735df679e0fd5f5996a34ac4accc6, the original late timed-out-hook overlap regression passes without this patch. The refactored implementation runs each callback in its own method frame, so a timed-out callback cannot complete a successor invocation. No code from this branch was needed after revalidation.

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 P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants