Skip to content

Conversation

jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Sep 25, 2025

Description

One Line Summary

Prevent retrying 400 errors during cold start and clean up any previously saved bad records on device.

Details

Motivation

400 responses are client-side errors that will never succeed on retry. Currently, these can trigger retries during cold start, wasting resources and delaying initialization. In addition, if invalid/bad records were already persisted locally, the SDK would attempt to re-use them, leading to repeated failures.

Scope

  • Affects cold-start retry handling logic and local record persistence.
  • No impact to valid records or successful network requests.
  • No API or behavior changes for SDK consumers beyond improved reliability and faster recovery from bad state.

Testing

Unit testing

Added/updated unit tests to cover:

  • 400 errors are skipped from retry logic.
  • Bad records persisted locally are correctly removed.

Manual testing

Environment: OneSignal example app, Pixel emulator API 33, debug build.

  • Simulated cold start with server returning 400 → verified no retry attempted.
  • Injected bad records into local storage → verified they are cleaned up on next init.

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

} catch (ex: BackendException) {
Logging.warn(
"""OutcomeEventsController.sendSavedOutcomeEvent: Sending outcome with name: ${event.outcomeId} failed with status code: ${ex.statusCode} and response: ${ex.response}
if (ex.statusCode == HttpURLConnection.HTTP_BAD_REQUEST) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using our existing NetworkUtils infrastructure? To drop this for any non-RETRYABLE response

Copy link
Contributor

@abdulraqeeb33 abdulraqeeb33 Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the backend is adding this to the header OneSignal-Retry-Limit

private fun retryLimitFromResponse(con: HttpURLConnection): Int? {

I am wondering if its better to add some logic on the backend that if its a 400 (bad request - which is a client error) then set the retry limit to 0?
I can't think of any reason why a 400 should have a retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using our existing NetworkUtils infrastructure? To drop this for any non-RETRYABLE response

Good point, retrying is now only for retryable response. Omitting outcome event for any other error.

@jinliu9508 jinliu9508 requested a review from nan-li September 25, 2025 23:22
@jinliu9508 jinliu9508 force-pushed the fix-outcome-retry-on-400 branch from c71d30d to b657f4b Compare September 26, 2025 16:23
@nan-li
Copy link
Contributor

nan-li commented Sep 26, 2025

For manual testing, did you compare with the current behavior?

Simulated cold start with server returning 400 → verified no retry attempted.

I'm seeing even without this PR, those 400 requests are not retried on new session or new cold start.

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.

3 participants