fix(jsonrpc2): stop panicking when a stream payload races StreamOpen init#197
Conversation
…init
`recv()` registers a placeholder `c.receivers[id] = nil` when it sees a
StreamOpen payload, and the real channel is only installed later on a
different goroutine when the request handler runs
`receiver.Receiver(...)`. Between those two points, any follow-up
stream payload for the same id lands in `consumependings()` with
`ok == true` and `receiver == nil`. The previous code reacted with
`panic("receiver is nil")`, which tears down the entire codec
goroutine — i.e. the whole session — for a benign in-flight
initialisation race that the protocol explicitly allows.
Requeue the payload instead, gated on a 10ms `time.AfterFunc` tick so
we don't hot-spin if the handler is slow. This matches the existing
`<-time.After(timeout)` recovery a few lines below, which also
requeues on a transient block.
`go test ./wire/jsonrpc2/... -race` passes; the change only affects
the previously-panicking branch, so all happy-path tests still
exercise the original behaviour.
|
Friendly ping 🙏 — opened 3 days ago, no CI trigger / no review yet. Quick summary for triage: 21-line race-condition fix in @x5iu (looks like you touched this file most recently) / @wbxl2000 — could you take a look when you get a moment? Happy to address any review notes. |
|
Hi! Just a friendly ping on this PR. It's been open for a while — would appreciate a review when you get a chance. Thanks for your time! |
What
recv()ingo/wire/jsonrpc2/codec.goreacts to aStreamOpenpayload by registering a placeholder entry in the receiver map:The real channel is only installed later, on a different goroutine, when the request handler runs
receiver.Receiver(...)(line 442 for requests, 616 for responses). Between those two points, any follow-up stream payload for the same id lands inconsumependings()withok == trueandreceiver == nil:The protocol explicitly allows the StreamOpen → first StreamData ordering to land back-to-back (callers stream
StreamOpen → data → data → ... → StreamClose), so this race is reachable in normal traffic — and panicking takes down the entire codec goroutine for what is fundamentally a benign in-flight initialisation race.Fix
Requeue the payload instead of panicking, gated on a 10 ms
time.AfterFunctick so we don't hot-spin if the handler is slow installing the real channel. Same recovery shape as the existing<-time.After(timeout)branch a few lines below, which already requeues when a send into a receiver blocks.The fdlist element stays in place (we didn't call
rmelement), so whenconsumependingspicks up the requeued waker it finds the same payload again. If the handler is slow we tick at ~100 Hz per stuck stream — orders of magnitude cheaper than dropping the session.Verification
go build ./...passesgo test ./wire/jsonrpc2/... -racepasses (ok github.com/MoonshotAI/kimi-agent-sdk/go/wire/jsonrpc2 11.455s)