Merged
Conversation
Contributor
Reviewer's GuideThis PR enriches the multi-packet response roadmap with concrete implementation sub-tasks for driving, stamping, and terminating streaming frames, and captures the underlying connection-actor design decisions in the streaming design document. Sequence diagram for multi-packet response handling in Connection actorsequenceDiagram
participant RequestHandler
participant ConnectionActor
participant MessageReceiver
participant FrameSender
participant ProtocolHooks
RequestHandler->>ConnectionActor: Send request (with correlation_id)
ConnectionActor->>MessageReceiver: Start receiving multi-packet response
loop For each Message
MessageReceiver->>ConnectionActor: Receive Message
ConnectionActor->>FrameSender: Serialize Message to Frame (stamp correlation_id)
FrameSender->>ConnectionActor: Frame sent
ConnectionActor->>ProtocolHooks: Emit tracing/metrics for frame
end
MessageReceiver->>ConnectionActor: Channel closes (None from recv)
ConnectionActor->>FrameSender: Send end-of-stream marker frame (reuse correlation_id)
ConnectionActor->>ProtocolHooks: Notify lifecycle hooks for stream termination
ConnectionActor->>ConnectionActor: Log termination reason
Class diagram for updated Connection actor multi-packet response logicclassDiagram
class ConnectionActor {
+select_loop()
+handle_multi_packet_response(receiver, correlation_id)
+serialize_message_to_frame(message, correlation_id)
+emit_tracing_and_metrics(frame)
+send_end_of_stream_marker(correlation_id)
+notify_protocol_hooks(stream_terminated)
}
class MessageReceiver {
+recv() Message | None
}
class FrameSender {
+send(frame)
}
class ProtocolHooks {
+on_frame_sent(frame)
+on_stream_terminated(request_id)
}
ConnectionActor "1" -- "1" MessageReceiver : owns
ConnectionActor "1" -- "1" FrameSender : uses
ConnectionActor "1" -- "1" ProtocolHooks : notifies
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds a new “Design Decisions” section to the multi-packet streaming design doc and expands the roadmap’s Phase 6 with concrete steps for connection loop ownership, correlation ID propagation, EOS handling, observability, API ergonomics, and tests. No code or public API signatures are changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ConnectionActor as Connection Actor (select! loop)
participant Receiver as Multi‑packet Receiver
participant Serializer as Serialization Hooks
participant Hooks as Protocol Lifecycle Hooks
participant Obs as Tracing/Metrics
participant Transport as Outbound Transport
Client->>ConnectionActor: Request (with correlation_id)
Note over ConnectionActor: Store correlation_id
rect rgba(230,245,255,0.6)
loop While Receiver yields Message
ConnectionActor->>Receiver: recv()
Receiver-->>ConnectionActor: Message
ConnectionActor->>Serializer: to Frame (stamp correlation_id)
Serializer-->>ConnectionActor: Frame
ConnectionActor->>Obs: trace/metrics per frame
ConnectionActor->>Transport: send(Frame)
end
end
alt Receiver closed (EOS)
ConnectionActor->>Obs: log EOS + reason
ConnectionActor->>Transport: send(EOS with correlation_id)
ConnectionActor->>Hooks: on_stream_complete/cleanup
else Error
ConnectionActor->>Obs: log error
ConnectionActor->>Hooks: on_stream_error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/roadmap.md:202` </location>
<code_context>
- [ ] Modify the `Connection` actor: upon receiving `Response::MultiPacket`,
it should consume messages from the receiver and send each one as a `Frame`.
+ - [ ] Extend the outbound `select!` loop to own the receiver so
+ multi-packet responses share the same back-pressure and shutdown handling
+ as other frame sources.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 2
<location> `docs/roadmap.md:205` </location>
<code_context>
+ - [ ] Extend the outbound `select!` loop to own the receiver so
+ multi-packet responses share the same back-pressure and shutdown handling
+ as other frame sources.
+ - [ ] Convert each received `Message` into a `Frame` via the existing
+ serialization helpers rather than bypassing protocol hooks or metrics.
+ - [ ] Emit tracing and metrics for each forwarded frame so streaming
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 3
<location> `docs/roadmap.md:207` </location>
<code_context>
+ as other frame sources.
+ - [ ] Convert each received `Message` into a `Frame` via the existing
+ serialization helpers rather than bypassing protocol hooks or metrics.
+ - [ ] Emit tracing and metrics for each forwarded frame so streaming
+ traffic remains visible to observability pipelines.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 4
<location> `docs/roadmap.md:216` </location>
<code_context>
+ control to the multi-packet dispatcher.
+ - [ ] Stamp the stored `correlation_id` onto every frame emitted from the
+ channel before it is queued for transmission.
+ - [ ] Guard against accidental omission by asserting in debug builds and
+ covering the behaviour with targeted tests.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 5
<location> `docs/roadmap.md:224` </location>
<code_context>
+ reason for operational insight.
+ - [ ] Send the designated end-of-stream marker frame through the same
+ send path, reusing the request's `correlation_id`.
+ - [ ] Notify protocol lifecycle hooks so higher layers can tidy any
+ per-request state when a stream drains naturally.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 6
<location> `docs/multi-packet-and-streaming-responses-design.md:266` </location>
<code_context>
+## 9. Design Decisions
+
+- Multi-packet channels are driven inside the existing connection actor
+ `select!` loop rather than by a detached task. This keeps a single locus for
+ back-pressure, aligns with the production resilience guidance on avoiding
+ orphaned tasks, and ensures orderly shutdown propagates to streaming work.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 7
<location> `docs/multi-packet-and-streaming-responses-design.md:267` </location>
<code_context>
+
+- Multi-packet channels are driven inside the existing connection actor
+ `select!` loop rather than by a detached task. This keeps a single locus for
+ back-pressure, aligns with the production resilience guidance on avoiding
+ orphaned tasks, and ensures orderly shutdown propagates to streaming work.
+- The actor captures the request's `correlation_id` before iterating the
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 8
<location> `docs/multi-packet-and-streaming-responses-design.md:268` </location>
<code_context>
+- Multi-packet channels are driven inside the existing connection actor
+ `select!` loop rather than by a detached task. This keeps a single locus for
+ back-pressure, aligns with the production resilience guidance on avoiding
+ orphaned tasks, and ensures orderly shutdown propagates to streaming work.
+- The actor captures the request's `correlation_id` before iterating the
+ channel and stamps it onto every serialised frame. This preserves protocol
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 9
<location> `docs/multi-packet-and-streaming-responses-design.md:270` </location>
<code_context>
+ back-pressure, aligns with the production resilience guidance on avoiding
+ orphaned tasks, and ensures orderly shutdown propagates to streaming work.
+- The actor captures the request's `correlation_id` before iterating the
+ channel and stamps it onto every serialised frame. This preserves protocol
+ invariants without requiring handlers to mutate frames post-creation and
+ mirrors the message attribution strategy outlined in the capability roadmap.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 10
<location> `docs/multi-packet-and-streaming-responses-design.md:271` </location>
<code_context>
+ orphaned tasks, and ensures orderly shutdown propagates to streaming work.
+- The actor captures the request's `correlation_id` before iterating the
+ channel and stamps it onto every serialised frame. This preserves protocol
+ invariants without requiring handlers to mutate frames post-creation and
+ mirrors the message attribution strategy outlined in the capability roadmap.
+- When the receiver reports closure, the actor emits the configured
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 11
<location> `docs/multi-packet-and-streaming-responses-design.md:272` </location>
<code_context>
+- The actor captures the request's `correlation_id` before iterating the
+ channel and stamps it onto every serialised frame. This preserves protocol
+ invariants without requiring handlers to mutate frames post-creation and
+ mirrors the message attribution strategy outlined in the capability roadmap.
+- When the receiver reports closure, the actor emits the configured
+ end-of-stream marker via the normal send path and then triggers the protocol
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 12
<location> `docs/multi-packet-and-streaming-responses-design.md:274` </location>
<code_context>
+ invariants without requiring handlers to mutate frames post-creation and
+ mirrors the message attribution strategy outlined in the capability roadmap.
+- When the receiver reports closure, the actor emits the configured
+ end-of-stream marker via the normal send path and then triggers the protocol
+ lifecycle hooks. This guarantees downstream clean-up and observability cues
+ stay consistent with other stream completions.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 13
<location> `docs/multi-packet-and-streaming-responses-design.md:275` </location>
<code_context>
+ mirrors the message attribution strategy outlined in the capability roadmap.
+- When the receiver reports closure, the actor emits the configured
+ end-of-stream marker via the normal send path and then triggers the protocol
+ lifecycle hooks. This guarantees downstream clean-up and observability cues
+ stay consistent with other stream completions.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet point exceeds 80 columns and should be wrapped for readability.
Please wrap this bullet point so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/multi-packet-and-streaming-responses-design.md(1 hunks)docs/roadmap.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
docs/**/*.md: Use docs/ markdown as the source of truth for requirements and decisions
Proactively update docs/ when requirements, dependencies, or architecture change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name exempt)When long lines are warranted in ...
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
🔍 Remote MCP Deepwiki, Ref
Concise additional context for PR #362
-
PR metadata: Title "Detail multi-packet response steps", #362, author leynos, branch codex/identify-steps-for-core-library-implementation → main, created 2025-09-16T11:16:23Z, label: codex.
-
Files changed/read: docs/multi-packet-and-streaming-responses-design.md and docs/roadmap.md (branch codex/...); both add concrete implementation steps and design rationale for multi-packet streaming.
-
Key design decisions documented:
- Multi-packet channels are driven inside the existing connection actor’s outbound select! loop (single back-pressure locus, avoid orphan tasks, orderly shutdown).
- The actor captures the originating request’s correlation_id and stamps it onto every serialized frame; debug assertions/tests are proposed to guard against omission.
- On receiver closure the actor sends an end‑of‑stream (EOS) marker via the normal send path (reusing the request correlation_id) and triggers protocol lifecycle hooks for per-request cleanup/observability.
-
Roadmap / testing / observability notes:
- Phase 6 (Multi-Packet Streaming Responses) expanded with per-frame observability (tracing/metrics), integration tests for interleaved multi-packet responses and EOS handling, and an ergonomic API suggestion (Sender, Response).
-
Repo docs relevant to review: repository wiki contains a ConnectionActor page and Development Roadmap page — review those for existing actor semantics and lifecycle hooks that the PR relies on.
-
Quick review action items (derived from the docs):
- Confirm outbound select! loop will own the receiver and preserve back-pressure semantics.
- Confirm serialization path and enqueue/send path reliably stamp and preserve correlation_id for every frame (and in EOS).
- Ensure lifecycle hooks invoked on EOS match existing cleanup/observability behavior.
- Ensure proposed tests and metrics cover interleaving and EOS cases; run formatting (make fmt) as noted.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test
- GitHub Check: Sourcery review
🔇 Additional comments (1)
docs/roadmap.md (1)
202-208: LGTM: Back‑pressure ownership, ID propagation tasks, and EOS handling are well scoped.Proceed with implementation as outlined.
Also applies to: 212-217, 220-225
leynos
deleted the
codex/identify-steps-for-core-library-implementation
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c93fbadb8c8322bb18ac24589a5c84
Summary by Sourcery
Detail the multi-packet response workflow in the roadmap and document the design rationale for integrating streaming responses within the connection actor
Documentation: