feat(sf_core): honor database options during connection initialization by zeroshade · Pull Request #1340 · snowflakedb/drivers

zeroshade · 2026-08-11T22:02:59Z

Summary

dereference the database handle during connection initialization and use its validated options as the base programmatic configuration layer
apply connection options over database options, including case-insensitive precedence for arbitrary session parameters
use the effective database-plus-connection seed for config resolution, login session parameters, retry and timeout policy, and inherited post-login client settings
reject missing or invalid database handles instead of silently ignoring them

This removes the need for wrappers to copy database options onto every connection or independently reproduce database/connection precedence.

Validation

cargo fmt --all --check
cargo clippy -p sf_core --lib --tests --offline -- -D clippy::all
sf_core connection unit suite: 53 passed
complete sf_core library suite: 1694 passed, 1 ignored, 0 failed
git diff --check

Copilot

Pull request overview

This PR updates sf_core’s DatabaseDriverV1 connection initialization path to treat database-level options as the base programmatic configuration layer, then overlay connection-level options (including case-insensitive precedence for arbitrary/unknown session parameters). The goal is to make database/connection option precedence consistent across config resolution, login session parameters, retry/timeout policy, and inherited post-login client settings, while rejecting missing/invalid database handles instead of silently ignoring them.

Changes:

Add ParamStore helpers to support empty checks and case-insensitive key overriding for layered programmatic options.
Introduce DatabaseDriverV1::database_settings() and use database settings as the seed for connection_init, merging connection options on top.
Store the database seed on the Connection and update derived behaviors (login params, retry/timeout, logout config) to use the effective merged seed; add targeted tests for precedence and invalid database handles.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`sf_core/src/config/param_store.rs`	Adds `is_empty()` and a case-insensitive merge helper to enforce precedence for arbitrary session parameters.
`sf_core/src/apis/database_driver_v1/database.rs`	Exposes an async accessor to retrieve validated database settings by handle.
`sf_core/src/apis/database_driver_v1/connection.rs`	Applies database+connection seed precedence during init and related configuration derivations; stores database seed on the connection; adds tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

sf_core/src/config/param_store.rs:146

extend_from_case_insensitive can still produce nondeterministic results if other contains multiple keys that differ only by ASCII case (e.g. both foo and FOO). Because HashMap iteration order is randomized, the winning value will vary across runs even though the doc comment promises deterministic precedence.

    pub(crate) fn extend_from_case_insensitive(&mut self, other: &ParamStore) {
        for (key, value) in &other.inner {
            self.inner
                .retain(|existing, _| !existing.eq_ignore_ascii_case(key));
            self.inner.insert(key.clone(), value.clone());
        }

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

nodejs/tests/unit/core.test.ts:11

This test name is misleading: the body only verifies that destroy() resolves on a newly-constructed CoreConnection, but it does not actually assert anything about database-handle ownership/lifetime. Rename the test to reflect what it covers (or add a concrete assertion that demonstrates the intended ownership behavior, if observable from the JS API).

it('owns the database handle for its lifetime', async () => {

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

nodejs/tests/unit/core.test.ts:15

The test name suggests it verifies database-handle ownership, but the assertions only check that destroy() resolves. Consider renaming the test (or extending it) so the name matches what is actually being validated.

it('owns the database handle for its lifetime', async () => {
  const connection = new CoreConnection({});

  await expect(connection.destroy()).resolves.toBeUndefined();
});

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

sf_core/src/apis/database_driver_v1/connection.rs:1290

Connection::effective_settings() clones and merges an entire ParamStore on every call. It’s now used in hot paths (e.g., query + PUT/GET retry policy derivation) and typically while holding the connection mutex, which adds avoidable allocations and lock hold time. Consider introducing a cheap “effective settings” view (precedence-aware getters) or caching a merged ParamStore and updating it when connection_seed changes, so callers like RetryPolicy::* don’t require a full clone per operation.

    pub(crate) fn effective_settings(&self) -> ParamStore {
        let mut settings = self
            .resolved_connect
            .clone()
            .unwrap_or_else(|| self.database_seed.clone());

sf_core/src/apis/database_driver_v1/statement.rs:291

This computes put_get_policy via RetryPolicy::put_get(&conn.effective_settings()), which allocates/clones the full settings map while holding the connection mutex. Since this runs on the statement execution path, it could become a noticeable per-query overhead. Consider extracting only the retry-related parameters under the lock (or cloning the minimal inputs) and building the policy after releasing the mutex, or switching RetryPolicy::put_get to use a precedence-aware reader that doesn’t require materializing a merged ParamStore.

            let regional = conn.use_s3_regional_url_session_param().await;
            let flags = crate::stage_binding::StageBindingFlags {
                stage_state: conn.stage_state.clone(),
            };
            let put_get_policy = RetryPolicy::put_get(&conn.effective_settings());
            (regional, flags, put_get_policy)

sf_core/src/apis/database_driver_v1/stream_transfer.rs:320

RetryPolicy::put_get(&conn.effective_settings()) creates a merged/cloned ParamStore while the connection mutex is held. This is on the stream transfer path and can be invoked frequently, so the repeated cloning/allocation may impact throughput. Consider building the retry policy from a cached merged settings snapshot, or redesigning the retry policy builders to read from a precedence-aware view without allocating a full ParamStore.

            let (put_get_policy, transport) = {
                let conn = conn_ptr.lock().await;
                (
                    crate::config::retry::RetryPolicy::put_get(&conn.effective_settings()),
                    file_manager::StageTransport {

sfc-gh-pfus · 2026-08-26T06:38:02Z

@zeroshade can you resolve the conflicts?

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

zeroshade · 2026-08-26T15:33:50Z

@sfc-gh-pfus I've resolved the conflict and updated this. Should be good now! Thanks!

zeroshade · 2026-09-03T15:15:01Z

Done — kept the AUTHENTICATION_TIMEOUT arm and added the cap fix in a3e7561.

One placement change from your suggestion: I could not write login_timeout at the ODBC crate boundary. All three sites (DSN key, SQL_ATTR_LOGIN_TIMEOUT, and the 300 s default follow-up) go through resolve_options(Wrapper::Odbc), where login_timeout is a scoped alias of authentication_timeout. Writing both keys there makes them resolve to the same canonical name, and since odbc_key_for("authentication_timeout") is None there is no tiebreak, so it fails the connect with a ConflictingParameters error.

Instead the mirror runs in resolve_options's ODBC branch, right after alias resolution, so a canonical login_timeout survives into the resolved snapshot (resolver::resolve in connection_init takes no wrapper, so it is not re-shadowed). No Wrapper special-case in TimeoutConfig. It covers all three write sites centrally: AUTHENTICATION_TIMEOUT=300 waits 300 s, the default outer cap tracks the 300 s follow-up instead of the registry 120, and AUTHENTICATION_TIMEOUT=2 still fails in ~2 s.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

The mirror added in a3e7561 ran inside `resolve_options`, so the value landed in `connection_seed` — the explicit layer, which `resolve_with_paths` applies last. The ODBC driver injects a synthetic 300 s `authentication_timeout` whenever the caller sets no timeout, so that driver-side default was mirrored into `login_timeout` at explicit priority and outranked a `connections.toml` profile. An ODBC connection using a profile with `login_timeout = 30` and no DSN timeout got 300 s instead of 30 — a regression this branch would have introduced. `param_store.rs` documents exactly this hazard for pre-populated defaults. A config profile is the one path that reaches the canonical `login_timeout` under ODBC (profile keys resolve under the Python flavor, so they escape the scoped `LOGIN_TIMEOUT` alias). Move the mirror to `apply_wrapper_layer_fixups`, called from the resolver once every layer is merged, and skip it when the profile or the explicit layer already supplied `login_timeout`. That matches the review's rule: only fill `login_timeout` if the caller did not set it themselves. The resolver needs the wrapper to do this, so the connect path now calls `resolve_for_wrapper`. `resolve` stays wrapper-neutral for `resolved_settings`, which only validates options and has no timeout stake. Caller-facing behavior is unchanged from the previous commit: `AUTHENTICATION_TIMEOUT=300` still waits 300 s, the default cap still tracks the 300 s follow-up rather than the registry 120, and `AUTHENTICATION_TIMEOUT=2` still fails in ~2 s. Found by review of a3e7561.

395da8f skipped the mirror whenever any user layer supplied `login_timeout`, so a `connections.toml` profile with `login_timeout = 30` kept control of the outer wrap even when the caller passed `AUTHENTICATION_TIMEOUT=300` on the DSN. That inverts the resolver's usual explicit-over-profile precedence, and left the outer wrap (30 s) below the auth budget the caller actually asked for (300 s), so login died early. The guard could not distinguish the two because both the caller's DSN value and the ODBC driver's synthetic 300 s default arrived in the explicit layer as `authentication_timeout`. Give the driver default its own layer instead: the ODBC crate no longer injects the follow-up, and `apply_wrapper_layer_fixups` applies ODBC's 300 s `authentication_timeout` default underneath `connections.toml`. `authentication_timeout` in the explicit layer is then a genuine caller value, so the mirror can prefer it over a profile `login_timeout` while the wrapper default still yields. Resulting precedence, ODBC only: - caller DSN / SQLSetConnectAttr → drives both timeouts, beats a profile - connections.toml profile → beats the wrapper's 300 s default - nothing set → 300 s for both, not the registry 120 This also fixes the same class of bug for `authentication_timeout` itself: the synthetic default previously overrode a profile's value. `DEFAULT_LOGIN_TIMEOUT_SECS` in the ODBC crate, still reported by `SQLGetConnectAttr(SQL_ATTR_LOGIN_TIMEOUT)`, now reads from the shared `ODBC_DEFAULT_AUTHENTICATION_TIMEOUT_SECS` so both sides cannot drift. Found by review of 395da8f.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

odbc/src/api/connection.rs:2598

This test comment still refers to a “default-timeout follow-up in connect_with_params”, but that follow-up code was removed. Updating the comment will prevent future confusion about why AUTHENTICATION_TIMEOUT is normalized.

        // The caller's own spelling has to reach sf_core under the canonical
        // key. Left uppercased, the default-timeout follow-up in
        // `connect_with_params` writes `authentication_timeout` beside it and
        // overwrites the caller's value once both canonicalize.

Two conflicts, both where upstream reworked code this branch also touches. sf_core connection_close: upstream replaced the `is_closed` swap guard with a `close_state`/`close_done` loop plus a `perform_close` helper. Took that structure wholesale, then re-applied this branch's close-time logout config derivation on top: `perform_close` now re-derives from `conn.effective_settings()` rather than `conn.connection_seed`, which is what preserves the config-file layers selected by database options such as `connection_name`, and passes those settings as the third argument `prepare_logout_from_conn` grew on this branch. nodejs Connection: upstream added a `ConnectionState` (PRISTINE / CONNECTED / TERMINATED) that gates `execute` and `get_query_result`; this branch added the `database_handle` field. Both are kept. `connect()` combines them — it reports the outcome through the new state while passing the real database handle, which retires upstream's `Handle { id: 0, magic: 0 }` placeholder and the TODO about the argument being unused. `destroy()` releases the database handle and marks the connection terminated.

`Connection::new` registers a connection handle and, on this branch, a database handle, but `destroy()` was the only path that released either. Nothing obliges a JS caller to call it, so a `Connection` that is simply dropped or garbage collected stranded both handles. `HandleManager` never reuses ids (see its TODO), so those entries are held for the life of the process — the ids are consumed permanently, not merely the memory. Add `impl Drop for Connection` to reclaim both handles. `destroy()` and `Drop` can both run, in either order, so the release is claimed once through a shared `AtomicBool`: a caller that never awaits the `destroy()` promise can have the object collected while the close is still in flight. Releasing twice would otherwise be harmless — the handle magic guards against deleting a successor's entry — but it logs an error on the mismatch, so the guard keeps that noise out of the logs. `Drop` only releases handles; it does not log out. That is network I/O and has no business running in a finalizer, so `destroy()` remains the way to end a session gracefully. Addresses a review comment on PR snowflakedb#1340.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

The `released` bool added in 6feac04 made the release idempotent but not ordered, so `Drop` could still win the race against `destroy()`. The async block captures only raw handles and nothing keeps the JS object alive while it runs, so a connection collected between `destroy()` being called and the block reaching `connection_close` had its handles released first. The close then found no connection and returned early, silently skipping the logout and stranding the session server-side — a worse leak than the stranded handle ids `Drop` was added to prevent. Replace the bool with a `Cleanup` ownership state machine (IDLE -> OWNED -> RELEASED). `destroy()` claims ownership synchronously, before the async block is scheduled, so `Drop` sees the claim and leaves the handles alone; the owning block releases once the close returns. `Drop` releases only handles that were never claimed. A second `destroy()` still closes but does not release a second time. Claiming before the work runs cannot strand the handles: `async_to_js` builds through `AsyncBlockBuilder`, which schedules the future on the runtime whether or not the caller awaits the returned promise, so the owning block always reaches its release. Found by review of 6feac04.

Three paths that this branch's own effective-settings model had not reached. `enable_put_get()` still read `connection_seed`, so `enable_put_get=false` supplied through the database handle or a resolved profile was ignored and PUT/GET stayed allowed for JDBC. Resolve it from `effective_settings()` instead, which is the resolved snapshot with post-init connection overrides applied, and keep combining it with the server `JDBC_ENABLE_PUT_GET` flag. The ODBC OAuth authorization-code cache default checked only `connection_seed` before forcing `client_store_temporary_credential=true`. A database-level `false` lives in `effective_seed` but not in `connection_seed`, so the default overwrote the caller's choice and turned credential caching back on. Pass the effective seed and rename the parameter to `user_seed`, since what it needs is every layer the user can set the flag through, not one of them. In the nodejs bridge, `connect()` captured only raw handles, so a JS object that became unreachable after the promise was created could have its handles released while `connection_init` was still running — failing the login, or establishing a server session against a handle that no longer exists and can never be closed. Replace the release-ownership flag with an `Arc<Handles>` that owns both handles and releases them when the last holder is done. Every async operation clones it, so handles now outlive the JS object for exactly as long as some operation still needs them, and the same guarantee extends to statements via `Statement::from_pending`. `destroy()` still releases eagerly so teardown frees core resources when the caller asks rather than at the next GC. Found by review of 8fe2f23..f168367.

`destroy()` released both handles and marked the connection terminated regardless of the outcome of `connection_close`. Core treats a failed close as retryable — `perform_close` settles the state back to `Open` precisely so a later close can reclaim the session — but releasing the handles threw away the only way to reach it, and marking the connection terminated made the JS object refuse further work. A logout that failed once was therefore unrecoverable, leaving a live server session behind. Release and mark terminated only once core reports the close succeeded. On failure both are skipped, so the caller can call `destroy()` again. Nothing leaks in that path either: the connection still holds its `Arc<Handles>`, so the handles are released when the JS object is finally collected. Found by review of 8fe2f23..1d5eefe.

Two comments still justified pre-canonicalizing `AUTHENTICATION_TIMEOUT` by pointing at the default-timeout injection in `connect_with_params`, which afbd3ff removed when `sf_core` took over that default. The reason the arm has to stay is unchanged, but it now rests solely on `apply_pre_connection_overrides` writing the canonical `authentication_timeout` for `SQL_ATTR_LOGIN_TIMEOUT`: leave the DSN spelling uppercased and the two keys reach `connection_set_options` side by side, only canonicalizing there, where the last-writer-wins insert silently discards one of them. Comment-only; the arm and its tests are unchanged. Addresses a review comment on PR snowflakedb#1340.

`destroy()` could close and release the handles while `connect()` was still initializing. Core reports a *successful* close for a connection that was never initialized, so the close returned Ok, the handles were released, and the concurrent `connection_init` then went on to establish a real session — behind handles that no longer existed. `connect()` marked the connection connected, later operations failed against the missing handle, and the live server session could never be logged out. Serialize the two lifecycle transitions behind an async mutex. `destroy()` now waits out an in-flight `connect()`, so teardown always runs against a settled connection: either fully established, and closed properly, or one that never came up. If `destroy()` wins the lock, the subsequent `connection_init` fails against the released handle and marks the connection terminated rather than orphaning a session. This race predates the branch — `main`'s `destroy()` releases with no coordination with `connect()` either — but this PR reworked both paths, so it is fixed here rather than left in place. Adds `tokio` with only the `sync` feature for the mutex; the runtime itself still comes from `sf_core`, and the other bridges already depend on tokio at this version. Found by review of 8fe2f23..8d4c835.

zeroshade · 2026-09-03T17:01:31Z

Correcting my earlier comment on the timeout cap: the placement changed twice after I wrote it, so please disregard the resolve_options description there.

395da8f46 — putting the mirror in resolve_options wrote login_timeout into the explicit connection seed, which resolve_with_paths applies last. Since the ODBC driver injects a synthetic 300 s authentication_timeout whenever the caller sets no timeout, that driver-side default outranked a connections.toml profile: a connection using a profile with login_timeout = 30 and no DSN timeout got 300 instead of 30. Moved the mirror to resolver::apply_wrapper_layer_fixups, which runs once every layer is merged and tracks which layer supplied the value.

afbd3ff06 — that guard then let a profile login_timeout outrank a real DSN AUTHENTICATION_TIMEOUT, inverting the usual explicit-over-profile precedence and leaving the outer wrap below the auth budget the caller actually asked for. The guard could not tell the two apart because the caller's DSN value and the driver's synthetic default both arrive in the explicit layer. So the ODBC crate no longer injects that follow-up at all: sf_core applies ODBC's 300 s authentication_timeout default beneath connections.toml, which makes an explicit authentication_timeout a genuine caller value again.

Resulting precedence, ODBC only:

Source	Wins over
DSN key / `SQLSetConnectAttr`	everything below
`connections.toml` profile	the wrapper default
wrapper default (300 s)	registry default (120 s)

This also fixes the same class of bug for authentication_timeout itself, which the synthetic default previously overrode.

Your three outcomes are unchanged: AUTHENTICATION_TIMEOUT=300 waits 300 s, the default outer cap tracks 300 rather than the registry 120, and AUTHENTICATION_TIMEOUT=2 still fails in ~2 s. Six resolver tests cover the precedence matrix. No Wrapper special-case in TimeoutConfig.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

zeroshade · 2026-09-03T17:10:20Z

@sfc-gh-pfus can we try again?

## Stack SNOW-2912540 typed session parameters, merge order: 1. [#1339](https://github.com/snowflake-eng/drivers/pull/1339) proto + `sf_core` (merged) 2. [#1340](https://github.com/snowflake-eng/drivers/pull/1340) Python — typed `SessionParametersProxy` (base of #1341) 3. [#1341](https://github.com/snowflake-eng/drivers/pull/1341) JDBC — typed `ParametersRegistry` (**this PR's base**) 4. **This PR** — ODBC typed `ConfigSetting` reads 5. [#1629](https://github.com/snowflake-eng/drivers/pull/1629) drop the deprecated string wire fields (stacked on this branch) ## Summary - The independent session-parameter read sites (autocommit, decimal-as-int, big-number-as-string, max-varchar-size, array-bind threshold, metadata-context bool) plus the TZ-offset-format cache now read `ConnectionGetParameterResponse.typed_value` (`ConfigSetting`) instead of parsing a re-derived dispring. - Three shared helpers (`config_setting_bool` / `config_setting_u64` / `config_setting_string`) live in `api::utils` so those call sites do not each duplicate the oneof match. Native variants are preferred; `string_value` remains a fallback. Non-string variants still collapse to `None` for string-typed parameters such as `TIMESTAMP_TZ_OUTPUT_FORMAT`. - Not doing the full `ParametersRegistry`-style consolidation (dedup of the near-identical RPC-fetch helpers across `connection.rs` / `statement.rs` / `catalog.rs`). That is independent of the typing fix. Merge-resolution restores after catching up to #1341 (from [#1656](https://github.com/snowflake-eng/drivers/pull/1656)): - `select_binding_mode` again requires `threshold > 0` before CSV/stage binding, so `CLIENT_STAGE_ARRAY_BINDING_THRESHOLD=0` stays on inline JSON (same as Python). - `SQLGetTypeInfo` IRD schema again tags string columns as `SQL_WVARCHAR` and `INTERVAL_PRECISION` as `SQL_SMALLINT`, with the corresponding `type_info_tests` restored. ## Test plan - [x] `cargo test -p odbc --lib` — helpers plus restored `type_info_tests` (`string_columns_are_tagged_wvarchar`, `interval_precision_is_smallint_num_prec_radix_is_integer`) - [x] `cargo clippy -p odbc` / `cargo fmt -p odbc` on changed files - [ ] CI `odbc_tests` on this branch after the merge-fix (the three e2e cases that failed on the pre-fix run: all-NULL row with threshold 0; GetTypeInfo WVARCHAR / SMALLINT IRD) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Filip Pawłowski <sfc-gh-fpawlowski@users.noreply.github.com> GitOrigin-RevId: 51ab345

sfc-gh-pfus · 2026-09-07T14:04:55Z

@zeroshade merged, thanks! Should be synced soon.

…connection initialization Imported from #1340. Original PR body: ## Summary - dereference the database handle during connection initialization and use its validated options as the base programmatic configuration layer - apply connection options over database options, including case-insensitive precedence for arbitrary session parameters - use the effective database-plus-connection seed for config resolution, login session parameters, retry and timeout policy, and inherited post-login client settings - reject missing or invalid database handles instead of silently ignoring them This removes the need for wrappers to copy database options onto every connection or independently reproduce database/connection precedence. ## Validation - cargo fmt --all --check - cargo clippy -p sf_core --lib --tests --offline -- -D clippy::all - sf_core connection unit suite: 53 passed - complete sf_core library suite: 1694 passed, 1 ignored, 0 failed - git diff --check --- Internal CI validates this change before merge. On merge to main the outbound mirror will push the commit back to the public repo; close the original mirror PR with a link to the mirrored commit. --------- Co-authored-by: Matt Topol <zotthewizard@gmail.com> Co-authored-by: Ruslan Savenok <ruslan.savenok@snowflake.com> Co-authored-by: Dmitry Rakushev <dmitry.rakushev@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com> GitOrigin-RevId: ac5a8af

sfc-gh-pfus · 2026-09-17T09:36:26Z

Merged in snowflake-eng, mirrored to this repo already.

Copilot AI lite review requested due to automatic review settings August 11, 2026 22:03

github-actions Bot added the sf_core label Aug 11, 2026

Copilot started reviewing on behalf of zeroshade August 11, 2026 22:03 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Comment thread sf_core/src/config/param_store.rs

Comment thread sf_core/src/apis/database_driver_v1/connection.rs

Copilot AI review requested due to automatic review settings August 12, 2026 15:55

github-actions Bot added the nodejs label Aug 12, 2026

Copilot started reviewing on behalf of zeroshade August 12, 2026 15:56 View session