Implement correlation ID stamping for multi-packet responses#371
Implement correlation ID stamping for multi-packet responses#371
Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 9 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)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughIntroduce a new CorrelatableFrame trait and implement it for Envelope; wire multi‑packet correlation state into ConnectionActor with set_multi_packet_with_correlation, stamp correlation IDs on multi‑packet frames (with debug assertions), update docs/roadmap, and add tests and BDD/world helpers for multi‑packet scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Conn as ConnectionActor
participant MP as Multi-packet Channel
participant Frame as Frame (CorrelatableFrame)
participant Out as Outbound Queue
Client->>Conn: set_multi_packet_with_correlation(rx, correlation_id?)
Note over Conn: install_multi_packet(rx) and store optional expected correlation
loop Drain multi-packet frames
MP-->>Conn: next Frame
Conn->>Frame: apply_multi_packet_correlation(frame)
alt correlation present
Note right of Conn #dff2d8: set correlation_id on frame\ndebug_assert stamped id == expected
else no correlation
Note right of Conn #f7f0f0: leave frame unchanged
end
Conn->>Out: enqueue Frame
end
Conn->>Conn: on channel close or shutdown -> clear_multi_packet()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideThis PR extends the connection actor to propagate request-level correlation IDs onto multi-packet response frames by introducing a generic stamping trait, encapsulating channel state and stamping config, adding a dedicated API and stamping logic with debug invariants, and covering the behavior with new tests and updated docs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider replacing the boxed closure stamper with a simple stored Option and stamping directly in apply_multi_packet_correlation to eliminate dynamic dispatch and simplify the code.
- The reset of multi_packet_stamper and multi_packet_correlation is scattered across several branches – consolidate the teardown into install_multi_packet (or a dedicated clear method) to avoid potential cleanup gaps.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the boxed closure stamper with a simple stored Option<u64> and stamping directly in apply_multi_packet_correlation to eliminate dynamic dispatch and simplify the code.
- The reset of multi_packet_stamper and multi_packet_correlation is scattered across several branches – consolidate the teardown into install_multi_packet (or a dedicated clear method) to avoid potential cleanup gaps.
## Individual Comments
### Comment 1
<location> `tests/correlation_id.rs:33-42` </location>
<code_context>
+#[tokio::test]
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative and edge case tests for correlation ID stamping.
Please add tests for cases where the correlation ID is None or pre-set to a different value to ensure robust handling of all scenarios.
Suggested implementation:
```rust
#[tokio::test]
async fn multi_packet_frames_carry_request_correlation_id() {
let cid = 7u64;
let (tx, rx) = mpsc::channel(4);
tx.send(Envelope::new(1, None, vec![1]))
.await
.expect("send frame");
tx.send(Envelope::new(1, Some(99), vec![2]))
.await
.expect("send frame");
drop(tx);
}
// Negative test: correlation ID is None
#[tokio::test]
async fn packet_with_none_correlation_id_is_handled() {
let (tx, mut rx) = mpsc::channel(1);
tx.send(Envelope::new(1, None, vec![42]))
.await
.expect("send frame");
drop(tx);
let mut out = Vec::new();
// Replace with actual actor invocation as in your codebase
actor_run(&mut out, &mut rx).await.expect("actor run failed");
// Expect correlation_id to be None
assert!(out.iter().all(|e| e.correlation_id().is_none()));
}
// Edge case: correlation ID is pre-set to a different value
#[tokio::test]
async fn packet_with_pre_set_correlation_id_is_preserved() {
let preset_cid = 12345u64;
let (tx, mut rx) = mpsc::channel(1);
tx.send(Envelope::new(1, Some(preset_cid), vec![99]))
.await
.expect("send frame");
drop(tx);
let mut out = Vec::new();
// Replace with actual actor invocation as in your codebase
actor_run(&mut out, &mut rx).await.expect("actor run failed");
// Expect correlation_id to be the preset value
assert!(out.iter().all(|e| e.correlation_id() == Some(preset_cid)));
}
```
- Replace `actor_run(&mut out, &mut rx)` with the actual function or method that processes the envelopes and populates `out`, matching your codebase's conventions.
- If your test harness or actor invocation differs, adjust the test body accordingly.
- Ensure that the `Envelope` and `correlation_id()` methods are available and behave as expected.
</issue_to_address>
### Comment 2
<location> `src/connection.rs:124` </location>
<code_context>
/// This preserves fairness with queued sources.
/// The actor emits the protocol terminator when the sender closes the channel.
multi_packet: Option<mpsc::Receiver<F>>,
+ multi_packet_stamper: Option<MultiPacketStamper<F>>,
+ multi_packet_correlation: Option<u64>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the three multi_packet-related fields and their helper methods into a single MultiPacketContext struct to centralize logic and simplify state management.
```markdown
You can collapse the three `multi_packet`‐related fields and their helper methods into a single `MultiPacketContext` struct. This centralizes setup/teardown and removes all of the `Option` juggling and manual clears.
1. Define the context in your module:
```rust
type MultiPacketStamper<F> = Box<dyn FnMut(&mut F) + Send + 'static>;
struct MultiPacketContext<F> {
channel: Option<mpsc::Receiver<F>>,
stamper: Option<MultiPacketStamper<F>>,
correlation_id: Option<u64>,
}
impl<F> MultiPacketContext<F> {
fn set(
&mut self,
channel: Option<mpsc::Receiver<F>>,
stamper: Option<MultiPacketStamper<F>>,
correlation_id: Option<u64>,
) {
self.channel = channel;
self.stamper = stamper;
self.correlation_id = correlation_id;
}
fn apply(&mut self, frame: &mut F)
where
F: CorrelatableFrame,
{
if let Some(stamper) = &mut self.stamper {
stamper(frame);
}
}
fn clear(&mut self) {
self.stamper = None;
self.correlation_id = None;
}
fn take_channel(&mut self) -> Option<mpsc::Receiver<F>> {
self.channel.take()
}
}
```
2. Collapse your three fields in `ConnectionActor`:
```rust
pub struct ConnectionActor<F, E> {
// …
multi_ctx: MultiPacketContext<F>,
// …
}
```
3. Update constructors and setters to delegate into the context:
```rust
impl<F, E> ConnectionActor<F, E>
where
F: CorrelatableFrame,
E: std::fmt::Debug,
{
pub fn set_multi_packet(&mut self, channel: Option<mpsc::Receiver<F>>) {
self.multi_ctx.set(channel, None, None);
}
pub fn set_multi_packet_with_correlation(
&mut self,
channel: Option<mpsc::Receiver<F>>,
correlation_id: Option<u64>,
) {
let expected = correlation_id;
let stamper: MultiPacketStamper<F> = Box::new(move |frame: &mut F| {
frame.set_correlation_id(expected);
debug_assert_eq!(
frame.correlation_id(),
expected,
"mismatch…"
);
});
self.multi_ctx.set(channel, Some(stamper), correlation_id);
}
}
```
4. In your queue‐processing and shutdown paths, replace the manual clears with:
```rust
// in process_queue:
if matches!(kind, QueueKind::Multi) {
self.multi_ctx.apply(&mut frame);
}
// in handle_closed_receiver / start_shutdown:
if let Some(rx) = self.multi_ctx.take_channel() {
rx.close();
state.mark_closed();
}
self.multi_ctx.clear();
```
This collapses all the scattered `multi_packet_*` logic into a single struct with three methods (.set, .apply, .clear), removing repeated assertions and manual clears while preserving behavior.
</issue_to_address>
### Comment 3
<location> `src/connection.rs:254` </location>
<code_context>
+ }
+
+ /// Set or replace the current multi-packet response channel and stamp correlation identifiers.
+ pub fn set_multi_packet_with_correlation(
+ &mut self,
+ channel: Option<mpsc::Receiver<F>>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for set_multi_packet_with_correlation and correlation stamping logic.
The new set_multi_packet_with_correlation method and related correlation stamping logic require both behavioural and unit tests to verify correct operation and guard against regressions. Please add targeted tests that exercise this functionality, including debug assertion coverage.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `src/correlation.rs:9` </location>
<code_context>
+//! representation.
+
+/// Access and mutate correlation identifiers on frames.
+pub trait CorrelatableFrame {
+ /// Return the correlation identifier associated with this frame, if any.
+ fn correlation_id(&self) -> Option<u64>;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for CorrelatableFrame trait and its implementations.
The new CorrelatableFrame trait and its usage should be covered by both behavioural and unit tests to ensure correct integration and trait contract adherence. Please add tests for this trait and its Envelope implementation.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `src/app/envelope.rs:104` </location>
<code_context>
fn from_parts(parts: PacketParts) -> Self { parts.into() }
}
+impl CorrelatableFrame for Envelope {
+ fn correlation_id(&self) -> Option<u64> { self.correlation_id }
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for Envelope's CorrelatableFrame implementation.
Envelope's implementation of CorrelatableFrame should be verified with both behavioural and unit tests to ensure correct handling of correlation_id. Please add these tests.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `docs/multi-packet-and-streaming-responses-design.md:278` </location>
<code_context>
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.
+ - Implementation stores the expected identifier alongside a closure built
+ from the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
</code_context>
<issue_to_address>
**issue (review_instructions):** This bullet point exceeds the 80 column wrapping limit for paragraphs and bullets.
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:279` </location>
<code_context>
invariants without requiring handlers to mutate frames post-creation and
mirrors the message attribution strategy outlined in the capability roadmap.
+ - Implementation stores the expected identifier alongside a closure built
+ from the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
</code_context>
<issue_to_address>
**issue (review_instructions):** This line in the bullet point exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this line so that it does not exceed 80 columns, as per the documentation requirements.
<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:280` </location>
<code_context>
mirrors the message attribution strategy outlined in the capability roadmap.
+ - Implementation stores the expected identifier alongside a closure built
+ from the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
</code_context>
<issue_to_address>
**issue (review_instructions):** This line in the bullet point exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this line to 80 columns to comply with the documentation formatting rules.
<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:281` </location>
<code_context>
+ - Implementation stores the expected identifier alongside a closure built
+ from the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
Debug-mode assertions must guard this stamping by checking
</code_context>
<issue_to_address>
**issue (review_instructions):** This line in the bullet point exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this line to 80 columns as required by the documentation standards.
<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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/multi-packet-and-streaming-responses-design.md(1 hunks)docs/roadmap.md(1 hunks)src/app/envelope.rs(2 hunks)src/connection.rs(9 hunks)src/correlation.rs(1 hunks)src/lib.rs(2 hunks)tests/correlation_id.rs(2 hunks)tests/features/correlation_id.feature(1 hunks)tests/steps/correlation_steps.rs(1 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/lib.rssrc/app/envelope.rssrc/correlation.rssrc/connection.rstests/world.rstests/correlation_id.rstests/steps/correlation_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/lib.rssrc/app/envelope.rssrc/correlation.rssrc/connection.rstests/world.rstests/correlation_id.rstests/steps/correlation_steps.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/lib.rssrc/app/envelope.rssrc/correlation.rssrc/connection.rstests/world.rstests/correlation_id.rstests/steps/correlation_steps.rs
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export eyre::Report from a library; convert to domain error enums at API boundaries
Files:
src/lib.rs
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 / -yse / -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
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/world.rstests/correlation_id.rstests/steps/correlation_steps.rs
🧬 Code graph analysis (5)
src/app/envelope.rs (1)
src/correlation.rs (2)
correlation_id(11-11)set_correlation_id(14-14)
src/correlation.rs (1)
src/app/envelope.rs (5)
correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)set_correlation_id(107-109)
src/connection.rs (2)
src/app/envelope.rs (6)
correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)new(83-89)new(115-121)src/correlation.rs (1)
correlation_id(11-11)
tests/world.rs (2)
src/app/envelope.rs (2)
new(83-89)new(115-121)src/connection.rs (3)
new(30-34)new(176-189)new(772-785)
tests/correlation_id.rs (4)
src/app/envelope.rs (6)
new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/connection.rs (4)
new(30-34)new(176-189)new(772-785)drop(38-41)src/push/queues/mod.rs (2)
new(92-109)builder(115-115)tests/support.rs (1)
builder(8-10)
🔍 Remote MCP Ref
Additional Context for PR #371: Correlation ID Stamping for Multi-Packet Streaming Responses
-
Design Rationale & Protocol Changes
- Introduces stamping of frames via a
CorrelatableFrametrait, allowing generic actors to set and read an optionalcorrelation_idon frames for multi-packet responses (docs/multi-packet-and-streaming-responses-design.md).
[Design doc]
- Introduces stamping of frames via a
-
New Trait & Envelope Implementation
CorrelatableFrametrait added withfn correlation_id(&self) -> Option<u64>andfn set_correlation_id(&mut self, Option<u64>).- Implemented for
Envelopeinsrc/app/envelope.rs, enablingcorrelation_idpropagation on the default packet type.
[src/app/envelope.rs]
-
ConnectionActor Enhancements
ConnectionActorextended with aMultiPacketStamperclosure and optionalcorrelation_idstate.- New API:
pub fn set_multi_packet_with_correlation<F, E>( &mut self, channel: Option<mpsc::Receiver<F>>, correlation_id: Option<u64> ) where F: CorrelatableFrame;
- Internally applies stamping to each frame drained from the multi-packet channel, with debug assertions to validate stamping invariants.
[src/connection.rs]
-
Trait & API Re-exports
- Public module
correlationand re-exportpub use correlation::CorrelatableFrameinsrc/lib.rs.
[src/lib.rs]
- Public module
-
Testing Coverage
- Unit test
multi_packet_frames_carry_request_correlation_idintests/correlation_id.rsverifies that all frames from a multi-packet channel carry the specifiedcorrelation_id. - Behavioural test scenario “Multi-packet responses reuse the request correlation id” added in
tests/features/correlation_id.featurewith a new Cucumber stepwhen a multi-packet channel emits framesimplemented intests/steps/correlation_steps.rs. - Supporting async helper
CorrelationWorld::process_multiadded intests/world.rs.
[tests/correlation_id.rs], [tests/features/correlation_id.feature], [tests/steps/correlation_steps.rs], [tests/world.rs]
- Unit test
-
Documentation & Roadmap Updates
- Design doc
docs/multi-packet-and-streaming-responses-design.mdupdated to reflect stamping mechanism. - Roadmap
docs/roadmap.mdchecklist items under Phase 6: Multi-Packet Streaming Responses flipped to implemented.
[docs/roadmap.md]
- Design doc
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
src/lib.rs (1)
14-32: Maintain this public surface alignment
ExposeCorrelatableFramefrom the root to keep the API cohesive with the new correlation module.tests/features/correlation_id.feature (1)
7-10: Exercise the multi-packet scenario
Drive this scenario to assert that the multi-packet channel reuses the originating correlation identifier across frames.tests/world.rs (1)
169-190: Drive the actor via the multi-packet path
Drive the actor through a live channel and capture stamped frames; this mirrors production behaviour in the world helper.tests/steps/correlation_steps.rs (1)
12-13: Bind the multi-packet When-step
Expose this step so the new feature scenario can activate the world helper.tests/correlation_id.rs (1)
33-60: Verify multi-packet stamping in unit coverage
Assert the stamped correlation identifier in this unit test to complement the behavioural coverage.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* drop the boxed stamper in favour of storing the correlation id and stamping frames directly\n* centralise the multi-packet teardown logic via clear_multi_packet and ensure shutdown clears correlation state\n* implement CorrelatableFrame for u8 and Vec<u8> so ConnectionActor can require the trait
- cover multi-packet stamping with rstest cases including absent correlations - extend cucumber scenarios and world state to support missing correlation ids - add CorrelatableFrame unit tests for Envelope and no-op types
Track whether multi-packet correlation stamping is active by storing an outer option, only applying stamps when configured, and documenting the Clippy allowances required by the review guidance.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Apply the configured correlation id to hook-provided stream end frames before they are emitted so multi-packet pipelines keep the request context on terminators. Extend the async tests to cover stamped and cleared terminators by wiring a custom hook through the helper.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/connection.rs(9 hunks)src/correlation.rs(1 hunks)tests/correlation_id.rs(2 hunks)tests/features/correlation_id.feature(1 hunks)tests/steps/correlation_steps.rs(1 hunks)tests/world.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/steps/correlation_steps.rstests/world.rssrc/correlation.rstests/correlation_id.rssrc/connection.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
tests/steps/correlation_steps.rstests/world.rssrc/correlation.rstests/correlation_id.rssrc/connection.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/steps/correlation_steps.rstests/world.rssrc/correlation.rstests/correlation_id.rssrc/connection.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/steps/correlation_steps.rstests/world.rstests/correlation_id.rs
🧬 Code graph analysis (5)
tests/steps/correlation_steps.rs (1)
tests/world.rs (1)
expected(151-151)
tests/world.rs (3)
src/connection.rs (4)
new(30-34)new(174-187)new(783-796)drop(38-41)src/app/envelope.rs (6)
new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/push/queues/mod.rs (1)
new(92-109)
src/correlation.rs (1)
src/app/envelope.rs (7)
correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)set_correlation_id(107-109)new(83-89)new(115-121)
tests/correlation_id.rs (4)
src/app/envelope.rs (7)
payload(130-130)new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/connection.rs (5)
new(30-34)new(174-187)new(783-796)with_hooks(191-221)default(92-97)src/push/queues/mod.rs (2)
new(92-109)builder(115-115)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
src/connection.rs (5)
src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)src/app/envelope.rs (4)
correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)tests/lifecycle.rs (1)
correlation_id(122-122)tests/routes.rs (1)
correlation_id(42-42)tests/world.rs (1)
expected(151-151)
🔍 Remote MCP
Relevant Additional Context for PR #371: Implementing Correlation ID Stamping for Multi‐Packet Responses
-
Design Rationale & Invariants
- The new stamping logic follows the design in
docs/multi-packet-and-streaming-responses-design.md, which mandates:- Storing the expected correlation ID alongside a closure (
MultiPacketStamper) obtained from theCorrelatableFrametrait. - Debug builds assert that each stamped frame’s
correlation_id()matches the expected value, enforcing the invariant before dispatch.
Citation: design spec [raw docs/multi-packet-and-streaming-responses-design.md] (URL unavailable due to access limits).
- Storing the expected correlation ID alongside a closure (
- The new stamping logic follows the design in
-
Roadmap Phase 6 Overview
- Phase 6 (“Multi-Packet Streaming Responses”) was updated in
docs/roadmap.mdto mark correlation ID handling as implemented:- Sent frames carry the correct ID from the initial request.
- Early receiver closures and error paths now covered by tests.
Citation: Roadmap [::Deepwiki_read_wiki_contents::] (“Phase 6: Multi-Packet…” entries).
- Phase 6 (“Multi-Packet Streaming Responses”) was updated in
-
FrameLike vs CorrelatableFrame
FrameLikeis a marker trait (Send + 'static) for frames ([::Deepwiki_ask_question::]).CorrelatableFrameextendsFrameLiketo expose/get and set anOption<u64>correlation ID (methodscorrelation_id(),set_correlation_id(…)).
Citation: FrameLike & CorrelatableFrame definitions [::Deepwiki_ask_question::].
-
ConnectionActor Changes
- The
ConnectionActor<F,E>now requiresF: FrameLike + CorrelatableFrameenabling stamping in the multi-packet path. - New methods manage stamping lifecycle:
install_multi_packetstores the stamper closure and expected ID.apply_multi_packet_correlation(&mut self, frame: &mut F)stamps each outgoing frame.
Citation: ConnectionActor diff summary in PR context.
- The
-
Envelope Implementation
Envelopenow implementsCorrelatableFrame:impl CorrelatableFrame for Envelope { fn correlation_id(&self) -> Option<u64> { self.correlation_id } fn set_correlation_id(&mut self, cid: Option<u64>) { self.correlation_id = cid; } }
- Enables stamping on the default envelope type without altering other protocols.
Citation: Envelope impl diff insrc/app/envelope.rs.
-
Tests Covering Behavior
- Unit tests in
src/correlation.rsverify round-trip stamping and noop behavior onu8/Vec<u8>. - Integration tests in
tests/correlation_id.rsand Cucumber scenarios intests/features/correlation_id.featureconfirm:- Multi-packet frames reuse the request’s correlation ID.
- Cases with no initial ID produce
Noneon subsequent frames.
Citation: Test summaries in PR context.
- Unit tests in
⏰ 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). (1)
- GitHub Check: Sourcery review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/connection.rs(14 hunks)tests/correlation_id.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/correlation_id.rssrc/connection.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
tests/correlation_id.rssrc/connection.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/correlation_id.rssrc/connection.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/correlation_id.rs
🧬 Code graph analysis (2)
tests/correlation_id.rs (4)
src/connection.rs (8)
correlation(186-186)new(30-34)new(152-157)new(222-235)new(810-823)drop(38-41)with_hooks(239-268)default(92-97)src/app/envelope.rs (7)
payload(130-130)new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/push/queues/mod.rs (2)
new(92-109)builder(115-115)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
src/connection.rs (2)
src/app/envelope.rs (6)
new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
🔍 Remote MCP
Summary of additional facts relevant to reviewing PR #371
- New public trait CorrelatableFrame (correlation_id / set_correlation_id) added and re-exported from crate root; trivial no-op impls exist for u8 and Vec. [::Deepwiki_read_wiki_contents::]
- Envelope now implements CorrelatableFrame so Envelope frames can carry/set Option correlation IDs. [::Deepwiki_read_wiki_contents::]
- ConnectionActor generic bound tightened to F: FrameLike + CorrelatableFrame; actor stores multi_packet_correlation: Option<Option> and exposes set_multi_packet_with_correlation(...) to install a multi-packet channel plus optional correlation id. Private helpers install_multi_packet, clear_multi_packet, apply_multi_packet_correlation added; debug_asserts enforce stamping invariants (only in debug builds). [::Deepwiki_read_wiki_contents::]
- apply_multi_packet_correlation is applied along the multi-packet streaming path (queue processing and end-of-stream handling); correlation state is cleared on channel close/shutdown. [::Deepwiki_read_wiki_contents::]
- Tests added/updated:
- Unit tests for CorrelatableFrame behavior (Envelope round-trip; noop for primitives).
- Integration/unit tests exercising multi-packet correlation propagation and terminator behavior (tests/correlation_id.rs).
- BDD scenarios and step defs updated to use optional expected correlation (tests/features/correlation_id.feature, tests/steps/correlation_steps.rs, tests/world.rs). Test harness adjusted (ActorHarness.has_multi_queue uses is_active()). [::Deepwiki_read_wiki_contents::]
- Docs updated: design doc describes storing an expected correlation id + stamper closure and debug assertions; roadmap marks Phase 6 correlation handling items as implemented. [::Deepwiki_read_wiki_contents::]
- Runtime/testing notes: multi-packet flows in tests use tokio::sync::mpsc channels and rstest fixtures; developer/test commands remain make fmt / make lint / make test. [::Deepwiki_read_wiki_contents::]
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (22)
src/connection.rs (18)
49-49: Correlation import correctly added to support trait-bounded frames.The import enables the trait constraint on ConnectionActor's generic type parameter
F, providing correlation stamping capabilities.
124-124: Multi-packet state correctly refactored into context struct.The previous scattered state fields have been appropriately consolidated into a single
MultiPacketContext, reducing option juggling and centralising multi-packet logic.
140-189: Implement bounded correlation state tracking.The
MultiPacketContextcorrectly tracks stamping activation separately from the stored identifier using nestedOption<Option<u64>>. The debug assertion ensures state consistency when channels are active.
201-201: Generic constraint correctly tightened for correlatable frames.The trait bound
F: FrameLike + CorrelatableFrameenables correlation stamping whilst maintaining frame serialisation capabilities.
252-252: Context initialisation correctly integrated.The field initialisation uses the new context constructor, maintaining consistency with the refactored structure.
276-276: Invariant assertion correctly updated for new context.The assertion uses the context's
is_active()method to check multi-packet state consistency.
294-296: Correlation state correctly tracked for basic multi-packet setup.When no correlation stamping is required, the code correctly maps channel presence to
Some(None), indicating stamping is active but with no identifier.
298-313: Correlation API correctly implements optional stamping.The
set_multi_packet_with_correlationmethod correctly maps the channel to correlation state, usingSome(correlation_id)to indicate active stamping with the provided identifier.
315-315: Context clearing correctly delegated.The method delegates to the context's
clear()method, maintaining encapsulation.
317-345: Correlation stamping logic correctly handles all stamping states.The method appropriately handles three correlation states: explicit stamping with an identifier, explicit clearing with
None, and misconfigured state with proper debug assertions.
376-380: Invariant assertions correctly updated for context usage.The debug assertions now use the context's
is_active()method and the state constructor correctly tracks multi-packet activity.
407-407: Multi-packet availability check correctly uses context.The availability guard correctly queries the context's
is_active()method whilst respecting shutdown state.
416-416: Channel polling correctly delegates to context.The select branch correctly uses
channel_mut()to access the receiver through the context.
472-475: Correlation stamping correctly applied to multi-packet frames.The logic correctly mutates frames when correlation is configured and applies stamping only to multi-packet sources.
510-512: Channel extraction correctly delegated to context.The method appropriately uses the context's
take_channel()to extract the receiver whilst maintaining state consistency.
531-538: Terminator frame correlation correctly applied.The terminator frame from hooks receives correlation stamping when configured, ensuring end-of-stream frames carry the expected correlation identifier. This addresses the requirement that all frames in multi-packet responses maintain consistent correlation.
Based on previous review comments.
588-592: Shutdown correctly handles multi-packet context.The shutdown logic appropriately extracts and closes the channel, marks the state as closed, and clears the context to prevent resource leaks.
661-681: Opportunistic draining correctly applies correlation stamping.The multi-packet draining logic correctly applies correlation to frames when configured, maintaining consistency with the primary event processing path.
tests/correlation_id.rs (4)
4-14: Test dependencies correctly imported for multi-packet correlation.The imports provide necessary testing infrastructure including parameterised testing via rstest, channels for multi-packet scenarios, and correlation trait support.
36-66: Multi-packet test harness correctly implemented.The
run_multi_packet_channelhelper appropriately constructs a test environment with configurable correlation settings and protocol hooks, enabling comprehensive multi-packet scenario testing.
68-84: Parameterised correlation tests comprehensively cover stamping scenarios.The rstest cases appropriately verify stamping behaviour across three critical scenarios: applying request correlation to frames lacking identifiers, clearing correlation when the request has none, and preserving matching identifiers. This ensures robust correlation handling.
rstest generates independent test cases and the chosen scenarios align with the coding guidelines requiring comprehensive testing of new functionality.
86-105: Terminator correlation tests verify end-of-stream stamping.The tests correctly verify that terminator frames from protocol hooks receive appropriate correlation stamping, ensuring complete consistency across multi-packet response flows including end-of-stream signalling.
This resolves the issue flagged in previous reviews regarding terminator frame correlation handling.
Use a widening cast before serialising the marker bytes so the multi-packet correlation tests cannot panic on large indices.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
|
@sourcery-ai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider replacing the nested
Option<Option<u64>>correlation field with a small enum (e.g. Disabled / Enabled(None) / Enabled(Some)) to make the stamping configuration more self‐documenting and avoid the double‐Option pattern. - The correlation‐stamping logic is repeated around multiple frame emission points; you could centralize calls to
apply_multi_packet_correlationin a single emit path to reduce duplication and maintenance overhead. - Relying on
debug_assert(false)for the undefined‐state branch means it will be optimized away in non-debug builds—consider usingunreachable!()or an explicit panic to enforce invariants in all builds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the nested `Option<Option<u64>>` correlation field with a small enum (e.g. Disabled / Enabled(None) / Enabled(Some)) to make the stamping configuration more self‐documenting and avoid the double‐Option pattern.
- The correlation‐stamping logic is repeated around multiple frame emission points; you could centralize calls to `apply_multi_packet_correlation` in a single emit path to reduce duplication and maintenance overhead.
- Relying on `debug_assert(false)` for the undefined‐state branch means it will be optimized away in non-debug builds—consider using `unreachable!()` or an explicit panic to enforce invariants in all builds.
## Individual Comments
### Comment 1
<location> `src/connection/test_support.rs:97-99` </location>
<code_context>
/// Returns `true` when the multi-packet queue is still available.
#[must_use]
- pub fn has_multi_queue(&self) -> bool { self.actor.multi_packet.is_some() }
+ pub fn has_multi_queue(&self) -> bool { self.actor.multi_packet.is_active() }
/// Process a multi-packet poll result.
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for the test harness changes.
Please add or update tests to confirm that the harness accurately tracks the queue's state, including after clearing or installing the queue.
```suggestion
/// Returns `true` when the multi-packet queue is still available.
#[must_use]
pub fn has_multi_queue(&self) -> bool { self.actor.multi_packet.is_active() }
#[cfg(test)]
mod tests {
use super::*;
struct DummyMultiPacket {
active: bool,
}
impl DummyMultiPacket {
fn new() -> Self {
Self { active: false }
}
fn install(&mut self) {
self.active = true;
}
fn clear(&mut self) {
self.active = false;
}
fn is_active(&self) -> bool {
self.active
}
}
struct DummyActor {
multi_packet: DummyMultiPacket,
}
struct TestHarness {
actor: DummyActor,
}
impl TestHarness {
fn new() -> Self {
Self {
actor: DummyActor {
multi_packet: DummyMultiPacket::new(),
},
}
}
pub fn has_multi_queue(&self) -> bool {
self.actor.multi_packet.is_active()
}
pub fn install_multi_queue(&mut self) {
self.actor.multi_packet.install();
}
pub fn clear_multi_queue(&mut self) {
self.actor.multi_packet.clear();
}
}
#[test]
fn test_has_multi_queue_initially_false() {
let harness = TestHarness::new();
assert!(!harness.has_multi_queue(), "Queue should not be active initially");
}
#[test]
fn test_has_multi_queue_after_install() {
let mut harness = TestHarness::new();
harness.install_multi_queue();
assert!(harness.has_multi_queue(), "Queue should be active after install");
}
#[test]
fn test_has_multi_queue_after_clear() {
let mut harness = TestHarness::new();
harness.install_multi_queue();
harness.clear_multi_queue();
assert!(!harness.has_multi_queue(), "Queue should not be active after clear");
}
}
```
</issue_to_address>
### Comment 2
<location> `docs/multi-packet-and-streaming-responses-design.md:279` </location>
<code_context>
invariants without requiring handlers to mutate frames post-creation and
mirrors the message attribution strategy outlined in the capability roadmap.
+- Implementation stores the expected identifier alongside a closure built from
+ the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph line exceeds 80 columns and should be wrapped for readability.
Please wrap this paragraph 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/multi-packet-and-streaming-responses-design.md:280` </location>
<code_context>
mirrors the message attribution strategy outlined in the capability roadmap.
+- Implementation stores the expected identifier alongside a closure built from
+ the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph line exceeds 80 columns and should be wrapped for readability.
Please wrap this paragraph 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/multi-packet-and-streaming-responses-design.md:281` </location>
<code_context>
+- Implementation stores the expected identifier alongside a closure built from
+ the new `CorrelatableFrame` trait, ensuring frames can be stamped in a
+ generic actor without constraining other protocols. Debug builds assert the
+ stamped frame exposes the expected identifier so regressions fail fast.
Debug-mode assertions must guard this stamping by checking
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph line exceeds 80 columns and should be wrapped for readability.
Please wrap this paragraph 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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/connection.rs(15 hunks)src/connection/test_support.rs(2 hunks)tests/correlation_id.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/correlation_id.rssrc/connection/test_support.rssrc/connection.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
tests/correlation_id.rssrc/connection/test_support.rssrc/connection.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/correlation_id.rssrc/connection/test_support.rssrc/connection.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/correlation_id.rs
🧬 Code graph analysis (3)
tests/correlation_id.rs (3)
src/app/envelope.rs (7)
payload(130-130)new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/connection.rs (6)
new(30-34)new(158-163)new(220-233)new(812-825)with_hooks(237-266)default(92-97)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
src/connection/test_support.rs (1)
src/connection.rs (4)
new(30-34)new(158-163)new(220-233)new(812-825)
src/connection.rs (2)
src/app/envelope.rs (6)
new(83-89)new(115-121)correlation_id(52-52)correlation_id(97-97)correlation_id(105-105)correlation_id(127-127)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
🔍 Remote MCP Deepwiki
Summary of additional context relevant to reviewing PR #371
- New public trait CorrelatableFrame added and re-exported from crate root; provides correlation_id() -> Option and set_correlation_id(Option) and has noop impls for u8/Vec.
- Envelope now implements CorrelatableFrame so Envelope-based frames carry/set Option correlation ids.
- ConnectionActor generic bound tightened to F: FrameLike + CorrelatableFrame; new API set_multi_packet_with_correlation(channel, correlation_id: Option) installs multi-packet channel plus optional correlation id. apply_multi_packet_correlation logic stamps emitted multi-packet frames and terminators with the stored correlation id. Debug-only assertions enforce invariants (fail-fast in debug builds).
- MultiPacketContext replaces prior optional receiver field; exposes is_active() and channel_mut/take_channel helpers; multi-packet state is cleared on channel close/shutdown.
- Tests updated/added:
- Unit tests for CorrelatableFrame behavior (Envelope round-trip; noop impls).
- Connection-level tests exercising multi-packet correlation propagation and terminator stamping (tests/correlation_id.rs).
- BDD feature scenarios and step defs updated to use optional expected correlation id (tests/features/, tests/steps/, tests/world.rs). Tests use tokio::sync::mpsc and rstest fixtures.
- Docs updated: design doc documents storing expected correlation id + stamper closure and debug assertions; roadmap marks Phase 6 multi-packet correlation items as implemented.
- Runtime/test notes / CI expectations unchanged: make fmt, make lint, make test remain the developer commands; tests rely on wireframe testing helpers and bincode/LengthPrefixed framing.
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
src/connection/test_support.rs (2)
99-99: Use ofis_active()aligns harness with new multi‑packet context.Keep this; it correctly reflects the new API and avoids Option poking.
184-225: Good: added harness tests for multi‑packet availability.These tests exercise install/clear transitions and match prior review asks.
tests/correlation_id.rs (3)
36-66: Solid channel driver for multi‑packet correlation tests.The helper wires stamping through
set_multi_packet_with_correlationand avoids payload overflows by using LE bytes.
68-85: Parametrized coverage of stamp/clear/preserve paths is complete.Retain these cases; they verify overwrite, clear‑on‑None, and preserve semantics succinctly.
86-105: Terminators are correctly stamped/cleared.This asserts the roadmap requirement that end‑of‑stream frames carry the originating correlation ID when configured.
src/connection.rs (5)
140-188: Lean stamping state machine is appropriate.
MultiPacketStampandMultiPacketContextcleanly separate channel presence from stamping behaviour; keep this design.
283-299: Default multi‑packet enablement clears correlation IDs.Enabling with
Enabled(None)is a sensible default; it guarantees deterministic output for multi‑packet frames without leaking stale IDs.
321-347: Stamp frames via a single path.
emit_multi_packet_frame+apply_multi_packet_correlationensures all multi‑packet frames pass through hooks/metrics with enforced invariants. Keep this.
535-549: Stamp the terminator before emission.Routing the hook‑provided end frame through
emit_multi_packet_framefixes the earlier gap where terminators could lose correlation.
664-683: Handle disconnected multi‑packet receiver robustly.Extract the receiver, emit a stamped terminator, and clear context; this prevents stale state and double‑close.
1 participant
Summary
CorrelatableFrametrait and implement it for the built-inEnvelopeTesting
https://chatgpt.com/codex/tasks/task_e_68d6d2f14f388322a3d51b04a3b6347d
Summary by Sourcery
Implement and enforce correlation ID stamping on multi-packet responses by introducing a CorrelatableFrame trait, refactoring multi-packet state into a structured context, and augmenting ConnectionActor to apply and assert the correct identifiers.
New Features:
Enhancements:
Documentation:
Tests: