perf(hooks): fire-and-forget telemetry hooks (zero blocking on every assistant turn)#573
Open
efenex wants to merge 4 commits into
Open
perf(hooks): fire-and-forget telemetry hooks (zero blocking on every assistant turn)#573efenex wants to merge 4 commits into
efenex wants to merge 4 commits into
Conversation
The Stop hook POSTs to /agentmemory/summarize on every assistant turn. Originally awaited the response with a 120s AbortSignal timeout, which gated Claude Code's next-prompt boundary on the daemon's full LLM-driven summarize cycle (~minutes on long sessions / slow providers like serverless OpenAI-compat endpoints). Worst case: every turn blocked for 120s. Dropping the `await` alone wasn't enough: Node keeps the event loop alive waiting for any pending fetch() to settle, so the script still hung until the AbortSignal fired. Empirically the patched-but-still- awaited version was clocked at 120.10s in direct testing, and Claude Code's status line showed "running stop hooks 1/2 · 3m 31s" mid-session. Fix: pair the fire-and-forget fetch with `setTimeout(() => process.exit(0), 500).unref()`. The unref'd timer doesn't keep the loop alive when the fetch happens to settle fast (local daemon dedup returns in <100ms), but force-exits the process 500ms after the request was dispatched if the response hangs. 500ms is more than enough to flush a 1KB POST to the local daemon's socket buffer; the daemon still receives and processes the summarize request server-side. Empirical result: stop.mjs invocation time dropped from 120.10s to 0.10s. Both Stop hooks combined (this one + agentmemory-import-on-stop.sh) now total 0.12s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the same pattern from hooks(stop) c83611d to every other
telemetry hook that does an `await fetch(...)` without consuming the
response. The Stop fix alone wasn't enough — slow LLM providers (e.g.
serverless OpenAI-compat endpoints) caused the same hang on Stop's
sister hooks, observed concretely as 30-40s blocks at SessionEnd and
unbounded latency on PostToolUse / PromptSubmit / Notification.
The bug: dropping `await` is not sufficient. Node keeps the event loop
alive waiting for any pending fetch() to settle. Without an explicit
exit, the script still blocks until the AbortSignal.timeout fires
(seconds to minutes depending on the per-hook cap).
The fix: pair the unawaited fetch with `setTimeout(() => process.exit(0),
N).unref()`. The unref'd timer doesn't keep the loop alive when fetch
settles fast, but force-exits the process N ms after the request was
dispatched if the response hangs. N is sized to flush the POST(s) to
the local daemon's socket buffer:
- 500ms for single-fetch hooks: notification, post-tool-failure,
post-tool-use, prompt-submit, subagent-start, subagent-stop,
task-completed
- 1500ms for session-end (4 sequential POSTs)
Empirically each patched hook now returns in 90-150ms regardless of
LLM-provider state (down from 0.8-30s+ per hook).
Hooks left untouched intentionally:
- pre-tool-use, pre-compact, session-start — these write context to
stdout for Claude Code to consume; force-exit would truncate
- stop — already patched in c83611d
Also picks up the build outputs for the prior project-resolver
refactor (the .ts source was already committed but the rebuilt
plugin/scripts/*.mjs and the shared chunk plugin/scripts/_project-
DDQ-L_E2.mjs were not — they appear as bonus +/- in this diff since
the rebuild produced them alongside the hook-fix lines).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claude Code's actual PostToolUse hook payload uses `tool_response`, not `tool_output`. The hook was reading from the wrong field, so `cleanOutput` was always `undefined`, the /observe POST reached the server with no output value, and mem::compress silently failed XML schema validation on every real tool call — observations got stored with empty narratives, degrading memory_recall and starving the consolidation pipeline. Read `data.tool_response` with `data.tool_output` as a fallback so any older integration still on the legacy field name keeps working. Independently discovered and PR'd by Rex57 upstream as rohitg00#561 — applying here so the hook fire-and-forget commit (3471847) plus this fix ship together as one coherent post-tool-use change. If rohitg00#561 lands first, this commit's diff for plugin/scripts/post-tool-use.mjs collapses to the comment-line addition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@efenex is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR converts hook and plugin telemetry POSTs from awaited try/catch flows to non-blocking fire-and-forget fetch calls, adds short unref'd timers that force process exit, and introduces a project-name resolver utility. ChangesTelemetry Fire-and-Forget Conversion
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook/Script
participant Fetch as fetch()
participant Observer as /agentmemory/observe
participant Exit as setTimeout(process.exit)
Hook->>Fetch: fetch(observer_request).catch(suppress)
Fetch->>Observer: POST telemetry (fire-and-forget)
Hook->>Exit: setTimeout(process.exit, ~500-1500ms).unref()
Note over Exit: short unref'd timer forces process exit
Observer-->>Fetch: response (ignored)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/hooks/post-tool-use.ts (1)
35-40: ⚡ Quick winTrim this explanatory WHAT-comment and let naming/code carry it.
This block is very detailed and comment-heavy for behavior visible in code; please reduce it to a short rationale-only note (or remove it).
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 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/post-tool-use.ts` around lines 35 - 40, The long explanatory block above the tool output handling should be replaced with a short rationale-only comment (or removed) that states the intent: preserve legacy fallback for deprecated field names while preferring the current one; leave the actual behavior in code (selecting tool_response with fallback to tool_output) unchanged — update the comment near the logic that reads tool_response / tool_output (and any mention of cleanOutput or mem::compress) to a one-line note like "Prefer tool_response; keep fallback to deprecated tool_output for backward compatibility."src/hooks/stop.ts (1)
42-51: ⚡ Quick winPlease shorten/remove this WHAT-heavy inline narrative.
The behavior is clear from code; keep only a brief why-focused note if needed.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 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/stop.ts` around lines 42 - 51, The long WHAT-heavy comment above the fire-and-forget logic in the Stop hook should be shortened: remove the multi-paragraph narrative and replace it with a single brief why-focused note that explains purpose (ensure the Stop hook doesn't hang waiting on a pending fetch and we force exit shortly after the POST) and references the mechanism used (the unref'd setTimeout paired with fetch() and AbortSignal.timeout). Update the comment near the Stop hook / setTimeout(...).unref() so it is a one-line rationale only, omitting the detailed step-by-step description.
🤖 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 `@plugin/scripts/_project-DDQ-L_E2.mjs`:
- Line 8: The current assignment for dir uses cwd && cwd.trim() which throws if
cwd is not a string; change the guard to verify cwd is a string before calling
trim (e.g., use typeof cwd === "string" && cwd.trim()) so that non-string cwd
values fall back to process.cwd(); update the expression that sets dir (the
variable dir and the cwd check) to use this robust type-safe check.
In `@src/hooks/notification.ts`:
- Around line 36-54: Wrap the best-effort fetch call in a try/catch block while
preserving AbortSignal.timeout(2000) and the existing fire-and-forget behavior
(the fetch(...).catch(()=>{}) replacement), so that any synchronous or promise
throw is caught per hook contract; locate the top-level fetch invocation that
posts to `${REST_URL}/agentmemory/observe` (using authHeaders(),
AbortSignal.timeout, and JSON body with hookType "notification") and surround it
with try { await fetch(...); } catch (err) { /* swallow/log if needed */ } but
keep the non-blocking exit via setTimeout(() => process.exit(0), 500).unref().
Ensure you do not remove the AbortSignal.timeout call and that the catch handler
swallows errors to maintain best-effort semantics.
In `@src/hooks/post-tool-failure.ts`:
- Around line 36-60: Wrap the existing fire-and-forget fetch call in an
immediately-invoked async function that uses try/catch so the HTTP dispatch is
still non-blocking but any thrown errors are caught; inside the IIFE call
fetch(...) with the same options (including AbortSignal.timeout(3000)) and await
it, catch and ignore/log the error in the catch block, and remove the trailing
.catch(() => {}) on the original fetch call; keep the setTimeout(() =>
process.exit(0), 500).unref() exit behavior unchanged.
In `@src/hooks/post-tool-use.ts`:
- Around line 45-64: Wrap the best-effort fetch call in an explicit try/catch
block (instead of relying only on .catch()) so any synchronous or awaited errors
are handled per hooks guideline: call fetch(...) with the existing
AbortSignal.timeout(3000) and await it inside a try block, swallow/log errors in
the catch, and preserve the fire-and-forget behavior by not blocking termination
(keep the setTimeout(() => process.exit(0), 500).unref() afterwards). Update the
block that currently uses fetch(...)... .catch(() => {}) to use try { await
fetch(...) } catch (err) { /* handle/suppress error */ } while retaining the
same request payload and AbortSignal.timeout usage.
In `@src/hooks/prompt-submit.ts`:
- Around line 35-49: Wrap the fire-and-forget fetch dispatch in an
immediately-invoked async function and surround the awaited fetch call with
try/catch so errors are caught (preserve AbortSignal.timeout usage); locate the
current fetch invocation and replace the bare promise + .catch with an IIFE like
(async () => { try { await fetch(..., { signal: AbortSignal.timeout(3000) }) }
catch (err) { /* log or ignore */ } })(); keep the existing setTimeout(() =>
process.exit(0), 500).unref() behavior unchanged.
In `@src/hooks/session-end.ts`:
- Around line 35-71: Wrap each best-effort fetch dispatch in a try/catch so
synchronous errors are caught and to follow the hook guideline: around the calls
that reference REST_URL and authHeaders() (the session/end,
agentmemory/crystals/auto, agentmemory/consolidate-pipeline, and
agentmemory/claude-bridge/sync fetches) enclose the fetch invocation in try {
void fetch(...).catch(()=>{}); } catch (err) { /* swallow */ } while keeping the
AbortSignal.timeout(...) signal and the final setTimeout(() => process.exit(0),
1500).unref(); call unchanged.
In `@src/hooks/stop.ts`:
- Around line 52-60: Wrap the best-effort fetch dispatch to
`${REST_URL}/agentmemory/summarize` in an explicit try/catch that awaits the
call so any thrown error from fetch or AbortSignal.timeout(120000) is caught and
ignored; keep using authHeaders() and JSON.stringify({ sessionId }) and preserve
the AbortSignal.timeout(...) option and the existing setTimeout(() =>
process.exit(0), 500).unref() timer behavior. Ensure the try block contains the
await fetch(...) invocation and the catch block is empty (or logs a non-fatal
message) so the hook remains standalone and does not propagate errors.
---
Nitpick comments:
In `@src/hooks/post-tool-use.ts`:
- Around line 35-40: The long explanatory block above the tool output handling
should be replaced with a short rationale-only comment (or removed) that states
the intent: preserve legacy fallback for deprecated field names while preferring
the current one; leave the actual behavior in code (selecting tool_response with
fallback to tool_output) unchanged — update the comment near the logic that
reads tool_response / tool_output (and any mention of cleanOutput or
mem::compress) to a one-line note like "Prefer tool_response; keep fallback to
deprecated tool_output for backward compatibility."
In `@src/hooks/stop.ts`:
- Around line 42-51: The long WHAT-heavy comment above the fire-and-forget logic
in the Stop hook should be shortened: remove the multi-paragraph narrative and
replace it with a single brief why-focused note that explains purpose (ensure
the Stop hook doesn't hang waiting on a pending fetch and we force exit shortly
after the POST) and references the mechanism used (the unref'd setTimeout paired
with fetch() and AbortSignal.timeout). Update the comment near the Stop hook /
setTimeout(...).unref() so it is a one-line rationale only, omitting the
detailed step-by-step description.
🪄 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: 2c4af9d3-3248-4311-91c2-20d2c221f1b2
📒 Files selected for processing (19)
plugin/scripts/_project-DDQ-L_E2.mjsplugin/scripts/notification.mjsplugin/scripts/post-tool-failure.mjsplugin/scripts/post-tool-use.mjsplugin/scripts/prompt-submit.mjsplugin/scripts/session-end.mjsplugin/scripts/stop.mjsplugin/scripts/subagent-start.mjsplugin/scripts/subagent-stop.mjsplugin/scripts/task-completed.mjssrc/hooks/notification.tssrc/hooks/post-tool-failure.tssrc/hooks/post-tool-use.tssrc/hooks/prompt-submit.tssrc/hooks/session-end.tssrc/hooks/stop.tssrc/hooks/subagent-start.tssrc/hooks/subagent-stop.tssrc/hooks/task-completed.ts
| function resolveProject(cwd) { | ||
| const explicit = process.env["AGENTMEMORY_PROJECT_NAME"]; | ||
| if (explicit && explicit.trim()) return explicit.trim(); | ||
| const dir = cwd && cwd.trim() ? cwd : process.cwd(); |
Contributor
There was a problem hiding this comment.
Guard cwd.trim() with a string check.
cwd && cwd.trim() throws when cwd is non-string (e.g., object/number), which bypasses your fallback path. Please harden this branch.
Suggested patch
- const dir = cwd && cwd.trim() ? cwd : process.cwd();
+ const dir = typeof cwd === "string" && cwd.trim() ? cwd : process.cwd();📝 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.
Suggested change
| const dir = cwd && cwd.trim() ? cwd : process.cwd(); | |
| const dir = typeof cwd === "string" && cwd.trim() ? cwd : process.cwd(); |
🤖 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 `@plugin/scripts/_project-DDQ-L_E2.mjs` at line 8, The current assignment for
dir uses cwd && cwd.trim() which throws if cwd is not a string; change the guard
to verify cwd is a string before calling trim (e.g., use typeof cwd === "string"
&& cwd.trim()) so that non-string cwd values fall back to process.cwd(); update
the expression that sets dir (the variable dir and the cwd check) to use this
robust type-safe check.
Comment on lines
+36
to
+54
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | ||
| fetch(`${REST_URL}/agentmemory/observe`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ | ||
| hookType: "notification", | ||
| sessionId, | ||
| project: data.cwd || process.cwd(), | ||
| cwd: data.cwd || process.cwd(), | ||
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| notification_type: data.notification_type, | ||
| title: data.title, | ||
| message: data.message, | ||
| }, | ||
| }), | ||
| signal: AbortSignal.timeout(2000), | ||
| }).catch(() => {}); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Wrap the best-effort fetch dispatch in try/catch to match hook contract.
This segment uses only .catch() on the Promise; the hook guideline requires try/catch around best-effort HTTP calls (while keeping AbortSignal.timeout()).
Suggested patch
- fetch(`${REST_URL}/agentmemory/observe`, {
- method: "POST",
- headers: authHeaders(),
- body: JSON.stringify({
- hookType: "notification",
- sessionId,
- project: data.cwd || process.cwd(),
- cwd: data.cwd || process.cwd(),
- timestamp: new Date().toISOString(),
- data: {
- notification_type: data.notification_type,
- title: data.title,
- message: data.message,
- },
- }),
- signal: AbortSignal.timeout(2000),
- }).catch(() => {});
+ try {
+ fetch(`${REST_URL}/agentmemory/observe`, {
+ method: "POST",
+ headers: authHeaders(),
+ body: JSON.stringify({
+ hookType: "notification",
+ sessionId,
+ project: data.cwd || process.cwd(),
+ cwd: data.cwd || process.cwd(),
+ timestamp: new Date().toISOString(),
+ data: {
+ notification_type: data.notification_type,
+ title: data.title,
+ message: data.message,
+ },
+ }),
+ signal: AbortSignal.timeout(2000),
+ }).catch(() => {});
+ } catch {}As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
📝 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.
Suggested change
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | |
| fetch(`${REST_URL}/agentmemory/observe`, { | |
| method: "POST", | |
| headers: authHeaders(), | |
| body: JSON.stringify({ | |
| hookType: "notification", | |
| sessionId, | |
| project: data.cwd || process.cwd(), | |
| cwd: data.cwd || process.cwd(), | |
| timestamp: new Date().toISOString(), | |
| data: { | |
| notification_type: data.notification_type, | |
| title: data.title, | |
| message: data.message, | |
| }, | |
| }), | |
| signal: AbortSignal.timeout(2000), | |
| }).catch(() => {}); | |
| setTimeout(() => process.exit(0), 500).unref(); | |
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | |
| try { | |
| fetch(`${REST_URL}/agentmemory/observe`, { | |
| method: "POST", | |
| headers: authHeaders(), | |
| body: JSON.stringify({ | |
| hookType: "notification", | |
| sessionId, | |
| project: data.cwd || process.cwd(), | |
| cwd: data.cwd || process.cwd(), | |
| timestamp: new Date().toISOString(), | |
| data: { | |
| notification_type: data.notification_type, | |
| title: data.title, | |
| message: data.message, | |
| }, | |
| }), | |
| signal: AbortSignal.timeout(2000), | |
| }).catch(() => {}); | |
| } catch {} | |
| setTimeout(() => process.exit(0), 500).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/notification.ts` around lines 36 - 54, Wrap the best-effort fetch
call in a try/catch block while preserving AbortSignal.timeout(2000) and the
existing fire-and-forget behavior (the fetch(...).catch(()=>{}) replacement), so
that any synchronous or promise throw is caught per hook contract; locate the
top-level fetch invocation that posts to `${REST_URL}/agentmemory/observe`
(using authHeaders(), AbortSignal.timeout, and JSON body with hookType
"notification") and surround it with try { await fetch(...); } catch (err) { /*
swallow/log if needed */ } but keep the non-blocking exit via setTimeout(() =>
process.exit(0), 500).unref(). Ensure you do not remove the AbortSignal.timeout
call and that the catch handler swallows errors to maintain best-effort
semantics.
Comment on lines
+36
to
+60
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | ||
| fetch(`${REST_URL}/agentmemory/observe`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ | ||
| hookType: "post_tool_failure", | ||
| sessionId, | ||
| project: data.cwd || process.cwd(), | ||
| cwd: data.cwd || process.cwd(), | ||
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| tool_name: data.tool_name, | ||
| tool_input: | ||
| typeof data.tool_input === "string" | ||
| ? data.tool_input.slice(0, 4000) | ||
| : JSON.stringify(data.tool_input ?? "").slice(0, 4000), | ||
| error: | ||
| typeof data.error === "string" | ||
| ? data.error.slice(0, 4000) | ||
| : JSON.stringify(data.error ?? "").slice(0, 4000), | ||
| }, | ||
| }), | ||
| signal: AbortSignal.timeout(3000), | ||
| }).catch(() => {}); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Best-effort telemetry call should still be guarded with try/catch.
Please keep the fire-and-forget behavior, but wrap this HTTP dispatch in try/catch to satisfy the hook-script contract.
As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
🤖 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/post-tool-failure.ts` around lines 36 - 60, Wrap the existing
fire-and-forget fetch call in an immediately-invoked async function that uses
try/catch so the HTTP dispatch is still non-blocking but any thrown errors are
caught; inside the IIFE call fetch(...) with the same options (including
AbortSignal.timeout(3000)) and await it, catch and ignore/log the error in the
catch block, and remove the trailing .catch(() => {}) on the original fetch
call; keep the setTimeout(() => process.exit(0), 500).unref() exit behavior
unchanged.
Comment on lines
+45
to
+64
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | ||
| fetch(`${REST_URL}/agentmemory/observe`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ | ||
| hookType: "post_tool_use", | ||
| sessionId, | ||
| project: data.cwd || process.cwd(), | ||
| cwd: data.cwd || process.cwd(), | ||
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| tool_name: data.tool_name, | ||
| tool_input: data.tool_input, | ||
| tool_output: truncate(cleanOutput, 8000), | ||
| ...(imageData ? { image_data: imageData } : {}), | ||
| }, | ||
| }), | ||
| signal: AbortSignal.timeout(3000), | ||
| }).catch(() => {}); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Reintroduce try/catch around this best-effort hook request.
Current pattern uses Promise .catch() only; hook scripts here are required to use try/catch with AbortSignal.timeout() for REST calls.
As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
🤖 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/post-tool-use.ts` around lines 45 - 64, Wrap the best-effort fetch
call in an explicit try/catch block (instead of relying only on .catch()) so any
synchronous or awaited errors are handled per hooks guideline: call fetch(...)
with the existing AbortSignal.timeout(3000) and await it inside a try block,
swallow/log errors in the catch, and preserve the fire-and-forget behavior by
not blocking termination (keep the setTimeout(() => process.exit(0),
500).unref() afterwards). Update the block that currently uses fetch(...)...
.catch(() => {}) to use try { await fetch(...) } catch (err) { /*
handle/suppress error */ } while retaining the same request payload and
AbortSignal.timeout usage.
Comment on lines
+35
to
+49
| // Fire-and-forget + force-exit; see src/hooks/stop.ts for rationale. | ||
| fetch(`${REST_URL}/agentmemory/observe`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ | ||
| hookType: "prompt_submit", | ||
| sessionId, | ||
| project: data.cwd || process.cwd(), | ||
| cwd: data.cwd || process.cwd(), | ||
| timestamp: new Date().toISOString(), | ||
| data: { prompt: data.prompt }, | ||
| }), | ||
| signal: AbortSignal.timeout(3000), | ||
| }).catch(() => {}); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep fire-and-forget, but wrap the dispatch in try/catch.
For this hook path, best-effort REST calls need try/catch + timeout; .catch() alone doesn’t satisfy the project rule.
As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
🤖 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/prompt-submit.ts` around lines 35 - 49, Wrap the fire-and-forget
fetch dispatch in an immediately-invoked async function and surround the awaited
fetch call with try/catch so errors are caught (preserve AbortSignal.timeout
usage); locate the current fetch invocation and replace the bare promise +
.catch with an IIFE like (async () => { try { await fetch(..., { signal:
AbortSignal.timeout(3000) }) } catch (err) { /* log or ignore */ } })(); keep
the existing setTimeout(() => process.exit(0), 500).unref() behavior unchanged.
Comment on lines
+35
to
+71
| // Fire-and-forget + force-exit. SessionEnd was the slowest hook by a | ||
| // wide margin — up to ~90s total when CONSOLIDATION_ENABLED+CLAUDE_ | ||
| // MEMORY_BRIDGE were on, since 4 sequential awaits stacked. Now all | ||
| // four requests dispatch in parallel and the script exits 1500ms later | ||
| // regardless. The daemon still processes everything server-side. | ||
| fetch(`${REST_URL}/agentmemory/session/end`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ sessionId }), | ||
| signal: AbortSignal.timeout(30000), | ||
| }).catch(() => {}); | ||
|
|
||
| if (process.env["CONSOLIDATION_ENABLED"] === "true") { | ||
| try { | ||
| await fetch(`${REST_URL}/agentmemory/crystals/auto`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ olderThanDays: 0 }), | ||
| signal: AbortSignal.timeout(60000), // Increased from 15s | ||
| }); | ||
| } catch {} | ||
| fetch(`${REST_URL}/agentmemory/crystals/auto`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ olderThanDays: 0 }), | ||
| signal: AbortSignal.timeout(60000), | ||
| }).catch(() => {}); | ||
|
|
||
| try { | ||
| await fetch(`${REST_URL}/agentmemory/consolidate-pipeline`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ tier: "all", force: true }), | ||
| signal: AbortSignal.timeout(120000), // Increased from 30s | ||
| }); | ||
| } catch {} | ||
| fetch(`${REST_URL}/agentmemory/consolidate-pipeline`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ tier: "all", force: true }), | ||
| signal: AbortSignal.timeout(120000), | ||
| }).catch(() => {}); | ||
| } | ||
|
|
||
| if (process.env["CLAUDE_MEMORY_BRIDGE"] === "true") { | ||
| try { | ||
| await fetch(`${REST_URL}/agentmemory/claude-bridge/sync`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| signal: AbortSignal.timeout(30000), // Increased from 5s | ||
| }); | ||
| } catch { | ||
| // best-effort | ||
| } | ||
| fetch(`${REST_URL}/agentmemory/claude-bridge/sync`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| signal: AbortSignal.timeout(30000), | ||
| }).catch(() => {}); | ||
| } | ||
|
|
||
| setTimeout(() => process.exit(0), 1500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Wrap this session-end dispatch block in try/catch guards.
Good move parallelizing requests, but this still needs explicit try/catch around the best-effort REST dispatches in this hook.
As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
🤖 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` around lines 35 - 71, Wrap each best-effort fetch
dispatch in a try/catch so synchronous errors are caught and to follow the hook
guideline: around the calls that reference REST_URL and authHeaders() (the
session/end, agentmemory/crystals/auto, agentmemory/consolidate-pipeline, and
agentmemory/claude-bridge/sync fetches) enclose the fetch invocation in try {
void fetch(...).catch(()=>{}); } catch (err) { /* swallow */ } while keeping the
AbortSignal.timeout(...) signal and the final setTimeout(() => process.exit(0),
1500).unref(); call unchanged.
Comment on lines
+52
to
+60
| fetch(`${REST_URL}/agentmemory/summarize`, { | ||
| method: "POST", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ sessionId }), | ||
| signal: AbortSignal.timeout(120000), | ||
| }).catch(() => { | ||
| // summarize is best-effort | ||
| } | ||
| }); | ||
| setTimeout(() => process.exit(0), 500).unref(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add try/catch around this best-effort summarize call.
This hook keeps timeout (good), but still needs explicit try/catch around the REST dispatch per project hook conventions.
As per coding guidelines, "Hook scripts in src/hooks/ are standalone Node.js scripts without iii-sdk imports that read JSON from stdin, make HTTP calls to REST APIs, and always use try/catch with AbortSignal.timeout() for best-effort calls".
🤖 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/stop.ts` around lines 52 - 60, Wrap the best-effort fetch dispatch
to `${REST_URL}/agentmemory/summarize` in an explicit try/catch that awaits the
call so any thrown error from fetch or AbortSignal.timeout(120000) is caught and
ignored; keep using authHeaders() and JSON.stringify({ sessionId }) and preserve
the AbortSignal.timeout(...) option and the existing setTimeout(() =>
process.exit(0), 500).unref() timer behavior. Ensure the try block contains the
await fetch(...) invocation and the catch block is empty (or logs a non-fatal
message) so the hook remains standalone and does not propagate errors.
…context) CodeRabbit on rohitg00#573 flagged that the previous "always use try/catch with await" contract is violated by this PR's fire-and-forget pattern. The contract was the bug — try/catch+await blocks Claude Code's next-prompt boundary on every assistant turn, by up to the AbortSignal.timeout duration (120 s per Stop on slow LLM providers). Update AGENTS.md to reflect the split: - Context-injecting hooks (pre-tool-use, pre-compact, session-start): keep try/catch + await. Claude Code consumes their stdout, so the script MUST wait for the response before exiting. - Telemetry-only hooks (notification, post-tool-failure, post-tool-use, prompt-submit, stop, session-end, subagent-start, subagent-stop, task-completed): fire-and-forget + setTimeout(...).unref() force-exit. The unawaited fetch dispatches the request; the unref'd timer force-exits the process after the request flushes to the local daemon's socket buffer (~500 ms). Without the timer, Node keeps the event loop alive waiting for the fetch — defeating the purpose. The split matches the existing categorization in this PR's commits and mirrors the per-hook decision documented in AGENTS.md's PR description. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
The bug
Plugin hooks block Claude Code's next-prompt boundary on `await fetch(/observe)` (and `/summarize`, `/session/end` etc.) for the full `AbortSignal.timeout` — up to 120 s per turn when the LLM provider is slow (serverless cold-start, novita, etc.). Observed concretely as `✻ Lollygagging… (running stop hooks… 1/2 · 3m 31s)` in the Claude Code status line on long sessions.
The subtle bug under the bug
Dropping the `await` alone is not enough. Node keeps the event loop alive waiting for any pending `fetch()` to settle, so the script still hangs until the AbortSignal fires. The patched-but-still-awaited version was clocked at exactly 120.10 s in direct testing.
The fix
Pair the un-awaited fetch with `setTimeout(() => process.exit(0), N).unref()`:
Hooks fixed
Hooks left untouched
`pre-tool-use`, `pre-compact`, `session-start` write context to stdout that Claude Code consumes — force-exit would truncate the inject. They stay synchronous.
Bundled bug fix from #561
The 3rd commit picks up Rex57's #561: Claude Code's PostToolUse payload uses `tool_response`, not `tool_output` — so `mem::compress` was silently failing XML validation on every real tool call. Folded in here (with Co-Authored attribution) because:
Related
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Improvements
Documentation