fix(sandbox): stop multi-worker orphan reconcile from killing peer sandboxes

[AnnaSuSu]

Summary

Fixes #4206.

Reworked per @WillemJiang's review: ownership state is now shared through a third-party service instead of being maintained per gateway instance, following the stream_bridge precedent (type: memory | redis).

The previous revision of this PR coordinated ownership through file leases + a same-host flock guard. That only covers multiple worker processes on one host, but the deployment that actually hits #4206 is a load-balanced multi-instance gateway. The file lease and the flock guard are deleted, not ported.

It also fixes the two protocol-level bugs @fancyboi999 found on the previous revision. Both were store-agnostic — they survive the mechanism swap verbatim — so they are fixed here rather than left behind with the code that carried them.

Approach

New pluggable ownership store at community/aio_sandbox/ownership/, selected by sandbox.ownership.type and resolved exactly like stream_bridge: factory dispatch on config.type, lazy per-branch import (redis stays out of memory-only installs), the existing optional redis extra (plus scripts/detect_uv_extras.py auto-detection), an env escape hatch, and an injectable client seam for tests.

memory is the default and, like stream_bridge's memory backend, is single-instance only — it declares supports_cross_process = False and the provider logs a warning at startup when the configured store cannot see peers. Multi-instance deployments do not have to notice that: the resolver reads app_config.stream_bridge and DEER_FLOW_STREAM_BRIDGE_REDIS_URL, in the same order the bridge's own resolver uses, so any deployment already pointing the stream bridge at Redis — i.e. every load-balanced one, since the bridge requires it — gets a redis ownership store with no extra config.

One deliberate divergence from stream_bridge: the store interface is sync, not async. StreamBridge is async because it is driven from the event loop. Ownership is the opposite — its callers are AioSandboxProvider.__init__ / _reconcile_orphans, the background idle and renewal threads, and release() from the sync after_agent hook. An async ABC would force every one of those sync paths to marshal onto a loop for no benefit. So this copies the pluggability shape — type: discriminator, config model, factory with lazy imports, optional extra, injectable client, supports_cross_process capability flag, redis not re-exported from __init__ — rather than the async-ness. The one path that does run on the event loop (get()) deliberately never touches the store at all.

A lease answers "who is responsible for reaping this container", not "who may use it". That distinction drives the interface, and getting it wrong strands threads:

• take() — acquire path, takes over from whoever held it. A container is deterministic per (user, thread), so consecutive turns of one thread legitimately land on different instances. A conditional claim here would refuse while the previous instance's lease was still live and break every load-balanced follow-up turn.
• claim() — succeeds only if unowned or already ours; gates every adopt/reap path. This is 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206.
• release() never clears a peer's lease.

A lease carries a state, and that is what closes the destroy window without a mutex. own:<owner> means "responsible for this container"; del:<owner> (claim(..., for_destroy=True)) means "tearing it down". take() is refused against a del: lease. Without that, an unconditional takeover would silently overwrite a destroyer's claim: B claims container X for destroy and starts the (multi-second) container stop, a turn for X's thread routes to A, A takes the lease and hands X to an agent, B's stop lands, and the agent gets 502 mid-turn — #4206 with extra steps. The previous revision covered this window with a per-sandbox flock; the two-state lease covers it atomically instead, which is what lets the flock go rather than be ported. A destroyer that dies mid-stop leaves a marker that lapses with the TTL.

Each mutation is one Lua script: 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.

renew() distinguishes a lapsed lease from a lost one (RenewOutcome.LAPSED / LOST), and the caller decides. A lapsed lease is absent — nobody took it — so it is re-established; a lease a peer holds is never re-taken. Collapsing the two into one falsy value meant a Redis restart without persistence (every key gone) would drop every in-flight sandbox on every instance at once, even though the containers were fine and unclaimed.

[P1, @fancyboi999] Renewal no longer rides the idle checker. _renew_active_leases had exactly one caller (_cleanup_idle_resources), and __init__ only starts the idle checker when idle_timeout > 0. config.example.yaml documents 0 as "keep warm VMs until shutdown", so on that supported config nothing ever refreshed a lease and #4206 returned one TTL later. Renewal is now its own bounded-interval thread, and the TTL derives from that interval (renewal_interval_seconds × ttl_multiplier, multiplier ≥ 2 so one missed renewal cannot expire a live owner) — never from idle_timeout. Liveness and reaping no longer share a switch. Renewal covers warm entries too, and losing a lease drops the sandbox from this instance's maps without touching the container — destroying it there would be the very cross-instance kill this store prevents.

[P1, @fancyboi999] Ownership establishment is fail-closed. Every publish helper used to swallow OSError into a warning while the caller returned the sandbox id on the next line, so a read-only lease dir or a failed write silently disabled the only cross-instance exclusion while the sandbox was handed out as usable. Now a sandbox whose ownership cannot be published is never handed out: acquiring raises, and a just-created container is destroyed rather than leaked as an adoptable orphan. Reaping fails closed symmetrically — a store that cannot answer counts as peer-owned. This matches the stream bridge's documented fail-hard v1 policy.

Test plan

• Store contract (tests/test_sandbox_ownership_store.py) — one parametrized suite run against both backends, so memory and redis cannot drift on the semantics the provider depends on: claim exclusivity, failed claim does not steal, take transfers from a live peer and makes the old owner's renew report LOST, take refused during a teardown and allowed once released, a stale teardown marker expires, renew reports LAPSED (not LOST) for an absent lease and never re-acquires on its own, release only drops our own, TTL expiry frees a crashed owner's container
• Provider behavior (tests/test_sandbox_orphan_reconciliation.py) — two providers sharing one store (models two instances on one Redis): peer-owned container not adopted / not reaped, the 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 issue-log path, crashed-owner container adoptable after TTL, fail-closed on publish + destroy + reconcile, __init__ starts renewal at idle_timeout: 0 and 600, lost lease drops tracking without destroying, acquire takes over so a thread can move instances, acquire refuses a container a peer is destroying, a store losing all state does not evict live sandboxes, destroy claims before untracking, a refused idle destroy keeps its warm entry, an unhealthy peer-owned sandbox is not destroyed
• Blocking-IO gate (tests/blocking_io/test_aio_sandbox_get.py) — get() stays a pure in-memory lookup. Rewritten to inject a deliberately-blocking probe store, so the anchor keeps its teeth regardless of which backend is configured; plus a meta-test proving the probe actually trips the gate (otherwise the anchor could pass vacuously)
• Red check, per test: reverting each mechanism turns the matching tests red — fail-open publish → the 2 fail-closed acquire tests; renewal coupled back to the idle checker → test_init_always_starts_lease_renewal[0] and [600]; destroy skipping the ownership check → 3 tests; unconditional reconcile adopt (the original 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206) → 4 tests; take() made conditional → 4 tests; take() ignoring the teardown state → 4 tests; renew() collapsing lapsed into lost → 3 tests; destroy untracking before its claim → 1 test
• Full backend suite: 7586 passed. 8 failures in test_browserless_client / test_crawl4ai_tools / test_fastcrw_tools are environmental on my machine (local DNS resolves example.com to 198.18.4.13, inside the reserved 198.18.0.0/15, so the SSRF guard correctly refuses); those three modules reference nothing from this diff
• cd backend && make lint / make format clean

