Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugin/scripts/session-end.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ async function main() {
headers: authHeaders(),
signal: AbortSignal.timeout(3e4)
}).catch(() => {});
setTimeout(() => process.exit(0), 1500).unref();
setTimeout(() => process.exit(0), 500).unref();
}
main();

Expand Down
2 changes: 1 addition & 1 deletion src/hooks/session-end.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ async function main() {
}).catch(() => {});
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

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();
🤖 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.

}

main();
77 changes: 77 additions & 0 deletions test/session-end-exit-timing.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { describe, it, expect, beforeAll, afterAll } from "vitest";
import { spawn } from "node:child_process";
import { createServer, type Server } from "node:http";
import { join } from "node:path";
import { once } from "node:events";

// Runs the BUILT hook artifact (what ships), not the TypeScript source. CI
// builds before testing; locally run `npm run build` after editing
// src/hooks/session-end.ts or this exercises stale code.
const HOOK = join(
import.meta.dirname,
"..",
"plugin",
"scripts",
"session-end.mjs",
);

// A server that accepts the connection and reads the request but never sends a
// response, so the hook's fire-and-forget `fetch` stays in flight. With the
// request hanging, the only thing that ends the hook process is its deferred
// `setTimeout(() => process.exit(0), N).unref()` — which lets us measure N.
let blackHole: Server;
let blackHoleUrl: string;

beforeAll(async () => {
blackHole = createServer(() => {
// Intentionally never respond.
});
blackHole.listen(0, "127.0.0.1");
await once(blackHole, "listening");
const address = blackHole.address();
const port = typeof address === "object" && address ? address.port : 0;
blackHoleUrl = `http://127.0.0.1:${port}`;
});

afterAll(async () => {
blackHole.close();
await once(blackHole, "close");
});

function runHook(
stdin: string,
env: Record<string, string>,
): Promise<{ exitCode: number | null; tookMs: number }> {
return new Promise((resolve, reject) => {
const start = Date.now();
const child = spawn(process.execPath, [HOOK], {
env: { PATH: process.env["PATH"] ?? "", ...env },
stdio: ["pipe", "ignore", "ignore"],
});
child.on("error", reject);
child.on("close", (exitCode) => {
resolve({ exitCode, tookMs: Date.now() - start });
});
child.stdin.write(stdin);
child.stdin.end();
});
}

describe("session-end hook exits within Claude Code's shutdown grace (#991)", () => {
it("exits well under the old 1500ms cap even when the memory server hangs", async () => {
const payload = JSON.stringify({ session_id: "ses_timing_test" });
const { exitCode, tookMs } = await runHook(payload, {
AGENTMEMORY_URL: blackHoleUrl,
});

expect(exitCode).toBe(0);
// Positive control: the hung request must actually hold the process open to
// the deferred-exit timer, otherwise the timing assertion is meaningless.
expect(tookMs).toBeGreaterThan(250);
// The deferred-exit timer must fire comfortably inside Claude Code's
// SessionEnd shutdown grace. The bound sits between the fixed 500ms timer
// (plus startup) and the old 1500ms cap that overran the grace, so the
// harness killed the hook and reported "Hook cancelled" (#991).
expect(tookMs).toBeLessThan(1400);
});
});