From 49378b2c80f4be0d154be5f767aa17c3dc255654 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Tue, 29 Sep 2026 01:13:15 +1000 Subject: [PATCH] Devices: fixes from review round 24 - While the connector is taking a queued reconnect off its list, the old connection no longer counts as live, so no install starts on it. - Importing a block without a Port takes the port ssh would really use (ssh -F -G), e.g. one a later Host * sets. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_devices.py | 7 +++++++ tests/test_link.py | 6 ++++++ ui/frame_devices.py | 25 ++++++++++++++++++++++--- ui/frame_link.py | 4 ++-- 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/tests/test_devices.py b/tests/test_devices.py index baabfea..c256df1 100644 --- a/tests/test_devices.py +++ b/tests/test_devices.py @@ -124,6 +124,13 @@ class Migration(Base): self.assertEqual([d["alias"] for d in self.reg.devices()], ["frame-2", "frame"]) self.assertEqual(self.reg.active(), self.reg.by_alias("frame")["id"]) + @unittest.skipUnless(shutil.which("ssh"), "needs ssh") + def test_a_port_inherited_from_another_host_entry_is_kept(self): + (self.ssh / "config").write_text(CONFIG.replace("Host *\n ServerAliveInterval 60", "Host *\n Port 2222")) + self.reg.sync_from_config(seed=False) + self.assertEqual(self.reg.by_alias("frame")["port"], 2222) # what ssh itself would use + self.assertEqual(self.reg.by_alias("frame-2")["port"], 2222) # its own Port line + def test_setup_finding_a_new_address_adds_it(self): self.reg.sync_from_config(seed=False) (self.ssh / "config").write_text(CONFIG.replace("HostName frame.tail1234.ts.net", "HostName 192.168.1.237")) diff --git a/tests/test_link.py b/tests/test_link.py index 4d114cc..6566a00 100644 --- a/tests/test_link.py +++ b/tests/test_link.py @@ -328,6 +328,12 @@ class Connecting(unittest.TestCase): self.assertEqual(self.link.failed("login", ["steamos@frame: Permission denied (publickey)."], False, "frame"), "stop") + def test_a_reconnect_being_started_isnt_a_live_connection(self): + self.link.state["phase"] = "connected" + self.assertTrue(self.link.alive()) + self.link.busy = True # the loop took a Retry off the queue and is about to reconnect + self.assertFalse(self.link.alive()) # so ensure() waits instead of starting work on it + def test_probes_from_an_earlier_attempt_leave_the_new_rows_alone(self): self.link.state.update(attempt=2, probes=[{"host": "b", "state": "waiting"}]) self.link.probe_update(0, 1, state="answered", ip="10.0.0.2") diff --git a/ui/frame_devices.py b/ui/frame_devices.py index c02076d..fedd346 100644 --- a/ui/frame_devices.py +++ b/ui/frame_devices.py @@ -157,7 +157,8 @@ def parse_blocks(text): for line in text.splitlines(): m = BLOCK_RE.fullmatch(line.strip()) if m: - cur = {"alias": m.group(1), "hostname": None, "user": None, "port": 22, "identity_files": []} + cur = {"alias": m.group(1), "hostname": None, "user": None, "port": 22, "port_set": False, + "identity_files": []} continue if cur is None: continue @@ -174,7 +175,7 @@ def parse_blocks(text): elif key == "user" and cur["user"] is None: cur["user"] = value elif key == "port" and value.isdigit(): - cur["port"] = int(value) + cur["port"], cur["port_set"] = int(value), True elif key == "identityfile": cur["identity_files"].append(value) return blocks @@ -278,7 +279,9 @@ def rewrite_block(alias, path=None, hostname=None, user=None, port=None, expect= def change(lines): if expect: block = next((b for b in parse_blocks("\n".join(lines)) if b["alias"] == alias), None) - if not block or any(v is not None and block[k] != v for k, v in expect.items()): + # A port the block doesn't set is inherited from elsewhere in the file: not compared. + if not block or any(v is not None and block[k] != v and (k != "port" or block["port_set"]) + for k, v in expect.items()): return None return _rewritten(lines, alias, hostname, user, port) return _edit_config(Path(path or ssh_config()), change) @@ -321,6 +324,18 @@ def remove_block(alias, path=None): return _edit_config(Path(path or ssh_config()), change) +def effective_port(alias, config): + """The port ssh uses for ALIAS with this config file (`ssh -F FILE -G ALIAS`), else 22.""" + try: + out = subprocess.run(["ssh", "-F", str(config), "-G", alias], capture_output=True, text=True, + stdin=subprocess.DEVNULL, timeout=10).stdout + except (OSError, subprocess.TimeoutExpired): + return 22 + m = re.search(r"^port (\d+)$", out, re.M) + port = int(m.group(1)) if m else 22 + return port if 1 <= port <= 65535 else 22 + + # ---- pinned host keys ------------------------------------------------------------- def _keygen(*args): @@ -670,6 +685,10 @@ class Registry: """Import managed blocks we don't know yet, and pick up a HostName that Set Up Connection changed since we last looked. -> True if anything changed.""" blocks = parse_blocks(read_config(self.config)) + for b in blocks: + if not b["port_set"]: + # No Port in the block: another Host entry may give one (ssh uses the first). + b["port"] = effective_port(b["alias"], self.config or ssh_config()) changed = False with self.lock: first = not self.data["devices"] and not self.data.get("active") diff --git a/ui/frame_link.py b/ui/frame_link.py index 569f755..d8e0813 100644 --- a/ui/frame_link.py +++ b/ui/frame_link.py @@ -259,8 +259,8 @@ class Link: self.close_master() def alive(self): - if self.state["phase"] != "connected" or self.kicks: - return False + if self.state["phase"] != "connected" or self.kicks or self.busy: + return False # a reconnect is queued or starting: don't begin anything on this connection if not self.control: return True return self.master is None or self.master.poll() is None