fix: meta dev does not exit properly upon SIGTERM

[michael-0acf4]

Migration notes

None

• The change comes with new or modified tests
• Hard-to-understand functions have explanatory comments
• End-user documentation is updated to reflect the change

[michael-0acf4]

A few remarks so far,

• This is not an issue with Ctrl+C or SIGINT
• This happens when we send a SIGTERM, SIGKILL to the parent process meta dev or SIGHUP (controlling terminal death) , meta typegate is still pending: making next cli spawn error out saying that the port is already used. So I imagine the issue is apparent in programmatically spawned processes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.49%. Comparing base (18a13d6) to head (07bc751).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
+ Coverage   77.46%   77.49%   +0.02%     
==========================================
  Files         149      149              
  Lines       18244    18304      +60     
  Branches     1785     1787       +2     
==========================================
+ Hits        14133    14184      +51     
- Misses       4088     4097       +9     
  Partials       23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Natoandro]

Do you think you can add some tests?

[michael-0acf4]

Do you think you can add some tests?

So, I digged a bit and as far as I know, there seems to be no way to retrieve all PIDs or get handles of the child processes that are spawned by child processes in Deno.

Spawn, kill with SIGTERM/SIGHUP, spawn again then checking the std output for Addr in use is an alternative but I wonder if it's really worth it as it may came from bad port allocation and not the child process being alive itself...

[luckasRanarison]

So, I digged a bit and as far as I know, there seems to be no way to retrieve all PIDs or get handles of the child processes that are spawned by child processes in Deno.

Can't you just check the processes using shell commands?

[Yohe-Am]

Looks good. So, there's this FIXME case in the tests where the meta dev leaks the typegate process. It doesn't seem to show up on my dev machine anymore but it was a regular on the coder machines. It also regularly leaked on the CI machines and...

...yeah, I just checked by running the CI on this PR and it still leaks it seems. The following screenshot is from this CI run observed through a tmate session.

The two tests that use the meta-full build have already passed by the time I took this screenshot.
Hopefully, this PR improves the state in developer shells but the bug seems still at large.

Edit: replaced screenshot with one that shows currently running test.

[michael-0acf4]

Looks good. So, there's this FIXME case in the tests where the meta dev leaks the typegate process. It doesn't seem to show up on my dev machine anymore but it was a regular on the coder machines. It also regularly leaked on the CI machines and...

...yeah, I just checked by running the CI on this PR and it still leaks it seems. The following screenshot is from this CI run observed through a tmate session.

The two tests that use the meta-full build have already passed by the time I took this screenshot.
Hopefully, this PR improves the state in developer shells but the bug seems still at large.

Edit: replaced screenshot with one that shows currently running test.

That is surprising, the way I tested it was sending the signals directly through htop, and kill the parent cargo run -p meta-cli -- dev for SIGTERM. I also experimented quitting the terminal that spawned it for the SIGHUP case (+htop) so it's all manual.. not sure if there is something that I might have missed but I didn't have the already used port issue anymore in all cases, ultimately i will try to write a test for it after all.

[michael-0acf4]

there's this FIXME

Ctrlc would be SIGKILL for that matter and that at least should work even without this patch.

[Yohe-Am]

Ctrlc would be SIGKILL for that matter and that at least should work even without this patch.

Ctrl C corresponds to SIGTERM actually. SIGKILL is the harsher cousin that immediately kills the process. You're not able to register handlers for it and do clean up. In that FIXME, I'm using SIGTERM to give it a chance to clean up but no bueno.

[michael-0acf4]

Ctrlc would be SIGKILL for that matter and that at least should work even without this patch.

Ctrl C corresponds to SIGTERM actually. SIGKILL is the harsher cousin that immediately kills the process. You're not able to register handlers for it and do clean up. In that FIXME, I'm using SIGTERM to give it a chance to clean up but no bueno.

My bad, I meant SIGINT. But you are right, SIGTERM is more appropriate.