Bug: Concurrent updates can bypass optimistic locking and silently overwrite each other #20

New issue

Closed

opened 2026-10-02 16:40:26 +00:00 by araemer · 1 comment

araemer commented

2026-10-02 16:40:26 +00:00

Owner

AbstractRepository.update() (src/repositories/AbstractRepository.ts) does not reliably reject a stale write. Two saves of the same entity that overlap in time can both succeed, and the later one silently overwrites the earlier one. Recipes and meal plans use this path.

Cause

update() checks the version and saves in two separate statements within one transaction:

findOneOrFail(..., { lock: { mode: "optimistic", version } }) reads the row and compares its version.
em.save(entity) writes the row. @VersionColumn adds version = version + 1 to the UPDATE, but TypeORM does not add the version to the WHERE clause.

Under PostgreSQL's default isolation (READ COMMITTED), the check does not lock the row. Two transactions that both read version N both pass the check, and both UPDATEs run. The second waits for the first's row lock, then applies on top of it.

Example

A recipe is at version 3. A user saves it from their phone and their laptop at almost the same moment, both sending version: 3.
Both requests pass the version check.
Both saves run. The laptop's save overwrites the phone's changes and the version ends at 5.
Neither client gets the 409 that the optimistic-locking convention (recipe-platform/docs/api-conventions.md, "Optimistic Locking") promises.

The window is small (the time between the check and the save), so this is rare in practice, but the guarantee does not hold.

Possible fix

Put the version check into the write itself, as a conditional UPDATE ... WHERE id = :id AND version = :version. If no row is affected, a follow-up existence check decides between 404 and 409. docs/adr/2026-10-02-shopping-list-item-writes.md (spec 001) takes this approach for shopping list items. Alternatives would be SELECT ... FOR UPDATE before the check, or a pessimistic lock in update().

Found

During the recipe-backend repo architecture step for spec 001 (shopping lists). The new shopping list item writes avoid the problem. Recipes and meal plans were left unchanged on purpose and need this fix separately.

`AbstractRepository.update()` (`src/repositories/AbstractRepository.ts`) does not reliably reject a stale write. Two saves of the same entity that overlap in time can both succeed, and the later one silently overwrites the earlier one. Recipes and meal plans use this path. ## Cause `update()` checks the version and saves in two separate statements within one transaction: 1. `findOneOrFail(..., { lock: { mode: "optimistic", version } })` reads the row and compares its version. 2. `em.save(entity)` writes the row. `@VersionColumn` adds `version = version + 1` to the `UPDATE`, but TypeORM does not add the version to the `WHERE` clause. Under PostgreSQL's default isolation (READ COMMITTED), the check does not lock the row. Two transactions that both read version N both pass the check, and both `UPDATE`s run. The second waits for the first's row lock, then applies on top of it. ## Example 1. A recipe is at version 3. A user saves it from their phone and their laptop at almost the same moment, both sending `version: 3`. 2. Both requests pass the version check. 3. Both saves run. The laptop's save overwrites the phone's changes and the version ends at 5. 4. Neither client gets the 409 that the optimistic-locking convention (`recipe-platform/docs/api-conventions.md`, "Optimistic Locking") promises. The window is small (the time between the check and the save), so this is rare in practice, but the guarantee does not hold. ## Possible fix Put the version check into the write itself, as a conditional `UPDATE ... WHERE id = :id AND version = :version`. If no row is affected, a follow-up existence check decides between 404 and 409. `docs/adr/2026-10-02-shopping-list-item-writes.md` (spec 001) takes this approach for shopping list items. Alternatives would be `SELECT ... FOR UPDATE` before the check, or a pessimistic lock in `update()`. ## Found During the recipe-backend repo architecture step for spec 001 (shopping lists). The new shopping list item writes avoid the problem. Recipes and meal plans were left unchanged on purpose and need this fix separately.

araemer commented

2026-10-02 17:19:08 +00:00

Author

Owner

Two additions from the spec 001 repo architect, for whoever picks this up:

Let AbstractRepository take an EntityManager (default AppDataSource.manager) instead of binding to AppDataSource.getRepository in its constructor. Its methods can then run inside a caller's transaction, e.g. with SELECT … FOR UPDATE. Once update() is a race-free conditional update, ShoppingListRepository (spec 001) could extend the base class safely: a list or share row has no columns that version leaves unguarded. ShoppingListItemRepository must never extend it (see docs/adr/2026-10-02-shopping-list-item-writes.md).
Fix the AbstractEntity JSDoc. It claims save() throws OptimisticLockVersionMismatchError when the version does not match. That is false: save() only increments version, and only a findOne with lock: { mode: "optimistic" } throws. The false claim is probably how the check-then-save pattern came about, so it belongs with this fix.

Also: update() currently throws 409 when version is missing. The conventions want 400 VALIDATION_FAILED.

Two additions from the spec 001 repo architect, for whoever picks this up: 1. **Let `AbstractRepository` take an `EntityManager`** (default `AppDataSource.manager`) instead of binding to `AppDataSource.getRepository` in its constructor. Its methods can then run inside a caller's transaction, e.g. with `SELECT … FOR UPDATE`. Once `update()` is a race-free conditional update, `ShoppingListRepository` (spec 001) could extend the base class safely: a list or share row has no columns that `version` leaves unguarded. `ShoppingListItemRepository` must never extend it (see `docs/adr/2026-10-02-shopping-list-item-writes.md`). 2. **Fix the `AbstractEntity` JSDoc.** It claims `save()` throws `OptimisticLockVersionMismatchError` when the version does not match. That is false: `save()` only increments `version`, and only a `findOne` with `lock: { mode: "optimistic" }` throws. The false claim is probably how the check-then-save pattern came about, so it belongs with this fix. Also: `update()` currently throws 409 when `version` is missing. The conventions want 400 `VALIDATION_FAILED`.

araemer referenced this issue

2026-10-02 17:19:16 +00:00

feat(shoppingList) Shopping lists (spec 001) #21

araemer referenced this issue from a commit

2026-10-03 12:07:20 +00:00

fix(repositories) Make optimistic-lock updates race-free

araemer referenced this issue from a pull request that will close it,

2026-10-03 12:07:33 +00:00

fix(repositories) Make optimistic-lock updates race-free #23

araemer referenced this issue

2026-10-03 17:40:06 +00:00

Release: shopping lists (spec 001) #30