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 13 commits into
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fcec2f2 | 357.47 ms | 447.32 ms | 89.85 ms |
| f634d01 | 375.06 ms | 420.04 ms | 44.98 ms |
| 22f4345 | 312.78 ms | 347.40 ms | 34.62 ms |
| 057ba36 | 305.64 ms | 379.43 ms | 73.79 ms |
| f064536 | 335.52 ms | 408.79 ms | 73.27 ms |
| 6b019b7 | 403.90 ms | 546.09 ms | 142.19 ms |
| 0eaac1e | 316.82 ms | 357.34 ms | 40.52 ms |
| 5865051 | 324.24 ms | 356.02 ms | 31.78 ms |
| 7c1a728 | 289.46 ms | 368.15 ms | 78.69 ms |
| 05aa61d | 326.06 ms | 385.46 ms | 59.40 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| 057ba36 | 0 B | 0 B | 0 B |
| f064536 | 1.58 MiB | 2.20 MiB | 633.90 KiB |
| 6b019b7 | 0 B | 0 B | 0 B |
| 0eaac1e | 1.58 MiB | 2.19 MiB | 619.17 KiB |
| 5865051 | 0 B | 0 B | 0 B |
| 7c1a728 | 0 B | 0 B | 0 B |
| 05aa61d | 0 B | 0 B | 0 B |
📲 Install BuildsAndroid
|
ddf1173 to
e167c56
Compare
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>
The SessionEnd guard only paid off in a narrow window: a SessionEnd still queued while a newer session had already started and recorded an unhandled error. Dropping it degrades to the behaviour on main, because the queued SessionStart rewrites the file moments later, just without the flag. That cost a session.json read and deserialize on every SessionEnd for every SDK. The SessionStart guard stays. Its window is far wider, since app start is when the transport is busiest flushing the previous run's cache, and its failure mode is worse than a lost flag: movePreviousSession files the running session as the previous one. Co-authored-by: Cursor <cursoragent@cursor.com>
buenaflor
changed the title
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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| lastPersistedSessionId = session.getSessionId(); | ||
| } |
There was a problem hiding this comment.
Bug: persistCurrentSession unconditionally sets lastPersistedSessionId even if writeSessionToDisk fails silently. This causes subsequent session start events to be discarded, leading to data loss.
Severity: HIGH
Suggested Fix
The writeSessionToDisk method should indicate failure, for example, by returning a boolean or re-throwing a specific exception. persistCurrentSession should then check for this failure and only set lastPersistedSessionId if the write operation was successful. This ensures the in-memory state accurately reflects the persisted state on disk.
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/cache/EnvelopeCache.java#L387-L388
Potential issue: The `writeSessionToDisk` method catches all `Throwable` exceptions and
only logs them, effectively hiding any disk write failures, such as from a full disk or
permission errors. The calling method, `persistCurrentSession`, proceeds to set
`lastPersistedSessionId` regardless of whether the write was successful. When the
corresponding `SessionStart` envelope is processed later, `isAlreadyPersisted` will
incorrectly return `true` based on the in-memory `lastPersistedSessionId`. This prevents
the session from being written to disk, resulting in the loss of session data.
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a7f837f. Configure here.
| writeSessionToDisk(currentSessionFile, startingSession); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
SessionEnd can drop newer snapshot
High Severity
persistCurrentSession can write the new live session to session.json while a previous session's SessionEnd is still queued. That SessionEnd still deletes session.json unconditionally and without sessionLock. The following SessionStart then matches lastPersistedSessionId and skips the rewrite, so the current snapshot, including any unhandled flag, is gone if the process dies.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit a7f837f. Configure here.
| public void persistCurrentSession(final @NotNull Session session) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
| writeSessionToDisk(getCurrentSessionFile(directory.getOrCreate().getAbsolutePath()), session); | ||
| lastPersistedSessionId = session.getSessionId(); |
There was a problem hiding this comment.
Failed persist still skips SessionStart
Low Severity
persistCurrentSession sets lastPersistedSessionId even when writeSessionToDisk fails. SessionStart then treats that session as already on disk and skips its write, so a failed persist also suppresses the queued fallback and can leave session.json missing or truncated.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a7f837f. Configure here.
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.
| public void persistCurrentSession(final @NotNull Session session) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
| writeSessionToDisk(getCurrentSessionFile(directory.getOrCreate().getAbsolutePath()), session); | ||
| lastPersistedSessionId = session.getSessionId(); | ||
| } |
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.SessionEndis untouched and still deletes unconditionally. By then the session has left the scope, sopersistCurrentSessioncan no longer fire for it and the end envelope carries the final state.💡 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 the branch rotates and writes exactly as it does 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. 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