Capture the error if the gateway CLI client executable is not found

[ravicious]

What doesn't work and why

#26441 makes it so that we spawn the gateway CLI client directly. Up until this point however, Connect never had to worry about spawning a nonexistent program. We'd spawn either the default shell or tsh shipped with Connect, so we could assume that those programs are always present on the device running Connect.

With #26441 however, if you try to click Run and you don't have the necessary CLI client installed, the process will fail to spawn and Connect will simply show an empty tab with no error message.

Why does this happen? The call which throws an error is nodePTY.spawn which is called from here:

teleport/web/packages/teleterm/src/sharedProcess/ptyHost/ptyProcess.ts

Lines 49 to 50 in 6bb0ee5

	start(cols: number, rows: number) {
	this._process = nodePTY.spawn(this.options.path, this.options.args, {

PtyProcess.start itself is an event sent by the client (the frontend app). It's not an unary call, meaning that there's no way to really return an error here.

How this PR fixes this

We add a new server event called PtyEventStartError. It is sent to the client if the call to nodePty.spawn fails. The client then listens for this event and if it happens, it shows an error message instead of the terminal.

---

I tried to improve what I could, particularly the DocumentTerminal/Terminal component, but ultimately I spent way too much time on this than I should've.

Tested on macOS, Windows 11 and Ubuntu.

[ravicious]

I made it so that all on(Event) functions return cleanup functions, even though we call the cleanup functions only for a few of them.

---

I know that we discussed returning cleanup functions under the cleanup key of an object, but I'm still not sure about this approach. In my opinion it makes the function definition a little bit more noisy. Besides, what else would an onEvent function return? Usually they return void, so having a convention where they instead return a cleanup function instead makes sense to me.

[ravicious]

Refactored this in a proper enum so that it's easier to use it as the type in addListenerAndReturnRemovalFunction.

[ravicious]

This follows the approach explained in Resetting all state when a prop changes from You Might Not Need an Effect.

[ravicious]

An example of why not checking the status before accessing attempt.data is dangerous.

When the run callback from useAsync is called again, status goes from success to processing. However, attempt.data does not get cleanup up!

This is useful in some cases, for example if you want to show stale state while loading fresh state. But the downside to it is that if you don't account for attempt.status, you might end up using stale values.

---

Here when I was initially working on this PR, I couldn't understand why we call stuff on the stale ptyProcess. Turns out this line was causing this.

[ravicious]

Removed this dispose from here. ctrl.destroy() is called from the cleanup function in the useEffect in Terminal. useDocumentTerminal already calls ptyProcess.dispose on unmounting.

What's more, useDocumentTerminal is the place which creates the ptyProcess, so it feels that it should also be responsible for disposing it. Similar to how in Go, the goroutine that owns a channel should also be responsible for closing the channel.

[ravicious]

Why handle this in Terminal rather than in DocumentTerminal which already handles errors related to initializing the PTY process?

Terminal is the component responsible for calling start on the PTY process. I tried to have this logic in DocumentTerminal at first, but I ran into some weird edge cases when reattempting the call to initialize a PTY process.

If the logic is contained in Terminal, then we know that we're going to call start only once for each PTY and that when a new PTY is created, Terminal is going to have its state reset.

[ravicious]

I tried to refactor the names a little bit to be more clear as to what is happening. useDocumentTerminal doesn't actually start the terminal session, it makes the RPC to intantiate a new PtyProcess in the shared process:

teleport/web/packages/teleterm/src/sharedProcess/ptyHost/ptyHostService.ts

Lines 31 to 41 in 3588d98

	createPtyProcess: (call, callback) => {
	const ptyOptions = call.request.toObject();
	const ptyId = unique();
	try {
	const ptyProcess = new PtyProcess({
	...ptyOptions,
	ptyId,
	args: ptyOptions.argsList,
	env: call.request.getEnv()?.toJavaScript() as Record<string, string>,
	});
	ptyProcesses.set(ptyId, ptyProcess);

The start of the process, which actually spawns the process, is done from ctrl.open.

[ravicious]

Fortunately, this doesn't apply during normal usage of the app in non-dev mode.

[gzdunek]

LGTM, but could you rebase the PR and remove the WIP commit?

[ravicious]

LGTM, but could you rebase the PR and remove the WIP commit?

Oh, so that's how people leak secrets by mistake. 🤔 I didn't leak anything this time but I have no idea how this commit got there.

I also forgot to rebase this after the base branch got merged, I'll do so in a sec.