fix(pool): discard warm firecracker entries whose process died while parked by epicvinny · Pull Request #181 · kvcache-ai/AgentENV

epicvinny · 2026-08-18T00:46:10Z

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

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.

…parked Parked warm processes run with oom_score_adj=1000, so they are the first OOM-kill candidates and can exit while idle in the pool. The acquire path handed whatever it popped straight to snapshot resume, which then failed on the dead process. Add FirecrackerInstance::is_process_running() and make try_acquire pop until a live entry is found. Dead entries skip the graceful-stop path (the process is already gone): the instance is dropped and the network slot is released synchronously, keeping the acquire path free of runtime block_on calls so it stays safe in async context.

github-actions · 2026-08-18T00:48:57Z

🔍 OpenCodeReview found 10 issue(s) in this PR.

✅ Successfully posted inline: 10 comment(s)

FirecrackerInstance::is_process_running collapsed every try_wait() I/O error into "process not running", so the pool would tear down an entry (and release its network slot) on an inconclusive probe. Return std::io::Result<bool> instead; on Err the pool logs the failure, returns the entry to the pool keeping its slot, and reports a regular miss. try_acquire() also ran full network teardown for dead entries inline, from the async snapshot-resume path; cleanup_allocated_slot does blocking netlink/ip work that can stall a runtime worker thread. Dead entries now go onto a dead_entries queue drained by the maintenance worker at the start of each cycle, next to the teardown it already owns. Shutdown paths drain the queue too, and with maintenance disabled the cleanup falls back to inline execution since no worker exists.

…failures Address follow-up review on the deferred dead-entry cleanup: - The maintenance-disabled fallback ran blocking teardown inline on the async acquire path; it now spawns a detached cleanup thread instead. - Enqueue was not coordinated with shutdown: an acquire racing drain_all could queue an entry after shutdown had already drained the queue, leaving it without a consumer. The queue now carries a closed flag checked under the same lock; shutdown closes it right after drain_all and late enqueues clean up inline. - cleanup_dead_warm_entries took every entry out of the queue before cleanup and only logged failures, losing track of stale host network state while the slot index was already back in the allocation bitmap. Failed entries are now retained in the queue and retried on later cycles. Retries go through the new NetworkManager::cleanup_slot_resources which redoes only the resource teardown: the allocation bit is released on the first attempt and must never be released twice, since the index may have been reallocated to a live sandbox. cleanup_allocated_slot now borrows the Slot so failed teardown can keep the entry alive for retry; Slot::drop remains the last-resort cleanup.

Address remaining review threads on dead warm entry cleanup: - A failed first cleanup released the allocation bit while the retained SlotOnly entry could still retry index-derived teardown (veth-<idx>) after the index was reallocated to a live sandbox. The bit now stays allocated until teardown succeeds via NetworkManager::cleanup_allocated_slot_retain_bit_on_failure, so every retry still owns the resources it deletes. - The detached cleanup thread (maintenance-disabled mode) was untracked and unjoined, and a spawn failure dropped the entry on the async acquire path, forcing synchronous Slot::drop cleanup. A managed cleanup worker now drains the queue, is stored in the queue and joined by close_dead_queue, sleeps in bounded slices so shutdown join stays fast, and a spawn failure leaves entries queued (non-blocking) for the next enqueue or for shutdown to drain inline. - Retained failed entries propagated their error out of the maintenance cycle, blocking fill/drain and hot-looping the worker on the same failing teardown. Dead-entry cleanup and watermark maintenance now run as independent work with errors aggregated afterwards, and failed teardowns are requeued with exponential backoff (100ms doubling, capped at 30s) instead of being retried every cycle.

… leaks Address the second round of review threads: - The retry counter overflowed after u32::MAX failed attempts, panicking the cleanup worker in debug builds; it now saturates (the backoff already caps at 30s). - All dead-queue locks recover from mutex poisoning via into_inner(), so a panicked cleanup worker cannot turn into a pool-wide panic on the next lock. - Entries whose teardown failed while close_dead_queue raced in-flight cleanup were dropped: Slot::drop re-ran resource cleanup but never released the allocation bit, leaking the index permanently, and the failure never reached shutdown's result. A shared finalize_closed_dead_entry helper now retries teardown once on the calling blocking thread, releases the bit explicitly as a last resort, and reports the failure; the worker records it in the queue so close_dead_queue returns it to shutdown. - The shutdown join was not actually bounded because the synchronous 'ip link del' fallback ran Command::output() without a timeout. The fallback now enforces a 10s timeout (spawn + try_wait + kill), and close_dead_queue joins the worker with a bounded polling wait (DEAD_WARM_CLEANUP_JOIN_TIMEOUT) instead of an unbounded join.

Address the third round of review threads: - cleanup_dead_warm_entries paired failed entries with errors via a misaligned zip: backing-off entries could be dropped on shutdown and leak their allocation bits. Backing-off and failed entries are now tracked separately, and on queue close every retained entry is finalized (failed ones with their own error, backing-off ones with a plain teardown attempt that reports only if it actually fails). finalize_closed_dead_entry now takes Option<Error> and returns Option<String> accordingly. - A worker spawn failure in maintenance-disabled mode left entries retained until the next enqueue or shutdown; the spawn is now retried once via an unnamed Builder before falling back to the queued path. - A join timeout no longer detaches the cleanup worker: the handle is kept managed in the queue for a later close to retry, and an explicit incomplete-shutdown failure is reported instead of silent completion. - The async shutdown() ran the bounded join, sleeps, and blocking netlink/ip teardown inline on a Tokio worker; that portion now runs in tokio::task::spawn_blocking via shutdown_dead_entries_blocking. - The 'ip link del' timeout path could still block: kill() errors are propagated and the child is reaped with bounded try_wait polling (1s grace) instead of a blocking wait(). - Tests now inject a genuine teardown failure (fail_slot_teardown hook) while the allocation bit stays held, asserting the index cannot be reallocated until a successful retry releases it.

…allback