fix(core): CAS-guard the draft-revision pointer flip on concurrent saves#2130
Conversation
🦋 Changeset detectedLatest commit: a6ff8f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Could not push formatting changes to this fork. The contributor may have "Allow edits by maintainers" disabled. Please run the formatter locally: |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
CI format:check flagged this file after the PR emdash-cms#2130 diff shifted line wrapping. Reformatted with oxfmt, no logic change.
f501b03 to
5e6438f
Compare
CI format:check flagged this file after the PR emdash-cms#2130 diff shifted line wrapping. Reformatted with oxfmt, no logic change.
github-actions
Bot
added
area/ci
area/docs
needs-rebase
size/XL
cla: signed
and removed
size/M
labels
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Two saves to a collection with no draft yet (autosave + explicit save, or two concurrent explicit saves) both read draft_revision_id: null, then race an unconditional UPDATE to flip the pointer. Whichever lands last wins regardless of which read the fresher base data, so the other save's edit is silently discarded even though the API reports success. D1 has no multi-statement transactions, so the pointer-flip UPDATE is now a CAS keyed on the draft_revision_id read at the start of the attempt; a losing save retries against the fresh pointer (re-merging its edit on top) instead of clobbering it. Reported as a comment on emdash-cms#1158 (the original TipTap-dispatch cause of emdash-cms#1158 itself was already fixed by 0.28.1/0.29.0). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI format:check flagged this file after the PR emdash-cms#2130 diff shifted line wrapping. Reformatted with oxfmt, no logic change.
Branch was rebased against the fork's stale main; regenerating against the real emdash-cms/emdash main to match the PR's actual merge base and pick up upstream's own lockfile fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5e6438f to
786dfa8
Compare
|
All checks green, no scope-gate applies (bug fix, save-race repro from #1158). Marking ready for review. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is the right one for the reported race: D1 has no multi-statement transactions, so a compare-and-set on draft_revision_id keyed to the value read at the start of the attempt, plus bounded retries, makes the loser re-merge on top of the fresh draft instead of clobbering it. The test is well designed—it gates both saves at RevisionRepository.create so the interleaving is deterministic across drivers, and it asserts the observable behavior (both fields survive) rather than implementation details.
However, the retry bound leaves a small but real silent-data-loss path, and there are some unrelated/scope/convention items to clean up:
- Retry exhaustion still drops data silently. After three failed CAS attempts the loop exits with
draftStorageChanged === false. BecauseusesDraftRevisionsis alreadytrue, the followinghandleContentUpdatecall writes metadata only (data: undefined) and the method returns success even though the caller's edit never landed. The fix should detect exhaustion and return a conflict error instead of pretending the save succeeded. - Unrelated lockfile entry.
pnpm-lock.yamladdsdemos/emdash-dev-main, but that package does not exist in the repo, so it looks like an accidental commit. Scope discipline (AGENTS.md) says no drive-by changes. - Comment issue/PR references. AGENTS.md prohibits issue/PR references in code comments. The new test file's header and describe title reference
#1158and#1119. - Sibling restore path has the same race.
handleContentRestorestill does an unconditionalUPDATEofdraft_revision_id; the same CAS guard should be applied there as a follow-up.
None of these are catastrophic, but the silent-drop case should be addressed before merge.
Findings
-
[needs fixing]
packages/core/src/emdash-runtime.ts:2892-2893If the CAS
UPDATEloses the race for all three attempts, the loop exits withdraftStorageChangedstillfalse. BecauseusesDraftRevisionsis alreadytrue, the subsequenthandleContentUpdatecall updates only metadata (data: undefined) and the API returns success even though the caller's edit was never written. This keeps a bounded version of the same silent data loss the PR is meant to eliminate.Return a conflict error when the edit couldn't be applied instead of falling through to a metadata-only update.
} if ( !draftStorageChanged && processedData && Object.keys(processedData).length > 0 ) { return { success: false as const, error: { code: "DRAFT_SAVE_CONFLICT", message: "Could not save draft due to concurrent edits; please retry.", }, }; } } -
[needs fixing]
packages/core/tests/integration/database/content-update-save-race.test.ts:2-4AGENTS.md prohibits issue/PR references in code comments because they become stale narrative. The test header and describe title name
#1158and#1119; keep that context in the commit message and PR description, and leave the comment for future readers.* Concurrent draft-revision saves should not silently drop an edit. * * `EmDashRuntime.handleContentUpdate`'s draft-revision path is a * read-existing -> merge -> create-revision -> flip-pointer sequence. * Two concurrent saves that both read `draft_revision_id: null` before * either writes can both create a revision and race to flip the pointer; * the slower save's edit is silently discarded even though the API * reports success. -
[needs fixing]
pnpm-lock.yaml:471-504This adds an importer entry for
demos/emdash-dev-main, but that package does not exist in the repo (demos/only containscloudflare,playground,plugins-demo,postgres,preview,simple). It's unrelated to the save-race fix and appears to have been committed accidentally. Please remove it. -
[suggestion]
packages/core/src/emdash-runtime.ts:3292-3296Systemic follow-up:
handleContentRestorestill performs an unconditionalUPDATEofdraft_revision_id. Two concurrent restores can race the same way the save path did, and the loser will be silently discarded. Consider applying the same CAS-and-retry guard here (read the currentdraft_revision_id, flip only if it matches, and retry on conflict).
github-actions
Bot
added
review/awaiting-author
…xhaustion Addresses review on emdash-cms#2130: when all 3 CAS retries on the draft-revision pointer flip lose the race, the save now returns DRAFT_SAVE_CONFLICT instead of falling through to a metadata-only update that reported success while discarding the edit. Also drops issue/PR references from comments and removes an accidental pnpm-lock.yaml entry for a local-only demos/emdash-dev-main workspace member. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all "needs fixing" items:
Left the |
github-actions
Bot
added
review/needs-rereview
The DRAFT_SAVE_CONFLICT fix (previous commit) broke tests/integration/mcp/concurrency.test.ts: 10 concurrent updates to the same item now legitimately exhaust 3 CAS attempts with zero backoff, since all racers retry in lockstep and keep re-colliding on the same tick. That test only passed before because of the silent-drop bug it exposed -- unlucky racers were succeeding with a metadata-only update instead of erroring. Bumps MAX_ATTEMPTS to 8 and adds jittered backoff between retries so concurrent savers spread out instead of colliding every round. Verified tests/integration/mcp/concurrency.test.ts 5x in a row (it's inherently timing-sensitive) plus a full `vitest run` of packages/core -- the only failures are pre-existing Windows-environment flakes (tar/vite-config/ sqlite-file-persist), confirmed identical on a stash of this branch's changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed a follow-up: my previous commit broke `tests/integration/mcp/concurrency.test.ts` in CI — 10 concurrent updates to the same item now legitimately exhaust 3 CAS attempts with zero backoff (all racers retry in lockstep and keep re-colliding on the same tick). That test only passed before because of the exact silent-drop bug this PR fixes — unlucky racers were succeeding with a metadata-only update instead of erroring. Bumped `MAX_ATTEMPTS` to 8 and added jittered backoff between retries. Verified the concurrency test 5x in a row (it's timing-sensitive) plus a full core `vitest run` — the only remaining failures are pre-existing Windows-environment flakes (tar/vite-config/sqlite-file-persist), confirmed identical against a stash of this branch's changes. |
1 participant
What does this PR do?
Fixes a save race reported as a follow-up comment on closed issue #1158.
EmDashRuntime.handleContentUpdate's draft-revision path does read-existing → merge → create-revision → flip-pointer with no wrapping transaction (D1 has no multi-statement transactions) and an unconditional pointer-flipUPDATE. Two concurrent saves (autosave racing an explicit save, or two explicit saves) that both readdraft_revision_id: nullbefore either writes both create a new revision and race to flip the pointer — whicheverUPDATElands last wins regardless of which read the fresher data, so the other save's edit is silently discarded even though the API reports success. This matches the DCathal repro in the #1158 thread (two revisions created in the same second, the stale one becomesdraft_revision_id).Note: the original #1158 report (TipTap
editor.view.dispatchbypassingonUpdate) was already fixed by #1119 and is unaffected here — this addresses the separate, still-reproducing save race described in the newest comment on that thread (0.28.1/0.29.0).Fix: the pointer-flip
UPDATEis now a compare-and-set keyed on thedraft_revision_idread at the start of the attempt (COALESCE(draft_revision_id, '') = <value read>, NULL-safe across SQLite/D1 and Postgres). If the CAS fails, the losing save retries — re-reading the now-current draft, re-merging its own edit on top, and retrying the create+CAS — instead of silently overwriting. Bounded to 3 attempts. The orphaned revision row from a lost race is harmless;pruneOldRevisionssweeps it up on a later successful save since it's keyed byentry_id, not by pointer.Known residual, out of scope for this PR: the autosave-only "update existing draft revision in place" path (
skipRevisionwhen a draft already exists) still isn't CAS-guarded — two concurrent autosaves (no explicit save involved) can still last-write-wins on that single row'sdatacolumn. Closing that fully would need a version column onrevisions, which felt like more surface than this fix needs; flagging in case a maintainer disagrees.Closes (comment thread on) #1158
Type of change
Checklist
pnpm typecheckpasses (pre-existing unrelated errors inoauth-state-store.ts/plugins/context.tsconfirmed present without this change)pnpm lintpasses (oxlint, scoped to changed files)pnpm testpasses (targeted: new race test + adjacent content/revision/media-usage integration suites, both sqlite and postgres dialects)pnpm formathas been run (skipped locally — CRLF checkout makesformat:checkunreliable on Windows; will check CI)emdash)AI-generated code disclosure
Test plan
Added
packages/core/tests/integration/database/content-update-save-race.test.ts: creates a published entry, then fires two concurrenthandleContentUpdatecalls editing different fields, with aRevisionRepository.createspy that gates the first call until the second also arrives — forcing both to readdraft_revision_id: nullbefore either writes, regardless of driver timing. Verified the test fails against the pre-fix code (save B's edit silently dropped, matching the report) and passes with the fix (both edits survive).