Skip to content

Product writes have no optimistic locking (lost-update risk) #367

Description

@alukach

Summary

The product-mirror server actions — addProductMirror, removeProductMirror, setPrimaryMirror (src/lib/actions/product-mirrors.ts) — read a product, mutate metadata.mirrors / primary_mirror in memory, and write the whole product back via productsTable.update. There is no condition on the write, so two concurrent admin operations on the same product can silently clobber each other (lost update).

This was raised as a non-blocking finding in the #364 review.

Scope

This is not specific to mirrors: productsTable.update is a read-modify-write everywhere it's used, and Product has no version attribute. So a real fix is a small cross-cutting change, not a one-off — which is why it's split out of #364 rather than patched only in the mirror actions.

Impact

Low in practice: these are admin-only operations and concurrent edits of the same product are rare. But the race is real and silent when it happens.

Proposed fix

Add optimistic concurrency to productsTable.update:

  • Condition the UpdateCommand on the updated_at the caller read (ConditionExpression: "updated_at = :prev_updated_at"), or introduce a numeric version attribute and bump+condition it.
  • Surface ConditionalCheckFailedException to callers as a "this product changed, please retry" error.

Apply consistently across product mutations so the behavior is uniform.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Fields

    No fields configured for Feature.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions