Skip to content

fix(chat): catch ProcessTransport errors on interrupt #416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dimakis wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(chat): catch ProcessTransport errors on interrupt #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion server/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,7 +1273,16 @@ export async function interruptChat(
);
await Promise.allSettled(stops);
}
await session.queryInstance.interrupt();
try {

@dimakis dimakis Jun 28, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 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 dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 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]

await session.queryInstance.interrupt();
} catch (err: unknown) {
// ProcessTransport may already be dead (process exited) — the interrupt

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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]

// is moot in that case. Swallow rather than crashing the server.
log.warn('interrupt() failed (transport likely dead)', {
clientId,
err: err instanceof Error ? err.message : String(err),
});
}
// Only push to inputQueue on first delivery — a retried interrupt should
// still call interrupt() (to halt the agent) but not double-queue the prompt.
if (!isDup) {
Expand Down
6 changes: 6 additions & 0 deletions server/ws-handler-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,12 @@ export function handleInterruptV2(
msg.contextBlocks,
msg.clientMsgId,
msg.model,
).catch((err: unknown) =>

@dimakis dimakis Jun 28, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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 dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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]

log.error('interruptChat failed', {
connectionId,
sessionId: msg.sessionId,
err: err instanceof Error ? err.message : String(err),
}),
);
log.info('interrupt', { connectionId, sessionId: msg.sessionId });
return;
Expand Down
Loading