Skip to content

fix(driver-memory): make bulkCreate all-or-nothing so a refused batch leaves no surviving prefix - #13441

Merged
zhuangjianguo merged 2 commits into
mainfrom
claude/issue-13340-bulkcreate-all-or-nothing
Aug 30, 2026
Merged

fix(driver-memory): make bulkCreate all-or-nothing so a refused batch leaves no surviving prefix#13441
zhuangjianguo merged 2 commits into
mainfrom
claude/issue-13340-bulkcreate-all-or-nothing

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13340

InMemoryDriver.bulkCreate was one line — Promise.all(dataArray.map(data => this.create(object, data, options))) — and create writes into the table synchronously. So a batch was not atomic: when any row was refused, every row accepted before it stayed in the store, and the caller got a rejection describing a batch that had partly landed. That made a caller's retry of the same array unsafe for reasons that had nothing to do with constraints, and it made the two batch doors of one driver disagree about whether a batch is all-or-nothing — seven lines apart in one file.

bulkCreate now builds and checks every row — against the table and against the rest of the batch — before pushing any of them. That is the posture updateMany has had since #13197, one method over, and the one driver-sql gets from sending a batch as a single insert. create's own single-row path is untouched; the record-building expression is copied from it deliberately, so the two doors cannot drift on what a row looks like (own-key-undefined included).

The measurement, and why the obvious assertion would not have caught this

The refusal was already correct and already pinned by #13197 / #13239 — an assertion that "the refusal still happens" was green throughout the defect's entire life. The discriminating fact is the row count:

before: 2 rows
bulkCreate([A9/Z, A9/Z]) -> rejects UNIQUE_VIOLATION / 409
after: 3 rows <- the first batch row landed. This was the defect.
after (this PR): 2 rows

Every new and changed assertion reads the store after the refusal rather than stopping at the envelope.

Verification

All readings on 714b7b46a8, the head of this branch.

Behaviour change, measured before any test was touched. Running the existing suite against the fixed driver flipped exactly one test, in the predicted direction — memory-declared-index-unique.test.ts, expected [] to have a length of 1 but got +0. That is the recorded-behaviour assertion observing that the surviving prefix is gone. 918 other tests were unaffected.

Ablation. Reverting bulkCreate to the old one-liner on the committed tree turns 6 tests red across the three suites (4 new, plus the two existing bulkCreate cases). The mutation was confirmed on disk before the run — injected old one-liner grep -c = 1, removed push loop grep -c = 0, and the blob hash moved 9f1201ff to 395a8ccd. The restore leg was verified the same way, not by exit code: blob back to 9f1201ff… (matching the HEAD blob exactly), git diff HEAD empty, tree clean. These tests import ./memory-driver.js — relative, same package — so vitest resolves them against source, not dist/; the red result under a source-only mutation is itself the proof of that resolution path, and no rebuild is involved.

Suite:pnpm --filter @objectstack/driver-memory exec vitest run --maxWorkers=2 gives Test Files 33 passed (33), Tests 927 passed (927), exit 0.

Typecheck:pnpm --filter @objectstack/driver-memory typecheck (tsc --noEmit) exit 0 — and confirmed non-vacuous: tsc --listFiles shows all four changed files in the program, so this package does not exclude its tests.

Full-repo lint:pnpm lint (eslint . --no-inline-config) exit 0, no problems. Run whole, not narrowed.

Gates, derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack against the actual 5-path diff — all exit 0: check:driver-conformance (owed by the driver change), check:cross-package-test-inputs, check:test-source-alias, check:engine-double-contract, check:where-matcher, check:objectql-double-limit, check:query-options-erasure, check:type-check-coverage, check:doc-authoring, check:published-files, check:type-source-resolution, check:logger-receiver-detach, check:slot-lookup, check:page-declaration-shape, check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check:pm-half-states, and the script-form gates (check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-keyed-text-bounds, check-comment-mask-adoption, check-undeclared-dep-imports, check-plugin-teardown-shape, check-ci-filter-parity, check-shard-attestation, release-rehearsal-clone --self-test, docs-audit/check-affected-docs).

