Merge pull request #2322 from mvalentsev/fix/2320-sync-unresolved-source

fix(sync): require an established absence before removing a drawer (#2320)

Author: igorls

CHANGELOG.md

@@ -8,6 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ## [Unreleased]
 
+### Bug Fixes
+
+- **`sync --apply` no longer deletes drawers whose source file it could not reach.** `_classify_drawer` separated keep from remove with one `Path.exists()`, and `missing` feeds `removable_ids`, so every state that answered no became a deletion. Of twelve source-file states driven through `sync_palace`, eight were deleted and four of those had really gone: two were a file on a volume that is not mounted, at an empty mount point or at one holding a committed `.gitkeep`, and two were a path that could not be walked at all. Three further states ended the whole run instead, dry run included, and two of those moved with the interpreter as `pathlib` stopped raising. End to end, a project mined from a mounted volume lost every drawer to one `sync --apply` while the volume was away, and the search came back empty once it returned. Removal now asks for corroboration rather than a probe: a file not at its path is removable only while the palace still sees a source of its own, a regular file, in that same directory. A deletion leaves the file's neighbours where they were; an unmounted volume takes all of them at once, and no errno separates the two. Both halves of the verdict are read again at the moment it is formed, because a volume can leave inside one pass and come back inside one. Everything uncorroborated lands in a new bucket, `unresolved`, never added to `removable_ids` and reported the way removals are: counted beside the other buckets, and its source files named in `unresolved_by_source` and printed, with the remainder stated when that list is cut at five. The price: a directory with no surviving known source corroborates nothing, so a file deleted alone from a one-file directory, or a directory's files deleted together, is kept and reported rather than pruned. (#2320)
+
 ---
 
 ## [3.8.0] — 2026-08-20

mempalace/cli.py

@@ -1138,6 +1138,7 @@ def cmd_sync(args):
     print(f"  Kept:           {report['kept']}")
     print(f"  Gitignored:     {report['gitignored']}  {removed_suffix}")
     print(f"  Missing:        {report['missing']}  {removed_suffix}")
+    print(f"  Unresolved:     {report['unresolved']}  (kept)")
     print(f"  No source:      {report['no_source']}  (kept)")
     print(f"  Out of scope:   {report['out_of_scope']}  (kept)")
 
@@ -1149,6 +1150,17 @@ def cmd_sync(args):
         for src, n in top:
             print(f"    {src}  ({n})")
 
+    if report["unresolved"]:
+        print("\n  Unresolved drawers are kept: nothing here could show their source file is gone.")
+        unresolved_sources = report.get("unresolved_by_source") or {}
+        if unresolved_sources:
+            top = sorted(unresolved_sources.items(), key=lambda kv: -kv[1])[:5]
+            for src, n in top:
+                print(f"    {src}  ({n})")
+            rest = len(unresolved_sources) - len(top)
+            if rest:
+                print(f"    and {rest} more source file(s)")
+
     if args.dry_run:
         if report["gitignored"] + report["missing"] > 0:
             print("\n  Re-run with --apply to commit these deletions.")

mempalace/service.py

@@ -364,6 +364,7 @@ def run_sync(payload: dict[str, Any]) -> dict[str, Any]:
     print(f"  Kept:           {report['kept']}")
     print(f"  Gitignored:     {report['gitignored']}  {removed_suffix}")
     print(f"  Missing:        {report['missing']}  {removed_suffix}")
+    print(f"  Unresolved:     {report['unresolved']}  (kept)")
     print(f"  No source:      {report['no_source']}  (kept)")
     print(f"  Out of scope:   {report['out_of_scope']}  (kept)")
 
@@ -375,6 +376,17 @@ def run_sync(payload: dict[str, Any]) -> dict[str, Any]:
         for src, n in top:
             print(f"    {src}  ({n})")
 
+    if report["unresolved"]:
+        print("\n  Unresolved drawers are kept: nothing here could show their source file is gone.")
+        unresolved_sources = report.get("unresolved_by_source") or {}
+        if unresolved_sources:
+            top = sorted(unresolved_sources.items(), key=lambda kv: -kv[1])[:5]
+            for src, n in top:
+                print(f"    {src}  ({n})")
+            rest = len(unresolved_sources) - len(top)
+            if rest:
+                print(f"    and {rest} more source file(s)")
+
     if dry_run:
         if report["gitignored"] + report["missing"] > 0:
             print("\n  Re-run with --apply to commit these deletions.")

mempalace/sync.py

@@ -12,9 +12,11 @@
 """
 
 import logging
+import os
+import stat as stat_module
 from collections import defaultdict
 from pathlib import Path
-from typing import Callable, Optional, TypedDict
+from typing import Callable, Final, Literal, Optional, TypedDict
 
 from .miner import is_gitignored, load_gitignore_matcher
 from .palace import (
@@ -34,12 +36,14 @@ class SyncReport(TypedDict):
     kept: int
     gitignored: int
     missing: int
+    unresolved: int
     no_source: int
     out_of_scope: int
     removed_drawers: int
     removed_closets: int
     dry_run: bool
     by_source: dict[str, int]
+    unresolved_by_source: dict[str, int]
 
 
 def _resolve_project_root(source_file: Path, project_roots: list) -> Optional[Path]:
@@ -96,12 +100,74 @@ def _is_registry_row(meta: dict, drawer_id: str) -> bool:
     return False
 
 
+# ``Final`` keeps these literal types rather than widening them to ``str``,
+# which the annotated return of ``_source_state`` needs.
+_STATE_PRESENT: Final = "present"
+_STATE_NOT_THERE: Final = "not_there"
+_STATE_UNKNOWN: Final = "unknown"
+
+
+def _source_state(src: Path) -> Literal["present", "not_there", "unknown"]:
+    """Report what one ``stat`` of ``src`` established, and nothing beyond it.
+
+    ``not_there`` means the path answered ``ENOENT``. On its own that is not
+    a reason to delete anything, because no errno separates "nothing is
+    here" from "this cannot be reached right now".  ``_uncopyable_reason``
+    in ``backups.py`` states the same rule for an operation that only leaves
+    a file out of a backup copy, and records that on Windows an unmapped
+    drive letter and an unreachable share both arrive as ``ENOENT`` as well.
+    Turning ``not_there`` into a removal is ``sync_palace``'s job, and it
+    needs a second reading to do it.
+
+    ``unknown`` is every other failure: a path that could not be walked at
+    all, which says nothing whatever about the leaf.
+    """
+    try:
+        os.stat(src)
+    except FileNotFoundError:
+        return _STATE_NOT_THERE
+    except (OSError, ValueError):
+        # ENOTDIR, ELOOP, EACCES, a share that stopped answering. ValueError
+        # is a path the platform cannot encode; the caller's ``resolve``
+        # raises on those first, so it should not arrive, and catching it
+        # costs nothing next to letting one drawer end the run.
+        return _STATE_UNKNOWN
+    return _STATE_PRESENT
+
+
+def _is_a_present_file(src: Path) -> bool:
+    """Whether ``src`` is a regular file that answers right now.
+
+    A witness has to be a file *in* the directory it speaks for, and
+    ``os.path.dirname`` alone does not establish that. A ``source_file``
+    whose last component is empty, ``.`` or ``..`` keys to that directory
+    while naming the directory itself, or its parent, and a directory
+    outlives the unmount that takes its contents away. ``tool_add_drawer``
+    stores whatever string its caller passed, so those spellings reach a
+    real palace without anything being corrupt.
+
+    Three answers collapse to ``False`` here on purpose: not a file, not
+    there, and could not be read. Only the first of them corroborates a
+    removal, so anything else has to keep the drawer.
+    """
+    try:
+        return stat_module.S_ISREG(os.stat(src).st_mode)
+    except (OSError, ValueError):
+        return False
+
+
 def _classify_drawer(
     meta: dict, matcher_cache: dict, project_roots: list, drawer_id: str = ""
 ) -> str:
     """Classify a drawer by its source_file metadata.
 
-    Returns one of: kept, gitignored, missing, no_source, out_of_scope.
+    Returns one of: kept, gitignored, absent, unresolved, no_source,
+    out_of_scope.
+
+    ``absent`` is provisional and never reaches a ``SyncReport``. It says
+    the file is not at its path, which is not yet a reason to remove the
+    drawer; ``sync_palace`` decides that, and settles every ``absent`` into
+    ``missing`` or ``unresolved`` once the whole pass is done.
     """
     # Defensive: main loop filters registry rows; this guards direct callers.
     if _is_registry_row(meta, drawer_id):
@@ -114,14 +180,25 @@ def _classify_drawer(
     src = Path(source_file)
     if not src.is_absolute():
         return "no_source"
-    src = src.resolve(strict=False)
+    try:
+        src = src.resolve(strict=False)
+    except (OSError, RuntimeError, ValueError):
+        # A symlink loop: up to 3.12 pathlib turns ELOOP into RuntimeError
+        # here, and 3.13 stopped raising and leaves it to the stat below. A
+        # path the platform cannot encode raises ValueError here instead,
+        # before any probe runs at all. Either way this drawer is one the
+        # run must survive, not end on.
+        return "unresolved"
 
     root = _resolve_project_root(src, project_roots)
     if root is None:
         return "out_of_scope"
 
-    if not src.exists():
-        return "missing"
+    state = _source_state(src)
+    if state == _STATE_UNKNOWN:
+        return "unresolved"
+    if state == _STATE_NOT_THERE:
+        return "absent"
 
     matchers = _ancestor_matchers(src, root, matcher_cache)
     if matchers and is_gitignored(src, matchers, is_dir=False):
@@ -172,12 +249,44 @@ def _auto_detect_project_roots(col, wing: Optional[str]) -> list:
         if not src.is_absolute():
             continue
         for parent in src.parents:
-            if (parent / ".git").exists() or (parent / ".gitignore").is_file():
+            if not _has_project_marker(parent):
+                continue
+            try:
                 roots.add(parent.resolve(strict=False))
-                break
+            except (OSError, RuntimeError, ValueError):
+                # The marker is here but this path will not resolve. Stop
+                # rather than let the climb register a higher ancestor as
+                # the root, which would widen what the run treats as in
+                # scope instead of narrowing it. No POSIX input reaches
+                # this: a marker that stats means its own path resolved.
+                # It is kept because the cost of being wrong about that on
+                # another platform is the whole run, not one drawer.
+                pass
+            break
     return sorted(roots, key=lambda p: (-len(str(p)), str(p)))
 
 
+def _has_project_marker(directory: Path) -> bool:
+    """Whether ``directory`` looks like a project root, without ever raising.
+
+    Each marker is probed on its own. An ancestor this process cannot walk
+    holds no marker it could have read anyway, and an unreadable ``.git``
+    must not hide a ``.gitignore`` beside it. A root that goes undetected
+    leaves its drawers ``out_of_scope``, which is the side that keeps them,
+    while a probe that escaped would end the run before a single drawer was
+    classified.
+    """
+    try:
+        if (directory / ".git").exists():
+            return True
+    except (OSError, RuntimeError, ValueError):
+        pass
+    try:
+        return (directory / ".gitignore").is_file()
+    except (OSError, RuntimeError, ValueError):
+        return False
+
+
 def _normalize_project_dirs(project_dirs) -> list:
     """Resolve and sort project dirs so deepest-prefix wins on first match."""
     resolved = [Path(p).resolve(strict=False) for p in project_dirs]
@@ -213,6 +322,34 @@ def sync_palace(
     Returns a SyncReport with bucket counts. Dry-run by default; pass
     dry_run=False to actually delete drawers and matching closets.
 
+    Only ``gitignored`` and ``missing`` are removed. A source file this
+    could not establish as deleted lands in ``unresolved`` and is counted,
+    printed and kept: an unmounted volume must not be read as a deletion.
+
+    A file that is not at its path reaches ``missing`` only when the palace
+    can still see a source file of its own in that same directory. A
+    deletion leaves the file's neighbours where they were; a volume that is
+    not mounted takes every one of them away at once, and there is no call
+    that tells those two apart from the file alone. Both halves of that are
+    read again when the verdict is formed rather than trusted from earlier
+    in the pass, since a volume can leave inside one pass and can come back
+    inside one.
+
+    Three limits of that reading are worth stating. A directory the palace
+    knows no *surviving* file in cannot corroborate anything, so a file
+    deleted on its own from a one-file directory is kept and reported, and
+    so is a whole directory's worth of files deleted together. A mount
+    point that also holds a mined file of its own does corroborate, since
+    that file survives the unmount: separating those needs the identity of
+    the filesystem each source was mined from, which is not recorded. And a
+    volume that leaves and returns between two adjacent ``stat`` calls is
+    not covered, because nothing spans two syscalls.
+
+    ``wing`` scopes the corroboration as well as the scan, since only that
+    wing's drawers are read. A wing-scoped run therefore keeps what a run
+    over the whole palace would prune, and every candidate the wider run
+    has is a candidate it has too.
+
     Holds ``mine_palace_lock`` for the whole call so the classify pass and
     the apply branch see the same drawer snapshot. Raises
     ``MineAlreadyRunning`` if another mine is in progress on this palace.
@@ -238,10 +375,12 @@ def sync_palace(
         "kept": 0,
         "gitignored": 0,
         "missing": 0,
+        "unresolved": 0,
         "no_source": 0,
         "out_of_scope": 0,
     }
     by_source: dict = defaultdict(int)
+    unresolved_by_source: dict = defaultdict(int)
     removable_ids: list = []
     removable_sources: set = set()
 
@@ -258,12 +397,22 @@ def sync_palace(
         # blocks concurrent writers and the loop is synchronous.
         classification_cache: dict = {}
 
+        # Candidate witnesses per directory, and the drawers whose source
+        # file is not at its path. The second list waits for the first to be
+        # complete: one drawer cannot say whether its directory lost one
+        # file or all of them. Every candidate is kept rather than the first
+        # one met, so the verdict does not turn on which drawer the pass
+        # happened to reach first.
+        live_dirs: dict = {}
+        not_there: list = []
+
         for drawer_id, meta in _iter_drawer_metadata(col, wing):
             counts["scanned"] += 1
             meta = meta or {}
             source_file = meta.get("source_file")
 
-            if _is_registry_row(meta, drawer_id):
+            registry_row = _is_registry_row(meta, drawer_id)
+            if registry_row:
                 bucket = "kept"
             elif source_file and source_file in classification_cache:
                 bucket = classification_cache[source_file]
@@ -272,19 +421,77 @@ def sync_palace(
                 if source_file:
                     classification_cache[source_file] = bucket
 
+            if bucket == "absent":
+                not_there.append((drawer_id, source_file))
+                continue
+
+            # A registry row is kept without the file being looked at, so it
+            # is not evidence that anything is on disk. The inner mapping is
+            # a set that keeps its insertion order: a file arrives once per
+            # chunk it was split into, and re-reading the same path a
+            # hundred times would be the whole cost of this pass.
+            if source_file and not registry_row and bucket in ("kept", "gitignored"):
+                live_dirs.setdefault(os.path.dirname(source_file), {})[source_file] = None
+
             counts[bucket] += 1
-            if bucket in ("gitignored", "missing"):
+            if bucket == "unresolved":
+                # Reachable only through a source_file that is a non-empty
+                # absolute path, like the removal below.
+                unresolved_by_source[source_file] += 1
+            if bucket == "gitignored":
                 removable_ids.append(drawer_id)
                 if source_file:
                     removable_sources.add(source_file)
                     by_source[source_file] += 1
 
+        # The directory keys are the metadata strings as written, while the
+        # classification above works on the resolved path, so two spellings
+        # of one directory need not meet: ``os.path.dirname`` leaves a
+        # ``/./`` segment alone, and a path through a symlink keeps the
+        # link's spelling. Not unifying them errs towards keeping. It cannot
+        # err the other way as long as a candidate is a file in the
+        # directory it is filed under, which is what ``_is_a_present_file``
+        # is there to require: a path whose last component is empty, ``.``
+        # or ``..`` is filed under the directory it names, or under that
+        # directory's parent, and neither is a file in it.
+        #
+        # Both halves of the verdict are read again here rather than trusted
+        # from earlier in the pass, and back to back rather than one of them
+        # early: a pass over a large palace runs for minutes, so a volume can
+        # leave or return inside one. Re-reading only the neighbour would let
+        # a volume that came back condemn every drawer read while it was
+        # away, exactly as re-reading neither would let one that left condemn
+        # every drawer read after it. ``any`` stops at the candidate that
+        # answered, so that reading is the one immediately before the
+        # source's own. Two syscalls are still two syscalls, and a volume
+        # that flaps between them is not covered by anything here.
+        settled: dict = {}
+        for drawer_id, source_file in not_there:
+            # ``absent`` is only reachable through a source_file that is a
+            # non-empty absolute path, so nothing here guards against a
+            # missing one, the same way the removal below does not.
+            if source_file not in settled:
+                witnesses = live_dirs.get(os.path.dirname(source_file), ())
+                settled[source_file] = (
+                    any(_is_a_present_file(Path(w)) for w in witnesses)
+                    and _source_state(Path(source_file)) == _STATE_NOT_THERE
+                )
+            established = settled[source_file]
+            counts["missing" if established else "unresolved"] += 1
+            if established:
+                removable_ids.append(drawer_id)
+                removable_sources.add(source_file)
+                by_source[source_file] += 1
+            else:
+                unresolved_by_source[source_file] += 1
+
         report: SyncReport = {
             **counts,
             "removed_drawers": 0,
             "removed_closets": 0,
             "dry_run": dry_run,
             "by_source": dict(by_source),
+            "unresolved_by_source": dict(unresolved_by_source),
         }
 
         if dry_run or not removable_ids: