Skip to content

feat(sql): a multi-statement request is one transaction (TC-1491, 1a part A) - #384

Open
samgbafa wants to merge 12 commits into
mainfrom
tc1491/1a-atomic-requests
Open

samgbafa wants to merge 12 commits into
mainfrom
tc1491/1a-atomic-requests

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

TC-1491 slice 1a, part A: a multi-statement SQL request is one transaction. Split from the rest of 1a (PR B) so it merges first and can be reverted alone. Zero migrations, no setting, no schema change.

Stacked on #376 (head 5ccc305, bounded reads, approved, not yet merged). Review only the diff after 5ccc305: one commit, feb8ec9; the later commits (376270d, 07af351) only merge origin/main (CHANGELOG conflicts; main's 1.21.4 section kept intact, the entry sits under Unreleased). #383 is a separate follow-up and not part of this.

Behaviour change

A batch, or an execute that carries schema statements, is now all-or-nothing. A failing request leaves nothing instead of a committed prefix.

Before: every statement ran in SQLite autocommit. If statement k+1 failed, statements 1..k stayed committed in the node's local file. persist_write runs only after a fully successful request, so Postgres did not have them. The local file and Postgres then differed until the next successful write persisted both, and that write persisted the stale prefix too. After this change the failing request leaves the local file as it was, and it matches what Postgres holds.

Not changed:

  • Single-statement requests: every write request, including a single statement, runs in the transaction.
  • A request containing a PRAGMA (admin only) keeps running statement by statement. foreign_keys is a no-op inside a transaction and journal_mode errors.
  • GrowthPolicy::Forbid keeps its existing savepoint guard. When it is active the new wrapper stays out, so its error precedence (growth reported ahead of a SQL error) is unchanged.
  • Response shape and error types.
  • Telemetry: such a request counts two more native SQL calls (SAVEPOINT, RELEASE/ROLLBACK), matching how the Forbid guard's savepoint is already counted. The expected value in embedded_request_telemetry.rs goes 4 to 6, and docs/request-telemetry.md says so.

Mechanism and invariants

  • handle_message (sql/database.rs) wraps handle_request (the old body, unchanged) in an outermost SAVEPOINT tinycloud_request when is_atomic_request and no transaction is open.
  • Success: RELEASE is the COMMIT. A COMMIT failure (deferred constraint) is followed by ROLLBACK and returns the error.
  • Any error: ROLLBACK, not ROLLBACK TO + RELEASE. The latter commits an empty write transaction and rewrites the database header (change counter, bytes 27 and 95). I found this because the byte-for-byte test failed on it.
  • Users still cannot issue transaction control: the authorizer denies Transaction/Savepoint actions (authorizer.rs:488), and tests try BEGIN, COMMIT, ROLLBACK, END, SAVEPOINT, RELEASE tinycloud_request, ROLLBACK TO tinycloud_request in both orders and in schema.
  • Not claimed: durability. This PR does not touch synchronous or fsync (PR B). It changes which statements are kept, not when they reach disk.

Test evidence

New, in sql/database.rs and sql/service.rs, each run for in-memory and file-backed databases where it applies:

  • tc1491_failed_batch_leaves_the_local_file_unchanged: failing batch (UPDATE, INSERT, then a NOT NULL violation). Rows unchanged, and the checkpoint bytes are identical before and after (Allow). Under Forbid the content is checked; its pre-existing ROLLBACK TO+RELEASE bumps the header counter, which I left alone.
  • tc1491_failed_schema_request_leaves_no_schema_behind: schema statements are undone when the statement fails.
  • tc1491_successful_batch_and_ddl_batch_apply_in_full: success path unchanged (4 results, last_insert_row_id), and a failing DDL batch undoes its DDL.
  • tc1491_user_transaction_control_is_still_denied: see above; no prefix commits.
  • tc1491_pragma_requests_keep_running_statement_by_statement.
  • tc1491_failed_batch_matches_what_the_durable_artifact_holds (service level, counting repository): a failing batch makes zero repository calls and leaves the checkpoint unchanged. A following successful 3-statement batch makes exactly one persist call (save or save_delta), no load_meta, so no extra Postgres round trip. After that write a cold service hydrated from the repository returns exactly the local rows, and none of the failed batch's rows.

Changed existing test: tc626_review_serialized_growth_is_rejected_and_discarded built a local-ahead-of-durable state with a failing request that left a prefix, which is exactly the behaviour removed here. It now writes through the actor handle directly (no persist) to get the same state; the assertion it protects (durable-size comparison under Forbid) is unchanged.

Run: cargo fmt --check, cargo clippy --workspace --all-targets -D warnings clean; cargo test --workspace --exclude tinycloud-sdk-wasm --no-fail-fast: 0 failures. A first full run on a host at load 37 had 5 timing-sensitive tc1274 failures; they pass on rerun and in isolation.

Clients and docs that might assume the prefix behaviour

Searched ~/github/tinycloud-dev/repositories/js-sdk (sdk-services, operations, cli), the docs repo and this repo's docs/ and README.md.

  • js-sdk ISQLService.batch: "Execute multiple statements in a transaction." The SDK already promises atomicity; this makes it true.
  • SQLService.test.ts batch tests use mocked fetch (signing/PRAGMA/schema permission); no assertion about partial application.
  • CLI commands/sql.ts: no batch/transaction handling.
  • The docs repo has no page describing partial batch application.
  • Node docs/: nothing documented the prefix behaviour. The only text that needed a change is docs/request-telemetry.md (query counts).
  • Nothing found that relies on the prefix. The code comment Hooks ... MVP intentionally under-emits rather than guessing partial success (batch write hooks) was written for it and is now moot: a failed batch emits nothing and applied nothing.

Rollback

Revert feb8ec9; no data format or migration is involved.

Update: merged main (#389: TC-1581/1585/1586), head 9df7eec

  • Main's actor now calls handle_message_authorized directly, so the request transaction wraps there. handle_request_authorized is main's former body, unchanged; handle_message is the #[cfg(test)] wrapper. Main's authority, classifier, authorizer scoping and DDL policy are kept as-is.
  • Classifier: the wrap decision (is_atomic_request) counts statements and looks for a PRAGMA; it never asks whether a statement writes, so TC-1585's corrected classification needs no change here. A WITH … INSERT in a batch is wrapped like any statement. New test tc1491_nested_write_cte_in_a_failed_batch_leaves_nothing: a CTE write followed by a failing statement leaves the checkpoint unchanged and makes zero repository (Postgres) calls; the success case persists and a cold node matches.
  • TC-1586 reset: it runs in the actor loop after the request returns, i.e. after RELEASE/ROLLBACK, so the reset PRAGMAs never run inside the transaction. A request with a PRAGMA is still excluded from the wrap (parser::is_pragma_sql).
  • Fixtures: service tests that create tables now hold sql/admin (DDL needs schema). tc1491_pragma_requests_keep_running_statement_by_statement uses PRAGMA user_version = 7 (allow-listed) instead of foreign_keys = OFF, which the TC-1586 allow-list now refuses.
  • Behaviour changed relative to the earlier head: none in this PR's own feature.
  • Checked locally: fmt, clippy -D warnings, tinycloud-core all tests (598 lib), tinycloud-node lib (613).

Naomi and others added 7 commits October 19, 2026 18:44
Replace DatabaseArtifactRepository::load/load_sized with load_meta (metadata
columns only) and read_chunks (4 MiB substr slices of payload then
delta_payload, guarded by revision + checkpoint hash, hashes verified while
streaming) into an ArtifactSink. SQL and DuckDB hydration stream into
staged cache files (published only after the read verified); export and the
Forbid-growth size check read metadata only. Drops the bulk-read scope.

Adds docs/runbooks/tc-822-artifact-payload-storage.md (Postgres STORAGE
EXTERNAL operator step).
The slice statement was not on the read-only call allowlist, so a lost
connection mid-slice was not retried (blackhole_mid_hydration CI failure).
A batch, or an execute with schema statements, ran each statement in SQLite
autocommit, so a later failure left a committed prefix in the local file that
the durable artifact (saved only after a fully successful request) did not
have. Run such requests in an outermost SAVEPOINT and ROLLBACK on any error.
Requests with a PRAGMA and single statements are unchanged; the Forbid guard
keeps its own savepoint. Tests, docs/request-telemetry.md and CHANGELOG.
Naomi added 2 commits October 20, 2026 06:49
Main's actor calls handle_message_authorized, so the request transaction
wraps there (handle_request_authorized is main's former body). Fixtures that
create tables hold admin; the PRAGMA test uses user_version (allow-listed).
Adds a test that a WITH ... INSERT in a failed batch leaves nothing.
@samgbafa

Copy link
Copy Markdown
Contributor Author

CHANGES REQUESTED: see TC-1491 (head 95186f6)

A request that writes (execute, batch, a named statement, or a query that
writes or carries a PRAGMA) runs in one SAVEPOINT transaction: a failing
statement or a deferred constraint at the commit rolls the whole request
back, including a single statement that stops with INSERT OR FAIL or a
column constraint with ON CONFLICT FAIL. One decision function,
needs_transaction, says which requests are wrapped. The actor retires a
connection that is still inside a transaction after its request, as it
does when the TC-1586 reset fails.

Details: TC-1491

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant