sneak/sfdupes
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Hash-phase error path abandons and deadlocks the worker pool #6

New Issue
Closed
opened 2026-08-09 03:43:45 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:43:45 +02:00
Collaborator
Copy Link

hashPhase returns immediately when recordRun fails (scan.go:399-402), abandoning the remaining runs. The feeder goroutine (scan.go:375-381) then blocks forever sending on jobs, and every hash worker (scan.go:791-800) blocks forever sending on results once its 1024-slot buffer fills.

Today this is masked: the caller calls fatalf and the process dies with the goroutines still parked. It becomes a real leak the moment the scan runs in-process — which is exactly what #5 (graceful interrupt), #16 (CLI-surface tests) and #18 (race-detector target) all require.

Definition of done

  1. hashPhase cancels the feeder and drains results before returning an error, or the pool becomes context-aware and unwinds through the same cancellation path as #5.
  2. A test injects a database write failure partway through the hash phase and asserts the scan entrypoint returns an error rather than hanging.
  3. The same test asserts runtime.NumGoroutine() returns to its pre-scan baseline, allowing a short settle window.
  4. make check green.
`hashPhase` returns immediately when `recordRun` fails (`scan.go:399-402`), abandoning the remaining runs. The feeder goroutine (`scan.go:375-381`) then blocks forever sending on `jobs`, and every hash worker (`scan.go:791-800`) blocks forever sending on `results` once its 1024-slot buffer fills. Today this is masked: the caller calls `fatalf` and the process dies with the goroutines still parked. It becomes a real leak the moment the scan runs in-process — which is exactly what #5 (graceful interrupt), #16 (CLI-surface tests) and #18 (race-detector target) all require. ## Definition of done 1. `hashPhase` cancels the feeder and drains `results` before returning an error, or the pool becomes context-aware and unwinds through the same cancellation path as #5. 2. A test injects a database write failure partway through the hash phase and asserts the scan entrypoint returns an error rather than hanging. 3. The same test asserts `runtime.NumGoroutine()` returns to its pre-scan baseline, allowing a short settle window. 4. `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:45 +02:00
clawbot commented 2026-08-09 04:45:49 +02:00
Author
Collaborator
Copy Link

Implementation plan (branch hash-pool-cleanup, from main at 2a055c0):

1. Make the scan context-aware, end to end.
ctx context.Context becomes the first parameter of runScan and
syncScan, sourced from cmd.Context() in the cobra RunE wrapper
(runE grows a ctx parameter). Nothing installs a signal handler
here — that is #5's job; this PR only builds the cancellation path #5
will hook into, plus uses it for the error path that is broken today.

