Fix bug with Lockfile sticking around

crates/cli/src/config.rs

@@ -3,7 +3,7 @@ use anyhow::Context;
 use jsonwebtoken::DecodingKey;
 use serde::{Deserialize, Serialize};
 use spacetimedb::auth::identity::decode_token;
-use spacetimedb_fs_utils::{create_parent_dir, lockfile::Lockfile};
+use spacetimedb_fs_utils::{atomic_write, create_parent_dir};
 use spacetimedb_lib::Identity;
 use std::{
     fs,
@@ -106,23 +106,10 @@ pub struct RawConfig {
     server_configs: Vec<ServerConfig>,
 }
 
+#[derive(Debug, Clone)]
 pub struct Config {
     proj: RawConfig,
     home: RawConfig,
-
-    /// The path from which we loaded the system config `home`,
-    /// and a [`Lockfile`] which guarantees us exclusive access to it.
-    ///
-    /// The lockfile is created before reading or creating the home config,
-    /// and closed upon dropping the config.
-    /// This allows us to mutate the config via [`Config::save`] without worrying about other processes.
-    ///
-    /// None only in tests, which are allowed to concurrently mutate configurations as much as they want,
-    /// since they use a hardcoded configuration rather than loading from a file.
-    ///
-    /// Note that it's not necessary to lock or remember the path of the project config,
-    /// since we never write to that file.
-    home_file: Option<(PathBuf, Lockfile)>,
 }
 
 const HOME_CONFIG_DIR: &str = ".spacetime";
@@ -595,20 +582,6 @@ Fetch the server's fingerprint with:
 }
 
 impl Config {
-    /// Release the system configuration `Lockfile` contained in `self`, if it exists,
-    /// allowing other processes to access the configuration.
-    ///
-    /// This is equivalent to dropping `self`, but more explicit in purpose.
-    pub fn release_lock(self) {
-        // Just drop `self`, cleaning up its lockfile.
-        //
-        // If we had CLI subcommands which wanted to read a `Config` but never `Config::save` it,
-        // we could have this message take `&mut self` and set the `home_lock` to `None`.
-        // This seems unlikely to be useful,
-        // as many accesses to the `Config` will implicitly mutate and `save`,
-        // e.g. by creating a new identity if none exists.
-    }
-
     pub fn default_server_name(&self) -> Option<&str> {
         self.proj
             .default_server
@@ -850,7 +823,6 @@ impl Config {
 
     pub fn load() -> Self {
         let home_path = Self::system_config_path();
-        let home_lock = Lockfile::for_file(&home_path).unwrap();
         let home = if home_path.exists() {
             Self::load_from_file(&home_path)
                 .inspect_err(|e| eprintln!("config file {home_path:?} is invalid: {e:#?}"))
@@ -861,50 +833,26 @@ impl Config {
 
         let proj = Self::load_proj_config();
 
-        Self {
-            home,
-            proj,
-
-            home_file: Some((home_path, home_lock)),
-        }
+        Self { home, proj }
     }
 
     #[doc(hidden)]
-    /// Used in tests; not backed by a file.
+    /// Used in tests.
     pub fn new_with_localhost() -> Self {
         Self {
             home: RawConfig::new_with_localhost(),
             proj: RawConfig::default(),
-
-            home_file: None,
-        }
-    }
-
-    #[doc(hidden)]
-    pub fn clone_for_test(&self) -> Self {
-        assert!(
-            self.home_file.is_none(),
-            "Cannot clone_for_test a Config derived from file {:?}",
-            self.home_file,
-        );
-        Config {
-            home: self.home.clone(),
-            proj: self.proj.clone(),
-            home_file: None,
         }
     }
 
     pub fn save(&self) {
-        let Some((home_path, _)) = &self.home_file else {
-            return;
-        };
-
+        let home_path = Self::system_config_path();
         // If the `home_path` is in a directory, ensure it exists.
-        create_parent_dir(home_path).unwrap();
+        create_parent_dir(home_path.as_ref()).unwrap();
 
         let config = toml::to_string_pretty(&self.home).unwrap();
 
-        if let Err(e) = std::fs::write(home_path, config) {
+        if let Err(e) = atomic_write(&home_path, config) {
             eprintln!("could not save config file: {e}")
         }
     }

crates/cli/src/lib.rs

@@ -59,11 +59,7 @@ pub async fn exec_subcommand(config: Config, cmd: &str, args: &ArgMatches) -> Re
         "build" => build::exec(config, args).await,
         "server" => server::exec(config, args).await,
         #[cfg(feature = "standalone")]
-        "start" => {
-            // Release the lockfile on the config, since we don't need it.
-            config.release_lock();
-            start::exec(args).await
-        }
+        "start" => start::exec(args).await,
         "upgrade" => upgrade::exec(config, args).await,
         unknown => Err(anyhow::anyhow!("Invalid subcommand: {}", unknown)),
     }

crates/cli/src/subcommands/build.rs

@@ -32,10 +32,7 @@ pub fn cli() -> clap::Command {
         )
 }
 
-pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     let project_path = args.get_one::<PathBuf>("project_path").unwrap();
     let skip_clippy = args.get_flag("skip_clippy");
     let build_debug = args.get_flag("debug");

crates/cli/src/subcommands/generate/mod.rs

@@ -100,10 +100,7 @@ pub fn cli() -> clap::Command {
         .after_help("Run `spacetime help publish` for more detailed information.")
 }
 
-pub fn exec(config: Config, args: &clap::ArgMatches) -> anyhow::Result<()> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+pub fn exec(_config: Config, args: &clap::ArgMatches) -> anyhow::Result<()> {
     let project_path = args.get_one::<PathBuf>("project_path").unwrap();
     let wasm_file = args.get_one::<PathBuf>("wasm_file").cloned();
     let out_dir = args.get_one::<PathBuf>("out_dir").unwrap();

crates/cli/src/subcommands/init.rs

@@ -112,10 +112,7 @@ fn check_for_git() -> bool {
     false
 }
 
-pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     let project_path = args.get_one::<PathBuf>("project-path").unwrap();
     let project_lang = *args.get_one::<ModuleLanguage>("lang").unwrap();
 

crates/cli/src/subcommands/local.rs

@@ -37,10 +37,7 @@ async fn exec_subcommand(config: Config, cmd: &str, args: &ArgMatches) -> Result
     }
 }
 
-async fn exec_clear(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+async fn exec_clear(_config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     let force = args.get_flag("force");
     if std::env::var_os("STDB_PATH").map(PathBuf::from).is_none() {
         let mut path = dirs::home_dir().unwrap_or_default();

crates/cli/src/subcommands/logs.rs

@@ -114,7 +114,6 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
     let query_parms = LogsParams { num_lines, follow };
 
     let host_url = config.get_host_url(server)?;
-    config.release_lock();
 
     let builder = reqwest::Client::new().get(format!("{}/database/logs/{}", host_url, address));
     let builder = add_auth_header_opt(builder, &auth_header);

crates/cli/src/subcommands/upgrade.rs

@@ -121,10 +121,7 @@ async fn download_with_progress(client: &reqwest::Client, url: &str, temp_path:
     Ok(())
 }
 
-pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     let version = args.get_one::<String>("version");
     let current_exe_path = env::current_exe()?;
     let force = args.get_flag("force");

crates/cli/src/subcommands/version.rs

@@ -17,10 +17,7 @@ pub fn cli() -> clap::Command {
         )
 }
 
-pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
-    // Release the lockfile on the config, since we don't need it.
-    config.release_lock();
-
+pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     if args.get_flag("cli") {
         println!("{}", CLI_VERSION);
         return Ok(());

crates/fs-utils/Cargo.toml

@@ -7,8 +7,10 @@ license-file = "LICENSE"
 description = "Assorted utilities for filesystem operations used in SpacetimeDB"
 
 [dependencies]
+anyhow.workspace = true
 thiserror.workspace = true
 hex.workspace = true
+tempfile.workspace = true
 
 [dev-dependencies]
 tempdir.workspace = true

crates/fs-utils/src/lib.rs

@@ -1,4 +1,5 @@
-use std::path::Path;
+use std::path::{Path, PathBuf};
+use tempfile::NamedTempFile;
 
 pub mod dir_trie;
 pub mod lockfile;
@@ -31,3 +32,12 @@ pub fn create_parent_dir(file: &Path) -> Result<(), std::io::Error> {
     // If `parent` already exists as a directory, this is a no-op.
     std::fs::create_dir_all(parent)
 }
+
+pub fn atomic_write(file_path: &PathBuf, data: String) -> anyhow::Result<()> {
+    let temp_file = NamedTempFile::new()?;
+    // Close the file, but keep the path to it around.
+    let temp_path = temp_file.into_temp_path();
+    std::fs::write(&temp_path, data)?;
+    std::fs::rename(&temp_path, file_path)?;
+    Ok(())
+}

crates/testing/src/lib.rs

@@ -34,6 +34,6 @@ pub fn invoke_cli(args: &[&str]) {
     let (cmd, args) = args.subcommand().expect("Could not split subcommand and args");
 
     RUNTIME
-        .block_on(spacetimedb_cli::exec_subcommand((*CONFIG).clone_for_test(), cmd, args))
+        .block_on(spacetimedb_cli::exec_subcommand((*CONFIG).clone(), cmd, args))
         .unwrap()
 }