fix(replay): park a timing-gated buffer across a page unload - #4756
Draft
posthog[bot] wants to merge 2 commits into
Draft
fix(replay): park a timing-gated buffer across a page unload#4756posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
The minimum-duration gate and the markers-only gate hold the snapshot buffer for a retry through setTimeout. An unloading page never runs that retry, so the buffered snapshots died with the page and the recording opened partway through the session. An unload flush now parks the held buffer in sessionStorage, and start() picks it up on the next page in the same tab. sessionStorage dies with the tab, so a session that really ended on that page still ships nothing, which keeps the configured minimum duration intact. Generated-By: PostHog Desktop Task-Id: 89e3aa18-a0af-4b5e-ae7c-ba2e44677880
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 2 should fix, 4 consider. Published 8 findings (view the review). Resolved comments: 1 fixed, 2 declined, 4 left for you |
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
Contributor
posthog-js Compliance ReportDate: 2026-09-03 01:15:45 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
|
Size Change: +8.01 kB (+0.04%) Total Size: 21 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Author
|
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
Author
There was a problem hiding this comment.
PostHog Review
Found 2 must fix, 2 should fix, 4 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.
Marker-only parking bypasses stronger recording gates
Priority: consider | File: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2210-2221, 2229-2240 | Category: bug
Why we think it's a valid issue
- Checked: the whole
_flushBufferorder inpackages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts, the_onBeforeUnloadcaller,_wouldOpenRecordingWithMarkersOnly, theurlBlockeddrop inonRRwebEmit, the status matchers intriggerMatching.ts, and the parking tests inlazy-sessionrecording.test.ts. - Found: the marker-only branch at
lazy-loaded-session-recorder.ts:2212parks at:2218-2220and returns beforeensureSamplingDecisionat:2231and before the status gate at:2240. The gate at:2244restricts the minimum-duration park toACTIVE, but the marker-only park has no such restriction. The premise holds. - Found:
BUFFERINGis not reachable._onBeforeUnloadclears the buffer and returns atlazy-loaded-session-recorder.ts:2446-2449, and:2466is the only_flushBuffer(true)call site.PAUSEDandDISABLEDdo reach the branch. - Found: the paused test at
lazy-sessionrecording.test.ts:6926callsstartBelowMinimumDuration(), which emits an incremental snapshot. The buffer is not marker-only, so the test takes the status branch and never covers this path. The reviewer's note on the test is correct. - Impact: a page that loads on a blocklisted URL pauses at
_pauseRecording(:1054) before any full snapshot, andonRRwebEmitthen drops every event exceptrecording paused(:1833). On unload the recorder parks that marker tosessionStorage. The next page in the same tab restores it and can ship it at the head of a recording that opens later, with no matchingrecording resumed. Before this change the marker died with the page. - Impact: the parked payload holds only Custom lifecycle markers, by the definition at
:2093. No DOM, no full snapshot, and no console data from the blocked page can enter it, so there is no content leak from a blocklisted URL. TheDISABLEDpath is narrower still, because rrweb sets_lastFullSnapshotSessionIdwithin the first emits and the branch stops applying. - Priority: lowered to
consider. The gap is real and reachable with a URL blocklist, and the fix is small, but the confirmed blast radius is a lifecycle debug marker that survives one same-tab navigation. That is a contract gap against the invariant the change states, not a data-loss or privacy defect.
Issue description
The marker-only branch runs before ensureSamplingDecision and before the final status check. An unload can therefore park marker-only data while the recorder is PAUSED, DISABLED, or undecided. This contradicts the invariant that only timing gates may park data.
Suggested fix
Resolve sampling and status before this branch. Park marker-only buffers only for ACTIVE or SAMPLED. Add a paused marker-only test. The current paused test includes an incremental event, so it does not enter this branch.
Comment on lines
+653
to
+657
| // the sessionid manager's key scheme, repeated rather than read from it because this | ||
| // recorder is loaded from the CDN and can run against a core that does not expose it. | ||
| // Two apps on one origin park separately, as they already do for the window id | ||
| const persistenceName = this._instance.config.persistence_name || this._instance.config.token | ||
| this._pendingBufferStorageKey = 'ph_' + persistenceName + PENDING_BUFFER_STORAGE_SUFFIX |
Contributor
Author
There was a problem hiding this comment.
Shared persistence names can send replay data to the wrong project
Why we think it's a valid issue
- Checked: the key build at
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:653-657, the restore guards at:2191, and where the recorder reads its ids (:644-646in the constructor,:1218-1224instart()). - Checked: what a shared
persistence_nameshares between two instances —parseNameatpackages/browser/src/posthog-persistence.ts:86-89,_groupEntryNameat:969, the session id key scheme atpackages/browser/src/sessionid.ts:96, and the window id key atpackages/browser/src/sessionid.ts:111-112. - Found: the premise holds. Two instances with one
persistence_namereadSESSION_IDfrom the same persistence blob, so the session ids match._getWindowIdatpackages/browser/src/sessionid.ts:178-183falls back to the sharedph_<name>_window_idkey, so the second instance adopts the window id the first instance wrote. Both guards at:2191therefore pass, and the restore at:2198moves one token's snapshots into the other token's buffer. - Found: that same configuration already merges much more than replay data across the two tokens.
packages/browser/src/posthog-persistence.ts:86-89puts the whole super-property blob underph_<persistence_name>, and:969puts the feature flag storage group under the same name. Two different tokens with onepersistence_namealready overwrite each other's distinct id, session id, and cached flags. The configuration breaks flag evaluation before it reaches replay, so it is not a state a customer can hold silently. - Found: the key follows the scheme the SDK already uses for
_window_id_storage_key(packages/browser/src/sessionid.ts:111) and_sessionRegisteredPropertiesStorageKey(packages/browser/src/posthog-core.ts:843). The pull request repeats the existing convention rather than departing from it. - Impact: confirmed but narrow. The trigger needs two tokens, one shared
persistence_name, and an active recorder in both instances. Both recorders sit on the same page in the same tab, so the parked payload is a capture of the page the second project records anyway. The content actually differs only when the two instances use different masking settings. - Priority: lowered from
must_fixtoconsider. The tenant boundary is real, and addingconfig.tokento the key, or to the parked value, costs almost nothing. The reachable path, however, needs a configuration that already corrupts identity and flags across the two projects, so this does not block the merge.
Issue description
The key does not always include the project token. Two instances can use different tokens with one shared persistence_name. Their session and window identifiers are also shared. Both restore checks can therefore pass. One instance can upload the other instance's DOM snapshots to the wrong PostHog project.
Suggested fix
Always include config.token in the key. Also store and validate the token in the parked value. Add a two-instance test with a shared persistence name and different tokens.
Prompt to fix with AI (copy-paste)
## Context
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L653-657
<issue_description>
The key does not always include the project token. Two instances can use different tokens with one shared `persistence_name`. Their session and window identifiers are also shared. Both restore checks can therefore pass. One instance can upload the other instance's DOM snapshots to the wrong PostHog project.
</issue_description>
<issue_validation>
- **Checked:** the key build at `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:653-657`, the restore guards at `:2191`, and where the recorder reads its ids (`:644-646` in the constructor, `:1218-1224` in `start()`).
- **Checked:** what a shared `persistence_name` shares between two instances — `parseName` at `packages/browser/src/posthog-persistence.ts:86-89`, `_groupEntryName` at `:969`, the session id key scheme at `packages/browser/src/sessionid.ts:96`, and the window id key at `packages/browser/src/sessionid.ts:111-112`.
- **Found:** the premise holds. Two instances with one `persistence_name` read `SESSION_ID` from the same persistence blob, so the session ids match. `_getWindowId` at `packages/browser/src/sessionid.ts:178-183` falls back to the shared `ph_<name>_window_id` key, so the second instance adopts the window id the first instance wrote. Both guards at `:2191` therefore pass, and the restore at `:2198` moves one token's snapshots into the other token's buffer.
- **Found:** that same configuration already merges much more than replay data across the two tokens. `packages/browser/src/posthog-persistence.ts:86-89` puts the whole super-property blob under `ph_<persistence_name>`, and `:969` puts the feature flag storage group under the same name. Two different tokens with one `persistence_name` already overwrite each other's distinct id, session id, and cached flags. The configuration breaks flag evaluation before it reaches replay, so it is not a state a customer can hold silently.
- **Found:** the key follows the scheme the SDK already uses for `_window_id_storage_key` (`packages/browser/src/sessionid.ts:111`) and `_sessionRegisteredPropertiesStorageKey` (`packages/browser/src/posthog-core.ts:843`). The pull request repeats the existing convention rather than departing from it.
- **Impact:** confirmed but narrow. The trigger needs two tokens, one shared `persistence_name`, and an active recorder in both instances. Both recorders sit on the same page in the same tab, so the parked payload is a capture of the page the second project records anyway. The content actually differs only when the two instances use different masking settings.
- **Priority:** lowered from `must_fix` to `consider`. The tenant boundary is real, and adding `config.token` to the key, or to the parked value, costs almost nothing. The reachable path, however, needs a configuration that already corrupts identity and flags across the two projects, so this does not block the merge.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Always include `config.token` in the key. Also store and validate the token in the parked value. Add a two-instance test with a shared persistence name and different tokens.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
This is a real but narrow cross-project isolation gap, and I'm escalating it for a human decision rather than fixing it unattended, because it's a security/data-isolation boundary and the fix is a deliberate design choice rather than a mechanical correction.
What I confirmed against the current code: the pending-buffer storage key is derived from persistence_name || token and the restore guard only checks the session id and window id. Both of those are already shared between two instances that share a persistence_name, so both guards pass and one project's snapshots can be restored into the other's buffer — the path you describe holds. The trigger, though, requires two different tokens under one shared persistence_name with an active recorder in both, and that same configuration already collapses distinct-id, session id, and cached feature flags across the two projects, so it isn't a state a customer holds silently.
The reason this needs a human: the key deliberately mirrors the session-id manager's window-id key scheme (sessionid.ts), and the session id and window id themselves are already shared SDK-wide under a shared persistence_name. Adding config.token to the park key (or storing and validating it in the parked value) would make replay parking strictly more isolated than the identifiers the rest of the SDK shares — a departure from an established convention that a maintainer should sign off on, especially since it touches a tenant-boundary for replay data.
Decision needed: accept the narrow leak as consistent with the existing shared-persistence_name behavior, or add token scoping to the park key / parked-value guard (low cost) and diverge from the window-id convention for replay parking specifically. If the latter, a two-instance test with a shared persistence_name and different tokens should accompany it.
Comment on lines
+2142
to
+2177
| private _canParkPendingBuffer(): boolean { | ||
| // the same three conditions the sessionid manager applies to the window id, so an opt-out | ||
| // that stops one from writing stops the other | ||
| return ( | ||
| this._instance.config.persistence !== 'memory' && | ||
| this._instance.persistence?._disabled !== true && | ||
| sessionStore._is_supported() | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Parks the buffer for the next page in this tab. A timing gate holds the buffer for a retry | ||
| * that an unloading page never runs, so without this the snapshots die with the page and the | ||
| * recording opens partway through the session. | ||
| */ | ||
| private _parkBufferForNextPage(): void { | ||
| if ( | ||
| this._buffer.data.length === 0 || | ||
| this._buffer.size === this._lastParkedBufferSize || | ||
| this._buffer.size > RECORDING_MAX_EVENT_SIZE || | ||
| !this._canParkPendingBuffer() | ||
| ) { | ||
| return | ||
| } | ||
| this._lastParkedBufferSize = this._buffer.size | ||
| sessionStore._set(this._pendingBufferStorageKey, this._buffer) | ||
| } | ||
|
|
||
| private _restorePendingBuffer(): void { | ||
| if (this._pendingBufferRestored || !this._canParkPendingBuffer()) { | ||
| return | ||
| } | ||
| this._pendingBufferRestored = true | ||
|
|
||
| const parked = sessionStore._parse(this._pendingBufferStorageKey) | ||
| sessionStore._remove(this._pendingBufferStorageKey) |
Contributor
Author
There was a problem hiding this comment.
Parked snapshots survive persistence and consent cleanup
Why we think it's a valid issue
- Checked: the park and restore guards at
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2142-2177, the recorder teardown pathsstop()(:1538-1554),discard()(:1603-1613) and_teardown()(:1449-1456), the consent flow inpackages/browser/src/posthog-core.ts(opt_out_capturingat:4490-4529,opt_in_capturingat:4449-4456,_sync_opt_out_with_persistenceat:4351-4370,_is_persistence_disabledat:4339-4350), and the sibling sessionStorage key handling atpackages/browser/src/posthog-core.ts:843-855and:2217-2237. - Found: the restore path leaves the key behind.
_restorePendingBufferreturns at:2171-2173when_canParkPendingBuffer()is false, which is before thesessionStore._removeat:2177. On a page that loads with persistence disabled, the recorder therefore reads nothing and deletes nothing, and the parked DOM stays insessionStoragefor the life of the tab. - Found: parking is not gated on capture consent, and the recorder keeps running after a plain opt-out.
opt_out_capturing()only callsconsent.optInOut(false)and_sync_opt_out_with_persistence()in the non-cookieless path (packages/browser/src/posthog-core.ts:4506-4507); it never callsstartIfEnabledOrStop().getStatuspasses a hard-codedisRecordingEnabled: true(packages/browser/src/extensions/replay/external/recording-strategies.ts:205-216and:388-393), so the live recorder staysACTIVEand keeps buffering. With the defaultopt_out_persistence_by_default: false,_is_persistence_disabled()returns false, so_canParkPendingBuffer()at:2145-2149still passes and the unload park at:2167writes DOM captured after the opt-out intosessionStorage. - Found: every other exit from the buffer runs through
posthog.capture, which drops the event when consent is denied (packages/browser/src/posthog-core.ts:1551).sessionStore._setat:2167is the first replay path that skips that gate. - Found: a later opt-in can ship that data.
opt_in_capturing()callsstartIfEnabledOrStop()(packages/browser/src/posthog-core.ts:4456), which starts the recorder and runs_restorePendingBuffer()at:1223. In the same tab the session id comes from the shared persistence blob and the window id fromsessionStorage, so both guards at:2191pass and the opted-out snapshots flush. - Found: teardown does not clear the key either.
stop()anddiscard()clear onlythis._buffer;_teardown()removes listeners and timers.dispose({ discardBufferedEvents: true })reachesdiscard()(packages/browser/src/extensions/replay/session-recording.ts:108-116), and that is the path used for the cookieless opt-out (packages/browser/src/posthog-core.ts:4498) and when remote config turns replay off (packages/browser/src/extensions/replay/session-recording.ts:213-215). - Found: the codebase already applies the hygiene the finding asks for.
_persistSessionRegisteredPropKeysremoves itsph_<name>_session_registered_propertieskey whenever persistence is memory, disabled, or unsupported (packages/browser/src/posthog-core.ts:2222-2229, and:853-855at init), and it runs on opt-out (:4367-4368) and on reset (:3467-3468). The new key follows no equivalent rule. - Impact: raw DOM captured after a consent withdrawal reaches browser storage, and a re-consent in the same tab and session sends it. The residue is bounded, because
sessionStoragedies with the tab and the data is not transmitted while the user stays opted out. - Priority: lowered from
must_fixtoshould_fix. The consent bypass is real and new in this change, but transmission needs the full sequence of opt-out, an unload with a timing-gated buffer, and an opt-in in the same tab and session. Two small edits cover the substance: add a capture-consent check to_canParkPendingBuffer()for the park path, and move thesessionStore._removeat:2177ahead of the early return at:2171. The wider cleanup across opt-out, reset, and dispose that the suggestion lists is more machinery than the defect needs.
Issue description
The new key is outside PostHogPersistence. Therefore, set_disabled(true) does not remove an existing parked snapshot. Restore also returns before removing the key while persistence is disabled. Raw DOM data can remain after persistence or capture is disabled. A later opt-in can restore a matching buffer.
Suggested fix
Make the eager recorder remove this key during opt-out, persistence disablement, replay disablement, reset, and disposal. Also check capture consent before parking and remove blocked entries during restore.
Prompt to fix with AI (copy-paste)
## Context
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2142-2177
<issue_description>
The new key is outside `PostHogPersistence`. Therefore, `set_disabled(true)` does not remove an existing parked snapshot. Restore also returns before removing the key while persistence is disabled. Raw DOM data can remain after persistence or capture is disabled. A later opt-in can restore a matching buffer.
</issue_description>
<issue_validation>
- **Checked:** the park and restore guards at `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2142-2177`, the recorder teardown paths `stop()` (`:1538-1554`), `discard()` (`:1603-1613`) and `_teardown()` (`:1449-1456`), the consent flow in `packages/browser/src/posthog-core.ts` (`opt_out_capturing` at `:4490-4529`, `opt_in_capturing` at `:4449-4456`, `_sync_opt_out_with_persistence` at `:4351-4370`, `_is_persistence_disabled` at `:4339-4350`), and the sibling sessionStorage key handling at `packages/browser/src/posthog-core.ts:843-855` and `:2217-2237`.
- **Found:** the restore path leaves the key behind. `_restorePendingBuffer` returns at `:2171-2173` when `_canParkPendingBuffer()` is false, which is before the `sessionStore._remove` at `:2177`. On a page that loads with persistence disabled, the recorder therefore reads nothing and deletes nothing, and the parked DOM stays in `sessionStorage` for the life of the tab.
- **Found:** parking is not gated on capture consent, and the recorder keeps running after a plain opt-out. `opt_out_capturing()` only calls `consent.optInOut(false)` and `_sync_opt_out_with_persistence()` in the non-cookieless path (`packages/browser/src/posthog-core.ts:4506-4507`); it never calls `startIfEnabledOrStop()`. `getStatus` passes a hard-coded `isRecordingEnabled: true` (`packages/browser/src/extensions/replay/external/recording-strategies.ts:205-216` and `:388-393`), so the live recorder stays `ACTIVE` and keeps buffering. With the default `opt_out_persistence_by_default: false`, `_is_persistence_disabled()` returns false, so `_canParkPendingBuffer()` at `:2145-2149` still passes and the unload park at `:2167` writes DOM captured after the opt-out into `sessionStorage`.
- **Found:** every other exit from the buffer runs through `posthog.capture`, which drops the event when consent is denied (`packages/browser/src/posthog-core.ts:1551`). `sessionStore._set` at `:2167` is the first replay path that skips that gate.
- **Found:** a later opt-in can ship that data. `opt_in_capturing()` calls `startIfEnabledOrStop()` (`packages/browser/src/posthog-core.ts:4456`), which starts the recorder and runs `_restorePendingBuffer()` at `:1223`. In the same tab the session id comes from the shared persistence blob and the window id from `sessionStorage`, so both guards at `:2191` pass and the opted-out snapshots flush.
- **Found:** teardown does not clear the key either. `stop()` and `discard()` clear only `this._buffer`; `_teardown()` removes listeners and timers. `dispose({ discardBufferedEvents: true })` reaches `discard()` (`packages/browser/src/extensions/replay/session-recording.ts:108-116`), and that is the path used for the cookieless opt-out (`packages/browser/src/posthog-core.ts:4498`) and when remote config turns replay off (`packages/browser/src/extensions/replay/session-recording.ts:213-215`).
- **Found:** the codebase already applies the hygiene the finding asks for. `_persistSessionRegisteredPropKeys` removes its `ph_<name>_session_registered_properties` key whenever persistence is memory, disabled, or unsupported (`packages/browser/src/posthog-core.ts:2222-2229`, and `:853-855` at init), and it runs on opt-out (`:4367-4368`) and on reset (`:3467-3468`). The new key follows no equivalent rule.
- **Impact:** raw DOM captured after a consent withdrawal reaches browser storage, and a re-consent in the same tab and session sends it. The residue is bounded, because `sessionStorage` dies with the tab and the data is not transmitted while the user stays opted out.
- **Priority:** lowered from `must_fix` to `should_fix`. The consent bypass is real and new in this change, but transmission needs the full sequence of opt-out, an unload with a timing-gated buffer, and an opt-in in the same tab and session. Two small edits cover the substance: add a capture-consent check to `_canParkPendingBuffer()` for the park path, and move the `sessionStore._remove` at `:2177` ahead of the early return at `:2171`. The wider cleanup across opt-out, reset, and dispose that the suggestion lists is more machinery than the defect needs.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Make the eager recorder remove this key during opt-out, persistence disablement, replay disablement, reset, and disposal. Also check capture consent before parking and remove blocked entries during restore.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
This is a real and newly-introduced consent gap, and because it sits on PostHog's capture-consent boundary I'm escalating it for a human decision rather than landing a consent-gate change unattended.
What I confirmed against the current code:
-
Parking is not gated on capture consent.
_parkBufferForNextPageonly checks the buffer size and_canParkPendingBuffer()(persistence not memory/disabled, sessionStorage supported); it never consults consent, and the recorder has no consent references at all. After a plain opt-out the live recorder stays ACTIVE and keeps buffering, so an unload park writes DOM captured after the opt-out into sessionStorage — the first replay path that skips theposthog.captureconsent gate. A later opt-in in the same tab restores and flushes it, because the session id and window id still match. -
The restore path leaves the key behind when it can't park.
_restorePendingBufferreturns early when_canParkPendingBuffer()is false, which is before thesessionStore._remove, so a page that loads with persistence disabled reads nothing and deletes nothing — the parked DOM lingers for the life of the tab. -
Teardown doesn't clear the key.
stop(),discard()and_teardown()clear only the in-memory buffer, sodispose({ discardBufferedEvents: true })(the cookieless opt-out and remote-config-off path) leaves the parked buffer in storage.
The residue is bounded — sessionStorage dies with the tab and nothing is transmitted while the user stays opted out — but re-consent in the same tab and session does send it.
Decision needed from a maintainer: gate _parkBufferForNextPage on capture consent so nothing is parked while opted out, and unconditionally remove the pending-buffer key on restore (regardless of _canParkPendingBuffer()), on opt-out, and on teardown — mirroring the hygiene the SDK already applies to ph_<name>_session_registered_properties. I'm not making that change autonomously because it modifies consent-gating behavior.
Comment on lines
+2218
to
+2221
| if (isUnloading) { | ||
| this._parkBufferForNextPage() | ||
| } | ||
| return this._buffer |
Contributor
Author
There was a problem hiding this comment.
BFCache pages keep a second copy of parked snapshots
Why we think it's a valid issue
- Checked: both park call sites and their return values in
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts, the_onPageHide/_onBeforeUnloadpair,_onVisibilityChange, the restore guards, the window id handover inpackages/browser/src/sessionid.ts, and a repository-wide search forpageshowandpersisted. - Found: the park copies rather than transfers. The markers branch parks and then returns
this._bufferun-drained (:2218-2221), and the minimum-duration branch parks, schedules another flush, and returnsthis._buffer(:2244-2247). After a park the same events exist insessionStorageand in the live page's memory at the same time. - Found: the handler cannot tell a freeze from an unload.
_onPageHideat:2473-2475takes no argument and delegates straight to_onBeforeUnload, soevent.persistedis never read. - Found: no bfcache recovery exists. A search across
packages/browser/srcreturns nopageshowlistener and nopersistedcheck in any SDK path, so a page restored from the back-forward cache resumes withthis._bufferintact and with no signal to drop the copy it left behind._onVisibilityChangeat:2485-2493only appends a custom event; it does not touch the buffer. - Found: the next page really does take the copy. Page A's
beforeunloadclearsprimary_window_existsthrough the session manager listener atpackages/browser/src/sessionid.ts:370-378, so page B's constructor adopts the stored window id atpackages/browser/src/sessionid.ts:117-125, and the session id comes from the shared persistence blob. Both guards at:2191therefore pass and page B ships those events. - Found: a copy left by a navigation that never completes also survives.
_parkBufferForNextPagereturns early whendata.length === 0(:2159) and never removes the key, so once the live page ships and clears its own buffer, the stale stored copy stays until some later page restores it. - Impact: the same snapshot events can ship twice inside one session — once from the page that restored the parked copy, and again from the original page when it resumes from the back-forward cache and its next flush finds the minimum duration satisfied. The park is non-idempotent whenever the source page survives. Duplicate snapshot volume is billed, and a repeated full snapshot followed by repeated mutations degrades playback. Before this change the held buffer never left page A, so it could only ship once.
Issue description
pagehide also fires when the browser places a page in the back-forward cache. rrweb explicitly continues recording after this event. This branch parks the events and returns the same live buffer. The next page can restore and ship those events. If the user returns, the cached page can ship the same events again. A canceled beforeunload creates the same stale copy.
Suggested fix
Pass the PageTransitionEvent to _onPageHide and make _parkBufferForNextPage report write success. For persisted === true, transfer ownership after the final pagehide park and rebuild the recorder on pageshow. Also remove the stored copy when the live buffer later ships or is discarded. Add a real-browser A-to-B-to-back test that asserts each snapshot ships once.
Prompt to fix with AI (copy-paste)
## Context
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2218-2221
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2247-2248
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2469-2474
<issue_description>
`pagehide` also fires when the browser places a page in the back-forward cache. rrweb explicitly continues recording after this event. This branch parks the events and returns the same live buffer. The next page can restore and ship those events. If the user returns, the cached page can ship the same events again. A canceled `beforeunload` creates the same stale copy.
</issue_description>
<issue_validation>
- **Checked:** both park call sites and their return values in `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts`, the `_onPageHide` / `_onBeforeUnload` pair, `_onVisibilityChange`, the restore guards, the window id handover in `packages/browser/src/sessionid.ts`, and a repository-wide search for `pageshow` and `persisted`.
- **Found:** the park copies rather than transfers. The markers branch parks and then returns `this._buffer` un-drained (`:2218-2221`), and the minimum-duration branch parks, schedules another flush, and returns `this._buffer` (`:2244-2247`). After a park the same events exist in `sessionStorage` and in the live page's memory at the same time.
- **Found:** the handler cannot tell a freeze from an unload. `_onPageHide` at `:2473-2475` takes no argument and delegates straight to `_onBeforeUnload`, so `event.persisted` is never read.
- **Found:** no bfcache recovery exists. A search across `packages/browser/src` returns no `pageshow` listener and no `persisted` check in any SDK path, so a page restored from the back-forward cache resumes with `this._buffer` intact and with no signal to drop the copy it left behind. `_onVisibilityChange` at `:2485-2493` only appends a custom event; it does not touch the buffer.
- **Found:** the next page really does take the copy. Page A's `beforeunload` clears `primary_window_exists` through the session manager listener at `packages/browser/src/sessionid.ts:370-378`, so page B's constructor adopts the stored window id at `packages/browser/src/sessionid.ts:117-125`, and the session id comes from the shared persistence blob. Both guards at `:2191` therefore pass and page B ships those events.
- **Found:** a copy left by a navigation that never completes also survives. `_parkBufferForNextPage` returns early when `data.length === 0` (`:2159`) and never removes the key, so once the live page ships and clears its own buffer, the stale stored copy stays until some later page restores it.
- **Impact:** the same snapshot events can ship twice inside one session — once from the page that restored the parked copy, and again from the original page when it resumes from the back-forward cache and its next flush finds the minimum duration satisfied. The park is non-idempotent whenever the source page survives. Duplicate snapshot volume is billed, and a repeated full snapshot followed by repeated mutations degrades playback. Before this change the held buffer never left page A, so it could only ship once.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Pass the `PageTransitionEvent` to `_onPageHide` and make `_parkBufferForNextPage` report write success. For `persisted === true`, transfer ownership after the final pagehide park and rebuild the recorder on `pageshow`. Also remove the stored copy when the live buffer later ships or is discarded. Add a real-browser A-to-B-to-back test that asserts each snapshot ships once.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed this is a real regression introduced by this change, and I'm escalating it for a human to design rather than fixing it unattended, because its correctness can only be proven in a real browser and the fix adds new page-lifecycle machinery.
What I verified in the current code:
- The park copies rather than transfers. Both park branches call
_parkBufferForNextPage()and then return the livethis._bufferun-drained, so after a park the same events exist in bothsessionStorageand the page's memory. _onPageHidetakes no argument and delegates straight to_onBeforeUnload, soevent.persistedis never read, and there is nopageshowlistener or any back-forward-cache check anywhere in the replay code.- So on a bfcache freeze (where
pagehidefires withpersisted === trueand rrweb keeps recording), the next page can restore and ship the parked copy, and if the user navigates back the resumed page can ship the same events again. A canceledbeforeunloadsimilarly leaves a stale copy, because_parkBufferForNextPagenever removes the key once the live page ships. Before this PR the held buffer never left the page, so it could only ship once.
Why it needs a human:
- The fix has to be validated against real back-forward-cache freeze/restore behavior, which unit tests in jsdom can't reproduce — this needs the real-browser A→B→back test that asserts each snapshot ships exactly once.
- The remedy is architectural, not a one-liner: read
event.persistedin_onPageHide, add apageshowlistener and a recorder-rebuild/ownership-transfer path, make_parkBufferForNextPagereport whether it wrote, and remove the stored copy when the live buffer later ships or is discarded. - There's a genuine design choice: the simplest option (don't park on
persisted === true) trades the double-ship for a possible loss if the frozen page is evicted rather than restored — which is the very loss this PR sets out to fix — whereas the full transfer-and-rebuild-on-pageshowapproach handles both eviction and restore. Which trade-off to take should be a maintainer's call.
Note: the 'remove the stored copy when the live buffer ships or is discarded' part overlaps the separate consent/persistence-cleanup thread on this PR, which was also escalated; whoever picks this up should handle the key-lifecycle cleanup once, covering both.
Comment on lines
+1222
to
+1224
| // pick up snapshots a timing gate held when the previous page in this tab unloaded | ||
| this._restorePendingBuffer() | ||
|
|
Contributor
Author
There was a problem hiding this comment.
The fresh-start overflow path can erase a restored buffer
Why we think it's a valid issue
- Checked: the ordering inside
start(), the_holdFlushUntilInteractionlifecycle, the held-overflow branch in_captureSnapshotBuffered,_releaseHoldAndFlush, and the size guards in_parkBufferForNextPage/_restorePendingBuffer. - Found: the ordering claim holds.
lazy-loaded-session-recorder.ts:1191-1195sets_holdFlushUntilInteractionand resets_heldBufferOverflowedinsideif (!this.isStarted), and_restorePendingBuffer()runs later at:1223.isStartedreads!!this._stopRrweb(:1128-1130), so a fresh page always takes that block before the restore. - Found: the hold is on by default for the restore case.
_isIdleinitialises to'unknown'(:514), sothis._isIdle !== falseis true on every fresh page load. The restored buffer therefore sits under the hold, and neither_flushBuffer(early return at:2206) nor_scheduleFlushBuffer(skipped at:2389) can drain it first. - Found: the overflow branch at
:2370-2380clears the whole buffer, restored prefix included, and sets_heldBufferOverflowed. That flag is sticky: it is only reset at:1195(a newstart()) and:1597(_releaseHoldAndFlush), and while set the branch drops every later event. - Impact: confirmed regression against
main, not just a missed improvement. With parked size P and a new full snapshot N where P and N are each under the cap but P + N crosses it, the new page loses its own pre-interaction data too. Onmainthe buffer starts empty, N alone fits, and that data ships at the hold release. The parked prefix itself was already lost onmain, so only the new page's loss is new. - Impact: the loss is bounded.
_releaseHoldAndFlushclears_heldBufferOverflowedand calls_tryTakeFullSnapshot()(:1596-1599), so the recording resumes from the first interaction rather than failing for the session. - Priority: lowered to
should_fix. The trigger needs a combined post-compression payload overRECORDING_MAX_EVENT_SIZE(~0.9 MB,:132), with both parts individually under it —_parkBufferForNextPagealready refuses to park anything over the cap (:2161), and a single snapshot over the cap overflows onmaintoo. That narrows it to heavy-DOM apps, and the recorder recovers at the hold release, so it is not amust_fix. A minimal fix (skip the restore when it leaves no headroom, or drop only the restored prefix on overflow) covers it; the suggested two-buffer merge with timestamp ordering is more machinery than the defect needs.
Issue description
start() enables _holdFlushUntilInteraction before it restores the parked buffer. The next page appends its initial Meta and FullSnapshot events. If the combined size crosses RECORDING_MAX_EVENT_SIZE, the overflow branch clears the complete buffer. A large DOM can therefore delete a valid parked prefix before the user releases the hold.
Suggested fix
Track restored data separately from the fresh-start hold. If the new snapshot crosses the cap, keep the parked prefix and start a new live buffer. Ship both buffers in timestamp order after the timing gate opens. Add a test where a FullSnapshot pushes the combined size over the cap.
Prompt to fix with AI (copy-paste)
## Context
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L1222-1224
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2198
@packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts#L2368-2379
<issue_description>
`start()` enables `_holdFlushUntilInteraction` before it restores the parked buffer. The next page appends its initial Meta and FullSnapshot events. If the combined size crosses `RECORDING_MAX_EVENT_SIZE`, the overflow branch clears the complete buffer. A large DOM can therefore delete a valid parked prefix before the user releases the hold.
</issue_description>
<issue_validation>
- **Checked:** the ordering inside `start()`, the `_holdFlushUntilInteraction` lifecycle, the held-overflow branch in `_captureSnapshotBuffered`, `_releaseHoldAndFlush`, and the size guards in `_parkBufferForNextPage` / `_restorePendingBuffer`.
- **Found:** the ordering claim holds. `lazy-loaded-session-recorder.ts:1191-1195` sets `_holdFlushUntilInteraction` and resets `_heldBufferOverflowed` inside `if (!this.isStarted)`, and `_restorePendingBuffer()` runs later at `:1223`. `isStarted` reads `!!this._stopRrweb` (`:1128-1130`), so a fresh page always takes that block before the restore.
- **Found:** the hold is on by default for the restore case. `_isIdle` initialises to `'unknown'` (`:514`), so `this._isIdle !== false` is true on every fresh page load. The restored buffer therefore sits under the hold, and neither `_flushBuffer` (early return at `:2206`) nor `_scheduleFlushBuffer` (skipped at `:2389`) can drain it first.
- **Found:** the overflow branch at `:2370-2380` clears the whole buffer, restored prefix included, and sets `_heldBufferOverflowed`. That flag is sticky: it is only reset at `:1195` (a new `start()`) and `:1597` (`_releaseHoldAndFlush`), and while set the branch drops every later event.
- **Impact:** confirmed regression against `main`, not just a missed improvement. With parked size P and a new full snapshot N where P and N are each under the cap but P + N crosses it, the new page loses its own pre-interaction data too. On `main` the buffer starts empty, N alone fits, and that data ships at the hold release. The parked prefix itself was already lost on `main`, so only the new page's loss is new.
- **Impact:** the loss is bounded. `_releaseHoldAndFlush` clears `_heldBufferOverflowed` and calls `_tryTakeFullSnapshot()` (`:1596-1599`), so the recording resumes from the first interaction rather than failing for the session.
- **Priority:** lowered to `should_fix`. The trigger needs a combined post-compression payload over `RECORDING_MAX_EVENT_SIZE` (~0.9 MB, `:132`), with both parts individually under it — `_parkBufferForNextPage` already refuses to park anything over the cap (`:2161`), and a single snapshot over the cap overflows on `main` too. That narrows it to heavy-DOM apps, and the recorder recovers at the hold release, so it is not a `must_fix`. A minimal fix (skip the restore when it leaves no headroom, or drop only the restored prefix on overflow) covers it; the suggested two-buffer merge with timestamp ordering is more machinery than the defect needs.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track restored data separately from the fresh-start hold. If the new snapshot crosses the cap, keep the parked prefix and start a new live buffer. Ship both buffers in timestamp order after the timing gate opens. Add a test where a FullSnapshot pushes the combined size over the cap.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed this is a real regression introduced by the parking change, and I'm escalating the fix for a human rather than editing this path unattended, because the correct fix needs new buffer state in a spot dense with invariants and there are several non-equivalent ways to do it.
What I verified: start() turns on the pre-interaction hold and resets the overflow flag before it restores the parked buffer, and because _isIdle starts as 'unknown', the hold is on for every fresh page load. So the restored prefix sits in the live buffer under the hold; when the new page's Meta + FullSnapshot arrive and the combined size crosses RECORDING_MAX_EVENT_SIZE, the held-overflow branch clears the entire buffer and latches _heldBufferOverflowed, dropping the rest until the hold releases. With a parked prefix P and a new snapshot N each under the cap but P+N over it, the new page loses its own pre-interaction data — on main, with an empty start, N fits and ships at the hold release. (The parked prefix itself was already lost on main, so only the new page's loss is new.)
Why it needs a human: to remove the regression without breaking the genuine case, the code has to tell apart 'overflowed because of the restored prefix' (where it should drop the prefix and keep the new epoch's data) from 'overflowed on its own' (where the sticky drop-and-stop memory bound must stay). There's no prefix-boundary tracking today, so this means adding new state (a restored-prefix marker, or a second buffer) whose lifecycle has to be kept correct across every buffer-clearing path — session-rotation re-entrancy, idle transitions, flush, and _clearBuffer. A blanket tweak to the overflow branch would quietly weaken the memory bound for the heavy-DOM case it exists to protect.
Recommended direction for whoever picks it up: the smallest safe option is to drop only the restored prefix on the first held overflow (keeping the new epoch's data and only latching the sticky flag if the new data alone still exceeds the cap), with a unit test where a FullSnapshot pushes the combined size over RECORDING_MAX_EVENT_SIZE. The heavier two-buffer timestamp-merge is more than this defect needs. The loss is bounded either way — the recorder already recovers from the first interaction via _releaseHoldAndFlush, which takes a fresh full snapshot — so this is worth doing but not urgent enough to risk an unattended change here.
The unload park guard only fired for status === ACTIVE, but a sampled-in session reports SAMPLED, not ACTIVE. A session configured with both a sampleRate and a minimumDurationMilliseconds therefore reached the hold branch through _isBelowMinimumDuration and was refused parking, so its buffer died with the page - the exact prefix loss this feature repairs. SAMPLED is a shippable status that, like ACTIVE, can only reach this branch via the minimum-duration gate, so park it the same way. Adds a regression test that combines a sampled-in decision with a minimum duration and asserts the buffer is parked (it reports SAMPLED, not ACTIVE, and fails without the guard change). Generated-By: PostHog Desktop Task-Id: abffc4b6-113f-49d1-be44-b9aca8df4817
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
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
_flushBufferholds the buffer on two timing gates — below the minimum duration, and markers-only (no full snapshot yet). Both gates assume a retry throughsetTimeout./s/.Changes
Mechanism: an unload flush parks the held buffer in
sessionStorage;start()picks it up on the next page in the same tab._flushBuffertakes anisUnloadingflag. Only_onBeforeUnload(and so_onPageHide) passestrue.PAUSED,DISABLED, sampling-undecided and the interaction hold still ship nothing and park nothing.sessionStoragedies with the tab.Restore guards
ph_<persistence_name \|\| token>_replay_pending_buffer, the scheme the sessionid manager already uses, so two apps on one origin park separately.sessionStoragebut mints a new window id, so it does not replay another tab's snapshots.persistence: 'memory'/ persistence is disabled.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
Written with Claude Code (PostHog Desktop) from a self-driving inbox report. Six unit tests were added to
lazy-sessionrecording.test.ts; the three that assert parking fail onmainand pass here.pnpm typecheck,oxlintand the browser unit suite are green (src/__tests__/entrypoints/module.test.tsfails before and after, because it readsdist/).Decisions along the way:
_onBeforeUnload. That reads simpler, but it would also parkDISABLEDbuffers and interaction-held epochs, which must stay discarded. Only the flush knows why the buffer is held, so the decision stays there.beforeunloadandpagehideboth drive an unload flush, so a park is skipped when the buffer has not grown since the last one, instead of serialising the same buffer twice.Created with PostHog Desktop from this inbox report, addressing #93856.