Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing by zhuangjianguo · Pull Request #13875 · objectstack-ai/objectstack · GitHub
Skip to content

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing - #13875

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic
Aug 31, 2026
Merged

fix(driver-memory): make bulkUpdate and bulkDelete all-or-nothing#13875
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-13435-bulk-update-delete-atomic

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13435

What changed

driver-memory's bulkUpdate and bulkDelete were still
Promise.all(map(...)) over update/delete, and both of those write into
the table synchronously. So a mid-batch refusal — a UNIQUE_VIOLATION/409 on
bulkUpdate, a missing-id throw under strictMode on bulkDelete — left
every row processed before it already mutated, and the caller got a
rejection describing a batch that had partly landed. #13340 fixed the
identical shape on bulkCreate; this PR is the third and fourth batch door.

  • bulkUpdate now builds and checks every pending row's post-image
    before writing any of them. This is new construction, not a copy of a
    sibling door: updateMany stamps one shared data onto every matched row
    (no per-row pre-image to exclude) and bulkCreate has no pre-image at all
    (every row is new); bulkUpdate gives each id its own patch, so each
    pending row gets its own exceptId and is checked against a projected row
    set — the untouched (settled) rows plus every already-validated pending
    row's post-image. A not-yet-processed batch row needs no look-ahead entry:
    whichever of two colliding rows is checked second always finds the first
    already sitting in pending, the same incremental discipline
    bulkCreate/updateMany use, generalized to per-row patches.
  • bulkDelete resolves every id to a table index first — refusing the
    whole batch under strictMode before touching the table if one is missing
    — and only then splices (highest index first).
  • A missing id follows update/delete's own existing contract, never a
    third posture: skip when strictMode is off (the returned array simply
    omits that entry — IDataDriver.bulkUpdate is declared to resolve to
    a plain array of row objects (Record-of-string-to-unknown, no null
    member), and SqlDriver's
    own bulkUpdate already resolves a missing id the same way:
    if (updated) results.push(updated) — this follows that established
    convention rather than inventing a second one), refuse the whole batch
    when it is on.
  • bulkDelete still returns void — no current caller reads a per-row
    outcome, so widening the return type stays out of scope (per dispatch).
  • Corrected the InMemoryDriver class docstring, which (since driver-memory: bulkCreate is Promise.all(map(create)), so a refused row leaves every earlier row of the batch landed — updateMany on the same driver refuses before mutating anything #13340) had a
    caveat paragraph explicitly calling out bulkUpdate/bulkDelete as
    non-atomic — now updated to state all four batch doors agree.

The four-door table, filled in

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row set (new construction)#13435 (this PR)
bulkDeleteresolve-indices-then-splice, refuse-before-touching under strictMode#13435 (this PR)

All four now agree: a refusal leaves the table byte-identical to before the
call.

Zone 2 assumptions — measured

  • A2.1 (the crux) — CONFIRMED.assertUnique/assertNoUniqueViolation
    already accept a rows parameter (defaulting to the live table) and an
    exceptId that filters by row id, not index — the exact seam
    updateMany already uses for its own projected set. No new seam was
    needed.
  • A2.2 — neither sibling's shape transferred verbatim.bulkCreate is
    check-then-push with no pre-image; updateMany is check-then-mutate with
    one shared patch. bulkUpdate needed a new per-row-patch projected-set
    construction; only the discipline (check everything before writing
    anything) transferred.
  • A2.3 — FALSIFIED. The dispatch assumed driver-turso's super. calls
    inherit this fix. They do not: TursoDriver extends SqlDriver
    (@objectstack/driver-sql), which has no relationship to
    InMemoryDriver
    super.bulkUpdate/super.bulkDelete resolve to
    SqlDriver's own, separate implementations. SqlDriver.bulkDelete is a
    single WHERE id IN (...) statement per shard (already atomic on its own);
    SqlDriver.bulkUpdate is a sequential for-await loop over update()
    with no transaction, which has the same defect class this PR fixes, one
    layer up. Filed as driver-sql: bulkUpdate is a sequential per-row loop with no transaction — a mid-batch refusal leaves earlier rows committed (driver-turso inherits it via super.) #13854 (out of scope here per the dispatch: "do NOT edit
    driver-turso... if it needs its own change, that is a separate card" —
    and the actual defect lives in driver-sql, not driver-turso).
  • A2.4 — no live caller or test depends on the partial-prefix behaviour.
    Grepped every .bulkUpdate(/.bulkDelete( call site outside
    driver-sql/driver-turso's own implementations: driver-memory's own
    test suite never called either method before this PR;
    lifecycle-service.ts/history-cleanup.ts (the two live bulkDelete
    callers) and driver-mongodb/driver-turso's own test suites all use
    fresh ids or mock drivers that bypass real batch-write semantics entirely.

A tsc finding worth naming (not a STOP condition — resolved without touching the contract)

Explicitly typing the new bulkUpdate's intermediate array surfaced a real
tsc error (TS2416) that the ORIGINAL Promise.all(map(update)) shape
never triggered: update() itself returns null for a missing id under
non-strict mode, which is not part of IDataDriver.update()'s declared
return shape either (a Promise resolving to a single plain row object,
never null) — but that mismatch was masked by
TypeScript's "any absorbs a union" behavior (toStoredRecord's inferred
return type collapses the success branch to effectively any), so it never
surfaced. My new code's plain-object-typed intermediate array did
not get that same accidental pass. Resolved by following SqlDriver's own
existing convention for bulkUpdate (omit a missing row from the result
rather than padding with null) — no packages/spec change needed.

Testing

New file memory-bulk-update-delete-atomicity.test.ts, modeled on
#13340's memory-bulk-create-atomicity.test.ts: table-byte-identical (not
merely "it throws") pins for both bulkUpdate and bulkDelete, in both
strictMode postures, plus a non-regression suite for updateMany and
bulkCreate and a four-door agreement test.

