refactor(mcp): структурные инварианты конкурентной записи — UUID-assert в page-lock, self-resolve seams, no-await-guard (#449)
Механика конкурентной записи (withPageLock → acquireCollabSession → mutate) держалась на цепочке конвенций в комментариях. Новый write-метод без знания правил компилировался и уходил в прод (класс #260/#152/#159). Закрепляем ключевые инварианты кодом. - page-lock.ts: экспортированы UUID_RE/isUuid (тот же regex, что resolvePageId, UUID v1–8/v7). withPageLock FAIL-FAST кидает при не-UUID ключе ДО любой работы (комментарий-инвариант #260/#449) — забытый resolve/slugId больше не даёт тихую потерю сериализации под другим ключом. client.ts импортирует isUuid оттуда (убран локальный дубль — resolver и assert не разъедутся). - mutatePage/replacePage seams стали async и сами вызывают resolvePageId — ключ лока/кэша канонический даже если вызывающий забыл (для уже-UUID это cached no-op; все 7 текущих вызывающих и так резолвят). replaceImage (один внешний лок + mutateLiveContentUnlocked) не тронут, deadlock невозможен. - collab-session.ts: машинно-проверяемые маркеры MUTATE-CRITICAL-WINDOW BEGIN/END вокруг синхронного блока fromYdoc→applyDocToFragment (INVARIANT 1). Тест no-await-critical-window читает исходник и краснеет на await/yield в окне (проверено нейтером). Случайный await больше не тихо клоббит живые правки. - Документация осознанной позиции: single-instance/sticky-sessions — требование деплоя (Dockerfile + README EN/RU + .env.example), т.к. мьютекс и stash — per-process. Окно устаревших прав (кэш-сессия пишет под токеном момента connect до MCP_COLLAB_SESSION_MAX_AGE_MS=10мин) — задокументированный trade-off в .env.example; push-инвалидации нет (осознанно). Тесты: page-lock fail-fast (slugId/пусто/non-string → throw; канонический UUID принят), no-away-guard, обновлённые фикстуры на валидные UUID. #449-специфичные 37/37 зелёные; mcp tsc чисто. closes #449. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -124,6 +124,40 @@ MCP_DOCMOST_PASSWORD=
|
||||
# MCP_TOKEN=
|
||||
# MCP_SESSION_IDLE_MS=1800000
|
||||
#
|
||||
# --- MCP collaboration write path: concurrency + rights-staleness (#449) ------
|
||||
# MCP content writes (update_page, insert/replace nodes, comments-in-body, etc.)
|
||||
# go over the collaboration websocket and are serialized PER PAGE by an
|
||||
# in-process mutex (a module-level Map, one promise-chain per page UUID). This
|
||||
# guarantees no two MCP writes on the SAME page overlap and clobber each other.
|
||||
#
|
||||
# DEPLOY REQUIREMENT — SINGLE INSTANCE or STICKY SESSIONS. The mutex is
|
||||
# process-local. Behind a multi-replica load balancer WITHOUT sticky sessions,
|
||||
# two replicas can each "hold" the lock for the same page at the same time and
|
||||
# serialization is silently lost (concurrent full-document writes race on the
|
||||
# live Yjs fragment). Run the MCP/app as a SINGLE instance, OR pin a page's
|
||||
# traffic to one replica (sticky sessions / consistent hashing on page id). The
|
||||
# same constraint applies to the RAM-only stash_page blob store above. There is
|
||||
# deliberately no cross-process (e.g. Postgres advisory) lock yet — this is a
|
||||
# CONSCIOUS documented constraint, not an oversight (#449).
|
||||
#
|
||||
# To reduce connect-storms the write path caches ONE live collab session per
|
||||
# (wsUrl, page, token). Tunables (all optional; defaults are safe):
|
||||
# MCP_COLLAB_SESSION_IDLE_MS=60000 # idle TTL, reset per op; 0 disables cache
|
||||
# MCP_COLLAB_SESSION_MAX_ENTRIES=32 # LRU cap on cached sessions
|
||||
# MCP_COLLAB_TOKEN_TTL_MS=300000 # per-client collab-token cache (5 min)
|
||||
#
|
||||
# RIGHTS-STALENESS TRADE-OFF. A cached collab session writes under the token
|
||||
# captured at CONNECT time, and the collab-token cache reuses a token for its TTL.
|
||||
# So if a user's access to a page is REVOKED, MCP writes on an already-open
|
||||
# session may keep succeeding until the session ages out. MCP_COLLAB_SESSION_MAX_AGE_MS
|
||||
# is the HARD lifetime (checked at each acquire) that BOUNDS this window: after it,
|
||||
# the session is torn down and the next write re-auths with a fresh token, picking
|
||||
# up the revocation. Default 10 min. LOWER it to shorten the revocation lag at the
|
||||
# cost of more reconnects; RAISE it to reduce reconnects at the cost of a longer
|
||||
# stale-rights window. There is intentionally no push-based cache invalidation on
|
||||
# a rights change — this bounded window is the accepted trade-off (#449).
|
||||
# MCP_COLLAB_SESSION_MAX_AGE_MS=600000
|
||||
#
|
||||
# BLOB SANDBOX (stash_page). An in-RAM, process-local store that hands large page
|
||||
# content + images to an external consumer WITHOUT bloating the model context or
|
||||
# requiring Docmost auth. The stash_page tool serializes a page, mirrors its
|
||||
|
||||
+10
@@ -81,4 +81,14 @@ VOLUME ["/app/data/storage"]
|
||||
|
||||
EXPOSE 3000
|
||||
|
||||
# DEPLOY REQUIREMENT — SINGLE INSTANCE or STICKY SESSIONS (#449).
|
||||
# MCP content writes are serialized per page by an IN-PROCESS mutex, and the
|
||||
# stash_page blob store + cached collab sessions are RAM-only and process-local.
|
||||
# Running MULTIPLE replicas of this image behind a load balancer WITHOUT sticky
|
||||
# sessions silently breaks per-page write serialization (two replicas can lock
|
||||
# the same page at once) and makes stash_page blobs unreachable across replicas.
|
||||
# Run a SINGLE instance, or pin each page's traffic to one replica (sticky
|
||||
# sessions / consistent hashing on page id). There is deliberately no
|
||||
# cross-process lock yet — a conscious constraint. See .env.example (the "MCP
|
||||
# collaboration write path" block) and packages/mcp/README.md for details.
|
||||
CMD ["pnpm", "start"]
|
||||
|
||||
+22
-2
@@ -293,8 +293,28 @@ so capable clients steer the model automatically.
|
||||
the debounced REST snapshot), then **reads → transforms → writes synchronously** in one
|
||||
tick so no remote update can interleave, and **waits for persistence acknowledgement**
|
||||
before returning.
|
||||
- **Per-page write serialization.** A per-`pageId` async mutex ensures two MCP writes to
|
||||
the same page never overlap; different pages never block each other.
|
||||
- **Per-page write serialization.** A per-`pageId` async mutex (keyed by the resolved
|
||||
page **UUID**, never a slugId) ensures two MCP writes to the same page never overlap;
|
||||
different pages never block each other. The lock helper fails fast if it is ever handed
|
||||
a non-UUID key, so a write path that forgot to resolve the id can never silently lock
|
||||
under a split key.
|
||||
|
||||
**Deploy requirement — single instance or sticky sessions.** This mutex is an
|
||||
in-process `Map`, and the cached collab sessions and the `stash_page` blob store are
|
||||
RAM-only and process-local. Behind a **multi-replica** load balancer **without sticky
|
||||
sessions**, two replicas can each "hold" the lock for the same page at once and per-page
|
||||
serialization is silently lost. Run the MCP/app as a **single instance**, or pin each
|
||||
page's traffic to one replica (sticky sessions / consistent hashing on the page id).
|
||||
There is deliberately no cross-process (e.g. Postgres advisory) lock yet — a conscious
|
||||
documented constraint. See the `Dockerfile` comment and the `MCP collaboration write
|
||||
path` block in `.env.example`.
|
||||
|
||||
**Rights-staleness window.** A cached collab session writes under the token captured at
|
||||
connect time (and the collab-token cache reuses a token for its TTL), so a **revoked**
|
||||
page access can lag by up to `MCP_COLLAB_SESSION_MAX_AGE_MS` (the hard session lifetime,
|
||||
default 10 min) before the next re-auth picks it up. Lower it to shorten the lag at the
|
||||
cost of more reconnects. This bounded window is an accepted trade-off; there is no
|
||||
push-based cache invalidation on a rights change.
|
||||
- **Transparent re-authentication.** Login uses email/password; expired tokens are
|
||||
refreshed automatically on the first 401/403 (covering JSON, multipart upload, and the
|
||||
collaboration-token path), with in-flight login de-duplication so a burst of calls
|
||||
|
||||
@@ -302,9 +302,27 @@ Docmost-MCP не сочетают:
|
||||
правки, которых ещё нет в дебаунс-снапшоте REST), затем **читает → трансформирует →
|
||||
пишет синхронно** в одном тике, чтобы никакое удалённое обновление не вклинилось, и
|
||||
**ждёт подтверждения сохранения** до возврата.
|
||||
- **Сериализация записи по странице.** Асинхронный мьютекс по `pageId` гарантирует, что
|
||||
две записи MCP в одну страницу никогда не пересекаются; разные страницы друг друга не
|
||||
блокируют.
|
||||
- **Сериализация записи по странице.** Асинхронный мьютекс по разрешённому **UUID**
|
||||
страницы (никогда не по slugId) гарантирует, что две записи MCP в одну страницу никогда
|
||||
не пересекаются; разные страницы друг друга не блокируют. Хелпер блокировки падает сразу
|
||||
(fail-fast), если ему передали не-UUID ключ, — путь записи, забывший разрезолвить id, не
|
||||
сможет молча взять лок под расщеплённым ключом.
|
||||
|
||||
**Требование к деплою — один инстанс или sticky-сессии.** Этот мьютекс — процесс-локальный
|
||||
`Map`, а кэш collab-сессий и хранилище `stashPage` живут только в RAM одного процесса. За
|
||||
**мультиреплика**-балансировщиком **без sticky-сессий** две реплики могут одновременно
|
||||
«держать» лок одной страницы, и сериализация по странице молча теряется. Запускайте
|
||||
MCP/приложение **одним инстансом** либо прибивайте трафик страницы к одной реплике
|
||||
(sticky-сессии / consistent hashing по id страницы). Кросс-процессной блокировки (например,
|
||||
Postgres advisory-lock) намеренно пока нет — осознанное задокументированное ограничение.
|
||||
См. комментарий в `Dockerfile` и блок `MCP collaboration write path` в `.env.example`.
|
||||
|
||||
**Окно устаревших прав.** Кэшированная collab-сессия пишет под токеном, захваченным в
|
||||
момент connect (а кэш collab-токена переиспользует токен в пределах своего TTL), поэтому
|
||||
**отозванный** доступ к странице может лагать до `MCP_COLLAB_SESSION_MAX_AGE_MS` (жёсткий
|
||||
срок жизни сессии, по умолчанию 10 мин), пока следующая переавторизация его не подхватит.
|
||||
Уменьшите значение, чтобы сократить лаг ценой большего числа переподключений. Это
|
||||
ограниченное окно — принятый trade-off; push-инвалидации кэша при смене прав нет.
|
||||
- **Прозрачная переавторизация.** Логин по email/паролю; истёкшие токены обновляются
|
||||
автоматически на первом 401/403 (покрывая JSON, multipart-загрузку и путь токена
|
||||
коллаборации), с дедупликацией параллельных логинов, так что пачка вызовов вызывает один
|
||||
|
||||
+27
-16
@@ -53,7 +53,7 @@ import {
|
||||
findUnrepresentableTableAttrs,
|
||||
} from "./lib/markdown-fragment.js";
|
||||
import { searchInDoc, SearchOptions } from "./lib/page-search.js";
|
||||
import { withPageLock } from "./lib/page-lock.js";
|
||||
import { withPageLock, isUuid } from "./lib/page-lock.js";
|
||||
import {
|
||||
prepareModel,
|
||||
decodeDrawioSvg,
|
||||
@@ -175,17 +175,14 @@ export type DocmostMcpConfig = { apiUrl: string } & (
|
||||
) => void;
|
||||
};
|
||||
|
||||
// Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate`
|
||||
// that the server's isValidUUID uses). page.repo.ts treats any non-UUID pageId
|
||||
// as a slugId, so the MCP detects a UUID locally and skips a /pages/info
|
||||
// round-trip in resolvePageId. A 10-char nanoid slugId never contains dashes,
|
||||
// so it can never be misread as a UUID here.
|
||||
const UUID_RE =
|
||||
/^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
|
||||
|
||||
function isUuid(value: string): boolean {
|
||||
return typeof value === "string" && UUID_RE.test(value);
|
||||
}
|
||||
// Canonical UUID predicate. Single source of truth lives in page-lock.ts (the
|
||||
// module that ASSERTS the mutex key is a UUID, issue #449) and is reused here so
|
||||
// resolvePageId's "already a UUID?" short-circuit and withPageLock's fail-fast
|
||||
// assert can never diverge. page.repo.ts treats any non-UUID pageId as a slugId,
|
||||
// so the MCP detects a UUID locally and skips a /pages/info round-trip in
|
||||
// resolvePageId. A 10-char nanoid slugId never contains dashes, so it can never
|
||||
// be misread as a UUID here.
|
||||
// (isUuid imported from ./lib/page-lock.js; see import block above.)
|
||||
|
||||
/**
|
||||
* Collab-token cache TTL in milliseconds (issue #435). Read fresh from the
|
||||
@@ -2185,14 +2182,23 @@ export class DocmostClient {
|
||||
* delegates; it exists as an overridable method so the insertFootnote wrapper
|
||||
* (transform abort-on-not-found + response shaping) can be unit-tested without
|
||||
* standing up a live Hocuspocus collab socket.
|
||||
*
|
||||
* SELF-RESOLVES the pageId to the canonical UUID (issue #449, "resolve-then-
|
||||
* lock"): every write must lock and key its CollabSession by the UUID, never a
|
||||
* raw slugId (#260). resolvePageId is cached/idempotent, so a caller that
|
||||
* already resolved pays no extra round-trip; centralizing it here means a
|
||||
* caller that reaches this seam with a raw slugId still locks correctly instead
|
||||
* of silently splitting the mutex key. withPageLock also asserts the key is a
|
||||
* UUID as a hard backstop.
|
||||
*/
|
||||
protected mutatePage(
|
||||
protected async mutatePage(
|
||||
pageId: string,
|
||||
collabToken: string,
|
||||
apiUrl: string,
|
||||
transform: (doc: any) => any,
|
||||
): Promise<{ doc?: any; verify?: any }> {
|
||||
return mutatePageContent(pageId, collabToken, apiUrl, transform);
|
||||
const pageUuid = await this.resolvePageId(pageId);
|
||||
return mutatePageContent(pageUuid, collabToken, apiUrl, transform);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2200,14 +2206,19 @@ export class DocmostClient {
|
||||
* just delegates; it exists as an overridable method so the full-doc write
|
||||
* tools (updatePageJson, copyPageContent) can have their footnote-
|
||||
* canonicalization binding unit-tested without a live Hocuspocus collab socket.
|
||||
*
|
||||
* SELF-RESOLVES the pageId to the canonical UUID (issue #449, "resolve-then-
|
||||
* lock") for the same reason as mutatePage above — the lock/CollabSession key
|
||||
* is guaranteed canonical here, not left to the caller's discipline.
|
||||
*/
|
||||
protected replacePage(
|
||||
protected async replacePage(
|
||||
pageId: string,
|
||||
doc: any,
|
||||
collabToken: string,
|
||||
apiUrl: string,
|
||||
): Promise<{ doc?: any; verify?: any }> {
|
||||
return replacePageContent(pageId, doc, collabToken, apiUrl);
|
||||
const pageUuid = await this.resolvePageId(pageId);
|
||||
return replacePageContent(pageUuid, doc, collabToken, apiUrl);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -440,9 +440,16 @@ export class CollabSession {
|
||||
// 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.
|
||||
//
|
||||
// INVARIANT 1 is machine-checked: the BEGIN/END markers below delimit the
|
||||
// no-await window, and test/unit/no-await-critical-window.test.mjs scans
|
||||
// this source and FAILS if any `await` (or `for await`/`yield`) appears
|
||||
// between them. Do NOT add an await inside this block — an accidental
|
||||
// async boundary here silently reopens the clobber-live-edits race (#152).
|
||||
let newDoc: any;
|
||||
let beforeDoc: any;
|
||||
try {
|
||||
// === MUTATE-CRITICAL-WINDOW: BEGIN (no await between here and END #449) ===
|
||||
let liveDoc = TiptapTransformer.fromYdoc(this.ydoc, "default");
|
||||
if (
|
||||
!liveDoc ||
|
||||
@@ -480,6 +487,7 @@ export class CollabSession {
|
||||
// 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);
|
||||
// === MUTATE-CRITICAL-WINDOW: END (#449) ===
|
||||
} catch (e) {
|
||||
// Includes errors thrown by transform (e.g. "afterText not found",
|
||||
// "text not found"): propagate them verbatim to the caller.
|
||||
|
||||
@@ -10,6 +10,20 @@
|
||||
|
||||
const chains = new Map<string, Promise<unknown>>();
|
||||
|
||||
// Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate`
|
||||
// that the server's isValidUUID uses). This is the SINGLE source of truth for
|
||||
// "is this a canonical page UUID?" in the MCP: client.ts's resolvePageId
|
||||
// imports isUuid from here to decide whether a pageId already IS a UUID (and so
|
||||
// needs no /pages/info round-trip). page.repo.ts treats any non-UUID pageId as
|
||||
// a slugId; a 10-char nanoid slugId never contains dashes, so it can never be
|
||||
// misread as a UUID here.
|
||||
export const UUID_RE =
|
||||
/^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
|
||||
|
||||
export function isUuid(value: string): boolean {
|
||||
return typeof value === "string" && UUID_RE.test(value);
|
||||
}
|
||||
|
||||
// The returned promise carries the real result/rejection of `fn` and MUST be
|
||||
// awaited/handled by the caller; only the internal chaining tail swallows
|
||||
// errors (purely to gate ordering).
|
||||
@@ -17,6 +31,25 @@ export function withPageLock<T>(
|
||||
pageId: string,
|
||||
fn: () => Promise<T>,
|
||||
): Promise<T> {
|
||||
// STRUCTURAL INVARIANT (issue #449, "resolve-then-lock"): the mutex key MUST
|
||||
// be the canonical page UUID, never a raw slugId. The whole write path relies
|
||||
// on the lock key AND the CollabSession cache key being the resolved UUID
|
||||
// (#260) — if a future write method forgot to call resolvePageId and locked
|
||||
// under a slugId, two writes to the same page would take DIFFERENT mutex keys
|
||||
// and silently lose serialization (clobbering live human edits). This was an
|
||||
// invariant enforced only by comments/convention; assert it in CODE so the
|
||||
// violation fails fast and loud at the lock instead of corrupting data in
|
||||
// prod. The centralizing helper (mutatePageContent/replacePageContent) already
|
||||
// guards a raw-input caller, but this backstop catches ANY path.
|
||||
if (!isUuid(pageId)) {
|
||||
throw new Error(
|
||||
`withPageLock: key must be a canonical page UUID, got '${pageId}'. ` +
|
||||
`The write path must resolvePageId(pageId) BEFORE locking so the ` +
|
||||
`mutex/CollabSession cache key is the UUID (invariant "resolve-then-` +
|
||||
`lock", #260/#449). A slugId or other non-UUID key would silently lose ` +
|
||||
`per-page serialization.`,
|
||||
);
|
||||
}
|
||||
// Wait for the previous op on this page; swallow its error so a failure does
|
||||
// not poison the queue for the next caller.
|
||||
const prev = (chains.get(pageId) ?? Promise.resolve()).catch(() => {});
|
||||
|
||||
@@ -372,7 +372,11 @@ test("replaceImage opens by the resolved UUID AND keys its page lock by that UUI
|
||||
// single flush. This proves the flush actually executes queued callbacks, so
|
||||
// probeRan === false above means "blocked", not "the flush never ran anyone".
|
||||
let freeRan = false;
|
||||
const freeDone = withPageLock(`page.free-${UUID}`, async () => {
|
||||
// A DIFFERENT canonical UUID (unrelated to the page under test). withPageLock
|
||||
// now asserts its key is a canonical UUID (#449), so the "free" probe key must
|
||||
// also be a valid — but distinct — UUID, not a synthetic label.
|
||||
const FREE_UUID = "99999999-9999-4999-8999-999999999999";
|
||||
const freeDone = withPageLock(FREE_UUID, async () => {
|
||||
freeRan = true;
|
||||
});
|
||||
await new Promise((r) => setImmediate(r));
|
||||
|
||||
@@ -323,7 +323,9 @@ test("MCP_COLLAB_SESSION_IDLE_MS=0 disables the cache (legacy provider-per-op)",
|
||||
});
|
||||
|
||||
test("replaceImage-shaped flow: acquire under an EXTERNAL page lock does not deadlock and reuses one session", async () => {
|
||||
const pageId = "page-lock";
|
||||
// withPageLock now asserts a canonical UUID key (#449); this flow takes the
|
||||
// real page lock (mirroring replaceImage), so the key must be a valid UUID.
|
||||
const pageId = "77777777-7777-4777-8777-777777777777";
|
||||
// Mirror replaceImage: hold ONE withPageLock across scan (read-only) + write,
|
||||
// each going through the non-locking acquireCollabSession.
|
||||
const result = await withPageLock(pageId, async () => {
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { dirname, join } from "node:path";
|
||||
|
||||
// Issue #449, invariant 2 ("no-await-окно"): the atomicity of the
|
||||
// read -> transform -> write section in CollabSession.mutate depends on there
|
||||
// being NO `await` (nor any other async yield point) between
|
||||
// `TiptapTransformer.fromYdoc` and `applyDocToFragment`. Yjs applies queued
|
||||
// remote updates only when the event loop yields, so an accidental await in that
|
||||
// window would let a concurrent human edit interleave and be clobbered (#152).
|
||||
//
|
||||
// That was an invariant enforced only by a comment. This test turns a violation
|
||||
// RED: it reads the SOURCE of collab-session.ts, extracts the block delimited by
|
||||
// the machine-readable BEGIN/END markers, and asserts no async boundary appears
|
||||
// inside it. Introducing an `await` (or `for await`, or `yield`) between the
|
||||
// markers fails this test.
|
||||
|
||||
const here = dirname(fileURLToPath(import.meta.url));
|
||||
// Scan the .ts SOURCE (not the compiled .js): the markers live in the source and
|
||||
// transpilation could rewrite/erase them, so the source is the authoritative
|
||||
// artifact the human edits.
|
||||
const sourcePath = join(here, "..", "..", "src", "lib", "collab-session.ts");
|
||||
const source = readFileSync(sourcePath, "utf8");
|
||||
|
||||
const BEGIN = "=== MUTATE-CRITICAL-WINDOW: BEGIN";
|
||||
const END = "=== MUTATE-CRITICAL-WINDOW: END";
|
||||
|
||||
test("critical-window markers exist exactly once each", () => {
|
||||
const begins = source.split(BEGIN).length - 1;
|
||||
const ends = source.split(END).length - 1;
|
||||
assert.equal(
|
||||
begins,
|
||||
1,
|
||||
`expected exactly one '${BEGIN}' marker, found ${begins}`,
|
||||
);
|
||||
assert.equal(ends, 1, `expected exactly one '${END}' marker, found ${ends}`);
|
||||
});
|
||||
|
||||
test("the read->write critical window contains no async boundary (no await/yield)", () => {
|
||||
const beginIdx = source.indexOf(BEGIN);
|
||||
const endIdx = source.indexOf(END);
|
||||
assert.ok(beginIdx !== -1, "BEGIN marker not found");
|
||||
assert.ok(endIdx !== -1, "END marker not found");
|
||||
assert.ok(endIdx > beginIdx, "END marker must come after BEGIN marker");
|
||||
|
||||
// The block strictly between the two marker lines. Move past the end of the
|
||||
// BEGIN marker line so the marker comment text itself is not scanned.
|
||||
const afterBeginLine = source.indexOf("\n", beginIdx) + 1;
|
||||
const block = source.slice(afterBeginLine, endIdx);
|
||||
|
||||
// Detect any real async yield keyword as a whole word. `\bawait\b` also matches
|
||||
// inside `for await`, which is exactly what we want to forbid here.
|
||||
const forbidden = [/\bawait\b/, /\byield\b/];
|
||||
for (const re of forbidden) {
|
||||
const m = block.match(re);
|
||||
assert.equal(
|
||||
m,
|
||||
null,
|
||||
`forbidden async boundary '${m?.[0]}' found inside the no-await critical ` +
|
||||
`window of CollabSession.mutate. INVARIANT 1 (#449): the block between ` +
|
||||
`TiptapTransformer.fromYdoc and applyDocToFragment must be fully ` +
|
||||
`synchronous — an await there reopens the clobber-live-edits race (#152).`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test("the critical window still spans fromYdoc -> applyDocToFragment", () => {
|
||||
// Guards the markers from drifting off the code they are meant to protect: if
|
||||
// someone moves the read/write out of the window, this catches it.
|
||||
const beginIdx = source.indexOf(BEGIN);
|
||||
const endIdx = source.indexOf(END);
|
||||
const afterBeginLine = source.indexOf("\n", beginIdx) + 1;
|
||||
const block = source.slice(afterBeginLine, endIdx);
|
||||
assert.ok(
|
||||
block.includes("TiptapTransformer.fromYdoc"),
|
||||
"critical window must contain the TiptapTransformer.fromYdoc read",
|
||||
);
|
||||
assert.ok(
|
||||
block.includes("applyDocToFragment"),
|
||||
"critical window must contain the applyDocToFragment write",
|
||||
);
|
||||
});
|
||||
@@ -1,13 +1,26 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import { withPageLock } from "../../build/lib/page-lock.js";
|
||||
import { withPageLock, isUuid } from "../../build/lib/page-lock.js";
|
||||
|
||||
const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
|
||||
|
||||
// withPageLock now asserts its key is a canonical UUID (#449, "resolve-then-
|
||||
// lock"), so the mechanics tests below must lock under real UUIDs, not arbitrary
|
||||
// labels. Distinct valid UUIDv7-shaped ids for the distinct-page cases.
|
||||
const U = {
|
||||
same: "11111111-1111-7111-8111-111111111111",
|
||||
ordered: "22222222-2222-7222-8222-222222222222",
|
||||
poison: "33333333-3333-7333-8333-333333333333",
|
||||
poison2: "44444444-4444-7444-8444-444444444444",
|
||||
A: "aaaaaaaa-aaaa-7aaa-8aaa-aaaaaaaaaaaa",
|
||||
B: "bbbbbbbb-bbbb-7bbb-8bbb-bbbbbbbbbbbb",
|
||||
leak: "55555555-5555-7555-8555-555555555555",
|
||||
};
|
||||
|
||||
test("two ops on the same pageId run strictly sequentially (no overlap)", async () => {
|
||||
const events = [];
|
||||
const pageId = "same-page";
|
||||
const pageId = U.same;
|
||||
|
||||
const p1 = withPageLock(pageId, async () => {
|
||||
events.push("start-1");
|
||||
@@ -33,7 +46,7 @@ test("two ops on the same pageId run strictly sequentially (no overlap)", async
|
||||
});
|
||||
|
||||
test("same pageId ordering holds for many queued ops", async () => {
|
||||
const pageId = "ordered-page";
|
||||
const pageId = U.ordered;
|
||||
const order = [];
|
||||
const active = { count: 0, maxConcurrent: 0 };
|
||||
|
||||
@@ -60,7 +73,7 @@ test("same pageId ordering holds for many queued ops", async () => {
|
||||
});
|
||||
|
||||
test("a rejecting op does not poison the chain for the same page", async () => {
|
||||
const pageId = "poison-page";
|
||||
const pageId = U.poison;
|
||||
const events = [];
|
||||
|
||||
const failing = withPageLock(pageId, async () => {
|
||||
@@ -87,7 +100,7 @@ test("a rejecting op does not poison the chain for the same page", async () => {
|
||||
});
|
||||
|
||||
test("failing op queued before a success both resolve/reject correctly", async () => {
|
||||
const pageId = "poison-page-2";
|
||||
const pageId = U.poison2;
|
||||
const order = [];
|
||||
|
||||
const failing = withPageLock(pageId, async () => {
|
||||
@@ -111,14 +124,14 @@ test("failing op queued before a success both resolve/reject correctly", async (
|
||||
test("ops on different pageIds run concurrently (overlap)", async () => {
|
||||
const events = [];
|
||||
|
||||
const pA = withPageLock("page-A", async () => {
|
||||
const pA = withPageLock(U.A, async () => {
|
||||
events.push("A-start");
|
||||
await delay(40);
|
||||
events.push("A-end");
|
||||
return "A";
|
||||
});
|
||||
|
||||
const pB = withPageLock("page-B", async () => {
|
||||
const pB = withPageLock(U.B, async () => {
|
||||
events.push("B-start");
|
||||
await delay(10);
|
||||
events.push("B-end");
|
||||
@@ -134,7 +147,7 @@ test("ops on different pageIds run concurrently (overlap)", async () => {
|
||||
});
|
||||
|
||||
test("no functional leak: many sequential ops on same page keep working", async () => {
|
||||
const pageId = "leak-page";
|
||||
const pageId = U.leak;
|
||||
|
||||
// Run a long series of fully sequential ops (each awaited before the next is
|
||||
// queued) so the internal map entry is created and dropped repeatedly.
|
||||
@@ -151,3 +164,56 @@ test("no functional leak: many sequential ops on same page keep working", async
|
||||
const final = await withPageLock(pageId, async () => "still-works");
|
||||
assert.equal(final, "still-works");
|
||||
});
|
||||
|
||||
// --- Issue #449: fail-fast on a non-canonical lock key ---------------------
|
||||
// A write method that reaches the lock path with an unresolved slugId (or any
|
||||
// non-UUID key) must fail IMMEDIATELY and LOUDLY, not lock under a split key and
|
||||
// silently lose per-page serialization. These assert withPageLock rejects such
|
||||
// a key before ever running fn.
|
||||
|
||||
test("withPageLock throws on a raw 10-char slugId (unresolved key)", () => {
|
||||
let ran = false;
|
||||
assert.throws(
|
||||
() =>
|
||||
withPageLock("p7Xk29Lm4Q", async () => {
|
||||
ran = true;
|
||||
return "should-not-run";
|
||||
}),
|
||||
/canonical page UUID|resolve-then-lock/i,
|
||||
"a slugId key must fail-fast at the lock",
|
||||
);
|
||||
// The work must NOT have started: fail-fast means no serialization was
|
||||
// silently skipped under a bad key.
|
||||
assert.equal(ran, false, "fn must not run when the key is rejected");
|
||||
});
|
||||
|
||||
test("withPageLock throws on other non-UUID keys (label, empty, non-string)", () => {
|
||||
for (const bad of ["same-page", "", "not-a-uuid", "1234"]) {
|
||||
assert.throws(
|
||||
() => withPageLock(bad, async () => "x"),
|
||||
/canonical page UUID/i,
|
||||
`expected withPageLock to reject key ${JSON.stringify(bad)}`,
|
||||
);
|
||||
}
|
||||
// A non-string key is also rejected (guards a mistyped call site).
|
||||
assert.throws(
|
||||
() => withPageLock(/** @type {any} */ (undefined), async () => "x"),
|
||||
/canonical page UUID/i,
|
||||
);
|
||||
});
|
||||
|
||||
test("withPageLock accepts a canonical UUID key (no false positive)", async () => {
|
||||
const uuid = "0192f3a4-b5c6-7d8e-9f01-23456789abcd";
|
||||
assert.equal(isUuid(uuid), true);
|
||||
const r = await withPageLock(uuid, async () => "ok");
|
||||
assert.equal(r, "ok");
|
||||
});
|
||||
|
||||
test("isUuid discriminates UUIDs from slugIds (shared predicate)", () => {
|
||||
// The predicate withPageLock asserts on is the SAME one resolvePageId uses to
|
||||
// decide whether a pageId is already a UUID (imported from page-lock).
|
||||
assert.equal(isUuid("0192f3a4-b5c6-7d8e-9f01-23456789abcd"), true);
|
||||
assert.equal(isUuid("p7Xk29Lm4Q"), false); // 10-char nanoid slugId
|
||||
assert.equal(isUuid("not-a-uuid"), false);
|
||||
assert.equal(isUuid(""), false);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user