Replace the old dispatcher+queue IdentityManager with a Actor#387
Replace the old dispatcher+queue IdentityManager with a Actor#387
Conversation
that serializes all identity mutations through a single mutex.
- IdentityState: immutable data class with pure reducers (Updates) and
async side-effecting actions (Actions) via IdentityContext
- IdentityPersistenceInterceptor: diff-based storage writes on state change
- SdkContext: cross-slice bridge so identity can call ConfigManager
without a direct dependency
- Reset flow gates identity readiness during cleanup, matching iOS
- mergeAttributes aligned with iOS (no null filtering in nested collections)
- Startup repair widened to handle empty stored attributes
- Tracking/notify paths use enrichedAttributes consistently
Includes pure reducer unit tests and SequentialActor integration tests
covering concurrency, rapid identify/reset interleaving, and persistence.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| override val sdkContext: SdkContext, | ||
| ) : IdentityContext { | ||
| override val scope: CoroutineScope get() = ioScope | ||
| override val track: suspend (Trackable) -> Unit = { trackEvent(it as TrackableSuperwallEvent) } |
There was a problem hiding this comment.
Trackable contract
The property is declared as suspend (Trackable) -> Unit in IdentityContext, but the implementation forcibly casts every Trackable to TrackableSuperwallEvent. All current callers in IdentityManagerActor.kt happen to pass InternalSuperwallEvent (which extends TrackableSuperwallEvent), so this works today — but if any future action dispatches a Trackable that isn't a TrackableSuperwallEvent, this will throw a ClassCastException at runtime with no compile-time warning.
The safest fix is to narrow the interface property type so the contract matches the implementation:
| override val track: suspend (Trackable) -> Unit = { trackEvent(it as TrackableSuperwallEvent) } | |
| override val track: suspend (TrackableSuperwallEvent) -> Unit = { trackEvent(it) } |
Alternatively, update IdentityContext.track to suspend (TrackableSuperwallEvent) -> Unit consistently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/identity/IdentityManager.kt
Line: 46
Comment:
**Unsafe cast violates `Trackable` contract**
The property is declared as `suspend (Trackable) -> Unit` in `IdentityContext`, but the implementation forcibly casts every `Trackable` to `TrackableSuperwallEvent`. All current callers in `IdentityManagerActor.kt` happen to pass `InternalSuperwallEvent` (which extends `TrackableSuperwallEvent`), so this works today — but if any future action dispatches a `Trackable` that isn't a `TrackableSuperwallEvent`, this will throw a `ClassCastException` at runtime with no compile-time warning.
The safest fix is to narrow the interface property type so the contract matches the implementation:
```suggestion
override val track: suspend (TrackableSuperwallEvent) -> Unit = { trackEvent(it) }
```
Alternatively, update `IdentityContext.track` to `suspend (TrackableSuperwallEvent) -> Unit` consistently.
How can I resolve this? If you propose a fix, please make it concise.| withTimeout(5000) { manager.hasIdentity.first() } | ||
| manager.identify("user-1") | ||
| // Wait for identify to complete | ||
| Thread.sleep(200) |
There was a problem hiding this comment.
Thread.sleep in runTest blocks creates flaky tests
Several tests use Thread.sleep(200) / Thread.sleep(500) inside runTest to wait for fire-and-forget effect actions to process through the SequentialActor. This pattern is timing-dependent and can produce false positives on slow CI machines or under load (e.g. the 200 ms budget may not be enough for the full mutex queue to drain).
The same pattern appears at lines 222, 295, 309, 312, and 357.
A more deterministic approach would be to expose a test hook on SequentialActor (e.g. suspend fun awaitIdle() that waits until the internal queue is empty), or to make the integration-test actor configurable with UnconfinedTestDispatcher so advanceUntilIdle() from kotlinx.coroutines.test can drain all pending work without real-time sleeps.
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/test/java/com/superwall/sdk/identity/IdentityActorIntegrationTest.kt
Line: 181
Comment:
**`Thread.sleep` in `runTest` blocks creates flaky tests**
Several tests use `Thread.sleep(200)` / `Thread.sleep(500)` inside `runTest` to wait for fire-and-forget `effect` actions to process through the `SequentialActor`. This pattern is timing-dependent and can produce false positives on slow CI machines or under load (e.g. the 200 ms budget may not be enough for the full mutex queue to drain).
The same pattern appears at lines 222, 295, 309, 312, and 357.
A more deterministic approach would be to expose a test hook on `SequentialActor` (e.g. `suspend fun awaitIdle()` that waits until the internal queue is empty), or to make the integration-test actor configurable with `UnconfinedTestDispatcher` so `advanceUntilIdle()` from `kotlinx.coroutines.test` can drain all pending work without real-time sleeps.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // If switching users, reset other managers BEFORE updating state | ||
| // so storage.reset() doesn't wipe the new IDs | ||
| if (wasLoggedIn) { | ||
| completeReset() | ||
| immediate(Reset) |
There was a problem hiding this comment.
storage.reset() wiping identity fields
The comment says "reset other managers BEFORE updating state so storage.reset() doesn't wipe the new IDs". However, LocalStorage.reset() only clears confirmedAssignments and didTrackFirstSeen — it does not touch AliasId, AppUserId, Seed, or UserAttributes. The ordering concern described in the comment does not apply.
Contrast this with FullReset, which calls update(Updates.Reset) first (writing new IDs via the persistence interceptor) and then calls completeReset(). The inconsistent ordering between the two code paths is harmless in practice, but the misleading comment could cause future contributors to draw incorrect conclusions when modifying either flow.
Consider updating the comment to accurately describe why completeReset() is called before the state update (e.g. to ensure other managers are torn down before the new identity takes effect).
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/identity/IdentityManagerActor.kt
Line: 232-237
Comment:
**Misleading comment about `storage.reset()` wiping identity fields**
The comment says "reset other managers BEFORE updating state so storage.reset() doesn't wipe the new IDs". However, `LocalStorage.reset()` only clears `confirmedAssignments` and `didTrackFirstSeen` — it does **not** touch `AliasId`, `AppUserId`, `Seed`, or `UserAttributes`. The ordering concern described in the comment does not apply.
Contrast this with `FullReset`, which calls `update(Updates.Reset)` first (writing new IDs via the persistence interceptor) and then calls `completeReset()`. The inconsistent ordering between the two code paths is harmless in practice, but the misleading comment could cause future contributors to draw incorrect conclusions when modifying either flow.
Consider updating the comment to accurately describe *why* `completeReset()` is called before the state update (e.g. to ensure other managers are torn down before the new identity takes effect).
How can I resolve this? If you propose a fix, please make it concise.1 participant
IdentityState: immutable data class with pure reducers (Updates) and async side-effecting actions (Actions) via IdentityContext
Includes pure reducer unit tests and SequentialActor integration tests
covering concurrency, rapid identify/reset interleaving, and persistence.
Changes in this pull request
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.ktlintin the main directory and fixed any issues.Greptile Summary
This PR replaces the old single-threaded-executor +
runBlockingIdentityManagerwith a clean actor-model implementation: an immutableIdentityStatedata class, pureUpdatesreducers, asyncActions, aSequentialActorbacked by aMutex, and a diff-basedIdentityPersistenceInterceptor. The reset flow is now gated on identity readiness (matching iOS), andmergeAttributesdrops null-filtering from nested collections for iOS parity.Key changes:
IdentityState+ sealedUpdates/Actionsreplace all mutable fields and ad-hoc coroutine launches in the old managerSequentialActorwith re-entrant mutex serializes all identity transitions without blocking threadsIdentityPersistenceInterceptorperforms diff-based storage writes, eliminating redundant I/OSdkContextbridge decouples the identity slice fromConfigManagerdirectlyFullResetgates paywall presentation (Pending → Ready) around the cleanup windowSequentialActorintegration tests for concurrency and persistenceIssues found:
IdentityManager.ktline 46:override val track: suspend (Trackable) -> Unit = { trackEvent(it as TrackableSuperwallEvent) }— the interface declaresTrackablebut the implementation hard-casts toTrackableSuperwallEvent. This will throwClassCastExceptionat runtime if the contract is ever exercised with a plainTrackable.Thread.sleep()insiderunTestto wait for fire-and-forgeteffectactions; this creates timing-sensitive tests that can flake on slow CI.Identify's user-switch branch ("sostorage.reset()doesn't wipe the new IDs") is factually incorrect —LocalStorage.reset()only clearsconfirmedAssignments/didTrackFirstSeen, not identity fields.Confidence Score: 4/5
Trackablecast inIdentityManager; all other findings are non-blocking style improvements.it as TrackableSuperwallEventcast inIdentityManager.tracksilently violates theTrackablecontract and will throw at runtime if the abstraction is exercised as declared. All current callers passInternalSuperwallEvent(aTrackableSuperwallEvent) so it works today, but it is a latent bug one refactor away from surfacing. The P2 findings (flaky sleep-based tests, misleading comment) are clean-up items that don't block merge.superwall/src/main/java/com/superwall/sdk/identity/IdentityManager.kt— the unsafe cast on line 46 needs to be resolved before merge.Important Files Changed
IdentityState(immutable data class), pureUpdatesreducers, and asyncActions. Well-structured; the misleading comment inIdentify's user-switch branch aboutstorage.reset()wiping identity fields is the only notable issue.override val track: suspend (Trackable) -> Unit = { trackEvent(it as TrackableSuperwallEvent) }— will throwClassCastExceptionif a non-TrackableSuperwallEventTrackableis ever dispatched.SequentialActorserializes all actions via aMutexwith correct re-entrancy detection usingCoroutineContextelements. Clean and correct implementation.immediateUntilusesStateFlow.firstafter firing the action — safe sinceStateFlow.firstchecks the current value before suspending.AppUserIdnull (delete vs write). Works safely underSequentialActor's mutex since no concurrent updates can occur between readingbeforeandafter.identityManager.reset()), whileduringIdentify=trueperforms only the non-identity cleanup. Correctly gates paywall presentation during the reset window, matching iOS behavior.mergeAttributes— removed null-filtering from nestedList/Mapvalues to align with iOS. Intentional behavior change; consumers must tolerate nulls in nested collections.Thread.sleep()insiderunTestblocks for timing synchronization makes tests fragile on slow CI.Sequence Diagram
sequenceDiagram participant Caller participant IdentityManager participant SequentialActor participant Action participant PersistenceInterceptor participant Storage participant SdkContext Note over SequentialActor: Mutex serializes all actions Caller->>IdentityManager: configure(neverCalledStaticConfig) IdentityManager->>SequentialActor: effect(Actions.Configure) SequentialActor->>Action: executeAction (mutex acquired) Action->>PersistenceInterceptor: update(Updates.Configure) PersistenceInterceptor->>Storage: write only changed fields Action->>SdkContext: fetchAssignments() [if needed] Action->>PersistenceInterceptor: update(Updates.AssignmentsCompleted) Note over SequentialActor: phase → Ready Caller->>IdentityManager: identify(userId) IdentityManager->>SequentialActor: effect(Actions.Identify) SequentialActor->>Action: executeAction (waits for mutex) alt switching users Action->>Caller: completeReset() [storage/config/paywall cleanup] Action->>SequentialActor: immediate(Reset) [re-entrant, skips mutex] end Action->>PersistenceInterceptor: update(Updates.Identify) PersistenceInterceptor->>Storage: write appUserId, attributes Action->>SequentialActor: immediate(IdentityChanged) [re-entrant] SequentialActor->>Action: effect(ResolveSeed) [queued, fire-and-forget] SequentialActor->>Action: effect(FetchAssignments) [queued] Caller->>IdentityManager: reset() IdentityManager->>SequentialActor: effect(Actions.FullReset) SequentialActor->>Action: executeAction (waits for mutex) Action->>PersistenceInterceptor: update(Updates.Reset) [phase=Pending, new aliasId/seed] PersistenceInterceptor->>Storage: write aliasId, seed, clear appUserId Action->>Caller: completeReset() [storage/config/paywall cleanup] Action->>PersistenceInterceptor: update(Updates.ResetComplete) Note over SequentialActor: phase → ReadyPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Replace the old dispatcher+queue Identit..." | Re-trigger Greptile