Skip to content
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

fix: Delete old session replay files #4446

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ via the option `swizzleClassNameExclude`.
- Finish TTID correctly when viewWillAppear is skipped (#4417)
- Swizzling RootUIViewController if ignored by `swizzleClassNameExclude` (#4407)
- Data race in SentrySwizzleInfo.originalCalled (#4434)
- Delete old session replay files (#4446)
- Thread running at user-initiated quality-of-service for session replay (#4439)


### Improvements

- Serializing profile on a BG Thread (#4377) to avoid potentially slightly blocking the main thread.
Expand Down
6 changes: 6 additions & 0 deletions Sources/Sentry/SentryFileManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,12 @@ - (void)deleteOldEnvelopesFromPath:(NSString *)envelopesPath
}
}

- (BOOL)isDirectory:(NSString *)path
{
BOOL isDir = NO;
return [NSFileManager.defaultManager fileExistsAtPath:path isDirectory:&isDir] && isDir;
}

- (void)deleteAllEnvelopes
{
[self removeFileAtPath:self.envelopesPath];
Expand Down
55 changes: 47 additions & 8 deletions Sources/Sentry/SentrySessionReplayIntegration.m
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ - (void)setupWith:(SentryReplayOptions *)replayOptions enableTouchTracker:(BOOL)
_notificationCenter = SentryDependencyContainer.sharedInstance.notificationCenterWrapper;

[self moveCurrentReplay];
[self cleanUp];

[SentrySDK.currentHub registerSessionListener:self];
[SentryGlobalEventProcessor.shared
addEventProcessor:^SentryEvent *_Nullable(SentryEvent *_Nonnull event) {
Expand All @@ -103,6 +105,19 @@ - (void)setupWith:(SentryReplayOptions *)replayOptions enableTouchTracker:(BOOL)
[SentryDependencyContainer.sharedInstance.reachability addObserver:self];
}

- (nullable NSDictionary<NSString *, id> *)lastReplayInfo
{
NSURL *dir = [self replayDirectory];
NSURL *lastReplayUrl = [dir URLByAppendingPathComponent:SENTRY_LAST_REPLAY];
NSData *lastReplay = [NSData dataWithContentsOfURL:lastReplayUrl];

if (lastReplay == nil) {
return nil;
}

return [SentrySerialization deserializeDictionaryFromJsonData:lastReplay];
}

/**
* Send the cached frames from a previous session that eventually crashed.
* This function is called when processing an event created by SentryCrashIntegration,
Expand All @@ -112,15 +127,8 @@ - (void)setupWith:(SentryReplayOptions *)replayOptions enableTouchTracker:(BOOL)
- (void)resumePreviousSessionReplay:(SentryEvent *)event
{
NSURL *dir = [self replayDirectory];
NSURL *lastReplayUrl = [dir URLByAppendingPathComponent:SENTRY_LAST_REPLAY];
NSData *lastReplay = [NSData dataWithContentsOfURL:lastReplayUrl];
NSDictionary<NSString *, id> *jsonObject = [self lastReplayInfo];

if (lastReplay == nil) {
return;
}

NSDictionary<NSString *, id> *jsonObject =
[SentrySerialization deserializeDictionaryFromJsonData:lastReplay];
if (jsonObject == nil) {
return;
}
Expand Down Expand Up @@ -365,6 +373,37 @@ - (void)moveCurrentReplay
}
}

- (void)cleanUp
{
NSURL *replayDir = [self replayDirectory];
NSDictionary<NSString *, id> *lastReplayInfo = [self lastReplayInfo];
NSString *lastReplayFolder = lastReplayInfo[@"path"];

SentryFileManager *fileManager = SentryDependencyContainer.sharedInstance.fileManager;
// Mapping replay folder here and not in dispatched queue to prevent a race condition between
// listing files and creating a new replay session.
Comment on lines +383 to +384
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dispatch queue in the dependency manager is a serial queue, so there shouldn't be a danger of race condition:

dispatch_queue_attr_t attributes = dispatch_queue_attr_make_with_qos_class(
DISPATCH_QUEUE_SERIAL, DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);

In fact, you might even be creating a race condition by not placing this on the serial queue, depending on what other replay operations are or aren't also performed on the queue. Something that was previously deferred might be interleaved with this line and the next block enqueued below in various combinations.

For instance, what happens if multiple calls to SentryReplayApi.start are received in quick succession?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dispatch queue in the dependency manager is a serial queue

But the SentrySessionReplay class that creates a new replay session has it own queue.

what happens if multiple calls to SentryReplayApi.start

The Integration is initialized only once and the cleanUp will run only once.

Copy link
Member

@armcknight armcknight Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the only reason I mentioned the dependency container's queue is because that's what's being used in this file. Should we use one queue for all replay activity? Why the dependency container's here and the private queue in SentrySessionReplay.swift, wouldn't that open up the possibility of more race conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I dont think there is a need for shared dispatch queue, the integration do some setup, and SessionReplay do the rest.

NSArray *replayFiles = [fileManager allFilesInFolder:replayDir.path];
if (replayFiles.count == 0) {
return;
}

[SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchAsyncWithBlock:^{
for (NSString *file in replayFiles) {
// Skip the last replay folder.
if ([file isEqualToString:lastReplayFolder]) {
continue;
}

NSString *filePath = [replayDir.path stringByAppendingPathComponent:file];

// Check if the file is a directory before deleting it.
if ([fileManager isDirectory:filePath]) {
[fileManager removeFileAtPath:filePath];
}
}
}];
}

- (void)pause
{
[self.sessionReplay pause];
Expand Down
3 changes: 2 additions & 1 deletion Sources/Sentry/include/SentryFileManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ SENTRY_NO_INIT
- (NSNumber *_Nullable)readTimezoneOffset;
- (void)storeTimezoneOffset:(NSInteger)offset;
- (void)deleteTimezoneOffset;

- (NSArray<NSString *> *)allFilesInFolder:(NSString *)path;
- (BOOL)isDirectory:(NSString *)path;
BOOL createDirectoryIfNotExists(NSString *path, NSError **error);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,12 +358,26 @@ class SentrySessionReplayIntegrationTests: XCTestCase {
XCTAssertTrue(sessionReplay.isFullSession)
}

func createLastSessionReplay(writeSessionInfo: Bool = true, errorSampleRate: Double = 1) throws {
let options = Options()
options.dsn = "https://user@test.com/test"
options.cacheDirectoryPath = FileManager.default.temporaryDirectory.path
func testCleanUp() throws {
// Create 3 old Sessions
try createLastSessionReplay()
try createLastSessionReplay()
try createLastSessionReplay()
SentryDependencyContainer.sharedInstance().dispatchQueueWrapper = TestSentryDispatchQueueWrapper()

let replayFolder = options.cacheDirectoryPath + "/io.sentry/\(options.parsedDsn?.getHash() ?? "")/replay"
// Start the integration with a configuration that will enable it
startSDK(sessionSampleRate: 0, errorSampleRate: 1)

// Check whether there is only one old session directory and the current session directory
let content = try FileManager.default.contentsOfDirectory(atPath: replayFolder()).filter { name in
!name.hasPrefix("replay") && !name.hasPrefix(".") //remove replay info files and system directories
}

XCTAssertEqual(content.count, 2)
}

func createLastSessionReplay(writeSessionInfo: Bool = true, errorSampleRate: Double = 1) throws {
let replayFolder = replayFolder()
let jsonPath = replayFolder + "/replay.current"
var sessionFolder = UUID().uuidString
let info: [String: Any] = ["replayId": SentryId().sentryIdString,
Expand All @@ -389,6 +403,13 @@ class SentrySessionReplayIntegrationTests: XCTestCase {
sentrySessionReplaySync_writeInfo()
}
}

func replayFolder() -> String {
let options = Options()
options.dsn = "https://user@test.com/test"
options.cacheDirectoryPath = FileManager.default.temporaryDirectory.path
return options.cacheDirectoryPath + "/io.sentry/\(options.parsedDsn?.getHash() ?? "")/replay"
}
}

#endif