Known coverage gap (flagging deliberately): the redis tier is @pytest.mark.integration + self-skipping, per this repo's documented policy (backend/pyproject.toml: "integration: tests that require an external service (e.g. Redis); skipped when unavailable"), so CI does not exercise the Lua scripts — same status as the existing stream-bridge integration tier. I ran that tier locally against a real Redis 7.2.3 (40 passed, DEER_FLOW_TEST_REDIS_URL=redis://localhost:6399/15). I deliberately did not add fakeredis[lua]: its Lua is a reimplementation of Redis semantics, so it would give false confidence on precisely the atomicity this depends on, and it drags a C extension into the dev group for every contributor. If you want this covered in CI, the honest fix is a redis service container in backend-unit-tests.yml — happy to do it, but it also activates the currently-skipped stream-bridge integration tests, so it felt like it belonged outside a bug-fix PR rather than bundled in silently.

Out of scope (pre-existing, not introduced here): SandboxMiddleware._acquire_sandbox_async calls get_sandbox_provider() directly on the event loop, so the first acquire constructs the provider and runs _reconcile_orphans() there. That path already did blocking container-backend IO; with type: redis it becomes network IO. Same shape, worth a separate look.

AI assistance

How you used it: Used Claude Code to map the stream_bridge factory/config/extra wiring onto sandbox ownership, work out which provider call sites are sync-only, draft the store plus tests, and then to review the result adversarially — that review is where several of the bugs below were caught. I verified everything myself against a real Redis, driving two AioSandboxProvider instances through the real provider methods over a shared container backend: a peer neither adopts nor destroys a live owner's container; a thread whose next turn moves instance still acquires (the previous owner then drops tracking without touching the container); a crashed owner's container becomes adoptable once its lease expires. I reproduced each bug end-to-end before fixing it rather than trusting the tests — at idle_timeout: 0 the old renewal wiring left owner = None after the TTL and the peer destroyed the live container, and for the destroy race I gave the fake container stop a real delay so the interleaving was genuine: without the teardown state the acquiring instance was handed a container the reaper then killed (502 mid-turn), with it the acquire is refused and cold-starts. I also ran the per-test red check described above, the full suite, and ruff.

Two things worth surfacing for review, since both were my own mistakes:

1. My first draft used a conditional claim on the acquire path. That is wrong — worker A holding the lease would have made thread T's next turn on worker B fail, so ownership had to split into take (acquire) and claim (adopt/reap). test_acquire_takes_over_ownership_so_a_thread_can_move_instances pins it.
2. My second draft then claimed "a held claim is the exclusion, so the flock guard is unnecessary" — and that claim was false, because take() ignored it. The destroy → re-acquire window was still open, i.e. 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 was not actually fixed. The two-state lease is what makes the original assertion true; test_acquire_refuses_a_container_a_peer_is_destroying and the store-level teardown tests pin it. I mention it because it was the stated justification for deleting the flock guard, and it did not hold until this revision.

I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[fancyboi999]

The lease direction matches #4206, but three runtime correctness gaps remain. The check/destroy race can still kill a peer-owned container, unreadable lease state currently fails open, and lease renewal adds synchronous filesystem I/O to async tool execution. I reproduced all three against this head; the focused 57-test suite does not cover these paths.

[fancyboi999]

[P1] Make the ownership check and container destruction one cross-process atomic operation. touch_lease() can replace the lease after this predicate returns but before _backend.destroy() runs. I forced exactly that interleaving and destroy() was still called while the lease owner was already worker-b, so the original cross-worker kill remains possible. A per-sandbox lock/CAS needs to cover check-through-destroy, and lease acquisition/renewal must participate in the same protocol.

[fancyboi999]

[P2] Preserve the distinction between a missing lease and an unreadable lease. Returning None here makes foreign_lease_blocks() return false, so the provider's intended fail-closed except OSError path is unreachable. With Path.read_text() raising PermissionError, _destroy_warm_entry() still called the backend destroy. Propagate the error or return a tri-state so reconcile/destroy skips when ownership cannot be proved; an existing corrupt lease should receive the same conservative treatment.

[fancyboi999]

[P2] Do not renew the file lease synchronously from get(). ensure_sandbox_initialized_async() calls provider.get() directly (sandbox/tools.py:1417 and :1434), and this path now performs mkdir + temp-file write + fsync + os.replace on the event loop for every sandbox tool lookup. The strict Blockbuster probe fails with Blocking call to os.mkdir. Move renewal to an already-offloaded path or provide an async/offloaded lease touch.

[WillemJiang]

@AnnaSuSu, please run 'make install' from your repo root to avoid the lint error when you commit the code.

[AnnaSuSu]

Thanks @fancyboi999 — all three were real, fixed in 6020cbf:

• P1 (check→destroy race): the ownership check and backend.destroy() now run under a per-sandbox flock guard (lease_ownership_guard, keyed by sandbox_id under the lease dir), and every lease write takes the same guard, so a peer's touch_lease can no longer interleave between the check and the stop. Added test_peer_touch_cannot_interleave_with_destroy (a peer touch blocks while a destroy holds the guard). Scope is same-host multi-worker (the 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 case); it is documented as explicitly not a multi-pod distributed lock, since flock over a shared network volume is unreliable.
• P2 (fail-open read): read_lease now returns None only for a genuinely-absent lease and raises (CorruptLeaseError/OSError) when the lease exists but is unreadable/corrupt, so the provider's fail-closed path is actually reached — a corrupt lease gets the same conservative treatment. Tests cover both corrupt content and PermissionError.
• P2 (sync lease IO in get()): removed lease renewal from get() (back to a pure in-memory lookup); active leases are renewed off the event loop from the idle checker (_renew_active_leases). Added a tests/blocking_io/ anchor that drives get() under the strict Blockbuster probe (it caught the exact os.mkdir block before the fix).

