GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [pipeline-manager] Retry opening the connection to a pipeline for up to 35 s by gz · Pull Request #7868 · feldera/feldera

Conversation

@gz

@gz gz commented Nov 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Problem

Right after a pipeline (re)start the api-server can fail to open a connection to it:

  • cluster DNS keeps answering with the previous pod's IP for the record's TTL (30 s with kube-dns and NodeLocal DNSCache on GKE), measured in feldera/cloud#2179;
  • a cached negative answer can outlive the new Service;
  • a pod can refuse connections for a moment before its server listens.

Since 004e386 the proxy answers such failures with 503 PipelineInteractionConnectFailed and the SDK and fda retry on it, but the manager itself makes a single attempt, so every other client still sees the 503. feldera/cloud#2179 removes the restart case by giving each deployment its own host name; a pod replaced within a deployment (preemption, the fault-tolerance tests' pod kills, feldera/cloud#1470) keeps its name and still hits the window.

Change

A connect failure means the request was never sent, so repeating it cannot apply it twice. forward_http_request_to_pipeline and streaming_proxy now retry connect-phase failures (timeout, refused, resolver error; not a disconnect after the connection was open) every 500 ms for up to 35 s, which outlasts the DNS window with an attempt to spare.

  • A streaming request body is shared across attempts (SharedPayload) and only replayed while no attempt has read from it; awc opens the connection before it reads the body, so a connect failure leaves it untouched.
  • When the proxy gives up, the PipelineInteractionConnectFailed message says how many attempts it made over how long. When a retry rescues a request, the proxy logs it, which also gives the observability asked for in feldera/cloud#2125 a first data point.
  • The classification lives in one place, is_connect_failure, used by both the retry loop and the error mapping.
  • Before each further attempt the proxy re-checks the pipeline through the same check_pipeline the first attempt used (cached, evicted on every pipeline change). A pipeline that was stopped or became Unavailable meanwhile ends the loop with its status error instead of running down the budget; a pipeline that was restarted meanwhile is followed to its new location.

Not changed: the runner's own status polling, which has its own semantics for an unreachable pipeline, the websocket proxy, and the static forward_http_request_to_pipeline used by the metrics scrape, which walks every pipeline in sequence from a one-time snapshot and therefore keeps making a single attempt (ConnectRetry::NONE).

Testing

  • test_streaming_proxy_retries_until_the_pipeline_listens: nothing listens on the port for 1.5 s, then a server appears; the POST succeeds and the server receives the whole body after the refused attempts.
  • test_streaming_proxy_connect_retry_budget: nothing ever listens; PipelineInteractionConnectFailed arrives only after the 1 s budget and names the attempts.
  • test_streaming_proxy_connect_retry_stops_when_the_pipeline_is_gone: a re-check that reports the pipeline unavailable ends the loop at once with that error.
  • test_streaming_proxy_connect_retry_follows_a_moved_pipeline: a re-check that returns a new location sends the next attempt there.
  • Each pair fails with its mechanism disabled (the retry loop, the re-check). The existing refused-connection test now runs with a zero budget to keep testing the single-attempt classification.
  • cargo test -p pipeline-manager --lib runner::interaction: 14 passed. clippy and pre-commit clean.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gz
gz requested a review from blp November 6, 2026 06:00
@gz
gz force-pushed the gz/pipeline-connect-retry branch 2 times, most recently from b8e4cf7 to c04c659 Compare November 6, 2026 06:16
@gz

gz commented Nov 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There is a case to be made that we should retry only at the edge to avoid retry amplification -- which is a principle this PR violates. On the other hand the api-server knows best if it makes sense to retry because it can tell if the pod is supposed to be up.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gz
gz force-pushed the gz/pipeline-connect-retry branch 4 times, most recently from 2ba88d6 to 29b97f3 Compare November 6, 2026 06:43
@gz
[pipeline-manager] Retry opening the connection to a pipeline for up …
180f7b3
…to 35 s

