Skip to content

fix: treat time since last in-app as unbounded when no in-app was dismissed - #2814

Open
RaphaelFakhri wants to merge 2 commits into
OneSignal:mainfrom
RaphaelFakhri:fix/time-since-last-iam-default
Open

RaphaelFakhri wants to merge 2 commits into
OneSignal:mainfrom
RaphaelFakhri:fix/time-since-last-iam-default

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Description

One Line Summary

Fix "Time since last in-app message" triggers that wait instead of firing when no in-app message has been dismissed yet.

Details

Motivation

Fixes #1602. When lastTimeInAppDismissed is null, DynamicTriggerController falls back to DEFAULT_LAST_IN_APP_TIME_AGO, which is meant to mean "a very long time ago". The constant is 999999, but the value is compared against intervals in milliseconds, so it only represents about 16.7 minutes.

As a result, a trigger such as "time since last in-app message greater than 3600 seconds" does not fire for a user who has never dismissed an in-app message. Instead, the SDK schedules a timer for the remaining 43 minutes or so, and the message shows late or not at all if the session ends first. Any threshold above about 16.7 minutes is affected.

The fallback is now Long.MAX_VALUE, so a user who has never dismissed an in-app message always satisfies a "greater than" threshold, and never satisfies a "less than" threshold.

Scope

Only the fallback used when no in-app message has been dismissed changes. Triggers evaluated after a dismissal still use the persisted dismissal time, and session-time triggers are unchanged.

Testing

Unit testing

Added DynamicTriggerControllerTests:

  • A "time since last in-app greater than 3600 seconds" trigger fires when no in-app message has been dismissed. This test fails before the fix.
  • The same trigger does not fire one minute after a dismissal.

Manual testing

Not tested on a device. The change only affects the trigger evaluation covered by the unit tests.

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

@RaphaelFakhri
RaphaelFakhri requested a review from a team as a code owner October 25, 2026 07:40

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The null fallback is the right sentinel. Elapsed time is compared in milliseconds, and Long.MAX_VALUE is large enough that a greater-than threshold matches immediately and a less-than threshold does not. For a positive threshold the follow-up offset stays negative, and the method returns before scheduling a timer. start() copies the persisted dismissal time in before trigger subscribers run, so null means there is no stored dismissal.

Both new tests passed. The red test check is the coverage-comment step: this PR is from a fork, and the workflow token got a 403 creating the issue comment. The unit-test task and the coverage threshold both passed. That 403 also fails the aggregate build check.

One edge on the new sentinel: a negative min_time_since value of about -0.002 seconds or below wraps requiredTimeInterval - Long.MAX_VALUE to a large positive delay (DynamicTriggerController.kt offset subtraction). Timer.schedule then throws IllegalArgumentException ("Illegal execution time") after DynamicTriggerTimer has already started a non-daemon timer thread. suspendify logs that and drops the evaluation pass. The previous 999999 sentinel stayed on the offset <= 0 return. Positive thresholds, which are what this fix is for, still take that return. Worth a guard if a negative value can show up in a payload.

@abdulraqeeb33

Copy link
Copy Markdown
Contributor

Ran DynamicTriggerControllerTests locally on cb5577487. All six passed (BUILD SUCCESSFUL).

The two tests on the PR:

  • Greater than 3600 seconds fires when no in-app message has been dismissed.
  • The same trigger does not fire one minute after a dismissal.

Four extra probes, left out of the branch:

  • Less than 3600 seconds returns false, and starts no timer.
  • Less than -0.001 seconds returns false, and starts no timer.
  • Less than -0.002 seconds throws IllegalArgumentException: "Illegal execution time."
  • Less than -1 second throws the same exception, and leaves a live non-daemon thread named trigger_timer:neg-one-second.

With no in-app message dismissed, the time since the last one is
Long.MAX_VALUE, so a negative required interval overflowed the offset into
a large positive delay and Timer.schedule threw. Return before computing
the offset when the interval has already passed.
@RaphaelFakhri

Copy link
Copy Markdown
Author

Thanks for the probes. I added the guard in 4783cc7. dynamicTriggerShouldFire now returns when currentTimeInterval >= requiredTimeInterval, before it computes the offset. That condition is the same as the previous offset <= 0 check without the subtraction, so a negative less-than value with no dismissal returns false and starts no timer. A new test covers less than -1 second with no dismissed in-app message.

I couldn't run the Gradle unit tests here because this machine has no Android SDK, so CI is the first run of the new test.

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.

Time since last In-App Message trigger, not working when time is greater than DEFAULT_LAST_IN_APP_TIME_AGO

2 participants