fix(persistence): only persist session activity timestamp every 5s#3686
Open
pauldambra wants to merge 7 commits into
Open
fix(persistence): only persist session activity timestamp every 5s#3686pauldambra wants to merge 7 commits into
pauldambra wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
This was referenced May 28, 2026
Contributor
Contributor
|
Size Change: +11.4 kB (+0.07%) Total Size: 16.5 MB
ℹ️ View Unchanged
|
pauldambra
commented
May 28, 2026
Member
Author
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
Multi-perspective review complete. See inline comments + summary thread below.
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 Verdict: 💬 APPROVE WITH NITSAll four reviewers landed in the same place: the change is well-scoped, the logic is correct, the tests pin the right boundaries. The 5s throttle vs 60s minimum idle timeout gives a 12x safety margin so this cannot cause incorrect idle detection. No critical or high findings. Several low/nit observations worth considering but none block merge. Key findings🟢 LOW
⚪ NIT
ConvergenceFindings flagged independently by 2+ reviewers (high confidence):
Reviewer summaries
Automated by QA Swarm — not a human review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86ebe2ac23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browser/src/sessionid.ts:240
`this._sessionId ?? null` is redundant — `_sessionId` is declared as `string | null`, so it can never be `undefined` and the nullish-coalescing fallback never fires. The two other fields in the same tuple (`this._sessionActivityTimestamp`, `this._sessionStartTimestamp`) are written directly without `?? null`, so this is also inconsistent.
```suggestion
[SESSION_ID]: [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp],
```
Reviews (1): Last reviewed commit: "refactor(persistence): address QA Swarm ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
lricoy
approved these changes
May 28, 2026
Member
lricoy
left a comment
lricoy
left a comment
There was a problem hiding this comment.
Let's test it, LGTM.
How will we track whether it IS working or failing, though?
Member
|
uff...
yeah not sure either, down to try tho! |
marandaneto
approved these changes
May 28, 2026
5 tasks
Every `capture()` call updates the session activity timestamp via
`SessionIdManager._setSessionId`. Until now this called
`persistence.register({ SESSION_ID: [...] })` for every change, which
wrote the entire posthog-js persistence blob to localStorage. On an
active page that was 4+ writes per second, each ~hundreds of KB,
broadcasting cross-tab via `storage` events on every other same-origin
tab.
The activity timestamp's only consumer is idle-timeout detection,
which uses thresholds in the tens-of-minutes range. Per-millisecond
freshness in the persisted value is not needed; per-5-seconds is more
than accurate enough — the minimum configurable idle timeout is 60s.
This change keeps the in-memory `_sessionActivityTimestamp` at full
resolution so in-process readers see the latest tick. Only the
write-through to persistence is throttled, by comparing the new
timestamp against the last value we actually persisted.
No throttling applies when the sessionId or sessionStartTimestamp
change — those are semantically important and always persisted.
Generated-By: PostHog Code
Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
…ctivityTimestamp Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
… throttle - Extract _isActivityChangeBelowGranularity helper, embed clock-skew rationale alongside the Math.abs in the helper rather than the call site - Introduce isActivityOnlyChange local to remove duplicated predicate - Reset _lastPersistedActivityTimestamp explicitly in resetSessionId so the reset path is self-evident rather than relying on _setSessionId re-assignment - Flush the pending activity timestamp on destroy and beforeunload so a tab closing inside the granularity window does not leave the persisted value up to 5s stale for other tabs / future loads - Export MIN_SESSION_IDLE_TIMEOUT_SECONDS and ACTIVITY_TIMESTAMP_PERSIST_GRANULARITY_MS - Add tests: invariant (granularity << min idle timeout), backward-delta table (clock skew), post-id-change-baseline, flush-on-destroy with and without pending data Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
…inst cross-tab rotation Two correctness fixes raised in Codex review of the activity-timestamp throttle: 1. Idle detection now compares the current wall clock against `max(persisted, in_memory)` via a new `_freshestActivityTimestamp` helper. Without this, the throttle could hold in-memory activity up to 5s ahead of persisted, and idle checks reading from persistence alone would rotate an active session up to 5s early. Used in both `checkAndGetSessionAndWindowId` and the `_resetIdleTimer` callback. 2. `_flushPendingActivityTimestamp` now refreshes from storage and bails out if the persisted session id / start timestamp no longer match this tab's cached values. Otherwise an unloading tab with a pending throttled tick could overwrite a sibling tab's rotated session with stale id/start values. Tests: - Replaced the pre-existing `persistence is source of truth over in-memory cache` block, which was implicitly testing the bug (it forced persisted to go backwards relative to in-memory and expected idle to fire). New tests cover the realistic scenarios: wall clock crossing timeout while both views are static, and an older sibling write being ignored. - Added a 'does not rotate session early when in-memory is fresher than persisted' regression test pinning the Codex case. - Added flush-clobber and flush-proceeds tests for the cross-tab guard. Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
Method and constant names already say what the code does. Keep only the non-obvious WHY: Math.abs clock-skew defense, in-memory-ahead-of-persisted design, cross-tab clobber recheck, and the granularity-vs-min-timeout invariant. Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
…le blind spot resetSessionId previously left _enforceIdleTimeout armed, so a queued fire after an external reset could call resetSessionId on an already- nulled session. Clearing the timer in resetSessionId closes that latent bug — surfaced more aggressively by the cross-tab refresh follow-up. Also adds a one-line comment near ACTIVITY_TIMESTAMP_PERSIST_GRANULARITY_MS that sibling tabs only observe persisted activity, not in-memory ticks within the throttle window — a documented design tradeoff. Generated-By: PostHog Code Task-Id: 3946962f-7365-4b7b-8f5f-8f9309ed88c6
pauldambra
force-pushed
the
posthog-code/persist-activity-ts-granularity
branch
from
639ae7d to
eb0a2e5
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
3 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
there is an open P2 bug in chromium that local storage writes cause a memory leak https://issues.chromium.org/issues/394823902
we write to local storage a lotthe PH memory leak in PostHog/posthog#32479 is fiendish to replicate in a test
Changes
let's write to local storage less since it can't hurt...
first, let's not track activitity timestamp in millsecond increments writing each to local storage. we can afford to drift a little
-----
Problem
SessionIdManagerwrites the session-activity-timestamp into PostHog persistence on every event capture (4+ times per second during active use). Each write serializes the full props blob, writes through localStorage (or cookie + localStorage), and broadcasts a cross-tabstorageevent in every same-origin tab — even when no listener reacts. Chrome allocates the payload buffer in mojo IPC regardless of subscribers, so this is real work.Changes
Throttle activity-timestamp-only writes to a 5s granularity. Id-changes and session-start-changes still persist immediately. The in-memory
_sessionActivityTimestampis updated at full resolution so within-tab reads still see the freshest tick.Throttle logic:
Math.absdefends against backwards clock jumps (NTP correction, devtools time travel, another tab writing an older value).destroyandbeforeunload, flush the latest in-memory value through to persistence so a tab closing inside the granularity window does not leave the persisted timestamp up to 5s stale for other tabs / future loads.Cross-tab semantics
This is a deliberate, narrow change to cross-tab behaviour: other tabs reading
SESSION_IDdirectly frompersistence.props[SESSION_ID]now see an activity timestamp that can lag the freshest tick by up to 5s. With the minimum configurablesession_idle_timeout_seconds = 60, that's ~8% of the minimum idle threshold — well under the safety margin, and a regression test pins the invariant. Downstream consumers that readSESSION_IDdirectly (session replay, surveys, feature flags) are not sensitive to sub-5s timestamp freshness.Release info
Libraries affected
Checklist