From a89cd2682cb95ff5baf26550dc8de86c0e6ed661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stellar=E9=B1=BC?= <2182712990@qq.com> Date: Thu, 24 Sep 2026 18:11:35 +0800 Subject: [PATCH] fix(sandbox): reject overflowing ownership lease TTL (#5104) * fix(sandbox): reject overflowing ownership lease TTL * fix(sandbox): bound Redis ownership lease TTL * fix(sandbox): reserve Redis lease expiry headroom * fix(sandbox): reject sub-millisecond Redis lease TTL * fix(sandbox): preserve fractional Redis lease TTLs * docs(sandbox): align ownership TTL constraints * docs(tests): align ownership TTL comment --------- Co-authored-by: Willem Jiang --- .../aio_sandbox/aio_sandbox_provider.py | 10 +-- .../community/aio_sandbox/ownership/redis.py | 5 +- .../harness/deerflow/config/AGENTS.md | 7 ++ .../harness/deerflow/config/sandbox_config.py | 21 ++++- .../harness/deerflow/sandbox/AGENTS.md | 2 +- .../test_sandbox_orphan_reconciliation.py | 2 +- backend/tests/test_sandbox_ownership_store.py | 90 ++++++++++++++++++- 7 files changed, 127 insertions(+), 10 deletions(-) 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 c343d25483..61d9378c4b 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 @@ -594,11 +594,11 @@ class AioSandboxProvider(WarmPoolLifecycleMixin[SandboxInfo], SandboxProvider): 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. + abnormal backend — the config schema permits very short derived TTLs + (Redis down to 1 ms), so a legal setting can put 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 diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py index 923ab563d6..6008d7731b 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/ownership/redis.py @@ -21,6 +21,7 @@ closes. from __future__ import annotations import logging +import math from .base import OwnershipBackendError, RenewOutcome, SandboxOwnershipStore @@ -132,7 +133,9 @@ class RedisOwnershipStore(SandboxOwnershipStore): client: Redis | None = None, ) -> None: self._owner_id = owner_id - self._ttl_ms = max(1, int(float(ttl_seconds) * 1000)) + # Redis accepts integer milliseconds; round up so conversion never + # shortens the configured lease. + self._ttl_ms = max(1, math.ceil(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 diff --git a/backend/packages/harness/deerflow/config/AGENTS.md b/backend/packages/harness/deerflow/config/AGENTS.md index c33c44ab36..dced05055d 100644 --- a/backend/packages/harness/deerflow/config/AGENTS.md +++ b/backend/packages/harness/deerflow/config/AGENTS.md @@ -88,6 +88,13 @@ Extensions are optional only in the fallback *search* mode (priority 3-4 above): - `tools[]` - Tool configs with `use` variable path and `group` - `tool_groups[]` - Logical groupings for tools - `sandbox.use` - Sandbox provider class path +- `sandbox.ownership` - Cross-instance sandbox lease storage. Renewal interval + and TTL multiplier must each be finite, and their derived lease TTL must also + remain finite. Redis-backed lease TTLs must be at least one millisecond and + use at most half of Redis's signed 64-bit millisecond range, leaving + deterministic headroom for conversion from a relative TTL to an absolute Unix + timestamp during validation. The Redis store rounds fractional milliseconds + up so its integer TTL never shortens the validated lease. - `skills.path` / `skills.container_path` - Host and container paths to skills directory. AIO and E2B snapshot the container path at provider startup. Their local/remote backends and the Kubernetes provisioner require one canonical absolute non-root path outside reserved platform mounts; custom roots participate in deterministic sandbox identity, and E2B records the root in remote metadata. - `skills.deferred_discovery` - When `true`, replaces the full-metadata `` prompt block with a compact `` (names only) and registers the `describe_skill` tool so the agent fetches metadata on demand. Defaults to `false` (legacy full-metadata injection) - `title` - Auto-title generation (enabled, max_words, max_chars, model_name; null model_name uses fast local fallback, explicit model_name uses the prompt_template LLM path) diff --git a/backend/packages/harness/deerflow/config/sandbox_config.py b/backend/packages/harness/deerflow/config/sandbox_config.py index b6d5f4aaef..4ce45a4ace 100644 --- a/backend/packages/harness/deerflow/config/sandbox_config.py +++ b/backend/packages/harness/deerflow/config/sandbox_config.py @@ -1,13 +1,19 @@ import ipaddress +import math from typing import Literal -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator SandboxOwnershipType = Literal["memory", "redis"] SandboxOverflowPolicy = Literal["wait", "reject", "burst"] SandboxNetworkMode = Literal["open", "isolated", "allowlist"] SandboxNetworkApproval = Literal["deny", "prompt"] +# Redis converts relative PX values to absolute Unix-millisecond timestamps. +# Reserving half the signed range for that timestamp keeps accepted TTLs usable +# without making config validation depend on the current clock. +_REDIS_MAX_SAFE_TTL_MILLISECONDS = (2**63 - 1) // 2 + class SandboxNetworkConfig(BaseModel): """Outbound network policy for locally managed AIO sandboxes.""" @@ -111,6 +117,19 @@ class SandboxOwnershipConfig(BaseModel): description="Redis key prefix for ownership leases. Only applies to the redis ownership type.", ) + @model_validator(mode="after") + def validate_lease_ttl(self) -> "SandboxOwnershipConfig": + lease_ttl_seconds = self.renewal_interval_seconds * self.ttl_multiplier + if not math.isfinite(lease_ttl_seconds): + raise ValueError("sandbox.ownership lease TTL must be finite") + if self.type == "redis": + lease_ttl_milliseconds = lease_ttl_seconds * 1000 + if lease_ttl_milliseconds < 1: + raise ValueError("sandbox.ownership Redis lease TTL must be at least 1 millisecond") + if lease_ttl_milliseconds > _REDIS_MAX_SAFE_TTL_MILLISECONDS: + raise ValueError("sandbox.ownership Redis lease TTL must fit the signed 64-bit millisecond range with absolute-expiry headroom") + return self + class VolumeMountConfig(BaseModel): """Configuration for a volume mount.""" diff --git a/backend/packages/harness/deerflow/sandbox/AGENTS.md b/backend/packages/harness/deerflow/sandbox/AGENTS.md index 71748f9cf9..8fee9d4eee 100644 --- a/backend/packages/harness/deerflow/sandbox/AGENTS.md +++ b/backend/packages/harness/deerflow/sandbox/AGENTS.md @@ -71,7 +71,7 @@ to respond. - **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. + - **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 permits very short derived TTLs (Redis down to 1 ms), so a legal config can put 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. diff --git a/backend/tests/test_sandbox_orphan_reconciliation.py b/backend/tests/test_sandbox_orphan_reconciliation.py index c9e2856624..e788d1200e 100644 --- a/backend/tests/test_sandbox_orphan_reconciliation.py +++ b/backend/tests/test_sandbox_orphan_reconciliation.py @@ -1565,7 +1565,7 @@ def test_teardown_marker_is_held_for_a_stop_that_outlives_the_lease_ttl(): 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. + # A legal memory-backed config can still use a short 150 ms derived TTL. worker_a._ownership_config = SandboxOwnershipConfig(renewal_interval_seconds=0.05, ttl_multiplier=3.0) info = SandboxInfo( sandbox_id="doomed1", diff --git a/backend/tests/test_sandbox_ownership_store.py b/backend/tests/test_sandbox_ownership_store.py index 89bfe40012..ea4498fea1 100644 --- a/backend/tests/test_sandbox_ownership_store.py +++ b/backend/tests/test_sandbox_ownership_store.py @@ -19,6 +19,7 @@ import os import threading import time import uuid +from unittest.mock import MagicMock import pytest @@ -359,10 +360,97 @@ def test_ttl_multiplier_below_two_is_rejected(): @pytest.mark.parametrize("field", ["renewal_interval_seconds", "ttl_multiplier"]) @pytest.mark.parametrize("value", [float("inf"), float("-inf"), float("nan")]) def test_lease_timing_rejects_non_finite_values(field, value): - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="finite"): SandboxOwnershipConfig(**{field: value}) +def test_lease_timing_rejects_finite_values_with_infinite_product(): + with pytest.raises(ValueError, match="lease TTL must be finite"): + SandboxOwnershipConfig( + renewal_interval_seconds=1e308, + ttl_multiplier=4.0, + ) + + +def test_redis_lease_timing_rejects_ttl_above_signed_64_bit_milliseconds(): + ttl_seconds_above_limit = 2**63 / 1000 + + with pytest.raises(ValueError, match="signed 64-bit millisecond range"): + SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=ttl_seconds_above_limit / 4, + ttl_multiplier=4.0, + ) + + +def test_redis_lease_timing_rejects_ttl_without_absolute_expiry_headroom(): + ttl_seconds_at_safe_limit = 2**62 / 1000 + + with pytest.raises(ValueError, match="absolute-expiry headroom"): + SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=ttl_seconds_at_safe_limit / 4, + ttl_multiplier=4.0, + ) + + +def test_redis_lease_timing_rejects_ttl_below_one_millisecond(): + with pytest.raises(ValueError, match="at least 1 millisecond"): + SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=0.0004, + ttl_multiplier=2, + ) + + +def test_redis_lease_timing_allows_one_millisecond_ttl(): + config = SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=0.0005, + ttl_multiplier=2, + ) + + assert compute_lease_ttl(config) * 1000 == pytest.approx(1) + + +def test_redis_lease_timing_allows_operational_ttl(): + config = SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=60 * 60, + ttl_multiplier=24, + ) + + assert compute_lease_ttl(config) == 24 * 60 * 60 + + +@pytest.mark.parametrize( + ("renewal_interval_seconds", "expected_ttl_milliseconds"), + [ + pytest.param(0.0009, 2, id="lower-bound"), + pytest.param((2**62 - 1024) / 2000, 2**62 - 1024, id="upper-bound"), + ], +) +def test_redis_store_rounds_validated_ttl_up_near_range_boundaries(renewal_interval_seconds, expected_ttl_milliseconds): + from deerflow.community.aio_sandbox.ownership.redis import RedisOwnershipStore + + config = SandboxOwnershipConfig( + type="redis", + renewal_interval_seconds=renewal_interval_seconds, + ttl_multiplier=2, + ) + ttl_seconds = compute_lease_ttl(config) + + store = RedisOwnershipStore( + owner_id="A", + redis_url="redis://unused", + ttl_seconds=ttl_seconds, + client=MagicMock(), + ) + + assert store._ttl_ms == expected_ttl_milliseconds + assert store._ttl_ms >= ttl_seconds * 1000 + + 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()