Skip to content

fix(options): --append must imply --inplace - #7243

Merged
oferchen merged 1 commit into
masterfrom
fix/append-implies-inplace
Aug 6, 2026
Merged

fix(options): --append must imply --inplace#7243
oferchen merged 1 commit into
masterfrom
fix/append-implies-inplace

Conversation

@oferchen

@oferchen oferchen commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Upstream rule

options.c:2400-2411:

	if (append_mode) {
		if (whole_file > 0) {
			snprintf(err_buf, sizeof err_buf,
				 "--append cannot be used with --whole-file\n");
			goto cleanup;
		}
		if (refused_inplace) {
			create_refuse_error(refused_inplace);
			goto cleanup;
		}
		inplace = 1;
	}

After that assignment upstream never consults append_mode again to decide
where the receiver writes or what survives a failure. Every consumer reads the
implied flag:

upstream site what it decides
receiver.c:968 if (inplace || one_inplace) write target is the live destination, not a temp file
receiver.c:327,496 inplace_sizing preallocated_len = size_r, and the closing ftruncate(fd, offset)
generator.c:1862,1898 if (inplace && make_backups > 0 && fnamecmp_type == FNAMECMP_FNAME) COPIES the pre-image aside before the in-place write destroys it
receiver.c:872 if (inplace && make_backups > 0) the delta basis switches to that backup, fnamecmp_type = FNAMECMP_BACKUP
receiver.c:1029 (recv_ok && ...) || inplace RETAINS a failed-verification update instead of discarding it
receiver.c:1074 !(keep_partial && partialptr) && !inplace keptstr says "retained" rather than "discarded"
sender.c:337 updating_basis_file gates match.c:211's backward-Copy suppression
compat.c:688 basis_dir_cnt && inplace protocol < 29 refuses a basis dir
options.c:2424-2432 rejects --partial-dir / --delay-updates, naming append_mode ? "append" : "inplace"
batch.c:72 flag_ptr[12] = &inplace the batch stream-flags bit carries the implied value

What oc did

inplace was never set. ClientConfigBuilder::validate reconstructed the rule
locally (let is_inplace = self.inplace || self.append), so the conflict table
fired correctly while inplace itself stayed false for every other consumer.
A second reconstruction sat in transfer_ops::resolve_use_inplace
(inplace || append), which is why the network receiver's write target was
right by accident.

Observable consequence (measured, rsync 3.4.4 vs oc, aarch64 Linux)

Destination AAAABBBB (8 bytes), source AAAABBBBCCCCDDDD (16 bytes).

cell upstream 3.4.4 oc before oc after
--append --backup, backup content AAAABBBB AAAABBBBCCCCDDDD AAAABBBB
--append --backup, backup shares dest inode no YES no
--append --backup --backup-dir, copy content AAAABBBB AAAABBBBCCCCDDDD AAAABBBB
--append --backup run twice, backup content AAAABBBB AAAABBBBCCCCDDDD AAAABBBB
--inplace --backup (control), backup content AAAABBBB AAAABBBB AAAABBBB
--append, destination content AAAABBBBCCCCDDDD same same
--append onto a fresh destination AAAABBBBCCCCDDDD, no temp left same same
--append --partial-dir / --delay-updates / --whole-file exit 1, --append cannot be used with --X same same

Without the flag the generator takes the non-inplace backup path, which
hard-links the destination into the backup area before the transfer
(rsync.c:740). The append then grows that shared inode, so the "backup" is
byte-identical to the new file and the pre-image is gone. The explicit
--inplace --backup control was always correct, which isolates the defect to
the missing implication rather than to the backup code.

Scope of that defect: the local-copy path only. On the network path
resolve_use_inplace already ORed append, so disk_commit's
make_inplace_backup fired and the remote --append --backup cell measured
correct both before and after.

Server argv - unchanged, and matching upstream

