From a7f5f5e0ff6596c18aa9e1c833208c3b0e088046 Mon Sep 17 00:00:00 2001 From: Steven Rhoods Date: Wed, 12 Aug 2026 20:50:56 +0100 Subject: [PATCH] Handle a source path changing type against a non-empty destination A source directory replaced by a symlink (or file, or special) between passes left the destination stuck forever: remove_dst's plain unlinkat/rmdir fails with ENOTEMPTY against the still-populated old directory, then the create call that follows (symlinkat, mkdirat, mknodat, or open-for-copy) fails too (EEXIST) since the old object is still there. Reported live for the directory->symlink case, but remove_dst is the single shared helper every d_type transition in handle_entry's switch already routes through, so the bug applied to any type change against a non-empty former directory, not just symlinks. remove_dst now renames a non-empty directory aside under a dedicated .drsync.stale.-.. prefix (distinct from temp_prefix, so the orphan sweep's temp-reclaim logic never mistakes it for live copy residue) and journals the new name as an ordinary JR_ORPHAN, instead of just failing. The caller then creates the new object at the now-vacated original name in the same pass. The stale subtree is picked up and removed by the next explicit DELETE pass through the same fan-out machinery any other orphan already uses - safe at arbitrary depth/size, no new coordinator-side code needed. Adds type_change_e2e.sh: a directory with nested content, synced normally, then replaced on the source with a symlink - verifies the symlink lands with the right target, the old content is renamed aside and journaled as an orphan, and a subsequent delete pass fully reclaims it while leaving the new symlink untouched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm --- .github/workflows/ci.yml | 1 + README.md | 1 + agent/src/walker.c | 78 ++++++++++++++++--- docs/DESIGN-agent.md | 37 ++++++++- test/type_change_e2e.sh | 162 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 269 insertions(+), 10 deletions(-) create mode 100755 test/type_change_e2e.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2f9119..792f79e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -127,6 +127,7 @@ jobs: - scale_e2e # pathological shapes: huge dir, huge file - temp_reclaim_e2e # sweep reclaims crash residue, spares live temps - tls_e2e # mTLS, auth enforcement, reconnect-resume + - type_change_e2e # source path changes type (dir->symlink) between passes - ucopy_e2e # io_uring copy path with server-side copy disabled steps: - uses: actions/checkout@v7 diff --git a/README.md b/README.md index ddb1b98..be275b4 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ tree, and builds the binaries it needs itself: | `scale_e2e.sh` | pathological shapes: huge directory, huge file | | `temp_reclaim_e2e.sh` | sweep reclaims crash residue, spares live temps | | `tls_e2e.sh` | mTLS, auth enforcement, reconnect-resume | +| `type_change_e2e.sh` | source path changes type (dir->symlink) between passes | | `ucopy_e2e.sh` | io_uring copy path with server-side copy disabled | CI (`.github/workflows/ci.yml`) runs gofmt, vet, build and the Go tests; the diff --git a/agent/src/walker.c b/agent/src/walker.c index debde3c..acaf7c6 100644 --- a/agent/src/walker.c +++ b/agent/src/walker.c @@ -292,11 +292,71 @@ static void apply_meta_dirfd(struct walk_ctx *ctx, int fd, const struct estat *s } } -static void remove_dst(struct walk_ctx *ctx, int dfd, const char *name, bool is_dir) +/* Default prefix for a destination object renamed aside because its type no + * longer matches the source and it could not be unlinked/rmdir'd in place + * (non-empty directory: ENOTEMPTY: unlinkat/rmdir refuses a non-empty + * directory, and this code has no business doing a recursive remove inline + * mid-walk — that machinery already exists, at shard-fan-out scale, in the + * DELETE pass (delete.c), so this hands off to it instead of duplicating it). + * Deliberately distinct from o->temp_prefix: handle_orphan's temp-reclaim + * check matches destination entries against THAT prefix and treats a match + * as possibly-live in-progress-copy residue (protected unless the tag names + * an older pass) — a renamed-aside stale object is neither in-progress nor + * protectable that way, it is unconditionally orphaned the moment it lands, + * so it must never be mistaken for a copy temp by that logic. Sharing + * temp_name_fmt's -.. shape (ctx->tmp_seq, the same + * atomic counter copy.c's own temp names use) only for uniqueness within + * this shard — the tag is not read back by anything, since this name is + * journaled as a plain JR_ORPHAN immediately, not treated as protected + * residue like a real copy temp. */ +#define STALE_PREFIX ".drsync.stale." + +/* Removes name (or, if that fails because it is a non-empty directory whose + * type no longer matches the source, renames it aside under STALE_PREFIX and + * journals the new name as an ordinary JR_ORPHAN) so the caller can then + * create the new source-typed object at the original name. rel is name's + * containing directory (dir_rel shape, "" at the tree root) — needed only to + * build the orphan's full relative path for the journal record; the rename + * itself stays within dfd, no path composition required for that half. + * + * This is the general fix for EVERY type transition at a given path (dir<-> + * file<->symlink<->special, docs/DESIGN-agent.md §2's "type differs -> + * replace"), not a symlink-specific one — remove_dst is the single call site + * every case in handle_entry's switch already routes through. Before this, + * a non-empty directory conflicting with a new symlink/file/special at the + * same path failed with ENOTEMPTY here and then EEXIST (or similar) at + * whatever create call followed, leaving the destination stuck in its stale + * state forever — reported live against exactly the symlink case (a source + * directory replaced with a symlink), but the bug was never symlink-specific; + * every type transition against a non-empty former directory hit it. */ +static void remove_dst(struct walk_ctx *ctx, const char *rel, int dfd, + const char *name, bool is_dir) { - if (unlinkat(dfd, name, is_dir ? AT_REMOVEDIR : 0) < 0 && errno != ENOENT) - walk_err(ctx, "replace-unlink", name); /* non-empty dir vs file conflict: - * recursive remove TODO(slice3) */ + if (unlinkat(dfd, name, is_dir ? AT_REMOVEDIR : 0) == 0 || errno == ENOENT) + return; + if (!is_dir || errno != ENOTEMPTY) { + walk_err(ctx, "replace-unlink", name); + return; + } + if (ctx->oe->o.dry_run) + return; /* would rename+orphan; nothing to actually move */ + char stale[NAME_MAX + 1]; + unsigned seq = __atomic_fetch_add(&ctx->tmp_seq, 1, __ATOMIC_RELAXED); + temp_name_fmt(stale, sizeof stale, STALE_PREFIX, ctx->it->job_id, + ctx->it->pass_no, ctx->it->shard_id, seq); + if (renameat(dfd, name, dfd, stale) < 0) { + walk_err(ctx, "replace-unlink", name); /* original ENOTEMPTY still stands */ + return; + } + char orel[PATH_MAX]; + if (snprintf(orel, sizeof orel, "%s%s%s", rel, rel[0] ? "/" : "", stale) >= + (int)sizeof orel) { + errno = ENAMETOOLONG; + walk_err(ctx, "path", stale); + return; + } + CTR_ADD(ctx->c.orphans, 1); + jrn_emit(ctx, JR_ORPHAN, orel, NULL, NULL, 0, NULL); } static void copy_symlink(struct walk_ctx *ctx, const char *dir_rel, int sfd, @@ -322,7 +382,7 @@ static void copy_symlink(struct walk_ctx *ctx, const char *dir_rel, int sfd, } } if (!ctx->oe->o.dry_run) - remove_dst(ctx, dfd, name, false); + remove_dst(ctx, dir_rel, dfd, name, false); } CTR_ADD(ctx->c.symlinks, 1); if (ctx->oe->o.dry_run) { @@ -684,7 +744,7 @@ static void handle_entry(struct walk_ctx *ctx, struct dpend *dp, const char *rel case S_IFDIR: CTR_ADD(ctx->c.dirs, 1); if (ds && !type_match && !o->dry_run) - remove_dst(ctx, dfd, name, false); + remove_dst(ctx, rel, dfd, name, false); if ((!ds || !type_match) && !o->dry_run && mkdirat(dfd, name, 0700) < 0 && errno != EEXIST) { walk_err(ctx, "mkdir", name); @@ -753,7 +813,7 @@ static void handle_entry(struct walk_ctx *ctx, struct dpend *dp, const char *rel return; } if (ds && !type_match) - remove_dst(ctx, dfd, name, S_ISDIR(ds->mode)); + remove_dst(ctx, rel, dfd, name, S_ISDIR(ds->mode)); if (should_chunk(o, ss->size)) { /* Hand a big file to the coordinator to copy across the fleet * instead of on this agent alone (design §2.3). The dst dir exists @@ -770,7 +830,7 @@ static void handle_entry(struct walk_ctx *ctx, struct dpend *dp, const char *rel case S_IFLNK: if (ds && !type_match && !o->dry_run) { - remove_dst(ctx, dfd, name, S_ISDIR(ds->mode)); + remove_dst(ctx, rel, dfd, name, S_ISDIR(ds->mode)); ds = NULL; } copy_symlink(ctx, rel, sfd, dfd, name, ss, ds && type_match); @@ -790,7 +850,7 @@ static void handle_entry(struct walk_ctx *ctx, struct dpend *dp, const char *rel return; } if (ds) - remove_dst(ctx, dfd, name, S_ISDIR(ds->mode)); + remove_dst(ctx, rel, dfd, name, S_ISDIR(ds->mode)); if (mknodat(dfd, name, ss->mode, makedev(ss->rdev_major, ss->rdev_minor)) < 0) { walk_err(ctx, "mknod", name); /* usually EPERM without CAP_MKNOD */ diff --git a/docs/DESIGN-agent.md b/docs/DESIGN-agent.md index 0e622b4..8685950 100644 --- a/docs/DESIGN-agent.md +++ b/docs/DESIGN-agent.md @@ -221,7 +221,11 @@ walk_shard(shard): ### 2.1 Diff predicate (per merged entry, cheap → expensive) ``` -1. d_type differs (file vs dir vs symlink vs special) → emit replace (unlink+create) +1. d_type differs (file vs dir vs symlink vs special) → emit replace (unlink+create; + a non-empty directory being + replaced is renamed aside and + orphaned instead — remove_dst, + §2.1a below) 2. regular file: size differs → emit copy 3. mtime differs beyond mtime_slop_ns (default 1 ms) → emit copy 4. symlink: target string differs → emit relink @@ -236,6 +240,37 @@ walk_shard(shard): Step 6 keeps the common path at one `statx` per side per entry; xattr round trips are paid only by entries that are otherwise clean and only when xattr/ACL preservation is on. +### 2.1a Type-change replace: a non-empty former directory can't just unlink + +Step 1's "replace" is `remove_dst` (`agent/src/walker.c`) followed by whatever +`mkdirat`/`symlinkat`/`mknodat`/open-for-copy the new type needs — one shared +helper for every `d_type` transition (dir↔file↔symlink↔special), not a +symlink-specific path. The plain case is `unlinkat`/`rmdir`: fine for a file, +a symlink, a special, or an *empty* directory. A directory that still has +content in it — the source replaced a real directory (with data already +copied to the destination in an earlier pass) with a file/symlink/special — +fails `rmdir` with `ENOTEMPTY`, and (found live, reported as a symlink +replacing a directory, but not actually symlink-specific — every type +transition against a non-empty former directory hits the same thing) the +create call that follows then fails too (`EEXIST` for `symlinkat`, similarly +for the others), leaving the destination stuck in its stale state on every +subsequent pass — the diff predicate keeps re-emitting "replace" and it keeps +failing the same way. + +`remove_dst` doesn't attempt a recursive removal inline (that machinery +already exists, at shard-fan-out scale, in the DELETE pass — duplicating it +here mid-walk would be wrong twice over). Instead, on `ENOTEMPTY`, it +`renameat`s the whole subtree aside under a dedicated prefix +(`.drsync.stale.-..`, deliberately distinct from +`temp_prefix` — the orphan sweep's temp-reclaim check treats anything under +`temp_prefix` as possibly-live copy residue, and a renamed-aside stale +subtree must never be mistaken for that) and journals the new name as an +ordinary `JR_ORPHAN` — the walk's caller then creates the new +file/symlink/special at the now-vacated original name in the same pass, and +the stale subtree is picked up and removed by the next explicit DELETE pass +through the same fan-out machinery any other orphan uses, no coordinator-side +awareness of "type change" required. + ### 2.2 statx batching — the NFS scan multiplier Serial `stat` over NFS at 0.5 ms RTT = 2k entries/s/thread — hopeless. The walker diff --git a/test/type_change_e2e.sh b/test/type_change_e2e.sh new file mode 100755 index 0000000..a4d8aab --- /dev/null +++ b/test/type_change_e2e.sh @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +# drsync type-change e2e: a source path changes TYPE between passes (a real +# directory with content replaced by a symlink) — reported live as a bug: +# the destination still has the old directory (with content, so non-empty), +# creating the symlink failed with ENOTEMPTY (from replace-unlink's +# unlinkat/rmdir) then EEXIST (from the subsequent symlinkat), leaving the +# destination stuck in its stale state forever every pass thereafter. +# +# remove_dst (agent/src/walker.c) now renames a non-empty directory aside +# under a dedicated STALE_PREFIX instead of just failing, and journals the +# new name as an ordinary JR_ORPHAN — so the symlink (or file, or special) +# creation that follows succeeds immediately, and the stale content is +# cleaned up by the next explicit delete pass through the SAME machinery +# (delete.c) that already handles arbitrary-depth/pathological orphan +# removal, no new coordinator-side code needed. +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/.." && pwd) +. "$ROOT/test/lib.sh" +WORK=$(mktemp -d "${TMPDIR:-/tmp}/drsync-typechange.XXXXXX") +read -r _CP _HP < <(pick_ports) +CP=${CP:-$_CP}; HP=${HP:-$_HP} +API="http://127.0.0.1:${HP}"; AUTH="Authorization: Bearer typechangetok" +PASS=0 +cleanup() { + for p in "${APID:-}" "${CPID:-}"; do [[ -n "$p" ]] && kill "$p" 2>/dev/null || true; done + wait 2>/dev/null || true + if [[ $PASS -eq 1 ]]; then rm -rf "$WORK"; else echo "work dir kept: $WORK"; fi +} +trap cleanup EXIT +fail() { echo "FAIL: $*" >&2; exit 1; } +has() { + local pat=$1 out + shift + out=$("$@") || return 1 + grep -q -- "$pat" <<<"$out" +} +export DRSYNC_SERVER="$API" DRSYNC_TOKEN=typechangetok + +API_TOKEN_FILE="$WORK/api-token" +echo -n typechangetok >"$API_TOKEN_FILE" +chmod 600 "$API_TOKEN_FILE" +DRSYNC="$ROOT/bin/drsync" + +# --- build ------------------------------------------------------------------- +make -C "$ROOT/agent" -s +( cd "$ROOT" && go build -o bin/drsyncd ./coordinator/cmd/drsyncd \ + && go build -o bin/drsync ./cli/drsync ) + +# --- pass 1: a real, non-empty directory synced normally ---------------------- +SRC="$WORK/src"; DST="$WORK/dst" +mkdir -p "$SRC/keep" "$SRC/wasdir/nested" +echo keepme > "$SRC/keep/file.txt" +echo one > "$SRC/wasdir/a.txt" +echo two > "$SRC/wasdir/nested/b.txt" +LINKTARGET=/somewhere/else + +# --- services ---------------------------------------------------------------- +"$ROOT/bin/drsyncd" -data-dir "$WORK/coord" -listen-agent 127.0.0.1:$CP \ + -listen-http 127.0.0.1:$HP -api-token-file "$API_TOKEN_FILE" -log-level warn \ + >"$WORK/coord.log" 2>&1 & +CPID=$! +wait_coordinator "$API" "$AUTH" || exit 1 +"$ROOT/agent/bin/drsync-agent" -c 127.0.0.1:$CP -i typechange-agent -w 4 -C 4 \ + >"$WORK/agent.log" 2>&1 & +APID=$! +sleep 1 + +cat > "$WORK/job.yaml" < "$WORK/report2.json" +python3 - "$WORK/report2.json" <<'EOF' || fail "pass 2 report shows errors" +import json, sys +r = json.load(open(sys.argv[1])) +assert r["totals"]["errors"] == 0, r["totals"]["errors"] +EOF + +# 3. the old directory's content must have been renamed aside under the +# destination directory (not left at "wasdir" under its old name, which is +# now the symlink) and journaled as an ORPHAN — the delete pass's own +# entry point, so no coordinator-side awareness of "type change" is needed. +STALE=$(find "$DST" -maxdepth 1 -name '.drsync.stale.*') +[[ -n "$STALE" ]] || fail "no .drsync.stale.* entry found at the destination root after pass 2" +[[ -d "$STALE" ]] || fail "stale entry $STALE is not a directory" +[[ -f "$STALE/nested/b.txt" ]] || fail "stale entry $STALE lost the old directory's content" + +ORPHANED=$(curl -sf -H "$AUTH" "$API/api/v1/jobs/typechange/journal?type=ORPHAN&pass=2" | python3 -c ' +import sys, json +recs = json.load(sys.stdin).get("records", []) +print("\n".join(r.get("rel_path", "") for r in recs)) +') +grep -qF "$(basename "$STALE")" <<<"$ORPHANED" \ + || fail "renamed-aside stale directory was not journaled as an ORPHAN (got: $(tr "\n" " " <<<"$ORPHANED"))" + +# --- delete pass: the stale content must be fully cleaned up ------------------- +has "DELETE pass triggered" \ + "$DRSYNC" pass trigger typechange --delete-pass --i-know-this-deletes \ + || fail "delete pass trigger refused" +for _ in $(seq 1 60); do + curl -sf -H "$AUTH" "$API/api/v1/jobs/typechange" | grep -q '"state":"COMPLETED"' && break + sleep 0.25 +done +curl -sf -H "$AUTH" "$API/api/v1/jobs/typechange" | grep -q '"state":"COMPLETED"' \ + || { tail -n 8 "$WORK"/agent.log "$WORK"/coord.log; fail "delete pass did not complete"; } + +[[ ! -e "$STALE" ]] || fail "stale renamed-aside directory was not removed by the delete pass" +[[ -L "$DST/wasdir" ]] || fail "the delete pass must not have touched the symlink itself" + +# 4. everything else about the tree is still correct. --no-dereference: the +# symlink's own target ($LINKTARGET) is a deliberately dangling path (its +# content was never part of what this test syncs), so a following diff -r +# would try to read through it on both sides and fail with ENOENT — the +# symlink itself (already checked above: -L + readlink match) is what +# must match, not whatever it happens to point at. +DIFF=$(diff -r --no-dereference "$SRC" "$DST" 2>&1 || true) +[[ -z "$DIFF" ]] || fail "final tree diverged from source:"$'\n'"$DIFF" + +echo "wasdir directory->symlink transition applied cleanly; stale content orphaned and reaped" +PASS=1 +echo "PASS: destination type change (non-empty directory -> symlink) handled OK"