fix(server): reap orphaned runtime processes and retry RocksDB lock on startup - #189
fix(server): reap orphaned runtime processes and retry RocksDB lock on startup#189epicvinny wants to merge 1 commit into
Conversation
…n startup After a hard kill (SIGKILL, OOM, force-deleted pod), firecracker and uvm-ublk-daemon children spawned with process_group(0) can outlive the server while still holding ublk devices, open image files, and inherited fds that pin the RocksDB LOCK of the persisted-sandbox store. The next server then fails to open records.db and crash-loops. Observed in staging: recovery required kubectl debug node + manually killing the leaked PIDs. Two changes make startup resilient: - src/setup/orphans.rs (wired in src/bin/server.rs before any subsystem spawns): scan /proc for processes whose executable is the configured firecracker or uvm-ublk-daemon binary - necessarily leftovers from a previous incarnation - and terminate them (SIGTERM, 5s grace, SIGKILL). Zombies are ignored. - src/local_store.rs: LocalKvStore::open now retries lock-contention errors with exponential backoff for up to 120s, covering lock holders in a different PID namespace (e.g. a previous container still terminating on the same hostPath) that the reaper cannot see.
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let db = loop { | ||
| let attempt_path = path.clone(); | ||
| let result = tokio::task::spawn_blocking(move || { | ||
| let mut options = Options::default(); | ||
| options.create_if_missing(true); | ||
| DB::open(&options, &attempt_path) | ||
| .with_context(|| format!("open RocksDB {}", attempt_path.display())) | ||
| }) | ||
| .await | ||
| .context("join RocksDB open task")?; |
There was a problem hiding this comment.
The retry budget is not a strict upper bound: when the sleep consumes the remaining budget, the loop unconditionally starts another DB::open attempt because the deadline is checked only after that attempt fails. A final attempt can therefore begin (and potentially succeed) after LOCK_RETRY_BUDGET, and a slow blocking open can extend startup even further. Check the deadline before starting each attempt (and avoid sleeping/attempting once no time remains); if the bound must include DB::open, also account for or bound the blocking operation.
| let mut orphans = find_processes_by_exe(Path::new("/proc"), &targets); | ||
| if orphans.is_empty() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This scan treats every process in the PID namespace running either configured executable as an orphan, but it does not verify that the process was spawned by this agent or belongs to a prior incarnation. The startup path has no ownership marker/instance lock in this module, so a concurrently started server, another agent instance, or an independently managed Firecracker using the same binary can be terminated here, disrupting active sandboxes. Scope discovery to agent-owned process groups/metadata (and coordinate concurrent startups) before signaling.
| let mut orphans = find_processes_by_exe(Path::new("/proc"), &targets); | ||
| if orphans.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| orphans.sort_unstable(); |
There was a problem hiding this comment.
Matching processes solely by executable path is not sufficient to establish that they are orphaned children of a previous server. This scans the whole PID namespace and will terminate every process running the configured firecracker/uvm-ublk-daemon, including workloads belonging to another live server, a concurrently starting instance, or an operator-launched process. It also leaves a PID-reuse race between discovery and kill. Record and validate an ownership marker (for example, the expected process-group/session plus parent/start-time identity, or a per-incarnation pid/state file) before signaling; otherwise this startup cleanup can cause cross-instance workload termination.
| for pid in &orphans { | ||
| signal(*pid, Signal::SIGTERM); | ||
| } | ||
| let survivors = wait_for_exit(&orphans, TERM_GRACE).await; |
There was a problem hiding this comment.
Firecracker is explicitly launched with process_group(0), and this module's contract says runtimes may have descendants in that group, but this sends signals only to the executable PID. If the leader has spawned helpers/children, they can retain inherited file descriptors, namespaces, or ublk resources after the leader exits, so startup can still be wedged despite reporting the orphan as reaped. Capture/use the process-group ID and signal the group (with an ownership check) rather than only the leader.
| for pid in &survivors { | ||
| signal(*pid, Signal::SIGKILL); | ||
| } | ||
| let stuck = wait_for_exit(&survivors, KILL_GRACE).await; |
There was a problem hiding this comment.
After the initial /proc scan, the code keeps only numeric PIDs and later signals those PIDs without rechecking /proc/<pid>/exe (or a process start-time/ownership identity) immediately before each signal. A matching process can exit and its PID be reused during the 5-second wait, causing SIGKILL to reach an unrelated process; a replacement runtime can likewise be mistaken for the original. Revalidate identity before escalation and while polling, or retain a stable process identity such as PID plus /proc/<pid>/stat start time.
1 participant
What
Makes node-agent startup resilient to leftover runtime processes and held RocksDB locks from a previous server incarnation:
src/setup/orphans.rs, wired insrc/bin/server.rs): early in startup — before the ublk daemon is spawned and before any store is opened — the server scans/procfor processes whose executable is the configured Firecracker binary oruvm-ublk-daemonbinary. Any match necessarily belongs to a previous incarnation (nothing has been spawned yet), so they are terminated: SIGTERM, 5s grace, then SIGKILL. Zombie entries are ignored (they hold no resources).src/local_store.rs):LocalKvStore::opennow retries lock-contention errors (No locks available/Resource temporarily unavailableon theLOCKfile) with exponential backoff for up to 120s instead of failing immediately. This covers the case where the lock holder lives in a different PID namespace (e.g. a previous container still terminating on the same hostPath) and therefore cannot be seen or killed by the reaper.Why
Observed in staging: after a pod was terminated (force-deleted after the preStop hang addressed in #187),
firecrackeranduvm-ublk-daemonprocesses from the old container were still alive on the host, and the replacement pod CrashLoopBackOffed trying to open the persisted-sandbox store at/var/lib/aenv/env/persisted-sandboxes/records.db— the RocksDBLOCKwas still held. Recovery requiredkubectl debug node/... -- chroot /hostand manually killing the leaked PIDs.Two contributing gaps in the current code:
src/sandbox/firecracker/instance.rs) and the ublk daemon (storage/ublk-daemon/src/client.rs) are spawned in their own process groups (process_group(0)). Graceful teardown (orchestrator pause-all, pool shutdown, daemon shutdown) only runs on SIGTERM/Ctrl+C; on SIGKILL/OOM/force-delete nothing runs and the children persist with their resources (ublk devices, open image files, inherited fds).LocalKvStore::openmade a singleDB::openattempt, so a still-terminating previous holder turned startup into an immediate fatal error → CrashLoopBackOff.Scope and non-goals
LocalKvStore::open(benefits all three callers: persisted-sandbox records, image cache graph, P2P catalog), unit tests for both.Compatibility and operations
Validation
cargo test -p agentenv --lib local_store(3 passed, incl. retry-until-release and budget-exhaustion cases with a real held RocksDB lock)cargo test -p agentenv --lib setup::orphans(4 passed: exe matching,(deleted)binary suffix, non-numeric/missing exe entries, zombie state parsing)cargo clippy -p agentenv --lib --bins -- -D warningscargo fmt --checkon touched files