From 8394d14ece605377c8cb03eb6dcb5b4b69b0de1d Mon Sep 17 00:00:00 2001 From: "prath.shenoy" Date: Thu, 22 Oct 2026 04:04:55 +0000 Subject: [PATCH 1/2] feat(stovepipe): Record incomplete build analysis --- doc/rfc/stovepipe/request-log.md | 7 +- .../controller/buildanalysis/BUILD.bazel | 15 +- stovepipe/controller/buildanalysis/README.md | 10 +- stovepipe/controller/buildanalysis/dlq.go | 185 +++++++++++++ .../controller/buildanalysis/dlq_test.go | 248 ++++++++++++++++++ stovepipe/entity/request_log.go | 4 +- stovepipe/entity/request_log_test.go | 10 + 7 files changed, 473 insertions(+), 6 deletions(-) create mode 100644 stovepipe/controller/buildanalysis/dlq.go create mode 100644 stovepipe/controller/buildanalysis/dlq_test.go diff --git a/doc/rfc/stovepipe/request-log.md b/doc/rfc/stovepipe/request-log.md index a32aac6de..83d7c04d5 100644 --- a/doc/rfc/stovepipe/request-log.md +++ b/doc/rfc/stovepipe/request-log.md @@ -2,13 +2,14 @@ ## Summary -Stovepipe retains an append-only request log for each validation request. Its internal `RequestLog` is the counterpart of SubmitQueue's `RequestLog`: both retain request status changes and explanatory lifecycle events, while Stovepipe calls its materializer directly instead of sending records through a cross-service log topic. The public API presents these records as request history. The log records every durable `Request.State` transition plus asynchronous milestones needed to explain those transitions, the public verdict, and abandoned terminal-stage work: +Stovepipe retains an append-only request log for each validation request. Its internal `RequestLog` is the counterpart of SubmitQueue's `RequestLog`: both retain request status changes and explanatory lifecycle events, while Stovepipe calls its materializer directly instead of sending records through a cross-service log topic. The public API presents these records as request history. The log records every durable `Request.State` transition plus asynchronous milestones needed to explain those transitions, the public verdict, and abandoned stage work: - `build_triggered`; - `build_finished`; - `validation_fact_recorded`; - `project_facts_recorded`; -- `record_abandoned`. +- `record_abandoned`; +- `build_analysis_abandoned`. The model deliberately follows SubmitQueue's distinction between statuses describing where a request is and events describing important activity that does not move it. It remains a bounded request-lifecycle log rather than a generic event bus or an audit of every correlated operation. @@ -126,6 +127,7 @@ Immutable Request context such as URI, build strategy, base URI, and terminal bu | `validation_fact_recorded` | The immutable whole-repository fact became durable. | Degree metadata and fact creation time | | `project_facts_recorded` | All project validation facts returned for a request became durable. | Project-fact count metadata and completion time | | `record_abandoned` | Record-stage work stopped after exhausting primary retries. | Event retention time | +| `build_analysis_abandoned` | Build analysis could not complete; issue observations may be incomplete. This is not a validation verdict. | Build ID metadata and event retention time | Build running and unchanged polls are not retained. Trigger and terminal result explain the request outcome without turning polling into an unbounded log. Individual project facts remain outside the vocabulary; one batch-completion event summarizes their durable recording. @@ -214,6 +216,7 @@ Request creation, Build changes, and fact creation use the same source-write, lo | Buildsignal | Persist terminal Build and retain `build_finished`; CAS the Request outcome and retain its terminal state. | Existing terminal Build and Request outcome each ensure their own entry before record publication. | | Record | Create or verify the whole-repository fact, then retain `validation_fact_recorded`; after recording project facts, retain one `project_facts_recorded` event. | An identical whole-repository fact and project-fact batch ensure their events before bookmark or promotion work. | | Record DLQ | Retain `record_abandoned`, then acknowledge the remaining record work. | The stable event ID makes history retention idempotent without replaying facts, bookmarks, promotion, or hooks. | +| Build analysis DLQ | Retain `build_analysis_abandoned` without rerunning detection or changing validation state. | One stable event per build deduplicates recovery deliveries; only state reads and history retention retry. | | Reconciler | CAS an unrecoverable non-terminal Request to failed, then retain failed. | An existing terminal Request is repaired from its persisted outcome without relabeling it. | Build running and unchanged polls create no entry. A failed runner trigger that creates no Build creates no event. Cancelled and superseded requests create no validation fact. diff --git a/stovepipe/controller/buildanalysis/BUILD.bazel b/stovepipe/controller/buildanalysis/BUILD.bazel index d83fe9cc7..37afa9972 100644 --- a/stovepipe/controller/buildanalysis/BUILD.bazel +++ b/stovepipe/controller/buildanalysis/BUILD.bazel @@ -2,7 +2,10 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["buildanalysis.go"], + srcs = [ + "buildanalysis.go", + "dlq.go", + ], importpath = "github.com/uber/submitqueue/stovepipe/controller/buildanalysis", visibility = ["//visibility:public"], deps = [ @@ -11,6 +14,7 @@ go_library( "//platform/metrics:go_default_library", "//stovepipe/core/loader:go_default_library", "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/core/requestlog:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/buildissue:go_default_library", "//stovepipe/extension/storage:go_default_library", @@ -21,14 +25,21 @@ go_library( go_test( name = "go_default_test", - srcs = ["buildanalysis_test.go"], + srcs = [ + "buildanalysis_test.go", + "dlq_test.go", + ], embed = [":go_default_library"], deps = [ + "//platform/base/failure:go_default_library", "//platform/base/messagequeue:go_default_library", + "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/core/requestlog:go_default_library", + "//stovepipe/core/requestlog/mock:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/buildissue:go_default_library", "//stovepipe/extension/buildissue/mock:go_default_library", diff --git a/stovepipe/controller/buildanalysis/README.md b/stovepipe/controller/buildanalysis/README.md index aac7cde7b..95c78104d 100644 --- a/stovepipe/controller/buildanalysis/README.md +++ b/stovepipe/controller/buildanalysis/README.md @@ -18,4 +18,12 @@ Status polling and updates remain the responsibility of buildsignal. Analysis ne Detection, persistence, configuration, state-loading, and message-validation errors are returned to the consumer for its normal retry or DLQ policy, whether the Build is running or terminal. Analysis errors do not use the two-minute polling hold. The consumer classifies errors and applies its configured retry limits and backoff; a final analysis can still fail and dead-letter. -The message and controller are defined without runtime producers or consumer registration. Host integration and detector implementations are separate work. +## DLQ recovery + +The recovery controller records a `build_analysis_abandoned` event in the Request's history, with the build ID, to indicate that analysis could not complete and issue observations may be incomplete. It does not rerun detection, delete earlier observations, change Request or Build state, or release a build slot. Original failure details are logged rather than included in the immutable history event. + +History identity is stable per build, so repeated recovery deliveries and separate analysis notices for the same build retain one event. Different builds retain different events, even for the same Request. A later successful analysis does not erase the historical occurrence. + +State-read and history-write errors are returned for retry. Hosts must run recovery with the DLQ consumer's always-retryable processor and unlimited-retry subscription; only recovery work is retried, not analysis. Unusable payloads, invalid or unresolvable queues, missing Build or Request records, and queue mismatches are logged and acknowledged rather than retried indefinitely. + +The controllers are defined without runtime producers or consumer registration. Host integration and detector implementations are separate work. diff --git a/stovepipe/controller/buildanalysis/dlq.go b/stovepipe/controller/buildanalysis/dlq.go new file mode 100644 index 000000000..36dad5636 --- /dev/null +++ b/stovepipe/controller/buildanalysis/dlq.go @@ -0,0 +1,185 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package buildanalysis + +import ( + "context" + "errors" + "fmt" + + "github.com/uber-go/tally" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/metrics" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/core/requestlog" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/storage" + "go.uber.org/zap" +) + +const _dlqOpName = "buildanalysis_dlq" + +// DLQController records incomplete analysis without rerunning detection or +// changing the validation outcome. +type DLQController struct { + logger *zap.SugaredLogger + metricsScope tally.Scope + stores storage.Factory + materializer requestlog.Materializer + topicKey consumer.TopicKey + consumerGroup string +} + +// NewDLQController creates a controller for incomplete build analysis. +func NewDLQController( + logger *zap.SugaredLogger, + scope tally.Scope, + stores storage.Factory, + materializer requestlog.Materializer, + topicKey consumer.TopicKey, + consumerGroup string, +) *DLQController { + name := string(topicKey) + "_controller" + return &DLQController{ + logger: logger.Named(name), + metricsScope: scope.SubScope(name), + stores: stores, + materializer: materializer, + topicKey: topicKey, + consumerGroup: consumerGroup, + } +} + +// Process retains incomplete-analysis history and acknowledges unusable messages. +// State-read and history-write failures are returned for DLQ recovery retry. +func (c *DLQController) Process(ctx context.Context, delivery consumer.Delivery) error { + msg := delivery.Message() + analysis := &stovepipemq.BuildAnalysis{} + if err := stovepipemq.Unmarshal(msg.Payload, analysis); err != nil { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "deserialize_errors", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding malformed analysis dlq message", + "message_id", msg.ID, + "error", err, + ) + return nil + } + + if err := entityqueue.ValidatePayloadQueue(msg, analysis.GetQueueName()); err != nil { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "queue_identity_errors", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message with invalid queue identity", + "message_id", msg.ID, + "build_id", analysis.GetId(), + "error", err, + ) + return nil + } + + if analysis.GetId() == "" { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "empty_id_errors", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message with empty build id", + "message_id", msg.ID, + "queue", analysis.GetQueueName(), + ) + return nil + } + + store, err := c.stores.For(storage.Config{QueueName: analysis.GetQueueName()}) + if err != nil { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "storage_resolve_errors", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message for unresolvable queue", + "message_id", msg.ID, + "build_id", analysis.GetId(), + "queue", analysis.GetQueueName(), + "error", err, + ) + return nil + } + + build, err := store.GetBuildStore().Get(ctx, analysis.GetId()) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "build_not_found", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message for missing build", + "message_id", msg.ID, "build_id", analysis.GetId(), "queue", analysis.GetQueueName()) + return nil + } + + return fmt.Errorf("failed to load build %s for analysis dlq: %w", analysis.GetId(), err) + } + + if build.RequestID == "" { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "empty_request_id", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message for build without a request", + "message_id", msg.ID, "build_id", analysis.GetId()) + return nil + } + + request, err := store.GetRequestStore().Get(ctx, build.RequestID) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "request_not_found", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message for missing request", + "message_id", msg.ID, "build_id", analysis.GetId(), "request_id", build.RequestID) + return nil + } + + return fmt.Errorf("failed to load request %s for analysis dlq: %w", build.RequestID, err) + } + + if request.Queue != analysis.GetQueueName() { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "queue_identity_errors", 1, metrics.TagsFromContext(ctx)...) + c.logger.Errorw("discarding analysis dlq message for request in another queue", + "message_id", msg.ID, "request_id", request.ID, "queue", request.Queue, + "message_queue", analysis.GetQueueName()) + return nil + } + + log := requestlog.NewRequestEventLog(request, entity.RequestEventBuildAnalysisAbandoned, analysis.GetId(), + map[string]string{requestlog.MetadataKeyBuildID: analysis.GetId()}) + if err := c.materializer.PersistLog(ctx, store, log); err != nil { + metrics.NamedCounter(c.metricsScope, _dlqOpName, "history_errors", 1, metrics.TagsFromContext(ctx)...) + return fmt.Errorf("failed to retain incomplete build analysis for request %s: %w", request.ID, err) + } + + originalFailure, hasFailure := delivery.Failure() + metrics.NamedCounter(c.metricsScope, _dlqOpName, "analysis_abandoned", 1, metrics.TagsFromContext(ctx)...) + fields := []any{ + "message_id", msg.ID, + "request_id", request.ID, + "build_id", analysis.GetId(), + "queue", request.Queue, + "request_state", request.State, + "history_event", entity.RequestEventBuildAnalysisAbandoned, + "attempt", delivery.Attempt(), + } + if hasFailure { + fields = append(fields, + "failure", originalFailure.Message, + "failure_subjects", originalFailure.Subjects, + "failure_detail", originalFailure.Detail, + ) + } + c.logger.Errorw("build analysis could not complete; issue observations may be incomplete", fields...) + return nil +} + +func (c *DLQController) Name() string { return string(c.topicKey) } + +func (c *DLQController) TopicKey() consumer.TopicKey { return c.topicKey } + +func (c *DLQController) ConsumerGroup() string { return c.consumerGroup } + +var _ consumer.Controller = (*DLQController)(nil) diff --git a/stovepipe/controller/buildanalysis/dlq_test.go b/stovepipe/controller/buildanalysis/dlq_test.go new file mode 100644 index 000000000..6280d71b4 --- /dev/null +++ b/stovepipe/controller/buildanalysis/dlq_test.go @@ -0,0 +1,248 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package buildanalysis + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/base/failure" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" + consumermock "github.com/uber/submitqueue/platform/consumer/mock" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/core/requestlog" + requestlogmock "github.com/uber/submitqueue/stovepipe/core/requestlog/mock" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/storage" + storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock" + "go.uber.org/mock/gomock" + "go.uber.org/zap" +) + +type dlqMocks struct { + ctrl *gomock.Controller + factory *storagemock.MockFactory + store *storagemock.MockStorage + builds *storagemock.MockBuildStore + requests *storagemock.MockRequestStore + materializer *requestlogmock.MockMaterializer +} + +func newDLQTestController(t *testing.T) (*DLQController, dlqMocks) { + t.Helper() + + ctrl := gomock.NewController(t) + m := dlqMocks{ + ctrl: ctrl, + factory: storagemock.NewMockFactory(ctrl), + store: storagemock.NewMockStorage(ctrl), + builds: storagemock.NewMockBuildStore(ctrl), + requests: storagemock.NewMockRequestStore(ctrl), + materializer: requestlogmock.NewMockMaterializer(ctrl), + } + m.store.EXPECT().GetBuildStore().Return(m.builds).AnyTimes() + m.store.EXPECT().GetRequestStore().Return(m.requests).AnyTimes() + + c := NewDLQController(zap.NewNop().Sugar(), tally.NoopScope, m.factory, m.materializer, + consumer.TopicKey("buildanalysis_dlq"), "stovepipe-buildanalysis-dlq") + + return c, m +} + +func (m dlqMocks) expectLoad(request entity.Request) { + m.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) + m.builds.EXPECT().Get(gomock.Any(), testBuildID). + Return(entity.Build{ID: testBuildID, RequestID: request.ID, Status: entity.BuildStatusRunning}, nil) + m.requests.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) +} + +func (m dlqMocks) delivery(msg entityqueue.Message, hasFailure bool) *consumermock.MockDelivery { + d := consumermock.NewMockDelivery(m.ctrl) + d.EXPECT().Message().Return(msg) + d.EXPECT().Failure().Return(failure.Failure{Message: "detector unavailable"}, hasFailure).AnyTimes() + d.EXPECT().Attempt().Return(1).AnyTimes() + + return d +} + +func TestDLQRetainsHistoryWithoutChangingValidation(t *testing.T) { + for _, state := range []entity.RequestState{ + entity.RequestStateAccepted, entity.RequestStateProcessing, entity.RequestStateSucceeded, + entity.RequestStateFailed, entity.RequestStateCancelled, entity.RequestStateSuperseded, + } { + t.Run(string(state), func(t *testing.T) { + c, m := newDLQTestController(t) + request := entity.Request{ID: "request/1", Queue: testQueue, State: state, Version: 3} + m.expectLoad(request) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( + func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { + assert.Equal(t, "event/build_analysis_abandoned/"+testBuildID, log.ID) + assert.Equal(t, request.ID, log.RequestID) + assert.Equal(t, testQueue, log.Queue) + assert.Equal(t, entity.RequestEventBuildAnalysisAbandoned, log.Event) + assert.Equal(t, map[string]string{requestlog.MetadataKeyBuildID: testBuildID}, log.Metadata) + assert.Empty(t, log.State) + assert.Zero(t, log.RequestVersion) + assert.Empty(t, log.OutcomeReason) + return nil + }, + ) + + require.NoError(t, c.Process(context.Background(), m.delivery(analysisMessage(t), state.IsTerminal()))) + }) + } +} + +func TestDLQReturnsStateAndHistoryErrorsForRetry(t *testing.T) { + for _, step := range []string{"load build", "load request", "retain history"} { + t.Run(step, func(t *testing.T) { + c, m := newDLQTestController(t) + m.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) + + if step == "load build" { + m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{}, assert.AnError) + } else { + m.builds.EXPECT().Get(gomock.Any(), testBuildID). + Return(entity.Build{ID: testBuildID, RequestID: "request/1", Status: entity.BuildStatusFailed}, nil) + + if step == "load request" { + m.requests.EXPECT().Get(gomock.Any(), "request/1").Return(entity.Request{}, assert.AnError) + } else { + m.requests.EXPECT().Get(gomock.Any(), "request/1"). + Return(entity.Request{ID: "request/1", Queue: testQueue}, nil) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(assert.AnError) + } + } + + err := c.Process(context.Background(), m.delivery(analysisMessage(t), false)) + require.ErrorIs(t, err, assert.AnError) + }) + } +} + +func TestDLQAcknowledgesUnusableMessages(t *testing.T) { + for _, kind := range []string{ + "malformed payload", "empty queue", "empty tenant", "tenant mismatch", "metadata mismatch", "empty build ID", + "unresolvable queue", "missing build", "build without request", "missing request", "request queue mismatch", + } { + t.Run(kind, func(t *testing.T) { + c, m := newDLQTestController(t) + msg := analysisMessage(t) + + switch kind { + case "malformed payload": + msg.Payload = []byte("{") + case "empty queue": + msg.Payload = []byte(`{"id":"build/42"}`) + case "empty tenant": + msg.Tenant = "" + case "tenant mismatch": + msg.Tenant = "other/main" + case "metadata mismatch": + msg.Metadata[entityqueue.MetadataKeyQueueName] = "other/main" + case "empty build ID": + msg.Payload = []byte(`{"queue_name":"validation/main"}`) + case "unresolvable queue": + m.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(nil, assert.AnError) + default: + m.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) + switch kind { + case "missing build": + m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{}, storage.ErrNotFound) + case "build without request": + m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{ID: testBuildID}, nil) + case "missing request": + m.builds.EXPECT().Get(gomock.Any(), testBuildID). + Return(entity.Build{ID: testBuildID, RequestID: "request/1"}, nil) + m.requests.EXPECT().Get(gomock.Any(), "request/1").Return(entity.Request{}, storage.ErrNotFound) + case "request queue mismatch": + m.builds.EXPECT().Get(gomock.Any(), testBuildID). + Return(entity.Build{ID: testBuildID, RequestID: "request/1"}, nil) + m.requests.EXPECT().Get(gomock.Any(), "request/1"). + Return(entity.Request{ID: "request/1", Queue: "other/main"}, nil) + } + } + + require.NoError(t, c.Process(context.Background(), m.delivery(msg, false))) + }) + } +} + +func TestDLQDeduplicatesHistoryAcrossRetriesAndNotices(t *testing.T) { + c, m := newDLQTestController(t) + c.materializer = requestlog.NewMaterializer(tally.NoopScope) + logStore := storagemock.NewMockRequestLogStore(m.ctrl) + m.store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() + retained := make(map[string]entity.RequestLog) + failFirstWrite := true + + logStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, log entity.RequestLog) error { + require.NoError(t, log.Validate()) + if _, exists := retained[log.ID]; exists { + return storage.ErrAlreadyExists + } + + retained[log.ID] = log + if failFirstWrite { + failFirstWrite = false + return assert.AnError + } + + return nil + }, + ).Times(4) + logStore.EXPECT().Get(gomock.Any(), "request/1", gomock.Any()).DoAndReturn( + func(_ context.Context, _ string, id string) (entity.RequestLog, error) { + return retained[id], nil + }, + ).Times(2) + + for i, buildID := range []string{testBuildID, testBuildID, testBuildID, "build/43"} { + request := entity.Request{ID: "request/1", Queue: testQueue, State: entity.RequestStateProcessing, Version: int32(i + 1)} + if i > 0 { + request.State = entity.RequestStateSucceeded + } + + m.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) + m.builds.EXPECT().Get(gomock.Any(), buildID).Return(entity.Build{ID: buildID, RequestID: request.ID}, nil) + m.requests.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) + msg := analysisMessage(t) + msg.ID = fmt.Sprintf("notice-%d", i) + msg.PartitionKey = buildID + payload, err := stovepipemq.Marshal(&stovepipemq.BuildAnalysis{Id: buildID, QueueName: testQueue}) + require.NoError(t, err) + msg.Payload = payload + + err = c.Process(context.Background(), m.delivery(msg, i != 0)) + if i == 0 { + require.ErrorIs(t, err, assert.AnError) + } else { + require.NoError(t, err) + } + } + + require.Len(t, retained, 2) + for _, buildID := range []string{testBuildID, "build/43"} { + log, ok := retained["event/build_analysis_abandoned/"+buildID] + require.True(t, ok) + assert.Equal(t, buildID, log.Metadata[requestlog.MetadataKeyBuildID]) + } +} diff --git a/stovepipe/entity/request_log.go b/stovepipe/entity/request_log.go index 321a3061a..7c53434c4 100644 --- a/stovepipe/entity/request_log.go +++ b/stovepipe/entity/request_log.go @@ -32,6 +32,8 @@ const ( RequestEventProjectFactsRecorded RequestEvent = "project_facts_recorded" // RequestEventRecordAbandoned records that record-stage work stopped after exhausting retries. RequestEventRecordAbandoned RequestEvent = "record_abandoned" + // RequestEventBuildAnalysisAbandoned records incomplete build analysis, not a validation outcome. + RequestEventBuildAnalysisAbandoned RequestEvent = "build_analysis_abandoned" ) // RequestOutcomeReason identifies the durable domain reason for a terminal request state. @@ -137,7 +139,7 @@ func (e RequestLog) validateEvent() error { return fmt.Errorf("event log must not contain request-state context") } switch e.Event { - case RequestEventBuildTriggered, RequestEventBuildFinished, RequestEventValidationFactRecorded, RequestEventProjectFactsRecorded, RequestEventRecordAbandoned: + case RequestEventBuildTriggered, RequestEventBuildFinished, RequestEventValidationFactRecorded, RequestEventProjectFactsRecorded, RequestEventRecordAbandoned, RequestEventBuildAnalysisAbandoned: default: return fmt.Errorf("unknown request event %q", e.Event) } diff --git a/stovepipe/entity/request_log_test.go b/stovepipe/entity/request_log_test.go index 67954c209..dc6a82975 100644 --- a/stovepipe/entity/request_log_test.go +++ b/stovepipe/entity/request_log_test.go @@ -145,6 +145,16 @@ func TestRequestLogValidate(t *testing.T) { return entry }, }, + { + name: "incomplete build analysis event", + mutate: func(entry RequestLog) RequestLog { + entry.State = RequestStateUnknown + entry.Event = RequestEventBuildAnalysisAbandoned + entry.RequestVersion = 0 + entry.Metadata = map[string]string{"build_id": "42"} + return entry + }, + }, {name: "missing ID", mutate: func(entry RequestLog) RequestLog { entry.ID = ""; return entry }, wantErr: true}, {name: "missing queue", mutate: func(entry RequestLog) RequestLog { entry.Queue = ""; return entry }, wantErr: true}, {name: "missing request ID", mutate: func(entry RequestLog) RequestLog { entry.RequestID = ""; return entry }, wantErr: true}, From 62c8e8aa760a37ea686e71e3d996b8ae00e7a607 Mon Sep 17 00:00:00 2001 From: "prath.shenoy" Date: Wed, 21 Oct 2026 20:39:56 +0000 Subject: [PATCH 2/2] feat(stovepipe): Wire build analysis pipeline --- doc/rfc/stovepipe/steps/buildanalysis.md | 23 +++ doc/rfc/stovepipe/workflow.md | 2 + service/stovepipe/server/BUILD.bazel | 4 + service/stovepipe/server/main.go | 31 +++- service/stovepipe/server/main_test.go | 19 ++- stovepipe/controller/build/BUILD.bazel | 11 +- stovepipe/controller/build/build.go | 37 ++++- .../controller/build/build_analysis_test.go | 155 ++++++++++++++++++ stovepipe/controller/build/build_test.go | 94 +++++++++++ stovepipe/controller/buildanalysis/README.md | 4 +- stovepipe/controller/buildsignal/BUILD.bazel | 3 + .../controller/buildsignal/buildsignal.go | 36 +++- .../buildsignal/buildsignal_test.go | 96 +++++++++++ stovepipe/core/messagequeue/README.md | 2 +- .../extension/buildissue/noop/BUILD.bazel | 23 +++ stovepipe/extension/buildissue/noop/README.md | 3 + stovepipe/extension/buildissue/noop/noop.go | 42 +++++ .../extension/buildissue/noop/noop_test.go | 36 ++++ 18 files changed, 603 insertions(+), 18 deletions(-) create mode 100644 doc/rfc/stovepipe/steps/buildanalysis.md create mode 100644 stovepipe/controller/build/build_analysis_test.go create mode 100644 stovepipe/extension/buildissue/noop/BUILD.bazel create mode 100644 stovepipe/extension/buildissue/noop/README.md create mode 100644 stovepipe/extension/buildissue/noop/noop.go create mode 100644 stovepipe/extension/buildissue/noop/noop_test.go diff --git a/doc/rfc/stovepipe/steps/buildanalysis.md b/doc/rfc/stovepipe/steps/buildanalysis.md new file mode 100644 index 000000000..bca6c68f2 --- /dev/null +++ b/doc/rfc/stovepipe/steps/buildanalysis.md @@ -0,0 +1,23 @@ +# Build issue observation + +Build analysis runs alongside status polling and recording. Hosts supply a queue-resolved detector factory. The reference service uses a no-op detector so the analysis stage is always wired without inspecting provider output; deployments replace the factory with their integration. No-op results do not establish build success or recovery. + +## Start and completion notices + +After persisting a Build and its trigger occurrence, build publishes to buildsignal and buildanalysis. On completion, buildsignal persists status, releases the build slot, records the Request outcome, and publishes to record and buildanalysis. Each pair of publications attempts both destinations even if one fails. Publication is not atomic: one destination can receive its message while the other publication fails. + +Start and completion notices use distinct deterministic message IDs and the same build-ID partition. Retries of one notice deduplicate without suppressing the other. A held start delivery can delay the completion notice in that partition, but it also reloads Build status on every pass and can perform terminal analysis itself. Both notices may analyze the terminal Build; duplicate observations are safe. + +Publication errors are returned through the producing stage's retry/DLQ policy. Analysis processing does not gate record, but notification publication is still part of the producing stage: a failed start publication can send the build-stage message to its existing fail-closed recovery path. + +## Observations and polling + +The [analysis controller](../../../../stovepipe/controller/buildanalysis/README.md) owns detection, per-build observation persistence, polling, and the stop condition for Requests that end without a recorded build result. It only accumulates historical observations; it does not maintain open issues or infer recovery from absence. + +Status-polling DLQ recovery does not publish a build-completion notice. The analysis polling loop observes the ended Request through storage instead. + +## Analysis errors and dead letters + +Analysis errors are returned to the normal consumer whether the Build is running or terminal; errors do not use the two-minute polling hold. The reference service uses backend error classification and the extended retry configuration: a maximum of 10 attempts, with retry delays of 5, 10, 20, 40, then 60 seconds, capped at 60 seconds. A successful hold resets failure accounting. Non-retryable errors and exhausted retries send the message to the analysis DLQ, so final analysis is attempted rather than guaranteed to succeed. + +The analysis DLQ uses the existing DLQ consumer, with its always-retryable processor and unlimited-retry subscription. The recovery controller records incomplete analysis in Request history without rerunning detection or changing the validation outcome. Only state reads and history persistence retry; unusable messages are logged and acknowledged. See [DLQ recovery](../../../../stovepipe/controller/buildanalysis/README.md#dlq-recovery) for the history identity and error-handling rules. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index 2a746e7c0..4383a0175 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -6,6 +6,8 @@ The pipeline is a queue-driven chain of small, single-purpose controllers, in th > **poll for a new head → ingest → process the build strategy → build → buildsignal → record whole-repo greenness (and any project results the resolver returns) → hook.** +The [build issue observation](steps/buildanalysis.md) branch runs alongside this chain. It accumulates issues while builds run and attempts final analysis after build completion. Record does not wait for analysis to succeed. The reference host wires a no-op detector; deployments supply a real detector factory. + There is no `analyze` controller and no `analyze` topic. A later project-analysis stage that would map a target graph onto another `build` pass is design only, described under [Designed, not built](#designed-not-built-project-analysis). `record` resolves project results inline on the same delivery that writes the repository fact. ## What Stovepipe is agnostic about diff --git a/service/stovepipe/server/BUILD.bazel b/service/stovepipe/server/BUILD.bazel index 581a1c9f5..2989150ee 100644 --- a/service/stovepipe/server/BUILD.bazel +++ b/service/stovepipe/server/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//service/stovepipe/server/handler:go_default_library", "//stovepipe/controller:go_default_library", "//stovepipe/controller/build:go_default_library", + "//stovepipe/controller/buildanalysis:go_default_library", "//stovepipe/controller/buildsignal:go_default_library", "//stovepipe/controller/dlq:go_default_library", "//stovepipe/controller/process:go_default_library", @@ -34,6 +35,8 @@ go_library( "//stovepipe/core/queuelog:go_default_library", "//stovepipe/core/queuestate:go_default_library", "//stovepipe/core/requestlog:go_default_library", + "//stovepipe/extension/buildissue:go_default_library", + "//stovepipe/extension/buildissue/noop:go_default_library", "//stovepipe/extension/buildrunner:go_default_library", "//stovepipe/extension/buildrunner/fake:go_default_library", "//stovepipe/extension/projectstatus/noop:go_default_library", @@ -89,6 +92,7 @@ go_test( "//stovepipe/controller/dlq:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", + "//stovepipe/extension/buildissue/noop:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", "@com_github_uber_go_tally//:go_default_library", diff --git a/service/stovepipe/server/main.go b/service/stovepipe/server/main.go index b19596252..f13f8b8cb 100644 --- a/service/stovepipe/server/main.go +++ b/service/stovepipe/server/main.go @@ -48,6 +48,7 @@ import ( "github.com/uber/submitqueue/service/stovepipe/server/handler" "github.com/uber/submitqueue/stovepipe/controller" "github.com/uber/submitqueue/stovepipe/controller/build" + "github.com/uber/submitqueue/stovepipe/controller/buildanalysis" "github.com/uber/submitqueue/stovepipe/controller/buildsignal" "github.com/uber/submitqueue/stovepipe/controller/dlq" "github.com/uber/submitqueue/stovepipe/controller/process" @@ -56,6 +57,8 @@ import ( "github.com/uber/submitqueue/stovepipe/core/queuelog" "github.com/uber/submitqueue/stovepipe/core/queuestate" "github.com/uber/submitqueue/stovepipe/core/requestlog" + "github.com/uber/submitqueue/stovepipe/extension/buildissue" + buildissuenoop "github.com/uber/submitqueue/stovepipe/extension/buildissue/noop" "github.com/uber/submitqueue/stovepipe/extension/buildrunner" buildrunnerfake "github.com/uber/submitqueue/stovepipe/extension/buildrunner/fake" projectstatusnoop "github.com/uber/submitqueue/stovepipe/extension/projectstatus/noop" @@ -214,6 +217,7 @@ func run() error { subscriberName = fmt.Sprintf("stovepipe-%d", time.Now().Unix()) } + detectorFactory := buildissuenoop.New() registry, err := newTopicRegistry(mysqlQueue, subscriberName) if err != nil { return fmt.Errorf("failed to create topic registry: %w", err) @@ -254,7 +258,7 @@ func run() error { } } materializer := requestlog.NewMaterializer(scope) - primaryCount, err := registerPrimaryControllers(primaryConsumer, logger.Sugar(), scope, storageFty, materializer, registry, sourceControl, brf, hookResolver{}) + primaryCount, err := registerPrimaryControllers(primaryConsumer, logger.Sugar(), scope, storageFty, materializer, registry, sourceControl, brf, hookResolver{}, detectorFactory) if err != nil { return err } @@ -373,6 +377,7 @@ func registerPrimaryControllers( sourceControl sourcecontrol.Factory, brf buildrunner.Factory, hooks hookext.Hooks, + detectorFactory buildissue.DetectorFactory, ) (int, error) { var count int @@ -404,6 +409,12 @@ func registerPrimaryControllers( } count++ + analysisController := buildanalysis.NewController(logger, scope, store, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") + if err := c.Register(analysisController); err != nil { + return count, fmt.Errorf("failed to register buildanalysis controller: %w", err) + } + count++ + recordController := record.NewController(logger, scope, store, materializer, projectstatusnoop.New(), sourceControl, registry, stovepipemq.TopicKeyRecord, "stovepipe-record") if err := c.Register(recordController); err != nil { return count, fmt.Errorf("failed to register record controller: %w", err) @@ -448,6 +459,12 @@ func registerDLQControllers( } count++ + analysisDLQController := buildanalysis.NewDLQController(logger, scope, store, materializer, dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis), "stovepipe-buildanalysis-dlq") + if err := c.Register(analysisDLQController); err != nil { + return count, fmt.Errorf("failed to register buildanalysis dlq controller: %w", err) + } + count++ + recordDLQController := record.NewDLQController(logger, scope, store, materializer, dlq.TopicKey(stovepipemq.TopicKeyRecord), "stovepipe-record-dlq") if err := c.Register(recordDLQController); err != nil { return count, fmt.Errorf("failed to register record dlq controller: %w", err) @@ -544,6 +561,18 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe Queue: q, Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-hook-dlq"), }, + { + Key: stovepipemq.TopicKeyBuildAnalysis, + Name: "buildanalysis", + Queue: q, + Subscription: extqueue.ExtendedSubscriptionConfig(subscriberName, "stovepipe-buildanalysis"), + }, + { + Key: dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis), + Name: "buildanalysis_dlq", + Queue: q, + Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-buildanalysis-dlq"), + }, }) } diff --git a/service/stovepipe/server/main_test.go b/service/stovepipe/server/main_test.go index ec9614d0f..a864d2305 100644 --- a/service/stovepipe/server/main_test.go +++ b/service/stovepipe/server/main_test.go @@ -28,6 +28,7 @@ import ( "github.com/uber/submitqueue/stovepipe/controller/dlq" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" + buildissuenoop "github.com/uber/submitqueue/stovepipe/extension/buildissue/noop" "go.uber.org/zap/zaptest" ) @@ -60,7 +61,7 @@ func registeredControllers(t *testing.T) (consumer.TopicRegistry, []consumer.Con deadLetter := &recordingConsumer{} _, err = registerPrimaryControllers(primary, logger, tally.NoopScope, store, requestlog.NewMaterializer(tally.NoopScope), registry, - fakeSourceControlFactory{}, fakeBuildRunnerFactory{}, hookResolver{}) + fakeSourceControlFactory{}, fakeBuildRunnerFactory{}, hookResolver{}, buildissuenoop.New()) require.NoError(t, err) _, err = registerDLQControllers(deadLetter, logger, tally.NoopScope, store, requestlog.NewMaterializer(tally.NoopScope)) @@ -129,6 +130,7 @@ func TestPipelineSubscriptionRetryPolicies(t *testing.T) { {name: "process", topicKey: stovepipemq.TopicKeyProcess, consumerGroup: "stovepipe-process", wantRetry: defaultRetry}, {name: "build", topicKey: stovepipemq.TopicKeyBuild, consumerGroup: "stovepipe-build", wantRetry: defaultRetry}, {name: "buildsignal", topicKey: stovepipemq.TopicKeyBuildSignal, consumerGroup: "stovepipe-buildsignal", wantRetry: extendedRetry}, + {name: "buildanalysis", topicKey: stovepipemq.TopicKeyBuildAnalysis, consumerGroup: "stovepipe-buildanalysis", wantRetry: extendedRetry}, {name: "record", topicKey: stovepipemq.TopicKeyRecord, consumerGroup: "stovepipe-record", wantRetry: extendedRetry}, {name: "hook", topicKey: basehook.TopicKeyHook, consumerGroup: "stovepipe-hook", wantRetry: defaultRetry}, } @@ -154,3 +156,18 @@ func TestDLQSubscriptionRetryPoliciesRemainUnlimited(t *testing.T) { }) } } + +func TestBuildAnalysisRegistration(t *testing.T) { + registry, primary, deadLetter := registeredControllers(t) + assert.Contains(t, topicKeys(primary), stovepipemq.TopicKeyBuildAnalysis) + assert.Contains(t, topicKeys(deadLetter), dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis)) + + dlqName, ok := registry.TopicName(dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis)) + require.True(t, ok) + assert.Equal(t, "buildanalysis_dlq", dlqName) + + cfg, ok := registry.SubscriptionConfig(stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") + require.True(t, ok) + assert.True(t, cfg.DLQ.Enabled) + assert.Equal(t, "_dlq", cfg.DLQ.TopicSuffix) +} diff --git a/stovepipe/controller/build/BUILD.bazel b/stovepipe/controller/build/BUILD.bazel index f0dbe76a3..540880e2a 100644 --- a/stovepipe/controller/build/BUILD.bazel +++ b/stovepipe/controller/build/BUILD.bazel @@ -24,19 +24,28 @@ go_library( go_test( name = "go_default_test", - srcs = ["build_test.go"], + srcs = [ + "build_analysis_test.go", + "build_test.go", + ], embed = [":go_default_library"], deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", "//platform/errs:go_default_library", + "//platform/errs/mysql:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//platform/metrics:go_default_library", + "//platform/publish:go_default_library", + "//stovepipe/controller/buildanalysis:go_default_library", + "//stovepipe/controller/buildsignal:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", "//stovepipe/core/requestlog/mock:go_default_library", "//stovepipe/entity:go_default_library", + "//stovepipe/extension/buildissue:go_default_library", + "//stovepipe/extension/buildissue/mock:go_default_library", "//stovepipe/extension/buildrunner:go_default_library", "//stovepipe/extension/buildrunner/mock:go_default_library", "//stovepipe/extension/storage:go_default_library", diff --git a/stovepipe/controller/build/build.go b/stovepipe/controller/build/build.go index 8891f23dd..eae495190 100644 --- a/stovepipe/controller/build/build.go +++ b/stovepipe/controller/build/build.go @@ -15,7 +15,7 @@ // Package build holds the build-stage queue controller. It consumes BuildRequest // messages (a request id), reloads the Request, triggers the build-runner for // the scope process already decided, persists a Build row, and publishes the -// build id to buildsignal. +// build id to buildsignal and buildanalysis. package build import ( @@ -39,7 +39,7 @@ import ( // Controller consumes BuildRequest messages, reloads the referenced Request, // triggers a build for its already-decided scope, and publishes the resulting -// build id to buildsignal. Implements consumer.Controller. +// build id to status polling and analysis. Implements consumer.Controller. type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope @@ -51,9 +51,6 @@ type Controller struct { consumerGroup string } -// Verify Controller implements consumer.Controller interface at compile time. -var _ consumer.Controller = (*Controller)(nil) - // _opName is the metric operation name shared by every emit in this file. const _opName = "build" @@ -155,8 +152,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return err } - if err := c.publishBuildSignal(ctx, build.ID, request.Queue); err != nil { - return fmt.Errorf("failed to publish build signal for %s: %w", build.ID, err) + signalErr := c.publishBuildSignal(ctx, build.ID, request.Queue) + if signalErr != nil { + signalErr = fmt.Errorf("failed to publish build signal for %s: %w", build.ID, signalErr) + } + + analysisErr := c.publishBuildAnalysis(ctx, build.ID, request.Queue) + if err := errs.Group(signalErr, analysisErr); err != nil { + return err } c.logger.Debugw("triggered build", @@ -219,3 +222,23 @@ func (c *Controller) TopicKey() consumer.TopicKey { func (c *Controller) ConsumerGroup() string { return c.consumerGroup } + +func (c *Controller) publishBuildAnalysis(ctx context.Context, buildID, queue string) error { + payload, err := stovepipemq.Marshal(&stovepipemq.BuildAnalysis{Id: buildID, QueueName: queue}) + if err != nil { + return fmt.Errorf("failed to serialize build analysis: %w", err) + } + + if err := publish.Message(ctx, c.registry, stovepipemq.TopicKeyBuildAnalysis, publish.MessageParams{ + Tenant: queue, + ID: publish.IntentID(buildID, "started"), + Payload: payload, + PartitionKey: buildID, + }); err != nil { + return fmt.Errorf("failed to publish build analysis for %s: %w", buildID, err) + } + + return nil +} + +var _ consumer.Controller = (*Controller)(nil) diff --git a/stovepipe/controller/build/build_analysis_test.go b/stovepipe/controller/build/build_analysis_test.go new file mode 100644 index 000000000..e6b2cfcfd --- /dev/null +++ b/stovepipe/controller/build/build_analysis_test.go @@ -0,0 +1,155 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package build + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + consumermock "github.com/uber/submitqueue/platform/consumer/mock" + "github.com/uber/submitqueue/stovepipe/controller/buildanalysis" + "github.com/uber/submitqueue/stovepipe/controller/buildsignal" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/buildissue" + buildissuemock "github.com/uber/submitqueue/stovepipe/extension/buildissue/mock" + "github.com/uber/submitqueue/stovepipe/extension/buildrunner" + "github.com/uber/submitqueue/stovepipe/extension/storage" + storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock" + "go.uber.org/mock/gomock" + "go.uber.org/zap" +) + +func TestBuildAnalysisPipeline(t *testing.T) { + ctrl := gomock.NewController(t) + buildController, m := newController(t, ctrl) + ctx := queueContext() + + var savedBuild entity.Build + request := processingRequest(entity.BuildStrategyFull, "") + queue := entity.Queue{Name: testQueue, InFlightCount: 1, Version: 1} + + queueStore := storagemock.NewMockQueueStore(ctrl) + issueStore := storagemock.NewMockBuildIssueStore(ctrl) + m.store.EXPECT().GetQueueStore().Return(queueStore).AnyTimes() + m.store.EXPECT().GetBuildIssueStore().Return(issueStore).AnyTimes() + + m.reqStore.EXPECT().Get(gomock.Any(), testID).DoAndReturn(func(context.Context, string) (entity.Request, error) { + return request, nil + }).AnyTimes() + m.reqStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, value entity.Request, old, next int32) error { + require.Equal(t, request.Version, old) + request = value + request.Version = next + return nil + }) + + queueStore.EXPECT().Get(gomock.Any(), testQueue).DoAndReturn(func(context.Context, string) (entity.Queue, error) { + return queue, nil + }).AnyTimes() + queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, value entity.Queue, old, next int32) error { + require.Equal(t, queue.Version, old) + queue = value + queue.Version = next + return nil + }) + + m.buildStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, value entity.Build) error { + savedBuild = value + return nil + }) + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).DoAndReturn(func(context.Context, string) (entity.Build, error) { + return savedBuild, nil + }).AnyTimes() + m.buildStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, value entity.Build, old, next int32) error { + require.Equal(t, savedBuild.Version, old) + savedBuild = value + savedBuild.Version = next + return nil + }) + + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).AnyTimes() + + m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil).Times(2) + m.runner.EXPECT().Trigger(gomock.Any(), "", testHeadURI, entity.BuildMetadata(nil)).Return(entity.BuildID{ID: testBuildID}, nil) + m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusFailed, nil, nil) + + published := map[string][]entityqueue.Message{} + m.publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, topic string, msg entityqueue.Message) error { + require.Equal(t, testBuildID, savedBuild.ID) + published[topic] = append(published[topic], msg) + return nil + }).AnyTimes() + + a := entity.BuildIssue{TaskID: "validation", UnitID: "unit-a"} + b := entity.BuildIssue{TaskID: "validation", UnitID: "unit-b"} + persisted := map[entity.BuildIssue]bool{} + issueStore.EXPECT().Create(gomock.Any(), testBuildID, gomock.Any()).DoAndReturn(func(_ context.Context, _ string, issue entity.BuildIssue) error { + if persisted[issue] { + return storage.ErrAlreadyExists + } + persisted[issue] = true + return nil + }).AnyTimes() + + detectorFactory := buildissuemock.NewMockDetectorFactory(ctrl) + detector := buildissuemock.NewMockDetector(ctrl) + detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(detector, nil).Times(3) + gomock.InOrder( + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a}, nil), + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a, b}, nil), + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a, b}, nil), + ) + + analysisController := buildanalysis.NewController(zap.NewNop().Sugar(), tally.NoopScope, + staticStorageFactory{store: m.store}, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "analysis") + statusController := buildsignal.NewController(zap.NewNop().Sugar(), tally.NoopScope, + staticStorageFactory{store: m.store}, m.materializer, m.runnerFactory, buildController.registry, + stovepipemq.TopicKeyBuildSignal, "signal") + + delivered := func(msg entityqueue.Message) *consumermock.MockDelivery { + d := consumermock.NewMockDelivery(ctrl) + d.EXPECT().Message().Return(msg) + return d + } + + require.NoError(t, buildController.Process(ctx, delivery(t, ctrl, buildPayload(t, testID)))) + require.Len(t, published["buildanalysis"], 1) + require.Len(t, published["buildsignal"], 1) + + firstPoll := delivered(published["buildanalysis"][0]) + firstPoll.EXPECT().Hold(buildanalysis.PollDelayMs) + require.NoError(t, analysisController.Process(ctx, firstPoll)) + assert.True(t, persisted[a]) + assert.False(t, persisted[b]) + + require.NoError(t, statusController.Process(ctx, delivered(published["buildsignal"][0]))) + require.Len(t, published["record"], 1) + require.Len(t, published["buildanalysis"], 2) + assert.Equal(t, entity.BuildStatusFailed, savedBuild.Status) + assert.Equal(t, entity.RequestStateFailed, request.State) + assert.Zero(t, queue.InFlightCount) + assert.False(t, persisted[b]) + assert.NotEqual(t, published["buildanalysis"][0].ID, published["buildanalysis"][1].ID) + + require.NoError(t, analysisController.Process(ctx, delivered(published["buildanalysis"][0]))) + require.NoError(t, analysisController.Process(ctx, delivered(published["buildanalysis"][1]))) + assert.Len(t, persisted, 2) + assert.True(t, persisted[b]) +} diff --git a/stovepipe/controller/build/build_test.go b/stovepipe/controller/build/build_test.go index bba48204d..6b83a5a7b 100644 --- a/stovepipe/controller/build/build_test.go +++ b/stovepipe/controller/build/build_test.go @@ -16,6 +16,8 @@ package build import ( "context" + "database/sql" + "database/sql/driver" "errors" "testing" @@ -26,8 +28,10 @@ import ( "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" "github.com/uber/submitqueue/platform/errs" + mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" mqmock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/platform/publish" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" requestlogmock "github.com/uber/submitqueue/stovepipe/core/requestlog/mock" @@ -94,6 +98,8 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, buildMoc registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ {Key: stovepipemq.TopicKeyBuildSignal, Name: "buildsignal", Queue: queue}, + {Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", Queue: queue}, + {Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue}, }) require.NoError(t, err) @@ -223,6 +229,7 @@ func TestProcess(t *testing.T) { createCall := m.buildStore.EXPECT().Create(gomock.Any(), build).Return(nil) logCall := expectBuildTriggered(m).After(createCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -241,6 +248,7 @@ func TestProcess(t *testing.T) { createCall := m.buildStore.EXPECT().Create(gomock.Any(), build).Return(nil) logCall := expectBuildTriggered(m).After(createCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -379,6 +387,7 @@ func TestProcess(t *testing.T) { createCall := m.buildStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) logCall := expectBuildTriggered(m).After(createCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()).Return(errors.New("queue down")).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -414,3 +423,88 @@ func TestProcess(t *testing.T) { }) } } + +func TestProcessPublishesBothPollsAfterPersistence(t *testing.T) { + signalFailure := errors.New("status publish unavailable") + analysisFailure := errors.New("analysis publish unavailable") + + for _, tt := range []struct { + name string + signalErr error + analysisErr error + wantRetry bool + }{ + {name: "both succeed"}, + {name: "status publish fails", signalErr: signalFailure}, + {name: "analysis publish fails", analysisErr: analysisFailure}, + {name: "both fail", signalErr: signalFailure, analysisErr: analysisFailure}, + {name: "transient status publish", signalErr: driver.ErrBadConn, wantRetry: true}, + {name: "transient analysis publish", analysisErr: driver.ErrBadConn, wantRetry: true}, + {name: "permanent status and transient analysis", signalErr: sql.ErrTxDone, analysisErr: driver.ErrBadConn, wantRetry: true}, + {name: "transient status and permanent analysis", signalErr: driver.ErrBadConn, analysisErr: sql.ErrTxDone, wantRetry: true}, + {name: "both permanent", signalErr: sql.ErrTxDone, analysisErr: sql.ErrTxDone}, + } { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newController(t, ctrl) + + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(processingRequest(entity.BuildStrategyFull, ""), nil) + + m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) + m.runner.EXPECT().Trigger(gomock.Any(), "", testHeadURI, entity.BuildMetadata(nil)).Return(entity.BuildID{ID: testBuildID}, nil) + + persisted := m.buildStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + logged := expectBuildTriggered(m).After(persisted) + + m.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()).Return(tt.signalErr).After(logged) + + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + var payload stovepipemq.BuildAnalysis + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, &payload)) + assert.Equal(t, testBuildID, payload.Id) + assert.Equal(t, testQueue, payload.QueueName) + assert.Equal(t, testBuildID, msg.PartitionKey) + assert.Equal(t, testQueue, msg.Tenant) + assert.Equal(t, testQueue, msg.Metadata[entityqueue.MetadataKeyQueueName]) + assert.Equal(t, publish.IntentID(testBuildID, "started"), msg.ID) + return tt.analysisErr + }).After(logged) + + err := c.Process(queueContext(), delivery(t, ctrl, buildPayload(t, testID))) + if tt.signalErr == nil && tt.analysisErr == nil { + require.NoError(t, err) + } + + if tt.signalErr != nil { + require.ErrorIs(t, err, tt.signalErr) + } + + if tt.analysisErr != nil { + require.ErrorIs(t, err, tt.analysisErr) + } + + if err != nil { + processed := errs.NewClassifierProcessor(mysqlerrs.Classifier).Process(err) + assert.Equal(t, tt.wantRetry, errs.IsRetryable(processed)) + } + }) + } +} + +func TestAnalysisPublishDeduplicatesRetriesOfTheSameOccasion(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newController(t, ctrl) + + var ids []string + + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + ids = append(ids, msg.ID) + return nil + }).Times(2) + + require.NoError(t, c.publishBuildAnalysis(queueContext(), testBuildID, testQueue)) + require.NoError(t, c.publishBuildAnalysis(queueContext(), testBuildID, testQueue)) + + assert.Equal(t, ids[0], ids[1]) + assert.NotEqual(t, publish.IntentID(testBuildID, "finished"), ids[0]) +} diff --git a/stovepipe/controller/buildanalysis/README.md b/stovepipe/controller/buildanalysis/README.md index 95c78104d..33cb3b3d2 100644 --- a/stovepipe/controller/buildanalysis/README.md +++ b/stovepipe/controller/buildanalysis/README.md @@ -26,4 +26,6 @@ History identity is stable per build, so repeated recovery deliveries and separa State-read and history-write errors are returned for retry. Hosts must run recovery with the DLQ consumer's always-retryable processor and unlimited-retry subscription; only recovery work is retried, not analysis. Unusable payloads, invalid or unresolvable queues, missing Build or Request records, and queue mismatches are logged and acknowledged rather than retried indefinitely. -The controllers are defined without runtime producers or consumer registration. Host integration and detector implementations are separate work. +## Host integration + +The reference service supplies a no-op detector and publishes analysis notices at build start and completion. It registers the normal analysis controller with bounded retries and the recovery controller with the existing DLQ consumer. See [build issue observation](../../../doc/rfc/stovepipe/steps/buildanalysis.md) for the publication rules and retry configuration. diff --git a/stovepipe/controller/buildsignal/BUILD.bazel b/stovepipe/controller/buildsignal/BUILD.bazel index 61088f534..77ac7b005 100644 --- a/stovepipe/controller/buildsignal/BUILD.bazel +++ b/stovepipe/controller/buildsignal/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//platform/publish:go_default_library", "//stovepipe/core/loader:go_default_library", @@ -30,8 +31,10 @@ go_test( "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", "//platform/errs:go_default_library", + "//platform/errs/mysql:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//platform/metrics:go_default_library", + "//platform/publish:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", "//stovepipe/core/requestlog/mock:go_default_library", diff --git a/stovepipe/controller/buildsignal/buildsignal.go b/stovepipe/controller/buildsignal/buildsignal.go index 924eafb85..32ceb7be4 100644 --- a/stovepipe/controller/buildsignal/buildsignal.go +++ b/stovepipe/controller/buildsignal/buildsignal.go @@ -16,7 +16,7 @@ // BuildSignal messages (a build id), polls the build-runner until the build // reaches a terminal status, persists that status, and — once terminal — // releases the queue's build slot, projects the outcome onto the request, and -// publishes the request id to record. See +// publishes directly to record plus terminal analysis. See // doc/rfc/stovepipe/steps/buildsignal.md. package buildsignal @@ -28,6 +28,7 @@ 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/platform/publish" "github.com/uber/submitqueue/stovepipe/core/loader" @@ -69,9 +70,6 @@ type Controller struct { consumerGroup string } -// Verify Controller implements consumer.Controller interface at compile time. -var _ consumer.Controller = (*Controller)(nil) - // _opName is the metric operation name shared by every emit in this file. const _opName = "buildsignal" @@ -183,8 +181,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er if err := c.persistOutcomeLog(ctx, store, request); err != nil { return err } - if err := c.publishRecord(ctx, request.ID, request.Queue); err != nil { - return fmt.Errorf("failed to publish record for request %s: %w", request.ID, err) + recordErr := c.publishRecord(ctx, request.ID, request.Queue) + if recordErr != nil { + recordErr = fmt.Errorf("failed to publish record for request %s: %w", request.ID, recordErr) + } + + analysisErr := c.publishBuildAnalysis(ctx, build.ID, request.Queue) + if err := errs.Group(recordErr, analysisErr); err != nil { + return err } c.logger.Infow("build reached terminal status", "build_id", build.ID, @@ -432,3 +436,23 @@ func (c *Controller) TopicKey() consumer.TopicKey { func (c *Controller) ConsumerGroup() string { return c.consumerGroup } + +func (c *Controller) publishBuildAnalysis(ctx context.Context, buildID, queue string) error { + payload, err := stovepipemq.Marshal(&stovepipemq.BuildAnalysis{Id: buildID, QueueName: queue}) + if err != nil { + return fmt.Errorf("failed to serialize build analysis: %w", err) + } + + if err := publish.Message(ctx, c.registry, stovepipemq.TopicKeyBuildAnalysis, publish.MessageParams{ + Tenant: queue, + ID: publish.IntentID(buildID, "finished"), + Payload: payload, + PartitionKey: buildID, + }); err != nil { + return fmt.Errorf("failed to publish build analysis for %s: %w", buildID, err) + } + + return nil +} + +var _ consumer.Controller = (*Controller)(nil) diff --git a/stovepipe/controller/buildsignal/buildsignal_test.go b/stovepipe/controller/buildsignal/buildsignal_test.go index aac4eecf7..d789f839e 100644 --- a/stovepipe/controller/buildsignal/buildsignal_test.go +++ b/stovepipe/controller/buildsignal/buildsignal_test.go @@ -16,9 +16,13 @@ package buildsignal import ( "context" + "database/sql" + "database/sql/driver" "errors" "testing" + "github.com/uber/submitqueue/platform/publish" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally" @@ -26,6 +30,7 @@ import ( "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" "github.com/uber/submitqueue/platform/errs" + mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" mqmock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/platform/metrics" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" @@ -96,6 +101,7 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, buildsig registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ {Key: stovepipemq.TopicKeyBuildSignal, Name: "buildsignal", Queue: queue}, + {Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", Queue: queue}, {Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue}, }) require.NoError(t, err) @@ -335,6 +341,7 @@ func TestProcess(t *testing.T) { m.buildStore.EXPECT().Update(gomock.Any(), build(entity.BuildStatusSucceeded, 2), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateSucceeded) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -347,6 +354,7 @@ func TestProcess(t *testing.T) { m.buildStore.EXPECT().Update(gomock.Any(), build(entity.BuildStatusFailed, 2), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateFailed) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -359,6 +367,7 @@ func TestProcess(t *testing.T) { m.buildStore.EXPECT().Update(gomock.Any(), build(entity.BuildStatusCancelled, 2), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateCancelled) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -375,6 +384,7 @@ func TestProcess(t *testing.T) { eventCall := expectBuildFinished(m) logCall := expectOutcomeLog(m, entity.RequestStateSucceeded, 2).After(eventCall) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -429,6 +439,7 @@ func TestProcess(t *testing.T) { // status, not the poll, decides the request's outcome. logCall := expectFinish(m, entity.RequestStateSucceeded) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -518,6 +529,7 @@ func TestProcess(t *testing.T) { m.buildStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateSucceeded) m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(errors.New("queue down")).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, { @@ -601,3 +613,87 @@ func TestOutcomeState(t *testing.T) { }) } } + +func TestTerminalAnalysisPublicationDoesNotGateRecordPublication(t *testing.T) { + recordFailure := errors.New("record publish unavailable") + analysisFailure := errors.New("analysis publish unavailable") + + for _, tt := range []struct { + name string + recordErr error + analysisErr error + wantRetry bool + }{ + {name: "both succeed"}, + {name: "record publish fails", recordErr: recordFailure}, + {name: "analysis publish fails", analysisErr: analysisFailure}, + {name: "both fail", recordErr: recordFailure, analysisErr: analysisFailure}, + {name: "transient record publish", recordErr: driver.ErrBadConn, wantRetry: true}, + {name: "transient analysis publish", analysisErr: driver.ErrBadConn, wantRetry: true}, + {name: "permanent record and transient analysis", recordErr: sql.ErrTxDone, analysisErr: driver.ErrBadConn, wantRetry: true}, + {name: "transient record and permanent analysis", recordErr: driver.ErrBadConn, analysisErr: sql.ErrTxDone, wantRetry: true}, + {name: "both permanent", recordErr: sql.ErrTxDone, analysisErr: sql.ErrTxDone}, + } { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newController(t, ctrl) + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusFailed, 2), nil) + request := requestWithState(entity.RequestStateFailed) + + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil) + + m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) + m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusFailed, nil, nil) + + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).Times(2) + + m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(tt.recordErr) + + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + var payload stovepipemq.BuildAnalysis + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, &payload)) + assert.Equal(t, testBuildID, payload.Id) + assert.Equal(t, testQueue, payload.QueueName) + assert.Equal(t, testBuildID, msg.PartitionKey) + assert.Equal(t, testQueue, msg.Tenant) + assert.Equal(t, publish.IntentID(testBuildID, "finished"), msg.ID) + assert.NotEqual(t, publish.IntentID(testBuildID, "started"), msg.ID) + return tt.analysisErr + }) + + err := c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID))) + if tt.recordErr == nil && tt.analysisErr == nil { + require.NoError(t, err) + } + + if tt.recordErr != nil { + require.ErrorIs(t, err, tt.recordErr) + } + + if tt.analysisErr != nil { + require.ErrorIs(t, err, tt.analysisErr) + } + + if err != nil { + processed := errs.NewClassifierProcessor(mysqlerrs.Classifier).Process(err) + assert.Equal(t, tt.wantRetry, errs.IsRetryable(processed)) + } + }) + } +} + +func TestNonterminalPollDoesNotPublishTerminalAnalysis(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newController(t, ctrl) + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusRunning, 2), nil) + + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateProcessing), nil) + + m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) + m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusRunning, nil, nil) + + d := delivery(t, ctrl, buildSignalPayload(t, testBuildID)) + d.EXPECT().Hold(PollDelayRunningMs) + + require.NoError(t, c.Process(queueContext(), d)) +} diff --git a/stovepipe/core/messagequeue/README.md b/stovepipe/core/messagequeue/README.md index 225a8e238..39d8df7a8 100644 --- a/stovepipe/core/messagequeue/README.md +++ b/stovepipe/core/messagequeue/README.md @@ -9,5 +9,5 @@ Payloads are defined in proto3 (`proto/`, generated into `protopb/`) and seriali - **process** (`TopicKeyProcess`, `ProcessRequest`) — ingest publishes the minted request id here once it accepts a new head; the process controller reloads the `Request` from storage and decides the build strategy. Only the id travels: producer and consumer share the store, so messages stay small and redelivery is idempotent. - **build** (`TopicKeyBuild`, `BuildRequest`) — process publishes the request id here once its build scope (`BuildStrategy`/`BaseURI`) is decided; the build controller reloads the `Request`, triggers the build, and persists the resulting `Build`. Partitioned by request id. - **buildsignal** (`TopicKeyBuildSignal`, `BuildSignal`) — build publishes the build id here after triggering; buildsignal holds the delivery between polls until the build reaches a terminal status. Partitioned by build id, so each build's poll loop is an independent partition. -- **buildanalysis** (`TopicKeyBuildAnalysis`, `BuildAnalysis`) — identifies a persisted build for periodic issue detection. The controller holds between nonterminal passes and performs a final pass after observing terminal status. Partitioned by build id. This contract is not yet activated by runtime producers or consumers. +- **buildanalysis** (`TopicKeyBuildAnalysis`, `BuildAnalysis`) — identifies a persisted build for issue detection, partitioned by build id. Build publishes a start notice and buildsignal publishes a completion notice with a distinct message id. See [build analysis](../../controller/buildanalysis/README.md) for polling and persistence behavior. - **record** (`TopicKeyRecord`, `Record`) — buildsignal publishes the request id after persisting the terminal outcome; record reloads the request and records its validation fact. Partitioned by request id. diff --git a/stovepipe/extension/buildissue/noop/BUILD.bazel b/stovepipe/extension/buildissue/noop/BUILD.bazel new file mode 100644 index 000000000..cf8f77e55 --- /dev/null +++ b/stovepipe/extension/buildissue/noop/BUILD.bazel @@ -0,0 +1,23 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["noop.go"], + importpath = "github.com/uber/submitqueue/stovepipe/extension/buildissue/noop", + visibility = ["//visibility:public"], + deps = [ + "//stovepipe/entity:go_default_library", + "//stovepipe/extension/buildissue:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["noop_test.go"], + embed = [":go_default_library"], + deps = [ + "//stovepipe/entity:go_default_library", + "//stovepipe/extension/buildissue:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/stovepipe/extension/buildissue/noop/README.md b/stovepipe/extension/buildissue/noop/README.md new file mode 100644 index 000000000..8834682b0 --- /dev/null +++ b/stovepipe/extension/buildissue/noop/README.md @@ -0,0 +1,3 @@ +# No-op build issue detector + +Returns no observations without inspecting build output. The package supplies both the detector and its factory. The reference host uses this implementation to keep the analysis stage wired; deployments supply a real detector through host wiring. An empty result does not establish build success or recovery. diff --git a/stovepipe/extension/buildissue/noop/noop.go b/stovepipe/extension/buildissue/noop/noop.go new file mode 100644 index 000000000..b3f555b11 --- /dev/null +++ b/stovepipe/extension/buildissue/noop/noop.go @@ -0,0 +1,42 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package noop provides a detector that returns no issue observations. +package noop + +import ( + "context" + + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/buildissue" +) + +// DetectorFactory returns a no-op detector for every queue. +type DetectorFactory struct{} + +// New returns a no-op detector factory. +func New() DetectorFactory { return DetectorFactory{} } + +func (DetectorFactory) For(buildissue.DetectorConfig) (buildissue.Detector, error) { + return detector{}, nil +} + +type detector struct{} + +func (detector) Detect(context.Context, entity.BuildID) ([]entity.BuildIssue, error) { + return nil, nil +} + +var _ buildissue.DetectorFactory = DetectorFactory{} +var _ buildissue.Detector = detector{} diff --git a/stovepipe/extension/buildissue/noop/noop_test.go b/stovepipe/extension/buildissue/noop/noop_test.go new file mode 100644 index 000000000..5f73245b5 --- /dev/null +++ b/stovepipe/extension/buildissue/noop/noop_test.go @@ -0,0 +1,36 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package noop + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/buildissue" +) + +func TestDetectReturnsNoObservations(t *testing.T) { + factory := New() + detector, err := factory.For(buildissue.DetectorConfig{QueueName: "validation/main"}) + require.NoError(t, err) + + for _, buildID := range []string{"build/1", "build/2"} { + issues, err := detector.Detect(context.Background(), entity.BuildID{ID: buildID}) + require.NoError(t, err) + require.Empty(t, issues) + } +}