Skip to content

Commit 8fcee67

Browse files
authored
fix(replay): Fixes potential out-of-order segments (#13609)
This fixes a potential issue where segments can come in out of order due to our flush "lock" was not being respected in two cases: 1) No current flush in progress, but flush throws an error (this should be rare as the common errors that get thrown should stop the replay completely) 2) Flush is in progress, which skips the code block that releases lock and then calls debouncedFlush. This leaves the lock always set to a resolved (or rejected) promise. This ultimately should not change too much as the flush calls are debounced anyway, but this cleans up the code a bit and also logs any exceptions that may occur. However this can fix issues where segments can come in out of order depending on how long the send request takes. e.g. ![image](https://github.com/user-attachments/assets/ea304892-1c72-4e96-acc6-c714d263980c) where ideally it looks like ![image](https://github.com/user-attachments/assets/8c3e706c-d3b2-43bd-a970-561b32b05458)
1 parent d4a2fcd commit 8fcee67

File tree

2 files changed

+73
-13
lines changed

2 files changed

+73
-13
lines changed

packages/replay-internal/src/replay.ts

+15-13
Original file line numberDiff line numberDiff line change
@@ -1226,27 +1226,29 @@ export class ReplayContainer implements ReplayContainerInterface {
12261226
// TODO FN: Evaluate if we want to stop here, or remove this again?
12271227
}
12281228

1229-
// this._flushLock acts as a lock so that future calls to `_flush()`
1230-
// will be blocked until this promise resolves
1229+
const _flushInProgress = !!this._flushLock;
1230+
1231+
// this._flushLock acts as a lock so that future calls to `_flush()` will
1232+
// be blocked until current flush is finished (i.e. this promise resolves)
12311233
if (!this._flushLock) {
12321234
this._flushLock = this._runFlush();
1233-
await this._flushLock;
1234-
this._flushLock = undefined;
1235-
return;
12361235
}
12371236

1238-
// Wait for previous flush to finish, then call the debounced `_flush()`.
1239-
// It's possible there are other flush requests queued and waiting for it
1240-
// to resolve. We want to reduce all outstanding requests (as well as any
1241-
// new flush requests that occur within a second of the locked flush
1242-
// completing) into a single flush.
1243-
12441237
try {
12451238
await this._flushLock;
12461239
} catch (err) {
1247-
DEBUG_BUILD && logger.error(err);
1240+
this.handleException(err);
12481241
} finally {
1249-
this._debouncedFlush();
1242+
this._flushLock = undefined;
1243+
1244+
if (_flushInProgress) {
1245+
// Wait for previous flush to finish, then call the debounced
1246+
// `_flush()`. It's possible there are other flush requests queued and
1247+
// waiting for it to resolve. We want to reduce all outstanding
1248+
// requests (as well as any new flush requests that occur within a
1249+
// second of the locked flush completing) into a single flush.
1250+
this._debouncedFlush();
1251+
}
12501252
}
12511253
};
12521254

packages/replay-internal/test/integration/flush.test.ts

+58
Original file line numberDiff line numberDiff line change
@@ -493,6 +493,64 @@ describe('Integration | flush', () => {
493493
await replay.start();
494494
});
495495

496+
it('resets flush lock if runFlush rejects/throws', async () => {
497+
mockRunFlush.mockImplementation(
498+
() =>
499+
new Promise((resolve, reject) => {
500+
reject(new Error('runFlush'));
501+
}),
502+
);
503+
try {
504+
await replay['_flush']();
505+
} catch {
506+
// do nothing
507+
}
508+
expect(replay['_flushLock']).toBeUndefined();
509+
});
510+
511+
it('resets flush lock when flush is called multiple times before it resolves', async () => {
512+
let _resolve;
513+
mockRunFlush.mockImplementation(
514+
() =>
515+
new Promise(resolve => {
516+
_resolve = resolve;
517+
}),
518+
);
519+
const mockDebouncedFlush: MockedFunction<ReplayContainer['_debouncedFlush']> = vi.spyOn(replay, '_debouncedFlush');
520+
mockDebouncedFlush.mockImplementation(vi.fn);
521+
mockDebouncedFlush.cancel = vi.fn();
522+
523+
const results = [replay['_flush'](), replay['_flush']()];
524+
expect(replay['_flushLock']).not.toBeUndefined();
525+
526+
_resolve && _resolve();
527+
await Promise.all(results);
528+
expect(replay['_flushLock']).toBeUndefined();
529+
mockDebouncedFlush.mockRestore();
530+
});
531+
532+
it('resets flush lock when flush is called multiple times before it rejects', async () => {
533+
let _reject;
534+
mockRunFlush.mockImplementation(
535+
() =>
536+
new Promise((_, reject) => {
537+
_reject = reject;
538+
}),
539+
);
540+
const mockDebouncedFlush: MockedFunction<ReplayContainer['_debouncedFlush']> = vi.spyOn(replay, '_debouncedFlush');
541+
mockDebouncedFlush.mockImplementation(vi.fn);
542+
mockDebouncedFlush.cancel = vi.fn();
543+
expect(replay['_flushLock']).toBeUndefined();
544+
replay['_flush']();
545+
const result = replay['_flush']();
546+
expect(replay['_flushLock']).not.toBeUndefined();
547+
548+
_reject && _reject(new Error('Throw runFlush'));
549+
await result;
550+
expect(replay['_flushLock']).toBeUndefined();
551+
mockDebouncedFlush.mockRestore();
552+
});
553+
496554
/**
497555
* Assuming the user wants to record a session
498556
* when calling flush() without replay being enabled

0 commit comments

Comments
 (0)