Skip to content

Commit be2c9ad

Browse files
committed
fix(collab-doc): close review-round race/durability gaps
Address Greptile P1 + Cursor findings: - Seed publishes to the shared stream AWAITED *before* seeding the local doc, so a publish failure leaves the doc unseeded and the stream empty for a clean retry rather than serving an unpublished local seed a peer would re-seed over (split-brain). - flushPersist falls back to the synchronously-captured local snapshot when getStreamState throws (not only when it returns null), so a transient Redis read no longer drops the final durable write as the room is torn down. - streamHasContent fails CLOSED (returns true on xLen error): a Redis blip can no longer let the seed fence pass and double-seed. - Client collabReady initializes from the collaborative prop, so a collaborative editor never has a mount-window where client autosave could clobber the server write.
1 parent b528509 commit be2c9ad

4 files changed

Lines changed: 50 additions & 20 deletions

File tree

apps/realtime/src/handlers/file-doc-store.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,15 +268,20 @@ export class FileDocStore {
268268

269269
/**
270270
* Whether the file's stream already holds content — fences a seed apply against a peer that seeded
271-
* while this task held a (possibly stale) lock, so we never write a second seed (split-brain). False
272-
* when disabled or on error (the caller then proceeds under its lock, as before).
271+
* while this task held a (possibly stale) lock, so we never write a second seed (split-brain). Both
272+
* callers treat `true` as "do not seed", so this fails CLOSED: a Redis `xLen` error returns `true`
273+
* (cannot confirm the stream is empty → do not risk a double-seed). `false` only when genuinely empty,
274+
* or when disabled (single-replica, where seeding locally is always correct).
273275
*/
274276
async streamHasContent(name: string): Promise<boolean> {
275277
if (!this.enabled || !this.write) return false
276278
try {
277279
return (await this.write.xLen(streamKey(name))) > 0
278-
} catch {
279-
return false
280+
} catch (error) {
281+
logger.warn(`FileDocStore streamHasContent failed for ${name}`, {
282+
error: getErrorMessage(error),
283+
})
284+
return true
280285
}
281286
}
282287

apps/realtime/src/handlers/file-doc.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,8 @@ const FILE_DOC_FIELD = 'default'
121121

122122
/** Let a fire-and-forget `void ensureServerSeed(...)` chain settle (mock resolves synchronously). */
123123
async function flushMicrotasks(): Promise<void> {
124-
await Promise.resolve()
125-
await Promise.resolve()
124+
// Enough to drain the fire-and-forget seed chain (shouldSeed → fetch → fence → publish → apply).
125+
for (let i = 0; i < 8; i++) await Promise.resolve()
126126
}
127127

128128
/**

apps/realtime/src/handlers/file-doc.ts

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,19 @@ async function flushPersist(name: string, room: FileDocRoom, final: boolean): Pr
216216
try {
217217
if (!final && !(await store.acquirePersistSlot(name, FILE_DOC_TIMEOUTS.persistRequestMs)))
218218
return
219-
const docState = (store.enabled ? await store.getStreamState(name) : null) ?? localState
219+
let docState = localState
220+
if (store.enabled) {
221+
try {
222+
docState = (await store.getStreamState(name)) ?? localState
223+
} catch (streamError) {
224+
// A transient Redis read must NOT drop the write when we already hold a valid local snapshot —
225+
// else the last-disconnect flush loses the session's edits as the room is torn down.
226+
if (!localState) throw streamError
227+
logger.warn(`Stream state unavailable for file ${room.fileId}; persisting local snapshot`, {
228+
error: getErrorMessage(streamError),
229+
})
230+
}
231+
}
220232
if (!docState) return // nothing seeded/authoritative to persist yet
221233
await fetchFileDocPersist(room.workspaceId, room.fileId, room.lastEditorUserId, docState)
222234
} catch (error) {
@@ -332,18 +344,15 @@ async function ensureServerSeed(
332344
// Fence against a peer that seeded while we held a stale lock (a rare long stall): if the stream
333345
// already has content it is seeded, so never write a SECOND seed on top — that would split-brain.
334346
if (await store.streamHasContent(name)) return
335-
// SEED_ORIGIN → `doc.on('update')` fans the seed to THIS task's clients but does NOT publish it
336-
// (nor persist it — the seed is the file's current content). We publish it EXPLICITLY and AWAIT the
337-
// stream write below, so the seed is durably in the stream BEFORE the lock releases — then any later
338-
// seeder's `streamHasContent` fence is guaranteed to see it. Closes the fence's publish-after-release
339-
// gap that a fire-and-forget publish left open.
340-
if (update) Y.applyUpdate(room.doc, update, SEED_ORIGIN)
341-
else
342-
room.doc.transact(
343-
() => room.doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true),
344-
SEED_ORIGIN
345-
)
346-
await store.publishAndWait(name, Y.encodeStateAsUpdate(room.doc))
347+
// Build the seed (file content + seed flag, or just the flag for an empty/missing file), then
348+
// PUBLISH it to the shared stream AWAITED *before* seeding the local doc. The doc is marked seeded
349+
// only once the seed is durably shared — so if the publish fails we leave the doc unseeded and the
350+
// stream empty for a clean retry, rather than serving an unpublished local seed that a peer would
351+
// re-seed on top of (split-brain). SEED_ORIGIN keeps `doc.on('update')` from re-publishing it.
352+
const seedUpdate = update ?? emptySeedUpdate()
353+
await store.publishAndWait(name, seedUpdate)
354+
if (fileDocRooms.get(name) !== room || isDocSeeded(room.doc)) return
355+
Y.applyUpdate(room.doc, seedUpdate, SEED_ORIGIN)
347356
} catch (error) {
348357
logger.warn(`Server seed failed for file ${room.fileId} (workspace ${workspaceId})`, error)
349358
room.serverSeedStarted = false
@@ -352,6 +361,18 @@ async function ensureServerSeed(
352361
}
353362
}
354363

364+
/** The seed update for an empty/missing file: just the `initialContentLoaded` flag, so an empty doc
365+
* still reaches readiness (and its emptiness is durably shared like any seed). */
366+
function emptySeedUpdate(): Uint8Array {
367+
const doc = new Y.Doc()
368+
doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true)
369+
try {
370+
return Y.encodeStateAsUpdate(doc)
371+
} finally {
372+
doc.destroy()
373+
}
374+
}
375+
355376
/** Serializes live merges per file so overlapping calls never race the same doc (see below). */
356377
const fileDocMergeChains = new Map<string, Promise<unknown>>()
357378

apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,12 @@ export const RichMarkdownEditor = memo(function RichMarkdownEditor({
126126
* shared document to markdown server-side, so the client must never also autosave — a stale keystroke
127127
* saving over a server/copilot edit is exactly the clobber the server path closes. The child reports
128128
* the right value up via `onCollabReadyChange`.
129+
*
130+
* Initialize from the `collaborative` prop (NOT unconditionally `true`): a collaborative file must
131+
* start with autosave OFF, or a save could fire in the window before the child mounts and reports —
132+
* re-clobbering exactly what this closes. The child turns it on for the non-collaborative fallback.
129133
*/
130-
const [collabReady, setCollabReady] = useState(true)
134+
const [collabReady, setCollabReady] = useState(!collaborative)
131135

132136
const {
133137
content,

0 commit comments

Comments
 (0)