Skip to content

Commit 2b5a3d4

Browse files
drpcstream,drpcwire: wire the receive-window grant policy into the read path
Install a per-stream recvWindow and drive it from the read/recv path: - HandleFrame intercepts an incoming KindWindowUpdate before the assembler and applies it to the send window. Intercepting early keeps a grant that interleaves with an in-progress message (grants are emitted off the write lock) from disturbing reassembly. - A dispatched KindMessage frame returns credit via recvWindow, emitting a KindWindowUpdate once the accrued amount crosses the threshold and the high-water gate is open. - RawRecv/MsgRecv call recvWindow after dequeuing; consuming can reopen a gate the high-water mark had closed and flush the accrued grant. Grants are control frames, appended without blocking, so emitting them from the reader goroutine is safe. The window is opt-in: with no recvw installed no grants are emitted, and an incoming grant with no send window is ignored, so default behavior is unchanged. Stream-level only; the connection-level receive budget and the enablement path that installs the windows come in later commits. The packet assembler can silently drop an unfinished message when a higher message id supersedes it (a legitimate sequence: a send preempted mid-message, then a cancel). Those bytes were counted into the receive window at dispatch and would inflate buffered forever, wedging the high-water gate shut and deadlocking the sender once its credit ran out. The assembler now records the drop (TakeDiscarded) and HandleFrame releases the bytes as consumed, letting their credit flow back. The release runs last (deferred past the replacement frame's accounting and handlePacket), so the grant decision sees the final buffered state rather than the transient dip, and a terminal packet that supersedes a partial suppresses the grant via termination. emitGrant is also a no-op once the stream has terminated: the ring drains queued messages after close, and returning credit behind the terminal frame would invite the peer to send more doomed data. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
1 parent 48c98b5 commit 2b5a3d4

4 files changed

Lines changed: 400 additions & 1 deletion

File tree

Lines changed: 297 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,297 @@
1+
// Copyright (C) 2026 Cockroach Labs.
2+
// See LICENSE for copying information.
3+
4+
package drpcstream
5+
6+
import (
7+
"context"
8+
"io"
9+
"testing"
10+
"time"
11+
12+
"github.com/zeebo/assert"
13+
"github.com/zeebo/errs"
14+
15+
"storj.io/drpc"
16+
"storj.io/drpc/drpcwire"
17+
)
18+
19+
// captureWriter returns a MuxWriter whose output frames are decoded and
20+
// delivered on the returned channel, so tests can observe emitted grants.
21+
func captureWriter(t *testing.T) (*drpcwire.MuxWriter, <-chan drpcwire.Frame) {
22+
t.Helper()
23+
pr, pw := io.Pipe()
24+
mw := drpcwire.NewMuxWriter(pw, func(error) {})
25+
frames := make(chan drpcwire.Frame, 64)
26+
rd := drpcwire.NewReader(pr)
27+
go func() {
28+
for {
29+
fr, err := rd.ReadFrame()
30+
if err != nil {
31+
return
32+
}
33+
frames <- fr
34+
}
35+
}()
36+
t.Cleanup(func() {
37+
mw.Stop(nil)
38+
<-mw.Done()
39+
_ = pw.Close()
40+
_ = pr.Close()
41+
})
42+
return mw, frames
43+
}
44+
45+
func waitFrame(t *testing.T, frames <-chan drpcwire.Frame) drpcwire.Frame {
46+
t.Helper()
47+
select {
48+
case fr := <-frames:
49+
return fr
50+
case <-time.After(time.Second):
51+
t.Fatal("expected a frame to be written, got none")
52+
return drpcwire.Frame{}
53+
}
54+
}
55+
56+
func assertNoFrame(t *testing.T, frames <-chan drpcwire.Frame) {
57+
t.Helper()
58+
select {
59+
case fr := <-frames:
60+
t.Fatalf("unexpected frame written: %v", fr)
61+
case <-time.After(blockShort):
62+
}
63+
}
64+
65+
func msgFrame(sid, mid uint64, data []byte, done bool) drpcwire.Frame {
66+
return drpcwire.Frame{
67+
ID: drpcwire.ID{Stream: sid, Message: mid},
68+
Kind: drpcwire.KindMessage,
69+
Data: data,
70+
Done: done,
71+
}
72+
}
73+
74+
// An incoming KindWindowUpdate is intercepted and applied to the send window.
75+
func TestStream_IncomingGrantAppliesToSendWindow(t *testing.T) {
76+
st := newGateStream(t)
77+
st.sendw = newSendWindow(0)
78+
79+
assert.NoError(t, st.HandleFrame(drpcwire.WindowUpdateFrame(st.ID(), 500)))
80+
assert.Equal(t, st.sendw.available(), int64(500))
81+
}
82+
83+
// A grant interleaved with an in-progress message is intercepted before the
84+
// assembler, so reassembly is undisturbed.
85+
func TestStream_GrantDoesNotDisturbReassembly(t *testing.T) {
86+
st := newGateStream(t)
87+
sid := st.ID()
88+
89+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 1, []byte("foo"), false)))
90+
assert.NoError(t, st.HandleFrame(drpcwire.WindowUpdateFrame(sid, 100))) // interleaved
91+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 1, []byte("bar"), true)))
92+
93+
got, err := st.RawRecv()
94+
assert.NoError(t, err)
95+
assert.Equal(t, string(got), "foobar")
96+
}
97+
98+
// Dispatching a data frame returns credit once the threshold is met.
99+
func TestStream_DispatchEmitsGrant(t *testing.T) {
100+
mw, frames := captureWriter(t)
101+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
102+
st.recvw = newRecvWindow(1<<20, 100)
103+
104+
assert.NoError(t, st.HandleFrame(msgFrame(st.ID(), 1, make([]byte, 150), true)))
105+
106+
fr := waitFrame(t, frames)
107+
sid, delta, ok := drpcwire.ParseWindowUpdate(fr)
108+
assert.That(t, ok)
109+
assert.Equal(t, sid, st.ID())
110+
assert.Equal(t, delta, uint64(150))
111+
}
112+
113+
// Above the high-water mark, dispatch withholds; consuming resumes and flushes
114+
// the accrued credit.
115+
func TestStream_ConsumeEmitsGrantOnResume(t *testing.T) {
116+
mw, frames := captureWriter(t)
117+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
118+
st.recvw = newRecvWindow(100, 50)
119+
120+
// 120 bytes buffered >= high-water 100 -> withheld.
121+
assert.NoError(t, st.HandleFrame(msgFrame(st.ID(), 1, make([]byte, 120), true)))
122+
assertNoFrame(t, frames)
123+
124+
// Consume -> buffered drops below high-water -> flush accrued 120.
125+
_, err := st.RawRecv()
126+
assert.NoError(t, err)
127+
128+
fr := waitFrame(t, frames)
129+
_, delta, ok := drpcwire.ParseWindowUpdate(fr)
130+
assert.That(t, ok)
131+
assert.Equal(t, delta, uint64(120))
132+
}
133+
134+
// With no receive window installed, no grants are emitted and an incoming grant
135+
// with no send window is harmlessly ignored.
136+
func TestStream_NoWindowsNoGrants(t *testing.T) {
137+
mw, frames := captureWriter(t)
138+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
139+
140+
assert.NoError(t, st.HandleFrame(msgFrame(st.ID(), 1, make([]byte, 150), true)))
141+
assertNoFrame(t, frames)
142+
143+
assert.NoError(t, st.HandleFrame(drpcwire.WindowUpdateFrame(st.ID(), 100)))
144+
}
145+
146+
// Nonconforming grant frames are dropped by the intercept without crediting.
147+
func TestStream_MalformedGrantIgnored(t *testing.T) {
148+
st := newGateStream(t)
149+
st.sendw = newSendWindow(0)
150+
151+
missing := drpcwire.WindowUpdateFrame(st.ID(), 1)
152+
missing.Data = nil // no delta payload
153+
assert.NoError(t, st.HandleFrame(missing))
154+
assert.Equal(t, st.sendw.available(), int64(0))
155+
156+
zero := drpcwire.WindowUpdateFrame(st.ID(), 1)
157+
zero.Data = drpcwire.AppendVarint(nil, 0) // zero delta
158+
assert.NoError(t, st.HandleFrame(zero))
159+
assert.Equal(t, st.sendw.available(), int64(0))
160+
}
161+
162+
// A grant arriving after termination is dropped.
163+
func TestStream_GrantAfterTerminationDropped(t *testing.T) {
164+
st := newGateStream(t)
165+
st.sendw = newSendWindow(0)
166+
167+
st.Cancel(errs.New("boom"))
168+
assert.NoError(t, st.HandleFrame(drpcwire.WindowUpdateFrame(st.ID(), 100)))
169+
assert.Equal(t, st.sendw.available(), int64(0))
170+
}
171+
172+
// An unfinished message superseded by a higher message id must release its
173+
// buffered bytes, or the high-water gate sticks shut and the withheld grant
174+
// is never emitted (sender deadlock once its credit is exhausted).
175+
func TestStream_DiscardedPartialMessageReleasesCredit(t *testing.T) {
176+
mw, frames := captureWriter(t)
177+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
178+
st.recvw = newRecvWindow(100, 50)
179+
sid := st.ID()
180+
181+
// Partial message 1 reaches the high-water mark: grants withheld.
182+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 1, make([]byte, 120), false)))
183+
assertNoFrame(t, frames)
184+
185+
// Message 2 supersedes it; the discarded 120 bytes leave the buffer after
186+
// the replacement is counted, the gate reopens, and all accrued credit
187+
// (120 discarded + 10 replacement) flushes.
188+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 2, make([]byte, 10), true)))
189+
190+
_, delta, ok := drpcwire.ParseWindowUpdate(waitFrame(t, frames))
191+
assert.That(t, ok)
192+
assert.Equal(t, delta, uint64(130))
193+
194+
// The completed message is intact and its own credit accounting holds.
195+
got, err := st.RawRecv()
196+
assert.NoError(t, err)
197+
assert.Equal(t, len(got), 10)
198+
assert.Equal(t, st.recvw.bufferedBytes(), int64(0))
199+
}
200+
201+
// The discard release must not decide on the transient dip before the
202+
// replacement frame is counted: a large replacement that lands above
203+
// high-water withholds everything, including the discarded bytes' credit.
204+
func TestStream_DiscardReleaseSeesReplacementFrame(t *testing.T) {
205+
mw, frames := captureWriter(t)
206+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
207+
st.recvw = newRecvWindow(100, 50)
208+
sid := st.ID()
209+
210+
// Partial message 1 pins buffered at the high-water mark.
211+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 1, make([]byte, 120), false)))
212+
assertNoFrame(t, frames)
213+
214+
// Message 2 (1000 bytes) supersedes it. Final buffered = 1000 >= 100, so
215+
// no grant may be emitted -- releasing the 120 on the dip would be wrong.
216+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 2, make([]byte, 1000), true)))
217+
assertNoFrame(t, frames)
218+
219+
// Consuming the completed message reopens the gate and flushes everything.
220+
_, err := st.RawRecv()
221+
assert.NoError(t, err)
222+
_, delta, ok := drpcwire.ParseWindowUpdate(waitFrame(t, frames))
223+
assert.That(t, ok)
224+
assert.Equal(t, delta, uint64(1120))
225+
}
226+
227+
// A terminal frame that supersedes a partial message must not flush the
228+
// discarded bytes' credit: termination is applied first, and emitGrant
229+
// no-ops on a terminated stream.
230+
func TestStream_TerminalSupersedeEmitsNoGrant(t *testing.T) {
231+
mw, frames := captureWriter(t)
232+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
233+
st.recvw = newRecvWindow(100, 50)
234+
sid := st.ID()
235+
236+
// Partial message 1: grants withheld above high-water.
237+
assert.NoError(t, st.HandleFrame(msgFrame(sid, 1, make([]byte, 120), false)))
238+
assertNoFrame(t, frames)
239+
240+
// A remote cancel with a higher message id discards the partial and
241+
// terminates the stream; the withheld credit must stay unemitted.
242+
assert.NoError(t, st.HandleFrame(drpcwire.Frame{
243+
ID: drpcwire.ID{Stream: sid, Message: 2}, Kind: drpcwire.KindCancel, Done: true,
244+
}))
245+
assert.That(t, st.IsTerminated())
246+
assertNoFrame(t, frames)
247+
}
248+
249+
// Draining queued messages after termination must not emit grants: credit
250+
// returned behind the terminal frame would invite more doomed data.
251+
func TestStream_NoGrantsAfterTermination(t *testing.T) {
252+
mw, frames := captureWriter(t)
253+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
254+
st.recvw = newRecvWindow(100, 50)
255+
256+
// 120 bytes buffered >= high-water 100: grant withheld while live.
257+
assert.NoError(t, st.HandleFrame(msgFrame(st.ID(), 1, make([]byte, 120), true)))
258+
assertNoFrame(t, frames)
259+
260+
st.Cancel(errs.New("boom"))
261+
262+
// The queued message still drains (ring close keeps buffered data
263+
// readable), which on a live stream would flush the accrued 120 bytes of
264+
// credit; after termination it must stay unemitted.
265+
got, err := st.RawRecv()
266+
assert.NoError(t, err)
267+
assert.Equal(t, len(got), 120)
268+
assertNoFrame(t, frames)
269+
}
270+
271+
// rawEnc round-trips raw byte slices for MsgRecv tests.
272+
type rawEnc struct{}
273+
274+
func (rawEnc) Marshal(msg drpc.Message) ([]byte, error) { return *(msg.(*[]byte)), nil }
275+
func (rawEnc) Unmarshal(buf []byte, msg drpc.Message) error {
276+
*(msg.(*[]byte)) = append([]byte(nil), buf...)
277+
return nil
278+
}
279+
280+
// The MsgRecv consume path also returns credit, like RawRecv.
281+
func TestStream_MsgRecvEmitsGrant(t *testing.T) {
282+
mw, frames := captureWriter(t)
283+
st := NewWithOptions(context.Background(), 1, mw, NewBufferPool(), Options{SplitSize: 64 << 10})
284+
st.recvw = newRecvWindow(100, 50)
285+
286+
// 120 bytes buffered >= high-water 100 -> withheld on dispatch.
287+
assert.NoError(t, st.HandleFrame(msgFrame(st.ID(), 1, make([]byte, 120), true)))
288+
assertNoFrame(t, frames)
289+
290+
var msg []byte
291+
assert.NoError(t, st.MsgRecv(&msg, rawEnc{}))
292+
assert.Equal(t, len(msg), 120)
293+
294+
_, delta, ok := drpcwire.ParseWindowUpdate(waitFrame(t, frames))
295+
assert.That(t, ok)
296+
assert.Equal(t, delta, uint64(120))
297+
}

