Commit 74ca851
authored
feat(collab-doc): If-Match optimistic concurrency so persist never clobbers an out-of-band edit (#6085)
* feat(collab-doc): optimistic-concurrency guard so persist never clobbers an out-of-band edit
The relay projected the live Yjs doc back to durable markdown unconditionally (last-write-wins), so
a persist already in flight when an external write landed could overwrite it. Add RFC 7232 If-Match
optimistic concurrency end to end, reconciling through the CRDT (never rejecting user work):
- updateWorkspaceFileContent gains an expectedUpdatedAt guard: the write commits only if the file is
still at that version (checked against the SELECT ... FOR UPDATE-locked row, so it is atomic with
the write), else it throws the new ContentVersionConflictError without clobbering.
- persistFileDoc takes expectedVersion and returns a discriminated result (persisted | missing |
conflict). On conflict it returns the current durable content + version instead of writing.
- The relay tracks the durable version its live doc is synced to — set on seed, advanced when a
durable write is merged in (apply-edit carries the version), and on each successful persist. It is
held cluster-wide in Redis (filedoc:syncver:{name}) so whichever task persists reads the same
version, with the per-room value as the single-pod fallback.
- flushPersist sends that version as If-Match. On a conflict it merges the current durable content
into the live doc (so the out-of-band edit AND the live edits converge) and retries (bounded), so
even a last-leave flush racing an external write persists the reconciled result rather than losing
the session's edits.
Threads the version through the seed + persist contracts and the apply-edit payload. No schema change
(reuses workspace_files.updatedAt as the version token). Tests: app-side CAS (match writes, mismatch
throws + cleans up the orphan upload), relay conflict handled gracefully without clobber/loop; 236
realtime + 76 sim collab/uploads tests, tsc x2, lint, api-validation, boundaries, prune all green.
* chore(collab-doc): heartbeat-refresh the synced-version key TTL alongside its stream
Keep filedoc:syncver:{name} alive as long as the room's stream (it was only re-set on
seed/merge/persist), so an open-but-idle doc's persist If-Match token can't expire and force a
needless reconcile.
* fix(collab-doc): stop persist-conflict retries when there is no live doc to reconcile
On an If-Match conflict with no live doc to reconcile into (last collaborator gone, no shared
stream), applyMarkdownToLiveFileDoc returns no-live-room; re-projecting the same pre-teardown
snapshot would only re-conflict, so break the retry loop immediately and leave the out-of-band
(durable) content authoritative — the intended conflict policy. Addresses Greptile review.
* fix(collab-doc): close three optimistic-concurrency edge cases from review
- Single-pod persist retry projected the pre-reconcile snapshot (captureState always returned the
initial localState), while the synced version had been advanced by the reconcile — so the If-Match
could pass and clobber the reconciled edit. captureState now re-reads the live doc on each attempt
(falling back to the pre-teardown snapshot only once the room is gone).
- The synced version was recorded from this task's own seed FETCH before knowing whether this task's
seed actually won; a peer winning with a different version could leave a newer token than the stream
content. Record it only inside the didSeed branch (the task whose seed won); peer-seeded tasks read
the winner's cluster value.
- Persist wrote UNCONDITIONALLY when no version was available (relay version momentarily missing), which
could clobber non-empty durable content. It now returns conflict for a non-empty file with no version
(reconcile/retry once the version is re-established); an empty file's first write stays unconditional.
* fix(collab-doc): defer (not reconcile) on missing version, and use the freshest version token
- Missing-version persist now returns 'deferred' instead of 'conflict'. A missing version token (a
Redis blip on a peer-seeded task) is NOT a genuine out-of-band change, so triggering a reconcile
would wipe live edits (incoming-wins) even though nothing changed durably. Deferred means: don't
write, don't reconcile — leave the edits in the stream and let a later persist write them once the
version is re-established.
- currentVersion now takes the MAX of the cluster (Redis) and local room versions rather than always
preferring Redis, so a lagged/failed fire-and-forget Redis set can't shadow a newer local value and
cause spurious If-Match conflicts. Versions are monotonic epoch-ms, so the larger is the later sync.
* fix(collab-doc): make persist If-Match teardown-race-immune and recover missing version on final flush
Close two last-leave concurrency holes Cursor flagged:
- Thread the reconciled version LOCALLY through the persist retry loop. After a
conflict+reconcile the correct next If-Match is exactly result.version, so carry
it in a local var instead of re-deriving from room.syncedVersion/Redis. On a
last-leave flush destroyRoomIfIdle removes the room from the map before the async
flush finishes, so mergeMarkdownIntoRoom's recordVersion can no longer update
room.syncedVersion — threading makes each retry's precondition correct by
construction, immune to that dropped mutation and to a best-effort Redis re-read.
- Cache the resolved version back into room.syncedVersion in currentVersion() so a
peer-seeded/tail-only task (which never sets it locally) or a later transient
Redis read failure still resolves it from the last value seen (monotonic max,
never regresses).
- On a FINAL flush, briefly retry resolving the If-Match when the version read
momentarily fails, rather than deferring and stranding the session's edits in the
TTL'd stream — the version is cluster-wide and heartbeat-refreshed.
* fix(collab-doc): stamp cluster sync version the moment the seed wins, before the liveness guard
The winning seeder set the If-Match token (room + Redis filedoc:syncver) only after the
liveness/seeded guard that follows seedIfEmpty. But the tailer can integrate the just-appended
seed DURING the seedIfEmpty await, so isDocSeeded(room.doc) is already true when the guard runs
and it returns early — leaving the stream holding seed content with no cluster version. Later
persists then send no If-Match, the app returns `deferred`, and session edits stay only in the
TTL'd stream (the exact stranding this PR prevents elsewhere).
Move the version stamp to immediately after seedIfEmpty wins, before the guard. Recording it only
once our seed won (not from the fetch) is preserved, so it still can't shadow a peer's winning
seed.
* fix(collab-doc): make the synced-version token monotonic at every write site
The If-Match token is written fire-and-forget from the seed stamp, merges, and persists, both
locally and to Redis. An out-of-order write (e.g. a seed's lagged setSyncedVersion landing after a
later merge's) could regress it below the version the live doc already incorporates, causing
spurious If-Match conflicts — and on a last-leave flush with no live room to reconcile into, a
spurious conflict leaves durable authoritative and drops the session's edits.
- setSyncedVersion now writes via SET_VERSION_IF_NEWER_SCRIPT (Redis-side compare-and-set): it
overwrites only when the new value is greater, refreshing the TTL either way.
- recordVersion / the persisted branch / the seed stamp all take Math.max instead of assigning
room.syncedVersion directly.
Versions are monotonic epoch-ms, so "newer" is a plain numeric compare, exact within a Lua double.
* fix(collab-doc): close three last-leave persist edge cases from review
- Stale snapshot after reconcile (High): the multi-task captureState fell back to the pre-await
localState snapshot even after a reconcile advanced ifMatch, so a failed stream re-read could
persist the pre-reconcile state against the new version and clobber the out-of-band edit the
reconcile just incorporated. NULL localState after a reconcile so a failed read aborts instead.
- Lock miss aborts reconcile (Medium): a merge-lock acquisition failure returned 'no-live-room',
indistinguishable from an absent stream, so flushPersist treated transient contention as
terminal. Return a distinct 'merge-unavailable' and handle it as retry-later (edits stay in the
stream), never as "nothing to reconcile into".
- Peer syncver never recovers (Medium): the winner's setSyncedVersion was fire-and-forget with
swallowed errors — the only way a peer-seeded task learns the durable version — so a dropped
write left that peer deferring forever. Make it retry (bounded) like appendUpdate/seedIfEmpty;
the monotonic script keeps a racing retry a no-op.
* fix(collab-doc): scope the persist If-Match to a content version so metadata bumps can't clobber edits
The optimistic-concurrency validator was `updatedAt`, which rename/move/delete/restore also bump
with no content change. A racing live-doc persist then saw a stale token, got `conflict`,
reconciled the pre-edit durable body via updateYFragment (incoming-wins on overlap), and wiped the
user's in-flight edits.
Scope the validator to content (RFC 7232 semantics — validate the representation, not the row):
- New `workspace_files.content_updated_at` (NOT NULL, `now()` fast-default — no table rewrite).
Advances ONLY on content writes (upload / overwrite / create); metadata writes never touch it.
- The FOR UPDATE CAS, the merge-notify version, and the seed version all use `content_updated_at`.
A rename now leaves it unchanged, so the persist If-Match still matches -> no spurious conflict,
no reconcile, no lost edits. Genuine out-of-band content writes still conflict and reconcile.
- Consolidated the collab schema into one migration (the collab-state table + the new column) per
request, rather than a separate follow-up migration.
Relay/store/contracts unchanged (still a numeric monotonic version).
* chore(collab-doc): condense the densest persist comments (no behavior change)
Cleanup pass: tighten the three longest comment blocks added while hardening the persist path
(currentVersion cache, ifMatch threading, final-flush version retry) without dropping any invariant.
No dead code found (biome lint clean; all new symbols referenced).
* fix(collab-doc): persist must return the content version, not updatedAt
Follow-up to the content-scoped If-Match: persistFileDoc still returned `updatedAt` as the version
in both the persisted and conflict results, while the CAS/seed/merge all guard on
`content_updated_at`. A content write sets both to the same instant, so it was coincidentally
correct — until they diverge: if a metadata write bumps `updatedAt` past `content_updated_at`, the
conflict path returned the larger `updatedAt`, so the relay's re-persist sent an If-Match the CAS
(which checks `content_updated_at`) could never match → perpetual conflict → dropped reconciled
edits. Return `contentUpdatedAt` in both paths so the relay's token always matches what it's checked
against.
* fix(collab-doc): defer persist whenever the version is missing; guard the content-version test
- Empty-file CAS race (Medium): the unconditional-write carve-out for size===0 read `record.size`
outside the write transaction, so a concurrent first content write could land after the check and
be clobbered. With content_updated_at NOT NULL every existing file always has a real version, so a
missing expectedVersion is always transient — always defer, never write unconditionally. Removes
the TOCTOU hole.
- Content-version test (Low): the merge-chokepoint test kept updatedAt == contentUpdatedAt, so it
passed even if wired to the wrong field. Mock distinct values and assert contentUpdatedAt, so a
regression to updatedAt now fails the test.
* fix(collab-doc): don't reconcile a conflict the live doc already reflects (would wipe newer edits)
flushPersist reconciled the durable body into the live doc on every conflict. But when the conflict
comes from a racing self-persist (or an apply-edit the chokepoint already merged), the durable body
is a STALE SUBSET of the live stream, and the incoming-wins updateYFragment merge moves the doc
backward — wiping newer in-flight edits, which the retry then persists.
Before reconciling, re-check the freshest synced version. If it already covers the conflict version,
the live doc has already incorporated that content (or is ahead), so skip the reconcile and just retry
with the freshest version as If-Match — the re-projection captures the current live stream, preserving
every edit. Only a genuine out-of-band change the live doc hasn't incorporated (freshest < conflict
version) is reconciled in. freshest never exceeds the durable version, so this can't loop.
* fix(collab-doc): make content_updated_at monotonic per file; skip-reconcile can't loop
The If-Match token was stamped with app-local new Date() on each content write, so cross-instance
clock skew could stamp a later write with an EARLIER content_updated_at — breaking the version
ordering the whole optimistic-concurrency scheme (and the skip-reconcile branch's freshest>=version
assumption) depends on. Under skew the relay's monotonic syncedVersion could exceed the durable
version, sticking the If-Match: persist conflicts forever, exhausts retries, drops the session's edits.
- Stamp content_updated_at strictly after the current committed value (we hold the row's FOR UPDATE
lock): new Date(max(now, currentFile.contentUpdatedAt + 1ms)). Monotonic per file regardless of
clocks; also removes same-millisecond collisions. updatedAt stays plain wall-clock (display/sort).
- Skip-reconcile branch retries with result.version (the durable value the CAS will match), never
freshest (which could exceed it and loop). Belt-and-suspenders now that the version is monotonic.
* refactor(collab-doc): drop the destructive in-persist reconcile; adopt-version-and-retry on conflict
The in-persist reconcile projected the durable body back over the live doc via updateYFragment
("make the doc match"). That is destructive: when the live stream is already ahead — the common case,
because the write chokepoint (mergeEditIntoLiveFileDoc) already merged the out-of-band change into the
stream — it moved the doc backward and wiped newer in-flight edits. This produced a run of races
(stale snapshot, wipe-newer-edits, version-lag skip miss) that a full-document reconcile fundamentally
can't avoid, since deciding when it's safe relies on a laggy cross-task version token.
Remove it. On conflict, adopt the durable version as the new If-Match and retry: captureState re-reads
the current stream (which holds the out-of-band change AND the live edits), so the re-projection
persists the converged result. The durable change reaches the live doc via the chokepoint, never here.
Trade-off: the only unmerged out-of-band write is one whose chokepoint merge itself failed (rare,
logged), which we accept over the frequent reconcile-wipes-edits race.
- flushPersist: conflict -> ifMatch = result.version, retry (bounded). No applyMarkdownToLiveFileDoc.
- conflict response drops `markdown` (contract + relay type + persist) — no body needed, saves a blob
fetch. applyMarkdownToLiveFileDoc stays (still used by the apply-edit route / the chokepoint).
* fix(collab-doc): don't let a last-leave conflict retry clobber via the stale local snapshot
Regression from dropping the reconcile: on conflict the retry adopts result.version and re-reads
captureState. But after single-pod last-leave teardown the room is already destroyed, so captureState
falls back to the pre-teardown localState (which lacks the out-of-band change); the retry then CAS-passes
and overwrites the committed external write — undoing the external-wins last-leave policy.
Null localState on the first conflict, so the retry can only use freshly-read authoritative state
(stream / live doc). When none is available (single-pod room gone, or a transient stream-read failure)
captureState returns null and the retry stops, leaving durable content authoritative. Covers both the
single-pod and multi-task-stream-unavailable variants of the stale-snapshot clobber.
* fix(collab-doc): stop (don't re-persist) on a persist conflict — closes the commit-window clobber
The conflict retry adopted the durable version and immediately re-persisted the current stream,
assuming the stream already held the out-of-band change. But an external write commits durable BEFORE
its chokepoint merge (mergeEditIntoLiveFileDoc) reaches the stream, so a persist landing in that window
CAS-passed with a stream that still lacked the external content and clobbered the committed write — not
just the rare merge-failed path, but a race on every external write, worst at last-leave flushes.
Make persist a single attempt: on conflict, STOP and leave durable authoritative. The chokepoint merges
the change into the stream and — only once it is actually there — advances the synced version via its own
recordVersion; a later flush (debounced or final) then projects the converged stream with a matching
token. The session's edits stay in the stream meanwhile. The conflict handler deliberately does NOT
advance the synced version, or the next flush would clobber with a still-behind stream. Removes the retry
loop and PERSIST_CONFLICT_RETRIES.1 parent 525c340 commit 74ca851
19 files changed
Lines changed: 633 additions & 109 deletions
File tree
- apps
- realtime/src
- handlers
- routes
- sim
- app/api/internal/file-doc
- persist
- seed
- lib
- api/contracts
- collab-doc
- realtime
- uploads/contexts/workspace
- packages/db
- migrations
- meta
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
| 29 | + | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
38 | | - | |
| 38 | + | |
39 | 39 | | |
40 | | - | |
41 | | - | |
42 | | - | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
43 | 43 | | |
44 | | - | |
| 44 | + | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
| 47 | + | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
| |||
73 | 73 | | |
74 | 74 | | |
75 | 75 | | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
76 | 89 | | |
77 | 90 | | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
82 | 95 | | |
83 | 96 | | |
84 | 97 | | |
85 | 98 | | |
86 | 99 | | |
87 | | - | |
88 | | - | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
89 | 103 | | |
90 | 104 | | |
91 | | - | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
92 | 112 | | |
93 | 113 | | |
94 | 114 | | |
95 | 115 | | |
96 | 116 | | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
97 | 127 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
64 | 76 | | |
65 | 77 | | |
66 | 78 | | |
| |||
79 | 91 | | |
80 | 92 | | |
81 | 93 | | |
| 94 | + | |
| 95 | + | |
82 | 96 | | |
83 | 97 | | |
84 | 98 | | |
| |||
444 | 458 | | |
445 | 459 | | |
446 | 460 | | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
447 | 511 | | |
448 | 512 | | |
449 | 513 | | |
| |||
534 | 598 | | |
535 | 599 | | |
536 | 600 | | |
| 601 | + | |
| 602 | + | |
| 603 | + | |
537 | 604 | | |
538 | 605 | | |
539 | 606 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
130 | 130 | | |
131 | 131 | | |
132 | 132 | | |
133 | | - | |
| 133 | + | |
134 | 134 | | |
135 | 135 | | |
136 | 136 | | |
137 | | - | |
| 137 | + | |
138 | 138 | | |
139 | 139 | | |
140 | 140 | | |
| |||
189 | 189 | | |
190 | 190 | | |
191 | 191 | | |
| 192 | + | |
| 193 | + | |
192 | 194 | | |
193 | 195 | | |
194 | 196 | | |
| |||
262 | 264 | | |
263 | 265 | | |
264 | 266 | | |
265 | | - | |
| 267 | + | |
266 | 268 | | |
267 | 269 | | |
268 | 270 | | |
| |||
276 | 278 | | |
277 | 279 | | |
278 | 280 | | |
279 | | - | |
| 281 | + | |
280 | 282 | | |
281 | 283 | | |
282 | 284 | | |
| |||
297 | 299 | | |
298 | 300 | | |
299 | 301 | | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
300 | 333 | | |
301 | | - | |
| 334 | + | |
302 | 335 | | |
303 | 336 | | |
304 | 337 | | |
| |||
324 | 357 | | |
325 | 358 | | |
326 | 359 | | |
327 | | - | |
| 360 | + | |
328 | 361 | | |
329 | 362 | | |
330 | 363 | | |
| |||
360 | 393 | | |
361 | 394 | | |
362 | 395 | | |
363 | | - | |
| 396 | + | |
364 | 397 | | |
365 | 398 | | |
366 | 399 | | |
| |||
370 | 403 | | |
371 | 404 | | |
372 | 405 | | |
373 | | - | |
| 406 | + | |
374 | 407 | | |
375 | 408 | | |
376 | 409 | | |
| |||
401 | 434 | | |
402 | 435 | | |
403 | 436 | | |
404 | | - | |
| 437 | + | |
405 | 438 | | |
406 | 439 | | |
407 | 440 | | |
| |||
428 | 461 | | |
429 | 462 | | |
430 | 463 | | |
431 | | - | |
| 464 | + | |
432 | 465 | | |
433 | 466 | | |
434 | 467 | | |
| |||
437 | 470 | | |
438 | 471 | | |
439 | 472 | | |
440 | | - | |
| 473 | + | |
441 | 474 | | |
442 | 475 | | |
443 | 476 | | |
| |||
447 | 480 | | |
448 | 481 | | |
449 | 482 | | |
450 | | - | |
| 483 | + | |
451 | 484 | | |
452 | 485 | | |
453 | 486 | | |
| |||
461 | 494 | | |
462 | 495 | | |
463 | 496 | | |
464 | | - | |
| 497 | + | |
465 | 498 | | |
466 | 499 | | |
467 | 500 | | |
| |||
478 | 511 | | |
479 | 512 | | |
480 | 513 | | |
481 | | - | |
| 514 | + | |
482 | 515 | | |
483 | 516 | | |
484 | 517 | | |
| |||
511 | 544 | | |
512 | 545 | | |
513 | 546 | | |
514 | | - | |
| 547 | + | |
515 | 548 | | |
516 | 549 | | |
517 | 550 | | |
| |||
0 commit comments