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

@@ -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.

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()

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: