perf(hooks): stop blocking the agent's tool loop on the notify response - #142
Conversation
sendNotification and postManagedEvent are fire-and-forget at the JS level — nothing awaits them — but the outstanding http.request keeps Node's event loop alive until the response is drained, and Claude Code blocks the agent until the hook process exits. So every AskUserQuestion paid the full /api/notify round-trip: the pending_requests insert plus the push dispatch. Measured against the live local server, one real push each: before 1570 ms process lifetime after 60 ms Unref the socket in req.end()'s flush callback. The POST is already on the wire by then, so the payload still arrives — verified: both probe notifications were recorded in pending_requests even though the client exited at 60 ms. sendNotificationAndPoll is deliberately NOT changed. It has the same two-line shape but genuinely waits for an allow/deny decision; unref'ing there would make permission prompts resolve to nothing. Cost: the "=== NOTIFICATION SENT ===" stderr block no longer prints for these two paths, so non-200s from /api/notify go unlogged. Side effect: with no output, Claude Code stops persisting these hook runs to session transcripts.
Single Commit Policy - COMPLIANTStatus: Policy requirements met - 1 commit - Valid format - Ready for merge View validation detailsCommit Details
Validation Results
Automated validation by Shooter Single Commit Enforcement |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe notification hooks now unreference request sockets after both POST payloads flush. Existing payload delivery and error handling remain unchanged. ChangesNotification request lifecycle
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The hook now exits without waiting for notification responses, substantially reducing tool-loop latency while preserving delivery of the notification payload. No actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 1 (.claude/hooks/notifier.cjs)
New issues raised: 1 MINOR
Blocking issues: 0
Findings
The change correctly identifies that sendNotification and postManagedEvent are fire-and-forget at the JS level but were effectively synchronous because the outstanding http.request kept the event loop alive until the response was drained. Unref'ing the socket in req.end()'s flush callback allows the Claude Code hook process to exit as soon as the POST payload is on the wire, which matches the measured 26× improvement.
sendNotificationAndPoll is deliberately left blocking, which is the right call since it genuinely waits for an allow/deny decision.
No blocking criteria are triggered:
- No hardcoded secrets or credential exposure.
- No auth bypass or weakening.
- No command/shell injection.
- No destructive-by-default changes or permission regressions.
Minor note
The new unref behavior is a runtime change that currently relies on manual measurement. I left an inline suggestion to add a tests/*.cjs integration test that verifies the hook process exits quickly against a slow-responding local server while still delivering the payload. This aligns with the project's MINOR standard that new runtime behavior should have test coverage.
Approving — the fix is safe and the performance gain is well justified.
| // and Claude Code blocks the agent's tool loop until the process exits — so | ||
| // don't hold the event loop open for a response nothing here consumes. | ||
| // Measured: ~1150ms -> ~50ms, with the payload still delivered in full. | ||
| req.socket?.unref(); |
There was a problem hiding this comment.
💡 This new runtime behavior (unref the socket so the hook process can exit before the HTTP response arrives) is currently only verified manually. Per the project standards, new runtime behavior should have a test in tests/*.cjs. Consider adding a small integration test that spawns notifier.cjs against a local slow-responding HTTP server and asserts the process exits in <100 ms while the request body is still received. That would guard against an accidental revert to blocking req.end().
|
🎉 This PR is included in version 1.36.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Problem
The Claude Code
PreToolUsehook took ~1.2 s on everyAskUserQuestion. Measured across ~3 months of session transcripts: 2,601 runs, p50 1,193 ms, p90 1,669 ms, max 10,569 ms — the last one hitting thereq.setTimeout(10000)ceiling.Node startup isn't the cause. Cold start plus parsing the 83 KB
notifier.cjsmeasures 40–80 ms, about 5% of it. The rest is the/api/notifyround-trip: thepending_requestsinsert plus the push dispatch.sendNotificationandpostManagedEventare already fire-and-forget at the JS level — nothing awaits them. But the outstandinghttp.requestkeeps Node's event loop alive until the response is drained atres.on('end'), and Claude Code blocks the agent until the hook process exits. So they were synchronous in effect.Fix
Unref the socket in
req.end()'s flush callback. The POST is already on the wire by then, so the payload still arrives.Measured, against the live local server
26× faster. Both probe notifications were recorded in
pending_requestseven though the client exited at 60 ms — nothing dropped. A separate controlled test against a throwaway server that stalls 1.1 s reproduced it exactly: 1,150 ms → 50 ms, full payload received.What is deliberately NOT changed
sendNotificationAndPollhas the identical two-line shape but genuinely waits for an allow/deny decision and polls/api/response. Unref'ing there would make permission prompts resolve to nothing. Left blocking, and the patch script asserts it stayed that way.Cost
The
=== NOTIFICATION SENT ===stderr block no longer prints for these two paths, so non-200s from/api/notifygo unlogged. Side effect worth knowing: with no output, Claude Code stops persisting these hook runs to session transcripts — which is also why the slow runs were only ever visible forAskUserQuestionin the first place.Related, not fixed here
The hook was registered with an empty matcher, so it spawned Node on all 263,952 tool calls in the scan window while
handleToolStartno-ops for everything exceptAskUserQuestion— roughly 4.4 hours of blocked loop. That's a client-sidesettings.jsonfix ("matcher": "AskUserQuestion"), not a change to this repo.Summary by CodeRabbit