Add loom-based concurrency tests for push queues#360
Conversation
Reviewer's GuideThis PR instruments PushHandle internals for loom-based testing by exposing a drop counter probe, introduces comprehensive loom concurrency tests for drop handling and queue-full errors (registered in Cargo.toml), and updates the design docs and roadmap to reflect the new coverage. Class diagram for PushHandle and PushHandleProbe loom instrumentationclassDiagram
class PushHandleInner {
+dlq_drops: AtomicUsize
}
class PushHandle {
+from_arc(arc: Arc<PushHandleInner<F>>): Self
+probe(): PushHandleProbe<F> [loom only]
}
class PushHandleProbe {
+dlq_drop_count(): usize
inner: Arc<PushHandleInner<F>>
}
PushHandleInner <|-- PushHandle
PushHandle "1" -- "1" PushHandleProbe: probe() [loom only]
PushHandleProbe "1" -- "1" PushHandleInner: inner
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdd a loom-gated dependency and test target, introduce loom-only sync re-exports and a PushHandle probe API, add a public SessionRegistry using Weak refs, and replace advanced concurrency tests with loom-based deterministic interleavings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T1 as Loom Thread 1
participant T2 as Loom Thread 2
participant PQ as PushQueues
participant DLQ as Dead-Letter Queue
participant Probe as PushHandleProbe (loom)
rect rgba(230,245,255,0.25)
note over T1,T2: Loom explores interleavings
T1->>PQ: try_push(msg1)
T2->>PQ: try_push(msg2)
alt Queue full -> error
PQ-->>T1: Err(QueueFull)
PQ-->>T2: Err(QueueFull)
else Drop routed to DLQ
PQ->>DLQ: enqueue dropped frame
PQ->>Probe: increment dlq_drops (SeqCst)
end
end
note right of Probe: Tests read Probe.dlq_drop_count() for assertions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)docs/**/*.md📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Files:
**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
docs/**📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
Files:
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
Cargo.toml(2 hunks)docs/asynchronous-outbound-messaging-design.md(1 hunks)docs/roadmap.md(1 hunks)src/push/queues/handle.rs(2 hunks)src/push/queues/mod.rs(1 hunks)tests/advanced/concurrency_loom.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml; keep dependencies up to date
All dependencies in Cargo.toml must use caret (^) SemVer-compatible requirements (e.g., "1.2.3")
Forbid wildcard (*) and open-ended (>=) versions; use ~ only with a specific documented reason
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/push/queues/mod.rstests/advanced/concurrency_loom.rssrc/push/queues/handle.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/push/queues/mod.rstests/advanced/concurrency_loom.rssrc/push/queues/handle.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/push/queues/mod.rstests/advanced/concurrency_loom.rssrc/push/queues/handle.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
docs/**/*.md: Use docs/ markdown as the source of truth for requirements and decisions
Proactively update docs/ when requirements, dependencies, or architecture change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name exempt)When long lines are warranted in ...
Files:
docs/roadmap.mddocs/asynchronous-outbound-messaging-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/roadmap.mddocs/asynchronous-outbound-messaging-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/asynchronous-outbound-messaging-design.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/roadmap.mddocs/asynchronous-outbound-messaging-design.md
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/advanced/concurrency_loom.rs
🧬 Code graph analysis (1)
tests/advanced/concurrency_loom.rs (4)
tests/connection_actor_errors.rs (1)
queues(30-36)src/push/queues/mod.rs (2)
builder(115-115)new(92-109)tests/support.rs (1)
builder(8-10)src/push/queues/handle.rs (2)
probe(74-78)dlq_drop_count(66-66)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (9)
docs/roadmap.md (1)
177-178: Roadmap item completion confirmed.The Phase 5 loom-based concurrency testing item has been correctly marked as complete, aligning with the implementation in
tests/advanced/concurrency_loom.rs.Cargo.toml (2)
113-116: Test configuration looks good.The new
concurrency_loomtest entry is properly configured with the correct path and required features.
71-73: Approve: loom version consistency verifiedBoth entries use loom = "0.7.2" (Cargo.toml lines 52 and 72). No action required.
src/push/queues/mod.rs (1)
9-20: Conditional compilation structure is correct.The cfg-gated imports properly separate loom and non-loom synchronisation primitives whilst maintaining
Arcavailability in both configurations.src/push/queues/handle.rs (2)
3-21: Well-structured loom instrumentation.The conditional sync module elegantly abstracts synchronisation primitives between loom and standard implementations, maintaining API compatibility whilst enabling deterministic concurrency testing.
56-67: Probe API provides clean testing interface.The
PushHandleProbestruct and itsdlq_drop_count()method offer a minimal, focused API for loom-based verification without polluting the production interface.tests/advanced/concurrency_loom.rs (3)
1-10: Module documentation accurately describes test scope.The documentation clearly explains the purpose of these loom-based tests and their focus on DLQ accounting and queue-full error handling.
12-59: Robust DLQ counter reset verification.The test effectively validates that the DLQ drop counter resets after reaching the logging threshold, with proper deterministic verification of dropped frames.
61-97: Queue-full error handling properly tested.The test correctly verifies that concurrent producers receive
QueueFullerrors when attempting to push to a full queue withReturnErrorIfFullpolicy.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/asynchronous-outbound-messaging-design.md (1)
483-491: Remove first‑person pronouns per docs style; rephrase the loom subsection.Align with the docs guidelines and avoid “we”.
- To reason about concurrent producers we added a loom-specific probe to - `PushHandle`. The probe exposes the dead-letter queue drop counter when tests + To reason about concurrent producers, a loom-specific probe was added to + `PushHandle`. The probe exposes the dead-letter queue drop counter when tests are compiled with `--cfg loom`. The `tests/advanced/concurrency_loom.rs` suite drives `PushHandle::try_push` from multiple `loom::thread`s to assert that drop counts reset after the logging threshold across both priority queues, that queue-full errors remain deterministic, and that the probe reports zero when the DLQ is absent or idle. This keeps the production API unchanged whilst enabling exhaustive interleaving checks during the advanced test workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/asynchronous-outbound-messaging-design.md(1 hunks)src/push/queues/handle.rs(2 hunks)tests/advanced/concurrency_loom.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/push/queues/handle.rstests/advanced/concurrency_loom.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/push/queues/handle.rstests/advanced/concurrency_loom.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/push/queues/handle.rstests/advanced/concurrency_loom.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/advanced/concurrency_loom.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
docs/**/*.md: Use docs/ markdown as the source of truth for requirements and decisions
Proactively update docs/ when requirements, dependencies, or architecture change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name exempt)When long lines are warranted in ...
Files:
docs/asynchronous-outbound-messaging-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/asynchronous-outbound-messaging-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/asynchronous-outbound-messaging-design.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/asynchronous-outbound-messaging-design.md
🧬 Code graph analysis (1)
tests/advanced/concurrency_loom.rs (2)
src/push/queues/mod.rs (2)
builder(115-115)new(92-109)src/push/queues/handle.rs (2)
probe(78-82)dlq_drop_count(66-66)
🔍 Remote MCP Deepwiki
Summary of additional repo/docs context relevant to reviewing PR #360 (loom instrumentation + SessionRegistry)
-
Cargo/test changes
- Cargo.toml: adds a cfg(loom) dependency entry for loom and registers a new test binary [[test]] concurrency_loom (tests/advanced/concurrency_loom.rs; required-features = ["advanced-tests"]). Reviewer action: confirm there are no conflicting/duplicate loom entries or version mismatches (dev-deps vs target.'cfg(loom)') and that CI will run the test with RUSTFLAGS="--cfg loom".
-
PushHandle / loom probe instrumentation
- A loom-only PushHandleProbe (pub behind #[cfg(loom)]) and PushHandle::probe() were added in src/push/queues/handle.rs; dlq_drop_count reads an AtomicUsize (SeqCst). A cfg-gated internal sync module re-exports Mutex/atomic types vs non-loom std variants. Reviewer action: verify the #[cfg(loom)] gating is complete (no accidental production exposure) and the probe only observes internal state (no behavioral change).
-
Synchronization primitive strategy (important)
- PR summary/doc indicates std::sync::Arc is reused while Mutex/Atomic types are swapped under cfg(loom). Reviewer action: confirm this choice is intentional — using std::sync::Arc under loom can reduce loom’s ability to model Arc-related interleavings; consider whether Arc/Weak should also use loom::sync when cfg(loom) to ensure full loom coverage.
-
SessionRegistry public API
- Docs & design describe SessionRegistry backed by DashMap<ConnectionId, Weak<PushHandleInner>> with methods get/insert/remove/active_handles and lazy cleanup (upgrade Weak -> Arc during get, prune stale entries). Reviewer action: verify implementation uses Arc::downgrade for insert, properly upgrades/prunes in get/active_handles, and that trait bounds (Send/Sync) and lifetimes are correct for use from background tasks.
-
Tests converted to loom
- tests/advanced/concurrency_loom.rs migrates earlier Tokio-based concurrency tests to loom::thread and asserts deterministic DLQ accounting and queue-full error behavior. Reviewer action: ensure tests do not rely on non-loom synchronization or Tokio runtime primitives and that they exercise the intended interleavings (especially where the Arc/Weak choice may matter). Also confirm the test binary is wired so running Cargo with RUSTFLAGS="--cfg loom" builds what it expects.
-
Docs / roadmap updates
- docs/asynchronous-outbound-messaging-design.md documents the loom probe and the SessionRegistry API; docs/roadmap.md marks the loom test item complete. Reviewer action: ensure documentation matches the implemented behavior and public surface (visibility, method signatures).
Notes for the reviewer (high priority)
- Double-check the sync re-export strategy: if you want exhaustive loom modeling, prefer using loom::sync::{Arc, Weak, Mutex, atomic::AtomicUsize} under cfg(loom) rather than only switching mutex/atomics. This is the highest-risk correctness/coverage question.
- Confirm Cargo.toml/CI: ensure CI runs the loom test with the required RUSTFLAGS and that duplicate loom declarations (dev-deps vs target cfg) don’t cause resolver/warning issues.
- Confirm SessionRegistry’s upgrade/prune semantics are robust under concurrent access and that active_handles() does not produce surprising strong references that prolong connections.
Sources used
- Repository wiki/docs & file summaries.,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (8)
src/push/queues/handle.rs (3)
56-67: LGTM: probe type is minimal and correctly gated.The loom-only probe encapsulates internals without surfacing to production; SeqCst load is appropriate for test determinism.
72-83: LGTM: probe() API is correctly gated and documented.Method is #[must_use], returns a cheap clone, and preserves production surface.
Confirm that re-exports expose PushHandleProbe at the documented path used in tests/docs.
3-11: Use loom::sync::Arc/Weak under cfg(loom).Replace std::sync::{Arc, Weak} with loom::sync::{Arc, Weak} in the #[cfg(loom)] shim so loom can model-check Arc/Weak interleavings.
Apply:
#[cfg(loom)] mod sync { - pub use std::sync::{Arc, Weak}; - - pub use loom::sync::{ - Mutex, - atomic::{AtomicUsize, Ordering}, - }; + pub use loom::sync::{ + Arc, + Weak, + Mutex, + atomic::{AtomicUsize, Ordering}, + }; }rg found std::sync::Arc usages in:
- src/server/config/mod.rs:38 (code)
- src/hooks.rs:7 (use)
Other matches are doc/examples (src/extractor.rs, src/app/builder.rs, src/server/runtime.rs). Ensure none of these are compiled under cfg(loom); replace with loom::sync::{Arc, Weak} where they are.Run this to verify no std::sync::Arc/Weak remain in loom-compiled code:
#!/bin/bash set -eu rg -nP --type=rust '\bstd::sync::(Arc|Weak)\b' src | sed 's/^/std-arc-usage: /' rg -l --type=rust '\bstd::sync::(Arc|Weak)\b' src | sort -u | while read -r f; do if rg -n --type=rust '#\s*\[cfg\(loom\)\]' "$f" >/dev/null; then echo "loom-compiled-std-arc: $f" fi donetests/advanced/concurrency_loom.rs (5)
13-65: LGTM: parameterised DLQ drop/reset test covers both priorities.Test sets a deterministic logging threshold, forces two drops, then asserts post‑reset state and DLQ contents.
Ensure this test remains stable if DLQ logging interval logic changes; gate the reset solely on the N‑multiple path as done here.
67-112: LGTM: queue‑full errors under higher contention are asserted.Three producers with a full buffer correctly observe
QueueFullwithout relying on Tokio runtime.
114-143: LGTM: DLQ‑disabled probe case validates zero drops.Probe semantics remain zero when DLQ is disabled and a drop occurs.
145-171: LGTM: probe reports zero when DLQ exists but is idle.Covers the idle path, ensuring no spurious accounting.
1-12: Enable Tokio's "loom" feature under cfg(loom) so tokio::sync::mpsc is instrumented.Add to Cargo.toml (match your existing Tokio version):
[target.'cfg(loom)'.dependencies] tokio = { version = "1", features = ["sync", "loom"] }Verify configuration:
# Expect a [target.'cfg(loom)'.dependencies] section with tokio features including "loom". rg -n "^\[target\.'cfg\(loom\)'\.dependencies\]" Cargo.toml || true rg -n "tokio.*features.*loom" Cargo.toml || true rg -n "tokio" Cargo.toml || truePrevious automated check produced no matches; confirm Cargo.toml manually.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
deleted the
codex/implement-loom-concurrency-testing-with-unit-tests
branch
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68c8353cf8dc8322badf7955fd73ff46
Summary by Sourcery
Introduce loom-based concurrency testing for PushQueues by instrumenting PushHandle, adding loom-specific tests, updating build config, and documenting the changes
New Features:
Build:
Documentation:
Tests: