Cover the post-walk cancellation guard with a test that reaches it
All checks were successful
check / check (push) Successful in 1m32s
All checks were successful
check / check (push) Successful in 1m32s
TestSyncScanCancelledWalkKeepsRecords handed syncScan a context that was already cancelled. loadIndex is the first thing syncScan does, and its QueryContext fails on that context, so the scan returned before startWalk was ever called: no walk ran, no pool started, no write path was reachable, and all three of the test's assertions held for the wrong reason. The post-walk ctx.Err() guard, which is the highest-stakes line in the change, had no coverage at all — a panic in its body, or deleting it outright, left the suite green. Replace it with TestSyncScanCancelledMidWalkKeepsRecords, which cancels during the walk and so reaches the guard holding a genuinely partial census and a still-populated record index. The cancellation is driven by the scan's own progress rather than by a timer: walkClock is a context that cancels itself once its Done method has been consulted a set number of times, and since every blocking channel operation in the walk selects on Done — one consultation per event, a couple per directory, against the index load's fixed three — a threshold set to a quarter of the fixture's file count lands the cancellation deep inside the walk on every run. The census settles at around 380 of 2000 files, leaving some 1600 records that a complete-looking census would have handed to the update phase as deletions. The already-cancelled case is kept, renamed to what it actually tests and with its goroutine assertion dropped, since nothing that could leak is ever started. Direct tests cover the remaining cancellation branches of both pools: sendEvent abandoning a blocked send, walk workers dropping queued directories, a walk worker abandoning its subdirectory hand-off, dispatchDirs closing jobs on its way out, feedHashJobs doing the same, hashWorker dropping queued runs, and hashPhase leaving its result loop. Each is deterministic — the channels involved are unbuffered, unread or pre-filled, so the cancellation case is the only one that can be ready. Also correct two overstated claims. The hashLeakFiles comment described a mechanism that does not occur: the surplus is absorbed exactly by the two pool channels plus the workers in flight, so the feeder drains and exits, and what an abandoned pool leaves parked is the workers and the goroutine waiting on them. And the guard is defence in depth, not the sole barrier against data loss: the update phase's BeginTx fails on the same cancelled context before deleting anything today. The guard is what keeps that true once an interrupted scan is allowed to commit what it has.
This commit is contained in:
49
scan_test.go
49
scan_test.go
@@ -6,7 +6,6 @@ import (
|
||||
"crypto/sha256"
|
||||
"database/sql"
|
||||
"encoding/hex"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
@@ -827,10 +826,14 @@ const injectedWriteFailure = "injected write failure"
|
||||
// hashLeakFiles is the size of the fixture for the hash-phase failure
|
||||
// test. The batch commit inside the hash phase is what fails, so the
|
||||
// tree must hold more than updateBatchSize files for the failure to
|
||||
// happen at all; the surplus over that is what is still queued when it
|
||||
// does, and it exceeds the depth of both pool channels so that the
|
||||
// workers have nowhere left to put their results. An abandoned pool
|
||||
// therefore parks forever, which is exactly what this test detects.
|
||||
// happen at all, and the surplus over that is what is still queued
|
||||
// when it does. That surplus is 2*workQueueDepth, which jobs, results
|
||||
// and the workers in flight between them absorb exactly, so the feeder
|
||||
// itself drains and exits; what an abandoned pool leaves parked is
|
||||
// every worker, each holding a result nobody will ever receive, plus
|
||||
// the goroutine waiting on them. That is what this test detects, and
|
||||
// its margin over detecting nothing at all is the worker count —
|
||||
// worth knowing before changing hashLeakWorkers or workQueueDepth.
|
||||
const hashLeakFiles = updateBatchSize + 2*workQueueDepth
|
||||
|
||||
// hashLeakWorkers is the worker count for that scan: a fixed, modest
|
||||
@@ -966,42 +969,6 @@ func TestScanHashWriteFailureUnwindsPool(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestSyncScanCancelledWalkKeepsRecords checks the other half of the
|
||||
// cancellation path: a scan whose context is already cancelled stops
|
||||
// with that error instead of treating its truncated walk as the truth
|
||||
// and deleting the record of every file it never reached.
|
||||
//
|
||||
//nolint:paralleltest // counts goroutines: must not run beside others
|
||||
func TestSyncScanCancelledWalkKeepsRecords(t *testing.T) {
|
||||
dir := buildSmokeTree(t)
|
||||
db := openTestDB(t)
|
||||
|
||||
syncTree(t, db, dir)
|
||||
|
||||
before := recordPaths(dbRecords(t, db))
|
||||
|
||||
ctx, cancel := context.WithCancel(t.Context())
|
||||
cancel()
|
||||
|
||||
base := baselineGoroutines(t)
|
||||
|
||||
_, err := syncScan(ctx, db, []string{dir}, 4, false)
|
||||
if !errors.Is(err, context.Canceled) {
|
||||
t.Fatalf("syncScan on a cancelled context = %v, want %v",
|
||||
err, context.Canceled)
|
||||
}
|
||||
|
||||
if got := recordPaths(dbRecords(t, db)); !slices.Equal(got, before) {
|
||||
t.Errorf("records = %q after a cancelled scan, want %q",
|
||||
got, before)
|
||||
}
|
||||
|
||||
if got := settledGoroutines(t, base); got > base {
|
||||
t.Errorf("goroutines = %d after the cancelled scan, want %d back",
|
||||
got, base)
|
||||
}
|
||||
}
|
||||
|
||||
func TestHashRuns(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user