Fix identity-cache save race and deinit deadlock in forceSave

The previous commit moved cryptographicIdentities into the persisted
IdentityCache, which made the pre-existing off-queue cache access in
the save path fatal instead of merely racy: forceSave() -> performSave()
read and JSON-encoded `cache` on the caller's thread while a concurrent
`queue.async(.barrier)` writer mutated the same dictionary. ThreadSanitizer
flags this as a data race in saveIdentityCache(), and because JSONEncoder
walks the dictionary storage, an interleaved mutation can spin forever —
which is what hung the CI "Run Swift Tests (app)" job (killed at the
watchdog timeout).

The naive fix (snapshot `cache` under `queue.sync` in forceSave) instead
introduced a deadlock: forceSave() is reachable from deinit, and the
object's final release can run *on* the identity queue (the fire-and-forget
barrier saves capture self), so queue.sync there is a re-entrant same-queue
wait -> SIGTRAP. That matched the intermittent crash the hang investigation
surfaced.

Fix:
- Split persistence into persist(snapshot:) which encodes/seals/writes a
  by-value IdentityCache snapshot, decoupled from reading `cache`.
- saveIdentityCache() (only ever called inside a barrier writer) passes
  `cache` directly — already serialized, race-free.
- forceSave() now snapshots + persists inside `queue.async(flags:.barrier)`
  (async, never sync): the read is on the barrier context so it never races
  an in-flight write, and being async it can't deadlock when invoked from
  deinit running on the queue. Durability is unaffected: every mutating API
  already persists inline within its own barrier, so forceSave is a
  belt-and-suspenders flush.

Test: test_concurrentUpsertsAndForceSaveDoNotRaceOrHang hammers concurrent
upserts interleaved with forceSave; it reproduces the data race under
`--sanitize=thread` on the old code (SecureIdentityStateManager.swift:250)
and passes cleanly with the fix. A lock-guarded LockedKeychain double is
used so the test exercises the manager's own cache race rather than the
non-thread-safe MockKeychain. Verified green over repeated
`swift test --parallel` runs both with and without --enable-code-coverage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

bitchat/Identity/SecureIdentityStateManager.swift

@@ -243,20 +243,26 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
     }
     
     /// Persists the cache. Always invoked on `queue` under a barrier (its callers
-    /// run inside `queue.async(.barrier)`), so it simply marks the cache dirty
-    /// and persists it on the same serialized context — no timer, nothing left
-    /// scheduled to keep the process alive.
+    /// run inside `queue.async(.barrier)`), so `cache` is read while serialized.
+    /// The encode + keychain write are done here (already on the exclusive
+    /// barrier context), so no separate hop is scheduled and nothing is left to
+    /// keep the process alive.
     private func saveIdentityCache() {
         pendingSave = true
-        performSave()
+        // On the barrier context already: snapshot is trivially consistent.
+        persist(snapshot: cache)
+        pendingSave = false
     }
 
-    /// Writes the cache to the keychain. Must run on `queue` with exclusive
-    /// (barrier) access.
-    private func performSave() {
-        guard pendingSave else { return }
-        pendingSave = false
-
+    /// Encodes, seals, and writes a *snapshot* of the cache to the keychain.
+    ///
+    /// Takes the cache by value so callers can capture a consistent snapshot
+    /// under `queue` and then encode without holding it. Reading `cache`
+    /// concurrently with a barrier writer would be a data race on the
+    /// dictionary storage, which — because `JSONEncoder` walks that storage —
+    /// can spin forever (observed as a CI test-suite hang), so the snapshot
+    /// must be taken on `queue`, never off it.
+    private func persist(snapshot: IdentityCache) {
         // Never persist under an ephemeral key — it would overwrite the real
         // cache with data the next launch cannot decrypt.
         guard !encryptionKeyIsEphemeral else {
@@ -265,7 +271,7 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
         }
 
         do {
-            let data = try JSONEncoder().encode(cache)
+            let data = try JSONEncoder().encode(snapshot)
             let sealedBox = try AES.GCM.seal(data, using: encryptionKey)
             let saved = keychain.saveIdentityKey(sealedBox.combined!, forKey: cacheKey)
             if saved {
@@ -276,14 +282,26 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
         }
     }
 
-    // Force immediate save (for app termination / lifecycle events). Mutations
-    // already persist synchronously via saveIdentityCache, so this is normally a
-    // no-op (performSave early-returns when nothing is pending). Runs directly on
-    // the caller's thread — deliberately NOT a `queue.sync(barrier)`, which is
-    // reachable from `deinit` and from async tests on the swift-concurrency
-    // cooperative pool where a blocking barrier-sync can starve/deadlock it.
+    // Force a flush (for app termination / lifecycle events). Every mutating
+    // API already persists inline inside its own barrier via
+    // `saveIdentityCache`, so by the time this is called the keychain is
+    // already up to date and this is normally a no-op; it exists as a
+    // belt-and-suspenders flush of any `pendingSave` left set.
+    //
+    // The snapshot + persist run inside a `queue.async(flags: .barrier)`:
+    // reading `cache` on the barrier context is race-free (a plain off-queue
+    // read races in-flight barrier writers — JSONEncoder walking a
+    // concurrently-mutated dictionary can spin forever, which surfaced as a CI
+    // hang). It is deliberately `async`, NOT `sync`: `forceSave` is reachable
+    // from `deinit`, and the final release can itself run *on* `queue` (the
+    // fire-and-forget saves capture `self`), so a `queue.sync` here would be a
+    // re-entrant same-queue wait and deadlock (SIGTRAP).
     func forceSave() {
-        performSave()
+        queue.async(flags: .barrier) { [weak self] in
+            guard let self, self.pendingSave else { return }
+            self.pendingSave = false
+            self.persist(snapshot: self.cache)
+        }
     }
     
     // MARK: - Social Identity Management

bitchatTests/Services/SecureIdentityStateManagerTests.swift

@@ -173,6 +173,55 @@ final class SecureIdentityStateManagerTests: XCTestCase {
         XCTAssertEqual(reloaded.getSocialIdentity(for: fingerprint)?.claimedNickname, "victim")
     }
 
+    func test_concurrentUpsertsAndForceSaveDoNotRaceOrHang() {
+        // Regression guard for a data race between a barrier writer mutating
+        // `cache` and `forceSave` encoding it off-queue: JSONEncoder walking a
+        // dictionary being concurrently mutated can spin forever (observed as
+        // a CI suite hang, killed at the watchdog timeout). forceSave must
+        // snapshot `cache` on `queue` before encoding.
+        //
+        // forceSave is funnelled through a single serial actor: the guard is
+        // for the manager's own `cache` race, and MockKeychain is not itself
+        // thread-safe (production forceSave is not called concurrently). The
+        // interleaving with the concurrent barrier writers is what matters.
+        let manager = SecureIdentityStateManager(LockedKeychain())
+        let noiseKeys = (0..<32).map { Data(repeating: UInt8($0), count: 32) }
+        let forceSaveQueue = DispatchQueue(label: "test.forceSave.serial")
+
+        let group = DispatchGroup()
+        for (index, noiseKey) in noiseKeys.enumerated() {
+            group.enter()
+            DispatchQueue.global().async {
+                let fingerprint = noiseKey.sha256Fingerprint()
+                for iteration in 0..<20 {
+                    manager.upsertCryptographicIdentity(
+                        fingerprint: fingerprint,
+                        noisePublicKey: noiseKey,
+                        signingPublicKey: Data(repeating: UInt8(index), count: 32),
+                        claimedNickname: "peer-\(index)-\(iteration)"
+                    )
+                    // Interleave a serialized off-queue save with the in-flight
+                    // barrier writes from all the other threads.
+                    forceSaveQueue.async { manager.forceSave() }
+                }
+                group.leave()
+            }
+        }
+
+        let completed = group.wait(timeout: .now() + 20)
+        XCTAssertEqual(completed, .success, "concurrent upsert/forceSave workload hung")
+        forceSaveQueue.sync {} // drain outstanding saves
+
+        // The pins landed and are readable (also fences pending barrier writes).
+        for (index, noiseKey) in noiseKeys.enumerated() {
+            let peerID = PeerID(publicKey: noiseKey)
+            XCTAssertEqual(
+                manager.getCryptoIdentitiesByPeerIDPrefix(peerID).first?.signingPublicKey,
+                Data(repeating: UInt8(index), count: 32)
+            )
+        }
+    }
+
     func test_setBlocked_clearsFavoriteState() async {
         let manager = SecureIdentityStateManager(MockKeychain())
         let fingerprint = String(repeating: "ab", count: 32)
@@ -523,6 +572,70 @@ final class SecureIdentityStateManagerTests: XCTestCase {
     }
 }
 
+/// Thread-safe in-memory keychain for the concurrent stress test. MockKeychain
+/// itself is not synchronized (production keychain access is serialized), so a
+/// dedicated lock-guarded double is used so the test exercises the manager's
+/// `cache` race rather than crashing on the double's own unsynchronized dict.
+private final class LockedKeychain: KeychainManagerProtocol {
+    private let lock = NSLock()
+    private var storage: [String: Data] = [:]
+    private var serviceStorage: [String: [String: Data]] = [:]
+
+    private func sync<T>(_ body: () -> T) -> T {
+        lock.lock(); defer { lock.unlock() }
+        return body()
+    }
+
+    func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool {
+        sync { storage[key] = keyData; return true }
+    }
+
+    func getIdentityKey(forKey key: String) -> Data? {
+        sync { storage[key] }
+    }
+
+    func deleteIdentityKey(forKey key: String) -> Bool {
+        sync { storage.removeValue(forKey: key); return true }
+    }
+
+    func deleteAllKeychainData() -> Bool {
+        sync { storage.removeAll(); serviceStorage.removeAll(); return true }
+    }
+
+    func secureClear(_ data: inout Data) { data = Data() }
+    func secureClear(_ string: inout String) { string = "" }
+
+    func verifyIdentityKeyExists() -> Bool {
+        sync { storage["identity_noiseStaticKey"] != nil }
+    }
+
+    func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult {
+        sync {
+            if let data = storage[key] { return .success(data) }
+            return .itemNotFound
+        }
+    }
+
+    func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult {
+        sync { storage[key] = keyData; return .success }
+    }
+
+    func save(key: String, data: Data, service: String, accessible: CFString?) {
+        sync {
+            if serviceStorage[service] == nil { serviceStorage[service] = [:] }
+            serviceStorage[service]?[key] = data
+        }
+    }
+
+    func load(key: String, service: String) -> Data? {
+        sync { serviceStorage[service]?[key] }
+    }
+
+    func delete(key: String, service: String) {
+        sync { serviceStorage[service]?.removeValue(forKey: key) }
+    }
+}
+
 private final class FailingCacheSaveKeychain: KeychainManagerProtocol {
     private var storage: [String: Data] = [:]
     private var serviceStorage: [String: [String: Data]] = [:]