fix(tracing): End timed-out transactions when the timeout is due - #6091
fix(tracing): End timed-out transactions when the timeout is due#6091runningcode wants to merge 8 commits into
Conversation
…AVA-642) The idle and deadline timers run on a thread that is frozen while the device is in deep sleep or the process is cached, so a timeout scheduled for 30s can fire hours later. Both callbacks stamped the spans with the wake-up time, turning an app start the user walked away from into a multi-hour transaction. Record when each timeout falls due at scheduling time and stamp with that instead, whenever the timer runs late. The deadline path passes the clamped timestamp down to forceFinish, which stamps every child before the root finishes and so otherwise defeats trimEnd. Fixes #5752 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📲 Install BuildsAndroid
|
Whether a timeout expired was decided by comparing a fresh wall-clock reading against the projected due date. That is a duration measured from two independent wall-clock readings, which the clock can lengthen, shorten or make negative. A backward step while the timer waits made an expired timeout look pending, so the transaction was stamped with the wake-up time again; a forward step truncated a transaction whose timeout had not expired. Measure expiry on io.sentry.time.Deadline, which runs on the monotonic ticker and, on Android, on CLOCK_BOOTTIME so the interval includes deep sleep. The instant to end at stays a wall-clock timestamp, projected once from the same reading that sets the deadline. An expired timeout now always ends the transaction at the instant it fell due, including an on-time fire, which drops the scheduler jitter from the reported duration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tracer's timeout logic now works in io.sentry.time terms throughout: the duration on a Deadline, the instant as a Timestamp read from the options' EpochClock. Timestamp.toSentryDate bridges to the type the span API still takes, at the single point of use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FixedEpochClock, TestMonotonicTicker and DeferredExecutorService already existed; the tests were hand-rolling a date-provider fake and waiting on the real timer. Driving the ticker and the timer directly also removes the race between the test advancing its clock and the 20ms timer actually firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename Timestamp.toSentryDate to getSentryDate so Kotlin callers reach it as expiry?.expiredAt()?.sentryDate, and note at the call sites what the bridge is for and where the tracer reaches into options for its clocks. Also drops a meaningless @NotNull on forceFinish's void return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The paragraph on which class holds the instant restated the field types. What is left is the part the code cannot say: why a late timer is possible at all, and why only a monotonic deadline can tell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| if (idleTimeout != null) { | ||
| cancelIdleTimer(); | ||
| isIdleFinishTimerRunning.set(true); | ||
| idleExpiry = expiryIn(idleTimeout); | ||
|
|
||
| try { | ||
| idleTimeoutFuture = |
There was a problem hiding this comment.
Bug: A race condition in scheduleFinish() can cause an old timer to use a new idleExpiry value, resulting in an incorrect transaction finish time.
Severity: MEDIUM
Suggested Fix
To prevent the race condition, avoid using a shared, mutable idleExpiry field. Instead, the Expiry object should be captured by the timer's callback at the time of scheduling. Pass the specific Expiry instance to the onIdleTimeoutReached runnable, ensuring it uses the correct value and is not affected by subsequent calls to scheduleFinish().
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: sentry/src/main/java/io/sentry/SentryTracer.java#L124-L130
Potential issue: A race condition exists when rescheduling an idle transaction's finish
timer. When `scheduleFinish()` is called, it overwrites the class field `idleExpiry`. If
a previously scheduled timer's callback, `onIdleTimeoutReached()`, executes after this
field is overwritten but before the old timer is successfully canceled, it will read the
new `idleExpiry` value. This causes the deadline check to fail, and the transaction
finishes with the current time instead of its originally intended timeout time,
defeating the purpose of using a monotonic clock for late-firing timers.
Also affects:
sentry/src/main/java/io/sentry/SentryTracer.java:148~154
Did we get this right? 👍 / 👎 to inform future reviews.
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 edd56b0. Configure here.
|
|
||
| final @NotNull SentryDate finishTimestamp = scopes.getOptions().getDateProvider().now(); | ||
| final @NotNull SentryDate finishTimestamp = | ||
| finishDate != null ? finishDate : scopes.getOptions().getDateProvider().now(); |
There was a problem hiding this comment.
Child timestamps ignore timeout cutoff
Medium Severity
When a late timeout backdates the transaction, children that already finished keep timestamps after that cutoff and extend past the parent. Unfinished children started after the due instant are ended at that earlier time, so their finish precedes their start.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit edd56b0. Configure here.


📜 Description
in #5752 we see that an app can go in to doze during startup leading to very long startup transactions or spans.
The issue is that when the device goes in to doze, the timer is also paused and only fires when the device wakes up leading to hours long startup spans.
To fix this we essentially set the length of the transaction to the timeout duration. This is a PRODUCT DECISION! So feel free to disagree with this. Another alternate is to discard the transaction (which is also easier code-wise). Data-wise that might make more sense too!
Designwise, I added a
Timestamp.getSentryDate()to bridge the gap between the new APIs and the existing. Otherwise we need to make a lot of breaking changes to theSpanobject. I'm also open to that, but we should save that for v9.💡 Motivation and Context
#5752
See also this other PR which tries to solve the same problem by discarding the transaction: #5755
💚 How did you test it?
Well we can't really test the doze feature here. We're just cutting off everything that exceeds the deadline.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
🤖 Generated with Claude Code