Skip to content

Add a query statement_timeout so broad searches fail fast - #198

Open
skearnes wants to merge 1 commit into
mainfrom
query-timeout
Open

Add a query statement_timeout so broad searches fail fast#198
skearnes wants to merge 1 commit into
mainfrom
query-timeout

Conversation

@skearnes

@skearnes skearnes commented Jun 24, 2026

Copy link
Copy Markdown
Member

Substructure/similarity search is ~150 ms for selective patterns, but a pathological broad pattern (e.g. bare benzene) can run for many seconds. This caps query runtime so those fail fast with a helpful message instead of hanging.

  • get_cursor sets statement_timeout (default 30 s, tunable via ORD_INTERFACE_QUERY_TIMEOUT_MS).
  • The resulting QueryCanceled becomes a 400 with a clear detail ("too broad; refine your search — a more specific substructure or a higher similarity threshold") on:
    • the synchronous /query path, and
    • the async submit_queryfetch_query_result path (run_task stores a {"error": "timeout"} marker so the task doesn't sit "pending" forever).

Normal queries are unaffected (the existing suite passes under the 30 s cap). Added test_query_timeout_returns_400 (mocks QueryCanceled for a deterministic check of the 400 path).

🤖 Generated with Claude Code

Greptile Summary

This PR adds a PostgreSQL statement_timeout (default 30 s, tunable via ORD_INTERFACE_QUERY_TIMEOUT_MS) to every database connection opened by get_cursor(), so pathologically broad substructure/similarity searches fail fast rather than hanging. The resulting QueryCanceled exception is translated to a user-friendly HTTP 400 on both the synchronous /query path and the async submit_query / fetch_query_result path.

  • get_cursor() injects -c statement_timeout=<ms> into the connection options string, applying the cap uniformly to all DB connections.
  • run_task catches QueryCanceled and stores an {"error": "timeout"} sentinel in Redis, which fetch_query_result detects and converts to a 400 instead of leaving the task forever "pending".
  • A new unit test mocks run_queries to raise QueryCanceled and confirms the 400 + descriptive detail message on the sync path.

Confidence Score: 4/5

Safe to merge; the core timeout logic is correct and the happy path is unchanged. The two flagged items are operational polish rather than functional defects.

The timeout is wired correctly at the psycopg connection level, the QueryCanceled → 400 translation works on both code paths, and the existing test suite was verified to pass under the 30 s cap. The two concerns are: (1) int() on ORD_INTERFACE_QUERY_TIMEOUT_MS without a fallback will crash every cursor creation if an operator passes a non-integer value like '30s', turning a narrow config mistake into a service-wide outage; and (2) background-task timeouts produce no distinguishable log output, making them hard to detect in production. Neither affects normal operation.

ord_interface/api/search.py — specifically the env-var parsing in get_cursor() and the silent exception handling in run_task.

Important Files Changed

Filename Overview
ord_interface/api/search.py Adds statement_timeout to every DB connection, catches QueryCanceled on both the sync /query path and the async run_task path, and surfaces a 400 with a user-friendly message. Two minor concerns: the int() conversion of the env var has no error handling, and background-task timeouts are logged at debug/no distinction from successes.
ord_interface/api/search_test.py Adds test_query_timeout_returns_400 which mocks run_queries to raise QueryCanceled and verifies the 400 status and "broad" keyword in the detail message. Covers the sync path; no async-path timeout test is added.

Reviews (1): Last reviewed commit: "Add a query statement_timeout so broad s..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

A pathological search (e.g. a very broad substructure) could run for many seconds
or hang. Set a statement_timeout on the search connection (default 30s, tunable
via ORD_INTERFACE_QUERY_TIMEOUT_MS) and translate the resulting QueryCanceled into
a clear 400 ("too broad; refine your search") on both the synchronous /query path
and the async submit_query/fetch_query_result path (which stores a timeout marker
instead of leaving the task pending forever).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
)
# Cap query runtime so a pathological (e.g. very broad substructure) search
# fails fast with a clear message instead of hanging; tunable via env.
timeout_ms = int(os.getenv("ORD_INTERFACE_QUERY_TIMEOUT_MS", "30000"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Invalid env var silently crashes all cursor creation

int(os.getenv(...)) raises ValueError if the variable is set to a non-integer value (e.g. "30s" or "30000ms"). Because this runs inside get_cursor(), which is called for every single endpoint, a misconfigured ORD_INTERFACE_QUERY_TIMEOUT_MS would make the entire service return 500 on all requests — not just search ones. Wrapping this in a try/except (falling back to the default) or validating at startup would make the failure mode much narrower.

Comment on lines +279 to 283
except psycopg.errors.QueryCanceled:
# Surface the timeout via fetch_query_result instead of leaving the task
# forever "pending".
result = {"error": "timeout"}
logger.debug(f"Finished task {task_id}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Timeout in background task produces no log output

When QueryCanceled is caught in run_task, the exception is silently swallowed and execution falls through to the existing logger.debug(f"Finished task {task_id}") line. From the logs, a timed-out task looks identical to a successful one. A logger.warning (or at least logger.info) here would make it possible to detect and quantify broad-query timeouts without having to query Redis directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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