diff --git a/.env.example b/.env.example index 1241ab18..0a032c26 100644 --- a/.env.example +++ b/.env.example @@ -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 diff --git a/Dockerfile b/Dockerfile index f3b1f646..a7d11dc8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -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"] diff --git a/packages/mcp/README.md b/packages/mcp/README.md index b16b8c29..e6c93341 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -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 diff --git a/packages/mcp/README.ru.md b/packages/mcp/README.ru.md index 8c124439..3b3de41d 100644 --- a/packages/mcp/README.ru.md +++ b/packages/mcp/README.ru.md @@ -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-загрузку и путь токена коллаборации), с дедупликацией параллельных логинов, так что пачка вызовов вызывает один diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index b7d80b8b..07212941 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -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); } /** diff --git a/packages/mcp/src/lib/collab-session.ts b/packages/mcp/src/lib/collab-session.ts index 9a4b7a6d..3dc16161 100644 --- a/packages/mcp/src/lib/collab-session.ts +++ b/packages/mcp/src/lib/collab-session.ts @@ -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. diff --git a/packages/mcp/src/lib/page-lock.ts b/packages/mcp/src/lib/page-lock.ts index 75b50c4a..c17033c8 100644 --- a/packages/mcp/src/lib/page-lock.ts +++ b/packages/mcp/src/lib/page-lock.ts @@ -10,6 +10,20 @@ const chains = new Map>(); +// 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( pageId: string, fn: () => Promise, ): Promise { + // 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(() => {}); diff --git a/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs b/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs index dddd581c..b40aaebe 100644 --- a/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs +++ b/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs @@ -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)); diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index c71ddb9b..c7009a69 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -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 () => { diff --git a/packages/mcp/test/unit/no-await-critical-window.test.mjs b/packages/mcp/test/unit/no-await-critical-window.test.mjs new file mode 100644 index 00000000..96bf5014 --- /dev/null +++ b/packages/mcp/test/unit/no-await-critical-window.test.mjs @@ -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", + ); +}); diff --git a/packages/mcp/test/unit/page-lock.test.mjs b/packages/mcp/test/unit/page-lock.test.mjs index 15e91ad2..438cd4a6 100644 --- a/packages/mcp/test/unit/page-lock.test.mjs +++ b/packages/mcp/test/unit/page-lock.test.mjs @@ -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); +});