From a179c13ee06e4c9dc47844355a0fd2f831ea8afb Mon Sep 17 00:00:00 2001 From: nammovuivui Date: Thu, 22 Oct 2026 14:44:39 +0700 Subject: [PATCH] perf(translate): hard-link PDF cache entries instead of copying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A whole-PDF cache entry and the job output it came from hold the same bytes, so store() and materializing a hit now go through pdf_cache.link_or_copy: a hard link staged at .tmp and os.replace'd, falling back to a copy where links are refused (EXDEV from a tmpfs /tmp, FAT/exFAT, some network shares). Saves one PDF of disk and the copy per translation. A .tmp left by a crashed store is removed before linking: copying onto it would rewrite in place the bytes it may share with an earlier output. The new rule this rests on — never rewrite a translated PDF in place — is recorded in CLAUDE.md and translation-pipeline.md. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 4 + docs/architecture/translation-pipeline.md | 19 ++- .../processors/pdf_cache.py | 45 +++++-- .../processors/processor.py | 31 ++--- tests/test_pdf_cache_store.py | 120 +++++++++++++++++- 5 files changed, 186 insertions(+), 33 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index aab8c26..c75fd7b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -258,6 +258,10 @@ rule has, unless noted, a test. They are the things most easily undone by - Two caches (whole-PDF, paragraph), both SQLite, content-addressed, versioned via `storage/migrations.py`. `is_cacheable_artifact` decides what may be written. +- A whole-PDF cache entry is a **hard link** to the job output it came from + (and a hit is linked back), via `pdf_cache.link_or_copy`, so **never rewrite + a translated PDF in place**. Write a new file and `os.replace` it, or the + write reaches the cache entry too. **Data and chat** → `data-and-chat.md` - Three stores under one root: `pdfusion.db` (system of record), `vectors/` diff --git a/docs/architecture/translation-pipeline.md b/docs/architecture/translation-pipeline.md index 64140c8..d8ff782 100644 --- a/docs/architecture/translation-pipeline.md +++ b/docs/architecture/translation-pipeline.md @@ -270,11 +270,28 @@ offset. A cache file written by a newer build is moved aside and started fresh that only held while the API pinned every request to `auto`. The LLM system prompts name the source explicitly, so `auto` and `en` must not collide — see the comment at `_make_cache_key`. A hit lets `process_pdf` **skip the entire BabelDOC - pipeline**, copy the cached PDF into the live output dir, and emit synthetic + pipeline**, link the cached PDF into the live output dir, and emit synthetic SSE `progress`/`done` events. LRU-evicted to `pdf_cache_max_size_mb` (default 1000 MB). Bump `PIPELINE_VERSION` when any BabelDOC config field that changes output (font, watermark mode, etc.) changes, or how that output is post-processed before it is cached (v2: fonts subset per chunk). + + **An entry and the job output it came from are one file under two names.** + Both directions — `store()` and materializing a hit — go through + `pdf_cache.link_or_copy`, a hard link staged at `.tmp` and `os.replace`d, + falling back to a copy where links are refused (`EXDEV` from a tmpfs + `/tmp`, FAT/exFAT, some network shares). That saves one PDF of disk and + the copy per translation. Removing or replacing either name is safe — the + next job's rmtree, an LRU eviction, a new file `os.replace`d over it — but + **nothing may rewrite an artifact in place** once it exists, or the write + reaches the other name too. That holds today because every rolling + version and every job dir is a fresh name, and anything that rewrites a + PDF (`subset_embedded_fonts`, `export_pdf`) stages a new file and renames + it. Two side effects: on Windows, an eviction while the viewer is + streaming the same bytes from the job's name can fail its unlink (a + warning; the row is gone and the next store of that key replaces the + file); and an evicted entry's space comes back only once the job's name + is gone too, at the next job. 2. **Paragraph cache** (`translators/translation_cache.py`, `get_translation_cache()`). Keyed on `lang_in|lang_out|service|model|text`, with a TTL (`expires_at`). Memoizes individual `translate()` calls across all diff --git a/src/desktop_pdf_translator/processors/pdf_cache.py b/src/desktop_pdf_translator/processors/pdf_cache.py index d6aefc9..9dd7ca7 100644 --- a/src/desktop_pdf_translator/processors/pdf_cache.py +++ b/src/desktop_pdf_translator/processors/pdf_cache.py @@ -30,6 +30,7 @@ files/.pdf -- content-addressed translated PDFs """ +import contextlib import hashlib import logging import os @@ -79,6 +80,39 @@ def compute_file_hash(path: Path) -> str: return h.hexdigest() +def link_or_copy(src: Path, dest: Path) -> None: + """Give `src`'s bytes the name `dest`, atomically. + + A hard link, where the filesystem allows one: the job's temp output and + its cache entry hold the same bytes, so a second name saves a whole PDF + of disk and the time to copy it. Deleting either name — the next job's + rmtree, an LRU eviction — leaves the other intact. What a link does *not* + survive is a rewrite in place, which would reach both names; nothing + writes to an artifact once it exists (each rolling version and each job + dir is new), and that must stay true. Where links are refused (`EXDEV` + across filesystems, FAT/exFAT, some network shares) this copies. + + Staged at `dest.tmp` and moved with `os.replace`, so a concurrent reader + (pdf.js, or another job materializing the same entry) never sees a + half-written `dest`. A missing `src` raises `FileNotFoundError` and + leaves nothing behind. + """ + tmp = dest.with_suffix(dest.suffix + ".tmp") + try: + # A crash between link and rename leaves `tmp` sharing bytes with an + # earlier output; copying onto it would rewrite those in place. + tmp.unlink(missing_ok=True) + try: + os.link(src, tmp) + except OSError: + shutil.copyfile(src, tmp) + os.replace(tmp, dest) + except BaseException: + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) + raise + + def _make_cache_key( file_hash: str, lang_in: str, @@ -423,19 +457,10 @@ def store( pages, ) dest = self.files_dir / f"{key}.pdf" - # Copy via .tmp then atomic rename so a concurrent reader (PdfViewer) - # or another `lookup()` materialize never sees a half-written file at - # `dest`. os.replace is atomic on Windows and POSIX. - tmp = dest.with_suffix(dest.suffix + ".tmp") try: - shutil.copyfile(translated_path, tmp) - os.replace(tmp, dest) + link_or_copy(translated_path, dest) except OSError as exc: logger.warning("PDF cache store: copy failed (%s)", exc) - try: - tmp.unlink(missing_ok=True) - except OSError: - pass return None try: diff --git a/src/desktop_pdf_translator/processors/processor.py b/src/desktop_pdf_translator/processors/processor.py index 1becd5e..d96dbe0 100644 --- a/src/desktop_pdf_translator/processors/processor.py +++ b/src/desktop_pdf_translator/processors/processor.py @@ -42,7 +42,12 @@ rolling_segments, selected_pages, ) -from .pdf_cache import compute_file_hash, get_pdf_cache, is_cacheable_artifact +from .pdf_cache import ( + compute_file_hash, + get_pdf_cache, + is_cacheable_artifact, + link_or_copy, +) from .pdf_pages import ( inspect_pdf, rebuild_rolling_pdf, @@ -428,36 +433,22 @@ async def process_pdf( # output_dir is a fresh per-job temp dir, there are no # stale leftovers to nuke.) target = output_dir / f"{file_path.stem}_translated_v999.pdf" - # Copy via .tmp + atomic rename so a concurrent reader - # (PdfViewer, or another job hitting the same input) never - # sees a half-written `target`. os.replace is atomic on - # Windows and POSIX. - tmp_target = target.with_suffix(target.suffix + ".tmp") try: - shutil.copyfile(hit.cached_path, tmp_target) - os.replace(tmp_target, target) + link_or_copy(hit.cached_path, target) except FileNotFoundError as exc: - # The cached file was evicted between lookup() and our - # copyfile — most likely by another job's store() + # The cached file was evicted between lookup() and the + # link — most likely by another job's store() # triggering an LRU sweep. Treat as a miss and fall # through to the real pipeline. logger.info( "PDF cache entry evicted under us by a concurrent " "store (%s); falling through to full pipeline", exc, ) - try: - tmp_target.unlink(missing_ok=True) - except OSError: - pass except OSError as exc: logger.warning( "PDF cache hit but materialize failed (%s); " "running full pipeline", exc, ) - try: - tmp_target.unlink(missing_ok=True) - except OSError: - pass else: logger.info( "PDF cache HIT (cached_at=%s, hits=%d) → %s", @@ -588,7 +579,7 @@ async def process_pdf( # Persist into the PDF-level cache so the next translation of this # exact file (same service+model+langs) short-circuits. Run on a - # worker thread because store() hashes the input and copies the + # worker thread because store() hashes the input and links the # output — both blocking. Errors are non-fatal. # # What may be cached at all is `is_cacheable_artifact`'s call: the @@ -604,7 +595,7 @@ async def process_pdf( ) if should_cache: # Fire-and-forget: don't block CompletionEvent on store(). The - # SHA-256 + shutil.copyfile + SQLite INSERT + LRU sweep would + # SHA-256 + link (or copy) + SQLite INSERT + LRU sweep would # otherwise add hundreds of ms (and sometimes seconds, on cap # hits with big evictions) between BabelDOC finishing and the # user seeing "done". Track the task in a module-level set so diff --git a/tests/test_pdf_cache_store.py b/tests/test_pdf_cache_store.py index 33c6790..a43c164 100644 --- a/tests/test_pdf_cache_store.py +++ b/tests/test_pdf_cache_store.py @@ -8,6 +8,9 @@ than served, and a store that fails leaves neither a row nor a stray `.tmp`; * the atomic copy, which is what stops a concurrent reader (pdf.js, or another job materializing the same entry) from seeing a half-written PDF; +* that the "copy" is a hard link where the filesystem allows one, a real copy + where it doesn't, and that nothing the lifecycle does to the job's name + reaches the entry; * LRU eviction, and the size accounting it depends on; * the two rules that keep a page selection's entry from costing a user their whole-document ones — it is evicted first, and superseded outright once the @@ -22,6 +25,8 @@ from __future__ import annotations +import errno +import os import shutil import sqlite3 from datetime import datetime, timedelta @@ -37,6 +42,7 @@ _MIGRATIONS, _make_cache_key, compute_file_hash, + link_or_copy, ) from conftest import MINIMAL_PDF @@ -192,6 +198,110 @@ def test_the_cached_copy_outlives_the_job_output_dir( assert hit is not None and hit.cached_path.exists() +def test_a_store_links_the_job_output_rather_than_copying_it( + cache: PDFTranslationCache, source_pdf: Path, translated_pdf: Path +): + """The job's temp file and the cache entry hold the same bytes, so a second + name for them costs no disk and no copy time — a whole PDF's worth of each + on every translation otherwise.""" + dest = put(cache, source_pdf, translated_pdf) + + assert dest is not None + assert os.path.samefile(dest, translated_pdf) + + +@pytest.mark.parametrize( + "refusal", + [ + OSError(errno.EXDEV, "Invalid cross-device link"), # a tmpfs /tmp + PermissionError(errno.EPERM, "Operation not permitted"), # FAT, exFAT + ], +) +def test_a_filesystem_that_refuses_links_gets_a_copy( + cache: PDFTranslationCache, + source_pdf: Path, + translated_pdf: Path, + monkeypatch: pytest.MonkeyPatch, + refusal: OSError, +): + def refuse(*_a, **_kw): + raise refusal + + monkeypatch.setattr(pdf_cache_module.os, "link", refuse) + dest = put(cache, source_pdf, translated_pdf) + + assert dest is not None + assert dest.read_bytes() == translated_pdf.read_bytes() + assert look(cache, source_pdf) is not None + + +def test_a_staging_file_left_by_a_crash_is_replaced_not_written_through( + cache: PDFTranslationCache, + source_pdf: Path, + translated_pdf: Path, + tmp_path: Path, +): + """A store killed between link and rename leaves `.pdf.tmp` sharing + bytes with that earlier run's output. Writing over it would rewrite those + bytes in place; the stale name has to go first.""" + earlier = tmp_path / "earlier_job" / "paper_translated_v001.pdf" + earlier.parent.mkdir() + earlier.write_bytes(MINIMAL_PDF + b"% earlier run\n") + key = _make_cache_key( + compute_file_hash(source_pdf), "en", "vi", "openai", "gpt-4o", + PIPELINE_VERSION, + ) + os.link(earlier, cache.files_dir / f"{key}.pdf.tmp") + + dest = put(cache, source_pdf, translated_pdf) + + assert dest is not None and os.path.samefile(dest, translated_pdf) + assert earlier.read_bytes() == MINIMAL_PDF + b"% earlier run\n" + + +def test_replacing_the_job_output_leaves_the_cache_entry_alone( + cache: PDFTranslationCache, source_pdf: Path, translated_pdf: Path +): + """What the lifecycle does to a job's file after the store — a new name + `os.replace`d over it, or the whole dir removed — only ever changes which + bytes a *name* points at, so the cache keeps its own.""" + dest = put(cache, source_pdf, translated_pdf) + stored = dest.read_bytes() + + newer = translated_pdf.with_name("newer.pdf") + newer.write_bytes(MINIMAL_PDF + b"% something else\n") + os.replace(newer, translated_pdf) + + assert dest.read_bytes() == stored + + +def test_a_hit_is_materialized_as_a_link(tmp_path: Path): + """The cache-hit path names the cached PDF inside the job dir.""" + cached = tmp_path / "files" / "key.pdf" + cached.parent.mkdir() + cached.write_bytes(MINIMAL_PDF) + target = tmp_path / "job" / "paper_translated_v999.pdf" + target.parent.mkdir() + + link_or_copy(cached, target) + + assert os.path.samefile(cached, target) + assert [p.name for p in target.parent.iterdir()] == [target.name] + + +def test_an_entry_evicted_before_it_is_materialized_raises_not_found( + tmp_path: Path, +): + """`process_pdf` treats exactly this as a miss: another job's LRU sweep + deleted the file between `lookup()` and the link.""" + target = tmp_path / "job" / "paper_translated_v999.pdf" + target.parent.mkdir() + + with pytest.raises(FileNotFoundError): + link_or_copy(tmp_path / "files" / "gone.pdf", target) + assert list(target.parent.iterdir()) == [] + + def test_a_hit_needs_every_key_field_to_match( cache: PDFTranslationCache, source_pdf: Path, translated_pdf: Path ): @@ -275,8 +385,12 @@ def test_the_whole_document_answers_any_selection( """A full translation has every selected page translated too, so it is tried first and counts as one hit.""" put(cache, source_pdf, translated_pdf) - translated_pdf.write_bytes(MINIMAL_PDF + b"% pages 1-20\n") - put(cache, source_pdf, translated_pdf, pages="1-20") + # A second run writes into a job dir of its own, never over the first + # run's file — which the cache entry may share bytes with. + partial = translated_pdf.parent.with_name("job2") / translated_pdf.name + partial.parent.mkdir() + partial.write_bytes(MINIMAL_PDF + b"% pages 1-20\n") + put(cache, source_pdf, partial, pages="1-20") hit = look(cache, source_pdf, pages="1-20") assert hit is not None and hit.pages is None @@ -393,6 +507,8 @@ def test_a_failed_copy_leaves_no_row_and_no_temp_file( def deny(*_a, **_kw): raise PermissionError(13, "Access is denied") + # Both ways of placing the bytes: a link refused falls back to a copy. + monkeypatch.setattr(pdf_cache_module.os, "link", deny) monkeypatch.setattr(pdf_cache_module.shutil, "copyfile", deny) assert put(cache, source_pdf, translated_pdf) is None