fix(hooks): persist tool stats from short-lived hook processes#1492
Merged
Conversation
PostToolUse hooks run in fresh Python processes that record exactly one tool call before exiting. With flush_interval=10, that single record_tool_call() never reached disk, leaving the Stop hook summary permanently stuck at "[CB] Xm | 0 tools | 0 errors" no matter how many tools were used in the session. - post-tool-use.py: explicit stats.flush() right after record for immediate visibility (statusline, Stop summary). - stats.py: track every live SessionStats in a module-level WeakSet and flush them via a single atexit handler. WeakSet keeps GC unchanged and avoids the per-instance handler leak that a naive atexit.register(self.method) would cause. - finalize() drops the instance from the WeakSet so the post-finalize flush sweep does not resurrect the cleaned-up file. - Module docstring documents the short-lived process pattern. - Regression tests cover: WeakSet membership, persistence via the module flush handler, finalize() removal, and the no-handler-leak invariant (20 instances must not register 20 handlers).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Summary
PostToolUsehooks run in fresh, short-lived Python processes — each one creates aSessionStats, callsrecord_tool_call()exactly once, then exits. With the defaultflush_interval=10, that single increment lived only in the in-memory accumulator and was discarded at process exit. The disk file never advanced pasttool_count: 0, so the Stop hook summary always read[CB] Xm | 0 tools | 0 errorsno matter how many tools were used.Root cause
hooks/lib/stats.pyflush()only writes when_pending_count >= flush_interval.hooks/post-tool-use.pyrecords exactly once per process and never callsflush().hooks/stop.pyreads disk in a separate process whose own in-memory accumulator is empty, so itsflush()is a no-op andformat_summary()reports the stale 0.tests/test_stats.py::test_data_persists_across_instancesalready had to calls1.flush()explicitly — the library required caller flushing but the real caller didn't do it.Fix
post-tool-use.py): explicitstats.flush()right afterrecord_tool_call()for immediate visibility (statusline, Stop summary).stats.py): track every liveSessionStatsin a module-levelweakref.WeakSetand flush them via a single atexit handler. WeakSet keeps GC unchanged and avoids the per-instance handler leak thatatexit.register(self.method)would cause when many instances are created in one process.finalize(): drops the instance from the WeakSet so the post-finalize flush sweep does not resurrect the cleaned-up file.Regression tests (
TestShortLivedProcess)test_instance_added_to_live_set— newly constructed instances are tracked.test_record_persists_via_module_atexit_handler— records reach disk via_flush_all_pending()even when the caller forgetsflush().test_finalize_removes_instance_from_live_set— finalized instances disappear and the file is not resurrected.test_only_one_atexit_handler_regardless_of_instance_count— creating 20 instances must not grow the atexit handler list (no leak).Test plan
yarn workspace codingbuddy-claude-plugin lint— cleanyarn workspace codingbuddy-claude-plugin format:check— cleanyarn workspace codingbuddy-claude-plugin typecheck— cleanyarn workspace codingbuddy-claude-plugin test:coverage— 144/144yarn workspace codingbuddy-claude-plugin circular— no cyclesyarn workspace codingbuddy-claude-plugin build— okpython3 -m pytest tests/test_stats.py tests/test_post_tool_use.py tests/test_stop.py— 56/56Bash,Edit,Read,Bash,Grep) → disk showstool_count: 5, tool_names: {Bash:2, Edit:1, Read:1, Grep:1}andformat_summaryreturns[CB] 0m | 5 tools | 0 errors | Bash:2 Edit:1 Read:1 Grep:1.Follow-ups (out of scope, separate issues recommended)
flush()read-modify-write race:_locked_readand_locked_writerelease the lock between calls, allowing concurrent PostToolUse processes to lose updates.record_hook_timingis never called: defined instats.pybut no hook actually invokes it, so hook timing aggregation is dead code today.