Repository navigation
Add fire-and-forget async statement submission - #198
Merged
Merged
Conversation
Submitting a statement through Snowflex holds a connection pool slot for the statement's entire run: the transport polls Snowflake's 202 response to completion inside the GenServer.call that owns the checked-out connection. For work where the caller only needs to start a statement and never reads its rows (stored procedures, long MERGEs, warehouse maintenance), that slot is occupied for no benefit and blocks other queries from acquiring it. Snowflake's SQL API already supports this directly via the `async=true` query parameter, which acknowledges with a statement handle as soon as it accepts the statement. Snowflex never set it. Thread it through and expose the lifecycle as public API, so consumers stop reimplementing this against Transport.Http.options/1. Submission is routed through the pool rather than around it, so these calls keep JWT refresh, retry/backoff, Finch pool routing, telemetry and log metadata. The slot is now held only for the submission round-trip. The three transport callbacks are optional so existing custom transports still satisfy the behaviour; calling them on a transport that lacks them returns a Snowflex.Error instead of raising UndefinedFunctionError. Refs #197
Code.ensure_loaded?/1 returns a boolean that was being discarded, which read as though the call were there for its side effect alone. Using it as a guard also short-circuits function_exported?/3 for a transport that cannot be loaded.
Adversarial review of the previous two commits turned up a set of defects,
several of which were only provable empirically. Fixes, roughly by severity:
Async ops inside Ecto.Repo.checkout/2 asked the pool for a second connection
instead of reusing the one the caller already held. At pool_size: 1 that queued
behind the caller's own connection until the queue timeout fired and the outer
checkout was force-disconnected — while still returning {:ok, handle}, so the
corruption was silent. run_op/5 now reuses a checked-out connection the way
stream_query/5 already did (5.0s + disconnect -> 0.07s clean).
The three new transport entry points called GenServer.call without the try/catch
execute_statement/4 has, so a call timeout let an uncaught exit escape
handle_execute; DBConnection re-raised it and the caller never saw a
Snowflex.Error. They now catch, and the call deadline gets a grace period over
the HTTP timeout so a slow request surfaces as the handler's own error instead of
racing the deadline. Status polls also no longer inherit Req's transient retry,
which could otherwise block ~187s against a 45s deadline re-asking a question
Snowflake had already answered with 202.
There was no way to read an async statement's rows: the 200 response carries
data and resultSetMetaData, but those were discarded. Consumers would have had to
hand-roll HTTP against Transport.Http.options/1 — the duplication this work set
out to remove. Adds fetch_result/3, reusing gather_results/3 and
format_response_body/1 so rows decode and multi-partition results merge exactly
as they do for a normal query.
run_op/5 built %Query{} directly, skipping the IO.iodata_to_binary normalization
Query.new/1 applies, so an iodata statement was JSON-encoded as an array rather
than a string. A null statementHandle also satisfied the success pattern and
produced {:ok, nil}, which then failed statement_status/3's is_binary guard.
The statement request body sent :timeout in milliseconds where Snowflake defines
seconds, so the 45s default asked for ~12.5 hours and effectively disabled the
server-side timeout — the only bound on a fire-and-forget statement. JWT exp had
the same unit mismatch, advertising ~7 days instead of the configured lifetime.
Both now convert; the token is also cached for its lifetime rather than being
re-signed (PEM decode + RSA) on every request.
Smaller: statement_status/3 reports status positively instead of inferring
:succeeded by exhaustion, so it no longer depends on Result.metadata being a bare
map; error building is unified and keeps the :statement metadata that aids
debugging; Error.code is always a string; handle-addressed GETs share one URL
builder; :hibernate is consistent across reply paths; and Req gets [] rather than
%{} for "no query params", which is the value it treats as a no-op.
The pool-occupancy test was passing for the wrong reason: mutation testing showed
that reintroducing the polling bug left its central assertion green, because
submit_async is awaited before the second query starts. It now asserts on
submit's own elapsed time, which does fail under that mutation. Also covers the
timeout path, checkout reuse, fetch_result, iodata, and the null handle.
jeremyowensboggs
approved these changes
Sep 23, 2026
| # encode path instead. | ||
| query_params = if call_opts[:async?], do: %{async: true}, else: [] | ||
|
|
||
| case Req.post(req_client, |
There was a problem hiding this comment.
We should consider adding retry: false here as well - to prevent a transient error from causing a re-submission. https://github.com/pepsico-ecommerce/galileo/blob/master/apps/api_services/lib/api_services/common/snowflake/async_statement.ex#L244
Contributor
There was a problem hiding this comment.
Should we use Snowflake's requestId for that? Have callers supply one if they want idempotent requests?
Iirc Req does not automatically retry POST
There was a problem hiding this comment.
hmm -yup you are correct - it's not on by default http://req.hexdocs.pm/0.7.2/Req.Steps.html#retry/1-request-options.
Withdrawn.
Ch4s3
marked this pull request as ready for review
September 24, 2026 13:29
imorales-g2
pushed a commit
to imorales-g2/snowflex
that referenced
this pull request
Sep 30, 2026
Cuts the Unreleased section to 1.7.0. The async statement submission work (pepsico-ecommerce#198) and the Req 0.7 minimum bump (pepsico-ecommerce#194) both landed since 1.6.0. Promotes the statement-timeout unit fix from Bug Fixes to Breaking Changes: sending :timeout in milliseconds where Snowflake expects seconds had effectively disabled the server-side timeout, so statements that previously ran unbounded will now be cancelled. That is a behaviour change consumers need to see before upgrading, not a quiet fix.
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
Adds fire-and-forget async statement submission so a caller can start a Snowflake statement, get its handle back without holding a connection pool slot for the statement's full duration, and retrieve its rows later.
Today
Snowflex.Transport.Httppolls Snowflake's 202 response to completion inside theGenServer.callthat owns the checked-out connection (await_async_execution/3), so a slot stays occupied until the statement finishes. Snowflake's SQL API already supportsasync=true, which acknowledges with astatementHandleas soon as it accepts the statement — Snowflex just never set it.Rows come back decoded exactly as
Repo.query/3decodes them, becausefetch_result/3reuses the existinggather_results/3+format_response_body/1machinery (including parallel multi-partition merging).Design notes
Routed through the pool, not around it. Since
async=truereturns in milliseconds, going through the pool costs only the submission round-trip while keeping JWT refresh, retry/backoff, Finch pool routing, telemetry and log metadata.Reuses the execute path.
Snowflex.Querygains an:opfield;Connection.handle_execute/4dispatches on it. For status/fetch/cancel,:statementcarries the handle so it lands insnowflex_statementlog metadata — deliberate, so a stuck statement is greppable. Happy to split it into a separate field if you'd prefer.Transport callbacks are optional, so existing custom transports still satisfy the behaviour with no change and no compile warnings; calling one that's missing returns a descriptive
Snowflex.Error.:current_statementis untouched by the async path — that field belongs to the cursor/streaming path, and writing it would corrupt an in-flightRepo.streamon the same connection.Included bug fixes
Two pre-existing unit mismatches, both in code this PR builds on. I'd originally planned to split these out, but the first one directly undermines the feature — for a fire-and-forget statement the server-side timeout is the only bound, since nothing client-side waits on it.
timeoutin milliseconds where Snowflake defines seconds, so the 45s default asked Snowflake to wait ~12.5 hours, effectively disabling the server-side statement timeout. Behaviour change: statements that previously ran unbounded will now be cancelled at the configured timeout. Called out in the CHANGELOG.expadded a millisecond:token_lifetimeto a second-precision timestamp, advertising ~7 days instead of the configured 10 minutes. Verified fixed:exp - iat = 600s.Also: the JWT is now cached for its lifetime instead of being re-signed (PEM decode + RSA) on every request — verified 1 distinct token across 4 requests, previously 4.
Review round
I ran an adversarial review over the first two commits (9 finder angles, 5 verifiers, mutation testing).
03f7fcffixes what it found. The ones worth calling out:Repo.checkout/2silently corrupted the enclosing checkout.run_op/5asked the pool for a second connection instead of reusing the held one; atpool_size: 1that queued until the timeout fired and the outer checkout was force-disconnected — while still returning{:ok, handle}, so it looked fine. Reproduced at 5.0s + disconnect, now 0.07s clean. Fixed by reusing the checked-out connection the waystream_query/5already did.execute_statement/4has, so an uncaught exit escapedhandle_executeand DBConnection re-raised it. Notedeclare/fetchalso lack it, soexecute_statementwas the outlier — worth a follow-up to make all six consistent.receive_timeoutequalled the call timeout and Req retried the GET. Retries are now off for status (Snowflake already answered with 202) and the call deadline has grace over the HTTP timeout.fetch_result/3.submit_asyncis awaited before the second query starts. It now asserts on submit's own elapsed time, which does fail under that mutation.Two candidate findings were refuted by testing and deliberately not "fixed":
Code.ensure_loaded?is load-bearing (proved by purging the module) and its embedded-mode failure path is unreachable for a running module; andparams: %{}vs no params is byte-identical on the wire (Finch normalizes""andnil) — tidied to[]anyway.Test plan
mix test --exclude integration— 159 passed (25 async tests)mix compile --warnings-as-errorsmix credo --strict— no issuesmix format --check-formattedmix assay(dialyzer) — 0 errorsrefute_receivedexpHttpRepoTest/ConnectionTestare credential-dependent and fail identically onmaster(verified by diffing failing test names — the sets match)The statement-timeout fix in particular deserves a live check before release, since it changes how long Snowflake lets a statement run.
Note on the linked issue
Two API names in #197 don't exist: there is no
Snowflex.query_async/2(the real mechanism is the implicit 202 polling above), and noSnowflex.HttpClient.transport_options/1— the function consumers actually use isSnowflex.Transport.Http.options/1, which is public and documented. So the issue's "coupled to Snowflex internals" concern is unfounded. I'll follow up on the issue to correct both.Refs #197