From 2f339fc53a786334171f9faabd1355c69ff91e38 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 07:44:17 +0000 Subject: [PATCH] Correct four inaccurate comments in cancel_test.go and rename a constant (closes #33) Fix the comments the re-review of https://git.eeqj.de/sneak/sfdupes/issues/6 found misdescribing their tests; no test's behaviour changes. State the property the walkClock tests rely on, that the index load's Done cost does not grow with the record count, instead of a wrong fixed figure. Stop implying every test is bounded by poolUnwind; the three that are not now say so. Record that hashWorker's results-send exit is reachable and covered by TestScanHashWriteFailureUnwindsPool. Rename walkCancelInFlightDirs to walkCancelInFlightFiles, since it counts files. Note at the top why the file departs from the one-test-file-per-source-file convention. Model: opus-4-8 (implementation); opus-5-5 (rework) --- TODO.md | 4 +++ cancel_test.go | 77 +++++++++++++++++++++++++++++++++++++------------- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/TODO.md b/TODO.md index c912a5c..e794517 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,10 @@ # Completed Steps +- correct four inaccurate comments in `cancel_test.go` and rename + `walkCancelInFlightDirs` to `walkCancelInFlightFiles` (2026-10-04, + https://git.eeqj.de/sneak/sfdupes/issues/33) + - README documents install, Docker, a daily cron scan and how to read and check the reports (2026-10-04, https://git.eeqj.de/sneak/sfdupes/issues/54) diff --git a/cancel_test.go b/cancel_test.go index e4ecd29..91963e6 100644 --- a/cancel_test.go +++ b/cancel_test.go @@ -18,11 +18,20 @@ import ( "time" ) -// poolUnwind bounds how long a goroutine is given to leave a pool -// after its context is cancelled. Only a failing run ever waits this -// long: a pool that ignored its cancellation parks forever, and this -// is what turns that into a failed assertion instead of a suite that -// hangs until the test binary's own timeout. +// This file gathers the tests for scan cancellation and worker-pool +// unwinding. Everything it exercises lives in scan.go, so by the repo's +// convention of one test file per source file it would belong in +// scan_test.go. It is kept separate on purpose: cancellation behaviour +// cuts across both the walk pool and the hash pool as a single concern, +// and scan_test.go is already over 1,600 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 @@ -33,9 +42,14 @@ 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, while -// the index load that runs ahead of it spends a small fixed number -// (three) whatever the record count. +// one consultation per file event plus a couple per directory. The +// index load that runs ahead of it also consults Done, but a bounded +// number of times that does not grow with the record count. The tests +// depend on that property, not on the bound's exact value: each test +// sets n from the consultations of the walk, plus those of the hash +// phase when it cancels mid-hash, far from both ends of the phase it +// interrupts, so the cancellation lands inside that phase whatever the +// record count. type walkClock struct { n int64 seen atomic.Int64 @@ -91,11 +105,14 @@ func (c *walkClock) Value(_ any) any { // directory still queued and only the handful already in flight can // emit anything more. const ( - walkCancelDirs = 100 - walkCancelFilesPerDir = 20 - walkCancelFiles = walkCancelDirs * walkCancelFilesPerDir - walkCancelWorkers = 4 - walkCancelInFlightDirs = walkCancelWorkers * walkCancelFilesPerDir + walkCancelDirs = 100 + walkCancelFilesPerDir = 20 + walkCancelFiles = walkCancelDirs * walkCancelFilesPerDir + walkCancelWorkers = 4 + // The most files the walkCancelWorkers directories already in + // flight when the scan is cancelled can still emit, at + // walkCancelFilesPerDir each. A file count, not a directory count. + walkCancelInFlightFiles = walkCancelWorkers * walkCancelFilesPerDir ) // walkCancelAtDone is the consultation on which the fixture's context @@ -158,6 +175,11 @@ func assertRecordsIntact(t *testing.T, db *sql.DB, before []string) { // unreachable, makes the scan carry its truncated view into the update // phase, which counts every record the walk never reached for removal. // +// 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) @@ -209,10 +231,11 @@ func assertWalkGuardAborted(t *testing.T, st scanStats, err error) { } // The workers drop every directory still queued once the scan is - // cancelled, so only the directories already in flight can add to - // the census after the fact. A census beyond that bound would mean - // the cancellation was not observed where it should have been. - limit := walkCancelAtDone + walkCancelInFlightDirs + // cancelled, so only the files in the directories already in flight + // can add to the census after the fact. A census beyond that bound + // would mean the cancellation was not observed where it should have + // been. + 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", @@ -646,7 +669,11 @@ 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. +// channel, which is what lets the workers' range terminate. The +// 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() @@ -674,6 +701,15 @@ 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. It is exercised by +// TestScanHashWriteFailureUnwindsPool, which strands every worker on a +// full results channel until stop() unwinds the pool. So both of +// hashWorker's cancellation branches have a test. func TestHashWorkerDropsQueuedRuns(t *testing.T) { t.Parallel() @@ -706,7 +742,10 @@ 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. +// receive that cannot happen. This call is not bounded by poolUnwind: a +// 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()