Skip to content

fix(gcp_pubsub source): recover from stalled streams and reduce error noise - #25901

Open
SamyDjemai wants to merge 20 commits into
vectordotdev:masterfrom
SamyDjemai:fix/gcp-pubsub-stalled-stream
Open

fix(gcp_pubsub source): recover from stalled streams and reduce error noise#25901
SamyDjemai wants to merge 20 commits into
vectordotdev:masterfrom
SamyDjemai:fix/gcp-pubsub-stalled-stream

Conversation

@SamyDjemai

@SamyDjemai SamyDjemai commented Jul 20, 2026

Copy link
Copy Markdown

Summary

The gcp_pubsub source could stop processing messages until Vector was restarted, and floods the logs with misleading failed_fetching_events errors on idle/low-volume/bursty subscriptions. This addresses both:

  • Send HTTP/2 keepalive pings so a silently broken connection is detected instead of hanging (interval/timeout configurable via the new keepalive option, mirroring the vector sink).
  • Use a non-blocking keepalive send so a backed-up request channel can't deadlock the task.
  • Add idle_timeout_secs (default 900) to restart a stream that stops delivering responses without erroring.
  • Only escalate stream failures to a component error after max_retry_errors (default 5) consecutive failures with no successful fetch in between; transient failures are logged at debug. A successful fetch resets the counter.

How did you test this PR?

cargo fmt --check, cargo clippy -- -D warnings, unit tests, and vdev check events all pass (run in a Rust 1.95 container).

Testing caveat, in the interest of transparency: the runtime behavior here is hard to cover with automated tests. The code=8a75 "no messages" response is a real-GCP behavior that the Pub/Sub emulator does not reproduce, and exercising the stalled-connection recovery (HTTP/2 keepalive + idle timeout) would require fault-injection infrastructure that doesn't currently exist in the repo. The existing gcp_pubsub emulator tests are also disabled (#24133). Unit tests here therefore only cover the config validation; the retry-escalation and connection-resilience changes are validated by reasoning and would benefit from manual verification against a real GCP subscription.

Change Type

  • Bug fix
  • New feature
  • Dependencies
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the no-changelog label to this PR.

References


Per Vector's AI policy: this PR was substantially generated with an AI agent (Claude Opus 4.8). I have reviewed, understood, and tested the changes, and take ownership of them.

@github-actions github-actions Bot added docs review on hold The documentation team reviews PRs only after a PR is approved by the COSE team. domain: sources Anything related to the Vector's sources domain: external docs Anything related to Vector's external, public documentation and removed docs review on hold The documentation team reviews PRs only after a PR is approved by the COSE team. labels Jul 20, 2026
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@datadog-vectordotdev

This comment has been minimized.

@SamyDjemai

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@github-actions github-actions Bot added the docs review on hold The documentation team reviews PRs only after a PR is approved by the COSE team. label Jul 20, 2026
@SamyDjemai
SamyDjemai marked this pull request as ready for review July 20, 2026 18:42
@SamyDjemai
SamyDjemai requested review from a team as code owners July 20, 2026 18:42

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e6e1a0236

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs
Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs Outdated
- Defer idle-timeout restart while acknowledgements are pending so
  finalized-but-unsent ack IDs are not dropped and redelivered.
- Skip the keepalive when the request channel is full instead of
  reconnecting, preserving queued acks.
- Make idle_timeout_secs optional and derive a default larger than
  keepalive_secs so existing configs with a long keepalive keep starting.

Adds an Option<DurationSecondsWithFrac> Configurable impl in vector-config
to support the now-optional field.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47479d311e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread changelog.d/19418_gcp_pubsub_idle_reconnect.fix.md
Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs Outdated
- Default the HTTP/2 keepalive interval to 300s: Pub/Sub follows gRPC's
  default server enforcement and closes streams pinged more often than
  every 5 minutes with too_many_pings.
- Share the consecutive-failure counter across concurrent streams so a
  successful fetch on any stream resets the source-level count.
- Also gate the idle restart on the request channel being drained, not
  just pending_acks, to avoid dropping queued ack IDs.
- Add the required authors trailer to the changelog fragment.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f0b4d2881

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs
- Always restart on idle timeout: the previous ack-drain guard disabled
  the recovery path exactly when the request stream stalled, hanging the
  source. Unsent ack IDs are dropped and redelivered, acceptable under
  at-least-once delivery.
- Emit the fetch component error once per failure streak (== threshold)
  instead of on every retry past it, so an idle subscription no longer
  resumes the error flood.
- Give max_retry_errors a derivative default so programmatic/generated
  configs keep the threshold instead of collapsing to 0.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d45eddce26

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs
Awaiting a full ack channel inside the select arm stalled the whole loop,
so a backed-up request stream could never be recovered by the idle
timeout. Use a non-blocking send and reconnect when the channel is full;
dropped ack IDs are redelivered under at-least-once delivery.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5526793559

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs
Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs Outdated
Emitting only at the exact threshold silenced a persistently broken
stream after one report and made max_retry_errors: 1 report just the
first failure. Use >= so failures keep surfacing until a success resets
the counter, matching the documented behavior.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc31918cc9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +707 to +710
// The recovery path for a stalled stream: always fires, never
// gated on the request channel that may have stopped draining.
// Unsent ack IDs are dropped and redelivered (at-least-once).
_ = &mut idle_deadline => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep the idle timeout polling in auto-ack mode

When end-to-end source acknowledgements are disabled, which is the default, handle_response still auto-acks by awaiting the bounded ack_ids.send(ids) path. If the request stream is the stalled side and that 8-slot channel fills, the task is parked inside handle_response, so this newly added idle-timeout branch is never polled and the source can still hang until restart. Please make the auto-ack send non-blocking/reconnect as well, or otherwise keep the idle timer active while waiting on that channel.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — this was the one remaining ack path that still blocked. Fixed in 3f992d5: the auto-ack branch in handle_response now uses a non-blocking try_send and reconnects on a full channel, matching the finalizer and keepalive paths, so the idle timeout can always recover a stalled request stream. Dropped ack IDs are redelivered under at-least-once.

The default (acknowledgements disabled) path in handle_response still
awaited the bounded ack channel, which parks the task inside the
stream.next() arm and starves the idle-timeout recovery when the request
stream stalls. Send non-blocking and reconnect on a full channel, like
the finalizer and keepalive paths; dropped ack IDs are redelivered.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f992d59c8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment thread src/sources/gcp_pubsub.rs Outdated
…meout

If Pub/Sub accepts the connection but streaming_pull never resolves (the
no-data startup case), the source could hang with no recovery. Arm the
idle deadline before the startup call so a stuck startup also reconnects.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

emit!(GcpPubsubStreamingPullError { error });

P2 Badge Route startup stream failures through retry threshold

When Pub/Sub returns a transient Unavailable before response headers are received, this startup branch still emits a component error on every retry and never updates consecutive_failures; the new max_retry_errors threshold only applies later in stream.next(). This matters for idle or low-volume subscriptions because this code already notes that streaming_pull may sit in startup until data is available, so the same “no messages” failures can continue to flood failed_streaming_pull logs/metrics instead of being suppressed until the configured threshold.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +740 to +744
match ack_ids_sender.try_send(Vec::new()) {
Ok(()) => {}
Err(mpsc::error::TrySendError::Full(_)) => {
debug!("Keepalive skipped: request stream has acks queued.");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Avoid spinning when zero keepalive meets a full request queue

If a user configures keepalive_secs: 0 and the request stream stops draining, this new non-blocking full-queue path returns immediately; the surrounding tokio::time::sleep(self.keepalive) is then ready again on the next loop, so the task can hot-loop and repeatedly log this debug message instead of yielding until space is available. The previous awaited send backpressured on a full channel, so this regression only shows up with zero keepalive and a stalled/full ack queue.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +674 to +682
// Non-blocking: awaiting a full channel would stall the
// select loop, preventing the idle timeout below from
// recovering a stalled request stream. Reconnect instead;
// dropped ack IDs are redelivered (at-least-once).
match ack_ids_sender.try_send(receipts) {
Ok(()) => {}
Err(mpsc::error::TrySendError::Full(_)) => {
debug!("Request stream stalled sending acks, restarting stream.");
break State::RetryDelay;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the backpressure/blocking is desirable here -- perhaps the ack sender should be a different loop/worker, so the channel provides a bounded buffer, but the actual ack work would not block the main stream loop.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah okay, was unsure since Codex bounced back and forth between blocking and non-blocking. Implemented in c2e578a

Sending acks directly from the select loop meant a stalled request stream
either blocked the loop (starving the idle timeout) or forced dropping
acks. Forward them through a worker task instead: the loop enqueues without
blocking and the worker absorbs backpressure with a bounded send, so a
backed-up request stream applies flow control instead of stalling or
dropping. The worker exits when the stream is torn down.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2e578a6c1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66e33f972a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
…ssure

Replace the ad-hoc try_send/reconnect-on-full handoff with the aws_sqs
worker pattern: a dedicated task owns the finalizer ack stream and forwards
receipts to the bounded request channel with a blocking send. A slow request
stream backpressures the finalizer instead of blocking the main loop or
growing an unbounded queue. pending_acks is shared so the loop still gates
shutdown and keepalive on in-flight acks.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6cc12f9f1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
// Auto-ack: a send error means the request stream is gone; the
// caller reconnects on the next `stream.next()`.
None => {
ack_ids.send(ids).await.ok();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep auto-acks from blocking the idle timer

With the default acknowledgements-disabled configuration, if the outbound request stream stalls while responses continue arriving, this bounded 8-slot channel eventually fills and the awaited send parks the main tokio::select! loop, so the new idle timeout is never polled and the source remains hung until restart. Fresh evidence after the earlier 3f992d5 fix is that the final bounded-worker rewrite restored ack_ids.send(ids).await here while routing only finalized acknowledgements through the worker.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +678 to +680
pending_acks.fetch_sub(1, Ordering::Relaxed);
if status == BatchStatus::Delivered
&& ack_ids_sender.send(receipts).await.is_err()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Decrement pending acks only after enqueueing them

With acknowledgements enabled, if the request channel is full when the worker receives the last finalization, this decrements the count to zero before the acknowledgement has been enqueued and while the following send is blocked. The main task can then take the guarded shutdown branch—or classify this as an inactive stream at the keepalive branch—and drop the request stream, causing that send to fail and the already-delivered message to be redelivered instead of flushing remaining acknowledgements as intended; keep the acknowledgement pending until its send completes or fails.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +670 to +676
let (finalizer, mut ack_stream) =
Finalizer::maybe_new(self.acknowledgements, Some(self.shutdown.clone()));
let mut pending_acks = 0;
let pending_acks = Arc::new(AtomicUsize::new(0));
{
let ack_ids_sender = ack_ids_sender.clone();
let pending_acks = Arc::clone(&pending_acks);
crate::spawn_in_current_span(async move {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid spawning an immortal worker when acks are disabled

With the default acknowledgements-disabled configuration, Finalizer::maybe_new(false, ...) returns an EmptyStream whose poll_next is permanently pending, but this detached worker is still spawned unconditionally. It therefore never observes shutdown or channel closure and never exits; every successful stream setup followed by an idle reconnect, transient error, or configuration reload leaves another Tokio task and channel allocation behind, producing unbounded task growth during long-running error/reconnect cycles.

Useful? React with 👍 / 👎.

- Only spawn the ack worker when acknowledgements are enabled; otherwise
  the finalizer's EmptyStream never resolves and each reconnect leaks a
  task.
- Decrement pending_acks after the worker's forward send resolves so the
  shutdown/keepalive guards don't drop the stream mid-flush.
- Make the main-loop auto-ack and keepalive sends non-blocking; a full
  channel means the request stream stalled, so reconnect instead of
  parking the select loop and starving the idle timeout.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3196b32193

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Finalizer::maybe_new(true, Some(self.shutdown.clone()));
let ack_ids_sender = ack_ids_sender.clone();
let pending_acks = Arc::clone(&pending_acks);
crate::spawn_in_current_span(async move {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Cancel acknowledgement workers when their stream ends

When acknowledgements are enabled and a stream exits while a downstream batch is still pending—for example, through the new idle-timeout branch—this detached task outlives run_once: dropping the finalizer closes its producer, but UnorderedFinalizer waits for outstanding BatchStatusReceivers before ack_stream ends. Because the task handle is discarded, reconnects can strand additional workers holding receipts and a cloned ShutdownSignal; if a batch never finalizes, even one such worker can prevent source shutdown completion indefinitely. Fresh evidence beyond the fixed acknowledgements-disabled case is that the enabled path still spawns an unmanaged worker that drains pending finalizers after its gRPC stream has already been torn down.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs
// error (a reset, or `Unavailable` when an idle subscription has no
// messages). Only escalate to a component error once failures persist,
// to avoid flooding the logs on quiet subscriptions.
fn handle_stream_error(&self, error: tonic::Status) -> State {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply the retry threshold to setup-time statuses

When Pub/Sub returns a transient status before the response stream is established—for example, an Unavailable status in the initial StreamingPull response—Tonic returns it from client.streaming_pull(...), so this new handler is never called and the setup branch emits GcpPubsubStreamingPullError on every retry. This bypasses max_retry_errors and the shared success reset, meaning that with concurrent streams a healthy stream can keep receiving events while a sibling floods component errors at retry_delay_secs cadence; route setup-time statuses through the same retry classification.

Useful? React with 👍 / 👎.

…eshold

- Abort the ack worker when run_once returns (via a local AbortOnDrop
  guard), so a reconnect can't strand workers that keep draining a
  finalizer and holding a ShutdownSignal after their request stream is
  gone (which could otherwise block source shutdown indefinitely).
- Route connect and streaming-pull setup errors through the same
  consecutive-failure threshold as stream errors, so a transient setup
  status on an idle subscription no longer floods failed_streaming_pull /
  failed_connecting on every retry.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b4d1826cc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs
finalizer
.as_ref()
.expect("Finalizer must have been set up for acknowledgements")
.add(ids, notifier);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Bound the finalizer ingress queue

With source acknowledgements enabled, if the outbound request stream stops draining while Pub/Sub continues delivering responses, the ack worker blocks on its bounded send but this call never backpressures the producer: UnorderedFinalizer::add writes each (notifier, ids) pair to the unbounded channel in lib/vector-common/src/finalizer.rs. Each incoming response also resets idle_deadline, so a busy subscription can grow that queue indefinitely and eventually exhaust memory. Fresh evidence beyond the earlier handoff finding is that the final bounded-worker rewrite moved the unbounded ingress into FinalizerSet rather than removing it; bound this handoff or reconnect when it cannot accept more entries.

Useful? React with 👍 / 👎.

@SamyDjemai SamyDjemai Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The unbounded channel is in lib/vector-common, meaning it's used by all ack-enabled sources. This seems out of scope

Comment thread src/sources/gcp_pubsub.rs
Comment on lines +863 to +867
debug!(
message = "Transient error, retrying.",
consecutive_failures,
max_retry_errors = self.max_retry_errors,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve suppressed errors in debug logs

For the first max_retry_errors - 1 connect, setup, or receive failures, the caller does not emit its error and this debug record includes only counters, so the actual transport error or gRPC status is discarded. With concurrent streams, successful responses can repeatedly reset the shared counter and keep a persistently failing sibling below the threshold indefinitely, leaving no actionable record of the failure even at debug level; pass the error and operation context into this path.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +701 to +702
let sent = ack_ids_sender.send(receipts).await.is_ok();
pending_acks.fetch_sub(1, Ordering::Relaxed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Wake the main loop after the final acknowledgement

When shutdown arrives while pending_acks is nonzero, the shutdown branch's guard prevents its future from being polled; if the worker then forwards the last acknowledgement and decrements the atomic to zero, that atomic update does not wake the tokio::select! task to reevaluate the guard. On an otherwise idle subscription, shutdown is therefore delayed until the keepalive or idle timer fires—60 seconds by default and potentially much longer when configured—which can exceed an orchestrator's grace period and force-kill Vector. Notify the main loop when the count reaches zero or receive worker completions through a polled channel.

Useful? React with 👍 / 👎.

Comment thread src/sources/gcp_pubsub.rs
Comment on lines +836 to +838
None => match ack_ids.try_send(ids) {
Ok(()) | Err(mpsc::error::TrySendError::Closed(_)) => {}
Err(mpsc::error::TrySendError::Full(_)) => return Some(State::RetryDelay),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reconnect when the auto-ack channel is closed

With acknowledgements disabled, TrySendError::Closed means the outbound request stream has already dropped its receiver, so these ack IDs cannot be sent. Treating that result as success lets the source continue consuming any buffered or still-arriving responses, silently dropping every corresponding acknowledgement; those responses also keep resetting the idle deadline, so reconnect need not happen promptly. Return RetryNow for the closed case as the keepalive path already does.

Useful? React with 👍 / 👎.

- Reconnect (RetryNow) when the auto-ack channel is closed instead of
  silently dropping acknowledgements.
- Log the underlying error and operation for suppressed transient
  failures so there is an actionable debug record below the threshold.
- Notify the main loop when pending acks drain to zero so shutdown is not
  delayed until the next keepalive or idle timer fires.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ce2770e30

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs Outdated
Comment on lines +851 to +852
.add(ids, notifier);
*pending_acks += 1;
pending_acks.fetch_add(1, Ordering::Relaxed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Increment pending acks before publishing the finalizer

With acknowledgements enabled, a fast downstream—or a response that decodes to no events—can make the notifier ready before this call. Because FinalizerSet::add exposes it to the concurrently running ack worker before pending_acks is incremented, the worker can execute fetch_sub while the counter is zero, wrapping it to usize::MAX; the stream then appears to have acknowledgements pending indefinitely, preventing prompt shutdown and inactive-stream cleanup. Increment the counter before making the finalizer visible to the worker.

Useful? React with 👍 / 👎.

The worker can finalize and decrement pending_acks the instant a batch is
added (fast downstream or a zero-event response), so incrementing after
add() races and can underflow the counter to usize::MAX, leaving the
stream permanently marked as having acks pending. Increment first.
@SamyDjemai

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 119f7d96d9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sources/gcp_pubsub.rs
Comment on lines +703 to +706
let sent = status != BatchStatus::Delivered
// Keep the ack pending until its send resolves, so the
// main task's guards don't drop the stream mid-flush.
|| ack_ids_sender.send(receipts).await.is_ok();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Bound finalized acknowledgements while the request stream stalls

When acknowledgements are enabled and the outbound gRPC request stream stops draining, this worker blocks on the bounded channel, but the main response loop continues adding entries to FinalizerSet's unbounded input and incrementing pending_acks. With sustained incoming Pub/Sub traffic, memory can therefore grow without backpressure until the next keepalive or idle timeout—an arbitrarily long period for configurations with a large keepalive_secs. Propagate the full-channel condition to the main loop or otherwise bound the finalizer queue rather than isolating the blocking send in this worker.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as the other thread, the unbounded channel is in lib/vector-common, shared by all ack-enabled sources.

Comment thread src/sources/gcp_pubsub.rs
Comment on lines +842 to +845
None => match ack_ids.try_send(ids) {
Ok(()) => {}
Err(mpsc::error::TrySendError::Full(_)) => return Some(State::RetryDelay),
Err(mpsc::error::TrySendError::Closed(_)) => return Some(State::RetryNow),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve backpressure when queuing automatic acknowledgements

Under a burst of responses or temporary request-side backpressure, a full eight-entry ack channel does not prove that the stream is stalled; it can simply mean the consumer has not polled the request body yet. Returning here tears down the stream after the events were already delivered downstream but before these IDs were acknowledged, causing redelivery and potentially repeated reconnect/duplicate loops under sustained load. The previous awaited send safely applied backpressure, so automatic acknowledgements should be queued without treating ordinary channel saturation as a connection failure.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This reverses the earlier P1 in this review that made me switch to non-blocking. Going back to a blocking send reintroduces that hang: it parks the select loop so the idle timeout can't recover a stalled stream. Maybe a human maintainer can help here? @milas?

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

Labels

docs review on hold The documentation team reviews PRs only after a PR is approved by the COSE team. domain: external docs Anything related to Vector's external, public documentation domain: sources Anything related to the Vector's sources

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GCP PubSub Source: messages stop being picked up and do not resume until restart

2 participants