Skip to content

Commit f2323a4

Browse files
Yeachan-Heodevswha
authored andcommitted
fix(chat): stop rendering a failed run's error twice
Every failed GJC turn puts two identical error bubbles in the transcript. The supervisor's terminal path forwards `error` and `complete` through the writer and then rejects the run promise, and `handleChatSend` catches that rejection and sends a second `error` message with a fresh id for the same failure. Observed on the wire for one `chat.send` (2.0.0-beta.6): error "GJC worker failed." id error_7748930f-... complete exitCode 1 error "GJC worker failed." id error_fee23e76-... The catch now only synthesizes an error message while the run is still running, i.e. when the runtime died without reporting anything. Once the terminal `complete` has passed through the registry the failure is already on screen, and the console line in the same branch keeps the rejection visible server-side. Tested: new websocket test drives a runtime that reports its own failure and then rejects, and asserts the client sees exactly error + complete. (cherry picked from commit 6a0a284)
1 parent 1b88da9 commit f2323a4

2 files changed

Lines changed: 99 additions & 1 deletion

File tree

server/modules/websocket/services/chat-websocket.service.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,11 @@ async function sendChat(ws: WebSocket, userId: string | number | null, data: Any
153153
console.error(`[Chat] Provider runtime "${provider}" failed`, { sessionId, error: message });
154154
const code = error && typeof error === 'object' && 'code' in error && typeof error.code === 'string' ? error.code : null;
155155
if (provider === 'gjc' && code) protocolFailure(ws, code, message, sessionId);
156-
else run.writer.send(createNormalizedMessage({ kind: 'error', provider, sessionId: storedSession.provider_session_id ?? sessionId, content: message }));
156+
// A run that already passed its terminal `complete` reported the failure
157+
// itself (GJC forwards `error` + `complete` before rejecting), so a second
158+
// bubble here is the same failure rendered twice in the transcript. The
159+
// console line above keeps the rejection visible server-side either way.
160+
else if (run.status === 'running') run.writer.send(createNormalizedMessage({ kind: 'error', provider, sessionId: storedSession.provider_session_id ?? sessionId, content: message }));
157161
} finally {
158162
chatRunRegistry.completeRunIfCurrent(run, { exitCode: 1 });
159163
}

server/modules/websocket/tests/chat-websocket.service.test.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,100 @@ test('chat.send dispatches a non-Git GJC session directly in its persisted proje
388388
});
389389
});
390390

391+
test('a runtime that reported its own failure does not get a second error bubble', async () => {
392+
await withIsolatedDatabase(async () => {
393+
sessionsDb.createAppSession('failing-session', 'gjc', '/workspace/failing-project');
394+
395+
const server = new WebSocketServer({ host: '127.0.0.1', port: 0 });
396+
try {
397+
await once(server, 'listening');
398+
server.on('connection', (socket, request) => {
399+
handleChatConnection(
400+
socket,
401+
Object.assign(request, { user: { id: 'test-user' } }),
402+
{
403+
spawnFns: {
404+
// This is what the GJC supervisor does on a failed run: forward
405+
// the error and the terminal complete, then reject the promise.
406+
gjc: (_command, _options, writer) => {
407+
const chatWriter = writer as {
408+
send(message: unknown): void;
409+
sendComplete(options: { exitCode: number }): void;
410+
};
411+
chatWriter.send({
412+
kind: 'error',
413+
provider: 'gjc',
414+
sessionId: 'failing-session',
415+
content: 'GJC worker failed.',
416+
});
417+
chatWriter.sendComplete({ exitCode: 1 });
418+
return Promise.reject(new Error('GJC worker failed.'));
419+
},
420+
},
421+
abortFns: { gjc: async () => false },
422+
resolveToolApproval() {},
423+
getPendingApprovalsForSession: () => [],
424+
},
425+
);
426+
});
427+
428+
const address = server.address();
429+
if (!address || typeof address === 'string') {
430+
throw new Error('Expected the websocket test server to bind a TCP port.');
431+
}
432+
433+
const client = new WebSocket(`ws://127.0.0.1:${address.port}`);
434+
try {
435+
await once(client, 'open');
436+
const frames: OutboundFrame[] = [];
437+
const completed = new Promise<void>((resolve, reject) => {
438+
client.on('message', (raw) => {
439+
try {
440+
const frame = parseOutboundFrame(String(raw));
441+
frames.push(frame);
442+
if (frame.kind === 'complete') {
443+
resolve();
444+
}
445+
} catch (error) {
446+
reject(error);
447+
}
448+
});
449+
});
450+
451+
client.send(JSON.stringify({
452+
type: 'chat.send',
453+
sessionId: 'failing-session',
454+
content: 'fail please',
455+
options: {},
456+
}));
457+
await completed;
458+
// The rejection is handled after the terminal frame, so give the
459+
// catch branch a turn before asserting nothing else arrived.
460+
await flushMessages();
461+
await flushMessages();
462+
463+
assert.deepEqual(frames.map((frame) => frame.kind), ['error', 'complete']);
464+
assert.equal(frames[0]?.content, 'GJC worker failed.');
465+
} finally {
466+
client.terminate();
467+
}
468+
} finally {
469+
for (const client of server.clients) {
470+
client.terminate();
471+
}
472+
await new Promise<void>((resolve, reject) => {
473+
server.close((error) => {
474+
if (error) {
475+
reject(error);
476+
return;
477+
}
478+
resolve();
479+
});
480+
});
481+
}
482+
});
483+
});
484+
391485
test('chat.abort uses the direct GJC run handle before the app session id', async () => {
392486
await withIsolatedDatabase(async () => {
393487
sessionsDb.createAppSession('abort-session', 'gjc', '/workspace/non-git-project');

0 commit comments

Comments
 (0)