Repository navigation
feat(semantic-layer): inject a bounded provider HTTP transport - #45553
mikebridge wants to merge 26 commits into
Conversation
github-actions
Bot
added
api
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #45553 +/- ##
===========================================
- Coverage 84.73% 65.66% -19.08%
===========================================
Files 3069 3075 +6
Lines 205819 206823 +1004
Branches 48408 48450 +42
===========================================
- Hits 174403 135800 -38603
- Misses 28122 67933 +39811
+ Partials 3294 3090 -204
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
Bot
added
the
requires:rebase
Keep bulk-validation uniqueness assertions and transport error coverage together. Move the transport contract into master's consolidated provider guide and update its isolated schema example.
github-actions
Bot
removed
the
requires:rebase
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if _RESOLVER is None: | ||
| _RESOLVER = ThreadPoolExecutor(8, "provider-dns") | ||
| lookup: Future[Any] = _RESOLVER.submit( | ||
| socket.getaddrinfo, host, port, socket.AF_UNSPEC, socket.SOCK_STREAM | ||
| ) | ||
| try: | ||
| answers: Any = lookup.result(timeout=timeout) | ||
| finally: | ||
| lookup.cancel() # Queued work is disposable; running system DNS cannot stop. |
There was a problem hiding this comment.
Suggestion: When eight system DNS calls stall, Future.cancel() cannot stop running lookups; the shared pool stays exhausted, so otherwise valid provider requests queue and time out.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Performance
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/network.py
**Line:** 296:304
**Comment:**
*Performance: When eight system DNS calls stall, `Future.cancel()` cannot stop running lookups; the shared pool stays exhausted, so otherwise valid provider requests queue and time out.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—Future.cancel() on a not-yet-running lookup only marks it cancelled; the work item stays in the executor queue until a worker pops it. With the eight workers stuck, 108 timed-out calls left 100 cancelled items queued (observed), so memory grows even though every caller has already failed. Would a bounded submission count, or a shared in-flight limit that fails fast, address it?
There was a problem hiding this comment.
Yes. Bound all outstanding resolver submissions—both running lookups and queued work—and wait for a slot only until the request deadline. If capacity is exhausted, fail the request instead of adding another item to the executor’s unbounded queue.
The accounting matters: don’t release a slot merely because the caller timed out or Future.cancel() succeeded. A cancelled queued future can remain in the queue, and cancellation may mark it done immediately. Either leave timed-out futures submitted and release their slots only when the worker actually completes them, or use a bounded queue that can reliably account for dequeue/removal.
This prevents unbounded queue growth and makes overload fail fast. It won’t restore DNS availability while all resolver threads are stuck in uncancellable system lookups; recovering from that would require stronger isolation, such as resolver processes.
There was a problem hiding this comment.
Good catch. DNS admission now bounds outstanding submissions as well as workers. A timed-out request keeps its slot until the submitted lookup actually finishes, and a saturated pool fails immediately instead of queueing. A blocked-worker test covers queued work and slot recovery afterwards, in e791fe9.
github-actions
Bot
removed
the
requires:rebase
github-actions
Bot
added
the
requires:rebase
Merge apache master while retaining provider error normalization and transport injection. Address Joe's r4394124322 by submitting DNS work outside the native lock and creating the scheduler lock after gevent patching; publish only after successful startup. Preserve timeout classification through HTTP wrappers. Address r4394124328 with real-response body tripwires and set the test certificate context's TLS minimum.
Guard only lazy lock publication so native threads share one scheduler while preload gevent startup remains outside the native guard. Reset the guard after fork. Keep host transport construction outside provider fault attribution. Add widened-window race and host-fault regression coverage for the two review LOWs.
github-actions
Bot
removed
the
requires:rebase
Close each abandoned response and its deadline-owned duplicate descriptors before following the next hop, so single-threaded keep-alive providers can accept it. Preserve the original deadline across hops. Add the real-server regression and strengthen refused-body and non-default-port tests for Joe's three redirect review threads.
Exercise the no-scheduler release path without initializing deadline infrastructure, closing the final branch-coverage gap while keeping production behavior unchanged.
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
removed
the
requires:rebase
| session.mount("http://", adapter) | ||
| hop: int | ||
| for hop in range(MAX_REDIRECTS + 1): | ||
| with session.request( |
There was a problem hiding this comment.
With LOG_LEVEL=DEBUG, urllib3's connection-pool logger writes the request path and query string for this call, so a provider URL like https://provider.example/catalog?token=SECRET puts the token in the application log on every successful request. The sanitized warning in this module doesn't cover those records. Should the transport redact or suppress urllib3.connectionpool debug output for provider calls?
There was a problem hiding this comment.
Good catch, fixed in b54ff69. The transport now filters urllib3.connectionpool records only while a provider request is active. A DEBUG loopback test fails without the filter (the query token shows up in the log) and checks that unrelated concurrent HTTP still logs.
There was a problem hiding this comment.
This only covers the urllib3.connectionpool logger. With the pinned urllib3 2.8.0, a header-parse failure is logged at WARNING by urllib3.connection as Failed to parse headers (url=...), and that URL includes the query string. A provider reply of HTTP/1.1 200 OK, Content-Length: 2, then a line like Not-A-Header would put ?token=SECRET in the application log at default levels, before the transport sanitizes the error. Could the filter also cover the urllib3.connection logger?
There was a problem hiding this comment.
Good catch. The header-parse warning from urllib3.connection carried the full URL. The request-scoped filter now covers that logger too (transport.py:62-71). test_malformed_provider_headers_do_not_log_query_tokens sends a reply with a malformed header line and checks that ?token=SECRET never reaches a log record. It fails without the change.
| with ( | ||
| ThreadPoolExecutor(1) as worker, | ||
| _framing_server(public_tls[0], mode) as (port, ready, release), | ||
| _internal_hosts(allow_internal), |
There was a problem hiding this comment.
_internal_hosts(allow_internal) enters its Flask app context in the pytest thread, but provider_body calls build_provider_transport() inside the ThreadPoolExecutor worker, which has no app context. Both parameter values therefore build a transport with allow_internal_hosts=False, and the allow_internal=True cases never reach the internal-host connection path. For example, changing _PinnedHTTPConnection._new_conn to call bound.watch(sock) only when public_only is set would still pass these framing-deadline cases. Could the transport be built inside the app context before it is submitted to the worker, or the context pushed in the worker?
There was a problem hiding this comment.
You were right: the worker thread had no app context, so both parameters built a public-only transport. The test now builds the transport inside the app context and only sends the request from the worker. The internal cases use plain HTTP on 127.0.0.1, so they reach the internal-host connection path. Moving construction back into the worker makes the allow_internal=True cases fail again.
github-actions
Bot
added
the
requires:rebase
Joe review threads r4415570656 and r4415570062 identified a urllib3.connection header warning that could log provider query tokens and a framing test whose worker lost the Flask internal-host context. Filter the warning during provider requests and build the test transport inside that context so the internal path is exercised.
github-actions
Bot
added
requires:rebase
github-actions
Bot
removed
the
requires:rebase
SUMMARY
TL;DR: Give semantic-layer extensions a host-provided HTTP transport with a mandatory deadline and response byte cap. Keep destination policy in Superset.
Background: Providers need HTTP during construction, schema discovery and query execution. The SDK currently has no portable HTTP capability, so provider helpers can depend on a particular host's private code.
Problem: Moving those helpers without preserving address pinning, TLS, redirect checks and absolute deadlines would weaken the existing transport. A timeout on an individual read does not bound DNS, slow framing or a full multi-request operation.
What this changes:
transport=keyword on provider construction and both schema-discovery entry points. Update all five host call sites and in-tree providers/fixtures together; keep static read-only schema access and memo identity unchanged.SEMANTIC_LAYER_ALLOW_INTERNAL_HOSTS = False. Opting in permits internal/HTTP endpoints but retains budgets, TLS verification, redirect checks and direct connections without environment proxies. Internal mode honorsREQUESTS_CA_BUNDLE/CURL_CA_BUNDLEfor private TLS certificates; neither mode discovers.netrccredentials.This prepares the unreleased SDK contract. Existing downstream provider bundles must migrate their HTTP helpers and advance their core pin together in a separate change. This PR does not claim that old bundles work against the new signatures.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: no UI change.
TESTING INSTRUCTIONS
Run from this checkout using its development environment:
PYTHONPATH=.:superset-core/src python -m pytest -q -o addopts='' \ tests/unit_tests/semantic_layers tests/unit_tests/commands/semantic_layer \ superset-core/tests tests/unit_tests/utils/test_network.pyThe tests use local listeners and fakes, with an explicit loopback-only socket check in the transport suite. They cover all 33 original transport test functions, plus complete-answer DNS rejection, no second resolution, every stalled phase, exact/exceeded byte caps, safe logs, unread errors/redirects, and ten concurrent requests with descriptor cleanup. The connect-stall fixture blocks real socket I/O at the connect boundary; it is not a kernel SYN-blackhole experiment.
The same phase/concurrency cases are also exercised in a fresh subprocess that calls
gevent.monkey.patch_all()before importing pytest. A real Apache app factory boot loads the updated local stub against disposable SQLite; no live provider, real credentials, shared database or container is used.Final candidate validation: 1,901 semantic-layer tests passed with 100.00% coverage, 53 SDK tests passed, and 7 SDK conformance registration tests passed. Changed-file hooks including MyPy passed. Earlier transport validation also passed 16 isolated-gevent cases; those were not rerun for the dead-code removal and master merge. All 33 reference transport test functions are ported. New regression tests cover closed deadlines, shutdown errors, fork reset and invalid URLs.
Follow-up: harden the standalone
ProviderRequestErrorconstructor against string/integer subclasses that override formatting, and validate status fields. The stacked host boundary validates these fields independently; this constructor hardening is not included here.ADDITIONAL INFORMATION
SEMANTIC_LAYERSavailability gate; provider destination opt-out is a default-off operator configuration setting.ProviderTransportand required keyword.Compatibility: this is a first-release SDK gate. Update provider bundles in lockstep with the host SDK before enabling them. Rollback requires the matching host/bundle pair; this change makes no metadata database writes or migrations. OAuth and webhook transport settings are unaffected. OS DNS calls already in progress cannot be cancelled; caller waiting is bounded and the resolver uses at most eight workers. Independent security review completed; remote CI and downstream bundle/query acceptance are still pending. No live-provider acceptance is claimed.
Port coverage
All Apache targets are in
tests/unit_tests/semantic_layers/transport_test.py. Proxy behavior and error wording changes below are intentional; every call is bounded.test_https_guard_ignores_background_requeststest_https_guard_ignores_background_requeststest_internal_address_is_refused_before_connectingtest_internal_address_is_refused_before_connectingtest_any_internal_answer_refuses_the_hosttest_any_internal_answer_refuses_the_hosttest_translated_ipv6_answers_are_checked_as_ipv4test_translated_ipv6_answers_are_checked_as_ipv4test_checked_connection_ignores_background_connectionstest_checked_connection_ignores_background_connectionstest_public_tls_ignores_background_loopback_lookupstest_public_tls_ignores_background_loopback_lookupstest_default_path_verifies_the_configured_hostnametest_default_path_verifies_the_configured_hostnametest_default_path_follows_same_origin_get_redirects_onlytest_default_path_follows_same_origin_get_redirects_onlytest_flag_allows_local_http_development_endpointstest_flag_allows_local_http_development_endpointstest_flag_off_keeps_local_endpoints_refusedtest_flag_off_keeps_local_endpoints_refusedtest_post_redirects_are_not_followedtest_post_redirects_are_not_followedtest_redirect_chains_are_boundedtest_redirect_chains_are_boundedtest_error_message_carries_status_onlytest_error_message_carries_provider_and_status_onlytest_transport_failure_hides_the_destinationtest_transport_failure_hides_the_destinationtest_failure_logs_never_carry_url_secretstest_failure_logs_never_carry_url_secretstest_malformed_redirects_raise_the_safe_errortest_malformed_redirects_raise_the_safe_errortest_malformed_endpoint_urls_raise_the_safe_errortest_malformed_endpoint_urls_raise_the_safe_errortest_bounded_body_is_read_through_the_checked_connectiontest_bounded_body_is_read_through_the_checked_connectiontest_bounded_body_is_refused_past_its_byte_captest_bounded_body_is_refused_past_its_byte_captest_bounded_body_stops_at_the_deadline_while_drippingtest_bounded_body_stops_at_the_deadline_while_drippingtest_bounded_read_after_progress_waits_only_for_the_time_lefttest_bounded_read_after_progress_waits_only_for_the_time_lefttest_bounded_lookup_waits_no_longer_than_the_deadlinetest_bounded_lookup_waits_no_longer_than_the_deadlinetest_bounded_body_refuses_internal_destinationstest_bounded_body_refuses_internal_destinationstest_bounded_error_bodies_are_not_read_or_loggedtest_bounded_error_bodies_are_not_read_or_loggedtest_bounded_body_refuses_encoded_responsestest_bounded_body_refuses_encoded_responsestest_bounded_post_redirects_are_not_followedtest_bounded_post_redirects_are_not_followedtest_bounded_body_refuses_an_expired_deadlinetest_bounded_body_refuses_an_expired_deadlinetest_bounded_redirect_bodies_are_never_readtest_bounded_redirect_bodies_are_never_readtest_bounded_framing_obeys_the_absolute_deadlinetest_bounded_framing_obeys_the_absolute_deadlinetest_bounded_lookup_and_connect_attempts_share_the_deadlinetest_bounded_lookup_and_connect_attempts_share_the_deadlinetest_bounded_internal_requests_ignore_environment_proxiestest_bounded_internal_requests_ignore_environment_proxiestest_unbounded_internal_requests_keep_environment_proxiestest_former_unbounded_internal_requests_ignore_environment_proxiestest_dns_failure_is_not_an_address_policy_refusaltest_dns_failure_is_not_an_address_policy_refusal