Skip to content

Fix library to include multiple SQL command support #9

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
merged 9 commits into from
Sep 19, 2025
Merged

Fix library to include multiple SQL command support #9

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7146802
refactor library to have multiple sql command support
findolor Aug 29, 2025
a26ceca
fix multi logic
findolor Sep 15, 2025
dc84fbd
add ui tests
findolor Sep 15, 2025
7dd9d81
apply ai suggestions
findolor Sep 15, 2025
e9b9c80
Merge branch 'main' into multiple-sql-commands
findolor Sep 18, 2025
b9a9527
Merge branch 'main' into multiple-sql-commands
findolor Sep 19, 2025
232661f
update
findolor Sep 19, 2025
ba3db29
Merge branch 'main' into multiple-sql-commands
findolor Sep 19, 2025
0e13611
update
findolor Sep 19, 2025
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
36 changes: 24 additions & 12 deletions packages/sqlite-web-core/src/coordination.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::messages::{ChannelMessage, PendingQuery};
pub struct WorkerState {
pub worker_id: String,
pub is_leader: Rc<RefCell<bool>>,
pub db: Rc<RefCell<Option<Rc<SQLiteDatabase>>>>,
pub db: Rc<RefCell<Option<SQLiteDatabase>>>,
pub channel: BroadcastChannel,
pub pending_queries: Rc<RefCell<HashMap<String, PendingQuery>>>,
}
Expand Down Expand Up @@ -50,11 +50,18 @@ impl WorkerState {
let channel = channel.clone();

spawn_local(async move {
let database = db.borrow().clone();
let result = if let Some(database) = database {
database.exec(&sql).await
} else {
Err("Database not initialized".to_string())
let result = {
// Extract database temporarily to avoid holding RefCell across await
let db_opt = db.borrow_mut().take();
match db_opt {
Some(mut database) => {
let result = database.exec(&sql).await;
// Put the database back
*db.borrow_mut() = Some(database);
result
}
None => Err("Database not initialized".to_string()),
}
};
Comment on lines 77 to 90
Copy link

@coderabbitai coderabbitai bot Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure DB reinsertion even on early returns/panics (RAII guard).

Wrap the taken DB in a small guard that puts it back in Drop to avoid leaving it None if future aborts.

struct DbGuard<'a> {
    cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>,
    db: Option<SQLiteDatabase>,
}
impl<'a> DbGuard<'a> {
    fn take(cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>) -> Result<Self, String> {
        let db = cell.borrow_mut().take().ok_or("Database not initialized".to_string())?;
        Ok(Self { cell, db: Some(db) })
    }
    fn db_mut(&mut self) -> &mut SQLiteDatabase { self.db.as_mut().unwrap() }
}
impl<'a> Drop for DbGuard<'a> {
    fn drop(&mut self) {
        if let Some(db) = self.db.take() {
            *self.cell.borrow_mut() = Some(db);
        }
    }
}

Use inside the critical sections instead of manual take/put.

Also applies to: 170-182

🤖 Prompt for AI Agents 
In packages/sqlite-web-core/src/coordination.rs around lines 52-65 (and
similarly 170-182), the code takes the SQLiteDatabase out of the
Rc<RefCell<Option<...>>> and manually reinserts it after awaiting, which can
leave the cell None if the future aborts or panics; implement a small RAII
DbGuard type that takes the Option out on construction, exposes a mutable
reference to the inner SQLiteDatabase, and always reinserts the database into
the cell in its Drop implementation, then replace the manual take/put logic with
creating a DbGuard, calling guard.db_mut().exec(&sql).await (or other
operations) so the DB is restored even on early returns or panics.

⚠️ Potential issue

Race: concurrent leader queries can see "Database not initialized".

While one task takes() the DB, a second concurrent task will read None and fail spuriously. Serialize access with an async mutex to avoid false errors.

Apply this diff (uses async-lock; add dependency in Cargo.toml: async-lock = "3"):

@@
-use crate::database::SQLiteDatabase;
+use crate::database::SQLiteDatabase;
+use async_lock::Mutex;
@@
 pub struct WorkerState {
@@
-    pub db: Rc<RefCell<Option<SQLiteDatabase>>>,
+    pub db: Rc<RefCell<Option<SQLiteDatabase>>>,
+    pub exec_lock: Rc<Mutex<()>>,
@@
     pub fn new() -> Result<Self, JsValue> {
@@
         Ok(WorkerState {
@@
             db: Rc::new(RefCell::new(None)),
+            exec_lock: Rc::new(Mutex::new(())),
@@
     pub fn setup_channel_listener(&self) {
@@
-        let pending_queries = Rc::clone(&self.pending_queries);
+        let pending_queries = Rc::clone(&self.pending_queries);
+        let exec_lock = Rc::clone(&self.exec_lock);
@@
                             spawn_local(async move {
-                                let result = {
+                                let _guard = exec_lock.lock().await;
+                                let result = {
                                     // Extract database temporarily to avoid holding RefCell across await
                                     let db_opt = db.borrow_mut().take();
                                     match db_opt {
                                         Some(mut database) => {
                                             let result = database.exec(&sql).await;
                                             // Put the database back
                                             *db.borrow_mut() = Some(database);
                                             result
                                         }
                                         None => Err("Database not initialized".to_string()),
                                     }
                                 };
@@
     pub async fn execute_query(&self, sql: String) -> Result<String, String> {
         if *self.is_leader.borrow() {
-            // Extract database temporarily to avoid holding RefCell across await
-            let db_opt = self.db.borrow_mut().take();
+            let _guard = self.exec_lock.lock().await;
+            // Extract database temporarily to avoid holding RefCell across await
+            let db_opt = self.db.borrow_mut().take();
             match db_opt {
                 Some(mut database) => {
                     let result = database.exec(&sql).await;
                     // Put the database back
-                    *self.db.borrow_mut() = Some(database);
+                    *self.db.borrow_mut() = Some(database);
                     result
                 }
                 None => Err("Database not initialized".to_string()),
             }

Also applies to: 170-182

🤖 Prompt for AI Agents 
In packages/sqlite-web-core/src/coordination.rs around lines 52-65 (and
similarly 170-182), concurrent tasks call borrow_mut().take() on the
RefCell-wrapped database which causes spurious "Database not initialized" errors
when another task has temporarily taken it; replace the RefCell approach with an
async-aware mutex: add async-lock = "3" to Cargo.toml, change the shared DB
container to an Arc<async_lock::Mutex<Option<YourDatabaseType>>> (or equivalent
async mutex), and in the spawned async tasks await a lock,
check/take/use/put-back the Option while holding the mutex (releasing the lock
after the awaited DB operation completes or, better, clone a handle if
available), ensuring all accesses are serialized to prevent the race.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't have this error. we can safely use the db connection.

Copy link

Choose a reason for hiding this comment

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

@findolor, understood! Thanks for clarifying that this race condition doesn't occur in your implementation. I appreciate you sharing your experience with the database access pattern.

✏️ Learnings added 
Learnt from: findolor
PR: rainlanguage/sqlite-web#9
File: packages/sqlite-web-core/src/coordination.rs:52-65
Timestamp: 2025-09-15T06:11:31.713Z
Learning: In packages/sqlite-web-core/src/coordination.rs, the pattern of using `db.borrow_mut().take()` followed by async database operations and then putting the database back with `*db.borrow_mut() = Some(database)` is safe in their use case and doesn't cause concurrent access issues or "Database not initialized" errors.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


let response = match result {
Expand Down Expand Up @@ -131,7 +138,7 @@ impl WorkerState {
spawn_local(async move {
match SQLiteDatabase::initialize_opfs().await {
Ok(database) => {
*db.borrow_mut() = Some(Rc::new(database));
*db.borrow_mut() = Some(database);

let msg = ChannelMessage::NewLeader {
leader_id: worker_id.clone(),
Expand Down Expand Up @@ -162,11 +169,16 @@ impl WorkerState {

pub async fn execute_query(&self, sql: String) -> Result<String, String> {
if *self.is_leader.borrow() {
let database = self.db.borrow().clone();
if let Some(database) = database {
database.exec(&sql).await
} else {
Err("Database not initialized".to_string())
// Extract database temporarily to avoid holding RefCell across await
let db_opt = self.db.borrow_mut().take();
match db_opt {
Some(mut database) => {
let result = database.exec(&sql).await;
// Put the database back
*self.db.borrow_mut() = Some(database);
result
}
None => Err("Database not initialized".to_string()),
}
} else {
let query_id = Uuid::new_v4().to_string();
Expand Down
Loading