Skip to content

fix(core): Prevent recursion when a callback triggers another capture#5737

Merged
runningcode merged 12 commits into
mainfrom
no/java-callback-reentrancy-guard
Jul 20, 2026
Merged

fix(core): Prevent recursion when a callback triggers another capture#5737
runningcode merged 12 commits into
mainfrom
no/java-callback-reentrancy-guard

Conversation

@runningcode

@runningcode runningcode commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

📜 Description

Adds a core-level re-entrancy guard so a user callback (beforeSend, beforeBreadcrumb, beforeSendLog, and the other beforeSend* variants) that triggers another SDK capture no longer recurses into a StackOverflowError.

💡 Motivation and Context

These callbacks run synchronously on the caller thread with only a try/catch — nothing prevents re-entrant capture. So a callback that captures again — directly, or transitively through a logging integration that routes back into Sentry — loops forever:

captureX()executeBeforeX() → callback logs/captures → (integration) → captureX() → …

This is the same class of bug as the logcat StackOverflowError (SDK-CRASHES-JAVA-3T3H, fixed per-integration in #5734). It also affects the Timber integration (SentryTimberTree fires captureEvent + addBreadcrumb + logger().log() on every Timber call) and any app code that captures from within a callback. Rather than patch each integration, this fixes the whole class once in the core.

Solution
SentryCallbackReentrancyGuard is a shared thread-local flag set only around each callback's execute(...). The capture entry points (SentryClient.captureEvent/captureTransaction/captureLog, Scope.addBreadcrumb) drop a nested capture while the flag is active, breaking the loop across all capture types on the same thread. Captures made by event processors run outside the flag and are unaffected. The real Log.* output in integrations is untouched.

Dropping the nested capture (rather than sending it without running beforeSend) is deliberate — bypassing beforeSend would risk sending unscrubbed PII, which the SDK already guards against.

💥 Behavioral change

A capture a callback makes intentionally (e.g. a diagnostic event, or re-capturing an enriched event) is now dropped while that callback runs, and logged at DEBUG. This is the safe trade-off — such a nested capture is indistinguishable from recursion. Event processors are not affected. The per-integration guard added in #5734 becomes redundant (left in place; harmless).

💚 How did you test it?

  • SentryClientTest: beforeSendcaptureEvent, beforeSendLog→log, and cross-type (beforeSend emitting a log) — each verified to StackOverflowError/fail without the guard and pass with it, with the nested capture dropped.
  • ScopeTest: beforeBreadcrumbaddBreadcrumb.
  • SentryTimberIntegrationTest: end-to-end with a real Sentry.init, a planted SentryTimberTree, and a beforeSend that calls Timber.e(...) — verified to recurse without the guard and to run the callback exactly once with it.

Fixes SDK-CRASHES-JAVA-3T3H

JAVA-638

@linear-code

linear-code Bot commented Jul 7, 2026

Copy link
Copy Markdown

JAVA-638

Comment thread sentry/src/main/java/io/sentry/util/SentryCallbackReentrancyGuard.java Outdated
@sentry

sentry Bot commented Jul 7, 2026

Copy link
Copy Markdown

📲 Install Builds

Android

🔗 App Name App ID Version Configuration
SDK Size io.sentry.tests.size 8.49.0 (1) release

⚙️ sentry-android Build Distribution Settings

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 326.13 ms 361.94 ms 35.81 ms
Size 0 B 0 B 0 B

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5b1a06b 310.56 ms 362.79 ms 52.22 ms
ad8da22 339.92 ms 407.37 ms 67.45 ms
d15471f 307.28 ms 381.85 ms 74.57 ms
2195398 345.88 ms 411.71 ms 65.82 ms
ee747ae 396.82 ms 441.67 ms 44.86 ms
abfcc92 304.04 ms 370.33 ms 66.29 ms
22f4345 312.78 ms 347.40 ms 34.62 ms
d15471f 304.55 ms 408.43 ms 103.87 ms
ad8da22 362.98 ms 453.94 ms 90.96 ms
bbc35bb 298.53 ms 372.17 ms 73.64 ms

App size

Revision Plain With Sentry Diff
5b1a06b 0 B 0 B 0 B
ad8da22 1.58 MiB 2.29 MiB 719.83 KiB
d15471f 1.58 MiB 2.13 MiB 559.54 KiB
2195398 0 B 0 B 0 B
ee747ae 1.58 MiB 2.10 MiB 530.95 KiB
abfcc92 1.58 MiB 2.13 MiB 557.31 KiB
22f4345 1.58 MiB 2.29 MiB 719.83 KiB
d15471f 1.58 MiB 2.13 MiB 559.54 KiB
ad8da22 1.58 MiB 2.29 MiB 719.83 KiB
bbc35bb 1.58 MiB 2.12 MiB 553.01 KiB

Previous results on branch: no/java-callback-reentrancy-guard

Startup times

Revision Plain With Sentry Diff
93997ce 312.40 ms 370.43 ms 58.03 ms
61d85ae 310.71 ms 362.96 ms 52.25 ms
e594dca 313.87 ms 374.69 ms 60.82 ms
8e81c78 314.80 ms 361.58 ms 46.78 ms

App size

Revision Plain With Sentry Diff
93997ce 0 B 0 B 0 B
61d85ae 0 B 0 B 0 B
e594dca 0 B 0 B 0 B
8e81c78 0 B 0 B 0 B

Comment thread sentry/src/main/java/io/sentry/SentryClient.java

@adinauer adinauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, we could consider also handling EventProcessor, onOversizedEvent, captureEnvelope and scope callbacks on things like Scopes.captureException.

Comment thread sentry/src/main/java/io/sentry/util/SentryCallbackReentrancyGuard.java Outdated
@runningcode
runningcode force-pushed the no/java-callback-reentrancy-guard branch from 4c2bc51 to 7dd8463 Compare July 20, 2026 09:40

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit df2c1d2. Configure here.

Comment thread sentry/src/main/java/io/sentry/SentryClient.java
runningcode and others added 12 commits July 20, 2026 14:00
A user beforeSend/beforeBreadcrumb/beforeSendLog callback that itself
captures — directly, or transitively through a logging integration that
routes back into Sentry (e.g. Timber, or the Gradle plugin's logcat
instrumentation) — recursed until a StackOverflowError, because the callbacks
run synchronously on the caller thread with no re-entrancy protection.

Add a shared, thread-local SentryCallbackReentrancyGuard that is set only
while a callback's execute() runs. Capture entry points (captureEvent,
captureTransaction, captureLog, Scope.addBreadcrumb) drop nested captures
while the guard is active, breaking the loop for every capture type at once.
Dropping (rather than sending) the nested capture is intentional: bypassing
beforeSend would send unscrubbed data.

The nested capture is dropped, not sent. This also makes the per-integration
guard in SentryLogcatAdapter redundant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These three capture methods had their beforeSend* executors wrapped by the
re-entrancy guard but no entry check, so they were never dropped when a
callback was active. That broke the "callbacks never nest" invariant: a
callback that captured feedback/replay/metric ran that executor, whose exit()
cleared the shared flag mid-callback, re-enabling recursion for any
captureEvent/captureLog that followed in the same callback. They also recursed
directly (e.g. beforeSendFeedback -> captureFeedback).

Add the same isActive() entry guard to all three so every executor-wrapped
capture path also drops while a callback runs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g callbacks

Two robustness fixes for the callback re-entrancy guard:

Replace the boolean flag with a depth counter so a nested exit() cannot
disarm the guard while an outer callback is still running. The boolean
relied on the "callbacks never nest" invariant, which every capture entry
point must uphold by convention - a future capture path added without an
entry check would silently re-open the recursion hole. The counter makes
that failure mode structurally impossible, and lets exit() remove() the
thread-local entry instead of parking a stale value on pooled threads.

Wrap beforeErrorSampling and beforeEnvelopeCallback, the two remaining
user callbacks in SentryClient without enter()/exit(). A capture from
within beforeErrorSampling recursed unguarded (captureEvent ->
beforeErrorSampling -> captureEvent -> ...). Both regression tests were
verified to fail with StackOverflowError without the wraps.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the manual enter()/finally-exit() pattern at every callback site
with an ISentryLifecycleToken returned from enter(), used via
try-with-resources. This removes nine copies of the finally boilerplate
and the risk of forgetting the exit() call.

The depth counter stays as the guard's state model - the token's close()
just decrements it - so the nesting correctness guarantee is unchanged.
The token is a shared static singleton, so no allocation happens per
callback, matching the AutoClosableReentrantLock idiom already in the
codebase.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rebase folded the previous Unreleased section into the released
8.48.0, stranding the callback re-entrancy entry there. Move it back
under Unreleased.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
captureEnvelope and captureCheckIn route into sendEnvelope without the
isActive() entry check every other capture path has. A beforeEnvelope
callback that itself captured an envelope or check-in re-entered
sendEnvelope, re-ran the callback, and recursed to a StackOverflowError -
the same failure class the rest of this PR fixes.

Check the guard at the top of sendEnvelope, the single choke point every
send flows through, rather than adding a check to each public entry
point. In normal flow the guard is already inactive there (the before*
callback has exited), so an active guard can only mean a callback
triggered the send, which is dropped. This also closes the hole for any
future sender routed through sendEnvelope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
captureCheckIn and captureEnvelope both funnel into sendEnvelope and hit
the same isActive() guard, so the check-in test exercised no code path
the envelope test did not already cover.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The re-entrancy guard dropped nested captures but logged a DEBUG line at
seven of the eight drop sites. That log is itself routed back into Sentry
by the same logging integrations this guard protects against (the Gradle
plugin's logcat instrumentation feeds captureLog, whose own drop logs
again), so logging on the drop path re-opens the very recursion the guard
breaks. It only bites with SDK debug logging enabled, since the internal
logger is otherwise a no-op, but dropping silently removes the failure
mode outright.

Drop without logging everywhere and document why in the guard's Javadoc
and at each drop site.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A customer implementing beforeSend/beforeBreadcrumb/beforeSendLog/etc.
has no way to know that capturing from within the callback — directly or
through a logging integration — is silently dropped to prevent recursion.
State the contract on each customer-facing callback interface.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The token refactor changed SentryCallbackReentrancyGuard.enter() to
return an ISentryLifecycleToken and made exit() private, but the API dump
was not regenerated at the time. The guard is @ApiStatus.Internal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@runningcode
runningcode force-pushed the no/java-callback-reentrancy-guard branch from 1dd59c9 to b328ffa Compare July 20, 2026 12:01
@runningcode
runningcode enabled auto-merge (squash) July 20, 2026 12:14
@runningcode
runningcode merged commit 747ff0b into main Jul 20, 2026
70 of 71 checks passed
@runningcode
runningcode deleted the no/java-callback-reentrancy-guard branch July 20, 2026 12:17
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.

3 participants