Skip to content

fix(adbc): set connect timeout to bound stalled dials (DI-5183) - #108

Draft
theyostalservice wants to merge 1 commit into
mainfrom
py/di-5183-adbc-connect-timeout
Draft

fix(adbc): set connect timeout to bound stalled dials (DI-5183)#108
theyostalservice wants to merge 1 commit into
mainfrom
py/di-5183-adbc-connect-timeout

Conversation

@theyostalservice

Copy link
Copy Markdown

Why

DI-5183 documents dbt Semantic Layer clients that stall against Arrow Flight SQL with no way to distinguish "slow" from "actually dead." BaseADBCClient currently sets no RPC timeout at all when building db_kwargs for adbc_connect (there was a literal # TODO: timeouts are not implemented for ADBC), so a hung connection attempt blocks forever.

This PR closes the connect-time half of that gap: adbc_driver_flightsql supports fine-grained, per-RPC-type timeout options, and .connect bounds the dial itself.

Handshake-vs-dial verification (read this before reviewing the diff)

Before implementing, I verified — against the actual driver source, not assumption — exactly what adbc.flight.sql.rpc.timeout_seconds.connect bounds, since the customer-reported symptom in DI-5183 specifically names the Flight Handshake RPC as the one that hangs.

Finding: .connect bounds only the raw gRPC dial (TCP/TLS connection establishment). It does not, and cannot, attach a deadline to Handshake.

Traced through apache/arrow-adbc (go/adbc/driver/flightsql, pinned driver version 1.12.0, confirmed against the go/adbc/v1.12.0 tag):

  • timeouts.go connectParams() returns grpc.ConnectParams{MinConnectTimeout: t.connectTimeout}, wired into the client's dial options in flightsql_database.go getFlightClient() via grpc.WithConnectParams(...). This is a transport-level construct governing the dial only.
  • The only place any of the four .timeout_seconds.* options attach a deadline to an RPC is timeouts.go getTimeout(), called from unaryTimeoutInterceptor/streamTimeoutInterceptor. Its method-suffix switch matches DoGet, GetFlightInfo, DoPut, DoActionHandshake is not one of them, for any of the four timeout options, including .connect.
  • Additionally (a stronger finding than the ticket anticipated): for the bearer-token auth this SDK uses (DatabaseOptions.AUTHORIZATION_HEADER), the driver never issues a Handshake call in the first place. flightsql_database.go getFlightClient() calls flightsql.NewClient(target, nil, middleware, dialOpts...) — passing nil for the ClientAuthHandler parameter. In apache/arrow-go's arrow/flight/client.go, Handshake() is only invoked from two places: AuthenticateBasicToken() (username/password auth, which we don't use — Open() returns early once it sees an Authorization header already set) and Authenticate() (which requires a non-nil authHandler, which is never set on our path). So Handshake isn't just unbounded here — for this client, it isn't even called.

Conclusion: the loanDepot Datadog evidence in DI-5183 (Call completed for method='HANDSHAKE') was for Power BI's ODBC driver (grpc-c++), a different implementation from this Go-based ADBC driver, which evidently does call Handshake through some other code path. It's not established that the ADBC-based cases (AwayDay, INC-7884) hang inside Handshake specifically — this driver, with our auth configuration, may not exercise that RPC at all. The real hang surface for this client is the dial (grpc.NewClient's lazy connection, triggered on the first RPC issued during Open()), which .connect does bound.

Known limitation: if some other, unverified code path in this driver did ever call Handshake, this fix would not bound it — there is no ADBC-exposed option that attaches a deadline to Handshake specifically. Given the auth-handler tracing above, I don't believe that applies to this client, but flagging it since I can't prove a negative over the driver's entire surface.

What

  • BaseADBCClient._extra_db_kwargs() now sets adbc.flight.sql.rpc.timeout_seconds.connect to 15 seconds. .query/.fetch/.update are deliberately left unset — untouched from today — since real customer queries can legitimately run 10+ minutes (see semantic-layer-gateway's ingress chart, which sets a 670s proxy-read-timeout for exactly that reason). A short timeout must never apply to query execution.
  • The vendored adbc_driver_flightsql.DatabaseOptions enum (as pinned here) doesn't expose a TIMEOUT_CONNECT member, even though the underlying Go/C driver has accepted this string key since before this SDK's declared minimum adbc-driver-flightsql>=0.11.0 (confirmed: the option was added to arrow-adbc main on 2024-02-09, and adbc-driver-flightsql 0.11.0 wasn't released until 2024-03-31). So the key is set as a raw string constant with a comment explaining why.
  • 15s was chosen with wide margin over observed latency to SLG: DI-5183's Datadog investigation found the (distinct, RPC-level) Handshake call completes in ~650-810ms fleet-wide — a reasonable proxy for normal network+TLS overhead on this path — so 15s gives roughly 20x headroom, generous enough to absorb jitter while still failing far faster than the unbounded hang it replaces.
  • Updated the stale # TODO: timeouts are not implemented for ADBC comment in _handle_error to reflect that connect timeouts are now implemented while query/fetch/update remain intentionally unset.
  • Added a changie fragment (.changes/unreleased/Fixes-*.yaml) per this repo's changelog convention.

Testing

  • tests/api/adbc/test_base_client.py::test_connect_timeout_bounds_hung_dial: spins up a raw TCP server that accepts connections but never speaks any protocol on them (simulating exactly the DI-5183 symptom — a connection that appears established but never progresses), then asserts that connecting through BaseADBCClient._get_connection_context_manager() fails within a bounded window instead of hanging. Red-green confirmed: with the .connect db_kwarg commented out, this test genuinely hung for the full 10s bailout window (proving it wasn't already bounded some other way); with the fix restored, it passes in ~1s (using a monkeypatched 1s timeout so the test stays fast regardless of the 15s production default).
  • tests/api/adbc/test_base_client.py::test_extra_db_kwargs_sets_only_connect_timeout: asserts .connect is set and .query/.fetch/.update are absent from db_kwargs, guarding against scope creep.
  • Full unit suite: pytest --ignore tests/integration/ --server-schema tests/server_schema.gql — 80 passed (78 pre-existing + 2 new), 0 failed.
  • hatch run dev:ruff check / ruff format --check / basedpyright / python -m mypy dbtsl all clean on the changed files (lefthook pre-commit hooks passed on commit).
  • Integration tests were not run (require live SL_HOST/SL_TOKEN/SL_ENV_ID credentials against a real Semantic Layer account); out of scope for this change.

Refs


Drafted by Claude Sonnet 5 under the direction of @theyostalservice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant