-
Notifications
You must be signed in to change notification settings - Fork 4
fix: don't create new environment instances every time we poll #239
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
Conversation
The polling logic has a list of "environments" which is updated by the poll loop. The loop retrieves a list of workspaces and agents from the backend, and then it looks if the pairs are already part of the existing environments. This is where we have an issue: we look by temporarily creating a new "RemoteEnvironment" and use the id of the temporary env. to search in the existing collection. Besides creating a lot of temporary objects every 5 seconds, the logic has another flaw: the constructor of the RemoteEnvironment triggers the ssh connection for that workspace if it was marked in the settings store as connected in the previous state. Asking to establish the ssh connection every 5 seconds is completely unnecessary, from my testings the newer versions of Toolbox are able to remember in the same session that a connection was opened, and it needs to be re-established again if it fails during that session. From now on the constructor for the RemoteEnvironment will request it only for new login sessions, from then on it is up to TBX to remember the connection state. In the early versions we added custom logic to retrigger the ssh connection when API keys expired. Turns out that is also no longer necessary to do. When an API key expires the user will have to go again through the login sequence, which means starting with a clean sheet of "environments". At the first poll run the RemoteEnvironment constructor will request TBX to establish the ssh connection again.
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.
Pull request overview
This PR fixes a performance issue where RemoteEnvironment instances were being unnecessarily created every 5 seconds during the polling loop. The previous implementation created temporary RemoteEnvironment objects to check if workspace-agent pairs already existed, which inadvertently triggered SSH connection attempts. The refactored code now uses direct ID comparison to find existing environments, eliminating temporary object creation and excessive SSH connection requests.
Key changes:
- Replaced temporary object creation with direct ID string comparison in the polling logic
- Removed the
WorkspaceConnectionManagerclass and all related SSH reconnection logic - Changed
startSshConnection()from returning a boolean to a Unit function with updated logging
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/com/coder/toolbox/WorkspaceConnectionManager.kt | Removed entire file - connection state management is no longer needed |
| src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt | Refactored polling logic to use ID-based lookup instead of creating temporary environments; removed WorkspaceConnectionManager integration |
| src/main/kotlin/com/coder/toolbox/CoderRemoteEnvironment.kt | Updated startSshConnection() to void return type and improved logging messages |
| gradle.properties | Version bump from 0.8.1 to 0.8.2 |
| CHANGELOG.md | Added entry documenting the fix for excessive SSH connection requests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The polling logic has a list of "environments" which is updated by the poll loop. The loop retrieves a list of workspaces and agents from the backend, and then it looks if the pairs are already part of the existing environments. This is where we have an issue: we look by temporarily creating a new "RemoteEnvironment" and use the id of the temporary env. to search in the existing collection. Besides creating a lot of temporary objects every 5 seconds, the logic has another flaw: the constructor of the RemoteEnvironment triggers the ssh connection for that workspace if it was marked in the settings store as connected in the previous state. Asking to establish the ssh connection every 5 seconds is completely unnecessary, from my testings the newer versions of Toolbox are able to remember in the same session that a connection was opened, and it needs to be re-established again if it fails during that session.
From now on the constructor for the RemoteEnvironment will request it only for new login sessions, from then on it is up to TBX to remember the connection state. In the early versions we added custom logic to retrigger the ssh connection when API keys expired. Turns out that is also no longer necessary to do. When an API key expires the user will have to go again through the login sequence, which means starting with a clean sheet of "environments". At the first poll run the RemoteEnvironment constructor will request TBX to establish the ssh connection again.