fix: eliminate Circle DeFi isAccountOpen race that flaked unit tests by aminsato · Pull Request #4177 · vultisig/vultisig-android

aminsato · 2026-04-23T12:52:03Z

Summary

CircleDeFiPositionsViewModelTest > onCreateAccount shows a Success snackbar and saves the new address on success started failing on CI with a TimeoutCancellationException from the test's 5s real-time wait on isAccountOpen.
Root cause is a real race in CircleDeFiPositionsViewModel, not a test-only flake. Both loadCirclePositions() (triggered by setData) and onCreateAccount() hop to a hard-coded Dispatchers.IO; the test scheduler cannot order them. On the "no cached + no on-chain account" branch, loadCirclePositions unconditionally writes isAccountOpen = false. When that write lands after a racing onCreateAccount success, it stomps isAccountOpen back to false — both in the test and in production.
Fix: inject an IoDispatcher-qualified CoroutineDispatcher (defaults to Dispatchers.IO via a new Hilt module); replace every Dispatchers.IO usage in the ViewModel with it. Stop stomping isAccountOpen on the no-account branch — preserve current state so a concurrent create wins cleanly. Update the test to pass testDispatcher as the IO dispatcher, and drop the Dispatchers.Default.limitedParallelism(1) + real-time timeout hacks (everything now completes synchronously under one scheduler).

Note on the "Could not connect to maven central to look up the latest available version for io.ktor:ktor-client-mock:3.0.3" lines in the failing CI log: those are Lint's version-lookup warnings, emitted for ~10 unrelated libraries in the same run (rive, hilt, okhttp, timber, commons-compress, kotlinx-datetime, …). They are not the failure cause; the CI line just interleaved with the test failure log.

Test plan

./gradlew :app:testDebugUnitTest --tests CircleDeFiPositionsViewModelTest --rerun-tasks run 10× → 10/10 pass
./gradlew :app:testDebugUnitTest (full suite) → pass
./gradlew :app:testReleaseUnitTest (the CI task that was red) → pass
./gradlew :app:assembleDebug → builds (Hilt wiring compiles)
./gradlew ktfmtFormat → clean
CI green on this branch

Co-Authored-By: aminsato Amin.saradar@yahoo.com

Summary by CodeRabbit

Bug Fixes
- Fixed concurrent account state updates to prevent unintended resets during operations
Refactor
- Enhanced dispatcher injection pattern for improved code maintainability and testability
Tests
- Simplified test logic and removed unnecessary asynchronous wait mechanisms

…itTest loadCirclePositions hopped to a hard-coded Dispatchers.IO and, on the "no cached and no on-chain account" branch, unconditionally wrote isAccountOpen = false. A concurrent onCreateAccount — also on real IO — could win the race on fast environments and then have its true stomped back to false when loadCirclePositions' stale finding finally landed. On slower CI this order flipped deterministically enough to fail CircleDeFiPositionsViewModelTest via a 5s real-time timeout. Inject an IO CoroutineDispatcher so tests run every coroutine under one TestScheduler, and preserve isAccountOpen from current state on the no-account branch so a racing create is never overwritten. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>

coderabbitai · 2026-04-23T12:52:16Z

📝 Walkthrough

Walkthrough

This pull request refactors the CircleDeFiPositionsViewModel to inject a coroutine ioDispatcher instead of using hardcoded Dispatchers.IO, and updates corresponding tests to use the injected dispatcher and simplify snackbar assertion logic via MockK slot capture.

Changes

Cohort / File(s)	Summary
Dispatcher Injection `app/src/main/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModel.kt`	Added `@IoDispatcher` annotation for injected `ioDispatcher` parameter; replaced all `Dispatchers.IO` usages with `ioDispatcher` for IO-bound calls. Modified `loadCirclePositions` to preserve concurrent `isAccountOpen` updates by removing the forced `isAccountOpen = false` assignment when no account is found.
Test Refactoring `app/src/test/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModelTest.kt`	Replaced async snackbar waiting logic via `awaitSnackbarAfter` with direct `SnackbarType` capture using MockK `slot`; removed post-snackbar synchronization step (`awaitAccountOpen`); updated ViewModel construction to inject `ioDispatcher = testDispatcher` for test consistency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A dispatcher now flows from injection's spring,
No hardcoded paths—just the threads we bring!
Tests whisper softly through MockK's embrace,
Slots capture truth at their rightful place. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main fix: eliminating a race condition in Circle DeFi that caused flaky unit tests, which is the core objective of the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch aminsato/fix_circle_defi_race_flake

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The duplicate CoroutineDispatchersModule in app/di redeclared a qualifier and provider that MainDataModule already supplies for the whole SingletonComponent. Delete it and import com.vultisig.wallet.data.IoDispatcher instead. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

app/src/main/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModel.kt (1)
214-230: ⚠️ Potential issue | 🟠 Major

Use safeLaunch for onCreateAccount() and rethrow CancellationException in fetchAssociatedMscaAccount().

runCatching in onCreateAccount() and catch (t: Throwable) in fetchAssociatedMscaAccount() both swallow CancellationException. If the ViewModel is cleared mid-request, the exception is treated as a failure, allowing the error snackbar to show and isAccountOpen to be flipped after the scope is cancelled.

Replace the bare viewModelScope.launch in onCreateAccount() with viewModelScope.safeLaunch (per coding guidelines for network calls). For fetchAssociatedMscaAccount(), add an explicit catch (t: CancellationException) block before the catch (t: Throwable) block to rethrow cancellation:
Possible fix for fetchAssociatedMscaAccount()
         } catch (t: Throwable) {
+        } catch (t: CancellationException) {
+            throw t
+        } catch (t: Throwable) {
             Timber.e(t)
             null
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModel.kt`
around lines 214 - 230, The current onCreateAccount() uses viewModelScope.launch
with runCatching which swallows CancellationException and
fetchAssociatedMscaAccount() catches Throwable and likewise masks cancellations;
change onCreateAccount() to use viewModelScope.safeLaunch (per project
network-call pattern) so cancellations are handled by the safe launcher, and
inside fetchAssociatedMscaAccount() add an explicit catch (t:
CancellationException) { throw t } before the general catch (t: Throwable) to
rethrow cancellation; ensure any runCatching blocks that handle network calls do
not absorb CancellationException (filter or rethrow) and reference the functions
onCreateAccount(), fetchAssociatedMscaAccount(), and any runCatching usages to
locate the changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModel.kt`:
- Around line 214-230: The current onCreateAccount() uses viewModelScope.launch
with runCatching which swallows CancellationException and
fetchAssociatedMscaAccount() catches Throwable and likewise masks cancellations;
change onCreateAccount() to use viewModelScope.safeLaunch (per project
network-call pattern) so cancellations are handled by the safe launcher, and
inside fetchAssociatedMscaAccount() add an explicit catch (t:
CancellationException) { throw t } before the general catch (t: Throwable) to
rethrow cancellation; ensure any runCatching blocks that handle network calls do
not absorb CancellationException (filter or rethrow) and reference the functions
onCreateAccount(), fetchAssociatedMscaAccount(), and any runCatching usages to
locate the changes.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3c1942d8-756a-4808-9f6a-9aa6fb5b5c50

📥 Commits

Reviewing files that changed from the base of the PR and between d06919e and 72383f4.

📒 Files selected for processing (1)

app/src/main/java/com/vultisig/wallet/ui/screens/v2/defi/circle/CircleDeFiPositionsViewModel.kt

johnnyluo

LGTM

aminsato requested review from johnnyluo and rkokhatskyi as code owners April 23, 2026 12:52

coderabbitai Bot reviewed Apr 23, 2026

View reviewed changes

johnnyluo approved these changes Apr 23, 2026

View reviewed changes

johnnyluo added this pull request to the merge queue Apr 23, 2026

Merged via the queue into main with commit bae5e1b Apr 24, 2026
2 checks passed

johnnyluo deleted the aminsato/fix_circle_defi_race_flake branch April 24, 2026 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: eliminate Circle DeFi isAccountOpen race that flaked unit tests#4177

fix: eliminate Circle DeFi isAccountOpen race that flaked unit tests#4177
johnnyluo merged 2 commits into
mainfrom
aminsato/fix_circle_defi_race_flake

aminsato commented Apr 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 23, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

johnnyluo left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

aminsato commented Apr 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

johnnyluo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aminsato commented Apr 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 23, 2026 •

edited

Loading