From 05dd440dea2c99ad53ea167e3beae8f794c37702 Mon Sep 17 00:00:00 2001 From: Nico <47644445+nicotsx@users.noreply.github.com> Date: Sat, 21 Mar 2026 20:50:23 +0100 Subject: [PATCH] fix(restic): separate error summary from diagnostic details (#694) --- app/server/app.ts | 4 +-- .../__tests__/backups.execution.test.ts | 32 ++++++++++++++++++- .../modules/backups/backups.execution.ts | 9 +++--- .../__tests__/repositories.controller.test.ts | 3 +- app/server/utils/__tests__/spawn.test.ts | 18 +++++++++++ app/server/utils/errors.ts | 17 ++++++++++ .../restic/commands/__tests__/backup.test.ts | 21 ++++++++++++ packages/core/src/restic/commands/backup.ts | 4 +-- packages/core/src/restic/commands/ls.ts | 2 +- packages/core/src/restic/commands/restore.ts | 2 +- packages/core/src/restic/error.ts | 10 ++++-- packages/core/src/utils/spawn.ts | 10 ++++++ 12 files changed, 117 insertions(+), 15 deletions(-) diff --git a/app/server/app.ts b/app/server/app.ts index fca8126f..bdf541fe 100644 --- a/app/server/app.ts +++ b/app/server/app.ts @@ -92,9 +92,9 @@ export const createApp = () => { logger.error(err.cause.message); } - const { status, message } = handleServiceError(err); + const { status, message, details } = handleServiceError(err); - return c.json({ message }, status as 500); + return c.json(details ? { message, details } : { message }, status as 500); }); return app; diff --git a/app/server/modules/backups/__tests__/backups.execution.test.ts b/app/server/modules/backups/__tests__/backups.execution.test.ts index bc984c58..28980273 100644 --- a/app/server/modules/backups/__tests__/backups.execution.test.ts +++ b/app/server/modules/backups/__tests__/backups.execution.test.ts @@ -177,6 +177,36 @@ describe("stop backup", () => { expect(updatedSchedule.lastBackupError).toBe("error: open /mnt/data/private.db: permission denied"); }); + test("should store restic diagnostic details instead of the generic summary on hard failure", async () => { + const { resticBackupMock } = setup(); + const volume = await createTestVolume(); + const repository = await createTestRepository(); + const schedule = await createTestBackupSchedule({ + volumeId: volume.id, + repositoryId: repository.id, + }); + + resticBackupMock.mockImplementationOnce((params: SafeSpawnParams) => { + params.onStderr?.("Permissions 0755 for '/tmp/zerobyte-ssh-key' are too open."); + params.onStderr?.("This private key will be ignored."); + + return Promise.resolve({ + exitCode: 1, + summary: "", + error: "ssh command exited", + stderr: "Permissions 0755 for '/tmp/zerobyte-ssh-key' are too open.\nThis private key will be ignored.", + }); + }); + + await backupsExecutionService.executeBackup(schedule.id); + + const updatedSchedule = await backupsService.getScheduleById(schedule.id); + expect(updatedSchedule.lastBackupStatus).toBe("error"); + expect(updatedSchedule.lastBackupError).toBe( + "Permissions 0755 for '/tmp/zerobyte-ssh-key' are too open.\nThis private key will be ignored.", + ); + }); + test("should stop a running backup", async () => { // arrange const { resticBackupMock } = setup(); @@ -205,7 +235,7 @@ describe("stop backup", () => { // assert const updatedSchedule = await backupsService.getScheduleById(schedule.id); expect(updatedSchedule.lastBackupStatus).toBe("warning"); - expect(updatedSchedule.lastBackupError).toBe("Backup was stopped by user"); + expect(updatedSchedule.lastBackupError).toBe("Backup was stopped by the user"); }); test("should throw ConflictError when trying to stop non-running backup", async () => { diff --git a/app/server/modules/backups/backups.execution.ts b/app/server/modules/backups/backups.execution.ts index 11a8ba4e..6de2b227 100644 --- a/app/server/modules/backups/backups.execution.ts +++ b/app/server/modules/backups/backups.execution.ts @@ -4,7 +4,7 @@ import { restic } from "../../core/restic"; import { logger } from "@zerobyte/core/node"; import { cache, cacheKeys } from "../../utils/cache"; import { getVolumePath } from "../volumes/helpers"; -import { toMessage } from "../../utils/errors"; +import { toErrorDetails, toMessage } from "../../utils/errors"; import { serverEvents } from "../../core/events"; import { notificationsService } from "../notifications/notifications.service"; import { repoMutex } from "../../core/repository-mutex"; @@ -225,11 +225,12 @@ const handleBackupFailure = async ( partialContext?: Partial, ): Promise => { const errorMessage = toMessage(error); + const errorDetails = toErrorDetails(error); await scheduleQueries.updateStatus(scheduleId, organizationId, { lastBackupAt: Date.now(), lastBackupStatus: "error", - lastBackupError: errorMessage, + lastBackupError: errorDetails, }); if (partialContext?.schedule && partialContext?.volume && partialContext?.repository) { @@ -252,7 +253,7 @@ const handleBackupFailure = async ( volumeName: ctx.volume.name, repositoryName: ctx.repository.name, scheduleName: ctx.schedule.name, - error: errorMessage, + error: errorDetails, }) .catch((notifError) => { logger.error(`Failed to send backup failure notification: ${toMessage(notifError)}`); @@ -323,7 +324,7 @@ const stopBackup = async (scheduleId: number) => { } finally { await scheduleQueries.updateStatus(scheduleId, organizationId, { lastBackupStatus: "warning", - lastBackupError: "Backup was stopped by user", + lastBackupError: "Backup was stopped by the user", }); } }; diff --git a/app/server/modules/repositories/__tests__/repositories.controller.test.ts b/app/server/modules/repositories/__tests__/repositories.controller.test.ts index ed8d0b75..7b255ec3 100644 --- a/app/server/modules/repositories/__tests__/repositories.controller.test.ts +++ b/app/server/modules/repositories/__tests__/repositories.controller.test.ts @@ -341,7 +341,8 @@ describe("repositories updates", () => { expect(res.status).toBe(500); const body = await res.json(); - expect(body.message).toContain("Command failed"); + expect(body.message).toBe("Command failed: An error occurred while executing the command."); + expect(body.details).toBe("Fatal: unexpected HTTP response (403): 403 Forbidden"); } finally { deleteSnapshotSpy.mockRestore(); } diff --git a/app/server/utils/__tests__/spawn.test.ts b/app/server/utils/__tests__/spawn.test.ts index 7758eea8..8066a442 100644 --- a/app/server/utils/__tests__/spawn.test.ts +++ b/app/server/utils/__tests__/spawn.test.ts @@ -206,6 +206,24 @@ describe("safeSpawn", () => { expect(result.error).toBe("err_last"); }); + test("stderr keeps the captured stderr output", async () => { + const result = await safeSpawn({ + command: "sh", + args: ["-c", "echo err_first >&2 && echo err_last >&2 && exit 1"], + }); + + expect(result.stderr).toBe("err_first\nerr_last"); + }); + + test("stderr keeps only the last 50 stderr lines", async () => { + const result = await safeSpawn({ + command: "sh", + args: ["-c", "i=1; while [ $i -le 55 ]; do echo err_$i >&2; i=$((i+1)); done; exit 1"], + }); + + expect(result.stderr).toBe(Array.from({ length: 50 }, (_, index) => `err_${index + 6}`).join("\n")); + }); + test("returns exitCode -1 when the command is not found", async () => { const result = await safeSpawn({ command: "this-command-does-not-exist-zerobyte", diff --git a/app/server/utils/errors.ts b/app/server/utils/errors.ts index b592505e..d1adf687 100644 --- a/app/server/utils/errors.ts +++ b/app/server/utils/errors.ts @@ -1,11 +1,20 @@ import { HttpError } from "http-errors-enhanced"; import { sanitizeSensitiveData } from "@zerobyte/core/node"; +import { ResticError } from "@zerobyte/core/restic"; export const handleServiceError = (error: unknown) => { if (error instanceof HttpError) { return { message: sanitizeSensitiveData(error.message), status: error.statusCode }; } + if (error instanceof ResticError) { + return { + message: sanitizeSensitiveData(error.summary), + details: error.details ? sanitizeSensitiveData(error.details) : undefined, + status: 500 as const, + }; + } + return { message: sanitizeSensitiveData(toMessage(error)), status: 500 as const }; }; @@ -13,3 +22,11 @@ export const toMessage = (err: unknown): string => { const message = err instanceof Error ? err.message : String(err); return sanitizeSensitiveData(message); }; + +export const toErrorDetails = (err: unknown): string => { + if (err instanceof ResticError) { + return sanitizeSensitiveData(err.details || err.summary); + } + + return toMessage(err); +}; diff --git a/packages/core/src/restic/commands/__tests__/backup.test.ts b/packages/core/src/restic/commands/__tests__/backup.test.ts index 476c253f..9c4eb763 100644 --- a/packages/core/src/restic/commands/__tests__/backup.test.ts +++ b/packages/core/src/restic/commands/__tests__/backup.test.ts @@ -301,6 +301,27 @@ describe("backup command", () => { expect((error as ResticError).code).toBe(12); }); + test("stores restic summary separately from diagnostic stderr", async () => { + setup({ + spawnResult: { + exitCode: 1, + summary: "", + error: "ssh command exited", + }, + onSpawnCall: (params) => { + params.onStderr?.("Permissions 0755 for '/tmp/zerobyte-ssh-key' are too open."); + params.onStderr?.("This private key will be ignored."); + }, + }); + + const error = await backup(config, "/mnt/data", { organizationId: "org-1" }, mockDeps).catch((e) => e); + expect(error).toBeInstanceOf(ResticError); + expect((error as ResticError).summary).toBe("Command failed: An error occurred while executing the command."); + expect((error as ResticError).details).toBe( + "Permissions 0755 for '/tmp/zerobyte-ssh-key' are too open.\nThis private key will be ignored.", + ); + }); + test("returns { result: null } when the abort signal is triggered", async () => { const controller = new AbortController(); setup({ diff --git a/packages/core/src/restic/commands/backup.ts b/packages/core/src/restic/commands/backup.ts index 1a9078e1..1c4f3eeb 100644 --- a/packages/core/src/restic/commands/backup.ts +++ b/packages/core/src/restic/commands/backup.ts @@ -170,7 +170,7 @@ export const backup = async ( if (options.signal?.aborted) { logger.warn("Restic backup was aborted by signal."); - return { result: null, exitCode: res.exitCode, warningDetails: null }; + return { result: null, exitCode: res.exitCode, warningDetails: "Backup was stopped by the user" }; } if (res.exitCode === 3) { @@ -181,7 +181,7 @@ export const backup = async ( logger.error(`Restic backup failed: ${res.error}`); logger.error(`Command executed: restic ${args.join(" ")}`); - throw new ResticError(res.exitCode, res.error); + throw new ResticError(res.exitCode, stderrLines.join("\n") || res.stderr || res.error); } const lastLine = res.summary.trim(); diff --git a/packages/core/src/restic/commands/ls.ts b/packages/core/src/restic/commands/ls.ts index 1d703025..4ea52265 100644 --- a/packages/core/src/restic/commands/ls.ts +++ b/packages/core/src/restic/commands/ls.ts @@ -126,7 +126,7 @@ export const ls = async ( if (res.exitCode !== 0) { logger.error(`Restic ls failed: ${res.error}`); - throw new ResticError(res.exitCode, res.error); + throw new ResticError(res.exitCode, res.stderr || res.error); } if (totalNodes > offset + limit) { diff --git a/packages/core/src/restic/commands/restore.ts b/packages/core/src/restic/commands/restore.ts index 8788f0c1..2f298e49 100644 --- a/packages/core/src/restic/commands/restore.ts +++ b/packages/core/src/restic/commands/restore.ts @@ -134,7 +134,7 @@ export const restore = async ( if (res.exitCode !== 0) { logger.error(`Restic restore failed: ${res.error}`); - throw new ResticError(res.exitCode, res.error); + throw new ResticError(res.exitCode, res.stderr || res.error); } const lastLine = res.summary.trim(); diff --git a/packages/core/src/restic/error.ts b/packages/core/src/restic/error.ts index 60e039b7..7f631543 100644 --- a/packages/core/src/restic/error.ts +++ b/packages/core/src/restic/error.ts @@ -10,12 +10,16 @@ const resticErrorCodes: Record = { export class ResticError extends Error { code: number; + summary: string; + details: string; - constructor(code: number, stderr: string) { - const message = resticErrorCodes[code] || `Unknown restic error with code ${code}`; - super(`${message}\n${stderr}`); + constructor(code: number, details: string) { + const summary = resticErrorCodes[code] || `Unknown restic error with code ${code}`; + super(details ? `${summary}\n${details}` : summary); this.code = code; + this.summary = summary; + this.details = details; this.name = "ResticError"; } } diff --git a/packages/core/src/utils/spawn.ts b/packages/core/src/utils/spawn.ts index 35b2e0e0..618ee7ac 100644 --- a/packages/core/src/utils/spawn.ts +++ b/packages/core/src/utils/spawn.ts @@ -59,8 +59,11 @@ export type SpawnResult = { exitCode: number; summary: string; error: string; + stderr?: string; }; +const MAX_STDERR_LINES = 50; + export function safeSpawn(params: SafeSpawnParamsLines): Promise; export function safeSpawn(params: SafeSpawnParamsRaw): Promise; export function safeSpawn(params: SafeSpawnParams): Promise { @@ -70,6 +73,7 @@ export function safeSpawn(params: SafeSpawnParams): Promise { let lastStdout = ""; let lastStderr = ""; + const stderrLines: string[] = []; return new Promise((resolve) => { const child = spawn(command, args, { @@ -101,6 +105,10 @@ export function safeSpawn(params: SafeSpawnParams): Promise { rlErr.on("line", (line) => { if (onStderr) onStderr(line); + stderrLines.push(line); + if (stderrLines.length > MAX_STDERR_LINES) { + stderrLines.shift(); + } const trimmed = line.trim(); if (trimmed.length > 0) { lastStderr = line; @@ -115,6 +123,7 @@ export function safeSpawn(params: SafeSpawnParams): Promise { exitCode: -1, summary: lastStdout, error: err.message || lastStderr, + stderr: stderrLines.join("\n"), }); }); @@ -123,6 +132,7 @@ export function safeSpawn(params: SafeSpawnParams): Promise { exitCode: code ?? -1, summary: lastStdout, error: lastStderr, + stderr: stderrLines.join("\n"), }); }); });