fix(chat): catch ProcessTransport errors on interrupt

[dimakis]

Summary

• Wraps session.queryInstance.interrupt() in try/catch in chat.ts — when the SDK child process has already exited, the call throws ProcessTransport is not ready for writing as an unhandled promise rejection, crashing the Node server
• Adds .catch() to the fire-and-forget interruptChat() call in ws-handler-v2.ts to prevent unhandled rejections from that path
• 4 crashes attributed to this bug across June 22-27 (3 on June 27 alone)

Test plan

• Verify server stays up when interrupting a session whose agent process has exited
• Verify normal interrupt flow still works (send message while agent is generating)
• Check logs for the new warn message under the dead-transport scenario

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/chat.ts

Correct defensive fix for a real crash path. The catch in chat.ts preserves post-interrupt logic (inputQueue push), and the .catch in ws-handler-v2 prevents an unhandled rejection on the fire-and-forget call. Main gap is a missing unit test for the new catch branch.

• 🟡 missing_tests (L1271): No test covers the new try/catch around session.queryInstance.interrupt(). A test where interrupt() rejects (e.g., mockRejectedValue(new Error('process exited'))) should verify that (a) the error is swallowed, (b) interruptChat still returns true, and (c) the inputQueue.push on line 1283 still executes. Without this, there's no regression guard that the catch clause actually preserves the post-interrupt logic. [fixable]
• 🔵 style (L1274): The comment is good context but slightly long (3 lines). Consider condensing to a single line: // ProcessTransport may already be dead — interrupt is moot. [fixable]

server/index.ts

Correct defensive fix for a real crash path. The catch in chat.ts preserves post-interrupt logic (inputQueue push), and the .catch in ws-handler-v2 prevents an unhandled rejection on the fire-and-forget call. Main gap is a missing unit test for the new catch branch.

• 🔵 regressions (L911): The v1 WS handler in index.ts:911 also calls interruptChat (via await inside withSpanAsync). It has its own outer try/catch at line 931, but the error message there is generic ('failed to handle WS message') and sends an error frame to the client — different from the fire-and-forget .catch in the v2 handler. Now that interruptChat internally swallows the interrupt() error, the v1 path is also safe, but the inconsistency in error-reporting semantics (v1: error frame to client, v2: silent log) is worth noting for future maintainers.

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

server/chat.ts

Correct defensive fix — catches a real crash vector. Missing a test for the new try/catch path; the two-layer error handling (warn inside, error outside) could confuse log triage.

• 🟡 missing_tests (L1271): No test covers the case where queryInstance.interrupt() rejects (e.g. dead ProcessTransport). The existing tests in send-to-chat.test.ts all mock interrupt to resolve. A test that mocks interrupt to reject and asserts that interruptChat still returns true (and logs a warning) would lock in this fix. [fixable]

server/ws-handler-v2.ts

Correct defensive fix — catches a real crash vector. Missing a test for the new try/catch path; the two-layer error handling (warn inside, error outside) could confuse log triage.

• 🔵 style (L734): The .catch() was added to handle the fire-and-forget interruptChat() call, but interruptChat now catches its own interrupt() error internally and doesn't rethrow. The only remaining throw paths are earlier guards (registry.get returning null — returns false silently, not a throw) or the storeAndEchoIfNew/assemblePrompt path. The outer .catch() is still reasonable as defense-in-depth, but the log level (log.error) could be misleading since the most likely failure (dead transport) is already handled as log.warn inside interruptChat. Consider aligning the severity or noting that this is a fallback for unexpected errors. [fixable]

[dimakis]

🟡 missing_tests: No test covers the case where queryInstance.interrupt() rejects (e.g. dead ProcessTransport). The existing tests in send-to-chat.test.ts all mock interrupt to resolve. A test that mocks interrupt to reject and asserts that interruptChat still returns true (and logs a warning) would lock in this fix. [fixable]

[dimakis]

🔵 style: The .catch() was added to handle the fire-and-forget interruptChat() call, but interruptChat now catches its own interrupt() error internally and doesn't rethrow. The only remaining throw paths are earlier guards (registry.get returning null — returns false silently, not a throw) or the storeAndEchoIfNew/assemblePrompt path. The outer .catch() is still reasonable as defense-in-depth, but the log level (log.error) could be misleading since the most likely failure (dead transport) is already handled as log.warn inside interruptChat. Consider aligning the severity or noting that this is a fallback for unexpected errors. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/chat.ts

Correct and focused fix — wraps a crash-causing interrupt() call in try/catch and adds a .catch() guard on the fire-and-forget caller. Both changes are complementary (inner catch allows normal completion; outer catch guards other failures). Main gap is missing test coverage for the new error paths.

• 🟡 missing_tests (L1276): No test covers the new try/catch around session.queryInstance.interrupt(). interruptChat has zero test coverage in chat.test.ts. A test where queryInstance.interrupt() rejects would verify the error is swallowed and the function still pushes to inputQueue (the happy path after catch). [fixable]
• 🔵 style (L1279): The multi-line comment inside the catch block ('ProcessTransport may already be dead…') is useful on the first read but could be trimmed to one line per project convention ('No comments unless the WHY is non-obvious'). The log message already captures the intent. [fixable]

server/ws-handler-v2.ts

Correct and focused fix — wraps a crash-causing interrupt() call in try/catch and adds a .catch() guard on the fire-and-forget caller. Both changes are complementary (inner catch allows normal completion; outer catch guards other failures). Main gap is missing test coverage for the new error paths.

• 🔵 missing_tests (L736): The .catch() on the fire-and-forget interruptChat call is untested. interruptChat is mocked in the ws-handler-v2 test suite — a test where the mock rejects would confirm the rejection is caught and logged rather than becoming an unhandled promise rejection. [fixable]

[dimakis]

🟡 missing_tests: No test covers the new try/catch around session.queryInstance.interrupt(). interruptChat has zero test coverage in chat.test.ts. A test where queryInstance.interrupt() rejects would verify the error is swallowed and the function still pushes to inputQueue (the happy path after catch). [fixable]

[dimakis]

🔵 style: The multi-line comment inside the catch block ('ProcessTransport may already be dead…') is useful on the first read but could be trimmed to one line per project convention ('No comments unless the WHY is non-obvious'). The log message already captures the intent. [fixable]

[dimakis]

🔵 missing_tests: The .catch() on the fire-and-forget interruptChat call is untested. interruptChat is mocked in the ws-handler-v2 test suite — a test where the mock rejects would confirm the rejection is caught and logged rather than becoming an unhandled promise rejection. [fixable]