From bc433d12e6db2c065011ea4089a8ee6feffcbe05 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 00:03:59 +0300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20#5?= =?UTF-8?q?13=20=E2=80=94=20=D0=B3=D0=BE=D0=BD=D0=BA=D0=B0=20LRU-=D1=8D?= =?UTF-8?q?=D0=B2=D0=B8=D0=BA=D1=86=D0=B8=D0=B8=20+=20=D0=B4=D0=BE=D0=B2?= =?UTF-8?q?=D0=BE=D0=B4=D0=BA=D0=B8=20=D0=BA=D0=BE=D0=BD=D1=82=D1=80=D0=B0?= =?UTF-8?q?=D0=BA=D1=82=D0=B0/=D0=B4=D0=BE=D0=BA=D0=BE=D0=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WARNING [stability]: эвикция могла убить ЧУЖУЮ ещё-connecting сессию как idle-жертву — isBusy()=false во время open(), сессия вставлена в мапу до await open(); параллельный acquire на ДРУГУЮ страницу при насыщении выселял её → спурьёзный reject не-начатой записи / старвейшн новых записей. Фикс: idle-victim скан теперь — connecting падает в last-resort oldestBusy, честный idle по-прежнему предпочитается, кап держится, коалесинг того же ключа (per-page lock) не задет. Тест на интерливинг (connecting не выселяется под насыщением), mutation-verified. Доводки (проза, логику не меняют): drawioCreate description (nodeId:null на nested-вставке); forward-коммент у writeWithCollabAuthRetry (ретрай только auth; при расширении — проверять isCollabIndeterminateError, #435); хедер comment-anchor (все 4 точки делегируют в resolveAnchorSelection); CHANGELOG. Ребейзнут на develop (волна 1 смержена): только 4 коммита #494. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 13 ++++ packages/mcp/src/client/context.ts | 7 ++ packages/mcp/src/lib/collab-session.ts | 24 +++++-- packages/mcp/src/lib/comment-anchor.ts | 16 ++--- packages/mcp/src/tool-specs.ts | 18 +++-- .../mcp/test/unit/collab-session.test.mjs | 68 +++++++++++++++++++ 6 files changed, 125 insertions(+), 21 deletions(-) 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");