From 0952a2ee4737384b3a0cfa5916cacbefd16c7ffb Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 22 Oct 2026 19:14:08 +0200 Subject: [PATCH] fix(config-backup): a restore that fails partway names what it restored and what failed Merge-time fix for #661. A restore that could not write one file (a full disk, a directory where the file should be) used to stop there and answer a bare 500, naming neither the files it had already put back nor the pre-restore copy, and the Settings form was not re-read, so the next Save wrote the old settings back over a restored settings.json. restoreConfigBackup now writes every verified file it can, then throws ConfigBackupPartialRestoreError with restored, failed (name and error), untouched and safetySnapshot. The route runs the same reload hooks and audit for the files written as a full restore does, and answers 500 with the error envelope plus those fields (and restartRequired) in data, as the tab-layout 409 carries its layout. Settings refills the form when settings.json is among the restored files, also on that path. Tests: a unit case where a later file is still restored, the clis.json-as-a-directory route case, and two vm cases for the Settings handler. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/api-reference.md | 2 +- docs/wiki/Config-Backups.md | 6 +++ src/config-backup.ts | 45 +++++++++++++++--- src/web/public/settings-ui.js | 13 +++-- src/web/routes/config-backup-routes.ts | 60 +++++++++++++++++------- test/config-backup-restore-ui.test.ts | 48 ++++++++++++++++--- test/config-backup.test.ts | 25 ++++++++++ test/routes/config-backup-routes.test.ts | 30 ++++++++++++ 8 files changed, 196 insertions(+), 33 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index ae5e881c..82af1322 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1165,7 +1165,7 @@ Settings (synced, in `settings.json`): `configBackupEnabled` (absent = on), `con | `GET` | `/api/config-backups` | none | `settings` in effect, `dir`, `defaultDir`, `tracked[]` and `backups[]` (`id`, `createdAt`, `reason`, `appVersion`, `files[]`, `bytes`), newest first. | | `POST` | `/api/config-backups` | none | Snapshot now, even if nothing changed (`reason: manual`), then apply retention. `status` is `created` or `empty` (nothing tracked exists yet); `removed[]` lists the ids retention deleted. `503` when a custom folder does not answer. | | `GET` | `/api/config-backups/diff?from=&to=` | none | What differs between two states: `from`/`to` are a backup id or `current` (the running config; the default for `to`). `files[]` (`name`, `status` `added`/`removed`/`changed`/`unchanged`, `changes[]` of `path`, `kind`, `before`/`after`, `truncated`, `detail`) and `changed`. Credential values are never returned (`redacted: true`). `400` without `from`; `404` unknown id; `409` if a file fails its checksum. | -| `POST` | `/api/config-backups/:id/restore` | none | Put a snapshot's files back, atomically, after a `pre-restore` snapshot of the current ones. `404` unknown id; `409 CONFLICT` if a file fails its checksum (nothing changed). Returns `restored[]`, `untouched[]` (tracked files present now that the backup did not hold; left as they are), `safetySnapshot` (the `pre-restore` id) and `restartRequired[]` (files the running server only reads at start: `.env`, `push-keys.json`, `settings.json`). | +| `POST` | `/api/config-backups/:id/restore` | none | Put a snapshot's files back, atomically, after a `pre-restore` snapshot of the current ones. `404` unknown id; `409 CONFLICT` if a file fails its checksum (nothing changed). Returns `restored[]`, `untouched[]` (tracked files present now that the backup did not hold; left as they are), `safetySnapshot` (the `pre-restore` id) and `restartRequired[]` (files the running server only reads at start: `.env`, `push-keys.json`, `settings.json`). A file that cannot be written (a full disk, a directory in its place) does not stop the others: the answer is then `500 OPERATION_FAILED` with the same fields in `data` plus `failed[]` (`name`, `error`); the files listed in `restored[]` are the backup's and are reloaded like a full restore. | ## Webhook notifications diff --git a/docs/wiki/Config-Backups.md b/docs/wiki/Config-Backups.md index 5dabfd24..3918af94 100644 --- a/docs/wiki/Config-Backups.md +++ b/docs/wiki/Config-Backups.md @@ -137,6 +137,12 @@ From the list, press **Restore** on a backup and confirm. Codeman then: Only the tracked names are ever written, whatever a manifest lists. +If a file cannot be written (a full disk, a folder where the file should be), the restore still +writes the others and then reports an error that names the files it restored, the ones it could +not write, and the `pre-restore` backup that holds the files as they were before. The restored +files are in effect as after a full restore (Settings re-reads `settings.json` into the form), so +fix the cause and restore again, or restore the `pre-restore` backup to go back. + Restoring `settings.json` keeps the four backup settings (on or off, folder, how many, how many days) at their current values, so a restore from before a folder change does not point the list at the old folder and hide the `pre-restore` copy it just made. The exception is the two diff --git a/src/config-backup.ts b/src/config-backup.ts index 34b4ec86..adf58a5b 100644 --- a/src/config-backup.ts +++ b/src/config-backup.ts @@ -476,6 +476,26 @@ export function pruneConfigBackups(dir: string, opts: PruneOptions): string[] { export class ConfigBackupNotFoundError extends Error {} export class ConfigBackupCorruptError extends Error {} +/** + * A restore that wrote some files and could not write others (a full disk, a directory where a file + * should be). Every file passed verification first, so the ones written are the backup's: the caller + * still has to reload what they feed and tell the user which files are which. + */ +export class ConfigBackupPartialRestoreError extends Error { + constructor( + readonly restored: string[], + readonly failed: Array<{ name: string; error: string }>, + readonly untouched: string[], + readonly safetySnapshot: string | null + ) { + const done = restored.length ? `${restored.join(', ')} restored` : 'nothing was restored'; + const undo = safetySnapshot ? `; the files from before the restore are in backup ${safetySnapshot}` : ''; + super( + `Restore incomplete: could not write ${failed.map((f) => f.name).join(', ')} (${failed[0]?.error}); ${done}${undo}` + ); + } +} + /** * Restore refuses what this module never writes. Snapshots are 0700/0600 real files, and a restore * writes the file as 0600: a symlink, or a file with a group/world bit, was planted or loosened @@ -556,7 +576,8 @@ export interface RestoreResult { /** * Put a snapshot's files back into the data dir. Every file is checked against the manifest's digest * BEFORE anything is written, a `pre-restore` snapshot of the live files is taken first, and each - * file is replaced atomically (temp + rename) with mode 0600. + * file is replaced atomically (temp + rename) with mode 0600. A file that cannot be written does not + * stop the others; it throws {@link ConfigBackupPartialRestoreError} at the end, naming both lists. */ export function restoreConfigBackup(opts: { dataDir: string; @@ -603,6 +624,7 @@ export function restoreConfigBackup(opts: { mkdirSync(opts.dataDir, { recursive: true }); const restored: string[] = []; + const failed: Array<{ name: string; error: string }> = []; for (const f of loaded) { const target = join(opts.dataDir, f.name); const tmp = `${target}.restore-${process.pid}`; @@ -611,17 +633,28 @@ export function restoreConfigBackup(opts: { mode: 0o600, }); renameSync(tmp, target); + restored.push(f.name); } catch (err) { - rmSync(tmp, { force: true }); - throw err; + try { + rmSync(tmp, { force: true }); + } catch { + /* the temp is a leftover at worst; the failure below is what matters */ + } + // Keep going: each file is replaced on its own, and the files written before this one are the + // backup's already. Stopping here used to answer a bare 500 that named neither list. + failed.push({ name: f.name, error: err instanceof Error ? err.message : String(err) }); } - restored.push(f.name); } // A restore never deletes: tracked files the snapshot did not hold stay as they are. const untouched = CONFIG_BACKUP_FILES.filter( - (n) => !restored.includes(n) && lstatSync(join(opts.dataDir, n), { throwIfNoEntry: false })?.isFile() + (n) => + !restored.includes(n) && + !failed.some((x) => x.name === n) && + lstatSync(join(opts.dataDir, n), { throwIfNoEntry: false })?.isFile() ); - return { restored, untouched, safetySnapshot: safety.status === 'created' ? safety.id : null }; + const safetySnapshot = safety.status === 'created' ? safety.id : null; + if (failed.length) throw new ConfigBackupPartialRestoreError(restored, failed, untouched, safetySnapshot); + return { restored, untouched, safetySnapshot }; } /** The live files in the data dir, as a side of a diff. */ diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index c40dcd75..2962adfa 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -1516,18 +1516,25 @@ Object.assign(CodemanApp.prototype, { const res = await this._apiPost(`/api/config-backups/${encodeURIComponent(id)}/restore`, {}); let body = null; try { body = res ? await res.json() : null; } catch { /* fall through */ } - if (!res || !res.ok || !body || body.success === false) { + const ok = res && res.ok && body && body.success !== false; + const d = body?.data; + // A restore that failed partway still answers with what it wrote (`restored`, `failed`, `safetySnapshot`): + // those files are the backup's now and need the same refill as a full restore. + if (!ok && !Array.isArray(d?.restored)) { this.showToast(body?.error || 'Restore failed', 'error'); return; } - const d = body.data; const restart = d.restartRequired?.length ? ` Restart Codeman to apply: ${d.restartRequired.join(', ')}.` : ''; // The form still holds the pre-restore values, and Save would PUT them over the restored file. if (d.restored.includes('settings.json')) { await this.loadAppSettingsFromServer(); this.openAppSettings(); } - this.showToast(`Restored ${d.restored.length} file${d.restored.length === 1 ? '' : 's'}.${restart}`, 'success'); + if (ok) { + this.showToast(`Restored ${d.restored.length} file${d.restored.length === 1 ? '' : 's'}.${restart}`, 'success'); + } else { + this.showToast(`${body.error || 'Restore failed partway'}.${restart}`, 'error'); + } this.loadConfigBackups(); }, diff --git a/src/web/routes/config-backup-routes.ts b/src/web/routes/config-backup-routes.ts index e4ecea1f..8ecff444 100644 --- a/src/web/routes/config-backup-routes.ts +++ b/src/web/routes/config-backup-routes.ts @@ -11,7 +11,9 @@ * are admin only. Responses carry file NAMES and sizes, never file content, except the diff, which shows * changed values but never a credential's (`.env`, `users.json`, `push-keys.json`, `webhook.json`, `intents.json` and any key that looks * like a token/secret/password are reported as "changed" only). Restore replaces files the running server - * may have cached: `restartRequired` says which ones only apply after a restart. + * may have cached: `restartRequired` says which ones only apply after a restart. A restore that could not + * write every file answers 500 with the error envelope plus `data` (`restored`, `failed`, `untouched`, + * `safetySnapshot`, `restartRequired`): the files it wrote stay restored and are reloaded like a full one. */ import { readFile } from 'node:fs/promises'; @@ -31,6 +33,7 @@ import { CONFIG_DIFF_CURRENT, ConfigBackupCorruptError, ConfigBackupNotFoundError, + ConfigBackupPartialRestoreError, createConfigBackup, defaultConfigBackupDir, diffConfigBackups, @@ -234,6 +237,28 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { } }); + /** + * What a restore that wrote files must do, whether or not every file made it: reload what the running + * server caches, audit it, and name the files that only apply after a restart. + */ + const afterRestore = (req: FastifyRequest, id: string, restored: string[], failed: string[] = []): string[] => { + // IntentStore caches the file and rewrites it whole on the next prompt, which would undo the restore. + if (restored.includes('intents.json')) intentStore.reload(); + // restoreConfigBackup writes clis.json directly (the one writer besides mutateRegistryFile). + if (restored.includes('clis.json')) reloadCliRegistry(); + // A restore of users.json replaces accounts, password hashes, roles and bypass grants. + if (isMultiUserMode()) { + void appendAdminAudit({ + admin: getAuthUser(req).username, + action: 'config_backup_restore', + target: id, + ip: req.ip, + detail: { restored, ...(failed.length > 0 && { failed }) }, + }); + } + return restored.filter((n) => RESTART_REQUIRED.has(n)); + }; + app.post('/api/config-backups/:id/restore', async (req, reply) => { const denied = deny(req, reply); if (denied) return denied; @@ -241,27 +266,13 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { try { const { dir } = await readEffectiveBackupSettings(); const out = restoreConfigBackup({ dataDir: getDataDir(), backupDir: dir, id, appVersion: APP_VERSION }); - // IntentStore caches the file and rewrites it whole on the next prompt, which would undo the restore. - if (out.restored.includes('intents.json')) intentStore.reload(); - // restoreConfigBackup writes clis.json directly (the one writer besides mutateRegistryFile). - if (out.restored.includes('clis.json')) reloadCliRegistry(); - // A restore of users.json replaces accounts, password hashes, roles and bypass grants. - if (isMultiUserMode()) { - void appendAdminAudit({ - admin: getAuthUser(req).username, - action: 'config_backup_restore', - target: id, - ip: req.ip, - detail: { restored: out.restored }, - }); - } return { success: true, data: { restored: out.restored, untouched: out.untouched, safetySnapshot: out.safetySnapshot, - restartRequired: out.restored.filter((n) => RESTART_REQUIRED.has(n)), + restartRequired: afterRestore(req, id, out.restored), }, }; } catch (err) { @@ -273,6 +284,23 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { reply.code(409); return createErrorResponse(ApiErrorCode.CONFLICT, `${err.message}. Nothing was changed.`); } + if (err instanceof ConfigBackupPartialRestoreError) { + // Some files are the backup's now: the client must learn which (Settings re-reads a restored + // settings.json, or its next Save writes the old values back) and where the undo copy is. + const failedNames = err.failed.map((f) => f.name); + const restartRequired = afterRestore(req, id, err.restored, failedNames); + reply.code(500); + return { + ...createErrorResponse(ApiErrorCode.OPERATION_FAILED, err.message), + data: { + restored: err.restored, + failed: err.failed, + untouched: err.untouched, + safetySnapshot: err.safetySnapshot, + restartRequired, + }, + }; + } reply.code(err instanceof BackupDirUnavailableError ? 503 : 500); return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `Restore failed: ${getErrorMessage(err)}`); } diff --git a/test/config-backup-restore-ui.test.ts b/test/config-backup-restore-ui.test.ts index 840ee567..bb086859 100644 --- a/test/config-backup-restore-ui.test.ts +++ b/test/config-backup-restore-ui.test.ts @@ -9,9 +9,14 @@ import vm from 'node:vm'; const SRC = readFileSync(new URL('../src/web/public/settings-ui.js', import.meta.url), 'utf-8'); -function makeApp(restored: string[], confirmed = true) { +function makeApp( + restored: string[], + confirmed = true, + reply: { ok: boolean; body: unknown } = { ok: true, body: { success: true, data: { restored, restartRequired: [] } } } +) { const calls: string[] = []; const confirms: string[] = []; + const toasts: Array<[string, string]> = []; function CodemanApp(this: unknown) {} const ctx = vm.createContext({ CodemanApp, @@ -29,16 +34,13 @@ function makeApp(restored: string[], confirmed = true) { files: [{ name: 'settings.json' }, { name: 'users.json' }, { name: '.env' }, { name: 'clis.json' }], }, ], - _apiPost: async () => ({ - ok: true, - json: async () => ({ success: true, data: { restored, restartRequired: [] } }), - }), + _apiPost: async () => ({ ok: reply.ok, json: async () => reply.body }), loadAppSettingsFromServer: async () => void calls.push('reload-settings'), openAppSettings: () => void calls.push('refill-form'), loadConfigBackups: () => void calls.push('reload-list'), - showToast: () => {}, + showToast: (m: string, type: string) => void toasts.push([m, type]), }); - return { app, calls, confirms }; + return { app, calls, confirms, toasts }; } describe('restoreConfigBackup (Settings UI)', () => { @@ -54,6 +56,38 @@ describe('restoreConfigBackup (Settings UI)', () => { expect(calls).toEqual(['reload-list']); }); + it('a restore that failed partway still refills the form when settings.json was among the files written', async () => { + const { app, calls, toasts } = makeApp([], true, { + ok: false, + body: { + success: false, + errorCode: 'OPERATION_FAILED', + error: 'Restore incomplete: could not write clis.json (EISDIR); settings.json restored', + data: { + restored: ['settings.json'], + failed: [{ name: 'clis.json', error: 'EISDIR' }], + safetySnapshot: 'cfg-2', + restartRequired: ['settings.json'], + }, + }, + }); + await (app.restoreConfigBackup as (id: string) => Promise)('cfg-1'); + expect(calls).toEqual(['reload-settings', 'refill-form', 'reload-list']); + expect(toasts).toHaveLength(1); + expect(toasts[0][1]).toBe('error'); + expect(toasts[0][0]).toContain('clis.json'); + }); + + it('a refused restore (nothing written) only reports the error', async () => { + const { app, calls, toasts } = makeApp([], true, { + ok: false, + body: { success: false, errorCode: 'CONFLICT', error: 'settings.json does not match its checksum' }, + }); + await (app.restoreConfigBackup as (id: string) => Promise)('cfg-1'); + expect(calls).toEqual([]); + expect(toasts).toEqual([['settings.json does not match its checksum', 'error']]); + }); + it('names the files being replaced and calls out the credential ones', async () => { const { app, confirms } = makeApp(['settings.json'], false); await (app.restoreConfigBackup as (id: string) => Promise)('cfg-1'); diff --git a/test/config-backup.test.ts b/test/config-backup.test.ts index 2cbaa58e..fca4243f 100644 --- a/test/config-backup.test.ts +++ b/test/config-backup.test.ts @@ -22,6 +22,7 @@ import { CONFIG_BACKUP_FILES, ConfigBackupCorruptError, ConfigBackupNotFoundError, + ConfigBackupPartialRestoreError, createConfigBackup, defaultConfigBackupDir, diffConfigBackups, @@ -345,6 +346,30 @@ describe('restoreConfigBackup', () => { } }); + it('a file it cannot write does not stop the others, and the error names both lists', () => { + put('settings.json', '{"good":true}'); + put('clis.json', '{"clis":{}}'); + put('webhook.json', '{"enabled":false}'); + const made = createConfigBackup({ dataDir: data, backupDir: backups, reason: 'auto', now: T0 }) as { id: string }; + put('settings.json', '{"polluted":true}'); + put('webhook.json', '{"enabled":true}'); + rmSync(join(data, 'clis.json')); + mkdirSync(join(data, 'clis.json')); // between the other two in the tracked list + let caught: unknown; + try { + restoreConfigBackup({ dataDir: data, backupDir: backups, id: made.id, now: T0 + 1000 }); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(ConfigBackupPartialRestoreError); + const err = caught as ConfigBackupPartialRestoreError; + expect(err.restored).toEqual(['settings.json', 'webhook.json']); + expect(err.failed.map((f) => f.name)).toEqual(['clis.json']); + expect(err.safetySnapshot).toBeTruthy(); + expect(readFileSync(join(data, 'webhook.json'), 'utf8')).toBe('{"enabled":false}'); + expect(readdirSync(data).filter((n) => n.includes('.restore-'))).toEqual([]); // no temp left behind + }); + it('only ever writes allowlisted names, even if a manifest lists others', () => { put('settings.json', '{"a":1}'); const made = createConfigBackup({ dataDir: data, backupDir: backups, reason: 'auto', now: T0 }) as { id: string }; diff --git a/test/routes/config-backup-routes.test.ts b/test/routes/config-backup-routes.test.ts index 40dce415..3eb25547 100644 --- a/test/routes/config-backup-routes.test.ts +++ b/test/routes/config-backup-routes.test.ts @@ -90,6 +90,36 @@ describe('/api/config-backups', () => { expect(JSON.parse(readFileSync(SETTINGS_PATH, 'utf8')).displayName).toBe('before'); }); + it('a restore that fails partway names what it restored, what failed and the pre-restore copy', async () => { + const { app } = await createRouteTestHarness(registerConfigBackupRoutes); + const clis = join(dirname(SETTINGS_PATH), 'clis.json'); + writeFileSync(clis, '{"schemaVersion":1,"clis":{}}', { mode: 0o600 }); + const id = (await app.inject({ method: 'POST', url: '/api/config-backups' })).json().data.id; + writeFileSync(SETTINGS_PATH, JSON.stringify({ displayName: 'polluted' })); + // A file the server cannot replace: settings.json (first in the list) goes back, clis.json cannot. + rmSync(clis); + mkdirSync(clis); + try { + const res = await app.inject({ method: 'POST', url: `/api/config-backups/${id}/restore` }); + expect(res.statusCode).toBe(500); + const body = res.json(); + expect(body).toMatchObject({ success: false, errorCode: 'OPERATION_FAILED' }); + expect(body.error).toContain('clis.json'); + expect(body.data.restored).toContain('settings.json'); + expect(body.data.failed.map((f: { name: string }) => f.name)).toEqual(['clis.json']); + expect(body.data.restartRequired).toContain('settings.json'); + expect(JSON.parse(readFileSync(SETTINGS_PATH, 'utf8')).displayName).toBe('before'); + // The undo copy exists, holds the state the restore replaced, and is the one named. + const safety = body.data.safetySnapshot; + expect(safety).toBeTruthy(); + expect(body.error).toContain(safety); + const listed = (await app.inject({ method: 'GET', url: '/api/config-backups' })).json().data.backups; + expect(listed.find((b: { id: string }) => b.id === safety)?.reason).toBe('pre-restore'); + } finally { + rmSync(clis, { recursive: true, force: true }); + } + }); + it('a restored intents.json is not reverted by the next captured prompt', async () => { const { app } = await createRouteTestHarness(registerConfigBackupRoutes); const dir = mkdtempSync(join(tmpdir(), 'cfg-backup-intent-'));