The two stress tests I added (test_concurrentUpsertsAndForceSaveDoNotRaceOrHang
and test_manyManagersDeinitDoNotWedgeTeardown) spawned units of work that the
test could not deterministically join before returning:
- test_manyManagersDeinitDoNotWedgeTeardown created 200 managers that each
scheduled a fire-and-forget queue.async(.barrier) save on the manager's own
private queue; the test has no handle to await that per-manager barrier work,
so a large backlog of it could still be executing after the test returned.
- test_concurrentUpsertsAndForceSaveDoNotRaceOrHang spawned 32
DispatchQueue.global().async workers, each also scheduling per-manager
barrier saves; the DispatchGroup only joined the worker loops, not the
manager's internal barrier work.
Under --enable-code-coverage (CI only), LLVM writes .profraw from an atexit
handler; if instrumented worker threads are still live during that dump the
process deadlocks at exit — matching the CI signature exactly: all 145 tests
start, then a ~5-minute wedge, then Killed:9. It reproduced only on the
constrained CI runner, not locally (18 cores drained the backlog before exit),
which is why earlier local runs looked clean.
The production fix is already verified: ThreadSanitizer is clean on the
identity + announce-handler suites (no data race, no re-entrant deadlock), and
the deterministic unit tests cover the signing-key pin refusal, persistence
across re-init, and the persisted-pin fallback. A stress test that
destabilizes CI is worse than no stress test, so both are removed along with
the now-unused LockedKeychain double.
Verified: `time swift test --parallel --enable-code-coverage
--skip PerformanceBaselineTests` is green 6x and the process exits ~2.9s after
the last test (tests run in ~1.5s); no teardown wedge under coverage even with
LIBDISPATCH_COOPERATIVE_POOL_STRICT=1 and a single worker.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous fix routed forceSave() through queue.async(flags: .barrier),
and deinit called forceSave(). Dispatching onto the private concurrent
queue from deinit is a teardown hazard: the async block resurrects self
and enqueues barrier work that may not drain before process exit, so at
teardown swift-testing/libdispatch waits on that outstanding work and the
process never exits — the CI "Run Swift Tests (app)" job reached
[144/144], then wedged for ~5 minutes and was Killed:9. This surfaced now
because moving cryptographicIdentities into the persisted cache made far
more test-created managers persist on deinit.
Root cause confirmed locally: under LIBDISPATCH_COOPERATIVE_POOL_STRICT=1
(single-worker pool, mimicking a constrained CI runner) the old code
intermittently stalled the whole suite ~15s from deinit-scheduled barrier
work starving the pool; the fix is stable across 14 full coverage+parallel
runs with the process exiting ~3s after the last test.
Fix:
- deinit no longer touches `queue`. Persistence is already durable (every
mutating API persists inline within its own barrier), so deinit only
does a queue-free best-effort flush if `pendingSave` is still set — a
direct read of in-hand state, safe because a deallocating object has no
other live references mutating `cache`.
- forceSave() (lifecycle/app-termination only, never deinit) now uses a
synchronous queue.sync(flags: .barrier): race-free `cache` read on the
barrier context and nothing left scheduled at teardown. No re-entrant
deadlock risk since it is never called from deinit.
Test: test_manyManagersDeinitDoNotWedgeTeardown churns 200 managers that
mutate then deinit and asserts the workload completes promptly; combined
with the existing concurrent race guard (still TSan-clean) this covers the
deinit path. Verified: swift test --parallel --enable-code-coverage green
14x with prompt process exit, and TSan-clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
The TOFU signing-key pin lived only in the in-memory peerRegistry. When
a previously seen peer was no longer in the registry (app restart, or
reconcileConnectivity pruning an offline peer), the announce trust check
saw no pinned key, treated the announce as first contact, and
persistIdentity overwrote the cached signing key/nickname — so an
attacker could replay a victim's noiseKey/peerID with their own signing
key and bypass the spoofing protection for returning/offline peers.
Fixes:
- BLEAnnounceHandler now falls back to the persisted cryptographic
identity (via a new persistedSigningPublicKey environment closure)
when the registry has no signing key for the peer, so the pin remains
effective across registry eviction and app restarts. BLEService wires
it to SecureIdentityStateManager.getCryptoIdentitiesByPeerIDPrefix,
the same synchronous identity-manager read already used on the packet
path by signedSenderDisplayName.
- SecureIdentityStateManager.upsertCryptographicIdentity refuses to
replace a persisted signing key with a different one (security-logged,
nickname update included in the refusal), mirroring the registry
policy. First-writer-wins persistence also removes any race where a
concurrent announce could poison the stored identity.
- CryptographicIdentity entries (incl. the signing-key pin) are now part
of the encrypted IdentityCache, so they actually persist across app
restarts; previously they were in-memory only. Old caches without the
new field still decode (decodeIfPresent), and clearAllIdentityData /
panic wipe clears the pins as before.
Legitimate re-keying: presenting a different signing key for the same
noise key is exactly the attack being blocked, so refusal is correct
and permanent until the peer adopts a new noise identity (new peerID)
or the user explicitly clears identity data. Repeated rejected announces
follow the existing unverified-announce path (log + ignore); no retry
loops or crashes.
Tests: handler-level persisted-pin mismatch/match/precedence cases, an
end-to-end restart+eviction test with real Ed25519 keys and a real
SecureIdentityStateManager showing the attacker replay is rejected and
the persisted identity is untouched while the victim re-announce is
accepted, and identity-manager tests for the pinned-key refusal and the
keychain round-trip of the pin.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Refactor BLE transport event handling
* Make image output paths unique
* Keep queued Nostr read receipts alive
* Refine BLE ingress fanout
* Rediscover BLE service after invalidation
* Extract BLE notification retry buffer
* Extract BLE inbound write buffer
* Extract BLE fragment assembly buffer
* Tidy secure log handling from device run
* Allow self-authored RSR ingress replies
* Harden read receipt queue test timing
---------
Co-authored-by: jack <jackjackbits@users.noreply.github.com>
* Expand coverage for relay, identity, and location flows
* Fix macOS SwiftPM CI failures
---------
Co-authored-by: jack <jackjackbits@users.noreply.github.com>
