smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): harden async reftable resolution

Browse Source 
(cherry picked from commit e451f1d6f3537d4b58dfe479a6daf8919d169397)
This commit is contained in:
robomp-bot
2026-07-29 20:50:43 +09:00
committed by can1357
parent d966b3f8a0
commit c5b0348150
5 changed files with 134 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+1 -1
View File
@@ -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
packages/coding-agent/src/modes/components/status-line/component.ts
+28 -15
View File
@@ -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;
}
packages/coding-agent/src/utils/git.ts
+5 -4
View File
@@ -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: "" };
packages/coding-agent/test/git-reftable.test.ts
+20
View File
@@ -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<typeof Bun.spawnSync>;
const successfulRevParse = {
...baseResult,
exitCode: 0,
stdout: Buffer.from(`${headSha}\n`),
} satisfies ReturnType<typeof Bun.spawnSync>;
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();
packages/coding-agent/test/status-line-vcs-refresh.test.ts
+80 -1
View File
@@ -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<GitStatus | null>().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<GitHeadState | null>((_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<GitHeadState | null>().promise);
vi.spyOn(git.status, "summary").mockReturnValue(Promise.withResolvers<GitStatus | null>().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 });
}
});
});