Merge pull request #138 from smol-machines/binbin-fix-db-lock-retry

fix: db retries since all req during hold fails currently.

Author: BinSquare

src/db.rs

@@ -13,6 +13,32 @@ use redb::{Database, ReadableTable, TableDefinition, TableError};
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
+use std::time::Duration;
+
+/// Maximum number of attempts to open the database when another process holds
+/// the lock. Each concurrent `smolvm` CLI invocation (e.g., parallel
+/// `machine start` calls) opens the database exclusively via redb's file lock.
+/// Without retry, the second process fails immediately with
+/// "Database already open. Cannot acquire lock."
+///
+/// With 10 retries and exponential backoff (50ms initial, 1s cap), the total
+/// wait before giving up is ~5 seconds — enough for any normal CLI operation
+/// to release the lock.
+const DB_OPEN_MAX_RETRIES: u32 = 10;
+
+/// Initial backoff delay between database open retries.
+/// Starts short (10ms) since typical DB operations complete in ~1-2ms.
+/// Doubles on each attempt: 10ms → 20ms → 40ms → 80ms → 160ms → 320ms → 640ms → 1000ms (capped).
+const DB_OPEN_INITIAL_BACKOFF: Duration = Duration::from_millis(10);
+
+/// Maximum backoff delay between retries. Prevents excessive wait on any
+/// single retry when the backoff would otherwise grow beyond this.
+const DB_OPEN_MAX_BACKOFF: Duration = Duration::from_secs(1);
+
+/// Check if a redb error indicates another process holds the database lock.
+fn is_lock_contention(e: &redb::DatabaseError) -> bool {
+    matches!(e, redb::DatabaseError::DatabaseAlreadyOpen)
+}
 
 /// Table for storing VM records (name -> JSON-serialized VmRecord).
 const VMS_TABLE: TableDefinition<&str, &[u8]> = TableDefinition::new("vms");
@@ -56,17 +82,53 @@ impl std::fmt::Debug for SmolvmDb {
 
 impl SmolvmDb {
     /// Run a closure with the cached database handle, opening it on first use.
+    ///
+    /// If another process holds the database lock, retries with exponential
+    /// backoff rather than failing immediately. This allows concurrent CLI
+    /// commands (e.g., parallel `machine start` calls) to succeed.
     fn with_db<T, F>(&self, f: F) -> Result<T>
     where
         F: FnOnce(&Database) -> Result<T>,
     {
         let mut guard = self.handle.lock();
         if guard.is_none() {
-            *guard = Some(Database::create(&self.path).db_err("open database")?);
+            *guard = Some(Self::open_with_retry(&self.path)?);
         }
         f(guard.as_ref().unwrap())
     }
 
+    /// Open the database file, retrying with exponential backoff on lock contention.
+    ///
+    /// redb uses an exclusive file lock — only one process can have the database
+    /// open at a time. When multiple CLI commands run concurrently (e.g., parallel
+    /// `machine start` calls), the second process retries until the first releases
+    /// the lock. The API server avoids this entirely by holding a single long-lived
+    /// database connection.
+    fn open_with_retry(path: &Path) -> Result<Database> {
+        let mut backoff = DB_OPEN_INITIAL_BACKOFF;
+        for attempt in 0..=DB_OPEN_MAX_RETRIES {
+            match Database::create(path) {
+                Ok(db) => return Ok(db),
+                Err(e) if attempt < DB_OPEN_MAX_RETRIES && is_lock_contention(&e) => {
+                    tracing::debug!(
+                        attempt = attempt + 1,
+                        max = DB_OPEN_MAX_RETRIES,
+                        backoff_ms = backoff.as_millis(),
+                        "database locked by another process, retrying"
+                    );
+                    std::thread::sleep(backoff);
+                    backoff = std::cmp::min(backoff * 2, DB_OPEN_MAX_BACKOFF);
+                }
+                Err(e) => {
+                    return Err(Error::database_unavailable(format!("open database: {}", e)));
+                }
+            }
+        }
+        // All retries exhausted — the loop always returns on the last iteration
+        // (attempt == DB_OPEN_MAX_RETRIES falls through to the Err arm).
+        unreachable!()
+    }
+
     /// Open the database at the default location.
     ///
     /// Default path: `~/Library/Application Support/smolvm/server/smolvm.redb` (macOS)

tests/test_machine.sh

@@ -1086,6 +1086,42 @@ test_port_conflict_across_vms() {
 
 run_test "Port: cross-VM conflict detected" test_port_conflict_across_vms || true
 
+# Regression: concurrent machine starts used to fail with "Database already
+# open. Cannot acquire lock." The fix retries with exponential backoff.
+test_concurrent_machine_start() {
+    local vm_a="conc-start-a-$$"
+    local vm_b="conc-start-b-$$"
+
+    $SMOLVM machine create "$vm_a" --cpus 1 --mem 256 2>&1 >/dev/null || return 1
+    $SMOLVM machine create "$vm_b" --cpus 1 --mem 256 2>&1 >/dev/null || return 1
+
+    # Start both simultaneously — previously the second would fail with DB lock error
+    $SMOLVM machine start --name "$vm_a" 2>&1 >/dev/null &
+    local pid_a=$!
+    $SMOLVM machine start --name "$vm_b" 2>&1 >/dev/null &
+    local pid_b=$!
+    wait $pid_a; local exit_a=$?
+    wait $pid_b; local exit_b=$?
+
+    # Both should succeed
+    [[ $exit_a -eq 0 ]] || { echo "FAIL: start a failed (exit $exit_a)"; }
+    [[ $exit_b -eq 0 ]] || { echo "FAIL: start b failed (exit $exit_b)"; }
+
+    # Both should be running
+    local status_a status_b
+    status_a=$($SMOLVM machine status --name "$vm_a" 2>&1)
+    status_b=$($SMOLVM machine status --name "$vm_b" 2>&1)
+
+    $SMOLVM machine stop --name "$vm_a" 2>/dev/null || true
+    $SMOLVM machine stop --name "$vm_b" 2>/dev/null || true
+    $SMOLVM machine delete "$vm_a" -f 2>/dev/null || true
+    $SMOLVM machine delete "$vm_b" -f 2>/dev/null || true
+
+    [[ "$status_a" == *"running"* ]] && [[ "$status_b" == *"running"* ]]
+}
+
+run_test "Concurrent machine starts" test_concurrent_machine_start || true
+
 echo ""
 echo "--- Machine Run (Ephemeral) Tests ---"
 echo ""