Commit Graph
2 Commits
Author SHA1 Message Date
Neil 96565fe370 perf(source-control): stop re-running every git read on each file selection (#15036) (#16600)
* perf(source-control): stop blocking main on four sync git-dir probes per status poll

detectConflictOperation ran four existsSync calls against the git dir on every
status poll. On a `\\wsl.localhost\...` worktree each one is a 9p round trip, and
being synchronous they landed on the Electron main thread back to back.

Replace them with concurrent fs/promises access probes: same "any failure reads
as absent" semantics existsSync had, one wave instead of four serialized blocking
calls. The outer try/catch went with them -- neither resolveGitDir nor the probes
can throw now, so it was unreachable.

Part of #15036 (source-control latency).

* perf(wsl): let git reads take the shell-free route from a cwd-derived distro

shouldAttemptWslDirectGit required options.wslDistro, so a `\\wsl.localhost\...`
worktree without a resolved WSL project runtime never qualified -- even though the
distro is right there in the cwd and wslDistroForCommand already knew how to read
it. Every `git show` behind a diff therefore ran through the user's login shell,
executing their rc once per blob read.

Three changes:

- Derive the distro from the cwd when no override was supplied. This is the fix;
  the routing decision now depends on where the repo actually lives.
- Wait, bounded, for a cold read-environment probe instead of resolving without it.
  The probe is one wsl.exe call shared per distro, so the wait is paid at most once,
  and past WSL_GIT_READ_ENVIRONMENT_WAIT_MS the shell route runs exactly as before.
  It returns null rather than a settled promise when there is nothing to wait for,
  so a non-WSL git call is not pushed into a later microtask.
- Opt the blob reads into preferWslDirectGit via gitReadOptionsForWorktree (renamed
  from gitStatusReadOptionsForWorktree; it was never status-specific). Belt-and-
  braces only: `show`, `config --get-regexp`, `ls-files` and `rev-parse` were all
  already matched by isWslDirectGitReadCommand, so this changes no routing today --
  it just stops the diff path depending on a heuristic it knows the answer to.

git-blob-read also gains a `failed` flag distinguishing "git ran and reported the
path absent" (exit 128) from "the read never got an answer"; nothing consumes it
yet, the settled diff cache does.

Part of #15036 (source-control latency).

* perf(source-control): give diff reads a settled cache keyed on stamped git state

gitDiffReadDedupe coalesces only while a read is in flight, so every file
selection re-ran the whole read: a `git config --file .gitmodules` spawn, one or
two `git show` spawns, and a working-tree stat+read. On a WSL/UNC worktree each
git spawn is a wsl.exe invocation, which is the ">3s Loading diff..." in #15036.

Correctness first -- a stale diff is worse than a slow one. The cache never
expires on a clock and there is no TTL to tune. Instead:

- worktree-diff-stamp.ts takes a subprocess-free stamp of exactly the inputs a
  file diff is built from: HEAD (by resolved tip *content*, so a commit is
  visible even though HEAD's own bytes never move), `.git/index` (mtime+size),
  `.gitmodules` (submodule routing), and the working-tree file. A linked
  worktree's commondir and the packed-refs/reftable fallback are handled; an
  unborn branch is caught by recording "no loose ref" rather than only the
  packed stamps.
- The stamp is captured BEFORE the read and stored with the result. Anything
  that moves during or after the read leaves the stored stamp behind, so the
  next lookup misses. That, not a freshness window, is why a stale diff cannot
  be served.
- A store is refused unless the stamp was taken a full mtime bucket (2s, FAT's
  granularity) after its newest component. Below that, a second write inside the
  same bucket would be invisible -- git's own racy-index rule.
- `null` stamp means "cannot prove" and never caches: a folder workspace, a repo
  whose layout cannot be read, or a filesystem reporting no usable mtime.
- Submodule routes and reads that failed rather than proved absence are not
  reusable. A wsl.exe hiccup produces the same empty left side a new file does,
  and pinning that would persist a wrong diff.
- invalidateGitReadCaches clears it and bumps a generation, so a read that
  started pre-mutation cannot store its result post-mutation.

`ino` is deliberately optional in the working-tree component: Windows reports 0
for it on the redirector behind `\\wsl.localhost`, and requiring an unstable 0 to
match would make the cache silently never hit on the exact host it exists for.
Cache counters are exposed for the same reason -- a miss storm and a cold start
otherwise look identical.

Also drops gitDiffReadDedupe.clear() from getStatus. A status poll is a read; all
it did was destroy a live coalescing entry so a concurrent identical request
started duplicate git work. Mutations still invalidate through the shared point.

Memory is bounded by retained characters, not entry count -- one diff result can
legitimately hold megabytes.

Fixes the source-control half of #15036.

* perf(source-control): reuse BoundedMap and stop the WSL probe wait from outliving its answer

Review follow-ups on the settled-diff-cache work:

- SettledDiffCache now sits on the shared BoundedMap instead of hand-rolling the
  same Map + character ledger + evict-oldest loop.
- pendingWslDirectGitReadEnvironment returns null once the probe has settled
  either way, so a distro whose direct route was disabled no longer pays for a
  1.5s timer and two microtask hops on every git read.
- That wait now honours the read's abort signal and goes through withTimeout, so
  an aborted read is not held for the full bound and a probe rejection can never
  surface as a read failure.
- The settled-cache generation fence is taken before the stamp read, so a
  mutation that lands entirely inside the stamp's stats can no longer store an
  entry whose stamp is torn across it.
- The cache counters are folded into the main-thread churn probe report, which is
  what tells a permanently-cold cache apart from a cold start in the field.

* fix(source-control): tell WSL clock skew apart from a genuinely fresh write

The racy-write margin compares two clocks: capturedAtMs is this host's, while
the component mtimes come from whatever wrote the files. On a \\wsl.localhost
worktree the guest sets them, so a guest running ahead pushes every
recently-touched file past the margin and the cache refuses to store — for as
long as the skew lasts, on exactly the platform this cache exists for.

Nothing was wrong with the refusal; it was invisible. racyWrites alone cannot
distinguish "the repo was just edited" from "the clocks disagree and this will
never resolve on its own", so a permanently cold cache looked like a cold start.

isDiffStampClockSkewed flags the one thing no local write can produce — an mtime
in this host's future — and the cache counts those separately as
clockSkewedWrites. A nonzero count is the signal that the cache is off for a
reason idling will not fix.

Found by review of #16600; behavior is unchanged, only observability.
2026-08-26 15:44:11 -07:00
Neil 9367169888 refactor(tests): split every oversized test file off the max-lines suppression list (#14728)
* refactor(tests): split oversized test files off the max-lines suppression list

Every `*.test.ts`/`*.spec.ts` that carried an `eslint/oxlint-disable max-lines`
directive is now split into focused, behavior-scoped suites that fit the 800-line
test budget, with shared setup extracted into co-located `*-test-harness.ts` /
`*-test-fixtures.ts` modules (300-line budget). 83 files became ~930; the largest
output is 797 effective lines. `orca-runtime.test.ts` is intentionally untouched.

Test bodies were moved by scripted line-range slicing rather than retyped, so
assertions are byte-identical. The only permitted body edits were mechanical
rebinding where a shared value moved into a harness (e.g. `tmpHome` ->
`homes.tmpHome`).

Registries that enumerate test files were updated in lockstep:
- config/max-lines-baseline.txt: pruned 341 -> 258 entries (all 83 removed).
- config/reliability-gates.jsonc: 33 gates repointed at the split files, with
  assertionRefs split per file where a gate's coverage now spans several.
- .github/workflows/pr.yml: the real-zsh lane now lists the 4 split files that
  actually exercise zsh, so they keep running in the dedicated shell lane.

Also renamed agent-hooks `server-test-fixtures.ts` to `server.test-fixtures.ts`
so the global-fetch call-site audit keeps skipping it, and added `.js` extensions
to the CLI suites' dynamic harness imports (node16 resolution) to unbreak
`build:cli`.

Verification: full suite 52,449 passing vs 52,448 at baseline with zero
assertions lost; `pnpm lint`, `pnpm typecheck`, and `pnpm build:cli` all exit 0;
the terminal-pane e2e spec runs 31/31 headless.

* refactor(tests): split hook-idle arbitration suite that oxfmt pushed over budget

The pre-commit oxfmt pass reflowed pty-connection-hook-idle-arbitration.test.ts
to 811 effective lines, 11 over the test budget. Split the hook-completion side
effect and replacement-agent veto cases into their own suite; both files now sit
well under the cap and the 15 tests are unchanged.

* test: port upstream test changes into the split files after rebase

Rebasing onto main surfaced 27 tests that main had added to files this branch
deleted, plus edits to tests that had already moved. Taking the deletion side of
those modify/delete conflicts would have dropped that coverage silently, so each
upstream change is ported into the split file that now owns the behavior — for
example main's six orchestration mailbox tests land across orchestration-runs,
-send, and -check.

Also repoints `orchestration.notification-mailbox-consistency`, a gate main added
after this branch's gate remap, at those same three split files, and re-prunes
the max-lines baseline against main's (257 entries).

Verified: all 27 upstream test titles present; full suite 52,761 passing with the
only diff vs baseline being 12 tests main itself removed and 3 that moved from
skipped to passing; lint and typecheck exit 0.

* fix(test): flush pending continuations before tearing down terminal test globals

CI shard 5/16 failed on both Node 24 and 26 with `ReferenceError: window is not
defined` from pty-connection.ts, surfacing through
pty-connection-daemon-snapshot-replay.test.ts.

The reattach/settle chains `await` a real promise and then touch `window.api`.
Under fake timers those continuations cannot run, so they only become schedulable
once restoreTerminalTestGlobals() switches back to real timers — which previously
happened immediately before `delete globalThis.window`, so a late continuation
threw and failed the whole file. Flush async ticks in that window instead.

This is latent in the source rather than new: the pre-split 25k-line file kept
running other tests after these, which gave the chains time to settle before
teardown. Splitting the file moved teardown directly behind them.

* fix(test): keep an inert window after terminal test teardown instead of deleting it

The async-tick flush was not enough: the reattach/settle chain can resolve after
teardown regardless of how long we drain, so CI shard 5/16 still failed with
`ReferenceError: window is not defined` from pty-connection.ts.

A real renderer never loses `window`, so deleting it was the artificial part.
Swap in an inert proxy whose properties resolve to callables and whose calls
resolve to undefined, making a late `window.api.pty.*` call a harmless no-op.
The next test replaces it wholesale via installTerminalTestGlobals(), and no test
asserts that `window` is absent.
2026-08-15 00:54:20 -07:00