feat(rome_lsp): stop the daemon after the last client disconnects

[leops]

Summary

Fixes #3636

This change makes use of the existing session tracking code to schedule a background task once the last session is dropped from the server. After a short timeout (correctly set at 60 seconds), this task will broadcast the shutdown signal to the server if no new session was started in the meantime.

This behavior can be disabled by passing the --no-timeout flag to the __run_server internal command, this is done automatically by the rome start command to ensure manually started instances of the server never times out. Importantly, the timeout task is only scheduled when a client disconnects, meaning a newly started server instance will run indefinitely until at least one client connects event if it wasn't passed the --no-timeout flag (this should rarely happen if ever as the daemon spawning code tries to open a connection immediately afterward to ensure the newly created process is working correctly).

Test Plan

I tested this locally by configuring the VSCode extension to use a locally built binary from this branch, and checking the rome process correctly stops about one minute after exiting the editor.

[netlify]

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	d2ebdc5
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/636d0c33d657a800096e22bf
😎 Deploy Preview	https://deploy-preview-3643--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[MichaReiser]

The code changes look good to me but I recommend terminating Rome immediately because it aligns better with what users expect.

60s is too long to avoid the LSP being outdated after upgrading the extension. I understand that VS Code stops the LSP client when updating the extension but immediately tries to connect as soon as the update is done. This happens in a few milliseconds to low seconds.

The same is the case when the user triggers the "Restart Rome LSP" command from VS Code or does a "Reload Workspace" and probably even is if the user closes VS Code and re-opens it manually to see if the problem is now "fixed".

I see that this works if we ship the fix that ensures that Rome only connects to the same version but believe that not stopping Rome can still result in a frustrating experience if Rome is in an "invalid" state (for whatever reason). The intuition of most users is to restart VS Code if they run into this problem, which won't fix their problem with a timeout of 60s.

[leops]

The code changes look good to me but I recommend terminating Rome immediately because it aligns better with what users expect.

I could shorten the timeout to maybe 10 seconds, but a grace period is required before terminating the process since as I mentioned in the PR description, the CLI opens and close a connection after spawning the daemon to ensure it works correctly. If the server was to stop immediately then the actual client requesting the name of the socket from the Rome CLI (like the editor extension or JS API) would never be able to connect as the server would have been stopped before the rome __print_socket command exited when the initial test connection got dropped

[MichaReiser]

The code changes look good to me but I recommend terminating Rome immediately because it aligns better with what users expect.

I could shorten the timeout to maybe 10 seconds, but a grace period is required before terminating the process since as I mentioned in the PR description, the CLI opens and close a connection after spawning the daemon to ensure it works correctly. If the server was to stop immediately then the actual client requesting the name of the socket from the Rome CLI (like the editor extension or JS API) would never be able to connect as the server would have been stopped before the rome __print_socket command exited when the initial test connection got dropped

That fair but 10s is still a very long time but I see the risk that comes with reducing it to e.g. 1s or even lower.

the CLI opens and close a connection after spawning the daemon to ensure it works correctly

I can see why this is necessary for an existing socket but it should be fairly unlikely that a newly created socket isn't working (and there are good reasons that re-creating the socket again won't succeed as well).

Could we avoid connecting for newly created sockets instead? That still leaves the risk that the rome server terminates between ensureDeamon running and the LSP actually connecting but that's something that VS code should handle for us.

[leops]

I can see why this is necessary for an existing socket but it should be fairly unlikely that a newly created socket isn't working (and there are good reasons that re-creating the socket again won't succeed as well).

If I remember correctly the connection test is also important for newly spawned process as the socket will be initially unresponsive and cause IO errors if the server hasn't started yet, so some specific error handling code is required there to ensure the daemon is actually ready before handing control back to the client code.

[MichaReiser]

I think we should reduce the time out to 10s or even 5s.

Does the lsp-proxy command also shut down the deamon after the timeout?

A more complete solution could be to pass a flag in the initialize request that this is only a probing connection or or can we manually send the exit notification with a custom flag to NOT terminate the server.

[jeysal]

Not very familiar with this code so could be talking nonsense, but would it help to replace a no_timeout=bool with a timeout=u64|false (default value we can still decide) that can be set differently from CLI vs from an editor extension? Definitely not the cleanest solution if it even helps at all, but either way it seems like a good thing to have and a logical extension of the no_timeout flag, plus it avoids a no_* negated option to avoid confusing !no double negation constructs.

[leops]

Does the lsp-proxy command also shut down the deamon after the timeout?

The implementation of the command in start_lsp_proxy calls ensure_daemon with no_timeout: false so if the commands does spawn a daemon instance it would get stopped once the proxy disconnects.

A more complete solution could be to pass a flag in the initialize request that this is only a probing connection or or can we manually send the exit notification with a custom flag to NOT terminate the server.

The probing connection only opens a connection on the socket then immediately closes it without event sending the initialize or stop request, on the other hand that could be a mean of differentiating the probe connection by inhibiting the shutdown behavior before a session has been initialized at least once.

Not very familiar with this code so could be talking nonsense, but would it help to replace a no_timeout=bool with a timeout=u64|false (default value we can still decide) that can be set differently from CLI vs from an editor extension? Definitely not the cleanest solution if it even helps at all, but either way it seems like a good thing to have and a logical extension of the no_timeout flag, plus it avoids a no_* negated option to avoid confusing !no double negation constructs.

The --no-timeout flag is internal and only ever injected by the CLI as it spawns a the daemon server, so there wasn't really any use case customizing the timeout duration. I do agree the negation make the semantic more complicated than it needs, hence why I already negated it into has_timeout in the language server itself, but let me see if I can refactor it into something with --with-timeout instead

[MichaReiser]

I find the variable name is_oneshot not very intuitive. How about shutdown_on_disconnect or automatically_started

Can you update the README of the VS Code extension to remove the known limitation of an outdated rome server as this should no longer apply.