fix(agent-manager): release worktree locks on Windows

This commit is contained in:
marius-kilocode
2026-08-26 14:58:34 +02:00
parent ed3380ea49
commit 32e4b0a39e
13 changed files with 394 additions and 19 deletions
@@ -0,0 +1,5 @@
---
"kilo-code": patch
---
Release worktree processes and terminals before removing Agent Manager worktrees on Windows.
+22
View File
@@ -53,3 +53,25 @@ jobs:
- name: Check for kilocode_change markers
working-directory: packages/kilo-vscode
run: bun run check-kilocode-change
windows-worktree:
name: Windows worktree cleanup
runs-on: blacksmith-4vcpu-windows-2025
timeout-minutes: 20
defaults:
run:
shell: bash
steps:
- name: Checkout repository
uses: actions/checkout@v6
with:
token: ${{ secrets.GITHUB_TOKEN }}
- name: Setup Bun
uses: ./.github/actions/setup-bun
- name: Run Windows worktree cleanup tests
working-directory: packages/kilo-vscode
run: |
bun test tests/unit/worktree-manager.test.ts --test-name-pattern "Windows|directory remains locked" --timeout 30000
bun test tests/unit/session-terminal-manager.test.ts tests/unit/pty-cleanup.test.ts tests/unit/run-script-manager.test.ts --timeout 30000
@@ -1033,11 +1033,11 @@ export class AgentManagerProvider implements Disposable {
return acquirePtyCleanup({
directory,
terminals: this.terminalRouter,
integrated: this.terminalManager,
scripts: this.scripts.manager,
getClient: (dir) => this.connectionService.getClientAsync(dir),
})
}
private async discardWorktree(id: string, dir: string, branch: string, sessionId?: string): Promise<void> {
const ctx = this.context
if (!ctx) return
@@ -1,4 +1,5 @@
import type { WorktreeStateManager } from "./WorktreeStateManager"
import { normalizePath } from "./git-import"
// ---------------------------------------------------------------------------
// TerminalHost — narrow interface for the VS Code capabilities this module
@@ -225,6 +226,17 @@ export class SessionTerminalManager {
return (sessionId !== undefined && active === SessionTerminalManager.sessionKey(sessionId)) || active === key
}
closeDirectory(directory: string): void {
const target = normalizePath(directory)
for (const [key, entry] of this.terminals) {
if (normalizePath(entry.cwd) !== target) continue
this.terminals.delete(key)
entry.terminal.dispose()
this.log(`Removed terminal mapping for ${key} (worktree deleted)`)
}
this.updateContextKey()
}
dispose(): void {
void this.host.setContext("kilo-code.agentTerminalFocus", false)
for (const entry of this.terminals.values()) entry.terminal.dispose()
@@ -470,10 +470,15 @@ export class WorktreeManager {
const temp = path.join(path.dirname(worktreePath), `.kilo-delete-${randomUUID()}`)
try {
await fs.promises.rename(worktreePath, temp)
} catch {
// Rename failed (e.g. locked files on Windows) — fall back to force remove
this.log(`Rename failed, falling back to force remove: ${worktreePath}`)
await this.git.raw(["worktree", "remove", "--force", worktreePath]).catch(() => {})
} catch (err) {
this.log(`Rename failed, falling back to force remove: ${worktreePath}: ${err}`)
await this.git.raw(["worktree", "remove", "--force", worktreePath]).catch((error: unknown) => {
this.log(`Git worktree removal failed for ${worktreePath}: ${error}`)
})
if (fs.existsSync(worktreePath)) await fs.promises.rm(worktreePath, RM_OPTS)
await this.git.raw(["worktree", "prune", "--expire", "now"]).catch((error: unknown) => {
this.log(`Failed to prune worktree metadata for ${worktreePath}: ${error}`)
})
if (branch) await this.deleteBranch(branch)
return
}
@@ -114,33 +114,44 @@ export async function deleteLifecycleWorktree(
host.log(`Worktree ${worktreeId} not found in state`)
return null
}
// Stop pollers before cleanup. State is removed only after PTYs and disk are gone so a failed
// process cleanup cannot leave a live shell rooted in an untracked worktree.
const sessions = state.getSessions(worktreeId)
const fail = (message: string) => {
host.unskipStats(worktreeId)
host.post({ type: "agentManager.worktreeSetup", status: "error", message, worktreeId })
}
host.skipStats(worktreeId)
host.stopDiffs(worktree.path, sessions)
await host.removeRun(worktreeId)
if (!(await host.clearRun(worktreeId))) {
host.unskipStats(worktreeId)
host.post({ type: "error", message: "Failed to stop the Run script before deleting the worktree" })
fail("Failed to stop the Run script before deleting the worktree")
return null
}
const branch = worktree.branchOwned === false ? undefined : (worktree.originalBranch ?? worktree.branch)
let releasePtyCleanup: () => void
try {
if (sessions.length > 0) {
await host.sessions.abort(sessions.map((session) => session.id))
const client = host.client()
await Promise.all(sessions.map((session) => stopSessionProcesses(client, session.id, worktree.path)))
}
releasePtyCleanup = await host.acquirePtyCleanup(worktree.path)
} catch (error) {
host.log(`Failed to remove worktree from disk: ${error}`)
host.unskipStats(worktreeId)
host.log(`Failed to stop worktree processes: ${error}`)
fail(`Failed to stop worktree processes: ${getErrorMessage(error)}`)
return null
}
try {
await host.client().instance.dispose({ directory: worktree.path }, { throwOnError: true })
await ctx.worktreeManager().removeWorktree(worktree.path, branch)
const orphaned = state.removeWorktree(worktreeId)
host.removePR(worktreeId)
host.forgetName(worktreeId)
host.stopDiffs(worktree.path, orphaned)
for (const s of orphaned) host.sessions.clearDirectory(s.id)
for (const session of orphaned) host.sessions.clearDirectory(session.id)
host.push()
host.log(`Deleted worktree ${worktreeId}${branch ? ` (${branch})` : ""}`)
} catch (error) {
host.log(`Failed to remove worktree from disk: ${error}`)
fail(`Failed to delete worktree: ${getErrorMessage(error)}`)
} finally {
releasePtyCleanup()
}
@@ -1,5 +1,6 @@
import type { KiloClient } from "@kilocode/sdk/v2/client"
import type { ScriptTerminalManager } from "./ScriptTerminalManager"
import type { SessionTerminalManager } from "./SessionTerminalManager"
import type { TerminalRouter } from "./terminal-routing"
export async function removePtys(
@@ -24,6 +25,7 @@ export async function removePtys(
export async function acquirePtyCleanup(input: {
directory: string
terminals: TerminalRouter
integrated: SessionTerminalManager
scripts: ScriptTerminalManager
getClient: (directory: string) => Promise<KiloClient>
}) {
@@ -32,6 +34,7 @@ export async function acquirePtyCleanup(input: {
input.scripts.blockDirectory(input.directory),
])
try {
input.integrated.closeDirectory(input.directory)
await input.terminals.closeDirectory(input.directory)
await input.scripts.closeDirectory(input.directory)
await removePtys(input.getClient, input.directory)
@@ -10,6 +10,7 @@ import * as vscode from "vscode"
import type { RunHandle } from "./manager"
const GRACE_MS = 250
const STOP_TIMEOUT_MS = 5_000
export interface RunTaskConfig {
worktreeId: string
@@ -45,6 +46,11 @@ export async function startVscodeRunTask(config: RunTaskConfig, done: (exit: Run
}
const execution = await vscode.tasks.executeTask(task)
const ended: { resolve?: () => void; reject?: (error: Error) => void } = {}
const exit = new Promise<void>((resolve, reject) => {
ended.resolve = resolve
ended.reject = reject
})
let closed = false
let cleaned = false
let grace: ReturnType<typeof setTimeout> | undefined
@@ -55,12 +61,14 @@ export async function startVscodeRunTask(config: RunTaskConfig, done: (exit: Run
processListener.dispose()
endListener.dispose()
if (grace) clearTimeout(grace)
if (!closed) ended.resolve?.()
}
const finish = (exit: RunTaskExit = {}) => {
if (closed) return
closed = true
cleanup()
ended.resolve?.()
done(exit)
}
@@ -74,8 +82,21 @@ export async function startVscodeRunTask(config: RunTaskConfig, done: (exit: Run
grace = setTimeout(() => finish(), GRACE_MS)
})
if (!vscode.tasks.taskExecutions.includes(execution)) finish()
return {
stop: () => execution.terminate(),
stop: async () => {
if (closed) return
execution.terminate()
const timeout = setTimeout(() => {
ended.reject?.(new Error(`Run task did not stop: ${config.branch}`))
}, STOP_TIMEOUT_MS)
try {
await exit
} finally {
clearTimeout(timeout)
}
},
dispose: cleanup,
}
}
@@ -611,6 +611,11 @@ describe("Agent Manager Provider — onMessage routing", () => {
it("keeps the legacy integrated Run adapter isolated and removable", () => {
const task = fs.readFileSync(RUN_TASK_FILE, "utf-8")
expect(task).toContain("vscode.tasks.executeTask")
expect(task).toContain("execution.terminate()")
expect(task).toContain("await exit")
expect(task).toContain("STOP_TIMEOUT_MS")
expect(task).toContain("ended.resolve?.()")
expect(task.indexOf("vscode.tasks.onDidEndTaskProcess")).toBeLessThan(task.indexOf("vscode.tasks.taskExecutions"))
expect(task).toContain("Remove this")
const dest = fs.readFileSync(RUN_DESTINATION_FILE, "utf-8")
expect(dest).not.toContain('from "vscode"')
@@ -1,9 +1,12 @@
import { describe, expect, it } from "bun:test"
import type { KiloClient } from "@kilocode/sdk/v2/client"
import type { ProjectContext } from "../../src/agent-manager/project/context"
import type { LifecycleHost } from "../../src/agent-manager/provider-lifecycle"
import { deleteLifecycleWorktree, type LifecycleHost } from "../../src/agent-manager/provider-lifecycle"
import { discardWorktree } from "../../src/agent-manager/discard-worktree"
import { removePtys } from "../../src/agent-manager/pty-cleanup"
import { acquirePtyCleanup, removePtys } from "../../src/agent-manager/pty-cleanup"
import type { ScriptTerminalManager } from "../../src/agent-manager/ScriptTerminalManager"
import type { SessionTerminalManager } from "../../src/agent-manager/SessionTerminalManager"
import type { TerminalRouter } from "../../src/agent-manager/terminal-routing"
describe("Agent Manager PTY cleanup", () => {
it("removes every listed PTY even when one removal fails", async () => {
@@ -36,6 +39,56 @@ describe("Agent Manager PTY cleanup", () => {
await expect(removePtys(async () => client, "/worktree")).rejects.toThrow("offline")
})
it("closes integrated terminals before removing embedded worktree PTYs", async () => {
const calls: string[] = []
const client = {
v2: {
pty: {
list: async () => {
calls.push("list")
return { data: { data: [] } }
},
},
},
} as unknown as KiloClient
const terminals = {
blockDirectory: async () => {
calls.push("block-terminals")
return () => calls.push("release-terminals")
},
closeDirectory: async () => calls.push("close-terminals"),
} as unknown as TerminalRouter
const scripts = {
blockDirectory: async () => {
calls.push("block-scripts")
return () => calls.push("release-scripts")
},
closeDirectory: async () => calls.push("close-scripts"),
} as unknown as ScriptTerminalManager
const integrated = {
closeDirectory: (dir: string) => calls.push(`integrated:${dir}`),
} as unknown as SessionTerminalManager
const release = await acquirePtyCleanup({
directory: "/worktree",
terminals,
integrated,
scripts,
getClient: async () => client,
})
expect(calls).toEqual([
"block-terminals",
"block-scripts",
"integrated:/worktree",
"close-terminals",
"close-scripts",
"list",
])
release()
expect(calls.slice(-2)).toEqual(["release-terminals", "release-scripts"])
})
it("blocks worktree deletion when PTY cleanup fails", async () => {
const calls: string[] = []
const ctx = {
@@ -96,4 +149,121 @@ describe("Agent Manager PTY cleanup", () => {
await discardWorktree(ctx, host, "wt-1", "/worktree", "branch", "session-1")
expect(calls).toEqual(["log", "disk", "state", "push", "release"])
})
it("stops worktree sessions and disposes backend resources before deleting disk state", async () => {
const calls: string[] = []
const sessions = [{ id: "session-a" }, { id: "session-b" }]
const state = {
getWorktree: () => ({ path: "/worktree", branch: "branch" }),
getSessions: () => sessions,
removeWorktree: () => {
calls.push("state")
return sessions
},
}
const ctx = {
peekState: () => state,
worktreeManager: () => ({ removeWorktree: async () => calls.push("disk") }),
} as unknown as ProjectContext
const client = {
backgroundProcess: {
stopSession: async (input: { sessionID: string; directory: string }) => {
expect(input.directory).toBe("/worktree")
calls.push(`process:${input.sessionID}`)
},
},
instance: {
dispose: async (input: { directory: string }) => {
expect(input.directory).toBe("/worktree")
calls.push("instance")
},
},
} as unknown as KiloClient
const host = {
sessions: {
abort: async (ids: string[]) => calls.push(`abort:${ids.join(",")}`),
clearDirectory: (id: string) => calls.push(`clear:${id}`),
},
skipStats: () => calls.push("skip"),
stopDiffs: () => calls.push("diffs"),
removeRun: async () => calls.push("run"),
clearRun: async () => {
calls.push("scripts")
return true
},
acquirePtyCleanup: async () => {
calls.push("pty")
return () => calls.push("release")
},
client: () => client,
removePR: () => calls.push("pr"),
forgetName: () => calls.push("name"),
push: () => calls.push("push"),
log: () => undefined,
} as unknown as LifecycleHost
await deleteLifecycleWorktree(ctx, host, "wt-1")
expect(calls).toEqual([
"skip",
"diffs",
"run",
"scripts",
"abort:session-a,session-b",
"process:session-a",
"process:session-b",
"pty",
"instance",
"disk",
"state",
"pr",
"name",
"clear:session-a",
"clear:session-b",
"push",
"release",
])
})
it("preserves worktree state and reports a directory that remains locked", async () => {
const calls: string[] = []
const messages: unknown[] = []
const state = {
getWorktree: () => ({ path: "/worktree", branch: "branch" }),
getSessions: () => [],
removeWorktree: () => calls.push("state"),
}
const ctx = {
peekState: () => state,
worktreeManager: () => ({
removeWorktree: async () => {
calls.push("disk")
throw new Error("directory busy")
},
}),
} as unknown as ProjectContext
const host = {
skipStats: () => calls.push("skip"),
unskipStats: () => calls.push("unskip"),
stopDiffs: () => calls.push("diffs"),
removeRun: async () => undefined,
clearRun: async () => true,
acquirePtyCleanup: async () => () => calls.push("release"),
client: () => ({ instance: { dispose: async () => calls.push("instance") } }) as unknown as KiloClient,
post: (message: unknown) => messages.push(message),
log: () => undefined,
} as unknown as LifecycleHost
await deleteLifecycleWorktree(ctx, host, "wt-1")
expect(calls).toEqual(["skip", "diffs", "instance", "disk", "unskip", "release"])
expect(messages).toEqual([
{
type: "agentManager.worktreeSetup",
status: "error",
message: "Failed to delete worktree: directory busy",
worktreeId: "wt-1",
},
])
})
})
@@ -129,6 +129,28 @@ describe("RunScriptManager", () => {
expect(ctx.manager.all()).toEqual([])
})
it("waits for the run process to exit before completing worktree removal", async () => {
const ctx = createManager()
const gate = deferred<void>()
const calls: string[] = []
await ctx.manager.start("wt-1", async () => ({
stop: async () => {
calls.push("stop")
await gate.promise
calls.push("exit")
},
dispose: () => calls.push("dispose"),
}))
const removed = ctx.manager.remove("wt-1").then(() => calls.push("removed"))
await Promise.resolve()
expect(calls).toEqual(["stop"])
gate.resolve()
await removed
expect(calls).toEqual(["stop", "exit", "dispose", "removed"])
})
it("stops and disposes once when removal races startup", async () => {
const ctx = createManager()
const gate = deferred<RunHandle>()
@@ -189,6 +189,7 @@ describe("SessionTerminalManager command restoration", () => {
describe("SessionTerminalManager worktree terminals", () => {
function scene(opts: { worktreePath?: string; repoPath?: string } = {}) {
const created: Array<{ cwd: string; name: string }> = []
const disposed: string[] = []
const warnings: string[] = []
let shown = 0
const host: TerminalHost = {
@@ -196,7 +197,7 @@ describe("SessionTerminalManager worktree terminals", () => {
created.push(o)
return {
show: () => shown++,
dispose() {},
dispose: () => disposed.push(o.cwd),
exitStatus: undefined,
}
},
@@ -210,11 +211,13 @@ describe("SessionTerminalManager worktree terminals", () => {
executeCommand: () => Promise.resolve(),
}
const state = {
directoryFor: () => opts.worktreePath,
getSession: (id: string) => (opts.worktreePath ? { id, worktreeId: "wt-1" } : undefined),
getWorktree: (id: string) =>
opts.worktreePath ? { id, path: opts.worktreePath, branch: "feature/x" } : undefined,
} as unknown as WorktreeStateManager
const manager = new SessionTerminalManager(() => {}, host)
return { manager, state, created, warnings, shown: () => shown }
return { manager, state, created, disposed, warnings, shown: () => shown }
}
it("creates a terminal rooted at the worktree path", () => {
@@ -255,4 +258,29 @@ describe("SessionTerminalManager worktree terminals", () => {
expect(s.created).toHaveLength(0)
expect(s.warnings).toHaveLength(1)
})
it("closes session and worktree terminals without closing local terminals", () => {
const s = scene({ worktreePath: "/repo/.kilo/worktrees/wt-1", repoPath: "/repo" })
s.manager.showLocalTerminal()
s.manager.showTerminal("session-1", s.state)
s.manager.showWorktreeTerminal("wt-1", s.state)
s.manager.closeDirectory("/repo/.kilo/worktrees/wt-1/")
expect(s.disposed).toEqual(["/repo/.kilo/worktrees/wt-1", "/repo/.kilo/worktrees/wt-1"])
expect(s.manager.showExisting("session-1")).toBe(false)
expect(s.manager.showExistingLocal()).toBe(true)
s.manager.closeDirectory("/repo/.kilo/worktrees/wt-1")
expect(s.disposed).toHaveLength(2)
})
it("matches Windows worktree directories without case or separator differences", () => {
const s = scene({ worktreePath: "C:\\Repo\\.kilo\\worktrees\\Feature", repoPath: "C:\\Repo" })
s.manager.showWorktreeTerminal("wt-1", s.state)
s.manager.closeDirectory("c:/repo/.KILO/worktrees/feature/")
expect(s.disposed).toEqual(["C:\\Repo\\.kilo\\worktrees\\Feature"])
})
})
@@ -1,4 +1,4 @@
import { afterEach, describe, expect, it } from "bun:test"
import { afterEach, describe, expect, it, spyOn } from "bun:test"
import os from "node:os"
import path from "node:path"
import fs from "node:fs/promises"
@@ -394,6 +394,77 @@ describe("WorktreeManager.removeWorktree", () => {
expect(exists).toBe(false)
}, 15_000)
it("falls back to git removal when Windows prevents renaming the worktree", async () => {
const root = await createTempRepo()
const manager = createManager(root)
const worktree = await manager.createWorktree({ branchName: "rename-blocked" })
const rename = spyOn(fs, "rename").mockRejectedValueOnce(
Object.assign(new Error("directory busy"), { code: "EBUSY" }),
)
try {
await manager.removeWorktree(worktree.path, worktree.branch)
expect(existsSync(worktree.path)).toBe(false)
expect((await simpleGit(root).branch()).all).not.toContain(worktree.branch)
} finally {
rename.mockRestore()
}
})
it("keeps the branch when the worktree directory remains locked", async () => {
const root = await createTempRepo()
const manager = createManager(root)
const worktree = await manager.createWorktree({ branchName: "locked-worktree" })
await simpleGit(root).raw(["worktree", "lock", worktree.path])
const rename = spyOn(fs, "rename").mockRejectedValueOnce(
Object.assign(new Error("directory busy"), { code: "EBUSY" }),
)
const remove = spyOn(fs, "rm").mockRejectedValueOnce(Object.assign(new Error("directory busy"), { code: "EBUSY" }))
try {
await expect(manager.removeWorktree(worktree.path, worktree.branch)).rejects.toThrow("directory busy")
expect(existsSync(worktree.path)).toBe(true)
expect((await simpleGit(root).branch()).all).toContain(worktree.branch)
} finally {
rename.mockRestore()
remove.mockRestore()
}
})
it.skipIf(process.platform !== "win32")(
"keeps a Windows worktree tracked while a live process locks its directory",
async () => {
const root = await createTempRepo()
const manager = createManager(root)
const worktree = await manager.createWorktree({ branchName: "windows-process-lock" })
const child = Bun.spawn(
[process.execPath, "-e", 'process.stdout.write("ready\\n"); setInterval(() => {}, 1000)'],
{
cwd: worktree.path,
stdout: "pipe",
stderr: "pipe",
windowsHide: true,
},
)
try {
const ready = await child.stdout.getReader().read()
expect(Buffer.from(ready.value ?? []).toString()).toContain("ready")
await expect(manager.removeWorktree(worktree.path, worktree.branch)).rejects.toThrow()
expect(existsSync(worktree.path)).toBe(true)
expect((await simpleGit(root).branch()).all).toContain(worktree.branch)
} finally {
child.kill()
await child.exited
}
await manager.removeWorktree(worktree.path, worktree.branch)
expect(existsSync(worktree.path)).toBe(false)
expect((await simpleGit(root).branch()).all).not.toContain(worktree.branch)
},
30_000,
)
it("does not throw when worktree path does not exist", async () => {
const root = await createTempRepo()
const mgr = createManager(root)