Repository navigation
[bugfix] Streaming: answer malformed client frames with invalid_message instead of dropping the session - #2073
Conversation
…ge instead of dropping the session
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 PR merge requirements
|
[Medium impact] The PR appears safe to merge; no blocking issues remain.
|
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 4/5
Read the diff and the call sites; the fix is right. _receive_client_json correctly treats a missing text as a binary frame, the disconnect branch mirrors Starlette's, and the two stages now match docs/design/server_contracts/streaming.md (:78 for the opening frame, :98 for non-fatal errors keeping the connection open). Nothing here blocks merge; two non-blocking points:
-
fastvideo/tests/entrypoints/streaming/test_server.py:104-121, 168-195— the new tests callreceive_json()/receive()with no timeout, so a regression in the path under test hangs the unit lane (.buildkite/scripts/unit_test.sh) instead of failing it;pyproject.tomlconfigures nopytest-timeout. The PR body already notes this for the before-change comparison. A bounded receive helper would make a future regression fail fast. The same blocking pattern already exists in this file, so this is a nit. -
docs/design/server_contracts/streaming.md:98— theerrorrow listscode, message, butErrorMessagealso carriesretryable, which this change relies on, and "client to server frames are JSON text" is now a tested contract. One line in the catalogue would close that gap; the doc header only requires edits whenprotocol.py/session.pychange, so "not needed" is defensible.
CI already collects this directory, and the four tests cover both frame kinds at both stages, including a snapshot_state round trip that proves the session survives.
— fv-superagent · run by @aryan5v
…JSON-text client frames (hao-ai-lab#2073)
|
Thanks. Addressed in 9e40574:
|
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 5/5 (was 4/5)
Re-reviewed only the delta in 9e40574; server.py is unchanged.
- Point 1 (tests could hang on regression) is addressed:
_deadline()wraps the three websocket tests with a 10 sSIGALRMthat raisesTimeoutError, and no-ops whereSIGALRMis unavailable. A regression now fails the unit lane instead of stalling it. - Point 2 (contract) is addressed: a paragraph after the client to server table in
streaming.mdstates that client frames are JSON text and that a malformed frame is answered withinvalid_message—1003 invalid_initon the opening frame, retryable and session-continuing mid-session. Moving theretryablefield on theerrorrow to #2072 is a fair call to avoid a conflict in that table. - The comment Greptile flagged is gone.
Nothing blocking from my side.
— fv-superagent · run by @aryan5v
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 5/5 — malformed-frame handling matches the streaming contract; Greptile clean and the new tests are bounded.
|
/merge |
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Purpose
On
/v1/stream, one client frame that is not JSON text ends the session without anerrorframe._read_init_messageand_run_segment_loopread frames withWebSocket.receive_json(). For text that is not JSON it raisesjson.JSONDecodeError, and for a binary frame it raisesKeyError: 'text'. Neither is caught at those call sites, so the exception reaches the catch-all in_stream, which logsunhandled errorand drops the socket. A frame that is valid JSON but fails schema validation already getsinvalid_messageand the session continues, anddocs/design/server_contracts/streaming.mdsays validation errors are reported witherrorand non-fatal ones keep the connection open.Measured over a real connection (uvicorn + a
websocketsclient, with the mock generator fromtest_server.py):main(f886424){not jsonas the opening frameerrorinvalid_message(retryable: false); close 1003invalid_init{not jsonmid-sessionltx2_stream_completestop_requested; close 1006errorinvalid_message(retryable: true); the session keeps serving (snapshot_stateis answered)Changes
_receive_client_json()reads one frame and decodes it. A binary frame or text that is not JSON raises_MalformedClientFrame.invalid_message, then close 1003invalid_init).invalid_messagewithretryable: true, and the loop continues, as for a schema-invalid frame.streaming.md: one paragraph after the client-to-server table says client frames are JSON text and how a malformed frame is answered.Test Plan
Test Results
test_server.py: a non-JSON and a binary opening frame, and both kinds mid-session followed by asnapshot_stateround trip. Each runs under a 10 sSIGALRMdeadline, so a missing frame fails the test instead of hanging the unit lane. Onmainall four fail: the two opening-frame tests withTimeoutError: no server frame within 10 s(the server sends nothing), and the two mid-session tests because they getltx2_stream_completeinstead oferror.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, main
Real-connection output, this PR
#2072 fixes the reason in the session's last frame: it fixes the
stop_requestedabove and adds the documentedsession_timeout. 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)streaming.md: client frames are JSON text, and how a malformed one is answered)