fix(hooks): fire PostToolUse hooks through fire_and_forget_trigger - #2575
Open
ayaangazali wants to merge 1 commit into
Open
fix(hooks): fire PostToolUse hooks through fire_and_forget_trigger#2575ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
Both PostToolUse and PostToolUseFailure fired their hooks with a bare asyncio.create_task whose handle was dropped on the next line. asyncio only keeps tasks in a WeakSet, so a pending hook task held by nothing else can be collected before its subprocess finishes, and the inline done callback swallowed exceptions without logging them. HookEngine.fire_and_forget_trigger already exists for exactly this and is what SubagentStop uses: it holds a strong reference until the task completes and logs failures.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Resolve #2564
Description
PostToolUseandPostToolUseFailurefired their hooks with a bareasyncio.create_task(...)and then dropped the handle on the next line. Two problems with that:WeakSet, so a pending hook task that nothing else references can be collected before its subprocess finishes. This is the footgun the CPython docs warn about.lambda t: t.exception() if not t.cancelled() else None) retrieved the exception purely to silence the "never retrieved" warning, so a hook that failed did so silently.HookEngine.fire_and_forget_triggeralready exists for exactly this pattern, its docstring describes this bug, andSubagentStopinsubagents/runner.pyalready uses it. It holds a strong reference in_pending_fire_and_forgetuntil the task completes and logs failures via_log_fire_and_forget_failure. These two call sites just were not migrated. So this is a reuse of what is already here rather than anything new.One honest caveat: I could not build a test that deterministically reproduces the collection itself. I tried forcing
gc.collect()while a real hook subprocess was mid-flight and the hook still completed on unpatched code, since the task stays reachable through the loop's callback chain in that window. So the collection is a real but nondeterministic race, and I did not want to commit a test that passes for the wrong reason. The test I did add asserts the contract that actually differs: both hook events now route through the tracked helper. It fails on unpatched code for both events and passes after. The silent-failure-logging half of the fix is unconditional either way.Checklist
make gen-changelogto update the changelog. (hand-edited CHANGELOG.md in the same style, I do not have the Kimi API setup the skill needs)make gen-docsto update the user documentation. (nothing in docs describes this internal behavior)for transparency: freshman, and the issue author had already traced this one well so most of the credit is theirs. i verified the two call sites and the helper at HEAD myself before touching anything, and talked through the asyncio task lifetime bits with claude code since that part is genuinely subtle. if the test framing is not what you want here i am happy to change it :)