Serve a download from the built book, and delete stale uploads

A download built the book again each time. It now sends the kept build,
and builds only if the book changed after it. Stop tracking deletes the
built book. A cancelled import deletes its uploaded file, and the start
deletes the uploads that no job needs.

bindery/jobs.py

@@ -225,6 +225,15 @@ class JobQueue:
             self._save(job)
         return True
 
+    def get(self, job_id: str) -> Job | None:
+        with self._lock:
+            return self._jobs.get(job_id)
+
+    def active(self) -> list[Job]:
+        """The jobs that wait or run."""
+        with self._lock:
+            return [j for j in self._jobs.values() if j.status in (QueueStatus.QUEUED, QueueStatus.RUNNING)]
+
     def has_active(self, story_id: int) -> bool:
         with self._lock:
             return any(

bindery/orchestrator.py

@@ -9,7 +9,7 @@ import time
 import uuid
 from datetime import datetime, timedelta
 from pathlib import Path
-from typing import Any
+from typing import Any, BinaryIO
 
 from bindery import config, fff
 from bindery.backup import daily_backup
@@ -172,7 +172,20 @@ def queue_status(story_id: int | None = None) -> list[dict[str, Any]]:
 
 
 def cancel_job(job_id: str) -> bool:
-    return job_queue.cancel(job_id)
+    job = job_queue.get(job_id)
+    cancelled = job_queue.cancel(job_id)
+    if cancelled and job and job.mode == DownloadMode.IMPORT_FILE:
+        Path(job.options["path"]).unlink(missing_ok=True)
+    return cancelled
+
+
+def remove_stale_uploads() -> None:
+    """Delete the uploaded files that no waiting job needs, for example of a job that failed at each restart."""
+    needed = {Path(j.options["path"]).name for j in job_queue.active() if j.mode == DownloadMode.IMPORT_FILE}
+    for path in config.UPLOAD_DIR.glob("*.epub"):
+        if path.name not in needed:
+            logger.info(f"Deleting the uploaded file {path.name}, which no job needs")
+            path.unlink(missing_ok=True)
 
 
 # Story, chapter and rule management for the UI
@@ -595,6 +608,9 @@ def approve_upload(story_id: int, book_uuid: str | None) -> dict[str, Any] | Non
 
 def delete_story(story_id: int) -> None:
     store.delete_story(story_id)
+    with _build_lock:
+        for built in config.TMP_DIR.glob(f"story-{story_id}-*.epub"):
+            built.unlink(missing_ok=True)
 
 
 def search_library(query: str) -> list[dict[str, Any]]:
@@ -617,12 +633,15 @@ def library_candidates() -> list[dict[str, Any]]:
     ]
 
 
-def build_for_download(story_id: int) -> tuple[Path, str]:
-    """Build the epub from stored data, for a browser download."""
+def build_for_download(story_id: int) -> tuple[BinaryIO, str]:
+    """The epub of a story for a browser download, as an open file. Bindery builds it only if the book changed
+    after the last build."""
     story = _require_story(story_id)
-    path = config.TMP_DIR / f"download-{story_id}.epub"
-    _build(story, _epub_overrides(story, story["pinned"]), path)
-    return path, story["title"]
+    overrides = _epub_overrides(story, story["pinned"])
+    key = _build_key(story, overrides, story["site_values"])
+    # The file is opened under the lock, so a new build cannot delete it first.
+    with _build_lock:
+        return open(_built_epub(story, overrides, key, record=False), "rb"), story["title"]
 
 
 # Metadata locks
@@ -695,7 +714,7 @@ def _build(story: dict[str, Any], overrides: dict[str, Any], out_path: Path) ->
 # changed. The name of the built file holds the key. The story settings "built_key" and "sent_key" are the keys of
 # the last build and of the book that went to CWA.
 
-_build_lock = threading.Lock()
+_build_lock = threading.RLock()
 
 
 def _book_metadata(site: dict[str, Any], overrides: dict[str, Any]) -> dict[str, Any]:
