fix(ai-chat): провал beginRun фейлит ход честным 503 (A_RUN_BEGIN_FAILED) (#486)

Раньше провал 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 клиентского классификатора из ПОЛНОГО реального тела ответа с гвардом порядка.
2026-07-11 02:36:43 +03:00
parent 799dcfe7c5
commit 4ac014def2
4 changed files with 167 additions and 3 deletions
@@ -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,
@@ -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"),
@@ -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();
+  });
+});
@@ -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,
+        });
      }
    }