Launcher: lock the secrets file across processes, hand it back to the keychain

Review fixes for the secrets file (#5095):

- Every read-modify-replace holds account-secrets.lock (std File::lock),
  so launcher and CLI processes signing in or out at once no longer
  overwrite each other; each write uses a temporary file of its own.
- The file no longer sticks once made: at the first start the keychain
  answers, its secrets move into the keychain and the file is removed,
  so an account kept in the keychain before a start it missed comes
  back beside the ones signed in meanwhile. If the keychain refuses any,
  the file stays whole and in use.
- A broken file's error names only the error kind, line and column:
  serde's own message can quote a string the file holds.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01DG1mYGWXNxQzmhMnjB36p1

docs/microsoft-account.md

@@ -138,10 +138,13 @@ one is selected. "Sign out" deletes the account's secrets from the keychain.
   account is selected. Never a token.
 - No keychain available (a Linux box without a Secret Service, a locked keychain, #5095): the
   secrets go to `account-secrets.json` in the launcher's data folder instead, readable and writable
-  by the player only (0600 on Unix) and replaced whole on every write, as most launchers keep them.
-  Once that file exists the launcher keeps using it, without asking the keychain, so sign-ins don't
-  come and go with a keychain that answers only some of the time; signing the last account out
-  removes it, and the keychain is asked again from the next start. `HORIZON_NO_KEYCHAIN` keeps
+  by the player only (0600 on Unix), as most launchers keep them. Every change holds
+  `account-secrets.lock` across its read and replace, so launcher and CLI processes signing in or out
+  at once lose nothing, and each write goes through a temporary file of its own. At the first start
+  the keychain answers again, the file's secrets move into it (they are the newer ones) and the file
+  is removed, so accounts signed in on a start the keychain missed sit beside the ones it always
+  held; if the keychain refuses any of them, the file stays whole and in use for that run. A broken
+  file is an error naming only its line and column, never what it holds. `HORIZON_NO_KEYCHAIN` keeps
   secrets in memory for the run instead (the launcher's own tests), and the launcher then says "No
   keychain available: you are signed in until the launcher closes".
 - Tokens never appear in logs, errors, `horizon-launcher` CLI output, MCP results or crash reports.

launcher/src/account.rs

@@ -590,11 +590,10 @@ impl SecretStore for MemoryStore {
 /// The secrets file in the launcher's data folder, when no keychain answers (#5095): many Linux
 /// systems run no Secret Service, and sign-ins would otherwise last one run. One JSON object, key
 /// to secret, readable and writable by the player only (0600 on Unix), replaced whole on every
-/// write. Gone once the last account signs out, and the keychain is used again from the next start.
+/// write. A lock file beside it keeps each read-modify-replace whole across every launcher and CLI
+/// process. The keychain takes its secrets back at the first start it answers ([`store`]).
 pub struct FileStore {
     path: PathBuf,
-    /// Writes are read-modify-write: one at a time.
-    lock: Mutex<()>,
 }
 
 /// The secrets file's name in the launcher's data folder.
@@ -602,10 +601,7 @@ pub const SECRETS_FILE: &str = "account-secrets.json";
 
 impl FileStore {
     pub fn new(path: PathBuf) -> Self {
-        Self {
-            path,
-            lock: Mutex::new(()),
-        }
+        Self { path }
     }
 
     /// The launcher's own: [`SECRETS_FILE`] in its data folder.
@@ -613,32 +609,58 @@ impl FileStore {
         Self::new(crate::backend::data_dir().join(SECRETS_FILE))
     }
 
+    fn shown(&self, e: std::io::Error) -> String {
+        format!("{}: {e}", self.path.display())
+    }
+
+    /// Runs `f` holding the lock file (`account-secrets.lock`): other processes wait.
+    fn locked<T>(&self, f: impl FnOnce() -> Result<T, String>) -> Result<T, String> {
+        if let Some(dir) = self.path.parent() {
+            std::fs::create_dir_all(dir).map_err(|e| self.shown(e))?;
+        }
+        let lock = std::fs::OpenOptions::new()
+            .create(true)
+            .truncate(false)
+            .write(true)
+            .open(self.path.with_extension("lock"))
+            .map_err(|e| self.shown(e))?;
+        lock.lock().map_err(|e| self.shown(e))?;
+        // Closing the file releases the lock, after `f` whatever it answers.
+        f()
+    }
+
     fn read(&self) -> Result<std::collections::BTreeMap<String, String>, String> {
         match std::fs::read(&self.path) {
-            // The error says where the file breaks, never what it holds.
-            Ok(bytes) => serde_json::from_slice(&bytes)
-                .map_err(|e| format!("{} can't be read: {e}", self.path.display())),
+            // Only where the file breaks: serde's own message can quote what the file holds.
+            Ok(bytes) => serde_json::from_slice(&bytes).map_err(|e| {
+                format!(
+                    "{} can't be read ({:?} error at line {}, column {}).",
+                    self.path.display(),
+                    e.classify(),
+                    e.line(),
+                    e.column()
+                )
+            }),
             Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Default::default()),
-            Err(e) => Err(format!("{}: {e}", self.path.display())),
+            Err(e) => Err(self.shown(e)),
         }
     }
 
-    /// Replaces the file with `secrets` through a temporary file beside it, so a crash leaves the
-    /// old file or the new one; removes it when there are none left.
+    /// Replaces the file with `secrets` through a temporary file of this write's own beside it, so
+    /// a crash leaves the old file or the new one; removes it when there are none left.
     fn write(&self, secrets: &std::collections::BTreeMap<String, String>) -> Result<(), String> {
-        let shown = |e: std::io::Error| format!("{}: {e}", self.path.display());
         if secrets.is_empty() {
             return match std::fs::remove_file(&self.path) {
-                Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(shown(e)),
+                Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(self.shown(e)),
                 _ => Ok(()),
             };
         }
-        if let Some(dir) = self.path.parent() {
-            std::fs::create_dir_all(dir).map_err(shown)?;
-        }
+        static WRITES: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
+        let n = WRITES.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        let tmp = self
+            .path
+            .with_extension(format!("json.{}.{n}.tmp", std::process::id()));
         let json = serde_json::to_vec(secrets).map_err(|e| e.to_string())?;
-        let tmp = self.path.with_extension("json.tmp");
-        let _ = std::fs::remove_file(&tmp);
         let mut options = std::fs::OpenOptions::new();
         options.write(true).create_new(true);
         #[cfg(unix)]
@@ -649,30 +671,47 @@ impl FileStore {
         });
         if let Err(e) = written.and_then(|()| std::fs::rename(&tmp, &self.path)) {
             let _ = std::fs::remove_file(&tmp);
-            return Err(shown(e));
+            return Err(self.shown(e));
         }
         Ok(())
     }
+
+    /// Moves every secret into `keychain`, then removes the file. Nothing is removed unless the
+    /// keychain took them all, and the file's copy wins: it was written while the keychain was
+    /// away, so it is the newer one.
+    fn move_into(&self, keychain: &dyn SecretStore) -> Result<(), String> {
+        if !self.path.exists() {
+            return Ok(());
+        }
+        self.locked(|| {
+            let secrets = self.read()?;
+            for (key, secret) in &secrets {
+                keychain.set(key, secret)?;
+            }
+            self.write(&Default::default())
+        })
+    }
 }
 
 impl SecretStore for FileStore {
     fn get(&self, key: &str) -> Result<Option<String>, String> {
-        let _held = self.lock.lock().unwrap();
-        Ok(self.read()?.remove(key))
+        self.locked(|| Ok(self.read()?.remove(key)))
     }
     fn set(&self, key: &str, secret: &str) -> Result<(), String> {
-        let _held = self.lock.lock().unwrap();
-        let mut secrets = self.read()?;
-        secrets.insert(key.into(), secret.into());
-        self.write(&secrets)
+        self.locked(|| {
+            let mut secrets = self.read()?;
+            secrets.insert(key.into(), secret.into());
+            self.write(&secrets)
+        })
     }
     fn delete(&self, key: &str) -> Result<(), String> {
-        let _held = self.lock.lock().unwrap();
-        let mut secrets = self.read()?;
-        if secrets.remove(key).is_none() {
-            return Ok(());
-        }
-        self.write(&secrets)
+        self.locked(|| {
+            let mut secrets = self.read()?;
+            if secrets.remove(key).is_none() {
+                return Ok(());
+            }
+            self.write(&secrets)
+        })
     }
     fn persistent(&self) -> bool {
         true
@@ -1033,24 +1072,21 @@ pub fn no_keychain() -> Option<String> {
 #[derive(Debug, PartialEq, Eq)]
 enum Kept {
     Keychain,
-    /// [`FileStore`]: the file is already there, or the keychain didn't answer.
     File,
     Memory,
 }
 
-/// `HORIZON_NO_KEYCHAIN` keeps secrets in memory (the launcher's own tests). A secrets file left by
-/// an earlier run stays in use, so sign-ins don't come and go with a keychain that answers only
-/// some of the time; signing every account out removes it. Otherwise the keychain if it answers
-/// `probe`, else the file.
-fn keep_in(no_keychain: bool, file_there: bool, probe: impl FnOnce() -> bool) -> Kept {
+/// `HORIZON_NO_KEYCHAIN` keeps secrets in memory (the launcher's own tests). Otherwise the keychain
+/// if it answers, once the secrets the file kept while it didn't have moved into it: an account
+/// signed in on a start the keychain missed comes back beside the ones it always held. Else the
+/// file, which then holds only what this run signs in.
+fn keep_in(no_keychain: bool, keychain: &dyn SecretStore, file: &FileStore) -> Kept {
     if no_keychain {
         Kept::Memory
-    } else if file_there {
+    } else if keychain.get("probe").is_err() || file.move_into(keychain).is_err() {
         Kept::File
-    } else if probe() {
-        Kept::Keychain
     } else {
-        Kept::File
+        Kept::Keychain
     }
 }
 
@@ -1072,11 +1108,8 @@ pub fn store() -> &'static dyn SecretStore {
             ))]
             {
                 let file = FileStore::launcher();
-                match keep_in(
-                    std::env::var_os("HORIZON_NO_KEYCHAIN").is_some(),
-                    file.path.exists(),
-                    || SystemStore.get("probe").is_ok(),
-                ) {
+                let no_keychain = std::env::var_os("HORIZON_NO_KEYCHAIN").is_some();
+                match keep_in(no_keychain, &SystemStore, &file) {
                     Kept::Keychain => return Box::new(SystemStore),
                     Kept::File => return Box::new(file),
                     Kept::Memory => {

launcher/src/account/tests.rs

@@ -1034,6 +1034,20 @@ fn secrets_file(name: &str) -> PathBuf {
     dir.join("launcher").join(SECRETS_FILE)
 }
 
+fn remove_secrets_dir(path: &Path) {
+    let _ = std::fs::remove_dir_all(path.parent().unwrap().parent().unwrap());
+}
+
+/// The files beside the secrets file, by name.
+fn files_beside(path: &Path) -> Vec<String> {
+    let mut names: Vec<String> = std::fs::read_dir(path.parent().unwrap())
+        .unwrap()
+        .map(|e| e.unwrap().file_name().to_string_lossy().into_owned())
+        .collect();
+    names.sort();
+    names
+}
+
 #[test]
 fn the_secrets_file_keeps_sign_ins_across_starts() {
     let path = secrets_file("starts");
@@ -1060,13 +1074,17 @@ fn the_secrets_file_keeps_sign_ins_across_starts() {
         let mode = std::fs::metadata(&path).unwrap().permissions().mode();
         assert_eq!(mode & 0o777, 0o600, "only the player reads it");
     }
-    assert!(!path.with_extension("json.tmp").exists());
-    // Signing the last account out removes the file: the keychain is used again from the next start.
+    // No temporary file left behind.
+    assert_eq!(
+        files_beside(&path),
+        ["account-secrets.json", "account-secrets.lock"]
+    );
+    // Signing the last account out removes the file.
     accounts.remove(&again, UUID).unwrap();
     assert_eq!(again.get(UUID).unwrap(), None);
     assert!(!path.exists());
     again.delete(UUID).unwrap();
-    let _ = std::fs::remove_dir_all(path.parent().unwrap().parent().unwrap());
+    remove_secrets_dir(&path);
 }
 
 #[test]
@@ -1081,24 +1099,153 @@ fn the_secrets_file_keeps_each_key_apart() {
         (store.get("a").unwrap(), store.get("b").unwrap()),
         (Some("three".into()), None)
     );
-    // A broken file is an error, never secrets lost in silence, and the error doesn't show it.
-    std::fs::write(&path, "{\"a\":\"leaked-secret").unwrap();
-    let said = store.get("a").unwrap_err();
-    assert!(!said.contains("leaked-secret"), "{said}");
-    assert!(store.set("c", "four").is_err());
-    let _ = std::fs::remove_dir_all(path.parent().unwrap().parent().unwrap());
+    remove_secrets_dir(&path);
 }
 
 #[test]
-fn the_file_takes_over_where_no_keychain_answers() {
-    // HORIZON_NO_KEYCHAIN: memory, whatever is there.
-    assert_eq!(keep_in(true, true, || true), Kept::Memory);
-    // The keychain first, and the file only when it doesn't answer.
-    assert_eq!(keep_in(false, false, || true), Kept::Keychain);
-    assert_eq!(keep_in(false, false, || false), Kept::File);
-    // A file from an earlier start stays in use, without asking the keychain.
+fn a_broken_secrets_file_is_an_error_that_never_shows_it() {
+    let path = secrets_file("broken");
+    let store = FileStore::new(path.clone());
+    // Cut short, and a lone string: serde's own messages quote the latter.
+    for broken in [
+        "{\"a\":\"leaked-secret",
+        "\"leaked-secret\"",
+        "[\"leaked-secret\"]",
+    ] {
+        std::fs::create_dir_all(path.parent().unwrap()).unwrap();
+        std::fs::write(&path, broken).unwrap();
+        let said = store.get("a").unwrap_err();
+        assert!(!said.contains("leaked-secret"), "{said}");
+        assert!(said.contains("line 1"), "{said}");
+        // Never written over: the secrets in it aren't lost in silence.
+        assert!(store.set("c", "four").is_err());
+        assert_eq!(std::fs::read_to_string(&path).unwrap(), broken);
+    }
+    remove_secrets_dir(&path);
+}
+
+/// Separate stores share nothing but the files, as separate launcher and CLI processes do.
+#[test]
+fn processes_sharing_the_secrets_file_lose_nothing() {
+    let path = secrets_file("processes");
+    FileStore::new(path.clone()).set("signed-out", "x").unwrap();
+    let writers: Vec<_> = (0..8)
+        .map(|w| {
+            let path = path.clone();
+            std::thread::spawn(move || {
+                let store = FileStore::new(path);
+                for i in 0..25 {
+                    store
+                        .set(&format!("{w}-{i}"), &format!("secret-{w}-{i}"))
+                        .unwrap();
+                }
+                if w == 0 {
+                    store.delete("signed-out").unwrap();
+                }
+            })
+        })
+        .collect();
+    for writer in writers {
+        writer.join().unwrap();
+    }
+    let store = FileStore::new(path.clone());
+    for w in 0..8 {
+        for i in 0..25 {
+            assert_eq!(
+                store.get(&format!("{w}-{i}")).unwrap(),
+                Some(format!("secret-{w}-{i}"))
+            );
+        }
+    }
+    assert_eq!(store.get("signed-out").unwrap(), None);
     assert_eq!(
-        keep_in(false, true, || panic!("the keychain isn't asked")),
-        Kept::File
+        files_beside(&path),
+        ["account-secrets.json", "account-secrets.lock"]
     );
+    remove_secrets_dir(&path);
+}
+
+/// A keychain that can be away, as a Secret Service that didn't start.
+#[derive(Default)]
+struct Keychain {
+    kept: MemoryStore,
+    away: std::sync::atomic::AtomicBool,
+    refuses_writes: std::sync::atomic::AtomicBool,
+}
+
+impl Keychain {
+    fn answer(&self) -> Result<(), String> {
+        if self.away.load(std::sync::atomic::Ordering::Relaxed) {
+            return Err("The name org.freedesktop.secrets was not provided".into());
+        }
+        Ok(())
+    }
+}
+
+impl SecretStore for Keychain {
+    fn get(&self, key: &str) -> Result<Option<String>, String> {
+        self.answer()?;
+        self.kept.get(key)
+    }
+    fn set(&self, key: &str, secret: &str) -> Result<(), String> {
+        self.answer()?;
+        if self
+            .refuses_writes
+            .load(std::sync::atomic::Ordering::Relaxed)
+        {
+            return Err("the keychain is full".into());
+        }
+        self.kept.set(key, secret)
+    }
+    fn delete(&self, key: &str) -> Result<(), String> {
+        self.answer()?;
+        self.kept.delete(key)
+    }
+    fn persistent(&self) -> bool {
+        true
+    }
+}
+
+#[test]
+fn the_keychain_takes_back_what_the_file_kept_while_it_was_away() {
+    use std::sync::atomic::Ordering::Relaxed;
+    let path = secrets_file("recovery");
+    let file = FileStore::new(path.clone());
+    let keychain = Keychain::default();
+    // HORIZON_NO_KEYCHAIN: memory, whatever answers.
+    assert_eq!(keep_in(true, &keychain, &file), Kept::Memory);
+    // A is signed in while the keychain answers: no file.
+    assert_eq!(keep_in(false, &keychain, &file), Kept::Keychain);
+    keychain.set("A", "secret-a").unwrap();
+    assert!(!path.exists());
+    // A start the keychain misses: B goes to the file.
+    keychain.away.store(true, Relaxed);
+    assert_eq!(keep_in(false, &keychain, &file), Kept::File);
+    file.set("B", "secret-b").unwrap();
+    // The keychain is back: it holds both, and the file is gone.
+    keychain.away.store(false, Relaxed);
+    assert_eq!(keep_in(false, &keychain, &file), Kept::Keychain);
+    assert_eq!(
+        (keychain.get("A").unwrap(), keychain.get("B").unwrap()),
+        (Some("secret-a".into()), Some("secret-b".into()))
+    );
+    assert!(!path.exists());
+    remove_secrets_dir(&path);
+}
+
+#[test]
+fn a_keychain_that_refuses_the_move_leaves_the_file_whole() {
+    use std::sync::atomic::Ordering::Relaxed;
+    let path = secrets_file("refused");
+    let file = FileStore::new(path.clone());
+    file.set("A", "secret-a").unwrap();
+    file.set("B", "secret-b").unwrap();
+    let keychain = Keychain::default();
+    keychain.refuses_writes.store(true, Relaxed);
+    assert_eq!(keep_in(false, &keychain, &file), Kept::File);
+    assert_eq!(
+        (file.get("A").unwrap(), file.get("B").unwrap()),
+        (Some("secret-a".into()), Some("secret-b".into()))
+    );
+    remove_secrets_dir(&path);
 }