Skip to content

revert(proxy): drop overfit Caido reconnect/HTTPQL band-aids, keep serialization lock#799

Merged
0xallam merged 2 commits into
mainfrom
devin/1784313153-revert-proxy-overfit
Jul 17, 2026
Merged

revert(proxy): drop overfit Caido reconnect/HTTPQL band-aids, keep serialization lock#799
0xallam merged 2 commits into
mainfrom
devin/1784313153-revert-proxy-overfit

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Followup to #794. Most of what #794 added to strix/tools/proxy/{tools,caido_api}.py overfit to specific past error messages rather than fixing anything: a reconnect path that matched on substrings like "transport is already connected" / "connector is closed", and an injected HTTPQL "hint" gated on "httpql" in str(exc). This reverts both files to their pre-#794 shape and keeps only the one real fix — serializing access to the non-concurrency-safe Caido GraphQL transport.

Kept (real concurrency fix): the asyncio lock. Two in-flight requests on the shared client race and raise "Transport is already connected", so all proxy calls run one at a time.

  • caido_api.call_with_client stays, reduced to pure serialization:
    async def call_with_client[T](fn) -> T:
        async with _CLIENT_LOCK:
            client = _CLIENT_CACHE.get("default") or await _new_client()
            _CLIENT_CACHE["default"] = client
            return await fn(client)   # no reconnect, no idempotent=
  • host-side tools.py serializes through a module-level _CAIDO_CALL_LOCK via a small _call(client, fn) helper.

Dropped (overfit / redundant):

  • _CONNECTION_ERROR_MARKERS, _is_connection_error, _safe_aclose, the reconnect + idempotent= retry, and SharedCaidoClient.
  • reconnect_caido and the project-id plumbing — bootstrap_caido returns just the Client again; session_manager/runner store the raw client.
  • _HTTPQL_HINT / _is_httpql_error / _httpql_error. The HTTPQL syntax it injected is already documented in the list_requests docstring.

Moved to the prompt (where it belongs): a concise HTTPQL syntax note added to the PROXY section of system_prompt.jinja (quote strings, unquoted ints, AND/OR with no NOT, operator-by-field-type), instead of surfacing it as a runtime error hint.

Tests: tests/test_proxy_client.py trimmed to the surviving guarantees — client caching, error propagation, and serialization on both the call_with_client and host _call paths.

Net: +86 / −438 across 7 files.

Link to Devin session: https://app.devin.ai/sessions/5453b8e1865246cca776e31159dd9a02
Requested by: @0xallam

…rialization lock

Followup to #794. The error-string-matching reconnect logic and injected
HTTPQL error hints overfit to specific past error messages without solving
the underlying problem, so revert both proxy files to their pre-#794 shape
and keep only the real concurrency fix:

- caido_api: keep the asyncio lock that serializes access to the
  non-concurrency-safe Caido GraphQL transport (call_with_client), drop
  the connection-error markers, _is_connection_error, reconnect/idempotent
  retry, and SharedCaidoClient.
- tools: revert to the raw context client; serialize host-side calls through
  a module-level lock. Drop the _HTTPQL_HINT / _is_httpql_error /
  _httpql_error injection (HTTPQL syntax already lives in the list_requests
  docstring).
- runtime: bootstrap_caido returns just the client again; drop reconnect_caido
  and the project-id plumbing in session_manager/runner.
- prompt: add a concise HTTPQL syntax note to the proxy section of the system
  prompt, where this guidance belongs.
- tests: trim to the surviving caching + serialization guarantees.
@0xallam 0xallam self-assigned this Jul 17, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the Caido reconnect and runtime HTTPQL hint workarounds while keeping request serialization. The main changes are:

  • Serializes sandbox-side and host-side Caido calls with async locks.
  • Restores raw Caido client ownership in the runtime session bundle.
  • Closes the connected client when project setup fails.
  • Moves HTTPQL syntax guidance into the system prompt.
  • Narrows proxy tests to caching, serialization, and error propagation.

Confidence Score: 5/5

This looks safe to merge.

  • Project setup failures now close the connected client before propagating.
  • Successful bootstrap ownership moves into the session cache without an intervening async boundary.
  • No blocking issues remain in the changed code.

Important Files Changed

Filename Overview
strix/runtime/caido_bootstrap.py Closes the connected Caido client when project creation or selection fails.
strix/runtime/session_manager.py Stores and cleans up the raw Caido client in the session bundle.
strix/tools/proxy/caido_api.py Removes message-based reconnection and retries while retaining serialized client access.
strix/tools/proxy/tools.py Serializes host-side proxy calls and removes runtime HTTPQL error rewriting.
strix/agents/prompts/system_prompt.jinja Adds concise HTTPQL syntax guidance to the proxy prompt.
tests/test_proxy_client.py Focuses proxy client tests on caching, serialization, and error propagation.

Reviews (2): Last reviewed commit: "fix(proxy): close Caido client if projec..." | Re-trigger Greptile

Comment thread strix/runtime/caido_bootstrap.py Outdated
Addresses Greptile P1: when connect() succeeds but project create/select
fails, the connected client never reaches the session bundle, so close it
on the failure path to avoid leaking the transport.
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Thanks for the review.

P1 (bootstrap client leak) — fixed in 4b07166: bootstrap_caido now closes the connected client (best-effort) if project.create/select fails, so a failed init no longer leaks the transport.

Teardown race (close_client outside the serialization lock) — leaving as-is. This is pre-existing behavior, not introduced by this PR (this is a revert of #794 back to the prior shape), and coordinating session teardown with in-flight proxy calls is a separate, more involved change that's out of scope for this revert. Happy to track it separately if wanted.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptile

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

✅ Live proxy-function testing passed

Exercised every caido_api proxy function end-to-end against a real caido-cli v0.56.0 instance (same version/flags as containers/docker-entrypoint.sh) with real proxied HTTP/HTTPS traffic. Shell/script-based, no recording.

Function coverage — all PASS
  • get_client() connects
  • list_requests() valid HTTPQL → 10 edges
  • view_request() request (151B) + response (651B)
  • repeat_request() replay → DONE, 651B, 1269ms
  • list_sitemap() → example.com + httpbin.org
  • view_sitemap_entry() → related requests
  • scope_rules() create/list/get/update/delete round-trip
Revert behavior verified
  • Module-level invalid HTTPQL raises OperationUserError cleanly; loop stays healthy.
  • Host-side tools.list_requests invalid filter → generic {"success": false, "error": "list_requests failed: ...Invalid HTTPQL query..."} with none of the removed structured-hint phrases. Runtime hint injection confirmed gone.
Concurrency — the serialization fix works
  • 40× concurrent list_requests → 40/40 ok, 0 transport races.
  • 40× concurrent mixed call_with_client → 40/40 ok, 0 races.
  • Control (bypass the lock): 40/40 fail, 20 exactly "Transport is already connected" — proves the lock is the fix and the test genuinely detects the race.

uv run pytest tests/test_proxy_client.py -q → 8 passed.

Testing via Devin: https://app.devin.ai/sessions/5453b8e1865246cca776e31159dd9a02

@0xallam
0xallam merged commit 96ca7e5 into main Jul 17, 2026
2 checks passed
@0xallam
0xallam deleted the devin/1784313153-revert-proxy-overfit branch July 17, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant