From 6752da9a7735add1aff6ebc632c7e83fc4005a48 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 18 Jun 2026 11:32:18 +0530 Subject: [PATCH] fix(dashboard): clean up upload temp file on client disconnect + pin python-multipart (NS-501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #47663 (streaming multipart upload), fixing two issues that landed with it. 1. Temp file leaked on client disconnect. The streaming upload endpoint's except chain caught only HTTPException / PermissionError / OSError — all Exception subclasses. asyncio.CancelledError, raised when a browser aborts a large upload mid-stream (the exact NS-501 scenario), is a BaseException, so it bypassed every except clause and reached a finally that only closed the file handle and never unlinked the temp file. Every aborted large upload orphaned a partial `.{name}.*.upload` file (up to ~100 MB) in the target directory. Cleanup now lives in finally, keyed on a `renamed` success flag, so the temp file is removed on every non-success exit including BaseException paths. Added test_stream_upload_cleans_temp_on_cancellation, which fails on the pre-fix code (leaks the temp file) and passes with the fix. 2. python-multipart pinned to ==0.0.27 instead of ==0.0.20. The package was already resolved at 0.0.27 transitively (via daytona) before #47663; the explicit ==0.0.20 pin in the [web] extra and the tool.dashboard lazy-install set downgraded it. Bumped both to ==0.0.27 and regenerated with `uv lock`, keeping the lockfile coherent. The base dependency stays >=0.0.9,<1. --- hermes_cli/web_server.py | 12 ++++-- pyproject.toml | 2 +- tests/hermes_cli/test_web_server_files.py | 52 +++++++++++++++++++++++ tools/lazy_deps.py | 2 +- uv.lock | 8 ++-- 5 files changed, 67 insertions(+), 9 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index ed619979bfb..ad82d9fdfef 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -1529,6 +1529,7 @@ async def upload_managed_file_stream( ) tmp_path = Path(tmp_name) total = 0 + renamed = False try: with os.fdopen(tmp_fd, "wb") as out: while True: @@ -1540,16 +1541,21 @@ async def upload_managed_file_stream( raise HTTPException(status_code=413, detail="File is too large") out.write(chunk) os.replace(tmp_path, target) + renamed = True except HTTPException: - tmp_path.unlink(missing_ok=True) raise except PermissionError: - tmp_path.unlink(missing_ok=True) raise HTTPException(status_code=403, detail="File is not writable") except OSError as exc: - tmp_path.unlink(missing_ok=True) raise HTTPException(status_code=500, detail=f"Could not write file: {exc}") finally: + # Clean up the temp file on every non-success exit, including + # BaseException paths the `except` clauses above don't catch — most + # importantly asyncio.CancelledError when a browser aborts a large + # upload mid-stream (the exact NS-501 scenario). os.replace clears + # tmp_path on success, so only unlink when the rename didn't happen. + if not renamed: + tmp_path.unlink(missing_ok=True) await file.close() return { diff --git a/pyproject.toml b/pyproject.toml index 6e371126dd2..cab849dc755 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -258,7 +258,7 @@ youtube = [ # `hermes dashboard` (localhost SPA + API). Not in core to keep the default install lean. # starlette==1.0.1 pinned for CVE-2026-48710 (BadHost) — fastapi pulls Starlette # transitively and pre-1.0.1 is the vulnerable range. See the mcp extra above. -web = ["fastapi==0.133.1", "uvicorn[standard]==0.41.0", "starlette==1.0.1", "python-multipart==0.0.20"] +web = ["fastapi==0.133.1", "uvicorn[standard]==0.41.0", "starlette==1.0.1", "python-multipart==0.0.27"] all = [ # Policy (2026-05-12): `[all]` includes only extras that genuinely # CAN'T be lazy-installed via `tools/lazy_deps.py` — i.e. things every diff --git a/tests/hermes_cli/test_web_server_files.py b/tests/hermes_cli/test_web_server_files.py index 46ba18b1355..b295f0ab998 100644 --- a/tests/hermes_cli/test_web_server_files.py +++ b/tests/hermes_cli/test_web_server_files.py @@ -436,3 +436,55 @@ def test_stream_upload_large_file_under_cap_succeeds(forced_files_client, monkey assert created.status_code == 200 assert file_path.stat().st_size == len(payload) assert file_path.read_bytes() == payload + + +def test_stream_upload_cleans_temp_on_cancellation(forced_files_client): + """A client disconnect mid-stream (asyncio.CancelledError) must not leak a temp file. + + CancelledError is a BaseException, not an Exception, so it bypasses the + endpoint's ``except`` clauses entirely. The cleanup therefore lives in a + ``finally`` keyed on a success flag — without it, every aborted large + upload (the exact NS-501 scenario) would orphan a partial ``.upload`` temp + file in the target directory. We invoke the endpoint coroutine directly so + the BaseException propagates instead of being swallowed by the test client. + """ + import asyncio + + _client, root = forced_files_client + target = root / "out" / "aborted.bin" + target.parent.mkdir(parents=True, exist_ok=True) + + class _AbortingUpload: + """UploadFile stand-in that yields one chunk then aborts like a dropped client.""" + + filename = "aborted.bin" + + def __init__(self): + self._calls = 0 + + async def read(self, _size): + self._calls += 1 + if self._calls == 1: + return b"partial chunk before the client vanished" + raise asyncio.CancelledError() + + async def close(self): + return None + + request = SimpleNamespace() + + with pytest.raises(asyncio.CancelledError): + asyncio.run( + web_server.upload_managed_file_stream( + request=request, + file=_AbortingUpload(), + path=str(target), + overwrite=True, + ) + ) + + # No partial data was promoted into place ... + assert not target.exists() + # ... and no .upload temp file was left behind. + leftovers = [p.name for p in target.parent.iterdir() if ".upload" in p.name] + assert leftovers == [], f"temp upload files leaked on cancellation: {leftovers}" diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index 98bacbf42a0..4e2159a1a02 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -178,7 +178,7 @@ LAZY_DEPS: dict[str, tuple[str, ...]] = { "fastapi==0.133.1", "uvicorn[standard]==0.41.0", "starlette==1.0.1", # CVE-2026-48710 (BadHost) — keep lazy-install in sync with pyproject [web] - "python-multipart==0.0.20", # FastAPI UploadFile/Form for streaming uploads (NS-501) + "python-multipart==0.0.27", # FastAPI UploadFile/Form for streaming uploads (NS-501) ), # Vision image-resize recovery (Pillow). Pillow is now a CORE dependency # (pyproject `dependencies`), so this entry is a belt-and-suspenders fallback diff --git a/uv.lock b/uv.lock index fc340bdbe89..095b7563311 100644 --- a/uv.lock +++ b/uv.lock @@ -1713,7 +1713,7 @@ requires-dist = [ { name = "pytest-asyncio", marker = "extra == 'dev'", specifier = "==1.3.0" }, { name = "python-dotenv", specifier = "==1.2.2" }, { name = "python-multipart", specifier = ">=0.0.9,<1" }, - { name = "python-multipart", marker = "extra == 'web'", specifier = "==0.0.20" }, + { name = "python-multipart", marker = "extra == 'web'", specifier = "==0.0.27" }, { name = "python-telegram-bot", extras = ["webhooks"], marker = "extra == 'messaging'", specifier = "==22.6" }, { name = "python-telegram-bot", extras = ["webhooks"], marker = "extra == 'termux'", specifier = "==22.6" }, { name = "pywinpty", marker = "sys_platform == 'win32'", specifier = ">=2.0.0,<3" }, @@ -3317,11 +3317,11 @@ wheels = [ [[package]] name = "python-multipart" -version = "0.0.20" +version = "0.0.27" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/f3/87/f44d7c9f274c7ee665a29b885ec97089ec5dc034c7f3fafa03da9e39a09e/python_multipart-0.0.20.tar.gz", hash = "sha256:8dd0cab45b8e23064ae09147625994d090fa46f5b0d1e13af944c331a7fa9d13", size = 37158, upload-time = "2024-12-16T19:45:46.972Z" } +sdist = { url = "https://files.pythonhosted.org/packages/69/9b/f23807317a113dc36e74e75eb265a02dd1a4d9082abc3c1064acd22997c4/python_multipart-0.0.27.tar.gz", hash = "sha256:9870a6a8c5a20a5bf4f07c017bd1489006ff8836cff097b6933355ee2b49b602", size = 44043, upload-time = "2026-04-27T10:51:26.649Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/45/58/38b5afbc1a800eeea951b9285d3912613f2603bdf897a4ab0f4bd7f405fc/python_multipart-0.0.20-py3-none-any.whl", hash = "sha256:8a62d3a8335e06589fe01f2a3e178cdcc632f3fbe0d492ad9ee0ec35aab1f104", size = 24546, upload-time = "2024-12-16T19:45:44.423Z" }, + { url = "https://files.pythonhosted.org/packages/99/78/4126abcbdbd3c559d43e0db7f7b9173fc6befe45d39a2856cc0b8ec2a5a6/python_multipart-0.0.27-py3-none-any.whl", hash = "sha256:6fccfad17a27334bd0193681b369f476eda3409f17381a2d65aa7df3f7275645", size = 29254, upload-time = "2026-04-27T10:51:24.997Z" }, ] [[package]]