From 7bd7da7a8b64f77d9816a491149ec7dfe0bdaf7d Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 23:50:42 -0700 Subject: [PATCH] fix(terminal): judge admission eligibility on the largest deferred set seen Review found the launch worktree never warms up: it is restored active before hydration opens the startup gate, so admission read an empty deferred set, cached ineligible, and never recomputed once the real plan landed. Judge on the high-water mark instead - an over-cap worktree still stays ineligible as its set drains, but a later plan is seen. Also from review: the e2e WebGL counter read getPanes(), which returns a public projection with no webglAddon field, so it was always 0; read getRenderingDiagnostics() instead. Filler worktrees now clean up on failure (testRepoPath is worker-scoped), and the restore metric is named for what it measures rather than implying a pixel assertion. --- .../activation-deferred-tab-admission.test.ts | 16 ++ .../use-activation-deferred-tab-admission.ts | 29 ++- tests/e2e/worktree-switch-first-paint.spec.ts | 201 ++++++++++-------- 3 files changed, 147 insertions(+), 99 deletions(-) diff --git a/src/renderer/src/components/terminal/activation-deferred-tab-admission.test.ts b/src/renderer/src/components/terminal/activation-deferred-tab-admission.test.ts index 1bb0be78700..e84b98683aa 100644 --- a/src/renderer/src/components/terminal/activation-deferred-tab-admission.test.ts +++ b/src/renderer/src/components/terminal/activation-deferred-tab-admission.test.ts @@ -48,6 +48,22 @@ describe('activation-deferred tab admission', () => { expect(isActivationAdmissionEligible(ACTIVATION_DEFERRED_ADMISSION_LIMIT + 1)).toBe(false) }) + // The launch worktree is restored active before hydration opens the startup + // gate, so admission first sees an empty set and only later the real plan. + // Judging on the high-water mark is what lets that plan still be admitted, + // while an over-cap worktree stays ineligible as its set drains. + it('judges eligibility on the largest deferred set seen, not the latest', () => { + const highWaterMark = (counts: readonly number[]): number => + counts.reduce((seen, count) => Math.max(seen, count), 0) + + // Launch: empty reading before hydration, then the real 3-tab plan. + expect(isActivationAdmissionEligible(highWaterMark([0, 3]))).toBe(true) + // Draining 3 -> 2 -> 1 must not change the verdict. + expect(isActivationAdmissionEligible(highWaterMark([0, 3, 2, 1]))).toBe(true) + // An over-cap activation stays ineligible however far it drains. + expect(isActivationAdmissionEligible(highWaterMark([7, 4, 2]))).toBe(false) + }) + // The contract that makes deferral free: repeated admission ends with the // worktree exactly as fully mounted as it would have been without deferral. it('drains to a fully mounted worktree with no restriction left behind', () => { diff --git a/src/renderer/src/components/terminal/use-activation-deferred-tab-admission.ts b/src/renderer/src/components/terminal/use-activation-deferred-tab-admission.ts index 5ebef1d7146..8a46a4e7594 100644 --- a/src/renderer/src/components/terminal/use-activation-deferred-tab-admission.ts +++ b/src/renderer/src/components/terminal/use-activation-deferred-tab-admission.ts @@ -29,24 +29,31 @@ export function useActivationDeferredTabAdmission( renderedActiveWorktreeId, setBackgroundMountRevision } = controller - // Why the verdict is taken once per activation: draining the set must not walk - // an over-cap worktree down into eligibility and warm up tabs the pre-deferral - // behaviour would have left unmounted. - const admissionRef = useRef<{ worktreeId: string; eligible: boolean } | null>(null) + // Why the high-water mark rather than the live count: draining must not walk an + // over-cap worktree down into eligibility and warm up tabs the pre-deferral + // behaviour left unmounted — but a verdict latched on one reading would never + // recover either. At launch the active worktree is restored before hydration + // opens the startup gate, so the first reading is an empty set; only re-reading + // on growth lets that worktree's real plan be judged when it finally lands. + const admissionRef = useRef<{ worktreeId: string; maxDeferredTabCount: number } | null>(null) useEffect(() => { const worktreeId = renderedActiveWorktreeId if (!worktreeId) { return } - const deferredTabIds = activationDeferredMountTabIdsByWorktreeRef.current.get(worktreeId) - if (admissionRef.current?.worktreeId !== worktreeId) { - admissionRef.current = { - worktreeId, - eligible: isActivationAdmissionEligible(deferredTabIds?.size ?? 0) - } + const deferredTabCount = + activationDeferredMountTabIdsByWorktreeRef.current.get(worktreeId)?.size ?? 0 + if (deferredTabCount === 0) { + return } - if (!admissionRef.current.eligible || !deferredTabIds?.size) { + const previous = admissionRef.current + const maxDeferredTabCount = + previous?.worktreeId === worktreeId + ? Math.max(previous.maxDeferredTabCount, deferredTabCount) + : deferredTabCount + admissionRef.current = { worktreeId, maxDeferredTabCount } + if (!isActivationAdmissionEligible(maxDeferredTabCount)) { return } return scheduleActivationDeferredAdmission(() => { diff --git a/tests/e2e/worktree-switch-first-paint.spec.ts b/tests/e2e/worktree-switch-first-paint.spec.ts index 6524afca4c3..2e5e6b0147c 100644 --- a/tests/e2e/worktree-switch-first-paint.spec.ts +++ b/tests/e2e/worktree-switch-first-paint.spec.ts @@ -48,7 +48,7 @@ const SWITCH_SAMPLE_COUNT = Number(process.env.ORCA_SWITCH_ROUNDS ?? '5') type SwitchSample = { activationMs: number | null paneMountedMs: number | null - contentPaintedMs: number | null + contentRestoredMs: number | null maxFrameGapMs: number longTaskTotalMs: number worstLongTaskMs: number @@ -62,7 +62,7 @@ type SwitchPaintProbe = { t0: number activationMs: number | null paneMountedMs: number | null - contentPaintedMs: number | null + contentRestoredMs: number | null frames: number[] longTasks: number[] mountedAtActivation: number @@ -146,7 +146,7 @@ async function measureSwitch( t0: performance.now(), activationMs: null as number | null, paneMountedMs: null as number | null, - contentPaintedMs: null as number | null, + contentRestoredMs: null as number | null, frames: [] as number[], longTasks: [] as number[], mountedAtActivation: 0, @@ -192,9 +192,11 @@ async function measureSwitch( if (probe.paneMountedMs === null && pane?.container?.isConnected) { probe.paneMountedMs = now } - if (probe.contentPaintedMs === null && pane) { - // Painted = the revealed viewport actually carries restored text, not - // an empty grid. A blank reveal fails this until the replay lands. + if (probe.contentRestoredMs === null && pane) { + // Restored = the revealed viewport carries real text rather than an + // empty grid. Read on a frame callback, so this is the frame the + // content became renderable — one frame ahead of the pixels, and not + // a pixel assertion. Both arms are measured identically. const buffer = pane.terminal.buffer.active let filledRows = 0 for (let row = 0; row < pane.terminal.rows; row += 1) { @@ -204,7 +206,7 @@ async function measureSwitch( } } if (filledRows >= Math.min(5, pane.terminal.rows)) { - probe.contentPaintedMs = now + probe.contentRestoredMs = now } } requestAnimationFrame(tick) @@ -238,12 +240,14 @@ async function measureSwitch( let settledWebglContexts = 0 const managers = window.__paneManagers for (const manager of managers?.values() ?? []) { - for (const pane of manager.getPanes?.() ?? []) { - settledPanes += 1 - if ((pane as { webglAddon?: unknown }).webglAddon) { - settledWebglContexts += 1 - } - } + settledPanes += (manager.getPanes?.() ?? []).length + // Why diagnostics and not `pane.webglAddon`: getPanes() hands back a public + // projection that has no webglAddon field, so reading it is always falsy. + const diagnostics = + ( + manager as { getRenderingDiagnostics?: () => { hasWebgl?: boolean }[] } + ).getRenderingDiagnostics?.() ?? [] + settledWebglContexts += diagnostics.filter((entry) => entry.hasWebgl === true).length } return { settledPaneManagers: managers?.size ?? 0, @@ -251,7 +255,7 @@ async function measureSwitch( settledWebglContexts, activationMs: probe.activationMs, paneMountedMs: probe.paneMountedMs, - contentPaintedMs: probe.contentPaintedMs, + contentRestoredMs: probe.contentRestoredMs, maxFrameGapMs: +maxGap.toFixed(1), longTaskTotalMs: +probe.longTasks.reduce((total, value) => total + value, 0).toFixed(1), worstLongTaskMs: +probe.longTasks @@ -267,7 +271,7 @@ function report(label: string, sample: SwitchSample): string { `${label}:`, ` activation ${sample.activationMs?.toFixed(1) ?? 'n/a'}ms`, ` pane mounted ${sample.paneMountedMs?.toFixed(1) ?? 'n/a'}ms`, - ` content painted ${sample.contentPaintedMs?.toFixed(1) ?? 'never'}ms`, + ` content restored ${sample.contentRestoredMs?.toFixed(1) ?? 'never'}ms`, ` max frame gap ${sample.maxFrameGapMs}ms`, ` long tasks total=${sample.longTaskTotalMs}ms worst=${sample.worstLongTaskMs}ms`, ` panes at switch ${sample.mountedAtActivation}/${TABS_PER_WORKTREE}`, @@ -293,12 +297,46 @@ async function addFillerWorktrees( const paths = Array.from({ length: FILLER_WORKTREE_COUNT }, (_, index) => path.join(parent, `filler-${index}`) ) - for (const worktreePath of paths) { - execFileSync('git', ['worktree', 'add', '--detach', worktreePath, 'HEAD'], { - cwd: testRepoPath, - stdio: 'ignore' - }) + const removeAll = (): void => { + for (const worktreePath of paths) { + try { + execFileSync('git', ['worktree', 'remove', '--force', worktreePath], { + cwd: testRepoPath, + stdio: 'ignore' + }) + } catch { + /* best effort */ + } + } + rmSync(parent, { recursive: true, force: true }) } + // Why clean up before rethrowing: testRepoPath is worker-scoped and reused by + // later specs, so a half-built fixture would leak worktrees into them. + try { + for (const worktreePath of paths) { + execFileSync('git', ['worktree', 'add', '--detach', worktreePath, 'HEAD'], { + cwd: testRepoPath, + stdio: 'ignore' + }) + } + } catch (error) { + removeAll() + throw error + } + try { + return await registerFillerWorktrees(page, testRepoPath, paths, removeAll) + } catch (error) { + removeAll() + throw error + } +} + +async function registerFillerWorktrees( + page: Page, + testRepoPath: string, + paths: readonly string[], + cleanup: () => void +): Promise<{ ids: string[]; cleanup: () => void }> { const repoId = await page.evaluate( (repoPath) => window.__store!.getState().repos.find((repo) => repo.path === repoPath)?.id ?? null, @@ -307,7 +345,7 @@ async function addFillerWorktrees( if (!repoId) { throw new Error(`seeded repo not registered: ${testRepoPath}`) } - await loadWorktreesUntilPathsPresent(page, repoId, paths) + await loadWorktreesUntilPathsPresent(page, repoId, [...paths]) const ids = await page.evaluate( ({ id, wanted }) => (window.__store!.getState().worktreesByRepo[id] ?? []) @@ -315,22 +353,7 @@ async function addFillerWorktrees( .map((worktree) => worktree.id), { id: repoId, wanted: paths } ) - return { - ids, - cleanup: () => { - for (const worktreePath of paths) { - try { - execFileSync('git', ['worktree', 'remove', '--force', worktreePath], { - cwd: testRepoPath, - stdio: 'ignore' - }) - } catch { - /* best effort */ - } - } - rmSync(parent, { recursive: true, force: true }) - } - } + return { ids, cleanup } } function median(values: readonly number[]): number { @@ -354,65 +377,67 @@ test.describe('Worktree switch first paint', () => { const [primaryId, targetId] = worktreeIds const filler = await addFillerWorktrees(orcaPage, testRepoPath) - const targetTabIds = await ensureTabs(orcaPage, targetId, 'WTB') - - // Give the filler worktrees persisted tabs without mounting them, so the - // store carries a field-scale tab population (the profile that motivated - // this budget has 846 tabs across 449 worktrees). - await orcaPage.evaluate( - ({ ids, perWorktree }) => { - const state = window.__store!.getState() - for (const id of ids) { - const existing = state.tabsByWorktree[id] ?? [] - for (let index = existing.length; index < perWorktree; index += 1) { - state.createTab(id) - } - } - }, - { ids: filler.ids, perWorktree: 2 } - ) - const samples: SwitchSample[] = [] const lines: string[] = [] - for (let round = 0; round < SWITCH_SAMPLE_COUNT; round += 1) { - // Leave the primary active and let the session persist before reloading: - // startup restores the persisted active worktree, so this is what makes - // the target come back with tabs in the session and no pane ever mounted - // — the state every switch lands in once the worktree count exceeds the - // hot-retain working set. - await switchToWorktree(orcaPage, primaryId) - await ensureTerminalVisible(orcaPage) - await orcaPage.waitForTimeout(2_500) - await orcaPage.reload() - await waitForSessionReady(orcaPage) - await waitForActiveWorktree(orcaPage) - await ensureTerminalVisible(orcaPage) - await orcaPage.waitForTimeout(2_500) - const unmounted = await waitForUnmountedTabs(orcaPage, targetTabIds) - expect(unmounted, 'target worktree was already mounted before the switch').toBe(true) + try { + const targetTabIds = await ensureTabs(orcaPage, targetId, 'WTB') - const sample = await measureSwitch(orcaPage, targetId, targetTabIds) - samples.push(sample) - lines.push(report(`round ${round + 1} (target unmounted=${unmounted})`, sample)) - - // The half of the contract that keeps the speed-up free: the hidden tabs - // the switch skipped still end up mounted, so the next tab switch is as - // warm as it was before the reveal stopped mounting them up front. - const warmedTabIds = await waitForMountedTabs(orcaPage, targetTabIds) - expect(warmedTabIds, 'deferred tabs never joined the warm working set').toEqual( - [...targetTabIds].sort() + // Give the filler worktrees persisted tabs without mounting them, so the + // store carries a field-scale tab population (the profile that motivated + // this budget has 846 tabs across 449 worktrees). + await orcaPage.evaluate( + ({ ids, perWorktree }) => { + const state = window.__store!.getState() + for (const id of ids) { + const existing = state.tabsByWorktree[id] ?? [] + for (let index = existing.length; index < perWorktree; index += 1) { + state.createTab(id) + } + } + }, + { ids: filler.ids, perWorktree: 2 } ) + + for (let round = 0; round < SWITCH_SAMPLE_COUNT; round += 1) { + // Leave the primary active and let the session persist before reloading: + // startup restores the persisted active worktree, so this is what makes + // the target come back with tabs in the session and no pane ever mounted + // — the state every switch lands in once the worktree count exceeds the + // hot-retain working set. + await switchToWorktree(orcaPage, primaryId) + await ensureTerminalVisible(orcaPage) + await orcaPage.waitForTimeout(2_500) + await orcaPage.reload() + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + await orcaPage.waitForTimeout(2_500) + const unmounted = await waitForUnmountedTabs(orcaPage, targetTabIds) + expect(unmounted, 'target worktree was already mounted before the switch').toBe(true) + + const sample = await measureSwitch(orcaPage, targetId, targetTabIds) + samples.push(sample) + lines.push(report(`round ${round + 1} (target unmounted=${unmounted})`, sample)) + + // The half of the contract that keeps the speed-up free: the hidden tabs + // the switch skipped still end up mounted, so the next tab switch is as + // warm as it was before the reveal stopped mounting them up front. + const warmedTabIds = await waitForMountedTabs(orcaPage, targetTabIds) + expect(warmedTabIds, 'deferred tabs never joined the warm working set').toEqual( + [...targetTabIds].sort() + ) + } + } finally { + filler.cleanup() } - filler.cleanup() - - const painted = samples - .map((sample) => sample.contentPaintedMs) + const restored = samples + .map((sample) => sample.contentRestoredMs) .filter((value): value is number => value !== null) - expect(painted.length, 'revealed terminal never painted restored content').toBe(samples.length) + expect(restored.length, 'revealed terminal never restored its content').toBe(samples.length) const summary = [ `first activation -> ${TABS_PER_WORKTREE}-tab worktree, ${samples.length} rounds`, - ` content painted: median=${median(painted).toFixed(1)}ms samples=${painted + ` content restored: median=${median(restored).toFixed(1)}ms samples=${restored .map((value) => value.toFixed(0)) .join(', ')}ms`, ` activation: median=${median( @@ -432,6 +457,6 @@ test.describe('Worktree switch first paint', () => { 'the switch mounted more than the pane the user is looking at' ).toBe(1) } - expect(median(painted)).toBeLessThanOrEqual(FIRST_PAINT_BUDGET_MS) + expect(median(restored)).toBeLessThanOrEqual(FIRST_PAINT_BUDGET_MS) }) })