[GH-ISSUE #165] Auto-eviction can delete a sandbox after a keep-alive was accepted (expiry never re-validated) #56

New issue

Closed

opened 2026-10-29 14:18:56 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:18:56 +00:00

Owner

Originally created by @sunkencity999 on GitHub (Aug 12, 2026).
Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/165

First — thank you for the last few days. You corrected me twice with citations to your spec and your test suite, and both times you were right and I wasn't. That's a better outcome than if you'd just merged my patch, and it's not the response most projects give a drive-by contributor. It also changed how I read this codebase, which is why I went back through it properly before filing anything else.

While doing that I found something I'm fairly confident is a genuine bug, so I wanted to bring it to you on its own rather than bundled with anything else.

Summary

A POST /sandboxes/{id}/timeout (or /refreshes, or /connect) can be acknowledged successfully and the sandbox then destroyed anyway by the auto-eviction pass that is already in flight. With timeout_action = Delete the sandbox is gone after the control plane told the caller its lifetime was extended.

The race

evict_expired_sandboxes (src/orchestrator/service.rs:1795) takes a point-in-time snapshot and then acts on each entry serially:

let expired = self.store.list_expired(SystemTime::now()).await?;   // :1801

for metadata in expired {
    if metadata.state != SandboxState::Running {                   // :1805 — from the stale copy
        continue;
    }
    match metadata.timeout_action {
        SandboxTimeoutAction::Pause  => self.pause_sandbox_inner(metadata.id).await,
        SandboxTimeoutAction::Delete => self.delete_sandbox_inner(metadata.id).await,
    }

The only re-check before acting is state, read from the snapshot. Expiry is never re-validated, and the CAS inside the inner functions can't help: update_if_state(&sandbox_id, &[SandboxState::Running], …) (store/in_memory.rs:172) matches on state only — its signature is expected_states: &[SandboxState], so it has no way to express "…and still expired."

Meanwhile keep_alive_for (service.rs:735) validates state, then updates only the expiry through that same state-only CAS (service.rs:779) and reports success. It never changes state, so a subsequent Running → Killing CAS in delete_sandbox_inner succeeds normally.

Sequence

Sandboxes A and B both expire in the same tick, both timeout_action = Delete.
list_expired returns [A, B].
Eviction begins deleting A. This takes real time — VM stop, ublk device release, slot release.
During that window a client calls POST /sandboxes/B/timeout. keep_alive_for CASes on Running, succeeds, sets expires_at = now + 1h, returns success. The client now believes B is safe.
The loop reaches B, consults its stale metadata, sees Running, and deletes it.

The window is as long as it takes to process earlier entries, so it widens with the number of simultaneously-expiring sandboxes. With timeout_action = Pause the outcome is milder — the sandbox survives but is paused against an explicit extension.

Why this reads as an omission rather than a decision

SandboxMetadata::is_expired exists at src/orchestrator/store/metadata.rs:121 and has its own unit test at :141-142. Its only other caller in the whole tree is a benchmark (crates/benchmarks/benches/orchestrator_store_benchmark.rs:66). It is called from no production code path — I checked with grep -rn is_expired across the repo.

That looks like the predicate was written for exactly this purpose and never wired into the eviction path.

Existing coverage

The eviction tests are good, they just don't race this. auto_evict_expired_sandbox (src/orchestrator/tests.rs:3608) and auto_evict_sandboxes_skips_non_running_and_non_expired_and_continues_on_error (:2762) both drive eviction with no concurrent keep-alive. keep_alive_uses_latest_metadata_when_deciding_whether_to_shorten (:2680) does race keep-alive against a concurrent store write, but not against an eviction pass — so the interaction falls between two well-tested behaviours.

Possible fix shapes

You'll know better than me which fits the architecture, but the options I can see:

Re-validate expiry inside the CAS. Extend update_if_state (or add a variant) to take a predicate over the whole SandboxMetadata rather than just state, and have the inner eviction functions require "still expired." This is where is_expired would naturally be used.
Pass the observed expires_at down from the loop and have the eviction path abort if it no longer matches — a compare-and-swap on expiry rather than state.
Re-read metadata at the top of each iteration instead of trusting the snapshot. Simplest, and closes most of the window, though not atomically.

Option 1 seems most robust and also fixes the Pause variant; option 3 is a much smaller change if you'd rather not touch the store trait.

Caveats

I have not reproduced this at runtime — I can't run the suite locally (no KVM on this machine), so this is from reading the code rather than from a failing test. If there's a serialisation guarantee I've missed that prevents the interleaving, I'd genuinely like to know, both because it means there's no bug and because it means I'm still misreading something about the lifecycle model.

Happy to write a regression test for it if that's useful — the mock backend in tests.rs looks like it could drive the interleaving deterministically, and I'd rather contribute a failing test than a fix, given I've been wrong about your intent twice already.

Thanks again for the care you've been putting into these threads. Genuinely nice to work with a project where the maintainers check things.

Originally created by @sunkencity999 on GitHub (Aug 12, 2026). Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/165 First — thank you for the last few days. You corrected me twice with citations to your spec and your test suite, and both times you were right and I wasn't. That's a better outcome than if you'd just merged my patch, and it's not the response most projects give a drive-by contributor. It also changed how I read this codebase, which is why I went back through it properly before filing anything else. While doing that I found something I'm fairly confident is a genuine bug, so I wanted to bring it to you on its own rather than bundled with anything else. ## Summary A `POST /sandboxes/{id}/timeout` (or `/refreshes`, or `/connect`) can be acknowledged successfully and the sandbox then destroyed anyway by the auto-eviction pass that is already in flight. With `timeout_action = Delete` the sandbox is gone after the control plane told the caller its lifetime was extended. ## The race `evict_expired_sandboxes` (`src/orchestrator/service.rs:1795`) takes a point-in-time snapshot and then acts on each entry serially: ```rust let expired = self.store.list_expired(SystemTime::now()).await?; // :1801 for metadata in expired { if metadata.state != SandboxState::Running { // :1805 — from the stale copy continue; } match metadata.timeout_action { SandboxTimeoutAction::Pause => self.pause_sandbox_inner(metadata.id).await, SandboxTimeoutAction::Delete => self.delete_sandbox_inner(metadata.id).await, } ``` The only re-check before acting is `state`, read from the snapshot. Expiry is never re-validated, and the CAS inside the inner functions can't help: `update_if_state(&sandbox_id, &[SandboxState::Running], …)` (`store/in_memory.rs:172`) matches on **state only** — its signature is `expected_states: &[SandboxState]`, so it has no way to express "…and still expired." Meanwhile `keep_alive_for` (`service.rs:735`) validates state, then updates only the expiry through that same state-only CAS (`service.rs:779`) and reports success. It never changes `state`, so a subsequent `Running → Killing` CAS in `delete_sandbox_inner` succeeds normally. ### Sequence 1. Sandboxes A and B both expire in the same tick, both `timeout_action = Delete`. 2. `list_expired` returns `[A, B]`. 3. Eviction begins deleting A. This takes real time — VM stop, ublk device release, slot release. 4. During that window a client calls `POST /sandboxes/B/timeout`. `keep_alive_for` CASes on `Running`, succeeds, sets `expires_at = now + 1h`, returns success. The client now believes B is safe. 5. The loop reaches B, consults its stale `metadata`, sees `Running`, and deletes it. The window is as long as it takes to process earlier entries, so it widens with the number of simultaneously-expiring sandboxes. With `timeout_action = Pause` the outcome is milder — the sandbox survives but is paused against an explicit extension. ## Why this reads as an omission rather than a decision `SandboxMetadata::is_expired` exists at `src/orchestrator/store/metadata.rs:121` and has its own unit test at `:141-142`. Its only other caller in the whole tree is a benchmark (`crates/benchmarks/benches/orchestrator_store_benchmark.rs:66`). It is called from no production code path — I checked with `grep -rn is_expired` across the repo. That looks like the predicate was written for exactly this purpose and never wired into the eviction path. ## Existing coverage The eviction tests are good, they just don't race this. `auto_evict_expired_sandbox` (`src/orchestrator/tests.rs:3608`) and `auto_evict_sandboxes_skips_non_running_and_non_expired_and_continues_on_error` (`:2762`) both drive eviction with no concurrent keep-alive. `keep_alive_uses_latest_metadata_when_deciding_whether_to_shorten` (`:2680`) does race keep-alive against a concurrent store write, but not against an eviction pass — so the interaction falls between two well-tested behaviours. ## Possible fix shapes You'll know better than me which fits the architecture, but the options I can see: 1. **Re-validate expiry inside the CAS.** Extend `update_if_state` (or add a variant) to take a predicate over the whole `SandboxMetadata` rather than just state, and have the inner eviction functions require "still expired." This is where `is_expired` would naturally be used. 2. **Pass the observed `expires_at` down** from the loop and have the eviction path abort if it no longer matches — a compare-and-swap on expiry rather than state. 3. **Re-read metadata at the top of each iteration** instead of trusting the snapshot. Simplest, and closes most of the window, though not atomically. Option 1 seems most robust and also fixes the `Pause` variant; option 3 is a much smaller change if you'd rather not touch the store trait. ## Caveats I have not reproduced this at runtime — I can't run the suite locally (no KVM on this machine), so this is from reading the code rather than from a failing test. If there's a serialisation guarantee I've missed that prevents the interleaving, I'd genuinely like to know, both because it means there's no bug and because it means I'm still misreading something about the lifecycle model. Happy to write a regression test for it if that's useful — the mock backend in `tests.rs` looks like it could drive the interleaving deterministically, and I'd rather contribute a failing test than a fix, given I've been wrong about your intent twice already. Thanks again for the care you've been putting into these threads. Genuinely nice to work with a project where the maintainers check things.

bsakaguchi closed this issue

2026-10-29 14:18:56 +00:00

bsakaguchi referenced this issue

2026-10-29 14:20:11 +00:00

[PR #56] [MERGED] fix(proxy): isolate connections across runtime generations #143

bsakaguchi referenced this issue

2026-10-29 14:20:43 +00:00

[PR #103] [MERGED] fix(sandbox): isolate envd bootstrap connections across generations #180