From 3bfc23c6760bf94f691805f2b2f950b11035cd9f Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 24 Jul 2026 19:12:11 +0800 Subject: [PATCH] fix(backup): publish archives durably (#113302) --- docs/cli/backup.md | 4 +- src/commands/backup.atomic.test.ts | 62 ++- src/infra/backup-archive-publication.test.ts | 320 +++++++++++++++ src/infra/backup-archive-publication.ts | 398 +++++++++++++++++++ src/infra/backup-create-stream.ts | 116 +++++- src/infra/backup-create.test.ts | 101 +++-- src/infra/backup-create.ts | 90 ++--- src/infra/backup-create.windows.test.ts | 28 +- src/infra/backup-tar-retry.ts | 40 +- 9 files changed, 982 insertions(+), 177 deletions(-) create mode 100644 src/infra/backup-archive-publication.test.ts create mode 100644 src/infra/backup-archive-publication.ts diff --git a/docs/cli/backup.md b/docs/cli/backup.md index 7725e584ee7a..2e702e650897 100644 --- a/docs/cli/backup.md +++ b/docs/cli/backup.md @@ -124,7 +124,9 @@ OpenClaw does not enforce a built-in maximum backup size or per-file size limit. - Available space for the temporary archive write plus the final archive - Time to walk large workspace trees and compress them into a `.tar.gz` - Time to rescan the archive with `--verify` or `openclaw backup verify` -- Destination filesystem behavior: OpenClaw prefers a no-overwrite hard-link publish step and falls back to exclusive copy when hard links are unsupported +- Destination filesystem behavior: OpenClaw requires no-overwrite hard-link publication so a final archive path never exposes an in-progress copy; unsupported filesystems fail with an actionable error + +If final-directory durability confirmation fails after publication, the command reports failure but preserves the complete final entry rather than risk deleting a concurrent replacement. Large workspaces are usually the main driver of archive size. Use `--no-include-workspace` for a smaller/faster backup, or `--only-config` for the smallest archive. diff --git a/src/commands/backup.atomic.test.ts b/src/commands/backup.atomic.test.ts index dc327111917b..0b3b791c1656 100644 --- a/src/commands/backup.atomic.test.ts +++ b/src/commands/backup.atomic.test.ts @@ -1,8 +1,8 @@ // Backup atomicity tests cover temp-file writes, rollback behavior, and backup archive consistency. +import fsSync from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { expectDefined } from "@openclaw/normalization-core"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { createTempHomeEnv, type TempHomeEnv } from "../test-utils/temp-home.js"; import { @@ -95,28 +95,26 @@ describe("backupCreateCommand atomic archive write", () => { } }); - it("cleans intermediate retry temp archives after cleanup races", async () => { + it("cleans intermediate retry archives after a later attempt succeeds", async () => { const { archiveDir, outputPath, runtime } = await prepareAtomicBackupScenario({ archivePrefix: "openclaw-backup-retry-cleanup-", }); - const realRm = fs.rm.bind(fs); - const rmAttempts = new Map(); - const attemptFiles: string[] = []; - const rmSpy = vi.spyOn(fs, "rm").mockImplementation((async ( - targetPath: Parameters[0], - options?: Parameters[1], - ) => { - const key = String(targetPath); - const attempt = (rmAttempts.get(key) ?? 0) + 1; - rmAttempts.set(key, attempt); - if (key.startsWith(`${outputPath}.`) && !attemptFiles.includes(key)) { - attemptFiles.push(key); + const originalUnlinkSync = fsSync.unlinkSync.bind(fsSync); + let blockedPartialPath: string | undefined; + let blockedPartialCleanupAttempts = 0; + const unlinkSpy = vi.spyOn(fsSync, "unlinkSync").mockImplementation((target) => { + const targetPath = path.resolve(String(target)); + if (!blockedPartialPath && targetPath.endsWith("archive.tar.gz.tmp")) { + blockedPartialPath = targetPath; } - if (attemptFiles.length <= 2 && key === attemptFiles.at(-1) && attempt === 1) { - throw Object.assign(new Error("resource busy"), { code: "EBUSY" }); + if (targetPath === blockedPartialPath) { + blockedPartialCleanupAttempts += 1; + if (blockedPartialCleanupAttempts === 1) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } } - await realRm(targetPath, options); - }) as typeof fs.rm); + return originalUnlinkSync(target); + }); try { let tarAttempt = 0; tarCreateMock.mockImplementation(() => { @@ -139,17 +137,10 @@ describe("backupCreateCommand atomic archive write", () => { expect(result.archivePath).toBe(outputPath); expect(sleepMock.mock.calls).toStrictEqual([[10_000], [20_000]]); - expect(attemptFiles).toStrictEqual([ - attemptFiles[0], - `${attemptFiles[0]}.retry-2`, - `${attemptFiles[0]}.retry-3`, - ]); - expect( - rmAttempts.get(expectDefined(attemptFiles[1], "attemptFiles[1] test invariant")), - ).toBeGreaterThanOrEqual(2); + expect(blockedPartialCleanupAttempts).toBeGreaterThanOrEqual(2); expect((await fs.readdir(archiveDir)).toSorted()).toStrictEqual([path.basename(outputPath)]); } finally { - rmSpy.mockRestore(); + unlinkSpy.mockRestore(); await fs.rm(archiveDir, { recursive: true, force: true }); } }); @@ -180,9 +171,9 @@ describe("backupCreateCommand atomic archive write", () => { } }); - it("falls back to exclusive copy when hard-link publication is unsupported", async () => { + it("fails closed when hard-link publication is unsupported", async () => { const { archiveDir, outputPath, runtime } = await prepareAtomicBackupScenario({ - archivePrefix: "openclaw-backup-copy-fallback-", + archivePrefix: "openclaw-backup-no-hardlink-", }); const linkSpy = vi.spyOn(fs, "link"); try { @@ -191,12 +182,13 @@ describe("backupCreateCommand atomic archive write", () => { Object.assign(new Error("hard links not supported"), { code: "EOPNOTSUPP" }), ); - const result = await backupCreateCommand(runtime, { - output: outputPath, - }); - - expect(result.archivePath).toBe(outputPath); - expect(await fs.readFile(outputPath, "utf8")).toBe("archive-bytes"); + await expect( + backupCreateCommand(runtime, { + output: outputPath, + }), + ).rejects.toThrow(/requires hard-link support/iu); + await expectPathMissing(outputPath); + await expect(fs.readdir(archiveDir)).resolves.toEqual([]); } finally { linkSpy.mockRestore(); await fs.rm(archiveDir, { recursive: true, force: true }); diff --git a/src/infra/backup-archive-publication.test.ts b/src/infra/backup-archive-publication.test.ts new file mode 100644 index 000000000000..15c5d004c3d3 --- /dev/null +++ b/src/infra/backup-archive-publication.test.ts @@ -0,0 +1,320 @@ +import fsSync from "node:fs"; +import fs from "node:fs/promises"; +import type { FileHandle } from "node:fs/promises"; +import path from "node:path"; +import { PassThrough } from "node:stream"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; +import { + cleanupBackupArchivePublication, + createBackupArchivePublication, + publishPreparedBackupArchive, + type BackupArchivePublication, +} from "./backup-archive-publication.js"; +import { writeArchiveStreamToFile, type PreparedBackupArchive } from "./backup-create-stream.js"; + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); + +async function createPublication( + prefix: string, +): Promise<{ outputDir: string; outputPath: string; plan: BackupArchivePublication }> { + const root = tempDirs.make(prefix); + const outputDir = path.join(root, "backups"); + const outputPath = path.join(outputDir, "backup.tar.gz"); + await fs.mkdir(outputDir, { recursive: true }); + const plan = await createBackupArchivePublication(outputPath); + return { outputDir, outputPath, plan }; +} + +async function prepareArchive( + plan: BackupArchivePublication, + content = "complete archive", +): Promise { + const archiveStream = new PassThrough(); + const preparedPromise = writeArchiveStreamToFile({ + archivePath: plan.tempArchivePath, + archiveStream, + }); + archiveStream.end(content); + return await preparedPromise; +} + +describe("backup archive publication", () => { + it("publishes a complete archive and removes its private staging directory", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-publish-"); + const prepared = await prepareArchive(plan); + const originalOpen = fs.open.bind(fs); + const openedPaths: string[] = []; + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + openedPaths.push(path.resolve(String(target))); + return await originalOpen(target, flags, mode); + }); + + try { + await publishPreparedBackupArchive({ plan, prepared }); + + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("complete archive"); + await expect(fs.lstat(prepared.archivePath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.lstat(plan.stagingDir)).rejects.toMatchObject({ code: "ENOENT" }); + expect(openedPaths).not.toContain(path.resolve(outputPath)); + } finally { + openSpy.mockRestore(); + } + }); + + it("removes its staging directory when private setup fails", async () => { + const root = tempDirs.make("openclaw-backup-setup-failure-"); + const outputDir = path.join(root, "backups"); + await fs.mkdir(outputDir); + const chmodSpy = vi + .spyOn(fs, "chmod") + .mockRejectedValue(Object.assign(new Error("chmod failed"), { code: "EIO" })); + try { + await expect( + createBackupArchivePublication(path.join(outputDir, "backup.tar.gz")), + ).rejects.toThrow(/chmod failed/iu); + await expect(fs.readdir(outputDir)).resolves.toEqual([]); + } finally { + chmodSpy.mockRestore(); + } + }); + + it.each(["EPERM", "EXDEV", "ENOTSUP", "EOPNOTSUPP", "ENOSYS"])( + "fails closed when hard-link publication returns %s", + async (code) => { + const { outputPath, plan } = await createPublication("openclaw-backup-no-link-"); + const prepared = await prepareArchive(plan); + const linkSpy = vi + .spyOn(fs, "link") + .mockRejectedValue(Object.assign(new Error("unsupported"), { code })); + try { + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /requires hard-link support/iu, + ); + await expect(fs.lstat(outputPath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.lstat(prepared.archivePath)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + linkSpy.mockRestore(); + } + }, + ); + + it("preserves a destination raced in before publication", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-destination-race-"); + const prepared = await prepareArchive(plan); + await fs.writeFile(outputPath, "racer", "utf8"); + + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /Refusing to overwrite existing backup archive/iu, + ); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("racer"); + }); + + it("rejects a replaced staging pathname without publishing replacement bytes", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-staging-race-"); + const prepared = await prepareArchive(plan); + const originalPath = `${prepared.archivePath}.original`; + await fs.rename(prepared.archivePath, originalPath); + await fs.writeFile(prepared.archivePath, "replacement", "utf8"); + + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /staging file changed/iu, + ); + await expect(fs.lstat(outputPath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.readFile(prepared.archivePath, "utf8")).resolves.toBe("replacement"); + }); + + it.runIf(process.platform !== "win32")( + "rejects a requested output-parent symlink retarget", + async () => { + const root = tempDirs.make("openclaw-backup-parent-retarget-"); + const firstDir = path.join(root, "first"); + const secondDir = path.join(root, "second"); + const requestedDir = path.join(root, "current"); + await fs.mkdir(firstDir); + await fs.mkdir(secondDir); + await fs.symlink(firstDir, requestedDir); + const outputPath = path.join(requestedDir, "backup.tar.gz"); + const plan = await createBackupArchivePublication(outputPath); + const prepared = await prepareArchive(plan); + await fs.unlink(requestedDir); + await fs.symlink(secondDir, requestedDir); + + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /output directory changed/iu, + ); + await expect(fs.lstat(path.join(firstDir, "backup.tar.gz"))).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(fs.lstat(path.join(secondDir, "backup.tar.gz"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "rejects a canonical output-parent directory replacement", + async () => { + const { outputDir, outputPath, plan } = await createPublication( + "openclaw-backup-parent-replace-", + ); + const prepared = await prepareArchive(plan); + const movedOutputDir = `${outputDir}.moved`; + await fs.rename(outputDir, movedOutputDir); + await fs.mkdir(outputDir); + + await expect( + publishPreparedBackupArchive({ + plan, + prepared, + }), + ).rejects.toThrow(/output directory changed/iu); + await expect(fs.lstat(outputPath)).rejects.toMatchObject({ code: "ENOENT" }); + }, + ); + + it.runIf(process.platform !== "win32").each(["EIO", "EINVAL"])( + "preserves the complete final archive when commit directory sync fails with %s", + async (code) => { + const { outputPath, plan } = await createPublication("openclaw-backup-sync-failure-"); + const prepared = await prepareArchive(plan); + const log = vi.fn(); + const originalOpen = fs.open.bind(fs); + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + if (path.resolve(String(target)) === path.resolve(plan.canonicalParentPath)) { + return { + close: vi.fn().mockResolvedValue(undefined), + sync: vi.fn().mockRejectedValue(Object.assign(new Error("sync failed"), { code })), + } as unknown as FileHandle; + } + return await originalOpen(target, flags, mode); + }); + try { + await expect(publishPreparedBackupArchive({ plan, prepared, log })).rejects.toThrow( + /sync failed/iu, + ); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("complete archive"); + expect(log).toHaveBeenCalledWith(expect.stringContaining("concurrent replacement")); + } finally { + openSpy.mockRestore(); + } + }, + ); + + it("preserves a destination that replaces the linked archive before validation", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-linked-race-"); + const prepared = await prepareArchive(plan); + const displacedPath = `${outputPath}.displaced`; + const originalLstat = fs.lstat.bind(fs); + let targetLstatCount = 0; + const lstatSpy = vi.spyOn(fs, "lstat").mockImplementation(async (target, options) => { + if (path.resolve(String(target)) === path.resolve(plan.canonicalOutputPath)) { + targetLstatCount += 1; + } + if (targetLstatCount === 2) { + targetLstatCount += 1; + await fs.rename(plan.canonicalOutputPath, displacedPath); + await fs.writeFile(plan.canonicalOutputPath, "racer", "utf8"); + } + return await originalLstat(target, options); + }); + try { + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /Backup archive changed during publication/iu, + ); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("racer"); + await expect(fs.readFile(displacedPath, "utf8")).resolves.toBe("complete archive"); + } finally { + lstatSpy.mockRestore(); + } + }); + + it("keeps the committed final archive when staging cleanup fails", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-cleanup-failure-"); + const prepared = await prepareArchive(plan); + const log = vi.fn(); + const originalUnlinkSync = fsSync.unlinkSync.bind(fsSync); + const unlinkSpy = vi.spyOn(fsSync, "unlinkSync").mockImplementation((target) => { + if (path.resolve(String(target)) === path.resolve(prepared.archivePath)) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + return originalUnlinkSync(target); + }); + try { + await expect(publishPreparedBackupArchive({ plan, prepared, log })).resolves.toBeUndefined(); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("complete archive"); + expect(log).toHaveBeenCalledWith( + `Backup archiver preserved changed staging file ${prepared.archivePath}.`, + ); + } finally { + unlinkSpy.mockRestore(); + await cleanupBackupArchivePublication(plan); + await expect(fs.lstat(prepared.archivePath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.lstat(plan.stagingDir)).rejects.toMatchObject({ code: "ENOENT" }); + } + }); + + it("retries cleanup when descriptor and pathname identity reads initially fail", async () => { + const { plan } = await createPublication("openclaw-backup-unidentified-partial-"); + const archiveStream = new PassThrough(); + const originalLstatSync = fsSync.lstatSync.bind(fsSync); + const fstatSpy = vi.spyOn(fsSync, "fstatSync").mockImplementationOnce(() => { + throw Object.assign(new Error("fstat failed"), { code: "EIO" }); + }); + let stagedLstatAttempts = 0; + const lstatSpy = vi.spyOn(fsSync, "lstatSync").mockImplementation((target, options) => { + if (path.resolve(String(target)) === path.resolve(plan.tempArchivePath)) { + stagedLstatAttempts += 1; + if (stagedLstatAttempts === 1) { + throw Object.assign(new Error("lstat failed"), { code: "EIO" }); + } + } + return originalLstatSync(target, options); + }); + try { + const writePromise = writeArchiveStreamToFile({ + archivePath: plan.tempArchivePath, + archiveStream, + onPartialArchive: (receipt) => { + plan.pendingCleanupArchives.push(receipt); + }, + }); + archiveStream.end("partial archive"); + + await expect(writePromise).rejects.toThrow("fstat failed"); + expect(plan.pendingCleanupArchives).toEqual([{ archivePath: plan.tempArchivePath }]); + } finally { + fstatSpy.mockRestore(); + lstatSpy.mockRestore(); + } + + await cleanupBackupArchivePublication(plan); + await expect(fs.lstat(plan.tempArchivePath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.lstat(plan.stagingDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("preserves a final-path replacement detected after the commit point", async () => { + const { outputPath, plan } = await createPublication("openclaw-backup-final-race-"); + const prepared = await prepareArchive(plan); + const displacedPath = `${outputPath}.displaced`; + const originalUnlinkSync = fsSync.unlinkSync.bind(fsSync); + let replaced = false; + const unlinkSpy = vi.spyOn(fsSync, "unlinkSync").mockImplementation((target) => { + if (!replaced && path.resolve(String(target)) === path.resolve(prepared.archivePath)) { + replaced = true; + fsSync.renameSync(plan.canonicalOutputPath, displacedPath); + fsSync.writeFileSync(plan.canonicalOutputPath, "racer", "utf8"); + } + return originalUnlinkSync(target); + }); + try { + await expect(publishPreparedBackupArchive({ plan, prepared })).rejects.toThrow( + /Published backup archive changed/iu, + ); + await expect(fs.readFile(outputPath, "utf8")).resolves.toBe("racer"); + await expect(fs.readFile(displacedPath, "utf8")).resolves.toBe("complete archive"); + } finally { + unlinkSpy.mockRestore(); + } + }); +}); diff --git a/src/infra/backup-archive-publication.ts b/src/infra/backup-archive-publication.ts new file mode 100644 index 000000000000..4488aaf32980 --- /dev/null +++ b/src/infra/backup-archive-publication.ts @@ -0,0 +1,398 @@ +import { randomUUID } from "node:crypto"; +import { constants as fsConstants, type Stats } from "node:fs"; +import fs from "node:fs/promises"; +import type { FileHandle } from "node:fs/promises"; +import path from "node:path"; +import { + removePreparedBackupArchive, + type BackupArchiveCleanupReceipt, + type PreparedBackupArchive, +} from "./backup-create-stream.js"; +import { sameFileIdentity } from "./fs-safe-advanced.js"; + +type BackupArchiveLogger = (message: string) => void; + +export type BackupArchivePublication = { + canonicalOutputPath: string; + canonicalParentPath: string; + parentIdentity: Stats; + pendingCleanupArchives: BackupArchiveCleanupReceipt[]; + requestedOutputPath: string; + requestedParentPath: string; + stagingDir: string; + stagingIdentity: Stats; + tempArchivePath: string; +}; + +function pathsEqual(left: string, right: string): boolean { + const resolvedLeft = path.resolve(left); + const resolvedRight = path.resolve(right); + return process.platform === "win32" + ? resolvedLeft.toLowerCase() === resolvedRight.toLowerCase() + : resolvedLeft === resolvedRight; +} + +async function assertTargetAbsent(targetPath: string): Promise { + try { + await fs.lstat(targetPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return; + } + throw error; + } + throw new Error(`Refusing to overwrite existing backup archive: ${targetPath}`); +} + +async function assertPublicationParentUnchanged(plan: BackupArchivePublication): Promise { + const currentCanonicalParent = await fs.realpath(plan.requestedParentPath); + const currentParentIdentity = await fs.lstat(plan.canonicalParentPath); + if ( + !pathsEqual(currentCanonicalParent, plan.canonicalParentPath) || + !currentParentIdentity.isDirectory() || + !sameFileIdentity(plan.parentIdentity, currentParentIdentity) + ) { + throw new Error( + `Backup output directory changed during archive creation: ${plan.requestedParentPath}`, + ); + } +} + +async function removeDirectoryIfOwned( + directoryPath: string, + expectedIdentity: Stats, +): Promise { + // This is a cooperative same-user fence, not hostile local-user isolation; + // SECURITY.md treats co-equal host mutation as inside the operator boundary. + const currentIdentity = await fs.lstat(directoryPath).catch(() => undefined); + if ( + !currentIdentity || + !currentIdentity.isDirectory() || + !sameFileIdentity(expectedIdentity, currentIdentity) + ) { + return false; + } + try { + await fs.rmdir(directoryPath); + return true; + } catch { + return false; + } +} + +async function removeStagingDirectoryIfOwned(plan: BackupArchivePublication): Promise { + return await removeDirectoryIfOwned(plan.stagingDir, plan.stagingIdentity); +} + +function isUnsupportedDirectorySyncError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return ( + code === "EINVAL" || + code === "ENOTSUP" || + code === "ENOSYS" || + (process.platform === "win32" && (code === "EISDIR" || code === "EPERM" || code === "EACCES")) + ); +} + +async function syncDirectoryBestEffort(directoryPath: string): Promise { + const handle = await fs.open(directoryPath, "r").catch((error: unknown) => { + if (isUnsupportedDirectorySyncError(error)) { + return undefined; + } + throw error; + }); + if (!handle) { + return; + } + try { + await handle.sync(); + } catch (error) { + if (!isUnsupportedDirectorySyncError(error)) { + throw error; + } + } finally { + await handle.close(); + } +} + +async function syncPublishedArchiveCommit( + plan: BackupArchivePublication, + preparedHandle: FileHandle, +): Promise { + if (process.platform === "win32") { + // Windows FlushFileBuffers requires a writable file handle and flushes + // buffered file metadata. The prepared handle pins the published inode. + await preparedHandle.sync(); + return; + } + const directoryHandle = await fs.open(plan.canonicalParentPath, "r"); + try { + // Publication success requires a real directory fsync. Unsupported + // filesystems fail closed instead of weakening crash durability. + await directoryHandle.sync(); + } finally { + await directoryHandle.close(); + } +} + +function isUnsupportedHardLinkError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return ( + code === "EPERM" || + code === "EXDEV" || + code === "ENOTSUP" || + code === "EOPNOTSUPP" || + code === "ENOSYS" + ); +} + +async function openPreparedArchive( + plan: BackupArchivePublication, + prepared: PreparedBackupArchive, +): Promise { + const accessMode = process.platform === "win32" ? fsConstants.O_RDWR : fsConstants.O_RDONLY; + const flags = accessMode | (fsConstants.O_NOFOLLOW ?? 0) | (fsConstants.O_NONBLOCK ?? 0); + const handle = await fs.open(prepared.archivePath, flags); + try { + const openedIdentity = await handle.stat(); + const currentIdentity = await fs.lstat(prepared.archivePath); + if ( + !pathsEqual(path.dirname(prepared.archivePath), plan.stagingDir) || + !openedIdentity.isFile() || + !currentIdentity.isFile() || + !sameFileIdentity(prepared.identity, openedIdentity) || + !sameFileIdentity(prepared.identity, currentIdentity) + ) { + throw new Error( + `Backup archive staging file changed before publication: ${prepared.archivePath}`, + ); + } + return handle; + } catch (error) { + await handle.close().catch(() => undefined); + throw error; + } +} + +async function assertPublishedArchiveUnchanged( + plan: BackupArchivePublication, + handle: FileHandle, + expectedIdentity: Stats, +): Promise { + const openedIdentity = await handle.stat(); + const currentIdentity = await fs.lstat(plan.canonicalOutputPath); + if ( + !openedIdentity.isFile() || + !currentIdentity.isFile() || + !sameFileIdentity(expectedIdentity, openedIdentity) || + !sameFileIdentity(expectedIdentity, currentIdentity) + ) { + throw new Error(`Published backup archive changed: ${plan.requestedOutputPath}`); + } +} + +export async function createBackupArchivePublication( + outputPath: string, +): Promise { + const requestedOutputPath = path.resolve(outputPath); + const requestedParentPath = path.dirname(requestedOutputPath); + const canonicalParentPath = await fs.realpath(requestedParentPath); + const parentIdentity = await fs.lstat(canonicalParentPath); + if (!parentIdentity.isDirectory()) { + throw new Error(`Backup output parent is not a directory: ${requestedParentPath}`); + } + const canonicalOutputPath = path.join(canonicalParentPath, path.basename(requestedOutputPath)); + await assertTargetAbsent(canonicalOutputPath); + const stagingDir = await fs.mkdtemp( + path.join(canonicalParentPath, `.openclaw-backup-publish-${randomUUID()}-`), + ); + let stagingIdentity: Stats | undefined; + try { + stagingIdentity = await fs.lstat(stagingDir); + await fs.chmod(stagingDir, 0o700); + return { + canonicalOutputPath, + canonicalParentPath, + parentIdentity, + pendingCleanupArchives: [], + requestedOutputPath, + requestedParentPath, + stagingDir, + stagingIdentity, + tempArchivePath: path.join(stagingDir, "archive.tar.gz.tmp"), + }; + } catch (error) { + if (stagingIdentity) { + await removeDirectoryIfOwned(stagingDir, stagingIdentity); + } + throw error; + } +} + +function retainArchiveForCleanup( + plan: BackupArchivePublication, + receipt: BackupArchiveCleanupReceipt, +): void { + for (const [index, candidate] of plan.pendingCleanupArchives.entries()) { + if (!pathsEqual(candidate.archivePath, receipt.archivePath)) { + continue; + } + if (!candidate.identity || !receipt.identity) { + if (!candidate.identity && receipt.identity) { + plan.pendingCleanupArchives[index] = receipt; + } + return; + } + if (sameFileIdentity(candidate.identity, receipt.identity)) { + return; + } + } + plan.pendingCleanupArchives.push(receipt); +} + +async function removePendingBackupArchive( + plan: BackupArchivePublication, + receipt: BackupArchiveCleanupReceipt, +): Promise { + if (!pathsEqual(path.dirname(receipt.archivePath), plan.stagingDir)) { + return false; + } + if (receipt.identity) { + return removePreparedBackupArchive(receipt as PreparedBackupArchive); + } + let currentIdentity: Stats; + try { + currentIdentity = await fs.lstat(receipt.archivePath); + } catch (error) { + return (error as NodeJS.ErrnoException).code === "ENOENT"; + } + if (!currentIdentity.isFile()) { + return false; + } + return removePreparedBackupArchive({ + archivePath: receipt.archivePath, + identity: currentIdentity, + }); +} + +export async function cleanupBackupArchivePublication( + plan: BackupArchivePublication, + log?: BackupArchiveLogger, +): Promise { + const retainedArchives = plan.pendingCleanupArchives.splice(0); + for (const receipt of retainedArchives) { + if (!(await removePendingBackupArchive(plan, receipt))) { + retainArchiveForCleanup(plan, receipt); + } + } + if (await removeStagingDirectoryIfOwned(plan)) { + await syncDirectoryBestEffort(plan.canonicalParentPath).catch(() => undefined); + return; + } + const currentIdentity = await fs.lstat(plan.stagingDir).catch(() => undefined); + if (currentIdentity) { + log?.(`Backup archiver preserved changed or non-empty staging directory ${plan.stagingDir}.`); + } +} + +export async function publishPreparedBackupArchive(params: { + plan: BackupArchivePublication; + prepared: PreparedBackupArchive; + log?: BackupArchiveLogger; +}): Promise { + const { plan, prepared } = params; + let preparedHandle: FileHandle | undefined; + let publishedIdentity: Stats | undefined; + let hardLinkCreated = false; + let committed = false; + try { + await assertPublicationParentUnchanged(plan); + preparedHandle = await openPreparedArchive(plan, prepared); + await assertTargetAbsent(plan.canonicalOutputPath); + // Node has no portable link-by-handle primitive. Under OpenClaw's one-user + // host trust model, post-link identity checks fence cooperative replacement + // races and ensure a changed staging pathname can never produce success. + try { + await fs.link(prepared.archivePath, plan.canonicalOutputPath); + hardLinkCreated = true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") { + throw new Error( + `Refusing to overwrite existing backup archive: ${plan.requestedOutputPath}`, + { cause: error }, + ); + } + if (isUnsupportedHardLinkError(error)) { + throw new Error( + `Atomic backup publication requires hard-link support in ${plan.requestedParentPath}.`, + { cause: error }, + ); + } + throw error; + } + + await assertPublicationParentUnchanged(plan); + const currentTargetIdentity = await fs.lstat(plan.canonicalOutputPath); + const currentStagingIdentity = await fs.lstat(prepared.archivePath); + if ( + !currentTargetIdentity.isFile() || + !currentStagingIdentity.isFile() || + !sameFileIdentity(prepared.identity, currentTargetIdentity) || + !sameFileIdentity(prepared.identity, currentStagingIdentity) + ) { + throw new Error(`Backup archive changed during publication: ${plan.requestedOutputPath}`); + } + publishedIdentity = currentTargetIdentity; + await assertPublishedArchiveUnchanged(plan, preparedHandle, publishedIdentity); + + // The first parent sync commits the final pathname. After this point, + // cleanup failures must not remove or invalidate the durable archive. + await syncPublishedArchiveCommit(plan, preparedHandle); + committed = true; + + if (!removePreparedBackupArchive(prepared)) { + retainArchiveForCleanup(plan, prepared); + params.log?.(`Backup archiver preserved changed staging file ${prepared.archivePath}.`); + } + if (!(await removeStagingDirectoryIfOwned(plan))) { + params.log?.( + `Backup archiver preserved changed or non-empty staging directory ${plan.stagingDir}.`, + ); + } + await syncDirectoryBestEffort(plan.canonicalParentPath).catch((error: unknown) => { + params.log?.( + `Backup archiver could not sync cleanup in ${plan.canonicalParentPath}: ${ + (error as NodeJS.ErrnoException).code ?? String(error) + }.`, + ); + }); + await assertPublicationParentUnchanged(plan); + await assertPublishedArchiveUnchanged(plan, preparedHandle, publishedIdentity); + } catch (error) { + if (!committed) { + if (!publishedIdentity && hardLinkCreated) { + const currentTargetIdentity = await fs + .lstat(plan.canonicalOutputPath) + .catch(() => undefined); + if ( + currentTargetIdentity?.isFile() && + sameFileIdentity(currentTargetIdentity, prepared.identity) + ) { + publishedIdentity = currentTargetIdentity; + } + } + if (publishedIdentity) { + params.log?.( + `Backup archiver preserved the final archive after publication failed so a concurrent replacement could not be deleted: ${plan.requestedOutputPath}.`, + ); + } + if (!removePreparedBackupArchive(prepared)) { + retainArchiveForCleanup(plan, prepared); + } + await removeStagingDirectoryIfOwned(plan); + } + throw error; + } finally { + await preparedHandle?.close().catch(() => undefined); + } +} diff --git a/src/infra/backup-create-stream.ts b/src/infra/backup-create-stream.ts index 0f652ab4af21..6430cb9c4ded 100644 --- a/src/infra/backup-create-stream.ts +++ b/src/infra/backup-create-stream.ts @@ -1,6 +1,8 @@ -import { createWriteStream } from "node:fs"; +import fsSync, { createWriteStream, type Stats } from "node:fs"; +import fs from "node:fs/promises"; import { Transform } from "node:stream"; import { pipeline } from "node:stream/promises"; +import { sameFileIdentity } from "./fs-safe-advanced.js"; const BACKUP_ARCHIVE_IDLE_TIMEOUT_MS = 5 * 60_000; @@ -8,16 +10,48 @@ type DestroyableArchiveStream = (NodeJS.ReadableStream | AsyncIterable { + onPartialArchive?: (receipt: BackupArchiveCleanupReceipt) => void; +}): Promise { // Own both stream lifecycles so a tar read error closes the output handle // before retry cleanup touches the partial archive. Exclusive creation also // refuses a pre-existing path instead of following a symlink. const idleTimeoutMs = params.idleTimeoutMs ?? BACKUP_ARCHIVE_IDLE_TIMEOUT_MS; const controller = new AbortController(); + let openedIdentity: Stats | undefined; let idleTimer: ReturnType | undefined; let idleTimeoutError: Error | undefined; const armIdleTimer = () => { @@ -39,15 +73,79 @@ export async function writeArchiveStreamToFile(params: { }, }); - armIdleTimer(); + const archiveWriteStream = createWriteStream(params.archivePath, { + flags: "wx", + flush: true, + mode: 0o600, + }); + archiveWriteStream.once("open", (fileDescriptor) => { + try { + openedIdentity = fsSync.fstatSync(fileDescriptor); + } catch (error) { + archiveWriteStream.destroy(error as Error); + } + }); try { - await pipeline( - params.archiveStream, - progress, - createWriteStream(params.archivePath, { flags: "wx", mode: 0o600 }), - { signal: controller.signal }, - ); + const pipelinePromise = pipeline(params.archiveStream, progress, archiveWriteStream, { + signal: controller.signal, + }); + armIdleTimer(); + await pipelinePromise; + const currentIdentity = await fs.lstat(params.archivePath); + if ( + !openedIdentity?.isFile() || + !currentIdentity.isFile() || + !sameFileIdentity(openedIdentity, currentIdentity) + ) { + throw new Error(`Backup archive path changed while writing: ${params.archivePath}`); + } + return { archivePath: params.archivePath, identity: currentIdentity }; } catch (err) { + archiveWriteStream.destroy(); + let cleanupReceipt: BackupArchiveCleanupReceipt | undefined = openedIdentity + ? { archivePath: params.archivePath, identity: openedIdentity } + : undefined; + if (!cleanupReceipt) { + try { + const currentIdentity = fsSync.lstatSync(params.archivePath); + cleanupReceipt = currentIdentity.isFile() + ? { + archivePath: params.archivePath, + identity: currentIdentity, + } + : { archivePath: params.archivePath }; + } catch (cleanupError) { + if ((cleanupError as NodeJS.ErrnoException).code !== "ENOENT") { + // Preserve the cleanup obligation even when the filesystem cannot + // supply an identity until a later outer-cleanup attempt. + cleanupReceipt = { archivePath: params.archivePath }; + } + } + } + if ( + cleanupReceipt && + (!cleanupReceipt.identity || + !removePreparedBackupArchive(cleanupReceipt as PreparedBackupArchive)) + ) { + params.onPartialArchive?.(cleanupReceipt); + } + if (cleanupReceipt && !cleanupReceipt.identity) { + // The outer cleanup owns the retry because this scope cannot safely + // unlink a pathname whose identity is temporarily unavailable. + if (!params.onPartialArchive) { + try { + const currentIdentity = fsSync.lstatSync(cleanupReceipt.archivePath); + if (currentIdentity.isFile()) { + removePreparedBackupArchive({ + archivePath: cleanupReceipt.archivePath, + identity: currentIdentity, + }); + } + } catch { + // No outer owner was provided; preserve the original write error. + } + } + } throw idleTimeoutError ?? err; } finally { if (idleTimer) { diff --git a/src/infra/backup-create.test.ts b/src/infra/backup-create.test.ts index eaf47cf87240..650d49b32ff9 100644 --- a/src/infra/backup-create.test.ts +++ b/src/infra/backup-create.test.ts @@ -1,5 +1,5 @@ // Covers backup archive creation and verification filtering. -import { rmSync } from "node:fs"; +import fsSync, { rmSync } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -394,23 +394,21 @@ describe("writeTarArchiveWithRetry", () => { expect(log).toHaveBeenCalledTimes(2); }); - it("uses a fresh temp archive path when cleanup cannot remove a failed attempt", async () => { + it("uses a fresh temp archive path without pathname-based cleanup", async () => { const eofErr = Object.assign(new Error("did not encounter expected EOF"), { path: "/state/sessions/s-abc/transcript.jsonl", }); const tempArchivePath = "/tmp/backup.tar.gz.tmp"; const runTar = vi - .fn<(attemptTempArchivePath: string) => Promise>() + .fn<(attemptTempArchivePath: string) => Promise>() .mockRejectedValueOnce(eofErr) - .mockResolvedValueOnce(undefined); + .mockResolvedValueOnce("complete"); const log = vi.fn(); const sleep = vi.fn<(ms: number) => Promise>().mockResolvedValue(undefined); - const rmSpy = vi.spyOn(fs, "rm").mockImplementation(async () => { - throw Object.assign(new Error("resource busy"), { code: "EBUSY" }); - }); + const rmSpy = vi.spyOn(fs, "rm"); try { - const completedTempArchivePath = await writeTarArchiveWithRetry({ + const result = await writeTarArchiveWithRetry({ tempArchivePath, runTar, log, @@ -419,17 +417,15 @@ describe("writeTarArchiveWithRetry", () => { expect(runTar).toHaveBeenNthCalledWith(1, tempArchivePath); expect(runTar).toHaveBeenNthCalledWith(2, `${tempArchivePath}.retry-2`); - expect(completedTempArchivePath).toBe(`${tempArchivePath}.retry-2`); - expect(rmSpy).toHaveBeenCalledWith(tempArchivePath, { force: true }); - expect(log).toHaveBeenCalledWith( - `Backup archiver could not remove temp archive ${tempArchivePath} between retries: EBUSY. Continuing.`, - ); + expect(result).toBe("complete"); + expect(rmSpy).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledOnce(); } finally { rmSpy.mockRestore(); } }); - it("cleans retry temp archive paths when a later attempt fails", async () => { + it("does not remove retry paths by pathname when a later attempt fails", async () => { const eofErr = Object.assign(new Error("did not encounter expected EOF"), { path: "/state/sessions/s-abc/transcript.jsonl", }); @@ -452,7 +448,7 @@ describe("writeTarArchiveWithRetry", () => { expect(runTar).toHaveBeenNthCalledWith(1, tempArchivePath); expect(runTar).toHaveBeenNthCalledWith(2, `${tempArchivePath}.retry-2`); - expect(rmSpy).toHaveBeenCalledWith(`${tempArchivePath}.retry-2`, { force: true }); + expect(rmSpy).not.toHaveBeenCalled(); } finally { rmSpy.mockRestore(); } @@ -1891,6 +1887,7 @@ describe("createBackupArchive", () => { const originalReaddir = fs.readdir.bind(fs); let createdLatePath = false; + let stagedArchiveCleanupAttempts = 0; const readdirSpy = vi.spyOn(fs, "readdir").mockImplementation((async ( ...args: unknown[] ) => { @@ -1906,6 +1903,20 @@ describe("createBackupArchive", () => { } return entries; }) as typeof fs.readdir); + const originalUnlinkSync = fsSync.unlinkSync.bind(fsSync); + const unlinkSpy = vi.spyOn(fsSync, "unlinkSync").mockImplementation((target) => { + const targetPath = path.resolve(String(target)); + if ( + targetPath.startsWith(path.resolve(outputDir)) && + targetPath.includes(".openclaw-backup-publish-") + ) { + stagedArchiveCleanupAttempts += 1; + if (stagedArchiveCleanupAttempts === 1) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + } + return originalUnlinkSync(target); + }); try { await expect( @@ -1916,8 +1927,10 @@ describe("createBackupArchive", () => { }), ).rejects.toThrow(/SQLite state appeared after snapshot discovery/); expect(createdLatePath).toBe(true); + expect(stagedArchiveCleanupAttempts).toBeGreaterThanOrEqual(2); expect(await fs.readdir(outputDir)).toEqual([]); } finally { + unlinkSpy.mockRestore(); readdirSpy.mockRestore(); } }, @@ -2626,40 +2639,56 @@ describe("createBackupArchive", () => { }); describe.runIf(process.platform !== "win32")("archive permissions", () => { - it.each([ - ["hard link", false], - ["copy fallback", true], - ] as const)("publishes via %s with owner-only 0o600 permissions", async (_name, forceCopy) => { - const linkSpy = forceCopy - ? vi - .spyOn(fs, "link") - .mockRejectedValue( - Object.assign(new Error("hard links unsupported"), { code: "EPERM" }), - ) - : undefined; + it("publishes via hard link with owner-only 0o600 permissions", async () => { + await withOpenClawTestState( + { + layout: "state-only", + prefix: "openclaw-backup-mode-", + scenario: "minimal", + }, + async (state) => { + const outputDir = state.path("backups"); + await fs.mkdir(outputDir, { recursive: true }); + + const result = await createBackupArchive({ + output: outputDir, + includeWorkspace: false, + nowMs: Date.UTC(2026, 4, 9, 12, 0, 0), + }); + + const stat = await fs.stat(result.archivePath); + expect(stat.mode & 0o777).toBe(0o600); + }, + ); + }); + + it("fails closed when the destination does not support hard links", async () => { + const linkSpy = vi + .spyOn(fs, "link") + .mockRejectedValue(Object.assign(new Error("hard links unsupported"), { code: "EPERM" })); try { await withOpenClawTestState( { layout: "state-only", - prefix: "openclaw-backup-mode-", + prefix: "openclaw-backup-no-hardlinks-", scenario: "minimal", }, async (state) => { const outputDir = state.path("backups"); await fs.mkdir(outputDir, { recursive: true }); - const result = await createBackupArchive({ - output: outputDir, - includeWorkspace: false, - nowMs: Date.UTC(2026, 4, 9, 12, 0, 0), - }); - - const stat = await fs.stat(result.archivePath); - expect(stat.mode & 0o777).toBe(0o600); + await expect( + createBackupArchive({ + output: outputDir, + includeWorkspace: false, + nowMs: Date.UTC(2026, 4, 9, 12, 0, 0), + }), + ).rejects.toThrow(/requires hard-link support/iu); + await expect(fs.readdir(outputDir)).resolves.toEqual([]); }, ); } finally { - linkSpy?.mockRestore(); + linkSpy.mockRestore(); } }); }); diff --git a/src/infra/backup-create.ts b/src/infra/backup-create.ts index a3c6068d0a2e..5dac59c0e85c 100644 --- a/src/infra/backup-create.ts +++ b/src/infra/backup-create.ts @@ -1,6 +1,5 @@ // Creates backup archives while filtering volatile runtime state. -import { randomUUID } from "node:crypto"; -import { constants as fsConstants, type Stats } from "node:fs"; +import type { Stats } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -24,12 +23,14 @@ import { } from "../state/openclaw-state-snapshot-sanitizer.js"; import { resolveHomeDir, resolveUserPath } from "../utils.js"; import { resolveRuntimeServiceVersion } from "../version.js"; -import { writeArchiveStreamToFile } from "./backup-create-stream.js"; import { - removeBackupTempArchiveBestEffort, - resolveBackupTarAttemptTempPaths, - writeTarArchiveWithRetry, -} from "./backup-tar-retry.js"; + cleanupBackupArchivePublication, + createBackupArchivePublication, + publishPreparedBackupArchive, + type BackupArchivePublication, +} from "./backup-archive-publication.js"; +import { removePreparedBackupArchive, writeArchiveStreamToFile } from "./backup-create-stream.js"; +import { writeTarArchiveWithRetry } from "./backup-tar-retry.js"; import { isVolatileBackupPath } from "./backup-volatile-filter.js"; import { createBackupVolatileStatCache } from "./backup-volatile-stat-cache.js"; import { formatErrorMessage } from "./errors.js"; @@ -179,10 +180,6 @@ async function assertOutputPathReady(outputPath: string): Promise { } } -function buildTempArchivePath(outputPath: string): string { - return `${outputPath}.${randomUUID()}.tmp`; -} - // The temp manifest is passed to `tar.c` alongside the asset source paths. If // the temp file lives inside any asset, recursive traversal pulls it in a // second time and both copies remap to `/manifest.json`, which @@ -220,46 +217,6 @@ async function chooseBackupTempRoot(params: { return fallback; } -function isLinkUnsupportedError(code: string | undefined): boolean { - return code === "ENOTSUP" || code === "EOPNOTSUPP" || code === "EPERM"; -} - -async function publishTempArchive(params: { - tempArchivePath: string; - outputPath: string; -}): Promise { - try { - await fs.link(params.tempArchivePath, params.outputPath); - } catch (err) { - const code = (err as NodeJS.ErrnoException | undefined)?.code; - if (code === "EEXIST") { - throw new Error(`Refusing to overwrite existing backup archive: ${params.outputPath}`, { - cause: err, - }); - } - if (!isLinkUnsupportedError(code)) { - throw err; - } - - try { - // Some backup targets support ordinary files but not hard links. - await fs.copyFile(params.tempArchivePath, params.outputPath, fsConstants.COPYFILE_EXCL); - } catch (copyErr) { - const copyCode = (copyErr as NodeJS.ErrnoException | undefined)?.code; - if (copyCode !== "EEXIST") { - await fs.rm(params.outputPath, { force: true }).catch(() => undefined); - } - if (copyCode === "EEXIST") { - throw new Error(`Refusing to overwrite existing backup archive: ${params.outputPath}`, { - cause: copyErr, - }); - } - throw copyErr; - } - } - await fs.rm(params.tempArchivePath, { force: true }); -} - async function canonicalizePathForContainment(targetPath: string): Promise { const resolved = path.resolve(targetPath); const suffix: string[] = []; @@ -845,8 +802,14 @@ export async function createBackupArchive( await fs.mkdir(tempRoot, { recursive: true }); const tempDir = await fs.mkdtemp(path.join(tempRoot, "openclaw-backup-")); const manifestPath = path.join(tempDir, "manifest.json"); - const tempArchivePath = buildTempArchivePath(outputPath); - const tempArchiveCleanupPaths = resolveBackupTarAttemptTempPaths(tempArchivePath); + let publication: BackupArchivePublication; + try { + publication = await createBackupArchivePublication(outputPath); + } catch (error) { + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => undefined); + throw error; + } + const tempArchivePath = publication.tempArchivePath; const stateAsset = result.assets.find((asset) => asset.kind === "state"); const preservedStatePaths = [ plan.configPath, @@ -966,7 +929,7 @@ export async function createBackupArchive( } return true; }; - const completedTempArchivePath = await writeTarArchiveWithRetry({ + const completedArchive = await writeTarArchiveWithRetry({ tempArchivePath, log: opts.log, runTar: async (attemptTempArchivePath) => { @@ -975,7 +938,7 @@ export async function createBackupArchive( // cumulative skip counts across attempts instead of the final one. skippedVolatileCount = 0; unexpectedSqliteSourcePaths.length = 0; - await writeArchiveStreamToFile({ + const prepared = await writeArchiveStreamToFile({ archivePath: attemptTempArchivePath, archiveStream: tar.c( { @@ -1001,13 +964,20 @@ export async function createBackupArchive( ...result.assets.map((asset) => asset.sourcePath), ], ), + onPartialArchive: (partialArchive) => { + publication.pendingCleanupArchives.push(partialArchive); + }, }); const unexpectedSqliteSourcePath = unexpectedSqliteSourcePaths[0]; if (unexpectedSqliteSourcePath) { + if (!removePreparedBackupArchive(prepared)) { + publication.pendingCleanupArchives.push(prepared); + } throw new Error( `SQLite state appeared after snapshot discovery: ${unexpectedSqliteSourcePath}. Retry backup so it can be snapshotted.`, ); } + return prepared; }, }); result.skippedVolatileCount = skippedVolatileCount; @@ -1018,11 +988,13 @@ export async function createBackupArchive( } (live sessions, cron logs, queues, sockets, pid/tmp).`, ); } - await publishTempArchive({ tempArchivePath: completedTempArchivePath, outputPath }); + await publishPreparedBackupArchive({ + plan: publication, + prepared: completedArchive, + log: opts.log, + }); } finally { - for (const cleanupPath of tempArchiveCleanupPaths) { - await removeBackupTempArchiveBestEffort(cleanupPath); - } + await cleanupBackupArchivePublication(publication, opts.log); await fs.rm(tempDir, { recursive: true, force: true }).catch(() => undefined); } diff --git a/src/infra/backup-create.windows.test.ts b/src/infra/backup-create.windows.test.ts index 4d1cac35bd45..5713608de85d 100644 --- a/src/infra/backup-create.windows.test.ts +++ b/src/infra/backup-create.windows.test.ts @@ -1,3 +1,4 @@ +import fsSync from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { PassThrough } from "node:stream"; @@ -8,6 +9,27 @@ import { writeArchiveStreamToFile } from "./backup-create-stream.js"; const tempDirs = useAutoCleanupTempDirTracker(afterEach); describe("writeArchiveStreamToFile", () => { + it("removes the exclusive partial archive when its initial descriptor stat fails", async () => { + const tempDir = tempDirs.make("openclaw-backup-stream-fstat-"); + const archivePath = path.join(tempDir, "partial.tar.gz"); + const archiveStream = new PassThrough(); + const fstatSpy = vi.spyOn(fsSync, "fstatSync").mockImplementationOnce(() => { + throw Object.assign(new Error("fstat failed"), { code: "EIO" }); + }); + try { + const writePromise = writeArchiveStreamToFile({ + archivePath, + archiveStream, + }); + archiveStream.end("partial archive"); + + await expect(writePromise).rejects.toThrow("fstat failed"); + await expect(fs.lstat(archivePath)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + fstatSpy.mockRestore(); + } + }); + it("closes a partial archive before propagating a stream error", async () => { const tempDir = tempDirs.make("openclaw-backup-stream-"); const archivePath = path.join(tempDir, "partial.tar.gz"); @@ -20,7 +42,7 @@ describe("writeArchiveStreamToFile", () => { archiveStream.destroy(new Error("injected tar read failure")); await expect(writePromise).rejects.toThrow("injected tar read failure"); - await expect(fs.rm(archivePath)).resolves.toBeUndefined(); + await expect(fs.lstat(archivePath)).rejects.toMatchObject({ code: "ENOENT" }); }); it("aborts and closes a partial archive when the source stops producing data", async () => { @@ -42,7 +64,7 @@ describe("writeArchiveStreamToFile", () => { await vi.advanceTimersByTimeAsync(60); await rejection; expect(archiveStream.destroyed).toBe(true); - await expect(fs.rm(archivePath)).resolves.toBeUndefined(); + await expect(fs.lstat(archivePath)).rejects.toMatchObject({ code: "ENOENT" }); } finally { vi.useRealTimers(); } @@ -66,7 +88,7 @@ describe("writeArchiveStreamToFile", () => { await vi.advanceTimersByTimeAsync(40); archiveStream.end("third"); - await expect(writePromise).resolves.toBeUndefined(); + await expect(writePromise).resolves.toMatchObject({ archivePath }); await expect(fs.readFile(archivePath, "utf8")).resolves.toBe("firstsecondthird"); } finally { vi.useRealTimers(); diff --git a/src/infra/backup-tar-retry.ts b/src/infra/backup-tar-retry.ts index c91d8e0aabc8..45c904ab25c8 100644 --- a/src/infra/backup-tar-retry.ts +++ b/src/infra/backup-tar-retry.ts @@ -1,4 +1,3 @@ -import fs from "node:fs/promises"; import { sleep } from "../utils/sleep.js"; const BACKUP_TAR_MAX_ATTEMPTS = 3; @@ -24,52 +23,25 @@ function resolveBackupTarAttemptTempPath(tempArchivePath: string, attempt: numbe return attempt === 1 ? tempArchivePath : `${tempArchivePath}.retry-${attempt}`; } -export function resolveBackupTarAttemptTempPaths(tempArchivePath: string): string[] { - return Array.from({ length: BACKUP_TAR_MAX_ATTEMPTS }, (_value, index) => - resolveBackupTarAttemptTempPath(tempArchivePath, index + 1), - ); -} - -export async function removeBackupTempArchiveBestEffort(tempArchivePath: string): Promise { - await fs.rm(tempArchivePath, { force: true }).catch(() => undefined); -} - -export async function writeTarArchiveWithRetry(params: { +export async function writeTarArchiveWithRetry(params: { tempArchivePath: string; - runTar: (tempArchivePath: string) => Promise; + runTar: (tempArchivePath: string) => Promise; log?: BackupTarRetryLogger; sleepMs?: (ms: number) => Promise; -}): Promise { +}): Promise { const sleepFn = params.sleepMs ?? sleep; let lastErr: unknown; - const attemptTempArchivePaths: string[] = []; for (let attempt = 1; attempt <= BACKUP_TAR_MAX_ATTEMPTS; attempt += 1) { const attemptTempArchivePath = resolveBackupTarAttemptTempPath(params.tempArchivePath, attempt); - attemptTempArchivePaths.push(attemptTempArchivePath); try { - await params.runTar(attemptTempArchivePath); - for (const staleTempArchivePath of attemptTempArchivePaths.slice(0, -1)) { - await removeBackupTempArchiveBestEffort(staleTempArchivePath); - } - return attemptTempArchivePath; + return await params.runTar(attemptTempArchivePath); } catch (err) { lastErr = err; if (!isTarEofRaceError(err) || attempt === BACKUP_TAR_MAX_ATTEMPTS) { - for (const staleTempArchivePath of attemptTempArchivePaths) { - await removeBackupTempArchiveBestEffort(staleTempArchivePath); - } break; } - try { - await fs.rm(attemptTempArchivePath, { force: true }); - } catch (cleanupErr) { - const code = (cleanupErr as NodeJS.ErrnoException).code; - if (code && code !== "ENOENT") { - params.log?.( - `Backup archiver could not remove temp archive ${attemptTempArchivePath} between retries: ${code}. Continuing.`, - ); - } - } + // The writer owns checked cleanup inside the private staging directory. + // A fresh path keeps retries independent when a changed entry is preserved. const backoff = BACKUP_TAR_BACKOFF_MS[attempt - 1] ?? 0; const offendingPath = (err as NodeJS.ErrnoException).path; params.log?.(