From 7f0d237874bc849823df5f5bbcbf18ab4641b0fa Mon Sep 17 00:00:00 2001 From: Austin Turner Date: Fri, 21 Aug 2026 07:04:07 -0500 Subject: [PATCH] fix: lower Refresh All Orgs concurrency and improve OPFS worker error detail Yesterday's Refresh All Orgs release put one production org-groups page through 130+ concurrent health checks in a single burst; ~28% failed fast with a 400 that never reached Salesforce. Dropping the fan-out from 4 to 2 gives the org-resolution path more headroom per request. Also caught while triaging: the Data History OPFS worker's crash handler logged "unknown" whenever `ErrorEvent.message` came back blank. It now captures filename/line/col and the underlying Error's name/message/stack when the browser provides one, so the next crash is diagnosable instead of opaque. --- .../src/lib/RefreshAllOrgsButton.tsx | 2 +- .../__tests__/RefreshAllOrgsButton.spec.tsx | 13 ++- .../ui-core/src/app/useInitDataHistory.ts | 11 +- libs/shared/ui-data-history/src/index.ts | 2 + .../src/lib/__tests__/opfs-file-store.spec.ts | 104 ++++++++++++++++++ .../src/lib/failure-reporter.ts | 22 ++++ .../src/lib/file-store/opfs-file-store.ts | 32 +++++- 7 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 libs/shared/ui-data-history/src/lib/__tests__/opfs-file-store.spec.ts diff --git a/libs/features/org-groups/src/lib/RefreshAllOrgsButton.tsx b/libs/features/org-groups/src/lib/RefreshAllOrgsButton.tsx index 70768c82f..f8166fe27 100644 --- a/libs/features/org-groups/src/lib/RefreshAllOrgsButton.tsx +++ b/libs/features/org-groups/src/lib/RefreshAllOrgsButton.tsx @@ -12,7 +12,7 @@ import PQueue from 'p-queue'; import { useState } from 'react'; /** Each health check is a round trip to Salesforce, so fan out enough to stay quick without flooding the user's orgs */ -const REFRESH_CONCURRENCY = 4; +const REFRESH_CONCURRENCY = 2; interface RefreshAllOrgsButtonProps { className?: string; diff --git a/libs/features/org-groups/src/lib/__tests__/RefreshAllOrgsButton.spec.tsx b/libs/features/org-groups/src/lib/__tests__/RefreshAllOrgsButton.spec.tsx index 359c64522..4ff8056e2 100644 --- a/libs/features/org-groups/src/lib/__tests__/RefreshAllOrgsButton.spec.tsx +++ b/libs/features/org-groups/src/lib/__tests__/RefreshAllOrgsButton.spec.tsx @@ -96,8 +96,17 @@ describe('RefreshAllOrgsButton', () => { expect(button.hasAttribute('disabled')).toBe(true); expect(button.textContent).toContain('Refreshing 0 of 3'); - - await act(async () => pendingHealthChecks.forEach((resolve) => resolve())); + // The queue caps in-flight checks, so the remaining orgs stay queued until an earlier one settles + expect(checkOrgHealth).toHaveBeenCalledTimes(2); + + // Resolve in waves - each wave lets the queue start the orgs that were still waiting behind it + await act(async () => { + while (pendingHealthChecks.length > 0) { + pendingHealthChecks.splice(0).forEach((resolve) => resolve()); + await new Promise((resolve) => setTimeout(resolve, 0)); + } + }); + expect(checkOrgHealth).toHaveBeenCalledTimes(3); await waitFor(() => expect(button.hasAttribute('disabled')).toBe(false)); }); diff --git a/libs/shared/ui-core/src/app/useInitDataHistory.ts b/libs/shared/ui-core/src/app/useInitDataHistory.ts index cc42635b4..0d3c328ca 100644 --- a/libs/shared/ui-core/src/app/useInitDataHistory.ts +++ b/libs/shared/ui-core/src/app/useInitDataHistory.ts @@ -1,7 +1,12 @@ import { tracker } from '@jetstream/shared/ui-utils'; import { getErrorMessage } from '@jetstream/shared/utils'; import { dataHistoryCaptureEnabledState, dataHistoryLimitsState } from '@jetstream/ui/app-state'; -import { DataHistoryFailureInfo, initDataHistory, setDataHistoryFailureReporter } from '@jetstream/ui/data-history'; +import { + DataHistoryFailureInfo, + getDataHistoryErrorDetails, + initDataHistory, + setDataHistoryFailureReporter, +} from '@jetstream/ui/data-history'; import { useSetAtom } from 'jotai'; import { useCallback } from 'react'; @@ -16,6 +21,10 @@ import { useCallback } from 'react'; */ function reportDataHistoryFailureToTracker({ operation, error, entryKey, backend }: DataHistoryFailureInfo): void { tracker.error(`[DATA_HISTORY] ${operation} failed`, error, { + // Spread first so the operation context below always wins on a key collision. Detail the error + // carries (e.g. which worker crashed and where) only reaches the tracker this way — the logger is + // a no-op in production. + ...getDataHistoryErrorDetails(error), entryKey, backend, // Included explicitly: FSA and OPFS reject with DOMException, which the tracker cannot unwrap diff --git a/libs/shared/ui-data-history/src/index.ts b/libs/shared/ui-data-history/src/index.ts index bded45a80..4c35951aa 100644 --- a/libs/shared/ui-data-history/src/index.ts +++ b/libs/shared/ui-data-history/src/index.ts @@ -11,7 +11,9 @@ export * from './lib/data-history.service'; // Lets the host app point swallowed capture failures at its error tracker (this lib cannot import // one — see `failure-reporter.ts`) export { + getDataHistoryErrorDetails, setDataHistoryFailureReporter, + type DataHistoryErrorDetails, type DataHistoryFailureInfo, type DataHistoryFailureOperation, type DataHistoryFailureReporter, diff --git a/libs/shared/ui-data-history/src/lib/__tests__/opfs-file-store.spec.ts b/libs/shared/ui-data-history/src/lib/__tests__/opfs-file-store.spec.ts new file mode 100644 index 000000000..0f24b9ef0 --- /dev/null +++ b/libs/shared/ui-data-history/src/lib/__tests__/opfs-file-store.spec.ts @@ -0,0 +1,104 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getDataHistoryErrorDetails } from '../failure-reporter'; +import { OpfsFileStore } from '../file-store/opfs-file-store'; + +/** + * Stands in for the real storage worker so the store can be driven through its `onerror` path without + * OPFS (which jsdom does not implement) or a real module load. + */ +class StubWorker { + static instances: StubWorker[] = []; + + onmessage: ((event: MessageEvent) => void) | null = null; + onerror: ((event: ErrorEvent) => void) | null = null; + postMessage = vi.fn(); + terminate = vi.fn(); + + constructor() { + StubWorker.instances.push(this); + } +} + +const originalWorker = globalThis.Worker; + +/** + * Starts a request so there is a pending promise for the crash to reject, and hands back the worker + * that request spawned. + */ +function startPendingRequest() { + const store = new OpfsFileStore('u-0123456789abcdef'); + const pending = store.init(); + const worker = StubWorker.instances.at(-1); + if (!worker) { + throw new Error('Expected the store to spawn a worker'); + } + return { store, pending, worker }; +} + +describe('OpfsFileStore worker error handling', () => { + beforeEach(() => { + StubWorker.instances = []; + globalThis.Worker = StubWorker as unknown as typeof Worker; + }); + + afterEach(() => { + globalThis.Worker = originalWorker; + }); + + it('reports the message and source location of an exception thrown inside the worker', async () => { + const { pending, worker } = startPendingRequest(); + + worker.onerror?.( + new ErrorEvent('error', { + message: 'Uncaught TypeError: handle.createSyncAccessHandle is not a function', + filename: 'https://getjetstream.app/assets/history-storage.worker-abc123.js', + lineno: 42, + colno: 17, + }), + ); + + await expect(pending).rejects.toThrow( + 'Data history storage worker error: Uncaught TypeError: handle.createSyncAccessHandle is not a function ' + + '(https://getjetstream.app/assets/history-storage.worker-abc123.js:42:17)', + ); + }); + + // A module that fails to load fires a plain `error` Event with no message/filename/lineno/colno, + // which is exactly the crash shape that used to report only "unknown". + it('names the worker module when the error event carries no detail at all', async () => { + const { pending, worker } = startPendingRequest(); + + worker.onerror?.(new Event('error') as ErrorEvent); + + await expect(pending).rejects.toThrow('Data history storage worker error: worker module failed to load (./history-storage.worker.ts)'); + }); + + it('attaches the crash detail to the rejected error so it reaches the error tracker', async () => { + const { pending, worker } = startPendingRequest(); + + worker.onerror?.(new ErrorEvent('error', { message: 'boom', filename: 'worker.js', lineno: 7, colno: 3 })); + + const error = await pending.catch((ex: unknown) => ex); + expect(getDataHistoryErrorDetails(error)).toEqual({ + message: 'boom', + filename: 'worker.js', + lineno: 7, + colno: 3, + workerModule: undefined, + }); + }); + + it('terminates the crashed worker and spawns a fresh one for the next request', async () => { + const { store, pending, worker } = startPendingRequest(); + + worker.onerror?.(new Event('error') as ErrorEvent); + await expect(pending).rejects.toThrow(); + expect(worker.terminate).toHaveBeenCalledTimes(1); + + const retry = store.init(); + expect(StubWorker.instances).toHaveLength(2); + + StubWorker.instances[1].onerror?.(new Event('error') as ErrorEvent); + await expect(retry).rejects.toThrow(); + }); +}); diff --git a/libs/shared/ui-data-history/src/lib/failure-reporter.ts b/libs/shared/ui-data-history/src/lib/failure-reporter.ts index a9d9a33c1..12b11699e 100644 --- a/libs/shared/ui-data-history/src/lib/failure-reporter.ts +++ b/libs/shared/ui-data-history/src/lib/failure-reporter.ts @@ -15,6 +15,28 @@ export interface DataHistoryFailureInfo { export type DataHistoryFailureReporter = (info: DataHistoryFailureInfo) => void; +/** + * Diagnostic context attached to an Error so it survives the trip to the error tracker. `logger.warn` + * is a no-op unless the user turns logging on, and the reported Error is the only thing that reaches + * the reporter — so anything a failure needs to be diagnosable has to ride on the Error itself. The + * host app forwards these alongside the operation context (see `useInitDataHistory`). + */ +export type DataHistoryErrorDetails = Record; + +const ERROR_DETAILS_KEY = 'dataHistoryErrorDetails'; + +export function withDataHistoryErrorDetails(error: TError, details: DataHistoryErrorDetails): TError { + return Object.assign(error, { [ERROR_DETAILS_KEY]: details }); +} + +export function getDataHistoryErrorDetails(error: unknown): DataHistoryErrorDetails | undefined { + if (typeof error !== 'object' || error === null) { + return undefined; + } + const details = (error as Record)[ERROR_DETAILS_KEY]; + return typeof details === 'object' && details !== null ? (details as DataHistoryErrorDetails) : undefined; +} + /** * Conditions where the environment said no rather than Jetstream being broken: the user revoked or * denied folder access, a picker ran outside a user gesture, or a stream was aborted because the diff --git a/libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts b/libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts index b598959ed..858b43256 100644 --- a/libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts +++ b/libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts @@ -1,7 +1,15 @@ import { logger } from '@jetstream/shared/client-logger'; +import { withDataHistoryErrorDetails } from '../failure-reporter'; import type { HistoryFileStore, HistoryFileStoreCapabilities, HistoryWriteStream } from './file-store.types'; import type { HistoryWorkerRequestBody, HistoryWorkerResponse, HistoryWorkerResultByOp } from './worker-messages'; +/** + * Used for error messages only. The bundler emits the worker chunk by pattern-matching the literal + * `new Worker(new URL(...))` call below, so the specifier cannot be shared with it — and the resolved + * href is not obtainable at runtime, since the built worker lives at a hashed path of its own. + */ +const WORKER_MODULE = './history-storage.worker.ts'; + /** * Default Data History file store: OPFS, with all I/O delegated to a dedicated worker * (`history-storage.worker.ts`) over a small promise-map RPC. The worker is spawned lazily on @@ -148,8 +156,28 @@ export class OpfsFileStore implements HistoryFileStore { this.maybeTerminate(); }; this.worker.onerror = (event) => { - logger.warn('[DATA_HISTORY][OPFS] Storage worker crashed, rejecting pending requests', event.message); - const error = new Error(`Data history storage worker error: ${event.message || 'unknown'}`); + // Neither crash shape hands us the thrown value: an uncaught exception inside a running worker + // arrives as an ErrorEvent whose `error` is always null (the value cannot cross the agent + // boundary), and a worker whose module never loads arrives as a plain `error` Event carrying no + // message, filename, lineno or colno at all. An empty message with no filename is therefore the + // only signal that we are looking at a failed load rather than a crash mid-run. + const moduleLoadFailed = !event.message && !event.filename; + const errorDetails = { + message: event.message || undefined, + filename: event.filename || undefined, + lineno: event.lineno || undefined, + colno: event.colno || undefined, + workerModule: moduleLoadFailed ? WORKER_MODULE : undefined, + }; + logger.warn('[DATA_HISTORY][OPFS] Storage worker crashed, rejecting pending requests', errorDetails); + const summary = moduleLoadFailed ? `worker module failed to load (${WORKER_MODULE})` : errorDetails.message || 'unknown'; + // lineno/colno are 0 (normalized to undefined above) for load failures, so only append what we actually have. + const location = errorDetails.filename + ? ` (${[errorDetails.filename, errorDetails.lineno, errorDetails.colno].filter(Boolean).join(':')})` + : ''; + // The detail rides on the Error, not the log line: `logger.warn` is a no-op in production, while + // this Error is what reaches the error tracker through `reportDataHistoryFailure`. + const error = withDataHistoryErrorDetails(new Error(`Data history storage worker error: ${summary}${location}`), errorDetails); const pending = Array.from(this.pendingRequests.values()); this.pendingRequests.clear(); pending.forEach(({ reject }) => reject(error));