diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/quarantine.py b/backend/packages/harness/deerflow/community/aio_sandbox/quarantine.py index 9908bce300..1e9c5d8e9a 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/quarantine.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/quarantine.py @@ -100,10 +100,37 @@ class SandboxQuarantine: The provider must hold its local reservation and the cross-worker teardown lease throughout that proof and this removal. A successful delete request or failed health check alone does not prove absence. + + A peer may fence the same logical ID between the listing below and the + ``rmdir``. The teardown lease is keyed by that ID, and a replacement + shares it, so while the ``del:`` marker is held a peer's claim — and an + acquire-side ``take()`` for the replacement — are excluded. What is not + guaranteed is that the exclusion is still held when ``rmdir`` runs: it + is a TTL lease, not a lock, so the heartbeat can lose it mid-teardown + (the documented "Lost the teardown exclusion" path stops refreshing + while the stop continues) and the config schema permits TTLs below a + normal stop. A record written in that lapse proves the directory holds a + live generation, so the ``rmdir`` failure is swallowed and the directory + is kept — exactly as :meth:`retire_replaced_generations` keeps a live + generation's record: teardown must not abort on a fence a peer just + wrote. + + This is best-effort, not full fence preservation: it closes the listing + → ``rmdir`` window only. A peer record landing before the listing is + still unlinked along with the rest, and the unlink loop has unguarded + sibling windows of its own — a concurrent ``retire()`` can remove the + directory between the existence check and the listing + (``FileNotFoundError``), and on Windows a peer's ``mark()`` still + holding the fresh record open for its ``fsync`` raises + ``PermissionError``. """ directory = self._directory(info) if not self._exists(directory): return for record in directory.iterdir(): record.unlink() - directory.rmdir() + try: + directory.rmdir() + except OSError: + # A live or freshly fenced generation still has its record. + pass diff --git a/backend/tests/test_quarantine_retire_race_regression.py b/backend/tests/test_quarantine_retire_race_regression.py new file mode 100644 index 0000000000..7805764332 --- /dev/null +++ b/backend/tests/test_quarantine_retire_race_regression.py @@ -0,0 +1,47 @@ +"""Red/green regression: retire() must survive a peer fencing concurrently. + +`retire()` unlinks every record then calls `directory.rmdir()` unguarded. +Between `iterdir()` and `rmdir()`, a concurrent `mark()` — a peer fencing the +same logical sandbox — can create a fresh record, so `rmdir()` fails with +ENOTEMPTY and the exception escapes into `_retire_absent_quarantine`'s +teardown path. Its sibling `retire_replaced_generations()` guards the identical +`rmdir` with `try/except OSError` for exactly this reason; `retire()` does not. + +Deterministic: the race is replayed by wrapping `Path.rmdir` so a peer record +lands between the `iterdir()` and the `rmdir()` call. +""" + +from __future__ import annotations + +from pathlib import Path + +from deerflow.community.aio_sandbox.quarantine import SandboxQuarantine +from deerflow.community.aio_sandbox.sandbox_info import SandboxInfo + + +def test_retire_survives_a_peer_fencing_between_listing_and_rmdir(tmp_path, monkeypatch): + """A peer's mark() landing after retire()'s iterdir() must not raise out of retire(). + + The fence the peer wrote is the live one; the directory must simply be left + in place (not removed), so the peer's fence survives and teardown does not + abort with an unhandled OSError. + """ + store = SandboxQuarantine(tmp_path, "provisioner") + info = SandboxInfo("thread-sandbox", "http://sandbox", container_id="gen-1") + store.mark(info) + directory = tmp_path / store._key("provisioner") / store._key("thread-sandbox") + + real_rmdir = Path.rmdir + + def racing_rmdir(self): + if self == directory: + # Peer fences the live generation right after retire() listed the dir. + (self / store._key("gen-2")).touch() + return real_rmdir(self) + + monkeypatch.setattr(Path, "rmdir", racing_rmdir) + + store.retire(info) # must not raise + + assert directory.exists() + assert (directory / store._key("gen-2")).exists()