fix(replay): honor PostHogConfig.sessionReplay writes after setup - #757
fix(replay): honor PostHogConfig.sessionReplay writes after setup#757posthog[bot] wants to merge 3 commits into
Conversation
`PostHogConfig.sessionReplay` was a plain constructor `var` with no observer. The SDK read it at setup, then again only on a session rotation or a remote config delivery. An app that read its own feature flag and assigned the result kept recording a user the flag excluded, for the rest of the app lifetime. Writing the property now notifies the session replay handler, which re-evaluates recording through `reevaluateRecordingState()`. That is the same decision the remote config path already makes, so the flag, the event triggers, the sampling decision, the manual-start carve-out, and the forced keyframe on resume all keep their existing behavior. A write that does not change the value does nothing. Generated-By: PostHog Desktop Task-Id: 4058c316-29eb-4c85-8406-4e187f0732d9
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 1 should fix, 0 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 left for you |
posthog-android Compliance ReportDate: 2026-09-02 16:00:08 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
| override fun onSessionReplayConfigChanged() { | ||
| reevaluateRecordingState() |
There was a problem hiding this comment.
An event trigger can restart replay after the master switch turns off
Why we think it's a valid issue
- Checked: the full
onEventbody at posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt:2311-2341,start()at 2254-2280,shouldWaitForEventTriggers()at 2411-2426,install()at 525-556, every read and write ofstartedWithAutomaticDisabled, and the dispatch site in the core. - Found: the chain has no master-switch gate at any link.
PostHog.kt:875dispatchessessionReplayHandler?.onEvent(...)unconditionally.install()(line 525) has noconfig.sessionReplaycheck, so the integration is live even when the switch is off.onEventreads only the session id, the trigger list, andtriggerActivatedSessionIdbefore it callsstart(resumeCurrent = true)at line 2339. - Found: the one guard inside
start()is already satisfied by the time it runs.onEventassignstriggerActivatedSessionId = currentSessionIdat line 2335, andshouldWaitForEventTriggers()returnsactivatedSession != currentSessionId(line 2425), so it returns false andstart()proceeds. - Found: the resulting recording is immune to the switch, not merely started by mistake.
start()setsstartedWithAutomaticDisabled = !config.sessionReplayat line 2267, which istruehere. The guard(!config.sessionReplay && !startedWithAutomaticDisabled)then evaluates to false inreevaluateRecordingState(line 2638), inisRecordingPermittedForCurrentSession(line 2613), and on session rotation (line 2387). Onlystop()(line 2292) oruninstall()(line 582) clears the flag, so nothing short of an explicitstopSessionReplay()call recovers. - Found: a clean sequence reaches it with no manual API use. The project has event triggers configured. The app sets
sessionReplay = true. Recording waits for a trigger, so it is inactive. The app writesfalse, andstopIfActiveat line 2640 finds nothing to stop. A matching event then arrives and starts recording with the switch off. - Found: it also reaches the state after a correct stop. A trigger starts recording in session A while the switch is on, the app writes
falseand recording stops, the session rotates, and the next matching event in session B starts recording again through the same path. - Found:
onEventitself is untouched by this PR, so the hole predates it. What is new is the documented contract at posthog/src/main/java/com/posthog/PostHogConfig.kt:409-411, "Set it to false to stop recording, for example when your own feature flag turns off for this user", which this path defeats within the same session. - Impact: an app that excludes a user through the switch keeps recording that user as soon as a project-side trigger event fires, and no later config write, remote-config delivery, or session rotation turns it off. This is the exact privacy failure the PR sets out to close, reached through the SDK's own trigger path rather than through anything the app did.
Issue description
Even when no manual start exists, a matching event can reactivate replay after a false write. onEvent() does not check config.sessionReplay. It calls start(), which marks the recording as manually started. This records a user whom the app excluded.
Suggested fix
Track explicit manual intent separately from an event trigger. In onEvent(), return when config.sessionReplay is false unless a manual start is pending. Add a true-to-false-to-trigger regression test.
Prompt to fix with AI (copy-paste)
## Context
@posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L2311-2339
@posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L2404-2405
<issue_description>
Even when no manual start exists, a matching event can reactivate replay after a false write. `onEvent()` does not check `config.sessionReplay`. It calls `start()`, which marks the recording as manually started. This records a user whom the app excluded.
</issue_description>
<issue_validation>
- **Checked:** the full `onEvent` body at posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt:2311-2341, `start()` at 2254-2280, `shouldWaitForEventTriggers()` at 2411-2426, `install()` at 525-556, every read and write of `startedWithAutomaticDisabled`, and the dispatch site in the core.
- **Found:** the chain has no master-switch gate at any link. `PostHog.kt:875` dispatches `sessionReplayHandler?.onEvent(...)` unconditionally. `install()` (line 525) has no `config.sessionReplay` check, so the integration is live even when the switch is off. `onEvent` reads only the session id, the trigger list, and `triggerActivatedSessionId` before it calls `start(resumeCurrent = true)` at line 2339.
- **Found:** the one guard inside `start()` is already satisfied by the time it runs. `onEvent` assigns `triggerActivatedSessionId = currentSessionId` at line 2335, and `shouldWaitForEventTriggers()` returns `activatedSession != currentSessionId` (line 2425), so it returns false and `start()` proceeds.
- **Found:** the resulting recording is immune to the switch, not merely started by mistake. `start()` sets `startedWithAutomaticDisabled = !config.sessionReplay` at line 2267, which is `true` here. The guard `(!config.sessionReplay && !startedWithAutomaticDisabled)` then evaluates to false in `reevaluateRecordingState` (line 2638), in `isRecordingPermittedForCurrentSession` (line 2613), and on session rotation (line 2387). Only `stop()` (line 2292) or `uninstall()` (line 582) clears the flag, so nothing short of an explicit `stopSessionReplay()` call recovers.
- **Found:** a clean sequence reaches it with no manual API use. The project has event triggers configured. The app sets `sessionReplay = true`. Recording waits for a trigger, so it is inactive. The app writes `false`, and `stopIfActive` at line 2640 finds nothing to stop. A matching event then arrives and starts recording with the switch off.
- **Found:** it also reaches the state after a correct stop. A trigger starts recording in session A while the switch is on, the app writes `false` and recording stops, the session rotates, and the next matching event in session B starts recording again through the same path.
- **Found:** `onEvent` itself is untouched by this PR, so the hole predates it. What is new is the documented contract at posthog/src/main/java/com/posthog/PostHogConfig.kt:409-411, "Set it to false to stop recording, for example when your own feature flag turns off for this user", which this path defeats within the same session.
- **Impact:** an app that excludes a user through the switch keeps recording that user as soon as a project-side trigger event fires, and no later config write, remote-config delivery, or session rotation turns it off. This is the exact privacy failure the PR sets out to close, reached through the SDK's own trigger path rather than through anything the app did.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track explicit manual intent separately from an event trigger. In `onEvent()`, return when `config.sessionReplay` is false unless a manual start is pending. Add a true-to-false-to-trigger regression test.
</potential_solution>
There was a problem hiding this comment.
Confirmed and escalating for a human design decision rather than fixing unattended — this one needs a policy call I shouldn't make on my own. The bug is real: onEvent starts recording on a matching trigger without checking config.sessionReplay, and start() stamps the recording as startedWithAutomaticDisabled, so once a trigger fires with the switch off the recording is immune to the switch, to remote-config deliveries, and to session rotation — only an explicit stopSessionReplay() clears it. That defeats the false-stops-recording contract this PR documents.
Why not a quick fix: the safe-looking change — return from onEvent when config.sessionReplay is false — would break a supported flow. startSessionReplay() does not set config.sessionReplay, and when event triggers are configured it defers (start() early-returns until a trigger matches), so onEvent is the only path that ever begins recording under triggers — for automatic and manual replay. Gating it on the master switch would silently disable manual startSessionReplay() for any project that also uses event triggers with the switch off, which is exactly the manual carve-out this PR is preserving.
Doing it right means introducing explicit manual-intent state (there is none today) and deciding the intended semantics: should an automatic event trigger respect the master switch while an explicit manual start survives it, and how should manual intent be carried through the trigger-deferral path? That plus the fact that onEvent predates this PR (is this in scope here or a follow-up?) is the decision I'd like a maintainer to make before I implement.
…l switch The new KDoc on PostHogConfig.sessionReplay stated that a false write stops recording, but reevaluateRecordingState() deliberately preserves a recording that started while the switch was already false (the startedWithAutomaticDisabled carve-out shared with the session-rotation and remote-config paths). A manual startSessionReplay() or an event-trigger start while the switch is off therefore survives a later false write. Document that the property governs automatic replay only and point callers at stopSessionReplay() to stop such recordings. Documentation-only; no behavior change. Generated-By: PostHog Desktop Task-Id: 9129ac5d-1e19-4e7e-bac5-0edeb68b7611
…sume reevaluateRecordingState() posts the resume to the main thread but the posted task only re-checked !isSessionReplayActive. Because start() never re-reads config.sessionReplay, a write of true followed by a write of false within the same looper turn left a stale resume queued: the false write saw an inactive recorder and posted no stop, then the resume ran and started recording with config.sessionReplay == false. start() also sets startedWithAutomaticDisabled = !config.sessionReplay (true here), so the (!config.sessionReplay && !startedWithAutomaticDisabled) guard then skipped every later stop — inverting the privacy switch this PR exists to deliver. The same window could reactivate the integration after uninstall(). Re-check isRecordingPermittedForCurrentSession() inside the posted task before start(). It re-reads config.sessionReplay, startedWithAutomaticDisabled, the project flag, event triggers, and the sampling decision on the main thread, and returns false once postHog is null (which uninstall() sets), so both races are covered. Adding a precondition can only make resume more conservative, never start a recording that wasn't already going to happen. Adds two regression tests that toggle true->false, and uninstall, before the looper drains; both fail without the guard. Generated-By: PostHog Desktop Task-Id: 9129ac5d-1e19-4e7e-bac5-0edeb68b7611
💡 Motivation and Context
PostHogConfig.sessionReplaylooks live (a publicvar) but is not. The SDK read it at setup, then re-read it only on a session rotation or a remote config delivery. An app that assigns the flag result mid-session gets silence.config.sessionReplaytrue->falsefalse->trueChanges
sessionReplaynotifies the session replay handler, which re-evaluates recording through the existingreevaluateRecordingState().startSessionReplay/stopSessionReplaykeeps one policy for the decision: the project flag, the event triggers, the sampling decision, the manual-start carve-out, and the forced meta + full-snapshot keyframe on resume all behave exactly as they do on the remote config path.onSessionReplayConfigChanged()is a defaulted no-op onPostHogSessionReplayHandler, so hybrid SDKs that implement the interface are unaffected..apidumps change additively. ThesessionReplaygetter and setter signatures are unchanged, so this is source and binary compatible.flowchart LR W["app writes<br/>config.sessionReplay"] --> C{"value changed?"} C -- no --> N["no-op"] C -- yes --> H["replay handler"] H --> R["reevaluateRecordingState()"] R --> S["stop, resume, or leave as is"] RC["remote config delivery"] --> R💚 How did you test it?
Ran locally on JDK 17 with the Android SDK:
:posthog:test,:posthog-android:testDebugUnitTest, and:posthog-server:testall pass with--rerun-tasks, plusspotlessApplyandapiDump.New tests, at three levels:
PostHogTest— a write after setup reaches the replay handler; a write of the same value does not.PostHogReplayIntegrationTest— against the real integration: the switch going off stops an active recording, going on resumes an inactive one, and going on does not resume when the project flag is off. These sit next to the existingonRemoteConfigtests and reuse their fixture.PostHogAndroidTest— end to end throughPostHogAndroid.with(...)and the realPostHogReplayIntegration: replay is active after setup, and turning the switch off makes it inactive with no rotation and no remote config delivery.Each new test was confirmed to fail with the wiring removed, so none of them pass vacuously. No emulator run: the symptom is behavioral rather than visual, and the end-to-end test drives the real integration through the production setup path.
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
/flagsrequest per foreground for every app, and no other SDK does it. I did not re-litigate that call. This change is orthogonal: it makes the app's own re-check effective whenever the app makes it, at zero network cost./flagsload already re-derives the recording decision from the cached/configand notifies the replay integration, so the project linked flag turning off already stops capture on the next flag reload. The untouched gap was the app-owned channel.startSessionReplay(resumeCurrent = true)/stopSessionReplay(). That worked but sat a layer too shallow: it skipped theclearSnapshotStates()+ forced redraw that the resume path needs to avoid orphaned incremental snapshots, and it stopped a manual recording that the remote config path deliberately preserves. Routing throughreevaluateRecordingState()removed both divergences and shrank the diff.PostHogLogCatIntegrationtracks installation in a static field, so the new end-to-end test setssessionReplayConfig.captureLogcat = falseto avoid leaking that flag intoPostHogLogCatIntegrationTest.Created with PostHog Desktop from this inbox report.