From 5eb59cb130039fde024798f017dbe33aa8ab5724 Mon Sep 17 00:00:00 2001 From: Aari Date: Tue, 21 Jul 2026 09:09:40 +0800 Subject: [PATCH] fix(sandbox): stop multi-worker orphan reconcile from killing peer sandboxes (#4221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- backend/AGENTS.md | 24 +- .../aio_sandbox/aio_sandbox_provider.py | 1011 ++++++- .../community/aio_sandbox/local_backend.py | 22 +- .../aio_sandbox/ownership/__init__.py | 23 + .../community/aio_sandbox/ownership/base.py | 188 ++ .../aio_sandbox/ownership/factory.py | 108 + .../community/aio_sandbox/ownership/memory.py | 106 + .../community/aio_sandbox/ownership/redis.py | 214 ++ .../harness/deerflow/config/sandbox_config.py | 52 + .../tests/blocking_io/test_aio_sandbox_get.py | 155 ++ .../tests/blocking_io/test_sandbox_release.py | 141 + .../tests/test_aio_sandbox_local_backend.py | 41 + backend/tests/test_aio_sandbox_provider.py | 38 +- .../test_sandbox_orphan_reconciliation.py | 2335 ++++++++++++++++- backend/tests/test_sandbox_ownership_store.py | 512 ++++ config.example.yaml | 48 + scripts/detect_uv_extras.py | 6 + 17 files changed, 4936 insertions(+), 88 deletions(-) create mode 100644 backend/packages/harness/deerflow/community/aio_sandbox/ownership/__init__.py create mode 100644 backend/packages/harness/deerflow/community/aio_sandbox/ownership/base.py create mode 100644 backend/packages/harness/deerflow/community/aio_sandbox/ownership/factory.py create mode 100644 backend/packages/harness/deerflow/community/aio_sandbox/ownership/memory.py create mode 100644 backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py create mode 100644 backend/tests/blocking_io/test_aio_sandbox_get.py create mode 100644 backend/tests/blocking_io/test_sandbox_release.py create mode 100644 backend/tests/test_sandbox_ownership_store.py diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 57ca6071b..7f2e60032 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -372,7 +372,29 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti **Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASS*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved. **Implementations**: - `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Legacy global-custom mounts are gated by the same user-scoped skill discovery rule used for prompt/list visibility; providers must not infer visibility from raw directory presence alone. -- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths. Legacy global-custom mounts follow the same shared visibility helper as local and remote providers. +- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths — it never touches the ownership store (that would be blocking IO on the event loop); ownership is published on acquire/reclaim and refreshed off the event loop by the dedicated renewal thread (`_renew_owned_leases`). Legacy global-custom mounts follow the same shared visibility helper as local and remote providers. + - **Cross-instance ownership store** (`aio_sandbox/ownership/`, #4206): gateway instances sharing a container backend coordinate container ownership through a pluggable lease store, selected by `sandbox.ownership.type` (`memory` | `redis`) and resolved like `stream_bridge` (`factory.py`, lazy per-branch import, `redis` optional extra, `DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL` env escape hatch; a set `DEER_FLOW_STREAM_BRIDGE_REDIS_URL` implies a multi-instance deployment and infers `redis`). `memory` is single-instance only and declares `supports_cross_process = False`. + - **A lease answers "who reaps this container", not "who may use it".** That splits the interface in two: `take()` transfers ownership on the **acquire** path (a container is deterministic per user/thread, so consecutive turns legitimately land on different instances — a conditional claim there would strand the thread until the previous lease expired), while `claim()` succeeds only if the container is unowned or already ours and gates every **adopt/reap** path. `release()` never clears a peer's lease. + - **A lease carries a state, and that is what makes the destroy window safe.** `own:` = responsible for this container; `del:` = tearing it down (`claim(..., for_destroy=True)`). `take()` is refused against a `del:` lease, so a container cannot be re-acquired between a destroy path's claim and its container stop. Without the two states an unconditional `take()` would silently overwrite the destroyer's claim and the peer's stop would land on a container the new owner had already handed to an agent — i.e. #4206 again. That pairing is what replaced the previous same-host `flock` guard, which is gone; Redis makes the scope genuinely multi-instance instead of same-host. A destroyer that dies mid-stop leaves a `del:` marker that lapses with the TTL. On the acquire path a refused take raises `SandboxBeingDestroyedError`: the reuse/reclaim paths drop the container and cold-start, and the discover path propagates (falling through to create would collide with the not-yet-removed container name). + - **The `del:` state has to be *held* for the stop, not just written before it.** The two states alone do not make `flock` redundant — a held lock cannot expire, whereas a lease can, and `claim(..., for_destroy=True)` writes the marker with the ordinary lease TTL. Nothing else refreshes it: `renew()` extends only `own:` and deliberately reports a teardown as `LOST`, and the destroy paths drop the sandbox from the maps `_renew_owned_leases` 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 — the exact window `del:` exists to close, reopened by its own expiry. `_held_teardown_lease` wraps **every** `del:`-marked stop — `_destroy_warm_entry`, `destroy()`, and `_drop_unhealthy_sandbox` — and re-claims the marker every `renewal_interval_seconds` until the stop returns. `_drop_unhealthy_sandbox` needs it most: it untracks *before* claiming (under its `expected_info` TOCTOU guard), so `_renew_owned_leases` cannot see the id either. **The final release is the heartbeat's own last act, not the caller's** — a refresh `claim` still in flight when the context exits (the store's socket timeout bounds it, but it can be mid-round-trip) would otherwise land *after* a caller-side release and rewrite `del:` on a container whose stop already completed, stranding a fresh `take()` (or rolling back a fresh create) until the TTL. Releasing from inside the heartbeat, after its loop stops, sequences the release strictly after the last refresh, so no claim can follow it; the context join is bounded and, on a genuine wedge, defers the release to that thread rather than clearing the marker itself. This covers a **failed** stop too (the container is probably still up, and a marker left behind refuses its own thread's `take()` until the TTL lapses); `destroy()` still lets the error propagate out of the `with` — `shutdown()` logs per sandbox off it. `RedisOwnershipStore` sets a `socket_timeout` so no store round trip — and so no heartbeat refresh — can block unbounded, keeping that deferred release finite. This needs no 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. `LocalContainerBackend._stop_container` now passes a `timeout` to `subprocess.run` (`_STOP_TIMEOUT_SECONDS`) so a wedged daemon cannot block unbounded — that bounds the residual window independently of the ownership layer, for the case where the `del:` marker lapses mid-stop (a store outage longer than the TTL) and the stop then lands on a container a peer has been handed. A timed-out stop propagates rather than being swallowed like a `CalledProcessError`: the container is probably still running, so reporting a clean stop would drop the warm entry and leak it. 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. Raising a separate teardown TTL instead would only be sufficient if it were bounded above every backend's real stop deadline. + - **Fail-closed both directions.** Establishment: a sandbox whose ownership cannot be published is never handed out (a just-created container is destroyed rather than leaked as an adoptable orphan) — acquiring raises `OwnershipBackendError`, matching the stream bridge's fail-hard v1 policy. Reaping: a store that cannot answer is treated as peer-owned, so an outage never turns live peer containers into orphans. **Renewal is the deliberate exception**: an unanswerable store there means *unknown*, not lost, so `_refresh_ownership` keeps the sandbox and retries — failing closed on that path would evict every live sandbox on every instance the moment the store blinked. The TTL still bounds how long a genuinely dead owner holds a lease. Both paths that stop a container they still track — `destroy()` and `_destroy_warm_entry` — claim **before** untracking, so a refused claim cannot leave a container running and untracked. (`_drop_unhealthy_sandbox` untracks first, under its `expected_info` TOCTOU guard, then claims before the stop; a refused claim there leaves the container to the next reconcile, which re-adopts it after the grace.) + - **A lease excludes peers, never ourselves — same-process exclusion is the provider's job.** `claim()` and `take()` both succeed against this instance's 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. Every reaper — idle checker, renewal, warm eviction, unhealthy drop — decides outside `self._lock`, because a store round trip must not be held under the lock that guards every acquire; so each one acts on a decision its own acquire path may already have invalidated. Two guards cover the two directions, and both live in `AioSandboxProvider`, not the store: + - **Reaping** (`_reserve_local_teardown` / `_local_teardown`): the reaper marks the id, and every promote path — `_reuse_in_process_sandbox`, `_reclaim_warm_pool_sandbox`, `_register_discovered_sandbox` — refuses a marked id exactly as it refuses a peer's `del:` (drop and cold-start). The "is this still reapable?" check runs **in the same critical section as the mark**, passed down as a `still_reapable` predicate rather than run by the caller beforehand: checking first and marking second *is* the window, not a narrower version of it. This matters most where the entry deliberately stays visible during the stop — both warm reapers defer their pop so a refused claim cannot lose the container — and where the maps are cleared first (`_drop_unhealthy_sandbox`), which leaves backend discovery as the open path. On `main` the mixin's `_evict_oldest_warm` / `_reap_expired_warm` popped under the lock, so the deferred pop is what made this reachable. + - **Forgetting** (`_acquire_epoch`): when `renew()` reports `LOST` the peer legitimately wins, so here the *promote* is the thing to detect. `_publish_ownership` bumps a per-id acquire epoch; `_renew_owned_leases` and `release()` snapshot it before the round trip and hand it to `_forget_lost_sandbox`, which skips the pop if it moved. Object identity is not enough: 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. + - **A guard must become visible no later than the transition it guards.** The epoch cannot satisfy that for `take()`: the takeover is durable before `take()` returns (redis has committed the SET while the reply is in flight), and the epoch can only be written afterwards, so a renewal holding an older `LOST` walks through the gap, drops the maps, and closes the client the acquire is about to hand back — acquire then returns an id whose `get()` is `None`. `_publish_ownership` therefore publishes an **intent mark** (`_acquire_inflight`) under `_lock` *before* the round trip; the epoch covers the other half, "an acquire completed since you decided". `_forget_lost_sandbox` honours the intent mark unconditionally, not only when an epoch is supplied — "no epoch" must not read as "no guard". + - **A reservation must cover the removal, not just the stop.** `_destroy_warm_entry` pops the warm entry itself, inside the reservation. Releasing the reservation when the stop returns and letting the caller pop afterwards leaves a gap where the container is stopped, the entry is still in `_warm_pool`, and nothing marks it — a reclaim there hands out a dead container. The pop stays deferred relative to the *stop* (a refused or failed stop keeps the entry), just no longer relative to the reservation. + - **A check taken before a round trip must be retaken after it.** `_reuse_in_process_sandbox` re-verifies both its map entry and the local teardown reservation, `_reclaim_warm_pool_sandbox` re-checks the reservation, and `_register_discovered_sandbox` re-checks before installing its client, all after publishing ownership. Before the intent mark is set a renewal's `LOST` is both current and correct, so the forget can legitimately remove the entry the acquire decided to hand out; independently, a local reaper can reserve an id while reuse is outside `_lock` for its health/store calls and deliberately leaves the map entry present until its destroy claim succeeds. Falling through re-discovers or cold-starts instead of returning/installing a client for either stale decision. The pre-round-trip checks remain as early-outs that skip backend and store work on an already-doomed entry. + - **Adoption is a promote too.** `_reconcile_orphans` honours the reservation: a container being torn down is untracked and still running, which is exactly the shape that loop adopts, and neither the claim (ours) nor the recovery grace (skipped entirely on `memory`, where `supports_cross_process` is `False`) excludes it. + - **Active and warm are exclusive, and only a promote can violate it.** Both register paths pop `_warm_pool` inside the same locked section that inserts into `_sandboxes`: a warm entry for an id is stale the moment that id becomes active, and leaving it gives the container *two* reapers — `_reap_expired_warm` judges it by the warm timestamp and never consults `_last_activity`, so it stops a container an agent is using while `_sandboxes` still hands out its client. Reachable because reconciliation adopts into the warm pool inside the register's publish → track window, and on `memory` 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). On `main` the track was a single locked insert with nothing before it, so the window did not exist. + A non-destroy `claim()` is the one case the store does police against its own owner: it refuses to overwrite our own `del:`, because the stop it marks is already in flight and downgrading the marker would let a `take()` hand out a container about to die. Enforced in both backends (Lua and Python) so they cannot drift. + - **Renewal is independent of `idle_timeout`** (`_start_lease_renewal`, own daemon thread; TTL = `renewal_interval_seconds × ttl_multiplier`). Renewal used to ride on the idle checker, which `__init__` only starts when `idle_timeout > 0` — so `idle_timeout: 0` ("keep warm VMs until shutdown", a documented config) let every lease lapse. Liveness and reaping must not share a switch. Renewal covers warm entries as well as active ones; losing a lease drops the sandbox from this instance's maps **without touching the container** (`_forget_lost_sandbox`) — destroying it there would be the very cross-instance kill this store prevents. + - A warm teardown is the local exception to that forget rule: `_destroy_warm_entry` deliberately keeps the entry in `_warm_pool` until the backend stop succeeds, while its own `del:` marker makes ordinary `renew()` report `LOST`. `_forget_lost_sandbox` therefore honours `_local_teardown`; otherwise the renewal thread can pop the retained entry mid-stop and a failed stop leaves a running container untracked. + - **`renew()` distinguishes lapsed from lost** (`RenewOutcome`), and the two must not be collapsed. `LAPSED` means the lease is simply absent — nobody took it — so `_refresh_ownership` re-establishes it; `LOST` means a peer holds it and it is never re-taken. Treating an absent lease as lost meant a Redis restart without persistence (every key gone) evicted every in-flight sandbox on every instance at once. + - Renewal's fail-open rule covers both store round trips. If `renew()` returns `LAPSED` but the follow-up `claim()` cannot answer, ownership is still unknown rather than lost, so the provider keeps the sandbox and retries. The ordinary `_claim_ownership` helper remains fail-closed for adopt/reap callers and is intentionally not used for this re-claim. + - **Teardown join budget covers refresh plus release.** Redis bounds each ownership operation at five seconds, and context exit can catch the heartbeat in one final refresh before its `finally` performs the final release. `_TEARDOWN_JOIN_TIMEOUT_SECONDS` is therefore 12 seconds — greater than both sequential operation bounds — so a normal pair of socket timeouts does not emit the deferred-release warning; a still-running heartbeat continues to own the release safely. + - **An absent lease means the same thing on both paths, and reconciliation must say so too.** The `LAPSED` rule above only covers an owner renewing its *own* lease; on its own it does not make state loss safe, because reconciliation reads the same absent key as "orphan, adopt". After a Redis flush (restart without persistence, or eviction under `maxmemory`) every owner is alive and merely pre-renewal-tick, so whichever instance reconciles first would adopt every live container, each real owner's next renewal would report `LOST`, and it would drop a sandbox mid-turn for the adopter to idle-destroy — #4206 through the back door. `_adoptable_after_grace` closes it: an untracked container must be seen unowned (`owner()`, a read-only peek — the atomic `claim()` is still what actually gates adoption) across a full lease TTL before it can be adopted, tracked per container in `_unowned_since`. That rebuilds the delay the flush erased — a live owner republishes within one renewal interval, shorter than the TTL by construction (`ttl_multiplier >= 2`) — while a genuinely crashed owner never republishes, 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 grace is skipped when `supports_cross_process` is `False`: 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 (peers are invisible to each other's leases with or without it). + - **The `memory` store is single-instance only** and says so via `supports_cross_process = False`; the provider logs a warning at startup when the configured store cannot see peers. A multi-worker gateway on `memory` has no cross-process coordination at all — same contract as `stream_bridge`'s memory backend. This is why the redis inference matters: it reads `app_config.stream_bridge` **and** the env var, in the same order the bridge's own resolver does, so any deployment already pointing the bridge at Redis (i.e. every multi-instance one) gets a redis ownership store without extra config. + - `get()` stays a pure in-memory lookup and must never call the store (that is blocking filesystem/network IO on the event loop); anchored by `tests/blocking_io/test_aio_sandbox_get.py`, which injects a deliberately-blocking probe store so the anchor keeps its teeth regardless of the configured backend. Tests: `tests/test_sandbox_ownership_store.py` (store contract, defined once for **both** backends — but the redis tier is `@pytest.mark.integration` + opt-in via `DEER_FLOW_TEST_REDIS_URL` and self-skips, and **CI provisions no redis**, so the merge gate runs the memory tier only and the Lua scripts never execute there; drift between the backends is caught only when the suite runs against a live redis. There is no fake-redis tier because the redis exclusion lives in Lua a fake would not execute) and `tests/test_sandbox_orphan_reconciliation.py` (provider behaviour, two providers sharing one store). - `BoxliteProvider` (`packages/harness/deerflow/community/boxlite/`) - BoxLite micro-VM isolation. The `boxlite` runtime is optional (`deerflow-harness[boxlite]`) and lazy-imported only when this provider is selected. The provider owns one private asyncio event loop on a daemon thread because BoxLite handles are loop-affine; sync `Sandbox` calls marshal onto that loop with `run_coroutine_threadsafe`. Boxes are named deterministically from `user_id:thread_id`, released into an in-process warm pool after each agent turn, and reclaimed only by the same user/thread. Warm-pool health checks use a short explicit timeout and forward that timeout through both BoxLite `exec(timeout=...)` and the private-loop `.result(timeout)` bridge so a hung VM cannot pin the per-thread acquire lock indefinitely. `sandbox.replicas` caps active + warm VMs per gateway process; if capacity is exhausted, only warm-pool VMs are evicted. `sandbox.idle_timeout` stops idle warm VMs after the configured seconds. `reset()` is intentionally a lightweight registry clear for `reset_sandbox_provider()` and does not close boxes, stop the idle reaper, or close the private loop; full teardown remains `shutdown()`. diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py index c6abc87d7..09bccad91 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py @@ -12,6 +12,7 @@ The provider itself handles: import asyncio import atexit +import contextlib import hashlib import logging import os @@ -19,6 +20,7 @@ import signal import threading import time import uuid +from collections.abc import Callable from concurrent.futures import ThreadPoolExecutor try: @@ -45,6 +47,15 @@ from deerflow.skills.storage import user_should_see_legacy_skills from .aio_sandbox import AioSandbox from .backend import SandboxBackend, wait_for_sandbox_ready, wait_for_sandbox_ready_async from .local_backend import LocalContainerBackend +from .ownership import ( + OwnershipBackendError, + RenewOutcome, + SandboxOwnershipStore, + compute_lease_ttl, + generate_owner_id, + make_sandbox_ownership_store, + resolve_ownership_config, +) from .remote_backend import RemoteSandboxBackend from .sandbox_info import SandboxInfo @@ -60,6 +71,20 @@ _THREAD_LOCK_EXECUTOR = ThreadPoolExecutor(max_workers=THREAD_LOCK_EXECUTOR_WORK atexit.register(_THREAD_LOCK_EXECUTOR.shutdown, wait=False, cancel_futures=True) +class SandboxBeingDestroyedError(RuntimeError): + """A peer is tearing this container down, so it must not be handed out. + + Raised on the acquire path when the ownership lease is in its teardown state. + The caller drops the container from tracking and lets the normal + discover-or-create path provision a fresh one, rather than handing an agent a + sandbox that is about to stop underneath it. + """ + + def __init__(self, sandbox_id: str) -> None: + super().__init__(f"sandbox {sandbox_id} is being destroyed by another instance") + self.sandbox_id = sandbox_id + + def _lock_file_exclusive(lock_file) -> None: if fcntl is not None: fcntl.flock(lock_file, fcntl.LOCK_EX) @@ -136,6 +161,14 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): API_KEY: $MY_API_KEY """ + # How long `_held_teardown_lease` waits for its heartbeat thread to exit + # before deferring the final lease release to that (still-running) thread. + # The store's socket timeout bounds each operation, but context exit can + # catch the heartbeat in one final refresh and must then wait for its final + # release. Keep this above both sequential five-second operation bounds so a + # normally timing-out refresh + release still finishes synchronously. + _TEARDOWN_JOIN_TIMEOUT_SECONDS = 12.0 + def __init__(self): self._lock = threading.Lock() self._sandboxes: dict[str, AioSandbox] = {} # sandbox_id -> AioSandbox instance @@ -148,11 +181,39 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): # Containers here can be reclaimed quickly (no cold-start) or destroyed # when replicas capacity is exhausted. self._warm_pool: dict[str, tuple[SandboxInfo, float]] = {} + # sandbox_id -> when reconciliation first saw it running with no lease. + # Gates adoption behind a recovery grace (see _adoptable_after_grace). + self._unowned_since: dict[str, float] = {} + # The two halves of same-process exclusion. The ownership store excludes + # peers and nothing else — `claim()` and `take()` both succeed against + # our own lease by design — so `del:` says nothing to this process's own + # threads. See _reserve_local_teardown / _acquire_epoch. + self._local_teardown: set[str] = set() + self._acquire_epoch: dict[str, int] = {} + self._acquire_epoch_counter = 0 + self._acquire_inflight: dict[str, int] = {} self._shutdown_called = False self._idle_checker_stop = threading.Event() self._idle_checker_thread: threading.Thread | None = None + self._renewal_stop = threading.Event() + self._renewal_thread: threading.Thread | None = None + # Per-instance id used for cross-instance sandbox ownership leases (#4206). + self._owner_id = generate_owner_id() self._config = self._load_config() + self._ownership_config = resolve_ownership_config(self._config.get("ownership"), stream_bridge=self._config.get("stream_bridge")) + self._ownership: SandboxOwnershipStore = make_sandbox_ownership_store(self._ownership_config, owner_id=self._owner_id) + if not self._ownership.supports_cross_process: + # Peers cannot see these leases, so every container looks like an + # orphan to them. Say so once rather than letting #4206 resurface + # silently on a multi-worker deployment that never set the config. + logger.warning( + "Sandbox ownership store cannot coordinate across processes (sandbox.ownership.type: %s). " + "Safe for a single gateway instance only — multi-worker / load-balanced gateways sharing a " + "container backend must set sandbox.ownership.type: redis, or peers will adopt and idle-destroy " + "each other's live sandboxes (#4206).", + self._ownership_config.type, + ) self._backend: SandboxBackend = self._create_backend() # Register shutdown handler @@ -162,6 +223,11 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): # Reconcile orphaned containers from previous process lifecycles self._reconcile_orphans() + # Renewal is independent of idle cleanup: an owner must keep proving it is + # alive even when the idle reaper is disabled, or peers adopt its live + # containers once the lease lapses (idle_timeout: 0 is a supported config). + self._start_lease_renewal() + # Start idle checker if enabled if self._config.get("idle_timeout", DEFAULT_IDLE_TIMEOUT) > 0: self._start_idle_checker() @@ -220,6 +286,11 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): "replicas": replicas if replicas is not None else DEFAULT_REPLICAS, "mounts": sandbox_config.mounts or [], "environment": self._resolve_env_vars(sandbox_config.environment or {}), + "ownership": getattr(sandbox_config, "ownership", None), + # A redis stream bridge means the deployment is multi-instance, which + # is what the ownership store must default to. Read the same source + # the bridge's own resolver reads, not just its env var. + "stream_bridge": getattr(config, "stream_bridge", None), # provisioner URL for dynamic pod management (e.g. http://provisioner:8002) "provisioner_url": getattr(sandbox_config, "provisioner_url", None) or "", "provisioner_api_key": getattr(sandbox_config, "provisioner_api_key", None) or "", @@ -237,24 +308,338 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): resolved[key] = str(value) return resolved + # ── Cross-instance ownership leases ─────────────────────────────────── + + def _publish_ownership(self, sandbox_id: str) -> None: + """Take responsibility for *sandbox_id* on the acquire path. + + Takes over from whichever instance served this thread last — the + container is deterministic per (user, thread), so a turn routing here is + a legitimate handover. The previous owner's next renewal reports LOST and + it stops tracking the container without touching it. + + Deliberately **not** fail-open. Swallowing the error and handing the + sandbox out anyway would leave it unowned while in active use, so a peer + would see an orphan and reap it — the exact failure this store exists to + stop. Callers must let this propagate. + + The intent mark is published **before** the round trip, and that ordering + is the point. ``take()`` makes the takeover durable before it returns — + on redis the server has committed the SET while the reply is still in + flight — so bumping the epoch afterwards leaves a window where the store + already says the container is ours but the guard still reads as though it + were not. A renewal holding an older ``LOST`` walks straight through it, + drops the maps, and closes the client this call is about to hand back, so + acquire returns an id the provider no longer tracks and ``get()`` answers + ``None``. A guard must become visible no later than the transition it + guards; the epoch alone cannot, because it can only be written after the + call that performs the transition returns. + + So the two marks cover the two halves and are both needed: the intent + mark covers "an acquire is mid-flight", the epoch covers "an acquire + completed since you decided". + + Raises: + SandboxBeingDestroyedError: a peer is tearing this container down, so + it must not be handed to an agent (the destroy → re-acquire race). + OwnershipBackendError: ownership could not be published. + """ + with self._lock: + self._acquire_inflight[sandbox_id] = self._acquire_inflight.get(sandbox_id, 0) + 1 + try: + if not self._ownership.take(sandbox_id): + raise SandboxBeingDestroyedError(sandbox_id) + with self._lock: + self._acquire_epoch_counter += 1 + self._acquire_epoch[sandbox_id] = self._acquire_epoch_counter + finally: + # A count rather than a set: acquires for one id are serialized by + # the per-thread lock today, so a set would be equivalent — but that + # is an assumption about a caller two layers up, and if it ever + # stopped holding, a set would be cleared by the first finisher and + # silently reopen this window. Counting removes the assumption. + with self._lock: + remaining = self._acquire_inflight.get(sandbox_id, 0) - 1 + if remaining > 0: + self._acquire_inflight[sandbox_id] = remaining + else: + self._acquire_inflight.pop(sandbox_id, None) + + # ── Same-process exclusion (the half the store does not provide) ─────── + # + # A lease excludes *peers*: `claim()` succeeds against our own `own:` lease + # by design (that is what lets a destroy path claim what it already owns), + # and `take()` succeeds against it too. So between this process's reaper + # threads — idle checker, renewal, eviction — and its own acquire path, the + # store provides **no exclusion at all**. Every reaper decides outside + # `self._lock` (a store round trip must not be held under the lock that + # guards every acquire), so each one acts on a decision an acquire may + # already have invalidated. The two helpers below are that missing half, one + # per direction: + # + # reaping — we are about to stop/drop it, so nothing may promote it: + # reserve it, and make every promote path honour the reservation + # exactly as it honours a peer's `del:`. + # forgetting — a peer legitimately owns it and must win, so the promote is + # the thing to detect: compare the acquire epoch we decided on. + + def _reserve_local_teardown(self, sandbox_id: str, still_reapable: Callable[[], bool]) -> bool: + """Reserve *sandbox_id* for teardown by this process. + + ``still_reapable`` is evaluated in the **same** critical section as the + reservation, so no acquire can slip between the last check and the mark. + That pairing is the whole point: checking first and marking second is the + window, not a narrower version of it. + + Consequence, and the one rule a new caller has to know: **the predicate + runs with ``self._lock`` held**, which is a plain ``Lock``, so a predicate + that touches the lock — directly, or via a provider method that takes it — + deadlocks. Predicates must be cheap, non-blocking reads of the maps + (``sandbox_id in self._warm_pool``, a ``_last_activity`` comparison). The + constraint is stated rather than engineered around on purpose: making the + lock reentrant to tolerate it would trade a loud hang for a quiet class of + re-entrancy bugs everywhere else in this provider. + """ + with self._lock: + if sandbox_id in self._local_teardown or not still_reapable(): + return False + self._local_teardown.add(sandbox_id) + return True + + def _finish_local_teardown(self, sandbox_id: str) -> None: + with self._lock: + self._local_teardown.discard(sandbox_id) + + def _being_torn_down_locally(self, sandbox_id: str) -> bool: + """Whether a reaper thread in *this* process holds *sandbox_id*. + + Callers must already hold ``self._lock``. + """ + return sandbox_id in self._local_teardown + + def _acquire_epoch_of(self, sandbox_id: str) -> int: + """Snapshot the acquire generation, so a stale decision can be detected. + + Bumped only by ``_publish_ownership`` — i.e. exactly when an acquire path + (re)takes the lease on the way to handing the sandbox to an agent. + Re-establishing a lapsed lease from ``_refresh_ownership`` deliberately + does not bump it: nothing was handed out, so a reaper's decision about + that id is still current. + """ + with self._lock: + return self._acquire_epoch.get(sandbox_id, 0) + + def _claim_ownership(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + """Take (or refresh) ownership of *sandbox_id*. + + A successful claim is what makes acting on the container safe: while we + hold the lease a peer's claim fails. With ``for_destroy`` the lease is + additionally marked as a teardown, which a concurrent acquire-side + ``take()`` refuses — that is what closes the ownership-check → container- + stop window the deleted per-sandbox flock guard used to cover. + + Fails closed on a backend error: ownership unknown is treated as + "not ours" so we neither adopt nor destroy the container. + """ + try: + return self._ownership.claim(sandbox_id, for_destroy=for_destroy) + except OwnershipBackendError as e: + logger.warning("Sandbox ownership claim failed for %s (treating as not owned): %s", sandbox_id, e) + return False + + def _release_ownership(self, sandbox_id: str) -> None: + try: + self._ownership.release(sandbox_id) + except OwnershipBackendError as e: + # Best effort: the lease expires on its own, so a failed release + # delays reuse rather than corrupting ownership. + logger.warning("Failed to release sandbox ownership for %s: %s", sandbox_id, e) + + def _refresh_ownership(self, sandbox_id: str) -> bool: + """Keep holding *sandbox_id*'s lease. False when a peer has taken it. + + A **lapsed** lease is re-established rather than treated as lost: nobody + holds it, so re-claiming is safe, and this is what keeps a Redis restart + (which drops every key) from evicting every live sandbox fleet-wide. A + lease a peer actually holds is never re-taken — that is the #4206 kill. + """ + try: + outcome = self._ownership.renew(sandbox_id) + except OwnershipBackendError as e: + # Unknown, not lost: keep the sandbox and retry next tick. The TTL + # still bounds how long a genuinely dead owner holds the lease. + logger.warning("Could not renew sandbox ownership for %s, will retry: %s", sandbox_id, e) + return True + + if outcome is RenewOutcome.RENEWED: + return True + if outcome is RenewOutcome.LAPSED: + # Free: re-establish. This is the deliberate fail-open renewal path, + # so it cannot use `_claim_ownership`: that helper turns a backend + # error into False for adopt/reap callers, which would conflate an + # outage between these two round trips with a peer takeover and + # evict a live sandbox. Unknown means keep-and-retry here, exactly as + # it does when the `renew()` call itself cannot answer above. + try: + if self._ownership.claim(sandbox_id): + logger.info("Re-established a lapsed ownership lease for %s", sandbox_id) + return True + except OwnershipBackendError as e: + logger.warning("Could not re-establish lapsed lease for %s, will retry: %s", sandbox_id, e) + return True + logger.warning("Lapsed ownership lease for %s was taken by a peer", sandbox_id) + return False + return False + + @contextlib.contextmanager + def _held_teardown_lease(self, sandbox_id: str): + """Keep *sandbox_id*'s teardown marker alive for as long as its stop runs. + + ``claim(..., for_destroy=True)`` writes the ``del:`` marker with the + ordinary lease TTL, and normal ``renew()`` extends only ``own:`` while + deliberately reporting a teardown as ``LOST``. Active and unhealthy + destroy paths drop the sandbox from the maps ``_renew_owned_leases`` + iterates; the warm path keeps its entry visible until the stop succeeds, + so ``_forget_lost_sandbox`` separately honours ``_local_teardown`` rather + than misreading our own marker as a peer takeover. Without this heartbeat, + a container stop that outlived the TTL let the marker lapse, a peer's + ``take()`` succeeded against the still-running container, and the stop + landed on the turn that had just been handed it — the very window the + ``del:`` state exists to close, reopened by its own expiry. + + This is what the per-sandbox ``flock`` used to cover for free: a held lock + cannot expire. A lease can, so the exclusion has to be held deliberately + rather than assumed to outlast the work it guards. Reachable without an + abnormal backend — the config schema bounds only ``renewal_interval_seconds`` + (> 0) and ``ttl_multiplier`` (>= 2), so a legal setting 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. + + 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. + + The final release is the heartbeat's own last act, not the caller's. A + refresh ``claim`` still in flight when the context exits (the socket + timeout bounds it, but it can be mid-call) would otherwise land *after* a + caller-side release and rewrite the ``del:`` marker on a container whose + stop had already completed — stranding a fresh ``take()`` (or rolling back + a fresh create) until the TTL. Releasing from inside the heartbeat, after + its loop has stopped, sequences the release strictly after the last + refresh, so no claim can follow it. + """ + stop = threading.Event() + + def beat() -> None: + interval = self._ownership_config.renewal_interval_seconds + try: + while not stop.wait(interval): + try: + if not self._ownership.claim(sandbox_id, for_destroy=True): + # Only reachable if the store lost our marker *and* a + # peer took it (e.g. a flush mid-stop). The stop is + # already in flight and cannot be recalled, so say so + # loudly rather than let a peer's container die without + # a trace. + logger.error( + "Lost the teardown exclusion for %s while its container stop was still in flight; a peer may have taken it", + sandbox_id, + ) + return + except Exception as e: + # Broad on purpose: a refresh that raises must not kill the + # heartbeat and strand the marker for a stop that can run + # unbounded. Unknown, not lost — the marker may still be + # live and the TTL bounds a stale one. Retry on the next tick. + logger.warning("Could not refresh the teardown lease for %s, will retry: %s", sandbox_id, e) + finally: + # Release last, from the heartbeat itself, so an in-flight refresh + # can never run after the marker is cleared. `release()` drops only + # our own lease, so this is a safe no-op if a peer took it above. + self._release_ownership(sandbox_id) + + beater = threading.Thread(target=beat, name="sandbox-teardown-lease", daemon=True) + beater.start() + try: + yield + finally: + stop.set() + beater.join(timeout=self._TEARDOWN_JOIN_TIMEOUT_SECONDS) + if beater.is_alive(): + # The budget covers a normally timing-out refresh plus the final + # release. The release is the heartbeat's job and is still + # pending; clearing the marker here would reopen the exact race + # this owns, so leave it — the thread will release when it + # unblocks, or the TTL will reap it. + logger.warning( + "Teardown heartbeat for %s did not exit within %.1fs; its lease release is deferred to that thread", + sandbox_id, + self._TEARDOWN_JOIN_TIMEOUT_SECONDS, + ) + # ── Startup reconciliation ──────────────────────────────────────────── + def _adoptable_after_grace(self, sandbox_id: str, now: float) -> bool: + """Whether *sandbox_id* has looked unowned long enough to be a real orphan. + + An absent lease normally proves the owner died and its TTL ran out. But + the store can lose every key while every owner is alive and serving — a + Redis restart without persistence, or eviction under ``maxmemory`` + pressure. ``_refresh_ownership`` already refuses to read that as + abandonment (``LAPSED`` is re-established, not surrendered). Reading the + same signal as "orphan, adopt" here would contradict it on the other + path: whoever reconciles first would adopt every live container in the + window before its owner's next renewal tick, that owner's renewal would + then report ``LOST``, and it would drop a sandbox it is actively serving + for the adopter to idle-destroy — #4206 through the back door. + + Waiting one full lease TTL rebuilds the delay the state loss erased. A + live owner republishes within one renewal interval, which is shorter than + the TTL by construction (``ttl_multiplier >= 2``), so only a container + whose owner is really gone stays unowned across the whole grace. + """ + if not self._ownership.supports_cross_process: + # No peer can hold a lease this store would show us, so an unowned + # container cannot be a live peer's — it is from a dead lifecycle of + # this process. Single-instance deployments keep instant cleanup, and + # a grace could not help a multi-worker one on this store anyway: + # peers are invisible to each other's leases with or without it. + return True + + try: + current_owner = self._ownership.owner(sandbox_id) + except OwnershipBackendError as e: + # Unknown, not free: fail closed, same as _claim_ownership. + logger.warning("Could not read sandbox ownership for %s during reconciliation (deferring adoption): %s", sandbox_id, e) + return False + + if current_owner is not None: + # Owned — by a peer, or already by us. Either way not an orphan, and + # a live owner republishing must restart the grace rather than let a + # stale one expire over its lease. + self._unowned_since.pop(sandbox_id, None) + return False + + first_seen = self._unowned_since.setdefault(sandbox_id, now) + return now - first_seen >= compute_lease_ttl(self._ownership_config) + def _reconcile_orphans(self) -> None: """Reconcile orphaned containers left by previous process lifecycles. - On startup, enumerate all running containers matching our prefix - and adopt them all into the warm pool. The idle checker will reclaim - containers that nobody re-acquires within ``idle_timeout``. + On startup (and periodically from the idle checker), enumerate running + containers matching our prefix and adopt **true orphans** into the warm + pool. A container is only adopted when this instance can claim its + ownership lease — so multi-instance gateways cannot adopt and later + idle-destroy a peer's live sandbox (#4206). - All containers are adopted unconditionally because we cannot - distinguish "orphaned" from "actively used by another process" - based on age alone — ``idle_timeout`` represents inactivity, not - uptime. Adopting into the warm pool and letting the idle checker - decide avoids destroying containers that a concurrent process may - still be using. + Adopted orphans get a fresh warm-pool timestamp; the idle checker then + destroys them if nobody re-acquires within ``idle_timeout``. That still + cleans containers left by a crashed process once its lease expires. - This closes the fundamental gap where in-memory state loss (process - restart, crash, SIGKILL) leaves Docker containers running forever. + An unowned container is not adopted on sight — it must stay unowned for a + recovery grace first, so a store that lost its state cannot be mistaken + for a fleet of dead owners (see ``_adoptable_after_grace``). """ try: running = self._backend.list_running() @@ -262,25 +647,68 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): logger.warning(f"Failed to enumerate running containers during startup reconciliation: {e}") return + # Forget grace timers for containers that no longer exist, so a + # long-lived instance does not accumulate an entry per destroyed + # container. Runs before the empty-list return so it also drains. + running_ids = {info.sandbox_id for info in running} + self._unowned_since = {sid: seen for sid, seen in self._unowned_since.items() if sid in running_ids} + if not running: return current_time = time.time() adopted = 0 + skipped_live = 0 + deferred = 0 for info in running: age = current_time - info.created_at if info.created_at > 0 else float("inf") + if not self._adoptable_after_grace(info.sandbox_id, current_time): + deferred += 1 + logger.debug("Deferring container %s during reconciliation: owned, or not yet past the recovery grace", info.sandbox_id) + continue + + # Claim second: a successful claim proves the container is not a + # peer's and locks peers out. It says nothing about *us* — it + # succeeds against our own lease by design — so it is not a substitute + # for the local teardown check below. The grace above is likewise a + # precondition, not a substitute; only the claim is atomic. + if not self._claim_ownership(info.sandbox_id): + skipped_live += 1 + logger.debug("Skipping container %s during reconciliation: owned by another instance", info.sandbox_id) + continue + # Single lock acquisition per container: atomic check-and-insert. - # Avoids a TOCTOU window between the "already tracked?" check and - # the warm-pool insert. + # Avoids a TOCTOU window between the "already tracked?" check and the + # warm-pool insert. with self._lock: if info.sandbox_id in self._sandboxes or info.sandbox_id in self._warm_pool: continue + if self._being_torn_down_locally(info.sandbox_id): + # Adoption is a promote, so it needs the same reservation + # check as the other three. A container being torn down here + # is untracked and still running, which is exactly the shape + # this loop adopts — and neither the claim nor the grace + # excludes it. On `memory` the grace is skipped outright + # (`supports_cross_process = False`), so nothing else stands + # in the way there at all; adopting would park a container + # into the warm pool moments before its stop lands, leaving a + # dead entry for the next reclaim to hand out. + deferred += 1 + logger.debug("Deferring container %s during reconciliation: this instance is tearing it down", info.sandbox_id) + continue self._warm_pool[info.sandbox_id] = (info, current_time) + self._unowned_since.pop(info.sandbox_id, None) adopted += 1 logger.info(f"Adopted container {info.sandbox_id} into warm pool (age: {age:.0f}s)") - logger.info(f"Startup reconciliation complete: {adopted} adopted into warm pool, {len(running)} total found") + logger.info( + "Startup reconciliation complete: %s adopted into warm pool, %s skipped (live peer ownership), %s deferred (owned or within recovery grace), %s total found", + adopted, + skipped_live, + deferred, + len(running), + ) # ── Deterministic ID ───────────────────────────────────────────────── @@ -415,8 +843,134 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): def _cleanup_idle_resources(self, idle_timeout: float) -> None: """Clean AIO resources idle longer than ``idle_timeout`` seconds.""" + # Pick up containers whose peer leases expired since startup (crash path). + self._reconcile_orphans() self._cleanup_idle_sandboxes(idle_timeout) + # ── Ownership lease renewal ────────────────────────────────────────── + + def _start_lease_renewal(self) -> None: + """Start the daemon thread that keeps this instance's leases alive. + + Deliberately not folded into the idle checker: that thread only starts + when ``idle_timeout > 0``, so renewal riding on it silently stopped for + ``idle_timeout: 0`` deployments — a supported config ("keep warm VMs + until shutdown") — letting every lease lapse and reopening #4206 one TTL + later. Liveness and reaping must not share a switch. + """ + if self._renewal_thread is not None and self._renewal_thread.is_alive(): + return + + self._renewal_stop.clear() + self._renewal_thread = threading.Thread( + target=self._lease_renewal_loop, + name="sandbox-lease-renewal", + daemon=True, + ) + self._renewal_thread.start() + logger.info( + "Started sandbox ownership renewal thread (interval: %.1fs, ttl: %.1fs)", + self._ownership_config.renewal_interval_seconds, + self._ownership_config.renewal_interval_seconds * self._ownership_config.ttl_multiplier, + ) + + def _stop_lease_renewal(self) -> None: + self._renewal_stop.set() + thread = self._renewal_thread + if thread is not None and thread.is_alive() and thread is not threading.current_thread(): + thread.join(timeout=5) + + def _lease_renewal_loop(self) -> None: + interval = self._ownership_config.renewal_interval_seconds + while not self._renewal_stop.wait(interval): + try: + self._renew_owned_leases() + except Exception: + logger.exception("Error in sandbox ownership renewal loop") + + def _renew_owned_leases(self) -> None: + """Renew every container this instance believes it owns. + + Covers warm entries as well as active ones: a warm container is still + ours (we hold it for fast reclaim), so letting its lease lapse would let + a peer adopt a container we are about to hand back to its thread. + + Only a lease a **peer** now holds means the container is no longer ours; + a lapsed one is re-established (see ``_refresh_ownership``). Conflating + the two would evict every live sandbox on this instance the first time + the store lost its state. + """ + with self._lock: + owned_ids = list(self._sandboxes.keys()) + list(self._warm_pool.keys()) + + for sandbox_id in owned_ids: + # Snapshot before the round trip: by the time `renew()` answers LOST, + # an acquire in this process may already have taken the lease back + # and promoted the id, and the answer is about the lease we held then. + epoch = self._acquire_epoch_of(sandbox_id) + if not self._refresh_ownership(sandbox_id): + logger.warning("Lost sandbox ownership lease for %s; dropping it from this instance", sandbox_id) + self._forget_lost_sandbox(sandbox_id, expected_epoch=epoch) + + def _forget_lost_sandbox(self, sandbox_id: str, *, expected_epoch: int | None = None) -> None: + """Drop a sandbox whose lease we no longer hold, without touching the container. + + The container now belongs to whichever instance holds the lease, so + stopping it here would be the very cross-instance kill this store exists + to prevent. Only our host-side handle goes away. + + ``expected_epoch`` guards callers whose "we lost it" decision came from a + store round trip made outside the lock. An acquire **mid-flight** counts + too: its ``take()`` can already have made the takeover durable while the + epoch is still unwritten, so the epoch alone would let a stale decision + through (see ``_publish_ownership``). An acquire that re-took the lease + in that window has already handed the sandbox to a turn — and, on the + reuse path, handed out the *same* tracked client, so no object-identity + check would notice. Dropping it then closes a client mid-turn and leaves + the agent holding an id whose tool calls fail until the next turn. + """ + with self._lock: + # A warm teardown deliberately keeps its entry visible until the + # backend stop succeeds. Its own `del:` marker makes `renew()` report + # LOST, but that is not a peer takeover and must not pop the retained + # entry — especially when the stop fails and the container remains + # live for retry/reclaim. The teardown path removes it on success. + if sandbox_id in self._local_teardown: + logger.debug("Not dropping sandbox %s: this instance is tearing it down", sandbox_id) + return + # The in-flight check is deliberately *not* conditional on + # `expected_epoch`. Today's epoch-less callers (the two + # `SandboxBeingDestroyedError` handlers) cannot collide with a + # publish for the same id — `_publish_ownership` has already cleared + # the mark by the time they run, and acquires for one id are + # serialized by the per-thread lock — so this changes no current + # behaviour. It is here because "no epoch supplied" reading as "no + # guard at all" is how the next caller of a dangerous primitive gets + # written; an id being acquired right now must never be dropped. + if sandbox_id in self._acquire_inflight: + logger.info("Not dropping sandbox %s: an acquire is publishing ownership for it", sandbox_id) + return + if expected_epoch is not None and self._acquire_epoch.get(sandbox_id, 0) != expected_epoch: + logger.info("Not dropping sandbox %s: this instance re-acquired it after the lease check", sandbox_id) + return + + sandbox = self._sandboxes.pop(sandbox_id, None) + self._sandbox_infos.pop(sandbox_id, None) + self._last_activity.pop(sandbox_id, None) + self._warm_pool.pop(sandbox_id, None) + self._acquire_epoch.pop(sandbox_id, None) + for key, mapped_id in list(self._thread_sandboxes.items()): + if mapped_id == sandbox_id: + del self._thread_sandboxes[key] + + # Close the host-side HTTP client we are dropping (#2872); the container + # itself stays up for its new owner. + if sandbox is not None: + try: + sandbox.close() + except Exception as e: + logger.warning(f"Error closing sandbox {sandbox_id} after losing its lease: {e}") + def _cleanup_idle_sandboxes(self, idle_timeout: float) -> None: current_time = time.time() active_to_destroy = [] @@ -429,29 +983,78 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): active_to_destroy.append(sandbox_id) logger.info(f"Sandbox {sandbox_id} idle for {idle_duration:.1f}s, marking for destroy") - # Destroy active sandboxes (re-verify still idle before acting) + # Destroy active sandboxes (re-verify still idle before acting). + # + # The re-verify has to happen in the same critical section as the + # teardown reservation, which is why it is handed to `_destroy_tracked` + # as a predicate rather than run here. Checking here and destroying + # afterwards left a window — widened by this PR from a few instructions + # to a store round trip, since `destroy()` now claims ownership before it + # untracks — in which a turn re-acquires the sandbox and then has its + # container stopped underneath it. + def still_idle(sandbox_id: str) -> bool: + last_activity = self._last_activity.get(sandbox_id) + if last_activity is None: + # Already released or destroyed by another path — skip. + logger.info(f"Sandbox {sandbox_id} already gone before idle destroy, skipping") + return False + if (time.time() - last_activity) < idle_timeout: + # Re-acquired (activity updated) since the snapshot — skip. + logger.info(f"Sandbox {sandbox_id} was re-acquired before idle destroy, skipping") + return False + return True + for sandbox_id in active_to_destroy: try: - # Re-verify the sandbox is still idle under the lock before destroying. - # Between the snapshot above and here, the sandbox may have been - # re-acquired (last_activity updated) or already released/destroyed. - with self._lock: - last_activity = self._last_activity.get(sandbox_id) - if last_activity is None: - # Already released or destroyed by another path — skip. - logger.info(f"Sandbox {sandbox_id} already gone before idle destroy, skipping") - continue - if (time.time() - last_activity) < idle_timeout: - # Re-acquired (activity updated) since the snapshot — skip. - logger.info(f"Sandbox {sandbox_id} was re-acquired before idle destroy, skipping") - continue logger.info(f"Destroying idle sandbox {sandbox_id}") - self.destroy(sandbox_id) + self._destroy_tracked(sandbox_id, still_reapable=lambda sid=sandbox_id: still_idle(sid)) except Exception as e: logger.error(f"Failed to destroy idle sandbox {sandbox_id}: {e}") self._reap_expired_warm(idle_timeout) + def _reap_expired_warm(self, idle_timeout: float | None = None) -> None: + """Destroy warm entries older than ``idle_timeout``, never a peer's live container.""" + timeout = float(self._config.get("idle_timeout", DEFAULT_IDLE_TIMEOUT) if idle_timeout is None else idle_timeout) + if timeout <= 0: + return + + now = time.time() + expired: list[tuple[str, SandboxInfo]] = [] + with self._lock: + for sandbox_id, (entry, timestamp) in self._warm_pool.items(): + if now - timestamp > timeout: + expired.append((sandbox_id, entry)) + + # Only drop an entry from the warm pool once we know it is really going + # away. Popping first would lose the container on a refused or + # unanswerable claim: still running, no longer tracked by anyone. The + # deferred pop is why the reservation is needed — the entry stays visible + # to `_reclaim_warm_pool_sandbox` for the whole stop. + for sandbox_id, entry in expired: + self._destroy_warm_entry(sandbox_id, entry, reason="idle_timeout", still_reapable=lambda sid=sandbox_id: sid in self._warm_pool) + + def _evict_oldest_warm(self) -> str | None: + """Evict the oldest warm entry this instance still owns.""" + with self._lock: + if not self._warm_pool: + return None + # Snapshot oldest-first under the lock; ownership is resolved outside + # it, since a claim can be a network round trip and the provider lock + # guards every acquire path. + candidates = [(sandbox_id, entry) for sandbox_id, (entry, _) in sorted(self._warm_pool.items(), key=lambda item: item[1][1])] + + for sandbox_id, entry in candidates: + # "Still in the warm pool?" is the reapable check, and it has to run + # in the same critical section as the reservation — checking it here + # and reserving afterwards is exactly the window a reclaim slips + # through. `_destroy_warm_entry` does both under one lock hold. + if not self._destroy_warm_entry(sandbox_id, entry, reason="replica_enforcement", still_reapable=lambda sid=sandbox_id: sid in self._warm_pool): + continue + return sandbox_id + + return None + # ── Signal handling ────────────────────────────────────────────────── def _register_signal_handlers(self) -> None: @@ -512,6 +1115,11 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): return None existing_id = self._thread_sandboxes[key] + if self._being_torn_down_locally(existing_id): + # A reaper thread in this process is stopping this container. + # Same answer as a peer's `del:` lease: cold-start instead. + logger.info("Cached sandbox %s is being destroyed by this instance; not reusing it", existing_id) + return None if existing_id in self._sandboxes: info = self._sandbox_infos.get(existing_id) else: @@ -537,7 +1145,40 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): suffix = " (post-lock check)" if post_lock else "" logger.info(f"Reusing in-process sandbox {existing_id} for user/thread {effective_user_id}/{thread_id}{suffix}") self._last_activity[existing_id] = time.time() - return existing_id + + # Fail closed: an OwnershipBackendError propagates rather than handing out + # a sandbox we could not publish ownership for. + try: + self._publish_ownership(existing_id) + except SandboxBeingDestroyedError: + # A peer is stopping this container. Drop it and let the caller + # discover-or-create a fresh one instead of handing over a sandbox + # that is about to disappear. + logger.info("Cached sandbox %s is being destroyed by another instance; not reusing it", existing_id) + self._forget_lost_sandbox(existing_id) + return None + + with self._lock: + if self._being_torn_down_locally(existing_id): + # The first reservation check ran before the backend health + # check and ownership round trip. A local reaper can win while + # either is in flight, and it deliberately keeps the entry in + # `_sandboxes` until its destroy claim succeeds. Membership + # alone therefore cannot prove this id is still safe to return. + logger.info("Cached sandbox %s was reserved for teardown while publishing ownership; not reusing it", existing_id) + return None + if existing_id not in self._sandboxes: + # Dropped while we were publishing. The intent mark closes the + # window *inside* `_publish_ownership`, but not the gap before + # it: until the mark is set a renewal's `LOST` is both current + # and correct — the peer really did hold the lease — so the + # forget legitimately runs and closes this client. Returning the + # id anyway would hand back a sandbox whose `get()` is `None`. + # Fall through instead; the caller re-discovers and builds a + # fresh client, and the lease we just took is already ours. + logger.info("Cached sandbox %s was dropped while publishing ownership; falling through to discovery", existing_id) + return None + return existing_id def _reclaim_warm_pool_sandbox( self, @@ -556,6 +1197,12 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): with self._lock: if sandbox_id not in self._warm_pool: return None + if self._being_torn_down_locally(sandbox_id): + # The entry deliberately stays in `_warm_pool` for the whole stop + # (so a refused claim does not lose the container), so pool + # membership alone does not mean it is reclaimable. + logger.info("Warm-pool sandbox %s is being destroyed by this instance; not reclaiming it", sandbox_id) + return None info, _ = self._warm_pool[sandbox_id] @@ -568,7 +1215,28 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): ) return None + # Publish ownership before the warm → active transition: a raise here must + # not leave the sandbox tracked as active but unowned (a peer would see an + # orphan and reap it mid-turn). On failure the entry stays warm and this + # instance keeps its existing lease. + try: + self._publish_ownership(sandbox_id) + except SandboxBeingDestroyedError: + logger.info("Warm-pool sandbox %s is being destroyed by another instance; not reclaiming it", sandbox_id) + self._forget_lost_sandbox(sandbox_id) + return None + with self._lock: + if self._being_torn_down_locally(sandbox_id): + # Re-checked, because the first check was before the round trip. + # A reaper can reserve *after* our `take()` — the warm entry is + # still there, since its pop is deferred until the stop returns — + # then claim `del:` (which succeeds: the lease is ours, we just + # took it) and stop the container. Whichever pop lands first + # decides, and if ours does we install a client for a container + # that is already stopped. + logger.info("Warm-pool sandbox %s was claimed for teardown while publishing ownership; not reclaiming it", sandbox_id) + return None warm_item = self._warm_pool.pop(sandbox_id, None) if warm_item is None: return None @@ -593,14 +1261,58 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): ) def _register_discovered_sandbox(self, thread_id: str, info: SandboxInfo, *, user_id: str) -> str: - """Track a sandbox discovered through the backend.""" + """Track a sandbox discovered through the backend. + + Raises: + SandboxBeingDestroyedError: discovery found the container still + running, but a peer is stopping it. Deliberately propagated + rather than swallowed: falling through to create would collide + with the not-yet-removed container name, and handing this one to + an agent is exactly the mid-turn death (#4206) the store exists to + prevent. The window is a peer's in-flight container stop, so the + thread's next turn discovers nothing and cold-starts cleanly. + """ + with self._lock: + if self._being_torn_down_locally(info.sandbox_id): + # Discovery is the fall-through once the caches miss, so it is + # also the path a reaper's own untracking opens up. `take()` would + # only refuse this once the reaper's `del:` claim has landed; + # until then it succeeds against our own lease. + raise SandboxBeingDestroyedError(info.sandbox_id) + sandbox = AioSandbox(id=info.sandbox_id, base_url=info.sandbox_url) key = self._thread_key(thread_id, user_id) - with self._lock: - self._sandboxes[info.sandbox_id] = sandbox - self._sandbox_infos[info.sandbox_id] = info - self._last_activity[info.sandbox_id] = time.time() - self._thread_sandboxes[key] = info.sandbox_id + # Ownership first, so a failure cannot leave a tracked-but-unowned sandbox. + # There is no container to roll back (we did not create it), but the + # host-side HTTP client constructed above is ours and must not leak — + # same close-on-failure as `_register_created_sandbox`. + try: + self._publish_ownership(info.sandbox_id) + with self._lock: + if self._being_torn_down_locally(info.sandbox_id): + # The pre-publish reservation check is only an early-out: a + # local reaper can reserve the id during the store round + # trip. Do not install a client for a container that reaper + # has already committed to stopping. + raise SandboxBeingDestroyedError(info.sandbox_id) + # Active and warm are exclusive states, and only this insert can + # violate that: a warm entry for the same id is stale the moment + # the id becomes active. Leaving it there gives the container two + # reapers — `_reap_expired_warm` judges it by the warm timestamp + # and never looks at `_last_activity`, so it stops a container an + # agent is actively using while `_sandboxes` still hands out its + # client. + self._warm_pool.pop(info.sandbox_id, None) + self._sandboxes[info.sandbox_id] = sandbox + self._sandbox_infos[info.sandbox_id] = info + self._last_activity[info.sandbox_id] = time.time() + self._thread_sandboxes[key] = info.sandbox_id + except (OwnershipBackendError, SandboxBeingDestroyedError): + try: + sandbox.close() + except Exception as e: + logger.warning(f"Error closing sandbox {info.sandbox_id} after failed ownership publish: {e}") + raise logger.info(f"Discovered existing sandbox {info.sandbox_id} for user/thread {user_id}/{thread_id} at {info.sandbox_url}") return info.sandbox_id @@ -608,7 +1320,30 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): def _register_created_sandbox(self, thread_id: str | None, sandbox_id: str, info: SandboxInfo, *, user_id: str | None = None) -> str: """Track a newly-created sandbox in the active maps.""" sandbox = AioSandbox(id=sandbox_id, base_url=info.sandbox_url) + # Ownership first. Unlike the discover path there IS something to roll + # back: we just started this container, and an unowned running container + # is exactly what a peer's reconciliation adopts. Leaking it would hand a + # peer a container this instance is about to use. + # SandboxBeingDestroyedError is possible even here: a peer that died + # mid-stop leaves a teardown marker until its TTL lapses. Roll back on + # both, or the container we just started is leaked. + try: + self._publish_ownership(sandbox_id) + except (OwnershipBackendError, SandboxBeingDestroyedError): + logger.error("Could not publish ownership for new sandbox %s; destroying it rather than leaking an unowned container", sandbox_id) + try: + sandbox.close() + except Exception as e: + logger.warning(f"Error closing sandbox {sandbox_id} during ownership rollback: {e}") + try: + self._backend.destroy(info) + except Exception as e: + logger.error("Failed to destroy unowned sandbox %s after ownership failure: %s", sandbox_id, e) + raise + with self._lock: + # Same exclusivity rule as the discover path. + self._warm_pool.pop(sandbox_id, None) self._sandboxes[sandbox_id] = sandbox self._sandbox_infos[sandbox_id] = info self._last_activity[sandbox_id] = time.time() @@ -654,6 +1389,7 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): for key in thread_keys_to_remove: del self._thread_sandboxes[key] self._last_activity.pop(sandbox_id, None) + self._acquire_epoch.pop(sandbox_id, None) if info is None and sandbox_id in self._warm_pool: info, _ = self._warm_pool.pop(sandbox_id) else: @@ -663,6 +1399,19 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): def _drop_unhealthy_sandbox(self, sandbox_id: str, reason: str, *, expected_info: SandboxInfo | None = None) -> None: """Remove and destroy a sandbox after a definitive failed health check.""" + # Reserved for the whole path, not just the stop: this one untracks + # first, so between the untrack and the `del:` claim an acquire misses + # the caches and falls through to discovery, where `take()` still + # succeeds against our own lease. + if not self._reserve_local_teardown(sandbox_id, lambda: True): + logger.info(f"Skipped dropping sandbox {sandbox_id}: already being torn down by this instance") + return + try: + self._drop_unhealthy_reserved(sandbox_id, reason, expected_info=expected_info) + finally: + self._finish_local_teardown(sandbox_id) + + def _drop_unhealthy_reserved(self, sandbox_id: str, reason: str, *, expected_info: SandboxInfo | None = None) -> None: sandbox, info, removed = self._remove_tracked_sandbox(sandbox_id, expected_info=expected_info) if not removed: logger.info(f"Skipped dropping sandbox {sandbox_id}: tracked info changed after health check") @@ -675,10 +1424,23 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): logger.warning(f"Error closing unhealthy sandbox {sandbox_id}: {e}") if info is not None: - try: - self._backend.destroy(info) - except Exception as e: - logger.warning(f"Error destroying unhealthy sandbox {sandbox_id}: {e}") + # Gate this like every other reap path. The container failed a + # definitive health check, but "definitively dead to us" is not proof + # it is ours: a peer may have replaced the container behind this id, + # in which case stopping it is the cross-instance kill again. + if self._claim_ownership(sandbox_id, for_destroy=True): + try: + # Held like the other two stop paths: this one untracks before + # claiming, so `_renew_owned_leases` cannot see the id either + # and nothing else would refresh the marker. The heartbeat + # releases the marker on exit (success or failure), so there is + # no caller-side release to race a late refresh. + with self._held_teardown_lease(sandbox_id): + self._backend.destroy(info) + except Exception as e: + logger.warning(f"Error destroying unhealthy sandbox {sandbox_id}: {e}") + else: + logger.info("Not destroying unhealthy sandbox %s: owned by another instance", sandbox_id) logger.warning(f"Dropped unhealthy sandbox {sandbox_id}: {reason}") @@ -686,18 +1448,71 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): """Return active AIO sandbox count while ``_lock`` is held.""" return len(self._sandboxes) - def _destroy_warm_entry(self, sandbox_id: str, entry: SandboxInfo, *, reason: str) -> None: - """Destroy a warm-pool sandbox using AIO-specific backend logging.""" + def _destroy_warm_entry(self, sandbox_id: str, entry: SandboxInfo, *, reason: str, still_reapable: Callable[[], bool]) -> bool: + """Destroy a warm-pool sandbox using AIO-specific backend logging. + + Claiming for destroy is the exclusion against **peers**: the lease is + marked as a teardown, so a concurrent acquire on another instance is + refused and the container cannot be re-acquired between this decision and + the stop. That pairing is what replaced the per-sandbox flock guard. A + claim that fails — peer-owned or backend unavailable — fails closed and + we do not destroy. + + It is *not* an exclusion against this process: `claim()` succeeds against + our own `own:` lease, so a same-process reclaim that ran before it wins + the container and this stop lands on a turn already using it. The + reservation is that half, and it is taken before the claim — after it, + the entry stays visible in `_warm_pool` for the whole stop, so a reclaim + would otherwise still find it. + + ``still_reapable`` is required rather than defaulting to unconditional: + the safe default is the one that makes a new call site think about it, + and this signature deliberately diverges from ``WarmPoolLifecycleMixin``'s + hook for that reason. Safe because this provider overrides both mixin + callers (``_evict_oldest_warm`` / ``_reap_expired_warm``); if those + overrides were ever dropped, the mixin's call would fail loudly here + rather than silently reopen the window. + + Returns: + ``True`` when the container was stopped and the caller should drop + its warm-pool entry; ``False`` when it is still running. + """ + if not self._reserve_local_teardown(sandbox_id, still_reapable): + logger.info("Refusing to destroy warm-pool sandbox %s for %s: reclaimed by this instance", sandbox_id, reason) + return False + try: - self._backend.destroy(entry) - except Exception as e: - if reason == "idle_timeout": - logger.error(f"Failed to destroy idle warm-pool sandbox {sandbox_id}: {e}") - elif reason == "replica_enforcement": - logger.error(f"Failed to destroy warm-pool sandbox {sandbox_id}: {e}") - else: - logger.error(f"Failed to destroy warm-pool sandbox {sandbox_id} for {reason}: {e}") - return + if not self._claim_ownership(sandbox_id, for_destroy=True): + logger.info("Refusing to destroy warm-pool sandbox %s for %s: owned by another instance", sandbox_id, reason) + return False + + try: + # The marker must outlast the stop, not the TTL it was written with, + # and is released by the heartbeat on exit. On a failed stop that + # release matters just as much — the container is probably still up, + # so a marker left behind would block its thread from re-acquiring it. + with self._held_teardown_lease(sandbox_id): + self._backend.destroy(entry) + except Exception as e: + if reason == "idle_timeout": + logger.error(f"Failed to destroy idle warm-pool sandbox {sandbox_id}: {e}") + elif reason == "replica_enforcement": + logger.error(f"Failed to destroy warm-pool sandbox {sandbox_id}: {e}") + else: + logger.error(f"Failed to destroy warm-pool sandbox {sandbox_id} for {reason}: {e}") + return False + + # Remove the entry here, inside the reservation, rather than leaving + # it to the caller. Releasing the reservation when the stop returns + # and popping afterwards leaves a gap in which the container is + # already stopped, the entry is still in `_warm_pool`, and nothing + # marks it — so a reclaim picks it up and hands out a dead container. + # The pop stays deferred relative to the *stop* (a refused or failed + # stop keeps the entry), just no longer relative to the reservation. + with self._lock: + self._warm_pool.pop(sandbox_id, None) + finally: + self._finish_local_teardown(sandbox_id) if reason == "idle_timeout": logger.info(f"Destroyed idle warm-pool sandbox {sandbox_id}") @@ -705,6 +1520,7 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): logger.info(f"Destroyed warm-pool sandbox {sandbox_id}") else: logger.info(f"Destroyed warm-pool sandbox {sandbox_id} for {reason}") + return True # ── Core: acquire / get / release / shutdown ───────────────────────── @@ -852,7 +1668,10 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): # Docker and perform a health check; keep it off the event loop. discovered = await asyncio.to_thread(self._backend.discover, sandbox_id) if discovered is not None: - return self._register_discovered_sandbox(thread_id, discovered, user_id=effective_user_id) + # Registration publishes ownership, which is blocking store IO + # (filesystem or network depending on the backend) — same reason + # every other step in this coroutine is offloaded. + return await asyncio.to_thread(self._register_discovered_sandbox, thread_id, discovered, user_id=effective_user_id) return await self._create_sandbox_async(thread_id, sandbox_id, user_id=effective_user_id) finally: @@ -911,11 +1730,20 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): await asyncio.to_thread(self._backend.destroy, info) raise RuntimeError(f"Sandbox {sandbox_id} failed to become ready within timeout at {info.sandbox_url}") - return self._register_created_sandbox(thread_id, sandbox_id, info, user_id=effective_user_id) + # Registration publishes ownership (blocking store IO), so it is offloaded + # like every other blocking step on this path. + return await asyncio.to_thread(self._register_created_sandbox, thread_id, sandbox_id, info, user_id=effective_user_id) def get(self, sandbox_id: str) -> Sandbox | None: """Get a sandbox by ID. Updates last activity timestamp. + Stays a pure in-memory lookup: async tool paths call this directly on the + event loop (``ensure_sandbox_initialized_async``), so it must not touch + the ownership store — that is blocking filesystem or network IO depending + on the backend. Ownership is published off the event loop on + acquire/reclaim and refreshed by the renewal thread (see + ``_renew_owned_leases``). + Args: sandbox_id: The ID of the sandbox. @@ -926,7 +1754,7 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): sandbox = self._sandboxes.get(sandbox_id) if sandbox is not None: self._last_activity[sandbox_id] = time.time() - return sandbox + return sandbox def release(self, sandbox_id: str) -> None: """Release a sandbox from active use into the warm pool. @@ -967,6 +1795,20 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): except Exception as e: logger.warning(f"Error closing sandbox {sandbox_id} during release: {e}") + # Keep the lease while warm so a peer cannot adopt+destroy before we + # reclaim, re-establishing it if it lapsed during a long turn. Never + # raises: the turn is already over, so a store problem must not surface + # through after_agent, and the renewal thread (which covers warm entries) + # is the actual guarantee — this only narrows the window. + if info is not None: + # Same staleness as the renewal thread: the refresh is a store round + # trip, and the thread's next turn can reclaim this warm entry while + # it is in flight. Only drop it if nothing re-acquired it since. + epoch = self._acquire_epoch_of(sandbox_id) + if not self._refresh_ownership(sandbox_id): + logger.warning("Sandbox %s is owned by another instance; releasing it from this warm pool", sandbox_id) + self._forget_lost_sandbox(sandbox_id, expected_epoch=epoch) + logger.info(f"Released sandbox {sandbox_id} to warm pool (container still running)") def destroy(self, sandbox_id: str) -> None: @@ -982,6 +1824,33 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): Args: sandbox_id: The ID of the sandbox to destroy. """ + self._destroy_tracked(sandbox_id, still_reapable=lambda: True) + + def _destroy_tracked(self, sandbox_id: str, *, still_reapable: Callable[[], bool]) -> None: + """``destroy()`` with a caller-supplied "is this still reapable" gate. + + Callers that decided to destroy *earlier* (the idle checker) pass their + own predicate so the decision is re-validated in the same critical + section that reserves the teardown. ``destroy()`` itself passes a + constant: an explicit destroy is a decision made now. + """ + if not self._reserve_local_teardown(sandbox_id, still_reapable): + logger.info("Skipping destroy of sandbox %s: re-acquired by this instance or already being torn down", sandbox_id) + return + + try: + self._destroy_reserved(sandbox_id) + finally: + self._finish_local_teardown(sandbox_id) + + def _destroy_reserved(self, sandbox_id: str) -> None: + # Claim before untracking. The reverse order loses the container on a + # refused claim: still running, and no longer in any of our maps, so + # nothing here would ever reap or reclaim it. + if not self._claim_ownership(sandbox_id, for_destroy=True): + logger.warning("Refusing to destroy sandbox %s: owned by another instance", sandbox_id) + return + sandbox, info, _ = self._remove_tracked_sandbox(sandbox_id) if sandbox is not None: @@ -994,8 +1863,20 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): logger.warning(f"Error closing sandbox {sandbox_id} during destroy: {e}") if info: - self._backend.destroy(info) + # The marker must outlast the stop, not the TTL it was written with, + # and the heartbeat releases it on exit — on both outcomes. On a + # failed stop the container is probably still up, so a marker left + # behind would refuse its own thread's `take()` until the TTL lapses; + # the error still propagates out of the `with` (`shutdown()` logs per + # sandbox off it), it is just no longer this method's job to release. + with self._held_teardown_lease(sandbox_id): + self._backend.destroy(info) logger.info(f"Destroyed sandbox {sandbox_id}") + else: + # No container to stop, so no teardown lease was held: clear the + # marker the claim above wrote, so an untracked id cannot leave a + # lease stuck in `del:`. + self._release_ownership(sandbox_id) def shutdown(self) -> None: """Shutdown all sandboxes. Thread-safe and idempotent.""" @@ -1008,6 +1889,10 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): self._warm_pool.clear() self._stop_idle_checker() + # Stop renewing before destroying: the destroy paths claim ownership + # themselves, and a renewal racing them only re-publishes leases we are + # about to drop. + self._stop_lease_renewal() logger.info(f"Shutting down {len(sandbox_ids)} active + {len(warm_items)} warm-pool sandbox(es)") @@ -1018,8 +1903,14 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): logger.error(f"Failed to destroy sandbox {sandbox_id} during shutdown: {e}") for sandbox_id, (info, _) in warm_items: - try: - self._backend.destroy(info) - logger.info(f"Destroyed warm-pool sandbox {sandbox_id} during shutdown") - except Exception as e: - logger.error(f"Failed to destroy warm-pool sandbox {sandbox_id} during shutdown: {e}") + # Route through _destroy_warm_entry so the ownership claim and the + # container stop stay together, as on the idle path. Unconditional + # here: the entries were removed from `_warm_pool` under the lock + # above, so the pool-membership predicate the other callers use would + # refuse every one of them. + self._destroy_warm_entry(sandbox_id, info, reason="shutdown", still_reapable=lambda: True) + + try: + self._ownership.close() + except Exception as e: + logger.warning(f"Error closing sandbox ownership store during shutdown: {e}") diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/local_backend.py b/backend/packages/harness/deerflow/community/aio_sandbox/local_backend.py index 12ab6b032..6876a3ebb 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/local_backend.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/local_backend.py @@ -200,6 +200,11 @@ class LocalContainerBackend(SandboxBackend): - Support for volume mounts and environment variables """ + # Wall clock for a single `stop`. Comfortably above the runtime's own default + # SIGKILL escalation (10s for docker/podman), so this only fires when the + # daemon itself is wedged rather than truncating a slow-but-progressing stop. + _STOP_TIMEOUT_SECONDS = 120.0 + def __init__( self, *, @@ -603,15 +608,30 @@ class LocalContainerBackend(SandboxBackend): raise RuntimeError(f"Failed to start sandbox container: {e.stderr}") def _stop_container(self, container_id: str) -> None: - """Stop a container (--rm ensures automatic removal).""" + """Stop a container (--rm ensures automatic removal). + + The timeout bounds the worst case independently of the ownership layer. + The teardown lease keeps a peer from re-acquiring the container while + this runs, but that exclusion is a lease and can lapse (a store outage + longer than the TTL); an unbounded ``docker stop`` against a wedged + daemon could then outlive it and land on a peer's live container — #4206. + Bounding the stop caps how long that exposure can last even when the + store is perfectly healthy. + """ try: subprocess.run( [self._runtime, "stop", container_id], capture_output=True, text=True, check=True, + timeout=self._STOP_TIMEOUT_SECONDS, ) logger.info(f"Stopped container {container_id} using {self._runtime}") + except subprocess.TimeoutExpired: + # Deliberately not swallowed like a CalledProcessError: the container + # may still be running, so the caller must not report a clean stop. + logger.error(f"Timed out after {self._STOP_TIMEOUT_SECONDS}s stopping container {container_id} using {self._runtime}") + raise except subprocess.CalledProcessError as e: logger.warning(f"Failed to stop container {container_id}: {e.stderr}") diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/__init__.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/__init__.py new file mode 100644 index 000000000..2c04c7300 --- /dev/null +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/__init__.py @@ -0,0 +1,23 @@ +"""Cross-instance ownership leases for shared sandbox containers (#4206).""" + +# NOTE: ``RedisOwnershipStore`` is intentionally NOT imported here. ``redis`` is an +# optional extra, and this package is imported by ``aio_sandbox_provider`` at +# provider construction. Importing ``.redis`` eagerly would couple every AIO +# sandbox install to the redis package even when ownership is memory-only. It is +# imported lazily inside ``make_sandbox_ownership_store`` only when +# ``sandbox.ownership.type == "redis"``. + +from .base import OwnershipBackendError, RenewOutcome, SandboxOwnershipStore +from .factory import compute_lease_ttl, generate_owner_id, make_sandbox_ownership_store, resolve_ownership_config +from .memory import MemoryOwnershipStore + +__all__ = [ + "MemoryOwnershipStore", + "OwnershipBackendError", + "RenewOutcome", + "SandboxOwnershipStore", + "compute_lease_ttl", + "generate_owner_id", + "make_sandbox_ownership_store", + "resolve_ownership_config", +] diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/base.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/base.py new file mode 100644 index 000000000..37ae7bf23 --- /dev/null +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/base.py @@ -0,0 +1,188 @@ +"""Ownership store contract for shared sandbox containers (#4206). + +Gateway instances share sandbox containers but each keeps its own in-memory warm +pool. Without shared ownership state, one instance's startup reconciliation +adopts a container another instance is actively using and later idle-destroys it, +so tool calls fail with 502 / connection refused. + +A lease answers "**which instance is responsible for reaping this container?**", +not "which instance may use it". That distinction drives the whole interface: + +* A container is deterministic per (user, thread), so consecutive turns of one + thread legitimately land on different instances. The instance now serving the + thread :meth:`take` s the lease from whoever held it — refusing because a peer + still held it would strand the thread until that lease expired. +* Reaping is the opposite. :meth:`claim` succeeds only when the container is + unowned or already ours, so an instance can never adopt (and later + idle-destroy) a container a live peer is responsible for. That is #4206. + +**A lease has two states, and that is what makes the destroy window safe.** +`own:` means "I am responsible for this container"; `del:` means "I am tearing +this container down". A takeover (:meth:`take`) is refused against a `del:` +lease, so a container cannot be re-acquired between a destroy path's claim and +its container stop — the window the deleted per-sandbox flock guard used to +cover. Without the two states an unconditional `take` would silently overwrite a +destroyer's claim and the peer's stop would land on a container the new owner had +already handed to an agent. + +Contract notes for implementers: + +* Every method is **synchronous**. Unlike ``StreamBridge`` (whose async API exists + because it is driven from the event loop), ownership is driven from + ``AioSandboxProvider.__init__``, the background idle/renewal threads, and the + sync ``release()`` path. Sandbox tool paths that *do* run on the event loop + (``get()``) deliberately never touch the store, and async acquire paths offload + registration through ``asyncio.to_thread``. +* Methods **raise** ``OwnershipBackendError`` on backend failure rather than + returning a falsy value. Callers must fail closed: a sandbox whose ownership + could not be published is not safe to hand out, and a container whose ownership + cannot be proven free is not safe to destroy. A ``False`` return means + "definitively not ours"; raising means "unknown". +""" + +from __future__ import annotations + +import abc +import enum + + +class OwnershipBackendError(RuntimeError): + """The ownership backend could not answer. + + Distinct from a definitive "not ours" (``False``): this means ownership is + *unknown*, so callers must fail closed rather than assume the container is + free. + """ + + +class RenewOutcome(enum.Enum): + """Why a renewal did or did not succeed. + + ``LAPSED`` and ``LOST`` must not be collapsed into one falsy value. A lapsed + lease is *absent* — nobody took it, so re-establishing it is safe and is what + keeps a Redis restart from dropping every live sandbox fleet-wide. A lost + lease belongs to a peer, and re-taking it is the #4206 cross-instance kill. + """ + + #: Still ours; TTL refreshed. + RENEWED = "renewed" + #: No lease present (expired, or the store lost its state). Free to re-claim. + LAPSED = "lapsed" + #: A peer holds it, or it is being torn down. Do not re-take. + LOST = "lost" + + +class SandboxOwnershipStore(abc.ABC): + """Cross-instance ownership leases for sandbox containers.""" + + #: Whether this store coordinates instances beyond the current process. + #: ``False`` means peers cannot see our leases, so every container looks like + #: an orphan to them — single-instance deployments only. + supports_cross_process: bool = False + + @property + @abc.abstractmethod + def owner_id(self) -> str: + """This instance's owner id, as written into leases.""" + + @abc.abstractmethod + def take(self, sandbox_id: str) -> bool: + """Take responsibility for *sandbox_id* on the acquire path. + + Takes over from a live peer: a turn for this container's thread has + routed here, and the previous owner learns to stop tracking it when its + next renewal reports ``LOST``. It must not destroy it — see + ``AioSandboxProvider._forget_lost_sandbox``. + + Refuses only a container that is being torn down, which is what closes + the destroy → re-acquire window. + + Returns: + ``True`` when this instance owns the lease afterwards. + ``False`` when the container is being destroyed and must not be used. + + Raises: + OwnershipBackendError: ownership could not be published. Callers must + fail closed — an unpublished sandbox is not safe to hand out, + because peers will see it as an orphan. + """ + + @abc.abstractmethod + def claim(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + """Take ownership of *sandbox_id* only if it is unowned or already ours. + + Exclusive: succeeds only when the container is unowned or already ours, + which is what gates every adopt/reap path. + + Exclusive against **peers**, not against the caller's own process: a + claim against our own ``own:`` lease succeeds by design, which is what + lets a destroy path claim what it already owns. Same-process exclusion + between an instance's reaper threads and its own acquire path is the + provider's job, not this store's (``_reserve_local_teardown``). + + One exception, so ``for_destroy`` cannot be silently unwound: a + **non**-destroy claim against our own ``del:`` lease is refused. The stop + it marks is already in flight and cannot be recalled, so downgrading the + marker would let a :meth:`take` hand out a container that is about to + die. + + The read-modify-write must not interleave. On redis that is Lua (one + script, server-side); the memory store serializes on a process-local lock + and is single-instance anyway, so "different instances" cannot arise + there. Note what is *not* verified: the contract suite drives sequential + calls, so it pins the exclusion predicate, not the atomicity — and CI + runs the memory tier only, so the Lua that carries it never executes on + the merge gate. + + Args: + for_destroy: mark the lease as a teardown in progress, so a + concurrent :meth:`take` is refused for as long as it is held. + Destroy paths must set this; the marker is cleared by + :meth:`release` once the container is stopped, and expires with + the TTL if the destroyer dies mid-stop. + + Returns: + ``True`` when this instance owns the lease afterwards. + ``False`` when a live peer holds it. + + Raises: + OwnershipBackendError: ownership could not be determined. + """ + + @abc.abstractmethod + def renew(self, sandbox_id: str) -> RenewOutcome: + """Refresh our lease on *sandbox_id*. + + Deliberately does not re-acquire on its own — the caller decides, because + only the caller can tell a safe re-establish (``LAPSED``) from a + cross-instance steal (``LOST``). + + Raises: + OwnershipBackendError: ownership could not be determined. + """ + + @abc.abstractmethod + def release(self, sandbox_id: str) -> None: + """Drop our lease on *sandbox_id*, in either state. + + A no-op when the lease is not ours, so a peer's live lease is never + cleared. Best-effort: an expiring lease reaches the same state. + + Raises: + OwnershipBackendError: the release could not be published. + """ + + @abc.abstractmethod + def owner(self, sandbox_id: str) -> str | None: + """Return the current owner id of *sandbox_id*, or ``None`` if unowned. + + Read-only: unlike :meth:`claim`, this never takes ownership. Use it to + inspect (tests, logging) rather than to gate a destroy — a read is stale + the moment it returns, whereas a successful claim keeps peers out. + + Raises: + OwnershipBackendError: ownership could not be read. + """ + + def close(self) -> None: + """Release backend resources. Default is a no-op.""" diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/factory.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/factory.py new file mode 100644 index 000000000..3070f8c21 --- /dev/null +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/factory.py @@ -0,0 +1,108 @@ +"""Resolve the configured sandbox ownership store. + +Mirrors ``stream_bridge``'s ``make_stream_bridge``: dispatch on ``config.type``, +lazy per-branch imports so a memory-only install never imports ``redis``, and an +env-var escape hatch so a container deployment can flip the backend without +editing config.yaml. +""" + +from __future__ import annotations + +import logging +import os +import socket +import uuid + +from deerflow.config.sandbox_config import SandboxOwnershipConfig +from deerflow.config.stream_bridge_config import StreamBridgeConfig + +from .base import SandboxOwnershipStore + +logger = logging.getLogger(__name__) + +_ENV_OWNERSHIP_REDIS_URL = "DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL" +_ENV_STREAM_BRIDGE_REDIS_URL = "DEER_FLOW_STREAM_BRIDGE_REDIS_URL" + + +def generate_owner_id() -> str: + """Return a unique id for this provider instance: ``hostname:hex``. + + Per-instance, not per-host: two gateway workers on one host must be able to + tell their leases apart. + """ + return f"{socket.gethostname()}:{uuid.uuid4().hex}" + + +def resolve_ownership_config(config: SandboxOwnershipConfig | None, *, stream_bridge: StreamBridgeConfig | None = None) -> SandboxOwnershipConfig: + """Fill in an omitted ownership section. + + A deployment that already points the stream bridge at Redis is by definition + multi-instance, so it gets a redis ownership store rather than silently + falling back to memory (which cannot see peers and would leave #4206 open). + + Both of the stream bridge's own redis triggers are honoured, and in its + order (``stream_bridge/async_provider.py::_resolve_config``): the config.yaml + section first, then the env var. Reading only the env var would miss the + config.yaml-native way of pointing the bridge at Redis — i.e. exactly the + multi-instance deployments this inference exists for. + """ + if config is not None: + return config + + if stream_bridge is not None and stream_bridge.type == "redis": + redis_url = stream_bridge.redis_url or os.getenv(_ENV_OWNERSHIP_REDIS_URL) or os.getenv(_ENV_STREAM_BRIDGE_REDIS_URL) + logger.info("Sandbox ownership: redis inferred from stream_bridge.type (multi-instance deployment)") + return SandboxOwnershipConfig(type="redis", redis_url=redis_url) + + redis_url = os.getenv(_ENV_OWNERSHIP_REDIS_URL) or os.getenv(_ENV_STREAM_BRIDGE_REDIS_URL) + if redis_url: + logger.info("Sandbox ownership: redis inferred from environment (multi-instance deployment)") + return SandboxOwnershipConfig(type="redis", redis_url=redis_url) + return SandboxOwnershipConfig() + + +def _resolve_redis_url(config: SandboxOwnershipConfig) -> str: + return config.redis_url or os.getenv(_ENV_OWNERSHIP_REDIS_URL) or os.getenv(_ENV_STREAM_BRIDGE_REDIS_URL) or os.getenv("REDIS_URL") or "redis://localhost:6379/0" + + +def compute_lease_ttl(config: SandboxOwnershipConfig) -> float: + """Lease TTL in seconds. + + Derived from the renewal interval, never from ``sandbox.idle_timeout``: + coupling liveness to the idle reaper is what let ownership lapse under + ``idle_timeout: 0``, where the idle checker never starts. + """ + return config.renewal_interval_seconds * config.ttl_multiplier + + +def make_sandbox_ownership_store(config: SandboxOwnershipConfig | None, *, owner_id: str | None = None) -> SandboxOwnershipStore: + """Build the ownership store for *config*. + + Caller owns the returned store and must ``close()`` it. + """ + # Trust an already-resolved config; only fill in an omitted section. The + # provider resolves once (with the stream_bridge inference this factory + # cannot do) and passes that in, so re-resolving here would be a no-op. + resolved = config if config is not None else resolve_ownership_config(None) + effective_owner_id = owner_id or generate_owner_id() + ttl = compute_lease_ttl(resolved) + + if resolved.type == "memory": + from .memory import MemoryOwnershipStore + + logger.info("Sandbox ownership store: memory (single-instance; ttl=%.1fs)", ttl) + return MemoryOwnershipStore(owner_id=effective_owner_id, ttl_seconds=ttl) + + if resolved.type == "redis": + from .redis import RedisOwnershipStore + + redis_url = _resolve_redis_url(resolved) + logger.info("Sandbox ownership store: redis (ttl=%.1fs, renewal=%.1fs)", ttl, resolved.renewal_interval_seconds) + return RedisOwnershipStore( + owner_id=effective_owner_id, + redis_url=redis_url, + ttl_seconds=ttl, + key_prefix=resolved.key_prefix, + ) + + raise ValueError(f"Unknown sandbox ownership type: {resolved.type!r}") diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/memory.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/memory.py new file mode 100644 index 000000000..e30bd5be2 --- /dev/null +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/memory.py @@ -0,0 +1,106 @@ +"""In-process ownership store for single-instance deployments. + +Correct only when one gateway process owns the container backend: nothing here is +visible to another process, so a peer would see every container as unowned and +adopt it. :attr:`supports_cross_process` is ``False`` to say so, and the provider +warns at startup. Multi-worker / multi-instance gateways must use the redis +store — the same rule `stream_bridge`'s memory backend carries. + +TTL and the two lease states are implemented for real rather than stubbed out, so +one store-contract suite exercises both backends and a lapsed lease behaves +identically whichever store is configured. +""" + +from __future__ import annotations + +import threading +import time +from dataclasses import dataclass + +from .base import RenewOutcome, SandboxOwnershipStore + + +@dataclass(frozen=True, slots=True) +class _Lease: + owner_id: str + expires_at: float + destroying: bool + + +class MemoryOwnershipStore(SandboxOwnershipStore): + """Ownership leases held in this process only.""" + + supports_cross_process = False + + def __init__(self, *, owner_id: str, ttl_seconds: float, time_source=time.monotonic) -> None: + self._owner_id = owner_id + self._ttl = float(ttl_seconds) + self._now = time_source + # sandbox_id -> _Lease. Guarded by _lock: the acquire path, the idle + # checker thread, and the renewal thread all touch it. + self._leases: dict[str, _Lease] = {} + self._lock = threading.Lock() + + @property + def owner_id(self) -> str: + return self._owner_id + + def _live_lease_locked(self, sandbox_id: str) -> _Lease | None: + lease = self._leases.get(sandbox_id) + if lease is None: + return None + if self._now() >= lease.expires_at: + del self._leases[sandbox_id] + return None + return lease + + def _write_locked(self, sandbox_id: str, *, destroying: bool) -> None: + self._leases[sandbox_id] = _Lease(owner_id=self._owner_id, expires_at=self._now() + self._ttl, destroying=destroying) + + def take(self, sandbox_id: str) -> bool: + with self._lock: + lease = self._live_lease_locked(sandbox_id) + # Refuse only a teardown in progress; a live peer's normal lease is + # taken over, which is the point of take(). + if lease is not None and lease.destroying: + return False + self._write_locked(sandbox_id, destroying=False) + return True + + def claim(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + with self._lock: + lease = self._live_lease_locked(sandbox_id) + if lease is not None and lease.owner_id != self._owner_id: + return False + if not for_destroy and lease is not None and lease.destroying: + # Never unwind our own teardown: the stop is already in flight + # and cannot be recalled, so downgrading to `own:` would let a + # `take()` hand out a container that is about to die. + return False + self._write_locked(sandbox_id, destroying=for_destroy) + return True + + def renew(self, sandbox_id: str) -> RenewOutcome: + with self._lock: + lease = self._live_lease_locked(sandbox_id) + if lease is None: + return RenewOutcome.LAPSED + if lease.owner_id != self._owner_id or lease.destroying: + return RenewOutcome.LOST + self._write_locked(sandbox_id, destroying=False) + return RenewOutcome.RENEWED + + def release(self, sandbox_id: str) -> None: + with self._lock: + lease = self._live_lease_locked(sandbox_id) + if lease is not None and lease.owner_id == self._owner_id: + del self._leases[sandbox_id] + + def owner(self, sandbox_id: str) -> str | None: + with self._lock: + lease = self._live_lease_locked(sandbox_id) + return None if lease is None else lease.owner_id + + def close(self) -> None: + with self._lock: + self._leases.clear() diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py new file mode 100644 index 000000000..923ab563d --- /dev/null +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py @@ -0,0 +1,214 @@ +"""Redis-backed ownership store for multi-instance gateways (#4206). + +Ownership is a single key per sandbox whose value encodes both the owner and the +lease state — ``own:`` (responsible for this container) or +``del:`` (tearing it down) — with a TTL the owning instance refreshes. + +The state prefix is what makes the destroy window safe without a lock: a +takeover is refused against a ``del:`` lease, so a container cannot be +re-acquired between a destroy path's claim and its container stop. + +The sync client is deliberate: this store is driven from provider construction +and from background threads, never from the event loop (see ``base`` for the +contract). ``redis.asyncio`` would be the wrong client here. + +Every mutation goes through a Lua script so the read and the write cannot be +interleaved by a peer. ``SET NX`` alone is not enough: it fails on a key we +already own, and a GET-then-SET fallback in Python reopens the race the script +closes. +""" + +from __future__ import annotations + +import logging + +from .base import OwnershipBackendError, RenewOutcome, SandboxOwnershipStore + +try: + from redis import Redis + from redis.exceptions import RedisError +except ImportError: # pragma: no cover - only hit when the optional extra is missing + # ``redis`` is an optional extra (mirrors the stream_bridge redis path). This + # module is imported lazily from ``make_sandbox_ownership_store`` only when + # ``sandbox.ownership.type == "redis"``, so this hint surfaces exactly when a + # redis ownership store is requested without the package. + raise ImportError( + "sandbox.ownership.type is set to 'redis' but the redis package is not installed.\n" + "Install it with:\n" + " cd backend && uv sync --all-packages --extra redis\n" + "On the next `make dev` the redis extra is auto-detected from config.yaml\n" + "(sandbox.ownership.type: redis) and reinstalled, so it will not be wiped again.\n" + "Or switch to sandbox.ownership.type: memory in config.yaml for single-instance deployment." + ) from None + +logger = logging.getLogger(__name__) + +_OWN = "own:" +_DEL = "del:" + +# Bound every store round-trip so a stalled Redis cannot wedge a caller. This +# matters most for the teardown heartbeat: its exit — and the final lease +# release that exit performs — must stay finite, otherwise a refresh blocked on +# a black-holed connection could hold a destroy path (and its deferred release) +# open indefinitely. Without a socket timeout redis-py blocks forever. +_STORE_SOCKET_TIMEOUT_SECONDS = 5.0 + +# Acquire-path takeover. Overwrites a live peer's normal lease on purpose — a +# thread's turn has routed here — but refuses a teardown in progress, which is +# what stops us handing out a container a peer is about to stop. +_TAKE_SCRIPT = """ +local current = redis.call('GET', KEYS[1]) +if current ~= false and string.sub(current, 1, 4) == 'del:' then + return 0 +end +redis.call('SET', KEYS[1], 'own:' .. ARGV[1], 'PX', ARGV[2]) +return 1 +""" + +# Adopt/reap gate: only if unowned or already ours (in either state). +# ARGV[3] selects the state written: '1' marks a teardown in progress. +# +# A non-destroy claim never unwinds our *own* teardown: a stop is already in +# flight and cannot be recalled, so downgrading the marker to `own:` would let a +# `take()` hand out a container that is about to die. No caller does this today +# (the `for_destroy=false` callers run against an absent or unowned key), but the +# contract has to forbid it rather than rely on that staying true. +_CLAIM_SCRIPT = """ +local current = redis.call('GET', KEYS[1]) +local mine_own = 'own:' .. ARGV[1] +local mine_del = 'del:' .. ARGV[1] +if ARGV[3] == '0' and current == mine_del then + return 0 +end +if current == false or current == mine_own or current == mine_del then + local value = mine_own + if ARGV[3] == '1' then + value = mine_del + end + redis.call('SET', KEYS[1], value, 'PX', ARGV[2]) + return 1 +end +return 0 +""" + +# Three-way so the caller can tell an absent lease (safe to re-establish) from a +# peer's (re-taking it is the #4206 kill). Collapsing them is what let a Redis +# restart drop every live sandbox fleet-wide. +# 1 = renewed, -1 = lapsed/absent, 0 = held by a peer or being torn down +_RENEW_SCRIPT = """ +local current = redis.call('GET', KEYS[1]) +if current == false then + return -1 +end +if current == 'own:' .. ARGV[1] then + redis.call('PEXPIRE', KEYS[1], ARGV[2]) + return 1 +end +return 0 +""" + +# Drop only our own lease, in either state, so a peer's is never cleared. +_RELEASE_SCRIPT = """ +local current = redis.call('GET', KEYS[1]) +if current == 'own:' .. ARGV[1] or current == 'del:' .. ARGV[1] then + return redis.call('DEL', KEYS[1]) +end +return 0 +""" + + +class RedisOwnershipStore(SandboxOwnershipStore): + """Ownership leases shared across gateway instances via Redis.""" + + supports_cross_process = True + + def __init__( + self, + *, + owner_id: str, + redis_url: str, + ttl_seconds: float, + key_prefix: str = "deerflow:sandbox:owner", + client: Redis | None = None, + ) -> None: + self._owner_id = owner_id + self._ttl_ms = max(1, int(float(ttl_seconds) * 1000)) + self._key_prefix = key_prefix.rstrip(":") + # Redis.from_url is lazy, so an unreachable Redis does not block provider + # construction; the first claim raises instead. socket_timeout bounds + # every round-trip (see _STORE_SOCKET_TIMEOUT_SECONDS) so no store call — + # in particular a teardown-heartbeat refresh — can block unbounded. + self._redis = ( + client + if client is not None + else Redis.from_url( + redis_url, + decode_responses=True, + socket_timeout=_STORE_SOCKET_TIMEOUT_SECONDS, + socket_connect_timeout=_STORE_SOCKET_TIMEOUT_SECONDS, + ) + ) + self._owns_client = client is None + self._take = self._redis.register_script(_TAKE_SCRIPT) + self._claim = self._redis.register_script(_CLAIM_SCRIPT) + self._renew = self._redis.register_script(_RENEW_SCRIPT) + self._release = self._redis.register_script(_RELEASE_SCRIPT) + + @property + def owner_id(self) -> str: + return self._owner_id + + def _key(self, sandbox_id: str) -> str: + return f"{self._key_prefix}:{sandbox_id}" + + def take(self, sandbox_id: str) -> bool: + try: + result = self._take(keys=[self._key(sandbox_id)], args=[self._owner_id, self._ttl_ms]) + except RedisError as e: + raise OwnershipBackendError(f"failed to publish sandbox ownership for {sandbox_id}: {e}") from e + return bool(result) + + def claim(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + try: + result = self._claim(keys=[self._key(sandbox_id)], args=[self._owner_id, self._ttl_ms, "1" if for_destroy else "0"]) + except RedisError as e: + raise OwnershipBackendError(f"failed to claim sandbox ownership for {sandbox_id}: {e}") from e + return bool(result) + + def renew(self, sandbox_id: str) -> RenewOutcome: + try: + result = int(self._renew(keys=[self._key(sandbox_id)], args=[self._owner_id, self._ttl_ms])) + except RedisError as e: + raise OwnershipBackendError(f"failed to renew sandbox ownership for {sandbox_id}: {e}") from e + if result == 1: + return RenewOutcome.RENEWED + if result == -1: + return RenewOutcome.LAPSED + return RenewOutcome.LOST + + def release(self, sandbox_id: str) -> None: + try: + self._release(keys=[self._key(sandbox_id)], args=[self._owner_id]) + except RedisError as e: + raise OwnershipBackendError(f"failed to release sandbox ownership for {sandbox_id}: {e}") from e + + def owner(self, sandbox_id: str) -> str | None: + try: + value = self._redis.get(self._key(sandbox_id)) + except RedisError as e: + raise OwnershipBackendError(f"failed to read sandbox ownership for {sandbox_id}: {e}") from e + if value is None: + return None + # An injected client may not set decode_responses. + text = value.decode("utf-8") if isinstance(value, bytes) else value + if text.startswith(_OWN) or text.startswith(_DEL): + return text[4:] + return text + + def close(self) -> None: + if not self._owns_client: + return + try: + self._redis.close() + except Exception as e: # pragma: no cover - teardown best effort + logger.warning("Error closing sandbox ownership redis client: %s", e) diff --git a/backend/packages/harness/deerflow/config/sandbox_config.py b/backend/packages/harness/deerflow/config/sandbox_config.py index 992f8a7eb..218f00b73 100644 --- a/backend/packages/harness/deerflow/config/sandbox_config.py +++ b/backend/packages/harness/deerflow/config/sandbox_config.py @@ -1,5 +1,48 @@ +from typing import Literal + from pydantic import BaseModel, ConfigDict, Field +SandboxOwnershipType = Literal["memory", "redis"] + + +class SandboxOwnershipConfig(BaseModel): + """Configuration for cross-instance sandbox container ownership (#4206). + + Gateway instances share sandbox containers but each keeps its own in-memory + warm pool. Without shared ownership state, one instance's reconciliation + adopts another's live container and later idle-destroys it. This selects + where that ownership state lives. + """ + + type: SandboxOwnershipType = Field( + default="memory", + description=( + "Sandbox ownership store backend. 'memory' keeps ownership in-process (single-instance deployments only, where cross-instance adoption cannot occur). " + "'redis' shares ownership across gateway instances and is required for load-balanced / multi-worker deployments that share a container backend." + ), + ) + redis_url: str | None = Field( + default=None, + description="Redis URL for the redis ownership type. If omitted, DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL, DEER_FLOW_STREAM_BRIDGE_REDIS_URL, REDIS_URL, or redis://localhost:6379/0 is used.", + ) + renewal_interval_seconds: float = Field( + default=30.0, + gt=0, + description=( + "How often an owning instance refreshes its leases. The lease TTL is derived from this (interval x ttl_multiplier), so ownership liveness is independent of sandbox.idle_timeout: " + "renewal keeps running even when idle cleanup is disabled (idle_timeout: 0)." + ), + ) + ttl_multiplier: float = Field( + default=4.0, + ge=2, + description="Lease TTL as a multiple of renewal_interval_seconds. At least 2, so a single missed renewal (slow host, brief Redis blip) cannot expire a live owner's lease. Default 4 tolerates three consecutive misses.", + ) + key_prefix: str = Field( + default="deerflow:sandbox:owner", + description="Redis key prefix for ownership leases. Only applies to the redis ownership type.", + ) + class VolumeMountConfig(BaseModel): """Configuration for a volume mount.""" @@ -43,6 +86,8 @@ class SandboxConfig(BaseModel): port: Base port for sandbox containers (default: 8080) container_prefix: Prefix for container names (default: deer-flow-sandbox) mounts: List of volume mounts to share directories with the container + ownership: Cross-instance container ownership store (memory | redis). Multi-instance + deployments sharing a container backend need redis; see SandboxOwnershipConfig. """ use: str = Field( @@ -78,6 +123,13 @@ class SandboxConfig(BaseModel): ge=0, description="BoxLite-only reclaim skip window in seconds for boxes recently released by this provider instance. Set to 0 to always validate before warm reuse.", ) + ownership: SandboxOwnershipConfig | None = Field( + default=None, + description=( + "AioSandboxProvider-only: where cross-instance container ownership is tracked (#4206). Omitted = memory (single-instance). " + "Multi-worker / load-balanced gateways sharing one container backend must set type: redis, or peers will adopt and idle-destroy each other's live sandboxes." + ), + ) mounts: list[VolumeMountConfig] = Field( default_factory=list, description="List of volume mounts to share directories between host and container", diff --git a/backend/tests/blocking_io/test_aio_sandbox_get.py b/backend/tests/blocking_io/test_aio_sandbox_get.py new file mode 100644 index 000000000..4f6e11761 --- /dev/null +++ b/backend/tests/blocking_io/test_aio_sandbox_get.py @@ -0,0 +1,155 @@ +"""Regression: ``AioSandboxProvider.get()`` must not do blocking IO. + +``ensure_sandbox_initialized_async`` (``sandbox/tools.py``) calls +``provider.get()`` directly on the LangGraph event loop for every sandbox tool +lookup. A prior change renewed the cross-process lease inside ``get()`` +(``mkdir`` + temp-file write + ``fsync`` + ``os.replace``), which blocks the loop +— reported on PR #4221. + +Under the strict Blockbuster context (this directory's conftest), any blocking IO +reached from ``deerflow.*`` while on the event loop raises ``BlockingError``. + +The ownership store is injected here as a **blocking probe**: every store method +does real file IO. That keeps the anchor honest across backends — the configured +store may be in-memory (no IO to catch), but the redis store does network IO and a +future store could do anything, so what must be pinned is that ``get()`` performs +*no store call at all*, not merely that today's default store happens to be cheap. +If ownership work is put back on this path, this test fails. +""" + +from __future__ import annotations + +import threading +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +pytestmark = pytest.mark.asyncio + + +class _BlockingProbeStore: + """Ownership store whose every operation does real blocking file IO.""" + + supports_cross_process = True + + def __init__(self, probe_path: Path): + self._probe_path = probe_path + self._probe_path.write_text("owner", encoding="utf-8") + + @property + def owner_id(self) -> str: + return "worker-blockingio" + + def _blocking_touch(self) -> str: + # Mirrors what a real store does on this call: sync IO the strict gate sees. + return self._probe_path.read_text(encoding="utf-8") + + def take(self, sandbox_id: str) -> bool: + self._blocking_touch() + return True + + def claim(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + self._blocking_touch() + return True + + def renew(self, sandbox_id: str): + from deerflow.community.aio_sandbox.ownership import RenewOutcome + + self._blocking_touch() + return RenewOutcome.RENEWED + + def release(self, sandbox_id: str) -> None: + self._blocking_touch() + + def owner(self, sandbox_id: str) -> str | None: + return self._blocking_touch() + + def close(self) -> None: + pass + + +def _make_provider(tmp_path: Path): + """Build an ``AioSandboxProvider`` without ``__init__`` (no Docker, no threads).""" + from deerflow.community.aio_sandbox.aio_sandbox_provider import AioSandboxProvider + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + provider = AioSandboxProvider.__new__(AioSandboxProvider) + provider._lock = threading.Lock() + provider._sandboxes = {} + provider._sandbox_infos = {} + provider._thread_sandboxes = {} + provider._thread_locks = {} + provider._last_activity = {} + provider._warm_pool = {} + provider._local_teardown = set() + provider._acquire_epoch = {} + provider._acquire_epoch_counter = 0 + provider._acquire_inflight = {} + provider._shutdown_called = False + provider._idle_checker_stop = threading.Event() + provider._idle_checker_thread = None + provider._renewal_stop = threading.Event() + provider._renewal_thread = None + provider._config = {"idle_timeout": 600, "replicas": 3} + provider._backend = MagicMock() + provider._owner_id = "worker-blockingio" + provider._ownership_config = SandboxOwnershipConfig() + provider._ownership = _BlockingProbeStore(tmp_path / "ownership-probe") + return provider + + +async def test_get_does_no_blocking_io_on_event_loop(tmp_path): + provider = _make_provider(tmp_path) + provider._sandboxes["sb-blockingio"] = MagicMock() + + # If get() touches the ownership store, the probe's file read trips the gate. + assert provider.get("sb-blockingio") is not None + + +async def test_blocking_probe_store_actually_trips_the_gate(tmp_path): + """Meta-check: prove the probe has teeth, so the test above is not vacuous. + + Without this, a store that silently stopped doing IO would make the anchor + pass for the wrong reason. + """ + from blockbuster import BlockingError + + provider = _make_provider(tmp_path) + + with pytest.raises(BlockingError): + provider._publish_ownership("sb-blockingio") + + +async def test_async_acquire_offloads_ownership_publish(tmp_path, monkeypatch): + """The async acquire paths must offload registration, not just discovery. + + ``_register_discovered_sandbox`` / ``_register_created_sandbox`` publish + ownership, which is blocking store IO. Every other blocking step in + ``_discover_or_create_with_lock_async`` is wrapped in ``asyncio.to_thread``; + these two were called directly, putting a Redis round trip on the event loop + for every discover/create. + """ + import deerflow.community.aio_sandbox.aio_sandbox_provider as aio_mod + from deerflow.community.aio_sandbox.sandbox_info import SandboxInfo + + provider = _make_provider(tmp_path) + info = SandboxInfo( + sandbox_id="sb-async", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-sb-async", + created_at=1.0, + ) + provider._backend.discover = MagicMock(return_value=info) + + # Stub the path layer: `get_paths()` resolves the base dir via os.getcwd on + # the event loop, which is a pre-existing blocking call in this coroutine and + # not what this anchor is about. Scoping it out keeps the test pinned to the + # ownership publish this diff added. + fake_paths = MagicMock() + fake_paths.thread_dir.return_value = tmp_path + monkeypatch.setattr(aio_mod, "get_paths", lambda: fake_paths) + + sandbox_id = await provider._discover_or_create_with_lock_async("t-async", "sb-async", user_id="u1") + + assert sandbox_id == "sb-async" diff --git a/backend/tests/blocking_io/test_sandbox_release.py b/backend/tests/blocking_io/test_sandbox_release.py new file mode 100644 index 000000000..2dcd4b6b7 --- /dev/null +++ b/backend/tests/blocking_io/test_sandbox_release.py @@ -0,0 +1,141 @@ +"""Regression anchor: sandbox release must not block the event loop. + +``AioSandboxProvider.release()`` refreshes the ownership lease +(``_refresh_ownership`` -> store ``renew``/``claim``), which is blocking +filesystem or network IO depending on the backend. It runs from +``SandboxMiddleware`` at the end of every turn: the async gateway path +(``aafter_agent``) offloads it with ``asyncio.to_thread``, so the store round +trip stays off the loop. This pins that offload — a refactor that dropped it +(or wired ``aafter_agent`` to call ``release`` directly) would put a Redis round +trip on the event loop for sync graph execution, as flagged in review of +PR #4221. + +The ownership store is injected as a **blocking probe** whose every method does +real file IO, so the anchor keeps its teeth regardless of the configured backend +(the default ``memory`` store does no IO to catch; redis does network IO). +""" + +from __future__ import annotations + +import threading +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +pytestmark = pytest.mark.asyncio + + +class _BlockingProbeStore: + """Ownership store whose every operation does real blocking file IO.""" + + supports_cross_process = True + + def __init__(self, probe_path: Path): + self._probe_path = probe_path + self._probe_path.write_text("owner", encoding="utf-8") + + @property + def owner_id(self) -> str: + return "worker-blockingio" + + def _blocking_touch(self) -> str: + return self._probe_path.read_text(encoding="utf-8") + + def take(self, sandbox_id: str) -> bool: + self._blocking_touch() + return True + + def claim(self, sandbox_id: str, *, for_destroy: bool = False) -> bool: + self._blocking_touch() + return True + + def renew(self, sandbox_id: str): + from deerflow.community.aio_sandbox.ownership import RenewOutcome + + self._blocking_touch() + return RenewOutcome.RENEWED + + def release(self, sandbox_id: str) -> None: + self._blocking_touch() + + def owner(self, sandbox_id: str) -> str | None: + return self._blocking_touch() + + def close(self) -> None: + pass + + +def _make_provider_with_active_sandbox(tmp_path: Path, sandbox_id: str): + """A real provider (no ``__init__``) holding one active sandbox to release.""" + from deerflow.community.aio_sandbox.aio_sandbox_provider import AioSandboxProvider + from deerflow.community.aio_sandbox.sandbox_info import SandboxInfo + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + provider = AioSandboxProvider.__new__(AioSandboxProvider) + provider._lock = threading.Lock() + provider._sandboxes = {sandbox_id: MagicMock()} + provider._sandbox_infos = { + sandbox_id: SandboxInfo( + sandbox_id=sandbox_id, + sandbox_url="http://localhost:8080", + container_name=f"deer-flow-sandbox-{sandbox_id}", + created_at=1.0, + ) + } + provider._thread_sandboxes = {} + provider._thread_locks = {} + provider._last_activity = {sandbox_id: 1.0} + provider._warm_pool = {} + provider._unowned_since = {} + provider._local_teardown = set() + provider._acquire_epoch = {} + provider._acquire_epoch_counter = 0 + provider._acquire_inflight = {} + provider._shutdown_called = False + provider._idle_checker_stop = threading.Event() + provider._idle_checker_thread = None + provider._renewal_stop = threading.Event() + provider._renewal_thread = None + provider._config = {"idle_timeout": 600, "replicas": 3} + provider._backend = MagicMock() + provider._owner_id = "worker-blockingio" + provider._ownership_config = SandboxOwnershipConfig() + provider._ownership = _BlockingProbeStore(tmp_path / "ownership-probe") + return provider + + +async def test_aafter_agent_offloads_release_off_the_event_loop(tmp_path, monkeypatch): + """The async release hook must keep the ownership-store round trip off-loop. + + If it regresses to calling ``release`` directly, the probe's file IO trips + the strict Blockbuster gate. + """ + import deerflow.sandbox.middleware as mw_mod + + provider = _make_provider_with_active_sandbox(tmp_path, "sb-release") + monkeypatch.setattr(mw_mod, "get_sandbox_provider", lambda: provider) + + mw = mw_mod.SandboxMiddleware() + state = {"sandbox": {"sandbox_id": "sb-release"}} + + # Offloaded via asyncio.to_thread, so no BlockingError under the strict gate. + await mw.aafter_agent(state, MagicMock()) + + # The release actually happened (parked in the warm pool), so the anchor is + # exercising the real path, not a no-op. + assert "sb-release" in provider._warm_pool + + +async def test_release_on_loop_trips_the_gate(tmp_path): + """Meta-check: prove the probe has teeth, so the test above is not vacuous. + + Calling ``release`` directly on the event loop must raise, otherwise the + offload anchor could pass because the store quietly stopped doing IO. + """ + from blockbuster import BlockingError + + provider = _make_provider_with_active_sandbox(tmp_path, "sb-onloop") + + with pytest.raises(BlockingError): + provider.release("sb-onloop") diff --git a/backend/tests/test_aio_sandbox_local_backend.py b/backend/tests/test_aio_sandbox_local_backend.py index bf0767bc7..db4526eaf 100644 --- a/backend/tests/test_aio_sandbox_local_backend.py +++ b/backend/tests/test_aio_sandbox_local_backend.py @@ -333,3 +333,44 @@ def test_is_container_running_raises_on_unrelated_not_found_error(monkeypatch): with pytest.raises(RuntimeError, match="Failed to inspect container sandbox-busy"): backend._is_container_running("sandbox-busy") + + +def test_stop_container_passes_a_timeout(monkeypatch): + """An unbounded `stop` can outlive the teardown lease that guards it. + + The `del:` marker keeps a peer from re-acquiring the container during the + stop, but a lease can lapse (a store outage longer than the TTL) while a + wedged daemon leaves `docker stop` blocked forever — and the stop then lands + on a container the peer has since been handed. Bounding the call caps that + exposure independently of the ownership layer. + """ + backend = _backend_for_inspect_tests() + seen = {} + + def fake_run(cmd, **kwargs): + seen.update(kwargs) + return SimpleNamespace(stdout="", stderr="", returncode=0) + + monkeypatch.setattr("subprocess.run", fake_run) + backend._stop_container("sandbox-slow") + + assert seen.get("timeout") == backend._STOP_TIMEOUT_SECONDS + + +def test_stop_container_propagates_a_timeout_instead_of_reporting_success(monkeypatch): + """A timed-out stop must not be swallowed like a failed one. + + `CalledProcessError` 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. + """ + backend = _backend_for_inspect_tests() + + def fake_run(cmd, **kwargs): + raise subprocess.TimeoutExpired(cmd=cmd, timeout=kwargs["timeout"]) + + monkeypatch.setattr("subprocess.run", fake_run) + + with pytest.raises(subprocess.TimeoutExpired): + backend._stop_container("sandbox-wedged") diff --git a/backend/tests/test_aio_sandbox_provider.py b/backend/tests/test_aio_sandbox_provider.py index ae7503b12..f34e0d999 100644 --- a/backend/tests/test_aio_sandbox_provider.py +++ b/backend/tests/test_aio_sandbox_provider.py @@ -45,14 +45,33 @@ def test_host_thread_dir_rejects_invalid_thread_id(tmp_path): def _make_provider(tmp_path): - """Build a minimal AioSandboxProvider instance without starting the idle checker.""" + """Build a minimal AioSandboxProvider instance without starting the idle checker. + + ``tmp_path`` is accepted and ignored: ownership no longer lives on disk. Each + provider gets its own in-process ownership store, so it owns every sandbox it + tracks — cross-instance behaviour is covered in + ``test_sandbox_orphan_reconciliation.py`` (shared store) and + ``test_sandbox_ownership_store.py`` (store contract). + """ + from deerflow.community.aio_sandbox.ownership.memory import MemoryOwnershipStore + from deerflow.config.sandbox_config import SandboxOwnershipConfig + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") with patch.object(aio_mod.AioSandboxProvider, "_start_idle_checker"): provider = aio_mod.AioSandboxProvider.__new__(aio_mod.AioSandboxProvider) - provider._config = {} + provider._config = {"idle_timeout": 600, "replicas": 3} provider._sandboxes = {} + provider._local_teardown = set() + provider._acquire_epoch = {} + provider._acquire_epoch_counter = 0 + provider._acquire_inflight = {} provider._lock = MagicMock() provider._idle_checker_stop = MagicMock() + provider._renewal_stop = MagicMock() + provider._renewal_thread = None + provider._owner_id = "test-worker" + provider._ownership_config = SandboxOwnershipConfig() + provider._ownership = MemoryOwnershipStore(owner_id="test-worker", ttl_seconds=600) return provider @@ -430,6 +449,10 @@ def _make_provider_with_active_sandbox(tmp_path, sandbox_id: str): } provider._thread_sandboxes = {} provider._last_activity = {sandbox_id: 0.0} + provider._local_teardown = set() + provider._acquire_epoch = {} + provider._acquire_epoch_counter = 0 + provider._acquire_inflight = {} provider._shutdown_called = False provider._idle_checker_thread = None provider._backend = SimpleNamespace(destroy=MagicMock()) @@ -651,12 +674,19 @@ def test_cleanup_idle_sandboxes_keeps_active_cleanup_and_delegates_warm_expiry(t } calls = [] - provider.destroy = MagicMock(side_effect=lambda _sandbox_id: calls.append("active")) + # The idle path destroys through `_destroy_tracked`, not `destroy()`: its + # "still idle?" re-check has to run in the same critical section that + # reserves the teardown, so it is passed down as a predicate. Asserting on + # `destroy` here would pass vacuously — it is no longer on this path. + provider._destroy_tracked = MagicMock(side_effect=lambda _sandbox_id, **_kw: calls.append("active")) provider._reap_expired_warm = MagicMock(side_effect=lambda _idle_timeout: calls.append("warm")) provider._cleanup_idle_sandboxes(1.0) - provider.destroy.assert_called_once_with("active-old") + assert provider._destroy_tracked.call_count == 1 + assert provider._destroy_tracked.call_args.args == ("active-old",) + # The gate must actually be a live predicate, not a constant-true placeholder. + assert provider._destroy_tracked.call_args.kwargs["still_reapable"]() is True provider._reap_expired_warm.assert_called_once_with(1.0) assert calls == ["active", "warm"] diff --git a/backend/tests/test_sandbox_orphan_reconciliation.py b/backend/tests/test_sandbox_orphan_reconciliation.py index 5ff446003..a0a2aa7b8 100644 --- a/backend/tests/test_sandbox_orphan_reconciliation.py +++ b/backend/tests/test_sandbox_orphan_reconciliation.py @@ -14,10 +14,12 @@ import signal import threading import time from datetime import UTC, datetime -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest +from deerflow.community.aio_sandbox.aio_sandbox_provider import SandboxBeingDestroyedError +from deerflow.community.aio_sandbox.ownership import compute_lease_ttl from deerflow.community.aio_sandbox.sandbox_info import SandboxInfo # ── SandboxBackend.list_running() default ──────────────────────────────────── @@ -329,17 +331,97 @@ def test_extract_host_port_handles_missing_fields(): # ── AioSandboxProvider._reconcile_orphans() ────────────────────────────────── -def _make_provider_for_reconciliation(): +def _make_shared_ownership_store(**kwargs): + """A store two provider instances can share. + + Sharing one store object between two providers is how these tests model two + gateway instances pointed at one Redis: the provider only ever sees the + ``SandboxOwnershipStore`` ABC, so the ownership behaviour exercised here is + backend-agnostic. The redis backend's own semantics are pinned separately in + ``test_sandbox_ownership_store.py``. + """ + from deerflow.community.aio_sandbox.ownership.memory import MemoryOwnershipStore + + kwargs.setdefault("ttl_seconds", 600) + return MemoryOwnershipStore(owner_id="__shared__", **kwargs) + + +class _ScopedOwnershipStore: + """View of a shared store as seen by one instance (rebinds ``owner_id``). + + Rebinding is a test-only trick to model two instances against one store, and + it is only sound while one thread at a time is inside the shared store. The + heartbeat-hold tests deliberately run a main-thread ``take`` as worker-b + concurrently with a heartbeat-thread ``claim`` as worker-a, so the rebind has + to be serialized with the call it applies to: otherwise a ``claim`` can + execute under worker-b's id, read worker-a's ``del:`` lease as a peer's, + return ``False``, and kill the heartbeat — after which the marker lapses and + the next ``take`` succeeds spuriously. The GIL makes that window small, not + absent, so the tests flake rather than fail. + """ + + # Serializes owner_id rebind + call across every view of one shared store. + # A class-level lock is deliberate: each worker gets its own view object, and + # the thing being guarded is the single shared store they both mutate. + _rebind_lock = threading.Lock() + + def __init__(self, shared, owner_id: str): + self._shared = shared + self._owner_id = owner_id + + @property + def owner_id(self) -> str: + return self._owner_id + + @property + def supports_cross_process(self) -> bool: + return True + + def _as_me(self, fn, *args): + with self._rebind_lock: + previous = self._shared._owner_id + self._shared._owner_id = self._owner_id + try: + return fn(*args) + finally: + self._shared._owner_id = previous + + def take(self, sandbox_id): + return self._as_me(self._shared.take, sandbox_id) + + def claim(self, sandbox_id, *, for_destroy: bool = False): + return self._as_me(lambda sid: self._shared.claim(sid, for_destroy=for_destroy), sandbox_id) + + def renew(self, sandbox_id): + return self._as_me(self._shared.renew, sandbox_id) + + def release(self, sandbox_id): + return self._as_me(self._shared.release, sandbox_id) + + def owner(self, sandbox_id): + return self._shared.owner(sandbox_id) + + def close(self): + pass + + +def _make_provider_for_reconciliation(tmp_path=None, *, worker_id: str = "worker-test", store=None): """Build a minimal AioSandboxProvider without triggering __init__ side effects. WARNING: This helper intentionally bypasses ``__init__`` via ``__new__`` so - tests don't depend on Docker or touch the real idle-checker thread. The - downside is that this helper is tightly coupled to the set of attributes + tests don't depend on Docker or touch the real idle-checker/renewal threads. + The downside is that this helper is tightly coupled to the set of attributes set up in ``AioSandboxProvider.__init__``. If ``__init__`` gains a new attribute that ``_reconcile_orphans`` (or other methods under test) reads, this helper must be updated in lockstep — otherwise tests will fail with a confusing ``AttributeError`` instead of a meaningful assertion failure. + + Pass a shared *store* (see ``_make_shared_ownership_store``) to two providers + to model two gateway instances coordinating through one ownership backend. + ``tmp_path`` is accepted and ignored: ownership no longer lives on disk. """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") provider = aio_mod.AioSandboxProvider.__new__(aio_mod.AioSandboxProvider) provider._lock = threading.Lock() @@ -349,20 +431,35 @@ def _make_provider_for_reconciliation(): provider._thread_locks = {} provider._last_activity = {} provider._warm_pool = {} + provider._unowned_since = {} + provider._local_teardown = set() + provider._acquire_epoch = {} + provider._acquire_epoch_counter = 0 + provider._acquire_inflight = {} provider._shutdown_called = False provider._idle_checker_stop = threading.Event() provider._idle_checker_thread = None + provider._renewal_stop = threading.Event() + provider._renewal_thread = None provider._config = { "idle_timeout": 600, "replicas": 3, } provider._backend = MagicMock() + provider._owner_id = worker_id + provider._ownership_config = SandboxOwnershipConfig() + if store is None: + from deerflow.community.aio_sandbox.ownership.memory import MemoryOwnershipStore + + provider._ownership = MemoryOwnershipStore(owner_id=worker_id, ttl_seconds=600) + else: + provider._ownership = _ScopedOwnershipStore(store, worker_id) return provider -def test_reconcile_adopts_old_containers_into_warm_pool(): - """All containers are adopted into warm pool regardless of age — idle checker handles cleanup.""" - provider = _make_provider_for_reconciliation() +def test_reconcile_adopts_old_containers_into_warm_pool(tmp_path): + """Lease-free containers are adopted into warm pool — idle checker handles cleanup.""" + provider = _make_provider_for_reconciliation(tmp_path) now = time.time() old_info = SandboxInfo( @@ -380,9 +477,9 @@ def test_reconcile_adopts_old_containers_into_warm_pool(): assert "old12345" in provider._warm_pool -def test_reconcile_adopts_young_containers(): - """Young containers are adopted into warm pool for potential reuse.""" - provider = _make_provider_for_reconciliation() +def test_reconcile_adopts_young_containers(tmp_path): + """Young lease-free containers are adopted into warm pool for potential reuse.""" + provider = _make_provider_for_reconciliation(tmp_path) now = time.time() young_info = SandboxInfo( @@ -401,9 +498,9 @@ def test_reconcile_adopts_young_containers(): assert adopted_info.sandbox_id == "young123" -def test_reconcile_mixed_containers_all_adopted(): - """All containers (old and young) are adopted into warm pool.""" - provider = _make_provider_for_reconciliation() +def test_reconcile_mixed_containers_all_adopted(tmp_path): + """All lease-free containers (old and young) are adopted into warm pool.""" + provider = _make_provider_for_reconciliation(tmp_path) now = time.time() old_info = SandboxInfo( @@ -427,9 +524,9 @@ def test_reconcile_mixed_containers_all_adopted(): assert "young_one" in provider._warm_pool -def test_reconcile_skips_already_tracked_containers(): +def test_reconcile_skips_already_tracked_containers(tmp_path): """Containers already in _sandboxes or _warm_pool should be skipped.""" - provider = _make_provider_for_reconciliation() + provider = _make_provider_for_reconciliation(tmp_path) now = time.time() existing_info = SandboxInfo( @@ -449,9 +546,9 @@ def test_reconcile_skips_already_tracked_containers(): assert "existing1" not in provider._warm_pool -def test_reconcile_handles_backend_failure(): +def test_reconcile_handles_backend_failure(tmp_path): """Reconciliation should not crash if backend.list_running() fails.""" - provider = _make_provider_for_reconciliation() + provider = _make_provider_for_reconciliation(tmp_path) provider._backend.list_running.side_effect = RuntimeError("docker not available") # Should not raise @@ -460,9 +557,9 @@ def test_reconcile_handles_backend_failure(): assert provider._warm_pool == {} -def test_reconcile_no_running_containers(): +def test_reconcile_no_running_containers(tmp_path): """Reconciliation with no running containers is a no-op.""" - provider = _make_provider_for_reconciliation() + provider = _make_provider_for_reconciliation(tmp_path) provider._backend.list_running.return_value = [] provider._reconcile_orphans() @@ -471,9 +568,1402 @@ def test_reconcile_no_running_containers(): assert provider._warm_pool == {} -def test_reconcile_multiple_containers_all_adopted(): - """Multiple containers should all be adopted into warm pool.""" - provider = _make_provider_for_reconciliation() +def test_reconcile_skips_container_owned_by_peer(): + """#4206: do not adopt a container another instance still owns.""" + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + now = time.time() + info = SandboxInfo( + sandbox_id="shared01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-shared01", + created_at=now - 50, + ) + worker_a._publish_ownership("shared01") + worker_b._backend.list_running.return_value = [info] + + worker_b._reconcile_orphans() + + assert "shared01" not in worker_b._warm_pool + worker_b._backend.destroy.assert_not_called() + # The lease is still A's — B's failed claim must not have stolen it. + assert shared.owner("shared01") == "worker-a" + + +def test_idle_reap_does_not_destroy_peer_owned_warm_entry(): + """#4206: idle reaper must not stop a container another instance owns.""" + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_b._config["idle_timeout"] = 60 + now = time.time() + info = SandboxInfo( + sandbox_id="a99c8444", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-a99c8444", + created_at=now - 50, + ) + # Simulate the bad old path: B already has it in warm (or adopted wrongly). + worker_b._warm_pool["a99c8444"] = (info, now - 61) + worker_a._publish_ownership("a99c8444") + + worker_b._reap_expired_warm(idle_timeout=60) + + worker_b._backend.destroy.assert_not_called() + + +def test_multi_worker_release_then_peer_reconcile_cannot_kill(): + """#4206 issue-log path: A release→warm; B reconcile+reap must not destroy.""" + shared = _make_shared_ownership_store() + destroyed: list[str] = [] + running: dict[str, SandboxInfo] = {} + + def list_running(): + return list(running.values()) + + def destroy(info: SandboxInfo): + destroyed.append(info.sandbox_id) + running.pop(info.sandbox_id, None) + + backend = MagicMock() + backend.list_running.side_effect = list_running + backend.destroy.side_effect = destroy + + sid = "a99c8444" + info = SandboxInfo( + sandbox_id=sid, + sandbox_url="http://localhost:8080", + container_name=f"deer-flow-sandbox-{sid}", + created_at=time.time() - 50, + ) + running[sid] = info + + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_a._backend = backend + worker_a._config["idle_timeout"] = 60 + # A released to warm and holds the lease. + worker_a._warm_pool[sid] = (info, time.time()) + worker_a._publish_ownership(sid) + + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_b._backend = backend + worker_b._config["idle_timeout"] = 60 + worker_b._reconcile_orphans() + assert sid not in worker_b._warm_pool + + # Even if B somehow had it warm, reap must refuse. + worker_b._warm_pool[sid] = (info, time.time() - 61) + worker_b._reap_expired_warm(idle_timeout=60) + assert sid not in destroyed + assert sid in running + assert sid in worker_a._warm_pool + + +def test_expired_lease_lets_peer_adopt_crashed_owner_container(): + """The crash path still works: once a dead owner's lease lapses, adopt it. + + The counterpart to the tests above — ownership must not become a permanent + leak when the owning instance dies without releasing. Adoption is delayed by + the recovery grace, but a dead owner never republishes, so it still happens. + """ + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + shared = _make_shared_ownership_store(ttl_seconds=0.05) + dead = _make_provider_for_reconciliation(worker_id="worker-dead", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="crashed1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-crashed1", + created_at=time.time() - 50, + ) + dead._publish_ownership("crashed1") + worker_b._backend.list_running.return_value = [info] + + # Owner "crashes": stops renewing. Its lease lapses in the store. + time.sleep(0.1) + + now = time.time() + with patch.object(aio_mod.time, "time", return_value=now): + worker_b._reconcile_orphans() + assert "crashed1" not in worker_b._warm_pool, "adopted a lapsed lease without waiting out the recovery grace" + + # The dead owner never republishes, so the grace runs out and B adopts. + with patch.object(aio_mod.time, "time", return_value=now + compute_lease_ttl(worker_b._ownership_config) + 1): + worker_b._reconcile_orphans() + + assert "crashed1" in worker_b._warm_pool + assert shared.owner("crashed1") == "worker-b" + + +# ── Ownership store rework (#4206): fail-closed publish, renewal independence ── + + +def test_acquire_fails_closed_when_ownership_cannot_be_published(): + """Establishment is fail-closed: never hand out a sandbox we could not own. + + The provider used to swallow the lease-write error and return the sandbox id + on the next line, so a store outage silently disabled the only cross-instance + exclusion while the sandbox was handed out as usable — peers then saw an + unowned live container and reaped it. + """ + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership = MagicMock() + worker._ownership.take.side_effect = OwnershipBackendError("store down") + + info = SandboxInfo( + sandbox_id="new001", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-new001", + created_at=time.time(), + ) + + with pytest.raises(OwnershipBackendError): + worker._register_created_sandbox("t1", "new001", info, user_id="u1") + + # The just-created container must not be leaked as an unowned orphan. + worker._backend.destroy.assert_called_once_with(info) + assert "new001" not in worker._sandboxes + + +def test_reuse_fails_closed_when_ownership_cannot_be_published(): + """Same fail-closed rule on the in-process reuse path.""" + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + info = SandboxInfo( + sandbox_id="sb1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-sb1", + created_at=time.time(), + ) + worker._sandboxes["sb1"] = MagicMock() + worker._sandbox_infos["sb1"] = info + worker._thread_sandboxes[("u1", "t1")] = "sb1" + worker._check_tracked_sandbox_alive = MagicMock(return_value=True) + worker._ownership = MagicMock() + worker._ownership.take.side_effect = OwnershipBackendError("store down") + + with pytest.raises(OwnershipBackendError): + worker._reuse_in_process_sandbox("t1", user_id="u1") + + +def test_destroy_fails_closed_when_ownership_unknown(): + """A store that cannot answer must not be read as 'container is free'.""" + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership = MagicMock() + worker._ownership.claim.side_effect = OwnershipBackendError("store down") + + info = SandboxInfo( + sandbox_id="unknown1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-unknown1", + created_at=time.time() - 50, + ) + + worker._destroy_warm_entry("unknown1", info, reason="idle_timeout", still_reapable=lambda: True) + + worker._backend.destroy.assert_not_called() + + +def test_reconcile_fails_closed_when_ownership_unknown(): + """A store outage must not turn every peer container into an adoptable orphan.""" + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-b") + worker._ownership = MagicMock() + # Configure what the adoption grace reads, or it short-circuits before the + # claim: a bare MagicMock answers `owner()` with a truthy mock, which reads + # as "peer-owned" and defers — so the assertion below would pass without the + # fail-closed branch ever running (it did exactly that until this was fixed). + worker._ownership.supports_cross_process = True + worker._ownership.owner.return_value = None + worker._ownership.claim.side_effect = OwnershipBackendError("store down") + info = SandboxInfo( + sandbox_id="unknown2", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-unknown2", + created_at=time.time() - 50, + ) + worker._backend.list_running.return_value = [info] + + # Unowned for a full grace, so the container is adoptable and the only thing + # left standing between it and the warm pool is the claim. + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + now = time.time() + with patch.object(aio_mod.time, "time", return_value=now): + worker._reconcile_orphans() + with patch.object(aio_mod.time, "time", return_value=now + compute_lease_ttl(worker._ownership_config) + 1): + worker._reconcile_orphans() + + assert worker._ownership.claim.called, "the fail-closed claim branch was never reached; this test guards nothing" + assert "unknown2" not in worker._warm_pool + worker._backend.destroy.assert_not_called() + + +@pytest.mark.parametrize("idle_timeout", [0, 600]) +def test_init_always_starts_lease_renewal(monkeypatch, idle_timeout): + """Renewal liveness must not ride on the idle checker's switch. + + ``_renew_active_leases`` used to have exactly one caller — ``_cleanup_idle_resources`` + — and ``__init__`` only starts the idle checker when ``idle_timeout > 0``. + ``idle_timeout: 0`` is a supported config (``config.example.yaml`` documents it + as "keep warm VMs until shutdown"), so on that config nothing ever refreshed a + lease and #4206 returned one TTL later. + + This drives ``__init__`` on purpose: the defect is in *who starts renewal*, so + a test that calls ``_start_lease_renewal()`` directly passes on the broken code + and guards nothing. + """ + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + + started: list[str] = [] + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_load_config", lambda self: {"idle_timeout": idle_timeout, "replicas": 3, "ownership": None}) + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_create_backend", lambda self: MagicMock()) + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_reconcile_orphans", lambda self: None) + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_register_signal_handlers", lambda self: None) + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_start_lease_renewal", lambda self: started.append("renewal")) + monkeypatch.setattr(aio_mod.AioSandboxProvider, "_start_idle_checker", lambda self: started.append("idle")) + monkeypatch.setattr(aio_mod.atexit, "register", lambda *a, **k: None) + + aio_mod.AioSandboxProvider() + + assert "renewal" in started, f"renewal must start at idle_timeout={idle_timeout}; ownership liveness cannot depend on the idle reaper" + assert ("idle" in started) is (idle_timeout > 0) + + +def test_renewal_keeps_the_sandbox_when_the_store_cannot_answer(): + """The one deliberate exception to fail-closed, and it had no test. + + Everywhere else an unanswerable store means "not ours". Renewal is the + opposite on purpose: `_refresh_ownership` returns True on an + `OwnershipBackendError` because unknown is not lost, and the TTL still bounds + how long a genuinely dead owner holds the lease. Invert it and a Redis outage + makes every instance drop every active and warm sandbox at once — the same + fleet-wide eviction the LAPSED/LOST split exists to prevent, which is pinned + only for the flushed-store path, never for a raising one. + """ + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership = MagicMock() + worker._ownership.renew.side_effect = OwnershipBackendError("store down") + info = SandboxInfo( + sandbox_id="live02", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-live02", + created_at=time.time(), + ) + sandbox = MagicMock() + worker._sandboxes["live02"] = sandbox + worker._sandbox_infos["live02"] = info + worker._thread_sandboxes[("u1", "t1")] = "live02" + worker._warm_pool["warm02"] = (info, time.time()) + + worker._renew_owned_leases() + + assert worker._ownership.renew.called, "renewal never reached the store; this test guards nothing" + assert "live02" in worker._sandboxes, "a store outage evicted a live sandbox nobody had taken" + assert ("u1", "t1") in worker._thread_sandboxes + assert "warm02" in worker._warm_pool, "a store outage dropped a warm entry nobody had taken" + sandbox.close.assert_not_called() + worker._backend.destroy.assert_not_called() + + +def test_renewal_keeps_the_sandbox_when_lapsed_reclaim_cannot_answer(): + """A store outage after LAPSED is still unknown, not proof of a peer. + + ``renew()`` and the follow-up ``claim()`` are separate store round trips. A + key can be observed absent and the store can then become unreachable before + the claim. Renewal must keep the sandbox and retry rather than route the + claim through the ordinary fail-closed reap helper and evict a live entry. + """ + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError, RenewOutcome + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership = MagicMock() + worker._ownership.renew.return_value = RenewOutcome.LAPSED + worker._ownership.claim.side_effect = OwnershipBackendError("store down after lapsed renew") + info = _info("live03") + sandbox = MagicMock() + worker._sandboxes["live03"] = sandbox + worker._sandbox_infos["live03"] = info + worker._thread_sandboxes[("u1", "t1")] = "live03" + + worker._renew_owned_leases() + + worker._ownership.claim.assert_called_once_with("live03") + assert "live03" in worker._sandboxes, "a failed LAPSED re-claim evicted a live sandbox nobody had taken" + assert worker._thread_sandboxes[("u1", "t1")] == "live03" + sandbox.close.assert_not_called() + worker._backend.destroy.assert_not_called() + + +def test_load_config_carries_the_stream_bridge_section(): + """Hop 1 of the "no extra config for multi-instance" promise. + + The redis inference reads `app_config.stream_bridge`, so `_load_config` has to + carry it. Nothing pinned this: the only test that drives `__init__` + monkeypatches `_load_config` wholesale and omits the key entirely, so deleting + it here left every test green while every config.yaml-native multi-instance + deployment silently fell back to `memory` — #4206 reopened on exactly the + deployments the inference exists for. + """ + from deerflow.config.stream_bridge_config import StreamBridgeConfig + + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + provider = aio_mod.AioSandboxProvider.__new__(aio_mod.AioSandboxProvider) + + bridge = StreamBridgeConfig(type="redis", redis_url="redis://bridge:6379/0") + app_config = MagicMock() + app_config.stream_bridge = bridge + app_config.sandbox = MagicMock(ownership=None, image=None, port=None, container_prefix=None, idle_timeout=600, replicas=3, mounts=[], environment={}) + + with patch.object(aio_mod, "get_app_config", return_value=app_config): + loaded = provider._load_config() + + assert loaded["stream_bridge"] is bridge, "_load_config dropped the stream_bridge section the redis inference reads" + + +def test_init_infers_redis_ownership_from_a_redis_stream_bridge(): + """Hop 2: `__init__` must actually feed the bridge into the resolver. + + Drives the real `__init__` against a real `AppConfig`-shaped object rather + than stubbing `_load_config`, because the defect would be in the wiring + between them — the same reason `test_init_always_starts_lease_renewal` drives + `__init__` instead of calling `_start_lease_renewal` directly. + """ + from deerflow.config.stream_bridge_config import StreamBridgeConfig + + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + + app_config = MagicMock() + app_config.stream_bridge = StreamBridgeConfig(type="redis", redis_url="redis://bridge:6379/0") + # No sandbox.ownership section at all: the deployment never configured one. + app_config.sandbox = MagicMock(ownership=None, image=None, port=None, container_prefix=None, idle_timeout=600, replicas=3, mounts=[], environment={}) + + built: list = [] + + def fake_store(config, *, owner_id=None): + built.append(config) + store = MagicMock() + store.supports_cross_process = True + return store + + with ( + patch.object(aio_mod, "get_app_config", return_value=app_config), + patch.object(aio_mod, "make_sandbox_ownership_store", side_effect=fake_store), + patch.object(aio_mod.AioSandboxProvider, "_create_backend", lambda self: MagicMock()), + patch.object(aio_mod.AioSandboxProvider, "_reconcile_orphans", lambda self: None), + patch.object(aio_mod.AioSandboxProvider, "_register_signal_handlers", lambda self: None), + patch.object(aio_mod.AioSandboxProvider, "_start_lease_renewal", lambda self: None), + patch.object(aio_mod.AioSandboxProvider, "_start_idle_checker", lambda self: None), + patch.object(aio_mod.atexit, "register", lambda *a, **k: None), + ): + aio_mod.AioSandboxProvider() + + assert len(built) == 1 + assert built[0].type == "redis", "a redis stream bridge did not infer a redis ownership store; multi-instance deployments silently fall back to memory" + assert built[0].redis_url == "redis://bridge:6379/0" + + +def test_renewal_loop_refreshes_owned_leases(): + """The renewal thread actually renews (the loop body, not just its wiring).""" + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05) + worker._sandboxes["sb1"] = MagicMock() + worker._publish_ownership("sb1") + + renewed: list[str] = [] + real_renew = worker._ownership.renew + + def counting_renew(sandbox_id): + renewed.append(sandbox_id) + return real_renew(sandbox_id) + + worker._ownership.renew = counting_renew + + worker._start_lease_renewal() + try: + deadline = time.time() + 3 + while not renewed and time.time() < deadline: + time.sleep(0.02) + finally: + worker._stop_lease_renewal() + + assert renewed == ["sb1"] or renewed[0] == "sb1" + + +def test_renewal_covers_warm_entries_not_just_active(): + """A warm container is still ours; letting its lease lapse invites adoption.""" + worker = _make_provider_for_reconciliation(worker_id="worker-a") + info = SandboxInfo( + sandbox_id="warm01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-warm01", + created_at=time.time(), + ) + worker._sandboxes["active01"] = MagicMock() + worker._warm_pool["warm01"] = (info, time.time()) + worker._publish_ownership("active01") + worker._publish_ownership("warm01") + + renewed: list[str] = [] + worker._ownership = MagicMock() + worker._ownership.renew.side_effect = lambda sid: renewed.append(sid) or True + + worker._renew_owned_leases() + + assert set(renewed) == {"active01", "warm01"} + + +def test_renewal_does_not_forget_a_warm_entry_mid_teardown(): + """Renewal must not pop the warm entry retained by an in-flight stop. + + A warm teardown deliberately leaves the entry visible until the backend stop + succeeds. Its ``del:us`` marker makes ordinary ``renew()`` report LOST, but + that is local teardown state rather than a peer takeover. If the stop then + fails, the warm entry must still be present for retry or reclaim. + """ + worker = _make_provider_for_reconciliation(worker_id="worker-a") + info = SandboxInfo( + sandbox_id="warm-stop", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-warm-stop", + created_at=time.time(), + ) + worker._warm_pool["warm-stop"] = (info, time.time()) + worker._publish_ownership("warm-stop") + + stop_started = threading.Event() + let_stop_fail = threading.Event() + + def failing_stop(_entry): + stop_started.set() + assert let_stop_fail.wait(timeout=5), "the test never released the backend stop" + raise RuntimeError("stop failed") + + worker._backend.destroy.side_effect = failing_stop + result = {} + reaper = threading.Thread( + target=lambda: result.update( + destroyed=worker._destroy_warm_entry( + "warm-stop", + info, + reason="idle_timeout", + still_reapable=lambda: True, + ) + ), + daemon=True, + ) + reaper.start() + try: + assert stop_started.wait(timeout=5), "the warm teardown never reached the backend stop" + assert "warm-stop" in worker._local_teardown, "precondition: the local teardown reservation is not held" + + worker._renew_owned_leases() + + assert "warm-stop" in worker._warm_pool, "renewal forgot a warm entry while this instance was stopping it" + finally: + let_stop_fail.set() + reaper.join(timeout=5) + + assert not reaper.is_alive(), "the failed warm teardown did not finish" + assert result.get("destroyed") is False + assert "warm-stop" in worker._warm_pool, "a failed stop lost the warm entry it promised to retain" + + +def test_lost_lease_drops_sandbox_without_destroying_container(): + """Losing the lease means the container is someone else's — drop it, don't kill it. + + Destroying here would be the exact cross-instance kill the store exists to + prevent, just triggered from the renewal path instead of the reaper. Only our + host-side handle goes away, and it must be closed rather than leaked (#2872). + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="moved01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-moved01", + created_at=time.time(), + ) + sandbox = MagicMock() + worker_a._sandboxes["moved01"] = sandbox + worker_a._sandbox_infos["moved01"] = info + worker_a._thread_sandboxes[("u1", "t1")] = "moved01" + worker_a._last_activity["moved01"] = time.time() + worker_a._publish_ownership("moved01") + + # The thread's next turn routes to B, which takes over ownership. + worker_b._publish_ownership("moved01") + + worker_a._renew_owned_leases() + + assert "moved01" not in worker_a._sandboxes + assert "moved01" not in worker_a._sandbox_infos + assert ("u1", "t1") not in worker_a._thread_sandboxes + worker_a._backend.destroy.assert_not_called() + sandbox.close.assert_called_once() + assert shared.owner("moved01") == "worker-b" + + +def test_ownership_rollback_on_create_closes_the_client_it_drops(): + """The rollback destroys the container; its host-side client must not leak (#2872).""" + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + worker = _make_provider_for_reconciliation(worker_id="worker-a") + worker._ownership = MagicMock() + worker._ownership.take.side_effect = OwnershipBackendError("store down") + info = SandboxInfo( + sandbox_id="new002", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-new002", + created_at=time.time(), + ) + + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + created: list[MagicMock] = [] + + def fake_aio_sandbox(**kwargs): + sandbox = MagicMock() + created.append(sandbox) + return sandbox + + with patch.object(aio_mod, "AioSandbox", side_effect=fake_aio_sandbox): + with pytest.raises(OwnershipBackendError): + worker._register_created_sandbox("t1", "new002", info, user_id="u1") + + worker._backend.destroy.assert_called_once_with(info) + assert created and created[0].close.call_count == 1 + + +def test_acquire_takes_over_ownership_so_a_thread_can_move_instances(): + """A thread's next turn can land on another instance; it must not be stranded. + + Ownership answers "who reaps this", not "who may use it". A conditional claim + here would refuse while the previous instance's lease was still live and break + every load-balanced follow-up turn. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="thread01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-thread01", + created_at=time.time(), + ) + worker_a._publish_ownership("thread01") + assert shared.owner("thread01") == "worker-a" + + # B serves the thread's next turn and discovers the existing container. + assert worker_b._register_discovered_sandbox("t1", info, user_id="u1") == "thread01" + assert shared.owner("thread01") == "worker-b" + + +def test_store_losing_all_state_does_not_evict_live_sandboxes(): + """A Redis restart must not drop every in-flight sandbox fleet-wide. + + `renew()` returns falsy for two very different situations: a peer took the + lease, and the lease is simply absent. Treating them the same meant that when + Redis restarted without persistence — every key gone, nobody holding + anything — each instance evicted every sandbox it was actively serving. + A lapsed lease must be re-established instead. + """ + shared = _make_shared_ownership_store() + worker = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + info = SandboxInfo( + sandbox_id="live01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-live01", + created_at=time.time(), + ) + sandbox = MagicMock() + worker._sandboxes["live01"] = sandbox + worker._sandbox_infos["live01"] = info + worker._thread_sandboxes[("u1", "t1")] = "live01" + worker._publish_ownership("live01") + + # The store loses everything, as a Redis restart without persistence does. + shared._leases.clear() + + worker._renew_owned_leases() + + assert "live01" in worker._sandboxes, "a store restart evicted a live sandbox nobody had taken" + assert ("u1", "t1") in worker._thread_sandboxes + sandbox.close.assert_not_called() + worker._backend.destroy.assert_not_called() + # And it is ours again, so peers still cannot reap it. + assert shared.owner("live01") == "worker-a" + + +def test_peer_reconcile_after_state_loss_does_not_steal_a_live_container(): + """The other half of the store-restart case: a peer must not adopt first. + + ``_refresh_ownership`` already refuses to read an absent lease as + abandonment. Reconciliation must not contradict it on the other path: after + the store loses every key, each live owner is still serving its containers + and simply has not reached its next renewal tick. An instance reconciling in + that window sees no lease and would adopt every one of them; the real owner's + next renewal then reports LOST and it drops a sandbox mid-turn, which the + adopter later idle-destroys — #4206 through the back door. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + info = SandboxInfo( + sandbox_id="live01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-live01", + created_at=time.time(), + ) + sandbox = MagicMock() + worker_a._sandboxes["live01"] = sandbox + worker_a._sandbox_infos["live01"] = info + worker_a._thread_sandboxes[("u1", "t1")] = "live01" + worker_a._publish_ownership("live01") + + # The store loses everything, as a Redis restart without persistence does. + # Worker A is alive and still serving live01. + shared._leases.clear() + + # A peer starts up and reconciles before A's renewal tick fires. + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_b._backend.list_running.return_value = [info] + worker_b._reconcile_orphans() + + assert "live01" not in worker_b._warm_pool, "a peer adopted a container whose owner is still alive and serving it" + + # A's renewal tick finally fires: it must still own and keep the sandbox. + worker_a._renew_owned_leases() + + assert "live01" in worker_a._sandboxes, "a peer's reconcile evicted a live sandbox after the store lost its state" + assert ("u1", "t1") in worker_a._thread_sandboxes + sandbox.close.assert_not_called() + worker_b._backend.destroy.assert_not_called() + assert shared.owner("live01") == "worker-a" + + +def test_adoption_grace_expires_so_a_truly_orphaned_container_is_still_adopted(): + """The grace must delay adoption, not disable it. + + A container that stays unowned across a full lease TTL has no live owner — + a surviving owner republishes within one renewal interval, which is shorter + than the TTL by construction. Reconciliation must adopt it then, or a crashed + instance's containers would leak forever. + """ + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + shared = _make_shared_ownership_store() + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + ttl = compute_lease_ttl(worker_b._ownership_config) + info = SandboxInfo( + sandbox_id="crashed1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-crashed1", + created_at=time.time() - 50, + ) + worker_b._backend.list_running.return_value = [info] + + now = time.time() + with patch.object(aio_mod.time, "time", return_value=now): + worker_b._reconcile_orphans() + assert "crashed1" not in worker_b._warm_pool, "adopted a keyless container without waiting out the recovery grace" + + # Nobody republished the lease across a full TTL: the owner is really gone. + with patch.object(aio_mod.time, "time", return_value=now + ttl + 1): + worker_b._reconcile_orphans() + + assert "crashed1" in worker_b._warm_pool, "the grace never expired, so a crashed owner's container would leak forever" + assert shared.owner("crashed1") == "worker-b" + + +def test_adoption_grace_restarts_when_a_live_owner_republishes(): + """A republished lease must reset the grace, not just pause it. + + Reset and pause only diverge on a **second** lapse. Pause leaves the original + timestamp behind, so the next time the lease drops the grace is already spent + and the adopter takes a live container with no wait at all. Stopping at "A + republished, B defers" would prove nothing — a paused timer defers there too, + because the container simply reads as owned. So the second lapse is the whole + test; without it this passes with the reset deleted. + """ + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + ttl = compute_lease_ttl(worker_b._ownership_config) + info = SandboxInfo( + sandbox_id="live01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-live01", + created_at=time.time(), + ) + worker_b._backend.list_running.return_value = [info] + + now = time.time() + # B starts its grace on a container that currently looks unowned. + with patch.object(aio_mod.time, "time", return_value=now): + worker_b._reconcile_orphans() + + # A republishes mid-grace (its renewal tick re-establishing a lapsed lease). + worker_a._publish_ownership("live01") + + with patch.object(aio_mod.time, "time", return_value=now + ttl + 1): + worker_b._reconcile_orphans() + + assert "live01" not in worker_b._warm_pool, "a stale grace expired over a lease a live owner had already republished" + assert shared.owner("live01") == "worker-a" + + # The republish must have cleared B's timer, not merely paused it. A second + # blip drops the key again: B has to serve a *fresh* full grace, which A's + # next renewal tick will beat. A paused timer would still hold the original + # start, so B would adopt A's live container instantly, with no grace at all. + assert "live01" not in worker_b._unowned_since, "the republish left a stale grace timer behind" + + shared._leases.clear() + with patch.object(aio_mod.time, "time", return_value=now + ttl + 2): + worker_b._reconcile_orphans() + + assert "live01" not in worker_b._warm_pool, "a grace timer left over from before the republish expired instantly on the next lapse" + + +def test_acquire_refuses_a_container_a_peer_is_destroying(): + """#4206's last window: `take()` must not overrun a destroyer's claim. + + Sequence: B's reaper claims X for destroy and starts the (slow) container + stop; a turn for X's thread routes to A. An unconditional takeover would hand + A a sandbox that B's stop is about to kill mid-turn. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="dying01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-dying01", + created_at=time.time(), + ) + + # B decides to reap it and marks the teardown, then its stop is in flight. + assert worker_b._claim_ownership("dying01", for_destroy=True) is True + + # A's acquire must refuse rather than hand out a doomed container. + with pytest.raises(SandboxBeingDestroyedError): + worker_a._register_discovered_sandbox("t1", info, user_id="u1") + + assert "dying01" not in worker_a._sandboxes + + +def test_teardown_marker_is_held_for_a_stop_that_outlives_the_lease_ttl(): + """The `del:` state must not expire out from under an in-flight stop. + + `test_acquire_refuses_a_container_a_peer_is_destroying` above proves the + marker refuses a takeover — but never lets it expire. `claim(for_destroy)` + writes it with the ordinary lease TTL and nothing refreshes it: `renew()` + only extends `own:` and reports a teardown as LOST, and the destroy paths + drop the sandbox from the maps the renewal loop iterates. So a container + stop that outlives the TTL let the marker lapse, a peer's `take()` then + succeeded against the still-running container, and the stop landed on the + turn that had just been handed it — the very window `del:` exists to close, + reopened by its own expiry. The `flock` this replaced could not expire; a + lease can, so it has to be held on purpose. + """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + lease_ttl = 0.15 + shared = _make_shared_ownership_store(ttl_seconds=lease_ttl) + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + # A legal config: the schema bounds only renewal > 0 and multiplier >= 2. + worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05, ttl_multiplier=3.0) + info = SandboxInfo( + sandbox_id="doomed1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-doomed1", + created_at=time.time(), + ) + + stop_entered = threading.Event() + release_stop = threading.Event() + + def slow_destroy(entry): + stop_entered.set() + release_stop.wait(timeout=5) + + worker_a._backend.destroy = MagicMock(side_effect=slow_destroy) + worker_a._warm_pool["doomed1"] = (info, time.time()) + + reaper = threading.Thread( + target=lambda: worker_a._destroy_warm_entry("doomed1", info, reason="idle_timeout", still_reapable=lambda: True), + daemon=True, + ) + reaper.start() + try: + assert stop_entered.wait(timeout=5), "the reaper never reached the backend stop" + + # Across a span several times the lease TTL, a turn for this thread must + # keep being refused — the container is still being stopped. + deadline = time.time() + lease_ttl * 4 + while time.time() < deadline: + assert not worker_b._ownership.take("doomed1"), "a peer took a container whose stop was still in flight" + time.sleep(0.02) + finally: + release_stop.set() + reaper.join(timeout=5) + + # Once the stop returns the marker is dropped, so the thread can cold-start. + assert shared.owner("doomed1") is None, "the teardown marker outlived the stop that justified it" + assert worker_b._ownership.take("doomed1") is True + + +def test_unhealthy_drop_holds_the_teardown_marker_for_its_stop(): + """The third `del:`-marked stop path needs the same hold as the other two. + + `_drop_unhealthy_sandbox` claims for destroy and then blocks on the backend + stop exactly like `_destroy_warm_entry` and `destroy()`. Its sibling test + `test_unhealthy_sandbox_owned_by_peer_is_not_destroyed` pins the *gate* — a + peer-owned container is not stopped — but never lets the marker **expire** + during an in-flight stop, which is the same blind spot that hid this window + on the other two paths. It untracks before claiming, so `_renew_owned_leases` + cannot see the id either: nothing refreshes the marker unless the stop holds + it. + """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + lease_ttl = 0.15 + shared = _make_shared_ownership_store(ttl_seconds=lease_ttl) + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05, ttl_multiplier=3.0) + info = SandboxInfo( + sandbox_id="sick01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-sick01", + created_at=time.time(), + ) + + stop_entered = threading.Event() + release_stop = threading.Event() + + def slow_destroy(entry): + stop_entered.set() + release_stop.wait(timeout=5) + + worker_a._backend.destroy = MagicMock(side_effect=slow_destroy) + worker_a._sandboxes["sick01"] = MagicMock() + worker_a._sandbox_infos["sick01"] = info + + dropper = threading.Thread( + target=lambda: worker_a._drop_unhealthy_sandbox("sick01", "health check failed"), + daemon=True, + ) + dropper.start() + try: + assert stop_entered.wait(timeout=5), "the drop never reached the backend stop" + + deadline = time.time() + lease_ttl * 4 + while time.time() < deadline: + assert not worker_b._ownership.take("sick01"), "a peer took a container whose unhealthy-drop stop was still in flight" + time.sleep(0.02) + finally: + release_stop.set() + dropper.join(timeout=5) + + assert shared.owner("sick01") is None, "the teardown marker outlived the stop that justified it" + + +def test_destroy_holds_the_teardown_marker_for_its_stop(): + """The third of the three `del:`-marked stops, and the one with no test. + + `_destroy_warm_entry` and `_drop_unhealthy_sandbox` each have a held-marker + test; `destroy()` is wrapped but nothing pins the wrap, so deleting it goes + unnoticed. "Every path does X" claims keep leaving exactly one sibling + untested — this is that sibling. + """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + lease_ttl = 0.15 + shared = _make_shared_ownership_store(ttl_seconds=lease_ttl) + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05, ttl_multiplier=3.0) + info = SandboxInfo( + sandbox_id="doomed3", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-doomed3", + created_at=time.time(), + ) + + stop_entered = threading.Event() + release_stop = threading.Event() + + def slow_destroy(entry): + stop_entered.set() + release_stop.wait(timeout=5) + + worker_a._backend.destroy = MagicMock(side_effect=slow_destroy) + worker_a._sandboxes["doomed3"] = MagicMock() + worker_a._sandbox_infos["doomed3"] = info + + destroyer = threading.Thread(target=lambda: worker_a.destroy("doomed3"), daemon=True) + destroyer.start() + try: + assert stop_entered.wait(timeout=5), "destroy never reached the backend stop" + + deadline = time.time() + lease_ttl * 4 + while time.time() < deadline: + assert not worker_b._ownership.take("doomed3"), "a peer took a container whose destroy() stop was still in flight" + time.sleep(0.02) + finally: + release_stop.set() + destroyer.join(timeout=5) + + assert shared.owner("doomed3") is None, "the teardown marker outlived the stop that justified it" + + +def test_destroy_releases_the_teardown_marker_when_the_stop_fails(): + """A failed stop must not strand the id under a `del:` marker. + + `_destroy_warm_entry` releases on both outcomes and says why: the container is + still up, so leaving it marked would block its thread from ever re-acquiring + it. `destroy()` had no such guard — a raising backend propagated straight past + the release, and `take()` stays refused against `del:`, so the thread could not + acquire until the TTL lapsed. Fails safe rather than fatal (nobody stops a live + container), but the three stop paths must agree on this. + + The error still propagates: `shutdown()` logs per sandbox off it, so releasing + must not turn into swallowing. + """ + shared = _make_shared_ownership_store() + worker = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + info = SandboxInfo( + sandbox_id="boom01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-boom01", + created_at=time.time(), + ) + worker._sandboxes["boom01"] = MagicMock() + worker._sandbox_infos["boom01"] = info + worker._backend.destroy = MagicMock(side_effect=RuntimeError("docker daemon is unreachable")) + + with pytest.raises(RuntimeError, match="docker daemon is unreachable"): + worker.destroy("boom01") + + assert shared.owner("boom01") is None, "a failed stop left the id stranded under a teardown marker" + # The container may well still be running, so its thread must be able to take + # it back rather than wait out the TTL. + assert worker._ownership.take("boom01") is True + + +class _BlockHeartbeatClaim: + """Store view whose heartbeat *refresh* claim blocks until released. + + ``claim(for_destroy=True)`` is issued twice per teardown: once by the + caller's gate (``_claim_ownership``) and then repeatedly by the heartbeat. + This lets the gate through and blocks from the second onward, so a refresh + can be held in flight while the context manager exits — the interleaving the + release-ordering fix has to survive. + """ + + def __init__(self, inner): + self._inner = inner + self._for_destroy_claims = 0 + self._lock = threading.Lock() + self.heartbeat_blocked = threading.Event() + self.unblock = threading.Event() + self.refresh_completed = threading.Event() + + @property + def owner_id(self): + return self._inner.owner_id + + @property + def supports_cross_process(self): + return self._inner.supports_cross_process + + def take(self, sandbox_id): + return self._inner.take(sandbox_id) + + def renew(self, sandbox_id): + return self._inner.renew(sandbox_id) + + def release(self, sandbox_id): + return self._inner.release(sandbox_id) + + def owner(self, sandbox_id): + return self._inner.owner(sandbox_id) + + def close(self): + pass + + def claim(self, sandbox_id, *, for_destroy: bool = False): + if for_destroy: + with self._lock: + self._for_destroy_claims += 1 + nth = self._for_destroy_claims + if nth >= 2: # the heartbeat refresh, not the caller's gate claim + self.heartbeat_blocked.set() + self.unblock.wait(timeout=10) + result = self._inner.claim(sandbox_id, for_destroy=for_destroy) + # The refresh has now rewritten `del:`. Signalling only after it + # lands lets the test settle on the final state instead of racing + # the transient window between a caller-side release and this write. + self.refresh_completed.set() + return result + return self._inner.claim(sandbox_id, for_destroy=for_destroy) + + +def test_teardown_join_budget_covers_refresh_and_final_release(): + """A normally timing-out refresh plus release must fit before deferral. + + Redis bounds each store operation at five seconds. Context exit can catch + the heartbeat in a refresh and must then wait for its final release, so the + join budget needs to exceed both sequential operation bounds rather than + only one of them. + """ + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + store_operation_timeout_seconds = 5.0 + + assert aio_mod.AioSandboxProvider._TEARDOWN_JOIN_TIMEOUT_SECONDS > 2 * store_operation_timeout_seconds + + +def test_teardown_release_waits_for_the_heartbeat_to_exit(): + """A refresh still in flight when the stop finishes must not resurrect `del:`. + + The `del:` marker's final release is the heartbeat's own last act, not the + caller's. If the caller cleared it, a refresh `claim` still blocked in the + store (redis has no infinitely-patient call, but it can be mid-round-trip) + would land *after* that release and rewrite `del:` on a container whose stop + already completed — refusing a fresh `take()` (or rolling back a fresh + create) until the TTL. Owning the release inside the heartbeat sequences it + strictly after the last refresh. (fancyboi999, PR #4221) + """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + # Long store TTL so nothing lapses via TTL during the test — the only thing + # that may clear the marker is a real release. + shared = _make_shared_ownership_store(ttl_seconds=30) + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.02, ttl_multiplier=2.0) + # Give up on the join while the heartbeat is still blocked, so the context + # returns with a refresh genuinely in flight. + worker_a._TEARDOWN_JOIN_TIMEOUT_SECONDS = 0.1 + blocking = _BlockHeartbeatClaim(worker_a._ownership) + worker_a._ownership = blocking + + info = SandboxInfo( + sandbox_id="defer1", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-defer1", + created_at=time.time(), + ) + + def slow_destroy(entry): + # Return only once the heartbeat has entered (and blocked in) a refresh, + # so the context exit runs its bounded join against an in-flight claim. + assert blocking.heartbeat_blocked.wait(timeout=5), "the heartbeat never issued a refresh claim" + + worker_a._backend.destroy = MagicMock(side_effect=slow_destroy) + worker_a._warm_pool["defer1"] = (info, time.time()) + + reaper = threading.Thread( + target=lambda: worker_a._destroy_warm_entry("defer1", info, reason="idle_timeout", still_reapable=lambda: True), + daemon=True, + ) + reaper.start() + # The join times out (heartbeat blocked), so the destroy path returns while + # the refresh is still in flight — exactly where the old caller-side release ran. + reaper.join(timeout=10) + assert not reaper.is_alive(), "the destroy path never returned while the heartbeat was blocked" + + # Let the in-flight refresh complete: it rewrites `del:`. Wait until it has + # actually landed, so the assertion reads the settled state rather than the + # transient window between a caller-side release and this write. Only a + # heartbeat-owned release, sequenced after the refresh, can leave the id clean. + blocking.unblock.set() + assert blocking.refresh_completed.wait(timeout=5), "the in-flight refresh never completed" + + deadline = time.time() + 5 + while time.time() < deadline and shared.owner("defer1") is not None: + time.sleep(0.02) + assert shared.owner("defer1") is None, "a refresh that landed after the release stranded the id under a `del:` marker" + assert worker_b._ownership.take("defer1") is True + + +def test_evict_keeps_the_warm_entry_when_the_claim_is_refused(): + """Replica eviction must not pop before it knows the container is going away. + + The sibling of `test_refused_idle_destroy_keeps_the_warm_entry`, which pins + the same rule for the idle path. Popping first on a refused claim loses the + container: still running, owned by a peer, and no longer in any of our maps — + so nothing here would ever reap or reclaim it. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="peer01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-peer01", + created_at=time.time(), + ) + worker_a._warm_pool["peer01"] = (info, time.time() - 5) + # A peer owns it, so A's eviction claim is refused. + worker_b._publish_ownership("peer01") + + assert worker_a._evict_oldest_warm() is None + + worker_a._backend.destroy.assert_not_called() + assert "peer01" in worker_a._warm_pool, "evicting popped a container it was refused permission to destroy" + assert shared.owner("peer01") == "worker-b" + + +def test_reclaim_drops_a_container_a_peer_is_destroying(): + """The warm-pool half of the acquire-side teardown refusal. + + `test_cached_sandbox_being_destroyed_is_dropped_not_reused` pins the + in-process reuse path and `test_acquire_refuses_a_container_a_peer_is_destroying` + the discover path; reclaim is the third and had no test. It must not raise + (the caller falls through to a cold start) and must not leave the doomed + container in the warm pool. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="dying02", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-dying02", + created_at=time.time(), + ) + worker_a._warm_pool["dying02"] = (info, time.time()) + worker_a._check_tracked_sandbox_alive = MagicMock(return_value=True) + + # B's reaper marks the teardown; its stop is in flight. + assert worker_b._claim_ownership("dying02", for_destroy=True) is True + + reclaimed = worker_a._reclaim_warm_pool_sandbox("t1", "dying02", user_id="u1") + + assert reclaimed is None, "reclaimed a container a peer is tearing down" + assert "dying02" not in worker_a._warm_pool + assert "dying02" not in worker_a._sandboxes + worker_a._backend.destroy.assert_not_called() + + +def test_created_sandbox_is_rolled_back_when_a_peer_is_destroying_its_id(): + """Rollback must cover a teardown marker, not just a store outage. + + `test_ownership_rollback_on_create_closes_the_client_it_drops` drives this + path with `OwnershipBackendError` only. The comment says the teardown case is + reachable too — a peer that died mid-stop leaves a `del:` marker until its + TTL lapses — and without rollback the container we just started is leaked as + an adoptable orphan. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="fresh01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-fresh01", + created_at=time.time(), + ) + # A peer's teardown marker is still on this id when we finish creating. + assert worker_b._claim_ownership("fresh01", for_destroy=True) is True + + with pytest.raises(SandboxBeingDestroyedError): + worker_a._register_created_sandbox("t1", "fresh01", info, user_id="u1") + + worker_a._backend.destroy.assert_called_once_with(info) + assert "fresh01" not in worker_a._sandboxes, "a container we could not own was handed out anyway" + + +def test_shutdown_does_not_stop_a_peers_warm_container(): + """Shutdown is a reap path and must be gated like every other one. + + Nothing drove `shutdown()` with a non-empty warm pool, so a loop that called + `_backend.destroy` directly — skipping the ownership claim — would go + unnoticed. On a multi-instance gateway that is #4206 on the shutdown path: + our exit stops a container a live peer is serving. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + mine = SandboxInfo(sandbox_id="mine01", sandbox_url="http://localhost:8080", container_name="c-mine01", created_at=time.time()) + theirs = SandboxInfo(sandbox_id="peer02", sandbox_url="http://localhost:8081", container_name="c-peer02", created_at=time.time()) + + worker_a._warm_pool["mine01"] = (mine, time.time()) + worker_a._publish_ownership("mine01") + worker_a._warm_pool["peer02"] = (theirs, time.time()) + worker_b._publish_ownership("peer02") # a live peer owns this one + + worker_a.shutdown() + + destroyed = {call.args[0].sandbox_id for call in worker_a._backend.destroy.call_args_list} + assert "mine01" in destroyed, "shutdown left our own warm container running" + assert "peer02" not in destroyed, "shutdown stopped a container a live peer owns" + assert shared.owner("peer02") == "worker-b" + + +def test_teardown_heartbeat_stops_when_the_stop_returns(): + """A finite TTL must survive the fix, or a crashed destroyer leaks forever. + + The heartbeat is what holds the exclusion, so it has to die with the stop: + if it outlived the destroy the marker would be refreshed indefinitely and no + peer could ever adopt or recreate the container. + """ + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + shared = _make_shared_ownership_store(ttl_seconds=0.15) + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05, ttl_multiplier=3.0) + info = SandboxInfo( + sandbox_id="doomed2", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-doomed2", + created_at=time.time(), + ) + worker_a._warm_pool["doomed2"] = (info, time.time()) + + assert worker_a._destroy_warm_entry("doomed2", info, reason="idle_timeout", still_reapable=lambda: True) is True + + # Named rather than counted: threading.active_count() is global and other + # tests' idle-checker/renewal threads make it noise, so a count comparison + # here passes straight through a leak. + assert [t for t in threading.enumerate() if t.name == "sandbox-teardown-lease"] == [], "a teardown heartbeat thread outlived its stop" + + # And nothing keeps refreshing the marker past its TTL. + time.sleep(0.3) + assert shared.owner("doomed2") is None + + +def test_cached_sandbox_being_destroyed_is_dropped_not_reused(): + """The same window on the warm/in-process reuse path falls through cleanly.""" + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="dying02", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-dying02", + created_at=time.time(), + ) + sandbox = MagicMock() + worker_a._sandboxes["dying02"] = sandbox + worker_a._sandbox_infos["dying02"] = info + worker_a._thread_sandboxes[("u1", "t1")] = "dying02" + worker_a._check_tracked_sandbox_alive = MagicMock(return_value=True) + + worker_b._claim_ownership("dying02", for_destroy=True) + + # Returns None (not the id, and not an exception) so acquire cold-starts. + assert worker_a._reuse_in_process_sandbox("t1", user_id="u1") is None + assert "dying02" not in worker_a._sandboxes + worker_a._backend.destroy.assert_not_called() + + +def test_destroy_claims_before_untracking(): + """A refused claim must not lose the container from every map. + + Untracking first meant a peer-owned container was dropped from `_sandboxes` + and `_warm_pool` and then not destroyed — still running, and now invisible to + the instance that had been tracking it. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="peer01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-peer01", + created_at=time.time(), + ) + sandbox = MagicMock() + worker_a._sandboxes["peer01"] = sandbox + worker_a._sandbox_infos["peer01"] = info + worker_b._publish_ownership("peer01") + + worker_a.destroy("peer01") + + worker_a._backend.destroy.assert_not_called() + assert "peer01" in worker_a._sandboxes, "untracked a container it was refused permission to destroy" + sandbox.close.assert_not_called() + + +def test_refused_idle_destroy_keeps_the_warm_entry(): + """Popping before deciding loses the container: running, tracked by nobody.""" + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + worker_a._config["idle_timeout"] = 60 + info = SandboxInfo( + sandbox_id="warmpeer", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-warmpeer", + created_at=time.time(), + ) + worker_a._warm_pool["warmpeer"] = (info, time.time() - 999) + worker_b._publish_ownership("warmpeer") + + worker_a._reap_expired_warm(idle_timeout=60) + + worker_a._backend.destroy.assert_not_called() + assert "warmpeer" in worker_a._warm_pool, "dropped a warm entry it did not actually destroy" + + +def test_unhealthy_sandbox_owned_by_peer_is_not_destroyed(): + """The one reap path that used to skip the ownership gate entirely.""" + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = SandboxInfo( + sandbox_id="sick01", + sandbox_url="http://localhost:8080", + container_name="deer-flow-sandbox-sick01", + created_at=time.time(), + ) + worker_a._sandboxes["sick01"] = MagicMock() + worker_a._sandbox_infos["sick01"] = info + worker_b._publish_ownership("sick01") + + worker_a._drop_unhealthy_sandbox("sick01", "failed health check") + + worker_a._backend.destroy.assert_not_called() + assert shared.owner("sick01") == "worker-b" + + +def test_get_does_not_touch_ownership_store(): + """get() is a pure in-memory lookup — it must not do store IO. + + ``ensure_sandbox_initialized_async`` calls ``provider.get()`` directly on the + event loop, so store IO here would be blocking filesystem or network IO on + the hot path. Ownership is published on acquire and refreshed by the renewal + thread instead. + """ + worker = _make_provider_for_reconciliation(worker_id="worker-a") + sandbox = MagicMock() + worker._sandboxes["sb1"] = sandbox + worker._ownership = MagicMock() + + assert worker.get("sb1") is sandbox + + worker._ownership.take.assert_not_called() + worker._ownership.claim.assert_not_called() + worker._ownership.renew.assert_not_called() + worker._ownership.owner.assert_not_called() + + +def test_reconcile_multiple_containers_all_adopted(tmp_path): + """Multiple lease-free containers should all be adopted into warm pool.""" + provider = _make_provider_for_reconciliation(tmp_path) now = time.time() info1 = SandboxInfo(sandbox_id="cont_one", sandbox_url="http://localhost:8081", created_at=now - 1200) @@ -549,3 +2039,804 @@ def test_sighup_handler_registered(): signal.signal(signal.SIGHUP, original_sighup) signal.signal(signal.SIGTERM, original_sigterm) signal.signal(signal.SIGINT, original_sigint) + + +# ── Same-process reap vs. promote ──────────────────────────────────────────── +# +# The ownership store excludes peers and nothing else: `claim()` and `take()` +# both succeed against our *own* lease by design. So between this instance's +# reaper threads and its own acquire path there is no store-level exclusion at +# all, and every reaper decides outside `_lock` (a store round trip must not be +# held under the lock that guards every acquire). +# +# Each test below blocks its reaper inside the store round trip — where the +# window actually lives — lets a promote path run, then reads the SETTLED state +# after both threads have finished. The assertion is never on a transient. + + +def _active_sandbox(provider, sandbox_id, info, *, thread_key=("u", "t1")): + """Track *info* as an active sandbox on *provider*.""" + from deerflow.community.aio_sandbox.aio_sandbox import AioSandbox + + provider._sandboxes[sandbox_id] = AioSandbox(id=sandbox_id, base_url=info.sandbox_url) + provider._sandbox_infos[sandbox_id] = info + provider._last_activity[sandbox_id] = time.time() + provider._thread_sandboxes[thread_key] = sandbox_id + provider._backend.is_alive.return_value = True + + +def _info(sandbox_id, *, created_at=None): + return SandboxInfo( + sandbox_id=sandbox_id, + sandbox_url="http://localhost:8080", + container_name=f"deer-flow-sandbox-{sandbox_id}", + created_at=time.time() if created_at is None else created_at, + ) + + +class _GateOnClaim: + """Store view that holds the first ``claim`` until released. + + Models the Redis round trip the reaper makes outside ``_lock`` — the window + a same-process acquire slips through. + """ + + def __init__(self, inner): + self._inner = inner + self.entered = threading.Event() + # Not named `release`: __getattr__ forwards everything else to the real + # store, and an attribute by that name would shadow `store.release()`. + self.let_through = threading.Event() + self._armed = True + + def __getattr__(self, name): + return getattr(self._inner, name) + + @property + def supports_cross_process(self): + return True + + def claim(self, sandbox_id, *, for_destroy=False): + if self._armed: + self._armed = False + self.entered.set() + assert self.let_through.wait(timeout=5), "the test never released the gated claim" + return self._inner.claim(sandbox_id, for_destroy=for_destroy) + + +def _run_reap_vs_promote(provider, gate, reap, promote): + """Run *reap* until it blocks in the store, then *promote*; settle and return.""" + reaper = threading.Thread(target=reap, daemon=True) + reaper.start() + try: + assert gate.entered.wait(timeout=5), "the reaper never reached the ownership store" + handed_out = promote() + finally: + gate.let_through.set() + reaper.join(timeout=5) + assert not reaper.is_alive(), "the reaper never finished" + return handed_out + + +@pytest.mark.parametrize( + ("reason", "reap_name"), + [("replica_enforcement", "_evict_oldest_warm"), ("idle_timeout", "_reap_expired_warm")], +) +def test_warm_reaper_does_not_stop_a_container_this_instance_just_reclaimed(reason, reap_name): + """Both warm reapers defer the pop, so both need the same reservation. + + The deferred pop is deliberate — popping first loses the container on a + refused claim — but it leaves the entry visible in `_warm_pool` for the whole + stop, so `_reclaim_warm_pool_sandbox` can promote it and hand it to an agent + while the stop is in flight. `claim(for_destroy=True)` does not refuse this: + the lease is already `own:us`, and a claim only refuses *peers*. + + On `main` neither reaper could hit this — `WarmPoolLifecycleMixin` popped the + entry under the lock before destroying — so this is a regression these + overrides introduce, not a pre-existing gap. + """ + provider = _make_provider_for_reconciliation() + gate = _GateOnClaim(provider._ownership) + provider._ownership = gate + info = _info("warm1") + # Old enough for the idle reaper; the only entry, so also the eviction pick. + provider._warm_pool["warm1"] = (info, time.time() - 10_000) + provider._ownership._inner.take("warm1") + + reap = provider._evict_oldest_warm if reap_name == "_evict_oldest_warm" else lambda: provider._reap_expired_warm(600) + handed_out = _run_reap_vs_promote( + provider, + gate, + reap, + lambda: provider._reclaim_warm_pool_sandbox("t1", "warm1", user_id="u"), + ) + + assert handed_out is None, f"{reap_name} let a turn reclaim a container it was stopping ({reason})" + assert "warm1" not in provider._sandboxes, "the reclaimed sandbox was promoted into active tracking" + + +def test_idle_destroy_does_not_stop_a_container_this_instance_just_reused(): + """The idle checker's "still idle?" re-check must gate the reservation, not precede it. + + `_cleanup_idle_sandboxes` re-verified idleness under the lock and then called + `destroy()`, which claims ownership *before* untracking — so the re-check and + the act are separated by a store round trip. A turn that reuses the sandbox + in that window gets its container stopped mid-turn. + + The window is pre-existing in shape (main re-checks and destroys the same + way) but this PR widened it from a few instructions to a network round trip + by adding the ownership claim, so it is in scope here. + """ + provider = _make_provider_for_reconciliation() + gate = _GateOnClaim(provider._ownership) + provider._ownership = gate + info = _info("idle1") + _active_sandbox(provider, "idle1", info) + provider._last_activity["idle1"] = 0.0 # long idle + provider._ownership._inner.take("idle1") + + handed_out = _run_reap_vs_promote( + provider, + gate, + lambda: provider._cleanup_idle_sandboxes(600), + lambda: provider._reuse_in_process_sandbox("t1", user_id="u"), + ) + + assert handed_out is None, "a turn reused a sandbox whose container the idle checker was stopping" + + +def test_reuse_does_not_return_a_sandbox_reserved_during_its_health_check(): + """A teardown that wins during reuse's unlocked health check must stay won. + + Reuse checks the local teardown reservation before calling ``is_alive``, but + that backend call runs outside ``_lock``. The idle reaper can reserve the id + in that gap and then pause in its ownership claim while reuse publishes a + fresh ``own:`` lease. Map membership alone is not a safe final check: the + reaper deliberately keeps the active entry tracked until its claim succeeds. + """ + provider = _make_provider_for_reconciliation() + gate = _GateOnClaim(provider._ownership) + provider._ownership = gate + info = _info("idlerace") + _active_sandbox(provider, "idlerace", info) + provider._last_activity["idlerace"] = 0.0 + provider._ownership._inner.take("idlerace") + + health_started, let_health_finish = threading.Event(), threading.Event() + + def gated_health(_info): + health_started.set() + assert let_health_finish.wait(timeout=5), "the test never released the health check" + return True + + provider._backend.is_alive.side_effect = gated_health + result = {} + acquire = threading.Thread(target=lambda: result.update(id=provider._reuse_in_process_sandbox("t1", user_id="u")), daemon=True) + reaper = threading.Thread(target=lambda: provider._cleanup_idle_sandboxes(600), daemon=True) + + acquire.start() + try: + assert health_started.wait(timeout=5), "reuse never reached its unlocked health check" + reaper.start() + assert gate.entered.wait(timeout=5), "the idle reaper never reserved the sandbox and reached its ownership claim" + assert "idlerace" in provider._local_teardown, "precondition: the idle reaper must hold the local reservation" + + let_health_finish.set() + acquire.join(timeout=5) + assert not acquire.is_alive(), "reuse never completed" + handed_out = result.get("id") + finally: + let_health_finish.set() + gate.let_through.set() + acquire.join(timeout=5) + reaper.join(timeout=5) + + assert not reaper.is_alive(), "the idle reaper never finished" + assert handed_out is None, "reuse returned a sandbox whose local teardown reservation had already won" + provider._backend.destroy.assert_called_once_with(info) + assert provider.get("idlerace") is None + + +def test_discovery_does_not_install_a_sandbox_reserved_while_publishing(): + """Discovery must re-check the local reservation after its store round trip.""" + provider = _make_provider_for_reconciliation() + info = _info("discoverrace") + published, let_install = threading.Event(), threading.Event() + real_publish = provider._publish_ownership + + def gated_publish(sandbox_id): + result = real_publish(sandbox_id) + published.set() + assert let_install.wait(timeout=5), "the test never released discovery after publish" + return result + + provider._publish_ownership = gated_publish + result = {} + + def register(): + try: + result["id"] = provider._register_discovered_sandbox("t1", info, user_id="u") + except Exception as e: + result["error"] = e + + registrar = threading.Thread(target=register, daemon=True) + registrar.start() + try: + assert published.wait(timeout=5), "discovery never published ownership" + assert provider._reserve_local_teardown("discoverrace", lambda: True), "the test could not reserve the discovered sandbox" + let_install.set() + registrar.join(timeout=5) + finally: + let_install.set() + registrar.join(timeout=5) + provider._finish_local_teardown("discoverrace") + + assert not registrar.is_alive(), "discovery never completed" + assert isinstance(result.get("error"), SandboxBeingDestroyedError) + assert result.get("id") is None + assert "discoverrace" not in provider._sandboxes + assert provider.get("discoverrace") is None + + +def test_unhealthy_drop_refuses_discovery_of_the_container_it_is_stopping(): + """`_drop_unhealthy_sandbox` untracks first, so discovery is its open window. + + Once the maps are cleared, an acquire misses both caches and falls through to + backend discovery, which finds the still-running container. `take()` only + refuses a `del:` lease, and this path's claim has not run yet — so without a + local reservation the acquire is handed a container this instance is about + to stop. + """ + provider = _make_provider_for_reconciliation() + info = _info("sick1") + _active_sandbox(provider, "sick1", info) + provider._ownership.take("sick1") + # Block at the claim, not at the stop: after the claim the `del:` marker is + # already published and the store refuses the take by itself, so gating any + # later would pass without the reservation existing at all. The window that + # needs the reservation is untrack -> claim, where the lease still says + # `own:us` and `take()` succeeds. + gate = _GateOnClaim(provider._ownership) + provider._ownership = gate + + reaper = threading.Thread(target=lambda: provider._drop_unhealthy_sandbox("sick1", "failed health check"), daemon=True) + reaper.start() + try: + assert gate.entered.wait(timeout=5), "the drop never reached the ownership claim" + assert provider._ownership._inner.owner("sick1") == provider._owner_id, "precondition: the lease must still read as ours, not a teardown" + with pytest.raises(SandboxBeingDestroyedError): + provider._register_discovered_sandbox("t1", info, user_id="u") + finally: + gate.let_through.set() + reaper.join(timeout=5) + + assert "sick1" not in provider._sandboxes + + +def test_renewal_does_not_drop_a_sandbox_this_instance_re_acquired(): + """A `LOST` verdict is stale the moment an acquire takes the lease back. + + `_renew_owned_leases` calls `renew()` outside `_lock`; between that answer + and `_forget_lost_sandbox`'s pop, this instance's own acquire can `take()` + the lease back and hand the sandbox to a turn. The reuse path hands out the + **same** tracked `AioSandbox`, so an object-identity check would not notice — + the pop then closes a client mid-turn and the agent's tool calls fail until + the next turn re-discovers. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = _info("moved1") + _active_sandbox(worker_a, "moved1", info) + worker_b._ownership.take("moved1") # peer holds it -> renew() will report LOST + + entered = threading.Event() + release = threading.Event() + original_forget = worker_a._forget_lost_sandbox + + def gated_forget(sandbox_id, **kwargs): + entered.set() + assert release.wait(timeout=5) + return original_forget(sandbox_id, **kwargs) + + worker_a._forget_lost_sandbox = gated_forget + + reaper = threading.Thread(target=worker_a._renew_owned_leases, daemon=True) + reaper.start() + try: + assert entered.wait(timeout=5), "renewal never decided the lease was lost" + assert worker_a._reuse_in_process_sandbox("t1", user_id="u") == "moved1" + finally: + release.set() + reaper.join(timeout=5) + + assert shared.owner("moved1") == "worker-a", "the acquire should have taken the lease back" + assert "moved1" in worker_a._sandboxes, "renewal dropped a sandbox this instance owns again" + + +def test_release_does_not_drop_a_warm_entry_this_instance_re_acquired(): + """`release()` refreshes ownership outside the lock and has the same staleness. + + Sibling of the renewal path: the turn ends, `release()` parks the entry warm + and refreshes its lease, and the thread's *next* turn can reclaim it while + that round trip is in flight. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = _info("parked1") + _active_sandbox(worker_a, "parked1", info) + worker_b._ownership.take("parked1") # peer holds it -> refresh reports LOST + + entered = threading.Event() + release_gate = threading.Event() + original_forget = worker_a._forget_lost_sandbox + + def gated_forget(sandbox_id, **kwargs): + entered.set() + assert release_gate.wait(timeout=5) + return original_forget(sandbox_id, **kwargs) + + worker_a._forget_lost_sandbox = gated_forget + + reaper = threading.Thread(target=lambda: worker_a.release("parked1"), daemon=True) + reaper.start() + try: + assert entered.wait(timeout=5), "release never decided the lease was lost" + assert worker_a._reclaim_warm_pool_sandbox("t1", "parked1", user_id="u") == "parked1" + finally: + release_gate.set() + reaper.join(timeout=5) + + assert shared.owner("parked1") == "worker-a" + assert "parked1" in worker_a._sandboxes, "release dropped a warm entry this instance re-acquired" + + +def test_discovered_sandbox_client_is_closed_when_ownership_publish_fails(): + """The discover path owns a host-side client even though it owns no container. + + "Nothing to roll back" is true of the container — we did not create it — but + not of the `AioSandbox` HTTP client constructed before the publish. The + sibling create path already closes it on the same failure. + """ + from deerflow.community.aio_sandbox.ownership import OwnershipBackendError + + provider = _make_provider_for_reconciliation() + provider._ownership = MagicMock() + provider._ownership.take.side_effect = OwnershipBackendError("store down") + + created = [] + aio_mod = importlib.import_module("deerflow.community.aio_sandbox.aio_sandbox_provider") + real_sandbox_cls = aio_mod.AioSandbox + + def tracking_sandbox(**kwargs): + sandbox = real_sandbox_cls(**kwargs) + sandbox.close = MagicMock(side_effect=sandbox.close) + created.append(sandbox) + return sandbox + + with patch.object(aio_mod, "AioSandbox", side_effect=tracking_sandbox): + with pytest.raises(OwnershipBackendError): + provider._register_discovered_sandbox("t1", _info("found1"), user_id="u") + + assert len(created) == 1, "the discover path should have constructed exactly one client" + created[0].close.assert_called_once() + assert "found1" not in provider._sandboxes + + +def test_idle_destroy_skips_a_sandbox_re_acquired_after_the_idle_snapshot(): + """The idle predicate must read live activity, not just gate the reservation. + + `_cleanup_idle_sandboxes` snapshots idle candidates under the lock and acts + on them one at a time; a turn landing in between makes the snapshot wrong. + The reservation alone does not catch this — nothing is being torn down yet — + so the "still idle?" check has to travel with it as a live predicate. + """ + provider = _make_provider_for_reconciliation() + info = _info("idle2") + _active_sandbox(provider, "idle2", info) + provider._last_activity["idle2"] = 0.0 + provider._ownership.take("idle2") + + real_destroy_tracked = provider._destroy_tracked + + def turn_lands_first(sandbox_id, **kwargs): + # A turn arrives between the idle snapshot and the reservation. + provider._last_activity[sandbox_id] = time.time() + return real_destroy_tracked(sandbox_id, **kwargs) + + provider._destroy_tracked = turn_lands_first + provider._cleanup_idle_sandboxes(600) + + provider._backend.destroy.assert_not_called() + assert "idle2" in provider._sandboxes, "idle cleanup destroyed a sandbox that was re-acquired after the snapshot" + + +# ── Guard visibility vs. the transition it guards ──────────────────────────── +# +# A guard must become visible no later than the state transition it guards. The +# epoch alone cannot satisfy that for `take()`: the takeover is durable before +# `take()` returns (redis has committed the SET while the reply is in flight), +# and the epoch can only be written after it returns. So the acquire path +# publishes an *intent* mark before the round trip, and the epoch covers the +# other half — "an acquire completed since you decided". + + +class _TakeCommitsThenPauses: + """Store view whose ``take`` performs the real write, then pauses. + + Not an artificial injection point: on redis the server has committed the + takeover while the reply is still travelling back, so `own:us` is externally + visible before the caller resumes. + """ + + def __init__(self, inner): + self._inner = inner + self.committed = threading.Event() + self.let_return = threading.Event() + self._armed = True + + def __getattr__(self, name): + return getattr(self._inner, name) + + @property + def supports_cross_process(self): + return True + + def take(self, sandbox_id): + result = self._inner.take(sandbox_id) + if self._armed: + self._armed = False + self.committed.set() + assert self.let_return.wait(timeout=5), "the test never released take()" + return result + + +def _hold_forget(provider): + """Park *provider*'s next `_forget_lost_sandbox` and return its two events.""" + ready, release = threading.Event(), threading.Event() + original = provider._forget_lost_sandbox + + def gated(sandbox_id, **kwargs): + ready.set() + assert release.wait(timeout=5), "the test never released the forget" + return original(sandbox_id, **kwargs) + + provider._forget_lost_sandbox = gated + return ready, release + + +def test_renewal_does_not_drop_a_sandbox_whose_takeover_is_mid_flight(): + """The epoch is written after `take()` returns; the takeover is durable before it. + + In that interval the store already says the container is ours while the + epoch still reads as it did when the renewal decided `LOST`. Without a guard + established *before* the round trip, the stale forget walks through, drops + the maps and closes the client — and the acquire path then returns an id the + provider no longer tracks, so `get()` answers `None` for the rest of the turn. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = _info("midtake") + _active_sandbox(worker_a, "midtake", info) + worker_b._ownership.take("midtake") # peer holds it -> renew() reports LOST + + ready, release = _hold_forget(worker_a) + renewal = threading.Thread(target=worker_a._renew_owned_leases, daemon=True) + renewal.start() + assert ready.wait(timeout=5), "renewal never decided the lease was lost" + + gate = _TakeCommitsThenPauses(worker_a._ownership) + worker_a._ownership = gate + result = {} + acquire = threading.Thread(target=lambda: result.update(id=worker_a._reuse_in_process_sandbox("t1", user_id="u")), daemon=True) + acquire.start() + try: + assert gate.committed.wait(timeout=5), "take() never committed the takeover" + assert shared.owner("midtake") == "worker-a", "precondition: the takeover must already be durable" + release.set() + renewal.join(timeout=5) + finally: + gate.let_return.set() + acquire.join(timeout=5) + + assert result.get("id") == "midtake" + assert "midtake" in worker_a._sandboxes, "a stale LOST dropped a sandbox whose takeover was already committed" + assert worker_a.get("midtake") is not None, "acquire returned an id whose get() is None" + + +def test_reuse_falls_through_when_its_entry_is_dropped_while_publishing(): + """Before the intent mark is set, a `LOST` forget is both current and correct. + + Distinct from the mid-flight window: here nothing is wrong with the forget — + the peer really does hold the lease and no acquire has taken it back yet, so + the epoch matches and no intent is registered. The bug is on the other side: + reuse decided to hand out a tracked entry, and must not return that decision + once the entry is gone. Falling through re-discovers and builds a fresh + client; returning the id would hand back a sandbox whose `get()` is `None`. + """ + shared = _make_shared_ownership_store() + worker_a = _make_provider_for_reconciliation(worker_id="worker-a", store=shared) + worker_b = _make_provider_for_reconciliation(worker_id="worker-b", store=shared) + info = _info("prepub") + _active_sandbox(worker_a, "prepub", info) + worker_b._ownership.take("prepub") + + ready, release = _hold_forget(worker_a) + renewal = threading.Thread(target=worker_a._renew_owned_leases, daemon=True) + renewal.start() + assert ready.wait(timeout=5), "renewal never decided the lease was lost" + + # Park reuse in the gap between its map re-check and the intent mark. + at_publish, let_publish = threading.Event(), threading.Event() + real_publish = worker_a._publish_ownership + + def gated_publish(sandbox_id): + at_publish.set() + assert let_publish.wait(timeout=5) + return real_publish(sandbox_id) + + worker_a._publish_ownership = gated_publish + result = {} + acquire = threading.Thread(target=lambda: result.update(id=worker_a._reuse_in_process_sandbox("t1", user_id="u")), daemon=True) + acquire.start() + try: + assert at_publish.wait(timeout=5), "reuse never reached the ownership publish" + release.set() + renewal.join(timeout=5) + finally: + let_publish.set() + acquire.join(timeout=5) + + assert result.get("id") is None, "reuse returned an id it no longer tracks" + + +def test_reconcile_does_not_adopt_a_container_this_instance_is_tearing_down(): + """Adoption is a promote, so it needs the same reservation check as the rest. + + `_drop_unhealthy_sandbox` untracks before it claims, so in that window the + container is running, untracked, and still carries our own `own:` lease — + exactly the shape this loop adopts. Neither guard in the loop excludes it: + the claim succeeds because the lease is ours, and on the `memory` store the + recovery grace is skipped outright (`supports_cross_process = False`), so + nothing stands in the way at all. Adopting parks a container in the warm pool + moments before its stop lands, leaving a dead entry for the next reclaim. + """ + provider = _make_provider_for_reconciliation() + info = _info("reap1") + _active_sandbox(provider, "reap1", info) + provider._ownership.take("reap1") + provider._backend.list_running.return_value = [info] + + at_claim, let_claim = threading.Event(), threading.Event() + real_claim = provider._claim_ownership + + def gated_claim(sandbox_id, *, for_destroy=False): + if for_destroy: + at_claim.set() + assert let_claim.wait(timeout=5) + return real_claim(sandbox_id, for_destroy=for_destroy) + + provider._claim_ownership = gated_claim + reaper = threading.Thread(target=lambda: provider._drop_unhealthy_sandbox("reap1", "failed health check"), daemon=True) + reaper.start() + try: + assert at_claim.wait(timeout=5), "the drop never reached its claim" + # Untracked, still running, and the `del:` marker is not written yet. + provider._reconcile_orphans() + assert "reap1" not in provider._warm_pool, "reconcile adopted a container this instance is stopping" + finally: + let_claim.set() + reaper.join(timeout=5) + + +def test_teardown_reservation_is_cleared_once_the_stop_returns(): + """The reservation must not outlive the stop it guards. + + Release ordering is the mirror of acquire ordering: the heartbeat drops the + `del:` lease before `_finish_local_teardown` clears the local mark. That + direction is the safe one — the mark only refuses *our* promotes, and the + container is gone anyway — but a mark left behind would keep refusing this + thread's cold-start until the process restarts. + """ + provider = _make_provider_for_reconciliation() + info = _info("cleared") + provider._warm_pool["cleared"] = (info, time.time() - 10_000) + provider._ownership.take("cleared") + + assert provider._destroy_warm_entry("cleared", info, reason="idle_timeout", still_reapable=lambda: True) is True + assert provider._local_teardown == set(), "a teardown reservation outlived the stop it guarded" + assert provider._acquire_inflight == {}, "an acquire intent mark leaked" + + +def test_reclaim_does_not_hand_out_a_container_a_reaper_is_stopping(): + """Reclaim's reservation check runs before its round trip, so it must run again. + + A reaper can reserve *after* that first check — the warm entry is still + there, since the pop is deferred so a refused stop cannot lose the container + — and then claim `del:`, which succeeds because reclaim's own `take()` just + made the lease ours. With the reaper parked inside the container stop, the + entry is still in `_warm_pool` and still reserved, so only the re-check + stands between reclaim and installing a client for a dying container. + """ + provider = _make_provider_for_reconciliation() + info = _info("handout") + provider._warm_pool["handout"] = (info, time.time() - 10_000) + provider._ownership.take("handout") + + # Park reclaim between its `take()` and its install. + published, let_install = threading.Event(), threading.Event() + real_publish = provider._publish_ownership + + def gated_publish(sandbox_id): + result = real_publish(sandbox_id) + published.set() + assert let_install.wait(timeout=5) + return result + + provider._publish_ownership = gated_publish + out = {} + reclaim = threading.Thread(target=lambda: out.update(id=provider._reclaim_warm_pool_sandbox("t1", "handout", user_id="u")), daemon=True) + reclaim.start() + assert published.wait(timeout=5), "reclaim never published ownership" + + # Reaper reserves and enters the stop, then parks there -- so when reclaim + # resumes the entry is still in `_warm_pool` and the reservation is held. + in_stop, let_stop_finish = threading.Event(), threading.Event() + provider._backend.destroy = MagicMock(side_effect=lambda entry: (in_stop.set(), let_stop_finish.wait(timeout=5))) + reaper = threading.Thread(target=lambda: provider._reap_expired_warm(600), daemon=True) + reaper.start() + try: + assert in_stop.wait(timeout=5), "the reaper never reached the container stop" + assert "handout" in provider._warm_pool, "precondition: the pop must still be pending" + let_install.set() + reclaim.join(timeout=5) + finally: + let_stop_finish.set() + reaper.join(timeout=5) + + assert out.get("id") is None, "reclaim handed out a container a reaper was stopping" + assert "handout" not in provider._sandboxes + + +def test_warm_entry_is_removed_before_its_teardown_reservation_is_released(): + """Removal must happen under the reservation, not after it. + + The reservation is what keeps promotes off the entry. If it is released when + the stop returns and the entry is removed afterwards, there is a gap in which + the container is already stopped, the entry is still in `_warm_pool`, and + nothing marks it — so a reclaim in that gap re-checks the reservation, finds + it clear, and hands out a dead container. Ordering is the guarantee, so the + assertion is on the ordering rather than on one interleaving. + """ + provider = _make_provider_for_reconciliation() + info = _info("ordered") + provider._warm_pool["ordered"] = (info, time.time() - 10_000) + provider._ownership.take("ordered") + + observed = {} + real_finish = provider._finish_local_teardown + + def spy(sandbox_id): + observed["warm_at_release"] = sandbox_id in provider._warm_pool + return real_finish(sandbox_id) + + provider._finish_local_teardown = spy + assert provider._destroy_warm_entry("ordered", info, reason="idle_timeout", still_reapable=lambda: True) is True + + assert observed["warm_at_release"] is False, "the warm entry outlived the reservation that protected it" + assert "ordered" not in provider._warm_pool + + +def test_reclaim_short_circuits_a_reserved_entry_without_touching_the_backend(): + """The pre-round-trip reservation check earns its keep as an early-out. + + Correctness is covered by the re-check after the publish, so this first check + is not what makes reclaim safe. What it does is refuse a doomed entry before + spending a container health check and an ownership round trip on it, so that + is what this pins — otherwise the clause is unanchored and a later edit can + drop it silently. + """ + provider = _make_provider_for_reconciliation() + info = _info("shortcut") + provider._warm_pool["shortcut"] = (info, time.time()) + provider._ownership = MagicMock(wraps=provider._ownership) + with provider._lock: + provider._local_teardown.add("shortcut") + + assert provider._reclaim_warm_pool_sandbox("t1", "shortcut", user_id="u") is None + provider._backend.is_alive.assert_not_called() + provider._ownership.take.assert_not_called() + + +def test_forget_without_an_epoch_still_refuses_an_id_being_acquired(): + """ "No epoch supplied" must not read as "no guard at all". + + The epoch-less callers are the two `SandboxBeingDestroyedError` handlers, + which cannot collide with a publish for the same id today. This pins the + primitive's contract rather than that reachability argument: an id whose + acquire is mid-publish is never dropped, whoever asks. + """ + provider = _make_provider_for_reconciliation() + info = _info("guarded") + _active_sandbox(provider, "guarded", info) + with provider._lock: + provider._acquire_inflight["guarded"] = 1 + + provider._forget_lost_sandbox("guarded") + + assert "guarded" in provider._sandboxes, "an id being acquired was dropped by an epoch-less forget" + + +@pytest.mark.parametrize("register", ["discovered", "created"]) +def test_registering_a_sandbox_clears_any_stale_warm_entry(register): + """Active and warm are exclusive states, and only register can violate it. + + Reconciliation adopts an untracked-but-running container into the warm pool, + and on the `memory` store that happens on sight — `_adoptable_after_grace` + short-circuits when `supports_cross_process` is False, so an id carrying this + process's *own* lease is treated as adoptable. That is reachable during the + publish → track window of either register path, which this branch introduced + (on `main` the track was a single locked insert with nothing before it). + + Leaving both entries gives one container two reapers: `_reap_expired_warm` + judges it by the warm timestamp and never consults `_last_activity`, so it + stops a container an agent is using while `_sandboxes` still hands out its + client — #4206's symptom on the default backend. + + The exclusivity is what actually fixes this, so it is asserted directly and + on both register paths rather than through one interleaving. + """ + provider = _make_provider_for_reconciliation() + info = _info("stale") + provider._warm_pool["stale"] = (info, time.time()) + + if register == "discovered": + provider._register_discovered_sandbox("t1", info, user_id="u") + else: + provider._register_created_sandbox("t1", "stale", info, user_id="u") + + assert "stale" in provider._sandboxes + assert "stale" not in provider._warm_pool, "a stale warm entry survived the id becoming active" + + +def test_a_container_adopted_during_register_is_not_reaped_from_the_warm_pool(): + """The end-to-end harm the exclusivity rule removes. + + Reconcile runs inside the register's publish → track window and adopts the + container; once tracking lands, the warm entry must be gone, otherwise warm + expiry stops the container this turn is holding. + """ + provider = _make_provider_for_reconciliation() + info = _info("adopted") + provider._backend.list_running.return_value = [info] + provider._backend.is_alive.return_value = True + + at_gap, go = threading.Event(), threading.Event() + real_publish = provider._publish_ownership + + def gated_publish(sandbox_id): + result = real_publish(sandbox_id) + at_gap.set() + assert go.wait(timeout=5) + return result + + provider._publish_ownership = gated_publish + out = {} + registrar = threading.Thread(target=lambda: out.update(id=provider._register_discovered_sandbox("t1", info, user_id="u")), daemon=True) + registrar.start() + try: + assert at_gap.wait(timeout=5), "register never reached the publish → track window" + provider._reconcile_orphans() + finally: + go.set() + registrar.join(timeout=5) + + assert out.get("id") == "adopted" + assert "adopted" not in provider._warm_pool, "the adopted entry survived, giving the container two reapers" + + time.sleep(0.02) + provider._reap_expired_warm(0.01) + provider._backend.destroy.assert_not_called() + assert provider.get("adopted") is not None, "warm expiry stopped a container this turn is holding" diff --git a/backend/tests/test_sandbox_ownership_store.py b/backend/tests/test_sandbox_ownership_store.py new file mode 100644 index 000000000..55d6aca2c --- /dev/null +++ b/backend/tests/test_sandbox_ownership_store.py @@ -0,0 +1,512 @@ +"""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() diff --git a/config.example.yaml b/config.example.yaml index 23b8a7a93..df3d8c2ad 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -1185,6 +1185,54 @@ sandbox: # # DEBUG: "false" # # API_KEY: $MY_API_KEY # Reads from host's MY_API_KEY env var # # DATABASE_URL: $DATABASE_URL # Reads from host's DATABASE_URL env var +# +# # Optional: Cross-instance container ownership (issue #4206). +# # +# # Gateway instances share sandbox containers but each keeps its own in-memory +# # warm pool. Without shared ownership state, one instance's startup +# # reconciliation adopts a container another instance is actively using and +# # later idle-destroys it — tool calls then fail with 502 / connection refused. +# # +# # Single gateway instance? Leave this out; `memory` is the default and the +# # cross-instance kill cannot happen. +# # +# # MULTIPLE gateway instances / workers sharing one container backend +# # (load-balanced deployments, Docker Compose) MUST set type: redis. Docker +# # Compose already sets DEER_FLOW_STREAM_BRIDGE_REDIS_URL, which is taken as +# # proof the deployment is multi-instance, so redis ownership is inferred even +# # if this section is omitted. +# # +# # The redis ownership store requires the optional `redis` extra. It is +# # auto-detected from this section on `make dev` and always installed in the +# # Docker image. To install it manually: +# # cd backend && uv sync --all-packages --extra redis +# # +# # ownership: +# # type: memory # single gateway instance only +# # +# # ownership: +# # type: redis # required for multi-instance / load-balanced +# # redis_url: redis://redis:6379/0 +# # renewal_interval_seconds: 30 # how often an owner refreshes its leases +# # ttl_multiplier: 4 # lease TTL = interval x this (min 2, so a +# # # single missed renewal cannot expire a live +# # # owner). Liveness is deliberately independent +# # # of idle_timeout: renewal keeps running even +# # # at idle_timeout: 0. +# # key_prefix: deerflow:sandbox:owner +# # +# # NOTE: the redis ownership store is fail-closed, matching the stream bridge's +# # fail-hard policy. Redis.from_url is lazy so a down Redis does not block +# # startup, but a sandbox whose ownership cannot be published is not handed out +# # — acquiring raises instead. The alternative (proceed unowned) is exactly the +# # #4206 cross-instance kill. Run Redis with HA / a restart policy. +# # +# # NOTE: the other boundary is the lease TTL (renewal_interval_seconds x +# # ttl_multiplier). A Redis outage longer than the TTL can let a reconciling +# # instance adopt a live owner's container: a lapsed lease is indistinguishable +# # from a dead owner, so one TTL of adoption grace is the whole safety margin. +# # Size the TTL against your Redis availability target (HA Redis keeps this +# # window out of reach). # Option 3: BoxLite micro-VM Sandbox # Runs each sandbox as a BoxLite micro-VM. Released boxes stay in an in-process diff --git a/scripts/detect_uv_extras.py b/scripts/detect_uv_extras.py index f8b774dcf..078d978e4 100755 --- a/scripts/detect_uv_extras.py +++ b/scripts/detect_uv_extras.py @@ -11,8 +11,10 @@ Order of resolution: - database.backend == postgres -> postgres - checkpointer.type == postgres -> postgres - stream_bridge.type == redis -> redis + - sandbox.ownership.type == redis -> redis 3. Runtime environment toggles that enable optional backends: - DEER_FLOW_STREAM_BRIDGE_REDIS_URL -> redis + - DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL -> redis Each extra name is validated against ``^[A-Za-z][A-Za-z0-9_-]*$`` (the same shape uv enforces for `[project.optional-dependencies]` keys). Anything else @@ -233,6 +235,8 @@ def detect_from_config(path: Path) -> list[str]: extras.add("postgres") if (section_value(lines, "stream_bridge", "type") or "").lower() == "redis": extras.add("redis") + if (nested_section_value(lines, "sandbox.ownership", "type") or "").lower() == "redis": + extras.add("redis") if (nested_section_value(lines, "channels.discord", "enabled") or "").lower() == "true": extras.add("discord") return sorted(extras) @@ -242,6 +246,8 @@ def detect_from_runtime_env() -> list[str]: extras: set[str] = set() if os.environ.get("DEER_FLOW_STREAM_BRIDGE_REDIS_URL", "").strip(): extras.add("redis") + if os.environ.get("DEER_FLOW_SANDBOX_OWNERSHIP_REDIS_URL", "").strip(): + extras.add("redis") return sorted(extras)