From 721caca02309158ad837f7756988ec6f34d2ceb6 Mon Sep 17 00:00:00 2001 From: jay <1569941651@qq.com> Date: Sat, 17 Oct 2026 19:36:37 +0800 Subject: [PATCH] fix(sandbox): keep a concurrent quarantine fence when retire() races it (#6961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sandbox): keep a concurrent quarantine fence when retire() races it `SandboxQuarantine.retire()` unlinks every generation record and then calls `directory.rmdir()` unguarded. A peer fencing the same logical sandbox between the `iterdir()` and the `rmdir()` — a concurrent teardown of a replacement, which this caller's cross-worker lease does not exclude because it fences only its own logical ID — leaves a fresh record in the directory, so `rmdir()` raises ENOTEMPTY. That OSError escapes `_retire_absent_quarantine`'s teardown path and aborts the release flow on a fence a peer just wrote. Its sibling `retire_replaced_generations()` already guards the identical `rmdir` with `try/except OSError` for exactly this reason. Apply the same guard here: the fresh record proves the directory is live, so it is kept and teardown does not abort. Regression test replays the race deterministically by wrapping `Path.rmdir`; red on main (OSError: directory not empty), green with the guard. * docs(sandbox): scope the retire() fence guard to what it actually holds Review feedback from @willem-bd on #6961: two non-blocking docstring accuracy notes, both about the paragraph added by e62e0c3c. Docstring-only, no behaviour change. 1. The mechanism was described as if a peer teardown of a replacement were a different key. It is not. Per `_claim_ownership` / `_held_teardown_lease` the teardown lease is keyed by the logical ID and a replacement shares that ID, 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 at `rmdir` time: 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. Reworded to that effect, which also corrects the same sentence in e62e0c3c's message. 2. The paragraph read as if the listing → `rmdir` window were the only one. Added an explicit scope clause: the guard is best-effort rather than full fence preservation. 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`. Validation: `ruff check` and `ruff format --check` pass on the file and across `backend/`; the new regression test plus the existing quarantine suite are 9 passed; `scripts/check_agent_guidance.py` reports 46 AGENTS.md / 0 errors. --------- Co-authored-by: Willem Jiang --- .../community/aio_sandbox/quarantine.py | 29 +++++++++++- .../test_quarantine_retire_race_regression.py | 47 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 backend/tests/test_quarantine_retire_race_regression.py 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()