Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): refuse and roll back a MySQL upsert that merges onto a row the caller never identified (#8807) - #8940
Conversation
…R-0087 entry, changeset (#8807)
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 3 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
…merge (#8807) Both entries stack: #8936's engine-dotted-filter-refused and this card's driver-sql-upsert-cross-row-identity-merge-refused. 97 semantic entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
hotlong
commented
Aug 16, 2026
Handover note — an obligation I took on that must not live only in a session. StateThis PR was dequeued with
#8936 added its own ADR-0087 semantic entry and regenerated The obligationI told the implementing dev, verbatim: "Push when green. I will re-queue; you do not need to." Recording it here because a commitment held only in a session is a half-state. ⇒ When the dev pushes the resolved merge and CI is green, this PR needs auto-merge re-armed. It is already A self check-in is armed at 02:51Z carrying this criterion. If that fires and nothing has been pushed, it re-arms silently rather than acting.
|
…ert-unnamed-unique-key-merge
Fixes#8807
ON DUPLICATE KEY UPDATEcarries no conflict target, so on MySQL a merge lands on whichever UNIQUE key the row collides with first. #8621 closed the half where nothing backed a caller-named target; #8755 closed the half where a rival key could absorb a caller-named one. This closes the residue those two left by construction: theconflictKeys-less call and the['id']call, which compile byte-identically and which no pre-flight can judge, because neither names anything.Built on#8806's pre-flight, not beside it — the same
physicalKeyIndexesread, the same cache, the samejudge(), now returning a third verdict instead of a second guard.Premise re-verified on live MySQL 8.0.46 (both shapes)
Measured through the same knex +
mysql2pathupserttakes, againstorigin/main@716ac9bf8,emailandtax_idbothunique: true:The
['id']spelling reproduced identically. The legitimate same-id merge was unaffected.Zone 2 — the falsifier the ruling adopted
Measured: 0 of 2. The objects declaring
lifecycle.archivein this repo are exactlysys_audit_logandsys_metadata_audit; neither declares auniquefield or a unique index. The set is empty, not large, so the STOP condition does not fire — and the archiver needed no call-site change, for a stronger reason than the count (below).The enforcement chosen, and why
A post-hoc identity check, the ruling's "acceptable alternative", because measurement favoured it decisively over A-narrowed:
A-narrowed collapses into blanket A on this path. For a caller-named target, rivals are the keys other than the named one — a narrow set. Here the target is always the primary key, so every non-primary UNIQUE key is a rival: "narrowed to tables carrying a rival key" and "every table with a business unique key" are the same set. The narrowing that made a pre-flight refusal proportionate for #8755 does not exist here, so A-narrowed would have refused a legitimate
upsert(object, row)on all 43 first-party objects declaring a unique field, plus every customer object with a natural business key — the blanket ban the ruling excluded by name.The post-hoc check is exact, not heuristic.
idis insert-only on the merge path (#8622), so a row merged on the primary key always still carries the supplied id, and a row merged on any other key never does. Row-absent is therefore a biconditional for "landed on a row the caller never identified" — which is why it has zero false refusals, the property A-narrowed could not have.The biconditional needs one fact beyond #8622, and it was measured rather than assumed: when the incoming row collides on the primary key and a rival key at once, a pre-existing row carrying our id would mask a merge that went elsewhere. MySQL updates only the first matched index. Measured on live MySQL 8.0.46 — rows
R1(id=R1, tax_id=T-1)andR2(id=R2, tax_id=T-2), inserting(id=R1, tax_id=T-2):The primary key is matched first, so the masking case does not arise.
It runs inside a transaction with the statement: "must never modify" is not satisfied by noticing afterwards, so the check's failure is what rolls the wrong write back.
The identity read is tenant-scoped — to the tenant the row was WRITTEN under
The check issues a read, so it routes through
applyTenantScopeon a bound builder like every other read door (check:tenant-chokepointasserts exactly that; the builder is bound rather than used inline). The substantive question is which tenant to scope to, and the two readings differ on a real case:injectTenantOnInsertstamped the caller's org on the row. Scoping to the written tenant is scoping to the caller's org: identical to the sibling doors.injectTenantOnInsert: "explicit values are never overwritten"). The row lands under the tenant the caller named, so scoping to the caller's active org would miss a row that really was written and refuse a correct write. Scoping to the written tenant does not.applyTenantScopereturns the builder untouched whentenantIdis empty, by its own contract. That is what keeps the archiver working: it callscold.upsert(object, row, ['id'])with no options, so the read is unscoped there, exactly as before.The verdict is tenant-independent regardless —
idis the PRIMARY KEY, so at most one row can carry it.tenantIds(the ADR-0105 group-union posture) is dropped deliberately: that posture widens a read to a membership set, and this is an identity probe for one row.Pinned in all three states, and the third pin discriminates: under the naive "caller's active org" reading it goes red (ablation C below).
Selectivity — what does not pay
ON CONFLICT (id)already honours the arbiter.The archiver, first-party
cold.upsert(object, row, ['id'])is unchanged and correct by construction: it copies rows that already carry their own id, so the merge lands on the supplied identity and the check passes — including idempotent re-copies. The one case it refuses is a cold row colliding on a business key while carrying a different id, which is archival about to overwrite an unrelated archived record; there the upsert throws,bulkDeletenever runs, and hot rows survive for the next sweep (the Archiver's own safety rule). The measurement and this reasoning are recorded at the call site.Verification — at final head
bf7e01142Both branches pinned per dialect, as the ruling requires:
['id']spelling, message/cause, tenantedsql-driver-upsert-conflict-target-dialects.test.ts— 38 passed, 2 skipped against live MySQL 8.0.46 (OS_TEST_MYSQL_URL), re-run at the final head.sql-driver-tenant-scope-read-doors.test.ts— 21 passed. The chokepoint gate's own docblock names this file as the other half of its contract ("the gate proves the call is there; only the fixture proves it works"), so it is run explicitly.--filter '...@objectstack/driver-sql'— 48 packages):driver-sql1724 passed,driver-turso1002,driver-sqlite-wasm394,spec10719,objectql3721,metadata-protocol1512 — all green, zero failures.driver-sql,objectql,spec.--re-measureOK, 33 entries, 1926 raw errors, none above its recorded number. No ceiling raised.check:query-options-erasuregreen. Both ratchets were re-measured after the merge rather than assumed to carry over, since the merge imported feat(objectql,metadata-protocol): refuse a dotted filter key whose head is a relation, a formula, or a plain scalar — at both doors (#8371) #8936's test surface.Ablations — direction predicted before running, all three matched
A. Identity check disabled — predicted the refusal pins red and all controls green. Observed (4 failed / 31 passed):
B. Transaction wrapper removed, check kept — predicted the envelope pins stay green and only the stored-row pins go red, isolating the rollback. Observed exactly that (2 failed / 33 passed).
C. Scoping switched to the caller's active org — predicted exactly one red, the admin cross-org pin, with the other two tenanted pins green. Observed (1 failed / 37 passed):
All restored to byte-identity (
git statusclean, 0 ablation markers).Gates
Re-derived from actual changed paths via
scripts/pm/dispatch-gates.mjs, and — since that script derives from paths and cannot name a content-keyed gate — the entire 48-gate ESLint job was run explicitly, all green,check:tenant-chokepointincluded (20 bindings across 3 files; every read builder scoped, every unscoped one an insert). Also green:type-check-debt --re-measure,query-options-erasure,adr-0087-registration,changeset-no-major,empty-changeset,dev-prereqs,engine-split-ratio,doc-formula-expressions,migration-registry.The 48-gate union ran at
bf46f6379(the first merge commit). The second merge that producedbf7e01142brought in 10 furthermaincommits with zero file overlap with this diff (computed, not assumed), after whichtenant-chokepoint,nul-bytes,migration-registryand the live-MySQL suite were re-confirmed atbf7e01142, plus the two gates that newly landed onmainin that window —check-examples-live-imports.mjsandcheck-release-page-status.mjs— both green on this branch.check:tenant-chokepointis the gate that caught the first push: it is keyed on builder shape, not path, so the derived list was correct and still incomplete — the standing "lower bound" rule.ADR-0087: implicated and added —
entries/semantic/18.driver-sql-upsert-cross-row-identity-merge-refused.ts, matching the precedent #8790 set for a driver-sql accept-set change.Merge-conflict resolution (the
MERGE_CONFLICTdequeue)packages/spec/src/migrations/registry.tsis generated, and #8936 regenerated it for its own entry. Resolved by regeneration in the mandated order — merge, commit the merge, thengen:migration-registry— never regenerating while in MERGE state. Both intents verified by exact name in the entry file and the registry body:driver-sql-upsert-cross-row-identity-merge-refused(this PR)engine-dotted-filter-refused(#8936)97 semantic entries,
check:migration-registrygreen. #8936's implementation body was checked too, not just its index entry: this branch's net diff againstmainacrossobjectqlandmetadata-protocolis exactly this PR's 26-line lifecycle comment and nothing else. No pin was weakened, deleted or adjusted.Out of scope, as ruled
The #6943 autonumber re-seed reachability thread is untouched and unmeasured here.
Generated by Claude Code