From b2a5a7077c2ae0c46b67fc9ca7c82ec32a9762bc Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 10:42:26 +0300 Subject: [PATCH 1/7] sqlite3: Fix WAL tests by closing connection Commit ac33ae90 ("core: Enforce single, shared database object per database file") changes the semantics of the WAL because unless we close all the connections, the WAL remains open due to `Database` being kept open in memory. Fix test failures by properly closing the connection between different test cases. --- sqlite3/tests/compat/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sqlite3/tests/compat/mod.rs b/sqlite3/tests/compat/mod.rs index a6aba67c5..9b1b1b56d 100644 --- a/sqlite3/tests/compat/mod.rs +++ b/sqlite3/tests/compat/mod.rs @@ -355,6 +355,7 @@ mod tests { ), SQLITE_OK ); + assert_eq!(sqlite3_close(db), SQLITE_OK); } let mut wal_path = temp_file.path().to_path_buf(); assert!(wal_path.set_extension("db-wal")); @@ -380,6 +381,7 @@ mod tests { assert_eq!(sqlite3_step(stmt), SQLITE_DONE); assert_eq!(sqlite3_finalize(stmt), SQLITE_OK); } + assert_eq!(sqlite3_close(db), SQLITE_OK); } } @@ -459,6 +461,7 @@ mod tests { ), SQLITE_OK ); + assert_eq!(sqlite3_close(db), SQLITE_OK); } let mut wal_path = temp_file.path().to_path_buf(); assert!(wal_path.set_extension("db-wal")); @@ -483,6 +486,7 @@ mod tests { assert_eq!(sqlite3_step(stmt), SQLITE_DONE); assert_eq!(sqlite3_finalize(stmt), SQLITE_OK); } + assert_eq!(sqlite3_close(db), SQLITE_OK); } } From 551df4ce0dea6526e9a6965aabeccd693fa4444e Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 11:30:07 +0300 Subject: [PATCH 2/7] bindings/rust: Fix test_database_persistence_many_frames() We need to make sure database connection is closed to avoid reusing he same `Database` object. --- bindings/rust/src/lib.rs | 54 +++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/bindings/rust/src/lib.rs b/bindings/rust/src/lib.rs index a3ef69b96..f8da13081 100644 --- a/bindings/rust/src/lib.rs +++ b/bindings/rust/src/lib.rs @@ -552,37 +552,39 @@ mod tests { } } // db and conn are dropped here, simulating closing - // Now, re-open the database and check if the data is still there - let db = Builder::new_local(db_path).build().await?; - let conn = db.connect()?; + { + // Now, re-open the database and check if the data is still there + let db = Builder::new_local(db_path).build().await?; + let conn = db.connect()?; - let mut rows = conn - .query("SELECT data FROM test_large_persistence ORDER BY id;", ()) - .await?; + let mut rows = conn + .query("SELECT data FROM test_large_persistence ORDER BY id;", ()) + .await?; - for (i, value) in original_data.iter().enumerate().take(NUM_INSERTS) { - let row = rows - .next() - .await? - .unwrap_or_else(|| panic!("Expected row {i} but found None")); - assert_eq!( - row.get_value(0)?, - Value::Text(value.clone()), - "Mismatch in retrieved data for row {i}" + for (i, value) in original_data.iter().enumerate().take(NUM_INSERTS) { + let row = rows + .next() + .await? + .unwrap_or_else(|| panic!("Expected row {i} but found None")); + assert_eq!( + row.get_value(0)?, + Value::Text(value.clone()), + "Mismatch in retrieved data for row {i}" + ); + } + + assert!( + rows.next().await?.is_none(), + "Expected no more rows after retrieving all inserted data" ); + + // Delete the WAL file only and try to re-open and query + let wal_path = format!("{db_path}-wal"); + std::fs::remove_file(&wal_path) + .map_err(|e| eprintln!("Warning: Failed to delete WAL file for test: {e}")) + .unwrap(); } - assert!( - rows.next().await?.is_none(), - "Expected no more rows after retrieving all inserted data" - ); - - // Delete the WAL file only and try to re-open and query - let wal_path = format!("{db_path}-wal"); - std::fs::remove_file(&wal_path) - .map_err(|e| eprintln!("Warning: Failed to delete WAL file for test: {e}")) - .unwrap(); - // Attempt to re-open the database after deleting WAL and assert that table is missing. let db_after_wal_delete = Builder::new_local(db_path).build().await?; let conn_after_wal_delete = db_after_wal_delete.connect()?; From edbbeefed5e97901b02689942dd1688427b3b4fe Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 12:20:02 +0300 Subject: [PATCH 3/7] bindings/javascript: Fix Database.close() We need to drop reference to `turso_core::Database` for this to work. --- bindings/javascript/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bindings/javascript/src/lib.rs b/bindings/javascript/src/lib.rs index 1616d6b87..5363ab9ff 100644 --- a/bindings/javascript/src/lib.rs +++ b/bindings/javascript/src/lib.rs @@ -65,7 +65,7 @@ pub struct Database { pub open: bool, #[napi(writable = false)] pub name: String, - _db: Arc, + db: Option>, conn: Arc, _io: Arc, } @@ -108,7 +108,7 @@ impl Database { Ok(Self { readonly: opts.readonly(), memory, - _db: db, + db: Some(db), conn, open: true, name: path, @@ -237,6 +237,7 @@ impl Database { pub fn close(&mut self) -> napi::Result<()> { if self.open { self.conn.close().map_err(into_napi_error)?; + self.db.take(); self.open = false; } Ok(()) From 5b6a30c1dfde430074d6e91baaa605d1cd7a95c5 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 11:57:12 +0300 Subject: [PATCH 4/7] core/storage: Fix B-Tree test cases to use ":memory:" ...otherwise they all share the same `Database` object. --- core/storage/btree.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 80fc27e56..9ca83bfe7 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -7203,7 +7203,7 @@ mod tests { fn empty_btree() -> (Rc, usize, Arc, Arc) { #[allow(clippy::arc_with_non_send_sync)] let io: Arc = Arc::new(MemoryIO::new()); - let db = Database::open_file(io.clone(), "test.db", false, false).unwrap(); + let db = Database::open_file(io.clone(), ":memory:", false, false).unwrap(); let conn = db.connect().unwrap(); let pager = conn.pager.borrow().clone(); @@ -8129,7 +8129,7 @@ mod tests { let io: Arc = Arc::new(MemoryIO::new()); let db_file = Arc::new(DatabaseFile::new( - io.open_file("test.db", OpenFlags::Create, false).unwrap(), + io.open_file(":memory:", OpenFlags::Create, false).unwrap(), )); let wal_file = io.open_file("test.wal", OpenFlags::Create, false).unwrap(); From 9b67eb0e77e848edd342f6007c77faec9bf89c9e Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 12:49:28 +0300 Subject: [PATCH 5/7] core: Fix transaction cleanup in Connection::close() --- core/lib.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/lib.rs b/core/lib.rs index d1e17b2f3..d222f8982 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -1114,6 +1114,26 @@ impl Connection { return Ok(()); } self.closed.set(true); + + match self.transaction_state.get() { + TransactionState::Write { schema_did_change } => { + let _result = self.pager.borrow().end_tx( + true, // rollback = true for close + schema_did_change, + self, + self.wal_checkpoint_disabled.get(), + ); + self.transaction_state.set(TransactionState::None); + } + TransactionState::Read => { + let _result = self.pager.borrow().end_read_tx(); + self.transaction_state.set(TransactionState::None); + } + TransactionState::None => { + // No active transaction + } + } + self.pager .borrow() .checkpoint_shutdown(self.wal_checkpoint_disabled.get()) From ab1a15210078fff27502b07500cd0e28ad2a69f2 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 10:09:20 +0300 Subject: [PATCH 6/7] core: Enforce single shared database object per database file We need to ensures that there is a single, shared `Database` object per a database file. We need because it is not safe to have multiple independent WAL files open because coordination happens at process-level POSIX file advisory locks. Fixes #2267 Co-authored-by: ultraman --- core/lib.rs | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/core/lib.rs b/core/lib.rs index d222f8982..72f11738f 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -72,7 +72,7 @@ use std::{ num::NonZero, ops::Deref, rc::Rc, - sync::{Arc, Mutex}, + sync::{Arc, LazyLock, Mutex, Weak}, }; #[cfg(feature = "fs")] use storage::database::DatabaseFile; @@ -108,6 +108,15 @@ pub(crate) type MvStore = mvcc::MvStore; pub(crate) type MvCursor = mvcc::cursor::ScanCursor; +/// The database manager ensures that there is a single, shared +/// `Database` object per a database file. We need because it is not safe +/// to have multiple independent WAL files open because coordination +/// happens at process-level POSIX file advisory locks. +static DATABASE_MANAGER: LazyLock>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// The `Database` object contains per database file state that is shared +/// between multiple connections. pub struct Database { mv_store: Option>, schema: Mutex>, @@ -224,6 +233,36 @@ impl Database { flags: OpenFlags, enable_mvcc: bool, enable_indexes: bool, + ) -> Result> { + if path == ":memory:" { + return Self::do_open_with_flags(io, path, db_file, flags, enable_mvcc, enable_indexes); + } + + let mut registry = DATABASE_MANAGER.lock().unwrap(); + + let canonical_path = std::fs::canonicalize(path) + .ok() + .and_then(|p| p.to_str().map(|s| s.to_string())) + .unwrap_or_else(|| path.to_string()); + + if let Some(db) = registry.get(&canonical_path) { + if let Some(db) = db.upgrade() { + return Ok(db); + } + } + let db = Self::do_open_with_flags(io, path, db_file, flags, enable_mvcc, enable_indexes)?; + registry.insert(canonical_path, Arc::downgrade(&db)); + Ok(db) + } + + #[allow(clippy::arc_with_non_send_sync)] + fn do_open_with_flags( + io: Arc, + path: &str, + db_file: Arc, + flags: OpenFlags, + enable_mvcc: bool, + enable_indexes: bool, ) -> Result> { let wal_path = format!("{path}-wal"); let maybe_shared_wal = WalFileShared::open_shared_if_exists(&io, wal_path.as_str())?; From 50e03ee90e97384af9773b86931dfa10ac44977b Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Mon, 28 Jul 2025 19:16:01 +0300 Subject: [PATCH 7/7] core: Clean up Connection::open_with_flags() Co-authored-by: bit-aloo <84662239+Shourya742@users.noreply.github.com> --- core/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/lib.rs b/core/lib.rs index 72f11738f..1c4c56557 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -245,10 +245,8 @@ impl Database { .and_then(|p| p.to_str().map(|s| s.to_string())) .unwrap_or_else(|| path.to_string()); - if let Some(db) = registry.get(&canonical_path) { - if let Some(db) = db.upgrade() { - return Ok(db); - } + if let Some(db) = registry.get(&canonical_path).and_then(Weak::upgrade) { + return Ok(db); } let db = Self::do_open_with_flags(io, path, db_file, flags, enable_mvcc, enable_indexes)?; registry.insert(canonical_path, Arc::downgrade(&db));