Closed lun-4 closed 3 months ago
Thank you for the PR, it didn't happen in my testing, but maybe I was sending some other signal. I will test it tonight
I double checked that behavior against main branch, without my patch, and it does shutdown the child processes... oops. I was checking some weird interactions against the proxy and podman and wanted to explicitly catch the incoming SIGTERM so I know it is actually being sent to a certain PID rather than depending on some other runtime behavior to do it (this patch would still be likely useful in a future where we use better heuristics to figure out if something is actually shutdown and the relevant VRAM is free)
I tested this, and it works well, I'll fix the message separately
@lun-4 On a closer inspection, runningService.cmd.ProcessState was always nil, seems like it doesn't get automatically populated after sending a signal, so this was always exiting immediately (and the right check would've been processState != nil
), but that wasn't working anyway. I reworked the waiting code a bit: https://github.com/perk11/large-model-proxy/commit/d1c4afe388627b4154d84b4f122b7689c3be0a4b calling process.Wait() seems to do the trick.
oh I see, thanks for the note (and the merge), just tested it myself and seems to work as well, will keep the technique in mind whenever I'm polling a PID in the future!
currently, the behavior on SIGTERM is to exit ourselves but still leave all child processes running. this patch changes the behavior to actively catch SIGTERM and so, gracefully shutdown the services (by sending SIGTERM to them, waiting 2 seconds, then SIGKILL).
the TODO comment regarding wait-until-SIGKILL is left as more of a suggestion, if that's welcome I can submit that in a separate PR.