Conversation
…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
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 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}
- 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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
- 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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e8e1309. Configure here.
…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.
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).
Leaving it true is intentional: the next 'active' event retries the restart. Resetting it would suppress that implicit retry.
| inactiveStopTimeout = setTimeout(() => { | ||
| inactiveStopTimeout = null; | ||
| stopIfNeeded(); | ||
| }, IOS_INACTIVE_STOP_DELAY_MS); | ||
| } |
There was a problem hiding this comment.
Bug: On iOS, after a replay is restarted, a subsequent 'inactive' state lasting 5+ seconds (e.g., a phone call) will incorrectly stop the replay because stopIfNeeded() is called unconditionally.
Severity: MEDIUM
Suggested Fix
In stopIfNeeded(), add a guard to prevent stopping the replay if the app is not actually backgrounding. This could be achieved by checking if isBackgrounded is false. Alternatively, prevent the 'inactive' timer from being scheduled if a replay has been successfully restarted and is not in a backgrounding transition.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/js/replay/foregroundReplayGuard.ts#L153-L157
Potential issue: After a replay session is successfully restarted (e.g., after the app
returns to the foreground), the logic for the `'inactive'` state unconditionally
schedules `stopIfNeeded()` to run after 5 seconds. This is intended as a fallback for
when the app is backgrounding. However, `stopIfNeeded()` does not check if the app is
actually in the process of backgrounding (e.g., by checking an `isBackgrounded` flag).
Consequently, if the app enters an 'inactive' state for 5+ seconds (like during a phone
call) while a replay is active, the timer will fire and incorrectly stop the running
replay session, leading to data loss.
There was a problem hiding this comment.
Not valid as suggested: checking isBackgrounded before the inactive-fallback stop would defeat the fallback's own purpose - it exists precisely for when 'background' never arrives (isBackgrounded never becomes true), e.g. JS suspended before delivery. Same 5s threshold/trade-off is already accepted for the identical scenario in onSpanEndUtils.cancelInBackground (IOS_INACTIVE_CANCEL_DELAY_MS). Leaving as-is.
| * @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?
| replay?.flush(); // flush the buffered replay to Sentry | ||
| ``` | ||
|
|
||
| - Add `avoidForegroundResumeHang` (iOS) to `mobileReplayIntegration` to work around a fatal App Hang that can occur when Session Replay resumes capture on returning to the foreground with a heavy view hierarchy on screen ([#6727](https://github.com/getsentry/sentry-react-native/pull/6727)) |
There was a problem hiding this comment.
Q: Since this is mitigating a feature bug I wonder if it should be moved in the fixes section 🤔
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.
2 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