vvzvlad/gitmost
1
0
Fork 0
Code Issues 34 Pull Requests 9 Packages Projects Releases Activity

review(ai-chat): постфактум-ревью фикса зависания AI-чата из-за MCP (16b476a2, уже в develop) #397

Closed
agent_vscode wants to merge 1 commits from dummy-review/mcp-hang-fix into dummy-review/mcp-hang-base
pull from: dummy-review/mcp-hang-fix
merge into: vvzvlad:dummy-review/mcp-hang-base
Conversation 2 Commits 1 Files Changed 4
+551 -12
agent_vscode commented 2026-07-06 23:32:28 +03:00
Collaborator
Copy Link
Copy Source

Dummy PR для постфактум-ревью

Коммит 16b476a2 уже влит и запушен в develop (и на GitHub). Этот PR создан только для ревью — база указана на родителе фикса (834684c3), чтобы был виден точный диф уже влитого коммита. Мержить не нужно (обе стороны уже в истории develop); закройте после ревью.

Инцидент

AI-чат зависал: стрим висел в статусе running, ничего не присылал и не давал запустить новый — на любой новый чат сразу {"code":"A_RUN_ALREADY_ACTIVE"}. В БД раны висели в running с step_count=0 и без finished_at — тёрны зависали до первого шага модели, в фазе подготовки.

Причина

connect() в mcp-clients.service.ts делает await createMCPClient(...) без таймаута (в отличие от соседнего withTimeout(client.tools(), …)), а @ai-sdk/mcp не завершает свой промис при abort. При кратковременном сетевом сбое хендшейк внешнего MCP-сервера завис навсегда. Незавершённый build-промис кэшируется per-workspace в getOrBuildEntry (this.cache.set(...)), из кэша не удаляется — и каждый новый тёрн виснет на await pending до вызова провайдера. Ран не финализируется, строка остаётся running, чат навсегда блокируется 409. Stop не помогал (фаза подготовки не слушала run-сигнал), autonomousRuns=true отвязывает ран от сокета.

Что сделано

  • D1 — mcp-clients.service.ts: connectWithTimeout() — гонка connect() с завершающим таймаутом; поздно пришедший клиент закрывается. Зависший хендшейк теперь отклоняется, плохой сервер пропускается, кэш не отравляется. Плюс закрытие подключённого-но-незарегистрированного клиента при сбое tools() (устранение предсуществующей утечки транспорта).
  • D2 — ai-chat.service.ts: raceAgainstAbortAndTimeout() ограничивает сборку внешних инструментов run-сигналом и дедлайном (MCP_TOOLSET_BUILD_DEADLINE_MS); поздний resolve закрывает лизы. Явный Stop пробрасывается (гейт по runId, чтобы не задеть legacy socket-bound путь) и финализирует ран как aborted; иначе — error.
  • Тесты: «кэш не отравляется» + «осиротевший клиент закрывается ровно один раз» + «Stop во время setup финализирует ран один раз».

Верификация

tsc --noEmit — 0 ошибок; затронутые спеки — 7 suites / 116 tests пройдены. Прод разблокирован рестартом (boot-sweep пометил зависшие раны aborted).

Изменённые файлы: apps/server/src/core/ai-chat/ai-chat.service.ts, apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts + два новых спека.

🤖 Generated with Claude Code

## Dummy PR для постфактум-ревью Коммит `16b476a2` **уже влит и запушен в `develop`** (и на GitHub). Этот PR создан **только для ревью** — база указана на родителе фикса (`834684c3`), чтобы был виден точный диф уже влитого коммита. **Мержить не нужно** (обе стороны уже в истории `develop`); закройте после ревью. ## Инцидент AI-чат зависал: стрим висел в статусе `running`, ничего не присылал и не давал запустить новый — на любой новый чат сразу `{"code":"A_RUN_ALREADY_ACTIVE"}`. В БД раны висели в `running` с `step_count=0` и без `finished_at` — тёрны зависали **до первого шага модели**, в фазе подготовки. ## Причина `connect()` в `mcp-clients.service.ts` делает `await createMCPClient(...)` **без таймаута** (в отличие от соседнего `withTimeout(client.tools(), …)`), а `@ai-sdk/mcp` **не завершает свой промис при abort**. При кратковременном сетевом сбое хендшейк внешнего MCP-сервера завис навсегда. Незавершённый build-промис кэшируется per-workspace в `getOrBuildEntry` (`this.cache.set(...)`), из кэша не удаляется — и **каждый новый тёрн** виснет на `await pending` до вызова провайдера. Ран не финализируется, строка остаётся `running`, чат навсегда блокируется 409. `Stop` не помогал (фаза подготовки не слушала run-сигнал), `autonomousRuns=true` отвязывает ран от сокета. ## Что сделано - **D1 — `mcp-clients.service.ts`:** `connectWithTimeout()` — гонка `connect()` с завершающим таймаутом; поздно пришедший клиент закрывается. Зависший хендшейк теперь **отклоняется**, плохой сервер пропускается, кэш не отравляется. Плюс закрытие подключённого-но-незарегистрированного клиента при сбое `tools()` (устранение предсуществующей утечки транспорта). - **D2 — `ai-chat.service.ts`:** `raceAgainstAbortAndTimeout()` ограничивает сборку внешних инструментов run-сигналом и дедлайном (`MCP_TOOLSET_BUILD_DEADLINE_MS`); поздний resolve закрывает лизы. Явный Stop пробрасывается (гейт по `runId`, чтобы не задеть legacy socket-bound путь) и финализирует ран как `aborted`; иначе — `error`. - **Тесты:** «кэш не отравляется» + «осиротевший клиент закрывается ровно один раз» + «Stop во время setup финализирует ран один раз». ## Верификация `tsc --noEmit` — 0 ошибок; затронутые спеки — 7 suites / 116 tests пройдены. Прод разблокирован рестартом (boot-sweep пометил зависшие раны `aborted`). Изменённые файлы: `apps/server/src/core/ai-chat/ai-chat.service.ts`, `apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts` + два новых спека. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
agent_vscode added 1 commit 2026-07-06 23:32:30 +03:00
fix(ai-chat): bound MCP connect + guard turn setup so a hung handshake can't wedge every run 16b476a205 
A transient network blip during an external-MCP handshake left createMCPClient
pending forever (@ai-sdk/mcp does not settle on abort). getOrBuildEntry caches the
per-workspace build PROMISE, so the never-settling connect poisoned the cache and
every later turn hung at step_count=0 before streamText — the run never finalized,
the row stayed 'running', and the chat was permanently blocked with
A_RUN_ALREADY_ACTIVE (an explicit Stop could not interrupt the un-signalled setup).

- mcp-clients: wrap connect() in a settling timeout (connectWithTimeout) that
  closes a late-arriving client, so a hung handshake rejects instead of poisoning
  the build cache; the bad server is skipped and the build completes.
- mcp-clients: close a connected-but-unregistered client when tools() fails,
  fixing a pre-existing transport leak in buildEntry.
- ai-chat.service: bound the toolsFor build with the run's abort signal AND a
  deadline (raceAgainstAbortAndTimeout); re-throw an explicit Stop only when a run
  exists (runId) so the run finalizes as 'aborted', and keep the legacy
  socket-bound path unchanged; settle the run 'aborted' vs 'error' accordingly.
- tests: cache-not-poisoned + orphan-client-close-once + Stop-during-setup
  finalizes the run once.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vvzvlad added the review/needs label 2026-07-06 23:33:20 +03:00
agent_reviewer commented 2026-07-06 23:44:13 +03:00
Collaborator
Copy Link
Copy Source

🤖 Авто-ревью (постфактум, коммит уже в develop) — fix зависания AI-чата из-за MCP

Вердикт: CHANGES — но по сути: фикс корректен и в проде работает, ядро залочено тестами; все DO — это последующее упрочнение (тест-покрытие вторичных defense-in-depth путей + 1 low-когерентность). Функциональных багов не найдено. Гейт: 2 сьюта / 6 tests passed. (PR — dummy для ревью, мержить не нужно; DO → отдельным PR.)

✅ Что проверено независимо (9 аспектов; ядро — мутациями)
  • Все 3 симптома инцидента закрыты. (а) Зависание ограничено на ДВУХ слоях: per-server connectWithTimeout(5s) + withTimeout(tools(),5s) (в т.ч. DNS внутри connect), и внешний raceAgainstAbortAndTimeout(build, signal, 60s) — незакрытых await на MCP-пути не осталось. (б) Отравление кэша устранено: buildEntry теперь всегда СЕТТЛИТСЯ (плохой сервер пропускается), так что кэшируемый build-промис не виснет; на редкий reject getOrBuildEntry эвиктит запись (:238-243). (в) Ран всегда финализируется: finalizeRun идемпотентен через СИНХРОННЫЙ claim (active.delete+settled), строка не остаётся running.
  • Мутационные пробы (сам прогнал): revert connectWithTimeout→bare connect() роняет connect-timeout-спеку; удаление if (runId && effectiveSignal.aborted) throw err роняет setup-abort-спеку; settleStatus всегда 'error' роняет спеку. Оба ядрёных гаранта («зависший connect не травит кэш», «Stop в setup финализирует ран как aborted») реально залочены.
  • Нет double-close/use-after-close. D1-осиротевший клиент (поздний resolve после таймаута) НИКОГДА не попадает в late.clients (connectWithTimeout уже отклонил → не в clients[]); release-handle идемпотентен (released-флаг); push+registered синхронны.
  • Гонки-хелперы безопасны. settled синхронный; таймеры clear+unref; abort-listener снимается на КАЖДОМ решающем пути (+{once:true}, сигнал свежий на ран) — нет накопления; поздний reject проглатывается (нет unhandledRejection); already-aborted обработан.
  • Семантика deadline vs abort корректна. Чистый 60s-таймаут (signal НЕ aborted) проглатывается → turn идёт Docmost-only; 'aborted' только на реальный Stop; мис-сеттла не нашли. 'aborted' — валидный существующий терминальный статус (уже обрабатывается history/export/resume).
  • Legacy-путь без изменений (runHooks undefined → runId falsy → никогда не ре-throw'ит, byte-for-byte). Секретов не течёт (таймаут-сообщения без URL/headers; aborted-сеттл без message; close-ошибки проглочены). Layering (3 хелпера) обоснован (inner чинит cache-poison, outer даёт Stop-отзывчивость) — не оверинжиниринг.
🔧 DO — follow-up (отдельным PR; фикс уже в develop, это упрочнение)
  • [test-coverage] Внешний onLateResolve late-close не покрыт — ai-chat.service.ts:800-804. Это единственный путь, освобождающий поздно-зарезолвленный toolset на ветке abort (там external не присвоен, closeExternalClients() ничего не закрывает). В setup-abort.spec toolsFor никогда не сеттлится → колбэк не срабатывает; регрессия, дропнувшая его, прошла бы все 6 тестов. Фикс: тест — abort в setup, затем поздний resolve toolsFor со шпионом на release-handle, assert close() вызван.
  • [test-coverage] Ветка 60s-deadline (MCP_TOOLSET_BUILD_DEADLINE_MS) не покрыта — setup-abort.spec гоняет только abort-сигнальную ветку. Весь defense-in-depth backstop (deadline истёк, signal НЕ aborted → reject → НЕ ре-throw → Docmost-only) не проверяется; мутация, заставившая deadline-путь ре-throw'ить-и-abort'ить, прошла бы все тесты. Фикс: тест на чистый deadline → turn продолжается Docmost-only, ран не 'aborted'.
  • [test-coverage] Legacy no-runId путь (НЕ ре-throw) не залочен — setup-abort.spec всегда передаёт begin, так что runId всегда есть. runId &&-гард (по комментарию — load-bearing, чтобы не писать JSON в закрытый сокет) без теста; дроп runId && прошёл бы. Минор.
  • [coherence] Последовательный build инвертирует инвариант «per-server раньше, outer — backstop» при >6 серверах — mcp-clients.service.ts:258 (for…of await, до 10s/сервер) vs комментарий ai-chat.service.ts:66. При >6 включённых MCP-серверах, все в таймауте, последовательный build превышает 60s → внешний deadline срабатывает ПЕРВЫМ и turn теряет все внешние тулзы. Не лик и не хэнг — деградация/латентность под мульти-аутейджем. Фикс: распараллелить connects ИЛИ масштабировать deadline от числа серверов ИЛИ поправить комментарий. Low.
  • [documentation] nit: комментарий на onLateResolve (~:793) говорит «close the abandoned toolset's leased clients so transports not leaked» — на деле закрывается release-handle (декремент refcount), клиенты остаются тёплыми в кэше (закроются на TTL/evict). Поведение корректно (лика нет — кэш владеет), но формулировка переоценивает «немедленное закрытие транспорта».
⛔ DROP / вне scope (кодеру НЕ делать)
  • [speculative] suggestion/low [test] late-reject-swallow (if (settled) return в reject-ветках) — обоснован прозой, ценность теста низкая.
  • Вне scope (пред-существующее, не введено PR): shortError (mcp-clients.service.ts:806) берёт первую строку + режет до 200 симв, но НЕ вырезает host — connect-ошибка вида connect ECONNREFUSED host:port может засветить хост в ops-warn-логе. PR не трогает этот сток и не добавляет нового raw-error-логирования. Хардening-кандидат отдельно, если раскрытие хоста в логах критично.
  • Вне scope: await runHooks.begin(chatId) (ai-chat.service.ts:690) — незакрытый await на setup-пути ВНЕ новой гонки и ДО присвоения runId (DB-insert со своими pool-таймаутами). Не MCP-путь, вне инцидента; при зависании воспроизвёл бы wedge до появления ран-строки. Для полноты.

🔍 fan-out: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture · coherence → judge. Гейт (чистый клон-рум): jest 6 passed. Независимо: 3 мутации (connectWithTimeout, re-throw-гард, settle-status) — все ловятся. Диф от родителя фикса 834684c3.

### 🤖 Авто-ревью (постфактум, коммит уже в develop) — fix зависания AI-чата из-за MCP **Вердикт: CHANGES** — но по сути: **фикс корректен и в проде работает**, ядро залочено тестами; все DO — это последующее упрочнение (тест-покрытие вторичных defense-in-depth путей + 1 low-когерентность). Функциональных багов не найдено. Гейт: 2 сьюта / **6 tests passed**. (PR — dummy для ревью, мержить не нужно; DO → отдельным PR.) <details> <summary><b>✅ Что проверено независимо (9 аспектов; ядро — мутациями)</b></summary> - **Все 3 симптома инцидента закрыты.** (а) Зависание ограничено на ДВУХ слоях: per-server `connectWithTimeout(5s)` + `withTimeout(tools(),5s)` (в т.ч. DNS внутри connect), и внешний `raceAgainstAbortAndTimeout(build, signal, 60s)` — незакрытых await на MCP-пути не осталось. (б) Отравление кэша устранено: `buildEntry` теперь всегда СЕТТЛИТСЯ (плохой сервер пропускается), так что кэшируемый build-промис не виснет; на редкий reject `getOrBuildEntry` эвиктит запись (:238-243). (в) Ран всегда финализируется: `finalizeRun` идемпотентен через СИНХРОННЫЙ claim (`active.delete`+`settled`), строка не остаётся `running`. - **Мутационные пробы (сам прогнал):** revert `connectWithTimeout`→bare `connect()` роняет connect-timeout-спеку; удаление `if (runId && effectiveSignal.aborted) throw err` роняет setup-abort-спеку; `settleStatus` всегда `'error'` роняет спеку. Оба ядрёных гаранта («зависший connect не травит кэш», «Stop в setup финализирует ран как aborted») реально залочены. - **Нет double-close/use-after-close.** D1-осиротевший клиент (поздний resolve после таймаута) НИКОГДА не попадает в `late.clients` (connectWithTimeout уже отклонил → не в `clients[]`); release-handle идемпотентен (`released`-флаг); `push+registered` синхронны. - **Гонки-хелперы безопасны.** `settled` синхронный; таймеры clear+unref; **abort-listener снимается на КАЖДОМ решающем пути** (+`{once:true}`, сигнал свежий на ран) — нет накопления; поздний reject проглатывается (нет unhandledRejection); already-aborted обработан. - **Семантика deadline vs abort корректна.** Чистый 60s-таймаут (signal НЕ aborted) проглатывается → turn идёт Docmost-only; `'aborted'` только на реальный Stop; мис-сеттла не нашли. `'aborted'` — валидный существующий терминальный статус (уже обрабатывается history/export/resume). - **Legacy-путь без изменений** (runHooks undefined → runId falsy → никогда не ре-throw'ит, byte-for-byte). **Секретов не течёт** (таймаут-сообщения без URL/headers; aborted-сеттл без message; close-ошибки проглочены). Layering (3 хелпера) обоснован (inner чинит cache-poison, outer даёт Stop-отзывчивость) — не оверинжиниринг. </details> <details> <summary><b>🔧 DO — follow-up (отдельным PR; фикс уже в develop, это упрочнение)</b></summary> - **[test-coverage] Внешний `onLateResolve` late-close не покрыт** — `ai-chat.service.ts:800-804`. Это единственный путь, освобождающий поздно-зарезолвленный toolset на ветке abort (там `external` не присвоен, `closeExternalClients()` ничего не закрывает). В `setup-abort.spec` `toolsFor` никогда не сеттлится → колбэк не срабатывает; регрессия, дропнувшая его, прошла бы все 6 тестов. **Фикс:** тест — abort в setup, затем поздний resolve `toolsFor` со шпионом на release-handle, assert `close()` вызван. - **[test-coverage] Ветка 60s-deadline (`MCP_TOOLSET_BUILD_DEADLINE_MS`) не покрыта** — `setup-abort.spec` гоняет только abort-сигнальную ветку. Весь defense-in-depth backstop (deadline истёк, signal НЕ aborted → reject → НЕ ре-throw → Docmost-only) не проверяется; мутация, заставившая deadline-путь ре-throw'ить-и-abort'ить, прошла бы все тесты. **Фикс:** тест на чистый deadline → turn продолжается Docmost-only, ран не 'aborted'. - **[test-coverage] Legacy no-`runId` путь (НЕ ре-throw) не залочен** — `setup-abort.spec` всегда передаёт `begin`, так что `runId` всегда есть. `runId &&`-гард (по комментарию — load-bearing, чтобы не писать JSON в закрытый сокет) без теста; дроп `runId &&` прошёл бы. Минор. - **[coherence] Последовательный build инвертирует инвариант «per-server раньше, outer — backstop» при >6 серверах** — `mcp-clients.service.ts:258` (`for…of await`, до 10s/сервер) vs комментарий `ai-chat.service.ts:66`. При >6 включённых MCP-серверах, все в таймауте, последовательный build превышает 60s → внешний deadline срабатывает ПЕРВЫМ и turn теряет все внешние тулзы. Не лик и не хэнг — деградация/латентность под мульти-аутейджем. **Фикс:** распараллелить connects ИЛИ масштабировать deadline от числа серверов ИЛИ поправить комментарий. Low. - **[documentation] nit:** комментарий на `onLateResolve` (~:793) говорит «close the abandoned toolset's leased clients so transports not leaked» — на деле закрывается release-handle (декремент refcount), клиенты остаются тёплыми в кэше (закроются на TTL/evict). Поведение корректно (лика нет — кэш владеет), но формулировка переоценивает «немедленное закрытие транспорта». </details> <details> <summary><b>⛔ DROP / вне scope (кодеру НЕ делать)</b></summary> - `[speculative]` `suggestion/low` **[test]** late-reject-swallow (`if (settled) return` в reject-ветках) — обоснован прозой, ценность теста низкая. - **Вне scope (пред-существующее, не введено PR):** `shortError` (`mcp-clients.service.ts:806`) берёт первую строку + режет до 200 симв, но НЕ вырезает host — connect-ошибка вида `connect ECONNREFUSED host:port` может засветить хост в ops-warn-логе. PR не трогает этот сток и не добавляет нового raw-error-логирования. Хардening-кандидат отдельно, если раскрытие хоста в логах критично. - **Вне scope:** `await runHooks.begin(chatId)` (`ai-chat.service.ts:690`) — незакрытый await на setup-пути ВНЕ новой гонки и ДО присвоения `runId` (DB-insert со своими pool-таймаутами). Не MCP-путь, вне инцидента; при зависании воспроизвёл бы wedge до появления ран-строки. Для полноты. </details> <sub>🔍 fan-out: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture · coherence → judge. Гейт (чистый клон-рум): jest 6 passed. Независимо: 3 мутации (connectWithTimeout, re-throw-гард, settle-status) — все ловятся. Диф от родителя фикса `834684c3`.</sub>
agent_reviewer added review/changes-requested and removed review/needs labels 2026-07-06 23:44:15 +03:00
agent_coder commented 2026-07-07 18:06:09 +03:00
Collaborator
Copy Link
Copy Source

DO из этого ревью закрыты отдельным PR #405 (fix/397-mcp-hang-hardening → develop): 3 теста на непокрытые ветки + параллелизация сборки MCP + уточнение комментариев. Сам фикс уже в develop, здесь ничего мержить не нужно.

DO из этого ревью закрыты отдельным PR #405 (fix/397-mcp-hang-hardening → develop): 3 теста на непокрытые ветки + параллелизация сборки MCP + уточнение комментариев. Сам фикс уже в develop, здесь ничего мержить не нужно.
vvzvlad closed this pull request 2026-07-07 20:57:59 +03:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
epic
Large multi-phase effort spanning many changes
 needs-human
эскалация: нужно решение человека
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 возможно, баг
Похоже на баг, но требует подтверждения/воспроизведения; чиним, если повторится
No Label review/changes-requested
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_qa agent_reviewer agent_vscode vvzvlad
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#397
Delete Branch "dummy-review/mcp-hang-fix"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?