run pool completion after a worker error, not by chance

[paskal]

poolCompleteFn ran or not depending on which goroutine got scheduled first.

A failing worker cancels the errgroup context. A surviving worker can then leave finishWorker two ways: through wCtx.Done with lastErr = context.Canceled, or through the drained channels with lastErr = nil. The suppression check was !errors.Is(lastErr, context.Canceled), so the first path skipped the callback and the second ran it. For a chained pool that meant the next stage got closed about half the time.

Suppression now checks p.callerCtx.Err(), which only the caller can cancel. When the caller has not cancelled, the callback gets context.WithoutCancel(ctx) so a peer worker's failure cannot stop it from closing the next pool, while keeping context values including metrics.

TestPool_PoolCompletion/worker_error_still_runs_pool_completion fails on master (5/5 runs) and passes here.

[umputun]

the gate change is right and I verified the bug it fixes: putting your new test on top of master's pool.go fails with "pool completion must run when the caller did not cancel" every run, and passes here. Moving suppression to callerCtx is the correct call, a worker returning context.Canceled is a worker error rather than caller cancellation.

the WithoutCancel is too broad though. It applies whenever p.callerCtx.Err() == nil, which includes an ordinary successful close, not just the peer-error case the comment describes. So on a clean run the callback loses the caller's deadline and its Done becomes nil:

p := New[int](1, worker).WithPoolCompleteFn(func(cctx context.Context) error {
    _, hasDeadline := cctx.Deadline()   // false on this branch, true on master
    return nil
})
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
p.Go(ctx); p.Submit(1); p.Close(ctx)

that matters for exactly the callback you are protecting: one that closes the next pool or does I/O can now block with nothing able to interrupt it, where before the caller's deadline would have ended it. There is also a small race, a caller cancelling just after the callerCtx.Err() check gets a detached context that never becomes done.

could you narrow it so the normal path passes ctx through unchanged, and only detach when a peer worker actually failed? In that case keeping values from the worker context while taking Deadline, Done and Err from callerCtx would preserve metrics without dropping the caller's cancellation.

worth a test for the retained deadline as well as the existing one, since the current suite would not have caught this.

on #14, the description says it removes about 28MB of committed build output but the branch does not delete anything. It is 21 modified files, no deletions, and the binaries are not in the merge base or in history. The .gitignore entries are the only part of that, and they are worth having. Just the body needs correcting.

[umputun]

after a worker error the callback now runs unless the caller cancelled, but which context it gets is still decided by a race, so the chained-pool case this sets out to fix can still be cancelled underneath.

finishWorker picks between the live context and a detached one by reading ctx.Err() at pool.go:449. A failing worker decrements activeWorkers at 442 and only afterwards returns its error, and errgroup cancels only once that return happens: if err := f(); err != nil then errOnce.Do, x/sync v0.19.0 errgroup.go:93. So with two workers and the input already closed by Close, there is a window where A has decremented but not yet returned, B drains the closed channels and leaves without ever seeing a cancellation, decrements to zero, reads ctx.Err() == nil, takes the completeCtx := ctx branch and starts poolCompleteFn on the errgroup context. A returns, errgroup cancels, and the callback is cancelled mid-flight by a peer error while callerCtx is still live.

that callback is where a chained pool closes the next one, as with the chaining pattern in examples/direct_chain/main.go:64. If cancellation arrives before pool.go:516, pending accumulator items are never submitted, and Close then closes the channels. If it arrives during flushing or waiting, Close can return early.

the new case at pool_test.go:1573 does <-p.ctx.Done() before it closes the input, so it only ever exercises the already-cancelled branch and this ordering never comes up.

what would close it: take cancellation and deadline from callerCtx and values from the worker context, always, instead of choosing the policy from a snapshot of whether errgroup has cancelled yet. A small wrapper delegating Value to the worker context and Done/Err/Deadline to callerCtx does it, and it keeps the caller's own deadline on the worker-error path, which the current detached branch drops. A regression for it needs completion to start before the peer's error reaches errgroup.

everything else here checks out: the invocation gate is deterministic now, completeErr has one writer before eg.Wait, and the caller-cancellation gate and the joined completion errors are right.