Add interleaved push queues tests (unit/BDD) and docs

[leynos]

Summary

• Implements and wires unit tests for interleaved high- and low-priority push queues, plus BDD tests, fixtures, and scenario wiring. Adds design/docs updates. All changes are additive; no public API changes.

Changes

Stage A: Unit tests scaffold

• Added tests/interleaved_push_queues.rs with unit tests for interleaved push queues and rate-limiter symmetry.
• Added tests/fixtures/interleaved_push_queues.rs and wired fixture/module registrations to support interleaved queue tests.
• Updated tests/fixtures/mod.rs to export interleaved fixture for test wiring.

Stage B: BDD tests scaffold

• Added tests/features/interleaved_push_queues.feature describing interleaved traffic scenarios.
• Added tests/steps/interleaved_push_queues_steps.rs and tests/scenarios/interleaved_push_queues_scenarios.rs to wire BDD scenarios to Rust tests.
• Updated wiring modules: tests/fixtures/mod.rs, tests/steps/mod.rs, and tests/scenarios/mod.rs to include new modules.

Stage C: Documentation updates

• New exec plan doc: docs/execplans/10-3-2-exercise-interleaved-queues.md outlining the interleaved queues testing plan, constraints, tolerances, risks, progress, and deliverables.
• Updated docs/multi-packet-and-streaming-responses-design.md with section 13: Interleaved push queue validation, detailing motivation, testing strategy, and design decisions.
• Updated docs/roadmap.md to mark 10.3.2 as done.
• Updated docs/users-guide.md to note interleaved push queue validation and shared-rate-limiter behavior.
• Roadmap update note included to reflect completion of 10.3.2.

Stage D: Validation guidance

• Provided a concrete validation plan to run unit tests, BDD tests, formatting, linting, and markdown linting as part of the quality gates.
• Outlines artifact expectations (new unit tests, BDD scenarios) and success criteria (no regressions in core flows).

Note: This PR focuses on planning and scaffolding for exercising interleaved queues. No public API signatures are changed.

Rationale / Design decisions

• The system continues to rely on a shared leaky-bucket RateLimiter across both high- and low-priority queues to ensure symmetrical throughput and to validate that interleaving does not bypass global rate limits.
• FairnessConfig governs when low-priority frames yield to high-priority traffic via a counter-based and/or time-slice mechanism, ensuring low-priority frames are eventually delivered under sustained bursts.
• Tests are split between unit tests (direct PushQueues/ConnectionActor behavior) and BDD tests (end-to-end actor wiring) to validate both isolated and integrated behavior.
• Virtual time and current_thread Tokio runtime considerations are documented to avoid flakiness in time-based tests.

Tests planned / Artifacts

• Unit tests: 8 tests in tests/interleaved_push_queues.rs (rate-limit symmetry, counter-based fairness, time-slice fairness, total throughput, completeness, and strict priority scenarios).
• BDD tests: 4 scenarios in tests/features/interleaved_push_queues.feature with supporting fixture, steps, and scenario wiring modules.
• Artifacts:
  • New: docs/execplans/10-3-2-exercise-interleaved-queues.md
  • New: tests/interleaved_push_queues.rs
  • New: tests/fixtures/interleaved_push_queues.rs
  • New: tests/features/interleaved_push_queues.feature
  • New: tests/scenarios/interleaved_push_queues_scenarios.rs
  • New: tests/fixtures/mod.rs (updated)
  • New: tests/steps/interleaved_push_queues_steps.rs
  • New: tests/steps/mod.rs (updated)
  • New: tests/scenarios/mod.rs (updated)

How to validate locally

• Stage A: Unit tests
  • cargo check --all-targets --all-features
  • cargo test --test interleaved_push_queues -- --nocapture
• Stage B: BDD tests
  • make test-bdd (or equivalent BDD runner in your setup)
• Stage C: Documentation
  • make check-fmt; make lint; make markdownlint
• Stage D: Full validation
  • make test; verify roadmap/docs updates reflect 10.3.2 done

Risk & mitigation

• Potential flakiness due to virtual-time tests; mitigations include serializing sensitive tests and ensuring large time deltas relative to rate limiter intervals (documented in the ExecPlan).
• BDD step text conflicts are mitigated by scenario prefixes (interleaved) and careful step naming.

Acceptance criteria

