[PR #181] fix(pool): discard warm firecracker entries whose process died while parked #233

New issue

Open

opened 2026-10-29 14:21:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:21:19 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/181
Author: @epicvinny
Created: 8/18/2026
Status: 🔄 Open

Base: main ← Head: upstream/pool-dead-warm-entries

📝 Commits (8)

ee28f6c fix(pool): discard warm firecracker entries whose process died while parked
0b20901 fix(pool): propagate liveness probe errors and defer dead warm cleanup
0a88bc2 fix(pool): harden dead warm entry cleanup against shutdown races and failures
10e4cd8 fix(pool): keep dead warm slot allocated until teardown completes
90cd557 fix(pool): harden dead-entry cleanup against overflow, poisoning, and leaks
ce5d2ff fix(pool): close shutdown races in dead-entry cleanup finalization
e7ef3cb fix(pool): close dead-entry lost wakeup and schedule backoff retries
ddf7377 fix(pool): type dead-entry teardown failures and harden ip link del fallback

📊 Changes

6 files changed (+1363 additions, -25 deletions)

View changed files

📝 crates/warm-pool/src/lib.rs (+158 -2)
📝 src/sandbox/firecracker/instance.rs (+16 -0)
📝 src/sandbox/firecracker/pool.rs (+982 -10)
📝 src/sandbox/network/manager.rs (+69 -5)
📝 src/sandbox/network/mod.rs (+1 -1)
📝 src/sandbox/network/slot.rs (+137 -7)

📄 Description

What

FirecrackerPool::try_acquire now skips warm entries whose Firecracker process has already exited, discarding them (and releasing their network slot) instead of handing them to snapshot resume.

Why

Parked warm processes run with oom_score_adj=1000, so they are the first OOM-kill candidates and can die while idle in the pool. The acquire path popped whatever was parked and passed it straight to snapshot resume, which then failed on the dead process. Under host memory pressure this shows up as spurious sandbox-creation failures.

N/A. Small focused fix submitted directly per the contributing guide.

Scope and non-goals

Included: liveness check on acquire, discard of dead entries with network slot cleanup.
Excluded: any change to pool sizing, refill, or decay policy. No changes to the graceful-stop path for live entries.

Design and behavior changes

New FirecrackerInstance::is_process_running() based on try_wait.
try_acquire pops until it finds a live entry. Dead entries are logged and discarded.
The discard path skips graceful stop (the process is known dead) and avoids runtime block_on, so try_acquire stays safe to call from async context.

Compatibility and operations

Public API or generated protocol: N/A, no API change.
Configuration or defaults: N/A, no config change.
Snapshot manifest, artifact layout, or storage format: N/A.
Upgrade and rollback: N/A, behavior-only hardening of the acquire path.
Host requirements, permissions, ports, or dependencies: N/A.

Validation

make fmt (via cargo fmt --check)
make clippy (via cargo clippy -p warm-pool; this change touches src/sandbox/firecracker/)
make test-unit (scoped: cargo test -p warm-pool, 16 passed)
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated (rustdoc on the new/changed methods)
Benchmarks or performance comparison completed

Commands and results:

cargo fmt --check         # clean
cargo clippy -p warm-pool # clean
cargo test -p warm-pool   # test result: ok. 16 passed; 0 failed

Skipped checks and reasons: integration tests require root and /dev/kvm, not available on the Windows dev box where this was written. Happy to run them on a Linux host if reviewers want.

Risks and reviewer notes

Main risk is the dead-entry discard releasing the network slot; it mirrors the existing cleanup path and only runs for entries whose process is confirmed dead.
Review focus: src/sandbox/firecracker/pool.rs (acquire_live_warm, discard_dead_warm) and instance.rs (is_process_running).

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/181 **Author:** [@epicvinny](https://github.com/epicvinny) **Created:** 8/18/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `upstream/pool-dead-warm-entries` --- ### 📝 Commits (8) - [`ee28f6c`](https://github.com/kvcache-ai/AgentENV/commit/ee28f6cb581a545a161e4b2ecfb087291f67a5c1) fix(pool): discard warm firecracker entries whose process died while parked - [`0b20901`](https://github.com/kvcache-ai/AgentENV/commit/0b20901c26eaf57a392fca7d29835e0c44b8ea8c) fix(pool): propagate liveness probe errors and defer dead warm cleanup - [`0a88bc2`](https://github.com/kvcache-ai/AgentENV/commit/0a88bc2b123ed21c44d054145b4a67c568c22d15) fix(pool): harden dead warm entry cleanup against shutdown races and failures - [`10e4cd8`](https://github.com/kvcache-ai/AgentENV/commit/10e4cd8755002ac82363980d0ea7d1c3f52afc52) fix(pool): keep dead warm slot allocated until teardown completes - [`90cd557`](https://github.com/kvcache-ai/AgentENV/commit/90cd557d7c5d9ae02bd1bbf8c3b0610db58b0221) fix(pool): harden dead-entry cleanup against overflow, poisoning, and leaks - [`ce5d2ff`](https://github.com/kvcache-ai/AgentENV/commit/ce5d2ff19525cc33cff2cee998aac69798f43af0) fix(pool): close shutdown races in dead-entry cleanup finalization - [`e7ef3cb`](https://github.com/kvcache-ai/AgentENV/commit/e7ef3cb9bad7f2587b200134fda3078b22a5e5dd) fix(pool): close dead-entry lost wakeup and schedule backoff retries - [`ddf7377`](https://github.com/kvcache-ai/AgentENV/commit/ddf73770920842b664bb58b37f5541c496360dcf) fix(pool): type dead-entry teardown failures and harden ip link del fallback ### 📊 Changes **6 files changed** (+1363 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `crates/warm-pool/src/lib.rs` (+158 -2) 📝 `src/sandbox/firecracker/instance.rs` (+16 -0) 📝 `src/sandbox/firecracker/pool.rs` (+982 -10) 📝 `src/sandbox/network/manager.rs` (+69 -5) 📝 `src/sandbox/network/mod.rs` (+1 -1) 📝 `src/sandbox/network/slot.rs` (+137 -7) </details> ### 📄 Description ## What `FirecrackerPool::try_acquire` now skips warm entries whose Firecracker process has already exited, discarding them (and releasing their network slot) instead of handing them to snapshot resume. ## Why Parked warm processes run with `oom_score_adj=1000`, so they are the first OOM-kill candidates and can die while idle in the pool. The acquire path popped whatever was parked and passed it straight to snapshot resume, which then failed on the dead process. Under host memory pressure this shows up as spurious sandbox-creation failures. ## Related issue N/A. Small focused fix submitted directly per the contributing guide. ## Scope and non-goals - Included: liveness check on acquire, discard of dead entries with network slot cleanup. - Excluded: any change to pool sizing, refill, or decay policy. No changes to the graceful-stop path for live entries. ## Design and behavior changes - New `FirecrackerInstance::is_process_running()` based on `try_wait`. - `try_acquire` pops until it finds a live entry. Dead entries are logged and discarded. - The discard path skips graceful stop (the process is known dead) and avoids runtime `block_on`, so `try_acquire` stays safe to call from async context. ## Compatibility and operations - Public API or generated protocol: N/A, no API change. - Configuration or defaults: N/A, no config change. - Snapshot manifest, artifact layout, or storage format: N/A. - Upgrade and rollback: N/A, behavior-only hardening of the acquire path. - Host requirements, permissions, ports, or dependencies: N/A. ## Validation - [x] `make fmt` (via `cargo fmt --check`) - [x] `make clippy` (via `cargo clippy -p warm-pool`; this change touches `src/sandbox/firecracker/`) - [x] `make test-unit` (scoped: `cargo test -p warm-pool`, 16 passed) - [ ] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [x] Documentation updated (rustdoc on the new/changed methods) - [ ] Benchmarks or performance comparison completed Commands and results: ```text cargo fmt --check # clean cargo clippy -p warm-pool # clean cargo test -p warm-pool # test result: ok. 16 passed; 0 failed ``` Skipped checks and reasons: integration tests require root and `/dev/kvm`, not available on the Windows dev box where this was written. Happy to run them on a Linux host if reviewers want. ## Risks and reviewer notes - Main risk is the dead-entry discard releasing the network slot; it mirrors the existing cleanup path and only runs for entries whose process is confirmed dead. - Review focus: `src/sandbox/firecracker/pool.rs` (`acquire_live_warm`, `discard_dead_warm`) and `instance.rs` (`is_process_running`). ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-29 14:21:19 +00:00

bsakaguchi referenced this issue

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

[PR #233] [CLOSED] test(e2e): cover multi-node p2p across sandbox churn #273