fix(android): Decide session rotation on a monotonic clock (JAVA-573)

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Decide whether foregrounding the app starts a new session on a monotonic clock instead of the wall clock, so that a device time change no longer starts a session that should have been resumed, or resumes one that should have ended ([#6096](https://github.com/getsentry/sentry-java/pull/6096))
+
 ## 8.56.0
 
 ### Behavioral Changes

sentry-android-core/src/main/java/io/sentry/android/core/AppLifecycleIntegration.java

@@ -56,7 +56,9 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions
                 scopes,
                 this.options.getSessionTrackingIntervalMillis(),
                 this.options.isEnableAutoSessionTracking(),
-                this.options.isEnableAppLifecycleBreadcrumbs());
+                this.options.isEnableAppLifecycleBreadcrumbs(),
+                this.options.getMonotonicTicker(),
+                this.options.getEpochClock());
 
         AppState.getInstance().addAppStateListener(watcher);
       }

sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java

@@ -5,53 +5,52 @@
 import io.sentry.ISentryLifecycleToken;
 import io.sentry.SentryLevel;
 import io.sentry.Session;
-import io.sentry.transport.CurrentDateProvider;
-import io.sentry.transport.ICurrentDateProvider;
+import io.sentry.time.Deadline;
+import io.sentry.time.EpochClock;
+import io.sentry.time.MonotonicTicker;
 import io.sentry.util.AutoClosableReentrantLock;
