From 75fa028e89b7e675053decc660dde95a8bd78b58 Mon Sep 17 00:00:00 2001
From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com>
Date: Sat, 18 Jul 2026 16:31:29 -0700
Subject: [PATCH] fix(artifacts): serve inline binary artifacts via
 FileResponse for Range support (#4281)

* fix(artifacts): serve inline binary artifacts via FileResponse for Range support

Audio/video/image artifacts that are previewed inline (not downloaded, not
active content) were served by reading the whole file into memory and
returning it through a plain Response. That response never sets
Accept-Ranges and ignores any Range header the browser sends, so seeking
an <audio>/<video> element backed by this endpoint always gets the full
200 response back instead of a 206 partial response for the requested byte
range -- which is why dragging the seek bar on an audio artifact restarts
playback from the beginning instead of jumping to the new position.

Route this case through FileResponse instead (as the active-content/
download branch already does), which handles Range/If-Range natively.
Verified with a real Range request against the endpoint: initial load now
reports Accept-Ranges: bytes, and a ranged GET returns 206 with the correct
Content-Range and only the requested slice of bytes.

Fixes #3240

* fix(artifacts): address review nits on the inline FileResponse branch

Two non-blocking nits from review:

- Drop the redundant filename= kwarg on the inline_file FileResponse
  call. Content-Disposition is already set explicitly on this branch,
  and FileResponse only uses filename to setdefault that same header
  -- which is a no-op once it's already present. Harmless today, but
  removes the latent risk that a future Starlette version turning that
  setdefault into a hard set would silently flip inline preview to
  attachment.
- Reword the blocking-IO regression-anchor docstring: it claimed
  awaiting get_artifact for a binary artifact does "zero filesystem
  IO", but _read_artifact_payload still runs exists/is_file/
  mimetypes.guess_type/is_text_file_by_content (an 8 KB read) for
  binary files too, just offloaded via asyncio.to_thread like the text
  branch. The gate has nothing to catch because that IO is off the
  event loop, not because there's none -- reworded to say no full-file
  read happens, matching the accurate framing already used one
  paragraph up.

tests/test_artifacts_router.py, tests/blocking_io/test_artifacts_router.py
(19 tests) are green, and ruff check/format are clean on both changed
files.
---
 backend/app/gateway/routers/artifacts.py      | 33 +++++++++---
 .../blocking_io/test_artifacts_router.py      | 31 ++++++++---
 backend/tests/test_artifacts_router.py        | 54 +++++++++++++++++++
 3 files changed, 104 insertions(+), 14 deletions(-)

diff --git a/backend/app/gateway/routers/artifacts.py b/backend/app/gateway/routers/artifacts.py
index d7580eb40..d39c066e3 100644
--- a/backend/app/gateway/routers/artifacts.py
+++ b/backend/app/gateway/routers/artifacts.py
@@ -123,11 +123,14 @@ def _read_artifact_payload(actual_path: Path, path: str, download: bool) -> tupl
     """Worker-thread body for the regular branch of ``get_artifact``.
 
     Stat probes, MIME sniffing (``mimetypes`` lazily stats the system MIME
-    database on first use), and full-file reads are all blocking filesystem IO.
-    Returns a ``(kind, mime_type, payload)`` plan the handler turns into a
-    response on the loop: ``("file", mime, None)`` (let ``FileResponse`` stream
-    it), ``("text", mime, str)``, or ``("bytes", mime, bytes)``. Behavior/error
-    codes match the previous inline logic.
+    database on first use), and text reads are blocking filesystem IO. Returns
+    a ``(kind, mime_type, payload)`` plan the handler turns into a response on
+    the loop: ``("file", mime, None)`` (attachment / forced-download active
+    content, streamed by ``FileResponse``), ``("inline_file", mime, None)``
+    (inline binary preview — also streamed by ``FileResponse`` so the client
+    can issue byte-``Range`` requests, e.g. to seek within audio/video
+    artifacts instead of always replaying from byte 0), or ``("text", mime,
+    str)``. Behavior/error codes match the previous inline logic.
     """
     if not actual_path.exists():
         raise HTTPException(status_code=404, detail=f"Artifact not found: {path}")
@@ -141,7 +144,10 @@ def _read_artifact_payload(actual_path: Path, path: str, download: bool) -> tupl
         return ("text", mime_type, actual_path.read_text(encoding="utf-8"))
     if is_text_file_by_content(actual_path):
         return ("text", mime_type, actual_path.read_text(encoding="utf-8"))
-    return ("bytes", mime_type, actual_path.read_bytes())
+    # Binary inline preview (images, audio, video, PDFs, ...): stream via
+    # FileResponse instead of buffering the whole file in memory, so it also
+    # gets FileResponse's built-in byte-Range handling (see get_artifact).
+    return ("inline_file", mime_type, None)
 
 
 @router.get(
@@ -235,7 +241,20 @@ async def get_artifact(thread_id: str, path: str, request: Request, download: bo
         # execution in the application origin when users open generated artifacts.
         return FileResponse(path=actual_path, filename=actual_path.name, media_type=mime_type, headers=_build_attachment_headers(actual_path.name))
 
+    if kind == "inline_file":
+        # FileResponse (unlike a fully-buffered Response) honors byte-Range
+        # requests. Browsers issue these when seeking an <audio>/<video>
+        # element backed by a remote URL; serving the same bytes through a
+        # plain Response ignores Range headers and always replays from byte
+        # 0, which is why dragging an audio/video artifact's progress bar
+        # reset playback to the start instead of jumping to the new position.
+        return FileResponse(
+            path=actual_path,
+            media_type=mime_type,
+            headers={"Content-Disposition": _build_content_disposition("inline", actual_path.name)},
+        )
+
     if kind == "text":
         return PlainTextResponse(content=payload, media_type=mime_type)
 
-    return Response(content=payload, media_type=mime_type, headers={"Content-Disposition": _build_content_disposition("inline", actual_path.name)})
+    raise AssertionError(f"Unhandled artifact response kind: {kind!r}")
diff --git a/backend/tests/blocking_io/test_artifacts_router.py b/backend/tests/blocking_io/test_artifacts_router.py
index f444064f0..7bb0d6751 100644
--- a/backend/tests/blocking_io/test_artifacts_router.py
+++ b/backend/tests/blocking_io/test_artifacts_router.py
@@ -1,11 +1,22 @@
 """Regression anchor: serving artifacts must not block the event loop.
 
 ``get_artifact`` probes the artifact path (``exists`` / ``is_file``), reads
-text/binary content (``read_text`` / ``read_bytes``), sniffs text-ness
-(``is_text_file_by_content``), and extracts ``.skill`` archive members — all
-blocking filesystem IO. The handler offloads each branch's IO via
-``asyncio.to_thread``; if any regresses back onto the event loop, the strict
-Blockbuster gate raises ``BlockingError`` and these tests fail.
+text content (``read_text``), sniffs text-ness (``is_text_file_by_content``),
+and extracts ``.skill`` archive members — all blocking filesystem IO. The
+handler offloads each branch's IO via ``asyncio.to_thread``; if any regresses
+back onto the event loop, the strict Blockbuster gate raises ``BlockingError``
+and these tests fail.
+
+Binary (non-text, non-active-content) artifacts still run the same
+``exists`` / ``is_file`` / ``mimetypes.guess_type`` / ``is_text_file_by_content``
+probes as the text branch, offloaded the same way via ``asyncio.to_thread``.
+What differs is the payload: instead of a read, the handler returns a
+``FileResponse`` that defers its own file IO to ASGI response time (streamed
+via ``anyio.open_file`` when the response is actually sent, which this test
+never triggers) — so awaiting ``get_artifact`` itself for a binary artifact
+does no full-file read; ``FileResponse`` streams the bytes at ASGI send
+time, off the loop, which is why the gate has nothing to catch there
+either way.
 
 The ``@require_permission`` decorator is bypassed via ``__wrapped__`` so the
 anchor exercises the handler's own filesystem IO, not the authz layer. Imports
@@ -19,6 +30,7 @@ import zipfile
 from pathlib import Path
 
 import pytest
+from starlette.responses import FileResponse
 
 from app.gateway.path_utils import resolve_thread_virtual_path
 from app.gateway.routers.artifacts import get_artifact
@@ -56,13 +68,18 @@ async def test_get_artifact_text_does_not_block_event_loop(tmp_path: Path, monke
 async def test_get_artifact_binary_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
     vpath = "mnt/user-data/outputs/blob.bin"
     target = await _seed(tmp_path, monkeypatch, "t1", vpath)
-    payload = b"\x00\x01\x02PNGDATA"  # null byte -> binary branch (read_bytes)
+    payload = b"\x00\x01\x02PNGDATA"  # null byte -> binary branch (inline FileResponse)
     await asyncio.to_thread(target.write_bytes, payload)
 
     resp = await _get_artifact("t1", vpath, request=None, download=False)
 
+    # Binary artifacts are streamed via FileResponse (so browsers can issue
+    # byte-Range requests) instead of being read into memory up front, so the
+    # file bytes are only touched later during ASGI send, never here.
+    assert isinstance(resp, FileResponse)
     assert resp.status_code == 200
-    assert resp.body == payload
+    assert Path(resp.path) == target
+    assert resp.headers.get("content-disposition", "").startswith("inline;")
 
 
 async def test_get_artifact_skill_archive_member_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None:
diff --git a/backend/tests/test_artifacts_router.py b/backend/tests/test_artifacts_router.py
index f53078e0b..6f5864c81 100644
--- a/backend/tests/test_artifacts_router.py
+++ b/backend/tests/test_artifacts_router.py
@@ -90,6 +90,60 @@ def test_get_artifact_download_false_does_not_force_attachment(tmp_path, monkeyp
     assert "content-disposition" not in response.headers
 
 
+def test_get_artifact_binary_preview_is_inline_file_response(tmp_path, monkeypatch) -> None:
+    # Binary (non-text, non-active-content) artifacts must go through the
+    # "inline_file" plan so get_artifact serves them via FileResponse.
+    artifact_path = tmp_path / "clip.mp3"
+    artifact_path.write_bytes(b"\x00\x01ID3fakeaudiobytes")
+
+    monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path)
+
+    response = asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/clip.mp3", _make_request()))
+
+    assert isinstance(response, FileResponse)
+    assert response.media_type == "audio/mpeg"
+    assert response.headers.get("content-disposition", "").startswith("inline;")
+
+
+def test_get_artifact_binary_preview_supports_range_requests(tmp_path, monkeypatch) -> None:
+    # Regression test for #3240: dragging an audio/video artifact's seek bar
+    # reset playback to the start because the binary-preview branch used to
+    # buffer the whole file into a plain Response, which ignores byte-Range
+    # requests entirely (always 200 + full body, never 206). Browsers fall
+    # back to restarting playback from byte 0 when a seek's Range request
+    # doesn't come back as 206. FileResponse (used for the "file" plan
+    # already) handles Range/If-Range natively, so switching the binary
+    # branch to FileResponse fixes seeking for free.
+    # Cycle through all 256 byte values (incl. \x00) so is_text_file_by_content
+    # correctly sniffs this as binary, same as a real audio file would be -- an
+    # all-printable-ASCII payload would (correctly) be sniffed as text and miss
+    # the branch this test targets.
+    payload = bytes(i % 256 for i in range(1_000_000))
+    artifact_path = tmp_path / "clip.mp3"
+    artifact_path.write_bytes(payload)
+
+    monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path)
+
+    app = make_authed_test_app()
+    app.include_router(artifacts_router.router)
+
+    with TestClient(app) as client:
+        full = client.get("/api/threads/thread-1/artifacts/mnt/user-data/outputs/clip.mp3")
+        seek = client.get(
+            "/api/threads/thread-1/artifacts/mnt/user-data/outputs/clip.mp3",
+            headers={"Range": "bytes=500000-"},
+        )
+
+    assert full.status_code == 200
+    assert full.headers.get("accept-ranges") == "bytes"
+    assert full.content == payload
+
+    assert seek.status_code == 206
+    assert seek.headers.get("content-range") == f"bytes 500000-999999/{len(payload)}"
+    assert seek.content == payload[500000:]
+    assert seek.headers.get("content-disposition", "").startswith("inline;")
+
+
 def test_get_artifact_download_true_forces_attachment_for_skill_archive(tmp_path, monkeypatch) -> None:
     skill_path = tmp_path / "sample.skill"
     with zipfile.ZipFile(skill_path, "w") as zip_ref: