Skip to content

Commit 503ae22

Browse files
authored
feat(storage): add QueueBatchStateStore and shared batch state helpers (#523)
## Summary ### Why? `BatchStore.GetByQueueAndStates` is the storage contract's only query-by-attribute, backed by `idx_queue_state` — the only secondary index in the entire schema. The storage README's key-value contract flags exactly this shape: a genuinely needed reverse lookup should be a first-class mapping store keyed by the lookup attribute, so any backend (SQL, DynamoDB, Bigtable) can serve it as a primary-key read. This PR adds that store and the shared helpers controllers will migrate onto; `speculate/run.go` already carries a TODO for this replacement. ### What? - `entity.QueueBatchState` — an advisory membership record filing an in-queue batch under a (queue, state) bucket, existing from batch creation until the batch exits the queue (through terminal states, hence not "active"). Also adds `entity.AllBatchStates()` for the future conclude-time sweep. - `storage.QueueBatchStateStore` — `List(queue, state)` / `Put` / `Delete`, all idempotent. The MySQL impl is backed by a new `queue_batch_state` table whose PK (queue, state, batch_id) *is* the lookup: listing a state bucket is a PK-prefix scan, no secondary index. - `submitqueue/core/batch` — the shared protocol primitives: `Transition` (batch CAS, then Put the new-bucket record before Deleting the old one, so a batch always has at least one record), `EnsureRecord` (idempotent repair for redelivery skip branches), and `ListByStates` (scan requested buckets, dedupe, hydrate by key with bounded concurrency, classify by the hydrated authoritative state — a stale record can misplace a batch but never misreport it). - Contract-suite coverage for the new store and a README pointer beside the `ChangeRecord` mapping-store example. Scoped to the foundation only: the store has no callers yet, so there is no runtime behavior change. Follow-ups migrate the six batch-transition sites and the two `GetByQueueAndStates` readers onto the helpers, add conclude-time record deletion, and then remove `GetByQueueAndStates` + `idx_queue_state`. ## Test Plan - ✅ `bazel test //submitqueue/entity:all //submitqueue/core/batch:all //submitqueue/extension/storage/...` - ✅ `bazel test //test/integration/submitqueue/extension/storage/mysql:go_default_test` (real MySQL; includes the new `TestStorage_QueueBatchStateRecordLifecycle` contract case) - ✅ `make tidy` / `make gazelle` / `make fmt` / `make mocks` — all stable (re-running produces no diffs) - Note: `make test` failures in `orchestrator/controller/batch` and `controller/cancel` are pre-existing — they reproduce at clean HEAD db51df4 in a fresh worktree.
1 parent 63f4f56 commit 503ae22

24 files changed

Lines changed: 1174 additions & 24 deletions

MODULE.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ use_repo(
6565
"org_golang_google_grpc_cmd_protoc_gen_go_grpc",
6666
"org_golang_google_protobuf",
6767
"org_golang_x_oauth2",
68+
"org_golang_x_sync",
6869
"org_uber_go_fx",
6970
"org_uber_go_mock",
7071
"org_uber_go_yarpc",

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ require (
1313
go.uber.org/yarpc v1.81.0
1414
go.uber.org/zap v1.27.1
1515
golang.org/x/oauth2 v0.34.0
16+
golang.org/x/sync v0.19.0
1617
google.golang.org/grpc v1.68.1
1718
google.golang.org/protobuf v1.36.10
1819
gopkg.in/yaml.v3 v3.0.1
@@ -46,7 +47,6 @@ require (
4647
golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 // indirect
4748
golang.org/x/mod v0.32.0 // indirect
4849
golang.org/x/net v0.49.0 // indirect
49-
golang.org/x/sync v0.19.0 // indirect
5050
golang.org/x/sys v0.40.0 // indirect
5151
golang.org/x/text v0.34.0 // indirect
5252
golang.org/x/tools v0.41.0 // indirect

submitqueue/core/batch/BUILD.bazel

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "go_default_library",
5+
srcs = [
6+
"list.go",
7+
"transition.go",
8+
],
9+
importpath = "github.com/uber/submitqueue/submitqueue/core/batch",
10+
visibility = ["//visibility:public"],
11+
deps = [
12+
"//submitqueue/entity:go_default_library",
13+
"//submitqueue/extension/storage:go_default_library",
14+
"@org_golang_x_sync//errgroup:go_default_library",
15+
],
16+
)
17+
18+
go_test(
19+
name = "go_default_test",
20+
srcs = [
21+
"list_test.go",
22+
"transition_test.go",
23+
],
24+
embed = [":go_default_library"],
25+
deps = [
26+
"//submitqueue/entity:go_default_library",
27+
"//submitqueue/extension/storage:go_default_library",
28+
"//submitqueue/extension/storage/mock:go_default_library",
29+
"@com_github_stretchr_testify//assert:go_default_library",
30+
"@com_github_stretchr_testify//require:go_default_library",
31+
"@org_uber_go_mock//gomock:go_default_library",
32+
],
33+
)

submitqueue/core/batch/list.go

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package batch
16+
17+
import (
18+
"context"
19+
"fmt"
20+
21+
"golang.org/x/sync/errgroup"
22+
23+
"github.com/uber/submitqueue/submitqueue/entity"
24+
"github.com/uber/submitqueue/submitqueue/extension/storage"
25+
)
26+
27+
// hydrateConcurrency bounds the parallel per-key batch reads a single
28+
// ListByStates call issues while hydrating candidate IDs.
29+
const hydrateConcurrency = 16
30+
31+
// ListByStates returns the queue's batches whose current state is one of the given
32+
// states, read through the queue's membership records: each requested state bucket
33+
// is listed, candidate IDs are deduplicated across buckets, every candidate is
34+
// hydrated by key with bounded concurrency, and the result keeps only batches whose
35+
// hydrated State is in states. Classification always uses the hydrated state — a
36+
// record found in a stale bucket can therefore never misreport a batch, only route
37+
// an extra read. Result order is unspecified.
38+
//
39+
// A candidate ID whose batch does not exist is returned as an error rather than
40+
// skipped: batch rows are never deleted, so a dangling record means the store is
41+
// inconsistent, not that the batch concluded.
42+
func ListByStates(ctx context.Context, store storage.Storage, queue string, states []entity.BatchState) ([]entity.Batch, error) {
43+
wanted := make(map[entity.BatchState]bool, len(states))
44+
seen := make(map[string]bool)
45+
var ids []string
46+
for _, state := range states {
47+
if wanted[state] {
48+
continue
49+
}
50+
wanted[state] = true
51+
52+
records, err := store.GetQueueBatchStateStore().List(ctx, queue, state)
53+
if err != nil {
54+
return nil, fmt.Errorf("failed to list queue batch state records for queue %s state %s: %w", queue, state, err)
55+
}
56+
for _, record := range records {
57+
if seen[record.BatchID] {
58+
continue
59+
}
60+
seen[record.BatchID] = true
61+
ids = append(ids, record.BatchID)
62+
}
63+
}
64+
65+
hydrated := make([]entity.Batch, len(ids))
66+
g, gctx := errgroup.WithContext(ctx)
67+
g.SetLimit(hydrateConcurrency)
68+
for i, id := range ids {
69+
g.Go(func() error {
70+
batch, err := store.GetBatchStore().Get(gctx, id)
71+
if err != nil {
72+
return fmt.Errorf("failed to get batch %s of queue %s: %w", id, queue, err)
73+
}
74+
hydrated[i] = batch
75+
return nil
76+
})
77+
}
78+
if err := g.Wait(); err != nil {
79+
return nil, err
80+
}
81+
82+
var result []entity.Batch
83+
for _, batch := range hydrated {
84+
if wanted[batch.State] {
85+
result = append(result, batch)
86+
}
87+
}
88+
return result, nil
89+
}
Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package batch
16+
17+
import (
18+
"context"
19+
"errors"
20+
"testing"
21+
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
"go.uber.org/mock/gomock"
25+
26+
"github.com/uber/submitqueue/submitqueue/entity"
27+
"github.com/uber/submitqueue/submitqueue/extension/storage"
28+
storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock"
29+
)
30+
31+
const testQueue = "monorepo"
32+
33+
// record builds a QueueBatchState for testQueue.
34+
func record(state entity.BatchState, batchID string) entity.QueueBatchState {
35+
return entity.QueueBatchState{Queue: testQueue, State: state, BatchID: batchID}
36+
}
37+
38+
// batchIn builds a hydrated Batch for testQueue in the given state.
39+
func batchIn(id string, state entity.BatchState) entity.Batch {
40+
return entity.Batch{ID: id, Queue: testQueue, State: state, Version: 1}
41+
}
42+
43+
func TestListByStates(t *testing.T) {
44+
storeErr := errors.New("storage failed")
45+
46+
tests := map[string]struct {
47+
states []entity.BatchState
48+
setup func(*storagemock.MockBatchStore, *storagemock.MockQueueBatchStateStore)
49+
want []entity.Batch
50+
wantErr error
51+
}{
52+
"empty states lists nothing": {
53+
states: nil,
54+
setup: func(*storagemock.MockBatchStore, *storagemock.MockQueueBatchStateStore) {},
55+
},
56+
"hydrates every bucket and dedupes across them": {
57+
states: []entity.BatchState{entity.BatchStateCreated, entity.BatchStateSpeculating},
58+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
59+
// b2 appears in both buckets (mid-move duplicate): it must be hydrated
60+
// and returned exactly once.
61+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
62+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1"), record(entity.BatchStateCreated, "b2")}, nil)
63+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateSpeculating).
64+
Return([]entity.QueueBatchState{record(entity.BatchStateSpeculating, "b2"), record(entity.BatchStateSpeculating, "b3")}, nil)
65+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateCreated), nil)
66+
batchStore.EXPECT().Get(gomock.Any(), "b2").Return(batchIn("b2", entity.BatchStateSpeculating), nil)
67+
batchStore.EXPECT().Get(gomock.Any(), "b3").Return(batchIn("b3", entity.BatchStateSpeculating), nil)
68+
},
69+
want: []entity.Batch{
70+
batchIn("b1", entity.BatchStateCreated),
71+
batchIn("b2", entity.BatchStateSpeculating),
72+
batchIn("b3", entity.BatchStateSpeculating),
73+
},
74+
},
75+
"classifies by hydrated state, not by bucket": {
76+
states: []entity.BatchState{entity.BatchStateCreated},
77+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
78+
// A stale record files b1 under created, but the batch has moved on to
79+
// speculating — a state outside the requested set, so it is dropped.
80+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
81+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil)
82+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateSpeculating), nil)
83+
},
84+
},
85+
"stale bucket still surfaces a batch whose true state is requested": {
86+
states: []entity.BatchState{entity.BatchStateCreated, entity.BatchStateSpeculating},
87+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
88+
// Only a stale created record exists for b1, but its hydrated state is
89+
// speculating — requested, so the batch is returned under its true state.
90+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
91+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil)
92+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateSpeculating).
93+
Return(nil, nil)
94+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateSpeculating), nil)
95+
},
96+
want: []entity.Batch{batchIn("b1", entity.BatchStateSpeculating)},
97+
},
98+
"duplicate input states are listed once": {
99+
states: []entity.BatchState{entity.BatchStateCreated, entity.BatchStateCreated},
100+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
101+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
102+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil).
103+
Times(1)
104+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateCreated), nil)
105+
},
106+
want: []entity.Batch{batchIn("b1", entity.BatchStateCreated)},
107+
},
108+
"list failure surfaces": {
109+
states: []entity.BatchState{entity.BatchStateCreated},
110+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
111+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).Return(nil, storeErr)
112+
},
113+
wantErr: storeErr,
114+
},
115+
"hydrate failure surfaces": {
116+
states: []entity.BatchState{entity.BatchStateCreated},
117+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
118+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
119+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil)
120+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storeErr)
121+
},
122+
wantErr: storeErr,
123+
},
124+
"dangling record is an error, not a skip": {
125+
states: []entity.BatchState{entity.BatchStateCreated},
126+
setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) {
127+
recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).
128+
Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil)
129+
batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storage.WrapNotFound(errors.New("no rows")))
130+
},
131+
wantErr: storage.ErrNotFound,
132+
},
133+
}
134+
135+
for name, tt := range tests {
136+
t.Run(name, func(t *testing.T) {
137+
mockStorage, mockBatchStore, mockRecordStore := testStores(t)
138+
tt.setup(mockBatchStore, mockRecordStore)
139+
140+
got, err := ListByStates(context.Background(), mockStorage, testQueue, tt.states)
141+
if tt.wantErr != nil {
142+
require.Error(t, err)
143+
assert.ErrorIs(t, err, tt.wantErr)
144+
return
145+
}
146+
require.NoError(t, err)
147+
assert.ElementsMatch(t, tt.want, got)
148+
})
149+
}
150+
}
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package batch provides the shared primitives for moving a batch through its
16+
// lifecycle states while keeping the queue's per-state membership records
17+
// (entity.QueueBatchState) in step.
18+
//
19+
// The records are advisory and the Batch entity is authoritative, so the
20+
// primitives follow one protocol:
21+
//
22+
// - A transition CASes the batch first, then files the record under the new
23+
// state before removing the one under the old state, so a batch always has
24+
// at least one record while it is in the queue.
25+
// - A crash between the CAS and the record move is repaired by the pipeline's
26+
// at-least-once redelivery: the retry's "already in target state" branch
27+
// calls EnsureRecord, and every record write is idempotent.
28+
// - Readers treat records as candidate batch IDs only: they hydrate each
29+
// batch by key and classify it by its own State, never by the bucket the
30+
// record was found in, so a stale record can misplace a batch but never
31+
// misreport it.
32+
package batch
33+
34+
import (
35+
"context"
36+
"fmt"
37+
38+
"github.com/uber/submitqueue/submitqueue/entity"
39+
"github.com/uber/submitqueue/submitqueue/extension/storage"
40+
)
41+
42+
// Transition moves a batch to newState: it performs the optimistic-locking CAS on
43+
// the batch (newVersion = Version+1, assigned in memory only after the store write
44+
// succeeds), then re-files the queue's membership record — Put under newState first,
45+
// Delete under the prior state after, so the batch is never without a record. The
46+
// Delete is skipped when the state is unchanged. It returns the batch as last
47+
// successfully written.
48+
//
49+
// A storage.ErrVersionMismatch from the CAS is returned wrapped (errors.Is works),
50+
// with no record writes attempted, so callers keep their existing lost-race
51+
// semantics. Any other non-nil error means the transition may have partially
52+
// applied — the CAS may have committed with the record move incomplete — and the
53+
// caller is expected to let redelivery retry; the retry's already-in-target-state
54+
// branch repairs the record via EnsureRecord.
55+
func Transition(ctx context.Context, store storage.Storage, batch entity.Batch, newState entity.BatchState) (entity.Batch, error) {
56+
oldState := batch.State
57+
newVersion := batch.Version + 1
58+
updated := batch
59+
updated.State = newState
60+
if err := store.GetBatchStore().Update(ctx, updated, batch.Version, newVersion); err != nil {
61+
return batch, fmt.Errorf("failed to update batch %s state to %s: %w", batch.ID, newState, err)
62+
}
63+
updated.Version = newVersion
64+
65+
record := entity.QueueBatchState{Queue: updated.Queue, State: newState, BatchID: updated.ID}
66+
if err := store.GetQueueBatchStateStore().Put(ctx, record); err != nil {
67+
return updated, fmt.Errorf("failed to put queue batch state record for batch %s under state %s: %w", updated.ID, newState, err)
68+
}
69+
if oldState != newState {
70+
if err := store.GetQueueBatchStateStore().Delete(ctx, updated.Queue, oldState, updated.ID); err != nil {
71+
return updated, fmt.Errorf("failed to delete queue batch state record for batch %s under state %s: %w", updated.ID, oldState, err)
72+
}
73+
}
74+
return updated, nil
75+
}
76+
77+
// EnsureRecord idempotently files the batch under its current state bucket. It is
78+
// the repair half of the transition protocol: idempotent redelivery branches that
79+
// skip the CAS because the batch is already in the target state call this instead,
80+
// covering a prior attempt that crashed between the CAS and the record move.
81+
func EnsureRecord(ctx context.Context, store storage.Storage, batch entity.Batch) error {
82+
record := entity.QueueBatchState{Queue: batch.Queue, State: batch.State, BatchID: batch.ID}
83+
if err := store.GetQueueBatchStateStore().Put(ctx, record); err != nil {
84+
return fmt.Errorf("failed to put queue batch state record for batch %s under state %s: %w", batch.ID, batch.State, err)
85+
}
86+
return nil
87+
}

0 commit comments

Comments
 (0)