Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions internal/impl/salesforce/input_salesforce_cdc.go
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,10 @@ func (e *salesforceCDCInputExecutor) emitSnapshot(
return fmt.Errorf("track snapshot checkpoint: %w", err)
}

// The ack error is deliberately ignored: nacks are replayed by
// auto_replay_nacks (the default), and disabling that is a documented
// opt-in to DROP rejected messages, so the checkpoint must advance past
// them rather than pin the tracker.
ackFn := func(ackCtx context.Context, _ error) error {
resolved := resolveFn()
if resolved == nil {
Expand All @@ -740,11 +744,11 @@ func (e *salesforceCDCInputExecutor) emitSnapshot(
e.stateMu.Lock()
e.state.SnapshotComplete = acked.SnapshotComplete
e.state.RestCursor = acked.RestCursor
err := e.saveStateLocked(ackCtx)
persistErr := e.saveStateLocked(ackCtx)
e.stateMu.Unlock()

if err != nil {
return fmt.Errorf("persist snapshot checkpoint: %w", err)
if persistErr != nil {
return fmt.Errorf("persist snapshot checkpoint: %w", persistErr)
}
return nil
}
Expand Down Expand Up @@ -900,6 +904,10 @@ func (e *salesforceCDCInputExecutor) flushTopic(
return fmt.Errorf("track checkpoint for %s: %w", topic, err)
}

// The ack error is deliberately ignored: nacks are replayed by
// auto_replay_nacks (the default), and disabling that is a documented
// opt-in to DROP rejected messages, so the checkpoint must advance past
// them rather than pin the tracker.
ackFn := func(ackCtx context.Context, _ error) error {
resolved := resolveFn()
if resolved == nil {
Expand All @@ -912,11 +920,11 @@ func (e *salesforceCDCInputExecutor) flushTopic(

e.stateMu.Lock()
e.state.Topics[topic] = acked
err := e.saveStateLocked(ackCtx)
persistErr := e.saveStateLocked(ackCtx)
e.stateMu.Unlock()

if err != nil {
return fmt.Errorf("persist checkpoint: %w", err)
if persistErr != nil {
return fmt.Errorf("persist checkpoint: %w", persistErr)
}
return nil
}
Expand Down
246 changes: 225 additions & 21 deletions internal/impl/salesforce/salesforcegrpc/subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
package salesforcegrpc

import (
"bytes"
"context"
"errors"
"fmt"
Expand All @@ -30,6 +31,16 @@ import (
// provides safety margin without meaningfully slowing startup.
const subscribeSettleDelay = 5 * time.Second

// maxConsecutiveDecodeFailures bounds redelivery attempts for an event whose
// Avro decode keeps failing at the same replay position. A decode failure
// against a successfully fetched schema is deterministic, and the replay
// anchor advances per delivered event, so each reconnect redelivers from
// exactly the failing event; once the same position has failed this many
// times in a row the payload is treated as permanently undecodable and the
// stream fails loudly. Schema-fetch failures never count toward this bound -
// they are transport-class errors governed by the reconnect policy.
const maxConsecutiveDecodeFailures = 5

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This doc comment states the opposite of what the code now does.

Schema-fetch failures never count toward this bound - they are transport-class errors governed by the reconnect policy.

recordSchemaFailure bounds deterministic schema-fetch failures by exactly this constant, explicitly regardless of the reconnect policy:

count := s.schemaFailures.record(replayID)
if deterministic {
return count >= maxConsecutiveDecodeFailures
}
budget := s.client.maxReconnect
return budget > 0 && count >= budget
}

and so does the unanchored inline-retry path:

for attempt := 0; err != nil; attempt++ {
if deterministicSchemaFailure(err) && attempt >= maxConsecutiveDecodeFailures-1 {
// A bad or inaccessible schema cannot heal by retrying.
break
}

failDecode's doc carries the same stale claim ("Schema-fetch failures do NOT come through here - they are transport-class errors governed by the reconnect policy", L172-L174): the first clause is still true, but the "transport-class / governed by the reconnect policy" characterisation no longer holds for the deterministic class.

A comment that contradicts the failure-handling policy it documents is a maintenance hazard on exactly the semantics this PR is iterating on — CONTRIBUTING.md §3.1.2 (self-documenting code) and §1.2.3 (known limitations and edge cases documented accurately). Suggest rewording both to say that only transient schema-fetch failures are governed by the reconnect policy, while deterministic ones share this bound, and renaming the constant to something that covers both paths it now bounds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6eb239f — the constant is renamed maxDeterministicPositionFailures (it bounds both decode failures and deterministic schema failures), and the failDecode, struct-field, and batch-loop comments now all state the deterministic/transient split: only transient schema-fetch failures are governed by the reconnect policy.


// Subscription owns one subscribe stream for a single Pub/Sub topic. It reuses
// the parent Client's connection, auth, and schema cache.
type Subscription struct {
Expand All @@ -46,6 +57,13 @@ type Subscription struct {
ready chan struct{}
streamErr error
state StreamState
// decodeFailures and schemaFailures count consecutive failures pinned to
// one replay position each (guarded by mu): deterministic Avro-decode
// failures bounded by maxConsecutiveDecodeFailures, and schema-fetch
// failures bounded by the reconnect policy (reconnect_max_attempts).
// Position-scoped so an unrelated event's success never resets them.
decodeFailures positionFailures
schemaFailures positionFailures

// Atomic counters for health reporting.
eventsReceived atomic.Int64
Expand Down Expand Up @@ -121,22 +139,145 @@ func (s *Subscription) connectLocked(ctx context.Context) error {
// so waiting for the first Recv would block indefinitely on idle topics.
s.markReadyLocked()

go s.receiveLoop(ctx)
go s.receiveLoop(ctx, streamCtx)

return nil
}

// anchored reports whether a replay resume position exists: only then can a
// reconnect redeliver the current batch (CUSTOM replay is exclusive-after).
// On a fresh stream with no anchor, a reconnect falls back to the configured
// preset - with LATEST that silently drops the batch, so unanchored failures
// must never take the reconnect path.
func (s *Subscription) anchored() bool {
s.mu.Lock()
defer s.mu.Unlock()
return len(s.lastReplayID) > 0
}

// failTerminal fails the stream permanently: streamErr surfaces through the
// health tick and no reconnect is attempted.
func (s *Subscription) failTerminal(err error) {
s.lastError.Store(err)
s.lastErrorTime.Store(time.Now().UnixNano())
s.mu.Lock()
s.streamErr = err
s.state = StreamStateDisconnected
s.mu.Unlock()
s.client.log.Errorf("Pub/Sub stream failed permanently (topic=%s): %v", s.config.TopicName, err)
}
Comment on lines +163 to +172

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TerminalStreamError is never constructed, so the checkpoint shield this PR adds is dead code.

failTerminal stores the raw err into s.streamErr:

func (s *Subscription) failTerminal(err error) {
s.lastError.Store(err)
s.lastErrorTime.Store(time.Now().UnixNano())
s.mu.Lock()
s.streamErr = err
s.state = StreamStateDisconnected
s.mu.Unlock()
s.client.log.Errorf("Pub/Sub stream failed permanently (topic=%s): %v", s.config.TopicName, err)
}

TerminalStreamError is declared and documented here but has no construction site anywhere in the repo — the only other reference is the errors.As match in the consumer:

func (e *salesforceCDCInputExecutor) handleStreamErr(ctx context.Context, topic string, err error) bool {
var terminal *salesforcegrpc.TerminalStreamError
if errors.As(err, &terminal) {
return false
}
if grpcErr, ok := status.FromError(err); ok && grpcErr.Code() == codes.InvalidArgument {

So errors.As(err, &terminal) can never be true, and the exact failure this PR's final commit describes ("a terminal schema error carrying InvalidArgument would have DELETED the topic's durable checkpoint and resubscribed via the preset") is still live:

  1. deterministicSchemaFailure classifies codes.InvalidArgument as deterministic (L207), so a schema fetch rejected with InvalidArgument trips the bound and calls failTerminal(fmt.Errorf("fetching schema ...: %w", err)) (L407-L409).
  2. StreamErr() hands that unwrapped error to handleStreamErr (L875-L876).
  3. The terminal guard misses; status.FromError (grpc-go v1.82.1 resolves a GRPCStatus() through the error chain via errors.As) matches InvalidArgument, so delete(e.state.Topics, topic) runs and the durable checkpoint is destroyed — then the topic resubscribes via the preset, silently skipping everything since the checkpoint under the default LATEST.

Fix: have failTerminal wrap its argument in &TerminalStreamError{Err: err} before storing it in streamErr/lastError (or construct the wrapper at each failTerminal call site), so the guard actually engages.

Related gap: handleStreamErr has no unit test — internal/impl/salesforce has only integration tests for the CDC input, so nothing exercises the new terminal branch. A table-driven test over handleStreamErr (terminal-wrapped InvalidArgumentfalse and checkpoint intact; bare InvalidArgumenttrue) would have caught this and would lock the behavior in, per the test patterns referenced from CLAUDE.md.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6eb239f — and the finding was doubly right: the previous commit's failTerminal edit had silently failed to apply, which is exactly the class of gap the missing consumer-side test would have caught. failTerminal now genuinely wraps (the receive-loop terminal tests assert the marker type reaches StreamErr), and the suggested table-driven handleStreamErr test is added in the input package: bare/stream-wrapped InvalidArgument resets the checkpoint, terminal verdicts never touch it, other errors are fatal with the checkpoint intact. The same commit also fixes a panic this surfaced: lastError is an atomic.Value and one stream lifetime stores differently-typed errors (wrapped fmt, gRPC status, now TerminalStreamError), so all stores are boxed through one wrapper type, with a mixed-type regression test.


// failDecode routes an Avro decode failure: reconnect-and-redeliver while the
// position is under the consecutive-failure bound, terminal stream failure
// once it is exceeded. Schema-fetch failures do NOT come through here - they
// are transport-class errors governed by the reconnect policy, not evidence
// of an undecodable payload.
func (s *Subscription) failDecode(replayID []byte, failStream func(error), err error) {
if !s.recordDecodeFailure(replayID) {
failStream(err)
return
}
s.failTerminal(fmt.Errorf("decoding event at replay position %x: %d consecutive failures, treating as permanently undecodable: %w", replayID, maxConsecutiveDecodeFailures, err))
}

// positionFailures counts consecutive failures pinned to one replay
// position. A failure at a different position resets the count (the stream
// has moved on); a success clears it only when it lands at the tracked
// position - successes elsewhere must not reset it, since a redelivery cycle
// decodes the events around a permanently failing one successfully every
// time, which would keep the count oscillating below any bound forever.
// Callers must hold Subscription.mu.
type positionFailures struct {
count int
replayID []byte
}

func (p *positionFailures) record(replayID []byte) int {
if !bytes.Equal(replayID, p.replayID) {
p.replayID = append([]byte(nil), replayID...)
p.count = 0
}
p.count++
return p.count
}

func (p *positionFailures) clear(replayID []byte) {
if p.replayID != nil && bytes.Equal(replayID, p.replayID) {
p.count = 0
p.replayID = nil
}
}

// recordDecodeFailure counts an Avro-decode failure at the given replay
// position and reports whether the position has now failed
// maxConsecutiveDecodeFailures times in a row.
func (s *Subscription) recordDecodeFailure(replayID []byte) bool {
s.mu.Lock()
defer s.mu.Unlock()
return s.decodeFailures.record(replayID) >= maxConsecutiveDecodeFailures
}

// clearDecodeFailures resets the decode-failure count when the event at the
// tracked failing position decodes successfully.
func (s *Subscription) clearDecodeFailures(replayID []byte) {
s.mu.Lock()
s.decodeFailures.clear(replayID)
s.mu.Unlock()
}

// recordSchemaFailure counts a schema-fetch failure at the given replay
// position against the reconnect policy: with reconnect_max_attempts 0
// (unlimited) it never trips - the user asked for indefinite retries on
// transport-class failures - but a bounded policy must also bound this path.
// Each reconnect here succeeds (the transport is healthy, the schema is
// not), so reconnectWithBackoff's own per-call budget never accumulates: a
// deterministically unfetchable schema (uncompilable SchemaJson, permanent
// NotFound) would otherwise reconnect-redeliver forever without ever
// surfacing an error, the same livelock the decode bound exists for.
func (s *Subscription) recordSchemaFailure(replayID []byte) bool {
s.mu.Lock()
defer s.mu.Unlock()
budget := s.client.maxReconnect
return budget > 0 && s.schemaFailures.record(replayID) >= budget
}

// clearSchemaFailures resets the schema-failure count when a fetch succeeds
// at the tracked failing position.
func (s *Subscription) clearSchemaFailures(replayID []byte) {
s.mu.Lock()
s.schemaFailures.clear(replayID)
s.mu.Unlock()
}

// receiveLoop reads from the gRPC stream and pushes decoded events into the
// buffer. On stream errors it attempts reconnection with backoff instead of
// exiting.
func (s *Subscription) receiveLoop(ctx context.Context) {
// exiting. streamCtx is this stream's cancellation context: it unblocks a
// backpressured buffer send when the subscription closes or reconnects.
func (s *Subscription) receiveLoop(ctx, streamCtx context.Context) {
// Capture done at goroutine start. reconnectWithBackoff → connectLocked
// replaces s.done with a fresh channel for the new goroutine; closing the
// old reference here prevents a double-close panic when both goroutines
// eventually return.
done := s.done
defer close(done)

// failStream logs err and hands control to the reconnect path. Because
// s.lastReplayID has not been advanced past the current batch, the
// reconnected stream redelivers it: duplicates, never loss.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment states the opposite of the invariant the same PR introduces.

"Because s.lastReplayID has not been advanced past the current batch, the reconnected stream redelivers it" was true before this PR, when the anchor only moved at batch boundaries. The buffer-send branch added below now advances it per delivered event (L461-L472), so on reconnect redelivery resumes from the last buffered event, not from the start of the batch — which is precisely the behaviour the per-event anchor exists to provide (no re-emitted prefix), and it is what the batch-loop comment at L359-L374 already says.

Since the whole no-silent-loss argument in this PR rests on exactly this invariant, the stale wording is actively misleading for the next reader. Suggest rewording to state that the anchor is not advanced past the failing event, so redelivery resumes at it.

Ref: CONTRIBUTING §3.1.2 (self-documenting code).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b3c2a32 — the comment now states the per-event invariant: the anchor never advances past the failing event, so redelivery resumes exactly there (duplicates at most, no re-emitted prefix).

failStream := func(err error) {
s.client.log.Errorf("Pub/Sub stream error (topic=%s), reconnecting: %v", s.config.TopicName, err)
s.lastError.Store(err)
s.lastErrorTime.Store(time.Now().UnixNano())

if reconnErr := s.reconnectWithBackoff(ctx); reconnErr != nil {
s.mu.Lock()
s.streamErr = reconnErr
s.state = StreamStateDisconnected
s.mu.Unlock()
s.client.log.Errorf("Reconnection failed permanently (topic=%s): %v", s.config.TopicName, reconnErr)
}
}

for {
resp, err := s.stream.Recv()
if err != nil {
Expand All @@ -148,17 +289,7 @@ func (s *Subscription) receiveLoop(ctx context.Context) {
}
s.mu.Unlock()

s.client.log.Errorf("Pub/Sub stream error (topic=%s): %v", s.config.TopicName, err)
s.lastError.Store(err)
s.lastErrorTime.Store(time.Now().UnixNano())

if reconnErr := s.reconnectWithBackoff(ctx); reconnErr != nil {
s.mu.Lock()
s.streamErr = reconnErr
s.state = StreamStateDisconnected
s.mu.Unlock()
s.client.log.Errorf("Reconnection failed permanently (topic=%s): %v", s.config.TopicName, reconnErr)
}
failStream(err)
return
}

Expand All @@ -177,19 +308,79 @@ func (s *Subscription) receiveLoop(ctx context.Context) {
continue
}

// A schema fetch or decode failure must not skip the event: the
// batch's replay ID would advance past it and the event would be
// silently lost. When a replay anchor exists, reconnect instead —
// lastReplayID advances per delivered event, so redelivery resumes
// exactly at the failing event. Both failure classes are bounded
// per replay position: Avro decode failures against a fetched
// schema (deterministic) by maxConsecutiveDecodeFailures, and
// schema-fetch failures by the reconnect policy itself
// (reconnect_max_attempts; 0 = retry forever) — counted here
// because each individual reconnect succeeds, so the per-call
// reconnect budget alone can never trip. On a fresh stream with
// NO anchor a reconnect would fall back to the configured preset
// and could silently drop the batch (LATEST), so schema fetches
// retry inline under the same policy and decode failures fail the
// stream terminally instead.
schema, err := s.client.schemaCache.GetSchema(ctx, event.SchemaId)
if err != nil && !s.anchored() {
// A reconnect here would resume via the configured preset and
// could silently drop this batch (LATEST), so the fetch retries
// inline instead - governed by the same reconnect policy a
// reconnect would use (reconnect_min_delay/_max_delay and
// reconnect_max_attempts; 0 = retry indefinitely).
for attempt := 0; err != nil; attempt++ {
if s.client.maxReconnect > 0 && attempt >= s.client.maxReconnect {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

reconnect_max_attempts gains new user-visible semantics that its documentation does not mention.

This PR makes reconnect_max_attempts govern two things beyond reconnection: the inline schema-fetch retry budget on a fresh stream (here) and the per-position schema-failure budget on an anchored stream (L281-L290). Exhausting it now permanently fails the topic with a TerminalStreamError that the input deliberately refuses to recover from (handleStreamErr).

The field still documents only the old meaning (config.go#L179-L181):

Maximum number of gRPC reconnection attempts. 0 means unlimited.

A user who sets a small bound to cap reconnect churn has no way to know they are also choosing when a transient schema-endpoint failure becomes fatal for the topic. Please extend the field description (and re-run task docs) to cover the schema-fetch retry budget and the terminal outcome, and note that deterministic schema failures trip maxDeterministicPositionFailures regardless of this setting.

Refs: CONTRIBUTING §1.1.1 (documentation with configuration examples), §1.2.3 (known limitations and edge cases documented).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b3c2a32 — the field description (and regenerated docs) now state that reconnect_max_attempts also bounds transient schema-fetch retries with a permanent topic failure when exhausted, and that deterministic schema/decode failures give up after a fixed number of attempts regardless of this setting.

break
}
delay := grpcBackoffWithJitter(s.client.baseBackoff, s.client.maxBackoff, attempt)
s.client.log.Warnf("Schema fetch failed on a fresh stream with no replay anchor to redeliver from (topic=%s, schemaID=%s), retrying inline in %v (attempt %d): %v", s.config.TopicName, event.SchemaId, delay, attempt+1, err)
t := time.NewTimer(delay)
select {
case <-t.C:
case <-streamCtx.Done():
t.Stop()
return
case <-ctx.Done():
t.Stop()
return
}
schema, err = s.client.schemaCache.GetSchema(ctx, event.SchemaId)
}
Comment on lines +387 to +423

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The unanchored inline schema retry never refreshes credentials, so an expired token stalls the topic indefinitely.

This loop re-fetches the schema with the credentials already held by SchemaCache, and it neither stores the failure in lastError nor asks the client to refresh auth. refreshAuth is only ever invoked from reconnectWithBackoff (subscription.go#L546-L562), which this branch deliberately avoids, and nothing else in the connector refreshes the gRPC/schema-cache token (Client.UpdateAuth has refreshAuth as its only caller).

Failure scenario: no persisted replay ID for the topic (first run, or right after handleStreamErr cleared a stale checkpoint) and the first event arrives once the access token has expired — e.g. a low-traffic topic, or a long REST snapshot phase before streaming starts. GetSchema returns codes.Unauthenticated, which deterministicSchemaFailure classifies as transient (subscription.go#L210-L225), so under the shipped default reconnect_max_attempts: 0 the loop retries forever with the same dead token. streamErr/lastError are never set, so the input's health tick stays blind and the topic stalls silently until the process is restarted; under a bounded policy it instead dies with a misleading "retry budget is exhausted" error that a token refresh would have avoided. The anchored path recovers from exactly this case, so the two branches disagree.

Suggested fix: in this loop, record the error (s.lastError.Store(storedErr{err: err})) and call s.client.refreshAuth(ctx) when the failure carries codes.Unauthenticated before the next attempt, so the retry uses freshly-resolved credentials — per CONTRIBUTING.md §5.4.5 ("on connection loss, reconnect with freshly-resolved credentials") and §3.1.7.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 04c938f — the inline retry now mirrors everything reconnectWithBackoff does short of reconnecting: each failure is recorded in lastError (so Health() sees the stall), and an Unauthenticated failure triggers refreshAuth before the next attempt, propagating fresh credentials to the schema cache via UpdateAuth. Covered by a recovery test: GetSchema fails Unauthenticated until the refresh hook fires, then the event delivers inline with zero preset reconnects, with Health visibility asserted. Cross-checked the two branches for any remaining asymmetry — backoff/jitter, error recording, and auth refresh are the complete set reconnectWithBackoff contributes, and all three are now mirrored.

if err != nil {
s.eventsDecodeErrors.Add(1)
s.failTerminal(fmt.Errorf("fetching schema for the first event of a fresh stream (schemaID=%s): no replay anchor to redeliver from and reconnect_max_attempts (%d) exhausted: %w", event.SchemaId, s.client.maxReconnect, 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.

Terminal schema-fetch errors now put a wrapped gRPC status into streamErr, where the stale-replay handler can misread it and delete the durable checkpoint.

Both new terminal schema paths (this one and subscription.go:365) wrap the error from SchemaCache.GetSchema, which itself wraps the raw gRPC error with %w (avro.go:78). That error becomes s.streamErr, and the input's health tick feeds it straight into handleStreamErr:

func (e *salesforceCDCInputExecutor) handleStreamErr(ctx context.Context, topic string, err error) bool {
if grpcErr, ok := status.FromError(err); ok && grpcErr.Code() == codes.InvalidArgument {
e.logger.Warnf("topic %s replay_id rejected (%s); clearing and reconnecting via configured preset", topic, grpcErr.Message())
e.stateMu.Lock()
delete(e.state.Topics, topic)
saveErr := e.saveStateLocked(ctx)
e.stateMu.Unlock()
if saveErr != nil {
e.logger.Errorf("clear stale replay_id for %s: %v", topic, saveErr)
}
return true
}
return false
}

status.FromError unwraps through %w (it uses errors.As on the GRPCStatus() interface), so if the Pub/Sub GetSchema RPC ever fails with codes.InvalidArgument (e.g. a malformed/rejected schema ID), this branch fires for a schema error: the persisted replay ID for the topic is deleted and the topic resubscribes via the configured preset — with the default LATEST that silently skips every event between the checkpoint and now. Before this change only Recv()/reconnect errors reached streamErr, so the InvalidArgument heuristic could only mean "replay ID rejected"; that assumption no longer holds.

Suggested fix: make the terminal decode/schema failures distinguishable (a sentinel or typed error checked with errors.Is/errors.As) and restrict the stale-replay reset in handleStreamErr to stream-level errors, so a schema-stage failure can never clear the checkpoint. Silent skip-ahead is the exact loss class this PR is closing elsewhere (CONTRIBUTING.md §5.4.1/§5.4.2 — durable checkpoint and at-least-once delivery).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a66868a — failTerminal now wraps its verdicts in a TerminalStreamError marker (Unwrap preserved for transparency), and handleStreamErr checks for it before the InvalidArgument stale-replay heuristic: a terminal schema/decode verdict can never clear the persisted replay ID. Terminal errors propagate as fatal topic errors with the checkpoint intact, so the stale-replay reset is once again reachable only by genuine stream-level errors.

return
}
Comment on lines +381 to +428

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The unanchored schema-fetch retry budget reuses an unrelated constant and bypasses the user's reconnect policy.

Two problems with this loop:

  1. Wrong constant. maxConsecutiveDecodeFailures is used as the schema-fetch retry count, but failDecode's own doc comment states the opposite intent:

    Schema-fetch failures do NOT come through here - they are transport-class errors governed by the reconnect policy, not evidence of an undecodable payload.

    That split is the whole point of commit 844c3af. Here the decode bound leaks back into the schema-fetch path as a magic bound. The constant's own doc at subscription.go#L34-L40 is also stale for the same reason — it still says it bounds "an event whose schema fetch or Avro decode keeps failing" and talks about "redelivering the batch prefix forever", neither of which is true after the anchor now advances per event.

  2. Fixed ~10s budget, ignoring reconnect_max_attempts/backoff. Attempts sleep 1s+2s+3s+4s, then failTerminal kills the topic permanently. Commit 844c3af's message identifies exactly this failure mode as the bug it fixes:

    a ~1 minute schema-endpoint outage terminally killed the topic with a misleading 'permanently undecodable' error even under reconnect_max_attempts: 0 (unlimited)

    The fix only covers the anchored path. On a fresh stream — which is every process start with replay_preset: latest, and every topic that has not yet delivered an event — a ~15s Salesforce schema-endpoint blip still fails the input terminally, with reconnect_max_attempts: 0 silently ignored. Reconnecting is genuinely unsafe here (LATEST would drop the batch), but the inline retry should be governed by the same configured backoff/attempt policy rather than a hardcoded ~10s, per CONTRIBUTING.md §3.2.3 (unfamiliar/confusing UX) — a documented reconnect setting that does not apply on the first batch is hard to reason about.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 810750f on both points. The retry budget is now reconnect_max_attempts itself — 0 retries indefinitely, matching what the user asked for on transport failures — with the configured jittered backoff between attempts, so a schema-endpoint blip on a fresh stream no longer terminally kills the topic under an unlimited policy. When a bounded policy is exhausted the terminal error names the schema stage and the attempt budget rather than claiming the payload is undecodable. The stale maxConsecutiveDecodeFailures doc is rewritten for what it bounds post-split (deterministic per-position decode failures only). Reconnecting stays deliberately off this path: with no anchor, the preset could silently drop the batch.

}
if err != nil {
s.client.log.Errorf("get schema for event (schemaID=%s): %v", event.SchemaId, err)
s.eventsDecodeErrors.Add(1)
continue
// Bounded by the reconnect policy: each reconnect below
// succeeds (the transport is healthy), so the per-call
// reconnect budget never accumulates - the per-position count
// is what makes reconnect_max_attempts meaningful for a
// deterministically unfetchable schema. 0 = retry forever.
if s.recordSchemaFailure(consumerEvent.ReplayId) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new schema-fetch bound never engages under the shipped default, so the livelock this commit targets persists out of the box.

recordSchemaFailure returns budget > 0 && ... (subscription.go:241), and reconnect_max_attempts defaults to 0:

service.NewIntField(sfFieldGRPCReconnectMaxAttempts).
Description("Maximum number of gRPC reconnection attempts. 0 means unlimited.").
Default(0),

So with the default config a deterministically unfetchable schema (uncompilable SchemaJson, permanent NotFound) takes failStreamreconnectWithBackoff on every redelivery, forever: streamErr is never set, the health tick in subscribeAndPump stays blind, and the pipeline spins re-subscribing indefinitely — exactly the failure mode the commit message says this bound exists to break. The commit message describes 0 as "the user's explicit opt-in to indefinite retries", but nobody has to opt in — it is the default value, so the guard is inert for every user who does not set the field.

Suggested fix: give this path its own bound analogous to maxConsecutiveDecodeFailures (used only when reconnect_max_attempts is unlimited), or treat 0 as "unbounded reconnects, still bounded per replay position". Either way the default configuration should surface a terminal error rather than reconnect-redeliver forever, per CONTRIBUTING.md §3.1.4 (complete and correct, no known missing core functionality) and §1.2.2 (unexpected behavior emits warning/error logs and is diagnosable).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a66868a. Schema failures are now classified: deterministic ones — NotFound, InvalidArgument, PermissionDenied, Unimplemented, and compile failures (no gRPC status) — cannot heal by retrying, so they trip at maxConsecutiveDecodeFailures regardless of the reconnect policy; the shipped default no longer livelocks. Transport-class failures stay governed by the policy (0 = indefinite retries), preserving the earlier outage-must-not-kill-the-topic fix. Both the anchored and unanchored paths apply the same classification, and the counting test now covers the deterministic-under-default-policy case.

s.failTerminal(fmt.Errorf("fetching schema (schemaID=%s) failed %d consecutive times at replay position %x (reconnect_max_attempts exhausted): %w", event.SchemaId, s.client.maxReconnect, consumerEvent.ReplayId, err))
return
}
failStream(fmt.Errorf("get schema for event (schemaID=%s): %w", event.SchemaId, 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.

Turning a schema/decode failure into a reconnect does not produce the "stalls the topic loudly" behaviour the comment above describes — for a permanently undecodable payload it produces an unbounded redelivery loop.

s.lastReplayID is only advanced after the whole batch has been pushed (subscription.go#L242-L247), so failStream reconnects from a replay ID that points before the current batch. Salesforce then redelivers the batch: events preceding the bad one are decoded and pushed into eventBuffer again (and emitted downstream again), then the same event fails again, and the cycle repeats.

maxReconnect does not bound this: reconnectWithBackoff returns nil on a successful reconnect (subscription.go#L288-L332), so attempt restarts at 0 on every iteration and the "max reconnect attempts exceeded" guard never trips. streamErr is never set either, so subscribeAndPump's health tick (input_salesforce_cdc.go#L874-L880) never surfaces the failure — the input keeps running while re-emitting the same prefix of events forever. That is hard-to-diagnose error handling (CONTRIBUTING.md §3.2.2) rather than a loud stall.

Suggested fix: track consecutive decode failures at the same replay position and, once a small bound is exceeded, set s.streamErr/s.state = StreamStateDisconnected so the input fails visibly instead of looping — or give undecodable payloads an explicit terminal path. Either way the comment should describe the actual behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9cce455. Consecutive schema/decode failures are now counted per replay position (recordDecodeFailure, cleared on any successful decode or when the failing position changes); past maxConsecutiveDecodeFailures=5 the stream fails terminally — streamErr set and state Disconnected, so subscribeAndPump's health tick surfaces it and the pipeline stops loudly instead of redelivering the batch prefix forever. Transient schema-fetch errors still heal by bounded redelivery. The comment now describes the actual behaviour.

return
}
Comment on lines 430 to 443

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Anchored schema-fetch failures are not actually bounded by the reconnect policy — a permanently unfetchable schema loops forever without ever surfacing an error.

The comment above claims schema-fetch failures "stay governed by the reconnect policy" (subscription.go#L273-L275), but reconnect_max_attempts cannot bound this path: reconnectWithBackoff restarts attempt at 0 on every call and returns nil as soon as connectLocked succeeds (subscription.go#L417-L461). Since the reconnect itself always succeeds here, the budget is never approached.

Failure scenario — a schema whose SchemaJson fails to compile (avro.Parse error in avro.go#L81-L84), or a schemaID that returns a permanent NotFound/PermissionDenied:

  1. GetSchema fails deterministically; an anchor exists, so failStream is called and the stream reconnects from lastReplayID.
  2. Salesforce redelivers starting at the same event; GetSchema fails again identically.
  3. streamErr is never set (the reconnect succeeded), so the health tick in input_salesforce_cdc.go#L874-L880 never observes anything, and handleStreamErr is never reached.

Result: the topic reconnects roughly once per backoff interval indefinitely, never advances its replay position, and never fails — the pipeline hangs silently apart from a repeated Errorf. This is the same unbounded-redelivery livelock that maxConsecutiveDecodeFailures was introduced for in 9cce455, reintroduced for the schema-fetch class when 844c3af split schema fetches out of that bound. Note the unanchored path already terminates correctly (lines 288–310), so the two branches disagree on whether a permanently failing schema fetch is survivable.

Suggested fix: give this path its own bound rather than relying on the per-call reconnect budget — e.g. track consecutive schema-fetch failures at the same replay position (mirroring recordDecodeFailure/clearDecodeFailures), or carry a reconnect budget across failStream calls that are triggered by schema fetches, so an uncompilable/inaccessible schema eventually reaches failTerminal with an error naming the schema stage — the same terminal outcome the unanchored branch already produces.

Rules referenced: CONTRIBUTING.md §3.1.4 ("implementation is complete and correct, with no known bugs"), §3.2.2 ("poor error handling or difficult-to-diagnose bugs"), and §1.2.2 (unexpected behavior should surface, not silently stall).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in f9f6346. Consecutive schema-fetch failures are now counted per replay position (the decode counter's machinery, extracted as a shared positionFailures type) with reconnect_max_attempts as the budget — counted across failStream cycles precisely because each individual reconnect succeeds, so the per-call budget alone can never trip. 0 keeps the documented opt-in to indefinite retries; a bounded policy now genuinely bounds this path and reaches failTerminal with an error naming the schema stage and the exhausted budget, matching the unanchored branch. Success at the tracked position clears the count; unrelated successes never do. Covered by counting-semantics and exhausted-budget receive-loop tests, and the stale 'stay governed by the reconnect policy' comment now describes the actual mechanism.

s.clearSchemaFailures(consumerEvent.ReplayId)

decoded, err := DecodeAvroPayload(schema, event.Payload)
if err != nil {
s.client.log.Errorf("decode Avro payload (schemaID=%s): %v", event.SchemaId, err)
s.eventsDecodeErrors.Add(1)
continue
if !s.anchored() {
s.failTerminal(fmt.Errorf("decoding the first event of a fresh stream (schemaID=%s, replay position %x): payload is undecodable and no replay anchor exists to redeliver from: %w", event.SchemaId, consumerEvent.ReplayId, err))
return
}
s.failDecode(consumerEvent.ReplayId, failStream, fmt.Errorf("decode Avro payload (schemaID=%s): %w", event.SchemaId, err))
return
}
s.clearDecodeFailures(consumerEvent.ReplayId)

pubsubEvent := &PubSubEvent{
ReplayID: consumerEvent.ReplayId,
Expand All @@ -209,14 +400,27 @@ func (s *Subscription) receiveLoop(ctx context.Context) {
}
}

// A full buffer applies backpressure instead of dropping: while
// this send blocks, no flow-control FetchRequest is issued, so
// Salesforce stops sending and the replay cursor cannot advance
// past an undelivered event. The stream context unblocks the send
// on close or reconnect.
select {
case s.eventBuffer <- pubsubEvent:
s.eventsReceived.Add(1)
s.lastEventTime.Store(time.Now().UnixNano())
// Advance the replay anchor past this event: CUSTOM replay is
// exclusive-after, so a reconnect now redelivers from exactly
// the next (possibly failing) event - no lost first batch, no
// re-emitted prefix.
s.mu.Lock()
s.lastReplayID = pubsubEvent.ReplayID
s.mu.Unlock()
s.client.log.Debugf("Pub/Sub event received (topic=%s, schemaID=%s, replayID=%x)", pubsubEvent.TopicName, pubsubEvent.SchemaID, pubsubEvent.ReplayID)
default:
s.eventsDropped.Add(1)
s.client.log.Warnf("Pub/Sub event buffer full (topic=%s), dropping event", s.config.TopicName)
case <-streamCtx.Done():
return
case <-ctx.Done():
return
}
}

Expand Down
Loading