Skip to content

Commit 19ca812

Browse files
badGarnetclaude
andcommitted
fix: clean up the async conversion's output file when cancelled
Offloading the JSON-to-NDJSON conversion to a thread last commit removed the event loop block but introduced a leak: a thread cannot be cancelled, so on cancellation the conversion still runs to completion and creates its file, while the awaiting coroutine has already raised CancelledError and discarded the path. Nothing was left that could delete it. Reproduced -- cancel mid-conversion and an unst_elements_*.ndjson survives. The conversion is now awaited through asyncio.shield, which keeps a handle on the thread's result after the caller stops waiting, and a done callback unlinks the finished file. A conversion that raised needs no callback, since it already removes its own partial file. This is the same shape as the split hook's cancellation race: work that outlives the operation that requested it, publishing a path nobody will read. It did not exist on the inline version, which had no await point to cancel at. Regression test asserts against the path production actually created, so it cannot pass by observing that no file was ever made. Confirmed to fail against the plain awaited to_thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 140ced0 commit 19ca812

2 files changed

Lines changed: 68 additions & 3 deletions

File tree

_test_unstructured_client/unit/test_ndjson_elements_file.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@
99
- leave no temp files behind other than the combined file the caller owns
1010
"""
1111

12+
import asyncio
1213
import errno
1314
import json
1415
import os
1516
import tempfile
1617
import threading
18+
import time
1719
from pathlib import Path
1820
from unittest import mock
1921

@@ -752,3 +754,41 @@ def _spy(http_res):
752754
assert _read_ndjson(res.elements_file) == elements
753755
finally:
754756
Path(res.elements_file).unlink(missing_ok=True)
757+
758+
759+
@pytest.mark.asyncio
760+
async def test_async_conversion_cleans_up_when_cancelled(tmp_path, monkeypatch):
761+
"""A cancelled conversion must not orphan the file its thread went on to create.
762+
763+
Regression guard for the cost of the offload: a thread cannot be cancelled, so the
764+
conversion runs to completion regardless, and a plain `await asyncio.to_thread(...)`
765+
discards the path it returned -- leaving nothing that could delete the file.
766+
"""
767+
monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
768+
created = _record_created_paths(monkeypatch, general, "_new_elements_file")
769+
real_convert = general._json_body_to_elements_file
770+
771+
def _slow_convert(http_res):
772+
time.sleep(0.3)
773+
return real_convert(http_res)
774+
775+
monkeypatch.setattr(general, "_json_body_to_elements_file", _slow_convert)
776+
response = httpx.Response(
777+
200, headers={"Content-Type": "application/json"}, json=[{"type": "Table"}]
778+
)
779+
780+
task = asyncio.ensure_future(general._json_body_to_elements_file_async(response))
781+
await asyncio.sleep(0.05)
782+
task.cancel()
783+
with pytest.raises(asyncio.CancelledError):
784+
await task
785+
786+
# Let the shielded thread finish and its cleanup callback run.
787+
for _ in range(200):
788+
await asyncio.sleep(0.02)
789+
if created and not os.path.exists(created[0]):
790+
break
791+
792+
# Non-vacuous: a file really was created, and it is now gone.
793+
assert len(created) == 1
794+
assert not os.path.exists(created[0])

src/unstructured_client/general.py

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,33 @@ def _json_body_to_elements_file(http_res: httpx.Response) -> str:
8383
return out.name
8484

8585

86+
async def _json_body_to_elements_file_async(http_res: httpx.Response) -> str:
87+
"""Run the conversion off the event loop, cleaning up if the caller gives up.
88+
89+
The work is offloaded so it does not block the loop, but a thread cannot be cancelled:
90+
on cancellation the conversion still runs to completion and creates its file, and a
91+
plain `await asyncio.to_thread(...)` discards the returned path, so nothing is left
92+
that could delete it. Shielding keeps a handle on the thread's result so the finished
93+
file can be removed once the caller has stopped waiting for it.
94+
"""
95+
task = asyncio.ensure_future(asyncio.to_thread(_json_body_to_elements_file, http_res))
96+
try:
97+
return await asyncio.shield(task)
98+
except BaseException:
99+
task.add_done_callback(_discard_abandoned_elements_file)
100+
raise
101+
102+
103+
def _discard_abandoned_elements_file(task: "asyncio.Future[str]") -> None:
104+
"""Delete the file a conversion produced after its caller stopped waiting."""
105+
if task.cancelled():
106+
return
107+
if task.exception() is not None:
108+
# The conversion raised, and it already removed its own partial file.
109+
return
110+
_discard_elements_file(task.result())
111+
112+
86113
def _discard_elements_file(path: str) -> None:
87114
"""Remove a partially written elements file, ignoring a failure to do so.
88115
@@ -383,9 +410,7 @@ async def partition_async(
383410
if utils.match_response(http_res, "200", "application/json"):
384411
if _ndjson_requested(req):
385412
return operations.PartitionResponse(
386-
elements_file=await asyncio.to_thread(
387-
_json_body_to_elements_file, http_res
388-
),
413+
elements_file=await _json_body_to_elements_file_async(http_res),
389414
status_code=http_res.status_code,
390415
content_type=http_res.headers.get("Content-Type") or "",
391416
raw_response=http_res,

0 commit comments

Comments
 (0)