Compare commits

..

19 Commits

Author SHA1 Message Date
agent_coder d84e5ddbad test(comment): покрыть fire-and-forget resolve-путь при отказе очереди (ревью #438)
resolve/unresolve enqueue — fire-and-forget (void ...catch(warn)): смысл #399
в том, что недоступность очереди НЕ должна ронять HTTP-запрос. Delete-путь уже
покрыт (enqueue awaited перед hard-delete), а reject resolve-пути — нет. Тест:
generalQueue.add реджектит -> resolveComment всё равно resolves (не throws) +
warn залогирован (ошибка проглочена на микротаске после возврата, поэтому
flushMicrotasks перед ассертом). Мутационно: сделать enqueue awaited без catch
-> тест краснеет.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 05:32:08 +03:00
agent_coder 6bf8361936 perf(comment): унести Yjs-обновление comment-mark с HTTP-критического пути (#399)
resolve/unresolve/delete comment-mark синхронно дёргали collab-gateway
(handleYjsEvent) прямо в HTTP-запросе — сетевой раунд-трип к collab на
горячем пути. Теперь:
- DB-строка пишется синхронно (источник истины) с общим таймстампом;
- сама mark-операция уходит идемпотентным COMMENT_MARK_UPDATE-джобом на
  GENERAL_QUEUE, воркер проигрывает тот же handleYjsEvent;
- resolve/unresolve — fire-and-forget (best-effort), delete — await энкью
  ДО необратимого hard-delete (durability split);
- race-guard: устаревшее ПРОТИВОПОЛОЖНОЕ событие (ts <= updatedAt строки и
  состояние расходится) пропускается, а не флипает mark в устаревшее;
- DI-цикл обойдён ленивым moduleRef.get(CollaborationGateway, strict:false).

Внутренний цикл: 2 прохода. Правки по внутреннему ревью: `<` → `<=` в
race-guard (безопасная обработка суб-миллисекундной ничьи двух
противоположных тогглов); задокументирован сознательный компромисс —
транзиентный page.updated-broadcast из воркера несёт только {id}, теряя
name/avatarUrl «кто редактировал» (lastUpdatedById выставляется верно,
косметика, самочинится на следующем реальном редактировании).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 05:15:11 +03:00
agent_vscode f46d89eafb fix(ai-chat): wire drawio CRUD tools in-app to restore SHARED_TOOL_SPECS parity
PR #434 (drawio stage 1) added drawioGet/drawioCreate/drawioUpdate to the
shared tool-spec registry with in-app metadata (inAppKey, deferred tier,
catalogLine) but wired them only in the standalone MCP server, breaking the
contract-parity and phantom-catalog unit tests on develop CI.

- expose drawioGet/drawioCreate/drawioUpdate in forUser() via sharedTool(),
  mirroring the MCP transport's argument mapping (format ?? 'xml' default,
  flat schema regrouped into the client's `where` object, positional
  baseHash pass-through)
- extend the DocmostClientLike hand-mirror with the three client methods
- append the three names to the HOST_CONTRACT_METHODS drift-guard whitelist

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-10 04:51:24 +03:00
vvzvlad ee33a293b9 Merge pull request 'feat(mcp): drawio стадия 1 — CRUD-инструменты drawio_get/create/update (сырой XML)' (#434) from feat/423-drawio-crud into develop
Reviewed-on: #434
2026-07-10 04:27:32 +03:00
vvzvlad 86830b860d Merge pull request 'feat(ai-chat): авто-реконнект к detached-рану после живого обрыва SSE' (#432) from feat/430-live-reconnect into develop
Reviewed-on: #432
2026-07-10 04:27:16 +03:00
vvzvlad d0d2a7880f Merge pull request 'feat(client): сноски — рендер без сдвига (номер инлайн через ::before) и шрифтом sm' (#421) from feat/420-footnote-render into develop
Reviewed-on: #421
2026-07-10 04:26:49 +03:00
vvzvlad 9acbc07f7d Merge pull request 'feat(ai-chat): персист tool-error частей — упавшие тулы видны в истории и сохраняют текст ошибки' (#426) from feat/407-persist-tool-errors into develop
Reviewed-on: #426
2026-07-10 04:26:37 +03:00
vvzvlad a0eb3131a6 Merge pull request 'feat(mcp): createComment — подсказки самокоррекции якоря (closest-block, markdown-strip, multi-block)' (#427) from feat/408-createcomment-hints into develop
Reviewed-on: #427
2026-07-10 04:26:25 +03:00
vvzvlad 50bb086edf Merge pull request 'perf(mcp): кэш живого collab-соединения (CollabSession) — серия правок за один connect/sync' (#431) from perf/400-collab-session into develop
Reviewed-on: #431
2026-07-10 04:26:04 +03:00
vvzvlad f2ad0121a5 Merge pull request 'perf(collab): три фикса горячего пути — connect-vs-unload гонка, двойная перекодировка, isDeepStrictEqual' (#433) from perf/401-collab-hotpath into develop
Reviewed-on: #433
2026-07-10 04:25:47 +03:00
agent_coder 9685074237 perf(collab): три фикса горячего пути сервера — connect-vs-unload гонка, двойная перекодировка, isDeepStrictEqual (замер)
Побочные находки инцидента #400 (правка большой таблицы через MCP подвешивает всё).

1. Гонка connect-vs-unload в @hocuspocus/server 3.4.4 (вероятный источник 25s
   connect-таймаутов): createDocument проверяет loadingDocuments/documents, но НЕ
   ждёт unloadingDocuments -> новое соединение может захендшейкаться на умирающий
   Document -> redis-sync идёт по пути 'doc не загружен', провайдер висит до
   таймаута. Апстрим (main) не починен. pnpm-патч (инфра как у yjs-патча): в начале
   createDocument await in-flight unload (обёрнут в try/catch — отклонённый unload
   не отравляет открытие, поведение как до патча), в ОБОИХ рантаймах (cjs+esm).
   Тест hocuspocus-unload-race: реальный createDocument с засеянным in-flight
   unload -> не грузит пока unload не осел; при откате патча тест краснеет.

2. Двойная перекодировка в onLoadDocument (persistence.extension.ts): хук строил
   НОВЫЙ Y.Doc и возвращал его -> hocuspocus делал applyUpdate(encodeStateAsUpdate)
   ВТОРОЙ раз (315КБ на каждую холодную загрузку); в JSON-ветке результат encode
   выбрасывался (мёртвый вызов). Теперь стейт применяется прямо в data.document,
   возврат undefined (hocuspocus мержит только при возврате Doc); мёртвый encode
   убран. Содержимое документа не меняется — только меньше encode/alloc.

3. isDeepStrictEqual по 84КБ JSON на каждом store: замерил — 1.32мс на 90КБ
   (immaterial, <50мс порога; доминируют fromYdoc+encodeStateAsUpdate). Изменений
   кода НЕТ по правилу задачи (dirty-флаг только при material).

closes #401

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 02:53:23 +03:00
agent_coder 22f687c39e feat(ai-chat): авто-реконнект к detached-рану после живого обрыва SSE
Автономный ран продолжается на сервере при обрыве SSE (Safari роняет длинный
стрим), но клиент показывал баннер 'Lost connection' и мёртвую вкладку до ручной
перезагрузки: resumeStream() звался только на mount. Добавлен недостающий триггер.

- chat-thread.tsx: в onFinish на живом isDisconnect (гард !wasResumed &&
  autonomousRunsEnabled && mounted && assistant) -> beginReconnect с экспон.
  backoff (1/2/4/8/16с, лимит 5). Стоп: status->streaming / 2xx re-attach /
  терминальный хвост reconcile / stop / unmount. Исчерпание -> Retry.
- Дедуп (главный риск): зеркалит mount strip/anchor — пиннит текущий streaming-ряд
  как anchor (id ассистент-строки), стрипает его из стора ДО replay, сервер
  ?expect=live&anchor=<id> пересобирает без дублей; на отказе/204 строка
  восстанавливается через onNoActiveStream (контент не теряется).
- 204/overflow -> degraded poll через существующий onNoActiveStream.
- RUN_STREAM_MAX_BUFFER_BYTES 4->32МБ (марафонские раны 11-25мин переполняли 4МБ);
  204->poll остаётся backstop. Degraded-poll: фиксированный 10-мин-от-старта кап
  заменён на inactivity-кап (продлевается пока приходят новые ряды).
- UI: баннер 'reconnecting… (N/5)' + ручной Retry на исчерпании.

closes #430

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 02:51:58 +03:00
agent_coder 0d4f719f47 fix(mcp): fail-fast гард на одновременный mutate одной CollabSession (ревью #431)
mutate трекает in-flight одним полем inflightReject; наложенный второй вызов
перезаписал бы рехджектор первого -> при disconnect отклонился бы только второй,
первый бы висел до PERSIST_TIMEOUT_MS (20с). В проде безопасно (оба call-site
сериализуют через per-page withPageLock), но это футган на разделяемом примитиве.
Гард в начале mutate (после ready-проверки, до касания inflightReject): наличие
in-flight -> reject нового вызова без порчи состояния первого. Docstring CONCURRENCY.
Последовательные mutate не задеты (localFinish синхронно чистит inflightReject до
резолва). +2 теста: конкурентный второй реджектится, первый цел; последовательные
оба успешны.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 02:44:40 +03:00
agent_coder 572f0a2ab9 perf(mcp): кэш живого collab-соединения (CollabSession) — серия правок за один connect/sync
Инцидент: агент заполнял большую таблицу десятками table_update_cell, каждый —
полный цикл connect/auth/load/initial-sync/store/unload; часть падала с 25s connect-
timeout, event loop lag до 1.7с. Вариант B: кэшировать живой HocuspocusProvider
per page на серию правок — пока провайдер жив (connections>0), сервер не входит в
store->unload->reload, дебаунс реально коалесцирует записи (N ячеек -> 1-2 store).

Новый модуль collab-session.ts: класс CollabSession (connecting->ready->dead) +
реестр (ключ wsUrl+pageId+token) с idle-TTL/max-age/LRU-evict. mutatePageContent
(collaboration.ts) и mutateLiveContentUnlocked (client.ts, replaceImage) переведены
на acquireCollabSession; one-shot Promise-машина (~360 строк дублирования) удалена.

5 инвариантов (подтверждены внутренним ревью): (1) read->write атомарна — между
fromYdoc и applyDocToFragment нет await; (2) per-edit ack сохранён дословно (гард
connectionLost от false-success при реконнекте); (3) disconnect=смерть сессии (без
авто-реконнекта, in-flight реджектится теми же текстами ошибок); (4) изоляция
identity (токен в ключе); (5) валидация при reuse. replaceImage работает под
внешним page-локом без дедлока (acquire лок не берёт). Тексты ошибок == develop.

Env: MCP_COLLAB_SESSION_IDLE_MS (0=выкл кэш, точное легаси), _MAX_AGE_MS, _MAX_ENTRIES.
Teardown: destroyAllSessions обвязан в stdio (exit/SIGINT/SIGTERM) + реэкспорт из
index для встраивающего хоста.

closes #400

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 02:28:30 +03:00
agent_coder 72c2d1687e docs(mcp): починить осиротевший docstring + уточнить коммент про exact-wins (ревью #427)
- text-normalize.ts: closestBlockHint был вставлен между docstring'ом
  stripInlineMarkdown и его определением -> docstring осиротел. closestBlockHint
  перенесён ПОСЛЕ stripInlineMarkdown, каждый docstring снова примыкает к своей
  функции. Поведение не менялось (только порядок объявлений).
- comment-anchor.ts: header-коммент завышал маршрутизацию — countAnchorMatches НЕ
  зовёт resolveAnchorSelection, у него своя параллельная реализация exact-wins.
  Коммент уточнён: can/get/apply идут через resolveAnchorSelection, count держит
  свой счётчик-примитив, синхронный с ним; обе реализации exact-wins должны
  держаться в синхроне при правках.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-10 00:06:28 +03:00
agent_coder 96faa28220 docs: отразить ключ error в заглавной секции reading-ai-logs (устранить противоречие)
Ревью #426: секция «How tool calls are stored — READ THIS» всё ещё утверждала,
что единственные ключи элемента — toolName/input/output и «нет error», хотя этот
же PR добавляет error и подробно описывает его ниже. Заглавный абзац приведён в
соответствие: error — возможный ключ для брошенных ошибок на строках после #407;
подсчёт инвокаций и пайринг учитывают error как парный результат.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-09 23:50:05 +03:00
agent_coder c9293e316b feat(mcp): createComment — подсказки самокоррекции якоря (closest-block, markdown-strip, multi-block)
createComment — топ-хотспот ошибок агента (промахи по якорю, слепые ретраи).
Портированы аффордансы самокоррекции из editPageText:
- Closest-block hint: общий хелпер closestBlockHint вынесен в text-normalize.ts
  (json-edit.ts теперь тоже его зовёт), подключён во все 3 throw-а createComment.
- Markdown-strip fallback в comment-anchor.ts согласованно по всем 4 функциям
  (can/count/apply/get) через единый resolveAnchorSelection: exact-verbatim wins
  глобально, stripped — только если raw не якорится нигде; soft warning как в
  editPageText. Инвариант уникальности suggestion (0/1/>=2) сохранён: raw-unique
  никогда не запускает fallback -> не может стать ambiguous. Хранимый selection
  остаётся СЫРОЙ подстрокой документа (strip только для поиска).
- Multi-block detection: явное сообщение 'selection spans multiple blocks' когда
  per-block поиск провалился, но выделение есть в объединённом тексте блоков.
- tool-spec createComment: копировать selection дословно из getPage/searchInPage.
Известное мелкое ограничение (нит внутреннего ревью): детектор multi-block
использует raw selection, поэтому markdown-стилизованное выделение через границу
блоков получит generic-подсказку вместо spans-multiple-blocks (редко, guidance
всё равно корректный).

closes #408

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-09 23:45:11 +03:00
agent_coder 654ba9f249 feat(ai-chat): персист tool-error частей — упавшие тулы видны в истории и сохраняют текст ошибки при реплее
В ai@6 упавший тул — это tool-error часть в step.content ({type,toolCallId,
toolName,input,error}), а не элемент toolResults. Раньше serializeSteps писал
только toolCalls+toolResults (ошибка терялась, orphan tool-call без результата),
а assistantParts эмитил заглушку 'Tool call did not complete.' (реальный текст
ошибки терялся для мультиходового реплея — модель не знала, почему упало, и
повторяла ошибку).

- StepLike расширен полем content; новый хелпер normalizeToolError (Error/string/
  object -> строка, обрезка через существующий compactValue/лимиты).
- serializeSteps: на каждый tool-error пушит парный {toolName, error} тем же
  паттерном, что успешный {toolName, output} -> колонка tool_calls фиксирует сбой.
- assistantParts: при наличии tool-error эмитит output-error с РЕАЛЬНЫМ текстом;
  заглушка остаётся только для по-настоящему непарных вызовов (прерванных).
- docs/reading-ai-logs.md обновлён под новую форму + cutover-оговорка.
Обратно совместимо: старые строки читаются как раньше, error-элемент аддитивен.

closes #407

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-09 23:35:32 +03:00
agent_coder 9120ad3b2d feat(client): сноски — рендер без сдвига (номер инлайн через ::before) и шрифтом sm
Убран отдельный столбец-маркер .definitionMarker (order:-1, min-width:1.5em),
дававший висячий отступ. Номер сноски теперь рисуется инлайн в начале первого
параграфа через .definitionContent > :first-child::before из CSS-переменной
--footnote-number (в модель документа не попадает, экспорт не затрагивает).
Кегль сносок уменьшен до var(--mantine-font-size-sm). Инвариант #146 (contentDOM
первый в DOM) и логика мульти-бэклинков #168 сохранены.

closes #420

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-09 23:12:35 +03:00
41 changed files with 3287 additions and 635 deletions
@@ -86,11 +86,19 @@ const MIN_HEIGHT = 400;
// Margin kept between the window and the viewport edges while dragging.
const EDGE_MARGIN = 8;
// #184 phase 1.5: hard cap on the degraded-poll fallback. The poll is armed when
// a resume attempt could not attach to the live run and disarmed by the thread on
// settle / local stream; this cap is the ONLY backstop against an endless tick
// (a stuck 'streaming' row before the boot-sweep, or a user-tail 204 with no run).
const DEGRADED_POLL_MAX_MS = 10 * 60_000;
// #184 phase 1.5 / #430: backstop for the degraded-poll fallback. The poll is
// armed when a resume attempt could not attach to the live run and disarmed by the
// thread on settle / local stream; this cap is the ONLY backstop against an endless
// tick (a stuck 'streaming' row before the boot-sweep, or a user-tail 204 with no
// run).
//
// #430: measured from RUN ACTIVITY, not from arm-time. A real autonomous run takes
// 11-25 min — longer than a fixed 10-min-from-start cap, which used to cut the poll
// off mid-run. Instead we cap on INACTIVITY: keep polling as long as the run is
// still making progress (its persisted rows keep changing), and only give up after
// this long with NO new activity. A genuinely stuck run produces no row changes, so
// the idle cap still bounds it; a long-but-progressing run polls to completion.
const DEGRADED_POLL_IDLE_MAX_MS = 10 * 60_000;
/** Compact token formatter: 1.2M / 3.4k / 950. */
function formatTokens(n: number): string {
@@ -254,9 +262,12 @@ export default function AiChatWindow() {
// onResumeFallback(true); the thread disarms it on settle / local stream. The
// window only OWNS the timer (armedAtRef stamps when it was armed for the cap).
const [degradedPoll, setDegradedPoll] = useState(false);
const armedAtRef = useRef(0);
// #430: timestamp of the LAST run activity while the poll is armed — stamped on
// arm and re-stamped whenever the polled rows change (see the effect below). The
// idle cap is measured from this, so a long-but-progressing run keeps polling.
const lastActivityAtRef = useRef(0);
const onResumeFallback = useCallback((active: boolean): void => {
if (active) armedAtRef.current = Date.now();
if (active) lastActivityAtRef.current = Date.now();
setDegradedPoll(active);
}, []);
// Reset the degraded poll whenever the open chat changes: it is scoped to the
@@ -269,18 +280,28 @@ export default function AiChatWindow() {
useAiChatMessagesQuery(
activeChatId ?? undefined,
// DELIBERATELY DUMB (invariant 8 / task 2.4): poll every 2.5s while armed
// and under the 10-min cap; otherwise off. NO error checks (TanStack v5
// resets fetchFailureCount each fetch, so consecutive errors are not
// expressible — and the poll must survive a server restart) and NO tail
// checks (the settled/local-stream semantics live in ChatThread, which
// disarms via onResumeFallback(false)). The time cap is the only backstop.
// and while the run is still active (#430: under the INACTIVITY cap, not a
// fixed-from-start cap); otherwise off. NO error checks (TanStack v5 resets
// fetchFailureCount each fetch, so consecutive errors are not expressible —
// and the poll must survive a server restart) and NO tail checks (the
// settled/local-stream semantics live in ChatThread, which disarms via
// onResumeFallback(false)). The idle cap is the only backstop.
() =>
degradedPoll === true &&
Date.now() - armedAtRef.current < DEGRADED_POLL_MAX_MS
Date.now() - lastActivityAtRef.current < DEGRADED_POLL_IDLE_MAX_MS
? 2500
: false,
);
// #430: re-stamp the activity clock whenever the polled rows change while the
// poll is armed. TanStack keeps the same `messageRows` reference across refetches
// that return deep-equal data (structural sharing), so a new reference means the
// run genuinely progressed — which extends the inactivity cap above. A stuck run
// yields no reference change, so the cap eventually fires and stops the poll.
useEffect(() => {
if (degradedPoll) lastActivityAtRef.current = Date.now();
}, [degradedPoll, messageRows]);
// #184 reconnect-and-live-follow. Whether detached agent runs are enabled for
// this workspace. When the feature is off no runs are ever created, so the
// resume attempt would only ever 204; gating ChatThread's resume on it avoids a
@@ -739,3 +739,170 @@ function renderResumable(initialRows: IAiChatMessageRow[]) {
act(() => view.rerender(<Wrapper rows={rows} />));
return { rerender, onResumeFallback };
}
// #430: auto-reconnect to a DETACHED run after a LIVE SSE disconnect. The mount
// path only resumes on mount/reload; these cover the missing trigger — a live
// `isDisconnect` on onFinish must (backoff-)re-attach WITHOUT a reload, pin+strip
// the live row to avoid duplicates, fall back to the degraded poll on a 204, and
// exhaust to a manual Retry.
describe("ChatThread — live reconnect after isDisconnect (#430)", () => {
// A LIVE local turn that just dropped: the settled tail existed before, and the
// partial assistant row lives only in `messages` (not persisted as a tail).
const settledTail = () => [
row("u1", "user", undefined, "hi"),
row("a1", "assistant", "succeeded", "done"),
];
// The partial assistant message onFinish hands us for the dropped LIVE turn.
const liveMsg = {
id: "a2",
role: "assistant",
parts: [{ type: "text", text: "partial live answer" }],
};
beforeEach(() => {
resetState();
// status "ready": with a live disconnect the mock is not streaming, so the
// status==="streaming" auto-clear effect stays out of the way.
h.state.status = "ready";
vi.useFakeTimers();
});
afterEach(() => {
vi.useRealTimers();
cleanup();
});
// Render a NON-resuming mount (settled tail -> no mount resume) with autonomous
// runs on, then simulate a live disconnect via onFinish.
function renderLiveThenDisconnect() {
const view = renderThread({
autonomousRunsEnabled: true,
initialRows: settledTail(),
});
// The settled tail must NOT have triggered a mount resume.
expect(h.state.resumeStream).not.toHaveBeenCalled();
act(() => {
h.state.onFinish?.({
message: liveMsg,
isAbort: false,
isDisconnect: true,
isError: false,
});
});
return view;
}
// Fire the pending (scheduled) attempt for `attempt` (backoff = 1s,2s,4s,...).
function advanceToAttempt(attempt: number) {
act(() => {
vi.advanceTimersByTime(1000 * 2 ** (attempt - 1));
});
}
// Simulate the reconnect GET returning 204 (nothing live) so the transport's
// no-active-stream recovery runs.
async function reconnect204() {
vi.stubGlobal(
"fetch",
vi.fn().mockResolvedValue({ status: 204, ok: false }),
);
await act(async () => {
await h.state.transport!.fetch!("http://x", { method: "GET" });
});
}
// Simulate the reconnect GET returning a live 2xx stream.
async function reconnect200() {
vi.stubGlobal(
"fetch",
vi.fn().mockResolvedValue({ status: 200, ok: true }),
);
await act(async () => {
await h.state.transport!.fetch!("http://x", { method: "GET" });
});
}
it("calls resumeStream POST-mount (a live disconnect triggers a backoff reconnect)", () => {
renderLiveThenDisconnect();
// The banner shows immediately; the attach itself fires after the first backoff.
expect(screen.getByText(/reconnecting/i)).toBeTruthy();
expect(h.state.resumeStream).not.toHaveBeenCalled();
advanceToAttempt(1);
// resumeStream is now called AFTER mount — the bug was it only ever fired once
// on mount. The reconnect URL pins expect=live&anchor to OUR run.
expect(h.state.resumeStream).toHaveBeenCalledTimes(1);
expect(h.state.transport!.prepareReconnectToStreamRequest!().api).toBe(
"/api/ai-chat/runs/c1/stream?expect=live&anchor=a2",
);
});
it("strips the pinned live row before replay so content is NOT duplicated", () => {
renderLiveThenDisconnect();
advanceToAttempt(1);
// The attempt strips the anchor row from the store (the live replay rebuilds
// it). Apply the setMessages updater to prove it removes exactly the anchor.
const updater = h.state.setMessages.mock.calls.at(-1)![0] as (
prev: { id: string }[],
) => { id: string }[];
expect(updater([{ id: "u1" }, { id: "a2" }])).toEqual([{ id: "u1" }]);
});
it("a live re-attach (2xx) clears the reconnect banner", async () => {
renderLiveThenDisconnect();
advanceToAttempt(1);
await reconnect200();
expect(screen.queryByText(/reconnecting/i)).toBeNull();
});
it("a 204 arms the degraded poll and backs off to the next attempt", async () => {
const { onResumeFallback } = renderLiveThenDisconnect();
advanceToAttempt(1);
expect(h.state.resumeStream).toHaveBeenCalledTimes(1);
await reconnect204();
// Fallback engaged: the degraded poll is armed (204 -> onNoActiveStream).
expect(onResumeFallback).toHaveBeenCalledWith(true);
// Still reconnecting — the banner advanced to attempt 2/5.
expect(screen.getByText(/reconnecting.*2\/5/i)).toBeTruthy();
// The next backoff fires attempt 2 (another resumeStream).
advanceToAttempt(2);
expect(h.state.resumeStream).toHaveBeenCalledTimes(2);
});
it("exhausts the attempt limit into a manual Retry, which restarts the sequence", async () => {
renderLiveThenDisconnect();
// Drive all 5 attempts, each failing with a 204.
for (let n = 1; n <= 5; n++) {
advanceToAttempt(n);
expect(h.state.resumeStream).toHaveBeenCalledTimes(n);
await reconnect204();
}
// The 5th 204 exhausted the cap -> the manual Retry replaces the banner.
expect(screen.queryByText(/reconnecting/i)).toBeNull();
const retry = screen.getByText("Retry");
expect(retry).toBeTruthy();
// Retry fires attempt 1 immediately (no backoff) — a 6th resumeStream.
act(() => {
fireEvent.click(retry);
});
expect(h.state.resumeStream).toHaveBeenCalledTimes(6);
expect(screen.getByText(/reconnecting/i)).toBeTruthy();
});
it("does NOT reconnect when autonomous runs are disabled", () => {
renderThread({ autonomousRunsEnabled: false, initialRows: settledTail() });
act(() => {
h.state.onFinish?.({
message: liveMsg,
isAbort: false,
isDisconnect: true,
isError: false,
});
});
expect(screen.queryByText(/reconnecting/i)).toBeNull();
// The terminal "connection lost" notice is shown instead (unchanged behavior).
expect(
screen.getByText("Connection lost — the answer was interrupted."),
).toBeTruthy();
advanceToAttempt(1);
expect(h.state.resumeStream).not.toHaveBeenCalled();
});
});
@@ -1,7 +1,17 @@
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
import { useQueryClient } from "@tanstack/react-query";
import { generateId } from "ai";
import { ActionIcon, Box, Group, Stack, Text, Tooltip } from "@mantine/core";
import {
ActionIcon,
Alert,
Box,
Button,
Group,
Loader,
Stack,
Text,
Tooltip,
} from "@mantine/core";
import {
IconClockHour4,
IconPlayerPlayFilled,
@@ -51,6 +61,15 @@ import classes from "@/features/ai-chat/components/ai-chat.module.css";
// from the token rate.
const STREAM_THROTTLE_MS = 50;
// #430: auto-reconnect after a LIVE SSE disconnect of a DETACHED (autonomous) run.
// The run keeps executing server-side, so instead of a dead "Lost connection"
// banner we re-attach to the live tail through the SAME resumable machinery the
// mount path uses. Attempts back off exponentially and are capped; on exhaustion
// the user gets a manual Retry (the degraded poll keeps catching up underneath).
const RECONNECT_MAX_ATTEMPTS = 5;
// Backoff before attempt N (1-based): 1s, 2s, 4s, 8s, 16s.
const RECONNECT_BASE_DELAY_MS = 1000;
/** The page the user is currently viewing, sent as chat context. */
export interface OpenPageContext {
id: string;
@@ -175,6 +194,10 @@ export default function ChatThread({
const reconcileTailRef = useRef(false);
const noStreamHandledRef = useRef(false);
const onNoActiveStreamRef = useRef<(() => void) | null>(null);
// #430: called from the transport's reconnect-GET success branch when a live
// stream re-attached (2xx, not 204) — clears the reconnect banner. Kept in a ref
// because the transport's fetch closure (useMemo([])) reads it live.
const onReconnectAttachedRef = useRef<(() => void) | null>(null);
// Live mount flag. The attach GET and the resumed `onFinish` are async and can
// land AFTER this thread unmounts (the parent remounts per chat via `key`); with
// chatIdRef then pointing at the NEW chat, an ungated late callback would arm a
@@ -378,6 +401,10 @@ export default function ChatThread({
// NOT drop the in-progress row or stop tracking the durable run.
if (response.status === 204 || !response.ok)
onNoActiveStreamRef.current?.();
// #430: a 2xx stream re-attached (live tail or finished-replay). Signal
// the reconnect controller to clear its banner. No-op outside an active
// reconnect sequence (e.g. the mount attach), so it is safe here.
else onReconnectAttachedRef.current?.();
return response;
} catch (err) {
// Network throw: same no-onFinish recovery, then rethrow so the SDK
@@ -481,6 +508,31 @@ export default function ChatThread({
);
}
}
// (2b) #430: a LIVE (non-resumed) detached run whose SSE just dropped. The
// server run keeps executing, so instead of a dead "Lost connection" banner
// start a reconnect sequence: pin the CURRENT streaming assistant row as the
// strip/anchor (the live tail is the already-shown partial in `messages`, not
// a persistent row) and re-attach to the live tail via the resumable machinery.
const startedReconnect =
isDisconnect &&
!wasResumed &&
autonomousRunsEnabled === true &&
mountedRef.current &&
message?.role === "assistant" &&
typeof message.id === "string";
if (startedReconnect) {
beginReconnect({
id: message.id,
role: "assistant",
content: "",
status: "streaming",
createdAt: new Date().toISOString(),
// Preserve the partial parts so a 204 restore (onNoActiveStream) re-shows
// what was on screen while the degraded poll catches the run up to
// terminal (rowToUiMessage prefers metadata.parts).
metadata: { parts: message.parts },
});
}
// (3) Standard branches.
// Forward the authoritative server chatId (streamed on the assistant
// message metadata) so the parent adopts the REAL created chat id for a new
@@ -490,9 +542,11 @@ export default function ChatThread({
onTurnFinished(extractServerChatId(message), threadKey);
// Show a neutral "stopped" marker for an aborted turn; the red error banner
// (via `error`) already covers isError, and a clean finish clears any marker.
// On a live disconnect that STARTED a reconnect, suppress the terminal
// "connection lost" notice — the reconnect banner takes over (#430).
if (isError) setStopNotice(null);
else if (isAbort) setStopNotice("manual");
else if (isDisconnect) setStopNotice("disconnect");
else if (isDisconnect) setStopNotice(startedReconnect ? null : "disconnect");
else setStopNotice(null);
// A resumed turn NEVER flushes the queue (invariant 7): skip BOTH the
// flush-on-abort branch and the plain flush. The local streamer is the only
@@ -579,6 +633,106 @@ export default function ChatThread({
const isStreaming = status === "submitted" || status === "streaming";
// #430: live-disconnect reconnect controller. `null` = idle; `{ trying, attempt }`
// = a backoff sequence is running (drives the "reconnecting… (N/max)" banner);
// `{ failed }` = attempts exhausted (drives the manual Retry). Mirrored into a ref
// so the transport/onNoActiveStream closures branch on the LIVE value.
type ReconnectState =
| null
| { phase: "trying"; attempt: number }
| { phase: "failed" };
const [reconnectState, setReconnectState] = useState<ReconnectState>(null);
const reconnectStateRef = useRef<ReconnectState>(null);
const setReconnectStatePair = useCallback((s: ReconnectState) => {
reconnectStateRef.current = s;
setReconnectState(s);
}, []);
const reconnectTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const clearReconnectTimer = useCallback(() => {
if (reconnectTimerRef.current) {
clearTimeout(reconnectTimerRef.current);
reconnectTimerRef.current = null;
}
}, []);
// One reconnect attempt — MIRRORS the mount strip/anchor path for the LIVE case.
// beginReconnect pinned strippedRowRef/stripRef to the run's assistant row, so:
// - remove that row from the store (the mount path strips it from the SEED; here
// it is already shown, so filter it out) — the live replay's `text-start` then
// rebuilds it without DUPLICATING parts (the main dedup risk, #430);
// - reset the one-shot 204 guard so onNoActiveStream can fire for THIS attempt;
// - mark the turn resumed (invariant 7/8) so onFinish runs the recovery block and
// never flushes the queue;
// - resumeStream() -> prepareReconnectToStreamRequest builds
// ?expect=live&anchor=<pinned id>, pinning the replay to OUR run (invariant 6).
const attemptReconnectOnce = useCallback(
(attempt: number) => {
if (!mountedRef.current) return;
const anchor = strippedRowRef.current;
if (anchor) {
setMessages((prev) => prev.filter((m) => m.id !== anchor.id));
}
noStreamHandledRef.current = false;
setResumedTurnPair(true);
setReconnectStatePair({ phase: "trying", attempt });
void resumeStream();
},
[setMessages, setResumedTurnPair, setReconnectStatePair, resumeStream],
);
// Schedule attempt `attempt` after an exponential backoff.
const scheduleReconnectAttempt = useCallback(
(attempt: number) => {
clearReconnectTimer();
setReconnectStatePair({ phase: "trying", attempt });
reconnectTimerRef.current = setTimeout(
() => attemptReconnectOnce(attempt),
RECONNECT_BASE_DELAY_MS * 2 ** (attempt - 1),
);
},
[clearReconnectTimer, setReconnectStatePair, attemptReconnectOnce],
);
// Start a fresh reconnect sequence, pinning `anchorRow` (the live run's assistant
// row) as the strip/anchor reused by every attempt.
const beginReconnect = useCallback(
(anchorRow: IAiChatMessageRow) => {
if (!autonomousRunsEnabled || !mountedRef.current) return;
strippedRowRef.current = anchorRow;
stripRef.current = true;
scheduleReconnectAttempt(1);
},
[autonomousRunsEnabled, scheduleReconnectAttempt],
);
// Manual Retry (shown once attempts are exhausted): restart at attempt 1 and fire
// immediately (the user asked for it now — no backoff).
const retryReconnect = useCallback(() => {
clearReconnectTimer();
attemptReconnectOnce(1);
}, [clearReconnectTimer, attemptReconnectOnce]);
// Live SSE re-attached (the reconnect GET returned a 2xx stream): clear the
// banner + any pending backoff. No-op outside a sequence (e.g. the mount attach).
const onReconnectAttached = useCallback(() => {
if (!mountedRef.current || !reconnectStateRef.current) return;
clearReconnectTimer();
setReconnectStatePair(null);
}, [clearReconnectTimer, setReconnectStatePair]);
onReconnectAttachedRef.current = onReconnectAttached;
// The reconnect GET could not attach (204 / error). onNoActiveStream has already
// armed the degraded poll (the robust fallback that drives the row to terminal
// from the DB), so this only decides the LIVE-attach retry: back off and try
// again up to the cap, else surface the manual Retry.
const onReconnectNoStream = useCallback(() => {
const s = reconnectStateRef.current;
if (s?.phase !== "trying") return;
if (s.attempt < RECONNECT_MAX_ATTEMPTS)
scheduleReconnectAttempt(s.attempt + 1);
else setReconnectStatePair({ phase: "failed" });
}, [scheduleReconnectAttempt, setReconnectStatePair]);
// 204-handler (`onNoActiveStream`): the attach returned 204 — nothing live to
// resume (overflow / begin-failure / after retention / anchor-mismatch). One-
// shot via noStreamHandledRef (we do NOT null onNoActiveStreamRef). Exactly four
@@ -610,7 +764,17 @@ export default function ChatThread({
// (d) 204 means onFinish will NOT fire — clear the suppression flag so it
// cannot swallow the NEXT local turn's queue flush.
setResumedTurnPair(false);
}, [setMessages, queryClient, onResumeFallback, setResumedTurnPair]);
// (e) #430: if this 204/error landed during a live-disconnect reconnect
// sequence, back off and retry the live attach (or give up to the manual
// Retry). The degraded poll armed in (c) is the fallback either way.
onReconnectNoStream();
}, [
setMessages,
queryClient,
onResumeFallback,
setResumedTurnPair,
onReconnectNoStream,
]);
onNoActiveStreamRef.current = onNoActiveStream;
// Mount effect: kick off the resume attempt for a non-settled tail. Marking the
@@ -628,6 +792,9 @@ export default function ChatThread({
return () => {
mountedRef.current = false;
attachAbortRef.current?.abort();
// #430: drop any pending reconnect backoff so it can't fire against the next
// chat this thread's refs are reused for.
if (reconnectTimerRef.current) clearTimeout(reconnectTimerRef.current);
};
// Mount-only by design; the parent remounts per chat via `key`.
// eslint-disable-next-line react-hooks/exhaustive-deps
@@ -666,12 +833,27 @@ export default function ChatThread({
if (tail.status !== "streaming") {
reconcileTailRef.current = false;
onResumeFallback?.(false);
// #430: the run reached its terminal state via the degraded poll — there is
// no live tail left to reconnect to, so drop any reconnect banner / Retry.
clearReconnectTimer();
setReconnectStatePair(null);
}
// onResumeFallback intentionally omitted (parent-stable callback); deps are
// fixed by the resume design.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialRows, isStreaming, setMessages]);
// #430: a real stream is live again — the reconnect re-attached to the live tail
// (status -> "streaming") OR the user started a new local turn. Either way clear
// the reconnect banner + any pending backoff. Gated on "streaming" (not the
// broader "submitted") so a still-pending attach GET does not clear prematurely.
useEffect(() => {
if (status === "streaming") {
clearReconnectTimer();
setReconnectStatePair(null);
}
}, [status, clearReconnectTimer, setReconnectStatePair]);
// "Send now" on a queued message: interrupt the current turn and immediately
// send THIS message, keeping the agent's partial output. Other queued messages
// stay queued and flush normally after the new turn. Reuses the existing
@@ -719,6 +901,9 @@ export default function ChatThread({
// observer's Stop would otherwise leave the attach fetch running.
attachAbortRef.current?.abort();
stop();
// #430: pressing Stop also cancels an in-progress reconnect sequence.
clearReconnectTimer();
setReconnectStatePair(null);
if (!autonomousRunsEnabled) return;
if (chatIdRef.current) {
onServerStop?.(chatIdRef.current);
@@ -740,7 +925,13 @@ export default function ChatThread({
// for this fix. Documented so a future change can address the abort-ordering.
stopPendingRef.current = true;
}
}, [stop, autonomousRunsEnabled, onServerStop]);
}, [
stop,
autonomousRunsEnabled,
onServerStop,
clearReconnectTimer,
setReconnectStatePair,
]);
// Clear the stopped marker as soon as a new turn begins streaming, and drop any
// stale "Send now" interrupt flags. On the legit interrupt path both refs are
@@ -825,6 +1016,43 @@ export default function ChatThread({
detail={errorView.detail}
mb="xs"
/>
) : reconnectState ? (
// #430: while auto-reconnecting to a detached run's live tail, show progress
// instead of a dead "Lost connection" banner; once attempts are exhausted,
// offer a manual Retry (the degraded poll keeps catching up underneath).
<Alert
variant="light"
color="gray"
p="xs"
mb="xs"
style={{ flexShrink: 0 }}
>
<Group gap={8} wrap="nowrap" align="center">
{reconnectState.phase === "trying" ? (
<>
<Loader size={14} color="gray" style={{ flex: "none" }} />
<Text size="sm" lh={1.3} c="dimmed">
{t("Connection lost — reconnecting…")}
{` (${reconnectState.attempt}/${RECONNECT_MAX_ATTEMPTS})`}
</Text>
</>
) : (
<>
<Text size="sm" lh={1.3} c="dimmed" style={{ flex: 1 }}>
{t("Couldn't reconnect to the answer.")}
</Text>
<Button
size="compact-xs"
variant="light"
color="gray"
onClick={retryReconnect}
>
{t("Retry")}
</Button>
</>
)}
</Group>
</Alert>
) : stopNotice ? (
<ChatStoppedNotice
text={
@@ -49,19 +49,14 @@ export default function FootnoteDefinitionView(props: NodeViewProps) {
className={classes.definition}
style={{ ["--footnote-number" as any]: `"${number}"` }}
>
{/* #146: contentDOM MUST be the first child — a non-editable marker before
{/* #146: contentDOM MUST be the first child — non-editable chrome before
it makes click hit-testing snap the caret above. Content first; the
marker + back-link follow in DOM and are placed left/right via CSS
flex `order`. The second #146 mitigation lives in
back-link follows in DOM and is placed on the right via CSS flex. The
decorative "N." number is rendered inline via the .definitionContent
::before rule (from the --footnote-number var), so no marker element
precedes the content. The second #146 mitigation lives in
editor-paste-handler.tsx (reflowAfterPaste). */}
<NodeViewContent className={classes.definitionContent} />
<span
className={classes.definitionMarker}
contentEditable={false}
aria-hidden="true"
>
{number}.
</span>
{refCount > 1 ? (
// Multiple references -> ↩ followed by one lettered link per occurrence.
<span
@@ -81,34 +81,34 @@
.definition {
display: flex;
align-items: flex-start;
/* Tight number→text spacing (~one space) so it reads like "1. text"
instead of leaving a wide gap after the period. */
gap: 0.4em;
/* Tight spacing between the content and the trailing ↩ back-link. */
gap: 0.3em;
padding: 2px 0;
/* Footnotes read smaller than body text (16px). Matches .listHeading. */
font-size: var(--mantine-font-size-sm);
}
.definitionMarker {
order: -1; /* keep the "N." marker on the LEFT though it follows content in DOM (#146) */
flex: 0 0 auto;
min-width: 1.5em;
/* Right-align within the narrow column so the period sits next to the text
and multi-digit numbers (10, 11, …) stay aligned on their right edge. */
text-align: right;
font-variant-numeric: tabular-nums;
color: var(--mantine-color-dimmed);
user-select: none;
}
/* The "N." number is decorative (from the --footnote-number CSS var on the
wrapper, never in the document model) and is rendered inline at the start of
the first content line via ::before. This keeps text and wrapped lines flush
to the left margin — no hanging indent — while the editable contentDOM stays
the FIRST DOM child (#146). */
.definitionContent {
flex: 1 1 auto;
min-width: 0;
}
/* The inner editable paragraph inherits `.ProseMirror p { margin: 0.5em 0 }`,
which pushes the first text line ~0.5em below the "N." marker (aligned to
flex-start), making the number float above the text. Drop the outer margins
so the marker and the first line share the same top edge — same approach
used for callouts in core.css. */
.definitionContent > :first-child::before {
content: var(--footnote-number, "?") ". ";
color: var(--mantine-color-dimmed);
font-variant-numeric: tabular-nums;
user-select: none;
}
/* The inner editable paragraph inherits `.ProseMirror p { margin: 0.5em 0 }`.
Drop the outer margins so the definition sits tight to the heading above and
the ::before number aligns with the top of the row — same approach used for
callouts in core.css. */
.definitionContent > :first-child {
margin-top: 0;
}
@@ -0,0 +1,140 @@
/**
* gitmost #401 — regression test for the connect-vs-unload race in
* @hocuspocus/server 3.4.4 (patched via patches/@hocuspocus__server@3.4.4.patch).
*
* The race (unpatched): when the last client disconnects, storeDocumentHooks'
* `finally` schedules an async `unloadDocument`. That unload runs its
* `beforeUnloadDocument` hooks asynchronously and, meanwhile, records an
* in-flight promise in `this.unloadingDocuments`. In the original 3.4.4
* `createDocument`, a NEW connection arriving in that window falls straight
* through to the `loadingDocuments`/`documents` checks — it never consults
* `unloadingDocuments`. So the new connection can start loading (or reuse) a
* document while the old instance is still being torn down; the re-check inside
* unload (`shouldUnloadDocument`, which sees 0 connections because async auth
* hooks have not registered the new connection yet) then deletes/destroys the
* doc out from under the freshly-connected client → orphaned Document → later
* redis-sync takes the "doc not loaded" path → sync never completes → the
* provider hangs until its ~25s timeout.
*
* The patch: `createDocument` first awaits any in-flight
* `unloadingDocuments.get(name)` before proceeding. Once that settles, the
* decision is deterministic — either the doc was fully unloaded (gone from
* `documents`, so a clean fresh load) or the unload aborted (healthy doc still
* in `documents`, reused). The new connection can never hand-shake onto an
* about-to-be-destroyed Document.
*
* These tests exercise the REAL patched `Hocuspocus.createDocument` (the class
* is directly constructible) by seeding `unloadingDocuments` with a controllable
* in-flight unload and observing that createDocument waits for it.
*/
import { Hocuspocus } from '@hocuspocus/server';
// A promise we can resolve on demand, to model an unload that is mid-flight.
function deferred<T = void>() {
let resolve!: (v: T) => void;
const promise = new Promise<T>((r) => {
resolve = r;
});
return { promise, resolve };
}
describe('gitmost #401 — hocuspocus createDocument awaits in-flight unload', () => {
it('does NOT start loading a new doc until the in-flight unload settles, then loads fresh', async () => {
const hp = new Hocuspocus();
const name = 'page.race';
// Observe loadDocument: on the unpatched code it is invoked synchronously
// within createDocument (before the unload settles); on the patched code it
// must be deferred until unloadingDocuments resolves.
const freshDoc = { name, __fresh: true } as any;
const loadSpy = jest
.spyOn(hp as any, 'loadDocument')
.mockResolvedValue(freshDoc);
// Model an unload in progress: an entry sits in unloadingDocuments and, when
// it completes, it removes the doc from `documents` (a real full unload).
const unload = deferred();
(hp as any).documents.set(name, { name, __dying: true });
(hp as any).unloadingDocuments.set(
name,
unload.promise.then(() => {
(hp as any).documents.delete(name);
}),
);
// Kick off a new connection's createDocument but do not await it yet.
const createPromise = (hp as any).createDocument(
name,
{},
'socket-1',
{ isAuthenticated: true, readOnly: false },
{},
);
// Let all currently-schedulable microtasks run. The patched createDocument is
// now parked on `await unloadingDocuments.get(name)`, so loadDocument must
// NOT have been called yet, and it must NOT have returned the dying doc.
await Promise.resolve();
await Promise.resolve();
expect(loadSpy).not.toHaveBeenCalled();
// The unload completes (doc removed from `documents`).
unload.resolve();
// createDocument now proceeds: sees no existing doc → fresh load.
const doc = await createPromise;
expect(loadSpy).toHaveBeenCalledTimes(1);
expect(doc).toBe(freshDoc);
// The freshly-loaded doc is the one registered — never the dying instance.
expect((hp as any).documents.get(name)).toBe(freshDoc);
});
it('reuses the live doc when the in-flight unload aborts (doc left in documents)', async () => {
const hp = new Hocuspocus();
const name = 'page.abort';
const loadSpy = jest.spyOn(hp as any, 'loadDocument');
// Model an unload that ABORTS (e.g. a new connection reappeared before the
// sync re-check): it settles WITHOUT deleting the doc from `documents`.
const unload = deferred();
const liveDoc = { name, __live: true } as any;
(hp as any).documents.set(name, liveDoc);
(hp as any).unloadingDocuments.set(name, unload.promise); // no-op unload
const createPromise = (hp as any).createDocument(
name,
{},
'socket-2',
{ isAuthenticated: true, readOnly: false },
{},
);
unload.resolve();
const doc = await createPromise;
// The still-present live doc is reused; no fresh load happened.
expect(doc).toBe(liveDoc);
expect(loadSpy).not.toHaveBeenCalled();
});
it('no in-flight unload → behaves normally (fresh load)', async () => {
const hp = new Hocuspocus();
const name = 'page.normal';
const freshDoc = { name } as any;
const loadSpy = jest
.spyOn(hp as any, 'loadDocument')
.mockResolvedValue(freshDoc);
const doc = await (hp as any).createDocument(
name,
{},
'socket-3',
{ isAuthenticated: true, readOnly: false },
{},
);
expect(loadSpy).toHaveBeenCalledTimes(1);
expect(doc).toBe(freshDoc);
});
});
@@ -0,0 +1,130 @@
/**
* gitmost #401 fix 2 — onLoadDocument applies the DB state directly into the
* hook's target document and returns undefined (instead of building a NEW Y.Doc
* and returning it, which made hocuspocus re-encode+apply the whole state a
* SECOND time on every cold load).
*
* These tests assert:
* - the hook mutates `data.document` in place so its content equals the DB doc,
* - onLoadDocument returns undefined (so hocuspocus keeps the mutated doc and
* does NOT run its own applyUpdate(encodeStateAsUpdate(...)) merge),
* - both the raw-ydoc branch and the json→ydoc conversion branch behave so.
*
* Returning undefined is the observable signal that the double-encode is gone
* (the old code returned a new Y.Doc, which made hocuspocus re-encode+apply the
* state a second time); we assert that contract rather than counting internal
* encode calls, which is brittle given the encodes inside toYdoc and the test's
* own `expected` fixtures.
*/
import * as Y from 'yjs';
import { Document } from '@hocuspocus/server';
import { TiptapTransformer } from '@hocuspocus/transformer';
import { PersistenceExtension } from './persistence.extension';
import { tiptapExtensions } from '../collaboration.util';
// A fresh hocuspocus Document (extends Y.Doc, adds isEmpty()) as hocuspocus
// hands to onLoadDocument on a cold load.
const freshDoc = () => new Document(`page.${PAGE_ID}`, {});
const PAGE_ID = '550e8400-e29b-41d4-a716-446655440000';
const doc = (text: string) => ({
type: 'doc',
content: [{ type: 'paragraph', content: [{ type: 'text', text }] }],
});
const jsonOf = (ydoc: Y.Doc) =>
TiptapTransformer.fromYdoc(ydoc, 'default');
describe('PersistenceExtension.onLoadDocument — #401 fix 2 (apply-into-hook-doc)', () => {
let ext: PersistenceExtension;
let pageRepo: { findById: jest.Mock };
beforeEach(() => {
pageRepo = { findById: jest.fn() };
ext = new PersistenceExtension(
pageRepo as any,
{} as any,
{} as any,
{} as any,
{} as any,
{} as any,
{} as any,
{} as any,
);
jest.spyOn(ext['logger'], 'debug').mockImplementation(() => undefined);
jest.spyOn(ext['logger'], 'warn').mockImplementation(() => undefined);
});
const load = (document: Document) =>
ext.onLoadDocument({ documentName: `page.${PAGE_ID}`, document } as any);
it('raw ydoc branch: mutates the hook doc to the DB state and returns undefined', async () => {
// Source doc representing the persisted ydoc state.
const source = TiptapTransformer.toYdoc(
doc('DB CONTENT'),
'default',
tiptapExtensions,
);
const dbState = Buffer.from(Y.encodeStateAsUpdate(source));
pageRepo.findById.mockResolvedValue({ id: PAGE_ID, ydoc: dbState });
// The hook target is a fresh empty doc (as hocuspocus supplies on cold load).
const target = freshDoc();
const result = await load(target);
// Return undefined so hocuspocus keeps `target` as-is (no second merge).
expect(result).toBeUndefined();
// The hook document now carries the DB content.
expect(jsonOf(target)).toEqual(jsonOf(source));
});
it('json→ydoc branch: converts page.content into the hook doc and returns undefined', async () => {
pageRepo.findById.mockResolvedValue({
id: PAGE_ID,
ydoc: null,
content: doc('JSON CONTENT'),
});
const target = freshDoc();
const result = await load(target);
// Returning undefined is what keeps hocuspocus from re-encoding+applying the
// state a second time (the old code returned the doc, forcing that extra
// encode). We assert the observable contract here — the return value and the
// resulting content — rather than counting internal encode calls, which is
// brittle: toYdoc and the `expected` build below both encode too.
expect(result).toBeUndefined();
// The converted content landed in the hook document.
const expected = TiptapTransformer.toYdoc(
doc('JSON CONTENT'),
'default',
tiptapExtensions,
);
expect(jsonOf(target)).toEqual(jsonOf(expected));
});
it('live doc already non-empty: early return, no DB read', async () => {
// A hocuspocus Document carrying live content (isEmpty('default') === false).
const target = freshDoc();
const live = TiptapTransformer.toYdoc(
doc('LIVE'),
'default',
tiptapExtensions,
);
Y.applyUpdate(target, Y.encodeStateAsUpdate(live));
const result = await load(target);
expect(result).toBeUndefined();
expect(pageRepo.findById).not.toHaveBeenCalled();
});
it('no persisted state: leaves the fresh empty doc untouched, returns undefined', async () => {
pageRepo.findById.mockResolvedValue({ id: PAGE_ID, ydoc: null, content: null });
const target = freshDoc();
const result = await load(target);
expect(result).toBeUndefined();
expect(target.isEmpty('default')).toBe(true);
});
});
@@ -171,15 +171,21 @@ export class PersistenceExtension implements Extension {
return;
}
// #401 fix 2 — apply the DB state DIRECTLY into the hook's target document
// (`document` === `data.document`) and return undefined. When onLoadDocument
// returns undefined, hocuspocus keeps the mutated hook document as-is; only
// when the hook RETURNS a Y.Doc does hocuspocus re-`applyUpdate(document,
// encodeStateAsUpdate(returned))` — a second full encode+apply of the whole
// (e.g. 315KB) state on every cold load. Mutating in place performs a single
// apply and avoids the throwaway `new Y.Doc()` allocation.
if (page.ydoc) {
this.logger.debug(`ydoc loaded from db: ${pageId}`);
const doc = new Y.Doc();
const dbState = new Uint8Array(page.ydoc);
Y.applyUpdate(doc, dbState);
Y.applyUpdate(document, dbState);
observeCollabLoad(dbState.length, (performance.now() - startedAt) / 1000);
return doc;
return;
}
// if no ydoc state in db convert json in page.content to Ydoc.
@@ -192,18 +198,23 @@ export class PersistenceExtension implements Extension {
tiptapExtensions,
);
// Reuse this single encode for the size label (do NOT add a second one).
// Encode the converted doc ONCE, reuse the bytes for both the size label
// and the single apply into the hook document (previously this encode's
// result was returned and hocuspocus re-encoded+applied it a second time).
const encoded = Y.encodeStateAsUpdate(ydoc);
Y.applyUpdate(document, encoded);
observeCollabLoad(
encoded.byteLength,
(performance.now() - startedAt) / 1000,
);
return ydoc;
return;
}
// No persisted state: the hook document is already a fresh empty Y.Doc, so
// leave it untouched and return undefined (no re-encode of an empty doc).
this.logger.debug(`creating fresh ydoc: ${pageId}`);
observeCollabLoad(0, (performance.now() - startedAt) / 1000);
return new Y.Doc();
return;
}
async onStoreDocument(data: onStoreDocumentPayload) {
@@ -17,10 +17,24 @@ import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common';
/** How long a finished entry is retained for late attach (replay + immediate end). */
export const RUN_STREAM_RETAIN_FINISHED_MS = 30_000;
/** Per-run replay buffer cap. Past this the buffer is dropped (attach -> 204). */
export const RUN_STREAM_MAX_BUFFER_BYTES = 4 * 1024 * 1024;
/**
* Per-run replay buffer cap. Past this the buffer is dropped (attach -> 204, and
* the client falls back to its restore + degraded-poll path, #430).
*
* Raised from 4MB to 32MB (#430): marathon autonomous runs (11-25 min observed)
* stream far more than 4MB of SSE frames, so a live disconnect mid-run would find
* an already-overflowed buffer and could only degrade-poll instead of re-attaching
* to the live tail. 32MB comfortably covers those runs while staying bounded.
*
* Memory cost: this is the WORST-CASE retained size PER ACTIVE run (the buffer is
* freed on finish + retention, or dropped immediately on overflow). With the small
* number of concurrent autonomous runs a single workspace realistically has, 32MB
* each is an acceptable ceiling; the overflow->204->degraded-poll fallback remains
* the backstop for anything larger, so correctness never depends on this bound.
*/
export const RUN_STREAM_MAX_BUFFER_BYTES = 32 * 1024 * 1024;
// 2x the replay cap: a just-written 4MB replay burst alone can never trip the
// 2x the replay cap: a just-written full-replay burst alone can never trip the
// per-subscriber cap (see controller); only a genuinely stalled socket can.
export const SUBSCRIBER_MAX_BUFFERED_BYTES = 2 * RUN_STREAM_MAX_BUFFER_BYTES;
@@ -2,6 +2,7 @@ import {
AiChatStreamRegistryService,
RUN_STREAM_MAX_BUFFER_BYTES,
RUN_STREAM_RETAIN_FINISHED_MS,
SUBSCRIBER_MAX_BUFFERED_BYTES,
RunStreamCallbacks,
} from './ai-chat-stream-registry.service';
@@ -210,9 +211,10 @@ describe('AiChatStreamRegistryService', () => {
const att = (await registry.attach(CHAT, false, undefined, c.cb))!;
att.start();
const oneMb = 'x'.repeat(1024 * 1024);
// 5 x 1MB = 5MB > 4MB cap; the 5th frame is the one that crosses.
for (let i = 0; i < 5; i++) src.push(oneMb + i);
// Cap-relative so it survives a buffer-cap change (#430): a quarter-cap frame
// means 5 frames comfortably exceed the replay cap; the last one crosses.
const chunk = 'x'.repeat(Math.floor(RUN_STREAM_MAX_BUFFER_BYTES / 4));
for (let i = 0; i < 5; i++) src.push(chunk + i);
await flush();
const entry = (registry as any).entries.get(CHAT);
@@ -220,7 +222,7 @@ describe('AiChatStreamRegistryService', () => {
expect(entry.bytes).toBeGreaterThan(RUN_STREAM_MAX_BUFFER_BYTES);
// The live subscriber received ALL 5 frames, including the crossing one.
expect(c.frames).toHaveLength(5);
expect(c.frames[4]).toBe(oneMb + 4);
expect(c.frames[4]).toBe(chunk + 4);
// A NEW attach after overflow gets null (replay buffer is gone).
const c2 = collector();
@@ -240,9 +242,11 @@ describe('AiChatStreamRegistryService', () => {
const attB = (await registry.attach(CHAT, false, undefined, b.cb))!;
attB.start();
const oneMb = 'x'.repeat(1024 * 1024);
// 9 x 1MB = 9MB > 8MB per-subscriber cap; A's pending overflows, B streams live.
for (let i = 0; i < 9; i++) src.push(oneMb + i);
// Cap-relative so it survives a buffer-cap change (#430): a quarter-of-the-
// per-subscriber-cap frame means 5 frames exceed A's paused-pending cap while
// B streams every frame live.
const chunk = 'x'.repeat(Math.floor(SUBSCRIBER_MAX_BUFFERED_BYTES / 4));
for (let i = 0; i < 5; i++) src.push(chunk + i);
await flush();
const entry = (registry as any).entries.get(CHAT);
@@ -250,7 +254,7 @@ describe('AiChatStreamRegistryService', () => {
expect(entry.subscribers.size).toBe(1);
expect(a.frames).toEqual([]); // paused + overflowed: nothing was delivered
// B received every frame live (delivery unaffected by A's overflow).
expect(b.frames).toHaveLength(9);
expect(b.frames).toHaveLength(5);
// A's start() (arriving late) degrades to an immediate end, not a partial replay.
attA.start();
@@ -148,6 +148,53 @@ describe('assistantParts', () => {
expect(toolPart).not.toHaveProperty('output');
});
it('replays the REAL error text for a THROWN tool (tool-error part)', () => {
const steps = [
{
text: '',
toolCalls: [
{ toolCallId: 'c7', toolName: 'editPageText', input: { id: 'p1' } },
],
// A thrown tool is a `tool-error` content part; toolResults holds only
// successes and stays empty for this call.
toolResults: [],
content: [
{
type: 'tool-error',
toolCallId: 'c7',
toolName: 'editPageText',
input: { id: 'p1' },
error: new Error('page is locked'),
},
],
},
];
const parts = assistantParts(steps, '') as AnyPart[];
const toolPart = parts.find((p) => p.type === 'tool-editPageText');
expect(toolPart).toBeDefined();
expect(toolPart!.state).toBe('output-error');
// The REAL error is replayed, NOT the 'Tool call did not complete.' placeholder.
expect(toolPart!.errorText).toBe('page is locked');
expect(toolPart).not.toHaveProperty('output');
});
it('keeps the placeholder ONLY for a call with neither result nor tool-error', () => {
const steps = [
{
text: '',
toolCalls: [
{ toolCallId: 'c8', toolName: 'insertNode', input: { node: {} } },
],
toolResults: [],
content: [], // aborted mid-step: no result AND no tool-error
},
];
const parts = assistantParts(steps, '') as AnyPart[];
const toolPart = parts.find((p) => p.type === 'tool-insertNode');
expect(toolPart!.state).toBe('output-error');
expect(toolPart!.errorText).toBe('Tool call did not complete.');
});
it('skips malformed tool-calls (missing toolName or toolCallId)', () => {
const steps = [
{
@@ -195,6 +242,45 @@ describe('serializeSteps', () => {
expect(trace[0]).toEqual({ toolName: 'getPage', input: { id: 'p1' } });
expect(trace[1]).toEqual({ toolName: 'getPage', output: { title: 'T' } });
});
it('records a THROWN tool failure (tool-error part) with its error message', () => {
const trace = serializeSteps([
{
toolCalls: [{ toolName: 'editPageText', input: { id: 'p1' } }],
toolResults: [],
content: [
{
type: 'tool-error',
toolName: 'editPageText',
error: new Error('page is locked'),
},
],
},
]) as Array<Record<string, unknown>>;
// The call element is followed by a paired error element (mirroring how a
// successful result is appended), so the failure survives in the trace.
expect(trace).toHaveLength(2);
expect(trace[0]).toEqual({ toolName: 'editPageText', input: { id: 'p1' } });
expect(trace[1]).toEqual({
toolName: 'editPageText',
error: 'page is locked',
});
});
it('truncates a very long tool-error message to the tool-output limit', () => {
const long = 'x'.repeat(5000);
const trace = serializeSteps([
{
toolCalls: [{ toolName: 'editPageText', input: {} }],
toolResults: [],
content: [{ type: 'tool-error', toolName: 'editPageText', error: long }],
},
]) as Array<Record<string, unknown>>;
const errorText = trace[1].error as string;
// Truncated (not the full 5000 chars) and carries the omission marker.
expect(errorText.length).toBeLessThan(long.length);
expect(errorText).toContain('chars omitted');
});
});
describe('rowToUiMessage', () => {
@@ -1637,6 +1637,17 @@ type StepLike = {
toolName?: string;
output?: unknown;
}>;
// ai@6.0.134: a tool that THREW surfaces as a `tool-error` content part
// ({ type:'tool-error', toolCallId, toolName, input, error }), NOT as a
// `toolResults` entry (which holds only successes). Read from here so failed
// calls are persisted with their real error instead of being dropped.
content?: ReadonlyArray<{
type?: string;
toolCallId?: string;
toolName?: string;
input?: unknown;
error?: unknown;
}>;
};
/**
@@ -1739,6 +1750,26 @@ function compactValue(value: unknown, depth: number): unknown {
return value;
}
/**
* Extract a bounded string message from a `tool-error` part's `error` field for
* persistence and history replay. The field may be an `Error`, a string, or an
* arbitrary object, so pull a message robustly. The result is passed through
* `compactValue` so a very long error honors the SAME truncation limits the file
* already applies to tool outputs (no new limit is introduced here).
*/
function normalizeToolError(error: unknown): string {
const message =
error instanceof Error
? error.message
: typeof error === 'string'
? error
: error != null &&
typeof (error as { message?: unknown }).message === 'string'
? (error as { message: string }).message
: String(error);
return compactValue(message, 0) as string;
}
/**
* Rebuild the FULL UIMessage `parts` for an assistant turn from the SDK steps,
* so multi-turn history replays prior tool-calls/results to the model (not just
@@ -1771,6 +1802,14 @@ export function assistantParts(
for (const r of step.toolResults ?? []) {
if (r.toolCallId) resultsById.set(r.toolCallId, r.output);
}
// Index this step's THROWN tool failures (ai@6 `tool-error` content parts)
// by tool call id, so a call that failed replays with its real error text.
const errorsById = new Map<string, unknown>();
for (const part of step.content ?? []) {
if (part.type === 'tool-error' && part.toolCallId) {
errorsById.set(part.toolCallId, part.error);
}
}
for (const call of step.toolCalls ?? []) {
if (!call.toolName || !call.toolCallId) continue;
const hasResult = resultsById.has(call.toolCallId);
@@ -1783,9 +1822,21 @@ export function assistantParts(
input: call.input,
output: compactToolOutput(resultsById.get(call.toolCallId)),
});
} else if (errorsById.has(call.toolCallId)) {
// The tool THREW: replay the REAL error so the model on the next turn
// knows WHY the call failed (and does not blindly repeat it). An
// output-error round-trips through convertToModelMessages as a balanced
// tool-call + tool-result, keeping the rebuilt history valid.
parts.push({
type: `tool-${call.toolName}`,
toolCallId: call.toolCallId,
state: 'output-error',
input: call.input,
errorText: normalizeToolError(errorsById.get(call.toolCallId)),
});
} else {
// No paired result (e.g. aborted mid-step). Persisting a bare
// tool-call (input-available) would replay as an unpaired call and
// No paired result AND no tool-error (e.g. aborted mid-step). Persisting
// a bare tool-call (input-available) would replay as an unpaired call and
// throw MissingToolResultsError on the next turn (convertToModelMessages
// emits no tool-result for it). Emit a SYNTHETIC paired result instead:
// an output-error round-trips through convertToModelMessages as a
@@ -2021,10 +2072,19 @@ export function serializeSteps(
steps: ReadonlyArray<{
toolCalls?: ReadonlyArray<{ toolName?: string; input?: unknown }>;
toolResults?: ReadonlyArray<{ toolName?: string; output?: unknown }>;
content?: ReadonlyArray<{
type?: string;
toolName?: string;
error?: unknown;
}>;
}>,
): unknown {
const calls: Array<{ toolName?: string; input?: unknown; output?: unknown }> =
[];
const calls: Array<{
toolName?: string;
input?: unknown;
output?: unknown;
error?: string;
}> = [];
for (const step of steps ?? []) {
for (const call of step.toolCalls ?? []) {
calls.push({ toolName: call.toolName, input: call.input });
@@ -2032,6 +2092,18 @@ export function serializeSteps(
for (const r of step.toolResults ?? []) {
calls.push({ toolName: r.toolName, output: compactToolOutput(r.output) });
}
// ai@6 surfaces a THROWN tool failure as a `tool-error` content part, NOT as
// a `toolResults` entry. Record it as its own paired element (mirroring how a
// successful result is appended) so the failure and its reason survive in the
// trace instead of leaving an orphaned call with no result.
for (const part of step.content ?? []) {
if (part.type === 'tool-error') {
calls.push({
toolName: part.toolName,
error: normalizeToolError(part.error),
});
}
}
}
return calls.length > 0 ? calls : null;
}
@@ -729,6 +729,35 @@ export class AiChatToolsService {
}),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// meta.hash in the result is the baseHash drawioUpdate requires.
drawioGet: sharedTool(
sharedToolSpecs.drawioGet,
async ({ pageId, node, format }) =>
await client.drawioGet(pageId, node, format ?? 'xml'),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// The flat schema fields are regrouped into the client's `where` object.
drawioCreate: sharedTool(
sharedToolSpecs.drawioCreate,
async ({ pageId, xml, position, anchorNodeId, anchorText, title }) =>
await client.drawioCreate(
pageId,
{ position, anchorNodeId, anchorText },
xml,
title,
),
),
// Schema + description live in @docmost/mcp's SHARED_TOOL_SPECS (#423).
// baseHash is the optimistic lock: mismatch => structured conflict error.
drawioUpdate: sharedTool(
sharedToolSpecs.drawioUpdate,
async ({ pageId, node, xml, baseHash }) =>
await client.drawioUpdate(pageId, node, xml, baseHash),
),
// Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294).
// The table reference parameter was unified to `table` (was `tableRef`).
tableInsertRow: sharedTool(
@@ -168,6 +168,32 @@ export interface DocmostClientLike {
url: string,
opts?: { align?: 'left' | 'center' | 'right'; alt?: string },
): Promise<Record<string, unknown>>;
// --- draw.io diagrams (#423, stage 1) ---
// Read a diagram as decoded mxGraph XML (default) or the raw .drawio.svg.
// meta.hash is the optimistic-lock key drawioUpdate expects as baseHash.
drawioGet(
pageId: string,
node: string,
format?: 'xml' | 'svg',
): Promise<Record<string, unknown>>;
// Lint mxGraph XML, build the .drawio.svg attachment and insert a drawio node.
drawioCreate(
pageId: string,
where: {
position: 'before' | 'after' | 'append';
anchorNodeId?: string;
anchorText?: string;
},
xml: string,
title?: string,
): Promise<Record<string, unknown>>;
// Optimistic-locked full replacement of a diagram (baseHash from drawioGet).
drawioUpdate(
pageId: string,
node: string,
xml: string,
baseHash: string,
): Promise<Record<string, unknown>>;
tableInsertRow(
pageId: string,
tableRef: string,
@@ -5,6 +5,16 @@ import {
} from '@nestjs/common';
import { CommentService } from './comment.service';
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
import { QueueJob } from '../../integrations/queue/constants';
// #399: the resolve/unresolve flip and the ephemeral anchor removal are enqueued
// as COMMENT_MARK_UPDATE jobs (off the HTTP path), NOT awaited against the collab
// gateway. applyCommentSuggestion (the document TEXT edit) is untouched — it
// still runs synchronously via the gateway.
const markJob = (generalQueue: any, action: string) =>
generalQueue.add.mock.calls.find(
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE && c[1]?.action === action,
);
/**
* Focused coverage for CommentService.applySuggestion (comment.service.ts).
@@ -59,6 +69,7 @@ describe('CommentService — applySuggestion', () => {
commentRepo,
wsService,
collaborationGateway,
generalQueue,
auditService,
};
}
@@ -86,9 +97,15 @@ describe('CommentService — applySuggestion', () => {
// --- no replies → ephemeral delete branch -------------------------------
it('applied=true, no replies → replaces text, hard-deletes, strips the anchor mark, audits APPLIED, outcome=deleted', async () => {
const { service, commentRepo, wsService, collaborationGateway, auditService } =
makeService({ applied: true, currentText: 'new text' });
it('applied=true, no replies → replaces text, hard-deletes, enqueues the anchor-mark removal, audits APPLIED, outcome=deleted', async () => {
const {
service,
commentRepo,
wsService,
collaborationGateway,
generalQueue,
auditService,
} = makeService({ applied: true, currentText: 'new text' });
const result = await service.applySuggestion(suggestionComment(), user());
@@ -105,12 +122,20 @@ describe('CommentService — applySuggestion', () => {
);
// Ephemeral: the redundant comment is hard-deleted (atomic-conditional) and
// its inline anchor mark removed via the deleteCommentMark collab event.
// its inline anchor mark removal is ENQUEUED (#399), no longer a sync gateway
// call. The gateway was only touched for the applyCommentSuggestion text edit.
expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1');
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
const del = markJob(generalQueue, 'delete');
expect(del).toBeDefined();
expect(del[1]).toMatchObject({
documentName: 'page.page-1',
commentId: 'c-1',
userId: 'user-1',
});
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith(
'deleteCommentMark',
'page.page-1',
expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }),
expect.anything(),
expect.anything(),
);
// No applied stamps are written for a row about to be deleted.
expect(appliedPatch(commentRepo)).toBeUndefined();
@@ -258,7 +283,7 @@ describe('CommentService — applySuggestion', () => {
// The suggested text is already applied to the document, but between the
// hasChildren read and the atomic delete a reply landed. The parent must NOT
// be hard-deleted (cascade would destroy the reply); resolve the thread.
const { service, commentRepo, wsService, collaborationGateway } =
const { service, commentRepo, wsService, generalQueue } =
makeService({ applied: true, currentText: 'new text' }, false, 0);
const result = await service.applySuggestion(suggestionComment(), user());
@@ -275,11 +300,8 @@ describe('CommentService — applySuggestion', () => {
.map((c: any[]) => c[0])
.find((p: any) => 'resolvedAt' in p);
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
expect.objectContaining({ commentId: 'c-1', resolved: true }),
);
// The resolve mark is enqueued (#399), not a sync gateway call.
expect(markJob(generalQueue, 'resolve')).toBeDefined();
expect(result.outcome).toBe('resolved');
});
@@ -313,11 +313,15 @@ describe('CommentService — behavior', () => {
});
const [patch] = commentRepo.updateComment.mock.calls[0];
expect(patch).toEqual({
// #399: resolve/unresolve now also stamps updatedAt (the async mark
// worker's race-guard reads it to order out-of-order events). The
// resolve-state fields are still cleared to null on unresolve.
expect(patch).toMatchObject({
resolvedAt: null,
resolvedById: null,
resolvedSource: null,
});
expect(patch.updatedAt).toBeInstanceOf(Date);
});
it("notifies the author when SOMEONE ELSE resolves their comment", async () => {
@@ -1,6 +1,15 @@
import { BadRequestException } from '@nestjs/common';
import { CommentService } from './comment.service';
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
import { QueueJob } from '../../integrations/queue/constants';
// #399: the inline comment-mark op (resolve flip / ephemeral-suggestion anchor
// removal) is now enqueued as a COMMENT_MARK_UPDATE job instead of being awaited
// against the collab gateway on the HTTP path. Find that job by action.
const markJob = (generalQueue: any, action: string) =>
generalQueue.add.mock.calls.find(
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE && c[1]?.action === action,
);
/**
* Coverage for CommentService.dismissSuggestion (#329). Dismiss ("Не применять")
@@ -44,7 +53,14 @@ describe('CommentService — dismissSuggestion', () => {
auditService,
);
return { service, commentRepo, wsService, collaborationGateway, auditService };
return {
service,
commentRepo,
wsService,
collaborationGateway,
generalQueue,
auditService,
};
}
const suggestionComment = (over?: Partial<any>): any => ({
@@ -62,25 +78,30 @@ describe('CommentService — dismissSuggestion', () => {
});
const user = (over?: Partial<any>): any => ({ id: 'user-1', ...over });
it('no replies → hard-deletes, strips the anchor mark, does NOT touch page text, audits DISMISSED, outcome=deleted', async () => {
const { service, commentRepo, wsService, collaborationGateway, auditService } =
makeService(false);
it('no replies → hard-deletes, enqueues the anchor-mark removal, does NOT touch page text, audits DISMISSED, outcome=deleted', async () => {
const {
service,
commentRepo,
wsService,
collaborationGateway,
generalQueue,
auditService,
} = makeService(false);
const result = await service.dismissSuggestion(suggestionComment(), user());
// Never applies the suggestion to the document.
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith(
'applyCommentSuggestion',
expect.anything(),
expect.anything(),
);
// Hard-delete (atomic-conditional) + strip mark.
// Never applies the suggestion to the document (no sync gateway call at all
// now — the mark op is off the HTTP path, #399).
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
// Hard-delete (atomic-conditional) + enqueue the anchor-mark strip.
expect(commentRepo.deleteCommentIfChildless).toHaveBeenCalledWith('c-1');
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'deleteCommentMark',
'page.page-1',
expect.objectContaining({ commentId: 'c-1', user: expect.any(Object) }),
);
const del = markJob(generalQueue, 'delete');
expect(del).toBeDefined();
expect(del[1]).toMatchObject({
documentName: 'page.page-1',
commentId: 'c-1',
userId: 'user-1',
});
expect(wsService.emitCommentEvent).toHaveBeenCalledWith(
'space-1',
'page-1',
@@ -96,20 +117,20 @@ describe('CommentService — dismissSuggestion', () => {
expect(result.outcome).toBe('deleted');
});
it('no replies → if the anchor-mark removal FAILS, the row is NOT deleted and the error propagates (#329: no orphan anchor)', async () => {
const { service, commentRepo, wsService, collaborationGateway } =
makeService(false);
// Mark removal is FATAL and runs BEFORE the irreversible row delete: a collab
// failure (e.g. COLLAB_DISABLE_REDIS "no live instance") must abort the whole
// operation, leaving row + mark consistent — never a deleted row with an
// orphan anchor left in the document reporting success.
collaborationGateway.handleYjsEvent = jest.fn(async () => {
throw new Error('requires a live collaboration instance');
it('no replies → if the anchor-mark ENQUEUE FAILS, the row is NOT deleted and the error propagates (#329/#399: no orphan anchor)', async () => {
const { service, commentRepo, wsService, generalQueue } = makeService(false);
// #399: the mark removal now runs async in a worker, but the ENQUEUE is
// awaited BEFORE the irreversible row delete — so the anchor-removal job is
// durably scheduled before the row can vanish. If even the enqueue fails
// (e.g. Redis down), the whole operation aborts, leaving row + mark
// consistent — never a deleted row with an orphan anchor reporting success.
generalQueue.add = jest.fn(async () => {
throw new Error('queue add failed: no redis');
});
await expect(
service.dismissSuggestion(suggestionComment(), user()),
).rejects.toThrow(/live collaboration/);
).rejects.toThrow(/queue add failed/);
expect(commentRepo.deleteCommentIfChildless).not.toHaveBeenCalled();
expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith(
@@ -120,23 +141,29 @@ describe('CommentService — dismissSuggestion', () => {
});
it('WITH replies → resolves (not delete), does NOT apply, audits DISMISSED, outcome=resolved', async () => {
const { service, commentRepo, wsService, collaborationGateway, auditService } =
makeService(true);
const {
service,
commentRepo,
collaborationGateway,
generalQueue,
auditService,
} = makeService(true);
const result = await service.dismissSuggestion(suggestionComment(), user());
// Resolved via resolveComment (resolve patch + resolve mark), NOT deleted.
// Resolved via resolveComment (resolve patch + enqueued resolve mark), NOT
// deleted.
const resolvePatch = commentRepo.updateComment.mock.calls
.map((c: any[]) => c[0])
.find((p: any) => 'resolvedAt' in p);
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
expect(resolvePatch.resolvedById).toBe('user-1');
expect(commentRepo.deleteComment).not.toHaveBeenCalled();
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
expect.objectContaining({ commentId: 'c-1', resolved: true }),
);
// No sync gateway call; the resolve mark is enqueued (#399).
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
const res = markJob(generalQueue, 'resolve');
expect(res).toBeDefined();
expect(res[1]).toMatchObject({ documentName: 'page.page-1', commentId: 'c-1' });
// No applied stamp — dismiss does not apply the edit.
const appliedPatch = commentRepo.updateComment.mock.calls
.map((c: any[]) => c[0])
@@ -156,8 +183,7 @@ describe('CommentService — dismissSuggestion', () => {
// but the atomic delete matches 0 rows because a reply landed in the window
// between that read and the delete. The parent must NOT be hard-deleted
// (a cascade would destroy the just-added reply); the thread is resolved.
const { service, commentRepo, wsService, collaborationGateway } =
makeService(false, 0);
const { service, commentRepo, wsService, generalQueue } = makeService(false, 0);
const result = await service.dismissSuggestion(suggestionComment(), user());
@@ -175,11 +201,9 @@ describe('CommentService — dismissSuggestion', () => {
.find((p: any) => 'resolvedAt' in p);
expect(resolvePatch.resolvedAt).toBeInstanceOf(Date);
expect(resolvePatch.resolvedById).toBe('user-1');
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
expect.objectContaining({ commentId: 'c-1', resolved: true }),
);
// A resolve mark job is enqueued (the anchor was already delete-marked; the
// resolve mirror is idempotent — #399).
expect(markJob(generalQueue, 'resolve')).toBeDefined();
expect(result.outcome).toBe('resolved');
});
@@ -0,0 +1,179 @@
import { Logger } from '@nestjs/common';
import { CommentService } from './comment.service';
import { QueueJob } from '../../integrations/queue/constants';
// Flush pending microtasks so a fire-and-forget `.catch(...)` runs before we assert.
const flushMicrotasks = () => new Promise((r) => setImmediate(r));
/**
* #399: the comment inline-mark update is moved OFF the HTTP critical path.
* resolveComment / unresolve / the ephemeral-suggestion delete must NO LONGER
* await CollaborationGateway.handleYjsEvent (which loaded the whole Y.Doc and
* ran the store pipeline synchronously, ~4.5s p95). Instead they enqueue an
* idempotent COMMENT_MARK_UPDATE job onto the GENERAL_QUEUE with the payload the
* worker replays.
*
* The service is constructed directly with jest mocks (the @InjectQueue tokens
* cannot be resolved by Test.createTestingModule see comment.service.spec.ts).
*/
describe('CommentService — async comment mark (#399)', () => {
function makeService() {
const commentRepo: any = {
findById: jest.fn(async (id: string) => ({
id,
content: {},
spaceId: 'space-1',
pageId: 'page-1',
})),
updateComment: jest.fn(async () => undefined),
hasChildren: jest.fn(async () => false),
deleteCommentIfChildless: jest.fn(async () => 1),
};
const pageRepo: any = {};
const wsService: any = { emitCommentEvent: jest.fn() };
// The gateway MUST NOT be touched on the HTTP path anymore.
const collaborationGateway: any = {
handleYjsEvent: jest.fn(async () => undefined),
};
const generalQueue: any = { add: jest.fn(() => Promise.resolve()) };
const notificationQueue: any = { add: jest.fn(async () => undefined) };
const auditService: any = { log: jest.fn() };
const service = new CommentService(
commentRepo,
pageRepo,
wsService,
collaborationGateway,
generalQueue,
notificationQueue,
auditService,
);
return {
service,
commentRepo,
collaborationGateway,
generalQueue,
auditService,
};
}
const comment = (over?: Partial<any>): any => ({
id: 'c-1',
creatorId: 'user-1',
pageId: 'page-1',
spaceId: 'space-1',
workspaceId: 'ws-1',
...over,
});
const user = (over?: Partial<any>): any => ({ id: 'user-1', ...over });
const markJob = (generalQueue: any) =>
generalQueue.add.mock.calls.find(
(c: any[]) => c[0] === QueueJob.COMMENT_MARK_UPDATE,
);
it('resolveComment does NOT call the gateway synchronously, and enqueues a resolve mark job', async () => {
const { service, collaborationGateway, generalQueue } = makeService();
await service.resolveComment(comment(), true, user());
// The whole point of #399: the Y.Doc mark op is off the HTTP path.
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
const job = markJob(generalQueue);
expect(job).toBeDefined();
expect(job[0]).toBe(QueueJob.COMMENT_MARK_UPDATE);
expect(job[1]).toMatchObject({
documentName: 'page.page-1',
commentId: 'c-1',
action: 'resolve',
userId: 'user-1',
});
expect(typeof job[1].ts).toBe('number');
// ts equals the resolvedAt stamp written to the row (shared timestamp).
const [patch] = (service as any).commentRepo.updateComment.mock.calls[0];
expect(job[1].ts).toBe((patch.resolvedAt as Date).getTime());
expect(job[1].ts).toBe((patch.updatedAt as Date).getTime());
});
it('unresolve enqueues an unresolve mark job (action mapped from resolved=false)', async () => {
const { service, collaborationGateway, generalQueue } = makeService();
await service.resolveComment(comment(), false, user());
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
const job = markJob(generalQueue);
expect(job[1]).toMatchObject({
documentName: 'page.page-1',
commentId: 'c-1',
action: 'unresolve',
userId: 'user-1',
});
});
it('dismissing a childless ephemeral suggestion enqueues a delete mark job (not a sync gateway call)', async () => {
const { service, collaborationGateway, generalQueue } = makeService();
await service.dismissSuggestion(
comment({ suggestedText: 'new text', selection: 'old', resolvedAt: null }),
user(),
);
// The anchor removal is queued, not awaited against the gateway.
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
const job = markJob(generalQueue);
expect(job).toBeDefined();
expect(job[1]).toMatchObject({
documentName: 'page.page-1',
commentId: 'c-1',
action: 'delete',
userId: 'user-1',
});
expect(typeof job[1].ts).toBe('number');
});
it('awaits the delete ENQUEUE before the irreversible row hard-delete (ordering preserved)', async () => {
const { service, generalQueue, commentRepo } = makeService();
const order: string[] = [];
generalQueue.add.mockImplementation(async (name: string) => {
order.push(`enqueue:${name}`);
});
commentRepo.deleteCommentIfChildless.mockImplementation(async () => {
order.push('delete-row');
return 1;
});
await service.dismissSuggestion(
comment({ suggestedText: 'new text', selection: 'old', resolvedAt: null }),
user(),
);
// The mark-removal job must be durably queued BEFORE the row disappears.
expect(order).toEqual([
`enqueue:${QueueJob.COMMENT_MARK_UPDATE}`,
'delete-row',
]);
});
it('resolve is fire-and-forget: a queue-add rejection does NOT fail the HTTP call (best-effort warn)', async () => {
const { service, generalQueue } = makeService();
// The queue is unavailable — the whole point of #399 is that this must NOT
// propagate out of resolveComment onto the HTTP request.
const queueErr = new Error('queue is down');
generalQueue.add.mockRejectedValue(queueErr);
const warnSpy = jest
.spyOn(Logger.prototype, 'warn')
.mockImplementation(() => undefined);
// Must resolve, never throw, even though the enqueue rejects.
await expect(service.resolveComment(comment(), true, user())).resolves.not.toThrow();
// The rejection is swallowed on a microtask AFTER the method returns; flush it.
await flushMicrotasks();
expect(warnSpy).toHaveBeenCalledWith(
expect.stringContaining('Failed to enqueue comment mark update for comment c-1'),
queueErr,
);
warnSpy.mockRestore();
});
});
+68 -24
View File
@@ -21,6 +21,7 @@ import { CursorPaginationResult } from '@docmost/db/pagination/cursor-pagination
import { QueueJob, QueueName } from '../../integrations/queue/constants';
import { extractUserMentionIdsFromJson } from '../../common/helpers/prosemirror/utils';
import {
ICommentMarkUpdateJob,
ICommentNotificationJob,
ICommentResolvedNotificationJob,
} from '../../integrations/queue/constants/queue.interface';
@@ -298,7 +299,11 @@ export class CommentService {
// source is cleared alongside resolvedAt/resolvedById.
provenance?: AuthProvenanceData,
): Promise<Comment> {
const resolvedAt = resolved ? new Date() : null;
// One shared timestamp: it stamps resolvedAt AND updatedAt on the row and is
// carried as the mark job's `ts`, so the worker's race-guard can order this
// event against the row's authoritative resolve-state mutation time (#399).
const now = new Date();
const resolvedAt = resolved ? now : null;
const resolvedById = resolved ? authUser.id : null;
const isAgent = provenance?.actor === 'agent';
// Set the agent marker only when resolving; on unresolve clear it back to
@@ -307,25 +312,33 @@ export class CommentService {
const resolvedSource = resolved && isAgent ? 'agent' : null;
await this.commentRepo.updateComment(
{ resolvedAt, resolvedById, resolvedSource },
// Bump updatedAt (not editedAt — that drives the "edited" badge) so the
// row records WHEN the resolve state last changed; the async mark worker
// compares its job ts against this to skip a superseded out-of-order event.
{ resolvedAt, resolvedById, resolvedSource, updatedAt: now },
comment.id,
);
// Reflect the resolved state on the inline comment mark in the
// collaborative document so all connected clients stay in sync.
// #399: mirror the resolved state onto the inline comment mark OFF the HTTP
// critical path. The DB row above is the source of truth (updated in ms); the
// mark is an eventual mirror for connected clients, and its failure was
// ALREADY swallowed (best-effort warn) — so instead of awaiting the whole
// Y.Doc load + immediate store pipeline (~4.5s p95), enqueue an idempotent,
// retryable COMMENT_MARK_UPDATE job. (Store-pipeline cost itself is #348's
// scope, not duplicated here.)
const documentName = `page.${comment.pageId}`;
try {
await this.collaborationGateway.handleYjsEvent(
'resolveCommentMark',
documentName,
{ commentId: comment.id, resolved, user: authUser },
);
} catch (error) {
void this.enqueueCommentMarkUpdate(
documentName,
comment.id,
resolved ? 'resolve' : 'unresolve',
now.getTime(),
authUser.id,
).catch((error) =>
this.logger.warn(
`Failed to update comment mark for comment ${comment.id}`,
`Failed to enqueue comment mark update for comment ${comment.id}`,
error,
);
}
),
);
// Notify the comment author when someone else resolves their comment.
if (resolved && comment.creatorId !== authUser.id) {
@@ -671,23 +684,54 @@ export class CommentService {
}
/**
* Remove the inline `comment` mark for a comment from the collaborative
* document. FATAL, NOT best-effort: unlike resolveComment (which keeps the row,
* so a failed mark update is recoverable), this is used before an irreversible
* hard-delete, so the mark removal MUST succeed or throw. Under
* COLLAB_DISABLE_REDIS the gateway invokes the deleteCommentMark handler
* directly (never a silent no-op) and a missing live instance surfaces as a
* thrown error, which we let propagate so the caller aborts before deleting.
* Schedule removal of the inline `comment` anchor mark from the collaborative
* document (ephemeral suggestion #329), OFF the HTTP critical path (#399).
*
* ORDERING PRESERVED: we `await` the ENQUEUE (a fast Redis add), not the mark
* op, and the caller only proceeds to the irreversible row hard-delete after
* this resolves. So the anchor-removal job is DURABLY queued before the row
* vanishes a queue-add failure throws here and aborts the delete (row + mark
* stay consistent), preserving the invariant the old FATAL sync call gave. The
* mark op itself now runs async in the worker: it is idempotent and retried
* (3 attempts), so a transient collab failure self-heals; only an exhausted-
* retries job leaves a DBmark divergence, now VISIBLE via BullMQ failed-job
* metrics (was a hard 5xx before). Delete carries no state guard the row is
* being removed, and stripping an absent mark is a no-op.
*/
private async deleteCommentMark(comment: Comment, user: User): Promise<void> {
const documentName = `page.${comment.pageId}`;
await this.collaborationGateway.handleYjsEvent(
'deleteCommentMark',
await this.enqueueCommentMarkUpdate(
documentName,
{ commentId: comment.id, user },
comment.id,
'delete',
Date.now(),
user.id,
);
}
/**
* Enqueue an idempotent COMMENT_MARK_UPDATE job (#399) the single path that
* mirrors a comment's inline-mark state into the collab Y.Doc off the HTTP
* response. The worker (GeneralQueueProcessor) runs the SAME handleYjsEvent
* the sync code used, so the mark op is byte-identical.
*/
private enqueueCommentMarkUpdate(
documentName: string,
commentId: string,
action: 'resolve' | 'unresolve' | 'delete',
ts: number,
userId: string,
): Promise<unknown> {
const jobData: ICommentMarkUpdateJob = {
documentName,
commentId,
action,
ts,
userId,
};
return this.generalQueue.add(QueueJob.COMMENT_MARK_UPDATE, jobData);
}
private async queueCommentNotification(
content: any,
oldMentionIds: string[],
@@ -61,6 +61,9 @@ export enum QueueJob {
COMMENT_NOTIFICATION = 'comment-notification',
COMMENT_RESOLVED_NOTIFICATION = 'comment-resolved-notification',
// #399: off-critical-path mirror of a comment's inline mark into the collab
// Y.Doc (resolve/unresolve flip, or ephemeral-suggestion anchor removal).
COMMENT_MARK_UPDATE = 'comment-mark-update',
PAGE_MENTION_NOTIFICATION = 'page-mention-notification',
PAGE_PERMISSION_GRANTED = 'page-permission-granted',
PAGE_UPDATE_DIGEST = 'page-update-digest',
@@ -63,6 +63,33 @@ export interface ICommentNotificationJob {
notifyWatchers: boolean;
}
/**
* GENERAL_QUEUE payload for the off-critical-path comment inline-mark mirror
* (#399). The comment DB row is the source of truth and is already updated
* synchronously (ms); this job flips/removes the inline `comment` mark in the
* collaborative Y.Doc for connected clients, OFF the HTTP response path, so
* `POST /api/comments/resolve` no longer waits the whole Y.Doc load + store
* pipeline (was ~4.5s p95). The mark op is idempotent, so BullMQ retries are
* safe.
*
* `action`:
* - 'resolve' / 'unresolve' flip the mark's `resolved` attribute (exactly
* what the synchronous resolveCommentMark path did);
* - 'delete' strip the anchor mark entirely (ephemeral suggestion #329).
* `ts` is the DB-mutation timestamp (ms). The worker's race-guard uses it (with
* the row's authoritative resolved state) to skip a resolve/unresolve event
* that a newer, opposite event has already superseded (out-of-order drain).
* `userId` supplies the connection-context user the store pipeline attributes
* the change to (persistence.extension reads context.user.id).
*/
export interface ICommentMarkUpdateJob {
documentName: string;
commentId: string;
action: 'resolve' | 'unresolve' | 'delete';
ts: number;
userId: string;
}
export interface ICommentResolvedNotificationJob {
commentId: string;
commentCreatorId: string;
@@ -0,0 +1,151 @@
import { Job } from 'bullmq';
import { GeneralQueueProcessor } from './general-queue.processor';
import { QueueJob } from '../constants';
import { ICommentMarkUpdateJob } from '../constants/queue.interface';
/**
* #399: the GENERAL_QUEUE worker replays the comment inline-mark op that used to
* run synchronously on the HTTP path. It must call the SAME gateway handler with
* the SAME semantics (resolve/unresolve flip the `resolved` attribute; delete
* strip the anchor), and its timestamp race-guard must skip an event a newer,
* opposite event already superseded.
*/
describe('GeneralQueueProcessor — COMMENT_MARK_UPDATE (#399)', () => {
function makeProc() {
const collaborationGateway: any = {
handleYjsEvent: jest.fn(async () => undefined),
};
const commentRepo: any = { findById: jest.fn() };
// #399: the processor resolves CollaborationGateway lazily via ModuleRef
// (strict:false) to avoid a DI cycle; the fake returns our gateway spy.
const moduleRef: any = { get: jest.fn(() => collaborationGateway) };
const proc = new GeneralQueueProcessor(
{} as any, // db
{} as any, // backlinkRepo
{} as any, // watcherRepo
commentRepo,
moduleRef,
);
return { proc, collaborationGateway, commentRepo };
}
const job = (data: ICommentMarkUpdateJob): Job =>
({ name: QueueJob.COMMENT_MARK_UPDATE, data }) as unknown as Job;
const base = {
documentName: 'page.page-1',
commentId: 'c-1',
userId: 'user-1',
};
it('resolve → resolveCommentMark with resolved:true and the same-shape args', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
const ts = 1000;
// Row reflects the resolve (source of truth), stamped at the same ts.
commentRepo.findById.mockResolvedValue({
id: 'c-1',
resolvedAt: new Date(ts),
updatedAt: new Date(ts),
});
await proc.process(job({ ...base, action: 'resolve', ts }));
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledTimes(1);
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
{ commentId: 'c-1', resolved: true, user: { id: 'user-1' } },
);
});
it('unresolve → resolveCommentMark with resolved:false', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
const ts = 2000;
commentRepo.findById.mockResolvedValue({
id: 'c-1',
resolvedAt: null,
updatedAt: new Date(ts),
});
await proc.process(job({ ...base, action: 'unresolve', ts }));
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
{ commentId: 'c-1', resolved: false, user: { id: 'user-1' } },
);
});
it('delete → deleteCommentMark (strip the anchor), no row lookup / no state guard', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
await proc.process(job({ ...base, action: 'delete', ts: 123 }));
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'deleteCommentMark',
'page.page-1',
{ commentId: 'c-1', user: { id: 'user-1' } },
);
// Delete carries no state guard — the row is (being) removed.
expect(commentRepo.findById).not.toHaveBeenCalled();
});
it('SKIPS a stale resolve superseded by a newer unresolve (row unresolved, job ts older)', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
// A later unresolve already set the row: resolvedAt null, updatedAt = 5000.
commentRepo.findById.mockResolvedValue({
id: 'c-1',
resolvedAt: null,
updatedAt: new Date(5000),
});
// Stale resolve job enqueued at ts=1000 (< 5000), intends resolved=true,
// but the row's authoritative state is unresolved → skip.
await proc.process(job({ ...base, action: 'resolve', ts: 1000 }));
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
});
it('SKIPS a stale unresolve superseded by a newer resolve', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
commentRepo.findById.mockResolvedValue({
id: 'c-1',
resolvedAt: new Date(5000),
updatedAt: new Date(5000),
});
await proc.process(job({ ...base, action: 'unresolve', ts: 1000 }));
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
});
it('applies when the row state agrees even if ts is older (idempotent, not a stale flip)', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
// Row is resolved and its updatedAt is newer than the job ts, but the state
// AGREES with the job → this is a harmless idempotent replay, not a stale
// opposite event, so it must still apply.
commentRepo.findById.mockResolvedValue({
id: 'c-1',
resolvedAt: new Date(9000),
updatedAt: new Date(9000),
});
await proc.process(job({ ...base, action: 'resolve', ts: 1000 }));
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
'resolveCommentMark',
'page.page-1',
{ commentId: 'c-1', resolved: true, user: { id: 'user-1' } },
);
});
it('skips (no throw) when the comment row has vanished', async () => {
const { proc, collaborationGateway, commentRepo } = makeProc();
commentRepo.findById.mockResolvedValue(undefined);
await expect(
proc.process(job({ ...base, action: 'resolve', ts: 1000 })),
).resolves.toBeUndefined();
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
});
});
@@ -4,6 +4,7 @@ import { Job } from 'bullmq';
import { QueueJob, QueueName } from '../constants';
import {
IAddPageWatchersJob,
ICommentMarkUpdateJob,
IPageBacklinkJob,
} from '../constants/queue.interface';
import { InjectKysely } from 'nestjs-kysely';
@@ -13,8 +14,11 @@ import {
WatcherRepo,
WatcherType,
} from '@docmost/db/repos/watcher/watcher.repo';
import { InsertableWatcher } from '@docmost/db/types/entity.types';
import { InsertableWatcher, User } from '@docmost/db/types/entity.types';
import { processBacklinks } from '../tasks/backlinks.task';
import { ModuleRef } from '@nestjs/core';
import { CollaborationGateway } from '../../../collaboration/collaboration.gateway';
import { CommentRepo } from '@docmost/db/repos/comment/comment.repo';
@Processor(QueueName.GENERAL_QUEUE)
export class GeneralQueueProcessor
@@ -22,14 +26,32 @@ export class GeneralQueueProcessor
implements OnModuleDestroy
{
private readonly logger = new Logger(GeneralQueueProcessor.name);
// #399: CollaborationGateway lives in CollaborationModule. We resolve it lazily
// via ModuleRef instead of importing that module into the @Global QueueModule —
// CollaborationModule's own HistoryProcessor injects this module's global
// GENERAL_QUEUE token, so a static import edge here would form a DI cycle. A
// lazy strict:false lookup (cached) sidesteps it; the gateway is a singleton in
// both the API-server and collab processes that run this worker.
private collaborationGateway?: CollaborationGateway;
constructor(
@InjectKysely() private readonly db: KyselyDB,
private readonly backlinkRepo: BacklinkRepo,
private readonly watcherRepo: WatcherRepo,
private readonly commentRepo: CommentRepo,
private readonly moduleRef: ModuleRef,
) {
super();
}
private getCollaborationGateway(): CollaborationGateway {
if (!this.collaborationGateway) {
this.collaborationGateway = this.moduleRef.get(CollaborationGateway, {
strict: false,
});
}
return this.collaborationGateway;
}
async process(job: Job): Promise<void> {
try {
switch (job.name) {
@@ -56,12 +78,87 @@ export class GeneralQueueProcessor
);
break;
}
case QueueJob.COMMENT_MARK_UPDATE: {
await this.processCommentMarkUpdate(
job.data as ICommentMarkUpdateJob,
);
break;
}
}
} catch (err) {
throw err;
}
}
/**
* #399: apply a comment's inline-mark mirror in the collab Y.Doc, off the HTTP
* critical path. Runs the SAME gateway path the synchronous comment.service
* code used (byte-identical mark op):
* - resolve / unresolve resolveCommentMark (flip the `resolved` attribute);
* - delete deleteCommentMark (strip the ephemeral-suggestion anchor #329).
* The op is idempotent, so a BullMQ retry is safe. Throwing propagates to
* WorkerHost the job is retried and, on exhaustion, surfaces in failed-job
* metrics (the divergence is now visible rather than a silently-swallowed warn).
*/
private async processCommentMarkUpdate(
data: ICommentMarkUpdateJob,
): Promise<void> {
const { documentName, commentId, action, ts, userId } = data;
// Minimal connection-context user: the store pipeline reads context.user.id
// to attribute the change (persistence.extension). The mark mutation itself
// does not depend on the user, so the op stays byte-identical. Deliberate
// trade-off: the store pipeline's transient `page.updated` broadcast carries
// only { id } here, so its live "who edited" badge loses name/avatarUrl for
// this async mark replay. lastUpdatedById is still set correctly; the diff is
// cosmetic and self-heals on the next real edit — worth it to stay off the
// HTTP path and avoid re-loading the users row.
const user = { id: userId } as User;
if (action === 'delete') {
await this.getCollaborationGateway().handleYjsEvent(
'deleteCommentMark',
documentName,
{ commentId, user },
);
return;
}
// resolve / unresolve. The comment row is written SYNCHRONOUSLY before this
// job is enqueued, so it is the source of truth for the final resolved state
// and its updatedAt records when that state last changed. Race-guard: if a
// newer, OPPOSITE event has already superseded this one (its ts is older than
// the row's last resolve-state mutation AND the row's current resolved state
// disagrees with what this job intends — e.g. an unresolve that drained ahead
// of this resolve), skip it rather than flip the mark to a stale state.
const comment = await this.commentRepo.findById(commentId);
if (!comment) {
// The comment vanished (e.g. hard-deleted) → nothing left to mirror.
return;
}
const wantResolved = action === 'resolve';
const rowResolved = comment.resolvedAt != null;
const rowMutatedAt = new Date(comment.updatedAt).getTime();
// `<=`, not `<`: on a sub-millisecond tie (two opposite toggles stamped in
// the same ms) skip the disagreeing job rather than let queue order decide.
// The consistent job (whose intent matches the row) short-circuits on the
// first condition, so a real update is never dropped; only a mark that both
// disagrees with the row AND is no newer than it is discarded.
if (rowResolved !== wantResolved && ts <= rowMutatedAt) {
this.logger.debug(
`Skipping stale comment mark '${action}' for ${commentId} ` +
`(job ts ${ts} < row ${rowMutatedAt}, row resolved=${rowResolved})`,
);
return;
}
await this.getCollaborationGateway().handleYjsEvent(
'resolveCommentMark',
documentName,
{ commentId, resolved: wantResolved, user },
);
}
@OnWorkerEvent('active')
onActive(job: Job) {
this.logger.debug(`Processing ${job.name} job`);
+77 -39
View File
@@ -13,12 +13,14 @@ Read the **Gotchas** section before you trust any error count.
- Agent chats live in Postgres, DB `docmost`, tables `ai_chat_*`.
- Each tool invocation is stored as **two** array elements (a `tool-call` part and
a `tool-result` part), so naive counting double-counts.
- **A tool that *throws* writes no result part at all.** Its error text is nowhere
in the DB — not in `tool_calls`, `content`, or `metadata`. It is shown live in
the UI only. So `isError` / `success=false` scans under-report by design.
- To find where agents fail you need **three** sources: (1) soft-failure markers in
`tool_calls`, (2) the orphan-gap proxy for thrown errors, (3) server logs / the
live UI for the actual error text.
- **A tool that *throws* writes no result part.** Since the #407 fix its error is
persisted as a dedicated `{toolName, error}` element in `tool_calls` (queryable +
replayed to the model). **Rows written before #407 still drop it** — the error is
nowhere in the DB and shows only in the live UI. So `isError` / `success=false`
scans under-report by design, and pre-#407 thrown errors are invisible.
- To find where agents fail: (1) soft-failure markers in `tool_calls`, (2) the new
`error` field for thrown errors (new rows) / the orphan-gap proxy (old rows),
(3) server logs / the live UI for full stack traces beyond the truncated message.
## Where the data lives
@@ -61,13 +63,17 @@ index 0: { "toolName": "getPage", "input": { "pageId": "…" } } ← tool-ca
index 1: { "toolName": "getPage", "output": { … } } ← tool-result (has output, NO input)
```
The **only** keys that ever appear on an element are `toolName`, `input`, `output`.
There is no `state`, no `errorText`, no `type`. Consequences:
The keys that appear on an element are `toolName`, `input`, `output`, and — for a
**thrown** failure on rows written after the #407 fix — `error` (the tool's error
message; see the "Hard failures" section below). There is no `state`, no `errorText`,
no `type`. On pre-#407 rows a thrown failure has NO paired result element at all
(silent orphan). Consequences:
1. **Real invocation count = elements that have `output`.** Counting every element
double-counts (you get ~2× and a spurious "~50% of every tool has no output").
2. **Pairing:** a successful call = a `tool-call` part followed by its `tool-result`
part. Both carry `toolName`, so you can group by tool on either.
1. **Real invocation count = elements that have `output` or `error`.** Counting every
element double-counts (you get ~2× and a spurious "~50% of every tool has no output").
2. **Pairing:** a call = a `tool-call` part followed by its result part. A success
carries `output`; a thrown failure (post-#407) carries `error` instead. Both carry
`toolName`, so you can group by tool on either.
## The two classes of failure (and which the DB can see)
@@ -85,25 +91,37 @@ These are visible in the `tool-result` `output`. The marker differs per tool:
Note `editPageText` returns `failed: []` on success — filtering on the *presence*
of the key gives false positives; filter on **non-empty**.
### 2. Hard failures — tool THREW → NOT PERSISTED (the trap)
### 2. Hard failures — tool THREW → NOW PERSISTED (since the #407 fix)
When a tool throws (the classic one is `patchNode` / `insertNode` / `tableUpdateCell`
`Failed to encode document to Yjs (fromJSON): Unknown node type: undefined`), the
runtime writes **no `tool-result` part**. The orphaned `tool-call` part stays, but
the error text is **nowhere in the DB**. It is streamed to the UI live and (until
rotation) to server logs — that is it.
runtime still writes **no `tool-result` part** — the failure is an ai@6 `tool-error`
content part instead. **Since the #407 fix, that error is persisted**: `serializeSteps`
appends a dedicated element `{toolName, error: "<message>"}` right after the failed
call, mirroring how a successful `{toolName, output}` element is appended. So a thrown
error now leaves a queryable `error` field carrying its (truncated) reason, and the
same real text is replayed to the model on the next turn (an `output-error` part with
the real `errorText`, no longer the `'Tool call did not complete.'` placeholder).
So any query like `count(*) FILTER (WHERE output.success = false)` will happily
return **0** for `patchNode` even when the chat is visibly full of red failures.
That is survivorship bias, not reliability.
**Cutover caveat — old rows keep the old blind shape.** Rows written **before** this
change have the two-part shape (`call` + `output` only) and simply **drop** thrown
errors, leaving a silent **orphan** (a `call` with no `output` *and* no `error`). Rows
written **after** the fix additionally carry the `error` element. So:
The only DB-side proxy for a thrown error is an **orphan**: a `tool-call` part with
no matching `tool-result`. Caveat: orphans also appear when a run is **aborted**
mid-flight (server restart), so a high-volume tool (`createComment`, `searchInPage`,
`Search_web_search`) shows orphans from aborts, not from real errors. Treat the
orphan gap as an *upper bound* on hard errors, and cross-check the tool: a gap on a
structural editor (`patchNode`, `insertNode`, `updatePageJson`, `transformPage`) is
almost certainly a thrown Yjs-encode error; a gap on `createComment` is mostly aborts.
- **New rows:** query the `error` field directly (see the hard-error query below) — no
orphan heuristic needed for thrown failures.
- **Old rows (pre-#407):** the only DB-side proxy is still an **orphan**: a `tool-call`
part with no matching `tool-result` *and* no `error`. Orphans also appear when a run
is **aborted** mid-flight (server restart), so a high-volume tool (`createComment`,
`searchInPage`, `Search_web_search`) shows orphans from aborts, not real errors on
old rows. Treat the orphan gap as an *upper bound*, and cross-check the tool: a gap on
a structural editor (`patchNode`, `insertNode`, `updatePageJson`, `transformPage`) is
almost certainly a thrown Yjs-encode error; a gap on `createComment` is mostly aborts.
A note on the aborted-call fallback: a call with **neither** a result **nor** a
`tool-error` (genuinely interrupted mid-step) still replays with the
`'Tool call did not complete.'` placeholder and persists as an orphan — that path is
unchanged, and is distinct from a real thrown error, which now carries `error`.
### 3. Run-level failures → `ai_chat_runs`
@@ -164,14 +182,28 @@ WHERE jsonb_typeof(o->'failed') = 'array'
GROUP BY 1 ORDER BY 2 DESC;
```
**Hard-error proxy — orphan gap per tool, WITH a spread column** (call parts minus
result parts, plus how many distinct chats the gap is spread across):
**Hard errors — persisted `error` field per tool (NEW rows, since #407)** — thrown
tool failures now carry their real reason, so query them directly:
```sql
SELECT elem->>'toolName' AS tool, count(*) AS thrown_errors,
min(elem->>'error') AS sample_error
FROM ai_chat_messages m, jsonb_array_elements(m.tool_calls) elem
WHERE jsonb_typeof(m.tool_calls) = 'array' AND elem ? 'error'
GROUP BY 1 ORDER BY 2 DESC;
```
**Hard-error proxy for OLD rows (pre-#407) — orphan gap per tool, WITH a spread column**
(call parts minus result parts, plus how many distinct chats the gap is spread across).
This covers rows written before thrown errors were persisted; on new rows a thrown
failure now has its own `error` element (use the query above) and an orphan means only
a genuinely aborted mid-step call:
```sql
WITH parts AS (
SELECT m.chat_id, elem->>'toolName' AS tool,
(elem ? 'input' AND NOT (elem ? 'output')) AS is_call,
(elem ? 'output') AS is_result
(elem ? 'output' OR elem ? 'error') AS is_result
FROM ai_chat_messages m, jsonb_array_elements(m.tool_calls) elem
WHERE jsonb_typeof(m.tool_calls) = 'array' AND m.role = 'assistant'
),
@@ -188,8 +220,12 @@ HAVING sum(gap) FILTER (WHERE gap > 0) > 0
ORDER BY missing_results DESC;
```
**`missing_results` mixes thrown errors AND aborted/interrupted runs — you cannot
split them from `output` alone** (a positional "what follows the orphan" heuristic
The `is_result` predicate counts an `error` element as a paired result too, so on new
rows a persisted thrown error no longer inflates the orphan gap; a remaining gap is an
aborted/interrupted call.
**On OLD rows, `missing_results` mixes thrown errors AND aborted/interrupted runs — you
cannot split them from `output` alone** (a positional "what follows the orphan" heuristic
breaks on parallel tool batches, which persist as `call,call,…,result,result`). Use
`chats_spread` to disambiguate:
@@ -244,18 +280,20 @@ docker compose -p gitmost logs -f --tail=100 # whole stack
```
Logging is `json-file`, `max-size=10m max-file=5` → ~50 MB retained, then rotated,
and **wiped on container recreate**. So thrown-tool error text is only reliably
caught **in real time** (or in the live chat UI, which renders the failed part with
its message). There is no durable, queryable store of hard tool errors today — if you
need one, that is a feature to add (persist `output-error` parts, or emit a
`tool_calls_total{tool,status}` metric to VictoriaMetrics).
and **wiped on container recreate**. Since the #407 fix, thrown-tool error text is
**persisted in the `error` field** of `tool_calls` (see the hard-error query above), so
you no longer depend on live logs for it. Logs/live UI remain useful for **pre-#407
rows** (whose thrown errors were dropped) and for full stack traces beyond the
truncated stored message. A per-tool `tool_calls_total{tool,status}` metric to
VictoriaMetrics is still a possible future add for aggregate dashboards.
## Gotchas checklist
- [ ] Counting every `tool_calls` element → **overcount**. Count elements with `output`.
- [ ] `isError` / `success=false` ≈ 0 does **not** mean "no errors" — thrown errors aren't persisted.
- [ ] Counting every `tool_calls` element → **overcount**. Count `output` elements; add `error` elements for thrown failures (new rows), but don't count both as invocations.
- [ ] `isError` / `success=false` ≈ 0 does **not** mean "no errors" — thrown errors are a separate `error` element (new rows) or dropped entirely (pre-#407 rows).
- [ ] Thrown errors persist only on rows written **after the #407 fix** — pre-#407 rows still drop them (orphan only). Mind the cutover when trending over time.
- [ ] `editPageText.failed` is `[]` on success — test for **non-empty**, not presence.
- [ ] Orphan gap mixes thrown errors **and** aborted runs — split by tool before concluding.
- [ ] Orphan gap on OLD rows mixes thrown errors **and** aborted runs — split by tool. On NEW rows a thrown error is its own `error` element, so a gap ≈ aborted call.
- [ ] `aborted` runs = server restarts, `failed` runs = provider overload — not agent mistakes.
- [ ] Never dump a raw `tool_calls` cell — it can be hundreds of KB.
- [ ] Logs are ephemeral (≤50 MB, wiped on recreate) — grab hard-error text live.
+2 -1
View File
@@ -97,7 +97,8 @@
"patchedDependencies": {
"scimmy@1.3.5": "patches/scimmy@1.3.5.patch",
"yjs@13.6.30": "patches/yjs@13.6.30.patch",
"ai@6.0.134": "patches/ai@6.0.134.patch"
"ai@6.0.134": "patches/ai@6.0.134.patch",
"@hocuspocus/server@3.4.4": "patches/@hocuspocus__server@3.4.4.patch"
},
"overrides": {
"prosemirror-changeset": "2.4.0",
+140 -188
View File
@@ -8,10 +8,6 @@ import {
filterComment,
filterSearchResult,
} from "./lib/filters.js";
import { HocuspocusProvider } from "@hocuspocus/provider";
import { TiptapTransformer } from "@hocuspocus/transformer";
import * as Y from "yjs";
import WebSocket from "ws";
import { convertProseMirrorToMarkdown } from "./lib/markdown-converter.js";
import {
collectInternalFileNodes,
@@ -24,11 +20,10 @@ import {
markdownToProseMirror,
markdownToProseMirrorCanonical,
mutatePageContent,
buildCollabWsUrl,
assertYjsEncodable,
applyDocToFragment,
MutationResult,
} from "./lib/collaboration.js";
import { acquireCollabSession } from "./lib/collab-session.js";
import { footnoteWarningsField } from "./lib/footnote-analyze.js";
import { buildPageTree } from "./lib/tree.js";
import {
@@ -40,6 +35,7 @@ import {
deleteNodeById,
assertUnambiguousMatch,
insertNodeRelative,
blockPlainText,
buildOutline,
getNodeByRef,
readTable,
@@ -68,10 +64,12 @@ import { getCollabToken, performLogin } from "./lib/auth-utils.js";
import { diffDocs, summarizeChange } from "./lib/diff.js";
import {
applyAnchorInDoc,
canAnchorInDoc,
countAnchorMatches,
getAnchoredText,
resolveAnchorSelection,
normalizeForMatch,
} from "./lib/comment-anchor.js";
import { closestBlockHint } from "./lib/text-normalize.js";
import {
blockText,
walk,
@@ -426,177 +424,32 @@ export class DocmostClient {
* change report. The report is computed AFTER the atomic read->write and
* never throws.
*/
private mutateLiveContentUnlocked(
private async mutateLiveContentUnlocked(
pageId: string,
collabToken: string,
transform: (liveDoc: any) => any | null,
): Promise<MutationResult> {
const CONNECT_TIMEOUT_MS = 25000;
const PERSIST_TIMEOUT_MS = 20000;
const ydoc = new Y.Doc();
const wsUrl = buildCollabWsUrl(this.apiUrl);
return new Promise<MutationResult>((resolve, reject) => {
let provider: HocuspocusProvider | undefined;
let applied = false; // onSynced may fire again on reconnect — apply once.
let settled = false;
let connectionLost = false;
let connectTimer: ReturnType<typeof setTimeout> | undefined;
let persistTimer: ReturnType<typeof setTimeout> | undefined;
let unsyncedHandler: ((data: { number: number }) => void) | undefined;
// The verifiable result resolved on every success/abort path. Set on abort
// (no-op report) and after a real write (computed change report).
let mutationResult: MutationResult;
const cleanup = () => {
if (connectTimer) clearTimeout(connectTimer);
if (persistTimer) clearTimeout(persistTimer);
if (provider) {
if (unsyncedHandler) {
try {
provider.off("unsyncedChanges", unsyncedHandler);
} catch (err) {}
}
try {
provider.destroy();
} catch (err) {}
}
};
const finish = (err: Error | null, value?: MutationResult) => {
if (settled) return;
settled = true;
cleanup();
if (err) reject(err);
else resolve(value as MutationResult);
};
connectTimer = setTimeout(() => {
// Only the actual 25s collab connect timeout fires here — the agent's
// collab connection to the server never became ready. This is the
// connect-vs-unload signal; the other finish() paths must NOT emit it.
this.onMetricFn?.("collab_connect_timeouts_total", 1);
finish(new Error("Connection timeout to collaboration server"));
}, CONNECT_TIMEOUT_MS);
const waitForPersistence = () => {
if (settled) return;
if (!provider) {
finish(new Error("collab provider gone before persistence"));
return;
}
if (provider.unsyncedChanges === 0) {
finish(null, mutationResult);
return;
}
persistTimer = setTimeout(() => {
finish(
new Error(
"Timeout waiting for collaboration server to persist the update",
),
);
}, PERSIST_TIMEOUT_MS);
unsyncedHandler = (data: { number: number }) => {
if (data.number === 0 && !connectionLost) {
finish(null, mutationResult);
}
};
provider.on("unsyncedChanges", unsyncedHandler);
};
provider = new HocuspocusProvider({
url: wsUrl,
name: `page.${pageId}`,
document: ydoc,
token: collabToken,
// @ts-ignore - Required for Node.js environment
WebSocketPolyfill: WebSocket,
onDisconnect: () => {
connectionLost = true;
finish(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
);
},
onClose: () => {
connectionLost = true;
finish(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
);
},
onSynced: () => {
if (applied || settled) return;
applied = true;
// CRITICAL: keep everything between reading and writing the live doc
// synchronous (no await) so no remote update can interleave.
let newDoc: any;
let beforeDoc: any;
try {
let liveDoc = TiptapTransformer.fromYdoc(ydoc, "default");
if (
!liveDoc ||
typeof liveDoc !== "object" ||
!Array.isArray(liveDoc.content)
) {
liveDoc = { type: "doc", content: [] };
}
// Snapshot the before-doc for the change report (safe deep clone).
beforeDoc = JSON.parse(JSON.stringify(liveDoc));
newDoc = transform(liveDoc);
if (newDoc == null) {
// Transform aborted — write nothing, return the live doc with a
// no-op change report.
mutationResult = {
doc: liveDoc,
verify: {
changed: false,
textInserted: 0,
textDeleted: 0,
blocksChanged: 0,
marks: {},
summary: "no changes (transform aborted)",
},
};
finish(null, mutationResult);
return;
}
// Structural diff into the live fragment (issue #152), mirroring
// the main write path: preserves the Yjs ids of unchanged nodes so
// an open editor's cursor is not yanked to the end of the document.
// The previous destructive rewrite (delete-all + applyUpdate of a
// fresh Y.Doc) discarded every node id, so replaceImage — the only
// caller of this method — still reproduced the #152 cursor jump
// (#164). applyDocToFragment runs its own atomic `transact`.
applyDocToFragment(ydoc, newDoc);
} catch (e) {
finish(e instanceof Error ? e : new Error(String(e)));
return;
}
// Compute the verifiable change report AFTER the transact write: it
// only needs the JSON before/after, so it cannot affect the atomic
// read->write window, and summarizeChange never throws.
mutationResult = {
doc: newDoc,
verify: summarizeChange(beforeDoc, newDoc),
};
waitForPersistence();
},
onAuthenticationFailed: () => {
finish(
new Error("Authentication failed for collaboration connection"),
);
},
});
// Reuse a live CollabSession for the page (issue #400) instead of opening a
// fresh provider per op. acquireCollabSession does NOT take the per-page
// lock — the caller (replaceImage) already holds ONE withPageLock across its
// scan -> upload -> write sequence, and the mutex is not reentrant, so
// taking it here would deadlock. The synchronous read->write section and the
// unsyncedChanges/connectionLost ack logic live in CollabSession.mutate,
// preserved verbatim from the old inline machine (incl. the #152 structural
// diff that keeps a live editor's cursor anchored).
const session = await acquireCollabSession(pageId, collabToken, this.apiUrl, {
// Only the actual 25s collab connect timeout emits this — the connect-vs-
// unload signal; the other failure paths must NOT emit it.
onConnectTimeout: () =>
this.onMetricFn?.("collab_connect_timeouts_total", 1),
});
try {
return await session.mutate(transform);
} catch (e) {
// Drop the session on any failure so the next call reconnects fresh.
session.destroy("mutate failed");
throw e;
}
}
/**
@@ -2483,6 +2336,64 @@ export class DocmostClient {
};
}
/** Plain text of each TOP-LEVEL block of `doc`, for anchor-failure hints. */
private topLevelBlockTexts(doc: any): string[] {
const content = doc && Array.isArray(doc.content) ? doc.content : [];
return content
.map((b: any) => blockPlainText(b))
.filter((t: string) => t.length > 0);
}
/**
* True when per-block anchoring failed but the (normalized) selection DOES
* appear in the blocks' joined plain text i.e. it straddles a block
* boundary. Blocks are joined with a newline (collapsed to one space by
* normalizeForMatch) so a selection whose parts are separated by a paragraph
* break still matches. Callers only reach here after single-block anchoring
* (incl. the markdown-strip fallback) has already failed.
*/
private selectionSpansMultipleBlocks(
blockTexts: string[],
selection: string,
): boolean {
const normSel = normalizeForMatch(selection).norm.trim();
if (normSel.length === 0) return false;
const joined = normalizeForMatch(blockTexts.join("\n")).norm;
return joined.indexOf(normSel) !== -1;
}
/**
* Build the actionable error for a create_comment anchor MISS, porting
* edit_page_text's self-correction affordances: an explicit "spans multiple
* blocks" message when the selection straddles a block boundary, otherwise a
* "closest block text" hint quoting the block that holds the selection's
* longest token. `live` switches the wording between the pre-check (reading the
* persisted page) and the post-create live-anchor failure (which rolls back).
*/
private anchorNotFoundError(
doc: any,
selection: string,
live: boolean,
): Error {
const blockTexts = this.topLevelBlockTexts(doc);
const rolled = live ? " The comment was rolled back." : "";
if (this.selectionSpansMultipleBlocks(blockTexts, selection)) {
return new Error(
"create_comment: the selection spans multiple blocks; anchor on a " +
"contiguous fragment within a SINGLE paragraph/block (<=250 chars)." +
rolled,
);
}
const where = live ? "in the live document" : "in the page";
return new Error(
`create_comment: could not find the selection text ${where} to anchor ` +
"the comment. Provide the EXACT contiguous text from a single " +
"paragraph/block (<=250 chars)." +
closestBlockHint(blockTexts, selection) +
rolled,
);
}
/**
* Create an inline comment anchored to its `selection` text, or a reply.
*
@@ -2544,6 +2455,10 @@ export class DocmostClient {
// Captured in the pre-check below (which already reads the page) and used as
// payload.selection. Ordinary comments keep sending the raw agent selection.
let anchoredSelection: string | null = null;
// Set when the anchor matched only after stripping markdown from the
// selection (the strip fallback); surfaced as a soft warning like
// edit_page_text does, so a stale-markdown selection is flagged.
let anchorNormalized = false;
// For a top-level comment, fail BEFORE creating anything when the selection
// is not present in the persisted document — this avoids leaving an orphan
@@ -2559,10 +2474,7 @@ export class DocmostClient {
// rejected BEFORE creating the comment.
const matches = countAnchorMatches(page.content, selection);
if (matches === 0) {
throw new Error(
"create_comment: could not find the selection text in the page to anchor the comment. " +
"Provide the EXACT contiguous text from a single paragraph/block (<=250 chars).",
);
throw this.anchorNotFoundError(page.content, selection, false);
}
if (matches >= 2) {
throw new Error(
@@ -2576,18 +2488,27 @@ export class DocmostClient {
// null despite countAnchorMatches===1 (shouldn't happen), fall back to
// the raw agent selection below rather than crash.
anchoredSelection = getAnchoredText(page.content, selection);
} else if (!canAnchorInDoc(page.content, selection)) {
throw new Error(
"create_comment: could not find the selection text in the page to anchor the comment. " +
"Provide the EXACT contiguous text from a single paragraph/block (<=250 chars).",
);
anchorNormalized = resolveAnchorSelection(
page.content,
selection,
).normalized;
} else {
const resolved = resolveAnchorSelection(page.content, selection);
if (!resolved.found) {
throw this.anchorNotFoundError(page.content, selection, false);
}
anchorNormalized = resolved.normalized;
}
} catch (e) {
// Rethrow our own "not found"/"ambiguous" errors; swallow read/network
// errors so the live anchor step can still try (and enforce) anchoring.
// Rethrow our own "not found"/"ambiguous"/"spans multiple blocks" errors;
// swallow read/network errors so the live anchor step can still try (and
// enforce) anchoring.
if (
e instanceof Error &&
(e.message.startsWith("create_comment: could not find the selection") ||
e.message.startsWith(
"create_comment: the selection spans multiple blocks",
) ||
e.message.startsWith(
"create_comment: the suggestion's selection is ambiguous",
))
@@ -2659,6 +2580,10 @@ export class DocmostClient {
// Set inside the transform when a suggestion's live anchor is ambiguous
// (>=2 occurrences), so the rollback path can surface the right error.
let ambiguousInLiveDoc = false;
// Captured inside the transform on a not-found abort, so the rollback path
// can surface the closest-block / spans-multiple-blocks hint built from the
// LIVE document (the pre-check page is not in scope there).
let liveNotFoundError: Error | null = null;
try {
const collabToken = await this.getCollabTokenWithReauth();
// Open the collab doc by the canonical UUID, never the slugId (#260). The
@@ -2686,6 +2611,13 @@ export class DocmostClient {
const liveCount = countAnchorMatches(doc, selection as string);
if (liveCount !== 1) {
ambiguousInLiveDoc = liveCount >= 2;
if (liveCount === 0) {
liveNotFoundError = this.anchorNotFoundError(
doc,
selection as string,
true,
);
}
return null;
}
}
@@ -2695,6 +2627,11 @@ export class DocmostClient {
}
// Selection text not found in the LIVE document: abort the write. The
// rollback + throw below turns this into a hard error.
liveNotFoundError = this.anchorNotFoundError(
doc,
selection as string,
true,
);
return null;
},
);
@@ -2711,13 +2648,28 @@ export class DocmostClient {
// suggestion, was ambiguous) in the live document. Roll back the comment
// and surface a hard error.
await this.safeDeleteComment(newCommentId);
throw new Error(
ambiguousInLiveDoc
? "create_comment: the suggestion's selection is ambiguous in the live document (multiple occurrences); the comment was rolled back. Expand the selection with surrounding context so it is unique."
: "create_comment: failed to anchor the comment (selection not found in the live document); the comment was rolled back",
if (ambiguousInLiveDoc) {
throw new Error(
"create_comment: the suggestion's selection is ambiguous in the live document (multiple occurrences); the comment was rolled back. Expand the selection with surrounding context so it is unique.",
);
}
throw (
liveNotFoundError ??
new Error(
"create_comment: failed to anchor the comment (selection not found in the live document); the comment was rolled back",
)
);
}
// Soft warning (like edit_page_text): the selection only matched after
// stripping markdown, so the caller likely quoted a styled fragment.
if (anchorNormalized) {
result.warning =
"The selection matched only after stripping markdown syntax; the comment " +
"was anchored on the document's plain text. Copy the selection verbatim " +
"from get_page / search_in_page output to avoid this.";
}
result.anchored = true;
return result;
}
+5
View File
@@ -13,6 +13,11 @@ import { SHARED_TOOL_SPECS, SharedToolSpec } from "./tool-specs.js";
export { DocmostClient } from "./client.js";
export type { DocmostMcpConfig } from "./client.js";
// Teardown for the live per-page CollabSession cache (issue #400). An embedding
// HTTP host (the gitmost NestJS server) should call this from its own shutdown
// hook so no cached collab provider outlives the process.
export { destroyAllSessions } from "./lib/collab-session.js";
// Re-export the zod-agnostic shared tool-spec registry so the in-app AI-SDK
// service can read it off the loaded module (it cannot import the ESM package's
// internals directly; it goes through loadDocmostMcp()).
+668
View File
@@ -0,0 +1,668 @@
import { HocuspocusProvider } from "@hocuspocus/provider";
import { TiptapTransformer } from "@hocuspocus/transformer";
import * as Y from "yjs";
import WebSocket from "ws";
import {
buildCollabWsUrl,
applyDocToFragment,
MutationResult,
} from "./collaboration.js";
import { summarizeChange } from "./diff.js";
/**
* Live per-page collaboration session cache (issue #400).
*
* The one-shot write path (collaboration.mutatePageContent /
* client.mutateLiveContentUnlocked) used to open a NEW HocuspocusProvider, run
* the full connect -> auth -> onLoadDocument -> initial-sync handshake, apply a
* single edit, wait for persistence, and then `provider.destroy()` for EVERY
* content mutation. Disconnecting after every edit means that once the pause
* between calls exceeds the server's write debounce, the server does a full
* store -> unload -> reload per cell, causing 25s connect timeouts and
* event-loop lag under a burst of edits on one page.
*
* This module keeps ONE live provider + ydoc per (wsUrl, pageId, token) alive
* across a SERIES of edits. While the provider stays connected the server never
* enters store -> unload -> reload, its debounce coalesces N writes into 1-2
* stores, and the repeated auth/load/initial-sync disappears.
*
* The synchronous read -> transform -> write section and the per-edit
* persistence-ack logic are preserved VERBATIM from the one-shot machine the
* only change is that they run on a persistent provider instead of a throwaway
* one. See CollabSession.mutate.
*/
/** Time we wait for the initial handshake/sync before giving up. */
const CONNECT_TIMEOUT_MS = 25000;
/** Time we wait for the server to acknowledge our write before giving up. */
const PERSIST_TIMEOUT_MS = 20000;
/**
* Tunables, read fresh from the environment on every acquire so tests (and a
* live rollback) can change them without reloading the module. Mirrors how
* http.ts parses MCP_SESSION_IDLE_MS.
* - MCP_COLLAB_SESSION_IDLE_MS: idle TTL, reset after every op. Default 60s.
* 0 (or negative) DISABLES the cache every op opens its own provider and
* destroys it after the op, i.e. the exact legacy per-op-provider behavior
* (the rollback path).
* - MCP_COLLAB_SESSION_MAX_AGE_MS: hard lifetime checked at acquire; bounds
* the permission-staleness window. Default 10 min.
* - MCP_COLLAB_SESSION_MAX_ENTRIES: registry cap; the least-recently-used
* session is destroy-evicted when the cap is reached. Default 32.
*/
interface SessionConfig {
idleMs: number;
maxAgeMs: number;
maxEntries: number;
}
function parseEnvInt(value: string | undefined, fallback: number): number {
const parsed = parseInt(value ?? "", 10);
return Number.isFinite(parsed) ? parsed : fallback;
}
function readConfig(): SessionConfig {
// idleMs: allow 0 (disable). A malformed value falls back to the default.
const idleRaw = parseInt(process.env.MCP_COLLAB_SESSION_IDLE_MS ?? "", 10);
const idleMs = Number.isFinite(idleRaw) ? Math.max(0, idleRaw) : 60 * 1000;
const maxAgeMs = Math.max(
0,
parseEnvInt(process.env.MCP_COLLAB_SESSION_MAX_AGE_MS, 10 * 60 * 1000),
);
const maxEntriesRaw = parseEnvInt(
process.env.MCP_COLLAB_SESSION_MAX_ENTRIES,
32,
);
const maxEntries = maxEntriesRaw > 0 ? maxEntriesRaw : 32;
return { idleMs, maxAgeMs, maxEntries };
}
/**
* The subset of HocuspocusProvider this module depends on, so the provider can
* be replaced with a fake in unit tests (there is no server in the test env).
*/
export interface CollabProviderLike {
synced: boolean;
unsyncedChanges: number;
destroy(): void;
on(event: "unsyncedChanges", handler: (data: { number: number }) => void): void;
off(event: "unsyncedChanges", handler: (data: { number: number }) => void): void;
}
/** The configuration object passed to the provider factory. */
export interface CollabProviderConfig {
url: string;
name: string;
document: Y.Doc;
token: string;
WebSocketPolyfill: unknown;
onConnect: () => void;
onSynced: () => void;
onDisconnect: () => void;
onClose: () => void;
onAuthenticationFailed: () => void;
}
export type CollabProviderFactory = (
config: CollabProviderConfig,
) => CollabProviderLike;
const defaultProviderFactory: CollabProviderFactory = (config) =>
// @ts-ignore - WebSocketPolyfill is required for the Node.js environment.
new HocuspocusProvider(config) as unknown as CollabProviderLike;
let providerFactory: CollabProviderFactory = defaultProviderFactory;
/**
* TEST SEAM: swap the provider factory (pass null to restore the real one).
* Not part of the public API used only by the unit tests, which cannot reach
* a real collaboration server.
*/
export function __setCollabProviderFactory(
factory: CollabProviderFactory | null,
): void {
providerFactory = factory ?? defaultProviderFactory;
}
/** Optional per-acquire hooks (metrics), passed through from the call site. */
export interface AcquireOptions {
/** Invoked when the initial connect handshake times out (CONNECT_TIMEOUT_MS). */
onConnectTimeout?: () => void;
}
type SessionState = "connecting" | "ready" | "dead";
/**
* One live provider + ydoc for a single (wsUrl, pageId, token) triple.
*
* Lifecycle: connecting -> ready -> dead. A session becomes `dead` on the first
* disconnect/close/auth-failure at ANY time, on an idle/eviction/max-age
* teardown, or on an explicit destroy(); death is terminal and removes the
* session from the registry so the next acquire opens a fresh one. We never use
* the provider's auto-reconnect destroying on the first disconnect closes the
* "reconnect drove unsyncedChanges to 0 without retransmitting our write" class
* of false success.
*/
export class CollabSession {
readonly key: string;
readonly pageId: string;
readonly wsUrl: string;
readonly token: string;
readonly createdAt: number;
state: SessionState = "connecting";
/**
* Set true on disconnect/close/auth-failure so a reconnect-driven
* unsyncedChanges->0 cannot be mistaken for a successful persist of our
* write (preserved verbatim from the one-shot machine).
*/
connectionLost = false;
provider: CollabProviderLike | undefined;
private readonly ydoc: Y.Doc;
private readonly cfg: SessionConfig;
/**
* Ephemeral sessions (cache disabled, MCP_COLLAB_SESSION_IDLE_MS<=0) are never
* registered and self-destroy after their single op the legacy
* provider-per-op behavior.
*/
private readonly ephemeral: boolean;
private readonly opts: AcquireOptions | undefined;
private dead = false;
private connectTimer: ReturnType<typeof setTimeout> | undefined;
private idleTimer: ReturnType<typeof setTimeout> | undefined;
private openPromise: Promise<void> | undefined;
private openResolve: (() => void) | undefined;
private openReject: ((err: Error) => void) | undefined;
private openSettled = false;
/**
* The rejector of the CURRENT in-flight mutate, if any. A disconnect/close/
* auth-failure or timeout at ANY time rejects the in-flight op through this
* with the SAME error text the one-shot machine emitted.
*/
private inflightReject: ((err: Error) => void) | undefined;
constructor(
key: string,
pageId: string,
wsUrl: string,
token: string,
cfg: SessionConfig,
ephemeral: boolean,
opts: AcquireOptions | undefined,
) {
this.key = key;
this.pageId = pageId;
this.wsUrl = wsUrl;
this.token = token;
this.cfg = cfg;
this.ephemeral = ephemeral;
this.opts = opts;
this.createdAt = Date.now();
this.ydoc = new Y.Doc();
}
/**
* A cached session may be reused only when it is fully ready, still synced,
* has not lost its connection, and has not exceeded its max age (invariant 5
* "validate on reuse" + the max-age acquire check).
*/
isReusable(): boolean {
return (
!this.dead &&
this.state === "ready" &&
!this.connectionLost &&
!!this.provider &&
this.provider.synced === true &&
Date.now() - this.createdAt < this.cfg.maxAgeMs
);
}
/**
* Connect and wait for the initial sync (onSynced) within CONNECT_TIMEOUT_MS.
* Idempotent: repeated calls return the same in-flight/settled promise.
*/
open(): Promise<void> {
if (this.openPromise) return this.openPromise;
this.openPromise = new Promise<void>((resolve, reject) => {
this.openResolve = resolve;
this.openReject = reject;
this.connectTimer = setTimeout(() => {
// The 25s connect timeout: the collab connection never became ready.
this.opts?.onConnectTimeout?.();
this.teardown(
new Error("Connection timeout to collaboration server"),
false,
);
}, CONNECT_TIMEOUT_MS);
if (process.env.DEBUG)
console.error(`Connecting to WebSocket: ${this.wsUrl}`);
this.provider = providerFactory({
url: this.wsUrl,
name: `page.${this.pageId}`,
document: this.ydoc,
token: this.token,
WebSocketPolyfill: WebSocket,
onConnect: () => {
if (process.env.DEBUG) console.error("WS Connect");
},
// An unexpected disconnect/close at ANY time (during the connect-wait,
// between edits, or during a persistence wait) makes the session dead:
// surface it now instead of hanging, reject any in-flight op with the
// same error text as the one-shot machine, and remove ourselves from
// the registry so the next acquire opens fresh. `teardown` is idempotent
// so the onClose our own destroy() triggers is a harmless no-op.
onDisconnect: () => {
if (process.env.DEBUG) console.error("WS Disconnect");
this.teardown(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
true,
);
},
onClose: () => {
if (process.env.DEBUG) console.error("WS Close");
this.teardown(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
true,
);
},
onSynced: () => {
if (this.dead || this.openSettled) return;
if (process.env.DEBUG) console.error("Connected and synced!");
if (this.connectTimer) {
clearTimeout(this.connectTimer);
this.connectTimer = undefined;
}
this.state = "ready";
this.openSettled = true;
this.openResolve?.();
},
onAuthenticationFailed: () => {
this.teardown(
new Error("Authentication failed for collaboration connection"),
true,
);
},
});
});
return this.openPromise;
}
/**
* Run one atomic read -> transform -> write against the LIVE doc and wait for
* the server to acknowledge the write.
*
* INVARIANT 1 (read->write atomicity): between `TiptapTransformer.fromYdoc`
* and `applyDocToFragment` there is NO `await`. Yjs applies remote updates
* only when the event loop yields, so this synchronous block sees a consistent
* live doc and no concurrent human edit can interleave and be clobbered
* exactly as in the one-shot onSynced code, just on a persistent provider.
*
* INVARIANT 2 (per-edit ack): after the write, resolve immediately if
* unsyncedChanges is already 0, else wait for the unsyncedChanges->0 event
* (PERSIST_TIMEOUT_MS), guarded by connectionLost so a reconnect handshake
* cannot report a false success.
*
* CONCURRENCY: not safe to invoke concurrently on ONE session the caller
* MUST serialize (hold the per-page lock), mirroring acquireCollabSession.
* The in-flight op is tracked in a single `inflightReject` field, so an
* overlapping second call would clobber the first's rejector and leave it
* hanging on disconnect. A fail-fast guard below rejects the overlap instead.
* Sequential (awaited) mutates are fine: localFinish clears inflightReject
* before the promise settles, so the guard is clear by the time the next runs.
*/
mutate(
transform: (liveDoc: any) => any | null,
): Promise<MutationResult> {
// Belt-and-suspenders (acquire already validated): refuse to write on a
// session that is not in a live, synced, ready state.
if (
this.dead ||
this.state !== "ready" ||
this.connectionLost ||
!this.provider ||
this.provider.synced !== true
) {
return Promise.reject(
new Error("Collaboration session is not in a ready state"),
);
}
// Fail-fast on concurrent use: a second overlapping mutate would overwrite
// the first's inflightReject, so a disconnect would only reject the second
// and hang the first until PERSIST_TIMEOUT_MS. Reject the overlap WITHOUT
// touching the in-flight op's state (no localFinish/teardown here).
if (this.inflightReject) {
return Promise.reject(
new Error(
"mutate already in-flight; caller must serialize (hold the page lock)",
),
);
}
return new Promise<MutationResult>((resolve, reject) => {
let settled = false;
let persistTimer: ReturnType<typeof setTimeout> | undefined;
let unsyncedHandler:
| ((data: { number: number }) => void)
| undefined;
// The verifiable result resolved on every success/abort path. Set on
// abort (no-op report) and after a real write (computed change report).
let mutationResult: MutationResult;
const localFinish = (err: Error | null, value?: MutationResult) => {
if (settled) return;
settled = true;
if (persistTimer) clearTimeout(persistTimer);
if (unsyncedHandler && this.provider) {
try {
this.provider.off("unsyncedChanges", unsyncedHandler);
} catch (e) {}
}
this.inflightReject = undefined;
if (err) reject(err);
else resolve(value as MutationResult);
// Post-settle lifecycle: an ephemeral (cache-disabled) session dies with
// its single op; a cached session that is still alive re-arms its idle
// TTL so the clock starts from the LAST op.
if (this.ephemeral) {
this.destroy("ephemeral op complete");
} else if (!this.dead) {
this.armIdle();
}
};
// Register so a disconnect/close/auth-failure/teardown rejects THIS op
// with the connection-loss error text. localFinish's `settled` guard makes
// a racing teardown + normal resolve safe (first one wins).
this.inflightReject = (e: Error) => localFinish(e);
// Resolve once the server acknowledges our update: the provider increments
// unsyncedChanges when the local update is sent and decrements it on the
// server's SyncStatus(applied=true); reaching 0 means the authoritative
// in-memory ydoc on the server now contains our write.
const waitForPersistence = () => {
if (settled) return;
// A missing provider is a failure, not a success: without it the write
// can never have been acknowledged.
if (!this.provider) {
localFinish(new Error("collab provider gone before persistence"));
return;
}
if (this.provider.unsyncedChanges === 0) {
localFinish(null, mutationResult);
return;
}
persistTimer = setTimeout(() => {
localFinish(
new Error(
"Timeout waiting for collaboration server to persist the update",
),
);
}, PERSIST_TIMEOUT_MS);
unsyncedHandler = (data: { number: number }) => {
// Only treat unsyncedChanges->0 as success when the connection is
// still up. A transient disconnect + reconnect handshake can drive the
// counter back to 0 without our write being re-transmitted; in that
// case let the disconnect/close error win instead.
if (data.number === 0 && !this.connectionLost) {
localFinish(null, mutationResult);
}
};
this.provider.on("unsyncedChanges", unsyncedHandler);
};
// CRITICAL: everything between reading the live doc and writing it back
// must stay synchronous (no await). While the JS event loop is not
// yielded, no incoming remote update can interleave, so any already-synced
// concurrent edits are preserved in liveDoc.
let newDoc: any;
let beforeDoc: any;
try {
let liveDoc = TiptapTransformer.fromYdoc(this.ydoc, "default");
if (
!liveDoc ||
typeof liveDoc !== "object" ||
!Array.isArray(liveDoc.content)
) {
liveDoc = { type: "doc", content: [] };
}
// Snapshot the before-doc for the change report. Docs are
// JSON-serializable, so this is a safe deep clone.
beforeDoc = JSON.parse(JSON.stringify(liveDoc));
newDoc = transform(liveDoc);
if (newDoc == null) {
// Transform aborted — write nothing, return the live doc with a no-op
// change report.
mutationResult = {
doc: liveDoc,
verify: {
changed: false,
textInserted: 0,
textDeleted: 0,
blocksChanged: 0,
marks: {},
summary: "no changes (transform aborted)",
},
};
localFinish(null, mutationResult);
return;
}
// Structural diff into the live fragment (issue #152): preserves the Yjs
// ids of unchanged nodes, so an open editor's cursor is not yanked to the
// end of the document on every agent write.
applyDocToFragment(this.ydoc, newDoc);
} catch (e) {
// Includes errors thrown by transform (e.g. "afterText not found",
// "text not found"): propagate them verbatim to the caller.
localFinish(e instanceof Error ? e : new Error(String(e)));
return;
}
// Compute the verifiable change report AFTER the transact write: it only
// needs the JSON before/after, so it cannot affect the atomic read->write
// window, and summarizeChange never throws.
mutationResult = {
doc: newDoc,
verify: summarizeChange(beforeDoc, newDoc),
};
if (process.env.DEBUG)
console.error("Content written, waiting for server to persist...");
waitForPersistence();
});
}
/** (Re)arm the idle TTL so the clock starts from the most recent activity. */
armIdle(): void {
if (this.dead || this.ephemeral) return;
if (this.idleTimer) clearTimeout(this.idleTimer);
if (this.cfg.idleMs > 0) {
this.idleTimer = setTimeout(() => {
this.destroy("idle timeout");
}, this.cfg.idleMs);
// Never let the idle timer keep the process alive.
(this.idleTimer as any).unref?.();
}
}
/**
* Idempotent teardown: mark dead, clear timers, remove from the registry, fail
* any pending open/in-flight op, and destroy the provider. `inflightError` is
* the error a pending open or in-flight op is rejected with; `connectionLoss`
* marks the session as connection-lost so the ack guard cannot report a false
* success on a racing unsyncedChanges->0.
*/
private teardown(inflightError: Error | null, connectionLoss: boolean): void {
if (this.dead) return;
this.dead = true;
this.state = "dead";
if (connectionLoss) this.connectionLost = true;
if (this.connectTimer) {
clearTimeout(this.connectTimer);
this.connectTimer = undefined;
}
if (this.idleTimer) {
clearTimeout(this.idleTimer);
this.idleTimer = undefined;
}
// Remove ourselves from the registry (only if we are still the live entry —
// a re-open under the same key must not be evicted by our teardown).
if (sessions.get(this.key) === this) {
sessions.delete(this.key);
}
// Fail a pending open() and any in-flight mutate with the terminal error.
if (!this.openSettled) {
this.openSettled = true;
this.openReject?.(
inflightError ?? new Error("Collaboration session destroyed"),
);
}
if (this.inflightReject) {
const rej = this.inflightReject;
this.inflightReject = undefined;
rej(inflightError ?? new Error("Collaboration session destroyed"));
}
if (this.provider) {
try {
this.provider.destroy();
} catch (e) {}
this.provider = undefined;
}
}
/**
* Public idempotent teardown used by the acquire/eviction paths and by a
* caller that wants the session dropped after a failed op ("next call
* reconnects fresh").
*/
destroy(reason: string): void {
if (this.dead) return;
if (process.env.DEBUG)
console.error(`Destroying collab session ${this.pageId}: ${reason}`);
this.teardown(new Error(`Collaboration session destroyed: ${reason}`), false);
}
}
/** key = wsUrl + pageId + collabToken (identity isolation: invariant 4). */
const sessions = new Map<string, CollabSession>();
function sessionKey(wsUrl: string, pageId: string, token: string): string {
// The token is part of the key so sessions are NEVER shared between different
// users' MCP sessions (HTTP mode), and a token rotation makes a new entry
// while the old one idles out.
return `${wsUrl}${pageId}${token}`;
}
/**
* Get a live, synced CollabSession for a page, reusing a cached one when it is
* still valid or opening a fresh one otherwise. Does NOT take the per-page lock
* the caller MUST already hold it (both call sites run inside withPageLock,
* which is not reentrant, so acquiring the lock here would deadlock
* mutateLiveContentUnlocked).
*/
export async function acquireCollabSession(
pageId: string,
collabToken: string,
baseUrl: string,
opts?: AcquireOptions,
): Promise<CollabSession> {
const cfg = readConfig();
const wsUrl = buildCollabWsUrl(baseUrl);
// Cache disabled (rollback path): open an unregistered ephemeral session that
// self-destroys after its single op — the exact legacy per-op-provider flow.
if (cfg.idleMs <= 0) {
const session = new CollabSession(
sessionKey(wsUrl, pageId, collabToken),
pageId,
wsUrl,
collabToken,
cfg,
true,
opts,
);
await session.open();
return session;
}
const key = sessionKey(wsUrl, pageId, collabToken);
const existing = sessions.get(key);
if (existing) {
if (existing.isReusable()) {
// Reuse. Refresh LRU order (re-insert = most recently used) and re-arm the
// idle TTL so the reuse counts as activity.
sessions.delete(key);
sessions.set(key, existing);
existing.armIdle();
if (process.env.DEBUG)
console.error(`Reusing collab session for page ${pageId}`);
return existing;
}
// Stale (not synced / past max age / lost): drop it and open fresh.
existing.destroy("stale on reuse");
}
// Enforce the registry cap before inserting: destroy-evict the least recently
// used (the first entry in insertion order) until there is room.
while (sessions.size >= cfg.maxEntries) {
const oldestKey: string | undefined = sessions.keys().next().value;
if (oldestKey === undefined) break;
const victim = sessions.get(oldestKey);
if (victim) victim.destroy("evicted (LRU cap)");
// destroy() removes it from the map; guard against a no-op destroy.
if (sessions.has(oldestKey)) sessions.delete(oldestKey);
}
const session = new CollabSession(
key,
pageId,
wsUrl,
collabToken,
cfg,
false,
opts,
);
sessions.set(key, session);
try {
await session.open();
} catch (e) {
// Failed connect/sync: make sure it is not left cached.
session.destroy("open failed");
throw e;
}
session.armIdle();
if (process.env.DEBUG)
console.error(`Opened new collab session for page ${pageId}`);
return session;
}
/**
* Destroy every cached session. Wired into the process shutdown so a hanging
* session does not keep a doc loaded on the server past exit.
*/
export function destroyAllSessions(): void {
for (const session of [...sessions.values()]) {
session.destroy("process shutdown");
}
sessions.clear();
}
/** TEST-ONLY: number of currently cached sessions. */
export function __sessionCountForTests(): number {
return sessions.size;
}
+24 -210
View File
@@ -1,4 +1,3 @@
import { HocuspocusProvider } from "@hocuspocus/provider";
import { TiptapTransformer } from "@hocuspocus/transformer";
import * as Y from "yjs";
import WebSocket from "ws";
@@ -16,7 +15,8 @@ import { docmostExtensions, docmostSchema } from "./docmost-schema.js";
import { withPageLock } from "./page-lock.js";
import { sanitizeForYjs, findUnstorableAttr } from "./node-ops.js";
import { canonicalizeFootnotes } from "./footnote-canonicalize.js";
import { summarizeChange, VerifyReport } from "./diff.js";
import { VerifyReport } from "./diff.js";
import { acquireCollabSession } from "./collab-session.js";
export { markdownToProseMirror };
@@ -194,26 +194,27 @@ export function assertYjsEncodable(doc: any): void {
}
}
/** Time we wait for the initial handshake/sync before giving up. */
const CONNECT_TIMEOUT_MS = 25000;
/** Time we wait for the server to acknowledge our write before giving up. */
const PERSIST_TIMEOUT_MS = 20000;
/**
* Safely mutate the live content of a page over the collaboration websocket.
*
* This is the single safe write path for every MCP content mutation. It:
* 1. serializes per-page writes through withPageLock (no two MCP writes on
* the same page overlap);
* 2. connects to Hocuspocus and waits for the initial sync so the local ydoc
* mirrors the authoritative server doc INCLUDING edits/comments/images
* that are not yet in the debounced REST snapshot;
* 3. inside onSynced, SYNCHRONOUSLY reads the live doc, runs `transform`, and
* writes the result back with no `await` between read and write so no
* remote update can interleave and clobber concurrent human edits;
* 2. acquires a LIVE, synced CollabSession for the page (issue #400) a
* cached provider whose local ydoc mirrors the authoritative server doc
* (INCLUDING edits/comments/images not yet in the debounced REST snapshot),
* reused across a series of edits instead of a fresh connect/auth/sync per
* call;
* 3. SYNCHRONOUSLY reads the live doc, runs `transform`, and writes the result
* back with no `await` between read and write so no remote update can
* interleave and clobber concurrent human edits (CollabSession.mutate);
* 4. waits for the server to acknowledge the write (unsyncedChanges -> 0)
* before resolving, so the next operation observes our change.
*
* On any mutate failure the session is destroyed so the next call reconnects
* fresh; the page lock is held for the whole acquire+mutate so the session's
* synchronous read->write window never overlaps another MCP write on the page.
*
* `transform` receives the live ProseMirror doc and returns the NEW full
* ProseMirror doc to write, or `null` to abort with no write (a no-op). If
* `transform` throws, the error is propagated to the caller (not swallowed).
@@ -230,7 +231,7 @@ export async function mutatePageContent(
baseUrl: string,
transform: (liveDoc: any) => any | null,
): Promise<MutationResult> {
return withPageLock(pageId, () => {
return withPageLock(pageId, async () => {
if (process.env.DEBUG) {
console.error(`Starting realtime content mutate for page ${pageId}`);
// Token prefix is sensitive; only log it under DEBUG.
@@ -239,202 +240,15 @@ export async function mutatePageContent(
);
}
const ydoc = new Y.Doc();
const wsUrl = buildCollabWsUrl(baseUrl);
if (process.env.DEBUG) console.error(`Connecting to WebSocket: ${wsUrl}`);
return new Promise<MutationResult>((resolve, reject) => {
let provider: HocuspocusProvider | undefined;
let applied = false; // onSynced may fire again on reconnect — apply once.
let settled = false;
// Set true on disconnect/close so a reconnect-driven unsyncedChanges->0
// cannot be mistaken for a successful persist of our write.
let connectionLost = false;
let connectTimer: ReturnType<typeof setTimeout> | undefined;
let persistTimer: ReturnType<typeof setTimeout> | undefined;
let unsyncedHandler: ((data: { number: number }) => void) | undefined;
const cleanup = () => {
if (connectTimer) clearTimeout(connectTimer);
if (persistTimer) clearTimeout(persistTimer);
if (provider) {
if (unsyncedHandler) {
try {
provider.off("unsyncedChanges", unsyncedHandler);
} catch (err) {}
}
try {
provider.destroy();
} catch (err) {}
}
};
const finish = (err: Error | null, value?: MutationResult) => {
if (settled) return;
settled = true;
cleanup();
if (err) reject(err);
else resolve(value as MutationResult);
};
connectTimer = setTimeout(() => {
finish(new Error("Connection timeout to collaboration server"));
}, CONNECT_TIMEOUT_MS);
// Resolve once the server has acknowledged our update. The provider
// increments unsyncedChanges when our local update is sent and
// decrements it when the server replies with a SyncStatus(applied=true);
// reaching 0 means the authoritative in-memory ydoc on the server now
// contains our write.
const waitForPersistence = () => {
if (settled) return;
// A missing provider is a failure, not a success: without it the write
// can never have been acknowledged. Only an actual unsyncedChanges===0
// on a live provider counts as persisted.
if (!provider) {
finish(new Error("collab provider gone before persistence"));
return;
}
if (provider.unsyncedChanges === 0) {
finish(null, mutationResult);
return;
}
persistTimer = setTimeout(() => {
finish(
new Error(
"Timeout waiting for collaboration server to persist the update",
),
);
}, PERSIST_TIMEOUT_MS);
unsyncedHandler = (data: { number: number }) => {
// Only treat unsyncedChanges->0 as success when the connection is
// still up. A transient disconnect + reconnect handshake can drive
// the counter back to 0 without our write being re-transmitted; in
// that case let the disconnect/close error win instead.
if (data.number === 0 && !connectionLost) {
finish(null, mutationResult);
}
};
provider.on("unsyncedChanges", unsyncedHandler);
};
// The verifiable result resolved on every success/abort path. Set on
// abort (no-op report) and after a real write (computed change report).
let mutationResult: MutationResult;
provider = new HocuspocusProvider({
url: wsUrl,
name: `page.${pageId}`,
document: ydoc,
token: collabToken,
// @ts-ignore - Required for Node.js environment
WebSocketPolyfill: WebSocket,
onConnect: () => {
if (process.env.DEBUG) console.error("WS Connect");
},
// An unexpected disconnect/close while we are still waiting (during the
// connect-wait before onSynced, or during the persistence wait after the
// write) means the update will never be acknowledged — surface it now
// instead of hanging until the connect/persist timeout fires. `finish`
// is idempotent via the `settled` flag, so the onClose that our own
// cleanup()->provider.destroy() triggers (after settled=true is set) is
// a harmless no-op and cannot cause a double-resolve.
onDisconnect: () => {
if (process.env.DEBUG) console.error("WS Disconnect");
// Mark BEFORE finish so the unsyncedChanges handler (if it races)
// sees the connection as lost and won't report a false success.
connectionLost = true;
finish(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
);
},
onClose: () => {
if (process.env.DEBUG) console.error("WS Close");
// Mark BEFORE finish so the unsyncedChanges handler (if it races)
// sees the connection as lost and won't report a false success.
connectionLost = true;
finish(
new Error(
"Collaboration connection closed before the update was persisted/synced",
),
);
},
onSynced: () => {
if (applied || settled) return;
applied = true;
if (process.env.DEBUG) console.error("Connected and synced!");
// CRITICAL: everything between reading the live doc and writing it
// back must stay synchronous (no await). While the JS event loop is
// not yielded, no incoming remote update can interleave, so any
// already-synced concurrent edits are preserved in liveDoc.
let newDoc: any;
let beforeDoc: any;
try {
let liveDoc = TiptapTransformer.fromYdoc(ydoc, "default");
if (
!liveDoc ||
typeof liveDoc !== "object" ||
!Array.isArray(liveDoc.content)
) {
liveDoc = { type: "doc", content: [] };
}
// Snapshot the before-doc for the change report. Docs are
// JSON-serializable, so this is a safe deep clone.
beforeDoc = JSON.parse(JSON.stringify(liveDoc));
newDoc = transform(liveDoc);
if (newDoc == null) {
// Transform aborted — write nothing, return the live doc with a
// no-op change report.
mutationResult = {
doc: liveDoc,
verify: {
changed: false,
textInserted: 0,
textDeleted: 0,
blocksChanged: 0,
marks: {},
summary: "no changes (transform aborted)",
},
};
finish(null, mutationResult);
return;
}
// Structural diff into the live fragment (issue #152): preserves
// the Yjs ids of unchanged nodes, so an open editor's cursor is not
// yanked to the end of the document on every agent write.
applyDocToFragment(ydoc, newDoc);
} catch (e) {
// Includes errors thrown by transform (e.g. "afterText not found",
// "text not found"): propagate them verbatim to the caller.
finish(e instanceof Error ? e : new Error(String(e)));
return;
}
// Compute the verifiable change report AFTER the transact write: it
// only needs the JSON before/after, so it cannot affect the atomic
// read->write window, and summarizeChange never throws.
mutationResult = {
doc: newDoc,
verify: summarizeChange(beforeDoc, newDoc),
};
if (process.env.DEBUG)
console.error("Content written, waiting for server to persist...");
waitForPersistence();
},
onAuthenticationFailed: () => {
finish(
new Error("Authentication failed for collaboration connection"),
);
},
});
});
const session = await acquireCollabSession(pageId, collabToken, baseUrl);
try {
return await session.mutate(transform);
} catch (e) {
// Drop the session on any failure so the next call reconnects fresh (this
// also closes the "reconnect drove the counter to 0" false-success class).
session.destroy("mutate failed");
throw e;
}
});
}
+84 -10
View File
@@ -17,8 +17,23 @@
* comparing and match across maximal runs of consecutive text nodes within a
* single block, while mapping every normalized character back to its raw index
* so the mark lands on the exact original characters.
*
* MARKDOWN-STRIP FALLBACK: when the agent copies a selection that still carries
* inline markdown (`**bold**`, `` `code` ``, `[t](u)`), the raw locator will not
* match the document's plain text. Exactly like edit_page_text's json-edit
* fallback, we first try the verbatim selection and, ONLY if it anchors nowhere
* in the whole document, retry with `stripInlineMarkdown` applied. `canAnchorInDoc`,
* `getAnchoredText` and `applyAnchorInDoc` share this decision via
* `resolveAnchorSelection`. `countAnchorMatches` keeps its OWN parallel exact-wins
* implementation (it needs a raw match COUNT, not a single resolved locator), kept
* deliberately in sync with `resolveAnchorSelection`: raw match use raw, else fall
* back to the stripped count. All four therefore agree on which locator matched
* the suggestion-uniqueness gate depends on count and can/get never disagreeing, so
* these two exact-wins implementations MUST stay in sync if either is changed.
*/
import { stripInlineMarkdown } from "./text-normalize.js";
/** Typographic double-quote variants mapped to ASCII `"`. */
const DOUBLE_QUOTES = "«»„“”‟〝〞"";
/** Typographic single-quote/apostrophe variants mapped to ASCII `'`. */
@@ -214,15 +229,17 @@ function reconstructRawText(blockContent: any[], match: AnchorMatch): string {
* un-appliable (spurious 409).
*/
export function getAnchoredText(doc: any, selection: string): string | null {
const { selection: effective, found } = resolveAnchorSelection(doc, selection);
if (!found) return null;
const visit = (node: any, depth: number): string | null => {
if (depth > MAX_DEPTH || !node || typeof node !== "object") return null;
if (!Array.isArray(node.content)) return null;
const match = findAnchorInBlock(node.content, selection);
const match = findAnchorInBlock(node.content, effective);
if (match) return reconstructRawText(node.content, match);
for (const child of node.content) {
if (child && typeof child === "object" && Array.isArray(child.content)) {
const found = visit(child, depth + 1);
if (found !== null) return found;
const foundText = visit(child, depth + 1);
if (foundText !== null) return foundText;
}
}
return null;
@@ -231,12 +248,11 @@ export function getAnchoredText(doc: any, selection: string): string | null {
}
/**
* Depth-first, document-order check for whether `selection` can be anchored
* anywhere in `doc`. At each node with an array `content`, first try to match
* within that node's own content, then recurse into children that themselves
* have a `content` array.
* RAW (no markdown-strip fallback) depth-first check that `selection` anchors
* somewhere in `doc`. This is the primitive `resolveAnchorSelection` builds on;
* public callers should use `canAnchorInDoc`, which adds the strip fallback.
*/
export function canAnchorInDoc(doc: any, selection: string): boolean {
function rawCanAnchorInDoc(doc: any, selection: string): boolean {
const visit = (node: any, depth: number): boolean => {
if (depth > MAX_DEPTH || !node || typeof node !== "object") return false;
if (!Array.isArray(node.content)) return false;
@@ -251,6 +267,43 @@ export function canAnchorInDoc(doc: any, selection: string): boolean {
return visit(doc, 0);
}
/**
* Decide the locator that ACTUALLY anchors `selection` in `doc`, applying the
* markdown-strip fallback once (so every public entry point agrees):
* - EXACT WINS: if the verbatim selection anchors anywhere, use it as-is.
* - FALLBACK: only if the verbatim selection anchors nowhere, and the
* markdown-stripped form differs and DOES anchor, use the stripped form and
* flag `normalized` so callers can surface a soft warning.
* - otherwise `found` is false and `selection` is returned unchanged.
*
* The stripped form is used ONLY to LOCATE the anchor; getAnchoredText still
* reconstructs and stores the RAW document substring, so the strip never leaks
* into what gets persisted.
*/
export function resolveAnchorSelection(
doc: any,
selection: string,
): { selection: string; found: boolean; normalized: boolean } {
if (rawCanAnchorInDoc(doc, selection)) {
return { selection, found: true, normalized: false };
}
const stripped = stripInlineMarkdown(selection);
if (stripped !== selection && rawCanAnchorInDoc(doc, stripped)) {
return { selection: stripped, found: true, normalized: true };
}
return { selection, found: false, normalized: false };
}
/**
* Depth-first, document-order check for whether `selection` can be anchored
* anywhere in `doc` (with the markdown-strip fallback). At each node with an
* array `content`, first try to match within that node's own content, then
* recurse into children that themselves have a `content` array.
*/
export function canAnchorInDoc(doc: any, selection: string): boolean {
return resolveAnchorSelection(doc, selection).found;
}
/**
* Split the matched text nodes and splice the comment mark across the range.
* `blockContent` is mutated IN PLACE. `match.startChild..endChild` are all text
@@ -315,7 +368,7 @@ function spliceCommentMark(
* not use this. (Note: counts OCCURRENCES, not just matching blocks, so two
* occurrences inside one block are correctly reported as 2.)
*/
export function countAnchorMatches(doc: any, selection: string): number {
function rawCountAnchorMatches(doc: any, selection: string): number {
const normSel = normalizeForMatch(selection).norm.trim();
if (normSel.length === 0) return 0;
@@ -369,6 +422,25 @@ export function countAnchorMatches(doc: any, selection: string): number {
return total;
}
/**
* Uniqueness gate for suggestions, with the SAME markdown-strip fallback as the
* other entry points so count never disagrees with can/get/apply. EXACT WINS: if
* the verbatim selection occurs at all, return its raw occurrence count (so a
* selection that is unique raw stays unique the fallback never runs and cannot
* introduce a spurious second match). Only when the verbatim selection is absent
* do we count occurrences of the markdown-stripped form.
*/
export function countAnchorMatches(doc: any, selection: string): number {
const raw = rawCountAnchorMatches(doc, selection);
if (raw > 0) return raw;
const stripped = stripInlineMarkdown(selection);
if (stripped !== selection) {
const strippedCount = rawCountAnchorMatches(doc, stripped);
if (strippedCount > 0) return strippedCount;
}
return 0;
}
/**
* Depth-first (same order as canAnchorInDoc) over `doc`; on the FIRST block
* whose content matches `selection`, splice the comment mark across the matched
@@ -380,10 +452,12 @@ export function applyAnchorInDoc(
selection: string,
commentId: string,
): boolean {
const { selection: effective, found } = resolveAnchorSelection(doc, selection);
if (!found) return false;
const visit = (node: any, depth: number): boolean => {
if (depth > MAX_DEPTH || !node || typeof node !== "object") return false;
if (!Array.isArray(node.content)) return false;
const match = findAnchorInBlock(node.content, selection);
const match = findAnchorInBlock(node.content, effective);
if (match) {
spliceCommentMark(node.content, match, commentId);
return true;
+8 -24
View File
@@ -12,7 +12,11 @@
* re-import for small wording fixes.
*/
import { stripInlineMarkdown, stripBalancedWrappers } from "./text-normalize.js";
import {
stripInlineMarkdown,
stripBalancedWrappers,
closestBlockHint,
} from "./text-normalize.js";
export interface TextEdit {
find: string;
@@ -381,29 +385,9 @@ export function applyTextEdits(
} else {
// Append a bounded "closest text" hint: find the FIRST block that
// contains the longest whitespace-delimited token (>= 3 chars) of the
// (stripped, then raw) locator, and quote that block's plain text.
reason = "text not found in the document.";
const tokenSource = stripped.length > 0 ? stripped : edit.find;
const longestToken = tokenSource
.split(/\s+/)
.filter((t) => t.length >= 3)
.sort((a, b) => b.length - a.length)[0];
if (longestToken) {
const hitBlock = blockPlain.find((plain) =>
plain.includes(longestToken),
);
if (hitBlock) {
// Truncate by code point (spread iterates by code point) so a
// surrogate pair is never split; append the ellipsis only when the
// text was actually longer than the limit.
const points = [...hitBlock];
const snippet =
points.length > 120
? points.slice(0, 120).join("") + "…"
: hitBlock;
reason += ` Closest block text: "${snippet}".`;
}
}
// (stripped, then raw) locator, and quote that block's plain text. Shared
// with create_comment via closestBlockHint so both give the same hint.
reason = "text not found in the document." + closestBlockHint(blockPlain, edit.find);
}
failed.push({ find: edit.find, reason });
continue;
+34
View File
@@ -114,3 +114,37 @@ export function stripInlineMarkdown(s: string): string {
return out;
}
/**
* Build a bounded "closest text" hint for an anchor/find MISS, shared by
* edit_page_text (json-edit) and create_comment (client) so both surface the
* same self-correction affordance.
*
* Take the longest whitespace-delimited token (>= 3 chars) of the locator
* (markdown-stripped first, so `**bold**` contributes `bold`), find the FIRST
* of `blockTexts` that contains it, and return ` Closest block text: "…".` with
* the block quoted (truncated to 120 code points + ellipsis). Returns "" when
* no token qualifies or no block contains it, so the caller can append it
* unconditionally.
*/
export function closestBlockHint(
blockTexts: string[],
locator: string,
): string {
if (typeof locator !== "string" || locator.length === 0) return "";
const stripped = stripInlineMarkdown(locator);
const tokenSource = stripped.length > 0 ? stripped : locator;
const longestToken = tokenSource
.split(/\s+/)
.filter((t) => t.length >= 3)
.sort((a, b) => b.length - a.length)[0];
if (!longestToken) return "";
const hitBlock = blockTexts.find((plain) => plain.includes(longestToken));
if (!hitBlock) return "";
// Truncate by code point (spread iterates by code point) so a surrogate pair
// is never split; append the ellipsis only when the text was actually longer.
const points = [...hitBlock];
const snippet =
points.length > 120 ? points.slice(0, 120).join("") + "…" : hitBlock;
return ` Closest block text: "${snippet}".`;
}
+15
View File
@@ -1,6 +1,7 @@
#!/usr/bin/env node
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
import { createDocmostMcpServer } from "./index.js";
import { destroyAllSessions } from "./lib/collab-session.js";
// Standalone stdio entrypoint. This restores the original behavior of the
// package when run as a CLI (`docmost-mcp`): it reads credentials from the
@@ -33,6 +34,20 @@ async function run() {
console.error("Uncaught exception:", error);
});
// Teardown hook (issue #400): destroy every cached live CollabSession on exit
// so a hanging session does not keep a doc loaded on the server (which would
// also defer the server's afterUnloadDocument cleanup). `exit` runs the
// synchronous idempotent teardown; SIGINT/SIGTERM also run it, then exit.
process.on("exit", () => {
destroyAllSessions();
});
for (const signal of ["SIGINT", "SIGTERM"] as const) {
process.on(signal, () => {
destroyAllSessions();
process.exit(0);
});
}
const server = createDocmostMcpServer({
apiUrl: API_URL!,
email: EMAIL!,
+7 -3
View File
@@ -771,9 +771,13 @@ export const SHARED_TOOL_SPECS = {
'The comment is anchored inline to the given exact `selection` text ' +
'(which gets highlighted); page-level comments are NOT supported. A ' +
'new top-level comment REQUIRES a `selection`. Replies inherit the ' +
"parent's anchor and take no selection. If the call fails with a " +
'"selection not found" error, retry with a corrected EXACT selection ' +
'copied verbatim from a single paragraph/block. You may also attach a ' +
"parent's anchor and take no selection. Always COPY the `selection` " +
'VERBATIM from get_page / search_in_page output — do NOT quote it from ' +
'memory (stale-memory quoting is the top cause of anchor misses). If the ' +
'call fails with a "selection not found" error, the error quotes the ' +
"closest block text (or says the selection spans multiple blocks); retry " +
"with a corrected EXACT selection copied verbatim from a single " +
'paragraph/block. You may also attach a ' +
'`suggestedText` proposing a replacement for the `selection` (a human ' +
'applies it from the UI); when set, the `selection` must occur exactly ' +
'once in the page. Reversible via the comment UI.',
@@ -548,3 +548,94 @@ test("suggestedText: the stored selection is the doc's RAW typographic substring
);
assert.equal(createPayload.suggestedText, "goodbye");
});
// -----------------------------------------------------------------------------
// 8) #408: a not-found selection error QUOTES the closest block text so the
// model can self-correct instead of blind-retrying.
// -----------------------------------------------------------------------------
test("a not-found selection error includes a 'Closest block text' hint", async () => {
let createCalls = 0;
const { baseURL } = await spawn(async (req, res) => {
await readBody(req);
if (req.url === "/api/auth/login") {
sendJson(res, 200, { success: true }, {
"Set-Cookie": "authToken=t; Path=/; HttpOnly",
});
return;
}
if (req.url === "/api/pages/info") {
sendJson(res, 200, {
data: {
id: "page-1",
content: {
type: "doc",
content: [
{ type: "paragraph", content: [{ type: "text", text: "The quick brown fox jumps" }] },
],
},
},
});
return;
}
if (req.url === "/api/comments/create") {
createCalls++;
sendJson(res, 200, { data: { id: "should-not-happen" } });
return;
}
sendJson(res, 404, { message: "not found" });
});
const client = new DocmostClient(baseURL, "user@example.com", "pw");
await assert.rejects(
() => client.createComment("page-1", "body", "inline", "quick brown cat"),
/Closest block text: "The quick brown fox jumps"/,
"a not-found selection must quote the closest block text",
);
assert.equal(createCalls, 0, "/comments/create must NOT be called on a miss");
});
// -----------------------------------------------------------------------------
// 9) #408: a selection that straddles two blocks gets the explicit
// "spans multiple blocks" message instead of a bare not-found.
// -----------------------------------------------------------------------------
test("a selection spanning multiple blocks gets the explicit spans-multiple-blocks message", async () => {
let createCalls = 0;
const { baseURL } = await spawn(async (req, res) => {
await readBody(req);
if (req.url === "/api/auth/login") {
sendJson(res, 200, { success: true }, {
"Set-Cookie": "authToken=t; Path=/; HttpOnly",
});
return;
}
if (req.url === "/api/pages/info") {
sendJson(res, 200, {
data: {
id: "page-1",
content: {
type: "doc",
content: [
{ type: "paragraph", content: [{ type: "text", text: "the quick brown" }] },
{ type: "paragraph", content: [{ type: "text", text: "fox jumps over" }] },
],
},
},
});
return;
}
if (req.url === "/api/comments/create") {
createCalls++;
sendJson(res, 200, { data: { id: "should-not-happen" } });
return;
}
sendJson(res, 404, { message: "not found" });
});
const client = new DocmostClient(baseURL, "user@example.com", "pw");
await assert.rejects(
() => client.createComment("page-1", "body", "inline", "brown fox"),
/spans multiple blocks/,
"a cross-block selection must report the spans-multiple-blocks hint",
);
assert.equal(createCalls, 0, "/comments/create must NOT be called on a miss");
});
@@ -19,6 +19,7 @@ import { WebSocketServer } from "ws";
import { Hocuspocus } from "@hocuspocus/server";
import { DocmostClient } from "../../build/client.js";
import { buildYDoc } from "../../build/lib/collaboration.js";
import { destroyAllSessions } from "../../build/lib/collab-session.js";
// Import the SAME page-lock module instance that build/client.js imports. ESM
// caches modules by resolved URL, so this `withPageLock` shares the very
// per-page mutex map (`chains`) the client uses — letting the replaceImage test
@@ -188,6 +189,10 @@ async function spawnCollabStack(opts = {}) {
const openStacks = [];
after(async () => {
// #400: tests now leave a cached live CollabSession per page. Destroy them
// first (closes the client ws) so the server.close() below is not racing an
// open collab connection.
destroyAllSessions();
await Promise.all(
openStacks.map(
({ server, hocuspocus }) =>
@@ -270,17 +275,23 @@ test("a UUID input is passed through unchanged and triggers NO /pages/info fetch
);
});
test("a repeated slugId edit resolves the UUID only once (cache)", async () => {
test("repeated slugId edits reuse ONE live collab session and resolve the UUID only once (#400 cache)", async () => {
const { state, baseURL } = await spawnCollabStack();
const client = new DocmostClient(baseURL, "user@example.com", "pw");
// Each mock connection re-seeds a fresh "hello world" doc (the mock does not
// persist across connects), so both edits target "hello". The cache assertion
// only concerns the slugId->uuid resolution, not the document content.
// #400: a series of edits on the same page reuses ONE live CollabSession, so
// the connect/handshake happens once and the collab doc is OPENED a single
// time (not per edit). The live ydoc persists between edits (the whole point),
// so the second edit sees the first edit's result: after "hello" -> "hi world"
// it targets the still-present "world".
await client.editPageText(SLUG, [{ find: "hello", replace: "hi" }]);
await client.editPageText(SLUG, [{ find: "hello", replace: "hey" }]);
await client.editPageText(SLUG, [{ find: "world", replace: "planet" }]);
assert.deepEqual(state.docNames, [`page.${UUID}`, `page.${UUID}`]);
assert.deepEqual(
state.docNames,
[`page.${UUID}`],
"the two edits must reuse one live collab session -> a single collab-doc open (#400)",
);
assert.equal(
state.pagesInfoCalls.length,
1,
@@ -325,8 +336,9 @@ test("replaceImage opens by the resolved UUID AND keys its page lock by that UUI
await uploadStarted; // deterministic: replaceImage now holds its page lock.
// (a) OPEN BY UUID: the only collab doc opened so far (the scan pass) used the
// canonical UUID, never the slugId. (The write pass opens a second time after
// we release the gate; asserted at the end.)
// canonical UUID, never the slugId. (#400: the write pass will REUSE this same
// live session rather than reopen, so docNames stays a single entry — asserted
// at the end.)
assert.deepEqual(
state.docNames,
[`page.${UUID}`],
@@ -378,8 +390,9 @@ test("replaceImage opens by the resolved UUID AND keys its page lock by that UUI
assert.equal(res.success, true);
assert.equal(res.replaced, 1, "the one seeded image must be repointed");
// Both opens (scan pass + write pass) used the UUID; the slugId never appears.
assert.deepEqual(state.docNames, [`page.${UUID}`, `page.${UUID}`]);
// #400: the write pass REUSES the scan pass's live session, so the collab doc
// is opened ONCE across both passes (never reopened, never by the slugId).
assert.deepEqual(state.docNames, [`page.${UUID}`]);
assert.ok(
!state.docNames.includes(`page.${SLUG}`),
"replaceImage must NEVER open the collab doc by the slugId (the #260 bug)",
@@ -81,6 +81,10 @@ const HOST_CONTRACT_METHODS = [
"insertImage",
"replaceImage",
"insertFootnote",
// draw.io diagrams (#423, stage 1) — read + create + optimistic-locked update
"drawioGet",
"drawioCreate",
"drawioUpdate",
// write (comment)
"createComment",
"resolveComment",
@@ -0,0 +1,348 @@
import { test, beforeEach, afterEach, mock } from "node:test";
import assert from "node:assert/strict";
import { EventEmitter } from "node:events";
import {
acquireCollabSession,
destroyAllSessions,
__setCollabProviderFactory,
__sessionCountForTests,
} from "../../build/lib/collab-session.js";
import { withPageLock } from "../../build/lib/page-lock.js";
// A stand-in for HocuspocusProvider: it shares the ydoc (so the real yjs
// read/transform/write in CollabSession.mutate runs unchanged), auto-completes
// the connect+sync handshake on a microtask (unaffected by mock timers), and
// exposes hooks to drive disconnect/close/auth-failure and the unsyncedChanges
// ack. There is no collaboration server in the test env, so every test drives
// the provider through this fake.
class FakeProvider extends EventEmitter {
static instances = [];
static connectCount = 0;
static reset() {
FakeProvider.instances = [];
FakeProvider.connectCount = 0;
}
static last() {
return FakeProvider.instances[FakeProvider.instances.length - 1];
}
constructor(config, opts = {}) {
super();
this.config = config;
this.ydoc = config.document;
this.synced = false;
this.unsyncedChanges = opts.unsynced ?? 0;
this.destroyed = false;
FakeProvider.instances.push(this);
FakeProvider.connectCount += 1;
if (opts.autoSync !== false) {
// Real HocuspocusProvider fires onSynced asynchronously after the
// handshake; a microtask reproduces that without depending on timers.
queueMicrotask(() => {
if (this.destroyed) return;
this.config.onConnect?.();
this.synced = true;
this.config.onSynced?.();
});
}
}
destroy() {
this.destroyed = true;
}
// --- test drivers ---
_disconnect() {
this.config.onDisconnect?.();
}
_close() {
this.config.onClose?.();
}
_authFail() {
this.config.onAuthenticationFailed?.();
}
_ack() {
this.unsyncedChanges = 0;
this.emit("unsyncedChanges", { number: 0 });
}
}
/** Build a provider factory that stamps every provider with the given opts. */
function factory(opts = {}) {
return (config) => new FakeProvider(config, opts);
}
/** A minimal, schema-valid ProseMirror doc for a write. */
function docWith(text) {
return {
type: "doc",
content: [
{ type: "paragraph", content: [{ type: "text", text: String(text) }] },
],
};
}
const ENV_KEYS = [
"MCP_COLLAB_SESSION_IDLE_MS",
"MCP_COLLAB_SESSION_MAX_AGE_MS",
"MCP_COLLAB_SESSION_MAX_ENTRIES",
];
let savedEnv;
beforeEach(() => {
savedEnv = {};
for (const k of ENV_KEYS) savedEnv[k] = process.env[k];
FakeProvider.reset();
__setCollabProviderFactory(factory());
});
afterEach(() => {
destroyAllSessions();
__setCollabProviderFactory(null);
mock.timers.reset();
for (const k of ENV_KEYS) {
if (savedEnv[k] === undefined) delete process.env[k];
else process.env[k] = savedEnv[k];
}
});
test("acquire opens one provider; reuse returns the SAME live session", async () => {
const a = await acquireCollabSession("page-1", "tok", "http://h/api");
const b = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.equal(a, b, "same (wsUrl,page,token) must reuse the session");
assert.equal(FakeProvider.connectCount, 1, "exactly one connect/sync");
assert.equal(__sessionCountForTests(), 1);
});
test("N mutates on one page open the provider ONCE (coalesced series)", async () => {
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
for (let i = 0; i < 20; i++) {
const r = await session.mutate(() => docWith(`edit ${i}`));
assert.ok(r && r.doc, "each mutate resolves a MutationResult");
}
assert.equal(
FakeProvider.connectCount,
1,
"20 mutates must not reconnect — one live provider for the whole series",
);
});
test("mutate preserves the transform-abort no-op report (null transform)", async () => {
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
const r = await session.mutate(() => null);
assert.equal(r.verify.changed, false);
assert.equal(r.verify.summary, "no changes (transform aborted)");
});
test("registry key includes the token: different tokens => different sessions", async () => {
const a = await acquireCollabSession("page-1", "tok-A", "http://h/api");
const b = await acquireCollabSession("page-1", "tok-B", "http://h/api");
assert.notEqual(a, b, "different tokens must never share a session");
assert.equal(FakeProvider.connectCount, 2);
assert.equal(__sessionCountForTests(), 2);
// Same token reuses.
const a2 = await acquireCollabSession("page-1", "tok-A", "http://h/api");
assert.equal(a, a2);
assert.equal(FakeProvider.connectCount, 2);
});
test("disconnect at any time kills the session and removes it from the registry", async () => {
const a = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.equal(__sessionCountForTests(), 1);
FakeProvider.last()._disconnect();
assert.equal(__sessionCountForTests(), 0, "dead session is deregistered");
// Re-acquire opens a brand new provider.
const b = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.notEqual(a, b);
assert.equal(FakeProvider.connectCount, 2);
});
test("an in-flight mutate rejects with the connection-closed text on disconnect", async () => {
__setCollabProviderFactory(factory({ unsynced: 1 })); // stay pending after write
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
const p = session.mutate(() => docWith("x"));
// The write happened synchronously; persistence is pending. Now the socket drops.
FakeProvider.last()._disconnect();
await assert.rejects(
p,
/Collaboration connection closed before the update was persisted\/synced/,
);
});
test("auth failure rejects the pending open with the auth error text", async () => {
__setCollabProviderFactory(factory({ autoSync: false }));
const p = acquireCollabSession("page-1", "tok", "http://h/api");
// Let the provider be constructed, then fail auth.
await Promise.resolve();
FakeProvider.last()._authFail();
await assert.rejects(
p,
/Authentication failed for collaboration connection/,
);
assert.equal(__sessionCountForTests(), 0);
});
test("mutate-error invalidates the session (caller destroys; re-acquire is fresh)", async () => {
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
await assert.rejects(
session.mutate(() => {
throw new Error("afterText not found");
}),
/afterText not found/,
);
// The production caller destroys on failure; mirror that here.
session.destroy("mutate failed");
assert.equal(__sessionCountForTests(), 0);
const fresh = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.notEqual(fresh, session);
assert.equal(FakeProvider.connectCount, 2);
});
test("a pending write resolves when the server acks (unsyncedChanges -> 0)", async () => {
__setCollabProviderFactory(factory({ unsynced: 1 }));
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
const p = session.mutate(() => docWith("y"));
FakeProvider.last()._ack();
const r = await p;
assert.ok(r.doc);
});
test("concurrent mutate on one session: the second rejects, the first is unaffected", async () => {
__setCollabProviderFactory(factory({ unsynced: 1 })); // first stays pending after write
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
// First mutate: write happens synchronously, persistence ack still pending.
const first = session.mutate(() => docWith("first"));
// Second overlapping mutate on the SAME session must fail fast.
await assert.rejects(
session.mutate(() => docWith("second")),
/mutate already in-flight; caller must serialize \(hold the page lock\)/,
);
// The first op is untouched: its rejector was not clobbered. Ack it now.
FakeProvider.last()._ack();
const r = await first;
assert.ok(r.doc, "the first in-flight mutate still resolves on its own ack");
assert.equal(FakeProvider.connectCount, 1, "no reconnect from the rejected overlap");
});
test("sequential mutates on one session both succeed (the guard doesn't break serialized use)", async () => {
const session = await acquireCollabSession("page-1", "tok", "http://h/api");
// Await the first fully, then run the second: inflightReject was cleared by
// localFinish before the first settled, so the guard is clear for the second.
const r1 = await session.mutate(() => docWith("one"));
assert.ok(r1.doc, "first sequential mutate resolves");
const r2 = await session.mutate(() => docWith("two"));
assert.ok(r2.doc, "second sequential mutate resolves — guard not tripped");
assert.equal(FakeProvider.connectCount, 1, "sequential mutates reuse one provider");
});
test("connect timeout rejects with the connect-timeout text and fires the metric hook", async () => {
mock.timers.enable({ apis: ["setTimeout"] });
__setCollabProviderFactory(factory({ autoSync: false }));
let metricFired = 0;
const p = acquireCollabSession("page-1", "tok", "http://h/api", {
onConnectTimeout: () => {
metricFired += 1;
},
});
mock.timers.tick(25000);
await assert.rejects(p, /Connection timeout to collaboration server/);
assert.equal(metricFired, 1);
assert.equal(__sessionCountForTests(), 0);
});
test("idle TTL destroys the session; re-acquire reconnects", async () => {
mock.timers.enable({ apis: ["setTimeout"] });
process.env.MCP_COLLAB_SESSION_IDLE_MS = "1000";
const a = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.equal(__sessionCountForTests(), 1);
mock.timers.tick(1000); // idle fires
assert.equal(__sessionCountForTests(), 0, "idle timeout destroyed it");
const b = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.notEqual(a, b);
assert.equal(FakeProvider.connectCount, 2);
});
test("max age is enforced at acquire (destroy + open fresh), idle held off", async () => {
mock.timers.enable({ apis: ["setTimeout", "Date"] });
process.env.MCP_COLLAB_SESSION_MAX_AGE_MS = "1000";
process.env.MCP_COLLAB_SESSION_IDLE_MS = "10000000"; // never fires in this test
const a = await acquireCollabSession("page-1", "tok", "http://h/api");
mock.timers.tick(2000); // past max age, below idle
const b = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.notEqual(a, b, "a session past its max age must be replaced");
assert.equal(FakeProvider.connectCount, 2);
});
test("registry cap: least-recently-used session is destroy-evicted", async () => {
process.env.MCP_COLLAB_SESSION_MAX_ENTRIES = "2";
const s1 = await acquireCollabSession("page-1", "tok", "http://h/api");
const p1prov = FakeProvider.last();
const s2 = await acquireCollabSession("page-2", "tok", "http://h/api");
const p2prov = FakeProvider.last();
assert.equal(__sessionCountForTests(), 2);
// Touch page-1 so page-2 becomes the least-recently-used entry.
assert.equal(await acquireCollabSession("page-1", "tok", "http://h/api"), s1);
assert.equal(FakeProvider.connectCount, 2, "reuse must not reconnect");
// A third distinct page (cap 2) must evict the LRU (page-2), not page-1.
const s3 = await acquireCollabSession("page-3", "tok", "http://h/api");
assert.equal(__sessionCountForTests(), 2);
assert.equal(FakeProvider.connectCount, 3);
assert.equal(p2prov.destroyed, true, "the LRU provider was destroy-evicted");
assert.equal(p1prov.destroyed, false, "the MRU page-1 survived");
// page-1 still reused (survived), page-3 still reused.
assert.equal(await acquireCollabSession("page-1", "tok", "http://h/api"), s1);
assert.equal(await acquireCollabSession("page-3", "tok", "http://h/api"), s3);
assert.equal(FakeProvider.connectCount, 3, "no extra reconnects");
});
test("MCP_COLLAB_SESSION_IDLE_MS=0 disables the cache (legacy provider-per-op)", async () => {
process.env.MCP_COLLAB_SESSION_IDLE_MS = "0";
const s1 = await acquireCollabSession("page-1", "tok", "http://h/api");
// Never registered (ephemeral).
assert.equal(__sessionCountForTests(), 0);
const r1 = await s1.mutate(() => docWith("a"));
assert.ok(r1.doc);
// After its single op the ephemeral session self-destroyed.
assert.equal(FakeProvider.instances[0].destroyed, true);
// A second op opens a BRAND NEW provider (no reuse).
const s2 = await acquireCollabSession("page-1", "tok", "http://h/api");
assert.notEqual(s1, s2);
assert.equal(FakeProvider.connectCount, 2);
});
test("replaceImage-shaped flow: acquire under an EXTERNAL page lock does not deadlock and reuses one session", async () => {
const pageId = "page-lock";
// Mirror replaceImage: hold ONE withPageLock across scan (read-only) + write,
// each going through the non-locking acquireCollabSession.
const result = await withPageLock(pageId, async () => {
// pass 1: read-only scan (transform returns null -> no write)
const scan = await acquireCollabSession(pageId, "tok", "http://h/api");
const scanRes = await scan.mutate(() => null);
assert.equal(scanRes.verify.changed, false);
// pass 2: the actual write, same held lock
const write = await acquireCollabSession(pageId, "tok", "http://h/api");
return write.mutate(() => docWith("repointed"));
});
assert.ok(result.doc, "the locked scan+write completed without deadlock");
assert.equal(
FakeProvider.connectCount,
1,
"both passes under the held lock reuse ONE live session",
);
});
test("destroyAllSessions tears down every cached session", async () => {
await acquireCollabSession("page-1", "tok", "http://h/api");
await acquireCollabSession("page-2", "tok", "http://h/api");
assert.equal(__sessionCountForTests(), 2);
const provs = [...FakeProvider.instances];
destroyAllSessions();
assert.equal(__sessionCountForTests(), 0);
assert.ok(provs.every((p) => p.destroyed), "all providers destroyed");
});
@@ -8,6 +8,7 @@ import {
applyAnchorInDoc,
countAnchorMatches,
getAnchoredText,
resolveAnchorSelection,
} from "../../build/lib/comment-anchor.js";
const COMMENT_ID = "cmt-123";
@@ -308,3 +309,70 @@ test("getAnchoredText returns null when the selection does not anchor", () => {
const doc = paragraphDoc([{ type: "text", text: "hello world" }]);
assert.equal(getAnchoredText(doc, "not present"), null);
});
// ---------------------------------------------------------------------------
// #408 MARKDOWN-STRIP FALLBACK. A selection copied with inline markdown still
// carries `**`/`` ` ``/`[t](u)` markers the plain document text lacks. When the
// verbatim selection anchors nowhere, all four entry points retry with the
// markdown stripped — consistently, so the suggestion-uniqueness gate stays
// coherent — while what gets STORED remains the raw document substring.
// ---------------------------------------------------------------------------
test("a markdown-styled selection anchors against plain doc text via the strip fallback", () => {
const doc = paragraphDoc([{ type: "text", text: "a bold word here" }]);
// The agent quoted "**bold** word" from a styled view; the doc is plain text.
const sel = "**bold** word";
const resolved = resolveAnchorSelection(doc, sel);
assert.equal(resolved.found, true, "strip fallback finds the anchor");
assert.equal(resolved.normalized, true, "reports the soft-warning flag");
assert.equal(canAnchorInDoc(doc, sel), true);
assert.equal(countAnchorMatches(doc, sel), 1);
const ok = applyAnchorInDoc(doc, sel, COMMENT_ID);
assert.equal(ok, true);
const marked = doc.content[0].content.filter((p) => commentMark(p));
assert.equal(marked.map((m) => m.text).join(""), "bold word",
"the mark lands on the plain-text span");
});
test("getAnchoredText stores the RAW doc substring even when matched via the strip fallback", () => {
// Doc uses a smart apostrophe; the agent typed ASCII + markdown emphasis.
const doc = paragraphDoc([{ type: "text", text: "it’s bold now" }]);
const stored = getAnchoredText(doc, "it's **bold**");
assert.equal(stored, "it’s bold",
"stored selection is the raw document text, not the stripped/ASCII locator");
});
test("the strip fallback does not flip a raw-unique selection to ambiguous", () => {
// "config" appears twice, but the raw phrase "config value" appears once.
const doc = {
type: "doc",
content: [
{ type: "paragraph", content: [{ type: "text", text: "the config value here" }] },
{ type: "paragraph", content: [{ type: "text", text: "another config here" }] },
],
};
// Raw phrase is unique -> exactly 1, and no strip happens (nothing to strip).
assert.equal(countAnchorMatches(doc, "config value"), 1);
assert.equal(resolveAnchorSelection(doc, "config value").normalized, false);
});
test("EXACT WINS: a raw match short-circuits the strip fallback (count reflects raw)", () => {
// A literal "**" run exists raw once; its stripped form would also appear.
const doc = paragraphDoc([{ type: "text", text: "use **stars** and stars" }]);
// Raw "**stars**" occurs once -> count 1 from the verbatim locator; the
// fallback (which would find two "stars") never runs.
assert.equal(countAnchorMatches(doc, "**stars**"), 1);
assert.equal(resolveAnchorSelection(doc, "**stars**").normalized, false);
});
test("a markdown selection whose stripped form is ambiguous is counted as ambiguous", () => {
const doc = {
type: "doc",
content: [
{ type: "paragraph", content: [{ type: "text", text: "first config here" }] },
{ type: "paragraph", content: [{ type: "text", text: "second config here" }] },
],
};
// Verbatim "**config**" matches nothing; stripped "config" matches twice.
assert.equal(countAnchorMatches(doc, "**config**"), 2);
});
+62
View File
@@ -0,0 +1,62 @@
diff --git a/dist/hocuspocus-server.cjs b/dist/hocuspocus-server.cjs
index b24ff6d091c32f733089eeaa47b03f7b37cf5964..f003af304fc751b7edc1aee17f3651282d70666a 100644
--- a/dist/hocuspocus-server.cjs
+++ b/dist/hocuspocus-server.cjs
@@ -2426,6 +2426,26 @@ class Hocuspocus {
* Create a new document by the given request
*/
async createDocument(documentName, request, socketId, connection, context) {
+ // PATCH(gitmost #401): close the connect-vs-unload race. When the last
+ // client disconnects, storeDocumentHooks' finally schedules an unload;
+ // unloadDocument runs its beforeUnloadDocument hooks asynchronously and
+ // records an in-flight promise in `unloadingDocuments`. A NEW connection
+ // arriving in that window would otherwise fall straight through to the
+ // `documents`/`loadingDocuments` checks and could either reuse a doc that
+ // is about to be destroyed, or start loading a fresh doc concurrently
+ // with the destroy. Awaiting the in-flight unload first makes the decision
+ // deterministic: once it settles, either the doc was fully unloaded
+ // (removed from `documents`, so we do a clean fresh load below) or the
+ // unload aborted because work/connections reappeared (the healthy doc is
+ // still in `documents`, so we reuse it). Either way the new connection can
+ // never hand-shake onto an orphaned, about-to-be-destroyed Document.
+ const existingUnloadingDoc = this.unloadingDocuments.get(documentName);
+ if (existingUnloadingDoc) {
+ // Wait for the in-flight unload to settle so we never hand-shake onto a dying
+ // Document. Swallow a rejected unload — fall through to a fresh load, matching
+ // pre-patch behavior (the doc is already removed from `documents` by then).
+ try { await existingUnloadingDoc; } catch { /* unload rejected — fresh load */ }
+ }
const existingLoadingDoc = this.loadingDocuments.get(documentName);
if (existingLoadingDoc) {
return existingLoadingDoc;
diff --git a/dist/hocuspocus-server.esm.js b/dist/hocuspocus-server.esm.js
index 1f4dd80244e899128e2c4e5dad8eab7cfc1cbad6..8c2411747bba27fb9486e1df81678a14e41e884e 100644
--- a/dist/hocuspocus-server.esm.js
+++ b/dist/hocuspocus-server.esm.js
@@ -2406,6 +2406,26 @@ class Hocuspocus {
* Create a new document by the given request
*/
async createDocument(documentName, request, socketId, connection, context) {
+ // PATCH(gitmost #401): close the connect-vs-unload race. When the last
+ // client disconnects, storeDocumentHooks' finally schedules an unload;
+ // unloadDocument runs its beforeUnloadDocument hooks asynchronously and
+ // records an in-flight promise in `unloadingDocuments`. A NEW connection
+ // arriving in that window would otherwise fall straight through to the
+ // `documents`/`loadingDocuments` checks and could either reuse a doc that
+ // is about to be destroyed, or start loading a fresh doc concurrently
+ // with the destroy. Awaiting the in-flight unload first makes the decision
+ // deterministic: once it settles, either the doc was fully unloaded
+ // (removed from `documents`, so we do a clean fresh load below) or the
+ // unload aborted because work/connections reappeared (the healthy doc is
+ // still in `documents`, so we reuse it). Either way the new connection can
+ // never hand-shake onto an orphaned, about-to-be-destroyed Document.
+ const existingUnloadingDoc = this.unloadingDocuments.get(documentName);
+ if (existingUnloadingDoc) {
+ // Wait for the in-flight unload to settle so we never hand-shake onto a dying
+ // Document. Swallow a rejected unload — fall through to a fresh load, matching
+ // pre-patch behavior (the doc is already removed from `documents` by then).
+ try { await existingUnloadingDoc; } catch { /* unload rejected — fresh load */ }
+ }
const existingLoadingDoc = this.loadingDocuments.get(documentName);
if (existingLoadingDoc) {
return existingLoadingDoc;
+5 -2
View File
@@ -44,6 +44,9 @@ overrides:
ip-address: 10.1.1
patchedDependencies:
'@hocuspocus/server@3.4.4':
hash: d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42
path: patches/@hocuspocus__server@3.4.4.patch
ai@6.0.134:
hash: f60bfc3357e01e1f3978c6c40fdd65aeb33fefaad7179cde8676465b6c5ff4d9
path: patches/ai@6.0.134.patch
@@ -75,7 +78,7 @@ importers:
version: 3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
'@hocuspocus/server':
specifier: 3.4.4
version: 3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
version: 3.4.4(patch_hash=d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
'@hocuspocus/transformer':
specifier: 3.4.4
version: 3.4.4(@tiptap/core@3.20.4(@tiptap/pm@3.20.4))(@tiptap/pm@3.20.4)(y-prosemirror@1.3.7(prosemirror-model@1.25.1)(prosemirror-state@1.4.3)(prosemirror-view@1.40.0)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))
@@ -13058,7 +13061,7 @@ snapshots:
- bufferutil
- utf-8-validate
'@hocuspocus/server@3.4.4(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))':
'@hocuspocus/server@3.4.4(patch_hash=d8dc66e5ec3b9d23a876b979f493b6aa901fd2d965be54729495da5136296a42)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810))':
dependencies:
'@hocuspocus/common': 3.4.4
async-lock: 1.4.1