Per-statement timeout for the SQL executor (resource_limit) by sandeep-agami · Pull Request #115 · AgamiAI/agami-core

sandeep-agami · 2026-07-12T05:32:16Z

Adds a per-statement timeout to the SQL executor so a runaway query (a recursive CTE, a cartesian product, an unbounded scan) is bounded rather than hanging. The result-row cap shipped earlier (#109); this is the timeout half of the same work.

What changes

AGAMI_SQL_TIMEOUT_S (default 30s) bounds every query at the single executor chokepoint, so both the stdio and HTTP surfaces inherit it. A query that exceeds it is cancelled and returns a resource_limit refusal — the query is killed, no partial data.
Genuine server-side cancellation where the engine supports it: Postgres/Redshift statement_timeout (delivered via SET LOCAL so transaction-mode poolers don't reject it), MySQL max_execution_time / MariaDB max_statement_time, Snowflake STATEMENT_TIMEOUT_IN_SECONDS, BigQuery job_timeout_ms + job.cancel(), SQL Server / Oracle / Trino / Databricks each their native param or cursor cancel — layered under a universal wall-clock watchdog (per-driver conn.cancel / conn.interrupt / cur.cancel / socket timeout) so no engine hangs past the deadline.
Documented in both agami.env.example copies alongside the row-cap knob.

Verification

The genuine cancel is proven in-process against SQLite and DuckDB — a real recursive-CTE bomb is interrupted mid-flight (~1s under a 1s deadline) and returns a resource_limit refusal, not a hang or a generic error.
The cloud engines' native params are asserted by config (a unit test per driver, checking the param and its units — the ms-vs-s mix is the easy way to silently make a timeout 1000x wrong). CI has no live warehouses, so this mirrors the same call as the prior fail-closed spec.
Full test gate green; driven end-to-end through the executor CLI (bomb → resource_limit + exit 1; fast query unaffected).

Review

Ran the Agami review panel — correctness, silent-failure, test-quality, and a mandatory High-lane security review. Security sign-off satisfied (Postgres genuinely cancels DB-side; a timed-out query is a refused response with no data; no injection/leak/egress; the timeout can't be disabled by query or env). Fixes applied from the panel:

MySQL/MariaDB could hang — closing a connection from the watchdog thread doesn't unblock a recv() on Linux, and MariaDB's native variable differs; added socket read_timeout/write_timeout (a reliable client bound) and try both dialects' native timeout.
Postgres pooler regression — the libpq options startup parameter is rejected at connect by transaction-mode poolers; deliver statement_timeout per-transaction via SET LOCAL instead.
Postgres exit code — a cancelled query aborts the txn, so the server-side cursor close raised and masked the refusal; swallow the close so the resource_limit exit code survives.
Classifier race — classify a timeout on elapsed wall-clock time as well as the watchdog flag, so a native server timeout that wins the race is never mislabeled.

Stacked on #112; retarget to main once #111 and #112 merge.

Spec: ACE-038

🤖 Generated with Claude Code

Copilot

Pull request overview

Implements ACE-038’s per-statement SQL timeout at the shared executor chokepoint, ensuring runaway queries are cancelled and surfaced as a typed resource_limit refusal (no partial data) across both stdio and HTTP surfaces.

Changes:

Adds AGAMI_SQL_TIMEOUT_S (default 30s) plus a universal wall-clock watchdog layered with engine-native timeout/cancel mechanisms.
Ensures refusal semantics are consistent (exit code 1 + refusal JSON; discard any partial stdout on refusal).
Adds focused tests covering in-process cancellation (SQLite/DuckDB) and config/unit assertions for native timeout parameters, plus env example documentation.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`packages/agami-core/src/execute_sql.py`	Adds watchdog-based per-statement deadline, engine-specific native timeout wiring, and maps deadline hits to `resource_limit` refusals.
`plugins/agami/lib/execute_sql.py`	Mirrors the same executor timeout/refusal behavior for the plugin-distributed copy.
`tests/test_resource_limits.py`	New test suite proving real cancellation for SQLite/DuckDB and asserting native timeout parameters/units per driver.
`tests/test_execute_sql_envelope.py`	Pins that `resource_limit` refusals discard partial stdout so refused envelopes never include partial data.
`tests/test_ace044_bounded_fetch.py`	Updates test cursor stub with `cancel()` to support the new watchdog cancel primitive.
`deploy/agami.env.example`	Documents `AGAMI_SQL_TIMEOUT_S` alongside row cap as executor-enforced resource limits.
`plugins/agami/skills/agami-deploy/bundle/agami.env.example`	Same env documentation for the deploy bundle copy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+@contextmanager
+def _deadline(cancel: Callable[[], None]):
+    """Bound the enclosed statement by a wall-clock watchdog: after `_resolve_timeout_s()` seconds,
+    `cancel()` interrupts the in-flight query (per driver — `conn.cancel()` / `conn.interrupt()` /
+    `cur.cancel()`), which makes the blocked execute/fetch raise. The universal availability backstop
+    so no engine hangs past the deadline, even one with no native statement timeout. Yields an Event
+    that is set iff the deadline fired (so the caller can tell a timeout from a real error)."""
+    fired = threading.Event()
+
+    def _fire() -> None:
+        fired.set()
+        try:
+            cancel()
+        except Exception:
+            pass  # best-effort cancel; the deadline having fired is what the caller keys on
+
+    timer = threading.Timer(_resolve_timeout_s(), _fire)
+    timer.daemon = True
+    timer.start()


+@contextmanager
+def _deadline(cancel: Callable[[], None]):
+    """Bound the enclosed statement by a wall-clock watchdog: after `_resolve_timeout_s()` seconds,
+    `cancel()` interrupts the in-flight query (per driver — `conn.cancel()` / `conn.interrupt()` /
+    `cur.cancel()`), which makes the blocked execute/fetch raise. The universal availability backstop
+    so no engine hangs past the deadline, even one with no native statement timeout. Yields an Event
+    that is set iff the deadline fired (so the caller can tell a timeout from a real error)."""
+    fired = threading.Event()
+
+    def _fire() -> None:
+        fired.set()
+        try:
+            cancel()
+        except Exception:
+            pass  # best-effort cancel; the deadline having fired is what the caller keys on
+
+    timer = threading.Timer(_resolve_timeout_s(), _fire)
+    timer.daemon = True
+    timer.start()


A generated query can hang or exhaust the DB (recursive CTE, cartesian bomb, unbounded scan). AGAMI_SQL_TIMEOUT_S (default 30) bounds every statement: each engine sets its NATIVE server-side timeout where it has one (Postgres SET LOCAL statement_timeout, MySQL/MariaDB max_execution_time / max_statement_time, Snowflake STATEMENT_TIMEOUT_IN_SECONDS, BigQuery job_timeout_ms, Oracle call_timeout, SQL Server / Trino / Databricks native caps) — the genuine DB-side cancel — layered under a universal wall-clock watchdog (_run_bounded + _deadline) that cancels the in-flight query client-side for any engine without one (SQLite / DuckDB conn.interrupt). A fired deadline kills the query and returns NO result. Rebased onto the reconciled ACE-035 seam: a timeout raises _ResourceLimit up through the engine's transaction handling (so `with conn` rolls the cancelled txn back), and execute_guarded maps it — at the single chokepoint both surfaces funnel through — to the typed resource_limit Refusal. So the subprocess wire and the in-process handler both surface one refused Envelope with no partial data. _run_bounded now returns the bounded ExecResult; the CLI adapter (_emit_or_err) surfaces the same refusal + exit code. Spec: ACE-038 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…v re-read) Copilot review: _run_bounded (and the BigQuery path) resolve `timeout_s` once, feed it to `_deadline_hit` for the timeout-vs-real-error classification, but _deadline independently RE-READ `AGAMI_SQL_TIMEOUT_S` when constructing the watchdog Timer. A mid-flight change to the env var could make the watchdog duration and the classification diverge. Pass the already-resolved `timeout_s` into _deadline so both key off the SAME value; the env is read once per call, at the call site. Test pins that _deadline schedules the Timer with the passed value, not a fresh env read. Spec: ACE-038 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

sandeep-agami requested a review from ashwin-agami as a code owner July 12, 2026 05:32

sandeep-agami mentioned this pull request Jul 12, 2026

Recon deny-list + DB-error sanitization for the SQL guard #120

Open

5 tasks

sandeep-agami force-pushed the ACE-037-fail-closed-unscopable-sql branch from 637b302 to aa7ccff Compare July 12, 2026 22:09

sandeep-agami force-pushed the ACE-038-resource-limits-timeout branch from 0319246 to bd84345 Compare July 13, 2026 00:27

sandeep-agami requested a review from Copilot July 13, 2026 01:24

Copilot started reviewing on behalf of sandeep-agami July 13, 2026 01:25 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

sandeep-agami force-pushed the ACE-037-fail-closed-unscopable-sql branch from aa7ccff to a1e5065 Compare July 13, 2026 02:06

sandeep-agami and others added 2 commits July 12, 2026 19:07

sandeep-agami force-pushed the ACE-038-resource-limits-timeout branch from bd84345 to 17fe585 Compare July 13, 2026 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Per-statement timeout for the SQL executor (resource_limit)#115

Per-statement timeout for the SQL executor (resource_limit)#115
sandeep-agami wants to merge 2 commits into
ACE-037-fail-closed-unscopable-sqlfrom
ACE-038-resource-limits-timeout

sandeep-agami commented Jul 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sandeep-agami commented Jul 12, 2026

What changes

Verification

Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants