fix(mcp): LRU-эвикция не реджектит чужой in-flight mutate как failure (#494)
Коммит 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<string, unknown>)[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(
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user