From 1cc2103701526555bb01f80b1945225288407c93 Mon Sep 17 00:00:00 2001 From: Richard Solomou Date: Mon, 4 May 2026 14:33:18 +0300 Subject: [PATCH 1/3] fix(code): chmod read-only subtrees before deleting worktrees Worktrees that ran any flox/Go workflow accumulate a module cache under .flox/cache/go/pkg/mod with every directory marked 0555. Plain fs.rm cannot unlink entries from those parents, so archive and suspend would fail with EACCES, the rollback would undo the archive row, and the task would reappear in the sidebar. Add forceRemove() to walk the tree restoring write bits on EACCES/EPERM and retry, then route every worktree-deletion call site through it. Generated-By: PostHog Code Task-Id: aa517cb8-e2ed-4a1c-bf8e-e592c652d690 --- .../code/src/main/services/archive/service.ts | 5 +- .../main/services/suspension/service.test.ts | 17 ++++- .../src/main/services/suspension/service.ts | 3 +- packages/git/src/sagas/worktree.ts | 8 +-- packages/git/src/utils.test.ts | 68 +++++++++++++++++++ packages/git/src/utils.ts | 42 ++++++++++++ packages/git/src/worktree.ts | 4 +- 7 files changed, 135 insertions(+), 12 deletions(-) create mode 100644 packages/git/src/utils.test.ts diff --git a/apps/code/src/main/services/archive/service.ts b/apps/code/src/main/services/archive/service.ts index 06c98e29ad..1c3dec2600 100644 --- a/apps/code/src/main/services/archive/service.ts +++ b/apps/code/src/main/services/archive/service.ts @@ -7,6 +7,7 @@ import { deleteCheckpoint, RevertCheckpointSaga, } from "@posthog/git/sagas/checkpoint"; +import { forceRemove } from "@posthog/git/utils"; import { type WorktreeInfo, WorktreeManager } from "@posthog/git/worktree"; import { inject, injectable } from "inversify"; import type { @@ -232,7 +233,7 @@ export class ArchiveService { }); await manager.deleteWorktree(worktreePath); const parentDir = path.dirname(worktreePath); - await fs.rm(parentDir, { recursive: true, force: true }); + await forceRemove(parentDir); }, async () => {}, ); @@ -352,7 +353,7 @@ export class ArchiveService { ); await manager.deleteWorktree(worktreePath); const parentDir = path.dirname(worktreePath); - await fs.rm(parentDir, { recursive: true, force: true }); + await forceRemove(parentDir); } }, ); diff --git a/apps/code/src/main/services/suspension/service.test.ts b/apps/code/src/main/services/suspension/service.test.ts index de47862218..5447f26ccb 100644 --- a/apps/code/src/main/services/suspension/service.test.ts +++ b/apps/code/src/main/services/suspension/service.test.ts @@ -45,9 +45,20 @@ vi.mock("@posthog/git/worktree", () => ({ mockWorktreeManagerProto.createDetachedWorktreeAtCommit; }, })); -vi.mock("node:fs/promises", () => ({ - default: { rm: vi.fn(), access: vi.fn() }, -})); +vi.mock("node:fs/promises", () => { + const fns = { + rm: vi.fn(), + access: vi.fn(), + lstat: vi + .fn() + .mockRejectedValue( + Object.assign(new Error("ENOENT"), { code: "ENOENT" }), + ), + chmod: vi.fn(), + readdir: vi.fn().mockResolvedValue([]), + }; + return { default: fns, ...fns }; +}); vi.mock("../../utils/logger.js", () => ({ logger: { diff --git a/apps/code/src/main/services/suspension/service.ts b/apps/code/src/main/services/suspension/service.ts index 032051cbf6..5409bcff61 100644 --- a/apps/code/src/main/services/suspension/service.ts +++ b/apps/code/src/main/services/suspension/service.ts @@ -6,6 +6,7 @@ import { deleteCheckpoint, RevertCheckpointSaga, } from "@posthog/git/sagas/checkpoint"; +import { forceRemove } from "@posthog/git/utils"; import { type WorktreeInfo, WorktreeManager } from "@posthog/git/worktree"; import { inject, injectable } from "inversify"; import type { IArchiveRepository } from "../../db/repositories/archive-repository.js"; @@ -278,7 +279,7 @@ export class SuspensionService extends TypedEventEmitter { const manager = this.createWorktreeManager(folderPath); await manager.deleteWorktree(worktreePath); - await fs.rm(path.dirname(worktreePath), { recursive: true, force: true }); + await forceRemove(path.dirname(worktreePath)); } private async killTaskProcesses( diff --git a/packages/git/src/sagas/worktree.ts b/packages/git/src/sagas/worktree.ts index 4af5e5947c..9f9e1fea88 100644 --- a/packages/git/src/sagas/worktree.ts +++ b/packages/git/src/sagas/worktree.ts @@ -2,7 +2,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import { GitSaga, type GitSagaInput } from "../git-saga"; import { addToLocalExclude, branchExists, getDefaultBranch } from "../queries"; -import { safeSymlink } from "../utils"; +import { forceRemove, safeSymlink } from "../utils"; import { processWorktreeInclude, runPostCheckoutHook } from "../worktree"; export interface CreateWorktreeInput extends GitSagaInput { @@ -50,7 +50,7 @@ export class CreateWorktreeSaga extends GitSaga< try { await this.git.raw(["worktree", "remove", worktreePath, "--force"]); } catch { - await fs.rm(worktreePath, { recursive: true, force: true }); + await forceRemove(worktreePath); await this.git.raw(["worktree", "prune"]); } try { @@ -157,7 +157,7 @@ export class CreateWorktreeForBranchSaga extends GitSaga< try { await this.git.raw(["worktree", "remove", worktreePath, "--force"]); } catch { - await fs.rm(worktreePath, { recursive: true, force: true }); + await forceRemove(worktreePath); await this.git.raw(["worktree", "prune"]); } }, @@ -274,7 +274,7 @@ export class DeleteWorktreeSaga extends GitSaga< try { await this.git.raw(["worktree", "remove", worktreePath, "--force"]); } catch { - await fs.rm(worktreePath, { recursive: true, force: true }); + await forceRemove(worktreePath); await this.git.raw(["worktree", "prune"]); } }, diff --git a/packages/git/src/utils.test.ts b/packages/git/src/utils.test.ts new file mode 100644 index 0000000000..59f69e3725 --- /dev/null +++ b/packages/git/src/utils.test.ts @@ -0,0 +1,68 @@ +import { chmod, mkdir, mkdtemp, rm, stat, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { forceRemove } from "./utils"; + +async function fileExists(p: string): Promise { + try { + await stat(p); + return true; + } catch { + return false; + } +} + +describe("forceRemove", () => { + let workDir: string | undefined; + + afterEach(async () => { + if (workDir) { + // Restore writability so cleanup never fails the run. + await chmod(workDir, 0o700).catch(() => {}); + await rm(workDir, { recursive: true, force: true }).catch(() => {}); + workDir = undefined; + } + }); + + it("removes a writable tree", async () => { + workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); + const target = path.join(workDir, "tree"); + await mkdir(path.join(target, "nested"), { recursive: true }); + await writeFile(path.join(target, "nested", "file.txt"), "x"); + + await forceRemove(target); + + expect(await fileExists(target)).toBe(false); + }); + + it("is a no-op for a missing path", async () => { + workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); + await expect( + forceRemove(path.join(workDir, "missing")), + ).resolves.toBeUndefined(); + }); + + it("removes a tree with read-only directories (Go module cache shape)", async () => { + workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); + const target = path.join(workDir, "tree"); + const inner = path.join(target, "yaml.v3@v3.0.1", ".github"); + await mkdir(inner, { recursive: true }); + await writeFile(path.join(inner, "workflow.yml"), "on: push\n"); + + // Mimic Go's modcache lockdown: every directory inside loses its write bit. + await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); + await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); + + await expect( + rm(target, { recursive: true, force: true }), + ).rejects.toMatchObject({ code: "EACCES" }); + // Restore the dir so forceRemove starts from the same state every run. + await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); + await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); + + await forceRemove(target); + + expect(await fileExists(target)).toBe(false); + }); +}); diff --git a/packages/git/src/utils.ts b/packages/git/src/utils.ts index 1947cc62e5..1c3ace9fd3 100644 --- a/packages/git/src/utils.ts +++ b/packages/git/src/utils.ts @@ -107,6 +107,48 @@ function execFileAsync( }); } +async function chmodTreeWritable(target: string): Promise { + let stat: import("node:fs").Stats; + try { + stat = await fs.lstat(target); + } catch { + return; + } + if (stat.isSymbolicLink()) return; + try { + await fs.chmod(target, stat.isDirectory() ? 0o700 : 0o600); + } catch {} + if (!stat.isDirectory()) return; + let entries: string[]; + try { + entries = await fs.readdir(target); + } catch { + return; + } + await Promise.all( + entries.map((entry) => chmodTreeWritable(path.join(target, entry))), + ); +} + +/** + * Recursively remove a path, retrying after chmod'ing the tree writable when + * the kernel rejects the initial removal with EACCES/EPERM. Worktrees commonly + * contain read-only subtrees populated by Go's module cache (which marks every + * cached directory mode 0555); plain `fs.rm` cannot unlink entries from those + * parents until we restore the write bit. + */ +export async function forceRemove(target: string): Promise { + try { + await fs.rm(target, { recursive: true, force: true, maxRetries: 3 }); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "EACCES" && code !== "EPERM") throw error; + } + await chmodTreeWritable(target); + await fs.rm(target, { recursive: true, force: true, maxRetries: 3 }); +} + export interface GitHubPr { owner: string; repo: string; diff --git a/packages/git/src/worktree.ts b/packages/git/src/worktree.ts index 4eb299fa87..5fe1e7cd99 100644 --- a/packages/git/src/worktree.ts +++ b/packages/git/src/worktree.ts @@ -10,7 +10,7 @@ import { getHeadSha, listWorktrees as listWorktreesRaw, } from "./queries"; -import { clonePath, safeSymlink } from "./utils"; +import { clonePath, forceRemove, safeSymlink } from "./utils"; export interface WorktreeInfo { worktreePath: string; @@ -394,7 +394,7 @@ export class WorktreeManager { try { await git.raw(["worktree", "remove", worktreePath, "--force"]); } catch { - await fs.rm(worktreePath, { recursive: true, force: true }); + await forceRemove(worktreePath); await git.raw(["worktree", "prune"]); } }); From 0587da8016cd7839035d555d55a726294dc8a79b Mon Sep 17 00:00:00 2001 From: Richard Solomou Date: Mon, 4 May 2026 14:44:18 +0300 Subject: [PATCH 2/3] test(git): address review comments on forceRemove tests Use forceRemove in afterEach so cleanup handles read-only subdirs the same way the code under test does. Collapse the two tree-removal cases into a single it.each table to reduce boilerplate. Generated-By: PostHog Code Task-Id: aa517cb8-e2ed-4a1c-bf8e-e592c652d690 --- packages/git/src/utils.test.ts | 58 +++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/packages/git/src/utils.test.ts b/packages/git/src/utils.test.ts index 59f69e3725..ab558abead 100644 --- a/packages/git/src/utils.test.ts +++ b/packages/git/src/utils.test.ts @@ -18,24 +18,11 @@ describe("forceRemove", () => { afterEach(async () => { if (workDir) { - // Restore writability so cleanup never fails the run. - await chmod(workDir, 0o700).catch(() => {}); - await rm(workDir, { recursive: true, force: true }).catch(() => {}); + await forceRemove(workDir).catch(() => {}); workDir = undefined; } }); - it("removes a writable tree", async () => { - workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); - const target = path.join(workDir, "tree"); - await mkdir(path.join(target, "nested"), { recursive: true }); - await writeFile(path.join(target, "nested", "file.txt"), "x"); - - await forceRemove(target); - - expect(await fileExists(target)).toBe(false); - }); - it("is a no-op for a missing path", async () => { workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); await expect( @@ -43,23 +30,36 @@ describe("forceRemove", () => { ).resolves.toBeUndefined(); }); - it("removes a tree with read-only directories (Go module cache shape)", async () => { + it.each([ + { + name: "writable tree", + setup: async (target: string) => { + await mkdir(path.join(target, "nested"), { recursive: true }); + await writeFile(path.join(target, "nested", "file.txt"), "x"); + }, + }, + { + name: "read-only directories (Go module cache shape)", + setup: async (target: string) => { + const inner = path.join(target, "yaml.v3@v3.0.1", ".github"); + await mkdir(inner, { recursive: true }); + await writeFile(path.join(inner, "workflow.yml"), "on: push\n"); + await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); + await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); + // Confirm plain fs.rm cannot remove it. + await expect( + rm(target, { recursive: true, force: true }), + ).rejects.toMatchObject({ code: "EACCES" }); + // Restore read-only state so forceRemove starts clean. + await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); + await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); + }, + }, + ])("removes a $name", async ({ setup }) => { workDir = await mkdtemp(path.join(tmpdir(), "posthog-force-remove-")); const target = path.join(workDir, "tree"); - const inner = path.join(target, "yaml.v3@v3.0.1", ".github"); - await mkdir(inner, { recursive: true }); - await writeFile(path.join(inner, "workflow.yml"), "on: push\n"); - - // Mimic Go's modcache lockdown: every directory inside loses its write bit. - await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); - await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); - - await expect( - rm(target, { recursive: true, force: true }), - ).rejects.toMatchObject({ code: "EACCES" }); - // Restore the dir so forceRemove starts from the same state every run. - await chmod(path.join(target, "yaml.v3@v3.0.1"), 0o555); - await chmod(path.join(target, "yaml.v3@v3.0.1", ".github"), 0o555); + await mkdir(target); + await setup(target); await forceRemove(target); From cebffb44c2a6d22d4b8186772dde9ebea22d5446 Mon Sep 17 00:00:00 2001 From: Richard Solomou Date: Mon, 4 May 2026 14:46:43 +0300 Subject: [PATCH 3/3] fix(git): log chmod and readdir failures in chmodTreeWritable Silently swallowing these errors made it impossible to diagnose cases where forceRemove's chmod pass itself failed (e.g. immutable flags or ownership mismatches). Warn to console so the failure shows up in app logs without breaking the flow. Generated-By: PostHog Code Task-Id: aa517cb8-e2ed-4a1c-bf8e-e592c652d690 --- packages/git/src/utils.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/git/src/utils.ts b/packages/git/src/utils.ts index 1c3ace9fd3..d54b2e04f7 100644 --- a/packages/git/src/utils.ts +++ b/packages/git/src/utils.ts @@ -117,12 +117,15 @@ async function chmodTreeWritable(target: string): Promise { if (stat.isSymbolicLink()) return; try { await fs.chmod(target, stat.isDirectory() ? 0o700 : 0o600); - } catch {} + } catch (error) { + console.warn(`forceRemove: chmod failed on ${target}`, error); + } if (!stat.isDirectory()) return; let entries: string[]; try { entries = await fs.readdir(target); - } catch { + } catch (error) { + console.warn(`forceRemove: readdir failed on ${target}`, error); return; } await Promise.all(