mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-10-02 22:06:00 +01:00
splitsec2
b690832659
Credit where it is due: this defect was found and measured by **@DrNgo** in [DrNgo/shelfmark-fork@ae2185c](https://github.com/DrNgo/shelfmark-fork/commit/ae2185c8a83d537e55f1dc03f745ab844b5cdcdb). He recorded a 493 MB audiobook peaking near 963 MB and OOM-killing a 1 GiB container, with the proxy access log showing a single `GET /api/localdownload` returning 502 at the exact second of the kill. The analysis is his; I am sending it because it is still open here. Serving a completed book from the live queue calls `get_book_data`, which reads the whole file into bytes so the route can wrap it in a `BytesIO` for `send_file`. That is two copies of the book, with one resident for the length of the client transfer, to hand over a file that is already sitting on disk. `get_book_path` returns the path the task already holds and `send_file` streams it. The history fallback a few lines up in the same route has always worked this way, so this makes the two paths consistent rather than introducing anything new. `get_book_data` stays for callers that genuinely want the bytes, now documented as the expensive option. ## The fetch side has the same problem, and this PR does not fix it I should note: `download_url` in `shelfmark/download/http.py` still builds the inbound file in a `BytesIO` and returns it, so a large download is fully resident while it fetches. @DrNgo's commit fixes that too, with a `tempfile.SpooledTemporaryFile(max_size=...)` so small payloads stay in memory exactly as they do now and large ones spill to disk. I left it out deliberately. It changes the return type of `download_url` from `BytesIO` to a file object, which touches several callers, and it lands in a file that has been reworked around bypass handling, waiting rooms and resume since his branch point. That deserves its own PR rather than riding along with a two-function change. I may send it later; if @DrNgo sends it first, his should win, and if you would rather have both together say so and I will hold this one. ## Verification - `tests/download/test_orchestrator_retry.py`: `get_book_path` returns the path without opening the file (the test fails the run if it does), and reports a missing file rather than handing back a dead path. - The existing `/api/localdownload` tests still pass unchanged, including the history fallback and the ownership checks. - Full suite (3222), ruff, ruff format, basedpyright, vulture green.