Ablation.git stash-free, trap ... EXIT INT TERM-guarded,
absolute-path script: overwrote memory-driver.ts with the branch's pre-fix
base blob (git show BASE-SHA:TARGET-PATH, confirmed landed on disk via
anchored grep -c in both directions plus a git hash-object match against
the base blob), ran the new atomicity suite against the mutated tree —
8 of 18 tests failed (table-byte-identical pins for both bulkUpdate and
bulkDelete, and the four-door agreement test specifically on those two
doors, while bulkCreate/updateMany passed within the SAME mutated run,
confirming the ablation targeted the right doors) — then the trap restored
via git checkout HEAD -- TARGET-PATH, confirmed byte-identical to HEAD by
blob hash. See the report for the full anchored-grep + blob-hash detail.


Verified atb96aefe383 (post-merge with origin/main).

Generated by Claude Code

bulkUpdate and bulkDelete were still Promise.all(map(...)) over
update/delete, and both of those write into the table synchronously. A
mid-batch refusal left every row processed before it already mutated,
so the caller got a rejection describing a batch that had partly
landed -- the same defect #13340 fixed on bulkCreate, on the third and
fourth batch doors it did not reach.
bulkUpdate now builds and checks every pending row's post-image before
writing any of them -- new construction (each id keeps its own patch,
so the projected row set per pending row generalizes updateMany's
single-shared-patch posture rather than copying it) -- and bulkDelete
resolves every id to a table index first, refusing the whole batch
under strictMode before touching the table if one is missing. Both
follow update/delete's own existing missing-id contract rather than a
third posture. bulkDelete still returns void.
Fixes#13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
IDataDriver.bulkUpdate is declared Promise<Record<string, unknown>[]>
-- no null member. Padding the result with null for a non-strict
missing id (as update() itself returns) failed tsc against that
contract once the intermediate array was explicitly typed. Follow
SqlDriver.bulkUpdate's own existing convention instead: omit the row
entirely (if (updated) results.push(updated)) rather than inventing a
second "missing" representation.
Part of #13435
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-memory, touching 3 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/permissions/authentication.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via InMemoryDriver (symbol, a top-level class))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v16.mdx(via InMemoryDriver (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via InMemoryDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 — the merge of head b96aefe383ec3d41c6ef354d9a0a26d2c59284ef into base 9b21037d4198cd14105abd354b019e9323b4fe57, 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 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4 && git checkout 9c78cb9e80490ba66d8a9ba59364bb08314d9ad4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 b96aefe383ec3d41c6ef354d9a0a26d2c59284ef && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff b96aefe383ec3d41c6ef354d9a0a26d2c59284ef
node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9b21037d4198cd14105abd354b019e9323b4fe57 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance. ⛔ It corrects an error in my dispatch order, and the correction found a bigger bug.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⛔ A2.3 falsified — my Zone 1.1 was wrong, and so was the reading I inherited from triage

I wrote that driver-turso"inherits the non-atomic shape via super.", restating triage's census row. That is wrong about whose shape it inherits.TursoDriver extends SqlDriver (@objectstack/driver-sql), which has no relationship to InMemoryDriver — so super.bulkUpdate / super.bulkDelete resolve to SqlDriver's own, separate implementations. Fixing the memory driver was never going to reach them.

⚠️ Recording this plainly because triage's census was quoted into my order as binding Zone 1, and it carried the error forward. The census's substantive claim survives intact — those call sites are live and non-legacy, which is what killed the "just document them as non-atomic" escape hatch — but the inheritance clause did not.

And the falsification is worth more than the correction. Chasing it found:

SqlDriver.bulkUpdate is a sequential for-await loop over update() with no transaction — the same defect class this PR fixes, one layer up.

SqlDriver.bulkDelete is fine (a single WHERE id IN (...) per shard, atomic on its own). But bulkUpdate is not — and SqlDriver is the production driver, while driver-memory is the one the test suite runs against. Filed as #13854, correctly out of scope here. ⚠️ On the face of it that card matters more than this one did; I will grade it accordingly.

2. ⭐ The prohibition was honoured with understanding, not just compliance

#13340's standing rule — ⛔ do not copy updateMany's shape into bulkUpdate — is the kind a seat can satisfy by accident. This one explains exactly why neither sibling transferred:

  • bulkCreate — check-then-push, no pre-image at all (every row is new).
  • updateMany — check-then-mutate, one shared patch across every matched row.
  • bulkUpdateper-row patch, so each id needs its own exceptId and a projected row set (settled rows + every already-validated pending post-image).

Only the discipline transferred — "check everything before writing anything" — not the shape. That is precisely the distinction #13340's dispatch was drawing, and it is the difference between transferring a pattern and forcing one.

⭐ The neat part: "whichever of two colliding rows is checked second always finds the first already sitting in pending" ⇒ no look-ahead entry is needed. The incremental discipline generalises to per-row patches without extra machinery.

3. A2.1 confirmed, so STOP 1 never fired

assertUnique / assertNoUniqueViolation already take a rows parameter (defaulting to the live table) and an exceptId that filters by row id, not index — the exact seam updateMany already uses. No new seam, no improvised construction. That was the crux I flagged as most likely to bite, and it held.

4. The missing-id posture follows an existing convention rather than inventing a third

Zone 1.2 forbade a third posture. The seat went further than following delete's contract — it found that SqlDriver.bulkUpdate already resolves a missing id the same way (if (updated) results.push(updated), omitting rather than padding with null) and matched it. ⇒ Consistency with a sibling driver, not a local invention.

5. ⭐⭐ The tsc finding is the most interesting thing in this PR, and it is a latent contract violation

Typing the new intermediate array surfaced a real TS2416 that the original Promise.all(map(update)) never triggered:

update() itself returns null for a missing id under non-strict mode, which is not part of IDataDriver.update()'s declared return type either — but that mismatch was masked by TypeScript's "any absorbs a union" behaviour (toStoredRecord's inferred return collapses the success branch to effectively any), so it never surfaced.

A shipped driver returns a value its own declared contract forbids, and an inferred any has been hiding it. That is squarely this repo's declared-≠-enforced concern, and it was invisible until someone added a type. The seat resolved it correctly without touching packages/spec — but the underlying mismatch is still there. I am checking for an existing card and will file it if none exists.

6. The four-door table, delivered as asked

doorposturesince
updateManycheck-then-mutate (one shared patch)#13197
bulkCreatecheck-then-push (no pre-image)#13340
bulkUpdatecheck-then-mutate, per-row patch + projected row setthis PR
bulkDeleteresolve-indices-then-splice, refuse before touching under strictModethis PR

All four agree: a refusal leaves the table byte-identical to before the call. ⭐ That is the pin I asked for rather than "it throws", and the ablation confirms it discriminates — 8 of 18 fail on the pre-fix blob while bulkCreate/updateMany pass within the same mutated run, so the ablation is proved targeted rather than merely destructive.

⭐ Also correct: the InMemoryDriver docstring's now-false caveat paragraph (which since #13340 explicitly called out these two doors as non-atomic) was updated rather than left to rot.

Status


Generated by Claude Code

@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Docs drift re-verified by hand — clean. ⛔ Not a clean bill of health for the corpus.

The bot listed 4 hand-written pages + 3 release-owned, all anchored on the same thing: InMemoryDriver as a top-level class symbol. That is a weak anchor — it fires on any mention of the class, regardless of what the diff changed. So I checked what this diff actually changes semantics of: batch atomicity.

Searched content/docs for bulkUpdate / bulkDelete / bulkCreate and for InMemoryDriver:

PageWhat it actually saysVerdict
data-modeling/drivers.mdxdriver table row; persistence/ephemerality semanticsClean — says nothing about batch behaviour
permissions/authentication.mdxa new InMemoryDriver() code sampleClean — mention only
plugins/packages.mdxan import { InMemoryDriver } lineClean — mention only
protocol/objectql/query-syntax.mdxORDER BY on virtual columns being silently droppedClean — unrelated
references/data/driver.mdx, driver-sql.mdx, driver-nosql.mdxthe bulkCreate/bulkUpdate/bulkDelete rows are all [REMOVED]DriverCapabilities bits — about the capability flags being deleted under ADR-0049, not about atomicityClean
releases/v16.mdx, v17.mdx, implementation-status.mdxrelease-owned, read-onlyNot falsified — none states batch atomicity

No page in the corpus claims anything about batch atomicity in either direction, so this diff falsifies nothing. All 7 anchored rows are mention-level.

⭐ The one place that did state it was in code, not in docs: the InMemoryDriver class docstring carried a caveat (added by #13340) explicitly calling bulkUpdate/bulkDelete non-atomic. That was the real staleness, and this PR already corrected it. ⇒ The docs check finding nothing here is the correct outcome, not a gap in my search.

⚠️The limit, stated: this checks pages naming the class or the bulk methods. It does not discharge the bot's own declared blind spot — a page stating a rule by its inputs shares no identifier with the emitter and cannot be listed on any run. I have not hand-re-read every page that might describe batch write semantics in other words.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 16:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Auto-merge disarmed pending one fix — found in PM review, not by CI. Recording here so another seat does not re-arm it.

This PR is otherwise strong and all-green (36/36). The governed-surface predicate was re-run on the final file list and returns 0 — not governed, so ordinary queue landing applies once the fix lands.

The defect

In the new bulkUpdate, 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 pass an id whose JS type differs from the stored row's. When that happens touchedIds.has(r.id) is false while findIndex still resolves the row — so the row is updated and left in settled carrying its pre-image, while pending holds its post-image. It is then counted twice in the uniqueness check.

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'}}]).
  • Row 1 vacates D-0001 and row 2 takes it, which must succeed — but row 1's stale pre-image in settled still carries D-0001, so row 2 gets a false UNIQUE_VIOLATION.

Why the sibling door does not have it

updateMany (#13197) draws targetIds from table rows rather than caller input, and its findIndex uses strict ===, so both comparisons agree by construction. This PR generalized the discipline (check everything before writing anything) but not that internal consistency — so the body's claim of a faithful generalization needs one qualification.

This is a regression introduced by this PR: the previous Promise.all(updates.map(u => this.update(...))) shape had no settled set and could not false-refuse this way.

Direction

Derive settled from the same resolution the write uses — resolve each update to its table index first (as this PR's own bulkDelete already does), then build the touched set from the resolved rows' own ids. ⛔ Not by tightening findIndex to ===: that would silently narrow which ids resolve at all, and update() itself uses ==, so this door must keep matching its sibling's resolution semantics.

A regression test with mixed id types, plus a positive control, is required with the fix.


Generated by Claude Code

Merged via the queue into main with commit 4642f4cAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13435-bulk-update-delete-atomic branch August 31, 2026 16:43
zhuangjianguo pushed a commit that referenced this pull request Aug 31, 2026
…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
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 1, 2026
…own id resolution (objectstack-ai#13917)
* fix(driver-memory): bulkUpdate's touched-row set must agree with its 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(driver-memory): cite the issue this fix closes, not the PR that introduced the defect
Neighbouring comments in this file cite the ISSUE (`[objectstack-ai#13435]`, `[objectstack-ai#13197]`,
`[objectstack-ai#13340]`); this one cited objectstack-ai#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
* test(driver-memory): cite the issue the new block pins, matching the 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
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@zhuangjianguo@claude