diff --git a/src/wf_scheduling/ownership.py b/src/wf_scheduling/ownership.py index 2addc5d0..c95d6794 100644 --- a/src/wf_scheduling/ownership.py +++ b/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 ``/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 ``/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 ``/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 diff --git a/src/wf_scheduling/poll.py b/src/wf_scheduling/poll.py index 7bd79d6a..e93f946c 100644 --- a/src/wf_scheduling/poll.py +++ b/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" diff --git a/src/wf_scheduling/recovery.py b/src/wf_scheduling/recovery.py index d0cb39f8..ed3ccb9e 100644 --- a/src/wf_scheduling/recovery.py +++ b/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 " diff --git a/tests/scheduling/test_lock_identity.py b/tests/scheduling/test_lock_identity.py index 88cc935a..12c580b6 100644 --- a/tests/scheduling/test_lock_identity.py +++ b/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()