From 73ca882d1352b9d3d94f37e52d8dc16ec46afa42 Mon Sep 17 00:00:00 2001 From: "(CJ) Chukwudi Nwobodo" <142016413+chuks-qua@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:42:32 +0100 Subject: [PATCH] perf(server): bound worktree add timeout and unqueue read-only git calls git worktree add previously ran with no timeout, so a hung add stalled the RPC forever. Bound it at 120s. All git calls were also serialised per repo, so a slow mutation blocked every read on that repo (worktree list, branch probes, status, diffs). Mutating commands keep the per-repo queue; read-only commands now run unqueued. --- .../git/__tests__/git-service-push.test.ts | 4 +- .../__tests__/real-git-executor.test.ts | 76 +++++++++++- .../git/execution/real-git-executor.ts | 109 ++++++++++++++++-- .../projects/git/git-worktree-service.ts | 3 +- 4 files changed, 175 insertions(+), 17 deletions(-) diff --git a/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts b/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts index 2e339bf3f..f927a10f6 100644 --- a/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts +++ b/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts @@ -145,7 +145,7 @@ describe("GitRepositoryService and GitWorktreeService branch creation", () => { "--detach", NodePath.join("/mock/mcode", "worktrees", "repo", "main-branchless"), "main", - ], { timeout: 0 }); + ], { timeout: 120_000 }); }); it("creates a named worktree branch from the exact requested base ref", async () => { @@ -176,7 +176,7 @@ describe("GitRepositoryService and GitWorktreeService branch creation", () => { "-b", "codex/issue-960", "origin/main", - ], { timeout: 0 }); + ], { timeout: 120_000 }); }); it("does not return a worktree after Git reports an incomplete checkout", async () => { diff --git a/apps/server/src/features/projects/git/execution/__tests__/real-git-executor.test.ts b/apps/server/src/features/projects/git/execution/__tests__/real-git-executor.test.ts index f2b043f8f..13cd21d7d 100644 --- a/apps/server/src/features/projects/git/execution/__tests__/real-git-executor.test.ts +++ b/apps/server/src/features/projects/git/execution/__tests__/real-git-executor.test.ts @@ -23,7 +23,7 @@ describe("RealGitExecutor", () => { executor = new RealGitExecutor(); }); - it("serialises concurrent calls for the same cwd", async () => { + it("serialises concurrent mutating calls for the same cwd", async () => { const order: number[] = []; execFileMock.mockImplementation((_cmd, _args, _opts, cb) => { const id = order.length + 1; @@ -34,8 +34,8 @@ describe("RealGitExecutor", () => { }); const [first, second] = await Promise.all([ - executor.exec(["-C", "/repo", "status", "--porcelain"]), - executor.exec(["-C", "/repo", "rev-parse", "HEAD"]), + executor.exec(["-C", "/repo", "worktree", "add", "/wt", "-b", "mcode/x"]), + executor.exec(["-C", "/repo", "checkout", "-b", "other"]), ]); expect(order).toEqual([1, 2]); @@ -43,6 +43,59 @@ describe("RealGitExecutor", () => { expect(second.stdout).toBe("out-2"); }); + it("runs read-only calls without waiting on the queue", async () => { + execFileMock.mockImplementation((_cmd, _args, _opts, cb) => { + setTimeout(() => cb(null, { stdout: "ok\n", stderr: "" }), 10); + }); + + const first = executor.exec(["-C", "/repo", "status", "--porcelain"]); + const second = executor.exec(["-C", "/repo", "rev-parse", "HEAD"]); + + // execFile fires synchronously for unqueued commands; a queued call would + // only spawn after the previous operation resolved. + expect(execFileMock).toHaveBeenCalledTimes(2); + await expect(Promise.all([first, second])).resolves.toHaveLength(2); + }); + + it("lets read-only calls bypass a queue busy with a mutation", async () => { + let releaseMutation!: () => void; + const gate = new Promise((resolve) => { + releaseMutation = resolve; + }); + execFileMock.mockImplementation((_cmd, args, _opts, cb) => { + if ((args as string[]).includes("add")) { + void gate.then(() => cb(null, { stdout: "", stderr: "" })); + return; + } + cb(null, { stdout: "listed\n", stderr: "" }); + }); + + const mutation = executor.exec(["-C", "/repo", "add", "-A"]); + const listed = executor.exec(["-C", "/repo", "worktree", "list", "--porcelain"]); + + await expect(listed).resolves.toEqual({ stdout: "listed\n", stderr: "" }); + releaseMutation(); + await mutation; + }); + + it("keeps mutating forms of shared subcommands serialised", async () => { + const order: number[] = []; + execFileMock.mockImplementation((_cmd, _args, _opts, cb) => { + const id = order.length + 1; + setTimeout(() => { + order.push(id); + cb(null, { stdout: "", stderr: "" }); + }, 10); + }); + + const list = executor.exec(["-C", "/repo", "worktree", "list", "--porcelain"]); + const add = executor.exec(["-C", "/repo", "worktree", "add", "/wt", "-b", "mcode/x"]); + const force = executor.exec(["-C", "/repo", "branch", "-f", "mcode/x", "origin/mcode/x"]); + await Promise.all([list, add, force]); + + expect(order.indexOf(2)).toBeLessThan(order.indexOf(3)); + }); + it("caches rev-parse --git-dir only inside the queue", async () => { execFileMock.mockImplementation((_cmd, _args, _opts, cb) => { cb(null, { stdout: "/repo/.git\n", stderr: "" }); @@ -67,6 +120,23 @@ describe("RealGitExecutor", () => { expect(spawnMock).not.toHaveBeenCalled(); }); + it("does not invalidate the rev-parse cache for read-only commands", async () => { + execFileMock.mockImplementation((_cmd, args, _opts, cb) => { + const argv = args as string[]; + if (argv.includes("--git-dir")) { + cb(null, { stdout: "/repo/.git\n", stderr: "" }); + return; + } + cb(null, { stdout: "", stderr: "" }); + }); + + await executor.exec(["-C", "/repo", "rev-parse", "--git-dir"]); + await executor.exec(["-C", "/repo", "status", "--porcelain"]); + await executor.exec(["-C", "/repo", "rev-parse", "--git-dir"]); + + expect(execFileMock).toHaveBeenCalledTimes(2); + }); + it("invalidates rev-parse cache after mutating commands", async () => { execFileMock.mockImplementation((_cmd, args, _opts, cb) => { const argv = args as string[]; diff --git a/apps/server/src/features/projects/git/execution/real-git-executor.ts b/apps/server/src/features/projects/git/execution/real-git-executor.ts index 07ef149b7..8284924dd 100644 --- a/apps/server/src/features/projects/git/execution/real-git-executor.ts +++ b/apps/server/src/features/projects/git/execution/real-git-executor.ts @@ -1,7 +1,8 @@ /** * Production git executor. - * Wraps promisified execFile with per-repo serialisation, a configurable - * default timeout, and a transparent result cache for cheap rev-parse calls. + * Wraps promisified execFile with per-repo serialisation for mutating + * commands, a configurable default timeout, and a transparent result cache + * for cheap rev-parse calls. Read-only commands bypass the queue. */ import { injectable } from "tsyringe"; @@ -14,12 +15,94 @@ const execFile = NodeUtil.promisify(NodeChildProcess.execFile); /** Noop used to suppress unhandled-rejection warnings on queue chains. */ const noop = () => {}; +/** Git subcommands that never mutate repository state in any form. */ +const READ_ONLY_COMMANDS = new Set([ + "blame", + "cat-file", + "check-ignore", + "count-objects", + "describe", + "diff", + "for-each-ref", + "log", + "ls-files", + "ls-remote", + "ls-tree", + "merge-base", + "name-rev", + "rev-list", + "rev-parse", + "shortlog", + "show", + "show-ref", + "status", +]); + +/** Global git options that consume the following argument as their value. */ +const VALUE_TAKING_GLOBAL_FLAGS = new Set(["-C", "-c", "--git-dir", "--work-tree", "--exec-path"]); + +/** Flags that turn `git branch` into a mutation rather than a listing. */ +const BRANCH_MUTATION_FLAGS = new Set([ + "-d", "-D", "--delete", + "-m", "-M", "--move", + "-c", "-C", "--copy", + "-f", "--force", + "-u", "--set-upstream-to", "--unset-upstream", + "--edit-description", + "--track", "--no-track", +]); + +/** `git config` flags that read values instead of writing them. */ +const CONFIG_READ_FLAGS = new Set(["--get", "--get-all", "--get-regexp", "--get-urlmatch", "--list", "-l"]); + +/** Read-only arg shapes for subcommands that can also mutate. */ +const MIXED_COMMAND_READS: Record boolean> = { + worktree: (rest) => rest[0] === "list", + branch: (rest) => rest.every( + (arg) => arg.startsWith("-") && !BRANCH_MUTATION_FLAGS.has(arg.split("=", 1)[0]!), + ), + remote: (rest) => rest.length === 0 || rest[0] === "-v" || rest[0] === "get-url" || rest[0] === "show", + config: (rest) => rest.some((arg) => CONFIG_READ_FLAGS.has(arg)), + "symbolic-ref": (rest) => rest.some((arg) => arg === "--short" || arg === "-q" || arg === "--quiet") + && rest.filter((arg) => !arg.startsWith("-")).length <= 1, +}; + +/** + * Classify commands that cannot mutate repository state. Only these bypass + * the per-directory serialisation queue; every other command keeps the queue + * so index-, ref-, and worktree-mutating calls cannot race. + */ +function isReadOnlyGitCommand(args: readonly string[]): boolean { + const index = gitSubcommandIndex(args); + if (index < 0) return false; + const command = args[index]!; + const mixedRead = MIXED_COMMAND_READS[command]; + if (mixedRead) return mixedRead(args.slice(index + 1)); + return READ_ONLY_COMMANDS.has(command); +} + +/** Find the subcommand position after any leading global options like `-C`. */ +function gitSubcommandIndex(args: readonly string[]): number { + for (let i = 0; i < args.length; i += 1) { + const arg = args[i]!; + if (VALUE_TAKING_GLOBAL_FLAGS.has(arg)) { + i += 1; + continue; + } + if (arg.startsWith("-")) continue; + return i; + } + return -1; +} + /** * Production implementation of {@link GitExecutor}. * * Features: - * - Serialises concurrent git calls per effective working directory so that - * index-mutating operations (worktree add/remove, checkout) do not race. + * - Serialises concurrent mutating git calls per effective working directory + * so that index-mutating operations (worktree add/remove, checkout) do not + * race. Read-only commands run unqueued so a slow mutation cannot stall + * watchers and other queries on the same repository. * - Transparent LRU-style cache for `rev-parse --git-dir` and * `rev-parse --show-toplevel` results keyed by cwd. * - Default timeout of 10 s, overridable per call. @@ -39,28 +122,32 @@ export class RealGitExecutor implements GitExecutor { private readonly revParseCache = new Map(); /** - * Run `git` with the given arguments, serialising calls per effective cwd. + * Run `git` with the given arguments. Mutating commands are serialised per + * effective cwd; read-only commands run immediately. * Results of `rev-parse --git-dir` and `rev-parse --show-toplevel` are * cached transparently so repeated probe calls are free. */ async exec(args: string[], opts: GitExecOptions = {}): Promise { const cacheKey = this.getCacheKey(args); - const queueKey = this.getQueueKey(args, opts); + const readOnly = isReadOnlyGitCommand(args); - return this.enqueue(queueKey, async () => { - // Re-check cache inside the queue in case a concurrent queued operation - // already populated it while we were waiting. + const invoke = async (): Promise => { + // Re-check the cache inside the queue turn in case a concurrent queued + // operation already populated it while we were waiting. const cachedNow = cacheKey ? this.revParseCache.get(cacheKey) : undefined; if (cachedNow) return cachedNow; const result = await this.runGit(args, opts); if (cacheKey) { this.revParseCache.set(cacheKey, result); - } else { + } else if (!readOnly) { this.invalidateRevParseCacheForCwd(this.getEffectiveCwd(args, opts)); } return result; - }); + }; + + if (readOnly) return invoke(); + return this.enqueue(this.getQueueKey(args, opts), invoke); } // --------------------------------------------------------------------------- diff --git a/apps/server/src/features/projects/git/git-worktree-service.ts b/apps/server/src/features/projects/git/git-worktree-service.ts index fbbf91d28..24cd167ba 100644 --- a/apps/server/src/features/projects/git/git-worktree-service.ts +++ b/apps/server/src/features/projects/git/git-worktree-service.ts @@ -31,6 +31,7 @@ export interface RemoveWorktreeOptions { const PARENT_RMDIR_MAX_RETRIES = 5; const PARENT_RMDIR_RETRY_DELAY_MS = 300; +const WORKTREE_ADD_TIMEOUT_MS = 120_000; type WorktreeCreationRequest = { branch: string; @@ -220,7 +221,7 @@ export class GitWorktreeService { request: WorktreeCreationRequest, createdBranch: boolean, ): Promise { - await this.gitExecutor.exec(createWorktreeArgs(request, createdBranch), { timeout: 0 }); + await this.gitExecutor.exec(createWorktreeArgs(request, createdBranch), { timeout: WORKTREE_ADD_TIMEOUT_MS }); } private async createWorktreeRemovalRequest(