fix(trust-gate): prevent propose_truth from hanging indefinitely on LLM timeout

pyproject.toml

@@ -20,6 +20,7 @@ dependencies = [
     "pyyaml>=6.0",
     "python-ulid>=3.0.0",
     "any-llm-sdk>=1.10.0",
+    "httpx>=0.27.0",
 ]
 
 [project.optional-dependencies]

src/fava_trails/llm/client.py

@@ -7,6 +7,7 @@
 from dataclasses import dataclass
 
 import any_llm
+import httpx
 
 from ._retry import async_retry
 from .registry import get_registry
@@ -78,12 +79,21 @@ async def chat(
             kwargs["max_tokens"] = max_output_tokens
 
         async def _do_call() -> LLMResponse:
+            # Use explicit httpx.Timeout phases to ensure all timeout types are set.
+            # A scalar timeout only sets the total/read timeout; connect and pool
+            # timeouts may default to None (infinite), leaving a hang vector.
+            httpx_timeout = httpx.Timeout(
+                connect=10.0,
+                read=timeout,
+                write=timeout,
+                pool=10.0,
+            )
             response = await any_llm.acompletion(
                 model=resolved_model,
                 provider="openrouter",
                 messages=messages,
                 api_key=self._openrouter_api_key,
-                client_args={"timeout": timeout},
+                client_args={"timeout": httpx_timeout},
                 **kwargs,
             )
             choice = response.choices[0] if response.choices else None

src/fava_trails/models.py

@@ -6,7 +6,7 @@
 from enum import StrEnum
 from typing import Any
 
-from pydantic import BaseModel, Field, field_validator, model_validator
+from pydantic import BaseModel, Field, NonNegativeInt, field_validator, model_validator
 from ulid import ULID
 
 
@@ -218,5 +218,31 @@ class GlobalConfig(BaseModel):
     trust_gate: str = "llm-oneshot"  # llm-oneshot | human (future)
     openrouter_api_key_env: str = "OPENROUTER_API_KEY"
     trust_gate_model: str = "google/gemini-2.5-flash"
+    # Timeout for the Trust Gate LLM call (asyncio.wait_for guard).
+    # Should be well above a normal slow response (e.g. 60-90s) but short enough
+    # to recover from a hung provider before the session times out. 0 = disabled.
+    trust_gate_timeout_secs: NonNegativeInt = 120
+    # Timeout for an entire MCP tool call (outermost guard covering all tools).
+    # Catches jj hangs, slow syncs, and any other unanticipated blocking.
+    # Should be generous — set 0 to disable.
+    tool_timeout_secs: NonNegativeInt = 300
     trails: dict[str, TrailConfig] = Field(default_factory=dict)
     hooks: list[HookEntry] = Field(default_factory=list)
+
+    @model_validator(mode="after")
+    def trust_gate_timeout_within_tool_timeout(self) -> GlobalConfig:
+        """Ensure Trust Gate timeout fires before the outer tool timeout.
+
+        If both are enabled and trust_gate_timeout_secs >= tool_timeout_secs,
+        the outer guard fires first with a generic error, hiding the specific
+        Trust Gate message. Fail early so misconfiguration is obvious.
+        """
+        tg = self.trust_gate_timeout_secs
+        tool = self.tool_timeout_secs
+        if tg > 0 and tool > 0 and tg >= tool:
+            raise ValueError(
+                f"trust_gate_timeout_secs ({tg}) must be less than "
+                f"tool_timeout_secs ({tool}) so the Trust Gate timeout fires "
+                "before the outer tool timeout. Set either to 0 to disable it."
+            )
+        return self

src/fava_trails/server.py

@@ -7,11 +7,14 @@
 from __future__ import annotations
 
 import asyncio
+import functools
 import importlib.resources
 import json
 import logging
+import logging.handlers
 import os
 import sys
+from collections.abc import Callable, Coroutine
 from pathlib import Path
 from typing import Any
 
@@ -41,6 +44,26 @@
     stream=sys.stderr,
 )
 