Appreciate the reproductions — they pointed straight at the gaps. Full suite (aio sandbox + provider + orphan reconciliation + the new anchor + gate smoke) is green and ruff is clean.

[AnnaSuSu]

@WillemJiang while you're here — a quick gut-check on the direction of this #4206 fix? I went with file-based per-sandbox ownership leases + a same-host flock guard around the check→destroy window (rather than Docker labels or a heavier coordinator). Is that the direction you'd want for multi-worker sandbox ownership, or would you prefer a different mechanism before I invest further?

[WillemJiang]

@WillemJiang while you're here — a quick gut-check on the direction of this #4206 fix? I went with file-based per-sandbox ownership leases + a same-host flock guard around the check→destroy window (rather than Docker labels or a heavier coordinator). Is that the direction you'd want for multi-worker sandbox ownership, or would you prefer a different mechanism before I invest further?

@AnnaSuSu Since we need to support multiple gateway instances for load balancing, it's better to share sandbox states via a third-party service; otherwise, we would have to maintain sandbox state across multiple instances.

[AnnaSuSu]

@WillemJiang Agreed, that makes sense — the file lease was scoped to the same-host case, and multi-instance gateways need the state shared. Concrete plan I'd like to confirm before reworking:

Follow the existing stream_bridge precedent (type: memory | redis) and make sandbox ownership a pluggable lease store:

• redis — the multi-instance implementation. Ownership is SET sandbox:lease:<id> <owner> NX EX <ttl>, renewed off the event loop by the existing idle-checker path, and the check→destroy window becomes an atomic claim instead of the same-host flock guard (which goes away entirely). State is visible across gateway instances, so this covers the load-balanced deployment, not just multi-worker on one host.
• memory — single-process deployments, where the 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 cross-worker kill can't occur.

No new deployment dependency: redis is already an optional harness extra (deerflow-harness[redis]), and the Docker image already installs it by default for the stream bridge — multi-worker Docker deployments assume Redis today.

If that direction works for you, I'll rework this PR accordingly.

[fancyboi999]

Thanks @AnnaSuSu. I found two remaining ownership failures that should be addressed before this is ready.

[P1] Keep leases alive when idle cleanup is disabled

• Location: backend/packages/harness/deerflow/community/aio_sandbox/sandbox_lease.py:77-87
• Problem: idle_timeout: 0 is the documented way to disable idle cleanup, but compute_lease_ttl() gives that lease a fixed one-hour lifetime. The provider only renews active leases from the idle-checker loop, and that loop is not started when idle_timeout <= 0. After one hour a live worker's active or warm sandbox therefore looks orphaned to another worker, which can adopt and later destroy it, recreating #4206 under a supported configuration.
• Evidence: the added helper returns max(3600.0, grace) for the disabled-cleanup case; AioSandboxProvider.__init__() starts _start_idle_checker() only when the timeout is positive, and _renew_active_leases() is called only by that checker. get() intentionally performs no renewal.
• Suggested fix: decouple lease renewal/liveness from idle cleanup. Keep a renewal loop running while leases exist even when reaping is disabled, and derive the lease TTL from a bounded renewal interval rather than from idle_timeout alone.
• Test: configure two providers with idle_timeout=0, keep worker A's sandbox active past the lease horizon, run worker B's reconcile/reap path, and assert B never adopts or destroys A's container.

[P1] Fail acquisition when ownership cannot be published

• Location: backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py:260-290
• Problem: both lease writes and lease-lock acquisition catch OSError, log, and return success to callers. _register_created_sandbox(), _register_discovered_sandbox(), reclaim, and reuse then return the sandbox id as usable even though no ownership record protects it. A read-only lease directory, full disk, or failed atomic replace silently disables the only cross-worker exclusion and lets a peer reconcile/destroy an actively used container.
• Evidence: _touch_sandbox_lease_locked() swallows touch_lease() failure and _touch_sandbox_lease() has no success result; callers continue directly to their normal success return. The read/destroy side fails closed, but ownership establishment currently fails open.
• Suggested fix: make initial claim/renewal return a result or raise. Registration and reclaim must not expose the sandbox unless the lease is durably published; clean up a just-created container on failure. A later renewal failure should remove the sandbox from service or otherwise prevent peer destruction until ownership is re-established.
• Test: force the lease lock and write_lease() to raise during create/discover/reclaim, and assert the operation fails without returning a usable sandbox id or leaving an unowned container active.

Reviewed base SHA: 693507870cae26dadf3af487f811eb2bcfe18f87
Reviewed head SHA: 6020cbf01735d289a7ab48bd00956e30b031463d

[AnnaSuSu]

@fancyboi999 Both confirmed against this head — and both are protocol-level rather than file-lock-level, which means they outlive the mechanism and land on the rework plan rather than on this diff.

[P1] Renewal dies with idle cleanup. Verified: _renew_active_leases() has exactly one caller (_cleanup_idle_resources, line 521), and __init__ starts the idle checker only when idle_timeout > 0 (line 177). So with cleanup disabled nothing ever refreshes compute_lease_ttl()'s 1h floor. config.example.yaml documents 0 as "keep warm VMs until shutdown", so this is a supported configuration and #4206 returns on it after an hour. My TTL helper made the disabled case look handled while the renewal path it depends on was never started — worse than not special-casing it.

[P1] Ownership publish fails open. Verified: _touch_sandbox_lease(), _touch_sandbox_lease_locked() and _clear_sandbox_lease() all swallow OSError into a logger.warning and return None, and every caller returns the id on the next line — _reuse_thread_sandbox (698→699), _reclaim_warm_pool_sandbox (741→744), _recheck_cached_sandbox (765→767), _register_created_sandbox (779→781). I made the read/destroy side fail closed in 6020cbf and left establishment fail open, which is the asymmetry you describe: a read-only lease dir or a failed os.replace silently disables the only cross-worker exclusion while the sandbox is handed out as usable.

Where the fix goes. @WillemJiang asked above to share sandbox state through a third-party service instead of maintaining it per gateway instance, and I proposed a pluggable lease store (memory | redis, following the stream_bridge precedent). Your two findings survive that swap verbatim, and the first one kills a line in the plan I posted:

