Skip to content

fix(sdk): replace a first peer connection that has not connected after 3 s - #786

Draft
realhasanshoaib wants to merge 5 commits into
masterfrom
fix/first-connect-ice-deadline
Draft

realhasanshoaib wants to merge 5 commits into
masterfrom
fix/first-connect-ice-deadline

Conversation

@realhasanshoaib

@realhasanshoaib realhasanshoaib commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

About 1% of first connections to a new Cloudflare session never get a STUN reply, and Chrome reports failed only after about 15 s. A listener hears nothing for that long and a speaker's join fails. A first version of this fix replaced at 3 s. Rig runs (24 rooms per arm) showed that was worse: 6.9% of listeners heard after more than 5 s, against 4.7% without it. Replacing was slower than waiting in 14 of 17 cases, and in 5 the old connection had already reached ICE connected.

Fix

  • Deadline. A never-connected peer connection now gets 5 s (FIRST_CONNECT_DEADLINE_MS). Only 6 of 383 first connections took longer, so a healthy connection almost never reaches it. At the deadline the client checks the real state (iceIsProgressing). If ICE is connected, or checking with at least one server response, it keeps waiting out the old 8 s window. Only a stuck connection is replaced. If the extended wait ends with the connection still not live, it counts as stuck too and takes the same replacement path.
  • Replacement loop. Unchanged: publish and receive share #retryOnReplacedConnection, at most two replacements, backoff 0 s then 1 s.
  • Abandoned publication. ActiveAuthorizer.BindParticipantMediaConnection and CompleteParticipantMediaRefresh (apps/api/internal/accessgrants/active.go) now read mediapublications.Service.Latest right after the new connection is bound. Latest retires every publication whose connection no longer matches the participant and appends the observation, which fires the chalk_media_publications NOTIFY that Sync reconciles on.

What changed

Changed: SDK first-connect deadline and pre-replacement state check; API access grant now retires and announces the old connection's publication.

Untouched: the replacement loop, disconnected wait, the Sync service, the API contract.

Why listeners pulled the old publication: The access grant moved participants.media_connection_id and recorded nothing in the publication registry. The old publication stayed enabled in the registry and in Sync's projection until something read Latest (retirement is lazy, service.go:173-228) or the 15 s Sync timer fired. Listeners got it from sfu_signaling.go:532 or Sync, pulled it on the abandoned session, and heard only after 5 to 9 s.

If the announcement fails, the grant still succeeds and logs a warning, because the binding is committed and the next read retires the publication.

Proof

  • Go: new active_test.go asserts that a bind and a completed refresh each read the registry once after the binding, that a failed binding does not, and that a failed read does not fail the grant. It failed to build before the change (NewActiveAuthorizer took no publication reader). go test passes for accessgrants, httpapi, mediapublications.
  • SDK: new tests keep a connection at ICE connected, or ICE checking with a response, past the deadline (no replacement); they failed at the 3 s deadline. A checking connection with no response is still replaced. A connection with ICE responses that never goes live is replaced at 8 s (failed before the fix with a non-retryable media_failed). Full SDK suite 1520 passed, 2 expected fail; lint and fallow clean.
  • Recovery latency: a connection with no ICE progress is replaced at 5 s and live about 1.5 to 3 s later, so about 6.5 to 8 s instead of 15 s. A connection with ICE responses that never goes live is replaced at 8 s, so about 9.5 to 11 s. Two stuck peers in a row add 5 s and a 1 s backoff for the second replacement. A connection that is only slow is never replaced.
  • Not checked: the rig run on this head, and the adapters/postgres integration tests (no local database; the package compiles and vets).

Known and not changed here: a concurrent registry read during a refresh's temporary replacement: binding can retire a live publication if the grant then fails. That window exists on master (Latest is unchanged by this PR); follow-up in #787.

Revert with a plain revert of the two commits.

Created with Claude Sonnet 5.5 in Claude Code.

🤖 Generated with Claude Code

@realhasanshoaib realhasanshoaib added risk:medium Medium risk: Hasan reads the diff before it merges. size:M 30-99 changed lines, not counting tests, lockfiles or generated files. labels Nov 1, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T16:46:14.097373Z bc04b4a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

devin-ai-integration[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@realhasanshoaib realhasanshoaib added size:L 100-499 changed lines, not counting tests, lockfiles or generated files. and removed size:M 30-99 changed lines, not counting tests, lockfiles or generated files. labels Nov 1, 2026
realhasanshoaib and others added 4 commits November 1, 2026 21:39
…r 3 s

A first ICE negotiation that gets no STUN reply (about 1% of new Cloudflare
sessions) stayed silent for about 15 s, until Chrome reported failed. The
client now gives a never-connected peer connection 3 s after its answer is
applied, then replaces it with a new grant, session and peer connection,
at most twice, with a 0 s then 1 s wait.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…dget

The receive path replaced a stuck connection once, then let the second
timeout escape into the 15 s pull deferral. Publish and receive now share
one loop, retryOnReplacedConnection, that replaces and retries while the
failure may still be replaced. Splitting replaceMediaConnectionForRecovery
into small helpers also clears the fallow complexity finding.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ent grant

The grant moved participants.media_connection_id without touching the
registry, so the old connection's publication stayed enabled in the registry
and in Sync until a later read retired it. Listeners pulled it on a session
the client had already abandoned. Reading Latest after the binding retires it
and appends the observation that Sync is notified about.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The 3 s deadline replaced slow but healthy connections, including ones whose
ICE had already connected. The deadline is now 5 s, and a connection whose ICE
is connected, or checking with a server response, keeps waiting.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@realhasanshoaib
realhasanshoaib force-pushed the fix/first-connect-ice-deadline branch from 61d3ffd to 067a02e Compare November 1, 2026 16:40
devin-ai-integration[bot]

This comment was marked as resolved.

… live

When the extended wait ends on a still-unconnected first peer it now raises
the retryable first-connect failure tied to that peer, so the replacement
budget applies instead of a non-retryable media_failed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@realhasanshoaib

Copy link
Copy Markdown
Contributor Author

Parking this as a draft: the impaired-network rig doesn't support merging it.

Run 4: 30 rooms per arm, on the 067a02e server, interleaved on 2 machines. Intervals are 95% bootstrap intervals.

Arm Slow (over 5 s) Tail (over 10 s or never heard) Never heard
5.2.6 3.0% (1.8–4.5) 0.6% (0.2–1.1) 1
master SDK 3.5% (2.3–4.9) 0.8% (0.2–1.6) 1
this PR 3.1% (1.7–4.9) 1.7% (0.4–3.5) 6

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:medium Medium risk: Hasan reads the diff before it merges. size:L 100-499 changed lines, not counting tests, lockfiles or generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant