diff --git a/app/api/routes/ai.py b/app/api/routes/ai.py index 559a41f..e4a1e4e 100644 --- a/app/api/routes/ai.py +++ b/app/api/routes/ai.py @@ -135,6 +135,7 @@ async def _read_validated_bytes(file: UploadFile, quota) -> bytes: """Read the upload and apply the shared upload-safety plumbing (size cap, magic-byte deny-list) — same guards as the converters.""" data = await file.read() + await file.close() # the framework's own copy (on disk above 1 MB) goes now if len(data) > quota.max_file_size_bytes: limit_mb = quota.max_file_size_bytes // _MB raise HTTPException( diff --git a/app/api/routes/compress.py b/app/api/routes/compress.py index c2c78f0..733678f 100644 --- a/app/api/routes/compress.py +++ b/app/api/routes/compress.py @@ -163,6 +163,9 @@ async def _do_compress( with input_path.open("wb") as f: shutil.copyfileobj(file.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the + # response. + await file.close() input_size_bytes = input_path.stat().st_size # A-1: Magic-byte validation @@ -235,6 +238,9 @@ async def _do_compress( else: output_disk_size = output_path.stat().st_size + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + # S1-B: Output-cap check — defensive guard; compression should shrink # files, but a pathological input (already-optimized PNG, very-low-bitrate # video) may not gain anything, and we still want an upper bound. @@ -476,6 +482,7 @@ async def _do_compress_batch( with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + await upload.close() # the framework's own copy goes now with open(input_path, "rb") as f: header = f.read(16) diff --git a/app/api/routes/convert.py b/app/api/routes/convert.py index 4aa79f8..501bbce 100644 --- a/app/api/routes/convert.py +++ b/app/api/routes/convert.py @@ -158,6 +158,9 @@ async def _do_convert( with input_path.open("wb") as f: shutil.copyfileobj(file.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the + # response. + await file.close() input_size_bytes = input_path.stat().st_size # A-1: Magic-byte validation @@ -231,6 +234,11 @@ async def _do_convert( else: output_disk_size = output_path.stat().st_size + # Only the result is needed to answer; the upload goes now. PDF → PDF + # writes its result over the upload, so there the one file stays. + if input_path != output_path: + input_path.unlink(missing_ok=True) + # S1-B: Output-cap check — reject before loading into memory. Guards # against bandwidth amplification (JPG→PNG ~5-10×, MP3→WAV ~11×). if output_disk_size is None or output_disk_size > quota.output_cap_bytes: @@ -489,6 +497,7 @@ async def _do_convert_batch( with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + await upload.close() # the framework's own copy goes now with open(input_path, "rb") as f: header = f.read(16) diff --git a/app/api/routes/pdf_pages.py b/app/api/routes/pdf_pages.py index c8b2c2b..4c75da5 100644 --- a/app/api/routes/pdf_pages.py +++ b/app/api/routes/pdf_pages.py @@ -92,6 +92,8 @@ def _write_upload_and_check_magic( """ with input_path.open("wb") as f: shutil.copyfileobj(upload.file, f) + # The framework's own copy (on disk above 1 MB) goes now, not after the response. + upload.file.close() with open(input_path, "rb") as f: header = f.read(16) if any(header.startswith(sig) for sig in BLOCKED_MAGIC): @@ -187,6 +189,9 @@ async def _do_extract( detail="Page extraction failed. Verify your file is a valid PDF.", ) + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + output_disk_size = output_path.stat().st_size _enforce_output_cap(output_disk_size, quota, tier) @@ -215,9 +220,9 @@ async def _do_extract( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_extract") record_conversion("pdf_extract", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -335,9 +340,9 @@ async def _do_split( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_split") record_conversion("pdf_split", "pdf", "zip", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -496,6 +501,9 @@ async def _do_compress( headers={"X-FileMorph-Error-Code": "invalid_pdf"}, ) + # Only the result is needed to answer; the upload goes now. + input_path.unlink(missing_ok=True) + output_disk_size = output_path.stat().st_size # The engine targets ``target_bytes`` (already ≤ cap), but a # text-heavy PDF can legitimately come out larger than the target @@ -536,9 +544,9 @@ async def _do_compress( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_compress") record_conversion("pdf_compress", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) @@ -796,9 +804,9 @@ async def _do_insert( duration_ms=duration_ms, ) except HTTPException: + shutil.rmtree(tmp_dir, ignore_errors=True) await metric_increment("failures.pdf_insert") record_conversion("pdf_insert", "pdf", "pdf", "failure") - shutil.rmtree(tmp_dir, ignore_errors=True) raise except BaseException: shutil.rmtree(tmp_dir, ignore_errors=True) diff --git a/app/core/responses.py b/app/core/responses.py index 2d3b82b..fb1e9da 100644 --- a/app/core/responses.py +++ b/app/core/responses.py @@ -14,12 +14,14 @@ class TempDirFileResponse(FileResponse): - """Send ``path``, then remove ``tmp_dir`` — the request's temp dir that - holds the result and the upload — however the response ends. + """Send ``path``, then remove ``tmp_dir`` — the request's temp dir, which + by now holds only the result (the routes delete the upload once the + result exists) — however the response ends. A ``background`` task runs only after a complete send: a send that fails (a server reporting the client gone) or is cancelled (a middleware or a - shutdown stopping the response) would leave both on disk until the sweep. + shutdown stopping the response) would leave the result on disk until the + sweep. The removal sits in a ``finally`` instead, shielded so that a cancelled response still finishes it. """ diff --git a/changelog.d/2026-10-29-upload-deleted-early.md b/changelog.d/2026-10-29-upload-deleted-early.md new file mode 100644 index 0000000..a32119f --- /dev/null +++ b/changelog.d/2026-10-29-upload-deleted-early.md @@ -0,0 +1,10 @@ +### Changed — the upload is deleted as soon as the result exists + +Single-file conversions and compressions, page extraction and PDF compression +now delete the uploaded file — the copy in the request's temporary folder and +the web framework's own — as soon as the result is written, instead of +together with the result when the response ends. Inserting pages already +deleted its own copies early; it, splitting and redaction now release the +framework's copy early too, and a PDF → PDF conversion writes its result over +the upload. Batch requests release the framework's copy of each file as soon +as it is copied. diff --git a/docs/architecture.md b/docs/architecture.md index 2f4e282..b3cc987 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -109,10 +109,12 @@ Every conversion or compression request follows the same path: written (PDF compression: 1 GiB), with their temporary files in the request's own temp folder. 7. **Output stream + temp cleanup.** Converted bytes are returned via a - streaming response. The request's UUID-named temp dir — the upload, the - result and LibreOffice's scratch files — is removed when the response - ends, sent in full or cut off (`app/core/responses.py`), or in the - request's `finally` block when the conversion fails. Ghostscript's PDF/A + streaming response. The upload is deleted as soon as the result exists, + together with the web framework's own copy of it (PDF → PDF writes its + result over the upload). The request's UUID-named temp dir + — the result and LibreOffice's scratch files — is removed when the + response ends, sent in full or cut off (`app/core/responses.py`), or in + the request's `finally` block when the conversion fails. Ghostscript's PDF/A run keeps its scratch files in an `fm_gs_*` dir of its own, removed after the run. A sweep on app start and every ten minutes deletes any `fm_*` temp dir older than ten minutes (a worker killed mid-request). diff --git a/docs/security-overview.md b/docs/security-overview.md index 6128d74..df8d2df 100644 --- a/docs/security-overview.md +++ b/docs/security-overview.md @@ -561,10 +561,12 @@ mechanically. to disk under the original filename. - Where a temp path is needed for a multi-step pipeline, the path uses a `fm_`-prefixed directory and a UUID stem. -- Cleanup removes the request's temp dir when the response ends — sent - in full or cut off — and in the request's `finally` block when the - conversion fails. LibreOffice and Ghostscript keep their own temporary - files in that dir, or in an `fm_`-prefixed one of their own. +- The upload is deleted as soon as the result exists, together with the + web framework's own copy of it. Cleanup removes the request's temp + dir, with the result, when the response ends — sent in full or cut + off — and in the request's `finally` block when the conversion fails. + LibreOffice and Ghostscript keep their own temporary files in that + dir, or in an `fm_`-prefixed one of their own. - A startup sweep, plus a periodic background sweep (`TEMP_SWEEP_INTERVAL_MINUTES`, default 10 min), removes any `fm_`-prefixed directory older than `TEMP_SWEEP_MAX_AGE_MINUTES` diff --git a/tests/test_temp_cleanup.py b/tests/test_temp_cleanup.py index bf97e1c..618c6a2 100644 --- a/tests/test_temp_cleanup.py +++ b/tests/test_temp_cleanup.py @@ -15,9 +15,11 @@ import pytest from PIL import Image from pypdf import PdfWriter +from starlette import formparsers from app.api.routes import compress as compress_route from app.api.routes import convert as convert_route +from app.api.routes import pdf_pages as pdf_route from app.core import quotas as quotas_module from app.core.audit import AuditWriteError from app.core.quotas import QUOTAS @@ -150,9 +152,105 @@ async def audit_down(*args, **kwargs): marks=_NEEDS_PIKEPDF, ), pytest.param("/api/v1/pdf/insert", {"file": "pdf", "insert_file": "pdf"}, {}, id="pdf-insert"), + # The one conversion whose result has the upload's name: written over it. + pytest.param( + "/api/v1/convert", {"file": "pdf"}, {"target_format": "pdf"}, id="convert-pdf-to-pdf" + ), ] +@pytest.fixture +def spools(monkeypatch): + """Every copy of an upload part the framework makes (a SpooledTemporaryFile, + on disk above 1 MB).""" + made: list = [] + + class RecordingSpool(formparsers.SpooledTemporaryFile): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + made.append(self) + + monkeypatch.setattr(formparsers, "SpooledTemporaryFile", RecordingSpool) + return made + + +@pytest.fixture +def answer_state(monkeypatch, spools): + """What is on disk when a file route answers: the result's name, the + names in the request dir, and (rolled to disk, closed) for each of the + framework's copies of the uploads.""" + seen: list = [] + + class Recording(TempDirFileResponse): + def __init__(self, path, *args, tmp_dir, **kwargs): + names = sorted(p.name for p in Path(tmp_dir).iterdir()) + copies = [(spool._rolled, spool.closed) for spool in spools] + seen.append((Path(path).name, names, copies)) + super().__init__(path, *args, tmp_dir=tmp_dir, **kwargs) + + for route in (convert_route, compress_route, pdf_route): + monkeypatch.setattr(route, "TempDirFileResponse", Recording) + return seen + + +@pytest.mark.parametrize(("endpoint", "uploads", "data"), _FILE_ROUTES) +def test_only_the_result_is_left_when_the_response_starts( + client, auth_headers, answer_state, endpoint, uploads, data +): + """The upload goes as soon as the result exists, not when the download + ends: the route's copy in the request dir, and the framework's own.""" + r = client.post( + endpoint, + headers=auth_headers, + files={field: _UPLOADS[kind]() for field, kind in uploads.items()}, + data=data, + ) + assert r.status_code == 200, r.text + [(result, names, copies)] = answer_state + assert names == [result] + assert copies and all(closed for _rolled, closed in copies), copies + + +def test_a_large_upload_on_disk_is_closed_before_the_response(client, auth_headers, answer_state): + """Above 1 MB the framework keeps its copy on disk, not in memory.""" + big = _jpg_bytes() + bytes(1_200_000) # past the JPEG's end: ignored by the decoder + r = client.post( + "/api/v1/convert", + headers=auth_headers, + files={"file": ("big.jpg", big, "image/jpeg")}, + data={"target_format": "png"}, + ) + assert r.status_code == 200, r.text + [(_result, _names, copies)] = answer_state + assert copies == [(True, True)] + + +@pytest.mark.parametrize( + ("route", "endpoint", "data"), + [ + (convert_route, "/api/v1/convert/batch", {"target_formats": ["png"]}), + (compress_route, "/api/v1/compress/batch", {"quality": "85"}), + ], + ids=["convert", "compress"], +) +def test_a_batch_closes_each_upload_before_its_zip( + client, auth_headers, monkeypatch, spools, route, endpoint, data +): + closed_at_zip: list[bool] = [] + real = route.build_batch_zip + + def spy(*args, **kwargs): + closed_at_zip.extend(spool.closed for spool in spools) + return real(*args, **kwargs) + + monkeypatch.setattr(route, "build_batch_zip", spy) + # One file: the test client's tier takes one per batch. + files = [("files", ("a.jpg", _jpg_bytes(), "image/jpeg"))] + r = client.post(endpoint, headers=auth_headers, files=files, data=data) + assert r.status_code == 200, r.text + assert closed_at_zip == [True] + + @pytest.mark.parametrize(("endpoint", "uploads", "data"), _FILE_ROUTES) def test_a_download_cut_off_by_the_server_leaves_no_temp_dir( client, auth_headers, endpoint, uploads, data