fix(replay): follow session rotation while _isIdle is unknown - #4750
Open
sortafreel wants to merge 4 commits into
Open
fix(replay): follow session rotation while _isIdle is unknown#4750sortafreel wants to merge 4 commits into
sortafreel wants to merge 4 commits into
Conversation
A recorder that has seen no user interaction stays in the 'unknown' idle state, where truthy _isIdle guards skipped rotation restarts, session-id re-sync, and buffer flushing, leaving new sessions without a playable full snapshot (#4202).
…action Real-rrweb Playwright coverage for #4202: after an activity-timeout rotation in a tab with no interaction, the new session must ship Meta + FullSnapshot with no further activity.
…heck The $session_id_change custom event synchronously re-enters _updateWindowAndSessionIds via rrweb's emit, which already adopts the ids and restarts; the callback now restarts only when the ids are still stale.
Contributor
Contributor
|
Reviews (1): Last reviewed commit: "fix(replay): restart only once when rota..." | Re-trigger Greptile |
Contributor
|
Size Change: +186 B (0%) Total Size: 17.8 MB
ℹ️ View Unchanged
|
Contributor
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). |
Contributor
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review
Found 1 should fix, 1 consider.
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Preserve URL-trigger activation during the re-entrant restart
Priority: should_fix | File: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:1207-1220 | Category: bug
Why we think it's a valid issue
- Checked: The whole re-entrant chain, link by link:
_onSessionIdCallbackordering,_updateWindowAndSessionIds' restart,_startRecorder, rrweb'srecord()inpackages/rrweb/rrweb/src/record/index.ts,onRRwebEmit's trigger check,URLTriggerMatching.checkUrlTriggerConditions/stop,_activateTrigger,V1RecordingStrategy.stop/clearConditionalRecordingPersistence,AndTriggerMatching.triggerStatus, and_onBeforeUnload. - Found: The ordering claim holds.
$session_id_changeis emitted at:1207, and_clearConditionalRecordingPersistence()runs after it at:1209. The emit re-enters_updateWindowAndSessionIds, which adopts the new ids and callsstop(); start('session_id_changed')(:2030-:2039), all synchronously inside that emit. - Found: The restart really does activate the trigger before the clear. rrweb's
record()callsinit()→takeFullSnapshot()inline whendocument.readyStateisinteractiveorcomplete(packages/rrweb/rrweb/src/record/index.ts:741-:752), andtakeFullSnapshotemits a Meta first (:416-:422).onRRwebEmitrunsthis._strategy?.checkUrlTriggers(this.sessionId, ...)on every event (:1524) with the already-swapped new id, and_activateTriggerwritesSESSION_RECORDING_URL_TRIGGER_ACTIVATED_SESSION: this.sessionId(:930-:934).clearConditionalRecordingPersistence()then unregisters that exact key (recording-strategies.ts:304-:306). - Found: The URL cache does block recovery, and the restart is what makes the cache miss happen in the first place.
checkUrlTriggerConditionsreturns early whenurl === this._lastCheckedUrl(triggerMatching.ts:340-:344);URLTriggerMatching.stop()resets it to''(:366-:368) and is reached through_teardown→_strategy?.stop()(:1264,recording-strategies.ts:321-:326). So the restart clears the cache, the Meta evaluates and activates, the activation is wiped, and the cache is back to the current URL — every later event on that URL skips evaluation. With URL triggers only,AndTriggerMatchingdropsTRIGGER_DISABLEDand reportsTRIGGER_PENDING(triggerMatching.ts:152-:167), so the status staysBUFFERINGand_onBeforeUnloaddiscards the buffer in that state (:1919-:1922). - Found: Not a regression from this PR. On the unfixed code the same configuration was equally dead: in the
'unknown'state no restart ran,_clearConditionalRecordingPersistence()still wiped the activation, and_lastCheckedUrlwas never reset, so the recorder sat pending on a stale session id and shipped nothing either. The identical wipe-after-activation interleaving is also already reachable pre-PR for a non-idle recorder whose session rotates onsessionPastMaximumLength. - Impact: Confirmed loss of the entire rotated session's recording for V1 URL-trigger configurations when the URL still matches at rotation time and does not change afterwards. The fix is a reordering inside
_onSessionIdCallback— the same function whose restart guard this PR rewrites — so the author is well placed to make it. - Priority: Lowered to
should_fix. The defect is real and fully traced, but this PR does not introduce or worsen the user-visible outcome, and it needs the niche URL-trigger feature plus a matching, unchanging URL, so it should not block this fix for #4202.
Issue description
On a loaded page, rrweb emits the new recorder's Meta snapshot synchronously. That snapshot activates a matching URL trigger for the new session. After $session_id_change returns, _clearConditionalRecordingPersistence() removes the new activation. The matcher already cached the URL, so later events on the same URL skip evaluation. A URL-trigger-only recorder then stays in BUFFERING and does not send the rotated session.
Suggested fix
Flush the old buffer and clear the old session's conditional state before emitting $session_id_change. Then let the re-entrant restart create the new state. Add a test that rotates from 'unknown' on a matching URL and expects a non-buffering status without navigation.
Comment on lines
+2016
to
+2018
| // Only bail on confirmed idle: while 'unknown' the recorder still captures, so it must | ||
| // also keep checking for session changes or its events are stamped with a stale session id. | ||
| if (this._isIdle === true) { |
Contributor
There was a problem hiding this comment.
Flush the old session before adopting the new IDs
Why we think it's a valid issue
- Checked: The flagged lines against the PR diff (
gh pr diff 4750), plusstop(),_flushBuffer(),_captureSnapshotBuffered(),_onSessionIdCallback(),get status,allMatchSessionRecordingStatus/anyMatchSessionRecordingStatus,V1RecordingStrategy.makeSamplingDecisions/ensureSamplingDecision, andsampleOnProperty. - Found: The mechanism is real.
lazy-loaded-session-recorder.ts:2030assignsthis._windowId/this._sessionId, then:2038callsstop().stop()runs_flushBuffer()then_clearBuffer()(:1334)._flushBuffer()reads the new id:ensureSamplingDecision(this.sessionId)at:1737andstatusat:1645both usethis.sessionId, and_isSampled(:530) decodes the decision againstthis.sessionId. A sampled-out session returnsDISABLED(triggerMatching.ts:671,:714) and a pending trigger returnsBUFFERING(:666,:704); both make_flushBufferreturn the buffer un-drained (:1740), so the following_clearBuffer()drops it. - Found: The buffer is non-empty at that moment.
$session_endingis emitted at:1188before the re-entrant$session_id_changeand is appended under the old id (lifecycle events skip_updateWindowAndSessionIds,:1583), so the discarded buffer contains real old-session data plus the session-linking marker. - Found: The flagged ordering is NOT introduced by this PR. The diff touches only the guard at
:2016(if (this._isIdle)→if (this._isIdle === true)); the assign-then-stop()sequence at:2030-:2039is unchanged, and it is already the mainstream path whenever_isIdle === falseand a session rotates (for examplesessionPastMaximumLengthwhile the user is active). The PR only routes the'unknown'state into the same code. - Found: The same trade-off is already deliberate elsewhere in this file:
_captureSnapshotBufferedflushes onsessionChangedwith the new id in place and explicitly discards a suppressed flush (:1843-:1848, with a comment stating why relabeling would mis-attribute). - Found: The suggested fix is only partly effective on the path described.
_clearConditionalRecordingPersistence()runs at:1209, before the re-entrant emit, and unregisters the trigger-activated and sampling keys, so an old-id flush would still read trigger-pending →BUFFERING. It does help the sampling case, becausesampleOnProperty(extensions/sampling.ts:24) is a deterministic hash of the session id, so re-deciding for the old id reproduces the old decision. - Impact: Confirmed data loss, but bounded. Change 3 in this same PR schedules the 2 s flush timer during
'unknown'(RECORDING_BUFFER_TIMEOUT = 2000,:120), so the residual buffer at rotation is the last ≤2 s of old-session events plus$session_ending— not a whole recording. It bites only when sampling or triggers gate the new session. - Priority: Lowered to
consider. The defect is real and nameable, so it stays on record, butmust_fixoverstates it: the ordering is pre-existing untouched code shared with the already-reachable_isIdle === falserotation path, the loss is the tail of a session that ends anyway, and the codebase already accepts this discard at:1843. Reorderingstop()changes rotation semantics for every idle state and needs its own tests, which is wider than this targeted fix for Replay full snapshot remains on previous session after idle rotation in 1.404.1 #4202.
Issue description
Real rrweb re-enters _updateWindowAndSessionIds during $session_id_change. This new path replaces the IDs before stop() flushes the old buffer. _flushBuffer() then evaluates the new session's sampling and trigger state. A sampled-out or trigger-pending new session suppresses the flush. stop() then discards valid old-session data.
Suggested fix
Call stop() while the recorder still has the old IDs. Assign the new IDs after that flush. Add a real-rrweb test with a sampled-in old session and a sampled-out new session.
Prompt to fix with AI (copy-paste)
## Context
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2016-2018
<issue_description>
Real rrweb re-enters `_updateWindowAndSessionIds` during `$session_id_change`. This new path replaces the IDs before `stop()` flushes the old buffer. `_flushBuffer()` then evaluates the new session's sampling and trigger state. A sampled-out or trigger-pending new session suppresses the flush. `stop()` then discards valid old-session data.
</issue_description>
<issue_validation>
- **Checked:** The flagged lines against the PR diff (`gh pr diff 4750`), plus `stop()`, `_flushBuffer()`, `_captureSnapshotBuffered()`, `_onSessionIdCallback()`, `get status`, `allMatchSessionRecordingStatus`/`anyMatchSessionRecordingStatus`, `V1RecordingStrategy.makeSamplingDecisions`/`ensureSamplingDecision`, and `sampleOnProperty`.
- **Found:** The mechanism is real. `lazy-loaded-session-recorder.ts:2030` assigns `this._windowId`/`this._sessionId`, then `:2038` calls `stop()`. `stop()` runs `_flushBuffer()` then `_clearBuffer()` (`:1334`). `_flushBuffer()` reads the new id: `ensureSamplingDecision(this.sessionId)` at `:1737` and `status` at `:1645` both use `this.sessionId`, and `_isSampled` (`:530`) decodes the decision against `this.sessionId`. A sampled-out session returns `DISABLED` (`triggerMatching.ts:671`, `:714`) and a pending trigger returns `BUFFERING` (`:666`, `:704`); both make `_flushBuffer` return the buffer un-drained (`:1740`), so the following `_clearBuffer()` drops it.
- **Found:** The buffer is non-empty at that moment. `$session_ending` is emitted at `:1188` before the re-entrant `$session_id_change` and is appended under the old id (lifecycle events skip `_updateWindowAndSessionIds`, `:1583`), so the discarded buffer contains real old-session data plus the session-linking marker.
- **Found:** The flagged ordering is NOT introduced by this PR. The diff touches only the guard at `:2016` (`if (this._isIdle)` → `if (this._isIdle === true)`); the assign-then-`stop()` sequence at `:2030`-`:2039` is unchanged, and it is already the mainstream path whenever `_isIdle === false` and a session rotates (for example `sessionPastMaximumLength` while the user is active). The PR only routes the `'unknown'` state into the same code.
- **Found:** The same trade-off is already deliberate elsewhere in this file: `_captureSnapshotBuffered` flushes on `sessionChanged` with the new id in place and explicitly discards a suppressed flush (`:1843`-`:1848`, with a comment stating why relabeling would mis-attribute).
- **Found:** The suggested fix is only partly effective on the path described. `_clearConditionalRecordingPersistence()` runs at `:1209`, before the re-entrant emit, and unregisters the trigger-activated and sampling keys, so an old-id flush would still read trigger-pending → `BUFFERING`. It does help the sampling case, because `sampleOnProperty` (`extensions/sampling.ts:24`) is a deterministic hash of the session id, so re-deciding for the old id reproduces the old decision.
- **Impact:** Confirmed data loss, but bounded. Change 3 in this same PR schedules the 2 s flush timer during `'unknown'` (`RECORDING_BUFFER_TIMEOUT = 2000`, `:120`), so the residual buffer at rotation is the last ≤2 s of old-session events plus `$session_ending` — not a whole recording. It bites only when sampling or triggers gate the new session.
- **Priority:** Lowered to `consider`. The defect is real and nameable, so it stays on record, but `must_fix` overstates it: the ordering is pre-existing untouched code shared with the already-reachable `_isIdle === false` rotation path, the loss is the tail of a session that ends anyway, and the codebase already accepts this discard at `:1843`. Reordering `stop()` changes rotation semantics for every idle state and needs its own tests, which is wider than this targeted fix for #4202.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Call `stop()` while the recorder still has the old IDs. Assign the new IDs after that flush. Add a real-rrweb test with a sampled-in old session and a sampled-out new session.
</potential_solution>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #4202.
In a tab with no user interaction, the session recorder sits in the
'unknown'idle state (_isIdle: boolean | 'unknown'). Several guards treat that truthy string as "idle", so when the analytics session rotates via activity timeout (e.g. a long-lived background tab capturing an event), the recorder:_onSessionIdCallbackonly restarted when_isIdle === true, and_updateWindowAndSessionIds(the "otherwise" restart path its comment relies on) bailed early on truthy_isIdleNet effect, exactly as reported in #4202: the new session's only ingested blob is the
$session_startinglifecycle event, there is no FullSnapshot under the new session id, and the replay is unplayable whileget_session_replay_url()happily points at it.Changes
Four one-line guard fixes in
lazy-loaded-session-recorder.ts, all the same'unknown'-truthiness family:_onSessionIdCallbackrestarts the recorder when_isIdle !== false(was=== true), so a rotation in the'unknown'state restarts immediately and the new session gets its own FullSnapshot._updateWindowAndSessionIdsbails only on confirmed idle (=== true), so a recorder in'unknown'keeps checking for session changes on every event and self-heals a stale_sessionId. This revives the approach from fix(replay): detect session rotation while _isIdle is unknown #3511, which was approved but closed unmerged._captureSnapshotBufferedis scheduled while'unknown', so a no-interaction tab ships its buffer (including the post-rotation FullSnapshot) on the normal 2s cadence instead of holding it until the next rotation or unload.$snapshotcapture returns before the session check in_calculate_event_properties, so flushing cannot extend the session, and_flushBuffer's own gating (buffering/paused/minimum duration) still applies.'unknown', so the buffer can no longer grow unbounded in memory._onSessionIdCallbackalso requires the ids to still be stale: with real rrweb, the$session_id_changecustom event is delivered synchronously throughemitand re-enters_updateWindowAndSessionIds, which (after change 2) adopts the new ids and restarts the recorder itself — without the staleness check, every rotation in the'unknown'state restarted twice, shipping two full DOM serializations. A rotation now restarts the recorder exactly once, and degraded paths (queued emit, blocked URL, stopped recorder) still fall back to the callback restart.Capture (
onRRwebEmit) and periodic snapshot scheduling already treated only=== trueas idle; this brings the remaining guards in line.Test changes:
'unknown'rotation, the reported two-rotation background-tab sequence, the stale-session-id re-sync, and the'unknown'-state flush (red on the old code, green now).addCustomEventforwarded intoemitthe way real rrweb delivers it, pinning the exactly-one-restart behavior of change 5 (red before that change, green after), plus a size-cap-while-'unknown'test for change 4.'unknown'), and three fix(replay): attribute post-rotation snapshots to the new session while idle #4104 compression tests staged rotation by writing_sessionIdwithout telling the session manager — a divergence the recorder now corrects — so they stage the manager too.Release info Sub-libraries affected
Libraries affected
@posthog/react-native-plugin@posthog/react@posthog/ai@posthog/convex@posthog/next@posthog/nextjs-config@posthog/nuxt@posthog/openfeature-node-provider@posthog/openfeature-web-provider@posthog/rollup-plugin@posthog/webpack-plugin@posthog/types@posthog/browser-commonChecklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
SessionIdManager→ recorder callback chain before touching any code, then verified the fix end-to-end by rebuilding the bundle with and without the change and running the new Playwright spec against both (fails on the old bundle with exactly the reported symptom — only the$session_startingblob ships for the new session).$snapshotcapture cannot extend the session before making flushing more eager.