+import java.util.Date;
 import java.util.concurrent.Future;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.TestOnly;
 
 final class LifecycleWatcher implements AppState.AppStateListener {
 
-  private final AtomicLong lastUpdatedSession = new AtomicLong(0L);
-
   private final long sessionIntervalMillis;
 
+  /**
+   * When the session the app left behind stops being resumable, or null while in the foreground.
+   *
+   * <p>Only read or written while holding {@link #endSessionLock}, which is also what lets
+   * cancelling the pending task and taking this deadline happen as one step.
+   */
+  private @Nullable Deadline sessionEnd;
+
   private @Nullable Future<?> endSessionFuture;
   private final @NotNull AutoClosableReentrantLock endSessionLock = new AutoClosableReentrantLock();
   private final @NotNull IScopes scopes;
   private final boolean enableSessionTracking;
   private final boolean enableAppLifecycleBreadcrumbs;
 
-  private final @NotNull ICurrentDateProvider currentDateProvider;
-
-  LifecycleWatcher(
-      final @NotNull IScopes scopes,
-      final long sessionIntervalMillis,
-      final boolean enableSessionTracking,
-      final boolean enableAppLifecycleBreadcrumbs) {
-    this(
-        scopes,
-        sessionIntervalMillis,
-        enableSessionTracking,
-        enableAppLifecycleBreadcrumbs,
-        CurrentDateProvider.getInstance());
-  }
+  private final @NotNull MonotonicTicker ticker;
+  private final @NotNull EpochClock epochClock;
 
   LifecycleWatcher(
       final @NotNull IScopes scopes,
       final long sessionIntervalMillis,
       final boolean enableSessionTracking,
       final boolean enableAppLifecycleBreadcrumbs,
-      final @NotNull ICurrentDateProvider currentDateProvider) {
+      final @NotNull MonotonicTicker ticker,
+      final @NotNull EpochClock epochClock) {
     this.sessionIntervalMillis = sessionIntervalMillis;
     this.enableSessionTracking = enableSessionTracking;
     this.enableAppLifecycleBreadcrumbs = enableAppLifecycleBreadcrumbs;
     this.scopes = scopes;
-    this.currentDateProvider = currentDateProvider;
+    this.ticker = ticker;
+    this.epochClock = epochClock;
   }
 
   @Override
@@ -61,40 +60,48 @@ public void onForeground() {
   }
 
   private void startSession() {
-    cancelTask();
-
-    final long currentTimeMillis = currentDateProvider.getCurrentTimeMillis();
+    final @Nullable Deadline sessionEnd = takeSessionEnd();
 
-    scopes.configureScope(
-        scope -> {
-          if (lastUpdatedSession.get() == 0L) {
-            final @Nullable Session currentSession = scope.getSession();
-            if (currentSession != null && currentSession.getStarted() != null) {
-              lastUpdatedSession.set(currentSession.getStarted().getTime());
-            }
-          }
-        });
-
-    final long lastUpdatedSession = this.lastUpdatedSession.get();
     final boolean startNewSession =
-        lastUpdatedSession == 0L
-            || (lastUpdatedSession + sessionIntervalMillis) <= currentTimeMillis;
+        sessionEnd != null ? sessionEnd.hasPassed() : isSessionOnScopeStale();
     if (startNewSession) {
       if (enableSessionTracking) {
         scopes.startSession();
       }
     }
     scopes.getOptions().getReplayController().onAppForegrounded(startNewSession);
-    this.lastUpdatedSession.set(currentTimeMillis);
+  }
+
+  /**
+   * Whether the session on the scope is too old to resume, so foregrounding should start a new one.
+   *
+   * <p>Used when no background window is pending, which means the session was started by SDK init
+   * rather than by leaving and returning to the app. Nothing captured a tick back then, and the
+   * only record of when the session started is {@link Session#getStarted()} — a wall-clock instant,
+   * because it is sent to Sentry. So this check stays on the wall clock, clock steps included.
+   *
+   * <p>TODO [MAJOR]: let a session remember the tick it started on, so this can use a {@link
+   * Deadline} too. That tick must not be serialized.
+   */
+  private boolean isSessionOnScopeStale() {
+    final long nowMillis = TimeUnit.NANOSECONDS.toMillis(epochClock.now().epochNanos());
+    // No session, or one that never recorded a start, leaves nothing to resume.
+    final @NotNull AtomicBoolean stale = new AtomicBoolean(true);
+    scopes.configureScope(
+        scope -> {
+          final @Nullable Session session = scope.getSession();
+          final @Nullable Date started = session == null ? null : session.getStarted();
+          if (started != null) {
+            stale.set(started.getTime() + sessionIntervalMillis <= nowMillis);
+          }
+        });
+    return stale.get();
   }
 
   // App went to background and triggered this callback after 700ms
   // as no new screen was shown
   @Override
   public void onBackground() {
-    final long currentTimeMillis = currentDateProvider.getCurrentTimeMillis();
-    this.lastUpdatedSession.set(currentTimeMillis);
-
     scopes.getOptions().getReplayController().onAppBackgrounded();
     scheduleEndSession();
 
@@ -104,6 +111,9 @@ public void onBackground() {
   private void scheduleEndSession() {
     try (final @NotNull ISentryLifecycleToken ignored = endSessionLock.acquire()) {
       cancelTask();
+      final @NotNull Deadline sessionEnd =
+          Deadline.after(ticker, sessionIntervalMillis, TimeUnit.MILLISECONDS);
+      this.sessionEnd = sessionEnd;
       final @NotNull Runnable endSession =
           () -> {
             if (enableSessionTracking) {
@@ -114,11 +124,13 @@ private void scheduleEndSession() {
           };
 
       try {
+        // The executor's own delay stops while the device is suspended, while the deadline keeps
+        // counting, so this task can only run at or after the deadline. It needs no second check.
         endSessionFuture =
             scopes
                 .getOptions()
                 .getTimerExecutorService()
-                .schedule(endSession, sessionIntervalMillis);
+                .schedule(endSession, sessionEnd.remaining(TimeUnit.MILLISECONDS));
       } catch (Throwable e) {
         scopes
             .getOptions()
@@ -131,6 +143,16 @@ private void scheduleEndSession() {
     }
   }
 
+  /** Stops the pending end of session and hands back the deadline it was going to run at. */
+  private @Nullable Deadline takeSessionEnd() {
+    try (final @NotNull ISentryLifecycleToken ignored = endSessionLock.acquire()) {
+      cancelTask();
+      final @Nullable Deadline sessionEnd = this.sessionEnd;
+      this.sessionEnd = null;
+      return sessionEnd;
+    }
+  }
+
   private void cancelTask() {
     try (final @NotNull ISentryLifecycleToken ignored = endSessionLock.acquire()) {
       if (endSessionFuture != null) {

sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt

@@ -12,7 +12,12 @@ import io.sentry.SentryLevel
 import io.sentry.SentryOptions
 import io.sentry.Session
 import io.sentry.Session.State
-import io.sentry.transport.ICurrentDateProvider
+import io.sentry.time.EpochClock
+import io.sentry.time.TestMonotonicTicker
+import io.sentry.time.Timestamp
+import java.util.concurrent.TimeUnit.HOURS
+import java.util.concurrent.TimeUnit.MILLISECONDS
+import java.util.concurrent.atomic.AtomicLong
 import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -32,7 +37,10 @@ import org.mockito.kotlin.whenever
 class LifecycleWatcherTest {
   private class Fixture {
     val scopes = mock<IScopes>()
-    val dateProvider = mock<ICurrentDateProvider>()
+    val ticker = TestMonotonicTicker()
+    // the wall clock, which only the staleness of a session already on the scope depends on
+    val nowMillis = AtomicLong(0L)
+    val epochClock = EpochClock { Timestamp.ofEpochNanos(MILLISECONDS.toNanos(nowMillis.get())) }
     // a real executor so scheduled end-session tasks actually run
     val options = SentryOptions().apply { setTimerExecutorService(SentryExecutorService(this)) }
     val replayController = mock<ReplayController>()
@@ -60,7 +68,8 @@ class LifecycleWatcherTest {
         sessionIntervalMillis,
         enableAutoSessionTracking,
         enableAppLifecycleBreadcrumbs,
-        dateProvider,
+        ticker,
+        epochClock,
       )
     }
   }
@@ -81,26 +90,67 @@ class LifecycleWatcherTest {
   }
 
   @Test
-  fun `if last started session is after interval, start new session`() {
-    val watcher = fixture.getSUT(enableAppLifecycleBreadcrumbs = false)
-    whenever(fixture.dateProvider.currentTimeMillis).thenReturn(1L, 2L)
+  fun `if the background window has elapsed, start new session`() {
+    val watcher =
+      fixture.getSUT(sessionIntervalMillis = 30000L, enableAppLifecycleBreadcrumbs = false)
     watcher.onForeground()
+    watcher.onBackground()
+    fixture.ticker.advance(30000, MILLISECONDS)
+
     watcher.onForeground()
+
     verify(fixture.scopes, times(2)).startSession()
     verify(fixture.replayController, times(2)).onAppForegrounded(true)
   }
 
   @Test
-  fun `if last started session is before interval, it should not start a new session`() {
-    val watcher = fixture.getSUT(enableAppLifecycleBreadcrumbs = false)
-    whenever(fixture.dateProvider.currentTimeMillis).thenReturn(2L, 1L)
+  fun `if the app returns within the background window, it should not start a new session`() {
+    val watcher =
+      fixture.getSUT(sessionIntervalMillis = 30000L, enableAppLifecycleBreadcrumbs = false)
     watcher.onForeground()
+    watcher.onBackground()
+    fixture.ticker.advance(29999, MILLISECONDS)
+
     watcher.onForeground()
+
     verify(fixture.scopes).startSession()
     verify(fixture.replayController).onAppForegrounded(true)
     verify(fixture.replayController).onAppForegrounded(false)
   }
 
+  @Test
+  fun `a wall clock stepping forward during the background window does not rotate the session`() {
+    val watcher =
+      fixture.getSUT(sessionIntervalMillis = 30000L, enableAppLifecycleBreadcrumbs = false)
+    watcher.onForeground()
+    watcher.onBackground()
+
+    // the device syncs its clock an hour forward, which two wall-clock reads used to report as an
+    // hour spent in the background
+    fixture.nowMillis.addAndGet(HOURS.toMillis(1))
+    fixture.ticker.advance(1, MILLISECONDS)
+    watcher.onForeground()
+
+    verify(fixture.scopes).startSession()
+    verify(fixture.replayController).onAppForegrounded(false)
+  }
+
+  @Test
+  fun `a wall clock stepping backwards during the background window still rotates the session`() {
+    val watcher =
+      fixture.getSUT(sessionIntervalMillis = 30000L, enableAppLifecycleBreadcrumbs = false)
+    fixture.nowMillis.set(HOURS.toMillis(1))
+    watcher.onForeground()
+    watcher.onBackground()
+
+    fixture.nowMillis.set(0L)
+    fixture.ticker.advance(30000, MILLISECONDS)
+    watcher.onForeground()
+
+    verify(fixture.scopes, times(2)).startSession()
+    verify(fixture.replayController, times(2)).onAppForegrounded(true)
+  }
+
   @Test
   fun `if app goes to background, end session after interval`() {
     val watcher = fixture.getSUT(enableAppLifecycleBreadcrumbs = false)
@@ -249,7 +299,6 @@ class LifecycleWatcherTest {
 
   @Test
   fun `background-foreground replay`() {
-    whenever(fixture.dateProvider.currentTimeMillis).thenReturn(1L)
     val watcher =
       fixture.getSUT(sessionIntervalMillis = 500L, enableAppLifecycleBreadcrumbs = false)
     watcher.onForeground()