• I wrote that renewal would happen "off the event loop by the existing idle-checker path". That is precisely the coupling your first P1 breaks: with idle_timeout: 0 the checker never starts, the redis key's EX lapses, and a peer's SET NX succeeds against a live sandbox. Same bug, different store. Taking your fix instead: renewal becomes its own loop that runs whenever leases exist, and the TTL derives from a bounded renewal interval, not from idle_timeout — liveness and reaping stop sharing a switch.
• SET … NX EX fails on connection loss the same way write_lease() fails on a read-only dir, and my plan never said which way that fails. Taking your fix: the claim returns a result rather than None; _register_created_sandbox / _register_discovered_sandbox / reclaim must not expose a sandbox whose ownership was not durably published, a just-created container is destroyed on failure, and a later renewal failure takes the sandbox out of service rather than leaving it unowned and adoptable.

Both of your suggested tests carry over unchanged (two providers at idle_timeout=0 past the lease horizon; forced claim failure during create/discover/reclaim) — they are store-agnostic, so I will write them against the store interface and run them on both backends.

@WillemJiang — amended plan, same shape as before plus the two above:

1. sandbox.ownership: memory | redis (default memory), resolved like stream_bridge. redis is already an optional harness extra and ships in the Docker image, so no new deployment dependency for the multi-instance case; memory covers single-process, where the 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 cross-worker kill cannot occur.
2. Ownership = SET sandbox:lease:<id> <owner> NX EX <ttl>; the check→destroy window becomes an atomic claim, so the same-host flock guard from this PR is deleted entirely rather than ported.
3. Renewal runs on its own bounded-interval loop, independent of idle_timeout; TTL derives from that interval.
4. Claim/renewal are fail-closed: no lease, no sandbox handed out.

If that works for you I will rework this PR onto it. Happy to close this one and open a fresh PR instead if you would rather review the store as its own change — the diff will not have much in common with what is here now.

[WillemJiang]

@AnnaSuSu +1 to follow the steam_bridge style implementation, we can have memory and Redis implementations to share this status information.

[AnnaSuSu]

@WillemJiang Reworked onto the stream_bridge-style store as agreed — sandbox.ownership.type: memory | redis, factory + lazy import + the existing optional redis extra + detect_uv_extras auto-detection. The file lease and its flock guard are deleted, not ported. Body updated in full; the short version:

One deliberate divergence from stream_bridge, flagging it rather than hiding it: the store interface is sync. StreamBridge is async because it is driven from the event loop; ownership is the opposite — its callers are AioSandboxProvider.__init__, the background idle/renewal threads, and release() from the sync after_agent hook, and an async ABC would make all of them marshal onto a loop for nothing. So I copied the pluggability shape (type: discriminator, config model, lazy factory, optional extra, injectable client, supports_cross_process, redis not re-exported) rather than the async-ness. The one path that is on the event loop (get()) never touches the store, and the async acquire paths offload registration through asyncio.to_thread.

Multi-instance deployments need no new config. memory is the default and, like the bridge's memory backend, is single-instance only (it declares supports_cross_process = False and the provider warns at startup). The resolver reads app_config.stream_bridge and DEER_FLOW_STREAM_BRIDGE_REDIS_URL, in the same order the bridge's own resolver uses — so any deployment already pointing the bridge at Redis (i.e. every load-balanced one) gets a redis ownership store automatically.

@fancyboi999 both of your P1s are fixed, and they survived the mechanism swap exactly as you'd expect:

• Renewal is now its own thread with a TTL derived from its own interval, never from idle_timeout. Reproduced end-to-end first: at idle_timeout: 0 the old wiring left the lease at owner = None after the TTL and the peer destroyed the live container.
• Establishment is fail-closed: a sandbox whose ownership can't be published is never handed out, and a just-created container is destroyed rather than leaked as an adoptable orphan. Reaping fails closed symmetrically.

Two of my own mistakes worth surfacing, since one of them was load-bearing:

1. My first draft used a conditional claim on the acquire path. Wrong — a lease answers who reaps this container, not who may use it, so worker A holding it would have broken thread T's next turn on worker B. Split into take() (acquire) and claim() (adopt/reap).
2. My second draft asserted "a held claim is the exclusion, so the flock guard is unnecessary" — and that was false, because take() ignored the claim. B claims X for destroy → starts the multi-second container stop → a turn for X's thread routes to A → A takes the lease and hands X to an agent → B's stop lands → 502 mid-turn. 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206, unfixed, with the flock removed on a justification that didn't hold. Leases now carry a state (own: / del:) and a takeover is refused against a teardown; that's what makes the original claim true and the flock genuinely redundant. I reproduced the race against a real Redis with a delayed container stop, both with and without the guard.

Also fixed in the same pass: renew() now distinguishes a lapsed lease from one a peer took (collapsing them meant a Redis restart without persistence evicted every in-flight sandbox on every instance at once); _drop_unhealthy_sandbox was the one reap path with no ownership gate; destroy() untracked before claiming, so a refused claim leaked the container.

One coverage gap I want to flag rather than paper over: the redis tier is @pytest.mark.integration + self-skipping per this repo's documented policy (backend/pyproject.toml: "integration: tests that require an external service (e.g. Redis); skipped when unavailable"), so CI does not execute the Lua scripts — same status as the existing stream-bridge integration tier. I ran that tier locally against a real Redis 7.2.3 (all green) and every store-contract test runs against both backends, so the semantics the provider depends on can't drift. I deliberately did not add fakeredis[lua]: its Lua is a reimplementation of Redis semantics, so it would give false confidence on exactly the atomicity this depends on, and it drags a C extension into the dev group for everyone. If you want this in CI the honest fix is a redis service container in backend-unit-tests.yml — happy to do it, but it would also activate the currently-skipped stream-bridge integration tests, so it felt like it belonged outside a bug-fix PR rather than bundled in silently. Your call.

Out of scope, pre-existing: SandboxMiddleware._acquire_sandbox_async calls get_sandbox_provider() directly on the event loop, so the first acquire constructs the provider and runs _reconcile_orphans() there; _discover_or_create_with_lock_async also resolves get_paths() (→ os.getcwd) on the loop. Both predate this PR — noting them rather than widening the diff.

[fancyboi999]

Thanks @AnnaSuSu. I found one remaining ownership race that should be addressed before this is ready.

[P1] Do not treat a lost Redis key as proof that a running container is orphaned

