Skip to content

Fix Snowflake SQL API wait_for_query timing out on finished queries - #73761

Merged
potiuk merged 2 commits into
apache:mainfrom
KarthikMohankumar:snowflake-wait-for-query-timeout
Oct 5, 2026
Merged

potiuk merged 2 commits into
apache:mainfrom
KarthikMohankumar:snowflake-wait-for-query-timeout

Conversation

@KarthikMohankumar

Copy link
Copy Markdown
Contributor

SnowflakeSqlApiHook.wait_for_query checked the elapsed time before it looked at the status it had just fetched. If the status call itself returned after the timeout, the method raised TimeoutError even when that status showed the query had already finished (success or error).

This is easy to hit in practice. Each status call goes through _make_api_call_with_retries, which retries 429/503/504 and connection errors with exponential backoff, so one call can take several seconds. The only caller in the provider, the OpenLineage helper _run_single_query_with_api_hook, uses timeout=3. A slow but successful status check therefore surfaced as a timeout, and the query-history details for lineage were dropped.

Changes:

  • the status is checked first; TimeoutError is raised only while the query is still running
  • elapsed time uses time.monotonic() instead of time.time(), so wall-clock adjustments (NTP sync, DST) cannot cut the wait short or stretch it

Tests:

  • new test_wait_for_query_returns_finished_status_after_timeout_elapsed (parametrized for success and error): a single status call that takes longer than the timeout returns the finished status instead of raising. It fails with the previous check order.
  • test_wait_for_query_timeout_error now drives a fake monotonic clock, because time_machine does not move time.monotonic(). It still asserts the same number of polls and sleeps before TimeoutError.

Checks run locally:

  • uv run --project providers/snowflake pytest tests/unit/snowflake/hooks/test_snowflake_sql_api.py: 91 passed
  • uv run --project providers/snowflake pytest tests/unit/snowflake/utils/test_openlineage.py: 48 passed
  • prek run --stage pre-commit on the changed files: 45 hooks passed. check-provider-yaml-valid needs the Breeze CI image and could not run locally (no provider.yaml change here); ast-grep was skipped because it could not download its Node runtime.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5.5)

Generated-by: Claude Code (Opus 5.5) following the guidelines

@boring-cyborg boring-cyborg Bot added area:providers provider:snowflake Issues related to Snowflake provider labels Sep 26, 2026
@KarthikMohankumar KarthikMohankumar changed the title Snowflake wait for query timeout Fix Snowflake SQL API wait_for_query timing out on finished queries Sep 26, 2026

@justinpakzad justinpakzad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR, the change makes sense to me.

wait_for_query checked the elapsed time before looking at the status it had just fetched, so a status call that returned after the timeout raised TimeoutError even when the query had already succeeded or failed. Status calls can take several seconds because they retry with exponential backoff, and the OpenLineage integration waits with a 3-second timeout, so a finished query could be reported as timed out and its lineage details dropped. Elapsed time now also uses the monotonic clock so wall-clock adjustments cannot shorten or extend the wait.
@potiuk
potiuk force-pushed the snowflake-wait-for-query-timeout branch from c8ef66b to 6aa8982 Compare October 4, 2026 23:48

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ordering fix is right: a query that already finished is reported even when the status call itself ran past the timeout, and a query still running past the timeout still raises. time.monotonic() is the right clock for this, and mocking it is needed since time_machine doesn't control the monotonic clock. Thanks!


Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 3bd4df6 into apache:main Oct 5, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:snowflake Issues related to Snowflake provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants