Add Fragmenter and transport-level fragmentation support#395
Conversation
Introduced a new Fragmenter utility that serializes and splits oversized messages into multiple sequential fragments. Each fragment is tagged with a FragmentHeader containing MessageId, FragmentIndex, and an is_last_fragment flag to enable transparent transport-level reassembly. The API supports generating fresh message IDs automatically or accepting user-provided IDs. FragmentBatch and FragmentFrame types are provided to access and iterate fragments conveniently. This change includes comprehensive unit and behavioral tests, updates to documentation and design specs, and exposes the new types through the crate root. It facilitates transparent outbound message fragmentation, establishing the groundwork before integrating the full FragmentAdapter layer. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 29 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 (2)
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
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded an outbound Fragmenter utility that serialises and splits messages/byte payloads into transport fragments with sequential FragmentHeader metadata and monotonic MessageId generation. Introduced FragmentationError (Encode, IndexOverflow), public types Fragmenter, FragmentBatch, FragmentFrame, plus docs, roadmap and tests covering the new behaviour. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(243,247,255)
participant Caller
participant Fragmenter
participant Batch
participant Frame
end
Caller->>Fragmenter: Fragmenter::new(max_size)
Caller->>Fragmenter: fragment_bytes(payload)
activate Fragmenter
Fragmenter->>Fragmenter: allocate MessageId
Fragmenter->>Fragmenter: compute fragment count (div_ceil)
Fragmenter->>Frame: create Frame(header(index,...), chunk) x N
Fragmenter->>Batch: assemble Batch(message_id, frames)
deactivate Fragmenter
Batch-->>Caller: return FragmentBatch
Caller->>Batch: fragments() / into_fragments()
Batch-->>Caller: &[FragmentFrame] / Vec<FragmentFrame>
Caller->>Frame: header() / payload()
Frame-->>Caller: metadata & bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideImplements a new outbound Fragmenter helper that slices messages into capped transport fragments, introduces FragmentFrame/FragmentBatch containers and FragmentationError, wires them into the public fragment API, and extends tests and docs to cover transport-level fragmentation behaviour. Sequence diagram for fragmenting a message with FragmentersequenceDiagram
actor "Caller" as Caller
participant "Fragmenter" as Fragmenter
participant "Message" as Message
participant "FragmentBatch" as FragmentBatch
participant "FragmentFrame" as FragmentFrame
participant "FragmentIndex" as FragmentIndex
"Caller" ->> "Fragmenter" : "fragment_message(&message)"
activate "Fragmenter"
"Fragmenter" ->> "Message" : "to_bytes()"
activate "Message"
"Message" -->> "Fragmenter" : "Result<Vec<u8>, EncodeError>"
deactivate "Message"
alt "encode fails"
"Fragmenter" -->> "Caller" : "Err(FragmentationError::Encode(EncodeError))"
else "encode succeeds"
"Fragmenter" ->> "Fragmenter" : "fragment_bytes(payload)"
"Fragmenter" ->> "Fragmenter" : "next_message_id() -> MessageId"
"Fragmenter" ->> "Fragmenter" : "fragment_with_id(message_id, payload)"
"Fragmenter" ->> "Fragmenter" : "build_fragments(message_id, payload)"
loop "for each fragment slice"
"Fragmenter" ->> "Fragmenter" : "construct FragmentHeader(message_id, index, is_last)"
"Fragmenter" ->> "FragmentFrame" : "new(header, payload_chunk.to_vec())"
activate "FragmentFrame"
"FragmentFrame" -->> "Fragmenter" : "FragmentFrame"
deactivate "FragmentFrame"
alt "need more fragments"
"Fragmenter" ->> "FragmentIndex" : "checked_increment(index)"
"FragmentIndex" -->> "Fragmenter" : "Option<FragmentIndex>"
else "last fragment"
"Fragmenter" ->> "Fragmenter" : "stop loop"
end
end
alt "index overflow"
"Fragmenter" -->> "Caller" : "Err(FragmentationError::IndexOverflow { last })"
else "no overflow"
"Fragmenter" ->> "FragmentBatch" : "new(message_id, fragments)"
activate "FragmentBatch"
"FragmentBatch" -->> "Fragmenter" : "FragmentBatch"
deactivate "FragmentBatch"
"Fragmenter" -->> "Caller" : "Ok(FragmentBatch)"
end
end
deactivate "Fragmenter"
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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +200 to +210 pub fn assert_fragment_payload_len(&self, index: usize, expected: usize) {
let fragments = self.batch().fragments();
let fragment = fragments
.get(index)
.unwrap_or_else(|| panic!("fragment {index} missing"));
assert_eq!(
fragment.payload().len(),
expected,
"payload length mismatch"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…ication Refactor FragmentWorld test code by introducing a get_fragment_at helper method. This method retrieves a fragment at a given index or panics if missing, replacing repeated code within assert_fragment_payload_len and assert_fragment_final_flag. This improves code clarity and reduces duplication in test assertions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Added FragmentFrame to imports in tests/worlds/fragment.rs to support upcoming tests or code changes requiring this struct. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/worlds/fragment.rs (1)
1-5: Update module‑level docs to mention outbound fragmentationExtend the module‑level
//!comment to mention thatFragmentWorldnow also drives outbound fragmentation scenarios (via the fragmenter and fragment batch assertions), not only re‑assembly. Keep the current description of ordering, completion detection, and error handling, but add a short sentence about exercising fragment production as well.Also applies to: 37-59, 197-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)docs/asynchronous-outbound-messaging-design.md(1 hunks)docs/generic-message-fragmentation-and-re-assembly-design.md(4 hunks)docs/multi-packet-and-streaming-responses-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/users-guide.md(1 hunks)src/fragment/error.rs(2 hunks)src/fragment/fragmenter.rs(1 hunks)src/fragment/mod.rs(1 hunks)src/fragment/tests.rs(2 hunks)src/lib.rs(1 hunks)tests/features/fragment.feature(1 hunks)tests/steps/fragment_steps.rs(1 hunks)tests/worlds/fragment.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
CHANGELOG.mddocs/roadmap.mddocs/users-guide.mddocs/multi-packet-and-streaming-responses-design.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/asynchronous-outbound-messaging-design.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:
CHANGELOG.mddocs/roadmap.mddocs/users-guide.mddocs/multi-packet-and-streaming-responses-design.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/asynchronous-outbound-messaging-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/message-versioning.md)
Document examples showing how to deprecate old message versions gracefully
When long lines are warranted in documentation, disable markdownlint rule MD013 inline using the comment:
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation accurate
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)Follow markdownlint recommendations
Files:
docs/roadmap.mddocs/users-guide.mddocs/multi-packet-and-streaming-responses-design.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/asynchronous-outbound-messaging-design.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/roadmap.mddocs/users-guide.mddocs/multi-packet-and-streaming-responses-design.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/asynchronous-outbound-messaging-design.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,rs}: Use British English (en-GB-oxendict) spelling in documentation prose
Keep US spelling when it appears in API identifiers (e.g.,color)
The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are…”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure Markdown tables include a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns
Do not wrap tables
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding images, use Markdownwith brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/roadmap.mddocs/users-guide.mddocs/multi-packet-and-streaming-responses-design.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/asynchronous-outbound-messaging-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely 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; 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 with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
src/fragment/tests.rssrc/fragment/mod.rssrc/fragment/error.rstests/steps/fragment_steps.rstests/worlds/fragment.rssrc/fragment/fragmenter.rssrc/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/fragment/tests.rssrc/fragment/mod.rssrc/fragment/error.rstests/steps/fragment_steps.rstests/worlds/fragment.rssrc/fragment/fragmenter.rssrc/lib.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/steps/fragment_steps.rstests/worlds/fragment.rs
🧬 Code graph analysis (5)
src/fragment/tests.rs (4)
src/fragment/fragmenter.rs (10)
new(26-28)new(138-138)new(161-167)payload(146-146)len(179-179)is_fragmented(187-187)message_id(171-171)fragments(175-175)header(142-142)with_starting_id(32-37)src/fragment/id.rs (1)
new(25-25)src/fragment/header.rs (4)
new(35-45)message_id(49-49)is_last_fragment(57-57)fragment_index(53-53)src/fragment/index.rs (2)
new(30-30)zero(34-34)
src/fragment/mod.rs (1)
src/fragment/fragmenter.rs (1)
header(142-142)
tests/steps/fragment_steps.rs (3)
src/fragment/fragmenter.rs (2)
len(179-179)message_id(171-171)src/fragment/header.rs (1)
message_id(49-49)src/fragment/series.rs (1)
message_id(33-33)
tests/worlds/fragment.rs (3)
src/fragment/fragmenter.rs (7)
new(26-28)new(138-138)new(161-167)len(179-179)payload(146-146)header(142-142)message_id(171-171)src/fragment/id.rs (1)
new(25-25)src/fragment/header.rs (3)
new(35-45)is_last_fragment(57-57)message_id(49-49)
src/fragment/fragmenter.rs (1)
src/fragment/index.rs (1)
zero(34-34)
🪛 LanguageTool
docs/generic-message-fragmentation-and-re-assembly-design.md
[style] ~132-~132: Would you like to use the Oxford spelling “serializes”? The spelling ‘serialises’ is also correct.
Context: ...ree entry points: - fragment_message serialises any type implementing the Message tra...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~143-~143: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...` overflow and returns an explicit error so protocols can downgrade to streaming or...
(COMMA_COMPOUND_SENTENCE_2)
🔍 Remote MCP Deepwiki
Additional Context for PR #395 Review
I've gathered relevant design and architectural context to supplement the PR review. Here's the key information:
Fragmentation Architecture Context
Design Goals and Requirements
The core requirements for message fragmentation include transparent inbound reassembly (router/handlers receive complete logical frames), transparent outbound fragmentation (payloads exceeding protocol-specific sizes are automatically fragmented), pluggable strategy for protocol-agnostic implementation, DoS protection with resource and time limits, and zero friction for non-fragmented messages
Core Architecture
The FragmentAdapter is implemented as a FrameProcessor middleware between raw I/O and the main router, handling splitting of large outbound frames and reassembling inbound fragments. State management for multiplexing uses a dashmap::DashMap for concurrent reassembly of multiple logical messages on the same connection, keyed by msg_id, providing lock-free reads and sharded writes
How Fragmenter Relates to the System
The Fragmenter acts as a FrameProcessor middleware sitting between raw I/O (transport layer) and message handling logic (router/handlers), transparently splitting large outbound logical messages into smaller physical frames and reassembling inbound fragments into complete logical messages. For outbound messages, when a handler returns a Response::Stream or asynchronous push is made, the FragmentAdapter automatically fragments large logical frames into smaller physical frames before writing to the socket, with application code remaining unaware
Related PR #393 Context
PR #393 introduced the FragmentMeta struct carrying decoded fragment header metadata (payload_len, total_message_len, is_final, msg_id, seq) and the FragmentStrategy trait defining protocol-specific rules for fragmentation, including max_fragment_payload, decode_header, and encode_header methods
FragmentStrategy Integration
The FragmentStrategy trait provides methods for determining maximum payload sizes, parsing fragment headers, and emitting fragment headers, while the FragmentAdapter is instantiated with a protocol-specific FragmentStrategy and uses decode_header for inbound reassembly and encode_header for outbound fragmentation
Key Review Points
-
Scope Alignment: The PR introduces utility types (
Fragmenter,FragmentBatch,FragmentFrame) for explicit message fragmentation, complementing the middlewareFragmentAdapterthat was introduced in PR #393 for transparent fragmentation/reassembly at theFrameProcessorlayer. -
Code Duplication Issue: As noted in the PR comments,
tests/worlds/fragment.rscontains duplicated fragment retrieval logic inassert_fragment_payload_lenandassert_fragment_final_flagthat should be extracted into a sharedget_fragment_athelper. -
Backwards Compatibility: The Fragmenter API is opt-in and does not modify existing fragmentation logic, keeping existing code paths unaffected.
⏰ 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: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (17)
docs/roadmap.md (1)
261-262: Fragmenter completion marker is accurate and correctly positioned.The checkbox update to
[x]appropriately reflects the completion of the Fragmenter implementation introduced in PR #395, including the public API (Fragmenter, FragmentBatch, FragmentFrame), unit tests, and documentation. The roadmap structure remains logically ordered, with Reassembler and full F&R integration tasks correctly left unchecked as future work.tests/features/fragment.feature (1)
37-47: LGTM: Comprehensive Fragmenter scenario added.The new scenario thoroughly exercises the Fragmenter API, validating fragment counts, payload sizes, final/non-final flags, and message ID assignment. The test steps are specific and align with the PR objectives.
CHANGELOG.md (1)
12-14: LGTM: Changelog entry is clear and accurate.The entry accurately describes the new Fragmenter helper and its purpose for transport-level reassembly.
src/fragment/tests.rs (5)
1-3: LGTM: Imports added for new test functionality.The imports for
NonZeroUsizeand bincode traits are necessary for the new Fragmenter tests.
84-103: LGTM: Comprehensive test for multi-fragment splitting.The test validates fragment counts, payload correctness, and final/non-final flags across all fragments.
105-116: LGTM: Empty payload edge case covered.The test correctly validates that an empty payload produces a single fragment marked as final.
118-140: LGTM: Message ID increment and dual API tested.The test validates that the Fragmenter correctly increments message IDs across calls and exercises both
fragment_messageandfragment_bytesAPIs.
142-155: LGTM: IntoIterator implementation tested.The test validates that
FragmentBatchcorrectly implementsIntoIteratorand yields all fragments in order.src/lib.rs (1)
34-45: LGTM: Public API expanded with new fragmentation types.The re-exports for
FragmentBatch,FragmentFrame,FragmentationError, andFragmentercorrectly expose the new fragmentation utilities at the crate root, consistent with the PR objectives.src/fragment/mod.rs (2)
9-9: LGTM: Fragmenter module added.The new
fragmentermodule is correctly declared and integrated into the fragment module structure.
15-20: LGTM: Public re-exports extended for new fragmentation types.The re-exports for
FragmentationError,FragmentBatch,FragmentFrame,Fragmenter, andFragmentSeriescorrectly expose the new fragmentation utilities at the fragment module level.docs/asynchronous-outbound-messaging-design.md (1)
41-49: Suppression is justified; no changes required.The table uses aligned style (column separators padded to match content width), which is a specific formatting choice that may conflict with other table styles in the documentation. The MD060 suppression is correctly applied and warrants retention.
Run
make markdownlintlocally to confirm no other MD060 violations exist in the file.docs/multi-packet-and-streaming-responses-design.md (1)
88-96: Clarify which rule violation requires suppression.The table structure appears consistently formatted. However, the coding guidelines specify disabling
MD013for long lines in documentation, yet this suppression targetsMD060(table column style). The second column contains text exceeding 120 characters—triggering MD013, not necessarily MD060.Verify with
make markdownlintwhether both rules are violated or only one. If line length is the concern, align the suppression with the documented guidance:<!-- markdownlint-disable-next-line MD013 -->rather than MD060. If both rules require suppression, document why.
src/fragment/error.rs (1)
7-8: Define dedicated outboundFragmentationErrorKeep this separation between inbound
FragmentErrorand outboundFragmentationError. The two explicit variants (EncodeandIndexOverflow) are narrowly scoped, error messages match the inbound overflow wording, and theFrommapping keeps call‑sites clean. No changes required.Also applies to: 44-52
tests/steps/fragment_steps.rs (1)
44-75: Wire fragmenter steps cleanly throughFragmentWorldKeep these steps as thin delegates to
FragmentWorld. Parameter types match the underlying helpers, the final/non‑final steps correctly re‑use the same assertion with a boolean flag, and the message‑id check uses the explicit world assertion. No changes required.tests/worlds/fragment.rs (1)
7-8: Centralise fragment assertions viaget_fragment_atKeep
configure_fragmenter,fragment_payload,batch, andget_fragment_atstructured as they are. The world now initialises a fragmenter with a non‑zero cap, records the last batch, and uses a single lookup helper for both payload‑length and final‑flag assertions. This removes the duplication highlighted in the PR notes while preserving clear panic messages in test‑only code.Also applies to: 22-28, 37-59, 98-109, 197-241
src/fragment/fragmenter.rs (1)
16-89: Keep Fragmenter/FragmentFrame/FragmentBatch API and invariants as-isRetain the current API surface:
- Use
NonZeroUsizeformax_fragment_sizeand panic‑free handling of empty payloads via a single zero‑length fragment.- Generate message identifiers atomically and expose
next_message_idfor callers that need to inspect IDs.- Enforce
FragmentIndexoverflow viachecked_increment, mapping directly toFragmentationError::IndexOverflow.- Keep
FragmentFrameandFragmentBatchas lightweight wrappers with#[must_use]accessors andIntoIteratorfor ergonomic consumption.The design matches the documented behaviour, keeps invariants explicit, and introduces no unnecessary allocations or panics in library code.
Also applies to: 128-199
Refactor fragment_bytes method to accept any type implementing AsRef<[u8]> instead of a byte slice. This enables greater flexibility in the payload argument without changing internal logic. Additional enhancements: - Refined next_message_id to safely handle overflow with fetch_update and documented its panic behavior. - Updated documentation in users-guide.md to warn users about error handling when fragmenting. - Removed is_empty method from FragmentBatch as batches are guaranteed non-empty. - Added tests to validate explicit message IDs and changed tests and examples to leverage payload argument flexibility. These changes improve API robustness, clarity, and usability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7007 @leynos: Yes, both issues have been resolved: Issue 1: /// # Panics
///
/// Panics if the identifier counter reaches `u64::MAX` and overflows. Callers
/// should provisionally treat `MessageId` values as unique for the lifetime of
/// the fragmenter.
Issue 2: #[allow(
clippy::len_without_is_empty,
reason = "batches are guaranteed non-empty"
)]The non-empty invariant is enforced via |
Replaced inline vector in fragment_bytes call with a named variable `payload` to improve code clarity in the users guide example. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/fragment/tests.rs (1)
118-170: Retain ID-progression, iterator, and explicit-ID coverageKeep
DummyMessageand the testsfragmenter_fragments_messages_and_increments_ids,fragment_batch_into_iterator_yields_all_fragments, andfragmenter_respects_explicit_message_ids. Together they validate:
- Shared ID progression across
fragment_messageandfragment_bytesfrom a known startingMessageId.FragmentBatch’sIntoIteratorimplementation yielding all fragments in order.fragment_with_idhonouring the caller-suppliedMessageIdwithout advancing the internal counter, then resuming from the configured starting id on the next auto-assigned batch.This set resolves the earlier request for a focused
fragment_with_idtest while tightening overall Fragmenter behaviour coverage.tests/worlds/fragment.rs (1)
37-58: Keep centralised batch/fragment helpers and new assertionsKeep
configure_fragmenter,fragment_payload,batch, andget_fragment_at; they provide a clear Fragmenter workflow in the world state, enforce non-zero caps, and centralise both “no payload fragmented yet” and “fragment {index} missing” failure modes. The new assertions (assert_fragment_count,assert_fragment_payload_len,assert_fragment_final_flag,assert_message_id) build on these helpers to produce precise diagnostics while eliminating the earlier duplicated fragment-access logic.Also applies to: 98-109, 197-241
src/fragment/fragmenter.rs (2)
43-59: Keep the documented, non-wrapping MessageId generationKeep this
next_message_idimplementation and documentation. Thefetch_update+checked_addlogic prevents silent wraparound, panics with a clear “message id counter exhausted” message onu64::MAX, and matches both the documented behaviour and the tests that rely onwith_starting_idyielding the startingMessageIdon the first allocation.
105-139: Eliminate potentialusizeoverflow in fragment boundary calculationReplace the plain
offset + maxaddition with a saturating addition before clamping tototal. With an unconstrainedNonZeroUsizemax_fragment_size, extreme configurations (largemaxand large payload) can causeoffset + maxto overflowusize, which either wraps or panics depending on build configuration.Apply this change in
build_fragments:- while offset < total { - let end = (offset + max).min(total); + while offset < total { + let end = offset.saturating_add(max).min(total); let is_last = end == total; fragments.push(FragmentFrame::new( FragmentHeader::new(message_id, index, is_last), payload[offset..end].to_vec(), ));This preserves the intended behaviour for normal sizes while removing the overflow risk.
#!/bin/bash # Verify that fragment boundary computation uses saturating_add after patching. rg -n 'offset\.saturating_add\(max\)\.min\(total\)' src/fragment/fragmenter.rs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/users-guide.md(1 hunks)src/fragment/fragmenter.rs(1 hunks)src/fragment/tests.rs(2 hunks)tests/worlds/fragment.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (docs/message-versioning.md)
Document examples showing how to deprecate old message versions gracefully
When long lines are warranted in documentation, disable markdownlint rule MD013 inline using the comment:
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation accurate
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)Follow markdownlint recommendations
Files:
docs/users-guide.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/users-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/users-guide.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/users-guide.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,rs}: Use British English (en-GB-oxendict) spelling in documentation prose
Keep US spelling when it appears in API identifiers (e.g.,color)
The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are…”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure Markdown tables include a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns
Do not wrap tables
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding images, use Markdownwith brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/users-guide.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely 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; 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 with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
src/fragment/tests.rstests/worlds/fragment.rssrc/fragment/fragmenter.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/fragment/tests.rstests/worlds/fragment.rssrc/fragment/fragmenter.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/worlds/fragment.rs
🧬 Code graph analysis (2)
tests/worlds/fragment.rs (5)
src/fragment/fragmenter.rs (7)
new(26-28)new(153-153)new(176-182)len(198-198)payload(161-161)header(157-157)message_id(186-186)src/fragment/id.rs (1)
new(25-25)src/fragment/header.rs (3)
new(35-45)is_last_fragment(57-57)message_id(49-49)src/fragment/index.rs (1)
new(30-30)src/fragment/series.rs (2)
new(23-29)message_id(33-33)
src/fragment/fragmenter.rs (1)
src/fragment/index.rs (1)
zero(34-34)
⏰ 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: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (2)
docs/users-guide.md (1)
151-177: Align Fragmenter docs and example with the public APIKeep this block: it now names
FragmentBatchandFragmentFrame, explains thatFragmentationErrorreports encode failures and index overflows, and illustrates correct usage ofFragmenter::newandfragment_byteswith a payload slice and realistictracingfields. This aligns the user guide with the current API and resolves the earlier fallibility and signature concerns.src/fragment/tests.rs (1)
84-116: Keep these Fragmenter edge-case tests for payload splittingKeep
fragmenter_splits_payload_into_multiple_framesandfragmenter_handles_empty_payload; they exercise core invariants: correct chunking at the configured cap,is_fragmentedsemantics,MessageId::new(0)fromFragmenter::new, and the empty-payload case producing a single final fragment at index zero.
…ation Add outbound fragmentation scenarios by chunking payloads using the Fragmenter helper and inspecting FragmentBatch states. Enhance documentation and assertions to verify ordering, completion detection, and error handling for both inbound and outbound fragmentation in FragmentWorld behavioural tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…lice instead of vec In docs/users-guide.md, replaced `vec![0_u8; 1400]` with fixed-size array `[0_u8; 1400]` for slice compatibility. In src/fragment/fragmenter.rs, changed Clippy lint attribute from `#[allow(clippy::len_without_is_empty)]` to `#[expect(clippy::len_without_is_empty)]` to better express intent about non-empty batches. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
deleted the
terragon/implement-fragmentation-reassembly-fowx16
branch
1 participant
Summary
Changes
Core functionality
Public API surface
Tests
Documentation & changelog
Backwards compatibility
Example usage
Test plan
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/b73c1632-521c-4ee5-9a0a-045c2128dbbf
Summary by Sourcery
Introduce an outbound fragmentation helper and wire it into the public fragment API, along with tests and documentation describing its behavior.
New Features:
Enhancements:
Documentation:
Tests:
Chores: