Skip to content

Fix asynchronous push docs diagrams #119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leynos merged 4 commits into from
Jun 24, 2025
Merged

Fix asynchronous push docs diagrams #119

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7a131f
Clarify push queue diagrams
leynos Jun 23, 2025
f2771a4
Fix push diagram parsing
leynos Jun 23, 2025
d4e98e9
Clarify push diagrams
leynos Jun 23, 2025
919ce60
Clarify push handle flowchart
leynos Jun 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions docs/asynchronous-outbound-messaging-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,17 @@ protocols.

The implementation must satisfy the following core requirements:

<!-- markdownlint-disable MD013 -->

| ID | Requirement |
| G1 | Any async task must be able to push frames to a live connection. |
| G2 | Ordering-safety: Pushed frames must interleave correctly with normal request/response traffic and respect any per-message sequencing rules. |
| G3 | Back-pressure: Writers must block (or fail fast) when the peer cannot drain the socket, preventing unbounded memory consumption. |
| G4 | Generic—independent of any particular protocol; usable by both servers and clients built on wireframe. |
| G5 | Preserve the simple “return a reply” path for code that does not need pushes, ensuring backward compatibility and low friction for existing users. |

<!-- markdownlint-enable MD013 -->
Comment on lines +27 to +36
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Scope-down the MD013 suppression to the exact table.

Disabling MD013 for an entire block makes future maintenance harder because any accidental long lines outside the table slip through linting. Consider the more surgical directive
<!-- markdownlint-disable-next-line MD013 --> immediately above the table header instead of an open/close pair around the whole section.

🤖 Prompt for AI Agents 
In docs/asynchronous-outbound-messaging-design.md around lines 27 to 36, the
MD013 markdownlint rule is disabled for the entire block containing the table,
which is too broad. To fix this, remove the open/close MD013 disable comments
around the whole section and instead add a single line directive <!--
markdownlint-disable-next-line MD013 --> immediately above the table header line
to limit the suppression only to the table line, ensuring long lines elsewhere
are still linted.


## 3. Core Architecture: The Connection Actor

The foundation of this design is the **actor-per-connection** model, where each
Expand Down Expand Up @@ -153,8 +157,8 @@ impl<F: FrameLike> PushHandle<F> {
&self,
frame: F,
priority: PushPriority,
policy: PushPolicy
) -> Result<(), PushError>;
policy: PushPolicy,
) -> Result<(), PushError>;
}
```

Expand Down Expand Up @@ -182,41 +186,46 @@ classDiagram
high_prio_tx: mpsc::Sender<F>
low_prio_tx: mpsc::Sender<F>
}
class PushHandle {
class PushHandle~F~ {
+push_high_priority(frame: F): Result<(), PushError>
+push_low_priority(frame: F): Result<(), PushError>
+try_push(frame: F, priority: PushPriority, policy: PushPolicy): Result<(), PushError>
}
class PushQueues {
class PushQueues~F~ {
Comment on lines +189 to +194
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Ensure consistent generics notation.

The class names use ~F~ to indicate generics, but the Mermaid and Rust diagrams commonly use angle brackets (<F>). Consider switching to PushHandle<F> and PushQueues<F> to align with Rust syntax and avoid confusion with Mermaid’s tilde styling.

🤖 Prompt for AI Agents 
In docs/asynchronous-outbound-messaging-design.md around lines 185 to 190, the
generic type notation in class names uses tildes (~F~) which is inconsistent
with Rust and Mermaid conventions. Replace all occurrences of `~F~` with angle
brackets `<F>` in class names like `PushHandle<F>` and `PushQueues<F>` to align
with Rust syntax and improve clarity.

+high_priority_rx: mpsc::Receiver<F>
+low_priority_rx: mpsc::Receiver<F>
+bounded(high_capacity: usize, low_capacity: usize): (PushQueues, PushHandle)
+bounded(high_capacity: usize, low_capacity: usize): (PushQueues~F~, PushHandle~F~)
+recv(): Option<(PushPriority, F)>
Comment on lines +189 to 198
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Mermaid generics are clear, but the method arrows need consistency.

try_push now shows three parameters in the Rust snippet but only two in the diagram (frame, priority, policy → actually three). Double-check that the diagram text exactly matches the Rust order (frame, priority, policy) so readers aren’t forced to reconcile disparities.

🤖 Prompt for AI Agents 
In docs/asynchronous-outbound-messaging-design.md around lines 189 to 198, the
Mermaid diagram method `try_push` shows only two parameters while the Rust code
has three (frame, priority, policy). Update the diagram to list all three
parameters in the correct order: frame, priority, policy, ensuring it matches
the Rust method signature exactly for clarity and consistency.

}

PushHandleInner <.. PushHandle : contains
PushQueues o-- PushHandle : bounded()
PushHandleInner <.. PushHandle~F~ : contains
PushQueues~F~ o-- PushHandle~F~ : bounded(high_capacity, low_capacity)
PushHandle --> PushPriority
Comment on lines +201 to 203
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Arrow direction for “contains” looks inverted.

Mermaid’s composition/aggregation usually reads left to right as owner → part.
PushHandleInner <.. PushHandle~F~ : contains currently suggests inner depends on handle. Reversing gives a clearer visual:

-PushHandleInner <.. PushHandle~F~ : contains
+PushHandle~F~ *-- PushHandleInner : contains

[*-- draws a filled-diamond composition]

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PushHandleInner <.. PushHandle~F~ : contains
PushQueues~F~ o-- PushHandle~F~ : bounded(high_capacity, low_capacity)
PushHandle --> PushPriority
PushHandle~F~ *-- PushHandleInner : contains
PushQueues~F~ o-- PushHandle~F~ : bounded(high_capacity, low_capacity)
PushHandle --> PushPriority
🤖 Prompt for AI Agents 
In docs/asynchronous-outbound-messaging-design.md around lines 201 to 203, the
arrow direction for the "contains" relationship in the Mermaid diagram is
inverted, suggesting the wrong ownership direction. Reverse the arrow so it
reads from PushHandle to PushHandleInner, indicating that PushHandle owns
PushHandleInner. Use the filled-diamond composition arrow `*--` from PushHandle
to PushHandleInner to correctly represent ownership.

PushHandle --> PushPolicy
PushHandle --> PushError
PushQueues --> PushHandle
PushQueues --> FrameLike
PushHandle --> FrameLike
```

The diagram uses `~F~` to represent the `<F>` generic parameter because Mermaid
treats angle brackets as HTML.

```mermaid
flowchart TD
Producer[Producer]
PushHandle[PushHandle]
Handle[PushHandle~F~]
HighQueue[High Priority Queue]
LowQueue[Low Priority Queue]
Policy[PushPolicy]
Error[PushError or Drop]

Producer -->|push_high_priority / push_low_priority / try_push| PushHandle
PushHandle -->|High| HighQueue
PushHandle -->|Low| LowQueue
PushHandle -->|If queue full| Policy
Producer -->|push_high_priority| Handle
Handle -->|priority: High| HighQueue
Producer -->|push_low_priority| Handle
Handle -->|priority: Low| LowQueue

Producer -->|try_push| Policy
Policy -->|Queue available| Handle
Handle -->|priority: High| HighQueue
Handle -->|priority: Low| LowQueue
Policy -->|ReturnErrorIfFull| Error
Comment on lines +214 to 229
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Flowchart duplicates priority edges and may clutter comprehension.

The sequence:

Producer --> Handle
Handle --> HighQueue / LowQueue
Producer --> Policy --> Handle --> High/Low

re-draws the same Handle→Queue edges twice. Collapsing the second pair (226-228) into a single note “delegates to Handle” keeps the diagram readable without losing meaning.

🤖 Prompt for AI Agents 
In docs/asynchronous-outbound-messaging-design.md around lines 214 to 229, the
flowchart redundantly shows the Handle to HighQueue and LowQueue edges twice,
which clutters the diagram. To fix this, remove the duplicate Handle to
HighQueue and LowQueue edges in the second sequence (lines 226-228) and replace
them with a single note or label indicating that the Policy delegates to Handle,
preserving clarity without repeating the same connections.

Policy -->|DropIfFull| Error
Policy -->|WarnAndDropIfFull| Error
Expand Down Expand Up @@ -342,10 +351,14 @@ features of the 1.0 release.

## 7. Measurable Objectives & Success Criteria

<!-- markdownlint-disable MD013 -->

| Category | Objective | Success Metric |
| API Correctness | The PushHandle, SessionRegistry, and WireframeProtocol trait are implemented exactly as specified in this document. | 100% of the public API surface is present and correctly typed. |
| Functionality | Pushed frames are delivered reliably and in the correct order of priority. | A test with concurrent high-priority, low-priority, and streaming producers must show that all frames are delivered and that the final written sequence respects the strict priority order. |
| Back-pressure | A slow consumer must cause producer tasks to suspend without consuming unbounded memory. | A test with a slow consumer and a fast producer must show the producer's push().await call blocks, and the process memory usage remains stable. |
| Resilience | The SessionRegistry must not leak memory when connections are terminated. | A long-running test that creates and destroys thousands of connections must show no corresponding growth in the SessionRegistry's size or the process's overall memory footprint. |
| Performance | The overhead of the push mechanism should be minimal for connections that do not use it. | A benchmark of a simple request-response workload with the push feature enabled (but unused) should show < 2% performance degradation compared to a build without the feature. |
| Performance | The latency for a high-priority push under no contention should be negligible. | The time from push_high_priority().await returning to the frame being written to the socket buffer should be < 10µs. |

<!-- markdownlint-enable MD013 -->