refactor(stovepipe): clarify request log persistence

Author: mnoah1

stovepipe/controller/build/build.go

@@ -145,10 +145,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 		Status:    entity.BuildStatusAccepted,
 		Version:   1,
 	}
-	if err := c.persistBuild(ctx, store, build); err != nil {
+	if err := c.createOrVerifyBuildForRequest(ctx, store, build); err != nil {
 		return err
 	}
-	if err := c.persistBuildTriggered(ctx, store, request, build.ID); err != nil {
+	if err := c.persistBuildTriggeredLog(ctx, store, request, build.ID); err != nil {
 		return err
 	}
 
@@ -165,14 +165,20 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 	return nil
 }
 
-func (c *Controller) persistBuild(ctx context.Context, store storage.Storage, build entity.Build) error {
+func (c *Controller) createOrVerifyBuildForRequest(ctx context.Context, store storage.Storage, build entity.Build) error {
 	buildStore := store.GetBuildStore()
-	if err := buildStore.Create(ctx, build); err == nil {
+	err := buildStore.Create(ctx, build)
+	if err == nil {
 		return nil
-	} else if !errors.Is(err, storage.ErrAlreadyExists) {
+	}
+	if !errors.Is(err, storage.ErrAlreadyExists) {
 		return fmt.Errorf("failed to persist build %s: %w", build.ID, err)
 	}
 
+	// ErrAlreadyExists can mean an earlier delivery already stored this Request's Build,
+	// or another Request received the same runner-assigned build ID first. Only the first
+	// case is a safe retry; continuing after a collision would make BuildSignal update the
+	// wrong Request.
 	stored, err := buildStore.Get(ctx, build.ID)
 	if err != nil {
 		return fmt.Errorf("failed to load existing build %s: %w", build.ID, err)
@@ -183,7 +189,7 @@ func (c *Controller) persistBuild(ctx context.Context, store storage.Storage, bu
 	return nil
 }
 
-func (c *Controller) persistBuildTriggered(ctx context.Context, store storage.Storage, request entity.Request, buildID string) error {
+func (c *Controller) persistBuildTriggeredLog(ctx context.Context, store storage.Storage, request entity.Request, buildID string) error {
 	log := requestlog.NewRequestEventLog(
 		request,
 		entity.RequestEventBuildTriggered,

stovepipe/controller/build/build_test.go

@@ -307,7 +307,7 @@ func TestProcess(t *testing.T) {
 			},
 		},
 		{
-			name: "already exists on create is swallowed and publish still happens",
+			name: "existing build for this request repairs the event and publication",
 			setup: func(m buildMocks) {
 				req := processingRequest(entity.BuildStrategyFull, "")
 				m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(req, nil)
@@ -321,6 +321,20 @@ func TestProcess(t *testing.T) {
 				m.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()).Return(nil).After(logCall)
 			},
 		},
+		{
+			name:    "existing build for another request is rejected",
+			wantErr: true,
+			setup: func(m buildMocks) {
+				req := processingRequest(entity.BuildStrategyFull, "")
+				m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(req, 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)
+				createCall := m.buildStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists)
+				m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).
+					Return(entity.Build{ID: testBuildID, RequestID: "request/monorepo/main/8", Status: entity.BuildStatusAccepted, Version: 1}, nil).
+					After(createCall)
+			},
+		},
 		{
 			name:      "build store error is not retryable",
 			wantErr:   true,

stovepipe/controller/buildsignal/buildsignal.go

@@ -170,7 +170,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 	}
 
 	if effective.IsTerminal() {
-		if err := c.persistBuildFinished(ctx, store, request, build.ID); err != nil {
+		if err := c.persistBuildFinishedLog(ctx, store, request, build.ID); err != nil {
 			return err
 		}
 		if err := c.finishRequest(ctx, store, &request, effective); err != nil {
@@ -204,7 +204,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 	return nil
 }
 
-func (c *Controller) persistBuildFinished(ctx context.Context, store storage.Storage, request entity.Request, buildID string) error {
+func (c *Controller) persistBuildFinishedLog(ctx context.Context, store storage.Storage, request entity.Request, buildID string) error {
 	log := requestlog.NewRequestEventLog(
 		request,
 		entity.RequestEventBuildFinished,

stovepipe/controller/record/record.go

@@ -143,7 +143,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 		if err != nil {
 			return err
 		}
-		if err := c.persistValidationFactRecorded(ctx, store, request, fact); err != nil {
+		if err := c.persistValidationFactRecordedLog(ctx, store, request, fact); err != nil {
 			return err
 		}
 		if err := c.applyFactToDerivedCaches(ctx, store, request, fact, created); err != nil {
@@ -172,7 +172,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
 	}
 }
 
-func (c *Controller) persistValidationFactRecorded(
+func (c *Controller) persistValidationFactRecordedLog(
 	ctx context.Context,
 	store storage.Storage,
 	request entity.Request,