feat(replay): add avoidForegroundResumeHang to mitigate iOS foreground App Hang - #6727
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
Plus 1 more 🤖 This preview updates automatically when you update the PR. |
|
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall LGMT after the full test suite is 🟢 (blocked on merging and resolving the changelog conflict).
Worth checking the last Sentry comment and the not raised Warden findings (if not fixed already).
| * @default false | ||
| * @platform ios | ||
| */ | ||
| avoidForegroundResumeHang?: boolean; |
There was a problem hiding this comment.
Q: Since this is an API change though not marked as such let's also loop in other mobile folks. Also, is there a way to fix that on the Cocoa side? Should we mark the additions as experimental to have the flexibility of removing them?
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Overall this approach LGTM as a temporary mitigation. Let's merge the latest from main and resolve the conflict to run the full ci suite.
Added a couple of questions. I think that we should raise this with the iOS folks before shipping this. If we decide to ship I would advocate on using an experimental flag and opening a Cocoa issue to track a follow up fix on the iOS side.
|
Q: This sounds like a Sentry Cocoa issue not Sentry React Native, if so, shouldn't this be fixed on Sentry Cocoa (and with that fixing the issue on other hybrid/native SDKs)? |
…d App Hang sentry-cocoa resumes Session Replay capture synchronously on UIApplicationDidBecomeActiveNotification. On a heavy view hierarchy this can block the main thread past iOS's foreground-transition watchdog and get the app killed. A manual pause()/resume() doesn't help - cocoa's automatic resume shares the same state and unconditionally re-arms capture regardless of it. When enabled, an AppState listener stops replay just before the app backgrounds (which makes the automatic resume a safe no-op) and restarts it in buffer mode a short delay after returning to the foreground, off the watchdog window. Fixes #6701
- Stale cached replay id: the guard now calls into mobilereplay.ts's own cache-invalidation path instead of raw NATIVE.stopReplay/ startReplayBuffering, so getReplayId()/DSC/metric linking don't keep pointing at the pre-background session. - Dropped restart on interrupting state: the state machine no longer clears the pending restart on every AppState event - only on an actual background/active transition, with a stoppedByGuard flag instead of an eagerly-reset boolean. - Never detached: setupForegroundReplayGuard now registers detach on the client's 'close' hook. - Missing inactive handling: iOS can suspend JS between 'inactive' and 'background', so 'background' may never arrive. Mirrors onSpanEndUtils' cancelInBackground pattern - a delayed, cancelable stop on 'inactive' as a fallback.
- stoppedByGuard was cleared before startReplayBuffering() settled, so a background event landing while the restart was still in flight could miss stopping the newly-started (unprotected) session. Track the in-flight restart separately and stop the new session once it resolves if a background event arrived in the meantime. - A rejected stopReplay() left stoppedByGuard set to true, so a later foreground event would schedule a restart against a session that may never have actually stopped. Reset the flag on stop failure instead. Also moves the restart delay's default (1000ms) into setupForegroundReplayGuard itself, keeping the mobilereplay.ts call site short enough to avoid the formatter wrapping it across extra lines.
Merges near-duplicate cases (repeated background/active events tested across separate its; the two cache-invalidation tests) and removes a test that duplicated the "stopReplay rejects" scenario with only a different assertion. No coverage lost - one regression case remains per behavior/bug found, just fewer redundant setups.
26 tests / 540 lines was excessive for this module. Down to 11 tests / 287 lines: one test per real behavior or regression (each bug found in review still has a dedicated case), dropped separate tests for symmetric/lower-risk paths (e.g. startReplayBuffering rejection logging, detach's inactive-fallback branch) and combined idempotency checks into the tests they naturally belong to instead of standalone cases.
- detach() only cleared timers, not an in-flight restart's own promise callback. If the client closed while a restart was underway and a background event had landed, the callback could still fire stopReplay() after close. Added a detached flag checked before any such post-detach side effect. - restart() fired startReplayBuffering() purely on a fixed delay, assuming that was enough time for stopReplay() to finish. If stop is slow (or the configured delay is short), the calls could overlap. restart() now waits for the in-flight stop to actually settle first.
…tart backgroundedDuringRestart was set once (on a background event landing mid-restart) and never revisited. If the app went active again before the restart settled, its resolution still stopped the just-started session based on that stale flag - and nothing rescheduled a restart, since we were already active. Replaced it with isBackgrounded, updated on every transition and checked (not latched) when the restart resolves, so the decision reflects where we ended up, not where we were partway through.
Leaving it true is intentional: the next 'active' event retries the restart. Resetting it would suppress that implicit retry.
Merging with the per-class masking PR (#6725) pushed mobilereplay.ts back over the 300-line lint budget.
|
@lucas-zimerman good point, and you're right that the root cause is in sentry-cocoa ( |
📲 Install BuildsAndroid
|
yarn fix doesn't enforce max-lines (only lint:oxlint's --deny-warnings does), so the previous brace removal was masking the real budget issue rather than fixing it. Compacting the pre-existing merged-object literal in mergeOptions frees enough room to keep braces on the guard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e7e9f8f. Configure here.
| } | ||
| if (stoppedByGuard) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard skips later session stop
Medium Severity
stopIfNeeded returns early while stoppedByGuard is true, so a replay started after the guard's own stop is left running. A getReplay().start() (or similar) during the foreground delay, followed by another background, skips stopReplay(). Cocoa then resumes that live session on the next foreground, which is the hang this option is meant to prevent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e7e9f8f. Configure here.
There was a problem hiding this comment.
Real but narrow scope: this needs the app to call replay.start()/startBuffering() manually while avoidForegroundResumeHang is also managing it automatically. Same category as the existing manual pause()-interaction caveat - documented limitation, not fixing: mixing manual replay controls with this automatic guard isn't supported.
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a2585ce+dirty | 426.36 ms | 483.26 ms | 56.90 ms |
| 882f8ae+dirty | 399.98 ms | 427.06 ms | 27.08 ms |
| af33f3b+dirty | 434.90 ms | 506.14 ms | 71.24 ms |
| ecf47a2+dirty | 420.40 ms | 458.02 ms | 37.62 ms |
| acd838e+dirty | 422.63 ms | 462.39 ms | 39.76 ms |
| 7fd0012+dirty | 444.73 ms | 470.08 ms | 25.35 ms |
| 580fb5c+dirty | 436.34 ms | 471.63 ms | 35.28 ms |
| 5ca03f9+dirty | 423.30 ms | 467.00 ms | 43.70 ms |
| 5c1e987+dirty | 423.52 ms | 471.64 ms | 48.12 ms |
| 822d35b+dirty | 429.31 ms | 498.04 ms | 68.73 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a2585ce+dirty | 49.74 MiB | 55.36 MiB | 5.61 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| af33f3b+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| ecf47a2+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| acd838e+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 7fd0012+dirty | 50.56 MiB | 56.46 MiB | 5.90 MiB |
| 580fb5c+dirty | 49.74 MiB | 54.79 MiB | 5.05 MiB |
| 5ca03f9+dirty | 49.74 MiB | 55.26 MiB | 5.52 MiB |
| 5c1e987+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 822d35b+dirty | 49.74 MiB | 54.84 MiB | 5.10 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 64630e5+dirty | 3842.70 ms | 1218.11 ms | -2624.60 ms |
| b0d3373+dirty | 3831.75 ms | 1227.29 ms | -2604.46 ms |
| b04af96+dirty | 3818.92 ms | 1219.76 ms | -2599.16 ms |
| 3d31fcf+dirty | 3838.09 ms | 1223.46 ms | -2614.63 ms |
| a0a3177+dirty | 3844.73 ms | 1225.23 ms | -2619.51 ms |
| af33f3b+dirty | 3849.98 ms | 1236.45 ms | -2613.53 ms |
| 09a902f+dirty | 3835.67 ms | 1217.11 ms | -2618.57 ms |
| 5a316ea+dirty | 3820.11 ms | 1211.28 ms | -2608.83 ms |
| acd838e+dirty | 3849.78 ms | 1230.00 ms | -2619.78 ms |
| c2e182c+dirty | 3848.40 ms | 1211.79 ms | -2636.61 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 64630e5+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| b04af96+dirty | 4.98 MiB | 6.54 MiB | 1.56 MiB |
| 3d31fcf+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
| a0a3177+dirty | 4.98 MiB | 6.55 MiB | 1.58 MiB |
| af33f3b+dirty | 4.98 MiB | 6.51 MiB | 1.54 MiB |
| 09a902f+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 5a316ea+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| acd838e+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| c2e182c+dirty | 4.98 MiB | 6.50 MiB | 1.52 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3b6e9f9+dirty | 3822.77 ms | 1208.00 ms | -2614.77 ms |
| 4e0b819+dirty | 3828.96 ms | 1205.64 ms | -2623.32 ms |
| 1e5d96d+dirty | 3845.93 ms | 1222.51 ms | -2623.42 ms |
| 0a9e622+dirty | 3825.35 ms | 1219.04 ms | -2606.31 ms |
| 5ca03f9+dirty | 3873.76 ms | 1236.26 ms | -2637.51 ms |
| 40c9884+dirty | 3826.11 ms | 1217.04 ms | -2609.07 ms |
| 57e0069+dirty | 3842.23 ms | 1210.00 ms | -2632.23 ms |
| 7ac3378+dirty | 1202.35 ms | 1198.31 ms | -4.04 ms |
| 20fbd51+dirty | 3832.52 ms | 1206.13 ms | -2626.39 ms |
| 822d35b+dirty | 3841.52 ms | 1221.75 ms | -2619.77 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3b6e9f9+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 4e0b819+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 1e5d96d+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 0a9e622+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 5ca03f9+dirty | 4.98 MiB | 6.53 MiB | 1.55 MiB |
| 40c9884+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 57e0069+dirty | 4.98 MiB | 6.50 MiB | 1.52 MiB |
| 7ac3378+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 20fbd51+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 822d35b+dirty | 4.98 MiB | 6.50 MiB | 1.53 MiB |
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
@alwx since we will soon expose the new SR start/stop api (in 8.27.0) wdyt of providing instructions to the user to use that directly on their app rather than adding permanent public surface (non experimental). This would give us some time to investigate a proper fix on the iOS side.
Something like the following might work as a recipe (haven't reproduced the issue to verify):
export function installForegroundReplayGuard({ delayMs = 1000 } = {}) {
if (Platform.OS !== 'ios') return () => {};
let stoppedByGuard = false;
let resumeTimer;
const clearResume = () => {
if (resumeTimer) {
clearTimeout(resumeTimer);
resumeTimer = undefined;
}
};
const subscription = AppState.addEventListener('change', state => {
const replay = Sentry.getReplay();
if (!replay) return;
if (state === 'background' || state === 'inactive') {
clearResume();
// Only stop if a replay is actually running, and only once.
if (!stoppedByGuard && replay.getReplayId()) {
stoppedByGuard = true;
replay.stop().catch(() => {
stoppedByGuard = false; // let the next background retry
});
}
} else if (state === 'active' && stoppedByGuard) {
clearResume();
resumeTimer = setTimeout(() => {
resumeTimer = undefined;
stoppedByGuard = false;
replay.startBuffering();
}, delayMs);
}
});
// Call this on teardown (e.g. logout) if you ever need to stop guarding.
return () => {
clearResume();
subscription.remove();
};
}
|
@antonis I'd lean against a docs recipe here because your own snippet already reintroduces about half the races I spent this PR fixing (no in-flight-restart tracking, latched flag instead of a live state check, no await on the pending stop, no detach guard). |
| expect(deps.startReplayBuffering).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not stop replay again after detach, even if a restart was still in flight', async () => { |
There was a problem hiding this comment.
In-flight restart can start replay after client close
Before calling startReplayBuffering() in restart(), bail out when detached is set so client close cannot leave a new native replay session running.
Evidence
detach()only setsdetached = trueand clears timers; it does not cancel an in-flightrestart()(foregroundReplayGuard.ts).restart()always callsdeps.startReplayBuffering()afterpendingStopwith nodetachedguard before the start.setupForegroundReplayGuardregisters thatdetachonclient.on('close', detach), so close can land while a delayed restart is already running.- The test starting at line 207 only asserts no follow-up
stopReplayafter detach, not that start is aborted.
Also found at 1 additional location
packages/core/test/replay/foregroundReplayGuard.test.ts:208-228
Identified by Warden · code-review · WBX-44Y
There was a problem hiding this comment.
Confirmed and fixed in 18012de: restart() now checks detached before calling startReplayBuffering(), not just in the post-success stopIfNeeded() branch.
A stopgap for a sentry-cocoa issue, not a permanent API commitment. Per antonis's review - avoids locking in new public surface while the proper fix is pursued upstream.
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
I'd rather mark it @experimental then as that would get us the same "no permanent commitment" property, while users get the tested implementation, wdyt?
Thank you. Let's ship that as experimental and open an issue to track this on iOS.
LGTM as a mitigation and to get feedback from the user 👍
restart() guarded the post-success stopIfNeeded() call against a detach that happened mid-restart, but not the startReplayBuffering() call itself: closing the client while still waiting on pendingStop left the scheduled restart free to start a new native session anyway.
3 participants
📢 Type of change
📜 Description
Adds an opt-in
avoidForegroundResumeHangoption tomobileReplayIntegration()(iOS only). When enabled, anAppStatelistener stops Session Replay just before the app backgrounds and restarts it in buffer mode a short delay after returning to the foreground.💡 Motivation and Context
sentry-cocoa resumes Session Replay capture synchronously on
UIApplicationDidBecomeActiveNotification. On a heavy view hierarchy this can block the main thread past iOS's foreground-transition watchdog and get the app killed (Fatal App Hang Fully Blocked).The already-shipped
pause()/resume()runtime controls (#6703) don't help here: cocoa's automatic resume shares the same underlying state and unconditionally re-arms capture regardless of a prior manualpause().stopReplay()is different - it tears down the native session entirely, so the automatic resume becomes a safe no-op. This option uses that to stop replay before backgrounding and restart it (in buffer mode) safely outside the watchdog window on foreground.Investigated and confirmed there's no way to have it resume in the exact prior mode (session vs. buffer) - neither sentry-cocoa nor sentry-java currently expose that via the surfaces this SDK can reach, so it always restarts in buffer mode. Documented as a trade-off on the option.
Fixes #6701
💚 How did you test it?
foregroundReplayGuardmodule (background/foreground transitions, pending-restart cancellation, native rejection handling) and for themobileReplayIntegrationwiring.yarn build,yarn test,yarn lint,yarn circularDepCheck,yarn api-report:checkall pass (no public API change - the new option isn't exported from the barrel).📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps