diff --git a/submitqueue/entity/batch.go b/submitqueue/entity/batch.go index b360e329..06ca38a5 100644 --- a/submitqueue/entity/batch.go +++ b/submitqueue/entity/batch.go @@ -156,6 +156,13 @@ type Batch struct { Version int32 } +// WithState returns a shallow copy of the batch with State replaced. +// Slice fields continue to share their backing arrays. +func (b Batch) WithState(state BatchState) Batch { + b.State = state + return b +} + // ToBytes serializes the Batch to JSON bytes for queue message payload. func (b Batch) ToBytes() ([]byte, error) { return json.Marshal(b) diff --git a/submitqueue/entity/batch_test.go b/submitqueue/entity/batch_test.go index 0afc38f2..030af982 100644 --- a/submitqueue/entity/batch_test.go +++ b/submitqueue/entity/batch_test.go @@ -66,6 +66,25 @@ func TestDependencyBatchStates_ExcludesCreating(t *testing.T) { assert.NotContains(t, DependencyBatchStates(), BatchStateCreating) } +func TestBatch_WithState(t *testing.T) { + batch := Batch{ + ID: "queueA/batch/1", + Queue: "queueA", + State: BatchStateCreated, + Version: 1, + } + + updated := batch.WithState(BatchStateSpeculating) + + assert.Equal(t, BatchStateCreated, batch.State) + assert.Equal(t, Batch{ + ID: batch.ID, + Queue: batch.Queue, + State: BatchStateSpeculating, + Version: batch.Version, + }, updated) +} + func TestBatch_SerializationRoundTrip(t *testing.T) { tests := []struct { name string diff --git a/submitqueue/extension/storage/batch_store.go b/submitqueue/extension/storage/batch_store.go index eed17967..5d12a7ac 100644 --- a/submitqueue/extension/storage/batch_store.go +++ b/submitqueue/extension/storage/batch_store.go @@ -31,10 +31,10 @@ type BatchStore interface { // Returns ErrAlreadyExists if a batch with the same ID already exists. Create(ctx context.Context, batch entity.Batch) error - // UpdateState updates the state of a batch to newState and the version to newVersion + // Update replaces every non-key field of a batch and writes newVersion // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. // Version arithmetic is owned by the caller; the store performs a pure conditional write. - UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.BatchState) error + Update(ctx context.Context, batch entity.Batch, oldVersion, newVersion int32) error // GetByQueueAndStates retrieves all batches that belong to the given queue and are in the given states. GetByQueueAndStates(ctx context.Context, queue string, states []entity.BatchState) ([]entity.Batch, error) diff --git a/submitqueue/extension/storage/mock/batch_store_mock.go b/submitqueue/extension/storage/mock/batch_store_mock.go index ae571bd1..f4a589de 100644 --- a/submitqueue/extension/storage/mock/batch_store_mock.go +++ b/submitqueue/extension/storage/mock/batch_store_mock.go @@ -85,16 +85,16 @@ func (mr *MockBatchStoreMockRecorder) GetByQueueAndStates(ctx, queue, states any return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByQueueAndStates", reflect.TypeOf((*MockBatchStore)(nil).GetByQueueAndStates), ctx, queue, states) } -// UpdateState mocks base method. -func (m *MockBatchStore) UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.BatchState) error { +// Update mocks base method. +func (m *MockBatchStore) Update(ctx context.Context, batch entity.Batch, oldVersion, newVersion int32) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateState", ctx, id, oldVersion, newVersion, newState) + ret := m.ctrl.Call(m, "Update", ctx, batch, oldVersion, newVersion) ret0, _ := ret[0].(error) return ret0 } -// UpdateState indicates an expected call of UpdateState. -func (mr *MockBatchStoreMockRecorder) UpdateState(ctx, id, oldVersion, newVersion, newState any) *gomock.Call { +// Update indicates an expected call of Update. +func (mr *MockBatchStoreMockRecorder) Update(ctx, batch, oldVersion, newVersion any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateState", reflect.TypeOf((*MockBatchStore)(nil).UpdateState), ctx, id, oldVersion, newVersion, newState) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*MockBatchStore)(nil).Update), ctx, batch, oldVersion, newVersion) } diff --git a/submitqueue/extension/storage/mysql/batch_store.go b/submitqueue/extension/storage/mysql/batch_store.go index 9e8e7254..3aa70584 100644 --- a/submitqueue/extension/storage/mysql/batch_store.go +++ b/submitqueue/extension/storage/mysql/batch_store.go @@ -102,21 +102,31 @@ func (s *batchStore) Create(ctx context.Context, batch entity.Batch) (retErr err return nil } -// UpdateState updates the state of a batch to newState and the version to newVersion +// Update replaces every non-key field of a batch and writes newVersion // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. // Version arithmetic is owned by the caller; this is a pure conditional write. -func (s *batchStore) UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.BatchState) (retErr error) { +func (s *batchStore) Update(ctx context.Context, batch entity.Batch, oldVersion, newVersion int32) (retErr error) { op := metrics.Begin(s.scope, "update_state", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + containsJSON, err := json.Marshal(batch.Contains) + if err != nil { + return fmt.Errorf("failed to marshal contains=%v id=%s for Update batch entity: %w", batch.Contains, batch.ID, err) + } + + dependenciesJSON, err := json.Marshal(batch.Dependencies) + if err != nil { + return fmt.Errorf("failed to marshal dependencies=%v id=%s for Update batch entity: %w", batch.Dependencies, batch.ID, err) + } + result, err := s.db.ExecContext(ctx, - "UPDATE batch SET state = ?, version = ? WHERE id = ? AND version = ?", - newState, newVersion, id, oldVersion, + "UPDATE batch SET queue = ?, contains = ?, dependencies = ?, state = ?, version = ? WHERE id = ? AND version = ?", + batch.Queue, containsJSON, dependenciesJSON, batch.State, newVersion, batch.ID, oldVersion, ) if err != nil { return fmt.Errorf( - "failed to update batch state for id=%q oldVersion=%d newVersion=%d newState=%v: %w", - id, oldVersion, newVersion, newState, err, + "failed to update batch for id=%q oldVersion=%d newVersion=%d newState=%v: %w", + batch.ID, oldVersion, newVersion, batch.State, err, ) } @@ -124,14 +134,14 @@ func (s *batchStore) UpdateState(ctx context.Context, id string, oldVersion, new if err != nil { return fmt.Errorf( "failed to get rows affected from update for id=%q oldVersion=%d newVersion=%d newState=%v: %w", - id, oldVersion, newVersion, newState, err, + batch.ID, oldVersion, newVersion, batch.State, err, ) } if rowsAffected != 1 { return fmt.Errorf( "version mismatch for batch update: id=%q expected_version=%d newState=%v: %w", - id, oldVersion, newState, storage.ErrVersionMismatch, + batch.ID, oldVersion, batch.State, storage.ErrVersionMismatch, ) } diff --git a/submitqueue/extension/storage/mysql/batch_store_test.go b/submitqueue/extension/storage/mysql/batch_store_test.go index 40e74645..28b0b4f6 100644 --- a/submitqueue/extension/storage/mysql/batch_store_test.go +++ b/submitqueue/extension/storage/mysql/batch_store_test.go @@ -198,53 +198,98 @@ func TestBatchStore_Create(t *testing.T) { } } -func TestBatchStore_UpdateState(t *testing.T) { - const id = "monorepo/batch/1" +func TestBatchStore_Update(t *testing.T) { const oldVersion, newVersion = int32(1), int32(2) - const newState = entity.BatchStateMerging + batch := entity.Batch{ + ID: "monorepo/batch/1", + Queue: "monorepo-updated", + Contains: []string{"monorepo/3", "monorepo/4"}, + Dependencies: []string{"monorepo/batch/1", "monorepo/batch/2"}, + State: entity.BatchStateMerging, + Version: oldVersion, + } + containsJSON, err := json.Marshal(batch.Contains) + require.NoError(t, err) + dependenciesJSON, err := json.Marshal(batch.Dependencies) + require.NoError(t, err) tests := []struct { name string + batch entity.Batch setup func(mock sqlmock.Sqlmock) wantErr bool wantErrIs error }{ { - name: "success", + name: "success", + batch: batch, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE batch"). - WithArgs(newState, newVersion, id, oldVersion). + WithArgs(batch.Queue, containsJSON, dependenciesJSON, batch.State, newVersion, batch.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, { - name: "version mismatch", + name: "version mismatch", + batch: batch, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE batch"). - WithArgs(newState, newVersion, id, oldVersion). + WithArgs(batch.Queue, containsJSON, dependenciesJSON, batch.State, newVersion, batch.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, wantErrIs: storage.ErrVersionMismatch, }, { - name: "exec error", + name: "exec error", + batch: batch, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE batch"). - WithArgs(newState, newVersion, id, oldVersion). + WithArgs(batch.Queue, containsJSON, dependenciesJSON, batch.State, newVersion, batch.ID, oldVersion). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, }, { - name: "rows affected error", + name: "rows affected error", + batch: batch, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE batch"). - WithArgs(newState, newVersion, id, oldVersion). + WithArgs(batch.Queue, containsJSON, dependenciesJSON, batch.State, newVersion, batch.ID, oldVersion). WillReturnResult(sqlmock.NewErrorResult(fmt.Errorf("driver error"))) }, wantErr: true, }, + { + name: "nil collections", + batch: entity.Batch{ + ID: batch.ID, + Queue: batch.Queue, + State: batch.State, + Version: batch.Version, + }, + setup: func(mock sqlmock.Sqlmock) { + mock.ExpectExec("UPDATE batch"). + WithArgs(batch.Queue, []byte("null"), []byte("null"), batch.State, newVersion, batch.ID, oldVersion). + WillReturnResult(sqlmock.NewResult(0, 1)) + }, + }, + { + name: "empty collections", + batch: entity.Batch{ + ID: batch.ID, + Queue: batch.Queue, + Contains: []string{}, + Dependencies: []string{}, + State: batch.State, + Version: batch.Version, + }, + setup: func(mock sqlmock.Sqlmock) { + mock.ExpectExec("UPDATE batch"). + WithArgs(batch.Queue, []byte("[]"), []byte("[]"), batch.State, newVersion, batch.ID, oldVersion). + WillReturnResult(sqlmock.NewResult(0, 1)) + }, + }, } for _, tt := range tests { @@ -254,7 +299,7 @@ func TestBatchStore_UpdateState(t *testing.T) { tt.setup(mock) - err := store.UpdateState(context.Background(), id, oldVersion, newVersion, newState) + err := store.Update(context.Background(), tt.batch, oldVersion, newVersion) if tt.wantErr { require.Error(t, err) if tt.wantErrIs != nil { diff --git a/submitqueue/orchestrator/controller/batch/batch.go b/submitqueue/orchestrator/controller/batch/batch.go index ac8de908..8f7e1392 100644 --- a/submitqueue/orchestrator/controller/batch/batch.go +++ b/submitqueue/orchestrator/controller/batch/batch.go @@ -338,13 +338,13 @@ func (c *Controller) populateBatch(ctx context.Context, batch entity.Batch) (ent // The batch's own reverse-index row now exists and every dependency lists this batch as a dependent. // Structural initialization is complete, so transition Creating → Created to make the batch ready for processing once published to speculate. newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateCreated); err != nil { + updated := batch.WithState(entity.BatchStateCreated) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) return entity.Batch{}, fmt.Errorf("failed to mark batch %s created: %w", batch.ID, err) } - batch.Version = newVersion - batch.State = entity.BatchStateCreated - return batch, nil + updated.Version = newVersion + return updated, nil } // publish publishes a batch ID to the specified topic key. diff --git a/submitqueue/orchestrator/controller/batch/batch_test.go b/submitqueue/orchestrator/controller/batch/batch_test.go index e0657556..23b1065a 100644 --- a/submitqueue/orchestrator/controller/batch/batch_test.go +++ b/submitqueue/orchestrator/controller/batch/batch_test.go @@ -86,7 +86,7 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.M mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockBatchStore.EXPECT().UpdateState(gomock.Any(), gomock.Any(), int32(1), int32(2), entity.BatchStateCreated).Return(nil).AnyTimes() + mockBatchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil).AnyTimes() mockReqStore := storagemock.NewMockRequestStore(ctrl) req := testRequest() @@ -173,7 +173,14 @@ func TestController_Process_PublishesBatchedLog(t *testing.T) { mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - mockBatchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/1", int32(1), int32(2), entity.BatchStateCreated).Return(nil) + mockBatchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/1", + Queue: request.Queue, + Contains: []string{request.ID}, + Dependencies: []string{}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) mockReqStore := storagemock.NewMockRequestStore(ctrl) mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) @@ -349,7 +356,14 @@ func TestController_Process_WithDependencies(t *testing.T) { mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), "test-queue", gomock.Any()).Return(activeBatches, nil) mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - mockBatchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/1", int32(1), int32(2), entity.BatchStateCreated).Return(nil) + mockBatchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/1", + Queue: request.Queue, + Contains: []string{request.ID}, + Dependencies: []string{"test-queue/batch/1", "test-queue/batch/2"}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) // batch/1 has no existing dependents. @@ -418,7 +432,14 @@ func TestController_Process_AnalyzerSelectsSubset(t *testing.T) { mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), "test-queue", gomock.Any()).Return(activeBatches, nil) mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - mockBatchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/1", int32(1), int32(2), entity.BatchStateCreated).Return(nil) + mockBatchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/1", + Queue: request.Queue, + Contains: []string{request.ID}, + Dependencies: []string{"test-queue/batch/2"}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) // Only batch/2 is selected by the analyzer, so only it gets a reverse-index update. @@ -712,7 +733,14 @@ func TestController_Process_RecoveryAfterPriorCAS(t *testing.T) { mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), "test-queue", gomock.Any()).Return(nil, nil) mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - mockBatchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/1", int32(1), int32(2), entity.BatchStateCreated).Return(nil) + mockBatchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/1", + Queue: request.Queue, + Contains: []string{request.ID}, + Dependencies: []string{}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) @@ -784,7 +812,7 @@ func TestController_Process_ReadiesBatchBeforePublishing(t *testing.T) { Dependents: []string{}, Version: 1, }).Return(nil), - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCreated).Return(nil), + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCreated), int32(1), int32(2)).Return(nil), publisher.EXPECT().Publish(gomock.Any(), "log", gomock.Any()).Return(nil), publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).Return(nil), ) @@ -846,8 +874,22 @@ func TestController_Process_RedeliveryMintsFreshBatchID(t *testing.T) { return nil }, ).Times(2) - batchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/1", int32(1), int32(2), entity.BatchStateCreated).Return(nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "test-queue/batch/2", int32(1), int32(2), entity.BatchStateCreated).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/1", + Queue: firstRequest.Queue, + Contains: []string{firstRequest.ID}, + Dependencies: []string{}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: "test-queue/batch/2", + Queue: firstRequest.Queue, + Contains: []string{firstRequest.ID}, + Dependencies: []string{}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) batchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) batchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).Times(2) @@ -978,7 +1020,7 @@ func TestController_PopulateBatch_Errors(t *testing.T) { Dependents: []string{"test-queue/batch/old", batch.ID}, Version: 2, }, int32(2), int32(3)).Return(nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCreated).Return(storeErr) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCreated), int32(1), int32(2)).Return(storeErr) }, errMsg: "failed to mark batch", }, diff --git a/submitqueue/orchestrator/controller/cancel/cancel.go b/submitqueue/orchestrator/controller/cancel/cancel.go index 73689fe0..366c3d22 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel.go +++ b/submitqueue/orchestrator/controller/cancel/cancel.go @@ -312,7 +312,8 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error if batch.State != entity.BatchStateCancelling { newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateCancelling); err != nil { + updated := batch.WithState(entity.BatchStateCancelling) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_update_errors", 1) // storage.ErrVersionMismatch here means the batch advanced concurrently // (e.g. speculate / merge progressed). Returned as-is because the @@ -321,8 +322,8 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error // again. return fmt.Errorf("failed to mark batch %s as cancelling: %w", batch.ID, err) } - batch.Version = newVersion - batch.State = entity.BatchStateCancelling + updated.Version = newVersion + batch = updated metrics.NamedCounter(c.metricsScope, opName, "batch_cancelling", 1) } else { metrics.NamedCounter(c.metricsScope, opName, "batch_already_cancelling", 1) diff --git a/submitqueue/orchestrator/controller/cancel/cancel_test.go b/submitqueue/orchestrator/controller/cancel/cancel_test.go index 4e5b97b5..9e08a4d8 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel_test.go +++ b/submitqueue/orchestrator/controller/cancel/cancel_test.go @@ -355,7 +355,7 @@ func TestProcess_BatchPath_HandsOffToSpeculate(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) // Single batch CAS: intent only. No terminal CAS. - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(3), int32(4), entity.BatchStateCancelling).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelling), int32(3), int32(4)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() @@ -385,14 +385,14 @@ func TestProcess_CancelsEveryApplicableBatch(t *testing.T) { var operations []string batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().UpdateState(gomock.Any(), batch1.ID, int32(1), int32(2), entity.BatchStateCancelling).DoAndReturn( - func(context.Context, string, int32, int32, entity.BatchState) error { + batchStore.EXPECT().Update(gomock.Any(), batch1.WithState(entity.BatchStateCancelling), int32(1), int32(2)).DoAndReturn( + func(context.Context, entity.Batch, int32, int32) error { operations = append(operations, "update:"+batch1.ID) return nil }, ) - batchStore.EXPECT().UpdateState(gomock.Any(), batch2.ID, int32(3), int32(4), entity.BatchStateCancelling).DoAndReturn( - func(context.Context, string, int32, int32, entity.BatchState) error { + batchStore.EXPECT().Update(gomock.Any(), batch2.WithState(entity.BatchStateCancelling), int32(3), int32(4)).DoAndReturn( + func(context.Context, entity.Batch, int32, int32) error { operations = append(operations, "update:"+batch2.ID) return nil }, @@ -433,8 +433,8 @@ func TestProcess_BatchFailureDoesNotPreventLaterCancellation(t *testing.T) { requestStore.EXPECT().UpdateState(gomock.Any(), request.ID, int32(2), int32(3), entity.RequestStateCancelling).Return(nil) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().UpdateState(gomock.Any(), batch1.ID, int32(1), int32(2), entity.BatchStateCancelling).Return(storeErr) - batchStore.EXPECT().UpdateState(gomock.Any(), batch2.ID, int32(2), int32(3), entity.BatchStateCancelling).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch1.WithState(entity.BatchStateCancelling), int32(1), int32(2)).Return(storeErr) + batchStore.EXPECT().Update(gomock.Any(), batch2.WithState(entity.BatchStateCancelling), int32(2), int32(3)).Return(nil) publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).DoAndReturn( func(_ context.Context, _ string, msg entityqueue.Message) error { assert.Equal(t, batch2.ID, msg.ID) @@ -574,7 +574,7 @@ func TestProcess_BatchAlreadyCancelling_RepublishesToSpeculate(t *testing.T) { // No request UpdateState — already in Cancelling. batchStore := storagemock.NewMockBatchStore(ctrl) - // No batch UpdateState — already in Cancelling. + // No batch Update — already in Cancelling. store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() @@ -604,7 +604,7 @@ func TestProcess_BatchIntentVersionMismatch_Retryable(t *testing.T) { reqStore.EXPECT().UpdateState(gomock.Any(), "q/1", int32(2), int32(3), entity.RequestStateCancelling).Return(nil) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelling). + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelling), int32(1), int32(2)). Return(storage.ErrVersionMismatch) store := storagemock.NewMockStorage(ctrl) diff --git a/submitqueue/orchestrator/controller/dlq/batch_test.go b/submitqueue/orchestrator/controller/dlq/batch_test.go index 826c8e98..d07add99 100644 --- a/submitqueue/orchestrator/controller/dlq/batch_test.go +++ b/submitqueue/orchestrator/controller/dlq/batch_test.go @@ -43,11 +43,12 @@ func TestDLQBatchController_Process_FailsAndFansOut(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/9").Return(entity.Batch{ + batch := entity.Batch{ ID: "q/batch/9", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateMerging, Version: 2, - }, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "q/batch/9", int32(2), int32(3), entity.BatchStateFailed).Return(nil) + } + batchStore.EXPECT().Get(gomock.Any(), "q/batch/9").Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(2), int32(3)).Return(nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ diff --git a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go index 41da60a2..36ff636c 100644 --- a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go @@ -49,11 +49,12 @@ func TestDLQBuildSignalController_Process_FansOutToBatch(t *testing.T) { }, nil) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/2").Return(entity.Batch{ + batch := entity.Batch{ ID: "q/batch/2", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateSpeculating, Version: 3, - }, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "q/batch/2", int32(3), int32(4), entity.BatchStateFailed).Return(nil) + } + batchStore.EXPECT().Get(gomock.Any(), "q/batch/2").Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(3), int32(4)).Return(nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ diff --git a/submitqueue/orchestrator/controller/dlq/dlq.go b/submitqueue/orchestrator/controller/dlq/dlq.go index 8ccf73e3..fbca21ea 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq.go +++ b/submitqueue/orchestrator/controller/dlq/dlq.go @@ -137,13 +137,16 @@ func failBatch(ctx context.Context, store storage.Storage, registry consumer.Top return nil default: newVersion := batch.Version + 1 - if err := store.GetBatchStore().UpdateState(ctx, batchID, batch.Version, newVersion, entity.BatchStateFailed); err != nil { + updated := batch.WithState(entity.BatchStateFailed) + if err := store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { return fmt.Errorf("failed to update batch %s state to failed: %w", batchID, err) } + updated.Version = newVersion logger.Infow("dlq reconcile: batch marked failed", "batch_id", batchID, "previous_state", string(batch.State), ) + batch = updated } for _, requestID := range batch.Contains { diff --git a/submitqueue/orchestrator/controller/dlq/dlq_test.go b/submitqueue/orchestrator/controller/dlq/dlq_test.go index ad8f6ff3..e7a4ae96 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq_test.go +++ b/submitqueue/orchestrator/controller/dlq/dlq_test.go @@ -177,11 +177,12 @@ func TestFailBatch_TransitionsAndFansOut(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + batch := entity.Batch{ ID: "q/batch/1", Queue: "q", Contains: []string{"q/1", "q/2"}, State: entity.BatchStateMerging, Version: 4, - }, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "q/batch/1", int32(4), int32(5), entity.BatchStateFailed).Return(nil) + } + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(4), int32(5)).Return(nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ @@ -213,7 +214,7 @@ func TestFailBatch_FailedFansOutForRepair(t *testing.T) { ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateFailed, Version: 5, }, nil) - // no batchStore.UpdateState expected + // no batchStore.Update expected requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ @@ -261,11 +262,12 @@ func TestFailBatch_CancellingTransitionsToFailed(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + batch := entity.Batch{ ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateCancelling, Version: 6, - }, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "q/batch/1", int32(6), int32(7), entity.BatchStateFailed).Return(nil) + } + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(6), int32(7)).Return(nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ diff --git a/submitqueue/orchestrator/controller/dlq/mergesignal_test.go b/submitqueue/orchestrator/controller/dlq/mergesignal_test.go index b44257eb..d4016f0e 100644 --- a/submitqueue/orchestrator/controller/dlq/mergesignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/mergesignal_test.go @@ -46,11 +46,12 @@ func TestDLQMergeSignalController_Process_ReconcilesBatch(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + batch := entity.Batch{ ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateMerging, Version: 2, - }, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), "q/batch/1", int32(2), int32(3), entity.BatchStateFailed).Return(nil) + } + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(2), int32(3)).Return(nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{ diff --git a/submitqueue/orchestrator/controller/mergesignal/mergesignal.go b/submitqueue/orchestrator/controller/mergesignal/mergesignal.go index cccff266..e347f8a7 100644 --- a/submitqueue/orchestrator/controller/mergesignal/mergesignal.go +++ b/submitqueue/orchestrator/controller/mergesignal/mergesignal.go @@ -139,12 +139,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, newState); err != nil { + updated := batch.WithState(newState) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "state_update_errors", 1) return fmt.Errorf("failed to transition batch %s to %s: %w", batch.ID, newState, err) } - batch.Version = newVersion - batch.State = newState + updated.Version = newVersion + batch = updated return c.fanout(ctx, batch.ID, batch.Queue) } diff --git a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go index 7ca96911..ac5c2b04 100644 --- a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go @@ -98,9 +98,16 @@ func TestProcess_MergedAdvancesBatch(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), testBatchID).Return( - entity.Batch{ID: testBatchID, Queue: testQueue, State: entity.BatchStateMerging, Version: 1}, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), testBatchID, int32(1), int32(2), entity.BatchStateSucceeded).Return(nil) + batch := entity.Batch{ + ID: testBatchID, + Queue: testQueue, + Contains: []string{"test-queue/1"}, + Dependencies: []string{"test-queue/batch/0"}, + State: entity.BatchStateMerging, + Version: 1, + } + batchStore.EXPECT().Get(gomock.Any(), testBatchID).Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateSucceeded), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -125,9 +132,16 @@ func TestProcess_NotMergedMarksBatchFailed(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), testBatchID).Return( - entity.Batch{ID: testBatchID, Queue: testQueue, State: entity.BatchStateMerging, Version: 3}, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), testBatchID, int32(3), int32(4), entity.BatchStateFailed).Return(nil) + batch := entity.Batch{ + ID: testBatchID, + Queue: testQueue, + Contains: []string{"test-queue/1"}, + Dependencies: []string{"test-queue/batch/0"}, + State: entity.BatchStateMerging, + Version: 3, + } + batchStore.EXPECT().Get(gomock.Any(), testBatchID).Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(3), int32(4)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -153,7 +167,7 @@ func TestProcess_CancellingShortCircuit(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - // No UpdateState and no fan-out: gomock fails if either runs. + // No Update and no fan-out: gomock fails if either runs. var got []string c := newController(t, store, recordingRegistry(t, ctrl, &got)) diff --git a/submitqueue/orchestrator/controller/speculate/speculate.go b/submitqueue/orchestrator/controller/speculate/speculate.go index f027990b..ae8d1cb1 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate.go +++ b/submitqueue/orchestrator/controller/speculate/speculate.go @@ -159,7 +159,8 @@ func (c *Controller) startSpeculation(ctx context.Context, batch entity.Batch) e // Optimistic CAS: if the version has already advanced (concurrent speculate), // the next event will see the new state and behave correctly. newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateSpeculating); err != nil { + updated := batch.WithState(entity.BatchStateSpeculating) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to update batch %s state to speculating: %w", batch.ID, err) } @@ -220,7 +221,8 @@ func (c *Controller) tryFinalize(ctx context.Context, batch entity.Batch) error } newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateMerging); err != nil { + updated := batch.WithState(entity.BatchStateMerging) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to update batch %s state to merging: %w", batch.ID, err) } @@ -242,10 +244,13 @@ func (c *Controller) failOnDependency(ctx context.Context, batch entity.Batch, d ) newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateFailed); err != nil { + updated := batch.WithState(entity.BatchStateFailed) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to update batch %s state to failed: %w", batch.ID, err) } + updated.Version = newVersion + batch = updated if err := c.publish(ctx, topickey.TopicKeyConclude, batch.ID, batch.Queue); err != nil { metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1) @@ -304,12 +309,13 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error } newVersion := batch.Version + 1 - if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateCancelled); err != nil { + updated := batch.WithState(entity.BatchStateCancelled) + if err := c.store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to update batch %s state to cancelled: %w", batch.ID, err) } - batch.Version = newVersion - batch.State = entity.BatchStateCancelled + updated.Version = newVersion + batch = updated if err := c.respeculateDependents(ctx, batch); err != nil { return err diff --git a/submitqueue/orchestrator/controller/speculate/speculate_test.go b/submitqueue/orchestrator/controller/speculate/speculate_test.go index 592a2914..aadc565c 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate_test.go +++ b/submitqueue/orchestrator/controller/speculate/speculate_test.go @@ -46,6 +46,7 @@ func testBatch(state entity.BatchState, deps ...string) entity.Batch { return entity.Batch{ ID: "test-queue/batch/1", Queue: "test-queue", + Contains: []string{"test-queue/1"}, Dependencies: deps, State: state, Version: 1, @@ -118,7 +119,7 @@ func TestController_Process_StartSpeculation(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateSpeculating).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateSpeculating), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -136,7 +137,7 @@ func TestController_Process_FinalizeNoDeps(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateMerging).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateMerging), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -156,7 +157,7 @@ func TestController_Process_FinalizeAllDepsSucceeded(t *testing.T) { batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) batchStore.EXPECT().Get(gomock.Any(), depA.ID).Return(depA, nil) batchStore.EXPECT().Get(gomock.Any(), depB.ID).Return(depB, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateMerging).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateMerging), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -174,7 +175,7 @@ func TestController_Process_WaitingOnDep(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) batchStore.EXPECT().Get(gomock.Any(), dep.ID).Return(dep, nil) - // No UpdateState expected — gomock will fail if it is called. + // No Update expected — gomock will fail if it is called. store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -194,7 +195,7 @@ func TestController_Process_FailedDepFailsBatch(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) batchStore.EXPECT().Get(gomock.Any(), dep.ID).Return(dep, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateFailed).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateFailed), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -216,7 +217,7 @@ func TestController_Process_CancelledDepSkipped(t *testing.T) { batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) batchStore.EXPECT().Get(gomock.Any(), depCancelled.ID).Return(depCancelled, nil) batchStore.EXPECT().Get(gomock.Any(), depSucceeded.ID).Return(depSucceeded, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateMerging).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateMerging), int32(1), int32(2)).Return(nil) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -232,7 +233,7 @@ func TestController_Process_MergingNoOp(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - // No UpdateState expected. + // No Update expected. store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -242,7 +243,7 @@ func TestController_Process_MergingNoOp(t *testing.T) { } // Terminal states re-fan-out to conclude for self-healing in case a previous -// publish was lost. State must not change (no UpdateState). The Cancelled +// publish was lost. State must not change (no Update). The Cancelled // terminal also re-fans-out dependents and is covered separately in // TestController_Process_CancelledTerminalSelfHealsDependents. func TestController_Process_TerminalSelfHeals(t *testing.T) { @@ -256,7 +257,7 @@ func TestController_Process_TerminalSelfHeals(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - // No UpdateState expected. + // No Update expected. store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -285,7 +286,7 @@ func TestController_Process_TerminalSelfHeals(t *testing.T) { // Cancelled is terminal: redelivery must re-fan-out dependents (so a crash // between the terminal CAS and the dependent publish does not strand them) -// AND re-publish to conclude. State must not change (no UpdateState; no +// AND re-publish to conclude. State must not change (no Update; no // build cancel). The BuildStore must not be touched on this self-heal path. func TestController_Process_CancelledTerminalSelfHealsDependents(t *testing.T) { ctrl := gomock.NewController(t) @@ -293,7 +294,7 @@ func TestController_Process_CancelledTerminalSelfHealsDependents(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - // No UpdateState expected. + // No Update expected. depStore := storagemock.NewMockBatchDependentStore(ctrl) depStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.BatchDependent{ @@ -355,7 +356,7 @@ func TestController_Process_CancellingTerminalFlow(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelled), int32(1), int32(2)).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) build := entity.Build{ @@ -423,7 +424,7 @@ func TestController_Process_CancellingBuildAlreadyTerminal(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelled), int32(1), int32(2)).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{ @@ -454,7 +455,7 @@ func TestController_Process_CancellingNoBuildYet(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelled), int32(1), int32(2)).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, storage.ErrNotFound) @@ -484,7 +485,7 @@ func TestController_Process_CancellingNoDependents(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelled), int32(1), int32(2)).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, storage.ErrNotFound) @@ -527,7 +528,7 @@ func TestController_Process_CancellingTerminalCASVersionMismatch(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled). + batchStore.EXPECT().Update(gomock.Any(), batch.WithState(entity.BatchStateCancelled), int32(1), int32(2)). Return(storage.ErrVersionMismatch) buildStore := storagemock.NewMockBuildStore(ctrl) @@ -599,7 +600,7 @@ func TestController_Process_PublishFailure(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) - // No UpdateState expected — publish fails before we get there. + // No Update expected — publish fails before we get there. store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index edaa3528..24616b12 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -228,6 +228,64 @@ func (s *StorageContractSuite) TestStorage_BatchDependentUpdate() { assert.Equal(t, int32(3), retrieved.Version) } +func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() { + t := s.T() + ctx := s.ctx + store := s.storage.GetBatchStore() + batch := entity.Batch{ + ID: "batch-update/batch/1", + Queue: "batch-update", + Contains: []string{"batch-update/1"}, + Dependencies: []string{"batch-update/batch/0"}, + State: entity.BatchStateCreated, + Version: 1, + } + require.NoError(t, store.Create(ctx, batch)) + + nilCollections := batch + nilCollections.Queue = "batch-update-nil" + nilCollections.Contains = nil + nilCollections.Dependencies = nil + nilCollections.State = entity.BatchStateSpeculating + require.NoError(t, store.Update(ctx, nilCollections, 1, 2)) + + got, err := store.Get(ctx, batch.ID) + require.NoError(t, err) + assert.Equal(t, "batch-update-nil", got.Queue) + assert.Nil(t, got.Contains) + assert.Nil(t, got.Dependencies) + assert.Equal(t, entity.BatchStateSpeculating, got.State) + assert.Equal(t, int32(2), got.Version) + + emptyCollections := got + emptyCollections.Queue = "batch-update-empty" + emptyCollections.Contains = []string{} + emptyCollections.Dependencies = []string{} + emptyCollections.State = entity.BatchStateMerging + require.NoError(t, store.Update(ctx, emptyCollections, 2, 3)) + + got, err = store.Get(ctx, batch.ID) + require.NoError(t, err) + assert.Equal(t, "batch-update-empty", got.Queue) + assert.NotNil(t, got.Contains) + assert.Empty(t, got.Contains) + assert.NotNil(t, got.Dependencies) + assert.Empty(t, got.Dependencies) + assert.Equal(t, entity.BatchStateMerging, got.State) + assert.Equal(t, int32(3), got.Version) + + stale := got + stale.Queue = "stale-queue" + stale.Contains = []string{"stale/request"} + stale.Dependencies = []string{"stale/batch"} + stale.State = entity.BatchStateFailed + require.ErrorIs(t, store.Update(ctx, stale, 2, 4), storage.ErrVersionMismatch) + + unchanged, err := store.Get(ctx, batch.ID) + require.NoError(t, err) + assert.Equal(t, got, unchanged) +} + // TestStorage_NotFound tests getting a non-existent request func (s *StorageContractSuite) TestStorage_NotFound() { t := s.T()