From 4d64c9d95cf45089ba9eafeac155d95a60d1f7a2 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 23 Jul 2026 16:11:57 -0700 Subject: [PATCH] refactor: remove unreachable scored batch and request states MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The dedicated score stage was removed in #435, so nothing transitions a batch into `BatchStateScored` or emits the `RequestStatusScored` request-log status anymore. They are now dead, unreachable states — the speculate controller only kept accepting `BatchStateScored` defensively for a producer that no longer exists. ### What? Removed the `BatchStateScored` and `RequestStatusScored` enum values. - `entity/batch.go`: dropped the const and its entries in `ActiveBatchStates()` and `DependencyBatchStates()`. - `entity/request_log.go`: dropped the `RequestStatusScored` const. - `speculate` controller: `startSpeculation` now switches on `BatchStateCreated` only (dropped the unreachable `BatchStateScored` case); doc comment updated to match. - Tests: dropped the speculate `from_scored` case and switched the `RequestStatusScored` sample-data usages in the request-log tests to `RequestStatusBatched`. ## Test Plan ✅ `make test` — all unit tests pass; no remaining `BatchStateScored` / `RequestStatusScored` references. Not run locally: integration and e2e (need Docker), and `//submitqueue/extension/pusher/git` (pre-existing macOS OpenSSL link issue unrelated to this change) — CI covers these on Linux. --- submitqueue/core/request/log_test.go | 6 +++--- submitqueue/entity/batch.go | 4 ---- submitqueue/entity/request_log.go | 3 --- submitqueue/orchestrator/controller/speculate/speculate.go | 4 ++-- .../orchestrator/controller/speculate/speculate_test.go | 5 ++--- 5 files changed, 7 insertions(+), 15 deletions(-) diff --git a/submitqueue/core/request/log_test.go b/submitqueue/core/request/log_test.go index bda0c81b..39dd65c9 100644 --- a/submitqueue/core/request/log_test.go +++ b/submitqueue/core/request/log_test.go @@ -70,7 +70,7 @@ func TestPublishBatchLogs_Success(t *testing.T) { err := PublishBatchLogs(context.Background(), registry, []string{"req/1", "req/2", "req/3"}, - entity.RequestStatusScored, + entity.RequestStatusBatched, map[string]string{"batch_id": "b/1"}, ) require.NoError(t, err) @@ -101,7 +101,7 @@ func TestPublishBatchLogs_PartialFailure(t *testing.T) { err = PublishBatchLogs(context.Background(), registry, []string{"req/1", "req/2", "req/3"}, - entity.RequestStatusScored, + entity.RequestStatusBatched, map[string]string{"batch_id": "b/1"}, ) require.Error(t, err) @@ -111,7 +111,7 @@ func TestPublishBatchLogs_Empty(t *testing.T) { ctrl := gomock.NewController(t) registry := newTestRegistry(t, ctrl, nil) - err := PublishBatchLogs(context.Background(), registry, nil, entity.RequestStatusScored, nil) + err := PublishBatchLogs(context.Background(), registry, nil, entity.RequestStatusBatched, nil) require.NoError(t, err) } diff --git a/submitqueue/entity/batch.go b/submitqueue/entity/batch.go index d0bce274..9deb5e07 100644 --- a/submitqueue/entity/batch.go +++ b/submitqueue/entity/batch.go @@ -32,8 +32,6 @@ const ( BatchStateSucceeded BatchState = "succeeded" // BatchStateFailed is the terminal state of a batch that has failed. BatchStateFailed BatchState = "failed" - // BatchStateScored is the state of a batch that has been scored for build success probability. - BatchStateScored BatchState = "scored" // BatchStateCancelling is the non-terminal intent state set when a cancel has been requested but the // batch has not yet been transitioned to BatchStateCancelled. A batch in this state may still reach // BatchStateSucceeded or BatchStateFailed if a concurrent merge wins the race (e.g. the push had @@ -75,7 +73,6 @@ func IsBatchStateHalted(s BatchState) bool { func ActiveBatchStates() []BatchState { return []BatchState{ BatchStateCreated, - BatchStateScored, BatchStateSpeculating, BatchStateMerging, BatchStateCancelling, @@ -95,7 +92,6 @@ func ActiveBatchStates() []BatchState { func DependencyBatchStates() []BatchState { return []BatchState{ BatchStateCreated, - BatchStateScored, BatchStateSpeculating, BatchStateMerging, } diff --git a/submitqueue/entity/request_log.go b/submitqueue/entity/request_log.go index 17899c09..4d99b27d 100644 --- a/submitqueue/entity/request_log.go +++ b/submitqueue/entity/request_log.go @@ -53,9 +53,6 @@ const ( // RequestStatusBatched indicates that the request has been included in a new batch and will be sent to speculation. RequestStatusBatched RequestStatus = "batched" - // RequestStatusScored indicates that the batch containing the request has been scored for build success probability. - RequestStatusScored RequestStatus = "scored" - // RequestStatusSpeculating indicates that the request is currently being speculated (e.g., speculative merge/rebase, etc.). RequestStatusSpeculating RequestStatus = "speculating" diff --git a/submitqueue/orchestrator/controller/speculate/speculate.go b/submitqueue/orchestrator/controller/speculate/speculate.go index 2d46978f..0b453808 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate.go +++ b/submitqueue/orchestrator/controller/speculate/speculate.go @@ -36,7 +36,7 @@ import ( // Per invocation, the controller advances the batch one step in the // state machine: // -// - Created or Scored → publish to build, transition to Speculating. +// - Created → publish to build, transition to Speculating. // - Speculating → if all deps are Succeeded, publish to merge and // transition to Merging; otherwise no-op (or fail-fast if a dep is // in a non-succeeding terminal state). @@ -133,7 +133,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } switch batch.State { - case entity.BatchStateCreated, entity.BatchStateScored: + case entity.BatchStateCreated: return c.startSpeculation(ctx, batch) case entity.BatchStateSpeculating: return c.tryFinalize(ctx, batch) diff --git a/submitqueue/orchestrator/controller/speculate/speculate_test.go b/submitqueue/orchestrator/controller/speculate/speculate_test.go index f430f7d5..85168da8 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate_test.go +++ b/submitqueue/orchestrator/controller/speculate/speculate_test.go @@ -103,14 +103,13 @@ func TestNewController(t *testing.T) { var _ consumer.Controller = controller } -// startSpeculation: Created/Scored should publish to build and CAS to Speculating with newVersion = oldVersion+1. +// startSpeculation: Created should publish to build and CAS to Speculating with newVersion = oldVersion+1. func TestController_Process_StartSpeculation(t *testing.T) { tests := []struct { name string state entity.BatchState }{ {name: "from_created", state: entity.BatchStateCreated}, - {name: "from_scored", state: entity.BatchStateScored}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -593,7 +592,7 @@ func TestController_Process_StorageFailure(t *testing.T) { // Publish failure must not advance the batch state. func TestController_Process_PublishFailure(t *testing.T) { ctrl := gomock.NewController(t) - batch := testBatch(entity.BatchStateScored) + batch := testBatch(entity.BatchStateCreated) batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil)