ACE-038: the per-statement timeout — one budget, four ordered bounds, one boolean - #174
Merged
Conversation
Adds the config surface and the watchdog that a per-statement timeout will sit on, with nothing wired into an engine yet. `_resolve_timeout_s` mirrors `_resolve_row_cap`: `AGAMI_SQL_TIMEOUT_S` is the operator-configurable deployment budget, 30s by default, and a missing or non-positive value falls back. It differs in one deliberate way — a value that is present but unparseable is logged at warning naming the rejected text before falling back, rather than being swallowed, because an operator who wrote `45.5` asked for something specific. The warning goes to the module logger and never to stderr, which the subprocess transport parses. `_deadline` is a context manager that arms a daemon `threading.Timer`, sets its Event before invoking the driver's cancel so the flag is observable by whoever catches the resulting error, swallows and logs a cancel that raises from the timer thread, and disarms in a `finally`. `_ResourceLimit` is the internal marker a cancelled statement will unwind on. Spec: ACE-038 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…te (S2)
Wires S1's watchdog into one engine and turns a cancelled statement into the
refusal the contract has been reserving `resource_limit` for. SQLite goes first
because it is in-process, needs no network, and `Connection.interrupt()` is a
genuine cancel rather than a request the driver may ignore.
`_run_sqlite` resolves its budget once and wraps BOTH `cur.execute` and
`_collect_cursor` in `_deadline(conn.interrupt, timeout_s)`. The fetch is inside
the clock deliberately: `_collect_cursor` pulls `cap + 1` rows in a single
`fetchmany`, and on a cursor that streams its result that pull is where the scan
happens, so a clock stopping at `execute` would bound the cheap half.
**The flag is the classification, and only the flag.** A cancelled statement
raises `OperationalError("interrupted")`, so neither the message nor the elapsed
clock can be the test — both are things an ordinary database error can have by
coincidence, and reading either would tell an unlucky caller to narrow a query
that never ran long. `_deadline` sets its Event before the cancel lands, so
"did WE stop this?" is answered without inference. Asserted in both directions:
the same error text with the flag clear stays `failed`, including when it
arrives well past the budget.
An `except _ResourceLimit: raise` sits ahead of `_run_sqlite`'s `code=5`
catch-all, and ahead of both handlers in `execute_guarded`, so the marker
reaches the chokepoint instead of being reported as the database's outcome. The
other nine engines are unchanged and remain bounded only by the supervisor.
The refusal quotes the configured budget — a deployment setting, not a data
value — and names no environment variable: on the served path the caller is an
assistant with no shell and no deployment, so "raise AGAMI_SQL_TIMEOUT_S" is
advice aimed past it at an operator who is not in the conversation.
Contract changes to existing tests, each because the contract changed:
* `test_ace035_no_enumeration.py` — `resource_limit` had a `_NO_VECTOR` entry
saying "the day something emits it, it needs a vector here". Today is that
day: the entry is gone and a real vector takes its place, a runaway CTE that
passes every gate and is stopped inside the executor. It is the only row in
the matrix whose refusal is produced AFTER the model has been loaded and
consulted, which is the state in which a helpful message has the declared
surface closest to hand. The matrix is 20 rows, not 16.
* `test_ace035_guardrail_audit.py` — `test_no_gate_produces_the_resource_limit_refusal`
is renamed to `test_the_supervisor_kill_does_not_borrow_the_resource_limit_refusal`.
Not weakened and not deleted: it drives the same branch and makes the same
assertions. Only its premise moved — "no gate produces it" is no longer true,
while "this branch must not borrow it" is now the sharper claim.
* `guardrail.py` and `tools.py` docstrings asserting "nothing imposes one yet"
now describe the producer.
Spec: ACE-038
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hat back it (S3)
S2 proved the contract on one engine. The remaining nine now run under the same
watchdog, and three of them also carry a native server-side bound behind it.
**Each engine names its own cancel, explicitly.** A duck-typed probe was considered
and is provably wrong, in both directions. `pymysql.Connection` has NO `cancel()` —
its methods are `close`, `_force_close`, `kill` — so a probe falls through to
`close()`, which sends COM_QUIT down the very socket the blocked statement owns and
can itself block; `_force_close()` shuts the socket outright and is the one that
unblocks. In the other direction `oracledb.Connection` DOES have `cancel()`, so a
probe ordering `cancel` first would silently pick it for Oracle and never notice
that Snowflake, Databricks and Trino put theirs on the CURSOR. So: postgres/redshift
`conn.cancel`, mysql/mariadb `conn._force_close`, sqlserver the `_mssql` connection's
`cancel` behind pymssql's private `_conn` (its DB-API connection has none), oracle
`conn.cancel`, databricks and trino `cur.cancel`, duckdb `conn.interrupt` — where
`cur` IS `conn`, because `conn.execute` hands back the connection itself.
Snowflake departs from the plan on evidence: the connector exposes no `cancel()` on
either the connection or the cursor. `SnowflakeCursor.abort_query(qid)` is the only
public abort, so the cancel resolves the cursor's `sfqid` and calls it, and no-ops
before the statement has a query id — the window the session parameter covers.
Every engine wraps BOTH `cur.execute` and `_collect_cursor` in the deadline, resolves
its budget ONCE per call, and converts to `_ResourceLimit` only when the Event is set.
An `except _ResourceLimit: raise` now sits ahead of all ten `code=5` catch-alls,
BigQuery included even though nothing there raises it today: the rule is that no
engine may relabel the marker, and a rule checkable on nine of ten is not one.
**Postgres needed a structural fix before the deadline could work there at all.**
Closing a server-side cursor sends `CLOSE agami_bounded`. On the timeout path the
transaction is already aborted, so that statement raises in turn — and from inside
`__exit__`, where a new exception REPLACES the one being propagated. The marker
vanished and the catch-all reported every Postgres timeout as a `failed` envelope.
The named cursor comes out of its `with` and is closed by hand, with that raise
swallowed; `with conn` stays, because it is the transaction and the rollback.
**Native bounds on exactly three engines, at `timeout_s + 5`.** The skew is the
design: our watchdog fires first, so its flag stays the sole classification signal,
and the native bound is the backstop for a process that dies mid-statement.
Postgres sets `SET LOCAL statement_timeout` in ms on the same transaction, on a
regular cursor, before the named cursor declares — not the libpq `options` startup
parameter, which a transaction-mode pooler can reject at connect. Snowflake sets
`STATEMENT_TIMEOUT_IN_SECONDS` via `session_parameters`, where an abandoned statement
still bills credits. BigQuery sets `job_timeout_ms` and gets NO watchdog: there is no
connection to cancel and the blocking call is `job.result()`, reached only after
`client.query()` returns, so at the instant a watchdog would fire there is nothing in
hand. That residual is recorded in the code and asserted in the tests — on BigQuery a
client-side stall yields `failed`, not `resource_limit`. No other engine gets one.
Tests are table-driven over all ten `_run_*` functions, against fake driver modules
installed in `sys.modules` (the engines do their own `import <driver>`), so the whole
matrix runs on a machine with no database drivers at all:
* every engine re-raises the marker; and, inversely, no engine mistakes an ordinary
driver error for it. A coverage test asserts the table names every `_run_*` in the
module, so an engine added without the clause fails the suite.
* the named cancel is the one actually invoked. The fake connection exposes EVERY
cancel-shaped method these drivers have — `cancel`, `interrupt`, `_force_close`,
`kill`, `close`, a cursor `cancel` and a cursor `abort_query` — each logging a
distinct marker, so reaching for the wrong one lands on the wrong marker.
* Postgres: the marker survives a named-cursor close that raises (fails without the
fix, as `failed`/`syntax` carrying "current transaction is aborted"), the
transaction still rolls back, and `SET LOCAL` runs on the same transaction before
the named cursor with `(budget + 5) * 1000` ms.
* the three native bounds and their values; and that no fourth engine grew one.
* a real cartesian bomb bounded on a real in-process DuckDB. It skips here, since
the suite deliberately installs no DB drivers.
`test_ace044_bounded_fetch.py`'s Postgres fake gains `cancel()` and a `params`
argument on `execute` — the calls the engine now really makes. Neither assertion in
it changed.
Spec: ACE-038
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e supervisor (S4) The inner deadline lives inside the BUILT-IN executor's ten engine functions, which leaves two holes the criterion "the limit applies identically on stdio and HTTP, and no executor escapes it" does not survive. `execute_guarded` now bounds `executor.execute` itself, and the subprocess supervisor stops being a number of its own. **An injected executor was entirely unbounded.** `tools` routes to `_run_in_process` with the injected executor whenever a consumer supplies one, which is what the hosted connection-reuse path does — so that deployment had no per-statement bound at all. And on the built-in path BigQuery has no watchdog (no connection object to cancel, and the blocking call is reached only after `client.query()` returns), so a client-side stall there was unbounded too. Both are closed by the SAME layer, deliberately: applying the outer bound only to non-built-in executors would leave the BigQuery hole open while looking closed. **The mechanism is a daemon worker joined with the budget, because this layer holds nothing it can cancel.** An arbitrary `Executor` exposes one method and no connection, so the only thing this code owns is its own WAIT. On expiry the worker is abandoned, not stopped: it is still inside the driver call and may hold a database connection — and a statement still running on the server — until that call returns on its own. That cost is stated in the code rather than papered over, and it is exactly why this bound is the outer one and the watchdog is set to win. The refusal says so too: it does not claim the statement was cancelled, because nothing was. **Exception type survives the thread hop**, which is the subtle part. `BaseException` is captured in the worker and re-raised in the caller, so `except _ResourceLimit` and `except ExecutorError` still match and a driver's deep `sys.exit` still reaches the `SystemExit` net in `tools._run_in_process` instead of vanishing into a silent wait. The call runs inside `copy_context()`: a new thread starts with an EMPTY context, so without it the request-scoped `_timeout_override` and `_max_rows_override` would read as unset and every in-process call would quietly fall back to the deployment defaults. **One refusal, however many layers are armed.** The outer marker is a `_ResourceLimit` subclass, so the single existing handler mints exactly one envelope and no second one is possible; when the inner watchdog fired, its marker is what arrives there and the outer join had already returned, long before its own budget. **The supervisor is now `budget + 60`, from the same resolver.** A hardcoded 240s became the FIRST bound to fire for any statement budget approaching it, turning a refusal that can name the statement into a `failed`/`timeout` that names nothing. Only the DURATION moved: an unresponsive child is still `failed`/`timeout` and never `resource_limit`, because all we observe is that the child stopped responding — it may have hung in connect, in credential resolution or in loading the model. The child inherits `os.environ` and `subprocess.run` passes no `env=`, so it resolves the identical budget. The four bounds are one ordered family from one resolved budget — watchdog < native (+5) < outer (+10) < supervisor (+60) — and the tests assert that as a SINGLE fact rather than one skew at a time, since the order is the property and four separate assertions would each stay green while the family drifted apart. Ten new tests: a blocking injected executor bounded and refused (verified to fail without the outer layer); the outer refusal's wording, which must not say "cancelled"; the inner refusal winning a real cancel race with both layers armed at their real values; `ExecutorError` and a plain `Exception` still landing in their own handlers with the same envelope and kind as before; both ContextVars visible on a thread asserted not to be the caller's; the supervisor's computed value at a RAISED budget (300 -> 360, where the fixed 240 would have fired a minute early) read off the `subprocess.run` call rather than waited out, and its verdict unchanged; and both surfaces — a real `python -m mcp_harness` subprocess and `TestClient` over `/mcp` — returning identical refusals for the same runaway statement. No existing test changed a claim. Two fixtures gained setup only: `_reset_override` now also resets the row cap (the worker copies the whole context, so a leaked cap would be visible to the next test's executor), and `audited` sets the two vars the HTTP surface needs to mint a token. A new autouse fixture clears `_INJECTED_EXECUTOR` around every test in the file, since `create_app()` installs one as a side effect. Spec: ACE-038 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… budget (S5) The in-process tests prove the refusal; none of them can prove the cancel. A client that merely stopped waiting produces the identical Envelope while the backend keeps scanning for nobody, so the security claim has to be asserted against the SERVER's view. `test_postgres_timeout_integration.py` does that on a live Postgres, from a second connection: the statement is observed running in `pg_stat_activity`, and after the refusal it is gone. It also proves the native backstop landed by disarming our watchdog and letting Postgres kill the statement itself — "canceling statement due to statement timeout" is wording a client cancel cannot produce. The runaway is three narrow cross-joined series rather than one wide one: a function scan materializes into a tuplestore, so the obvious `generate_series(1, 1e11)` burns time by filling the server's temp tablespace, and a test that leaves the disk full has done more damage than the bug it checked for. Opt-in on the existing gate (skips unless AGAMI_IT_PG_PASSWORD is set), so CI without a database is unaffected. `AGAMI_SQL_TIMEOUT_S` reaches both env examples — it was the one bound an operator had no way to discover. AGAMI_SQL_MAX_ROWS is left undocumented on purpose: its truncation behaviour is being changed by separate work. The four ACE-038 comment markers in execute_sql.py (and three more in tools.py) sat on row-cap and result-transfer code that belongs to a different spec. The prose stays; the ids go. Spec ids belong in the PR trailer.
…hat eats the refusal, the uncapped leak The per-statement deadline held on the happy path and leaked its guarantee on three edges either side of it. `_deadline`'s disarm was a request, not a join: `Timer.cancel()` only sets the timer's internal Event, so a `fire` that had already passed its own check ran anyway — setting the flag after the block returned, and delivering a cancel to a connection the engine had moved on from. A lock plus a disarmed flag make the disarm wait for a cancel in flight and make a losing `fire` land nowhere, so "checked after the watchdog is disarmed, so the flag is final" is now true. Three engines closed the connection unguarded in `finally`, where a raised exception REPLACES the one propagating — destroying the marker the `except _ResourceLimit: raise` above had just re-raised, and handing the caller an unclassified failure instead of the refusal. MySQL was the most exposed: its cancel destroys the socket that close then writes to. All ten now match. The abandoned worker the outer bound costs had no ceiling. On the injected path there is no inner watchdog, so every slow statement abandons one, and bounding the caller's wait removed the backpressure that used to cap them — leaks accumulated until the pool belonged to work nobody was waiting for. A process-wide cap refuses fast at the chokepoint instead of starting leak N+1, and says the executor is saturated rather than blaming a statement that never ran. `_timeout_override` is deleted rather than documented. It outranked the environment in the parent and could not cross the fork, so the supervisor bound derived from it could sit below the child's actual budget and fire first, inverting the ordered family. The budget now has exactly one configuration surface, pinned structurally and across a real subprocess. Also: the guardrail contract no longer understates the reach of the rule (all ten engines are wired; BigQuery's server-side-only bound is the one real residual); the resolver uses `isdecimal` so a superscript digit cannot raise out of an unguarded call site, and warns whenever the budget differs from what the operator wrote rather than only when the text was unreadable; the CLI adapter renders the marker as a refusal instead of a traceback. Tests: the fetch-inside-the-deadline criterion is now pinned on all nine watchdog engines rather than two (mutation-verified against sqlserver, oracle and snowflake); the both-surfaces test asserts an audit row per transport; the cancel-that-raises test polls instead of sleeping; and the short budget in the no-enumeration suite is scoped to the vector that needs it.
There was a problem hiding this comment.
Pull request overview
Adds an app-layer, per-statement SQL timeout that is enforced consistently across all executor transport routes (in-process and forked), with a single resolved budget (AGAMI_SQL_TIMEOUT_S) fan-out into ordered time bounds and a single “did our watchdog fire?” classification signal. This closes the previous gap where only connect/login were bounded and long-running statements could run unbounded (or be cut off only by a fixed subprocess supervisor timeout).
Changes:
- Implement per-statement watchdog cancellation plus ordered native/outer/supervisor bounds derived from
AGAMI_SQL_TIMEOUT_S, including abandoned-worker capping for the outer bound. - Wire driver-specific cancellation mechanisms and native server-side timeouts (Postgres/Snowflake/BigQuery) while preserving correct refusal vs failure classification.
- Add comprehensive tests (SQLite contract, engine matrix, outer-bound behavior) plus an opt-in live Postgres integration test; update guardrail contract text and deployment env examples.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_postgres_timeout_integration.py | New opt-in live Postgres integration assertions (server-side visibility + native timeout proof). |
| tests/test_ace044_bounded_fetch.py | Adjust fakes to tolerate Postgres SET LOCAL ... parameterized execute + cancellation/close hooks. |
| tests/test_ace038_timeout.py | New large contract test suite for timeout resolution, watchdog semantics, engine matrix, and outer bound behavior. |
| tests/test_ace035_no_enumeration.py | Extend refusal scanning matrix to include resource_limit vector; update narrative/pins accordingly. |
| tests/test_ace035_guardrail_audit.py | Rename/pivot supervisor-timeout test to ensure it doesn’t borrow resource_limit semantics. |
| plugins/agami/skills/agami-deploy/bundle/agami.env.example | Document AGAMI_SQL_TIMEOUT_S in the bundled deploy example env file. |
| plugins/agami/lib/guardrail.py | Update rule docs to reflect resource_limit now has a producer (per-statement timeout). |
| plugins/agami/lib/execute_sql.py | Vendored executor: implement watchdog/native/outer bounds, cancellation wiring, and refusal construction. |
| packages/agami-core/src/tools.py | Derive subprocess supervisor timeout from _resolve_timeout_s() instead of fixed 240s. |
| packages/agami-core/src/mcp_http.py | Update docs/comments to reflect new bounded execution behavior. |
| packages/agami-core/src/guardrail.py | Update rule docs to reflect resource_limit now has a producer (per-statement timeout). |
| packages/agami-core/src/execute_sql.py | Core executor: implement watchdog/native/outer bounds, cancellation wiring, and refusal construction. |
| deploy/agami.env.example | Document AGAMI_SQL_TIMEOUT_S in the root deploy example env file. |
Suppressed comments (2)
packages/agami-core/src/execute_sql.py:1789
_execute_boundedcan incorrectly treat a call as abandoned even if the worker finished, because the main thread decidesabandoned = not outcome.get('finished')while the worker setsoutcome['finished']under_abandoned_lockin itsfinally. If the worker finishes right around the join timeout, it may be unable to acquire the lock to setfinishedbefore the main thread acquires it, causing a falseresource_limitrefusal and incorrect abandoned-worker accounting. Useworker.is_alive()as the authoritative source of whether the thread is still running afterjoin(), and keep the lock only for updating_abandoned_workers/outcome['abandoned'].
worker.join(timeout_s + _OUTER_BOUND_SKEW_S)
with _abandoned_lock:
abandoned = not outcome.get("finished")
if abandoned:
_abandoned_workers += 1
outcome["abandoned"] = True
plugins/agami/lib/execute_sql.py:1789
_execute_boundedcan incorrectly treat a call as abandoned even if the worker finished, because the main thread decidesabandoned = not outcome.get('finished')while the worker setsoutcome['finished']under_abandoned_lockin itsfinally. If the worker finishes right around the join timeout, it may be unable to acquire the lock to setfinishedbefore the main thread acquires it, causing a falseresource_limitrefusal and incorrect abandoned-worker accounting. Useworker.is_alive()as the authoritative source of whether the thread is still running afterjoin(), and keep the lock only for updating_abandoned_workers/outcome['abandoned'].
worker.join(timeout_s + _OUTER_BOUND_SKEW_S)
with _abandoned_lock:
abandoned = not outcome.get("finished")
if abandoned:
_abandoned_workers += 1
outcome["abandoned"] = True
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two from the Copilot review, both real. The disarm set its flag after `Timer.cancel()`. Since cancel is not a join, that left a window between the bounded block returning and the flag being set, in which an already-expired `fire` could take the lock and mark a statement that had in fact completed. Claiming the race under the lock first closes it; the cancel becomes belt and braces for a timer that has not started. The new test fires the callback from inside `cancel()`, which puts a `fire` in exactly that gap on every run rather than once in a thousand. The integration test's DSN builder used `urllib.parse.quote`'s default, which leaves `/` alone. A `/` in a password ends the userinfo early and one in a database name ends the path early, either way building a URL that parses cleanly and connects somewhere else. Escaped with `safe=""`, and pinned by a test that needs no database, so it runs in ordinary CI. Copilot also flagged the abandoned-worker accounting in `_execute_bounded` as the same class of issue. It is not: the worker sets `finished` under the same lock the abandonment check takes, so a worker that returns in the race window is counted as returned and its result is used. Left as is.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 35623139 | Triggered | Generic Password | 449ddfc | tests/test_postgres_timeout_integration.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…rd-shaped literal The DSN escaping test used "p@ss/w:rd", which reads as a credential to a secret scanner even though it is a fixture. The value now names the characters it is exercising, which is both quieter and clearer.
…t a gap `readonly-grants.md` told operators that "a per-statement query timeout and error-text/recon hardening are not in place yet" and that until one shipped they should set `statement_timeout` on the role themselves. The timeout half of that is no longer true. Recon and error-text hardening still are, so the sentence is split rather than deleted. Two residuals go in with the good news, because an operator planning capacity needs both: BigQuery's bound is the server-side job timeout alone (a query there is a job with no connection to cancel), and a per-statement bound is not a concurrency bound, so enough simultaneous queries still load a database. The engine-specific notes for BigQuery and SQLite/DuckDB say which side of that line they fall on. Merges origin/main to pick up 41cfd7d, which is the commit that added the line this corrects.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
plugins/agami/lib/execute_sql.py:1799
- In
_execute_bounded, the “finished” flag is used to decide whether the join timed out. That can misclassify a call as abandoned if the worker finishes right afterjoin()returns (timeout) but before it acquires_abandoned_lockto setoutcome['finished'], causing a falseresource_limitrefusal even though the executor returned in time. Useworker.is_alive()after the join to decide abandonment, then markoutcome['abandoned']only when the thread is still alive.
worker.join(timeout_s + _OUTER_BOUND_SKEW_S)
with _abandoned_lock:
abandoned = not outcome.get("finished")
if abandoned:
_abandoned_workers += 1
packages/agami-core/src/execute_sql.py:1799
- In
_execute_bounded, the “finished” flag is used to decide whether the join timed out. That can misclassify a call as abandoned if the worker finishes right afterjoin()returns (timeout) but before it acquires_abandoned_lockto setoutcome['finished'], causing a falseresource_limitrefusal even though the executor returned in time. Useworker.is_alive()after the join to decide abandonment, then markoutcome['abandoned']only when the thread is still alive.
worker.join(timeout_s + _OUTER_BOUND_SKEW_S)
with _abandoned_lock:
abandoned = not outcome.get("finished")
if abandoned:
_abandoned_workers += 1
This was referenced Aug 1, 2026
The comment said a worker finishing between the join timing out and the lock being taken "is counted as having returned, and its result is used rather than a refusal invented over the top of it". That holds for the slot accounting and not for the outcome: whichever thread reaches the lock first decides, and when the caller wins it reads `finished` unset and raises, refusing a call whose work had completed. Corrected in place and filed as #177 rather than fixed here, because it fails closed, the built-in path is shielded by the watchdog firing a full skew earlier, and the reachable case needs an injected executor returning inside the few microseconds between the join expiring and the next lock acquisition. The comment also warns off `worker.is_alive()`, which is the obvious repair and does not work — a worker parked in its `finally` is alive and reads as abandoned in the same losing ordering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
There was no per-statement timeout anywhere in the executor.
mainhad onlyconnect_timeout=10andlogin_timeout=15, which bound connecting, and a fixedsubprocess.run(timeout=240)supervisor that killed our own process while the database kept working. The in-process path had no bound at all. Since the read-only role is not mandated to carry a role-levelstatement_timeout, this app-layer control is the sole availability guarantee.Concretely: a runaway recursive CTE ran for 163 seconds unbounded on the base commit. It now refuses in ~1.1s, identically on all four transport routes.
This fills the
RULE_RESOURCE_LIMITproducer slot that ACE-035 declared and deliberately left empty.Changes
One resolved budget, four ordered bounds.
AGAMI_SQL_TIMEOUT_S(default 30, integer-only) resolves once per call from the environment and nothing else, and every other bound derives from it: inner watchdog attimeout_s, native server-side bound at+5, outer chokepoint bound at+10, subprocess supervisor at+60. Their order is asserted together as one fact so no future change can let them drift apart. Resolving from the environment alone is deliberate — it is the forked child's source of truth, so the order cannot invert across the process boundary.Classification is one boolean. Did our watchdog fire? Set means we stopped it, so it is a refusal. Unset means the exception is the database's and propagates untouched. There is no SQLSTATE table, no cancellation-string matching, and elapsed time is never a signal — a post-deadline OOM, disk error or admin kill is
failed, not a refusal telling the caller to narrow a query that was fine.The cancel is a named per-driver map, not a duck-typed probe. A probe was the original design and is provably wrong on three drivers:
pymysql.Connectionhas nocancel()at all (so a probe falls through toclose(), which sends COM_QUIT on the socket the blocked query owns), the Snowflake connector has nocancel()anywhere (the cursor'sabort_query(sfqid)is the real one), and pymssql's DB-API connection exposes none either. Each would have failed silently rather than loudly.Native bounds on three engines only — Postgres
SET LOCAL statement_timeout, BigQueryjob_timeout_ms, SnowflakeSTATEMENT_TIMEOUT_IN_SECONDS— set at+5so our watchdog always wins and its flag stays the sole signal.Postgres needed a structural fix. Its named cursor sat inside
with conn; on exit the cursor issuedCLOSEagainst an aborted transaction, raising a second exception that replaced the timeout marker. The refusal was lost on the one engine that had awith conn.Both executor paths are bounded. The inner deadline needs the connection, which only the builtin executor has, so an injected executor (the hosted connection-reuse path) would have been entirely unbounded. The outer layer at the
executor.execute()chokepoint closes that, and the BigQuery no-cancel hole with it.Decisions worth a reviewer's eye
job.result(), reached only afterclient.query()returns, so at the moment a watchdog fires there is nothing to cancel. A client-side stall there yieldsfailedrather than aresource_limitrefusal. Named in the security sign-off rather than hidden behind a uniform-looking wrapper.Verification
uv run dev.py checkgreen — 2250 passed, 4 skipped, 1 xfailed, up from 2110 at base. Ruff, format, gitleaks and the vendored-slice drift check all pass.pg_stat_activity, the caller getsrefused/resource_limit, and within 2s it is gone. The assertion was falsified to prove it has teeth — with no cancel sent, the same statement is stillactiveat t+12s. A separate test disarms our watchdog and lets the native bound fire, and Postgres reports "canceling statement due to statement timeout", wording a client-sideconn.cancel()cannot produce.except _ResourceLimit: raiseclauses fails the matrix. Reverting Postgres to the named-cursor-inside-withshape reproduces the exact predicted bug. Reversingfired.set()andcancel()fails. Droppingcopy_context()fails. Moving_collect_cursorout of the deadline on sqlserver, oracle or snowflake fails.uvx --with duckdb), and three live-Postgres tests gated onAGAMI_IT_PG_PASSWORD.Stated residuals: BigQuery as above; on connection-scoped engines a hard process kill relies on connection teardown rather than a bound the engine set for itself; the
pg_stat_activityassertion runs on the opt-in fixture, not in CI, because there is no Postgres service container in the workflow.Checklist
uv run dev.py checkgreen (ruff · format · pytest · gitleaks · lib-drift)execute_sqlchokepoint, no per-surface fork, no second SQL parserSpec: ACE-038