Skip to content

fix(replay): cover render lag and recycled cells in background capture masks - #1063

Merged
turnipdabeets merged 5 commits into
mainfrom
fix/replay-background-capture-mask-lag
Nov 13, 2026
Merged

turnipdabeets merged 5 commits into
mainfrom
fix/replay-background-capture-mask-lag

Conversation

@turnipdabeets

@turnipdabeets turnipdabeets commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Review bots on #1052 flagged two privacy gaps in how screenshotModeBackgroundCapture (experimental, off by default) masks frames. Both have shipped since 3.70.1 and are on main today.

Background capture samples mask geometry on main, renders with drawHierarchy off-main, samples again, and masked the per-owner union of the two samples (sweptRects):

  • Render lag. The off-main render copies the last committed frame, which can trail the first sample. A label sampled at y=100 then y=105 was masked over y=100–125, while its pixels could sit at y=95.
  • Recycled cells. An owner present in only one sample was masked only where it was sampled, not where it sat when the pixels were committed.

Changes:

  1. sweptRects is replaced by backgroundCaptureMaskRects, built on the settleVerdict banding the default capture path already uses:
Band Before After
Still (≤ 2pt) union of both samples union of both samples
Drift (≤ 50pt) union of both samples settleVerdict inflation, including the trail behind the first sample
Motion, or an unpaired owner union, or the one sampled rect frame skipped

Motion is skipped rather than re-rendered because the presentation-tree renderer the default path falls back to is main-only.

  1. An extra mask sample is now taken on main backgroundCaptureLeadSeconds (100ms) before the usual pre-render sample, so the render sits after three samples: lead, before, after. Each consecutive pair must be coverable on its own and the masks are their union, so anything that moves, appears or vanishes during the wait still skips the frame. All samples are released on main. On device, a render taken right after an abrupt stop still showed the pre-stop scroll position. Without the wait, change 1 made this worse: every moving frame was skipped, so the first frame accepted after a scroll landed inside that window, with each mask a row off its text. With the wait, a still verdict means the geometry held still for longer than that lag. In testing 25ms was already enough, so 100ms leaves margin for slower devices.

Trade-off: with this option on, frames are skipped while masked content scrolls or animates, so recordings jump from before to after a scroll. Recording resumes once motion stops; skipped frames don't touch the capture backoff. Each background capture also starts 100ms later.

💚 How did you test it?

On device (iPhone 14 Pro Max, screenshotModeBackgroundCapture on, throttleDelay 0.2s). A table of magenta text marked ph-no-capture is scrolled programmatically at fixed speeds with rests in between. Every screenshot is decoded in beforeSend and its magenta pixels counted (> 20 = leak). The same probe ran on a main build and on this branch:

Build Frames during 300 / 1000 / 2500 pt/s scrolls Leak frames Max magenta px in any frame
main 8 / 7 / 6 0 13 (slivers while scrolling)
Change 1 only 9 / 2 / 0 2, the first frame after each stop 34,181
Change 1 + 2 (3 runs) 3–5 / 0 / 0 0 0
Change 1 + 2, heavy cells (3 runs) 0 / 0 / 0 0 0
Three-sample version, light + heavy (1 run each) 3 / 0 / 0 and 0 / 0 / 0 0 0

Heavy cells add 30 un-pathed shadows, a blur and 2ms of main-thread work per cell. To check the margin, the wait was lowered to 50ms and 25ms in the probe build only: 0 leak frames across 5 light and 4 heavy runs.

Unit tests: PostHogSettleVerdictTest on Mac Catalyst (compiles the #if os(iOS) code), 25/25 including 9 new backgroundCaptureMaskRects tests. With the old sweptRects logic swapped back in, the render-lag, recycled-owner, appeared-owner and motion tests fail.

make lint: no serious violations. Not run locally: the iOS simulator tests (the test runner hangs before connecting, so CI's test-ios-simulator is the signal), make test (macOS SPM compiles this out) and make buildSdk.

Merging into the 4.0 branch: #1052 changes performBracketedBackgroundCapture so the first sample is released on main. This PR moves that sample into scheduleBracketedBackgroundCapture, so expect a conflict there; keep #1052's release-on-main behaviour.

📝 Checklist

  • All commits are signed (see Commit signing)
  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

  • Claude Code (Opus 5.5) wrote the fix after the bot findings on feat: keep screenshotModeBackgroundCapture in 4.0 #1052. That PR stays a straight revert; this fixes the shared code on main so 3.x gets it too.
  • Reusing settleVerdict was chosen over scaling thresholds by the measured render duration, which would add new tuning logic.
  • The device A/B probe caught that change 1 alone leaked after every scroll stop. The 100ms lead wait (change 2) is the fix, verified over three runs. The 100ms value is 4x the smallest wait that tested clean (25ms); it isn't tuned further. Only one device (iPhone 14 Pro Max) was tested.

🤖 Generated with Claude Code

…e masks

With screenshotModeBackgroundCapture, the off-main render can copy pixels
that trail the first mask sample, and an owner present in only one sample
was masked only where it was sampled. Mask background frames with
settleVerdict instead of sweptRects: still frames cover both samples,
drifted frames get the trail inflation, and motion or unpaired owners skip
the frame.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@turnipdabeets turnipdabeets self-assigned this Nov 12, 2026
@turnipdabeets turnipdabeets mentioned this pull request Nov 12, 2026
6 of 18 tasks
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

posthog-ios Compliance Report

Date: 2026-11-12T21:33:14.762831+00:00
Duration: 236271ms

⚠️ Some Tests Failed

46/47 tests passed, 1 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 2943ms
Format Validation.Event Has Uuid ✅ 2779ms
Format Validation.Event Has Lib Properties ✅ 2672ms
Format Validation.Distinct Id Is String ✅ 2799ms
Format Validation.Token Is Present ✅ 2752ms
Format Validation.Custom Properties Preserved ✅ 2784ms
Format Validation.Event Has Timestamp ✅ 2696ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 2718ms
Retry Behavior.Retries On 503 ✅ 11302ms
Retry Behavior.Does Not Retry On 400 ✅ 4922ms
Retry Behavior.Does Not Retry On 401 ✅ 4729ms
Retry Behavior.Respects Retry After Header ✅ 7774ms
Retry Behavior.Implements Backoff ✅ 21253ms
Retry Behavior.Retries On 500 ✅ 8789ms
Retry Behavior.Retries On 502 ✅ 9225ms
Retry Behavior.Retries On 504 ✅ 9317ms
Retry Behavior.Max Retries Respected ❌ 21374ms
Deduplication.Generates Unique Uuids ✅ 2876ms
Deduplication.Preserves Uuid On Retry ✅ 9253ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 16276ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 7797ms
Deduplication.No Duplicate Events In Batch ✅ 2753ms
Deduplication.Different Events Have Different Uuids ✅ 2737ms
Compression.Sends Gzip When Enabled ✅ 2767ms
Batch Format.Uses Proper Batch Structure ✅ 2794ms
Batch Format.Flush With No Events Sends Nothing ✅ 305ms
Batch Format.Multiple Events Batched Together ✅ 2851ms
Error Handling.Does Not Retry On 403 ✅ 4726ms
Error Handling.Does Not Retry On 413 ✅ 2309ms
Error Handling.Retries On 408 ✅ 9154ms

Failures

retry_behavior.max_retries_respected

Expected 4 requests, got 6

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 2797ms
Request Payload.Flags Request Uses V2 Query Param ✅ 2763ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 2805ms
Request Payload.Flags Request Omits Authorization Header ✅ 2661ms
Request Payload.Token In Flags Body Matches Init ✅ 2781ms
Request Payload.Groups Round Trip ✅ 2753ms
Request Payload.Groups Default To Empty Object ✅ 2785ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 2700ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 2813ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 2765ms
Request Lifecycle.No Flags Request On Init Alone ✅ 58ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 2691ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 5455ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 2867ms
Retry Behavior.Retries Flags On 502 ✅ 2803ms
Retry Behavior.Retries Flags On 504 ✅ 2821ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 3013ms

@turnipdabeets
turnipdabeets marked this pull request as ready for review November 12, 2026 19:55
@turnipdabeets
turnipdabeets requested a review from a team as a code owner November 12, 2026 19:55
@veria-ai

veria-ai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

Reviews

@turnipdabeets
turnipdabeets marked this pull request as draft November 12, 2026 20:04
…aught up

On device, the off-main render still showed the pre-stop scroll position
for 30-40ms after an abrupt stop. Skipping every moving frame meant the
first accepted frame landed in that window, with masks a row off the text.
Take the first mask sample 100ms before the render, so a still verdict
spans longer than that lag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@turnipdabeets
turnipdabeets marked this pull request as ready for review November 12, 2026 20:14

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 4ec8754: 1 Must fix, 1 Should fix.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

turnipdabeets and others added 2 commits November 12, 2026 15:46
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The lead wait moved the only pre-render sample 100ms earlier, so content
that moved, appeared or vanished during the wait could go unmasked, and the
delayed sample could release its views on the replay queue. Sample again
right before the render, require each consecutive pair to be coverable, and
release every sample inside the final main.sync.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 27303a4: 1 Should fix.

@dustinbyrne dustinbyrne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo another reason for considering dropping screenshotModeBackgroundCapture altogether - we're basically stuck evaluating heuristics to keep content masked

@turnipdabeets

Copy link
Copy Markdown
Contributor Author

imo another reason for considering dropping screenshotModeBackgroundCapture altogether - we're basically stuck evaluating heuristics to keep content masked

Cc @ioannisj

@turnipdabeets
turnipdabeets merged commit d401bd7 into main Nov 13, 2026
53 checks passed
@turnipdabeets
turnipdabeets deleted the fix/replay-background-capture-mask-lag branch November 13, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants