Treat UnexpectedEnd as graceful session closure

[mondain]

Problem

When the CONNECT stream receives UnexpectedEnd during capsule reading,
the session closes with error code 1. However, UnexpectedEnd typically
means the peer closed their end of the stream normally, which is a valid
way to end a WebTransport session.

This causes issues when integrating with clients that close the CONNECT
stream to signal session end rather than sending a CloseWebTransportSession
capsule.

Solution

• Distinguish between UnexpectedEnd (graceful, code 0) and other errors (code 1)
• Add debug/info logging for capsule reader lifecycle
• Include actual error details in error messages for debugging

Changes

• UnexpectedEnd now returns code 0 ("session closed") instead of code 1
• Added logging for capsule reader start, graceful close, and errors
• Renamed _send to _send_keep_alive for clarity (documents why we keep it)

[coderabbitai]

Walkthrough

This pull request makes the RecvStream::new and SendStream::new constructors publicly accessible by changing their visibility from pub(crate) to pub. Additionally, internal changes to session.rs enhance error handling in the run_closed function to differentiate between graceful closure and read errors, and add debug logging throughout the session lifecycle including stream acceptance, bidirectional stream opening, and stream decoding operations.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly summarizes the main change: treating UnexpectedEnd as a graceful session closure, which is the core problem-solution focus of this PR.
Description check	✅ Passed	The description is well-structured, clearly explaining the problem (UnexpectedEnd causing error code 1), the solution (treat as graceful with code 0), and the specific changes made, all directly related to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

web-transport-quinn/src/session.rs (4)

96-125: Graceful handling of UnexpectedEnd is correct, but detection via string match is brittle

The behavioral change to treat EOF on the CONNECT capsule stream as a graceful WebTransport session closure (returning (0, "session closed")) matches the intent of mapping CapsuleError::UnexpectedEnd to a clean shutdown, and the _send_keep_alive binding correctly keeps the send half alive to avoid sending a FIN prematurely.

However, detecting UnexpectedEnd by checking format!("{e:?}").contains("UnexpectedEnd") is fragile and could break if the Debug representation of the error ever changes, or if other error variants happen to include the same substring.

If possible, consider matching the concrete error variant instead of relying on its string form, e.g.:

match web_transport_proto::Capsule::read(&mut recv).await {
    // ...
-   Err(e) => {
-       let error_str = format!("{e:?}");
-       if error_str.contains("UnexpectedEnd") {
-           log::info!("WebTransport CONNECT stream closed by peer (graceful)");
-           return (0, "session closed".to_string());
-       } else {
-           log::error!("WebTransport CONNECT stream capsule read error: {e:?}");
-           return (1, format!("capsule error: {e:?}"));
-       }
-   }
+   Err(web_transport_proto::CapsuleError::UnexpectedEnd) => {
+       log::info!("WebTransport CONNECT stream closed by peer (graceful)");
+       return (0, "session closed".to_string());
+   }
+   Err(e) => {
+       log::error!("WebTransport CONNECT stream capsule read error: {e:?}");
+       return (1, format!("capsule error: {e:?}"));
+   }
}

(or whatever concrete path to the error type is exposed by web_transport_proto).

This keeps the new semantics while making the match robust to formatting changes.

---

195-201: Replace println! in open_bi with structured logging or a debug-only path

The detailed diagnostics for open_bi (stream ID, index, header bytes in hex) are useful, but unconditional println! calls in a library will spam stdout and add allocation/formatting overhead for every bidirectional stream opened.

Consider switching to the existing log macros at an appropriate level (e.g. log::debug! or log::trace!) or gating this behind a feature/compile-time flag:

let hex_str: String = self.header_bi.iter().map(|b| format!("{:02x}", b)).collect::<Vec<_>>().join(" ");
let stream_id_u64: u64 = send.id().into();
log::debug!(
    "open_bi: QUIC stream ID: {} (index={}, {}), writing header ({} bytes): [{}]",
    stream_id_u64,
    send.id().index(),
    send.id(),
    self.header_bi.len(),
    hex_str,
);
// ...
log::debug!("open_bi: QUIC stream ID {} header written successfully", stream_id_u64);

This keeps the observability benefit while respecting caller control over logging output.

---

473-500: Avoid println! in poll_accept_bi in favor of existing logging infrastructure

The println! calls that log accepted and validated stream IDs are helpful for debugging, but they unconditionally write to stdout in all builds, which is undesirable for a reusable transport library.

Prefer using log::debug! or log::trace! here so that applications can control verbosity via logger configuration:

let (send, recv) = res?;
log::debug!("poll_accept_bi: accepted stream ID: {}, queuing for decode", recv.id());
let pending = Self::decode_bi(send, recv, self.session_id);
// ...
if let Some((send, recv)) = res {
    log::debug!("poll_accept_bi: returning validated stream ID: {} to application", recv.id());
    // wrap and return
}

This keeps the tracing detail without hard-wiring I/O to stdout.

---

512-535: Consolidate decode_bi diagnostics under the logging framework

The new println! calls in decode_bi (start marker, type byte, session ID, mismatch, success) provide excellent visibility into bidirectional stream decoding, but for a library they should ideally go through the logging system rather than direct println!s, and possibly at debug/trace level:

log::debug!("decode_bi: start - stream ID: {}", recv.id());
// ...
log::debug!(
    "decode_bi: stream {} - read type byte: 0x{:02x} (expected WEBTRANSPORT = 0x41)",
    recv.id(),
    typ.into_inner(),
);
// and similar for mismatch, session_id, success

That way users can enable this rich tracing when needed without incurring unconditional stdout noise and overhead in all environments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 21f3417 and 626d59f.

📒 Files selected for processing (3)

• web-transport-quinn/src/recv.rs (1 hunks)
• web-transport-quinn/src/send.rs (1 hunks)
• web-transport-quinn/src/session.rs (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

web-transport-quinn/src/recv.rs (1)

web-transport-quinn/src/send.rs (1)

• new (21-23)

web-transport-quinn/src/send.rs (1)

web-transport-quinn/src/recv.rs (1)

• new (18-20)

web-transport-quinn/src/session.rs (3)

web-transport-quinn/src/settings.rs (1)

• connect (37-44)

web-transport-proto/src/capsule.rs (1)

• read (74-88)

web-transport-quinn/src/connect.rs (2)

• into_inner (103-105)
• session_id (91-96)

🔇 Additional comments (2)

web-transport-quinn/src/send.rs (1)

20-23: Public SendStream::new looks reasonable and consistent

Making SendStream::new public aligns with RecvStream::new and gives consumers a straightforward way to wrap raw quinn::SendStreams. No hidden invariants appear to be violated here.

web-transport-quinn/src/recv.rs (1)

17-20: Public RecvStream::new is consistent with the send-side API

Exposing RecvStream::new publicly mirrors SendStream::new and preserves all existing behavior; the wrapper remains a thin newtype around quinn::RecvStream.