fix(db-bigquery): poll query results until the configured timeout

[girishjeswani]

Summary

The BigQuery connector could not complete any query that ran longer than about
six minutes, regardless of configuration. runSQL (via
createBigQueryJobAndGetResults) fetched results with job.getQueryResults
using a hardcoded 2-minute timeoutMs, wrapped in a fixed 3-retry loop. Each
call waits ~2 minutes and throws The query did not complete before 120000ms
while the job is still running fine; after 3 retries the loop gives up at
~6 minutes. The 120000ms in the message is a single poll's window, not the
real ceiling. The connection's configured timeoutMs never reached this path
(it only bounded jobTimeoutMs), so no configuration could extend it.

This resolves the long-standing TODO in createBigQueryJobAndGetResults, which
already called for exactly this: "extend the wait for results using a timeout
set by the user, and probably needs to loop to check for results."

Root cause

getQueryResults is a request-level wait, not a job-level one. Per the BigQuery
docs a single call returns after at most ~200s regardless of the requested
timeoutMs, reporting jobComplete: false while the job is still running; the
documented pattern is to keep calling until jobComplete is true. The old code
treated the incomplete response as a failure and gave up after a fixed number of
retries.

Fix

The connection's configured timeoutMs now drives one resolved value that
bounds both the BigQuery job and the client-side wait, and getQueryResults is
polled to completion instead of being retried a fixed number of times:

• Shadow timeoutMs to the job's jobTimeoutMs so BigQuery cancels a runaway
  job server-side, and poll getQueryResults up to the same deadline on the
  client, totaling the wait across calls (BigQuery returns each call after ~200s
  regardless of the requested timeoutMs).
• Structured completion signal: "still running" is read from the callback
  overload's apiResponse.jobComplete === false, not a regex on the client's
  error string, so it cannot silently regress if the wording changes.
• On reaching the client deadline, cancel the job before throwing. BigQuery's
  jobTimeoutMs should already be cancelling it, but this covers the case where
  that timeout error has not come back yet, so the job does not keep running
  (and billing) after we have stopped waiting.
• Abort-aware: the abort signal is checked each iteration and races the in-flight
  poll, so a cancel settles the call promptly (and cancels the job) instead of
  waiting out the deadline.
• Clamped per-poll wait: min(GET_QUERY_RESULTS_POLL_MS, remaining), so short
  timeouts fail fast and the deadline is not overshot by a poll.
• Minimum spacing between polls: if BigQuery returns jobComplete: false sooner
  than the per-poll wait we requested, the loop waits out only the shortfall
  below a 1s floor (bounded by the remaining deadline) instead of busy-polling.
• Actionable deadline error naming the configured timeout and the knob to raise.
• Bounded transient retry preserved for the intermittent access-denied error on
  first fetch, replacing the old blanket 3-retries-on-everything.
• timeoutMs resolves to a positive number or the default: unset, blank,
  non-numeric, zero, or negative all fall back to the 10-minute default; only a
  positive value overrides it. A non-positive value must not reach the job, or
  it would cancel on the first poll.

Scope

BigQuery-only, by design. Among the connectors only BigQuery hand-rolls result
polling: Snowflake passes its configured timeout through, Databricks and Trino
delegate polling to their client libraries, and Postgres/MySQL/DuckDB are
synchronous. This brings BigQuery in line with the others rather than adding a
shared abstraction. Per review, two follow-ups: aligning the Snowflake
connector's 0 handling with this (open as #3022), and adding
a timeoutMs setting to Databricks.

Tests

• get_query_results_polling.spec.ts drives the poll loop directly with a
  scripted fake job and an injected clock: immediate completion (with no cancel);
  polling past "still running" until done; the minimum inter-poll spacing
  (waiting only the shortfall when a poll returns early, and not waiting when a
  poll already blocked at least the interval); per-poll clamp to the remaining
  deadline; the actionable deadline error and that the job is cancelled on
  give-up; bounded transient retry then rethrow; that a still-running poll does
  not reset the transient-retry budget; and prompt exit on abort (mid-poll,
  already-aborted, and around the inter-poll wait).
• bigquery_connection.unit.spec.ts drives runSQL against a stubbed job to
  lock the seam: a positive timeoutMs is passed to the job as jobTimeoutMs;
  '0', a whitespace-only value, and a negative value all fall back to the
  default jobTimeoutMs; and a jobComplete: false response is polled rather
  than surfaced as empty data.

tsc, eslint, and the tests all pass.

Notes

• Config-value resolution: unset, blank, non-numeric, zero, and negative all
  resolve to the 10-minute default; only a positive value overrides it. What
  changes functionally is that the resolved timeout now actually bounds the
  results wait (previously it only bounded jobTimeoutMs, and the results fetch
  was capped near six minutes by a fixed retry loop).
• timeoutMs still defaults to 10 minutes; running longer queries requires
  raising it on the connection. When the connector runs behind an HTTP layer
  (e.g. the Malloy Publisher), keep the connection timeoutMs below that layer's
  socket/request timeout so a long query ends with the connector's clear error
  rather than an opaque socket reset (documented at TIMEOUT_MS).

Checklist

• Updated malloydata.github.io/src/documentation/setup/config.malloynb
  (companion PR docs(config): clarify BigQuery timeoutMs semantics malloydata.github.io#336): timeoutMs now bounds
  the results wait as well as the job, with the 10-minute default documented.

[jswir]

Pre-submission review notes (malloy-scoped). Overall this is a correct, well-diagnosed fix — I verified the mechanism claim (request-level vs job-level wait) against the @google-cloud/bigquery source, git history supports it (it implements the long-standing TODO left by the 2022 timeout commits rather than reverting them), and moving the abort-listener removal into an outer finally fixes a real latent bug where retries after the first were non-cancelable.

Inline comments below. The two worth addressing before merge:

• the poll loop widens worst-case abort latency, because "still running" is no longer bounded by the retry count (only by the deadline); and
• "still running" is detected only by a regex on the client's error string, which degrades silently back to the pre-fix behavior if that wording ever changes.

Both have a more robust alternative noted inline. The rest are nits (deadline tolerance, error actionability, '0' timeout handling, spread order, test coverage).

Two more, not inline:

• The TODO around line 346 (confirm that resulting metadata has "jobComplete: true") is effectively resolved by this mechanism: the client throws precisely when jobComplete === false and a timeoutMs is requested (which this code always sends), so a non-throwing resolve implies it's true. Worth removing/updating.
• Downstream consideration for the Malloy Publisher: its HTTP socket timeout is 10 min — equal to this connector's new default deadline. Under Publisher's default 5-min query-timeout wrapper this is moot, but if an operator raises that wrapper to let long BigQuery queries finish (the case this unblocks), the 10-min deadline races the 10-min socket timeout with zero margin. Worth choosing the connector default to sit under common downstream HTTP timeouts, or documenting the relationship.

[girishjeswani]

@jswir
Thanks for the thorough review

Structured jobComplete signal (the regex concern). Switched getQueryResultsUntilComplete to the callback overload and now branch on apiResponse.jobComplete === false instead of matching the error string. isQueryStillRunningError is removed, so the silent-regression-on-message-drift path no longer exists. Verified against @google-cloud/bigquery@7.9.4 that the still-running callback carries jobComplete: false. Thanks for pointing at the callback form; it's a strictly better signal than the message.

Abort latency. The abort signal is now threaded into getQueryResultsUntilComplete: checked at the top of each iteration, and pollQueryResults also races the signal so an in-flight poll settles immediately on abort rather than after the server-side wait. So the Publisher's AbortController now actually ends the call. Added an "abort fires mid-poll" test and an "already aborted" one, matching the coverage bar from #2760.

Soft deadline / clamp. Each poll's timeoutMs is clamped to min(GET_QUERY_RESULTS_POLL_MS, remaining), so a short configured timeout fails fast and the overall deadline can't overshoot by a full poll. Test added.

Spread order. timeoutMs is now applied after ...getQueryResultsOptions, so a caller can't override the poll interval.

Unactionable error. On deadline exceeded it now throws a wrapped error naming the actual configured deadline and pointing at timeoutMs, instead of rethrowing the misleading last-poll ...120000ms string.

'0' timeout. Added resolveTimeoutMs, which preserves an explicit '0' (fail-fast) and only falls back to the default on an unset/empty/non-numeric value. It's used for both the poll deadline and jobTimeoutMs so the two stay consistent.

line-346 TODO (confirm ... jobComplete: true): removed. The new polling contract only returns once the job is complete, so it's guaranteed; left a one-line note in its place.

Mixed-signal test. Added one that interleaves still-running and transient responses and asserts a still-running poll does not refill the 3-retry budget.

Downstream 10-min race. Went with documenting rather than changing the default. TIMEOUT_MS is pre-existing and also bounds jobTimeoutMs, and this is a general-purpose connector that shouldn't hard-code a margin against one embedder's socket timeout. Added a comment at TIMEOUT_MS noting it should sit below any downstream HTTP/request timeout. The abort fix above also helps in practice: the Publisher's query-timeout wrapper now settles the call, so that wrapper (kept below the socket timeout) becomes the effective ceiling and the connector deadline is a backstop. Happy to lower the default instead if you'd prefer a built-in margin.

[sagarswamirao]

Traced the polling rewrite against the @google-cloud/bigquery@7.9.4 source. Keying "still running" off apiResponse.jobComplete === false rather than the error string is the right call, and moving the abort-listener removal into the outer finally fixes a real latent issue where a retry after the first was left non-cancelable.

Two small non-blocking things:

1. resolveTimeoutMs treats a whitespace-only timeoutMs as 0 (fail-fast) rather than falling back to the default. Number(' ') is 0 and Number.isFinite(0) is true, so a blank-but-not-empty config value slips past the configured === '' guard and produces an immediately-failing connection. A configured.trim() === '' in the guard would close it.

2. Worth a line in the description/release notes that this changes the meaning of timeoutMs: '0'. The previous code (Number(...) || TIMEOUT_MS) treated 0 as falsy and fell back to the 10-minute default, whereas this now preserves '0' as fail-fast. Looks intentional given the resolveTimeoutMs doc comment; just flagging it as a user-visible behavior change.

Optional: getQueryResultsUntilComplete is covered thoroughly in isolation, but the config.timeoutMs to deadline wiring through createBigQueryJobAndGetResults (and "a jobComplete:false response never reaches the caller as data") is only exercised by the live-BigQuery integration test. A hermetic BigQueryConnection test with a stubbed job would lock that seam down.

[girishjeswani]

Thanks Sagar, addressed all of them.

Whitespace timeoutMs: The guard is now configured.trim() === '', so a blank-but-not-empty value falls back to the default instead of coercing to 0 and failing every query. resolveTimeoutMs is now exported and directly unit-tested (unset / empty / whitespace / '0' / numeric / non-numeric).

'0' behavior change: Agreed it's user-visible and intentional. I called it out explicitly in the PR description Notes: '0' is now preserved as fail-fast, where the previous Number(...) || TIMEOUT_MS treated it as falsy and fell back to the 10-minute default.

Hermetic wiring coverage: Added bigquery_connection.unit.spec.ts, which drives runSQL against a stubbed job (no live BigQuery) and locks down the seam: that config.timeoutMs reaches the deadline ('0' fails fast before any poll; a whitespace value falls back and the query completes), and that a jobComplete: false response is polled rather than surfaced to the caller as empty data.

[kylenesbit]

Went through the diff against every other connector to check the "Scope" claim, and it holds up: Snowflake is the only other connector that does real client-side timeout work. Databricks and Trino/Presto delegate to their client libraries, Postgres/MySQL/DuckDB are synchronous, and the Publisher forwards over HTTP with a fixed axios timeout while currently dropping abortSignal entirely. There's no shared timeout concept in core either — RunSQLOptions only carries abortSignal — so keeping this BigQuery-local is right.

That makes Snowflake the reference point to align against, and the good news is this PR already matches it where it matters most: the same 10-minute default, and the same abort discipline (fail fast when the signal is already aborted, listener cancels the in-flight statement/job, settle promptly rather than waiting out the server). Two things to close out before merge, one of which is a divergence from that standard.

1. timeoutMs of 0 should mean "no timeout", the way it does on Snowflake

Snowflake deliberately preserves an explicit 0 and then treats it as disabled:

// snowflake_connection.ts — `??` keeps an explicit 0
this.timeoutMs = options?.timeoutMs ?? TIMEOUT_MS;

// snowflake_executor.ts — 0 is falsy, so no cancel timer is ever armed
const timeoutId = timeoutMs ? setTimeout(cancel, timeoutMs) : undefined;

(snowflake_connection.ts#L202, snowflake_executor.ts#L195)

So timeoutMs: 0 on Snowflake means an unbounded wait. resolveTimeoutMs gives the same value the opposite meaning on BigQuery: the deadline is already blown before the first poll is issued, so every query on that connection fails. The hermetic test encodes exactly that — '0' fails fast, with getQueryResults never called.

Three reasons to flip it rather than document it:

• Same knob, same documentation, opposite behavior. The docs site describes the two identically today: timeoutMs | string | Query timeout in ms for BigQuery, timeoutMs | number | Query timeout in ms for Snowflake. Someone moving a value between connections has no way to anticipate that one means "wait forever" and the other means "fail everything".
• "0 disables" is the ambient convention for socket and HTTP timeouts, and it's the convention already established inside this repo by Snowflake. This PR would make BigQuery the odd one out.
• Fail-fast has no use case. Nobody intentionally configures a connection whose every query throws before dispatch, so the new behavior is only reachable by accident. "No client-side ceiling, let the warehouse decide" is a real thing operators ask for — and it's especially relevant here, since long-running queries are precisely what this PR unblocks.

Worth noting the same resolved value also flows into jobTimeoutMs, so '0' currently means we ask BigQuery for a zero job timeout too. The hermetic test shows createQueryJob still runs before the client-side failure, so we dispatch a job and then abandon it. Aligning on Snowflake means never having to reason about what jobTimeoutMs: 0 does server-side.

Concretely: keep resolveTimeoutMs — the whitespace fix earns its place regardless — but have a resolved 0 mean "no poll deadline" instead of "deadline already passed", and omit jobTimeoutMs rather than sending 0. If an unbounded mode feels like too much, the next best option is falling back to the default on '0' (the pre-PR behavior), which at least keeps it from being a footgun. The one thing to avoid is a third, connector-specific meaning for the same value.

2. Companion docs PR

packages/malloy/src/connection/CONTEXT.md (lines 131-141) asks for a companion PR to malloydata/malloydata.github.io (src/documentation/setup/config.malloynb) whenever a registered property's semantics change, plus a checklist item here. This qualifies: timeoutMs now bounds the total results wait, where before it only bounded jobTimeoutMs and the results wait was hard-capped near six minutes no matter what was configured.

That existing doc line — "Query timeout in ms" — is arguably only becoming true with this PR, which is a nice thing to be able to say in the companion. Worth spelling out what it bounds now (job plus results polling) and whatever 0 settles into from point 1.

[girishjeswani]

Thanks @kylenesbit , Addressed in d75cd3d.

1. timeoutMs: 0 now means "no timeout", matching Snowflake. Agree that fail-fast was the opposite of the established convention and had no real use case. A resolved 0 now means no client-side limit: no poll deadline (poll until the job completes or the caller aborts), and jobTimeoutMs is omitted rather than sent as 0, so BigQuery applies its own default. resolveTimeoutMs stays (the whitespace guard earns its place), and the disabled semantic is applied at the two call sites. Tests updated: the hermetic '0' case now asserts poll-through plus an omitted jobTimeoutMs, a positive value is asserted to pass through as jobTimeoutMs, and the whitespace guard now asserts the default.

2. Companion docs + checklist. Opened docs(config): clarify BigQuery timeoutMs semantics malloydata.github.io#336, which updates the BigQuery timeoutMs row to spell out what it bounds now (the job plus results polling), the default, and that 0 disables the client-side timeout. Added the checklist item here.

Also, thanks for independently verifying the scope claim against every connector, glad it holds up.

[girishjeswani]

Thanks @mtoy-googly-moogly for the review. Updated the BigQuery PR to match:

0 now falls back to the internal default. Since unset/blank/non-numeric/0 all resolve to the 10 min default, this drops the special-case helper entirely and is back to the original parse, just correctly wired through the new poll path.

timeoutMs shadows to jobTimeoutMs and the client polls getQueryResults to the same deadline, totaling the wait across the ~200s-capped calls.

Cancel on our own deadline. If we reach the client deadline and BigQuery's job-timeout error hasn't come back yet, we call job.cancel() before throwing, so the job doesn't keep running (and billing) past the point we stopped waiting.

[mtoy-googly-moogly]

Going to close and re-open which should, accoridng to claude, run the new CI,.