• All new unit tests pass locally and integrate with existing test suite.
• All 4 BDD scenarios pass via the BDD pipeline.
• Documentation reflects the updated roadmap and interleaved queue validation approach.
• No public API changes; project formatting and linting pass.

Related files touched

• New/updated docs:
  • docs/execplans/10-3-2-exercise-interleaved-queues.md
  • docs/multi-packet-and-streaming-responses-design.md (section 13)
  • docs/roadmap.md (10.3.2 done)
  • docs/users-guide.md (interleaved push queue validation notes)
• New/updated tests:
  • tests/interleaved_push_queues.rs
  • tests/features/interleaved_push_queues.feature
  • tests/fixtures/interleaved_push_queues.rs
  • tests/fixtures/mod.rs (updated)
  • tests/scenarios/interleaved_push_queues_scenarios.rs
  • tests/scenarios/mod.rs (updated)
  • tests/steps/interleaved_push_queues_steps.rs
  • tests/steps/mod.rs (updated)
• New/updated code scaffolding for interleaved tests:
  • (wired in) tests/fixtures/interleaved_push_queues.rs
  • (wired in) tests/fixtures/mod.rs
  • (wired in) tests/steps/interleaved_push_queues_steps.rs
  • (wired in) tests/steps/mod.rs
  • (wired in) tests/scenarios/interleaved_push_queues_scenarios.rs
  • (wired in) tests/scenarios/mod.rs
• Shared test helpers:
  • tests/common/interleaved_push_helpers.rs
• Common test fixture:
  • tests/fixtures/mod.rs (new module exposure for interleaved fixtures)
• Unit tests module:
  • tests/interleaved_push_queues.rs (new)
• Scenario wiring:
  • tests/scenarios/interleaved_push_queues_scenarios.rs (new)
• BDD wiring modules:
  • tests/bdd/mod.rs (existing) updated via new wiring modules
• Shared BDD world:
  • tests/common/interleaved_push_helpers.rs (shared helpers)

(13 files changed, 1181 insertions(+), 2 deletions(-))

📎 Task: https://www.devboxer.com/task/c920ccb1-e43f-4251-8823-acd4a1e2369e

[sourcery-ai]

Reviewer's Guide

Adds a focused test and documentation scaffold to validate interleaved high- and low-priority push queues: new unit tests around shared rate limiting and fairness in ConnectionActor/PushQueues, new rstest-bdd feature, fixture, steps, and scenarios, plus design/roadmap/users-guide updates and an exec plan documenting constraints, risks, and validation steps.

Sequence diagram for interleaved push queue rate limiting in unit tests

sequenceDiagram
    participant UnitTest as unit_test_interleaved_push_queues
    participant Runtime as TokioRuntime
    participant HighHandle as PushHandle_high
    participant LowHandle as PushHandle_low
    participant Limiter as RateLimiter
    participant Queues as PushQueues
    participant ConnActor as ConnectionActor

    UnitTest->>Runtime: spawn async test task
    Runtime->>HighHandle: push_high_priority(frame_h1)
    HighHandle->>Limiter: wait_for_permit()
    Limiter-->>HighHandle: grant_token
    HighHandle->>Queues: send_high(frame_h1)

    Runtime->>LowHandle: push_low_priority(frame_l1)
    LowHandle->>Limiter: wait_for_permit()
    alt token_bucket_empty
        Limiter-->>LowHandle: block_until_refill
        Runtime->>Limiter: advance_virtual_time()
        Limiter-->>LowHandle: grant_token_after_refill
    else tokens_available
        Limiter-->>LowHandle: grant_token
    end
    LowHandle->>Queues: send_low(frame_l1)

    UnitTest->>ConnActor: run(out_frames)
    loop biased_select
        ConnActor->>Queues: recv()
        Queues-->>ConnActor: next_frame(high_or_low)
        ConnActor->>ConnActor: after_high_or_low(fairness)
    end
    ConnActor-->>UnitTest: out_frames_contains[h1,l1]

    UnitTest->>UnitTest: assert_rate_limit_symmetry()
    UnitTest->>UnitTest: assert_no_frame_loss()

Loading

Class diagram for interleaved push queue testing and core types

