diff --git a/doc/rfc/stovepipe/steps/buildanalysis.md b/doc/rfc/stovepipe/steps/buildanalysis.md index bca6c68f2..b3fca372b 100644 --- a/doc/rfc/stovepipe/steps/buildanalysis.md +++ b/doc/rfc/stovepipe/steps/buildanalysis.md @@ -1,6 +1,6 @@ -# Build issue observation +# Build issue detection -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. +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. The reference host registers an issue-tracking publication route but does not yet consume those notifications. ## Start and completion notices @@ -10,9 +10,9 @@ Start and completion notices use distinct deterministic message IDs and the same 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 +## Build issues 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. +The [analysis controller](../../../../stovepipe/controller/buildanalysis/README.md) owns detection, per-build issue persistence, polling, and the stop condition for Requests that end without a recorded build result. It accumulates per-build issues and publishes queue-partitioned issue-tracking notifications after persistence; it does not maintain open issues itself. Successful terminal analysis requests final reconciliation only after the Request records that Build as its winning outcome. Final reconciliation uses the accumulated build issues, including issues omitted from the last detector response. See the controller documentation for handoff and deduplication rules. Status-polling DLQ recovery does not publish a build-completion notice. The analysis polling loop observes the ended Request through storage instead. diff --git a/service/stovepipe/server/main.go b/service/stovepipe/server/main.go index f13f8b8cb..3924ba235 100644 --- a/service/stovepipe/server/main.go +++ b/service/stovepipe/server/main.go @@ -409,7 +409,7 @@ func registerPrimaryControllers( } count++ - analysisController := buildanalysis.NewController(logger, scope, store, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") + analysisController := buildanalysis.NewController(logger, scope, store, detectorFactory, registry, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") if err := c.Register(analysisController); err != nil { return count, fmt.Errorf("failed to register buildanalysis controller: %w", err) } @@ -561,6 +561,11 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe Queue: q, Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-hook-dlq"), }, + { + Key: stovepipemq.TopicKeyIssueTrack, + Name: "issuetrack", + Queue: q, + }, { Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", diff --git a/service/stovepipe/server/main_test.go b/service/stovepipe/server/main_test.go index a864d2305..a3ea7a959 100644 --- a/service/stovepipe/server/main_test.go +++ b/service/stovepipe/server/main_test.go @@ -92,6 +92,15 @@ func TestEveryRegisteredControllerResolvesInTheTopicRegistry(t *testing.T) { } } +func TestIssueTrackingHasPublicationRouteWithoutConsumption(t *testing.T) { + registry, primary, deadLetter := registeredControllers(t) + name, ok := registry.TopicName(stovepipemq.TopicKeyIssueTrack) + require.True(t, ok) + assert.Equal(t, "issuetrack", name) + assert.NotContains(t, topicKeys(primary), stovepipemq.TopicKeyIssueTrack) + assert.NotContains(t, topicKeys(deadLetter), dlq.TopicKey(stovepipemq.TopicKeyIssueTrack)) +} + func TestHookStage(t *testing.T) { registry, primary, deadLetter := registeredControllers(t) diff --git a/stovepipe/controller/build/build_analysis_test.go b/stovepipe/controller/build/build_analysis_test.go index e6b2cfcfd..0609168d0 100644 --- a/stovepipe/controller/build/build_analysis_test.go +++ b/stovepipe/controller/build/build_analysis_test.go @@ -110,15 +110,16 @@ func TestBuildAnalysisPipeline(t *testing.T) { detectorFactory := buildissuemock.NewMockDetectorFactory(ctrl) detector := buildissuemock.NewMockDetector(ctrl) - detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(detector, nil).Times(3) + detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(detector, nil).Times(4) 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), + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{b}, nil), + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(nil, nil), + detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(nil, nil), ) analysisController := buildanalysis.NewController(zap.NewNop().Sugar(), tally.NoopScope, - staticStorageFactory{store: m.store}, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "analysis") + staticStorageFactory{store: m.store}, detectorFactory, buildController.registry, stovepipemq.TopicKeyBuildAnalysis, "analysis") statusController := buildsignal.NewController(zap.NewNop().Sugar(), tally.NoopScope, staticStorageFactory{store: m.store}, m.materializer, m.runnerFactory, buildController.registry, stovepipemq.TopicKeyBuildSignal, "signal") @@ -139,17 +140,37 @@ func TestBuildAnalysisPipeline(t *testing.T) { assert.True(t, persisted[a]) assert.False(t, persisted[b]) + secondPoll := delivered(published["buildanalysis"][0]) + secondPoll.EXPECT().Hold(buildanalysis.PollDelayMs) + require.NoError(t, analysisController.Process(ctx, secondPoll)) + assert.True(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.True(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]) + assert.True(t, persisted[a]) + + require.Len(t, published["issuetrack"], 4) + for i, msg := range published["issuetrack"] { + assert.Equal(t, testQueue, msg.PartitionKey) + assert.Equal(t, testQueue, msg.Tenant) + notification := &stovepipemq.IssueTrack{} + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, notification)) + assert.Equal(t, testBuildID, notification.GetBuildId()) + assert.Equal(t, testQueue, notification.GetQueueName()) + assert.Equal(t, i > 1, notification.GetFinalize()) + } + assert.NotEqual(t, published["issuetrack"][0].ID, published["issuetrack"][1].ID) + assert.NotEqual(t, published["issuetrack"][1].ID, published["issuetrack"][2].ID) + assert.Equal(t, published["issuetrack"][2].ID, published["issuetrack"][3].ID) } diff --git a/stovepipe/controller/build/build_test.go b/stovepipe/controller/build/build_test.go index 6b83a5a7b..e98089e8e 100644 --- a/stovepipe/controller/build/build_test.go +++ b/stovepipe/controller/build/build_test.go @@ -99,6 +99,7 @@ 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.TopicKeyIssueTrack, Name: "issuetrack", Queue: queue}, {Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue}, }) require.NoError(t, err) diff --git a/stovepipe/controller/buildanalysis/BUILD.bazel b/stovepipe/controller/buildanalysis/BUILD.bazel index 37afa9972..ae5acadb4 100644 --- a/stovepipe/controller/buildanalysis/BUILD.bazel +++ b/stovepipe/controller/buildanalysis/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/metrics:go_default_library", + "//platform/publish:go_default_library", "//stovepipe/core/loader:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", @@ -36,7 +37,9 @@ go_test( "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", "//platform/errs: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/buildanalysis/README.md b/stovepipe/controller/buildanalysis/README.md index 33cb3b3d2..a61adff6d 100644 --- a/stovepipe/controller/buildanalysis/README.md +++ b/stovepipe/controller/buildanalysis/README.md @@ -1,14 +1,22 @@ # Build analysis -## Observations +## Build issues -The controller reloads the queue-scoped Build, invokes the queue's issue detector, and persists each returned observation individually. Observations already stored for the same build, task, and unit within a queue are not persisted again, so repeated polls and retries after partial writes are safe. Detector results accompanying an error are discarded. +The controller reloads the queue-scoped Build, invokes the queue's issue detector, and persists each returned build issue individually. Build issues already stored for the same build, task, and unit within a queue are not persisted again, so repeated polls and retries after partial writes are safe. Detector results accompanying an error are discarded. -This controller only accumulates historical observations. An empty detection result neither deletes earlier observations nor establishes recovery. Open-issue projection, lifecycle history, and resolution are outside its scope. +After persistence, the controller publishes an issue-tracking notification partitioned by queue. Issue tracking maintains open issues and lifecycle history; analysis never writes them directly. An empty detection result does not delete earlier build issues. + +## Issue-tracking handoff + +Refresh notifications let issue tracking open issues reported by the build without closing other open issues. Final notifications let it also close open issues absent from the build's accumulated issues. A successful terminal detection must discover all remaining reportable issues; it need not repeat earlier build issues. An empty terminal response still triggers finalization, but does not retract earlier build issues from the same build. See the [detector contract](../../extension/buildissue/README.md). + +All analysis deliveries for a build must share its build-ID partition and consumer group. The controller validates that partition key. Finalization requires a successful pass that began with a terminal Build and the Request's recorded winning build ID. If status becomes terminal before that outcome is recorded, analysis publishes a refresh and holds until the outcome is available. Known losing builds do not publish. A terminal Request without a recorded winning build permits a refresh but not closing issues based on absence. + +Every refresh gets a fresh notification ID because held polling deliveries do not identify individual passes. A fixed ID would suppress later refreshes after earlier notifications were consumed. Retries can therefore publish duplicate refreshes; issue tracking is idempotent. Final notifications use one deterministic ID per build, so duplicate terminal passes deduplicate. Publication errors return through the normal analysis retry/DLQ policy; no hold or successful completion occurs before publication succeeds. ## Polling and completion -Every delivery reloads the Build and its Request before detection. After a successful pass, a nonterminal Build holds the delivery for two minutes; a terminal Build completes the delivery after persistence. The status read before detection determines whether the pass holds: if the Build finishes during detection, another delivery is needed to analyze it after observing terminal status. More than one delivery may analyze the same terminal Build; observation persistence remains idempotent. +Every delivery reloads the Build and its Request before detection. After a successful pass, a nonterminal Build holds the delivery for two minutes; a terminal Build completes after persistence and publication once its Request outcome is known, or the Request has ended without one. A pass that begins with a nonterminal Build always holds, even if the Build finishes during detection, so another delivery analyzes it after observing terminal status. More than one delivery may analyze the same terminal Build; build-issue persistence remains idempotent. If the Request is terminal but the loaded Build is nonterminal, analysis reloads the Build before deciding to stop. The Build may have finished between the initial Build read and the Request read; reloading avoids skipping its final analysis. If it is still nonterminal, analysis stops without detection or a hold. This can occur when the status-polling DLQ handler fails the Request without learning the actual build result, leaving the Build at its last observed status. @@ -16,11 +24,11 @@ Status polling and updates remain the responsibility of buildsignal. Analysis ne ## Errors -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. +Detection, persistence, publication, 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. ## 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. +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 the recorded build issues may be incomplete. It does not rerun detection, delete earlier build issues, 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. @@ -28,4 +36,4 @@ State-read and history-write errors are returned for retry. Hosts must run recov ## 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. +The reference service supplies a no-op detector and publishes analysis notices at build start and completion. It also registers a publish-only issue-tracking topic; issue-tracking consumption and recovery are not enabled yet. It registers the normal analysis controller with bounded retries and the recovery controller with the existing DLQ consumer. See [build issue detection](../../../doc/rfc/stovepipe/steps/buildanalysis.md) for the publication rules and retry configuration. diff --git a/stovepipe/controller/buildanalysis/buildanalysis.go b/stovepipe/controller/buildanalysis/buildanalysis.go index 4ebaff55e..5bbb35147 100644 --- a/stovepipe/controller/buildanalysis/buildanalysis.go +++ b/stovepipe/controller/buildanalysis/buildanalysis.go @@ -25,6 +25,7 @@ import ( entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/platform/publish" "github.com/uber/submitqueue/stovepipe/core/loader" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/entity" @@ -33,17 +34,18 @@ import ( "go.uber.org/zap" ) -// PollDelayMs is the delay in milliseconds after a successful nonterminal pass. +// PollDelayMs is the delay in milliseconds between successful polling passes. // Two minutes limits provider traffic during long-running builds. const PollDelayMs int64 = 120000 // Controller accumulates observations while builds run and after completion. -// Terminal analysis must finish persistence before its delivery completes. +// Terminal analysis must finish persistence and publication before its delivery completes. type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope stores storage.Factory detectorFactory buildissue.DetectorFactory + registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string } @@ -53,6 +55,7 @@ func NewController( scope tally.Scope, stores storage.Factory, detectorFactory buildissue.DetectorFactory, + registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, ) *Controller { @@ -61,13 +64,14 @@ func NewController( metricsScope: scope.SubScope("buildanalysis_controller"), stores: stores, detectorFactory: detectorFactory, + registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, } } -// Process holds after a successful nonterminal pass. Errors are returned for -// normal queue retry or DLQ handling; partial writes are safe to retry. +// Process persists before publication and holds while further analysis is needed. +// Errors use normal queue retry or DLQ handling; partial writes are safe to retry. func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (retErr error) { op := metrics.Begin(c.metricsScope, "process", metrics.LongLatencyBuckets, metrics.TagsFromContext(ctx)...) defer func() { op.Complete(retErr) }() @@ -85,6 +89,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if analysis.GetId() == "" { return fmt.Errorf("build analysis requires a build ID") } + if msg.PartitionKey != analysis.GetId() { + return fmt.Errorf("build analysis must be partitioned by build %q", analysis.GetId()) + } store, err := c.stores.For(storage.Config{QueueName: analysis.GetQueueName()}) if err != nil { @@ -104,6 +111,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if request.Queue != analysis.GetQueueName() { return fmt.Errorf("request queue %q does not match analysis queue %q", request.Queue, analysis.GetQueueName()) } + if request.TerminalBuildID != "" && request.TerminalBuildID != build.ID { + return nil + } // The Build may finish between the initial Build read and the Request read. // Recheck its status so stopping here does not skip final analysis. @@ -137,9 +147,29 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r } } - // A nonterminal status read before detection requires another pass, - // even if the build finishes while detection is running. - if !build.Status.IsTerminal() { + hold := !build.Status.IsTerminal() + finalize := false + if build.Status.IsTerminal() { + // Build status can become terminal before buildsignal records the winning build. + request, err = loader.ByID(ctx, build.RequestID, store.GetRequestStore().Get, "request") + if err != nil { + return err + } + if request.Queue != analysis.GetQueueName() { + return fmt.Errorf("request queue %q does not match analysis queue %q", request.Queue, analysis.GetQueueName()) + } + if request.TerminalBuildID != "" && request.TerminalBuildID != build.ID { + return nil + } + finalize = request.TerminalBuildID == build.ID + hold = !finalize && !request.State.IsTerminal() + } + + if err := c.publishIssueTracking(ctx, request.Queue, build.ID, finalize); err != nil { + return err + } + + if hold { delivery.Hold(PollDelayMs) } @@ -153,6 +183,27 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return nil } +func (c *Controller) publishIssueTracking(ctx context.Context, queue, buildID string, finalize bool) error { + payload, err := stovepipemq.Marshal(&stovepipemq.IssueTrack{ + BuildId: buildID, QueueName: queue, Finalize: finalize, + }) + if err != nil { + return fmt.Errorf("serialize issue tracking notification: %w", err) + } + + // Polls reuse one delivery, so no stable occurrence ID distinguishes refreshes. + messageID := publish.UniqueID(buildID) + if finalize { + messageID = publish.IntentID(buildID, "finished") + } + if err := publish.Message(ctx, c.registry, stovepipemq.TopicKeyIssueTrack, publish.MessageParams{ + Tenant: queue, ID: messageID, Payload: payload, PartitionKey: queue, + }); err != nil { + return fmt.Errorf("publish issue tracking notification: %w", err) + } + return nil +} + func (c *Controller) Name() string { return "buildanalysis" } func (c *Controller) TopicKey() consumer.TopicKey { return c.topicKey } diff --git a/stovepipe/controller/buildanalysis/buildanalysis_test.go b/stovepipe/controller/buildanalysis/buildanalysis_test.go index 2cb3376e3..42d9a0ac3 100644 --- a/stovepipe/controller/buildanalysis/buildanalysis_test.go +++ b/stovepipe/controller/buildanalysis/buildanalysis_test.go @@ -23,9 +23,12 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" "github.com/uber/submitqueue/platform/errs" + 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/entity" "github.com/uber/submitqueue/stovepipe/extension/buildissue" @@ -48,9 +51,11 @@ type analysisMocks struct { store *storagemock.MockStorage builds *storagemock.MockBuildStore requests *storagemock.MockRequestStore + requestRead *gomock.Call issues *storagemock.MockBuildIssueStore detectorFactory *buildissuemock.MockDetectorFactory detector *buildissuemock.MockDetector + publisher *mqmock.MockPublisher scope tally.TestScope } @@ -68,15 +73,23 @@ func newTestController(t *testing.T) (*Controller, analysisMocks) { issues: storagemock.NewMockBuildIssueStore(ctrl), detectorFactory: buildissuemock.NewMockDetectorFactory(ctrl), detector: buildissuemock.NewMockDetector(ctrl), + publisher: mqmock.NewMockPublisher(ctrl), scope: tally.NewTestScope("test", nil), } m.store.EXPECT().GetBuildStore().Return(m.builds).AnyTimes() m.store.EXPECT().GetRequestStore().Return(m.requests).AnyTimes() - m.requests.EXPECT().Get(gomock.Any(), gomock.Any()).DoAndReturn(func(context.Context, string) (entity.Request, error) { return *m.request, nil }).AnyTimes() + m.requestRead = m.requests.EXPECT().Get(gomock.Any(), gomock.Any()).DoAndReturn(func(context.Context, string) (entity.Request, error) { return *m.request, nil }).AnyTimes() m.store.EXPECT().GetBuildIssueStore().Return(m.issues).AnyTimes() - c := NewController(zap.NewNop().Sugar(), m.scope, m.stores, m.detectorFactory, + queue := mqmock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(m.publisher).AnyTimes() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: stovepipemq.TopicKeyIssueTrack, Name: "issue_track", Queue: queue}, + }) + require.NoError(t, err) + + c := NewController(zap.NewNop().Sugar(), m.scope, m.stores, m.detectorFactory, registry, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") return c, m @@ -102,13 +115,41 @@ func (m analysisMocks) delivery(t *testing.T) *consumermock.MockDelivery { } func (m analysisMocks) expectPass(status entity.BuildStatus, issues []entity.BuildIssue, err error) *gomock.Call { + if status.IsTerminal() { + m.request.TerminalBuildID = testBuildID + } resolve := m.stores.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) load := m.builds.EXPECT().Get(gomock.Any(), testBuildID). - Return(entity.Build{ID: testBuildID, Status: status}, nil).After(resolve) + Return(entity.Build{ID: testBuildID, RequestID: m.request.ID, Status: status}, nil).After(resolve) detector := m.detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(m.detector, nil).After(load) return m.detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(issues, err).After(detector) } +func (m analysisMocks) expectNotification(t *testing.T, finalize bool, after *gomock.Call) *gomock.Call { + t.Helper() + call := m.publisher.EXPECT().Publish(gomock.Any(), "issue_track", gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + assert.Equal(t, testQueue, msg.Tenant) + assert.Equal(t, testQueue, msg.PartitionKey) + assert.Equal(t, testQueue, msg.Metadata[entityqueue.MetadataKeyQueueName]) + notification := &stovepipemq.IssueTrack{} + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, notification)) + assert.Equal(t, testBuildID, notification.GetBuildId()) + assert.Equal(t, testQueue, notification.GetQueueName()) + assert.Equal(t, finalize, notification.GetFinalize()) + if finalize { + assert.Equal(t, publish.IntentID(testBuildID, "finished"), msg.ID) + } else { + assert.NotEmpty(t, msg.ID) + } + return nil + }) + if after != nil { + call.After(after) + } + return call +} + func TestProcessPersistsBeforeHoldingOrCompleting(t *testing.T) { issue := entity.BuildIssue{TaskID: "validation", UnitID: "unit-a"} @@ -128,6 +169,7 @@ func TestProcessPersistsBeforeHoldingOrCompleting(t *testing.T) { if !empty { last = m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(last) } + last = m.expectNotification(t, status.IsTerminal(), last) d := m.delivery(t) if !status.IsTerminal() { @@ -148,6 +190,7 @@ func TestProcessAcceptsRepeatedAndTaskWideObservations(t *testing.T) { last = m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(last) last = m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue). Return(fmt.Errorf("already recorded: %w", storage.ErrAlreadyExists)).After(last) + last = m.expectNotification(t, false, last) d := m.delivery(t) d.EXPECT().Hold(PollDelayMs).After(last) @@ -181,6 +224,7 @@ func TestProcessRetriesPartialPersistenceAndReloadsStatus(t *testing.T) { require.ErrorIs(t, err, writeErr) assert.True(t, errs.IsRetryable(err)) } else { + last = m.expectNotification(t, pass.status.IsTerminal(), last) if !pass.status.IsTerminal() { d.EXPECT().Hold(PollDelayMs).After(last) } @@ -211,10 +255,13 @@ func TestProcessRunsAgainWhenBuildCompletesDuringDetection(t *testing.T) { first := m.delivery(t) first.EXPECT().Hold(PollDelayMs) + m.expectNotification(t, false, nil) require.NoError(t, c.Process(context.Background(), first)) m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil) + m.request.TerminalBuildID = testBuildID + m.expectNotification(t, true, nil) require.NoError(t, c.Process(context.Background(), m.delivery(t))) } @@ -267,7 +314,7 @@ func TestProcessPropagatesDependencyErrors(t *testing.T) { } func TestProcessRejectsInvalidMessagesBeforeResolvingDependencies(t *testing.T) { - for _, kind := range []string{"malformed payload", "empty queue", "empty build ID", "empty tenant", "tenant mismatch", "metadata mismatch"} { + for _, kind := range []string{"malformed payload", "empty queue", "empty build ID", "empty tenant", "tenant mismatch", "metadata mismatch", "partition mismatch"} { t.Run(kind, func(t *testing.T) { c, m := newTestController(t) msg := analysisMessage(t) @@ -284,6 +331,8 @@ func TestProcessRejectsInvalidMessagesBeforeResolvingDependencies(t *testing.T) msg.Tenant = "other/main" case "metadata mismatch": msg.Metadata[entityqueue.MetadataKeyQueueName] = "other/main" + case "partition mismatch": + msg.PartitionKey = testQueue } d := consumermock.NewMockDelivery(m.ctrl) @@ -356,7 +405,8 @@ func TestProcessStillAnalyzesTerminalBuildOfTerminalRequest(t *testing.T) { issue := entity.BuildIssue{TaskID: "validation", UnitID: "unit"} detected := m.expectPass(entity.BuildStatusFailed, []entity.BuildIssue{issue}, nil) - m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(detected) + persisted := m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(detected) + m.expectNotification(t, true, persisted) require.NoError(t, c.Process(context.Background(), m.delivery(t))) } @@ -374,6 +424,7 @@ func TestProcessRejectsRequestFromDifferentQueue(t *testing.T) { func TestCompletionBetweenBuildAndRequestReadStillAnalyzes(t *testing.T) { c, m := newTestController(t) m.request.State = entity.RequestStateFailed + m.request.TerminalBuildID = testBuildID m.stores.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) gomock.InOrder( @@ -384,7 +435,8 @@ func TestCompletionBetweenBuildAndRequestReadStillAnalyzes(t *testing.T) { m.detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(m.detector, nil) issue := entity.BuildIssue{TaskID: "validation", UnitID: "unit"} detected := m.detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{issue}, nil) - m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(detected) + persisted := m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(detected) + m.expectNotification(t, true, persisted) require.NoError(t, c.Process(context.Background(), m.delivery(t))) } @@ -419,6 +471,8 @@ func TestProcessRetriesErrorsAndReloadsStatus(t *testing.T) { assert.True(t, errs.IsRetryable(err)) m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil) + m.request.TerminalBuildID = testBuildID + m.expectNotification(t, true, nil) require.NoError(t, c.Process(context.Background(), m.delivery(t))) } @@ -464,3 +518,132 @@ func TestCancelledAnalysisDoesNotHold(t *testing.T) { }) } } + +func TestProcessRetriesPublicationAfterPersistence(t *testing.T) { + for _, status := range []entity.BuildStatus{entity.BuildStatusRunning, entity.BuildStatusFailed} { + t.Run(string(status), func(t *testing.T) { + c, m := newTestController(t) + issue := entity.BuildIssue{TaskID: "validation", UnitID: "unit"} + publishErr := errs.NewRetryableError(assert.AnError) + var failedMessageID string + + detected := m.expectPass(status, []entity.BuildIssue{issue}, nil) + persisted := m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).Return(nil).After(detected) + m.publisher.EXPECT().Publish(gomock.Any(), "issue_track", gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + failedMessageID = msg.ID + return publishErr + }).After(persisted) + + require.ErrorIs(t, c.Process(context.Background(), m.delivery(t)), publishErr) + + detected = m.expectPass(status, []entity.BuildIssue{issue}, nil) + persisted = m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue). + Return(storage.ErrAlreadyExists).After(detected) + published := m.expectNotification(t, status.IsTerminal(), persisted). + Do(func(_ context.Context, _ string, msg entityqueue.Message) { + if status.IsTerminal() { + assert.Equal(t, failedMessageID, msg.ID) + } else { + assert.NotEqual(t, failedMessageID, msg.ID) + } + }) + d := m.delivery(t) + if !status.IsTerminal() { + d.EXPECT().Hold(PollDelayMs).After(published) + } + + require.NoError(t, c.Process(context.Background(), d)) + }) + } +} + +func TestProcessPublishesDistinctRefreshesFromHeldMessage(t *testing.T) { + c, m := newTestController(t) + var messageIDs []string + + for range 2 { + detected := m.expectPass(entity.BuildStatusRunning, nil, nil) + published := m.expectNotification(t, false, detected). + Do(func(_ context.Context, _ string, msg entityqueue.Message) { + messageIDs = append(messageIDs, msg.ID) + }) + d := m.delivery(t) + d.EXPECT().Hold(PollDelayMs).After(published) + + require.NoError(t, c.Process(context.Background(), d)) + } + + require.Len(t, messageIDs, 2) + assert.NotEqual(t, messageIDs[0], messageIDs[1]) +} + +func TestProcessWaitsForWinningBuildBeforeFinalizing(t *testing.T) { + for _, outcome := range []string{"pending", "recorded during detection", "abandoned", "another build wins"} { + t.Run(outcome, func(t *testing.T) { + c, m := newTestController(t) + detected := m.expectPass(entity.BuildStatusFailed, nil, nil) + m.request.TerminalBuildID = "" + if outcome == "abandoned" { + m.request.State = entity.RequestStateFailed + } + detected.Do(func(context.Context, entity.BuildID) { + switch outcome { + case "recorded during detection": + m.request.TerminalBuildID = testBuildID + m.request.State = entity.RequestStateFailed + case "another build wins": + m.request.TerminalBuildID = "other-build" + m.request.State = entity.RequestStateFailed + } + }) + + d := m.delivery(t) + if outcome != "another build wins" { + published := m.expectNotification(t, outcome == "recorded during detection", detected) + if outcome == "pending" { + d.EXPECT().Hold(PollDelayMs).After(published) + } + } + + require.NoError(t, c.Process(context.Background(), d)) + + if outcome == "pending" { + next := m.expectPass(entity.BuildStatusFailed, nil, nil) + m.request.State = entity.RequestStateFailed + m.expectNotification(t, true, next) + + require.NoError(t, c.Process(context.Background(), m.delivery(t))) + } + }) + } +} + +func TestProcessIgnoresKnownLosingBuildBeforeDetection(t *testing.T) { + c, m := newTestController(t) + m.request.TerminalBuildID = "other-build" + m.stores.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) + m.builds.EXPECT().Get(gomock.Any(), testBuildID). + Return(entity.Build{ID: testBuildID, RequestID: m.request.ID, Status: entity.BuildStatusFailed}, nil) + + require.NoError(t, c.Process(context.Background(), m.delivery(t))) +} + +func TestProcessReturnsOutcomeReloadErrorsWithoutPublishing(t *testing.T) { + c, m := newTestController(t) + m.requestRead.Times(1) + detected := m.expectPass(entity.BuildStatusFailed, nil, nil) + m.requests.EXPECT().Get(gomock.Any(), m.request.ID).Return(entity.Request{}, assert.AnError).After(detected) + + require.ErrorIs(t, c.Process(context.Background(), m.delivery(t)), assert.AnError) +} + +func TestProcessRejectsQueueMismatchOnOutcomeReload(t *testing.T) { + c, m := newTestController(t) + detected := m.expectPass(entity.BuildStatusFailed, nil, nil) + detected.Do(func(context.Context, entity.BuildID) { + m.request.Queue = "other/main" + }) + + require.Error(t, c.Process(context.Background(), m.delivery(t))) +} diff --git a/stovepipe/controller/issuetrack/README.md b/stovepipe/controller/issuetrack/README.md index 062400951..accd48c4e 100644 --- a/stovepipe/controller/issuetrack/README.md +++ b/stovepipe/controller/issuetrack/README.md @@ -12,6 +12,10 @@ After all issue and history writes succeed, the controller saves the last fully Build issues added behind a refresh's cursor are picked up by a later refresh, which starts from the beginning. Before requesting finalization, the producer must successfully detect and persist the build's complete issue set. Finalization uses all issues accumulated for that build, not just the latest detector response. +## Analysis handoff + +Build analysis publishes queue-partitioned refresh and final notifications only after persisting the build issues. Final publication requires successful terminal detection and the request's recorded winning build. Build issues accumulate: an issue reported earlier remains associated with that build even if the last detector response omits it. The reference host registers the publication route but leaves issue-tracking consumption disabled until worker takeover is safe and recovery is implemented. + ## Delivery orchestration Notifications carry a queue, build ID, and finalization flag. They must share a queue partition and consumer group. A delivery resumes pending work before admitting another source. When no pass is pending, request ingest order rejects older sources; an already-finalized request cannot accept more work. A completed owning notification is also ignored on replay. diff --git a/stovepipe/extension/buildissue/README.md b/stovepipe/extension/buildissue/README.md index 691014210..8a54ddf3c 100644 --- a/stovepipe/extension/buildissue/README.md +++ b/stovepipe/extension/buildissue/README.md @@ -1,12 +1,14 @@ # Build issue detection -`Detector` discovers confirmed validation issues from a build. It returns failure observations, not a complete view of build state. +`Detector` discovers confirmed validation issues from a build. It returns build issues, not a complete view of build state. Issue values are defined in the [domain model](../../entity/build_issue.go). A validation task is a logical validation operation. A validation unit is an identifiable piece of work within a task. -Within a validation queue, TaskID and UnitID together identify the validation condition. Repeated observations of the same condition use the same pair across builds. UnitID is empty for a task-wide issue. +Within a validation queue, TaskID and UnitID together identify the validation condition. Repeated reports of the same condition use the same pair across builds. UnitID is empty for a task-wide issue. -Each detection returns issues discoverable at that time and may repeat earlier observations. Detection does not mark issues as handled. An issue's absence from a detection result does not mean it is resolved. +Each detection returns issues discoverable at that time and may repeat or omit previously reported issues. Reported issues remain part of that build's accumulated results; later omissions do not retract them. Detection does not mark issues as handled. + +A successful detection after build completion must discover every remaining reportable issue within the queue's relevant validation scope. Together with previously persisted build issues, this makes the accumulated set complete for final reconciliation. An empty final response means no additional issues, not removal of previously reported issues. Incomplete final detection must return an error rather than report success. Implementations decide when a failure qualifies as a reportable issue. Detectors must support concurrent calls. Dependency failures return plain errors; issue results returned with an error must not be used. diff --git a/stovepipe/extension/buildissue/buildissue.go b/stovepipe/extension/buildissue/buildissue.go index c5fdcd0b9..f4bf9a528 100644 --- a/stovepipe/extension/buildissue/buildissue.go +++ b/stovepipe/extension/buildissue/buildissue.go @@ -24,10 +24,10 @@ import ( "github.com/uber/submitqueue/stovepipe/entity" ) -// Detector discovers confirmed issues without waiting for build completion. -// Implementations own confirmation and retry rules and must be concurrency-safe. -// Detection does not mark issues as handled. -// An issue's absence from a detection result does not mean it is resolved. +// Detector discovers issues retained in a build's accumulated results. +// Calls may repeat or omit earlier issues; absence does not retract them. +// A successful terminal call must discover all remaining reportable issues. +// Implementations must be concurrency-safe and must not consume evidence. // Return plain dependency errors; issue results returned with an error must not be used. type Detector interface { Detect(ctx context.Context, buildID entity.BuildID) ([]entity.BuildIssue, error)