+# Add rotating file handler so MCP server logs are persisted to disk.
+# Claude Code captures MCP server stderr only in session.jsonl (tool results),
+# not as raw log output — without this, server-side hangs are undiagnosable.
+# Wrapped in try/except so a restricted filesystem never prevents server startup.
+try:
+    _log_dir = Path(os.environ.get("FAVA_TRAILS_LOG_DIR", Path.home() / ".fava-trails" / "logs"))
+    _log_dir.mkdir(parents=True, exist_ok=True)
+    _file_handler = logging.handlers.RotatingFileHandler(
+        _log_dir / "mcp-server.log",
+        maxBytes=5 * 1024 * 1024,  # 5 MB per file
+        backupCount=3,
+    )
+    _file_handler.setFormatter(
+        logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s")
+    )
+    logging.getLogger().addHandler(_file_handler)
+except OSError as _log_err:
+    # Non-fatal: log setup failed (read-only fs, container, etc.) — stderr only.
+    logging.getLogger(__name__).warning("File logging disabled: %s", _log_err)
+
 
 def _build_server_instructions() -> str:
     """Build the MCP server instructions string.
@@ -120,6 +143,17 @@ def _load_usage_guide() -> str:
 
 # Trail manager cache: trail_name -> TrailManager
 _trail_managers: dict[str, TrailManager] = {}
+# Lock guards first-time trail init so parallel calls for the same new scope
+# don't race to double-initialize the jj trail directory.
+_trail_init_lock: asyncio.Lock | None = None
+
+
+def _get_trail_init_lock() -> asyncio.Lock:
+    """Return the trail init lock, creating it lazily inside the running event loop."""
+    global _trail_init_lock
+    if _trail_init_lock is None:
+        _trail_init_lock = asyncio.Lock()
+    return _trail_init_lock
 
 # Shared backend for monorepo init, GC, push, fetch
 _shared_backend: JjBackend | None = None
@@ -192,15 +226,21 @@ async def _get_trail(trail_name: str | None = None) -> TrailManager:
         )
 
     safe_name = sanitize_scope_path(trail_name)
-    if safe_name not in _trail_managers:
-        repo_root = get_data_repo_root()
-        trail_path = get_trails_dir() / safe_name
-        backend = JjBackend(repo_root=repo_root, trail_path=trail_path)
-        manager = TrailManager(safe_name, vcs=backend, hooks=_hook_registry)
-        # Auto-initialize if trail doesn't exist (detect by thoughts/ dir, not .jj)
-        if not (manager.trail_path / "thoughts").exists():
-            await manager.init()
-        _trail_managers[safe_name] = manager
+    # Fast path: already cached (no lock needed — dict reads are safe in asyncio).
+    if safe_name in _trail_managers:
+        return _trail_managers[safe_name]
+    # Slow path: first call for this scope — serialize to prevent double-init.
+    async with _get_trail_init_lock():
+        # Double-check after acquiring lock (another coroutine may have just finished).
+        if safe_name not in _trail_managers:
+            repo_root = get_data_repo_root()
+            trail_path = get_trails_dir() / safe_name
+            backend = JjBackend(repo_root=repo_root, trail_path=trail_path)
+            manager = TrailManager(safe_name, vcs=backend, hooks=_hook_registry)
+            # Auto-initialize if trail doesn't exist (detect by thoughts/ dir, not .jj)
+            if not (manager.trail_path / "thoughts").exists():
+                await manager.init()
+            _trail_managers[safe_name] = manager
 
     return _trail_managers[safe_name]
 
@@ -488,6 +528,37 @@ def _build_trail_name_desc() -> str:
 ]
 
 
+def with_tool_timeout(
+    fn: Callable[..., Coroutine[Any, Any, list[TextContent]]],
+) -> Callable[..., Coroutine[Any, Any, list[TextContent]]]:
+    """Decorator: wraps an MCP tool handler with a configurable asyncio timeout.
+
+    Reads ``tool_timeout_secs`` from GlobalConfig at call time (not decoration time)
+    so config changes take effect without restarting the server.
+    Set ``tool_timeout_secs: 0`` in config.yaml to disable.
+    """
+    @functools.wraps(fn)
+    async def wrapper(name: str, arguments: dict[str, Any]) -> list[TextContent]:
+        timeout = ConfigStore.get().global_config.tool_timeout_secs
+        if timeout <= 0:
+            return await fn(name, arguments)
+        try:
+            return await asyncio.wait_for(fn(name, arguments), timeout=float(timeout))
+        except TimeoutError:
+            logger.error("Tool '%s' timed out after %ds", name, timeout)
+            result = {
+                "status": "error",
+                "message": (
+                    f"Tool '{name}' timed out after {timeout}s. "
+                    "The operation did not complete. "
+                    "If this is sync, check remote connectivity. "
+                    "For propose_truth, the LLM provider may be unresponsive — retry."
+                ),
+            }
+            return [TextContent(type="text", text=json.dumps(result, indent=2))]
+    return wrapper
+
+
 @server.list_tools()
 async def handle_list_tools() -> list[Tool]:
     """List all FAVA Trails tools."""
@@ -502,6 +573,7 @@ async def handle_list_tools() -> list[Tool]:
 
 
 @server.call_tool()
+@with_tool_timeout
 async def handle_call_tool(name: str, arguments: dict[str, Any]) -> list[TextContent]:
     """Route tool calls to handlers. Responses are structured JSON (except get_usage_guide which returns markdown)."""
     from .tools.navigation import (

src/fava_trails/tools/navigation.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import asyncio
 import logging
 import os
 from typing import Any
@@ -153,13 +154,34 @@ async def handle_propose_truth(
                 openrouter_api_key=openrouter_key,
             )
 
-            trust_result = await review_thought(
+            tg_timeout = global_config.trust_gate_timeout_secs
+            _review_coro = review_thought(
                 record=record,
                 prompt=prompt,
                 model=global_config.trust_gate_model,
                 client=llm_client,
                 policy=policy,
             )
+            if tg_timeout > 0:
+                try:
+                    trust_result = await asyncio.wait_for(
+                        _review_coro, timeout=float(tg_timeout)
+                    )
+                except TimeoutError:
+                    logger.error(
+                        "Trust Gate LLM call timed out after %ds for thought %s",
+                        tg_timeout,
+                        thought_id,
+                    )
+                    return {
+                        "status": "error",
+                        "message": (
+                            f"Trust Gate timed out after {tg_timeout}s reviewing thought {thought_id[:8]}. "
+                            "The LLM provider did not respond. Retry propose_truth to try again."
+                        ),
+                    }
+            else:
+                trust_result = await _review_coro
 
         promoted = await trail.propose_truth(thought_id, trust_result=trust_result)
         result = {

tests/test_config.py

@@ -267,3 +267,56 @@ def test_config_store_reads_hooks_from_config_yaml(monkeypatch, tmp_path):
     store = ConfigStore.get()
     assert len(store.global_config.hooks) == 1
     assert store.global_config.hooks[0].path == "./my_hook.py"
+
+
+# --- Timeout config validation ---
+
+def test_global_config_timeout_defaults():
+    """Default timeout values are sane and satisfy the ordering constraint."""
+    config = GlobalConfig()
+    assert config.trust_gate_timeout_secs == 120
+    assert config.tool_timeout_secs == 300
+    assert config.trust_gate_timeout_secs < config.tool_timeout_secs
+
+
+def test_global_config_timeout_validator_rejects_inversion():
+    """trust_gate_timeout_secs >= tool_timeout_secs (both > 0) is rejected."""
+    import pytest
+    from pydantic import ValidationError
+    with pytest.raises(ValidationError, match="must be less than tool_timeout_secs"):
+        GlobalConfig(trust_gate_timeout_secs=300, tool_timeout_secs=120)
+
+
+def test_global_config_timeout_validator_equal_values_rejected():
+    """Equal values are also rejected — inner must fire strictly before outer."""
+    from pydantic import ValidationError
+    with pytest.raises(ValidationError, match="must be less than tool_timeout_secs"):
+        GlobalConfig(trust_gate_timeout_secs=120, tool_timeout_secs=120)
+
+
+def test_global_config_timeout_validator_allows_zero_trust_gate():
+    """trust_gate_timeout_secs=0 disables the inner guard — no ordering constraint."""
+    config = GlobalConfig(trust_gate_timeout_secs=0, tool_timeout_secs=120)
+    assert config.trust_gate_timeout_secs == 0
+
+
+def test_global_config_timeout_validator_allows_zero_tool():
+    """tool_timeout_secs=0 disables the outer guard — no ordering constraint."""
+    config = GlobalConfig(trust_gate_timeout_secs=120, tool_timeout_secs=0)
+    assert config.tool_timeout_secs == 0
+
+
+def test_global_config_timeout_validator_allows_both_zero():
+    """Both disabled is valid (no hang protection, but user asked for it)."""
+    config = GlobalConfig(trust_gate_timeout_secs=0, tool_timeout_secs=0)
+    assert config.trust_gate_timeout_secs == 0
+    assert config.tool_timeout_secs == 0
+
+
+def test_global_config_timeout_rejects_negative():
+    """Negative timeout values are rejected by NonNegativeInt."""
+    from pydantic import ValidationError
+    with pytest.raises(ValidationError):
+        GlobalConfig(trust_gate_timeout_secs=-1)
+    with pytest.raises(ValidationError):
+        GlobalConfig(tool_timeout_secs=-1)