Commit a2c028e
authored
fix: simplify URI handling when the same deployment URL is already opened (#227)
Netflix reported that only seems to reproduce on Linux (we've only
tested Ubuntu so far).
I can’t reproduce it on macOS. First, here’s some context:
1. Polling workspaces:
Coder Toolbox polls the deployment every 5 seconds for workspace
updates.
These updates (new workspaces, deletions,status changes) are stored in a
cached “environments” list (an oversimplified explanation). When a URI
is executed,
we reset the content of the list and run the login sequence, which
re-initializes
the HTTP poller and CLI using the new deployment URL and token. A new
polling loop
then begins populating the environments list again.
2. Cache monitoring:
Toolbox watches this cached list for changes—especially status changes,
which determine
when an SSH connection can be established.
In Netflix’s case, they launched Toolbox, created a workspace from the
Dashboard, and the
poller added it to the environments list. When the workspace switched
from starting to ready,
they used a URI to connect to it. The URI reset the list, then the
poller repopulated it. But
because the list had the same IDs (but new object references), Toolbox
didn’t detect any changes.
As a result, it never triggered the SSH connection. This issue only
reproduces on Linux, but it
might explain some of the sporadic macOS failures Atif mentioned in the
past.
I need to dig deeper into the Toolbox bytecode to determine whether this
is a Toolbox bug, but
it does seem like Toolbox wasn’t designed to switch cleanly between
multiple deployments and/or users.
The current Coder plugin behavior—always performing a full login
sequence on every URI—is also ...sub-optimal.
It only really makes sense in these scenarios:
1. Toolbox started with deployment A, but the URI targets deployment B.
2. Toolbox started with deployment A/user X, but the URI targets
deployment A/user Y.
But this design is inefficient for the most common case: connecting via
URI to a workspace on the
same deployment and same user. While working on the fix, I realized that
scenario (2) is not realistic.
On the same host machine, why would multiple users log into the same
deployment via Toolbox? The whole
fix revolves around the idea of just recreating the http client and
updating the CLI with the new token
instead of going through the full authentication steps when the URI
deployment URL is the same as the
currently opened URL
The fix focuses on simply recreating the HTTP client and updating the
CLI token when the URI URL matches the existing deployment URL, instead
of running a full login.
This PR splits responsibilities more cleanly:
- CoderProtocolHandler now only finds the workspace and agent and
handles IDE installation and launch.
- the logic for creating a new HTTP client, updating the CLI, cleaning
up old resources (polling loop, environment cache), and handling
deployment URL changes is separated out.
The benefits would be:
- shared logic for cleanup and re-initialization, with less coupling and
clearer, more maintainable code.
- a clean way to check whether the URI’s deployment URL matches the
current one and react appropriately when they differ.1 parent 874e8cc commit a2c028e
File tree
9 files changed
+182
-163
lines changed- src
- main/kotlin/com/coder/toolbox
- util
- views
- test/kotlin/com/coder/toolbox/util
9 files changed
+182
-163
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
5 | 13 | | |
6 | 14 | | |
7 | 15 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
Lines changed: 4 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
276 | 276 | | |
277 | 277 | | |
278 | 278 | | |
279 | | - | |
280 | | - | |
281 | | - | |
282 | | - | |
| 279 | + | |
| 280 | + | |
283 | 281 | | |
284 | 282 | | |
285 | 283 | | |
| |||
312 | 310 | | |
313 | 311 | | |
314 | 312 | | |
315 | | - | |
316 | | - | |
317 | | - | |
318 | | - | |
| 313 | + | |
| 314 | + | |
319 | 315 | | |
320 | 316 | | |
321 | 317 | | |
| |||
Lines changed: 127 additions & 29 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
8 | 9 | | |
9 | 10 | | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
10 | 19 | | |
11 | 20 | | |
12 | 21 | | |
| |||
37 | 46 | | |
38 | 47 | | |
39 | 48 | | |
| 49 | + | |
| 50 | + | |
40 | 51 | | |
41 | 52 | | |
42 | 53 | | |
43 | 54 | | |
44 | 55 | | |
45 | 56 | | |
46 | 57 | | |
| 58 | + | |
47 | 59 | | |
48 | 60 | | |
49 | 61 | | |
| |||
61 | 73 | | |
62 | 74 | | |
63 | 75 | | |
| 76 | + | |
64 | 77 | | |
65 | 78 | | |
66 | 79 | | |
67 | 80 | | |
68 | 81 | | |
| 82 | + | |
69 | 83 | | |
70 | 84 | | |
71 | 85 | | |
| |||
82 | 96 | | |
83 | 97 | | |
84 | 98 | | |
85 | | - | |
| 99 | + | |
86 | 100 | | |
87 | 101 | | |
88 | 102 | | |
| |||
254 | 268 | | |
255 | 269 | | |
256 | 270 | | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
257 | 282 | | |
258 | 283 | | |
259 | 284 | | |
| |||
262 | 287 | | |
263 | 288 | | |
264 | 289 | | |
265 | | - | |
266 | | - | |
267 | | - | |
268 | | - | |
269 | | - | |
270 | | - | |
271 | 290 | | |
272 | 291 | | |
273 | 292 | | |
| |||
331 | 350 | | |
332 | 351 | | |
333 | 352 | | |
334 | | - | |
335 | | - | |
336 | | - | |
337 | | - | |
338 | | - | |
339 | | - | |
340 | | - | |
341 | | - | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
342 | 377 | | |
343 | | - | |
344 | | - | |
345 | | - | |
346 | | - | |
347 | | - | |
348 | | - | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
349 | 387 | | |
350 | | - | |
351 | | - | |
352 | | - | |
353 | | - | |
354 | 388 | | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
355 | 396 | | |
356 | 397 | | |
357 | 398 | | |
| |||
363 | 404 | | |
364 | 405 | | |
365 | 406 | | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
366 | 424 | | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
367 | 464 | | |
368 | 465 | | |
369 | 466 | | |
| |||
373 | 470 | | |
374 | 471 | | |
375 | 472 | | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
376 | 476 | | |
377 | 477 | | |
378 | 478 | | |
| |||
420 | 520 | | |
421 | 521 | | |
422 | 522 | | |
| 523 | + | |
423 | 524 | | |
424 | 525 | | |
425 | 526 | | |
| |||
428 | 529 | | |
429 | 530 | | |
430 | 531 | | |
431 | | - | |
432 | | - | |
433 | | - | |
434 | | - | |
| 532 | + | |
435 | 533 | | |
436 | 534 | | |
437 | 535 | | |
| |||
0 commit comments