Launcher: read the keychain and the secrets file together on Linux

Review fixes for the secrets file (#5095):

- No store is picked once per run any more: on Linux and FreeBSD every
  read takes the file's copy first and the keychain's otherwise, and
  every write goes to the keychain, to the file only when it refuses.
  A launcher that wrote to the file keeps reading, renewing and signing
  out after another process hands the file to the keychain.
- Each start hands the keychain the file's secrets one at a time, each
  leaving the file once the keychain took it, so a move it half refuses
  never leaves a secret in both places, and signing out removes it.
- Signing out while the keychain is away leaves an empty entry that
  hides the keychain's copy until it can be deleted.
- macOS and Windows keep the keychain or memory, as before.
- The concurrency test now runs real child processes.

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

docs/microsoft-account.md

@@ -136,17 +136,21 @@ one is selected. "Sign out" deletes the account's secrets from the keychain.
   other apps' items.
 - `accounts.json` in the launcher's data folder keeps only public data: UUID, name, skin URL, which
   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), 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".
+- Linux and FreeBSD with no keychain answering (no Secret Service, a locked keychain, #5095): the
+  secrets the keychain refuses go to `account-secrets.json` in the launcher's data folder instead,
+  readable and writable by the player only (0600 on Unix), as most launchers keep them. The
+  launcher never picks one store for a run: every read takes the file's copy first (it was written
+  while the keychain was away, so it is the newer one) and the keychain's otherwise, and every write
+  goes to the keychain and to the file only when it refuses. Each call holds `account-secrets.lock`
+  across its read and replace, and each write goes through a temporary file of its own, so launcher
+  and CLI processes running at once lose nothing and a secret is never in two places. Every start
+  hands the keychain what the file kept, one secret at a time; what it refuses stays in the file,
+  which goes once it is empty. Signing out while the keychain is away leaves an empty entry for the
+  account, which hides the keychain's copy until it can be deleted. A broken file is an error naming
+  only its line and column, never what it holds.
+- No keychain on macOS or Windows, or `HORIZON_NO_KEYCHAIN` (the launcher's own tests): secrets
+  stay in memory for the run, and the launcher 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.
   Requests are logged by endpoint name only.
 

launcher/src/account.rs

@@ -591,7 +591,7 @@ impl SecretStore for MemoryStore {
 /// 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. 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`]).
+/// process. The keychain takes its secrets back at the first start it answers ([`Layered`]).
 pub struct FileStore {
     path: PathBuf,
 }
@@ -676,23 +676,33 @@ impl FileStore {
         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> {
+    /// Hands the file's secrets to `keychain` one by one, each leaving the file once the keychain
+    /// took it (the file's copy is the newer one: it was written while the keychain was away), and
+    /// sign-outs the keychain missed are done there too. What the keychain refuses stays in the
+    /// file, and the file goes once it is empty. Whatever stops it, every secret is in one place.
+    fn move_into(&self, keychain: &dyn SecretStore) {
         if !self.path.exists() {
-            return Ok(());
+            return;
         }
-        self.locked(|| {
-            let secrets = self.read()?;
-            for (key, secret) in &secrets {
-                keychain.set(key, secret)?;
-            }
-            self.write(&Default::default())
-        })
+        let _ = self.locked(|| {
+            let mut secrets = self.read()?;
+            secrets.retain(|key, secret| {
+                let moved = if secret.as_str() == SIGNED_OUT {
+                    keychain.delete(key)
+                } else {
+                    keychain.set(key, secret)
+                };
+                moved.is_err()
+            });
+            self.write(&secrets)
+        });
     }
 }
 
+/// A file entry for an account signed out while the keychain was away: the keychain's copy, if any,
+/// counts as gone, and goes at the first start it answers.
+const SIGNED_OUT: &str = "";
+
 impl SecretStore for FileStore {
     fn get(&self, key: &str) -> Result<Option<String>, String> {
         self.locked(|| Ok(self.read()?.remove(key)))
@@ -1068,30 +1078,70 @@ pub fn no_keychain() -> Option<String> {
     })
 }
 
-/// Where [`store`] keeps secrets.
-#[derive(Debug, PartialEq, Eq)]
-enum Kept {
-    Keychain,
-    File,
-    Memory,
+/// The keychain, with the secrets file for what it can't keep (#5095). Each call asks both, never a
+/// choice made once: a launcher that wrote to the file while the keychain was away still finds its
+/// secrets after another process handed them to the keychain. Reads take the file's copy first (it
+/// is the newer one), writes go to the keychain and to the file only when it refuses, and every call
+/// holds the file's lock, so a secret is in one place whatever runs at once.
+pub struct Layered {
+    keychain: Box<dyn SecretStore>,
+    file: FileStore,
 }
 
-/// `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 keychain.get("probe").is_err() || file.move_into(keychain).is_err() {
-        Kept::File
-    } else {
-        Kept::Keychain
+impl Layered {
+    /// Starts by handing the keychain what the file kept while it was away.
+    pub fn new(keychain: Box<dyn SecretStore>, file: FileStore) -> Self {
+        file.move_into(keychain.as_ref());
+        Self { keychain, file }
     }
 }
 
-/// The keychain if it answers, else [`FileStore`] (#5095); memory with `HORIZON_NO_KEYCHAIN` (said
-/// in the UI with [`no_keychain`]).
+impl SecretStore for Layered {
+    fn get(&self, key: &str) -> Result<Option<String>, String> {
+        if self.file.path.exists() {
+            match self.file.locked(|| Ok(self.file.read()?.remove(key)))? {
+                Some(secret) if secret == SIGNED_OUT => return Ok(None),
+                Some(secret) => return Ok(Some(secret)),
+                None => {}
+            }
+        }
+        self.keychain.get(key)
+    }
+    fn set(&self, key: &str, secret: &str) -> Result<(), String> {
+        self.file.locked(|| {
+            let mut secrets = self.file.read()?;
+            match self.keychain.set(key, secret) {
+                Ok(()) if secrets.remove(key).is_none() => return Ok(()),
+                Ok(()) => {}
+                Err(_) => {
+                    secrets.insert(key.into(), secret.into());
+                }
+            }
+            self.file.write(&secrets)
+        })
+    }
+    fn delete(&self, key: &str) -> Result<(), String> {
+        self.file.locked(|| {
+            let mut secrets = self.file.read()?;
+            match self.keychain.delete(key) {
+                Ok(()) if secrets.remove(key).is_none() => return Ok(()),
+                Ok(()) => {}
+                // The keychain may hold a copy: the mark hides it until it can go.
+                Err(_) => {
+                    secrets.insert(key.into(), SIGNED_OUT.into());
+                }
+            }
+            self.file.write(&secrets)
+        })
+    }
+    fn persistent(&self) -> bool {
+        true
+    }
+}
+
+/// Linux and FreeBSD: the Secret Service with the secrets file beside it ([`Layered`], #5095).
+/// macOS and Windows: the keychain if it answers. Memory with `HORIZON_NO_KEYCHAIN` (the launcher's
+/// own tests) or no keychain on macOS and Windows (said in the UI with [`no_keychain`]).
 pub fn store() -> &'static dyn SecretStore {
     static STORE: OnceLock<Box<dyn SecretStore>> = OnceLock::new();
     STORE
@@ -1107,15 +1157,19 @@ pub fn store() -> &'static dyn SecretStore {
                 )
             ))]
             {
-                let file = FileStore::launcher();
-                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 => {
-                        let _ = NO_KEYCHAIN_WHY.set("HORIZON_NO_KEYCHAIN is set.".into());
+                let why = match std::env::var_os("HORIZON_NO_KEYCHAIN") {
+                    Some(_) => "HORIZON_NO_KEYCHAIN is set.".to_string(),
+                    #[cfg(any(target_os = "linux", target_os = "freebsd"))]
+                    None => {
+                        return Box::new(Layered::new(Box::new(SystemStore), FileStore::launcher()))
                     }
-                }
+                    #[cfg(any(target_os = "macos", target_os = "windows"))]
+                    None => match SystemStore.get("probe") {
+                        Ok(_) => return Box::new(SystemStore),
+                        Err(e) => e,
+                    },
+                };
+                let _ = NO_KEYCHAIN_WHY.set(why);
             }
             Box::<MemoryStore>::default()
         })

