-
Notifications
You must be signed in to change notification settings - Fork 35
Refactor SSH process monitoring to support VS Code forks #665
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
base: main
Are you sure you want to change the base?
Conversation
1797956 to
68fa375
Compare
| // Poll interval for SSH process and file discovery | ||
| pollInterval?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this one be exponential? Right now it will keep retrying every second forever if it cannot find the SSH process :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If made exponential, do put a ceiling in, like 30s.
| const stats = await fs.stat(networkInfoFile); | ||
| const ageMs = Date.now() - stats.mtime.getTime(); | ||
|
|
||
| if (ageMs > staleThreshold) { | ||
| // Prevent tight loop: if we just searched due to stale, wait before searching again | ||
| const timeSinceLastSearch = Date.now() - this.lastStaleSearchTime; | ||
| if (timeSinceLastSearch < staleThreshold) { | ||
| await this.delay(staleThreshold - timeSinceLastSearch); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the reconnection detection logic, the issue here is that we do not get events for disconnecting or connecting and thus we have no way to subscribe to them. So what I did here, was assume a connection is stale if the network stats file (generated by the CLI) does not change in poll * 5 seconds - If a network is stale and we find the same process then nothing would break so it's harmless, just means we have to search again.
dedicated SshProcessMonitor class. Add centralized Remote SSH extension detection to better support Cursor, Windsurf, and other VS Code forks. Key changes: - Extract SSH monitoring logic from remote.ts into sshProcess.ts - Add sshExtension.ts to detect installed Remote SSH extension - Use createRequire instead of private module._load API - Fix port detection to find most recent port (handles reconnects) - Add Cursor's "Socks port:" log format to port regex
e20c456 to
e7f92b2
Compare
mafredri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major issues spotted, nice refactor 👍🏻 (albeit a bit much TS for me today 😂)
| * When found, starts monitoring. | ||
| */ | ||
| private async searchForProcess(): Promise<void> { | ||
| const { pollInterval, logger, sshHost } = this.options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sshHost only seems to be used for logging, don't we need to filter anything based on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not no, the port is sufficient (this is based on the old logic that was refactored here). Using the hostname to search was an approach I tested out but it's worse because we can have the same host connected on multiple windows so there's no way to tell them apart...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove it from the options to simplify or still valuable as a log? Either way, up to you 👍🏻.
| // output_logging folder doesn't exist | ||
| } | ||
|
|
||
| return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor concern, if our trys always fail, we'll end up in a polling interval, never informing the user of anything (searchForProcess)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would get SSH process search attempt ${attempt} for host: ${sshHost}, do you mean that they won't know it failed at that process exactly? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's more like a potential danger that we never stop trying even if it's never going to match (maybe some expectation changes in the future, etc). And the user has to realize that even though it keeps logging that, it will never resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go through the different pollings and try to make sure we have sufficient logging (but not too much). So at least if there's a failure here the users know why it might have happened (in this case, it could be that the Remote - SSH log location was changed or somehow never written)
| // Poll interval for SSH process and file discovery | ||
| pollInterval?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If made exponential, do put a ceiling in, like 30s.
jakehwll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy from a Typescript-side of things 🙂 Once @mafredri's comments are resolved LGTM
mafredri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
| const lastMatch = allMatches.at(-1)!; | ||
| // Each capture group corresponds to a different Remote SSH extension log format: | ||
| // [0] full match, [1] and [2] ms-vscode-remote.remote-ssh, | ||
| // [3] windsurf/open-remote-ssh/antigravity, [4] anysphere.remote-ssh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| `; | ||
| expect(findPort(log)).toBe(3333); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks for adding it!
Extract SSH process discovery and network status display into a
dedicated
SshProcessMonitorclass. Add centralized Remote SSH extensiondetection to support Cursor, Windsurf, and other VS Code forks.
Key changes:
remote.tsintosshProcess.tssshExtension.tsto detect installed Remote SSH extensioncreateRequireinstead of privatemodule._loadAPI to load the Remote SSH extensionCloses #660