fix(db-snowflake): resolve timeoutMs 0 to the default, matching BigQuery - #3022
Merged
mtoy-googly-moogly merged 3 commits intoJul 31, 2026
Conversation
Collaborator
|
the test reaches into privates through a cast and asserts by argument position, so it breaks on refactors unrelated to timeouts while pinning a two-line guard the comment already explains. just delete it |
The Snowflake connector resolved timeoutMs with 'options?.timeoutMs ?? TIMEOUT_MS', so an explicit 0 was kept and then read by the executor's 'timeoutMs ? setTimeout(cancel, timeoutMs) : undefined' as no timer, i.e. wait forever. A blank or non-numeric value parses to NaN and behaved the same. Resolve with '|| TIMEOUT_MS' so 0 and NaN fall back to the 10-minute default, matching the BigQuery change in malloydata#3010. Applied to timeoutMs and the sibling schemaSampleTimeoutMs. The per-statement client-side cancel is unchanged; on timeout the executor already cancels the running statement server-side. Adds snowflake_connection.unit.spec.ts asserting the resolution via a stubbed executor (0, unset, and NaN fall back; a positive value passes through). Signed-off-by: Girish Jeswani <girish@credibledata.com>
Extend the 0-to-default resolution to negatives: a bare || let a negative through (it is truthy), and the executor would then schedule setTimeout(cancel, <negative>) and abort the statement almost immediately. Resolve with Number(...) > 0 so zero and negative both default, for timeoutMs and the sibling schemaSampleTimeoutMs. Adds a negative-timeoutMs unit test. Signed-off-by: Girish Jeswani <girish@credibledata.com>
Signed-off-by: Girish Jeswani <girish@credibledata.com>
mtoy-googly-moogly
force-pushed
the
fix/db-snowflake-timeout-zero-default
branch
from
July 31, 2026 12:59
4fbdca5 to
5873ede
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
The Snowflake connector treated a configured
timeoutMsof0as "waitforever", which is inconsistent with the BigQuery connector. In
#3010 BigQuery's
0was aligned to fall back to the default;this does the same for Snowflake, so a
0, a negative value, or a blank /non-numeric value (parsed to
NaN) now resolves to the 10-minute default.Root cause
timeoutMsis resolved in the connection constructor withoptions?.timeoutMs ?? TIMEOUT_MS, so??only defaultsnull/undefinedandkeeps an explicit
0. That0then reaches the executor'stimeoutMs ? setTimeout(cancel, timeoutMs) : undefined, where a falsy valueschedules no timer, i.e. no client-side timeout. A blank or non-numeric
connection value is parsed to
NaNbyparseIntinindex.ts, which is alsofalsy and behaves the same way. A negative value, by contrast, is truthy, so it
reaches
setTimeout(cancel, <negative>)and aborts the statement almostimmediately.
Change
and negative all fall back to the 10-minute default, and only a positive value
overrides it. Uses
Number(...) > 0rather than a bare||, because anegative value is truthy and would otherwise reach
setTimeout(cancel, <negative>)and abort the statement at once. Matches the BigQuery connector,and applies to both
timeoutMsand the siblingschemaSampleTimeoutMs.on timeout the executor already calls
statement.cancel(), which cancels therunning query server-side. The per-statement model (each statement in a batch
bounded by
timeoutMs) is retained.Scope
Snowflake-only, and intentionally minimal. This is the consistency follow-up
requested on #3010; it does not add a server-side job timeout.
Snowflake's
STATEMENT_TIMEOUT_IN_SECONDS(the server-side cap, analogous toBigQuery's
jobTimeoutMs) is left unset, as today. ShadowingtimeoutMsto itis a reasonable future addition if a per-job rather than per-statement bound is
wanted, but it is a larger change (seconds granularity, session-vs-statement
scoping) and is deliberately out of scope here.
Tests
There is no new unit test. The resolution is a two-line guard
(
Number(...) > 0 ? ... : TIMEOUT_MS) whose comment documents intent; a testfor it could only go red if the test and the guard were edited together, so the
guard and its comment stand on their own. The existing live integration test
(
cancels long-running batch on timeout,timeoutMs = 500) exercises theclient-side cancel and is unaffected, since a positive value still passes
through.
tscandeslintpass.Notes
timeoutMsof0previously disabled theclient-side timeout (unbounded) and a negative value cancelled the statement
almost immediately; both now use the 10-minute default. After this there is no
value that means "no timeout"; that is intentional and matches the BigQuery
connector.
Checklist
Snowflake
timeoutMsrow (default, per-statement,0falls back to thedefault).