fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session #5920
+220
−13
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,12 @@ public class EnvelopeCache extends CacheStrategy implements IEnvelopeCache { | |
| protected final @NotNull AutoClosableReentrantLock cacheLock = new AutoClosableReentrantLock(); | ||
| protected final @NotNull AutoClosableReentrantLock sessionLock = new AutoClosableReentrantLock(); | ||
|
|
||
| /** | ||
| * Session id last written to the current session file by {@link #persistCurrentSession(Session)}, | ||
| * which bypasses the transport queue that every other write to that file goes through. | ||
| */ | ||
| private @Nullable String lastPersistedSessionId; | ||
|
|
||
| public static @NotNull IEnvelopeCache create(final @NotNull SentryOptions options) { | ||
| final String cacheDirPath = options.getCacheDirPath(); | ||
| final int maxCacheItems = options.getMaxCacheItems(); | ||
|
|
@@ -118,8 +124,11 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not | |
| final File previousSessionFile = getPreviousSessionFile(directoryPath); | ||
|
|
||
| if (HintUtils.hasType(hint, SessionEnd.class)) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
| lastPersistedSessionId = null; | ||
| if (!currentSessionFile.delete()) { | ||
| options.getLogger().log(WARNING, "Current envelope doesn't exist."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -129,8 +138,15 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not | |
| } | ||
|
|
||
| if (HintUtils.hasType(hint, SessionStart.class)) { | ||
| 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); | ||
| } | ||
| } | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| boolean crashedLastRun = false; | ||
| final File crashMarkerFile = new File(options.getCacheDirPath(), NATIVE_CRASH_MARKER_FILE); | ||
|
|
@@ -274,8 +290,7 @@ private void writeCrashMarkerFile() { | |
| } | ||
| } | ||
|
|
||
| private @Nullable Session readSessionFromEnvelope(final @NotNull SentryEnvelope envelope) { | ||
| final Iterable<SentryEnvelopeItem> items = envelope.getItems(); | ||
|
|
||
| // we know that an envelope with a SessionStart hint has a single item inside | ||
|
|
@@ -295,7 +310,7 @@ private void updateCurrentSession( | |
| "Item of type %s returned null by the parser.", | ||
| item.getHeader().getType()); | ||
| } else { | ||
| return session; | ||
| } | ||
| } catch (Throwable e) { | ||
| options.getLogger().log(ERROR, "Item failed to process.", e); | ||
|
|
@@ -309,10 +324,26 @@ private void updateCurrentSession( | |
| item.getHeader().getType()); | ||
| } | ||
| } else { | ||
| options.getLogger().log(INFO, "Current envelope is empty."); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Whether a {@link SessionStart} envelope refers to the session {@link | ||
| * #persistCurrentSession(Session)} already wrote to the current session file. That copy is the | ||
| * live session, so it is at least as advanced as this envelope. Rotating and overwriting it would | ||
| * file a running session as the previous one and roll back any unhandled error it has recorded | ||
| * since. | ||
| * | ||
| * <p>A null session id never matches, so sessions we cannot tell apart are rotated as before. | ||
| */ | ||
| private boolean isAlreadyPersisted(final @Nullable Session startingSession) { | ||
| if (startingSession == null) { | ||
| return false; | ||
| } | ||
| final @Nullable String startingSessionId = startingSession.getSessionId(); | ||
| return startingSessionId != null && startingSessionId.equals(lastPersistedSessionId); | ||
|
Contributor
There was a problem hiding this comment. do we need to acquire the
Contributor
Author
There was a problem hiding this comment. The read on its own doesn't need it, but the check and what follows from it ( If the lock only covered the read you could get:
|
||
| } | ||
|
|
||
| private boolean writeEnvelopeToDisk( | ||
|
|
@@ -337,7 +368,7 @@ private boolean writeEnvelopeToDisk( | |
| return true; | ||
| } | ||
|
|
||
| private boolean writeSessionToDisk(final @NotNull File file, final @NotNull Session session) { | ||
| try (final OutputStream outputStream = new FileOutputStream(file); | ||
| final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) { | ||
| options | ||
|
|
@@ -349,6 +380,20 @@ private void writeSessionToDisk(final @NotNull File file, final @NotNull Session | |
| options | ||
| .getLogger() | ||
| .log(ERROR, e, "Error writing Session to offline storage: %s", session.getSessionId()); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| @ApiStatus.Internal | ||
| public void persistCurrentSession(final @NotNull Session session) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
| final boolean written = | ||
| writeSessionToDisk( | ||
| getCurrentSessionFile(directory.getOrCreate().getAbsolutePath()), session); | ||
| if (written) { | ||
| lastPersistedSessionId = session.getSessionId(); | ||
| } | ||
|
There was a problem hiding this comment. Failed persist can clobber previous sessionMedium Severity
Additional Locations (1)  Reviewed by Cursor Bugbot for commit ae99f6c. Configure here.
Contributor
There was a problem hiding this comment. how do we handle failed persists in other cases? |
||
| } | ||
|
buenaflor marked this conversation as resolved.
|
||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today
session.jsononly 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:
So we skip the move and the write if the SessionStart is for a session we already persisted. Nothing else changes.