fix: --delay-updates delete/backup collisions, publish-failure test, staging lock
Review fixes for --delay-updates: - --delete no longer deletes the staged files: the delete walker gains a skip_root_child parameter and receive_manifest passes DELAY_UPDATES_STAGING_DIR when delay_updates is active, so deletion removes genuine extras while the staging dir (a direct child of the receive root) is left for publication in both single and -m modes. - --backup-dir is rejected when it collides with the reserved internal staging name .fastsync-stage (trailing slash normalized), in client validation and in the received-config wire validation, preventing old backups from being silently installed as new files. - Staging dir is now held under an exclusive advisory flock for the whole transfer (context lifetime): two simultaneous delayed transfers to one destination root no longer share/destroy each other's staged data - the second fails cleanly. Cleanup only touches the staging dir when this context owns the lock, so a lock-contention failure cannot wipe a live session. - Post-publish staging cleanup now returns/logs instead of discarding failures (warning when the staging dir cannot be fully removed). - Reworked the publish-failure integration test to exercise real mid-publish semantics (top-level file published, nested rename fails, no rollback, sources retained under --remove-source-files) and added integration tests for --delete + --delay-updates ordering and reserved --backup-dir rejection. - RSYNC_COMPAT note documents delete ordering, the reserved-name hazard, and the concurrency guard.
This commit is contained in:
@@ -12,7 +12,7 @@ from common import (
|
||||
PROJECT_ROOT, BUILD_DIR, TEST_DATA_DIR,
|
||||
run_client,
|
||||
generate_test_files, verify_transfer, clean_dir, make_result,
|
||||
get_dest_received_dir, CLIENT_CMD,
|
||||
get_dest_received_dir, CLIENT_CMD, ServerManager,
|
||||
)
|
||||
|
||||
SOURCE_DIR = os.path.join(TEST_DATA_DIR, "feature_source")
|
||||
@@ -1494,30 +1494,103 @@ class TestDelayUpdates:
|
||||
assert not os.path.isdir(os.path.join(dest, self.STAGING))
|
||||
|
||||
@pytest.mark.parametrize("mt", [False, True])
|
||||
def test_abort_publish_failure_installs_nothing(self, shared_server, mt):
|
||||
"""A deterministic publication failure must fail the transfer, leave no
|
||||
file in the final destination, and clean up the staging area. A plain
|
||||
file is planted where the final destination directory must be created,
|
||||
so the very first stage->publish rename fails (mkdir is impossible on
|
||||
top of a file even for root). Exercised in both single and -m modes so
|
||||
the multithreaded publish-once ordering is covered."""
|
||||
source = self._make_source("delay_abort_src")
|
||||
dest = os.path.join(TEST_DATA_DIR, "delay_abort_dst")
|
||||
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)."""
|
||||
source = os.path.join(TEST_DATA_DIR, "delay_delete_src")
|
||||
dest = os.path.join(TEST_DATA_DIR, "delay_delete_dst")
|
||||
clean_dir(source)
|
||||
clean_dir(dest)
|
||||
with open(os.path.join(source, "f.txt"), "wb") as fh:
|
||||
fh.write(b"AAAA")
|
||||
with open(os.path.join(source, "extra.txt"), "wb") as fh:
|
||||
fh.write(b"seed extra")
|
||||
with ServerManager() as server:
|
||||
server.start(extra_args=["--allow-delete"])
|
||||
result, _ = run_client(source, dest, port=server.port)
|
||||
assert result.returncode == 0, f"seed sync failed: {result.stderr[:200]}"
|
||||
received = get_dest_received_dir(dest, source)
|
||||
assert _read_file(os.path.join(received, "extra.txt")) == b"seed extra"
|
||||
|
||||
# Second source state: f.txt changed, extra.txt removed from source.
|
||||
with open(os.path.join(source, "f.txt"), "wb") as fh:
|
||||
fh.write(b"BBBB")
|
||||
os.remove(os.path.join(source, "extra.txt"))
|
||||
|
||||
flags = ["--delete", "--delay-updates"] + (["-m"] if mt else [])
|
||||
result, _ = run_client(source, dest, flags=flags, port=server.port)
|
||||
assert result.returncode == 0, \
|
||||
f"delete+delay-updates sync failed: {result.stderr[:200]}"
|
||||
assert _read_file(os.path.join(received, "f.txt")) == b"BBBB", \
|
||||
"changed file was not published after deletion"
|
||||
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))
|
||||
|
||||
def test_delay_updates_rejects_reserved_backup_dir(self):
|
||||
"""--backup-dir equal to the internal staging name must be rejected so
|
||||
an old backup can never be silently installed as the "new" file."""
|
||||
source = self._make_source("delay_reserved_bak_src")
|
||||
for variant, suffix in (("bare", ""), ("slash", "/")):
|
||||
dest = os.path.join(TEST_DATA_DIR, f"delay_reserved_bak_{variant}_dst")
|
||||
clean_dir(dest)
|
||||
flags = ["--delay-updates", "--backup", "--backup-dir",
|
||||
".fastsync-stage" + suffix]
|
||||
result, _ = run_client(source, dest, flags=flags, port=None)
|
||||
assert result.returncode != 0, \
|
||||
f"reserved --backup-dir '{suffix}' was accepted"
|
||||
assert not os.path.isdir(os.path.join(dest, self.STAGING)), \
|
||||
"staging directory created by a rejected run"
|
||||
|
||||
@pytest.mark.parametrize("remove_source_files", [False, True])
|
||||
@pytest.mark.parametrize("mt", [False, True])
|
||||
def test_mid_publish_failure_keeps_published_no_rollback(self, shared_server, mt,
|
||||
remove_source_files):
|
||||
"""A stage->publish rename failing part way through publication must
|
||||
fail the whole transfer, keep the already-published top-level file (no
|
||||
rollback), leave the not-yet-published nested file absent, and clean up
|
||||
the staging area. A regular file is planted where the final "sub"
|
||||
directory must be created, so the nested rename fails (mkdir over a
|
||||
file is impossible even for root) while the top-level file, which is
|
||||
always staged first, publishes. With --remove-source-files the sender
|
||||
must keep every source because no success/outcome frame is ever sent."""
|
||||
source = os.path.join(TEST_DATA_DIR, "delay_mid_src")
|
||||
dest = os.path.join(TEST_DATA_DIR, "delay_mid_dst")
|
||||
clean_dir(source)
|
||||
clean_dir(dest)
|
||||
top_path = os.path.join(source, "top.txt")
|
||||
deep_path = os.path.join(source, "sub", "deep.txt")
|
||||
with open(top_path, "wb") as fh:
|
||||
fh.write(b"top payload\n")
|
||||
os.makedirs(os.path.dirname(deep_path))
|
||||
with open(deep_path, "wb") as fh:
|
||||
fh.write(b"deep payload\n")
|
||||
|
||||
received = get_dest_received_dir(dest, source)
|
||||
os.makedirs(os.path.dirname(received), exist_ok=True)
|
||||
with open(received, "wb") as fh:
|
||||
fh.write(b"blocks the destination directory")
|
||||
os.makedirs(received)
|
||||
with open(os.path.join(received, "sub"), "wb") as fh:
|
||||
fh.write(b"blocks the nested destination directory")
|
||||
|
||||
flags = ["--delay-updates"] + (["-m"] if mt else [])
|
||||
if remove_source_files:
|
||||
flags += ["--remove-source-files"]
|
||||
result, _ = run_client(source, dest, flags=flags, port=shared_server.port)
|
||||
assert result.returncode != 0, "blocked destination publish did not fail"
|
||||
for root, _dirs, files in os.walk(source):
|
||||
for name in files:
|
||||
rel = os.path.relpath(os.path.join(root, name), source)
|
||||
assert not os.path.exists(os.path.join(received, rel)), \
|
||||
f"file appeared at final destination despite failed publish: {rel}"
|
||||
assert result.returncode != 0, "blocked nested publish did not fail"
|
||||
|
||||
# The top-level file was published before the nested rename failed and
|
||||
# is intentionally NOT rolled back.
|
||||
assert _read_file(os.path.join(received, "top.txt")) == b"top payload\n"
|
||||
# The nested file was never published.
|
||||
assert not os.path.lexists(os.path.join(received, "sub", "deep.txt")), \
|
||||
"nested file appeared despite a failed publish"
|
||||
assert not os.path.isdir(os.path.join(dest, self.STAGING)), \
|
||||
"staging leftovers after a failed publish"
|
||||
"staging leftovers after a failed mid-publish"
|
||||
# Sources survive: no success frame was sent, so a remove-source-files
|
||||
# sender must not delete anything.
|
||||
assert os.path.isfile(top_path)
|
||||
assert os.path.isfile(deep_path)
|
||||
|
||||
@pytest.mark.parametrize("mt", [False, True])
|
||||
def test_remove_source_files_keeps_receiver_skipped_source(self, shared_server, mt):
|
||||
|
||||
@@ -1287,6 +1287,27 @@ static void test_validate_config_delay_updates_rejects_inplace() {
|
||||
config_delete(cfg);
|
||||
}
|
||||
|
||||
/* --backup-dir may not collide with the internal --delay-updates staging
|
||||
directory (with or without a trailing slash), or old backups would silently
|
||||
be installed as the "new" file. */
|
||||
static void test_validate_config_delay_updates_rejects_reserved_backup_dir() {
|
||||
static const char* const reserved[] = {".fastsync-stage", ".fastsync-stage/"};
|
||||
for (size_t i = 0; i < sizeof(reserved) / sizeof(reserved[0]); i++) {
|
||||
Config* cfg = valid_client_config();
|
||||
cfg->delay_updates = true;
|
||||
cfg->backup_dir = str_dup(reserved[i]);
|
||||
EXPECT_FALSE(validate_config(cfg));
|
||||
config_delete(cfg);
|
||||
}
|
||||
|
||||
/* A non-colliding backup dir is fine alongside --delay-updates. */
|
||||
Config* ok = valid_client_config();
|
||||
ok->delay_updates = true;
|
||||
ok->backup_dir = str_dup("backups");
|
||||
EXPECT_TRUE(validate_config(ok));
|
||||
config_delete(ok);
|
||||
}
|
||||
|
||||
void test_client_cli() {
|
||||
test_validate_config_required_paths();
|
||||
test_validate_config_incompatible_options();
|
||||
@@ -1368,4 +1389,5 @@ void test_client_cli() {
|
||||
test_parse_args_temp_dir();
|
||||
test_parse_args_delay_updates();
|
||||
test_validate_config_delay_updates_rejects_inplace();
|
||||
test_validate_config_delay_updates_rejects_reserved_backup_dir();
|
||||
}
|
||||
|
||||
@@ -408,6 +408,30 @@ static void test_config_temp_dir_roundtrip() {
|
||||
config_delete(c);
|
||||
}
|
||||
|
||||
static void test_config_delay_updates_reserved_backup_rejected() {
|
||||
if (is_running_under_valgrind())
|
||||
return;
|
||||
Config* c = config_create();
|
||||
EXPECT_NOT_NULL(c);
|
||||
c->send_directory = str_dup("/src");
|
||||
c->receive_root_directory = str_dup("/dst");
|
||||
c->delay_updates = true;
|
||||
c->backup_dir = str_dup(".fastsync-stage");
|
||||
/* The receiver-side wire validation must reject a --backup-dir that collides
|
||||
with the internal delay-updates staging directory. */
|
||||
EXPECT_FALSE(roundtrip_config_ok(c));
|
||||
config_delete(c);
|
||||
|
||||
c = config_create();
|
||||
EXPECT_NOT_NULL(c);
|
||||
c->send_directory = str_dup("/src");
|
||||
c->receive_root_directory = str_dup("/dst");
|
||||
c->delay_updates = true;
|
||||
c->backup_dir = str_dup("backups");
|
||||
EXPECT_TRUE(roundtrip_config_ok(c));
|
||||
config_delete(c);
|
||||
}
|
||||
|
||||
static void test_config_is_remote_dest() {
|
||||
/* Valid SSH-style destinations */
|
||||
EXPECT_TRUE(config_is_remote_dest("user@host:/path"));
|
||||
@@ -443,6 +467,7 @@ void test_config() {
|
||||
test_config_receive_truncated();
|
||||
test_config_string_null_vs_empty_roundtrip();
|
||||
test_config_temp_dir_roundtrip();
|
||||
test_config_delay_updates_reserved_backup_rejected();
|
||||
}
|
||||
test_config_is_remote_dest();
|
||||
}
|
||||
|
||||
@@ -274,7 +274,20 @@ out:
|
||||
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() {
|
||||
EXPECT_TRUE(delay_updates_staging_name_conflict(".fastsync-stage"));
|
||||
EXPECT_TRUE(delay_updates_staging_name_conflict(".fastsync-stage/"));
|
||||
EXPECT_TRUE(delay_updates_staging_name_conflict(".fastsync-stage///"));
|
||||
EXPECT_FALSE(delay_updates_staging_name_conflict(NULL));
|
||||
EXPECT_FALSE(delay_updates_staging_name_conflict(""));
|
||||
EXPECT_FALSE(delay_updates_staging_name_conflict("backups"));
|
||||
EXPECT_FALSE(delay_updates_staging_name_conflict(".fastsync-stage.bak"));
|
||||
}
|
||||
|
||||
void test_delay_updates() {
|
||||
test_delay_updates_reserved_name_helper();
|
||||
test_delay_updates_no_final_before_publish();
|
||||
test_delay_updates_publish_installs_files();
|
||||
test_delay_updates_cleanup_removes_staged();
|
||||
|
||||
Reference in New Issue
Block a user