fix: 24-hour max + 30-min inactivity session rotation, align with iOS#494
fix: 24-hour max + 30-min inactivity session rotation, align with iOS#494turnipdabeets merged 25 commits intomainfrom
Conversation
posthog-android Compliance ReportDate: 2026-05-05 16:26:21 UTC
|
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 258ms |
| Request Payload.Flags Request Uses V2 Query Param | ❌ | 22ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ❌ | 19ms |
| Request Payload.Flags Request Omits Authorization Header | ❌ | 35ms |
| Request Payload.Token In Flags Body Matches Init | ❌ | 18ms |
| Request Payload.Groups Round Trip | ❌ | 20ms |
| Request Payload.Groups Default To Empty Object | ❌ | 18ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ❌ | 17ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ❌ | 17ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 17ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ❌ | 17ms |
| Request Lifecycle.No Flags Request On Init Alone | ❌ | 9ms |
| Request Lifecycle.No Flags Request On Normal Capture | ❌ | 2054ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ❌ | 15ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 14ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 14ms |
Failures
request_payload.request_with_person_properties_device_id
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_uses_v2_query_param
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_hits_flags_path_not_decide
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_omits_authorization_header
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.token_in_flags_body_matches_init
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_round_trip
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_default_to_empty_object
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.person_properties_distinct_id_auto_populated_when_caller_omits_it
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_false_propagates_as_geoip_disable_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_omitted_defaults_to_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flag_keys_to_evaluate_contains_only_requested_key
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.no_flags_request_on_init_alone
Expected 0 /flags requests, got 1
request_lifecycle.no_flags_request_on_normal_capture
Expected 0 /flags requests, got 1
request_lifecycle.two_flag_calls_produce_two_remote_requests
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.mock_response_value_is_returned_to_caller
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
side_effect_events.get_feature_flag_captures_feature_flag_called_event
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
|
Slight behavioral drift vs. iOS: In iOS when we retrieve the active session ID, it checks to see if it's still valid before returning the identifier. If it's not, it's immediately rotated. https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHog/PostHogSessionManager.swift#L109-L114 This differs from the current implementation in that we only rotate the session on a fg/bg transition. iOS calls this getter when building event properties, so it can happen during any event transmission: https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHog/PostHogSDK.swift#L421-L423 There's a test that validates this, it's probably worth porting over: https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHogTests/PostHogSessionManagerTest.swift#L282-L283 |
|
@ioannisj implemented this on iOS so worth an extra pair of eyes (this also affects Flutter iirc) |
|
@ioannisj when you get a moment, mind giving this another review? |
68e3e9a to
595ab25
Compare
3a94a7f to
04c119f
Compare
dustinbyrne
left a comment
There was a problem hiding this comment.
Overall the logic in this change seems good. I've left a couple of comments below, but mainly just concerned about the first.
…em.currentTimeMillis()
When the 24h session limit expires while the app is backgrounded, onStop ends the session and stops replay. The subsequent onStart was only calling startSession(), leaving replay disabled even when session replay was active before the rotation. Mirror the foreground-rotation branch by tracking whether replay was active prior to rotation and restarting it under the new session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the iOS pattern: getActiveSessionId() now checks expiry on every read and rotates (foreground) or clears (background) when the session has lived past SESSION_MAX_DURATION. Previously, rotation only fired on lifecycle transitions, so a continuously foregrounded app could ride a stale session id indefinitely. Adds setAppInBackground() toggled by the lifecycle observer and a setOnSessionIdChangedListener() registered in PostHog.setup() to notify the session replay handler when the getter rotates silently. Ports the iOS sessionRotatedAfterMaxSessionLength test plus sibling cases for bg-clear, RN skip, under-24h no-op, and listener firing.
Mirrors the iOS pattern: when properties already contains $session_id (e.g., session replay attaches it at frame-build time), use it directly instead of calling PostHogSessionManager.getActiveSessionId(). The getter can silently rotate the manager's session, but the caller's value wins downstream via putAll — so the rotation would be wasted.
…close Adds the missing PostHog-level test verifying that a caller-provided $session_id wins over the session manager's value (the change in 165e5f2 had no integration test). Also clears the session listener in PostHog.close() to avoid leaking the PostHog instance via the process-singleton PostHogSessionManager, and simplifies a ?.also { it.invoke() } down to ?.invoke().
Addresses two points from @ioannisj's review: - setSessionId now stamps sessionStartedAt so an externally-set session participates in the 24h expiry check. Without it, sessionStartedAt stayed 0 and the session would never expire. - PostHogReplayIntegration.onSessionIdChanged now restarts the recording when the session rotates silently (e.g., 24h getter rotation), so the new session emits fresh meta + full wireframe events. Previously the new session received only incremental events, leaving the replay viewer with no baseline to render. If the session was cleared (background expiry), recording stops outright since snapshots without $session_id are dropped anyway.
The replay restart from onSessionIdChanged calls start(resumeCurrent=false) which iterates a non-thread-safe WeakHashMap. The getter listener can fire from any thread that calls capture(), so post both stop and start to the main handler. Also adds a PostHogTest that verifies the listener actually wires through: forces an expired session via setSessionId + a backdated date provider, calls getSessionId(), and asserts the fake replay handler's onSessionIdChangedCalled flag flips. Drops a weak assertion from the caller-provided session_id test that was tautologically true.
Closes the parity gap with iOS PostHogSessionManager that ioannisj flagged. The Android session manager only rotated on background/ foreground transitions and on the 24h max-duration check; iOS also rotates after 30min of no user activity, regardless of fg/bg state. Manager changes: - New sessionActivityTimestamp field tracking last activity. - New touchSession() method mirroring iOS: rotates if idle past SESSION_INACTIVITY_DURATION (30min), else refreshes timestamp. No-op when backgrounded so background events don't keep a dead session alive. - getActiveSessionId() also checks inactivity (in iOS order: inactivity first, then 24h max). - Extracted isIdle/isMaxExpired/rotateLocked/clearLocked helpers to deduplicate the three places that compute expiry, and pulled the React Native check inside the lock to remove a TOCTOU race. Wiring (call sites for touchSession): - PostHog.capture(): touch at the start so any captured event counts as activity. iOS achieves this via UIEvent swizzling; Android lacks the equivalent so capture() is the safe fallback. - PostHogLifecycleObserverIntegration.onStart: touch after setAppInBackground(false) (mirror iOS lifecycle hook). - PostHogReplayIntegration.onTouchEventListener: touch on every intercepted touch (closest Android equivalent to iOS UIEvents). The lifecycle observer's existing 30min Timer is now overlapping with the manager's bg-clear path; kept as defense-in-depth for backgrounded apps that don't fire any events. Tests: added 7 unit tests for touchSession behavior, foreground inactivity rotation, background inactivity clear, and the no-op-while-backgrounded guarantee. Added a PostHog-level test verifying that capture() rotates an idle session via touchSession before reading the session id. Updated the existing "under 24 hours" test to keep the session active with periodic touches so the new inactivity check doesn't break it.
- setSessionId no-ops when id is unchanged (avoids resetting the 24h / 30-min clocks when RN re-asserts the same id on every event) - add peekSessionId for read-only callers; replay listener uses it to avoid re-entering the mutating getter from inside onSessionIdChanged - add restartSessionReplay (sampling-aware, no rotation) so a silently rotated session re-evaluates sampling instead of bypassing it - decouple touch-activity tracking from session replay: new PostHogTouchActivityIntegration wires touchSession independently so apps with replay disabled still get touch-driven inactivity rotation - isAppInBackground defaults to true (iOS parity); first onStart flips it to foreground - touchSession on background entry, mirroring iOS onDidEnterBackground - onSessionIdChanged listener now fires from startSession, endSession, and setSessionId (when state actually changes), so replay reacts to every state change, not only silent getter rotations Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- drop redundant attachedWindows tracking in touch integration; rely on Curtains rootViews and per-window interceptor list as the source of truth - narrow getSessionStartedAt and setOnSessionIdChangedListener to internal - add setSessionId(sameId) no-op test - add smoke tests for PostHogTouchActivityIntegration - note isAppInBackground default change in changeset - trim narrative and iOS-parity comments throughout; keep only WHY-explaining comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…active, and onStop touchSession - 6 PostHog.restartSessionReplay tests: closed SDK, disabled flag, active+sampling-pass, inactive+sampling-pass, sampling-fail, no-rotation guarantee - 3 PostHogReplayIntegration.onSessionIdChanged tests: fires restart when active, fires when inactive (sampling re-evaluation), no-op when session cleared - 1 PostHogLifecycleObserverIntegration test: onStop touches session before flipping bg so a fresh idle window starts from the moment of backgrounding Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- guard the onSessionIdChanged listener with try/catch so a misbehaving replay handler can't propagate into the session-mutating call sites (capture, touchSession, etc.) and crash the host app - mark dateProvider as @volatile; it's set on setup and read from any thread inside the session lock, but the setter doesn't take the lock so without volatile a reader could observe a stale null Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- PostHog.startSession had a stale direct sessionReplayHandler.onSessionIdChanged() call left over from before the listener mechanism. Now that PostHogSessionManager.startSession fires the listener, the direct call was redundant and produced two listener invocations on bg->fg foregrounding, causing replay to do an unnecessary stop/restart cycle. - After start(false) clears snapshot states, post a redraw on each tracked decor view. Without this the new session's first user-driven onDraw can be tens of seconds away on a static UI, so type:3 incrementals shipped before the type:4/2 keyframes the player needs to render them. Verified: keyframes now ship <1s after rotation (was ~49s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes restartSessionReplay() from PostHogInterface — it's the SDK's central public surface and even @PostHogInternal additions are highly visible. Move the sampling+restart logic into PostHogReplayIntegration.onSessionIdChanged itself, using config.remoteConfigHolder directly (it already accesses remoteConfigHolder for event triggers). - PostHogInterface.restartSessionReplay() removed (along with PostHog impl and Companion delegate). - PostHogReplayIntegration.onSessionIdChanged inlines: isSessionReplayFlagActive + makeSamplingDecision + start(false). - PostHogLifecycleObserverIntegration's 24h-foreground branch drops the explicit postHog.restartSessionReplay() call — the manager's listener fire from startSession now drives the integration directly. - PostHogTest restartSessionReplay tests removed. - PostHogReplayIntegrationTest rewritten to verify observable replay state (isActive) under mocked PostHogRemoteConfig with various flag/sampling combinations, instead of asserting on a fake-counter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PostHogLifecycleObserverIntegration was tracking its own copy of "session activity" via a lastUpdatedSession AtomicLong updated only on lifecycle transitions, plus a replayActiveBeforeRotation flag for the bg-then-fg rebound. Both shadow state the manager already owns authoritatively, and both can drift the moment the user touches the screen or fires an event between lifecycle ticks. With the manager now firing onSessionIdChangedListener on every state change and the replay integration handling sampling-aware restart inline, those fields are redundant: - onStart simply touches the session (manager rotates if idle), then calls startSession (no-op if alive, creates if cleared during bg). Both fire the listener; the integration handles replay restart with sampling. - onStop's 24h-expired branch ends the session and stops replay synchronously (process may suspend before the listener's main-thread post runs); the else branch just schedules the bg-end timer. Two affected tests rewritten to assert the observable contract: - "onStop ends session and stops replay synchronously when 24h expired" - "onStart creates a fresh session after a 24h-expired onStop" Replay restart on rotation is verified via the listener path in PostHogReplayIntegrationTest with mocked sampling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
04c119f to
1c7f81c
Compare
dustinbyrne
left a comment
There was a problem hiding this comment.
One more thing worth addressing
| } | ||
| if (isSessionReplayActive) stop() | ||
| start(resumeCurrent = false) | ||
| } |
There was a problem hiding this comment.
In iOS, session replay is only started automatically if config.sessionReplay is true (this is a simplification, we actually won't install the integration if it's false).
On main, we don't start automatically recording at any point unless config.sessionReplay is true (isSessionReplayConfigEnabled()).
This onSessionIdChanged implementation will start automatic recording on session rotation if session replay is enabled via remote config. It doesn't automatically start on initialization because the listener isn't bound yet.
There was a problem hiding this comment.
Worth a test case or two
There was a problem hiding this comment.
Added a check for config.sessionReplay plus a test
dustinbyrne flagged that onSessionIdChanged would auto-start replay on session rotation whenever the remote flag and sampling both pass — even if the customer disabled replay at config level via config.sessionReplay = false. That diverges from iOS / Android-main, where config.sessionReplay is the master switch for any auto-start. Add the config.sessionReplay check to the rotation post block, alongside the existing remote-flag and sampling gates. Customer-driven starts via PostHog.startSessionReplay() and trigger-matched starts are unaffected. Two new PostHogReplayIntegrationTest cases: - does not auto-start when config.sessionReplay is false (flag + sampling pass) - stops an already-active replay on rotation under config.sessionReplay = false Also fixed a stale comment in PostHogLifecycleObserverIntegrationTest that referenced lastUpdatedSession (removed in the simplification refactor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d3197b2 to
15548f7
Compare
|
either before or after merging, make sure that this does not break anything on react native and flutter since they use some of those classes eg the session manager etc |
| return | ||
| } | ||
| // Any capture counts as activity so an idle session rotates before its id is read. | ||
| PostHogSessionManager.touchSession() |
There was a problem hiding this comment.
Not 100% sure this should be the case here. We could be capturing without active user engagement (e.g replay network snapshots). Also the event captured could have a custom $session_id, in which case we should not be touching the current session.
I'll leave it up to you but I would personally just consider gestures and/or backgrounding-foregrounding the app as points to touch the current session
There was a problem hiding this comment.
Done — removed touchSession() from capture(); activity tracking is now strictly gestures (PostHogTouchActivityIntegration) + lifecycle bg/fg. Replay snapshots
and caller-provided $session_id events no longer refresh the SDK's session timestamp.
| // Defaults to true so an expired session before the first onStart is cleared rather | ||
| // than silently rotated against a process that has no UI yet. | ||
| @Volatile | ||
| private var isAppInBackground: Boolean = true |
There was a problem hiding this comment.
We default to true here, but what happens if this is not Android (so we never set to false?) I imagine the session will be cleared and not rotated after 30mins right?
There was a problem hiding this comment.
I did an audit and it's only affecting Android, but switched to false and explicitly set android setAppInBackground(true) so that it's more clear.
| // Capture expiry before flipping the bg flag: once bg=true, getActiveSessionId | ||
| // would clear an expired session and zero sessionStartedAt, hiding it from the | ||
| // wasExpired branch below. | ||
| val wasExpired = PostHogSessionManager.isSessionExceedingMaxDuration(currentTimeMillis) |
There was a problem hiding this comment.
Will this return false on RN (since any session rotation logic should be no-op for RN)?
There was a problem hiding this comment.
Good catch — added the RN bypass (returns false when isReactNative)
Two changes from ioannisj review: 1. capture() no longer calls touchSession(). Replay snapshots happen without user engagement, and capture() can carry a caller-provided $session_id that targets a different session entirely. Activity tracking is now strictly gestures (PostHogTouchActivityIntegration) + lifecycle bg/fg transitions (PostHogLifecycleObserverIntegration). Drops the now-stale `capture rotates idle session via touchSession before reading session id` test. 2. isSessionExceedingMaxDuration short-circuits to false when isReactNative. JS owns the session lifecycle on RN; the native side must not drive rotation decisions on top of it. Adds matching test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tested on RN and Flutter and confirmed they don't break |
…K init Per ioannisj review: with the default at true, any non-Android JVM consumer of `posthog` core (which has no lifecycle observer to flip the flag) would forever clear expired sessions instead of rotating them. No real consumer hits this today (`posthog-server` uses `PostHogStateless`, not the session manager), but `PostHog.with(config)` is a public static factory and a future JVM consumer could trip on it. - PostHogSessionManager.isAppInBackground default flipped to false (rotates on expiry, sensible for headless usage). - PostHogAndroid.setAndroidConfig explicitly calls setAppInBackground(true) at init to preserve the iOS-parity "no UI yet at startup" semantic; the lifecycle observer's first onStart still flips it to false as before. Net effect: zero behavioural change for Android (the flag is still true at SDK init and false after the first onStart). Non-Android JVM consumers now get sensible rotate-on-expiry behaviour by default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Let's update PostHog/posthog#30889 once this is out as well |
💡 Motivation and Context
Brings Android session-id behavior into parity with iOS: 30-min inactivity rotation, 24-hour max-duration cap, foreground/background-aware rotate-or-clear, sampling-aware replay restart on rotation, and replay-independent touch-driven activity tracking.
Continues @marandaneto's work from #470 (auto-closed by stale-bot; branch was rebased + force-pushed so GitHub wouldn't allow reopening).
Closes #220 for Android.
What's in scope:
PostHogSessionManager.getActiveSessionId()(mirrors iOS'sgetSessionId(readOnly:)); foreground rotates, background clears.touchSession()— refresh-or-rotate on user activity. Called from lifecycle, the new replay-independent touch interceptor, andcapture().PostHogTouchActivityIntegration— touch-driven activity tracking decoupled from session replay, so behavior is consistent regardless of replay sampling.PostHogInterface.restartSessionReplay()(@PostHogInternal) — sampling-aware restart that does not rotate the session id, so the listener path can avoid double-rotation when invoked from a silent rotation.PostHogSessionManager.peekSessionId()— read-only sibling ofgetActiveSessionId(); lets the replay listener inspect the new id without re-entering the mutating getter.setSessionId(sameId)is now a no-op (Android-specific divergence from iOS, requested in review). Without it, RN/Flutter re-asserting the same id on every event would reset the 24-hour and inactivity clocks and defeat the cap.startSession/endSession/setSessionId, not only silent getter rotations.isAppInBackgrounddefaults totrue(iOS parity) so an expired session before the firstonStartis cleared rather than silently rotated.In-PR follow-ups landed during review:
sessionReplayHandler?.onSessionIdChanged()inPostHog.startSession()that was producing a duplicate listener fire on every bg→fg now that the manager fires the listener itself.start(false)clears snapshot states, postpostInvalidate()on each tracked decor view so the new session emits itstype:4meta + full-wireframe keyframes immediately rather than on the next user-driven onDraw (was ~49 s on a static UI; now <1 s).MutableSet<View>tracker inPostHogTouchActivityIntegration— Curtains'rootViewsand each window'stouchEventInterceptorsare already authoritative.onSessionIdChangedlistener invocation in try/catch so a misbehaving replay handler can't propagate into session-mutating call sites.PostHogSessionManager.dateProvider@Volatilesince it's set once at setup and read from any thread under the session lock.💚 How did you test it?
Unit tests in
PostHogSessionManagerTest,PostHogTest,PostHogLifecycleObserverIntegrationTest,PostHogReplayIntegrationTest, andPostHogTouchActivityIntegrationTestcover:startSession/endSession/setSessionIdlistener-fire semantics (includingsetSessionId(sameId)no-op)touchSessionrefresh, rotate-on-idle, bg/RN/no-session no-opsgetActiveSessionIdrotate-fg vs clear-bg for both inactivity and max-durationpeekSessionIdread-only contractPostHog.restartSessionReplay— SDK closed, replay flag disabled, active+sampling-pass, inactive+sampling-pass, sampling-fail, no-rotation guaranteePostHogReplayIntegration.onSessionIdChangedfiresrestartSessionReplayregardless ofisSessionReplayActive; no-op when session is clearedPostHogLifecycleObserverIntegration— 24h foreground rotation, RN bypass, replay restart on bg→fg,onStoptouches session before flipping bgmake checkFormatand./gradlew :posthog:test :posthog-android:testDebugUnitTest :posthog:apiCheck :posthog-android:apiCheckpass.Manual tests on Pixel 4 (Android 13) with shortened constants (2-min max, 30-s inactivity):
$session_idbefore/after;[Session Replay] Session changedlog fires.$session_idthroughout. Touch decoupling refreshes activity even with replay sampled out.endSessionduring bg,[Session Replay] Session clearedlog); foreground returns under fresh$session_id.type:4+ full-wireframe keyframes within ~1 s of rotation.sessionReplayConfig.sampleRate = 0.5and 6 rotations: roughly half passed (events shipped), half failed (Sample rate (0.5) has determined that this sessionId X will not be sent), confirmingrestartSessionReplayre-rolls sampling each time.Manual test setup
Temporarily flip both constants in
PostHogSessionManager.ktand the lifecycle observer:Build + install:
./gradlew :posthog-samples:posthog-android-sample:installDebugWatch logs:
adb logcat -s PostHogAfter testing, revert constants to 24 h / 30 min.
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file