Uh oh!
There was an error while loading. Please reload this page.
fix(driver-memory): bulkUpdate's touched-row set must agree with its own id resolution - #13917
Conversation
…own id resolution `bulkUpdate` resolved each id with a loose `==` (matching `update`, one method up) but built its untouched-row set `settled` from the CALLER's ids with a strict `Set.has`. `IDataDriver.bulkUpdate` declares `id: string | number`, so a caller may name a stored `1` as `'1'` — and then the two lookups disagreed: `findIndex` resolved the row and updated it, while `settled` still carried that row's PRE-image. The row was represented twice in the projected set handed to `assertUnique` — once with the value it was vacating, once with the value it was taking — so a batch that merely HANDS a unique value from one row to another was refused with a false `UNIQUE_VIOLATION` / 409. `exceptId` does not help, since it only excludes the row currently being checked, never a sibling row of the same batch. Resolve every id to its table index first, then derive the touched set from the RESOLVED rows' own ids. Both lookups now read the same stored value and cannot drift apart — the property `updateMany` gets for free by drawing its `targetIds` from table rows. The loose resolution is deliberately preserved: narrowing it to `===` would silently change which ids resolve at all, well beyond this defect. `bulkDelete` needs no change, and this is checked rather than assumed: it has exactly ONE id comparison, and dedups on the RESOLVED table index rather than on caller input, so a mixed-type or repeated id collapses to one index by construction. Keying that set on caller ids instead would splice one index twice and take a neighbouring row with it — pinned by test. Regression test uses mixed id types, alongside a positive control with consistent id types so it cannot pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
📓 Docs Drift Check1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 275e755a7f3183e3711b85e5661aa771ed73b6c7 && git checkout 275e755a7f3183e3711b85e5661aa771ed73b6c7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 dcbcb1e22ae69c400adc6d9c617eb0348c67bb7e && git checkout -B drift-repro 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 && git merge --no-ff dcbcb1e22ae69c400adc6d9c617eb0348c67bb7e
node scripts/docs-audit/affected-docs.mjs --json 4642f4c64c002f94f1d737bbb2a5fc94ae43ddf3 |
…introduced the defect Neighbouring comments in this file cite the ISSUE (`[#13435]`, `[#13197]`, `[#13340]`); this one cited #13875, which is the PR that introduced the defect. Comment text only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
…rest of the file Comment and describe-title text only — no assertion changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
Uh oh!
There was an error while loading. Please reload this page.
Fixes#13911
The defect
InMemoryDriver.bulkUpdateused two lookups over the same ids, and they disagreed:IDataDriver.bulkUpdatedeclaresid: string | number, and this driver resolves an id toa row with a loose comparison — as
updateanddeletealways have — so naming a stored1as'1'finds the same row. ButSetmembership is always strict. For a mixed-typeid the row was therefore resolved and updated and left in
settledcarrying itspre-image, so it entered the projected row set twice: once with the value it was
vacating, once with the value it was taking.
exceptIddoes not help — it excludes onlythe row currently being checked, never a sibling row of the same batch.
Net effect: a batch that merely hands a unique value from one row to another was
refused with a false
UNIQUE_VIOLATION/ 409.The fix
Resolve every id to its table index first, then derive the touched set from the
resolved rows' own ids rather than from caller input:
Both lookups now read the same stored value and cannot drift apart — the property
updateManygets for free by drawing its target ids from table rows.⛔ Deliberately not fixed by tightening
findIndexto===. That would silentlynarrow which ids resolve at all — a behaviour change far wider than this defect — and
update()one method up uses==, so this door must keep matching its sibling'sresolution semantics. The bug is the disagreement; the resolution side is the side that
had to be preserved.
The main loop now reads each id's already-resolved index instead of resolving a second
time, which is what let the two lookups drift apart in the first place. Error precedence
is unchanged: the missing-id throw and the uniqueness refusal still fire at the same point
in batch order as before.
Correcting the record on the construction
The PR that introduced this (#13875, landed at
4642f4c64c) described itsbulkUpdateasgeneralizing
updateMany's posture. It generalized the discipline — check everypending row's post-image before writing any of them — but not
updateMany's internalconsistency. That is the one way it diverged:
updateMany'stargetIdscome from tablerows and its
findIndexis strict===, so both of its comparisons agree byconstruction;
bulkUpdatedrew one side from caller input and the other from a looseresolution. The landed changeset is left untouched (it is already merged); this note is
the correction.
bulkDelete— checked, not assumedbulkDeletehas no analogous gap, and the reason is structural rather thancoincidental: it performs exactly one id comparison (the resolving
findIndex), andeverything downstream is keyed on that comparison's result — a table index — never on
caller input. There is no second lookup to disagree with the first.
That also makes its de-duplication type-proof:
['1', 1]against a stored1collapses toone index. Had the set been keyed on caller ids instead, those would be two distinct
entries and the two splices would remove index 0 twice, taking a neighbouring row with it.
Both properties are now pinned by test rather than left to inspection.
exceptIdsemanticsassertNoUniqueViolationfiltersexceptIdstrictly (row.id === exceptId).bulkUpdatealready passed the storedtable[index].idthere rather than the caller'sid, so self-exclusion was type-consistent and is unaffected by this fix — worth recording,
since passing caller input there would have been a second instance of the same class.
Tests
Added to
memory-bulk-update-delete-atomicity.test.ts(the existing suite used string idsthroughout, which is why this passed 36/36):
regression cannot pass vacuously;
check off);
bulkDeletewith a mixed-type id, and the same row named twice in two id types.Before the fix:
1 failed | 23 passed, the failure being exactly the mixed-type case withUnique constraint violated on doc.doc_no: a record with the value "D-0001" already exists— a value the batch had just vacated. After:24 passed.Full
driver-memorypackage suite: 38 files, 1018 tests, all passing.Ablation on the committed fix: reverting the touched-set derivation to caller input,
proven on disk with anchored greps in both directions plus a changed blob hash, turns the
new test red and only that test; restored under
trapwith absolute paths, verified by anempty
git diff HEADand a blob-hash match.Generated by Claude Code
Generated by Claude Code