Implement soft-limit memory back-pressure for inbound reads (8.3.3)#478
Merged
Implement soft-limit memory back-pressure for inbound reads (8.3.3)#478
Conversation
Implemented roadmap item 8.3.3 to apply back-pressure by pausing inbound socket reads under configured per-connection memory budgets before hitting hard caps. Introduced a new internal helper module in frame_handling to decide paused reads based on current buffered bytes and aggregate caps. Modified the inbound read loop to consult this policy and asynchronously pause when soft pressure is active, ensuring paced reads for memory pressure without regressions to existing hard-cap enforcement (8.3.2). Added rstest unit tests for policy and integration points, plus rstest-bdd behavioural scenarios to validate pause and resume semantics with deterministic virtual time. Updated documentation, decision logs, and roadmap to reflect implementation. No public API changes or new dependencies. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements soft-limit memory back-pressure for inbound reads by introducing a dedicated backpressure helper module, wiring it into the inbound read loop, and adding focused unit/BDD tests plus documentation updates while fixing a few pre-existing lint issues in tests. Sequence diagram for inbound read loop with soft-limit back-pressuresequenceDiagram
actor Client
participant InboundHandler
participant FrameHandling
participant MessageAssemblyState
participant FramedTransport
Client->>InboundHandler: open_connection
loop process_stream_loop
InboundHandler->>MessageAssemblyState: total_buffered_bytes()
InboundHandler->>FrameHandling: should_pause_inbound_reads(state, memory_budgets)
alt soft pressure active
InboundHandler->>FrameHandling: soft_limit_pause_duration()
InboundHandler->>InboundHandler: sleep(pause_duration)
InboundHandler->>InboundHandler: purge_expired(pipeline, message_assembly)
end
InboundHandler->>FramedTransport: timeout(read_timeout, next())
FramedTransport-->>InboundHandler: frame_result
alt frame ok
InboundHandler->>InboundHandler: handle_frame(frame, pipeline, message_assembly)
else error or timeout
InboundHandler->>InboundHandler: existing error and shutdown handling
end
end
Updated class diagram for soft-limit backpressure helpers and dependenciesclassDiagram
class Backpressure {
<<module>>
+should_pause_inbound_reads(state: Option_MessageAssemblyState_ref, budgets: Option_MemoryBudgets_) bool
+soft_limit_pause_duration() Duration
-SOFT_LIMIT_NUMERATOR u128
-SOFT_LIMIT_DENOMINATOR u128
-SOFT_LIMIT_PAUSE_DURATION Duration
-active_aggregate_limit_bytes(budgets: MemoryBudgets) usize
-is_at_or_above_soft_limit(buffered_bytes: usize, aggregate_limit: usize) bool
}
class MessageAssemblyState {
+total_buffered_bytes() usize
}
class MemoryBudgets {
+bytes_per_connection() BudgetBytes
+bytes_in_flight() BudgetBytes
}
class BudgetBytes {
+as_usize() usize
}
class InboundHandler {
-memory_budgets MemoryBudgets_optional
+process_stream()
}
Backpressure ..> MessageAssemblyState : reads_buffered_bytes
Backpressure ..> MemoryBudgets : uses_budgets
MemoryBudgets ..> BudgetBytes : returns
InboundHandler ..> Backpressure : calls
InboundHandler ..> MessageAssemblyState : owns_reference
InboundHandler ..> MemoryBudgets : owns_optional
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
WalkthroughImplement soft-limit back-pressure for inbound reads: add a crate-scoped backpressure helper that signals brief per-iteration pauses when buffered assembly bytes reach 80% of the smaller aggregate budget; integrate it into the inbound read loop to sleep and purge expired assemblies before polling the next frame. Changes
Sequence Diagram(s)sequenceDiagram
participant Inbound as Inbound Read Loop
participant BP as Backpressure Policy
participant State as Message Assembly State
participant Budgets as Memory Budgets
participant Reader as Framed Socket
Inbound->>BP: should_pause_inbound_reads(state, budgets)
BP->>State: read buffered bytes
BP->>Budgets: compute aggregate limit (min per_connection, in_flight)
BP->>BP: compare buffered bytes to 80% threshold
alt At or above soft limit
BP-->>Inbound: true (pause)
Inbound->>Inbound: sleep (soft_limit_pause_duration)
Inbound->>State: purge expired assemblies
Inbound->>Inbound: continue loop
else Below soft limit
BP-->>Inbound: false (read normally)
Inbound->>Reader: poll next frame (timeout)
Reader-->>Inbound: frame or timeout
end
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
Implemented roadmap item 8.3.3 to apply soft-limit back-pressure by pacing inbound reads when buffered assembly bytes approach memory budget thresholds. Added back-pressure helpers in src/app/frame_handling/backpressure.rs, integrated paced pauses in the inbound read loop, and provided comprehensive tests and rstest-bdd coverage. Updated related documentation and completed all quality gates. - Added backpressure.rs with soft-limit detection and pacing duration - Integrated soft-limit pauses before polling additional frames - Added unit tests and behavioral tests for memory budget back-pressure - Updated ADR, exec plans, user guide, and roadmap docs - Fixed lint and unused_must_use issues in test fixtures This change allows the system to propagate back-pressure without stalling in-flight assemblies, improving flow control under memory pressure. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/fixtures/memory_budget_backpressure.rs" line_range="32-35" />
<code_context>
+ pub in_flight: usize,
+}
+
+impl FromStr for BackpressureConfig {
+ type Err = String;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ let mut values = s.split('/').map(str::trim);
+ let timeout_ms = values.next().ok_or("missing timeout_ms")?;
</code_context>
<issue_to_address>
**suggestion (testing):** BackpressureConfig::from_str lacks direct tests, which could make BDD failures hard to diagnose when feature strings are malformed.
Since these strings are human-authored (e.g. `"timeout_ms/per_message/per_connection/in_flight"`), mistakes or format changes will surface only as opaque BDD failures. Please add focused unit tests for `BackpressureConfig::from_str` in this fixture, including:
- A valid example matching the `.feature` file, asserting all fields are parsed correctly.
- A few invalid cases (e.g. missing fields, non-numeric segments) asserting that parsing fails with a clear error message.
This will better specify parsing behaviour and make regressions or feature-file mistakes easier to diagnose.
Suggested implementation:
```rust
pub in_flight: usize,
}
```
To implement the requested tests for `BackpressureConfig::from_str`, please add the following test module near the end of `tests/fixtures/memory_budget_backpressure.rs` (at module scope, not inside any function/impl):
```rust
#[cfg(test)]
mod backpressure_config_from_str_tests {
use super::*;
use std::str::FromStr;
#[test]
fn parses_valid_backpressure_config() {
// Match the shape used in the `.feature` file:
// "timeout_ms/per_message/per_connection/in_flight"
let config = BackpressureConfig::from_str("1000/10/20/30").expect("parse should succeed");
assert_eq!(config.timeout_ms, 1000);
assert_eq!(config.per_message, 10);
assert_eq!(config.per_connection, 20);
assert_eq!(config.in_flight, 30);
}
#[test]
fn fails_when_missing_fields() {
// Only three segments instead of four
let error = BackpressureConfig::from_str("1000/10/20")
.expect_err("parse should fail on missing in_flight");
assert_eq!(error, "missing in_flight");
// Only one segment
let error = BackpressureConfig::from_str("1000")
.expect_err("parse should fail on missing per_message");
assert_eq!(error, "missing per_message");
// Empty string
let error = BackpressureConfig::from_str("")
.expect_err("parse should fail on missing timeout_ms");
assert_eq!(error, "missing timeout_ms");
}
#[test]
fn fails_with_clear_error_on_non_numeric_segments() {
let error = BackpressureConfig::from_str("not-a-number/10/20/30")
.expect_err("parse should fail on non-numeric timeout_ms");
assert!(
error.starts_with("timeout_ms:"),
"unexpected error message: {error}"
);
let error = BackpressureConfig::from_str("1000/not-a-number/20/30")
.expect_err("parse should fail on non-numeric per_message");
assert!(
error.starts_with("per_message:"),
"unexpected error message: {error}"
);
let error = BackpressureConfig::from_str("1000/10/not-a-number/30")
.expect_err("parse should fail on non-numeric per_connection");
assert!(
error.starts_with("per_connection:"),
"unexpected error message: {error}"
);
let error = BackpressureConfig::from_str("1000/10/20/not-a-number")
.expect_err("parse should fail on non-numeric in_flight");
assert!(
error.starts_with("in_flight:"),
"unexpected error message: {error}"
);
}
}
```
These tests:
1. Verify a valid configuration string matching the `"timeout_ms/per_message/per_connection/in_flight"` pattern parses correctly and populates all fields.
2. Cover several missing-field cases, asserting that the user sees the specific `"missing <field>"` error string that `from_str` currently returns.
3. Check non-numeric segments for each field, asserting that the error messages are clearly attributed to the failing field by their prefixes (e.g. `"timeout_ms:"`, `"per_message:"`, etc.).
If the fixture already has an existing `#[cfg(test)] mod tests` module, you can either:
- Move these tests into that module, or
- Merge the imports (`use super::*; use std::str::FromStr;`) and keep a single test module in the file.
</issue_to_address>
### Comment 2
<location path="docs/execplans/8-3-3-soft-limit-behaviour.md" line_range="60" />
<code_context>
+
+## Tolerances (exception triggers)
+
+- Scope: if implementation requires changes to more than 14 files or more than
+ 700 net LOC, stop and escalate.
+- Interface: if implementing `8.3.3` requires a new public builder method,
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “LOC” is used later in this bullet without being expanded on first use, which violates the requirement to define uncommon acronyms.
Consider expanding the acronym on first use, for example:
"Scope: if implementation requires changes to more than 14 files or more than 700 lines of code (LOC), stop and escalate."
<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.
![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: 5
🤖 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/adr-002-streaming-requests-and-shared-message-assembly.md`:
- Around line 321-332: Update the “Soft budget pressure (back-pressure)” section
text to reflect the implemented paced pausing behavior: describe that soft-limit
back-pressure in inbound read handling (inbound_handler.rs) uses short
per-iteration pauses (implemented in frame_handling/backpressure.rs) when
buffered assembly bytes exceed the soft threshold (80% of
min(bytes_per_connection, bytes_in_flight)), rather than an indefinite read
stop; mention that pacing is a temporary pause allowing in-flight assemblies to
make progress and preserve the aggregate cap logic and threshold semantics used
in the codebase.
In `@docs/execplans/8-3-3-soft-limit-behaviour.md`:
- Around line 13-16: Reflow the markdown in the ExecPlan section describing
Roadmap item `8.3.3` so all paragraphs and bullet points are wrapped to 80
columns; locate the opening paragraph that mentions `8.3.3` and `Wireframe` and
rewrap its lines and the following bullets to the 80-column limit, preserving
sentence boundaries and emphasis/markdown, ensuring lists keep a single blank
line before and after; apply the same 80-column wrapping to the rest of this
ExecPlan document so all prose conforms to the project’s markdown wrapping
guideline.
In `@src/app/frame_handling/backpressure_tests.rs`:
- Around line 19-56: The tests currently mask invalid zero inputs by falling
back to NonZeroUsize::MIN; replace those silent fallbacks with explicit expects
so tests fail fast. Update the three NonZeroUsize::new(...) calls in the
budgets() fixture (used in MemoryBudgets::new) to use .expect("budget bytes must
be non-zero in test"), change the NonZeroUsize::new(...) that constructs max in
state_with_buffered_bytes to .expect("buffered_bytes+64 must be non-zero in
test"), and likewise replace the other occurrences referenced (around the other
test helper at lines noted in the review) with .expect(...) and clear messages
so any zero input surfaces as a test failure.
In `@tests/fixtures/memory_budget_backpressure.rs`:
- Around line 108-112: Add Rustdoc comments for the public fixture function
memory_budget_backpressure_world and for all public methods on the
MemoryBudgetBackpressureWorld type: describe what the fixture returns, the
intended test scenarios/use-cases, and any important behaviors or invariants
callers should know; place triple-slash (///) comments immediately above the pub
fn memory_budget_backpressure_world declaration and above each pub impl method
on MemoryBudgetBackpressureWorld so cargo doc will include clear documentation
for these public APIs.
- Around line 22-55: The FromStr implementation for BackpressureConfig currently
ignores any trailing segments after parsing in_flight; update the from_str
method (for BackpressureConfig / impl FromStr::from_str) to check that
values.next() returns None after extracting in_flight and return an Err (e.g.,
"surplus segments") if any extra token exists, so surplus config segments are
rejected rather than silently ignored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
docs/adr-002-streaming-requests-and-shared-message-assembly.mddocs/execplans/8-3-3-soft-limit-behaviour.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/roadmap.mddocs/users-guide.mdsrc/app/frame_handling/backpressure.rssrc/app/frame_handling/backpressure_tests.rssrc/app/frame_handling/mod.rssrc/app/inbound_handler.rstests/features/memory_budget_backpressure.featuretests/fixtures/interleaved_push_queues.rstests/fixtures/memory_budget_backpressure.rstests/fixtures/mod.rstests/interleaved_push_queues.rstests/scenarios/memory_budget_backpressure_scenarios.rstests/scenarios/mod.rstests/steps/memory_budget_backpressure_steps.rstests/steps/mod.rs
…g and testing - Enhance BackpressureConfig::from_str to reject empty and trailing segments with clear errors. - Add extensive unit tests validating parsing errors and correct config processing. - Refactor test fixtures with clearer NonZeroUsize construction and panics. - Add new test helper methods and more descriptive documentation in backpressure BDD tests. - Minor doc fixes in soft limit behavior and back-pressure ADR. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
docs/execplans/8-3-3-soft-limit-behaviour.md (1)
29-29:⚠️ Potential issue | 🟡 MinorRewrap lines that exceed 80 columns.
Line 29 (
- behaviour is covered by...) and line 85 (- Risk: full read suspension can deadlock...) exceed the 80-column limit. Reflow these bullets to stay within bounds.Suggested reflow
-- behaviour is covered by `rstest-bdd` (`0.5.0`) scenarios using a live inbound - runtime fixture; +- behaviour is covered by `rstest-bdd` (`0.5.0`) scenarios using a + live inbound runtime fixture;-- Risk: full read suspension can deadlock in buffered multi-frame assembly if no - bytes can be reclaimed without reading additional frames. Severity: high +- Risk: full read suspension can deadlock in buffered multi-frame + assembly if no bytes can be reclaimed without reading additional + frames. Severity: highAs per coding guidelines, "Markdown paragraphs and bullet points must be wrapped at 80 columns."
Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/8-3-3-soft-limit-behaviour.md` at line 29, Rewrap the markdown bullets that exceed 80 columns: split the line starting with "- behaviour is covered by `rstest-bdd` (`0.5.0`) scenarios using a live inbound" into multiple wrapped lines under 80 chars (preserve the backticked package/version text and sentence meaning), and likewise reflow the bullet beginning "- Risk: full read suspension can deadlock..." so each wrapped line is ≤80 columns and retains the original wording; update only those two bullets in the document.
🤖 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-3-soft-limit-behaviour.md`:
- Around line 326-350: The indented shell command blocks (e.g., the sequences
starting with "set -o pipefail", "cargo test --lib frame_handling", "cargo test
--lib inbound_handler", "cargo test --test bdd --all-features
memory_budget_backpressure", and the "make fmt/make lint/make test" block) must
be converted to fenced code blocks with a language identifier; replace each
indented block with triple-backtick fenced blocks using ```shell (or ```bash) so
the snippets carry a language identifier for syntax highlighting and linting
tools and follow the repository guideline to always provide a language
identifier for fenced code blocks.
- Line 320: Replace the hardcoded path string "/home/user/project" with a
generic or relative reference; locate the exact occurrence of
"/home/user/project" in the text and change it to something like "Run from the
repository root (e.g., .)" or "Run from the repository root (<repository-root>)"
so the instruction is portable for all contributors.
In `@src/app/frame_handling/backpressure_tests.rs`:
- Around line 19-61: The tests use manual match/else panics for fallible setup
in fixtures/helpers (e.g., budgets() with NonZeroUsize::new,
state_with_buffered_bytes with NonZeroUsize::new, FirstFrameInput::new, and
state.accept_first_frame) — replace those explicit match/else blocks with
.expect(...) calls that include a clear message (for example "creating
NonZeroUsize for per_message", "creating first frame input", "accepting first
frame") to reduce boilerplate and provide better diagnostics; update every
occurrence in budgets(), state_with_buffered_bytes(), and the noted
FirstFrameInput::new / state.accept_first_frame usages (and the similar blocks
around lines 100–113) to use .expect(...) instead of manual panics.
- Around line 80-96: Combine the three duplicate tests into a single
parameterised rstest: replace the functions should_not_pause_below_soft_limit,
should_pause_at_soft_limit, and should_pause_above_soft_limit with one #[rstest]
that iterates over cases of buffered bytes (e.g., 79, 80, 95) and expected
boolean results; for each case call state_with_buffered_bytes(...) and assert
that should_pause_inbound_reads(Some(&state), Some(budgets)) equals the expected
value, keeping MemoryBudgets as the fixture parameter so test wiring remains the
same.
In `@tests/fixtures/memory_budget_backpressure.rs`:
- Around line 274-301: Add a one-line inline comment immediately above the
self.client.take() / self.client = Some(client) pattern in send_payload
explaining that the temporary take is required to move the Framed (client) into
the async block (block_on) without splitting the &mut self borrow; mention that
this allows sending/flushing the frame inside the async closure and then
restoring the client to self.
- Around line 328-338: Replace the verbose "let Ok(config) else { panic!(...)}"
in the parses_valid_backpressure_config test with a direct .expect(...) call on
BackpressureConfig::from_str so the test reads (conceptually) that
BackpressureConfig::from_str("1000/10/20/30").expect("valid config should
parse") and use .expect_err(...) on tests that assert parsing fails; update
references to the parsed value accordingly in assertions to follow the project's
testing convention.
- Around line 84-110: The Default impl for MemoryBudgetBackpressureWorld repeats
identical field initializations in both match arms; refactor by creating a
single base struct (or helper like MemoryBudgetBackpressureWorld::with_runtime)
and use struct update syntax to set the differing fields runtime and
runtime_error based on
tokio::runtime::Builder::new_current_thread().enable_all().build() result, so
the shared fields (client, server, observed_rx, observed_payloads,
last_send_error) are only initialized once and the Ok/Err arms set only runtime
and runtime_error.
- Around line 1-401: The file exceeds 400 lines because the test module
backpressure_config_from_str_tests (the #[cfg(test)] mod) is included in the
same file; move that entire test module into a new sibling file named
tests/fixtures/memory_budget_backpressure_tests.rs (keeping the same module
contents and imports) so the fixture file drops below the 400-line limit, and in
the original file remove the #[cfg(test)] mod block; also remove the
#[rustfmt::skip] attribute from the memory_budget_backpressure_world() fixture
function so it is simply #[fixture] pub fn memory_budget_backpressure_world() ->
MemoryBudgetBackpressureWorld { ... } (preserve the function name and
signature).
---
Duplicate comments:
In `@docs/execplans/8-3-3-soft-limit-behaviour.md`:
- Line 29: Rewrap the markdown bullets that exceed 80 columns: split the line
starting with "- behaviour is covered by `rstest-bdd` (`0.5.0`) scenarios using
a live inbound" into multiple wrapped lines under 80 chars (preserve the
backticked package/version text and sentence meaning), and likewise reflow the
bullet beginning "- Risk: full read suspension can deadlock..." so each wrapped
line is ≤80 columns and retains the original wording; update only those two
bullets in the document.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/adr-002-streaming-requests-and-shared-message-assembly.mddocs/execplans/8-3-3-soft-limit-behaviour.mdsrc/app/frame_handling/backpressure_tests.rstests/fixtures/memory_budget_backpressure.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2908 🏁 Script executed: Length of output: 362
Comment 1 (BackpressureConfig::from_str tests): The file
Comment 2 (LOC acronym expansion): The file Both issues have been fully addressed. |
…g parsing tests - Added unit tests for BackpressureConfig::from_str covering valid input parsing. - Added tests for missing fields and non-numeric segment errors. - Added test for unexpected trailing segments ensuring error correctness. - Implemented tests as new file `memory_budget_backpressure_tests.rs` and integrated in fixtures mod. This improves coverage and validation of backpressure configuration parsing logic. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
… calls
Replaced multiple uses of pattern matching with `let Some(..) else { panic! }` by concise `.expect(..)` calls in backpressure_tests.rs. This improves code readability and aligns with the added Clippy lint expectation for use of `expect` in test setup for concise failure diagnostics.
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/execplans/8-3-3-soft-limit-behaviour.md (1)
13-35:⚠️ Potential issue | 🟡 MinorWrap Markdown paragraphs and bullets to 80 columns.
Reflow the long lines in this section to the 80‑column limit.✂️ Example reflow
-Roadmap item `8.3.3` requires soft-limit behaviour for per-connection memory -budgets: when inbound buffered bytes approach configured aggregate caps, -Wireframe should apply back-pressure by pausing socket reads rather than -immediately consuming the next frame. +Roadmap item `8.3.3` requires soft-limit behaviour for per-connection memory +budgets: when inbound buffered bytes approach configured aggregate +caps, Wireframe should apply back-pressure by pausing socket reads rather +than immediately consuming the next frame.As per coding guidelines, "Markdown paragraphs and bullet points must be wrapped at 80 columns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/8-3-3-soft-limit-behaviour.md` around lines 13 - 35, The Markdown in docs/execplans/8-3-3-soft-limit-behaviour.md contains lines longer than 80 columns; reflow every paragraph and each bullet so no line exceeds 80 characters while preserving inline code/backticks (e.g., WireframeApp::memory_budgets(...)), backquoted identifiers (rstest, rstest-bdd), headings, lists, and referenced filenames (docs/adr-002-streaming-requests-and-shared-message-assembly.md, docs/users-guide.md, docs/roadmap.md); ensure list item hyphens remain at line start and that wrapped lines keep sentence/word boundaries (not breaking code tokens) so the content and links remain semantically identical.
🤖 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/frame_handling/backpressure_tests.rs`:
- Around line 2-5: Remove the module-level clippy suppression
`#![expect(clippy::expect_used, ...)]` and refactor any test helpers and test
functions in this module to return Result types instead of calling
`.expect(...)`: change test signatures to return Result<(), Box<dyn
std::error::Error>> (or a concrete error type), replace `.expect(...)` calls
inside fixtures/helpers with `?` or `ok_or("context")?` and propagate errors,
and update any callers to handle the Result; ensure all `.expect` occurrences in
this file are eliminated and the attribute line itself is deleted.
---
Duplicate comments:
In `@docs/execplans/8-3-3-soft-limit-behaviour.md`:
- Around line 13-35: The Markdown in
docs/execplans/8-3-3-soft-limit-behaviour.md contains lines longer than 80
columns; reflow every paragraph and each bullet so no line exceeds 80 characters
while preserving inline code/backticks (e.g.,
WireframeApp::memory_budgets(...)), backquoted identifiers (rstest, rstest-bdd),
headings, lists, and referenced filenames
(docs/adr-002-streaming-requests-and-shared-message-assembly.md,
docs/users-guide.md, docs/roadmap.md); ensure list item hyphens remain at line
start and that wrapped lines keep sentence/word boundaries (not breaking code
tokens) so the content and links remain semantically identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/execplans/8-3-3-soft-limit-behaviour.mdsrc/app/frame_handling/backpressure_tests.rstests/fixtures/memory_budget_backpressure.rstests/fixtures/memory_budget_backpressure_tests.rstests/fixtures/mod.rs
…rror handling Refactored backpressure test functions to return Result and use error propagation instead of unwraps and expects. This improves error diagnostic clarity, avoiding panics and employing error handling idioms consistently throughout tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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 `@src/app/frame_handling/backpressure_tests.rs`:
- Around line 90-92: Replace the backslash-continued string in the
Err(io::Error::other(format!(...))) call with concat!: remove the trailing
backslash and join the two string literals using concat!(), then call format! on
that concat! result so the variables buffered_bytes, should_pause and actual are
still formatted into the message; update the expression in the same return site
in backpressure_tests.rs where the soft limit mismatch error is created.
Comment on lines
+90
to
+92
| return Err(io::Error::other(format!( | ||
| "soft limit mismatch: buffered_bytes={buffered_bytes}, expected={should_pause}, \ | ||
| actual={actual}" |
Contributor
There was a problem hiding this comment.
Replace the backslash-continued format string with concat!().
Remove the line-continuation backslash and join the literal with concat!() to meet the long-string rule.
Proposed change
- return Err(io::Error::other(format!(
- "soft limit mismatch: buffered_bytes={buffered_bytes}, expected={should_pause}, \
- actual={actual}"
- ))
+ return Err(io::Error::other(format!(
+ concat!(
+ "soft limit mismatch: buffered_bytes={buffered_bytes}, expected={should_pause}, ",
+ "actual={actual}"
+ )
+ ))
.into());As per coding guidelines, "Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/frame_handling/backpressure_tests.rs` around lines 90 - 92, Replace
the backslash-continued string in the Err(io::Error::other(format!(...))) call
with concat!: remove the trailing backslash and join the two string literals
using concat!(), then call format! on that concat! result so the variables
buffered_bytes, should_pause and actual are still formatted into the message;
update the expression in the same return site in backpressure_tests.rs where the
soft limit mismatch error is created.
Owner
Author
There was a problem hiding this comment.
@coderabbitai please confirm that concat!(...) will work in this context.
Contributor
There was a problem hiding this comment.
@leynos, I can confirm that concat!() can technically work in this context (it's not a proc macro attribute position), but the bot's specific suggestion is incorrect and won't compile.
Here's why the suggested code is wrong:
The format!() macro requires a format string as its first argument, which must be a string literal, and concat!() yields a &'static str by concatenating literals at compile time. However, the bot's suggestion places the named format placeholders {buffered_bytes}, {should_pause}, and {actual} inside the concat!() call without passing the actual variables as arguments to format!().
The correct approach with concat!() would be:
return Err(io::Error::other(format!(
concat!(
"soft limit mismatch: buffered_bytes={}, expected={}, ",
"actual={}"
),
buffered_bytes, should_pause, actual
)).into());Note the changes:
- Named placeholders
{buffered_bytes}→ positional placeholders{} - Variables passed as additional arguments to
format!()
However, I should note:
- Your current backslash-continuation approach is valid Rust and works correctly
- I don't see a specific guideline in your provided coding guidelines that mandates
concat!()over backslash continuation for format strings - The learnings about avoiding
concat!()in proc macro attributes (like#[when(...)]) don't apply here—this is a regular function body
Unless there's a specific style preference for your project, the current code is fine as-is. Would you like me to add a learning that backslash continuation is acceptable for format strings in regular code?
🧠 Learnings used
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Learnt from: leynos
Repo: leynos/wireframe PR: 455
File: src/app_data_store.rs:138-164
Timestamp: 2026-02-16T21:11:35.392Z
Learning: In Rust test modules using rstest fixtures, when a #[fixture] proc-macro triggers lints like unused_braces, place #[expect(lint)] at the module level instead of on the fixture function. Item-level attributes are consumed by the proc-macro before Clippy evaluates them, leading to unfulfilled_lint_expectations errors. Use module-level scoping as the narrowest viable option. For the reason string in the attribute, use a backslash-continued multi-line string literal (not concat!()) since attribute meta items require string literals.
Learnt from: leynos
Repo: leynos/wireframe PR: 458
File: tests/steps/message_assembly_inbound_steps.rs:24-27
Timestamp: 2026-02-17T00:28:31.370Z
Learning: In Rust proc macro attribute positions (e.g., #[when(...)], #[given(...)], #[then(...)] from rstest_bdd_macros), use backslash line continuations for long string literals instead of concat!(). Proc macros process token streams before macro expansion, so they require actual string literal tokens. Using concat!() in these positions can cause compilation errors such as "expected 'result' or 'value'" and may lead to runtime errors like "No matching step definition found". Apply this guidance to Rust test/step files broadly (e.g., all .rs files or tests/**/*.rs as appropriate) when defining macro-based steps.
Learnt from: leynos
Repo: leynos/wireframe PR: 458
File: tests/steps/message_assembly_inbound_steps.rs:24-27
Timestamp: 2026-02-17T00:28:51.250Z
Learning: In Rust, concat!() cannot be used in procedural macro attribute positions (e.g., #[when(...)], #[given(...)], #[then(...)]), because proc macros receive tokens before macro expansion. For long string literals in such attributes, use backslash-escaped newlines as the correct and only approach.
Owner
Author
There was a problem hiding this comment.
Please provide an AI agent prompt for the correct approach.
Contributor
There was a problem hiding this comment.
@leynos, here's the corrected AI agent prompt:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/frame_handling/backpressure_tests.rs` around lines 90-92, replace
the backslash-continued format string with concat!(): change the named format
placeholders {buffered_bytes}, {should_pause}, and {actual} to positional
placeholders {}, then wrap the string literals in concat!() and pass the
variables buffered_bytes, should_pause, and actual as additional arguments to
format!() after the concat!() expression.
The transformation should be:
FROM:
"soft limit mismatch: buffered_bytes={buffered_bytes}, expected={should_pause}, \
actual={actual}"
TO:
concat!(
"soft limit mismatch: buffered_bytes={}, expected={}, ",
"actual={}"
),
buffered_bytes, should_pause, actual
🧠 Learnings used
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Learnt from: leynos
Repo: leynos/wireframe PR: 455
File: src/app_data_store.rs:138-164
Timestamp: 2026-02-16T21:11:35.392Z
Learning: In Rust test modules using rstest fixtures, when a #[fixture] proc-macro triggers lints like unused_braces, place #[expect(lint)] at the module level instead of on the fixture function. Item-level attributes are consumed by the proc-macro before Clippy evaluates them, leading to unfulfilled_lint_expectations errors. Use module-level scoping as the narrowest viable option. For the reason string in the attribute, use a backslash-continued multi-line string literal (not concat!()) since attribute meta items require string literals.
Learnt from: leynos
Repo: leynos/wireframe PR: 458
File: tests/steps/message_assembly_inbound_steps.rs:24-27
Timestamp: 2026-02-17T00:28:31.370Z
Learning: In Rust proc macro attribute positions (e.g., #[when(...)], #[given(...)], #[then(...)] from rstest_bdd_macros), use backslash line continuations for long string literals instead of concat!(). Proc macros process token streams before macro expansion, so they require actual string literal tokens. Using concat!() in these positions can cause compilation errors such as "expected 'result' or 'value'" and may lead to runtime errors like "No matching step definition found". Apply this guidance to Rust test/step files broadly (e.g., all .rs files or tests/**/*.rs as appropriate) when defining macro-based steps.
Learnt from: leynos
Repo: leynos/wireframe PR: 458
File: tests/steps/message_assembly_inbound_steps.rs:24-27
Timestamp: 2026-02-17T00:28:51.250Z
Learning: In Rust, concat!() cannot be used in procedural macro attribute positions (e.g., #[when(...)], #[given(...)], #[then(...)]), because proc macros receive tokens before macro expansion. For long string literals in such attributes, use backslash-escaped newlines as the correct and only approach.
This was referenced Feb 26, 2026
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
Changes
Core functionality
Testing
Documentation
Artefacts
Why this change
Testing plan
Validation and acceptance criteria
Interfaces and dependencies
Migration and rollback
How to review
Notes: The updated plan aligns with the ADR updates and roadmap commitment for 8.3.3, and integrates soft-limit pacing into the inbound path without altering hard-cap semantics.
📎 Task: https://www.devboxer.com/task/7980c70a-2dd6-4450-980e-340b69fa8fd1