fix(api): stop synchronous route work from stalling the whole server - #9436
fix(api): stop synchronous route work from stalling the whole server#9436Pfannkuchensack wants to merge 10 commits into
Conversation
The gallery list/name routes and the auth dependencies were declared `async def` while calling synchronous SQLite services, so their database work ran on the event loop. For its whole duration the process served no other request and delivered no socket.io event, which users experienced as the backend freezing mid-generation rather than as a slow gallery. Declaring them `def` hands them to Starlette's threadpool instead. Measured against a 200k-image, 1.7 GB database: the latency of an unrelated request issued while a gallery name query is in flight drops from 1662 ms to 4 ms (search) and from 2298 ms to 881 ms (no search). The queries themselves are unchanged; only the loop is freed. The residual 881 ms in the no-search case is response serialization of 202k items, which is tracked separately. Adds a regression test that stubs a blocking service call and asserts an unrelated route still answers during it, plus a contributor doc describing the rule.
…y ones The name list that drives the virtualized gallery wrapped every entry in an object carrying a `kind` discriminator. Building those models cost 820ms of the 2225ms service call on a 200k-item library, and every consumer threw the field away — `itemRefsToNames` mapped it off immediately and each caller re-derived the kind from the file extension via `isVideoName`. Adds `GET /v1/gallery/item_names`, returning a flat name list in the same shape as the image-only `ImageNamesResult`. An optional `created_date` filter subsumes the separate by-date virtual-board route, so regular boards and virtual dates now share one endpoint, one cache and one query-args selector instead of a skipToken branch duplicated across the grid hook, range selection and both auto-select listeners. Measured on a 200k-image, 1.7 GB database: 2.51s -> 1.57s per request, 8.48 MB -> 3.85 MB of response, and the residual event-loop stall from serializing the response drops from 466ms to 102ms at p95. Existing integrations still call the old routes, so all five legacy name endpoints keep working and are marked `deprecated=True` with a pointer to the replacement.
Starlette's GZipMiddleware compresses every response type except text/event-stream, so every image and video the gallery serves was being deflate-compressed a second time. Measured: a 1024x1024 PNG (3.00 MB) costs 52ms of event-loop time to gzip and comes back at 3.01 MB — larger than it went in; a 2048x2048 PNG costs 210ms for the same non-result. Compression runs on the event loop, so that time is a full stall of the process. With auto-switch enabled the UI fetches the full image after every generated image, so the cost lands repeatedly during a batch. Replaces it with a content-type-aware subclass that compresses an allowlist of text, JSON, XML and SVG responses and passes everything else through. The UI bundle and the API's JSON keep their compression unchanged. Lowering compresslevel is not an alternative for this case: on already-compressed input level 1 costs 51ms against level 9's 52ms, because deflate still scans the whole body. Making the level configurable is worthwhile for the *compressible* path and is tracked separately. Note for deployments: media responses no longer carry Content-Encoding: gzip.
The pin sat at 0.118.3 with a comment guessing the OpenAPI crash on 0.119 was "probably Invoke's [bug], because we are doing something unusual with AnyInvocation". It was not: fastapi/_compat/v2.py assumed every field mapping carries a `$ref` and raised KeyError otherwise. Upstream fixed it in 0.124.0 with no change needed here. Two later changes needed adapting to, both of which fail silently: - 0.130 emits `contentMediaType: application/octet-stream` instead of `format: binary` for file uploads. typegen.js mapped only the latter to `Blob`, so upload call sites would have started typing their `File` argument as `string`. It now maps both. - 0.141 keeps an included router as a single node in `app.routes` instead of copying its routes into it. The default-deny auth guard walked `app.routes` looking for APIRoute instances and found 2 of 197 — passing while inspecting almost nothing. It now walks `iter_route_contexts`, the traversal FastAPI's own OpenAPI generation uses, and asserts a floor on the route count so going blind fails loudly instead. Schema changes are limited to ValidationError gaining the optional `input`/`ctx` fields; upload fields still resolve to Blob. Starlette stays at 0.48.0.
Package A converted the eight gallery and search routes that caused the reported multi-minute stalls. The same defect was present across the rest of the API: 167 route handlers were declared `async def` while awaiting nothing, so their synchronous service calls ran on the event loop. Each one stalls the entire process for its duration - no other request served, no socket.io event delivered - which is why the symptom looked like the application freezing rather than one slow endpoint. Candidates were identified by AST rather than by hand: `async def` route handlers with no `await`, `async with` or `async for` anywhere in the body, cross-checked for references to asyncio, anyio or the loop. Two flagged candidates were false positives (both the word "loop" in a comment). The diff is 167 signature lines plus one signature that ruff collapsed onto a single line once `async ` was removed. Adds tests/app/routers/test_no_blocking_async_routes.py, which enforces the rule for every handler including ones written later - a per-route test cannot cover a route that does not exist yet, and this failure mode is invisible until a user has a large enough library to notice. Two tests that invoked route handlers directly were updated to call them as the plain functions they now are.
|
I was just bumbling around yesterday trying to figure out Invoke's approach to concurrency, as I realized the FastAPI handlers are async but most of our code (including the BaseInvocation API) very much is not.
Wait, what? This is just removing "async" from 167 existing "async def"? Is that…? Okay, the explanation for how this works is at FastAPI, Concurrency and async / await: FastAPI automatically kicks any non-async function to a thread pool. In an async-first application, "parse every router module and fail if any handler is async def without awaiting" is really not the heuristic you want to use. You want to be async by default and only introduce the complexity of thread switching if you're doing a blocking operation. Especially in Python, where threads don't actually get you multi-core parallel execution. The aforementioned FastAPI docs back me up on that:
However… in a codebase where most of the code is written synchronously and people aren't used to thinking about whether they're about to call a blocking function? As a former Twisted developer, it hurts me to even think it, but no-async-by-default might be the right call, I guess? We're not trying to optimize requests/second throughput, as the number of users and frequency of requests on any one InvokeAI sever is actually pretty low. We're trying to reduce the chances of someone accidentally making a commit that blocks the server's event loop. So. I can't say I'm a fan, but I guess I understand why you might want to do it that way. I still feel tempted to argue for some kind of "it doesn't use |
|
I guess the hazard of "kick it to the thread pool by default" is then all your code has to be thread-safe by default. Which I don't think is an easier/safer assumption to make than knowing if your code is blocking. i.e. does sending all sqlite-related activity to a general-purpose thread-pool mean we have multiple threads opening and writing to the same sqlite database at once? Is that a thing we can assume it's safe to do? |
Worth answering precisely, because the premise is slightly off in a way that matters. We never open a connection per thread. There is exactly one And this predates the PR. Multiple threads already write to that database on every generation: What does change, and I don't want to gloss over it: two On the general point, I'd argue the two assumptions aren't symmetric. "Does this handler block?" is |
Fair challenge, and the FastAPI docs quote is right — but note its condition: "unless your path Two things I could measure rather than argue: The thread hop costs ~156 µs. Trivial handler, in-process, 600 requests: 0.320 ms as The high-volume route you're hoping is a StaticFile isn't one. One clarification on the GIL point: for pure-Python CPU work you're right that threads buy nothing. But On the escape hatch — you're right that "doesn't await" ≠ "doesn't block", and the guard is deliberately |
Summary
Kind: fix + perf (backend, frontend, dependencies)
Users reported the backend becoming unresponsive for minutes at a time. The cause was not one bug but a chain, and reproducing it needed three conditions at once — a large library, an active gallery search, and a running generation — which is why it resisted diagnosis.
The mechanism. The gallery list and name routes were declared
async defwhile calling synchronous SQLite services. That work therefore ran on the event loop, so for its entire duration the process served no other HTTP request and delivered no socket.io event. Users experienced this as the whole application freezing mid-generation rather than as a slow gallery. The same defect was present in 167 further route handlers.Measured on a seeded 200k-image, 1.7 GB database (1.56 GB of it metadata blobs). The metric is the latency of an unrelated trivial request issued while a gallery name query is in flight — the queries themselves are not made faster, the loop is freed:
Sample counts tell the same story more plainly: in the two-second window the probe returned 1 response before and 28 after.
What each commit does:
fix(api): run gallery and search routes off the event loop— eight gallery/search routes and the five auth dependencies declareddefso FastAPI dispatches them to the threadpool.perf(gallery): add a flat item-names endpoint— the name list wrapped every entry in an object carrying akinddiscriminator. Building those models cost 820 ms of the 2225 ms service call at 200k items, and every consumer discarded the field, re-deriving the kind from the file extension. A newGET /v1/gallery/item_namesreturns a flat list; an optionalcreated_datefilter subsumes the separate by-date virtual-board route, so regular boards and virtual dates now share one endpoint, one cache and one query-args selector instead of askipTokenbranch duplicated in four places. Result: 2.51 s → 1.57 s, 8.48 MB → 3.85 MB. Five legacy name endpoints keep working and are markeddeprecated=True— external integrations still call them.perf(api): stop gzipping responses that are already compressed— Starlette'sGZipMiddlewarecompresses every content type excepttext/event-stream. A 3 MB PNG cost 52 ms of event-loop time to gzip and came back at 3.01 MB, larger than it went in; a 12 MB PNG cost 210 ms. With auto-switch on, that lands after every generated image. Loweringcompresslevelis not an alternative — on incompressible input level 1 costs 51 ms against level 9's 52 ms.feat(queue): add lightweight item summaries endpoint— from @JPPhoto'soptimize-queue-return-data, adopted with the route declareddef. The queue list fetched fullSessionQueueItemobjects carrying the completeGraphExecutionState: measured against a real 65-item queue database, 50 items cost 56 ms to parse and 9 ms to re-serialize — ~66 ms of event-loop time plus ~1.8 MB per request, for fields the list never renders. It also replaces an N+1 (one query per requested id, each taking the process-wide database lock).build: unpin FastAPI and move to 0.141.1— see Merge Plan.perf(api): run every synchronous route handler off the event loop— the remaining 167 handlers, identified by AST rather than by hand.Related Issues / Discussions
optimize-queue-return-databranch.gallery_default.py, which PR perf(db): use a covering index and anti-join for the gallery image names query #9385 also rewrites — see Merge Plan.QA Instructions
Automated. Two new guards, both verified to fail before the fix and pass after:
tests/app/routers/test_no_blocking_async_routes.pyparses every router module and fails if any handler isasync defwithout awaiting. Reintroduceasyncon any handler to see it name the offender.tests/app/routers/test_event_loop_blocking.pystubs a service call to block for one second and asserts an unrelated route still answers during it. Six routes covered, GET and POST.Plus
tests/app/routers/test_gallery_item_names.py(7 tests, two of which compare the new endpoint against the deprecated one so ordering and counts cannot drift while both are served) andtests/app/api/test_gzip_content_types.py(14 tests, including one asserting the real app has the middleware wired).Full run: 2157 backend tests, 1707 frontend tests, ruff / tsc / eslint / knip / dpdm / prettier clean. Nine pre-existing failures in
test_download_queue,test_model_installandtest_load_apiare network-dependent and reproduce identically on an unmodified tree.Manual, to see the effect. Needs a large library — a few hundred MB of image metadata is enough; the effect scales with
SUM(LENGTH(metadata)) FROM images.To measure rather than eyeball it: fire
GET /api/v1/gallery/item_names?search_term=…and pollGET /api/v1/app/versionconcurrently, recording the latency of the second. Note that a benchmark of the search endpoint alone shows no improvement — that is the wrong instrument here.Deprecated routes.
GET /v1/gallery/items/names,/v1/images/names,/v1/videos/namesand both/v1/virtual_boards/by_date/{date}/*_namesstill return their original shapes; only the OpenAPIdeprecatedflag changed.Media responses no longer carry
Content-Encoding: gzip. Confirm images and videos still load, including behind a reverse proxy.Merge Plan
The FastAPI bump needs attention.
pyproject.tomlmoves fromfastapi==0.118.3to>=0.141.1,<0.142; contributors must re-sync (uv sync) after pulling. The old pin carried a comment guessing the OpenAPI crash on 0.119 was "probably Invoke's [bug], because we are doing something unusual with AnyInvocation". It was not:fastapi/_compat/v2.pyassumed every field mapping carries a$ref. Upstream fixed it in 0.124.0 with no change needed here.Two later FastAPI changes break silently and are handled — both are worth a reviewer's attention:
contentMediaTypeinstead offormat: binaryfor file uploads.typegen.jsmapped only the latter toBlob, so upload call sites would have started typing theirFileargument asstring. Caught only becausetschappened to fail.app.routesinstead of copying its routes into it. The default-deny auth guard walkedapp.routesforAPIRouteinstances and found 2 of 197 — passing while inspecting almost nothing. It now walksiter_route_contexts(the traversal FastAPI's own OpenAPI generation uses) and asserts a floor on the route count so going blind fails loudly. Only the allowlist-staleness assertion caught this; "fixing" it by trimmingPUBLIC_ROUTESwould have killed the guard.Conflicts with PR #9385, which rewrites
_build_halfingallery_default.py. This PR adds a shared_query_name_rowsin the same file. Whichever merges second needs a manual pass. Unrelated note for that PR's own review: it introducesINDEXED BYhints into the shared query builder, which is SQLite-only syntax.Not in scope, deliberately: the
metadata LIKE '%…%'full scan (six sites, unindexable by construction) is being addressed differently in v7; the SQLite single-connection/global-lock design is tracked separately; makingcompresslevelconfigurable is written up with measurements but not implemented — level 9 costs 5.5× the CPU of level 1 for 0.4 percentage points of output size.Suggested split: if the FastAPI commit would rather be reviewed on its own, it is self-contained and the auth-guard finding deserves its own title.
Checklist
docs/contributing/blocking-work-in-api-routesWhat's Newcopy (if doing a release after this PR)