fix: ChildResultBroker.wait drops a delivered result on a cancel race #458

New issue

Open

opened 2026-07-25 23:33:34 +02:00 by dries · 0 comments

dries commented

2026-07-25 23:33:34 +02:00

Owner

Code-quality audit (P2). internal/mcp/child_results.go:91-98.

select {
case result := <-ch:
	if remove {
		b.remove(childID, ch)
	}
	if err := ctx.Err(); err != nil {
		return ChildResult{}, err     // <- result already received, now discarded
	}
	return result, nil
case <-ctx.Done():
	...
}

Why it matters

If the child's result and ctx.Done() become ready in the same scheduling window, select picks uniformly at random. When it takes the result branch, the post-check throws the payload away — and the waiter has already been removed from b.waiters, so the result is unrecoverable. The parent session loses a completed child's output and reports a timeout instead. Rare, non-deterministic, and unreproducible from the log.

The ctx.Err() pre-check before the select is correct and should stay; only the post-receive check is wrong.

Suggested fix

Delete the post-receive ctx.Err() check. A result in hand beats a cancellation that arrived at the same instant.

case result := <-ch:
	if remove {
		b.remove(childID, ch)
	}
	return result, nil

Acceptance criteria

wait returns a received result even when ctx is already done.
Regression test: buffer a result on the channel, pass an already-cancelled ctx, assert the result is returned. Must fail on current main.

Effort: S.

Code-quality audit (P2). `internal/mcp/child_results.go:91-98`. ```go select { case result := <-ch: if remove { b.remove(childID, ch) } if err := ctx.Err(); err != nil { return ChildResult{}, err // <- result already received, now discarded } return result, nil case <-ctx.Done(): ... } ``` ## Why it matters If the child's result and `ctx.Done()` become ready in the same scheduling window, `select` picks uniformly at random. When it takes the result branch, the post-check throws the payload away — and the waiter has already been removed from `b.waiters`, so the result is unrecoverable. The parent session loses a completed child's output and reports a timeout instead. Rare, non-deterministic, and unreproducible from the log. The `ctx.Err()` pre-check before the `select` is correct and should stay; only the post-receive check is wrong. ## Suggested fix Delete the post-receive `ctx.Err()` check. A result in hand beats a cancellation that arrived at the same instant. ```go case result := <-ch: if remove { b.remove(childID, ch) } return result, nil ``` ## Acceptance criteria - [ ] `wait` returns a received result even when `ctx` is already done. - [ ] Regression test: buffer a result on the channel, pass an already-cancelled ctx, assert the result is returned. Must fail on current `main`. Effort: S.