Preserve fallback token refreshes when the keyring is unreadable (#58343)

## Why

Automatic credential storage can load fallback credentials when the keyring is unreadable, but refreshing those credentials previously failed to save the rotated tokens. Unresolvable `auth.json` symlinks could also block keyring credential updates and logout.

## What changed

- Save refreshed fallback credentials to the file store when the keyring cannot be read, while still rejecting stale credentials and refreshes after logout. Leave the unread keyring entry untouched, even if it recovers before the save.
- Allow keyring commits to proceed when fallback symlink resolution fails. Continue sharing the resolved target's commit lock for valid symlink chains so logout coordinates with concurrent refreshes.

## Testing

Add regression coverage for persistent keyring read failures, stale fallback refreshes, logout, unusable fallback symlinks, and cross-process locking through chained symlinks.

GitOrigin-RevId: 32e35f54f9791df73247ae0af40ff3795c6253ba

codex-rs/login/src/auth/storage.rs

@@ -559,6 +559,37 @@ impl AuthStorageBackend for AutoAuthStorage {
         }
     }
 
+    fn compare_and_save(&self, expected: &AuthDotJson, auth: &AuthDotJson) -> std::io::Result<()> {
+        let keyring_auth = self.keyring_storage.load();
+        let current = match keyring_auth {
+            Ok(Some(current)) => Some(current),
+            Ok(None) => self.file_storage.load()?,
+            Err(ref error) => {
+                // Preserve refreshed fallback tokens without writing to a keyring
+                // whose current generation we could not read, even if it recovers.
+                let mut telemetry = storage_telemetry::telemetry(
+                    AuthCredentialsStoreMode::Auto,
+                    self.keyring_backend_kind,
+                    Operation::Save,
+                );
+                telemetry.record_load_attempt(
+                    storage_telemetry::keyring_store(self.keyring_backend_kind),
+                    &keyring_auth,
+                );
+                telemetry.record_secure_error(error);
+                let result = self.file_storage.compare_and_save(expected, auth);
+                telemetry.record_save_attempt(Store::File, &result);
+                return result;
+            }
+        };
+        if current.as_ref() != Some(expected) {
+            return Err(std::io::Error::other(
+                "Credentials changed before refresh commit.",
+            ));
+        }
+        self.save(auth)
+    }
+
     fn load(&self) -> std::io::Result<Option<AuthDotJson>> {
         let mut telemetry = storage_telemetry::telemetry(
             AuthCredentialsStoreMode::Auto,

codex-rs/login/src/auth/storage_commit.rs

@@ -27,10 +27,16 @@ struct CommittedStorage {
 
 impl CommittedStorage {
     fn lock(&self) -> std::io::Result<File> {
-        // File, keyring, and auto writers must coordinate: auto may use the file
-        // fallback. Resolve symlinks so aliases of that file share the same lock.
-        let path =
-            storage_file::resolve_auth_file(get_auth_file(&self.home))?.with_extension("json.lock");
+        let auth_file = get_auth_file(&self.home);
+        // Keyring commits can unlink an intermediate symlink used by another
+        // home, so resolvable chains must share the target's commit lock.
+        let path = match storage_file::resolve_auth_file(auth_file.clone()) {
+            Ok(path) => path,
+            // Unresolvable fallback links can still be removed by keyring commits.
+            Err(_) if self.mode == AuthCredentialsStoreMode::Keyring => auth_file,
+            Err(error) => return Err(error),
+        }
+        .with_extension("json.lock");
         std::fs::create_dir_all(
             path.parent()
                 .ok_or_else(|| std::io::Error::other("credential lock has no parent"))?,

codex-rs/login/src/auth/storage_commit_tests.rs

@@ -6,6 +6,54 @@ use std::io::BufRead;
 use std::io::Read;
 use std::process::Stdio;
 
+#[cfg(unix)]
+#[test]
+fn keyring_commits_ignore_unusable_fallback_symlink_targets() -> anyhow::Result<()> {
+    use codex_keyring_store::tests::MockKeyringStore;
+    use std::os::unix::fs::symlink;
+
+    for kind in [
+        AuthKeyringBackendKind::Direct,
+        AuthKeyringBackendKind::Secrets,
+    ] {
+        for target in ["auth.json", "not-a-directory/auth.json"] {
+            let home = tempfile::tempdir()?;
+            std::fs::write(home.path().join("not-a-directory"), "")?;
+            let auth_file = get_auth_file(home.path());
+            let storage = create_auth_storage_with_store(
+                home.path().to_path_buf(),
+                AuthCredentialsStoreMode::Keyring,
+                Arc::new(MockKeyringStore::default()),
+                kind,
+            );
+            let first = serde_json::from_value(serde_json::json!({"OPENAI_API_KEY": "first"}))?;
+            let second = serde_json::from_value(serde_json::json!({"OPENAI_API_KEY": "second"}))?;
+            let _guard = storage.lock_for_write()?;
+
+            symlink(target, &auth_file)?;
+            storage.save(&first)?;
+            assert_eq!(storage.load()?, Some(first.clone()));
+            assert_eq!(
+                std::fs::symlink_metadata(&auth_file).unwrap_err().kind(),
+                std::io::ErrorKind::NotFound
+            );
+
+            symlink(target, &auth_file)?;
+            storage.compare_and_save(&first, &second)?;
+            assert_eq!(storage.load()?, Some(second));
+
+            symlink(target, &auth_file)?;
+            assert!(storage.delete()?);
+            assert_eq!(storage.load()?, None);
+            assert_eq!(
+                std::fs::symlink_metadata(&auth_file).unwrap_err().kind(),
+                std::io::ErrorKind::NotFound
+            );
+        }
+    }
+    Ok(())
+}
+
 #[test]
 fn deleting_absent_file_store_does_not_create_home() -> anyhow::Result<()> {
     let root = tempfile::tempdir()?;
@@ -97,3 +145,90 @@ fn commits_respect_another_process_lock() -> anyhow::Result<()> {
     assert_eq!(storage.load()?, Some(new));
     Ok(())
 }
+
+#[cfg(unix)]
+#[test]
+fn keyring_logout_coordinates_with_refresh_through_chained_symlinks() -> anyhow::Result<()> {
+    use codex_keyring_store::tests::MockKeyringStore;
+    use std::os::unix::fs::symlink;
+
+    const CHILD_HOME: &str = "CODEX_AUTH_CHAINED_REFRESH_TEST_HOME";
+    let old: AuthDotJson = serde_json::from_value(serde_json::json!({"OPENAI_API_KEY": "old"}))?;
+    let new: AuthDotJson = serde_json::from_value(serde_json::json!({"OPENAI_API_KEY": "new"}))?;
+    if let Some(home) = std::env::var_os(CHILD_HOME) {
+        let home = PathBuf::from(home);
+        let storage = CommittedStorage {
+            inner: Arc::new(FileAuthStorage::new(home.clone())),
+            home,
+            mode: AuthCredentialsStoreMode::File,
+        };
+        let _write_guard = storage.lock_for_write()?;
+        let _commit_guard = storage.lock()?;
+        // Pause between validation and save under compare_and_save's commit lock.
+        assert_eq!(storage.inner.load_account_grant()?, Some(old));
+        println!("REFRESH_VALIDATED");
+        let mut release = String::new();
+        std::io::stdin().read_to_string(&mut release)?;
+        storage.inner.save(&new)?;
+        return Ok(());
+    }
+
+    for kind in [
+        AuthKeyringBackendKind::Direct,
+        AuthKeyringBackendKind::Secrets,
+    ] {
+        let shared = tempfile::tempdir()?;
+        let keyring_home = tempfile::tempdir()?;
+        let writer_home = tempfile::tempdir()?;
+        let shared_storage = FileAuthStorage::new(shared.path().to_path_buf());
+        shared_storage.save(&old)?;
+        let middle = get_auth_file(keyring_home.path());
+        symlink(get_auth_file(shared.path()), &middle)?;
+        symlink(&middle, get_auth_file(writer_home.path()))?;
+        let storage = create_auth_storage_with_store(
+            keyring_home.path().to_path_buf(),
+            AuthCredentialsStoreMode::Keyring,
+            Arc::new(MockKeyringStore::default()),
+            kind,
+        );
+        let _guard = storage.lock_for_write()?;
+        let mut child = codex_utils_process::background_command(std::env::current_exe()?)
+            .args([
+                "--exact",
+                "auth::storage::storage_commit::tests::keyring_logout_coordinates_with_refresh_through_chained_symlinks",
+                "--nocapture",
+            ])
+            .env(CHILD_HOME, writer_home.path())
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .spawn()?;
+        let input = child.stdin.take();
+        let mut output = std::io::BufReader::new(child.stdout.take().unwrap());
+        loop {
+            let mut line = String::new();
+            anyhow::ensure!(
+                output.read_line(&mut line)? > 0,
+                "refresh exited before validating credentials"
+            );
+            if line.contains("REFRESH_VALIDATED") {
+                break;
+            }
+        }
+        let result = storage.delete();
+        drop(input);
+        let status = child.wait()?;
+        assert!(status.success());
+        assert_eq!(result.unwrap_err().kind(), std::io::ErrorKind::TimedOut);
+        assert!(std::fs::symlink_metadata(&middle)?.file_type().is_symlink());
+        assert_eq!(shared_storage.load()?, Some(new.clone()));
+
+        // Logout can unlink the middle symlink once the refresh has committed.
+        assert!(storage.delete()?);
+        assert_eq!(
+            std::fs::symlink_metadata(&middle).unwrap_err().kind(),
+            std::io::ErrorKind::NotFound
+        );
+        assert_eq!(shared_storage.load()?, Some(new.clone()));
+    }
+    Ok(())
+}

codex-rs/login/src/auth/storage_policy_tests.rs

@@ -305,13 +305,11 @@ fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()>
     assert_eq!(storage.load()?, Some(encrypted.clone()));
     assert_eq!(storage.file_storage.load()?, Some(expected.clone()));
     mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into()));
-    assert!(
-        storage
-            .compare_and_save(&expected, &auth_with_prefix("must-not-save"))
-            .is_err()
-    );
+    let refreshed_fallback = auth_with_prefix("refreshed-fallback");
+    storage.compare_and_save(&expected, &refreshed_fallback)?;
+    // Recovery must not promote the fallback over an unread keyring generation.
     assert_eq!(storage.load()?, Some(encrypted));
-    assert_eq!(storage.file_storage.load()?, Some(expected));
+    assert_eq!(storage.file_storage.load()?, Some(refreshed_fallback));
 
     // A recovered keyring may contain a newer generation than the fallback.
     // Never overwrite it with a response obtained using the fallback token.
@@ -386,6 +384,59 @@ fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()>
     Ok(())
 }
 
+#[test]
+fn auto_auth_storage_refresh_preserves_fallback_when_keyring_stays_unreadable() -> anyhow::Result<()>
+{
+    let home = tempdir()?;
+    let keyring = MockKeyringStore::default();
+    let key = compute_store_key(home.path())?;
+    // A malformed entry keeps every keyring read failing, while writes would
+    // succeed. Reuse the keyring mock without a one-shot failure that recovers.
+    keyring.save(KEYRING_SERVICE, &key, "unreadable credential document")?;
+    let storage = create_auth_storage_with_store(
+        home.path().to_path_buf(),
+        AuthCredentialsStoreMode::Auto,
+        Arc::new(keyring.clone()),
+        AuthKeyringBackendKind::Direct,
+    );
+    let file = FileAuthStorage::new(home.path().to_path_buf());
+    let mut previous = auth_with_prefix("fallback");
+    previous.auth_mode = Some(AuthMode::Chatgpt);
+    previous.openai_api_key = None;
+    file.save(&previous)?;
+    let previous_tokens = previous.tokens.as_ref().context("fallback tokens")?;
+    let refreshed = crate::auth::manager::persist_tokens(
+        &storage,
+        previous_tokens,
+        /*provenance*/ None,
+        /*id_token*/ None,
+        Some("rotated-access".into()),
+        Some("rotated-refresh".into()),
+    )?;
+    assert_eq!(
+        refreshed.tokens,
+        Some(TokenData {
+            access_token: "rotated-access".into(),
+            refresh_token: "rotated-refresh".into(),
+            ..previous_tokens.clone()
+        })
+    );
+    assert_eq!(storage.load()?, Some(refreshed.clone()));
+    assert_eq!(file.load()?, Some(refreshed.clone()));
+
+    // Fallback commits must still reject stale generations and logout.
+    assert!(storage.compare_and_save(&previous, &refreshed).is_err());
+    assert_eq!(file.load()?, Some(refreshed.clone()));
+    file.delete()?;
+    assert!(storage.compare_and_save(&refreshed, &previous).is_err());
+    assert_eq!(file.load()?, None);
+    assert_eq!(
+        keyring.saved_value(&key),
+        Some("unreadable credential document".into())
+    );
+    Ok(())
+}
+
 #[test]
 fn auto_auth_storage_save_prefers_keyring() -> anyhow::Result<()> {
     let codex_home = tempdir()?;