Skip to content

call ready before starting the goroutine that calls ended - #495

Open
erikhortsch wants to merge 1 commit into
mainfrom
erikhortsch/whip-ready-before-ended
Open

call ready before starting the goroutine that calls ended#495
erikhortsch wants to merge 1 commit into
mainfrom
erikhortsch/whip-ready-before-ended

Conversation

@erikhortsch

Copy link
Copy Markdown
Contributor

Stacked on #491. Review that first; this branch is one file.

The defect

createStream hands the caller ready and ended as a pair with no ordering between them. ready was deferred, so it ran on the way out of the goroutine that launches the one calling ended:

go func() {
    if ready != nil {
        defer func() { stats := ready(mimeTypes, err) }()   // runs last
    }
    mimeTypes, err = h.Start(ctx)
    ...
    go func() {                                            // launched first
        defer func() { ended(err) }()
        err = h.WaitForSessionEnd(s.ctx)
    }()
}()

For a bypassed WHIP session that is already over, WaitForSessionEnd selects on an already-broken done and returns without blocking. Init registers the RTC notify topic before it returns, so the SFU can break done at any point after the SDP exchange — a publisher whose connection fails during the handshake, a client that hangs up on the answer, an immediate resource delete.

This is not a narrow scheduling window. ready's second statement is SendStateUpdate, a psrpc round trip, so ready parks about two statements in and hands its P to the goroutine sitting in runnext. ended then runs to completion in the middle of it:

ready:  SetStatus(PUBLISHING) -> SendStateUpdate --park---------------+
ended:                          SetStatus(INACTIVE) -> SendStateUpdate|
                                SessionEnded -> IngressEnded          |
ready:  <-------------------------------------------------------------+
        IngressStarted -> SessionStarted     <- registers a dead session

Three consequences: the terminal state is overwritten by PUBLISHING (SetStatus has no terminal guard, last write wins); the notifier is told a released session started; and IngressStarted lands after IngressEnded, leaving an entry in the session manager that nothing removes, so IsIdle never comes true and shutdown does not complete.

Only bypassed WHIP is affected — ended is non-nil only under !*p.EnableTranscoding. Every other input registers synchronously inside ready and is handed a nil ended.

The fix

Call ready inline, before the goroutine that calls ended exists. There was exactly one return between the defer and the go, so the restructure is mechanical: the deferred body becomes straight-line code and the error branch keeps its own return.

No synchronization primitive. A sync.Once would force ended to synthesize a ready(nil, err) it has no arguments for, which rewrites a clean short session's terminal state to ENDPOINT_ERROR; a channel barrier works but deadlocks if ready is ever nil, since the close sits under the same if.

The only thing lost is panic-unwinding coverage, which was not real — nothing recovers here, so the process dies either way.

Verification

go build ./..., go vet ./pkg/..., go test -count=1 ./pkg/... — 8 packages, all pass.

Not covered by a test: createStream reaches the race only through a live SFU HTTP exchange in Init and a psrpc-backed notifier, neither of which has a seam here.

@erikhortsch
erikhortsch force-pushed the erikhortsch/lifecycle-driven-session-tracking branch from abeb962 to 4c9ff78 Compare September 9, 2026 03:49
Base automatically changed from erikhortsch/lifecycle-driven-session-tracking to main September 9, 2026 03:57
createStream hands the caller ready and ended as an unordered pair. ready
was deferred, so it ran on the way out of the goroutine that launches the
one calling ended -- and a session whose done channel is already broken
reaches ended without blocking. Init registers the RTC notify topic before
it returns, so the SFU can break done at any point after the SDP exchange.

The ordering is not close. ready's second statement sends a state update,
which is a psrpc round trip, so ready parks almost immediately and hands
its P to the goroutine sitting in runnext. ended then runs to completion
before ready resumes to register the session.

Calling ready inline before that goroutine exists removes the race by
construction. It also keeps IngressStarted from landing after IngressEnded,
which would leave a session in the manager that nothing removes and an
IsIdle that never comes true.
@erikhortsch
erikhortsch force-pushed the erikhortsch/whip-ready-before-ended branch from 19df8b0 to a2d7e80 Compare September 9, 2026 17:22
@erikhortsch
erikhortsch marked this pull request as ready for review September 10, 2026 16:59
@erikhortsch
erikhortsch requested a review from a team as a code owner September 10, 2026 16:59

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant