- Notifications
You must be signed in to change notification settings - Fork926
fix: cap max X11 forwarding ports and evict old#18561
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
base:main
Are you sure you want to change the base?
Conversation
spikecurtis commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
d6cba82 to77f7937CompareThere 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 a good refactor, nice work 👍🏻. I'm however not entirely sold on the lru eviction policy since it's based on connection establishment time and not connection usage (i.e. we may end evicting a busy connection). (See related comment for additional context.)
| // the X11 TCP listener port for a new X11 forwarding session. If we left the SSH session up, then graphical apps | ||
| // started in that session could potentially connect to an unintended X11 Server (i.e. the display on a different | ||
| // computer than the one that started the SSH session). Most likely, this session is a zombie anyway if we've | ||
| // reached the maximum number of X11 forwarding sessions. |
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.
Do we know of any cases where customers use up this many X11 forwarding ports? Another alternative would simply be to error out which might be preferable. I imagine a user could start one main session early, then play around with different behaviors and depleting the pool, only to have the main session evicted.
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.
#18263 has a description from the customer. He has like 15 sessions open at a time, gets disconnected, and reconnects all 15. Since we have long server-side timeouts, they end up accumulating.
This LRU thing is definitely a band-aid and we need to fix the fact that his SSH sessions don't recover and/or don't get cleaned up server side, but that's likely involved.
Erroring out is not preferrable in this case, since once the SSH sessions are orphaned on the server there is nothing you can do to force them down early other than restarting the workspace which is not acceptable.
Wecould instrument the bicopy to periodically bump the usage time of the session, so that "active" X11 forwarding would be less likely to be evicted, but I'm not sure it's worth it, given our understanding of the problem. My working hypothesis is that you're just never going to be creating over 200 X11 forwarding sessions on purpose unless you're getting repeatedly disconnected, in which case the early sessions are toast.
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.
Alright, thanks for the detailed explanation! I think your reasoning makes sense, and considering we have some ideas on how to improve this in the future if the need arises, I’m ok with the approach. I’ll go through the rest of the PRs tomorrow.
| return display, true | ||
| // cleanSession closes and cleans up the session. | ||
| func (x *x11Forwarder) cleanSession(x11session *x11Session) { |
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.
Suggestion:closeSession sounds a bit more informative to me than clean. I guess it's a matter of implicit close vs implicit clean, and which is the primary action.
| display int | ||
| err error | ||
| ) | ||
| // retry up to 10 times |
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.
Nit: Comment isn't informative and could be removed or describe why.
Uh oh!
There was an error while loading.Please reload this page.
partial for#18263
Caps the X11 forwarding sessions at a maximum port of 6200, and evicts the oldest session if we create new sessions while at the max.
Unit tests included higher in the stack.