• Location: backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py:367-409
• Problem: after Redis loses its lease keys, a newly starting gateway instance immediately claims every running container during startup reconciliation. The live instance that is still using a container then sees RenewOutcome.LOST, drops its handle, and the new instance can later idle-destroy that container. This recreates #4206 during the Redis-reset case the new LAPSED handling claims to make safe.
• Evidence: __init__ runs _reconcile_orphans() before starting its renewal thread; reconciliation calls claim() for every running container, and claim() succeeds whenever the Redis key is absent. Existing owners only republish absent leases on their next renewal tick. A fresh instance started in that window wins the key first, so the old owner's later renewal reports LOST. The added lost-state test exercises the original owner renewing first, not a peer reconciling first.
• Suggested fix: add a recovery grace before an instance may adopt a keyless running container, long enough for surviving owners to republish, or persist enough owner/liveness metadata to distinguish a crashed owner from backend state loss. Do not immediately claim an absent key from startup reconciliation.
• Test: with provider A actively tracking a container, delete the Redis ownership key, start/reconcile provider B before A's renewal tick, and assert B does not adopt or reap the container; after A renews, it must remain the owner.

Reviewed base SHA: 8da7cbf0285db4781054bddac765947626c07748
Reviewed head SHA: a91cd789b60c6932045307f6552630c6f0f13a4c

[AnnaSuSu]

Thanks @fancyboi999 — real, and fixed in 94a12a8. Reproduced against a real Redis before touching anything:

1. A publishes ownership       -> redis owner = 'worker-a'
2. Redis FLUSHALL              -> redis owner = None
3. B reconciles first          -> B warm_pool = ['live01'], redis owner = 'worker-b'
   FAIL: B adopted a live peer's container

The shape of it. An absent lease meant two opposite things on two paths this PR introduced together: _refresh_ownership reads it as LAPSED and re-establishes it (nobody took it, so it is still ours), while _reconcile_orphans read the same absent key as "orphan" and adopted on sight. So the LAPSED handling only ever covered an owner renewing its own lease — on its own it never made state loss safe. That makes what I wrote into backend/AGENTS.md (a Redis restart "evicted every live sandbox on every instance at once", past tense, implying it no longer can) an overclaim. Corrected in the same commit rather than left standing.

Two corrections to the framing, both of which make it worse rather than better:

