sched: sibling-distinct lock identity; reject shared-store overlaps (R4 wave 4)

src/wf_scheduling/ownership.py

@@ -11,14 +11,18 @@ Locking design (Windows-tested, per the store transaction boundary): one
 new module holding ``msvcrt.locking`` (Windows) / ``fcntl.flock`` (POSIX)
 on ``<composition root>/scheduler.lock``. No ad-hoc per-run lock files.
 
-One store composition has exactly one lock identity: the canonical
-nearest common ancestor of its store roots (see
-:func:`canonical_lock_root`). Any ancestor is NOT good enough — different
-ancestors yield different lock files and therefore independent OS locks,
-so ancestor containment can never prove exclusivity. Guards require the
-held lock's frozen identity to equal the composition identity; overlapping
-compositions that share no common ancestor are rejected instead of
-claimed safe.
+One store composition has exactly one lock identity: the composition root
+whose distinct schedule and run store directories are its direct children
+(see :func:`canonical_lock_root`). Any ancestor is NOT good enough —
+different ancestors yield different lock files and therefore independent
+OS locks, so ancestor containment can never prove exclusivity. Nor is a
+mere common ancestor enough: cross pairs that reuse one protected store
+with a different partner (shared schedule store, shared run store),
+nested pairs, and same-directory dual use would map to different lock
+files while covering the same store files, so they have no lock identity
+at all. Guards require the held lock's frozen identity to equal the
+composition identity; compositions that share no supported layout are
+rejected instead of claimed safe.
 
 The lock is acquired on the composition root that contains the protected
 schedule and run stores. Entry-point guards do not trust the caller to
@@ -44,6 +48,26 @@ class StartupRejected(Exception):
     """Scheduler startup rejected where locking is unsupported."""
 
 
+def describe_unsupported_layout(*store_roots: Path | str | None) -> str | None:
+    """Explain why store roots form no supportable lock composition.
+
+    Returns None when :func:`canonical_lock_root` yields an identity;
+    otherwise returns a message naming the supported layout (distinct
+    store directories under one composition root) so operators learn the
+    layout is rejected, not merely the lock. Guards raise this as
+    :class:`SecondOwnerError`: without one provable lock file, no owner
+    can claim exclusive ownership of the stores.
+    """
+    if canonical_lock_root(*store_roots) is not None:
+        return None
+    return (
+        "scheduler stores are not a supported composition: the schedule "
+        "and run stores must be distinct directories under one composition "
+        "root; shared-store cross pairs, nested pairs, and same-directory "
+        "dual use cannot prove exclusive ownership and are rejected"
+    )
+
+
 def canonical_store_path(path: Path | str) -> str:
     """Canonicalize a store path for ownership-coverage comparison.
 
@@ -62,13 +86,22 @@ def canonical_store_path(path: Path | str) -> str:
 def canonical_lock_root(*store_roots: Path | str | None) -> str | None:
     """Return the one canonical lock identity for a store composition.
 
