feat: single-instance app via QLocalServer IPC#108
Merged
Conversation
New app/single_instance.py exposes: - _socket_name() — stable per-user, sha256(username)-prefixed - send_to_existing(paths) — second-invocation connector - SingleInstanceServer — listener QObject with new_paths signal Length-prefixed UTF-8 wire format with 64 KB cap. Stale socket cleanup via QLocalServer.removeServer on bind. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
main() now tries send_to_existing() before creating QApplication. If the call succeeds, the second invocation exits with code 0, leaving the running instance to load the PDFs. If no instance is running, falls through to normal startup. A throw-away QCoreApplication drives the QLocalSocket event loop during the probe; it is torn down before QApplication is constructed to satisfy Qt's single-app-singleton invariant. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
MainWindow.__init__ instantiates SingleInstanceServer and connects its new_paths signal to a handler that loads each PDF as a new tab, brings the window to the front (raise_/activateWindow/showNormal), and bypasses the second-instance Explorer flow entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Seven tests covering: - _socket_name() determinism and per-user hash naming - send_to_existing() returns False gracefully when no server is up (skips cleanly if a dev PDFApps instance is running) - module uses QtNetwork primitives - payload length is capped (DoS guard) - pdfapps.main() calls send_to_existing BEFORE QApplication(sys.argv) - MainWindow wires SingleInstanceServer + raises/activates on event Behavioural end-to-end tests would require subprocess + QLocalServer choreography that is fragile in CI; manual QA covers the integration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-11 review caught that `del _probe_app` does NOT destroy the Qt singleton — only releases the Python reference. The next `QApplication(sys.argv)` then raised `RuntimeError: Please destroy the QCoreApplication singleton before creating a new QApplication instance.` when the user double-clicked a PDF in Explorer with PDFApps not already running (the most common second-invocation scenario where send_to_existing returns False and we fall through to normal startup). QLocalSocket.waitForConnected/waitForBytesWritten work standalone without any Qt application instance, so the probe is unnecessary — removed entirely. Also from the same review: - Use NUL (\0) as the path separator instead of newline. Paths on macOS/Linux can legitimately contain newlines, which the previous \n-joined payload silently split into multiple bogus paths. NUL is illegal on every mainstream filesystem. - Add regression test pinning that main() never reintroduces a QCoreApplication probe. - Add coverage tests for the _read_payload guards (oversized header, zero-length header, non-UTF-8 payload) and for the NUL separator on both sender and receiver sides. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the user opens a PDF from Explorer/Finder while PDFApps is already running, the second invocation now forwards the path(s) to the running instance (which loads them as new tabs and raises its window), instead of spawning a duplicate process.
Closes R8 H4/H5 — the highest-value architectural gap from the audit roadmap.
Architecture
app/single_instance.py (new, ~170 LOC):
_socket_name()— stable per-user,sha256(getuser)[:12]-prefixed name to avoid collisions on shared machines.send_to_existing(paths)— second-invocation connector. Length-prefixed UTF-8 wire format (4-byte big-endian header + payload), 64 KB cap, NUL-delimited paths. Returns True on successful delivery (caller exits).SingleInstanceServer— QObject listening on QLocalServer, emitsnew_pathssignal. Stale-socket cleanup viaQLocalServer.removeServeron bind. Defensive defaults: fail-open on bind failure (warn + continue), graceful UTF-8 decode failures, sanity-capped payloads.pdfapps.py:main: tries
send_to_existing()BEFORE creating QApplication. If hit, exits cleanly. If miss, falls through to normal startup. No QCoreApplication probe needed — QLocalSocket works standalone.app/window.py:
MainWindow.__init__instantiatesSingleInstanceServer(self)and connectsnew_pathsto_on_second_instance, which validates each path (defensive re-check), calls_load_and_track, and brings the window to the front (showNormalif minimized +raise_+activateWindow— cross-platform).Security
os.path.isfile + .lower().endswith('.pdf')— never trusts the peer.Adversarial review
The review caught a critical bug in the original implementation:
del _probe_appdoes NOT destroy the Qt singleton, only releases the Python reference. The subsequentQApplication(sys.argv)raised RuntimeError when the user double-clicked a PDF in Explorer with PDFApps not already running — i.e., the most common second-invocation scenario crashed cold-start.Fix (commit
261abef): removed the probe entirely. QLocalSocket works without any Qt application instance (verified). Also tightened with NUL separator + 3 new tests for_read_payloadguards (oversized header, zero-length header, non-UTF-8 payload) + 1 regression test asserting the probe is absent.Tests
tests/test_single_instance.py— 12 tests, all passing:send_to_existingreturns False without server)_read_payloadEnd-to-end (spawn 2 processes) intentionally omitted — fragile in CI. Manual QA at install time confirms integration.
Validation
APP_VERSIONbumpPySide6.QtNetworkalready bundled)Out of scope