[pull] main from OpenHands:main

openhands-agent-server/openhands/agent_server/api.py

@@ -53,6 +53,7 @@
     require_initialized,
 )
 from openhands.agent_server.llm_router import llm_router
+from openhands.agent_server.local_secret_resolver import local_secret_resolution
 from openhands.agent_server.mcp_router import mcp_router
 from openhands.agent_server.middleware import CORSDispatcher
 from openhands.agent_server.openai.router import (
@@ -154,6 +155,7 @@ def _cleanup_stale_tmux_sessions() -> None:
 @asynccontextmanager
 async def api_lifespan(api: FastAPI) -> AsyncIterator[None]:
     tmux_tmpdir, tmux_tmpdir_was_defaulted = _ensure_server_tmux_tmpdir()
+    secret_resolution: local_secret_resolution | None = None
     try:
         # Clean up stale tmux sessions from previous server runs
         _cleanup_stale_tmux_sessions()
@@ -165,6 +167,11 @@ async def api_lifespan(api: FastAPI) -> AsyncIterator[None]:
         ) or create_conversation_registry(config)
         api.state.conversation_registry = conversation_registry
 
+        # Answer our own LookupSecret URLs in-process; a loopback fetch made
+        # from the event loop cannot be served by the loop blocked on it.
+        secret_resolution = local_secret_resolution(config)
+        secret_resolution.__enter__()
+
         # Deferred pods boot with telemetry disabled and are rebuilt by
         # InitService, so they emit `server_started` there instead.
         api.state.telemetry_sink = await build_telemetry_sink(config)
@@ -292,6 +299,8 @@ async def stop_tool_preload_service():
     finally:
         # Outer finally so a startup failure cannot leak the drain task, and
         # after `async with service` so terminal events are still accepted.
+        if secret_resolution is not None:
+            secret_resolution.__exit__(None, None, None)
         emit_server_stopped()
         await shutdown_telemetry_sink()
 

openhands-agent-server/openhands/agent_server/local_secret_resolver.py

@@ -0,0 +1,87 @@
+"""Serve this server's own ``LookupSecret`` URLs without a loopback request.
+
+``LookupSecret`` resolves lazily over HTTP so raw values never transit an SDK
+client. When the client and the server are the same process, that round trip
+buys nothing and costs correctness: a resolution on the event loop blocks the
+very loop that would answer it, so it stalls until the client times out.
+Resolving those URLs against the local store keeps the laziness and drops the
+round trip.
+"""
+
+import os
+from urllib.parse import urlsplit
+
+from openhands.agent_server.config import Config
+from openhands.agent_server.persistence import get_secrets_store
+from openhands.sdk.logger import get_logger
+from openhands.sdk.secret import (
+    register_local_secret_resolver,
+    unregister_local_secret_resolver,
+)
+
+
+logger = get_logger(__name__)
+
+_INTERNAL_SERVER_URL_ENV = "OH_INTERNAL_SERVER_URL"
+_DEFAULT_INTERNAL_SERVER_URL = "http://127.0.0.1:8000"
+_SECRET_PATH_PREFIX = "/api/settings/secrets/"
+_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "::1"})
+_DEFAULT_PORTS = {"http": 80, "https": 443}
+
+
+def _effective_port(scheme: str, port: int | None) -> int | None:
+    return port if port is not None else _DEFAULT_PORTS.get(scheme)
+
+
+def _secret_name_if_local(url: str) -> str | None:
+    """Return the secret name when ``url`` is one this process serves itself."""
+    parsed = urlsplit(url)
+    server = urlsplit(os.getenv(_INTERNAL_SERVER_URL_ENV, _DEFAULT_INTERNAL_SERVER_URL))
+    if parsed.scheme != server.scheme:
+        return None
+    if _effective_port(parsed.scheme, parsed.port) != _effective_port(
+        server.scheme, server.port
+    ):
+        return None
+    if parsed.hostname != server.hostname and not {
+        parsed.hostname,
+        server.hostname,
+    }.issubset(_LOOPBACK_HOSTS):
+        return None
+    if not parsed.path.startswith(_SECRET_PATH_PREFIX):
+        return None
+    name = parsed.path[len(_SECRET_PATH_PREFIX) :]
+    # Only a bare name; anything deeper is a different route.
+    if not name or "/" in name:
+        return None
+    return name
+
+
+def build_local_secret_resolver(config: Config):
+    """Build a resolver that answers this server's own secret URLs."""
+
+    def resolve(url: str) -> str | None:
+        name = _secret_name_if_local(url)
+        if name is None:
+            return None
+        value = get_secrets_store(config).get_secret(name)
+        if value is None:
+            # Fall through to HTTP so the caller still sees the server's 404.
+            return None
+        logger.debug("Resolved secret '%s' in-process", name)
+        return value
+
+    return resolve
+
+
+class local_secret_resolution:
+    """Context manager registering the in-process resolver for a server."""
+
+    def __init__(self, config: Config) -> None:
+        self._resolver = build_local_secret_resolver(config)
+
+    def __enter__(self) -> None:
+        register_local_secret_resolver(self._resolver)
+
+    def __exit__(self, *exc_info) -> None:
+        unregister_local_secret_resolver(self._resolver)

openhands-sdk/openhands/sdk/secret/__init__.py

@@ -4,16 +4,22 @@
 """
 
 from openhands.sdk.secret.secrets import (
+    LocalSecretResolver,
     LookupSecret,
     SecretSource,
     SecretValue,
     StaticSecret,
+    register_local_secret_resolver,
+    unregister_local_secret_resolver,
 )
 
 
 __all__ = [
+    "LocalSecretResolver",
     "SecretSource",
     "StaticSecret",
     "LookupSecret",
     "SecretValue",
+    "register_local_secret_resolver",
+    "unregister_local_secret_resolver",
 ]

openhands-sdk/openhands/sdk/secret/secrets.py

@@ -2,6 +2,7 @@
 
 import os
 from abc import ABC, abstractmethod
+from collections.abc import Callable
 from urllib.parse import urljoin, urlsplit
 
 import httpx
@@ -32,6 +33,50 @@ def _resolve_lookup_secret_url(url: str) -> str:
     return urljoin(f"{base_url.rstrip('/')}/", url)
 
 
+LocalSecretResolver = Callable[[str], str | None]
+
+_local_secret_resolvers: list[LocalSecretResolver] = []
+
+
+def register_local_secret_resolver(resolver: LocalSecretResolver) -> None:
+    """Register an in-process resolver for ``LookupSecret`` URLs.
+
+    A process that both serves and consumes ``LookupSecret`` URLs — an
+    agent-server running an in-process conversation, say — cannot fetch them
+    over HTTP from its own event loop: the request can only be answered by the
+    loop that is blocked waiting for it, so it stalls until the client times
+    out. Registering a resolver lets such a process answer its own URLs
+    directly and skip the round trip.
+
+    The resolver receives the fully-resolved URL and returns the secret value,
+    or ``None`` when it does not serve that URL, in which case resolution falls
+    through to the next resolver and finally to HTTP.
+    """
+    if resolver not in _local_secret_resolvers:
+        _local_secret_resolvers.append(resolver)
+
+
+def unregister_local_secret_resolver(resolver: LocalSecretResolver) -> None:
+    """Remove a resolver registered with ``register_local_secret_resolver``."""
+    if resolver in _local_secret_resolvers:
+        _local_secret_resolvers.remove(resolver)
+
+
+def _resolve_secret_locally(url: str) -> str | None:
+    """Return a locally-served value for ``url``, or ``None`` if there is none."""
+    for resolver in list(_local_secret_resolvers):
+        try:
+            value = resolver(url)
+        except Exception:
+            logger.warning(
+                "Local secret resolver raised; falling back to HTTP", exc_info=True
+            )
+            continue
+        if value is not None:
+            return value
+    return None
+
+
 class SecretSource(DiscriminatedUnionMixin, ABC):
     """Source for a named secret which may be obtained dynamically"""
 
@@ -77,6 +122,9 @@ def _normalize_url(cls, url: str) -> str:
         return _resolve_lookup_secret_url(url)
 
     def get_value(self) -> str:
+        local_value = _resolve_secret_locally(self.url)
+        if local_value is not None:
+            return local_value
         response = httpx.get(self.url, headers=self.headers, timeout=30.0)
         response.raise_for_status()
         return response.text

tests/agent_server/test_local_secret_resolver.py

@@ -0,0 +1,85 @@
+"""Tests for in-process resolution of this server's own LookupSecret URLs."""
+
+from collections.abc import Iterator
+from contextlib import contextmanager
+from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
+from threading import Thread
+
+from openhands.agent_server.config import Config
+from openhands.agent_server.local_secret_resolver import (
+    _secret_name_if_local,
+    build_local_secret_resolver,
+)
+from openhands.agent_server.persistence import get_secrets_store
+from openhands.sdk.secret import (
+    LookupSecret,
+    register_local_secret_resolver,
+    unregister_local_secret_resolver,
+)
+
+
+class _ForeignSecretHandler(BaseHTTPRequestHandler):
+    def do_GET(self) -> None:
+        self.send_response(200)
+        self.end_headers()
+        self.wfile.write(b"foreign-value")
+
+    def log_message(self, format: str, *args: object) -> None:
+        pass
+
+
+@contextmanager
+def _foreign_secret_server() -> Iterator[str]:
+    server = ThreadingHTTPServer(("127.0.0.1", 0), _ForeignSecretHandler)
+    thread = Thread(target=server.serve_forever, daemon=True)
+    thread.start()
+    try:
+        host = server.server_address[0]
+        port = server.server_address[1]
+        yield f"http://{host}:{port}"
+    finally:
+        server.shutdown()
+        thread.join()
+        server.server_close()
+
+
+def test_matches_loopback_secret_url(monkeypatch):
+    monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000")
+    assert (
+        _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/TOKEN")
+        == "TOKEN"
+    )
+    assert (
+        _secret_name_if_local("http://localhost:8000/api/settings/secrets/TOKEN")
+        == "TOKEN"
+    )
+
+
+def test_ignores_remote_host(monkeypatch):
+    monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000")
+    assert (
+        _secret_name_if_local("https://remote.example/api/settings/secrets/T") is None
+    )
+
+
+def test_ignores_other_routes_and_malformed_names(monkeypatch):
+    monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000")
+    assert _secret_name_if_local("http://127.0.0.1:8000/api/settings") is None
+    assert _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/") is None
+    assert (
+        _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/a/b") is None
+    )
+
+
+def test_foreign_loopback_port_uses_its_own_server(monkeypatch):
+    monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:18000")
+    config = Config()
+    get_secrets_store(config).set_secret("TOKEN", "local-value")
+    resolver = build_local_secret_resolver(config)
+    register_local_secret_resolver(resolver)
+    try:
+        with _foreign_secret_server() as base_url:
+            secret = LookupSecret(url=f"{base_url}/api/settings/secrets/TOKEN")
+            assert secret.get_value() == "foreign-value"
+    finally:
+        unregister_local_secret_resolver(resolver)