@@ -726,8 +745,13 @@ def _build_key(story: dict[str, Any], overrides: dict[str, Any], site: dict[str,
     return digest.hexdigest()
 
 
-def _built_epub(story: dict[str, Any], overrides: dict[str, Any], key: str, force: bool = False) -> Path:
-    """The epub of the story for this key. Bindery builds it only if it has no file for the key."""
+def _built_epub(
+    story: dict[str, Any], overrides: dict[str, Any], key: str, force: bool = False, record: bool = True
+) -> Path:
+    """The epub of the story for this key. Bindery builds it only if it has no file for the key.
+
+    A download passes record=False. Its build is not a build of a job, so the next job still sees the change.
+    """
     path = config.TMP_DIR / f"story-{story['id']}-{key[:16]}.epub"
     with _build_lock:
         if force or not path.exists():
@@ -741,7 +765,8 @@ def _built_epub(story: dict[str, Any], overrides: dict[str, Any], key: str, forc
             for old in config.TMP_DIR.glob(f"story-{story['id']}-*.epub"):
                 if old != path:
                     old.unlink(missing_ok=True)
-        store.set_setting(story["id"], "built_key", key)
+        if record:
+            store.set_setting(story["id"], "built_key", key)
     return path
 
 
@@ -1378,10 +1403,11 @@ _threads: list[threading.Thread] = []
 
 
 def start_threads() -> None:
-    try:
-        adopt_build_keys()
-    except Exception as e:
-        logger.exception(f"Cannot check the built books: {e}")
+    for prepare in (adopt_build_keys, remove_stale_uploads):
+        try:
+            prepare()
+        except Exception as e:
+            logger.exception(f"Error in {prepare.__name__}: {e}")
     for target in (download_loop, scheduler_loop):
         thread = threading.Thread(target=target, daemon=True, name=target.__name__)
         thread.start()

bindery/web.py

@@ -393,9 +393,11 @@ def delete_chapters(story_id: int) -> Any:
 
 @app.get("/api/stories/<int:story_id>/epub")
 def story_epub(story_id: int) -> Any:
-    path, title = orchestrator.build_for_download(story_id)
+    file, title = orchestrator.build_for_download(story_id)
     name = re.sub(r'[\\/:*?"<>|]', "_", title.strip())[:200] or "story"
-    return send_file(path, download_name=f"{name}.epub", as_attachment=True)
+    response = send_file(file, download_name=f"{name}.epub", as_attachment=True, mimetype="application/epub+zip")
+    response.content_length = os.fstat(file.fileno()).st_size
+    return response
 
 
 @app.get("/api/stories/<int:story_id>/cover")

tests/test_build_keys.py

@@ -203,3 +203,79 @@ def test_the_first_check_after_an_upgrade_sends_nothing(builds, monkeypatch):
     site(monkeypatch, ONE)
     assert update(story_id).startswith("No changes")
     assert builds.call_count == 0 and ingest_files() == []
+
+
+# Downloads in the browser
+
+
+def download(client, story_id):
+    response = client.get(f"/api/stories/{story_id}/epub")
+    assert response.status_code == 200 and response.data.startswith(b"PK")
+    assert response.content_length == len(response.data)
+    response.close()
+    return response.data
+
+
+def test_a_download_uses_the_built_book(builds, client, monkeypatch):
+    story_id = linked_story(ONE)
+    site(monkeypatch, ONE)
+    update(story_id)
+    (built,) = config.TMP_DIR.iterdir()
+    assert download(client, story_id) == download(client, story_id) == built.read_bytes()
+    assert builds.call_count == 1
+
+
+def test_a_download_builds_a_book_that_has_no_file_or_that_changed(builds, client, monkeypatch):
+    story_id = add_story()
+    first = download(client, story_id)
+    download(client, story_id)
+    assert builds.call_count == 1
+    orchestrator.save_rules(story_id, [{"pattern": "Zorian", "replacement": "Zach"}])
+    assert download(client, story_id) != first
+    assert builds.call_count == 2
+    assert len(list(config.TMP_DIR.iterdir())) == 1
+    # The build of a download is not the build of a job. The job sends the book, and marks the change.
+    assert orchestrator.store.get_setting(story_id, "built_key") is None
+    job = Job(mode=DownloadMode.REBUILD, story_id=story_id)
+    assert "library book 1" in orchestrator._run_rebuild(job)
+    assert builds.call_count == 2
+    assert story(story_id)["last_changed"]
+
+
+def test_stop_tracking_deletes_the_built_book(builds, client):
+    story_id, other = add_story(), orchestrator.store.create_story("https://other", "s", "Other", "W", [])
+    download(client, story_id)
+    kept = config.TMP_DIR / f"story-{other}-0123456789abcdef.epub"
+    kept.write_bytes(b"other")
+    orchestrator.delete_story(story_id)
+    assert list(config.TMP_DIR.iterdir()) == [kept]
+
+
+# Uploaded files
+
+
+@pytest.fixture
+def uploads(monkeypatch, tmp_path):
+    monkeypatch.setattr(config, "UPLOAD_DIR", tmp_path / "uploads")
+    config.UPLOAD_DIR.mkdir()
+    return config.UPLOAD_DIR
+
+
+def test_a_cancelled_import_deletes_its_uploaded_file(builds, jobs, uploads, story_epub):
+    path = uploads / "one.epub"
+    path.write_bytes(story_epub)
+    queued = orchestrator.queue_epub_file(path, "story.epub")
+    assert "path" not in queued["options"]
+    assert orchestrator.cancel_job(queued["id"]) is True
+    assert not path.exists()
+    assert orchestrator.cancel_job(queued["id"]) is False
+    assert orchestrator.cancel_job("no-such-job") is False
+
+
+def test_the_start_deletes_uploads_that_no_job_needs(builds, jobs, uploads, story_epub):
+    waiting, stale = uploads / "waiting.epub", uploads / "stale.epub"
+    for path in (waiting, stale):
+        path.write_bytes(story_epub)
+    orchestrator.queue_epub_file(waiting, "story.epub")
+    orchestrator.remove_stale_uploads()
+    assert list(uploads.iterdir()) == [waiting]