Skip to content

Commit e591a03

Browse files
behinddwallsJamyDev
authored andcommitted
refactor: remove unreachable scored batch and request states (#437)
## 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.
1 parent 81dfe62 commit e591a03

5 files changed

Lines changed: 7 additions & 15 deletions

File tree

submitqueue/core/request/log_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func TestPublishBatchLogs_Success(t *testing.T) {
7070

7171
err := PublishBatchLogs(context.Background(), registry,
7272
[]string{"req/1", "req/2", "req/3"},
73-
entity.RequestStatusScored,
73+
entity.RequestStatusBatched,
7474
map[string]string{"batch_id": "b/1"},
7575
)
7676
require.NoError(t, err)
@@ -101,7 +101,7 @@ func TestPublishBatchLogs_PartialFailure(t *testing.T) {
101101

102102
err = PublishBatchLogs(context.Background(), registry,
103103
[]string{"req/1", "req/2", "req/3"},
104-
entity.RequestStatusScored,
104+
entity.RequestStatusBatched,
105105
map[string]string{"batch_id": "b/1"},
106106
)
107107
require.Error(t, err)
@@ -111,7 +111,7 @@ func TestPublishBatchLogs_Empty(t *testing.T) {
111111
ctrl := gomock.NewController(t)
112112
registry := newTestRegistry(t, ctrl, nil)
113113

114-
err := PublishBatchLogs(context.Background(), registry, nil, entity.RequestStatusScored, nil)
114+
err := PublishBatchLogs(context.Background(), registry, nil, entity.RequestStatusBatched, nil)
115115
require.NoError(t, err)
116116
}
117117

submitqueue/entity/batch.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,6 @@ const (
3232
BatchStateSucceeded BatchState = "succeeded"
3333
// BatchStateFailed is the terminal state of a batch that has failed.
3434
BatchStateFailed BatchState = "failed"
35-
// BatchStateScored is the state of a batch that has been scored for build success probability.
36-
BatchStateScored BatchState = "scored"
3735
// BatchStateCancelling is the non-terminal intent state set when a cancel has been requested but the
3836
// batch has not yet been transitioned to BatchStateCancelled. A batch in this state may still reach
3937
// BatchStateSucceeded or BatchStateFailed if a concurrent merge wins the race (e.g. the push had
@@ -75,7 +73,6 @@ func IsBatchStateHalted(s BatchState) bool {
7573
func ActiveBatchStates() []BatchState {
7674
return []BatchState{
7775
BatchStateCreated,
78-
BatchStateScored,
7976
BatchStateSpeculating,
8077
BatchStateMerging,
8178
BatchStateCancelling,
@@ -95,7 +92,6 @@ func ActiveBatchStates() []BatchState {
9592
func DependencyBatchStates() []BatchState {
9693
return []BatchState{
9794
BatchStateCreated,
98-
BatchStateScored,
9995
BatchStateSpeculating,
10096
BatchStateMerging,
10197
}

submitqueue/entity/request_log.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,6 @@ const (
5353
// RequestStatusBatched indicates that the request has been included in a new batch and will be sent to speculation.
5454
RequestStatusBatched RequestStatus = "batched"
5555

56-
// RequestStatusScored indicates that the batch containing the request has been scored for build success probability.
57-
RequestStatusScored RequestStatus = "scored"
58-
5956
// RequestStatusSpeculating indicates that the request is currently being speculated (e.g., speculative merge/rebase, etc.).
6057
RequestStatusSpeculating RequestStatus = "speculating"
6158

submitqueue/orchestrator/controller/speculate/speculate.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import (
3636
// Per invocation, the controller advances the batch one step in the
3737
// state machine:
3838
//
39-
// - Created or Scored → publish to build, transition to Speculating.
39+
// - Created → publish to build, transition to Speculating.
4040
// - Speculating → if all deps are Succeeded, publish to merge and
4141
// transition to Merging; otherwise no-op (or fail-fast if a dep is
4242
// in a non-succeeding terminal state).
@@ -133,7 +133,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
133133
}
134134

135135
switch batch.State {
136-
case entity.BatchStateCreated, entity.BatchStateScored:
136+
case entity.BatchStateCreated:
137137
return c.startSpeculation(ctx, batch)
138138
case entity.BatchStateSpeculating:
139139
return c.tryFinalize(ctx, batch)

submitqueue/orchestrator/controller/speculate/speculate_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,13 @@ func TestNewController(t *testing.T) {
103103
var _ consumer.Controller = controller
104104
}
105105

106-
// startSpeculation: Created/Scored should publish to build and CAS to Speculating with newVersion = oldVersion+1.
106+
// startSpeculation: Created should publish to build and CAS to Speculating with newVersion = oldVersion+1.
107107
func TestController_Process_StartSpeculation(t *testing.T) {
108108
tests := []struct {
109109
name string
110110
state entity.BatchState
111111
}{
112112
{name: "from_created", state: entity.BatchStateCreated},
113-
{name: "from_scored", state: entity.BatchStateScored},
114113
}
115114
for _, tt := range tests {
116115
t.Run(tt.name, func(t *testing.T) {
@@ -593,7 +592,7 @@ func TestController_Process_StorageFailure(t *testing.T) {
593592
// Publish failure must not advance the batch state.
594593
func TestController_Process_PublishFailure(t *testing.T) {
595594
ctrl := gomock.NewController(t)
596-
batch := testBatch(entity.BatchStateScored)
595+
batch := testBatch(entity.BatchStateCreated)
597596

598597
batchStore := storagemock.NewMockBatchStore(ctrl)
599598
batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil)

0 commit comments

Comments
 (0)