mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(sandbox): keep a concurrent quarantine fence when retire() races it (#6961)
* 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 <willem.jiang@gmail.com>
This commit is contained in:
parent
5b2063d8b9
commit
721caca023
@ -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
|
||||
|
||||
47
backend/tests/test_quarantine_retire_race_regression.py
Normal file
47
backend/tests/test_quarantine_retire_race_regression.py
Normal file
@ -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()
|
||||
Loading…
x
Reference in New Issue
Block a user