Skip to content

Fix sqlite autocommit lifecycle #8387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 4 commits into from
Jul 27, 2026
Merged

Fix sqlite autocommit lifecycle #8387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afb45f6
fix: open disabled sqlite autocommit transactions
teddygood Jul 11, 2026
65f950c
fix: reopen disabled sqlite transactions after commit
teddygood Jul 11, 2026
0585423
fix: reopen disabled sqlite transactions after rollback
teddygood Jul 11, 2026
8c52401
fix: roll back disabled sqlite connections on close
teddygood Jul 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions Lib/test/test_sqlite3/test_transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,6 @@ def test_autocommit_setget_invalid(self):
with self.assertRaisesRegex(ValueError, msg):
sqlite.connect(":memory:", autocommit=mode)

@unittest.expectedFailure # TODO: RUSTPYTHON; autocommit behavior differs
def test_autocommit_disabled(self):
expected = [
"SELECT 1",
Expand All @@ -411,7 +410,6 @@ def test_autocommit_disabled(self):
cx.commit()
cx.rollback()

@unittest.expectedFailure # TODO: RUSTPYTHON; autocommit behavior differs
def test_autocommit_disabled_implicit_rollback(self):
expected = ["ROLLBACK"]
with memory_database(autocommit=False) as cx:
Expand All @@ -438,7 +436,6 @@ def test_autocommit_enabled_txn_ctl(self):
meth() # expect this to pass silently
self.assertFalse(cx.in_transaction)

@unittest.expectedFailure # TODO: RUSTPYTHON; autocommit behavior differs
def test_autocommit_disabled_then_enabled(self):
expected = ["COMMIT"]
with memory_database(autocommit=False) as cx:
Expand Down Expand Up @@ -472,7 +469,6 @@ def test_autocommit_enabled_ctx_mgr(self):
self.assertFalse(cx.in_transaction)
self.assertFalse(cx.in_transaction)

@unittest.expectedFailure # TODO: RUSTPYTHON; autocommit behavior differs
def test_autocommit_disabled_ctx_mgr(self):
expected = ["COMMIT", "BEGIN"]
with memory_database(autocommit=False) as cx:
Expand Down
63 changes: 45 additions & 18 deletions crates/stdlib/src/_sqlite3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ mod _sqlite3 {
zelf.reset_factories(vm);

if was_initialized {
zelf.drop_db();
zelf.drop_db(vm)?;
}

// Attempt to open the new database before mutating other state so failures leave
Expand Down Expand Up @@ -994,8 +994,20 @@ mod _sqlite3 {

#[pyclass(with(Constructor, Callable, Initializer), flags(BASETYPE, HAS_WEAKREF))]
impl Connection {
fn drop_db(&self) {
self.db.lock().take();
fn drop_db(&self, vm: &VirtualMachine) -> PyResult<()> {
let mut guard = self.db.lock();
let rollback_result = if let Some(db) = guard.as_ref()
&& *self.autocommit.lock() == AutocommitMode::Disabled
&& !db.is_autocommit()
{
db._exec(b"ROLLBACK\0", vm)
} else {
Ok(())
};
let db = guard.take();
drop(guard);
drop(db);
rollback_result
}

fn reset_factories(&self, vm: &VirtualMachine) {
Expand All @@ -1012,6 +1024,9 @@ mod _sqlite3 {
if let Some(isolation_level) = &args.isolation_level.0 {
begin_statement_ptr_from_isolation_level(isolation_level, vm)?;
}
if args.autocommit == AutocommitMode::Disabled {
db._exec(b"BEGIN\0", vm)?;
}
Ok(db)
}

Expand Down Expand Up @@ -1108,8 +1123,7 @@ mod _sqlite3 {
#[pymethod]
fn close(&self, vm: &VirtualMachine) -> PyResult<()> {
self.check_thread(vm)?;
self.drop_db();
Ok(())
self.drop_db(vm)
}

fn is_closed(&self) -> bool {
Expand All @@ -1118,16 +1132,35 @@ mod _sqlite3 {

#[pymethod]
fn commit(&self, vm: &VirtualMachine) -> PyResult<()> {
self.db_lock(vm)?.implicit_commit(vm)
let db = self.db_lock(vm)?;
let mode = *self.autocommit.lock();
match mode {
AutocommitMode::Legacy => db.implicit_commit(vm),
AutocommitMode::Enabled => Ok(()),
AutocommitMode::Disabled => {
db._exec(b"COMMIT\0", vm)?;
db._exec(b"BEGIN\0", vm)
}
}
}

#[pymethod]
fn rollback(&self, vm: &VirtualMachine) -> PyResult<()> {
let db = self.db_lock(vm)?;
if !db.is_autocommit() {
db._exec(b"ROLLBACK\0", vm)
} else {
Ok(())
let mode = *self.autocommit.lock();
match mode {
AutocommitMode::Legacy => {
if db.is_autocommit() {
Ok(())
} else {
db._exec(b"ROLLBACK\0", vm)
}
}
AutocommitMode::Enabled => Ok(()),
AutocommitMode::Disabled => {
db._exec(b"ROLLBACK\0", vm)?;
db._exec(b"BEGIN\0", vm)
}
}
}

Expand Down Expand Up @@ -1521,11 +1554,7 @@ mod _sqlite3 {

// If setting isolation_level to None (auto-commit mode), commit any pending transaction
if value.is_none() {
let db = self.db_lock(vm)?;
if !db.is_autocommit() {
// Keep the lock and call implicit_commit directly to avoid race conditions
db.implicit_commit(vm)?;
}
self.commit(vm)?;
}
let _ = unsafe { self.isolation_level.swap(value) };
Ok(())
Expand All @@ -1549,6 +1578,7 @@ mod _sqlite3 {
fn set_autocommit(&self, val: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
let mode = AutocommitMode::try_from_borrowed_object(vm, &val)?;
let db = self.db_lock(vm)?;
*self.autocommit.lock() = mode;

Comment on lines 1578 to 1582

@teddygood teddygood Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The literal NUL bytes were already present, but I think using the escaped \0 form is clearer and more consistent with the rest of the file. The second point is intentional. This implementation follows CPython's set_autocommit(), which stores the requested autocommit mode before executing COMMIT or BEGIN. If the SQL statement fails, CPython also retains the requested mode. Since this PR aims to match CPython's behavior, I will keep the current ordering.

// Handle transaction state based on mode change
match mode {
Expand All @@ -1568,9 +1598,6 @@ mod _sqlite3 {
// Legacy mode doesn't change transaction state
}
}

drop(db);
*self.autocommit.lock() = mode;
Ok(())
}

Expand Down
Loading