fix(sctp): correct PR-SCTP max_retransmits abandonment condition by nightness · Pull Request #78 · webrtc-rs/rtc

nightness · 2026-04-01T16:39:26Z

Summary

Fixes an off-by-one error in the PR-SCTP partial reliability abandonment check (closes webrtc-rs/webrtc#776).

Changed >= reliability_value to > reliability_value so a stream configured with max_retransmits(N) correctly allows N retransmissions before abandoning.
Rewrote the inline comment with concrete examples showing how nsent (total sends including initial) relates to reliability_value (retransmission cap), and why > is correct while >= abandons one send too early.
Rewrote regression test to use only realistic nsent values (>= 1), matching actual call-site behavior where nsent is set to 1 on initial send.
Added boundary tests for max_retransmits = 0, 1, and 2.
Added test_check_partial_reliability_rexmit_no_false_abandon covering DCEP payload, use_forward_tsn=false, and missing stream edge cases.

Test plan

cargo check -p rtc-sctp passes
cargo clippy -p rtc-sctp passes
cargo fmt --check passes
cargo test -p rtc-sctp passes (105/105 tests, including new regression tests)
Regression test uses realistic nsent values (>= 1) matching production call sites
Regression test validates boundary for max_retransmits = 0, 1, and 2
Key regression assertion: max_retransmits=1, nsent=1 must NOT abandon (fails with >=, passes with >)
Edge case test covers DCEP, disabled forward-TSN, and missing stream

🤖 Generated with Claude Code

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes an off-by-one error in PR-SCTP partial reliability (RFC 3758) abandonment logic so max_retransmits(0) does not abandon prematurely.

Changes:

Adjusted the abandonment condition from >= to > for ReliabilityType::Rexmit.
Added inline comments explaining the RFC rationale and the off-by-one behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-04T14:14:39Z

+                // (1 initial + N retransmissions), so abandon when nsent > N (reliability_value).
+                // Using `>=` would incorrectly abandon after only N total sends (one too few).
+                // Fixes: webrtc-rs/webrtc#776
+                if c.nsent > reliability_value {


This change fixes a specific regression-sensitive boundary condition (max_retransmits=0). Please add a focused regression test that fails with >= and passes with >, validating the abandonment decision at the boundary (0 and 1) so the off-by-one doesn’t reappear in future refactors.

Addressed: added test_check_partial_reliability_rexmit_boundary which tests max_retransmits=0 (nsent=1 must NOT abandon — fails with >= but passes with >) and max_retransmits=1 boundary. Also added test_check_partial_reliability_rexmit_no_false_abandon for DCEP, disabled PR-SCTP, and missing stream edge cases. This comment should be marked outdated.

codecov · 2026-04-04T14:18:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.17%. Comparing base (9feb4a3) to head (8387b6e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   71.17%   71.17%   -0.01%     
==========================================
  Files         442      442              
  Lines       67330    67330              
==========================================
- Hits        47922    47920       -2     
- Misses      19408    19410       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Clarify comment on `>` vs `>=` to explicitly describe what `nsent` counts (total sends including initial) vs what `reliability_value` represents (retransmission limit), per reviewer suggestion. - Add regression test `test_check_partial_reliability_rexmit_boundary` that validates the abandonment boundary for max_retransmits=0 and max_retransmits=1, ensuring the off-by-one cannot regress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…→ >) Fixes webrtc-rs/webrtc#776. RFC 3758 §5.3.1 specifies that a DATA chunk should be abandoned when it has been transmitted MORE THAN max_retransmits times. The previous check used `>=`, which abandoned chunks after only max_retransmits sends (one fewer than the allowed total of max_retransmits + 1). For example, with max_retransmits=1 (reliability_value=1): Before: abandon when nsent >= 1 → abandoned after first send (zero retransmits) After: abandon when nsent > 1 → abandoned after second send (one retransmit) ✓ With max_retransmits=0 (reliability_value=0), the behavior is identical (nsent=1 satisfies both >= 0 and > 0 after the first send). Premature abandonment was the root cause of channels unexpectedly closing: the abandoned chunk triggered Forward TSN skip-ahead, which in some browser interop scenarios caused the peer to reset the stream. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Clarify comment on `>` vs `>=` to explicitly describe what `nsent` counts (total sends including initial) vs what `reliability_value` represents (retransmission limit), per reviewer suggestion. - Add regression test `test_check_partial_reliability_rexmit_boundary` that validates the abandonment boundary for max_retransmits=0 and max_retransmits=1, ensuring the off-by-one cannot regress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Rewrite the Rexmit abandonment comment to explicitly document the relationship between nsent (total sends) and reliability_value (retransmission cap), with concrete examples for max_retransmits=0 and max_retransmits=1. - Rewrite regression test to use only realistic nsent values (>= 1), matching actual call-site behavior where nsent is set to 1 before the first call to check_partial_reliability_status. - Add max_retransmits=2 boundary test for additional coverage. - Add test_check_partial_reliability_rexmit_no_false_abandon covering DCEP payload, use_forward_tsn=false, and missing stream edge cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

nightness · 2026-04-10T05:26:05Z

Rebased onto upstream/master so this PR contains only its own changes. Previous branch structure caused merge conflicts when PRs were merged in sequence. Each PR is now independently mergeable.

rainliu requested a review from Copilot April 4, 2026 14:07

Copilot AI reviewed Apr 4, 2026

View reviewed changes

Copilot started reviewing on behalf of rainliu April 4, 2026 14:22 View session

nightness requested a review from Copilot April 8, 2026 08:00

Copilot started reviewing on behalf of nightness April 8, 2026 08:01 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread rtc-sctp/src/association/mod.rs Outdated

Comment thread rtc-sctp/src/association/association_test.rs Outdated

nightness and others added 3 commits April 10, 2026 00:16

nightness force-pushed the fix/sctp-max-retransmits branch from 59be360 to 211857a Compare April 10, 2026 05:16

nightness mentioned this pull request Apr 10, 2026

Summary: 19 PRs ready for review #87

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(sctp): correct PR-SCTP max_retransmits abandonment condition#78

fix(sctp): correct PR-SCTP max_retransmits abandonment condition#78
nightness wants to merge 3 commits into
webrtc-rs:masterfrom
Brainwires:fix/sctp-max-retransmits

nightness commented Apr 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Uh oh!

nightness Apr 9, 2026

Uh oh!

codecov Bot commented Apr 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nightness commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 4, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nightness commented Apr 1, 2026 •

edited

Loading