fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session - #5920
fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session#5920buenaflor wants to merge 34 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Adds Session.State.Unhandled from the session protocol, plus a pending-unhandled marker that survives serialization. A session carrying the marker finalizes as Unhandled instead of Exited on end(), while Crashed and Abnormal keep taking precedence. Co-authored-by: Cursor <cursoragent@cursor.com>
"Unhandled" alone is ambiguous: a native crash is also an unhandled error, it just terminates the process and so ends the session as crashed rather than unhandled. Name the flag after the property that actually distinguishes the two and match the vocabulary of captureEnvelopeNonTerminating. Also clarify that the setter only restores the flag when rebuilding a session and must not be used to mutate a live one. Co-authored-by: Cursor <cursoragent@cursor.com>
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 85fd8b1 | 306.66 ms | 377.10 ms | 70.44 ms |
| 62b579c | 299.75 ms | 364.84 ms | 65.09 ms |
| 7414e9b | 322.49 ms | 378.88 ms | 56.39 ms |
| 44472da | 319.72 ms | 351.90 ms | 32.18 ms |
| 62b579c | 318.48 ms | 367.71 ms | 49.24 ms |
| 694d587 | 305.45 ms | 378.38 ms | 72.94 ms |
| 2195398 | 319.02 ms | 342.38 ms | 23.36 ms |
| 5865051 | 324.24 ms | 356.02 ms | 31.78 ms |
| bb0ff41 | 344.70 ms | 413.82 ms | 69.12 ms |
| 092f017 | 353.13 ms | 433.84 ms | 80.71 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 85fd8b1 | 0 B | 0 B | 0 B |
| 62b579c | 0 B | 0 B | 0 B |
| 7414e9b | 0 B | 0 B | 0 B |
| 44472da | 0 B | 0 B | 0 B |
| 62b579c | 0 B | 0 B | 0 B |
| 694d587 | 1.58 MiB | 2.19 MiB | 620.06 KiB |
| 2195398 | 0 B | 0 B | 0 B |
| 5865051 | 0 B | 0 B | 0 B |
| bb0ff41 | 0 B | 0 B | 0 B |
| 092f017 | 0 B | 0 B | 0 B |
Previous results on branch: feat/unhandled-sessions-cache
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6376fbe | 313.53 ms | 364.04 ms | 50.51 ms |
| 783a09c | 356.23 ms | 424.70 ms | 68.47 ms |
| d73395c | 315.63 ms | 350.86 ms | 35.23 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6376fbe | 0 B | 0 B | 0 B |
| 783a09c | 0 B | 0 B | 0 B |
| d73395c | 0 B | 0 B | 0 B |
clone() and Session.Deserializer are both inside Session, so they can restore the field directly. Dropping the setter keeps it off the public API surface and makes it impossible to flip the flag on a live session without counting the error and advancing the sequence. Co-authored-by: Cursor <cursoragent@cursor.com>
📲 Install BuildsAndroid
|
Every other field is set at construction; the flag was the odd one out, assigned afterwards. A private canonical constructor keeps construction complete without putting the flag on the public API, which a 15-arg public overload would do. Co-authored-by: Cursor <cursoragent@cursor.com>
As a bare noun phrase the field read like it held the error rather than a boolean, most visibly where it is passed as a constructor argument. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The round-trip case duplicated one already added to SessionSerializationTest. Keep JSON concerns in the serialization test and leave SessionTest to state transitions. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
It was the only field in Session without the one-line comment the surrounding declarations all carry. Co-authored-by: Cursor <cursoragent@cursor.com>
ba38862 to
40c8329
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
40c8329 to
ddf1173
Compare
hasNonTerminatingUnhandledError is not final - recordNonTerminating UnhandledError and update() both write it - so setting it through a constructor established no invariant that a plain assignment does not. Both call sites are inside Session, so clone() and the deserializer can assign the field directly, which is what the deserializer already does for unknown. Removes the 15-parameter overload and the javadoc that existed to justify it. The public constructor is unchanged, so sentry.api is too. Co-authored-by: Cursor <cursoragent@cursor.com>
ddf1173 to
e167c56
Compare
Also swaps assertFalse(serialize(...).contains(...)) for Truth's doesNotContain, which reports the offending json on failure instead of just "expected false". The two new PreviousSessionFinalizerTest cases are left on Mockito argThat, which needs a Boolean predicate rather than an assertion. Co-authored-by: Cursor <cursoragent@cursor.com>
SessionEnd previously deleted session.json unconditionally and SessionStart always rotated it. A delayed end or start could therefore drop a newer session snapshot. Both paths now compare session ids and start times before deleting or rotating, and a new persistCurrentSession lets callers flush the active session to disk. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Both session paths in EnvelopeCache answer the same question - is this envelope stale relative to what is already on disk - but the end path inlined eight clauses and phrased it as "preserve", while the start path hid it behind a helper and negated it. Name both isStaleSessionEnd and isStaleSessionStart so the shared idea is visible, and move the "why" onto those helpers. Also narrows the JavaUtilDate suppression to the comparison itself and fixes a comment that still claimed the item reader only served starts. Co-authored-by: Cursor <cursoragent@cursor.com>
e167c56 to
d5fec24
Compare
session.json has only two writers, the SessionStart path and persistCurrentSession. So if a start envelope finds its own session id already on disk, persistCurrentSession put it there for the live session, and that copy is necessarily at least as advanced. There is nothing to measure: comparing the unhandled flag and error count answered a question that only ever has one answer. The start path collapses to "if this envelope is about a different session than the one on disk, behave as before; otherwise leave it alone", which also avoids rotating a running session into previous_session.json. Co-authored-by: Cursor <cursoragent@cursor.com>
Narrowing this pre-existing catch was incidental to the feature and the only thing in this PR that alters existing behaviour: an Error while parsing the session item used to be swallowed so the store continued and the envelope still reached disk, whereas propagating it abandons the store partway. It was also inconsistent, converting one of six catch (Throwable) blocks in this file simply because the edit landed next to it. The new readSessionFromDisk keeps catch (Exception), so new code still refuses to swallow fatal errors. Co-authored-by: Cursor <cursoragent@cursor.com>
…recovery paths Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… from disk Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
| final @Nullable Session startingSession = readSessionFromEnvelope(envelope); | ||
| if (!isAlreadyPersisted(startingSession)) { | ||
| movePreviousSession(currentSessionFile, previousSessionFile); | ||
| if (startingSession != null) { | ||
| writeSessionToDisk(currentSessionFile, startingSession); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Today session.json only gets written from the transport queue. This PR adds a second writer, persistCurrentSession, which writes the live session straight to disk. #5921 calls it when Flutter reports an unhandled error that didn't kill the app.
The issue is that the SessionStart envelope is queued, so it can land after that:
session S starts → SessionStart(S) goes into the transport queue
unhandled Dart error → S gets flagged, persistCurrentSession writes it to session.json
SessionStart(S) drains → session.json moved to previous_session.json, so last run's session is gone
→ envelope's unflagged copy of S written over session.json
app killed → S comes in as exited instead of unhandled
So we skip the move and the write if the SessionStart is for a session we already persisted. Nothing else changes.
There was a problem hiding this comment.
Pull request overview
This PR updates the core EnvelopeCache session persistence logic to prevent a delayed SessionStart write (processed on the transport executor) from rotating/overwriting a newer, synchronously persisted live session snapshot.
Changes:
- Added
EnvelopeCache.persistCurrentSession(Session)and tracking vialastPersistedSessionIdto protect newer on-disk session state from delayedSessionStartenvelopes. - Refactored session extraction into
readSessionFromEnvelope(...)and guarded rotation+write in a sharedsessionLockcritical section. - Expanded
EnvelopeCacheTestcoverage for same-SID delayed starts, different-SID rotation behavior, and null-SID handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sentry/src/main/java/io/sentry/cache/EnvelopeCache.java | Adds synchronous current-session persistence and prevents stale SessionStart envelopes from clobbering newer session snapshots. |
| sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt | Adds tests validating same/different/null SID behaviors with the new persistence mechanism. |
| sentry/api/sentry.api | Updates the API surface to include the new persistCurrentSession(Session) method. |
Suppressed comments (1)
sentry/src/main/java/io/sentry/cache/EnvelopeCache.java:295
- readSessionFromEnvelope() calls items.iterator() twice (hasNext() and next()), which creates two independent iterators. Using a single Iterator avoids redundant work and prevents surprising behavior for non-collection Iterables.
final Iterable<SentryEnvelopeItem> items = envelope.getItems();
// we know that an envelope with a SessionStart hint has a single item inside
if (items.iterator().hasNext()) {
final SentryEnvelopeItem item = items.iterator().next();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…nStart The guard skipped the SessionStart write whenever the id matched the last persisted one, even when session.json no longer held that session: - a queued SessionEnd for the prior session deletes the file the live session was just persisted to, so the skipped write left no session on disk at all - a failed persist still recorded the id, so the queued write that would have repaired the truncated file was skipped too Clear the id on SessionEnd and only record it when the write succeeded. Co-authored-by: Cursor <cursoragent@cursor.com>
|
LGTM! |
…essions-cache Co-authored-by: Cursor <cursoragent@cursor.com>
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 ef55c1a. Configure here.
writeSessionToDisk truncates the current session file before serializing, so a failed persist leaves it corrupt. lastPersistedSessionId kept pointing at it, and isAlreadyPersisted then skipped the rotation that would have replaced the file, on the premise that it still held the live session. Co-authored-by: Cursor <cursoragent@cursor.com>

PR Stack (Unhandled Sessions)
📜 Description
storeEnveloperuns on the transport executor, so aSessionStartreaches disk whenever the queue drains rather than when the session actually started. This PR addsEnvelopeCache.persistCurrentSession(Session), a second writer that writes the live session synchronously and bypasses that queue, which makes the two orderings diverge. #5921 is what calls it.When they diverge, the
SessionStartpath rotates the still-running session intoprevious_session.jsonand overwritessession.jsonwith the envelope's older copy. That drops any error recorded since the session started, and it deletes the previous run's session thatMovePreviousSessionfiled at init, so that session is never reported.persistCurrentSessionnow records the id it wrote inlastPersistedSessionId, and theSessionStartpath skips both the rotation and the write when the starting session matches it. Two supporting changes: reading the envelope's session moved out ofupdateCurrentSessionintoreadSessionFromEnvelopeso the id is available for that comparison, and the rotation and the write now share onesessionLockcritical section sopersistCurrentSessioncannot land between them.Skipping the write is only safe while
session.jsonstill holds whatpersistCurrentSessionput there, so the id is cleared whenever that stops being true:SessionEnddeletessession.jsonwithout looking at whose session it holds.Scopes.startSession()queues the end of the old session and the start of the new one back to back, so a persist for the new session can land in between and then be deleted by the old session'sSessionEnd. That branch now clears the id undersessionLock, so the delayedSessionStartwrites the session back instead of skipping.writeSessionToDiskswallowed everyThrowableand returned void, so a failed persist still recorded the id — and since the stream truncates the file on open, it could leavesession.jsontruncated with no queued write left to repair it. It now returns whether the write succeeded, and only then is the id recorded.💡 Motivation and Context
Needed by #5921, which mutates the live session in place and persists it so the unhandled flag survives process death.
Nothing outside the hybrid capture path is affected.
lastPersistedSessionIdstarts null, so untilpersistCurrentSessionactually runs, both branches behave exactly as they do today.💚 How did you test it?
EnvelopeCacheTestcovers a delayed same-id start preserving a newer unhandled snapshot and a newer error count, different-id starts still rotating, and null session ids rotating rather than being treated as the same session. Two more cover the cleared id: aSessionEndlanding between the persist and the delayed start, and a persist whose write failed. The pre-existingSessionStart hint saves unfinished session to previous_session fileguards the default path, where nothing was persisted out of band.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
The capture API in #5921.
#skip-changelog