feat(replay): rotate recording session at a configurable size budget#3884
Draft
pauldambra wants to merge 2 commits into
Draft
feat(replay): rotate recording session at a configurable size budget#3884pauldambra wants to merge 2 commits into
pauldambra wants to merge 2 commits into
Conversation
Add session_recording.maxSessionSizeMb (default 300 under the 2026-05-30 defaults, clamped 1-500 MiB) driving size-based session rotation. When a session's flushed bytes reach the budget the recorder rotates to a new, linked session via a new sessionMaximumSize change reason, so no single recording exceeds the budget while recording continues uninterrupted and no data is lost. - types: maxSessionSizeMb option (with support-team guidance comment) and the sessionMaximumSize change reason - sessionid: rotateSessionForReplaySize() mints a linked session (not a reset) so $session_ending/$session_starting fire - recorder: budget getter, a post-flush check that emits a $session_size_rotation custom event marking the rotation point, and shouldLinkSessions extended to the new reason - posthog-core: 300 MiB default wired into the 2026-05-30 bundle Generated-By: PostHog Code Task-Id: b9b6f524-7f9d-421a-9b47-8e7131dd8536
pauldambra
requested review from
a team,
TueHaulund,
arnohillen,
fasyy612 and
ksvat
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // the recorder links the old and new sessions instead of treating it as a wipe. | ||
| rotateSessionForReplaySize(): void { | ||
| const timestamp = new Date().getTime() | ||
| const sessionId = this._sessionIdGenerator() |
![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: cf2e7ee39e
ℹ️ 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".
Comment on lines
+204
to
+206
| session_recording: | ||
| defaults && defaults >= '2026-05-30' | ||
| ? { strictMinimumDuration: true, maxSessionSizeMb: 300 } |
There was a problem hiding this comment.
With defaults: '2026-05-30', this new maxSessionSizeMb default is lost whenever the customer already supplies any partial session_recording config, because _init shallow-merges baseConfig and userConfig and only deep-merges rageclick. For example, { defaults: '2026-05-30', session_recording: { maskAllInputs: true } } replaces this whole object, leaves maxSessionSizeMb undefined, and silently disables the new rotation guardrail for many configured replay installations.
Useful? React with 👍 / 👎.
Comment on lines
+1595
to
+1598
| const flushedBuffer = this._clearBuffer() | ||
| // rotate after clearing so the rotation custom events land in the fresh buffer | ||
| this._maybeRotateForSessionSize() | ||
| return flushedBuffer |
There was a problem hiding this comment.
When _flushBuffer() is invoked from _captureSnapshotBuffered to make room for a large event and the size budget is reached, _maybeRotateForSessionSize() can synchronously emit rotation events that mutate this._buffer and nested-flush the old session, but this method still returns the pre-rotation buffer. The caller at the size-boundary path assigns that stale object back to this._buffer, which can drop the new-session buffer/linking events and continue buffering the triggering event under the old session.
Useful? React with 👍 / 👎.
Comment on lines
+1
to
+3
| --- | ||
| 'posthog-js': minor | ||
| --- |
There was a problem hiding this comment.
This changes public types in packages/types/src/posthog-config.ts and packages/types/src/session-recording.ts, but the changeset only bumps posthog-js. Since .changeset/config.json has no linked/fixed packages, @posthog/types will not publish a new version, so consumers importing SessionRecordingOptions or SessionIdChangedCallback from @posthog/types will not get maxSessionSizeMb or sessionMaximumSize after the release.
Useful? React with 👍 / 👎.
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browser/src/__tests__/extensions/replay/lazy-sessionrecording.test.ts:4156-4164
**Non-parameterised rotation gate tests**
The three `size-based rotation` tests share the same logical shape — "given this flushed size and this `maxSessionSizeMb`, should `rotateSessionForReplaySize` be called?" — and could be expressed as a single `it.each` table. The team's convention prefers parameterised tests for cases like this, and a table would make it easy to add new boundary scenarios (e.g. exactly at the threshold) without duplicating the `beforeEach` setup.
### Issue 2 of 2
.changeset/replay-size-rotation.md:1-3
**`@posthog/types` not listed in changeset**
The PR description marks `@posthog/types` as an affected library, and two files under `packages/types/src/` are modified (`posthog-config.ts` and `session-recording.ts`). The changeset only lists `posthog-js`, so `@posthog/types` would not receive a version bump in the release. If the types package is published independently, downstream consumers that depend on `@posthog/types` directly (rather than via `posthog-js`) may be stuck on a stale type definition for `SessionIdChangedCallback` and `SessionRecordingOptions.maxSessionSizeMb`.
Reviews (1): Last reviewed commit: "feat(replay): rotate recording session a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4156
to
+4164
| config.session_recording.maxSessionSizeMb = undefined | ||
| recorder['_flushedSizeTracker'].trackSize(recorder.sessionId, 10 * oneMb) | ||
|
|
||
| recorder['_maybeRotateForSessionSize']() | ||
|
|
||
| expect(rotateSpy).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
The three size-based rotation tests share the same logical shape — "given this flushed size and this maxSessionSizeMb, should rotateSessionForReplaySize be called?" — and could be expressed as a single it.each table. The team's convention prefers parameterised tests for cases like this, and a table would make it easy to add new boundary scenarios (e.g. exactly at the threshold) without duplicating the beforeEach setup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/extensions/replay/lazy-sessionrecording.test.ts
Line: 4156-4164
Comment:
**Non-parameterised rotation gate tests**
The three `size-based rotation` tests share the same logical shape — "given this flushed size and this `maxSessionSizeMb`, should `rotateSessionForReplaySize` be called?" — and could be expressed as a single `it.each` table. The team's convention prefers parameterised tests for cases like this, and a table would make it easy to add new boundary scenarios (e.g. exactly at the threshold) without duplicating the `beforeEach` setup.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+1
to
+3
| --- | ||
| 'posthog-js': minor | ||
| --- |
Contributor
There was a problem hiding this comment.
@posthog/types not listed in changeset
The PR description marks @posthog/types as an affected library, and two files under packages/types/src/ are modified (posthog-config.ts and session-recording.ts). The changeset only lists posthog-js, so @posthog/types would not receive a version bump in the release. If the types package is published independently, downstream consumers that depend on @posthog/types directly (rather than via posthog-js) may be stuck on a stale type definition for SessionIdChangedCallback and SessionRecordingOptions.maxSessionSizeMb.
Rule Used: In the posthog/posthog-js repository, /packages/br... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: .changeset/replay-size-rotation.md
Line: 1-3
Comment:
**`@posthog/types` not listed in changeset**
The PR description marks `@posthog/types` as an affected library, and two files under `packages/types/src/` are modified (`posthog-config.ts` and `session-recording.ts`). The changeset only lists `posthog-js`, so `@posthog/types` would not receive a version bump in the release. If the types package is published independently, downstream consumers that depend on `@posthog/types` directly (rather than via `posthog-js`) may be stuck on a stale type definition for `SessionIdChangedCallback` and `SessionRecordingOptions.maxSessionSizeMb`.
**Rule Used:** In the posthog/posthog-js repository, /packages/br... ([source](https://app.greptile.com/posthog-org-19734/github/PostHog/posthog-js/-/custom-context?memory=1e5ca16f-9836-4241-953d-21bcf9017456))
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Contributor
|
Size Change: +6.92 kB (+0.04%) Total Size: 17 MB
ℹ️ View Unchanged
|
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- guard maxSessionSizeMb against non-finite values explicitly (Number.isFinite) rather than relying on the downstream !maxBytes falsy check - name the 1/500 MiB bounds (MIN/MAX_SESSION_SIZE_MB) and BYTES_PER_MIB instead of bare magic numbers - add overshootBytes to $session_size_rotation so the (multi-tab undercount driven) late firing is observable - document that rotation fires session-change handlers synchronously and they must not flush, and that maxSessionSizeMb=0 clamps to 1 rather than disabling Generated-By: PostHog Code Task-Id: b9b6f524-7f9d-421a-9b47-8e7131dd8536
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (specialists), paul-reviewer, xp-reviewer. security-audit skipped (no security surface in this diff). Verdict: 💬 APPROVE WITH NITSSound, well-scoped change — reuses the existing Actioned in
|
| Reviewer | Assessment |
|---|---|
| 🔍 qa-team | Design sound; single-tab re-entrancy/loop concerns don't hold. Multi-tab double-rotation + marker attribution worth attention; rest is hardening. |
| 👤 paul | High trust; worries are coupling + observability, not correctness — wanted the overshoot graphable (done) and the load-bearing ordering explained. |
| 📐 xp | Follows the grain; the one real pull is the OnceAndOnlyOnce duplication with the existing rotation core — extract a shared minter. |
Automated by QA Swarm — not a human review
Contributor
There was a problem hiding this comment.
Needs attention — 1 issue in 1 file
The logic and design remain solid after the refactor commit. The Number.isFinite guard, named constants, and overshootBytes addition are clean improvements. However, CI is still failing for the same reason I flagged previously: terser-mangled-names.json needs regeneration. This is blocking merge.
What this PR does
Adds size-based session rotation for replay recordings with a configurable maxSessionSizeMb option (default 300 MiB under 2026-05-30 defaults, clamped 1–500). The second commit adds Number.isFinite guard, named constants, overshootBytes in the custom event, and improved documentation.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<assessment>
The logic and design remain solid after the refactor commit. The `Number.isFinite` guard, named constants, and `overshootBytes` addition are clean improvements. However, CI is still failing for the same reason I flagged previously: `terser-mangled-names.json` needs regeneration. This is blocking merge.
</assessment>
<file name="packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts">
<issue location="packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:505">
CI still failing: `Write mangled property names` job fails at `git diff --exit-code` because `_maxSessionSizeBytes` and `_maybeRotateForSessionSize` are not in `terser-mangled-names.json`. Run the build (`pnpm build` in `packages/browser`) and commit the regenerated file. This was flagged in the previous review and remains unaddressed.
</issue>
</file>
Tag @mendral-app with feedback or questions. View session
| } | ||
| const clampedMb = Math.min(MAX_SESSION_SIZE_MB, Math.max(MIN_SESSION_SIZE_MB, configuredMb)) | ||
| return clampedMb * BYTES_PER_MIB | ||
| } |
Contributor
There was a problem hiding this comment.
bug (P2): CI still failing: Write mangled property names job fails at git diff --exit-code because _maxSessionSizeBytes and _maybeRotateForSessionSize are not in terser-mangled-names.json. Run the build (pnpm build in packages/browser) and commit the regenerated file. This was flagged in the previous review and remains unaddressed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts, line 505:
<issue>
CI still failing: `Write mangled property names` job fails at `git diff --exit-code` because `_maxSessionSizeBytes` and `_maybeRotateForSessionSize` are not in `terser-mangled-names.json`. Run the build (`pnpm build` in `packages/browser`) and commit the regenerated file. This was flagged in the previous review and remains unaddressed.
</issue>
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
Per-session replay size has an extreme tail: web p99.9 is ~338 MiB, ~0.06% of web sessions exceed 500 MiB, and the largest run into the tens of GiB. A small number of unbounded sessions hold a large share of replay storage, strain the player, and push ingestion limits. We want to bound any single recording without losing data or interrupting recording.
Changes
Adds size-based session rotation. When a recording session's flushed bytes reach a configurable budget, the SDK rotates to a new, linked session rather than stopping — so no single recording exceeds the budget, but the recording continues and nothing is dropped.
session_recording.maxSessionSizeMb— defaults to300under the2026-05-30defaults bundle (off otherwise), clamped to the 1–500 MiB range. It carries a comment noting it is normally only changed after interaction with the PostHog support team.sessionMaximumSizechange reason drivesSessionIdManager.rotateSessionForReplaySize(), which mints a fresh session and fires the existing$session_ending/$session_startinglinking events (so the player can stitch the timeline back together). Deliberately notresetSessionId(), which reads downstream as aposthog.reset()and would suppress linking.$session_size_rotationcustom event marks the point in the recording where the budget was reached ({ sizeBytes, thresholdBytes }).Units are MiB (matching the server-side
sizecolumn and the SDK's$sdk_debug_replay_flushed_size).Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Paul directed this, sized the cutoff against cross-team replay data (US + EU, via Metabase), and chose the parameters: a 300 MiB default with a 1–500 MiB range, rotation (not hard-stop, so customers keep their data), a session-ended event, and an in-recording custom event marking the rotation point. The threshold and behaviour were chosen because rotation neither saves storage nor meaningfully changes revenue at this scale — its purpose is bounding per-session size, so the value is a guardrail tunable by support rather than a pricing lever.
Decisions made along the way:
sessionMaximumSizechange reason rather than reusingresetSessionId(), because the latter suppresses the$session_endinglinking the player needs._clearBuffer()so the rotation custom events aren't discarded with the flushed buffer.2026-05-30defaults bundle (off for existing integrations until they opt in), matching howsplit_storage/persistence_save_debounce_msshipped.Verification (run locally, not manual browser testing): 353 tests pass across the recorder, sessionid, config, and flushed-size-tracker suites — including new coverage for the 300 default, the clamp, the rotation method minting a linked (non-reset) session, and the recorder emitting
$session_size_rotation+ rotating at the budget. Lint clean;@posthog/typestypechecks.Note: the underlying session-scoped flushed-size tracker merged separately in #3868; this PR is the rotation feature on top of it.
Created with PostHog Code