-    The identity is the canonical nearest common ancestor of the store
-    roots; the lock file lives at ``<identity>/scheduler.lock``. Both the
-    held guard's frozen root and the guarded stores map through this
-    function, so exactly one lock file can authorize one composition.
-    Returns None when a root is missing or the roots share no common
-    ancestor (e.g. different drives): such compositions are unsupported
-    and guards reject them instead of claiming safety.
+    The supported layout is two or more distinct store directories that
+    are direct children of one composition root; the identity is that
+    parent and the lock file lives at ``<identity>/scheduler.lock``. Both
+    the held guard's frozen root and the guarded stores map through this
+    function, so exactly one lock file can authorize one composition, and
+    any two compositions covering the same store files map to the same
+    lock file (identical pairs) or are rejected (shared-store cross
+    pairs, which are not siblings). A single root maps to itself, so a
+    lock still proves the directory it was acquired on.
+    Returns None when a root is missing, when the roots are not distinct
+    siblings under one parent (nested pairs, same-directory dual use, or
+    ancestor-less roots on different drives), or when no roots are given:
+    such compositions are unsupported and guards reject them instead of
+    claiming safety. Nesting one composition's stores inside another
+    composition's store subtree without sharing the identical root stays
+    operator error outside the supported layout.
     """
     canonical: list[str] = []
     for store_root in store_roots:
@@ -77,10 +110,12 @@ def canonical_lock_root(*store_roots: Path | str | None) -> str | None:
         canonical.append(canonical_store_path(store_root))
     if not canonical:
         return None
-    try:
-        return os.path.commonpath(canonical)
-    except ValueError:
+    if len(canonical) == 1:
+        return canonical[0]
+    parents = {os.path.dirname(path) for path in canonical}
+    if len(parents) != 1 or len(set(canonical)) != len(canonical):
         return None
+    return parents.pop()
 
 
 class SchedulerOwnership:
@@ -126,10 +161,12 @@ class SchedulerOwnership:
         (:func:`canonical_lock_root`), not merely that the stores sit
         beneath some held ancestor: a lock on a parent directory is an
         independent OS lock from the composition's own lock and proves
-        nothing. An unrelated (even held) lock, a released lock, a
-        mutated handle, and rootless or ancestor-less stores are never
-        covered: guards reject those before any write or side effect
-        instead of trusting the caller's promise.
+        nothing, and a cross pair reusing one protected store with a
+        different partner has no lock identity at all. An unrelated
+        (even held) lock, a released lock, a mutated handle, and rootless
+        or non-sibling stores are never covered: guards reject those
+        before any write or side effect instead of trusting the caller's
+        promise.
         """
         if not self._locked or not store_roots:
             return False

src/wf_scheduling/poll.py

@@ -31,7 +31,11 @@ from wf_scheduling.history import (
     HistoryRecorder,
 )
 from wf_scheduling.models import PendingCandidate
-from wf_scheduling.ownership import SchedulerOwnership, SecondOwnerError
+from wf_scheduling.ownership import (
+    SchedulerOwnership,
+    SecondOwnerError,
+    describe_unsupported_layout,
+)
 from wf_scheduling.prepare import InvocationPreparer, PreparationRejected
 
 UTC = timezone.utc
@@ -146,10 +150,12 @@ class Scheduler:
         or administering schedules would risk double admission.
         """
         ownership = self.ownership
-        if ownership is None or not ownership.covers(
-            getattr(self.schedule_store, "root", None),
-            getattr(self.run_store, "root", None),
-        ):
+        sched_root = getattr(self.schedule_store, "root", None)
+        runs_root = getattr(self.run_store, "root", None)
+        unsupported = describe_unsupported_layout(sched_root, runs_root)
+        if unsupported is not None:
+            raise SecondOwnerError(unsupported)
+        if ownership is None or not ownership.covers(sched_root, runs_root):
             raise SecondOwnerError(
                 "scheduler ownership must cover the schedule and run stores "
                 "before polling or mutating schedules"

src/wf_scheduling/recovery.py

@@ -62,7 +62,11 @@ from wf_scheduling.history import (
     HistoryEntry,
     HistoryRecorder,
 )
-from wf_scheduling.ownership import SchedulerOwnership, SecondOwnerError
+from wf_scheduling.ownership import (
+    SchedulerOwnership,
+    SecondOwnerError,
+    describe_unsupported_layout,
+)
 
 UTC = timezone.utc
 
@@ -91,10 +95,12 @@ def recover(
     """Reconcile durable state after a restart without executing work."""
     from wf_artifacts.runs.models import StoredRunStatus
 
-    if ownership is None or not ownership.covers(
-        getattr(schedule_store, "root", None),
-        getattr(run_store, "root", None),
-    ):
+    sched_root = getattr(schedule_store, "root", None)
+    runs_root = getattr(run_store, "root", None)
+    unsupported = describe_unsupported_layout(sched_root, runs_root)
+    if unsupported is not None:
+        raise SecondOwnerError(unsupported)
+    if ownership is None or not ownership.covers(sched_root, runs_root):
         raise SecondOwnerError(
             "scheduler ownership must cover the schedule and run stores "
             "before recovery: an unowned recovery could abandon or "

tests/scheduling/test_lock_identity.py

@@ -205,3 +205,90 @@ def test_canonical_windows_path_handling_retained(tmp_path: Path) -> None:
         assert canonical_store_path("C:\\Temp\\SCHED") == canonical_store_path(
             "c:/temp/sched"
         )
+
+
+def test_non_sibling_store_pairs_have_no_lock_identity(tmp_path: Path) -> None:
+    """Only distinct siblings under one composition root share a lock file.
+
+    Cross pairs that reuse one protected store (shared schedule store or
+    shared run store), nested pairs, and same-directory dual use would map
+    to different lock files while covering the same store files, so they
+    have no lock identity at all: guards must reject them outright.
+    """
+    overlap = tmp_path / "overlap"
+    sched_store, run_store = _stores(overlap)
+    other_sched, other_runs = _stores(tmp_path / "other")
+    # Shared schedule store, different run store.
+    assert canonical_lock_root(sched_store.root, other_runs.root) is None
+    # Shared run store, different schedule store.
+    assert canonical_lock_root(other_sched.root, run_store.root) is None
+    # Nested pairs: one store inside the other.
+    assert canonical_lock_root(overlap, sched_store.root) is None
+    assert canonical_lock_root(sched_store.root, overlap) is None
+    # Same directory serving as both stores.
+    assert canonical_lock_root(sched_store.root, sched_store.root) is None
+
+
+def test_shared_schedule_store_cannot_gain_independent_authority(
+    tmp_path: Path,
+) -> None:
+    overlap = tmp_path / "overlap"
+    sched_store, run_store = _stores(overlap)
+    _, other_runs = _stores(tmp_path / "other")
+    owner = SchedulerOwnership(overlap, owner="owner").acquire()
+    intruder = SchedulerOwnership(tmp_path, owner="intruder").acquire()
+    try:
+        assert owner.covers(sched_store.root, run_store.root) is True
+        # The cross pair reuses the protected schedule store but maps to
+        # no lock identity, so no held lock can authorize it.
+        assert intruder.covers(sched_store.root, other_runs.root) is False
+        assert owner.covers(sched_store.root, other_runs.root) is False
+        now = datetime.now(UTC)
+        sched_recovery.recover(
+            schedule_store=sched_store,
+            run_store=run_store,
+            now=now,
+            ownership=owner,
+        )
+        before_runs = FileRunStore(overlap / "runs").list_runs()
+        with pytest.raises(SecondOwnerError):
+            sched_recovery.recover(
+                schedule_store=FileScheduleStore(overlap / "sched"),
+                run_store=FileRunStore(tmp_path / "other" / "runs"),
+                now=now,
+                ownership=intruder,
+            )
+        # Rejection happens before any write to either store.
+        assert FileRunStore(overlap / "runs").list_runs() == before_runs
+        assert FileRunStore(tmp_path / "other" / "runs").list_runs() == []
+        assert FileRunStore(tmp_path / "other" / "runs").list_admissions() == []
+    finally:
+        owner.release()
+        intruder.release()
+
+
+def test_shared_run_store_cannot_gain_independent_authority(
+    tmp_path: Path,
+) -> None:
+    overlap = tmp_path / "overlap"
+    sched_store, run_store = _stores(overlap)
+    other_sched, _ = _stores(tmp_path / "other")
+    owner = SchedulerOwnership(overlap, owner="owner").acquire()
+    intruder = SchedulerOwnership(tmp_path, owner="intruder").acquire()
+    try:
+        assert owner.covers(sched_store.root, run_store.root) is True
+        assert intruder.covers(other_sched.root, run_store.root) is False
+        assert owner.covers(other_sched.root, run_store.root) is False
+        now = datetime.now(UTC)
+        with pytest.raises(SecondOwnerError):
+            sched_recovery.recover(
+                schedule_store=FileScheduleStore(tmp_path / "other" / "sched"),
+                run_store=FileRunStore(overlap / "runs"),
+                now=now,
+                ownership=intruder,
+            )
+        assert FileRunStore(overlap / "runs").list_runs() == []
+        assert FileRunStore(overlap / "runs").list_admissions() == []
+    finally:
+        owner.release()
+        intruder.release()