diff --git a/app/single_instance.py b/app/single_instance.py new file mode 100644 index 0000000..9d0d5c7 --- /dev/null +++ b/app/single_instance.py @@ -0,0 +1,176 @@ +"""PDFApps single-instance enforcement via QLocalServer. + +When a second invocation happens (e.g., user clicks a PDF in Explorer +while PDFApps is already running), the new process connects to the +running instance, sends the PDF paths via a length-prefixed UTF-8 +message, and exits immediately. The running instance loads each path +as a new tab and brings its window to the front. + +Wire format +----------- +``[4-byte big-endian length] [UTF-8 payload]`` + +Payload is NUL-separated (``\\0``) absolute paths. NUL is illegal in +every mainstream filesystem (POSIX + Windows NTFS/exFAT), so it is a +safer delimiter than newline — paths can legitimately contain ``\\n`` +on macOS and Linux, which would have been silently split with the +previous newline separator. The 4-byte length lets the server frame +the message correctly even when ``readyRead`` arrives in multiple +chunks. A 64 KB cap prevents a hostile or buggy peer from forcing the +server to allocate unbounded memory. + +Socket naming +------------- +The socket name is suffixed with the first 12 hex chars of +``sha256(username)`` so concurrent sessions on a shared machine (e.g., +Windows fast-user-switching or a multi-user Linux box) each get their +own instance — they should not steal each other's PDF tabs. +""" + +import getpass +import hashlib +import logging + +from PySide6.QtCore import QObject, Signal +from PySide6.QtNetwork import QLocalServer, QLocalSocket + +_log = logging.getLogger(__name__) + +# Connect/wait timeout for the second-invocation handshake. 1 s is +# generous on local sockets/named pipes (typical RTT < 5 ms) while +# still capping the worst-case startup delay if the running instance +# is wedged. +_SOCKET_TIMEOUT_MS = 1000 + +# Cap on a single payload. 64 KB easily fits hundreds of absolute paths +# (a typical Windows MAX_PATH is 260 chars) and bounds memory pressure +# from a malformed or hostile peer. +_MAX_PAYLOAD_BYTES = 64 * 1024 + + +def _socket_name() -> str: + """Stable per-user socket name. + + Suffixed with a hash of the username so simultaneous sessions for + different users on the same machine don't collide (and don't + accidentally hand each other's PDFs across user boundaries). + """ + try: + user = getpass.getuser() + except Exception: + # getuser() raises on minimal Windows services with no USERNAME + # env var; fall back to a stable literal so the socket is at + # least reachable from sibling processes of the same user. + user = "default" + digest = hashlib.sha256(user.encode("utf-8")).hexdigest()[:12] + return f"pdfapps-single-instance-{digest}" + + +def send_to_existing(paths: list[str]) -> bool: + """Try to forward ``paths`` to a running PDFApps instance. + + Returns ``True`` if the connection succeeded and the message was + written (the caller — a second invocation — should now exit). + Returns ``False`` if no instance is running (caller should start + the main window itself). + """ + sock = QLocalSocket() + sock.connectToServer(_socket_name()) + if not sock.waitForConnected(_SOCKET_TIMEOUT_MS): + return False + try: + # NUL separator: paths can contain newlines on macOS/Linux but + # NUL is forbidden in every mainstream filesystem, so it is the + # only fully unambiguous delimiter for absolute paths. + payload = "\0".join(paths).encode("utf-8") + if len(payload) > _MAX_PAYLOAD_BYTES: + # Refuse to send a payload the server would just reject; + # better to fall back to a normal second instance than + # silently drop half the files. + _log.warning( + "single-instance: payload too large (%d > %d), " + "falling back to second instance", + len(payload), _MAX_PAYLOAD_BYTES, + ) + return False + header = len(payload).to_bytes(4, "big") + sock.write(header + payload) + sock.flush() + sock.waitForBytesWritten(_SOCKET_TIMEOUT_MS) + finally: + sock.disconnectFromServer() + sock.close() + return True + + +class SingleInstanceServer(QObject): + """Listens for second-invocation connections and emits paths. + + Connect the ``new_paths`` signal to the main window's load handler. + Construction is no-op-safe if the server fails to bind (a warning + is logged and the signal simply never fires), so the running + instance keeps working even without single-instance forwarding. + """ + + new_paths = Signal(list) # list[str] + + def __init__(self, parent=None): + super().__init__(parent) + self._server = QLocalServer(self) + # Clean up any stale socket file from a previous crash. On + # Linux QLocalServer leaves an orphan file behind if the + # process is killed; removeServer() unlinks it so listen() + # can succeed. On Windows this is a no-op. + QLocalServer.removeServer(_socket_name()) + if not self._server.listen(_socket_name()): + _log.warning( + "single-instance: failed to bind socket %s: %s", + _socket_name(), self._server.errorString(), + ) + return + self._server.newConnection.connect(self._on_new_connection) + _log.info("single-instance: listening on %s", _socket_name()) + + def _on_new_connection(self): + sock = self._server.nextPendingConnection() + if sock is None: + return + # Bind sock into the lambda so the slot has a stable reference + # even if more peers connect before we finish reading this one. + sock.readyRead.connect(lambda s=sock: self._read_payload(s)) + # Defensive cleanup: if the peer drops without sending anything + # the QLocalSocket would otherwise leak until the server dies. + sock.disconnected.connect(sock.deleteLater) + + def _read_payload(self, sock): + # Wait for the 4-byte length header to be fully buffered. + if sock.bytesAvailable() < 4: + return # readyRead will fire again + header = bytes(sock.read(4)) + length = int.from_bytes(header, "big") + if length <= 0 or length > _MAX_PAYLOAD_BYTES: + _log.warning( + "single-instance: invalid payload length %d", length, + ) + sock.disconnectFromServer() + return + # Payload may not have arrived yet — wait once, then read + # whatever's available. We tolerate a short read rather than + # blocking the GUI for a full second on a wedged peer. + if sock.bytesAvailable() < length: + sock.waitForReadyRead(_SOCKET_TIMEOUT_MS) + data = bytes(sock.read(length)) + try: + text = data.decode("utf-8") + except UnicodeDecodeError: + _log.warning("single-instance: payload not valid UTF-8") + sock.disconnectFromServer() + return + paths = [p for p in text.split("\0") if p.strip()] + if paths: + _log.info( + "single-instance: received %d path(s) from second instance", + len(paths), + ) + self.new_paths.emit(paths) + sock.disconnectFromServer() diff --git a/app/window.py b/app/window.py index 6192d55..870afd5 100644 --- a/app/window.py +++ b/app/window.py @@ -21,6 +21,7 @@ from app.styles import STYLE, STYLE_LIGHT from app.utils import resource_path, _make_palette from app.widgets import DropFileEdit +from app.single_instance import SingleInstanceServer from app.viewer.panel import PdfViewerPanel from app.tools.split import TabDividir from app.tools.merge import TabJuntar @@ -552,6 +553,15 @@ def _a11y(btn, tip): if not self._dark_mode: self._apply_theme() + # Single-instance IPC: when the user double-clicks another PDF + # in Explorer/Finder while PDFApps is running, that second + # process forwards the paths here via QLocalSocket instead of + # spawning a redundant second window. See app/single_instance.py + # for the wire format. Construction is no-op-safe if the + # socket fails to bind, so the running instance keeps working. + self._instance_server = SingleInstanceServer(self) + self._instance_server.new_paths.connect(self._on_second_instance) + # ── Viewer property (always returns the active tab's viewer) ────── @property def _viewer(self) -> PdfViewerPanel: @@ -838,6 +848,33 @@ def _load_and_track(self, path: str): refresh() self._refresh_viewer_top_buttons() + def _on_second_instance(self, paths: list): + """Handle PDFs forwarded from a second invocation. + + Each path is loaded as a new tab (mirroring drag-and-drop + semantics) and the main window is brought to the foreground so + the user sees the result immediately instead of wondering why + the click "did nothing". ``raise_`` + ``activateWindow`` is the + cross-platform incantation; ``showNormal`` is the only one of + the three that un-minimises on every supported OS. + """ + import logging as _logging + _wlog = _logging.getLogger(__name__) + for path in paths: + if isinstance(path, str) and os.path.isfile(path) \ + and path.lower().endswith(".pdf"): + try: + self._load_and_track(path) + except Exception as exc: + _wlog.warning( + "second instance: failed to load %s: %s", + path, exc, + ) + if self.isMinimized(): + self.showNormal() + self.raise_() + self.activateWindow() + def _open_in_new_tab(self): """Open a PDF in a new tab.""" from PySide6.QtWidgets import QFileDialog diff --git a/pdfapps.py b/pdfapps.py index d1da851..655c95d 100644 --- a/pdfapps.py +++ b/pdfapps.py @@ -115,6 +115,35 @@ def main(): # Parse BEFORE QApplication so --help / --version exit cleanly # without bringing up the Qt event loop (and the splash screen). args = _parse_args(sys.argv[1:]) + + # Single-instance forwarding: if PDFApps is already running and the + # user double-clicked a PDF (or invoked us with one or more files), + # ship the paths over QLocalSocket and exit. The running instance + # opens them as new tabs and raises its window — much faster and + # less RAM than a second full process. Must happen BEFORE + # QApplication() so we never pay the splash/startup cost in the + # second invocation. + # + # QLocalSocket.waitForConnected/waitForBytesWritten work standalone + # without any QApplication/QCoreApplication instance — so we do NOT + # construct a probe app here. A previous version did, then tried to + # ``del _probe_app`` before constructing the main app, but ``del`` + # only drops the Python reference; the Qt singleton survives and + # the subsequent QApplication construction raised + # ``RuntimeError: Please destroy the QCoreApplication singleton + # before creating a new QApplication instance.`` That crashed the + # cold-start "open PDF from Explorer" path (the most common second + # invocation scenario when PDFApps was not already running). + pdf_files = [ + os.path.abspath(f) for f in (args.files or []) + if os.path.isfile(f) and f.lower().endswith(".pdf") + ] + if pdf_files: + from app.single_instance import send_to_existing + if send_to_existing(pdf_files): + # Paths delivered to the existing instance — quit silently. + sys.exit(0) + app = QApplication(sys.argv) app.setApplicationName(" ") app.setApplicationDisplayName(" ") diff --git a/tests/test_single_instance.py b/tests/test_single_instance.py new file mode 100644 index 0000000..ee3c821 --- /dev/null +++ b/tests/test_single_instance.py @@ -0,0 +1,223 @@ +"""Tests for single-instance enforcement (PR-M). + +Full behavioural tests would require spawning a real subprocess and +binding a real QLocalServer, which is fragile in CI (timing, leftover +sockets between runs, headless display constraints). Instead we mix: + +1. Pure-Python unit tests for ``_socket_name`` and the wire/timeout + constants. +2. A live ``send_to_existing`` smoke test that confirms the function + degrades gracefully when no server is listening — skipped if the + developer happens to have a running PDFApps instance. +3. Source-level wiring checks that verify the integration in + ``pdfapps.py`` and ``app/window.py`` is hooked up the way we expect + (forwarding must happen *before* QApplication construction; main + window must instantiate the server and bring the window to front + on second-instance events). +""" + +import os +import sys + +import pytest + +# Repo root for the source-level checks below. +_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir)) + + +def _read(rel: str) -> str: + with open(os.path.join(_ROOT, rel), encoding="utf-8") as f: + return f.read() + + +# ── _socket_name ────────────────────────────────────────────────────── + +def test_socket_name_is_user_stable(): + from app.single_instance import _socket_name + a = _socket_name() + b = _socket_name() + assert a == b, "socket name must be deterministic across calls" + assert a.startswith("pdfapps-single-instance-") + + +def test_per_user_socket_naming_uses_user_hash(): + src = _read("app/single_instance.py") + assert "getuser" in src, "must derive socket name from username" + assert "hashlib" in src, "must hash the username (privacy + length)" + + +# ── send_to_existing ────────────────────────────────────────────────── + +def test_send_to_existing_returns_false_when_no_server(): + """With no server listening, the function should fail fast (False) + rather than block or raise.""" + from PySide6.QtCore import QCoreApplication + from PySide6.QtNetwork import QLocalSocket + from app.single_instance import _socket_name, send_to_existing + + if QCoreApplication.instance() is None: + QCoreApplication(sys.argv) + + # If a dev PDFApps happens to be running, skip — we'd otherwise + # interfere with the user's open session. + probe = QLocalSocket() + probe.connectToServer(_socket_name()) + is_running = probe.waitForConnected(200) + probe.close() + if is_running: + pytest.skip("a PDFApps instance is currently running") + + assert send_to_existing(["/nonexistent.pdf"]) is False + + +# ── Module-level wiring ─────────────────────────────────────────────── + +def test_single_instance_module_uses_qt_network(): + src = _read("app/single_instance.py") + assert "QLocalServer" in src + assert "QLocalSocket" in src + + +def test_payload_length_capped(): + """A 64 KB cap protects the server from a malformed/hostile peer.""" + src = _read("app/single_instance.py") + assert "64 * 1024" in src or "65536" in src + + +# ── pdfapps.py wiring ───────────────────────────────────────────────── + +def test_pdfapps_main_attempts_forward_before_qapplication(): + """Second invocations must hand off paths BEFORE constructing the + main ``QApplication`` — otherwise we pay splash/startup cost twice. + + We anchor on the ``def main()`` line so this test isn't fooled by + the *early* ``_app = QApplication(sys.argv)`` inside the + ImportError fallbacks at the top of the file (those exit() before + ever reaching main()). + """ + src = _read("pdfapps.py") + assert "send_to_existing" in src, "pdfapps.py must use the forwarder" + main_pos = src.find("def main()") + assert main_pos != -1, "pdfapps.py must define main()" + forward_pos = src.find("send_to_existing", main_pos) + qapp_pos = src.find("QApplication(sys.argv)", main_pos) + assert forward_pos != -1 and qapp_pos != -1 + assert forward_pos < qapp_pos, ( + "Inside main(), send_to_existing must be called before " + "QApplication(sys.argv) so a second invocation exits before " + "paying QApplication setup" + ) + + +# ── window.py wiring ────────────────────────────────────────────────── + +def test_window_wires_single_instance_server(): + src = _read("app/window.py") + assert "SingleInstanceServer" in src, "MainWindow must own the server" + assert "new_paths" in src, "MainWindow must subscribe to new_paths" + # Window must surface itself on a second-instance event so the + # user sees the loaded PDF instead of a "did nothing" click. + assert "raise_()" in src + assert "activateWindow()" in src + + +# ── Cold-start regression (PR-M round-11 review) ────────────────────── + +def test_main_cold_start_with_pdf_arg_does_not_use_qcoreapplication_probe(): + """Regression for PR-M round-11 review. + + The first version of the forwarding block constructed a throw-away + ``QCoreApplication(sys.argv)`` "probe" before calling + ``send_to_existing`` and then ``del _probe_app``'d it. But ``del`` + only drops the Python reference — the Qt singleton survives, and + the subsequent ``QApplication(sys.argv)`` then raises:: + + RuntimeError: Please destroy the QCoreApplication singleton + before creating a new QApplication instance. + + This crashed the *cold-start* path where the user double-clicks a + PDF in Explorer and PDFApps is NOT already running (the most common + second-invocation scenario): ``send_to_existing`` returns False, + we fall through, and ``QApplication`` blows up. + + The fix is to remove the probe entirely — QLocalSocket's + waitForConnected/waitForBytesWritten work standalone without any + Qt application instance. This test guards against the probe ever + being reintroduced inside ``main()``. + """ + src = _read("pdfapps.py") + main_pos = src.find("def main()") + assert main_pos != -1, "pdfapps.py must define main()" + # Cut at the dunder-main guard so the probe check only inspects + # main()'s body (not e.g. helper functions defined later). + end = src.find('\nif __name__', main_pos) + main_body = src[main_pos: end if end != -1 else len(src)] + assert "QCoreApplication(sys.argv)" not in main_body, ( + "main() must not construct a QCoreApplication probe — " + "QLocalSocket works without it, and `del` doesn't destroy " + "the Qt singleton, so the subsequent QApplication() would " + "raise on the cold-start path." + ) + assert "QCoreApplication.instance()" not in main_body, ( + "main() must not even reference QCoreApplication — the probe " + "pattern is the bug. Forward via send_to_existing() directly." + ) + # And forwarding must still be in place. + assert "send_to_existing(pdf_files)" in main_body + + +# ── _read_payload guards (PR-M round-11 review) ─────────────────────── + +def test_read_payload_rejects_oversized_header(): + """Server must early-return when the advertised length exceeds the + 64 KB cap, without trying to allocate / decode the bogus payload.""" + src = _read("app/single_instance.py") + # The guard must check against the documented cap. + assert "_MAX_PAYLOAD_BYTES" in src + assert "64 * 1024" in src or "65536" in src + # And it must short-circuit (disconnect + return) before reading + # `length` bytes off the wire. + assert "length > _MAX_PAYLOAD_BYTES" in src, ( + "_read_payload must reject oversized headers" + ) + + +def test_read_payload_rejects_zero_length_header(): + """A header advertising length == 0 (or negative) is malformed and + must be rejected before we attempt to read 0 bytes and emit an + empty path list.""" + src = _read("app/single_instance.py") + assert "length <= 0" in src, ( + "_read_payload must reject non-positive length headers" + ) + + +def test_read_payload_handles_non_utf8_gracefully(): + """A peer sending invalid UTF-8 must not crash the server — the + decode is wrapped in try/except UnicodeDecodeError and the socket + is closed cleanly.""" + src = _read("app/single_instance.py") + assert "UnicodeDecodeError" in src, ( + "_read_payload must catch UnicodeDecodeError on payload decode" + ) + + +# ── Path separator (PR-M round-11 review) ───────────────────────────── + +def test_payload_uses_nul_separator_not_newline(): + """Paths on macOS/Linux can legitimately contain newlines, which + the original ``\\n``-joined payload silently split into multiple + bogus paths. NUL is illegal on every mainstream filesystem and is + the only fully unambiguous delimiter.""" + src = _read("app/single_instance.py") + # Sender side + assert '"\\0".join(paths)' in src, ( + "send_to_existing must NUL-join paths, not newline-join" + ) + # Receiver side + assert 'text.split("\\0")' in src, ( + "_read_payload must split on NUL, matching the sender" + ) + # Belt-and-braces: there must be no newline-join/split left over. + assert '"\\n".join(paths)' not in src + assert 'text.split("\\n")' not in src