Skip to content

Git subprocess diet: 161 → 133 spawns, 32 → 12 peak concurrency #1062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arul28 merged 2 commits into from
Aug 10, 2026
Merged

Git subprocess diet: 161 → 133 spawns, 32 → 12 peak concurrency #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ca33dd
perf(git): cut the per-refresh git subprocess count
arul28 Aug 10, 2026
43566e5
review: key config-derived cache entries per worktree
arul28 Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions apps/desktop/src/main/services/conflicts/conflictService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<string> {
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<string> {
Expand Down
69 changes: 69 additions & 0 deletions apps/desktop/src/main/services/git/git.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import path from "node:path";
import { spawnSync } from "node:child_process";
import {
formatGitExecutionError,
getHeadSha,
runGit,
runGitMergeTree,
runGitOrThrow,
Expand Down Expand Up @@ -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);
}
});
});
177 changes: 172 additions & 5 deletions apps/desktop/src/main/services/git/git.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -316,12 +323,166 @@ async function runGitOnce(args: string[], opts: GitRunOptions): Promise<GitRunRe
});
}

/**
* Global ceiling on concurrent `git` processes.
*
* Nothing bounded this before. A lane status refresh fans out with an uncapped
* `Promise.all` per service, and on a 14-lane checkout that peaked at 32
* simultaneous git processes — linear in lane count, so 40 lanes would reach
* ~90. Measured spawn costs on that repo are 10-27 ms each, so serializing
* behind a modest cap costs no meaningful wall time while removing the
* thundering-herd tail. It matters much more on Windows, where process creation
* runs 35-100 ms.
*
* One shared cap rather than a limit per call site: the fan-outs do not know
* about each other, which is exactly how 32 happened.
*/
const MAX_CONCURRENT_GIT_PROCESSES = 12;
let runningGitProcesses = 0;
const gitProcessQueue: Array<() => void> = [];

async function acquireGitSlot(): Promise<void> {
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<void>((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<GitRunResult> {
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<string, string>();
const gitCommonDirRetryAtByCwd = new Map<string, number>();
const gitCommonDirInFlight = new Map<string, Promise<string>>();

/**
* 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<string> {
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;
}
Comment on lines +430 to +459

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

The successful common-dir memo is never dropped.

gitCommonDirByCwd keeps a resolved path for the process lifetime, and no worktree mutation clears it. If ADE removes a lane worktree and a later worktree of a different repository is created at the same absolute path, knownGitCommonDir returns the previous repository. Two consequences follow: runGitRepoCached reads land in the wrong repository bucket, and mutation invalidation drops the wrong bucket. The retry map is already cleared for failures, so only the success path is affected.

Drop the memo entry for a cwd when a worktree command runs through runGit, or key the memo on the worktree's git dir instead of the cwd.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/desktop/src/main/services/git/git.ts` around lines 430 - 459, Update the
successful common-directory memo used by gitCommonDirFor so it is invalidated
whenever a worktree mutation runs through runGit. Remove the corresponding cwd
entry from gitCommonDirByCwd (and keep related in-flight or retry state
consistent as needed) before or after executing the worktree command, ensuring
later lookups do not reuse a repository path from a previous worktree at the
same absolute cwd.


/**
* 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<GitRunResult> {
const commonGitDir = await gitCommonDirFor(opts.cwd);
return await cachedGitRepoValue<GitRunResult>({
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<string> {
Expand All @@ -343,7 +504,13 @@ export function formatGitExecutionError(raw: string): string {
* Shared across gitOperationsService, laneService, autoRebaseService, and rebaseSuggestionService.
*/
export async function getHeadSha(worktreePath: string): Promise<string | null> {
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" },
Comment on lines +509 to +512

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep safety-critical HEAD reads out of the TTL cache

When HEAD changes outside runGit—for example, an agent or user commits in an ADE terminal—this cache can return the previous SHA for 1.5 seconds. undoLastHeadChange uses getHeadSha for its "head has changed" guard and then runs reset --hard; during that window the stale value can make the guard pass and reset past the external commit, while runLaneOperation can also record an incorrect preHeadSha. Keep operational snapshots and destructive-action guards on an uncached HEAD read, reserving this cache for status/prediction polling.

Useful? React with 👍 / 👎.

);
Comment on lines +507 to +513

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Both HEAD readers build the cache key from a raw path string. The repository bucket is normalized with pathKey, but neither key is. The documented collision between the two services therefore holds only when both callers pass byte-identical spellings of one path, and a repo-wide ref such as the shared base branch cannot share a single entry across lane worktrees of one repository. Add one shared key builder that normalizes the path and omits it for refs that are not worktree-specific.

  • apps/desktop/src/main/services/git/git.ts#L507-L513: export a gitRefCacheKey(cwd, ref) helper that returns rev-parse:${pathKey(cwd)}:HEAD for HEAD and rev-parse:${ref} for any other ref, then use it here.
  • apps/desktop/src/main/services/conflicts/conflictService.ts#L278-L282: replace the inline template key with the same gitRefCacheKey(cwd, ref) helper so the two services collide by construction instead of by convention.
📍 Affects 2 files
  • apps/desktop/src/main/services/git/git.ts#L507-L513 (this comment)
  • apps/desktop/src/main/services/conflicts/conflictService.ts#L278-L282
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/desktop/src/main/services/git/git.ts` around lines 507 - 513, The cache
keys for HEAD and shared refs are built inconsistently from raw paths. In
apps/desktop/src/main/services/git/git.ts:507-513, export a gitRefCacheKey(cwd,
ref) helper that uses pathKey(cwd) for HEAD and omits the path for other refs,
then use it in the runGitRepoCached call; in
apps/desktop/src/main/services/conflicts/conflictService.ts:278-282, replace the
inline key template with gitRefCacheKey(cwd, ref) so both readers share
normalized keys.

if (res.exitCode !== 0) return null;
const sha = res.stdout.trim();
return sha.length ? sha : null;
Expand Down
26 changes: 20 additions & 6 deletions apps/desktop/src/main/services/git/gitOperationsService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -1690,11 +1690,19 @@ export function createGitOperationsService({
async getUserIdentity(args: { laneId: string }): Promise<GitUserIdentity> {
const lane = laneService.getLaneBaseAndBranch(args.laneId);
await assertLaneWorktreeRoot(lane);
// Keyed by worktree, not repo. `git config` is repo-wide *unless* the
// repo enables `extensions.worktreeConfig`, in which case each lane can
// carry its own `user.name`/`user.email` — and a repo-wide key would then
// serve whichever lane asked first to all of them, mis-attributing
// commits. ADE never enables that extension, but a user's repo can, and
// these calls are per-lane anyway so a repo-wide key saves nothing
// measurable here.
const readConfig = async (key: string): Promise<string> => {
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:${lane.worktreePath}:${key}`, cacheClass: "stable" },
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return result.exitCode === 0 ? result.stdout.trim() : "";
};
const [name, email] = await Promise.all([readConfig("user.name"), readConfig("user.email")]);
Expand All @@ -1708,7 +1716,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),
// Worktree-keyed for the same reason as the identity read above:
// `extensions.worktreeConfig` lets a lane override `remote.origin.url`.
runGitRepoCached(
["remote", "get-url", "origin"],
{ cwd: lane.worktreePath, timeoutMs: 8_000 },
{ key: `remote-url:${lane.worktreePath}: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),
Expand Down
Loading
Loading