fix(hooks): exit the SessionEnd hook within Claude Code's shutdown grace#992
fix(hooks): exit the SessionEnd hook within Claude Code's shutdown grace#992costajohnt wants to merge 1 commit into
Conversation
…ace (rohitg00#991) The SessionEnd hook deferred its exit by 1500ms via `setTimeout(() => process.exit(0), 1500).unref()`. When the memory server is slow to respond, the in-flight request keeps the process alive until that timer fires, which overruns Claude Code's SessionEnd shutdown grace -- the harness then kills the hook and prints "Hook cancelled". Lower the cap to 500ms, matching the seven other lightweight hooks (only session-end and stop shared the 1500ms outlier). 500ms is still ample to flush the fire-and-forget POST to the local memory server. Add a test that runs the built hook against a server that never responds and asserts it exits well inside the grace window.
|
@costajohnt is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe Session-end exit timing fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/session-end.ts`:
- Line 66: The `session-end` hook is unconditionally forcing process exit after
500ms, which is too short when it fans out to the extra fire-and-forget POSTs.
Update the exit timing in `src/hooks/session-end.ts` so the `setTimeout(() =>
process.exit(0), ...)` delay stays at the longer 1500ms cap whenever the
additional request paths in `session-end` are enabled, and only use a shorter
delay for the single-request path if needed. Use the existing `session-end` flow
around the `/session/end` call and the multi-POST logic to make the timeout
conditional instead of shrinking every run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83fff74b-4140-4f95-99b5-a9559072a32b
📒 Files selected for processing (3)
plugin/scripts/session-end.mjssrc/hooks/session-end.tstest/session-end-exit-timing.test.ts
| } | ||
|
|
||
| setTimeout(() => process.exit(0), 1500).unref(); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the longer exit cap when session-end fans out to multiple requests.
This hook always sends /session/end, and lines 42-64 can enqueue up to three more fire-and-forget POSTs. With those paths enabled, forcing exit after 500ms can cut the process before the later requests have flushed, so session-end can silently drop consolidation / bridge work. If you want the single-request path to stay under Claude Code’s grace window, make the delay conditional on whether the extra POSTs are enabled instead of shrinking every session-end run.
Suggested change
- setTimeout(() => process.exit(0), 500).unref();
+ const exitDelayMs =
+ process.env["CONSOLIDATION_ENABLED"] === "true" ||
+ process.env["CLAUDE_MEMORY_BRIDGE"] === "true"
+ ? 1500
+ : 500;
+
+ setTimeout(() => process.exit(0), exitDelayMs).unref();As per the hook contract in AGENTS.md, multi-request telemetry hooks like session-end should keep the 1500ms cap so all fire-and-forget requests have time to start.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout(() => process.exit(0), 500).unref(); | |
| const exitDelayMs = | |
| process.env["CONSOLIDATION_ENABLED"] === "true" || | |
| process.env["CLAUDE_MEMORY_BRIDGE"] === "true" | |
| ? 1500 | |
| : 500; | |
| setTimeout(() => process.exit(0), exitDelayMs).unref(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/session-end.ts` at line 66, The `session-end` hook is
unconditionally forcing process exit after 500ms, which is too short when it
fans out to the extra fire-and-forget POSTs. Update the exit timing in
`src/hooks/session-end.ts` so the `setTimeout(() => process.exit(0), ...)` delay
stays at the longer 1500ms cap whenever the additional request paths in
`session-end` are enabled, and only use a shorter delay for the single-request
path if needed. Use the existing `session-end` flow around the `/session/end`
call and the multi-POST logic to make the timeout conditional instead of
shrinking every run.
1 participant
What
Fixes #991 — the SessionEnd hook printed "Hook cancelled" on exit.
Root cause
src/hooks/session-end.tsdeferred its exit withsetTimeout(() => process.exit(0), 1500).unref(). The hook fires a fire-and-forget POST to the local memory server; while that request is in flight it keeps the process alive, so the process doesn't exit until the 1500ms timer fires. That overruns Claude Code's SessionEnd shutdown grace, so the harness kills the hook and reports "Hook cancelled".The other seven lightweight hooks already use a 500ms cap; only
session-endandstopwere outliers at 1500ms.Fix
Lower the cap to 500ms (source + the committed
plugin/scripts/session-end.mjsartifact). 500ms is still ample to flush the small POST to the local server, and keeps the hook comfortably inside the shutdown grace.Test
test/session-end-exit-timing.test.tsruns the built hook against a server that accepts the connection but never responds (so the request hangs and the deferred-exit timer is what ends the process), then asserts it exits well under the old 1500ms cap. Fails before this change (~1580ms), passes after (~580ms). Full suite: 1416 tests pass.Note:
stop.tssrc/hooks/stop.tshas the identical 1500ms deferred exit and likely shows the same symptom. I kept this PR scoped to the reported SessionEnd issue, but happy to fixstoptoo (here or a follow-up) if you'd like — let me know if 1500ms was intentional there.Summary by CodeRabbit