diff --git a/docs/devices.md b/docs/devices.md index d398f2c..6dc4d09 100644 --- a/docs/devices.md +++ b/docs/devices.md @@ -78,11 +78,11 @@ can add, edit, reorder and remove them without leaving the page you're on. When the headset reports a LAN IP on the same network as this computer and that IP isn't saved, it offers to add it. The offer puts the address first in the list, so on that network it wins over the Tailscale name; away from home the Tailscale -name still leads. A new or edited address is tested straight away. When the test -finishes and the first address that passed would be tried before the one in use, -that address gets **Use now**, which reconnects. A reconnect tries the addresses -in order and normally lands on it, but takes a later one if it's slow to answer -then. Otherwise the change takes effect the next time Frame Control connects. +name still leads. A new or edited address is tested straight away. While +connected, **Reconnect** tries the addresses again in their current order, so an +address just added or moved up is used now rather than at the next connection. +It doesn't pick a particular address: the first to answer in order wins, and a +slow one loses to a later one. ## Networks diff --git a/tests/test_link.py b/tests/test_link.py index 584b43e..d098a85 100644 --- a/tests/test_link.py +++ b/tests/test_link.py @@ -449,22 +449,38 @@ class Connecting(unittest.TestCase): d = self.device("::1", "127.0.0.1", "nothing.invalid") self.pin(d["id"]) self.hosts({"::1": "wrong", "127.0.0.1": "ok"}) - self.link.state["network"] = {"id": "n-home", "name": "Home", "tailscale": {"up": False}} self.link.test(d["id"]) rows = {r["host"]: r for r in self.link.snapshot()["tests"][d["id"]]["rows"]} self.assertEqual(rows["127.0.0.1"]["ssh"], "ok") self.assertEqual(rows["::1"]["ssh"], "wrong") self.assertEqual(rows["nothing.invalid"]["state"], "unresolved") - done = self.link.snapshot()["tests"][d["id"]] - self.assertTrue(done["done"]) - self.assertEqual(done["network"], "n-home") - # The address that passed has now worked on this network, so a reconnect tries it first: - # the page offers Use now from this order, which comes with the finished result. - self.assertEqual(done["order"][0], "127.0.0.1") - self.assertEqual(sorted(done["order"]), sorted(["::1", "127.0.0.1", "nothing.invalid"])) self.assertEqual(self.routes, []) self.assertTrue(all("ControlPath=none" in c for c in self.calls() if "-G" not in c)) + def test_a_test_started_earlier_cant_overwrite_a_newer_one(self): + d = self.device("nothing.invalid") + release, calls = threading.Event(), [] + + def probe(host, port, update=None): + calls.append(host) + if len(calls) == 1: + release.wait(10) # the first test is still probing when the second finishes + return {"state": "refused", "detail": "first test", "ip": None, "rtt_ms": None} + return {"state": "refused", "detail": "second test", "ip": None, "rtt_ms": None} + + with mock.patch.object(fl, "probe", probe): + first = threading.Thread(target=self.link.test, args=(d["id"],)) + first.start() + while not calls: + time.sleep(0.01) + self.link.test(d["id"]) + self.assertEqual(self.link.snapshot()["tests"][d["id"]]["rows"][0]["detail"], "second test") + release.set() + first.join(10) + result = self.link.snapshot()["tests"][d["id"]] + self.assertTrue(result["done"]) + self.assertEqual(result["rows"][0]["detail"], "second test") + def test_switching_to_a_headset_that_never_answers_stops_using_the_last_one(self): self.device("localhost") self.hosts({"localhost": "ok"}) diff --git a/ui/frame_link.py b/ui/frame_link.py index 09d11b9..aeb991b 100644 --- a/ui/frame_link.py +++ b/ui/frame_link.py @@ -169,6 +169,7 @@ class Link: self.version = 0 self.stopped = False self.kicks = [] # reasons someone asked for a (re)connect + self.test_gen = {} # device id -> its newest test of the addresses (see test()) self.busy = False # the loop is handling kicks self.state = {"phase": "idle", "reason": None, "device": None, "network": None, "stages": [], "probes": [], "via": None, "error": None, "retry_at": None, "attempt": 0, @@ -955,9 +956,13 @@ class Link: started = now() rows = [{"host": a["host"], "kind": a["kind"], "state": "waiting", "detail": "Waiting", "ip": None, "rtt_ms": None, "ssh": None} for a in device["addresses"]] + with self.cond: + gen = self.test_gen[device_id] = self.test_gen.get(device_id, 0) + 1 def put(**fields): with self.cond: + if gen != self.test_gen[device_id]: + return # a newer test has started: its results are the ones to show self.state["tests"][device_id] = dict({"started": started, "done": False, "rows": rows}, **fields) self.version += 1 self.cond.notify_all() @@ -1006,15 +1011,7 @@ class Link: t.start() for t in threads: t.join(40) - # The order a reconnect on this network would try them in, now that this test has - # recorded where they work: the page offers Use now only on the one it would pick. - try: - fresh = self.reg.get(device_id)["addresses"] - except frame_devices.DeviceError: - fresh = [] - order = [a["host"] for a, _ in frame_devices.order_addresses( - fresh, net.get("id"), bool((net.get("tailscale") or {}).get("up")))] - put(done=True, finished=now(), network=net.get("id"), order=order) + put(done=True, finished=now()) self.devices_changed() diff --git a/ui/index.html b/ui/index.html index 2fb244b..f4a708f 100644 --- a/ui/index.html +++ b/ui/index.html @@ -4472,8 +4472,12 @@ function renderConnDlg() { : `${s.reason || "Connecting"}…`; // Folded while connected; opened when something's wrong. Left as the user set it otherwise. if (s.phase !== cd.phase) { $("connMore").open = !connected; cd.phase = s.phase; } - $("connRetryBtn").hidden = !failed; + // Connected, it's Reconnect: the addresses are tried again in their current order, so an + // address just added or moved up gets its chance now rather than at the next connection. + $("connRetryBtn").hidden = !failed && !connected; $("connRetryBtn").classList.toggle("action", failed); + $("connRetryBtn").textContent = failed ? "Retry now" : "Reconnect"; + $("connRetryBtn").title = failed ? "" : "Try the addresses again, in order"; $("connSetup").hidden = connected; const n = s.network || {}, ts = n.tailscale || {}; const facts = [["Network", n.ssid || n.name || "—"], @@ -4539,13 +4543,8 @@ function renderConnAddrs() { // same address (after an edit, on that row's Edit button). const held = list.contains(document.activeElement) && document.activeElement.closest("[data-host]"); const refocus = cd.refocus ? [cd.refocus, "[data-ed]"] - : held ? [held.dataset.host, ["[data-use]", "[data-mv='-1']", "[data-mv='1']", "[data-ed]", "[data-rm]"].find(q => document.activeElement.matches(q))] : null; + : held ? [held.dataset.host, ["[data-mv='-1']", "[data-mv='1']", "[data-ed]", "[data-rm]"].find(q => document.activeElement.matches(q))] : null; cd.refocus = null; - // A reconnect tries the addresses in the order the finished test reports for this network - // and picks the first that answers: offer Use now on that one, if it isn't the one in use. - const test = (s.tests || {})[d.id], order = test && test.done && test.network === (s.network || {}).id && test.order; - const pick = order && order.find(h => (tested.get(h) || {}).ssh === "ok"); - const switchTo = pick && inUse && pick !== inUse && order.indexOf(inUse) > order.indexOf(pick) ? pick : null; list.innerHTML = d.addresses.map((a, i) => { if (cd.editing === a.host && cd.editDev === d.id) return `
@@ -4559,7 +4558,6 @@ function renderConnAddrs() { return `
${esc(a.host)} ${esc(KIND_TAG[a.kind] || a.kind)}${a.label ? ` ${esc(a.label)}` : ""}
${result}
- ${a.host === switchTo ? `` : ""}
`; @@ -4576,8 +4574,6 @@ function renderConnAddrs() { row.querySelector("[data-ed]").onclick = () => { cd.editing = host; cd.editDev = d.id; cd.session++; renderConnAddrs(); $("ceHost").focus(); }; - const use = row.querySelector("[data-use]"); - if (use) use.onclick = () => devAction(`Switch to ${host}`, { action: "retry", id: d.id }, use); row.querySelector("[data-rm]").onclick = e => { if (host === inUse && !confirm(`${host} is how Frame Control is reaching the headset now. Remove it?`)) return; devAction(`Remove ${host}`, { action: "address-remove", id: d.id, host }, e.currentTarget);