Skip to content

fix(replay): honor PostHogConfig.sessionReplay writes after setup - #757

Draft
posthog[bot] wants to merge 3 commits into
mainfrom
posthog-self-driving/fixreplay-stop-android-replay-when-its-fea916
Draft

fix(replay): honor PostHogConfig.sessionReplay writes after setup#757
posthog[bot] wants to merge 3 commits into
mainfrom
posthog-self-driving/fixreplay-stop-android-replay-when-its-fea916

Conversation

@posthog

@posthog posthog Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

  • Who is hurt: Android apps that gate session replay behind their own feature flag. They keep recording users the flag excludes — a privacy-shaped failure, not a cosmetic one.
  • Root cause: PostHogConfig.sessionReplay looks live (a public var) 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.
Write to config.sessionReplay Before After
At setup honored honored
After setup, same value no-op no-op
After setup, true -> false ignored until the next session rotation or remote config delivery, and never if neither happens recording stops now
After setup, false -> true same recording resumes now, if the other gates allow it

Changes

  • The master switch now has an observer. Writing sessionReplay notifies the session replay handler, which re-evaluates recording through the existing reevaluateRecordingState().
  • Routing through that function rather than through startSessionReplay / stopSessionReplay keeps 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 on PostHogSessionReplayHandler, so hybrid SDKs that implement the interface are unaffected.
  • Both .api dumps change additively. The sessionReplay getter 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
Loading

💚 How did you test it?

Ran locally on JDK 17 with the Android SDK: :posthog:test, :posthog-android:testDebugUnitTest, and :posthog-server:test all pass with --rerun-tasks, plus spotlessApply and apiDump.

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 existing onRemoteConfig tests and reuse their fixture.
  • PostHogAndroidTest — end to end through PostHogAndroid.with(...) and the real PostHogReplayIntegration: 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

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Fully autonomous

  • Authored by PostHog Desktop (Claude) from an inbox report, using Read/Grep/Bash and the repo's own Gradle tasks. No human drove it, so it is left unassigned for the owning team to triage. Agent-authored, so it needs human review.
  • Rejected: reviving the foreground flag reload. The report proposed reviving fix(replay): reload feature flags on foreground so flag-gated replay starts for returning users #723. That PR was closed on the grounds that it adds a /flags request 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.
  • Verified as already working, so left alone. A /flags load already re-derives the recording decision from the cached /config and 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.
  • First draft, then reworked. The first pass routed the write to the public startSessionReplay(resumeCurrent = true) / stopSessionReplay(). That worked but sat a layer too shallow: it skipped the clearSnapshotStates() + 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 through reevaluateRecordingState() removed both divergences and shrank the diff.
  • One test-only wrinkle worth knowing: PostHogLogCatIntegration tracks installation in a static field, so the new end-to-end test sets sessionReplayConfig.captureLogcat = false to avoid leaking that flag into PostHogLogCatIntegrationTest.

Created with PostHog Desktop from this inbox report.

`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

posthog Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

🦔 PostHog Review reviewed this pull request

Found 2 must fix, 1 should fix, 0 consider.

Published 3 findings (view the review).

Resolved comments: 2 fixed, 1 left for you

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

posthog-android Compliance Report

Date: 2026-09-02 16:00:08 UTC
Duration: 118489ms

✅ All Tests Passed!

46/46 tests passed


Capture Tests

29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 378ms
Format Validation.Event Has Uuid 30ms
Format Validation.Event Has Lib Properties 29ms
Format Validation.Distinct Id Is String 27ms
Format Validation.Token Is Present 28ms
Format Validation.Custom Properties Preserved 29ms
Format Validation.Event Has Timestamp 23ms
Retry Behavior.Retries On 503 7029ms
Retry Behavior.Does Not Retry On 400 4028ms
Retry Behavior.Does Not Retry On 401 4026ms
Retry Behavior.Respects Retry After Header 7024ms
Retry Behavior.Implements Backoff 17036ms
Retry Behavior.Retries On 500 7019ms
Retry Behavior.Retries On 502 7020ms
Retry Behavior.Retries On 504 7020ms
Retry Behavior.Max Retries Respected 17037ms
Deduplication.Generates Unique Uuids 43ms
Deduplication.Preserves Uuid On Retry 7019ms
Deduplication.Preserves Uuid And Timestamp On Retry 12035ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 7018ms
Deduplication.No Duplicate Events In Batch 42ms
Deduplication.Different Events Have Different Uuids 26ms
Compression.Sends Gzip When Enabled 22ms
Batch Format.Uses Proper Batch Structure 25ms
Batch Format.Flush With No Events Sends Nothing 15ms
Batch Format.Multiple Events Batched Together 36ms
Error Handling.Does Not Retry On 403 4022ms
Error Handling.Does Not Retry On 413 4021ms
Error Handling.Retries On 408 5032ms

Feature_Flags Tests

17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 37ms
Request Payload.Flags Request Uses V2 Query Param 28ms
Request Payload.Flags Request Hits Flags Path Not Decide 26ms
Request Payload.Flags Request Omits Authorization Header 27ms
Request Payload.Token In Flags Body Matches Init 24ms
Request Payload.Groups Round Trip 26ms
Request Payload.Groups Default To Empty Object 24ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 22ms
Request Payload.Disable Geoip Omitted Defaults To False 21ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 31ms
Request Lifecycle.No Flags Request On Init Alone 12ms
Request Lifecycle.No Flags Request On Normal Capture 29ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 39ms
Request Lifecycle.Mock Response Value Is Returned To Caller 22ms
Retry Behavior.Retries Flags On 502 325ms
Retry Behavior.Retries Flags On 504 322ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 25ms

@posthog

posthog Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PostHog Review

Found 2 must fix, 1 should fix.

Comment thread posthog/src/main/java/com/posthog/PostHogConfig.kt Outdated
Comment on lines +2404 to +2405
override fun onSessionReplayConfigChanged() {
reevaluateRecordingState()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An event trigger can restart replay after the master switch turns off

must_fix

Why we think it's a valid issue
  • 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 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>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

posthog Bot added 2 commits September 2, 2026 15:47
…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
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.

0 participants