Skip to content

Commit d6cafad

Browse files
committed
fix(realtime): bound a client-requested flush to edits it has not written
A requested flush deliberately bypasses the cross-task dedup window, because a deduped no-op acked as success would reintroduce the staleness the flush exists to prevent. But room.edited is set on the first edit and never cleared, so every repeat still performed a full projection: a Yjs-to-markdown conversion, a fresh blob upload, and a delete of the previous key. A client emitting flush in a loop could drive that unbounded. Pairs a monotonic edit counter with the sequence the last successful persist covered, so a flush with nothing new to write acks unchanged instead. The sequence is captured before the projection and stored only on success, so an edit arriving mid-write stays pending and a conflict is never mistaken for a completed write.
1 parent c7ccee5 commit d6cafad

3 files changed

Lines changed: 212 additions & 1 deletion

File tree

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,63 @@ describe('setupWorkspaceFileDocHandlers', () => {
461461
}
462462
})
463463

464+
it('does not re-project edits the durable file already has', async () => {
465+
mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server'))
466+
mockFetchFileDocPersist.mockResolvedValue({ status: 'persisted', version: 9 })
467+
const { io } = createIo()
468+
const { handlers, socket } = setup('socket-1', io)
469+
await joinAndEdit(handlers)
470+
471+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
472+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
473+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
474+
475+
// `edited` never clears, so without an edit-sequence check each repeat would mint another blob
476+
// version. Only the first has anything to write; the rest are honest no-ops.
477+
expect(mockFetchFileDocPersist).toHaveBeenCalledTimes(1)
478+
expect(flushAcks(socket)).toEqual([
479+
{ fileId: 'file-1', status: 'persisted', version: 9 },
480+
{ fileId: 'file-1', status: 'unchanged' },
481+
{ fileId: 'file-1', status: 'unchanged' },
482+
])
483+
})
484+
485+
it('writes again once a new edit lands after a flush', async () => {
486+
mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server'))
487+
mockFetchFileDocPersist.mockResolvedValue({ status: 'persisted', version: 9 })
488+
const { io } = createIo()
489+
const { handlers } = setup('socket-1', io)
490+
await joinAndEdit(handlers)
491+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
492+
493+
const more = new Y.Doc()
494+
more.getText(FILE_DOC_FIELD).insert(0, 'and more typing')
495+
handlers[FILE_DOC_EVENTS.MESSAGE](
496+
frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) =>
497+
syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(more))
498+
)
499+
)
500+
await flushMicrotasks()
501+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
502+
503+
// The dedup must bound redundant writes without ever swallowing real edits.
504+
expect(mockFetchFileDocPersist).toHaveBeenCalledTimes(2)
505+
})
506+
507+
it('leaves the edits pending when a persist did not land', async () => {
508+
mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server'))
509+
mockFetchFileDocPersist.mockResolvedValue({ status: 'conflict' })
510+
const { io } = createIo()
511+
const { handlers } = setup('socket-1', io)
512+
await joinAndEdit(handlers)
513+
514+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
515+
await handlers[FILE_DOC_EVENTS.FLUSH]({ fileId: 'file-1' })
516+
517+
// A conflict wrote nothing, so the second attempt must NOT be deduped away as already-durable.
518+
expect(mockFetchFileDocPersist).toHaveBeenCalledTimes(2)
519+
})
520+
464521
it('refuses a flush for a file this socket never joined', async () => {
465522
mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server'))
466523
const { io } = createIo()

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

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,19 @@ interface FileDocRoom {
148148
* is last to leave — even one that only tailed the edits.
149149
*/
150150
edited: boolean
151+
/**
152+
* Monotonic count of edits applied here. Paired with {@link FileDocRoom.persistedEditSeq} to answer
153+
* "is there anything new to write?" — `edited` alone only answers "was this doc EVER edited", which
154+
* stays true forever and would let a client force an unbounded run of redundant blob versions by
155+
* repeatedly asking for a flush.
156+
*/
157+
editSeq: number
158+
/**
159+
* The {@link FileDocRoom.editSeq} value the last successful persist projected. Equal to `editSeq`
160+
* means the durable file already reflects every edit this room has seen. Captured BEFORE the
161+
* projection and stored only on success, so an edit arriving mid-persist is never marked durable.
162+
*/
163+
persistedEditSeq: number
151164
/** Whether this room has observed its doc become seeded — so a post-seed update counts as an edit but
152165
* the seed transition itself does not. See the `doc.on('update')` edit-tracking below. */
153166
seededObserved: boolean
@@ -308,6 +321,12 @@ async function flushPersist(
308321
// Never project a doc no user actually edited back over the file (see {@link FileDocRoom.edited}).
309322
// Nothing to write is not a failure — the durable content is already current.
310323
if (!room.edited || !room.workspaceId || !room.lastEditorUserId) return { status: 'unchanged' }
324+
// Nor re-project edits the durable file already has. `edited` never clears, so without this a
325+
// caller that can ask for a flush could force an unbounded run of identical blob versions — each
326+
// one a fresh upload plus a delete of the old key. Captured here, before any await, so an edit
327+
// landing mid-persist is compared against the value this projection actually covers.
328+
const projectedEditSeq = room.editSeq
329+
if (projectedEditSeq === room.persistedEditSeq) return { status: 'unchanged' }
311330
const store = getFileDocStore()
312331
const workspaceId = room.workspaceId
313332
const userId = room.lastEditorUserId
@@ -395,6 +414,9 @@ async function flushPersist(
395414
}
396415
if (result.status === 'persisted') {
397416
room.syncedVersion = Math.max(room.syncedVersion ?? 0, result.version)
417+
// Only the edits this projection actually carried. An edit that arrived while the write was in
418+
// flight keeps `editSeq` ahead, so the next flush still has something to do.
419+
room.persistedEditSeq = Math.max(room.persistedEditSeq, projectedEditSeq)
398420
void store.setSyncedVersion(name, result.version)
399421
return { status: 'persisted', version: result.version }
400422
}
@@ -771,6 +793,8 @@ function getOrCreateRoom(io: Server, ref: RoomRef): FileDocRoom {
771793
workspaceId: null,
772794
lastEditorUserId: null,
773795
edited: false,
796+
editSeq: 0,
797+
persistedEditSeq: 0,
774798
seededObserved: false,
775799
persistTimer: null,
776800
persistDeadline: null,
@@ -837,8 +861,10 @@ function getOrCreateRoom(io: Server, ref: RoomRef): FileDocRoom {
837861
originSocketId(origin) ||
838862
origin === REDIS_SNAPSHOT_ORIGIN ||
839863
(seededBefore && origin === REDIS_ORIGIN)
840-
)
864+
) {
841865
room.edited = true
866+
room.editSeq++
867+
}
842868
// Debounce a persist for LOCAL user edits only (peers debounce their own).
843869
if (originSocketId(origin)) schedulePersist(name, room)
844870
})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*
4+
* The flush hand-off between the editor (which owns the realtime provider) and the file-detail
5+
* header (which acts on it). The header renders the provider, so it owns the ref rather than
6+
* reading it through context — these cover that the published flush is actually reachable from
7+
* there, and stays reachable.
8+
*/
9+
import { act, type ReactNode, useRef } from 'react'
10+
import type { FlushFileDocResult } from '@sim/realtime-protocol/file-doc'
11+
import { createRoot, type Root } from 'react-dom/client'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
import {
14+
type FileDocFlush,
15+
FileDocRoomProvider,
16+
flushFileDocRef,
17+
useReportFileDocFlush,
18+
} from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-room-context'
19+
20+
beforeEach(() => {
21+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
22+
})
23+
24+
const PERSISTED: FlushFileDocResult = { fileId: 'file-1', status: 'persisted', version: 1 }
25+
26+
/**
27+
* Mounts an owner that passes its own ref into the provider — the shape `files.tsx` uses — with a
28+
* publisher underneath standing in for the editor. `read()` is what the header's retype handler does.
29+
*/
30+
function renderOwner(initialFlush: FileDocFlush | null) {
31+
const container = document.createElement('div')
32+
const root: Root = createRoot(container)
33+
let read: (() => Promise<FlushFileDocResult>) | null = null
34+
35+
function Publisher({ flush }: { flush: FileDocFlush | null }) {
36+
useReportFileDocFlush(flush)
37+
return null
38+
}
39+
40+
function Owner({ flush, children }: { flush: FileDocFlush | null; children?: ReactNode }) {
41+
const flushRef = useRef<FileDocFlush | null>(null)
42+
read = () => flushFileDocRef(flushRef)
43+
return (
44+
<FileDocRoomProvider flushRef={flushRef}>
45+
<Publisher flush={flush} />
46+
{children}
47+
</FileDocRoomProvider>
48+
)
49+
}
50+
51+
const render = (flush: FileDocFlush | null) => {
52+
act(() => {
53+
root.render(<Owner flush={flush} />)
54+
})
55+
}
56+
render(initialFlush)
57+
58+
return {
59+
render,
60+
read: () => {
61+
if (!read) throw new Error('owner did not render')
62+
return read()
63+
},
64+
unmount: () => act(() => root.unmount()),
65+
}
66+
}
67+
68+
describe('file-doc flush hand-off', () => {
69+
it('reaches the ancestor-owned ref, not just descendants', async () => {
70+
const flush = vi.fn(async () => PERSISTED)
71+
const owner = renderOwner(flush)
72+
73+
await expect(owner.read()).resolves.toEqual(PERSISTED)
74+
expect(flush).toHaveBeenCalledTimes(1)
75+
owner.unmount()
76+
})
77+
78+
it('resolves skipped when nothing collaborative is mounted', async () => {
79+
const owner = renderOwner(null)
80+
81+
// The caller needs no null check: a non-collaborative file must cost nothing and never throw.
82+
await expect(owner.read()).resolves.toMatchObject({ status: 'skipped' })
83+
owner.unmount()
84+
})
85+
86+
/**
87+
* The regression that motivated the stable-publish design. A flush bound to the provider's
88+
* identity republished on every socket churn, and a churn ending on `null` silently left the
89+
* header with nothing to call — degrading a retype back to reading pre-edit bytes.
90+
*/
91+
it('survives publisher churn that ends on a live flush', async () => {
92+
const first = vi.fn(async () => PERSISTED)
93+
const second = vi.fn(async () => PERSISTED)
94+
const owner = renderOwner(first)
95+
96+
owner.render(null)
97+
owner.render(second)
98+
99+
await expect(owner.read()).resolves.toEqual(PERSISTED)
100+
expect(second).toHaveBeenCalledTimes(1)
101+
expect(first).not.toHaveBeenCalled()
102+
owner.unmount()
103+
})
104+
105+
it('goes quiet once the publisher reports no flush', async () => {
106+
const flush = vi.fn(async () => PERSISTED)
107+
const owner = renderOwner(flush)
108+
109+
owner.render(null)
110+
111+
await expect(owner.read()).resolves.toMatchObject({ status: 'skipped' })
112+
expect(flush).not.toHaveBeenCalled()
113+
owner.unmount()
114+
})
115+
116+
it('stops resolving a torn-down publisher', async () => {
117+
const flush = vi.fn(async () => PERSISTED)
118+
const owner = renderOwner(flush)
119+
owner.unmount()
120+
121+
// Nothing to assert against the ref after unmount, but the published function must not be
122+
// retained by a later mount — a fresh owner starts empty.
123+
const next = renderOwner(null)
124+
await expect(next.read()).resolves.toMatchObject({ status: 'skipped' })
125+
expect(flush).not.toHaveBeenCalled()
126+
next.unmount()
127+
})
128+
})

0 commit comments

Comments
 (0)