options.c:2951-2956 sends --append (twice for --append-verify) and never
--inplace beside it. Measured through an -e shim that logs the server argv:

invocation --inplace in argv --append count --append-verify in argv
--append (upstream / oc before / oc after) 0 / 0 / 0 1 / 1 / 1 0 / 0 / 0
--append-verify (upstream / oc before / oc after) 0 / 0 / 0 2 / 2 / 2 0 / 0 / 0
--inplace (upstream / oc before / oc after) 1 / 1 / 1 0 / 0 / 0 -

Both emitters (invocation/builder.rs, daemon_transfer/.../arguments.rs)
already used upstream's if append { ... } else if inplace { ... } shape, so
setting the flag does not add --inplace to any peer argv.

Change

Set the flag; delete the reconstruction.

  • ClientConfigBuilder::apply_implied_options() runs at the head of both
    validate and build, mirroring upstream's parse-then-check ordering, so
    the conflict table can test self.inplace directly. The is_inplace
    disjunction is gone; self.inplace is the single source of truth. The
    message still reads if self.append { "append" } else { "inplace" } -
    upstream's own naming ternary, not a second copy of the flag.
  • ServerConfig::apply_append_implies_inplace() on entry to
    run_server_with_handshake_adopting - the one path the --server argv
    parser, the daemon's client_args parser and the client's in-process half
    all reach, and it runs before setup_protocol()'s protocol < 29 checks.
  • transfer_ops::resolve_use_inplace drops its append parameter and reads
    inplace exactly as receiver.c:968 does.

--backup is not in upstream's conflict table, and no rejection is added.

Consumer audit

Every oc site that branches on inplace, and what it now does under --append:

  • transfer_ops::resolve_use_inplace - unchanged result; now single-sourced.
  • pipeline/receiver.rs::verification_kept_str - unchanged result; it reads
    the per-file use_inplace, which already carried append.
  • disk_commit::make_inplace_backup - unchanged result, same reason. This is
    oc's generator.c:1898 analogue and it is reached BY the flag.
  • transfer_ops/response.rs:402 - newly reached. The destination is
    truncated to the received length, matching receiver.c:496. Previously
    neither branch ran under --append.
  • generator/transfer/transfer_loop.rs:929 updating_basis_file - newly
    true
    , matching sender.c:337. This is the server-side sender on a pull,
    the site neither local reconstruction could reach.
  • setup/restrictions.rs:132 - --append with a basis dir at protocol < 29 is
    now refused, matching compat.c:688.
  • config/builder.rs:564,583 - transfer-layer --inplace conflict checks. Not
    user-reachable; the client rejects the same pairs first with upstream's
    wording.
  • run/batch.rs:115 - the batch stream-flags inplace bit now carries the
    implied value, matching batch.c:72 (flag_ptr[12] = &inplace).
  • engine local copy: select_write_strategy picks Inplace for a fresh
    destination instead of a temp file (receiver.c:968); execute/mod.rs:188
    takes the copy-aside backup path (generator.c:1862,1898) - this is the
    fix above; execute/mod.rs:517 sets preallocated_len = size_r
    (receiver.c:334). The FICLONE / clonefile / CopyFileEx fast paths now
    decline --append; they have no upstream analogue and the result is
    byte-identical either way.

Tests

  • tests/append_implies_inplace.rs - --append --backup must keep the
    pre-image AND must not share the destination inode (the two together are
    what separate "append happens to write in place" from "append IS inplace");
    the same for --backup-dir; an explicit --inplace --backup control;
    --append writes through the destination inode; the three upstream conflict
    diagnostics keep their exact wording and exit code.
  • crates/core/.../builder/tests.rs - the promotion fires for --append and
    --append-verify, and is one-directional.
  • crates/transfer/src/config/mod.rs - same for the server-side entry.

Verification

