rbrooks/Circa

Watch

Fork 0

Storage read interface returns a Path and cannot support S3 #90

New issue

Closed

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

claude-bot commented

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

Severity: MEDIUM (cheap now, breaking later)

The problem

backend/app/services/storage.py:27-28 defines the read side of the storage abstraction as:

def get_path(self, key: str) -> Path

The contract is "return a filesystem path the caller may read directly," consumed by
FileResponse at photos.py:177,190. An S3 backend (#41) cannot honour that without
downloading every object to a temp file first.

So the Phase 4 exit criterion — "swapping storage backends requires no core application logic
change" — fails at the interface, not the implementation.

By contrast the auth abstraction is already effectively generic (OIDC discovery URL), and the AI
backend does not exist yet, so #3's protocol plan is unconstrained.

Fix

Change the read contract now, while there is one implementation and two call sites:

def open(self, key: str) -> BinaryIO: ...
def public_url(self, key: str) -> str | None: ...   # presigned redirect, optional

Serve via StreamingResponse, falling back to a redirect when public_url returns a value.

Cheap today. Expensive once #41 is in flight and every media path assumes a local file.

Done when

The storage ABC exposes a stream-based read contract
Media serving works through it with no filesystem assumption in the route
Range requests and the caching headers still work
LocalStorage is the only implementation but #41 has nothing to renegotiate

References

backend/app/services/storage.py:27-28
backend/app/api/routes/photos.py:177,190

Blocks: #41 (S3 storage backend).

## Severity: MEDIUM (cheap now, breaking later) ## The problem `backend/app/services/storage.py:27-28` defines the read side of the storage abstraction as: ```python def get_path(self, key: str) -> Path ``` The contract is "return a filesystem path the caller may read directly," consumed by `FileResponse` at `photos.py:177,190`. An S3 backend (#41) **cannot honour that** without downloading every object to a temp file first. So the Phase 4 exit criterion — "swapping storage backends requires no core application logic change" — fails at the *interface*, not the implementation. By contrast the auth abstraction is already effectively generic (OIDC discovery URL), and the AI backend does not exist yet, so #3's protocol plan is unconstrained. ## Fix Change the read contract now, while there is one implementation and two call sites: ```python def open(self, key: str) -> BinaryIO: ... def public_url(self, key: str) -> str | None: ... # presigned redirect, optional ``` Serve via `StreamingResponse`, falling back to a redirect when `public_url` returns a value. Cheap today. Expensive once #41 is in flight and every media path assumes a local file. ## Done when - [ ] The storage ABC exposes a stream-based read contract - [ ] Media serving works through it with no filesystem assumption in the route - [ ] Range requests and the caching headers still work - [ ] `LocalStorage` is the only implementation but #41 has nothing to renegotiate ## References - `backend/app/services/storage.py:27-28` - `backend/app/api/routes/photos.py:177,190` Blocks: #41 (S3 storage backend).

claude-bot added this to the v0.2.0 milestone

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

claude-bot added the

enhancement

backend

labels

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

claude-bot referenced this issue

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

Assorted spec items: quality flagging, search, rebuild job, storage ops #119

claude-bot referenced this issue

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

Implement the S3 storage backend #41

rbrooks referenced this issue from a commit

2026-08-03 17:15:58 +00:00

Take the filesystem out of the storage read contract

rbrooks closed this issue

2026-08-03 17:15:58 +00:00

claude-bot commented

2026-08-03 17:16:27 +00:00

Author

Done in d5b796b. backend/tests/test_storage_interface.py, 19 tests.

Four methods, not one

The issue proposes open() + public_url(). That is most of it, but two consumers make it four — and the reason is the same reason get_path went wrong in the first place: the callers want different things and collapsing them hides a cost.


`open(key) -> BinaryIO`	a seekable stream, for serving bytes
`size(key) -> int`	a length for `Content-Length`, without reading anything — a metadata call on a remote backend, not a download
`local_path(key)`	a context-managed real file
`public_url(key)`	a presigned redirect, or `None`

local_path is the one the issue does not mention, and it is unavoidable. The image sandbox (#65) and tesseract (#4) are subprocesses. A subprocess cannot be handed a Python file object, and no interface can wish that away. What the context manager buys is that the cost is visible: a remote backend downloads and deletes inside the block, a local one yields the stored file for free, and a call site that only needs bytes cannot silently acquire a download by reaching for the wrong method.

public_url is deliberately not abstract. "Cannot" is the right answer for a local disk — and it must be, because handing out a filesystem path is not something this may ever do. Forcing every backend to write return None teaches nothing.

Range requests

FileResponse answered these; StreamingResponse does not. Dropping them in exchange for an abstraction would be a silent regression, and a client assembling a file from ranges would get the whole body back for each one and quietly produce nonsense. So the route does it:

single ranges (bytes=5-9), open-ended (bytes=30-), and suffix (bytes=-8 = the last eight bytes, not the first — getting that backwards produces a plausible response that is simply wrong)
clamped past the end
416 for a start past the end, not the whole body
an unparseable Range ignored, per RFC 9110
Accept-Ranges: bytes advertised

Multi-range is answered with the whole body, which is permitted and much simpler than assembling a multipart response for a case nothing here produces.

Tested against a backend with no filesystem

InMemoryStorage implements the contract and holds its objects in a dict. That is the only way to prove the route makes no filesystem assumption — LocalStorage would satisfy a route that still called get_path. Media serving, caching headers, 304s, ranges, 404s and the presigned redirect are all exercised through it.

The redirect path is asserted with authorization first: a presigned URL is a credential, and handing one to a caller who may not read the photograph would be worse than serving the bytes, because the URL outlives the request and can be passed on.

The OCR handler is exercised through the worker against the same in-memory backend, so "the worker stops assuming a local disk" is checked rather than asserted from reading the source.

Done when

The storage ABC exposes a stream-based read contract
Media serving works through it with no filesystem assumption in the route
Range requests and the caching headers still work — both asserted, including on the 304
LocalStorage is the only implementation but #41 has nothing to renegotiate — test_get_path_is_gone says so explicitly, because a get_path left in place "for compatibility" is a contract a future backend still has to honour

1098 passed, 8 skipped; 7 e2e; ruff clean; no OpenAPI change.

Done in d5b796b. `backend/tests/test_storage_interface.py`, 19 tests. ## Four methods, not one The issue proposes `open()` + `public_url()`. That is most of it, but two consumers make it four — and the reason is the same reason `get_path` went wrong in the first place: the callers want different things and collapsing them hides a cost. | | | |---|---| | `open(key) -> BinaryIO` | a seekable stream, for serving bytes | | `size(key) -> int` | a length for `Content-Length`, without reading anything — a metadata call on a remote backend, not a download | | `local_path(key)` | a context-managed **real file** | | `public_url(key)` | a presigned redirect, or `None` | **`local_path` is the one the issue does not mention, and it is unavoidable.** The image sandbox (#65) and tesseract (#4) are *subprocesses*. A subprocess cannot be handed a Python file object, and no interface can wish that away. What the context manager buys is that the cost is **visible**: a remote backend downloads and deletes inside the block, a local one yields the stored file for free, and a call site that only needs bytes cannot silently acquire a download by reaching for the wrong method. **`public_url` is deliberately not abstract.** "Cannot" is the right answer for a local disk — and it must be, because handing out a filesystem path is not something this may ever do. Forcing every backend to write `return None` teaches nothing. ## Range requests `FileResponse` answered these; `StreamingResponse` does not. Dropping them in exchange for an abstraction would be a silent regression, and a client assembling a file from ranges would get the whole body back for each one and quietly produce nonsense. So the route does it: - single ranges (`bytes=5-9`), open-ended (`bytes=30-`), and **suffix** (`bytes=-8` = the *last* eight bytes, not the first — getting that backwards produces a plausible response that is simply wrong) - clamped past the end - **416 for a start past the end**, not the whole body - an unparseable `Range` ignored, per RFC 9110 - `Accept-Ranges: bytes` advertised Multi-range is answered with the whole body, which is permitted and much simpler than assembling a multipart response for a case nothing here produces. ## Tested against a backend with no filesystem `InMemoryStorage` implements the contract and holds its objects in a dict. That is the only way to prove the *route* makes no filesystem assumption — `LocalStorage` would satisfy a route that still called `get_path`. Media serving, caching headers, 304s, ranges, 404s and the presigned redirect are all exercised through it. The redirect path is asserted **with authorization first**: a presigned URL is a credential, and handing one to a caller who may not read the photograph would be worse than serving the bytes, because the URL outlives the request and can be passed on. The OCR handler is exercised through the worker against the same in-memory backend, so "the worker stops assuming a local disk" is checked rather than asserted from reading the source. ## Done when - [x] The storage ABC exposes a stream-based read contract - [x] Media serving works through it with no filesystem assumption in the route - [x] Range requests and the caching headers still work — both asserted, including on the 304 - [x] `LocalStorage` is the only implementation but #41 has nothing to renegotiate — `test_get_path_is_gone` says so explicitly, because a `get_path` left in place "for compatibility" is a contract a future backend still has to honour **1098 passed, 8 skipped**; 7 e2e; ruff clean; no OpenAPI change.

claude-bot referenced this issue

2026-08-04 06:14:16 +00:00

A cancelled thumbnail request is logged as an ERROR with a traceback #139

rbrooks referenced this issue from a commit

2026-08-04 20:21:21 +00:00

Stop shouting about a browser that changed its mind

claude-bot referenced this issue

2026-08-04 20:35:48 +00:00

A cancelled thumbnail request is logged as an ERROR with a traceback #139