Skip to content

fix: lower Refresh All Orgs concurrency and improve OPFS worker error… - #1984

Merged
paustint merged 1 commit into
mainfrom
fix/refresh-all-orgs-concurrency
Aug 23, 2026
Merged

fix: lower Refresh All Orgs concurrency and improve OPFS worker error…#1984
paustint merged 1 commit into
mainfrom
fix/refresh-all-orgs-concurrency

Conversation

@paustint

Copy link
Copy Markdown
Contributor

… 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.

Copilot AI lite review requested due to automatic review settings August 21, 2026 12:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR reduces the “Refresh All Orgs” fan-out to avoid bursty concurrent Salesforce health checks, and improves OPFS Data History worker crash diagnostics by capturing richer error context (filename/line/col and underlying error details) when ErrorEvent.message is blank.

Changes:

  • Lowered Refresh All Orgs concurrency from 4 to 2 to reduce request bursts and associated fast-fail errors.
  • Enhanced OPFS worker onerror logging and error construction to include more actionable crash details.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts Improves worker crash logging and error messages by capturing additional error context.
libs/features/org-groups/src/lib/RefreshAllOrgsButton.tsx Reduces health-check concurrency to mitigate request bursts during “Refresh All Orgs”.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libs/shared/ui-data-history/src/lib/file-store/opfs-file-store.ts Outdated
@paustint
paustint force-pushed the fix/refresh-all-orgs-concurrency branch from 82c8116 to 10f1d59 Compare August 23, 2026 00:36
… 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.
@paustint
paustint force-pushed the fix/refresh-all-orgs-concurrency branch from 10f1d59 to 7f0d237 Compare August 23, 2026 01:40
@paustint

Copy link
Copy Markdown
Contributor Author

Code review summary

Automated review (Claude Code). Fixes below are pushed as an amend to the PR head.

Verdict: small and low-risk, but neither half fully lands as written — the concurrency change is a client-side stop-gap for a server-side contention problem, and most of the new OPFS error detail was inert in production.

Fixed and pushed

  • opfs-file-store.ts — dead event.error branch. For errors propagated out of a worker, ErrorEvent.error is null; a module-load failure fires a plain Event with no message or filename at all. The exact case the comments cited still logged "unknown". Removed the dead branch, corrected the comment, and added explicit module-load detection that names the worker module.
  • opfs-file-store.ts — error detail never reached Sentry. errorStack/errorName went only to logger.warn, which is a no-op in production. Detail is now attached to the rejected Error and forwarded through reportDataHistoryFailureToTracker as Sentry extras.
  • Added opfs-file-store.spec.tsOpfsFileStore had no spec anywhere. Covers ErrorEvent with message+filename, plain Event with neither, detail attachment, and terminate/respawn after a crash. (11 files / 157 tests pass.)

Two deliberate deviations worth knowing:

  • The worker URL is reported via a module-specifier constant, not new URL('./…worker.ts', import.meta.url). Vite emits the worker chunk by pattern-matching the literal new Worker(new URL(...)) form — hoisting it breaks worker bundling and resolves against the parent chunk in production.
  • Detail is forwarded as extras rather than via cause. Sentry's linkedErrors integration only follows cause when it is an Error; since event.error is always null here, a plain object would be silently dropped. This is why the PR now also touches ui-core (useInitDataHistory.ts) — it widens the footprint to 2 libs, flagging in case that's unwanted.

Needs a decision

  • The 4→2 concurrency drop treats a symptom. The route has no server-side rate limit, the 400s are UserFacingErrors, and the likely root cause is a Prisma interactive transaction held across the Salesforce token exchange in refreshTokensWithLock. Moving the token exchange outside the transaction, or bounding the route per user server-side, would fix the actual contention. Left as-is.

Branch is 11 commits behind main.

@paustint
paustint merged commit 851a2f6 into main Aug 23, 2026
14 checks passed
@paustint
paustint deleted the fix/refresh-all-orgs-concurrency branch August 23, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants