Skip to content

Make SCTP stream reset and reuse generation-safe - #494

Closed
idy wants to merge 7 commits into
pion:mainfrom
GizClaw:fix/stream-reset-reuse
Closed

idy wants to merge 7 commits into
pion:mainfrom
GizClaw:fix/stream-reset-reuse

Conversation

@idy

@idy idy commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

What changed

  • expose reset-completion notifications and negotiated inbound/outbound stream limits
  • retain stream ownership until both reset directions have completed
  • keep delayed acknowledgements and retransmitted or stale reset requests from affecting a reused stream ID
  • keep a single outgoing reset request in flight and bound the number of stream IDs per request
  • complete pending incoming reset requests when the cumulative TSN advances through FORWARD-TSN or I-FORWARD-TSN, and re-evaluate a retransmitted request that is still in progress instead of replaying the cached "In progress" response
  • ignore a late response to an outgoing reset for a stream the peer has already reused, instead of resetting the new generation's outgoing sequence numbers
  • copy configured inbound and outbound stream limits into client and server association options

Why

WebRTC needs to reuse SCTP stream IDs after a DataChannel closes, but an ID is not safe to reuse until the reset procedure has completed in both directions. Delayed acknowledgements or a retransmitted reset request from the previous stream generation must also not close or mutate the new generation.

An incoming reset request whose Sender's Last Assigned TSN is ahead of the cumulative TSN is answered "In progress". When the peer closes a partially reliable stream whose last message was abandoned, that gap is closed by (I-)FORWARD-TSN rather than DATA, and the reset previously never completed unless more DATA arrived later.

When a stream ID is reused immediately after its reset completes, the response to our own outgoing reset for the previous generation can arrive after the new generation has already sent data. Applying it then reset the new generation's outgoing MID/SSN, so the next message repeated sequence number 0 and was never delivered by the peer.

Scope

This supersedes #485, which was opened from a branch that also carried unrelated fixes. Those are now separate PRs:

Relationship to #463

This is an alternative implementation that covers the stream-limit and reset-completion goals of #463 and adds the lifecycle and retransmission handling needed by the corresponding WebRTC integration. As discussed in #485, we are happy to combine this with #463 or rebase on top of it once it lands, whichever maintainers prefer.

Validation

  • go vet ./...
  • go test -race ./...

🤖 Generated with Claude Code

@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.61%. Comparing base (597b321) to head (e815877).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   85.50%   85.61%   +0.11%     
==========================================
  Files          56       56              
  Lines        5465     5535      +70     
==========================================
+ Hits         4673     4739      +66     
- Misses        556      558       +2     
- Partials      236      238       +2     
Flag Coverage Δ
go 85.61% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

idy and others added 6 commits September 11, 2026 23:49
WebRTC needs the negotiated stream bounds and a definitive reset
boundary before it can safely reuse DataChannel IDs on a long-lived
association.

- Report negotiated inbound and outbound stream counts in association
  metadata
- Notify consumers only after a reset removes the matching stream
- Cover asymmetric negotiation, pending resets, duplicate resets, and
  both peers

Refs: pion#463

Generated with [Codex](https://github.com/openai)
Release a reset stream identifier only after the local outgoing reset
is acknowledged and the peer outgoing reset has removed the inbound
stream. This prevents a newly opened DataChannel from colliding with
the previous stream generation.
Retransmitted reset requests could arrive after a completed stream ID
had already been reused, deleting the new stream generation. The
deprecated Config path also dropped explicit negotiated stream limits.

- Track the peer reconfiguration sequence and replay the last response
- Reject stale reset requests before they can mutate a reused stream
- Preserve inbound and outbound stream limits through Config options
- Cover duplicate requests, stale requests, and Config propagation

Generated with [Codex](https://github.com/openai)
RFC 6525 permits only one reconfiguration request in flight. Queue
further outgoing stream resets until the outstanding request is
answered, and cap the number of stream identifiers per request so the
RECONFIG chunk fits in one packet.
An incoming Outgoing SSN Reset Request whose senderLastTSN is ahead of
the cumulative TSN is answered "In progress" and kept pending. Pending
requests were only re-checked when DATA advanced the cumulative TSN,
and retransmissions of the pending RSN replayed the cached "In
progress" response. When the gap was closed by an abandoned message's
(I-)FORWARD-TSN, the reset never completed.

Re-check pending requests whenever the cumulative TSN advances,
including via FORWARD-TSN and I-FORWARD-TSN, and re-evaluate a
retransmitted request that is still pending instead of replaying the
cached response. Completed requests keep the RSN ordering protections.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@idy
idy force-pushed the fix/stream-reset-reuse branch from e815877 to 307c5ec Compare September 11, 2026 15:49
Once a peer has answered our outgoing reset and seen its own reset
answered, it considers the stream reset in both directions and may
reuse the stream identifier immediately. Our answer from the peer can
still arrive after the new generation's first DATA, e.g. when it is
lost and only the answer to the retransmitted request gets through, or
when the peer sends new DATA before queueing the answer.

The response then reset the outgoing SSN/MID counters of whatever
stream was registered under that identifier, which by now was the
new, already-sending generation. Its next message repeated an SSN/MID
the peer had already delivered and was never read.

Outgoing resets are only requested by Stream.Close, so only apply the
counter reset to a stream that is no longer open.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@JoTurk JoTurk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please break or simplify this?
can you also check https://github.com/pion/webrtc/wiki/Contributing#ai-assisted-pull-requests

@idy idy closed this Sep 16, 2026
idy added a commit to GizClaw/pion-webrtc that referenced this pull request Sep 20, 2026
Resolve the sctptransport_test.go conflict by keeping both sides: this
branch's DataChannel ID allocation and generation tests and upstream's
new SCTPTransport StartContext tests. The ID reuse hooks
(OnStreamResetComplete, WithNumStreams) moved into upstream's new
StartContext with the rest of association setup.

The branch still does not build against the released pion/sctp: it uses
the APIs added in pion/sctp#494 (OnStreamResetComplete, WithNumStreams,
negotiated stream counts in AssociationMetadata), which is why this PR
is a draft.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants