rbrooks/Circa

Watch

Fork 0

Ingest blocks the event loop and buffers whole uploads in RAM #86

New issue

Closed

opened 2026-07-28 06:00:30 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 06:00:30 +00:00

Severity: HIGH

The bug

ingest_endpoint (backend/app/api/routes/ingest.py:44-80) is the only async def data
handler in the codebase — and it is exactly the one that must not be. Inside it, all of the
following run directly on the event loop:

sha256_file (full-file read)
extract_exif (Pillow)
shutil.copy2, twice (temp → storage)
Path.write_bytes
every synchronous SQLAlchemy call

Every other route is a sync def, which FastAPI correctly runs in a threadpool.

Additionally await front.read() loads the entire upload into memory before the size check,
so the data path is network → RAM → temp file → re-read for hashing → copy to storage: three disk
passes plus a full RAM copy, with spikes up to 2×100 MB per in-flight request.

Impact

During bulk ingest of tens of thousands of scans, every concurrent request — media serving,
review actions, everything — stalls behind each photo's hash and copy, because the event loop is
blocked. This directly violates #48's "no background job blocks interactive review."

Fix

Change the handler to a sync def. FastAPI threadpools it, and front.file is a
SpooledTemporaryFile readable synchronously. This one keyword removes the event-loop hazard
and is worth doing on its own.
Stream to the temp file in chunks while updating the SHA-256 incrementally — removes a full read
pass and the RAM buffer.
Enforce MAX_FILE_SIZE during streaming, not after. Today an oversized upload consumes memory
before the 413.

Done when

No blocking work runs on the event loop during ingest
Uploads stream rather than buffering whole files in memory
The size limit is enforced before the full body is read
Interactive requests stay responsive during a bulk ingest

References

backend/app/api/routes/ingest.py:44-80
backend/app/services/ingest.py:64-115
backend/app/services/storage.py:48-52,79-84

Coordinate with the filename sanitization fix in v0.1.1 — same handler.

## Severity: HIGH ## The bug `ingest_endpoint` (`backend/app/api/routes/ingest.py:44-80`) is the **only** `async def` data handler in the codebase — and it is exactly the one that must not be. Inside it, all of the following run directly on the event loop: - `sha256_file` (full-file read) - `extract_exif` (Pillow) - `shutil.copy2`, twice (temp → storage) - `Path.write_bytes` - every synchronous SQLAlchemy call Every other route is a sync `def`, which FastAPI correctly runs in a threadpool. Additionally `await front.read()` loads the **entire** upload into memory *before* the size check, so the data path is network → RAM → temp file → re-read for hashing → copy to storage: three disk passes plus a full RAM copy, with spikes up to 2×100 MB per in-flight request. ## Impact During bulk ingest of tens of thousands of scans, every concurrent request — media serving, review actions, everything — stalls behind each photo's hash and copy, because the event loop is blocked. This directly violates #48's "no background job blocks interactive review." ## Fix - Change the handler to a sync `def`. FastAPI threadpools it, and `front.file` is a `SpooledTemporaryFile` readable synchronously. **This one keyword removes the event-loop hazard** and is worth doing on its own. - Stream to the temp file in chunks while updating the SHA-256 incrementally — removes a full read pass and the RAM buffer. - Enforce `MAX_FILE_SIZE` *during* streaming, not after. Today an oversized upload consumes memory before the 413. ## Done when - [ ] No blocking work runs on the event loop during ingest - [ ] Uploads stream rather than buffering whole files in memory - [ ] The size limit is enforced before the full body is read - [ ] Interactive requests stay responsive during a bulk ingest ## References - `backend/app/api/routes/ingest.py:44-80` - `backend/app/services/ingest.py:64-115` - `backend/app/services/storage.py:48-52,79-84` Coordinate with the filename sanitization fix in v0.1.1 — same handler.

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:30 +00:00

claude-bot added the

backend

enhancement

labels

2026-07-28 06:00:30 +00:00

claude-bot referenced this issue

2026-07-28 06:07:50 +00:00

Performance optimization and progress tracking at collection scale #48

claude-bot referenced this issue

2026-07-28 17:59:09 +00:00

Arbitrary file write via unsanitized upload filename (RCE) #55

claude-bot referenced this issue

2026-08-01 20:20:11 +00:00

SQLite configuration is not ready for a concurrent worker #87

rbrooks referenced this issue from a commit

2026-08-03 16:33:32 +00:00

Take ingest off the event loop and stop buffering uploads

rbrooks closed this issue

2026-08-03 16:33:32 +00:00

claude-bot commented

2026-08-03 16:33:56 +00:00

Author

Done in d689da8. backend/tests/test_ingest_streaming.py, 15 tests.

The one keyword

ingest_endpoint is now a plain def. That alone removes the event-loop hazard, exactly as the issue says — FastAPI threadpools sync handlers, and upload.file is a SpooledTemporaryFile that reads synchronously.

Streaming, with the hash and the limit folded in

One pass now does three things that used to be three passes:

writes in 1 MiB chunks instead of materialising the whole body as one bytes object
accumulates the SHA-256 while writing, so ingest_photo no longer re-reads the entire file to compute a digest that was available for free
enforces MAX_FILE_SIZE as the bytes arrive, abandoning the request at the first chunk that crosses the line

That last one is the substantive part of the fix. The limit was applied to len(data) after the read — the 413 arrived having already spent exactly the memory it exists to protect. The partial file is also unlinked at the point of refusal rather than left for the temp-directory teardown, so a refused upload leaves no attacker-controlled bytes on disk even briefly.

ingest_photo takes the digest as an optional argument rather than requiring it, because the other callers (the CLI, a future watch-directory worker) have a path and nothing else — and a caller passing the wrong digest would silently deduplicate against the wrong photograph, so it is only ever supplied by code that hashed the same bytes it wrote.

Three more handlers were doing the same thing

logout, /api/auth/me, and both dev-login routes were async def with no await in them at all — the worst of both: no concurrency gained, and their SQLAlchemy calls running on the loop. Now sync. Only login and callback remain async, and they genuinely await an HTTP round trip to the identity provider, which is the work an event loop is for.

The rule is asserted, not remembered

test_no_data_route_is_async walks the whole route table and fails on any async handler outside those two, named individually rather than by an /api/auth/ prefix — a prefix exemption would have gone on saying the three above were fine.

The failure here was never that somebody chose wrongly; it was that nothing said which way is right.

Verification

Chunked reads observed directly (bounded sizes, more than one call), rather than by measuring memory, which is neither reliable nor portable
Stored bytes are byte-identical to what was sent — the thing chunking is most likely to break, and the worst thing to get wrong, since an original is irreplaceable and a truncated copy reports nothing
The digest matches the payload; duplicate detection still works through the streamed path
Refusal counts the reads: ≤6 chunks of 256 bytes against a 1 KiB limit, where the old code would have read all ~20 KiB
A file at exactly the limit is accepted; the back scan is bounded too
The checks that used to bracket the read still run in front of it: declared non-image → 415, hostile filename → 415 (#55's fix is on the same handler), viewer/reviewer → 403

Verified load-bearing: restoring async def fails both event-loop tests.

Done when

No blocking work runs on the event loop during ingest
Uploads stream rather than buffering whole files in memory
The size limit is enforced before the full body is read
Interactive requests stay responsive during a bulk ingest — a property of the handler being threadpooled; the assertion that holds it up is the route-table rule above, since a timing test would measure the machine

1059 passed, 8 skipped; 7 e2e; ruff clean.

Done in d689da8. `backend/tests/test_ingest_streaming.py`, 15 tests. ## The one keyword `ingest_endpoint` is now a plain `def`. That alone removes the event-loop hazard, exactly as the issue says — FastAPI threadpools sync handlers, and `upload.file` is a `SpooledTemporaryFile` that reads synchronously. ## Streaming, with the hash and the limit folded in One pass now does three things that used to be three passes: - **writes in 1 MiB chunks** instead of materialising the whole body as one `bytes` object - **accumulates the SHA-256 while writing**, so `ingest_photo` no longer re-reads the entire file to compute a digest that was available for free - **enforces `MAX_FILE_SIZE` as the bytes arrive**, abandoning the request at the first chunk that crosses the line That last one is the substantive part of the fix. The limit was applied to `len(data)` *after* the read — the 413 arrived having already spent exactly the memory it exists to protect. The partial file is also unlinked at the point of refusal rather than left for the temp-directory teardown, so a refused upload leaves no attacker-controlled bytes on disk even briefly. `ingest_photo` takes the digest as an optional argument rather than requiring it, because the other callers (the CLI, a future watch-directory worker) have a path and nothing else — and a caller passing the *wrong* digest would silently deduplicate against the wrong photograph, so it is only ever supplied by code that hashed the same bytes it wrote. ## Three more handlers were doing the same thing `logout`, `/api/auth/me`, and both dev-login routes were `async def` **with no `await` in them at all** — the worst of both: no concurrency gained, and their SQLAlchemy calls running on the loop. Now sync. Only `login` and `callback` remain async, and they genuinely await an HTTP round trip to the identity provider, which is the work an event loop is for. ## The rule is asserted, not remembered `test_no_data_route_is_async` walks the whole route table and fails on any async handler outside those two, named individually rather than by an `/api/auth/` prefix — a prefix exemption would have gone on saying the three above were fine. The failure here was never that somebody chose wrongly; it was that nothing said which way is right. ## Verification - Chunked reads observed directly (bounded sizes, more than one call), rather than by measuring memory, which is neither reliable nor portable - **Stored bytes are byte-identical to what was sent** — the thing chunking is most likely to break, and the worst thing to get wrong, since an original is irreplaceable and a truncated copy reports nothing - The digest matches the payload; duplicate detection still works through the streamed path - Refusal counts the reads: ≤6 chunks of 256 bytes against a 1 KiB limit, where the old code would have read all ~20 KiB - A file at exactly the limit is accepted; the back scan is bounded too - The checks that used to bracket the read still run in front of it: declared non-image → 415, hostile filename → 415 (#55's fix is on the same handler), viewer/reviewer → 403 Verified load-bearing: restoring `async def` fails both event-loop tests. ## Done when - [x] No blocking work runs on the event loop during ingest - [x] Uploads stream rather than buffering whole files in memory - [x] The size limit is enforced before the full body is read - [x] Interactive requests stay responsive during a bulk ingest — a property of the handler being threadpooled; the assertion that holds it up is the route-table rule above, since a timing test would measure the machine **1059 passed, 8 skipped**; 7 e2e; ruff clean.

claude-bot referenced this issue

2026-08-03 19:16:08 +00:00

Generate thumbnails and display derivatives at ingest #93