fix: end the session when an output cannot be built

[pksgit]

Fixes CS-2029.

Problem

Two places report ENDPOINT_ERROR for a track that cannot be built and then just return. Nothing ends the session, so it keeps running on whichever track did build:

• pipeline.go:126, when pad.Connect fails in onOutputReady
• pipeline.go:177, when sink.AddTrack or pipeline.Add fails in onParamsReady

SetStatus stamps an end time for any terminal status, so the session reports an end and an error while it is still publishing. That state is read downstream as the session being over:

• Reporting and billing stop while media flows. isTerminalState in cloud-ingress keys on status alone, so the update deletes the entry from activeIngresses and reports the session terminated. The periodic scrape then finds a nil entry and deregisters itself.
• The publisher is never told. CloseHandler is only reached through SessionManager.IngressEnded, which fires when the handler subprocess exits or on the WHIP non-transcoded end callback. A mid-session error reaches neither.
• Nothing reaps it. The cloud-io sweeper's zombie branch matches this state, but stopIngress is gated on sweep_deleted_ingress, which is not set in any staging or production cloud-io configmap.

GStreamer will not stop it either: a single dead output pad is tolerated for as long as one other output is alive.

Fix

Both call sites hand off to fail, which records the cause on pipelineErr, sets the terminal status, quits the loop, and then reports.

The ordering matters. SendStateUpdate has no deadline and runs on a GStreamer streaming thread, so it goes last: everything that ends the session has already happened by then, and a stalled RPC parks that thread alone rather than holding up the teardown. That is also where the call sat before this change, so the reporting behaviour is unchanged.

quitLoop rather than loop.Quit, because these callbacks can fire before Run has entered the loop, which is the case quitLoop exists for.

Tests

TestTrackBuildFailureStopsTheSession pins the contract: after a track fails to build, the session stops and Run receives a cause. TestTrackBuildFailureStopsTheSessionWhileReportingStalls pins the ordering, using a notifier that never answers.

Both verified by breaking the code they guard:

mutation	caught
the whole fix removed (test copied onto main)	the session kept running after a track failed to build
the state update moved ahead of the teardown	the teardown waited for the state update

The sink is nil in the first test deliberately: AddTrack reads the video resolution before it touches the sink, so caps without one reach the real failure path, and a reordering there panics rather than passing quietly.

Verification

From the CI config: go build ./... clean, go test -timeout 20m ./pkg/... passes across 8 packages, golangci-lint run --timeout=5m --modules-download-mode=mod reports 0 issues.

Deliberately not in this PR

The teardown can still stall on a silent publisher. Run reaches input.Close, which waits on RTMPRelaySource.Close, which waits on io.Copy, which waits on the next bytes from the relay. A publisher can hold its connection open while sending nothing. Measured with a relay that sends headers then goes quiet: Close blocked past 5s, and 236µs once the input is cancelled.

Two things make that acceptable to defer. The failure is reported immediately from fail, so the stall is visible rather than silent. And messageWatch's bus-error path already has the identical stall, so this is a second trigger for an existing problem rather than a new one. It is bounded by the media watchdog at roughly one to two minutes, which reaches the same cancel through the sink's disconnect callback, though I have not verified that path end to end.

Fixing it means cancelling the input from fail, which also needs Run's error precedence adjusted so the cancellation does not mask the cause, and a matching change to the WHIP relay wait. That is a coherent change on its own and will follow separately.

Known gaps

• The contradictory update window is still open. After fail, a surviving track can still report PUBLISHING over the terminal status. That was fix: keep a terminal session terminal #481's guard, closed unmerged alongside ensure a session is reported over when its handler exits #483 and needing revival on its own, so my earlier reply on this thread saying fix: keep a terminal session terminal #481 would fix it no longer holds.
• test/whip.go and the rest of test/ never run in CI. The test job runs only ./pkg/... and the integration job just builds a Docker image. That is CS-2036.

Open question

Was continuing on a partial failure ever intended? This is user-visible: a stream whose video format cannot be read no longer falls back to publishing audio alone.

If that fallback is wanted, this is the wrong fix, and the right one is a way to report partial failure that is not a terminal status, since a running session reporting ENDPOINT_ERROR is not something the rest of the ecosystem supports. That would be a protocol change rather than a pipeline change. Happy to take that direction instead.

🤖 Generated with Claude Code

[biglittlebigben]

Looks sensible. @milos-lk could you have a look as well?

[milos-lk]

Looks good with minor comments - this is though something we should state clearly in release notes - in case of problems with e.g video - audio only continuation won't happen.

[milos-lk]

There is one more scenario I think we might have problems with - a possibility for RTMP session & fail() to cause a stall.

If we have the following sequence:

1. fail queues loop.Quit via IdleAdd. It does not touch the Run context
2. The loop exits, and Run calls p.input.Close()
3. Input.Close calls RTMPRelaySource.Close which does two things: sets the writer's EOS flag, then blocks on <-s.result
4. s.result is only written by the copy goroutine started in RTMPRelaySource.Start, which is a plain io.Copy(s.writer, resp.Body) over the relay HTTP response
5. io.Copy only notices the EOS flag inside appSrcWriter.Write, which returns io.EOF when the flag is set. But Write is only called after resp.Body.Read returns a chunk.

So the chain is: Run waits on input.Close, which waits on result, which waits on io.Copy, which waits on the next bytes from the relay. If the publisher is connected but not sending (paused encoder, network stall, a client that holds the TCP session open with no data), Read blocks and the whole chain blocks with it.

In that stalled state the pipeline has already internally decided the session is over: status is ENDPOINT_ERROR with EndedAt stamped, the loop is gone, the sink is about to be closed. But the terminal SendStateUpdate in HandleIngress's deferred block only runs after Run returns, so nothing goes out to cloud-ingress until the publisher sends a byte or disconnects. The previous code sent the state update inline, so at least the ERROR status reached the control plane immediately, even if the session kept running.

We could think of canceling the context of input.Close() but in that case I guess we need to be careful to return original error instead.

[pksgit]

We could think of canceling the context of input.Close() but in that case I guess we need to be careful to return original error instead.

Addressed.

[milos-lk]

Cool - thanks - please don't merge it yet - I think we might break WHIP - does WHIP DELETE now end the session as ENDPOINT_ERROR instead of ENDPOINT_INACTIVE? need to check this tomorrow as right now my brain doesn't really work anymore.

[pksgit]

I think we might break WHIP - does WHIP DELETE now end the session as ENDPOINT_ERROR instead of ENDPOINT_INACTIVE? need to check this tomorrow as right now my brain doesn't really work anymore.

Thanks for catching. It seems you are right. I guess it better to handle the above condition that I tried to fix, in a separate PR. To avoid following regression in this PR, I have reinstated the code to send state update.

The previous code sent the state update inline, so at least the ERROR status reached the control plane immediately, even if the session kept running.

[devin-ai-integration]

Devin Review found 1 new potential issue.

2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

[pksgit]

@milos-lk , please take one final look when you get a chance and approve if this looks fine. Thanks.