fix: shpool kill: ignore missing target process - #405
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Please don't just delete the PR template, it is there for a reason. In this case, I would like to know your use of AI to generate this patch. |
| // long-dead child as an error leaves an unkillable phantom session | ||
| // that haunts the list until a daemon restart. | ||
| match signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGHUP)) { | ||
| Err(nix::errno::Errno::ESRCH) => return Ok(()), |
There was a problem hiding this comment.
This is not that common a code to work with, so let's document it as "no such process" in a comment.
| // but for shells SIGHUP serves as the graceful shutdown signal. | ||
| signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGHUP)) | ||
| .context("sending SIGHUP to child proc")?; | ||
| // A child that is already gone is exactly the state kill exists to |
There was a problem hiding this comment.
Please delete this whole comment block in favor of just documenting what the code means. It should be clear from context why we ignore ESRCH if we explain what it means. This comment block reads like an AI report about the bug injected into the patch, but I don't think it is worth adding a big comment about this over the long term.
|
Looks pretty good, please sign the CLA and disclose your AI use. |
ethanpailes
changed the title
47c6a07 to
55f3e88
Compare
|
@googlebot I signed it! |
|
Looks like CLA bot is still grumpy. Also, can you add a test for this. The regression or kill test suites are probably good places to put one. |
fc848ed to
3857db4
Compare
|
Sorry, I missed your test requests from Friday. All three have tests now:
Each one fails on master and passes with its patch. Also addressed both review comments: dropped the comment block and documented ESRCH as "no such process" at each call site. |
ethanpailes
left a comment
ethanpailes
left a comment
There was a problem hiding this comment.
Looks pretty good, just one last nit about redundancy in the support file.
| } | ||
|
|
||
| /// Scan lines until one matches the given regex, returning its captures. | ||
| pub fn scan_until_captures_re(&mut self, re: &str) -> anyhow::Result<Vec<Option<String>>> { |
There was a problem hiding this comment.
This is pretty redundant with scan_until_re. Let's implement scan_until_re in terms of this routine. I know there is overhead to getting capture groups, but it is pretty small. Also let's call this scan_until_re_captures instead because we're still scanning until the whole regex matches and just extractng the groups afterwards.
There was a problem hiding this comment.
Done, renamed to scan_until_re_captures and scan_until_re now just calls it and drops the captures.
If the shell is already gone, kill fails on ESRCH and the error stops the daemon from removing the session from its table, so the session can never be killed and lingers until the daemon restarts. Covered by kill::already_dead_shell.
3857db4 to
74f7286
Compare
2 participants
Issue Link
(none)
AI Policy Ack
I have read the AI Policy. This patch was written with Claude Code, working from failures in my own shpool deployment. Test coverage was added during review:
kill::already_dead_shell, which fails without the fix.This PR was:
Description
Session::kill()propagates ESRCH from the SIGHUP as an error, andhandle_killthen aborts before removing the session from the shellstable. A session whose child exited on its own (crash, external kill,
missed reap) therefore becomes a phantom:
shpool listkeeps advertisingit, attach fails, and no kill can ever remove it until the daemon
restarts.
Log signature:
Reproduce on stock 0.11.0:
kill -9a session's shell process, thenshpool killthat session.This treats ESRCH as the state kill exists to reach, at both the SIGHUP
and SIGKILL-escalation sites.