From 0c60982ff578a0bb42f97da9cb3dd7d8a45174e4 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 28 Jul 2026 15:06:16 -0700 Subject: [PATCH] fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? `buildsignal` wrapped a failed reschedule publish in `errs.NewRetryableError`. `platform/errs` rule 4 says not to do that: a failed queue publish must not be made retryable just because replaying it is convenient, because that turns a permanent enqueue failure into an infinite retry instead of dead-lettering. It was also inconsistent with the sibling path — the publish to `record`, one branch above, already returns raw. ### What? Returns the raw error, leaving the classifier's verdict to stand, and drops the now-unused `errs` dependency. ### Merge ordering **This should merge only after the build and buildsignal stages have DLQ consumers.** Today only `stovepipe-process-dlq` is registered, and `MoveToDLQ` routes by topic-suffix convention, so anything dead-lettered from `buildsignal` lands on `buildsignal_dlq` and is never drained — leaving the request non-terminal with its build slot held, which is the wedge the reconciler exists to prevent. This change does not create that hole; every other non-retryable failure in the stage already falls into it. But it does add one more path in, so the ordering is worth respecting rather than discovering later. If the DLQ work slips, the alternative is to land this anyway and accept parity with the existing failure modes. ## Test Plan ✅ `bazel test //stovepipe/...` — the reschedule-publish-failure case now asserts a non-retryable classification. ✅ `bazel test //test/e2e/stovepipe/...` — the poll loop is unaffected on the success path. # Conflicts: # stovepipe/controller/buildsignal/buildsignal.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 41992811 # Last command done (1 command done): # pick 356f028d fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable # No commands remaining. # You are currently rebasing branch 'preetam/stovepipe-publish-classification' on '41992811'. # # Changes to be committed: # modified: doc/rfc/stovepipe/steps/buildsignal.md # modified: stovepipe/controller/buildsignal/BUILD.bazel # modified: stovepipe/controller/buildsignal/buildsignal.go # modified: stovepipe/controller/buildsignal/buildsignal_test.go # --- doc/rfc/stovepipe/steps/buildsignal.md | 4 ++-- stovepipe/controller/buildsignal/BUILD.bazel | 1 - stovepipe/controller/buildsignal/buildsignal.go | 3 +-- stovepipe/controller/buildsignal/buildsignal_test.go | 4 ++-- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/doc/rfc/stovepipe/steps/buildsignal.md b/doc/rfc/stovepipe/steps/buildsignal.md index 05fa600f..67fb1c60 100644 --- a/doc/rfc/stovepipe/steps/buildsignal.md +++ b/doc/rfc/stovepipe/steps/buildsignal.md @@ -81,6 +81,7 @@ For a delivery carrying build id `B`: and the delivery being processed is still un-acked, so its row is present: reusing B as the message id makes every re-poll collide with the message that scheduled it and be silently discarded, ending the poll loop after one tick. + - publish failure -> return raw (non-retryable), same posture as step 7. - ack. ``` @@ -126,11 +127,10 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m |---|---|---| | `Status` call | raw error; classifier decides | Deliberately left open rather than fixed either way — runner timeout/connection is transient, "runner not deployed for this queue" is not, and only a backend classifier can tell them apart. | | `Update` CAS conflict (`ErrVersionMismatch`) | declaration-level retryable | A concurrent (redelivered) writer moved the row; reload and re-check converges. | -| `PublishAfter` re-poll | retryable | The poll heartbeat; it runs only after status/persist/record all succeeded, so a transient enqueue blip is worth replaying to `MaxAttempts` before dead-lettering. | `Build`/`Request` not found (`storage.ErrNotFound`) are **not** in this table: storage is required to be read-after-write consistent (see [storage README](stovepipe/extension/storage/README.md)), so a miss here is already the correct default (non-retryable, straight to DLQ) rather than a departure worth overriding. -Everything else — factory lookup, an `Update` store error other than a CAS conflict, and the publish to `record` — is returned raw with no override, because the default is already correct: a queue with no registered runner is a config error, and storage/queue failures dead-letter and let DLQ reconciliation recover. +Everything else — factory lookup, an `Update` store error other than a CAS conflict, and both publishes — is returned raw with no override, because the default is already correct: a queue with no registered runner is a config error, and storage/queue failures dead-letter and let DLQ reconciliation recover. The `PublishAfter` re-poll is included in that: per `platform/errs` rule 4 a failed queue publish is not wrapped retryable just because replaying it is convenient, which would turn a permanent enqueue failure into an infinite retry instead of dead-lettering. ## Idempotency diff --git a/stovepipe/controller/buildsignal/BUILD.bazel b/stovepipe/controller/buildsignal/BUILD.bazel index 9b33d891..767d30da 100644 --- a/stovepipe/controller/buildsignal/BUILD.bazel +++ b/stovepipe/controller/buildsignal/BUILD.bazel @@ -8,7 +8,6 @@ go_library( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", - "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/core/loader:go_default_library", "//stovepipe/core/messagequeue:go_default_library", diff --git a/stovepipe/controller/buildsignal/buildsignal.go b/stovepipe/controller/buildsignal/buildsignal.go index 42b6b819..4fbff6ab 100644 --- a/stovepipe/controller/buildsignal/buildsignal.go +++ b/stovepipe/controller/buildsignal/buildsignal.go @@ -30,7 +30,6 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" - "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/core/loader" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" @@ -171,7 +170,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er delayMs := pollDelay(effective) if err := c.publishBuildSignal(ctx, build.ID, msg.ID, delayMs); err != nil { - return errs.NewRetryableError(fmt.Errorf("failed to reschedule poll for build %s: %w", build.ID, err)) + return fmt.Errorf("failed to reschedule poll for build %s: %w", build.ID, err) } c.logger.Debugw("rescheduled build status poll", "build_id", build.ID, diff --git a/stovepipe/controller/buildsignal/buildsignal_test.go b/stovepipe/controller/buildsignal/buildsignal_test.go index f139e861..23e25875 100644 --- a/stovepipe/controller/buildsignal/buildsignal_test.go +++ b/stovepipe/controller/buildsignal/buildsignal_test.go @@ -367,9 +367,9 @@ func TestProcess(t *testing.T) { }, }, { - name: "reschedule publish failure is retryable", + name: "reschedule publish failure is not retryable", wantErr: true, - wantRetry: true, + wantRetry: false, setup: func(m buildsignalMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusAccepted, 1), nil) m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateProcessing), nil)