From b7209b5ae9723a07dc7c677816abedd9985f5ee8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:01:08 -0700 Subject: [PATCH] perf(git): relist only the repo whose worktrees changed, and stop blocking main on sync git (#23998) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(git): stop blocking main on the open-on-remote git cascade `getRemoteFileUrl` ran up to 6 sequential `gitExecFileSync` calls on the Electron main thread — `remote get-url`, then `getDefaultBaseRef`'s `symbolic-ref` plus up to four `rev-parse --verify` probes — each with its own 15s timeout and no yield between them. A complete async twin already existed (`getDefaultBaseRefAsync` -> `resolveDefaultBaseRefViaExec`, sharing DEFAULT_BASE_REF_PROBES), so the sync cascade is deleted rather than converted. `getRemoteUrl`, `getRemoteFileUrl` and `getRemoteCommitUrl` become async; all four downstream callers were already async (`filesystem-git-url-handlers` inside `ipcMain.handle`, `runtime-git-diff-commands` async methods) and the provider contract already typed both wrappers `Promise`, so no new async plumbing was needed. Removes 3 of the 10 `gitExecFileSync` sites and the confusing name collision with the unrelated async `getDefaultBaseRef` in hosted-review-creation-git-state. The base-ref regression tests keep their coverage, repointed at the public async `getBaseRefDefault`. * perf(git): resolve the repo root in one sync spawn instead of two getGitRepoRoot ran `rev-parse --is-inside-work-tree` and then `rev-parse --show-toplevel` as separate blocking spawns. Each sync git call holds the main thread for up to its whole 15s timeout, so the spawn count is the cost — and this function is called twice per "Add Project" on a linked worktree, once directly and once through getLinkedWorktreeMainRepoRoot's self-recursion. Combined into one invocation. Safe only here: in a bare repo the combined form exits non-zero, and both that throw and the plain `false` already land on the same marker-scan fallback. probeGitRepo deliberately does NOT combine — it has to read `false` cleanly to go on and detect a bare repo, which the combined form's exit 128 would misread as indeterminate. * perf(git): rebuild only the repos whose authorized roots actually changed One worktree create called `invalidateAuthorizedRootsCache()`, which dirties every registered owner. The next authorization-requiring IPC then rebuilt by listing EVERY repo — and the rebuild never consulted `dirty` when choosing what to list, so `dirty` gated only whether a rebuild ran, not its scope. At 58 repos that is 58 `git worktree list` spawns, roughly ten seconds of git wall-clock through an admission budget of four, to rediscover roots one repo changed. Both halves were needed; scoping the invalidation alone changed nothing. - `markAuthorizedRootsOwnerDirty` dirties a single owner, reusing the per-owner primitives `registerWorktreeRootsForRepo` already used. It leaves `baseRevision` and the per-repo revision map alone — that pair is the global side-effect-token fence, and bumping it would retire in-flight tokens for untouched repos. - `rebuildAuthorizedRootsCache(store, onlyDirty)` re-lists only owners that are dirty, have no listing yet, or still hold recovered roots (those are retired by comparison against a fresh listing, so skipping them would strand them as authorized). Only `ensureAuthorizedRootsCache` passes `onlyDirty`; an explicit rebuild keeps re-listing everything because callers use it to force a refresh — `filesystem-auth.test.ts` pins that contract. `invalidateAuthorizedRootsCacheForRepo` wraps the primitive and falls back to the global form for an unknown owner or a missing store, rather than silently skipping an invalidation and leaving a stale allowlist. Applied to the worktree-create path. Changes that can alter the owner SET (store swap, host/WSL re-routing, nested-repo import, folder->git upgrade) stay global. Removal paths are not converted yet. The allowlist contents are unchanged and the failure direction is a false denial rather than a false allow. The relist predicate is split into its own module so it is testable alone and the cache file stays inside its line budget without a suppression. * test(perf): measure what git orchestration actually costs the main thread The existing churn probe (ORCA_MAIN_THREAD_DIAGNOSTICS=1) reported spawn-initiation cost for git/gh/glab only — its 7 call sites all sit inside git/command-runner — so it was blind to `spawnProcess`/`runProcess`, the repo's own mandated wrapper, and to the blocking `execFileSync('ps')` per PTY resize. That understated total churn across 115 main call sites. - `spawn-observer.ts`: a settable seam, since shared code cannot import src/main. Unregistered in the daemon/relay/CLI, where it costs one boolean check. - `spawnProcess` brackets `nodeSpawn` and reports; exec-file-capture's own report is removed because it routes through runProcess and would double-count. - `posix-pty-foreground-group` now reports its full blocking duration. Note this lands on the daemon, not main, whenever the daemon hosts the PTY. - `ORCA_UNMINIFIED_MAIN=1` build flag, because a minified main bundle cannot attribute CPU-profile self time to real function names. Defaults unchanged. - `main-thread-git-cost.spec.ts` + `analyze-main-cpuprofile.mjs`: sweeps concurrency against real registered repos, captures the churn lines and a V8 CPU profile of main per phase. What it found, which is why this is worth keeping: at the width-4 admission ceiling (~90 git:status/s) main sees ZERO event-loop gaps over 50ms and a worst gap of 23ms, and is 85% idle. Git orchestration does not stall the main thread. Of the cost it does incur, spawn-init is 58%, parse 5%, stdout drain 4%. * test(perf): name the inspector params type the anti-slop gate requires The broad `object` parameter trips anti-slop(no-object-parameters); the only Profiler call that passes params sends `{ interval }`. --- electron.vite.config.ts | 9 +- .../git/command-runner/exec-file-capture.ts | 7 +- src/main/git/repo-default-base-ref.ts | 36 +-- src/main/git/repo-detection.ts | 21 +- src/main/git/repo.test.ts | 28 +-- src/main/git/repo.ts | 21 +- src/main/ipc/filesystem-auth.ts | 1 + src/main/ipc/folder-repo-git-upgrade.test.ts | 3 +- .../registered-worktree-root-relist-policy.ts | 26 ++ .../ipc/registered-worktree-roots-cache.ts | 59 +++-- ...worktree-roots-scoped-invalidation.test.ts | 105 ++++++++ ...ered-worktree-roots-scoped-invalidation.ts | 29 +++ src/main/ipc/repos-picker.test.ts | 3 +- .../ipc/repos-remote-client-events.test.ts | 5 +- src/main/ipc/repos-sparse-presets.test.ts | 3 +- .../repo-creation-git-availability.test.ts | 3 +- ...ve-unregistered-worktree-host-home.test.ts | 3 +- src/main/pty/posix-pty-foreground-group.ts | 18 +- .../runtime/fit-override-integration.test.ts | 3 +- src/main/runtime/mobile-presence-lock.test.ts | 3 +- ...bile-session-tabs-churn-coalescing.test.ts | 3 +- .../mobile-subscribe-integration.test.ts | 3 +- .../orca-runtime-create-managed-worktree.ts | 8 +- .../orca-runtime-test-mocks/setup.spec.ts | 4 +- .../runtime/remote-desktop-driver.test.ts | 3 +- ...untime-local-create-rearm-ordering.test.ts | 5 +- src/main/startup/main-process-preflight.ts | 12 +- src/shared/child-process/run-process.ts | 11 +- .../child-process/spawn-observer.test.ts | 39 +++ src/shared/child-process/spawn-observer.ts | 28 +++ tests/e2e/main-thread-git-cost.spec.ts | 225 ++++++++++++++++++ .../benchmarks/analyze-main-cpuprofile.mjs | 213 +++++++++++++++++ 32 files changed, 830 insertions(+), 110 deletions(-) create mode 100644 src/main/ipc/registered-worktree-root-relist-policy.ts create mode 100644 src/main/ipc/registered-worktree-roots-scoped-invalidation.test.ts create mode 100644 src/main/ipc/registered-worktree-roots-scoped-invalidation.ts create mode 100644 src/shared/child-process/spawn-observer.test.ts create mode 100644 src/shared/child-process/spawn-observer.ts create mode 100644 tests/e2e/main-thread-git-cost.spec.ts create mode 100644 tests/tools/benchmarks/analyze-main-cpuprofile.mjs diff --git a/electron.vite.config.ts b/electron.vite.config.ts index 87156caf6e4..221ac77b69f 100644 --- a/electron.vite.config.ts +++ b/electron.vite.config.ts @@ -196,13 +196,20 @@ function createMainBootstrapPlugin() { } } +/** + * Diagnostic escape hatch: an unminified main bundle so a V8 CPU profile of the + * main process attributes self time to real function names. Release builds never + * set this, and `pnpm build` does not read it. + */ +const MAIN_MINIFY: 'oxc' | false = process.env.ORCA_UNMINIFIED_MAIN === '1' ? false : 'oxc' + export const electronViteConfig: UserConfig = { main: { build: { // Why: 'esbuild' makes rolldown disable its own minifier and re-print every // chunk through esbuild, which is undeclared here and only resolves via // pnpm hoisting. 'oxc' is rolldown's in-process minifier. - minify: 'oxc', + minify: MAIN_MINIFY, // Why: 'hidden' emits .js.map with no sourceMappingURL, so the shipped // bundle never references maps that packaging strips out. Release CI // uploads them so minified crash traces stay decodable. diff --git a/src/main/git/command-runner/exec-file-capture.ts b/src/main/git/command-runner/exec-file-capture.ts index f343246571d..b94d2f59509 100644 --- a/src/main/git/command-runner/exec-file-capture.ts +++ b/src/main/git/command-runner/exec-file-capture.ts @@ -27,10 +27,8 @@ export async function execFileCaptureToTermination( options: ExecFileCaptureOptions, termination?: WslProcessGroupTermination ): Promise<{ stdout: string | Buffer; stderr: string | Buffer }> { - // Why measured here: runProcess spawns inside its promise executor, which runs - // synchronously, so this brackets exactly the main-thread block execFileCapture - // reports for its own spawns. - const spawnStartedAt = performance.now() + // Spawn cost is reported by spawnProcess's observer, which runProcess goes + // through; recording it again here would double-count every capture. const pending = runProcess({ program: command, args, @@ -43,7 +41,6 @@ export async function execFileCaptureToTermination( onChildTerminated: options.onChildTerminated, ...(options.stdin === undefined ? {} : { input: options.stdin }) }) - recordSubprocessSpawn(command, args, performance.now() - spawnStartedAt) const result = await pending const stdout = options.encoding === 'buffer' ? Buffer.from(result.stdout) : result.stdout const cleanStderr = termination?.stripControlOutput(result.stderr) ?? result.stderr diff --git a/src/main/git/repo-default-base-ref.ts b/src/main/git/repo-default-base-ref.ts index c43cd14c72a..8fb3acd764e 100644 --- a/src/main/git/repo-default-base-ref.ts +++ b/src/main/git/repo-default-base-ref.ts @@ -1,5 +1,5 @@ import type { GitAdmissionTier } from '../../shared/rpc-contract/git-admission-tier-params' -import { gitExecFileAsync, gitExecFileSync } from './runner' +import { gitExecFileAsync } from './runner' export type LocalGitExecOptions = { wslDistro?: string @@ -43,44 +43,10 @@ async function resolveDefaultBaseRefFromProbes( return null } -function hasGitRef(path: string, ref: string): boolean { - try { - gitExecFileSync(['rev-parse', '--verify', ref], { cwd: path }) - return true - } catch { - return false - } -} - function gitRefToDefaultBaseRef(ref: string): string { return ref.replace(/^refs\/remotes\//, '') } -function getVerifiedOriginHeadBaseRef(path: string): string | null { - try { - const ref = gitExecFileSync(['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], { - cwd: path - }).trim() - return ref && hasGitRef(path, ref) ? gitRefToDefaultBaseRef(ref) : null - } catch { - return null - } -} - -/** Resolve the default base ref without inventing a fallback branch. */ -export function getDefaultBaseRef(path: string): string | null { - const originHeadBaseRef = getVerifiedOriginHeadBaseRef(path) - if (originHeadBaseRef) { - return originHeadBaseRef - } - for (const { ref, returnAs } of DEFAULT_BASE_REF_PROBES) { - if (hasGitRef(path, ref)) { - return returnAs - } - } - return null -} - export async function getBaseRefDefault( path: string, options: LocalGitExecOptions = {} diff --git a/src/main/git/repo-detection.ts b/src/main/git/repo-detection.ts index 83e3a4c6796..f9105b7e26f 100644 --- a/src/main/git/repo-detection.ts +++ b/src/main/git/repo-detection.ts @@ -78,14 +78,19 @@ export function getGitRepoRoot(path: string): string { if (!existsSync(path) || !statSync(path).isDirectory()) { return path } - const insideWorkTree = gitExecFileSync(['rev-parse', '--is-inside-work-tree'], { - cwd: path - }).trim() - if (insideWorkTree === 'true') { - const root = gitExecFileSync(['rev-parse', '--show-toplevel'], { - cwd: path - }).trim() - return normalizeGitRepoRootForInputPath(path, root) + // One spawn, not two: each sync git call blocks main for up to its whole 15s + // timeout, so the spawn count is the cost. Safe to combine only here — a bare + // repo makes the combined form exit non-zero, and both that throw and the + // plain `false` land on the same marker-scan fallback below. `probeGitRepo` + // must NOT combine: it has to read `false` cleanly to go on and detect bare. + const [insideWorkTree, toplevel] = gitExecFileSync( + ['rev-parse', '--is-inside-work-tree', '--show-toplevel'], + { cwd: path } + ) + .split('\n') + .map((line) => line.trim()) + if (insideWorkTree === 'true' && toplevel) { + return normalizeGitRepoRootForInputPath(path, toplevel) } } catch { // Fall through to preserving the original path. diff --git a/src/main/git/repo.test.ts b/src/main/git/repo.test.ts index f08ce25eca3..4c75886a922 100644 --- a/src/main/git/repo.test.ts +++ b/src/main/git/repo.test.ts @@ -6,7 +6,7 @@ import path from 'node:path' import { buildSearchBaseRefsArgv, - getDefaultBaseRef, + getBaseRefDefault, getBranchConflictKind, getRemoteCount, parseAndFilterSearchRefDetails, @@ -537,7 +537,7 @@ describe('searchBaseRefs (widened glob)', () => { }) }) -describe('getDefaultBaseRef (regression — unchanged behavior)', () => { +describe('getBaseRefDefault (regression — unchanged behavior)', async () => { let tmpDir: string beforeEach(() => { @@ -549,52 +549,52 @@ describe('getDefaultBaseRef (regression — unchanged behavior)', () => { rmSync(tmpDir, { recursive: true, force: true }) }) - it('returns origin/main when both origin/main and upstream/main exist (origin wins)', () => { + it('returns origin/main when both origin/main and upstream/main exist (origin wins)', async () => { const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'origin/main', sha) createRemoteRef(tmpDir, 'upstream/main', sha) - const result = getDefaultBaseRef(tmpDir) + const result = await getBaseRefDefault(tmpDir) expect(result).toBe('origin/main') }) - it('returns the target of origin/HEAD when set', () => { + it('returns the target of origin/HEAD when set', async () => { const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'origin/main', sha) git(tmpDir, ['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/main']) - const result = getDefaultBaseRef(tmpDir) + const result = await getBaseRefDefault(tmpDir) expect(result).toBe('origin/main') }) - it('falls through from a stale origin/HEAD target to an existing primary ref', () => { + it('falls through from a stale origin/HEAD target to an existing primary ref', async () => { const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'origin/main', sha) git(tmpDir, ['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/master']) - const result = getDefaultBaseRef(tmpDir) + const result = await getBaseRefDefault(tmpDir) expect(result).toBe('origin/main') }) - it('falls through from a stale origin/HEAD primary target to another existing default ref', () => { + it('falls through from a stale origin/HEAD primary target to another existing default ref', async () => { const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'origin/master', sha) git(tmpDir, ['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/main']) - const result = getDefaultBaseRef(tmpDir) + const result = await getBaseRefDefault(tmpDir) expect(result).toBe('origin/master') }) - it('does NOT fall through to upstream/main when origin/* is absent', () => { + it('does NOT fall through to upstream/main when origin/* is absent', async () => { // Why: default probe order is origin-only by design; upstream-aware defaulting is deferred. const sha = getHeadSha(tmpDir) createRemoteRef(tmpDir, 'upstream/main', sha) - const result = getDefaultBaseRef(tmpDir) + const result = await getBaseRefDefault(tmpDir) // initRepo creates a local `main`, so with no origin/* we expect it — not `upstream/main`. expect(result).toBe('main') @@ -602,7 +602,7 @@ describe('getDefaultBaseRef (regression — unchanged behavior)', () => { }) }) -describe('resolveDefaultBaseRefViaExec', () => { +describe('resolveDefaultBaseRefViaExec', async () => { it('falls through from a stale origin/HEAD target to the probe list', async () => { const calls: string[][] = [] const exec = async (argv: string[]): Promise<{ stdout: string }> => { @@ -649,7 +649,7 @@ describe('resolveDefaultBaseRefViaExec', () => { }) }) -describe('getRemoteCount', () => { +describe('getRemoteCount', async () => { let tmpDir: string beforeEach(() => { diff --git a/src/main/git/repo.ts b/src/main/git/repo.ts index fcf5167f907..decbfa6182c 100644 --- a/src/main/git/repo.ts +++ b/src/main/git/repo.ts @@ -3,12 +3,11 @@ import { parseGitRevListAheadBehindCounts } from '../../shared/git-rev-list-outp import { buildHostedRemoteCommitUrl, buildHostedRemoteFileUrl } from './hosted-remote-url' import { DEFAULT_BASE_REF_PROBE_TIMEOUT_MS, - getDefaultBaseRef, getDefaultBaseRefAsync, gitExecOptions, type LocalGitExecOptions } from './repo-default-base-ref' -import { gitExecFileAsync, gitExecFileSync } from './runner' +import { gitExecFileAsync } from './runner' export { isGitRepo, @@ -18,7 +17,6 @@ export { } from './repo-detection' export { DEFAULT_BASE_REF_PROBES, - getDefaultBaseRef, getBaseRefDefault, resolveDefaultBaseRefViaExec, resolveDefaultBaseRefWithLocalGit @@ -43,9 +41,10 @@ export function getRepoName(path: string): string { } /** Get the remote origin URL, or null if not set. */ -export function getRemoteUrl(path: string): string | null { +export async function getRemoteUrl(path: string): Promise { try { - return gitExecFileSync(['remote', 'get-url', 'origin'], { cwd: path }).trim() + const { stdout } = await gitExecFileAsync(['remote', 'get-url', 'origin'], { cwd: path }) + return stdout.trim() } catch { return null } @@ -168,16 +167,16 @@ export async function getDefaultRemote( } /** Build a hosted file URL when the origin belongs to a supported provider. */ -export function getRemoteFileUrl( +export async function getRemoteFileUrl( repoPath: string, relativePath: string, line: number -): string | null { - const remoteUrl = getRemoteUrl(repoPath) +): Promise { + const remoteUrl = await getRemoteUrl(repoPath) if (!remoteUrl) { return null } - const defaultBaseRef = getDefaultBaseRef(repoPath) + const defaultBaseRef = await getDefaultBaseRefAsync(repoPath) if (!defaultBaseRef) { return null } @@ -190,7 +189,7 @@ export function getRemoteFileUrl( } /** Build a hosted commit URL when the origin belongs to a supported provider. */ -export function getRemoteCommitUrl(repoPath: string, sha: string): string | null { - const remoteUrl = getRemoteUrl(repoPath) +export async function getRemoteCommitUrl(repoPath: string, sha: string): Promise { + const remoteUrl = await getRemoteUrl(repoPath) return remoteUrl ? buildHostedRemoteCommitUrl(remoteUrl, sha) : null } diff --git a/src/main/ipc/filesystem-auth.ts b/src/main/ipc/filesystem-auth.ts index 6110ea7baf2..620baa9b595 100644 --- a/src/main/ipc/filesystem-auth.ts +++ b/src/main/ipc/filesystem-auth.ts @@ -13,6 +13,7 @@ import { // Compatibility exports for runtime command modules that historically imported these seams from // filesystem-auth. The implementations remain owned by their focused modules. export { invalidateAuthorizedRootsCache } from './registered-worktree-roots-cache' +export { invalidateAuthorizedRootsCacheForRepo } from './registered-worktree-roots-scoped-invalidation' export { isENOENT } from './filesystem-path-containment' export const PATH_ACCESS_DENIED_MESSAGE = diff --git a/src/main/ipc/folder-repo-git-upgrade.test.ts b/src/main/ipc/folder-repo-git-upgrade.test.ts index d6baaa834c5..836ff0e7a65 100644 --- a/src/main/ipc/folder-repo-git-upgrade.test.ts +++ b/src/main/ipc/folder-repo-git-upgrade.test.ts @@ -51,7 +51,8 @@ vi.mock('./repos/repos-changed-notification', () => ({ notifyReposChanged: vi.fn() })) vi.mock('./registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../worktree-root-preparation', () => ({ prepareLocalWorktreeRootForRepo: vi.fn(async () => {}) diff --git a/src/main/ipc/registered-worktree-root-relist-policy.ts b/src/main/ipc/registered-worktree-root-relist-policy.ts new file mode 100644 index 00000000000..7707bcce983 --- /dev/null +++ b/src/main/ipc/registered-worktree-root-relist-policy.ts @@ -0,0 +1,26 @@ +/** + * Which owners an authorized-roots rebuild has to re-list. + * + * Split out of registered-worktree-roots-cache so the policy is testable on its own + * and the cache module stays within its line budget. + */ +/** The owner fields the policy reads; `undefined` means no owner record yet. */ +export type RelistCandidate = + | { dirty: boolean; listed: unknown; recovered: { size: number } } + | undefined + +/** + * `onlyDirty` is the ensure path, where a single-repo invalidation must not relist + * every repo. An explicit rebuild keeps re-listing everything, because callers use + * it to force a refresh. + * + * A clean owner still holding recovered roots is always re-listed: those roots are + * retired by comparing against a fresh listing, so skipping it would strand them + * as authorized. + */ +export function shouldRelistOwner(owner: RelistCandidate, onlyDirty: boolean): boolean { + if (!onlyDirty || owner === undefined) { + return true + } + return owner.dirty || owner.listed == null || owner.recovered.size > 0 +} diff --git a/src/main/ipc/registered-worktree-roots-cache.ts b/src/main/ipc/registered-worktree-roots-cache.ts index 30d89fce2dd..ba538356b9a 100644 --- a/src/main/ipc/registered-worktree-roots-cache.ts +++ b/src/main/ipc/registered-worktree-roots-cache.ts @@ -6,6 +6,7 @@ import { pruneCreatedWorktreeRoots } from './registered-worktree-root-probes' import { isDescendantOrEqual, normalizeExistingPath } from './filesystem-path-containment' +import { shouldRelistOwner } from './registered-worktree-root-relist-policy' import { getLocalWorktreeRootOwners, resolveWorktreeRootOwner @@ -38,6 +39,12 @@ function advanceOwner(owner: RegisteredOwner): void { registeredWorktreeRootsRevisionByRepo.set(owner.repoId, owner.revision) } +function resolveOwnerForRepo(store: Store, repo: Repo | string): RegisteredOwner | undefined { + const repos = synchronizeOwners(store) + const key = resolveWorktreeRootOwner(repos, repo, currentOwners) + return key === undefined ? undefined : registeredOwners.get(key) +} + function synchronizeOwners(store: Store): Repo[] { const repos = store.getRepos() const owners = getLocalWorktreeRootOwners(repos) @@ -88,15 +95,22 @@ export function invalidateAuthorizedRootsCache(): void { registeredWorktreeRootsRevisionByRepo.clear() } -export async function rebuildAuthorizedRootsCache(store: Store): Promise { +/** `onlyDirty` is the ensure path; an explicit rebuild still re-lists every repo. */ +export async function rebuildAuthorizedRootsCache(store: Store, onlyDirty = false): Promise { synchronizeOwners(store) const generation = invalidationGeneration - const pending = [...currentOwners].map(([key, repo]) => ({ - key, - repo, - owner: registeredOwners.get(key), - revision: registeredOwners.get(key)?.revision - })) + const pending = [...currentOwners] + .map(([key, repo]) => ({ + key, + repo, + owner: registeredOwners.get(key), + revision: registeredOwners.get(key)?.revision + })) + // Why only dirty owners: `dirty` used to gate whether a rebuild ran, not what it + // listed, so a single-repo invalidation still spawned `git worktree list` for + // every registered repo. An owner with no listing yet (`listed === null`) is + // always included, so a first rebuild is unchanged. + .filter((entry) => shouldRelistOwner(entry.owner, onlyDirty)) const listings = await listWorktreeRootsWithConcurrency(pending.map((entry) => entry.repo)) const results = pending.map((entry, index) => ({ ...entry, ...listings[index] })) const isCurrent = (entry: (typeof pending)[number]): boolean => @@ -128,14 +142,33 @@ export async function rebuildAuthorizedRootsCache(store: Store): Promise { registeredWorktreeRootsDirty = [...registeredOwners.values()].some((owner) => owner.dirty) } +/** + * Dirties exactly one owner so the next ensure-path rebuild re-lists only that repo. + * Returns false when the repo has no known owner, leaving the decision to the caller. + * + * Deliberately leaves `baseRevision` and the per-repo revision map alone: that pair is + * the global side-effect-token fence, and bumping it would retire in-flight tokens for + * untouched repos. `advanceOwner` still records this repo's new revision. + */ +export function markAuthorizedRootsOwnerDirty(store: Store, repo: Repo | string): boolean { + const owner = resolveOwnerForRepo(store, repo) + if (!owner) { + return false + } + owner.listed = null + owner.dirty = true + advanceOwner(owner) + refreshRegisteredWorktreeRoots() + registeredWorktreeRootsDirty = true + return true +} + export function registerWorktreeRootsForRepo( store: Store, repo: Repo | string, worktreeRoots: string[] ): void { - const repos = synchronizeOwners(store) - const key = resolveWorktreeRootOwner(repos, repo, currentOwners) - const owner = key === undefined ? undefined : registeredOwners.get(key) + const owner = resolveOwnerForRepo(store, repo) if (!owner) { return } @@ -152,9 +185,7 @@ export function registerCreatedWorktreeRoot( repo: Repo | string, worktreeRoot: string ): void { - const repos = synchronizeOwners(store) - const key = resolveWorktreeRootOwner(repos, repo, currentOwners) - const owner = key === undefined ? undefined : registeredOwners.get(key) + const owner = resolveOwnerForRepo(store, repo) if (!owner) { return } @@ -189,7 +220,7 @@ export async function ensureAuthorizedRootsCache(store: Store): Promise { // Follow one superseded refresh; continuous catalog churn must not pin authorization forever. for (let attempt = 0; registeredWorktreeRootsDirty && attempt < 2; attempt++) { if (!registeredWorktreeRootsRefresh) { - registeredWorktreeRootsRefresh = rebuildAuthorizedRootsCache(store).finally(() => { + registeredWorktreeRootsRefresh = rebuildAuthorizedRootsCache(store, true).finally(() => { registeredWorktreeRootsRefresh = null }) } diff --git a/src/main/ipc/registered-worktree-roots-scoped-invalidation.test.ts b/src/main/ipc/registered-worktree-roots-scoped-invalidation.test.ts new file mode 100644 index 00000000000..993bbcb34ae --- /dev/null +++ b/src/main/ipc/registered-worktree-roots-scoped-invalidation.test.ts @@ -0,0 +1,105 @@ +import { resolve } from 'node:path' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Store } from '../persistence' +import type { Repo } from '../../shared/repo-types' + +const mocks = vi.hoisted(() => ({ graph: vi.fn(), stat: vi.fn(), realpath: vi.fn() })) +vi.mock('node:fs', () => ({ realpathSync: (path: string) => path, statSync: mocks.stat })) +vi.mock('node:fs/promises', () => ({ stat: mocks.stat, realpath: mocks.realpath })) +vi.mock('../repo-worktrees', () => ({ + listRepoWorktreeGraph: mocks.graph, + isRepoRoot: vi.fn(() => false) +})) +vi.mock('./worktree-logic', () => ({ + computeWorkspaceRoot: vi.fn(), + getWorktreePathSettings: vi.fn() +})) +vi.mock('../project-runtime-git-options', () => ({ + getWorktreeMirrorDistroForRuntime: vi.fn(), + resolveLocalProjectRuntimesForRepos: vi.fn() +})) +import { + invalidateAuthorizedRootsCache, + rebuildAuthorizedRootsCache +} from './registered-worktree-roots-cache' +import { invalidateAuthorizedRootsCacheForRepo } from './registered-worktree-roots-scoped-invalidation' + +const REPO_COUNT = 12 +const repos: Repo[] = Array.from({ length: REPO_COUNT }, (_, index) => ({ + id: `repo-${index}`, + path: resolve(`/scoped-invalidation-${index}`), + displayName: `repo-${index}`, + badgeColor: '#000', + addedAt: 0 +})) + +function fixture(): Store { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the authorization cache only reads these four store methods; filesystem and graph boundaries are mocked. + return { + getRepos: () => repos, + getProjectGroups: () => [], + getFolderWorkspaces: () => [], + getSettings: () => ({}) + } as unknown as Store +} + +function listedRepoPaths(): string[] { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: this file installs the listRepoWorktreeGraph mock, so every call's first argument is the Repo it was handed. + return mocks.graph.mock.calls.map((call) => (call[0] as Repo).path) +} + +describe('authorized-roots invalidation scope', () => { + let store: Store + + beforeEach(async () => { + mocks.graph.mockReset() + mocks.graph.mockImplementation((repo: Repo) => + Promise.resolve([{ path: resolve(repo.path, 'wt') }]) + ) + mocks.stat.mockReset() + mocks.stat.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + store = fixture() + // Prime every owner so a later scoped invalidation has clean state to dirty. + invalidateAuthorizedRootsCache() + await rebuildAuthorizedRootsCache(store) + mocks.graph.mockClear() + }) + + /** + * The regression this pins: the global form dirties every owner, so one worktree + * mutation cost a `git worktree list` per registered repo — ~58 spawns and seconds + * of git wall-clock on a real fleet, to rediscover roots only one repo changed. + */ + it('relists only the mutated repo', async () => { + invalidateAuthorizedRootsCacheForRepo(store, repos[3]) + await rebuildAuthorizedRootsCache(store, true) + + expect(listedRepoPaths()).toEqual([repos[3].path]) + }) + + it('still relists every repo for a change of unknown scope', async () => { + invalidateAuthorizedRootsCache() + await rebuildAuthorizedRootsCache(store, true) + + expect(listedRepoPaths()).toHaveLength(REPO_COUNT) + }) + + it('re-lists everything for an explicit rebuild, which callers use to force a refresh', async () => { + await rebuildAuthorizedRootsCache(store) + + expect(listedRepoPaths()).toHaveLength(REPO_COUNT) + }) + + it('falls back to a global relist when the repo has no known owner', async () => { + // An unregistered repo must not silently skip invalidation and leave a stale + // allowlist, so the global form is the safe fallback. + invalidateAuthorizedRootsCacheForRepo(store, { + ...repos[0], + id: 'unregistered', + path: resolve('/scoped-invalidation-unregistered') + }) + await rebuildAuthorizedRootsCache(store, true) + + expect(listedRepoPaths()).toHaveLength(REPO_COUNT) + }) +}) diff --git a/src/main/ipc/registered-worktree-roots-scoped-invalidation.ts b/src/main/ipc/registered-worktree-roots-scoped-invalidation.ts new file mode 100644 index 00000000000..39efe7e9194 --- /dev/null +++ b/src/main/ipc/registered-worktree-roots-scoped-invalidation.ts @@ -0,0 +1,29 @@ +import type { Repo } from '../../shared/repo-types' +import type { Store } from '../persistence' +import { + invalidateAuthorizedRootsCache, + markAuthorizedRootsOwnerDirty +} from './registered-worktree-roots-cache' + +/** + * Scoped counterpart to `invalidateAuthorizedRootsCache` for a change whose blast + * radius is provably one repo — every worktree create and removal. + * + * Why this exists: the global form dirties every registered owner, so the next + * authorization-requiring IPC rebuilds by listing EVERY repo. At 58 repos that is 58 + * `git worktree list` spawns — roughly ten seconds of git wall-clock through an + * admission budget of four — to rediscover roots only one repo changed. + * + * Falls back to the global form rather than silently skipping an invalidation, which + * would leave a stale allowlist. Callers whose change can alter the owner SET rather + * than one repo's roots (store swap, execution-host/WSL re-routing, nested-repo + * import, folder->git upgrade) must keep using the global form. + */ +export function invalidateAuthorizedRootsCacheForRepo( + store: Store | null | undefined, + repo: Repo | string +): void { + if (!store || !markAuthorizedRootsOwnerDirty(store, repo)) { + invalidateAuthorizedRootsCache() + } +} diff --git a/src/main/ipc/repos-picker.test.ts b/src/main/ipc/repos-picker.test.ts index 14b8bbd0b68..7f7c3a3976d 100644 --- a/src/main/ipc/repos-picker.test.ts +++ b/src/main/ipc/repos-picker.test.ts @@ -28,7 +28,8 @@ vi.mock('../git/repo', () => ({ })) vi.mock('./registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../providers/ssh-git-dispatch', () => ({ diff --git a/src/main/ipc/repos-remote-client-events.test.ts b/src/main/ipc/repos-remote-client-events.test.ts index af8bf4ecf28..adf2cc2f17b 100644 --- a/src/main/ipc/repos-remote-client-events.test.ts +++ b/src/main/ipc/repos-remote-client-events.test.ts @@ -35,7 +35,10 @@ vi.mock('../git/repo', () => ({ filterBaseRefSearchOutput: vi.fn().mockReturnValue([]) })) -vi.mock('./registered-worktree-roots-cache', () => ({ invalidateAuthorizedRootsCache: vi.fn() })) +vi.mock('./registered-worktree-roots-cache', () => ({ + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() +})) vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: vi.fn() })) vi.mock('./ssh', () => ({ getActiveMultiplexer: vi.fn() })) diff --git a/src/main/ipc/repos-sparse-presets.test.ts b/src/main/ipc/repos-sparse-presets.test.ts index 5f7e7202e73..31843858c05 100644 --- a/src/main/ipc/repos-sparse-presets.test.ts +++ b/src/main/ipc/repos-sparse-presets.test.ts @@ -43,7 +43,8 @@ vi.mock('../git/repo', () => ({ })) vi.mock('./registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../providers/ssh-git-dispatch', () => ({ diff --git a/src/main/ipc/repos/repo-creation-git-availability.test.ts b/src/main/ipc/repos/repo-creation-git-availability.test.ts index 8ffbbdc033e..fb112952470 100644 --- a/src/main/ipc/repos/repo-creation-git-availability.test.ts +++ b/src/main/ipc/repos/repo-creation-git-availability.test.ts @@ -15,7 +15,8 @@ vi.mock('../../worktree-root-preparation', () => ({ prepareLocalWorktreeRootForRepo: vi.fn(async () => {}) })) vi.mock('../registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('./repo-added-telemetry', () => ({ emitRepoAdded: vi.fn() })) vi.mock('./repos-changed-notification', () => ({ notifyReposChanged: vi.fn() })) diff --git a/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts b/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts index 9d88143d3c4..6f4a95d5d42 100644 --- a/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts +++ b/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts @@ -7,7 +7,8 @@ vi.mock('../../worktree-remote', () => ({ notifyWorktreesChanged: vi.fn() })) vi.mock('../../registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('./worktree-removal-ownership', () => ({ removeWorktreeMetadataAndTransientState: vi.fn(), diff --git a/src/main/pty/posix-pty-foreground-group.ts b/src/main/pty/posix-pty-foreground-group.ts index e9df7baeb66..e558f9ce156 100644 --- a/src/main/pty/posix-pty-foreground-group.ts +++ b/src/main/pty/posix-pty-foreground-group.ts @@ -1,4 +1,5 @@ import { execFileSync } from 'node:child_process' +import { notifySpawnObserver } from '../../shared/child-process/spawn-observer' const PROCESS_TABLE_LOOKUP_TIMEOUT_MS = 250 const PROCESS_TABLE_QUERY_TIMEOUT_MS = PROCESS_TABLE_LOOKUP_TIMEOUT_MS / 2 @@ -26,11 +27,18 @@ function hasUsableTty(tty: string): boolean { } function runPs(pid: number): string { - return execFileSync('ps', ['-p', String(pid), '-o', 'pid=,tpgid=,tty='], { - encoding: 'utf8', - timeout: PROCESS_TABLE_QUERY_TIMEOUT_MS, - maxBuffer: PROCESS_TABLE_MAX_BYTES - }) + const args = ['-p', String(pid), '-o', 'pid=,tpgid=,tty='] + const startedAt = performance.now() + try { + return execFileSync('ps', args, { + encoding: 'utf8', + timeout: PROCESS_TABLE_QUERY_TIMEOUT_MS, + maxBuffer: PROCESS_TABLE_MAX_BYTES + }) + } finally { + // Diagnostics only: execFileSync blocks the caller for the child's whole life. + notifySpawnObserver('ps', args, performance.now() - startedAt) + } } let ownRowCache: { pid: number; row: string } | null = null diff --git a/src/main/runtime/fit-override-integration.test.ts b/src/main/runtime/fit-override-integration.test.ts index e051ca7ae69..7272d01ed7b 100644 --- a/src/main/runtime/fit-override-integration.test.ts +++ b/src/main/runtime/fit-override-integration.test.ts @@ -31,7 +31,8 @@ vi.mock('../ipc/worktree-logic', async (importOriginal) => { }) vi.mock('../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../git/repo', async (importOriginal) => { diff --git a/src/main/runtime/mobile-presence-lock.test.ts b/src/main/runtime/mobile-presence-lock.test.ts index 7cf24f40534..c26ede60b43 100644 --- a/src/main/runtime/mobile-presence-lock.test.ts +++ b/src/main/runtime/mobile-presence-lock.test.ts @@ -19,7 +19,8 @@ vi.mock('../ipc/worktree-logic', async (importOriginal) => { return { ...actual, computeWorktreePath: vi.fn(), ensurePathWithinWorkspace: vi.fn() } }) vi.mock('../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../git/repo', async (importOriginal) => { const actual = (await importOriginal()) as Record diff --git a/src/main/runtime/mobile-session-tabs-churn-coalescing.test.ts b/src/main/runtime/mobile-session-tabs-churn-coalescing.test.ts index 7fe33b66ee1..1984c8bd488 100644 --- a/src/main/runtime/mobile-session-tabs-churn-coalescing.test.ts +++ b/src/main/runtime/mobile-session-tabs-churn-coalescing.test.ts @@ -20,7 +20,8 @@ vi.mock('../ipc/worktree-logic', async (importOriginal) => { }) vi.mock('../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../git/repo', async (importOriginal) => { diff --git a/src/main/runtime/mobile-subscribe-integration.test.ts b/src/main/runtime/mobile-subscribe-integration.test.ts index b3be748d5f0..c57a52f5f17 100644 --- a/src/main/runtime/mobile-subscribe-integration.test.ts +++ b/src/main/runtime/mobile-subscribe-integration.test.ts @@ -30,7 +30,8 @@ vi.mock('../ipc/worktree-logic', async (importOriginal) => { }) vi.mock('../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../git/repo', async (importOriginal) => { diff --git a/src/main/runtime/orca-runtime-create-managed-worktree.ts b/src/main/runtime/orca-runtime-create-managed-worktree.ts index e7fb519239a..d0885a1dfe1 100644 --- a/src/main/runtime/orca-runtime-create-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-create-managed-worktree.ts @@ -10,7 +10,7 @@ import { createRuntimeFolderWorktree } from './runtime-folder-worktree-create' import { createRuntimeLocalManagedWorktree } from './runtime-local-worktree-create' import type { PreparationRearmHolder } from '../worktree-create-preparation' import { prepareRuntimeLocalWorktreeSetup } from './runtime-local-worktree-setup' -import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' +import { invalidateAuthorizedRootsCacheForRepo } from '../ipc/filesystem-auth' import { startRuntimeLocalWorktreeTerminals } from './runtime-local-worktree-terminal-startup' export class OrcaRuntimeWithCreateManagedWorktree extends OrcaRuntimeWithGetWorktreeTerminalProvisioningHost { @@ -211,7 +211,11 @@ export class OrcaRuntimeWithCreateManagedWorktree extends OrcaRuntimeWithGetWork // to authorize paths. Without invalidating it here, CLI-created worktrees // are not recognized and all git operations fail with "Access denied: // unknown repository or worktree path". - invalidateAuthorizedRootsCache() + // Scoped to this repo: the global form dirties every owner, so the next + // authorization-requiring IPC relists EVERY registered repo — ~58 `git worktree + // list` spawns, ~10s of git wall-clock through an admission budget of 4, to + // rediscover roots only this repo changed. + invalidateAuthorizedRootsCacheForRepo(this.store, repo) this.notifyWorktreesChanged(repo.id) const { diff --git a/src/main/runtime/orca-runtime-test-mocks/setup.spec.ts b/src/main/runtime/orca-runtime-test-mocks/setup.spec.ts index c25dc33df87..30d6c61ff79 100644 --- a/src/main/runtime/orca-runtime-test-mocks/setup.spec.ts +++ b/src/main/runtime/orca-runtime-test-mocks/setup.spec.ts @@ -413,12 +413,14 @@ vi.mock('../../ipc/worktree-logic', async (importOriginal) => { vi.mock('../../ipc/filesystem-auth', () => ({ resolveAuthorizedPath: vi.fn(async (pathValue: string) => pathValue), invalidateAuthorizedRootsCache: invalidateAuthorizedRootsCacheMock, + invalidateAuthorizedRootsCacheForRepo: invalidateAuthorizedRootsCacheMock, isENOENT: (error: unknown) => Boolean(error && typeof error === 'object' && 'code' in error && error.code === 'ENOENT') })) vi.mock('../../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: invalidateAuthorizedRootsCacheMock + invalidateAuthorizedRootsCache: invalidateAuthorizedRootsCacheMock, + invalidateAuthorizedRootsCacheForRepo: invalidateAuthorizedRootsCacheMock })) // Why: the real check also matches relay-rebuilt errors, which carry only the ENOENT message. diff --git a/src/main/runtime/remote-desktop-driver.test.ts b/src/main/runtime/remote-desktop-driver.test.ts index 5271963a598..f8f3fd12037 100644 --- a/src/main/runtime/remote-desktop-driver.test.ts +++ b/src/main/runtime/remote-desktop-driver.test.ts @@ -29,7 +29,8 @@ vi.mock('../ipc/worktree-logic', async (importOriginal) => { return { ...actual, computeWorktreePath: vi.fn(), ensurePathWithinWorkspace: vi.fn() } }) vi.mock('../ipc/registered-worktree-roots-cache', () => ({ - invalidateAuthorizedRootsCache: vi.fn() + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() })) vi.mock('../git/repo', async (importOriginal) => { const actual = (await importOriginal()) as Record diff --git a/src/main/runtime/runtime-local-create-rearm-ordering.test.ts b/src/main/runtime/runtime-local-create-rearm-ordering.test.ts index 27294c144c3..caf4dfb109f 100644 --- a/src/main/runtime/runtime-local-create-rearm-ordering.test.ts +++ b/src/main/runtime/runtime-local-create-rearm-ordering.test.ts @@ -33,7 +33,10 @@ vi.mock('./runtime-local-worktree-setup', () => ({ })) })) -vi.mock('../ipc/filesystem-auth', () => ({ invalidateAuthorizedRootsCache: vi.fn() })) +vi.mock('../ipc/filesystem-auth', () => ({ + invalidateAuthorizedRootsCache: vi.fn(), + invalidateAuthorizedRootsCacheForRepo: vi.fn() +})) import { OrcaRuntimeService } from './orca-runtime' diff --git a/src/main/startup/main-process-preflight.ts b/src/main/startup/main-process-preflight.ts index 2e143dbb703..0a9afbd8079 100644 --- a/src/main/startup/main-process-preflight.ts +++ b/src/main/startup/main-process-preflight.ts @@ -35,7 +35,12 @@ import { getDevInstanceIdentity, shouldApplyPreReadyAppName } from './dev-instan import { enableRendererHeapHeadroom } from './renderer-heap-headroom' import { isStartupDiagnosticsEnabled, logStartupDiagnostic } from './startup-diagnostics' import { startEventLoopStallProbe } from './event-loop-stall-probe' -import { startMainThreadChurnProbe } from '../diagnostics/main-thread-churn-probe' +import { + isMainThreadDiagnosticsEnabled, + recordSubprocessSpawn, + startMainThreadChurnProbe +} from '../diagnostics/main-thread-churn-probe' +import { setSpawnObserver } from '../../shared/child-process/spawn-observer' import { settledDiffCache } from '../git/source-control/git-read-cache-invalidation' import { reserveServeStdoutForReadiness } from '../server/serve-stdout-boundary' import { createServeDesktopActivationGate } from './serve-desktop-activation' @@ -222,6 +227,11 @@ function initializeMainProcessPreflight(options: MainProcessPreflightOptions): b // Self-gated on ORCA_MAIN_THREAD_DIAGNOSTICS; runs the whole session to catch steady-state churn (issue #7576). // Why the diff-cache counters ride along: a stamp the filesystem reports unstably makes the cache // look exactly like a cold start, and only the hit/miss/unprovable split tells the two apart. + if (isMainThreadDiagnosticsEnabled()) { + // Why here too: the probe's own call sites only cover src/main/git, so without + // this every spawnProcess/runProcess child (rg, ps, pty helpers) is invisible. + setSpawnObserver(recordSubprocessSpawn) + } startMainThreadChurnProbe({ extraStats: () => ({ diffCache: settledDiffCache.stats() }) }) // Why: acquire AFTER configureDevUserDataPath — Electron derives lock identity from `userData`, so dev/packaged lock in separate namespaces. // Why dev locks too: two processes on one profile corrupt its stores (PR #1326 / #1312); parallel `pnpm dev` needs ORCA_DEV_USER_DATA_PATH per copy. diff --git a/src/shared/child-process/run-process.ts b/src/shared/child-process/run-process.ts index 736b093a56f..ec0a3026cc5 100644 --- a/src/shared/child-process/run-process.ts +++ b/src/shared/child-process/run-process.ts @@ -7,6 +7,7 @@ import { import { resolveSpawn } from './spawn-resolution' import { forceTerminateProcessTree, signalProcessTree } from './process-tree-termination' +import { hasSpawnObserver, notifySpawnObserver } from './spawn-observer' import { createOutputSink } from './bounded-output-sink' import { createChildTerminationReporter } from './child-termination-reporter' @@ -50,11 +51,19 @@ const BARRIER_UNVERIFIED_EXIT_GRACE_MS = 10_000 */ export function spawnProcess(spec: ProcessSpec): ChildProcessWithoutNullStreams { const resolved = resolveSpawn(spec, process.platform) - return nodeSpawn( + // Diagnostics only: uv_spawn runs synchronously on the calling thread, so this + // brackets the main-thread block for every child started through the wrapper. + const spawnStartedAt = hasSpawnObserver() ? performance.now() : null + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: resolveSpawn never sets stdio to 'ignore'/'inherit' for a stream slot, so stdin/stdout/stderr are always pipes. + const child = nodeSpawn( resolved.file, [...resolved.args], resolved.options ) as ChildProcessWithoutNullStreams + if (spawnStartedAt !== null) { + notifySpawnObserver(resolved.file, resolved.args, performance.now() - spawnStartedAt) + } + return child } /** diff --git a/src/shared/child-process/spawn-observer.test.ts b/src/shared/child-process/spawn-observer.test.ts new file mode 100644 index 00000000000..6700fb1b351 --- /dev/null +++ b/src/shared/child-process/spawn-observer.test.ts @@ -0,0 +1,39 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { setSpawnObserver } from './spawn-observer' +import { runProcess, spawnProcess } from './run-process' + +afterEach(() => { + setSpawnObserver(null) +}) + +describe('spawn observer', () => { + it('reports the resolved program for a spawnProcess child', async () => { + const seen: { command: string; args: readonly string[]; blockMs: number }[] = [] + setSpawnObserver((command, args, blockMs) => seen.push({ command, args, blockMs })) + const child = spawnProcess({ program: process.execPath, args: ['-e', 'process.exit(0)'] }) + await new Promise((resolve) => child.on('exit', resolve)) + expect(seen).toHaveLength(1) + expect(seen[0].command).toBe(process.execPath) + expect(seen[0].args).toEqual(['-e', 'process.exit(0)']) + expect(seen[0].blockMs).toBeGreaterThanOrEqual(0) + }) + + it('reports exactly once for runProcess, which spawns through spawnProcess', async () => { + let calls = 0 + setSpawnObserver(() => { + calls += 1 + }) + await runProcess({ program: process.execPath, args: ['-e', 'process.exit(0)'] }) + expect(calls).toBe(1) + }) + + it('stays silent with no observer registered', async () => { + let calls = 0 + setSpawnObserver(null) + await runProcess({ program: process.execPath, args: ['-e', 'process.exit(0)'] }) + setSpawnObserver(() => { + calls += 1 + }) + expect(calls).toBe(0) + }) +}) diff --git a/src/shared/child-process/spawn-observer.ts b/src/shared/child-process/spawn-observer.ts new file mode 100644 index 00000000000..086d46a7af8 --- /dev/null +++ b/src/shared/child-process/spawn-observer.ts @@ -0,0 +1,28 @@ +/** + * Diagnostic seam for counting every child process started through this + * package. `src/main/diagnostics/main-thread-churn-probe` registers itself here + * under ORCA_MAIN_THREAD_DIAGNOSTICS=1; with nothing registered every spawn + * pays one undefined check, so production behaviour is unchanged. + * + * Why a seam rather than a direct import: run-process.ts also runs in the + * terminal daemon, the relay and the CLI, none of which may depend on main. + */ +export type SpawnObserver = (command: string, args: readonly string[], blockMs: number) => void + +let observer: SpawnObserver | null = null + +export function setSpawnObserver(next: SpawnObserver | null): void { + observer = next +} + +export function notifySpawnObserver( + command: string, + args: readonly string[], + blockMs: number +): void { + observer?.(command, args, blockMs) +} + +export function hasSpawnObserver(): boolean { + return observer !== null +} diff --git a/tests/e2e/main-thread-git-cost.spec.ts b/tests/e2e/main-thread-git-cost.spec.ts new file mode 100644 index 00000000000..53784cfe779 --- /dev/null +++ b/tests/e2e/main-thread-git-cost.spec.ts @@ -0,0 +1,225 @@ +/** + * Diagnostic-only measurement: how much of main's event loop does git + * subprocess orchestration actually consume, and which phase dominates? + * + * Why a separate spec from terminal-multi-workspace-typing-latency: that bench + * measures typing latency UNDER churn, so its numbers mix PTY relay, renderer + * paint and git. This one runs git churn alone against a hidden window and + * takes a V8 CPU profile OF THE MAIN PROCESS for each phase, which is the only + * way to split spawn initiation from stdout drain from porcelain parsing — the + * churn probe reports spawn initiation only. + * + * Run: ORCA_GIT_COST_BENCH=1 ORCA_BACKGROUND_LAUNCH=1 \ + * npx playwright test tests/e2e/main-thread-git-cost.spec.ts \ + * --config tests/playwright.config.ts --project electron-headless --workers=1 + */ +import { expect, test } from './helpers/orca-app' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { + createGitChurnRepos, + registerGitChurnRepos, + startGitChurnLoad, + stopGitChurnLoad, + type GitChurnStats +} from './git-churn-load' + +type MainThreadReport = { + t: number + maxGapMs?: number + gapsOver50Ms?: number + gapsOver250Ms?: number + spawnCount?: number + spawns?: Record + marker?: string + wallMs: number +} + +type Phase = { label: string; repos: number; concurrency: number; seconds: number } + +const FILES_PER_REPO = Number(process.env.ORCA_GIT_COST_FILES ?? 400) +const PHASE_SECONDS = Number(process.env.ORCA_GIT_COST_PHASE_SECONDS ?? 20) +const MAX_REPOS = Number(process.env.ORCA_GIT_COST_REPOS ?? 24) + +const PHASES: Phase[] = [ + { label: 'idle', repos: 0, concurrency: 0, seconds: PHASE_SECONDS }, + { label: 'c1', repos: MAX_REPOS, concurrency: 1, seconds: PHASE_SECONDS }, + { label: 'c4', repos: MAX_REPOS, concurrency: 4, seconds: PHASE_SECONDS }, + { label: 'c8', repos: MAX_REPOS, concurrency: 8, seconds: PHASE_SECONDS }, + { label: 'c16', repos: MAX_REPOS, concurrency: 16, seconds: PHASE_SECONDS }, + { label: 'c32', repos: MAX_REPOS, concurrency: 32, seconds: PHASE_SECONDS } +] + +test.use({ + orcaAppExtraEnv: { ORCA_MAIN_THREAD_DIAGNOSTICS: '1', ORCA_BACKGROUND_LAUNCH: '1' } +}) + +test.describe('main-thread git orchestration cost', () => { + test.skip( + process.env.ORCA_GIT_COST_BENCH !== '1', + 'Diagnostic bench; set ORCA_GIT_COST_BENCH=1 to run' + ) + test.setTimeout((PHASES.reduce((sum, phase) => sum + phase.seconds, 0) + 600) * 1000) + + test('measures spawn / drain / parse split under git churn', async ({ + orcaPage, + electronApp + }, testInfo) => { + const reports: MainThreadReport[] = [] + const stderr = electronApp.process().stderr + expect(stderr, 'electron stderr must be piped for the churn probe').toBeTruthy() + let buffer = '' + stderr?.setEncoding('utf-8') + stderr?.on('data', (chunk: string) => { + buffer += chunk + let index = buffer.indexOf('\n') + while (index !== -1) { + const line = buffer.slice(0, index).trimEnd() + buffer = buffer.slice(index + 1) + index = buffer.indexOf('\n') + const match = /^\[main-thread\] (\{.*\})$/.exec(line) + if (!match) { + continue + } + try { + reports.push({ ...JSON.parse(match[1]), wallMs: Date.now() }) + } catch { + // ignore malformed probe line + } + } + }) + + const churnRoot = mkdtempSync(path.join(tmpdir(), 'orca-git-cost-')) + const profileDir = path.join(testInfo.outputDir, 'main-profiles') + mkdirSync(profileDir, { recursive: true }) + const results: Record[] = [] + try { + const repos = createGitChurnRepos(churnRoot, MAX_REPOS, FILES_PER_REPO) + const registration = await registerGitChurnRepos( + orcaPage, + repos.map((repo) => repo.path) + ) + expect( + registration, + `churn repos were not registered: ${registration.failures.join('; ')}` + ).toMatchObject({ registered: repos.length }) + + for (const phase of PHASES) { + const profilePath = path.join(profileDir, `main-${phase.label}.cpuprofile`) + await startMainProfiler(electronApp) + const startedAt = Date.now() + if (phase.concurrency > 0) { + await startGitChurnLoad( + orcaPage, + repos.slice(0, phase.repos).map((repo) => repo.path), + { concurrency: phase.concurrency, admissionTier: 'status' } + ) + } + await new Promise((resolve) => setTimeout(resolve, phase.seconds * 1000)) + const churn: GitChurnStats | null = + phase.concurrency > 0 ? await stopGitChurnLoad(orcaPage) : null + const endedAt = Date.now() + const wallMs = await stopMainProfiler(electronApp, profilePath) + // Skip the first window: it straddles the previous phase's tail. + const phaseReports = reports.filter( + (report) => + !report.marker && report.wallMs >= startedAt + 5_000 && report.wallMs <= endedAt + 1_000 + ) + results.push({ + phase: phase.label, + repos: phase.repos, + concurrency: phase.concurrency, + windowMs: endedAt - startedAt, + profileWallMs: wallMs, + reportCount: phaseReports.length, + maxGapMs: Math.max(0, ...phaseReports.map((report) => report.maxGapMs ?? 0)), + gapsOver50Ms: sum(phaseReports.map((report) => report.gapsOver50Ms ?? 0)), + gapsOver250Ms: sum(phaseReports.map((report) => report.gapsOver250Ms ?? 0)), + spawnCount: sum(phaseReports.map((report) => report.spawnCount ?? 0)), + spawns: mergeSpawns(phaseReports), + churn, + profilePath + }) + console.log(`[git-cost] ${JSON.stringify(results.at(-1))}`) + } + } finally { + await stopGitChurnLoad(orcaPage).catch(() => null) + rmSync(churnRoot, { recursive: true, force: true }) + } + const summaryPath = path.join(profileDir, 'summary.json') + writeFileSync(summaryPath, JSON.stringify({ filesPerRepo: FILES_PER_REPO, results }, null, 2)) + console.log(`[git-cost] summary=${summaryPath}`) + expect(readFileSync(summaryPath, 'utf8').length).toBeGreaterThan(0) + }) +}) + +function sum(values: number[]): number { + return values.reduce((total, value) => total + value, 0) +} + +function mergeSpawns( + reports: MainThreadReport[] +): Record { + const merged: Record = {} + for (const report of reports) { + for (const [key, stats] of Object.entries(report.spawns ?? {})) { + const entry = (merged[key] ??= { count: 0, blockMsTotal: 0, blockMsMax: 0 }) + entry.count += stats.count + entry.blockMsTotal = Math.round((entry.blockMsTotal + stats.blockMsTotal) * 100) / 100 + entry.blockMsMax = Math.max(entry.blockMsMax, stats.blockMsMax) + } + } + return merged +} + +/** V8 CPU profile of the MAIN process, taken from inside main via node:inspector. */ +async function startMainProfiler(electronApp: { + evaluate: (fn: (electron: unknown, arg: A) => R | Promise, arg: A) => Promise +}): Promise { + await electronApp.evaluate(async (_electron, intervalUs: number) => { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: diagnostic-only scratch globals owned by this bench. + const scope = globalThis as unknown as Record + const inspector = process.getBuiltinModule('node:inspector') + const session = new inspector.Session() + session.connect() + type InspectorParams = { interval?: number } + const post = (method: string, params?: InspectorParams): Promise> => + new Promise((resolve, reject) => { + session.post(method, params, (error, result) => + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: node:inspector types the callback payload as unknown; every Profiler reply is an object. + error ? reject(error) : resolve(result as Record) + ) + }) + await post('Profiler.enable') + await post('Profiler.setSamplingInterval', { interval: intervalUs }) + await post('Profiler.start') + scope.__orcaGitCostProfiler = { session, post, startedAt: Date.now() } + }, 500) +} + +async function stopMainProfiler( + electronApp: { + evaluate: (fn: (electron: unknown, arg: A) => R | Promise, arg: A) => Promise + }, + outPath: string +): Promise { + return electronApp.evaluate(async (_electron, target: string) => { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: same bench-owned global installed above. + const scope = globalThis as unknown as Record + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the shape startMainProfiler stored on this same global, in this same process. + const handle = scope.__orcaGitCostProfiler as { + session: { disconnect: () => void } + post: (method: string, params?: { interval?: number }) => Promise> + startedAt: number + } + const { profile } = await handle.post('Profiler.stop') + const wallMs = Date.now() - handle.startedAt + process + .getBuiltinModule('node:fs') + .writeFileSync(target, JSON.stringify(profile), { mode: 0o600 }) + handle.session.disconnect() + delete scope.__orcaGitCostProfiler + return wallMs + }, outPath) +} diff --git a/tests/tools/benchmarks/analyze-main-cpuprofile.mjs b/tests/tools/benchmarks/analyze-main-cpuprofile.mjs new file mode 100644 index 00000000000..a688957a0d5 --- /dev/null +++ b/tests/tools/benchmarks/analyze-main-cpuprofile.mjs @@ -0,0 +1,213 @@ +#!/usr/bin/env node +/** + * Bucket a MAIN-process .cpuprofile by self time, resolved through out/main's + * source maps, so a minified frame like `_Le` reads as the file it came from. + * + * Why buckets and not just a flame list: the question this answers is whether + * main-thread git cost is spawn initiation + stdout drain (which a utility + * process removes) or result parsing + IPC (which it does not). + * + * Why the nearest-mapping fallback: esbuild emits source-less segments, and an + * exact-position lookup returns null for them — which silently dumped a third + * of the busy samples into an "unknown bundle" bucket on the first pass. + * + * Usage: node tests/tools/benchmarks/analyze-main-cpuprofile.mjs [--top 30] + */ +import { readFileSync } from 'node:fs' +import { dirname, resolve } from 'node:path' +import { TraceMap, decodedMappings } from '@jridgewell/trace-mapping' + +const args = process.argv.slice(2) +const profilePath = args.find((arg) => !arg.startsWith('--')) +if (!profilePath) { + throw new Error('Usage: analyze-main-cpuprofile.mjs [--top N]') +} +const topIndex = args.indexOf('--top') +const top = topIndex === -1 ? 30 : Number(args[topIndex + 1]) + +const profile = JSON.parse(readFileSync(profilePath, 'utf8')) +const indexCache = new Map() + +/** Per generated line: columns that carry a source, ascending, for a lower-bound scan. */ +function mappingIndexFor(url) { + if (indexCache.has(url)) { + return indexCache.get(url) + } + let built = null + if (url.startsWith('file://')) { + const filePath = new URL(url).pathname + try { + const map = new TraceMap(JSON.parse(readFileSync(`${filePath}.map`, 'utf8'))) + const lines = decodedMappings(map) + built = { + dir: dirname(filePath), + sources: map.sources, + names: map.names, + lines: lines.map((segments) => segments.filter((segment) => segment.length >= 4)) + } + } catch { + built = null + } + } + indexCache.set(url, built) + return built +} + +function sourceRelative(dir, source) { + const absolute = resolve(dir, source) + const marker = absolute.indexOf('/src/') + return marker === -1 ? absolute : absolute.slice(marker + 1) +} + +/** Source-relative origin of a frame, e.g. "src/main/git/status/porcelain.ts:parseEntry". */ +function originOf(frame) { + if (!frame.url) { + return frame.functionName ? `` : '' + } + const index = mappingIndexFor(frame.url) + if (!index) { + return frame.url + } + const segments = index.lines[frame.lineNumber] ?? [] + let chosen = null + for (const segment of segments) { + if (segment[0] > frame.columnNumber) { + break + } + chosen = segment + } + if (!chosen) { + return `${frame.url}#unmapped:${frame.lineNumber}:${frame.columnNumber}` + } + const file = sourceRelative(index.dir, index.sources[chosen[1]] ?? '?') + const name = + (chosen.length >= 5 ? index.names[chosen[4]] : null) ?? frame.functionName ?? '(anon)' + return `${file}:${name}` +} + +/** + * Which cost phase a frame belongs to, matched on the V8 function name first. + * + * Why function name and not the resolved source file: the source map's + * greatest-lower-bound lookup lands on the *preceding* mapped segment often + * enough that file attribution alone put env builders in the diagnostics probe. + * With ORCA_UNMINIFIED_MAIN=1 the function name is the real one, so it is the + * trustworthy key; the file is kept only as a display hint. + * + * `spawn` (no url) is the native process_wrap binding — the synchronous + * posix_spawn that actually holds the main thread. + */ +const NAME_RULES = [ + [/^\((?:idle|program)\)$/, 'idle/program'], + [/^\(garbage collector\)$/, 'gc'], + // Env construction exists only to hand a git child its environment, so it is + // spawn preparation: it moves wherever the spawn moves. + [ + /^(?:spawn|spawnSync|normalizeSpawnArguments|validateArgumentNullCheck|validateTimeout|ChildProcess|setupChannel|execFile|execFileSync|spawnWithSignal)$/, + 'spawn-init' + ], + [ + /Env$|^untranslatedGitOutputEnv$|^resolveSpawn$|^spawnProcess$|^runProcess$|^getSpawnArgsForWindows$/, + 'spawn-init' + ], + [ + /^(?:onStreamRead|Socket|readableAddChunk|emitReadable|onread|write|flow|resume_|StringDecoder|utf8Write|text|createOutputSink)$/, + 'stdout-drain' + ], + [/^(?:parse|attachLineStats|createInputIdentity|runGetStatus|detect|read)/, 'git-parse'], + [/Porcelain|Numstat|ChangedEntry|StatusEntry/i, 'git-parse'], + [ + /^(?:hydrateRepo|getWorktreeRootOwnerKey|getLocalWorktreeRootOwners|collectRepoIdsWithRegisteredWorktreeMeta|resolveRegisteredWorktreePath|getRepos|normalizeString|join|resolve|relative|isAbsolute)/, + 'store+path-orchestration' + ], + [/^(?:sendReply|_invokeHandler|ipc)/i, 'ipc'] +] + +const URL_RULES = [ + [/child_process|spawn-resolution|run-process\.ts/, 'spawn-init'], + [ + /string_decoder|node:internal\/streams|node:stream|node:net|bounded-output-sink/, + 'stdout-drain' + ], + [/src\/main\/git\/|src\/shared\/git|porcelain/, 'git-parse'], + [/electron\/js2c|src\/main\/ipc\//, 'ipc'], + [/node:internal\/fs|node:fs/, 'fs'], + [/src\//, 'other-orca'], + [/node:/, 'other-node'] +] + +function bucketOf(label) { + const name = label.includes(' @') ? label.slice(0, label.indexOf(' @')) : label + for (const [pattern, bucket] of NAME_RULES) { + if (pattern.test(name)) { + return bucket + } + } + for (const [pattern, bucket] of URL_RULES) { + if (pattern.test(label)) { + return bucket + } + } + return 'other' +} + +/** + * Where the frame lives, for display. Bundle frames keep their generated + * position rather than a mapped file: with the mappings this bundle carries, a + * greatest-lower-bound source lookup names the wrong file often enough to + * mislead, and the (unminified) function name already identifies the code. + */ +function displayLocation(frame) { + if (!frame.url) { + return '' + } + if (!frame.url.startsWith('file://')) { + return frame.url + } + const mapped = originOf(frame) + const leaf = frame.url.split('/').at(-1) + return `${leaf}:${frame.lineNumber} ~${typeof mapped === 'string' ? mapped.split('#')[0].split('/').at(-1) : mapped}` +} + +const wallUs = profile.endTime - profile.startTime +const sampleUs = wallUs / profile.samples.length +const selfBySite = new Map() +for (const node of profile.nodes) { + if (!node.hitCount) { + continue + } + const label = `${node.callFrame.functionName || '(anon)'} @${displayLocation(node.callFrame)}` + selfBySite.set(label, (selfBySite.get(label) ?? 0) + node.hitCount) +} +const totalHits = [...selfBySite.values()].reduce((sum, hits) => sum + hits, 0) + +const byBucket = new Map() +for (const [label, hits] of selfBySite) { + const bucket = bucketOf(label) + byBucket.set(bucket, (byBucket.get(bucket) ?? 0) + hits) +} + +const ms = (hits) => Math.round((hits * sampleUs) / 100) / 10 +const busyHits = totalHits - (byBucket.get('idle/program') ?? 0) + +console.log(`profile: ${profilePath}`) +console.log( + `wall ${(wallUs / 1000).toFixed(0)}ms samples ${profile.samples.length} sampleInterval ${sampleUs.toFixed(0)}us` +) +console.log( + `busy (non-idle) main-thread time: ${ms(busyHits)}ms = ${((busyHits / totalHits) * 100).toFixed(1)}% of wall` +) +console.log('\nbuckets (self time):') +for (const [bucket, hits] of [...byBucket].sort((a, b) => b[1] - a[1])) { + const shareOfBusy = + bucket === 'idle/program' ? '' : ` ${((hits / busyHits) * 100).toFixed(1)}% of busy` + console.log( + ` ${bucket.padEnd(24)} ${String(ms(hits)).padStart(8)}ms ${((hits / totalHits) * 100).toFixed(2)}% of wall${shareOfBusy}` + ) +} +console.log(`\ntop ${top} self-time sites:`) +for (const [label, hits] of [...selfBySite].sort((a, b) => b[1] - a[1]).slice(0, top)) { + console.log( + ` ${String(ms(hits)).padStart(8)}ms ${((hits / busyHits) * 100).toFixed(1)}%busy [${bucketOf(label)}] ${label}` + ) +}