Fix subscriber losing App reset codes

[kixelated]

Summary

Fixes #936. When a publisher aborts a track with Error::App(code), subscribers now correctly receive Error::App(code) instead of Error::Cancel.

• Error::from_transport() checks stream_error() on transport errors to decode QUIC stream reset codes back into the original Error variant via the new from_code() method, instead of wrapping everything as Error::Transport
• Subscribers no longer collapse Error::Transport into Error::Cancel, so decoded errors like Error::App propagate correctly to consumers
• Simplified Error enum by removing inner data from Transport, Decode, Version, RequiredExtension, and BoundsExceeded variants, making the error type fully round-trippable through to_code()/from_code()

Test plan

• just check passes (compilation, tests, linting, formatting)
• End-to-end: publisher calls track.abort(Error::App(42)) → subscriber receives Error::App(42) (not Error::Cancel)

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

The PR refactors error representation and handling across the moq-lite crate: several Error variants (Transport, Decode, Version, RequiredExtension, BoundsExceeded) become unit-like, many new variants are added, and Error gains to_code, from_code, and from_transport conversions plus From impls. Call sites replace constructing Arc-wrapped transport errors with Error::from_transport. Reader adds decode_maybe and new has_more/read_more helpers. SessionInner switches from Arc to Box and its closed future no longer returns an error payload. Multiple modules adjust pattern matches and error mapping to the new error shape.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.72% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix subscriber losing App reset codes' directly addresses the main objective of this PR, which is fixing subscribers to correctly receive Error::App(code) instead of Error::Cancel when publishers abort with app errors.
Description check	✅ Passed	The description is well-related to the changeset. It explains the fix for issue #936, describes the three key changes (Error::from_transport, simplified Error enum, and subscriber error propagation), and outlines the test plan.
Linked Issues check	✅ Passed	The PR fully addresses the objectives from issue #936: Error::from_transport() decodes QUIC stream reset codes via new from_code() method, subscribers no longer collapse Error::Transport into Error::Cancel allowing Error::App to propagate, and the Error enum is simplified with unit-like variants making it round-trippable through to_code()/from_code().
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue #936. The PR modifies error handling throughout the codebase to support decoding app error codes, which is directly required by the issue. No unrelated refactoring or feature additions were detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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/subscriber-app-reset-codes

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

rs/moq-lite/src/error.rs (2)

83-137: to_code/from_code roundtrip is broken for UnknownAlpn

to_code maps UnknownAlpn(_) → 21, but from_code has no arm for 21, so it falls through to ProtocolViolation. This is likely fine since UnknownAlpn is a local ALPN-negotiation error (carrying a String that can't survive a u32 roundtrip), but it contradicts the PR objective of making errors "fully round-trippable."

Consider adding 21 => Self::UnknownAlpn(String::new()) or, alternatively, documenting that UnknownAlpn is intentionally local-only.

---

109-137: Consider adding unit tests for from_code/to_code roundtripping.

Given that this is the core mechanism for the bug fix (decoding stream reset codes back into the correct Error variant), inline tests would be valuable — especially to assert the App(code) roundtrip, boundary at code 64, and the fallback to ProtocolViolation for unknown codes.

As per coding guidelines, Rust tests should be integrated within source files.

rs/moq-lite/src/ietf/control.rs (1)

51-52: Minor semantic inconsistency between send failure and closed signaling.

send failure (line 51) maps to Error::Transport, while tx.closed() (line 90) returns Error::Cancel. Both indicate the receiver was dropped — i.e., the same underlying condition. Consider using Error::Cancel here for consistency, since the channel being closed is more of a cancellation than a transport failure.

Not a blocker — the practical impact is minimal since callers generally treat both as terminal.

---

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]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

rs/moq-lite/src/error.rs (1)

77-102: ⚠️ Potential issue | 🟡 Minor

to_code can overflow for large App values.

App(u32::MAX).to_code() computes u32::MAX + 64, which panics in debug builds and wraps in release builds. Since App(u32) is public, callers can construct any value. Consider either clamping/saturating or validating at construction time.

Proposed fix: saturating arithmetic

-			Self::App(app) => *app + 64,
+			Self::App(app) => app.saturating_add(64),

Alternatively, you could cap App values in from_code or add a constructor that validates app < u32::MAX - 63.

rs/moq-lite/src/coding/reader.rs (1)

92-110: ⚠️ Potential issue | 🟠 Major

read_exact may loop infinitely if the stream closes before the buffer is filled.

The RecvStream::read_buf method returns Result<Option<usize>>, where Ok(None) signals EOF. On line 106, the result is only checked for errors via .map_err(Error::from_transport)?, but Ok(None) is silently discarded and the loop continues. This causes an infinite loop if the stream closes prematurely. The read_more() method (line 151) correctly handles this with a match statement that checks for Ok(None).

Proposed fix

 		while buf.has_remaining_mut() {
-			self.stream.read_buf(&mut buf).await.map_err(Error::from_transport)?;
+			match self.stream.read_buf(&mut buf).await {
+				Ok(Some(_)) => {}
+				Ok(None) => return Err(Error::Decode),
+				Err(e) => return Err(Error::from_transport(e)),
+			}
 		}

[roberte777]

I think this looks good to me, basically what I came up with as well.