Skip to content

Conversation

@jussisaurio
Copy link
Collaborator

@jussisaurio jussisaurio commented Dec 18, 2025

Background

Concurrent use a of a connection across multiple threads is not supported, but we should not crash even if a client attempts to do so.

Problem

We do crash when this happens. See: #3911

Fix

The proper behavior is to return Busy if the same connection is already doing transactional work on another thread.

To ensure the above, this PR implements Connection::atomic_swap_tx_state() which uses CAS instead of simply setting the state.

Testing

This PR also adds a regression test modeled after the reproduction in #3911 that only accepts "database is locked" errors and panics on anything else.

Been running this in a loop for a while now.

Description of AI Usage

Prompt:

> This is Turso, the Rust rewrite of SQLite. Sharing connection across threads is not 
supported, but we should still guard against trying to promote conn transaction state with 
atomics.

See test: concurrent_inserts_on_shared_connection

Then see op_transaction_inner in execute.rs:

                // 3. Transaction state should be updated before checking for Schema cookie
 so that the tx is ended properly on error
                if updated {
                    conn.set_tx_state(new_transaction_state);
                }


This should be an atomic modification where we expect the current state to still be what we
 think it is, and if not, we get Busy

Copy link

@turso-bot turso-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review @pereman2

@blacksmith-sh

This comment has been minimized.

Concurrent use a of a connection across multiple threads is not supported,
but we should not crash even if a client attempts to do so.

We do crash when this happens. See: #3911

The proper behavior is to return Busy if the same connection is already doing
transactional work on another thread.

To ensure the above, this PR implements Connection::atomic_swap_tx_state() which
uses CAS instead of simply setting the state.

This PR also adds a regression test modeled after the reproduction in #3911 that
only accepts `"database is locked"` errors and panics on anything else.
@jussisaurio jussisaurio force-pushed the cas-protect-tx-state-changes branch from 184f27f to def135d Compare December 18, 2025 10:50
@jussisaurio jussisaurio marked this pull request as draft December 18, 2025 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants