durability: retry SMB replace-existing; triage derives PC when the me…

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)
 

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"

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)

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)
 
 

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()

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'<String ID="s{i}" HPOS="{10 * i}" VPOS="10" WIDTH="9" HEIGHT="9"'
+        + (f' WC="{wc}"' if wc is not None else "")
+        + f' CONTENT="{c}"/>'
+        for i, (c, wc) in enumerate(strings)
+    )
+    return (
+        f'<?xml version="1.0" encoding="UTF-8"?>\n'
+        f'<alto xmlns="{ALTO_NS}"><Layout><Page {page_attrs}><PrintSpace>'
+        f"<TextBlock ID=\"b0\"><TextLine ID=\"l0\">{words}</TextLine></TextBlock>"
+        f"</PrintSpace></Page></Layout></alto>"
+    )
+
+
+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()