fix: ensure wsproxy MultiAgent is closed when websocket dies
#11414
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In this stack: This stack of pull requests is managed by Graphite. Learn more about stacking. |
50c92b4 to
12f1878
Compare
spikecurtis
requested changes
Jan 5, 2024
| <-ctx.Done() | ||
| ma.Close() | ||
| }() | ||
|
|
||
| go func() { | ||
| defer cancel() | ||
| dec := json.NewDecoder(nc) |
There was a problem hiding this comment.
[Re: lines 488 to 488]
I think it's worth dropping an INFO log here.
See this comment inline on Graphite.
I think it's worth dropping an INFO log here.
See this comment inline on Graphite.
| @@ -472,6 +472,11 @@ func (c *Client) DialCoordinator(ctx context.Context) (agpl.MultiAgentConn, erro | |||
| OnRemove: func(agpl.Queue) { conn.Close(websocket.StatusGoingAway, "closed") }, | |||
| }).Init() | |||
|
|
|||
| go func() { | |||
| <-ctx.Done() | |||
There was a problem hiding this comment.
If I'm understanding this correctly, we're depending on the fact that the reader goroutine below cancels the context on a failed read.
I think we should also tear down the multi-agent on a failed write of subscription messages. It's unlikely that we'd have a failure that leaves the connection half-open (e.g. for reads but not writes), but such things are possible and you don't want the proxy limping on unable to subscribe to new agents.
| func HeartbeatClose(ctx context.Context, exit func(), conn *websocket.Conn) { | ||
| ticker := time.NewTicker(30 * time.Second) | ||
| ticker := time.NewTicker(15 * time.Second) | ||
| defer ticker.Stop() | ||
|
|
||
| for { |
There was a problem hiding this comment.
The `SingleTailnet` behavior only checked to see if the `MultiAgent` was closed, but the websocket error was not being propogated into the `MultiAgent`, causing it to never be swapped for a new working one.
spikecurtis
approved these changes
Jan 11, 2024
12f1878 to
72ad811
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
SingleTailnetbehavior only checked to see if theMultiAgentwasclosed, but the websocket error was not being propogated into the
MultiAgent, causing it to never be swapped for a new working one.Fixes #11401
Before:
After: