Skip to content

fix(jsonrpc2): stop panicking when a stream payload races StreamOpen init#197

Open
Ricardo-M-L wants to merge 1 commit into
MoonshotAI:mainfrom
Ricardo-M-L:contrib/jsonrpc2-no-panic-on-pending-receiver
Open

fix(jsonrpc2): stop panicking when a stream payload races StreamOpen init#197
Ricardo-M-L wants to merge 1 commit into
MoonshotAI:mainfrom
Ricardo-M-L:contrib/jsonrpc2-no-panic-on-pending-receiver

Conversation

@Ricardo-M-L

Copy link
Copy Markdown

What

recv() in go/wire/jsonrpc2/codec.go reacts to a StreamOpen payload by registering a placeholder entry in the receiver map:

if payload.Stream == StreamOpen {
    c.receiverlock.Lock()
    c.receivers[payload.ID] = nil   // ← placeholder, real channel comes later
    c.receiverlock.Unlock()
    go c.watchidle(payload.ID)
}

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 in consumependings() with ok == true and receiver == nil:

receiver, ok := c.receivers[payload.ID]
...
if receiver == nil {
    panic("receiver is nil")   // ← session dies
}

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.AfterFunc tick 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.

if receiver == nil {
    time.AfterFunc(10*time.Millisecond, func() {
        go requeue(receiverid)
    })
    continue
}

The fdlist element stays in place (we didn't call rmelement), so when consumependings picks 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 ./... passes
  • go test ./wire/jsonrpc2/... -race passes (ok github.com/MoonshotAI/kimi-agent-sdk/go/wire/jsonrpc2 11.455s)
  • The only code path changed is the one that previously panicked, so all happy-path tests still exercise the original behaviour

…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.
@Ricardo-M-L

Copy link
Copy Markdown
Author

Friendly ping 🙏 — opened 3 days ago, no CI trigger / no review yet.

Quick summary for triage: 21-line race-condition fix in go/wire/jsonrpc2/codec.go. When a stream payload arrives between c.receivers[id] = nil (placeholder at line 383, set on StreamOpen) and the real channel being installed by the handler (receiver.Receiver(...) at line 442/616), the dispatcher hits panic("receiver is nil"). Fix is to requeue the message after 10ms via time.AfterFunc instead of panicking — no behavior change in the happy path. go test -race clean.

@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.

@Ricardo-M-L

Copy link
Copy Markdown
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant