Implement preamble callback tests #28
Conversation
Reviewer's GuideAdds generic connection preamble parsing to WireframeServer—providing type-parameter support, registration of decode success/failure callbacks and a local_addr helper—integrated into the accept loop; implements an async read_preamble helper for incremental bincode decoding; adds unit tests for parsing and callback behavior; and updates documentation and dependencies accordingly. Updated Class Diagram for WireframeServerclassDiagram
class WireframeServer~F,T~ {
-factory: F
-listener: Option~Arc~TcpListener~~
-workers: usize
-on_preamble_success: Option~Arc~dyn Fn(&T)+Send+Sync~~
-on_preamble_failure: Option~Arc~dyn Fn(DecodeError)+Send+Sync~~
-_preamble: PhantomData~T~
+new(factory: F, workers: usize) WireframeServer~F,()~
+with_preamble~U~() WireframeServer~F,U~
+with_workers(workers: usize) Self
+with_listener(listener: TcpListener) Self
+on_preamble_decode_success~H~(handler: H) Self
+on_preamble_decode_failure~H~(handler: H) Self
+worker_count() usize
+local_addr() Option~SocketAddr~
+bind(addr: &str) Result~Self~
+run() Result~()~
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update introduces a typed connection preamble to the server, allowing user-defined preamble structures and validation callbacks. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant PreambleDecoder
participant SuccessCallback
participant FailureCallback
Client->>Server: Connects and sends preamble bytes
Server->>PreambleDecoder: Reads and decodes preamble
alt Decode success
PreambleDecoder-->>Server: Decoded preamble (T)
Server->>SuccessCallback: Invoke with preamble data
else Decode failure
PreambleDecoder-->>Server: DecodeError
Server->>FailureCallback: Invoke with error
end
Server-->>Client: (Continues or closes connection)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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}
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Deep, Nested Complexity)
(1 file with Deep, Nested Complexity)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| server.rs | 1 rule in this hotspot | 10.00 → 9.39 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| server.rs | 1 critical rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![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 and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server.rs:87` </location>
<code_context>
+ /// Convert this server to parse a custom preamble `T`.
+ #[must_use]
+ pub fn with_preamble<T>(self) -> WireframeServer<F, T>
+ where
+ T: bincode::Decode<()> + Send + 'static,
</code_context>
<issue_to_address>
`with_preamble` resets any pre-existing handlers
Invoking `with_preamble` after registering handlers will remove them. Consider preserving handlers or documenting the required order of operations.
</issue_to_address>
### Comment 2
<location> `src/server.rs:283` </location>
<code_context>
+ handler(&preamble);
+ }
+ }
+ Err(err) => {
+ if let Some(handler) = on_failure.as_ref() {
+ handler(err);
+ }
</code_context>
<issue_to_address>
Connections with invalid preambles are still handed to the app
Ensure that after handling the failure, the stream is not forwarded to the application. Consider closing the connection or using `continue` to prevent processing invalid clients.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Cargo.toml (1)
9-16: Consider scoping new features & dev-deps more tightly
tokio’s extraio-utilfeature and the globallibcdev-dependency both pull additional code into the build.
• If the preamble logic only relies onAsyncReadExt::read_exact, you could enable justtokio/io-utiland dropsignal/syncto keep compile times lean.
•libcis test-only; guard it behind a feature flag or move the tests into acfg(unix)module to avoid Windows CI breakage.These are optional but worthwhile to keep the crate’s public surface and compile matrix minimal.
README.md (1)
20-21: Minor doc nit – keep bullet list sorted for scan-abilityTo preserve the conceptual order (transport → … → middleware), consider inserting the “Connection preamble” entry after “Framing layer” rather than at the end.
Purely cosmetic – feel free to ignore.src/server.rs (1)
32-34: PassDecodeErrorby reference to avoid needless cloningThe failure handler currently owns the whole
DecodeError.
Passing&DecodeErroravoids allocations and keeps API symmetric with the success callback:- on_preamble_failure: Option<Arc<dyn Fn(DecodeError) + Send + Sync>>, + on_preamble_failure: Option<Arc<dyn Fn(&DecodeError) + Send + Sync>>, ... - pub fn on_preamble_decode_failure<H>(mut self, handler: H) -> Self - where - H: Fn(DecodeError) + Send + Sync + 'static, + pub fn on_preamble_decode_failure<H>(mut self, handler: H) -> Self + where + H: Fn(&DecodeError) + Send + Sync + 'static,Also applies to: 151-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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/roadmap.md(1 hunks)src/lib.rs(1 hunks)src/preamble.rs(1 hunks)src/server.rs(6 hunks)tests/preamble.rs(1 hunks)
🔇 Additional comments (2)
src/lib.rs (1)
6-6: Public API surface expandedExposing
preamblemakes sense for consumer crates. No concerns here; just remember to re-export relevant error types (DecodeError) if you expect callers to handle them directly.docs/roadmap.md (1)
58-61: Great to see this milestone ticked offThe wording is clear and the ✔ status matches the implementation.
| pub struct WireframeServer<F, T = ()> | ||
| where | ||
| F: Fn() -> WireframeApp + Send + Sync + Clone + 'static, | ||
| T: bincode::Decode<()> + Send + 'static, | ||
| { |
There was a problem hiding this comment.
Generic Decode bound is wrong – compilation will fail
bincode::Decode is not parameterised with ().
Use an object-safe, blanket bound instead, e.g.
- T: bincode::Decode<()> + Send + 'static,
+ for<'de> T: bincode::Decode<'de> + Send + 'static,(or plain bincode::Decode for bincode 2.*).
Apply the same fix inside with_preamble.
Without this change the crate will not compile.
Also applies to: 85-99
🤖 Prompt for AI Agents
In src/server.rs around lines 24 to 28 and also lines 85 to 99, the generic
bound on T uses bincode::Decode with an incorrect parameter (), which causes
compilation errors. Remove the parameter from bincode::Decode to use it as a
plain trait bound (e.g., T: bincode::Decode + Send + 'static). Apply the same
correction inside the with_preamble function to fix the compilation issues.
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Deep, Nested Complexity)
(1 file with Deep, Nested Complexity)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| server.rs | 1 rule in this hotspot | 10.00 → 9.39 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| server.rs | 1 critical rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Gates Failed
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| preamble.rs | 1 critical rule | 10.00 → 9.84 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/preamble-validator.md (2)
9-9: Prefer consistent spelling (“summarised” → “summarized”).The rest of the documentation uses American English. Aligning this single occurrence avoids jarring spelling switches.
🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ... number of bytes required. The flow is summarised below: ```mermaid sequenceDiagram ...(OXFORD_SPELLING_Z_NOT_S)
31-32: Missing comma before the clause starting with “but”.A small readability-improving tweak:
- In the tests a `HotlinePreamble` struct illustrates the pattern, but any + In the tests, a `HotlinePreamble` struct illustrates the pattern, but any🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...inues or closes connection) ``` In the tests aHotlinePreamblestruct illustrates ...(AI_HYDRA_LEO_MISSING_COMMA)
src/preamble.rs (1)
61-63: Minor perf nit: recreateconfigonce, outside the loop.
configis immutable; constructing it every iteration is redundant. Hoist it above the loop:- let config = config::standard() - .with_big_endian() - .with_fixed_int_encoding(); + // Construct once to avoid repeated allocations. + let config = config::standard() + .with_big_endian() + .with_fixed_int_encoding();src/server.rs (1)
334-343: Accept loop blocks on each connection; consider spawning per-connection tasks.
process_streamis awaited directly, so the listener cannot accept another client until the previous preamble finishes decoding (or times out). On slow or malicious clients this serialises the entire worker.Spawn a dedicated task per accepted stream instead:
- Ok((stream, _)) => { - process_stream(stream, on_success, on_failure).await; + Ok((stream, _)) => { + tokio::spawn(process_stream(stream, on_success, on_failure)); delay = Duration::from_millis(10); }This lets each worker handle many concurrent handshakes, matching Tokio’s typical server pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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/preamble-validator.md(1 hunks)docs/roadmap.md(1 hunks)src/preamble.rs(1 hunks)src/server.rs(6 hunks)tests/preamble.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- Cargo.toml
- docs/roadmap.md
- tests/preamble.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/preamble.rs (2)
tests/preamble.rs (6)
tokio(61-61)tokio(62-62)tokio(119-119)tokio(120-120)read_preamble(36-36)read_preamble(52-52)src/server.rs (1)
read_preamble(359-359)
🪛 LanguageTool
docs/preamble-validator.md
[style] ~9-~9: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ... number of bytes required. The flow is summarised below: ```mermaid sequenceDiagram ...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...inues or closes connection) ``` In the tests a HotlinePreamble struct illustrates ...
(AI_HYDRA_LEO_MISSING_COMMA)
| pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError> | ||
| where | ||
| R: AsyncRead + Unpin, | ||
| T: Decode<()>, | ||
| { |
There was a problem hiding this comment.
Decode<()> bound is invalid for bincode 2.*; compilation will fail.
Decode is generic over a lifetime, not a unit type. Use a lifetime-agnostic bound instead:
-pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError>
-where
- R: AsyncRead + Unpin,
- T: Decode<()> ,
+pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError>
+where
+ R: AsyncRead + Unpin,
+ for<'de> T: Decode<'de>,📝 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.
| pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError> | |
| where | |
| R: AsyncRead + Unpin, | |
| T: Decode<()>, | |
| { | |
| pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError> | |
| where | |
| R: AsyncRead + Unpin, | |
| for<'de> T: Decode<'de>, | |
| { | |
| // …rest of function body… | |
| } |
🤖 Prompt for AI Agents
In src/preamble.rs around lines 55 to 59, the generic bound `T: Decode<()>` is
incorrect because `Decode` expects a lifetime parameter, not a unit type.
Replace `Decode<()>` with a lifetime-agnostic bound such as `for<'de>
Decode<'de>` to fix the compilation error.
| pub struct WireframeServer<F, T = ()> | ||
| where | ||
| F: Fn() -> WireframeApp + Send + Sync + Clone + 'static, | ||
| T: bincode::Decode<()> + Send + 'static, | ||
| { | ||
| factory: F, | ||
| listener: Option<Arc<TcpListener>>, | ||
| workers: usize, | ||
| on_preamble_success: Option<Arc<dyn Fn(&T) + Send + Sync>>, | ||
| on_preamble_failure: Option<Arc<dyn Fn(&DecodeError) + Send + Sync>>, | ||
| _preamble: PhantomData<T>, | ||
| } |
There was a problem hiding this comment.
Fix generic bound to restore buildability across the crate.
The same incorrect Decode<()> bound appears in several spots and prevents compilation with current bincode. Replace it everywhere with the lifetime-parametrised form:
- T: bincode::Decode<()> + Send + 'static,
+ for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the change to:
- Struct definition (≈ lines 24-29)
with_preamblemethod bound (≈ lines 90-93)worker_taskgeneric bound (≈ lines 328-329)process_streamgeneric bound (≈ lines 357-358)
This single edit unblocks the build and the tests.
Also applies to: 90-95, 328-329, 357-358
🤖 Prompt for AI Agents
In src/server.rs around lines 24 to 35, the generic bound on T uses
bincode::Decode<()> which is incorrect and causes build failures. Replace all
occurrences of bincode::Decode<()> with bincode::Decode<'static> to fix the
lifetime parameter issue. Apply this fix not only in the struct definition but
also in the with_preamble method bounds (lines 90-95), worker_task generic
bounds (lines 328-329), and process_stream generic bounds (lines 357-358) to
restore buildability across the crate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/server.rs (3)
27-28:⚠️ Potential issueCompilation will still fail –
bincode::Decode<()>is not a valid bound
Previous reviews already pointed this out. All occurrences ofT: bincode::Decode<()>need the lifetime-parametrised form; otherwise the crate will not compile with currentbincodeversions.- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same change everywhere that bound appears (struct definition,
with_preamble,implblock,worker_task,process_stream).Also applies to: 92-93, 108-109, 321-322, 352-353
90-102: 🛠️ Refactor suggestion
with_preamblesilently resets registered handlers
Callingwith_preambleafteron_preamble_decode_*drops the previously set callbacks without warning. This can surprise users and recreate bugs reported in the last review. Either:
- Preserve existing handlers when the generic type remains
(), or- Document prominently (and maybe enforce through the type system) that
with_preamblemust be called first.At minimum emit a
debug_assert!orlog::warn!when handlers are discarded.
354-359:⚠️ Potential issueLeft-over bytes from
read_preambleare still discarded
read_preamblereturns(preamble, leftover)to preserve early application data, butleftoveris ignored. This was raised before and still breaks framing for the first packet after the preamble.Pass
leftoverto the application layer (e.g., by wrapping the stream so it replays those bytes).
🧹 Nitpick comments (2)
src/server.rs (2)
300-307: Swallowed join errors – worker task panics are lost
join_all.await;discards eachJoinHandle’sResult<_, tokio::task::JoinError>. If a worker panics the server will exit “successfully”, masking the fault.- join_all.await; + for res in join_all.await { + if let Err(e) = res { + // propagate or at least log + eprintln!("worker task failed: {e}"); + } + }Forwarding the first error (or aggregating them) makes production failures observable.
314-344:appis created then never used
worker_taskinstantiatesWireframeApponly todropit at the end. Unless the intent is to initialise global state vianew(), this is dead work and may hide a bug (e.g., you meant to pass the app intoprocess_stream).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/preamble-validator.md(1 hunks)src/preamble.rs(1 hunks)src/server.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/preamble-validator.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/preamble.rs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server.rs (2)
24-30:⚠️ Potential issueIncorrect
bincode::Decode<()>bound – crate still fails to compileThe generic constraint
T: bincode::Decode<()>is invalid for every currentbincoderelease (Decodeis lifetime-parametrised).
This exact problem was already raised in earlier reviews and remains unfixed at four call-sites (struct definition,with_preamble,worker_task,process_stream).- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same replacement to all occurrences listed above to restore buildability.
Failing to do so prevents the project from compiling and blocks the CI pipeline.Also applies to: 92-97, 324-327, 357-359
361-368: 🛠️ Refactor suggestionLeft-over bytes from
read_preambleare still discarded
read_preamblereturns(preamble, leftover)precisely so that any early application data is preserved, yet the implementation continues to ignoreleftover(commented as a TODO). This means the first bytes after the preamble vanish, corrupting the higher-level protocol.A minimal fix is to replay
leftoverbefore handing the stream to the application, e.g.:- Ok((preamble, _leftover)) => { + Ok((preamble, leftover)) => { if let Some(handler) = on_success.as_ref() { handler(&preamble); } - // TODO: hand off `stream` **and** `leftover` to the application layer, - // e.g. by wrapping the stream in a struct that replays `leftover` - // before delegating to the underlying socket. + // Example wrapper; adjust once the application layer is in place + // let stream = ReplayingStream::new(stream, leftover); + // app.handle(stream).await; }Until this is implemented, every client will lose data immediately after the preamble.
🧹 Nitpick comments (1)
src/server.rs (1)
328-349:appinstance is created but never used – unnecessary resource allocation
worker_taskinstantiates an application (let app = (factory)();) but does nothing with it except dropping it on shutdown. Besides wasting memory, any constructor side-effects (e.g., spawning tasks or opening resources) happen for no reason.Either:
- Pass
app(or a clone/Arc) down toprocess_stream, or- Remove the instantiation until the connection-handling path is implemented.
- let app = (factory)(); + // TODO: instantiate the app only when it is actually used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/preamble.rs(1 hunks)src/server.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/preamble.rs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server.rs (2)
24-30:⚠️ Potential issueIncorrect
bincode::Decodebound still breaks the buildThe crate will not compile with
bincode 2.*:Decodeis lifetime-parametrised, not context-parametrised.
Replace allT: bincode::Decode<()>constraints with the higher-ranked form:- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same change in the struct definition,
with_preamble, the impl block,worker_task, andprocess_stream.Failure to fix this results in:
error[E0404]: expected trait, found struct `bincode::Decode`Also applies to: 92-96, 112-113, 330-331, 360-361
362-370: 🛠️ Refactor suggestionLeft-over bytes from
read_preambleare still discarded
read_preamblereturns(preamble, leftover)but_leftoveris dropped.
Any early application data already read is lost, corrupting the first frame.Suggested quick fix:
- Ok((preamble, _leftover)) => { + Ok((preamble, leftover)) => { if let Some(handler) = on_success.as_ref() { handler(&preamble); } - // TODO: hand off `stream` **and** `leftover` to the application layer, - // e.g. by wrapping the stream in a struct that replays `leftover` - // before delegating to the underlying socket. + // TODO: wrap `stream` so that `leftover` is replayed before + // delegating to the underlying socket (e.g. `RewindableStream`). }Until this is addressed, higher-level protocols will mis-align.
🧹 Nitpick comments (1)
src/server.rs (1)
322-330: Unusedfactoryparameter inworker_task
_factoryis never used, making the genericFsuperfluous and inflating monomorphised code size.Options:
- Remove the parameter and generic if not needed yet.
- Prefix with double underscore (
__factory) and add#[allow(unused_variables)]to silence the warning until the implementation is complete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server.rs(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/server.rs (4)
src/preamble.rs (1)
read_preamble(55-79)tests/preamble.rs (4)
read_preamble(36-36)read_preamble(52-52)WireframeServer(65-67)WireframeServer(123-125)src/extractor.rs (1)
new(84-86)src/middleware.rs (1)
new(44-46)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server.rs (2)
87-105:with_preamblesilently drops previously registered handlersCalling
with_preambleafteron_preamble_*wipes those callbacks without warning.
Either preserve the handlers when the preamble type stays the same, or document the required call order explicitly.
24-30:⚠️ Potential issue
bincode::Decode<()>bound is still incorrect – build will fail
bincode::Decodeis lifetime-parametrised; using the unit context breaks compilation with currentbincode 2.*.
Replace everyDecode<()>bound withfor<'de> bincode::Decode<'de>(or plainbincode::Decodeif on 2.0-beta).This occurs at:
- struct definition (24-30)
with_preamblegeneric bound (95-96)- impl block header (112-113)
worker_taskbound (326-328)process_streambound (356-358)Also applies to: 95-96, 112-113, 326-328, 356-358
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/server.rs is usually changed with: wireframe/src/extractor.rs, wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server.rs (2)
28-31:⚠️ Potential issue
bincode::Decode<()>is still the wrong bound – the crate will not compileAll highlighted spots keep the erroneous
Decode<()>constraint.
Decodeis parametrised over a lifetime, not an arbitrary type – using()leads to a type-mismatch error on currentbincode2.x.- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same substitution everywhere the bound appears (
struct,with_preamble,worker_task,process_stream, etc.).
This was pointed out in earlier reviews but has not yet been addressed.Also applies to: 95-97, 326-329, 357-359
360-367: 🛠️ Refactor suggestionSuccessful preamble connections are immediately dropped
RewindStreamis created and bound to_stream, but nothing ever forwards it to the application layer; the stream is dropped when the function returns, closing the socket right after a “successful” handshake.
The original concern about forwarding both the socket and leftover bytes therefore remains unresolved.- Ok((preamble, leftover)) => { - let _stream = RewindStream::new(leftover, stream); - if let Some(handler) = on_success.as_ref() { - handler(&preamble); - } - // `RewindStream` plays back leftover bytes before using the socket. - } + Ok((preamble, leftover)) => { + if let Some(handler) = on_success.as_ref() { + handler(&preamble); + } + let stream = RewindStream::new(leftover, stream); + // TODO: hand `stream` off to the WireframeApp so the connection + // is actually served instead of being closed here. + }Until this hand-off is implemented, valid clients will experience an immediate disconnect.
🧹 Nitpick comments (1)
src/rewind_stream.rs (1)
45-47: Reset the cursor when the buffer is cleared
self.leftover.clear()drops the bytes yet leavesself.pospointing past the end of the now-empty vector.
While harmless for current logic, keeping the index in sync avoids future surprises and removes the need to reason about an out-of-bounds cursor.- if self.pos >= self.leftover.len() && !self.leftover.is_empty() { - self.leftover.clear(); + if self.pos >= self.leftover.len() && !self.leftover.is_empty() { + self.leftover.clear(); + self.pos = 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib.rs(1 hunks)src/rewind_stream.rs(1 hunks)src/server.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
|
@coderabbit-ai Please create a github issue for this: 28-31: bincode::Decode<()> is still the wrong bound – the crate will not compile All highlighted spots keep the erroneous Decode<()> constraint.
Apply the same substitution everywhere the bound appears (struct, with_preamble, worker_task, process_stream, etc.). Also applies to: 95-97, 326-329, 357-359 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for the bincode::Decode bound compilation problem. 🏁 Script executed: Length of output: 1120 Perfect! I've successfully created GitHub issue #31 for the bincode::Decode bound compilation problem. The issue has been assigned to you and includes all the relevant details about the incorrect type bounds that need to be fixed. Issue created: #31 The issue documents the compilation errors with |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/server.rs is usually changed with: wireframe/src/extractor.rs, wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server.rs (2)
28-31: Incorrectbincode::Decode<()>bound – build still broken
All occurrences still use the context-parameterised form (Decode<()>).
bincode 2.xdefinestrait Decode<'de>(lifetime, not context). The current bound therefore fails to compile.- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same change everywhere shown above.
Also applies to: 93-97, 112-114, 331-332, 364-365
90-92:with_preamblesilently drops previously-set handlers
Callingwith_preambleafter registering success / failure callbacks resets them toNone, surprising users. Either:
- Document the required call order loudly, or
- Carry the existing
Arc<…>values across to the new instance.
🧹 Nitpick comments (2)
src/app.rs (1)
85-96:handle_connectionis a stub that immediately closes the socket
The method yields once and returns, so every accepted connection is torn down right after the pre-amble phase. Until real frame handling is implemented, consider at least logging a warning or gating server code so it is not invoked in production. This avoids confusing behaviour when integration tests start using the API.src/server.rs (1)
341-342: Doubletokio::spawnintroduces unnecessary task churn
listener.accept()➔spawn(process_stream)➔ insideprocess_streamanotherspawn(app.handle_connection).
Unless the inner spawn isolates crash-safety or blocking work, you can callapp.handle_connectiondirectly inprocess_streamand save one scheduling hop.- tokio::spawn(async move { - app.handle_connection(stream).await; - }); + app.handle_connection(stream).await;Also applies to: 373-376
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.rs(1 hunks)src/rewind_stream.rs(1 hunks)src/server.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rewind_stream.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/server.rs (3)
src/preamble.rs (1)
read_preamble(55-79)tests/preamble.rs (8)
read_preamble(36-36)read_preamble(52-52)WireframeServer(65-67)WireframeServer(123-125)tokio(61-61)tokio(62-62)tokio(119-119)tokio(120-120)src/rewind_stream.rs (1)
new(18-24)
|
@coderabbitai Please create github issues for these: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create separate GitHub issues for each of these code quality concerns. Perfect! I've created three separate GitHub issues for the code quality concerns you identified:
Each issue includes detailed context, code locations, suggested solutions, and references back to this PR and your review comments. All issues have been assigned to you for tracking and resolution. 🏁 Script executed: Length of output: 3806 Perfect! All three GitHub issues have been successfully created: ✅ Issue #32: with_preamble method silently drops previously-set callback handlers
✅ Issue #33: handle_connection method is a stub that immediately closes connections
✅ Issue #34: Double tokio::spawn introduces unnecessary task scheduling overhead
Each issue includes detailed problem descriptions, affected code locations, suggested solutions, and proper references back to this PR. All issues have been assigned to you for tracking and resolution. This should help organize the follow-up work needed to address these code quality concerns. |
leynos
force-pushed
the
codex/add-connection-preamble-support
branch
from
cc93638 to
8c824fc
Compare
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/server.rs is usually changed with: wireframe/src/extractor.rs, wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai new github issue please: error: variables can be used directly in the error: could not compile |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for the Clippy uninlined format arguments error. 🏁 Script executed: Length of output: 1335 Perfect! I've successfully created GitHub issue #35 for the Clippy linting error. ✅ Issue #35: Fix clippy::uninlined_format_args lint error in tests/preamble.rs The issue documents the compilation failure caused by the uninlined format arguments lint, provides the exact error location (line 39 in tests/preamble.rs), and includes the simple fix to update |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server.rs (2)
337-344: Still double-spawning per connection
worker_taskspawnsprocess_stream, which in turn spawns the
application’shandle_connection. This yields two tasks for every accepted
socket (three with the outer worker), adding needless scheduler overhead and
context switches. Consider callinghandle_connectiondirectly from
process_streaminstead of wrapping it in anothertokio::spawn, or remove
theprocess_streamlayer entirely.(See previous review; behaviour unchanged.)
Also applies to: 373-376
28-31:⚠️ Potential issue
bincode::Decode<()>is still the wrong bound – the crate will not compileThe same incorrect context-type parameter persists in every generic bound.
Decodeis lifetime-parametrised (Decode<'de>from bincode 2.*), not context-parametrised, soDecode<()>does not exist and the compiler rejects it.- T: bincode::Decode<()> + Send + 'static, + for<'de> T: bincode::Decode<'de> + Send + 'static,Apply the same fix to:
- the struct definition (lines 28-31)
with_preamble(lines 95-97)- the impl block header (lines 113-114)
worker_task(lines 330-332)process_stream(lines 364-365)Without this change the PR does not build, blocking every downstream consumer.
Also applies to: 95-97, 113-114, 330-332, 364-365
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)README.md(1 hunks)docs/preamble-validator.md(1 hunks)docs/roadmap.md(1 hunks)src/app.rs(1 hunks)src/lib.rs(1 hunks)src/preamble.rs(1 hunks)src/rewind_stream.rs(1 hunks)src/server.rs(6 hunks)tests/preamble.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- Cargo.toml
- README.md
- src/lib.rs
- docs/roadmap.md
- docs/preamble-validator.md
- src/preamble.rs
- src/rewind_stream.rs
- tests/preamble.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/server.rs (3)
src/preamble.rs (1)
read_preamble(55-79)tests/preamble.rs (8)
read_preamble(36-36)read_preamble(52-52)WireframeServer(65-67)WireframeServer(123-125)tokio(61-61)tokio(62-62)tokio(119-119)tokio(120-120)src/rewind_stream.rs (1)
new(18-24)
| // Spawn worker tasks. | ||
| let mut handles = Vec::with_capacity(self.workers); | ||
| for _ in 0..self.workers { | ||
| let mut shutdown_rx = shutdown_tx.subscribe(); | ||
| let listener = Arc::clone(&listener); | ||
| let factory = self.factory.clone(); | ||
| let on_success = self.on_preamble_success.clone(); | ||
| let on_failure = self.on_preamble_failure.clone(); | ||
| let mut shutdown_rx = shutdown_tx.subscribe(); | ||
| handles.push(tokio::spawn(async move { | ||
| let app = (factory)(); | ||
| let mut delay = Duration::from_millis(10); | ||
| loop { | ||
| tokio::select! { | ||
| res = listener.accept() => match res { | ||
| Ok((_stream, _)) => { | ||
| // TODO: hand off stream to `app` | ||
| delay = Duration::from_millis(10); | ||
| } | ||
| Err(e) => { | ||
| eprintln!("accept error: {e}"); | ||
| sleep(delay).await; | ||
| delay = (delay * 2).min(Duration::from_secs(1)); | ||
| } | ||
| }, | ||
| _ = shutdown_rx.recv() => break, | ||
| } | ||
| } | ||
| drop(app); | ||
| worker_task(listener, factory, on_success, on_failure, &mut shutdown_rx).await; | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass the shutdown receiver by value, not &mut
Each worker owns its own broadcast::Receiver. Capturing &mut shutdown_rx inside an
async move closure creates an unnecessary lifetime dance and misleads the
reader into thinking the receiver outlives the task.
-async fn worker_task<F, T>(
+async fn worker_task<F, T>(
listener: Arc<TcpListener>,
factory: F,
on_success: Option<Arc<dyn Fn(&T) + Send + Sync>>,
on_failure: Option<Arc<dyn Fn(&DecodeError) + Send + Sync>>,
- shutdown_rx: &mut broadcast::Receiver<()>,
+) mut shutdown_rx: broadcast::Receiver<()>,
) where
@@
-let mut shutdown_rx = shutdown_tx.subscribe();
-tokio::spawn(async move {
- worker_task(listener, factory, on_success, on_failure, &mut shutdown_rx).await;
-});
+let shutdown_rx = shutdown_tx.subscribe();
+tokio::spawn(worker_task(listener, factory, on_success, on_failure, shutdown_rx));This removes the mutable reference, simplifies ownership, and avoids hidden
self-references inside the spawned future.
Also applies to: 321-333
🤖 Prompt for AI Agents
In src/server.rs around lines 289 to 299, the shutdown receiver is passed as a
mutable reference (&mut shutdown_rx) into the async move closure for each worker
task. This causes unnecessary lifetime complexity and confusion about ownership.
To fix this, pass shutdown_rx by value into the closure instead of by mutable
reference, so each worker owns its receiver directly. Apply the same change to
lines 321 to 333 where a similar pattern occurs.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai issue github
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for the shutdown receiver refactor suggestion.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the shutdown receiver refactor suggestion
gh issue create \
--title "Refactor: Pass shutdown receiver by value instead of mutable reference in worker tasks" \
--body "## Summary
The current implementation passes the shutdown receiver as a mutable reference (\`&mut shutdown_rx\`) to worker tasks, which creates unnecessary lifetime complexity and ownership confusion.
## Problem
Each worker task owns its own \`broadcast::Receiver\`, but the current approach of capturing \`&mut shutdown_rx\` inside an \`async move\` closure creates an unnecessary lifetime dance and misleads readers about the actual ownership model.
## Suggested Solution
Pass the shutdown receiver by value to each worker task so that each worker owns its receiver directly. This would simplify ownership semantics and avoid hidden self-references inside spawned futures.
## Context
This issue was identified during code review and affects the worker task spawning logic in the server implementation.
## References
- PR: https://github.com/leynos/wireframe/pull/28
- Comment: https://github.com/leynos/wireframe/pull/28#discussion_r2147271869
- Requested by: @leynos" \
--assignee leynos
Length of output: 1158
@leynos I've successfully created GitHub issue #36 for the shutdown receiver refactor suggestion: #36
The issue documents the recommendation to pass the shutdown receiver by value instead of by mutable reference to simplify ownership semantics and avoid unnecessary lifetime complexity in the worker tasks. It's been assigned to you for follow-up.
🐰
/| /|
( . .)
) (
( v )
^^ ^ ^^
🧬 Code Graph Analysis Results
Relevant snippets from src/rewind_stream.rs (lines 18-24)
pub fn new(leftover: Vec<u8>, inner: S) -> Self {
Self {
leftover,
pos: 0,
inner,
}
}This snippet shows the constructor for RewindStream, which wraps an inner stream and holds leftover bytes from preamble reading. It is relevant because process_stream in src/server.rs uses RewindStream::new(leftover, stream) to create a stream that first yields the leftover bytes before reading from the underlying TCP stream.
Relevant snippets from src/preamble.rs (lines 55-79)
pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError>
where
R: AsyncRead + Unpin,
// `Decode` expects a decoding context type, not a lifetime. Most callers
// use the unit type as the context, which requires no external state.
T: Decode<()>,
{
let mut buf = Vec::new();
// Build the decoder configuration once to avoid repeated allocations.
let config = config::standard()
.with_big_endian()
.with_fixed_int_encoding();
loop {
match decode_from_slice::<T, _>(&buf, config) {
Ok((value, consumed)) => {
let leftover = buf.split_off(consumed);
return Ok((value, leftover));
}
Err(DecodeError::UnexpectedEnd { additional }) => {
read_more(reader, &mut buf, additional).await?;
}
Err(e) => return Err(e),
}
}
}This async function reads and decodes a preamble of type T from an async reader, returning the decoded preamble and any leftover bytes. It is directly used in process_stream in src/server.rs to parse the connection preamble.
Relevant snippets from tests/preamble.rs (lines 36, 52, 61-62, 65-67, 119-125)
// Usage of read_preamble with a specific preamble type `HotlinePreamble`
read_preamble::<_, HotlinePreamble>
// Usage of WireframeServer with preamble type HotlinePreamble
WireframeServer::new(factory)
.workers(1)
.with_preamble::<HotlinePreamble>()These snippets show test usage of read_preamble and WireframeServer configured with a custom preamble type, illustrating how the generic preamble decoding is exercised in tests. This is relevant for understanding how the server handles preamble decoding success and failure callbacks.
Summary of relevance:
src/rewind_stream.rssnippet explains the construction of theRewindStreamused to prepend leftover bytes after preamble reading.src/preamble.rssnippet provides the implementation ofread_preambleused to decode the connection preamble.tests/preamble.rssnippets illustrate practical usage of preamble decoding and server configuration with preamble types, relevant for understanding the server's preamble handling logic.
No other snippets from the provided files appear directly relevant to the server's core functionality or the user comment context.
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I've created a GitHub issue to track the refactor suggestion about passing the shutdown receiver by value instead of mutable reference. This will help simplify ownership and remove unnecessary lifetime complexity in the worker task spawning logic.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for the shutdown receiver refactor suggestion
gh issue create \
--title "Refactor: Pass shutdown receiver by value instead of mutable reference" \
--body "## Description
The current implementation passes the shutdown receiver as a mutable reference (\`&mut shutdown_rx\`) into async move closures for worker tasks, creating unnecessary lifetime complexity and confusion about ownership.
## Affected Code
**File:** \`src/server.rs\`
**Lines:** 289-299 and 321-333
## Current Implementation Issue
Each worker owns its own \`broadcast::Receiver\`, but capturing \`&mut shutdown_rx\` inside an \`async move\` closure creates an unnecessary lifetime dance and misleads readers into thinking the receiver outlives the task.
## Suggested Solution
Pass \`shutdown_rx\` by value into the closure instead of by mutable reference, so each worker owns its receiver directly.
**Changes needed:**
1. Update \`worker_task\` function signature to take \`mut shutdown_rx: broadcast::Receiver<()>\` instead of \`shutdown_rx: &mut broadcast::Receiver<()>\`
2. Update spawn calls to pass receiver by value instead of mutable reference
3. Remove \`async move\` complexity around receiver ownership
## Benefits
- Simplifies ownership model
- Removes unnecessary lifetime complexity
- Avoids hidden self-references inside spawned futures
- Makes code more readable and maintainable
## References
- **PR:** https://github.com/leynos/wireframe/pull/28
- **Comment:** https://github.com/leynos/wireframe/pull/28#discussion_r2147271869
**Requested by:** leynos" \
--assignee leynos
Length of output: 1543
@leynos ✅ GitHub issue created successfully!
Issue #37: Refactor: Pass shutdown receiver by value instead of mutable reference
The issue tracks the refactor suggestion to simplify ownership by passing shutdown_rx by value instead of mutable reference in the worker task spawning logic. It's been assigned to you and includes all the relevant details and backlinks to this PR and comment.
🧬 Code Graph Analysis Results
Relevant snippets from src/rewind_stream.rs (lines 18-24)
pub fn new(leftover: Vec<u8>, inner: S) -> Self {
Self {
leftover,
pos: 0,
inner,
}
}This snippet shows the constructor for RewindStream, which wraps an inner stream and holds leftover bytes from preamble reading. It is relevant because process_stream in src/server.rs uses RewindStream::new to combine leftover bytes with the underlying TCP stream.
Relevant snippets from src/preamble.rs (lines 55-79)
pub async fn read_preamble<R, T>(reader: &mut R) -> Result<(T, Vec<u8>), DecodeError>
where
R: AsyncRead + Unpin,
T: Decode<()>,
{
let mut buf = Vec::new();
let config = config::standard()
.with_big_endian()
.with_fixed_int_encoding();
loop {
match decode_from_slice::<T, _>(&buf, config) {
Ok((value, consumed)) => {
let leftover = buf.split_off(consumed);
return Ok((value, leftover));
}
Err(DecodeError::UnexpectedEnd { additional }) => {
read_more(reader, &mut buf, additional).await?;
}
Err(e) => return Err(e),
}
}
}This async function reads and decodes a preamble of type T from an async reader, returning the decoded value and any leftover bytes. It is directly called by process_stream in src/server.rs to parse the connection preamble.
Relevant snippets from tests/preamble.rs (lines 36, 52, 61, 62, 65-67, 119-125)
read_preamble::<_, HotlinePreamble>and
WireframeServer::new(factory)
.workers(1)
.with_preamble::<HotlinePreamble>These test snippets show usage of read_preamble with a concrete preamble type HotlinePreamble and configuring WireframeServer with a custom preamble type. They provide context on how the server and preamble reading are tested and used.
Relevant snippets from src/app.rs (lines 44-46)
pub fn new() -> Result<Self> {
Ok(Self::default())
}This snippet shows the constructor for WireframeApp, which is the application instance created per connection in the server. It is relevant because WireframeServer uses a factory closure returning WireframeApp instances.
Summary of relevance:
src/rewind_stream.rssnippet explains the construction of theRewindStreamwrapper used inprocess_stream.src/preamble.rssnippet explains theread_preambleasync function used to decode the connection preamble.tests/preamble.rssnippets illustrate usage patterns ofread_preambleandWireframeServerwith custom preambles.src/app.rssnippet shows theWireframeAppconstructor used by the server factory closure.
These snippets provide foundational understanding of the server's connection handling, preamble decoding, and application instantiation.
1 participant
Summary
local_addrhelper onWireframeServeron_preamble_decode_successandon_preamble_decode_failureTesting
cargo fmt --allcargo clippy -- -D warningsRUSTFLAGS="-D warnings" cargo testmarkdownlint docs/roadmap.md README.mdhttps://chatgpt.com/codex/tasks/task_e_684cd0cc1ec48322b9cdb8a050c6957e
Summary by Sourcery
Implement generic connection preamble parsing and callback support in
WireframeServer, including theread_preamblehelper, new server API methods, documentation updates, and comprehensive testingNew Features:
read_preamblehelper for incremental bincode decoding of connection preamblesWireframeServerwithwith_preamble,on_preamble_decode_success,on_preamble_decode_failure, andlocal_addrmethodspreamblemodule and document connection preamble support in READMEBuild:
libcas a dev-dependency for signal handling in testsio-utilfeature for Tokio inCargo.tomlDocumentation:
docs/roadmap.mdTests:
preamble.rsSummary by CodeRabbit
New Features
Documentation
Tests