mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-10-06 09:34:39 +01:00
fix(download): stream a completed book instead of buffering it in RAM - lowering memory needs significantly (#1378)
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.
This commit is contained in:
@@ -285,3 +285,50 @@ def test_get_book_data_clears_download_path_when_file_read_fails(monkeypatch, tm
|
||||
assert file_data is None
|
||||
assert returned_task is task
|
||||
assert task.download_path is None
|
||||
|
||||
|
||||
def test_get_book_path_returns_the_path_without_reading_the_file(monkeypatch, tmp_path):
|
||||
import shelfmark.download.orchestrator as orchestrator
|
||||
|
||||
book = tmp_path / "book.epub"
|
||||
book.write_bytes(b"x" * 4096)
|
||||
task = DownloadTask(
|
||||
task_id="task-book-path-1",
|
||||
source="direct_download",
|
||||
title="Streamed Book",
|
||||
download_path=str(book),
|
||||
)
|
||||
|
||||
mock_queue = MagicMock()
|
||||
mock_queue.get_task.return_value = task
|
||||
monkeypatch.setattr(orchestrator, "book_queue", mock_queue)
|
||||
|
||||
def _no_reads(*_args, **_kwargs):
|
||||
raise AssertionError("get_book_path must not open the file")
|
||||
|
||||
monkeypatch.setattr(Path, "open", _no_reads)
|
||||
|
||||
path, returned_task = orchestrator.get_book_path(task.task_id)
|
||||
|
||||
assert path == str(book)
|
||||
assert returned_task is task
|
||||
|
||||
|
||||
def test_get_book_path_reports_a_missing_file_rather_than_a_dead_path(monkeypatch, tmp_path):
|
||||
import shelfmark.download.orchestrator as orchestrator
|
||||
|
||||
task = DownloadTask(
|
||||
task_id="task-book-path-2",
|
||||
source="direct_download",
|
||||
title="Gone",
|
||||
download_path=str(tmp_path / "gone.epub"),
|
||||
)
|
||||
|
||||
mock_queue = MagicMock()
|
||||
mock_queue.get_task.return_value = task
|
||||
monkeypatch.setattr(orchestrator, "book_queue", mock_queue)
|
||||
|
||||
path, returned_task = orchestrator.get_book_path(task.task_id)
|
||||
|
||||
assert path is None
|
||||
assert returned_task is task
|
||||
|
||||
Reference in New Issue
Block a user