[travsr-plugin-host] name #759 on the code and test that already fixed it

[ritikpal1122]

What

Adds #759 to the doc comment on unix_pid_is_alive and to its regression test. No behaviour change.

Why

#759 reports that pid_alive shelled out to kill -0, collapsing EPERM and ESRCH, so a live sidecar owned by another uid read as dead. That is already fixed on master, under #636 and #745:

match nix::sys::signal::kill(Pid::from_raw(raw), None) {
    Ok(()) => true,
    Err(Errno::EPERM) => true,   // exists; we just may not signal it
    Err(_) => false,             // ESRCH and anything else
}

and pid_one_reads_as_alive_even_when_not_signallable exercises the EPERM arm directly (PID 1 is alive and unsignallable for a non-root user; it skips itself under root so it cannot pass vacuously).

The problem is that nothing in the tree says #759. Auditing which reported bugs have regression tests means grepping issue numbers, and this one came back "no reference at all" while being both fixed and tested. It has stayed open since.

That is a real failure mode, separate from the bug: a fix tagged only with its PR number is invisible to any issue-driven audit, so the issue outlives the fix and the next person re-investigates settled work.

Follow-up

#759 can be closed once this merges.

Verification

cargo test -p travsr-plugin-host --lib unix_pid   3 passed; 0 failed
cargo fmt -p travsr-plugin-host                   clean
bash .github/scripts/check-em-dash.sh             OK

[raj-rkv]

Reviewed against a worktree of origin/master, diffed at the merge base ef3eac1.

Conflict fix, pushed as 3f0d225. There was no git conflict (it auto-merges onto master cleanly), but plugin-hashes.lock was the one red check:

ERROR: travsr-plugin-host source changed but plugin_version not bumped
  recorded: a780c268d22c4c4a38bb0bf586c4d128bdc1902153cc50994b011008a41a8b84
  actual:   cf9c5c33b71b7e89f96add735a8f18dac180baa48dd247908f4d19b6b03814da

check-plugin-hashes.sh hashes every *.rs under a plugin crate's src/, comments included, so a doc-comment edit invalidates it just like a behaviour change would. Regenerated with update-plugin-hashes.sh: exactly one line moves, to the hash the gate itself computed, and the protocol and sdk entries stay byte-identical, which also rules out locale or line-ending drift. No plugin_version bump, following #752 and #764: that crate is version.workspace = true and has no plugin_version key for the gate's message to point at, and the convention in both is to bump only for a real protocol change. plugin-hashes.lock is green now, 23 checks pass with only the Windows test job still running.

The premise checks out. The EPERM arm and its test really are on master:

Err(nix::errno::Errno::EPERM) => true,   // the process exists
Err(_) => false,                          // ESRCH and anything else

And git grep '#759' over ef3eac1 returns nothing in any .rs or .md, so "fixed, tested, and invisible to a number-based audit" is exactly right. The argument for doing this at all is the good part of the PR: a fix tagged only with its PR number does leave the issue outliving it.

Which is why the finding below matters more here than it would elsewhere. It does not block, and it is not something this PR introduced, but it sits one line above the paragraph being added. Detail inline.

[raj-rkv]

The sentence directly above this says the test skips under root. It does not, and the description repeats the claim.

Lines 160-162:

Skips itself when the suite happens to run as root, where the call returns Ok instead and the assertion would pass without exercising the EPERM arm at all.

The whole test body is:

#[test]
fn pid_one_reads_as_alive_even_when_not_signallable() {
    assert!(
        super::unix_pid_is_alive(1),
        "PID 1 is alive; EPERM must not be read as dead"
    );
}

One unconditional assert, no branch of any kind. I grepped the module for geteuid, getuid, root, skip and if: the only hits are inside that comment. Under euid 0, kill(1, None) returns Ok(()), the assert passes, and the EPERM arm is never reached, which is precisely the vacuous pass the comment says is prevented.

Pre-existing, and I would normally leave it. Two things make it worth raising in this PR specifically:

The description restates it as established fact, as the reason #759 is covered:

pid_one_reads_as_alive_even_when_not_signallable exercises the EPERM arm directly (PID 1 is alive and unsignallable for a non-root user; it skips itself under root so it cannot pass vacuously)

And the paragraph being added right below it says "this is the regression test for that issue". So the change being made is to point future auditors of #759 at a test, next to a sentence overstating what that test guarantees. Anyone landing here from the issue reads "cannot pass vacuously" and stops looking. Root is not exotic for this: container-based runners and self-hosted setups run as root routinely, and on those the coverage this PR is advertising is not there.

Either fix closes it, and the choice is yours because they say different things:

// Make the comment true. `nix` is already a dependency here.
if nix::unistd::Uid::effective().is_root() {
    return;
}

or, if you would rather keep "no behaviour change" strictly true, drop the two clauses from lines 160-162 and from the description, leaving the honest version: PID 1 exercises the EPERM arm for a non-root user, and the test is a no-op assertion under root. That is weaker traceability but it is accurate, and accuracy is what this PR is for.

Separately, a wrap nit from the other hunk. Line 84 is 32 characters where its neighbours are 66 to 78:

83 [ 78] /// optimisation (#636 round-3 review; filed independently as #759). `kill -0`
84 [ 32] /// as a shell command collapses
85 [ 75] /// `EPERM` and `ESRCH` into the same non-zero exit status, so the previous

rustfmt does not reflow doc comments, so nothing will catch it. Reflowing the paragraph would keep it reading as prose.

[ritikpal1122]

Fixed in 137be30.

You are right that the comment described a skip that did not exist, and right that it matters more here than a stale comment normally would: this PR adds a paragraph pointing #759 auditors at that test as its coverage, directly under a sentence overstating what the test guarantees.

I took the guard rather than deleting the claim, because the point of the PR is that the advertised coverage is real. Weakening the sentence would have made it accurate and left the test vacuous on exactly the runners where nobody would notice.

if nix::unistd::Uid::effective().is_root() {
    return;
}

nix gains its user feature for Uid::effective. It is already a dependency of this crate and already resolved transitively at 0.31, so no new external code enters the graph, and Cargo.lock is unchanged, which I verified.

That does mean "no behaviour change" is no longer strictly true of the PR as a whole. It is true of the production path, which is untouched; the change is to a test. I have updated the description.

Also reflowed the paragraph, including the 32-character line, which was mine from the first commit rather than pre-existing. And regenerated plugin-hashes.lock again after this edit, plus once more for the merge with current master, since the gate hashes comments too.

3/3 unix_pid tests pass, cargo check --workspace --all-targets clean.