Conversation
Reviewer's GuideAdds a leak-proof SessionRegistry backed by DashMap<ConnectionId, Weak> with insert/get/remove/prune methods, extends PushHandle with downgrade/from_arc, exports ConnectionId and SessionRegistry in the crate root, introduces dashmap dependency, includes comprehensive registry tests, and updates related documentation. Class diagram for SessionRegistry and related typesclassDiagram
class ConnectionId {
+u64 value
+from(u64) ConnectionId
}
class PushHandleInner~F~ {
+high_prio_tx: mpsc::Sender<F>
+low_prio_tx: mpsc::Sender<F>
}
class PushHandle~F~ {
+from_arc(arc: Arc<PushHandleInner<F>>) PushHandle<F>
+downgrade() Weak<PushHandleInner<F>>
+push(frame: F)
+try_push(frame: F)
}
class SessionRegistry~F~ {
+get(id: &ConnectionId) Option<PushHandle<F>>
+insert(id: ConnectionId, handle: &PushHandle<F>)
+remove(id: &ConnectionId)
+prune()
}
SessionRegistry~F~ --> "*" ConnectionId : uses as key
SessionRegistry~F~ --> "*" PushHandleInner~F~ : stores Weak refs
PushHandle~F~ --> PushHandleInner~F~ : wraps Arc
PushHandle~F~ ..> SessionRegistry~F~ : used in insert/get
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Task as Background Task
participant Registry as SessionRegistry
participant Conn as Connection (PushHandle)
participant Queue as Frame Queue
Task->>Registry: get(ConnectionId)
alt If handle exists and alive
Registry-->>Task: PushHandle
Task->>Conn: push(frame)
Conn->>Queue: enqueue(frame)
else If handle missing or dropped
Registry-->>Task: None
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`docs/*`: Each file in the docs/ directory provides design, roadmap, testing, op...
📄 Source: CodeRabbit Inference Engine (docs/contents.md) List of files the instruction was applied to:
`docs/**/*.md`: Use British English spelling based on the Oxford English Diction...
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md) List of files the instruction was applied to:
`docs/**/*`: Write official documentation for new features, including separate g...
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md) List of files the instruction was applied to:
🪛 LanguageTooldocs/asynchronous-outbound-messaging-design.md[uncategorized] ~73-~73: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [style] ~448-~448: Would you like to use the Oxford spelling “summarizes”? The spelling ‘summarises’ is also correct. (OXFORD_SPELLING_Z_NOT_S) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider having
getremove expired entries (e.g. prune whenupgradereturnsNone) so that dead sessions don’t accumulate in the registry. - You might want to add an iterator or listing method for active
ConnectionIds/PushHandles to support broadcast or diagnostics without reaching into theDashMapdirectly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider having `get` remove expired entries (e.g. prune when `upgrade` returns `None`) so that dead sessions don’t accumulate in the registry.
- You might want to add an iterator or listing method for active `ConnectionId`s/`PushHandle`s to support broadcast or diagnostics without reaching into the `DashMap` directly.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}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)README.md(1 hunks)docs/asynchronous-outbound-messaging-roadmap.md(1 hunks)src/lib.rs(1 hunks)src/push.rs(4 hunks)src/session.rs(1 hunks)tests/session_registry.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
`Cargo.toml`: Use explicit version ranges in `Cargo.toml` and keep dependencies up-to-date.
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
Cargo.toml
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation in thedocs/directory must use en-GB-oxendict spelling and grammar, except for the word 'license'.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI by runningnixie *.md **/*.md.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mdREADME.md
`docs/*`: Each file in the docs/ directory provides design, roadmap, testing, op...
docs/*: Each file in the docs/ directory provides design, roadmap, testing, operations, or reference documentation for the project. Use these files as a reference for understanding the project's architecture, development plans, testing strategies, operational guidelines, and documentation conventions.
📄 Source: CodeRabbit Inference Engine (docs/contents.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`docs/**/*.md`: Use British English spelling based on the Oxford English Diction...
docs/**/*.md: Use British English spelling based on the Oxford English Dictionary, except retain US spelling in API names (e.g., 'color').
Use the Oxford comma in lists.
Write headings in sentence case and use Markdown heading levels in order without skipping.
Follow markdownlint recommendations for Markdown formatting.
Always use fenced code blocks with a language identifier; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change.
Prefer inline links using 'text' or angle brackets around the URL.
Expand any uncommon acronym on first use, e.g., Continuous Integration (CI).
Wrap paragraphs at 80 columns, code at 120 columns, and do not wrap tables.
Use footnotes referenced with '[^label]'.
When embedding figures, use '' and provide concise alt text describing the content.
Add a short description before each Mermaid diagram for screen readers.
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`docs/**/*`: Write official documentation for new features, including separate g...
docs/**/*: Write official documentation for new features, including separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each with runnable examples and explanations of relevant concepts and APIs.
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mdREADME.md
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
**/*.rs: Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/lib.rstests/session_registry.rssrc/push.rssrc/session.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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.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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/lib.rstests/session_registry.rssrc/push.rssrc/session.rs
🧬 Code Graph Analysis (1)
src/session.rs (1)
src/push.rs (1)
from_arc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (15)
Cargo.toml (1)
15-15: Good addition for SessionRegistry support.The
dashmapdependency correctly supports the concurrent SessionRegistry implementation. The major version constraint is appropriate for this concurrent data structure library.docs/asynchronous-outbound-messaging-roadmap.md (1)
29-31: Accurate completion tracking.The roadmap correctly marks the SessionRegistry implementation as complete, with proper references to the design documentation.
src/lib.rs (2)
21-21: Proper module addition for session functionality.The new
sessionmodule correctly exposes the SessionRegistry functionality.
26-26: Appropriate public re-exports.The re-exports of
ConnectionIdandSessionRegistryfollow the established pattern and make the new types accessible to library users.README.md (1)
148-169: Clear and helpful documentation addition.The Session Registry section effectively explains the purpose and usage of the new functionality. The code example demonstrates practical integration within a
WireframeProtocolimplementation.Minor style note: Consider using British English spelling throughout for consistency with coding guidelines (though this may be acceptable as technical documentation).
src/push.rs (4)
9-9: Necessary import for weak reference support.The
Weakimport is required for the new downgrade functionality supporting the SessionRegistry.
48-48: Appropriate visibility change for crate-internal access.Making
PushHandleInnerpub(crate)allows the session module to access it while maintaining encapsulation from external users.
58-58: Useful constructor for Arc-based creation.The
from_arcmethod provides a clean way to createPushHandleinstances from existingArc<PushHandleInner<F>>, supporting the SessionRegistry's internal operations.
121-122: Essential method for SessionRegistry integration.The
downgrademethod enables creating weak references for storage in the SessionRegistry, preventing memory leaks whilst allowing frame delivery to active connections.tests/session_registry.rs (1)
1-1: LGTM: Proper module documentation.The module-level doc comment correctly explains the module's purpose as required by the coding guidelines.
src/session.rs (5)
1-6: LGTM: Excellent module documentation.The module-level doc comment properly explains the module's purpose and utility, meeting the coding guidelines requirements.
25-32: LGTM: Correct weak reference upgrade pattern.The implementation correctly upgrades the weak reference and uses the
from_arcmethod to reconstruct the PushHandle. This properly handles the case where the connection may have been dropped.
39-40: LGTM: Simple and correct removal implementation.The remove method correctly removes entries from the DashMap.
42-43: LGTM: Proper weak reference pruning logic.The pruning implementation correctly uses
strong_count() > 0to identify live weak references and removes dead entries. This prevents the registry from accumulating dead entries over time.
34-37: Verified:PushHandle::downgrade()exists and returns the expectedWeak<PushHandleInner<F>>
- Defined in src/push.rs at line 122:
pub(crate) fn downgrade(&self) -> Weak<PushHandleInner<F>> { Arc::downgrade(&self.0) }No changes are required here.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/session.rs(1 hunks)tests/session_registry.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI by runningnixie *.md **/*.md.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
README.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
README.md
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
**/*.rs: Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/session_registry.rssrc/session.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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.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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/session_registry.rssrc/session.rs
🧬 Code Graph Analysis (2)
tests/session_registry.rs (3)
src/push.rs (1)
bounded(135-149)src/session.rs (3)
from(18-18)new(24-24)get(43-48)tests/connection_actor.rs (1)
queues(18-18)
src/session.rs (1)
src/push.rs (1)
from_arc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (4)
tests/session_registry.rs (1)
1-62: Excellent test coverage using rstest fixtures.The tests effectively cover the SessionRegistry lifecycle management with proper use of rstest fixtures to reduce duplication, comprehensive doc comments, and clear test scenarios. This addresses the requirements from the coding guidelines perfectly.
src/session.rs (3)
1-11: Excellent module documentation and imports.The module doc comment clearly explains the purpose and design rationale for using weak references. The imports are well-organised and appropriate.
13-35: Well-designed ConnectionId type with comprehensive API.The ConnectionId implementation follows Rust best practices with appropriate derives, conversion traits, utility methods with
#[must_use]attributes, and a helpful Display implementation. This addresses the previous feedback about adding utility methods.
37-60: Robust SessionRegistry implementation with proper lifecycle management.The SessionRegistry correctly uses weak references to prevent memory leaks whilst enabling asynchronous frame sending. The implementation is thread-safe with DashMap and provides comprehensive lifecycle management through get, insert, remove, and prune methods.
The
prunemethod correctly removes dead entries by checkingstrong_count() > 0, and thegetmethod safely upgrades weak references with proper fallback toNonewhen handles are dropped.
leynos
deleted the
codex/implement-leak-proof-sessionregistry-with-dashmap
branch
1 participant
Summary
DashMap<ConnectionId, Weak<PushHandleInner>>ConnectionIdtypeTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_686081a8382c832283b477b2cffa0269
Summary by Sourcery
Implement a leak-proof
SessionRegistrythat usesDashMapandWeakpointers to manage connectionPushHandles without preventing their cleanup.New Features:
SessionRegistryto track activePushHandles byConnectionIdusing weak referencesConnectionIdtype and expose it in the public APIEnhancements:
PushHandle::downgradeandPushHandle::from_arcmethods to support weak reference storageBuild:
dashmapdependency for concurrent session registryDocumentation:
SessionRegistryin the README and mark it as completed in the asynchronous outbound messaging roadmapTests:
SessionRegistrycovering handle retrieval, cleanup after drop, and pruning dead entries