fix(mutation): harden prepare reaper and retarget for Windows

Three cross-platform defects in the lock-free prepare path (ee22914),
each with a POSIX-simulated regression test (CI is Linux-only):

- reaper stalled the KB on a locked loose file: the file branch's
  unlink(missing_ok=True) only swallows FileNotFoundError, so a
  PermissionError (AV/indexer on Windows) escaped kb_lock and crashed
  every exclusive-lock command. Now wrapped in try/except OSError.
- read-only staging never self-healed: copy2 preserves a read-only
  source's bit, and rmtree(ignore_errors=True) can't delete read-only
  entries on Windows, so the orphan re-logged 'Could not fully reap' on
  every lock acquisition. rmtree now passes an onerror that clears the
  read-only bit and retries.
- retarget rewrote line endings: write_text without newline= translated
  \n to \r\n on Windows, so a collision-renamed source became CRLF
  while all others stayed LF. Switched to atomic_write_text (LF-preserving).

Author: gwokhou

openkb/cli.py

@@ -535,7 +535,10 @@ def _retarget_prepared_document_artifacts(prepared, doc_name: str) -> None:
     if old_source.exists():
         text = old_source.read_text(encoding="utf-8")
         text = text.replace(f"sources/images/{old_doc_name}/", f"sources/images/{doc_name}/")
-        old_source.write_text(text, encoding="utf-8")
+        # LF-preserving rewrite: write_text without newline= would translate \n
+        # to \r\n on Windows, leaving a collision-renamed source CRLF while every
+        # other source (written via atomic_write_text in convert) stays LF.
+        atomic_write_text(old_source, text)
         old_source.rename(new_source)
         result.source_path = new_source
     # Defensive: if prepare ever writes source_path off the <doc_name>.md

openkb/locks.py

@@ -13,6 +13,7 @@
 import logging
 import os
 import shutil
+import stat
 import tempfile
 import threading
 from pathlib import Path
@@ -127,6 +128,32 @@ def _drain_pending_journals(openkb_dir: Path) -> None:
     _reap_prepare_staging(openkb_dir)
 
 
+def _reap_prepare_staging_onerror(func, path, exc_info) -> None:
+    """``shutil.rmtree`` onerror: clear a read-only bit so the reap self-heals.
+
+    ``shutil.copy2`` preserves a read-only source's attribute into staging; on
+    Windows ``os.unlink``/``os.rmdir`` deny a read-only entry, so rmtree would
+    leave the orphan behind on every reap ("Could not fully reap …" forever,
+    re-logged on each exclusive-lock acquisition). Adding the owner-write bit
+    clears ``FILE_ATTRIBUTE_READONLY`` and the retry succeeds (POSIX is
+    unaffected — a read-only bit never blocks unlink there). Any other error is
+    swallowed to preserve the best-effort reap this branch has always had; a
+    still-stuck orphan is reported via the ``orphan.exists()`` check below.
+    ``path`` is the absolute path shutil reports (``_rmtree_safe_fd`` →
+    ``onexc`` → here), so ``chmod`` and the retry resolve correctly.
+    """
+    exc = exc_info[1] if exc_info else None
+    if isinstance(exc, PermissionError) and func in (os.unlink, os.rmdir):
+        try:
+            os.chmod(path, os.stat(path).st_mode | stat.S_IWUSR)
+        except OSError:
+            return
+        try:
+            func(path)
+        except OSError:
+            return
+
+
 def _reap_prepare_staging(openkb_dir: Path) -> None:
     """Remove orphaned prepare-staging dirs left by interrupted prepares.
 
@@ -152,9 +179,16 @@ def _reap_prepare_staging(openkb_dir: Path) -> None:
             log.warning("Skipping symlink in prepare staging (not followed): %s", orphan)
             continue
         if orphan.is_dir():
-            shutil.rmtree(orphan, ignore_errors=True)
+            shutil.rmtree(orphan, onerror=_reap_prepare_staging_onerror)
         else:
-            orphan.unlink(missing_ok=True)
+            # Best-effort: a loose file an AV/indexer holds open on Windows
+            # raises PermissionError (missing_ok only swallows FileNotFoundError).
+            # Swallow OSError so one unreachable orphan can't escape the reap,
+            # run up through kb_lock, and stall every exclusive-lock command.
+            try:
+                orphan.unlink(missing_ok=True)
+            except OSError as exc:
+                log.debug("Could not unlink orphaned prepare staging %s: %s", orphan, exc)
         if orphan.exists():
             log.warning("Could not fully reap orphaned prepare staging: %s", orphan)
         else:

tests/test_add_prepare.py

@@ -115,6 +115,54 @@ def test_commit_prepared_document_resolves_final_name_under_serial_owner(tmp_pat
     assert (kb_dir / "wiki" / "sources" / f"{renamed[0]['doc_name']}.md").exists()
 
 
+def test_retarget_preserves_lf_line_endings(tmp_path, monkeypatch):
+    """Renaming a prepared source on collision must keep it LF.
+
+    The convert phase writes via atomic_write_text (binary, LF preserved), but
+    retarget used Path.write_text without newline=, which on Windows translates
+    \\n to \\r\\n — so a collision-renamed source ends up CRLF while every other
+    source stays LF (inconsistent KB, noisy git diffs, stray \\r in \\n-split
+    parsers). POSIX write_text already leaves LF, so we force the Windows
+    translation to prove the contract holds regardless of platform.
+    """
+    from openkb.add_prepare import PreparedDocument
+    from openkb.cli import _retarget_prepared_document_artifacts
+    from openkb.converter import ConvertResult
+
+    staging = tmp_path / "staging"
+    (staging / "raw").mkdir(parents=True)
+    sources = staging / "wiki" / "sources"
+    sources.mkdir(parents=True)
+
+    old_name = "orig"
+    new_name = "orig-deadbeef"
+    old_source = sources / f"{old_name}.md"
+    old_source.write_text("# Title\n\nsources/images/orig/x.png\n", encoding="utf-8")
+    old_raw = staging / "raw" / f"{old_name}.md"
+    old_raw.write_text("raw\n", encoding="utf-8")
+
+    prepared = PreparedDocument(
+        input_index=0,
+        source_path=Path(f"{old_name}.md"),
+        staging_dir=staging,
+        result=ConvertResult(doc_name=old_name, raw_path=old_raw, source_path=old_source),
+    )
+
+    real_write_text = Path.write_text
+
+    def windows_translate(self, data, *args, **kwargs):
+        # Mimic Windows default text-mode write (newline=None): \n -> \r\n.
+        return real_write_text(self, data.replace("\n", "\r\n"), *args, **kwargs)
+
+    monkeypatch.setattr(Path, "write_text", windows_translate)
+
+    _retarget_prepared_document_artifacts(prepared, new_name)
+
+    new_source = sources / f"{new_name}.md"
+    assert new_source.exists()
+    assert b"\r\n" not in new_source.read_bytes()
+
+
 def test_commit_prepared_document_requires_serial_owner_lock(tmp_path):
     import pytest
 

tests/test_locks.py

@@ -3,8 +3,10 @@
 from __future__ import annotations
 
 import json
+import os
 import stat
 import threading
+from pathlib import Path
 
 import pytest
 
@@ -209,3 +211,78 @@ def test_reaper_does_not_follow_symlink_in_prepare_staging(tmp_path):
         pass
 
     assert (target / "keep.txt").exists()
+
+
+def test_reaper_survives_denied_unlink_of_loose_file_in_prepare_staging(tmp_path, monkeypatch):
+    """A loose file (not a dir) in staging/prepare/ whose unlink is denied must
+    not escape kb_lock and stall the whole KB.
+
+    On Windows an AV/indexer holding such a file makes os.unlink raise
+    PermissionError; ``missing_ok=True`` only swallows FileNotFoundError, so the
+    error currently propagates out of every exclusive-lock acquisition
+    (add/remove/recompile/chat). The directory branch uses rmtree and is safe;
+    this guards the asymmetric file branch.
+    """
+    openkb_dir = tmp_path / ".openkb"
+    prepare_root = openkb_dir / "staging" / "prepare"
+    prepare_root.mkdir(parents=True)
+    loose = prepare_root / "stray.dat"
+    loose.write_text("x", encoding="utf-8")
+
+    real_unlink = Path.unlink
+
+    def deny_unlink(self, *args, **kwargs):
+        if Path(self) == loose:
+            raise PermissionError(13, "Access is denied", str(self))
+        return real_unlink(self, *args, **kwargs)
+
+    monkeypatch.setattr(Path, "unlink", deny_unlink)
+
+    # Must not raise: the reap is best-effort and must not stall the lock holder.
+    with kb_ingest_lock(openkb_dir):
+        pass
+
+
+def test_reaper_self_heals_readonly_dir_under_prepare_staging(tmp_path, monkeypatch):
+    """A read-only file inside an orphaned prepare dir must be reaped, not left
+    behind forever.
+
+    shutil.copy2 preserves a read-only source's attribute into staging; on
+    Windows os.unlink denies a read-only file and rmtree(ignore_errors=True)
+    leaves the tree behind, so the orphan resurfaces as "Could not fully reap"
+    on every lock acquisition and never self-heals. POSIX deletes read-only
+    files fine, so we simulate the Windows denial: deny once, then let the
+    retry (after the handler clears the read-only bit) succeed.
+    """
+    openkb_dir = tmp_path / ".openkb"
+    prepare_root = openkb_dir / "staging" / "prepare"
+    prepare_root.mkdir(parents=True)
+    orphan = prepare_root / "000005-doc-11223344"
+    orphan.mkdir()
+    readonly = orphan / "readonly.md"
+    readonly.write_text("locked", encoding="utf-8")
+
+    real_unlink = os.unlink
+    attempts = {"n": 0}
+
+    def deny_once_then_succeed(*args, **kwargs):
+        # shutil's POSIX fast path calls os.unlink(entry_name, dir_fd=topfd) — a
+        # relative name — so identify the read-only file by basename. Deny the
+        # first attempt (Windows Access-denied on a read-only file), then let the
+        # handler's retry (after it clears the read-only bit) succeed.
+        name = args[0] if args else kwargs.get("path")
+        if Path(str(name)).name == readonly.name:
+            attempts["n"] += 1
+            if attempts["n"] == 1:
+                raise PermissionError(13, "Access is denied", str(name))
+        return real_unlink(*args, **kwargs)
+
+    # shutil.rmtree calls os.unlink internally; patching the shared os module
+    # makes its first attempt on the read-only file raise (Windows behaviour).
+    monkeypatch.setattr(os, "unlink", deny_once_then_succeed)
+
+    with kb_ingest_lock(openkb_dir):
+        pass
+
+    assert not orphan.exists()  # fully reaped, no residue to warn about forever
+    assert attempts["n"] == 2  # denied once, recovered on the retry