Handle a source path changing type against a non-empty destination

.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

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

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 <job>-<pass>.<shard>.<seq> 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 */

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.<job>-<pass>.<shard>.<seq>`, 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

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" <<EOF
+apiVersion: drsync/v1
+kind: Job
+metadata: { name: typechange }
+spec:
+  source: { path: $SRC }
+  destination: { path: $DST }
+  probe: { require_mount: false }
+  passes: { max: 1, converge_when: { delta_files_below: 1 } }
+EOF
+"$DRSYNC" job submit "$WORK/job.yaml" --start | grep -q "job typechange started" \
+    || fail "submit failed"
+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 "pass 1 did not converge"; }
+
+[[ -d "$DST/wasdir" ]] || fail "pass 1: wasdir was not synced as a directory"
+[[ -f "$DST/wasdir/nested/b.txt" ]] || fail "pass 1: wasdir contents were not synced"
+
+# --- source: replace the (still non-empty) directory with a symlink -----------
+rm -rf "$SRC/wasdir"
+ln -s "$LINKTARGET" "$SRC/wasdir"
+
+# --- pass 2: the type change must be applied, not left stuck -------------------
+has "pass triggered" "$DRSYNC" pass trigger typechange || fail "pass 2 trigger failed"
+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 "pass 2 did not converge"; }
+
+# 1. the symlink now exists at the destination, pointing at the right target —
+#    the actual bug: this used to fail with ENOTEMPTY then EEXIST and never
+#    happen at all, leaving wasdir as a stale real directory forever.
+[[ -L "$DST/wasdir" ]] || fail "wasdir is not a symlink at the destination after pass 2"
+GOT=$(readlink "$DST/wasdir")
+[[ "$GOT" == "$LINKTARGET" ]] || fail "wasdir symlink target = $GOT, want $LINKTARGET"
+
+# 2. no errors reported for the type-change pass — a stuck ENOTEMPTY/EEXIST
+#    pair would show up here.
+"$DRSYNC" report typechange --json > "$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"