diff --git a/bindery/jobs.py b/bindery/jobs.py index 2575b03..339cc7c 100644 --- a/bindery/jobs.py +++ b/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( diff --git a/bindery/orchestrator.py b/bindery/orchestrator.py index 6b5c0ef..12a3973 100644 --- a/bindery/orchestrator.py +++ b/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() diff --git a/bindery/web.py b/bindery/web.py index 8f54a36..a683096 100644 --- a/bindery/web.py +++ b/bindery/web.py @@ -393,9 +393,11 @@ def delete_chapters(story_id: int) -> Any: @app.get("/api/stories//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//cover") diff --git a/tests/test_build_keys.py b/tests/test_build_keys.py index 89b0128..6a3987e 100644 --- a/tests/test_build_keys.py +++ b/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]