From 514cf5b4c9b635ffa62b19a37efcb054519a1317 Mon Sep 17 00:00:00 2001 From: Oluwatobi Ogundimu Date: Sun, 2 Aug 2026 17:56:23 +0100 Subject: [PATCH] SC-12: partial and append file transfers Verified against real rsync source (generator.c, sender.c): plain --append blindly trusts the existing prefix with zero real checksums; --append-verify runs the identical normal per-block signature algorithm, needing no new algorithm, only an eligibility gate. Both share two rules: non-existent destination transfers normally, destination not shorter than source is skipped entirely. Prerequisite correctness fix, not scope creep: Receiver previously wrote regular files directly to their final destination with no temp file at all, meaning a process kill mid-write could corrupt a good destination file regardless of any flag - this predates SC-12 entirely. Implementing --partial correctly required making temp-file-then-atomic- rename the unconditional default write path first; --partial/ --partial-dir only control what happens to that temp file if a transfer aborts before the rename. --partial/--partial-dir: file-granularity, explicitly disclosed as not true mid-file resumption given grsync's atomic-per-file wire frames (same disclosure pattern as SC-10's progress reporting). Real content-level resumption across runs via using a leftover partial-dir file as the delta comparison basis. --append: wire-level blind trust via a directly-constructed CopyOp, reusing sync.ApplyDelta unchanged. --append-verify: normal pipeline, gated by eligibility. Self-review: --append's documented corruption risk faithfully reproduced, not worsened, locked in by a dedicated test. Found and fixed sync.ApplyDelta unconditionally rejecting BlockSize <= 0 even when no CopyOp needed it - would have broken append mode against a brand-new empty destination file. Real wire-byte-count proof partial-dir resumption is genuinely cheaper. Real interrupted-multi-file-sync test, corruption-preserved-vs-corrected proof, dry-run and hard-link exclusion tests, real SSH test. Clean on native Windows and cross-compiled Linux. --- README.md | 206 +++++++++++- internal/cli/partial_append_test.go | 144 +++++++++ internal/cli/root.go | 20 ++ internal/cli/sync.go | 64 +++- internal/pipeline/append_test.go | 284 ++++++++++++++++ internal/pipeline/itemize.go | 47 +++ internal/pipeline/messages.go | 38 ++- internal/pipeline/messages_test.go | 2 +- internal/pipeline/partial.go | 207 ++++++++++++ internal/pipeline/partial_integration_test.go | 240 ++++++++++++++ internal/pipeline/partial_test.go | 305 ++++++++++++++++++ internal/pipeline/receiver.go | 181 ++++++----- internal/pipeline/sender.go | 58 +++- internal/pipeline/ssh_test.go | 51 +++ internal/sync/delta.go | 14 +- internal/sync/delta_test.go | 32 +- 16 files changed, 1796 insertions(+), 97 deletions(-) create mode 100644 internal/cli/partial_append_test.go create mode 100644 internal/pipeline/append_test.go create mode 100644 internal/pipeline/partial.go create mode 100644 internal/pipeline/partial_integration_test.go create mode 100644 internal/pipeline/partial_test.go diff --git a/README.md b/README.md index 950efcd..f3701ec 100644 --- a/README.md +++ b/README.md @@ -11,10 +11,10 @@ really walks, filters, diffs, transfers, and reconstructs files, applying requested attributes along the way. See [End-to-End Sync Pipeline](#end-to-end-sync-pipeline) below for exactly how the pieces connect and, just as importantly, what's still explicitly -out of scope (partial/append transfers, batch mode, full `--delete`, and -device/special files) - this is real, working sync, not yet full feature -parity. Hard links *are* now preserved, opt-in via `-H`/`--hard-links` -exactly like real rsync's own flag (see +out of scope (batch mode, full `--delete`, and device/special files) - +this is real, working sync, not yet full feature parity. Hard links +*are* now preserved, opt-in via `-H`/`--hard-links` exactly like real +rsync's own flag (see [File Attribute Preservation](#file-attribute-preservation) below), and `--dry-run`/`-n` is a genuine trial run - full planning, zero filesystem changes - with real `--itemize-changes`/`-i` output matching rsync's own @@ -24,6 +24,10 @@ formats (see [Progress and Stats](#progress-and-stats) below). `--compress`/`-z` now genuinely compresses a file's literal delta data with zlib, including real rsync's own `--compress-level` and `--skip-compress` (see [Compression](#compression) below). +`--partial`/`--partial-dir` and `--append`/`--append-verify` are now +implemented too, with a real, disclosed scope boundary around what +"partial" means in grsync's own architecture (see +[Partial and Append Transfers](#partial-and-append-transfers) below). `grsync --daemon` also now speaks a real subset of the rsync daemon protocol - `rsyncd.conf` parsing, the `@RSYNCD` greeting/handshake, module @@ -827,6 +831,200 @@ outright, recreating the link directly instead (see in the first place. `TestSenderReceiver_CompressWorksWithHardLinks` confirms that skip still holds correctly with compression enabled. +## Partial and Append Transfers + +`--partial`/`--partial-dir` keep (and can resume from) a file whose +transfer didn't finish; `--append`/`--append-verify` extend a +destination file that's shorter than the source without re-sending the +part that's already there. All four are implemented against real +rsync's own documented behavior (`rsync.1`) and, where the docs were +ambiguous, its actual source (`generator.c`, `sender.c`) - verified +rather than assumed. + +### A real prerequisite this ticket needed, not just added + +Before this ticket, `Receiver` wrote a regular file's new content +straight to its final destination path (`os.WriteFile`, or a chunked +`os.OpenFile` loop for `--progress`). That meant a process killed +mid-write left a genuinely truncated file sitting at the real +destination, with no flag able to prevent or recover from it - `--partial` +literally cannot mean anything sensible without a separate temp file to +keep or discard in the first place. Every regular file is now written to +a fresh temp file next to its destination (`.name.RANDOM.grsync-tmp`, +created via `os.CreateTemp` for safe unique naming, `0644` by default to +match `os.WriteFile`'s own prior behavior) and only renamed into place +once it's completely written - unconditionally, not just when `--partial` +is given. `--partial`/`--partial-dir` control only what happens to that +temp file if the transfer aborts before the rename. + +### What "partial" means in grsync's architecture (a real, disclosed scope boundary) + +grsync's wire protocol has no streaming I/O: one regular file's delta +arrives as a single, atomic gob-encoded frame, fully decoded into memory +before a single byte of it is written to disk (the same finding SC-10 +already made for `--progress`). There is no such thing as "half of this +file's delta arrived" - a dropped connection mid-frame just means that +file's transfer never started at all, while everything already written +for *earlier* files in the same sync stays exactly as complete as it +already was. + +**`--partial` in grsync is therefore file-granularity, not real rsync's +true byte-level mid-file resumption.** It describes which *whole files* +survive an interrupted multi-file sync, not a partially-written single +file left in a recoverable half-complete state on the wire. Concretely: +if file 3 of 5 is the one in flight when a connection drops, files 1-2 +are already complete and untouched by `--partial` either way; file 3 is +either fully absent (if the drop happened while its delta was still in +transit - the common case) or has its temp file kept/discarded per +`--partial`/`--partial-dir` (if the drop happened during the local write +itself); files 4-5 are never attempted at all. +`TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial` +is the direct proof of this, run with and without `--partial` to confirm +the flag genuinely doesn't change that particular outcome. + +### `--partial` + +Without it, a temp file left behind by an aborted transfer is deleted - +the destination is left exactly as it was before the sync started (or +absent, for a new file). With it, the temp file is instead renamed onto +the real destination path, matching real rsync's own default "keep it +where the real file goes" behavior - a subsequent run's normal +signature/delta exchange against that destination file then picks up +whatever prefix happens to still be correct, for free, with no +additional resume-lookup mechanism needed. + +### `--partial-dir DIR` + +Implies `--partial` (matching real rsync's own documented "also implying +that [`--partial`] be enabled"). Instead of overwriting the real +destination with the partial result, the temp file is moved into DIR, +leaving the real destination completely untouched. A relative DIR is +created inside *each file's own destination directory* (real rsync's own +documented placement, so a `--partial-dir=.rsync-partial` can be reused +across an entire tree without files colliding); an absolute DIR is a +single shared directory, so grsync mirrors each file's full relative +path underneath it instead of just its basename - real rsync's own docs +don't spell out this exact scheme for the absolute case, but mirroring +the relative path is the only one that can't collide between two files +sharing a basename in different subdirectories. + +**Real content-level resumption, not just retention**: on a later run, +if a file has a leftover partial-dir file, it's used as the delta +comparison basis *instead of* the (possibly nonexistent) real +destination file - `sync.GenerateDelta` then naturally produces `CopyOp`s +for whatever prefix still matches and literal data only for the +genuinely new tail, exactly like a resumed transfer should. +`TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry` measures this +directly: a resumed transfer with a genuine partial-dir prefix writes +meaningfully fewer bytes to the wire than an identical transfer starting +from nothing. The partial-dir file is removed once it's successfully +folded into a completed transfer, matching real rsync's own documented +"delete it after it has served its purpose." + +**Self-review: could a partial file leak outside `--partial-dir`?** +No - verified both by re-reading `abandonOrKeep` (`internal/pipeline/partial.go`) +and by dedicated tests +(`TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent`, +`TestAbandonOrKeep_NeverLeavesATempFileNextToDestPath`): whenever +`--partial-dir` is set, an aborted temp file's only two possible +destinations are the partial-dir path itself or deletion - the real +destination path is never touched by that code path at all. + +### `--append` vs `--append-verify`: the real, verified distinction + +Both share the same two eligibility rules, verified against real +rsync's own source (`generator.c`) rather than assumed: a destination +file that doesn't exist yet is transferred completely normally (append +semantics only ever apply to an *existing*, shorter file - "new files +are transferred," per `rsync.1`); a destination that's already at least +as long as the source is skipped **entirely and unconditionally** - not +compared, not touched at all, even if its content genuinely differs. + +For a genuinely shorter destination, the two diverge exactly where real +rsync's own source diverges (`generate_and_send_sums`: plain `--append` +returns after writing only a header, sending zero real block checksums +at all; `--append-verify` falls through to the completely normal +per-block signature loop): + +- **`--append`** blindly trusts the existing prefix - grsync's receiver + never reads or hashes it at all, just its length. The sender is told + (via a wire-level `Append` marker on the signature message) to send + only the literal tail past that offset, represented as a single + `CopyOp` ("take the receiver's word for these bytes, unverified") + followed by a `DataOp` for the new data - reusing `sync.ApplyDelta` + completely unchanged, since `CopyOp`'s own contract only ever claims to + copy a block, never that it was checksum-verified. +- **`--append-verify`** runs the exact same `sync.GenerateSignature`/ + `GenerateDelta`/`ApplyDelta` pipeline as an entirely normal sync - + needing no new algorithm at all. The eligibility rules above are the + only thing it actually adds on top of a vanilla transfer. + +**Self-review: does `--append` risk silent corruption?** Yes - and this +is real rsync's own documented risk (`rsync.1`: "**can be dangerous** if +you aren't 100% sure... existing content... is also known to be the +same"), reproduced faithfully here, not worsened and not silently fixed. +`TestReceiver_AppendDoesNotVerifyCorruptedPrefix` locks this in +explicitly: a destination with a wrong existing prefix ends up with that +wrong prefix preserved verbatim, plus the correct new tail appended after +it - exactly real rsync's own documented behavior. +`TestReceiver_AppendVerifyDetectsCorruptedPrefix` proves the same +scenario is fully corrected under `--append-verify`. **Use `--append` +only when you are certain the existing destination content is already +correct** (e.g. a log file only this sync ever writes to) - exactly real +rsync's own guidance. + +A source file that shrinks below what the receiver already trusts +between the receiver's own check and the sender's actual read (real +rsync's own "diminished file" race, normally handled with a warning and +a per-file skip) is treated as a hard error here instead: grsync's +`Receiver` has no general "skip this one file, keep going" mechanism +anywhere else in the codebase, and building one solely for this narrow +race was judged a bigger change than this ticket's own scope - a +disclosed simplification, not a silent gap. + +`--append` and `--append-verify` are mutually exclusive (a clear CLI +error, matching the same "reject the ambiguous combination outright" +philosophy `--ipv4`/`--ipv6` already established). + +### Interaction with dry-run and hard links + +Both flags fully respect `--dry-run`: the append-aware signature/delta +exchange (and, for `--partial-dir`, the partial-file basis lookup) still +runs for accurate itemize planning, but the temp-file/rename/cleanup +code all lives inside the same `if !DryRun` guard every other write in +`Receiver` does, so nothing is ever created, modified, or deleted on +disk. `TestReceiver_AppendWorksWithDryRun` and +`TestReceiver_PartialDirCreatesNoFilesAndDeletesNothingDuringDryRun` +both confirm this directly, the latter specifically checking that a +leftover partial-dir file survives a dry run completely untouched +(neither consumed nor deleted). + +A hard-link group's secondary members never reach the signature/delta +exchange at all (see [Compression](#compression)'s own identical note) - +so `--partial`/`--append` are structurally moot for them, not specially +excluded by any append- or partial-specific code. +`TestReceiver_AppendExcludesHardLinkSecondaryMembers` confirms that +structural fact still holds with `--append` enabled. + +### Across transports + +- **Local and SSH**: fully supported. For SSH, `--partial`/ + `--partial-dir`/`--append`/`--append-verify` are forwarded to the + remote `--server` process as ordinary argv flags - the same mechanism + SC-11 established for `--dry-run`/`--itemize-changes` - parsed there + through the server's own normal flag handling, no wire-protocol change + needed. Verified over a real SSH connection by + `TestSSHLocalhost_AppendAndPartialDoNotBreakTheTransfer`. +- **`rsync://` daemon**: **not available**, a real, disclosed gap rather + than a silent one. An upload's module `Receiver` runs on the daemon + server (see [rsync Daemon Mode](#rsync-daemon-mode)), and + `syncToRsyncDaemon` only ever forwards `DryRun` to it via a dedicated + wire token - extending that protocol to also carry these four flags is + real, separate work outside this ticket's own scope. `grsync` prints a + one-time note when any of them is combined with an `rsync://` + destination, the same pattern already established for + itemize/verbose/progress/stats. + ## rsync Daemon Mode `internal/daemon` implements grsync's `--daemon` server mode: a second way diff --git a/internal/cli/partial_append_test.go b/internal/cli/partial_append_test.go new file mode 100644 index 0000000..249d591 --- /dev/null +++ b/internal/cli/partial_append_test.go @@ -0,0 +1,144 @@ +package cli + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestE2E_AppendAndAppendVerifyMutuallyExclusive(t *testing.T) { + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "f.txt"), "content") + dst := t.TempDir() + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "--append", "--append-verify", src, dst}) + cmd.SetOut(io.Discard) + err := cmd.Execute() + if err == nil { + t.Fatal("grsync --append --append-verify returned nil error, want a mutually-exclusive error") + } + if !strings.Contains(err.Error(), "mutually exclusive") { + t.Errorf("error = %q, want it to mention --append/--append-verify being mutually exclusive", err.Error()) + } +} + +// TestE2E_AppendExtendsShorterDestinationFile drives the real CLI +// command with --append against a destination file that's a genuine +// prefix of the source, and confirms the result is correct - the real, +// end-to-end proof that --append's flag wiring (root.go through +// effectiveReceiverOptions) actually reaches Receiver. +func TestE2E_AppendExtendsShorterDestinationFile(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + prefix := "already present on disk " + full := prefix + "and now the new tail data too" + mustWriteFile(t, filepath.Join(src, "growing.log"), full) + mustWriteFile(t, filepath.Join(dst, "growing.log"), prefix) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "--append", src, dst}) + cmd.SetOut(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(dst, "growing.log")) + if err != nil { + t.Fatalf("reading result: %v", err) + } + if string(got) != full { + t.Errorf("result = %q, want %q", got, full) + } +} + +// TestE2E_AppendVerifyCorrectsWrongPrefix is --append-verify's own real, +// end-to-end correctness proof: a destination whose existing "prefix" is +// actually wrong must still end up fully correct, since --append-verify +// (unlike plain --append) actually checks it. +func TestE2E_AppendVerifyCorrectsWrongPrefix(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + full := strings.Repeat("A", 1400) + "the genuinely new tail" + wrongPrefix := strings.Repeat("Z", 1400) + mustWriteFile(t, filepath.Join(src, "file.txt"), full) + mustWriteFile(t, filepath.Join(dst, "file.txt"), wrongPrefix) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "--append-verify", src, dst}) + cmd.SetOut(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(dst, "file.txt")) + if err != nil { + t.Fatalf("reading result: %v", err) + } + if string(got) != full { + t.Errorf("result = %q, want %q (--append-verify must catch and correct the wrong prefix)", got, full) + } +} + +// TestE2E_PartialDirUsedForResume is --partial-dir's own real, +// end-to-end resume proof: a leftover partial file (as if left there by +// an earlier interrupted `grsync --partial-dir=...` run) is picked up +// and used, then cleaned up, via the real CLI command. +func TestE2E_PartialDirUsedForResume(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + full := strings.Repeat("resumable data ", 100) + mustWriteFile(t, filepath.Join(src, "big.txt"), full) + + partialPath := filepath.Join(dst, ".rsync-partial", "big.txt") + mustMkdirAll(t, filepath.Dir(partialPath)) + mustWriteFile(t, partialPath, full[:len(full)*3/4]) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "--partial-dir", ".rsync-partial", src, dst}) + cmd.SetOut(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(dst, "big.txt")) + if err != nil { + t.Fatalf("reading result: %v", err) + } + if string(got) != full { + t.Errorf("result differs from source (len got=%d, want=%d)", len(got), len(full)) + } + if _, statErr := os.Stat(partialPath); !os.IsNotExist(statErr) { + t.Errorf("partial-dir file %q still exists after a successful resumed transfer, want it removed", partialPath) + } +} + +// TestE2E_PartialFlagsDoNotBreakAnOrdinaryUninterruptedSync confirms +// --partial/--partial-dir/--append/--append-verify are all harmless +// no-ops for a completely ordinary, uninterrupted sync - they only ever +// change behavior around eligibility/interruption, never the happy path. +func TestE2E_PartialFlagsDoNotBreakAnOrdinaryUninterruptedSync(t *testing.T) { + src := t.TempDir() + dst := t.TempDir() + mustWriteFile(t, filepath.Join(src, "f.txt"), "ordinary content") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + + cmd := NewRootCmd() + cmd.SetArgs([]string{"-a", "--partial", "--partial-dir", ".rsync-partial", src, dst}) + cmd.SetOut(io.Discard) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(dst, "f.txt")) + if err != nil || string(got) != "ordinary content" { + t.Errorf("f.txt content = %q, err = %v, want %q", got, err, "ordinary content") + } + got2, err := os.ReadFile(filepath.Join(dst, "sub", "nested.txt")) + if err != nil || string(got2) != "nested content" { + t.Errorf("sub/nested.txt content = %q, err = %v, want %q", got2, err, "nested content") + } +} diff --git a/internal/cli/root.go b/internal/cli/root.go index a12efd0..e450b5c 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -69,6 +69,10 @@ type options struct { ipv4 bool ipv6 bool address string + partial bool + partialDir string + appendMode bool + appendVerify bool } // filterRuleFlag implements pflag.Value. Each of --exclude/--include/ @@ -220,6 +224,22 @@ func NewRootCmd() *cobra.Command { "local/source address of the outbound connection when dialing an rsync:// daemon; matches real "+ "rsync's own --address scope exactly - has no effect on the SSH transport or a local sync "+ "(see the README's IPv4/IPv6 Support section)") + flags.BoolVar(&opts.partial, "partial", false, + "keep a partially transferred file (instead of deleting it) if the transfer is interrupted before "+ + "that file completes, so a later run can resume from it - file granularity, not true mid-file "+ + "resumption (see the README's Partial and Append Transfers section for grsync's exact scope here)") + flags.StringVar(&opts.partialDir, "partial-dir", "", + "put a partially transferred file into DIR instead of leaving it at the destination path; implies "+ + "--partial, and a file found here is used to speed up a later resumed transfer, then removed once "+ + "it's no longer needed - matches real rsync's own --partial-dir") + flags.BoolVar(&opts.appendMode, "append", false, + "for a destination file shorter than the source, blindly trust the existing bytes (never verified) "+ + "and transfer only the new tail - dangerous if that assumption is wrong; see --append-verify and "+ + "the README's Partial and Append Transfers section. A destination that is not shorter than the "+ + "source is left untouched entirely; a destination that doesn't exist yet is transferred normally") + flags.BoolVar(&opts.appendVerify, "append-verify", false, + "like --append, but verifies the existing prefix against the source instead of blindly trusting it "+ + "(safer, at the cost of re-comparing that data); mutually exclusive with --append") return cmd } diff --git a/internal/cli/sync.go b/internal/cli/sync.go index 2e3faba..66c68ce 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -54,12 +54,16 @@ func effectiveAttrOptions(opts *options) sync.AttrOptions { // reported to output. func effectiveReceiverOptions(opts *options, output io.Writer) pipeline.ReceiverOptions { return pipeline.ReceiverOptions{ - DryRun: opts.dryRun, - Itemize: opts.itemize, - Verbose: opts.verbose, - Progress: opts.progress, - Stats: opts.stats, - Output: output, + DryRun: opts.dryRun, + Itemize: opts.itemize, + Verbose: opts.verbose, + Progress: opts.progress, + Stats: opts.stats, + Output: output, + Partial: opts.partial, + PartialDir: opts.partialDir, + Append: opts.appendMode, + AppendVerify: opts.appendVerify, } } @@ -163,6 +167,15 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt return err } + // --append/--append-verify are two different behaviors for the same + // underlying idea (see the README's Partial and Append Transfers + // section for the real, verified-against-source distinction) - like + // --ipv4/--ipv6 above, rejecting the combination outright is clearer + // than silently preferring one. + if opts.appendMode && opts.appendVerify { + return fmt.Errorf("--append and --append-verify are mutually exclusive") + } + // isRsyncURL is checked, and rsyncURL parsed, before // transport.ParseRemotePath ever looks at destination: an rsync:// // URL is never valid [user@]host:path syntax (ParseRemotePath itself @@ -213,6 +226,21 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --itemize-changes/--verbose/--progress/--stats output is not available "+ "for an rsync:// daemon destination (the daemon protocol has no channel for it); --dry-run's no-write guarantee still applies") } + if isRsyncDaemon && (ropts.KeepPartial() || ropts.AppendMode()) { + // A different flavor of the same underlying limitation: for an + // rsync:// daemon upload, the module's Receiver runs on the + // server (see daemon.ServeModule's own DirectionPut comment), not + // here, and syncToRsyncDaemon only ever forwards DryRun to it (via + // daemon.dryRunToken) - not the rest of ReceiverOptions. Extending + // that wire protocol to also carry Partial/PartialDir/Append/ + // AppendVerify is real, separate protocol work outside SC-12's own + // scope (it depends on SC-17's daemon-CLI wiring, not the other + // way around), so this direction silently ignores all four rather + // than honoring them - noting that plainly, once, rather than + // letting the user assume they took effect. + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "note: --partial/--partial-dir/--append/--append-verify are not available "+ + "for an rsync:// daemon destination (the module's receiver runs on the server, which has no way to learn these were requested)") + } for _, src := range sources { switch { @@ -275,8 +303,11 @@ func syncLocal(src, dest string, walkOpts sync.WalkOptions, rules []sync.Rule, a // remote side, exactly where pipeline.Receiver actually runs for this // transport - there is nothing for the local, sending side to decide // here at all. This is the same mechanism SC-11 established for -// DryRun/Itemize/Verbose; Progress/Stats just reuse it rather than -// inventing a second one. +// DryRun/Itemize/Verbose; Progress/Stats, and now Partial/PartialDir/ +// Append/AppendVerify, just reuse it rather than inventing a second +// one - the remote --server process's own Receiver call decides +// everything about temp files/resumption/append locally, from its own +// argv, exactly like it already decides DryRun. // // copts needs none of that: --compress/-z is a Sender-side decision (see // pipeline.CompressOptions' own doc comment), and Sender runs right here, @@ -313,6 +344,23 @@ func syncToRemote(rsh, src string, remote transport.RemotePath, walkOpts sync.Wa if ropts.Stats { remoteArgs = append(remoteArgs, "--stats") } + if ropts.PartialDir != "" { + // --partial-dir implies --partial (see ReceiverOptions.KeepPartial's + // own doc comment), so there's no need to also forward a bare + // --partial alongside it - one less argv token, and it avoids ever + // sending a value that could look like two separate flags if it + // happened to be forwarded as "--partial-dir" "value" instead of + // this single "--partial-dir=value" token. + remoteArgs = append(remoteArgs, "--partial-dir="+ropts.PartialDir) + } else if ropts.Partial { + remoteArgs = append(remoteArgs, "--partial") + } + if ropts.Append { + remoteArgs = append(remoteArgs, "--append") + } + if ropts.AppendVerify { + remoteArgs = append(remoteArgs, "--append-verify") + } remoteArgs = append(remoteArgs, remote.Path) session, err := transport.Dial(rsh, remote.User, remote.Host, remoteArgs, ipv4, ipv6) diff --git a/internal/pipeline/append_test.go b/internal/pipeline/append_test.go new file mode 100644 index 0000000..54fec4c --- /dev/null +++ b/internal/pipeline/append_test.go @@ -0,0 +1,284 @@ +package pipeline + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +func opsEqual(t *testing.T, got []sync.DeltaOp, want []sync.DeltaOp) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("ops = %+v, want %+v", got, want) + } + for i := range want { + switch w := want[i].(type) { + case sync.CopyOp: + g, ok := got[i].(sync.CopyOp) + if !ok || g != w { + t.Errorf("op %d = %+v, want %+v", i, got[i], w) + } + case sync.DataOp: + g, ok := got[i].(sync.DataOp) + if !ok || !bytes.Equal(g.Bytes, w.Bytes) { + t.Errorf("op %d = %+v, want %+v", i, got[i], w) + } + } + } +} + +func TestAppendTailOps_TrustsPrefixAndSendsOnlyTail(t *testing.T) { + ops, err := appendTailOps(5, []byte("helloWORLD")) + if err != nil { + t.Fatalf("appendTailOps returned error: %v", err) + } + opsEqual(t, ops, []sync.DeltaOp{sync.CopyOp{BlockIndex: 0}, sync.DataOp{Bytes: []byte("WORLD")}}) +} + +func TestAppendTailOps_ZeroTrustedLenOmitsCopyOp(t *testing.T) { + ops, err := appendTailOps(0, []byte("all new")) + if err != nil { + t.Fatalf("appendTailOps returned error: %v", err) + } + opsEqual(t, ops, []sync.DeltaOp{sync.DataOp{Bytes: []byte("all new")}}) +} + +func TestAppendTailOps_ExactMatchOmitsDataOp(t *testing.T) { + ops, err := appendTailOps(5, []byte("hello")) + if err != nil { + t.Fatalf("appendTailOps returned error: %v", err) + } + opsEqual(t, ops, []sync.DeltaOp{sync.CopyOp{BlockIndex: 0}}) +} + +// TestAppendTailOps_DiminishedFileReturnsError is the "diminished file" +// race real rsync itself warns about (source shrank below what the +// receiver already trusted) - grsync treats it as a hard error rather +// than real rsync's own skip-with-warning, a disclosed, deliberate +// simplification (see appendTailOps' own doc comment). +func TestAppendTailOps_DiminishedFileReturnsError(t *testing.T) { + if _, err := appendTailOps(100, []byte("short")); err == nil { + t.Error("appendTailOps with a source shorter than the trusted length returned nil error, want an error") + } +} + +// TestReceiver_AppendTransfersOnlyNewTail is --append's core wire-level +// proof: a destination file that's a genuine prefix of the source must +// end up correct, while sending meaningfully fewer bytes than a full +// re-transfer would need - proving the existing prefix was never +// resent as literal data, not just that the final content happens to be +// right. +func TestReceiver_AppendTransfersOnlyNewTail(t *testing.T) { + prefix := strings.Repeat("already on disk, must not be resent ", 100) + tail := strings.Repeat("brand new tail data ", 100) + full := prefix + tail + + appendSrc, appendDest := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(appendSrc, "growing.log"), full) + mustWriteFile(t, filepath.Join(appendDest, "growing.log"), prefix) + appendBytes := runSenderReceiverWithCompressOptions(t, appendSrc, appendDest, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, + ReceiverOptions{Append: true}, CompressOptions{}) + + assertSameContent(t, filepath.Join(appendSrc, "growing.log"), filepath.Join(appendDest, "growing.log")) + + // Baseline: the exact same transfer with no append mode at all, which + // still needs to run the full weak/strong rolling-checksum block scan + // (real, genuine work, not literal retransmission) - --append is + // specifically about skipping the SIGNATURE exchange for the prefix + // entirely (see sender.go's own appendTailOps), not just about the + // resulting delta happening to be efficient, so this baseline mainly + // confirms append mode isn't somehow *more* expensive; the literal-data + // byte count comparison below is append's real, direct proof. + normalSrc, normalDest := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(normalSrc, "growing.log"), full) + mustWriteFile(t, filepath.Join(normalDest, "growing.log"), prefix) + normalBytes := runSenderReceiverWithCompressOptions(t, normalSrc, normalDest, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{}, CompressOptions{}) + + if appendBytes >= normalBytes { + t.Errorf("--append wrote %d bytes, normal delta-transfer wrote %d bytes, want --append no larger", appendBytes, normalBytes) + } + + // The direct proof: --stats' own "Literal data" field for the append + // run must equal exactly len(tail) - if the prefix had been resent as + // literal data too, this would be much larger. + var out bytes.Buffer + statsSrc, statsDest := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(statsSrc, "growing.log"), full) + mustWriteFile(t, filepath.Join(statsDest, "growing.log"), prefix) + runSenderReceiverWithCompressAndReceiverOptions(t, statsSrc, statsDest, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, + ReceiverOptions{Append: true, Stats: true, Output: &out}, CompressOptions{}) + if got := statsField(t, out.String(), "Literal data"); got != int64(len(tail)) { + t.Errorf("Literal data = %d, want exactly %d (len(tail)) - the existing prefix must never be counted as literal", got, len(tail)) + } +} + +// TestReceiver_AppendDoesNotVerifyCorruptedPrefix is the self-review's +// own explicit ask made concrete: --append's documented real-rsync risk +// ("can be dangerous if you aren't 100% sure... existing content...is +// also known to be the same") is reproduced faithfully here, not +// silently made safer or worse - a WRONG existing prefix is blindly +// trusted and the final file ends up with that wrong prefix intact, not +// silently corrected and not aborted with an error either. +func TestReceiver_AppendDoesNotVerifyCorruptedPrefix(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + realContent := "AAAAAAAAAA" + "tail data that gets appended" + corruptedExisting := "XXXXXXXXXX" // same length as the real prefix, deliberately wrong content + + mustWriteFile(t, filepath.Join(srcRoot, "file.txt"), realContent) + mustWriteFile(t, filepath.Join(destRoot, "file.txt"), corruptedExisting) + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{Append: true}) + + got, err := os.ReadFile(filepath.Join(destRoot, "file.txt")) + if err != nil { + t.Fatalf("reading result: %v", err) + } + want := corruptedExisting + "tail data that gets appended" + if string(got) != want { + t.Errorf("result = %q, want %q (the corrupted prefix preserved verbatim, matching real rsync's own documented --append risk)", got, want) + } +} + +// TestReceiver_AppendVerifyDetectsCorruptedPrefix is +// TestReceiver_AppendDoesNotVerifyCorruptedPrefix's --append-verify +// counterpart: the exact same corrupted-prefix scenario must instead +// produce a fully correct result, proving verification actually caught +// and fixed the mismatch rather than blindly trusting it. +func TestReceiver_AppendVerifyDetectsCorruptedPrefix(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + realContent := strings.Repeat("A", 1400) + "tail data that gets appended, well past one block" + corruptedExisting := strings.Repeat("X", 1400) // same length, deliberately wrong + + mustWriteFile(t, filepath.Join(srcRoot, "file.txt"), realContent) + mustWriteFile(t, filepath.Join(destRoot, "file.txt"), corruptedExisting) + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{AppendVerify: true}) + + assertSameContent(t, filepath.Join(srcRoot, "file.txt"), filepath.Join(destRoot, "file.txt")) +} + +// TestReceiver_AppendSkipsDestinationNotShorterThanSource is real +// rsync's own documented eligibility rule made concrete: a destination +// that's already at least as long as the source must be left completely +// untouched, even though its content genuinely differs from the +// source's - --append/--append-verify unconditionally skip such files +// rather than comparing and possibly updating them. +func TestReceiver_AppendSkipsDestinationNotShorterThanSource(t *testing.T) { + for _, ropts := range []ReceiverOptions{{Append: true}, {AppendVerify: true}} { + srcRoot, destRoot := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(srcRoot, "file.txt"), "short") + mustWriteFile(t, filepath.Join(destRoot, "file.txt"), "this destination is already longer than source") + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ropts) + + got, err := os.ReadFile(filepath.Join(destRoot, "file.txt")) + if err != nil { + t.Fatalf("ropts=%+v: reading destination: %v", ropts, err) + } + if string(got) != "this destination is already longer than source" { + t.Errorf("ropts=%+v: destination content = %q, want it left completely untouched", ropts, got) + } + } +} + +// TestReceiver_AppendTransfersBrandNewFileNormally confirms real +// rsync's own documented "new files are transferred" rule: append +// semantics only ever apply to an EXISTING, shorter destination file - +// a file that doesn't exist yet at the destination is transferred +// completely normally under --append/--append-verify, not skipped and +// not specially handled. +func TestReceiver_AppendTransfersBrandNewFileNormally(t *testing.T) { + for _, ropts := range []ReceiverOptions{{Append: true}, {AppendVerify: true}} { + srcRoot, destRoot := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(srcRoot, "new.txt"), "brand new content") + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ropts) + + assertSameContent(t, filepath.Join(srcRoot, "new.txt"), filepath.Join(destRoot, "new.txt")) + } +} + +// TestReceiver_AppendWorksWithDryRun confirms Step 4's dry-run +// requirement: the append-aware signature/delta exchange still runs (for +// accurate itemize planning), but no file is created or modified. +func TestReceiver_AppendWorksWithDryRun(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(srcRoot, "growing.log"), "prefixTAIL") + mustWriteFile(t, filepath.Join(destRoot, "growing.log"), "prefix") + + var out bytes.Buffer + runSenderReceiverWithCompressAndReceiverOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, + ReceiverOptions{Append: true, DryRun: true, Itemize: true, Output: &out}, CompressOptions{}) + + if !strings.Contains(out.String(), "growing.log") { + t.Errorf("dry-run itemize output = %q, want it to mention growing.log", out.String()) + } + got, err := os.ReadFile(filepath.Join(destRoot, "growing.log")) + if err != nil { + t.Fatalf("reading destination: %v", err) + } + if string(got) != "prefix" { + t.Errorf("destination content = %q after a dry run, want it unchanged (\"prefix\")", got) + } + + entries, err := os.ReadDir(destRoot) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.Contains(e.Name(), ".grsync-tmp") { + t.Errorf("a temp file %q was created during a dry run, want none at all", e.Name()) + } + } +} + +// TestReceiver_AppendExcludesHardLinkSecondaryMembers locks in Step 4's +// hard-link requirement: a hard-link group's secondary member never goes +// through the signature/delta exchange at all (it's linked directly from +// the group's first member instead - see Receiver's own hard-link pass), +// so --append is structurally moot for it, not specially excluded by any +// append-specific code. This test's real job is proving that structural +// fact still holds with --append enabled, not just documenting it. +func TestReceiver_AppendExcludesHardLinkSecondaryMembers(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + + content := "shared content for both hard-linked files" + mustWriteFile(t, filepath.Join(srcRoot, "original.txt"), content) + if err := os.Link(filepath.Join(srcRoot, "original.txt"), filepath.Join(srcRoot, "linked.txt")); err != nil { + t.Skipf("hard link creation unsupported in this environment: %v", err) + } + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{HardLinks: true}, + ReceiverOptions{Append: true}) + + assertSameContent(t, filepath.Join(srcRoot, "original.txt"), filepath.Join(destRoot, "original.txt")) + assertSameContent(t, filepath.Join(srcRoot, "linked.txt"), filepath.Join(destRoot, "linked.txt")) + + if !sync.HardLinksSupported() { + return + } + originalInfo, err := os.Stat(filepath.Join(destRoot, "original.txt")) + if err != nil { + t.Fatalf("Stat: %v", err) + } + linkedInfo, err := os.Stat(filepath.Join(destRoot, "linked.txt")) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if !os.SameFile(originalInfo, linkedInfo) { + t.Error("original.txt and linked.txt are independent files at the destination with --append enabled, want them still hard-linked") + } +} diff --git a/internal/pipeline/itemize.go b/internal/pipeline/itemize.go index dbcbd80..1049519 100644 --- a/internal/pipeline/itemize.go +++ b/internal/pipeline/itemize.go @@ -41,6 +41,53 @@ type ReceiverOptions struct { // A nil Output is treated as io.Discard, so a caller that wants none // of this doesn't need to construct a discard writer itself. Output io.Writer + + // Partial, when true, keeps a regular file's temp file (rather than + // deleting it) if the transfer aborts before that file's rename into + // place - see partial.go's own doc comment for exactly what "partial" + // means in grsync's frame-per-file architecture (file granularity, + // not true mid-file resumption). Implied by a non-empty PartialDir, + // matching real rsync's own documented "--partial-dir... also + // implying that [--partial] be enabled." + Partial bool + // PartialDir, when non-empty, is where an abandoned temp file goes + // instead of being renamed onto the destination path directly - see + // partial.go's partialFilePath for the relative-vs-absolute placement + // rule. A file found here is also used as a resume basis (the + // signature comparison source) on a later run, then deleted once its + // transfer completes successfully. + PartialDir string + // Append, when true, blindly trusts a shorter destination file's + // existing bytes and transfers only the new tail - see + // messages.go's appendTail and receiver.go's own doc comment for the + // real, verified-against-source distinction from AppendVerify. + // Mutually exclusive with AppendVerify (internal/cli validates this). + Append bool + // AppendVerify is like Append, but runs the completely normal + // signature/delta comparison over the existing prefix instead of + // trusting it blindly - see receiver.go's own doc comment for why + // this needs no new algorithm at all, just an eligibility gate on + // top of the pre-existing flow. + AppendVerify bool +} + +// AppendMode reports whether either append flag is set - Append and +// AppendVerify share the same file-eligibility rules (see receiver.go), +// differing only in whether the existing prefix is trusted or verified. +// Exported since internal/cli needs it too, to warn when combined with +// an rsync:// daemon upload destination (see runSync's own comment on +// why that direction can't honor either flag, the same disclosed +// daemon-PUT limitation SC-10/SC-11 already established for reporting). +func (o ReceiverOptions) AppendMode() bool { + return o.Append || o.AppendVerify +} + +// KeepPartial reports whether an aborted temp file should be kept at +// all - PartialDir implies Partial, matching real rsync's own +// documented "--partial-dir... also implying that [--partial] be +// enabled." Exported for the same reason AppendMode is. +func (o ReceiverOptions) KeepPartial() bool { + return o.Partial || o.PartialDir != "" } func (o ReceiverOptions) output() io.Writer { diff --git a/internal/pipeline/messages.go b/internal/pipeline/messages.go index f87aa17..a0eda2b 100644 --- a/internal/pipeline/messages.go +++ b/internal/pipeline/messages.go @@ -133,11 +133,41 @@ func fromWireDeltaOps(wire []wireDeltaOp, compressed bool, literal []byte) ([]sy // against a class of bug (an off-by-one, a dropped frame) that // position-only encoding could never detect and would silently // misapply one file's delta to another. +// +// Append (SC-12's own contribution) tells Sender how to respond to this +// signature - see appendAction's own doc comment for the three +// possibilities. It defaults to appendNone (gob's zero value), so every +// signatureMessage sent before --append/--append-verify existed decodes +// exactly as it always did. type signatureMessage struct { - Path string - Sig sync.Signature + Path string + Sig sync.Signature + Append appendAction } +// appendAction is carried on a signatureMessage to tell Sender how to +// respond to it - see receiver.go's own doc comment on where each value +// gets chosen, and sender.go's own doc comment on how each is handled. +type appendAction byte + +const ( + // appendNone is the normal, pre-SC-12 flow: Sender runs + // sync.GenerateDelta against Sig exactly as it always has. + appendNone appendAction = iota + // appendTail means "the receiver's existing Sig.BlockSize bytes are + // blindly trusted, unverified - send only the literal tail past + // that offset" (--append). Sig.BlockSize carries that trusted + // offset (not a real block size at all here); Sig.Blocks is unused. + // See receiver.go for exactly when this is chosen and sender.go for + // how it's handled. + appendTail + // appendSkip means "the destination is already at least as long as + // the source - don't read or compare anything at all, just + // acknowledge with an empty delta" (the --append/--append-verify + // "not shorter, skip entirely" eligibility rule). + appendSkip +) + // deltaMessage is FrameDelta's payload: one regular file's delta ops, // tagged with its Path for the same reason as signatureMessage. // @@ -225,8 +255,8 @@ func recvFileList(r io.Reader) ([]sync.FileEntry, []sync.HardLinkGroup, error) { return msg.Entries, msg.HardLinkGroups, nil } -func sendSignature(w io.Writer, path string, sig sync.Signature) error { - payload, err := encodeGob(signatureMessage{Path: path, Sig: sig}) +func sendSignature(w io.Writer, path string, sig sync.Signature, action appendAction) error { + payload, err := encodeGob(signatureMessage{Path: path, Sig: sig, Append: action}) if err != nil { return fmt.Errorf("encoding signature for %q: %w", path, err) } diff --git a/internal/pipeline/messages_test.go b/internal/pipeline/messages_test.go index 6454f47..f75ba89 100644 --- a/internal/pipeline/messages_test.go +++ b/internal/pipeline/messages_test.go @@ -82,7 +82,7 @@ func TestSignatureRoundTrip(t *testing.T) { sig := sync.GenerateSignatureWithBlockSize([]byte("AAAABBBBCCCC"), 4) var buf bytes.Buffer - if err := sendSignature(&buf, "some/file.txt", sig); err != nil { + if err := sendSignature(&buf, "some/file.txt", sig, appendNone); err != nil { t.Fatalf("sendSignature returned error: %v", err) } got, err := recvSignature(&buf) diff --git a/internal/pipeline/partial.go b/internal/pipeline/partial.go new file mode 100644 index 0000000..a33fef1 --- /dev/null +++ b/internal/pipeline/partial.go @@ -0,0 +1,207 @@ +// partial.go implements --partial/--partial-dir. +// +// grsync's wire protocol has no streaming I/O (see sync.ApplyDelta's own +// doc comment): one regular file's delta arrives as a single, atomic +// gob-encoded frame, fully decoded into memory before a single byte of +// it is written to disk. That means there is no such thing as "half of +// this file's delta arrived" - a dropped connection mid-frame just means +// this file's transfer never started at all, and everything already +// written for *earlier* files in the same sync stays exactly as +// complete as it already was. --partial in grsync is therefore +// file-granularity, not real rsync's true byte-level mid-file +// resumption: "partial" describes which whole files survive an aborted +// multi-file sync, not a partially-written single file left in a +// half-complete state. +// +// That distinction only holds, though, if writing a file's new content +// is itself all-or-nothing from the destination's point of view - and +// before this ticket, it wasn't: receiveRegularFile wrote straight to +// destPath (os.WriteFile / a chunked os.OpenFile+Write loop for +// progress reporting), so a process killed mid-write left a genuinely +// truncated, corrupted file sitting at the real destination path, with +// no flag able to prevent or recover from it. Implementing --partial +// correctly requires the prerequisite real rsync itself always has: a +// separate temp file, written first, then atomically renamed into +// place only once it's complete. That temp-file+rename path is now +// unconditional (see receiver.go), not something --partial turns on - +// --partial/--partial-dir only control what happens to that temp file +// if the transfer aborts before the rename. + +package pipeline + +import ( + "fmt" + "os" + "path/filepath" +) + +// createTempFile creates a new, uniquely-named temp file in the same +// directory as destPath - same-directory, not a system temp dir, so the +// eventual rename into place is same-filesystem (and therefore atomic on +// every platform this project supports). The returned file's mode is +// explicitly set to 0644 (best-effort - see the inline comment) to match +// what a file written without --perms has always ended up as +// (os.WriteFile's own default), since os.CreateTemp's own default of +// 0600 would otherwise quietly become the new no-perms default the +// moment nothing later overrides it via sync.ApplyAttributes. +func createTempFile(destPath string) (*os.File, error) { + dir := filepath.Dir(destPath) + base := filepath.Base(destPath) + f, err := os.CreateTemp(dir, "."+base+".*.grsync-tmp") + if err != nil { + return nil, err + } + // Best-effort: a handful of exotic filesystems don't support + // changing an open file's mode the same way a normal POSIX one + // does. Getting this wrong only matters when --perms is NOT given + // (sync.ApplyAttributes overwrites it correctly whenever --perms + // IS given, after the rename below), so a failure here is not worth + // aborting the whole transfer over. + _ = f.Chmod(0o644) + return f, nil +} + +// writeToTempFileWithProgress is writeFileWithProgress's SC-12 +// counterpart: identical chunking/progress-reporting behavior (see its +// own doc comment for why nil-progress and small files both skip +// chunking), but targets a fresh temp file next to destPath instead of +// writing destPath directly. tmpPath is always returned, even when err +// != nil, so the caller can still apply Partial/PartialDir policy to +// however much was actually written before the failure. +func writeToTempFileWithProgress(destPath string, data []byte, progress *progressReporter, path string, xferNum, totalFiles, filesLeft int) (tmpPath string, err error) { + f, err := createTempFile(destPath) + if err != nil { + return "", err + } + tmpPath = f.Name() + defer func() { _ = f.Close() }() + + if progress == nil || len(data) <= progressWriteChunkSize { + if _, err := f.Write(data); err != nil { + return tmpPath, err + } + if progress != nil { + progress.report(progressUpdate{ + path: path, bytesDone: int64(len(data)), fileSize: int64(len(data)), done: true, + xferNum: xferNum, totalFiles: totalFiles, filesLeft: filesLeft, + }) + } + return tmpPath, nil + } + + total := int64(len(data)) + var written int64 + for written < total { + end := written + progressWriteChunkSize + if end > total { + end = total + } + n, werr := f.Write(data[written:end]) + written += int64(n) + if werr != nil { + return tmpPath, werr + } + progress.report(progressUpdate{ + path: path, bytesDone: written, fileSize: total, done: written == total, + xferNum: xferNum, totalFiles: totalFiles, filesLeft: filesLeft, + }) + } + return tmpPath, nil +} + +// partialFilePath computes where relPath's partial file lives under +// ropts.PartialDir, mirroring real rsync's own documented placement +// rule: a relative PartialDir is created inside *each file's own +// destination directory* ("this makes it easy to use a relative path... +// to have rsync create the partial-directory in the destination file's +// directory"), so files with the same basename in different +// subdirectories can never collide with each other. An absolute +// PartialDir is a single shared directory, so relPath's full relative +// path is mirrored underneath it instead - real rsync's docs don't spell +// out this exact collision-avoidance scheme for the absolute case, but +// mirroring the relative path is the only scheme that can't collide +// between two files sharing a basename in different subdirectories. +func partialFilePath(destPath, partialDir, relPath string) string { + if filepath.IsAbs(partialDir) { + return filepath.Join(partialDir, filepath.FromSlash(relPath)) + } + return filepath.Join(filepath.Dir(destPath), partialDir, filepath.Base(destPath)) +} + +// loadPartialBasis returns the content of relPath's partial file, if +// ropts.PartialDir is set and a usable file is actually there - used in +// place of the real destination file's own content as the delta +// comparison basis, giving a resumed transfer real content-level +// speedup (see delta.go's own algorithm: a signature built from a +// partial file's already-correct prefix naturally yields CopyOps for +// whatever still matches and DataOps only for the genuinely new tail). +// ok is false whenever there's nothing usable to resume from - PartialDir +// unset, no file there, or an unreadable one - in which case the caller +// falls back to the real destination file exactly as it always has. +func loadPartialBasis(destPath string, ropts ReceiverOptions, relPath string) (data []byte, ok bool) { + if ropts.PartialDir == "" { + return nil, false + } + data, err := os.ReadFile(partialFilePath(destPath, ropts.PartialDir, relPath)) + if err != nil { + return nil, false + } + return data, true +} + +// finishRegularFileWrite is receiveRegularFile's single exit point for +// everything written by writeToTempFileWithProgress: on success +// (writeErr == nil), it commits by renaming tmpPath onto destPath and, +// if a partial-dir file was used as this transfer's basis, removes it +// (real rsync's own documented "delete it after it has served its +// purpose"). On any failure - the write itself, or the commit rename - +// it hands off to abandonOrKeep to apply Partial/PartialDir policy, then +// returns the ORIGINAL error (a failure to honor Partial is never more +// important than surfacing why the transfer actually failed). +func finishRegularFileWrite(tmpPath, destPath string, writeErr error, ropts ReceiverOptions, relPath string, usedPartialBasis bool) error { + if writeErr == nil { + if err := os.Rename(tmpPath, destPath); err != nil { + return abandonOrKeep(tmpPath, destPath, ropts, relPath, fmt.Errorf("renaming into place: %w", err)) + } + if usedPartialBasis { + _ = os.Remove(partialFilePath(destPath, ropts.PartialDir, relPath)) // best-effort: a stale leftover here is harmless, just wasted disk space + } + return nil + } + return abandonOrKeep(tmpPath, destPath, ropts, relPath, writeErr) +} + +// abandonOrKeep applies Partial/PartialDir policy to a temp file whose +// transfer did not complete successfully, then returns origErr +// unchanged so the caller's own error still propagates. Every exit path +// here ends with tmpPath gone from its original same-directory-as-dest +// location - either deleted outright, or moved somewhere else entirely +// (partial-dir or destPath itself) - so a temp file this function has +// handled is never left sitting next to the destination under its own +// ".name.RANDOM.grsync-tmp" name, regardless of which policy applies. +// +// This is also the self-review's own "could a partial file leak outside +// --partial-dir" answer made concrete: whenever ropts.PartialDir is set, +// the only two destinations tmpPath can end up at are partialFilePath's +// own return value or deletion - destPath itself is never touched by +// this function in that case, so a --partial-dir user's real destination +// tree is never modified by an aborted transfer at all. +func abandonOrKeep(tmpPath, destPath string, ropts ReceiverOptions, relPath string, origErr error) error { + if !ropts.KeepPartial() { + _ = os.Remove(tmpPath) + return origErr + } + + target := destPath + if ropts.PartialDir != "" { + target = partialFilePath(destPath, ropts.PartialDir, relPath) + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + _ = os.Remove(tmpPath) + return origErr + } + } + if err := os.Rename(tmpPath, target); err != nil { + _ = os.Remove(tmpPath) // never leave a stray temp file behind just because the "keep" step itself also failed + } + return origErr +} diff --git a/internal/pipeline/partial_integration_test.go b/internal/pipeline/partial_integration_test.go new file mode 100644 index 0000000..2c81892 --- /dev/null +++ b/internal/pipeline/partial_integration_test.go @@ -0,0 +1,240 @@ +package pipeline + +import ( + "bytes" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +// TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial +// is SC-12's own core "interrupted transfer" proof, matching the +// existing TestReceiver_ConnectionDropsMidTransfer/ +// TestReceiver_AppliesHardLinksFromReceivedGroups pattern of driving +// Receiver against a hand-built peer goroutine for exact control over +// when the connection dies. It drives home Step 3's own architectural +// finding: grsync's "partial" is file-granularity, not true mid-file +// resumption, so completed files survive a drop *regardless* of +// --partial, and the in-flight file at the moment of the drop is simply +// never attempted at all (absent, not corrupted, not partially written) - +// because its signature/delta exchange never got far enough to produce +// any bytes to write in the first place. This test runs the identical +// scenario with and without ReceiverOptions.Partial to prove that flag +// genuinely makes no difference to this particular failure mode - the +// flag's real, distinguishing effect (see partial_test.go's own +// abandonOrKeep tests) only matters for a failure *during* the local +// write phase, not a dropped connection between files. +func TestReceiver_InterruptedMultiFileSyncKeepsCompletedFilesRegardlessOfPartial(t *testing.T) { + for _, ropts := range []ReceiverOptions{{}, {Partial: true}, {PartialDir: ".rsync-partial"}} { + t.Run(fmt.Sprintf("%+v", ropts), func(t *testing.T) { + destRoot := t.TempDir() + + peerReadsFromReceiver, receiverWritesToPeer := io.Pipe() + receiverReadsFromPeer, peerWritesToReceiver := io.Pipe() + receiver := pipeReadWriter{Reader: receiverReadsFromPeer, Writer: receiverWritesToPeer} + + entries := []sync.FileEntry{ + {Path: "a.txt", Mode: 0o644, Size: 5}, + {Path: "b.txt", Mode: 0o644, Size: 5}, + {Path: "c.txt", Mode: 0o644, Size: 5}, // the connection dies before this one's delta ever arrives + } + + peerErrCh := make(chan error, 1) + go func() { + if err := sendFileList(peerWritesToReceiver, entries, nil); err != nil { + peerErrCh <- fmt.Errorf("sending file list: %w", err) + return + } + + for _, name := range []string{"a.txt", "b.txt"} { + sigMsg, err := recvSignature(peerReadsFromReceiver) + if err != nil { + peerErrCh <- fmt.Errorf("receiving signature for %s: %w", name, err) + return + } + if sigMsg.Path != name { + peerErrCh <- fmt.Errorf("signature requested for %q, want %q", sigMsg.Path, name) + return + } + ops := []sync.DeltaOp{sync.DataOp{Bytes: []byte("hello")}} + if err := sendDelta(peerWritesToReceiver, name, ops, CompressOptions{}); err != nil { + peerErrCh <- fmt.Errorf("sending delta for %s: %w", name, err) + return + } + } + + // Receive the signature request for c.txt (proving Receiver + // got that far), then vanish without ever responding - + // simulating a connection that drops mid-transfer, the same + // way TestReceiver_ConnectionDropsMidTransfer does for a + // single file. + if _, err := recvSignature(peerReadsFromReceiver); err != nil { + peerErrCh <- fmt.Errorf("receiving signature for c.txt: %w", err) + return + } + _ = peerWritesToReceiver.Close() + _ = peerReadsFromReceiver.Close() + peerErrCh <- nil + }() + + err := Receiver(receiver, destRoot, sync.AttrOptions{}, ropts) + if err == nil { + t.Fatal("Receiver returned nil error after the connection dropped mid-transfer, want an error") + } + + for _, name := range []string{"a.txt", "b.txt"} { + got, rerr := os.ReadFile(filepath.Join(destRoot, name)) + if rerr != nil { + t.Errorf("%s: reading result: %v", name, rerr) + continue + } + if string(got) != "hello" { + t.Errorf("%s content = %q, want %q (files completed before the drop must survive it intact)", name, got, "hello") + } + } + + // c.txt's transfer never got far enough to produce any bytes to + // write at all - it must be completely absent, not a + // zero-length or truncated file, and not present in a partial + // location either, regardless of ropts. + if _, statErr := os.Stat(filepath.Join(destRoot, "c.txt")); !os.IsNotExist(statErr) { + t.Errorf("c.txt exists after the connection dropped before its own transfer began, want it completely absent") + } + if ropts.PartialDir != "" { + if _, statErr := os.Stat(filepath.Join(destRoot, ropts.PartialDir, "c.txt")); !os.IsNotExist(statErr) { + t.Errorf("c.txt exists in PartialDir despite never having anything written for it, want it absent there too") + } + } + + // No stray temp file anywhere in destRoot either. + _ = filepath.WalkDir(destRoot, func(path string, d os.DirEntry, err error) error { + if err == nil && !d.IsDir() && strings.Contains(d.Name(), ".grsync-tmp") { + t.Errorf("stray temp file %q left behind after the interruption", path) + } + return nil + }) + + if peerErr := <-peerErrCh; peerErr != nil { + t.Fatalf("peer goroutine: %v", peerErr) + } + }) + } +} + +// TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry is "the pipeline +// can be told to resume," made concrete and measured, not just asserted: +// a leftover partial-dir file representing a genuine (correct) prefix of +// the real source content is picked up as the delta comparison basis on +// the next run - sync.GenerateDelta naturally turns that into CopyOps for +// the matching prefix and DataOps only for the new tail (see +// loadPartialBasis's own doc comment) - producing a real, measurable +// reduction in wire bytes compared to a from-scratch run with no +// resume basis available, and leaving the destination byte-for-byte +// correct either way. The partial-dir file is also removed once it's +// served its purpose, matching real rsync's own documented behavior. +func TestSenderReceiver_PartialDirUsedAsResumeBasisOnRetry(t *testing.T) { + fullContent := strings.Repeat("resumable content chunk, well over one block size ", 60) + partialDir := ".rsync-partial" + + resumedSrc, resumedDest := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(resumedSrc, "big.txt"), fullContent) + // A genuine prefix of the real content, as if an earlier run had + // gotten this far before being interrupted. + prefix := fullContent[:len(fullContent)*3/4] + partialPath := filepath.Join(resumedDest, partialDir, "big.txt") + mustMkdirAll(t, filepath.Dir(partialPath)) + mustWriteFile(t, partialPath, prefix) + + resumedBytes := runSenderReceiverWithCompressOptions(t, resumedSrc, resumedDest, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, + ReceiverOptions{PartialDir: partialDir}, CompressOptions{}) + + assertSameContent(t, filepath.Join(resumedSrc, "big.txt"), filepath.Join(resumedDest, "big.txt")) + if _, statErr := os.Stat(partialPath); !os.IsNotExist(statErr) { + t.Errorf("partial-dir file %q still exists after a successful resumed transfer, want it removed", partialPath) + } + + // Baseline: the identical transfer with no partial-dir file to resume + // from at all (a completely fresh destination). + freshSrc, freshDest := t.TempDir(), t.TempDir() + mustWriteFile(t, filepath.Join(freshSrc, "big.txt"), fullContent) + freshBytes := runSenderReceiverWithCompressOptions(t, freshSrc, freshDest, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{}, CompressOptions{}) + + if resumedBytes >= freshBytes { + t.Errorf("resumed transfer (with a partial-dir basis) wrote %d bytes, fresh transfer (no basis) wrote %d bytes, want the resumed one meaningfully smaller", resumedBytes, freshBytes) + } +} + +// TestReceiver_PartialDirBasisIgnoresRealDestinationContent confirms +// loadPartialBasis genuinely takes priority over the real destination +// file when both exist: the real destination here is deliberately wrong +// (would produce an incorrect comparison basis on its own), while the +// partial-dir file holds the correct prefix - the final result must +// still be correct, proving the partial-dir file is what actually got +// used, not silently ignored in favor of the real (wrong) destination. +func TestReceiver_PartialDirBasisIgnoresRealDestinationContent(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + fullContent := strings.Repeat("A", 1400) + "the new tail" + correctPrefix := strings.Repeat("A", 1400) + wrongExistingDest := strings.Repeat("Z", 1400) // deliberately wrong, must not be used as the basis + + mustWriteFile(t, filepath.Join(srcRoot, "file.txt"), fullContent) + mustWriteFile(t, filepath.Join(destRoot, "file.txt"), wrongExistingDest) + partialDir := ".rsync-partial" + partialPath := filepath.Join(destRoot, partialDir, "file.txt") + mustMkdirAll(t, filepath.Dir(partialPath)) + mustWriteFile(t, partialPath, correctPrefix) + + runSenderReceiverWithOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, ReceiverOptions{PartialDir: partialDir}) + + assertSameContent(t, filepath.Join(srcRoot, "file.txt"), filepath.Join(destRoot, "file.txt")) +} + +// TestReceiver_PartialDirCreatesNoFilesAndDeletesNothingDuringDryRun is +// this ticket's own explicit dependency requirement (SC-11 interaction): +// with --dry-run set, --partial-dir must neither write a temp file, nor +// commit anything to destPath, nor delete the leftover partial-dir file +// it would otherwise have consumed and cleaned up on a real run - +// loadPartialBasis is read-only by construction (see its own doc +// comment), and the temp-file/rename/cleanup code all lives inside +// receiveRegularFile's `if !ctx.ropts.DryRun` guard, so a dry run should +// never reach any of it; this test is the proof, not just the +// structural argument. +func TestReceiver_PartialDirCreatesNoFilesAndDeletesNothingDuringDryRun(t *testing.T) { + srcRoot, destRoot := t.TempDir(), t.TempDir() + fullContent := strings.Repeat("resumable content ", 100) + mustWriteFile(t, filepath.Join(srcRoot, "big.txt"), fullContent) + + partialDir := ".rsync-partial" + prefix := fullContent[:len(fullContent)/2] + partialPath := filepath.Join(destRoot, partialDir, "big.txt") + mustMkdirAll(t, filepath.Dir(partialPath)) + mustWriteFile(t, partialPath, prefix) + + var out bytes.Buffer + runSenderReceiverWithCompressAndReceiverOptions(t, srcRoot, destRoot, + sync.WalkOptions{Recursive: true}, nil, sync.AttrOptions{}, + ReceiverOptions{PartialDir: partialDir, DryRun: true, Itemize: true, Output: &out}, + CompressOptions{}) + + if !strings.Contains(out.String(), "big.txt") { + t.Errorf("dry-run itemize output = %q, want it to mention big.txt", out.String()) + } + if _, statErr := os.Stat(filepath.Join(destRoot, "big.txt")); !os.IsNotExist(statErr) { + t.Errorf("big.txt was created at the destination during a dry run, want nothing written at all") + } + got, err := os.ReadFile(partialPath) + if err != nil { + t.Fatalf("the leftover partial-dir file was removed/modified during a dry run, want it left exactly as it was: %v", err) + } + if string(got) != prefix { + t.Errorf("partial-dir file content = %q, want it unchanged (%q)", got, prefix) + } +} diff --git a/internal/pipeline/partial_test.go b/internal/pipeline/partial_test.go new file mode 100644 index 0000000..18bcde0 --- /dev/null +++ b/internal/pipeline/partial_test.go @@ -0,0 +1,305 @@ +package pipeline + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestCreateTempFile_SameDirectoryHiddenRecognizableName(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + + f, err := createTempFile(destPath) + if err != nil { + t.Fatalf("createTempFile returned error: %v", err) + } + defer func() { _ = f.Close() }() + defer func() { _ = os.Remove(f.Name()) }() + + if filepath.Dir(f.Name()) != dir { + t.Errorf("temp file dir = %q, want %q (same directory as destPath, so the eventual rename is same-filesystem)", filepath.Dir(f.Name()), dir) + } + base := filepath.Base(f.Name()) + if !strings.HasPrefix(base, ".file.txt.") || !strings.HasSuffix(base, ".grsync-tmp") { + t.Errorf("temp file name = %q, want a hidden, recognizable \".file.txt.*.grsync-tmp\" pattern", base) + } +} + +func TestCreateTempFile_DefaultModeMatchesOsWriteFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits aren't meaningfully comparable on Windows") + } + dir := t.TempDir() + f, err := createTempFile(filepath.Join(dir, "file.txt")) + if err != nil { + t.Fatalf("createTempFile returned error: %v", err) + } + defer func() { _ = f.Close() }() + defer func() { _ = os.Remove(f.Name()) }() + + info, err := f.Stat() + if err != nil { + t.Fatalf("Stat: %v", err) + } + // os.CreateTemp's own default is 0600; createTempFile must override + // that to 0644 so a sync run without --perms doesn't quietly start + // producing more restrictive files than os.WriteFile always has. + if info.Mode().Perm() != 0o644 { + t.Errorf("temp file mode = %v, want 0644 (matching os.WriteFile's own default for a no---perms sync)", info.Mode().Perm()) + } +} + +func TestPartialFilePath_RelativeDirIsInsideDestinationsOwnDirectory(t *testing.T) { + destPath := filepath.Join("dest", "sub", "file.txt") + got := partialFilePath(destPath, ".rsync-partial", "sub/file.txt") + want := filepath.Join("dest", "sub", ".rsync-partial", "file.txt") + if got != want { + t.Errorf("partialFilePath = %q, want %q", got, want) + } +} + +func TestPartialFilePath_AbsoluteDirMirrorsRelativePath(t *testing.T) { + absDir := string(filepath.Separator) + "partial-store" + if runtime.GOOS == "windows" { + absDir = `C:\partial-store` + } + destPath := filepath.Join("dest", "sub", "file.txt") + got := partialFilePath(destPath, absDir, "sub/file.txt") + want := filepath.Join(absDir, "sub", "file.txt") + if got != want { + t.Errorf("partialFilePath = %q, want %q", got, want) + } +} + +// TestPartialFilePath_AbsoluteDirAvoidsBasenameCollisions is the actual +// reason the absolute case mirrors the full relative path instead of +// just the basename: two different source files sharing a basename in +// different subdirectories must not collide under one shared +// partial-dir. +func TestPartialFilePath_AbsoluteDirAvoidsBasenameCollisions(t *testing.T) { + absDir := string(filepath.Separator) + "partial-store" + if runtime.GOOS == "windows" { + absDir = `C:\partial-store` + } + p1 := partialFilePath(filepath.Join("dest", "a", "file.txt"), absDir, "a/file.txt") + p2 := partialFilePath(filepath.Join("dest", "b", "file.txt"), absDir, "b/file.txt") + if p1 == p2 { + t.Errorf("partialFilePath collided for two different files sharing a basename: %q", p1) + } +} + +func TestLoadPartialBasis_NoPartialDirConfigured(t *testing.T) { + _, ok := loadPartialBasis(filepath.Join(t.TempDir(), "file.txt"), ReceiverOptions{}, "file.txt") + if ok { + t.Error("loadPartialBasis with no PartialDir configured returned ok=true, want false") + } +} + +func TestLoadPartialBasis_NothingThereYet(t *testing.T) { + dir := t.TempDir() + _, ok := loadPartialBasis(filepath.Join(dir, "file.txt"), ReceiverOptions{PartialDir: ".rsync-partial"}, "file.txt") + if ok { + t.Error("loadPartialBasis with nothing in the partial dir returned ok=true, want false") + } +} + +func TestLoadPartialBasis_FindsExistingPartialFile(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + ropts := ReceiverOptions{PartialDir: ".rsync-partial"} + pPath := partialFilePath(destPath, ropts.PartialDir, "file.txt") + if err := os.MkdirAll(filepath.Dir(pPath), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(pPath, []byte("partial prefix content"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + data, ok := loadPartialBasis(destPath, ropts, "file.txt") + if !ok { + t.Fatal("loadPartialBasis did not find the existing partial file") + } + if string(data) != "partial prefix content" { + t.Errorf("loadPartialBasis content = %q, want %q", data, "partial prefix content") + } +} + +func mustCreateTempFileWithContent(t *testing.T, destPath, content string) string { + t.Helper() + f, err := createTempFile(destPath) + if err != nil { + t.Fatalf("createTempFile: %v", err) + } + if _, err := f.WriteString(content); err != nil { + t.Fatalf("WriteString: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + return f.Name() +} + +func TestFinishRegularFileWrite_SuccessRenamesIntoPlace(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + tmpPath := mustCreateTempFileWithContent(t, destPath, "final content") + + if err := finishRegularFileWrite(tmpPath, destPath, nil, ReceiverOptions{}, "file.txt", false); err != nil { + t.Fatalf("finishRegularFileWrite returned error: %v", err) + } + + got, err := os.ReadFile(destPath) + if err != nil { + t.Fatalf("reading destPath: %v", err) + } + if string(got) != "final content" { + t.Errorf("destPath content = %q, want %q", got, "final content") + } + if _, err := os.Stat(tmpPath); !os.IsNotExist(err) { + t.Errorf("temp file %q still exists after a successful commit, want it gone (renamed away)", tmpPath) + } +} + +// TestFinishRegularFileWrite_SuccessCleansUpUsedPartialBasis is real +// rsync's own documented "delete it after it has served its purpose" +// behavior: once a partial-dir file's content has been successfully +// folded into a completed transfer, it should be removed so it doesn't +// linger as stale, misleading resume data for a future run. +func TestFinishRegularFileWrite_SuccessCleansUpUsedPartialBasis(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + ropts := ReceiverOptions{PartialDir: ".rsync-partial"} + pPath := partialFilePath(destPath, ropts.PartialDir, "file.txt") + if err := os.MkdirAll(filepath.Dir(pPath), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(pPath, []byte("stale partial prefix"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + tmpPath := mustCreateTempFileWithContent(t, destPath, "final content") + if err := finishRegularFileWrite(tmpPath, destPath, nil, ropts, "file.txt", true); err != nil { + t.Fatalf("finishRegularFileWrite returned error: %v", err) + } + + if _, err := os.Stat(pPath); !os.IsNotExist(err) { + t.Errorf("used partial-dir file %q still exists after a successful transfer, want it removed", pPath) + } +} + +func TestAbandonOrKeep_NoPartialDeletesTempFileEntirely(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + tmpPath := mustCreateTempFileWithContent(t, destPath, "interrupted content") + origErr := os.ErrClosed // any stand-in error + + err := abandonOrKeep(tmpPath, destPath, ReceiverOptions{}, "file.txt", origErr) + if err != origErr { + t.Errorf("abandonOrKeep returned %v, want the original error unchanged", err) + } + if _, statErr := os.Stat(tmpPath); !os.IsNotExist(statErr) { + t.Errorf("temp file %q still exists without --partial, want it deleted", tmpPath) + } + if _, statErr := os.Stat(destPath); !os.IsNotExist(statErr) { + t.Errorf("destPath %q exists without --partial, want it untouched (never created)", destPath) + } +} + +func TestAbandonOrKeep_PlainPartialKeepsContentAtDestPath(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + tmpPath := mustCreateTempFileWithContent(t, destPath, "interrupted content") + + _ = abandonOrKeep(tmpPath, destPath, ReceiverOptions{Partial: true}, "file.txt", os.ErrClosed) + + got, err := os.ReadFile(destPath) + if err != nil { + t.Fatalf("--partial should have left the partial content at destPath, but reading it failed: %v", err) + } + if string(got) != "interrupted content" { + t.Errorf("destPath content = %q, want %q", got, "interrupted content") + } + if _, statErr := os.Stat(tmpPath); !os.IsNotExist(statErr) { + t.Errorf("temp file %q still exists after being kept at destPath, want it gone from its original name", tmpPath) + } +} + +func TestAbandonOrKeep_PartialDirMovesContentThereInsteadOfDestPath(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + ropts := ReceiverOptions{PartialDir: ".rsync-partial"} + tmpPath := mustCreateTempFileWithContent(t, destPath, "interrupted content") + + _ = abandonOrKeep(tmpPath, destPath, ropts, "file.txt", os.ErrClosed) + + pPath := partialFilePath(destPath, ropts.PartialDir, "file.txt") + got, err := os.ReadFile(pPath) + if err != nil { + t.Fatalf("--partial-dir should have moved the partial content there, but reading it failed: %v", err) + } + if string(got) != "interrupted content" { + t.Errorf("partial-dir content = %q, want %q", got, "interrupted content") + } + if _, statErr := os.Stat(destPath); !os.IsNotExist(statErr) { + t.Errorf("destPath %q exists after an aborted --partial-dir transfer, want it left completely untouched", destPath) + } +} + +// TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent is the +// self-review's own "could a partial file leak outside --partial-dir" +// question, answered concretely: an aborted transfer with --partial-dir +// set must never modify whatever was already sitting at the real +// destination path, even though a plain --partial (no dir) would +// deliberately overwrite it with the partial content. +func TestAbandonOrKeep_PartialDirNeverOverwritesExistingDestContent(t *testing.T) { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + if err := os.WriteFile(destPath, []byte("original untouched content"), 0o644); err != nil { + t.Fatalf("seeding destPath: %v", err) + } + ropts := ReceiverOptions{PartialDir: ".rsync-partial"} + tmpPath := mustCreateTempFileWithContent(t, destPath, "new interrupted content") + + _ = abandonOrKeep(tmpPath, destPath, ropts, "file.txt", os.ErrClosed) + + got, err := os.ReadFile(destPath) + if err != nil { + t.Fatalf("reading destPath: %v", err) + } + if string(got) != "original untouched content" { + t.Errorf("destPath content = %q after an aborted --partial-dir transfer, want the original content left completely untouched", got) + } +} + +// TestAbandonOrKeep_NeverLeavesATempFileNextToDestPath is the same +// self-review question from the opposite angle: regardless of which +// policy applies, the raw ".file.txt.*.grsync-tmp" temp file must never +// be the thing left behind in the destination's own directory - it's +// always either deleted or moved to one of the two real, documented +// destinations (destPath itself, or inside PartialDir). +func TestAbandonOrKeep_NeverLeavesATempFileNextToDestPath(t *testing.T) { + for _, ropts := range []ReceiverOptions{ + {}, + {Partial: true}, + {PartialDir: ".rsync-partial"}, + } { + dir := t.TempDir() + destPath := filepath.Join(dir, "file.txt") + tmpPath := mustCreateTempFileWithContent(t, destPath, "content") + + _ = abandonOrKeep(tmpPath, destPath, ropts, "file.txt", os.ErrClosed) + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.Contains(e.Name(), ".grsync-tmp") { + t.Errorf("ropts=%+v: a raw temp file %q was left in the destination directory, want it always deleted or moved away", ropts, e.Name()) + } + } + } +} diff --git a/internal/pipeline/receiver.go b/internal/pipeline/receiver.go index 7ba9dbd..c344195 100644 --- a/internal/pipeline/receiver.go +++ b/internal/pipeline/receiver.go @@ -24,7 +24,7 @@ type receiveContext struct { ropts ReceiverOptions stats *Stats // nil unless ropts.Stats - progress *progressReporter // nil unless ropts.Progress (and never during DryRun - see writeFileWithProgress) + progress *progressReporter // nil unless ropts.Progress (and never during DryRun - see writeToTempFileWithProgress) totalFiles int // every entry in the received list, all types processed int // entries handled so far, for filesLeft = totalFiles - processed @@ -47,7 +47,8 @@ type receiveContext struct { // comparison against the destination's current state, stats // accumulation) still runs exactly as it would for a real transfer - see // the individual receive* helpers below for the specific guarded calls, -// all eight of them audited: two os.MkdirAll calls and an os.WriteFile in +// all of them audited: two os.MkdirAll calls and a +// writeToTempFileWithProgress+os.Rename pair (see partial.go) in // receiveRegularFile, an os.MkdirAll and sync.ApplyAttributes (which // itself calls os.Symlink for a symlink entry, not just chmod/chtimes) // in receiveSymlink, sync.ApplyAttributes for a directory in the @@ -56,7 +57,10 @@ type receiveContext struct { // a dry run: it specifically measures bytes committed to disk (see // progress.go's own doc comment for why, given grsync's non-streaming // wire protocol), and dry-run skips that write entirely, so there is -// nothing for it to report. +// nothing for it to report. ropts.Partial/PartialDir are likewise +// meaningless during a dry run: with no temp file ever created, there is +// nothing for either to keep or discard - see partial.go's own package +// doc comment for the full write-path design. // // A destination file not mentioned in the sender's list is never touched // at all: Receiver only ever acts on paths that appear in the received @@ -254,27 +258,86 @@ func receiveSymlink(ctx *receiveContext, destPath string, entry sync.FileEntry) // delta, and reconstructs what the file's bytes would become - all of // this runs identically in dry-run mode, since it's exactly the planning // work needed to report accurate itemize/stats output; only the final -// write (os.WriteFile/os.MkdirAll/sync.ApplyAttributes, and any progress -// reporting about it) is skipped. +// write (via a temp file - see partial.go - and sync.ApplyAttributes, and +// any progress reporting about either) is skipped. +// +// --append/--append-verify (SC-12) share the same two eligibility rules, +// verified against real rsync's own source (generator.c) rather than +// assumed: a file that doesn't exist at the destination yet is +// transferred completely normally - append semantics only ever apply to +// an existing, shorter file, matching real rsync's own documented "new +// files are transferred" rule. A destination that's already at least as +// long as the source is skipped entirely and unconditionally - not +// compared, not touched at all - matching real rsync's own documented +// behavior exactly (and generator.c's own append_mode-gated skip). +// +// For a genuinely shorter destination file, the two flags diverge +// exactly where real rsync's own source diverges (generate_and_send_sums: +// plain --append returns immediately after writing only a header, +// sending zero real block checksums; --append-verify falls through to +// the completely normal per-block signature loop): --append blindly +// trusts the existing prefix - no sync.GenerateSignature call for it at +// all, Sender is told (via appendTail) to send only the literal tail - +// while --append-verify runs the exact same sync.GenerateSignature/ +// GenerateDelta/ApplyDelta pipeline as an entirely normal sync, needing +// no new algorithm at all: the eligibility gate above is the only thing +// --append-verify actually adds on top of a vanilla transfer. func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, entry sync.FileEntry) error { old, existed, err := lstatExisting(destPath) if err != nil { return fmt.Errorf("checking existing %q: %w", entry.Path, err) } - oldData, err := os.ReadFile(destPath) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("reading existing %q: %w", entry.Path, err) + // usedPartialBasis is deliberately never true under append mode: + // --append/--append-verify are entirely about the REAL destination + // file's own existing bytes (trusted or verified), and mixing that + // with a --partial-dir staging file's content would blur two + // separately-reasoned-about features together for no real benefit - + // see partial.go's loadPartialBasis for the resume mechanism this + // skips here. + usedPartialBasis := false + var oldData []byte + if !ctx.ropts.AppendMode() { + if data, ok := loadPartialBasis(destPath, ctx.ropts, entry.Path); ok { + oldData, usedPartialBasis = data, true + } + } + if !usedPartialBasis { + oldData, err = os.ReadFile(destPath) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("reading existing %q: %w", entry.Path, err) + } } // oldData is nil when the file doesn't exist yet at the destination - // (the new-file case). sync.GenerateSignature on nil/empty data - // naturally produces a Signature with zero Blocks, which makes - // sync.GenerateDelta emit a single all-DataOp delta (nothing to match - // against) - exactly the "new file" behavior needed, falling directly - // out of the existing SC-3 API with no special-casing required here. - sig := sync.GenerateSignature(oldData) - - if err := sendSignature(rw, entry.Path, sig); err != nil { + // and no partial-dir file was found either (the new-file case). + // sync.GenerateSignature on nil/empty data naturally produces a + // Signature with zero Blocks, which makes sync.GenerateDelta emit a + // single all-DataOp delta (nothing to match against) - exactly the + // "new file" behavior needed, falling directly out of the existing + // SC-3 API with no special-casing required here. + + action := appendNone + var sig sync.Signature + switch { + case !ctx.ropts.AppendMode() || !existed: + sig = sync.GenerateSignature(oldData) + case int64(len(oldData)) >= entry.Size: + // Append eligibility rule: skip entirely, don't even compute a + // real signature for potentially-huge existing data we're never + // going to consult - see this function's own doc comment. + action = appendSkip + case ctx.ropts.AppendVerify: + sig = sync.GenerateSignature(oldData) + default: // ctx.ropts.Append, and genuinely shorter than entry.Size + action = appendTail + // BlockSize carries the trusted offset, not a real block size - + // see appendTail's own doc comment. Blocks is left empty: + // Sender never reads it for this action, since nothing here is + // verified at all. + sig = sync.Signature{BlockSize: len(oldData)} + } + + if err := sendSignature(rw, entry.Path, sig, action); err != nil { return fmt.Errorf("sending signature for %q: %w", entry.Path, err) } @@ -286,9 +349,22 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, return fmt.Errorf("delta arrived out of order: got %q, want %q", deltaPath, entry.Path) } - newData, err := sync.ApplyDelta(oldData, ops, sig) - if err != nil { - return fmt.Errorf("applying delta for %q: %w", entry.Path, err) + var newData []byte + if action == appendSkip { + // Unconditional skip: real rsync's own documented rule for a + // destination that's already not shorter than the source is to + // leave it alone entirely, not to compare and possibly find it + // already matches byte for byte - so newData is oldData, + // verbatim, not the result of applying Sender's (empty) delta to + // it (which sync.ApplyDelta would otherwise reduce to nothing at + // all, since it never implicitly copies oldData forward on its + // own - see its own doc comment). + newData = oldData + } else { + newData, err = sync.ApplyDelta(oldData, ops, sig) + if err != nil { + return fmt.Errorf("applying delta for %q: %w", entry.Path, err) + } } // ApplyDelta is pure in-memory work - computing it, and this // comparison, costs nothing extra and runs the same whether or not @@ -298,7 +374,7 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, // codebase doesn't otherwise have (see the README's Dry-Run Mode // section for why that's a deliberate, disclosed choice, not an // oversight). - contentChanged := !bytes.Equal(oldData, newData) + contentChanged := action != appendSkip && !bytes.Equal(oldData, newData) // transferred is deliberately not just contentChanged: a brand-new // *empty* file has oldData == nil and newData == nil (ApplyDelta's // accumulator is never appended to when there are no ops at all), so @@ -308,7 +384,9 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, // count. itemizeFile doesn't have this problem since it checks // !existed as its own first, higher-priority branch (see its own // code) - this mirrors that same priority for stats/xfer purposes. - transferred := !existed || contentChanged + // appendSkip is never "transferred" by definition - nothing was even + // compared, let alone changed. + transferred := action != appendSkip && (!existed || contentChanged) if ctx.stats != nil { literal, matched := deltaByteCounts(ops, sig.BlockSize, len(oldData)) @@ -339,10 +417,17 @@ func receiveRegularFile(ctx *receiveContext, rw io.ReadWriter, destPath string, if transferred { ctx.xferNum++ + tmpPath, werr := writeToTempFileWithProgress(destPath, newData, ctx.progress, entry.Path, ctx.xferNum, ctx.totalFiles, ctx.totalFiles-ctx.processed-1) + if err := finishRegularFileWrite(tmpPath, destPath, werr, ctx.ropts, entry.Path, usedPartialBasis); err != nil { + return fmt.Errorf("writing %q: %w", entry.Path, err) + } } - if err := writeFileWithProgress(destPath, newData, ctx.progress, entry.Path, ctx.xferNum, ctx.totalFiles, ctx.totalFiles-ctx.processed-1); err != nil { - return fmt.Errorf("writing %q: %w", entry.Path, err) - } + // Applied regardless of transferred: real rsync's own documented + // --append behavior explicitly "does not interfere with the + // updating of a file's non-content attributes... when the file + // does not need to be transferred" - an appendSkip'd file (or + // any other untransferred-but-existing file) still gets its + // permissions/times/etc. brought in line if requested. if _, err := sync.ApplyAttributes(entry, destPath, ctx.attrOpts); err != nil { return fmt.Errorf("applying attributes to %q: %w", entry.Path, err) } @@ -378,54 +463,6 @@ func deltaByteCounts(ops []sync.DeltaOp, blockSize int, oldDataLen int) (literal return literal, matched } -// writeFileWithProgress writes data to destPath, reporting progress -// through progress (nil-safe: with nil progress - including always -// during a dry run, see Receiver's own doc comment - this behaves as a -// plain os.WriteFile with no extra syscalls). Small files are written in -// a single call even with progress enabled - chunking a handful of -// bytes would only add syscall overhead for a duration too short to -// ever be visibly "in progress" - so chunking only kicks in once data -// exceeds progressWriteChunkSize. -func writeFileWithProgress(destPath string, data []byte, progress *progressReporter, path string, xferNum, totalFiles, filesLeft int) error { - if progress == nil || len(data) <= progressWriteChunkSize { - if err := os.WriteFile(destPath, data, 0o644); err != nil { - return err - } - if progress != nil { - progress.report(progressUpdate{ - path: path, bytesDone: int64(len(data)), fileSize: int64(len(data)), done: true, - xferNum: xferNum, totalFiles: totalFiles, filesLeft: filesLeft, - }) - } - return nil - } - - f, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) - if err != nil { - return err - } - defer func() { _ = f.Close() }() - - total := int64(len(data)) - var written int64 - for written < total { - end := written + progressWriteChunkSize - if end > total { - end = total - } - n, werr := f.Write(data[written:end]) - written += int64(n) - if werr != nil { - return werr - } - progress.report(progressUpdate{ - path: path, bytesDone: written, fileSize: total, done: written == total, - xferNum: xferNum, totalFiles: totalFiles, filesLeft: filesLeft, - }) - } - return nil -} - // lstatExisting is sync.LstatEntry with the "not found" case turned into // a plain (zero value, false, nil) result instead of an error the caller // has to unwrap - every call site here wants exactly that: "did diff --git a/internal/pipeline/sender.go b/internal/pipeline/sender.go index fa6e37b..cf860fe 100644 --- a/internal/pipeline/sender.go +++ b/internal/pipeline/sender.go @@ -87,12 +87,34 @@ func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []syn return fmt.Errorf("signature arrived out of order: got %q, want %q", sigMsg.Path, entry.Path) } + if sigMsg.Append == appendSkip { + // The receiver already decided this file doesn't need + // touching at all (--append/--append-verify's "destination + // not shorter than source" eligibility rule) - acknowledge + // with an empty delta without even opening the file, + // matching real rsync's own documented behavior of never + // comparing such files at all. + if err := sendDelta(rw, entry.Path, nil, copts); err != nil { + return fmt.Errorf("sending delta for %q: %w", entry.Path, err) + } + continue + } + data, err := os.ReadFile(filepath.Join(src, filepath.FromSlash(entry.Path))) if err != nil { return fmt.Errorf("reading %q: %w", entry.Path, err) } - ops := sync.GenerateDelta(sigMsg.Sig, data) + var ops []sync.DeltaOp + if sigMsg.Append == appendTail { + ops, err = appendTailOps(sigMsg.Sig.BlockSize, data) + if err != nil { + return fmt.Errorf("preparing append tail for %q: %w", entry.Path, err) + } + } else { + ops = sync.GenerateDelta(sigMsg.Sig, data) + } + if err := sendDelta(rw, entry.Path, ops, copts); err != nil { return fmt.Errorf("sending delta for %q: %w", entry.Path, err) } @@ -100,3 +122,37 @@ func Sender(rw io.ReadWriter, src string, walkOpts sync.WalkOptions, rules []syn return nil } + +// appendTailOps builds the delta for --append: a single CopyOp +// representing "trust the receiver's first trustedLen bytes exactly as +// they are, without verifying them at all" - a legitimate, direct use of +// CopyOp's own documented contract (it only ever claims to copy a block +// unchanged, never that the block was checksum-verified; that +// verification is a property of how sync.GenerateDelta happens to find +// a match, not of CopyOp itself), followed by a DataOp for whatever of +// data comes after that offset - literal bytes the receiver has never +// seen. Verified against real rsync's own source (generator.c/sender.c) +// for the underlying "trust the prefix, send only the tail" behavior. +// +// A source file that has shrunk below trustedLen since the receiver +// last checked its own file's length (a narrow, real race - real +// rsync calls this a "diminished" file and skips it with a warning, +// continuing the rest of the transfer) is treated as a hard error here +// instead: grsync's Receiver has no general "skip this one file, keep +// going" mechanism anywhere else in the codebase, and inventing one +// solely for this narrow race is a bigger change than this ticket +// calls for - see the README's Partial and Append Transfers section for +// this disclosed scope difference. +func appendTailOps(trustedLen int, data []byte) ([]sync.DeltaOp, error) { + if trustedLen > len(data) { + return nil, fmt.Errorf("source file shrank to %d bytes, below the %d bytes already trusted on the receiving side (a \"diminished\" file - see real rsync's own --append docs)", len(data), trustedLen) + } + var ops []sync.DeltaOp + if trustedLen > 0 { + ops = append(ops, sync.CopyOp{BlockIndex: 0}) + } + if tail := data[trustedLen:]; len(tail) > 0 { + ops = append(ops, sync.DataOp{Bytes: tail}) + } + return ops, nil +} diff --git a/internal/pipeline/ssh_test.go b/internal/pipeline/ssh_test.go index c6865e9..38c5bbb 100644 --- a/internal/pipeline/ssh_test.go +++ b/internal/pipeline/ssh_test.go @@ -299,3 +299,54 @@ func TestSSHLocalhost_IPv4ForwardedToSSHDoesNotBreakTheTransfer(t *testing.T) { assertSameContent(t, filepath.Join(src, "top.txt"), filepath.Join(dest, "top.txt")) } + +// TestSSHLocalhost_AppendAndPartialDoNotBreakTheTransfer is SC-12's +// real, over-the-wire proof for the SSH transport: --append/--partial +// are forwarded to the remote --server process as ordinary argv flags +// (see cli.syncToRemote's own doc comment - the same mechanism SC-11 +// established for --dry-run/--itemize-changes), parsed there through +// the server's own normal flag handling, with no wire-protocol change +// needed at all. This drives that real path end to end against a real +// local sshd: the destination file is a genuine prefix of the source, so +// a real --append tail-only transfer actually happens, not just a +// harmless no-op. +func TestSSHLocalhost_AppendAndPartialDoNotBreakTheTransfer(t *testing.T) { + requireLocalSSHServer(t) + grsyncPath := buildGrsyncBinary(t) + + src := t.TempDir() + dest := t.TempDir() + prefix := "already on the remote side " + full := prefix + "and now the new tail, sent over real ssh" + mustWriteFile(t, filepath.Join(src, "growing.log"), full) + mustWriteFile(t, filepath.Join(dest, "growing.log"), prefix) + + session, err := transport.Dial("", "", "127.0.0.1", []string{grsyncPath, "--server", "--append", "--partial", dest}, false, false) + if err != nil { + t.Fatalf("Dial returned error: %v", err) + } + + if err := transport.Handshake(session); err != nil { + t.Fatalf("Handshake returned error: %v", err) + } + + sendErrCh := make(chan error, 1) + go func() { + sendErrCh <- Sender(session, src, sync.WalkOptions{Recursive: true}, nil, false, CompressOptions{}) + }() + + select { + case err := <-sendErrCh: + if err != nil { + t.Fatalf("Sender returned error: %v", err) + } + case <-time.After(20 * time.Second): + t.Fatal("Sender did not complete within 20s") + } + + if err := session.Close(); err != nil { + t.Errorf("Session.Close returned error: %v", err) + } + + assertSameContent(t, filepath.Join(src, "growing.log"), filepath.Join(dest, "growing.log")) +} diff --git a/internal/sync/delta.go b/internal/sync/delta.go index 2a83af2..d8da8b0 100644 --- a/internal/sync/delta.go +++ b/internal/sync/delta.go @@ -141,16 +141,24 @@ func GenerateDelta(sig Signature, newData []byte) []DeltaOp { // to plug into - so an io.Writer parameter would just be unused // flexibility at this stage. That can change if/when a streaming // transport is introduced later. +// blockSize is validated lazily, only once a CopyOp actually needs it to +// translate a BlockIndex into a byte range - not unconditionally up +// front - so a delta with no CopyOps at all (every real caller today +// still has BlockSize > 0 via GenerateSignature's own DefaultBlockSize, +// but SC-12's append-mode construction can legitimately produce a +// Signature with BlockSize == 0 when the receiver's existing file is +// empty, since there is then no prefix block to describe at all) never +// gets rejected for a value that would never actually be used. func ApplyDelta(oldData []byte, ops []DeltaOp, sig Signature) ([]byte, error) { blockSize := sig.BlockSize - if blockSize <= 0 { - return nil, fmt.Errorf("invalid signature block size %d", blockSize) - } var out []byte for i, op := range ops { switch o := op.(type) { case CopyOp: + if blockSize <= 0 { + return nil, fmt.Errorf("op %d: invalid signature block size %d", i, blockSize) + } start := o.BlockIndex * blockSize if o.BlockIndex < 0 || start >= len(oldData) { return nil, fmt.Errorf("op %d: CopyOp block index %d is out of range for a %d-byte old file", i, o.BlockIndex, len(oldData)) diff --git a/internal/sync/delta_test.go b/internal/sync/delta_test.go index f993cdb..fc6fe23 100644 --- a/internal/sync/delta_test.go +++ b/internal/sync/delta_test.go @@ -32,10 +32,34 @@ func TestApplyDelta_OutOfRangeBlockIndexErrors(t *testing.T) { } } -func TestApplyDelta_InvalidBlockSizeErrors(t *testing.T) { - _, err := ApplyDelta([]byte("data"), nil, Signature{BlockSize: 0}) - if err == nil { - t.Fatalf("ApplyDelta with a zero block size returned nil error, want an error") +// TestApplyDelta_InvalidBlockSizeErrorsOnlyWhenACopyOpNeedsIt is SC-12's +// own relaxation of ApplyDelta's block-size validation, made concrete: a +// zero/negative BlockSize is only ever actually consulted when +// translating a CopyOp's BlockIndex into a byte range, so it must only +// be rejected when ops genuinely contains a CopyOp - not unconditionally +// up front, which would reject SC-12's own append-mode construction for +// a brand-new (zero-length existing) destination file, where a +// Signature with BlockSize == 0 and no CopyOp at all is entirely valid +// (there is no prefix block to describe). +func TestApplyDelta_InvalidBlockSizeErrorsOnlyWhenACopyOpNeedsIt(t *testing.T) { + if _, err := ApplyDelta([]byte("data"), []DeltaOp{CopyOp{BlockIndex: 0}}, Signature{BlockSize: 0}); err == nil { + t.Errorf("ApplyDelta with a zero block size and a CopyOp returned nil error, want an error") + } + + got, err := ApplyDelta(nil, []DeltaOp{DataOp{Bytes: []byte("new content")}}, Signature{BlockSize: 0}) + if err != nil { + t.Fatalf("ApplyDelta with a zero block size and no CopyOp returned error: %v", err) + } + if string(got) != "new content" { + t.Errorf("ApplyDelta result = %q, want %q", got, "new content") + } + + got, err = ApplyDelta(nil, nil, Signature{BlockSize: 0}) + if err != nil { + t.Fatalf("ApplyDelta with a zero block size and no ops at all returned error: %v", err) + } + if len(got) != 0 { + t.Errorf("ApplyDelta result = %q, want empty", got) } }