From 6ca33ddc24ab4cf33298a816ac34a1ab9b967e11 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sun, 9 Aug 2026 23:26:06 -0400 Subject: [PATCH 1/2] perf(git): cut the per-refresh git subprocess count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on this checkout (14 lanes), one full lane status refresh spawned **161 git processes with 32 peak concurrency**. 31 of those were byte-identical argv at a byte-identical cwd: `git rev-parse main` ran **14 times at the project root inside a single refresh**, and every lane paid two separate `rev-parse HEAD` because conflictService and rebaseSuggestionService each asked without knowing the other had. After: **133 processes, 12 peak, ~676ms -> ~560ms.** Lane data verified byte-identical against a cache-off control run in the same process — all 14 LaneSummary entries field-for-field, plus listSuggestions, listStatuses, and getBatchAssessment (ignoring its `computedAt` stamp). New `gitRepoCache.ts` is keyed by the repository's **common git dir**, so every lane worktree of one repo shares an entry. That sharing is the whole point: keying per worktree would leave a 14-lane repo holding 14 copies of one answer. Two freshness classes — 1.5s for resolved ref SHAs (long enough to collapse one refresh's duplicates, and less than a sixth of the 10s the lane list itself already caches for), 5 min for config-shaped answers. Concurrent misses share one load, which is what actually collapses a parallel fan-out; a plain TTL check lets all 14 through before the first resolves. `runGit` now bounds concurrent git processes at 12. Mutations bypass the queue deliberately — the cap exists to bound read fan-out, while mutations are user-initiated, few, and some allow 300s, so queueing a Commit behind a bulk conflict assessment would be a priority inversion the unbounded code did not have. Invalidation lives in `runGit` itself rather than at each mutation call site, so nothing has to remember — including code written later. It fires on failure as well as success: a rejected push can still have moved a remote-tracking ref, and an aborted rebase leaves HEAD moved. Two findings from review that were bugs, not polish: - `git -c core.editor=true rebase --continue` — the argv ADE actually uses — read `core.editor=true` as the verb, so a rebase that moved HEAD never invalidated. `runLaneOperation` then recorded a `postHeadSha` equal to the pre-rebase SHA, and Undo refuses to act when the recorded head does not match reality: Undo would have been permanently dead for every rebase/merge continue faster than 1.5s. - `ChatGitToolbar`'s changed-file badge summed `staged.length + unstaged.length`. `git status` reports a file with both staged and unstaged edits as `MM` and the parser puts it in both lists, so one dirty file counted as two. Both have regression tests verified to fail without their fix. Not done, with evidence: t3's staged/unstaged `--numstat` merge does not apply here. `git diff HEAD --numstat` reports the composite (a file staged +2/-1 then edited +1/-0 shows `3 1`, which is neither side), and a file whose worktree edit cancels its staged edit vanishes from the output while `git status` still reports it `MM`. ADE shows the two as separate lists, so the two calls are different questions, not one asked twice. Co-Authored-By: Claude Opus 5 --- .../services/conflicts/conflictService.ts | 33 ++- .../desktop/src/main/services/git/git.test.ts | 69 +++++ apps/desktop/src/main/services/git/git.ts | 177 ++++++++++- .../main/services/git/gitOperationsService.ts | 22 +- .../main/services/git/gitRepoCache.test.ts | 274 ++++++++++++++++++ .../src/main/services/git/gitRepoCache.ts | 274 ++++++++++++++++++ .../components/chat/ChatGitToolbar.tsx | 8 +- docs/ARCHITECTURE.md | 95 +++++- docs/features/chat/composer-and-ui.md | 2 +- docs/features/lanes/README.md | 52 ++++ 10 files changed, 985 insertions(+), 21 deletions(-) create mode 100644 apps/desktop/src/main/services/git/gitRepoCache.test.ts create mode 100644 apps/desktop/src/main/services/git/gitRepoCache.ts diff --git a/apps/desktop/src/main/services/conflicts/conflictService.ts b/apps/desktop/src/main/services/conflicts/conflictService.ts index 3e6209e61..b911c3298 100644 --- a/apps/desktop/src/main/services/conflicts/conflictService.ts +++ b/apps/desktop/src/main/services/conflicts/conflictService.ts @@ -74,7 +74,14 @@ import type { createProjectConfigService } from "../config/projectConfigService" import type { createAiIntegrationService } from "../ai/aiIntegrationService"; import type { createSessionService } from "../sessions/sessionService"; import type { LaneWorktreeLockService } from "../lanes/laneWorktreeLockService"; -import { normalizeConflictType, runGit, runGitMergeTree, runGitOrThrow } from "../git/git"; +import { + formatGitExecutionError, + normalizeConflictType, + runGit, + runGitMergeTree, + runGitOrThrow, + runGitRepoCached, +} from "../git/git"; import { redactSecretsDeep } from "../../utils/redaction"; import { extractFirstJsonObject } from "../ai/utils"; import { safeSegment } from "../shared/packLegacyUtils"; @@ -257,8 +264,30 @@ function extractOverlapFiles(row: ConflictPredictionRow | undefined): string[] { ]); } +/** + * Resolve `ref` to a SHA, shared across every caller asking the same question. + * + * The batch assessment fans out over lanes with an uncapped `Promise.all`, and + * every lane in a project typically shares one `baseRef` — measured, that was + * `git rev-parse main` running 14 times at the project root inside a single + * refresh. The cache key carries the cwd, so a repo-wide ref collapses to one + * subprocess while `HEAD` stays correctly distinct per worktree, and the lane's + * own `HEAD` read collides with the one `rebaseSuggestionService` makes. + */ async function readHeadSha(cwd: string, ref = "HEAD"): Promise { - return (await runGitOrThrow(["rev-parse", ref], { cwd, timeoutMs: 10_000 })).trim(); + const res = await runGitRepoCached( + ["rev-parse", ref], + { cwd, timeoutMs: 10_000 }, + { key: `rev-parse:${cwd}:${ref}`, cacheClass: "volatile" }, + ); + if (res.exitCode !== 0) { + // Same shaping runGitOrThrow applies. Without it, an unaccepted Xcode + // license surfaces git's raw complaint instead of ADE's guidance. + throw new Error(formatGitExecutionError( + res.stderr.trim() || res.stdout.trim() || `git rev-parse ${ref} failed`, + )); + } + return res.stdout.trim(); } async function readMergeBase(cwd: string, refA: string, refB: string): Promise { diff --git a/apps/desktop/src/main/services/git/git.test.ts b/apps/desktop/src/main/services/git/git.test.ts index e61ab5d97..de5188267 100644 --- a/apps/desktop/src/main/services/git/git.test.ts +++ b/apps/desktop/src/main/services/git/git.test.ts @@ -5,6 +5,7 @@ import path from "node:path"; import { spawnSync } from "node:child_process"; import { formatGitExecutionError, + getHeadSha, runGit, runGitMergeTree, runGitOrThrow, @@ -170,3 +171,71 @@ describe("macOS git selection", () => { expect(message).toContain("sudo xcodebuild -license"); }); }); + + +describe("repo cache invalidation through runGit", () => { + function scratchRepo(): string { + const repoRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-git-repo-cache-")); + fs.writeFileSync(path.join(repoRoot, "file.txt"), "one\n", "utf8"); + git(repoRoot, ["init", "-b", "main"]); + git(repoRoot, ["config", "user.email", "ade@test.local"]); + git(repoRoot, ["config", "user.name", "ADE Test"]); + git(repoRoot, ["add", "."]); + git(repoRoot, ["commit", "-m", "one"]); + return repoRoot; + } + + // `getHeadSha` is cached for 1.5s, so without the invalidation hook a commit + // made inside that window would keep reporting the pre-commit SHA — which is + // what `runLaneOperation` records as an operation's postHeadSha, and what + // Undo later refuses to act on. + it("serves a fresh HEAD immediately after a commit made through runGit", async () => { + const repoRoot = scratchRepo(); + const before = await getHeadSha(repoRoot); + expect(before).toBeTruthy(); + // Warm the cache a second time so a stale read would be served from it. + expect(await getHeadSha(repoRoot)).toBe(before); + + fs.writeFileSync(path.join(repoRoot, "file.txt"), "two\n", "utf8"); + await runGit(["add", "."], { cwd: repoRoot, timeoutMs: 20_000 }); + await runGit(["commit", "-m", "two"], { cwd: repoRoot, timeoutMs: 20_000 }); + + expect(await getHeadSha(repoRoot)).not.toBe(before); + }); + + // The argv ADE actually uses for rebase/merge continuation. The verb sits + // behind one of git's own options, and missing it means HEAD moves without + // the cache noticing. + it("still invalidates when the verb sits behind a git -c option", async () => { + const repoRoot = scratchRepo(); + const before = await getHeadSha(repoRoot); + expect(await getHeadSha(repoRoot)).toBe(before); + + fs.writeFileSync(path.join(repoRoot, "file.txt"), "three\n", "utf8"); + await runGit(["add", "."], { cwd: repoRoot, timeoutMs: 20_000 }); + await runGit(["-c", "core.editor=true", "commit", "-m", "three"], { + cwd: repoRoot, + timeoutMs: 20_000, + }); + + expect(await getHeadSha(repoRoot)).not.toBe(before); + }); + + it("bounds concurrent read fan-out without dropping any result", async () => { + const repoRoot = scratchRepo(); + const head = await getHeadSha(repoRoot); + // 40 distinct reads (distinct refs defeat the cache) all resolve under the + // semaphore rather than deadlocking or losing a result. + const results = await Promise.all( + Array.from({ length: 40 }, (_, index) => runGit( + ["rev-parse", index % 2 === 0 ? "HEAD" : "main"], + { cwd: repoRoot, timeoutMs: 20_000 }, + )), + ); + expect(results).toHaveLength(40); + for (const result of results) { + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe(head); + } + }); +}); diff --git a/apps/desktop/src/main/services/git/git.ts b/apps/desktop/src/main/services/git/git.ts index 8a853916b..1b44e0d4d 100644 --- a/apps/desktop/src/main/services/git/git.ts +++ b/apps/desktop/src/main/services/git/git.ts @@ -4,10 +4,17 @@ import { execFile, spawn } from "node:child_process"; import { promisify } from "node:util"; import type { ConflictFileType } from "../../../shared/types"; import { terminateProcessTree } from "../shared/processExecution"; +import { pathKey } from "../shared/pathCompare"; import { resolveExecutableCandidatesFromKnownLocations, type ResolvedExecutable, } from "../ai/cliExecutableResolver"; +import { + cachedGitRepoValue, + invalidateGitRepoCache, + isRefAffectingGitCommand, + type GitRepoCacheClass, +} from "./gitRepoCache"; // Electron apps launched from Finder/Dock can have a stripped PATH that misses // where the user actually installed git (e.g. /opt/homebrew/bin on Apple @@ -316,12 +323,166 @@ async function runGitOnce(args: string[], opts: GitRunOptions): Promise void> = []; + +async function acquireGitSlot(): Promise { + if (runningGitProcesses < MAX_CONCURRENT_GIT_PROCESSES) { + runningGitProcesses += 1; + return; + } + // The waiter is resolved by releaseGitSlot, which hands its slot over rather + // than freeing it — so the count stays accurate and never dips below the + // truth in the window between wake-up and resume. Decrementing first let a + // caller arriving in that window see an undercount and overshoot the cap. + await new Promise((resolve) => { gitProcessQueue.push(resolve); }); +} + +function releaseGitSlot(): void { + const next = gitProcessQueue.shift(); + if (next) next(); + else runningGitProcesses -= 1; +} + export async function runGit(args: string[], opts: GitRunOptions): Promise { - const first = await runGitOnce(args, opts); - if (!(await shouldRetryAfterIndexLock(first))) { - return first; + // Mutations skip the queue. The cap exists to bound read fan-out — one + // refresh issues ~130 reads across services that do not know about each + // other — and mutations are neither numerous nor interchangeable with it: + // they are user-initiated, and several run with multi-minute timeouts + // (`rebase --continue` allows 300s). Queueing a Commit or Push behind a bulk + // conflict assessment would be a priority inversion the old unbounded code + // did not have, and it is one gate for every project in a shared brain. + const mutating = isRefAffectingGitCommand(args); + if (!mutating) await acquireGitSlot(); + let result: GitRunResult; + try { + result = await runGitOnce(args, opts); + if (await shouldRetryAfterIndexLock(result)) { + result = await runGitOnce(args, opts); + } + } finally { + if (!mutating) releaseGitSlot(); } - return await runGitOnce(args, opts); + // Invalidate on failure as well as success. A rejected push can still have + // updated a remote-tracking ref, and an aborted rebase leaves HEAD moved — a + // cache that only drops on success would keep serving the pre-command answer + // for exactly the commands most likely to have changed it. + // + // Invalidating here rather than at each mutation call site is what makes the + // cache safe to add: nothing has to remember, including code written later + // and code that shells out through a path nobody mapped. + if (mutating) { + // `undefined` when this cwd's repo has not been resolved yet, which drops + // every repository. Over-invalidating costs a re-read; guessing wrong the + // other way serves a stale answer. + invalidateGitRepoCache(knownGitCommonDir(opts.cwd)); + } + return result; +} + +const gitCommonDirByCwd = new Map(); +const gitCommonDirRetryAtByCwd = new Map(); +const gitCommonDirInFlight = new Map>(); + +/** + * How long a failed common-dir probe is remembered. + * + * Neither extreme works. Memoizing a failure forever means one transient + * timeout silently disables this repo's cache for the process lifetime. + * Remembering nothing means a repo that genuinely cannot answer re-probes on + * every single cached read *and* misses the cache — strictly worse than not + * caching at all. A short window gives up on the repo briefly, then retries. + */ +const GIT_COMMON_DIR_RETRY_MS = 60_000; + +/** The memoized common git dir for `cwd`, or undefined if not resolved yet. */ +export function knownGitCommonDir(cwd: string): string | undefined { + return gitCommonDirByCwd.get(pathKey(cwd)); +} + +/** + * Absolute common git dir for `cwd` — the directory every lane worktree of one + * repository shares, and therefore the key everything repo-scoped hangs off. + * + * Memoized for the process lifetime on success: a worktree's common dir is + * fixed when the worktree is created. Returns `""` when the path is not a git + * repository, which callers treat as "do not cache". + * + * Deliberately not `--path-format=absolute`, which needs git 2.31 (2021). Bare + * `--git-common-dir` answers on every version and may return a path relative to + * `cwd`, so resolve it here. + */ +export async function gitCommonDirFor(cwd: string): Promise { + const cacheKey = pathKey(cwd); + const memo = gitCommonDirByCwd.get(cacheKey); + if (memo !== undefined) return memo; + const retryAt = gitCommonDirRetryAtByCwd.get(cacheKey); + if (retryAt !== undefined && retryAt > Date.now()) return ""; + const existing = gitCommonDirInFlight.get(cacheKey); + if (existing) return await existing; + + const request = (async () => { + const res = await runGit(["rev-parse", "--git-common-dir"], { + cwd, + timeoutMs: 8_000, + maxOutputBytes: 8 * 1024, + }); + const raw = res.exitCode === 0 ? res.stdout.trim() : ""; + if (!raw) { + gitCommonDirRetryAtByCwd.set(cacheKey, Date.now() + GIT_COMMON_DIR_RETRY_MS); + return ""; + } + const resolved = path.resolve(cwd, raw); + gitCommonDirByCwd.set(cacheKey, resolved); + gitCommonDirRetryAtByCwd.delete(cacheKey); + return resolved; + })().finally(() => { + if (gitCommonDirInFlight.get(cacheKey) === request) gitCommonDirInFlight.delete(cacheKey); + }); + gitCommonDirInFlight.set(cacheKey, request); + return await request; +} + +/** + * Run a read-only git command whose answer is the same for every worktree of + * the repository, serving it from the repo-scoped cache when it is warm. + * + * `key` must describe the question, not the caller — two services asking for + * the same ref's SHA should collide on purpose. + */ +export async function runGitRepoCached( + args: string[], + opts: GitRunOptions, + cache: { key: string; cacheClass: GitRepoCacheClass }, +): Promise { + const commonGitDir = await gitCommonDirFor(opts.cwd); + return await cachedGitRepoValue({ + commonGitDir, + key: cache.key, + cacheClass: cache.cacheClass, + load: () => runGit(args, opts), + }); +} + +export function resetGitCommonDirCacheForTests(): void { + gitCommonDirByCwd.clear(); + gitCommonDirRetryAtByCwd.clear(); + gitCommonDirInFlight.clear(); } export async function runGitOrThrow(args: string[], opts: GitRunOptions): Promise { @@ -343,7 +504,13 @@ export function formatGitExecutionError(raw: string): string { * Shared across gitOperationsService, laneService, autoRebaseService, and rebaseSuggestionService. */ export async function getHeadSha(worktreePath: string): Promise { - const res = await runGit(["rev-parse", "HEAD"], { cwd: worktreePath, timeoutMs: 8_000 }); + // Shares the repo cache — and the key shape — with conflictService's own HEAD + // read, which is how the two spawns every lane used to pay become one. + const res = await runGitRepoCached( + ["rev-parse", "HEAD"], + { cwd: worktreePath, timeoutMs: 8_000 }, + { key: `rev-parse:${worktreePath}:HEAD`, cacheClass: "volatile" }, + ); if (res.exitCode !== 0) return null; const sha = res.stdout.trim(); return sha.length ? sha : null; diff --git a/apps/desktop/src/main/services/git/gitOperationsService.ts b/apps/desktop/src/main/services/git/gitOperationsService.ts index 56de94248..87ca41740 100644 --- a/apps/desktop/src/main/services/git/gitOperationsService.ts +++ b/apps/desktop/src/main/services/git/gitOperationsService.ts @@ -1,6 +1,6 @@ import path from "node:path"; import { lookupOpenPrForBranch } from "./ghOpenPrLookup"; -import { getHeadSha, runGit, runGitOrThrow } from "./git"; +import { getHeadSha, runGit, runGitOrThrow, runGitRepoCached } from "./git"; import { detectConflictKind, parseNameOnly } from "./gitConflictState"; import type { GitActionResult, @@ -1690,11 +1690,15 @@ export function createGitOperationsService({ async getUserIdentity(args: { laneId: string }): Promise { const lane = laneService.getLaneBaseAndBranch(args.laneId); await assertLaneWorktreeRoot(lane); + // The committer identity is repo-wide (or global), so every lane asks the + // same question. Cached on the stable class keyed by the repo's common + // git dir; a `git config` write invalidates it through runGit. const readConfig = async (key: string): Promise => { - const result = await runGit(["config", "--get", key], { - cwd: lane.worktreePath, - timeoutMs: 5_000, - }); + const result = await runGitRepoCached( + ["config", "--get", key], + { cwd: lane.worktreePath, timeoutMs: 5_000 }, + { key: `config:${key}`, cacheClass: "stable" }, + ); return result.exitCode === 0 ? result.stdout.trim() : ""; }; const [name, email] = await Promise.all([readConfig("user.name"), readConfig("user.email")]); @@ -1708,7 +1712,13 @@ export function createGitOperationsService({ const lane = laneService.getLaneBaseAndBranch(laneId); await assertLaneWorktreeRoot(lane); const [remoteRes, branchRes] = await Promise.all([ - runGit(["remote", "get-url", "origin"], { cwd: lane.worktreePath, timeoutMs: 8_000 }).catch(() => null), + // Repo-wide: whether an origin exists, and its URL. Every lane of a + // repo shares one answer, and it only changes on `git remote`. + runGitRepoCached( + ["remote", "get-url", "origin"], + { cwd: lane.worktreePath, timeoutMs: 8_000 }, + { key: "remote-url:origin", cacheClass: "stable" }, + ).catch(() => null), lane.branchRef?.trim() ? Promise.resolve(null) : runGit(["rev-parse", "--abbrev-ref", "HEAD"], { cwd: lane.worktreePath, timeoutMs: 8_000 }).catch(() => null), diff --git a/apps/desktop/src/main/services/git/gitRepoCache.test.ts b/apps/desktop/src/main/services/git/gitRepoCache.test.ts new file mode 100644 index 000000000..804e49986 --- /dev/null +++ b/apps/desktop/src/main/services/git/gitRepoCache.test.ts @@ -0,0 +1,274 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { + cachedGitRepoValue, + invalidateGitRepoCache, + isRefAffectingGitCommand, + resetGitRepoCacheForTests, + GIT_REPO_CACHE_STABLE_TTL_MS, + GIT_REPO_CACHE_VOLATILE_TTL_MS, +} from "./gitRepoCache"; + +const REPO = "/repo/.git"; + +describe("isRefAffectingGitCommand", () => { + it("treats writes as invalidating", () => { + for (const args of [ + ["commit", "-m", "x"], + ["fetch", "--prune", "origin"], + ["push", "--force-with-lease"], + ["checkout", "-b", "feature"], + ["rebase", "--continue"], + ["reset", "--hard", "HEAD~1"], + ["worktree", "add", "/tmp/wt"], + ["remote", "add", "origin", "git@example.com:a/b.git"], + ["config", "user.email", "a@b.c"], + ["branch", "-D", "gone"], + ["symbolic-ref", "HEAD", "refs/heads/main"], + ["init"], + ]) { + expect(isRefAffectingGitCommand(args), args.join(" ")).toBe(true); + } + }); + + // ADE ships exactly these two argvs. Reading the first non-dash token as the + // verb finds `core.editor=true`, calls the command harmless, and skips + // invalidating after a rebase moved HEAD — which poisons the operation's + // recorded postHeadSha and permanently breaks Undo for it. + it("sees past git's own options to the verb", () => { + expect(isRefAffectingGitCommand(["-c", "core.editor=true", "rebase", "--continue"])).toBe(true); + expect(isRefAffectingGitCommand(["-c", "core.editor=true", "merge", "--continue"])).toBe(true); + expect(isRefAffectingGitCommand(["-C", "/repo", "commit", "-m", "x"])).toBe(true); + expect(isRefAffectingGitCommand(["--git-dir", "/repo/.git", "fetch"])).toBe(true); + expect(isRefAffectingGitCommand(["--git-dir=/repo/.git", "push"])).toBe(true); + expect(isRefAffectingGitCommand(["--no-pager", "log", "-n", "1"])).toBe(false); + expect(isRefAffectingGitCommand(["-c", "x=y", "rev-parse", "HEAD"])).toBe(false); + }); + + // An argument's *value* must not be able to pass for a read-only mood. + it("does not let an operand masquerade as a subcommand", () => { + expect(isRefAffectingGitCommand(["stash", "push", "--keep-index", "-u", "-m", "list"])).toBe(true); + expect(isRefAffectingGitCommand(["branch", "-m", "list"])).toBe(true); + expect(isRefAffectingGitCommand(["tag", "show"])).toBe(true); + }); + + // These are the ones that matter: ADE calls them on hot paths, and treating a + // read as a mutation would drop the cache on every lookup — strictly worse + // than not caching, since the subprocess still runs and takes every other + // entry down with it. + it("does not invalidate for the read-only mood of a writing verb", () => { + for (const args of [ + ["config", "--get", "user.email"], + ["config", "--get-all", "remote.origin.fetch"], + ["config", "--list"], + ["remote", "get-url", "origin"], + ["remote", "-v"], + ["worktree", "list", "--porcelain"], + ["branch", "--show-current"], + ["branch", "--list", "ade/*"], + ["tag", "--list"], + ["stash", "list"], + ]) { + expect(isRefAffectingGitCommand(args), args.join(" ")).toBe(false); + } + }); + + it("leaves ordinary read plumbing alone", () => { + for (const args of [ + ["rev-parse", "HEAD"], + ["status", "--porcelain=v2", "--branch"], + ["for-each-ref", "--format=%(refname)", "refs/remotes/"], + ["diff", "--numstat"], + ["rev-list", "--count", "a..b"], + ["merge-base", "a", "b"], + ["log", "-n", "20"], + ["ls-files", "-u"], + ]) { + expect(isRefAffectingGitCommand(args), args.join(" ")).toBe(false); + } + }); + + it("treats an unrecognized command as a mutation only if its verb writes", () => { + // Unknown verb: not in the writing set, so not invalidating. + expect(isRefAffectingGitCommand(["some-future-plumbing"])).toBe(false); + // Known writing verb in an unrecognized mood: invalidate rather than trust. + expect(isRefAffectingGitCommand(["config", "--some-future-flag", "x"])).toBe(true); + expect(isRefAffectingGitCommand([])).toBe(false); + }); +}); + +describe("cachedGitRepoValue", () => { + beforeEach(() => { + resetGitRepoCacheForTests(); + }); + + it("collapses concurrent misses for one key into a single load", async () => { + // The fan-out this exists for: 14 lanes ask for the same base ref's SHA at + // the same instant. A plain TTL check lets all 14 through before the first + // resolves, which is the 14 subprocesses we are removing. + let resolveLoad!: (value: string) => void; + const load = vi.fn(() => new Promise((resolve) => { resolveLoad = resolve; })); + + const reads = Array.from({ length: 14 }, () => cachedGitRepoValue({ + commonGitDir: REPO, + key: "rev-parse:main", + cacheClass: "volatile", + load, + })); + resolveLoad("sha-main"); + + expect(await Promise.all(reads)).toEqual(Array(14).fill("sha-main")); + expect(load).toHaveBeenCalledTimes(1); + }); + + it("keeps repositories and keys separate", async () => { + const load = vi.fn(async () => "value"); + await cachedGitRepoValue({ commonGitDir: REPO, key: "a", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: REPO, key: "a", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(1); + + await cachedGitRepoValue({ commonGitDir: REPO, key: "b", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "/other/.git", key: "a", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(3); + }); + + it("expires each class on its own TTL", async () => { + const load = vi.fn(async () => "value"); + const at = 1_000_000; + + await cachedGitRepoValue({ commonGitDir: REPO, key: "v", cacheClass: "volatile", load, nowMs: at }); + await cachedGitRepoValue({ commonGitDir: REPO, key: "s", cacheClass: "stable", load, nowMs: at }); + expect(load).toHaveBeenCalledTimes(2); + + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(at); + try { + // Just inside the volatile window: both still cached. + const stillFresh = at + GIT_REPO_CACHE_VOLATILE_TTL_MS - 1; + await cachedGitRepoValue({ commonGitDir: REPO, key: "v", cacheClass: "volatile", load, nowMs: stillFresh }); + expect(load).toHaveBeenCalledTimes(2); + + // Past volatile but well inside stable: only the ref SHA re-reads. + const pastVolatile = at + GIT_REPO_CACHE_VOLATILE_TTL_MS + 1; + nowSpy.mockReturnValue(pastVolatile); + await cachedGitRepoValue({ commonGitDir: REPO, key: "v", cacheClass: "volatile", load, nowMs: pastVolatile }); + await cachedGitRepoValue({ commonGitDir: REPO, key: "s", cacheClass: "stable", load, nowMs: pastVolatile }); + expect(load).toHaveBeenCalledTimes(3); + + const pastStable = at + GIT_REPO_CACHE_STABLE_TTL_MS + 1; + nowSpy.mockReturnValue(pastStable); + await cachedGitRepoValue({ commonGitDir: REPO, key: "s", cacheClass: "stable", load, nowMs: pastStable }); + expect(load).toHaveBeenCalledTimes(4); + } finally { + nowSpy.mockRestore(); + } + }); + + it("caches a non-throwing failure so a repo without an origin is asked once", async () => { + const load = vi.fn(async () => ({ exitCode: 128, stdout: "", stderr: "No such remote 'origin'" })); + await cachedGitRepoValue({ commonGitDir: REPO, key: "remote-url:origin", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: REPO, key: "remote-url:origin", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(1); + }); + + // "This repo has no origin" is the answer most likely to change out of band — + // the user runs `git remote add` in a terminal, which never goes through + // runGit — so it must not inherit the five-minute window a success gets. + it("holds a failure only for the short window even on the stable class", async () => { + const load = vi.fn(async () => ({ exitCode: 128, stdout: "", stderr: "no origin" })); + const at = 1_000_000; + await cachedGitRepoValue({ commonGitDir: REPO, key: "remote-url:origin", cacheClass: "stable", load, nowMs: at }); + + const pastVolatile = at + GIT_REPO_CACHE_VOLATILE_TTL_MS + 1; + await cachedGitRepoValue({ commonGitDir: REPO, key: "remote-url:origin", cacheClass: "stable", load, nowMs: pastVolatile }); + expect(load).toHaveBeenCalledTimes(2); + }); + + it("keys repositories by path identity, not by spelling", async () => { + const load = vi.fn(async () => "value"); + await cachedGitRepoValue({ commonGitDir: "/Repo/.git", key: "k", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "/Repo/.git/", key: "k", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(1); + }); + + it("does not pin a load that threw", async () => { + const load = vi.fn() + .mockRejectedValueOnce(new Error("git exploded")) + .mockResolvedValue("recovered"); + await expect(cachedGitRepoValue({ commonGitDir: REPO, key: "k", cacheClass: "stable", load })) + .rejects.toThrow("git exploded"); + await expect(cachedGitRepoValue({ commonGitDir: REPO, key: "k", cacheClass: "stable", load })) + .resolves.toBe("recovered"); + }); + + it("bypasses the cache entirely when the repo is unknown", async () => { + const load = vi.fn(async () => "value"); + await cachedGitRepoValue({ commonGitDir: "", key: "k", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "", key: "k", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(2); + }); + + describe("invalidation", () => { + it("drops one repo, or every repo", async () => { + const load = vi.fn(async () => "value"); + await cachedGitRepoValue({ commonGitDir: REPO, key: "k", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "/other/.git", key: "k", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(2); + + invalidateGitRepoCache(REPO); + await cachedGitRepoValue({ commonGitDir: REPO, key: "k", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "/other/.git", key: "k", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(3); + + invalidateGitRepoCache(); + await cachedGitRepoValue({ commonGitDir: REPO, key: "k", cacheClass: "stable", load }); + await cachedGitRepoValue({ commonGitDir: "/other/.git", key: "k", cacheClass: "stable", load }); + expect(load).toHaveBeenCalledTimes(5); + }); + + // The staleness case the epoch exists for: a mutation lands while a read is + // in flight. That read still answers its own caller, but it describes the + // pre-mutation repo and must not become the answer for anyone after. + it("does not hand an in-flight pre-mutation load to a caller arriving after", async () => { + let resolveFirst!: (value: string) => void; + const load = vi.fn() + .mockImplementationOnce(() => new Promise((r) => { resolveFirst = r; })) + .mockResolvedValue("after-mutation"); + + const inFlight = cachedGitRepoValue({ + commonGitDir: REPO, key: "rev-parse:HEAD", cacheClass: "volatile", load, + }); + + // Mutation lands while the read is still out. A caller arriving now must + // start its own read rather than joining the one already in the air. + invalidateGitRepoCache(REPO); + const joiner = cachedGitRepoValue({ + commonGitDir: REPO, key: "rev-parse:HEAD", cacheClass: "volatile", load, + }); + + resolveFirst("before-mutation"); + await expect(inFlight).resolves.toBe("before-mutation"); + await expect(joiner).resolves.toBe("after-mutation"); + expect(load).toHaveBeenCalledTimes(2); + }); + + it("does not let a load that started before a mutation publish afterwards", async () => { + let resolveFirst!: (value: string) => void; + const load = vi.fn() + .mockImplementationOnce(() => new Promise((r) => { resolveFirst = r; })) + .mockResolvedValue("after-mutation"); + + const inFlight = cachedGitRepoValue({ + commonGitDir: REPO, key: "rev-parse:HEAD", cacheClass: "volatile", load, + }); + + invalidateGitRepoCache(REPO); + resolveFirst("before-mutation"); + await expect(inFlight).resolves.toBe("before-mutation"); + + await expect(cachedGitRepoValue({ + commonGitDir: REPO, key: "rev-parse:HEAD", cacheClass: "volatile", load, + })).resolves.toBe("after-mutation"); + expect(load).toHaveBeenCalledTimes(2); + }); + }); +}); diff --git a/apps/desktop/src/main/services/git/gitRepoCache.ts b/apps/desktop/src/main/services/git/gitRepoCache.ts new file mode 100644 index 000000000..da7b32bfb --- /dev/null +++ b/apps/desktop/src/main/services/git/gitRepoCache.ts @@ -0,0 +1,274 @@ +/** + * Repo-scoped cache for git answers that are the same for every lane. + * + * ADE runs many worktrees of one repository, and a lane status refresh asks the + * same repo-wide question once per lane. Measured on a 14-lane checkout, one + * refresh spawned 161 `git` processes, of which 31 were byte-identical argv at + * a byte-identical cwd — `git rev-parse main` ran 14 times at the project root + * inside a single refresh, and every lane paid two separate `rev-parse HEAD` + * calls because two services asked without knowing about each other. + * + * Everything here is keyed by the repository's **common git dir**, so all lane + * worktrees of one repo share an entry. That is the multiplier: caching per + * worktree would leave a 14-lane repo with 14 copies of one answer. + * + * Two freshness classes, because the questions differ in kind: + * + * - `stable` (5 min) — the origin URL, the default branch, the committer + * identity. These change when someone edits git config, which ADE does not do + * on a poll loop. + * - `volatile` (1.5 s) — resolved ref SHAs. Long enough to collapse the + * duplicates inside one refresh (a full refresh measured 626 ms), short + * enough that a commit made outside ADE surfaces almost immediately. For + * comparison the lane list itself already caches for 10 s, so this is the + * more conservative of the two. + * + * Failures are cached too, deliberately. A repo with no `origin` answers + * "no origin" by failing, and re-asking that question once per lane per refresh + * costs exactly as much as asking a successful one. + */ + +import { pathKey } from "../shared/pathCompare"; + +/** Ref SHAs: collapses one refresh's duplicates, expires within a heartbeat. */ +export const GIT_REPO_CACHE_VOLATILE_TTL_MS = 1_500; +/** Config-shaped answers: origin URL, default branch, committer identity. */ +export const GIT_REPO_CACHE_STABLE_TTL_MS = 5 * 60_000; + +export type GitRepoCacheClass = "volatile" | "stable"; + +type CacheEntry = { + expiresAt: number; + value: unknown; +}; + +type RepoCache = { + entries: Map; + inFlight: Map>; + /** + * Bumped by every invalidation. A load that started before a mutation must + * not publish afterwards, and clearing the map cannot express that on its own + * because the load's `set` runs later. Same reason `laneService` carries a + * `laneListEpoch` next to its lane-list cache. + */ + epoch: number; +}; + +const repos = new Map(); + +/** + * Repos are keyed by a filesystem path, and on Windows and case-insensitive + * macOS two spellings of one directory are the same directory. Two buckets + * would mean a mutation issued under one spelling leaves the other's entries + * live — a missed invalidation, not merely a missed hit. + */ +function repoKey(commonGitDir: string): string { + return pathKey(commonGitDir); +} + +/** + * Git verbs that can change what a cached answer would be: refs, the remote + * set, config, or the worktree list. Anything that writes is treated as + * invalidating even when it fails — a `push` that is rejected may still have + * updated a remote-tracking ref, and a half-applied `rebase` moves HEAD. + */ +const REF_AFFECTING_GIT_VERBS = new Set([ + "am", "branch", "checkout", "cherry-pick", "clone", "commit", "config", + "fetch", "init", "merge", "pull", "push", "rebase", "remote", "reset", + "restore", "revert", "stash", "switch", "symbolic-ref", "tag", "update-ref", + "worktree", +]); + +/** + * Several of those verbs are read-only in one of their moods, and ADE calls + * exactly those moods on hot paths — `config --get user.email`, + * `remote get-url origin`, `worktree list`. Treating a read as a mutation would + * drop the cache on every lookup, which is worse than not caching at all: the + * subprocess still runs and every other entry dies with it. + * + * The mood lives in different places depending on the verb, so they are two + * separate tables rather than one scan of the whole argv. Scanning everything + * lets an argument *value* masquerade as a mood — `git stash push -m list` is a + * mutation whose message happens to be the word `list`. + */ +const READ_ONLY_GIT_SUBCOMMANDS: Record> = { + remote: new Set(["get-url", "show"]), + stash: new Set(["list", "show"]), + worktree: new Set(["list"]), +}; + +const READ_ONLY_GIT_FLAGS: Record> = { + branch: new Set(["--list", "-l", "--show-current", "--contains", "--points-at", "--merged", "--no-merged"]), + config: new Set(["--get", "--get-all", "--get-regexp", "--get-urlmatch", "--list", "-l"]), + remote: new Set(["-v", "--verbose"]), + tag: new Set(["--list", "-l", "--points-at", "--contains"]), +}; + +/** + * Git's own options, before the verb. The ones listed here take a separate + * operand, so both tokens have to be stepped over to reach the verb. + * + * ADE ships `git -c core.editor=true rebase --continue`. Reading the first + * non-dash token as the verb finds `core.editor=true`, concludes the command is + * harmless, and skips invalidating after a rebase actually moved HEAD. + */ +const GIT_GLOBAL_OPTIONS_WITH_OPERAND = new Set([ + "-c", "-C", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--config-env", +]); + +function gitVerbIndex(args: readonly string[]): number { + let index = 0; + while (index < args.length) { + const arg = args[index] ?? ""; + if (!arg.startsWith("-")) break; + index += GIT_GLOBAL_OPTIONS_WITH_OPERAND.has(arg) ? 2 : 1; + } + return index; +} + +/** + * True when running `args` may change a repo-wide answer this module caches. + * + * Ordinary read plumbing (`rev-parse`, `status`, `for-each-ref`, `diff`, …) is + * absent from the verb set entirely, so a refresh never invalidates its own + * cache. An unrecognized mood of a writing verb counts as a write — an unknown + * command is a reason to re-read, not a reason to trust stale data. + */ +export function isRefAffectingGitCommand(args: readonly string[]): boolean { + const verbIndex = gitVerbIndex(args); + const verb = args[verbIndex]; + if (!verb || !REF_AFFECTING_GIT_VERBS.has(verb)) return false; + + const rest = args.slice(verbIndex + 1); + const subcommands = READ_ONLY_GIT_SUBCOMMANDS[verb]; + if (subcommands) { + const subcommand = rest.find((arg) => !arg.startsWith("-")); + if (subcommand !== undefined) return !subcommands.has(subcommand); + } + const flags = READ_ONLY_GIT_FLAGS[verb]; + if (flags) return !rest.some((arg) => flags.has(arg)); + return true; +} + +function repoFor(commonGitDir: string): RepoCache { + const key = repoKey(commonGitDir); + let repo = repos.get(key); + if (!repo) { + repo = { entries: new Map(), inFlight: new Map(), epoch: 0 }; + repos.set(key, repo); + } + return repo; +} + +/** + * A `GitRunResult`-shaped value that reports a non-zero exit. Anything else — + * a plain string, an object without an exit code — is treated as a success, + * since only git results carry the concept. + */ +function isFailedGitResult(value: unknown): boolean { + return typeof value === "object" + && value !== null + && "exitCode" in value + && (value as { exitCode: unknown }).exitCode !== 0; +} + +function ttlFor(cacheClass: GitRepoCacheClass): number { + return cacheClass === "stable" + ? GIT_REPO_CACHE_STABLE_TTL_MS + : GIT_REPO_CACHE_VOLATILE_TTL_MS; +} + +/** + * Read `key` for `commonGitDir`, running `load` on a miss. + * + * Concurrent misses for one key share a single `load`, which is what collapses + * the fan-out — a refresh asks 14 lanes' worth of questions in parallel, so a + * plain TTL check would let all 14 through before the first one resolved. + */ +export async function cachedGitRepoValue(args: { + commonGitDir: string; + key: string; + cacheClass: GitRepoCacheClass; + load: () => Promise; + nowMs?: number; +}): Promise { + const { commonGitDir, key, cacheClass, load } = args; + if (!commonGitDir) return await load(); + const nowMs = args.nowMs ?? Date.now(); + const repo = repoFor(commonGitDir); + + const entry = repo.entries.get(key); + if (entry && entry.expiresAt > nowMs) return entry.value as T; + if (entry) repo.entries.delete(key); + + const existing = repo.inFlight.get(key); + if (existing) return await (existing as Promise); + + const loadEpoch = repo.epoch; + const request = load().then( + (value) => { + // Cache the answer whichever way it came out: "this repo has no origin" + // costs a subprocess to rediscover exactly like a successful lookup does. + // Unless the repo was mutated while we were reading, in which case this + // answer describes the old repo — return it to the caller that asked for + // it, but never pin it for the next one. + // TTL runs from when we asked, not from when git answered. A slow load + // therefore expires slightly sooner rather than extending its own + // freshness window past what the caller was told to expect — and it + // makes the window deterministic under an injected clock. + if (repo.epoch === loadEpoch) { + // A failure is cached — re-asking "does this repo have an origin?" once + // per lane costs the same as asking a successful question — but only on + // the short window. The absent answer is the one most likely to change + // (the user runs `git remote add` in a terminal, which never passes + // through runGit), so pinning it for five minutes would leave the + // publish affordances wrong for five minutes. + const failed = isFailedGitResult(value); + repo.entries.set(key, { + value, + expiresAt: nowMs + ttlFor(failed ? "volatile" : cacheClass), + }); + } + return value; + }, + (error) => { + // A thrown load is a different thing from a git command that exited + // non-zero — that one resolves with an exit code and is cached above. + // Something threw, so we know nothing; do not pin it. + throw error; + }, + ).finally(() => { + if (repo.inFlight.get(key) === request) repo.inFlight.delete(key); + }); + repo.inFlight.set(key, request); + return await request; +} + +/** + * Drop cached answers. With no argument, drops every repository. + * + * In-flight loads still resolve to the caller that asked for them, but the + * epoch bump stops them from pinning a pre-mutation answer, and dropping them + * from `inFlight` stops a later caller from *joining* one. Clearing only + * `entries` left that second door open: a read that started before a commit + * would be handed to every caller arriving after it, with no cache entry left + * behind to explain where the stale SHA came from. + */ +export function invalidateGitRepoCache(commonGitDir?: string): void { + const drop = (repo: RepoCache): void => { + repo.entries.clear(); + repo.inFlight.clear(); + repo.epoch += 1; + }; + if (commonGitDir === undefined) { + for (const repo of repos.values()) drop(repo); + return; + } + const repo = repos.get(repoKey(commonGitDir)); + if (repo) drop(repo); +} + +/** Test seam: forget every repository, in-flight loads included. */ +export function resetGitRepoCacheForTests(): void { + repos.clear(); +} diff --git a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx index 290638bc1..416240bf6 100644 --- a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx @@ -61,7 +61,13 @@ type ChatGitToolbarProps = { // --------------------------------------------------------------------------- function dirtyFileCount(changes: DiffChanges): number { - return changes.staged.length + changes.unstaged.length; + // Distinct paths, not the sum of the two lists. `git status` reports a file + // with both a staged and an unstaged edit as `MM`, and the parser puts it in + // both lists — so summing counted one dirty file as two. + const paths = new Set(); + for (const change of changes.staged) paths.add(change.path); + for (const change of changes.unstaged) paths.add(change.path); + return paths.size; } function checksIcon(status: PrSummary["checksStatus"], state: PrSummary["state"]) { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b63511c51..98e34153e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1247,11 +1247,94 @@ Related trust-boundary docs: [Computer-use artifact broker](./features/computer- ### 9.1 Strategy - ADE **shells out** to the system `git` binary (not isomorphic-git). Rationale: full feature parity, hook compatibility, native credential handling, performance. -- All commands go through `runGit` / `runGitOrThrow` in `apps/desktop/src/main/services/git/git.ts` (timeout support, structured output parsing). +- All commands go through `runGit` / `runGitOrThrow` in `apps/desktop/src/main/services/git/git.ts` (timeout support, structured output parsing, index-lock retry). `runGit` is also where the shared concurrency ceiling and cache invalidation live — see [Process budget](#92-process-budget-concurrency-ceiling-and-the-repo-scoped-cache). - Executable discovery reuses `ai/cliExecutableResolver.ts` to inspect PATH and known installation directories. On macOS, ADE prefers an independently installed Git over Apple's `/usr/bin/git` and probes the login shell before accepting `/usr/bin/git`; this keeps project opening usable when Apple's Git is blocked by an unaccepted Xcode license. If Apple's Git is the only available option, the surfaced error explains that accepting the license is a Git prerequisite, not an iOS Simulator or code-signing requirement, and points to installing Git separately as the alternative. - High-level ops in `gitOperationsService.ts` — wrap every mutation in `runLaneOperation()`: resolve lane, capture pre-HEAD, record operation, execute, capture post-HEAD, finalize record, fire `onHeadChanged` if needed. -### 9.2 Worktree-per-lane isolation +### 9.2 Process budget: concurrency ceiling and the repo-scoped cache + +Because ADE shells out, every git question is a process spawn, and ADE asks a +lot of them at once: a lane status refresh fans out over lanes with uncapped +`Promise.all` inside several services that do not know about each other. Two +shared mechanisms in `services/git/git.ts` bound that — a global concurrency +ceiling and a repo-scoped answer cache — and both sit inside `runGit`, so a +caller gets them without opting in. + +**Concurrency ceiling.** `runGit` holds a single process-wide semaphore capped +at `MAX_CONCURRENT_GIT_PROCESSES = 12`. One global cap rather than a limit per +call site, because it is precisely the mutual ignorance of the fan-outs that +produced the peak — concurrency scaled linearly with lane count. Spawn cost is +10–27 ms on macOS and 35–100 ms on Windows, so queueing behind a modest cap +costs no meaningful wall time while removing the thundering-herd tail. +**Mutating commands bypass the queue on purpose.** They are user-initiated, few, +and several run with multi-minute timeouts (`rebase --continue` allows 300 s), +so making a Commit wait behind a bulk conflict assessment would be a priority +inversion — and the brain is one gate for every project on the machine. +`isRefAffectingGitCommand` (in `gitRepoCache.ts`) is what draws that line; it +steps over git's own global options to find the verb, and treats read-only moods +of writing verbs (`config --get`, `remote get-url`, `worktree list`, +`branch --list`) as reads. + +**Repo-scoped cache** (`services/git/gitRepoCache.ts`). Entries are keyed by the +repository's **common git dir**, not by worktree — that is the whole point. +Every lane worktree of one repo resolves to the same key, so a repo-wide +question asked once per lane collapses to one subprocess; keying per worktree +would leave a 14-lane repo holding 14 copies of one answer and buy nothing. +`gitCommonDirFor(cwd)` resolves the key with bare `git rev-parse +--git-common-dir` (not `--path-format=absolute`, which needs git 2.31) and +memoizes it for the process lifetime, since a worktree's common dir is fixed at +creation; a failed probe is remembered for 60 s and then retried, so one +transient timeout cannot disable the repo's cache permanently. An unresolvable +path yields `""`, which means "do not cache" and passes straight through. + +Two freshness classes, because the questions differ in kind: + +| Class | TTL | Used for | +|-------|-----|----------| +| `volatile` | 1.5 s | Resolved ref SHAs — long enough to collapse the duplicates inside one refresh, short enough that a commit made outside ADE surfaces almost immediately. Stricter than the 10 s lane-list cache it feeds. | +| `stable` | 5 min | Config-shaped answers: `origin` URL, committer identity. These change when someone edits git config, which ADE does not do on a poll loop. | + +Failures are cached too — rediscovering "this repo has no `origin`" costs a +subprocess exactly like a successful lookup — but a failed result is always +pinned on the **volatile** window regardless of its declared class, because an +absent answer is the one most likely to be fixed out of band. Concurrent misses +for one key share a single load, which is what actually collapses the fan-out: a +plain TTL check would let all 14 parallel askers through before the first +resolved. + +`runGitRepoCached(args, opts, { key, cacheClass })` is the read path. The key +describes the *question*, not the caller, so two services asking the same thing +collide deliberately: `getHeadSha` and `conflictService.readHeadSha` both use +`rev-parse::`, which is how a lane's two independent HEAD reads became +one. `gitOperationsService.getUserIdentity` (`config:user.name` / +`config:user.email`) and `getOriginRemote` (`remote-url:origin`) sit on the +stable class. + +**Invalidation lives in `runGit`, and fires on failure too.** After any +ref-affecting verb, `runGit` drops that repository's entries — whether the +command succeeded or not, because a rejected push may still have moved a +remote-tracking ref and an aborted rebase leaves HEAD moved. Putting it there +rather than at each mutation call site is what makes the cache safe to add: +nothing has to remember, including code written later and code that shells out +through a path nobody mapped. When the cwd's repo has not been resolved yet the +invalidation drops *every* repository — over-invalidating costs a re-read, and +guessing the other way serves a stale answer. Invalidation is epoch-guarded and +clears in-flight loads as well as entries, so a read that started before a +mutation can still answer the caller that asked for it but can neither pin its +result nor be joined by a later caller. + +**Git run outside ADE does not invalidate anything.** A user committing in an +ADE terminal, or any tool that does not route through `runGit`, leaves the cache +untouched — the TTL is the only defense there, which is why the volatile window +is 1.5 s rather than something more generous. + +Measured on a real 14-lane ADE checkout, one full lane status refresh went from +161 git processes at 32 peak concurrency (~676 ms) to 133 at 12 (~560 ms): +`git rev-parse main` at the project root went from 14 spawns to 1, and each +lane's duplicated `rev-parse HEAD` went from 2 to 1. Lane data was verified +byte-identical against a cache-off control run. + +### 9.3 Worktree-per-lane isolation Each non-primary lane maps to a dedicated worktree: @@ -1271,14 +1354,14 @@ Worktree lifecycle: create (60s timeout), archive (DB status only, worktree rema Lane rows are not the authority on which worktrees exist — git is. Every `lanes.list` reconciles the two from a single `git worktree list`, inserting a lane for any worktree of this repository that no row claims and deleting the row for any non-primary, non-archived lane whose worktree is gone from both git and disk. There is no register or attach step. The reconcile is scoped by ownership (`git rev-parse --git-common-dir`): a project root that is itself a linked worktree sees the whole repository in that listing and therefore adopts and reaps only under its own `.ade/worktrees`. All of it runs in realpath space, because git resolves symlinks and ADE's stored paths do not. See [Lanes › Every git worktree is a lane](./features/lanes/README.md#every-git-worktree-is-a-lane). -### 9.3 Stack graph +### 9.4 Stack graph - Lanes have `parent_lane_id` (self-FK on `lanes`). Stacks are parent/child chains. - Stack operations: rebase propagation, base-ref resolution (`shared/laneBaseResolution.ts`). - `autoRebaseService.ts` + `rebaseSuggestionService.ts` — automatic rebase proposals when parent moves; user can accept/defer/dismiss. - `computeLaneStatus()` returns `{ dirty, ahead, behind }` on demand, no caching. Status derivation uses `git status --porcelain=v2 --branch` and `git rev-list --left-right --count`. The `--branch` header (`# branch.head`) carries the branch HEAD is actually on, so the same call that computes dirty state also yields `headBranchRef` — which is what makes HEAD-vs-`lanes.branch_ref` drift detection (`services/lanes/laneBranchDrift.ts`) cost no extra process spawn and need no timer of its own. Ignored files are still not listed (no `--ignored`), so dirty semantics are identical to the porcelain v1 form this replaced. See [Lanes › Branch drift](./features/lanes/README.md#branch-drift). -### 9.4 Queue + conflict simulation +### 9.5 Queue + conflict simulation - **GitHub stacked PRs** (`githubPrStackService.ts`) — repository-scoped stack reconciliation and membership snapshots. - **Conflict prediction** — `conflictService.ts` uses `runGitMergeTree()`: @@ -1289,14 +1372,14 @@ Lane rows are not the authority on which worktrees exist — git is. Every `lane - Triggered on debounced lane/head changes via the job engine; periodic prediction is off by default in dev stability mode. - Result: risk matrix surfaced on Graph + Conflicts pages, confidence-scored proposals (`high`/`medium`/`low`) with apply/discard UI. -### 9.5 Safety +### 9.6 Safety - `ensureRelativeRepoPath()` rejects empty, null-byte, absolute, and traversal paths. - Force push uses `--force-with-lease`, never `--force`. - Branch-protection support on primary lane. - Destructive ops (discard, hard reset) require UI confirmation. -### 9.6 Open-PR lookup for a lane branch +### 9.7 Open-PR lookup for a lane branch `gh pr list --head ` matches on branch **name only**, across every fork of the repository. A PR opened from somebody else's fork that happens to use the same branch name is returned by that query and, unfiltered, attaches itself to the lane. Filtering the result by head repository is therefore a correctness invariant, not an optimization. diff --git a/docs/features/chat/composer-and-ui.md b/docs/features/chat/composer-and-ui.md index 933a18fe1..d85dadcc9 100644 --- a/docs/features/chat/composer-and-ui.md +++ b/docs/features/chat/composer-and-ui.md @@ -45,7 +45,7 @@ subagents, computer use). The pane derives all visible state from the | `ChatIosSimulatorPanel.tsx` | macOS-only iOS Simulator drawer. Two mount points: under the chat composer and inside the Work right-edge sidebar. Tool-readiness checklist, device + target pickers, three-backend live preview, `interact` vs `inspect` mode, hit-test overlay, and selection emission as `IosElementContextItem`. Accepts an optional `laneId` prop, forwarded into `iosSimulator.launch` so the resulting `IosSimulatorSession` records its launching lane. Simulator controls are not blocked when another chat session owns the simulator — ownership only affects which session receives context insertions, not whether the user can interact with the device. See [iOS Simulator feature](../ios-simulator/README.md). | | `ChatBuiltInBrowserPanel.tsx` | In-app browser panel mounted under the Work right-edge sidebar's `browser` tab. Renders the address bar, navigation/tab strip, inspect toolbar, screenshot capture, and an empty/error state derived from `BuiltInBrowserStatus`; the actual page content is painted by a main-process `WebContentsView` whose bounds the panel reports back to the broker via `ade.builtInBrowser.setBounds`. Inspect-mode hit-tests emit `BuiltInBrowserContextItem` payloads through `onAddContext`; the sidebar then dispatches `ade:agent-chat:add-builtin-browser-context` to the active chat. The panel does not run inside `AgentChatPane` directly — instead, anywhere in the renderer that wants to open a URL calls `openUrlInAdeBrowser()` (in `apps/desktop/src/renderer/lib/openExternal.ts`), which fires `ADE_OPEN_BUILT_IN_BROWSER_EVENT` and asks the broker to open a new tab. | | `ChatTerminalDrawer.tsx` | Collapsible terminal drawer at the bottom of the chat. | -| `ChatGitToolbar.tsx` | Git status and quick-action toolbar above the composer. The PR action opens or toggles a linked PR when one exists, otherwise opens the PR creation handoff for the current lane targeting the primary branch. Opening the chat PR pane or compact PR menu performs a targeted, cooldown-bound refresh for that single linked PR. The toolbar is a **status strip only** — the manual PR-sync (↻) button moved into the PR pane's title bar, so surfaces that render this toolbar without a PR pane have no manual sync affordance and heal through reconcile-on-focus plus `prs-updated` instead. | +| `ChatGitToolbar.tsx` | Git status and quick-action toolbar above the composer. The changed-file count is the number of **distinct paths** across the staged and unstaged lists, not their lengths summed: `git status` reports a file with both a staged and an unstaged edit as `MM`, and the parser puts that one file in both lists. The PR action opens or toggles a linked PR when one exists, otherwise opens the PR creation handoff for the current lane targeting the primary branch. Opening the chat PR pane or compact PR menu performs a targeted, cooldown-bound refresh for that single linked PR. The toolbar is a **status strip only** — the manual PR-sync (↻) button moved into the PR pane's title bar, so surfaces that render this toolbar without a PR pane have no manual sync affordance and heal through reconcile-on-focus plus `prs-updated` instead. | | `ChatPrPane.tsx` | Left floating PR pane for Work chat. Owns a title bar (`Pull request` + ↻ refresh + ✕ close): ↻ calls `prs.syncLanePr` and then re-reads the pane's PR, and spins for either a manual sync or a backend reconcile-on-focus (`pr-reconcile`, debounced 300 ms on the hide so a fast reconcile does not flicker). ✕ is wired to the parent's `onClose` (the header PR pill still toggles it). Shows cached lane PR details immediately, then refreshes the linked PR row with the same targeted refresh path so pane toggles surface current merged/closed/check state without a broad PR sync. An unmapped lane PR (projection-derived, `pr.unmapped`) skips the refresh and checks/reviews enrichment — there is no DB row behind its synthetic `gh:` id. With no PR it embeds `ChatPrInlineCreator` and forwards the chat's `sessionTitle`; under a `runtimePin` it points at the owning machine instead, since creation is not pinned. Reads, the ↻ sync, and the event subscription all take the pin so a chat on another machine sees its lane's real PR. | | `ChatPrInlineCreator.tsx` | Inline create-PR form inside the PR pane. Laid out as a **flow** with no uppercase section captions: a flat, boxless source row (lane name + branch + lock glyph, immutable), a `↓` connector carrying `N ahead · N behind · clean`/`dirty` from `lane.status` (muted `comparing…` when the lane has no status yet), then the canonical `LaneCombobox` target dropdown (no free text), title, description, and Create. The title defaults to the chat session title whenever it is a real title (the placeholder `New chat` never wins), otherwise to the ` -> ` derivation. Linear magic words and the deeplink footer are added server-side by `prService`. On success it hands the created `PrSummary` up through `onCreated` so the pane swaps to details without waiting for `prs-updated`. | | `ChatUserMinimap.tsx`, `chatUserMinimap.logic.ts` | Tick rail down the transcript's **left** gutter, one hairline per user message, gated on the `chatUserMinimapEnabled` appearance setting and mouse pointers only (`[@media(pointer:fine)]`). Ticks are positioned by percentage of rail height, so they compress instead of overflowing and there is no marker cap or subsampling — the entry index stays 1:1 with the tick index, which is what pointer→index mapping depends on. The whole rail is a single `