-
-
Notifications
You must be signed in to change notification settings - Fork 454
fix(sessions): Move and flush unfinished previous session on init #4624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@markushi I'm unsure about how this affects the Hybrid SDKs, but form a quick look into InternalSentrySdk I don't think anything needs to change. But let me know if there's something I missed. |
final boolean renamed = currentSessionFile.renameTo(previousSessionFile); | ||
if (!renamed) { |
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.
Potential bug: If `renameTo` fails in `movePreviousSession`, a latch is still released, causing `PreviousSessionFinalizer` to proceed and silently lose the session data.
- Description: The
movePreviousSession
method inEnvelopeCache
attempts to rename thecurrentSessionFile
. If thisrenameTo()
operation fails due to filesystem issues or permissions, it only logs a warning. However, it proceeds to callflushPreviousSession()
, which releases thepreviousSessionLatch
. ThePreviousSessionFinalizer
is dependent on this latch. Once unblocked, it attempts to process the previous session but fails to find the file because the rename operation was unsuccessful. This results in the session data being silently and permanently lost, which can affect the accuracy of analytics and crash reporting. - Suggested fix: Check the boolean return value of
currentSessionFile.renameTo(previousSessionFile)
. If the rename operation returnsfalse
, do not callflushPreviousSession()
. This will prevent thePreviousSessionFinalizer
from proceeding prematurely and allow for proper error handling or retry logic for the failed file operation.
severity: 0.82, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Performance metrics 🚀
|
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.
LGTM, left a comment about edge case handling.
public void movePreviousSession( | ||
final @NotNull File currentSessionFile, final @NotNull File previousSessionFile) { | ||
try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) { | ||
if (previousSessionFile.exists()) { |
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.
I'm wondering if we should simply overwrite the file instead. If the file still exists, chances are that we've already sent that session information. On the other hand currentSessionFile
was never flushed, so renaming it to previousSessionFile
and later flushing that session feels more correct.
📜 Description
Previously, we were waiting on previous session flush latch on our background queue when processing ANRs or finalizing previous session for 15 seconds to give it a good chance to be flushed. However, the trigger for the flush was actually a new session start:
However this was not working well for background app starts. Since there's no foreground event, a new session would not be started and we were waiting for 15 seconds for nothing (on a background thread, but still). So this PR addresses it by moving any unfinished session to its own file right away on
init
(but before any of the integrations are registered, so those that rely on the previous session latch (like ANRs), do not need to wait for it).We also just rename
session.json
toprevious_session.json
now, as opposed to ser/deserializing the session multiple times from/into different filesBefore (perfetto trace of a background app start)
After (perfetto trace of a background app start)
💡 Motivation and Context
Speed up things on our background queue
💚 How did you test it?
Manually + automated
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps