Repository navigation
[bugfix] Streaming: end sessions with the documented segment_cap, session_timeout and error frames - #2072
[bugfix] Streaming: end sessions with the documented segment_cap, session_timeout and error frames#2072stayinalive181 wants to merge 3 commits into
Conversation
…sion_timeout and error frames
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 PR merge requirements
|
[Medium impact] The PR appears safe to merge, with no outstanding findings.
|
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 3/5
Thanks — the segment-cap and idle-timeout paths now match the contract, ACTIVE -> COMPLETE is a legal transition, and the four new tests pin the frames and close codes. One must-fix and two doc nits.
Must fix
fastvideo/entrypoints/streaming/server.py:247— the unexpected-failure path sendsltx2_stream_complete(reason="error")+ close 1011 but never theerrorframe. The client this PR is written for readsltx2_stream_completeas a normal finish and onlyerroras a session error, and ignoresreason(apps/dreamverse/web/src/lib/ws/protocol.ts:30-31,apps/dreamverse/web/src/lib/ws/reducer.ts:470-491). A server failure therefore still looks like a clean end — the same misread as the timeout case.docs/design/server_contracts/streaming.md:98also says fatal errors precede a close with anerror(code, message) frame. Please sendErrorMessage(code="internal_error", ...)before closing; whether you keep theltx2_stream_completethere too is your call.
While you're there (docs)
docs/design/server_contracts/streaming.md:95—stop_requestedis documented for "client requested clean shutdown", but nothing can emit it now:ClientMessagehas no stop message,disconnecteddeliberately sends nothing, and the only constructions areerror/segment_cap. Drop it from the row and fromprotocol.py:170's Literal/default, or mark it reserved.docs/design/server_contracts/streaming.md:44— the lifecycle diagram still routessegment_captoTIMEOUT;server.py:250now transitions toCOMPLETE, matching the prose below. Line 96 also sayssession_timeoutcarriesreason, but the frame carriestimeout_seconds(protocol.py:173) and now closes 1000/session_timeout.
— fv-superagent, automated maintainer run by @aryan5v
…or frame; doc and test fixes from review (hao-ai-lab#2072)
|
Thanks. Addressed in 3f6a9dd:
Greptile's notes: I removed the |
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 4/5 (was 3/5)
The must-fix is fixed: an unexpected loop failure now sends ErrorMessage(code="internal_error", retryable=False) and closes 1011 internal_error, and dropping the ltx2_stream_complete there was right — the client clears the notice on stream/completed. The doc rows and lifecycle diagram match the code, and the _deadline SIGALRM guard is sound. Nothing blocking; one leftover:
docs/design/server_contracts/streaming.md:163— the example-flow footer still ends(close) → (session → COMPLETE), but a client close returns"disconnected"with no frame and no transition (server.py:353;_handle_sessionhas no branch for it). Row 95 now says onlysegment_capis emitted, so this is the last place still claiming a clean leave reachesCOMPLETE.
Two optional follow-ups in the same area: streaming.md:54-55 says non-terminal sessions transition to TIMEOUT, but the opening-frame timeout runs while the session is INITIALIZING, which has no INITIALIZING -> TIMEOUT edge, so the transition at server.py:276 is a swallowed no-op (low impact — the session is dropped right after). And server.py:246 wraps only _run_segment_loop, so a fatal exception earlier in _handle_session still reaches _stream's catch-all without an error frame; worth a follow-up if you want streaming.md:98 to hold universally.
— fv-superagent · run by @aryan5v
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 5/5 — the doc nit is fixed in fdfa12b; all checks green, Greptile 5/5.
|
Heads up @stayinalive181: please rebase on — fv-superagent · run by @aryan5v |
Purpose
The last frame a
/v1/streamclient receives does not say why the session ended._handle_sessionsendsltx2_stream_completewithreason: "stop_requested"from afinally, whatever stopped the segment loop.protocol.pydefines thesegment_capanderrorreasons and aSessionTimeoutframe, anddocs/design/server_contracts/streaming.mdsayssession_timeoutis sent when the session hitssession_timeout_secondsand is "immediately followed by close". The server never sends either the timeout frame or those two reasons.Clients built to that contract read the frames differently. The DreamVerse web client sets
sessionExpired: trueonsession_timeoutand treatsltx2_stream_completeas a normal finish, and DreamVerse's own backend sendssession_timeoutand then closes with 1000. Against this server, an idle timeout therefore looks like a stream that finished normally.Measured over a real connection (uvicorn + a
websocketsclient, with the mock generator fromtest_server.py,generation_segment_cap=2,session_timeout_seconds=2):main(f886424)generation_segment_capltx2_stream_completestop_requested; close 1006ltx2_stream_completesegment_cap; close 1000segment_cap; session stateCOMPLETEsession_timeout_secondsltx2_stream_completestop_requested; close 1006session_timeout{"timeout_seconds": 2}; close 1000session_timeoutsession_timeout_secondssession_timeout; close 1000session_timeoutltx2_stream_completestop_requestederrorinternal_error(retryable: false); close 1011internal_errorChanges
_run_segment_loopreturns why it stopped (segment_cap,idle_timeoutordisconnected), and_handle_sessionsends the matching last frame and closes:segment_cap:ltx2_stream_completesegment_cap, close 1000, transition toCOMPLETE(the statestreaming.mdgives a capped session).idle_timeout(and the opening-frame timeout in_read_init_message):session_timeout, close 1000.disconnected: nothing. Before, thefinallytried to send to a closed socket.errorframe (internal_error, not retryable), closes with 1011, and is re-raised to the existing catch-all in_stream, which logs it as before. It sends noltx2_stream_complete: the DreamVerse client clears its error notice onstream/completed.streaming.md: the lifecycle diagram putssegment_capunderCOMPLETE. Theltx2_stream_complete,session_timeoutanderrorrows give their fields and close codes, and markstop_requestedanderroras reserved reasons.Test Plan
Test Results
TestSessionEnd(test_server.py) cover the segment cap, the idle timeout, the opening-frame timeout and an unexpected failure. Each runs under a 10 sSIGALRMdeadline, so a missing frame fails the test instead of hanging the unit lane. Onmainall four fail: three getltx2_stream_completestop_requestedinstead of the expected frame, and the opening-frame one fails withTimeoutError: no server frame within 10 s.fastvideo/tests/entrypoints/streaming/: 176 passed (172 onmain), CPU only, macOS, Python 3.12, Starlette 1.10. The fMP4 tests ran with ffmpeg 9.0.Real-connection output
#2073 makes malformed client frames return
invalid_messageinstead of ending the session. Both PRs touch_run_segment_loop, so whichever merges second needs a small rebase. I will do it.Checklist
pre-commit run --all-filesand fixed all issues (ran on the changed files)