From 0d4f719f473dc93d5674a9429968120824bf85d4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 02:44:40 +0300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20fail-fast=20=D0=B3=D0=B0=D1=80?= =?UTF-8?q?=D0=B4=20=D0=BD=D0=B0=20=D0=BE=D0=B4=D0=BD=D0=BE=D0=B2=D1=80?= =?UTF-8?q?=D0=B5=D0=BC=D0=B5=D0=BD=D0=BD=D1=8B=D0=B9=20mutate=20=D0=BE?= =?UTF-8?q?=D0=B4=D0=BD=D0=BE=D0=B9=20CollabSession=20(=D1=80=D0=B5=D0=B2?= =?UTF-8?q?=D1=8C=D1=8E=20#431)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mutate трекает in-flight одним полем inflightReject; наложенный второй вызов перезаписал бы рехджектор первого -> при disconnect отклонился бы только второй, первый бы висел до PERSIST_TIMEOUT_MS (20с). В проде безопасно (оба call-site сериализуют через per-page withPageLock), но это футган на разделяемом примитиве. Гард в начале mutate (после ready-проверки, до касания inflightReject): наличие in-flight -> reject нового вызова без порчи состояния первого. Docstring CONCURRENCY. Последовательные mutate не задеты (localFinish синхронно чистит inflightReject до резолва). +2 теста: конкурентный второй реджектится, первый цел; последовательные оба успешны. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/lib/collab-session.ts | 20 +++++++++++++ .../mcp/test/unit/collab-session.test.mjs | 28 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/packages/mcp/src/lib/collab-session.ts b/packages/mcp/src/lib/collab-session.ts index 20aefafa..1c1c8d81 100644 --- a/packages/mcp/src/lib/collab-session.ts +++ b/packages/mcp/src/lib/collab-session.ts @@ -309,6 +309,14 @@ export class CollabSession { * unsyncedChanges is already 0, else wait for the unsyncedChanges->0 event * (PERSIST_TIMEOUT_MS), guarded by connectionLost so a reconnect handshake * cannot report a false success. + * + * CONCURRENCY: not safe to invoke concurrently on ONE session — the caller + * MUST serialize (hold the per-page lock), mirroring acquireCollabSession. + * The in-flight op is tracked in a single `inflightReject` field, so an + * overlapping second call would clobber the first's rejector and leave it + * hanging on disconnect. A fail-fast guard below rejects the overlap instead. + * Sequential (awaited) mutates are fine: localFinish clears inflightReject + * before the promise settles, so the guard is clear by the time the next runs. */ mutate( transform: (liveDoc: any) => any | null, @@ -327,6 +335,18 @@ export class CollabSession { ); } + // Fail-fast on concurrent use: a second overlapping mutate would overwrite + // the first's inflightReject, so a disconnect would only reject the second + // and hang the first until PERSIST_TIMEOUT_MS. Reject the overlap WITHOUT + // touching the in-flight op's state (no localFinish/teardown here). + if (this.inflightReject) { + return Promise.reject( + new Error( + "mutate already in-flight; caller must serialize (hold the page lock)", + ), + ); + } + return new Promise((resolve, reject) => { let settled = false; let persistTimer: ReturnType | undefined; diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index 5265b04a..f5874721 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -210,6 +210,34 @@ test("a pending write resolves when the server acks (unsyncedChanges -> 0)", asy assert.ok(r.doc); }); +test("concurrent mutate on one session: the second rejects, the first is unaffected", async () => { + __setCollabProviderFactory(factory({ unsynced: 1 })); // first stays pending after write + const session = await acquireCollabSession("page-1", "tok", "http://h/api"); + // First mutate: write happens synchronously, persistence ack still pending. + const first = session.mutate(() => docWith("first")); + // Second overlapping mutate on the SAME session must fail fast. + await assert.rejects( + session.mutate(() => docWith("second")), + /mutate already in-flight; caller must serialize \(hold the page lock\)/, + ); + // The first op is untouched: its rejector was not clobbered. Ack it now. + FakeProvider.last()._ack(); + const r = await first; + assert.ok(r.doc, "the first in-flight mutate still resolves on its own ack"); + assert.equal(FakeProvider.connectCount, 1, "no reconnect from the rejected overlap"); +}); + +test("sequential mutates on one session both succeed (the guard doesn't break serialized use)", async () => { + const session = await acquireCollabSession("page-1", "tok", "http://h/api"); + // Await the first fully, then run the second: inflightReject was cleared by + // localFinish before the first settled, so the guard is clear for the second. + const r1 = await session.mutate(() => docWith("one")); + assert.ok(r1.doc, "first sequential mutate resolves"); + const r2 = await session.mutate(() => docWith("two")); + assert.ok(r2.doc, "second sequential mutate resolves — guard not tripped"); + assert.equal(FakeProvider.connectCount, 1, "sequential mutates reuse one provider"); +}); + test("connect timeout rejects with the connect-timeout text and fires the metric hook", async () => { mock.timers.enable({ apis: ["setTimeout"] }); __setCollabProviderFactory(factory({ autoSync: false }));