From c5b03481501f7b278506e887581378365a733059 Mon Sep 17 00:00:00 2001 From: robomp-bot Date: Wed, 29 Jul 2026 20:50:43 +0900 Subject: [PATCH] fix(coding-agent): harden async reftable resolution (cherry picked from commit e451f1d6f3537d4b58dfe479a6daf8919d169397) --- packages/coding-agent/CHANGELOG.md | 2 +- .../modes/components/status-line/component.ts | 43 ++++++---- packages/coding-agent/src/utils/git.ts | 9 ++- .../coding-agent/test/git-reftable.test.ts | 20 +++++ .../test/status-line-vcs-refresh.test.ts | 81 ++++++++++++++++++- 5 files changed, 134 insertions(+), 21 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1fcd9aac7..e8960a845 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed random multi-second TUI freezes in reftable-format repos: status-line branch resolution moved off the render path onto the async-with-cache shape its siblings use, and synchronous git spawns gained a 5 s deadline. +- Fixed random multi-second TUI freezes in reftable-format repos: status-line branch resolution moved off the render path onto the async-with-cache shape its siblings use, and synchronous git spawns gained a 5 s deadline ([#6997](https://github.com/can1357/oh-my-pi/pull/6997) by [@metaphorics](https://github.com/metaphorics)). ## [17.1.8] - 2026-07-28 diff --git a/packages/coding-agent/src/modes/components/status-line/component.ts b/packages/coding-agent/src/modes/components/status-line/component.ts index fde0da78c..ae4a649c2 100644 --- a/packages/coding-agent/src/modes/components/status-line/component.ts +++ b/packages/coding-agent/src/modes/components/status-line/component.ts @@ -174,6 +174,12 @@ interface ActiveRepoCache { worktree: WorktreeContext | null; } +interface BranchResolveRequest { + id: number; + cwd: string; + controller: AbortController; +} + interface WorktreeContext { /** Primary-checkout (project) name shown by the path segment. */ projectName: string; @@ -254,7 +260,7 @@ export class StatusLineComponent implements Component { // two live resolves can share a cwd string across an invalidation, and a // stale one must never free (or poison) a slot it no longer owns. #branchResolveSeq = 0; - #branchResolveActive: number | undefined = undefined; + #branchResolveActive: BranchResolveRequest | undefined = undefined; // Bumped on every branch-cache reset (#invalidateGitCaches — a HEAD move or // repo-context change). An in-flight reftable resolve captures this at // launch; a mismatch on resolve means the cache was invalidated underneath @@ -607,6 +613,8 @@ export class StatusLineComponent implements Component { dispose(): void { this.#disposed = true; + this.#branchResolveActive?.controller.abort(); + this.#branchResolveActive = undefined; this.#onBranchChange = null; this.#clearUsageStartTimer(); if (this.#gitWatcher) { @@ -638,10 +646,9 @@ export class StatusLineComponent implements Component { this.#cachedBranch = undefined; this.#cachedBranchRepoId = undefined; this.#cachedBranchCwd = undefined; - // Release the in-flight slot and bump the generation so any pending - // reftable resolve drops itself on resolve (see #getCurrentBranch): its - // result is now stale, and clearing the slot lets the next render start a - // fresh resolve immediately instead of waiting for the superseded one. + // Abort before releasing the in-flight slot. Releasing alone would allow + // repeated invalidations to fan out still-running git subprocesses. + this.#branchResolveActive?.controller.abort(); this.#branchResolveActive = undefined; this.#branchCacheGeneration++; this.#cachedPrContext = undefined; @@ -672,10 +679,16 @@ export class StatusLineComponent implements Component { const repository = git.repo.resolveSync(gitCwd); if (repository && git.repo.isReftableSync(repository)) { if (this.#branchResolveActive !== undefined) { - return this.#cachedBranchCwd === gitCwd ? (this.#cachedBranch ?? null) : null; + return this.#branchResolveActive.cwd === gitCwd && this.#cachedBranchCwd === gitCwd + ? (this.#cachedBranch ?? null) + : null; } - const requestId = ++this.#branchResolveSeq; - this.#branchResolveActive = requestId; + const request: BranchResolveRequest = { + id: ++this.#branchResolveSeq, + cwd: gitCwd, + controller: new AbortController(), + }; + this.#branchResolveActive = request; // Capture the cache generation at launch. #invalidateGitCaches bumps it // on a HEAD move and clears the in-flight slot, so a fresher resolve can // start while this one is still pending. Without a generation check the @@ -687,7 +700,7 @@ export class StatusLineComponent implements Component { let next: string | null = null; let repoId: string | null = null; try { - const headState = await git.head.resolve(gitCwd); + const headState = await git.head.resolve(gitCwd, request.controller.signal); repoId = headState?.headPath ?? null; next = !headState ? null @@ -701,7 +714,7 @@ export class StatusLineComponent implements Component { // invalidation a fresher resolve may hold it, and freeing that // slot here would let a third same-generation resolve launch and // race the fresh one to the cache commit. - if (this.#branchResolveActive === requestId) this.#branchResolveActive = undefined; + if (this.#branchResolveActive?.id === request.id) this.#branchResolveActive = undefined; } // Only the latest generation may update the cache; a mismatch means a // newer resolve superseded this one (or the component disposed). @@ -1281,11 +1294,11 @@ export class StatusLineComponent implements Component { : { projectDir, activeRepo: null, effectiveGitCwd: projectDir, worktree: null }; let gitBranch = includeGit || includePr ? this.#getCurrentBranch(activeRepoCache.effectiveGitCwd) : null; // A jj repo has no git branch to read: git HEAD is detached (colocated) or - // absent. Gate BOTH the jj branch label and the jj status counts on that - // same condition, captured before the label overlay rewrites gitBranch, so - // a nested ordinary git checkout under a parent jj workspace keeps its own - // git branch AND its own git status instead of the ancestor jj status. - const gitHeadIsJjLike = gitBranch === "detached" || gitBranch === null; + // absent. A pending reftable resolve owns this cwd as an explicit Git repo, + // so it must not be mistaken for an absent Git checkout and fall through to + // an ancestor jj workspace. + const gitHeadResolvePending = this.#branchResolveActive?.cwd === activeRepoCache.effectiveGitCwd; + const gitHeadIsJjLike = !gitHeadResolvePending && (gitBranch === "detached" || gitBranch === null); if (includeGit && gitHeadIsJjLike) { gitBranch = this.#getJjBranch(activeRepoCache.effectiveGitCwd) ?? gitBranch; } diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 617ec6b0b..b82c2b030 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -428,10 +428,11 @@ function gitSpawnSyncText( windowsHide: true, timeout: timeoutMs, }); - // `null` exitCode means the child was killed (deadline or signal) rather - // than exiting normally; report it as a timeout so read-only callers - // degrade instead of treating partial/empty stdout as success. - const exitCode = result.exitCode ?? GIT_COMMAND_TIMEOUT_EXIT_CODE; + // Bun's timeout marker is authoritative even when process cleanup reports + // exit code zero, so render-path callers never trust partial output. + const exitCode = result.exitedDueToTimeout + ? GIT_COMMAND_TIMEOUT_EXIT_CODE + : (result.exitCode ?? GIT_COMMAND_TIMEOUT_EXIT_CODE); return { exitCode, stdout: new TextDecoder().decode(result.stdout).trim() }; } catch (err) { if (isEnoent(err)) return { exitCode: GIT_SPAWN_ENOENT_EXIT_CODE, stdout: "" }; diff --git a/packages/coding-agent/test/git-reftable.test.ts b/packages/coding-agent/test/git-reftable.test.ts index 3dea84e27..5b05c4c15 100644 --- a/packages/coding-agent/test/git-reftable.test.ts +++ b/packages/coding-agent/test/git-reftable.test.ts @@ -123,6 +123,26 @@ describe.skipIf(!supportsReftable)("git reftable support", () => { } }); + test("head.resolveSync treats Bun's timeout marker as a failed symbolic-ref even with exit code zero", () => { + const baseResult = Bun.spawnSync(["true"], { stdout: "pipe", stderr: "pipe" }); + const timedOutSymbolicRef = { + ...baseResult, + exitCode: 0, + exitedDueToTimeout: true, + stdout: Buffer.from("refs/heads/feature-branch\n"), + } satisfies ReturnType; + const successfulRevParse = { + ...baseResult, + exitCode: 0, + stdout: Buffer.from(`${headSha}\n`), + } satisfies ReturnType; + vi.spyOn(Bun, "spawnSync").mockReturnValueOnce(timedOutSymbolicRef).mockReturnValueOnce(successfulRevParse); + + const headState = git.head.resolveSync(sharedRepoDir); + expect(headState?.kind).toBe("detached"); + expect(headState?.commit).toBe(headSha); + }); + test("handles git config trailing comments correctly", async () => { const repository = await git.repo.resolve(configRepoDir); expect(repository).not.toBeNull(); diff --git a/packages/coding-agent/test/status-line-vcs-refresh.test.ts b/packages/coding-agent/test/status-line-vcs-refresh.test.ts index 1b9970cea..4594bc4ce 100644 --- a/packages/coding-agent/test/status-line-vcs-refresh.test.ts +++ b/packages/coding-agent/test/status-line-vcs-refresh.test.ts @@ -12,6 +12,9 @@ * same callback is covered by status-line-dispose-async-leak.test.ts.) */ import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import type { StatusLineSettings } from "@oh-my-pi/pi-coding-agent/modes/components/status-line"; import { StatusLineComponent } from "@oh-my-pi/pi-coding-agent/modes/components/status-line"; @@ -174,7 +177,7 @@ describe("StatusLineComponent reftable branch resolve honors mid-flight invalida gitEntryPath: "/fake/.git", headPath: "/fake/.git/HEAD", repoRoot: "/fake", - } as GitRepository; + } satisfies GitRepository; vi.spyOn(git.repo, "resolveSync").mockReturnValue(fakeRepo); vi.spyOn(git.repo, "isReftableSync").mockReturnValue(true); // Keep the sibling async fetches quiet so only the branch resolve drives @@ -237,4 +240,80 @@ describe("StatusLineComponent reftable branch resolve honors mid-flight invalida component.dispose(); }); + + it("aborts an invalidated resolve and starts only one replacement resolve", async () => { + const fakeRepo = { + commonDir: "/fake/.git", + gitDir: "/fake/.git", + gitEntryPath: "/fake/.git", + headPath: "/fake/.git/HEAD", + repoRoot: "/fake", + } satisfies GitRepository; + vi.spyOn(git.repo, "resolveSync").mockReturnValue(fakeRepo); + vi.spyOn(git.repo, "isReftableSync").mockReturnValue(true); + vi.spyOn(git.status, "summary").mockReturnValue(Promise.withResolvers().promise); + vi.spyOn(jj.repo, "rootSync").mockReturnValue(null); + + const signals: AbortSignal[] = []; + vi.spyOn(git.head, "resolve").mockImplementation((_cwd, signal) => { + if (!signal) throw new Error("reftable resolve must receive an abort signal"); + signals.push(signal); + return new Promise((_resolve, reject) => { + signal.addEventListener("abort", () => reject(signal.reason), { once: true }); + }); + }); + + const component = new StatusLineComponent(makeSession()); + component.updateSettings(gitSegment); + component.getTopBorder(80); + expect(git.head.resolve).toHaveBeenCalledTimes(1); + + component.invalidate(); + expect(signals[0]?.aborted).toBe(true); + component.invalidate(); + component.getTopBorder(80); + component.getTopBorder(80); + expect(git.head.resolve).toHaveBeenCalledTimes(2); + + component.dispose(); + expect(signals[1]?.aborted).toBe(true); + await Promise.resolve(); + }); + + it("does not query an ancestor jj workspace while nested Git HEAD resolution is pending", async () => { + const jjRootDir = await fs.mkdtemp(path.join(os.tmpdir(), "status-line-jj-root-")); + const nestedGitCwd = path.join(jjRootDir, "nested-ordinary-git"); + await fs.mkdir(nestedGitCwd); + const fakeRepo = { + commonDir: `${nestedGitCwd}/.git`, + gitDir: `${nestedGitCwd}/.git`, + gitEntryPath: `${nestedGitCwd}/.git`, + headPath: `${nestedGitCwd}/.git/HEAD`, + repoRoot: nestedGitCwd, + } satisfies GitRepository; + vi.spyOn(git.repo, "resolveSync").mockReturnValue(fakeRepo); + vi.spyOn(git.repo, "isReftableSync").mockReturnValue(true); + vi.spyOn(git.head, "resolve").mockReturnValue(Promise.withResolvers().promise); + vi.spyOn(git.status, "summary").mockReturnValue(Promise.withResolvers().promise); + const jjRoot = vi.spyOn(jj.repo, "rootSync").mockReturnValue("/workspace/jj-root"); + const jjLabel = vi.spyOn(jj.workingCopy, "label").mockReturnValue(Promise.resolve("ancestor-bookmark")); + const jjStatus = vi + .spyOn(jj.status, "summary") + .mockReturnValue(Promise.resolve({ staged: 0, unstaged: 0, untracked: 0 })); + setProjectDir(nestedGitCwd); + + try { + const component = new StatusLineComponent(makeSession()); + component.updateSettings(gitSegment); + component.getTopBorder(80); + expect(git.head.resolve).toHaveBeenCalledTimes(1); + expect(jjRoot).not.toHaveBeenCalled(); + expect(jjLabel).not.toHaveBeenCalled(); + expect(jjStatus).not.toHaveBeenCalled(); + component.dispose(); + } finally { + setProjectDir(originalProjectDir); + await fs.rm(jjRootDir, { recursive: true, force: true }); + } + }); });