From c9f85473bb8f1dd26b82f9c844abc09654674058 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 12 Jul 2026 02:10:04 -0700 Subject: [PATCH] fix(sync): harden sync CLI, IPC, and UI paths from CodeRabbit review - reject option-like tokens as flag values (--snapshot --force wrote a file named --force); --flag=-value still works - PowerShell remote quoting uses single-quoted literals so $() in a quoted path cannot expand - sync-hosts.json written via temp file + rename; a crash mid-write truncated it and the reader's corrupt-fallback dropped every host - cancelled sync child escalates SIGTERM -> SIGKILL after 5s grace - NDJSON progress events validated field-by-field before casting - snapshot filenames include milliseconds to avoid same-second overwrite - syncAutoScheduler.stop() wired into will-quit cleanup - sync --ui exclusivity also rejects --make-temp/--remove-temp/--json - document --sync-window in app help; group --make-temp/--remove-temp under modes in sync usage --- launcher/config/cli-parser-builder.ts | 13 ++- src/cli/help.ts | 1 + src/core/services/stats-sync/cli-args.test.ts | 17 ++++ src/core/services/stats-sync/cli-args.ts | 9 +- src/core/services/stats-sync/ssh.test.ts | 14 +++ src/core/services/stats-sync/ssh.ts | 18 ++-- src/main.ts | 1 + .../runtime/app-lifecycle-actions.test.ts | 4 +- src/main/runtime/app-lifecycle-actions.ts | 2 + .../app-lifecycle-main-cleanup.test.ts | 3 + .../runtime/app-lifecycle-main-cleanup.ts | 2 + .../startup-lifecycle-composer.test.ts | 1 + src/main/runtime/sync-launcher-client.ts | 24 ++++++ src/main/runtime/sync-ui-runtime.test.ts | 2 +- src/main/runtime/sync-ui-runtime.ts | 6 +- src/shared/sync/sync-events.test.ts | 39 ++++++++- src/shared/sync/sync-events.ts | 86 ++++++++++++++++--- src/shared/sync/sync-hosts-store.test.ts | 22 +++++ src/shared/sync/sync-hosts-store.ts | 20 ++++- 19 files changed, 257 insertions(+), 27 deletions(-) diff --git a/launcher/config/cli-parser-builder.ts b/launcher/config/cli-parser-builder.ts index 2d1b1199..166afa22 100644 --- a/launcher/config/cli-parser-builder.ts +++ b/launcher/config/cli-parser-builder.ts @@ -351,7 +351,18 @@ export function parseCliPrograms( const makeTemp = options.makeTemp === true; const removeTemp = typeof options.removeTemp === 'string' ? options.removeTemp.trim() : ''; if (options.ui === true) { - if (host || snapshot || merge || push || pull || check || options.force === true) { + if ( + host || + snapshot || + merge || + push || + pull || + check || + makeTemp || + removeTemp || + options.json === true || + options.force === true + ) { throw new Error('Sync --ui cannot be combined with other sync options.'); } syncUiTriggered = true; diff --git a/src/cli/help.ts b/src/cli/help.ts index d46a4a59..158a58c0 100644 --- a/src/cli/help.ts +++ b/src/cli/help.ts @@ -80,6 +80,7 @@ ${B}Jellyfin${R} --jellyfin-subtitle-stream-index ${D}N${R} Subtitle stream override ${B}Stats sync${R} + --sync-window Open the stats sync window --sync-cli sync ${D}[host] [opts]${R} Headless stats sync ${D}(same commands as "subminer sync";${R} ${D}run --sync-cli --help for details)${R} diff --git a/src/core/services/stats-sync/cli-args.test.ts b/src/core/services/stats-sync/cli-args.test.ts index 5faf212e..1ad5c5fb 100644 --- a/src/core/services/stats-sync/cli-args.test.ts +++ b/src/core/services/stats-sync/cli-args.test.ts @@ -62,3 +62,20 @@ test('parseSyncCliTokens mirrors launcher sync validation', () => { assert.equal(parseSyncCliTokens(['sync', 'h', 'extra']).kind, 'error'); assert.equal(parseSyncCliTokens(['sync', '--snapshot']).kind, 'error'); }); + +test('parseSyncCliTokens rejects an option-like token where a value is required', () => { + // Without this guard `--snapshot --force` writes a snapshot to a file literally + // named "--force" and silently drops the flag. + assert.deepEqual(parseSyncCliTokens(['sync', '--snapshot', '--force']), { + kind: 'error', + message: 'Missing value for --snapshot.', + }); + assert.deepEqual(parseSyncCliTokens(['sync', '--remove-temp', '--force']), { + kind: 'error', + message: 'Missing value for --remove-temp.', + }); + // The `--flag=` form still accepts values that begin with "-". + const parsed = parseSyncCliTokens(['sync', '--snapshot=-weird-name.sqlite']); + assert.equal(parsed.kind, 'run'); + assert.equal(parsed.kind === 'run' && parsed.args.syncSnapshotPath, '-weird-name.sqlite'); +}); diff --git a/src/core/services/stats-sync/cli-args.ts b/src/core/services/stats-sync/cli-args.ts index eb5554b4..0bf67600 100644 --- a/src/core/services/stats-sync/cli-args.ts +++ b/src/core/services/stats-sync/cli-args.ts @@ -64,7 +64,10 @@ export function parseSyncCliTokens(tokens: readonly string[]): ParsedSyncCli { continue; } const value = rest[i + 1]; - if (value === undefined) { + // An option-like token is never a value: `--snapshot --force` must fail + // loudly instead of writing a snapshot to a file named "--force". Paths + // that really do start with "-" can still be passed as `--snapshot=-x`. + if (value === undefined || value.startsWith('-')) { return { kind: 'error', message: `Missing value for ${token}.` }; } assignValue(value); @@ -144,6 +147,8 @@ export function syncCliUsage(): string { ' Sync stats with an SSH destination (user@host or ssh alias)', ' --snapshot Write a consistent snapshot of the local stats database', ' --merge Merge a snapshot database file into the local stats database', + ' --make-temp Create a sync temp directory and print its path (used over SSH)', + ' --remove-temp Remove a sync temp directory created by --make-temp', '', 'Options:', ' --push Only merge local stats into the SSH host', @@ -153,8 +158,6 @@ export function syncCliUsage(): string { ' --remote-cmd SubMiner app or launcher command to run on the remote host', ' -f, --force Skip the running-app safety check', ' --json Emit machine-readable NDJSON progress output', - ' --make-temp Create a sync temp directory and print its path (used over SSH)', - ' --remove-temp Remove a sync temp directory created by --make-temp', ' --log-level Log level', ' --help Show this help', ' --version Show the SubMiner version', diff --git a/src/core/services/stats-sync/ssh.test.ts b/src/core/services/stats-sync/ssh.test.ts index e78e59e4..17d0c4d1 100644 --- a/src/core/services/stats-sync/ssh.test.ts +++ b/src/core/services/stats-sync/ssh.test.ts @@ -160,3 +160,17 @@ test('quoteForRemoteShell quotes per flavor and rejects unsafe Windows values', assert.throws(() => quoteForRemoteShell('windows-cmd', 'a"b'), /Refusing to quote/); assert.throws(() => quoteForRemoteShell('windows-powershell', 'a\nb'), /Refusing to quote/); }); + +test('quoteForRemoteShell does not let PowerShell expand a quoted value', () => { + // PowerShell expands $(...) and $var inside double quotes, so a single-quoted + // literal is the only safe form; '' is the escape for an embedded quote. + assert.equal( + quoteForRemoteShell('windows-powershell', 'C:/tmp/$(calc.exe)'), + `'C:/tmp/$(calc.exe)'`, + ); + assert.equal(quoteForRemoteShell('windows-powershell', "C:/tmp/it's"), `'C:/tmp/it''s'`); + assert.equal( + quoteForRemoteShell('windows-powershell', 'C:/Users/First Last/Temp/subminer-sync-ab'), + `'C:/Users/First Last/Temp/subminer-sync-ab'`, + ); +}); diff --git a/src/core/services/stats-sync/ssh.ts b/src/core/services/stats-sync/ssh.ts index 19567da0..c364b8d0 100644 --- a/src/core/services/stats-sync/ssh.ts +++ b/src/core/services/stats-sync/ssh.ts @@ -100,15 +100,21 @@ export function detectRemoteShellFlavor( } /** - * Quote one argument for the detected remote shell. Windows shells have no - * safe single-quote escaping (cmd treats ' literally; PowerShell and cmd - * disagree on embedded double quotes), so quote-containing values are - * rejected instead of escaped — sync only ever quotes paths it composed. + * Quote one argument for the detected remote shell. PowerShell expands $(...) + * and $var inside double quotes, so it gets a single-quoted literal ('' escapes + * a quote). cmd.exe has no single-quote form and treats ' literally, so it keeps + * double quotes and rejects values carrying a double quote of their own. */ export function quoteForRemoteShell(flavor: RemoteShellFlavor, value: string): string { if (flavor === 'posix') return shellQuote(value); - if (value.includes('"') || /[\r\n]/.test(value)) { - throw new Error(`Refusing to quote a value with quotes or newlines for a Windows shell: ${value}`); + if (/[\r\n]/.test(value)) { + throw new Error(`Refusing to quote a value with newlines for a Windows shell: ${value}`); + } + if (flavor === 'windows-powershell') { + return `'${value.replaceAll("'", "''")}'`; + } + if (value.includes('"')) { + throw new Error(`Refusing to quote a value with quotes for a Windows shell: ${value}`); } return `"${value}"`; } diff --git a/src/main.ts b/src/main.ts index 9512fe9f..07451132 100644 --- a/src/main.ts +++ b/src/main.ts @@ -3951,6 +3951,7 @@ const { annotationSubtitleWsService.stop(); }, stopTexthookerService: () => texthookerService.stop(), + stopSyncAutoScheduler: () => syncAutoScheduler.stop(), clearWindowsVisibleOverlayForegroundPollLoop: () => clearWindowsVisibleOverlayForegroundPollLoop(), clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => { diff --git a/src/main/runtime/app-lifecycle-actions.test.ts b/src/main/runtime/app-lifecycle-actions.test.ts index fc2817e5..96fcda8f 100644 --- a/src/main/runtime/app-lifecycle-actions.test.ts +++ b/src/main/runtime/app-lifecycle-actions.test.ts @@ -16,6 +16,7 @@ test('on will quit cleanup handler runs all cleanup steps', () => { unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), + stopSyncAutoScheduler: () => calls.push('stop-sync-auto-scheduler'), clearWindowsVisibleOverlayForegroundPollLoop: () => calls.push('clear-windows-visible-overlay-poll'), clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => @@ -47,7 +48,7 @@ test('on will quit cleanup handler runs all cleanup steps', () => { }); cleanup(); - assert.equal(calls.length, 33); + assert.equal(calls.length, 34); assert.equal(calls[0], 'destroy-tray'); assert.equal(calls[calls.length - 1], 'stop-discord-presence'); assert.ok(calls.includes('cleanup-jellyfin-subtitles')); @@ -68,6 +69,7 @@ test('on will quit cleanup handler cleans jellyfin subtitle cache when stopping unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + stopSyncAutoScheduler: () => {}, clearWindowsVisibleOverlayForegroundPollLoop: () => {}, clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => {}, destroyMainOverlayWindow: () => {}, diff --git a/src/main/runtime/app-lifecycle-actions.ts b/src/main/runtime/app-lifecycle-actions.ts index c3891661..3159ed6b 100644 --- a/src/main/runtime/app-lifecycle-actions.ts +++ b/src/main/runtime/app-lifecycle-actions.ts @@ -6,6 +6,7 @@ export function createOnWillQuitCleanupHandler(deps: { unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; + stopSyncAutoScheduler: () => void; clearWindowsVisibleOverlayForegroundPollLoop: () => void; clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => void; destroyMainOverlayWindow: () => void; @@ -41,6 +42,7 @@ export function createOnWillQuitCleanupHandler(deps: { deps.unregisterAllGlobalShortcuts(); deps.stopSubtitleWebsocket(); deps.stopTexthookerService(); + deps.stopSyncAutoScheduler(); deps.clearWindowsVisibleOverlayForegroundPollLoop(); deps.clearLinuxMpvFullscreenOverlayRefreshTimeouts(); deps.destroyMainOverlayWindow(); diff --git a/src/main/runtime/app-lifecycle-main-cleanup.test.ts b/src/main/runtime/app-lifecycle-main-cleanup.test.ts index 3b0cd6ba..51212ef1 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.test.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.test.ts @@ -19,6 +19,7 @@ test('cleanup deps builder returns handlers that guard optional runtime objects' unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), + stopSyncAutoScheduler: () => calls.push('stop-sync-auto-scheduler'), clearWindowsVisibleOverlayForegroundPollLoop: () => calls.push('clear-windows-visible-overlay-foreground-poll-loop'), clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => @@ -113,6 +114,7 @@ test('cleanup deps builder skips destroyed yomitan window', () => { unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + stopSyncAutoScheduler: () => {}, clearWindowsVisibleOverlayForegroundPollLoop: () => {}, clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => {}, getMainOverlayWindow: () => ({ @@ -173,6 +175,7 @@ test('cleanup deps builder skips global shortcut cleanup before app ready', () = }, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + stopSyncAutoScheduler: () => {}, clearWindowsVisibleOverlayForegroundPollLoop: () => {}, clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => {}, getMainOverlayWindow: () => null, diff --git a/src/main/runtime/app-lifecycle-main-cleanup.ts b/src/main/runtime/app-lifecycle-main-cleanup.ts index c4593714..d57cbb1f 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.ts @@ -26,6 +26,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; + stopSyncAutoScheduler: () => void; clearWindowsVisibleOverlayForegroundPollLoop: () => void; clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => void; getMainOverlayWindow: () => DestroyableWindow | null; @@ -73,6 +74,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { }, stopSubtitleWebsocket: () => deps.stopSubtitleWebsocket(), stopTexthookerService: () => deps.stopTexthookerService(), + stopSyncAutoScheduler: () => deps.stopSyncAutoScheduler(), clearWindowsVisibleOverlayForegroundPollLoop: () => deps.clearWindowsVisibleOverlayForegroundPollLoop(), clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => diff --git a/src/main/runtime/composers/startup-lifecycle-composer.test.ts b/src/main/runtime/composers/startup-lifecycle-composer.test.ts index b2dca84c..b4db3e1c 100644 --- a/src/main/runtime/composers/startup-lifecycle-composer.test.ts +++ b/src/main/runtime/composers/startup-lifecycle-composer.test.ts @@ -22,6 +22,7 @@ test('composeStartupLifecycleHandlers returns callable startup lifecycle handler unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + stopSyncAutoScheduler: () => {}, clearWindowsVisibleOverlayForegroundPollLoop: () => {}, clearLinuxMpvFullscreenOverlayRefreshTimeouts: () => {}, getMainOverlayWindow: () => null, diff --git a/src/main/runtime/sync-launcher-client.ts b/src/main/runtime/sync-launcher-client.ts index 99452b74..2deb1a28 100644 --- a/src/main/runtime/sync-launcher-client.ts +++ b/src/main/runtime/sync-launcher-client.ts @@ -2,6 +2,9 @@ import { spawn as nodeSpawn } from 'node:child_process'; import { parseSyncProgressLine, type SyncProgressEvent } from '../../shared/sync/sync-events'; import { SYNC_CLI_FLAG } from '../../core/services/stats-sync/cli-args'; +/** How long a cancelled sync child gets to exit on SIGTERM before SIGKILL. */ +const CANCEL_GRACE_MS = 5000; + export interface SyncLauncherChildLike { stdout: { on(event: 'data', listener: (chunk: Buffer | string) => void): unknown } | null; stderr: { on(event: 'data', listener: (chunk: Buffer | string) => void): unknown } | null; @@ -114,14 +117,35 @@ export function runSyncLauncher(options: { }); }); + // A sync child blocked on an ssh password prompt ignores SIGTERM, so escalate + // to SIGKILL if it is still alive after a grace period. The timer is cleared + // on close so a cancelled run cannot hold the process open. + let killTimer: ReturnType | null = null; + const clearKillTimer = (): void => { + if (killTimer === null) return; + clearTimeout(killTimer); + killTimer = null; + }; + child.on('close', clearKillTimer); + return { cancel: () => { + if (cancelled) return; cancelled = true; try { child.kill('SIGTERM'); } catch { // process may already be gone } + killTimer = setTimeout(() => { + killTimer = null; + try { + child.kill('SIGKILL'); + } catch { + // process may already be gone + } + }, CANCEL_GRACE_MS); + killTimer.unref?.(); }, done, }; diff --git a/src/main/runtime/sync-ui-runtime.test.ts b/src/main/runtime/sync-ui-runtime.test.ts index f5bf2568..dcad24db 100644 --- a/src/main/runtime/sync-ui-runtime.test.ts +++ b/src/main/runtime/sync-ui-runtime.test.ts @@ -21,7 +21,7 @@ function makeTestRig(root: string) { const windowStub = { isDestroyed: () => false, webContents: { - send: (channel: string, payload: unknown) => sent.push({ channel, payload }), + send: (channel: string, payload?: unknown) => sent.push({ channel, payload }), }, }; diff --git a/src/main/runtime/sync-ui-runtime.ts b/src/main/runtime/sync-ui-runtime.ts index 37970071..b266f454 100644 --- a/src/main/runtime/sync-ui-runtime.ts +++ b/src/main/runtime/sync-ui-runtime.ts @@ -58,9 +58,11 @@ interface ActiveRun { resultSeen: boolean; } -function formatSnapshotName(nowMs: number): string { +// Milliseconds are part of the stamp so two snapshots taken in the same second +// do not land on the same path and silently overwrite each other. +export function formatSnapshotName(nowMs: number): string { const iso = new Date(nowMs).toISOString(); - const stamp = iso.slice(0, 19).replace(/[-:]/g, '').replace('T', '-'); + const stamp = iso.slice(0, 23).replace(/[-:.]/g, '').replace('T', '-'); return `immersion-${stamp}.sqlite`; } diff --git a/src/shared/sync/sync-events.test.ts b/src/shared/sync/sync-events.test.ts index f865a197..3de91cc1 100644 --- a/src/shared/sync/sync-events.test.ts +++ b/src/shared/sync/sync-events.test.ts @@ -10,7 +10,22 @@ test('parseSyncProgressLine parses known event types', () => { const summaryLine = JSON.stringify({ type: 'merge-summary', target: 'local', - summary: { sessionsMerged: 2 }, + summary: { + sessionsMerged: 2, + sessionsAlreadyPresent: 0, + activeSessionsSkipped: 0, + animeAdded: 0, + videosAdded: 0, + wordsAdded: 0, + kanjiAdded: 0, + subtitleLinesAdded: 0, + telemetryRowsAdded: 0, + eventsAdded: 0, + excludedWordsAdded: 0, + dailyRollupsCopied: 0, + monthlyRollupsCopied: 0, + rollupGroupsRecomputed: 0, + }, }); const parsed = parseSyncProgressLine(summaryLine); assert.equal(parsed?.type, 'merge-summary'); @@ -28,3 +43,25 @@ test('parseSyncProgressLine rejects non-events and garbage', () => { assert.equal(parseSyncProgressLine(''), null); assert.equal(parseSyncProgressLine('42'), null); }); + +test('parseSyncProgressLine rejects events missing required fields', () => { + // A half-formed event used to be cast straight through; the UI reads + // `.ok` / `.summary` directly and would misreport a failed sync as done. + assert.equal(parseSyncProgressLine('{"type":"result"}'), null); + assert.equal(parseSyncProgressLine('{"type":"result","ok":"yes","error":null}'), null); + assert.equal(parseSyncProgressLine('{"type":"merge-summary","target":"local"}'), null); + assert.equal( + parseSyncProgressLine('{"type":"merge-summary","target":"nowhere","summary":{}}'), + null, + ); + assert.equal(parseSyncProgressLine('{"type":"stage","stage":"bogus","message":"x"}'), null); + assert.equal(parseSyncProgressLine('{"type":"snapshot-created"}'), null); + assert.equal(parseSyncProgressLine('{"type":"check-result","host":"h","sshOk":true}'), null); + + // Well-formed events still parse. + assert.deepEqual(parseSyncProgressLine('{"type":"result","ok":true,"error":null}'), { + type: 'result', + ok: true, + error: null, + }); +}); diff --git a/src/shared/sync/sync-events.ts b/src/shared/sync/sync-events.ts index 30806807..72f8823b 100644 --- a/src/shared/sync/sync-events.ts +++ b/src/shared/sync/sync-events.ts @@ -43,15 +43,51 @@ export type SyncProgressEvent = } | { type: 'result'; ok: boolean; error: string | null }; -const EVENT_TYPES = new Set([ - 'stage', - 'merge-summary', - 'remote-output', - 'snapshot-created', - 'check-result', - 'result', -]); +const SUMMARY_KEYS: readonly (keyof SyncMergeSummary)[] = [ + 'sessionsMerged', + 'sessionsAlreadyPresent', + 'activeSessionsSkipped', + 'animeAdded', + 'videosAdded', + 'wordsAdded', + 'kanjiAdded', + 'subtitleLinesAdded', + 'telemetryRowsAdded', + 'eventsAdded', + 'excludedWordsAdded', + 'dailyRollupsCopied', + 'monthlyRollupsCopied', + 'rollupGroupsRecomputed', +]; +const STAGES: readonly SyncStage[] = [ + 'snapshot-local', + 'snapshot-remote', + 'download', + 'upload', + 'merge-local', + 'merge-remote', +]; + +function isString(value: unknown): value is string { + return typeof value === 'string'; +} + +function isNullableString(value: unknown): value is string | null { + return value === null || typeof value === 'string'; +} + +function isMergeSummary(value: unknown): value is SyncMergeSummary { + if (!value || typeof value !== 'object' || Array.isArray(value)) return false; + const record = value as Record; + return SUMMARY_KEYS.every((key) => typeof record[key] === 'number'); +} + +/** + * The events cross a process boundary (the sync child's stdout), so each variant + * is validated field-by-field: a half-formed `result` or `merge-summary` would + * otherwise be cast straight into the UI, which reads `.ok`/`.summary` directly. + */ export function parseSyncProgressLine(line: string): SyncProgressEvent | null { const trimmed = line.trim(); if (!trimmed.startsWith('{')) return null; @@ -62,7 +98,35 @@ export function parseSyncProgressLine(line: string): SyncProgressEvent | null { return null; } if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return null; - const type = (parsed as { type?: unknown }).type; - if (typeof type !== 'string' || !EVENT_TYPES.has(type)) return null; - return parsed as SyncProgressEvent; + const event = parsed as Record; + + switch (event.type) { + case 'stage': + return STAGES.includes(event.stage as SyncStage) && isString(event.message) + ? (parsed as SyncProgressEvent) + : null; + case 'merge-summary': + return (event.target === 'local' || event.target === 'remote') && isMergeSummary(event.summary) + ? (parsed as SyncProgressEvent) + : null; + case 'remote-output': + return isString(event.text) ? (parsed as SyncProgressEvent) : null; + case 'snapshot-created': + return isString(event.path) ? (parsed as SyncProgressEvent) : null; + case 'check-result': + return isString(event.host) && + typeof event.sshOk === 'boolean' && + typeof event.ok === 'boolean' && + isNullableString(event.remoteCommand) && + isNullableString(event.remoteVersion) && + isNullableString(event.error) + ? (parsed as SyncProgressEvent) + : null; + case 'result': + return typeof event.ok === 'boolean' && isNullableString(event.error) + ? (parsed as SyncProgressEvent) + : null; + default: + return null; + } } diff --git a/src/shared/sync/sync-hosts-store.test.ts b/src/shared/sync/sync-hosts-store.test.ts index 6460f321..7aee0d34 100644 --- a/src/shared/sync/sync-hosts-store.test.ts +++ b/src/shared/sync/sync-hosts-store.test.ts @@ -180,3 +180,25 @@ test('writeSyncHostsState creates parent directories', () => { assert.deepEqual(readSyncHostsState(filePath), createDefaultSyncHostsState()); }); }); + +test('writeSyncHostsState leaves the previous file intact when the write fails', () => { + withTempDir((root) => { + const filePath = getSyncHostsPath(root); + let state = createDefaultSyncHostsState(); + state = upsertSyncHost(state, { host: 'user@laptop', label: 'Laptop' }, 42); + writeSyncHostsState(filePath, state); + + // A crash mid-write must not truncate the live file: readSyncHostsState + // falls back to defaults on a corrupt file, which would drop every host. + let next = upsertSyncHost(state, { host: 'user@desktop' }, 43); + assert.throws(() => + writeSyncHostsState(filePath, next, { + renameSync: () => { + throw new Error('disk full'); + }, + }), + ); + assert.deepEqual(readSyncHostsState(filePath), state); + assert.equal(fs.existsSync(`${filePath}.tmp`), false); + }); +}); diff --git a/src/shared/sync/sync-hosts-store.ts b/src/shared/sync/sync-hosts-store.ts index f1d02911..eb69d31f 100644 --- a/src/shared/sync/sync-hosts-store.ts +++ b/src/shared/sync/sync-hosts-store.ts @@ -180,16 +180,34 @@ export function readSyncHostsState( } } +// Written via a sibling temp file + rename so an interrupted write cannot leave +// a truncated sync-hosts.json behind (readSyncHostsState would silently fall +// back to defaults, dropping every configured host). export function writeSyncHostsState( filePath: string, state: SyncHostsState, deps?: { mkdirSync?: (candidate: string, options: { recursive: true }) => void; writeFileSync?: (candidate: string, content: string, encoding: BufferEncoding) => void; + renameSync?: (from: string, to: string) => void; + rmSync?: (target: string, options: { force: true }) => void; }, ): void { const mkdirSync = deps?.mkdirSync ?? fs.mkdirSync; const writeFileSync = deps?.writeFileSync ?? fs.writeFileSync; + const renameSync = deps?.renameSync ?? fs.renameSync; + const rmSync = deps?.rmSync ?? fs.rmSync; mkdirSync(path.dirname(filePath), { recursive: true }); - writeFileSync(filePath, `${JSON.stringify(state, null, 2)}\n`, 'utf8'); + const tempPath = `${filePath}.tmp`; + writeFileSync(tempPath, `${JSON.stringify(state, null, 2)}\n`, 'utf8'); + try { + renameSync(tempPath, filePath); + } catch (error) { + try { + rmSync(tempPath, { force: true }); + } catch { + // best effort: the temp file is disposable + } + throw error; + } }