From b9f96866e50436e49c2aacc5b33eb01b83fc2a43 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Thu, 1 Oct 2026 22:37:12 +1000 Subject: [PATCH] Replace Use now with a plain Reconnect, and ignore superseded tests Three review rounds kept finding ways Use now could point at an address a reconnect wouldn't pick: the page was predicting the outcome of the server's race from test results that could be unfinished, from another network, or out of date after a reorder or a Tailscale change. Adding the offered LAN address first in the list is what makes it win; Use now only hurried that along. The dialog's Retry button now also shows while connected, as Reconnect: it tries the addresses again in their current order and promises nothing about which answers first. The test results go back to plain rows. The review also found a test started earlier could overwrite a newer one still running, and mark it done. Each headset's tests now have a generation; only the newest one publishes. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/devices.md | 10 +++++----- tests/test_link.py | 32 ++++++++++++++++++++++++-------- ui/frame_link.py | 15 ++++++--------- ui/index.html | 16 ++++++---------- 4 files changed, 41 insertions(+), 32 deletions(-) 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 f2d92f2..3246e28 100644 --- a/ui/index.html +++ b/ui/index.html @@ -4337,8 +4337,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 || "—"], @@ -4404,13 +4408,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 `
@@ -4424,7 +4423,6 @@ function renderConnAddrs() { return `
${esc(a.host)} ${esc(KIND_TAG[a.kind] || a.kind)}${a.label ? ` ${esc(a.label)}` : ""}
${result}
- ${a.host === switchTo ? `` : ""}
`; @@ -4441,8 +4439,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);