Repository navigation
Conversation
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. |
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- Evict stale async TurboModule tracker frames ([#6824](https://github.com/getsentry/sentry-react-native/pull/6824))If none of the above apply, you can opt out of this check by adding |
`wrapTurboModule` pops an async frame only when its promise settles. A native method that accepts a promise but never settles it would pin its frame — and, via the scope sync, the native crash scope — for the whole process lifetime, so every later crash is mis-attributed to that call (the Android shape of #6821, but also any custom module passed to `turboModuleContextIntegration({ modules })`). Add an opportunistic, budgeted age sweep on push that drops async frames older than MAX_ASYNC_FRAME_AGE_MS (60s, matching CALLBACK_MAX_AGE_MS), reusing popTurboModuleCall for the scope re-sync. No timers. Bounds the damage to "attribution expires ~60s after the call started" instead of "wrong forever". Sync and callback frames are untouched — the wrapper already pops those synchronously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4a9a0a7 to
80f5785
Compare
|
@cursor review |
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| * frames are always popped synchronously by the wrapper. | ||
| */ | ||
| function evictStaleAsyncFrames(nowMs: number): void { | ||
| let staleIds: number[] | undefined; | ||
| let budget = ASYNC_FRAME_SWEEP_BUDGET; | ||
| for (const frame of stack) { |
There was a problem hiding this comment.
Bug: The evictStaleAsyncFrames function performs an O(n) scan of the entire async frame stack on every call push. The stack size is unbounded, creating a potential performance bottleneck.
Severity: LOW
Suggested Fix
To mitigate the O(n) scan, introduce a hard cap on the stack size, similar to MAX_PENDING_CALLBACK_CALLS used for callbacks. This would prevent the stack from growing indefinitely and limit the worst-case performance of the scan. Alternatively, if the stack can be chronologically ordered, modify the scan to break early when encountering the first non-stale frame.
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/turbomodule/turboModuleTracker.ts#L266-L271
Potential issue: The `evictStaleAsyncFrames` function, called on every
`pushTurboModuleCall`, iterates through the entire `stack` array. Unlike the callback
sweep mechanism, this scan does not break early when encountering non-stale entries.
Furthermore, the async frame `stack` has no hard size limit, allowing it to grow
unbounded. In a scenario with many concurrent, unresolved async TurboModule calls, the
stack can become very large. This results in an O(n) scan on a hot path, where `n` is
the potentially large stack size. This can cause performance degradation, such as
latency or frame drops, under heavy load.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 80f5785. Configure here.
|
Closing for now since the case covered is hypothetical at this point. |
1 participant
📢 Type of change
📜 Description
Hardens the TurboModule crash-attribution tracker against never-settling promises.
wrapTurboModulepops an async call's frame only when the returned promise settles (wrapTurboModule.ts— theisThenablebranch pops in both.thenhandlers). A native method that accepts aPromisebut never settles it therefore pins its frame forever, andpopTurboModuleCallre-syncs the native scope onto that leaked frame after every later pop — so every subsequent native crash is mis-attributed to it.This is the class-level cause behind #6821 (where Android's
initNativeReactNavigationNewFrameTrackingdropped its promise). The native fix for that specific method is #6823; this PR prevents the same shape from recurring via a future SDK method or a user module passed toturboModuleContextIntegration({ modules }).The guard is an opportunistic, budgeted age sweep on
pushTurboModuleCall:MAX_ASYNC_FRAME_AGE_MS(60s) are evicted on the next push, reusingpopTurboModuleCallso the scope re-sync / clear logic stays in one place.evictStalePendingCallbackCalls). This deliberately avoids the import-time/lingering-timer class fixed in fix(tracing): Don't start AsyncExpiringMap cleanup interval at import time #6811.ASYNC_FRAME_SWEEP_BUDGET(8) per push to keep it amortised O(1).asyncframes expire;syncand callback-style frames are always popped synchronously by the wrapper, so they're left untouched.The 60s bound and the sweep mechanism mirror the existing
CALLBACK_MAX_AGE_MS/evictStalePendingCallbackCallsguard on the callback path (added in #6542) — a never-settling call leaks both a frame and a pending-callback entry, and both now age out at the same bound.💡 Motivation and Context
Follow-up to #6821 / #6823. The native fix closes the only current instance; this closes the class, so a dangling promise (ours or a user module's) can't pin crash attribution for the process lifetime. Turns "wrong attribution forever" into "attribution expires ~60s after the call started".
💚 How did you test it?
Added 5 tracker tests (fake timers) covering: eviction of a leaked async frame on the next push, full scope clearing afterwards, no eviction within the age bound, sync frames never evicted, and budget-bounded multi-frame eviction. Full turbomodule +
turboModuleContextIntegrationsuites pass (113 tests);yarn build:sdk, oxlint andcircularDepCheckclean.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
The bound is an internal constant for now. If a use case needs it tunable, it can later be surfaced as an option on
turboModuleContextIntegration(public-API change — would need an API-report regen + review).🤖 Generated with Claude Code