test(sign): serialize the Keychain search-list tests (#4124)

* test(sign): serialize the Keychain search-list tests

Both ignored Keychain tests save the user's keychain search list, change it
and restore what they saved. Run together (`--ignored` runs them in
parallel), one could restore its list while the other needed its temporary
keychain, or put back a keychain the other had already deleted. A shared
lock now runs them one at a time.

With the lock, the issuer test still failed about one run in ten when it
came second: the Security framework caches the search list in-process and
learns of `security list-keychains` asynchronously, so `keychain::find` did
not yet look in the new keychain. The test now waits up to five seconds for
the identity to appear.

Follows up a CodeRabbit review comment on #3716.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test(sign): restore the keychain search list before deleting the keychain

If deleting the temporary keychain panicked, the test left it on the user's
search list, and the next Keychain test (which accepts the poisoned lock)
saved that list and later restored it. Restore the list first.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test(sign): fail when the keychain search list isn't restored

Both Keychain tests ignored the result of the `security list-keychains` that
restores the user's search list, so a failed restore went unnoticed and left
the deleted temporary keychain on the list. They now check it after deleting
the keychain.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test(sign): keep the issuer test's keychain at one path

trustd remembers which keychain file it found an issuer in. Each run of
`keychain_identities_carry_their_issuers` created its keychain at a path with
the process id and deleted it afterwards, so the next run's trust evaluation
kept looking in the deleted file and built an empty chain, until trustd
forgot it. The test failed every run after the first for a while.

The keychain now lives at the same path every run, and a copy left by a
killed run is deleted first.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: ffishman <20211677+ffishman@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

crates/sign/tests/it/pdf.rs

@@ -665,12 +665,20 @@ trailer << /Root 1 0 R >>
     }
 }
 
+/// Held by every test that changes the user's keychain search list. Each saves the list, changes
+/// it and restores what it saved, so two at once could undo each other's temporary keychain or
+/// put back one already deleted.
+#[cfg(target_os = "macos")]
+static KEYCHAIN_SEARCH_LIST: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
 /// Signing with Keychain identities, in a throwaway keychain file. Ignored by default: creating
 /// a keychain touches the user's keychain search list (restored afterwards).
 #[cfg(target_os = "macos")]
 #[test]
 #[ignore = "creates a temporary macOS keychain; run with --ignored"]
 fn signing_with_keychain_identities() {
+    // A test that failed while holding it leaves the list restored, so a poisoned lock is fine.
+    let _list = KEYCHAIN_SEARCH_LIST.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
     use std::process::Command;
     let dir = std::env::temp_dir().join(format!("pdfcraft-keychain-{}", std::process::id()));
     std::fs::create_dir_all(&dir).unwrap();
@@ -683,7 +691,7 @@ fn signing_with_keychain_identities() {
     let restore = || {
         let mut args = vec!["list-keychains", "-d", "user", "-s"];
         args.extend(saved.iter().map(String::as_str));
-        sec(&args);
+        sec(&args)
     };
     restore();
     // Everything that can fail runs before the clean-up below.
@@ -705,9 +713,11 @@ fn signing_with_keychain_identities() {
             assert_eq!(s.status, Status::Valid, "{:?}", s.details);
         }
     }));
+    // The list first: it is what outlives the test if deleting the keychain goes wrong.
+    let restored = restore();
     sec(&["delete-keychain", k]);
-    restore();
     let _ = std::fs::remove_dir_all(&dir);
+    assert!(restored.status.success(), "the keychain search list wasn't restored: {}", String::from_utf8_lossy(&restored.stderr));
     result.unwrap();
 }
 
@@ -718,20 +728,28 @@ fn signing_with_keychain_identities() {
 #[test]
 #[ignore = "creates a temporary macOS keychain; run with --ignored"]
 fn keychain_identities_carry_their_issuers() {
+    // A test that failed while holding it leaves the list restored, so a poisoned lock is fine.
+    let _list = KEYCHAIN_SEARCH_LIST.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
     use std::process::Command;
-    let dir = std::env::temp_dir().join(format!("pdfcraft-keychain-chain-{}", std::process::id()));
-    std::fs::create_dir_all(&dir).unwrap();
+    // The same path every run: trustd remembers which keychain file it found an issuer in, and
+    // after a run's keychain is deleted it keeps looking there, so a keychain at a new path
+    // would get an empty chain until that memory expires.
+    let dir = std::env::temp_dir().join("pdfcraft-keychain-chain");
     let kc = dir.join("test.keychain-db");
     let sec = |args: &[&str]| Command::new("security").args(args).output().unwrap();
+    let k = kc.to_str().unwrap();
+    // Left over by a run that was killed before its clean-up.
+    sec(&["delete-keychain", k]);
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
     let list = String::from_utf8(sec(&["list-keychains", "-d", "user"]).stdout).unwrap();
     let saved: Vec<String> = list.lines().map(|l| l.trim().trim_matches('"').to_string()).filter(|l| !l.is_empty()).collect();
-    let k = kc.to_str().unwrap();
     assert!(sec(&["create-keychain", "-p", "pc-test", k]).status.success());
     let set_list = |extra: Option<&str>| {
         let mut args = vec!["list-keychains", "-d", "user", "-s"];
         args.extend(saved.iter().map(String::as_str));
         args.extend(extra);
-        sec(&args);
+        sec(&args)
     };
     // macOS finds an identity's issuers in the keychains on the search list.
     set_list(Some(k));
@@ -742,15 +760,29 @@ fn keychain_identities_carry_their_issuers() {
         assert!(out.status.success(), "chain-legacy.p12: {}", String::from_utf8_lossy(&out.stderr));
         let listed = pdfcraft_sign::keychain::identities(Some(&kc)).unwrap();
         assert_eq!(listed.len(), 1, "{listed:?}");
-        let id = pdfcraft_sign::keychain::find(&pdfcraft_sign::keychain::reference(&listed[0].certificate)).unwrap();
+        // The Security framework caches the search list in-process and learns of `security
+        // list-keychains` asynchronously, so after another Keychain test it takes a moment to look
+        // in the new keychain.
+        let reference = pdfcraft_sign::keychain::reference(&listed[0].certificate);
+        let mut found = pdfcraft_sign::keychain::find(&reference);
+        for _ in 0..50 {
+            if found.is_ok() {
+                break;
+            }
+            std::thread::sleep(std::time::Duration::from_millis(100));
+            found = pdfcraft_sign::keychain::find(&reference);
+        }
+        let id = found.unwrap();
         let issuers: Vec<_> = id.chain.iter().map(|c| c.subject.common_name()).collect();
         assert_eq!(issuers, [Some("PrintCraft Test Root CA")]);
         let file = pkcs12::open(&data("chain.p12"), "test").unwrap();
         assert_eq!(id.chain.iter().map(|c| &c.raw).collect::<Vec<_>>(), file.chain.iter().map(|c| &c.raw).collect::<Vec<_>>());
     }));
+    // The list first: it is what outlives the test if deleting the keychain goes wrong.
+    let restored = set_list(None);
     sec(&["delete-keychain", k]);
-    set_list(None);
     let _ = std::fs::remove_dir_all(&dir);
+    assert!(restored.status.success(), "the keychain search list wasn't restored: {}", String::from_utf8_lossy(&restored.stderr));
     result.unwrap();
 }