feat(highlight.run): stop session replay when uploads cannot succeed

sdk/highlight-run/src/__tests__/client-worker-stop.test.ts

@@ -0,0 +1,50 @@
+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<typeof import('../client/otel')>()),
+	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<any>)
+
+		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', () => {
+		highlight.stopRecording(true)
+
+		expect(shutdown).toHaveBeenCalledTimes(1)
+		expect(highlight.state).toBe('NotRecording')
+	})
+})

sdk/highlight-run/src/__tests__/record-worker-stop.test.ts

@@ -0,0 +1,108 @@
+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()
+const recordSpy = vi.fn(() => recordStop)
+vi.mock('@highlight-run/rrweb', () => ({
+	addCustomEvent: vi.fn(),
+	record: () => recordSpy(),
+}))
+
+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(
+	options: Partial<ConstructorParameters<typeof RecordSDK>[0]> = {},
+): Promise<RecordSDK> {
+	const sdk = new RecordSDK({
+		organizationID: '1',
+		sessionSecureID: 'seed',
+		...options,
+	})
+	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<any>)
+}
+
+describe('RecordSDK worker stop handling', () => {
+	beforeEach(() => {
+		vi.useFakeTimers()
+		recordStop.mockClear()
+		recordSpy.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')
+		},
+	)
+
+	// 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)
+
+		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')
+	})
+})

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
@@ -298,10 +302,16 @@ export class Highlight {
 				)
 			} else if (e.data.response?.type === MessageType.Stop) {
 				HighlightWarning(
-					'Stopping recording due to worker failure',
+					`Stopping recording due to worker failure: ${e.data.response.reason}`,
 					e.data.response,
 				)
-				this.stopRecording(false)
+				// 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. 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 = []
 			}
 		}
 
@@ -1440,16 +1450,36 @@ 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'
-		// stop rrweb recording mutation observers
-		if (manual && this._recordStop) {
-			this._recordStop()
-			this._recordStop = undefined
+		if (detachRecorder) {
+			this._stopRecorder()
 		}
 		// stop all other event listeners, to be restarted on initialize()
 		this.listeners.forEach((stop) => stop())
 		this.listeners = []
-		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() {

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)
+	})
+})