drivers(sql): an upsert that merges on a non-PK conflict key silently REWRITES the existing row's primary key — measured on SQLite and MySQL alike

Found while measuring #8592. Unassigned. Not MySQL-specific, and not about conflict-target validity — this reproduces on a perfectly well-formed upsert whose conflict target IS backed by a unique index.

What was measured

Table with email declared unique: true — the conflict target is properly backed, so this is the supported path, not an error path. Two upserts merging on ['email']:

upsert({email:'x@b.com', title:'first'},  ['email'])
upsert({email:'x@b.com', title:'second'}, ['email'])

Observed — one row throughout, as intended, but watch the id:

[sqlite] before=[{id:'rSxhabt0nVz-9RsL', title:'first'}]
         after =[{id:'hXqcSC45F3IXFr5k', title:'second'}]   idPreserved=false
[mysql]  before=[{id:'euKyrh1dzHE153eC', title:'first'}]
         after =[{id:'Ycqn7yHaGHvUXi-c', title:'second'}]   idPreserved=false

SQLite (in-process) and live MySQL 8.0.46 both rewrite it. Postgres was not raised for this run, so it is deliberately NOT claimed here — the mechanism below is dialect-independent and predicts the same result, but that is an inference and wants the conformance runner to confirm it rather than a third row in the table above.

Why it happens

SqlDriver.upsert mints an id whenever the caller does not supply one:

} else if (toUpsert.id === undefined) {
  toUpsert.id = nanoid(DEFAULT_ID_LENGTH);
}

and then builds the merge set as everything not insert-only:

const insertOnlyColumns = this.insertOnlyUpsertColumns(object);
const mergeColumns = Object.keys(formatted).filter((c) => !insertOnlyColumns.has(c));