tests/sdk/conversation/test_secret_source.py

@@ -302,3 +302,78 @@ def test_lookup_secret_get_value_resolves_relative_url(monkeypatch):
         headers={},
         timeout=30.0,
     )
+
+
+@pytest.fixture
+def local_resolvers_cleared():
+    """Keep the process-wide resolver registry isolated per test."""
+    from openhands.sdk.secret import secrets as secrets_module
+
+    original = list(secrets_module._local_secret_resolvers)
+    secrets_module._local_secret_resolvers.clear()
+    yield secrets_module._local_secret_resolvers
+    secrets_module._local_secret_resolvers[:] = original
+
+
+def test_local_resolver_short_circuits_http(local_resolvers_cleared):
+    """A matching resolver answers without any HTTP request.
+
+    This is the deadlock guard: resolving over loopback from the event loop
+    blocks the loop that would serve the request.
+    """
+    from openhands.sdk.secret import register_local_secret_resolver
+
+    secret = LookupSecret(url="http://127.0.0.1:8000/api/settings/secrets/TOKEN")
+    register_local_secret_resolver(
+        lambda url: "resolved-locally" if url.endswith("/TOKEN") else None
+    )
+
+    with patch("httpx.get", side_effect=AssertionError("HTTP must not be used")):
+        assert secret.get_value() == "resolved-locally"
+
+
+def test_local_resolver_declining_falls_back_to_http(local_resolvers_cleared):
+    """A resolver returning None leaves the URL to HTTP."""
+    from openhands.sdk.secret import register_local_secret_resolver
+
+    secret = LookupSecret(url="https://remote.example/api/settings/secrets/TOKEN")
+    register_local_secret_resolver(lambda url: None)
+
+    response = Mock(text="from-http")
+    response.raise_for_status = Mock()
+    with patch("httpx.get", return_value=response) as http_get:
+        assert secret.get_value() == "from-http"
+    http_get.assert_called_once()
+
+
+def test_local_resolver_raising_falls_back_to_http(local_resolvers_cleared):
+    """A broken resolver must not take secret resolution down with it."""
+    from openhands.sdk.secret import register_local_secret_resolver
+
+    def boom(url: str) -> str | None:
+        raise RuntimeError("resolver is broken")
+
+    secret = LookupSecret(url="https://remote.example/api/settings/secrets/TOKEN")
+    register_local_secret_resolver(boom)
+
+    response = Mock(text="from-http")
+    response.raise_for_status = Mock()
+    with patch("httpx.get", return_value=response):
+        assert secret.get_value() == "from-http"
+
+
+def test_unregister_local_secret_resolver(local_resolvers_cleared):
+    from openhands.sdk.secret import (
+        register_local_secret_resolver,
+        unregister_local_secret_resolver,
+    )
+
+    def resolver(url: str) -> str | None:
+        return "resolved-locally"
+
+    register_local_secret_resolver(resolver)
+    register_local_secret_resolver(resolver)
+    assert len(local_resolvers_cleared) == 1
+
+    unregister_local_secret_resolver(resolver)
+    assert local_resolvers_cleared == []