Define derived defaults for connection memory budgets#486
Merged
Conversation
Implement roadmap item 8.3.5 to derive sensible default per-message, per-connection, and in-flight memory budgets based on the codec's buffer_capacity (max_frame_length) when explicit budgets are not set. Defaults use multipliers aligned with fragmentation defaults: per_message = frame_budget × 16, per_connection = frame_budget × 64, bytes_in_flight = frame_budget × 64. Budgets are computed lazily at connection time in process_stream via resolve_effective_budgets(), preserving existing builder APIs and respecting explicit budget overrides. Add unit tests for default_memory_budgets() and resolve_effective_budgets(), and behavioral tests validating derived budgets enforce protection tiers and explicit budgets override defaults. Update ADR-002 with implementation decisions, users guide with derived budget defaults, and mark roadmap item 8.3.5 done. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements lazy, derived default per-connection memory budgets from codec buffer_capacity/frame_budget, wires them into inbound processing via a resolve_effective_budgets helper, and adds tests and docs (including an execplan and ADR updates) to validate and document the new behavior and roadmap item 8.3.5. Sequence diagram for resolving and applying effective memory budgetssequenceDiagram
actor Client
participant WireframeApp
participant FrameHandling as FrameHandling
participant Backpressure as Backpressure
participant BuilderDefaults as BuilderDefaults
participant MessageAssembly as MessageAssemblyState
Client->>WireframeApp: process_stream(stream)
WireframeApp->>WireframeApp: codec.max_frame_length() -> requested_frame_length
WireframeApp->>WireframeApp: framed.read_buffer_mut().reserve(max_frame_length)
WireframeApp->>FrameHandling: resolve_effective_budgets(self.memory_budgets, requested_frame_length)
activate FrameHandling
FrameHandling->>Backpressure: resolve_effective_budgets(explicit, frame_budget)
activate Backpressure
Backpressure->>BuilderDefaults: default_memory_budgets(frame_budget) [when explicit is None]
activate BuilderDefaults
BuilderDefaults-->>Backpressure: derived MemoryBudgets
deactivate BuilderDefaults
Backpressure-->>FrameHandling: MemoryBudgets (effective_budgets)
deactivate Backpressure
FrameHandling-->>WireframeApp: MemoryBudgets (effective_budgets)
deactivate FrameHandling
WireframeApp->>FrameHandling: new_message_assembly_state(self.fragmentation, requested_frame_length, Some(effective_budgets))
FrameHandling-->>WireframeApp: Option MessageAssemblyState
WireframeApp->>WireframeApp: FramePipeline::new(self.fragmentation)
loop read_frames
WireframeApp->>FrameHandling: evaluate_memory_pressure(message_assembly.as_ref(), Some(effective_budgets))
FrameHandling-->>WireframeApp: Pressure
WireframeApp->>FrameHandling: apply_memory_pressure(Pressure, purge_expired)
FrameHandling-->>WireframeApp: (may sleep, purge)
WireframeApp->>MessageAssembly: process frame under budgets
end
Class diagram for derived memory budgets and inbound handlingclassDiagram
direction LR
class WireframeApp {
+process_stream(stream)
-Option~MemoryBudgets~ memory_budgets
-Option~FragmentationConfig~ fragmentation
}
class MemoryBudgets {
+BudgetBytes bytes_per_message()
+BudgetBytes bytes_per_connection()
+BudgetBytes bytes_in_flight()
+new(per_message, per_connection, in_flight) MemoryBudgets
}
class BudgetBytes {
+new(value) BudgetBytes
+as_usize() usize
}
class BuilderDefaults {
<<module>>
+default_fragmentation(frame_budget usize) Option~FragmentationConfig~
+default_memory_budgets(frame_budget usize) MemoryBudgets
-DEFAULT_MESSAGE_SIZE_MULTIPLIER usize
-DEFAULT_MESSAGE_BUDGET_MULTIPLIER usize
-DEFAULT_CONNECTION_BUDGET_MULTIPLIER usize
-DEFAULT_IN_FLIGHT_BUDGET_MULTIPLIER usize
}
class Backpressure {
<<module>>
+resolve_effective_budgets(explicit Option~MemoryBudgets~, frame_budget usize) MemoryBudgets
+evaluate_memory_pressure(message_assembly Option~MessageAssemblyState~, budgets Option~MemoryBudgets~) Pressure
+apply_memory_pressure(pressure Pressure, purge_fn)
-active_aggregate_limit_bytes(budgets MemoryBudgets) usize
}
class FrameHandlingMod {
<<module>>
+new_message_assembly_state(fragmentation Option~FragmentationConfig~, frame_budget usize, budgets Option~MemoryBudgets~) Option~MessageAssemblyState~
+evaluate_memory_pressure(message_assembly Option~MessageAssemblyState~, budgets Option~MemoryBudgets~) Pressure
+apply_memory_pressure(pressure Pressure, purge_fn)
+resolve_effective_budgets(explicit Option~MemoryBudgets~, frame_budget usize) MemoryBudgets
}
class MessageAssemblyState {
+with_budgets(fragmentation Option~FragmentationConfig~, frame_budget usize, budgets Option~MemoryBudgets~) MessageAssemblyState
+new(fragmentation Option~FragmentationConfig~, frame_budget usize) MessageAssemblyState
}
class FramePipeline {
+new(fragmentation Option~FragmentationConfig~) FramePipeline
}
WireframeApp --> FrameHandlingMod : uses
WireframeApp --> MemoryBudgets : holds Option
FrameHandlingMod --> Backpressure : reexports
FrameHandlingMod --> BuilderDefaults : uses via Backpressure
Backpressure --> MemoryBudgets : resolves
BuilderDefaults --> MemoryBudgets : constructs
MemoryBudgets --> BudgetBytes : composed_of
MessageAssemblyState --> MemoryBudgets : uses
FramePipeline --> MessageAssemblyState : coordinates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
WalkthroughSummarise the addition of derived default memory budgets computed from frame size when explicit Changes
Sequence DiagramsequenceDiagram
participant App as WireframeApp (Builder)
participant Handler as InboundHandler (process_stream)
participant Resolver as resolve_effective_budgets()
participant Memory as new_message_assembly_state()
participant Pressure as evaluate_memory_pressure()
App->>Handler: Frame received (requested_frame_length)
Handler->>Resolver: resolve_effective_budgets(explicit_budgets?, frame_length)
alt Explicit budgets set
Resolver-->>Handler: return explicit budgets
else No explicit budgets
Resolver->>Resolver: derive from frame_length via multipliers (16×, 64×, 64×)
Resolver-->>Handler: return derived budgets
end
Handler->>Memory: new_message_assembly_state(Some(effective_budgets))
Handler->>Pressure: evaluate_memory_pressure(Some(effective_budgets))
Pressure-->>Handler: MemoryPressureAction (BackOff | Abort)
Handler-->>App: ProcessingResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
builder_defaults.rs,DEFAULT_MESSAGE_BUDGET_MULTIPLIERis intended to stay aligned withDEFAULT_MESSAGE_SIZE_MULTIPLIER; consider deriving it directly from that constant or reusing the same value to avoid silent divergence if one is changed in future. - The derived-memory-budgets fixture reimplements fragmentation derivation (
buffer_capacity * 16and a 30s timeout); usingdefault_fragmentationor otherwise sharing the same helper as production code would reduce duplication and keep the test behaviour automatically aligned with the real defaults. - In
DerivedMemoryBudgetsWorld::start_app_derived/start_app_explicit, the fragmentation config uses the rawbuffer_capacitywhileWireframeApp::buffer_capacityappliesclamp_frame_length, so basing the fragmentation frame budget on the clamped value (e.g.app.length_codec().max_frame_length()) would prevent potential inconsistencies for extreme capacities.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `builder_defaults.rs`, `DEFAULT_MESSAGE_BUDGET_MULTIPLIER` is intended to stay aligned with `DEFAULT_MESSAGE_SIZE_MULTIPLIER`; consider deriving it directly from that constant or reusing the same value to avoid silent divergence if one is changed in future.
- The derived-memory-budgets fixture reimplements fragmentation derivation (`buffer_capacity * 16` and a 30s timeout); using `default_fragmentation` or otherwise sharing the same helper as production code would reduce duplication and keep the test behaviour automatically aligned with the real defaults.
- In `DerivedMemoryBudgetsWorld::start_app_derived`/`start_app_explicit`, the fragmentation config uses the raw `buffer_capacity` while `WireframeApp::buffer_capacity` applies `clamp_frame_length`, so basing the fragmentation frame budget on the clamped value (e.g. `app.length_codec().max_frame_length()`) would prevent potential inconsistencies for extreme capacities.
## Individual Comments
### Comment 1
<location path="src/app/builder_defaults.rs" line_range="42" />
<code_context>
+const DEFAULT_CONNECTION_BUDGET_MULTIPLIER: usize = 64;
+const DEFAULT_IN_FLIGHT_BUDGET_MULTIPLIER: usize = 64;
+
+pub(super) fn default_memory_budgets(frame_budget: usize) -> MemoryBudgets {
+ let frame_budget = clamp_frame_length(frame_budget);
+ let per_message = NonZeroUsize::new(
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated budget derivation logic into a helper and simplifying the test structure to reduce duplication and clarify intent.
You can reduce duplication and make the intent clearer both in `default_memory_budgets` and the tests without changing behavior.
### 1. Factor out the repeated NonZero budget pattern
The three `NonZeroUsize::new(...).unwrap_or(NonZeroUsize::MIN)` blocks are identical except for the multiplier. A tiny helper makes the policy (“derive a non-zero budget from frame budget and multiplier, saturating, defaulting to MIN”) explicit and centralized:
```rust
fn derive_budget(frame_budget: usize, multiplier: usize) -> BudgetBytes {
let bytes = NonZeroUsize::new(frame_budget.saturating_mul(multiplier))
.unwrap_or(NonZeroUsize::MIN);
BudgetBytes::new(bytes)
}
#[must_use]
pub(super) fn default_memory_budgets(frame_budget: usize) -> MemoryBudgets {
let frame_budget = clamp_frame_length(frame_budget);
MemoryBudgets::new(
derive_budget(frame_budget, DEFAULT_MESSAGE_BUDGET_MULTIPLIER),
derive_budget(frame_budget, DEFAULT_CONNECTION_BUDGET_MULTIPLIER),
derive_budget(frame_budget, DEFAULT_IN_FLIGHT_BUDGET_MULTIPLIER),
)
}
```
If there is a specific reason for the `unwrap_or(NonZeroUsize::MIN)` (e.g. overflow or a future change to `clamp_frame_length`), you can document it in `derive_budget` once instead of repeating it three times.
### 2. Simplify tests with `assert_eq!` and parameterization
The tests are currently very verbose, manually building `io::Error`s for simple equality checks. You can keep them as `rstest` tests but simplify to plain assertions and consolidate the repetitive structure:
```rust
#[cfg(test)]
mod tests {
use super::*;
use rstest::rstest;
#[rstest]
#[case(1024, 16_384, 65_536, 65_536)]
#[case(4096, 4096 * 16, 4096 * 64, 4096 * 64)]
fn default_budgets_scale_and_use_expected_multipliers(
#[case] frame_budget: usize,
#[case] expected_message: usize,
#[case] expected_connection: usize,
#[case] expected_in_flight: usize,
) {
let budgets = default_memory_budgets(frame_budget);
assert_eq!(expected_message, budgets.bytes_per_message().as_usize());
assert_eq!(expected_connection, budgets.bytes_per_connection().as_usize());
assert_eq!(expected_in_flight, budgets.bytes_in_flight().as_usize());
}
#[test]
fn default_budgets_clamp_minimum_frame_budget() {
let budgets = default_memory_budgets(10);
assert_eq!(64 * 16, budgets.bytes_per_message().as_usize());
}
#[test]
fn default_budgets_clamp_maximum_frame_budget() {
let max_frame: usize = 16 * 1024 * 1024;
let budgets = default_memory_budgets(max_frame + 1);
assert_eq!(max_frame * 16, budgets.bytes_per_message().as_usize());
}
#[test]
fn default_budgets_message_budget_aligns_with_fragmentation() {
let frame_budget = 2048_usize;
let budgets = default_memory_budgets(frame_budget);
let expected = frame_budget * DEFAULT_MESSAGE_SIZE_MULTIPLIER;
assert_eq!(
expected,
budgets.bytes_per_message().as_usize(),
"per-message budget does not match fragmentation multiplier"
);
}
}
```
This keeps all coverage and behavior but removes the boilerplate `Result`/`io::Error` plumbing and repeated `if` patterns, making the tests shorter and easier to maintain.
</issue_to_address>
### Comment 2
<location path="docs/execplans/8-3-5-Define derived defaults based on buffer_capacity.md" line_range="68" />
<code_context>
+- `src/app/inbound_handler.rs` is currently 392 lines. Changes must keep it at
+ or below 400.
+- Use en-GB-oxendict spelling in all comments and documentation.
+- Follow the BDD 4-file pattern (feature, fixture, steps, scenarios) with
+ globally unique step text using a `derived-budget` prefix.
+- Validate with `rstest` unit tests and `rstest-bdd` v0.5.0 behavioural tests.
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym “BDD” is introduced here without being expanded on first use, which violates the requirement to define uncommon acronyms.
Please expand “BDD” on its first occurrence in this document (for example, “behaviour‑driven development (BDD)”), and then use the acronym thereafter.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 3
<location path="docs/execplans/8-3-5-Define derived defaults based on buffer_capacity.md" line_range="199" />
<code_context>
+ multiple concurrent assemblies without being so large as to be meaningless.
+ Setting `bytes_in_flight` equal to `bytes_per_connection` simplifies the
+ mental model. For the default 1024-byte frame, this yields: per_message = 16
+ KiB, per_connection = 64 KiB, in_flight = 64 KiB. Date/Author: 2026-02-26 /
+ plan phase.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** The unit acronym “KiB” is used without definition, which conflicts with the requirement to define uncommon acronyms on first use.
Consider spelling out the unit on its first use, for example “16 KiB (kibibytes)”, and then using “KiB” alone afterwards.
<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.
![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.
Reviewed commit: 2b29008d6b
ℹ️ 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 "@codex address that feedback".
…line tests - Introduced `derive_budget` helper function to clean up repeated budget calculations. - Simplified `default_memory_budgets` by using the new helper. - Rewrote tests to use parameterized `rstest` cases for coverage and clarity. - Removed redundant error handling in tests by using assertions. - Improved test fixture alignment with production code by deriving fragmentation config from app instance. - Enhanced `assert_no_connection_error` to more reliably detect server errors by awaiting server shutdown. - Minor doc improvements and cleanup in test fixture code. These changes improve code clarity, reduce duplication, and increase test robustness without changing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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/builder_defaults.rs`:
- Around line 95-97: Replace the hard-coded literal 16 * 1024 * 1024 used to
build max_frame with the canonical codec max-frame constant (e.g.,
codec::MAX_FRAME or crate::codec::MAX_FRAME), then call
default_memory_budgets(max_frame + 1) and keep the assert using that constant
(assert_eq!(max_frame * 16, budgets.bytes_per_message().as_usize())); add an
import/path for the codec constant if needed so the test no longer duplicates
the numeric limit.
In `@tests/fixtures/derived_memory_budgets.rs`:
- Around line 271-283: The test helpers assert_connection_aborted and the second
assertion method currently await the server JoinHandle without a timeout,
risking hangs; wrap the server await (the JoinHandle taken from self.server) in
tokio::time::timeout(Duration::from_secs(2), server) inside the block_on call,
then adjust pattern matching for the nested Result/JoinError (e.g.
Ok(Ok(Ok(()))), Ok(Ok(Err(e))), Ok(Err(join_err))) and add an Err(_) branch that
returns a timed-out TestResult error message like "timed out waiting for server
task ...". Ensure you import Duration and tokio::time::timeout and replace
direct self.block_on(server)? calls with self.block_on(async { timeout(...,
server).await })? in both assert_connection_aborted and the other assertion
method.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
docs/adr-002-streaming-requests-and-shared-message-assembly.mddocs/execplans/8-3-5-Define derived defaults based on buffer_capacity.mddocs/roadmap.mddocs/users-guide.mdsrc/app/builder_defaults.rssrc/app/frame_handling/backpressure.rssrc/app/frame_handling/backpressure_tests.rssrc/app/frame_handling/mod.rssrc/app/inbound_handler.rstests/features/derived_memory_budgets.featuretests/fixtures/derived_memory_budgets.rstests/fixtures/mod.rstests/scenarios/derived_memory_budgets_scenarios.rstests/scenarios/mod.rstests/steps/derived_memory_budgets_steps.rstests/steps/mod.rs
Introduce a SERVER_JOIN_TIMEOUT constant and a join_server method that joins the server task with a bounded timeout. Refactor test helper code in derived_memory_budgets.rs to use this method, replacing manual blocking and result matching. Also update builder_defaults.rs tests to use codec constants for frame length limits. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/builder_defaults.rs`:
- Around line 71-73: The test default_budgets_scale_and_use_expected_multipliers
uses raw magic numbers for multipliers and misses asserting combined derived
fields at clamp boundaries; update the test to replace literal 16 and 64 with
the multiplier constants (e.g., BYTES_PER_CONNECTION_MULTIPLIER,
BYTES_IN_FLIGHT_MULTIPLIER or their defined names) and add assertions that check
both bytes_per_connection and bytes_in_flight individually and their sum
(bytes_per_connection + bytes_in_flight) for the min and max clamp cases so the
test verifies full derived-default coverage and prevents divergence from the
policy constants.
In `@tests/fixtures/derived_memory_budgets.rs`:
- Around line 17-23: Add Rustdoc comments for the public fields on
ExplicitBudgetConfig: add a leading `///` doc comment above `per_message`
describing its purpose (per-message budget), above `per_connection` explaining
the per-connection budget, and above `in_flight` explaining the in-flight/global
budget so the public fixture API is self-describing and cargo doc friendly;
update the struct fields `per_message`, `per_connection`, and `in_flight` with
concise Rustdoc lines.
- Around line 243-250: The send_first_frames_for_range function currently
swallows errors by breaking the loop and always returning Ok(()); change it to
propagate the first encountered error (from send_first_frame or spin_runtime) by
returning that TestResult error immediately instead of breaking—i.e., when
self.send_first_frame(key, body) returns Err(e) return Err(e) (or map/propagate
the error), and likewise propagate any Err from self.spin_runtime(); keep the
TestResult return type and ensure the function returns Ok(()) only when the
entire range completes successfully.
…d fixture robustness - Refine default memory budgets tests to use multipliers for message, connection, and in-flight budgets instead of hardcoded values. - Add checks for all related budget multipliers when clamping minimum and maximum frame budgets. - Improve DerivedMemoryBudgetsWorld fixture by documenting send_first_frames_for_range behavior and tolerating intermediate frame send failures except for the initial frame. - Add comments describing fields in ExplicitBudgetConfig struct. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/derived_memory_budgets.rs`:
- Around line 154-158: Add a short inline comment above the hardcoded 16 in
fragmentation_from_app explaining that the multiplier mirrors
DEFAULT_MESSAGE_SIZE_MULTIPLIER (from builder_defaults) used elsewhere, so
future maintainers know this value is intentionally chosen to match that
module-private constant and should be updated in tandem if the default
multiplier changes; mention the function name fragmentation_from_app and the
constant DEFAULT_MESSAGE_SIZE_MULTIPLIER to make the intent explicit.
… calculation Added a comment in `fragmentation_from_app` clarifying that the multiplier 16 mirrors the `DEFAULT_MESSAGE_SIZE_MULTIPLIER` constant in `builder_defaults`. This helps maintain consistency if the constant changes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements 8.3.5: derive memory budgets for connection budgets from buffer_capacity when explicit budgets are not provided. Budgets are computed lazily at connection time and wired through the inbound read path, with explicit budgets taking precedence. Adds unit tests, behavioural tests, and documentation updates to reflect the new derived-defaults behavior.
Changes
Testing plan
Acceptance criteria
Notes
📎 Task: https://www.devboxer.com/task/e2e75d74-42b5-4427-8981-65224a8a654a