From 17ec0d4f948efc01122c1aa07cf79fb480d3a892 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:14:45 -0700 Subject: [PATCH] fix(worktree): retain index warming ownership until group exit --- src/main/worktree-create-preparation.test.ts | 2 +- .../worktree-index-warming-ownership.test.ts | 31 ++++++++++++++++- src/main/worktree-index-warming-ownership.ts | 9 ++++- .../worktree-prepared-index-warming.test.ts | 34 ++++++++++++++++++- src/main/worktree-prepared-index-warming.ts | 11 ++++-- src/renderer/src/app-startup-routing.test.ts | 6 +++- 6 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/main/worktree-create-preparation.test.ts b/src/main/worktree-create-preparation.test.ts index c134f29a857..82b12f371e3 100644 --- a/src/main/worktree-create-preparation.test.ts +++ b/src/main/worktree-create-preparation.test.ts @@ -25,7 +25,7 @@ vi.mock('./worktree-index-warming-ownership', () => ({ WorktreeIndexWarmingOwnership: class { arm = vi.fn().mockResolvedValue(undefined) recordPid = vi.fn() - release = vi.fn().mockResolvedValue(undefined) + release = vi.fn().mockResolvedValue(true) }, canReclaimIndexWarming: vi.fn().mockResolvedValue(true), removeIndexWarmingOwnership: vi.fn().mockResolvedValue(undefined) diff --git a/src/main/worktree-index-warming-ownership.test.ts b/src/main/worktree-index-warming-ownership.test.ts index aaf30a55bfc..342037ec07e 100644 --- a/src/main/worktree-index-warming-ownership.test.ts +++ b/src/main/worktree-index-warming-ownership.test.ts @@ -24,10 +24,39 @@ it('retains the pre-spawn window and clears ownership only after PID persistence await owner.arm() expect(await canReclaimIndexWarming(prepared)).toBe(false) owner.recordPid(12345) - await owner.release() + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('probe'), { code: 'ESRCH' }) + }) + expect(await owner.release()).toBe(true) await expect(readFile(`${prepared}.index-warming`)).rejects.toMatchObject({ code: 'ENOENT' }) expect(await canReclaimIndexWarming(prepared)).toBe(true) }) +it('releases a never-spawned owner without probing a process group', async () => { + const owner = new WorktreeIndexWarmingOwnership(prepared) + await owner.arm() + const kill = vi.spyOn(process, 'kill') + expect(await owner.release()).toBe(true) + expect(kill).not.toHaveBeenCalled() + await expect(readFile(`${prepared}.index-warming`)).rejects.toMatchObject({ code: 'ENOENT' }) +}) +it.each(['EPERM', undefined])( + 'retains the marker after root exit while the group is live or unverifiable (%s)', + async (code) => { + const owner = new WorktreeIndexWarmingOwnership(prepared) + await owner.arm() + owner.recordPid(12345) + const kill = vi.spyOn(process, 'kill').mockImplementation(() => { + if (code) { + throw Object.assign(new Error('probe'), { code }) + } + return true + }) + expect(await owner.release()).toBe(false) + expect(kill).toHaveBeenCalledExactlyOnceWith(-12345, 0) + expect(await readFile(`${prepared}.index-warming`, 'utf8')).toBe('12345\n') + expect(await canReclaimIndexWarming(prepared)).toBe(false) + } +) it('does not overwrite another ownership record', async () => { await writeFile(`${prepared}.index-warming`, '12345\n') await expect(new WorktreeIndexWarmingOwnership(prepared).arm()).rejects.toMatchObject({ diff --git a/src/main/worktree-index-warming-ownership.ts b/src/main/worktree-index-warming-ownership.ts index d4b0d70cb4a..5813390f337 100644 --- a/src/main/worktree-index-warming-ownership.ts +++ b/src/main/worktree-index-warming-ownership.ts @@ -8,6 +8,7 @@ function markerPath(preparedPath: string): string { /** The pending marker precedes spawn; an interrupted PID write remains unverifiable. */ export class WorktreeIndexWarmingOwnership { private pidWrite: Promise = Promise.resolve() + private pid: number | undefined constructor(private readonly preparedPath: string) {} arm(): Promise { @@ -15,12 +16,18 @@ export class WorktreeIndexWarmingOwnership { } recordPid(pid: number): void { + this.pid = pid this.pidWrite = writeFile(markerPath(this.preparedPath), `${pid}\n`).catch(() => {}) } - async release(): Promise { + // Git hooks can outlive the root; retain ownership until the whole group has exited. + async release(): Promise { await this.pidWrite + if (this.pid !== undefined && !hasExitedPosixProcessGroup(this.pid)) { + return false + } await rm(markerPath(this.preparedPath), { force: true }) + return true } } diff --git a/src/main/worktree-prepared-index-warming.test.ts b/src/main/worktree-prepared-index-warming.test.ts index 9ebf1677232..0d09919d451 100644 --- a/src/main/worktree-prepared-index-warming.test.ts +++ b/src/main/worktree-prepared-index-warming.test.ts @@ -54,7 +54,7 @@ beforeEach(() => { mocks.git.mockReset().mockResolvedValue({ stdout: '' }) mocks.arm.mockReset().mockResolvedValue(undefined) mocks.recordPid.mockReset() - mocks.release.mockReset().mockResolvedValue(undefined) + mocks.release.mockReset().mockResolvedValue(true) }) afterEach(async () => { await _resetPreparationPoolForTests() @@ -226,3 +226,35 @@ it('retains the checkout if the ownership marker cannot be removed', async () => await vi.advanceTimersByTimeAsync(1_100) await expect(takePreparation(listPreparations()[0])).resolves.toBe(false) }) +it('successful Git exit with a live group blocks claim, retains ownership and disables warming', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + mocks.release.mockResolvedValue(false) + await startPreparation(args) + await vi.advanceTimersByTimeAsync(1_100) + expect(mocks.release).toHaveBeenCalledOnce() + await expect(takePreparation(listPreparations()[0])).resolves.toBe(false) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('process group live or unverifiable')) + await startPreparation(args) + await vi.advanceTimersByTimeAsync(2_000) + expect(mocks.git).toHaveBeenCalledOnce() + expect(mocks.discard).not.toHaveBeenCalled() +}) +it('does not discard an expired preparation whose group is live after Git success', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + mocks.release.mockResolvedValue(false) + await startPreparation(args) + await vi.advanceTimersByTimeAsync(WORKTREE_CREATE_PREPARATION_TTL_MS) + expect(mocks.discard).not.toHaveBeenCalled() +}) +it('a refresh failure still requires the spawned group to have exited', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + mocks.git.mockImplementation(async (_args, options) => { + options.onChildSpawned(12345) + throw new Error('index refresh failed') + }) + mocks.release.mockResolvedValue(false) + await startPreparation(args) + await vi.advanceTimersByTimeAsync(1_100) + expect(mocks.recordPid).toHaveBeenCalledWith(12345) + await expect(takePreparation(listPreparations()[0])).resolves.toBe(false) +}) diff --git a/src/main/worktree-prepared-index-warming.ts b/src/main/worktree-prepared-index-warming.ts index 56b255e2e92..1c5270544c8 100644 --- a/src/main/worktree-prepared-index-warming.ts +++ b/src/main/worktree-prepared-index-warming.ts @@ -81,13 +81,20 @@ export function createPreparedIndexWarming( if (terminationUnverifiable) { return false } + let released: boolean try { - await ownership.release() - return true + released = await ownership.release() } catch { disablePreparedIndexWarming() return false } + if (!released) { + disablePreparedIndexWarming() + console.warn( + `[worktree-create] index warming process group live or unverifiable after Git exit; retaining ${preparedPath} and disabling warming` + ) + } + return released })() }, INDEX_TIMESTAMP_AGE_MS) timer.unref() diff --git a/src/renderer/src/app-startup-routing.test.ts b/src/renderer/src/app-startup-routing.test.ts index fead2f6c7bb..31a1c5399b3 100644 --- a/src/renderer/src/app-startup-routing.test.ts +++ b/src/renderer/src/app-startup-routing.test.ts @@ -411,7 +411,11 @@ describe('renderer startup runtime routing', () => { const shellSource = readSource(WORKSPACE_SHELL_PATH) const layoutSource = readSource(CHROME_LAYOUT_PATH) - expect(shellSource).toContain("const Terminal = lazy(() => import('../components/Terminal'))") + const loaderSource = readSource('src/renderer/src/lib/terminal-component-loader.ts') + expect(shellSource).toContain("from '@/lib/terminal-component-loader'") + expect(shellSource).toContain('const Terminal = lazy(loadTerminalComponent)') + expect(loaderSource).toContain("() => import('../components/Terminal')") + expect(loaderSource).not.toContain("from '../components/Terminal'") expect(shellSource).not.toContain("from '../components/Terminal'") expect(layoutSource).toContain( 'const canMountTerminalWorkbenchNow = activeWorktreeId !== null || backgroundTerminalMountRequested'