drpcstream/stream.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ type Stream struct {
5959
// flow control is not enabled, in which case data writes are ungated.
6060
sendw *sendWindow
6161

62+
// recvw is the per-stream receive-side flow-control policy. It is nil when
63+
// flow control is not enabled, in which case no grants are emitted.
64+
recvw *recvWindow
65+
6266
mu sync.Mutex // protects state transitions
6367
sigs struct {
6468
send drpcsignal.Signal // set when done sending messages
@@ -211,10 +215,40 @@ func (s *Stream) HandleFrame(fr drpcwire.Frame) (err error) {
211215
return nil
212216
}
213217

218+
// Grants are out-of-band signaling: intercept before the assembler so one
219+
// arriving mid-message cannot disturb reassembly. Without a send window
220+
// (flow control disabled) the grant is ignored.
221+
if fr.Kind == drpcwire.KindWindowUpdate {
222+
if s.sendw != nil {
223+
if _, delta, ok := drpcwire.ParseWindowUpdate(fr); ok {
224+
s.sendw.grant(delta)
225+
}
226+
}
227+
return nil
228+
}
229+
214230
packet, packetReady, err := s.pa.AppendFrame(fr)
215231
if err != nil {
216232
return err
217233
}
234+
235+
// If the assembler dropped an unfinished message (superseded by a higher
236+
// message id, e.g. a send preempted mid-message), its bytes left the buffer:
237+
// release them as consumed or the high-water gate could stick shut. Release
238+
// last — after the replacement frame is counted and handlePacket has run —
239+
// so the gate decision sees the final buffered state, and a terminal packet
240+
// suppresses the grant (emitGrant checks term).
241+
defer func() {
242+
if kind, n := s.pa.TakeDiscarded(); n > 0 && kind == drpcwire.KindMessage && s.recvw != nil {
243+
s.emitGrant(s.recvw.consumed(int64(n)))
244+
}
245+
}()
246+
247+
// A data frame dispatched off the wire may return credit to the sender.
248+
if fr.Kind == drpcwire.KindMessage && s.recvw != nil {
249+
s.emitGrant(s.recvw.dispatched(int64(len(fr.Data))))
250+
}
251+
218252
if !packetReady {
219253
return nil
220254
}
@@ -280,6 +314,19 @@ func (s *Stream) handlePacket(pkt drpcwire.Packet) (err error) {
280314
}
281315
}
282316

317+
// emitGrant sends a credit grant of delta bytes to the peer; no-op unless
318+
// delta is positive and the stream is live (draining a terminated stream's
319+
// queue must not invite more doomed data behind the terminal frame). Grants
320+
// are control frames, which WriteFrame appends without blocking, so this is
321+
// safe on the reader goroutine. Best-effort: a write error means the stream
322+
// is going away.
323+
func (s *Stream) emitGrant(delta int64) {
324+
if delta <= 0 || s.sigs.term.IsSet() {
325+
return
326+
}
327+
_ = s.wr.WriteFrame(drpcwire.WindowUpdateFrame(s.id.Stream, uint64(delta)), nil)
328+
}
329+
283330
//
284331
// helpers
285332
//
@@ -448,8 +495,13 @@ func (s *Stream) RawRecv() (data []byte, err error) {
448495
return nil, err
449496
}
450497
data = append([]byte(nil), b...)
498+
n := len(b)
451499
s.recvQueue.Done()
452500

501+
// Consuming buffered data can return credit to the sender.
502+
if s.recvw != nil {
503+
s.emitGrant(s.recvw.consumed(int64(n)))
504+
}
453505
return data, nil
454506
}
455507

@@ -489,9 +541,14 @@ func (s *Stream) MsgRecv(msg drpc.Message, enc drpc.Encoding) (err error) {
489541
if err != nil {
490542
return err
491543
}
544+
n := len(b)
492545
err = enc.Unmarshal(b, msg)
493546
s.recvQueue.Done()
494547

548+
// Consuming buffered data can return credit to the sender.
549+
if s.recvw != nil {
550+
s.emitGrant(s.recvw.consumed(int64(n)))
551+
}
495552
return err
496553
}
497554

0 commit comments

Comments
 (0)