Skip to content

fix(tailnet): prevent redial after Coord graceful restart #15586

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

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Nov 19, 2024

fixes: coder/internal#217

There are a couple problems:

One is that we assert the RPCs succeed, but if the pipeDialer context is canceled at the end of the test, then these assertions happen after the test is officially complete, which panics and affects other tests.

This converts these to just return the error rather than assert.

The other is that the retrier is slightly bugged: if the current retry delay is 0 AND the ctx is done, (e.g. after successfully connecting and then gracefully disconnecting), then retrier.Wait(c.ctx) is racy and could return either true or false.

Fixes the phantom redial by explicitly checking the context before dialing. Also, in the test, we assert that the controller is closed before completing the test.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@spikecurtis spikecurtis force-pushed the spike/internal-217-redial branch from 7eaf0af to ba21ba8 Compare November 19, 2024 07:21
@spikecurtis spikecurtis marked this pull request as ready for review November 19, 2024 07:21
@spikecurtis spikecurtis merged commit 029cd5d into main Nov 19, 2024
30 checks passed
Copy link
Contributor Author

Merge activity

  • Nov 19, 2:37 AM EST: A user merged this pull request with Graphite.

@spikecurtis spikecurtis deleted the spike/internal-217-redial branch November 19, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flake: controllers_test.go
2 participants