fix: reliable analytical-query timeout signal + injectable runner

[devopam]

Summary

Follow-up hardening for run_analytical_query, addressing the two review points raised on #283 (both Gemini and Sourcery flagged them independently).

1. Reliable, structured timeout detection. The old run_select fallback that hints callers toward run_analytical_query matched "timed out" in str(exc).lower() — brittle, locale-dependent, and it missed the server-side path entirely: a Postgres statement_timeout surfaces as canceling statement due to statement timeout (SQLSTATE 57014), which never contains "timed out". So the hint fired for the client-side asyncio cap but silently not for the GUC timeout — the more common production trigger.

Now both timeout mechanisms raise a typed QueryTimeoutError (a QueryError subclass, so existing except QueryError handlers keep working). Detection is structural, not textual:

• client-side asyncio wall-clock cap — SafeSqlDriver re-wraps the asyncio.TimeoutError in a ValueError but chains the original as __cause__, so we check the cause;
• server-side statement_timeout — psycopg's SQLSTATE 57014.

The run_select hint branches on the type, and is shown only when a runner is actually wired up (the runner is the source of truth, not the settings flag).

2. Injectable runner; registration aligned with runner presence. With an injected database and enable_analytical_queries=True, #283 still registered run_analytical_query while the runner was None — advertising a tool that errors on call. Now:

• create_server(..., analytical_runner=...) accepts an injectable runner (mirroring listen_manager / cursor_manager);
• register_tools gains analytical_available, so the tool is registered only when a runner will back it. Contract tests that call register_tools directly fall back to the settings flag, so the surface snapshot is unchanged.

Docs (spots the original feature PR missed). Folded the analytical env vars into the README env-var table, added the bounded-analytical-path mitigation to the T3 (DoS) threat model in security.md, and added an analytical pool-overhead entry to scaling.md's peak-connection budget.

Lockfile drift (folded in). origin/main had pyproject.toml pinning pglast==8.4 (dependabot #281) but uv.lock still at 8.2. Synced the lock to 8.4; the SQL-kernel adversarial + fuzz + safety suites pass unchanged under the parser bump.

No tool-surface change (still 254). Full unit + contract suite green (2866 passed, +7 new); ruff / ruff format / mypy src/mcpg clean.

Roadmap linkage

Advances roadmap row: N/A — follow-up hardening on #283's reactive capability (itself N/A).

Checklist

• Tests added/updated first (TDD); suite passes locally
• ruff, ruff format, and mypy src/mcpg pass
• CHANGELOG.md updated under [Unreleased]
• Roadmap row cited above (or N/A); row marked ✅ when this PR completes it
• No hand-edits to src/mcpg/_vendor/

🤖 Generated with Claude Code

https://claude.ai/code/session_0122yLZLJ8t4W43sdN6BmTZc

[gemini-code-assist-2]

Code Review

This pull request hardens query timeout handling by introducing a typed QueryTimeoutError and a structured _is_timeout_exc helper to detect both client-side asyncio timeouts and server-side PostgreSQL statement timeouts (SQLSTATE 57014). It also refactors server creation to support injecting an AnalyticalRunner and ensures run_analytical_query is only registered when a runner is present. Additionally, documentation has been updated, and comprehensive unit tests have been added. The review feedback suggests extending the timeout detection helper to also check exc.__context__ for implicit exception chaining and adding a corresponding unit test to ensure robust error handling.

[gemini-code-assist-2]

The _is_timeout_exc helper currently checks exc and exc.__cause__ to detect client-side TimeoutError exceptions. However, in Python, exceptions can also be chained implicitly via the __context__ attribute (for example, if a ValueError or QueryError is raised while a TimeoutError is active). To ensure robust timeout detection and prevent the self-correcting fallback from failing to fire under implicit chaining, we should also check exc.__context__.

Suggested change

	if isinstance(exc, TimeoutError) or isinstance(exc.__cause__, TimeoutError):
	return True
	return bool(getattr(exc, "sqlstate", None) == _PG_QUERY_CANCELED_SQLSTATE)
	if isinstance(exc, TimeoutError) or isinstance(exc.__cause__, TimeoutError) or isinstance(exc.__context__, TimeoutError):
	return True
	return bool(getattr(exc, "sqlstate", None) == _PG_QUERY_CANCELED_SQLSTATE)

[devopam]

Addressed in c826185, with a more general fix than the single __context__ check: _is_timeout_exc now walks the full __cause__/__context__ chain (bounded by a seen-set against cyclic chains), so a timeout buried under any number of wrapping layers is detected — not just one level deep. Added the __context__ test you suggested plus deep-cause and cyclic-chain cases.

---

Generated by Claude Code

[gemini-code-assist-2]

Add a unit test to verify that _is_timeout_exc correctly detects TimeoutError when it is implicitly chained via the __context__ attribute.

def test_is_timeout_exc_asyncio_cap_wrapped() -> None:
    # SafeSqlDriver re-wraps the asyncio TimeoutError in a ValueError but
    # chains the original as __cause__ — we detect it via the cause, not text.
    wrapped = ValueError("Query execution timed out after 30 seconds")
    wrapped.__cause__ = TimeoutError()
    assert _is_timeout_exc(wrapped) is True


def test_is_timeout_exc_asyncio_cap_implicitly_chained() -> None:
    # We also check __context__ to support implicit exception chaining.
    wrapped = ValueError("Query execution timed out after 30 seconds")
    wrapped.__context__ = TimeoutError()
    assert _is_timeout_exc(wrapped) is True

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The new analytical_available plumbing means create_server(..., analytical_runner=...) will register run_analytical_query even when settings.enable_analytical_queries is False; consider AND‑ing the flag with the runner presence so operators can reliably disable the tool via config even if a runner is injected.
• Timeout detection in _is_timeout_exc only looks at the immediate exception and its direct __cause__; if the driver or caller adds extra wrapping layers, these timeouts may be missed, so it may be worth walking the full __cause__/__context__ chain (and possibly checking sqlstate there as well).

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `analytical_available` plumbing means `create_server(..., analytical_runner=...)` will register `run_analytical_query` even when `settings.enable_analytical_queries` is `False`; consider AND‑ing the flag with the runner presence so operators can reliably disable the tool via config even if a runner is injected.
- Timeout detection in `_is_timeout_exc` only looks at the immediate exception and its direct `__cause__`; if the driver or caller adds extra wrapping layers, these timeouts may be missed, so it may be worth walking the full `__cause__`/`__context__` chain (and possibly checking `sqlstate` there as well).

## Individual Comments

### Comment 1
<location path="src/mcpg/tools.py" line_range="2992-2999" />
<code_context>
-            # it so it needn't predict duration up front.
-            settings = ctx.request_context.lifespan_context.settings
-            if settings.enable_analytical_queries and "timed out" in str(exc).lower():
+        except query.QueryTimeoutError as exc:
+            # Self-correcting fallback: a timeout — whether the client-side
+            # wall-clock cap or a server-side statement_timeout — is surfaced
+            # as a typed QueryTimeoutError, so we branch on the type instead of
+            # matching message text. Point the agent at the analytical path,
+            # but only when it's actually wired up (a runner is present); the
+            # runner is the source of truth, not the settings flag.
+            if ctx.request_context.lifespan_context.analytical_runner is not None:
                 raise query.QueryError(
                     f"{exc} If this is a legitimate long-running analytical query, retry with "
</code_context>
<issue_to_address>
**issue (bug_risk):** QueryTimeoutError is swallowed when no analytical runner is present, causing silent None returns.

As written, when `analytical_runner` is `None` the `except` block completes without re-raising, so the function returns `None` instead of propagating the timeout. Previously all timeouts produced a `QueryError`, so this changes the contract and can lead to confusing `None` results. Please re-raise the timeout when no runner is present, e.g. by adding:

```python
        except query.QueryTimeoutError as exc:
            if ctx.request_context.lifespan_context.analytical_runner is not None:
                raise query.QueryError(
                    f"{exc} If this is a legitimate long-running analytical query, retry with "
                    "run_analytical_query — a higher, bounded timeout on an isolated connection."
                ) from exc
            raise  # or `raise exc`
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}