diff --git a/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx b/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx index a8c4e7c4c58..9028399b6a1 100644 --- a/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx +++ b/src/renderer/src/lib/react-commit-cascade-observer.react185.test.tsx @@ -49,9 +49,12 @@ const useCascadeStore = create()( ) /** - * Layout effect, not passive: only the synchronous counter throws #185. A - * useEffect loop leaves `pendingLanes: 0` at commit time (measured) because - * passive effects flush after the callback, and React only console.errors it. + * Layout effect, not passive: only this shape leaves cascading lanes visible at + * commit time, so it is what pins the LANE half of the diagnostic. A passive + * (useEffect) loop also reads `pendingLanes: 0` here — but it throws #185 all + * the same, and believing otherwise is what cost the field its attribution; the + * re-entrancy half covers it, pinned in + * react-commit-cascade-passive-effect-blind-spot.react185.test.tsx. */ function RunawayLayoutEffectPane(): React.JSX.Element { const ticks = useCascadeStore((state) => state.ticks) @@ -107,6 +110,9 @@ describe('react commit cascade observer', () => { const payload = (cascadeCalls[0]?.[1] ?? {}) as Record expect(payload.commits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT) expect(payload.pendingLanes).toBeGreaterThan(0) + // Measured, not inferred: every counted commit held cascading lanes. + expect(payload.laneCommits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT) + expect(payload.evidence).toBe('lanes') expect(payload.storeWrites).toBeGreaterThan(0) // The middleware boundary is elided, so this is the code that called `set`. expect(String(payload.driverFrame)).toContain('bump') diff --git a/src/renderer/src/lib/react-commit-cascade-passive-effect-blind-spot.react185.test.tsx b/src/renderer/src/lib/react-commit-cascade-passive-effect-blind-spot.react185.test.tsx new file mode 100644 index 00000000000..6d638f3c6e1 --- /dev/null +++ b/src/renderer/src/lib/react-commit-cascade-passive-effect-blind-spot.react185.test.tsx @@ -0,0 +1,154 @@ +/** @vitest-environment happy-dom */ +/** + * Crash cluster: renderer React #185 on v1.4.199 (payloads 1789034235 `terminal.workbench`, + * 1789077899 `sidebar.worktrees`; both carry `attribution: unreliable`). + * + * Neither payload contains a `react_commit_cascade` breadcrumb, and neither contains + * `react_commit_cascade_uninstalled` — so the observer was installed, saw commits, and still + * named nothing. This test shows why: the cascade shape that actually throws #185 in this app + * is a store-subscription loop (an `useSyncExternalStore` snapshot that is not reference-stable, + * i.e. a zustand selector handing back a fresh array/object), and that shape reports + * `root.pendingLanes === 0` at every `onCommitFiberRoot` because `forceStoreRerender` runs from + * a passive effect that react-dom flushes AFTER the devtools commit callback. + * + * react-commit-cascade-telemetry.ts resets the cascade on `(pendingLanes & REACT_CASCADING_LANES) === 0`, + * so it ends the cascade on every one of those commits and never reaches its notice limit. + */ +import './react-devtools-commit-hook-shim' +import { Component, act, useSyncExternalStore, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { REACT_NESTED_UPDATE_LIMIT } from '../../../shared/react-update-depth-attribution' +import { + REACT_COMMIT_CASCADE_BREADCRUMB, + resetReactCommitCascadeTelemetryForTests +} from './react-commit-cascade-telemetry' +import { + installReactCommitCascadeObserver, + resetReactCommitCascadeObserverForTests +} from './react-commit-cascade-observer' +import type { ReactDevtoolsCommitHook } from './react-devtools-commit-hook-shim' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +const recordBreadcrumb = vi.fn() +vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (name: string, data?: unknown) => recordBreadcrumb(name, data) +})) + +const listeners = new Set<() => void>() +const subscribe = (listener: () => void): (() => void) => { + listeners.add(listener) + return () => listeners.delete(listener) +} + +/** + * The production shape: a selector whose result is value-equal but never reference-equal. + * Zustand v5's `useStore` is a bare `useSyncExternalStore(api.subscribe, () => selector(api.getState()))`, + * so this is exactly what one uncached `.filter()` / `?? []` inside a `useAppStore` selector produces. + */ +const unstableSnapshot = (): readonly string[] => [] + +function UnstableStoreSnapshotPane(): React.JSX.Element { + const rows = useSyncExternalStore(subscribe, unstableSnapshot, unstableSnapshot) + return
{rows.length}
+} + +class CapturingBoundary extends Component<{ children: ReactNode; onError: (e: unknown) => void }> { + state = { failed: false } + static getDerivedStateFromError(): { failed: boolean } { + return { failed: true } + } + componentDidCatch(error: unknown): void { + this.props.onError(error) + } + render(): ReactNode { + return this.state.failed ? null : this.props.children + } +} + +const commitHook = (globalThis as { __REACT_DEVTOOLS_GLOBAL_HOOK__?: ReactDevtoolsCommitHook }) + .__REACT_DEVTOOLS_GLOBAL_HOOK__ + +let host: HTMLDivElement +let root: Root +let pendingLanesPerCommit: number[] +let boundaryErrors: string[] + +beforeEach(() => { + recordBreadcrumb.mockReset() + resetReactCommitCascadeTelemetryForTests() + resetReactCommitCascadeObserverForTests() + // Why cleared, not replaced: react-dom captured this hook object at its own module evaluation. + if (commitHook) { + commitHook.onCommitFiberRoot = undefined + } + installReactCommitCascadeObserver() + pendingLanesPerCommit = [] + boundaryErrors = [] + const observed = commitHook?.onCommitFiberRoot + if (commitHook) { + commitHook.onCommitFiberRoot = (rendererId, fiberRoot, priorityLevel, didError) => { + pendingLanesPerCommit.push( + (fiberRoot as { pendingLanes?: number } | null)?.pendingLanes ?? -1 + ) + observed?.call(commitHook, rendererId, fiberRoot, priorityLevel, didError) + } + } + // React's own dev warning for this shape is noise here; the assertions read the errors it throws. + vi.spyOn(console, 'error').mockImplementation(() => {}) + host = document.createElement('div') + document.body.appendChild(host) + root = createRoot(host, { + onCaughtError: (error) => + boundaryErrors.push(error instanceof Error ? error.message : String(error)), + onUncaughtError: (error) => + boundaryErrors.push(error instanceof Error ? error.message : String(error)) + }) +}) + +afterEach(() => { + act(() => root.unmount()) + host.remove() + listeners.clear() + vi.restoreAllMocks() +}) + +function renderCascade(): void { + act(() => { + root.render( + boundaryErrors.push(String(error))}> + + + ) + }) +} + +describe('React #185 driven by an unstable store snapshot', () => { + it('throws the production #185 message', () => { + renderCascade() + + expect( + boundaryErrors.some((message) => message.includes('Maximum update depth exceeded')) + ).toBe(true) + // React bails just past its nested-update limit, so the loop is short and leaves no trace. + expect(pendingLanesPerCommit.length).toBeGreaterThan(REACT_NESTED_UPDATE_LIMIT) + }) + + it('reports pendingLanes 0 on every commit, so the cascade observer never arms', () => { + renderCascade() + + expect(pendingLanesPerCommit.every((lanes) => lanes === 0)).toBe(true) + }) + + // FAILS on main: this is the diagnostic gap that leaves both production payloads + // with a bystander boundary_id and nothing naming the driver. + it('breadcrumbs the cascade before React throws', () => { + renderCascade() + + const cascadeCrumbs = recordBreadcrumb.mock.calls.filter( + ([name]) => name === REACT_COMMIT_CASCADE_BREADCRUMB + ) + expect(cascadeCrumbs.length).toBe(1) + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts b/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts index 89e0456c2ce..00a983fe9b8 100644 --- a/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts +++ b/src/renderer/src/lib/react-commit-cascade-telemetry.test.ts @@ -9,6 +9,7 @@ import { REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS, REACT_COMMIT_CASCADE_NOTICE_LIMIT, createReactCommitCascadeState, + observeReactCommit, recordReactCommit, resetReactCommitCascadeTelemetryForTests, type ReactCommitCascadeState @@ -337,3 +338,148 @@ describe('repeated arm and end cycles', () => { expect(state.cascadeRoot).toBeNull() }) }) + +/** + * Why the payload carries both: the crumb is the only artefact triage sees, and + * a lane that was inferred from commit timing must never be readable as one + * sampled from `root.pendingLanes`. + */ +describe('cascade evidence', () => { + it('reports the sampled lanes and marks a lane-confirmed run', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT }) + + expect(cascadePayload()).toMatchObject({ + pendingLanes: SYNC_LANE, + laneCommits: REACT_COMMIT_CASCADE_NOTICE_LIMIT, + evidence: 'lanes' + }) + }) + + it('never synthesizes lanes for a run counted by re-entrancy alone', () => { + const state = createReactCommitCascadeState() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT; commit += 1) { + recordReactCommit({ + state, + root: ROOT, + pendingLanes: 0, + reentrant: true, + readNowMs: () => 1_000 + }) + } + + expect(cascadePayload()).toMatchObject({ + pendingLanes: 0, + laneCommits: 0, + evidence: 'reentrant' + }) + }) + + it('marks a run mixed when only some commits held cascading lanes', () => { + const state = createReactCommitCascadeState() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT; commit += 1) { + recordReactCommit({ + state, + root: ROOT, + pendingLanes: commit % 2 === 0 ? SYNC_LANE : 0, + reentrant: true, + readNowMs: () => 1_000 + }) + } + + const payload = cascadePayload() + expect(payload.evidence).toBe('mixed') + expect(payload.laneCommits).toBe(REACT_COMMIT_CASCADE_NOTICE_LIMIT / 2) + }) + + it('ends a run on a commit that is neither lane-cascading nor re-entrant', () => { + const state = createReactCommitCascadeState() + driveCommits({ state, count: REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1 }) + + recordReactCommit({ state, root: ROOT, pendingLanes: 0, readNowMs: () => 1_000 }) + + expect(state.commits).toBe(0) + expect(state.laneCommits).toBe(0) + expect(state.cascadeRoot).toBeNull() + }) +}) + +/** The production entry point owns the re-entrancy term; the seam only receives it. */ +describe('observeReactCommit re-entrancy', () => { + const QUIET_ROOT = { pendingLanes: 0 } + + // The first commit of a tick has nothing to be re-entrant against, so the run + // is one shorter than the burst. + it('counts a same-tick burst of zero-lane commits', () => { + for (let commit = 0; commit <= REACT_COMMIT_CASCADE_NOTICE_LIMIT; commit += 1) { + observeReactCommit(QUIET_ROOT, 0) + } + + expect(recordBreadcrumb).toHaveBeenCalledTimes(1) + expect(cascadePayload().evidence).toBe('reentrant') + }) + + it('starts a new run after a microtask checkpoint', async () => { + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + observeReactCommit(QUIET_ROOT, 0) + } + await Promise.resolve() + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + observeReactCommit(QUIET_ROOT, 0) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // Why this and not a state assertion: the shared state is module-private, so + // the released root slot is only observable as a run that restarts from zero. + it('drops a run with no lane evidence at the span checkpoint', async () => { + for (let commit = 0; commit <= REACT_COMMIT_CASCADE_ARM_COMMITS; commit += 1) { + observeReactCommit(QUIET_ROOT, 0) + } + await Promise.resolve() + + // Enough to finish the dropped run, and one short of a fresh one. + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - 1; commit += 1) { + observeReactCommit(QUIET_ROOT, REACT_CASCADING_LANES) + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) + + // The other half of the same rule: a lane cascade is measured, legitimately + // spans ticks, and must survive the checkpoint that drops an inferred run. + it('keeps a lane-measured run across a span checkpoint', async () => { + for (let commit = 0; commit < REACT_COMMIT_CASCADE_ARM_COMMITS; commit += 1) { + observeReactCommit(QUIET_ROOT, REACT_CASCADING_LANES) + } + await Promise.resolve() + for ( + let commit = 0; + commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT - REACT_COMMIT_CASCADE_ARM_COMMITS; + commit += 1 + ) { + observeReactCommit(QUIET_ROOT, REACT_CASCADING_LANES) + } + + expect(recordBreadcrumb).toHaveBeenCalledTimes(1) + expect(cascadePayload().evidence).toBe('lanes') + }) + + // Why fail closed: with no checkpoint to clear the flag, the first commit would + // latch it and every later commit in the renderer's life would read re-entrant. + it('reports nothing re-entrant when queueMicrotask is unavailable', () => { + const host = globalThis as { queueMicrotask?: typeof queueMicrotask } + const original = host.queueMicrotask + delete host.queueMicrotask + try { + for (let commit = 0; commit < REACT_COMMIT_CASCADE_NOTICE_LIMIT * 2; commit += 1) { + observeReactCommit(QUIET_ROOT, 0) + } + } finally { + host.queueMicrotask = original + } + + expect(recordBreadcrumb).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/react-commit-cascade-telemetry.ts b/src/renderer/src/lib/react-commit-cascade-telemetry.ts index 73051d21599..1de37bf05f6 100644 --- a/src/renderer/src/lib/react-commit-cascade-telemetry.ts +++ b/src/renderer/src/lib/react-commit-cascade-telemetry.ts @@ -2,20 +2,34 @@ * Counts consecutive same-root commits that keep scheduling synchronous work, * and breadcrumbs the cascade before React #185 throws. * - * This mirrors React's own accounting rather than approximating it with a time - * window: react-dom resets its nested-update counter the moment a commit leaves - * no sync lanes pending, and increments it otherwise. Reading `root.pendingLanes` - * at commit time reproduces that reset exactly. + * Two independent signals feed the count, and the crumb names which one fired: * - * Known over-count: React also requires the committed lanes to intersect its own - * mask, which we cannot read from the devtools callback. Dropping that term can - * only make us count a commit React would have skipped, so we fire early, never - * late — the safe direction for a diagnostic that must beat the throw. + * 1. Lanes — a measurement. react-dom resets its nested-update counter the + * moment a commit leaves no cascading lanes pending and increments it + * otherwise, so sampling `root.pendingLanes` at commit time reproduces that + * reset. Known over-count: React also masks the COMMITTED lanes + * (`lanes & 261930`), which the devtools callback does not hand us. + * 2. Re-entrancy — an inference. Lane sampling alone is blind to the loop class + * that actually reached the field: react-dom calls onCommitFiberRoot BEFORE + * `0 !== (pendingEffectsLanes & 3) && flushPendingEffects()` and reads + * `root.pendingLanes` for nestedUpdateCount only AFTER it. An external store + * re-render is scheduled at SyncLane (`forceStoreRerender` -> lane 2), so a + * store-driven useEffect loop is flushed inline, counted by React, and throws + * #185 — while every sample here reads 0. A commit arriving before the stack + * unwinds to a microtask checkpoint is that loop. * - * Out of scope by construction: a passive-effect (useEffect) loop reports - * pendingLanes 0 here, because passive effects flush after the commit callback. - * That is correct — React tracks those in nestedPassiveUpdateCount, which only - * console.errors in development and never throws #185. + * Re-entrancy drops React's lane term entirely rather than approximating it, so + * it also counts bursts React scores as zero nested updates: 41 same-root + * commits inside one uninterrupted synchronous span (the first has nothing to be + * re-entrant against) reach the notice limit with no cascade present. Nothing in + * this app renders one root that many times without yielding — every production + * `flushSync` is one-shot, and the per-decoration roots in useDiffCommentDecorator + * are a distinct root each, which resets the run — and a run carrying no lane + * evidence is discarded at the span's microtask checkpoint, so it can never + * accumulate across spans. That is why the sampled lanes are reported RAW and + * never synthesized, and why `evidence` ships beside them — `lanes` was measured + * at commit time, `reentrant` (with `laneCommits: 0`) was inferred from commit + * timing alone, and triage must not read one as the other. */ import { compactBreadcrumbData } from '@/lib/crash-breadcrumb-data' import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' @@ -49,15 +63,24 @@ export const REACT_COMMIT_CASCADE_ARM_COMMITS = 20 /** Matches RENDERER_BREADCRUMB_COALESCE_MS; main drops anything faster anyway. */ export const REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS = 30_000 +/** Which signal counted this run's commits. See the header: only `lanes` is measured. */ +export type ReactCommitCascadeEvidence = 'lanes' | 'reentrant' | 'mixed' + export type ReactCommitCascadeState = { /** - * Held strongly, and never dereferenced. The next commit that leaves no - * cascading lanes clears the slot — milliseconds away in a live renderer — - * so the only tree this pins is a React root unmounted mid-cascade, such as - * the per-decoration roots in useDiffCommentDecorator. + * Held strongly, and never dereferenced. Any span with two or more commits + * marks its last commit re-entrant, so the slot is set far more often than a + * lane cascade alone would set it — and an unmounted root (the per-decoration + * roots in useDiffCommentDecorator) must not be pinned through an idle window + * waiting for a next commit that may never come. A run with no lane evidence + * therefore releases the slot at its span's microtask checkpoint; a + * lane-cascading run, which legitimately spans ticks, releases it on the next + * commit that is neither lane-cascading nor re-entrant. */ cascadeRoot: unknown commits: number + /** Of `commits`, how many carried genuinely cascading lanes at commit time. */ + laneCommits: number reported: boolean armedAtMs: number | null lastReportedAtMs: number | null @@ -69,6 +92,7 @@ export function createReactCommitCascadeState(): ReactCommitCascadeState { return { cascadeRoot: null, commits: 0, + laneCommits: 0, reported: false, armedAtMs: null, lastReportedAtMs: null, @@ -85,6 +109,8 @@ export function setReactCommitCascadeRendererSurface(surface: RendererSurface): export function resetReactCommitCascadeTelemetryForTests(): void { Object.assign(sharedState, createReactCommitCascadeState()) + yieldCheckpointScheduled = false + sawCommitSinceYield = false rendererSurface = 'main' resetReactCommitCascadeWriteSamples() } @@ -92,11 +118,19 @@ export function resetReactCommitCascadeTelemetryForTests(): void { function endCascade(state: ReactCommitCascadeState): void { state.cascadeRoot = null state.commits = 0 + state.laneCommits = 0 state.reported = false state.armedAtMs = null resetReactCommitCascadeWriteSamples() } +function cascadeEvidence(state: ReactCommitCascadeState): ReactCommitCascadeEvidence { + if (state.laneCommits === 0) { + return 'reentrant' + } + return state.laneCommits === state.commits ? 'lanes' : 'mixed' +} + function reportCascade(state: ReactCommitCascadeState, pendingLanes: number, nowMs: number): void { const suppressed = state.suppressed state.suppressed = 0 @@ -108,7 +142,11 @@ function reportCascade(state: ReactCommitCascadeState, pendingLanes: number, now commits: state.commits, commitBudget: REACT_NESTED_UPDATE_LIMIT, elapsedMs: state.armedAtMs === null ? undefined : nowMs - state.armedAtMs, + // Why raw: this is the one field that says whether a cascade was observed + // or inferred, so it never carries a lane the root did not hold. pendingLanes, + laneCommits: state.laneCommits, + evidence: cascadeEvidence(state), // Why 0 is worth shipping: it says the loop is useState-driven, not store-driven. storeWrites: writes.storeWrites, storeWriteSites: writes.storeWriteSites, @@ -126,12 +164,14 @@ function recordCommit( state: ReactCommitCascadeState, root: unknown, pendingLanes: number, + reentrant: boolean, readNowMs: () => number, noticeLimit: number, armCommits: number, minReportIntervalMs: number ): void { - if ((pendingLanes & REACT_CASCADING_LANES) === 0) { + const laneCascading = (pendingLanes & REACT_CASCADING_LANES) !== 0 + if (!laneCascading && !reentrant) { if (state.cascadeRoot !== null) { endCascade(state) } @@ -143,6 +183,9 @@ function recordCommit( } state.commits += 1 + if (laneCascading) { + state.laneCommits += 1 + } if (state.commits < armCommits) { return } @@ -172,6 +215,8 @@ export function recordReactCommit(args: { state: ReactCommitCascadeState root: unknown pendingLanes: number + /** The commit arrived before the stack unwound to a microtask checkpoint. */ + reentrant?: boolean readNowMs: () => number noticeLimit?: number armCommits?: number @@ -181,6 +226,7 @@ export function recordReactCommit(args: { args.state, args.root, args.pendingLanes, + args.reentrant ?? false, args.readNowMs, args.noticeLimit ?? REACT_COMMIT_CASCADE_NOTICE_LIMIT, args.armCommits ?? REACT_COMMIT_CASCADE_ARM_COMMITS, @@ -189,17 +235,53 @@ export function recordReactCommit(args: { } /** - * Per-commit hot path: one property read, one mask, two compares, one - * increment. No clock read and no allocation until a cascade arms. + * Per-commit hot path: one property read, one mask, three compares, one + * increment, plus at most one microtask per tick. No clock read and no + * allocation until a cascade arms. */ export function observeReactCommit(root: unknown, pendingLanes: number): void { recordCommit( sharedState, root, pendingLanes, + isReentrantCommit(), Date.now, REACT_COMMIT_CASCADE_NOTICE_LIMIT, REACT_COMMIT_CASCADE_ARM_COMMITS, REACT_COMMIT_CASCADE_MIN_REPORT_INTERVAL_MS ) } + +let yieldCheckpointScheduled = false +let sawCommitSinceYield = false + +/** Hoisted: the hot path must not allocate a closure per tick. */ +const clearYieldCheckpoint = (): void => { + yieldCheckpointScheduled = false + sawCommitSinceYield = false + // A run with no lane evidence was counted purely from commits sharing one + // synchronous span, which nothing can extend past this checkpoint. Dropping it + // here rather than at the next commit is what keeps `cascadeRoot` from pinning + // an unmounted root through an idle window. + if (sharedState.cascadeRoot !== null && sharedState.laneCommits === 0) { + endCascade(sharedState) + } +} + +// A microtask cannot run while a synchronous commit cascade is still unwinding, +// so "another commit before the checkpoint" is React's nested-update rule minus +// its lane term — over-counting the bursts the header describes. +function isReentrantCommit(): boolean { + // Why fail closed: with no checkpoint to clear it, the flag would latch on the + // first commit and report every later commit re-entrant for the module's life. + if (typeof queueMicrotask !== 'function') { + return false + } + const reentrant = sawCommitSinceYield + sawCommitSinceYield = true + if (!yieldCheckpointScheduled) { + yieldCheckpointScheduled = true + queueMicrotask(clearYieldCheckpoint) + } + return reentrant +}