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 <willem.jiang@gmail.com>

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

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

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 `<available_skills>` prompt block with a compact `<skill_index>` (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)

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."""

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.

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",

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()