2. Replace the ad-hoc hash pool with an owned, cancellable one.
A small hashPool type owns the feeder goroutine and the worker
goroutines. Its context is derived from the scan's, and every blocking
send inside the pool (jobs <- run in the feeder, results <- ... in
the workers) becomes a select against ctx.Done(). The feeder always
closes jobs on the way out, so the workers' range jobs always
terminates. hashPhase does defer pool.stop(), and stop cancels,
then drains results until every pool goroutine has exited — draining
is the part that matters, because a worker already parked on a send
will not observe the cancellation until someone reads. The loop over
results also selects on ctx.Done() so an externally cancelled scan
(i.e. #5) unwinds the same way. Net effect: hashPhase returning early
for any reason leaves zero goroutines behind (DoD 1).

3. Walk pool.
It has the same unbounded-blocking-send shape (events <- ... from the
walk workers and seedRoot, subdirs <- ..., jobs <- ... from the
dispatcher) but it does not leak today, because walkPhase has no
early return: it always drains events to close. #5 will introduce
exactly such an early return, so the same select-on-ctx.Done()
treatment is applied there, and dispatchDirs grows a
defer close(jobs) so the workers can never be stranded. One
correctness guard comes with it: syncScan checks ctx.Err() after
the walk, because a cancelled walk yields a truncated size census and
the update phase would read every unreached file as vanished and delete
its record. Zero behaviour change today (the context is never cancelled
in production yet); it is what keeps #5 from having to re-plumb this.

4. Test (DoD 2 and 3), scan_test.go.
Injection is a real database write failure, not a fake: the test
pre-creates the database at SFDUPES_DATABASE with the normal schema
plus CREATE TRIGGER ... BEFORE INSERT ON files BEGIN SELECT RAISE(ABORT, 'injected write failure'); END. Reads (the record index
load) still work; the first batch commit inside the hash phase fails,
which is exactly the recordRun error path in question. The tree is a
few thousand more empty files than updateBatchSize, so the failure
lands partway through the phase with well over workQueueDepth runs
still queued — that is what guarantees parked workers if the fix is
absent. Empty files are never opened by the hasher, so the fixture
costs directory entries and no I/O. The test drives the real entry
point (run([]string{cmdScan, dir}, ...)), asserts exitFatal, and
then polls runtime.NumGoroutine() back to its pre-scan baseline with
a bounded settle loop (poll, not a fixed sleep). I will verify the test
genuinely fails with the production change reverted.

5. TODO.md Completed Steps entry in the same commit; make fmt,
then both make check and make docker green before pushing; PR
against main titled ... (closes #6), labelled needs-review,
assigned to clawbot.

Correction to step 3, recorded during the rework of #31. The claim
above that "the update phase would read every unreached file as
vanished and delete its record" overstates what happens today. With the
guard deleted, updatePhase deletes nothing: its first BeginTx fails
on the same cancelled context first. The guard is defence in depth, not
the sole barrier against data loss. It is still the right thing to
have — it is the barrier that survives #5 deciding an interrupted scan
may commit what it has, and it discards the partial census at the phase
boundary instead of letting a confusing
begin transaction: context canceled surface from deep inside the
update phase.

Implementation plan (branch `hash-pool-cleanup`, from `main` at `2a055c0`): **1. Make the scan context-aware, end to end.** `ctx context.Context` becomes the first parameter of `runScan` and `syncScan`, sourced from `cmd.Context()` in the cobra `RunE` wrapper (`runE` grows a `ctx` parameter). Nothing installs a signal handler here — that is #5's job; this PR only builds the cancellation path #5 will hook into, plus uses it for the error path that is broken today. **2. Replace the ad-hoc hash pool with an owned, cancellable one.** A small `hashPool` type owns the feeder goroutine and the worker goroutines. Its context is derived from the scan's, and every blocking send inside the pool (`jobs <- run` in the feeder, `results <- ...` in the workers) becomes a `select` against `ctx.Done()`. The feeder always `close`s `jobs` on the way out, so the workers' `range jobs` always terminates. `hashPhase` does `defer pool.stop()`, and `stop` cancels, then drains `results` until every pool goroutine has exited — draining is the part that matters, because a worker already parked on a send will not observe the cancellation until someone reads. The loop over results also selects on `ctx.Done()` so an externally cancelled scan (i.e. #5) unwinds the same way. Net effect: `hashPhase` returning early for any reason leaves zero goroutines behind (DoD 1). **3. Walk pool.** It has the same unbounded-blocking-send shape (`events <- ...` from the walk workers and `seedRoot`, `subdirs <- ...`, `jobs <- ...` from the dispatcher) but it does not leak today, because `walkPhase` has no early return: it always drains `events` to close. #5 will introduce exactly such an early return, so the same `select`-on-`ctx.Done()` treatment is applied there, and `dispatchDirs` grows a `defer close(jobs)` so the workers can never be stranded. One correctness guard comes with it: `syncScan` checks `ctx.Err()` after the walk, because a cancelled walk yields a truncated size census and the update phase would read every unreached file as vanished and delete its record. Zero behaviour change today (the context is never cancelled in production yet); it is what keeps #5 from having to re-plumb this. **4. Test (DoD 2 and 3), `scan_test.go`.** Injection is a real database write failure, not a fake: the test pre-creates the database at `SFDUPES_DATABASE` with the normal schema plus `CREATE TRIGGER ... BEFORE INSERT ON files BEGIN SELECT RAISE(ABORT, 'injected write failure'); END`. Reads (the record index load) still work; the first batch commit inside the hash phase fails, which is exactly the `recordRun` error path in question. The tree is a few thousand more empty files than `updateBatchSize`, so the failure lands partway through the phase with well over `workQueueDepth` runs still queued — that is what guarantees parked workers if the fix is absent. Empty files are never opened by the hasher, so the fixture costs directory entries and no I/O. The test drives the real entry point (`run([]string{cmdScan, dir}, ...)`), asserts `exitFatal`, and then polls `runtime.NumGoroutine()` back to its pre-scan baseline with a bounded settle loop (poll, not a fixed sleep). I will verify the test genuinely fails with the production change reverted. **5.** `TODO.md` Completed Steps entry in the same commit; `make fmt`, then both `make check` and `make docker` green before pushing; PR against `main` titled `... (closes #6)`, labelled `needs-review`, assigned to `clawbot`. --- **Correction to step 3, recorded during the rework of #31.** The claim above that "the update phase would read every unreached file as vanished and delete its record" overstates what happens today. With the guard deleted, `updatePhase` deletes nothing: its first `BeginTx` fails on the same cancelled context first. The guard is defence in depth, not the sole barrier against data loss. It is still the right thing to have — it is the barrier that survives #5 deciding an interrupted scan may commit what it has, and it discards the partial census at the phase boundary instead of letting a confusing `begin transaction: context canceled` surface from deep inside the update phase.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI checks failing or not yet green
 needs-rebase
Branch conflicts with main; rebase required
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found blocking problems; author must rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/sfdupes#6
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?