fix: wrap concurrent operations in transactions for atomicity#495
Open
fix: wrap concurrent operations in transactions for atomicity#495
Conversation
- Content update: move _rev optimistic concurrency check inside transaction so read-then-write is atomic (H1) - Menu reorder: wrap sequential item updates in withTransaction to prevent partial reorder on crash (M4) - Byline delete: replace db.transaction() with withTransaction() for D1 compatibility (M7) - Seed content: wrap each entry creation/update + bylines + taxonomies in a per-entry transaction (M17)
Addresses adversarial review feedback: - Hoist findById to serve both _rev check and old slug capture (was called twice) - Remove unused HTTP status from thrown apiError objects (status is derived from error code downstream)
🦋 Changeset detectedLatest commit: 4cc3cb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 4cc3cb7 | Apr 12 2026, 06:53 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves write-atomicity across core operations by standardizing on withTransaction() for multi-step DB updates, aiming to prevent partially-applied changes during concurrent writes or mid-operation crashes.
Changes:
- Wrap content update flow (including
_revoptimistic concurrency check) in a transaction and dedupe redundant reads. - Make menu item reorder updates atomic by performing the full reorder + re-read inside
withTransaction(). - Use
withTransaction()for byline deletion (and seed content writes) to improve D1 compatibility and reduce partial-write risk.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/seed/apply.ts | Wrap per-entry content create/update + bylines + taxonomies in withTransaction() for atomic seed application. |
| packages/core/src/database/repositories/byline.ts | Use withTransaction() for byline delete to improve atomicity and D1 compatibility. |
| packages/core/src/api/handlers/menus.ts | Wrap reorder loop + fetch in withTransaction() to avoid partial reorders. |
| packages/core/src/api/handlers/content.ts | Move _rev check into the transactional update path and avoid duplicate findById calls. |
| .changeset/poor-feet-watch.md | Adds a patch changeset documenting the atomicity fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+507
to
+515
| // The _rev check is inside the transaction so the read-then-write | ||
| // is atomic -- no concurrent write can slip between the check and update. | ||
| const item = await withTransaction(db, async (trx) => { | ||
| const trxRepo = new ContentRepository(trx); | ||
| const bylineRepo = new BylineRepository(trx); | ||
|
|
||
| // Read existing item once for both _rev check and old slug capture | ||
| const existing = | ||
| body._rev || body.slug ? await trxRepo.findById(collection, resolvedId) : null; |
There was a problem hiding this comment.
Placing the _rev check inside a transaction doesn’t fully prevent lost updates on databases with READ COMMITTED / snapshot isolation unless the row is locked (e.g. SELECT ... FOR UPDATE) or the subsequent UPDATE is conditional on the expected version/updated_at. As written, another transaction can still commit an update after findById and before update, and this handler will overwrite it while still passing the earlier _rev check. Consider either locking the row when reading for the _rev check or extending ContentRepository.update to accept an expected rev and include it in the WHERE clause, returning a CONFLICT when zero rows are affected.
Suggested change
| // The _rev check is inside the transaction so the read-then-write | |
| // is atomic -- no concurrent write can slip between the check and update. | |
| const item = await withTransaction(db, async (trx) => { | |
| const trxRepo = new ContentRepository(trx); | |
| const bylineRepo = new BylineRepository(trx); | |
| // Read existing item once for both _rev check and old slug capture | |
| const existing = | |
| body._rev || body.slug ? await trxRepo.findById(collection, resolvedId) : null; | |
| // Lock the current row when reading it for _rev validation / slug capture | |
| // so no concurrent writer can update it between the check and the write. | |
| const item = await withTransaction(db, async (trx) => { | |
| const trxRepo = new ContentRepository(trx); | |
| const bylineRepo = new BylineRepository(trx); | |
| // Read and lock the existing item once for both _rev check and old slug capture. | |
| // This prevents lost updates under READ COMMITTED / snapshot isolation. | |
| const existing = | |
| body._rev || body.slug | |
| ? (((await sql<ContentItem>` | |
| select * | |
| from ${sql.table(collection)} | |
| where id = ${resolvedId} | |
| for update | |
| `.execute(trx)).rows[0] as ContentItem | undefined) ?? null | |
| : null; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes four atomicity gaps identified in the code quality review (findings H1, M4, M7, M17).
_revoptimistic concurrency check inside the transaction so the read-then-write is atomic. A concurrent write between the rev check and update can no longer succeed. Also deduplicated a redundantfindByIdcall when both_revandslugare provided.withTransaction()to prevent partial reorder if a crash occurs mid-loop.db.transaction().execute()withwithTransaction()for D1 compatibility (D1 doesn't support native transactions;withTransactionhandles the fallback).Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure