diff --git a/sqlit/cli.py b/sqlit/cli.py index 977205a4..f5b838f4 100644 --- a/sqlit/cli.py +++ b/sqlit/cli.py @@ -177,6 +177,32 @@ def _sane_tty() -> None: pass +def _prewarm_process_worker(runtime: RuntimeConfig) -> Any | None: + """Spawn the process worker before the Textual App is constructed. + + `multiprocessing.spawn` collects the parent's open file descriptors at + spawn time. Textual's `App.__init__` registers signal-handler pipes + and other non-inheritable FDs, which cause spawn to abort with + `ValueError: bad value(s) in fds_to_keep` (see issue #189). Spawning + here — before the App is constructed — is the latest point at which + the FD set is still clean. + + Returns the live client, or None if spawn fails or the worker is + disabled. On failure we fall through to the lazy path inside the UI; + on macOS that path raises and the in-process executor takes over. + """ + if not runtime.process_worker: + return None + if runtime.mock.enabled: + return None + try: + from sqlit.domains.process_worker.app.process_worker_client import ProcessWorkerClient + + return ProcessWorkerClient() + except Exception: + return None + + def _run_app(app: Any) -> int: exit_code: int | None = None handled_signals = [signal.SIGINT, signal.SIGTERM] @@ -843,10 +869,14 @@ def main() -> int: print(f"Error: {exc}") return 1 + # Spawn the worker before the Textual App is constructed; see + # _prewarm_process_worker for why this matters on macOS. + process_worker_client = _prewarm_process_worker(runtime) app = SSMSTUI( services=services, startup_connection=startup_config, exclusive_connection=exclusive_connection, + process_worker_client=process_worker_client, ) exit_code = _run_app(app) if exit_code != 0: @@ -907,7 +937,12 @@ def main() -> int: print(f"Error: {alert_error}") return 1 - app = SSMSTUI(services=services, startup_connection=temp_config) + process_worker_client = _prewarm_process_worker(runtime) + app = SSMSTUI( + services=services, + startup_connection=temp_config, + process_worker_client=process_worker_client, + ) return _run_app(app) if args.command in {"connections", "connection"}: diff --git a/sqlit/domains/process_worker/app/process_worker.py b/sqlit/domains/process_worker/app/process_worker.py index e1d55bfc..31008e7c 100644 --- a/sqlit/domains/process_worker/app/process_worker.py +++ b/sqlit/domains/process_worker/app/process_worker.py @@ -2,14 +2,39 @@ from __future__ import annotations +import faulthandler +import os import pickle +import sys +import tempfile import threading import time from collections import deque from dataclasses import dataclass, field from multiprocessing.connection import Connection +from pathlib import Path from typing import Any + +def _open_worker_log() -> Any | None: + """Open the worker log file, honoring SQLIT_WORKER_LOG when set. + + The parent process may attach this worker to a Textual-managed pipe; + libpq notice writes or unexpected stderr output would then SIGPIPE the + child. Redirecting both streams to a real file avoids that and gives a + place to land C-level fault tracebacks for future diagnosis. + """ + override = os.environ.get("SQLIT_WORKER_LOG") + if override: + path = Path(override).expanduser() + else: + path = Path(tempfile.gettempdir()) / "sqlit-worker.log" + try: + path.parent.mkdir(parents=True, exist_ok=True) + return path.open("a", buffering=1) + except OSError: + return None + from sqlit.domains.connections.domain.config import ConnectionConfig from sqlit.domains.connections.providers.catalog import get_provider from sqlit.domains.connections.providers.config_service import normalize_connection_config @@ -493,6 +518,14 @@ def _get_provider(self, db_type: str) -> Any | None: def run_process_worker(conn: Connection) -> None: """Process entrypoint for query execution.""" + log_file = _open_worker_log() + if log_file is not None: + sys.stdout = log_file + sys.stderr = log_file + try: + faulthandler.enable(file=log_file) + except (RuntimeError, ValueError): + pass state = _WorkerState(conn=conn) try: while True: diff --git a/sqlit/domains/process_worker/app/process_worker_client.py b/sqlit/domains/process_worker/app/process_worker_client.py index b1de75de..3b45e710 100644 --- a/sqlit/domains/process_worker/app/process_worker_client.py +++ b/sqlit/domains/process_worker/app/process_worker_client.py @@ -63,6 +63,14 @@ def _maybe_fallback_start(self, error: Exception) -> None: raise error if os.name != "posix" or sys.platform.startswith("win"): raise error + # macOS (especially Tahoe 26+) aborts in the child whenever a forked + # process touches CoreFoundation / Security.framework / os_log — + # which happens inside psycopg, getaddrinfo, and many other stdlib + # paths. Forking here causes hard segfaults (issue #189), so we + # surface the spawn failure and let the caller fall back to + # in-process execution. + if sys.platform == "darwin": + raise error try: self._start_with_context(get_context("fork")) except Exception: diff --git a/sqlit/domains/shell/app/main.py b/sqlit/domains/shell/app/main.py index aaffd636..e98c4c2c 100644 --- a/sqlit/domains/shell/app/main.py +++ b/sqlit/domains/shell/app/main.py @@ -100,9 +100,16 @@ def __init__( runtime: RuntimeConfig | None = None, startup_connection: ConnectionConfig | None = None, exclusive_connection: bool = False, + process_worker_client: Any | None = None, ): super().__init__() self.services = services or build_app_services(runtime or RuntimeConfig.from_env()) + # A pre-spawned worker handed in from cli.py, before Textual's + # FD set was contaminated. ProcessWorkerLifecycleMixin already + # checks self._process_worker_client first, so seeding it here + # short-circuits the lazy spawn path. + if process_worker_client is not None: + self._process_worker_client = process_worker_client from sqlit.core.connection_manager import ConnectionManager self._connection_manager = ConnectionManager(self.services) diff --git a/tests/unit/test_cli_prewarm.py b/tests/unit/test_cli_prewarm.py new file mode 100644 index 00000000..19040629 --- /dev/null +++ b/tests/unit/test_cli_prewarm.py @@ -0,0 +1,64 @@ +"""Tests for the pre-spawn helper in cli.py and the SSMSTUI kwarg plumbing.""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from sqlit.cli import _prewarm_process_worker +from sqlit.shared.app.runtime import MockConfig, RuntimeConfig + + +def test_prewarm_skipped_when_worker_disabled() -> None: + runtime = RuntimeConfig(process_worker=False) + with patch("sqlit.domains.process_worker.app.process_worker_client.ProcessWorkerClient") as klass: + result = _prewarm_process_worker(runtime) + assert result is None + klass.assert_not_called() + + +def test_prewarm_skipped_when_mock_enabled() -> None: + runtime = RuntimeConfig(process_worker=True, mock=MockConfig(enabled=True)) + with patch("sqlit.domains.process_worker.app.process_worker_client.ProcessWorkerClient") as klass: + result = _prewarm_process_worker(runtime) + assert result is None + klass.assert_not_called() + + +def test_prewarm_returns_client_when_enabled() -> None: + runtime = RuntimeConfig(process_worker=True) + sentinel = MagicMock(name="process-worker-client") + with patch( + "sqlit.domains.process_worker.app.process_worker_client.ProcessWorkerClient", + return_value=sentinel, + ): + result = _prewarm_process_worker(runtime) + assert result is sentinel + + +def test_prewarm_swallows_spawn_errors() -> None: + """A failing pre-spawn must not break startup; lazy path handles fallback.""" + runtime = RuntimeConfig(process_worker=True) + with patch( + "sqlit.domains.process_worker.app.process_worker_client.ProcessWorkerClient", + side_effect=ValueError("bad value(s) in fds_to_keep"), + ): + result = _prewarm_process_worker(runtime) + assert result is None + + +def test_ssmstui_stashes_prewarmed_worker() -> None: + """A client passed via the kwarg should land on self._process_worker_client. + + ProcessWorkerLifecycleMixin returns that attribute first, so this is + what makes the pre-spawn actually short-circuit the lazy path. + """ + from sqlit.domains.shell.app.main import SSMSTUI + from tests.ui.mocks import MockConnectionStore, MockSettingsStore, build_test_services, create_test_connection + + services = build_test_services( + connection_store=MockConnectionStore([create_test_connection("test-db", "sqlite")]), + settings_store=MockSettingsStore({}), + ) + sentinel = MagicMock(name="process-worker-client") + app = SSMSTUI(services=services, process_worker_client=sentinel) + assert app._process_worker_client is sentinel diff --git a/tests/unit/test_process_worker_fallback.py b/tests/unit/test_process_worker_fallback.py new file mode 100644 index 00000000..b5f38c76 --- /dev/null +++ b/tests/unit/test_process_worker_fallback.py @@ -0,0 +1,68 @@ +"""Tests for the macOS fork-fallback guard and the worker log redirect.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from sqlit.domains.process_worker.app.process_worker import _open_worker_log +from sqlit.domains.process_worker.app.process_worker_client import ProcessWorkerClient + + +def _make_client_without_init() -> ProcessWorkerClient: + """Build a bare ProcessWorkerClient instance without running __init__. + + __init__ spawns a real subprocess; we only want to exercise the + in-process branching logic in _maybe_fallback_start. + """ + return ProcessWorkerClient.__new__(ProcessWorkerClient) + + +def test_fork_fallback_disabled_on_darwin(monkeypatch: pytest.MonkeyPatch) -> None: + """On macOS the fallback path must re-raise instead of forking.""" + monkeypatch.setattr("sys.platform", "darwin") + client = _make_client_without_init() + + error = ValueError("bad value(s) in fds_to_keep") + with pytest.raises(ValueError, match="fds_to_keep"): + client._maybe_fallback_start(error) + + +def test_fork_fallback_skips_non_fds_errors(monkeypatch: pytest.MonkeyPatch) -> None: + """Errors unrelated to fds_to_keep should always re-raise verbatim.""" + monkeypatch.setattr("sys.platform", "linux") + client = _make_client_without_init() + + error = ValueError("totally unrelated message") + with pytest.raises(ValueError, match="totally unrelated"): + client._maybe_fallback_start(error) + + +def test_worker_log_honors_env_override( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """SQLIT_WORKER_LOG should redirect the worker log to a custom path.""" + target = tmp_path / "subdir" / "worker.log" + monkeypatch.setenv("SQLIT_WORKER_LOG", str(target)) + + log_file = _open_worker_log() + assert log_file is not None + try: + log_file.write("hello\n") + finally: + log_file.close() + + assert target.exists() + assert "hello" in target.read_text() + + +def test_worker_log_returns_none_on_unwritable_path( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """If the log path can't be opened, the worker should still boot.""" + # A path whose parent can't be created (root-owned, non-existent). + monkeypatch.setenv("SQLIT_WORKER_LOG", "/proc/definitely/not/writable/here.log") + + log_file = _open_worker_log() + assert log_file is None