Launcher: serialize keychain writes so a hand-over can't undo a sign-out

Review fixes for the secrets file (#5095):

- A start's hand-over wrote a snapshot of the file to the keychain, so
  a token another process signed out or renewed meanwhile could come
  back or be overwritten. Every keychain write now holds
  account-secrets.keychain.lock, and the hand-over reads each entry
  again under it. The file's own lock still never waits on a prompt.
- MCP secrets_file is true only while the file holds a sign-in: not
  for sign-out marks alone, nor for a file that can't be read.
- Tests: a hand-over held on a prompt while a running launcher signs
  out or renews the same account; what counts as a sign-in in the file.

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

docs/microsoft-account.md

@@ -146,7 +146,9 @@ one is selected. "Sign out" deletes the account's secrets from the keychain.
   of its own, so launcher and CLI processes running at once lose nothing. The lock is never held
   while the keychain is asked (it can wait on an unlock prompt): a file entry the keychain took is
   removed only if it still holds what was read before, so another process's newer write stays and
-  is read first. Once the keychain refuses or doesn't answer (a dismissed prompt), the run stops
+  is read first. Writes to the keychain hold `account-secrets.keychain.lock` instead, with what they
+  write read under it, so a hand-over never puts back a token another process renewed or signed out
+  meanwhile; only keychain writes wait on a prompt, as they would on the keychain itself. Once the keychain refuses or doesn't answer (a dismissed prompt), the run stops
   asking it and keeps new sign-ins in the file, until it finds that another process handed the
   keychain a secret it put there. Every start
   hands the keychain what the file kept, one secret at a time up to the first it refuses; the rest
@@ -155,7 +157,7 @@ one is selected. "Sign out" deletes the account's secrets from the keychain.
   only its line and column, never what it holds. Only sign-ins go there: the Tailscale access token
   (Internet servers) administers the player's whole tailnet, so without a keychain it stays in
   memory for the run (`account::keychain`). MCP's `account_list` says `secrets_file: true` while
-  sign-ins sit in the file.
+  the file holds a sign-in (sign-out marks alone, or a file that can't be read, don't count).
 - 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".

launcher/src/account.rs

@@ -613,8 +613,13 @@ impl FileStore {
         format!("{}: {e}", self.path.display())
     }
 
-    /// Runs `f` holding the lock file (`account-secrets.lock`): other processes wait.
+    /// Runs `f` holding the file's lock (`account-secrets.lock`): other processes wait.
     fn locked<T>(&self, f: impl FnOnce() -> Result<T, String>) -> Result<T, String> {
+        self.holding("lock", f)
+    }
+
+    /// Runs `f` holding the lock file `account-secrets.<name>`, made if need be.
+    fn holding<T>(&self, name: &str, 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))?;
         }
@@ -622,13 +627,19 @@ impl FileStore {
             .create(true)
             .truncate(false)
             .write(true)
-            .open(self.path.with_extension("lock"))
+            .open(self.path.with_extension(name))
             .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()
     }
 
+    /// Whether the file holds a sign-in: not only sign-out marks, and readable.
+    fn holds_secrets(&self) -> bool {
+        self.entries()
+            .is_ok_and(|secrets| secrets.values().any(|secret| secret != SIGNED_OUT))
+    }
+
     fn read(&self) -> Result<std::collections::BTreeMap<String, String>, String> {
         match std::fs::read(&self.path) {
             // Only where the file breaks: serde's own message can quote what the file holds.
@@ -1110,8 +1121,11 @@ const KEYCHAIN_AWAY: &str =
 /// is the newer one), writes go to the keychain and to the file only when it refuses.
 ///
 /// The file's lock is never held across a keychain call, which can wait on an unlock prompt: other
-/// processes keep reading and writing the file meanwhile. A file entry is only removed if it still
-/// holds what was read before the keychain call, so another process's newer write stays. And once the
+/// processes keep reading and writing the file meanwhile. Every write to the keychain holds a lock of
+/// its own instead (`account-secrets.keychain.lock`), with what it writes read under it, so a hand-over
+/// never puts back a secret another process renewed or signed out since; only keychain writes wait
+/// on a prompt, as they would on the keychain itself. A file entry is only removed if it still holds
+/// what was read before the keychain call, so another process's newer write stays. And once the
 /// keychain refuses or doesn't answer, this run stops asking it (no prompt after a dismissed
 /// prompt), until another process hands it a secret this run parked in the file.
 pub struct Layered {
@@ -1124,7 +1138,8 @@ pub struct Layered {
 
 impl Layered {
     /// Starts by handing the keychain what the file kept while it was away, one secret at a time,
-    /// up to the first it refuses; sign-outs it missed are done there too.
+    /// up to the first it refuses; sign-outs it missed are done there too. Each is read again under
+    /// the keychain's lock: one another process renewed or signed out meanwhile isn't handed over.
     pub fn new(keychain: Box<dyn SecretStore>, file: FileStore) -> Self {
         let layered = Self {
             keychain,
@@ -1132,21 +1147,29 @@ impl Layered {
             away: Default::default(),
             parked: Default::default(),
         };
-        let mut moved = Vec::new();
-        for (key, secret) in layered.file.entries().unwrap_or_default() {
-            let handed = layered.ask(|keychain| match secret.as_str() {
-                SIGNED_OUT => keychain.delete(&key),
-                _ => keychain.set(&key, &secret),
+        for key in layered.file.entries().unwrap_or_default().into_keys() {
+            let handed = layered.keychain_writes(|| {
+                let Some(secret) = layered.file.entry(&key)? else {
+                    return Ok(());
+                };
+                layered.ask(|keychain| match secret.as_str() {
+                    SIGNED_OUT => keychain.delete(&key),
+                    _ => keychain.set(&key, &secret),
+                })?;
+                layered.file.take_unchanged(&[(key.clone(), secret)])
             });
             if handed.is_err() {
                 break;
             }
-            moved.push((key, secret));
         }
-        let _ = layered.file.take_unchanged(&moved);
         layered
     }
 
+    /// Runs `f`, a write to the keychain and what it reads first, holding the keychain's lock.
+    fn keychain_writes<T>(&self, f: impl FnOnce() -> Result<T, String>) -> Result<T, String> {
+        self.file.holding("keychain.lock", f)
+    }
+
     fn is_away(&self) -> bool {
         self.away.load(std::sync::atomic::Ordering::Relaxed)
     }
@@ -1198,19 +1221,29 @@ impl SecretStore for Layered {
         self.ask(|keychain| keychain.get(key))
     }
     fn set(&self, key: &str, secret: &str) -> Result<(), String> {
-        let before = self.file.entry(key)?;
-        match self.ask(|keychain| keychain.set(key, secret)) {
-            Ok(()) => self.unpark(key, before),
-            Err(_) => self.park(key, secret),
+        if self.is_away() {
+            return self.park(key, secret);
         }
+        self.keychain_writes(|| {
+            let before = self.file.entry(key)?;
+            match self.ask(|keychain| keychain.set(key, secret)) {
+                Ok(()) => self.unpark(key, before),
+                Err(_) => self.park(key, secret),
+            }
+        })
     }
     fn delete(&self, key: &str) -> Result<(), String> {
-        let before = self.file.entry(key)?;
-        match self.ask(|keychain| keychain.delete(key)) {
-            Ok(()) => self.unpark(key, before),
-            // The keychain may hold a copy: the mark hides it until it can go.
-            Err(_) => self.park(key, SIGNED_OUT),
+        // The keychain may hold a copy: the mark hides it until it can go.
+        if self.is_away() {
+            return self.park(key, SIGNED_OUT);
         }
+        self.keychain_writes(|| {
+            let before = self.file.entry(key)?;
+            match self.ask(|keychain| keychain.delete(key)) {
+                Ok(()) => self.unpark(key, before),
+                Err(_) => self.park(key, SIGNED_OUT),
+            }
+        })
     }
     fn persistent(&self) -> bool {
         true
@@ -1282,11 +1315,12 @@ pub fn keychain() -> &'static dyn SecretStore {
     store()
 }
 
-/// Whether sign-ins sit in the secrets file now, the keychain having refused them (#5095).
+/// Whether sign-ins sit in the secrets file now, the keychain having refused them (#5095): sign-out
+/// marks alone, or a file that can't be read, don't count.
 pub fn in_secrets_file() -> bool {
     cfg!(any(target_os = "linux", target_os = "freebsd"))
         && store().persistent()
-        && FileStore::launcher().path.exists()
+        && FileStore::launcher().holds_secrets()
 }
 
 // ----- Accounts ------------------------------------------------------------------------------------

launcher/src/account/tests.rs

@@ -1380,6 +1380,80 @@ fn a_keychain_waiting_on_a_prompt_never_holds_the_file_up() {
     remove_secrets_dir(&path);
 }
 
+/// A start handing the file's A to the keychain, held up mid-way by an unlock prompt, while a
+/// launcher already running does `meanwhile` to A. The running one waits for the hand-over instead
+/// of racing it, and goes on once the prompt is answered.
+fn hand_over_racing(
+    name: &str,
+    meanwhile: impl FnOnce(&Layered) + Send + 'static,
+) -> (Arc<Keychain>, PathBuf) {
+    let path = secrets_file(name);
+    let keychain = Arc::new(Keychain::default());
+    // Started before the file had anything: it asked the keychain nothing.
+    let running = launcher_with(&keychain, &path);
+    // A run the keychain missed kept A in the file.
+    FileStore::new(path.clone()).set("A", "old-a").unwrap();
+    let prompt = keychain.prompt.lock().unwrap();
+    let asked = keychain.asked();
+    let starting = {
+        let (keychain, path) = (keychain.clone(), path.clone());
+        std::thread::spawn(move || drop(launcher_with(&keychain, &path)))
+    };
+    while keychain.asked() == asked {
+        std::thread::sleep(Duration::from_millis(1));
+    }
+    let racing = std::thread::spawn(move || meanwhile(&running));
+    std::thread::sleep(Duration::from_millis(100));
+    assert_eq!(
+        keychain.asked(),
+        asked + 1,
+        "the running launcher reached the keychain during the hand-over"
+    );
+    drop(prompt);
+    starting.join().unwrap();
+    racing.join().unwrap();
+    (keychain, path)
+}
+
+#[test]
+fn a_hand_over_never_brings_back_an_account_signed_out_meanwhile() {
+    let (keychain, path) = hand_over_racing("race-sign-out", |running| {
+        running.delete("A").unwrap();
+    });
+    assert_eq!(keychain.kept.get("A").unwrap(), None);
+    assert_eq!(launcher_with(&keychain, &path).get("A").unwrap(), None);
+    assert!(!path.exists());
+    remove_secrets_dir(&path);
+}
+
+#[test]
+fn a_hand_over_never_writes_over_a_token_renewed_meanwhile() {
+    let (keychain, path) = hand_over_racing("race-renewal", |running| {
+        running.set("A", "renewed-a").unwrap();
+    });
+    assert_eq!(keychain.kept.get("A").unwrap(), Some("renewed-a".into()));
+    assert_eq!(
+        launcher_with(&keychain, &path).get("A").unwrap(),
+        Some("renewed-a".into())
+    );
+    assert!(!path.exists());
+    remove_secrets_dir(&path);
+}
+
+#[test]
+fn the_secrets_file_counts_only_sign_ins() {
+    let path = secrets_file("counts");
+    let file = FileStore::new(path.clone());
+    assert!(!file.holds_secrets());
+    file.put("A", SIGNED_OUT).unwrap();
+    assert!(!file.holds_secrets(), "a sign-out mark alone");
+    file.put("B", "secret-b").unwrap();
+    assert!(file.holds_secrets());
+    std::fs::write(&path, "{\"B\":\"secret-b").unwrap();
+    assert!(!file.holds_secrets(), "a file that can't be read");
+    remove_secrets_dir(&path);
+}
+
 #[test]
 fn a_dismissed_prompt_is_asked_once_a_run() {
     let path = secrets_file("dismissed");

launcher/src/mcp.rs

@@ -200,7 +200,7 @@ impl Session {
                     "plays_as": accounts.selected().map(|a| a.name.clone()),
                     "sign_in_configured": account::client_id().is_some(),
                     // Sign-ins outlast the launcher: in the keychain, or in the secrets file
-                    // where it refused them (`secrets_file`).
+                    // where it refused them (`secrets_file`: the file holds a sign-in).
                     "keychain": account::store().persistent(),
                     "secrets_file": account::in_secrets_file(),
                     // Play needs an account that owns the game (the owner gate).