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-'));