MemoryBudgets type and per-connection budgets builder (8.3.1)#463
MemoryBudgets type and per-connection budgets builder (8.3.1)#463
Conversation
Reviewer's GuideIntroduces a new MemoryBudgets value type and wires optional per-connection memory budget configuration through the WireframeApp builder, rebuild paths, tests, and documentation without altering runtime behaviour when budgets are unset. Class diagram for MemoryBudgets and updated WireframeApp builderclassDiagram
class MemoryBudgets {
+NonZeroUsize message_budget
+NonZeroUsize connection_window
+NonZeroUsize assembly_bytes
+MemoryBudgets new(bytes_per_message NonZeroUsize, bytes_per_connection NonZeroUsize, bytes_in_flight NonZeroUsize)
+NonZeroUsize bytes_per_message()
+NonZeroUsize bytes_per_connection()
+NonZeroUsize bytes_in_flight()
}
class WireframeApp~S, C, E, F~ {
+Option~MemoryBudgets~ memory_budgets
+WireframeApp~S, C, E, F~ default()
+WireframeApp~S2, C, E, F2~ rebuild_with_params(params RebuildParams~S2, F2~)
+WireframeApp~S, C, E, F~ memory_budgets(budgets MemoryBudgets)
+WireframeApp~S, C, E2, F2~ with_codec(codec F2)
+WireframeApp~S2, C, E, F~ serializer(serializer S2)
}
class RebuildParams~S2, F2~ {
+S2 serializer
+F2 codec
+Option~WireframeProtocol~ protocol
+Option~FragmentationConfig~ fragmentation
+Option~MessageAssembler~ message_assembler
+Option~MemoryBudgets~ memory_budgets
}
WireframeApp "0..1" --> "0..1" MemoryBudgets : stores
RebuildParams "0..1" --> "0..1" MemoryBudgets : carries
WireframeApp "1" --> "1" RebuildParams : rebuild_with_params
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbit
WalkthroughIntroduce a per-connection memory budgeting configuration surface: add a public Changes
Sequence Diagram(s)(Skip — changes add a configuration surface and plumbing; no new multi-component runtime control flow requiring sequencing.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/fixtures/memory_budgets.rs Comment on lines +93 to +104 pub fn assert_message_budget(&self, expected: BudgetBytes) -> TestResult {
let budgets = self.budgets.ok_or("memory budgets not configured")?;
if budgets.bytes_per_message().get() != expected.0 {
return Err(format!(
"expected message budget {}, got {}",
expected.0,
budgets.bytes_per_message().get()
)
.into());
}
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
7b9bed7 to
9a7f880
Compare
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/fixtures/memory_budgets.rs:41-50` </location>
<code_context>
+ /// # Errors
+ ///
+ /// Returns an error when any value is zero.
+ pub fn set_budgets(
+ &mut self,
+ per_message: BudgetBytes,
+ per_connection: BudgetBytes,
+ in_flight: BudgetBytes,
+ ) -> TestResult {
+ let per_message =
+ NonZeroUsize::new(per_message.0).ok_or("message budget must be non-zero")?;
+ let per_connection =
+ NonZeroUsize::new(per_connection.0).ok_or("connection budget must be non-zero")?;
+ let in_flight =
+ NonZeroUsize::new(in_flight.0).ok_or("in-flight budget must be non-zero")?;
+ self.budgets = Some(MemoryBudgets::new(per_message, per_connection, in_flight));
+ Ok(())
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Add behavioural coverage for zero byte budgets being rejected.
`set_budgets` rejects zero values via `NonZeroUsize::new(...)`, but no scenario currently covers this error path. Please add one or more `memory_budgets.feature` scenarios (e.g. with a 0 message-byte budget) that assert an error, so the non-zero constraint is captured and verified at the Gherkin level.
Suggested implementation:
```rust
impl MemoryBudgetsWorld {
/// Configure memory budgets in the world.
///
/// # Errors
///
/// Returns an error when any value is zero.
pub fn set_budgets(
&mut self,
per_message: BudgetBytes,
per_connection: BudgetBytes,
in_flight: BudgetBytes,
) -> TestResult {
let per_message =
NonZeroUsize::new(per_message.0).ok_or("message budget must be non-zero")?;
let per_connection =
NonZeroUsize::new(per_connection.0).ok_or("connection budget must be non-zero")?;
let in_flight =
NonZeroUsize::new(in_flight.0).ok_or("in-flight budget must be non-zero")?;
self.budgets = Some(MemoryBudgets::new(per_message, per_connection, in_flight));
Ok(())
}
}
/// Behavioural test world for memory budget configuration.
```
To fully implement your review comment, you should also:
1. **Add Gherkin scenarios** in `tests/features/memory_budgets.feature` (or equivalent), for example:
- A scenario where the message-byte budget is `0` and the step asserts that configuring memory budgets fails.
- Optionally, similar scenarios for `per_connection` and `in_flight` budgets being `0`.
Example (to adapt to your existing step wording/conventions):
```gherkin
Scenario: Zero message-byte budget is rejected
When I configure memory budgets with a 0 byte message budget,
a 1024 byte connection budget,
and a 4096 byte in-flight budget
Then configuring memory budgets fails
```
2. **Update or add step definitions** (likely in a `memory_budgets_steps.rs` or similar) to:
- Call `world.set_budgets(...)` with the values from the scenario.
- Capture the `Result` from `set_budgets`.
- In the `Then configuring memory budgets fails` step, assert that the result is an `Err` and (optionally) matches the expected error message like `"message budget must be non-zero"`.
3. If you don’t currently propagate errors out of the steps, you may need to:
- Store the last `Result` on the `MemoryBudgetsWorld` (e.g. `last_error: Option<String>` or similar) so that your assertion steps can inspect it.
</issue_to_address>
### Comment 2
<location> `docs/execplans/8-3-1-memory-budgets-builder.md:335` </location>
<code_context>
+
+## Artifacts and notes
+
+Expected artifacts after completion:
+
+- New/updated app configuration files for `MemoryBudgets` and builder wiring.
</code_context>
<issue_to_address>
**nitpick (review_instructions):** "artifacts" uses en-US spelling; en-GB spelling "artefacts" should be used instead.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 3
<location> `docs/execplans/8-3-1-memory-budgets-builder.md:54` </location>
<code_context>
+
+## Tolerances (exception triggers)
+
+- Scope: if this work requires touching more than 12 files or exceeds 500 net
+ LOC, stop and escalate.
+- Interface: if implementing `memory_budgets(...)` requires additional public
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "LOC" on the following wrapped line is not defined on first use.
Consider expanding this to something like "500 net lines of code (LOC)" on the first occurrence so the acronym is clear.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location> `docs/execplans/8-3-1-memory-budgets-builder.md:340` </location>
<code_context>
+- New/updated app configuration files for `MemoryBudgets` and builder wiring.
+- New `rstest` unit tests covering memory-budget builder behaviour.
+- New `rstest-bdd` feature/fixture/steps/scenario for builder API behaviour.
+- Updated design docs (`ADR 0002` and related alignment notes),
+ `docs/users-guide.md`, and `docs/roadmap.md`.
+- Gate logs:
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "ADR" is introduced without being expanded on first use.
Since ADR is not a universally known acronym, consider expanding it on first mention, for example "Architecture Decision Record (ADR 0002)".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</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.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/builder/core.rs (1)
189-267:⚠️ Potential issue | 🟠 MajorReplace panicking
.expect()calls in rstest fixtures with fallible fixtures.Remove
.expect(...)fromapp_builderandmemory_budgetsfixtures. ReturnResultfrom fixtures and propagate with?in tests to comply with fixture error-handling rules and avoid panics. All eight test functions consuming these fixtures must also returnResultto handle fixture failures.🔧 Refactor fixtures to return Result and propagate failures
- #[fixture] - fn app_builder() -> TestApp { - let app = TestApp::new().expect("build app"); + #[fixture] + fn app_builder() -> Result<TestApp> { + let app = TestApp::new()?; assert!( app.fragmentation.is_none(), "fixture expects default fragmentation disabled" ); assert!( app.memory_budgets.is_none(), "fixture expects default memory budgets disabled" ); - app + Ok(app) } #[fixture] - fn memory_budgets() -> MemoryBudgets { - MemoryBudgets::new( - NonZeroUsize::new(1024).expect("non-zero"), - NonZeroUsize::new(4096).expect("non-zero"), - NonZeroUsize::new(2048).expect("non-zero"), - ) - } + fn memory_budgets() -> Result<MemoryBudgets, &'static str> { + Ok(MemoryBudgets::new( + NonZeroUsize::new(1024).ok_or("non-zero budget")?, + NonZeroUsize::new(4096).ok_or("non-zero budget")?, + NonZeroUsize::new(2048).ok_or("non-zero budget")?, + )) + } #[rstest] fn builder_defaults_fragmentation_to_disabled( - app_builder: TestApp, - ) { - let app = app_builder; + app_builder: Result<TestApp>, + ) -> Result<(), Box<dyn std::error::Error>> { + let app = app_builder?; assert!(app.fragmentation.is_none()); + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/builder/core.rs` around lines 189 - 267, The fixtures app_builder and memory_budgets currently panic via .expect(); change app_builder to return Result<TestApp, E> (call TestApp::new()? and return Ok(app)) and change memory_budgets to return Result<MemoryBudgets, E> by using NonZeroUsize::new(...).ok_or(...)? inside MemoryBudgets::new and returning Ok(...). Then update all test functions that consume these fixtures (builder_defaults_fragmentation_to_disabled, builder_defaults_memory_budgets_to_disabled, enable_fragmentation_requires_explicit_opt_in, with_codec_clears_fragmentation_to_require_reconfiguration, buffer_capacity_clears_fragmentation_to_require_reconfiguration, memory_budgets_builder_stores_configuration, with_codec_preserves_memory_budgets, serializer_preserves_memory_budgets) to accept Result<TestApp> or Result<MemoryBudgets> as appropriate, return Result<(), Box<dyn std::error::Error>> and use the ? operator to unwrap fixtures within each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/8-3-1-memory-budgets-builder.md`:
- Around line 274-277: Update the expected transcript snippet in
docs/execplans/8-3-1-memory-budgets-builder.md by replacing the lowercase "ok"
with the en-GB form "OK" in the unit line (change the string `test result: ok.`
to `test result: OK.`) so the snippet reads "unit: `test result: OK.` with new
builder-memory-budget tests listed."; ensure only that token is changed and
spacing/formatting stays the same.
- Around line 111-116: Remove the legacy Cucumber paragraph that references
`docs/behavioural-testing-in-rust-with-cucumber.md` and replace it with the
concise observation that behavioural guidance lives in
`docs/rstest-bdd-users-guide.md`, updating the Impact line to state that
behavioural test implementation should follow rstest-bdd patterns; specifically
edit the paragraph beginning with "Observation:" in the existing section to the
proposed two-line text ("Observation: behavioural guidance lives in
`docs/rstest-bdd-users-guide.md`." and "Impact: behavioural test implementation
should follow rstest-bdd patterns."), removing any historical-retention note.
In `@tests/fixtures/memory_budgets.rs`:
- Around line 29-33: Remove the #[rustfmt::skip] attribute and collapse the
trivial fixture function memory_budgets_world into a single-line form returning
MemoryBudgetsWorld::default(); specifically edit the function definition for pub
fn memory_budgets_world() -> MemoryBudgetsWorld to be a one-liner and ensure no
rustfmt skip remains.
---
Outside diff comments:
In `@src/app/builder/core.rs`:
- Around line 189-267: The fixtures app_builder and memory_budgets currently
panic via .expect(); change app_builder to return Result<TestApp, E> (call
TestApp::new()? and return Ok(app)) and change memory_budgets to return
Result<MemoryBudgets, E> by using NonZeroUsize::new(...).ok_or(...)? inside
MemoryBudgets::new and returning Ok(...). Then update all test functions that
consume these fixtures (builder_defaults_fragmentation_to_disabled,
builder_defaults_memory_budgets_to_disabled,
enable_fragmentation_requires_explicit_opt_in,
with_codec_clears_fragmentation_to_require_reconfiguration,
buffer_capacity_clears_fragmentation_to_require_reconfiguration,
memory_budgets_builder_stores_configuration,
with_codec_preserves_memory_budgets, serializer_preserves_memory_budgets) to
accept Result<TestApp> or Result<MemoryBudgets> as appropriate, return
Result<(), Box<dyn std::error::Error>> and use the ? operator to unwrap fixtures
within each test.
|
@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: 11664
✅ Comment 1: Zero-budget behavioural test coverageResolved — A complete scenario now exists in Scenario: Reject zero message-byte budget
When attempting to configure memory budgets with message bytes 0, connection bytes 4096, and in-flight bytes 2048
Then configuring memory budgets fails with error "message budget must be non-zero"The fixture implements ✅ Comment 2: en-GB spellingResolved — Line 335 now reads: Expected artefacts after completion:Changed from en-US "artifacts" to en-GB "artefacts". ✅ Comment 3: LOC acronym definitionResolved — Line 56 now expands the acronym on first use: - Scope: if this work requires touching more than 12 files or exceeds 500 net
lines of code (LOC), stop and escalate.✅ Comment 4: ADR acronym definitionResolved — Line 340 now expands the acronym on first use: - Updated design docs (Architecture Decision Record `ADR 0002` and related
alignment notes), `docs/users-guide.md`, and `docs/roadmap.md`.Bonus: The earlier code duplication issue has also been addressed — the three assertion methods now delegate to a private |
Introduce a dedicated MemoryBudgets configuration type capturing per-message, per-connection, and in-flight assembly byte limits, and add WireframeApp::memory_budgets(...) builder method to configure these budgets. This milestone (8.3.1) adds configuration surface only, with enforcement and runtime behavior changes planned for future roadmap items. Includes unit and behavioural tests validating builder API usability and composition, plus documentation updates and design decision records. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Introduce `MemoryBudgets` struct encapsulating three non-zero byte caps: bytes per message, bytes per connection, and bytes in flight. - Add `WireframeApp::memory_budgets(...)` builder method for setting budgets. - Thread memory budget option through all relevant builder and app layers. - Add unit tests using `rstest` and behavioral tests with `rstest-bdd` for coverage. - Update documentation including ADR, user guide, roadmap, and examples. - Mark roadmap item 8.3.1 as complete and record lint-related discoveries. This introduces explicit configuration of memory caps per connection for inbound buffering and assembly, forming groundwork for runtime enforcement in follow-up work. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Refactored the MemoryBudgetsWorld tests to consolidate repetitive code for asserting budgets by introducing a generic helper method `assert_budget`. This reduces duplication and improves maintainability of budget assertion logic in test fixtures. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…ests - Introduce memory budgets builder methods and internal validation for non-zero budgets. - Update existing builder tests to handle result-based fixtures with error checks. - Add behavioural tests and step definitions to cover memory budget configuration success and failure scenarios. - Enhance documentation with updated behavioural testing guidance and detailed memory budgets builder notes. - Refactor test fixtures and error handling for memory budgets setup. This enables explicit memory budget configurations for the Wireframe app builder with proper validation, improving resource control and test coverage. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Map TestApp::new() errors into io::Error in the rstest fixture so\n can propagate as under the stricter\npost-rebase trait bounds.
b4a4077 to
ea1a534
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/memory_budgets.rs`:
- Around line 1-5: Update the module docstring to state that this module only
exposes configuration and that enforcement will be implemented in a future
iteration (replace the sentence "Enforcement is applied by downstream inbound
assembly components" with "This module only exposes configuration; enforcement
lands in a future iteration."). Replace the public API uses of raw NonZeroUsize
with domain-specific NewType wrappers (e.g., MessageBudget, ConnectionBudget,
InFlightBudget) and update the MemoryBudgets struct to use these new types
(adjust constructors, accessors, and any impls on MemoryBudgets accordingly).
Propagate the types to the WireframeApp::memory_budgets API and any public
builders so call sites use the new types instead of plain integers, and add
minimal impls (From/Into/Debug/Clone/Copy/serde derives as appropriate) for the
new NewTypes to preserve ergonomics. Ensure no enforcement logic is added—only
type and doc changes.
- Around line 35-66: The PR asks to introduce a newtype BudgetBytes and replace
raw NonZeroUsize fields in MemoryBudgets; update the MemoryBudgets struct fields
(message_budget, connection_window, assembly_bytes) to BudgetBytes, change
MemoryBudgets::new signature to accept BudgetBytes, and make the accessor
methods bytes_per_message, bytes_per_connection, bytes_in_flight return
BudgetBytes; add BudgetBytes with a const new(BudgetBytes) constructor and a
getter like get() -> NonZeroUsize or as_usize() -> usize for ergonomics, then
update all call sites mentioned (builder/core.rs and builder/config.rs) and any
doctests/docs that consume the old NonZeroUsize to construct or extract via
BudgetBytes::new or the getter.
---
Duplicate comments:
In `@tests/fixtures/memory_budgets.rs`:
- Around line 29-34: The fixture function memory_budgets_world is unnecessarily
multi-line with an explanatory comment; collapse it into a single-line function
while keeping the #[fixture] attribute and the same signature/return type:
replace the current multi-line body and comment with a single-line body that
returns MemoryBudgetsWorld::default() (preserve function name
memory_budgets_world and type MemoryBudgetsWorld).
…memory budget units - Added a newtype `BudgetBytes` wrapping `NonZeroUsize` to explicitly represent byte budgets. - Updated `MemoryBudgets` struct and related APIs to use `BudgetBytes` instead of raw `NonZeroUsize`. - Updated builders, docs, tests, and fixtures accordingly to use `BudgetBytes` for clarity and type safety. - Improved serialization support and added conversions between `BudgetBytes` and underlying numeric types. - This change improves code clarity and enforces explicit handling of byte-based budget values. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/users-guide.md`:
- Around line 533-534: The paragraph starting "This builder method establishes
configuration for the current `WireframeApp` instance..." exceeds the 80‑column
wrap limit; reflow that markdown paragraph to wrap at 80 characters (split the
long sentence into multiple lines) so each line is ≤80 columns while preserving
the sentence content and inline code `WireframeApp`; ensure paragraph breaks
remain unchanged and no additional markdown formatting is added.
In `@tests/steps/memory_budgets_steps.rs`:
- Around line 7-66: The step patterns in given_memory_budgets,
when_attempting_to_configure_memory_budgets, then_message_budget,
then_connection_budget and then_in_flight_budget use `{...:usize}` but their
handler parameters are StepBudgetBytes; remove the `:usize` annotations from the
pattern tokens (e.g. change `{per_message:usize}` → `{per_message}` and
`{expected:usize}` → `{expected}`) so Cucumber will parse via
StepBudgetBytes::from_str; update the patterns in the functions named
given_memory_budgets, when_attempting_to_configure_memory_budgets,
when_configuring_app_with_budgets_and_codec (if present), then_message_budget,
then_connection_budget and then_in_flight_budget accordingly.
---
Duplicate comments:
In `@tests/fixtures/memory_budgets.rs`:
- Around line 32-37: The fixture function memory_budgets_world is unnecessarily
multi-line and annotated with #[rustfmt::skip]; collapse it into a single-line
definition and remove the #[rustfmt::skip] attribute so the function is
formatted normally (i.e., replace the current multi-line pub fn
memory_budgets_world() -> MemoryBudgetsWorld { MemoryBudgetsWorld::default() }
style in one line and drop #[rustfmt::skip]).
…tep parameters The usize type annotations within the test step attribute macros have been removed to simplify the step definitions. This change updates the given, when, and then steps in the memory_budgets_steps.rs test file by omitting explicit usize annotations from parameters and expected values. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Changes
Core data model
src/app/memory_budgets.rswith publicMemoryBudgetsvalue type and accessors:bytes_per_message: NonZeroUsizebytes_per_connection: NonZeroUsizebytes_in_flight: NonZeroUsizeMemoryBudgets::new(...)and getters.src/app/mod.rsfor crate-wide access.WireframeApp storage and builder wiring
WireframeAppstorage to hold optional memory budgets.Default,RebuildParams).memory_budgets(...)to the public builder API insrc/app/builder/config.rs:pub fn memory_budgets(self, budgets: crate::app::MemoryBudgets) -> SelfTests and validation scaffolding
WireframeApp::new()memory_budgets(...)stores the configured valueDocumentation and roadmaps
docs/execplans/8-3-1-memory-budgets-builder.mdand updateddocs/roadmap.mdanddocs/users-guide.mdaccordingly.Tests plan (status)
Validation and acceptance
WireframeApp::memory_budgets(...)is available to crate users.Interfaces and dependencies
src/app/memory_budgets.rs(MemoryBudgets type)src/app/builder/config.rs->WireframeApp::memory_budgets(...)rstest/rstest-bddsetup used for testsArtefacts and notes
How to review locally
📎 Task: https://www.devboxer.com/task/b7ec5f63-520d-4889-80a8-8da6c90ed234