From 692bffacb4055d201c8c34fa4620b6c3cc62063c Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 6 Sep 2026 08:33:09 +0000 Subject: [PATCH 1/2] fix: a manual sync transfers what you selected, and asks first Selecting two folders on a server and pressing Sync to Local copied the whole directory. Two movies became forty thousand files. The panes have always let you select entries. `page.tsx` built its request from `source.path` and never read `state.selected`, so the selection reached the counter in the pane footer and nothing else. Every transfer was the whole directory, every time. A second bug in the same click, found on the way. The rail sets the direction and starts the transfer in one handler: onDirection('rtl') onRun() `setDirection` does not change the value the current render closed over, so `run` built its request from the PREVIOUS direction. Pressing the arrow that was not armed transferred the opposite way: "Sync to Local" copied local over the server. With Mirror armed it would have deleted the wrong side. The direction is now passed to the run rather than read back out of state. Selections now become an rsync `--files-from` list. The trap there, confirmed against rsync 3.4.1 rather than assumed: rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/ rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/ >f+++++++++ movieA/a.mkv `--files-from` switches recursion off and `--archive` does not switch it back on, so a selected folder would have copied as an empty folder and reported success. `buildRsyncArgs` now forces `--recursive` whenever a file list is in play. The list is NUL-separated (`--from0`), because a newline is legal in a filename and a line-separated list would split one name into two paths that do not exist. Mirror plus a selection deletes only inside what was selected. Verified, not assumed: the other reading of `--delete` here would empty the destination. Everything previews and is approved now, mirror or not. Both the desktop and the CLI skipped the dry run for a plain sync, on the reasoning that it "buys no safety, because nothing is deleted either way". Deleting is not the only way to regret a transfer, and this bug is the proof: by the time anything was on screen it was already copying. The dialog says which it is, Sync or Mirror, and carries the scope ("2 selected items" / "Whole folder") next to the counts, because a count alone never says how many of what you asked for. Confirming starts the request that was measured, not one rebuilt from whatever the panes say by then. All three interfaces: - Desktop: the pane selection is sent, validated as entry names rather than paths. That list becomes a `--files-from` rooted at the source directory, so a renderer able to put `../` in it would be choosing which files leave the machine. - CLI: `--only NAME`, repeatable, and a plain sync now previews and asks. Only where somebody is there to answer: a pipe, `--non-interactive` or `--yes` proceeds as before, so scheduled profile runs keep working. `--no-preview` restores the old skip-the-scan behaviour. - TUI: it had a cursor and no way to select at all, and `s` started an immediate transfer of everything on screen. Space marks entries, `s` scans and asks before it moves anything. Two smaller things this turned up: - `--only` did not parse. It was missing from VALUE_FLAGS, so it consumed no value, the names fell through to the positionals, and the flag silently did nothing. Caught by running the built binary, not by reading it. - The preview dialog's "Trust this pair from now on" checkbox is gone. It set renderer state that nothing read, and could not have worked if wired: saveProfile hard-codes `trustDeletes: false` on purpose, because unattended mirroring is the one way a delete list runs with nobody looking at it. Verified against the real rsync and the real dev server. `--only movieA --only movieB` over ssh: 4 files, not 7. The desktop driven in headless Chromium under the app's real CSP: the preview request carries the ssh source, both names, and does not start until approved. The TUI driven in a pty: marks render, the confirm appears, `n` transfers nothing, `y` copies the two marked folders with their contents and leaves the rest. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52 --- apps/cli/src/commands/transfer.ts | 77 +++++++- apps/cli/src/parse-argv.test.ts | 25 +++ apps/cli/src/parse-argv.ts | 2 + apps/cli/src/tui/app.ts | 171 ++++++++++++++++-- .../electron/main/services/selection.test.ts | 160 ++++++++++++++++ .../electron/main/services/transfers.ts | 99 ++++++++-- apps/desktop/electron/shared/contract.ts | 45 +++-- apps/desktop/src/app/page.tsx | 109 ++++++++--- .../desktop/src/components/transfer-panel.tsx | 72 ++++++-- apps/desktop/src/components/transfer-rail.tsx | 13 +- packages/rsync-core/src/args.ts | 23 ++- packages/rsync-core/src/files-from.test.ts | 136 ++++++++++++++ packages/schemas/src/rsync-options.ts | 9 + 13 files changed, 839 insertions(+), 102 deletions(-) create mode 100644 apps/desktop/electron/main/services/selection.test.ts create mode 100644 packages/rsync-core/src/files-from.test.ts diff --git a/apps/cli/src/commands/transfer.ts b/apps/cli/src/commands/transfer.ts index 0e7aabb..8dddb89 100644 --- a/apps/cli/src/commands/transfer.ts +++ b/apps/cli/src/commands/transfer.ts @@ -1,4 +1,7 @@ import { randomUUID } from 'node:crypto' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { createInterface } from 'node:readline/promises' import type { DiskPushStore } from '@diskpush/database' import { capabilityCacheKey } from '../resolve.js' @@ -15,7 +18,7 @@ import { summarizeChanges, topologyOf } from '@diskpush/schemas' import { EXIT } from '../exit-codes.js' import { estimateRemaining, formatBytes, formatDuration, formatRate, pluralize, table } from '../format.js' import { failure, type Output } from '../output.js' -import { flagValue, hasFlag, type ParsedArgv } from '../parse-argv.js' +import { flagValue, flagValues, hasFlag, type ParsedArgv } from '../parse-argv.js' import { detectLocalCapabilities, optionsFromFlags, resolveEndpoint } from '../resolve.js' import type { RsyncCapabilities } from '@diskpush/rsync-core' @@ -61,6 +64,39 @@ export async function runTransfer( if (alias.deleteMode !== 'off') options.deleteMode = alias.deleteMode + /* + * `--only NAME` narrows the transfer to entries inside the source directory, + * the CLI's version of ticking rows in the desktop's pane. It becomes an + * rsync `--files-from` list, which is why the names are checked here: a + * separator or a `..` would silently widen the transfer to somewhere the + * user did not name. + */ + const only = flagValues(parsed, '--only') + let selectionCleanup: (() => void) | null = null + if (only.length > 0) { + const bad = only.find( + (name) => name.includes('/') || name.includes('\\') || name.includes('\0') || name === '.' || name === '..', + ) + if (bad !== undefined) { + return failure( + output, + `--only takes a name inside the source directory, not a path: ${JSON.stringify(bad)}.`, + EXIT.usage, + ) + } + const directory = mkdtempSync(join(tmpdir(), 'diskpush-only-')) + const listPath = join(directory, 'files-from') + // NUL-separated: a newline is legal in a filename, and a line-separated + // list would split one such name into two paths that do not exist. + writeFileSync(listPath, `${only.join('\0')}\0`) + options = { ...options, filesFrom: listPath, from0: true } + selectionCleanup = () => rmSync(directory, { recursive: true, force: true }) + // Registered rather than called at each of this function's many returns: + // rsync has read the list long before the process ends, and one handler + // cannot be forgotten the way eight call sites can. + process.once('exit', selectionCleanup) + } + const source = await resolveEndpoint(store, sourceInput) const destination = await resolveEndpoint(store, destinationInput) const topology = topologyOf(source.endpoint, destination.endpoint) @@ -100,9 +136,20 @@ export async function runTransfer( } // --- preview ------------------------------------------------------------ - // Mirror always previews before it can run. A plain sync previews only when - // asked, because its dry run costs a full scan for no safety benefit. - const wantsPreview = options.deleteMode !== 'off' || options.dryRun + /* + * Everything previews, not only a mirror. + * + * This used to skip the dry run for a plain sync, on the reasoning that it + * "costs a full scan for no safety benefit". Deleting is not the only way to + * regret a transfer: two named folders can turn into forty thousand files, + * and by the time anything is on screen it is already copying. A scan is + * cheap next to that. + * + * `--yes` still skips the question, which is what a script passes; the scan + * itself is skipped only by `--no-preview`, for someone who genuinely wants + * the old behaviour. + */ + const wantsPreview = !hasFlag(parsed, '--no-preview') let preview: Awaited> | null = null if (wantsPreview) { @@ -147,6 +194,28 @@ export async function runTransfer( } } + /* + * A plain sync is approved too, but only where there is somebody to ask. + * + * A script that pipes us, passes --non-interactive, or passes --yes has + * already decided, and turning those into a refusal would break every + * scheduled profile run. The prompt is for the interactive case, which is + * the one where a surprise is possible. + */ + if (options.deleteMode === 'off' && preview) { + const moving = preview.changes.filter((c) => c.action === 'add' || c.action === 'update').length + const asking = !hasFlag(parsed, '--yes') && !hasFlag(parsed, '--non-interactive') && process.stdin.isTTY + + if (moving === 0) { + return finish(output, EXIT.ok, 'Nothing to transfer. The destination already matches.', { + changes: summarizeChanges(preview.changes), + }) + } + if (asking && !(await confirm(`${alias.label} ${pluralize(moving, 'file')}?`))) { + return failure(output, `${alias.label} cancelled. Nothing was transferred.`, EXIT.refused) + } + } + // --- run ----------------------------------------------------------------- let plan: ExecutionPlan try { diff --git a/apps/cli/src/parse-argv.test.ts b/apps/cli/src/parse-argv.test.ts index b9e992d..ef858c3 100644 --- a/apps/cli/src/parse-argv.test.ts +++ b/apps/cli/src/parse-argv.test.ts @@ -116,3 +116,28 @@ describe('commands', () => { expect(parsed.positionals).toEqual(['sync', './b/']) }) }) + +describe('--only', () => { + /* + * The bug: `--only` was not in VALUE_FLAGS, so it consumed nothing. Each + * name fell through to the positionals, `flagValues` came back empty, and + * `diskpush sync SRC DST --only movieA --only movieB` synced the whole + * directory anyway. Which is the exact bug --only exists to fix. + */ + it('takes a value, and repeats', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'movieA', '--only', 'movieB']) + expect(flagValues(parsed, '--only')).toEqual(['movieA', 'movieB']) + // The names are flag values, not endpoints. + expect(parsed.positionals).toEqual(['dev:/srv/', './out/']) + }) + + it('keeps a name with spaces in one piece', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only', 'The Movie (2019)']) + expect(flagValues(parsed, '--only')).toEqual(['The Movie (2019)']) + }) + + it('accepts the --only=NAME form too', () => { + const parsed = parseArgv(['sync', 'dev:/srv/', './out/', '--only=movieA']) + expect(flagValues(parsed, '--only')).toEqual(['movieA']) + }) +}) diff --git a/apps/cli/src/parse-argv.ts b/apps/cli/src/parse-argv.ts index 0e1bd29..f31497a 100644 --- a/apps/cli/src/parse-argv.ts +++ b/apps/cli/src/parse-argv.ts @@ -33,6 +33,8 @@ export const VALUE_FLAGS = new Set([ '--exclude-from', '--include-from', '--files-from', + // Repeatable: one entry name inside the source directory, per occurrence. + '--only', '--bwlimit', '--max-size', '--min-size', diff --git a/apps/cli/src/tui/app.ts b/apps/cli/src/tui/app.ts index 56eed9c..5878b48 100644 --- a/apps/cli/src/tui/app.ts +++ b/apps/cli/src/tui/app.ts @@ -1,5 +1,5 @@ -import { readdirSync, statSync } from 'node:fs' -import { homedir } from 'node:os' +import { mkdtempSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' import { join, posix } from 'node:path' import { knownHostsPath } from '@diskpush/database' import { SftpBrowser, SshSession } from '@diskpush/ssh-core' @@ -31,10 +31,19 @@ export type Pane = { index: number offset: number error: string | null + /** + * Entries marked with space. Empty means the whole directory, which is what + * every transfer here used to be: the pane had a cursor and no way to say + * "these two", so `s` always sent everything you were looking at. + * + * Names, not paths, and cleared whenever the pane moves, because a mark + * refers to a row in the directory currently on screen. + */ + marked: Set } const HELP = - 'tab switch arrows/jk move enter open left up c change endpoint s sync to other p preview r refresh q quit' + 'tab switch arrows/jk move space mark enter open left up c endpoint s sync p preview r refresh q quit' /** Somewhere a pane can point at: this machine, or a server. */ export type EndpointChoice = { @@ -82,7 +91,7 @@ export function buildEndpointChoices( } export function blankPane(label: string, path: string, connection: Connection | null = null): Pane { - return { label, connection, path, entries: [], index: 0, offset: 0, error: null } + return { label, connection, path, entries: [], index: 0, offset: 0, error: null, marked: new Set() } } export class Tui { @@ -99,6 +108,8 @@ export class Tui { * resolver is held until a key answers it. */ private hostKey: { host: string; fingerprint: string; keyType: string; decide: (trust: boolean) => void } | null = null + /** The pending "transfer N files?" question, while it is on screen. */ + private confirm: { headline: string; detail: string; decide: (approved: boolean) => void } | null = null constructor( left: Pane, @@ -157,6 +168,9 @@ export class Tui { pane.entries = pane.connection ? await this.listRemote(pane) : listLocal(pane.path) pane.index = 0 pane.offset = 0 + // A mark names a row in the directory that was on screen. Carrying it + // into a new listing would transfer whatever happened to share the name. + pane.marked.clear() } catch (error) { pane.entries = [] pane.error = error instanceof Error ? error.message : String(error) @@ -210,10 +224,14 @@ export class Tui { const entry = pane.entries[pane.offset + row] if (!entry) return '' const selected = pane.offset + row === pane.index && side === this.active + const marked = pane.marked.has(entry.name) const name = entry.isDirectory ? `${entry.name}/` : entry.name + // The mark sits in its own column so a name never shifts when it is + // toggled, and it survives the reverse-video cursor. const size = entry.isDirectory ? '' : formatSize(entry.size) - const body = `${pad(truncate(name, paneWidth - 8), paneWidth - 8)} ${size.padStart(6)}` - return selected ? `${ansi.reverse}${body}${ansi.reset}` : body + const body = `${marked ? '*' : ' '}${pad(truncate(name, paneWidth - 9), paneWidth - 9)} ${size.padStart(6)}` + if (selected) return `${ansi.reverse}${body}${ansi.reset}` + return marked ? `${ansi.yellow}${body}${ansi.reset}` : body }) out.push(` ${pad(cells[0] ?? '', paneWidth)} ${pad(cells[1] ?? '', paneWidth)}`) } @@ -224,6 +242,7 @@ export class Tui { let frame = out.join('\n') if (this.picker) frame += this.renderPicker(columns, rows) if (this.hostKey) frame += this.renderHostKey(columns, rows) + if (this.confirm) frame += this.renderConfirm(columns, rows) process.stdout.write(frame) } @@ -257,6 +276,16 @@ export class Tui { return true } + if (this.confirm) { + // Anything that is not an explicit yes cancels. A transfer is not the + // sort of thing to start because a key was mashed, and `q` here means + // "not this" rather than "quit", the same as it does in the picker. + const decide = this.confirm.decide + this.confirm = null + decide(isChar(key, 'y') || isChar(key, 'Y')) + return true + } + if (this.picker) { // Escape closes the picker rather than the app: inside a dialog it means // "not this", which is not the same as "quit". @@ -295,6 +324,8 @@ export class Tui { await this.goUp() } else if (key === 'right' || key === 'enter' || isChar(key, 'l')) { await this.enter() + } else if (isChar(key, ' ')) { + this.toggleMark() } else if (isChar(key, 'c')) { this.openPicker() } else if (isChar(key, 'r')) { @@ -307,6 +338,21 @@ export class Tui { return true } + /** + * Marks or unmarks the row under the cursor, then steps down. + * + * Stepping down is what every file manager does and what makes marking a + * run of files one key repeated rather than an alternation of two. + */ + private toggleMark(): void { + const pane = this.current + const entry = pane.entries[pane.index] + if (!entry) return + if (pane.marked.has(entry.name)) pane.marked.delete(entry.name) + else pane.marked.add(entry.name) + this.move(1) + } + private move(delta: number): void { const pane = this.current const last = Math.max(0, pane.entries.length - 1) @@ -395,6 +441,30 @@ export class Tui { } /** The host-key question, drawn over everything. */ + private renderConfirm(columns: number, rows: number): string { + const ask = this.confirm! + const width = Math.max(40, Math.min(72, columns - 6)) + const left = Math.max(1, Math.floor((columns - width) / 2)) + const top = Math.max(1, Math.floor(rows / 2) - 2) + const inner = width - 2 + const out: string[] = [] + const line = (row: number, body: string) => out.push(`${ansi.moveTo(row, left)}${body}`) + + line(top, `${ansi.blue}+${'-'.repeat(inner)}+${ansi.reset}`) + line( + top + 1, + `${ansi.blue}|${ansi.reset}${ansi.bold}${pad(` ${truncate(ask.headline, inner - 2)}`, inner)}${ansi.reset}${ansi.blue}|${ansi.reset}`, + ) + line( + top + 2, + `${ansi.blue}|${ansi.reset}${ansi.dim}${pad(` ${truncate(ask.detail, inner - 2)}`, inner)}${ansi.reset}${ansi.blue}|${ansi.reset}`, + ) + line(top + 3, `${ansi.blue}|${ansi.reset}${pad('', inner)}${ansi.blue}|${ansi.reset}`) + line(top + 4, `${ansi.blue}|${ansi.reset}${pad(' y transfer n cancel', inner)}${ansi.blue}|${ansi.reset}`) + line(top + 5, `${ansi.blue}+${'-'.repeat(inner)}+${ansi.reset}`) + return out.join('') + } + private renderHostKey(columns: number, rows: number): string { const key = this.hostKey! const width = Math.max(40, Math.min(72, columns - 6)) @@ -432,28 +502,85 @@ export class Tui { await this.load(this.active) } + /** + * The marked entries as a file rsync can read, or null for the whole folder. + * + * NUL-separated, because a newline is legal in a filename and a + * line-separated list would split one such name into two paths that do not + * exist. + */ + private markList(pane: Pane): { path: string; cleanup: () => void } | null { + if (pane.marked.size === 0) return null + const directory = mkdtempSync(join(tmpdir(), 'diskpush-marks-')) + const path = join(directory, 'files-from') + writeFileSync(path, `${[...pane.marked].join('\0')}\0`) + return { path, cleanup: () => rmSync(directory, { recursive: true, force: true }) } + } + + /** + * Previews, then transfers what was previewed. + * + * `s` used to start an immediate transfer of the entire directory. There was + * no way to say "these two" and no moment at which anything could be + * refused: by the time a number was on screen the files were already moving. + * Now the dry run always runs first, and a real transfer waits on a yes. + */ private async transfer(previewOnly: boolean): Promise { const source = this.current const destination = this.other + const scope = source.marked.size > 0 ? `${source.marked.size} marked` : 'whole folder' this.busy = true - this.status = `${ansi.yellow}${previewOnly ? 'Previewing' : 'Syncing'} ${source.path} -> ${destination.path}${ansi.reset}` + this.status = `${ansi.yellow}Scanning ${source.path} -> ${destination.path} (${scope})${ansi.reset}` this.render() + const list = this.markList(source) try { const remote = source.connection ?? destination.connection - const plan = planTransfer({ + const optionsFor = (dryRun: boolean) => + defaultRsyncOptions({ + dryRun, + stats: true, + ...(list ? { filesFrom: list.path, from0: true } : {}), + }) + const shell = remote ? { remoteShell: { keyPath: remote.keyPath, port: remote.port } } : {} + const endpoints = { source: parseEndpoint(endpointString(source)), destination: parseEndpoint(endpointString(destination)), - options: defaultRsyncOptions({ dryRun: previewOnly, stats: true }), - ...(remote ? { remoteShell: { keyPath: remote.keyPath, port: remote.port } } : {}), - }) - const result = await runToCompletion(plan) + } + + const dry = await runToCompletion(planTransfer({ ...endpoints, options: optionsFor(true), ...shell })) + const preview = summarizeChanges(dry.changes) + const moving = preview.add + preview.update + + if (!dry.ok) { + this.status = `${ansi.red}${truncate(dry.message, 200)}${ansi.reset}` + return + } + if (previewOnly) { + this.status = `${ansi.green}Preview (${scope}): ${preview.add} to add, ${preview.update} to update, ${preview.unchanged} unchanged${ansi.reset}` + return + } + if (moving === 0) { + this.status = `${ansi.green}Nothing to transfer (${scope}). The destination already matches.${ansi.reset}` + return + } + + const approved = await this.ask( + `Transfer ${moving} file${moving === 1 ? '' : 's'} (${scope})?`, + `into ${destination.path}`, + ) + if (!approved) { + this.status = `${ansi.yellow}Cancelled. Nothing was transferred.${ansi.reset}` + return + } + + this.status = `${ansi.yellow}Syncing ${source.path} -> ${destination.path}${ansi.reset}` + this.render() + const result = await runToCompletion(planTransfer({ ...endpoints, options: optionsFor(false), ...shell })) const summary = summarizeChanges(result.changes) if (!result.ok) { this.status = `${ansi.red}${truncate(result.message, 200)}${ansi.reset}` - } else if (previewOnly) { - this.status = `${ansi.green}Preview: ${summary.add} to add, ${summary.update} to update, ${summary.unchanged} unchanged${ansi.reset}` } else { this.status = `${ansi.green}Synced ${summary.add + summary.update} files${ansi.reset}` await this.load(this.active === 'left' ? 'right' : 'left') @@ -461,10 +588,26 @@ export class Tui { } catch (error) { this.status = `${ansi.red}${truncate(error instanceof Error ? error.message : String(error), 200)}${ansi.reset}` } finally { + list?.cleanup() this.busy = false } } + /** + * A yes/no question over the panes. Resolves false on anything but y. + * + * Two lines, because `truncate` keeps the END of a string (the right choice + * for a path, the wrong one for a sentence). One combined line lost its own + * verb: "Transfer 4 files to /very/long/path?" rendered as "…r 4 files to + * /very/long/path?", which is a question about nothing. + */ + private ask(headline: string, detail: string): Promise { + return new Promise((resolve) => { + this.confirm = { headline, detail, decide: resolve } + this.render() + }) + } + close(): void { for (const session of this.sessions.values()) session.close() } diff --git a/apps/desktop/electron/main/services/selection.test.ts b/apps/desktop/electron/main/services/selection.test.ts new file mode 100644 index 0000000..1bded4f --- /dev/null +++ b/apps/desktop/electron/main/services/selection.test.ts @@ -0,0 +1,160 @@ +import { existsSync, readFileSync } from 'node:fs' +import { describe, expect, it, vi } from 'vitest' +import type { RsyncEvent } from '@diskpush/schemas' +import { TransferRequestSchema } from '../../shared/contract.js' + +/** A run that ends immediately; these tests are about the plan, not the stream. */ +function emptyRun() { + return { + cancel: () => {}, + events: { + [Symbol.asyncIterator]() { + return { next: () => Promise.resolve({ value: undefined as never, done: true as const }) } + }, + }, + } +} + +const planned: Array<{ options: { filesFrom?: string | null; from0?: boolean } }> = [] + +vi.mock('./store.js', () => ({ + store: async () => ({ getSetting: async (_key: string, fallback: T) => fallback }), +})) + +vi.mock('@diskpush/rsync-core', () => ({ + planTransfer: (input: { options: { filesFrom?: string | null; from0?: boolean } }) => { + planned.push({ options: input.options }) + return { binary: 'rsync', args: [], display: 'rsync', controlDisplay: null, warnings: [] } + }, + runPlan: () => emptyRun(), + parseRsyncCapabilities: () => ({}), + intersectCapabilities: (a: unknown) => a, + unknownCapabilities: () => ({}), +})) + +const { previewTransfer } = await import('./transfers.js') + +const BASE = { + source: { type: 'local' as const, path: '/src/' }, + destination: { type: 'local' as const, path: '/dst/' }, + options: { + archive: true, + checksum: false, + compression: 'auto' as const, + deleteMode: 'off' as const, + hardLinks: false, + acls: false, + xattrs: false, + numericIds: false, + update: false, + ignoreExisting: false, + existingOnly: false, + inplace: false, + excludes: [], + includes: [], + bwlimit: null, + maxSize: null, + minSize: null, + }, + deletesConfirmed: false, +} + +const sender = () => ({ isDestroyed: () => false, send: () => {} }) as never + +describe('a selection reaches rsync', () => { + /* + * The bug: the panes let you select entries and the request only ever + * carried the directory, so ticking two folders and pressing Sync copied the + * whole tree. Two movies became forty thousand files. + */ + it('turns picked entries into a NUL-separated --files-from list', async () => { + planned.length = 0 + await previewTransfer( + { ...BASE, selection: ['The Movie (2019)', 'Another Movie'], previewId: 's1' }, + sender(), + ) + + const options = planned.at(-1)!.options + expect(options.from0).toBe(true) + expect(options.filesFrom).toBeTruthy() + }) + + it('writes the names verbatim, separated by NUL', async () => { + planned.length = 0 + let listPath = '' + // Captured during the run, because the file is removed when it finishes. + const capture = new Promise((resolve) => { + const timer = setInterval(() => { + const path = planned.at(-1)?.options.filesFrom + if (path && existsSync(path)) { + listPath = readFileSync(path, 'utf8') + clearInterval(timer) + resolve() + } + }, 1) + setTimeout(() => { + clearInterval(timer) + resolve() + }, 2000) + }) + + const run = previewTransfer( + { ...BASE, selection: ['a b.mkv', 'weird\tname'], previewId: 's2' }, + sender(), + ) + await Promise.all([run, capture]) + + expect(listPath).toBe('a b.mkv\0weird\tname\0') + }) + + it('leaves an unselected transfer alone, so the whole folder still syncs', async () => { + planned.length = 0 + await previewTransfer({ ...BASE, selection: [], previewId: 's3' }, sender()) + + const options = planned.at(-1)!.options + expect(options.filesFrom).toBeFalsy() + expect(options.from0).toBeFalsy() + }) + + it('removes the list file when the run is over', async () => { + planned.length = 0 + await previewTransfer({ ...BASE, selection: ['one'], previewId: 's4' }, sender()) + + const path = planned.at(-1)!.options.filesFrom + expect(path).toBeTruthy() + expect(existsSync(path!)).toBe(false) + }) +}) + +describe('the selection is names, never paths', () => { + /* + * This list becomes an rsync --files-from rooted at the source directory, so + * a renderer that could put a separator or `..` in here would be choosing + * which files leave the machine. The schema is the only thing standing + * between those two facts. + */ + const reject = (selection: string[]) => + TransferRequestSchema.safeParse({ ...BASE, selection }).success + + it('rejects anything that could climb out of the directory', () => { + expect(reject(['../../etc/shadow'])).toBe(false) + expect(reject(['..'])).toBe(false) + expect(reject(['sub/dir'])).toBe(false) + expect(reject(['back\\slash'])).toBe(false) + expect(reject(['nul\0byte'])).toBe(false) + expect(reject(['/etc/passwd'])).toBe(false) + }) + + it('accepts ordinary names, spaces and all', () => { + expect(reject(['The Movie (2019).mkv', 'Another.Movie.2020'])).toBe(true) + }) + + it('defaults to the whole folder when the field is absent', () => { + const parsed = TransferRequestSchema.parse({ + source: BASE.source, + destination: BASE.destination, + options: BASE.options, + }) + expect(parsed.selection).toEqual([]) + }) +}) diff --git a/apps/desktop/electron/main/services/transfers.ts b/apps/desktop/electron/main/services/transfers.ts index 30f134b..ed8dd5a 100644 --- a/apps/desktop/electron/main/services/transfers.ts +++ b/apps/desktop/electron/main/services/transfers.ts @@ -1,4 +1,7 @@ import { randomUUID } from 'node:crypto' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import type { WebContents } from 'electron' import { intersectCapabilities, @@ -101,31 +104,85 @@ function optionsFrom(input: TransferOptions): RsyncOptions { }) } -async function buildPlan(request: TransferRequest, overrides: Partial = {}): Promise { +/** + * A selection, as a file rsync can read. + * + * NUL-separated (`--from0`) rather than one name per line, because a newline is + * a legal character in a filename and a line-separated list would silently + * split such a name into two paths that do not exist. + * + * The names were validated as entry names on the way in, so none of them can + * contain a separator or `..`; rsync resolves each one against the source + * directory and nothing here can point outside it. + * + * Returns the file path and the cleanup to run once rsync has exited. rsync + * reads the list at startup, but deleting it early is a race nobody needs. + */ +async function writeSelectionList( + selection: readonly string[], +): Promise<{ path: string; cleanup: () => Promise }> { + const directory = await mkdtemp(join(tmpdir(), 'diskpush-selection-')) + const path = join(directory, 'files-from') + await writeFile(path, `${selection.join('\0')}\0`, 'utf8') + return { + // Awaited rather than fire-and-forget, so "the run is over" and "the list + // is gone" are the same moment. A detached rm leaves a window where a + // caller cannot tell whether cleanup happened or simply had not yet. + path, + cleanup: () => rm(directory, { recursive: true, force: true }).catch(() => {}), + } +} + +type BuiltPlan = { plan: ExecutionPlan; cleanup: () => Promise } + +async function buildPlan(request: TransferRequest, overrides: Partial = {}): Promise { const source = await resolveEndpoint(request.source) const destination = await resolveEndpoint(request.destination) const capabilities = await capabilitiesFor([source.connectionId, destination.connectionId]) - const options = { ...optionsFrom(request.options), ...overrides } + + /* + * A selection narrows the transfer to what the user picked. Without this the + * request only ever carried the directory, so ticking two folders and + * pressing Sync copied the entire tree: the panes let you select, and the + * selection reached nothing. + */ + const selection = request.selection ?? [] + const list = selection.length > 0 ? await writeSelectionList(selection) : null + const options = { + ...optionsFrom(request.options), + ...(list ? { filesFrom: list.path, from0: true } : {}), + ...overrides, + } + const cleanup = async () => { + await list?.cleanup() + } const isServerToServer = source.endpoint.type === 'ssh' && destination.endpoint.type === 'ssh' const sourceConnection = source.connectionId ? await resolveConnection(source.connectionId) : null - return planTransfer({ - source: source.endpoint, - destination: destination.endpoint, - options, - capabilities, - deletesConfirmed: request.deletesConfirmed, - ...(isServerToServer - ? { - sourceShell: await shellOptionsFor(source.connectionId), - destinationShell: await shellOptionsFor(destination.connectionId), - sourceRsyncPath: sourceConnection?.rsyncPath ?? null, - } - : { - remoteShell: await shellOptionsFor(source.connectionId ?? destination.connectionId), - }), - }) + try { + const plan = planTransfer({ + source: source.endpoint, + destination: destination.endpoint, + options, + capabilities, + deletesConfirmed: request.deletesConfirmed, + ...(isServerToServer + ? { + sourceShell: await shellOptionsFor(source.connectionId), + destinationShell: await shellOptionsFor(destination.connectionId), + sourceRsyncPath: sourceConnection?.rsyncPath ?? null, + } + : { + remoteShell: await shellOptionsFor(source.connectionId ?? destination.connectionId), + }), + }) + return { plan, cleanup } + } catch (error) { + // A rejected plan still wrote a list file. + await cleanup() + throw error + } } /** @@ -199,7 +256,7 @@ export async function previewTransfer( request: TransferRequest & { previewId: string }, sender: WebContents, ): Promise { - const plan = await buildPlan(request, { dryRun: true }) + const { plan, cleanup } = await buildPlan(request, { dryRun: true }) const summary = summarizeChanges([]) const deletes: string[] = [] @@ -274,6 +331,7 @@ export async function previewTransfer( } } finally { previews.delete(request.previewId) + await cleanup() } emit(true) @@ -307,7 +365,7 @@ export function cancelPreview(previewId: string): boolean { export type StartedJob = { jobId: string; command: string; control: string | null; warnings: string[] } export async function startTransfer(request: TransferRequest, sender: WebContents): Promise { - const plan = await buildPlan(request) + const { plan, cleanup } = await buildPlan(request) const jobId = randomUUID() const db = await store() @@ -369,6 +427,7 @@ export async function startTransfer(request: TransferRequest, sender: WebContent } } running.delete(jobId) + await cleanup() })() return { jobId, command: plan.display, control: plan.controlDisplay ?? null, warnings: plan.warnings } diff --git a/apps/desktop/electron/shared/contract.ts b/apps/desktop/electron/shared/contract.ts index 78f10eb..0121e3e 100644 --- a/apps/desktop/electron/shared/contract.ts +++ b/apps/desktop/electron/shared/contract.ts @@ -75,6 +75,23 @@ export const PathSchema = z.string().min(1).max(4096) export const ConnectionIdSchema = z.string().min(1).max(128) +/** + * A single entry name inside a directory — never a path. + * + * Every mutating operation takes a directory plus one of these and joins them + * in the main process, so the renderer cannot walk out of the folder it is + * showing. `..`, a separator or a NUL would each be a way to do exactly that. + */ +export const EntryNameSchema = z + .string() + .min(1) + .max(255) + .refine((name) => !name.includes('/') && !name.includes('\\') && !name.includes('\0'), { + message: 'A name cannot contain a path separator.', + }) + .refine((name) => name !== '.' && name !== '..', { message: 'That name is reserved.' }) + .refine((name) => name.trim() === name, { message: 'A name cannot begin or end with a space.' }) + export const EndpointRefSchema = z.discriminatedUnion('type', [ z.object({ type: z.literal('local'), path: PathSchema }), z.object({ type: z.literal('ssh'), connectionId: ConnectionIdSchema, path: PathSchema }), @@ -134,6 +151,17 @@ export const TransferRequestSchema = z.object({ options: TransferOptionsSchema, /** Only meaningful for a delete-enabled job, and only after a preview. */ deletesConfirmed: z.boolean().default(false), + /** + * The entries the user picked in the source pane. Empty means the whole + * directory, which is what every transfer used to be. + * + * Names, never paths, and validated as such: the main process turns them + * into an rsync `--files-from` list rooted at the source directory, so a + * renderer that could put `../` or an absolute path in here would be + * choosing which files leave the machine. That is why this reuses + * EntryNameSchema rather than PathSchema. + */ + selection: z.array(EntryNameSchema).max(10000).default([]), }) export type TransferRequest = z.infer @@ -181,23 +209,6 @@ export const RenameRequestSchema = z.object({ to: PathSchema, }) -/** - * A single entry name inside a directory — never a path. - * - * Every mutating operation takes a directory plus one of these and joins them - * in the main process, so the renderer cannot walk out of the folder it is - * showing. `..`, a separator or a NUL would each be a way to do exactly that. - */ -export const EntryNameSchema = z - .string() - .min(1) - .max(255) - .refine((name) => !name.includes('/') && !name.includes('\\') && !name.includes('\0'), { - message: 'A name cannot contain a path separator.', - }) - .refine((name) => name !== '.' && name !== '..', { message: 'That name is reserved.' }) - .refine((name) => name.trim() === name, { message: 'A name cannot begin or end with a space.' }) - /** Create a directory or an empty file: `name` inside `directory`. */ export const CreateEntryRequestSchema = z.object({ connectionId: ConnectionIdSchema.optional(), diff --git a/apps/desktop/src/app/page.tsx b/apps/desktop/src/app/page.tsx index 61c7d41..c8cf488 100644 --- a/apps/desktop/src/app/page.tsx +++ b/apps/desktop/src/app/page.tsx @@ -21,7 +21,7 @@ import { ProfileBar } from '@/components/profile-bar' import { ServerManager } from '@/components/server-manager' import { endpointLabel, loadPane, Pane, type PaneEndpoint, type PaneState } from '@/components/pane' import { TransferRail } from '@/components/transfer-rail' -import { MirrorPreviewDialog, TransferBand, type ActiveJob } from '@/components/transfer-panel' +import { TransferBand, TransferPreviewDialog, type ActiveJob } from '@/components/transfer-panel' import { Button } from '@/components/ui/button' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { @@ -75,6 +75,15 @@ function TabButton({ ) } +/** What the renderer sends to preview or start a transfer. */ +type TransferDraft = { + source: ReturnType + destination: ReturnType + options: { deleteMode: 'off' | 'delay' } + deletesConfirmed: boolean + selection: string[] +} + const blankPane = (endpoint: PaneEndpoint, path: string): PaneState => ({ endpoint, path, @@ -93,7 +102,6 @@ export default function Workspace() { const [active, setActive] = useState<'left' | 'right'>('left') const [direction, setDirection] = useState<'ltr' | 'rtl'>('ltr') const [mirror, setMirror] = useState(false) - const [trust, setTrust] = useState(false) const [preview, setPreview] = useState(null) const [previewProgress, setPreviewProgress] = useState(null) const [previewOpen, setPreviewOpen] = useState(false) @@ -107,6 +115,14 @@ export default function Workspace() { * saw. Every result and every progress event is matched against this id. */ const previewIdRef = useRef(null) + /** + * The request the open preview was built from. + * + * Confirming runs THIS, not a request rebuilt from whatever the panes say by + * then. A dialog that says "delete 12 files" has to start the transfer it + * measured, even if something changed a pane underneath it. + */ + const approvedRequestRef = useRef(null) const [job, setJob] = useState(null) const [error, setError] = useState(null) const [showConnection, setShowConnection] = useState(false) @@ -216,16 +232,33 @@ export default function Workspace() { const allConnections = useMemo(() => [...saved, ...sshConfig], [saved, sshConfig]) const route = `${endpointLabel(source.endpoint, allConnections)} → ${endpointLabel(destination.endpoint, allConnections)}` - const request = useMemo( - () => ({ - // The panes show directory contents, so a sync between them means "make - // these contents match", not "nest this directory inside that one". - source: refFor(source, withTrailingSlash(source.path)), - destination: refFor(destination, withTrailingSlash(destination.path)), - options: { deleteMode: mirror ? ('delay' as const) : ('off' as const) }, - deletesConfirmed: false, - }), - [source, destination, mirror], + /** + * The transfer for a given direction. + * + * It takes the direction rather than reading it, because the rail sets the + * direction and starts the transfer in the same click. `setDirection` does + * not change the value this render already closed over, so a request built + * from state ran the PREVIOUS direction: pressing "Sync to Local" while the + * other arrow was armed copied local over the server instead, and with + * Mirror on it would have deleted the wrong side. + */ + const requestFor = useCallback( + (towards: 'ltr' | 'rtl'): TransferDraft => { + const from = towards === 'ltr' ? left : right + const to = towards === 'ltr' ? right : left + return { + // The panes show directory contents, so a sync between them means "make + // these contents match", not "nest this directory inside that one". + source: refFor(from, withTrailingSlash(from.path)), + destination: refFor(to, withTrailingSlash(to.path)), + options: { deleteMode: mirror ? ('delay' as const) : ('off' as const) }, + deletesConfirmed: false, + // What the user ticked in the pane the files come FROM. Empty means the + // whole directory. + selection: [...from.selected], + } + }, + [left, right, mirror], ) /** @@ -243,7 +276,9 @@ export default function Workspace() { if (previewId) void api()?.transfers.cancelPreview(previewId) }, []) - const runPreview = useCallback(async () => { + const runPreview = useCallback(async (towards: 'ltr' | 'rtl') => { + const request = requestFor(towards) + approvedRequestRef.current = request const previewId = crypto.randomUUID() // Supersedes any scan already running, so pressing Preview twice does not // leave two rsync processes walking the same trees. @@ -268,10 +303,12 @@ export default function Workspace() { setPreviewOpen(false) setError(caught instanceof Error ? caught.message : String(caught)) } - }, [request]) + }, [requestFor]) const start = useCallback( async (deletesConfirmed: boolean) => { + const request = approvedRequestRef.current + if (!request) return setError(null) previewIdRef.current = null setPreviewOpen(false) @@ -294,7 +331,7 @@ export default function Workspace() { setError(caught instanceof Error ? caught.message : String(caught)) } }, - [request], + [], ) /** @@ -337,6 +374,10 @@ export default function Workspace() { const saveProfile = useCallback( async (name: string) => { setError(null) + // A profile stores the pair and its options. Not the selection: a saved + // pair is meant to be re-runnable later, and a list of entry names that + // were ticked once is not a thing that stays true. + const request = requestFor(direction) try { await unwrap( api()?.profiles.save({ @@ -354,7 +395,7 @@ export default function Workspace() { setError(caught instanceof Error ? caught.message : String(caught)) } }, - [request, direction], + [requestFor, direction], ) const removeProfile = useCallback(async (id: string) => { @@ -367,12 +408,26 @@ export default function Workspace() { } }, []) - const run = useCallback(async () => { - // Mirror always previews. A plain sync does not: its dry run costs a full - // scan and buys no safety, because nothing is deleted either way. - if (mirror) await runPreview() - else await start(false) - }, [mirror, runPreview, start]) + /** + * Every manual transfer is previewed and approved. Mirror is not special. + * + * This used to start a plain sync immediately, on the reasoning that a dry + * run "buys no safety, because nothing is deleted either way". Deleting is + * not the only way to regret a transfer: the run that prompted this change + * was two selected folders that turned into forty thousand files, and by the + * time anything is on screen it is already copying. Now the same dialog that + * guards a mirror shows what a sync would do, and nothing starts until it is + * approved. + * + * The direction is passed in rather than read: the rail sets it and runs in + * one click, and the state has not updated yet. + */ + const run = useCallback( + async (towards: 'ltr' | 'rtl') => { + await runPreview(towards) + }, + [runPreview], + ) if (outsideShell) { return ( @@ -567,8 +622,8 @@ export default function Workspace() { rightLabel={railLabel(right.endpoint, allConnections)} onDirection={setDirection} onToggleMirror={() => setMirror((value) => !value)} - onPreview={runPreview} - onRun={run} + onPreview={() => void runPreview(direction)} + onRun={(towards) => void run(towards)} /> setShowConnection(false)} onSaved={() => void refreshConnections()} /> - void start(true)} diff --git a/apps/desktop/src/components/transfer-panel.tsx b/apps/desktop/src/components/transfer-panel.tsx index f10fc9e..1cbb329 100644 --- a/apps/desktop/src/components/transfer-panel.tsx +++ b/apps/desktop/src/components/transfer-panel.tsx @@ -5,7 +5,6 @@ import { ArrowRight, ChevronRight, CircleCheck, Trash2, TriangleAlert } from 'lu import type { PreviewProgress, PreviewResult } from '@/lib/api' import { formatBytes, formatDuration, formatRate } from '@/lib/format' import { Button } from '@/components/ui/button' -import { Checkbox } from '@/components/ui/checkbox' import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog' import { Progress } from '@/components/ui/progress' import { ScrollArea } from '@/components/ui/scroll-area' @@ -116,13 +115,13 @@ function ScanProgress({ progress, onCancel }: { progress: PreviewProgress; onCan * hundreds of thousands and a DOM node each is what froze the window. The * count on the confirm button is always the true one. */ -export function MirrorPreviewDialog({ +export function TransferPreviewDialog({ preview, progress, open, route, - trust, - onTrustChange, + mirror, + selectionCount, onCancel, onStopScan, onConfirm, @@ -131,8 +130,10 @@ export function MirrorPreviewDialog({ progress: PreviewProgress | null open: boolean route: string - trust: boolean - onTrustChange: (value: boolean) => void + /** Deletes are armed. Changes what this dialog is for, not only its wording. */ + mirror: boolean + /** Entries ticked in the source pane. 0 means the whole folder. */ + selectionCount: number onCancel: () => void onStopScan: () => void onConfirm: () => void @@ -140,6 +141,9 @@ export function MirrorPreviewDialog({ const deletes = preview?.deletes ?? [] const deleteTotal = preview?.deleteTotal ?? 0 const hidden = Math.max(0, deleteTotal - deletes.length) + // Additions and updates: what a plain sync actually moves. Deletions are + // counted separately, on the button that arms them. + const changeTotal = (preview?.summary.add ?? 0) + (preview?.summary.update ?? 0) const summary = preview?.summary const cancelRef = useRef(null) @@ -162,16 +166,43 @@ export function MirrorPreviewDialog({ initialFocus={cancelRef} className="max-w-2xl grid-rows-[auto_minmax(0,1fr)_auto] gap-0 border-line-strong bg-popover p-0 [--dialog-pad:0px]" > + {/* + The header says which of the two things this is. Every manual + transfer comes through here now, not only a mirror, and a plain sync + wearing a red warning triangle and the word "Mirror" would be its own + kind of wrong. + */} - - + + {mirror ? : }
- Mirror + {mirror ? 'Mirror' : 'Sync'} {route}
+ {/* + The scope, stated up front. Two ticked folders that turn into forty + thousand files is the failure this dialog exists to catch, and the + counts below only ever say how many, never how many of what was + asked for. + */} + 0 ? 'bg-primary/12 text-primary' : 'bg-secondary text-muted-foreground', + )} + > + {selectionCount > 0 + ? `${selectionCount.toLocaleString()} selected item${selectionCount === 1 ? '' : 's'}` + : 'Whole folder'} + {/* One scrolling body, so a preview with a long delete list keeps its @@ -280,13 +311,16 @@ export function MirrorPreviewDialog({ to render throughout the scan, so a full-strength Mirror button sat under the spinner offering to run a mirror whose delete list did not exist yet -- inert, which reads as a control that ignores you. + + There is no "trust this pair from now on" in here any more either. It + set a piece of renderer state that nothing read, and it could not have + worked if it were wired: saveProfile hard-codes `trustDeletes: false` + on purpose, because unattended mirroring is the one way a delete list + runs with nobody looking at it. A checkbox offering to skip a + confirmation that is deliberately never skipped is worse than none. */} {preview ? ( - - +
{/* Focus opens on Cancel, not on the confirm and not on the trust @@ -299,10 +333,16 @@ export function MirrorPreviewDialog({
diff --git a/apps/desktop/src/components/transfer-rail.tsx b/apps/desktop/src/components/transfer-rail.tsx index 4ba5f28..c33a3c1 100644 --- a/apps/desktop/src/components/transfer-rail.tsx +++ b/apps/desktop/src/components/transfer-rail.tsx @@ -133,7 +133,14 @@ export function TransferRail({ onDirection: (direction: 'ltr' | 'rtl') => void onToggleMirror: () => void onPreview: () => void - onRun: () => void + /* + * Takes the direction it is starting. The rail sets the direction and starts + * the run in the same click, and `setDirection` does not change what this + * render already closed over, so a caller reading the direction from state + * ran the previous one: the arrow you pressed and the way the files moved + * disagreed on the first press. + */ + onRun: (towards: 'ltr' | 'rtl') => void }) { return ( // Every control in the rail is now one width and one corner radius. It @@ -148,7 +155,7 @@ export function TransferRail({ busy={busy} onClick={() => { onDirection('ltr') - onRun() + onRun('ltr') }} /> { onDirection('rtl') - onRun() + onRun('rtl') }} /> diff --git a/packages/rsync-core/src/args.ts b/packages/rsync-core/src/args.ts index fe35a5b..e547313 100644 --- a/packages/rsync-core/src/args.ts +++ b/packages/rsync-core/src/args.ts @@ -98,6 +98,21 @@ export function buildRsyncArgs(input: BuildArgsInput): BuildArgsResult { if (options.archive) args.push('--archive') else args.push('--recursive') + /* + * `--files-from` turns rsync's recursion OFF, and `--archive` does not turn + * it back on. Without an explicit `--recursive` a selected directory copies + * as an empty directory and none of its contents, which looks like a + * successful transfer and is the worst possible failure for a sync tool. + * + * rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * >f+++++++++ movieA/a.mkv + * + * Harmless when `--archive` already asked for recursion without a file list: + * rsync takes the last-wins flag and both say the same thing. + */ + if (options.filesFrom && options.archive) args.push('--recursive') + if (options.hardLinks) args.push('--hard-links') if (options.acls) { if (capabilities.acls || !capabilities.version) args.push('--acls') @@ -140,7 +155,13 @@ export function buildRsyncArgs(input: BuildArgsInput): BuildArgsResult { if (options.includeFrom) args.push(`--include-from=${options.includeFrom}`) for (const exclude of options.excludes) args.push(`--exclude=${exclude}`) if (options.excludeFrom) args.push(`--exclude-from=${options.excludeFrom}`) - if (options.filesFrom) args.push(`--files-from=${options.filesFrom}`) + if (options.filesFrom) { + // Order between these two does not matter (rsync parses the whole argv + // before it opens the list); they are kept together so the separator and + // the file it applies to are read as one thing. + if (options.from0) args.push('--from0') + args.push(`--files-from=${options.filesFrom}`) + } if (options.pruneEmptyDirs) args.push('--prune-empty-dirs') if (options.relative) args.push('--relative') if (options.maxSize) args.push(`--max-size=${options.maxSize}`) diff --git a/packages/rsync-core/src/files-from.test.ts b/packages/rsync-core/src/files-from.test.ts new file mode 100644 index 0000000..c99f8b9 --- /dev/null +++ b/packages/rsync-core/src/files-from.test.ts @@ -0,0 +1,136 @@ +import { mkdtempSync, mkdirSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { defaultRsyncOptions } from '@diskpush/schemas' +import { buildRsyncArgs } from './args.js' +import { runToCompletion } from './runner.js' +import { planTransfer } from './plan.js' + +const local = (path: string) => ({ type: 'local' as const, path }) + +describe('buildRsyncArgs with a file list', () => { + /* + * The bug this exists to prevent, confirmed against rsync 3.4.1: + * + * rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/ + * >f+++++++++ movieA/a.mkv + * + * `--files-from` switches rsync's recursion off and `--archive` does not + * switch it back on, so selecting a folder would copy an empty folder and + * call it a success. + */ + it('forces --recursive, because --archive does not imply it under --files-from', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({ filesFrom: '/tmp/list', from0: true }), + }) + + expect(args).toContain('--archive') + expect(args).toContain('--recursive') + expect(args).toContain('--from0') + expect(args).toContain('--files-from=/tmp/list') + }) + + it('does not add --recursive for an ordinary archive transfer', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({}), + }) + + expect(args).toContain('--archive') + expect(args).not.toContain('--recursive') + }) + + it('leaves --from0 off unless it was asked for', () => { + const { args } = buildRsyncArgs({ + source: local('/src/'), + destination: local('/dst/'), + options: defaultRsyncOptions({ filesFrom: '/tmp/list' }), + }) + + expect(args).not.toContain('--from0') + expect(args).toContain('--files-from=/tmp/list') + }) +}) + +/** + * Against the real binary, because the whole point is a behaviour of rsync + * that the flag names do not tell you about. + */ +describe('live rsync: a selected folder', () => { + function fixture() { + const root = mkdtempSync(join(tmpdir(), 'diskpush-files-from-')) + for (const dir of ['src/movieA', 'src/movieB', 'src/other', 'dst']) { + mkdirSync(join(root, dir), { recursive: true }) + } + writeFileSync(join(root, 'src/movieA/a.mkv'), 'a') + writeFileSync(join(root, 'src/movieB/b.mkv'), 'b') + writeFileSync(join(root, 'src/other/o.txt'), 'o') + writeFileSync(join(root, 'src/top.txt'), 'top') + writeFileSync(join(root, 'list'), 'movieA\0movieB\0') + return root + } + + it('copies its contents, and nothing that was not selected', async () => { + const root = fixture() + const plan = planTransfer({ + source: local(`${join(root, 'src')}/`), + destination: local(`${join(root, 'dst')}/`), + options: defaultRsyncOptions({ + filesFrom: join(root, 'list'), + from0: true, + dryRun: true, + }), + }) + + const result = await runToCompletion(plan) + const paths = result.changes.map((change) => change.path) + + expect(result.ok).toBe(true) + // The contents, not just the directory entry. This is the assertion that + // fails if the --recursive above is ever removed. + expect(paths).toContain('movieA/a.mkv') + expect(paths).toContain('movieB/b.mkv') + // Everything the user did not select stays out of it. + expect(paths).not.toContain('other/o.txt') + expect(paths).not.toContain('top.txt') + }) + + /* + * Mirror plus a selection deletes only inside what was selected. Verified + * rather than assumed, because the alternative reading of `--delete` here + * (everything at the destination that is not in the list) would wipe the + * folder, and that is not a thing to find out in production. + */ + it('scopes a mirror delete to the selection', async () => { + const root = fixture() + mkdirSync(join(root, 'dst/movieA'), { recursive: true }) + mkdirSync(join(root, 'dst/keepme'), { recursive: true }) + writeFileSync(join(root, 'dst/movieA/stale.mkv'), 'stale') + writeFileSync(join(root, 'dst/keepme/k.txt'), 'keep') + writeFileSync(join(root, 'dst/untouched.txt'), 'untouched') + + const plan = planTransfer({ + source: local(`${join(root, 'src')}/`), + destination: local(`${join(root, 'dst')}/`), + options: defaultRsyncOptions({ + filesFrom: join(root, 'list'), + from0: true, + deleteMode: 'delay', + dryRun: true, + }), + deletesConfirmed: true, + }) + + const result = await runToCompletion(plan) + const deletes = result.changes.filter((c) => c.action === 'delete').map((c) => c.path) + + expect(deletes).toContain('movieA/stale.mkv') + expect(deletes).not.toContain('keepme/k.txt') + expect(deletes).not.toContain('untouched.txt') + }) +}) diff --git a/packages/schemas/src/rsync-options.ts b/packages/schemas/src/rsync-options.ts index 7f6aff6..b7de4a1 100644 --- a/packages/schemas/src/rsync-options.ts +++ b/packages/schemas/src/rsync-options.ts @@ -52,6 +52,15 @@ export const RsyncOptionsSchema = z.object({ excludeFrom: z.string().min(1).nullable().default(null), includeFrom: z.string().min(1).nullable().default(null), filesFrom: z.string().min(1).nullable().default(null), + /** + * Read `filesFrom` as NUL-separated rather than one path per line. + * + * A newline is a legal character in a filename on every platform DiskPush + * runs on, so a line-separated list silently turns one such name into two + * paths that do not exist. Anything generating the list from real directory + * entries should set this. + */ + from0: z.boolean().default(false), maxSize: z.string().min(1).nullable().default(null), minSize: z.string().min(1).nullable().default(null), pruneEmptyDirs: z.boolean().default(false), From 93cce3847a4580e4acefb90151ebd5f38f917eba Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 6 Sep 2026 08:35:52 +0000 Subject: [PATCH 2/2] test: read the selection list at plan time instead of racing cleanup The "writes the names verbatim" test polled on a 1ms timer for a file that exists only while the transfer is being planned, so on a machine where the preview finished first it read nothing and failed. It passed here and failed in CI, which is the signature of a test that is timing rather than asserting. The mocked planTransfer now reads the list when it is handed the path. That runs while the transfer still owns the file, so there is no window to race. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52 --- .../electron/main/services/selection.test.ts | 32 ++++--------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/apps/desktop/electron/main/services/selection.test.ts b/apps/desktop/electron/main/services/selection.test.ts index 1bded4f..e9ec887 100644 --- a/apps/desktop/electron/main/services/selection.test.ts +++ b/apps/desktop/electron/main/services/selection.test.ts @@ -15,7 +15,7 @@ function emptyRun() { } } -const planned: Array<{ options: { filesFrom?: string | null; from0?: boolean } }> = [] +const planned: Array<{ options: { filesFrom?: string | null; from0?: boolean }; list: string | null }> = [] vi.mock('./store.js', () => ({ store: async () => ({ getSetting: async (_key: string, fallback: T) => fallback }), @@ -23,7 +23,10 @@ vi.mock('./store.js', () => ({ vi.mock('@diskpush/rsync-core', () => ({ planTransfer: (input: { options: { filesFrom?: string | null; from0?: boolean } }) => { - planned.push({ options: input.options }) + // Read here rather than from the test body: this runs while the transfer + // still owns the file, so there is no window to race against its cleanup. + const path = input.options.filesFrom + planned.push({ options: input.options, list: path ? readFileSync(path, 'utf8') : null }) return { binary: 'rsync', args: [], display: 'rsync', controlDisplay: null, warnings: [] } }, runPlan: () => emptyRun(), @@ -81,30 +84,9 @@ describe('a selection reaches rsync', () => { it('writes the names verbatim, separated by NUL', async () => { planned.length = 0 - let listPath = '' - // Captured during the run, because the file is removed when it finishes. - const capture = new Promise((resolve) => { - const timer = setInterval(() => { - const path = planned.at(-1)?.options.filesFrom - if (path && existsSync(path)) { - listPath = readFileSync(path, 'utf8') - clearInterval(timer) - resolve() - } - }, 1) - setTimeout(() => { - clearInterval(timer) - resolve() - }, 2000) - }) - - const run = previewTransfer( - { ...BASE, selection: ['a b.mkv', 'weird\tname'], previewId: 's2' }, - sender(), - ) - await Promise.all([run, capture]) + await previewTransfer({ ...BASE, selection: ['a b.mkv', 'weird\tname'], previewId: 's2' }, sender()) - expect(listPath).toBe('a b.mkv\0weird\tname\0') + expect(planned.at(-1)!.list).toBe('a b.mkv\0weird\tname\0') }) it('leaves an unselected transfer alone, so the whole folder still syncs', async () => {