This is a draft, so I'm judging whether it's ready for human review. It introduces a full MessageConn transport layer for RPC — multiplexer (msgmux), three backends (mem, TCP, WebSocket), and all the client/server plumbing — plus a well-structured CBOR protocol on top of ed25519-signed frames. The architecture is clean, the new Authenticator interface refactor (*http.Request → *Credentials) is a meaningful improvement, and the test suite is genuinely thorough: unary, streaming, bidirectional callbacks, concurrent callstreams, panic propagation, authorization denial, bearer tokens, version gating, and frame-size splitting are all covered.
A few things I want to flag before this moves to merge review:
ws:// connects over TLS despite its name. In message_ws.go, dialWSMessageConn is called for both ws:// and wss:// remotes — and it always prepends "wss://" when dialing. So ws:// does not mean unencrypted; it's just an alias for wss:// at this layer. That's fine behaviour, but the scheme naming is misleading. The messageSchemes slice in transport_msg.go and the docs should clarify this, or ws:// should be dropped and only wss:// accepted, so operators don't assume they're getting a plaintext path.
accept channel capacity of 16 is a hidden backpressure point. In msgmux.go the accept channel is created with a buffer of 16. If the sessionRouter.run loop is slow dispatching streams (or a slow handler holds up the goroutine schedule), a peer sending more than 16 concurrent SYN frames will have subsequent SYNs silently dropped in handleFrame (the case s.accept <- st: select will fall through to case <-s.done:). The frame is then gone — not queued elsewhere, not retried — so the server will never serve those streams. This is a correctness risk under heavy concurrency. Consider either blocking in handleFrame (like a real mux does) or using a larger / dynamically growing queue, and documenting the limit clearly.
Dead session is not cleared after reconnect. In controlSession, when a dialed session's underlying connection dies, ctrlSess is never set to nil. The next call to controlSession returns the closed session, which will fail immediately, rather than redialing. Once a session is dead — whether its readPump exited or its conn closed — there is no path back to the dial() branch. For adopted sessions this is intentional and fine, but for mem://, ws://, and tcp:// clients the documented "lost capability → reresolve" recovery path assumes the transport can recover, which it currently cannot. The simplest fix would be to nil out ctrlSess in the shutdown path and have controlSession detect a closed session and re-dial.
freshTimestamp is one-sided. It rejects requests older than 10 minutes but doesn't reject timestamps in the future. A clock-skewed or replayed-ahead frame can be accepted unconditionally. This is the same issue that exists on the HTTP path, so it isn't a regression — but now that replay protection is being relied on for a new transport it's worth noting for a follow-up.
Everything else — signing, frame splitting, FIN/RST semantics, the inline-capability registry, the perCallRouter/inlineRegistry split — looks solid and well-tested. The draft is ready to graduate to human review with the caveats above noted.
accept channel has a fixed capacity of 16. In handleFrame, if all 16 slots are full the select races s.done, not a block — so the 17th inbound SYN is silently dropped and the stream is lost. Under a burst of concurrent client-opened streams this can silently lose accepted streams. Consider blocking the send (dropping the select entirely and using a plain s.accept <- st) if you're willing to stall the read pump briefly, or growing the buffer significantly and documenting the limit.readPump exits and calls shutdown), ctrlSess is never cleared. The next call to controlSession returns the already-closed session, which will fail immediately on OpenStreamSync, rather than falling through to the dial branch and reconnecting. For mem://, ws://, and tcp:// clients this means a dropped connection is unrecoverable. Consider setting ctrlSess = nil when the session shuts down so subsequent calls can re-dial.dialWSMessageConn always dials wss:// regardless of whether the caller passed ws:// or wss:// as the scheme. So ws:// is effectively an alias for TLS-over-WebSocket, not an unencrypted connection. This is fine security-wise, but the scheme name is misleading — operators might expect ws:// to mean plaintext. Either remove ws:// from the accepted scheme list (keeping only wss://) or add a clear note in the doc comment that ws:// is treated identically to wss://.freshTimestamp rejects requests older than authFreshness (10 min) but accepts any future timestamp, including one far in the future. A client could pre-sign requests with a timestamp 9 minutes in the future, gaining a ~19-minute window in which they remain valid and survive a clock rollback replay. Worth at least bounding: abs(time.Since(t)) > authFreshness.Verdict: caveats
{
"owner": "mirendev",
"repo": "runtime",
"number": 979,
"verdict": "caveats",
"event": "comment",
"summary": "This is a draft, so I'm judging whether it's ready for human review. It introduces a full `MessageConn` transport layer for RPC — multiplexer (`msgmux`), three backends (mem, TCP, WebSocket), and all the client/server plumbing — plus a well-structured CBOR protocol on top of ed25519-signed frames. The architecture is clean, the new Authenticator interface refactor (`*http.Request` → `*Credentials`) is a meaningful improvement, and the test suite is genuinely thorough: unary, streaming, bidirectional callbacks, concurrent callstreams, panic propagation, authorization denial, bearer tokens, version gating, and frame-size splitting are all covered.\n\nA few things I want to flag before this moves to merge review:\n\n**`ws://` connects over TLS despite its name.** In `message_ws.go`, `dialWSMessageConn` is called for both `ws://` and `wss://` remotes — and it always prepends `\"wss://\"` when dialing. So `ws://` does *not* mean unencrypted; it's just an alias for `wss://` at this layer. That's fine behaviour, but the scheme naming is misleading. The `messageSchemes` slice in `transport_msg.go` and the docs should clarify this, or `ws://` should be dropped and only `wss://` accepted, so operators don't assume they're getting a plaintext path.\n\n**`accept` channel capacity of 16 is a hidden backpressure point.** In `msgmux.go` the `accept` channel is created with a buffer of 16. If the `sessionRouter.run` loop is slow dispatching streams (or a slow handler holds up the goroutine schedule), a peer sending more than 16 concurrent SYN frames will have subsequent SYNs silently dropped in `handleFrame` (the `case s.accept \u003c- st:` select will fall through to `case \u003c-s.done:`). The frame is then gone — not queued elsewhere, not retried — so the server will never serve those streams. This is a correctness risk under heavy concurrency. Consider either blocking in `handleFrame` (like a real mux does) or using a larger / dynamically growing queue, and documenting the limit clearly.\n\n**Dead session is not cleared after reconnect.** In `controlSession`, when a dialed session's underlying connection dies, `ctrlSess` is never set to `nil`. The next call to `controlSession` returns the closed session, which will fail immediately, rather than redialing. Once a session is dead — whether its `readPump` exited or its conn closed — there is no path back to the `dial()` branch. For adopted sessions this is intentional and fine, but for `mem://`, `ws://`, and `tcp://` clients the documented \"lost capability → reresolve\" recovery path assumes the transport can recover, which it currently cannot. The simplest fix would be to nil out `ctrlSess` in the `shutdown` path and have `controlSession` detect a closed session and re-dial.\n\n**`freshTimestamp` is one-sided.** It rejects requests older than 10 minutes but doesn't reject timestamps in the *future*. A clock-skewed or replayed-ahead frame can be accepted unconditionally. This is the same issue that exists on the HTTP path, so it isn't a regression — but now that replay protection is being relied on for a new transport it's worth noting for a follow-up.\n\nEverything else — signing, frame splitting, FIN/RST semantics, the inline-capability registry, the `perCallRouter`/`inlineRegistry` split — looks solid and well-tested. The draft is ready to graduate to human review with the caveats above noted.",
"comments": [
{
"path": "pkg/rpc/msgmux.go",
"line": 78,
"side": "RIGHT",
"body": "The `accept` channel has a fixed capacity of 16. In `handleFrame`, if all 16 slots are full the select races `s.done`, not a block — so the 17th inbound SYN is silently dropped and the stream is lost. Under a burst of concurrent client-opened streams this can silently lose accepted streams. Consider blocking the send (dropping the select entirely and using a plain `s.accept \u003c- st`) if you're willing to stall the read pump briefly, or growing the buffer significantly and documenting the limit.",
"ai_prompt": "In pkg/rpc/msgmux.go, the accept channel is created with a buffer of 16 (line 78: `accept: make(chan *msgStream, 16)`). In handleFrame around line 177-183, when a new stream arrives and the accept channel is full, the select falls through to `case \u003c-s.done:` and the stream is silently dropped — the SYN frame is consumed but the caller's AcceptStream never sees it. Fix this by blocking on the send instead of selecting, e.g. replace the select block with a plain `s.accept \u003c- st` (while still checking s.done to avoid a goroutine leak), or document clearly what the capacity bound means for callers and increase it to something that won't be hit in realistic usage."
},
{
"path": "pkg/rpc/transport_msg.go",
"line": 42,
"side": "RIGHT",
"body": "Once a dialed session's connection closes (its `readPump` exits and calls `shutdown`), `ctrlSess` is never cleared. The next call to `controlSession` returns the already-closed session, which will fail immediately on `OpenStreamSync`, rather than falling through to the `dial` branch and reconnecting. For `mem://`, `ws://`, and `tcp://` clients this means a dropped connection is unrecoverable. Consider setting `ctrlSess = nil` when the session shuts down so subsequent calls can re-dial.",
"ai_prompt": "In pkg/rpc/transport_msg.go, controlSession (around line 38) checks `t.ctrlSess != nil` and returns the cached session immediately. When that session's underlying connection dies its readPump calls shutdown, but ctrlSess is never set back to nil. So the next call to controlSession returns a closed session that fails immediately, rather than re-dialing. Fix this by hooking into session closure: after `startAcceptLoop` launches, watch for the session's `done` channel in a goroutine, and when it fires, lock t.mu, compare and clear t.ctrlSess, then unlock — so the next controlSession call can re-dial using t.dial."
},
{
"path": "pkg/rpc/message_ws.go",
"line": 52,
"side": "RIGHT",
"body": "`dialWSMessageConn` always dials `wss://` regardless of whether the caller passed `ws://` or `wss://` as the scheme. So `ws://` is effectively an alias for TLS-over-WebSocket, not an unencrypted connection. This is fine security-wise, but the scheme name is misleading — operators might expect `ws://` to mean plaintext. Either remove `ws://` from the accepted scheme list (keeping only `wss://`) or add a clear note in the doc comment that `ws://` is treated identically to `wss://`."
},
{
"path": "pkg/rpc/signing.go",
"line": 63,
"side": "RIGHT",
"body": "`freshTimestamp` rejects requests older than `authFreshness` (10 min) but accepts any future timestamp, including one far in the future. A client could pre-sign requests with a timestamp 9 minutes in the future, gaining a ~19-minute window in which they remain valid and survive a clock rollback replay. Worth at least bounding: `abs(time.Since(t)) \u003e authFreshness`."
}
],
"posted_to_pr": true,
"draft": true
}