Two gates are NOT MEASURED, not green and not red — both exited on their own documented prerequisite branch (exit 3): check-test-completeness grades a saved turbo run test log that only CI tees, and scripts/pm/check-half-states.mjs needs a real GitHub credential (this container's token is the proxy placeholder). Neither says anything about this diff.

One declared narrowing:check:type-check-debt --re-measure was not run. It re-runs tsc per ledger entry, and driver-memory has none — its only two mentions in scripts/check-type-check-coverage.mjs are prose comments (lines 18, 3308), and the structural half check:type-check-coverage passes, which is the gate that would fail if the package needed an entry. This diff adds no package and changes no tsconfig, so no untouched package's verdict can move either.

The prose site, and a measurement that corrects the card

The InMemoryDriver docstring said "bulkCreate will still happily land two rows with the same id (unless id itself declares unique, or a declared index lists it)". Triage expected that sentence to become false. Measured, it does not: with no unique declaration on id, assertUnique returns early, so such a batch still lands in full — there is nothing to refuse, and atomicity is about what happens when a row is refused. That is now pinned by two boundary tests rather than left to reasoning.

What was actually wrong with the paragraph is that it under-described the method: a reader looking there for batch semantics found nothing about the new posture. It has been rewritten to carry both facts — the missing primary key, explicitly flagged as not a half-applied batch, and the new all-or-nothing behaviour.

⚠️ The first draft of that rewrite opened "a batch write is ALL-OR-NOTHING", which is itself false — see below. It is corrected in the second commit and scoped to the two doors that actually have the posture.

Out-of-scope finding: filed, deliberately not addressed here

#13435 remains open and is out of scope for this PR: bulkUpdate and bulkDelete are still Promise.all(map(...)) and have the same defect. Deliberately not folded in: the correction is not the mechanical one, so copying updateMany's shape would be forcing the pattern rather than transferring it. bulkUpdate applies a different patch per id, so each pending row needs its own exceptId plus a projected set holding the other rows' post-images; update returns null for a missing id when strictMode is off, so check-then-mutate has to decide whether that refuses the batch; and bulkDelete returns void, so "atomic delete" needs a decision about what it reports. Those are semantics questions for triage, not a transcription.

Scope

Five files. packages/spec/**, docs/adr/**, .claude/**, skills/** and content/docs/releases/** are untouched. No test was skipped, disabled or quarantined. The recorded-behaviour assertion in memory-declared-index-unique.test.ts was inverted in place with the old numbers kept in the comment as the discriminating reading — not re-baselined — and the field-level suite in memory-unique-constraint.test.ts gained the row-count control it lacked, since asserting the envelope alone was vacuous there.

Authored by Claude Code, session session_01F3jdziLbAPGeceVNmSox5L (https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L) — recorded in prose because a body edit rewrites the footer link.

… leaves no prefix
`InMemoryDriver.bulkCreate` was `Promise.all(dataArray.map(data =>
this.create(...)))`, and `create` writes into the table synchronously, so a
batch was not atomic: when any row was refused, every row accepted BEFORE it
stayed in the store. Measured on a two-row table, a refused two-row batch left
three rows behind, so a caller's retry of the same array was unsafe for reasons
unrelated to constraints.
Build and check every row -- against the table AND against the rest of the
batch -- before pushing any of them. This is the posture `updateMany` has had
since #13197, one method over, and the one `driver-sql` gets from sending a
batch as a single insert. `create`'s own single-row path is untouched, and the
record-building expression is copied from it deliberately so the two doors
cannot drift on what a row looks like.
The recorded-behaviour assertion in memory-declared-index-unique.test.ts is
INVERTED in place with the old numbers kept in the comment, not re-baselined.
The field-level suite gains the row-count control it lacked -- asserting the
envelope alone was vacuous, since the refusal was already correct while the
first batch row was still landing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
…reate and updateMany
The paragraph added with the bulkCreate fix opened "a batch write is
ALL-OR-NOTHING", which reads as covering all four batch doors. It does not:
`bulkUpdate` and `bulkDelete` are still `Promise.all(map(...))` and still leave
the rows applied before a refusal standing (filed as #13435). Name them, so
this docstring does not assert behaviour the code has for only two of them --
the same defect class the `bulkCreate` sentence at the top of this file had
before it was corrected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 5f0a9c4adb6628207bf3d285967b0b7576e96ab6packageMentionDocs.

Which tree this was computed on

This run read content/docs from 45bd2fae491908da9b8b0aeedffea638690a9133 — the merge of head 714b7b46a84c609a6de22a320457ee291c90a917 into base 5f0a9c4adb6628207bf3d285967b0b7576e96ab6, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 45bd2fae491908da9b8b0aeedffea638690a9133 && git checkout 45bd2fae491908da9b8b0aeedffea638690a9133
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5f0a9c4adb6628207bf3d285967b0b7576e96ab6 714b7b46a84c609a6de22a320457ee291c90a917 && git checkout -B drift-repro 5f0a9c4adb6628207bf3d285967b0b7576e96ab6 && git merge --no-ff 714b7b46a84c609a6de22a320457ee291c90a917
node scripts/docs-audit/affected-docs.mjs --json 5f0a9c4adb6628207bf3d285967b0b7576e96ab6

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude