Skip to content

Commit a9ec02a

Browse files
buenaflorcursoragent
buenaflor
and
cursoragent
authored
fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session (#5920)
* fix(core): Make session cache writes session-id aware 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> * ref: follow Session rename in EnvelopeCache Co-authored-by: Cursor <cursoragent@cursor.com> * docs: explain why stale session envelopes must not clobber newer state Co-authored-by: Cursor <cursoragent@cursor.com> * ref(session): make the two stale-envelope checks read the same way 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> * ref(cache): replace the stale-start comparison with an identity check 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> * ref(cache): restore catch (Throwable) in the envelope session reader 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> * ref(cache): delete the current session file unconditionally again 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> * ref(cache): fold the session-id comparison into one predicate Co-authored-by: Cursor <cursoragent@cursor.com> * ref(cache): track the out-of-band session id instead of re-reading it from disk Co-authored-by: Cursor <cursoragent@cursor.com> * ref(cache): rename isLateDuplicateStart to isAlreadyPersisted Co-authored-by: Cursor <cursoragent@cursor.com> * fix(sessions): don't let the persisted session guard swallow a SessionStart 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> * fix(cache): Clear the persisted session id when the write fails 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> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 0699a94 commit a9ec02a

3 files changed

Lines changed: 248 additions & 13 deletions

File tree

‎sentry/api/sentry.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4901,6 +4901,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
49014901
public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
49024902
public fun iterator ()Ljava/util/Iterator;
49034903
public fun movePreviousSession (Ljava/io/File;Ljava/io/File;)V
4904+
public fun persistCurrentSession (Lio/sentry/Session;)V
49044905
public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
49054906
public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
49064907
public fun waitPreviousSessionFlush ()Z

‎sentry/src/main/java/io/sentry/cache/EnvelopeCache.java‎

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ public class EnvelopeCache extends CacheStrategy implements IEnvelopeCache {
7676
protected final @NotNull AutoClosableReentrantLock cacheLock = new AutoClosableReentrantLock();
7777
protected final @NotNull AutoClosableReentrantLock sessionLock = new AutoClosableReentrantLock();
7878

79+
/**
80+
* Session id last written to the current session file by {@link #persistCurrentSession(Session)},
81+
* which bypasses the transport queue that every other write to that file goes through.
82+
*/
83+
private @Nullable String lastPersistedSessionId;
84+
7985
public static @NotNull IEnvelopeCache create(final @NotNull SentryOptions options) {
8086
final String cacheDirPath = options.getCacheDirPath();
8187
final int maxCacheItems = options.getMaxCacheItems();
@@ -118,8 +124,11 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not
118124
final File previousSessionFile = getPreviousSessionFile(directoryPath);
119125

120126
if (HintUtils.hasType(hint, SessionEnd.class)) {
121-
if (!currentSessionFile.delete()) {
122-
options.getLogger().log(WARNING, "Current envelope doesn't exist.");
127+
try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
128+
lastPersistedSessionId = null;
129+
if (!currentSessionFile.delete()) {
130+
options.getLogger().log(WARNING, "Current envelope doesn't exist.");
131+
}
123132
}
124133
}
125134

@@ -129,8 +138,15 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not
129138
}
130139

131140
if (HintUtils.hasType(hint, SessionStart.class)) {
132-
movePreviousSession(currentSessionFile, previousSessionFile);
133-
updateCurrentSession(currentSessionFile, envelope);
141+
try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
142+
final @Nullable Session startingSession = readSessionFromEnvelope(envelope);
143+
if (!isAlreadyPersisted(startingSession)) {
144+
movePreviousSession(currentSessionFile, previousSessionFile);
145+
if (startingSession != null) {
146+
writeSessionToDisk(currentSessionFile, startingSession);
147+
}
148+
}
149+
}
134150

135151
boolean crashedLastRun = false;
136152
final File crashMarkerFile = new File(options.getCacheDirPath(), NATIVE_CRASH_MARKER_FILE);
@@ -274,8 +290,7 @@ private void writeCrashMarkerFile() {
274290
}
275291
}
276292

277-
private void updateCurrentSession(
278-
final @NotNull File currentSessionFile, final @NotNull SentryEnvelope envelope) {
293+
private @Nullable Session readSessionFromEnvelope(final @NotNull SentryEnvelope envelope) {
279294
final Iterable<SentryEnvelopeItem> items = envelope.getItems();
280295

281296
// we know that an envelope with a SessionStart hint has a single item inside
@@ -295,7 +310,7 @@ private void updateCurrentSession(
295310
"Item of type %s returned null by the parser.",
296311
item.getHeader().getType());
297312
} else {
298-
writeSessionToDisk(currentSessionFile, session);
313+
return session;
299314
}
300315
} catch (Throwable e) {
301316
options.getLogger().log(ERROR, "Item failed to process.", e);
@@ -309,10 +324,26 @@ private void updateCurrentSession(
309324
item.getHeader().getType());
310325
}
311326
} else {
312-
options
313-
.getLogger()
314-
.log(INFO, "Current envelope %s is empty", currentSessionFile.getAbsolutePath());
327+
options.getLogger().log(INFO, "Current envelope is empty.");
328+
}
329+
return null;
330+
}
331+
332+
/**
333+
* Whether a {@link SessionStart} envelope refers to the session {@link
334+
* #persistCurrentSession(Session)} already wrote to the current session file. That copy is the
335+
* live session, so it is at least as advanced as this envelope. Rotating and overwriting it would
336+
* file a running session as the previous one and roll back any unhandled error it has recorded
337+
* since.
338+
*
339+
* <p>A null session id never matches, so sessions we cannot tell apart are rotated as before.
340+
*/
341+
private boolean isAlreadyPersisted(final @Nullable Session startingSession) {
342+
if (startingSession == null) {
343+
return false;
315344
}
345+
final @Nullable String startingSessionId = startingSession.getSessionId();
346+
return startingSessionId != null && startingSessionId.equals(lastPersistedSessionId);
316347
}
317348

318349
private boolean writeEnvelopeToDisk(
@@ -337,7 +368,7 @@ private boolean writeEnvelopeToDisk(
337368
return true;
338369
}
339370

340-
private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {
371+
private boolean writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {
341372
try (final OutputStream outputStream = new FileOutputStream(file);
342373
final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) {
343374
options
@@ -349,6 +380,19 @@ private void writeSessionToDisk(final @NotNull File file, final @NotNull Session
349380
options
350381
.getLogger()
351382
.log(ERROR, e, "Error writing Session to offline storage: %s", session.getSessionId());
383+
return false;
384+
}
385+
return true;
386+
}
387+
388+
@ApiStatus.Internal
389+
public void persistCurrentSession(final @NotNull Session session) {
390+
try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
391+
final boolean written =
392+
writeSessionToDisk(
393+
getCurrentSessionFile(directory.getOrCreate().getAbsolutePath()), session);
394+
// a failed write truncates the file, so there is no good copy left to protect
395+
lastPersistedSessionId = written ? session.getSessionId() : null;
352396
}
353397
}
354398

‎sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt‎

Lines changed: 192 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.sentry.cache
22

3+
import com.google.common.truth.Truth.assertThat
34
import io.sentry.DateUtils
45
import io.sentry.Hint
56
import io.sentry.ILogger
@@ -23,6 +24,7 @@ import io.sentry.hints.SessionStartHint
2324
import io.sentry.protocol.SentryId
2425
import io.sentry.util.HintUtils
2526
import java.io.File
27+
import java.io.Writer
2628
import java.nio.file.Files
2729
import java.nio.file.Path
2830
import java.util.Date
@@ -34,8 +36,10 @@ import kotlin.test.assertFalse
3436
import kotlin.test.assertNotNull
3537
import kotlin.test.assertTrue
3638
import org.mockito.kotlin.any
39+
import org.mockito.kotlin.eq
3740
import org.mockito.kotlin.mock
3841
import org.mockito.kotlin.same
42+
import org.mockito.kotlin.verify
3943
import org.mockito.kotlin.whenever
4044

4145
class EnvelopeCacheTest {
@@ -160,6 +164,189 @@ class EnvelopeCacheTest {
160164
assertTrue(didStore)
161165
}
162166

167+
@Test
168+
fun `delayed same SID SessionStart preserves newer unhandled snapshot`() {
169+
val cache = fixture.getSUT()
170+
val sid = SentryUUID.generateSentryId()
171+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
172+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
173+
val newerSession = createSession(sessionId = sid)
174+
newerSession.recordNonTerminatingUnhandledError()
175+
cache.persistCurrentSession(newerSession)
176+
177+
val delayedStart = createSession(sessionId = sid)
178+
val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
179+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
180+
181+
val persistedSession =
182+
fixture.options.serializer.deserialize(
183+
currentSessionFile.bufferedReader(),
184+
Session::class.java,
185+
)!!
186+
assertThat(persistedSession.sessionId).isEqualTo(sid)
187+
assertThat(persistedSession.hasNonTerminatingUnhandledError()).isTrue()
188+
assertThat(persistedSession.errorCount()).isEqualTo(1)
189+
assertThat(previousSessionFile.exists()).isFalse()
190+
}
191+
192+
@Test
193+
fun `delayed same SID SessionStart preserves newer error count snapshot`() {
194+
val cache = fixture.getSUT()
195+
val sid = SentryUUID.generateSentryId()
196+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
197+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
198+
val newerSession = createSession(sessionId = sid)
199+
newerSession.update(null, null, true)
200+
cache.persistCurrentSession(newerSession)
201+
202+
val delayedStart = createSession(sessionId = sid)
203+
val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
204+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
205+
206+
val persistedSession =
207+
fixture.options.serializer.deserialize(
208+
currentSessionFile.bufferedReader(),
209+
Session::class.java,
210+
)!!
211+
assertThat(persistedSession.sessionId).isEqualTo(sid)
212+
assertThat(persistedSession.hasNonTerminatingUnhandledError()).isFalse()
213+
assertThat(persistedSession.errorCount()).isEqualTo(1)
214+
assertThat(previousSessionFile.exists()).isFalse()
215+
}
216+
217+
@Test
218+
fun `failed persist stops the delayed same SID SessionStart from being skipped`() {
219+
val cache = fixture.getSUT()
220+
val sid = SentryUUID.generateSentryId()
221+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
222+
val newerSession = createSession(sessionId = sid)
223+
newerSession.recordNonTerminatingUnhandledError()
224+
cache.persistCurrentSession(newerSession)
225+
226+
// a directory where the session file belongs makes the write fail
227+
assertTrue(currentSessionFile.delete())
228+
assertTrue(currentSessionFile.mkdir())
229+
cache.persistCurrentSession(newerSession)
230+
assertTrue(currentSessionFile.delete())
231+
232+
val delayedStart = createSession(sessionId = sid)
233+
val envelope = SentryEnvelope.from(fixture.options.serializer, delayedStart, null)
234+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
235+
236+
val persistedSession =
237+
fixture.options.serializer.deserialize(
238+
currentSessionFile.bufferedReader(),
239+
Session::class.java,
240+
)!!
241+
assertThat(persistedSession.sessionId).isEqualTo(sid)
242+
assertThat(persistedSession.hasNonTerminatingUnhandledError()).isFalse()
243+
assertThat(persistedSession.errorCount()).isEqualTo(0)
244+
}
245+
246+
@Test
247+
fun `null SIDs on SessionStart rotate instead of preserving as same session`() {
248+
val cache = fixture.getSUT()
249+
val currentSession = createSession(sessionId = null)
250+
currentSession.update(null, null, true)
251+
cache.persistCurrentSession(currentSession)
252+
val startingSession = createSession(sessionId = null)
253+
254+
val envelope = SentryEnvelope.from(fixture.options.serializer, startingSession, null)
255+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
256+
257+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
258+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
259+
val persistedCurrent =
260+
fixture.options.serializer.deserialize(
261+
currentSessionFile.bufferedReader(),
262+
Session::class.java,
263+
)!!
264+
val persistedPrevious =
265+
fixture.options.serializer.deserialize(
266+
previousSessionFile.bufferedReader(),
267+
Session::class.java,
268+
)!!
269+
assertThat(persistedCurrent.sessionId).isNull()
270+
assertThat(persistedCurrent.errorCount()).isEqualTo(0)
271+
assertThat(persistedPrevious.sessionId).isNull()
272+
assertThat(persistedPrevious.errorCount()).isEqualTo(1)
273+
}
274+
275+
@Test
276+
fun `different SID SessionStart rotates current session`() {
277+
val cache = fixture.getSUT()
278+
val currentSession = createSession()
279+
cache.persistCurrentSession(currentSession)
280+
val nextSession = createSession()
281+
282+
val envelope = SentryEnvelope.from(fixture.options.serializer, nextSession, null)
283+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
284+
285+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
286+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
287+
val persistedCurrent =
288+
fixture.options.serializer.deserialize(
289+
currentSessionFile.bufferedReader(),
290+
Session::class.java,
291+
)!!
292+
val persistedPrevious =
293+
fixture.options.serializer.deserialize(
294+
previousSessionFile.bufferedReader(),
295+
Session::class.java,
296+
)!!
297+
assertThat(persistedCurrent.sessionId).isEqualTo(nextSession.sessionId)
298+
assertThat(persistedPrevious.sessionId).isEqualTo(currentSession.sessionId)
299+
}
300+
301+
@Test
302+
fun `SessionEnd deleting the persisted session lets the delayed SessionStart write it again`() {
303+
val cache = fixture.getSUT()
304+
val sid = SentryUUID.generateSentryId()
305+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
306+
cache.persistCurrentSession(createSession(sessionId = sid))
307+
308+
// the previous session's end envelope is still queued and deletes the file the live session
309+
// was just written to
310+
val endedSession = createSession()
311+
cache.storeEnvelope(
312+
SentryEnvelope.from(fixture.options.serializer, endedSession, null),
313+
HintUtils.createWithTypeCheckHint(SessionEndHint()),
314+
)
315+
assertThat(currentSessionFile.exists()).isFalse()
316+
317+
val delayedStart = createSession(sessionId = sid)
318+
cache.storeEnvelope(
319+
SentryEnvelope.from(fixture.options.serializer, delayedStart, null),
320+
HintUtils.createWithTypeCheckHint(SessionStartHint()),
321+
)
322+
323+
val persistedSession =
324+
fixture.options.serializer.deserialize(
325+
currentSessionFile.bufferedReader(),
326+
Session::class.java,
327+
)!!
328+
assertThat(persistedSession.sessionId).isEqualTo(sid)
329+
}
330+
331+
@Test
332+
fun `failed persist lets the delayed SessionStart write the session`() {
333+
val sid = SentryUUID.generateSentryId()
334+
val liveSession = createSession(sessionId = sid)
335+
val delayedStart = createSession(sessionId = sid)
336+
val serializer = mock<ISerializer>()
337+
whenever(serializer.serialize(same(liveSession), any<Writer>()))
338+
.thenThrow(RuntimeException("forced ex"))
339+
whenever(serializer.deserialize(any(), eq(Session::class.java))).thenReturn(delayedStart)
340+
val cache = fixture.getSUT { options -> options.setSerializer(serializer) }
341+
342+
cache.persistCurrentSession(liveSession)
343+
344+
val envelope = SentryEnvelope.from(SentryOptions.empty().serializer, delayedStart, null)
345+
cache.storeEnvelope(envelope, HintUtils.createWithTypeCheckHint(SessionStartHint()))
346+
347+
verify(serializer).serialize(same(delayedStart), any<Writer>())
348+
}
349+
163350
@Test
164351
fun `updates current file on session update and read it back`() {
165352
val cache = fixture.getSUT()
@@ -491,14 +678,17 @@ class EnvelopeCacheTest {
491678
assertFalse(didStore)
492679
}
493680

494-
private fun createSession(started: Date? = null): Session =
681+
private fun createSession(
682+
started: Date? = null,
683+
sessionId: String? = SentryUUID.generateSentryId(),
684+
): Session =
495685
Session(
496686
Ok,
497687
started ?: DateUtils.getCurrentDateTime(),
498688
DateUtils.getCurrentDateTime(),
499689
0,
500690
"dis",
501-
SentryUUID.generateSentryId(),
691+
sessionId,
502692
true,
503693
null,
504694
null,

0 commit comments

Comments
 (0)