Skip to content

Commit 62bce5c

Browse files
huangyiireneclaude
andauthored
fix(driver-turso): refuse transactions on the remote face instead of silently dropping them (#18717)
Fixes #18616 Clause-②: yes The Turso **remote** face now refuses transactions with `NOT_IMPLEMENTED` / `501` instead of accepting them and doing nothing with them. Local and embedded-replica modes are byte-for-byte unchanged in behaviour. ## What was wrong `@objectstack/spec`'s `driver.zod.ts` states the delivery mechanism verbatim: > A transaction handle to be passed to subsequent operations via `options.transaction`. On the remote transport nothing could receive it. ⭐ Every reading below was **re-derived by content on this branch's base** `f8eaf670454a`, which contains `ef67b47afb` (PR #18668) — that PR edited `remote-transport.ts` after the card was written, so the card's line numbers no longer locate anything: | reading | instrument | result | |:--|:--|:--| | `RemoteTransport` members naming a transaction | signature scan of `remote-transport.ts` | **3** — `beginTransaction()`, `commit(t)`, `rollback(t)` | | `RemoteTransport` **data** methods accepting `options` or a transaction | same file, same instrument | **0** | | ⭐ firing control — data methods present at all in that file | same file | **13** (`find`, `findOne`, `aggregate`, `create`, `update`, `upsert`, `delete`, `count`, `bulkCreate`, `bulkUpdate`, `bulkDelete`, `updateMany`, `deleteMany`) ⇒ the zero is a reading, not a dead grep | | `this.isRemote` guards in `turso-driver.ts` | `git grep -c` | **26** — matches the card | ⚠️ The card's reading said **9** data methods; re-derived by content it is **13**. The difference is in the instrument, not in the finding — the zero above and its firing control both stand, and the wider control makes the zero stronger, not weaker. So a write issued between `beginTransaction()` and `rollback()` executed on the plain connection, was **already durable**, and the rollback resolved having undone nothing. Every step reported success. The local-mode pin in `turso-driver.test.ts` ("should support transactions with rollback") already asserted the correct shape; the remote face was the unpinned one. ## The line triage drew, and this PR's radius ⛔ The deliverable is **stop the silence**, not implement remote transactions. Triage's ruling is quoted rather than paraphrased, because the boundary is the deliverable: > **止损(本卡)**:remote 模式遇到 `options.transaction` 或 `beginTransaction()` 时**大声拒绝 / 声明不支持**,让调用者立刻知道自己没有事务语义。 > **实现远程事务**:那是 **#18116 已完成的那次「measure, do not implement」量出来的半径**,是**另一件事**、另一个量级。⛔ 不要把它折进本卡。 Measured radius of the half NOT taken, reported rather than spent: implementing remote transactions means giving `RemoteTransport`'s 13 data methods an `options` parameter, threading a libsql `Transaction` through each statement builder, and deciding what `connect()`'s knex-free remote arm does about `SqlDriver`'s `openTransactions` accounting. That is a different order of change and it is not in this PR. ## The judgement the dispatch left to me: which door, and why not only one The dispatch named three candidates and asked which callers can reach a remote data method with a handle in hand. Measured, there are **two independent producers**, so the answer is **both doors**: 1. **`driver.beginTransaction()`** — every in-repo producer of a handle is one of these: `engine.transaction()`, `ScopedContext.beginTransaction()`, the sandbox trio. Closing this door closes all of them, and it closes them *before* the callback writes anything. 2. **`ExecutionContext.transaction`**, threaded by the caller. `buildDriverOptions` in `packages/objectql/src/engine.ts` reads it **first** — its own comment: *"Explicit wins; ambient is the safety net"* — and `ExecutionContext.transaction` is a declared member of the envelope in `packages/spec/src/kernel/execution-context.zod.ts`. Such a handle never passes through (1). The same-origin gate does not stop it either: `transactionCoversDriverFor` attributes a handle only when it **is** the ambient store's handle, and for anything else it declines to judge and returns `true` — its own recorded limit. ⇒ **Door (1) alone is the false floor the dispatch warned about**, and this is measured, not argued — see the second ablation below, where keeping only the `beginTransaction()` refusal leaves all 17 `options.transaction` cases green while silently dropping the handle. `commit()` / `rollback()` refuse on the remote arm too. With `beginTransaction()` refusing, this driver issues no remote handle at all, so the only way to reach them is with a handle from somewhere else — and `rollback()` accepting one is the precise silence the card is named for: it resolves, reports a successful undo, and undoes nothing. Both engine faces call `commit`/`rollback` only with a handle their own `beginTransaction()` returned, which now throws first, so no in-repo caller can be stranded with an unrolled-back transaction by this refusal. ## Where the refusal lives, and why not one layer down On `TursoDriver`, not inside `RemoteTransport` — the same layering argument the sibling `auto_number` refusal already records in this file. The transport's data methods have no `options` parameter at all, so the handle is already gone by the time a statement is built. This class is the last layer that still holds one. ## Why `NOT_IMPLEMENTED` / 501, and why no `packages/spec` edit The request is spelled correctly and the spec declares these members, so the gap is the backend's — the same two-class taxonomy this package already applies to remote `auto_number`, aggregate functions and date buckets. ⭐ **No new error code is needed and `packages/spec` is untouched**: `NOT_IMPLEMENTED` is a `StandardErrorCode` member, and the error-code ledger registers *extension* codes only, so nothing has to be added to it. The precedent is eleven hundred lines up in the same file. ## What this deliberately does not do - It does not implement transactions on the remote transport. - It does not remove `RemoteTransport`'s own `beginTransaction` / `commit` / `rollback`, which stay reachable through `getRemoteTransport()` and are pinned by an existing declared-types suite. - It does not touch `driver-sql`, the engine, or the spec. ## Verification All commands run from this branch; exit codes captured before any pipe. - **`pnpm --filter @objectstack/driver-turso test`** — `Test Files 54 passed (54)`, `Tests 1278 passed (1278)`. The pre-existing local-mode transaction pins are inside that green. - **`pnpm --filter @objectstack/driver-turso typecheck`** — exit 0. - **`pnpm --filter '@objectstack/driver-turso^...' build`** — `VERDICT command-exit 0` (dependency closure). - **Full package build** — `pnpm exec turbo run build --filter=./packages/* --filter=./packages/*/*` — `Tasks: 72 successful, 72 total`. - **Gate families** — derived with `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` (⛔ not from a hand-written diff): **60 families, 60 run, 0 NOT MEASURED, 0 UNRUN**, reconciled back through `--ran` with an exit code recorded per family. One was a real red and is fixed in this PR: `check:doc-authoring` refused the tracker id inside the refusal's runtime string (maintainer ruling 「处理 issue 时犯的错应该总结成经验,保留 issue id没有意义」) — the id moved to the docblock. Three more were `PREREQUISITE NOT MET` (exit 3) until the full build above, then green: `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt`. - **`pnpm lint`** — the repo-wide `eslint . --no-inline-config` union, exit 0, run at final commit `97009bc42e`. ⭐ This is the union, not a narrowed run, so no narrowing argument is owed. ### Reverse verification — two ablations, directions predicted before either was run Both ran from the **committed** fix, mutated on disk with the mutation proved by grep counts and a changed blob hash, and restored under a `trap` that verifies the restored blob equals the `HEAD` blob and that `git diff HEAD` is empty. Subject blob at `HEAD`: `4b2b21a0da4e`. **① Both doors removed** (the options guard emptied, the trio restored to its pre-fix delegation). Predicted: the two REMOTE blocks go red, the silence controls stay green. Measured **`21 failed | 5 passed (26)`** — 4 trio cases plus 17 `options.transaction` cases red, the 5 controls green. The failures reprint the defect rather than merely reporting an absence: `expected the remote face to refuse, but it resolved with ...`, carrying the row that landed. **② Only the `options.transaction` door removed**, the `beginTransaction` / `commit` / `rollback` refusals kept — i.e. exactly the shape of a fix that closed candidate (1) alone. Predicted: the 17 `options` cases red, the 4 trio cases and the 5 controls green. Measured **`17 failed | 9 passed (26)`**. ⭐ That is the false floor, measured: a PR that had stopped at (1) would have shown a green trio block and shipped the silent drop. Restore verified on both legs: `RESTORE OK — on-disk blob 4b2b21a... == HEAD blob 4b2b21a..., and git diff HEAD is empty`. ### Changeset, measured rather than assumed `skip-changeset` is wrong here and it was **measured**, not taken from the dispatch. `@objectstack/driver-turso` publishes and its `files[]` is `dist`, `README.md`, `CHANGELOG.md`. After building the package, grepping those paths for a symbol unique to this change gives `dist/index.js:1`, `dist/index.mjs:1`; the positive control (a pre-existing shipped refusal sentence) hits `dist/index.js:2`, `dist/index.mjs:2`, `CHANGELOG.md:2`; the negative control (a test-only helper name) hits **0** anywhere in `files[]`. ⇒ it publishes ⇒ a changeset is owed, and it is graded `minor` to satisfy the clause-② level axis while staying inside the launch-window ban on `major`. ## Acceptance notes Observations found while measuring, ⛔ not filed and ⛔ not fixed here. - **`engineCanRollBack` reads method PRESENCE and there is no way for a driver to say "present, but not on this connection mode".** `packages/core/src/utils/migration-journal.ts` gates on `typeof defaultDriver.beginTransaction === 'function'`, which is `true` for a remote `TursoDriver` because the method is inherited from `SqlDriver` at class-definition time while the transport mode is a runtime config. The spec's own retirement note for the `transactions` capability bit says transaction use "is gated on METHOD PRESENCE", so this is the declared design, not a bug in that helper. After this PR the consequence is loud rather than silent (`batchData`'s atomic gate now gets a refusal instead of a half-applied batch), so nothing is left silently wrong — but the underlying "presence cannot express a per-mode capability" shape is exactly the semantics half that #18063 and #18116 are already about, and it belongs to that decision, not to a new card. **承接者:** the #18063 ruling. - **`RemoteTransport.beginTransaction` / `commit` / `rollback` are now unreachable through `TursoDriver`.** They stay reachable via `getRemoteTransport()` and are pinned by `turso-driver-doors-declared-types.test.ts`, so they are not dead code; removing them is part of the implement-or-retire decision above, not of this refusal. **承接者:** the same ruling. Neither is a class (a) reproducible defect, a class (b) declared-contract violation, or a class (c) metadata-authoring trap, so neither is filed. --- _Generated by [Claude Code](https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5762eaf commit 62bce5c

3 files changed

Lines changed: 539 additions & 3 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
"@objectstack/driver-turso": minor
3+
---
4+
5+
`TursoDriver` in **remote** mode now **refuses** transactions with `NOT_IMPLEMENTED` / `501` instead of accepting them and silently doing nothing with them. Local and embedded-replica modes are unchanged — they inherit `SqlDriver`'s knex transactions and still honour `options.transaction`.
6+
7+
**What was wrong.** `@objectstack/spec`'s `driver.zod.ts` states the delivery mechanism verbatim: *"A transaction handle to be passed to subsequent operations via `options.transaction`."* On the remote transport nothing could receive it. `RemoteTransport` names a transaction in exactly three members (`beginTransaction()`, `commit(t)`, `rollback(t)`) and **zero** of its data methods take an `options` argument at all — against 13 data methods present in the file, which is what makes that zero a reading. So a write issued between `beginTransaction()` and `rollback()` executed on the plain connection, was **already durable**, and the rollback resolved having undone nothing. Every step reported success.
8+
9+
**What refuses now**, on the remote arm only:
10+
11+
- `beginTransaction()`, `commit()` and `rollback()` — the capability is never handed out, so the sequence above cannot start.
12+
- Any driver method that arrives carrying `options.transaction` — `find`, `findOne`, `count`, `aggregate`, `create`, `update`, `upsert`, `delete`, the three bulk methods, `updateMany`, `deleteMany`, `execute`, `syncSchema`, `syncSchemasBatch`, `dropTable`. This second door is not redundant: the engine's `buildDriverOptions` reads `execCtx.transaction` **first**, so a handle threaded through `ExecutionContext` reaches a data method without ever passing through `beginTransaction()`.
13+
14+
The refusal fires on the **handle**, not on remote mode: a remote call with no transaction in it is untouched, which is every call the platform makes today. It is raised before any statement is built, so a refused call costs no round trip and leaves no partial write.
15+
16+
**If this refusal now fires for you, it is telling you that you never had the transaction.** The remedies, in order: use the **local or embedded-replica** transport for work that needs atomicity; or take the non-transactional path deliberately — `engine.transaction()` without `require: true` on a driver with no transactions runs the callback with no rollback and says so (ADR-0119 D1). `NOT_IMPLEMENTED` / `501` rather than a `400` because the request is spelled correctly and the spec declares the members: the gap is the backend's, the same two-class taxonomy this driver already applies to remote `auto_number`, aggregate functions and date buckets.
17+
18+
Implementing real transactions on the remote transport is a separate, larger piece of work and is deliberately **not** part of this change.

0 commit comments

Comments
 (0)