fix(core): [Unhandled Sessions 2] Don't let a queued SessionStart overwrite a newer session #5920
+248
−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); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+141
to
+149
Contributor
Author
There was a problem hiding this comment. Today 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.
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); | ||
|
buenaflor marked this conversation as resolved.
|
||
| } | ||
|
|
||
| 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,19 @@ 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); | ||
| // a failed write truncates the file, so there is no good copy left to protect | ||
| lastPersistedSessionId = written ? session.getSessionId() : null; | ||
| } | ||
|
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.
Bug: When a new
SessionStartenvelope rotates the session file,lastPersistedSessionIdis not updated, leading to a potential state inconsistency with stale envelopes.Severity: LOW
Suggested Fix
To ensure state consistency, update the
lastPersistedSessionIdvariable with the new session's ID immediately afterwriteSessionToDiskis called within theSessionStarthandling block.Prompt for AI Agent