Skip to content

fix: [SDK-5538] stop the preferences write loop from spinning after cancellation - #2833

Closed
abdulraqeeb33 wants to merge 1 commit into
mainfrom
fix/preferences-write-loop-cancellation
Closed

abdulraqeeb33 wants to merge 1 commit into
mainfrom
fix/preferences-write-loop-cancellation

Conversation

@abdulraqeeb33

Copy link
Copy Markdown
Contributor

Description

One Line Summary

The PreferencesService write loop stops when its coroutine scope is cancelled instead of spinning on the error path.

Details

Motivation

Found while fault-testing init for SDK-5538. doWorkAsync catches Throwable, which includes CancellationException. Once the scope is cancelled, delay and waitForWake throw on every pass, the catch logs an ERROR, and the loop runs again without ever suspending: an empty editor.apply() plus a log line per iteration, forever. The repro test counted 22,723 iterations in half a second.

Today the only thing that cancels that scope is OneSignalDispatchers.resetForTest(), so this is a test-infrastructure problem rather than a production one: it has been spinning behind every Robolectric spec that initializes the SDK, and in one run it kept QueuedWork.waitToFinish from ever draining and hung the test JVM. The fix is one line and correct regardless of who cancels.

Scope

  • PreferencesService.doWorkAsync: rethrow CancellationException before the generic catch.

Also included in the SDK-5538 stack (#2803), where the fault tests need it; this PR lets it land on main on its own. The two are identical and merge cleanly either way.

Testing

Unit testing

PreferencesServiceTests: start the service, queue a write, reset the dispatchers, and assert no "Error with Preference work loop" line is logged afterwards. Red before the fix (22,723), green after. PreferencesServiceTests and spotless pass on this branch.

Manual testing

Not needed: the only cancelling caller is test-only.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

…ancellation

The loop caught Throwable, which includes CancellationException, so once its
scope was cancelled every pass logged an error, applied an empty edit and
looped again without ever suspending. Only reachable through resetForTest()
today, where it ran behind every Robolectric spec that initialized the SDK
and kept QueuedWork.waitToFinish from ever draining.

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33
abdulraqeeb33 requested a review from a team November 5, 2026 21:13
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • PreferencesService.kt: 2/2 touched executable lines (100.0%) (4 touched lines in diff)

Overall (aggregate gate)

2/2 touched executable lines covered (100.0% — requires ≥ 80%)

📥 View workflow run

@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

Same change is already 6f9ba08 on #2803, which #2828 includes. A PR stacked on #2828 would have no diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant