purchases: two concurrent purchases creating the same new item names in opposite order deadlock on the unique index #83

New issue

Closed

opened 2026-09-02 17:09:32 +00:00 by yuris · 1 comment

yuris commented

2026-09-02 17:09:32 +00:00

Owner

Found by Reviewer78r1 during review of PR #78, confirmed pre-existing on origin/main and therefore not fixed there.

What deadlocks

Two purchases that each create the same two new item names in opposite order deadlock on the uncommitted unique-index entries for lower(btrim(item_name)).

tx A: create newItem "ZZ ALPHA", then newItem "ZZ BETA"
tx B: create newItem "ZZ BETA",  then newItem "ZZ ALPHA"
      -> SQLSTATE 40P01, deadlock detected

The contention is not on inventory rows — neither row exists yet. It is on the index entries: each transaction inserts one name, the other transaction's matching insert blocks on that uncommitted entry, and they wait on each other.

Why PR #78's fix does not cover it

#78 added lockInventoryRows, which takes every needed row lock ascending in one statement before the per-line loop. That closes the deadlock #78 itself introduced for existing rows, and it cannot help here — but not for the reason first written in this issue.

The real reason: a row lock cannot order contention on an index entry for a row that does not exist yet. Whether a pre-lock statement is issued is beside the point. Measured across three variants:

all-newItem      [N1,N2] vs [N2,N1]          -> 40P01   (no pre-lock issued)
mixed, SHARED    [A,N1]  vs [A,N2]           -> clean
mixed, DISJOINT  [A,N1,N2] vs [B,N2,N1]      -> 40P01   (pre-lock issued in BOTH)

The disjoint case is decisive: both transactions issue a pre-lock, and it is useless, because the rows they contend on are the ones the pre-lock could not name. The shared case is clean only incidentally — the pre-lock on A serialises the two transactions before either reaches its inserts, so the index entries are never contended concurrently.

An earlier version of this issue said "an all-newItem set issues no pre-lock at all", which is true but implies a set that does issue one is protected. It is not.

The reviewer diffed the newItem branch of upsertPurchaseItem against origin/main and it is identical apart from Error → ServiceRejection, so this is in a file #78 touched but is not #78's doing.

Severity

Low reachability, and it fails safely. Postgres kills one transaction and both are atomic, so nothing is half-written — the loser gets a raw Failed query: … deadlock detected, which is the unreadable-error class #71 was filed about but on a genuinely concurrent path rather than a routine one.

Reaching it needs two purchases in flight simultaneously, each introducing the same two previously-unknown item names, in opposite line order. The likely real-world source is the WhatsApp bot: two receipt photos for the same delivery processed at once.

Options

Sort new-item names before inserting them (ascending, same rule as the id pre-lock) so both transactions take the index entries in the same order. Cheapest, symmetric with what lockInventoryRows already does for existing rows, and — per the measurements above — the only one of these options that actually closes it. Extending lockInventoryRows cannot, since the contended object is an index entry for a row that has no id yet.
Catch 40P01 at the transport and retry once. More general, but hides a real ordering bug behind a retry.
Accept it and translate 40P01 into a readable "coba lagi sebentar" message. Closes the unreadable-error symptom without closing the deadlock.

Option 1 is the one that matches the reasoning already recorded in lockInventoryRows.

#78 introduced and then removed the equivalent deadlock for existing rows; its lockInventoryRows docblock documents the ordering rule this would extend to names.

Found by `Reviewer78r1` during review of PR #78, confirmed **pre-existing** on `origin/main` and therefore not fixed there. ## What deadlocks Two purchases that each create the **same two new item names in opposite order** deadlock on the uncommitted unique-index entries for `lower(btrim(item_name))`. ``` tx A: create newItem "ZZ ALPHA", then newItem "ZZ BETA" tx B: create newItem "ZZ BETA", then newItem "ZZ ALPHA" -> SQLSTATE 40P01, deadlock detected ``` The contention is not on `inventory` rows — neither row exists yet. It is on the index entries: each transaction inserts one name, the other transaction's matching insert blocks on that uncommitted entry, and they wait on each other. ## Why PR #78's fix does not cover it #78 added `lockInventoryRows`, which takes every needed row lock ascending in one statement before the per-line loop. That closes the deadlock #78 itself introduced for **existing** rows, and it cannot help here — but not for the reason first written in this issue. **The real reason: a row lock cannot order contention on an index entry for a row that does not exist yet.** Whether a pre-lock statement is issued is beside the point. Measured across three variants: ``` all-newItem [N1,N2] vs [N2,N1] -> 40P01 (no pre-lock issued) mixed, SHARED [A,N1] vs [A,N2] -> clean mixed, DISJOINT [A,N1,N2] vs [B,N2,N1] -> 40P01 (pre-lock issued in BOTH) ``` The disjoint case is decisive: both transactions issue a pre-lock, and it is useless, because the rows they contend on are the ones the pre-lock could not name. The shared case is clean only **incidentally** — the pre-lock on `A` serialises the two transactions before either reaches its inserts, so the index entries are never contended concurrently. An earlier version of this issue said "an all-`newItem` set issues no pre-lock at all", which is true but implies a set that *does* issue one is protected. It is not. The reviewer diffed the `newItem` branch of `upsertPurchaseItem` against `origin/main` and it is identical apart from `Error` → `ServiceRejection`, so this is in a file #78 touched but is not #78's doing. ## Severity Low reachability, and it fails safely. Postgres kills one transaction and both are atomic, so nothing is half-written — the loser gets a raw `Failed query: … deadlock detected`, which is the unreadable-error class #71 was filed about but on a genuinely concurrent path rather than a routine one. Reaching it needs two purchases in flight simultaneously, each introducing the same two previously-unknown item names, in opposite line order. The likely real-world source is the WhatsApp bot: two receipt photos for the same delivery processed at once. ## Options 1. **Sort new-item names before inserting them** (ascending, same rule as the id pre-lock) so both transactions take the index entries in the same order. Cheapest, symmetric with what `lockInventoryRows` already does for existing rows, and — per the measurements above — the only one of these options that actually closes it. **Extending `lockInventoryRows` cannot**, since the contended object is an index entry for a row that has no id yet. 2. Catch `40P01` at the transport and retry once. More general, but hides a real ordering bug behind a retry. 3. Accept it and translate `40P01` into a readable "coba lagi sebentar" message. Closes the unreadable-error symptom without closing the deadlock. Option 1 is the one that matches the reasoning already recorded in `lockInventoryRows`. ## Related - #78 introduced and then removed the equivalent deadlock for existing rows; its `lockInventoryRows` docblock documents the ordering rule this would extend to names.

