fix(batch): panic recovery, deadlock-safe error sends, bounded prealloc, Done() race by MasterOfBinary · Pull Request #65 · MasterOfBinary/gobatch

MasterOfBinary · 2026-05-29T12:18:56Z

Summary

Hardens the batch engine against four failure modes found in a full-repo review. Each fix was written test-first: the panic test crashed the process and the Done() test reported a data race before the fixes.

Fixes

Panic recovery — a panic in a user Processor.Process is recovered and surfaced as a ProcessorError instead of crashing the host process.
Deadlock-safe error sends — internal error-channel sends now also select on ctx.Done(), so a full, undrained error buffer can no longer permanently wedge the pipeline.
Bounded pre-allocation — waitForItems caps its slice pre-alloc, so a huge MinItems (reachable at runtime via DynamicConfig.UpdateBatchSize) can't cause a makeslice panic / OOM.
Done() data race — Done() now reads b.done under the mutex.

Also included

New tests: double-Go panic, SourceError.Error() formatting, idle-timer re-arm, exact failing-item-ID assertions.
Docs: the errors.As pointer-target form (new(*SourceError)) documented on both error types (see errors.As with a value target never matches SourceError/ProcessorError #91 for the underlying type-identity question).

Compatibility

No exported API changes; behavior changes are confined to the fixed bugs and are strictly safer. Proactive stop-on-cancel is deferred to #76 (WithCancelMode).

Testing

go test -race ./... green, go vet and gofmt clean on the branch merged with current master.

🤖 Generated with Claude Code

codecov · 2026-05-29T12:20:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.03%. Comparing base (63ef757) to head (6ad2e5e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   96.76%   98.03%   +1.26%     
==========================================
  Files          12       12              
  Lines         371      407      +36     
==========================================
+ Hits          359      399      +40     
+ Misses          9        6       -3     
+ Partials        3        2       -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gemini-code-assist

Code Review

This pull request introduces several hardening improvements to the batch processing engine. Key changes include guarding the read of b.done with a mutex to prevent data races, making error channel sends context-aware to avoid deadlocks when the error buffer is full, recovering from panics in user processors, capping slice pre-allocation to prevent out-of-memory crashes from huge configurations, and refactoring timers to use time.Timer with Reset to prevent timer leaks. Extensive unit and integration tests have been added to verify these fixes. There are no review comments, so no feedback is provided.

A pathological config with a huge MinItems and MaxItems==0 (reachable via DynamicConfig.UpdateBatchSize(huge, 0)) caused waitForItems to call make([]*Item[T], 0, MinItems), attempting a multi-terabyte allocation that crashes/OOMs the process. Add clampPreallocCap, a pure helper that bounds the initial slice capacity to maxPreallocCap (4096) and treats MaxItems as a downward cap. The slice still grows via append as needed; only the allocation hint is bounded. Wire it into waitForItems. Behavior is unchanged for all normal configurations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Internal sends to b.errs in doReader and the per-batch processor goroutine were unguarded blocking sends. Once the error buffer filled and nobody drained it, the pipeline deadlocked and cancel() could not free it, so Done() never closed. Make each internal b.errs send a context-aware select, and add a <-ctx.Done() arm to doReader's main loop that closes b.items and returns. A cancelled context now always unblocks a wedged reader and any wedged per-batch goroutine, letting the pipeline complete (errs and done close). Draining the error channel remains the documented caller responsibility; this only restores the ability of context cancellation to break out of a wedge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The per-batch goroutine called proc.Process with no recover, so a panic in a user Processor crashed the entire host process. Add a deferred recover inside the goroutine, declared after defer wg.Done() so that (deferred-LIFO) recover runs first and wg.Done() still fires. The recovered panic is wrapped as a ProcessorError ("processor panic: <value>") and delivered via a context-aware send, so the pipeline completes cleanly (errs and done close) instead of taking down the process. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

waitForItems created its min/max timers with time.After and re-armed the max timer with a fresh time.After on every idle fire, leaking a runtime timer each cycle for the lifetime of the batch. Refactor to a single *time.Timer per bound created with time.NewTimer, stop both on every return path via a deferred Stop, and re-arm the max timer with Reset (the channel is already drained by the receiving case, so Reset is safe). The select waits on nil channels when a bound is unset, preserving the existing ignore-unset-timer behavior. Pure refactor; batching behavior is unchanged. Add TestMaxTimeMultipleIdleCyclesThenLateItem, which drives several idle MaxTime cycles then delivers a late item and asserts it is still processed, locking the re-arm behavior against this refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Done() read b.done without holding b.mu, while Go assigns b.done under the lock, which the race detector flags as a data race. Take b.mu in Done() to read the field into a local, release immediately, then return the channel (or the pre-closed sentinel). The lock is never held while waiting on the channel, so this introduces no deadlock. Add TestDoneRace, which hammers Done() concurrently with Go() and is clean under -race after the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add TestConcurrentGoPanics, which starts a batch against a blocked source and asserts a second concurrent Go call panics with the documented message "Concurrent calls to Batch.Go are not allowed", and TestSourceErrorMessage, which asserts SourceError.Error() formats as "source error: <cause>". Both cover existing behavior; no production change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The "items at indexes 0, 3, 6 ... 1 more error" comment reflected a misunderstanding: errorPerItemProcessor fails items by their per-batch local index (i%FailEvery), not by a fixed set of global indices, so which items fail depends on batch boundaries. Rewrite the comment to explain the real per-batch behavior and, with the deterministic MinItems=5 boundaries (batches [0..4] and [5..8]), strengthen the assertion to verify the exact set of failing item IDs {0, 3, 5, 8} by parsing the ID out of each error, instead of only counting four errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The engine wraps with &SourceError{} and &ProcessorError{}, so errors.As only matches the pointer-target form, errors.As(err, new(*SourceError)); the value-target form does not match because *SourceError is not assignable to SourceError. Add a godoc note with an example to both SourceError and ProcessorError so callers use the correct form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Keep the deadlock-safe ctx-aware error sends (a cancelled context can still break a pipeline wedged on a full, undrained error buffer), but remove the proactive `case <-ctx.Done()` arm from doReader's main select. That arm — which stops reading on cancel rather than relying on the Source — becomes the opt-in CancelStop behavior in the forthcoming WithCancelMode PR, so the default here matches the existing contract (rely on the Source to stop; already-read items are still processed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Update the hardening tests to Go's (<-chan error, error) signature and remove TestConcurrentGoPanics — a second Go now returns ErrBatchUsed instead of panicking, which single_use_test.go already covers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…el-escape paths Replaces the five duplicated select-based error sends with one sendErr helper, and adds tests for the two escape paths nothing exercised: the item-error send wedge and the nil-channel source report. Closes the codecov patch gap (4 uncovered ctx.Done arms). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…panic error identity Addresses three review findings: - sendErr now tries a non-blocking send first, so a canceled context never randomly preempts a send that would succeed. Cancellation only escapes a genuinely blocked send; error delivery to a draining consumer is deterministic again. Early-abort on failed sends is kept: it can now only trigger in the wedged state (buffer full + canceled), where further processing would only produce undeliverable errors. - Panic detection uses a completion flag instead of recover()'s return value, so panic(nil) (recover() == nil under this module's Go 1.18 semantics) is still reported as a ProcessorError. - Error-valued panic payloads are wrapped with %w, preserving errors.Is/errors.As identity through ProcessorError. Also deletes the unreachable nil-processor guard in the batch loop (Go filters nil processors at start). All three regression tests fail against the previous commit and pass with the fixes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Covers sendErr's slow-path success arm deterministically: with a 1-slot error buffer and a consumer that sleeps between receives, a send that finds the buffer full must block and deliver once a slot frees — never drop while the context is live. Previously this arm was only covered by scheduler luck (codecov patch flagged it on CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

gemini-code-assist Bot reviewed May 29, 2026

View reviewed changes

MasterOfBinary force-pushed the fix/engine-hardening branch from da42871 to 1fd4f94 Compare May 29, 2026 12:57

MasterOfBinary changed the base branch from master to fix/batch-timer-leak-and-reuse-safety May 29, 2026 12:58

MasterOfBinary changed the title ~~fix: harden batch engine (panic recovery, ctx cancellation, alloc cap, timer leak, Done race)~~ fix: harden batch engine — panic recovery, deadlock-safe error sends, alloc cap, Done() race May 29, 2026

MasterOfBinary mentioned this pull request May 29, 2026

feat(batch): configurable context-cancellation mode (WithCancelMode) #76

Open

MasterOfBinary force-pushed the fix/batch-timer-leak-and-reuse-safety branch from 27ee6f5 to a042228 Compare May 29, 2026 13:37

MasterOfBinary force-pushed the fix/engine-hardening branch from 1fd4f94 to f689df1 Compare May 29, 2026 13:38

MasterOfBinary force-pushed the fix/batch-timer-leak-and-reuse-safety branch from 3b59001 to d49ace6 Compare May 29, 2026 16:10

Base automatically changed from fix/batch-timer-leak-and-reuse-safety to master May 29, 2026 16:19

MasterOfBinary and others added 10 commits May 30, 2026 00:19

MasterOfBinary force-pushed the fix/engine-hardening branch from f689df1 to 027b25a Compare May 29, 2026 16:23

MasterOfBinary mentioned this pull request May 31, 2026

Error channel conflates per-item and processor-wide errors, and discards Item.ID #79

Open

MasterOfBinary mentioned this pull request Jul 10, 2026

errors.As with a value target never matches SourceError/ProcessorError #91

Open

Merge remote-tracking branch 'origin/master' into fix/engine-hardening

80cb06e

MasterOfBinary changed the title ~~fix: harden batch engine — panic recovery, deadlock-safe error sends, alloc cap, Done() race~~ fix(batch): panic recovery, deadlock-safe error sends, bounded prealloc, Done() race Jul 10, 2026

MasterOfBinary and others added 2 commits July 10, 2026 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(batch): panic recovery, deadlock-safe error sends, bounded prealloc, Done() race#65

fix(batch): panic recovery, deadlock-safe error sends, bounded prealloc, Done() race#65
MasterOfBinary wants to merge 14 commits into
masterfrom
fix/engine-hardening

MasterOfBinary commented May 29, 2026 •

edited

Loading

Uh oh!

codecov Bot commented May 29, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MasterOfBinary commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fixes

Also included

Compatibility

Testing

Uh oh!

codecov Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MasterOfBinary commented May 29, 2026 •

edited

Loading

codecov Bot commented May 29, 2026 •

edited

Loading