From cbb92211df10f9665bfff5ce081e89bb0ba41514 Mon Sep 17 00:00:00 2001 From: Broderick Davidson J Schipp Date: Thu, 30 Jul 2026 16:04:21 -0400 Subject: [PATCH] durability: retry SMB replace-existing; triage derives PC when the merge's write was lost (v0.8.9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing ALTO to \wl.mydata.purdue.edu, 3 of 41 pages in one object failed the confidence merge with WinError 5 while pages written seconds either side succeeded. Not a permissions problem: the files are not read-only, no temp file leaked, and a real ACL denial would have failed all 41. It is the familiar Windows/SMB case where a transient handle on the destination — antivirus, the Search indexer, an oplock break in flight — makes replace-existing fail, and Windows reports it as ERROR_ACCESS_DENIED (5) as readily as ERROR_SHARING_VIOLATION (32). Errno cannot tell transient from real; only retrying can. atomic_write_bytes now routes the final rename through _replace_with_retry: 6 attempts with backoff (~1.5s) on winerror 5/32 only. A genuine denial still raises, just later. Disk-full and POSIX-style errors fail fast, unretried. The second half of the bug was how the failure presented. Tesseract's own ALTO renderer emits per-word WC natively (verified against 5.5.0); our merge re-derives those and adds Page/@PC. So a lost merge write keeps every word confidence and drops only the page mean — and review_pages filtered on `p.pc is not None`, so those pages vanished from the correction queue entirely. On an object with mean WC 0.351 that is the worst possible page to hide. triage.scan_alto_file now derives PC from the word WCs when the attribute is absent — the same mean the merge would have stored, and the derivation remediate already applies when it rewrites PC after an edit. A stored PC is never overridden, and unscored (GLMOCR) and empty pages still report None. Measured on the affected object: review queue 29 -> 32 pages. The Review tab reads the same scan, so its worst-first ordering recovers too. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- src/alto_ocr/__init__.py | 2 +- src/alto_ocr/_util.py | 43 +++++++++- src/alto_ocr/triage.py | 12 +++ tests/test_atomic_write_retry.py | 133 +++++++++++++++++++++++++++++++ tests/test_triage_missing_pc.py | 103 ++++++++++++++++++++++++ 6 files changed, 292 insertions(+), 3 deletions(-) create mode 100644 tests/test_atomic_write_retry.py create mode 100644 tests/test_triage_missing_pc.py diff --git a/CLAUDE.md b/CLAUDE.md index 5ca97c3..3332dc3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,7 @@ Decisions already made (do not re-litigate without asking): - METS-per-Ref-ID is on by default, opt-out via `--no-mets`. The institution's planned LibNova workflow may not consume METS; keep the toggle exposed without changing the default until that's confirmed. - The `reformat` command takes `--source-tiff-dir` (typically a preservation directory) for per-page DPI lookup since no TIFs live in the output tree. Falls back to `--min-dpi` (300) when no source dir is provided. - XML parsing of pipeline files (ALTO, METS) goes through `alto_format.hardened_parser()` — DTD entity resolution off (output trees live on shared drives and round-trip through external editors; we never use DTD entities). Route any new `etree.parse` of pipeline files through it. `repackage` preserves the METS header's original `CREATEDATE` and stamps `LASTMODDATE` on each rebuild; triage CSV cells sourced from filenames are neutralized against Excel formula injection. -- Canonical outputs (ALTO, Text, METS — including review-GUI saves, failure stubs, and `repackage` rewrites) are written **atomically** via `_util.atomic_write_text/_bytes` (temp file + `os.replace` in the destination directory): a crash or network hiccup mid-write can never truncate an existing file. Load-bearing for hand-corrected ALTO, where truncation destroys human labor. Route any new output-file writes through these helpers; byte-parity with the old `Path.write_text` behavior (incl. Windows newline translation) is locked in by `tests/test_atomic_write.py`. +- Canonical outputs (ALTO, Text, METS — including review-GUI saves, failure stubs, and `repackage` rewrites) are written **atomically** via `_util.atomic_write_text/_bytes` (temp file + `os.replace` in the destination directory): a crash or network hiccup mid-write can never truncate an existing file. Load-bearing for hand-corrected ALTO, where truncation destroys human labor. Route any new output-file writes through these helpers; byte-parity with the old `Path.write_text` behavior (incl. Windows newline translation) is locked in by `tests/test_atomic_write.py`. The final `os.replace` **retries with backoff** (6 attempts, ~1.5 s) on Windows errors 5/32: writing to an SMB share, a transient handle from antivirus or the Search indexer makes replace-existing fail with `ERROR_ACCESS_DENIED` as often as `ERROR_SHARING_VIOLATION`, so errno can't distinguish it from a real ACL denial — only retrying can, and a genuine denial still raises after the attempts. Found 2026-07-30 writing ALTO to `\\wl.mydata.purdue.edu`: 3 of 41 pages in one object lost the confidence merge's write while pages seconds either side succeeded. Because Tesseract's own ALTO renderer emits per-word `WC` natively and *our* merge is what adds `Page/@PC`, each casualty kept its word confidences but lost its page mean — and `review_pages` filtered on `pc is not None`, so those pages silently vanished from the correction queue. `triage.scan_alto_file` now derives PC from the word WCs when the attribute is absent (never overriding a stored value). Locked by `tests/test_atomic_write_retry.py` and `tests/test_triage_missing_pc.py`. ## Image preprocessing for OCR accuracy (load-bearing) diff --git a/src/alto_ocr/__init__.py b/src/alto_ocr/__init__.py index 713e4c8..d10559a 100644 --- a/src/alto_ocr/__init__.py +++ b/src/alto_ocr/__init__.py @@ -1,3 +1,3 @@ """alto_ocr — TIF page scans → METS/ALTO XML + plain-text transcripts.""" -__version__ = "0.8.8" +__version__ = "0.8.9" diff --git a/src/alto_ocr/_util.py b/src/alto_ocr/_util.py index 78555ae..5c52e47 100644 --- a/src/alto_ocr/_util.py +++ b/src/alto_ocr/_util.py @@ -12,12 +12,50 @@ from __future__ import annotations +import logging import os import re import sys import tempfile +import time from pathlib import Path +logger = logging.getLogger(__name__) + +# os.replace onto an existing file intermittently fails on Windows/SMB shares +# when something else briefly holds a handle on the destination — an antivirus +# scanner, the Search indexer, or an oplock break still in flight. Windows +# surfaces that as ERROR_ACCESS_DENIED (5) as often as ERROR_SHARING_VIOLATION +# (32), so the errno alone can't distinguish it from a real permission problem; +# the retry does, since a genuine ACL denial fails every attempt. Observed +# 2026-07-30 writing ALTO to \\wl.mydata.purdue.edu (3 of 41 pages in one +# object, while pages written seconds either side succeeded). +_REPLACE_ATTEMPTS = 6 +_REPLACE_BACKOFF_S = 0.05 +_TRANSIENT_WINERRORS = frozenset({5, 32}) + + +def _replace_with_retry(tmp_name: str, path: Path) -> None: + """``os.replace`` with bounded backoff for transient Windows/SMB failures. + + Retries only errors that a concurrent handle can plausibly cause. A real + permission problem still raises — it just takes ~1.5s of retries first, + which is nothing next to an OCR pass and buys robustness on network drives. + """ + delay = _REPLACE_BACKOFF_S + for attempt in range(1, _REPLACE_ATTEMPTS + 1): + try: + os.replace(tmp_name, path) + if attempt > 1: + logger.debug("replace of %s succeeded on attempt %d", path.name, attempt) + return + except OSError as e: + transient = getattr(e, "winerror", None) in _TRANSIENT_WINERRORS + if not transient or attempt == _REPLACE_ATTEMPTS: + raise + time.sleep(delay) + delay *= 2 + def force_utf8_output() -> None: """Make stdout/stderr tolerate non-ASCII output on Windows consoles. @@ -59,6 +97,9 @@ def atomic_write_bytes(path: Path | str, data: bytes) -> None: load-bearing for hand-corrected ALTO, where a truncated file destroys human labor. The temp file lives in the destination's directory so the final rename never crosses filesystems. The parent directory must exist. + + The final rename retries briefly (see ``_replace_with_retry``) because on + SMB shares it intermittently fails with a transient access-denied. """ path = Path(path) fd, tmp_name = tempfile.mkstemp( @@ -72,7 +113,7 @@ def atomic_write_bytes(path: Path | str, data: bytes) -> None: os.fsync(f.fileno()) except OSError: pass # best effort — some network filesystems refuse fsync - os.replace(tmp_name, path) + _replace_with_retry(tmp_name, path) except BaseException: try: os.unlink(tmp_name) diff --git a/src/alto_ocr/triage.py b/src/alto_ocr/triage.py index 6f88c30..d3dc596 100644 --- a/src/alto_ocr/triage.py +++ b/src/alto_ocr/triage.py @@ -195,6 +195,18 @@ def scan_alto_file(alto_path: Path, *, low_wc: float = DEFAULT_LOW_WC) -> PageCo status = "scored" else: status = "unscored" + + if pc is None and scored > 0: + # A page with per-word WC but no Page/@PC means the confidence merge + # wrote the words and then failed before stamping the page mean — + # Tesseract's own ALTO renderer emits WC natively, ours adds PC. Left + # as None the page drops out of review_pages entirely and no corrector + # ever sees it, which is the worst possible failure mode for a page we + # know nothing good about. Recompute the mean from the WCs on hand: + # exactly what the merge would have stored, and the same derivation + # remediate already applies when it rewrites PC after an edit. + pc = wc_sum / scored + return PageConfidence(alto_path.stem, status, pc, words, scored, low, wc_sum) diff --git a/tests/test_atomic_write_retry.py b/tests/test_atomic_write_retry.py new file mode 100644 index 0000000..4f24188 --- /dev/null +++ b/tests/test_atomic_write_retry.py @@ -0,0 +1,133 @@ +"""The final rename retries transient Windows/SMB access-denied failures. + +``os.replace`` onto an existing file intermittently fails on SMB shares when +something else briefly holds a handle on the destination (antivirus, the Search +indexer, an oplock break in flight). Windows reports that as ERROR_ACCESS_DENIED +(5) just as often as ERROR_SHARING_VIOLATION (32), so errno alone cannot tell it +apart from a real permission problem — only retrying can. + +Observed 2026-07-30 writing ALTO to \\\\wl.mydata.purdue.edu: 3 of 41 pages in one +object failed the confidence merge's write while pages written seconds either +side succeeded. Each lost ``Page/@PC``, which silently removed them from the +review queue. + +Run from the repo root (so ``src.alto_ocr`` resolves): + + python -m unittest tests.test_atomic_write_retry +""" + +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from src.alto_ocr import _util # noqa: E402 + + +def _winerr(code: int) -> PermissionError: + e = PermissionError("Access is denied") + e.winerror = code # type: ignore[attr-defined] + return e + + +class TestReplaceRetry(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.dir = Path(self._tmp.name) + self.dest = self.dir / "page.xml" + self.dest.write_text("old", encoding="utf-8") + + def tearDown(self): + self._tmp.cleanup() + + def test_succeeds_after_transient_access_denied(self): + real = _util.os.replace + calls = {"n": 0} + + def flaky(src, dst): + calls["n"] += 1 + if calls["n"] < 3: + raise _winerr(5) + return real(src, dst) + + with mock.patch.object(_util.os, "replace", flaky), \ + mock.patch.object(_util.time, "sleep"): + _util.atomic_write_text(self.dest, "new") + + self.assertEqual(self.dest.read_text(encoding="utf-8"), "new") + self.assertEqual(calls["n"], 3) + + def test_sharing_violation_is_also_retried(self): + real = _util.os.replace + calls = {"n": 0} + + def flaky(src, dst): + calls["n"] += 1 + if calls["n"] < 2: + raise _winerr(32) + return real(src, dst) + + with mock.patch.object(_util.os, "replace", flaky), \ + mock.patch.object(_util.time, "sleep"): + _util.atomic_write_text(self.dest, "new") + self.assertEqual(self.dest.read_text(encoding="utf-8"), "new") + + def test_persistent_denial_still_raises_and_leaves_original(self): + """A real ACL problem must not be swallowed — and must not truncate.""" + with mock.patch.object(_util.os, "replace", side_effect=_winerr(5)), \ + mock.patch.object(_util.time, "sleep"): + with self.assertRaises(PermissionError): + _util.atomic_write_text(self.dest, "new") + + self.assertEqual(self.dest.read_text(encoding="utf-8"), "old") + self.assertEqual(list(self.dir.glob("*.tmp")), [], "temp file leaked") + + def test_non_transient_oserror_is_not_retried(self): + calls = {"n": 0} + + def boom(src, dst): + calls["n"] += 1 + e = OSError("no space") + e.winerror = 112 # ERROR_DISK_FULL + raise e + + with mock.patch.object(_util.os, "replace", boom), \ + mock.patch.object(_util.time, "sleep"): + with self.assertRaises(OSError): + _util.atomic_write_text(self.dest, "new") + self.assertEqual(calls["n"], 1, "should fail fast, not retry") + + def test_posix_style_oserror_without_winerror_is_not_retried(self): + calls = {"n": 0} + + def boom(src, dst): + calls["n"] += 1 + raise OSError(13, "Permission denied") + + with mock.patch.object(_util.os, "replace", boom), \ + mock.patch.object(_util.time, "sleep"): + with self.assertRaises(OSError): + _util.atomic_write_text(self.dest, "new") + self.assertEqual(calls["n"], 1) + + def test_happy_path_replaces_once(self): + real = _util.os.replace + calls = {"n": 0} + + def counted(src, dst): + calls["n"] += 1 + return real(src, dst) + + with mock.patch.object(_util.os, "replace", counted): + _util.atomic_write_text(self.dest, "new") + self.assertEqual(calls["n"], 1) + self.assertEqual(self.dest.read_text(encoding="utf-8"), "new") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_triage_missing_pc.py b/tests/test_triage_missing_pc.py new file mode 100644 index 0000000..5c01a1b --- /dev/null +++ b/tests/test_triage_missing_pc.py @@ -0,0 +1,103 @@ +"""A scored page with no Page/@PC must still reach the review queue. + +Tesseract's ALTO renderer emits per-word ``WC`` natively; our confidence merge +re-derives those and adds the page-level ``PC``. If the merge's write fails +(see tests/test_atomic_write_retry.py) the file keeps its words' WC but never +gains PC — and ``review_pages`` filters on ``p.pc is not None``, so the page +silently disappeared from the correction queue. Observed on three pages of +MSA00005_0653bfcb... on 2026-07-30. + +Run from the repo root (so ``src.alto_ocr`` resolves): + + python -m unittest tests.test_triage_missing_pc +""" + +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from src.alto_ocr.triage import scan_alto_file, scan_object # noqa: E402 + +ALTO_NS = "http://www.loc.gov/standards/alto/ns-v3#" + + +def _alto(strings: list[tuple[str, str | None]], pc: str | None = None) -> str: + """One-line ALTO with the given (CONTENT, WC) words; PC optional.""" + page_attrs = 'ID="page_0" PHYSICAL_IMG_NR="1" WIDTH="1000" HEIGHT="1000"' + if pc is not None: + page_attrs += f' PC="{pc}"' + words = "".join( + f'' + for i, (c, wc) in enumerate(strings) + ) + return ( + f'\n' + f'' + f"{words}" + f"" + ) + + +class TestMissingPageConfidence(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.dir = Path(self._tmp.name) + + def tearDown(self): + self._tmp.cleanup() + + def _write(self, name: str, xml: str) -> Path: + p = self.dir / name + p.write_text(xml, encoding="utf-8") + return p + + def test_pc_is_derived_from_word_confidences(self): + p = self._write("a.xml", _alto([("one", "0.20"), ("two", "0.40")])) + page = scan_alto_file(p) + self.assertEqual(page.status, "scored") + self.assertIsNotNone(page.pc, "page dropped its confidence entirely") + self.assertAlmostEqual(page.pc, 0.30, places=6) + + def test_stored_pc_still_wins(self): + """Derivation is a fallback, never an override of what the merge wrote.""" + p = self._write("a.xml", _alto([("one", "0.20"), ("two", "0.40")], pc="0.91")) + self.assertAlmostEqual(scan_alto_file(p).pc, 0.91, places=6) + + def test_unscored_page_keeps_no_pc(self): + """GLMOCR output has no WC at all — there is nothing to derive from.""" + p = self._write("a.xml", _alto([("one", None), ("two", None)])) + page = scan_alto_file(p) + self.assertEqual(page.status, "unscored") + self.assertIsNone(page.pc) + + def test_empty_page_keeps_no_pc(self): + p = self._write("a.xml", _alto([])) + page = scan_alto_file(p) + self.assertEqual(page.status, "empty") + self.assertIsNone(page.pc) + + def test_page_reaches_the_review_queue(self): + """The whole point: a low-confidence page with no PC must be flagged.""" + alto = self.dir / "ALTO" + alto.mkdir() + (alto / "good.xml").write_text( + _alto([("a", "0.95"), ("b", "0.95")], pc="0.95"), encoding="utf-8") + (alto / "lost_pc.xml").write_text( + _alto([("a", "0.20"), ("b", "0.30")]), encoding="utf-8") + + obj = scan_object(self.dir) + flagged = {p.stem for p in obj.review_pages} + self.assertIn("lost_pc", flagged, + "page with words but no PC never reaches a corrector") + self.assertNotIn("good", flagged) + + +if __name__ == "__main__": + unittest.main()