-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(desktop): cleanup stale packaged backend processes on startup #1479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||
| from __future__ import annotations | ||||||||||||||
|
|
||||||||||||||
| import os | ||||||||||||||
| import signal | ||||||||||||||
| import socket | ||||||||||||||
| import subprocess | ||||||||||||||
| import sys | ||||||||||||||
|
|
@@ -79,6 +80,194 @@ def _stream_reader(in_stream, out_stream) -> None: | |||||||||||||
| pass | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _pid_exists(pid: int) -> bool: | ||||||||||||||
| """Return True if *pid* exists and is accessible.""" | ||||||||||||||
| try: | ||||||||||||||
| os.kill(pid, 0) | ||||||||||||||
| except ProcessLookupError: | ||||||||||||||
| return False | ||||||||||||||
| except PermissionError: | ||||||||||||||
| return True | ||||||||||||||
| return True | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _is_packaged_desktop_backend(cmd: str) -> bool: | ||||||||||||||
| """Return True when a process command matches packaged desktop backend.""" | ||||||||||||||
| return ( | ||||||||||||||
| ".app/Contents/Resources/env/bin/python" in cmd | ||||||||||||||
| and "-m uvicorn" in cmd | ||||||||||||||
| and "copaw.app._app:app" in cmd | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _list_stale_desktop_backend_pids() -> list[int]: | ||||||||||||||
| """Collect stale packaged desktop backend process IDs.""" | ||||||||||||||
| try: | ||||||||||||||
| ps_out = subprocess.check_output( | ||||||||||||||
| ["ps", "-axo", "pid=,command="], | ||||||||||||||
| text=True, | ||||||||||||||
| ) | ||||||||||||||
| except Exception: | ||||||||||||||
| return [] | ||||||||||||||
|
|
||||||||||||||
| current_pid = os.getpid() | ||||||||||||||
| stale_pids: list[int] = [] | ||||||||||||||
| for raw in ps_out.splitlines(): | ||||||||||||||
| line = raw.strip() | ||||||||||||||
| if not line: | ||||||||||||||
| continue | ||||||||||||||
| parts = line.split(None, 1) | ||||||||||||||
| if len(parts) != 2: | ||||||||||||||
| continue | ||||||||||||||
| pid_str, cmd = parts | ||||||||||||||
| try: | ||||||||||||||
| pid = int(pid_str) | ||||||||||||||
| except ValueError: | ||||||||||||||
| continue | ||||||||||||||
| if pid == current_pid: | ||||||||||||||
| continue | ||||||||||||||
| if _is_packaged_desktop_backend(cmd): | ||||||||||||||
| stale_pids.append(pid) | ||||||||||||||
|
|
||||||||||||||
| return stale_pids | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _terminate_processes(pids: list[int]) -> list[int]: | ||||||||||||||
| """Send SIGTERM to processes and return the ones signaled successfully.""" | ||||||||||||||
| cleaned: list[int] = [] | ||||||||||||||
| for pid in pids: | ||||||||||||||
| try: | ||||||||||||||
| os.kill(pid, signal.SIGTERM) | ||||||||||||||
| cleaned.append(pid) | ||||||||||||||
| except ProcessLookupError: | ||||||||||||||
| continue | ||||||||||||||
| except Exception: | ||||||||||||||
| continue | ||||||||||||||
|
Comment on lines
+142
to
+145
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Suggested change
|
||||||||||||||
| return cleaned | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _kill_surviving_processes(pids: list[int]) -> None: | ||||||||||||||
| """Force kill any process that remains alive after the grace period.""" | ||||||||||||||
| for pid in pids: | ||||||||||||||
| if _pid_exists(pid): | ||||||||||||||
| try: | ||||||||||||||
| os.kill(pid, signal.SIGKILL) | ||||||||||||||
| except Exception: | ||||||||||||||
| pass | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _cleanup_stale_desktop_backends() -> list[int]: | ||||||||||||||
| """Terminate stale packaged desktop backend processes. | ||||||||||||||
|
|
||||||||||||||
| We only target packaged app backends (inside ``*.app`` bundle) | ||||||||||||||
| to avoid affecting source/development processes like | ||||||||||||||
| ``python -m copaw app --reload``. | ||||||||||||||
| """ | ||||||||||||||
| if sys.platform == "win32": | ||||||||||||||
| # Packaged desktop backend process pattern below is | ||||||||||||||
| # macOS/Linux-specific. | ||||||||||||||
| return [] | ||||||||||||||
|
|
||||||||||||||
| stale_pids = _list_stale_desktop_backend_pids() | ||||||||||||||
| cleaned = _terminate_processes(stale_pids) | ||||||||||||||
|
|
||||||||||||||
| if cleaned: | ||||||||||||||
| # Give processes a short grace period for graceful shutdown. | ||||||||||||||
| deadline = time.monotonic() + 2.0 | ||||||||||||||
| while time.monotonic() < deadline: | ||||||||||||||
| if all(not _pid_exists(pid) for pid in cleaned): | ||||||||||||||
| break | ||||||||||||||
| time.sleep(0.1) | ||||||||||||||
|
|
||||||||||||||
| _kill_surviving_processes(cleaned) | ||||||||||||||
|
|
||||||||||||||
| return cleaned | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _log_ssl_certificate_status(env: dict[str, str]) -> None: | ||||||||||||||
| """Log SSL certificate configuration for desktop launches.""" | ||||||||||||||
| if "SSL_CERT_FILE" in env: | ||||||||||||||
| cert_file = env["SSL_CERT_FILE"] | ||||||||||||||
| if os.path.exists(cert_file): | ||||||||||||||
| _log_desktop(f"[desktop] SSL certificate: {cert_file}") | ||||||||||||||
| else: | ||||||||||||||
| _log_desktop( | ||||||||||||||
| f"[desktop] WARNING: SSL_CERT_FILE set but not found: " | ||||||||||||||
| f"{cert_file}", | ||||||||||||||
| ) | ||||||||||||||
| return | ||||||||||||||
|
|
||||||||||||||
| _log_desktop("[desktop] WARNING: SSL_CERT_FILE not set") | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _start_windows_stream_threads(proc: subprocess.Popen) -> None: | ||||||||||||||
| """Start background threads draining subprocess output on Windows.""" | ||||||||||||||
| stdout_thread = threading.Thread( | ||||||||||||||
| target=_stream_reader, | ||||||||||||||
| args=(proc.stdout, sys.stdout), | ||||||||||||||
| daemon=True, | ||||||||||||||
| ) | ||||||||||||||
| stderr_thread = threading.Thread( | ||||||||||||||
| target=_stream_reader, | ||||||||||||||
| args=(proc.stderr, sys.stderr), | ||||||||||||||
| daemon=True, | ||||||||||||||
| ) | ||||||||||||||
| stdout_thread.start() | ||||||||||||||
| stderr_thread.start() | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _serve_desktop_window( | ||||||||||||||
| proc: subprocess.Popen, | ||||||||||||||
| host: str, | ||||||||||||||
| port: int, | ||||||||||||||
| url: str, | ||||||||||||||
| ) -> None: | ||||||||||||||
| """Wait for backend readiness and run the blocking desktop webview.""" | ||||||||||||||
| webview_module = webview | ||||||||||||||
| if webview_module is None: | ||||||||||||||
| raise click.ClickException( | ||||||||||||||
| "pywebview is required to run CoPaw desktop mode", | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| _log_desktop("[desktop] Waiting for HTTP ready...") | ||||||||||||||
| if _wait_for_http(host, port): | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] HTTP ready, creating webview window...", | ||||||||||||||
| ) | ||||||||||||||
| api = WebViewAPI() | ||||||||||||||
| webview_module.create_window( | ||||||||||||||
| "CoPaw Desktop", | ||||||||||||||
| url, | ||||||||||||||
| width=1280, | ||||||||||||||
| height=800, | ||||||||||||||
| text_select=True, | ||||||||||||||
| js_api=api, | ||||||||||||||
| ) | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] Calling webview.start() (blocks until closed)...", | ||||||||||||||
| ) | ||||||||||||||
| webview_module.start( | ||||||||||||||
| private_mode=False, | ||||||||||||||
| ) | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] webview.start() returned (window closed).", | ||||||||||||||
| ) | ||||||||||||||
| proc.terminate() | ||||||||||||||
| proc.wait() | ||||||||||||||
| return | ||||||||||||||
|
|
||||||||||||||
| _log_desktop("[desktop] Server did not become ready in time.") | ||||||||||||||
| click.echo( | ||||||||||||||
| "Server did not become ready in time; open manually: " + url, | ||||||||||||||
| err=True, | ||||||||||||||
| ) | ||||||||||||||
| try: | ||||||||||||||
| proc.wait() | ||||||||||||||
| except KeyboardInterrupt: | ||||||||||||||
| proc.terminate() | ||||||||||||||
| proc.wait() | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @click.command("desktop") | ||||||||||||||
| @click.option( | ||||||||||||||
| "--host", | ||||||||||||||
|
|
@@ -107,25 +296,20 @@ def desktop_cmd( | |||||||||||||
| window without conflicting with an existing CoPaw app instance. | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| cleaned = _cleanup_stale_desktop_backends() | ||||||||||||||
| if cleaned: | ||||||||||||||
| _log_desktop( | ||||||||||||||
| f"[desktop] Cleaned stale desktop backend process(es): {cleaned}", | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| port = _find_free_port(host) | ||||||||||||||
| url = f"http://{host}:{port}" | ||||||||||||||
| click.echo(f"Starting CoPaw app on {url} (port {port})") | ||||||||||||||
| _log_desktop("[desktop] Server subprocess starting...") | ||||||||||||||
|
|
||||||||||||||
| env = os.environ.copy() | ||||||||||||||
| env[LOG_LEVEL_ENV] = log_level | ||||||||||||||
|
|
||||||||||||||
| if "SSL_CERT_FILE" in env: | ||||||||||||||
| cert_file = env["SSL_CERT_FILE"] | ||||||||||||||
| if os.path.exists(cert_file): | ||||||||||||||
| _log_desktop(f"[desktop] SSL certificate: {cert_file}") | ||||||||||||||
| else: | ||||||||||||||
| _log_desktop( | ||||||||||||||
| f"[desktop] WARNING: SSL_CERT_FILE set but not found: " | ||||||||||||||
| f"{cert_file}", | ||||||||||||||
| ) | ||||||||||||||
| else: | ||||||||||||||
| _log_desktop("[desktop] WARNING: SSL_CERT_FILE not set") | ||||||||||||||
| _log_ssl_certificate_status(env) | ||||||||||||||
|
|
||||||||||||||
| is_windows = sys.platform == "win32" | ||||||||||||||
| try: | ||||||||||||||
|
|
@@ -150,55 +334,8 @@ def desktop_cmd( | |||||||||||||
| universal_newlines=True, | ||||||||||||||
| ) as proc: | ||||||||||||||
| if is_windows: | ||||||||||||||
| stdout_thread = threading.Thread( | ||||||||||||||
| target=_stream_reader, | ||||||||||||||
| args=(proc.stdout, sys.stdout), | ||||||||||||||
| daemon=True, | ||||||||||||||
| ) | ||||||||||||||
| stderr_thread = threading.Thread( | ||||||||||||||
| target=_stream_reader, | ||||||||||||||
| args=(proc.stderr, sys.stderr), | ||||||||||||||
| daemon=True, | ||||||||||||||
| ) | ||||||||||||||
| stdout_thread.start() | ||||||||||||||
| stderr_thread.start() | ||||||||||||||
| _log_desktop("[desktop] Waiting for HTTP ready...") | ||||||||||||||
| if _wait_for_http(host, port): | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] HTTP ready, creating webview window...", | ||||||||||||||
| ) | ||||||||||||||
| api = WebViewAPI() | ||||||||||||||
| webview.create_window( | ||||||||||||||
| "CoPaw Desktop", | ||||||||||||||
| url, | ||||||||||||||
| width=1280, | ||||||||||||||
| height=800, | ||||||||||||||
| text_select=True, | ||||||||||||||
| js_api=api, | ||||||||||||||
| ) | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] Calling webview.start() " | ||||||||||||||
| "(blocks until closed)...", | ||||||||||||||
| ) | ||||||||||||||
| webview.start( | ||||||||||||||
| private_mode=False, | ||||||||||||||
| ) # blocks until user closes the window | ||||||||||||||
| _log_desktop( | ||||||||||||||
| "[desktop] webview.start() returned (window closed).", | ||||||||||||||
| ) | ||||||||||||||
| proc.terminate() | ||||||||||||||
| proc.wait() | ||||||||||||||
| return # normal exit after user closed window | ||||||||||||||
| _log_desktop("[desktop] Server did not become ready in time.") | ||||||||||||||
| click.echo( | ||||||||||||||
| "Server did not become ready in time; open manually: " + url, | ||||||||||||||
| err=True, | ||||||||||||||
| ) | ||||||||||||||
| try: | ||||||||||||||
| proc.wait() | ||||||||||||||
| except KeyboardInterrupt: | ||||||||||||||
| proc.terminate() | ||||||||||||||
| proc.wait() | ||||||||||||||
| _start_windows_stream_threads(proc) | ||||||||||||||
| _serve_desktop_window(proc, host, port, url) | ||||||||||||||
|
|
||||||||||||||
| if proc.returncode != 0: | ||||||||||||||
| sys.exit(proc.returncode or 1) | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
except Exception:is too broad and can hide unexpected errors. It's better to catch specific exceptions thatsubprocess.check_outputis known to raise, such assubprocess.CalledProcessErrorandFileNotFoundError. Also, consider logging the error for easier debugging, which will be helpful if process scanning fails for some reason.