From 2b71d414e40a2a2aa84ff1fc7db7f5bd64141000 Mon Sep 17 00:00:00 2001 From: kert Date: Fri, 11 Sep 2026 16:38:00 -0400 Subject: [PATCH] fix(prisma): vpn down destroys the droplet first and survives a prefs.js PermissionError; the SSH tunnel is a Popen child Python owns (no -f), killed on a failed start (refs #664) --- src/prisma/vpn.py | 99 ++++++++++++++++++++++++------- tests/prisma/test_vpn_deep.py | 84 ++++++++++++++++++++++++++ tests/prisma/test_vpn_exercise.py | 54 ++++++++++++++++- 3 files changed, 213 insertions(+), 24 deletions(-) diff --git a/src/prisma/vpn.py b/src/prisma/vpn.py index 511d26f..ca8d9f8 100644 --- a/src/prisma/vpn.py +++ b/src/prisma/vpn.py @@ -269,19 +269,33 @@ def up(region: str | None = None, *, attach_zotero: bool = True) -> dict: def down() -> dict: - """Tear down tunnel + sidecar + Zotero prefs + droplet + local state.""" + """Tear down droplet + tunnel + sidecar + Zotero prefs + local state. + + Order matters (#664): the droplet is the billable resource, so it is + destroyed FIRST, before anything that can fail locally. A local + failure (the container-owned ``prefs.js`` raising ``PermissionError`` + once left a droplet idling for two days) is logged, never allowed to + skip the destroy, the key delete or the state wipe. + """ + meta = json.loads(_DROPLET_JSON.read_text()) if _DROPLET_JSON.exists() else None + if meta is not None: + client = _do_client() + try: + client.droplets.destroy(droplet_id=meta["id"]) + except Exception as e: # noqa: BLE001 + log.warning("destroy failed (maybe already gone): %s", e) + if meta.get("ssh_key_id"): + try: + _delete_account_ssh_key(client, meta["ssh_key_id"]) + except Exception as e: # noqa: BLE001 + log.warning("ssh key delete failed: %s", e) _tunnel_stop() - detach_zotero_proxy() - if not _DROPLET_JSON.exists(): - return {"status": "nothing-to-do"} - meta = json.loads(_DROPLET_JSON.read_text()) - client = _do_client() try: - client.droplets.destroy(droplet_id=meta["id"]) - except Exception as e: # noqa: BLE001 - log.warning("destroy failed (maybe already gone): %s", e) - if meta.get("ssh_key_id"): - _delete_account_ssh_key(client, meta["ssh_key_id"]) + detach_zotero_proxy() + except Exception as e: # noqa: BLE001 — local cleanup must not stop the teardown + log.warning("zotero proxy detach failed (prefs/sidecar): %s", e) + if meta is None: + return {"status": "nothing-to-do"} for f in ( _DROPLET_JSON, _SSH_KEY, @@ -324,14 +338,23 @@ def _tunnel_running() -> bool: def _tunnel_start(public_ip: str) -> int: - """Open the SSH -L tunnel as a detached child; return PID.""" + """Open the SSH -L tunnel as a child this process owns; return PID. + + ``ssh -N`` in a new session via ``Popen`` (not ``-f``): with ``-f`` + ssh forks and the PID had to be rediscovered from the listening port, + which fails silently under an unprivileged ``ss`` — the fetch run then + raised after the tunnel was already up and never killed it (#664: a + port-forward outlived its fetch by two days). Owning the child means + the PID is known before the listener even appears, and a start that + never becomes ready is killed before the error propagates. + """ if _tunnel_running(): return int(_TUNNEL_PID.read_text().strip()) # Bound SSH known_hosts file to state dir so we don't pollute the # caller's ~/.ssh. Accept the host key on first connect. args = [ "ssh", - "-fN", + "-N", "-L", f"{_LOCAL_PORT}:127.0.0.1:{_REMOTE_PORT}", "-i", @@ -348,15 +371,33 @@ def _tunnel_start(public_ip: str) -> int: "ExitOnForwardFailure=yes", f"root@{public_ip}", ] - subprocess.run(args, check=True, capture_output=True) - # ssh -f backgrounds; find the child by port. - for _ in range(20): - pid = _pid_listening_on(_LOCAL_PORT) - if pid: - _TUNNEL_PID.write_text(str(pid)) - return pid + proc = subprocess.Popen( + args, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + start_new_session=True, + ) + _TUNNEL_PID.write_text(str(proc.pid)) + for _ in range(40): + if proc.poll() is not None: + _TUNNEL_PID.unlink(missing_ok=True) + raise RuntimeError(f"ssh -L exited early (code {proc.returncode})") + if _pid_listening_on(_LOCAL_PORT) or _port_open(_LOCAL_PORT): + return proc.pid time.sleep(0.25) - raise RuntimeError("SSH -L backgrounded but no listener appeared") + proc.kill() + _TUNNEL_PID.unlink(missing_ok=True) + raise RuntimeError("ssh -L started but no listener appeared") + + +def _port_open(port: int) -> bool: + """True when something accepts on 127.0.0.1:*port* (readiness check + that needs no ``ss`` privileges).""" + import socket + + with contextlib.closing(socket.socket()) as s: + s.settimeout(0.2) + return s.connect_ex(("127.0.0.1", port)) == 0 def _tunnel_stop() -> None: @@ -537,7 +578,21 @@ def _restore_zotero_prefs() -> None: if _ZOTERO_PREFS_BACKUP.is_file() and _ZOTERO_PREFS.is_file(): import shutil - shutil.copy2(_ZOTERO_PREFS_BACKUP, _ZOTERO_PREFS) + try: + shutil.copy2(_ZOTERO_PREFS_BACKUP, _ZOTERO_PREFS) + except PermissionError: + # prefs.js is owned by the container user (uid 100999 on the + # host); write it back the same way _rewrite_prefs does (#664). + subprocess.run( + ["sudo", "cp", str(_ZOTERO_PREFS_BACKUP), str(_ZOTERO_PREFS)], + check=True, + capture_output=True, + ) + subprocess.run( + ["sudo", "chown", "1000:1000", str(_ZOTERO_PREFS)], + check=False, + capture_output=True, + ) _ZOTERO_PREFS_BACKUP.unlink(missing_ok=True) elif _ZOTERO_PREFS.is_file(): # No backup — just strip our pref lines. diff --git a/tests/prisma/test_vpn_deep.py b/tests/prisma/test_vpn_deep.py index 22be32f..525287d 100644 --- a/tests/prisma/test_vpn_deep.py +++ b/tests/prisma/test_vpn_deep.py @@ -6,6 +6,7 @@ from unittest.mock import MagicMock, patch import pytest +import prisma.vpn as vpn from prisma.vpn import ( _cloud_init, _delete_account_ssh_key, @@ -109,3 +110,86 @@ class TestStatus: mock_json.exists.return_value = False result = status() assert result["status"] == "down" + + +class TestDownOrdering: + """#664: the droplet (billable) is destroyed first; a local failure in + the Zotero detach must not skip the destroy, the key delete or the + state wipe.""" + + def _state(self, tmp_path): + dj = tmp_path / "droplet.json" + dj.write_text( + '{"id": 565110236, "ssh_key_id": 55643846, "public_ip": "1.2.3.4"}' + ) + files = [ + tmp_path / n for n in ("key", "key.pub", "known_hosts", "env", "tunnel.pid") + ] + for f in files: + f.write_text("x") + return dj, files + + @patch("prisma.vpn._do_client") + @patch("prisma.vpn._delete_account_ssh_key") + @patch("prisma.vpn._tunnel_stop") + @patch("prisma.vpn.detach_zotero_proxy", side_effect=PermissionError("prefs.js")) + def test_detach_failure_does_not_skip_destroy( + self, mc_detach, mc_stop, mc_delkey, mc_client, tmp_path + ): + dj, files = self._state(tmp_path) + client = MagicMock() + mc_client.return_value = client + with ( + patch.object(vpn, "_DROPLET_JSON", dj), + patch.object(vpn, "_SSH_KEY", files[0]), + patch.object(vpn, "_KNOWN_HOSTS", files[2]), + patch.object(vpn, "_ENV_FILE", files[3]), + patch.object(vpn, "_TUNNEL_PID", files[4]), + ): + result = down() + assert result == {"status": "destroyed", "droplet_id": 565110236} + client.droplets.destroy.assert_called_once_with(droplet_id=565110236) + mc_delkey.assert_called_once_with(client, 55643846) + assert not dj.exists() and not any(f.exists() for f in files) + + @patch("prisma.vpn._do_client") + @patch("prisma.vpn._delete_account_ssh_key") + @patch("prisma.vpn._tunnel_stop") + @patch("prisma.vpn.detach_zotero_proxy") + def test_destroy_happens_before_local_cleanup( + self, mc_detach, mc_stop, mc_delkey, mc_client, tmp_path + ): + dj, files = self._state(tmp_path) + order: list[str] = [] + client = MagicMock() + client.droplets.destroy.side_effect = lambda **k: order.append("destroy") + mc_client.return_value = client + mc_stop.side_effect = lambda: order.append("tunnel") + mc_detach.side_effect = lambda: order.append("detach") + with ( + patch.object(vpn, "_DROPLET_JSON", dj), + patch.object(vpn, "_SSH_KEY", files[0]), + patch.object(vpn, "_KNOWN_HOSTS", files[2]), + patch.object(vpn, "_ENV_FILE", files[3]), + patch.object(vpn, "_TUNNEL_PID", files[4]), + ): + down() + assert order == ["destroy", "tunnel", "detach"] + + +class TestRestorePrefsPermission: + @patch("prisma.vpn.subprocess.run") + def test_falls_back_to_sudo_cp(self, mc_run, tmp_path): + backup = tmp_path / "prefs.js.bak" + prefs = tmp_path / "prefs.js" + backup.write_text("bak") + prefs.write_text("cur") + with ( + patch.object(vpn, "_ZOTERO_PREFS_BACKUP", backup), + patch.object(vpn, "_ZOTERO_PREFS", prefs), + patch("shutil.copy2", side_effect=PermissionError), + ): + vpn._restore_zotero_prefs() + cmds = [c.args[0][:2] for c in mc_run.call_args_list] + assert ["sudo", "cp"] in cmds + assert not backup.exists() diff --git a/tests/prisma/test_vpn_exercise.py b/tests/prisma/test_vpn_exercise.py index 5f8e198..fba0f28 100644 --- a/tests/prisma/test_vpn_exercise.py +++ b/tests/prisma/test_vpn_exercise.py @@ -133,10 +133,22 @@ class TestTunnelRunning: class TestTunnelStart: - @patch("prisma.vpn.subprocess.run") + """#664: Python owns the ssh child (Popen, no -f) — the PID is known + before the listener appears, and a start that never becomes ready or + exits early is killed/cleared instead of leaking a tunnel.""" + + def _proc(self, pid=5678, poll=None): + proc = MagicMock() + proc.pid = pid + proc.poll.return_value = poll + proc.returncode = poll + return proc + + @patch("prisma.vpn.subprocess.Popen") @patch("prisma.vpn._tunnel_running", return_value=False) @patch("prisma.vpn._pid_listening_on", return_value=5678) - def test_starts(self, mc_pid, mc_running, mc_sub, tmp_path): + def test_starts(self, mc_pid, mc_running, mc_popen, tmp_path): + mc_popen.return_value = self._proc() pf = tmp_path / "tunnel.pid" with ( patch.object(vpn, "_TUNNEL_PID", pf), @@ -146,6 +158,44 @@ class TestTunnelStart: pid = vpn._tunnel_start("1.2.3.4") assert pid == 5678 assert pf.read_text() == "5678" + args = mc_popen.call_args.args[0] + assert "-N" in args and "-fN" not in args and "-f" not in args + assert mc_popen.call_args.kwargs["start_new_session"] is True + + @patch("prisma.vpn.subprocess.Popen") + @patch("prisma.vpn._tunnel_running", return_value=False) + def test_early_exit_clears_pid_and_raises(self, mc_running, mc_popen, tmp_path): + mc_popen.return_value = self._proc(poll=255) + pf = tmp_path / "tunnel.pid" + with ( + patch.object(vpn, "_TUNNEL_PID", pf), + patch.object(vpn, "_SSH_KEY", tmp_path / "key"), + patch.object(vpn, "_KNOWN_HOSTS", tmp_path / "known_hosts"), + pytest.raises(RuntimeError, match="exited early"), + ): + vpn._tunnel_start("1.2.3.4") + assert not pf.exists() + + @patch("prisma.vpn.time.sleep") + @patch("prisma.vpn._port_open", return_value=False) + @patch("prisma.vpn._pid_listening_on", return_value=None) + @patch("prisma.vpn.subprocess.Popen") + @patch("prisma.vpn._tunnel_running", return_value=False) + def test_never_ready_kills_the_child( + self, mc_running, mc_popen, mc_pid, mc_open, mc_sleep, tmp_path + ): + proc = self._proc() + mc_popen.return_value = proc + pf = tmp_path / "tunnel.pid" + with ( + patch.object(vpn, "_TUNNEL_PID", pf), + patch.object(vpn, "_SSH_KEY", tmp_path / "key"), + patch.object(vpn, "_KNOWN_HOSTS", tmp_path / "known_hosts"), + pytest.raises(RuntimeError, match="no listener"), + ): + vpn._tunnel_start("1.2.3.4") + proc.kill.assert_called_once() + assert not pf.exists() class TestTunnelStop: