Bootstrap database before opening connection (#2021)

This patch is a follow up on
#2017 It changes the
heuristics for bootstrapping the DB.

One more change is, it also does bootstrap before any connection to the
local SQLite is created

Earlier, we bootstrapped whenever we noticed a change in the generation.
However, this patch changes this behaviour and does bootstrap only if
local files don't exist:

1. if no db file or the metadata file exists, then user is starting from
scratch
and we will do the sync
2. if the db file exists, but the metadata file does not exist (or other
way around),
then local db is in an incorrect state. we stop and return with an error
3. if the db file exists and the metadata file exists, then we don't
need to do the
sync

Author: penberg

libsql/src/local/database.rs

@@ -465,8 +465,8 @@ impl Database {
     /// Sync WAL frames to remote.
     pub async fn sync_offline(&self) -> Result<crate::database::Replicated> {
         let mut sync_ctx = self.sync_ctx.as_ref().unwrap().lock().await;
+        crate::sync::bootstrap_db(&mut sync_ctx).await?;
         let conn = self.connect()?;
-
         crate::sync::sync_offline(&mut sync_ctx, &conn).await
     }
 

libsql/src/sync.rs

@@ -64,6 +64,8 @@ pub enum SyncError {
     PullDb(StatusCode, String),
     #[error("server returned a lower generation than local: local={0}, remote={1}")]
     InvalidLocalGeneration(u32, u32),
+    #[error("invalid local state: {0}")]
+    InvalidLocalState(String),
 }
 
 impl SyncError {
@@ -509,27 +511,63 @@ impl SyncContext {
     }
 
     async fn sync_db_if_needed(&mut self, generation: u32) -> Result<()> {
-        // we will get the export file only if the remote generation is different from the one we have
-        if generation == self.durable_generation {
-            return Ok(());
-        }
         // somehow we are ahead of the remote in generations. following should not happen because
         // we checkpoint only if the remote server tells us to do so.
         if self.durable_generation > generation {
             tracing::error!(
-                "server returned a lower generation than what we have: sent={}, got={}",
+                "server returned a lower generation than what we have: local={}, remote={}",
                 self.durable_generation,
                 generation
             );
             return Err(
                 SyncError::InvalidLocalGeneration(self.durable_generation, generation).into(),
             );
         }
-        tracing::debug!(
-            "syncing db file from remote server, generation={}",
-            generation
-        );
-        self.sync_db(generation).await
+        // we use the following heuristic to determine if we need to sync the db file
+        // 1. if no db file or the metadata file exists, then user is starting from scratch
+        //    and we will do the sync
+        // 2. if the db file exists, but the metadata file does not exist (or other way around),
+        //    then local db is in an incorrect state. we stop and return with an error
+        // 3. if the db file exists and the metadata file exists, then we don't need to do the
+        //    sync
+        let metadata_exists = check_if_file_exists(&format!("{}-info", self.db_path))?;
+        let db_file_exists = check_if_file_exists(&self.db_path)?;
+        match (metadata_exists, db_file_exists) {
+            (false, false) => {
+                // neither the db file nor the metadata file exists, lets bootstrap from remote
+                tracing::debug!(
+                    "syncing db file from remote server, generation={}",
+                    generation
+                );
+                self.sync_db(generation).await
+            }
+            (false, true) => {
+                // kinda inconsistent state: DB exists but metadata missing
+                // however, this generally not an issue. For a fresh db, a user might do writes
+                // locally and then try to do sync later. So in this case, we will not
+                // bootstrap the db file and let the user proceed. If it is not a fresh db, the
+                // push will fail anyways later.
+                // if metadata file does not exist, then generation should be zero
+                assert_eq!(self.durable_generation, 0);
+                // lets initialise it to first generation
+                self.durable_generation = 1;
+                Ok(())
+            }
+            (true, false) => {
+                // inconsistent state: Metadata exists but DB missing
+                tracing::error!(
+                    "local state is incorrect, metadata file exists but db file does not"
+                );
+                Err(SyncError::InvalidLocalState(
+                    "metadata file exists but db file does not".to_string(),
+                )
+                .into())
+            }
+            (true, true) => {
+                // both files exists, no need to sync
+                Ok(())
+            }
+        }
     }
 
     /// sync_db will download the db file from the remote server and replace the local file.
@@ -646,6 +684,28 @@ async fn atomic_write<P: AsRef<Path>>(path: P, data: &[u8]) -> Result<()> {
     Ok(())
 }
 
+/// bootstrap_db brings the .db file from remote, if required. If the .db file already exists, then
+/// it does nothing. Calling this function multiple times is safe.
+pub async fn bootstrap_db(sync_ctx: &mut SyncContext) -> Result<()> {
+    // todo: we are checking with the remote server only during initialisation. ideally,
+    // we need to do this when we notice a large gap in generations, when bootstrapping is cheaper
+    // than pulling each frame
+    if !sync_ctx.initial_server_sync {
+        // sync is being called first time. so we will call remote, get the generation information
+        // if we are lagging behind, then we will call the export API and get to the latest
+        // generation directly.
+        let info = sync_ctx.get_remote_info().await?;
+        sync_ctx
+            .sync_db_if_needed(info.current_generation)
+            .await?;
+        // when sync_ctx is initialised, we set durable_generation to 0. however, once
+        // sync_db is called, it should be > 0.
+        assert!(sync_ctx.durable_generation > 0, "generation should be > 0");
+        sync_ctx.initial_server_sync = true;
+    }
+    Ok(())
+}
+
 /// Sync WAL frames to remote.
 pub async fn sync_offline(
     sync_ctx: &mut SyncContext,
@@ -667,22 +727,6 @@ pub async fn sync_offline(
             Err(e) => Err(e),
         }
     } else {
-        // todo: we are checking with the remote server only during initialisation. ideally,
-        // we should check everytime we try to sync with the remote server. However, we need to close
-        // all the ongoing connections since we replace `.db` file and remove the `.db-wal` file
-        if !sync_ctx.initial_server_sync {
-            // sync is being called first time. so we will call remote, get the generation information
-            // if we are lagging behind, then we will call the export API and get to the latest
-            // generation directly.
-            let info = sync_ctx.get_remote_info().await?;
-            sync_ctx
-                .sync_db_if_needed(info.current_generation)
-                .await?;
-            // when sync_ctx is initialised, we set durable_generation to 0. however, once
-            // sync_db is called, it should be > 0.
-            assert!(sync_ctx.durable_generation > 0, "generation should be > 0");
-            sync_ctx.initial_server_sync = true;
-        }
         try_pull(sync_ctx, conn).await
     }
     .or_else(|err| {
@@ -831,3 +875,9 @@ async fn try_pull(
         })
     }
 }
+
+fn check_if_file_exists(path: &str) -> core::result::Result<bool, SyncError> {
+    Path::new(&path)
+        .try_exists()
+        .map_err(SyncError::io("metadata file exists"))
+}