yuris commented

2026-09-02 17:16:42 +00:00

Author

Owner

Correcting one line, at the filer's invitation.

The description says lockInventoryRows cannot help "because an all-newItem set issues no pre-lock at all". That is true but it is not the reason, and as written it implies a set that does issue a pre-lock is protected. It is not. Measured, three variants, two connections:

all-newItem      [N1,N2]   vs [N2,N1]      -> 40P01 DEADLOCK      (no pre-lock issued)
mixed, SHARED A  [A,N1,N2] vs [A,N2,N1]    -> clean               (pre-lock on A serialises them)
mixed, DISJOINT  [A,N1,N2] vs [B,N2,N1]    -> 40P01 DEADLOCK      (pre-lock issued in BOTH, and useless)

The third case is the one that matters: both transactions take a pre-lock, and they still deadlock, because the contention is not on the inventory rows at all — it is on the uncommitted unique-index entries for lower(btrim(item_name)) that each insert takes for a row the other transaction is also creating.

So the accurate reason is: a row lock cannot order contention on an index entry for a row that does not exist yet. lockInventoryRows orders access to rows that are already there; two newItem lines naming the same not-yet-existing products are ordered by nothing. The shared-existing-item case is clean only incidentally — the pre-lock on A happens to serialise the two transactions before either reaches its inserts, which is luck of the line-up rather than a property of the fix.

That distinction changes what a fix would have to do: ordering the newItem lines by name before inserting them would work, whereas extending lockInventoryRows cannot, since there is nothing to lock.

Everything else in the description matches what I measured, including that this is pre-existing — I diffed the newItem branch of upsertPurchaseItem against origin/main and it is identical apart from Error becoming ServiceRejection.

Correcting one line, at the filer's invitation. The description says `lockInventoryRows` cannot help "because an all-`newItem` set issues no pre-lock at all". That is true but it is not the reason, and as written it implies a set that *does* issue a pre-lock is protected. It is not. Measured, three variants, two connections: ``` all-newItem [N1,N2] vs [N2,N1] -> 40P01 DEADLOCK (no pre-lock issued) mixed, SHARED A [A,N1,N2] vs [A,N2,N1] -> clean (pre-lock on A serialises them) mixed, DISJOINT [A,N1,N2] vs [B,N2,N1] -> 40P01 DEADLOCK (pre-lock issued in BOTH, and useless) ``` The third case is the one that matters: both transactions take a pre-lock, and they still deadlock, because the contention is not on the inventory rows at all — it is on the uncommitted **unique-index entries** for `lower(btrim(item_name))` that each `insert` takes for a row the other transaction is also creating. So the accurate reason is: **a row lock cannot order contention on an index entry for a row that does not exist yet.** `lockInventoryRows` orders access to rows that are already there; two `newItem` lines naming the same not-yet-existing products are ordered by nothing. The shared-existing-item case is clean only incidentally — the pre-lock on `A` happens to serialise the two transactions before either reaches its inserts, which is luck of the line-up rather than a property of the fix. That distinction changes what a fix would have to do: ordering the `newItem` lines by name before inserting them would work, whereas extending `lockInventoryRows` cannot, since there is nothing to lock. Everything else in the description matches what I measured, including that this is pre-existing — I diffed the `newItem` branch of `upsertPurchaseItem` against `origin/main` and it is identical apart from `Error` becoming `ServiceRejection`.

yuris referenced this issue from a commit

2026-09-02 17:21:44 +00:00

docs(management): narrow the one overclaiming lock clause, record the deploy invariant

yuris referenced this issue from a commit

2026-09-28 20:42:25 +00:00

fix(purchases): sort new item insertions ascending to prevent unique index deadlock (#83)

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

2026-09-28 20:42:30 +00:00

fix(purchases): sort new item insertions ascending to prevent unique index deadlock #116

yuris closed this issue

2026-09-28 20:42:59 +00:00

yuris referenced this issue from a commit