From d22838411a0e0bed3f5bc05f7bea789a0bc4c01a Mon Sep 17 00:00:00 2001 From: rclod <3385524+rclod@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:20:54 -0500 Subject: [PATCH] fix: keep comments durable across collab session token expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A collab session token expired while the provider was connected. The server closed the socket 4401 "Invalid or expired collab session token", and comments added afterwards stayed visible in the tab while SQLite kept revision=1, marks='{}' and zero document_y_updates. Four independent defects had to line up, and each one alone loses the comment: 1. src/editor/index.ts renewal loop returned early whenever the connection was `connected && isSynced` — exactly the state a session is in for the whole minute before it expires — so it never renewed ahead of expiry and only reacted after being closed. 2. Nothing in src/ ever assigned collabClient.terminalCloseReason. The immediate-refresh hook that read it was therefore unreachable, so an auth failure had no fast path back. 3. flushShareMarks gated the durable REST write on collabEnabled and collabCanEdit. Those are capability flags and stay true on a provider that has been closed out, so the comment went to the local Y.Doc and nowhere else, with no unsaved state surfaced. 4. shouldDeferExpiringCollabRefresh deferred renewal whenever there was pending local state, without considering connection status. Adding a comment set unsyncedChanges=1, which then blocked the very refresh that would have saved it. The stalled-collab recovery path that could still fire passed preserveLocalState=false, resetting the Y.Doc and discarding the comment outright. The decisions move into src/bridge/collab-session-renewal.ts as pure functions, because the editor is browser-only and cannot be instantiated in this harness — the logic that lost data was untestable where it lived. Renewal now runs on a healthy connection, defers only while the connection is usable, and never defers past a hard deadline. Auth failures take one deterministic refresh that preserves local marks. REST persistence is selected on provider liveness rather than capability, and only while no provider is connected, so it never writes alongside a live Yjs writer. Tests: collab-session-renewal-expiry.test.ts drives a fake clock across expiry and runs each scenario against a reference implementation of the replaced inline logic, asserting the old rule gets it wrong — a test both implementations pass would not have caught this. collab-comment-durability-across-expiry.test.ts proves a comment written while the provider is dead persists once and converges on replay. --- src/bridge/collab-client.ts | 26 ++ src/bridge/collab-session-renewal.ts | 200 ++++++++++ src/editor/index.ts | 79 +++- ...b-comment-durability-across-expiry.test.ts | 189 +++++++++ .../collab-session-renewal-expiry.test.ts | 375 ++++++++++++++++++ 5 files changed, 852 insertions(+), 17 deletions(-) create mode 100644 src/bridge/collab-session-renewal.ts create mode 100644 src/tests/collab-comment-durability-across-expiry.test.ts create mode 100644 src/tests/collab-session-renewal-expiry.test.ts diff --git a/src/bridge/collab-client.ts b/src/bridge/collab-client.ts index 1011ee98..9a81139f 100644 --- a/src/bridge/collab-client.ts +++ b/src/bridge/collab-client.ts @@ -3,6 +3,7 @@ import { HocuspocusProvider } from '@hocuspocus/provider'; import type { Awareness } from 'y-protocols/awareness'; import { shareClient, type CollabSessionInfo, type ShareRole } from './share-client'; import { shouldPreserveMissingLocalMark } from './marks-preservation'; +import { classifyCollabAuthFailure, type CollabAuthFailureClass } from './collab-session-renewal'; import { recordClientIncidentEvent } from '../agent/client-incident-buffer'; type PresenceHandler = (count: number) => void; @@ -200,6 +201,9 @@ export class CollabClient { private sessionRole: ShareRole | null = null; terminalCloseReason: CollabTerminalCloseReason = null; lastAuthenticationFailureReason: string | null = null; + lastAuthFailureClass: CollabAuthFailureClass | null = null; + /** Set on an auth failure, consumed once by the editor to drive one refresh. */ + authRecoveryPending = false; constructor() { this.durableClientId = getOrCreateDurableClientId(); @@ -560,6 +564,8 @@ export class CollabClient { this.unsyncedChanges = 0; this.hasSynced = false; this.terminalCloseReason = null; + this.authRecoveryPending = false; + this.lastAuthFailureClass = null; this.lastAuthenticationFailureReason = null; this.emitSyncStatus(); this.durableUpdatesEnabled = this.canPersistDurableUpdates(session.role); @@ -622,6 +628,8 @@ export class CollabClient { } if (event.status === 'connected') { this.terminalCloseReason = null; + this.authRecoveryPending = false; + this.lastAuthFailureClass = null; this.lastAuthenticationFailureReason = null; if (this.lastDisconnectAt !== null) { const durationMs = Date.now() - this.lastDisconnectAt; @@ -658,9 +666,22 @@ export class CollabClient { provider.on('authenticationFailed', (event: { reason?: string }) => { const reason = typeof event?.reason === 'string' ? event.reason : 'permission-denied'; this.lastAuthenticationFailureReason = reason; + this.lastAuthFailureClass = classifyCollabAuthFailure(reason); this.connectionStatus = 'disconnected'; this.hasSynced = false; this.lastDisconnectAt = Date.now(); + // An expired token is recoverable, so it must not be recorded as a + // terminal close — that would put the tab into the read-only banner path + // instead of refreshing. Only an explicit denial is terminal, and even + // then the refresh response is what confirms it. + if (this.lastAuthFailureClass === 'permission-denied') { + this.terminalCloseReason = 'permission-denied'; + } + // Consumed once by the editor's sync-status handler. Without this the + // 4401 close left the session expired indefinitely: nothing in src/ ever + // assigned terminalCloseReason, so the immediate-refresh hook that read + // it was unreachable. + this.authRecoveryPending = true; recordClientIncidentEvent({ type: 'collab.authentication_failed', level: 'error', @@ -669,6 +690,7 @@ export class CollabClient { slug: session.slug, role: session.role, reason, + failureClass: this.lastAuthFailureClass, }, }); this.emitSyncStatus(); @@ -720,6 +742,8 @@ export class CollabClient { this.activeSession = { ...session }; this.sessionRole = session.role; this.terminalCloseReason = null; + this.authRecoveryPending = false; + this.lastAuthFailureClass = null; this.lastAuthenticationFailureReason = null; this.connectionStatus = 'connecting'; this.hasSynced = false; @@ -877,6 +901,8 @@ export class CollabClient { this.hasSynced = false; this.applyingLocalMarks = false; this.terminalCloseReason = null; + this.authRecoveryPending = false; + this.lastAuthFailureClass = null; this.lastAuthenticationFailureReason = null; this.sessionRole = null; this.emitSyncStatus(); diff --git a/src/bridge/collab-session-renewal.ts b/src/bridge/collab-session-renewal.ts new file mode 100644 index 00000000..4935cb06 --- /dev/null +++ b/src/bridge/collab-session-renewal.ts @@ -0,0 +1,200 @@ +/** + * Collab session renewal and mark-durability decisions. + * + * These were inline conditions in `src/editor/index.ts` and + * `src/bridge/collab-client.ts`. They are extracted here because the editor is + * browser-only and cannot be instantiated in the test harness, so the decisions + * that actually lose data were untestable where they lived. + * + * The defect they fix: a collab session token expires while the provider is + * connected and healthy. The old renewal loop returned early whenever the + * connection was `connected && isSynced`, so it never renewed ahead of expiry. + * The server then closed the socket with 4401 "Invalid or expired collab + * session token". A comment submitted after that point was written only into + * the local Y.Doc — the REST safety path was skipped because `collabEnabled` + * and `collabCanEdit` are capability flags that stay true on a dead provider — + * and the deferral rule then blocked recovery *because* there was unsaved work. + */ + +export type CollabConnectionStatus = 'connected' | 'connecting' | 'disconnected'; + +export type CollabAuthFailureClass = 'expired' | 'permission-denied' | 'unknown'; + +/** Renew this far ahead of expiry under normal conditions. */ +export const COLLAB_RENEWAL_LEAD_MS = 60_000; + +/** + * Inside this window, renewal happens regardless of typing or pending local + * state. Past expiry there is nothing left to protect: the server will reject + * the next message anyway. + */ +export const COLLAB_RENEWAL_HARD_DEADLINE_MS = 15_000; + +/** Minimum spacing between renewal attempts, so a failing refresh cannot spin. */ +export const COLLAB_RENEWAL_BACKOFF_MS = 5_000; + +/** + * Classify the reason carried by a Hocuspocus `authenticationFailed` event. + * + * The server produces two distinguishable shapes: the pre-gate in + * `server/ws.ts` closes 4401 with "Invalid or expired collab session token", + * and `authenticateCollabSession()` in `server/collab.ts` throws + * 'permission-denied' or 'session-stale'. + * + * Classification is diagnostic only. Both classes attempt exactly one refresh, + * because an expired token also surfaces as 'permission-denied' through the + * Hocuspocus hook — the refresh response, not this string, is what decides + * whether access is really gone. + */ +export function classifyCollabAuthFailure( + reason: string | null | undefined, +): CollabAuthFailureClass { + const normalized = (reason ?? '').trim().toLowerCase(); + if (!normalized) return 'unknown'; + if (normalized.includes('expired')) return 'expired'; + if (normalized.includes('session-stale') || normalized.includes('session stale')) return 'expired'; + if (normalized.includes('permission-denied') || normalized.includes('permission denied')) { + return 'permission-denied'; + } + if (normalized.includes('unauthorized')) return 'expired'; + return 'unknown'; +} + +export type ProactiveRenewalInput = { + expiresAtMs: number | null; + now: number; + connectionStatus: CollabConnectionStatus; + refreshInFlight: boolean; + lastRenewalAttemptMs: number | null; + hasPendingLocalState: boolean; + lastLocalTypingAt: number; + typingGraceMs: number; + leadMs?: number; + hardDeadlineMs?: number; + backoffMs?: number; +}; + +/** + * Whether to renew the collab session now. + * + * Two rules differ from the original inline logic, and each one on its own was + * enough to lose a comment: + * + * 1. A healthy connection is NOT a reason to skip renewal. The old code + * returned early on `connected && isSynced`, which is precisely the state a + * session is in for the whole minute before it expires. + * 2. Deferral for typing or unsaved work applies only while the connection is + * still usable. Once the provider is disconnected, unsaved work is the + * reason to reconnect, not a reason to wait — the old rule deadlocked + * exactly when recovery mattered. + */ +export function shouldRenewCollabSession(input: ProactiveRenewalInput): boolean { + const { + expiresAtMs, + now, + connectionStatus, + refreshInFlight, + lastRenewalAttemptMs, + hasPendingLocalState, + lastLocalTypingAt, + typingGraceMs, + leadMs = COLLAB_RENEWAL_LEAD_MS, + hardDeadlineMs = COLLAB_RENEWAL_HARD_DEADLINE_MS, + backoffMs = COLLAB_RENEWAL_BACKOFF_MS, + } = input; + + if (refreshInFlight) return false; + if (expiresAtMs === null || !Number.isFinite(expiresAtMs)) return false; + + const remainingMs = expiresAtMs - now; + if (remainingMs > leadMs) return false; + + if (lastRenewalAttemptMs !== null && (now - lastRenewalAttemptMs) < backoffMs) return false; + + // At or past the hard deadline nothing may defer renewal. + if (remainingMs <= hardDeadlineMs) return true; + + // Only a live connection earns the courtesy of not being interrupted. + if (connectionStatus === 'connected') { + if (hasPendingLocalState) return false; + if ((now - lastLocalTypingAt) < typingGraceMs) return false; + } + + return true; +} + +export type PreserveLocalStateInput = { + collabCanEdit: boolean; + hasPendingLocalState: boolean; +}; + +/** + * Whether a reconnect should replay local state instead of resetting the doc. + * + * The stalled-collab recovery path previously hardcoded `false` here, so the + * reconnect it triggered reset the Y.Doc and discarded the very comment the + * user was waiting to see saved. Replay is safe: marks are keyed by mark id and + * merged by key, so replaying an already-persisted comment is a no-op rather + * than a duplicate. + */ +export function shouldPreserveLocalStateOnReconnect(input: PreserveLocalStateInput): boolean { + return input.collabCanEdit && input.hasPendingLocalState; +} + +export type RestMarksFallbackInput = { + collabEnabled: boolean; + collabCanEdit: boolean; + legacyRestFallback: boolean; + connectionStatus: CollabConnectionStatus; +}; + +/** + * Whether `flushShareMarks` must also push marks over REST. + * + * `collabEnabled` and `collabCanEdit` describe what the session is *permitted* + * to do, not whether a provider is alive to do it. Gating the REST path on + * those alone meant a dead provider silently swallowed comments. Liveness is + * the missing term. + * + * This does not create a parallel writer: REST is used only when the provider + * is not connected, so there is no live Yjs writer to race. While connected, + * the provider remains the sole owner of the canonical document. + */ +export function shouldUseRestMarksFallback(input: RestMarksFallbackInput): boolean { + if (!input.collabEnabled) return true; + if (!input.collabCanEdit) return true; + if (input.legacyRestFallback) return true; + return input.connectionStatus !== 'connected'; +} + +/** + * Whether marks are durably acknowledged, i.e. whether the comment UI may stop + * showing them as pending. A mark is durable once it has either round-tripped + * through a synced provider or been accepted by the REST path. + */ +export function areMarksDurablyAcknowledged(input: { + connectionStatus: CollabConnectionStatus; + isSynced: boolean; + unsyncedChanges: number; + pendingLocalUpdates: number; + lastRestMarksAckAt: number | null; + lastLocalMarkAt: number | null; +}): boolean { + const { + connectionStatus, + isSynced, + unsyncedChanges, + pendingLocalUpdates, + lastRestMarksAckAt, + lastLocalMarkAt, + } = input; + + if (lastLocalMarkAt === null) return true; + + if (lastRestMarksAckAt !== null && lastRestMarksAckAt >= lastLocalMarkAt) return true; + + return connectionStatus === 'connected' + && isSynced + && unsyncedChanges === 0 + && pendingLocalUpdates === 0; +} diff --git a/src/editor/index.ts b/src/editor/index.ts index 86116fab..ebd1e58e 100644 --- a/src/editor/index.ts +++ b/src/editor/index.ts @@ -146,6 +146,11 @@ import { initThemePicker, getThemePicker } from '../ui/theme-picker'; import { fileClient } from '../bridge/file-client'; import { shareClient, type CollabSessionInfo, type SharePendingEvent } from '../bridge/share-client'; import { collabClient, type CollabSyncStatus } from '../bridge/collab-client'; +import { + shouldPreserveLocalStateOnReconnect, + shouldRenewCollabSession, + shouldUseRestMarksFallback, +} from '../bridge/collab-session-renewal'; import { shouldDeferShareMarksRefresh } from './share-marks-refresh'; import { collabCursorBuilder, collabSelectionBuilder } from './plugins/collab-cursors'; import { isAgentScopedId } from '../shared/agent-identity'; @@ -1117,6 +1122,7 @@ class ProofEditorImpl implements ProofEditor { private readonly collabRecoveryDelayMs: number = 4_000; private readonly collabRecoveryBackoffMs: number = 5_000; private readonly collabTypingRecoveryGraceMs: number = 3_000; + private lastCollabRenewalAttemptMs: number | null = null; private readonly shareEventPollMs: number = 1500; private readonly shareDocumentUpdatedDebounceMs: number = 600; private readonly commentPopoverDraftRestoreDelayMs: number = 120; @@ -1511,8 +1517,22 @@ class ProofEditorImpl implements ProofEditor { this.collabUnsyncedChanges = status.unsyncedChanges; this.collabPendingLocalUpdates = status.pendingLocalUpdates; this.updateShareEditGate(); - if (status.connectionStatus === 'disconnected' && collabClient.terminalCloseReason === 'permission-denied') { - void this.refreshCollabSessionAndReconnect(false); + // Any authentication failure gets exactly one immediate refresh, + // preserving unsaved local marks. The reason string cannot be trusted + // to separate "expired" from "revoked" — the server reports an + // expired token as permission-denied through the Hocuspocus hook — so + // the refresh response is what decides. A real revocation comes back + // 401/403/404/410 and refreshCollabSessionAndReconnect() tears down + // into the read-only banner from there. + // + // This replaces a check on terminalCloseReason, which nothing in src/ + // ever assigned, so the hook could never fire. + if (status.connectionStatus === 'disconnected' && collabClient.authRecoveryPending) { + collabClient.authRecoveryPending = false; + void this.refreshCollabSessionAndReconnect(shouldPreserveLocalStateOnReconnect({ + collabCanEdit: this.collabCanEdit, + hasPendingLocalState: this.shouldPreservePendingLocalCollabState(), + })); } if (status.connectionStatus === 'connected' && status.isSynced) { if (this.pendingCollabRebindOnSync) { @@ -2250,13 +2270,21 @@ class ProofEditorImpl implements ProofEditor { if (!this.collabEnabled || !this.activeCollabSession) return; await this.maybeRecoverStalledCollab(); const expiresAt = this.activeCollabSession.expiresAt; - if (!expiresAt) return; - const expiresAtMs = Date.parse(expiresAt); - if (!Number.isFinite(expiresAtMs)) return; + const expiresAtMs = expiresAt ? Date.parse(expiresAt) : null; const now = Date.now(); - if ((expiresAtMs - now) > 60_000) return; - if (this.collabConnectionStatus === 'connected' && this.collabIsSynced) return; - if (this.shouldDeferExpiringCollabRefresh(now)) return; + if (!shouldRenewCollabSession({ + expiresAtMs: expiresAtMs !== null && Number.isFinite(expiresAtMs) ? expiresAtMs : null, + now, + connectionStatus: this.collabConnectionStatus, + refreshInFlight: this.collabSessionRefreshInFlight, + lastRenewalAttemptMs: this.lastCollabRenewalAttemptMs, + hasPendingLocalState: this.shouldPreservePendingLocalCollabState(), + lastLocalTypingAt: this.lastLocalTypingAt, + typingGraceMs: this.collabTypingRecoveryGraceMs, + })) { + return; + } + this.lastCollabRenewalAttemptMs = now; await this.refreshCollabSessionAndReconnect(this.shouldPreservePendingLocalCollabState()); }, 2_000); } @@ -2266,13 +2294,10 @@ class ProofEditorImpl implements ProofEditor { && (this.collabUnsyncedChanges > 0 || this.collabPendingLocalUpdates > 0); } - private shouldDeferExpiringCollabRefresh(now: number): boolean { - if (!this.collabCanEdit) return false; - if (this.shouldPreservePendingLocalCollabState()) return true; - if (this.pendingProjectionPublish) return true; - if (this.contentSyncTimeout !== null) return true; - return (now - this.lastLocalTypingAt) < this.collabTypingRecoveryGraceMs; - } + // shouldDeferExpiringCollabRefresh() lived here. Its deferral rules moved into + // shouldRenewCollabSession(), which applies them only while the connection is + // still usable and never past the hard deadline. Deferring on a disconnected + // session was what let an expired token sit unrenewed for minutes. private updateCollabHealthWindow(status: CollabSyncStatus): void { const healthy = status.connectionStatus === 'connected' @@ -2302,7 +2327,13 @@ class ProofEditorImpl implements ProofEditor { if ((now - this.collabLastRecoveryAttemptMs) < this.collabRecoveryBackoffMs) return; this.collabLastRecoveryAttemptMs = now; this.collabUnhealthySinceMs = now; - await this.refreshCollabSessionAndReconnect(false); + // Previously hardcoded false, which reset the Y.Doc on reconnect and threw + // away any comment the user had just added — the loss this recovery path + // was supposed to prevent. + await this.refreshCollabSessionAndReconnect(shouldPreserveLocalStateOnReconnect({ + collabCanEdit: this.collabCanEdit, + hasPendingLocalState: this.shouldPreservePendingLocalCollabState(), + })); } private teardownCollabRuntimeAfterTerminalRefreshFailure(): void { @@ -4821,7 +4852,21 @@ class ProofEditorImpl implements ProofEditor { if (!shouldPersistMarks) { return; } - if (!this.collabEnabled || !this.collabCanEdit || LEGACY_REST_FALLBACK) { + // Liveness, not just capability. collabEnabled/collabCanEdit stay true + // on a provider that has been closed out from under the tab, so gating + // the durable REST write on them alone meant a comment submitted after + // a 4401 close was written to the local Y.Doc and nowhere else. + // + // REST runs only when the provider is not connected, so it never writes + // alongside a live Yjs writer. Marks are keyed by mark id and merged by + // key, so a mark that later replays on reconnect converges rather than + // duplicating. + if (shouldUseRestMarksFallback({ + collabEnabled: this.collabEnabled, + collabCanEdit: this.collabCanEdit, + legacyRestFallback: LEGACY_REST_FALLBACK, + connectionStatus: this.collabConnectionStatus, + })) { void shareClient.pushMarks(metadata, getCurrentActor(), { keepalive: Boolean(_options?.keepalive) }); } } catch (error) { diff --git a/src/tests/collab-comment-durability-across-expiry.test.ts b/src/tests/collab-comment-durability-across-expiry.test.ts new file mode 100644 index 00000000..21a58664 --- /dev/null +++ b/src/tests/collab-comment-durability-across-expiry.test.ts @@ -0,0 +1,189 @@ +/** + * A comment submitted while the collab provider is dead must still become + * durable, and replaying it on reconnect must not duplicate it. + * + * This is the server-side half of the token-expiry defect. The client-side + * decisions live in src/tests/collab-session-renewal-expiry.test.ts; this file + * proves the REST path those decisions now select actually persists a comment + * and converges on replay, with a live collab room loaded for the same slug. + * + * Field state being reproduced: revision=1, marks='{}', zero + * document_y_updates, no comment events, while the browser tab showed the + * comment. + */ + +import { unlinkSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +function assert(condition: boolean, message: string): void { + if (!condition) throw new Error(message); +} + +async function sleep(ms: number): Promise { + await new Promise((resolve) => setTimeout(resolve, ms)); +} + +function commentMark(id: string, text: string): Record { + return { + kind: 'comment', + by: 'human:durability-test', + text, + quote: 'collaborative session fixture', + resolved: false, + id, + }; +} + +function readMarks(row: { marks?: string | null } | null | undefined): Record { + if (!row?.marks) return {}; + try { + const parsed = JSON.parse(row.marks) as unknown; + return parsed && typeof parsed === 'object' ? parsed as Record : {}; + } catch { + return {}; + } +} + +function commentKeys(marks: Record): string[] { + return Object.entries(marks) + .filter(([, value]) => (value as { kind?: string } | null)?.kind === 'comment') + .map(([key]) => key) + .sort(); +} + +async function run(): Promise { + const dbName = `proof-comment-durability-${Date.now()}-${Math.random().toString(36).slice(2)}.db`; + const dbPath = path.join(os.tmpdir(), dbName); + const previousDbPath = process.env.DATABASE_PATH; + process.env.DATABASE_PATH = dbPath; + + const db = await import('../../server/db.ts'); + const collab = await import('../../server/collab.ts'); + + const slug = `comment-durability-${Math.random().toString(36).slice(2, 10)}`; + const markdown = [ + '# Durability', + '', + 'This is a longer collaborative session fixture with room for comment anchors.', + ].join('\n'); + + try { + db.createDocument(slug, markdown, {}, 'comment durability across token expiry'); + + await collab.startCollabRuntimeEmbedded(4000); + const instance = collab.__unsafeGetHocuspocusInstanceForTests() as { + createDocument?: ( + slug: string, + request: Record, + socketId: string, + context: Record, + hooks: Record, + ) => Promise; + }; + assert(Boolean(instance?.createDocument), 'Expected collab test instance'); + + // A tab has the document open; the room is live even though the tab's own + // provider is about to be closed out by an expired token. + await instance.createDocument!( + slug, + {}, + 'comment-durability-socket', + { isAuthenticated: true, readOnly: false, requiresAuthentication: true }, + {}, + ); + + const baseline = readMarks(db.getDocumentBySlug(slug)); + assert( + commentKeys(baseline).length === 0, + `Expected no comments before the test writes any. keys=${commentKeys(baseline).join(',')}`, + ); + + // The provider is dead (expired token). shouldUseRestMarksFallback() now + // selects REST, which lands here. + const first = commentMark('c-first', 'written while the provider was dead'); + assert(db.updateMarks(slug, { 'c-first': first }), 'Expected REST marks write to succeed'); + + await sleep(300); + + const afterFirst = readMarks(db.getDocumentBySlug(slug)); + assert( + commentKeys(afterFirst).length === 1, + `Expected exactly one durable comment after the REST fallback. keys=${commentKeys(afterFirst).join(',')}`, + ); + assert( + JSON.stringify(afterFirst['c-first']).includes('written while the provider was dead'), + 'Expected the comment body to survive the REST write', + ); + + // Reconnect: the preserved local Y.Doc replays the same mark. Marks are + // keyed by mark id, so this must converge rather than append a second copy. + assert( + db.updateMarks(slug, { 'c-first': first }), + 'Expected replay of an already-persisted mark to be accepted', + ); + + await sleep(300); + + const afterReplay = readMarks(db.getDocumentBySlug(slug)); + assert( + commentKeys(afterReplay).length === 1, + `Expected replay not to duplicate the comment. keys=${commentKeys(afterReplay).join(',')}`, + ); + + // A genuinely new comment must still land, so convergence is not just + // swallowing every subsequent write. + const second = commentMark('c-second', 'added after reconnect'); + assert( + db.updateMarks(slug, { 'c-first': first, 'c-second': second }), + 'Expected a second distinct comment to be accepted', + ); + + await sleep(300); + + const afterSecond = readMarks(db.getDocumentBySlug(slug)); + assert( + commentKeys(afterSecond).join(',') === 'c-first,c-second', + `Expected both comments to be durable and distinct. keys=${commentKeys(afterSecond).join(',')}`, + ); + + // The agent API reads through the canonical projection, which is where the + // field report saw '{}' while the browser showed comments. + const canonical = await collab.getCanonicalReadableDocument?.(slug, 'state'); + if (canonical) { + const canonicalMarks = typeof (canonical as { marks?: unknown }).marks === 'string' + ? readMarks(canonical as { marks?: string }) + : ((canonical as { marks?: Record }).marks ?? {}); + assert( + commentKeys(canonicalMarks).length === 2, + `Expected the canonical read used by the agent API to expose both comments. keys=${commentKeys(canonicalMarks).join(',')}`, + ); + } + + console.log('✓ comments written while the provider is dead persist once and survive replay'); + } finally { + if (previousDbPath === undefined) { + delete process.env.DATABASE_PATH; + } else { + process.env.DATABASE_PATH = previousDbPath; + } + try { + const collab = await import('../../server/collab.ts'); + await collab.stopCollabRuntime(); + } catch { + // ignore teardown errors + } + for (const suffix of ['', '-wal', '-shm']) { + try { + unlinkSync(`${dbPath}${suffix}`); + } catch { + // ignore cleanup errors + } + } + } +} + +run().catch((error) => { + console.error(error instanceof Error ? error.stack || error.message : String(error)); + process.exit(1); +}); diff --git a/src/tests/collab-session-renewal-expiry.test.ts b/src/tests/collab-session-renewal-expiry.test.ts new file mode 100644 index 00000000..53641a9a --- /dev/null +++ b/src/tests/collab-session-renewal-expiry.test.ts @@ -0,0 +1,375 @@ +/** + * Regression coverage for comments lost when a collab session token expires. + * + * Field report: the browser console showed + * [HocuspocusProvider] Connection closed with status Unauthorized: + * Invalid or expired collab session token + * and comments added after that close stayed visible in the tab while SQLite + * kept revision=1, marks='{}', zero document_y_updates and no comment events. + * + * Each scenario below runs against both the current predicates and a reference + * implementation of the inline logic they replaced, asserting that the old + * logic gets the answer wrong. A test that both implementations pass would not + * have caught this defect. + */ + +import { + areMarksDurablyAcknowledged, + classifyCollabAuthFailure, + shouldPreserveLocalStateOnReconnect, + shouldRenewCollabSession, + shouldUseRestMarksFallback, + COLLAB_RENEWAL_LEAD_MS, +} from '../bridge/collab-session-renewal'; + +function assert(condition: boolean, message: string): void { + if (!condition) throw new Error(message); +} + +/** A deterministic clock, so expiry is crossed by arithmetic and not by sleeping. */ +function fakeClock(startMs: number) { + let now = startMs; + return { + now: () => now, + advance: (ms: number) => { + now += ms; + return now; + }, + }; +} + +/** + * src/editor/index.ts:2249-2261 as it stood before this change. Reproduced so + * the scenarios can prove the old rule returns the wrong answer. + */ +function legacyShouldRenew(input: { + expiresAtMs: number; + now: number; + connectionStatus: 'connected' | 'connecting' | 'disconnected'; + isSynced: boolean; + collabCanEdit: boolean; + hasPendingLocalState: boolean; + lastLocalTypingAt: number; + typingGraceMs: number; +}): boolean { + if ((input.expiresAtMs - input.now) > 60_000) return false; + // The guard that prevented proactive renewal. + if (input.connectionStatus === 'connected' && input.isSynced) return false; + // shouldDeferExpiringCollabRefresh(), which ignored connection state. + if (input.collabCanEdit) { + if (input.hasPendingLocalState) return false; + if ((input.now - input.lastLocalTypingAt) < input.typingGraceMs) return false; + } + return true; +} + +const TYPING_GRACE_MS = 3_000; + +function scenarioProactiveRenewalBeforeExpiry(): void { + const clock = fakeClock(1_760_000_000_000); + const expiresAtMs = clock.now() + 600_000; // 10 minute session + + // Idle mid-session: nothing should happen yet. + assert( + shouldRenewCollabSession({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'connected', + refreshInFlight: false, + lastRenewalAttemptMs: null, + hasPendingLocalState: false, + lastLocalTypingAt: clock.now() - 60_000, + typingGraceMs: TYPING_GRACE_MS, + }) === false, + 'Expected no renewal 10 minutes ahead of expiry', + ); + + // Advance to 30s before expiry, connection perfectly healthy and idle. + clock.advance(570_000); + const remaining = expiresAtMs - clock.now(); + assert(remaining < COLLAB_RENEWAL_LEAD_MS, 'Fixture should be inside the renewal lead window'); + + const legacy = legacyShouldRenew({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'connected', + isSynced: true, + collabCanEdit: true, + hasPendingLocalState: false, + lastLocalTypingAt: clock.now() - 60_000, + typingGraceMs: TYPING_GRACE_MS, + }); + assert( + legacy === false, + 'Reference implementation should reproduce the defect by refusing to renew a healthy session', + ); + + assert( + shouldRenewCollabSession({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'connected', + refreshInFlight: false, + lastRenewalAttemptMs: null, + hasPendingLocalState: false, + lastLocalTypingAt: clock.now() - 60_000, + typingGraceMs: TYPING_GRACE_MS, + }) === true, + 'Expected a healthy connection to renew proactively inside the lead window', + ); + + console.log(' ✓ healthy session renews before expiry instead of waiting to be closed'); +} + +function scenarioTypingDoesNotDeferPastHardDeadline(): void { + const clock = fakeClock(1_760_000_000_000); + const expiresAtMs = clock.now() + 45_000; + + // Actively typing, 45s of life left: deferring is still correct. + assert( + shouldRenewCollabSession({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'connected', + refreshInFlight: false, + lastRenewalAttemptMs: null, + hasPendingLocalState: true, + lastLocalTypingAt: clock.now(), + typingGraceMs: TYPING_GRACE_MS, + }) === false, + 'Expected renewal to defer for a live connection with unsaved work and time to spare', + ); + + // 10s of life left and still typing: the hard deadline wins. + clock.advance(35_000); + assert( + shouldRenewCollabSession({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'connected', + refreshInFlight: false, + lastRenewalAttemptMs: null, + hasPendingLocalState: true, + lastLocalTypingAt: clock.now(), + typingGraceMs: TYPING_GRACE_MS, + }) === true, + 'Expected the hard deadline to override typing and pending-state deferral', + ); + + console.log(' ✓ deferral cannot push renewal past the hard deadline'); +} + +function scenarioUnsavedWorkDoesNotDeadlockRecovery(): void { + const clock = fakeClock(1_760_000_000_000); + const expiresAtMs = clock.now() - 5_000; // token already expired + + // The tab is disconnected after the 4401 close and holds an unsaved comment. + const disconnectedWithUnsavedComment = { + expiresAtMs, + now: clock.now(), + connectionStatus: 'disconnected' as const, + refreshInFlight: false, + lastRenewalAttemptMs: null, + hasPendingLocalState: true, + lastLocalTypingAt: clock.now() - 30_000, + typingGraceMs: TYPING_GRACE_MS, + }; + + const legacy = legacyShouldRenew({ + expiresAtMs, + now: clock.now(), + connectionStatus: 'disconnected', + isSynced: false, + collabCanEdit: true, + hasPendingLocalState: true, + lastLocalTypingAt: clock.now() - 30_000, + typingGraceMs: TYPING_GRACE_MS, + }); + assert( + legacy === false, + 'Reference implementation should reproduce the deadlock: unsaved work blocked its own recovery', + ); + + assert( + shouldRenewCollabSession(disconnectedWithUnsavedComment) === true, + 'Expected an expired, disconnected session holding unsaved work to renew immediately', + ); + + console.log(' ✓ an unsaved comment no longer blocks the refresh that would save it'); +} + +function scenarioRenewalBackoffPreventsSpin(): void { + const clock = fakeClock(1_760_000_000_000); + const expiresAtMs = clock.now() - 1_000; + + const base = { + expiresAtMs, + now: clock.now(), + connectionStatus: 'disconnected' as const, + refreshInFlight: false, + hasPendingLocalState: true, + lastLocalTypingAt: clock.now() - 30_000, + typingGraceMs: TYPING_GRACE_MS, + }; + + assert( + shouldRenewCollabSession({ ...base, lastRenewalAttemptMs: clock.now() - 500 }) === false, + 'Expected a renewal attempt 500ms ago to be inside the backoff window', + ); + assert( + shouldRenewCollabSession({ ...base, lastRenewalAttemptMs: clock.now() - 6_000 }) === true, + 'Expected renewal to resume once the backoff window has passed', + ); + assert( + shouldRenewCollabSession({ ...base, lastRenewalAttemptMs: null, refreshInFlight: true }) === false, + 'Expected an in-flight refresh to suppress a second concurrent attempt', + ); + + console.log(' ✓ renewal backs off instead of spinning on a failing refresh'); +} + +function scenarioReconnectPreservesUnsavedComment(): void { + assert( + shouldPreserveLocalStateOnReconnect({ collabCanEdit: true, hasPendingLocalState: true }) === true, + 'Expected reconnect to replay an unsaved comment rather than reset the doc', + ); + assert( + shouldPreserveLocalStateOnReconnect({ collabCanEdit: true, hasPendingLocalState: false }) === false, + 'Expected a clean session to reconnect against server state', + ); + assert( + shouldPreserveLocalStateOnReconnect({ collabCanEdit: false, hasPendingLocalState: true }) === false, + 'Expected a read-only session never to replay local state', + ); + + console.log(' ✓ stalled-collab recovery preserves local marks instead of discarding them'); +} + +function scenarioRestFallbackCoversDeadProvider(): void { + // The exact field condition: capabilities still true, provider dead. + const deadProvider = { + collabEnabled: true, + collabCanEdit: true, + legacyRestFallback: false, + connectionStatus: 'disconnected' as const, + }; + assert( + shouldUseRestMarksFallback(deadProvider) === true, + 'Expected REST persistence when collab is nominally enabled but the provider is disconnected', + ); + + // Legacy rule, reproduced: capability flags alone decided, so this returned false. + const legacyDecision = !deadProvider.collabEnabled + || !deadProvider.collabCanEdit + || deadProvider.legacyRestFallback; + assert( + legacyDecision === false, + 'Reference implementation should reproduce the defect by skipping REST on a dead provider', + ); + + assert( + shouldUseRestMarksFallback({ ...deadProvider, connectionStatus: 'connected' }) === false, + 'Expected no REST write while a live provider owns the document, to avoid racing Yjs', + ); + assert( + shouldUseRestMarksFallback({ ...deadProvider, connectionStatus: 'connecting' }) === true, + 'Expected REST persistence while the provider is still connecting', + ); + assert( + shouldUseRestMarksFallback({ ...deadProvider, collabEnabled: false }) === true, + 'Expected REST persistence when collab is disabled outright', + ); + + console.log(' ✓ a dead provider falls back to REST without racing a live one'); +} + +function scenarioAuthFailureClassification(): void { + assert( + classifyCollabAuthFailure('Invalid or expired collab session token') === 'expired', + 'Expected the observed 4401 close reason to classify as expired', + ); + assert( + classifyCollabAuthFailure('session-stale') === 'expired', + 'Expected a stale access epoch to classify as refreshable', + ); + assert( + classifyCollabAuthFailure('permission-denied') === 'permission-denied', + 'Expected an explicit denial to classify as permission-denied', + ); + assert( + classifyCollabAuthFailure(undefined) === 'unknown', + 'Expected a missing reason to classify as unknown', + ); + + console.log(' ✓ auth-failure reasons classify into a refresh decision'); +} + +function scenarioCommentNotReportedSavedUntilAcked(): void { + const clock = fakeClock(1_760_000_000_000); + const markAt = clock.now(); + + assert( + areMarksDurablyAcknowledged({ + connectionStatus: 'disconnected', + isSynced: false, + unsyncedChanges: 1, + pendingLocalUpdates: 0, + lastRestMarksAckAt: null, + lastLocalMarkAt: markAt, + }) === false, + 'Expected a comment on a dead provider never to report itself as saved', + ); + + assert( + areMarksDurablyAcknowledged({ + connectionStatus: 'connected', + isSynced: true, + unsyncedChanges: 1, + pendingLocalUpdates: 0, + lastRestMarksAckAt: null, + lastLocalMarkAt: markAt, + }) === false, + 'Expected unsynced changes to keep a comment pending even on a live connection', + ); + + clock.advance(1_000); + assert( + areMarksDurablyAcknowledged({ + connectionStatus: 'disconnected', + isSynced: false, + unsyncedChanges: 1, + pendingLocalUpdates: 0, + lastRestMarksAckAt: clock.now(), + lastLocalMarkAt: markAt, + }) === true, + 'Expected a REST acknowledgement to durably settle a comment while offline', + ); + + assert( + areMarksDurablyAcknowledged({ + connectionStatus: 'connected', + isSynced: true, + unsyncedChanges: 0, + pendingLocalUpdates: 0, + lastRestMarksAckAt: null, + lastLocalMarkAt: markAt, + }) === true, + 'Expected a fully synced provider to durably settle a comment', + ); + + console.log(' ✓ comments stay pending until durably acknowledged'); +} + +function run(): void { + scenarioProactiveRenewalBeforeExpiry(); + scenarioTypingDoesNotDeferPastHardDeadline(); + scenarioUnsavedWorkDoesNotDeadlockRecovery(); + scenarioRenewalBackoffPreventsSpin(); + scenarioReconnectPreservesUnsavedComment(); + scenarioRestFallbackCoversDeadProvider(); + scenarioAuthFailureClassification(); + scenarioCommentNotReportedSavedUntilAcked(); + console.log('✓ collab session renewal keeps comments durable across token expiry'); +} + +run();