Introduced by #13875 (issue #13435), which landed at 4642f4c64c. Found in PM review before the merge, but the PR was already in the merge queue and landed anyway — disabling auto-merge does not dequeue an already-enqueued PR.
The defect
In InMemoryDriver.bulkUpdate (packages/drivers/driver-memory/src/memory-driver.ts, lines ~910–917 on main), two lookups over the same ids disagree:
consttouchedIds=newSet(updates.map((u)=>u.id));// ids from the CALLERconstsettled=table.filter((r)=>!touchedIds.has(r.id));// STRICT Set.has
...
constindex=table.findIndex((r)=>r.id==u.id);// LOOSE ==
IDataDriver.bulkUpdate declares id: string | number, so a caller may legitimately pass an id whose JS type differs from the stored row's. When that happens:
touchedIds.has(r.id) is false (strict), so the row stays in settled carrying its pre-image;findIndex still resolves it (loose ==), so the row is updated and its post-image enters pending.
The row is therefore represented twice in the projected row set passed to assertUnique — once with the value it is vacating, once with the value it is taking. exceptId only excludes the row currently being checked, so it does not help a sibling row in the same batch.
Failure scenario
- Table:
{id: 1, doc_no: 'D-0001'}, {id: 2, doc_no: 'D-0002'} — numeric ids. - Call:
bulkUpdate('doc', [{id: '1', data: {doc_no: 'D-0900'}}, {id: 2, data: {doc_no: 'D-0001'}}]) — first id a string. - Row 1 vacates
D-0001; row 2 takes it. This must succeed. - Actual: row 1's stale pre-image sits in
settled still holding D-0001, so row 2's check sees a collision and throws a false UNIQUE_VIOLATION / 409.
Why the sibling door does not have it
updateMany (#13197) performs the same check-then-mutate discipline without the gap, because both its comparisons agree by construction:
consttargetIds=newSet(targetRecords.map((r)=>r.id));// ids from TABLE ROWSconstsettled=table.filter((r)=>!targetIds.has(r.id));constindex=table.findIndex(r=>r.id===record.id);// STRICT ===
Its ids originate from the table itself, so strict membership is correct there. #13875 generalized the discipline (check everything before writing anything) to per-row patches, but not that internal consistency.
Regression, not pre-existing
The pre-#13875 shape was Promise.all(updates.map(u => this.update(object, u.id, u.data, options))). It had no settled set at all — each update() did its own loose lookup against the live table — so it could not false-refuse this way. The false-refusal path is new.
Direction
Derive settled from the same resolution the write uses: resolve each update to its table index first (as #13875's own bulkDelete already does), then build the touched set from the resolved rows' own ids (table[index].id) rather than from caller input.
⛔ Not by tightening findIndex to ===. That would silently narrow which ids resolve at all — a behaviour change well beyond this defect — and update() itself (line ~525) uses ==, so this door must keep matching its sibling's resolution semantics.
Also to check
- Whether
bulkDelete in the same PR has the analogous gap. It resolves indices with the same loose findIndex into a Set<number> of indices; reason it through rather than assuming symmetry either way. - Whether
assertNoUniqueViolation's exceptId filtering is strict or loose, which interacts with the same question.
A regression test using mixed id types, plus a positive control with consistent id types, is required — the existing suite uses string ids throughout, which is why this passed 36/36.
Refs: #13875 (introduced it) · #13435 (the card it closed) · #13197 (the sibling door that does it right)
Introduced by #13875 (issue #13435), which landed at
4642f4c64c. Found in PM review before the merge, but the PR was already in the merge queue and landed anyway — disabling auto-merge does not dequeue an already-enqueued PR.The defect
In
InMemoryDriver.bulkUpdate(packages/drivers/driver-memory/src/memory-driver.ts, lines ~910–917 onmain), two lookups over the same ids disagree:IDataDriver.bulkUpdatedeclaresid: string | number, so a caller may legitimately pass an id whose JS type differs from the stored row's. When that happens:touchedIds.has(r.id)is false (strict), so the row stays insettledcarrying its pre-image;findIndexstill resolves it (loose==), so the row is updated and its post-image enterspending.The row is therefore represented twice in the projected row set passed to
assertUnique— once with the value it is vacating, once with the value it is taking.exceptIdonly excludes the row currently being checked, so it does not help a sibling row in the same batch.Failure scenario
{id: 1, doc_no: 'D-0001'},{id: 2, doc_no: 'D-0002'}— numeric ids.bulkUpdate('doc', [{id: '1', data: {doc_no: 'D-0900'}}, {id: 2, data: {doc_no: 'D-0001'}}])— first id a string.D-0001; row 2 takes it. This must succeed.settledstill holdingD-0001, so row 2's check sees a collision and throws a falseUNIQUE_VIOLATION/ 409.Why the sibling door does not have it
updateMany(#13197) performs the same check-then-mutate discipline without the gap, because both its comparisons agree by construction:Its ids originate from the table itself, so strict membership is correct there. #13875 generalized the discipline (check everything before writing anything) to per-row patches, but not that internal consistency.
Regression, not pre-existing
The pre-#13875 shape was
Promise.all(updates.map(u => this.update(object, u.id, u.data, options))). It had nosettledset at all — eachupdate()did its own loose lookup against the live table — so it could not false-refuse this way. The false-refusal path is new.Direction
Derive
settledfrom the same resolution the write uses: resolve each update to its table index first (as #13875's ownbulkDeletealready does), then build the touched set from the resolved rows' own ids (table[index].id) rather than from caller input.⛔ Not by tightening
findIndexto===. That would silently narrow which ids resolve at all — a behaviour change well beyond this defect — andupdate()itself (line ~525) uses==, so this door must keep matching its sibling's resolution semantics.Also to check
bulkDeletein the same PR has the analogous gap. It resolves indices with the same loosefindIndexinto aSet<number>of indices; reason it through rather than assuming symmetry either way.assertNoUniqueViolation'sexceptIdfiltering is strict or loose, which interacts with the same question.A regression test using mixed id types, plus a positive control with consistent id types, is required — the existing suite uses string ids throughout, which is why this passed 36/36.
Refs: #13875 (introduced it) · #13435 (the card it closed) · #13197 (the sibling door that does it right)