From 4ac014def282ad11941bc6b30a7198eb07c1062e Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 02:36:43 +0300 Subject: [PATCH] =?UTF-8?q?fix(ai-chat):=20=D0=BF=D1=80=D0=BE=D0=B2=D0=B0?= =?UTF-8?q?=D0=BB=20beginRun=20=D1=84=D0=B5=D0=B9=D0=BB=D0=B8=D1=82=20?= =?UTF-8?q?=D1=85=D0=BE=D0=B4=20=D1=87=D0=B5=D1=81=D1=82=D0=BD=D1=8B=D0=BC?= =?UTF-8?q?=20503=20(A=5FRUN=5FBEGIN=5FFAILED)=20(#486)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Раньше провал beginRun (кроме unique-violation), напр. blip пула БД, логировался и ход ПРОДОЛЖАЛСЯ без run-строки. В autonomous такой ран никто не абортит: /stop его не видит, дисконнект не абортит, one-run-гейт пропускает ВТОРОЙ ран — невидимый неостанавливаемый ран до рестарта. Теперь провал beginRun (кроме RunAlreadyActiveError → прежний 409) бросает ServiceUnavailableException с кодом A_RUN_BEGIN_FAILED ДО первого байта и до вставки user-строки (post-hijack catch контроллера отдаёт честный 503 на raw- сокет). Без ветвления по режимам — #487 наследует ту же политику. В тело кладём statusCode: 503 (object-arg исключение его не добавляет), чтобы клиент видел статус. Клиентский классификатор: ветка A_RUN_BEGIN_FAILED добавлена СТРОГО ДО generic- 503-матча — иначе показал бы «provider is not configured» вместо «временно, повторите». Тесты: unit fail-fast (stream() бросает 503 A_RUN_BEGIN_FAILED, ни байта в сокет, user-строка не вставлена; RunAlreadyActiveError по-прежнему 409); unit клиентского классификатора из ПОЛНОГО реального тела ответа с гвардом порядка. --- .../ai-chat/utils/error-message.test.ts | 19 +++ .../features/ai-chat/utils/error-message.ts | 15 +++ .../ai-chat/ai-chat.begin-run-failure.spec.ts | 109 ++++++++++++++++++ .../src/core/ai-chat/ai-chat.service.ts | 27 ++++- 4 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 apps/server/src/core/ai-chat/ai-chat.begin-run-failure.spec.ts diff --git a/apps/client/src/features/ai-chat/utils/error-message.test.ts b/apps/client/src/features/ai-chat/utils/error-message.test.ts index f60f8cb4..153455ac 100644 --- a/apps/client/src/features/ai-chat/utils/error-message.test.ts +++ b/apps/client/src/features/ai-chat/utils/error-message.test.ts @@ -23,6 +23,25 @@ describe("describeChatError", () => { }); }); + it("classifies an A_RUN_BEGIN_FAILED 503 as a temporary run-start failure, NOT provider-not-configured (#486)", () => { + // The FULL real body the server writes for a beginRun failure: a + // ServiceUnavailableException(object) whose response is serialized verbatim + // onto the raw socket, self-describing statusCode 503 + the run-start code. + const body = + '{"message":"Could not start the agent run. This is usually temporary — please try again.","code":"A_RUN_BEGIN_FAILED","statusCode":503}'; + expect(describeChatError(body, t)).toEqual({ + title: "Could not start the run", + detail: + "The agent run could not be started. This is usually temporary — please try again.", + }); + // ORDER GUARD: even though the body ALSO carries statusCode 503 (which the + // generic branch matches), the A_RUN_BEGIN_FAILED branch runs first, so it is + // never mislabeled "AI provider not configured". + expect(describeChatError(body, t).title).not.toBe( + "AI provider not configured", + ); + }); + it("classifies a dropped connection (ECONNRESET) as a lost-connection error", () => { expect( describeChatError("Cannot connect to API: read ECONNRESET", t).title, diff --git a/apps/client/src/features/ai-chat/utils/error-message.ts b/apps/client/src/features/ai-chat/utils/error-message.ts index cd6606b5..2d59c177 100644 --- a/apps/client/src/features/ai-chat/utils/error-message.ts +++ b/apps/client/src/features/ai-chat/utils/error-message.ts @@ -24,6 +24,21 @@ export function describeChatError( ): ChatErrorView { const msg = message ?? ""; + // Our own "could not start the run" gate (A_RUN_BEGIN_FAILED, #486): a 503 + // whose body carries this code is a TEMPORARY server-side failure while + // starting the run (e.g. a DB-pool blip), NOT an unconfigured provider. It MUST + // be matched STRICTLY BEFORE the generic 503 branch below, which would + // otherwise mislabel it "The AI provider is not configured" and tell the user + // to call an admin instead of just retrying. + if (/"code"\s*:\s*"A_RUN_BEGIN_FAILED"/.test(msg)) { + return { + title: t("Could not start the run"), + detail: t( + "The agent run could not be started. This is usually temporary — please try again.", + ), + }; + } + if (/"statusCode"\s*:\s*403\b/.test(msg)) { return { title: t("AI chat is disabled"), diff --git a/apps/server/src/core/ai-chat/ai-chat.begin-run-failure.spec.ts b/apps/server/src/core/ai-chat/ai-chat.begin-run-failure.spec.ts new file mode 100644 index 00000000..bd9e256b --- /dev/null +++ b/apps/server/src/core/ai-chat/ai-chat.begin-run-failure.spec.ts @@ -0,0 +1,109 @@ +import { + ConflictException, + Logger, + ServiceUnavailableException, +} from '@nestjs/common'; +import { AiChatService } from './ai-chat.service'; +import { RunAlreadyActiveError } from './ai-chat-run.service'; + +/** + * Fail-fast guard for beginRun failures (#486, commit 4). + * + * When runHooks.begin() rejects for a reason OTHER than RunAlreadyActiveError + * (e.g. a DB-pool blip), the turn must NOT continue untracked. The old code + * logged and streamed anyway, leaving a run with NO run-row: in autonomous mode + * nobody could abort it (/stop can't see it, disconnect doesn't abort it, and the + * one-run gate would admit a SECOND run) — an unstoppable invisible run until + * restart. The fix throws A_RUN_BEGIN_FAILED (503) BEFORE the first byte and + * before the user row is persisted. + * + * We drive `stream()` directly on a prototype instance wired with only the + * collaborators it touches before the throw, so the assertion is on the REAL + * control flow, not a mock of it. + */ +describe('AiChatService beginRun failure (#486)', () => { + function makeService(insertSpy: jest.Mock): AiChatService { + // Bypass the (heavy) DI constructor: exercise the real stream() method on a + // bare prototype instance with just the fields reached before the throw. + // `any` because the private `logger` field makes a typed intersection collapse. + const svc = Object.create(AiChatService.prototype); + svc.aiChatRepo = { + // Existing chat -> no insert path; chatId is kept as-is. + findById: jest.fn().mockResolvedValue({ id: 'chat1' }), + }; + svc.aiChatMessageRepo = { insert: insertSpy }; + svc.logger = new Logger('test'); + return svc as AiChatService; + } + + const baseArgs = () => { + const write = jest.fn(); + const res = { + raw: { write, writableEnded: false, headersSent: false }, + }; + return { + user: { id: 'u1' } as never, + workspace: { id: 'w1' } as never, + sessionId: 's1', + // openPage undefined -> resolveOpenPageContext returns null without any DB + // call; chatId present -> the existing-chat path. + body: { chatId: 'chat1', messages: [] } as never, + res: res as never, + signal: new AbortController().signal, + model: {} as never, + role: null, + write, + }; + }; + + it('throws A_RUN_BEGIN_FAILED (503) before the first byte and before persisting the user turn', async () => { + const insertSpy = jest.fn(); + const svc = makeService(insertSpy); + const { write, ...args } = baseArgs(); + + const runHooks = { + begin: jest.fn().mockRejectedValue(new Error('DB pool exhausted')), + } as never; + + let caught: unknown; + try { + await svc.stream({ ...args, runHooks }); + } catch (e) { + caught = e; + } + + expect(caught).toBeInstanceOf(ServiceUnavailableException); + const http = caught as ServiceUnavailableException; + expect(http.getStatus()).toBe(503); + expect(http.getResponse()).toMatchObject({ code: 'A_RUN_BEGIN_FAILED' }); + + // Fail-fast: nothing was written to the socket and NO user message row was + // persisted, so the turn left no orphan state to clean up. + expect(write).not.toHaveBeenCalled(); + expect(insertSpy).not.toHaveBeenCalled(); + }); + + it('still maps a lost-the-race RunAlreadyActiveError to a 409, not A_RUN_BEGIN_FAILED', async () => { + const insertSpy = jest.fn(); + const svc = makeService(insertSpy); + const { write, ...args } = baseArgs(); + + const runHooks = { + begin: jest.fn().mockRejectedValue(new RunAlreadyActiveError('chat1')), + } as never; + + let caught: unknown; + try { + await svc.stream({ ...args, runHooks }); + } catch (e) { + caught = e; + } + + expect(caught).toBeInstanceOf(ConflictException); + expect((caught as ConflictException).getResponse()).toMatchObject({ + code: 'A_RUN_ALREADY_ACTIVE', + }); + expect(write).not.toHaveBeenCalled(); + expect(insertSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index f4dbb31d..908c9200 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -4,6 +4,7 @@ import { Injectable, Logger, OnModuleInit, + ServiceUnavailableException, } from '@nestjs/common'; import { FastifyReply } from 'fastify'; import { @@ -797,12 +798,32 @@ export class AiChatService implements OnModuleInit { code: 'A_RUN_ALREADY_ACTIVE', }); } - // Any OTHER run-start failure must not break the turn — fall back to the - // socket signal (legacy behavior) and stream anyway. + // Any OTHER run-start failure (e.g. a DB-pool blip) must FAIL THE TURN, + // not silently stream without a run-row. The old fallback let the turn + // continue untracked: in autonomous mode nobody could then abort it — + // /stop can't see a run that doesn't exist, a client disconnect doesn't + // abort it, and the one-run-per-chat gate would let a SECOND run in. That + // is an unstoppable, invisible run until process restart. Reject NOW, + // BEFORE the first byte (nothing is written yet, no user row inserted, no + // MCP lease taken), so the controller's post-hijack catch turns this + // HttpException into an honest 503 on the raw socket. Same policy for BOTH + // modes — #487 inherits it (no mode-branching here). this.logger.error( - `Failed to begin agent run (chat ${chatId}); streaming without run tracking`, + `Failed to begin agent run (chat ${chatId}); failing the turn`, err as Error, ); + throw new ServiceUnavailableException({ + message: + 'Could not start the agent run. This is usually temporary — please try again.', + code: 'A_RUN_BEGIN_FAILED', + // Self-describe the status in the body: the controller's post-hijack + // catch writes getResponse() verbatim onto the raw socket, and an + // object-arg HttpException does NOT inject statusCode. Without it the + // client's 503 classifier (which reads the body JSON) could not see the + // status. With it present, the client's A_RUN_BEGIN_FAILED branch (which + // runs strictly before the generic-503 branch) shows "temporary, retry". + statusCode: 503, + }); } }