mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-26 12:26:38 -06:00
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.
This commit is contained in:
Peter Steinberger
committed by
GitHub
GitHub
parent
73294a85f2
commit
34067fcc2e
@@ -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/<id>`. 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/<id>`. 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:
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -434,13 +434,23 @@ async function rawPathExists(target: string | Buffer): Promise<boolean> {
|
||||
}
|
||||
|
||||
async function containsSnapshotGitMarker(
|
||||
record: ManagedWorktreeRecord,
|
||||
snapshotPaths: Iterable<Buffer>,
|
||||
checkoutRoot: string,
|
||||
snapshotPaths?: Iterable<Buffer>,
|
||||
): Promise<boolean> {
|
||||
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<string>();
|
||||
let ownedWorktrees: Set<string> | 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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -55,6 +55,12 @@ vi.mock("../plugins/providers.js", async (importOriginal) => ({
|
||||
resolveOwningPluginIdsForProviderRef: () => [],
|
||||
}));
|
||||
|
||||
vi.mock("../plugins/provider-runtime.js", async (importOriginal) => ({
|
||||
...(await importOriginal<typeof import("../plugins/provider-runtime.js")>()),
|
||||
// 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,
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user