From 63b04334c3f77145f11cd52296b1e52e60140ac7 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 14:06:51 -0700 Subject: [PATCH 1/6] feat(highlight.run): stop session replay when the backend refuses session data Every public graph failure was retried three times with backoff, permanent ones included, and recording only stopped when an upload timed out. A refused environment therefore cost four doomed requests per page load and left rrweb buffering events that nothing would ever upload. Failures are now classified the way the mobile SDKs classify them, by status code and by the server's `extensions.retryable` flag, so an unrecoverable one skips the retries and ends recording for this page load. Co-authored-by: Cursor --- sdk/highlight-run/src/client/index.tsx | 29 ++- .../client/utils/error-recoverability.test.ts | 113 +++++++++++ .../src/client/utils/error-recoverability.ts | 74 +++++++ .../src/client/utils/graph.test.ts | 90 +++++++++ sdk/highlight-run/src/client/utils/graph.ts | 18 +- .../highlight-client-worker-errors.test.ts | 183 ++++++++++++++++++ .../client/workers/highlight-client-worker.ts | 89 ++++++--- sdk/highlight-run/src/client/workers/types.ts | 12 +- sdk/highlight-run/src/sdk/record.ts | 29 ++- 9 files changed, 587 insertions(+), 50 deletions(-) create mode 100644 sdk/highlight-run/src/client/utils/error-recoverability.test.ts create mode 100644 sdk/highlight-run/src/client/utils/error-recoverability.ts create mode 100644 sdk/highlight-run/src/client/utils/graph.test.ts create mode 100644 sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts diff --git a/sdk/highlight-run/src/client/index.tsx b/sdk/highlight-run/src/client/index.tsx index 8a7f803222..a9dd8823db 100644 --- a/sdk/highlight-run/src/client/index.tsx +++ b/sdk/highlight-run/src/client/index.tsx @@ -112,7 +112,7 @@ import { getDefaultDataURLOptions, isMetricSafeNumber } from './utils/utils' import { type HighlightClientRequestWorker } from './workers/highlight-client-worker' import { payloadToBase64 } from './utils/payload' import HighlightClientWorker from './workers/highlight-client-worker?worker&inline' -import { MessageType, PropertyType } from './workers/types' +import { MessageType, PropertyType, StopReason } from './workers/types' import { parseError } from './utils/errors' import { Attributes, @@ -297,11 +297,18 @@ export class Highlight { e.data.response.payload, ) } else if (e.data.response?.type === MessageType.Stop) { + const { reason } = e.data.response HighlightWarning( - 'Stopping recording due to worker failure', + `Stopping recording due to worker failure: ${reason}`, e.data.response, ) this.stopRecording(false) + if (reason === StopReason.UnrecoverableError) { + // Nothing more will be uploaded during this page load, so detach the + // recorder and drop what it buffered instead of growing the buffer forever. + this._stopRecorder() + this.events = [] + } } } @@ -1441,10 +1448,8 @@ SessionSecureID: ${this.sessionData.sessionSecureID}`, ) } this.state = 'NotRecording' - // stop rrweb recording mutation observers - if (manual && this._recordStop) { - this._recordStop() - this._recordStop = undefined + if (manual) { + this._stopRecorder() } // stop all other event listeners, to be restarted on initialize() this.listeners.forEach((stop) => stop()) @@ -1452,6 +1457,18 @@ SessionSecureID: ${this.sessionData.sessionSecureID}`, void shutdown() } + /** + * Stops rrweb's recording mutation observers. `stopRecording` only does this on a manual stop + * because rrweb's stop -> restart path is unreliable (eg. iframe listeners), so call this only + * when recording will not resume during this page load. + */ + _stopRecorder() { + if (this._recordStop) { + this._recordStop() + this._recordStop = undefined + } + } + getCurrentSessionTimestamp() { return this._recordingStartTime } diff --git a/sdk/highlight-run/src/client/utils/error-recoverability.test.ts b/sdk/highlight-run/src/client/utils/error-recoverability.test.ts new file mode 100644 index 0000000000..955ca3749b --- /dev/null +++ b/sdk/highlight-run/src/client/utils/error-recoverability.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'vitest' +import { ClientError } from 'graphql-request' +import { PublicGraphError } from '../graph/generated/schemas' +import { + isErrorRecoverable, + isHttpErrorRecoverable, +} from './error-recoverability' + +type TestGraphQLError = { + message: string + extensions?: { retryable?: boolean } +} + +function clientError(status: number, errors?: TestGraphQLError[]): ClientError { + return new ClientError( + { status, errors } as unknown as ClientError['response'], + { query: 'mutation pushPayload { pushPayload }' }, + ) +} + +describe('isHttpErrorRecoverable', () => { + it('treats transient 4xx statuses as recoverable', () => { + expect(isHttpErrorRecoverable(400)).toBe(true) + expect(isHttpErrorRecoverable(408)).toBe(true) + expect(isHttpErrorRecoverable(429)).toBe(true) + }) + + it('treats every other 4xx status as unrecoverable', () => { + expect(isHttpErrorRecoverable(401)).toBe(false) + expect(isHttpErrorRecoverable(402)).toBe(false) + expect(isHttpErrorRecoverable(403)).toBe(false) + expect(isHttpErrorRecoverable(404)).toBe(false) + expect(isHttpErrorRecoverable(422)).toBe(false) + }) + + it('treats server errors and non-error statuses as recoverable', () => { + expect(isHttpErrorRecoverable(500)).toBe(true) + expect(isHttpErrorRecoverable(502)).toBe(true) + expect(isHttpErrorRecoverable(503)).toBe(true) + expect(isHttpErrorRecoverable(504)).toBe(true) + expect(isHttpErrorRecoverable(200)).toBe(true) + expect(isHttpErrorRecoverable(0)).toBe(true) + }) +}) + +describe('isErrorRecoverable', () => { + it('treats errors of unknown origin as recoverable', () => { + expect(isErrorRecoverable(new Error('Failed to fetch'))).toBe(true) + expect(isErrorRecoverable(new TypeError('NetworkError'))).toBe(true) + expect(isErrorRecoverable(undefined)).toBe(true) + }) + + it('classifies a rejected request by its status', () => { + expect(isErrorRecoverable(clientError(403))).toBe(false) + expect(isErrorRecoverable(clientError(404))).toBe(false) + expect(isErrorRecoverable(clientError(429))).toBe(true) + expect(isErrorRecoverable(clientError(503))).toBe(true) + }) + + it('treats a 200 carrying GraphQL errors as unrecoverable', () => { + expect( + isErrorRecoverable(clientError(200, [{ message: 'not allowed' }])), + ).toBe(false) + }) + + it('honors an explicit retryable flag over the status', () => { + expect( + isErrorRecoverable( + clientError(403, [ + { message: 'try again', extensions: { retryable: true } }, + ]), + ), + ).toBe(true) + expect( + isErrorRecoverable( + clientError(429, [ + { message: 'give up', extensions: { retryable: false } }, + ]), + ), + ).toBe(false) + expect( + isErrorRecoverable( + clientError(200, [ + { message: 'try again', extensions: { retryable: true } }, + ]), + ), + ).toBe(true) + }) + + it('takes the most pessimistic retryable flag when errors disagree', () => { + expect( + isErrorRecoverable( + clientError(200, [ + { message: 'try again', extensions: { retryable: true } }, + { message: 'give up', extensions: { retryable: false } }, + ]), + ), + ).toBe(false) + }) + + it('ignores a retryable flag on a permanent public graph error', () => { + expect( + isErrorRecoverable( + clientError(200, [ + { + message: PublicGraphError.BillingQuotaExceeded, + extensions: { retryable: true }, + }, + ]), + ), + ).toBe(false) + }) +}) diff --git a/sdk/highlight-run/src/client/utils/error-recoverability.ts b/sdk/highlight-run/src/client/utils/error-recoverability.ts new file mode 100644 index 0000000000..46a772f2c7 --- /dev/null +++ b/sdk/highlight-run/src/client/utils/error-recoverability.ts @@ -0,0 +1,74 @@ +import { ClientError } from 'graphql-request' +import { PublicGraphError } from '../graph/generated/schemas' + +type GraphQLErrors = NonNullable + +// Public graph errors that are permanent whatever the transport reports. +const UNRECOVERABLE_ERRORS: string[] = [ + PublicGraphError.BillingQuotaExceeded.toString(), +] + +/** + * Tests whether an HTTP error status represents a condition that might resolve on its own if we + * retry. 4xx statuses are permanent apart from the three that describe a transient condition; + * anything else, including 5xx, is worth another attempt. + */ +export const isHttpErrorRecoverable = (statusCode: number): boolean => { + if (statusCode < 400 || statusCode >= 500) { + return true + } + + switch (statusCode) { + case 400: // bad request + case 408: // request timeout + case 429: // too many requests + return true + default: + return false // all other 4xx errors are unrecoverable + } +} + +/** + * Classifies a failed public graph request as recoverable (retrying may succeed) or unrecoverable + * (permanent for this page load). Errors of unknown origin count as recoverable: retrying costs a + * backed-off request, while a wrong permanent verdict silently disables recording. + */ +export const isErrorRecoverable = (error: unknown): boolean => { + if (!(error instanceof ClientError)) { + // Offline, DNS and aborted-fetch failures reject with the raw error and carry no + // permanent signal. + return true + } + + const errors = error.response.errors + if (errors?.some((e) => UNRECOVERABLE_ERRORS.includes(e.message))) { + return false + } + + // An explicit `retryable` is more specific than the status code, so it wins. + const retryable = retryableFlag(errors) + if (retryable !== undefined) { + return retryable + } + + if (error.response.status >= 400) { + return isHttpErrorRecoverable(error.response.status) + } + + // The public graph also reports a rejected request as `200` + `errors`. Classify those like a + // generic 4xx: permanent unless the server marked an error retryable. + return !errors?.length +} + +/** The server's retry verdict for a set of GraphQL errors, or `undefined` when none states one. */ +const retryableFlag = ( + errors: GraphQLErrors | undefined, +): boolean | undefined => { + if (errors?.some((e) => e.extensions?.retryable === false)) { + return false + } + if (errors?.some((e) => e.extensions?.retryable === true)) { + return true + } + return undefined +} diff --git a/sdk/highlight-run/src/client/utils/graph.test.ts b/sdk/highlight-run/src/client/utils/graph.test.ts new file mode 100644 index 0000000000..d743707348 --- /dev/null +++ b/sdk/highlight-run/src/client/utils/graph.test.ts @@ -0,0 +1,90 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { ClientError } from 'graphql-request' +import { + getGraphQLRequestWrapper, + MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS, +} from './graph' + +function clientError(status: number): ClientError { + return new ClientError({ status } as unknown as ClientError['response'], { + query: 'mutation pushPayload { pushPayload }', + }) +} + +/** Runs the wrapper to completion, letting every backoff timer fire immediately. */ +async function withoutBackoff(request: Promise): Promise { + const settled = request.then( + (value) => ({ value }), + (error) => ({ error }), + ) + await vi.runAllTimersAsync() + const outcome = await settled + if ('error' in outcome) { + throw outcome.error + } + return outcome.value +} + +describe('getGraphQLRequestWrapper', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('does not retry a request that succeeds', async () => { + const requestFn = vi.fn().mockResolvedValue('ok') + + await expect( + withoutBackoff( + getGraphQLRequestWrapper()(requestFn, 'pushPayload'), + ), + ).resolves.toBe('ok') + expect(requestFn).toHaveBeenCalledTimes(1) + }) + + it('retries a recoverable failure until it succeeds', async () => { + const requestFn = vi + .fn() + .mockRejectedValueOnce(clientError(503)) + .mockRejectedValueOnce(new Error('Failed to fetch')) + .mockResolvedValue('ok') + + await expect( + withoutBackoff( + getGraphQLRequestWrapper()(requestFn, 'pushPayload'), + ), + ).resolves.toBe('ok') + expect(requestFn).toHaveBeenCalledTimes(3) + }) + + it('gives up on a recoverable failure after the retry budget', async () => { + const error = clientError(503) + const requestFn = vi.fn().mockRejectedValue(error) + + await expect( + withoutBackoff( + getGraphQLRequestWrapper()(requestFn, 'pushPayload'), + ), + ).rejects.toBe(error) + expect(requestFn).toHaveBeenCalledTimes( + MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS + 1, + ) + }) + + it('throws an unrecoverable failure without retrying', async () => { + const error = clientError(403) + const requestFn = vi.fn().mockRejectedValue(error) + + await expect( + withoutBackoff( + getGraphQLRequestWrapper()(requestFn, 'pushPayload'), + ), + ).rejects.toBe(error) + expect(requestFn).toHaveBeenCalledTimes(1) + }) +}) diff --git a/sdk/highlight-run/src/client/utils/graph.ts b/sdk/highlight-run/src/client/utils/graph.ts index 08d18fb5f7..28c6d19b55 100644 --- a/sdk/highlight-run/src/client/utils/graph.ts +++ b/sdk/highlight-run/src/client/utils/graph.ts @@ -1,5 +1,4 @@ -import { ClientError } from 'graphql-request' -import { PublicGraphError } from '../graph/generated/schemas' +import { isErrorRecoverable } from './error-recoverability' export const MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS = 3 @@ -7,17 +6,6 @@ export const MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS = 3 export const BASE_DELAY_MS = 1000 export const BACKOFF_DELAY_MS = 500 -// Do not retry if any of these public graph errors are thrown -const NON_RETRYABLE_ERRORS = [PublicGraphError.BillingQuotaExceeded.toString()] - -// A `ClientError` is retryable if none of the response errors is non-retryable -const isErrorRetryable = (error: ClientError): boolean => { - const match = error.response.errors?.find((e) => - NON_RETRYABLE_ERRORS.includes(e.message), - ) - return match === undefined -} - export const getGraphQLRequestWrapper = () => { const graphQLRequestWrapper = async ( requestFn: () => Promise, @@ -29,7 +17,9 @@ export const getGraphQLRequestWrapper = () => { try { return await requestFn() } catch (error: any) { - if (error instanceof ClientError && !isErrorRetryable(error)) { + // Retrying an unrecoverable failure only delays the caller and adds load the backend + // already rejected. + if (!isErrorRecoverable(error)) { throw error } diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts new file mode 100644 index 0000000000..0132653239 --- /dev/null +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts @@ -0,0 +1,183 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { ClientError } from 'graphql-request' +import { + CustomEventResponse, + HighlightClientWorkerParams, + HighlightClientWorkerResponse, + MessageType, + StatusResponse, + StopEventResponse, + StopReason, +} from './types' + +// Every operation the worker performs rejects with this, so a single knob decides whether the +// worker sees a recoverable or an unrecoverable failure. +const graph = vi.hoisted(() => ({ error: undefined as unknown })) + +vi.mock('../graph/generated/operations', async (importOriginal) => { + const actual = + await importOriginal() + const reject = () => Promise.reject(graph.error) + return { + ...actual, + getSdk: () => ({ + identifySession: reject, + addSessionProperties: reject, + PushSessionEvents: reject, + pushMetrics: reject, + }), + } +}) + +interface TestWorker { + postMessage: (params: HighlightClientWorkerParams) => void + onmessage: + | ((event: MessageEvent) => void) + | null + terminate: () => void +} + +function clientError(status: number): ClientError { + return new ClientError({ status } as unknown as ClientError['response'], { + query: 'mutation identifySession { identifySession }', + }) +} + +function initializeMessage(): HighlightClientWorkerParams { + return { + message: { + type: MessageType.Initialize, + backend: 'https://test.highlight.io/graphql', + sessionSecureID: 'test-session-123', + debug: false, + recordingStartTime: Date.now(), + }, + } +} + +function identifyMessage(userIdentifier: string): HighlightClientWorkerParams { + return { + message: { + type: MessageType.Identify, + userIdentifier, + userObject: { name: 'Test User' }, + }, + } +} + +describe('highlight-client-worker error handling', () => { + let worker: TestWorker + let responses: HighlightClientWorkerResponse[] + + const responsesOfType = (type: MessageType): T[] => + responses + .filter((r) => r.response?.type === type) + .map((r) => r.response as T) + + const status = async (): Promise => { + const before = responsesOfType( + MessageType.GetStatus, + ).length + worker.postMessage({ message: { type: MessageType.GetStatus } }) + await vi.waitFor(() => { + expect( + responsesOfType(MessageType.GetStatus).length, + ).toBeGreaterThan(before) + }) + const all = responsesOfType(MessageType.GetStatus) + return all[all.length - 1] + } + + beforeEach(() => { + vi.resetModules() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + // See highlight-client-worker.test.ts: the source module (rather than the `?worker&inline` + // build output) is what @vitest/web-worker can run for real. + worker = new Worker( + new URL('./highlight-client-worker.ts', import.meta.url), + { type: 'module' }, + ) as unknown as TestWorker + responses = [] + worker.onmessage = (event) => { + responses.push(event.data) + } + }) + + afterEach(() => { + worker.terminate() + vi.restoreAllMocks() + }) + + it('stops recording when the backend rejects a request permanently', async () => { + graph.error = clientError(403) + + worker.postMessage(initializeMessage()) + worker.postMessage(identifyMessage('refused-user')) + + await vi.waitFor(() => { + const stops = responsesOfType(MessageType.Stop) + expect(stops).toHaveLength(1) + expect(stops[0].reason).toBe(StopReason.UnrecoverableError) + }) + + expect((await status()).initialized).toBe(false) + }) + + it('drops later messages after a permanent rejection', async () => { + graph.error = clientError(403) + + worker.postMessage(initializeMessage()) + worker.postMessage(identifyMessage('refused-user')) + + await vi.waitFor(() => { + expect( + responsesOfType(MessageType.Stop), + ).toHaveLength(1) + }) + + worker.postMessage(identifyMessage('dropped-user')) + + const { pendingCount } = await status() + expect(pendingCount).toBe(0) + expect( + responsesOfType(MessageType.CustomEvent).some( + (e) => e.payload?.includes('dropped-user'), + ), + ).toBe(false) + }) + + it('keeps recording when the failure may resolve on its own', async () => { + graph.error = clientError(503) + + worker.postMessage(initializeMessage()) + worker.postMessage(identifyMessage('retried-user')) + + await vi.waitFor(() => { + expect( + responsesOfType( + MessageType.CustomEvent, + ).some((e) => e.payload?.includes('retried-user')), + ).toBe(true) + }) + + expect(responsesOfType(MessageType.Stop)).toEqual([]) + expect((await status()).initialized).toBe(true) + }) + + it('resumes after a new session initializes', async () => { + graph.error = clientError(403) + + worker.postMessage(initializeMessage()) + worker.postMessage(identifyMessage('refused-user')) + + await vi.waitFor(() => { + expect( + responsesOfType(MessageType.Stop), + ).toHaveLength(1) + }) + + worker.postMessage(initializeMessage()) + + expect((await status()).initialized).toBe(true) + }) +}) diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts index c6dcfa573c..a84e7a4324 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts @@ -10,6 +10,7 @@ import { payloadToBase64 } from '../utils/payload' import { ReplayEventsInput } from '../graph/generated/schemas' import { Logger } from '../logger' import { MetricCategory } from '../types/client' +import { isErrorRecoverable } from '../utils/error-recoverability' import { getGraphQLRequestWrapper } from '../utils/graph' import { MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS, @@ -26,6 +27,8 @@ import { MessageType, MetricsMessage, PropertiesMessage, + StopEventResponse, + StopReason, } from './types' export interface HighlightClientRequestWorker { @@ -100,6 +103,7 @@ function stringifyProperties( let sessionSecureID: string let numberOfFailedRequests: number = 0 let numberOfFailedPushPayloads: number = 0 + let hasFailedUnrecoverably: boolean = false let debug: boolean = false let recordingStartTime: number = 0 let logger = new Logger(false, '[worker]') @@ -116,6 +120,7 @@ function stringifyProperties( const shouldSendRequest = (): boolean => { return ( + !hasFailedUnrecoverably && recordingStartTime !== 0 && numberOfFailedRequests < MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS && !!sessionSecureID?.length @@ -132,6 +137,53 @@ function stringifyProperties( }) } + const stopRecording = ( + reason: StopReason, + details?: Pick< + StopEventResponse, + 'requestStart' | 'asyncEventsResponse' + >, + ) => { + worker.postMessage({ + response: { + type: MessageType.Stop, + reason, + ...details, + }, + }) + + processPropertiesMessage({ + type: MessageType.Properties, + propertiesObject: { + stopReason: reason, + }, + propertyType: { type: 'track' }, + }) + } + + /** + * Accounts for a message we could not send. A recoverable failure spends part of the retry + * budget, while an unrecoverable one ends recording for this page load: the backend would + * reject every later request the same way, so continuing only buffers events that can never + * be uploaded. + */ + const handleFailedMessage = (e: unknown) => { + if (debug) { + console.error(e) + } + + if (isErrorRecoverable(e)) { + numberOfFailedRequests += 1 + return + } + + console.warn(`Session data was rejected, stopping recording.`, e) + hasFailedUnrecoverably = true + pendingMessages.length = 0 + metricsPayload.length = 0 + stopRecording(StopReason.UnrecoverableError) + } + const processAsyncEventsMessage = async (msg: AsyncEventsMessage) => { const { id, @@ -229,20 +281,9 @@ function stringifyProperties( `Uploading pushPayload took too long, stopping recording to avoid OOM.`, ) - worker.postMessage({ - response: { - type: MessageType.Stop, - requestStart, - asyncEventsResponse: response, - }, - }) - - processPropertiesMessage({ - type: MessageType.Properties, - propertiesObject: { - stopReason: 'Push Payload Timeout', - }, - propertyType: { type: 'track' }, + stopRecording(StopReason.PushPayloadTimeout, { + requestStart, + asyncEventsResponse: response, }) } } @@ -374,10 +415,7 @@ function stringifyProperties( await processMessage(msg) numberOfFailedRequests = 0 } catch (e) { - if (debug) { - console.error(e) - } - numberOfFailedRequests += 1 + handleFailedMessage(e) } } } @@ -389,6 +427,9 @@ function stringifyProperties( debug = e.data.message.debug recordingStartTime = e.data.message.recordingStartTime logger.debug = debug + // The client only initializes after `initializeSession` succeeded, so an earlier + // rejection no longer applies. + hasFailedUnrecoverably = false graphqlSDK = getSdk( new GraphQLClient(backend, { headers: {}, @@ -407,6 +448,7 @@ function stringifyProperties( metricsPayload.length = 0 numberOfFailedRequests = 0 numberOfFailedPushPayloads = 0 + hasFailedUnrecoverably = false // Reset sessionSecureID and recordingStartTime so that messages arriving // between Reset and new Initialize are queued rather than processed with // the old session SecureID @@ -427,6 +469,12 @@ function stringifyProperties( return } + // Drop messages once the backend has permanently rejected this session: queueing them + // would grow without bound because nothing is going to send them. + if (hasFailedUnrecoverably) { + return + } + if (!shouldSendRequest()) { pendingMessages.push(e.data.message) return @@ -437,10 +485,7 @@ function stringifyProperties( numberOfFailedRequests = 0 await drainPendingMessages() } catch (e) { - if (debug) { - console.error(e) - } - numberOfFailedRequests += 1 + handleFailedMessage(e) } } } diff --git a/sdk/highlight-run/src/client/workers/types.ts b/sdk/highlight-run/src/client/workers/types.ts index 752b5216a8..4d3ab2e908 100644 --- a/sdk/highlight-run/src/client/workers/types.ts +++ b/sdk/highlight-run/src/client/workers/types.ts @@ -100,10 +100,18 @@ export type CustomEventResponse = { payload: any } +export enum StopReason { + /** Uploads are outrunning the recording, so events would pile up in memory. */ + PushPayloadTimeout = 'Push Payload Timeout', + /** The backend rejected the session data permanently; retrying cannot help. */ + UnrecoverableError = 'Unrecoverable Error', +} + export type StopEventResponse = { type: MessageType.Stop - requestStart: number - asyncEventsResponse: AsyncEventsResponse + reason: StopReason + requestStart?: number + asyncEventsResponse?: AsyncEventsResponse } export type HighlightClientWorkerParams = { diff --git a/sdk/highlight-run/src/sdk/record.ts b/sdk/highlight-run/src/sdk/record.ts index 79c0420f72..fde84a27de 100644 --- a/sdk/highlight-run/src/sdk/record.ts +++ b/sdk/highlight-run/src/sdk/record.ts @@ -79,7 +79,7 @@ import { getDefaultDataURLOptions } from '../client/utils/utils' import { type HighlightClientRequestWorker } from '../client/workers/highlight-client-worker' import { payloadToBase64 } from '../client/utils/payload' import HighlightClientWorker from '../client/workers/highlight-client-worker?worker&inline' -import { MessageType, PropertyType } from '../client/workers/types' +import { MessageType, PropertyType, StopReason } from '../client/workers/types' import { IntegrationClient } from '../integrations' import { Record } from '../api/record' import { internalLog } from './util' @@ -184,13 +184,20 @@ export class RecordSDK implements Record { e.data.response.payload, ) } else if (e.data.response?.type === MessageType.Stop) { + const { reason } = e.data.response internalLog( 'worker.onmessage', 'warn', - 'Stopping recording due to worker failure', + `Stopping recording due to worker failure: ${reason}`, e.data.response, ) this.stop(false) + if (reason === StopReason.UnrecoverableError) { + // Nothing more will be uploaded during this page load, so detach the recorder + // and drop what it buffered instead of growing the buffer forever. + this._stopRecorder() + this.events = [] + } } } @@ -950,16 +957,26 @@ SessionSecureID: ${this.sessionData.sessionSecureID}`, ) } this.state = 'NotRecording' - // stop rrweb recording mutation observers - if (manual && this._recordStop) { - this._recordStop() - this._recordStop = undefined + if (manual) { + this._stopRecorder() } // stop all other event listeners, to be restarted on initialize() this.listeners.forEach((stop) => stop()) this.listeners = [] } + /** + * Stops rrweb's recording mutation observers. `stop` only does this on a manual stop because + * rrweb's stop -> restart path is unreliable (eg. iframe listeners), so call this only when + * recording will not resume during this page load. + */ + _stopRecorder() { + if (this._recordStop) { + this._recordStop() + this._recordStop = undefined + } + } + /** * Returns the current timestamp for the current session. */ From 653ff38cd69666d91bd76cd7e6dcf4e83ecc1ff7 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 14:22:30 -0700 Subject: [PATCH 2/6] fix(highlight.run): release the recorder whenever uploads stop The worker already asked the client to stop when uploads timed out, but that stop left rrweb attached while `_save` no longer ran, so events piled up for the rest of the page load, which is what the stop was meant to prevent. Five failed sends in a row were worse: the worker went quiet without stopping anything and queued every later payload for good. Both cases now end recording the same way. The worker drops what it buffered and stops accepting work until the client initializes again, and the client detaches rrweb and clears its buffer for any stop the worker asks for. Co-authored-by: Cursor --- .../src/__tests__/record-worker-stop.test.ts | 86 +++++++++++++++++++ sdk/highlight-run/src/client/index.tsx | 16 ++-- .../highlight-client-worker-errors.test.ts | 22 +++++ .../client/workers/highlight-client-worker.ts | 55 +++++++----- sdk/highlight-run/src/client/workers/types.ts | 2 + sdk/highlight-run/src/sdk/record.ts | 16 ++-- 6 files changed, 158 insertions(+), 39 deletions(-) create mode 100644 sdk/highlight-run/src/__tests__/record-worker-stop.test.ts diff --git a/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts b/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts new file mode 100644 index 0000000000..d0b5a56e37 --- /dev/null +++ b/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts @@ -0,0 +1,86 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { RecordSDK } from '../sdk/record' +import { MessageType, StopReason } from '../client/workers/types' + +const recordStop = vi.fn() +vi.mock('@highlight-run/rrweb', () => ({ + addCustomEvent: vi.fn(), + record: () => recordStop, +})) + +vi.mock('../client/graph/generated/operations', () => ({ + getSdk: () => ({ + initializeSession: vi.fn().mockResolvedValue({ + initializeSession: { + secure_id: 'test-session', + project_id: '1', + }, + }), + }), +})) + +vi.mock('../client/workers/highlight-client-worker?worker&inline', () => ({ + default: class MockWorker { + onmessage: any + postMessage() {} + }, +})) + +async function startedSDK(): Promise { + const sdk = new RecordSDK({ + organizationID: '1', + sessionSecureID: 'seed', + }) + await sdk.start() + return sdk +} + +/** Delivers a worker response to the SDK the way the real worker would. */ +function postToSDK(sdk: RecordSDK, response: unknown) { + sdk._worker.onmessage({ data: { response } } as MessageEvent) +} + +describe('RecordSDK worker stop handling', () => { + beforeEach(() => { + vi.useFakeTimers() + recordStop.mockClear() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it.each([ + StopReason.PushPayloadTimeout, + StopReason.UnrecoverableError, + StopReason.RetriesExhausted, + ])( + 'releases the recorder when the worker stops us (%s)', + async (reason) => { + const sdk = await startedSDK() + sdk.events.push({ type: 3, data: {}, timestamp: 1 } as any) + + postToSDK(sdk, { type: MessageType.Stop, reason }) + + expect(recordStop).toHaveBeenCalledTimes(1) + expect(sdk.events).toEqual([]) + expect(sdk.getRecordingState()).toBe('NotRecording') + }, + ) + + it('keeps recording while the worker only reports uploads', async () => { + const sdk = await startedSDK() + sdk.events.push({ type: 3, data: {}, timestamp: 1 } as any) + + postToSDK(sdk, { + type: MessageType.AsyncEvents, + id: 1, + eventsSize: 100, + compressedSize: 50, + }) + + expect(recordStop).not.toHaveBeenCalled() + expect(sdk.events).toHaveLength(1) + expect(sdk.getRecordingState()).toBe('Recording') + }) +}) diff --git a/sdk/highlight-run/src/client/index.tsx b/sdk/highlight-run/src/client/index.tsx index a9dd8823db..3f7ccd7e8c 100644 --- a/sdk/highlight-run/src/client/index.tsx +++ b/sdk/highlight-run/src/client/index.tsx @@ -112,7 +112,7 @@ import { getDefaultDataURLOptions, isMetricSafeNumber } from './utils/utils' import { type HighlightClientRequestWorker } from './workers/highlight-client-worker' import { payloadToBase64 } from './utils/payload' import HighlightClientWorker from './workers/highlight-client-worker?worker&inline' -import { MessageType, PropertyType, StopReason } from './workers/types' +import { MessageType, PropertyType } from './workers/types' import { parseError } from './utils/errors' import { Attributes, @@ -297,18 +297,16 @@ export class Highlight { e.data.response.payload, ) } else if (e.data.response?.type === MessageType.Stop) { - const { reason } = e.data.response HighlightWarning( - `Stopping recording due to worker failure: ${reason}`, + `Stopping recording due to worker failure: ${e.data.response.reason}`, e.data.response, ) this.stopRecording(false) - if (reason === StopReason.UnrecoverableError) { - // Nothing more will be uploaded during this page load, so detach the - // recorder and drop what it buffered instead of growing the buffer forever. - this._stopRecorder() - this.events = [] - } + // The worker only asks us to stop when it cannot upload what we record, and + // `_save` no longer runs once we are not recording, so detach the recorder and + // drop its buffer instead of filling memory for the rest of this page load. + this._stopRecorder() + this.events = [] } } diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts index 0132653239..374d01f801 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { ClientError } from 'graphql-request' +import { MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS } from './constants' import { CustomEventResponse, HighlightClientWorkerParams, @@ -164,6 +165,27 @@ describe('highlight-client-worker error handling', () => { expect((await status()).initialized).toBe(true) }) + it('stops recording once the retry budget is spent', async () => { + graph.error = clientError(503) + + worker.postMessage(initializeMessage()) + for (let i = 0; i < MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS; i++) { + worker.postMessage(identifyMessage(`retried-user-${i}`)) + } + + await vi.waitFor(() => { + const stops = responsesOfType(MessageType.Stop) + expect(stops).toHaveLength(1) + expect(stops[0].reason).toBe(StopReason.RetriesExhausted) + }) + + worker.postMessage(identifyMessage('dropped-user')) + + const { pendingCount, initialized } = await status() + expect(pendingCount).toBe(0) + expect(initialized).toBe(false) + }) + it('resumes after a new session initializes', async () => { graph.error = clientError(403) diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts index a84e7a4324..154e038b2b 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts @@ -103,7 +103,7 @@ function stringifyProperties( let sessionSecureID: string let numberOfFailedRequests: number = 0 let numberOfFailedPushPayloads: number = 0 - let hasFailedUnrecoverably: boolean = false + let hasStoppedRecording: boolean = false let debug: boolean = false let recordingStartTime: number = 0 let logger = new Logger(false, '[worker]') @@ -120,9 +120,8 @@ function stringifyProperties( const shouldSendRequest = (): boolean => { return ( - !hasFailedUnrecoverably && + !hasStoppedRecording && recordingStartTime !== 0 && - numberOfFailedRequests < MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS && !!sessionSecureID?.length ) } @@ -137,6 +136,11 @@ function stringifyProperties( }) } + /** + * Asks the client to stop recording and stops accepting work until it initializes again. + * Everything buffered here is dropped: the client stops handing us events, so a queue kept + * across a stop would only hold memory for data that will never be uploaded. + */ const stopRecording = ( reason: StopReason, details?: Pick< @@ -144,6 +148,10 @@ function stringifyProperties( 'requestStart' | 'asyncEventsResponse' >, ) => { + hasStoppedRecording = true + pendingMessages.length = 0 + metricsPayload.length = 0 + worker.postMessage({ response: { type: MessageType.Stop, @@ -162,26 +170,30 @@ function stringifyProperties( } /** - * Accounts for a message we could not send. A recoverable failure spends part of the retry - * budget, while an unrecoverable one ends recording for this page load: the backend would - * reject every later request the same way, so continuing only buffers events that can never - * be uploaded. + * Accounts for a message we could not send. An unrecoverable failure ends recording right + * away, because the backend would reject every later request the same way. A recoverable one + * spends part of the retry budget, and exhausting that budget also ends recording: the client + * would otherwise keep producing payloads that pile up here unsent. */ const handleFailedMessage = (e: unknown) => { if (debug) { console.error(e) } - if (isErrorRecoverable(e)) { - numberOfFailedRequests += 1 + if (!isErrorRecoverable(e)) { + console.warn(`Session data was rejected, stopping recording.`, e) + stopRecording(StopReason.UnrecoverableError) return } - console.warn(`Session data was rejected, stopping recording.`, e) - hasFailedUnrecoverably = true - pendingMessages.length = 0 - metricsPayload.length = 0 - stopRecording(StopReason.UnrecoverableError) + numberOfFailedRequests += 1 + if (numberOfFailedRequests >= MAX_PUBLIC_GRAPH_RETRY_ATTEMPTS) { + console.warn( + `Session data failed to upload ${numberOfFailedRequests} times, stopping recording.`, + e, + ) + stopRecording(StopReason.RetriesExhausted) + } } const processAsyncEventsMessage = async (msg: AsyncEventsMessage) => { @@ -427,9 +439,10 @@ function stringifyProperties( debug = e.data.message.debug recordingStartTime = e.data.message.recordingStartTime logger.debug = debug - // The client only initializes after `initializeSession` succeeded, so an earlier - // rejection no longer applies. - hasFailedUnrecoverably = false + // The client only initializes after `initializeSession` succeeded, so whatever made + // us stop no longer applies. + hasStoppedRecording = false + numberOfFailedRequests = 0 graphqlSDK = getSdk( new GraphQLClient(backend, { headers: {}, @@ -448,7 +461,7 @@ function stringifyProperties( metricsPayload.length = 0 numberOfFailedRequests = 0 numberOfFailedPushPayloads = 0 - hasFailedUnrecoverably = false + hasStoppedRecording = false // Reset sessionSecureID and recordingStartTime so that messages arriving // between Reset and new Initialize are queued rather than processed with // the old session SecureID @@ -469,9 +482,9 @@ function stringifyProperties( return } - // Drop messages once the backend has permanently rejected this session: queueing them - // would grow without bound because nothing is going to send them. - if (hasFailedUnrecoverably) { + // Drop messages once recording has stopped: queueing them would grow without bound + // because nothing is going to send them. + if (hasStoppedRecording) { return } diff --git a/sdk/highlight-run/src/client/workers/types.ts b/sdk/highlight-run/src/client/workers/types.ts index 4d3ab2e908..f4faceb5e8 100644 --- a/sdk/highlight-run/src/client/workers/types.ts +++ b/sdk/highlight-run/src/client/workers/types.ts @@ -105,6 +105,8 @@ export enum StopReason { PushPayloadTimeout = 'Push Payload Timeout', /** The backend rejected the session data permanently; retrying cannot help. */ UnrecoverableError = 'Unrecoverable Error', + /** Enough uploads failed in a row that the backend is not answering us at all. */ + RetriesExhausted = 'Retries Exhausted', } export type StopEventResponse = { diff --git a/sdk/highlight-run/src/sdk/record.ts b/sdk/highlight-run/src/sdk/record.ts index fde84a27de..4587b20fdd 100644 --- a/sdk/highlight-run/src/sdk/record.ts +++ b/sdk/highlight-run/src/sdk/record.ts @@ -79,7 +79,7 @@ import { getDefaultDataURLOptions } from '../client/utils/utils' import { type HighlightClientRequestWorker } from '../client/workers/highlight-client-worker' import { payloadToBase64 } from '../client/utils/payload' import HighlightClientWorker from '../client/workers/highlight-client-worker?worker&inline' -import { MessageType, PropertyType, StopReason } from '../client/workers/types' +import { MessageType, PropertyType } from '../client/workers/types' import { IntegrationClient } from '../integrations' import { Record } from '../api/record' import { internalLog } from './util' @@ -184,20 +184,18 @@ export class RecordSDK implements Record { e.data.response.payload, ) } else if (e.data.response?.type === MessageType.Stop) { - const { reason } = e.data.response internalLog( 'worker.onmessage', 'warn', - `Stopping recording due to worker failure: ${reason}`, + `Stopping recording due to worker failure: ${e.data.response.reason}`, e.data.response, ) this.stop(false) - if (reason === StopReason.UnrecoverableError) { - // Nothing more will be uploaded during this page load, so detach the recorder - // and drop what it buffered instead of growing the buffer forever. - this._stopRecorder() - this.events = [] - } + // The worker only asks us to stop when it cannot upload what we record, and + // `_save` no longer runs once we are not recording, so detach the recorder and + // drop its buffer instead of filling memory for the rest of this page load. + this._stopRecorder() + this.events = [] } } From 512d816093d059569f90c519f6687c49870fe403 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 15:08:38 -0700 Subject: [PATCH 3/6] fix(highlight.run): clear the upload timeout count on initialize Initialize resets the rest of the failure state so a fresh recording attempt can upload again, but the timeout count survived it. A session that initialized without a reset after a timeout stop therefore started at the limit, and its first slow upload stopped recording on the spot. Co-authored-by: Cursor --- sdk/highlight-run/src/client/workers/highlight-client-worker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts index 154e038b2b..d69c18a51c 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts @@ -443,6 +443,7 @@ function stringifyProperties( // us stop no longer applies. hasStoppedRecording = false numberOfFailedRequests = 0 + numberOfFailedPushPayloads = 0 graphqlSDK = getSdk( new GraphQLClient(backend, { headers: {}, From 60e3d5cb7186d7e85d54eeeb7838f4e2eb36abcb Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 15:24:41 -0700 Subject: [PATCH 4/6] fix(highlight.run): stop recording once per session, and quietly Requests already in flight when recording stops keep failing and arrive after the stop, and a slow upload can time out and then be rejected as well, so the worker could ask the client to tear down several times over. Only the first request is served now. Each stop also emitted its reason as a timeline event, which the client received after it had already stopped recording. That event could never be captured, so it sat in `addCustomEvent`'s waiting path and polled every 500ms for the rest of the page load. The reason already travels on the stop message and both clients log it, so the event is gone. Co-authored-by: Cursor --- .../highlight-client-worker-errors.test.ts | 22 +++++++++++++++++++ .../client/workers/highlight-client-worker.ts | 15 ++++++------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts index 374d01f801..a985f98c98 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker-errors.test.ts @@ -124,6 +124,28 @@ describe('highlight-client-worker error handling', () => { expect((await status()).initialized).toBe(false) }) + it('does not report the stop as a timeline event', async () => { + graph.error = clientError(403) + + worker.postMessage(initializeMessage()) + worker.postMessage(identifyMessage('refused-user')) + + await vi.waitFor(() => { + expect( + responsesOfType(MessageType.Stop), + ).toHaveLength(1) + }) + + // The client is NotRecording by the time it handles this, so a Track event could never be + // captured; it would only leave `addCustomEvent` polling every 500ms for the rest of the + // page load. + expect( + responsesOfType(MessageType.CustomEvent).map( + (e) => e.tag, + ), + ).not.toContain('Track') + }) + it('drops later messages after a permanent rejection', async () => { graph.error = clientError(403) diff --git a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts index d69c18a51c..2384899fe1 100644 --- a/sdk/highlight-run/src/client/workers/highlight-client-worker.ts +++ b/sdk/highlight-run/src/client/workers/highlight-client-worker.ts @@ -140,6 +140,9 @@ function stringifyProperties( * Asks the client to stop recording and stops accepting work until it initializes again. * Everything buffered here is dropped: the client stops handing us events, so a queue kept * across a stop would only hold memory for data that will never be uploaded. + * + * Only the first caller is served. Requests already in flight when we stop keep failing and + * arrive here afterwards, and the client has nothing left to tear down by then. */ const stopRecording = ( reason: StopReason, @@ -148,6 +151,10 @@ function stringifyProperties( 'requestStart' | 'asyncEventsResponse' >, ) => { + if (hasStoppedRecording) { + return + } + hasStoppedRecording = true pendingMessages.length = 0 metricsPayload.length = 0 @@ -159,14 +166,6 @@ function stringifyProperties( ...details, }, }) - - processPropertiesMessage({ - type: MessageType.Properties, - propertiesObject: { - stopReason: reason, - }, - propertyType: { type: 'track' }, - }) } /** From a2d6f7b41445f04dff9f8a3457f3276dab6e7870 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 19:28:04 -0700 Subject: [PATCH 5/6] fix(highlight.run): keep telemetry running when replay stops The legacy client stopped recording by way of `stopRecording`, which also shuts OTel down, so a worker stop took tracing and metrics with it. That was already wrong for upload timeouts and this branch made it reachable whenever the backend refuses replay data, disabling browser telemetry for the rest of the page load over a failure that says nothing about it. Worker stops now end capture through `_stopCapture`, which leaves the providers alone. `stopRecording` still shuts them down for a manual stop or a reset, where the whole SDK is going away or about to reinitialize. Co-authored-by: Cursor --- .../src/__tests__/client-worker-stop.test.ts | 48 +++++++++++++++++++ sdk/highlight-run/src/client/index.tsx | 23 ++++++--- 2 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 sdk/highlight-run/src/__tests__/client-worker-stop.test.ts diff --git a/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts b/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts new file mode 100644 index 0000000000..94e8fb2b30 --- /dev/null +++ b/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts @@ -0,0 +1,48 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { Highlight } from '../client' +import { MessageType, StopReason } from '../client/workers/types' + +const shutdown = vi.fn() +vi.mock('../client/otel', async (importOriginal) => ({ + ...(await importOriginal()), + shutdown: () => shutdown(), +})) + +describe('Highlight worker stop handling', () => { + let highlight: Highlight + + beforeEach(() => { + vi.useFakeTimers() + shutdown.mockClear() + highlight = new Highlight({ + organizationID: '1', + sessionSecureID: 'seed', + backendUrl: 'https://pub.observability.app.launchdarkly.com', + }) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('keeps telemetry running when the worker stops replay', () => { + highlight._worker.onmessage({ + data: { + response: { + type: MessageType.Stop, + reason: StopReason.UnrecoverableError, + }, + }, + } as MessageEvent) + + expect(shutdown).not.toHaveBeenCalled() + expect(highlight.state).toBe('NotRecording') + }) + + it('shuts telemetry down when the SDK itself stops', () => { + highlight.stopRecording(true) + + expect(shutdown).toHaveBeenCalledTimes(1) + expect(highlight.state).toBe('NotRecording') + }) +}) diff --git a/sdk/highlight-run/src/client/index.tsx b/sdk/highlight-run/src/client/index.tsx index 3f7ccd7e8c..95d9f3b4d0 100644 --- a/sdk/highlight-run/src/client/index.tsx +++ b/sdk/highlight-run/src/client/index.tsx @@ -301,11 +301,10 @@ export class Highlight { `Stopping recording due to worker failure: ${e.data.response.reason}`, e.data.response, ) - this.stopRecording(false) - // The worker only asks us to stop when it cannot upload what we record, and - // `_save` no longer runs once we are not recording, so detach the recorder and - // drop its buffer instead of filling memory for the rest of this page load. - this._stopRecorder() + // Replay is what failed, so tracing and metrics keep running. `_save` no longer + // runs once we are not recording, so detach the recorder and drop its buffer + // instead of filling memory for the rest of this page load. + this._stopCapture(true) this.events = [] } } @@ -1445,14 +1444,24 @@ SessionSecureID: ${this.sessionData.sessionSecureID}`, 'H.stop() was called which stops Highlight from recording.', ) } + this._stopCapture(manual) + void shutdown() + } + + /** + * Stops session replay and leaves telemetry alone. `stopRecording` shuts OTel down as well, + * which is right when the whole SDK is stopping but not when only replay uploads have failed. + * + * @param detachRecorder also release rrweb's observers. + */ + _stopCapture(detachRecorder?: boolean) { this.state = 'NotRecording' - if (manual) { + if (detachRecorder) { this._stopRecorder() } // stop all other event listeners, to be restarted on initialize() this.listeners.forEach((stop) => stop()) this.listeners = [] - void shutdown() } /** From 485eea4699b7848774bb9aea5967c011657864ff Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 19:44:31 -0700 Subject: [PATCH 6/6] fix(highlight.run): keep tab visibility from undoing a worker stop The page visibility listener is attached for the page lifetime on purpose, so after the worker gave up it still ran and, with disableBackgroundRecording, started a session the worker had already refused. Co-authored-by: Cursor --- .../src/__tests__/client-worker-stop.test.ts | 2 ++ .../src/__tests__/record-worker-stop.test.ts | 26 +++++++++++++++++-- sdk/highlight-run/src/client/index.tsx | 8 +++++- sdk/highlight-run/src/sdk/record.ts | 7 +++++ 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts b/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts index 94e8fb2b30..dc22e9054f 100644 --- a/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts +++ b/sdk/highlight-run/src/__tests__/client-worker-stop.test.ts @@ -37,6 +37,8 @@ describe('Highlight worker stop handling', () => { expect(shutdown).not.toHaveBeenCalled() expect(highlight.state).toBe('NotRecording') + // Keeps the page visibility listener, which outlives a stop, from restarting us. + expect(highlight.manualStopped).toBe(true) }) it('shuts telemetry down when the SDK itself stops', () => { diff --git a/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts b/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts index d0b5a56e37..a76f903ffa 100644 --- a/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts +++ b/sdk/highlight-run/src/__tests__/record-worker-stop.test.ts @@ -3,9 +3,10 @@ import { RecordSDK } from '../sdk/record' import { MessageType, StopReason } from '../client/workers/types' const recordStop = vi.fn() +const recordSpy = vi.fn(() => recordStop) vi.mock('@highlight-run/rrweb', () => ({ addCustomEvent: vi.fn(), - record: () => recordStop, + record: () => recordSpy(), })) vi.mock('../client/graph/generated/operations', () => ({ @@ -26,10 +27,13 @@ vi.mock('../client/workers/highlight-client-worker?worker&inline', () => ({ }, })) -async function startedSDK(): Promise { +async function startedSDK( + options: Partial[0]> = {}, +): Promise { const sdk = new RecordSDK({ organizationID: '1', sessionSecureID: 'seed', + ...options, }) await sdk.start() return sdk @@ -44,6 +48,7 @@ describe('RecordSDK worker stop handling', () => { beforeEach(() => { vi.useFakeTimers() recordStop.mockClear() + recordSpy.mockClear() }) afterEach(() => { @@ -68,6 +73,23 @@ describe('RecordSDK worker stop handling', () => { }, ) + // The page visibility listener stays attached once recording has ever started, so it is the + // one thing that can restart us after the worker has given up. + it('stays stopped when the tab becomes visible again', async () => { + const sdk = await startedSDK({ disableBackgroundRecording: true }) + + postToSDK(sdk, { + type: MessageType.Stop, + reason: StopReason.UnrecoverableError, + }) + recordSpy.mockClear() + sdk._lastVisibilityChangeTime = 0 // clear the debounce frozen fake timers impose + await sdk._visibilityHandler(false) + + expect(recordSpy).not.toHaveBeenCalled() + expect(sdk.getRecordingState()).toBe('NotRecording') + }) + it('keeps recording while the worker only reports uploads', async () => { const sdk = await startedSDK() sdk.events.push({ type: 3, data: {}, timestamp: 1 } as any) diff --git a/sdk/highlight-run/src/client/index.tsx b/sdk/highlight-run/src/client/index.tsx index 95d9f3b4d0..039ac4de5c 100644 --- a/sdk/highlight-run/src/client/index.tsx +++ b/sdk/highlight-run/src/client/index.tsx @@ -201,6 +201,10 @@ export class Highlight { events!: eventWithTime[] sessionData!: SessionData ready!: boolean + /** + * Recording must not resume on its own: either the app stopped us, or the worker gave up on + * uploading. Starting again explicitly clears it. + */ manualStopped!: boolean state!: 'NotRecording' | 'Recording' logger!: Logger @@ -303,8 +307,10 @@ export class Highlight { ) // Replay is what failed, so tracing and metrics keep running. `_save` no longer // runs once we are not recording, so detach the recorder and drop its buffer - // instead of filling memory for the rest of this page load. + // instead of filling memory for the rest of this page load. The visibility + // listener outlives a stop by design, so mark the stop as one it must not undo. this._stopCapture(true) + this.manualStopped = true this.events = [] } } diff --git a/sdk/highlight-run/src/sdk/record.ts b/sdk/highlight-run/src/sdk/record.ts index 4587b20fdd..be723f051e 100644 --- a/sdk/highlight-run/src/sdk/record.ts +++ b/sdk/highlight-run/src/sdk/record.ts @@ -116,6 +116,10 @@ export class RecordSDK implements Record { events!: eventWithTime[] sessionData!: SessionData ready!: boolean + /** + * Recording must not resume on its own: either the app stopped us, or the worker gave up on + * uploading. Starting again explicitly clears it. + */ manualStopped!: boolean state!: 'NotRecording' | 'Recording' logger!: Logger @@ -194,6 +198,9 @@ export class RecordSDK implements Record { // The worker only asks us to stop when it cannot upload what we record, and // `_save` no longer runs once we are not recording, so detach the recorder and // drop its buffer instead of filling memory for the rest of this page load. + // The visibility listener outlives a stop by design, so mark the stop as one it + // must not undo. + this.manualStopped = true this._stopRecorder() this.events = [] }