Fix zombie ssh processes from accumulating

[cesfahani]

Noticed that with a long-running app that makes use of SSH transport, I end up with a bunch of zombie ssh processes. Fix this by implementing Drop trait on SpawnProcessOnDemand.

[Byron]

Oh wow, it's very interesting to see that this is necessary.
But maybe, dropping a Process instance without waiting for the spawned process can cause ssh to become a un-parented in the first place, which later causes them to misbehave?

Or maybe it's an intended ssh capability, but gitoxide triggers it accidentally by dropping without killing it first? Most programs die when they notice that their stdin/stdout aren't connected anymore.

Something that also interests me is if you could find code in the Git codebase that does a similar thing?

[cesfahani]

Or maybe it's an intended ssh capability, but gitoxide triggers it accidentally by dropping without killing it first? Most programs die when they notice that their stdin/stdout aren't connected anymore.

The kill() likely isn't necessary. By the time the SpawnProcessOnDemand is dropped, the remote should have already closed the connection, so the ssh process should already be exited. But even with an exited child process, if the parent doesn't wait on it, it remains in the process table as defunct/zombied (this is what retains the exit code in case the parent does get around to calling wait sometime later).

Something that also interests me is if you could find code in the Git codebase that does a similar thing?

In the Git codebase, the git_connect() function returns a child_process pointer. As mentioned in this comment, each connection is supposed to be completed with finish_connect(), as seen here. That leads to finish_command(), then wait_or_wine(), and finally to the waitpid() call here.

[Byron]

Thanks so much for elaborating! I never knew how zombies worked, and wasn't aware that they come to existence if the process who spawned it doesn't wait on it.

If you look around where gix_command is used, I am sure you will find other places where the code spawns a process (like in gix-filter or gix-credentials) that isn't waited on anywhere, in case you want to submit more PRs like this one. For now, I am keeping track of it here.

I would have removed the kill() line myself but decided to pass it by you for a second opinion.

Once that is sorted, I will merge. Thanks for your patience.

[Byron]

I'd feel better if we could remove the kill() then. Despite being aware that it would probably prevent to hang in the wait() call, I'd only want to add it when it's proven to be necessary. Since Git doesn't seem to be doing that, I think neither should we.

[cesfahani]

I've removed it in my most recent update. I also confirmed that with this change, my application (a long-running daemon that uses gitoxide to do tons of git clones over SSH) does not show any zombied ssh processes.

[cesfahani]

Looks like I spoke too soon! Seems there's some tests where removing the kill() is causing a hang. Let me look into this...

[Byron]

Thanks for running this experiment!

In this case, Git might not even have all the answers as it would never encounter such a case, after all, it can't be used as a library.

Something that surprises me though is how wait() (without kill()) can still leave zombies - if it doesn't block forever then it should shut down the child for good.

[cesfahani]

Something that surprises me though is how wait() (without kill()) can still leave zombies - if it doesn't block forever then it should shut down the child for good.

Not sure what you mean... The issue with the test hang is that wait() is blocking forever. After adding the wait(), we are guaranteed to never have zombies (at least for this child process scenario) - we're just exposed to a deadlock possibility if the ssh process didn't shutdown gracefully.

I was able to fix the test hang with this change:

diff --git a/gix/src/remote/connection/fetch/receive_pack.rs b/gix/src/remote/connection/fetch/receive_pack.rs
index 7634b34cf..aeaf17033 100644
--- a/gix/src/remote/connection/fetch/receive_pack.rs
+++ b/gix/src/remote/connection/fetch/receive_pack.rs
@@ -253,6 +253,10 @@ where
                         .transpose()?
                         .unwrap_or(false);
                     if reject_shallow_remote {
+                        drop(reader);
+                        gix_protocol::indicate_end_of_interaction(&mut con.transport, con.trace)
+                            .await
+                            .ok();
                         return Err(Error::RejectShallowRemote);
                     }
                     shallow_lock = acquire_shallow_lock(repo).map(Some)?;

But... now that I look at it more, I feel like there's other scenarios where tke kill() is going to be needed. For example, while we're in the middle of reading the pack, the user can abort the operation via should_interrupt. If that happens, then there is no "graceful" way to indicate to the remote that we'd like to shutdown (AFAIK).

Thoughts @Byron ?

[Byron]

Right, I didn't know that deadlocks are actually happening then, and it makes sense to see these until the remote hangs up. Oh, and it looks like CI is already deadlocking.

And it's true, once interrupted, everything winds down and it's unclear in which state ssh is in that moment, so a kill() call would be required. Git is different, as in doesn't have that problem, as they just abort on signal. There is some special handling for tempfiles, but that's about it.

Thus, we really have to call kill here. Could you protect that kill() call with a comment that briefly explains why?

Then I think this can be merged, and we are back to were we were, but with a comment and a better understanding :).

[cesfahani]

Done. Thanks for the feedback!

[Byron]

Thanks a lot for bearing with me, I learned a lot.

CI should be green shortly, so I can merge.