classDiagram
    class PushQueues~F~ {
        +high_priority_rx
        +low_priority_rx
        +build_with_config()
        +recv()
    }

    class PushHandle~F~ {
        +push_high_priority(frame)
        +push_low_priority(frame)
        +try_push(frame)
        +wait_for_permit(limiter)
    }

    class FairnessConfig {
        +usize max_high_before_low
        +Option_Duration_ time_slice
    }

    class FairnessTracker {
        +usize high_counter
        +Instant high_start
        +record_high_priority()
        +should_yield_to_low_priority(clock)
        +reset()
    }

    class RateLimiter {
        +wait_for_permit()
        +refill_tokens()
    }

    class ConnectionActor~F,E~ {
        +set_fairness(FairnessConfig)
        +run(out_frames)
        +next_event()
    }

    class InterleavedPushWorld {
        +PushQueues~u8~ queues
        +PushHandle~u8~ high_handle
        +PushHandle~u8~ low_handle
        +ConnectionActor~u8,unit~ actor
        +configure_fairness(FairnessConfig)
        +push_high_frames(frames)
        +push_low_frames(frames)
        +drain_actor()
    }

    PushHandle~F~ --> PushQueues~F~ : sends_frames_to
    PushQueues~F~ --> RateLimiter : uses_shared
    ConnectionActor~F,E~ --> PushQueues~F~ : drains_from
    ConnectionActor~F,E~ --> FairnessTracker : uses
    FairnessTracker --> FairnessConfig : configured_by
    InterleavedPushWorld --> PushQueues~u8~ : owns
    InterleavedPushWorld --> PushHandle~u8~ : owns
    InterleavedPushWorld --> ConnectionActor~u8,unit~ : owns
    InterleavedPushWorld --> RateLimiter : shares_with_handles

Loading

Flow diagram for BDD interleaved push queue scenarios

flowchart LR
    Feature["interleaved_push_queues.feature\n(Gherkin scenarios)"] --> Runner["rstest_bdd_runner"]
    Runner --> FixtureMod["tests/fixtures/mod.rs\nexports interleaved_push_queues"]
    FixtureMod --> FixtureFile["interleaved_push_queues.rs\nInterleavedPushWorld fixture"]
    FixtureFile --> World["InterleavedPushWorld\n(connection, queues, handles)"]

    Runner --> StepsMod["tests/steps/mod.rs\nexports interleaved_push_queues_steps"]
    StepsMod --> StepsFile["interleaved_push_queues_steps.rs\nGiven/When/Then"]
    StepsFile --> World

    Runner --> ScenariosMod["tests/scenarios/mod.rs\nexports interleaved_push_queues_scenarios"]
    ScenariosMod --> ScenariosFile["interleaved_push_queues_scenarios.rs\n#[scenario] wiring"]
    ScenariosFile --> World

    World --> ConnectionActor["ConnectionActor<u8, ()>"]
    World --> PushQueues["PushQueues<u8>"]
    World --> RateLimiter["shared RateLimiter"]
    ConnectionActor --> PushQueues
    PushQueues --> RateLimiter

Loading

File-Level Changes

Change	Details	Files
Introduce unit test suite validating interleaved high-/low-priority push queues, fairness behaviour, and shared rate-limiter semantics.	Add tests/interleaved_push_queues.rs with 8 async tests covering rate-limit symmetry, fairness threshold and time-slice behaviour, completeness, and strict-priority behaviour. Factor shared actor/queue setup helpers (run_actor_test, test_single_priority_rate_limit, drain_three) to keep tests focused on behaviour. Use tokio::time::pause/advance with serial_test to avoid flakiness in time-based tests and rely on shared PushQueues builder API.	tests/interleaved_push_queues.rs
Add rstest-bdd coverage for interleaved push queues via a dedicated world fixture, steps, and scenario wiring.	Create InterleavedPushWorld fixture encapsulating ConnectionActor setup plus helper methods to run strict-priority, fairness, rate-limit symmetry, and interleaved-delivery scenarios. Implement BDD step definitions that drive the world methods, including using a current_thread runtime where tokio::time::pause() is required. Wire four Gherkin scenarios to the fixture via rstest-bdd #[scenario] macros and register the new fixture/steps/scenarios modules in the existing BDD mod trees.	tests/fixtures/interleaved_push_queues.rs tests/steps/interleaved_push_queues_steps.rs tests/scenarios/interleaved_push_queues_scenarios.rs tests/features/interleaved_push_queues.feature tests/fixtures/mod.rs tests/steps/mod.rs tests/scenarios/mod.rs
Document the interleaved push queue validation strategy and mark roadmap item 10.3.2 complete.	Add an exec plan docs/execplans/10-3-2-exercise-interleaved-queues.md detailing purpose, constraints, risks, plan of work, validation steps, and retrospective for interleaved queue testing. Extend the multi-packet/streaming design doc with a new section describing motivation, testing strategy, and design decisions for interleaved queue validation. Update the users guide to reference the new validation of interleaved high-/low-priority pushes and the shared rate limiter, and mark roadmap entry 10.3.2 as done.	docs/execplans/10-3-2-exercise-interleaved-queues.md docs/multi-packet-and-streaming-responses-design.md docs/users-guide.md docs/roadmap.md

Possibly linked issues

• #: The PR’s interleaved_time_slice_fairness test uses max_high_before_low=0 and time_slice to confirm mid-stream low-priority yielding.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Walkthrough

Add comprehensive testing and documentation for interleaved high- and low-priority push queues, including an execution plan, design updates, test helpers, fixtures, unit tests, BDD feature and scenario wiring, and step definitions to validate fairness, rate-limiting and delivery behaviours.

Changes

Cohort / File(s)	Summary
Documentation docs/execplans/10-3-2-exercise-interleaved-queues.md, docs/multi-packet-and-streaming-responses-design.md, docs/roadmap.md, docs/users-guide.md	Add execution plan and design section for interleaved push-queue validation, update user's guide with fairness and rate-limit narrative, and mark roadmap item 10.3.2 as complete.
Test helper & fixtures tests/common/interleaved_push_helpers.rs, tests/fixtures/interleaved_push_queues.rs, tests/fixtures/mod.rs	Introduce run_actor_with_fairness helper and InterleavedPushWorld fixture with run modes (strict priority, fairness threshold, rate-limit symmetry, interleaved delivery) and verification methods.
Unit tests tests/interleaved_push_queues.rs	Add comprehensive unit tests covering rate-limit symmetry, mixed-priority interactions, fairness thresholds and time-slice fairness, throughput caps, and strict-priority behaviour using time control and cancellation.
BDD tests & wiring tests/features/interleaved_push_queues.feature, tests/scenarios/interleaved_push_queues_scenarios.rs, tests/steps/interleaved_push_queues_steps.rs, tests/scenarios/mod.rs, tests/steps/mod.rs	Add four BDD feature scenarios and scenario glue with step definitions that invoke the fixture methods and verification routines via Tokio runtimes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

📋 Queues align in fairness bright,
High-priority bursts then low takes flight,
Rate limiters dance in symmetry,
Tests ensure delivery free,
Priority intertwined, delivering right! 🎯

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The pull request title directly summarises the main changes: adding comprehensive interleaved push queues tests (both unit and BDD) along with documentation updates.
Description check	✅ Passed	The pull request description is detailed and directly related to the changeset, covering all stages of implementation (unit tests, BDD tests, docs, validation), with clear rationale and acceptance criteria.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch exercise-interleaved-queues-sifxgm

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[leynos]

@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/interleaved_push_queues.rs

Comment on file

async fn fairness_disabled_strict_priority(shutdown_token: CancellationToken) -> TestResult {
    let (queues, handle) = PushQueues::<u8>::builder()
        .high_capacity(8)
        .low_capacity(8)
        .unlimited()
        .build()?;

    let fairness = FairnessConfig {
        max_high_before_low: 0,
        time_slice: None,
    };

    push_expect!(handle.push_low_priority(101));
    push_expect!(handle.push_low_priority(102));
    push_expect!(handle.push_high_priority(1));
    push_expect!(handle.push_high_priority(2));
    push_expect!(handle.push_high_priority(3));

    let mut actor: ConnectionActor<_, ()> =
        ConnectionActor::new(queues, handle, None, shutdown_token);
    actor.set_fairness(fairness);
    let mut out = Vec::new();
    actor
        .run(&mut out)
        .await
        .map_err(|e| std::io::Error::other(format!("actor run failed: {e:?}")))?;

    assert_eq!(
        out,
        vec![1, 2, 3, 101, 102],
        "all high frames should precede all low frames"
    );
    Ok(())
}

❌ New issue: Code Duplication
The module contains 5 functions with similar structure: fairness_disabled_strict_priority,interleaved_fairness_yields_at_threshold,rate_limit_symmetric_high_only,rate_limit_symmetric_low_only and 1 more functions

[leynos]

@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/interleaved_push_queues.rs

Comment on file

    pub async fn run_strict_priority(&mut self) -> TestResult {
        let (queues, handle) = PushQueues::<u8>::builder()
            .high_capacity(8)
            .low_capacity(8)
            .unlimited()
            .build()?;

        let fairness = FairnessConfig {
            max_high_before_low: 0,
            time_slice: None,
        };

        push_expect!(handle.push_low_priority(101));
        push_expect!(handle.push_low_priority(102));
        push_expect!(handle.push_high_priority(1));
        push_expect!(handle.push_high_priority(2));
        push_expect!(handle.push_high_priority(3));

        let shutdown = CancellationToken::new();
        let mut actor: ConnectionActor<_, ()> =
            ConnectionActor::new(queues, handle, None, shutdown);
        actor.set_fairness(fairness);
        self.frames = Vec::new();
        actor
            .run(&mut self.frames)
            .await
            .map_err(|e| format!("actor run failed: {e:?}"))?;
        Ok(())
    }

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: InterleavedPushWorld.run_fairness_threshold,InterleavedPushWorld.run_interleaved_delivery,InterleavedPushWorld.run_strict_priority

[leynos]

@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/interleaved_push_queues.rs

Comment on file

    pub async fn run_strict_priority(&mut self) -> TestResult {
        self.run_actor_with_fairness(
            FairnessConfig {
                max_high_before_low: 0,
                time_slice: None,
            },
            |handle| async move {
                push_expect!(handle.push_low_priority(101));
                push_expect!(handle.push_low_priority(102));
                push_expect!(handle.push_high_priority(1));
                push_expect!(handle.push_high_priority(2));
                push_expect!(handle.push_high_priority(3));
            },
        )
        .await
    }

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: InterleavedPushWorld.run_fairness_threshold,InterleavedPushWorld.run_interleaved_delivery,InterleavedPushWorld.run_strict_priority

[leynos]

@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/interleaved_push_queues.rs

Comment on file

async fn rate_limit_symmetric_high_only() -> TestResult {
    time::pause();
    let (mut queues, handle) = PushQueues::<u8>::builder()
        .high_capacity(4)
        .low_capacity(4)
        .rate(Some(2))
        .build()?;

    // Burst of 2 should succeed immediately.
    push_expect!(handle.push_high_priority(1));
    push_expect!(handle.push_high_priority(2));

    // Third push should be blocked by the rate limiter.
    let mut blocked = handle.push_high_priority(3).boxed();
    tokio::task::yield_now().await;
    assert!(
        blocked.as_mut().now_or_never().is_none(),
        "third high push should be pending under rate limit"
    );

    // Advance past the refill window and push again.
    time::advance(Duration::from_secs(1)).await;
    push_expect!(handle.push_high_priority(4));

    // Drain to confirm frame delivery.
    let (_, a) = queues.recv().await.ok_or("recv 1 failed")?;
    let (_, b) = queues.recv().await.ok_or("recv 2 failed")?;
    let (_, c) = queues.recv().await.ok_or("recv 3 failed")?;
    assert_eq!((a, b, c), (1, 2, 4), "high frames delivered in order");
    Ok(())
}

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: rate_limit_symmetric_high_only,rate_limit_symmetric_low_only,rate_limit_symmetric_mixed

[coderabbitai]

Actionable comments posted: 7

🤖 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/10-3-2-exercise-interleaved-queues.md`:
- Line 94: Replace the non-possessive phrase "users guide" with the correct
possessive "user's guide" in the document (e.g., the line containing "Stage C:
Documentation — update design doc, users guide, roadmap." and the other
occurrences noted), updating all instances referenced (lines ~94, ~135, ~141) so
the phrase reads "user's guide".
- Around line 269-272: The word "Pre-load" should be corrected to "Preload" in
the documentation; update the occurrences in the description of the interleaved
fairness example for the test/section named
interleaved_fairness_yields_at_threshold (where max_high_before_low = 3 is
mentioned) and the similar occurrence later (around the second mention,
previously at line ~291) so both instances read "Preload" as a single word.

In `@docs/multi-packet-and-streaming-responses-design.md`:
- Around line 772-774: The phrase "parameterised" in the sentence referencing
unit tests and FairnessConfig should use Oxford spelling "parameterized"; update
the documentation line that mentions tests/interleaved_push_queues.rs and the
`ConnectionActor`, `PushQueues`, `FairnessConfig`, and `tokio::time::pause` to
replace "parameterised" with "parameterized" so the docs follow en-GB-oxendict
"-ize" convention.

In `@tests/fixtures/interleaved_push_queues.rs`:
- Around line 99-126: The test run_rate_limit_symmetry currently sets
rate_limit_recovered by checking blocked.as_mut().now_or_never().is_some(),
which treats Some(Err(_)) as success; change the check to explicitly match
Some(Ok(())) to mark recovery and treat Some(Err(e)) as a test failure (panic or
return Err) so only a successful push_unblock is considered recovery; update the
assignment to self.rate_limit_recovered and fail fast on any Err from
handle.push_low_priority to ensure correct semantics for the blocked future.

In `@tests/interleaved_push_queues.rs`:
- Around line 104-120: Replace the two nearly identical tests
rate_limit_symmetric_high_only and rate_limit_symmetric_low_only with a single
parameterised #[rstest] that passes cases for the push function and label to
test_single_priority_rate_limit; specifically, create an rstest with two cases
that supply either the closure that calls push_high_priority and label "high" or
the closure that calls push_low_priority and label "low", then call
test_single_priority_rate_limit(...) in the body once. Keep references to
push_high_priority, push_low_priority and test_single_priority_rate_limit so the
existing helper is reused and remove the duplicate wrapper functions.
- Around line 248-277: The spawned task currently discards
ConnectionActor::run's Result (using let _ = actor.run(...).await and a oneshot)
so test can false-green; change the spawn to propagate errors: either await the
JoinHandle and bubble up actor.run()'s Result (replace oneshot::channel with
using tokio::spawn(...) -> JoinHandle<Result<_, _>> and call .await? to
propagate panics/errors) or have the task send the actual Result via the oneshot
(send(Ok(out)) / send(Err(e))). Ensure actor.run(...)'s result is not ignored
and map or ? the error into the test failure; update references to actor.run,
tokio::spawn, oneshot::channel, tx and rx accordingly.

In `@tests/scenarios/interleaved_push_queues_scenarios.rs`:
- Around line 7-35: The four scenario functions strict_priority,
fairness_threshold, rate_limit_symmetry, and all_frames_delivered currently use
the pattern "let _ = interleaved_push_world;" which triggers
clippy::let_underscore_drop; replace those with an explicit
drop(interleaved_push_world) (or borrow the value if you need it) so the binding
is consumed without a lint; update each function body to call
drop(interleaved_push_world) instead of the let_underscore pattern.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1863a3 and d9cbf0d.

📒 Files selected for processing (13)

• docs/execplans/10-3-2-exercise-interleaved-queues.md
• docs/multi-packet-and-streaming-responses-design.md
• docs/roadmap.md
• docs/users-guide.md
• tests/common/interleaved_push_helpers.rs
• tests/features/interleaved_push_queues.feature
• tests/fixtures/interleaved_push_queues.rs
• tests/fixtures/mod.rs
• tests/interleaved_push_queues.rs
• tests/scenarios/interleaved_push_queues_scenarios.rs
• tests/scenarios/mod.rs
• tests/steps/interleaved_push_queues_steps.rs
• tests/steps/mod.rs

[leynos]

@coderabbitai Have the following now been resolved?

Please address the comments from this code review:

## Overall Comments
- The BDD fixture and the unit tests both hand-roll very similar setups for strict-priority, fairness-threshold, and interleaved-delivery cases; consider extracting a shared helper (or reusing `run_actor_test`) so behavioural expectations are defined in one place and harder to let drift over time.
- Error handling in the new test helpers is a bit inconsistent (e.g. sometimes wrapping with `std::io::Error::other`, sometimes formatting into a `String`); aligning on a single convention for `TestResult` would make failures easier to interpret and reduce boilerplate.

## Individual Comments

### Comment 1
<location> `docs/execplans/10-3-2-exercise-interleaved-queues.md:3` </location>
<code_context>
+# Exercise interleaved high- and low-priority push queues
+
+This ExecPlan is a living document. The sections `Constraints`, `Tolerances`,
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
</code_context>

<issue_to_address>
**issue (review_instructions):** ExecPlan is introduced as an acronym without first defining the full term.

To comply with the acronym rule, spell out the term before the acronym, for example: `This Execution Plan (ExecPlan) is a living document.` After that, using `ExecPlan` alone is fine.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 2
<location> `docs/execplans/10-3-2-exercise-interleaved-queues.md:50` </location>
<code_context>
+  `make markdownlint`.
+- Documentation must use en-GB-oxendict spelling per `AGENTS.md`.
+- No single source file may exceed 400 lines per `AGENTS.md`.
+- BDD tests must use `rstest-bdd` 0.5.0.
+- The `Packet` impl for `u8` in `src/connection/test_support.rs` is the
+  established frame type for connection actor tests; reuse it.
</code_context>

<issue_to_address>
**issue (review_instructions):** BDD is used without expanding it to its full term on first mention in this document.

Consider expanding BDD on first use, for example: `Behaviour-Driven Development (BDD) tests must use ...`. Subsequent uses of `BDD` alone are then clear.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll check the current state of the relevant files to verify whether these review comments have been addressed.

---

🏁 Script executed:

#!/bin/bash
# Read the shared helper module
echo "=== tests/common/interleaved_push_helpers.rs ==="
cat tests/common/interleaved_push_helpers.rs
echo ""
echo "=== tests/fixtures/interleaved_push_queues.rs ==="
cat tests/fixtures/interleaved_push_queues.rs
echo ""
echo "=== tests/interleaved_push_queues.rs ==="
cat tests/interleaved_push_queues.rs
echo ""
echo "=== docs/execplans/10-3-2-exercise-interleaved-queues.md (first 100 lines) ==="
head -100 docs/execplans/10-3-2-exercise-interleaved-queues.md

Length of output: 24700

---

@leynos: Yes, all the review comments have been successfully resolved! Here's the status:

✅ Overall Comment 1: Shared Helper Extraction

Resolved. The code now has tests/common/interleaved_push_helpers.rs containing a shared run_actor_with_fairness helper that:

• Creates PushQueues with the standard configuration
• Executes the setup closure
• Runs the ConnectionActor with the provided fairness config
• Returns collected output

Both the BDD fixture (tests/fixtures/interleaved_push_queues.rs) and unit tests (tests/interleaved_push_queues.rs) now delegate to this shared helper, eliminating duplication and ensuring behavioural expectations are defined in one place.

✅ Overall Comment 2: Error Handling Consistency

Resolved. All test helpers now consistently use String-based error formatting:

.map_err(|e| format!("actor run failed: {e:?}"))?

No mix of std::io::Error::other and String formatting remains—all errors are converted to strings via format!(), making test failures easier to interpret with consistent error messages.

✅ Individual Comment 1: ExecPlan Acronym

Resolved. Line 3 of docs/execplans/10-3-2-exercise-interleaved-queues.md now reads:

This Execution Plan (ExecPlan) is a living document.

The full term is expanded before the acronym on first use, with subsequent mentions using "ExecPlan" alone.

✅ Individual Comment 2: BDD Acronym

Resolved. Line 37 of the execution plan document now includes the full expansion on first use:

new Behaviour-Driven Development (BDD) scenarios

Line 50 then correctly uses "BDD" alone after it's been defined.

---

All review feedback has been addressed. The code now has proper helper extraction, consistent error handling, and correctly expanded acronyms in documentation.

[leynos]

@coderabbitai Have the following now been resolved?

Please address the comments from this code review:

## Overall Comments
- The BDD fixture and the unit tests both hand-roll very similar setups for strict-priority, fairness-threshold, and interleaved-delivery cases; consider extracting a shared helper (or reusing `run_actor_test`) so behavioural expectations are defined in one place and harder to let drift over time.
- Error handling in the new test helpers is a bit inconsistent (e.g. sometimes wrapping with `std::io::Error::other`, sometimes formatting into a `String`); aligning on a single convention for `TestResult` would make failures easier to interpret and reduce boilerplate.

## Individual Comments

### Comment 1
<location> `docs/execplans/10-3-2-exercise-interleaved-queues.md:3` </location>
<code_context>
+# Exercise interleaved high- and low-priority push queues
+
+This ExecPlan is a living document. The sections `Constraints`, `Tolerances`,
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
</code_context>

<issue_to_address>
**issue (review_instructions):** ExecPlan is introduced as an acronym without first defining the full term.

To comply with the acronym rule, spell out the term before the acronym, for example: `This Execution Plan (ExecPlan) is a living document.` After that, using `ExecPlan` alone is fine.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 2
<location> `docs/execplans/10-3-2-exercise-interleaved-queues.md:50` </location>
<code_context>
+  `make markdownlint`.
+- Documentation must use en-GB-oxendict spelling per `AGENTS.md`.
+- No single source file may exceed 400 lines per `AGENTS.md`.
+- BDD tests must use `rstest-bdd` 0.5.0.
+- The `Packet` impl for `u8` in `src/connection/test_support.rs` is the
+  established frame type for connection actor tests; reuse it.
</code_context>

<issue_to_address>
**issue (review_instructions):** BDD is used without expanding it to its full term on first mention in this document.

Consider expanding BDD on first use, for example: `Behaviour-Driven Development (BDD) tests must use ...`. Subsequent uses of `BDD` alone are then clear.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the shared helper module to verify it exists and check error handling
echo "=== tests/common/interleaved_push_helpers.rs ==="
if [ -f tests/common/interleaved_push_helpers.rs ]; then
  cat tests/common/interleaved_push_helpers.rs
else
  echo "File not found"
fi

echo ""
echo "=== tests/fixtures/interleaved_push_queues.rs (checking helper usage) ==="
if [ -f tests/fixtures/interleaved_push_queues.rs ]; then
  cat tests/fixtures/interleaved_push_queues.rs
else
  echo "File not found"
fi

echo ""
echo "=== tests/interleaved_push_queues.rs (checking helper usage) ==="
if [ -f tests/interleaved_push_queues.rs ]; then
  cat tests/interleaved_push_queues.rs
else
  echo "File not found"
fi

echo ""
echo "=== docs/execplans/10-3-2-exercise-interleaved-queues.md (first 60 lines - checking acronyms) ==="
if [ -f docs/execplans/10-3-2-exercise-interleaved-queues.md ]; then
  head -60 docs/execplans/10-3-2-exercise-interleaved-queues.md
else
  echo "File not found"
fi

Length of output: 22592

---

@leynos: Yes, all the review comments have been successfully resolved! Here's the verification:

✅ Overall Comment 1: Shared Helper Extraction

Resolved. The file tests/common/interleaved_push_helpers.rs exists and provides a shared run_actor_with_fairness helper that:

• Builds PushQueues with standard configuration (high_capacity=8, low_capacity=8, unlimited)
• Executes a setup closure to load frames
• Runs ConnectionActor with the provided FairnessConfig
• Returns collected output

Both the BDD fixture (tests/fixtures/interleaved_push_queues.rs) and unit tests (tests/interleaved_push_queues.rs) now delegate to this shared helper via path imports, eliminating duplication and ensuring behavioural expectations are defined in one place.

✅ Overall Comment 2: Error Handling Consistency

Resolved. All test helpers now use consistent String-based error formatting:

.map_err(|e| format!("actor run failed: {e:?}"))?

No mix of std::io::Error::other and String formatting remains—all errors are uniformly converted to strings for test failures.

✅ Individual Comment 1: ExecPlan Acronym

Resolved. Line 3 of docs/execplans/10-3-2-exercise-interleaved-queues.md correctly reads:

This Execution Plan (ExecPlan) is a living document.

The full term "Execution Plan" is expanded before the acronym on first use.

✅ Individual Comment 2: BDD Acronym

Resolved. Line 37 of the execution plan document correctly includes:

new Behaviour-Driven Development (BDD) scenarios

The full term "Behaviour-Driven Development" is expanded before the acronym on first use, with subsequent uses of "BDD" alone.

---

All review feedback has been properly addressed in the current codebase.

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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 `@docs/execplans/10-3-2-exercise-interleaved-queues.md`:
- Line 200: The document contains the heading/text "Users guide" which should
use a possessive apostrophe; update the string to "User's guide" wherever it
appears in this file (e.g., the heading entry shown in the diff) to maintain
consistency with prior fixes.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9cbf0d and e67b9d1.

📒 Files selected for processing (4)

• docs/execplans/10-3-2-exercise-interleaved-queues.md
• docs/multi-packet-and-streaming-responses-design.md
• tests/fixtures/interleaved_push_queues.rs
• tests/interleaved_push_queues.rs