diff --git a/CHANGELOG.md b/CHANGELOG.md index adecf01f..d55b008a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -336,6 +336,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **MCP write tools no longer report a false failure that provokes a duplicate + write.** `drawioCreate` used to throw when the diagram landed as a NESTED block + (anchored inside a callout or table cell) because there is no `#` handle + for it — but the diagram was already written, so a retry-prone agent re-created + it and produced a duplicate. It now returns success with `nodeId: null` plus a + warning that explains the write landed and how to re-read it (via + `getOutline` / `getPageJson` by `attachmentId`). Separately, when the live + collaboration-session cache hits its LRU entry cap, evicting a session whose + write is still in flight no longer rejects that write as a hard failure — it is + reported as INDETERMINATE ("the update may already have persisted; verify + before retry") so the agent re-reads instead of blind-retrying, and a + still-connecting session is no longer picked as an idle eviction victim by a + parallel acquire. (#494) - **A chat with one malformed message part no longer 500s on every turn, and a failed send no longer duplicates the user's message.** Incoming client parts are now whitelisted to `text` (a forged tool-result part can no longer reach diff --git a/packages/mcp/src/client/context.ts b/packages/mcp/src/client/context.ts index 8c784f53..a6e3518f 100644 --- a/packages/mcp/src/client/context.ts +++ b/packages/mcp/src/client/context.ts @@ -553,6 +553,13 @@ export abstract class DocmostClientContext { try { return await write(collabToken); } catch (e) { + // INVARIANT (#494/#435): this auto-retry MUST stay auth-only. A collab + // write can fail INDETERMINATE — its update may already have reached and + // persisted on the server (e.g. an LRU eviction of a busy session, tagged + // via isCollabIndeterminateError). Blindly retrying such a write duplicates + // it (the #435 double-apply class). If this gate is ever widened to retry a + // broader error class, FIRST check `isCollabIndeterminateError(e)` and do + // NOT retry an indeterminate write — re-read and verify before any retry. if (!isCollabAuthFailedError(e)) throw e; // The WS handshake rejected our token: drop it from the cache so it can't // be reused for the rest of the TTL, mint a fresh one (forceRefresh bypasses diff --git a/packages/mcp/src/lib/collab-session.ts b/packages/mcp/src/lib/collab-session.ts index 59f2ccc4..ae0153f5 100644 --- a/packages/mcp/src/lib/collab-session.ts +++ b/packages/mcp/src/lib/collab-session.ts @@ -732,20 +732,32 @@ export async function acquireCollabSession( // until there is room. PREFER an IDLE victim (#494): a session with an in-flight // mutate may have already sent (and persisted) its update, so evicting it would // reject that write as a FALSE failure → a retry-prone agent re-issues it → - // DUPLICATE write (the #435 class). So walk LRU order and skip busy sessions, - // evicting the oldest IDLE one. Only when EVERY cached session is busy (eviction - // unavoidable to admit this write) do we evict the LRU busy one — via - // evictForCap(), which rejects its in-flight op with a tagged INDETERMINATE + // DUPLICATE write (the #435 class). So walk LRU order and skip non-idle sessions, + // evicting the oldest IDLE one. Only when EVERY cached session is non-idle (eviction + // unavoidable to admit this write) do we evict the LRU non-idle one — via + // evictForCap(), which rejects an in-flight op with a tagged INDETERMINATE // "verify before retry" error rather than a plain failure. + // + // "Non-idle" = busy OR still opening. `sessions.set(key)` below runs BEFORE the + // `await session.open()` that resolves it, so a `connecting` session is in the + // map while its handshake is still in flight. Such a session is NOT busy yet + // (isBusy() needs an in-flight mutate), but it is ALSO not a legitimate idle + // victim: in multi-user HTTP two acquires for DIFFERENT pages interleave across + // that await, and under saturation the second acquire would otherwise pick the + // first's freshly-inserted `connecting` session as an "idle" victim and destroy + // it, rejecting the first's pending open() as "evicted (LRU cap)" — a spurious + // failure of a write that never even started. So exclude `state !== "ready"` + // from the idle scan; a connecting session falls into the last-resort bucket and + // is evicted ONLY when every other entry is busy-or-connecting too. while (sessions.size >= cfg.maxEntries) { let idleKey: string | undefined; let oldestBusyKey: string | undefined; for (const [k, s] of sessions) { - if (s.isBusy()) { + if (s.isBusy() || s.state !== "ready") { if (oldestBusyKey === undefined) oldestBusyKey = k; continue; } - idleKey = k; // first (LRU) idle session + idleKey = k; // first (LRU) genuinely-idle session break; } const victimKey = idleKey ?? oldestBusyKey; diff --git a/packages/mcp/src/lib/comment-anchor.ts b/packages/mcp/src/lib/comment-anchor.ts index 588c6b71..accf6c16 100644 --- a/packages/mcp/src/lib/comment-anchor.ts +++ b/packages/mcp/src/lib/comment-anchor.ts @@ -22,14 +22,14 @@ * inline markdown (`**bold**`, `` `code` ``, `[t](u)`), the raw locator will not * match the document's plain text. Exactly like editPageText'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. + * in the whole document, retry with `stripInlineMarkdown` applied. All four entry + * points — `canAnchorInDoc`, `getAnchoredText`, `applyAnchorInDoc` and + * `countAnchorMatches` — share this exact-wins / strip-fallback decision through the + * SINGLE resolver `resolveAnchorSelection`; there is no second copy of the control + * flow. `countAnchorMatches` just asks the resolver which selection form wins and + * returns the raw occurrence count of that winning form. Because count and anchor + * derive from the same resolver, the suggestion-uniqueness gate (which depends on + * count) can never disagree with what actually anchors. */ import { stripInlineMarkdown } from "./text-normalize.js"; diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 106602fa..f3f0a2fe 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -1963,13 +1963,17 @@ export const SHARED_TOOL_SPECS = { 'value escaping) — a violation returns a structured error naming the rule ' + 'and cellId so you can fix and retry. `where` positions the block like ' + 'insertNode: position before/after (with exactly one of anchorNodeId or ' + - 'anchorText) or append. Returns { nodeId, attachmentId, warnings }. The ' + - 'returned `nodeId` is an index-based "#" handle (drawio nodes carry ' + - 'no attrs.id): it addresses the new top-level block and can be fed straight ' + - 'back into drawioGet / drawioUpdate for THIS document. It is positional, ' + - 'so if you add or remove blocks before it, re-resolve via getOutline. The ' + - 'diagram is editable in the draw.io editor and can be re-read with ' + - 'drawioGet.' + + 'anchorText) or append. Returns { nodeId, attachmentId, warnings }. On a ' + + 'top-level insert the returned `nodeId` is an index-based "#" handle ' + + '(drawio nodes carry no attrs.id): it addresses the new block and can be fed ' + + 'straight back into drawioGet / drawioUpdate for THIS document. It is ' + + 'positional, so if you add or remove blocks before it, re-resolve via ' + + 'getOutline. When the block lands NESTED (e.g. anchored inside a callout or ' + + 'table cell), "#" cannot address it, so `nodeId` is `null` — the ' + + 'write STILL SUCCEEDED (success:true, a warning explains this); do NOT ' + + 're-create it. Re-read or edit that nested diagram by locating it via ' + + 'getOutline / getPageJson using the returned attachmentId. The diagram is ' + + 'editable in the draw.io editor and can be re-read with drawioGet.' + DRAWIO_HARD_RULES, tier: 'deferred', catalogLine: diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index b533f9b1..0dedbd6d 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -368,6 +368,74 @@ test("#494: evicting a busy session when all are busy rejects the write as INDET }); }); +// #494 — a session whose open() has NOT resolved yet (state "connecting") sits in +// the registry between `sessions.set(key)` and `await session.open()`, but it is +// NOT an idle eviction victim: its write has not even started, and a parallel +// acquire for a DIFFERENT page that interleaves across that await must not destroy +// it (which would reject its pending open() as "evicted (LRU cap)" — a spurious +// failure of a write that never began). Under saturation the connecting session is +// spared; a genuinely-busy LRU session is evicted (INDETERMINATE) instead, and the +// connecting session's open() still resolves. +test("#494: a still-CONNECTING session is NOT evicted as an idle victim by a parallel acquire under saturation", async () => { + process.env.MCP_COLLAB_SESSION_MAX_ENTRIES = "2"; + + // A = the OLDEST (LRU) session, made BUSY by an in-flight (un-acked) mutate. + __setCollabProviderFactory(factory({ unsynced: 1 })); + const sA = await acquireCollabSession("page-1", "tok", "http://h/api"); + const provA = FakeProvider.last(); + const inflightA = sA.mutate(() => docWith("in-flight write")); // pending + // Attach a handler NOW so a later reject is never "unhandled"; capture it. + let inflightErr; + inflightA.catch((e) => { + inflightErr = e; + }); + + // B = a session still mid-handshake: open() never resolves under autoSync:false, + // so it stays "connecting". acquireCollabSession runs synchronously through + // `sessions.set(key)` and into open()'s executor (provider created) before it + // suspends at `await session.open()`, so B is already registered here. + __setCollabProviderFactory(factory({ autoSync: false })); + const pB = acquireCollabSession("page-2", "tok", "http://h/api"); // NOT awaited + const provB = FakeProvider.last(); + assert.equal(__sessionCountForTests(), 2, "A (busy) + B (connecting) fill the cap"); + assert.notEqual(provA, provB); + + // C forces an eviction (cap 2). The idle-victim scan must treat B (connecting) as + // NON-idle and fall through to the busy LRU (A), evicting A as INDETERMINATE and + // SPARING the connecting B. + __setCollabProviderFactory(factory()); + const sC = await acquireCollabSession("page-3", "tok", "http://h/api"); + assert.equal(__sessionCountForTests(), 2); + assert.ok(sC); + + assert.equal( + provB.destroyed, + false, + "the CONNECTING session must be spared — a parallel acquire must not evict a mid-handshake write", + ); + assert.equal( + provA.destroyed, + true, + "the busy LRU session was the eviction victim instead", + ); + + // B's handshake completes -> its open() resolves and the acquire returns a live + // session (its write can now start), proving the eviction never touched it. + provB.config.onConnect?.(); + provB.synced = true; + provB.config.onSynced?.(); + const sB = await pB; + assert.ok(sB, "the spared connecting session's open() resolves normally"); + assert.equal(provB.destroyed, false); + + // The evicted busy write rejects as INDETERMINATE (verify-before-retry), not a + // plain failure — the existing all-busy guarantee still holds for A. + assert.ok( + isCollabIndeterminateError(inflightErr), + "the evicted busy write carries the INDETERMINATE marker", + ); +}); + 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");