Repository navigation
feat(media): hold a listener pull until the publisher track is active, behind a flag - #794
realhasanshoaib wants to merge 1 commit into
Conversation
…, behind a flag When a listener pulls a track just after its publisher published, the API now waits (about 200 ms polls, 4 s budget) for Cloudflare to list the track as active, then forwards the pull either way. Off by default with CHALK_MEDIA_PULL_HOLD_ENABLED; budget in CHALK_MEDIA_PULL_HOLD_BUDGET_MS. Only publications younger than 10 s are checked, and an active track is cached, so clean pulls add no Cloudflare call and no wait. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
realhasanshoaib
added
risk:low
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Parked as a draft. Rig notes show Cloudflare lists a track as active before the publisher connects (12 of 29), so holding pulls until active probably doesn't prevent the pre-connect track errors this targets. Waiting on the rig run that measures that before deciding whether to rework or close this. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if err != nil { | ||
| if ctx.Err() != nil { | ||
| return pending, "" | ||
| } | ||
| return pending, pullHoldOutcomeReadFailed | ||
| } | ||
| if !session.Found { | ||
| return pending, pullHoldOutcomeSessionMissing | ||
| } | ||
| h.markActive(connectionID, session) |
There was a problem hiding this comment.
🔴 Missing publisher releases every listener pull
When a pull names multiple young publishers, poll releases every track if any session is missing or its read fails. Other pending tracks reach Cloudflare before becoming active, risking a stuck listener track.
Learn more
An add-tracks request can pull up to 16 remote tracks. hold passes all pending identities into poll, which groups them by publisher connection. If one session is missing or its read fails, poll exits without waiting on the other connections. The endpoint then forwards the whole request through signalSFUTracks. The client supports partial remote responses, so the missing track does not necessarily prevent the remaining tracks from being attempted.
Example: A listener requests Alice's newly published microphone and Bob's departed camera in one request. Bob's session lookup returns Found: false; Alice's microphone is still inactive. The hold ends immediately, and the request can reach Cloudflare while Alice's microphone is unavailable instead of waiting for it.
Recommended fix: Remove only identities belonging to a missing or unreadable publisher from the pending set. Continue polling other publishers until they become active or the shared budget expires. Add a multi-publisher endpoint test where one session is missing and the other becomes active later.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func (f *pullHoldFixture) read(_ context.Context, connectionID string) (PublisherSession, error) { | ||
| f.t.Helper() | ||
| if connectionID != f.identity.ConnectionID { | ||
| f.t.Fatalf("session read for %q, want %q", connectionID, f.identity.ConnectionID) | ||
| } | ||
| f.reads++ | ||
| if f.readErr != nil { | ||
| return PublisherSession{}, f.readErr | ||
| } | ||
| if f.notFound { | ||
| return PublisherSession{}, nil | ||
| } | ||
| return PublisherSession{Found: true, Tracks: []PublisherTrack{{MID: "camera-mid", TrackName: f.identity.TrackName, Active: f.reads > f.activeAfter}}}, nil |
There was a problem hiding this comment.
🟡 Observed pull failure lacks regression proof
The fake read response invents track status and timing instead of replaying an observed failed pull. The production failure therefore has no regression test grounded in its retained evidence.
Learn more
The repository's review standard requires a production-failure fix to have a test that fails without the fix and uses an observed sanitized fixture, logged request, or retained trace. This fixture synthesizes publisher status from a read count, while the endpoint test likewise synthesizes the response. Neither checks the timing or response shape of the reported failure.
Example: If Cloudflare reports active before the publisher connects, this fake can be configured to turn active later and pass even though the real pull still fails.
Recommended fix: Build a regression from a sanitized captured failed request and provider/session responses, replay the observed ordering, and demonstrate that it fails on the base implementation. If the active signal cannot prevent the observed failure, make that limitation explicit in the proof rather than using fabricated status transitions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for _, track := range session.Tracks { | ||
| if !track.Active { | ||
| continue | ||
| } | ||
| if entry, ok := h.entries[RemoteTrackIdentity{ConnectionID: connectionID, TrackName: track.TrackName}]; ok { | ||
| entry.active = true |
There was a problem hiding this comment.
🔍 Active status does not prove connection
InspectSession treats every status except inactive as active. The reported rig observed active tracks before publisher connection, so the hold often releases before the track is pullable.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e157e3a57
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (h *pullHold) releasePoll(connectionID string) { | ||
| h.mu.Lock() | ||
| defer h.mu.Unlock() | ||
| delete(h.polling, connectionID) |
There was a problem hiding this comment.
With the flag enabled, multiple listeners pulling the same inactive publication only share a read while that request is in flight. Deleting the claim immediately afterward lets another listener issue a fresh session read within the same 200 ms interval, so staggered listeners each run their own polling loop instead of sharing one poll per publisher. This amplifies Cloudflare requests during publication fan-out precisely while the hold is needed. Retain a per-publisher next-poll time or share the latest poll result for the interval, including failed or missing-session outcomes.
Useful? React with 👍 / 👎.
A listener's pull that reaches Cloudflare before the speaker's connection is up poisons that track: it returns
not_found_track_error, and retries after the speaker connects fail too (11 of 11 such taps in the rig; pulls made after connect almost never failed). SDK 5.2.6 cannot publish again by itself, so the speaker stays silent until they toggle the mic.Fix
CHALK_MEDIA_PULL_HOLD_BUDGET_MS(200 to 5000), which stays well under the 5.2.6 client's pull timeout of about 8 s.CHALK_MEDIA_PULL_HOLD_ENABLED=trueturns it on. With it off, the API keeps no state and makes no extra call.chalk.media.pull_hold(by outcome) andchalk.media.pull_hold_wait(ms), and anmedia_publication.pull_holdlog with outcome, wait, budget and counts. It logs no participant or connection identifiers.Limit: Cloudflare marks a published track active when the publish is answered, not when the publisher's ICE/DTLS connects. The rig saw 12 of 29 new publisher connections already listed active before connect, and the first status read returned active for 27 of 29. So this hold most often releases on the first read and may not prevent poisoning on its own; the budget and flag are the part that matter once a real connect signal exists. The code keeps the signal in one place (
pullHold.markActive), so a connect signal can replace "active" without touching the endpoint.What changed
Changed: SFU add-tracks endpoint, publication service, API config, API env example.
Untouched: Sync, clients and SDKs, database, publication liveness and closing rules.
Proof
mediapublications, and through the real endpoint with a fake Cloudflare plane inhttpapi.go vet,gofmtand the touched packages' tests pass locally.Turn it off by unsetting
CHALK_MEDIA_PULL_HOLD_ENABLED; the change is inert while the flag is off.Created with Claude Sonnet 5.5 in Claude Code.