1. The __init__ ordering is not the root cause. B's internal order — reconcile before starting its renewal thread — does not affect A's renewal timing. The window is [state loss, A's next renewal tick], bounded by renewal_interval_seconds, and it is open regardless of what B's __init__ does in what order.
2. It is not startup-specific. _cleanup_idle_resources → _reconcile_orphans (line 621) runs periodically from the idle checker, so an already-running instance hits the identical race. A startup-only guard would have left that path open.

Also worth noting for the trigger surface: this does not need a Redis restart. A shared Redis with maxmemory + allkeys-lru can evict ownership keys under memory pressure at any time.

The fix. _adoptable_after_grace: an untracked container must be seen unowned across a full lease TTL before it can be adopted. That rebuilds the delay the state loss erased — a live owner republishes within one renewal interval, shorter than the TTL by construction (ttl_multiplier >= 2), while a crashed owner never does, so its containers are still adopted one grace later rather than leaking. The grace lives in _reconcile_orphans, so both call sites (startup and idle checker) are covered.

Three design points worth flagging:

• The peek is a pre-filter, not a gate. owner() only decides whether to consider adoption; the atomic claim() still decides. A stale "owned" merely delays adoption, and a stale "unowned" still has to survive the grace and the claim — so owner()'s documented "inspect, don't gate on this" contract stays intact.
• A republished lease resets the grace rather than pausing it. Otherwise an owner that republishes late still loses its container: the adopter's timer, started before the republish, would expire anyway. Pinned separately by test_adoption_grace_restarts_when_a_live_owner_republishes.
• 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, since peers are invisible to each other's leases with or without it.

Cost: a genuinely crashed owner's containers are adopted one TTL (120s by default) later than before. They are idle containers; that buys not killing live ones.

Your suggested test carries over as written. test_peer_reconcile_after_state_loss_does_not_steal_a_live_container is exactly your sequence — A actively tracking, ownership key deleted, B reconciles before A's renewal tick, assert B neither adopts nor reaps, then A renews and must remain the owner. Alongside it: test_adoption_grace_expires_so_a_truly_orphaned_container_is_still_adopted, so the grace cannot silently degrade into "never adopt", and the republish-resets case above. test_expired_lease_lets_peer_adopt_crashed_owner_container was updated to traverse the grace rather than deleted.

I checked each of them goes red in both directions — grace always-permissive (4 red) and grace never-expiring (2 red) — so neither half of the predicate is unguarded. Focused suite green (87 passed, test_sandbox_orphan_reconciliation + test_sandbox_ownership_store + the blocking_io get() anchor), ruff clean, and the real-Redis script above passes all seven steps after the fix, including the crash path still adopting once nobody republishes.

Unchanged from my last comment: the redis tier is still @pytest.mark.integration + self-skipping, so the new regression tests use the same two-providers-sharing-one-store harness as every other #4206 provider test here, and the real-Redis run stays a local verification. owner()'s "absent ⇒ None" semantics — the only thing the grace depends on — are already pinned against both backends by the store-contract tier.

[fancyboi999]

Thanks @AnnaSuSu. I found one remaining ownership race that should be addressed before this is ready.

[P1] Keep the teardown exclusion alive until the container stop completes

• Location: backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py:1104-1122 and :1433-1454
• Problem: claim(..., for_destroy=True) writes a del: marker with the ordinary lease TTL, but nothing refreshes that marker while _backend.destroy() is running. If the stop lasts past the TTL, a peer's take() succeeds and hands the still-running container to a new turn; the original destroy then completes underneath that turn, recreating the mid-turn 502 that this protocol is meant to prevent.
• Evidence: both destroy paths claim once and call the blocking backend stop before releasing. RedisOwnershipStore._CLAIM_SCRIPT applies the normal PX TTL to del:, while _RENEW_SCRIPT renews only own: and deliberately reports a teardown as LOST. The provider renewal loop also stops tracking an id once it sees that outcome, so it cannot extend the destroy exclusion. This is reachable without an abnormal backend: renewal_interval_seconds accepts any value greater than zero and ttl_multiplier only has a minimum of 2, so a valid configuration can make the teardown TTL shorter than a normal container stop; LocalContainerBackend._stop_container() itself has no subprocess timeout.
• Suggested fix: keep a destroy-specific lease heartbeat alive until the backend stop returns, while retaining a finite TTL so a destroyer crash eventually releases the container. A separate teardown TTL is only sufficient if it is bounded above every backend's real stop deadline; refreshing the del: state for the duration is the safer invariant.
• Test: block provider A's fake backend destroy longer than the configured lease TTL, assert provider B's take() remains refused for the full in-flight stop, then unblock A and assert the marker is released so B can acquire or recreate cleanly.

Reviewed base SHA: 693507870cae26dadf3af487f811eb2bcfe18f87
Reviewed head SHA: 94a12a817c1e8bfc9867f83051ac9ada64f88cb9

[AnnaSuSu]

Thanks @fancyboi999 — real, and fixed in 90936b4. Reproduced against a real Redis first, with a legal config (renewal_interval_seconds: 0.5, ttl_multiplier: 2.0 → 1s TTL) and a backend stop blocking 2.5s:

t=0.0s  A claimed teardown, stop in flight -> redis = b'del:worker-a'
t=0.3s  B's take() refused (teardown marker holds)
t=0.8s  B's take() refused (teardown marker holds)
t=1.0s  B's take() SUCCEEDED *mid-stop* -> redis = b'own:worker-b'

The marker evaporates exactly at the TTL, mid-stop, and B walks off with a container that is still being stopped.

What I got wrong, and it was load-bearing. When I deleted the per-sandbox flock in the rework I justified it as: "Leases now carry a state (own: / del:) and a takeover is refused against a teardown; that's what makes the original claim true and the flock genuinely redundant." That is false for a stop that outlives the TTL. A held lock cannot expire; a lease can. The two states were necessary but not sufficient — I swapped a mechanism that held for the duration of the work for one that holds for a fixed time, and carried the "window closed" conclusion across the swap without re-deriving it. The exclusion has to be held deliberately, not assumed to outlast the work it guards.

Your evidence chain checks out end to end: _RENEW_SCRIPT matches only own: and returns 0 for a teardown; MemoryOwnershipStore.renew likewise returns LOST on lease.destroying; and both destroy paths claim once, call the blocking stop, and only then release — while destroy() additionally _remove_tracked_sandboxes first, so the renewal loop cannot even see the id.

One addition to the trigger surface, which I think is worse than the config case you cited. You're right that the schema permits a TTL below a normal container stop (renewal_interval_seconds is only gt=0, ttl_multiplier only ge=2). But the default config is reachable too: LocalContainerBackend._stop_container passes no timeout= to subprocess.run, so a wedged daemon blocks unbounded and the marker lapses after 120s with nothing exotic configured. That is what pushed me off the two alternatives I considered — a timeout= on _stop_container fixes neither the legal-config case nor any other backend, and raising a separate teardown TTL is only sufficient if it is bounded above every backend's real stop deadline, which is exactly the fragility you called out. Refreshing for the duration is the invariant that does not depend on knowing that bound.

The fix. _held_teardown_lease wraps both _backend.destroy() call sites and re-claims the marker every renewal_interval_seconds until the stop returns. No store change was needed — claim(..., for_destroy=True) already refreshes an existing del: marker on both backends (_CLAIM_SCRIPT's current == mine_del branch; MemoryOwnershipStore._write_locked). The TTL stays finite, per your constraint: the heartbeat dies with the process, so a destroyer that crashes mid-stop still releases the container one TTL later rather than marking it undestroyable forever.

If the heartbeat's own claim ever returns false, the exclusion is already gone (store lost the key and a peer took it) and the stop cannot be recalled, so it logs an error rather than pretending to recover. That residue — a store flush landing inside a container stop — I'm leaving as a logged condition rather than adding more machinery for.

Same run after the fix:

t=0.0s  A claimed teardown, stop in flight -> redis = b'del:worker-a'
t=0.3s ... t=2.3s  B's take() refused (teardown marker holds)
t=2.6s  A's stop returned. redis = None
B stayed refused for the whole in-flight stop, and the marker was released once it returned.

Your test carries over as written — test_teardown_marker_is_held_for_a_stop_that_outlives_the_lease_ttl is your sequence exactly: block A's fake backend destroy past the TTL, assert B's take() stays refused for the full in-flight stop, then unblock and assert the marker is released so B can acquire cleanly. Plus test_teardown_heartbeat_stops_when_the_stop_returns, so the fix cannot overshoot into an exclusion that never lets go. Note that test_acquire_refuses_a_container_a_peer_is_destroying — the test I wrote for this window in the rework — passes throughout: it proves the marker refuses a takeover but never lets it expire, which is precisely the blind spot.

Two process notes, since both changed the result:

• My first repro script scored a take() that happened after the stop returned as a hit — which is legal, the container is gone by then and B just cold-starts. After fixing the verdict I re-ran the pre-fix half as well, so both sides are judged by the same rule; without that the "fixed" claim would have rested on a script whose verdict was wrong.
• My first draft of the heartbeat-stops test used threading.active_count() <= before to detect a leaked thread. The red check walked straight through it — that counter is global and other tests' idle-checker/renewal threads make it noise. Now asserted by thread name, which does catch it.

Focused suite green (89 passed: reconciliation + ownership store + the blocking_io get() anchor), full backend suite 7592 passed, ruff clean. backend/AGENTS.md records the invariant and drops the "flock is redundant" claim I can no longer support as stated.

[fancyboi999]

Thanks @AnnaSuSu. The new teardown heartbeat fixes the warm-pool and explicit destroy paths, but one remaining stop path can still reopen the same cross-worker race.

[P1] Hold the teardown lease through unhealthy-container stops

• Location: backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py:1131-1136
• Problem: _drop_unhealthy_sandbox() writes the del: marker once and then calls _backend.destroy() without _held_teardown_lease(). The sandbox has already been removed from the maps that the normal renewal loop scans, so if this stop outlives the lease TTL, a peer's take() can succeed and hand the still-running container to a new turn; the old stop then completes underneath that turn and recreates the mid-turn 502 from #4206.
• Evidence: this head wraps _backend.destroy() with _held_teardown_lease() in _destroy_warm_entry() and destroy(), but the third ownership-gated stop above still calls the backend directly. LocalContainerBackend._stop_container() has no subprocess timeout, and valid renewal_interval_seconds / ttl_multiplier values can make the TTL shorter than that stop. The added unhealthy-sandbox test proves only that a peer-owned claim is refused; it never lets this path's own teardown marker expire during the stop.
• Suggested fix: run the unhealthy-container backend stop inside _held_teardown_lease() as well, with ownership release handled for both success and failure consistently with the other destroy paths.
• Test: block _drop_unhealthy_sandbox()'s fake backend stop beyond the configured TTL, assert a peer's take() remains refused for the entire stop, then unblock it and assert the marker is released.

Reviewed base SHA: 693507870cae26dadf3af487f811eb2bcfe18f87
Reviewed head SHA: 90936b49aa24668e0de7bd59c40bb47e5bbb2e15

[AnnaSuSu]

Self-audit follow-up, pushed as 0d2377b. Nobody asked for this one — I went looking after your last review, and the first thing I found was that my own fix in 90936b4 was incomplete.

The fix I got wrong

90936b4 says _held_teardown_lease wraps "both" _backend.destroy() call sites. There are three:

line	path	writes del:	was held
1170	_destroy_warm_entry	✅	✅
1512	destroy()	✅	✅
1136	_drop_unhealthy_sandbox	✅	❌

Reproduced against a real redis, same harness as last time — legal config (renewal_interval_seconds: 0.5, ttl_multiplier: 2.0 → 1s TTL), backend stop blocked 2.5s:

t=0.0s  A claimed teardown, stop in flight -> redis = b'del:worker-a'
t=1.0s  B's take() SUCCEEDED *mid-stop* -> redis = b'own:worker-b'

Same window, third path — and this one is worse: _drop_unhealthy_sandbox calls _remove_tracked_sandbox before claiming, so _renew_owned_leases cannot see the id either. Nothing refreshed the marker at all. Its sibling test test_unhealthy_sandbox_owned_by_peer_is_not_destroyed pins the gate and never lets the marker expire — the exact blind spot I diagnosed for test_acquire_refuses_a_container_a_peer_is_destroying one comment ago, sitting in the path right next to it.

Why I went looking

Four rounds, four findings, and they were all the same shape — not "the newest code is weakest", which is what I'd have guessed, but a property asserted in prose with no test that could falsify it:

round	the prose	what made it false
1	# Fail closed for destroy paths	read_lease swallowed OSError, so that except was dead code
2	"long TTL so peers still observe ownership between renewals"	at idle_timeout: 0 there were no renewals
3	"LAPSED makes a Redis restart safe"	reconcile read the same absent key as "orphan, adopt"
4	"the del: state makes the destroy window safe / the flock is redundant"	the marker expires

Round 1's is the purest: the words "Fail closed" were sitting directly above the handler that could never fire. The documentation being good is what makes this dangerous — a confident comment on a special case reads as "considered and handled", which stops the next reader, and the author, from checking. You had it in round 2 already: "My TTL helper made the disabled case look handled while the renewal path it depends on was never started — worse than not special-casing it."

So I stopped asking "is this claim true" (I'm the worst-placed reader of prose I wrote — I found it convincing once already) and asked the mechanical question instead: which test goes red if this is false? That one I can answer by grepping, and it catches all four.

What that found

Every item below was verified by mutating the code and watching the suite stay green — not by reading.

Two tests that could not fail:

• test_reconcile_fails_closed_when_ownership_unknown reached the grace gate, never the claim. A bare MagicMock answers owner() with a truthy mock, so the container read as peer-owned and deferred; claim.call_count == 0. It stayed green with _claim_ownership failing open. I did this in 94a12a8 — adding _adoptable_after_grace ahead of the claim disarmed the test behind it. I red-checked my new tests and never checked whether the change had hollowed out existing ones.
• test_adoption_grace_restarts_when_a_live_owner_republishes — my own test, named for exactly this hazard, with the hazard spelled out in its docstring — never distinguished reset from pause. Those diverge only on a second lapse, which it never drove. It passed with the reset deleted. The real scenario: flush → B starts its grace → A republishes → B defers → a second blip drops A's key → B's stale timer is already spent → B adopts A's live container with no grace at all.

Claims with no test at all, now pinned (mutation → red, per test):

• destroy(), _evict_oldest_warm, _reclaim_warm_pool_sandbox, _register_created_sandbox, and shutdown()'s warm loop were each the one untested sibling of an "every path does X" enumeration. shutdown() had never been driven with a non-empty warm pool, so a loop bypassing the claim — stopping a live peer's container on our exit — went unnoticed. _reclaim_warm_pool_sandbox had no direct test on this provider at all.
• Renewal's unknown-is-not-lost rule — the single deliberate exception to fail-closed. Inverting it drops every active and warm sandbox on every instance the moment the store blinks: the same fleet-wide eviction the LAPSED/LOST split exists to prevent, pinned only for the flushed-store path and never for a raising one.
• Both hops of the stream-bridge redis inference. Deleting either left the whole suite green while every config.yaml-native multi-instance deployment silently fell back to memory — 多 Worker 下 AioSandboxProvider 的 orphan reconcile 会 Adopt 并 idle 销毁其他 Worker 的沙箱，导致工具调用 502 / Connection refused #4206 reopened on exactly the deployments that inference exists for, which is the "no extra config" promise in the PR body. The one test that drove __init__ monkeypatched _load_config and omitted the key, so it shadowed the gap.

Three claims narrowed instead, because they promised more than the code delivers:

• "run against both backends ... cannot drift" — CI provisions no redis, so the merge gate runs the memory tier only and the Lua never executes there. My own CI-gap disclosure two paragraphs later already said this; the test-plan sentence contradicted it.
• "Every destroy path claims before untracking" — _drop_unhealthy_sandbox untracks first, deliberately, under its expected_info TOCTOU guard. Narrowed rather than reordered: that guard is load-bearing and pinned by test_drop_unhealthy_sandbox_skips_recreated_entry.
• "Atomic: concurrent claims from different instances cannot both succeed" — true via Lua on redis, vacuous on the single-instance memory store, and pinned by neither: the contract suite drives sequential calls. I deliberately did not add a concurrency test here. Redis's atomicity is Redis's property, not ours; the memory store is single-process, so "different instances" cannot arise. A barrier test against the memory tier would make the claim look covered while the mechanism that carries it still never runs in CI — a test whose name matches the claim without constructing the falsifying scenario, which is the thing this whole exercise is about. Saying what is and isn't verified is the honest fix.

One left, and I don't think it belongs here

_create_sandbox / _create_sandbox_async destroy on a readiness timeout with no claim and no del: marker — the container is running and unowned for up to 60s, so a peer can discover and take it before our stop lands. That contradicts "claim() gates every adopt/reap path", so the sentence is mine to answer for.

But it isn't a regression — 259f51ca gated nothing — and the fix has design questions worth their own review (claim before the readiness wait? roll back readiness failure and ownership failure through one path?). Happy to file it as an issue rather than grow a four-round PR further, unless you'd rather it land here.

Verification: 133 passed focused (reconciliation + ownership store + provider + warm pool + the blocking-io get() anchor), 7601 passed full backend suite, ruff clean, and the real-redis repro flipped from exit 1 to exit 0 on the _drop_unhealthy_sandbox path. Every new test red-checked individually against its own mutation.

[AnnaSuSu]

@fancyboi999 We crossed — your review landed against 90936b49 at 17:28, my self-audit comment four minutes later at 17:32. You found the third stop path independently; 0d2377b2 had already fixed it. That's a good sign rather than a wasted round: I went looking for exactly this class of thing and we converged on the same defect from opposite directions.

Your three asks against the current head (e3146075):

ask	state
run the unhealthy stop inside _held_teardown_lease()	✅ 0d2377b2
release handled for success and failure, consistently with the other destroy paths	✅ for this path in 0d2377b2 — but the word "consistently" was load-bearing, see below
block the stop past the TTL, assert take() stays refused throughout, then unblock and assert release	✅ test_unhealthy_drop_holds_the_teardown_marker_for_its_stop

Your diagnosis matches mine line for line, including the part that makes this path worse than the other two: _remove_tracked_sandbox runs before the claim, so _renew_owned_leases cannot see the id either — nothing refreshed the marker at all. Real-redis repro on the old head, legal config (renewal_interval_seconds: 0.5, ttl_multiplier: 2.0 → 1s TTL), stop blocked 2.5s:

t=0.0s  A claimed teardown, stop in flight -> redis = b'del:worker-a'
t=1.0s  B's take() SUCCEEDED *mid-stop* -> redis = b'own:worker-b'

And you're right that test_unhealthy_sandbox_owned_by_peer_is_not_destroyed only pins the gate and never lets the marker expire — the same blind spot I'd diagnosed for test_acquire_refuses_a_container_a_peer_is_destroying one comment earlier, sitting in the path next to it.

Your second ask found one more, in e3146075

Taking "consistently with the other destroy paths" literally sent me to compare all three, and destroy() was the odd one out:

path	releases on a failed stop
_destroy_warm_entry	✅ — explicitly, with a comment saying why
_drop_unhealthy_sandbox	✅ (0d2377b2)
destroy()	❌ — a raising backend propagated straight past _release_ownership

_destroy_warm_entry already spells out the reason: the stop failed, so the container is probably still up, and a marker left behind blocks its own thread from re-acquiring it. destroy() left the id stranded under del: for a full TTL. Fails safe rather than fatal — a stuck marker stops peers from touching the container, it is not the cross-instance kill — but the three paths must agree, and they didn't.

Fixed in e3146075: release, then re-raise. Swallowing would have been the easier symmetry with _destroy_warm_entry's return False, but destroy() has no failure return and shutdown() logs per sandbox off the exception, so swallowing would have silently narrowed what callers can see. Red-checked both directions — no release → red; release but swallow → red.

Still open, and I'd rather not grow this PR for it

_create_sandbox / _create_sandbox_async destroy on a readiness timeout with no claim and no del: marker. The container is running and unowned for up to 60s, so a peer can discover and take it before our stop lands. That contradicts "claim() gates every adopt/reap path", which is my sentence to answer for.

It isn't a regression — 693507870 gated nothing at all — and the fix has design questions worth their own review (claim before the readiness wait, or after? does a readiness failure roll back through the same path as an ownership failure?). I'd rather file it than bolt it onto a five-round PR, but say the word if you'd prefer it here.

134 passed focused, 7602 passed full backend suite, ruff clean.

[AnnaSuSu]

Correction to my last comment, @fancyboi999 — the readiness-timeout item I raised is real but narrower than I stated, and I stated it without checking.

What I wrote:

_create_sandbox / _create_sandbox_async destroy on a readiness timeout with no claim and no del: marker. The container is running and unowned for up to 60s, so a peer can discover and take it before our stop lands.

The second half is false on the local backend. LocalContainerBackend.discover health-checks before it returns anything:

sandbox_url = f"http://{sandbox_host}:{port}"
if not wait_for_sandbox_ready(sandbox_url, timeout=5):
    return None

A container that has not become ready is exactly the container that fails that check, so a peer cannot discover it, and the deterministic-id path never reaches take(). Same-host workers are excluded a second time over by _discover_or_create_with_lock's file lock, which they do share; cross-host local deployments do not share a docker daemon at all, so there is no shared container to race for. The anonymous path (no thread_id) uses a random id, so no peer computes it.

RemoteSandboxBackend is the one where it holds. _provisioner_discover does not health-check — it returns the sandbox as soon as the Pod exists:

if resp.status_code == 404:
    return None
resp.raise_for_status()
data = resp.json()
return SandboxInfo(sandbox_id=sandbox_id, sandbox_url=data["sandbox_url"])

So in provisioner/K8s mode: A creates the Pod and starts its 60s readiness wait; a turn for the same thread routes to B on another host, where the file lock excludes nothing; B's discover returns the not-yet-ready Pod; take() succeeds because no marker exists; A's readiness times out and its unclaimed _backend.destroy(info) stops the Pod under B's turn.

So: real, provisioner-mode only, and the sentence "claim() gates every adopt/reap path" is still mine to answer for. I'll file it with that scope rather than the one I gave you.

Worth saying plainly, because it is the same defect I spent that comment describing: I took that finding from a source I did not verify, and repeated it as fact in a comment about not doing exactly that. Reading discover on both backends is what the claim needed, and it is what I skipped. The two paths that are covered — a live owner's container, and a container whose stop is in flight — I did drive against a real redis. This one I did not drive at all.

[AnnaSuSu]

Filed the readiness-timeout window as #4248 with the corrected scope (provisioner mode only; local is excluded three ways over, all recorded there).

I'll take it myself once this lands — it predates this PR (693507870 gated no reap path at all, so it isn't a regression), and the fix has design questions that deserve their own review rather than a sixth round here: claim before or after the readiness wait; whether a readiness failure and an ownership-publish failure should roll back through one path; and whether _provisioner_discover should health-check like LocalContainerBackend.discover does, which would close it from the other side but changes discovery semantics for every caller.

Say the word if you'd rather it land in this PR instead.