fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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('^' + ".*" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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('^' + ".*" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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('^' + ".*" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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('^' + ".*" + '
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@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); } })(); })();
Skip to content

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied - #14170

Merged
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction
Sep 1, 2026
Merged

fix(driver-sql): apply bulkUpdate as one transaction so a mid-batch refusal rolls back the rows already applied#14170
os-support-ai merged 1 commit into
mainfrom
claude/issue-13854-sql-bulkupdate-transaction

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#13854

SqlDriver.bulkUpdate is a sequential for-await over individual update() calls with no transaction around it, so each update() autocommitted its own row. A batch refused partway through left every row processed before the refusal permanently committed: the caller received an exception while the database held a state nobody declared — neither the pre-image nor the post-image — and a retry of the same array was not safe.

The loop now runs inside a transaction. The loop itself is unchanged and deliberately so: each id carries its own patch, so no single statement expresses N different SET lists (which is why bulkCreate's "send the batch as one INSERT" shape does not transfer). What changed is the boundary around it.

Same defect class #13340 / #13435 closed on driver-memory's batch doors, here on the production default SQL driver.

Mechanism — the file's own, no new one and no new dependency

SqlDriver already carries transaction support, and this reuses it rather than inventing a second mechanism:

const runner: Knex | Knex.Transaction = (options?.transaction as Knex.Transaction | undefined) ?? this.knex;
return runner.transaction(async (trx) => applyAll({ ...options, transaction: trx }));

The (caller's transaction) ?? this.knex runner idiom is the one collidingAutoNumberReservations already uses; getBuilder routes every statement onto options.transaction via .transacting(trx), so the whole loop — including the rotation-shard path through rotatedUpdateById, which threads options the same way — rides the one boundary.

The choice of runner is also the choice between a transaction and a savepoint, which is what makes the caller-transaction case correct:

  • No caller transactionthis.knex.transaction() opens the driver's own, the shape upsert's verifyIdentity branch uses.
  • Inside a caller transactiontrx.transaction() opens a knex nested transaction: a SAVEPOINT, released on success and rolled back to on failure. That is the primitive attemptWithoutPoisoning is built on, already verified in this file on PG 16 and on better-sqlite3 at pool max: 1, where the savepoint rides the parent's connection and never asks the pool for a second one. This matters concretely here: SQLite hands out exactly one connection (assertBareKnexSafe's subject), so a wrapper that reached for this.knex while a caller held a transaction would dead-lock.

⚠️ Joining a caller's transaction without a savepoint — the posture upsert takes at its own boundary — would be wrong here, and the difference is N statements versus one. upsert reasons "the statement is already transactional", true of one statement and false of this loop: a caller that catches the refusal and commits anyway lands exactly the partial batch this card is about, reachable on SQLite and MySQL (Postgres aborts the whole transaction on any statement error, so it is the one dialect where the hole was already shut by accident). The half of upsert's reasoning that does transfer is kept — the rollback decision for the caller's own work stays the caller's, and its transaction is left usable.

An empty batch still issues no statement at all, so that path is byte-identical to main.

driver-turso is untouched, by design

TursoDriver extends SqlDriver and overrides bulkUpdate only to route the remote path to remoteTransport; its local path is return super.bulkUpdate(object, updates, options). It therefore inherits this repair with no subclass change — which is the point, since patching the subclass would leave the next subclass in the same hole. The remote transport path is a different door and is not addressed here.

Evidence — the pin would have been red before

packages/drivers/driver-sql/src/sql-driver-13854-bulk-update-atomicity.test.ts. §1 and §4 each assert three things, and only the third is this card:

  1. the batch refuses; 2. the call throws — both pass on the broken code too; and
  2. the earlier row is unchanged in the database, read back through a bare knex query against the physical table rather than from the return value of a throwing call.

Which refusal, measured rather than assumed. A missing id does not refuse on this door: update() issues an UPDATE matching zero rows and returns null, which the loop skips — a pin built on one would never throw and would measure nothing. §6 pins that skip, since this card must not change it. The refusal used is the database's own: a unique: 'global' column with a later row in the batch assigned a value a third row already holds, measured on better-sqlite3 as UNIQUE constraint failed: account.code.

Ablation (on the committed tree, direction predicted before running). Reverting the wrapper to main's bare loop was confirmed on disk before any reading was taken — anchor counts moved 1 to 0 and 0 to 1 in the two directions, and the blob hash moved 0bb4e792 to d8efbb46 — rather than trusting the editor's exit code:

Tests 2 failed | 4 passed (6)
FAIL §1 rolls the whole batch back when a LATER row refuses (no caller transaction)
AssertionError: expected 'first-updated' to be 'first'
101| expect((await stored('a'))?.name).toBe('first');
FAIL §4 undoes the batch inside a caller transaction, leaving that transaction usable
AssertionError: expected 'first-updated' to be 'first'
156| expect((await stored('a'))?.name).toBe('first');

Exactly assertion 3, in both sections, with the received value being the committed post-image. Assertions 1 and 2 stayed green, and §2/§3/§5/§6 stayed green — they pin what must not change. Restored with an absolute-path git checkout HEAD -- FILE, proven by an empty git diff HEAD and a blob hash back to 0bb4e792.

No build is involved in that ablation and none is owed: the pin imports ../src/index.js, a relative source path, so the run reads the mutated source directly — which the red itself demonstrates.

The reading #13854 owed on driver-mongodb

Measured, and the answer is not the same shape (mongodb-driver.ts):

  • bulkUpdate (:505) is not a per-row loop. It maps the updates into bulkOps and issues onecollection.bulkWrite(bulkOps, { session }).
  • bulkDelete (:534) is one collection.deleteMany({ id: { $in: ids } }, { session }) — the same single-statement shape as SqlDriver.bulkDelete.
  • session is getSession(options) (:687) = options.transaction as ClientSession, the same parameterisation convention.

So the autocommit loop repaired here does not exist there. A related exposure may still be reachable by a different mechanism — bulkWrite stops at the first failing op but does not undo earlier ones without a transaction, and mongo transactions need a replica set, so the repair is not a mechanical port of this one. Filed separately as #14169 rather than widening this PR into a third package, with the measured and not-measured halves marked apart.

Verification

All at 8345bb5a, the final commit.

  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2150 passed / 9 skipped (159 files), 2279 tests passed / 136 skipped, zero failures. The 9 skipped files are the live-dialect matrix with no Postgres/MySQL URL provisioned.
  • pnpm --filter @objectstack/driver-sql typecheck — clean, and measured to actually cover the new test file (tsc --listFiles reports 1 hit; this package's tsconfig includes src/**/* with no test exclusion, so the green is about the new file too).
  • pnpm --filter '@objectstack/driver-sql^...' build before the suite, so nothing read a stale dist.
  • Gate family derived at the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (35 commands) and run with exit codes captured by redirect before any pipe: 33 measured green. Three are PREREQUISITE NOT MET / exit 3 — check-test-completeness, check:dual-build-cjs-loads, check:type-check-debt — each of which needs a full workspace build and says in its own words that it is "NOT a pass and NOT a finding: nothing was measured". Recorded as NOT MEASURED, not as passes; CI runs them with the build in place.

Scope narrowing, declared: driver-sql has 48 downstream packages and the farm belongs to CI. The driver-turso question — the one that matters, since it inherits this door — was settled statically by reading its override (local path delegates to super.bulkUpdate) rather than by a suite run; a downstream build sweep was attempted and abandoned after it failed on an unrelated filter artifact (@objectstack/runtime's DTS build cannot resolve @objectstack/rest, which is not a dependency of either driver and was never built by the filter).

Clause-②: no — re-declared from the actual diff. No exported type, signature or public error shape changes; every input accepted before is accepted after and every refusal that fired before still fires. What changes is what survives a mid-batch refusal.

Generated by Claude Code


Generated by Claude Code

SqlDriver.bulkUpdate is a sequential loop of individual update() calls with
no transaction around it, so each update() autocommitted its own row. A batch
refused partway through left every row processed before the refusal committed
permanently — the caller got an exception and the database held a state nobody
declared.
The loop now runs inside a transaction. With no caller transaction the driver
opens its own; inside one it opens a knex nested transaction (a SAVEPOINT) on
the caller's, so the batch is undone as a unit while the caller's own work and
its transaction both survive.
driver-turso inherits the repair through super.bulkUpdate; no subclass change.
bulkDelete is untouched — one whereIn(...).delete() per shard, already atomic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol, a top-level class))

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

  • content/docs/releases/v17.mdx(via SqlDriver (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 — 9 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 b003cf2e8b0bb59a7e262c42bbf728a82558b714 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-support-ai@claude