From 7ed558e457992ddbd1952abd15daa2dd8d027cf0 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 18:52:04 +0300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20LRU-=D1=8D=D0=B2=D0=B8=D0=BA=D1=86?= =?UTF-8?q?=D0=B8=D1=8F=20=D0=BD=D0=B5=20=D1=80=D0=B5=D0=B4=D0=B6=D0=B5?= =?UTF-8?q?=D0=BA=D1=82=D0=B8=D1=82=20=D1=87=D1=83=D0=B6=D0=BE=D0=B9=20in-?= =?UTF-8?q?flight=20mutate=20=D0=BA=D0=B0=D0=BA=20failure=20(#494)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Коммит 4. При достижении cap реестра live-сессий эвиктился LRU-кандидат через destroy(), что реджектило его in-flight mutate терминальной ошибкой. Но update такого mutate мог УЖЕ дойти до сервера и персиститься → эвикция превращала удачную запись в ложный proval → retry-склонный агент повторял запись → ДУБЛИКАТ (тот же механизм, что в инциденте #435). Правка: - цикл эвикции теперь ПРЕДПОЧИТАЕТ idle-жертву: идёт по LRU-порядку, пропускает busy-сессии (isBusy() — есть in-flight mutate) и эвиктит старейшую idle; - если ВСЕ сессии busy (эвикция неизбежна для приёма новой записи) — эвиктит LRU busy через evictForCap(), который реджектит in-flight op ПОМЕЧЕННОЙ ошибкой INDETERMINATE «write may have applied — verify before retry», а не плоским failure. Маркер collabIndeterminate + guard isCollabIndeterminateError (симметрично isCollabAuthFailedError), чтобы «проверь перед ретраем» можно было отличить от чистого провала. Тесты (мутационные): busy LRU-сессия сохраняется, эвиктится younger idle, и её запись доезжает на ack; when-all-busy — эвикция реджектит запись именно INDETERMINATE-ошибкой с маркером и текстом verify-before-retry. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/lib/collab-session.ts | 98 +++++++++++++++++-- .../mcp/test/unit/collab-session.test.mjs | 61 ++++++++++++ 2 files changed, 151 insertions(+), 8 deletions(-) diff --git a/packages/mcp/src/lib/collab-session.ts b/packages/mcp/src/lib/collab-session.ts index 5e1e40cc..59f2ccc4 100644 --- a/packages/mcp/src/lib/collab-session.ts +++ b/packages/mcp/src/lib/collab-session.ts @@ -56,6 +56,40 @@ export function isCollabAuthFailedError(e: unknown): boolean { ); } +/** + * Marker set on the Error a still-in-flight mutate is rejected with when its + * session is LRU-evicted while busy (#494). Unlike a plain destroy/failure, this + * write is INDETERMINATE: the local update may already have been sent to (and + * persisted by) the server before the eviction, so a blind retry risks a DUPLICATE + * write (the #435 double-apply class). A tagged property (not a message match) + * lets a caller distinguish "verify before retry" from a clean failure. + */ +const COLLAB_INDETERMINATE_MARKER = "collabIndeterminate"; + +/** True when `e` is the tagged "write may have applied — verify before retry" + * error raised when a busy session is LRU-evicted (see marker above). */ +export function isCollabIndeterminateError(e: unknown): boolean { + return !!( + e && + typeof e === "object" && + (e as Record)[COLLAB_INDETERMINATE_MARKER] === true + ); +} + +/** Build the tagged INDETERMINATE error an in-flight mutate is rejected with when + * its session must be evicted while busy (#494). */ +function makeCollabIndeterminateError(pageId: string): Error { + const err = new Error( + `Collaboration write INDETERMINATE (pageId ${pageId}): the live session was ` + + `evicted under the LRU cap while this write was in flight, and its update ` + + `MAY already have reached and persisted on the server. Do NOT blindly ` + + `retry — re-read the page and verify whether the edit applied first (a ` + + `blind retry risks a duplicate write).`, + ) as Error & { [COLLAB_INDETERMINATE_MARKER]?: boolean }; + err[COLLAB_INDETERMINATE_MARKER] = true; + return err; +} + /** * Tunables, read fresh from the environment on every acquire so tests (and a * live rollback) can change them without reloading the module. Mirrors how @@ -592,6 +626,36 @@ export class CollabSession { } } + /** + * True while a mutate is in flight (an update may already be on the wire / + * persisted server-side). The LRU-eviction path (#494) uses this to AVOID + * evicting a session mid-write when an idle victim exists, and to tag the error + * as INDETERMINATE when evicting a busy one is unavoidable. + */ + isBusy(): boolean { + return !this.dead && this.inflightReject !== undefined; + } + + /** + * Evict this session for the LRU cap (#494). When a mutate is IN FLIGHT its + * update may already have reached the server, so rejecting it as a plain + * failure would make a retry-prone agent re-issue the write and DUPLICATE it + * (the #435 class). Reject the in-flight op with the tagged INDETERMINATE error + * (verify-before-retry) instead. When idle, this is an ordinary destroy. + */ + evictForCap(): void { + if (this.dead) return; + if (this.isBusy()) { + if (process.env.DEBUG) + console.error( + `Evicting BUSY collab session ${this.pageId} (LRU cap) — in-flight write is INDETERMINATE`, + ); + this.teardown(makeCollabIndeterminateError(this.pageId), false); + } else { + this.destroy("evicted (LRU cap)"); + } + } + /** * Public idempotent teardown used by the acquire/eviction paths and by a * caller that wants the session dropped after a failed op ("next call @@ -664,15 +728,33 @@ export async function acquireCollabSession( 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. + // Enforce the registry cap before inserting: evict least-recently-used entries + // 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 + // "verify before retry" error rather than a plain failure. 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); + let idleKey: string | undefined; + let oldestBusyKey: string | undefined; + for (const [k, s] of sessions) { + if (s.isBusy()) { + if (oldestBusyKey === undefined) oldestBusyKey = k; + continue; + } + idleKey = k; // first (LRU) idle session + break; + } + const victimKey = idleKey ?? oldestBusyKey; + if (victimKey === undefined) break; // registry empty (shouldn't happen) + // evictForCap() picks the plain-destroy vs INDETERMINATE-reject path itself + // based on whether the victim is busy. + sessions.get(victimKey)?.evictForCap(); + // teardown removes it from the map; guard against a no-op. + if (sessions.has(victimKey)) sessions.delete(victimKey); } const session = new CollabSession( diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index c7009a69..b533f9b1 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -5,6 +5,7 @@ import { EventEmitter } from "node:events"; import { acquireCollabSession, destroyAllSessions, + isCollabIndeterminateError, __setCollabProviderFactory, __sessionCountForTests, } from "../../build/lib/collab-session.js"; @@ -307,6 +308,66 @@ test("registry cap: least-recently-used session is destroy-evicted", async () => assert.equal(FakeProvider.connectCount, 3, "no extra reconnects"); }); +// #494 — the LRU cap must PREFER an idle victim: evicting a session with an +// in-flight mutate (whose update may already be on the server) would reject that +// write as a false failure → the agent retries → duplicate. Here the LRU (oldest) +// session is BUSY and a younger one is idle; the busy one must be spared. +test("#494: LRU eviction SKIPS a busy session and evicts a younger idle one instead", async () => { + process.env.MCP_COLLAB_SESSION_MAX_ENTRIES = "2"; + __setCollabProviderFactory(factory({ unsynced: 1 })); // a mutate stays pending + // page-1 is the OLDEST (LRU). Start a mutate on it so it is BUSY (in-flight). + const s1 = await acquireCollabSession("page-1", "tok", "http://h/api"); + const p1prov = FakeProvider.last(); + const inflight = s1.mutate(() => docWith("in-flight write")); // pending (no ack) + // page-2 is younger and IDLE. + const s2 = await acquireCollabSession("page-2", "tok", "http://h/api"); + const p2prov = FakeProvider.last(); + assert.equal(__sessionCountForTests(), 2); + + // page-3 forces an eviction (cap 2). The LRU is page-1, but it is BUSY, so the + // guard must skip it and evict the idle page-2 instead. + await acquireCollabSession("page-3", "tok", "http://h/api"); + assert.equal(__sessionCountForTests(), 2); + assert.equal( + p1prov.destroyed, + false, + "the BUSY LRU session must be spared (its in-flight write may have landed)", + ); + assert.equal(p2prov.destroyed, true, "the idle younger session was evicted"); + + // The spared write still completes normally once the server acks it — it was + // never rejected by the eviction. + p1prov._ack(); + const r = await inflight; + assert.ok(r.doc, "the in-flight write on the spared session resolves on its ack"); +}); + +// #494 — when EVERY cached session is busy, eviction is unavoidable; the victim's +// in-flight write must reject as INDETERMINATE (verify-before-retry), NOT a plain +// failure that invites a blind, duplicate retry. +test("#494: evicting a busy session when all are busy rejects the write as INDETERMINATE", async () => { + process.env.MCP_COLLAB_SESSION_MAX_ENTRIES = "1"; + __setCollabProviderFactory(factory({ unsynced: 1 })); + const s1 = await acquireCollabSession("page-1", "tok", "http://h/api"); + const inflight = s1.mutate(() => docWith("maybe-persisted write")); // pending + assert.equal(__sessionCountForTests(), 1); + + // page-2 needs the single slot; page-1 is the only (busy) candidate, so its + // eviction is unavoidable. Its in-flight write must reject with the tagged + // indeterminate error. + await acquireCollabSession("page-2", "tok", "http://h/api"); + + await assert.rejects(inflight, (err) => { + assert.ok( + isCollabIndeterminateError(err), + "the evicted busy write must carry the INDETERMINATE marker, not be a plain failure", + ); + assert.match(err.message, /INDETERMINATE/); + assert.match(err.message, /verify|Do NOT blindly retry/i); + return true; + }); +}); + 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");