mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-27 16:37:55 +00:00
* fix(sandbox): stop multi-worker orphan reconcile from killing peer sandboxes Docker sandboxes are shared across gateway workers, but each worker kept its own in-memory warm pool. Startup reconciliation adopted every running container, so a peer idle reaper could destroy sandboxes another worker still owned and tool calls hit 502 / Connection refused. Add file-based ownership leases under sandbox-leases/, only adopt true orphans, refuse idle/replica/shutdown destroy while a foreign lease is live, and renew the lease on create/get/release/reclaim. Fixes #4206 * fix(sandbox): close lease fail-open, hot-path IO, and check→destroy race Address review of the multi-worker orphan lease (#4206): - read_lease returns None only for a genuinely-absent lease and raises (CorruptLeaseError/OSError) when a lease is unreadable or corrupt, so the ownership check fails closed instead of mistaking an unprovable peer lease for a free container. clear_lease still removes a stuck/corrupt file. - get() no longer renews the lease (blocking mkdir/fsync/os.replace on the event loop path used by ensure_sandbox_initialized_async); active leases are renewed off the event loop from the idle checker (_renew_active_leases). - The ownership check and container stop run under a per-sandbox flock guard (lease_ownership_guard); every lease write takes the same guard so a peer's touch cannot interleave with a destroy. Same-host multi-worker scope, not a multi-pod distributed lock. Also fixes the ruff format lint on the branch. Adds regression tests: corrupt and unreadable lease fail closed, a tests/blocking_io anchor keeping get() non-blocking on the event loop, and a peer-touch/destroy interleave test. * fix(sandbox): share container ownership across gateway instances Rework of the #4206 fix per review: ownership state is shared through a third-party service instead of being maintained per gateway instance, following the stream_bridge precedent (sandbox.ownership.type: memory | redis). The file lease and its same-host flock guard are deleted, not ported — they only covered workers on one host, while the deployment that hits #4206 is a load-balanced multi-instance gateway. A lease answers "who reaps this container", not "who may use it". Containers are deterministic per (user, thread), so consecutive turns legitimately land on different instances: take() transfers ownership on acquire, while claim() gates every adopt/reap path. Leases carry a state — own: or del: — so a takeover is refused against a teardown in progress. Without it an unconditional take() would overwrite a destroyer's claim and the peer's container stop would land on a sandbox the new owner had already handed to an agent. renew() distinguishes a lapsed lease from one a peer took; only the latter drops the sandbox. Collapsing them meant a Redis restart evicted every in-flight sandbox on every instance at once. Renewal runs on its own thread with a TTL derived from its interval, never from idle_timeout: renewal used to ride the idle checker, which does not start at idle_timeout: 0, so leases silently lapsed on a supported config. Ownership establishment is fail-closed: a sandbox whose ownership cannot be published is never handed out, and a just-created container is destroyed rather than leaked as an adoptable orphan. Every destroy path claims before untracking. The memory store is single-instance only and says so; the resolver reads app_config.stream_bridge and the env var in the bridge's own order, so deployments already using Redis get a redis ownership store without extra config. * fix(sandbox): wait out a recovery grace before adopting a keyless container An absent ownership lease meant two opposite things on two paths. Renewal reads it as LAPSED and re-establishes it: nobody took the lease, so the container is still ours. Reconciliation read the same absent key as "orphan" and adopted on sight. After the store loses its keys (a Redis restart without persistence, or eviction under maxmemory) every owner is alive and merely pre-renewal-tick. Whichever instance reconciled first therefore adopted every live container; each real owner's next renewal reported LOST and dropped a sandbox it was serving mid-turn, leaving it for the adopter to idle-destroy — #4206 through the back door, in the very case the LAPSED handling was added to make safe. Not limited to startup: an already-running instance hits the same window from the idle checker's periodic reconcile. _adoptable_after_grace requires an untracked container to be seen unowned across a full lease TTL before it can be adopted. That rebuilds the delay the state loss erased: a live owner republishes within one renewal interval, shorter than the TTL by construction, while a crashed owner never does, so its containers are still adopted one grace later rather than leaking. A republished lease resets the grace; a pausing-only timer would still expire over a live owner's lease. The peek is read-only — the atomic claim still gates adoption. The grace is skipped when the store cannot coordinate across processes: no peer can hold a lease such a store would show us, so single-instance deployments keep instant orphan cleanup, and a grace could not help a multi-worker gateway on memory anyway. * fix(sandbox): hold the teardown lease for as long as the container stop runs claim(..., for_destroy=True) wrote the del: marker with the ordinary lease TTL and nothing refreshed it. renew() extends only own: and deliberately reports a teardown as LOST, and the destroy paths drop the sandbox from the maps the renewal loop iterates — so a container stop that outlived the TTL let the marker lapse, a peer's take() succeeded against the still-running container, and the stop then landed on the turn that had just been handed it. That is the exact window the del: state exists to close, reopened by its own expiry. The two lease states alone never made the per-sandbox flock redundant, as I claimed when deleting it: a held lock cannot expire, a lease can. The exclusion has to be held deliberately rather than assumed to outlast the work it guards. _held_teardown_lease wraps both _backend.destroy() call sites and re-claims the marker every renewal_interval_seconds until the stop returns. No store change is needed: claim(for_destroy=True) already refreshes an existing del: marker on both backends. Reachable without an abnormal backend. The schema bounds only renewal_interval_seconds (> 0) and ttl_multiplier (>= 2), so a legal config puts the TTL below a normal container stop; and LocalContainerBackend._stop_container passes no timeout to subprocess.run, so a wedged daemon blocks unbounded even at the default 120s TTL. The TTL stays finite on purpose: the heartbeat dies with the process, so a destroyer that crashes mid-stop still releases the container one TTL later instead of marking it undestroyable forever. * fix(sandbox): hold the teardown lease on every del: stop, and pin the claims that had no test 90936b49 said `_held_teardown_lease` wrapped "both" `_backend.destroy()` call sites. There are three. `_drop_unhealthy_sandbox` marks `del:` and then blocks on the same unbounded stop, and it untracks *before* claiming, so `_renew_owned_leases` cannot see the id either — nothing refreshed the marker. Reproduced against a real redis: the peer's `take()` succeeds 1.0s into a 2.5s stop. Same window, third path. That miss came from the habit the rest of this commit addresses: a property asserted in prose, with no test that could falsify it. Auditing every load-bearing claim in this feature — AGENTS.md, the store docstrings, the provider's design comments — against the test that would go red turned up several more, each verified by mutating the code and watching the suite stay green. Tests that could not fail: - `test_reconcile_fails_closed_when_ownership_unknown` reached the grace gate, not the claim. A bare MagicMock answers `owner()` with a truthy mock, so the container read as peer-owned and deferred; `claim()` was never called. It stayed green with `_claim_ownership` failing open. Adding the grace ahead of the claim is what hollowed it out — inserting a gate can silently disarm the tests for the gate behind it. - `test_adoption_grace_restarts_when_a_live_owner_republishes` never distinguished reset from pause. Those diverge only on a *second* lapse, which it never drove, so it passed with the reset deleted. Claims with no test at all, each now pinned (mutation → red, per test): - `destroy()`, `_evict_oldest_warm`, `_reclaim_warm_pool_sandbox`, `_register_created_sandbox` and `shutdown()`'s warm loop were each the one untested sibling of an "every path does X" enumeration. `shutdown()` was never driven with a non-empty warm pool, so a loop bypassing the ownership claim — stopping a live peer's container on our exit — went unnoticed. - Renewal's unknown-is-not-lost rule, the single deliberate exception to fail-closed. Inverting it drops every active and warm sandbox on every instance the moment the store blinks. - Both hops of the stream-bridge redis inference. Deleting either left the suite green while every config.yaml-native multi-instance deployment silently fell back to memory — #4206 reopened on exactly the deployments the inference exists for. Claims narrowed instead, because they promised more than the code delivers: - "run against both backends ... cannot drift" — CI provisions no redis, so the merge gate runs the memory tier only and the Lua never executes there. - "Every destroy path claims before untracking" — `_drop_unhealthy_sandbox` untracks first, deliberately, under its `expected_info` TOCTOU guard. - "Atomic: concurrent claims from different instances cannot both succeed" — true via Lua on redis, vacuous on the single-instance memory store, and pinned by neither, since the contract suite drives sequential calls. A concurrency test against the memory store would make the claim look covered while the mechanism that carries it still never runs in CI. * fix(sandbox): release the teardown marker when a destroy() stop fails The three `del:`-marked stop paths disagreed on failure. `_destroy_warm_entry` releases on both outcomes and says why: the stop failed, so the container is probably still up, and a marker left behind refuses its own thread's `take()` until the TTL lapses. `_drop_unhealthy_sandbox` does the same. `destroy()` had no such guard — a raising backend propagated straight past `_release_ownership`, and the thread could not re-acquire for a full TTL. Fails safe rather than fatal: a stuck marker stops peers from touching the container, it is not the cross-instance kill. But the paths must agree, and this one is the odd one out. Release, then re-raise. Swallowing would be the easier symmetry with `_destroy_warm_entry`'s `return False`, but `destroy()` has no failure return and `shutdown()` logs per sandbox off the exception, so swallowing would silently narrow what callers can see. Found by comparing the three paths after @fancyboi999 asked for release to be handled "consistently with the other destroy paths" on the unhealthy path — which 0d2377b2 already does. This is the sibling that wasn't. * fix(deploy): bump chart config_version to 27 for sandbox.ownership config.example.yaml went to 27 with the new sandbox.ownership section, but the chart embeds its own copy and stayed at 26, so validate-chart failed. A bare bump: the chart already sets stream_bridge.type=redis, which is what resolve_ownership_config infers a redis ownership store from, so no field change is needed. * fix(sandbox): release the teardown lease from its heartbeat, not the caller `_held_teardown_lease` joined its heartbeat only briefly and the caller cleared the `del:` marker right after the stop. A refresh `claim` still in flight (`RedisOwnershipStore` had no socket timeout, so a round trip could block) could land *after* that release and rewrite `del:` on a container whose stop had already completed — refusing a fresh `take()` (or rolling back a fresh create) until the TTL. Move the release into the heartbeat's own `finally`, after its loop stops, so no refresh can run after it. The three destroy paths no longer release after the `with` (`destroy()`'s no-container branch still does, since no lease was held there). Bound every store round trip with a socket timeout so the in-flight refresh — and thus the deferred release — stays finite, and broaden the heartbeat's `except` so an unexpected error cannot strand the marker during a long stop. Also fold in the review follow-ups: stop re-resolving an already-resolved ownership config in the factory, document the Redis-outage-vs-TTL boundary in config.example.yaml, and add a tests/blocking_io anchor pinning that `release()`'s store round trip stays off the event loop. * fix(sandbox): refuse a non-destroy claim that would unwind our own teardown `claim(for_destroy=False)` against our own `del:` lease fell through and overwrote it with `own:`, cancelling a teardown that was already in flight. The container stop cannot be recalled, so downgrading the marker would let a `take()` hand out a container that is about to die -- #4206, self-inflicted. No caller does this today: the two non-destroy callers run against an absent key (the LAPSED re-claim) or an unowned one (post-grace reconcile). The contract has to forbid it rather than rely on that staying true. Fixed in both backends. The redis rule lives in Lua and the memory rule in Python, so fixing one only would let them drift silently -- and the shared contract suite is what is supposed to catch that drift, so it now covers this. Also adds a contention test for `claim`. The suite drove sequential calls only, so it pinned the exclusion predicate but not the atomicity that predicate depends on; eight instances now race for one container and exactly one must win. * fix(sandbox): bound the container stop so it cannot outlive its teardown lease `_stop_container` passed no `timeout` to `subprocess.run`, so a wedged container runtime blocks it forever. The `del:` marker is what keeps a peer from re-acquiring the container while the stop runs, but a marker is a lease and a lease can lapse: a store outage longer than the TTL frees it, a peer's `take()` succeeds against the still-running container, and the stop then lands on the turn that was just handed it -- the exact #4206 failure. The teardown heartbeat already covers the case where the store stays reachable. This bounds the worst case independently of the ownership layer, which is the point: it holds even when the ownership layer is the thing that failed. A timeout is not swallowed like a `CalledProcessError`. That error means the runtime answered "I could not stop it"; a timeout means we do not know, and the container is probably still running -- returning normally would let `_destroy_warm_entry` report a clean stop and drop the warm entry, leaking a running container nothing tracks. * fix(sandbox): exclude this instance's own reapers from its acquire path An ownership lease excludes peers and nothing else. `claim()` and `take()` both succeed against our own `own:` lease by design -- that is what lets a destroy path claim what it already owns -- so `del:` says nothing to this process's other threads. Meanwhile every reaper decides outside `_lock`, because a store round trip must not be held under the lock that guards every acquire. So each reaper acts on a decision its own acquire path may already have invalidated, and the store cannot see the difference. Six paths end in an irreversible act (a container stop, or closing a host-side client) on a decision made outside the lock. All six reproduce: _evict_oldest_warm re-checks warm membership, then releases the lock _reap_expired_warm no re-check at all _cleanup_idle_sandboxes re-verifies idle, then releases the lock _renew_owned_leases acts on a stale renew() -> LOST release() same staleness on its own refresh _drop_unhealthy_sandbox untracks before claiming, opening discovery Both warm reapers are a regression from the deferred pop this branch introduced: `WarmPoolLifecycleMixin` popped under the lock, so a reclaim's membership check failed and the race could not occur. Deferring the pop is still right (popping first loses the container on a refused claim), so the exclusion has to be made explicit instead. The idle path is pre-existing in shape, but this branch widened it from a few instructions to a network round trip by claiming ownership before untracking. Two guards, because the two directions want opposite answers: Reaping -- nothing may promote it. The reaper reserves the id, and every promote path refuses a reserved id exactly as it refuses a peer's `del:` (drop and cold-start). The "is this still reapable?" test travels with the reservation as a predicate and runs in the same critical section, because checking first and reserving second is the window, not a narrower version of it. Forgetting -- the peer legitimately wins, so the promote is what to detect. `_publish_ownership` bumps a per-id acquire epoch; the callers that decide from a store round trip snapshot it first, and the pop is skipped if it moved. Object identity cannot substitute: the reuse path re-publishes ownership while handing out the same tracked `AioSandbox`, so an identity check sees nothing and the pop closes a client mid-turn. `still_reapable` is required rather than defaulting to unconditional -- the safe default is the one that makes a new call site think about it. That diverges from the mixin hook, which is safe because this provider overrides both mixin callers, and loud rather than silent if those are ever dropped. Also closes a client leak on the discover path: "nothing to roll back" was true of the container but not of the HTTP client constructed before the publish, which the sibling create path already closes. The shared-store test view rebound `owner_id` outside the store's lock, so a concurrent claim could execute under the wrong id and read its own lease as a peer's. Serialized, so the heartbeat-hold tests stop flaking. * fix(sandbox): mark acquire intent before the ownership round trip A guard must become visible no later than the transition it guards. The acquire epoch cannot manage that for `take()`: the takeover is durable before `take()` returns -- redis has committed the SET while the reply is still in flight -- and the epoch can only be written afterwards. In that interval the store already says the container is ours while the epoch still reads as it did when a renewal decided `LOST`, so the stale forget walks through, drops the maps and closes the client the acquire is about to hand back. Acquire then returns an id the provider no longer tracks and `get()` answers `None` for the rest of the turn. `_publish_ownership` now publishes an intent mark under `_lock` before the round trip; the epoch keeps covering the other half, "an acquire completed since you decided". `_forget_lost_sandbox` honours the intent mark unconditionally rather than only when an epoch is supplied -- today's epoch-less callers cannot reach the window, but "no epoch" reading as "no guard" is how the next caller of a dangerous primitive gets written. The same invariant had four more instances, all reproduced: reuse returns a decision the forget already invalidated -- before the mark is set a `LOST` is both current and correct, so the forget legitimately runs and the entry reuse decided to hand out is gone. Re-check after publishing and fall through to discovery instead. reclaim installs an entry a reaper reserved after its check -- the warm entry is still visible during the stop, and the reaper's claim succeeds because reclaim's own take() just made the lease ours. Re-check likewise. the reservation was released before the entry was removed -- the pop belonged to the caller, leaving a gap where the container is stopped, the entry is still in `_warm_pool`, and nothing marks it. `_destroy_warm_entry` removes it itself, inside the reservation; the pop stays deferred relative to the stop, just not to the reservation. reconcile adopts a container this instance is tearing down -- adoption is a promote and needs the same reservation check as the others. Neither existing guard excludes it: the claim succeeds because the lease is ours, and on `memory` the recovery grace is skipped outright. The pre-round-trip checks in reuse and reclaim are kept as early-outs, since they skip a health check and a store round trip on a doomed entry, and are pinned to that job rather than to a correctness role they no longer hold. The teardown reservation predicate runs under `_lock`, so it must not touch the lock. Documented rather than engineered around: making the lock reentrant to tolerate it would trade a loud hang for a quiet class of re-entrancy bugs across the rest of the provider. * fix(sandbox): honor local teardown after ownership publish * fix(sandbox): clear a stale warm entry when an id becomes active Active and warm are exclusive states, and the two register paths were the only place that could hold both: they inserted into `_sandboxes` without popping `_warm_pool`, so one container ended up with two reapers. `_reap_expired_warm` judges an entry by its warm timestamp and never consults `_last_activity`, so it stops a container an agent is actively using while `_sandboxes` still hands out its client. Reachable because `_reconcile_orphans` adopts an untracked-but-running container into the warm pool inside the register's publish -> track window, and on the `memory` store it adopts on sight: `_adoptable_after_grace` short-circuits when `supports_cross_process` is False, so an id carrying this process's own lease reads as adoptable. That window is new to this branch -- on main the track was a single locked insert with nothing before it. Both register paths now pop the warm entry inside the same locked section that installs the active one. * fix(sandbox): harden ownership renewal teardown --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
513 lines
17 KiB
Python
513 lines
17 KiB
Python
"""Contract tests for the sandbox ownership store (#4206).
|
|
|
|
Every behavioural test here is **backend-agnostic**: it runs against each store
|
|
implementation through the same fixture, so the memory and redis backends cannot
|
|
drift apart on the semantics the provider depends on.
|
|
|
|
Redis coverage is opt-in and self-skipping, mirroring the stream-bridge
|
|
integration tier: point at a server with ``DEER_FLOW_TEST_REDIS_URL`` (defaults
|
|
to redis://localhost:6379/15 — DB 15 to avoid clobbering real data). There is no
|
|
fake-redis tier on purpose — the redis backend's exclusion lives in Lua scripts
|
|
that a hand-rolled fake would not execute, so a fake would pin the mock rather
|
|
than the script. When no server is reachable these skip and the memory backend
|
|
still covers the contract.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import threading
|
|
import time
|
|
import uuid
|
|
|
|
import pytest
|
|
|
|
from deerflow.community.aio_sandbox.ownership import (
|
|
MemoryOwnershipStore,
|
|
OwnershipBackendError,
|
|
RenewOutcome,
|
|
compute_lease_ttl,
|
|
generate_owner_id,
|
|
make_sandbox_ownership_store,
|
|
resolve_ownership_config,
|
|
)
|
|
from deerflow.config.sandbox_config import SandboxOwnershipConfig
|
|
from deerflow.config.stream_bridge_config import StreamBridgeConfig
|
|
|
|
REDIS_TEST_URL = os.environ.get("DEER_FLOW_TEST_REDIS_URL", "redis://localhost:6379/15")
|
|
|
|
|
|
def _redis_available() -> bool:
|
|
try:
|
|
import redis
|
|
except ImportError:
|
|
return False
|
|
try:
|
|
client = redis.Redis.from_url(REDIS_TEST_URL, socket_connect_timeout=0.5)
|
|
try:
|
|
client.ping()
|
|
finally:
|
|
client.close()
|
|
return True
|
|
except Exception:
|
|
return False
|
|
|
|
|
|
requires_redis = pytest.mark.skipif(not _redis_available(), reason=f"Redis not reachable at {REDIS_TEST_URL}")
|
|
|
|
|
|
class _StoreFactory:
|
|
"""Builds stores for one backend that all share the same keyspace."""
|
|
|
|
def __init__(self, kind: str, ttl_seconds: float = 60.0):
|
|
self.kind = kind
|
|
self.ttl = ttl_seconds
|
|
self._shared_leases: dict = {}
|
|
self._key_prefix = f"deerflow:test:{uuid.uuid4().hex}"
|
|
self._made: list = []
|
|
|
|
def make(self, owner_id: str, *, ttl_seconds: float | None = None):
|
|
ttl = self.ttl if ttl_seconds is None else ttl_seconds
|
|
if self.kind == "memory":
|
|
store = MemoryOwnershipStore(owner_id=owner_id, ttl_seconds=ttl)
|
|
# Share one dict so separate store objects model separate instances
|
|
# talking to one backend, as redis clients naturally do.
|
|
store._leases = self._shared_leases
|
|
else:
|
|
from deerflow.community.aio_sandbox.ownership.redis import RedisOwnershipStore
|
|
|
|
store = RedisOwnershipStore(
|
|
owner_id=owner_id,
|
|
redis_url=REDIS_TEST_URL,
|
|
ttl_seconds=ttl,
|
|
key_prefix=self._key_prefix,
|
|
)
|
|
self._made.append(store)
|
|
return store
|
|
|
|
def cleanup(self):
|
|
if self.kind == "redis" and self._made:
|
|
client = self._made[0]._redis
|
|
for key in client.scan_iter(f"{self._key_prefix}:*"):
|
|
client.delete(key)
|
|
for store in self._made:
|
|
store.close()
|
|
|
|
|
|
@pytest.fixture(params=["memory", pytest.param("redis", marks=[requires_redis, pytest.mark.integration])])
|
|
def stores(request):
|
|
factory = _StoreFactory(request.param)
|
|
try:
|
|
yield factory
|
|
finally:
|
|
factory.cleanup()
|
|
|
|
|
|
# ── The #4206 invariant: a peer cannot claim a live owner's container ─────────
|
|
|
|
|
|
def test_claim_is_exclusive_across_instances(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
|
|
assert a.claim("s1") is True
|
|
assert b.claim("s1") is False, "a peer claimed a container A already owns — #4206"
|
|
assert a.owner("s1") == "A"
|
|
|
|
|
|
def test_failed_claim_does_not_steal_the_lease(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
b.claim("s1")
|
|
|
|
assert a.owner("s1") == "A"
|
|
|
|
|
|
def test_claim_refreshes_our_own_lease(stores):
|
|
a = stores.make("A")
|
|
assert a.claim("s1") is True
|
|
assert a.claim("s1") is True
|
|
|
|
|
|
def test_claim_succeeds_once_a_lease_expires(stores):
|
|
"""The crash path: a dead owner's container must become adoptable."""
|
|
a = stores.make("A", ttl_seconds=0.2)
|
|
b = stores.make("B")
|
|
assert a.claim("s1") is True
|
|
assert b.claim("s1") is False
|
|
|
|
time.sleep(0.35)
|
|
|
|
assert a.owner("s1") is None
|
|
assert b.claim("s1") is True
|
|
|
|
|
|
# ── take(): ownership transfers when a thread moves instance ─────────────────
|
|
|
|
|
|
def test_take_transfers_ownership_from_a_live_peer(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
assert b.take("s1") is True
|
|
|
|
assert b.owner("s1") == "B"
|
|
|
|
|
|
def test_take_makes_the_previous_owners_renew_report_lost(stores):
|
|
"""How the previous owner learns to stop tracking the container."""
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
b.take("s1")
|
|
|
|
assert a.renew("s1") is RenewOutcome.LOST
|
|
assert b.renew("s1") is RenewOutcome.RENEWED
|
|
|
|
|
|
# ── The destroy window: take() must not overrun a teardown ──────────────────
|
|
|
|
|
|
def test_take_is_refused_while_a_peer_is_destroying(stores):
|
|
"""#4206's remaining window: an unconditional take would overwrite a
|
|
destroyer's claim, and the peer's container stop would then land on a
|
|
container this instance had already handed to an agent."""
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
assert a.claim("s1", for_destroy=True) is True
|
|
|
|
assert b.take("s1") is False, "took over a container that is being destroyed"
|
|
|
|
|
|
def test_take_is_allowed_once_the_teardown_marker_is_released(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1", for_destroy=True)
|
|
a.release("s1")
|
|
|
|
assert b.take("s1") is True
|
|
|
|
|
|
def test_a_destroyers_own_claim_is_idempotent(stores):
|
|
a = stores.make("A")
|
|
assert a.claim("s1", for_destroy=True) is True
|
|
assert a.claim("s1", for_destroy=True) is True
|
|
|
|
|
|
def test_claim_for_destroy_is_still_refused_against_a_peer(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
assert b.claim("s1", for_destroy=True) is False
|
|
|
|
|
|
def test_a_stale_teardown_marker_expires(stores):
|
|
"""A destroyer that dies mid-stop must not block the container forever."""
|
|
a = stores.make("A", ttl_seconds=0.2)
|
|
b = stores.make("B")
|
|
a.claim("s1", for_destroy=True)
|
|
assert b.take("s1") is False
|
|
|
|
time.sleep(0.35)
|
|
|
|
assert b.take("s1") is True
|
|
|
|
|
|
def test_renew_does_not_keep_a_teardown_alive(stores):
|
|
"""A teardown marker is not a normal lease; renewal must not extend it."""
|
|
a = stores.make("A")
|
|
a.claim("s1", for_destroy=True)
|
|
|
|
assert a.renew("s1") is RenewOutcome.LOST
|
|
|
|
|
|
# ── renew(): distinguishes lapsed from stolen ───────────────────────────────
|
|
|
|
|
|
def test_renew_reports_lapsed_not_lost_for_an_expired_lease(stores):
|
|
"""Collapsing these is what dropped every live sandbox on a Redis restart.
|
|
|
|
Nobody took the lease — it is simply gone — so the caller is free to
|
|
re-establish it. Reporting LOST would make the provider evict a container it
|
|
is actively using.
|
|
"""
|
|
a = stores.make("A", ttl_seconds=0.2)
|
|
a.claim("s1")
|
|
|
|
time.sleep(0.35)
|
|
|
|
assert a.renew("s1") is RenewOutcome.LAPSED
|
|
assert a.owner("s1") is None
|
|
|
|
|
|
def test_renew_does_not_reacquire_on_its_own(stores):
|
|
"""The caller decides; renew() never silently re-takes."""
|
|
a = stores.make("A", ttl_seconds=0.2)
|
|
a.claim("s1")
|
|
time.sleep(0.35)
|
|
|
|
a.renew("s1")
|
|
|
|
assert a.owner("s1") is None, "renew() re-acquired a lapsed lease by itself"
|
|
|
|
|
|
def test_renew_extends_the_lease(stores):
|
|
a = stores.make("A", ttl_seconds=0.4)
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
for _ in range(3):
|
|
time.sleep(0.15)
|
|
assert a.renew("s1") is RenewOutcome.RENEWED
|
|
|
|
assert b.claim("s1") is False, "a renewed lease must keep peers out"
|
|
|
|
|
|
def test_renew_of_unknown_sandbox_is_lapsed(stores):
|
|
a = stores.make("A")
|
|
assert a.renew("never-claimed") is RenewOutcome.LAPSED
|
|
|
|
|
|
# ── release(): only ever drops our own ──────────────────────────────────────
|
|
|
|
|
|
def test_release_frees_the_container_for_a_peer(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
a.release("s1")
|
|
|
|
assert a.owner("s1") is None
|
|
assert b.claim("s1") is True
|
|
|
|
|
|
def test_release_does_not_clear_a_peers_lease(stores):
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
a.claim("s1")
|
|
|
|
b.release("s1")
|
|
|
|
assert a.owner("s1") == "A", "B released a lease it does not hold"
|
|
|
|
|
|
def test_release_of_unowned_sandbox_is_a_noop(stores):
|
|
a = stores.make("A")
|
|
a.release("never-claimed")
|
|
|
|
|
|
# ── owner(): read-only ──────────────────────────────────────────────────────
|
|
|
|
|
|
def test_owner_returns_none_when_unowned(stores):
|
|
a = stores.make("A")
|
|
assert a.owner("nobody") is None
|
|
|
|
|
|
def test_owner_does_not_take_ownership(stores):
|
|
"""Unlike claim(), a read must leave ownership untouched."""
|
|
a = stores.make("A")
|
|
b = stores.make("B")
|
|
|
|
assert b.owner("s1") is None
|
|
|
|
assert a.claim("s1") is True, "owner() took the lease as a side effect"
|
|
|
|
|
|
# ── Factory / config ────────────────────────────────────────────────────────
|
|
|
|
|
|
def test_memory_store_declares_it_cannot_see_peers():
|
|
store = make_sandbox_ownership_store(SandboxOwnershipConfig(type="memory"), owner_id="A")
|
|
assert store.supports_cross_process is False
|
|
|
|
|
|
def test_default_config_is_memory():
|
|
store = make_sandbox_ownership_store(None, owner_id="A")
|
|
assert isinstance(store, MemoryOwnershipStore)
|
|
|
|
|
|
def test_unknown_type_raises():
|
|
config = SandboxOwnershipConfig()
|
|
object.__setattr__(config, "type", "bogus")
|
|
with pytest.raises(ValueError, match="Unknown sandbox ownership type"):
|
|
make_sandbox_ownership_store(config, owner_id="A")
|
|
|
|
|
|
def test_ttl_derives_from_renewal_interval_not_idle_timeout():
|
|
"""The coupling that let leases lapse under idle_timeout: 0 must stay broken."""
|
|
config = SandboxOwnershipConfig(renewal_interval_seconds=30, ttl_multiplier=4)
|
|
assert compute_lease_ttl(config) == 120
|
|
|
|
|
|
def test_ttl_tolerates_missed_renewals():
|
|
"""A single slow renewal cycle must not expire a live owner's lease."""
|
|
config = SandboxOwnershipConfig()
|
|
assert compute_lease_ttl(config) > config.renewal_interval_seconds * 2
|
|
|
|
|
|
def test_ttl_multiplier_below_two_is_rejected():
|
|
with pytest.raises(ValueError):
|
|
SandboxOwnershipConfig(ttl_multiplier=1.0)
|
|
|
|
|
|
def test_owner_ids_are_unique_per_instance():
|
|
"""Two workers on one host must not share an owner id."""
|
|
assert generate_owner_id() != generate_owner_id()
|
|
|
|
|
|
def test_stream_bridge_redis_env_implies_redis_ownership(monkeypatch):
|
|
"""A deployment already using redis for the stream bridge is multi-instance.
|
|
|
|
Defaulting it to memory ownership would leave #4206 open on exactly the
|
|
deployments that hit it.
|
|
"""
|
|
monkeypatch.setenv("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", "redis://somewhere:6379/0")
|
|
resolved = resolve_ownership_config(None)
|
|
assert resolved.type == "redis"
|
|
assert resolved.redis_url == "redis://somewhere:6379/0"
|
|
|
|
|
|
def test_stream_bridge_redis_in_config_yaml_implies_redis_ownership(monkeypatch):
|
|
"""The config.yaml-native way of using redis must trigger the inference too.
|
|
|
|
The stream bridge's own resolver reads `app_config.stream_bridge` *before*
|
|
the env var, so inferring only from the env var missed every deployment that
|
|
configures the bridge in config.yaml — i.e. exactly the multi-instance
|
|
deployments this inference exists for.
|
|
"""
|
|
monkeypatch.delenv("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", raising=False)
|
|
monkeypatch.delenv("DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL", raising=False)
|
|
|
|
resolved = resolve_ownership_config(None, stream_bridge=StreamBridgeConfig(type="redis", redis_url="redis://in-yaml:6379/0"))
|
|
|
|
assert resolved.type == "redis"
|
|
assert resolved.redis_url == "redis://in-yaml:6379/0"
|
|
|
|
|
|
def test_memory_stream_bridge_does_not_imply_redis_ownership(monkeypatch):
|
|
"""The other direction: a single-process bridge must not force redis."""
|
|
monkeypatch.delenv("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", raising=False)
|
|
monkeypatch.delenv("DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL", raising=False)
|
|
|
|
resolved = resolve_ownership_config(None, stream_bridge=StreamBridgeConfig(type="memory"))
|
|
|
|
assert resolved.type == "memory"
|
|
|
|
|
|
def test_explicit_config_wins_over_env(monkeypatch):
|
|
monkeypatch.setenv("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", "redis://somewhere:6379/0")
|
|
resolved = resolve_ownership_config(SandboxOwnershipConfig(type="memory"))
|
|
assert resolved.type == "memory"
|
|
|
|
|
|
def test_no_env_defaults_to_memory(monkeypatch):
|
|
monkeypatch.delenv("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", raising=False)
|
|
monkeypatch.delenv("DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL", raising=False)
|
|
assert resolve_ownership_config(None).type == "memory"
|
|
|
|
|
|
# ── Redis-specific: failure surfaces as OwnershipBackendError ───────────────
|
|
|
|
|
|
def test_redis_backend_error_is_wrapped_not_leaked():
|
|
"""Callers fail closed on OwnershipBackendError; a raw RedisError would escape that.
|
|
|
|
Deliberately **not** marked `integration`/`requires_redis`: it points at a
|
|
dead port, so it needs no server — only the `redis` package, which is pinned
|
|
in the dev group. Gating it on a live Redis would mean the fail-closed
|
|
contract was never exercised in CI, which is the one place it matters.
|
|
"""
|
|
from deerflow.community.aio_sandbox.ownership.redis import RedisOwnershipStore
|
|
|
|
store = RedisOwnershipStore(
|
|
owner_id="A",
|
|
redis_url="redis://127.0.0.1:1/0", # nothing listening
|
|
ttl_seconds=60,
|
|
key_prefix=f"deerflow:test:{uuid.uuid4().hex}",
|
|
)
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.claim("s1")
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.claim("s1", for_destroy=True)
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.take("s1")
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.renew("s1")
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.release("s1")
|
|
with pytest.raises(OwnershipBackendError):
|
|
store.owner("s1")
|
|
|
|
|
|
def test_non_destroy_claim_does_not_unwind_our_own_teardown(stores):
|
|
"""A `for_destroy=False` claim must not downgrade our own `del:` marker.
|
|
|
|
The stop it marks is already in flight and cannot be recalled, so turning the
|
|
lease back into `own:` would let a `take()` hand out a container that is
|
|
about to die — the #4206 failure, self-inflicted. No caller does this today
|
|
(the non-destroy callers run against an absent or unowned key), but the
|
|
contract has to forbid it rather than rely on that staying true.
|
|
|
|
Runs against both backends on purpose: the redis rule lives in Lua and the
|
|
memory rule in Python, so a fix applied to one only would drift silently.
|
|
"""
|
|
a = stores.make("A")
|
|
|
|
assert a.claim("s1", for_destroy=True) is True
|
|
assert a.claim("s1") is False, "a non-destroy claim unwound our own in-flight teardown"
|
|
# Still a teardown: the marker survived the refused claim intact.
|
|
assert a.renew("s1") is RenewOutcome.LOST
|
|
b = stores.make("B")
|
|
assert b.take("s1") is False, "the teardown marker stopped refusing takes"
|
|
|
|
# Refreshing the teardown itself is still allowed — that is the heartbeat.
|
|
assert a.claim("s1", for_destroy=True) is True
|
|
|
|
|
|
def test_concurrent_claims_serialize_to_one_winner(stores):
|
|
"""The exclusion must hold under contention, not just in sequence.
|
|
|
|
The rest of this suite drives sequential calls, so it pins the predicate and
|
|
not the atomicity the predicate depends on — redis carries it in Lua, the
|
|
memory store in a process lock. Eight instances race for one container; the
|
|
read-modify-write is only atomic if exactly one wins.
|
|
"""
|
|
barrier = threading.Barrier(8)
|
|
results = {}
|
|
lock = threading.Lock()
|
|
|
|
def contend(name):
|
|
store = stores.make(name)
|
|
barrier.wait(timeout=5)
|
|
won = store.claim("s1")
|
|
with lock:
|
|
results[name] = won
|
|
|
|
threads = [threading.Thread(target=contend, args=(f"W{i}",), daemon=True) for i in range(8)]
|
|
for t in threads:
|
|
t.start()
|
|
for t in threads:
|
|
t.join(timeout=10)
|
|
assert not t.is_alive(), "a contending claim never finished"
|
|
|
|
winners = [name for name, won in results.items() if won]
|
|
assert len(winners) == 1, f"claim is not atomic under contention: {len(winners)} winners ({winners})"
|
|
|
|
|
|
@pytest.mark.integration
|
|
@requires_redis
|
|
def test_redis_store_declares_cross_process_support():
|
|
from deerflow.community.aio_sandbox.ownership.redis import RedisOwnershipStore
|
|
|
|
store = RedisOwnershipStore(owner_id="A", redis_url=REDIS_TEST_URL, ttl_seconds=60, key_prefix=f"deerflow:test:{uuid.uuid4().hex}")
|
|
try:
|
|
assert store.supports_cross_process is True
|
|
finally:
|
|
store.close()
|