Test the -x filesystem-boundary rejection branch (closes #17)

The -x descent rules in subdirJob had no test executing the actual
rejection: the only case proved -x skips nothing when everything is on
one filesystem. Add tests that call subdirJob directly with a fabricated
operand device, so no second real filesystem is needed:

- reject descent when the subdirectory is on a different device;
- bypass the check when the operand's own device is unknown;
- warn and refuse descent when the entry's Info() fails;
- cross the boundary by default (no -x).

Tests only; scan.go is unchanged.

Model: opus-4-8

TODO.md

@@ -29,10 +29,11 @@
 
 # Completed Steps
 
-- correct four inaccurate comments in `cancel_test.go` and rename
+- test the `-x` filesystem-boundary rejection branch by calling
-  `walkCancelInFlightDirs` to `walkCancelInFlightFiles` (2026-09-21,
+  `subdirJob` directly: reject across a boundary, bypass when the root
-  branch `issue-33-cancel-test-comments`, closes
+  device is unknown, stat-error warning, and default crossing
-  https://git.eeqj.de/sneak/sfdupes/issues/33)
+  (2026-09-21, branch `next`, closes
+  https://git.eeqj.de/sneak/sfdupes/issues/17)
 
 - fix the lint-image pin comments and `FROM` form in `Dockerfile` and
   `Dockerfile.lint` (2026-08-10, branch `next`, closes

cancel_test.go

@@ -13,20 +13,11 @@ import (
 	"time"
 )
 
-// This file gathers the tests for scan cancellation and worker-pool
+// poolUnwind bounds how long a goroutine is given to leave a pool
-// unwinding. Everything it exercises lives in scan.go, so by the repo's
+// after its context is cancelled. Only a failing run ever waits this
-// convention of one test file per source file it would belong in
+// long: a pool that ignored its cancellation parks forever, and this
-// scan_test.go. It is kept separate on purpose: cancellation behaviour
+// is what turns that into a failed assertion instead of a suite that
-// cuts across both the walk pool and the hash pool as a single concern,
+// hangs until the test binary's own timeout.
-// and scan_test.go is already 900+ lines. That is the deliberate
-// exception the convention otherwise expects to be stated.
-
-// poolUnwind bounds how long a goroutine is given to leave a pool after
-// its context is cancelled. The tests that use it turn a pool that
-// ignored its cancellation — and so parks forever — into a failed
-// assertion within this bound instead of a hang. Not every test in this
-// file has that property: a few catch their regression only as the test
-// binary's own timeout, and each of those says so.
 const poolUnwind = 2 * time.Second
 
 // walkClock is a context whose cancellation is driven by the scan's
@@ -37,12 +28,9 @@ const poolUnwind = 2 * time.Second
 //
 // The accounting behind the n chosen by each test: every blocking
 // channel operation in the walk selects on Done, so the walk spends
-// one consultation per file event plus a couple per directory. The
+// one consultation per file event plus a couple per directory, while
-// index load that runs ahead of it also consults Done, but a bounded
+// the index load that runs ahead of it spends a small fixed number
-// number of times that does not grow with the record count. The tests
+// (three) whatever the record count.
-// depend on that property, not on the bound's exact value: each picks
-// an n comfortably above it and well short of the walk's total, so the
-// cancellation lands deep inside the walk whatever the record count.
 type walkClock struct {
 	n    int64
 	seen atomic.Int64
@@ -102,11 +90,7 @@ const (
 	walkCancelFilesPerDir  = 20
 	walkCancelFiles        = walkCancelDirs * walkCancelFilesPerDir
 	walkCancelWorkers      = 4
-	// The files carried by the walkCancelWorkers directories already in
+	walkCancelInFlightDirs = walkCancelWorkers * walkCancelFilesPerDir
-	// flight when the scan is cancelled: each such directory can still
-	// emit its walkCancelFilesPerDir files. This is a file count, not a
-	// directory count.
-	walkCancelInFlightFiles = walkCancelWorkers * walkCancelFilesPerDir
 )
 
 // walkCancelAtDone is the consultation on which the fixture's context
@@ -170,11 +154,6 @@ func assertRecordsIntact(t *testing.T, db *sql.DB, before []string) {
 // phase and fail there instead, with a wrapped error rather than the
 // bare cancellation.
 //
-// The syncScan call here is not bounded by poolUnwind: a regression
-// that left a worker pool parked would hang it, and that regression is
-// caught only by the test binary's own timeout, not by a quick
-// assertion.
-//
 //nolint:paralleltest // counts goroutines: must not run beside others
 func TestSyncScanCancelledMidWalkKeepsRecords(t *testing.T) {
 	dir := buildWalkCancelTree(t)
@@ -232,11 +211,10 @@ func assertWalkGuardAborted(t *testing.T, st scanStats, err error) {
 	}
 
 	// The workers drop every directory still queued once the scan is
-	// cancelled, so only the files in the directories already in flight
+	// cancelled, so only the directories already in flight can add to
-	// can add to the census after the fact. A census beyond that bound
+	// the census after the fact. A census beyond that bound would mean
-	// would mean the cancellation was not observed where it should have
+	// the cancellation was not observed where it should have been.
-	// been.
+	limit := walkCancelAtDone + walkCancelInFlightDirs
-	limit := walkCancelAtDone + walkCancelInFlightFiles
 	if st.unchanged > limit {
 		t.Errorf("census covers %d files, want at most %d: the walk kept "+
 			"taking directories off the queue after cancellation",
@@ -434,11 +412,7 @@ func TestDispatchDirsClosesJobsWhenCancelled(t *testing.T) {
 
 // TestFeedHashJobsClosesJobsWhenCancelled checks that the hash feeder
 // abandons the runs it has not queued yet and still closes the job
-// channel, which is what lets the workers' range terminate. The
+// channel, which is what lets the workers' range terminate.
-// receive on jobs below is not bounded: a feeder that returned without
-// closing jobs would leave that receive with no sender and no close, so
-// this regression is caught by the test binary's timeout rather than by
-// a bounded assertion.
 func TestFeedHashJobsClosesJobsWhenCancelled(t *testing.T) {
 	t.Parallel()
 
@@ -466,16 +440,6 @@ func TestFeedHashJobsClosesJobsWhenCancelled(t *testing.T) {
 // nobody wants the hashes of — while still letting the range run out
 // so the pool tears down. The queued run names a file that does not
 // exist, so a worker that hashed it anyway would produce a result.
-//
-// hashWorker has a second cancellation exit: the send of a completed
-// result on the results channel. That branch is reachable from the
-// production scan path, not dead code — pool.stop() cancels the context
-// before it starts draining results, so a worker parked on that send
-// leaves through this case, freed by the drain rather than by an empty
-// jobs channel. It is exercised by TestScanHashWriteFailureUnwindsPool,
-// which strands every worker on a full results channel until stop()
-// unwinds the pool. With both branches covered, every cancellation
-// branch of the walk and hash pools has a test.
 func TestHashWorkerDropsQueuedRuns(t *testing.T) {
 	t.Parallel()
 
@@ -508,10 +472,7 @@ func TestHashWorkerDropsQueuedRuns(t *testing.T) {
 // TestHashPhaseCancelledReturnsContextError checks the result loop's
 // own exit: with the pool cancelled, no result will ever arrive, and
 // the loop must leave through the cancellation rather than wait for a
-// receive that cannot happen. This call is not bounded by poolUnwind: a
+// receive that cannot happen.
-// loop that dropped its cancellation case would block on that receive,
-// so the regression surfaces as the test binary's timeout rather than
-// as a bounded assertion.
 func TestHashPhaseCancelledReturnsContextError(t *testing.T) {
 	t.Parallel()
 

scan_test.go

@@ -6,7 +6,9 @@ import (
 	"crypto/sha256"
 	"database/sql"
 	"encoding/hex"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -324,6 +326,152 @@ func TestDeviceOfInfo(t *testing.T) {
 	}
 }
 
+// dirEntryFor returns the fs.DirEntry for name within dir, obtained via
+// the same os.ReadDir the walk uses, so it carries a real Info().
+func dirEntryFor(t *testing.T, dir, name string) fs.DirEntry {
+	t.Helper()
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, e := range entries {
+		if e.Name() == name {
+			return e
+		}
+	}
+
+	t.Fatalf("entry %q not found in %q", name, dir)
+
+	return nil
+}
+
+// callSubdirJob runs subdirJob against e under parent, collecting any
+// warning events it emits (subdirJob emits at most one).
+func callSubdirJob(t *testing.T, p string, e fs.DirEntry,
+	parent dirJob, oneFS bool,
+) (dirJob, bool, []walkEvent) {
+	t.Helper()
+
+	events := make(chan walkEvent, 1)
+	job, ok := subdirJob(t.Context(), p, e, parent, oneFS, events)
+	close(events)
+
+	var evs []walkEvent
+	for ev := range events {
+		evs = append(evs, ev)
+	}
+
+	return job, ok, evs
+}
+
+// TestSubdirJobOneFilesystem exercises the -x boundary check in
+// subdirJob directly, so no second real filesystem is needed. The
+// subdirectory's real device is compared against a fabricated operand
+// device.
+func TestSubdirJobOneFilesystem(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+
+	sub := filepath.Join(dir, "sub")
+
+	err := os.Mkdir(sub, 0o750)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	info, err := os.Lstat(sub)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	dev, ok := deviceOfInfo(info)
+	if !ok {
+		t.Skip("platform exposes no device id")
+	}
+
+	// A device the subdirectory is not on, standing in for an operand
+	// rooted on a different filesystem.
+	otherDev := dev + 1
+	e := dirEntryFor(t, dir, "sub")
+
+	cases := []struct {
+		name   string
+		oneFS  bool
+		parent dirJob
+		wantOK bool
+	}{
+		// -x on, subdirectory on a different device than its operand:
+		// descent is refused.
+		{"reject across boundary", true,
+			dirJob{rootDev: otherDev, rootDevOK: true}, false},
+		// -x on but the operand's own device is unknown: the boundary
+		// check is bypassed and descent proceeds.
+		{"bypass when root device unknown", true,
+			dirJob{rootDev: otherDev, rootDevOK: false}, true},
+		// Default (no -x): boundaries are crossed even onto a different
+		// device.
+		{"cross by default", false,
+			dirJob{rootDev: otherDev, rootDevOK: true}, true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			job, ok, evs := callSubdirJob(t, sub, e, tc.parent, tc.oneFS)
+			if ok != tc.wantOK {
+				t.Fatalf("accepted = %v, want %v", ok, tc.wantOK)
+			}
+
+			if len(evs) != 0 {
+				t.Fatalf("unexpected events: %+v", evs)
+			}
+
+			if ok && job.path != sub {
+				t.Fatalf("job.path = %q, want %q", job.path, sub)
+			}
+		})
+	}
+}
+
+// errInfoUnavailable is returned by errDirEntry.Info().
+var errInfoUnavailable = errors.New("info unavailable")
+
+// errDirEntry is a directory entry whose Info() always fails, driving
+// subdirJob's stat-error branch deterministically.
+type errDirEntry struct{ name string }
+
+func (e errDirEntry) Name() string { return e.name }
+func (errDirEntry) IsDir() bool    { return true }
+func (errDirEntry) Type() fs.FileMode {
+	return fs.ModeDir
+}
+
+func (errDirEntry) Info() (fs.FileInfo, error) {
+	return nil, errInfoUnavailable
+}
+
+// TestSubdirJobStatError asserts that when a subdirectory's Info()
+// fails under -x, subdirJob warns and refuses descent.
+func TestSubdirJobStatError(t *testing.T) {
+	t.Parallel()
+
+	p := "/does/not/matter/sub"
+
+	_, ok, evs := callSubdirJob(t, p, errDirEntry{name: "sub"},
+		dirJob{rootDev: 1, rootDevOK: true}, true)
+	if ok {
+		t.Fatal("descent accepted after stat error, want refused")
+	}
+
+	if len(evs) != 1 || !evs[0].fail || !strings.Contains(evs[0].warn, p) {
+		t.Fatalf("want one warning naming the path, got %+v", evs)
+	}
+}
+
 // buildSmokeTree recreates the README smoke-test filesystem layout
 // with deterministic content and returns the tree root.
 func buildSmokeTree(t *testing.T) string {