Implement explicit MemoryBudgets for the wireframe app#304
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request implements explicit memory budgets for the Wireframe Hotline adapter and refactors inbound fragment handling. Physical frame extraction now delegates to a dedicated helper; stateful reassembly moves from Changes
Poem
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements explicit Wireframe MemoryBudgets for the Hotline wireframe adapter by introducing a Hotline-specific message assembler and logical message-size budgeting, wiring these budgets into the server app factory, and adding regression tests and documentation updates to validate oversized and stalled fragmented request handling. Class diagram for Hotline message assembly and budgetingclassDiagram
class HotlineFrameCodec {
+new() HotlineFrameCodec
+decoder() HotlineFrameDecoder
+wrap_payload(payload: Bytes) Vec~u8~
+max_frame_length() usize
}
class HotlineFrameDecoder {
-series: InboundSeriesState~Option~
+new() HotlineFrameDecoder
-build_first_frame_payload(header: FrameHeader, payload: [u8]) Result~Vec~u8~~
-build_continuation_payload(header: FrameHeader, payload: [u8]) Result~Vec~u8~~
+decode(src: BytesMut) Result~Option~Packet~~
}
class InboundSeriesState {
-first_header: FrameHeader
-message_key: MessageKey
-remaining: usize
-next_sequence: FrameSequence
-deadline: Instant
}
class HotlineMessageAssembler {
+new() HotlineMessageAssembler
+parse_frame_header(payload: [u8]) Result~ParsedFrameHeader~
-logical_header_bytes(header: FrameHeader) [u8]
}
class MessageAssembler {
<<interface>>
+parse_frame_header(payload: [u8]) Result~ParsedFrameHeader~
}
class FrameHeader {
+flags: u16
+is_reply: u16
+ty: u16
+id: u16
+error: u16
+total_size: u32
+data_size: u32
+from_bytes(bytes: [u8;20]) FrameHeader
+write_bytes(dst: [u8;20]) void
}
class FrameSequence {
+value: u32
+checked_increment() Option~FrameSequence~
}
class MessageKey {
+value: u64
}
class ParsedFrameHeader {
+header() AssemblyFrameHeader
+header_len() usize
}
class AssemblyFrameHeader {
<<enum>>
+First(FirstFrameHeader)
+Continuation(ContinuationFrameHeader)
}
class FirstFrameHeader {
+message_key: MessageKey
+metadata_len: usize
+body_len: usize
+total_body_len: Option~usize~
+is_last: bool
}
class ContinuationFrameHeader {
+message_key: MessageKey
+sequence: Option~FrameSequence~
+body_len: usize
+is_last: bool
}
class MemoryBudgets {
+new(per_message: BudgetBytes, per_connection: BudgetBytes, in_flight: BudgetBytes) MemoryBudgets
+bytes_per_message() BudgetBytes
+bytes_per_connection() BudgetBytes
+bytes_in_flight() BudgetBytes
}
class BudgetBytes {
+new(bytes: NonZeroUsize) BudgetBytes
+as_usize() usize
}
class HotlineBudgetsModule {
<<module>>
+explicit_memory_budgets() MemoryBudgets
-non_zero(bytes: usize) NonZeroUsize
}
class HotlineApp {
+default() HotlineApp
+fragmentation(option: Option~()~) HotlineApp
+memory_budgets(budgets: MemoryBudgets) HotlineApp
+with_message_assembler(assembler: HotlineMessageAssembler) HotlineApp
}
class BudgetsConst {
<<constant>>
+HOTLINE_LOGICAL_MESSAGE_BYTES: usize
}
HotlineFrameCodec --> HotlineFrameDecoder
HotlineFrameDecoder --> InboundSeriesState : manages
HotlineFrameDecoder --> FrameHeader
HotlineFrameDecoder --> MessageKey
HotlineFrameDecoder --> FrameSequence
HotlineFrameDecoder --> HotlineMessageAssembler : uses via Envelope payloads
HotlineMessageAssembler ..|> MessageAssembler
HotlineMessageAssembler --> FrameHeader
HotlineMessageAssembler --> MessageKey
HotlineMessageAssembler --> ParsedFrameHeader
ParsedFrameHeader --> AssemblyFrameHeader
AssemblyFrameHeader --> FirstFrameHeader
AssemblyFrameHeader --> ContinuationFrameHeader
HotlineBudgetsModule --> MemoryBudgets : builds
HotlineBudgetsModule --> BudgetBytes : derives
HotlineBudgetsModule --> BudgetsConst
HotlineApp --> MemoryBudgets : configures
HotlineApp --> HotlineMessageAssembler : installs
BudgetsConst --> FrameHeader : derived from HEADER_LEN
BudgetsConst --> FrameHeader : uses MAX_PAYLOAD_SIZE
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: src/wireframe/message_assembly.rs Comment on file pub(crate) fn first_frame_payload(
message_key: MessageKey,
header: &FrameHeader,
body: &[u8],
) -> Result<Vec<u8>, io::Error> {
let body_len = u32::try_from(body.len()).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidData,
"Hotline first-frame body length exceeds u32",
)
})?;
let mut payload = Vec::with_capacity(FIRST_FRAME_HEADER_LEN + HEADER_LEN + body.len());
payload.push(FIRST_FRAME_TAG);
payload.extend_from_slice(&u64::from(message_key).to_be_bytes());
payload.extend_from_slice(&header.total_size.to_be_bytes());
payload.extend_from_slice(&body_len.to_be_bytes());
payload.extend_from_slice(&logical_header_bytes(header));
payload.extend_from_slice(body);
Ok(payload)
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/wireframe/message_assembly.rs: What lead to degradation?The module contains 2 functions with similar structure: continuation_frame_payload,first_frame_payload Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/wireframe/message_assembly.rs Comment on lines +84 to +103 fn assemble_frame_payload(
tag: u8,
message_key: MessageKey,
middle: &[u8],
body: &[u8],
metadata: &[u8],
body_len_err: &'static str,
) -> Result<Vec<u8>, io::Error> {
let body_len = u32::try_from(body.len())
.map_err(|_| io::Error::new(io::ErrorKind::InvalidData, body_len_err))?;
let capacity = 1 + 8 + middle.len() + 4 + metadata.len() + body.len();
let mut payload = Vec::with_capacity(capacity);
payload.push(tag);
payload.extend_from_slice(&u64::from(message_key).to_be_bytes());
payload.extend_from_slice(middle);
payload.extend_from_slice(&body_len.to_be_bytes());
payload.extend_from_slice(metadata);
payload.extend_from_slice(body);
Ok(payload)
}❌ New issue: Excess Number of Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/wireframe/message_assembly.rs Comment on file //! Hotline-specific protocol message assembly helpers for Wireframe.
❌ New issue: Primitive Obsession |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/wireframe/message_assembly.rs Comment on file //! Hotline-specific protocol message assembly helpers for Wireframe.
❌ New issue: Primitive Obsession |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/wireframe/message_assembly_tests.rs Comment on lines +126 to +142 fn first_frame_parser_rejects_logical_header_total_mismatches() {
let header = header(10, 4);
let mut payload =
first_frame_payload(message_key_for(&header), &header, b"data").expect("payload");
payload[29..33].copy_from_slice(&9u32.to_be_bytes());
let err = HotlineMessageAssembler::new()
.parse_frame_header(&payload)
.expect_err("logical header total must match declared first-frame total");
assert_eq!(err.kind(), std::io::ErrorKind::InvalidData);
assert!(
err.to_string().contains(
"Hotline first-frame logical header length does not match declared total length",
),
"unexpected error: {err}"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Move the zero-byte continuation guard in `InboundSeriesTracker` behind the remaining-byte lookup so the tracker can treat an empty continuation as a no-progress protocol error and clear the active series before returning. Add regression coverage for the tracker-level state-clearing path and align the existing decoder test with the new error message.
Extract the shared payload-tampering assertion used by the message-assembly rejection tests so the three body-length and logical-header mismatch cases differ only in their payload source and u32 field offsets. Keep the rest of the test module unchanged while preserving the same error checks and targeted test coverage.
Insert the missing blank line before `first_frame_payload` so the internal payload builder and the following Rustdoc stay visually separated. Update `docs/file-sharing-design.md` to remove the remaining first-person phrasing in the verified metadata paragraph, and extend the Wireframe migration guide with the required Markdown validation CI job for Mermaid-bearing documentation.
Update `docs/design.md` to describe the actual `InboundSeriesTracker` error-handling contract. Document that `validate_fragment_consistency()` preserves the active series on header mismatch, and list the specific completion and error paths that clear state. Extract the repeated zero-progress tracker setup in `src/wireframe/codec/frame_tests.rs` into a small helper so the two tests keep only their distinct assertions.
f5910c6 to
fe4767c
Compare
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Update the memory-budget execplan and related documentation so the shipped contract is described precisely. Remove the stale pacing claim, rewrite the cited first-person passages in `docs/file-sharing-design.md`, and normalize the requested spelling fixes in the migration and layout guides. Replace three near-duplicate message-assembly rejection tests with a single `#[rstest]` table. Keep the same payload builders, tamper ranges, and expected error substrings while removing duplicated arrange/act/ assert scaffolding.
Reduce primitive-argument density in the Hotline message assembly helpers by introducing `ParseError` for static parse contexts and `IsLast` for continuation last-flag handling. Thread the newtypes through `PayloadCursor`, payload parsing, and the Wireframe frame decoder so the continuation payload path keeps the same behaviour while exposing fewer raw primitives at call sites. Update the message-assembly tests to pass the new continuation flag wrapper and rerun the full Rust gates.
|
@coderabbitai Please investigate the cause of the following test failure and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Have the following now been resolved?
|
This comment was marked as resolved.
This comment was marked as resolved.
Treat `ExecutableFileBusy` from the Helix version probe as a non-Helix result so a transient `execve(2)` race does not surface as a probe failure. Update the timeout-probe test to write the script through a file handle and call `sync_all()` before execution. This closes the Linux `ETXTBSY` window that appears when the inode is still considered writable.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. ❌ Failed checks (1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9697
The Testing check should now be satisfied. |
Replace the remaining American `normalized` spellings in the scoped wireframe doc comments with the repository's en-GB-oxendict `normalised` form. Keep the change limited to the four requested documentation occurrences and rerun `cargo test` to confirm there is no behavioural regression.
There was a problem hiding this comment.
Gates Passed
6 Quality Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Summary
Changes
Documentation
Design & Roadmap Artifacts
Planning Artifacts
Implementation (Code)
Tests
Validation / Aftercare
Plan of Work (highlights)
Validation / Aftercare (continued)
Context
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/99334822-1d9c-4854-beab-b7de13c18cfb
Summary by Sourcery
Configure explicit Wireframe memory budgets for Hotline request assembly and document the resulting behaviour and constraints.
New Features:
Enhancements:
Tests:
📎 Task: https://www.devboxer.com/task/f2676e40-37a8-46e6-98cd-15aa1ebb49d8