diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index c3fa0d022d..a6f2374515 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -261,7 +261,8 @@ def _link_staged_no_overwrite( place, for a caller that still holds a descriptor on the staged inode and therefore must remove it itself. Windows refuses to remove a file that has an open handle, so the removal cannot happen here in that case; the - caller retains ownership of the staged path on both success and failure. + caller owns the staged path from the moment this returns — including on + the failure arms below, which leave it for that caller's own cleanup. """ file_path = _pure_destination(uploads_dir, display_filename) try: @@ -294,9 +295,10 @@ def _commit_upload_temp_no_overwrite( Same no-overwrite contract as :func:`_link_staged_no_overwrite`: :class:`FileExistsError` leaves the staged part in place for a next-suffix retry (the handle's second ``close`` is idempotent); any - other failure removes it best-effort. With ``unlink_staged=False``, the - caller retains the staged name on success and failure until it releases - its descriptor on the inode. + other failure removes the staged name only when this call owns it. + ``unlink_staged=False`` leaves the name for the caller on every exit, + including the failure arms: that caller still holds a descriptor on the + inode and removes it itself (``_abort_upload_temp`` on its error path). """ upload_temp.handle.close() return _link_staged_no_overwrite(upload_temp.temp_path, uploads_dir, display_filename, unlink_staged=unlink_staged) diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index d729fe9911..096ce2de30 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -542,6 +542,22 @@ def test_link_staged_preserves_original_error_when_cleanup_fails(tmp_path): assert not (tmp_path / "report.pdf").exists() +def test_link_staged_failure_leaves_staged_file_for_caller_when_not_owning(tmp_path): + staged_path = tmp_path / "upload.part" + staged_path.write_bytes(b"upload bytes") + original_error = OSError(errno.EIO, "original link failure") + with ( + patch.object(os, "link", side_effect=original_error), + patch.object(uploads, "_remove_staged_file") as remove_mock, + pytest.raises(OSError) as exc_info, + ): + uploads._link_staged_no_overwrite(staged_path, tmp_path, "report.pdf", unlink_staged=False) + + assert exc_info.value is original_error + remove_mock.assert_not_called() + assert staged_path.read_bytes() == b"upload bytes" + + def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True)