Mcp perf optimization

[yasandu0505]

Governed the handshake between MCP server and the OpenGIN service.

• share single client session
• exception handling
• limiting, pooling
• logging
• request handling (single module to handle all the requests)

[gemini-code-assist]

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

[zaeema-n]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the OpenGIN client into a resilient class-based architecture, introducing a transport layer with circuit breaking, retries, and concurrency control, alongside a governance layer for rate limiting and input validation. Feedback highlights several critical reliability issues, including potential crashes due to unhandled null configurations or context objects, a memory leak in session tracking, and incorrect parsing of the 'Retry-After' header. Additionally, improvements are needed to ensure the governance decorator correctly captures positional arguments and that API responses are safely validated before processing.

[gemini-code-assist]

The config parameter defaults to None, but it is accessed directly in the constructor (e.g., at line 80). If OpenGINTransport is instantiated without a config object, it will raise an AttributeError.

    def __init__(
        self,
        base_url: str,
        config: OpenGINTransportConfig | None = None,
    ):
        config = config or OpenGINTransportConfig()

[gemini-code-assist]

If ctx is None (which can happen if the fallback logic also fails to find a context), calling getattr(ctx, ...) will raise an AttributeError. A check should be added to ensure ctx is valid before accessing its attributes.

Suggested change

	session_id = getattr(ctx, "client_id", None) or "anonymous"
	session_id = (getattr(ctx, "client_id", None) or "anonymous") if ctx else "anonymous"

[gemini-code-assist]

The current implementation only collects tool arguments from kwargs. Any arguments passed positionally will be ignored by the governance checks (budget, rate limiting, and input validation). Using inspect.signature to bind all arguments ensures that both positional and keyword arguments are correctly governed.

            sig = inspect.signature(fn)
            bound_args = sig.bind(*args, **kwargs)
            bound_args.apply_defaults()
            tool_args = {k: v for k, v in bound_args.arguments.items() if k != "ctx"}

[gemini-code-assist]

The Retry-After header can contain either a number of seconds or an HTTP-date string. Attempting to cast it directly to a float will raise a ValueError if the server returns a date string. It is safer to handle potential parsing errors and fall back to a default wait time.

                retry_after_raw = response.headers.get("Retry-After")
                try:
                    retry_after = float(retry_after_raw) if retry_after_raw else 1.0
                except ValueError:
                    retry_after = 1.0

[gemini-code-assist]

The _in_flight dictionary is never cleaned up. When a session's in-flight count returns to zero, the entry remains in the dictionary, leading to a memory leak over time as more unique sessions interact with the server.

Suggested change

	def _exit_call(self, session_id: str) -> None:
	count = self._in_flight.get(session_id, 1) - 1
	self._in_flight[session_id] = max(count, 0)
	def _exit_call(self, session_id: str) -> None:
	count = self._in_flight.get(session_id, 1) - 1
	if count <= 0:
	self._in_flight.pop(session_id, None)
	else:
	self._in_flight[session_id] = count

[gemini-code-assist]

Unlike other methods in this class, search_entities does not verify that result is a dictionary before calling .get(). If the transport returns an unexpected response format (e.g., a list or null), this will raise an AttributeError.

Suggested change

	for item in result.get("body", []):
	if isinstance(result, dict):
	for item in result.get("body", []):
	if "name" in item:
	item["name"] = decode_protobuf_name(item["name"])

[gemini-code-assist]

If the server returns a successful response without a body (e.g., HTTP 204 No Content), response.json() will raise a JSONDecodeError. It is safer to check for content before attempting to parse it.

Suggested change

	return response.json()
	return response.json() if response.content else None