feat(persistence): refresh from storage before declaring an idle session#3690
Draft
pauldambra wants to merge 5 commits into
Draft
feat(persistence): refresh from storage before declaring an idle session#3690pauldambra wants to merge 5 commits into
pauldambra wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Contributor
|
Contributor
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
pauldambra
commented
May 28, 2026
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (specialists + generalists), paul-reviewer, xp-reviewer, security-audit The reviewer was briefed that session-rotation changes are load-bearing for downstream analytics — once a session is incorrectly rotated, the data is unrecoverable. Findings calibrated accordingly: if a reviewer felt uneasy about something subtle, they were asked to surface it rather than drop it. Verdict:
|
| Concern | Reviewers | File:line |
|---|---|---|
Pre-load sessionId clobbers sibling rotation |
xp + security-audit | sessionid.ts:353 |
| Re-arm wiring (race + observability + external-rotation timer leak) | xp + paul + qa-team | sessionid.ts:401 |
Unit tests don't actually exercise load() semantic / re-arm path |
xp + qa-team + paul | sessionid.test.ts:576 |
Pre-existing issues this PR surfaces more strongly
Math.absin_sessionHasBeenIdleTooLong— clock-skew defence currently turns "future activity" into "long-ago activity" and falsely declares idle. Now reached more often via the new code path._persistence.load()cost on idle hot path — extralocalStorage.getItem+JSON.parseof the full persistence blob when activityTimeout fires. Gated correctly behind the rare idle condition.- Two-tab race on rotation — when this tab rotates, sibling reading storage between rotation and next event capture can independently generate a new session. Pre-existing, not introduced here.
Reviewer summaries
| Reviewer | Assessment |
|---|---|
| 🔍 qa-team | Right shape, two leaky abstractions: load() clobbers in-memory pending writes when debounce is enabled, and unit tests model the simulated semantic via direct props mutation rather than the actual load() call. |
| 👤 paul | Right fix for a scary bug, but the new indefinite re-arm loop has no unit coverage and no observability — very Knight Capital-y to silently setTimeout forever. |
| 📐 xp | Fix expresses the right idea OnceAndOnlyOnce, but the symmetry with _flushPendingActivityTimestamp is unfinished (clobber risk) and the re-arm races with destroy(). |
| 🛡 security-audit | No exploitable security findings — the trust boundary for cross-tab localStorage is unchanged. Notes the same correctness concern as xp (post-refresh sessionId clobber) but doesn't file as security. |
Automated by QA Swarm — not a human review
Contributor
|
Size Change: +2.89 kB (+0.02%) Total Size: 16.4 MB
ℹ️ View Unchanged
|
pauldambra
force-pushed
the
posthog-code/cross-tab-session-idle-refresh
branch
from
53db0af to
9d9ecd0
Compare
pauldambra
force-pushed
the
posthog-code/persist-save-debounce
branch
from
410bcb4 to
a81968c
Compare
pauldambra
force-pushed
the
posthog-code/cross-tab-session-idle-refresh
branch
from
f214da7 to
3381c3d
Compare
pauldambra
force-pushed
the
posthog-code/cross-tab-session-idle-refresh
branch
from
3381c3d to
de29137
Compare
pauldambra
force-pushed
the
posthog-code/cross-tab-session-idle-refresh
branch
from
de29137 to
6c84dbe
Compare
pauldambra
force-pushed
the
posthog-code/persist-save-debounce
branch
from
46c8da3 to
d54fa02
Compare
PostHogPersistence does not subscribe to `storage` events, so an idle tab's cached `_persistence.props[SESSION_ID]` lags writes from sibling tabs. Without this fix, when an idle tab's idle timer fires (or its next event capture runs the idle check) it reads its own stale activity timestamp and rotates the session — even though another tab has been capturing events that keep the session alive. The fix adds `_isSessionIdleAfterCrossTabRefresh(timestamp)` which calls `_persistence.load()` before consulting `_freshestActivityTimestamp`, wired into both the rotation path in `checkAndGetSessionAndWindowId` and the idle timer callback. The timer also re-arms when it decides the session is still alive so we re-check after another timeout window. Tests: - jest tests pin both directions (no rotate when sibling kept it alive; rotate when refresh confirms idle) - playwright tests with two real browser tabs in the same context verify the end-to-end behaviour against real localStorage and real `PostHogPersistence.load()` — they pass with the fix and fail without it (verified by stashing the change) Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
…destroy Three convergent HIGH findings from QA Swarm review: 1. `_persistence.load()` in `_isSessionIdleAfterCrossTabRefresh` clobbers pending debounced writes when `persistence_save_debounce_ms > 0` (the 2026-05-30 dated default landing in this same stack sets it to 250ms). `load()` replaces `props` wholesale. Now `flush()` is called first to push pending debounced writes to storage, mirroring the same fix applied to `_flushPendingActivityTimestamp` earlier in the stack. 2. `checkAndGetSessionAndWindowId`'s cross-tab refresh path could clobber a sibling tab's session rotation. Sequence: this tab has stale view (sessionA), sibling rotated to sessionB and wrote a fresh activity timestamp. Our refresh observes sessionB and decides not to rotate, but the local `sessionId`/`startTimestamp` captured BEFORE the load were stale. `_setSessionId` then writes sessionA back, clobbering sessionB. Fix: re-sample `_getSessionId()` after the cross-tab refresh, matching the same defence `_flushPendingActivityTimestamp` uses on the unload path. 3. The new self-perpetuating `_resetIdleTimer` recursion races with `destroy()` — a setTimeout callback queued before destroy could re-arm onto a torn-down instance. Added a `_destroyed` flag set in `destroy()`; both the entry of `_resetIdleTimer` and the timer callback short-circuit if set. Regression tests: - pins the flush()-before-load() order in cross-tab refresh - pins that the post-refresh write uses the refreshed sessionId, not the stale one captured before the refresh - pins that destroy() prevents re-arm even after the callback would have decided 'still alive' Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
- Drop `as any` casts. The `WindowWithPostHog` type already exposes `posthog: PostHog`, and `PostHog.sessionManager` / `SessionIdManager.checkAndGetSessionAndWindowId` are already public on the typed surface. A rename now fails typecheck instead of silently breaking the test. - Parameterize the two cases (sibling-keeps-alive vs all-idle) over a single shared body. The two tests had near-identical setup and only differed in one branch. - Rewrite the header comment to enumerate what the file does and does not cover, so a future reader does not assume timer-callback or throttle-interaction paths are exercised here (they are not; the unit tests in `sessionid.test.ts` cover them). - Document that both tabs get the Playwright clock installed (first tab via the fixture, second via explicit `tab.clock.install()` in `startSecondTab`). - Inline note that the all-idle case is a regression guard, not a fix-pin — it passes with or without the cross-tab refresh logic. Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
The `flush() + load()` cycle inside `_isSessionIdleAfterCrossTabRefresh` and `_flushPendingActivityTimestamp` writes the WHOLE props blob to storage (clobbering a sibling's fresh SESSION_ID write) before reading it back. Replace with a per-key `PostHogPersistence.refreshKey(prop)` that updates only the SESSION_ID slot from storage. Also emit `onSessionId` handlers when the cross-tab refresh observes a sibling rotation (previously silent). Both changes are gated on `persistence_save_debounce_ms > 0`. The clobber bug only bites when debounce is on (a no-op flush in the ungated path can't clobber). The dated default `>= 2026-05-30` enables debounce, so rollout matches the bug surface. New test suite (`cross-tab-persistence.test.ts`) drives two real `PostHogPersistence` instances sharing localStorage and documents the full ACCEPT/REJECT/PRESERVE contract across 36 scenarios. Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
Parameterize the cross-tab behavioural contract over
persistence_save_debounce_ms in {0, 250} so the legacy (flush+load)
path gets the same ACCEPT/REJECT/PRESERVE coverage as the hardened
(per-key refresh) path. Previously only the hardened path was
exercised, leaving the debounce-disabled default (every user on
defaults < 2026-05-30) behaviourally untested.
Pending-local-write survival is hardened-only by nature: with
debounce off, register() writes the whole blob immediately and
clobbers the sibling, so tab B spuriously rotates. That clobber is
pinned explicitly as a legacy-path KNOWN LIMITATION so the gating's
value is documented rather than silently assumed.
Generated-By: PostHog Code
Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
pauldambra
force-pushed
the
posthog-code/cross-tab-session-idle-refresh
branch
from
a02e0f0 to
9188e4d
Compare
pauldambra
force-pushed
the
posthog-code/persist-save-debounce
branch
from
d54fa02 to
35a9cd2
Compare
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
1 participant
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
Each tab in PostHog runs its own
SessionIdManagerwith its own idle timer. When the timer fires (or the nextcheckAndGetSessionAndWindowIdcall runs the idle check), it reads from_persistence.props[SESSION_ID]— but PostHogPersistence does not listen forstorageevents, so that cache only ever reflects what THIS tab has loaded or written. Sibling tabs' writes are invisible.The consequence: a user with three tabs open, active in one and idle in the other two, gets their session rotated by the idle tabs after
session_idle_timeout_seconds— splitting one continuous session into multiple session IDs in the analytics data.Changes
Add
_isSessionIdleAfterCrossTabRefresh(timestamp)which calls_persistence.load()before consulting_freshestActivityTimestamp. Wire it into both:checkAndGetSessionAndWindowId(only on the rotation branch, to avoid the cost on every event capture)_resetIdleTimerWhen the timer fires and the refresh shows the session is still alive (because a sibling kept it active), re-arm the timer so the next check happens after another timeout window.
Tests
localStorageand realPostHogPersistence.load()— they pass with the fix, and stashing the fix produces a fail on the cross-tab testRelease info
Libraries affected
Checklist