*: extract PacketAssembler for frame-to-packet assembly

[shubhamdhama]

Extract the frame assembly logic from Stream.HandleFrame into a
reusable PacketAssembler type in drpcwire. Both the stream and the
manager now use their own PacketAssembler instance, keeping assembly
logic in one place.

The manager's assembler handles the invoke sequence (metadata +
invoke), which removes the restriction that only KindMessage packets
could be split across frames. This also simplifies NewServerStream
from a packet-at-a-time loop into a single receive.

[Copilot]

Pull request overview

This PR refactors wire handling to separate frame parsing from frame-to-packet assembly by introducing a reusable PacketAssembler in drpcwire. This centralizes assembly invariants and lets both drpcstream.Stream and drpcmanager.Manager assemble packets from frames in a consistent way, while drpcwire.Reader now returns individual frames.

Changes:

• drpcwire.Reader now exposes ReadFrame (frame parsing only) and tests were updated accordingly.
• Added drpcwire.PacketAssembler to assemble frames into packets with monotonicity/kind invariants and stream-ID consistency.
• Updated drpcstream.Stream and drpcmanager.Manager to ingest frames and assemble/route complete packets; expanded test coverage around frame handling.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
drpcwire/reader.go	Convert reader from packet reconstruction to single-frame reads (ReadFrame).
drpcwire/reader_test.go	Update tests to validate frame-by-frame reading and buffering behavior.
drpcwire/packet_builder.go	Introduce PacketAssembler for reusable frame-to-packet assembly and invariants.
drpcstream/stream.go	Replace HandlePacket entrypoint with HandleFrame + internal handlePacket.
drpcstream/stream_test.go	Update existing tests for frame handling; add a large suite of HandleFrame tests.
drpcmanager/manager.go	Switch manager reader loop to ReadFrame, add invoke-sequence assembly and routing updates.
drpcmanager/manager_test.go	Add tests for global monotonicity, invoke sequencing, and frame delivery behavior.
drpcconn/conn_test.go	Update conn tests to use ReadFrame for verifying on-wire output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

PacketAssembler uses pk.ID.Stream==0 as the sentinel for “stream ID not set yet / infer from first frame”. That makes SetStreamID(0) indistinguishable from not calling SetStreamID at all, so a stream with ID 0 cannot enforce stream-ID matching and may accept frames for the wrong stream (it will just infer the first frame’s ID). If stream ID 0 is valid in this codebase, track “stream ID set” with a separate boolean; if stream ID 0 is invalid, consider rejecting it explicitly in SetStreamID/AppendFrame (and updating call sites/tests accordingly).

[shubhamdhama]

I don't feel strongly about this comment. If other agree let me know.

[cthumuluru-crdb]

I prefer we differentiate between uninitialized vs initialized but to default value.

While it is not important just yet, once we introduce flow control we have to dedicate a stream for control messages that is not subjected to flow control. In general that stream will be stream 0.

[shubhamdhama]

Fixed.

[suj-krishnan]

When assembling a KindMessage packet, since Frame has Kind too, we can have some sanity checks even before calling AppendFrame in HandleFrame to bail out early when we get a invalid frames in between, instead of waiting till handlePacket to do the validation.

Example:

Stream s1 was successfully created and curr.ID = s1.
manageReader() gets the below frames for this stream after KindInvoke:

[s1, m2, KindInvokeMetadata, d=f]
--> Invalid frame but we don't detect it because stream id is valid (curr != nil && incomingFrame.ID.Stream == curr.ID()). curr.HandleFrame is called, goes to the fr.ID.Message > pa.pk.ID.Message condition in AppendFrame, and a new packet is started.

[s1, m3, KindMessage, d=f]
--> curr.HandleFrame is called, HandleFrame starts a new packet again

[s1, m3, KindMessage, d=t]
--> Packet completed, so HandleFrame calls handlePacket

In this case, we go ahead and assemble the KindMessage packet even though the KindInvokeMetadata frame before it was invalid. We may eventually error out whenever the continuation frame for the KindInvokeMetadata packet arrives but the error could have been detected much earlier, something like this.

func (s *Stream) HandleFrame(fr drpcwire.Frame) (err error) {
       if s.sigs.term.IsSet() {
               return nil
       }

       if fr.Kind == drpcwire.KindInvoke || fr.Kind == drpcwire.KindInvokeMetadata {
               err := drpc.ProtocolError.New("invoke on existing stream")
               s.terminate(err)
               return err
       }

       packet, packetReady, err := s.pb.AppendFrame(fr)
       if err != nil {
               return err
       }
       if !packetReady {
               return nil
       }
       return s.handlePacket(packet)
 }

[suj-krishnan]

Also, I think we need some guard against never getting a done=true frame in the code. A max limit on the packet size can do it I guess.

[shubhamdhama]

I didn't get this one.

[suj-krishnan]

Updated the above comment with a clearer example

[cthumuluru-crdb]

s1, m1, KindMessage, d=f
s1, m2, KindInvokeMetadata, d=f (starts a new packet)

I guess this state may not be possible?

[suj-krishnan]

I actually think there are more such cases to think about here - mainly related to validating frames by kind early. We can discuss this on Monday.

[cthumuluru-crdb]

Sure. One thing to keep in mind is, since the transport is TCP unless the packets are written from different goroutines (ex: cancel), some combinations are not even possible.

[suj-krishnan]

Sure. One thing to keep in mind is, since the transport is TCP unless the packets are written from different goroutines (ex: cancel), some combinations are not even possible.

Yes, these fall under protocol violations - they cannot happen unless the client is buggy. The existing code already guards against KindInvokeMetadata packets coming out of turn (fails with a protocol violation "invoke on existing stream") - I'm just suggesting that for such errors, there are cases where we know as soon as the frame is read that it's an invalid frame, we don't need to wait to read the whole packet before erroring out. There is also a possibility of handling valid packet kinds (KindError, KindCancel) earlier, which I wanted to discuss on Monday.

[shubhamdhama]

[discussed offline too]

I see your point. But I'm not convinced that we should move it above. I certainly see this as an improvement to avoid doing unnecessary work but this is a rare case and likely only happen while development. Keeping it along with other switch cases keep things more readable without any performance hit.

[cthumuluru-crdb]

Can one of you please summarize the discussion on resolve this comment if this is fully addressed?

[cthumuluru-crdb]

s1, m1, KindMessage, d=f
s1, m2, KindInvokeMetadata, d=f (starts a new packet)

I guess this state may not be possible?

[shubhamdhama]

I've moved the invoke packet assembler and metadata from from manageReader to Manager as a field and clearly mentioned that Manager.metadata/pa are used in handleInvokeFrame. This is completely for readability. Having them passed as an argument to handleInvoke.. was a mouthful and having them initialized in manageReader adds up to the context one has to keep in mind. Now that it's in Manager it's much readable IMO at the obvious cost of increased scope but some documentation and common-sense should save us there.
I'm focusing on readability to reduce the number of things we have to keep in mind while reading manageReader. In subsequent PR I will change these fields to map from string to these types and no change would appear in manageReader.

I'm think we may move manageReader to its own struct.

[cthumuluru-crdb]

Can one of you please summarize the discussion on resolve this comment if this is fully addressed?

[cthumuluru-crdb]

I prefer we differentiate between uninitialized vs initialized but to default value.

While it is not important just yet, once we introduce flow control we have to dedicate a stream for control messages that is not subjected to flow control. In general that stream will be stream 0.