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

New issue

Closed

opened 2026-07-25 23:33:34 +02:00 by dries · 1 comment

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.

dries added the

labels

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

dries commented

2026-08-20 09:22:54 +02:00

Author

Owner

Validated against main @ 2d661364: downgraded — race exists, data no longer lost. The post-receive ctx.Err() discard is still there verbatim (internal/mcp/child_results.go:91-99). But the watcher persists the terminal status/summary to state.db before Deliver, and the caller's error path CASes waiting→disconnected + queues a reconnect reminder (tools_split.go:264-272); await_session_result then replays the result from the DB (tools_split.go:382-405). Net effect of losing the race: one extra round-trip for the parent, not a lost result. The one-line fix (drop the post-receive check) is still correct and trivial — keep as cleanup.

Validated against main @ 2d661364: **downgraded — race exists, data no longer lost**. The post-receive ctx.Err() discard is still there verbatim (internal/mcp/child_results.go:91-99). But the watcher persists the terminal status/summary to state.db *before* Deliver, and the caller's error path CASes waiting→disconnected + queues a reconnect reminder (tools_split.go:264-272); await_session_result then replays the result from the DB (tools_split.go:382-405). Net effect of losing the race: one extra round-trip for the parent, not a lost result. The one-line fix (drop the post-receive check) is still correct and trivial — keep as cleanup.

dries referenced this issue from a commit

2026-08-20 18:37:38 +02:00

fix: never discard a delivered child result on a cancel race (#458)

dries referenced this issue from a pull request that will close it,

2026-08-20 18:38:05 +02:00

fix: validated ticket sweep — tier 1 + tier 2 bugs #538

dries referenced this issue from a commit

2026-08-20 20:57:54 +02:00

fix: never discard a delivered child result on a cancel race (#458)

dries closed this issue

2026-08-20 23:12:38 +02:00