fix: serialize lastUsedIdentity to avoid EXC_BAD_ACCESS under concurrent async callers - #107
Open
zerotomas wants to merge 1 commit into
Open
Conversation
…ent async callers
`getFeatureFlags(forIdentity:...)` writes `lastUsedIdentity = identity` on
its first line, before dispatching the actual request. That write runs on
whatever executor the caller was on — for the async overloads in
`Flagsmith+Concurrency.swift`, that's whichever cooperative-pool thread
the awaited continuation happens to resume on.
When two `Task { await Flagsmith.shared.hasFeatureFlag(...) }` fire on
different cooperative threads, both hit the unsynchronized write. Under
concurrent writes, the refcounted heap storage of the `String?` can be
stomped, and the next reader dereferences a corrupted strong reference —
`EXC_BAD_ACCESS` with a small integer-looking address, inside the SDK
frame.
Back `lastUsedIdentity` with a serialized accessor through the existing
`apiManager.propertiesSerialAccessQueue`, matching the pattern used by
`_defaultFlags` and `_cacheConfig` immediately below in the same file
(and by every ivar on `APIManager`). No API change; no behavior change
for single-threaded callers.
zerotomas
requested review from
talissoncosta
and removed request for
a team
September 10, 2026 02:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Flagsmith.getFeatureFlags(forIdentity:...)writeslastUsedIdentity = identityon its first line, before any dispatch:https://github.com/Flagsmith/flagsmith-ios-client/blob/bb8564b/FlagsmithClient/Classes/Flagsmith.swift#L131
That write runs on whatever executor the caller was on. For the async overloads in
Flagsmith+Concurrency.swift, that's whichever cooperative-pool thread the awaited continuation happens to resume on — the async bridge (withCheckedThrowingContinuation { ... hasFeatureFlag(...) { ... } }) doesn't hop to any specific queue before calling into the completion-based API.When two
Task { await Flagsmith.shared.hasFeatureFlag(...) }fire on different cooperative threads, both hit the unsynchronized write concurrently. Under concurrent writes to aString?, the refcounted heap storage can be stomped, and the next reader dereferences a corrupted strong reference —EXC_BAD_ACCESSat a small integer-looking address, inside the SDK frame.We observed this in production:
EXC_BAD_ACCESS (code=1, address=0x59…)insideFlagsmith.hasFeatureFlag(withID:), on acom.apple.root.user-initiated-qos.cooperativetask, called from app code that fires several flag reads back-to-back on user-initiated actions.Fix
Back
lastUsedIdentitywith a serialized accessor through the existingapiManager.propertiesSerialAccessQueue, matching the pattern used by_defaultFlagsand_cacheConfigimmediately below in the same file (added in #45), and by every ivar onAPIManageritself.Scope
I deliberately kept this PR focused on
lastUsedIdentitybecause that's the ivar tied to the observed crash. Three other ivars onFlagsmith—lastUpdatedAt,lastFlags, andanyFlagStreamContinuation— are also declared without synchronization. In practice, bothSSEManagerandAPIManagerroute their completions throughOperationQueue.main/DispatchQueue.main.async, so the SDK-internal write paths for those three land on Main. They're theoretically racy (e.g.flagStreammaterialization on a background executor writinganyFlagStreamContinuationwhile a Main-dispatched update reads it), but I didn't observe crashes tied to them and I didn't want to broaden a bugfix PR into a defense-in-depth cleanup you haven't asked for.Happy to extend the same treatment to those three in this PR or a follow-up, whichever you prefer.
Testing
swift test— all 79 tests pass, 8 skipped (unchanged frommain).main(the file has pre-existing warnings unrelated to this diff).I did not add a test that deterministically reproduces the crash. Race reproduction is timing-sensitive, and given #47 already had to tone down
APIManagerTests.testConcurrentRequestsfor CI stability, I don't want to introduce a flaky test. If you'd like a targeted concurrency test in the shape oftestConcurrentRequests(i.e. best-effort, not deterministic), I'm happy to add one — let me know.Type of Change