Right after a pipeline (re)start the api-server can fail to connect to it: cluster DNS keeps answering with the previous pod's IP for the record's TTL (30 s with kube-dns and NodeLocal DNSCache on GKE), a cached negative answer can outlive the new Service, and a pod can refuse connections for a moment before its server listens. Until now the proxy made one attempt and answered 503 PipelineInteractionConnectFailed, leaving every client to retry.

A connect failure means the request was never sent, so repeating it cannot apply it twice. Both proxy paths now retry such failures every 500 ms for up to 35 s, which outlasts the DNS window with an attempt to spare. A streaming request body is shared across attempts and is only replayed while no attempt has read from it. When the proxy gives up, the error says how many attempts it made; when a retry rescues a request, the proxy logs it.

Tests: a server that starts listening 1.5 s after the first attempt receives the whole POST body, and a port nothing listens on yields PipelineInteractionConnectFailed only after the budget. Both fail with the retry disabled.
Signed-off-by: Gerd Zellweger <[email protected]>
@gz
gz force-pushed the gz/pipeline-connect-retry branch from 29b97f3 to 180f7b3 Compare November 6, 2026 06:52
@blp

blp commented Nov 6, 2026

Copy link
Copy Markdown
Member

The manager-side retry is the right layer: only the manager knows that the request never reached the pipeline, and it can stop when the pipeline stops or moves. But it now stacks with the client retries, because both clients also retry PipelineInteractionConnectFailed:

Client Retries Worst case today Worst case with this PR
Python SDK, default 3, backoff 2/4/8 s about 15 s about 154 s (4 × 35 + 14)
fda, default (rest-api RetryPolicy) 3, backoff 2/4/8 s about 15 s about 154 s
Python SDK in CI (deadline_seconds=300) until 300 s about 300 s about 370 s

Neither client sets a read timeout by default, so attempts do not overlap. But a user who sets timeout below 35 s now gets FelderaTimeoutError on ingress, because the SDK retries a read timeout only for idempotent requests. Before this PR, that request got a 503 and retried.

The clients can't just stop retrying this code, because v0.378.0 already sends it after a single attempt (#7860). My suggestion:

  1. Add the attempt count to the error details (for example "attempts": N), not only to the message.
  2. In a follow-up to the SDK and rest-api, retry PipelineInteractionConnectFailed only when attempts is missing or 1.
  3. Document the 35 s budget in the client timeout docs.

References: python/feldera/rest/retry.py:74-81, python/feldera/rest/_httprequests.py:72-86,196-199, python/feldera/testutils.py:91-101, crates/rest-api/src/retry.rs:40-48,112-127, crates/fda/src/cli.rs:186-202.

The times in the table are calculated from the code, not measured.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@blp blp mentioned this pull request Nov 6, 2026
Define which layer retries each kind of request failure #7879
Open
4 tasks
blp
blp reviewed Nov 6, 2026
View reviewed changes

@blp blp left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review of the retry change, as a follow-up to the earlier comment about client retry stacking. I checked these comments against the code at 180f7b3 but did not compile it or run tests. Two comments marked "Not verified" depend on awc behavior or lock timing that I did not confirm.

The most important ones are the 70 s worst case after a cache hit, the 35 s budget for best-effort callers with short timeouts, and permanent connect errors (TLS) that are retried as transient.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
timeout,
body_size_limit,
accept_encoding,
ConnectRetry::DEFAULT,

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a cache hit, an error removes the cache entry and calls this function again (lines 660-675), and that call also uses ConnectRetry::DEFAULT. When a cached Running pipeline refuses connections, the client therefore waits about 70 s for the 503, not the 35 s the description gives. A load balancer or client with a 60 s timeout cuts the request first. Possible fix: skip the cache-evict retry when the error is a connect failure that already used the budget, or give the second call ConnectRetry::NONE.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
timeout,
body_size_limit,
accept_encoding,
ConnectRetry::DEFAULT,

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All _by_name callers now get the 35 s budget, also callers that pass a short timeout because the call is best effort:

Caller Timeout Effect
fetch_connector_error_stats (pipeline_management.rs:931), run for each pipeline with join_all by the StatusWithConnectors list 500 ms One pipeline that refuses connections blocks the web console list for 35-70 s. Only on the fallback path for pipelines with no connector_stats in their status details.
support bundle (support_data_collector.rs:142) per endpoint Endpoints are fetched one at a time, so an unreachable pipeline can stall the bundle for minutes.
suspend (pipeline_management.rs:2118) 120 s Probably fine.

Suggestion: let the caller choose the ConnectRetry, or limit the budget by the caller's timeout.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
/// Whether `error` says the connection to the pipeline was never opened, so the request
/// was not sent.
fn is_connect_failure(error: &SendRequestError) -> bool {
matches!(error, SendRequestError::Connect(connect) if !matches!(connect, ConnectError::Disconnected))

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This retries every ConnectError except Disconnected. Some variants are permanent, for example SslError, SslIsNotSupported and H2NotSupported. A TLS configuration error then looks like a 35 s (or 70 s) hang, and it ends in PipelineInteractionConnectFailed, which the SDK and fda retry again. Suggest an allow-list: Timeout, Resolver, NoRecords, Io (refused).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
}
Err(error) => error,
};
if !is_connect_failure(&error) || !may_retry() || started.elapsed() >= retry.budget {

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not verified: the awc request timeout also covers the connect phase. If the request timeout is shorter than the connector's 5 s timeout, a connect to a dead (stale DNS) address fails as SendRequestError::Timeout, not Connect(Timeout), so it is not retried. Then the stale-DNS case is retried only for callers with a timeout longer than 5 s. A test with a non-routable address and a short timeout would show which way it goes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
/// leaves room for an attempt that starts after a 30 s entry expired.
pub(crate) const DEFAULT: Self = Self {
budget: Duration::from_secs(35),
pause: Duration::from_secs(1),

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pause is 1 s, but the PR description says 500 ms. One of the two needs to change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
tokio::time::sleep(retry.pause).await;
// The pipeline may have been stopped or restarted meanwhile: give up with its status
// error, or follow it to its new location.
if let Some(current) = recheck().await?

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? replaces the connect failure with any recheck error. If the cache entry expired (5 s TTL) and the DB read fails for a short time, or the pipeline is briefly not running during an in-place restart, the client gets that error instead of PipelineInteractionConnectFailed. Clients may not retry the replacement error. Consider returning the status error only when the recheck clearly says the pipeline is stopped or unavailable, and otherwise continuing the loop.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
tokio::time::sleep(retry.pause).await;
// The pipeline may have been stopped or restarted meanwhile: give up with its status
// error, or follow it to its new location.
if let Some(current) = recheck().await?

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not verified: on a cache miss, check_pipeline takes the global db.lock(). Every pipeline change clears the cache, so many concurrent retry loops (the list endpoint with several unreachable pipelines, or many ingress clients during a restart) all queue on that one lock about once per second. This can slow unrelated API requests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
type Item = Result<Bytes, PayloadError>;

fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
self.was_read.set(true);

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was_read becomes true on every poll, also when the inner payload returns Pending and no bytes were taken. Then one poll that consumed nothing stops all further retries. Setting it only on Poll::Ready(Some(_)) keeps the safety guarantee without that false positive. The practical effect depends on whether awc polls the body before a connect error is reported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/pipeline-manager/src/runner/interaction.rs
.pipeline_url(&location, endpoint, query_string);
let timeout = timeout.unwrap_or(Self::PIPELINE_HTTP_REQUEST_TIMEOUT);
streaming_proxy(client, &url, pipeline_name, &request, body, timeout).await
let recheck = || async move {

@blp blp Nov 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this closure is a copy of the one at line 623, and the two build_request closures repeat the same client.request().timeout().force_close().no_decompress() chain. A helper such as fn recheck_url(&self, tenant_id, name, endpoint, query_string) keeps the two proxy paths from drifting apart.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
blp
blp approved these changes Nov 6, 2026
View reviewed changes

@blp blp left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right approach, after looking at it at a high level. I think that there are refinements in the code that can be done, and I think that we should consider looking at retries more systematically (see the linked issues that I filed) but this is a good start.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

This branch has not been deployed

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

Reviewers

@blp blp blp approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@gz @blp
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.