Skip to content

fix(postgres): concurrent UpdateItem upserts creating one item must all succeed - #250

Open
LeeroyHannigan wants to merge 2 commits into
mainfrom
fix/concurrent-upsert-must-not-fail-conditional-check
Open

fix(postgres): concurrent UpdateItem upserts creating one item must all succeed#250
LeeroyHannigan wants to merge 2 commits into
mainfrom
fix/concurrent-upsert-must-not-fail-conditional-check

Conversation

@LeeroyHannigan

Copy link
Copy Markdown
Collaborator

What

Concurrent UpdateItem calls with no ConditionExpression racing to create the
same not-yet-existing item now all succeed, and each writer's update expression is
applied on top of whatever the previous writer committed.

Why

Closes #249.

Reproduced on this backend before changing anything, at the exact commit the
reporter cited (3961c63):

  • 10 of 20 calls failed with ConditionalCheckFailedException across five trials
  • four writers each setting a different attribute left an item holding only
    one of them, so three writers' data was lost outright
  • sequential create-then-update was unaffected, which is why this only appears
    under concurrency

Measured against real DynamoDB in us-east-1 for comparison: six concurrent writers
on one brand-new key succeeded 6/6 across three trials, and four writers setting
four different attributes produced an item holding all four. The service serialises
writes to one item, and an UpdateItem without a condition is an upsert, so every
writer must succeed and each expression applies on top of the previous commit.

Root cause

Every read in this layer already uses SELECT ... FOR UPDATE, so writers to an item
that exists serialise correctly on the row lock. A row that does not exist yet
cannot be locked, so two writers could both take the insert path. The loser's
ON CONFLICT DO NOTHING affected no rows, and the code answered that with
ConditionFailed regardless of whether the caller ever supplied a condition.

The sibling put_item path already draws the distinction correctly, using
DO UPDATE for the unconditional case and DO NOTHING plus ConditionFailed only
for the conditional one. update_item had only the second shape.

The fix

The read-modify-write now sits in a bounded loop with a single locking read at its
head. On the first pass that read is the original one; after a lost race it is the
re-read that resolves the conflict.

Re-reading FOR UPDATE is what makes this terminate. ON CONFLICT DO NOTHING does
not wait for a concurrent uncommitted inserter, so a plain re-read can see nothing
at all, whereas the locking read blocks until the winner commits (returning its row)
or aborts (returning none, so the insert can be retried). The loser then re-applies
its update expression on top of the winner's item.

Attempts are capped at five. A retry can only be provoked by another writer
committing or rolling back the row, so exhausting the cap means something
pathological and returns an internal error rather than looping.

Deliberately not ON CONFLICT DO UPDATE SET item_data = EXCLUDED.item_data

That is the obvious one-line fix, it is what the issue suggests, and it is wrong
here. The losing writer computed its item from an empty base, so EXCLUDED
holds only the attributes its own expression set. Overwriting with it silently drops
whatever the winner set, which is exactly the data loss measured above, while
keeping every call successful. It is correct for PutItem, where blind overwrite is
the semantics, and it stays in place there and in the transactional helper.

Testing done

cargo test                                  # 422 integration, 0 failed, 0 filtered out
cargo test --no-default-features --features postgres   # 671 workspace, 0 failed
cargo fmt --all --check                     # clean
cargo clippy --all-targets --no-default-features --features postgres -- -D warnings   # clean

Run against a live Postgres 16 backend over HTTP, not in process.

Two tests added to tests/rust/src/concurrency.rs:

  1. concurrent_unconditional_upserts_to_one_new_item_all_succeed_and_merge
    asserts the merge, not merely the absence of the error. This is deliberate: a
    test that only counted failures would pass against the EXCLUDED.item_data fix
    while data was being dropped.

    • Negative control: fails on the unpatched build with exactly the reported
      ConditionalCheckFailedException.
    • 20 consecutive passes on the fix, since one green run cannot distinguish
      deterministic from lucky.
  2. only_one_racing_conditional_create_wins is a regression guard, not a bug
    demonstration
    , and the code comment says so because it is easy to misread: it
    passes both before and after. Re-evaluating a condition against the race winner
    produces the same answers either way, because attribute_exists fails against
    its empty base before reaching the insert and attribute_not_exists still fails
    when re-evaluated against the winner. I initially read this as a second defect
    and that was wrong; the test exists to prove the retry did not perturb either
    outcome.

One thing worth recording so nobody re-investigates it: three capacity_throttling
failures appeared mid-investigation. They were an artefact of running that suite
against a warm server immediately after the full run, and were proven unrelated by
an A/B on a fresh server, 7 passed both with and without the change.

Scope

  • SQLite is unaffected. Its update_item serialises on a process-wide write lock
    and has no lost-race branch.
  • tx_helpers.rs is unchanged. It uses a blind upsert, which is defensible inside
    the serialised transact path, where DynamoDB itself rejects concurrent transactions
    on one item with TransactionConflictException.

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed
  • Breaking changes are noted below (if any)
  • If this changes the wire protocol, Storage trait, auth model, on-disk
    format, or public CLI surface, an RFC has been accepted or is linked
    below. Otherwise, an ADR captures the decision (link below).

ADR / RFC: n/a. This restores documented DynamoDB behaviour on an existing
operation; no contract, trait, format or CLI change.

Breaking changes

None. Callers that previously received ConditionalCheckFailedException from an
unconditional UpdateItem were receiving an error the real service never returns,
so no correct client can depend on it.


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

…ll succeed

Two or more UpdateItem calls with no ConditionExpression racing to create the
same not-yet-existing item returned ConditionalCheckFailedException to every
writer but one. Reproduced on this backend before changing anything: 10 of 20
calls failed across five trials, and four writers each setting a different
attribute left an item holding only one of them, so three writers' data was
lost outright. Sequential create-then-update was unaffected, which is why this
only shows up under concurrency. Reported as #249.

An UpdateItem carrying no condition is an upsert and must never surface a
conditional failure. Measured against the real service in us-east-1: six
concurrent writers on one brand-new key succeeded six times out of six across
three trials, and four writers setting four different attributes produced an
item holding all four. So the service serialises the writes and applies each
update expression on top of whatever the previous writer committed.

Why it happened

Every read in this layer already uses SELECT ... FOR UPDATE, so writers to an
item that EXISTS serialise correctly on the row lock. A row that does not exist
yet cannot be locked, so two writers could both take the insert path. The
loser's ON CONFLICT DO NOTHING affected no rows and the code answered that with
ConditionFailed, which is correct only when the caller supplied a condition and
was applied unconditionally. The sibling put_item path already draws this
distinction, using DO UPDATE for the unconditional case and DO NOTHING plus
ConditionFailed only for the conditional one.

The fix

The read-modify-write now sits in a bounded loop with a single locking read at
its head. On the first pass that read is the original one; after a lost race it
is the re-read that resolves the conflict. Re-reading FOR UPDATE is what makes
this terminate: ON CONFLICT DO NOTHING does not wait for a concurrent
uncommitted inserter, so a plain re-read can see nothing at all, whereas the
locking read blocks until the winner commits, returning its row, or aborts,
returning none so the insert can be retried. The loser then re-applies its
update expression on top of the winner's item.

Deliberately NOT ON CONFLICT DO UPDATE SET item_data = EXCLUDED.item_data, which
is the obvious one-line fix and is wrong here. The losing writer computed its
item from an empty base, so EXCLUDED holds only the attributes its own
expression set; overwriting with it silently drops whatever the winner set. That
is exactly the data loss measured above, and it would keep every call
succeeding. It is right for PutItem, where blind overwrite is the semantics, and
it stays in place there and in the transactional helper.

Attempts are capped at five. A retry can only be provoked by another writer
committing or rolling back the row, so exhausting the cap means something
pathological and returns an internal error rather than looping.

Verification

The new test asserts the MERGE, not merely the absence of the error, because a
test that only counted failures would pass against the wrong fix described
above. Negative control: it fails on the unpatched build with exactly the
reported ConditionalCheckFailedException. It then passed 20 consecutive runs on
the fix, since one green run cannot distinguish deterministic from lucky.

The conditional companion test is a regression guard rather than a bug
demonstration, and the commit records that distinction because it is easy to
misread: it passes both before and after. Re-evaluating a condition against the
race winner produces the same answers either way, because attribute_exists
fails against its empty base before reaching the insert and attribute_not_exists
still fails when re-evaluated against the winner. An earlier reading of this as
a second defect was wrong, and the test exists to prove the retry did not
perturb either outcome.

422 Rust integration tests and 671 workspace tests, 0 failed, 0 filtered out,
fmt and clippy -D warnings clean. Three capacity_throttling failures seen
mid-investigation were an artefact of running that suite against a warm server
after the full run; proven unrelated by an A/B on a fresh server, 7 passed with
and without the change.

SQLite is unaffected: its update_item serialises on a process-wide write lock
and has no lost-race branch.

Closes #249
… history

Review asked the right question: when concurrent writers update the SAME
attribute, who wins, and how do we know a write is not silently lost? The
existing tests did not answer it. The merge test uses disjoint attributes, so
it cannot detect a lost update to a contended one, and counting successes is
not sufficient: six writers could all return 200 while some applied to stale
bases and earlier values vanished.

The answer being asserted: writers serialize. Which writer ends up last is
scheduling-dependent, on real DynamoDB as well, but the history must be
linear: the final value is exactly one writer's value, never a merge or a torn
write, and every intermediate value is observed by exactly one successor.

The proof uses ReturnValues ALL_OLD, which turns each writer into a witness of
the committed item it replaced. For N writers on one new key, a clean
serialization forces three facts, asserted independently so a failure names
its defect: exactly one writer observed an absent item (two creators would
mean a committed value was overwritten by a fresh create); no two writers
observed the same predecessor (a duplicate means someone applied to a stale
base and the write between them was lost); and the final value is the single
written value nobody observed as old (otherwise the history forks). Together
these pin the N observations into one chain: none -> w_a -> ... -> final.

Negative control on baseline (main's update_item.rs, everything else at this
branch): fails in trial 0 with the original ConditionalCheckFailedException,
so the test discriminates. With the fix: 20/20 consecutive runs green, and the
full concurrency module passes 4/4. cargo fmt --all -- --check exit 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent UpdateItem upserts to the same new item fail with ConditionalCheckFailedException

1 participant