Skip to content

Commit 8f93e66

Browse files
committed
fix(files): close realEdited data-loss race + elect a settle writer
Independent audit surfaced two real gaps: - file-doc-store: realEdited was latched AFTER appendUpdate's awaits, but the edit already sits in room.doc synchronously. A concurrent agent-frame compaction could read realEdited=false, snapshot that real content, and stamp it a no-persist agent frame — a lost edit. Latch it synchronously (same tick as the doc mutation) before any await. Deterministic falsifiable test added. - rich-markdown-editor: at settle every tab applied the final body, and a non-leader's local microtask runs before the leader's final propagates, so both insert the tail (Yjs keeps both) -> duplicated tail. Elect a single settle writer (reliable — awareness is long converged by settle), reading leadership before clearing the announcement. Corrects the overclaiming idempotency comment and the handoff pick-up comment. Adds a y-tiptap internals upgrade-guardrail test.
1 parent 96fd287 commit 8f93e66

4 files changed

Lines changed: 75 additions & 23 deletions

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,23 @@ describe('FileDocStore', () => {
270270
bDoc.destroy()
271271
})
272272

273+
it('latches realEdited synchronously so a concurrent compaction can never mislabel a real edit', async () => {
274+
// The data-loss race: a real edit sits in room.doc synchronously, but if realEdited were set only
275+
// AFTER appendUpdate's awaits, a concurrent agent-triggered compaction could snapshot that content and
276+
// stamp it an agent (no-persist) frame — losing the edit. The latch must be set in the same tick.
277+
const a = await newStore()
278+
const doc = new Y.Doc()
279+
await a.attachRoom(NAME, doc)
280+
const room = (a as any).rooms.get(NAME)
281+
expect(room.realEdited).toBe(false)
282+
// Kick off a real (non-agent) append but do NOT await it: realEdited must already be true before the
283+
// xAdd/expire awaits resolve, so any compaction racing on the awaits sees the real edit.
284+
const pending = (a as any).appendUpdate(NAME, updateFor('real user edit'))
285+
expect(room.realEdited).toBe(true)
286+
await pending
287+
doc.destroy()
288+
})
289+
273290
it('stamps a compaction snapshot of an agent-ONLY stream as an agent frame (never persisted)', async () => {
274291
const streamKey = `filedoc:stream:${NAME}`
275292
const noop = Buffer.from(Y.encodeStateAsUpdate(new Y.Doc())).toString('base64')

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,17 @@ export class FileDocStore {
299299
*/
300300
private async appendUpdate(name: string, update: Uint8Array, agent = false): Promise<void> {
301301
if (!this.write) return
302+
// Latch realEdited SYNCHRONOUSLY — before the first await — for a real (non-agent) publish. The edit
303+
// already sits in room.doc (applied in doc.on('update') before publish was called), so if this set
304+
// were deferred past the xAdd/expire awaits a CONCURRENT agent-frame-triggered maybeCompact could read
305+
// realEdited=false, snapshot the doc (which already holds this real edit), and stamp it an agent
306+
// (no-persist) snapshot — a lost edit. Setting it in the same synchronous tick as the doc mutation
307+
// makes "room.doc holds a real edit ⇒ realEdited" hold before any compaction (always async) can run.
308+
// Monotonic latch, so an eager set is safe; the seed never flows through here (it uses seedIfEmpty).
309+
if (!agent) {
310+
const editedRoom = this.rooms.get(name)
311+
if (editedRoom) editedRoom.realEdited = true
312+
}
302313
const encoded = Buffer.from(update).toString('base64')
303314
const fields: Record<string, string> = { [UPDATE_FIELD]: encoded }
304315
if (agent) fields[AGENT_FIELD] = '1'
@@ -318,13 +329,7 @@ export class FileDocStore {
318329
}
319330
await this.write.expire(streamKey(name), STREAM_TTL_SEC).catch(() => {})
320331
const room = this.rooms.get(name)
321-
if (room) {
322-
// A local non-agent publish (a user edit or the awaited copilot durable merge) is a real edit; the
323-
// seed never flows through here (it uses seedIfEmpty). Set it BEFORE the compaction check below so a
324-
// real edit can never be folded into an agent (no-persist) snapshot due to a tail-back race.
325-
if (!agent) room.realEdited = true
326-
if (++room.publishes % COMPACT_CHECK_EVERY === 0) void this.maybeCompact(name)
327-
}
332+
if (room && ++room.publishes % COMPACT_CHECK_EVERY === 0) void this.maybeCompact(name)
328333
}
329334

330335
/**

apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/apply-streamed-markdown.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* @vitest-environment jsdom
33
*/
44
import { Editor } from '@tiptap/core'
5+
import { initProseMirrorDoc, updateYFragment, ySyncPluginKey } from '@tiptap/y-tiptap'
56
import { afterEach, beforeAll, describe, expect, it } from 'vitest'
67
import { Awareness } from 'y-protocols/awareness'
78
import * as Y from 'yjs'
@@ -49,6 +50,18 @@ function track(t: { editor: Editor; doc: Y.Doc; awareness: Awareness }) {
4950
}
5051

5152
describe('agent-stream applier', () => {
53+
it('relies on y-tiptap internals that still exist (upgrade guardrail)', () => {
54+
// beginAgentStream/applyAgentStreamFrame reach into y-tiptap internals (not public TipTap API):
55+
// `ySyncPluginKey`, `updateYFragment`, `initProseMirrorDoc`. A y-tiptap bump that renames or drops
56+
// any of them can pass typecheck yet break at runtime — assert their runtime shape here so an upgrade
57+
// fails loudly at test time instead of in production. Pinned to an exact y-tiptap version in
58+
// package.json; bump that pin and this guard together.
59+
expect(typeof updateYFragment).toBe('function')
60+
expect(typeof initProseMirrorDoc).toBe('function')
61+
expect(ySyncPluginKey).toBeDefined()
62+
expect(typeof ySyncPluginKey.getState).toBe('function')
63+
})
64+
5265
it('streams agent content into the live collaborative doc and broadcasts it as Yjs ops', () => {
5366
const { editor, doc } = track(makeCollabEditor())
5467

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

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -908,7 +908,10 @@ export function LoadedRichMarkdownEditor({
908908
// Single-writer election: only the leader (min clientID among clients announcing they apply this
909909
// stream) writes it into the shared doc, so multiple tabs/windows watching the same live copilot
910910
// stream don't each insert it and duplicate content. A non-leader renders the leader's ops via
911-
// Yjs; re-checked each frame, so it converges to one writer the moment awareness propagates.
911+
// Yjs; re-checked each frame, so a co-leader stops the moment awareness propagates. (The pick-up
912+
// direction — a successor beginning to write after the leader tab closes — waits for the next
913+
// content frame to run a tick; a stream that already delivered its last frame is covered by
914+
// settle and the durable write, so at worst a brief end-of-stream display lag, never a loss.)
912915
// Bounded residual (accepted): if two tabs start the SAME stream within the awareness-propagation
913916
// window they briefly both lead and duplicate a frame or two — a rare, transient, never-persisted
914917
// glitch (SYNC_NO_PERSIST keeps it out of storage; the durable edit_content write reconciles the
@@ -962,27 +965,41 @@ export function LoadedRichMarkdownEditor({
962965
cancelAnimationFrame(streamRafRef.current)
963966
streamRafRef.current = null
964967
}
965-
// Settle: apply the FINAL body so the Y.Doc exactly equals the streamed result. The mid-stream
966-
// leader REUSES its up-to-date shadow (just catching a throttled last frame); a client that never
967-
// applied mid-stream (a non-leader, a held `update`, or a pre-seed stream) opens a FRESH shadow
968-
// seeded from the CURRENT doc. Reconciling current→final is idempotent — a client that settles after
969-
// another already wrote the final reconciles to a noop — so there is NO settle-time election and no
970-
// base-shadow duplication, and a lone client (incl. an `update`) still applies rather than waiting on
971-
// the durable merge. That durable `edit_content` write then lands as a noop diff too.
968+
// Settle: apply the FINAL body so the Y.Doc exactly equals the streamed result — but ONLY the
969+
// elected writer applies it (the same min-clientID election the streaming tick uses). Without this,
970+
// N tabs watching one run each open a fresh shadow and reconcile current→final; a non-leader's local
971+
// settle microtask runs BEFORE the leader's final propagates (a server round-trip), so both insert
972+
// the same tail and Yjs keeps both (it does not dedupe identical text from two clients) → a
973+
// duplicated tail. The election is reliable here (unlike the bounded startup window): the stream ran
974+
// for seconds, so awareness is long converged. Each tab reads leadership BEFORE clearing its own
975+
// announcement — a remote clear is a network round-trip, always slower than these local microtasks,
976+
// so every tab sees the same announcer set and agrees on one leader. The leader reuses its
977+
// up-to-date shadow (catching a throttled last frame) or, if it never applied mid-stream (a held
978+
// `update`, or a pre-seed stream), opens a FRESH shadow from the current doc; a non-leader applies
979+
// nothing (the leader's final broadcasts to it) and frees any shadow it still held. The durable
980+
// `edit_content` write then lands as a noop diff for everyone.
972981
if (wasStreamingRef.current && collabReady) {
973982
wasStreamingRef.current = false
974983
agentAnnouncedRef.current = false
984+
const isSettleWriter =
985+
!collaboration || isAgentStreamLeader(collaboration.awareness, collaboration.doc.clientID)
975986
if (collaboration) clearAgentApplying(collaboration.awareness)
976987
lastStreamedBodyRef.current = null
977-
const finalBody = splitFrontmatter(content).body
978-
const session = agentStreamSessionRef.current ?? beginAgentStream(editor)
988+
const heldSession = agentStreamSessionRef.current
979989
agentStreamSessionRef.current = null
980-
if (session) {
981-
runOffRender(() => applyAgentStreamFrame(editor, session, finalBody))
982-
// Free the shadow with an UNGUARDED microtask (not `runOffRender`): a rapid follow-up stream
983-
// can supersede the run token and drop the apply above, but the shadow must always be
984-
// destroyed. Queued after the apply, so it frees the shadow only once that has had its chance.
985-
queueMicrotask(() => endAgentStream(session))
990+
if (isSettleWriter) {
991+
const finalBody = splitFrontmatter(content).body
992+
const session = heldSession ?? beginAgentStream(editor)
993+
if (session) {
994+
runOffRender(() => applyAgentStreamFrame(editor, session, finalBody))
995+
// Free the shadow with an UNGUARDED microtask (not `runOffRender`): a rapid follow-up stream
996+
// can supersede the run token and drop the apply above, but the shadow must always be
997+
// destroyed. Queued after the apply, so it frees the shadow only once that has had its chance.
998+
queueMicrotask(() => endAgentStream(session))
999+
}
1000+
} else if (heldSession) {
1001+
// Non-leader: it never writes the final (the leader does + broadcasts it); free any shadow it held.
1002+
queueMicrotask(() => endAgentStream(heldSession))
9861003
}
9871004
}
9881005
return

0 commit comments

Comments
 (0)