Skip to content

feat(connection): reject adbc.connection.readonly changes while a manual transaction is active - #280

Open
fornwall wants to merge 2 commits into
mainfrom
readonly-toggle-guard
Open

feat(connection): reject adbc.connection.readonly changes while a manual transaction is active#280
fornwall wants to merge 2 commits into
mainfrom
readonly-toggle-guard

Conversation

@fornwall

Copy link
Copy Markdown
Owner

Follow-up to #275.

What

Changing adbc.connection.readonly while a manual transaction is active — a first statement has fixed the transaction's kind (a query opened the shared read-only snapshot, or DML/ingest buffered work) — now fails with InvalidState, in both directions. Two deliberate softenings:

  • Re-setting the current value is a no-op and stays allowed (nothing changes, so there is nothing to reject).
  • A fresh manual transaction with no statement yet is not active: readonly can still be configured after autocommit=false, before the transaction does anything — so readonly=true + autocommit=false remains the way to get declared-read-only transactions.

Why

  • Spec: the ADBC spec is silent — adbc.connection.readonly is specified in one sentence with no toggle/transaction semantics, and post-init SetOption support is explicitly driver-defined. No other ADBC driver implements the option at all (surveyed apache/arrow-adbc C/Go/Rust and the whole adbc-drivers org: the string appears only in vendored adbc.h headers), so there is no convention to follow. The nearest prior art is JDBC's Connection.setReadOnly, which "cannot be called during a transaction" — this PR adopts that rule.
  • Soundness: previously the flag only gated statement admission; commit() never checked it, so DML buffered while writable would still be committed after the connection was flipped read-only. Rejecting the flip closes that hole by construction: a read-only connection can never be holding buffered writes.

How

TxnState::in_active_manual_txn() (manual mode ∧ kind fixed) gates the set_option arm; the check-and-store happens under the SharedTxn lock, so a concurrent statement cannot fix the transaction's kind between the check and the store.

Tests

  • Unit: active_manual_txn_tracks_the_fixed_kind — false in autocommit and in a fresh manual transaction, true once DML buffers, false again after a drained commit.
  • Mock (offline): readonly_toggle_is_rejected_in_an_active_manual_transaction — free toggle before any statement; rejection with DML buffered (→ true) and inside a query transaction (→ false); no-op re-sets allowed; rollback/commit free the toggle again.
  • Full Rust + emulator suites green on the rebased branch (3 consecutive clean emulator runs).

Docs updated: README, python/README.md, docs/options.md, lib.rs crate docs, read_only field rustdoc, CLAUDE.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ATAMk97A9CZw2MX1Xkp9j1

fornwall and others added 2 commits July 14, 2026 11:02
…is active

Changing adbc.connection.readonly while a manual transaction is active —
its kind fixed by a first statement — now fails with InvalidState, in
both directions (the JDBC setReadOnly rule; the ADBC spec is silent on
toggle timing, and no other ADBC driver implements the option at all).
Re-setting the current value is a no-op and stays allowed, and a fresh
manual transaction with no statement yet leaves the toggle free.

This closes the hole where DML buffered while the connection was
writable could still be committed after the connection was flipped
read-only: the flip itself is now rejected, so a read-only connection
can never have buffered writes to commit. The check-and-store happens
under the SharedTxn lock, so a concurrent statement cannot fix the
transaction's kind between the check and the store.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATAMk97A9CZw2MX1Xkp9j1
@fornwall

Copy link
Copy Markdown
Owner Author

CI failure diagnosed and fixed by merging current main (34e65c9).

What broke: every functional check failed with error[E0599]: no method named resetfound for structruntime::CancelSlot`` (plus E0308 mismatches) — the #272 vs #275/#279 cancel-API semantic conflict. This PR's own code never touches CancelSlot; its CI runs happened to build the merge ref during the window when `main` itself was broken (after #272 landed, before the #281 repair). Lint-only jobs passed because they don't compile the crate against the merge.

What changed: a plain merge of origin/main — no code adaptation was needed, since main now carries #281. Verified the guard still composes semantically with main's reworks: #269's manual-mode ingest buffering (buffer_mutation under the txn lock) still fixes the transaction's kind, so in_active_manual_txn() correctly blocks the readonly flip with buffered ingest mutations too, and the diff vs main is exactly this PR's intended change.

Local verification (post-merge): cargo fmt --check, clippy --all-targets --all-features -D warnings, and scripts/with-emulator.sh cargo test all green — 290 unit, 52 emulator integration, 20 mock, 6 resilience, 1 doctest; includes the PR's active_manual_txn_tracks_the_fixed_kind and readonly_toggle_is_rejected_in_an_active_manual_transaction.

🤖 Generated with Claude Code

https://claude.ai/code/session_019HPd57uCH8jpoMhApWd5uC

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