bug#57922: Shepherd doesn't seem to correctly handle waitpid itself

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

reopen 57922
tags 57922 -notabug
thanks

Hi again,

[...]

>>> Here's a small reproducer to apply on our code base:
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> modified   gnu/services/telephony.scm
>>> @@ -685,13 +685,7 @@ (define (archive-name->username archive)
>>>
>>>                      ;; Finally, return the PID of the daemon process.
>>>                      daemon-pid))
>>> -               (stop
>>> -                #~(lambda (pid . args)
>>> -                    (kill pid SIGKILL)
>>> -                    ;; Wait for the process to exit; this prevents overlapping
>>> -                    ;; processes when issuing 'herd restart'.
>>> -                    (waitpid pid)
>>> -                    #f))))))))
>>> +               (stop #~(make-kill-destructor))))))))
>
> I think the main difference between these two is that the first one uses
> SIGKILL while the second one uses SIGTERM.
>
> You could try #~(make-kill-destructor SIGKILL) to get the same effect.

> You are right, the important difference was SIGTERM vs SIGKILL.  I
> thought I had tried that.  The problem only shows itself in the
> 'jami-provisioning' system test, not the 'jami' one.

> Marking this one as notabug and closing.

I think I spoke too soon.  SIGKILL does fix the problem when *not* using
waitpid explicitly, but when using waitpid explicitly, SIGTERM can be
used just fine.  In other words, this works:

--8<---------------cut here---------------start------------->8---
@@ -687,7 +687,7 @@ (define (archive-name->username archive)
                     daemon-pid))
                (stop
                 #~(lambda (pid . args)
-                    (kill pid SIGKILL)
+                    (kill pid SIGTERM)
                     ;; Wait for the process to exit; this prevents overlapping
                     ;; processes when issuing 'herd restart'.
                     (waitpid pid)
--8<---------------cut here---------------end--------------->8---

but this doesn't:

--8<---------------cut here---------------start------------->8---
@@ -685,13 +685,7 @@ (define (archive-name->username archive)
 
                     ;; Finally, return the PID of the daemon process.
                     daemon-pid))
-               (stop
-                #~(lambda (pid . args)
-                    (kill pid SIGKILL)
-                    ;; Wait for the process to exit; this prevents overlapping
-                    ;; processes when issuing 'herd restart'.
-                    (waitpid pid)
-                    #f))))))))
+               (stop #~(make-kill-destructor))))))))
 
 (define jami-service-type
--8<---------------cut here---------------end--------------->8---

when exercised with 'make check-system TESTS=jami-provisioning':

--8<---------------cut here---------------start------------->8---
This is the GNU system.  Welcome.
jami login: Jami Daemon 13.4.0, by Savoir-faire Linux 2004-2019
https://jami.net/
[Video support enabled]
[Plugins support enabled]

23:29:05.375         os_core_unix.c !pjlib 2.12.1 for POSIX initialized
shepherd: Service jami has been stopped.
Caught signal Terminated, terminating...

Some deprecated features have been used.  Set the environment
variable GUILE_WARN_DEPRECATED to "detailed" and rerun the
program to get more information.  Set it to "no" to suppress
this message.
Jami Daemon 13.4.0, by Savoir-faire Linux 2004-2019
https://jami.net/
[Video support enabled]
[Plugins support enabled]

One does not simply initialize the client: Another daemon is detected
/gnu/store/2vcv1fyqfyym2zcyf5bvbj1pcgbcc515-shepherd-marionette.scm:1:1718: ERROR:
  1. &action-exception-error:
      service: jami
      action: start
      key: misc-error
      args: (#f "~A ~S ~S ~S" (dbus "method failed with error" "org.freedesktop.DBus.Error.NoReply" ("Message recipient disconnected from message bus without replying")) #f)
--8<---------------cut here---------------end--------------->8---
      
or manually through the test VM:

--8<---------------cut here---------------start------------->8---
$(./pre-inst-env guix system vm --no-graphic --no-grafts --no-offload \
  -e '(@@ (gnu tests telephony) %jami-os-provisioning)')  \
  -m 1G -smp $(nproc) "-nic" user,model=virtio-net-pci,hostfwd=tcp::10022-:22
--8<---------------cut here---------------end--------------->8---

This leads me to believe that Shepherd does not block until the process
is actually dead to mark the process as stopped (it just waitpid on the
group pid with WNOHANG), which means it won't block if the child process
hasn't exited yet, if I'm correct.

When we are in the stop slot, we know for sure that the process should
terminate completely, hence it'd make sense to call 'waitpid' *without*
WNOHANG there, to avoid 'herd restart' from starting the service while
its stopped process is not done terminating.

jamid can take quite some time to terminate cleanly because of the
networking threads in the opendht library that needs to be finalized,
which is probably the reason this problem can be observed here.

Thoughts?

Maxim