- Notifications
You must be signed in to change notification settings - Fork928
fix: stop waiting for Agent in a goroutine in ssh test#12268
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
| }) | ||
| // When the agent connects, the workspace was started, and we should | ||
| // have access to the shell. | ||
| _ = agenttest.New(t, client.URL, authToken) |
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.
Could consider adefer a.Close() here too, to speed up the agents tear-down, not sure whether or not it would help with the issue being fixed here though.
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 think it will amount to the same thing. There is already at.Cleanup() that closes it, which will run after the laterdefers in this function.
Uh oh!
There was an error while loading.Please reload this page.
Fixes race seen here:https://github.com/coder/coder/runs/21852483781
What happens is that the agent connects, completes the test, and then disconnects before the Eventually condition runs. The waiter then times out because it's looking for a connected agent.
Then, since it's a
requirein a goroutine, that causes thetGocleanup to hang and the whole test suite to timeout after 10 minutes.Anyway,
agenttest.Newdoesn't block, and we don't actually need to wait for the agent to connect, since a successful SSH session is evidence that it connected.