From 34067fcc2e298965512fa6ae80acf740722db45c Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 25 Aug 2026 14:16:44 -0700 Subject: [PATCH] fix(worktrees): preserve nested repositories during cleanup (#129454) * fix(worktrees): preserve nested repositories during cleanup Automatic idle, limit, and run-end cleanup now treats nested repositories and linked worktrees as separate lifecycle boundaries. This prevents repeated cleanup warnings and avoids deleting unregistered nested user state. Closes #129414 * test: isolate media auth fixture from plugin loading Keep the OAuth fixture outside the refresh window and stub only the unrelated retired-profile plugin lookup so the media auth contract completes deterministically in CI. --- docs/concepts/managed-worktrees.md | 4 +- src/agents/worktrees/service.gc.test.ts | 89 +++++++++++++++---- .../worktrees/service.run-end-cleanup.test.ts | 37 ++++++++ src/agents/worktrees/service.ts | 64 ++++++------- .../runner.local-no-auth.test.ts | 9 +- 5 files changed, 151 insertions(+), 52 deletions(-) diff --git a/docs/concepts/managed-worktrees.md b/docs/concepts/managed-worktrees.md index b7e01aff4a1f..09ce519b4eb2 100644 --- a/docs/concepts/managed-worktrees.md +++ b/docs/concepts/managed-worktrees.md @@ -61,7 +61,9 @@ The resulting managed worktree is owned by the session, and every agent run in t ## Snapshots, cleanup, and restore -Removal first creates a synthetic commit containing tracked and non-ignored untracked files, then pins it at `refs/openclaw/snapshots/`. Ignored files never enter the repository object database. OpenClaw stores only the ignored files it actually provisioned in chunked shared-state database rows; the recorded path set remains authoritative even if `.worktreeinclude` later changes or disappears. Restore reads those bytes from the immutable snapshot and reapplies their complete modes. Automatic cleanup preserves a live worktree when a recorded path can no longer be snapshotted safely. If snapshot creation fails, removal stops. An explicit force delete can continue without a snapshot. +Removal first creates a synthetic commit containing tracked and non-ignored untracked files, then pins it at `refs/openclaw/snapshots/`. Ignored files never enter the repository object database. OpenClaw stores only the ignored files it actually provisioned in chunked shared-state database rows; the recorded path set remains authoritative even if `.worktreeinclude` later changes or disappears. Restore reads those bytes from the immutable snapshot and reapplies their complete modes. Automatic cleanup preserves a live worktree when a recorded path can no longer be snapshotted safely. If snapshot creation fails, removal stops unless an explicit force delete discards snapshot safety. + +Nested Git repositories and linked worktrees are separate ownership boundaries. Automatic cleanup preserves the outer worktree even when a nested linked worktree shares its Git common directory. A nested OpenClaw-managed worktree is cleaned only through its own managed-worktree record. OpenClaw applies these cleanup rules: diff --git a/src/agents/worktrees/service.gc.test.ts b/src/agents/worktrees/service.gc.test.ts index 6410e8144f6f..04db132f3eb9 100644 --- a/src/agents/worktrees/service.gc.test.ts +++ b/src/agents/worktrees/service.gc.test.ts @@ -4,8 +4,9 @@ import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createWarnLogCapture } from "../../logging/test-helpers/warn-log-capture.js"; import { closeOpenClawStateDatabaseForTest } from "../../state/openclaw-state-db.js"; -import { getRegistryWorktree } from "./registry.js"; +import { findLiveRegistryWorktreeByPath, getRegistryWorktree } from "./registry.js"; import { IDLE_GC_MS, ManagedWorktreeService, @@ -91,7 +92,7 @@ describe("ManagedWorktreeService garbage collection", () => { expect(await fs.stat(manual.path)).toBeTruthy(); }); - it("garbage collects an ignored nested linked worktree", async () => { + it("preserves an ignored unregistered nested linked worktree without cleanup warnings", async () => { await fs.writeFile(path.join(repo, ".gitignore"), ".claude/\n"); await git(repo, "add", ".gitignore"); await git(repo, "commit", "-m", "ignore agent checkout state"); @@ -102,13 +103,23 @@ describe("ManagedWorktreeService garbage collection", () => { await git(repo, "worktree", "add", "--detach", nested, "HEAD"); expect((await fs.stat(path.join(nested, ".git"))).isFile()).toBe(true); await fs.writeFile(path.join(nested, "local.txt"), "ignored agent state\n"); + expect(findLiveRegistryWorktreeByPath(env, nested)).toBeUndefined(); expect(await git(created.path, "ls-files", "--others", "--exclude-standard")).toBe(""); now += IDLE_GC_MS + 1; - expect((await service.gc()).removed).toEqual([created.id]); - await expect(fs.stat(created.path)).rejects.toMatchObject({ code: "ENOENT" }); - expect(await git(repo, "worktree", "list", "--porcelain")).not.toContain(nested); - expect((await new ManagedWorktreeService({ env, now: () => now }).gc()).removed).toEqual([]); + const warnLogs = createWarnLogCapture("openclaw-worktree-gc-nested-linked"); + try { + expect((await service.gc()).removed).toEqual([]); + expect((await service.gc()).removed).toEqual([]); + expect(await warnLogs.findText(`idle cleanup failed for ${created.id}`)).toBeUndefined(); + expect(await fs.readFile(path.join(nested, "local.txt"), "utf8")).toBe( + "ignored agent state\n", + ); + expect(await git(repo, "worktree", "list", "--porcelain")).toContain(nested); + expect(getRegistryWorktree(env, created.id)?.removedAt).toBeUndefined(); + } finally { + warnLogs.cleanup(); + } }); it.each([ @@ -133,12 +144,19 @@ describe("ManagedWorktreeService garbage collection", () => { expect(await git(created.path, "ls-files", "--others", "--exclude-standard")).toBe(""); now += IDLE_GC_MS + 1; - expect((await service.gc()).removed).toEqual([]); - expect((await fs.stat(path.join(nested, ".git"))).isDirectory()).toBe(true); - if (hasLocalState) { - expect(await fs.readFile(localState, "utf8")).toBe("keep foreign repository state\n"); + const warnLogs = createWarnLogCapture("openclaw-worktree-gc-nested-foreign"); + try { + expect((await service.gc()).removed).toEqual([]); + expect((await service.gc()).removed).toEqual([]); + expect(await warnLogs.findText(`idle cleanup failed for ${created.id}`)).toBeUndefined(); + expect((await fs.stat(path.join(nested, ".git"))).isDirectory()).toBe(true); + if (hasLocalState) { + expect(await fs.readFile(localState, "utf8")).toBe("keep foreign repository state\n"); + } + expect(getRegistryWorktree(env, created.id)?.removedAt).toBeUndefined(); + } finally { + warnLogs.cleanup(); } - expect(getRegistryWorktree(env, created.id)?.removedAt).toBeUndefined(); }); it("garbage collects modified provisioned files into the immutable snapshot", async () => { @@ -198,18 +216,26 @@ describe("ManagedWorktreeService garbage collection", () => { expect(await fs.stat(created.path)).toBeTruthy(); }); - it("continues garbage collection after one worktree cannot be snapshotted", async () => { + it("protects a visible nested repository while collecting another idle worktree", async () => { const removable = await materializeRunOwnedFixture("removable", "workboard"); now += 1; const nestedRecord = await materializeRunOwnedFixture("nested-idle", "workboard"); - await initializeNestedRepository(nestedRecord.path, "nested"); + const nested = await initializeNestedRepository(nestedRecord.path, "nested"); + await fs.writeFile(path.join(nested, "local.txt"), "visible nested state\n"); now += IDLE_GC_MS + 1; - const result = await service.gc(); - - expect(result.removed).toEqual([removable.id]); - expect(getRegistryWorktree(env, nestedRecord.id)?.removedAt).toBeUndefined(); - await expect(fs.stat(removable.path)).rejects.toMatchObject({ code: "ENOENT" }); + const warnLogs = createWarnLogCapture("openclaw-worktree-gc-nested-visible"); + try { + expect((await service.gc()).removed).toEqual([removable.id]); + expect(await warnLogs.findText(`idle cleanup failed for ${nestedRecord.id}`)).toBeUndefined(); + expect(getRegistryWorktree(env, nestedRecord.id)?.removedAt).toBeUndefined(); + expect(await fs.readFile(path.join(nested, "local.txt"), "utf8")).toBe( + "visible nested state\n", + ); + await expect(fs.stat(removable.path)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + warnLogs.cleanup(); + } }); it("continues garbage collection when one repository control path is missing", async () => { @@ -266,6 +292,33 @@ describe("ManagedWorktreeService garbage collection", () => { expect(getRegistryWorktree(env, activeOldest.id)?.removedAt).toBeUndefined(); }); + it.each([ + { limit: "count", limits: { maxCount: 1 } }, + { limit: "size", limits: { maxTotalSizeBytes: 60_000 } }, + ])("protects nested repositories during $limit limit eviction", async ({ limits }) => { + const protectedRecord = await materializeRunOwnedFixture("limit-nested", "workboard"); + const nested = await initializeNestedRepository(protectedRecord.path, "nested"); + await fs.writeFile(path.join(nested, "local.txt"), "protected nested state\n"); + now += 1; + const removable = await materializeRunOwnedFixture("limit-removable", "workboard"); + await fs.writeFile(path.join(removable.path, "blob.bin"), Buffer.alloc(100_000)); + + const warnLogs = createWarnLogCapture("openclaw-worktree-gc-nested-limit"); + try { + expect((await service.gc({ limits })).removed).toEqual([removable.id]); + expect( + await warnLogs.findText(`cleanup limit removal failed for ${protectedRecord.id}`), + ).toBeUndefined(); + expect(getRegistryWorktree(env, protectedRecord.id)?.removedAt).toBeUndefined(); + expect(await fs.readFile(path.join(nested, "local.txt"), "utf8")).toBe( + "protected nested state\n", + ); + await expect(fs.stat(removable.path)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + warnLogs.cleanup(); + } + }); + it("evicts oldest worktrees until total size fits the size limit", async () => { const oldest = await materializeRunOwnedFixture( "size-oldest", diff --git a/src/agents/worktrees/service.run-end-cleanup.test.ts b/src/agents/worktrees/service.run-end-cleanup.test.ts index 2fd83b327af4..e27596bb6fa0 100644 --- a/src/agents/worktrees/service.run-end-cleanup.test.ts +++ b/src/agents/worktrees/service.run-end-cleanup.test.ts @@ -201,6 +201,43 @@ describe("ManagedWorktreeService run-end cleanup outcomes", () => { await expect(fs.readFile(dirtyFile, "utf8")).resolves.toBe("retain me\n"); }); + it.each([ + { kind: "independent", linked: false }, + { kind: "same-repository linked", linked: true }, + ])("retains an ignored nested $kind repository at run end", async ({ linked }) => { + await fs.writeFile(path.join(repo, ".gitignore"), "nested/\n"); + await git(repo, "add", ".gitignore"); + await git(repo, "commit", "-m", "ignore nested checkout state"); + await git(repo, "push", "origin", "main"); + + const created = await materialize(linked ? "nested-linked" : "nested-independent"); + const nested = path.join(created.path, "nested", "checkout"); + await fs.mkdir(linked ? path.dirname(nested) : nested, { recursive: true }); + if (linked) { + await git(repo, "worktree", "add", "--detach", nested, "HEAD"); + } else { + await git(nested, "init", "-b", "main"); + } + const localState = path.join(nested, "local.txt"); + await fs.writeFile(localState, "keep nested checkout state\n"); + expect(await git(created.path, "status", "--porcelain")).toBe(""); + expect(await git(created.path, "log", "HEAD", "--not", "--remotes", "--oneline")).toBe(""); + await service.acquire(created.id); + + await expect(service.removeIfLossless(created.id)).resolves.toBe(false); + + expect(getRegistryWorktree(env, created.id)).toMatchObject({ + runEndCleanup: { outcome: "retained-dirty", at: now }, + }); + expect(getRegistryWorktree(env, created.id)?.removedAt).toBeUndefined(); + expect(await fs.readFile(localState, "utf8")).toBe("keep nested checkout state\n"); + if (linked) { + expect(await git(repo, "worktree", "list", "--porcelain")).toContain(nested); + } else { + expect((await fs.stat(path.join(nested, ".git"))).isDirectory()).toBe(true); + } + }); + it("records unpushed retention", async () => { const created = await materialize("unpushed"); await service.acquire(created.id); diff --git a/src/agents/worktrees/service.ts b/src/agents/worktrees/service.ts index 08f64db9c7d9..520a7bc8f510 100644 --- a/src/agents/worktrees/service.ts +++ b/src/agents/worktrees/service.ts @@ -434,13 +434,23 @@ async function rawPathExists(target: string | Buffer): Promise { } async function containsSnapshotGitMarker( - record: ManagedWorktreeRecord, - snapshotPaths: Iterable, + checkoutRoot: string, + snapshotPaths?: Iterable, ): Promise { + const visiblePaths = snapshotPaths ? [...snapshotPaths] : []; + if (!snapshotPaths) { + const indexEntries = splitNullBuffer( + await requireGitBuffer(checkoutRoot, ["ls-files", "--stage", "-z"]), + ); + if (indexEntries.some((entry) => entry.subarray(0, 7).toString() === "160000 ")) { + return true; + } + const visibleGitPaths = ["ls-files", "-z", "--cached", "--others", "--exclude-standard"]; + visiblePaths.push(...splitNullBuffer(await requireGitBuffer(checkoutRoot, visibleGitPaths))); + } const checked = new Set(); - let ownedWorktrees: Set | undefined; const ignoredPaths = splitNullBuffer( - await requireGitBuffer(record.path, [ + await requireGitBuffer(checkoutRoot, [ "ls-files", "-z", "--others", @@ -448,28 +458,17 @@ async function containsSnapshotGitMarker( "--exclude-standard", ]), ); - for (const [paths, ignored] of [ - [snapshotPaths, false], - [ignoredPaths, true], - ] as const) { - for (const gitPath of paths) { - for (let end = gitPath.indexOf(47); end !== -1; end = gitPath.indexOf(47, end + 1)) { - const directory = gitPath.subarray(0, end); - const key = gitPathKey(directory); - if (checked.has(key)) { - continue; - } - checked.add(key); - const marker = Buffer.concat([directory, Buffer.from("/.git")]); - if (!(await rawPathExists(checkoutPathFromGitBytes(record.path, marker)))) { - continue; - } - ownedWorktrees ??= new Set( - (await listGitWorktrees(record.repoRoot)).map((entry) => path.resolve(entry.path)), - ); - if (!ignored || !ownedWorktrees.has(path.resolve(record.path, directory.toString()))) { - return true; - } + for (const gitPath of [...visiblePaths, ...ignoredPaths]) { + for (let end = gitPath.indexOf(47); end !== -1; end = gitPath.indexOf(47, end + 1)) { + const directory = gitPath.subarray(0, end); + const key = gitPathKey(directory); + if (checked.has(key)) { + continue; + } + checked.add(key); + const marker = Buffer.concat([directory, Buffer.from("/.git")]); + if (await rawPathExists(checkoutPathFromGitBytes(checkoutRoot, marker))) { + return true; } } } @@ -553,8 +552,7 @@ async function snapshotWorktree( addSnapshotPath(entry); } } - // Only Git-owned ignored worktrees are disposable; foreign repositories must stay protected. - if (await containsSnapshotGitMarker(record, snapshotPaths.values())) { + if (await containsSnapshotGitMarker(record.path, snapshotPaths.values())) { throw new Error("nested git repositories cannot be snapshotted losslessly"); } await requireGit(record.path, ["read-tree", "HEAD"], { env }); @@ -1194,7 +1192,9 @@ export class ManagedWorktreeService { ? "retained-unpushed" : ignoredDrift ? "retained-provisioned-drift" - : undefined; + : (await containsSnapshotGitMarker(record.path)) + ? "retained-dirty" + : undefined; if (retainedOutcome) { abortWorktreeRemoval(this.env, id, claimToken); recordOutcome(retainedOutcome); @@ -1287,8 +1287,8 @@ export class ManagedWorktreeService { } /** - * Shared auto-removal guard for idle and limit cleanup: owner protection, live - * run leases, and live/foreign git locks veto removal; a dead lock is cleared. + * Shared auto-removal guard: owners, leases, nested repositories, and live or + * foreign Git locks veto removal; a dead lock is cleared. */ private async isProtectedFromAutoRemoval( record: ManagedWorktreeRecord, @@ -1318,7 +1318,7 @@ export class ManagedWorktreeService { if (state.kind === "dead") { await requireGit(record.repoRoot, ["worktree", "unlock", record.path]); } - return false; + return await containsSnapshotGitMarker(record.path); } /** diff --git a/src/media-understanding/runner.local-no-auth.test.ts b/src/media-understanding/runner.local-no-auth.test.ts index 4c5fd2c8135f..676118278b25 100644 --- a/src/media-understanding/runner.local-no-auth.test.ts +++ b/src/media-understanding/runner.local-no-auth.test.ts @@ -55,6 +55,12 @@ vi.mock("../plugins/providers.js", async (importOriginal) => ({ resolveOwningPluginIdsForProviderRef: () => [], })); +vi.mock("../plugins/provider-runtime.js", async (importOriginal) => ({ + ...(await importOriginal()), + // This plugin-free suite must not load bundled plugins to find retired profiles. + resolveProviderDeprecatedAuthProfileIds: () => [], +})); + const AUTH_ENV = { LOCAL_AUDIO_API_KEY: undefined, REMOTE_AUDIO_API_KEY: undefined, @@ -293,7 +299,8 @@ describe("runCapability local no-auth audio providers", () => { provider: "openai", access: "oauth-chat-token", refresh: "oauth-refresh-token", - expires: Date.now() + 60_000, + // Stay outside the five-minute refresh window to exercise API-key selection. + expires: Date.now() + 10 * 60_000, }, }, };