diff --git a/CHANGELOG.md b/CHANGELOG.md index 73304c6..abf1b23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **An interrupted session upload no longer leaves partial bytes behind.** A + client disconnect (`ClientDisconnect`) or a cancelled task + (`CancelledError`) bypassed the `OSError`/`ValueError` handlers, so a + half-streamed chunk stayed appended to `frames.h264` while the session's + byte counter still reported its pre-request value — a retry could then + corrupt the elementary stream and the cap no longer matched disk contents. + Both the video append and the audio upload now roll back on every + unsuccessful exit: the video is truncated to the accepted prefix and its + counter restored, and the incomplete mix is removed so a retry is not + refused with `409`. Cancellation is re-raised after cleanup. - **A partial Split Screen offline-render bridge is reported as a version problem instead of a generic render failure.** The bridge calls were individually optional-chained, so a Split Screen build providing diff --git a/CLAUDE.md b/CLAUDE.md index c4591cb..7679fa7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -132,7 +132,14 @@ Encoded chunks go to a server-side export session (`/sessions`, streamed to `/sessions/{id}/video`) when those routes exist; the browser falls back to buffering everything and POSTing `/mux` when the host predates them or the audio source needs uploading. Session temp dirs are cleaned on cancel, on mux, -and by a TTL sweep — keep all three paths. +and by a TTL sweep — keep all three paths. Session uploads roll back on *every* +unsuccessful stream exit, not just `OSError`/`ValueError`: the video append +truncates `frames.h264` back to the accepted prefix and restores the byte +counter, and the audio upload removes the incomplete mix. A narrower handler +misses `ClientDisconnect` (a client that drops mid-stream) and `CancelledError` +(a cancelled task), so the cleanup catches `BaseException` and re-raises — that +re-raise is what keeps cancellation working, don't swallow it to return a +response. **Server-side audio resolution is opt-in and origin-pinned.** The browser only ever sends a site-relative `audio_url`, but that constrains the path, not the diff --git a/plugin.json b/plugin.json index b6ffbe8..13b17ee 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "id": "visual_export", "name": "Visual Export", - "version": "0.3.1", + "version": "0.3.2", "description": "Renders the current player layout to a video file for the entire song, without a live performance.", "type": "tools", "script": "screen.js", diff --git a/routes.py b/routes.py index 3f358d0..3cff6e5 100644 --- a/routes.py +++ b/routes.py @@ -332,11 +332,16 @@ async def append_session_video(session_id: str, request: Request) -> JSONRespons raise ValueError("encoded video is too large") target.write(chunk) state["video_bytes"] = start + total - except (OSError, ValueError): - # Roll back inside the lock. Doing it after release would - # let a concurrent append land between the write and the - # truncate, and this rollback would then discard an accepted - # chunk and un-bind the cap. + except BaseException: + # Roll back inside the lock on every unsuccessful exit, not + # just OSError/ValueError: a client disconnect + # (ClientDisconnect) or task cancellation (CancelledError) + # bypasses a narrower handler and would otherwise leave + # partial bytes in the elementary stream. Re-raising keeps + # cancellation semantics intact. Doing it after release + # would let a concurrent append land between the write and + # the truncate, and this rollback would then discard an + # accepted chunk and un-bind the cap. state["video_bytes"] = start if total: try: @@ -373,6 +378,13 @@ async def upload_session_audio(session_id: str, request: Request) -> JSONRespons except (OSError, ValueError) as exc: audio_path.unlink(missing_ok=True) return JSONResponse({"error": str(exc)}, status_code=400) + except BaseException: + # A client disconnect or task cancellation bypasses the handler + # above. Remove the incomplete mix on every unsuccessful exit so a + # retry is not refused with 409 for a file that was never finished, + # then re-raise to preserve cancellation. + audio_path.unlink(missing_ok=True) + raise return JSONResponse({"bytes": total}) @app.post(f"/api/plugins/{PLUGIN_ID}/sessions/{{session_id}}/mux") diff --git a/tests/test_routes.py b/tests/test_routes.py index d087d88..ae00e70 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -8,6 +8,7 @@ import pytest from fastapi import FastAPI from fastapi.testclient import TestClient +from starlette.requests import ClientDisconnect MODULE_PATH = Path(__file__).resolve().parents[1] / "routes.py" @@ -86,6 +87,17 @@ def _client(tmp_path): return TestClient(app) +def _endpoint(app, path, method): + for route in app.routes: + if getattr(route, "path", None) == path and method in (route.methods or set()): + return route.endpoint + raise AssertionError(f"no {method} route for {path}") + + +VIDEO_END = "/api/plugins/visual_export/sessions/{session_id}/video" +AUDIO_END = "/api/plugins/visual_export/sessions/{session_id}/audio" + + def _isolated_work(tmp_path, monkeypatch): """Point session temp dirs at tmp_path, one fresh dir per session.""" counter = [0] @@ -371,6 +383,136 @@ def test_rejected_append_keeps_earlier_chunks_and_the_cap(tmp_path, monkeypatch) ).status_code == 200 +@pytest.mark.asyncio +async def test_disconnected_video_stream_rolls_back_to_accepted_prefix(tmp_path, monkeypatch): + """The accepted prefix of frames.h264 survives when the client drops mid-append. + + A stream cancel is a starlette ClientDisconnect, not an OSError/ValueError, + so it used to bypass the rollback: the uncompressed bytes stayed in the + elementary stream while video_bytes stayed at its pre-request snapshot, + letting a retry corrupt the stream and un-bind the cap. + """ + work = tmp_path / "work" + monkeypatch.setattr(routes, "_ffmpeg_cmd", lambda: "ffmpeg") + monkeypatch.setattr(routes, "MAX_VIDEO_BYTES", 10) + monkeypatch.setattr(routes.tempfile, "mkdtemp", lambda **kwargs: str(work.mkdir() or work)) + client = _client(tmp_path) + session = _session(client, audio_url="/songs/pack/full_mix.wav").json()["session"] + url = f"/api/plugins/visual_export/sessions/{session}/video" + assert client.post(url, content=b"abcde").status_code == 200 + + async def interrupted_stream(): + yield b"zzz" + raise ClientDisconnect() + + with pytest.raises(ClientDisconnect): + await _endpoint(client.app, VIDEO_END, "POST")(session, SimpleNamespace(stream=interrupted_stream)) + + # The partial bytes are gone and the counter is back to the file's size: a + # 5-byte append fits only if the snapshot was truly restored. + assert (work / "frames.h264").read_bytes() == b"abcde" + assert client.post(url, content=b"fghij").status_code == 200 + assert (work / "frames.h264").read_bytes() == b"abcdefghij" + assert client.post(url, content=b"x").status_code == 400 + + +@pytest.mark.asyncio +async def test_cancelled_video_stream_rolls_back_and_preserves_cancellation(tmp_path, monkeypatch): + """Task cancellation restores the prefix and still propagates.""" + import asyncio + + work = tmp_path / "work" + monkeypatch.setattr(routes, "_ffmpeg_cmd", lambda: "ffmpeg") + monkeypatch.setattr(routes, "MAX_VIDEO_BYTES", 10) + monkeypatch.setattr(routes.tempfile, "mkdtemp", lambda **kwargs: str(work.mkdir() or work)) + client = _client(tmp_path) + session = _session(client, audio_url="/songs/pack/full_mix.wav").json()["session"] + url = f"/api/plugins/visual_export/sessions/{session}/video" + assert client.post(url, content=b"abcde").status_code == 200 + + first_chunk_written = asyncio.Event() + + async def stuck_stream(): + yield b"zzz" + await asyncio.sleep(0) + first_chunk_written.set() + await asyncio.Event().wait() + + task = asyncio.create_task( + _endpoint(client.app, VIDEO_END, "POST")(session, SimpleNamespace(stream=stuck_stream)) + ) + await first_chunk_written.wait() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert (work / "frames.h264").read_bytes() == b"abcde" + assert client.post(url, content=b"fghij").status_code == 200 + assert (work / "frames.h264").read_bytes() == b"abcdefghij" + + +@pytest.mark.asyncio +async def test_disconnected_audio_stream_is_removed_and_retry_succeeds(tmp_path, monkeypatch): + """A half-uploaded mix must not leave a 409 in the client's way.""" + work = tmp_path / "work" + monkeypatch.setattr(routes, "_ffmpeg_cmd", lambda: "ffmpeg") + monkeypatch.setattr(routes.tempfile, "mkdtemp", lambda **kwargs: str(work.mkdir() or work)) + _stub_ffmpeg(monkeypatch, work, video=b"video", audio=b"audio") + client = _client(tmp_path) + session = _session(client).json()["session"] + client.post(f"/api/plugins/visual_export/sessions/{session}/video", content=b"video") + url = f"/api/plugins/visual_export/sessions/{session}/audio" + + async def interrupted_stream(): + yield b"partial" + raise ClientDisconnect() + + with pytest.raises(ClientDisconnect): + await _endpoint(client.app, AUDIO_END, "POST")(session, SimpleNamespace(stream=interrupted_stream)) + + assert not (work / "audio.bin").exists() + assert client.post(url, content=b"audio").status_code == 200 + assert (work / "audio.bin").read_bytes() == b"audio" + assert client.post( + f"/api/plugins/visual_export/sessions/{session}/mux", json={"fps": 30, "duration": 1.0} + ).status_code == 200 + + +@pytest.mark.asyncio +async def test_cancelled_audio_stream_is_removed_and_retry_succeeds(tmp_path, monkeypatch): + """Task cancellation also discards the incomplete upload.""" + import asyncio + + work = tmp_path / "work" + monkeypatch.setattr(routes, "_ffmpeg_cmd", lambda: "ffmpeg") + monkeypatch.setattr(routes.tempfile, "mkdtemp", lambda **kwargs: str(work.mkdir() or work)) + _stub_ffmpeg(monkeypatch, work, video=b"video", audio=b"audio") + client = _client(tmp_path) + session = _session(client).json()["session"] + client.post(f"/api/plugins/visual_export/sessions/{session}/video", content=b"video") + url = f"/api/plugins/visual_export/sessions/{session}/audio" + + first_chunk_written = asyncio.Event() + + async def stuck_stream(): + yield b"partial" + await asyncio.sleep(0) + first_chunk_written.set() + await asyncio.Event().wait() + + task = asyncio.create_task( + _endpoint(client.app, AUDIO_END, "POST")(session, SimpleNamespace(stream=stuck_stream)) + ) + await first_chunk_written.wait() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert not (work / "audio.bin").exists() + assert client.post(url, content=b"audio").status_code == 200 + assert (work / "audio.bin").read_bytes() == b"audio" + + def test_audio_fetch_requires_a_configured_origin(tmp_path, monkeypatch): """A client-supplied Host header must not be able to aim the fetch.