insertOnlyUpsertColumns covers created_at and auto_number columns (#7011) — not id. So id travels in the merge set:

insert into `t` (…) values (…)
  on conflict (`email`) do update set …, `id` = excluded.`id`, …

When the conflict key IS the primary key (the default ['id'] path) that clause is a no-op — the value is identical, which is why this has stayed invisible. The moment conflictKeys names anything else, the losing insert's freshly minted nanoid overwrites the winning row's identity.

Why it matters

id is the platform's stable row identity: it is what relationships, audit records, external id mappings and any client-held reference point at. A merge-path upsert on a business key is the ordinary way to ingest external data — and today every such call silently re-identifies the row it merged into. No error is raised on any dialect.

This is the same argument #7011 already accepted for auto_number — "the number is an externally visible business identifier once assigned" — and the primary key is the strongest instance of it.

Likely fix

Treat the primary key as insert-only on the merge path, i.e. add id to insertOnlyUpsertColumns, exactly as created_at and auto_number already are. Wants checking against the _id alias branch, and against any caller that deliberately re-keys a row (update() is the deliberate path, per #7011's reasoning).

Existing coverage does not catch it: the control in sql-driver-upsert-conflict-target-dialects.test.ts asserts row count and title after a backed merge, never the id.

Related: #7011 (the same exclusion argument, for auto_number), #8592 (where this surfaced), #8621, #8567.

Blocked-by: #8592

Triage: lands in packages/drivers/driver-sql (SqlDriver.upsert — insertOnlyUpsertColumns, sql-driver.ts:4981 verified on current origin/main) ⇒ domain:drivers, type Bug, state pm:blocked on #8592.

Classification rationale: invariant repair with no open question — id is the platform's stable row identity, and upsert 每次「合并到既有行」都烧一个自增号,并覆写该行已有的业务号(健康计数器上实测,与陈旧无关) #7011 already accepted the identical insert-only exclusion argument for auto_number; the primary key is the strongest instance of the same class. Mechanical boundary test: accept set unchanged (the same calls succeed; the merge simply stops rewriting row identity) ⇒ repair lane, straight to the queue once unblocked.
Blocked-by rationale: drivers(sql): what MySQL does with a conflict target it cannot honour is unmeasured — knex compiles the target away, so the refusal cannot arise there #8592 (in flight) lands the characterization pins in sql-driver-upsert-conflict-target-dialects.test.ts, the same file this card's regression test must extend. Same-file hard serial; the unlock scan returns this card to the queue when drivers(sql): what MySQL does with a conflict target it cannot honour is unmeasured — knex compiles the target away, so the refusal cannot arise there #8592 closes.
Serial note for the drivers seat: drivers(sql): MySQL silently merges an upsert on a unique key the caller never named, and rewrites the merged row's primary key — measured, and it is a wrong write with no error #8621 shares the same code region (the mergeColumns build) and the same test file — never batch the two together; sequence them at the seat's discretion.
The Blocked-by: line rides this comment's first line, not the body: MCP body round-trip is unsafe (entity-escaped reads, platform-readings fact table), and unlock scans read comment first lines per the same table.
Size/model suggestion: S–M, mode:subagent, sonnet–opus.

Generated by Claude Code

Unlock sweep: blocker cleared. pm:blocked → pm:queue.

Declared blocker: objectstack#8592 — verified closed against the GitHub API on 2026-08-14. No other blocker is declared on this card.

⚠️ This card's Blocked-by: line lives in a comment, not the body. The unlock sweep greps the body, so this card was not discoverable by the automated sweep and was found only by a manual read of every pm:blocked card. Several seats recorded that they placed the line in a comment deliberately, to avoid the MCP body round-trip re-encoding the card's punctuation as HTML entities. That trade-off is sound per card and structurally lossy in aggregate: 26 of the 40 open pm:blocked cards across the three repos carry no body-level Blocked-by:. The durable fix belongs in the sweep (grep comments too), not in asking seats to risk body rewrites.

Authorization: maintainer directive (this session) —「先做 23 张 blocked 解锁 10 张 on-hold 按各自自述处理」· PM session 01JaVVMrSxt7Tgi1uwEuDtH7

Generated by Claude Code

Claim: PM loop round 1 (concurrency raised to 5 by the maintainer mid-round)
Session: session_01XeQRiAa7vYRVX5Fog7Zby8
Branch: claude/issue-8622-upsert-id-insert-only
Worktree: objectstack-issue-8622
Domain: domain:drivers
File surface: packages/drivers/driver-sql/src/sql-driver.ts — the insertOnlyUpsertColumns / mergeColumns region (:4981 and :5044 on origin/main) — plus packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts (stop on breach; explain in the report)
Container & model: S–M, mode:subagent, model: opus
Serial constraints cleared: #8592 closed, its characterization pins landed as 424bbd4. ⚠️ #8629 is in flight in domain:metadata and touches this same package — but only sql-driver-sys-setting-organization-unique.test.ts, a different file with a different subject. I held this card one round on the "same package ⇒ serialize when unsure" rule; I am no longer unsure, because both file surfaces are now measured and declared in writing on both cards. Releasing it with the alternative discipline applied voluntarily: region-level declaration on both sides, merge main before opening the PR, merge again once the sibling lands, and conflicts go to the merge queue rather than being hand-ordered. #8621 shares this card's exact code region and stays hard-serial behind it.

Dispatching under the maintainer's direct-dispatch instruction to this PM session (2026-08-14, verbatim, untranslated): 「metadata ＋ engine-core ＋ drivers 优先处理bug」, and 「并发加到5」. Lane seat #6020 is vacant.

Note on ordering: this card lands before #8621 deliberately — #8621's consequence ② (the merged row's primary key is replaced) is the general form of this card's defect, so fixing id-in-the-merge-set first shrinks #8621 to its MySQL-specific half.

Generated by Claude Code

{
  "issue": 8622,
  "status": "done",
  "branch": "claude/issue-8622-upsert-id-insert-only",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/8741",
  "premise_still_valid": true,
  "summary": "Premise confirmed, and the card's Postgres caveat retired by measurement: I raised system PG 16.13 in-container (initdb + pg_ctl) and reproduced the rewrite there as well as on SQLite, on a properly BACKED conflict target. `id` now joins `created_at` and the auto_number columns in `insertOnlyUpsertColumns`, resolved through `remoteColumn` rather than as a literal — the shipped ADR-0015 columnMap fixture binds `cust_id: 'id'`, so a literal would filter nothing on federated objects. Two things the card asked to check rather than assume: the `_id` alias folds into `id` before the merge set is built, so one exclusion covers both (pinned), and `update()` still re-keys deliberately (pinned on both cells). One thing it did not predict: excluding `id` makes the empty-merge-set fallback REACHABLE for the first time, and a bare `insertion.merge()` is merge-ALL — measured on live PG renumbering an auto_number CASE-0005 to CASE-0006, defeating #7011. Closed in-scope with a fallback to the conflict-target columns, which emits SQL byte-identical to main's for that shape, so the PR's net behavioural delta is the id exclusion and nothing else. SQLite cannot see that branch (stampInsertTimestamps keeps a mergeable updated_at in the payload), so raising PG is what caught it. Accept set unchanged throughout, per the triage ruling. #8592's MySQL pin flips from characterization to contract, as its own failure message instructed.",
  "tests": "All quoted at HEAD 2a4c120 (final commit = the `main` merge; nothing committed after). `pnpm --filter @objectstack/driver-sql typecheck` pass. `pnpm --filter @objectstack/driver-sql test` with OS_TEST_POSTGRES_URL attached and TZ=America/New_York: 99 files passed | 1 skipped, 1868 tests passed | 32 skipped. (An earlier run without TZ failed one temporal-conformance case — that is the matrix's own three-way zone-skew non-vacuity guard firing on my provisioning, not the diff; green once the process zone was skewed as CI does.) Gate union, all pass: check:nul-bytes, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:type-check-coverage, check:type-check-debt (built closure via turbo, 33 ledger entries re-measured, none above its recorded number), plus the changeset family the dispatch list did not name and re-derivation surfaced: check:changeset-gate-self-tests, check:objectui-changeset, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset. REVERSE VERIFICATION, direction predicted before running and deliberately NOT uniform. Leg 1, revert the id exclusion: predicted the two identity pins red on both cells, `created_at` and `update()` GREEN (independent limbs, not one lever), and the exhaustion pin GREEN because without the exclusion the merge set never empties — measured 4 failed / 20 passed, matched exactly. Leg 2, revert only the fallback: predicted red on live PG, green on SQLite — measured 1 failed / 23 passed, 'expected CASE-0006 to be CASE-0005'. Fix was committed BEFORE both legs and restored with `git restore --source=HEAD --staged --worktree`; byte-identity confirmed by `git hash-object` matching HEAD's blob (9399dc3), not by a --stat line count. No dogfood ablation in this card, so no dist rebuild claim to make; the driver-sql suite runs against src via vitest, and the dependency closure was rebuilt after the `main` merge before any of the numbers above were taken.",
  "open_questions": [],
  "out_of_scope_findings": [
    "filed as #8740 (label `finding`, no `pm:queue`, unassigned): `upsert`'s last-ditch empty-merge-set fallback is still merge-ALL. Now unreachable — it needs the conflict target absent from the payload, so the INSERT never supplies it and the NULL target cannot collide — but the reachability argument leans on NULL-vs-unique semantics being uniform across dialects, and any future addition to insertOnlyUpsertColumns re-opens it silently. Not widened into this PR because the clean spelling is not accept-set-neutral: knex compiles .onConflict().ignore() to MySQL's `insert ignore`, which swallows unrelated errors and drops the conflict target (measured), so it needs a decision rather than a rider."
  ]
}

Generated by Claude Code

ACCEPT — PR #8741. The scope stretch is confirmed, and it is the reason this PR is not a regression.

PM session session_01XeQRiAa7vYRVX5Fog7Zby8. Diff read on GitHub. Landing waits on gates — with one cell called out below that only CI can settle.

The catch that matters most is one nobody asked for

Excluding id made the empty-merge-set fallback reachable for the first time, and a bare insertion.merge() is merge-ALL — it re-admits every insert-only column. Measured on live Postgres with an object whose only non-id column is an auto_number:

upsert({ email:'f@b.com' }, ['email'])  ->  case_no = 0003
upsert({ id: <seeded> },    ['id'])     ->  case_no  0003 -> 0004   ← #7011 defeated

So the fix as scoped would have re-opened the exact defect class it inherits its argument from, in the same PR that closes it. And SQLite cannot see that branch — stampInsertTimestamps keeps a mergeable updated_at in the payload there — so a green run on the default dialect would have shipped it. Raising Postgres is what caught it.

Confirmed: keep the fallback fix. Shipping the id exclusion alone would have been shipping a known regression, and "the card only named one line" is not a reason to do that. The remedy's scope argument is what makes it safe rather than opportunistic: falling back to the conflict-target columns present in the payload is a provable no-op — a conflict means those columns matched, so excluded.<target> is already the stored value — and it emits SQL byte-identical to what main emits for that shape. The PR's net behavioural delta really is the id exclusion and nothing else.

Verified, not taken from the report

File surface: exactly three files. sql-driver.ts, the upsert dialect test, and the changeset. ⛔ sql-driver-sys-setting-organization-unique.test.ts — sys_setting's declared row identity is unenforced on every tenant and global row — user_id is NULL there and SQL UNIQUE is NULL-distinct #8629's pin — is untouched, as instructed. Changeset is @objectstack/driver-sql: patch with the ADR-0087 not-required rationale.
remoteColumn, not a literal: new Set(['created_at', this.remoteColumn(object, 'id', 'id')]). The justification is measured against a shipped fixture rather than argued — sql-driver-external-columnmap.test.ts binds cust_id: 'id', so a literal 'id' would filter nothing on precisely the federated objects federation exists to serve.
The identity pin cannot pass vacuously: it asserts the merge still collapses to one row and that title still merged, before it asserts the id. Without those two, "the id is unchanged" would also pass if the call had started refusing or inserting a second row. That is the anti-vacuity shape this card needed, since the whole point is that the accept set does not move.
Both things the card asked to check, checked rather than assumed: the _id alias folds into id before the merge set is built, so one exclusion covers both — pinned explicitly so the alias cannot regress onto a second code path; and update() remains the deliberate re-key path, pinned on both cells. That last one is what makes this a repair rather than a capability removal.
Reverse verification, two legs, both non-uniform and predicted: reverting the exclusion turned the identity pins red on both cells while created_at and update() stayed green (independent limbs, not one lever), and the exhaustion pin stayed green because without the exclusion the set never empties. Reverting only the fallback went red on Postgres and green on SQLite. Byte-identity on restore proved with git hash-object, not a --stat line count.

The Postgres measurement retired a caveat the card refused to claim

The card deliberately did not claim Postgres — it said the mechanism predicted it but that this was an inference. Standing up PG 16.13 in-container and reproducing it converts that inference into a measurement, and it is also what surfaced the fallback defect. Exactly the right instinct.

⚠️ One cell is un-run, and the PR says so where it matters

The MySQL pin is flipped from not.toBe to toBe — correct, since #8592's own failure message instructed whoever fixed it to rewrite it — but that container had no mysqld, so the assertion was never executed where it was written. The PR carries that caveat in the test's own docblock, along with the right instruction: if CI's Temporal Conformance (live PG + MySQL) is red there, the driver-level reasoning is wrong for MySQL specifically and that deserves its own card — ⛔ do not relax it back to not.toBe without one.

I am treating that job's conclusion as a landing precondition rather than a formality, and will read it directly. The driver-level argument is sound (the merge set is built in-process, before any dialect sees a statement), but "sound" is not "measured", and the PR is honest about which it has.

The two surrounding MySQL pins are correctly left alone as #8621's to move.

Out-of-scope work

#8740 filed for the remaining merge-ALL fallback, with the reachability argument stated and its own weakness named — it leans on NULL-vs-unique semantics being uniform across dialects, and any future addition to insertOnlyUpsertColumns silently re-opens it. Not widened into this PR for a measured reason rather than a scope reflex: .onConflict().ignore() compiles to MySQL's insert ignore, which swallows unrelated errors and drops the conflict target. That makes the clean spelling an accept-set change, which needs a decision, not a rider.

#8621 stays hard-serial behind this, and lands next in this lane — now smaller, since its consequence ② is what this card just fixed.

Generated by Claude Code

fix(driver-sql): treat `id` as insert-only on the upsert merge path (o…

fix(driver-sql): refuse a MySQL upsert whose named conflict target an…

docs(types): correct error-leak.ts's false MySQL dialect claim, and p…

fix(metadata-protocol): correct dialect-coverage wording in driver-te…

fix(spec): os migrate meta guidance for the driver-*, kernel-* and sy…

docs(types): re-anchor the dead tracker citations in packages/types/s…

feat(driver-turso): the remote transport issues auto_number values fr…

Navigation Menu

Repository navigation

drivers(sql): an upsert that merges on a non-PK conflict key silently REWRITES the existing row's primary key — measured on SQLite and MySQL alike #8622

What was measured

Why it happens

Why it matters

Likely fix

ACCEPT — PR #8741. The scope stretch is confirmed, and it is the reason this PR is not a regression.

The catch that matters most is one nobody asked for

Verified, not taken from the report

The Postgres measurement retired a caveat the card refused to claim

⚠️ One cell is un-run, and the PR says so where it matters

Out-of-scope work

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

drivers(sql): an upsert that merges on a non-PK conflict key silently REWRITES the existing row's primary key — measured on SQLite and MySQL alike #8622

Description

What was measured

Why it happens

Why it matters

Likely fix

Activity

hotlong commented on Aug 14, 2026

claude commented on Aug 14, 2026

hotlong commented on Aug 14, 2026

hotlong commented on Aug 14, 2026

hotlong commented on Aug 14, 2026

ACCEPT — PR #8741. The scope stretch is confirmed, and it is the reason this PR is not a regression.

The catch that matters most is one nobody asked for

Verified, not taken from the report

The Postgres measurement retired a caveat the card refused to claim

⚠️ One cell is un-run, and the PR says so where it matters

Out-of-scope work

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions