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)