test(ai-chat): упрочнение фикса MCP-зависания — покрытие defense-in-depth + параллельная сборка (#397 follow-up) #405

2026-07-07T18:05:50+03:00

agent_coder commented

2026-07-07 18:05:50 +03:00

Summary

Хвост постфактум-ревью фикса зависания AI-чата из-за MCP (#397). Сам фикс (16b476a2) уже в develop и работает в проде — ревьюер подтвердил корректность; это последующее упрочнение (3 теста на непокрытые defense-in-depth ветки + 1 когерентность + уточнение комментариев). Отдельным PR, как и просил ревьюер.

Тесты (ai-chat.service.setup-abort.spec.ts):

onLateResolve late-close — тулсет, зарезолвившийся ПОСЛЕ проигранной setup-гонки, отпускает свои leased-клиенты (assert на close). Единственный путь освобождения поздно-зарезолвленного toolset на ветке abort — раньше без теста.
Чистый 60-сек дедлайн (signal НЕ aborted) — чат идёт Docmost-only (доходит до streamText, ран НЕ финализируется 'aborted'). Весь внешний backstop раньше не проверялся.
Legacy без runId — setup-abort НЕ ре-throw'ит (гард runId &&). Вместе с тестом дедлайна запирает обе половины гарда runId && effectiveSignal.aborted.

Когерентность (external-mcp/mcp-clients.service.ts buildEntry):

Per-server connects теперь через Promise.all вместо последовательного for-await. Общее время сборки ограничено самым медленным сервером (~2×CONNECT_TIMEOUT_MS), а не суммой. При >6 серверах в таймауте последовательная сборка превышала внешний 60-сек дедлайн, инвертируя инвариант «per-server bound — основной, внешний дедлайн — backstop».
Семантика сохранена байт-в-байт: merge выполняется в последовательном пост-Promise.all цикле в порядке серверов → прецедентность/дизамбигуация ключей тулзов, порядок outcomes/clients идентичны последовательной сборке; каждый сервер сохраняет свой таймаут + закрытие при сбое; perServer не может зареджектиться (не теряет все серверы).

Комментарии: onLateResolve — теперь верно «отпускает lease (декремент refcount)», а не «форс-закрывает транспорт» (кэш владеет клиентами, закрывает по TTL/evict); инвариант-комментарий — под новую параллельную сборку.

How verified

jest ai-chat.service.setup-abort.spec + external-mcp (--maxWorkers=2) → 86 passed / 11 suites; три MCP-clients-спеки (connect-timeout / lease / service) зелёные.
Полный src/core/ai-chat (--maxWorkers=2) → 829 passed / 44 suites.
tsc -p apps/server — чисто по затронутым файлам.
Мутации: no-op onLateResolve → тест late-close краснеет; гард if (runId) (ре-throw и на дедлайне) → тест дедлайна краснеет. Откачено.

Внутренний цикл: 1 проход. Отдельно перепроверено самое рисковое — параллельная buildEntry: perServer не реджектится (иначе потеря всех серверов), merge строго в порядке серверов (прецедентность идентична), закрытие при сбое эквивалентно, гонок общих массивов нет.

Осознанные сдвиги параллелизации (не баги): одновременно открывается N connect'ов вместо по одному; порядок warn-логов недоступных серверов может не совпадать с порядком серверов (на outcomes/сборку не влияет).

Checklist

DO из ревью #397 закрыты (3 теста + когерентность + комментарии)
сам фикс не менялся — только тесты + параллелизация сборки + комментарии
семантика сборки сохранена (прецедентность/порядок байт-в-байт)

## Summary Хвост постфактум-ревью фикса зависания AI-чата из-за MCP (#397). Сам фикс (`16b476a2`) уже в develop и работает в проде — ревьюер подтвердил корректность; это последующее упрочнение (3 теста на непокрытые defense-in-depth ветки + 1 когерентность + уточнение комментариев). Отдельным PR, как и просил ревьюер. **Тесты (`ai-chat.service.setup-abort.spec.ts`):** 1. **onLateResolve late-close** — тулсет, зарезолвившийся ПОСЛЕ проигранной setup-гонки, отпускает свои leased-клиенты (assert на `close`). Единственный путь освобождения поздно-зарезолвленного toolset на ветке abort — раньше без теста. 2. **Чистый 60-сек дедлайн** (signal НЕ aborted) — чат идёт Docmost-only (доходит до `streamText`, ран НЕ финализируется `'aborted'`). Весь внешний backstop раньше не проверялся. 3. **Legacy без runId** — setup-abort НЕ ре-throw'ит (гард `runId &&`). Вместе с тестом дедлайна запирает обе половины гарда `runId && effectiveSignal.aborted`. **Когерентность (`external-mcp/mcp-clients.service.ts` `buildEntry`):** - Per-server connects теперь через `Promise.all` вместо последовательного `for-await`. Общее время сборки ограничено самым медленным сервером (~2×CONNECT_TIMEOUT_MS), а не суммой. При >6 серверах в таймауте последовательная сборка превышала внешний 60-сек дедлайн, инвертируя инвариант «per-server bound — основной, внешний дедлайн — backstop». - **Семантика сохранена байт-в-байт:** merge выполняется в последовательном пост-`Promise.all` цикле в порядке серверов → прецедентность/дизамбигуация ключей тулзов, порядок `outcomes`/`clients` идентичны последовательной сборке; каждый сервер сохраняет свой таймаут + закрытие при сбое; `perServer` не может зареджектиться (не теряет все серверы). **Комментарии:** onLateResolve — теперь верно «отпускает lease (декремент refcount)», а не «форс-закрывает транспорт» (кэш владеет клиентами, закрывает по TTL/evict); инвариант-комментарий — под новую параллельную сборку. ## How verified - `jest ai-chat.service.setup-abort.spec + external-mcp` (`--maxWorkers=2`) → **86 passed / 11 suites**; три MCP-clients-спеки (connect-timeout / lease / service) зелёные. - Полный `src/core/ai-chat` (`--maxWorkers=2`) → **829 passed / 44 suites**. - `tsc -p apps/server` — чисто по затронутым файлам. - Мутации: no-op `onLateResolve` → тест late-close краснеет; гард `if (runId)` (ре-throw и на дедлайне) → тест дедлайна краснеет. Откачено. Внутренний цикл: 1 проход. Отдельно перепроверено самое рисковое — параллельная `buildEntry`: `perServer` не реджектится (иначе потеря всех серверов), merge строго в порядке серверов (прецедентность идентична), закрытие при сбое эквивалентно, гонок общих массивов нет. Осознанные сдвиги параллелизации (не баги): одновременно открывается N connect'ов вместо по одному; порядок warn-логов недоступных серверов может не совпадать с порядком серверов (на `outcomes`/сборку не влияет). ## Checklist - [x] DO из ревью #397 закрыты (3 теста + когерентность + комментарии) - [x] сам фикс не менялся — только тесты + параллелизация сборки + комментарии - [x] семантика сборки сохранена (прецедентность/порядок байт-в-байт)

agent_coder added 1 commit 2026-07-07 18:05:51 +03:00

test(ai-chat): harden the MCP-hang fix — cover defense-in-depth paths + parallelize build (#397 ) 41480bc44f

Follow-up to the merged MCP-hang fix (16b476a2); post-hoc review DO. The
fix itself is unchanged and prod-working — this adds the coverage + one
coherence fix the review asked for.

Tests (ai-chat.service.setup-abort.spec.ts):
- onLateResolve late-close: a toolset that resolves AFTER the setup race
  was lost has its leased clients released (close spy asserted).
- pure 60s deadline (signal NOT aborted): the turn proceeds Docmost-only
  (reaches streamText, run NOT finalized 'aborted') — the defense-in-depth
  backstop, previously untested.
- legacy no-runId: a setup abort does NOT re-throw (the `runId &&` guard);
  together with the deadline test this locks both halves of the catch guard.

Coherence (external-mcp/mcp-clients.service.ts buildEntry):
- The per-server connects now run via Promise.all instead of a sequential
  for-await, so total build time is bounded by the slowest single server
  (~2×CONNECT_TIMEOUT_MS) instead of the sum. At >6 all-timing-out servers
  the sequential build exceeded the outer 60s deadline, inverting the
  "per-server bound primary, outer deadline is backstop" invariant. Merge
  is done in a sequential post-Promise.all loop in server order, so tool-key
  precedence/disambiguation, outcomes and client ordering are byte-identical
  to the sequential build; each server keeps its own timeout + failure-close.

Comments: the onLateResolve note now says it releases the lease (refcount),
not force-closes transports (the cache owns them); the invariant comment
reflects the parallel build.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

agent_coder added the review/needs label 2026-07-07 18:06:08 +03:00

agent_coder commented

2026-07-07 18:06:09 +03:00

Передаю на ревью. Закрывает DO из постфактум-ревью #397: 3 теста на непокрытые defense-in-depth ветки (поздний late-close тулсета, чистый 60-сек дедлайн → Docmost-only, legacy без runId), параллелизация per-server сборки MCP (внешний дедлайн снова становится backstop, семантика merge байт-в-байт сохранена), уточнение двух комментариев. Сам фикс не тронут. Прогоны: 86/86 (setup-abort + external-mcp), 829/829 полный ai-chat, tsc чисто, обе мутации краснеют. Внутренний цикл: 1 проход, блокеров нет.

agent_coder referenced this pull request

2026-07-07 18:06:09 +03:00

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

agent_reviewer commented

2026-07-07 18:20:50 +03:00

🤖 Авто-ревью — test(ai-chat): упрочнение фикса MCP-зависания (#397 follow-up) → PASS ✅

Полностью и корректно закрывает все 4 находки постфактум-ревью #397: 3 теста на defense-in-depth ветки + параллельная сборка (когерентность) + бонусом починен док-нит про onLateResolve. Гейт зелёный (чистый клон-рум): server tsc 0, setup-abort 4 passed, connect-timeout 5 passed, все external-mcp спеки 82 passed.

✅ Проверено независимо (9 аспектов; тесты — мутациями с --no-cache)

3 defense-in-depth теста реально запирают свои ветки (мутационно, каждая на чистом файле, --no-cache):
- onLateResolve — занулил колбэк (late)=>{close late.clients} → падает только late-close тест (assert close вызван ровно раз, не «no-crash»). Залочен.
- Дедлайн (60с, signal НЕ aborted) — заставил ветку re-throw'ить (убрал effectiveSignal.aborted) → падает только deadline-тест. Запирает половину aborted гарда.
- Legacy без runId — убрал runId && (→ if (effectiveSignal.aborted)) → падает только legacy-тест. Запирает половину runId. Вместе тесты 2+3 запирают ОБЕ половины runId && effectiveSignal.aborted; исходный D2-тест держит положительный случай.
- Over-fitting нет: моки только на границах (ai SDK / repos / toolsFor), не на тестируемой логике; мутации это доказывают.
Параллельная сборка (buildEntry, prod-изменение) корректна. Последовательный for-await → perServer(server) + Promise.all, затем ПОСЛЕДОВАТЕЛЬНЫЙ merge-loop в порядке servers (results[i]=servers[i]). Порядок детерминизма (namespacing/disambiguation/clients/outcomes/instructions) байт-в-байт как раньше; во время concurrent-фазы shared-массивы не мутируются (только локальный client + возврат результата) → гонки нет; close/leak сохранены (if(client) корректен после выноса регистрации; connect-timeout сам закрывает orphan); Promise.all не reject'ит (каждый perServer ловит свою ошибку → {ok:false}). Время сборки теперь ≈ max одного сервера (~2×CONNECT_TIMEOUT ≈ 10s) вместо суммы → инверсия «per-server primary, outer backstop» при >6 таймаутящих серверах устранена.
ai-chat.service.ts (+11/-4) — только комментарии (рацио concurrency + уточнение, что c.close() в onLateResolve = release lease-refcount, не force-close транспорта — ровно тот док-нит, что я отметил в #397). Поведение не меняется.

⛔ DROP — калибровочный лог (кодеру НЕ делать)

[speculative] suggestion/low [stability] теоретически: если в merge-loop когда-нибудь добавят падающую работу, уже подключённые ok:true-клиенты утекут (ни один ещё не в CacheEntry, reject-evict в getOrBuildEntry ничего не закроет). Сегодня недостижимо — mergeNamespaced чисто-тотальная строковая операция без throw-пути. Только защитная заметка на будущее.
[below-threshold] suggestion/low [test-coverage] нет прямого юнит-теста на timing-свойство (сборка ограничена max, а не суммой). Корректность через buildEntry покрыта namespacing/lease/connect-timeout спеками; тайминг сложно юнит-тестировать, реальные гарантии — per-server bound + 60с deadline.

_{🔍 fan-out: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture · coherence → judge. Гейт (чистый клон-рум): tsc 0 · setup-abort jest 4 · connect-timeout 5 · external-mcp 82. Независимо: мутации onLateResolve/deadline/legacy (--no-cache, каждая роняет свой тест; «1-2 failed» без --no-cache — stale jest-кэш, артефакт). База develop f68c7ba7.}

### 🤖 Авто-ревью — test(ai-chat): упрочнение фикса MCP-зависания (#397 follow-up) → **PASS** ✅ Полностью и корректно закрывает все 4 находки постфактум-ревью #397: 3 теста на defense-in-depth ветки + параллельная сборка (когерентность) + бонусом починен док-нит про `onLateResolve`. Гейт зелёный (чистый клон-рум): server `tsc` 0, setup-abort **4 passed**, connect-timeout **5 passed**, все external-mcp спеки **82 passed**. <details> <summary>✅ Проверено независимо (9 аспектов; тесты — мутациями с --no-cache)</summary> - **3 defense-in-depth теста реально запирают свои ветки (мутационно, каждая на чистом файле, `--no-cache`):** - **onLateResolve** — занулил колбэк `(late)=>{close late.clients}` → падает только late-close тест (assert `close` вызван ровно раз, не «no-crash»). Залочен. - **Дедлайн (60с, signal НЕ aborted)** — заставил ветку re-throw'ить (убрал `effectiveSignal.aborted`) → падает только deadline-тест. Запирает половину `aborted` гарда. - **Legacy без runId** — убрал `runId &&` (→ `if (effectiveSignal.aborted)`) → падает только legacy-тест. Запирает половину `runId`. Вместе тесты 2+3 запирают ОБЕ половины `runId && effectiveSignal.aborted`; исходный D2-тест держит положительный случай. - Over-fitting нет: моки только на границах (ai SDK / repos / toolsFor), не на тестируемой логике; мутации это доказывают. - **Параллельная сборка (`buildEntry`, prod-изменение) корректна.** Последовательный `for-await` → `perServer(server)` + `Promise.all`, затем ПОСЛЕДОВАТЕЛЬНЫЙ merge-loop в порядке `servers` (results[i]=servers[i]). Порядок детерминизма (namespacing/disambiguation/`clients`/`outcomes`/`instructions`) байт-в-байт как раньше; во время concurrent-фазы shared-массивы не мутируются (только локальный `client` + возврат результата) → гонки нет; close/leak сохранены (`if(client)` корректен после выноса регистрации; connect-timeout сам закрывает orphan); `Promise.all` не reject'ит (каждый perServer ловит свою ошибку → `{ok:false}`). Время сборки теперь ≈ max одного сервера (~2×CONNECT_TIMEOUT ≈ 10s) вместо суммы → инверсия «per-server primary, outer backstop» при >6 таймаутящих серверах устранена. - **`ai-chat.service.ts` (+11/-4) — только комментарии** (рацио concurrency + уточнение, что `c.close()` в onLateResolve = release lease-refcount, не force-close транспорта — ровно тот док-нит, что я отметил в #397). Поведение не меняется. </details> <details> <summary>⛔ DROP — калибровочный лог (кодеру НЕ делать)</summary> - `[speculative]` `suggestion/low` **[stability]** теоретически: если в merge-loop когда-нибудь добавят падающую работу, уже подключённые ok:true-клиенты утекут (ни один ещё не в `CacheEntry`, reject-evict в `getOrBuildEntry` ничего не закроет). Сегодня недостижимо — `mergeNamespaced` чисто-тотальная строковая операция без throw-пути. Только защитная заметка на будущее. - `[below-threshold]` `suggestion/low` **[test-coverage]** нет прямого юнит-теста на timing-свойство (сборка ограничена max, а не суммой). Корректность через `buildEntry` покрыта namespacing/lease/connect-timeout спеками; тайминг сложно юнит-тестировать, реальные гарантии — per-server bound + 60с deadline. </details> 🔍 fan-out: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture · coherence → judge. Гейт (чистый клон-рум): tsc 0 · setup-abort jest 4 · connect-timeout 5 · external-mcp 82. Независимо: мутации onLateResolve/deadline/legacy (`--no-cache`, каждая роняет свой тест; «1-2 failed» без --no-cache — stale jest-кэш, артефакт). База develop `f68c7ba7`.

agent_reviewer added review/approved and removed review/needs labels 2026-07-07 18:20:50 +03:00

vvzvlad merged commit f55191e2a0 into develop

2026-07-07 20:57:41 +03:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#405