launcher/src/account/tests.rs

@@ -1087,21 +1087,6 @@ fn the_secrets_file_keeps_sign_ins_across_starts() {
     remove_secrets_dir(&path);
 }
 
-#[test]
-fn the_secrets_file_keeps_each_key_apart() {
-    let path = secrets_file("keys");
-    let store = FileStore::new(path.clone());
-    store.set("a", "one").unwrap();
-    store.set("b", "two").unwrap();
-    store.set("a", "three").unwrap();
-    store.delete("b").unwrap();
-    assert_eq!(
-        (store.get("a").unwrap(), store.get("b").unwrap()),
-        (Some("three".into()), None)
-    );
-    remove_secrets_dir(&path);
-}
-
 #[test]
 fn a_broken_secrets_file_is_an_error_that_never_shows_it() {
     let path = secrets_file("broken");
@@ -1124,32 +1109,54 @@ fn a_broken_secrets_file_is_an_error_that_never_shows_it() {
     remove_secrets_dir(&path);
 }
 
-/// Separate stores share nothing but the files, as separate launcher and CLI processes do.
+const WRITER: &str = "HORIZON_TEST_SECRETS_WRITER";
+
+/// One of the processes [`processes_sharing_the_secrets_file_lose_nothing`] starts: does nothing
+/// unless it gives the file and the writer's number.
+#[test]
+#[ignore = "a child process of processes_sharing_the_secrets_file_lose_nothing"]
+fn secrets_file_writer() {
+    let Some(spec) = std::env::var_os(WRITER) else {
+        return;
+    };
+    let spec = spec.into_string().unwrap();
+    let (w, path) = spec.split_once(':').unwrap();
+    let store = FileStore::new(PathBuf::from(path));
+    for i in 0..25 {
+        store
+            .set(&format!("{w}-{i}"), &format!("secret-{w}-{i}"))
+            .unwrap();
+    }
+    if w == "0" {
+        store.delete("signed-out").unwrap();
+    }
+}
+
 #[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)
+    let exe = std::env::current_exe().unwrap();
+    let writers: Vec<_> = (0..6)
         .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();
-                }
-            })
+            std::process::Command::new(&exe)
+                .args([
+                    "--ignored",
+                    "--exact",
+                    "account::tests::secrets_file_writer",
+                    "--test-threads=1",
+                ])
+                .env(WRITER, format!("{w}:{}", path.display()))
+                .stdout(std::process::Stdio::null())
+                .spawn()
+                .unwrap()
         })
         .collect();
-    for writer in writers {
-        writer.join().unwrap();
+    for mut writer in writers {
+        assert!(writer.wait().unwrap().success());
     }
     let store = FileStore::new(path.clone());
-    for w in 0..8 {
+    for w in 0..6 {
         for i in 0..25 {
             assert_eq!(
                 store.get(&format!("{w}-{i}")).unwrap(),
@@ -1165,12 +1172,12 @@ fn processes_sharing_the_secrets_file_lose_nothing() {
     remove_secrets_dir(&path);
 }
 
-/// A keychain that can be away, as a Secret Service that didn't start.
+/// A keychain that can be away, as a Secret Service that didn't start, or refuse some keys.
 #[derive(Default)]
 struct Keychain {
     kept: MemoryStore,
     away: std::sync::atomic::AtomicBool,
-    refuses_writes: std::sync::atomic::AtomicBool,
+    refuses: Mutex<Vec<String>>,
 }
 
 impl Keychain {
@@ -1180,72 +1187,148 @@ impl Keychain {
         }
         Ok(())
     }
+    fn set_away(&self, away: bool) {
+        self.away.store(away, std::sync::atomic::Ordering::Relaxed);
+    }
 }
 
-impl SecretStore for Keychain {
+/// One keychain seen by several launchers.
+struct Shared(Arc<Keychain>);
+
+impl SecretStore for Shared {
     fn get(&self, key: &str) -> Result<Option<String>, String> {
-        self.answer()?;
-        self.kept.get(key)
+        self.0.answer()?;
+        self.0.kept.get(key)
     }
     fn set(&self, key: &str, secret: &str) -> Result<(), String> {
-        self.answer()?;
-        if self
-            .refuses_writes
-            .load(std::sync::atomic::Ordering::Relaxed)
-        {
+        self.0.answer()?;
+        if self.0.refuses.lock().unwrap().iter().any(|k| k == key) {
             return Err("the keychain is full".into());
         }
-        self.kept.set(key, secret)
+        self.0.kept.set(key, secret)
     }
     fn delete(&self, key: &str) -> Result<(), String> {
-        self.answer()?;
-        self.kept.delete(key)
+        self.0.answer()?;
+        self.0.kept.delete(key)
     }
     fn persistent(&self) -> bool {
         true
     }
 }
 
+/// A launcher starting with this keychain and secrets file.
+fn launcher_with(keychain: &Arc<Keychain>, path: &Path) -> Layered {
+    Layered::new(
+        Box::new(Shared(keychain.clone())),
+        FileStore::new(path.to_path_buf()),
+    )
+}
+
 #[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);
+    let keychain = Arc::new(Keychain::default());
     // 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();
+    launcher_with(&keychain, &path)
+        .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);
+    // A start the keychain misses: B goes to the file, and A can't be read.
+    keychain.set_away(true);
+    let missed = launcher_with(&keychain, &path);
+    missed.set("B", "secret-b").unwrap();
+    assert!(missed.get("A").is_err());
+    assert_eq!(missed.get("B").unwrap(), Some("secret-b".into()));
+    // The keychain is back: the next start hands it B, and the file is gone.
+    keychain.set_away(false);
+    let back = launcher_with(&keychain, &path);
     assert_eq!(
-        (keychain.get("A").unwrap(), keychain.get("B").unwrap()),
+        (
+            keychain.kept.get("A").unwrap(),
+            keychain.kept.get("B").unwrap()
+        ),
         (Some("secret-a".into()), Some("secret-b".into()))
     );
+    assert_eq!(back.get("B").unwrap(), Some("secret-b".into()));
+    assert!(!path.exists());
+    remove_secrets_dir(&path);
+}
+
+#[test]
+fn a_launcher_on_the_file_keeps_working_after_another_hands_it_to_the_keychain() {
+    let path = secrets_file("running");
+    let keychain = Arc::new(Keychain::default());
+    keychain.set_away(true);
+    let running = launcher_with(&keychain, &path);
+    running.set("A", "secret-a").unwrap();
+    // The keychain comes back and a second launcher (or the CLI) starts.
+    keychain.set_away(false);
+    let second = launcher_with(&keychain, &path);
+    assert!(!path.exists());
+    // The first one still reads, renews and signs out.
+    assert_eq!(running.get("A").unwrap(), Some("secret-a".into()));
+    running.set("A", "renewed-a").unwrap();
+    assert_eq!(second.get("A").unwrap(), Some("renewed-a".into()));
+    running.delete("A").unwrap();
+    assert_eq!(
+        (second.get("A").unwrap(), keychain.kept.get("A").unwrap()),
+        (None, None)
+    );
     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);
+fn a_move_the_keychain_half_refuses_leaves_each_secret_in_one_place() {
+    let path = secrets_file("half");
+    let keychain = Arc::new(Keychain::default());
+    keychain.set_away(true);
+    let away = launcher_with(&keychain, &path);
+    away.set("A", "secret-a").unwrap();
+    away.set("B", "secret-b").unwrap();
+    // Back, but it refuses B: A moves, B stays in the file.
+    keychain.set_away(false);
+    keychain.refuses.lock().unwrap().push("B".into());
+    let back = launcher_with(&keychain, &path);
     assert_eq!(
-        (file.get("A").unwrap(), file.get("B").unwrap()),
+        (
+            keychain.kept.get("A").unwrap(),
+            keychain.kept.get("B").unwrap()
+        ),
+        (Some("secret-a".into()), None)
+    );
+    assert_eq!(
+        (back.get("A").unwrap(), back.get("B").unwrap()),
         (Some("secret-a".into()), Some("secret-b".into()))
     );
+    // Signing A out leaves no copy anywhere; B is still kept.
+    back.delete("A").unwrap();
+    assert_eq!(
+        (back.get("A").unwrap(), keychain.kept.get("A").unwrap()),
+        (None, None)
+    );
+    assert_eq!(back.get("B").unwrap(), Some("secret-b".into()));
+    remove_secrets_dir(&path);
+}
+
+#[test]
+fn signing_out_while_the_keychain_is_away_forgets_its_copy() {
+    let path = secrets_file("sign-out");
+    let keychain = Arc::new(Keychain::default());
+    launcher_with(&keychain, &path)
+        .set("A", "secret-a")
+        .unwrap();
+    keychain.set_away(true);
+    let away = launcher_with(&keychain, &path);
+    away.delete("A").unwrap();
+    assert_eq!(away.get("A").unwrap(), None);
+    // The keychain's copy goes at the first start it answers, and the file with it.
+    keychain.set_away(false);
+    let back = launcher_with(&keychain, &path);
+    assert_eq!(
+        (back.get("A").unwrap(), keychain.kept.get("A").unwrap()),
+        (None, None)
+    );
+    assert!(!path.exists());
     remove_secrets_dir(&path);
 }