Fix a rare race when accepting a stream.

[kixelated]

If the stream is reset before the WebTransport header can be written, then web-transport-quinn was incorrectly returning an error. This can happen naturally under packet loss scenarios or potentially even when an empty stream is reset.

Summary by CodeRabbit

• Bug Fixes
  • Improved resilience of WebTransport stream processing by gracefully handling stream decode errors instead of terminating operations, allowing the system to skip problematic streams and continue processing.

[coderabbitai]

Walkthrough

The pull request modifies error handling in stream acceptance logic across two WebTransport implementations. In web-transport-quiche/src/connection.rs and web-transport-quinn/src/session.rs, the poll_accept_uni and poll_accept_bi methods now handle stream header decoding errors differently. Previously, decoding errors were propagated immediately, causing early termination. The changes log warnings when errors occur and continue processing remaining stream candidates instead of returning errors. This affects how the implementations handle faulty stream headers during unidirectional and bidirectional stream acceptance.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix a rare race when accepting a stream' directly and clearly summarizes the main change: addressing a race condition in stream acceptance during header decoding when streams are reset early.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-immediate-reset

---

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 (1)

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

391-397: LGTM - Good fix for the race condition.

The change from propagating errors to logging and continuing correctly addresses the race condition where streams are reset before the WebTransport header can be written. This makes stream acceptance more resilient to packet loss and early resets.

However, consider these improvements:

1. Error differentiation: The current code treats all decoding errors equally. Consider differentiating between expected errors (early stream resets) and unexpected errors (session ID mismatches, which could indicate protocol violations or bugs).

2. Observability: Add a counter or metric to track the frequency of these decode failures, which would help with monitoring and debugging in production.

Example refactor for better error handling:

 let (typ, recv) = match ready!(self.pending_uni.poll_next_unpin(cx)) {
     Some(Ok(res)) => res,
     Some(Err(err)) => {
-        // Ignore the error, the stream was probably reset early.
-        log::warn!("failed to decode unidirectional stream: {err:?}");
+        // Ignore the error, the stream was probably reset early.
+        // This is expected under packet loss or when empty streams are reset.
+        match &err {
+            SessionError::WebTransport(web_transport_proto::WebTransportError::UnknownSession) => {
+                log::warn!("stream decode failed with session ID mismatch: {err:?}");
+            }
+            _ => {
+                log::debug!("stream decode failed (likely early reset): {err:?}");
+            }
+        }
         continue;
     }
     None => return Poll::Pending,
 };

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6174875 and 11f1960.

📒 Files selected for processing (2)

• web-transport-quiche/src/connection.rs (2 hunks)
• web-transport-quinn/src/session.rs (2 hunks)

🔇 Additional comments (3)

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

462-468: Consistent error handling for bidirectional streams.

The same resilient error handling is applied here as in poll_accept_uni, which is good for consistency. The same recommendations apply regarding error differentiation and observability.

web-transport-quiche/src/connection.rs (2)

427-433: Consistent fix across both implementations.

The error handling change here mirrors the quinn implementation, which is excellent for consistency. Using tracing::warn! with structured logging (?err) provides better observability than plain string formatting.

The same recommendations from the quinn implementation apply: consider differentiating between expected errors (early resets) and unexpected errors (protocol violations), and consider adding metrics to track the frequency of these occurrences.

---

498-504: Consistent bidirectional stream handling.

The error handling for bidirectional streams matches the unidirectional implementation, maintaining good consistency.