Run on the aarch64 Linux host, since GitHub Actions is not scheduling:

  • cargo fmt --all -- --check - clean
  • cargo clippy --locked --workspace --all-targets --all-features --no-deps -- -D warnings - clean
  • cargo nextest run --workspace --all-features --no-fail-fast - 32283 tests,
    32277 passed, 6 failed, all 6 reproduced on unpatched origin/master:
    4 xtask::commands::package::tests::cross_compiler_* and 2
    transfer::uts_chmod_option cases.
  • Every cell in the tables above measured against a real rsync 3.4.4 binary,
    and against an unpatched origin/master build of oc for the before column.

Out of scope, found while auditing

receiver.c:872 has no oc analogue: FnameCmpType::Backup exists in
crates/protocol but is never produced, so under --inplace --backup oc's
delta basis stays the destination rather than switching to the backup copy.
Independent of this PR (it reproduces with an explicit --inplace), and it
lives in the fenced receiver/generator crates.

receiver.c:759,769 (make_backups = -make_backups on redo) is unreachable
under --inplace/--append in upstream itself: it is gated on keep_partial,
which options.c:2439 clears inside the same if (inplace) block. Nothing to
mirror.

A local copy never runs setup::restrictions, so --protocol=28 --inplace --link-dest=... exits 0 where rsync 3.4.4 exits 13. Not append-specific and
unchanged by this PR.

--write-devices has the same missing implication (options.c:2413-2419 also
sets inplace = 1); left alone.

upstream options.c:2410 sets `inplace = 1` for any `append_mode`, and from
that point nothing reads `append_mode` again to decide where the receiver
writes or what survives a failure. Everything reads the one flag: the write
target (receiver.c:968), the closing ftruncate (receiver.c:496), the
pre-write backup copy (generator.c:1862,1898), the basis switch to that
backup (receiver.c:872), the retained-vs-discarded branch (receiver.c:1029),
the keptstr wording (receiver.c:1074), the sender's updating_basis_file
(sender.c:337) and the protocol < 29 basis-dir refusal (compat.c:688).

oc never set it. `ClientConfigBuilder::validate` reconstructed the rule
locally as `inplace || append`, so the conflict table fired correctly while
`inplace` itself stayed false for every other consumer.

That silently broke `--append --backup` on the local-copy path. With
`inplace` false the generator takes the non-inplace backup path, which
hard-links the destination into the backup area (rsync.c:740) before the
transfer; the append then grows that shared inode, so the "backup" ends up
byte-identical to the new file and the pre-image is destroyed. Measured
against rsync 3.4.4: backup content was AAAABBBBCCCCDDDD where upstream
writes AAAABBBB, and the backup shared the destination's inode. Same with
--backup-dir. An explicit --inplace --backup was always correct, which is
what isolates the defect to the missing implication.

Set the flag instead of reconstructing it. `apply_implied_options()` runs at
the head of `validate` and `build`, mirroring upstream's parse-then-check
ordering, and the local `is_inplace` disjunction is deleted so `self.inplace`
is the single source of truth. The server-side halves parse their own argv,
so `ServerConfig::apply_append_implies_inplace()` applies the same rule once
on entry to the server body - the path shared by the `--server` parser, the
daemon's client_args parser and the client's in-process half.
`transfer_ops::resolve_use_inplace` drops its `append` parameter and reads
`inplace` exactly as receiver.c:968 does.

Server argv is unchanged and matches upstream: `--append` (twice for
--append-verify) with no `--inplace` beside it, and `--append-verify` never
goes on the wire (options.c:2951-2956).
@oferchen
oferchen force-pushed the fix/append-implies-inplace branch from 6fb57a9 to e9c4207 Compare August 6, 2026 22:45
@oferchen
oferchen marked this pull request as ready for review August 6, 2026 22:53
@oferchen
oferchen merged commit af97cc4 into master Aug 6, 2026
@github-actions github-actions Bot added the bug Something isn't working label Aug 6, 2026
@oferchen
oferchen deleted the fix/append-implies-inplace branch August 9, 2026 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant