From 57e699a09a8cc2877e3757ea80d102ab24f23117 Mon Sep 17 00:00:00 2001 From: Kaspre Date: Fri, 15 May 2026 00:08:30 -0400 Subject: [PATCH] fix(config): rotate clobber snapshots at cap --- docs/gateway/troubleshooting.md | 2 + src/config/io.clobber-snapshot.test.ts | 45 ++++++++++- src/config/io.clobber-snapshot.ts | 106 +++++++++++++++++++++---- src/config/io.observe-recovery.ts | 2 + src/config/io.write-config.test.ts | 9 ++- 5 files changed, 144 insertions(+), 20 deletions(-) diff --git a/docs/gateway/troubleshooting.md b/docs/gateway/troubleshooting.md index 3f2de8d49de..f158f5c85e4 100644 --- a/docs/gateway/troubleshooting.md +++ b/docs/gateway/troubleshooting.md @@ -370,6 +370,7 @@ Look for: - `Config write rejected: ...` - A timestamped `openclaw.json.rejected.*` file beside the active config - A timestamped `openclaw.json.clobbered.*` file if `doctor --fix` repaired a broken direct edit +- OpenClaw keeps the latest 32 `.clobbered.*` files for each config path and rotates older ones @@ -378,6 +379,7 @@ Look for: - Hot reload skips invalid external edits and keeps the current runtime config active. - OpenClaw-owned writes reject invalid/destructive payloads before commit and save `.rejected.*`. - `openclaw doctor --fix` owns repair. It can remove non-JSON prefixes or restore the last-known-good copy while preserving the rejected payload as `.clobbered.*`. + - When many repairs happen for one config path, OpenClaw rotates older `.clobbered.*` files so the newest repaired payload is still available. diff --git a/src/config/io.clobber-snapshot.test.ts b/src/config/io.clobber-snapshot.test.ts index f9a8f4d67c0..4d1fa78c277 100644 --- a/src/config/io.clobber-snapshot.test.ts +++ b/src/config/io.clobber-snapshot.test.ts @@ -35,7 +35,15 @@ describe("config clobber snapshots", () => { return entries.filter((entry) => entry.startsWith(prefix)); } - it("keeps concurrent async snapshots under the per-path cap", async () => { + async function readClobberFileContents(configPath: string): Promise { + const dir = path.dirname(configPath); + const clobberFiles = await listClobberFiles(configPath); + return await Promise.all( + clobberFiles.map((entry) => fsp.readFile(path.join(dir, entry), "utf-8")), + ); + } + + it("keeps concurrent async snapshots under the per-path cap by rotating oldest files", async () => { await withCase(async (configPath) => { const warn = vi.fn(); const observedAt = "2026-05-03T00:00:00.000Z"; @@ -61,7 +69,35 @@ describe("config clobber snapshots", () => { }); }); - it("keeps sync snapshots under the per-path cap and warns once", async () => { + it("rotates async snapshots so the latest clobbered config is preserved", async () => { + await withCase(async (configPath) => { + const warn = vi.fn(); + + for (let index = 0; index < CONFIG_CLOBBER_SNAPSHOT_LIMIT + 3; index++) { + await persistBoundedClobberedConfigSnapshot({ + deps: { fs, logger: { warn } }, + configPath, + raw: `polluted-${index}\n`, + observedAt: `2026-05-03T00:00:${String(index).padStart(2, "0")}.000Z`, + }); + } + + const clobberFiles = await listClobberFiles(configPath); + expect(clobberFiles).toHaveLength(CONFIG_CLOBBER_SNAPSHOT_LIMIT); + const contents = await readClobberFileContents(configPath); + expect(contents).not.toContain("polluted-0\n"); + expect(contents).not.toContain("polluted-1\n"); + expect(contents).not.toContain("polluted-2\n"); + expect(contents).toContain(`polluted-${CONFIG_CLOBBER_SNAPSHOT_LIMIT + 2}\n`); + const capWarnings = warn.mock.calls.filter( + ([message]) => + typeof message === "string" && message.includes("Config clobber snapshot cap reached"), + ); + expect(capWarnings).toHaveLength(1); + }); + }); + + it("rotates sync snapshots so the latest clobbered config is preserved", async () => { await withCase(async (configPath) => { const warn = vi.fn(); @@ -76,6 +112,11 @@ describe("config clobber snapshots", () => { const clobberFiles = await listClobberFiles(configPath); expect(clobberFiles).toHaveLength(CONFIG_CLOBBER_SNAPSHOT_LIMIT); + const contents = await readClobberFileContents(configPath); + expect(contents).not.toContain("polluted-0\n"); + expect(contents).not.toContain("polluted-1\n"); + expect(contents).not.toContain("polluted-2\n"); + expect(contents).toContain(`polluted-${CONFIG_CLOBBER_SNAPSHOT_LIMIT + 2}\n`); const capWarnings = warn.mock.calls.filter( ([message]) => typeof message === "string" && message.includes("Config clobber snapshot cap reached"), diff --git a/src/config/io.clobber-snapshot.ts b/src/config/io.clobber-snapshot.ts index 39d42a727cd..36d91cf281c 100644 --- a/src/config/io.clobber-snapshot.ts +++ b/src/config/io.clobber-snapshot.ts @@ -13,6 +13,7 @@ type ConfigClobberSnapshotFs = { readdir(path: string): Promise; rmdir(path: string): Promise; stat(path: string): Promise<{ mtimeMs?: number } | null>; + unlink(path: string): Promise; writeFile( path: string, data: string, @@ -23,6 +24,7 @@ type ConfigClobberSnapshotFs = { readdirSync(path: string): string[]; rmdirSync(path: string): unknown; statSync(path: string, options?: { throwIfNoEntry?: boolean }): { mtimeMs?: number } | null; + unlinkSync(path: string): unknown; writeFileSync( path: string, data: string, @@ -117,28 +119,59 @@ function acquireClobberLockSync(deps: ConfigClobberSnapshotDeps, lockPath: strin return false; } -async function countClobberedSiblings( +type ClobberedSiblingSnapshot = { + name: string; + path: string; + mtimeMs: number; +}; + +function compareClobberedSiblings( + left: ClobberedSiblingSnapshot, + right: ClobberedSiblingSnapshot, +): number { + return left.mtimeMs - right.mtimeMs || left.name.localeCompare(right.name); +} + +async function listClobberedSiblings( deps: ConfigClobberSnapshotDeps, dir: string, prefix: string, -): Promise { +): Promise { try { const entries = await deps.fs.promises.readdir(dir); - return entries.filter((entry) => entry.startsWith(prefix)).length; + const snapshots: ClobberedSiblingSnapshot[] = []; + for (const entry of entries) { + if (!entry.startsWith(prefix)) { + continue; + } + const snapshotPath = path.join(dir, entry); + const stat = await deps.fs.promises.stat(snapshotPath).catch(() => null); + snapshots.push({ name: entry, path: snapshotPath, mtimeMs: stat?.mtimeMs ?? 0 }); + } + return snapshots.toSorted(compareClobberedSiblings); } catch { - return 0; + return []; } } -function countClobberedSiblingsSync( +function listClobberedSiblingsSync( deps: ConfigClobberSnapshotDeps, dir: string, prefix: string, -): number { +): ClobberedSiblingSnapshot[] { try { - return deps.fs.readdirSync(dir).filter((entry) => entry.startsWith(prefix)).length; + const snapshots: ClobberedSiblingSnapshot[] = []; + for (const entry of deps.fs.readdirSync(dir)) { + if (!entry.startsWith(prefix)) { + continue; + } + const snapshotPath = path.join(dir, entry); + const stat = deps.fs.statSync(snapshotPath, { throwIfNoEntry: false }); + snapshots.push({ name: entry, path: snapshotPath, mtimeMs: stat?.mtimeMs ?? 0 }); + } + return snapshots.toSorted(compareClobberedSiblings); } catch { - return 0; + return []; } } @@ -152,10 +185,44 @@ function warnClobberCapReached( } clobberCapWarnedPaths.add(configPath); deps.logger.warn( - `Config clobber snapshot cap reached for ${configPath}: ${existing} existing .clobbered.* files; skipping additional forensic snapshots.`, + `Config clobber snapshot cap reached for ${configPath}: ${existing} existing .clobbered.* files; rotating oldest snapshots to preserve the latest forensic copy.`, ); } +async function rotateOldestClobberedSiblings( + deps: ConfigClobberSnapshotDeps, + snapshots: ClobberedSiblingSnapshot[], +): Promise { + const deleteCount = Math.max(0, snapshots.length - CONFIG_CLOBBER_SNAPSHOT_LIMIT + 1); + for (const snapshot of snapshots.slice(0, deleteCount)) { + try { + await deps.fs.promises.unlink(snapshot.path); + } catch (error) { + if (!isFsErrorCode(error, "ENOENT")) { + return false; + } + } + } + return true; +} + +function rotateOldestClobberedSiblingsSync( + deps: ConfigClobberSnapshotDeps, + snapshots: ClobberedSiblingSnapshot[], +): boolean { + const deleteCount = Math.max(0, snapshots.length - CONFIG_CLOBBER_SNAPSHOT_LIMIT + 1); + for (const snapshot of snapshots.slice(0, deleteCount)) { + try { + deps.fs.unlinkSync(snapshot.path); + } catch (error) { + if (!isFsErrorCode(error, "ENOENT")) { + return false; + } + } + } + return true; +} + function buildClobberedTargetPath(configPath: string, observedAt: string, attempt: number): string { const basePath = `${configPath}.clobbered.${formatConfigArtifactTimestamp(observedAt)}`; return attempt === 0 ? basePath : `${basePath}-${String(attempt).padStart(2, "0")}`; @@ -173,10 +240,13 @@ export async function persistBoundedClobberedConfigSnapshot(params: { return null; } try { - const existing = await countClobberedSiblings(params.deps, paths.dir, paths.prefix); - if (existing >= CONFIG_CLOBBER_SNAPSHOT_LIMIT) { - warnClobberCapReached(params.deps, params.configPath, existing); - return null; + const existing = await listClobberedSiblings(params.deps, paths.dir, paths.prefix); + if (existing.length >= CONFIG_CLOBBER_SNAPSHOT_LIMIT) { + warnClobberCapReached(params.deps, params.configPath, existing.length); + const rotated = await rotateOldestClobberedSiblings(params.deps, existing); + if (!rotated) { + return null; + } } for (let attempt = 0; attempt < CONFIG_CLOBBER_SNAPSHOT_LIMIT; attempt++) { const targetPath = buildClobberedTargetPath(params.configPath, params.observedAt, attempt); @@ -210,10 +280,12 @@ export function persistBoundedClobberedConfigSnapshotSync(params: { return null; } try { - const existing = countClobberedSiblingsSync(params.deps, paths.dir, paths.prefix); - if (existing >= CONFIG_CLOBBER_SNAPSHOT_LIMIT) { - warnClobberCapReached(params.deps, params.configPath, existing); - return null; + const existing = listClobberedSiblingsSync(params.deps, paths.dir, paths.prefix); + if (existing.length >= CONFIG_CLOBBER_SNAPSHOT_LIMIT) { + warnClobberCapReached(params.deps, params.configPath, existing.length); + if (!rotateOldestClobberedSiblingsSync(params.deps, existing)) { + return null; + } } for (let attempt = 0; attempt < CONFIG_CLOBBER_SNAPSHOT_LIMIT; attempt++) { const targetPath = buildClobberedTargetPath(params.configPath, params.observedAt, attempt); diff --git a/src/config/io.observe-recovery.ts b/src/config/io.observe-recovery.ts index 70ea2f28a3a..3cba90235d8 100644 --- a/src/config/io.observe-recovery.ts +++ b/src/config/io.observe-recovery.ts @@ -43,6 +43,7 @@ export type ObserveRecoveryDeps = { mkdir(path: string, options?: { recursive?: boolean; mode?: number }): Promise; readdir(path: string): Promise; rmdir(path: string): Promise; + unlink(path: string): Promise; appendFile( path: string, data: string, @@ -73,6 +74,7 @@ export type ObserveRecoveryDeps = { mkdirSync(path: string, options?: { recursive?: boolean; mode?: number }): unknown; readdirSync(path: string): string[]; rmdirSync(path: string): unknown; + unlinkSync(path: string): unknown; appendFileSync( path: string, data: string, diff --git a/src/config/io.write-config.test.ts b/src/config/io.write-config.test.ts index 31a940bc388..ac7a3c26ce7 100644 --- a/src/config/io.write-config.test.ts +++ b/src/config/io.write-config.test.ts @@ -780,7 +780,7 @@ describe("config io write", () => { }); }); - it("caps repeated prefix-recovery clobber snapshots for doctor-style repair loops", async () => { + it("rotates repeated prefix-recovery clobber snapshots for doctor-style repair loops", async () => { await withSuiteHome(async (home) => { const configPath = path.join(home, ".openclaw", "openclaw.json"); const cleanConfig = { @@ -806,6 +806,13 @@ describe("config io write", () => { const entries = await fs.readdir(path.dirname(configPath)); const clobbered = entries.filter((entry) => entry.includes(".clobbered.")); expect(clobbered).toHaveLength(CONFIG_CLOBBER_SNAPSHOT_LIMIT); + const clobberedContents = await Promise.all( + clobbered.map((entry) => fs.readFile(path.join(path.dirname(configPath), entry), "utf-8")), + ); + expect(clobberedContents).not.toContain(`Found and updated: False 0\n${cleanRaw}`); + expect(clobberedContents).toContain( + `Found and updated: False ${CONFIG_CLOBBER_SNAPSHOT_LIMIT + 3}\n${cleanRaw}`, + ); const capWarnings = warn.mock.calls.filter( ([message]) => typeof message === "string" && message.includes("Config clobber snapshot cap reached"),