Skip to content

[travsr-plugin-host] unix pid_alive shells out to kill -0, so an EPERM sidecar reads as dead and is orphaned on shutdown #759

Description

@raj-rkv

Summary

The unix pid_alive in crates/travsr-plugin-host/src/embed_catalog.rs:485 probes liveness by shelling out to kill -0, and treats a non-zero exit as "not alive". EPERM (the process exists, this uid may not signal it) and ESRCH (no such process) both collapse to false, so a sidecar running under a different uid reads as dead.

This is the exact defect unix_pid_is_alive was written to fix (lib.rs:114-138, #636), which the same crate already exports and which handles EPERM correctly:

match nix::sys::signal::kill(nix::unistd::Pid::from_raw(raw), None) {
    Ok(()) => true,
    // The process exists; this user just may not signal it.
    Err(nix::errno::Errno::EPERM) => true,
    Err(_) => false,
}

The Windows arm of pid_alive documents this same class of failure as #500 and fixes it there. The unix arm still has it.

Impact: an orphaned reindex sidecar, reported as a clean shutdown

Both call sites are in terminate_inflight_reindex(), the daemon shutdown path (also reached from stop-embed).

The grace poll (around line 539):

if REINDEX_CHILD_PID.load(Ordering::SeqCst) == 0 || !pid_alive(pid) {
    tracing::info!(pid, "embed: reindex drained gracefully after cancel");
    ...
    return;
}

A false pid_alive makes !pid_alive(pid) true on the first iteration, so the daemon logs a successful graceful drain and returns immediately, before waiting any of the grace window. Because it returns there, the kill_pid fallback after the loop (around line 555, itself guarded by a second pid_alive(pid) that would also read false) is never reached.

Net effect when the sidecar runs under a different uid:

  • the sidecar is never sent SIGTERM and keeps running after the daemon exits,
  • the log positively asserts it "drained gracefully", so the orphan is invisible in the logs,
  • the full CANCEL_GRACE_SECS wait is skipped, so this is not a timing race that a slower host would survive.

Suggested fix

Fold the unix arm onto the existing probe:

#[cfg(unix)]
fn pid_alive(pid: u32) -> bool {
    crate::unix_pid_is_alive(pid)
}

That also drops a process spawn per 200 ms poll iteration, and removes the Command/Stdio dependency from this path. unix_pid_is_alive already excludes pid 0 and out-of-range values, so it is strictly more careful than the current version.

Worth a regression test that a pid the current uid cannot signal is reported alive. That is awkward to arrange hermetically without a second uid, so asserting the EPERM-to-true mapping at the unix_pid_is_alive boundary (already covered) plus a test that pid_alive delegates to it may be the practical shape.

Provenance

Raised twice in the #745 review (by @anketpratapsingh as a Major finding, and again in my re-review) and deliberately deferred there: it is pre-existing and sits on a line #745 does not touch, so it was correctly kept out of scope of that PR. Filing it so the deferral is tracked rather than lost. #745 is approved and unaffected.

Verified against origin/master at 5875507, not against the #745 branch, so this stands on its own regardless of what lands there.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingrepo:travsrOriginates from the travsr repo

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions