From 7c0dfd74a5f6159ec4671eef47dec1b70c226e33 Mon Sep 17 00:00:00 2001 From: "psychedelicious@windows" <4822129+psychedelicious@users.noreply.github.com> Date: Sat, 13 Jul 2024 14:28:03 +1000 Subject: [PATCH] fix(api): deleting large images fails This issue is caused by a race condition. When a large image is served to the client, it is done using a streaming `FileResponse`. This concurrently serves the image straight from disk. The file is kept open by FastAPI until the image is fully served. When a user deletes an image before the file is done serving, the delete fails because the file is still held by FastAPI. To reproduce the issue: - Create a very large image (8k reliably creates the issue). - Create a smaller image, so that the first image in the gallery is not the large image. - Refresh the app. The small image should be selected. - Select the large image and immediately delete it. You have to be fast, to delete it before it finishes loading. - In the terminal, we expect to see an error saying `Failed to delete image file`, and the image does not disappear from the UI. - After a short wait, once the image has fully loaded, try deleting it again. We expect this to work. The workaround is to instead serve the image from memory. Loading the image to memory is very fast, so there is only a tiny window in which we could create the race condition, but it technically could still occur, because FastAPI is asynchronous and handles requests concurrently. Once we load the image into memory, deletions of that image will work. Then we return a normal `Response` object with the image bytes. This is essentially what `FileResponse` does - except it uses `anyio.open_file`, which is async. The tradeoff is that the server thread is blocked while opening the file. I think this is a fair tradeoff. A future enhancement could be to implement soft deletion of images (db is already set up for this), and then clean up deleted image files on startup/shutdown. We could move back to using the async `FileResponse` for best responsiveness in the server without any risk of race conditions. --- invokeai/app/api/routers/images.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index 8e3824ce93..2bc0b48251 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -233,21 +233,14 @@ async def get_image_workflow( ) async def get_image_full( image_name: str = Path(description="The name of full-resolution image file to get"), -) -> FileResponse: +) -> Response: """Gets a full-resolution image file""" try: path = ApiDependencies.invoker.services.images.get_path(image_name) - - if not ApiDependencies.invoker.services.images.validate_path(path): - raise HTTPException(status_code=404) - - response = FileResponse( - path, - media_type="image/png", - filename=image_name, - content_disposition_type="inline", - ) + with open(path, "rb") as f: + content = f.read() + response = Response(content, media_type="image/png") response.headers["Cache-Control"] = f"max-age={IMAGE_MAX_AGE}" return response except Exception: @@ -268,15 +261,14 @@ async def get_image_full( ) async def get_image_thumbnail( image_name: str = Path(description="The name of thumbnail image file to get"), -) -> FileResponse: +) -> Response: """Gets a thumbnail image file""" try: path = ApiDependencies.invoker.services.images.get_path(image_name, thumbnail=True) - if not ApiDependencies.invoker.services.images.validate_path(path): - raise HTTPException(status_code=404) - - response = FileResponse(path, media_type="image/webp", content_disposition_type="inline") + with open(path, "rb") as f: + content = f.read() + response = Response(content, media_type="image/webp") response.headers["Cache-Control"] = f"max-age={IMAGE_MAX_AGE}" return response except Exception: