Skip to content

fix(search): stop pipeline work on cancellation, fatal errors, and panics - #22

Merged
hammadmajid merged 1 commit into
fix/12-object-storefrom
fix/14-pipeline-cancellation
Sep 5, 2026
Merged

hammadmajid merged 1 commit into
fix/12-object-storefrom
fix/14-pipeline-cancellation

Conversation

@hammadmajid

@hammadmajid hammadmajid commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

The pipeline's goroutine lifecycle and channel closure were already correct. These defects are all about what it did after something went wrong: it kept working.

Closes #14

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Performance improvement
  • Refactoring or code cleanup
  • Documentation update
  • CI/CD or build workflow change

Key Changes

Derived cancellable context. ExecuteContext now does callerCtx := ctx; ctx, cancelWork := context.WithCancel(callerCtx). One change subsumes three defects:

  • A fatal error no longer costs a full scan. Previously the error went into the 1-slot errCh with a default arm and every remaining blob was still processed; the caller could not observe it until resultsCh closed. Now the worker publishes, delivers the result, and calls cancelWork().
  • --quiet stops instead of spin-draining. stop atomic.Bool and its stop.Load() skip are gone. A quiet hit calls cancelWork(), the dispatcher drops out at its existing ctx.Done() arm, and workers return.
  • A late SIGINT no longer discards a complete result set. The closer publishes callerCtx.Err() — never the derived context — and only when searched.Load() < len(jobOrder). The parent gate alone was insufficient: a cancel landing after the caller already received every result still reported context.Canceled, which is precisely the defect. searched is one atomic add per blob, read only after wg.Wait(), so searched == len(jobOrder) deterministically means the full set was handed over.

Worker panics contained. processTask drives zlib inflate and delta reconstruction over bytes from an arbitrary .git directory, and there was no recover anywhere in the repository — a panic on a worker goroutine killed the process with a raw stack dump, skipping defer reader.Close() and the exitCodeForError contract. New safeProcessTask recovers per task into an ordinary fatal BlobResult.Error, routed through the existing fatal branch. The recover is scoped to one task, not the worker loop.

ExecuteStream deleted. It hardcoded context.Background(), so an abandoned pipeline leaked for the process lifetime with no recovery. No callers.

Channel directions are now compiler-enforced. Extracted dispatch(ctx, jobOrder, tasksCh chan<- *blobTask) (sole sender and closer) and runWorker(ctx, cancelWork, searched, <-chan *blobTask, chan<- *BlobResult, chan<- error). Previously these were bidirectional locals captured by closures — the invariants whose violation panics were verified only by inspection.

Stale-result drop. After safeProcessTask, an internal cancellation (quiet/fatal) no longer gets re-published as a bogus context.Canceled fatal error via processTask's own ctx checks.

Docs. The errCh cap-1 + non-blocking-send invariant is now written down: it implements first-fatal-error-wins and is what guarantees a worker never blocks publishing an error, since the consumer may not read errCh until resultsCh is drained — raising the capacity reintroduces the deadlock. ExecuteContext's leak-freedom claim now states its precondition (drain or cancel). Pipeline and Matcher documented as immutable after construction and safe for concurrent use.

Untouched: single-owner closure, wg.Add before go, ctx.Done() on every blocking arm, both fast paths, Execute's deduplicated ordering.

Verification & Testing

  • Ran go test -v -count=1 ./...
  • Ran go test -race -shuffle=on -count=1 ./...
  • Ran go vet ./...
  • Added or updated unit/integration tests
  • Tested manually against sample Git repository histories
go build ./...                                          clean
go vet ./internal/search/                               clean (no lostcancel)
go test -race -shuffle=on -count=1 ./internal/search/   ok 1.17s

Mutation results — each new assertion proven to fail on pre-fix behaviour, then the file restored byte-identically and re-run green:

Mutation Result
Closer gates on the derived ctx both quiet subtests fail with context canceled
Drop the searched gate TestPipelineCancelAfterCompletion fails with context canceled
Remove the quiet cancel 200/200 and 2000/2000 blobs read
Call processTask instead of safeProcessTask test binary dies with a raw panic stack
Remove the fatal cancel 300/300 blobs read

New tests assert observable work, not just returned errors: a counting ObjectReader shows --quiet with one worker reads exactly 1 of 200 blobs, and the first fatal of 300 stops after exactly 1 read.

Note

Stacked on #21.

Checklist

  • gofmt clean
  • Every new test mutation-checked: reverting the fix makes it fail
  • This layer builds and passes the full race suite on its own, not just at the top of the stack

…nics

ExecuteContext ran everything it was handed no matter what went wrong. It
never derived a cancellable context, so a fatal error only landed in the
1-slot errCh while every remaining blob was still searched, and --quiet
spin-drained the whole queue through a `stop atomic.Bool` after the answer
was already known. The closer then republished ctx.Err() unconditionally,
so a SIGINT arriving just after the last worker exited turned a complete
result set into exit code 2.

Work now runs under a cancellable child of the caller's context. The first
fatal error and the first quiet-mode hit call cancelWork(), which unblocks
the dispatcher and lets workers return at their existing ctx.Done() arm
instead of draining. The closer defers cancelWork() so the derived context
is always released, and publishes a cancellation only when the caller's own
context was cancelled *and* fewer results were delivered than there were
deduplicated blobs -- so an internal early stop is never misreported as
context.Canceled, and a cancel that lands after the caller already received
everything cannot fail a complete run.

processTask inflates and reconstructs bytes from an arbitrary .git
directory, and a panic on a worker goroutine cannot be recovered by
runContext: it killed the process, skipping the reader's cleanup and the
exitCodeForError contract. safeProcessTask now recovers per task and turns
a panic into an ordinary fatal *BlobResult.Error routed through the normal
fatal-error path.

Also: deleted ExecuteStream, which hardcoded context.Background() and had
no callers, so every streaming consumer must supply a cancellable context;
extracted dispatch() and runWorker() with directional channel parameters so
the compiler enforces the sender/receiver roles whose violation panics; and
documented the errCh capacity-1 invariant, the drain-or-cancel precondition
behind the no-goroutine-leak claim, and the immutability of Pipeline and
Matcher.

Closes #14
@hammadmajid
hammadmajid force-pushed the fix/14-pipeline-cancellation branch from d36923e to 3bb1827 Compare September 5, 2026 14:14
@hammadmajid
hammadmajid marked this pull request as ready for review September 5, 2026 14:19
@hammadmajid
hammadmajid merged commit 22a2c9f into main Sep 5, 2026
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

search pipeline: no derived cancel context, fatal errors do not stop work, worker panics kill the process

1 participant