From 96e02f52c0aca2c7689f6abb3956da3cdfd49530 Mon Sep 17 00:00:00 2001 From: TapTap Date: Thu, 24 Sep 2026 01:18:13 +0200 Subject: [PATCH] fix(delay-updates): unique staging dir and implied --delete-after ordering (#317) --- RSYNC_COMPAT.md | 18 +- src/client/client_cli.c | 11 ++ src/server/receiver.c | 45 ++--- src/server/server.c | 36 ++-- src/shared/delay_updates.c | 154 ++++++++++++++---- src/shared/delay_updates.h | 7 +- src/shared/delete.c | 27 ++- src/shared/delete.h | 10 ++ src/shared/delete_commit.c | 6 +- src/shared/delete_plan.c | 1 + tests/integration/test_differential_parity.py | 17 ++ tests/integration/test_features.py | 87 ++++++++-- tests/test_client_cli.c | 43 +++++ tests/test_delay_updates.c | 71 +++++++- 14 files changed, 429 insertions(+), 104 deletions(-) diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index 135dc4d..677c8aa 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -7,8 +7,8 @@ This document maps rsync's full feature set to FastSync's current implementation | Status | Count | Description | |--------|-------|-------------| | ✅ Parity | 119 | Reproduces rsync's semantics for this option's scope | -| ⚠️ Caveat | 14 | Wired and tested, but carries a documented behavioral difference from rsync (named in the row and/or the wave notes) | -| ❌ Divergent | 24 | Rejected, an accepted no-op, deliberately non-rsync (native config/auth/batch, privileged namespaces, safe-subset privilege), or impossible on any portable filesystem call | +| ⚠️ Caveat | 16 | Wired and tested, but carries a documented behavioral difference from rsync (named in the row and/or the wave notes) | +| ❌ Divergent | 22 | Rejected, an accepted no-op, deliberately non-rsync (native config/auth/batch, privileged namespaces, safe-subset privilege), or impossible on any portable filesystem call | | **Total** | **157** | One row per rsync option/feature group; a row may name several spellings | This matrix reports honest rsync parity, not "implemented" as a synonym for @@ -55,6 +55,8 @@ matrix is **111 ✅ / 13 ⚠️ / 33 ❌ = 157**. **Lockstep track 6 (delete default; `PROTOCOL_VERSION` stays 2.28.0).** Plain `--delete` with no explicit timing flag now defaults to rsync's delete-during (`--del`) timing: the client normalizes it onto the existing `delete_during` wire bool in `cli_finalize_config`, so no config-frame field was added, and a tight destination no longer has to hold the whole old+new tree at once (the old atomic commit could hit `ENOSPC`). The old late whole-tree commit is opt-in via `--delete-after` or the FastSync-only long spelling `--delete-commit`, which selects the identical `delete_after` timing (documented equivalence). Precedence is unchanged and order-independent: each timing flag implies `--delete`, at most one timing flag may be given, and a timing flag with `--no-delete` is rejected. `-d/--dirs` still falls back to the end commit; `--delay-updates` still deletes genuine extras before publication (the per-directory skip list protects the staging dir); `--files-from`/`-R` scope is unchanged. The per-directory `STATUS_DELETE_PLAN` frame gained a one-int `apply` flag (still 2.28.0): the one-shot per-run config block (protected prefixes, size-pruned mirrors, `--delete-missing-args` exact paths) is now always sent first on a config-only carrier with `apply=false`, fixing a latent bug where a `--delete-missing-args` run whose `--files-from` list synchronized no directory never transmitted its exact deletions. Differential evidence: `delete` (plain, vs rsync's default), `delete_commit` (FastSync `--delete-commit` vs rsync `--delete-after`), and `filter_protect_after` (whole-tree protect) cases; `TestDeleteTimingFinalStateParity` compares plain `--delete`/`--delete-commit` against rsync on completed runs, and `TestDeleteTimingFailure` proves plain `--delete` removes reached extras on a mid-transfer abort while `--delete-commit` removes nothing. The matrix is unchanged at **116 ✅ / 14 ⚠️ / 27 ❌ = 157** (the `--delete`/`--delete-during` rows stay ⚠️ for the abort boundary; `--delete-after` stays ✅). +**Parity2 #317 (no wire change; `PROTOCOL_VERSION` stays 2.30.0).** Two `--delay-updates` residuals closed. (1) The receiver now stages under a per-run unique `.fastsync-stage..` directory instead of a fixed name, and `delay_updates_prepare` creates it with O_EXCL semantics: if that exact path already exists it refuses rather than wiping it, so a genuine destination entry named like the staging prefix is never destroyed (rsync likewise leaves a real entry named like its `.~tmp~` temp untouched; differential `test_delay_updates_staging_name_collision_preserved`). The delete walker now skips this transfer's runtime staging name. (2) `--delay-updates` implies `--delete-after`: the client normalizes it onto the existing `delete_after` wire bool in `cli_finalize_config` (no new wire field), and both the single-threaded and `--threads` receivers publish every staged file before committing the deferred delete. `--backup` files are now shielded from the delete-after pass (rsync never treats a backup as an extra), and a destination directory blocking a staged file is cleared at publication when `--delete`/`--force` is active (rsync's make-way). New differential cases `delay_updates` and `delay_updates_delete` plus `TestDelayUpdates` coverage assert final-state parity with rsync 3.4.1. Residuals: FastSync still does not create a backup of a *deleted* extra (rsync's `--backup --delete` does), and a crash-leftover staging directory is never reused (the next run picks a fresh name). The `--delay-updates` row moves ❌ → ⚠️. The matrix is now **119 ✅ / 16 ⚠️ / 22 ❌ = 157**. + **Parity cycle 2.29 (on `feat/parity-2.29`; `PROTOCOL_VERSION` stays 2.28.0 — no wire change was needed).** Five independent residuals were closed and four rows moved to ✅: - **Scanner order.** The sequential scanner now buffers and sorts each directory's inspected entries (non-directories ascending, then directories ascending) and walks them depth-first, reproducing rsync 3.4.1's flist order. This makes the `--info=name` transfer order, the `--delete-during`/`--delete-delay`/`-n` would-delete order, and the partial-`--max-delete` survivor set byte-identical to rsync (`test_parity_order.py`). `--threads` has no rsync analogue and stays unordered. - **Delete timing.** The complete per-directory plan set is transmitted before the first data frame, so a mid-transfer abort has already removed every planned extra like rsync's generator; `-d/--dirs` uses the same per-directory plans (shielded untraversed subdirectories) instead of the end-of-transfer commit (`test_delete_boundary_parity.py`). `-n`/`--delete`/`--del`/`--delete-delay` move ⚠️ → ✅. @@ -183,7 +185,7 @@ Every one of those has an entry below with its remaining caveats. | `-b`, `--backup` | Make backups of overwritten files | ✅ Parity | Backup before overwrite | | `--backup-dir=DIR` | Backup directory hierarchy | ✅ Parity | `backup_dir` config field | | `--suffix=SUFFIX` | Backup suffix (default ~) | ✅ Parity | `suffix` config field | -| `--delay-updates` | Put updated files in place at end | ❌ Divergent | Successfully received files are staged under a private 0700 `.fastsync-stage` dir inside the receive root and atomically renamed into their final destinations only after the whole transfer (manifest/delete handling included) succeeds, just before the success/outcome frame is sent. The delete walker deliberately skips the staging dir at the receive root, so `--delete` removes genuine extras but never the staged files (deletion runs before publication; rsync's delete-after ordering is not implemented). `--existing`/`--ignore-existing`/`--update` decide against the final destination path at stage time; `--backup` moves the old file aside at publication, and **`--force` is honored at publication** (protocol 2.23.0): a staged regular file or symlink may replace a destination directory that blocks it. Incompatible with `--inplace` and with `--backup-dir=.fastsync-stage` (the internal staging name is reserved; both are rejected). The staging dir name is fixed, so two simultaneous delayed transfers to the same destination root are serialized with an exclusive advisory lock held for the whole transfer: the second session fails cleanly instead of corrupting the first. Aborting or failing before publication installs nothing and removes the staging tree; a crash between stage and publish leaves staged leftovers that the next delayed run wipes at start (process death releases the lock). A stage→publish failure aborts the transfer (best-effort cleanup of the not-yet-published staged files; already-published files are not rolled back). **Reclassified Divergent (differential evidence):** the staging name is fixed and a delayed run wipes a pre-existing destination tree of that name at start even without `--delete`, whereas rsync uses its own internal temp name and leaves a genuine destination entry named `.fastsync-stage` untouched (`test_delay_updates_staging_name_collision_residual`); deletion also runs before publication while rsync's `--delay-updates` implies `--delete-after`. Works in single-threaded and `-j`/`--threads` modes | +| `--delay-updates` | Put updated files in place at end | ⚠️ Caveat | Successfully received files are staged under a private 0700 `.fastsync-stage..` directory inside the receive root and atomically renamed into their final destinations only after the whole transfer succeeds. The name is **per-run unique**, and prepare creates it with O_EXCL semantics: a genuine destination entry that happens to share the exact name is never wiped (the run refuses), matching rsync's own internal temp name (a pre-existing entry named like the staging prefix survives; differential `test_delay_updates_staging_name_collision_preserved`). The delete walker skips this transfer's runtime staging name at the receive root. **`--delay-updates` implies `--delete-after`** (client-normalized onto the existing `delete_after` wire bool; no new field): both the single-threaded and `--threads` receivers publish every staged file first and commit the deferred deletion afterwards, so extras are removed only after all updates land. A `--backup` file is shielded from that delete pass (rsync never treats a backup as an extra), and a destination directory blocking a staged regular file/symlink is cleared at publication when `--delete` or `--force` is active (rsync's generator make-way); without either, a non-empty blocker fails the run. `--existing`/`--ignore-existing`/`--update` decide against the final destination path at stage time. Incompatible with `--inplace` and with `--backup-dir=.fastsync-stage` (the internal staging prefix is reserved; both are rejected). Aborting or failing before publication installs nothing and removes the per-run staging tree; a stage→publish failure aborts the transfer (best-effort cleanup of the not-yet-published staged files; already-published files are not rolled back). **Reclassified ⚠️ Caveat (#317):** the fixed-name wipe and the delete-before-publication ordering are both gone; the remaining documented differences are that FastSync does not create a backup of a *deleted* extra the way rsync's `--backup --delete` does, and that a crash-leftover staging directory is never reused (each run picks a fresh name, so leftovers are not auto-cleaned). Works in single-threaded and `-j`/`--threads` modes | | `-T`, `--temp-dir=DIR` | Create temporary files in DIR | ❌ Divergent | `--temp-dir` with the rsync short `-T` (the timeout alias moved to long-only `--timeout`). A **relative** dir matches rsync exactly: it is resolved below the receive/destination root and must already exist (differentially verified: `rsync -a --temp-dir=scratch src/ dst/` and FastSync produce identical trees and an empty scratch dir). An **absolute** dir is accepted when it canonicalizes (`realpath(3)`) inside the receive root, so an in-root absolute scratch path is usable and used (unit- and integration-tested for both the local batch apply and a live TCP transfer). **Remaining divergence:** an absolute `--temp-dir` that escapes the receive root is rejected, and so is a relative one containing `..` — rsync standalone resolves an absolute `--temp-dir` verbatim (it will use `/tmp` or any other directory, including one outside the destination), but FastSync's security-reviewed receiver confines the scratch dir to the authorized receive root and refuses an out-of-root path before writing anything. **Audit-cycle hardening:** the opened dir is additionally judged by the real path of its fd (`/proc/self/fd`), so a client-planted symlink under the receive root cannot redirect receiver scratch files outside the authorized root (an escaping target is refused with `EACCES`), while an in-root symlink to another filesystem — the `EXDEV` fallback case — still works. A differential test confirms rsync exits 0 using an out-of-root absolute scratch dir while FastSync refuses before writing anything into it (the scratch dir stays empty). Its daemon mode also confines relative to the module. Temp copies use a unique name in the scratch dir and are atomically renamed into place; **on `EXDEV` (scratch dir and destination on different filesystems, reachable via a confined relative symlink) the receiver falls back to a non-atomic copy instead of aborting**, matching rsync. `--inplace` and `--partial-dir` writes bypass the scratch dir | | `--partial` | Keep partially transferred files | ✅ Parity | On a failed/interrupted write the already-written temp file is retained at the destination path (best-effort rename instead of unlink) so a later `--append`/`--append-verify` run can resume it. Retention never runs when no data was actually written or under `--ignore-existing`/`--existing` (the destination is not ours to overwrite), and it only ever renames the already-written temp. A failed rename falls back to the normal unlink | | `--partial-dir=DIR` | Keep partial files in DIR | ✅ Parity | The working file is written under the confined partial directory (a relative dir below the receive root) and atomically renamed into place once complete, so an interrupted transfer leaves a resumable copy there and completed transfers do not linger under it. `--inplace` bypasses the partial dir (rsync parity), and combining `--inplace` with `--partial-dir` is now **rejected up front** with rsync's message (`--inplace cannot be used with --partial-dir`) instead of silently ignoring the partial dir. **Implies `--partial`** (audit-cycle fix, matching rsync 3.4.1, which sets `keep_partial` after option parsing): `--partial-dir=DIR` alone retains an interrupted transfer's partial, and the implication wins over an explicit `--no-partial` regardless of order | @@ -192,7 +194,7 @@ Every one of those has an entry below with its remaining caveats. | Flag | Rsync Description | FastSync Status | Notes | |------|-------------------|-----------------|-------| -| `--delete` | Delete extraneous files from dest | ✅ Parity | `use_delete` config field. Deletion is always derived from the keep-set the sender actually transmitted (the per-directory `STATUS_DELETE_PLAN` set by default, or the whole-tree manifest for the late timings — never from unchecked input), runs through the symlink-safe walker bounded by `MAX_SERVER_DELETE_COUNT`, and skips the `.fastsync-stage` staging dir under `--delay-updates`. **Lockstep track 6 (protocol 2.28.0): plain `--delete` with no explicit timing flag now defaults to `--delete-during`**, exactly like rsync's `--del` (the client normalizes it to the existing `delete_during` wire bool; no new wire field). This frees destination space progressively during the transfer and avoids the whole-old+new-tree peak that could `ENOSPC` a tight destination. The old late whole-tree commit is opt-in via `--delete-after` or the FastSync-only long spelling `--delete-commit`. **Abort/ordering parity (parity-2.29):** the complete per-directory plan set is transmitted before the first data frame, so a mid-transfer abort has already applied every planned removal exactly like rsync's generator (which runs ahead of its throttled sender); `-d/--dirs` uses the same per-directory plans (the generator records only the directories whose direct children it enumerated, so an untraversed subdirectory's mirror is shielded); and the sorted depth-first traversal makes the removal order — and therefore the survivor set under a partial `--max-delete` — match rsync exactly (`test_delete_boundary_parity.py`, `test_parity_order.py`). By default the destination mirror of a path the source scan pruned (filter/exclude/size rules) is **protected** from deletion — matching rsync, which does not delete excluded files under `--delete`; `--delete-excluded` opts back into deleting them (see below). Deletion is scoped to the **synchronized directories** sent on the wire (protocol 2.23.0), so a `--files-from` subset no longer deletes untransmitted paths outside the listed directory subtrees. The walk is bounded: a client `--max-delete=NUM` (or the 100000-entry server bound) makes it **partial** — entries up to the bound are removed, the rest are skipped, and the client exits **25** (`RERR_PARTIAL`), matching rsync, rather than failing the transfer. Extraneous destination symlinks are unlinked by name (never followed); a directory still holding a kept/protected entry is left behind rather than failing | +| `--delete` | Delete extraneous files from dest | ✅ Parity | `use_delete` config field. Deletion is always derived from the keep-set the sender actually transmitted (the per-directory `STATUS_DELETE_PLAN` set by default, or the whole-tree manifest for the late timings — never from unchecked input), runs through the symlink-safe walker bounded by `MAX_SERVER_DELETE_COUNT`, and skips the runtime `--delay-updates` staging directory. **Lockstep track 6 (protocol 2.28.0): plain `--delete` with no explicit timing flag now defaults to `--delete-during`**, exactly like rsync's `--del` (the client normalizes it to the existing `delete_during` wire bool; no new wire field). This frees destination space progressively during the transfer and avoids the whole-old+new-tree peak that could `ENOSPC` a tight destination. The old late whole-tree commit is opt-in via `--delete-after` or the FastSync-only long spelling `--delete-commit`. **Abort/ordering parity (parity-2.29):** the complete per-directory plan set is transmitted before the first data frame, so a mid-transfer abort has already applied every planned removal exactly like rsync's generator (which runs ahead of its throttled sender); `-d/--dirs` uses the same per-directory plans (the generator records only the directories whose direct children it enumerated, so an untraversed subdirectory's mirror is shielded); and the sorted depth-first traversal makes the removal order — and therefore the survivor set under a partial `--max-delete` — match rsync exactly (`test_delete_boundary_parity.py`, `test_parity_order.py`). By default the destination mirror of a path the source scan pruned (filter/exclude/size rules) is **protected** from deletion — matching rsync, which does not delete excluded files under `--delete`; `--delete-excluded` opts back into deleting them (see below). Deletion is scoped to the **synchronized directories** sent on the wire (protocol 2.23.0), so a `--files-from` subset no longer deletes untransmitted paths outside the listed directory subtrees. The walk is bounded: a client `--max-delete=NUM` (or the 100000-entry server bound) makes it **partial** — entries up to the bound are removed, the rest are skipped, and the client exits **25** (`RERR_PARTIAL`), matching rsync, rather than failing the transfer. Extraneous destination symlinks are unlinked by name (never followed); a directory still holding a kept/protected entry is left behind rather than failing | | `--delete-before` | Delete before transfer | ✅ Parity | Implies `--delete`. The sender runs a full source pre-scan (paths only) and transmits the keep-set manifest BEFORE any file data; the receiver validates it, removes every destination entry not listed (bounded walk, staging-dir skip, protected prefixes honored), then acks `STATUS_OK`. The sender only starts streaming after the deletion committed, or aborts if the receiver reported a deletion error. By definition the deletions already happened when a later transfer phase fails — rsync's delete-before is destructive the same way; a subsequent failure does not restore the removed files. **Phase-0 divergence closed (no-wire):** both data passes now replay the exact file list the pre-scan built for the keep-set instead of re-reading the source — the single-threaded send loop and the `--threads` pipeline (whose scanner thread feeds the retained pre-scan chunks into the pipeline rather than re-scanning) — so a source file created after that scan is NOT transferred and its destination extra is deleted, exactly like rsync's single file list. The pre-scan captures the deferred directory times and the `--stats` directory count because no later scan runs (`test_delete_timing_parity.py::TestDeleteBeforeLateFileParity`, parametrized single-threaded vs `--threads=4`, differential vs rsync 3.4.1) | | `--del`, `--delete-during` | Delete during transfer | ✅ Parity | Both spellings accepted; imply `--delete`, and since lockstep track 6 this is also the default timing of a plain `--delete`. **Protocol 2.24.0 implements per-directory delete plans:** as the sender reaches each source directory it streams a `STATUS_DELETE_PLAN` for that directory and the receiver removes that directory's extras (verified with a byte-slicing proxy). The one-shot per-run config block (protected prefixes, size-pruned mirrors, `--delete-missing-args` exact paths) rides a dedicated config-only carrier frame with an `apply=false` flag, so it reaches the receiver even when the scope allows no directory plan at all (a `--files-from` list of bare files synchronizes no directory). **Abort/ordering parity (parity-2.29):** the complete plan set is transmitted before the first data frame, so on a mid-transfer abort every planned extra has already been removed exactly like rsync's generator (which runs ahead of its throttled sender); `-d/--dirs` no longer falls back to the end-of-transfer commit but records only the directories whose direct children it enumerated; and the sorted depth-first traversal makes the removal order — and the partial-`--max-delete` survivor set — identical to rsync (`test_delete_boundary_parity.py`, `test_parity_order.py`). `-R` plans are scoped to the transferred prefix subtree | | `--delete-delay` | Find deletions during, delete after | ✅ Parity | Implies `--delete`. **Protocol 2.24.0 implements rsync's delete-delay timing:** the sender records each directory's delete plan while scanning and the receiver commits those removals only after the whole transfer succeeds (per plan), so an extra created in the destination after its directory's plan survives while `--delete-after` re-scans and removes it, and a failed transfer removes nothing. The **reported** deleted count advances only on an actual removal. **Fixed (no-wire):** the `--max-delete` budget is now charged on ACTUAL removals (an unlink/rmdir that succeeded), not at plan/snapshot time, and a queued directory is re-scanned at commit and removed recursively (content created after the plan included), matching rsync: a snapshotted entry that fails or is skipped consumes no budget, so a later extra rsync would delete is still deleted. The deferred snapshot list keeps an independent hard cap (`DELETE_PLAN_SERVER_LIMIT`) so it cannot grow without bound now that the budget is no longer charged while scanning. A `--max-delete=2` partial delete reports exactly 2 and exits 25 in both tools, and the refilled-directory differential (late content removed, directory removed, budget shared) now matches rsync 3.4.1 on both sides (`test_delete_delay_budget_parity.py`, `test_delete_timing_parity.py`). Unit tests cover recursive removal, actual-removal charging, and the bounded deferred list. **Ordering parity (parity-2.29):** the sorted depth-first traversal plus the up-front plan set make the order in which extras are removed — and therefore the survivor set under a partial `--max-delete` — match rsync exactly (differential `test_parity_order.py::test_delete_delay_deletion_order_matches_rsync` and `::test_partial_max_delete_survivor_order_matches_rsync`) | @@ -964,7 +966,7 @@ These are the last compatibility items and the closing phase toward rsync flag p **Wire:** two trailing config-frame blocks after the `--iconv` spec, in fixed order — `send_privilege_options`/`receive_privilege_options` (one `super_mode` int, validated `0..2`), then `send_copy_as_options`/`receive_copy_as_options` (presence int + two int32 ids, validated `>= 0`, with `copy_as_set ⇒ use_metadata`). `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Divergences from rsync:** rsync's `--super` elevates the receiver and `--copy-as` actually switches its credentials; FastSync never elevates and only permits/forwards confined attempts, and `--copy-as` forces ownership rather than switching identity. -**Honest status after the parity 2.29 cycle (protocol 2.29.0 since the symlink-xattr wire wave, which adds no config-frame field and leaves this matrix unchanged), updated by the parity cycle 2.29 pass, the audit-cycle follow-ups, the triage cycle, and a later no-wire parity pass.** The wire backlog cycle (protocol 2.30.0) then moved `--stderr=MODE` ❌ → ⚠️ (the `client` mode is now accepted over the new `STATUS_CLIENT_MSG` channel; only the client->server direction is reproduced) and closed the `--devices` exit-code and `--remove-source-files` residuals via `STATUS_PARTIAL` (exit 23 with successful sources removed), leaving ✅ Parity 119 / ⚠️ Caveat 15 / ❌ Divergent 23 = 157 rows. The same cycle then extended the destination-state report to directories and symlinks (#314: the receiver answers `STATUS_MKDIR`/`STATUS_SYMLINK` and an ancestor probe, so `-i`/`--progress`/`--out-format` render `.d..t......`/`cLc........` instead of `cd`/`cL` and suppress unchanged entries) and appended the per-type `deleted_*` counters to `STATUS_STATS` (#316: `--stats` now reproduces rsync's `Number of deleted files (reg/dir/link/special)` breakdown) — both differential-tested against rsync 3.4.1; the affected rows' caveats narrow but their classifications are unchanged, so the matrix stays **119 ✅ / 15 ⚠️ / 23 ❌ = 157**. The no-wire parity pass accepted `--inc-recursive`/`--no-inc-recursive` as inert no-ops (❌ → ✅, since FastSync's full scan is rsync's `--no-inc-recursive` and the destination is identical), narrowed the `--temp-dir` divergence by accepting an absolute path that canonicalizes inside the receive root (the row stays ❌ for out-of-root absolute paths), closed the `--delete-before` phase-0 divergence (⚠️ → ✅: both the single-threaded and the `--threads` data passes now replay the pre-scan file list, so a source file created after the scan is neither transferred nor kept, matching rsync), and moved `--fake-super` and `--devices` ❌ → ⚠️ (`--fake-super` now writes/reads rsync's exact `user.rsync.%stat` key and ` , :` grammar, interoperating with real rsync 3.4.1 for regular files and faking char/block devices as regular files carrying the real rdev; `--devices` now logs a failed device `mknod` as a per-entry failure that continues the transfer instead of a silent non-root skip — see those rows for the remaining directory-faking and exit-code residuals). A review pass then hardened the fake-super stat parser (strict range-checked parsing), made rsync-style daemon modules read-only by default with a startup warning for accepted-but-unenforced access-control keys, and extended the `--delete-before` replay to the `--threads` path. The 2.29 cycle closed the scanner-order, delete-timing, relative-basis, and fuzzy-eligibility residuals (moving `-n`/`--delete`/`--del`/`--delete-delay` to ✅) and improved the `--info`/`--stats`/`--debug` partial rows; the triage cycle moved `-F` and `-i`/`--itemize-changes` ✅ → ⚠️ for their documented residuals. The remaining ⚠️ rows are `--info`, `--debug`, `--stderr=MODE`, `--msgs2stderr`, `--stats`, `--progress`, `-i`, `--filter`, `-F`, the three basis-dir options, `-y/--fuzzy`, `--fake-super`, and `--devices` (15). The wire cycle for issue #315 then closed the `--filter`/`-F` merge-modifier and per-directory-receiver residuals (protocol 2.30.0 carries each directory's compiled rules and implements `e`/`n`/`w`/`-`), narrowing both rows to the merge-file-side residual (rsync reads the destination's merge file, FastSync carries the source's) and leaving the tally at **119 ✅ / 15 ⚠️ / 23 ❌ = 157**. Earlier: **Honest status after the parity 2.28.0 cycle (protocol 2.28.0), updated by the rsync-parity-stats, rsync-parity-options, rsync-parity-fs, parity-review, no-wire parity-track-1/2b and wire parity-track-4a/5a passes.** ✅ Parity 116 / ⚠️ Caveat 14 / ❌ Divergent 27 = 157 rows. Earlier revisions of this document reported "143 ✅ / 0 divergence / 0 partial"; that conflated "parsed and tested" with "rsync parity", because many rows carried documented behavioral differences and some short options were not parsed at all. This reclassification makes every difference explicit. The completion wave closed 23 previously-caveated rows (9 that triage showed were already parity, plus 14 genuine fixes) and turned the 17 inherently non-rsync rows — native daemon config/auth, the FastSync batch container, the safe-subset device/privilege flags, `-X`'s privileged namespaces, `--fake-super`'s native xattr format, and the `--old-args` no-op — into explicit ❌ divergences. The stats pass flipped `--delete-delay` to ✅ (actual-removal accounting), but the parity-review pass moved it back to ⚠️ because FastSync charged the `--max-delete` budget at plan/snapshot time and left a refilled snapshotted directory in place, whereas rsync charges on actual removals and recursively removes a queued directory (including content created after its plan). The no-wire parity-track-1 pass fixed both (actual-removal charging plus recursive deferred removal with an independent deferred-list cap), narrowing the caveat to the partial-delete ordering. The stats pass also reclassified `--out-format` to ❌ (protocol-specific `%b`/delta-`%c`), and sharpened the `--stats`/`--progress`/`--checksum-choice` residuals. The options pass flipped `--bwlimit` and `--ignore-errors` to ✅ (rsync-exact size parsing and ~100 ms leaky-bucket throttling, and rsync's skip-unreadable-subdir plus IO-error-suppressed deletion with exit 23) and emits rsync-format `--info=name/flist/del/remove/nonreg/progress` lines (real-run `deleting`/`*deleting` carried over a new trailing `report_deletes` wire bool, `PROTOCOL_VERSION` 2.26.0 → 2.27.0), while reclassifying `-M` over daemon/TCP +**Honest status after the parity 2.29 cycle (protocol 2.29.0 since the symlink-xattr wire wave, which adds no config-frame field and leaves this matrix unchanged), updated by the parity cycle 2.29 pass, the audit-cycle follow-ups, the triage cycle, and a later no-wire parity pass.** The wire backlog cycle (protocol 2.30.0) then moved `--stderr=MODE` ❌ → ⚠️ (the `client` mode is now accepted over the new `STATUS_CLIENT_MSG` channel; only the client->server direction is reproduced) and closed the `--devices` exit-code and `--remove-source-files` residuals via `STATUS_PARTIAL` (exit 23 with successful sources removed), leaving ✅ Parity 119 / ⚠️ Caveat 15 / ❌ Divergent 23 = 157 rows. The same cycle then extended the destination-state report to directories and symlinks (#314: the receiver answers `STATUS_MKDIR`/`STATUS_SYMLINK` and an ancestor probe, so `-i`/`--progress`/`--out-format` render `.d..t......`/`cLc........` instead of `cd`/`cL` and suppress unchanged entries) and appended the per-type `deleted_*` counters to `STATUS_STATS` (#316: `--stats` now reproduces rsync's `Number of deleted files (reg/dir/link/special)` breakdown) — both differential-tested against rsync 3.4.1; the affected rows' caveats narrow but their classifications are unchanged, so the matrix stays **119 ✅ / 15 ⚠️ / 23 ❌ = 157**. The no-wire parity pass accepted `--inc-recursive`/`--no-inc-recursive` as inert no-ops (❌ → ✅, since FastSync's full scan is rsync's `--no-inc-recursive` and the destination is identical), narrowed the `--temp-dir` divergence by accepting an absolute path that canonicalizes inside the receive root (the row stays ❌ for out-of-root absolute paths), closed the `--delete-before` phase-0 divergence (⚠️ → ✅: both the single-threaded and the `--threads` data passes now replay the pre-scan file list, so a source file created after the scan is neither transferred nor kept, matching rsync), and moved `--fake-super` and `--devices` ❌ → ⚠️ (`--fake-super` now writes/reads rsync's exact `user.rsync.%stat` key and ` , :` grammar, interoperating with real rsync 3.4.1 for regular files and faking char/block devices as regular files carrying the real rdev; `--devices` now logs a failed device `mknod` as a per-entry failure that continues the transfer instead of a silent non-root skip — see those rows for the remaining directory-faking and exit-code residuals). A review pass then hardened the fake-super stat parser (strict range-checked parsing), made rsync-style daemon modules read-only by default with a startup warning for accepted-but-unenforced access-control keys, and extended the `--delete-before` replay to the `--threads` path. The 2.29 cycle closed the scanner-order, delete-timing, relative-basis, and fuzzy-eligibility residuals (moving `-n`/`--delete`/`--del`/`--delete-delay` to ✅) and improved the `--info`/`--stats`/`--debug` partial rows; the triage cycle moved `-F` and `-i`/`--itemize-changes` ✅ → ⚠️ for their documented residuals. The remaining ⚠️ rows are `--info`, `--debug`, `--stderr=MODE`, `--msgs2stderr`, `--stats`, `--progress`, `-i`, `--filter`, `-F`, the three basis-dir options, `-y/--fuzzy`, `--fake-super`, `--devices`, and `--delay-updates` (16). The wire cycle for issue #315 then closed the `--filter`/`-F` merge-modifier and per-directory-receiver residuals (protocol 2.30.0 carries each directory's compiled rules and implements `e`/`n`/`w`/`-`), narrowing both rows to the merge-file-side residual (rsync reads the destination's merge file, FastSync carries the source's) and leaving the tally at **119 ✅ / 15 ⚠️ / 23 ❌ = 157**; the #317 pass then moved `--delay-updates` ❌ → ⚠️, for **119 ✅ / 16 ⚠️ / 22 ❌ = 157**. Earlier: **Honest status after the parity 2.28.0 cycle (protocol 2.28.0), updated by the rsync-parity-stats, rsync-parity-options, rsync-parity-fs, parity-review, no-wire parity-track-1/2b and wire parity-track-4a/5a passes.** ✅ Parity 116 / ⚠️ Caveat 14 / ❌ Divergent 27 = 157 rows. Earlier revisions of this document reported "143 ✅ / 0 divergence / 0 partial"; that conflated "parsed and tested" with "rsync parity", because many rows carried documented behavioral differences and some short options were not parsed at all. This reclassification makes every difference explicit. The completion wave closed 23 previously-caveated rows (9 that triage showed were already parity, plus 14 genuine fixes) and turned the 17 inherently non-rsync rows — native daemon config/auth, the FastSync batch container, the safe-subset device/privilege flags, `-X`'s privileged namespaces, `--fake-super`'s native xattr format, and the `--old-args` no-op — into explicit ❌ divergences. The stats pass flipped `--delete-delay` to ✅ (actual-removal accounting), but the parity-review pass moved it back to ⚠️ because FastSync charged the `--max-delete` budget at plan/snapshot time and left a refilled snapshotted directory in place, whereas rsync charges on actual removals and recursively removes a queued directory (including content created after its plan). The no-wire parity-track-1 pass fixed both (actual-removal charging plus recursive deferred removal with an independent deferred-list cap), narrowing the caveat to the partial-delete ordering. The stats pass also reclassified `--out-format` to ❌ (protocol-specific `%b`/delta-`%c`), and sharpened the `--stats`/`--progress`/`--checksum-choice` residuals. The options pass flipped `--bwlimit` and `--ignore-errors` to ✅ (rsync-exact size parsing and ~100 ms leaky-bucket throttling, and rsync's skip-unreadable-subdir plus IO-error-suppressed deletion with exit 23) and emits rsync-format `--info=name/flist/del/remove/nonreg/progress` lines (real-run `deleting`/`*deleting` carried over a new trailing `report_deletes` wire bool, `PROTOCOL_VERSION` 2.26.0 → 2.27.0), while reclassifying `-M` over daemon/TCP and receiver-side `protect`/`risk` re-derivation to ❌ (no argv channel / receiver filter engine); the wire parity-track-4a pass later added that receiver filter engine, flipping `--filter=RULE` back to ✅ (see above; the @@ -1252,8 +1254,10 @@ These remain after the wave; the individual rows carry the precise wording. the source's). `--ignore-errors` exits 23 but its EACCES differential is not exercised in CI. -- **`--delay-updates`** uses a fixed staging name with an advisory lock and - deletes before publication; **`--temp-dir`** rejects out-of-root absolute and +- **`--delay-updates`** stages under a per-run unique name (refusing to wipe a + same-named destination entry) and publishes before its implied + `--delete-after` commit; it does not create a backup of a deleted extra. + **`--temp-dir`** rejects out-of-root absolute and foreign paths (in-root absolute paths are accepted; deliberately confined, see the row); **`--remote-option`** is SSH-only. **`--iconv`** now matches rsync's push direction (destination charset = the diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 9063b56..ce390a4 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -2626,6 +2626,17 @@ static int cli_finalize_config(Config* config, bool verbose, bool no_delta, bool bool debug_enabled = verbose || config->debug_level != 0; set_log_level(config->quiet ? LOG_LEVEL_ERROR : (debug_enabled ? LOG_LEVEL_DEBUG : LOG_LEVEL_WARNING)); + /* rsync parity: --delay-updates implies --delete-after. Every staged file is + published first and only then are extras removed. Normalize onto the + existing delete_after wire bool (no new wire field), overriding any other + explicit timing exactly as rsync does; without --delete there is no + deletion, so no timing is set (and the wire config stays valid). */ + if (config->delay_updates && config->use_delete) { + config->delete_before = false; + config->delete_during = false; + config->delete_delay = false; + config->delete_after = true; + } /* rsync's plain --delete defaults to delete-during (--del): each directory's extras are removed as that directory is processed, so space is freed progressively and a tight destination never has to hold the whole old+new diff --git a/src/server/receiver.c b/src/server/receiver.c index 25a183b..70823e5 100644 --- a/src/server/receiver.c +++ b/src/server/receiver.c @@ -797,15 +797,27 @@ int receiver_process_pending_ctx(Config* config, int file_descriptor, const Rece log_message(LOG_LEVEL_ERROR, "Did not receive FINISHED Status"); goto receive_error; } + /* --delay-updates: publish every staged file BEFORE the deferred delete + commit, matching rsync's --delete-after ordering (all updates land first, + then extras are removed). The single-threaded receiver stores files + synchronously, so every staged file is complete here. The -m receiver + hands both publication and deletion to its caller via + pending_manifest/pending_plans; that caller publishes first, after its disk + writer has drained. */ + bool handoff = state.pending_manifest != NULL || state.pending_plans != NULL; + if (!handoff && !config->dry_run && config->delay_updates && config->delay_context) { + if (!delay_updates_publish(config->delay_context, config)) { + send_status(file_descriptor, STATUS_ERROR); + goto fail; + } + } /* Commit-style (late) deletion: every data frame has been received and the sender proved the whole tree with STATUS_FINISHED. The single-threaded - receiver stores files synchronously, so everything is on disk here and the - deletion can be committed before the --delay-updates publication in - send_success (the walker skips the staging dir, so staged files are never - treated as extras). The -m receiver passes `pending_manifest` because its - disk writer may still be draining; the caller commits after the writer has - joined so no extra file is removed unless the transfer is known to have - succeeded. */ + receiver stores files synchronously, so everything is on disk here (and a + --delay-updates run has already published above). The -m receiver passes + `pending_manifest` because its disk writer may still be draining; the + caller commits after the writer has joined so no extra file is removed + unless the transfer is known to have succeeded. */ if (state.deferred_manifest) { if (state.pending_manifest) { *state.pending_manifest = state.deferred_manifest; @@ -989,21 +1001,10 @@ static bool receiver_send_success_frame(int fd, void* context_pointer) { nothing to publish and no directory times to stamp. */ if (context->config->dry_run) return receiver_send_final_success(fd, context->config, &context->outcomes, final_status); - /* --delay-updates: the whole protocol stream (including manifest/delete - handling, which ran inside receiver_process) has succeeded and every - staged file was fully written. Publish them atomically now, before the - success/outcome frame tells a --remove-source-files sender it may delete - its sources. */ - if (context->config->delay_updates && context->config->delay_context) { - if (!delay_updates_publish(context->config->delay_context, context->config)) { - send_status(fd, STATUS_ERROR); - return false; - } - } - /* P7 Wave D: every child is now written and the delete / --delay-updates - phases have committed, so it is finally safe to stamp directory times. - This runs after the deferred deletion because receiver_process commits it - before calling this success frame. */ + /* P7 Wave D: every child is now written and the --delay-updates publication + (done in receiver_process before the delete commit) plus the deferred + deletion have both committed, so it is finally safe to stamp directory + times. */ dir_metadata_list_apply(&context->dir_times, context->config->receive_root_directory, context->config); return receiver_send_final_success(fd, context->config, &context->outcomes, final_status); diff --git a/src/server/server.c b/src/server/server.c index f374ecc..aea5de3 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -981,12 +981,23 @@ static void server_run_mt_receiver(ServerSession* state) { thrd_join(writer, &writer_result); bool transfer_ok = receiver_result == thrd_success && writer_result == thrd_success; PipelineContextReceiver* context = state->context; + if (transfer_ok && !config->dry_run) { + /* --delay-updates: receive_thread has finished the whole protocol stream + and write_thread has drained its queue, so every staged file is complete. + Publish atomically BEFORE the deferred delete commit, matching rsync's + --delete-after ordering (all updates land first, then extras are + removed). */ + if (config->delay_updates && config->delay_context && + !delay_updates_publish(config->delay_context, config)) { + transfer_ok = false; + } + } if (transfer_ok && !config->dry_run) { /* Commit-style (late) deletion: receive_thread handed the keep-set manifest here instead of deleting while write_thread might still be - draining, so by now every file is on disk and the whole transfer is - known to have succeeded. Remove the extras before publishing a - --delay-updates run; the walker skips the staging directory. A + draining, so by now every file is on disk (and a --delay-updates run has + already published above) and the whole transfer is known to have + succeeded. The walker skips the staging directory. A server-contacting --dry-run deletes nothing (no manifest is sent). */ if (context->deferred_manifest) { size_t deleted = 0; @@ -1026,21 +1037,10 @@ static void server_run_mt_receiver(ServerSession* state) { delete_plan_session_destroy(context->deferred_plans); context->deferred_plans = NULL; } - } - if (transfer_ok && !config->dry_run) { - /* --delay-updates: receive_thread has finished the whole protocol stream - (including manifest/delete handling) and write_thread has drained its - queue, so every staged file is complete. Publish atomically before the - success/outcome frame so a --remove-source-files sender only learns of - files that were actually installed. */ - if (config->delay_updates && config->delay_context && - !delay_updates_publish(config->delay_context, config)) { - transfer_ok = false; - } - /* P7 Wave D: all writers have joined and the late deletion (and - --delay-updates publication) has committed above, so it is finally safe - to stamp directory times; a directory's mtime must not be clobbered by - its children or by an extra removal. */ + /* P7 Wave D: all writers have joined and the --delay-updates publication + plus the late deletion have committed above, so it is finally safe to + stamp directory times; a directory's mtime must not be clobbered by its + children or by an extra removal. */ if (transfer_ok) dir_metadata_list_apply(&context->dir_times, config->receive_root_directory, config); } diff --git a/src/shared/delay_updates.c b/src/shared/delay_updates.c index bfaca1b..1750b4a 100644 --- a/src/shared/delay_updates.c +++ b/src/shared/delay_updates.c @@ -8,13 +8,49 @@ #include #include #include +#include #include #include #include #include #include +#include #include +/* Process-wide counter so two staging contexts created in the same process (or + within the same clock tick) can never pick the same name. */ +static unsigned long long delay_updates_next_sequence(void) { + static atomic_ullong sequence; + return atomic_fetch_add_explicit(&sequence, 1, memory_order_relaxed); +} + +/* Build the per-run staging directory basename: the reserved prefix plus the + pid and an entropy token. A fixed name could collide with a genuine + destination entry; the token makes such a collision vanishingly unlikely and, + if it ever happens, prepare() refuses to touch the existing directory. */ +static char* delay_updates_make_staging_name(void) { + unsigned long long entropy = 0; + int fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC); + if (fd >= 0) { + ssize_t got = read(fd, &entropy, sizeof(entropy)); + close(fd); + if (got != (ssize_t)sizeof(entropy)) + entropy = 0; + } + if (entropy == 0) + entropy = ((unsigned long long)time(NULL) << 20) ^ ((unsigned long long)getpid() << 8) ^ + delay_updates_next_sequence(); + int length = snprintf(NULL, 0, DELAY_UPDATES_STAGING_DIR ".%ld.%llx", (long)getpid(), entropy); + if (length < 0) + return NULL; + char* name = malloc((size_t)length + 1); + if (!name) + return NULL; + snprintf(name, (size_t)length + 1, DELAY_UPDATES_STAGING_DIR ".%ld.%llx", (long)getpid(), + entropy); + return name; +} + DelayUpdatesContext* delay_updates_context_create(const char* root_directory) { if (!root_directory) return NULL; @@ -26,8 +62,15 @@ DelayUpdatesContext* delay_updates_context_create(const char* root_directory) { free(context); return NULL; } - context->staging_root = path_cat(root_directory, DELAY_UPDATES_STAGING_DIR); + context->staging_name = delay_updates_make_staging_name(); + if (!context->staging_name) { + free(context->root_directory); + free(context); + return NULL; + } + context->staging_root = path_cat(root_directory, context->staging_name); if (!context->staging_root) { + free(context->staging_name); free(context->root_directory); free(context); return NULL; @@ -39,6 +82,7 @@ DelayUpdatesContext* delay_updates_context_create(const char* root_directory) { context->lock_fd = -1; if (mtx_init(&context->mutex, mtx_plain) != thrd_success) { free(context->staging_root); + free(context->staging_name); free(context->root_directory); free(context); return NULL; @@ -54,6 +98,7 @@ void delay_updates_context_destroy(DelayUpdatesContext* context) { close(context->lock_fd); context->lock_fd = -1; free(context->staging_root); + free(context->staging_name); free(context->root_directory); for (size_t i = 0; i < context->count; i++) { free(context->entries[i].staged_path); @@ -125,48 +170,81 @@ bool delay_updates_prepare(DelayUpdatesContext* context) { return false; if (context->prepared) return true; - int fd = file_open_private_dir(context->staging_root); - if (fd < 0) { + /* Create the per-run staging directory with O_EXCL semantics. The name is + unique to this transfer, so if the path already exists it is NOT ours: + either a genuine destination entry that happens to share the name or a + leftover from another session. Refuse rather than wipe it -- the old + fixed-name design could destroy a real destination entry. A crash + leftover is never reused (the next run picks a fresh name). */ + char* leaf = NULL; + int parent_fd = file_open_secure_parent(context->staging_root, &leaf, true); + if (parent_fd < 0) { int saved_errno = errno; char* escaped = output_escape(context->staging_root, false); log_message(LOG_LEVEL_ERROR, "could not create --delay-updates staging directory '%s': %s", escaped ? escaped : "", strerror(saved_errno)); free(escaped); + free(leaf); return false; } - /* Hold an exclusive advisory lock on the staging directory for the whole - transfer. The staging directory name is fixed, so two simultaneous - delayed transfers to the same destination root would otherwise share it - and destroy each other's staged files. The lock makes the second session - fail cleanly instead of corrupting the first. The lock is released when - the context (and its file descriptor) is destroyed. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (fd >= 0) { + close(fd); + close(parent_fd); + char* escaped = output_escape(context->staging_root, false); + log_message(LOG_LEVEL_ERROR, + "--delay-updates staging directory '%s' already exists and is not owned by this " + "transfer; refusing to overwrite it", + escaped ? escaped : ""); + free(escaped); + free(leaf); + return false; + } + if (errno != ENOENT) { + int saved_errno = errno; + close(parent_fd); + char* escaped = output_escape(context->staging_root, false); + log_message(LOG_LEVEL_ERROR, "could not open --delay-updates staging directory '%s': %s", + escaped ? escaped : "", strerror(saved_errno)); + free(escaped); + free(leaf); + return false; + } + if (mkdirat(parent_fd, leaf, 0700) != 0) { + int saved_errno = errno; + close(parent_fd); + char* escaped = output_escape(context->staging_root, false); + log_message(LOG_LEVEL_ERROR, "could not create --delay-updates staging directory '%s': %s", + escaped ? escaped : "", strerror(saved_errno)); + free(escaped); + free(leaf); + return false; + } + fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + close(parent_fd); + free(leaf); + if (fd < 0) { + int saved_errno = errno; + char* escaped = output_escape(context->staging_root, false); + log_message(LOG_LEVEL_ERROR, "could not open --delay-updates staging directory '%s': %s", + escaped ? escaped : "", strerror(saved_errno)); + free(escaped); + return false; + } + /* Keep the exclusive advisory lock as defense in depth: the unique name + already prevents two sessions from sharing a staging directory, but the + lock also catches an improbable same-name collision that raced between the + existence check above and the open. */ if (flock(fd, LOCK_EX | LOCK_NB) != 0) { int saved_errno = errno; close(fd); - if (saved_errno == EWOULDBLOCK || saved_errno == EAGAIN) { - char* escaped = output_escape(context->staging_root, false); - log_message(LOG_LEVEL_ERROR, - "another --delay-updates transfer to '%s' is already in progress; refusing to " - "share the staging directory", - escaped ? escaped : ""); - free(escaped); - } else { - log_message(LOG_LEVEL_ERROR, "could not lock --delay-updates staging directory '%s': %s", - context->staging_root, strerror(saved_errno)); - } + char* escaped = output_escape(context->staging_root, false); + log_message(LOG_LEVEL_ERROR, "could not lock --delay-updates staging directory '%s': %s", + escaped ? escaped : "", strerror(saved_errno)); + free(escaped); return false; } context->lock_fd = fd; - /* Only now, with exclusive ownership, wipe leftovers from an interrupted - earlier transfer; this can never race with a live session. */ - bool ok = delay_wipe_dir_fd(fd); - if (!ok) { - log_message(LOG_LEVEL_ERROR, "could not clear stale --delay-updates staging files under '%s'", - context->staging_root); - close(context->lock_fd); - context->lock_fd = -1; - return false; - } context->prepared = true; return true; } @@ -264,12 +342,16 @@ static bool delay_publish_entry(DelayUpdatesContext* context, const Config* conf const StagedFileEntry* entry) { if (!delay_publish_backup(context, config, entry)) return false; - /* --force: an incoming regular file/symlink may replace a destination - DIRECTORY (possibly non-empty). The immediate-install path handles this in - file_receive; a --delay-updates run stages elsewhere and only discovers the - blocking directory here, so clear it before the rename (rsync's - "could not make way for new regular file" without --force). */ - if (config && config->force_delete && file_directory_exists_secure(entry->final_path)) { + /* An incoming regular file/symlink may replace a destination DIRECTORY that + blocks it. rsync removes the blocker recursively when --delete or --force + is active (its generator's "make way" deletion), and a --delay-updates run + stages elsewhere so it only discovers the blocker here. FastSync's + immediate-install path clears it too; without --delete/--force a non-empty + blocker fails the run (rsync's "could not make way for new regular file"). + use_delete is gated by the server --allow-delete policy, so a client can + never use this to bypass deletion authorization. */ + if (config && (config->force_delete || config->use_delete) && + file_directory_exists_secure(entry->final_path)) { if (!file_remove_tree_secure(entry->final_path)) { char* escaped = output_escape(entry->final_path, false); log_message(LOG_LEVEL_ERROR, "could not remove destination directory blocking '%s': %s", diff --git a/src/shared/delay_updates.h b/src/shared/delay_updates.h index 9e03311..e56907b 100644 --- a/src/shared/delay_updates.h +++ b/src/shared/delay_updates.h @@ -24,6 +24,7 @@ typedef struct { shared with the publish/cleanup phase that runs after the threads join. */ typedef struct DelayUpdatesContext { char* root_directory; /* receive root the staging dir lives under */ + char* staging_name; /* per-run unique staging dir basename */ char* staging_root; /* root_directory/ */ mtx_t mutex; StagedFileEntry* entries; @@ -33,7 +34,11 @@ typedef struct DelayUpdatesContext { int lock_fd; /* advisory exclusive flock held on the staging dir, or -1 */ } DelayUpdatesContext; -/* Name of the private staging subdirectory created under the receive root. */ +/* Reserved prefix for the private staging subdirectory created under the + receive root. The actual directory name is per-run unique (the prefix plus a + pid/entropy token) so it can never clobber a genuine destination entry that + happens to share the name; the bare prefix is still what a --backup-dir must + not collide with. */ #define DELAY_UPDATES_STAGING_DIR ".fastsync-stage" /* True when `dir` (ignoring a trailing "/") is the reserved staging directory diff --git a/src/shared/delete.c b/src/shared/delete.c index 0388cd5..942ec1c 100644 --- a/src/shared/delete.c +++ b/src/shared/delete.c @@ -39,12 +39,31 @@ FilterAction delete_protect_verdict(const DeleteProtectRules* protect, const cha const char* leaf, bool is_dir) { if (!protect) return FILTER_ACTION_NONE; + /* rsync protects its own --backup files from the delete pass: a name ending + in the backup suffix is never an extra. Checked before the filter rules so + an explicit exclude cannot be bypassed (the suffix is always a shield). */ + if (protect->backup_suffix && protect->backup_suffix[0] != '\0') { + size_t name_len = strlen(leaf); + size_t suffix_len = strlen(protect->backup_suffix); + if (name_len > suffix_len && + strcmp(leaf + (name_len - suffix_len), protect->backup_suffix) == 0) + return FILTER_ACTION_PROTECT; + } FilterAction action = filter_dir_rules_apply_side(protect->dir_rules, rel_path, leaf, is_dir); if (action != FILTER_ACTION_NONE) return action; return filter_rules_apply_side(protect->base_rules, rel_path, leaf, is_dir, FILTER_SIDE_RECEIVER); } +const char* delete_backup_suffix(const Config* config) { + if (!config || !config->backup || config->ignore_existing) + return NULL; + const char* suffix = config->suffix ? config->suffix : "~"; + if (!suffix[0] || strchr(suffix, '/')) + return NULL; + return suffix; +} + /* Classify a removed entry from its st_mode for the per-type delete counters. */ DeleteEntryType delete_entry_type_of_mode(mode_t mode) { if (S_ISDIR(mode)) @@ -631,7 +650,13 @@ bool delete_skips_build(const Config* config, const ArrayList* protected_paths, } int idx = 0; if (config->delay_updates) { - out->entries[idx].prefix = DELAY_UPDATES_STAGING_DIR; + /* Protect this transfer's actual (per-run unique) staging directory. The + runtime name is only known to the receiver-side context; fall back to the + reserved prefix for a context that was never created (e.g. a dry run). */ + const char* staging_name = (config->delay_context && config->delay_context->staging_name) + ? config->delay_context->staging_name + : DELAY_UPDATES_STAGING_DIR; + out->entries[idx].prefix = staging_name; out->entries[idx].top_level_only = true; idx++; } diff --git a/src/shared/delete.h b/src/shared/delete.h index 47c8eea..2c2d77d 100644 --- a/src/shared/delete.h +++ b/src/shared/delete.h @@ -37,6 +37,11 @@ typedef enum { typedef struct { const FilterRuleList* base_rules; const FilterRuleList* dir_rules; + /* When non-NULL and non-empty, a destination entry whose name ends with this + suffix is protected from deletion. rsync never treats a --backup file as + an extra, so a backup created at --delay-updates publication (or a + pre-existing one) survives the delete-after pass. */ + const char* backup_suffix; } DeleteProtectRules; /* rsync's first-match-wins receiver verdict for one candidate extra: the @@ -48,6 +53,11 @@ typedef struct { FilterAction delete_protect_verdict(const DeleteProtectRules* protect, const char* rel_path, const char* leaf, bool is_dir); +/* The backup suffix the delete walker must shield from deletion, or NULL when + --backup is inactive or the configured suffix is unusable (empty, or holding + a path separator). Matches the suffix file_save uses for backups. */ +const char* delete_backup_suffix(const Config* config); + /* One protected entry for the delete walker. When top_level_only is true the prefix is skipped only as a DIRECT child of dest_root (the --delay-updates staging directory, which must not hide genuine extras inside a nested diff --git a/src/shared/delete_commit.c b/src/shared/delete_commit.c index d175e9c..1360486 100644 --- a/src/shared/delete_commit.c +++ b/src/shared/delete_commit.c @@ -170,7 +170,8 @@ static bool delete_extras_budgeted_observed(const Config* config, const DeleteMa size_t deleted = 0; size_t skipped = 0; DeleteProtectRules protect = {.base_rules = config->protect_rules, - .dir_rules = manifest->per_dir_rules}; + .dir_rules = manifest->per_dir_rules, + .backup_suffix = delete_backup_suffix(config)}; DeleteWalkResult result = delete_extras_limited_observed( config->receive_root_directory, manifest->keeps, manifest->dirs, remaining, skips.entries, skips.count, &protect, &deleted, &skipped, observer, observer_context); @@ -398,7 +399,8 @@ bool manifest_would_delete_list(const Config* config, const DeleteManifest* mani if (!delete_skips_build(config, manifest->protected, NULL, true, &skips)) return false; DeleteProtectRules protect = {.base_rules = config->protect_rules, - .dir_rules = manifest->per_dir_rules}; + .dir_rules = manifest->per_dir_rules, + .backup_suffix = delete_backup_suffix(config)}; bool ok = delete_extras_list(config->receive_root_directory, manifest->keeps, manifest->dirs, skips.entries, skips.count, &protect, out, count_out); delete_skips_free(&skips); diff --git a/src/shared/delete_plan.c b/src/shared/delete_plan.c index 91e3bb4..0d2338c 100644 --- a/src/shared/delete_plan.c +++ b/src/shared/delete_plan.c @@ -801,6 +801,7 @@ static bool build_plan_skips(const Config* config, const DeletePlanSession* sess PlanSkips* out) { out->protect.base_rules = config->protect_rules; out->protect.dir_rules = session->per_dir_rules; + out->protect.backup_suffix = delete_backup_suffix(config); /* The per-directory plan walk keeps each basis path verbatim (it does not convert an absolute under-root path to its root-relative form, unlike the whole-tree commit walk). */ diff --git a/tests/integration/test_differential_parity.py b/tests/integration/test_differential_parity.py index b2c15af..6c82193 100644 --- a/tests/integration/test_differential_parity.py +++ b/tests/integration/test_differential_parity.py @@ -106,6 +106,15 @@ def seed_backup(_src, rroot, froot): _mk(os.path.join(root, "a.txt"), b"OLD-CONTENT\n", _OLD_MTIME) +def seed_delay_updates(_src, rroot, froot): + """A changed file plus an extra, so --delay-updates (and its implied + --delete-after) has both a publication and a deletion to order.""" + for root in (rroot, froot): + _mk(os.path.join(root, "a.txt"), b"OLD-CONTENT\n", _OLD_MTIME) + _mk(os.path.join(root, "extra.txt"), b"extra\n", _OLD_MTIME) + _mk(os.path.join(root, "extradir", "z.txt"), b"z\n", _OLD_MTIME) + + def seed_size_only(_src, rroot, froot): for root in (rroot, froot): _mk(os.path.join(root, "a.txt"), b"XXXXXXXXXXX\n", _OLD_MTIME) @@ -305,6 +314,14 @@ _CASES = [ H.Case("delete_commit", "basic", ["-a", "--delete-after"], seed=seed_extras, fastsync_flags=["-a", "--delete-commit"], server_args=DELETE, ref="FastSync-only --delete-commit == rsync --delete-after"), + # #317: --delay-updates stages under a per-run unique name and publishes + # every update before the implied --delete-after removes extras. + H.Case("delay_updates", "basic", ["-a", "--delay-updates"], + seed=seed_delay_updates, ref="--delay-updates stages then publishes"), + H.Case("delay_updates_delete", "basic", + ["-a", "--delay-updates", "--delete"], seed=seed_delay_updates, + server_args=DELETE, ci=True, + ref="--delay-updates implies --delete-after (publish before delete)"), H.Case("delete_excluded", "filters", ["-a", "--delete", "--delete-excluded", "--exclude=*.log"], seed=seed_delete_excluded, server_args=DELETE, ref="--delete-excluded"), diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index a93cfac..5602c4f 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -3089,12 +3089,12 @@ class TestDelayUpdates: "staging directory left behind after a successful delayed transfer" @pytest.mark.skipif(shutil.which("rsync") is None, reason="rsync not installed") - def test_delay_updates_staging_name_collision_residual(self): - """Documented residual (RSYNC_COMPAT.md `--delay-updates` row): FastSync - uses a fixed `.fastsync-stage` staging name and wipes a pre-existing tree - of that name at the start of a delayed run (crash-leftover cleanup), - even without `--delete`; rsync leaves a genuine destination entry of that - name untouched. Pins the divergence that keeps the row Divergent.""" + def test_delay_updates_staging_name_collision_preserved(self): + """rsync parity (RSYNC_COMPAT.md `--delay-updates` row): the receiver + stages under a per-run unique name, so a genuine pre-existing + destination entry named like the reserved staging prefix (`.fastsync- + stage`) is never wiped -- even without `--delete`. rsync likewise + leaves a real destination entry of its own temp name untouched.""" source = self._make_source("delay_collide_src") rdst = os.path.join(TEST_DATA_DIR, "delay_collide_rdst") fdst = os.path.join(TEST_DATA_DIR, "delay_collide_fdst") @@ -3120,8 +3120,11 @@ class TestDelayUpdates: result, _ = run_client(source, fdst, flags=["--delay-updates"], port=server.port) assert result.returncode == 0, (result.stderr or result.stdout)[:300] - assert not os.path.exists(os.path.join(fdst, self.STAGING)), \ - "FastSync did not wipe the reserved staging name (residual changed)" + assert _read_file(os.path.join(fdst, self.STAGING, "keepme.txt")) == b"genuine user data\n", \ + "FastSync destroyed a genuine destination entry named like the staging prefix" + # The per-run staging directory itself is removed after a clean run. + leftovers = [n for n in os.listdir(fdst) if n.startswith(self.STAGING + ".")] + assert leftovers == [], f"per-run staging directories left behind: {leftovers}" @pytest.mark.parametrize("mt", [False, True]) def test_delay_updates_incremental_rerun_no_leftovers(self, shared_server, mt): @@ -3161,10 +3164,11 @@ class TestDelayUpdates: @pytest.mark.parametrize("mt", [False, True]) def test_delete_with_delay_updates(self, mt): - """--delete runs before publication, so the delete walker must not treat - the staging directory as a set of extras: a changed file must still be - published after genuine extras are removed. Uses its own server started - with --allow-delete (the shared session server refuses deletion).""" + """rsync parity: --delay-updates implies --delete-after, so every staged + update is published first and the genuine extras are removed only after + that (the delete walker must never treat the staging directory as a set + of extras). Uses its own server started with --allow-delete (the shared + session server refuses deletion).""" source = os.path.join(TEST_DATA_DIR, "delay_delete_src") dest = os.path.join(TEST_DATA_DIR, "delay_delete_dst") clean_dir(source) @@ -3194,6 +3198,65 @@ class TestDelayUpdates: assert not os.path.exists(os.path.join(received, "extra.txt")), \ "genuine extra file was not deleted" assert not os.path.isdir(os.path.join(dest, self.STAGING)) + assert [n for n in os.listdir(dest) if n.startswith(self.STAGING + ".")] == [] + + @pytest.mark.parametrize("mt", [False, True]) + def test_delay_updates_delete_keeps_backup(self, mt): + """The --backup/--delay-updates interplay: the old destination file is + moved aside at publication, and that backup survives the implied + --delete-after pass (rsync never treats a backup file as an extra).""" + source = os.path.join(TEST_DATA_DIR, "delay_bak_del_src") + dest = os.path.join(TEST_DATA_DIR, "delay_bak_del_dst") + clean_dir(source) + clean_dir(dest) + with open(os.path.join(source, "f.txt"), "wb") as fh: + fh.write(b"NEW") + received = get_dest_received_dir(dest, source) + os.makedirs(received, exist_ok=True) + with open(os.path.join(received, "f.txt"), "wb") as fh: + fh.write(b"OLD") + os.utime(os.path.join(received, "f.txt"), (1_500_000_000, 1_500_000_000)) + # A pre-existing backup-looking extra must also be shielded. + with open(os.path.join(received, "stale.txt~"), "wb") as fh: + fh.write(b"stale backup") + with ServerManager() as server: + server.start(extra_args=["--allow-delete"]) + flags = ["--delete", "--backup", "--delay-updates"] + (["--threads"] if mt else []) + result, _ = run_client(source, dest, flags=flags, port=server.port) + assert result.returncode == 0, (result.stderr or result.stdout)[:300] + assert _read_file(os.path.join(received, "f.txt")) == b"NEW" + assert _read_file(os.path.join(received, "f.txt~")) == b"OLD", \ + "the publication backup was removed by the delete-after pass" + assert os.path.exists(os.path.join(received, "stale.txt~")), \ + "a pre-existing backup-suffixed entry was deleted" + + @pytest.mark.parametrize("mt", [False, True]) + def test_delay_updates_failed_run_leaves_no_staged_files(self, shared_server, mt): + """A run that fails before publication installs nothing and removes the + per-run staging directory (no staged leftovers).""" + source = os.path.join(TEST_DATA_DIR, "delay_fail_src") + dest = os.path.join(TEST_DATA_DIR, "delay_fail_dst") + clean_dir(source) + clean_dir(dest) + with open(os.path.join(source, "top.txt"), "wb") as fh: + fh.write(b"top\n") + os.makedirs(os.path.join(source, "sub")) + with open(os.path.join(source, "sub", "deep.txt"), "wb") as fh: + fh.write(b"deep\n") + # Plant a regular file where the "sub" directory must be created so the + # nested publish fails (the top-level file still publishes first). + received = get_dest_received_dir(dest, source) + os.makedirs(received) + with open(os.path.join(received, "sub"), "wb") as fh: + fh.write(b"blocker") + + flags = ["--delay-updates"] + (["--threads"] if mt else []) + result, _ = run_client(source, dest, flags=flags, port=shared_server.port) + assert result.returncode != 0, "a blocked nested publish must fail the run" + assert not os.path.lexists(os.path.join(received, "sub", "deep.txt")), \ + "a staged file appeared despite the failed run" + assert [n for n in os.listdir(dest) if n.startswith(self.STAGING + ".")] == [], \ + "the per-run staging directory survived a failed run" def test_delay_updates_rejects_reserved_backup_dir(self): """--backup-dir equal to the internal staging name must be rejected so diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 49c7f02..e4001b0 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -2977,6 +2977,48 @@ static void test_parse_args_delay_updates() { config_delete(cfg); } +/* rsync parity: --delay-updates implies --delete-after when --delete is + active (all updates publish first, then extras are removed). An explicit + other timing is overridden; without --delete no timing is set. */ +static void test_parse_args_delay_updates_implies_delete_after() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "--delay-updates", "--delete", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); + EXPECT_TRUE(cfg->use_delete); + EXPECT_TRUE(cfg->delete_after); + EXPECT_FALSE(cfg->delete_before); + EXPECT_FALSE(cfg->delete_during); + EXPECT_FALSE(cfg->delete_delay); + cfg->send_directory = str_dup("/src"); + cfg->receive_root_directory = str_dup("/dst"); + EXPECT_TRUE(validate_config(cfg)); + config_delete(cfg); + + /* An explicit conflicting timing is normalized to delete-after. */ + cfg = config_create(); + char* argv_before[] = {"fastsync", "--delay-updates", "--delete-before", "/src", "/dst"}; + positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 5, argv_before, positional_args, &positional_count), 0); + EXPECT_TRUE(cfg->use_delete); + EXPECT_TRUE(cfg->delete_after); + EXPECT_FALSE(cfg->delete_before); + config_delete(cfg); + + /* Without --delete there is no deletion, so no timing is selected. */ + cfg = config_create(); + char* argv_alone[] = {"fastsync", "--delay-updates", "/src", "/dst"}; + positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_alone, positional_args, &positional_count), 0); + EXPECT_FALSE(cfg->use_delete); + EXPECT_FALSE(cfg->delete_after); + EXPECT_FALSE(cfg->delete_before); + EXPECT_FALSE(cfg->delete_during); + EXPECT_FALSE(cfg->delete_delay); + config_delete(cfg); +} + /* rsync rejects --delay-updates with --inplace; FastSync must too. */ static void test_validate_config_delay_updates_rejects_inplace() { Config* cfg = valid_client_config(); @@ -5312,6 +5354,7 @@ void test_client_cli() { test_parse_args_checksum_seed(); test_parse_args_temp_dir(); test_parse_args_delay_updates(); + test_parse_args_delay_updates_implies_delete_after(); test_validate_config_delay_updates_rejects_inplace(); test_validate_config_delay_updates_rejects_reserved_backup_dir(); test_parse_args_files_from(); diff --git a/tests/test_delay_updates.c b/tests/test_delay_updates.c index 4ff9d81..809e750 100644 --- a/tests/test_delay_updates.c +++ b/tests/test_delay_updates.c @@ -87,8 +87,10 @@ static void test_delay_updates_no_final_before_publish() { const char* final_path = "test_delay_tmp/sub/file.txt"; /* Before publication the final destination must not contain the file. */ EXPECT_FALSE(file_path_exists_secure(final_path)); - /* The complete staged copy must live inside the staging tree. */ - char* staged = path_cat("test_delay_tmp/.fastsync-stage", "/sub/file.txt"); + /* The complete staged copy must live inside the per-run staging tree. */ + EXPECT_NOT_NULL(cfg->delay_context->staging_name); + EXPECT_EQ_INT(strncmp(cfg->delay_context->staging_name, ".fastsync-stage.", 16), 0); + char* staged = path_cat(cfg->delay_context->staging_root, "/sub/file.txt"); EXPECT_NOT_NULL(staged); // cppcheck-suppress knownConditionTrueFalse if (staged) { @@ -125,6 +127,8 @@ static void test_delay_updates_publish_installs_files() { const char* final_path = "test_delay_pub_tmp/sub/file.txt"; EXPECT_FALSE(file_path_exists_secure(final_path)); + char* staging_root = str_dup(cfg->delay_context->staging_root); + EXPECT_NOT_NULL(staging_root); EXPECT_TRUE(delay_updates_publish(cfg->delay_context, cfg)); /* After a successful publish the file is installed and staging is gone. */ char* content = read_all(final_path); @@ -134,7 +138,8 @@ static void test_delay_updates_publish_installs_files() { EXPECT_EQ_STR(content, "published payload"); free(content); } - EXPECT_FALSE(file_path_exists_secure("test_delay_pub_tmp/.fastsync-stage")); + EXPECT_FALSE(file_path_exists_secure(staging_root)); + free(staging_root); out: file_destroy(f); @@ -158,11 +163,17 @@ static void test_delay_updates_cleanup_removes_staged() { goto out; EXPECT_EQ_INT(file_save_to_disk_full(root, f, cfg), FILE_SAVE_WRITTEN); - EXPECT_TRUE(file_path_exists_secure("test_delay_clean_tmp/.fastsync-stage/sub/file.txt")); + char* staged_file = path_cat(cfg->delay_context->staging_root, "/sub/file.txt"); + char* staging_root = str_dup(cfg->delay_context->staging_root); + EXPECT_NOT_NULL(staged_file); + EXPECT_NOT_NULL(staging_root); + EXPECT_TRUE(file_path_exists_secure(staged_file)); delay_updates_cleanup(cfg->delay_context); - EXPECT_FALSE(file_path_exists_secure("test_delay_clean_tmp/.fastsync-stage")); + EXPECT_FALSE(file_path_exists_secure(staging_root)); EXPECT_FALSE(file_path_exists_secure("test_delay_clean_tmp/sub/file.txt")); + free(staged_file); + free(staging_root); out: file_destroy(f); @@ -274,6 +285,54 @@ out: remove_tree(root); } +/* Every context picks its own staging directory name, so two delayed + transfers to the same root can never share (and corrupt) a staging tree. */ +static void test_delay_updates_unique_staging_name() { + DelayUpdatesContext* first = delay_updates_context_create("test_delay_uniq_tmp"); + DelayUpdatesContext* second = delay_updates_context_create("test_delay_uniq_tmp"); + EXPECT_NOT_NULL(first); + EXPECT_NOT_NULL(second); + if (first && second) { + EXPECT_EQ_INT(strncmp(first->staging_name, ".fastsync-stage.", 16), 0); + EXPECT_EQ_INT(strncmp(second->staging_name, ".fastsync-stage.", 16), 0); + EXPECT_TRUE(strcmp(first->staging_name, second->staging_name) != 0); + EXPECT_TRUE(strcmp(first->staging_root, second->staging_root) != 0); + } + delay_updates_context_destroy(first); + delay_updates_context_destroy(second); +} + +/* A pre-existing destination entry at the exact (random) staging path is not + ours: prepare() must refuse rather than wipe it. */ +static void test_delay_updates_prepare_refuses_non_owned_collision() { + const char* root = "test_delay_collide_tmp"; + remove_tree(root); + DelayUpdatesContext* context = delay_updates_context_create(root); + EXPECT_NOT_NULL(context); + // cppcheck-suppress knownConditionTrueFalse + if (!context) + return; + /* Plant a genuine directory with user data at the exact staging path. */ + EXPECT_TRUE(file_ensure_directory_secure(context->staging_root)); + char* inner = path_cat(context->staging_root, "keepme.txt"); + EXPECT_NOT_NULL(inner); + // cppcheck-suppress knownConditionTrueFalse + if (inner) { + EXPECT_TRUE(file_write_to_disk(inner, "genuine", 7, false, false)); + EXPECT_FALSE(delay_updates_prepare(context)); + char* content = read_all(inner); + EXPECT_NOT_NULL(content); + // cppcheck-suppress knownConditionTrueFalse + if (content) { + EXPECT_EQ_STR(content, "genuine"); + free(content); + } + free(inner); + } + delay_updates_context_destroy(context); + remove_tree(root); +} + /* The reserved staging name must be recognizable for validation, including with a trailing slash. */ static void test_delay_updates_reserved_name_helper() { @@ -288,6 +347,8 @@ static void test_delay_updates_reserved_name_helper() { void test_delay_updates() { test_delay_updates_reserved_name_helper(); + test_delay_updates_unique_staging_name(); + test_delay_updates_prepare_refuses_non_owned_collision(); test_delay_updates_no_final_before_publish(); test_delay_updates_publish_installs_files(); test_delay_updates_cleanup_removes_staged();