mirror of
https://github.com/usestrix/strix.git
synced 2026-08-22 19:09:37 +02:00
fix(proxy): host-side reconnect, close stale clients, don't retry mutations
Addresses Greptile review on the reconnect logic: - Host path had no reconnect: a dead shared context client (Caido restart / network blip) previously disabled proxy tools for the rest of the scan. Add SharedCaidoClient, a serialized reconnect-safe holder stored once per scan in the run context and shared across agents. On a dead transport it rebuilds via reconnect_caido, which re-selects the SAME Caido project (preserving captured traffic) instead of creating a new empty one. - Don't repeat completed mutations: call_with_client / SharedCaidoClient.call take idempotent=. Reads retry once on reconnect; replay + scope create/update/delete heal the client but re-raise instead of risking a double-apply. - Don't leak replaced clients: the stale client is aclose()d (best-effort) on every reconnect. - Extend tests to cover close-on-reconnect, non-idempotent re-raise, and the SharedCaidoClient holder.
This commit is contained in:
+132
-17
@@ -1,20 +1,20 @@
|
||||
"""Tests for the shared Caido client lifecycle and proxy error handling.
|
||||
|
||||
Covers the concurrency/reconnect guarantees of ``caido_api.call_with_client``
|
||||
(the sandbox-imported path) and the host-side helpers in ``proxy.tools``
|
||||
(scan-wide lock + actionable HTTPQL errors).
|
||||
(the sandbox-imported path) and ``caido_api.SharedCaidoClient`` (the host-side
|
||||
holder), plus the actionable HTTPQL errors in ``proxy.tools``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import contextlib
|
||||
import json
|
||||
from typing import TYPE_CHECKING, Any, cast
|
||||
|
||||
import pytest
|
||||
|
||||
from strix.tools.proxy import caido_api, tools
|
||||
from strix.tools.proxy.caido_api import SharedCaidoClient
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -24,6 +24,10 @@ if TYPE_CHECKING:
|
||||
class _FakeClient:
|
||||
def __init__(self, name: str) -> None:
|
||||
self.name = name
|
||||
self.closed = False
|
||||
|
||||
async def aclose(self) -> None:
|
||||
self.closed = True
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
@@ -35,7 +39,7 @@ def _clear_cache() -> Iterator[None]:
|
||||
|
||||
async def test_call_with_client_reuses_cached_client(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
cached = _FakeClient("cached")
|
||||
caido_api._CLIENT_CACHE["default"] = cached
|
||||
caido_api._CLIENT_CACHE["default"] = cast("Any", cached)
|
||||
|
||||
async def _new() -> Any:
|
||||
raise AssertionError("_new_client must not run when a client is cached")
|
||||
@@ -87,12 +91,12 @@ async def test_failed_init_does_not_poison_cache(monkeypatch: pytest.MonkeyPatch
|
||||
assert "default" not in caido_api._CLIENT_CACHE
|
||||
|
||||
|
||||
async def test_call_with_client_reconnects_on_dead_transport(
|
||||
async def test_call_with_client_reconnects_and_closes_dead_transport(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
dead = _FakeClient("dead")
|
||||
fresh = _FakeClient("fresh")
|
||||
caido_api._CLIENT_CACHE["default"] = dead
|
||||
caido_api._CLIENT_CACHE["default"] = cast("Any", dead)
|
||||
|
||||
new_calls = {"n": 0}
|
||||
|
||||
@@ -114,13 +118,41 @@ async def test_call_with_client_reconnects_on_dead_transport(
|
||||
assert attempts == [dead, fresh]
|
||||
assert new_calls["n"] == 1
|
||||
assert caido_api._CLIENT_CACHE["default"] is fresh
|
||||
assert dead.closed is True # stale transport is not leaked
|
||||
|
||||
|
||||
async def test_call_with_client_non_idempotent_rebuilds_but_reraises(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
dead = _FakeClient("dead")
|
||||
fresh = _FakeClient("fresh")
|
||||
caido_api._CLIENT_CACHE["default"] = cast("Any", dead)
|
||||
|
||||
async def _new() -> Any:
|
||||
return fresh
|
||||
|
||||
monkeypatch.setattr(caido_api, "_new_client", _new)
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
async def fn(_client: Any) -> str:
|
||||
calls["n"] += 1
|
||||
raise RuntimeError("Server disconnected")
|
||||
|
||||
# A mutation must not be auto-retried (it may already have applied), but the
|
||||
# dead client is still healed so later calls succeed.
|
||||
with pytest.raises(RuntimeError, match="Server disconnected"):
|
||||
await caido_api.call_with_client(fn, idempotent=False)
|
||||
assert calls["n"] == 1
|
||||
assert caido_api._CLIENT_CACHE["default"] is fresh
|
||||
assert dead.closed is True
|
||||
|
||||
|
||||
async def test_call_with_client_does_not_retry_application_errors(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
cached = _FakeClient("cached")
|
||||
caido_api._CLIENT_CACHE["default"] = cached
|
||||
caido_api._CLIENT_CACHE["default"] = cast("Any", cached)
|
||||
|
||||
async def _new() -> Any:
|
||||
raise AssertionError("deterministic errors must not trigger a reconnect")
|
||||
@@ -142,7 +174,7 @@ async def test_call_with_client_does_not_retry_application_errors(
|
||||
async def test_call_with_client_serializes_concurrent_calls(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
caido_api._CLIENT_CACHE["default"] = _FakeClient("shared")
|
||||
caido_api._CLIENT_CACHE["default"] = cast("Any", _FakeClient("shared"))
|
||||
|
||||
async def _new() -> Any:
|
||||
raise AssertionError("no reconnect expected")
|
||||
@@ -162,6 +194,87 @@ async def test_call_with_client_serializes_concurrent_calls(
|
||||
assert state["max"] == 1
|
||||
|
||||
|
||||
async def test_shared_client_reconnects_and_closes_dead_transport() -> None:
|
||||
dead = _FakeClient("dead")
|
||||
fresh = _FakeClient("fresh")
|
||||
|
||||
async def _reconnect() -> Any:
|
||||
return fresh
|
||||
|
||||
holder = SharedCaidoClient(cast("Any", dead), _reconnect)
|
||||
|
||||
attempts: list[Any] = []
|
||||
|
||||
async def fn(client: Any) -> str:
|
||||
attempts.append(client)
|
||||
if len(attempts) == 1:
|
||||
raise RuntimeError("Connector is closed")
|
||||
return "ok"
|
||||
|
||||
assert await holder.call(fn) == "ok"
|
||||
assert attempts == [dead, fresh]
|
||||
assert dead.closed is True
|
||||
|
||||
|
||||
async def test_shared_client_non_idempotent_rebuilds_but_reraises() -> None:
|
||||
dead = _FakeClient("dead")
|
||||
fresh = _FakeClient("fresh")
|
||||
|
||||
async def _reconnect() -> Any:
|
||||
return fresh
|
||||
|
||||
holder = SharedCaidoClient(cast("Any", dead), _reconnect)
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
async def fn(_client: Any) -> str:
|
||||
calls["n"] += 1
|
||||
raise RuntimeError("Server disconnected")
|
||||
|
||||
with pytest.raises(RuntimeError, match="Server disconnected"):
|
||||
await holder.call(fn, idempotent=False)
|
||||
assert calls["n"] == 1
|
||||
assert dead.closed is True
|
||||
# The healthy client remains for the next call.
|
||||
assert await holder.call(lambda _c: _ok()) == "ok"
|
||||
|
||||
|
||||
async def _ok() -> str:
|
||||
return "ok"
|
||||
|
||||
|
||||
async def test_shared_client_serializes_concurrent_calls() -> None:
|
||||
async def _reconnect() -> Any:
|
||||
raise AssertionError("no reconnect expected")
|
||||
|
||||
holder = SharedCaidoClient(cast("Any", _FakeClient("shared")), _reconnect)
|
||||
|
||||
state = {"active": 0, "max": 0}
|
||||
|
||||
async def fn(_client: Any) -> str:
|
||||
state["active"] += 1
|
||||
state["max"] = max(state["max"], state["active"])
|
||||
await asyncio.sleep(0.01)
|
||||
state["active"] -= 1
|
||||
return "ok"
|
||||
|
||||
await asyncio.gather(*(holder.call(fn) for _ in range(6)))
|
||||
assert state["max"] == 1
|
||||
|
||||
|
||||
async def test_shared_client_passes_through_application_errors() -> None:
|
||||
async def _reconnect() -> Any:
|
||||
raise AssertionError("deterministic errors must not trigger a reconnect")
|
||||
|
||||
holder = SharedCaidoClient(cast("Any", _FakeClient("c")), _reconnect)
|
||||
|
||||
async def fn(_client: Any) -> str:
|
||||
raise ValueError("Invalid HTTPQL filter")
|
||||
|
||||
with pytest.raises(ValueError, match="Invalid HTTPQL"):
|
||||
await holder.call(fn)
|
||||
|
||||
|
||||
def test_is_connection_error_matches_markers_and_causes() -> None:
|
||||
assert caido_api._is_connection_error(RuntimeError("Transport is already connected"))
|
||||
assert caido_api._is_connection_error(RuntimeError("Connector is closed"))
|
||||
@@ -178,17 +291,19 @@ class _Ctx:
|
||||
self.context = context
|
||||
|
||||
|
||||
def test_ctx_lock_returns_lock_when_present() -> None:
|
||||
lock = asyncio.Lock()
|
||||
got = tools._ctx_lock(cast("Any", _Ctx({"caido_lock": lock})))
|
||||
assert got is lock
|
||||
def test_ctx_proxy_returns_holder_when_present() -> None:
|
||||
async def _reconnect() -> Any:
|
||||
raise AssertionError("unused")
|
||||
|
||||
holder = SharedCaidoClient(cast("Any", _FakeClient("c")), _reconnect)
|
||||
got = tools._ctx_proxy(cast("Any", _Ctx({"caido_client": holder})))
|
||||
assert got is holder
|
||||
|
||||
|
||||
def test_ctx_lock_falls_back_to_noop_without_lock() -> None:
|
||||
got = tools._ctx_lock(cast("Any", _Ctx({})))
|
||||
assert isinstance(got, contextlib.nullcontext)
|
||||
got_non_dict = tools._ctx_lock(cast("Any", _Ctx(None)))
|
||||
assert isinstance(got_non_dict, contextlib.nullcontext)
|
||||
def test_ctx_proxy_returns_none_without_holder() -> None:
|
||||
assert tools._ctx_proxy(cast("Any", _Ctx({}))) is None
|
||||
assert tools._ctx_proxy(cast("Any", _Ctx(None))) is None
|
||||
assert tools._ctx_proxy(cast("Any", _Ctx({"caido_client": object()}))) is None
|
||||
|
||||
|
||||
def test_is_httpql_error_detection() -> None:
|
||||
|
||||
Reference in New Issue
Block a user