From 0a4c46af81ca8b8bf57b12049c1a81eb86872292 Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Sun, 30 Aug 2026 18:49:12 +0300 Subject: [PATCH 1/2] fix: wire hold eviction per-Awareness-instance, not process-wide (#120) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initHoldEviction()'s single module-level boolean guard meant only the first WorkspaceContext ever resolved in a process got cross-client hold eviction wired — every subsequent {workspaceId, shardId} context's Awareness silently never got its 'change' listener registered, so a human cursor arriving on any shard but the first would never evict an agent's hold there. Already reachable today (multiple contexts are a real, tested capability of workspace-store.ts), and guaranteed to fire once any Collection gets its own shard (#120). Refs #120 Co-Authored-By: Claude Sonnet 5 --- src/lib/server/holds.test.ts | 58 ++++++++++++++++++++++++++++++++++++ src/lib/server/holds.ts | 15 ++++++---- 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/src/lib/server/holds.test.ts b/src/lib/server/holds.test.ts index 7cc54e1..95c36e2 100644 --- a/src/lib/server/holds.test.ts +++ b/src/lib/server/holds.test.ts @@ -190,3 +190,61 @@ describe('holds: agent hold requests', () => { expect(isHeldByClient(awareness, clientId, 'r2')).toBe(false); }); }); + +describe('holds: eviction wiring across multiple concurrent Awareness instances (#120)', () => { + // workspace-store.ts can resolve more than one concurrent {workspaceId, + // shardId} context in the same process — each with its own Awareness — + // and calls initHoldEviction() on every one of them. A single + // module-level "already wired" flag would only ever wire the first one, + // leaving every subsequent shard's cross-client hold eviction silently + // dead. This proves eviction works independently on a *second* instance + // resolved after the first, without needing real Collection sharding. + let docA: Y.Doc; + let docB: Y.Doc; + let awarenessA: Awareness; + let awarenessB: Awareness; + + beforeEach(() => { + resetHoldsForTests(); + docA = new Y.Doc(); + docB = new Y.Doc(); + awarenessA = new Awareness(docA); + awarenessB = new Awareness(docB); + }); + + afterEach(() => { + awarenessA.destroy(); + awarenessB.destroy(); + }); + + it('wires eviction independently on every distinct Awareness instance, not just the first', () => { + initHoldEviction(awarenessA); + initHoldEviction(awarenessB); + + const clientId = clientIdForToken('token-a'); + requestAgentHold(awarenessA, clientId, agent, ['r1'], () => true); + requestAgentHold(awarenessB, clientId, agent, ['r1'], () => true); + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(true); + expect(isHeldByClient(awarenessB, clientId, 'r1')).toBe(true); + + setHumanCursor(awarenessA, 'r1'); + setHumanCursor(awarenessB, 'r1'); + + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(false); + expect(isHeldByClient(awarenessB, clientId, 'r1')).toBe(false); + }); + + it('still wires the second instance even when the first was initialized long before it', () => { + initHoldEviction(awarenessA); + // Simulate real usage: awarenessB is only created/wired well after A. + const clientId = clientIdForToken('token-a'); + requestAgentHold(awarenessA, clientId, agent, ['r1'], () => true); + setHumanCursor(awarenessA, 'r1'); + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(false); + + initHoldEviction(awarenessB); + requestAgentHold(awarenessB, clientId, agent, ['r2'], () => true); + setHumanCursor(awarenessB, 'r2'); + expect(isHeldByClient(awarenessB, clientId, 'r2')).toBe(false); + }); +}); diff --git a/src/lib/server/holds.ts b/src/lib/server/holds.ts index 21d9f3c..52b31af 100644 --- a/src/lib/server/holds.ts +++ b/src/lib/server/holds.ts @@ -21,7 +21,12 @@ const AGENT_HOLD_TTL_MS = 100_000; // PRD target: 90-120s const agentClocks = new Map(); const agentTtlTimers = new Map>(); -let evictionWired = false; +// Per-Awareness-instance, not a single module-level flag: workspace-store.ts +// can resolve more than one concurrent {workspaceId, shardId} context (each +// with its own Awareness) — a single boolean guard would wire eviction only +// for whichever context happened to resolve first in the process, leaving +// every other shard's cross-client hold eviction silently dead. +let wiredAwareness = new WeakSet(); /** Stable synthetic clientID for a given access token, so a stateless HTTP * agent's holds persist across separate hold/write/release calls. */ @@ -66,8 +71,8 @@ export function aggregateHolds(awareness: Awareness): Map { * human's cursor already occupies. */ export function initHoldEviction(awareness: Awareness): void { - if (evictionWired) return; - evictionWired = true; + if (wiredAwareness.has(awareness)) return; + wiredAwareness.add(awareness); awareness.on( 'change', @@ -85,7 +90,7 @@ export function initHoldEviction(awareness: Awareness): void { } export function resetHoldEvictionForTests(): void { - evictionWired = false; + wiredAwareness = new WeakSet(); agentClocks.clear(); agentTtlTimers.forEach((timer) => clearTimeout(timer)); agentTtlTimers.clear(); @@ -231,5 +236,5 @@ export function resetHoldsForTests(): void { agentClocks.clear(); for (const timer of agentTtlTimers.values()) clearTimeout(timer); agentTtlTimers.clear(); - evictionWired = false; + wiredAwareness = new WeakSet(); } From 5b0699c910bd23bacee96125ac757b3a33855500 Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Sun, 30 Aug 2026 19:21:14 +0300 Subject: [PATCH 2/2] fix: scope hold TTL timers per-Awareness, not by clientId alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agentClocks/agentTtlTimers were keyed only by the synthetic clientId (deterministic per access token, regardless of which shard's Awareness it's operating against). A cross-shard agent hold batch under the same token now legitimately holds on two different Awareness instances at once (#125 made hold_records capable of this) — the second shard's scheduleTtl() call was silently cancelling the first shard's timer via the shared clientId key, so that hold never auto-expired. Both maps are now Map>, with get-or-create helpers threaded through writeRemoteState/scheduleTtl/clearTtl. 635/635 tests passing (2 new, proving independent TTL expiry across two Awareness instances under the same clientId). Refs #120 --- src/lib/server/holds.test.ts | 59 +++++++++++++++++++++++++++++++++ src/lib/server/holds.ts | 63 +++++++++++++++++++++++++++--------- 2 files changed, 107 insertions(+), 15 deletions(-) diff --git a/src/lib/server/holds.test.ts b/src/lib/server/holds.test.ts index 95c36e2..6623bc4 100644 --- a/src/lib/server/holds.test.ts +++ b/src/lib/server/holds.test.ts @@ -248,3 +248,62 @@ describe('holds: eviction wiring across multiple concurrent Awareness instances expect(isHeldByClient(awarenessB, clientId, 'r2')).toBe(false); }); }); + +describe('holds: TTL timers scoped per-Awareness, not by clientId alone (#120)', () => { + // The same access token always maps to the same synthetic clientId, + // regardless of which shard's Awareness it's holding records on — a + // cross-shard agent hold batch (a stated acceptance criterion) can + // legitimately hold under that same clientId on two different Awareness + // instances at once. A TTL timer map keyed only by clientId would let + // the second shard's scheduleTtl() silently cancel the first shard's + // timer, so the first hold would never auto-expire. + let docA: Y.Doc; + let docB: Y.Doc; + let awarenessA: Awareness; + let awarenessB: Awareness; + + beforeEach(() => { + resetHoldsForTests(); + docA = new Y.Doc(); + docB = new Y.Doc(); + awarenessA = new Awareness(docA); + awarenessB = new Awareness(docB); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + awarenessA.destroy(); + awarenessB.destroy(); + }); + + it('a hold on one shard keeps its own TTL even after the same clientId schedules a hold on another shard', () => { + const clientId = clientIdForToken('token-a'); + + requestAgentHold(awarenessA, clientId, agent, ['r1'], () => true); + vi.advanceTimersByTime(60_000); + // Scheduling a second, later hold under the *same* clientId on a + // *different* Awareness must not reset or cancel shard A's timer. + requestAgentHold(awarenessB, clientId, agent, ['r2'], () => true); + + vi.advanceTimersByTime(39_999); // 99,999ms since A's grant + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(true); + vi.advanceTimersByTime(1); // 100,000ms since A's grant + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(false); + }); + + it('both shards expire independently at their own 100s boundary', () => { + const clientId = clientIdForToken('token-a'); + + requestAgentHold(awarenessA, clientId, agent, ['r1'], () => true); + vi.advanceTimersByTime(50_000); + requestAgentHold(awarenessB, clientId, agent, ['r2'], () => true); + + vi.advanceTimersByTime(50_000); // 100,000ms since A, 50,000ms since B + expect(isHeldByClient(awarenessA, clientId, 'r1')).toBe(false); + expect(isHeldByClient(awarenessB, clientId, 'r2')).toBe(true); + + vi.advanceTimersByTime(50_000); // 100,000ms since B + expect(isHeldByClient(awarenessB, clientId, 'r2')).toBe(false); + }); +}); diff --git a/src/lib/server/holds.ts b/src/lib/server/holds.ts index 52b31af..6ccee93 100644 --- a/src/lib/server/holds.ts +++ b/src/lib/server/holds.ts @@ -19,8 +19,34 @@ export interface HoldAwarenessState { const AGENT_HOLD_TTL_MS = 100_000; // PRD target: 90-120s -const agentClocks = new Map(); -const agentTtlTimers = new Map>(); +// Keyed by Awareness *and* clientId, not clientId alone: the synthetic +// clientId is deterministic per access token (see clientIdForToken below), +// so the same token produces the same clientId regardless of which shard's +// Awareness it's operating against. A cross-shard agent hold batch (a +// stated acceptance criterion — see docs/specifications/collaboration.md) +// can legitimately hold records on two different Awareness instances under +// the same clientId; a flat Map would let the second +// scheduleTtl() call silently cancel the first shard's timer. +const agentClocks = new Map>(); +const agentTtlTimers = new Map>>(); + +function clocksFor(awareness: Awareness): Map { + let clocks = agentClocks.get(awareness); + if (!clocks) { + clocks = new Map(); + agentClocks.set(awareness, clocks); + } + return clocks; +} + +function timersFor(awareness: Awareness): Map> { + let timers = agentTtlTimers.get(awareness); + if (!timers) { + timers = new Map(); + agentTtlTimers.set(awareness, timers); + } + return timers; +} // Per-Awareness-instance, not a single module-level flag: workspace-store.ts // can resolve more than one concurrent {workspaceId, shardId} context (each // with its own Awareness) — a single boolean guard would wire eviction only @@ -92,7 +118,9 @@ export function initHoldEviction(awareness: Awareness): void { export function resetHoldEvictionForTests(): void { wiredAwareness = new WeakSet(); agentClocks.clear(); - agentTtlTimers.forEach((timer) => clearTimeout(timer)); + for (const timers of agentTtlTimers.values()) { + for (const timer of timers.values()) clearTimeout(timer); + } agentTtlTimers.clear(); } @@ -117,8 +145,9 @@ function writeRemoteState( clientId: number, state: HoldAwarenessState | null ): void { - const clock = (agentClocks.get(clientId) ?? 0) + 1; - agentClocks.set(clientId, clock); + const clocks = clocksFor(awareness); + const clock = (clocks.get(clientId) ?? 0) + 1; + clocks.set(clientId, clock); const encoder = encoding.createEncoder(); encoding.writeVarUint(encoder, 1); @@ -180,7 +209,7 @@ export function requestAgentHold( scheduleTtl(awareness, clientId); } else { writeRemoteState(awareness, clientId, null); - clearTtl(clientId); + clearTtl(awareness, clientId); } } @@ -197,14 +226,14 @@ export function releaseAgentHold( if (!recordIds) { writeRemoteState(awareness, clientId, null); - clearTtl(clientId); + clearTtl(awareness, clientId); return; } const nextHeld = existing.heldRecordIds.filter((id) => !recordIds.includes(id)); if (nextHeld.length === 0) { writeRemoteState(awareness, clientId, null); - clearTtl(clientId); + clearTtl(awareness, clientId); } else { writeRemoteState(awareness, clientId, { ...existing, heldRecordIds: nextHeld }); scheduleTtl(awareness, clientId); @@ -216,25 +245,29 @@ export function isHeldByClient(awareness: Awareness, clientId: number, recordId: } function scheduleTtl(awareness: Awareness, clientId: number): void { - clearTtl(clientId); + clearTtl(awareness, clientId); + const timers = timersFor(awareness); const timer = setTimeout(() => { writeRemoteState(awareness, clientId, null); - agentTtlTimers.delete(clientId); + timersFor(awareness).delete(clientId); }, AGENT_HOLD_TTL_MS); timer.unref?.(); - agentTtlTimers.set(clientId, timer); + timers.set(clientId, timer); } -function clearTtl(clientId: number): void { - const timer = agentTtlTimers.get(clientId); +function clearTtl(awareness: Awareness, clientId: number): void { + const timers = timersFor(awareness); + const timer = timers.get(clientId); if (timer) clearTimeout(timer); - agentTtlTimers.delete(clientId); + timers.delete(clientId); } /** Test-only: drop module-level state between test runs. */ export function resetHoldsForTests(): void { agentClocks.clear(); - for (const timer of agentTtlTimers.values()) clearTimeout(timer); + for (const timers of agentTtlTimers.values()) { + for (const timer of timers.values()) clearTimeout(timer); + } agentTtlTimers.clear(); wiredAwareness = new WeakSet(); }