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)