Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teleport Connect leaves orphaned processes #27125

Closed
ravicious opened this issue May 30, 2023 · 4 comments
Closed

Teleport Connect leaves orphaned processes #27125

ravicious opened this issue May 30, 2023 · 4 comments
Labels
bug teleport-connect Issues related to Teleport Connect.

Comments

@ravicious
Copy link
Member

Expected behavior:

When closing Teleport Connect, any shell or gateway CLI clients started by Connect should be closed together with the app.

Current behavior:

Connect does not close those processes, essentially leaving them orphaned.

Description

When working on #26441, I noticed that Connect leaks processes. I thought that it's maybe because of my changes, but the current stable version of Connect also exhibited this problem.

The processes are cleaned up only when the component displaying them gets unmounted. However, closing the window doesn't give the frontend app any time to clean up. As such, any cleanup should be done on the "backend" side of things.

ptyHostService in the shared process has access to all open processes. Before closing the shared process, we should dispose of all of them, similar to how we submit telemetry data from tshd before closing the process. Make sure that the shared process receives the kill signal correctly on Windows.

Reproduction steps

  1. Open Teleport Connect.
  2. Open a few local terminal tabs.
  3. Open Activity Monitor and then go to View -> All Processes, Hierarchically.
  4. Locate Teleport Connect. There should be Teleport Connect Helper process with those shell processes under it.
    process group
  5. Double-click on one of those processes to open a dialog window with its details, then close Connect.

You can see that the parent process is now null and the process did not get terminated. If it was terminated, the dialog window title would say so.

Before closing Connect After closing Connect
before after
@ravicious ravicious added bug teleport-connect Issues related to Teleport Connect. labels May 30, 2023
@ravicious
Copy link
Member Author

ravicious commented Jun 7, 2023

Not sure how relevant this is for Windows, but for Unix we could the PID range approach https://azimi.me/2014/12/31/kill-child_process-node-js.html

But we should probably just make sure that the shared process properly cleans everything before exiting. Since the shared process and the main process are both Node.js processes, we could use the same approach that the agent cleanup daemon uses for a cross-platform way to properly dispose of all processes.

However, this would only address a case where the shared process gets orphaned, we should still make sure that those processes are properly cleaned up during normal shutdown.


Also, I'm not sure if we need any special handling for processes started from the shell processes. Like what if a background process is started from one of the shells. Or what if the user attempts to quit the app while one of the shells has a program running in the foreground which is busy with some work.

iTerm usually displays a prompt asking if you really want to quit the app. I suppose for now we could just kill any processes?

@ravicious
Copy link
Member Author

On macOS, this is causing problems if you run the app straight from the .dmg. If you open a terminal, then close the app and try to eject the .dmg, you're going to see:

The disk "Teleport Connect 14.0.1-universal" wasn't ejected because one or more programs may be using it.
To eject the disk immediately, click the Force Eject button.

@ravicious
Copy link
Member Author

This might not be relevant anymore (see #39821 (comment)), but it'd be good to establish how exactly this got addressed.

As I said in that comment, my bet is on an Electron update, but I don't see anything relevant in patch notes.

@ravicious
Copy link
Member Author

I just verified that Electron does not leave orphaned processes on all three platforms. I'm adding a check for this to the test plan and to the update checklist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug teleport-connect Issues related to Teleport Connect.
Projects
None yet
Development

No branches or pull requests

1 participant