Skip to content

fix(session-end): remove .unref() so process force-exits after 1500ms - #1070

Open
dnraitzyk wants to merge 1 commit into
rohitg00:mainfrom
dnraitzyk:fix/session-end-hook-force-exit
Open

fix(session-end): remove .unref() so process force-exits after 1500ms#1070
dnraitzyk wants to merge 1 commit into
rohitg00:mainfrom
dnraitzyk:fix/session-end-hook-force-exit

Conversation

@dnraitzyk

@dnraitzyk dnraitzyk commented Jul 16, 2026

Copy link
Copy Markdown

Fixes #1069.

What

Remove .unref() from the setTimeout in session-end.mjs / session-end.ts.

Why

The session-end hook fires fire-and-forget fetch() calls to the local daemon, then does:

setTimeout(() => process.exit(0), 1500).unref();

AGENTS.md documents the intent: the process should force-exit after 1500ms so it does not block Claude Code's shutdown. But .unref() makes the timer passive — Node.js does not keep the event loop alive for an unref'd timer, but it also will not fire it while other handles (the pending fetch() calls) are keeping the loop alive.

The fetches use AbortSignal.timeout() values of 30s, 60s, and 120s. Those pending requests keep the event loop alive the entire time. Claude Code's hook deadline fires first and kills the process with:

SessionEnd hook [node "${CLAUDE_PLUGIN_ROOT}/scripts/session-end.mjs"] failed: Hook cancelled

This is intermittent because when the daemon responds fast the event loop drains before the deadline; when it is slow the race is lost.

Fix

Remove .unref(). An active timer is the only handle that keeps the event loop alive after the fetches are dispatched, so the process exits after exactly 1500ms regardless of whether the fetches have settled. This matches the documented intent and the fire-and-forget pattern AGENTS.md prescribes.

- setTimeout(() => process.exit(0), 1500).unref();
+ setTimeout(() => process.exit(0), 1500);

Summary by CodeRabbit

  • Bug Fixes
    • Improved session-end handling so processes terminate reliably after the session ends.
    • Prevented lingering network requests from delaying shutdown beyond the expected timeout.

Pending fetch() calls keep the Node.js event loop alive until their
AbortSignal timeouts fire (30s normally, up to 120s with CONSOLIDATION_ENABLED).
With .unref(), the setTimeout becomes passive and never fires while those
fetches are in flight, so the hook process outlives Claude Code's hook
timeout and gets killed with 'Hook cancelled'.

Removing .unref() makes the timer active: the process exits after 1500ms
regardless of in-flight fetches. The fetches are fire-and-forget to the
local daemon anyway, so exiting before they settle is correct behavior.
@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown

@dnraitzyk is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 45de2006-8654-4e63-8036-b4218b030b35

📥 Commits

Reviewing files that changed from the base of the PR and between 93ae9bc and 1ba4419.

📒 Files selected for processing (2)
  • plugin/scripts/session-end.mjs
  • src/hooks/session-end.ts

📝 Walkthrough

Walkthrough

Updated both session-end implementations to use referenced 1500ms exit timers, ensuring the process terminates after the timeout regardless of pending fetch requests. Inline comments document the event-loop behavior.

Changes

Session-end process termination

Layer / File(s) Summary
Force session-end termination
src/hooks/session-end.ts, plugin/scripts/session-end.mjs
Replaces unref’d exit timers with active 1500ms timers and documents the resulting event-loop behavior.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related issues

  • rohitg00/agentmemory#991 — Addresses the same session-end deferred exit timer behavior.

Possibly related PRs

  • rohitg00/agentmemory#688 — Implements the earlier fire-and-forget and unref’d timeout pattern in the same session-end exit logic.

Suggested reviewers: rohitg00

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: removing .unref() so session-end exits after 1500ms.
Linked Issues check ✅ Passed The PR implements the linked bug fix by removing .unref() from the 1500ms session-end timer in both files.
Out of Scope Changes check ✅ Passed The changes stay scoped to the session-end timeout behavior and related comments, with no unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dnraitzyk

Copy link
Copy Markdown
Author

How do approvals work?

@sla-te

sla-te commented Aug 21, 2026

Copy link
Copy Markdown

I hit the same "Hook cancelled" message and went looking for this PR, but the measurements do not support the diagnosis. This change makes the healthy case 27x slower and leaves the failing case unchanged.

.unref() does not make a timer passive in the sense assumed here. It only stops the timer from keeping the event loop alive by itself. The timer still fires normally as long as something else holds the loop open, and the pending fetch() is exactly that. So the 1500 ms guard already fires and already bounds the hook.

Measured on the shipped session-end.mjs (0.9.29 dist, node v24.19.0), same input on stdin, wall clock around the process:

daemon state current (.unref()) this PR (no .unref())
responding normally 56 ms 1549 ms
accepts the connection, never answers 1548 ms 1548 ms
SYN dropped, connect never completes 1546 ms 1548 ms

Two things follow. The hook never waits for the 30s/60s/120s AbortSignal timeouts, so that part of the root cause in #1069 does not happen. And removing .unref() cannot fix the report, because the failing case is already exiting at ~1.5 s. All the PR does is drag the fast path down to the same 1.5 s.

What actually produces the message is that ~1.5 s is longer than the shutdown grace Claude Code gives a SessionEnd hook, which is how #991 frames it. On my box the hook only crossed that line while the daemon was wedged (accept queue full, Recv-Q 129 > backlog 128, engine stuck in futex_do_wait at 12.9 GB RSS). Restarting the daemon put it back to 56 ms and the message stopped.

So the fix has to shorten the worst case, not force the best case to it. Lowering the guard on session-end and stop to something safely under the grace window would do it and keeps .unref() doing its job. The 1500 ms in d626b4e was chosen to let several fetches get initiated, and initiating them is fast, so there looks to be plenty of room below it.

Reproduction, if useful: point AGENTMEMORY_URL at nc -l 127.0.0.1 39111 for the accepts-but-silent case and at an address that drops SYNs for the other, then time echo '{"session_id":"x","reason":"other"}' | node scripts/session-end.mjs.

Is there a documented number for the SessionEnd grace window, or does the cap have to be found empirically?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: session-end hook causes 'Hook cancelled' due to .unref() making setTimeout passive while fetch keeps event loop alive

2 participants