fix: roll back session uploads on client disconnect and cancellation

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

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

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",

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")

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.