fix: MCP child sessions can be stranded in *_sending permanently #457

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/tools_comm.go:152,163.

A child session can end up permanently stuck in *_sending with no way out.

if err := t.sendMessage(ctx, childID, delivered); err != nil {
	_ = t.stateDB.RestoreChildFollowup(childID, *cs, sendingDelivery)   // :152 — error discarded
	...
}
...
claimed, err = t.stateDB.CompleteChildFollowup(childID, sendingDelivery, delivery)
if err != nil {
	...
	return mcplib.NewToolResultError(...)   // :163 — row left in *_sending
}

Two paths leave the row in ChildResultAsyncSending / ChildResultWaitSending:

Send fails and the restore also fails — the failure is _-discarded, so the caller sees only "sending message to child" and has no idea the row is now wedged.
Send succeeds but CompleteChildFollowup errors — no restore is attempted at all.

The guard at tools_comm.go:119 only accepts delivered / detached:

if cs.ResultDelivery != "delivered" && cs.ResultDelivery != "detached" {
	return mcplib.NewToolResultError("child session previous result is still pending delivery"), nil
}

so no subsequent request can ever claim that child again. The only recovery is editing state.db by hand.

internal/state/childsessions.go:74,87,103 — ClaimChildFollowup / CompleteChildFollowup / RestoreChildFollowup are the compare-and-swap primitives AGENTS.md calls "atomic" and race-critical, and all three are at 0.0% coverage. RestoreChildFollowup additionally discards RowsAffected:

func (d *DB) RestoreChildFollowup(id string, cs ChildSession, sendingDelivery string) error {
	_, err := d.db.Exec(`UPDATE child_sessions SET ... WHERE id = ? AND status = 'sending' AND result_delivery = ?`, ...)
	if err != nil { return fmt.Errorf("restoring child follow-up: %w", err) }
	return nil
}

A missed CAS (0 rows updated) is indistinguishable from success — which is exactly the case the caller above needs to detect.

Suggested fix

RestoreChildFollowup returns (bool, error) like its siblings; a missed CAS is reported.
Surface the restore failure in the tool error text so the caller knows the child is wedged.
Attempt a restore on the CompleteChildFollowup error path too.
Add a reaper (or a timestamp-based timeout on claim) that returns stale *_sending rows to delivered.
Direct unit tests for all three CAS primitives, including the contended/miss cases.

Acceptance criteria

No path leaves a child in *_sending without either a successful restore or a surfaced error.
Stale *_sending rows are reclaimed automatically.
All three CAS primitives have tests covering hit and miss.

Effort: M.

Code-quality audit (P2). `internal/mcp/tools_comm.go:152,163`. A child session can end up permanently stuck in `*_sending` with no way out. ```go if err := t.sendMessage(ctx, childID, delivered); err != nil { _ = t.stateDB.RestoreChildFollowup(childID, *cs, sendingDelivery) // :152 — error discarded ... } ... claimed, err = t.stateDB.CompleteChildFollowup(childID, sendingDelivery, delivery) if err != nil { ... return mcplib.NewToolResultError(...) // :163 — row left in *_sending } ``` Two paths leave the row in `ChildResultAsyncSending` / `ChildResultWaitSending`: 1. Send fails **and** the restore also fails — the failure is `_`-discarded, so the caller sees only "sending message to child" and has no idea the row is now wedged. 2. Send succeeds but `CompleteChildFollowup` errors — no restore is attempted at all. The guard at `tools_comm.go:119` only accepts `delivered` / `detached`: ```go if cs.ResultDelivery != "delivered" && cs.ResultDelivery != "detached" { return mcplib.NewToolResultError("child session previous result is still pending delivery"), nil } ``` so **no subsequent request can ever claim that child again**. The only recovery is editing `state.db` by hand. ## Related gap: the CAS primitives are untested `internal/state/childsessions.go:74,87,103` — `ClaimChildFollowup` / `CompleteChildFollowup` / `RestoreChildFollowup` are the compare-and-swap primitives AGENTS.md calls "atomic" and race-critical, and all three are at **0.0% coverage**. `RestoreChildFollowup` additionally discards `RowsAffected`: ```go func (d *DB) RestoreChildFollowup(id string, cs ChildSession, sendingDelivery string) error { _, err := d.db.Exec(`UPDATE child_sessions SET ... WHERE id = ? AND status = 'sending' AND result_delivery = ?`, ...) if err != nil { return fmt.Errorf("restoring child follow-up: %w", err) } return nil } ``` A missed CAS (0 rows updated) is indistinguishable from success — which is exactly the case the caller above needs to detect. ## Suggested fix - `RestoreChildFollowup` returns `(bool, error)` like its siblings; a missed CAS is reported. - Surface the restore failure in the tool error text so the caller knows the child is wedged. - Attempt a restore on the `CompleteChildFollowup` error path too. - Add a reaper (or a timestamp-based timeout on claim) that returns stale `*_sending` rows to `delivered`. - Direct unit tests for all three CAS primitives, including the contended/miss cases. ## Acceptance criteria - [ ] No path leaves a child in `*_sending` without either a successful restore or a surfaced error. - [ ] Stale `*_sending` rows are reclaimed automatically. - [ ] All three CAS primitives have tests covering hit and miss. Effort: M.

dries added the

labels

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

dries commented

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

Author

Owner

Validated against main @ 2d661364: downgraded — core claim fixed. The 5s watcher sweep now recovers stuck rows (internal/server/mcp_watcher.go:88-97: any *_sending row whose waiter is no longer Registered is CASed back to async_pending/waiting), and both failure paths Unregister first, so "permanent, hand-edit state.db" no longer holds. In-request lost-CAS recovery also exists (tools_comm.go:170-179). Coverage claim is stale: TestSendMessageToChild_RestoresCompletionWhenSendFails and friends exercise Claim/Complete/Restore. Residue worth a small cleanup: RestoreChildFollowup still discards RowsAffected (childsessions.go:103-113, now benign) and tools_comm.go:153 still _ =-discards the restore error. Treat as low-priority hygiene, not a bug.

Validated against main @ 2d661364: **downgraded — core claim fixed**. The 5s watcher sweep now recovers stuck rows (internal/server/mcp_watcher.go:88-97: any *_sending row whose waiter is no longer Registered is CASed back to async_pending/waiting), and both failure paths Unregister first, so "permanent, hand-edit state.db" no longer holds. In-request lost-CAS recovery also exists (tools_comm.go:170-179). Coverage claim is stale: TestSendMessageToChild_RestoresCompletionWhenSendFails and friends exercise Claim/Complete/Restore. Residue worth a small cleanup: RestoreChildFollowup still discards RowsAffected (childsessions.go:103-113, now benign) and tools_comm.go:153 still `_ =`-discards the restore error. Treat as low-priority hygiene, not a bug.

dries referenced this issue from a commit

2026-08-23 09:02:46 +02:00

fix: report child follow-up restore failures (#457)

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

2026-08-23 09:02:55 +02:00

fix: resolve session performance and delivery audits #546

dries closed this issue

2026-08-23 16:48:14 +02:00