diff --git a/AGENTS.md b/AGENTS.md index bfede3c5..ab8d48e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,6 +199,7 @@ The message types are generated; the contract package adds only generic `protojs ### Naming Conventions +- **Identifiers must be self-describing**: a function, method, or variable name states *what* it acts on, not just the verb. `associate`, `index`, `claim`, `announce`, `publish` are all questions — associate what, index what, publish where? Name them `associateRequestsWithBatch`, `writeDependentIndexes`, `claimRequestsForBatch`, `publishToSpeculate`. The test: read the name at the call site, with no surrounding context and without opening the definition — if you cannot say what it does to what, it is underspecified. Being a private helper on a controller is not an excuse; that is exactly where bare verbs accumulate. Length is cheap and a reader's second lookup is not, but stop at the point the name stops adding information: `publishToSpeculate` earns its suffix, `publishToSpeculateTopicViaRegistry` does not. This is also the cheapest way to satisfy [Comment Style](#comment-style) rule 1 — a name that answers "what" removes the comment that would have had to. - **Directories**: singular (`mock/`, `entity/`, not `mocks/`, `entities/`) - **Files**: `{method}.go`, `{entity}.go`, `{file}_test.go`, `BUILD.bazel` - **Proto files**: `{service}.proto` @@ -337,7 +338,9 @@ CI runs on every PR and enforces all checks via a `required-checks` gate. **Befo ### Comment Style -Comments carry real weight here — the CAS race-window note in `submitqueue/orchestrator/controller/batch/batch.go` and the constant rationales in `platform/extension/messagequeue/mysql/subscriber.go` are load-bearing documentation. That is exactly why noise is expensive: when most comments restate the code, readers skim past all of them, including the ones that matter. +**The default is no comment.** Most lines need none: the code already says what it does, and a comment that repeats it costs a reader time while teaching them nothing. When a comment does not clear the bar below, delete it rather than shortening it. + +This matters because noise is contagious. A file whose comments narrate its statements trains readers to skim every comment in it, including the one that would have warned them about a race. Aim for the shape of the good example below — a single line recording a decision that is invisible in the code. A hazard that genuinely needs forty lines to explain is a design note filed in the wrong place; see rule 4. ```go // Bad — restates the call on the next line. @@ -349,12 +352,12 @@ rid, err := entity.RequestIDFromBytes(msg.Payload) return fmt.Errorf("failed to resolve storage for queue %q: %w", rid.Queue, err) ``` -1. **Comment the *why*, never the *what*** — the code already says what it does. A comment earns its place by adding what the reader cannot see: an invariant, a race window, a rejected alternative, the reason a constant has the value it has, or a classification decision. -2. **The deletion test** — if the line below were rewritten with different calls but identical behavior, would the comment still be true and still be worth keeping? If it dies with the line, it was narration. Delete it. -3. **Keep doc comments on exported identifiers short and concise** — say what the reader needs and stop. Where the name already carries the meaning (`Name`, `TopicKey`, `ConsumerGroup`, `NewController`), a brief line is plenty. Spend words only on the non-obvious: units, ownership, nil behavior, concurrency safety, error semantics. Don't pad to fill a template — `make lint` is formatting-only and requires nothing here. -4. **Never narrate the change** — no `// Now also handles X`, `// Previously we ...`, `// Changed to ...`, `// New:`. A comment addresses the next person reading the file, who has no idea a diff ever happened. Why a change was made belongs in the commit message; why the *code* is the way it is belongs in the comment, written in the present tense as a standing fact. -5. **No scaffolding comments in tests** — no `// Arrange` / `// Act` / `// Assert`, no `// Setup`, no `// Close queue`, no `// Verify`. The `t.Run` name states the scenario and testify states the assertion. Comment a test only for a non-obvious fixture or to explain why an outcome is the expected one. -6. **Size the comment to the surprise, and hoist the big ones** — length is justified only by a correspondingly deep hazard. Once an explanation is really about a design rather than about a line, move it to the package doc, a `README.md`, or `doc/rfc/` and leave a one-line pointer. Design essays wedged into function bodies go stale silently. +1. **Name the category, or write nothing.** A comment earns its place only by recording one of: an invariant a reader could violate; a race or ordering hazard; an idempotency or deduplication assumption; a rejected alternative and why it fails; the rationale for a magic value; a classification decision the types do not carry (retryable vs not, user vs infra); a contract with another file that cannot be seen from this one. If you cannot say which of those a comment is, there is no comment to write. +2. **Budgets, so the judgement is not yours to make.** Inline comments inside function bodies: at most one comment line per ten lines of code in the file, and no single block longer than five lines. Doc comments on unexported identifiers: at most two lines, and none at all where the name already says it. Doc comments on exported identifiers: at most five lines, spent on contract only — units, nil behavior, concurrency safety, error semantics, idempotency. Package docs are unbounded: that is where design belongs. +3. **These openers are always narration.** `Fetch/Get/Read X`, `Deserialize/Parse X`, `Publish/Send X to Y`, `Persist/Store/Save X`, `Create/Build X`, `Check if X`, `Loop over X`, `Return X`. Each restates the call beneath it. Delete on sight. +4. **One hazard, one home.** Explain a hazard once, at the site that owns it; elsewhere stay silent or point to it in a few words. Once the explanation is about a design rather than a line, move it to the package doc, a `README.md`, or `doc/rfc/` and leave a one-line pointer. Design essays wedged into function bodies go stale silently and are the main way the budgets in rule 2 get blown. +5. **Never narrate the change** — no `// Now also handles X`, `// Previously we ...`, `// Changed to ...`, `// New:`. A comment addresses the next person reading the file, who has no idea a diff ever happened. Why a change was made belongs in the commit message; why the *code* is the way it is belongs in the comment, written in the present tense as a standing fact. +6. **No scaffolding comments in tests** — no `// Arrange` / `// Act` / `// Assert`, no `// Setup`, no `// Close queue`, no `// Verify`. The `t.Run` name states the scenario and testify states the assertion. Comment a test only for a non-obvious fixture or to explain why an outcome is the expected one. 7. **`TODO` needs a subject and a successor** — use the existing forms, `// TODO: ` or `// TODO(topic): `, and only for genuinely deferred work. Never leave a `TODO` describing work you just finished, and never use one to flag uncertainty about your own change — resolve it or raise it in the PR. Entity fields are governed separately: see [Entities](#entities) rules 4 and 7 — every field gets a comment, and that comment describes the data, not the choreography. diff --git a/doc/rfc/submitqueue/extension-contract.md b/doc/rfc/submitqueue/extension-contract.md index e2ebfb0e..81e086e5 100644 --- a/doc/rfc/submitqueue/extension-contract.md +++ b/doc/rfc/submitqueue/extension-contract.md @@ -6,7 +6,7 @@ Design notes for what SubmitQueue's pluggable extensions accept: orchestrator ** Extension input granularity is inconsistent across the pipeline stages (see [workflow.md](workflow.md)). `conflict.Analyzer` takes identity (`entity.Batch`); `scorer`, `changeprovider`, `buildrunner`, `pusher` take controller-resolved `entity.Change`. The split caps what an extension can do: -- `ConflictType` already names `target_overlap`, but a real target-overlap analyzer **cannot be written** — the batch controller hands it identity-level batches (no changed targets) and the contract has nowhere to put them. +- `ConflictType` already names `target_overlap`, but a real target-overlap analyzer **cannot be written** — the dependency-analysis stage hands it identity-level batches (no changed targets) and the contract has nowhere to put them. - `scorer` gets a URIs-only `Change`, so a heuristic scorer **cannot see** lines-changed / file-count. Both unblock with the shape `conflict` already uses: accept identity, resolve internally. @@ -22,7 +22,7 @@ Both unblock with the shape `conflict` already uses: accept identity, resolve in | Stage | Loads | Resolves for the extension | Hands to the extension | |---|---|---|---| | `validate` | `entity.Request` | nothing — `request.Change` is already in hand (the change-store reads here serve duplicate detection) | `request.Change` → `changeprovider` | -| `batch` | `entity.Request` + active `[]entity.Batch` | **nothing** — builds a batch whose `Contains` is `[requestID]` | `entity.Batch`, `[]entity.Batch` → `conflict` | +| `dependency` | `entity.Batch` + active `[]entity.Batch` | **nothing** — the batch it analyzes is already persisted, with `Contains` set to `[requestID]` | `entity.Batch`, `[]entity.Batch` → `conflict` | | `score` | `entity.Batch`, then each `entity.Request` | batch → requests | `request.Change` per request, then multiplies the scores → `scorer` | | `build` | `entity.Batch`, then `collectChanges` | batch → requests → changes, **flattening batch boundaries** | base `[]Change`, head `[]Change` → `buildrunner` | | `merge` | `entity.Batch`, then `collectChanges` | batch → requests → changes | `[]Change` → `pusher` | diff --git a/service/submitqueue/gateway/server/queues.yaml b/service/submitqueue/gateway/server/queues.yaml index a331805f..5e7434d3 100644 --- a/service/submitqueue/gateway/server/queues.yaml +++ b/service/submitqueue/gateway/server/queues.yaml @@ -12,6 +12,11 @@ queues: # serializes behind every in-flight one. That is what lets e2e build a # dependency chain and exercise how a dependent is woken. - name: e2e-chain-queue + # Also baseline-profile queues. Each owns a whole queue so its first batch is + # predictably "/batch/1", which is what lets e2e close a build gate on + # that batch before anything is landed. + - name: e2e-redelivery-queue + - name: e2e-strand-queue # Routes to an analyzer that always errors (conflictfake.FailAlways) so e2e can # exercise the conflict-analysis error path. See newQueueRegistry in the # orchestrator example server. diff --git a/submitqueue/core/batch/BUILD.bazel b/submitqueue/core/batch/BUILD.bazel index ef82b20a..a6459d92 100644 --- a/submitqueue/core/batch/BUILD.bazel +++ b/submitqueue/core/batch/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ + "find.go", "list.go", "transition.go", ], @@ -18,6 +19,7 @@ go_library( go_test( name = "go_default_test", srcs = [ + "find_test.go", "list_test.go", "transition_test.go", ], diff --git a/submitqueue/core/batch/find.go b/submitqueue/core/batch/find.go new file mode 100644 index 00000000..c028a96b --- /dev/null +++ b/submitqueue/core/batch/find.go @@ -0,0 +1,62 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package batch + +import ( + "context" + "errors" + "fmt" + "sort" + + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" +) + +// FindByRequestID resolves every batch attempt associated with a request, +// ordered by batch ID. The batches are independent of each other, but a +// deterministic order stabilizes logs, tests, and first-error selection. +// +// An association whose batch row is missing is skipped and counted in stale +// rather than failing the call: the batch row and the association are separate +// writes, so an attempt that died between them leaves the association behind. +// The count is returned so callers can meter it without re-reading. +// +// Unlike ListByStates, which treats a dangling membership record as store +// corruption, a dangling association is an expected retry artifact. +func FindByRequestID(ctx context.Context, store storage.Storage, requestID string) ([]entity.Batch, int, error) { + associations, err := store.GetRequestBatchStore().GetByRequestID(ctx, requestID) + if err != nil { + return nil, 0, fmt.Errorf("failed to get batch associations for request %s: %w", requestID, err) + } + + stale := 0 + var batches []entity.Batch + for _, association := range associations { + batch, err := store.GetBatchStore().Get(ctx, association.BatchID) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + stale++ + continue + } + return nil, 0, fmt.Errorf("failed to get associated batch %s for request %s: %w", association.BatchID, requestID, err) + } + batches = append(batches, batch) + } + + sort.Slice(batches, func(i, j int) bool { + return batches[i].ID < batches[j].ID + }) + return batches, stale, nil +} diff --git a/submitqueue/core/batch/find_test.go b/submitqueue/core/batch/find_test.go new file mode 100644 index 00000000..4a0c52ab --- /dev/null +++ b/submitqueue/core/batch/find_test.go @@ -0,0 +1,119 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package batch + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" + storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" +) + +const testRequestID = "monorepo/4" + +// association builds a RequestBatch linking testRequestID to a batch. +func association(batchID string) entity.RequestBatch { + return entity.RequestBatch{RequestID: testRequestID, BatchID: batchID, Version: 1} +} + +// findStores wires a MockStorage over a batch store and a request-batch store. +func findStores(t *testing.T) (*storagemock.MockStorage, *storagemock.MockBatchStore, *storagemock.MockRequestBatchStore) { + t.Helper() + + ctrl := gomock.NewController(t) + mockStorage := storagemock.NewMockStorage(ctrl) + mockBatchStore := storagemock.NewMockBatchStore(ctrl) + mockAssociationStore := storagemock.NewMockRequestBatchStore(ctrl) + mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() + mockStorage.EXPECT().GetRequestBatchStore().Return(mockAssociationStore).AnyTimes() + return mockStorage, mockBatchStore, mockAssociationStore +} + +func TestFindByRequestID(t *testing.T) { + storeErr := errors.New("storage failed") + + tests := map[string]struct { + setup func(*storagemock.MockBatchStore, *storagemock.MockRequestBatchStore) + want []entity.Batch + wantStale int + wantErr error + }{ + "no associations": { + setup: func(_ *storagemock.MockBatchStore, associations *storagemock.MockRequestBatchStore) { + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return(nil, nil) + }, + }, + "hydrates every association in batch id order": { + setup: func(batchStore *storagemock.MockBatchStore, associations *storagemock.MockRequestBatchStore) { + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID). + Return([]entity.RequestBatch{association("b3"), association("b1")}, nil) + batchStore.EXPECT().Get(gomock.Any(), "b3").Return(batchIn("b3", entity.BatchStateCreated), nil) + batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateSucceeded), nil) + }, + want: []entity.Batch{ + batchIn("b1", entity.BatchStateSucceeded), + batchIn("b3", entity.BatchStateCreated), + }, + }, + "missing batch is skipped and counted": { + setup: func(batchStore *storagemock.MockBatchStore, associations *storagemock.MockRequestBatchStore) { + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID). + Return([]entity.RequestBatch{association("b1"), association("b2")}, nil) + batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storage.WrapNotFound(errors.New("no rows"))) + batchStore.EXPECT().Get(gomock.Any(), "b2").Return(batchIn("b2", entity.BatchStateCreating), nil) + }, + want: []entity.Batch{batchIn("b2", entity.BatchStateCreating)}, + wantStale: 1, + }, + "association read failure surfaces": { + setup: func(_ *storagemock.MockBatchStore, associations *storagemock.MockRequestBatchStore) { + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return(nil, storeErr) + }, + wantErr: storeErr, + }, + "batch read failure surfaces": { + setup: func(batchStore *storagemock.MockBatchStore, associations *storagemock.MockRequestBatchStore) { + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID). + Return([]entity.RequestBatch{association("b1")}, nil) + batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storeErr) + }, + wantErr: storeErr, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + mockStorage, mockBatchStore, mockAssociationStore := findStores(t) + tt.setup(mockBatchStore, mockAssociationStore) + + got, stale, err := FindByRequestID(context.Background(), mockStorage, testRequestID) + if tt.wantErr != nil { + require.Error(t, err) + assert.ErrorIs(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantStale, stale) + }) + } +} diff --git a/submitqueue/core/topickey/topickey.go b/submitqueue/core/topickey/topickey.go index 28ff0f5b..1950ed26 100644 --- a/submitqueue/core/topickey/topickey.go +++ b/submitqueue/core/topickey/topickey.go @@ -29,6 +29,11 @@ const ( TopicKeyValidate TopicKey = "validate" // TopicKeyBatch is the pipeline stage where validated requests are published for batching. TopicKeyBatch TopicKey = "batch" + // TopicKeyDependencyAnalysis is the pipeline stage where newly created batches are + // published for conflict analysis. Messages must be partitioned by queue: + // analysis reads the queue's dependency-eligible batches, so two batches of + // one queue analyzed concurrently would each miss the other. + TopicKeyDependencyAnalysis TopicKey = "dependency-analysis" // TopicKeySpeculate is the pipeline stage where batches are published for speculation. TopicKeySpeculate TopicKey = "speculate" // TopicKeyBuild is the pipeline stage where speculated batches are published for builds. diff --git a/submitqueue/entity/batch.go b/submitqueue/entity/batch.go index 8975b513..f9b280cb 100644 --- a/submitqueue/entity/batch.go +++ b/submitqueue/entity/batch.go @@ -22,10 +22,12 @@ type BatchState string const ( // BatchStateUnknown is the unreachable state. It is set by default when the structure is initialized. It should never be seen in the system. BatchStateUnknown BatchState = "" - // BatchStateCreating indicates that the batch has been persisted but its dependency reverse indexes may not yet be fully initialized. - // A Creating batch is not eligible to be referenced as a dependency. + // BatchStateCreating indicates that the batch has been persisted but its dependency set and reverse indexes may not yet be complete. + // Dependencies is empty in this state, and a Creating batch is not eligible to be referenced as a dependency. BatchStateCreating BatchState = "creating" - // BatchStateCreated indicates that the batch and its dependency reverse indexes are fully initialized and ready for processing. + // BatchStateCreated indicates that the batch's dependency set and reverse indexes are final and it is ready for processing. + // A Created batch is eligible to be referenced as a dependency, so it is also certain to be admitted: one that could stall + // here would be an unresolvable dependency for every batch created after it. BatchStateCreated BatchState = "created" // BatchStateSpeculating is the state of a batch that is undergoing speculative execution. BatchStateSpeculating BatchState = "speculating" @@ -161,6 +163,8 @@ type Batch struct { // - queueA/batch/1 will be empty // - queueA/batch/2 will contain queueA/batch/1 // - queueA/batch/3 will contain queueA/batch/1 + // + // The list is empty while the batch is Creating and final from Created onwards. Dependencies []string // The state of the batch lifecycle this batch is in. Updateable field with Version for optimistic locking. diff --git a/submitqueue/entity/request.go b/submitqueue/entity/request.go index 52cbb89c..83bd30a5 100644 --- a/submitqueue/entity/request.go +++ b/submitqueue/entity/request.go @@ -31,12 +31,11 @@ const ( RequestStateStarted RequestState = "started" // RequestStateValidated indicates that the request has been validated (duplicate check, merge check etc.) successfully. RequestStateValidated RequestState = "validated" - // RequestStateBatched indicates that the request has been claimed by the batch controller and enrolled in a - // batch. The CAS-write of this state by the batch controller is the serialization point between batch and - // cancel: the batch controller transitions Validated → Batched immediately before persisting the new batch, - // so any concurrent cancel that has already transitioned the request to Cancelling will lose the CAS and - // abandon the batch. From this state forward, the request's terminal outcome is owned by the batch it is - // enrolled in (via conclude), not by the cancel controller's request-only fast path. + // RequestStateBatched indicates that the request is enrolled in a batch whose dependencies have been + // resolved. The CAS-write of this state is the serialization point against cancellation: it lands with + // the batch's promotion out of Creating, so a cancellation that has already moved the request to + // Cancelling wins the race and the batch is abandoned instead. From this state forward the request's + // terminal outcome is owned by the batch it is enrolled in, not by cancellation's request-only fast path. RequestStateBatched RequestState = "batched" // RequestStateProcessing is the state of a land request that is being processed. RequestStateProcessing RequestState = "processing" diff --git a/submitqueue/entity/request_log.go b/submitqueue/entity/request_log.go index aefb841e..eac65c04 100644 --- a/submitqueue/entity/request_log.go +++ b/submitqueue/entity/request_log.go @@ -51,6 +51,9 @@ const ( // RequestStatusValidated indicates that the request has been validated (duplicate check, merge check etc.) successfully. It corresponds to the RequestStateValidated state. RequestStatusValidated RequestStatus = "validated" + // RequestStatusBatching indicates that a batch has been created for the request and is resolving what it must serialize behind. + RequestStatusBatching RequestStatus = "batching" + // RequestStatusBatched indicates that the request has been included in a new batch and will be sent to speculation. RequestStatusBatched RequestStatus = "batched" diff --git a/submitqueue/orchestrator/BUILD.bazel b/submitqueue/orchestrator/BUILD.bazel index 82e60a9d..4d042c2a 100644 --- a/submitqueue/orchestrator/BUILD.bazel +++ b/submitqueue/orchestrator/BUILD.bazel @@ -23,6 +23,7 @@ go_library( "//submitqueue/orchestrator/controller/buildsignal:go_default_library", "//submitqueue/orchestrator/controller/cancel:go_default_library", "//submitqueue/orchestrator/controller/conclude:go_default_library", + "//submitqueue/orchestrator/controller/dependencyanalysis:go_default_library", "//submitqueue/orchestrator/controller/dlq:go_default_library", "//submitqueue/orchestrator/controller/merge:go_default_library", "//submitqueue/orchestrator/controller/mergeconflictsignal:go_default_library", diff --git a/submitqueue/orchestrator/controller/batch/BUILD.bazel b/submitqueue/orchestrator/controller/batch/BUILD.bazel index cf718405..0270e7e8 100644 --- a/submitqueue/orchestrator/controller/batch/BUILD.bazel +++ b/submitqueue/orchestrator/controller/batch/BUILD.bazel @@ -10,11 +10,9 @@ go_library( "//platform/extension/counter:go_default_library", "//platform/metrics:go_default_library", "//platform/publish:go_default_library", - "//submitqueue/core/batch:go_default_library", "//submitqueue/core/request:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/conflict:go_default_library", "//submitqueue/extension/storage:go_default_library", "@com_github_uber_go_tally//:go_default_library", "@org_uber_go_zap//:go_default_library", @@ -36,9 +34,6 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/conflict:go_default_library", - "//submitqueue/extension/conflict/all:go_default_library", - "//submitqueue/extension/conflict/mock:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", diff --git a/submitqueue/orchestrator/controller/batch/batch.go b/submitqueue/orchestrator/controller/batch/batch.go index 0744fa4e..0b60b32d 100644 --- a/submitqueue/orchestrator/controller/batch/batch.go +++ b/submitqueue/orchestrator/controller/batch/batch.go @@ -16,7 +16,6 @@ package batch import ( "context" - "errors" "fmt" "github.com/uber-go/tally" @@ -24,17 +23,15 @@ import ( "github.com/uber/submitqueue/platform/extension/counter" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/platform/publish" - corebatch "github.com/uber/submitqueue/submitqueue/core/batch" corerequest "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/storage" "go.uber.org/zap" ) // Controller handles batch queue messages. -// It consumes validated requests, groups them into batches, and publishes to the speculate stage. +// It consumes validated requests, mints a batch for each, and hands it to the dependency-analysis stage. // Implements consumer.Controller interface for integration with the consumer. type Controller struct { logger *zap.SugaredLogger @@ -42,7 +39,6 @@ type Controller struct { registry consumer.TopicRegistry counters counter.Factory stores storage.Factory - analyzers conflict.Factory topicKey consumer.TopicKey consumerGroup string } @@ -64,7 +60,6 @@ func NewController( registry consumer.TopicRegistry, counters counter.Factory, stores storage.Factory, - analyzers conflict.Factory, topicKey consumer.TopicKey, consumerGroup string, ) *Controller { @@ -74,14 +69,14 @@ func NewController( registry: registry, counters: counters, stores: stores, - analyzers: analyzers, topicKey: topicKey, consumerGroup: consumerGroup, } } // Process processes a batch delivery from the queue. -// Deserializes the request, groups into batch, and publishes to the speculate topic. +// Mints a batch for the request and hands it to the dependency-analysis topic, +// which decides whether that batch is the one that enrols the request. // Returns nil to ack (success), or error to nack (retry). func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) error { msg := delivery.Message() @@ -150,253 +145,74 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to generate batch ID for queue=%s: %w", request.Queue, err) } + // Dependencies stay empty here: what the batch must serialize behind is + // resolved by the dependency-analysis stage, which fills them in as it + // promotes the batch out of Creating. batch := entity.Batch{ - ID: fmt.Sprintf("%s/batch/%d", request.Queue, seq), - Queue: request.Queue, - Contains: []string{request.ID}, - State: entity.BatchStateCreating, - Version: 1, + ID: fmt.Sprintf("%s/batch/%d", request.Queue, seq), + Queue: request.Queue, + Contains: []string{request.ID}, + Dependencies: []string{}, + State: entity.BatchStateCreating, + Version: 1, } - // Get active batches for this queue and ask the conflict analyzer which - // of them the new batch must serialize behind. The dependency set drives - // the speculation graph downstream. The read goes through the queue's - // per-state membership records; classification uses each batch's own - // hydrated state, so a stale record can never misreport a batch. - activeBatches, err := corebatch.ListByStates(ctx, store, entity.DependencyBatchStates()) - if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) - return fmt.Errorf("failed to get active batches for queue=%s: %w", request.Queue, err) - } - - // Dedupe by batch ID since a single (analyzed, in-flight) pair may be - // reported with multiple Conflict entries when different conflict types - // apply; the dependency graph only tracks the relation. - analyzer, err := c.analyzers.For(conflict.Config{QueueName: batch.Queue}) - if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "conflict_analyzer_errors", 1) - return fmt.Errorf("failed to build conflict analyzer for queue=%s: %w", batch.Queue, err) - } - conflicts, err := analyzer.Analyze(ctx, batch, activeBatches) - if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "conflict_analyzer_errors", 1) - return fmt.Errorf("failed to analyze conflicts for batchID=%s: %w", batch.ID, err) - } - - seen := make(map[string]struct{}, len(conflicts)) - conflictingIDs := make([]string, 0, len(conflicts)) - for _, cf := range conflicts { - if _, ok := seen[cf.BatchID]; ok { - continue - } - seen[cf.BatchID] = struct{}{} - conflictingIDs = append(conflictingIDs, cf.BatchID) - } - - batch.Dependencies = conflictingIDs - - // Claim the request for this batch with a CAS-write that transitions the - // request to RequestStateBatched. This CAS is the serialization point - // between the batch controller and the cancel controller — without it, the - // two would race over an empty interleaving and produce an orphan batch - // containing a cancelled request. - // - // Concrete race that this CAS closes (T1..T7 are wall-clock orderings of - // independent batch- and cancel-controller goroutines): - // - // T1 batch.Get(R) → R{State: Validated, Version: 1} - // T2 cancel.Get(R) → R{State: Validated, Version: 1} - // T3 cancel.markCancelling CAS 1→2 → R{State: Cancelling, Version: 2} - // T4 cancel.findActiveBatch(R) → none (batch has not been Created yet) - // T5 cancel.cancelRequest CAS 2→3 → R{State: Cancelled, Version: 3} - // T6 batch.IsRequestStateHalted(R) → false (stale in-memory copy from T1) - // T7 batch.BatchStore.Create(B{[R]}) → orphan batch containing a cancelled R - // - // After T7 the orphan batch flows through speculate → merge → conclude; - // conclude does NOT gate on the source request state when writing the terminal - // state, so it would CAS the request from Cancelled back to Landed, silently - // undoing the user's cancel. - // - // The CAS below collapses that window. Whichever of request.Update(..., - // RequestStateBatched) and cancel.markCancelling(... RequestStateCancelling) - // reaches storage first wins; the loser sees storage.ErrVersionMismatch: - // - If cancel won: this CAS fails. We ack the message (cancel will drive R - // to its terminal state on its own; no batch or reverse-index data has - // been written). - // - If batch won: cancel.markCancelling will fail with ErrVersionMismatch - // on its next attempt, re-fetch R, observe RequestStateBatched, and take - // the batch-cancellation branch (which terminates the whole batch). - // - // Note on re-delivery: a retry of a batch message that already CAS'd R to - // Batched but failed before/after BatchStore.Create lands in this code with - // R already in RequestStateBatched. The top-level IsRequestStateHalted check - // does NOT include Batched (Batched is forward-progress, not halted), so we - // reach here and re-CAS Batched → Batched (a version-only bump). The bump - // keeps the same serialization invariant on every attempt — if cancel sneaks - // in between our Get and this CAS, our version is stale and we abandon, just - // like the first-delivery case. The cost is an extra batch (the previous - // attempt may have already created one) which is tolerated per the comment - // on BatchStore.Create below. - // - // Residual window: a thin race remains between this CAS and BatchStore.Create. - // During that window cancel.findActiveBatch can still observe R in Batched - // with no batch yet persisted, and take the request-only cancel path — which - // then leaves R in Cancelled and the batch we are about to create orphaned. - // Fully closing this requires cancel-side wait/retry when its pre-CAS - // observation was RequestStateBatched; deferred to a follow-up since the - // window is narrow (one storage round-trip) and the user-visible outcome - // (request cancelled) is still correct — the orphan batch just gets - // reconciled by conclude as if it had no requests to act on. - newRequestVersion := request.Version + 1 - request.State = entity.RequestStateBatched - if err := store.GetRequestStore().Update(ctx, request, request.Version, newRequestVersion); err != nil { - // ErrVersionMismatch == cancel (or another writer) advanced R first. Ack - // the message: there is nothing for us to do, and retrying would not help - // since the new state of R is now visible to the cancel pipeline. - if errors.Is(err, storage.ErrVersionMismatch) { - metrics.NamedCounter(c.metricsScope, opName, "request_claim_lost_race", 1) - c.logger.Infow("abandoning batch creation; request advanced concurrently (likely cancel)", - "request_id", request.ID, - "request_version", request.Version, - "unused_batch_id", batch.ID, - ) - return nil - } - metrics.NamedCounter(c.metricsScope, opName, "request_claim_errors", 1) - return fmt.Errorf("failed to claim request %s for batch %s: %w", request.ID, batch.ID, err) - } - request.Version = newRequestVersion - - // Persist the batch before creating references to it. A Creating batch is not eligible for dependency analysis or normal processing. + // Creating is inert: it is in neither ActiveBatchStates nor + // DependencyBatchStates, and nothing is associated with it yet, so a batch + // abandoned here is a row nobody can reach. if err := store.GetBatchStore().Create(ctx, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) return fmt.Errorf("failed to create batch in batch store: %w", err) } - // File the queue's membership record for the new batch so it is - // discoverable by state from its first moment in the queue. - if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { - metrics.NamedCounter(c.metricsScope, opName, "queue_batch_state_errors", 1) - return err - } - - for _, requestID := range batch.Contains { - association := entity.RequestBatch{ - RequestID: requestID, - BatchID: batch.ID, - Version: 1, - } - if err := store.GetRequestBatchStore().Create(ctx, association); err != nil { - metrics.NamedCounter(c.metricsScope, opName, "request_batch_store_errors", 1) - metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_creating", 1) - return fmt.Errorf("failed to associate request %s with batch %s: %w", requestID, batch.ID, err) - } - } - - batch, err = c.populateBatch(ctx, store, batch) - if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_creating", 1) - // Retries intentionally mint a new batch ID. Failures may therefore leave unpublished Creating or Created attempts behind. - // These attempts are inert and can be removed by a future background cleanup job if their volume becomes significant. - return err - } - - c.logger.Infow("batch created", - "batch_id", batch.ID, - "request_id", request.ID, - "queue", request.Queue, - "dependency_count", len(batch.Dependencies), - ) - - // Record the "batched" status in the request log. This status corresponds to - // the RequestStateBatched transition CAS'd above, so it carries the request - // version for reconciliation. No occurrence is passed, which scopes the - // message ID to (requestID, status): a redelivery that creates a fresh batch - // re-emits "batched" with a different batch_id but is deduped to the first - // entry — acceptable, the request is batched either way, and passing the - // batch ID here would instead surface every abandoned attempt as its own - // entry. - logEntry := entity.NewRequestStatusLog(request.Queue, request.ID, entity.RequestStatusBatched, request.Version, "", map[string]string{ + // Reported once the batch row exists, which is what makes it true: a batch is + // being built for this request. Deduped per (request, status), so a + // redelivery that mints another batch re-emits it harmlessly. + logEntry := entity.NewRequestStatusLog(request.Queue, request.ID, entity.RequestStatusBatching, request.Version, "", map[string]string{ "batch_id": batch.ID, }) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID, ""); err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_log_errors", 1) - metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_created", 1) return fmt.Errorf("failed to publish request log for request %s: %w", request.ID, err) } - // Publish to speculate topic for further processing. - // If it fails and the controller retries, a new batch will be created with the new batch ID but the same request ID. - // The downstream logic should be able to handle stale entries by looking at the state of the batch. - if err := c.publish(ctx, topickey.TopicKeySpeculate, batch.ID, batch.Queue); err != nil { + // Hand the batch to dependency analysis, which decides whether this batch is + // the one that enrols the request and, if so, resolves what it must serialize + // behind and promotes it. The batch is durable first: the consumer reloads it + // by ID, so a message that overtook its own write would find nothing. + // + // A failure here leaves an unreachable Creating row and the retry mints + // another. Enrolment is decided downstream, so duplicates here cost storage, + // not correctness. + if err := c.publishToDependencyAnalysis(ctx, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1) - metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_created", 1) - return fmt.Errorf("failed to publish batch ID to speculate topic: %w", err) + metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_creating", 1) + return fmt.Errorf("failed to publish batch ID to dependency-analysis topic: %w", err) } - c.logger.Infow("published batch to speculate topic", + c.logger.Infow("published batch to dependency-analysis topic", "batch_id", batch.ID, - "topic_key", topickey.TopicKeySpeculate, + "request_id", request.ID, + "queue", request.Queue, + "topic_key", topickey.TopicKeyDependencyAnalysis, ) return nil // Success - message will be acked } -// populateBatch creates the reverse-index structure and marks a Creating batch ready for publication. -func (c *Controller) populateBatch(ctx context.Context, store storage.Storage, batch entity.Batch) (entity.Batch, error) { - batchDependent := entity.BatchDependent{ - BatchID: batch.ID, - Dependents: []string{}, - Version: 1, - } - if err := store.GetBatchDependentStore().Create(ctx, batchDependent); err != nil { - metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) - return entity.Batch{}, fmt.Errorf("failed to create batch dependent index for new batchID=%s: %w", batch.ID, err) - } - - for _, dependencyID := range batch.Dependencies { - existing, err := store.GetBatchDependentStore().Get(ctx, dependencyID) - if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) - return entity.Batch{}, fmt.Errorf("failed to get batch dependent for batchID=%s: %w", dependencyID, err) - } - - updated := existing - updated.Dependents = append([]string(nil), existing.Dependents...) - updated.Dependents = append(updated.Dependents, batch.ID) - newVersion := existing.Version + 1 - if err := store.GetBatchDependentStore().Update(ctx, updated, existing.Version, newVersion); err != nil { - metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) - return entity.Batch{}, fmt.Errorf("failed to update batch dependent index for existing batchID=%s and new batchID=%s: %w", dependencyID, batch.ID, err) - } - } - - // 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. - batch, err := corebatch.Transition(ctx, store, batch, entity.BatchStateCreated) - if 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) - } - return batch, nil -} - -// publish announces a batch to the specified topic key, stamped with and -// partitioned by the batch's queue. +// publishToDependencyAnalysis hands the batch to the next stage, partitioned by +// its queue so analysis of one queue stays serial. // -// The message ID is the bare batch ID, with no cause: this is the batch's -// announcement of its own creation, which happens once in its life, so a -// redelivery that re-announces it is meant to be dropped. Every later publish -// about the same batch names its cause and so cannot collide with this row — -// see publish.IntentID. -func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, batchID string, queue string) error { - bid := entity.BatchID{ID: batchID, Queue: queue} - payload, err := bid.ToBytes() +// The message ID is the bare batch ID, with no cause: a batch is handed over +// once in its life, so a redelivery that re-sends it is meant to be dropped. +func (c *Controller) publishToDependencyAnalysis(ctx context.Context, batch entity.Batch) error { + payload, err := entity.BatchID{ID: batch.ID, Queue: batch.Queue}.ToBytes() if err != nil { return fmt.Errorf("failed to serialize batch ID: %w", err) } - if err := publish.Message(ctx, c.registry, key, publish.IntentID(batchID), payload, queue); err != nil { + if err := publish.Message(ctx, c.registry, topickey.TopicKeyDependencyAnalysis, + publish.IntentID(batch.ID), payload, batch.Queue); err != nil { return fmt.Errorf("failed to publish message: %w", err) } diff --git a/submitqueue/orchestrator/controller/batch/batch_test.go b/submitqueue/orchestrator/controller/batch/batch_test.go index e8a1c6e9..68bfc782 100644 --- a/submitqueue/orchestrator/controller/batch/batch_test.go +++ b/submitqueue/orchestrator/controller/batch/batch_test.go @@ -16,7 +16,6 @@ package batch import ( "context" - "errors" "fmt" "sync/atomic" "testing" @@ -34,29 +33,12 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/conflict" - "github.com/uber/submitqueue/submitqueue/extension/conflict/all" - conflictmock "github.com/uber/submitqueue/submitqueue/extension/conflict/mock" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" ) -// analyzerCfg is the per-queue identity handed to the conflict analyzer in -// cases that do not exercise per-queue routing. -var analyzerCfg = conflict.Config{QueueName: "test-queue"} - -func batchWithState(batch entity.Batch, state entity.BatchState) entity.Batch { - batch.State = state - return batch -} - -func requestWithState(request entity.Request, state entity.RequestState) entity.Request { - request.State = state - return request -} - // requestIDPayload serializes a RequestID to JSON bytes for test message payloads. func requestIDPayload(t *testing.T, id string) []byte { payload, err := entity.RequestID{ID: id}.ToBytes() @@ -76,28 +58,6 @@ func newSequentialCounter(ctrl *gomock.Controller) *countermock.MockCounter { return cnt } -// newQueueBatchStateStore returns a QueueBatchStateStore mock that accepts any -// record write and lists the given batches as membership records under their -// current state. Callers hydrating candidates must set up the corresponding -// BatchStore.Get expectations themselves. -func newQueueBatchStateStore(ctrl *gomock.Controller, active ...entity.Batch) *storagemock.MockQueueBatchStateStore { - s := storagemock.NewMockQueueBatchStateStore(ctrl) - s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().List(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, state entity.BatchState) ([]entity.QueueBatchState, error) { - var records []entity.QueueBatchState - for _, b := range active { - if b.State == state { - records = append(records, entity.QueueBatchState{Queue: b.Queue, State: state, BatchID: b.ID}) - } - } - return records, nil - }, - ).AnyTimes() - return s -} - // storageFactoryFor returns a storage.Factory mock that resolves any queue to // the given queue-scoped store aggregate. func storageFactoryFor(ctrl *gomock.Controller, store storage.Storage) *storagemock.MockFactory { @@ -124,345 +84,176 @@ func testRequest() entity.Request { } } +func newTestRegistry(t *testing.T, publisher *queuemock.MockPublisher, ctrl *gomock.Controller) consumer.TopicRegistry { + t.Helper() + + queue := queuemock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(publisher).AnyTimes() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: topickey.TopicKeyDependencyAnalysis, Name: "dependency-analysis", Queue: queue}, + {Key: topickey.TopicKeyLog, Name: "log", Queue: queue}, + }) + require.NoError(t, err) + return registry +} + // newTestController creates a controller with test dependencies. -// If mockStorage is nil, a default MockStorage with an empty batch store is created. -// If analyzer is nil, the "all" conflict analyzer is used (every active batch becomes a dependency). -// speculatePublishErr, if non-nil, is returned only for publishes to the "speculate" topic; the -// log publish (which the controller emits first) always succeeds, so callers exercising the -// speculate publish-failure path are not short-circuited on the earlier log publish. -func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.MockCounter, mockStorage *storagemock.MockStorage, analyzer conflict.Analyzer, speculatePublishErr error) *Controller { +// If mockStorage is nil, a default MockStorage accepting any batch write is created. +// handoffPublishErr, if non-nil, is returned for the hand-off publish. +func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.MockCounter, mockStorage *storagemock.MockStorage, handoffPublishErr error) *Controller { logger := zaptest.NewLogger(t).Sugar() scope := tally.NoopScope if mockStorage == nil { mockBatchStore := storagemock.NewMockBatchStore(ctrl) mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockBatchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil).AnyTimes() - mockReqStore := storagemock.NewMockRequestStore(ctrl) req := testRequest() + mockReqStore := storagemock.NewMockRequestStore(ctrl) mockReqStore.EXPECT().Get(gomock.Any(), req.ID).Return(req, nil).AnyTimes() - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(req, entity.RequestStateBatched), req.Version, req.Version+1).Return(nil).AnyTimes() - - mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() mockStorage = storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() } - if analyzer == nil { - analyzer = all.New(analyzerCfg) - } - mockPub := queuemock.NewMockPublisher(ctrl) - mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, topic string, msg entityqueue.Message) error { - if topic == "speculate" { - return speculatePublishErr - } - return nil - }, - ).AnyTimes() - - mockQ := queuemock.NewMockQueue(ctrl) - mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() + mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(handoffPublishErr).AnyTimes() - registry, err := consumer.NewTopicRegistry( - []consumer.TopicConfig{ - {Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: mockQ}, - {Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ}, - }, - ) - require.NoError(t, err) + return NewController(logger, scope, newTestRegistry(t, mockPub, ctrl), staticCounterFactory{counter: cnt}, + storageFactoryFor(ctrl, mockStorage), topickey.TopicKeyBatch, "orchestrator-batch") +} - analyzerFactory := conflictmock.NewMockFactory(ctrl) - analyzerFactory.EXPECT().For(gomock.Any()).Return(analyzer, nil).AnyTimes() +func newDelivery(t *testing.T, ctrl *gomock.Controller, request entity.Request, payloadQueue string) *consumermock.MockDelivery { + t.Helper() - return NewController(logger, scope, registry, staticCounterFactory{counter: cnt}, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch") + payload := requestIDPayload(t, request.ID) + if payloadQueue != "" { + bytes, err := entity.RequestID{ID: request.ID, Queue: payloadQueue}.ToBytes() + require.NoError(t, err) + payload = bytes + } + msg := entityqueue.NewMessage(request.ID, payload, request.Queue, nil) + delivery := consumermock.NewMockDelivery(ctrl) + delivery.EXPECT().Message().Return(msg).AnyTimes() + delivery.EXPECT().Attempt().Return(1).AnyTimes() + return delivery } func TestNewController(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil, nil) + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil) require.NotNil(t, controller) assert.Equal(t, topickey.TopicKeyBatch, controller.TopicKey()) assert.Equal(t, "orchestrator-batch", controller.ConsumerGroup()) assert.Equal(t, "batch", controller.Name()) + + var _ consumer.Controller = controller } func TestController_Process_Success(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil, nil) - - request := testRequest() - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.NoError(t, err) + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, testRequest(), ""))) } -// TestController_Process_QueueMismatchRejected asserts a payload whose queue -// disagrees with the request's authoritative queue is rejected without -// touching the counter, the batch store, or the publisher. +// A payload whose queue disagrees with the request's authoritative queue is +// rejected without touching the counter or the batch store. func TestController_Process_QueueMismatchRejected(t *testing.T) { ctrl := gomock.NewController(t) request := testRequest() - mockReqStore := storagemock.NewMockRequestStore(ctrl) mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) mockStorage := storagemock.NewMockStorage(ctrl) mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() + mockStorage.EXPECT().GetBatchStore().Return(storagemock.NewMockBatchStore(ctrl)).AnyTimes() // Counter with no EXPECTs — must not be called. - cnt := countermock.NewMockCounter(ctrl) - controller := newTestController(t, ctrl, cnt, mockStorage, nil, fmt.Errorf("should not publish")) - - payload, err := entity.RequestID{ID: request.ID, Queue: "some-other-queue"}.ToBytes() - require.NoError(t, err) - msg := entityqueue.NewMessage(request.ID, payload, request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - require.Error(t, controller.Process(context.Background(), delivery)) + controller := newTestController(t, ctrl, countermock.NewMockCounter(ctrl), mockStorage, nil) + require.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, "some-other-queue"))) } -// TestController_Process_StampsQueueOnSpeculatePayload asserts the batch ID -// published to speculate carries the batch's queue. -func TestController_Process_StampsQueueOnSpeculatePayload(t *testing.T) { +// The batch ID handed on carries the batch's queue, and the message is +// partitioned by queue so analysis of one queue stays serial. +func TestController_Process_StampsQueueOnHandoffPayload(t *testing.T) { ctrl := gomock.NewController(t) request := testRequest() - var speculateMsgs []entityqueue.Message - mockPub := queuemock.NewMockPublisher(ctrl) - mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, topic string, msg entityqueue.Message) error { - if topic == "speculate" { - speculateMsgs = append(speculateMsgs, msg) - } + var handoffs []entityqueue.Message + publisher := queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), "log", gomock.Any()).Return(nil).AnyTimes() + publisher.EXPECT().Publish(gomock.Any(), "dependency-analysis", gomock.Any()).DoAndReturn( + func(_ context.Context, _ string, msg entityqueue.Message) error { + handoffs = append(handoffs, msg) return nil }, - ).AnyTimes() - mockQ := queuemock.NewMockQueue(ctrl) - mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() - - registry, err := consumer.NewTopicRegistry( - []consumer.TopicConfig{ - {Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: mockQ}, - {Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ}, - }, ) - require.NoError(t, err) - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockBatchStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil).AnyTimes() - mockReqStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - analyzerFactory := conflictmock.NewMockFactory(ctrl) - analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(analyzerCfg), nil).AnyTimes() controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, - storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", + zaptest.NewLogger(t).Sugar(), tally.NoopScope, newTestRegistry(t, publisher, ctrl), + staticCounterFactory{counter: newSequentialCounter(ctrl)}, + storageFactoryFor(ctrl, store), topickey.TopicKeyBatch, "orchestrator-batch", ) - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - require.NoError(t, controller.Process(context.Background(), delivery)) - require.Len(t, speculateMsgs, 1) - bid, err := entity.BatchIDFromBytes(speculateMsgs[0].Payload) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) + require.Len(t, handoffs, 1) + bid, err := entity.BatchIDFromBytes(handoffs[0].Payload) require.NoError(t, err) assert.Equal(t, request.Queue, bid.Queue) - assert.Equal(t, request.Queue, speculateMsgs[0].PartitionKey) -} - -// TestController_Process_PublishesBatchedLog asserts the controller emits a -// "batched" request log carrying the request ID, the post-CAS request version, -// and the batch ID it was placed into. -func TestController_Process_PublishesBatchedLog(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).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) - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1).Return(nil) - - mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - // Capture messages published to the log topic. - var logMsgs []entityqueue.Message - mockPub := queuemock.NewMockPublisher(ctrl) - mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, topic string, msg entityqueue.Message) error { - if topic == "log" { - logMsgs = append(logMsgs, msg) - } - return nil - }, - ).AnyTimes() - mockQ := queuemock.NewMockQueue(ctrl) - mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() - - registry, err := consumer.NewTopicRegistry( - []consumer.TopicConfig{ - {Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: mockQ}, - {Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ}, - }, - ) - require.NoError(t, err) - - analyzerFactory := conflictmock.NewMockFactory(ctrl) - analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(analyzerCfg), nil).AnyTimes() - controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, - storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", - ) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - require.NoError(t, controller.Process(context.Background(), delivery)) - - require.Len(t, logMsgs, 1) - logEntry, err := entity.RequestLogFromBytes(logMsgs[0].Payload) - require.NoError(t, err) - assert.Equal(t, request.ID, logEntry.RequestID) - assert.Equal(t, entity.RequestStatusBatched, logEntry.Status) - assert.Equal(t, request.Version+1, logEntry.RequestVersion) - assert.Equal(t, "test-queue/batch/1", logEntry.Metadata["batch_id"]) + assert.Equal(t, request.Queue, handoffs[0].PartitionKey) } func TestController_Process_StorageFailure(t *testing.T) { ctrl := gomock.NewController(t) + request := testRequest() mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), "test-queue/123").Return(entity.Request{}, fmt.Errorf("db connection lost")) + mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(entity.Request{}, fmt.Errorf("db connection lost")) mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() + mockStorage.EXPECT().GetBatchStore().Return(storagemock.NewMockBatchStore(ctrl)).AnyTimes() - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil, nil) - - msg := entityqueue.NewMessage("test-queue/123", requestIDPayload(t, "test-queue/123"), "test-queue", nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - assert.Error(t, err) + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil) + assert.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) } -func TestController_Process_RequestBatchStoreFailure(t *testing.T) { +func TestController_Process_BatchStoreFailure(t *testing.T) { ctrl := gomock.NewController(t) request := testRequest() - storeErr := errors.New("storage failed") - batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - + batchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(fmt.Errorf("storage failed")) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1).Return(nil) - - requestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - requestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(storeErr) store := storagemock.NewMockStorage(ctrl) - store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - store.EXPECT().GetRequestBatchStore().Return(requestBatchStore).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), store, nil, nil) - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - assert.ErrorIs(t, err, storeErr) + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), store, nil) + assert.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) } func TestController_Process_PublishFailure(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil, fmt.Errorf("publish failed")) - - request := testRequest() - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - assert.Error(t, err) + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, fmt.Errorf("publish failed")) + assert.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, testRequest(), ""))) } func TestController_Process_CounterFailure(t *testing.T) { @@ -470,282 +261,13 @@ func TestController_Process_CounterFailure(t *testing.T) { cnt := countermock.NewMockCounter(ctrl) cnt.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(0), fmt.Errorf("counter unavailable")) - controller := newTestController(t, ctrl, cnt, nil, nil, nil) - request := testRequest() - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - assert.Error(t, err) -} - -func TestController_Process_WithDependencies(t *testing.T) { - ctrl := gomock.NewController(t) - - request := entity.Request{ - ID: "test-queue/456", - Queue: "test-queue", - Change: change.Change{URIs: []string{"github://github.example.com/uber/service/pull/789/789abc1234567890abcdef1234567890abcdef12"}}, - LandStrategy: mergestrategy.MergeStrategyRebase, - State: entity.RequestStateStarted, - Version: 1, - } - - // Set up storage with active batches to become dependencies. - activeBatches := []entity.Batch{ - {ID: "test-queue/batch/1", Queue: "test-queue", State: entity.BatchStateCreated, Version: 1}, - {ID: "test-queue/batch/2", Queue: "test-queue", State: entity.BatchStateSpeculating, Version: 2}, - } - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Get(gomock.Any(), "test-queue/batch/1").Return(activeBatches[0], nil) - mockBatchStore.EXPECT().Get(gomock.Any(), "test-queue/batch/2").Return(activeBatches[1], nil) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).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. - mockBatchDependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/1").Return(entity.BatchDependent{ - BatchID: "test-queue/batch/1", - Version: 1, - }, nil) - mockBatchDependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: "test-queue/batch/1", - Dependents: []string{"test-queue/batch/1"}, - Version: 1, - }, int32(1), int32(2)).Return(nil) - // batch/2 already has an existing dependent. - mockBatchDependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/2").Return(entity.BatchDependent{ - BatchID: "test-queue/batch/2", - Dependents: []string{"test-queue/batch/99"}, - Version: 2, - }, nil) - mockBatchDependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: "test-queue/batch/2", - Dependents: []string{"test-queue/batch/99", "test-queue/batch/1"}, - Version: 2, - }, int32(2), int32(3)).Return(nil) - // Create empty reverse index for the new batch. - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1).Return(nil) - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, activeBatches...)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.NoError(t, err) + controller := newTestController(t, ctrl, cnt, nil, nil) + assert.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, testRequest(), ""))) } -func TestController_Process_AnalyzerSelectsSubset(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - - // Two active batches in flight; analyzer picks only one as a conflict. - activeBatches := []entity.Batch{ - {ID: "test-queue/batch/1", Queue: "test-queue", State: entity.BatchStateCreated, Version: 1}, - {ID: "test-queue/batch/2", Queue: "test-queue", State: entity.BatchStateSpeculating, Version: 2}, - } - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Get(gomock.Any(), "test-queue/batch/1").Return(activeBatches[0], nil) - mockBatchStore.EXPECT().Get(gomock.Any(), "test-queue/batch/2").Return(activeBatches[1], nil) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).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. - mockBatchDependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/2").Return(entity.BatchDependent{ - BatchID: "test-queue/batch/2", - Version: 5, - }, nil) - mockBatchDependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: "test-queue/batch/2", - Dependents: []string{"test-queue/batch/1"}, - Version: 5, - }, int32(5), int32(6)).Return(nil) - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1).Return(nil) - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, activeBatches...)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - // Analyzer returns duplicate Conflict entries for the same batch (different - // conflict types) to prove the controller dedupes by BatchID. - analyzer := conflictmock.NewMockAnalyzer(ctrl) - analyzer.EXPECT().Analyze(gomock.Any(), gomock.Any(), gomock.Any()).Return([]entity.Conflict{ - {BatchID: "test-queue/batch/2", Type: entity.ConflictTypeConservative}, - {BatchID: "test-queue/batch/2", Type: entity.ConflictTypeTargetOverlap}, - }, nil) - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, analyzer, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.NoError(t, err) -} - -func TestController_Process_BatchDependentUpdateFailureDoesNotMutateFetchedDependents(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - activeBatch := entity.Batch{ - ID: "test-queue/batch/99", - Queue: "test-queue", - State: entity.BatchStateCreated, - Version: 1, - } - dependents := make([]string, 1, 2) - dependents[0] = "test-queue/batch/98" - existing := entity.BatchDependent{ - BatchID: activeBatch.ID, - Dependents: dependents, - Version: 4, - } - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Get(gomock.Any(), activeBatch.ID).Return(activeBatch, nil) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - mockBatchDependentStore.EXPECT().Get(gomock.Any(), activeBatch.ID).Return(existing, nil) - mockBatchDependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: activeBatch.ID, - Dependents: []string{"test-queue/batch/98", "test-queue/batch/1"}, - Version: existing.Version, - }, existing.Version, existing.Version+1).Return(errors.New("update failed")) - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1).Return(nil) - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, activeBatch)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.Error(t, err) - assert.Equal(t, "", dependents[:cap(dependents)][1]) - assert.Equal(t, int32(4), existing.Version) -} - -func TestController_Process_AnalyzerFailure(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - analyzer := conflictmock.NewMockAnalyzer(ctrl) - analyzer.EXPECT().Analyze(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("analyzer down")) - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, analyzer, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.Error(t, err) -} - -func TestController_InterfaceImplementation(t *testing.T) { - ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), nil, nil, nil) - - var _ consumer.Controller = controller -} - -// A request that is halted (terminal OR Cancelling) must be short-circuited -// before the batch controller queries the batch store, allocates a batch ID, -// CAS-claims the request, or publishes. We verify by configuring a batch -// store and counter with NO EXPECTs (gomock fails on any call), a request -// store that only expects the initial Get (no UpdateState), and a publisher -// that returns a sentinel error if invoked. -// -// Cancelling is non-terminal but must halt forward progress: the cancel -// controller has already recorded the cancellation intent on the request and -// owns the terminal write. Any new batch spawned here would be an orphan -// containing a request that is about to become Cancelled. +// A halted request must never spawn a batch. Cancelling is non-terminal but +// still halts: cancel owns the request's outcome from that point. func TestController_Process_HaltedShortCircuit(t *testing.T) { for _, state := range []entity.RequestState{ entity.RequestStateCancelling, @@ -760,185 +282,25 @@ func TestController_Process_HaltedShortCircuit(t *testing.T) { request.State = state request.Version = 7 - // Batch store with no EXPECTs — must not be queried. - mockBatchStore := storagemock.NewMockBatchStore(ctrl) + // Batch store and counter with no EXPECTs — neither may be touched. mockReqStore := storagemock.NewMockRequestStore(ctrl) mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - // No UpdateState expected — gomock fails if called. mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() + mockStorage.EXPECT().GetBatchStore().Return(storagemock.NewMockBatchStore(ctrl)).AnyTimes() mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - // Counter with no EXPECTs — must not be called. - cnt := countermock.NewMockCounter(ctrl) - - controller := newTestController(t, ctrl, cnt, mockStorage, nil, fmt.Errorf("should not publish")) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() + controller := newTestController(t, ctrl, countermock.NewMockCounter(ctrl), mockStorage, + fmt.Errorf("should not publish")) - require.NoError(t, controller.Process(context.Background(), delivery)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) }) } } -// Race-lost path: the cancel controller's markCancelling CAS landed first, -// so the batch controller's request-claim CAS (Validated → Batched) fails -// with storage.ErrVersionMismatch. The controller must ack the message (the -// cancel pipeline now owns the request) and must NOT call BatchStore.Create -// or publish to the speculate topic. -// -// This test exercises the race where the halted check at the top of Process -// passed against a stale in-memory copy from the initial Get (the cancel -// controller's CAS landed between our Get and our UpdateState). The CAS -// failure is the safety net that prevents an orphan batch in that window. -func TestController_Process_CASLostToCancel(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - // Create must NOT be called — gomock fails if it is. - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update( - gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1, - ).Return(fmt.Errorf("cas: %w", storage.ErrVersionMismatch)) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - // Publisher with no EXPECTs — must not be called. - mockPub := queuemock.NewMockPublisher(ctrl) - mockQ := queuemock.NewMockQueue(ctrl) - mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() - - registry, err := consumer.NewTopicRegistry( - []consumer.TopicConfig{{Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: mockQ}}, - ) - require.NoError(t, err) - - analyzerFactory := conflictmock.NewMockFactory(ctrl) - analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(analyzerCfg), nil).AnyTimes() - controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, - storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", - ) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - require.NoError(t, controller.Process(context.Background(), delivery)) - assert.Equal(t, entity.RequestStateStarted, request.State) - assert.Equal(t, int32(1), request.Version) -} - -// Race-unexpected-error: any CAS failure other than ErrVersionMismatch (e.g. -// transient storage error) must surface as an error so the message is nacked -// for retry. We must NOT call BatchStore.Create on the way out. -func TestController_Process_CASUnexpectedErrorPropagates(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - // Create must NOT be called — gomock fails if it is. - - casErr := fmt.Errorf("db connection lost") - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update( - gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1, - ).Return(casErr) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - err := controller.Process(context.Background(), delivery) - require.Error(t, err) - // Cause must be preserved for upstream classification. - assert.True(t, errors.Is(err, casErr)) - assert.Equal(t, entity.RequestStateStarted, request.State) - assert.Equal(t, int32(1), request.Version) -} - -// Recovery path: a re-delivered batch message whose prior attempt CAS'd the -// request to RequestStateBatched but failed before BatchStore.Create. The -// halted check at the top of Process does NOT include Batched (Batched is -// forward-progress, not halted), so we reach the CAS again and re-bump the -// version on the request (Batched → Batched, version+1). The batch is then -// re-created with a new batch ID, which is tolerated per the existing -// duplicate-handling comment on BatchStore.Create. -func TestController_Process_RecoveryAfterPriorCAS(t *testing.T) { - ctrl := gomock.NewController(t) - - request := testRequest() - request.State = entity.RequestStateBatched - request.Version = 2 // prior attempt bumped from 1 → 2 - - mockBatchStore := storagemock.NewMockBatchStore(ctrl) - mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).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) - - mockReqStore := storagemock.NewMockRequestStore(ctrl) - mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - mockReqStore.EXPECT().Update( - gomock.Any(), requestWithState(request, entity.RequestStateBatched), request.Version, request.Version+1, - ).Return(nil) - - mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - mockRequestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) - - mockStorage := storagemock.NewMockStorage(ctrl) - mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() - mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes() - mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes() - mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), mockStorage, nil, nil) - - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - require.NoError(t, controller.Process(context.Background(), delivery)) -} - -func TestController_Process_ReadiesBatchBeforePublishing(t *testing.T) { +// The batch is durable before it is handed off: the next stage reloads it by +// ID, so a hand-off that overtook its own write would find nothing. +func TestController_Process_WritesBatchBeforeHandoff(t *testing.T) { ctrl := gomock.NewController(t) request := testRequest() @@ -958,253 +320,99 @@ func TestController_Process_ReadiesBatchBeforePublishing(t *testing.T) { requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) batchStore := storagemock.NewMockBatchStore(ctrl) - - requestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - batchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) publisher := queuemock.NewMockPublisher(ctrl) gomock.InOrder( - requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), int32(1), int32(2)).Return(nil), batchStore.EXPECT().Create(gomock.Any(), batch).Return(nil), - requestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: batch.ID, - Version: 1, - }).Return(nil), - batchDependentStore.EXPECT().Create(gomock.Any(), entity.BatchDependent{ - BatchID: batch.ID, - Dependents: []string{}, - Version: 1, - }).Return(nil), - batchStore.EXPECT().Update(gomock.Any(), batchWithState(batch, 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), + publisher.EXPECT().Publish(gomock.Any(), "dependency-analysis", gomock.Any()).Return(nil), ) store := storagemock.NewMockStorage(ctrl) - store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - store.EXPECT().GetBatchDependentStore().Return(batchDependentStore).AnyTimes() - store.EXPECT().GetRequestBatchStore().Return(requestBatchStore).AnyTimes() - queue := queuemock.NewMockQueue(ctrl) - queue.EXPECT().Publisher().Return(publisher).AnyTimes() - registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ - {Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: queue}, - {Key: topickey.TopicKeyLog, Name: "log", Queue: queue}, - }) - require.NoError(t, err) - - analyzerFactory := conflictmock.NewMockFactory(ctrl) - analyzerFactory.EXPECT().For(conflict.Config{QueueName: request.Queue}).Return(all.New(analyzerCfg), nil) controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: cnt}, storageFactoryFor(ctrl, store), analyzerFactory, + zaptest.NewLogger(t).Sugar(), tally.NoopScope, newTestRegistry(t, publisher, ctrl), + staticCounterFactory{counter: cnt}, storageFactoryFor(ctrl, store), topickey.TopicKeyBatch, "orchestrator-batch", ) - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() - - assert.NoError(t, controller.Process(context.Background(), delivery)) + assert.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) } -func TestController_Process_RedeliveryMintsFreshBatchID(t *testing.T) { +// The batch leaves this stage unresolved and unclaimed: dependency analysis +// owns both. +func TestController_Process_CreatesBatchInCreatingWithoutDependencies(t *testing.T) { ctrl := gomock.NewController(t) - firstRequest := testRequest() - secondRequest := firstRequest - secondRequest.State = entity.RequestStateBatched - secondRequest.Version = 2 - - cnt := countermock.NewMockCounter(ctrl) - cnt.EXPECT().Next(gomock.Any(), counterDomainBatch).Return(int64(1), nil) - cnt.EXPECT().Next(gomock.Any(), counterDomainBatch).Return(int64(2), nil) - - requestStore := storagemock.NewMockRequestStore(ctrl) - requestStore.EXPECT().Get(gomock.Any(), firstRequest.ID).Return(firstRequest, nil) - requestStore.EXPECT().Update(gomock.Any(), requestWithState(firstRequest, entity.RequestStateBatched), int32(1), int32(2)).Return(nil) - requestStore.EXPECT().Get(gomock.Any(), firstRequest.ID).Return(secondRequest, nil) - requestStore.EXPECT().Update(gomock.Any(), requestWithState(secondRequest, entity.RequestStateBatched), int32(2), int32(3)).Return(nil) + request := testRequest() - var createdIDs []string + var created []entity.Batch batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( func(_ context.Context, batch entity.Batch) error { - createdIDs = append(createdIDs, batch.ID) - assert.Equal(t, entity.BatchStateCreating, batch.State) + created = append(created, batch) return nil }, - ).Times(2) - 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) - - requestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - requestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: firstRequest.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) - requestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: firstRequest.ID, - BatchID: "test-queue/batch/2", - Version: 1, - }).Return(nil) + ) + + // A request store that only answers Get — a claim here would fail the test. + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) store := storagemock.NewMockStorage(ctrl) - store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - store.EXPECT().GetBatchDependentStore().Return(batchDependentStore).AnyTimes() - store.EXPECT().GetRequestBatchStore().Return(requestBatchStore).AnyTimes() - controller := newTestController(t, ctrl, cnt, store, nil, nil) - msg := entityqueue.NewMessage(firstRequest.ID, requestIDPayload(t, firstRequest.ID), firstRequest.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(2).AnyTimes() + controller := newTestController(t, ctrl, newSequentialCounter(ctrl), store, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) - assert.NoError(t, controller.Process(context.Background(), delivery)) - assert.NoError(t, controller.Process(context.Background(), delivery)) - assert.Equal(t, []string{"test-queue/batch/1", "test-queue/batch/2"}, createdIDs) + require.Len(t, created, 1) + assert.Equal(t, entity.BatchStateCreating, created[0].State) + assert.Empty(t, created[0].Dependencies) + assert.Equal(t, []string{request.ID}, created[0].Contains) } -func TestController_Process_InitializationFailure(t *testing.T) { +// The request is told a batch is being built for it. "batching" rather than +// "batched": the batch has no dependencies yet and may still be discarded in +// favour of another. +func TestController_Process_PublishesBatchingStatus(t *testing.T) { ctrl := gomock.NewController(t) request := testRequest() - requestStore := storagemock.NewMockRequestStore(ctrl) - requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateBatched), int32(1), int32(2)).Return(nil) batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - - batchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - batchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(errors.New("storage failed")) - - requestBatchStore := storagemock.NewMockRequestBatchStore(ctrl) - requestBatchStore.EXPECT().Create(gomock.Any(), entity.RequestBatch{ - RequestID: request.ID, - BatchID: "test-queue/batch/1", - Version: 1, - }).Return(nil) + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) store := storagemock.NewMockStorage(ctrl) - store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - store.EXPECT().GetBatchDependentStore().Return(batchDependentStore).AnyTimes() - store.EXPECT().GetRequestBatchStore().Return(requestBatchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - controller := newTestController(t, ctrl, newSequentialCounter(ctrl), store, nil, nil) - msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) - delivery := consumermock.NewMockDelivery(ctrl) - delivery.EXPECT().Message().Return(msg).AnyTimes() - delivery.EXPECT().Attempt().Return(1).AnyTimes() + var logs []entity.RequestLog + publisher := queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, topic string, msg entityqueue.Message) error { + if topic == "log" { + entry, err := entity.RequestLogFromBytes(msg.Payload) + require.NoError(t, err) + logs = append(logs, entry) + } + return nil + }, + ).AnyTimes() - err := controller.Process(context.Background(), delivery) - assert.ErrorContains(t, err, "failed to create batch dependent index") -} + controller := NewController( + zaptest.NewLogger(t).Sugar(), tally.NoopScope, newTestRegistry(t, publisher, ctrl), + staticCounterFactory{counter: newSequentialCounter(ctrl)}, + storageFactoryFor(ctrl, store), topickey.TopicKeyBatch, "orchestrator-batch", + ) -func TestController_PopulateBatch_Errors(t *testing.T) { - batch := entity.Batch{ - ID: "test-queue/batch/1", - Dependencies: []string{"test-queue/batch/0"}, - State: entity.BatchStateCreating, - Version: 1, - } - storeErr := errors.New("storage failed") - - tests := []struct { - name string - mockFunc func(*storagemock.MockBatchStore, *storagemock.MockBatchDependentStore) - errMsg string - }{ - { - name: "own reverse index create fails", - mockFunc: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { - dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storeErr) - }, - errMsg: "failed to create batch dependent index", - }, - { - name: "dependency get fails", - mockFunc: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { - dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - dependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/0").Return(entity.BatchDependent{}, storeErr) - }, - errMsg: "failed to get batch dependent", - }, - { - name: "dependency update fails", - mockFunc: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { - dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - dependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/0").Return(entity.BatchDependent{ - BatchID: "test-queue/batch/0", - Version: 2, - }, nil) - dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: "test-queue/batch/0", - Dependents: []string{batch.ID}, - Version: 2, - }, int32(2), int32(3)).Return(storeErr) - }, - errMsg: "failed to update batch dependent index", - }, - { - name: "created transition fails", - mockFunc: func(batchStore *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { - dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - dependentStore.EXPECT().Get(gomock.Any(), "test-queue/batch/0").Return(entity.BatchDependent{ - BatchID: "test-queue/batch/0", - Dependents: []string{"test-queue/batch/old"}, - Version: 2, - }, nil) - dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ - BatchID: "test-queue/batch/0", - Dependents: []string{"test-queue/batch/old", batch.ID}, - Version: 2, - }, int32(2), int32(3)).Return(nil) - batchStore.EXPECT().Update(gomock.Any(), batchWithState(batch, entity.BatchStateCreated), int32(1), int32(2)).Return(storeErr) - }, - errMsg: "failed to mark batch", - }, - } + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, request, ""))) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - batchStore := storagemock.NewMockBatchStore(ctrl) - batchDependentStore := storagemock.NewMockBatchDependentStore(ctrl) - tt.mockFunc(batchStore, batchDependentStore) - - store := storagemock.NewMockStorage(ctrl) - store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - store.EXPECT().GetBatchDependentStore().Return(batchDependentStore).AnyTimes() - - controller := &Controller{metricsScope: tally.NoopScope} - _, err := controller.populateBatch(context.Background(), store, batch) - assert.ErrorContains(t, err, tt.errMsg) - }) - } + require.Len(t, logs, 1) + assert.Equal(t, request.ID, logs[0].RequestID) + assert.Equal(t, entity.RequestStatusBatching, logs[0].Status) + assert.Equal(t, "test-queue/batch/1", logs[0].Metadata["batch_id"]) } diff --git a/submitqueue/orchestrator/controller/cancel/cancel.go b/submitqueue/orchestrator/controller/cancel/cancel.go index aad8e0c5..243e3af3 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel.go +++ b/submitqueue/orchestrator/controller/cancel/cancel.go @@ -25,9 +25,9 @@ // // - The request is associated with one or more batch attempts — the controller // records cancellation intent on every cancellable attempt and hands each one -// to speculate. Creating attempts are ignored because their reverse indexes -// may be incomplete, while Merging and terminal attempts retain their existing -// outcome for conclude to reconcile. +// to speculate. Creating attempts are ignored because their dependency set is +// not yet resolved and nothing downstream can see them, while Merging and +// terminal attempts retain their existing outcome for conclude to reconcile. // // The split exists so that the terminal write and the work that must precede // it (cancelling builds, respeculating dependents) live in the same controller @@ -52,9 +52,7 @@ package cancel import ( "context" - "errors" "fmt" - "sort" "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/consumer" @@ -228,32 +226,14 @@ func (c *Controller) markCancelling(ctx context.Context, store storage.Storage, // findBatches resolves every batch attempt associated with the request. // Associations whose batch was never persisted are stale retry artifacts and are ignored. func (c *Controller) findBatches(ctx context.Context, store storage.Storage, request entity.Request) ([]entity.Batch, error) { - associations, err := store.GetRequestBatchStore().GetByRequestID(ctx, request.ID) + batches, stale, err := corebatch.FindByRequestID(ctx, store, request.ID) if err != nil { - metrics.NamedCounter(c.metricsScope, opName, "request_batch_store_errors", 1) - return nil, fmt.Errorf("failed to get batch associations for request %s: %w", request.ID, err) + metrics.NamedCounter(c.metricsScope, opName, "batch_lookup_errors", 1) + return nil, err } - - var batches []entity.Batch - for _, association := range associations { - batch, err := store.GetBatchStore().Get(ctx, association.BatchID) - if err != nil { - if errors.Is(err, storage.ErrNotFound) { - // The association may precede batch persistence or may outlive a failed attempt. - // If the batch is later persisted and published, speculate re-checks the contained request state before starting work. - metrics.NamedCounter(c.metricsScope, opName, "stale_batch_associations", 1) - continue - } - metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) - return nil, fmt.Errorf("failed to get associated batch %s for request %s: %w", association.BatchID, request.ID, err) - } - batches = append(batches, batch) + if stale > 0 { + metrics.NamedCounter(c.metricsScope, opName, "stale_batch_associations", int64(stale)) } - - // The batches are independent, but deterministic order stabilizes logs, tests, and first-error selection. - sort.Slice(batches, func(i, j int) bool { - return batches[i].ID < batches[j].ID - }) return batches, nil } diff --git a/submitqueue/orchestrator/controller/dependencyanalysis/BUILD.bazel b/submitqueue/orchestrator/controller/dependencyanalysis/BUILD.bazel new file mode 100644 index 00000000..fe5b08e6 --- /dev/null +++ b/submitqueue/orchestrator/controller/dependencyanalysis/BUILD.bazel @@ -0,0 +1,45 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["dependencyanalysis.go"], + importpath = "github.com/uber/submitqueue/submitqueue/orchestrator/controller/dependencyanalysis", + visibility = ["//visibility:public"], + deps = [ + "//platform/consumer:go_default_library", + "//platform/metrics:go_default_library", + "//platform/publish:go_default_library", + "//submitqueue/core/batch:go_default_library", + "//submitqueue/core/request:go_default_library", + "//submitqueue/core/topickey:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/conflict:go_default_library", + "//submitqueue/extension/storage:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["dependencyanalysis_test.go"], + embed = [":go_default_library"], + deps = [ + "//platform/base/messagequeue:go_default_library", + "//platform/consumer:go_default_library", + "//platform/consumer/mock:go_default_library", + "//platform/extension/messagequeue/mock:go_default_library", + "//submitqueue/core/topickey:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/conflict:go_default_library", + "//submitqueue/extension/conflict/all:go_default_library", + "//submitqueue/extension/conflict/mock:go_default_library", + "//submitqueue/extension/storage:go_default_library", + "//submitqueue/extension/storage/mock:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + "@org_uber_go_zap//zaptest:go_default_library", + ], +) diff --git a/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis.go b/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis.go new file mode 100644 index 00000000..cb36da1d --- /dev/null +++ b/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis.go @@ -0,0 +1,468 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package dependencyanalysis decides which batch carries a request, resolves +// what that batch must serialize behind, and promotes it from Creating to +// Created. +// +// # Why this is its own stage +// +// Created is dependency-eligible: the next batch's analysis will pick it up and +// serialize behind it. A batch may therefore only reach Created if it is certain +// to be admitted afterwards, or it becomes a permanent dependency nothing can +// resolve and the queue wedges behind it. +// +// Everything that makes a batch real happens here, in one stage: the enrolment +// decision, the association, the request claim, the dependency set, and the +// promotion. The batch stage upstream only mints an ID and hands it over, so a +// redelivery there costs an unreachable Creating row and nothing else — which is +// what lets this stage be the single place that decides. +// +// # Partitioning +// +// Messages must be partitioned by queue. Analysis reads the queue's +// dependency-eligible batches, so two batches of one queue analyzed concurrently +// would each see the other still in Creating, neither would serialize behind the +// other, and both would speculate as though the other did not exist. Serial +// consumption is also what makes the enrolment check safe: it reads, decides and +// writes without another batch of the same queue interleaving. +// +// # Idempotency +// +// Redelivery is expected and every step tolerates it. A batch already past +// Creating skips analysis entirely and only re-announces. Within analysis, the +// reverse-index and association writes are individually idempotent, because a +// failure part-way through leaves the state at Creating and the retry re-enters +// here. +package dependencyanalysis + +import ( + "context" + "errors" + "fmt" + "slices" + + "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/platform/publish" + corebatch "github.com/uber/submitqueue/submitqueue/core/batch" + corerequest "github.com/uber/submitqueue/submitqueue/core/request" + "github.com/uber/submitqueue/submitqueue/core/topickey" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/conflict" + "github.com/uber/submitqueue/submitqueue/extension/storage" + "go.uber.org/zap" +) + +// Controller handles dependency-analysis queue messages. +type Controller struct { + logger *zap.SugaredLogger + metricsScope tally.Scope + registry consumer.TopicRegistry + stores storage.Factory + analyzers conflict.Factory + topicKey consumer.TopicKey + consumerGroup string +} + +// Verify Controller implements consumer.Controller interface at compile time. +var _ consumer.Controller = (*Controller)(nil) + +const opName = "process" + +// NewController creates a new dependency-analysis controller for the orchestrator. +func NewController( + logger *zap.SugaredLogger, + scope tally.Scope, + stores storage.Factory, + analyzers conflict.Factory, + registry consumer.TopicRegistry, + topicKey consumer.TopicKey, + consumerGroup string, +) *Controller { + return &Controller{ + logger: logger.Named("dependency_analysis_controller"), + metricsScope: scope.SubScope("dependency_analysis_controller"), + registry: registry, + stores: stores, + analyzers: analyzers, + topicKey: topicKey, + consumerGroup: consumerGroup, + } +} + +// Process enrols a batch's requests, resolves its dependencies, promotes it to +// Created, and hands it to speculate. +// Returns nil to ack (success), or error to nack (retry). +func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) error { + msg := delivery.Message() + + bid, err := entity.BatchIDFromBytes(msg.Payload) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "deserialize_errors", 1) + return fmt.Errorf("failed to deserialize batch ID: %w", err) + } + + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + + batch, err := store.GetBatchStore().Get(ctx, bid.ID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) + return fmt.Errorf("failed to get batch %s: %w", bid.ID, err) + } + + // The payload's queue must match the batch's authoritative queue; a + // mismatch is a malformed message. Non-retryable — reject to the DLQ. + if bid.Queue != "" && bid.Queue != batch.Queue { + metrics.NamedCounter(c.metricsScope, opName, "queue_mismatch", 1) + return fmt.Errorf("payload queue %q does not match queue %q of batch %s", bid.Queue, batch.Queue, batch.ID) + } + + c.logger.Infow("received dependency-analysis event", + "batch_id", batch.ID, + "queue", batch.Queue, + "state", string(batch.State), + "attempt", delivery.Attempt(), + "partition_key", msg.PartitionKey, + ) + + switch { + case entity.IsBatchStateHalted(batch.State): + // Cancelled or concluded while the analysis message was in flight. + // Promoting it now would hand speculate a batch nobody expects to land. + metrics.NamedCounter(c.metricsScope, opName, "skipped_halted", 1) + return nil + + case batch.State == entity.BatchStateCreating: + enrolled, err := c.requestEnrolledInAnotherBatch(ctx, store, batch) + if err != nil { + return err + } + if enrolled { + // A previous batch message for this request was redelivered and minted + // this batch too. The first one through here owns the request; this one + // stays Creating, where nothing can reach it. + metrics.NamedCounter(c.metricsScope, opName, "skipped_already_enrolled", 1) + return nil + } + + halted, err := c.firstHaltedRequestID(ctx, store, batch) + if err != nil { + return err + } + if halted != "" { + // Nothing has claimed the request yet, so cancel owned it outright and + // has already written its outcome. + metrics.NamedCounter(c.metricsScope, opName, "skipped_halted_request", 1) + c.logger.Infow("abandoning batch; contained request is halted", + "batch_id", batch.ID, + "request_id", halted, + ) + return nil + } + + dependencies, err := c.resolveDependencies(ctx, store, batch) + if err != nil { + return err + } + if err := c.writeDependentIndexes(ctx, store, batch, dependencies); err != nil { + return err + } + if err := c.associateRequestsWithBatch(ctx, store, batch); err != nil { + return err + } + claimed, err := c.claimRequestsForBatch(ctx, store, batch) + if err != nil { + return err + } + if !claimed { + // Cancel reached the request first and owns its outcome. Leave the + // batch in Creating; promoting it would hand speculate a batch built + // on a request that is on its way out. + return nil + } + + // Transition writes the whole batch, so the dependency set and the state + // land in one compare-and-swap. + batch.Dependencies = dependencies + batch, err = corebatch.Transition(ctx, store, batch, entity.BatchStateCreated) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) + return fmt.Errorf("failed to mark batch %s created: %w", batch.ID, err) + } + c.logger.Infow("batch created", + "batch_id", batch.ID, + "queue", batch.Queue, + "dependency_count", len(batch.Dependencies), + ) + + case batch.State == entity.BatchStateCreated: + // A prior attempt transitioned but lost its announcement, so + // re-announcing is the whole job. + metrics.NamedCounter(c.metricsScope, opName, "reannounced", 1) + + default: + // Speculating or merging: the announcement landed and the batch has + // already moved past this stage. + metrics.NamedCounter(c.metricsScope, opName, "already_admitted", 1) + return nil + } + + // Reported here rather than at batch creation: until the transition above + // lands, the batch has no dependency set and nothing in the queue can see + // it, so "batched" would promise more than had happened. Deduped per + // (request, status), so the re-announce path re-publishes harmlessly. + if err := corerequest.PublishBatchLogs(ctx, c.registry, batch.Queue, batch.Contains, + entity.RequestStatusBatched, "", map[string]string{"batch_id": batch.ID}, + ); err != nil { + metrics.NamedCounter(c.metricsScope, opName, "request_log_errors", 1) + return fmt.Errorf("failed to publish request logs for batch %s: %w", batch.ID, err) + } + + return c.publishToSpeculate(ctx, batch) +} + +// requestEnrolledInAnotherBatch reports whether one of the batch's requests is already +// carried by a different batch that got past Creating. +// +// The batch stage mints a batch per delivery, so a lost ack leaves two, each +// with its own hand-off. This is where that is resolved: the topic is +// partitioned by queue and consumed in order, so the first hand-off through +// here enrols the request and the second finds it and stops. Without the check +// the same change would end up in two live batches, both admitted, both merged. +func (c *Controller) requestEnrolledInAnotherBatch(ctx context.Context, store storage.Storage, batch entity.Batch) (bool, error) { + for _, requestID := range batch.Contains { + existing, stale, err := corebatch.FindByRequestID(ctx, store, requestID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_lookup_errors", 1) + return false, err + } + if stale > 0 { + metrics.NamedCounter(c.metricsScope, opName, "stale_batch_associations", int64(stale)) + } + for _, other := range existing { + if other.ID == batch.ID || other.State == entity.BatchStateCreating { + continue + } + c.logger.Infow("abandoning batch; request is already carried by another", + "batch_id", batch.ID, + "request_id", requestID, + "enrolled_in", other.ID, + "enrolled_state", string(other.State), + ) + return true, nil + } + } + return false, nil +} + +// associateRequestsWithBatch links the batch to its requests. This is the record that makes the +// batch findable from a request, so writing it is what enrols them. +func (c *Controller) associateRequestsWithBatch(ctx context.Context, store storage.Storage, batch entity.Batch) error { + for _, requestID := range batch.Contains { + association := entity.RequestBatch{RequestID: requestID, BatchID: batch.ID, Version: 1} + if err := store.GetRequestBatchStore().Create(ctx, association); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + metrics.NamedCounter(c.metricsScope, opName, "request_batch_store_errors", 1) + return fmt.Errorf("failed to associate request %s with batch %s: %w", requestID, batch.ID, err) + } + } + return nil +} + +// claimRequestsForBatch CASes each of the batch's requests to RequestStateBatched, which is +// what enrols them. It reports whether every request was claimed; false means +// cancel reached one of them first and owns its outcome, so the batch must be +// abandoned in Creating rather than promoted. +// +// Two guards make the claim the serialization point against cancel, and both +// are load-bearing. The state check rejects a cancellation that completed +// before this read: the read is taken here rather than at the top of the stage, +// so it carries a fresh version, and a version guard alone would compare equal +// and write Batched straight over Cancelled. The version guard then rejects a +// cancellation landing in the remaining window between this read and the write. +// +// A redelivery re-applies Batched → Batched as a version-only bump, which keeps +// both guards in force on every attempt. +func (c *Controller) claimRequestsForBatch(ctx context.Context, store storage.Storage, batch entity.Batch) (bool, error) { + for _, requestID := range batch.Contains { + request, err := store.GetRequestStore().Get(ctx, requestID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) + return false, fmt.Errorf("failed to get request %s of batch %s: %w", requestID, batch.ID, err) + } + + if entity.IsRequestStateHalted(request.State) { + metrics.NamedCounter(c.metricsScope, opName, "request_claim_lost_race", 1) + c.logger.Infow("abandoning batch; request was halted during analysis", + "batch_id", batch.ID, + "request_id", requestID, + "request_state", string(request.State), + ) + return false, nil + } + + newVersion := request.Version + 1 + claimed := request + claimed.State = entity.RequestStateBatched + if err := store.GetRequestStore().Update(ctx, claimed, request.Version, newVersion); err != nil { + if errors.Is(err, storage.ErrVersionMismatch) { + metrics.NamedCounter(c.metricsScope, opName, "request_claim_lost_race", 1) + c.logger.Infow("abandoning batch; request advanced concurrently (likely cancel)", + "batch_id", batch.ID, + "request_id", requestID, + ) + return false, nil + } + metrics.NamedCounter(c.metricsScope, opName, "request_claim_errors", 1) + return false, fmt.Errorf("failed to claim request %s for batch %s: %w", requestID, batch.ID, err) + } + } + return true, nil +} + +// firstHaltedRequestID returns the first request in the batch the user has given up +// on, or the empty string if every member is still live. +func (c *Controller) firstHaltedRequestID(ctx context.Context, store storage.Storage, batch entity.Batch) (string, error) { + for _, requestID := range batch.Contains { + request, err := store.GetRequestStore().Get(ctx, requestID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) + return "", fmt.Errorf("failed to get request %s of batch %s: %w", requestID, batch.ID, err) + } + if entity.IsRequestStateHalted(request.State) { + return requestID, nil + } + } + return "", nil +} + +// resolveDependencies asks the queue's conflict analyzer which in-flight batches the new +// batch must serialize behind. The read goes through the queue's per-state +// membership records; classification uses each batch's own hydrated state, so +// a stale record can never misreport a batch. +func (c *Controller) resolveDependencies(ctx context.Context, store storage.Storage, batch entity.Batch) ([]string, error) { + inFlight, err := corebatch.ListByStates(ctx, store, entity.DependencyBatchStates()) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) + return nil, fmt.Errorf("failed to get active batches for queue=%s: %w", batch.Queue, err) + } + + analyzer, err := c.analyzers.For(conflict.Config{QueueName: batch.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "conflict_analyzer_errors", 1) + return nil, fmt.Errorf("failed to build conflict analyzer for queue=%s: %w", batch.Queue, err) + } + conflicts, err := analyzer.Analyze(ctx, batch, inFlight) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "conflict_analyzer_errors", 1) + return nil, fmt.Errorf("failed to analyze conflicts for batchID=%s: %w", batch.ID, err) + } + + // Dedupe by batch ID since a single (analyzed, in-flight) pair may be + // reported with multiple Conflict entries when different conflict types + // apply; the dependency graph only tracks the relation. + seen := make(map[string]struct{}, len(conflicts)) + dependencies := make([]string, 0, len(conflicts)) + for _, cf := range conflicts { + if _, ok := seen[cf.BatchID]; ok { + continue + } + seen[cf.BatchID] = struct{}{} + dependencies = append(dependencies, cf.BatchID) + } + return dependencies, nil +} + +// writeDependentIndexes creates the batch's own reverse-index row and lists it as a dependent +// of everything it depends on. +// +// Both writes are idempotent on their own: a failure part-way through the loop +// leaves the batch in Creating, so the retry re-enters here and would +// otherwise duplicate whatever the first pass already wrote. +func (c *Controller) writeDependentIndexes(ctx context.Context, store storage.Storage, batch entity.Batch, dependencies []string) error { + own := entity.BatchDependent{ + BatchID: batch.ID, + Dependents: []string{}, + Version: 1, + } + if err := store.GetBatchDependentStore().Create(ctx, own); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) + return fmt.Errorf("failed to create batch dependent index for new batchID=%s: %w", batch.ID, err) + } + + for _, dependencyID := range dependencies { + existing, err := store.GetBatchDependentStore().Get(ctx, dependencyID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) + return fmt.Errorf("failed to get batch dependent for batchID=%s: %w", dependencyID, err) + } + if slices.Contains(existing.Dependents, batch.ID) { + continue + } + + updated := existing + updated.Dependents = append(append([]string(nil), existing.Dependents...), batch.ID) + newVersion := existing.Version + 1 + if err := store.GetBatchDependentStore().Update(ctx, updated, existing.Version, newVersion); err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) + return fmt.Errorf("failed to update batch dependent index for existing batchID=%s and new batchID=%s: %w", dependencyID, batch.ID, err) + } + } + return nil +} + +// publishToSpeculate hands the batch to the speculate stage. +// +// The message ID is the bare batch ID, with no cause: a batch announces its own +// creation once in its life, so a redelivery that re-announces it is meant to be +// dropped. Every later publish about the same batch names its cause and so +// cannot collide with this row — see publish.IntentID. +func (c *Controller) publishToSpeculate(ctx context.Context, batch entity.Batch) error { + payload, err := entity.BatchID{ID: batch.ID, Queue: batch.Queue}.ToBytes() + if err != nil { + return fmt.Errorf("failed to serialize batch ID: %w", err) + } + + if err := publish.Message(ctx, c.registry, topickey.TopicKeySpeculate, publish.IntentID(batch.ID), payload, batch.Queue); err != nil { + metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1) + return fmt.Errorf("failed to publish batch ID to speculate topic: %w", err) + } + + c.logger.Infow("published batch to speculate topic", + "batch_id", batch.ID, + "topic_key", topickey.TopicKeySpeculate, + ) + return nil +} + +// Name returns the controller name for logging and metrics. +func (c *Controller) Name() string { + return "dependency-analysis" +} + +// TopicKey returns the topic key this controller subscribes to. +func (c *Controller) TopicKey() consumer.TopicKey { + return c.topicKey +} + +// ConsumerGroup returns the consumer group for offset tracking. +func (c *Controller) ConsumerGroup() string { + return c.consumerGroup +} diff --git a/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis_test.go b/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis_test.go new file mode 100644 index 00000000..1a04e703 --- /dev/null +++ b/submitqueue/orchestrator/controller/dependencyanalysis/dependencyanalysis_test.go @@ -0,0 +1,873 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dependencyanalysis + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" + consumermock "github.com/uber/submitqueue/platform/consumer/mock" + queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" + "github.com/uber/submitqueue/submitqueue/core/topickey" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/conflict" + "github.com/uber/submitqueue/submitqueue/extension/conflict/all" + conflictmock "github.com/uber/submitqueue/submitqueue/extension/conflict/mock" + "github.com/uber/submitqueue/submitqueue/extension/storage" + storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" + "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" +) + +const ( + testQueue = "test-queue" + testRequestID = "test-queue/123" + testBatchID = "test-queue/batch/3" +) + +// analyzerCfg is the per-queue identity handed to the conflict analyzer in +// cases that do not exercise per-queue routing. +var analyzerCfg = conflict.Config{QueueName: testQueue} + +// testBatch returns the batch under analysis, as the batch stage leaves it. +func testBatch() entity.Batch { + return entity.Batch{ + ID: testBatchID, + Queue: testQueue, + Contains: []string{testRequestID}, + Dependencies: []string{}, + State: entity.BatchStateCreating, + Version: 1, + } +} + +// liveRequest returns the batch's member request in a state that does not halt +// promotion. +func liveRequest() entity.Request { + return entity.Request{ + ID: testRequestID, + Queue: testQueue, + State: entity.RequestStateBatched, + Version: 2, + } +} + +func batchIDPayload(t *testing.T, id, queue string) []byte { + t.Helper() + payload, err := entity.BatchID{ID: id, Queue: queue}.ToBytes() + require.NoError(t, err) + return payload +} + +// newQueueBatchStateStore accepts any record write and lists the given batches +// as membership records under their current state. Callers hydrating candidates +// must set up the corresponding BatchStore.Get expectations themselves. +func newQueueBatchStateStore(ctrl *gomock.Controller, active ...entity.Batch) *storagemock.MockQueueBatchStateStore { + s := storagemock.NewMockQueueBatchStateStore(ctrl) + s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().List(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, state entity.BatchState) ([]entity.QueueBatchState, error) { + var records []entity.QueueBatchState + for _, b := range active { + if b.State == state { + records = append(records, entity.QueueBatchState{Queue: b.Queue, State: state, BatchID: b.ID}) + } + } + return records, nil + }, + ).AnyTimes() + return s +} + +func storageFactoryFor(ctrl *gomock.Controller, store storage.Storage) *storagemock.MockFactory { + f := storagemock.NewMockFactory(ctrl) + f.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() + return f +} + +// newTestController builds a controller over the given store. A nil analyzer +// resolves to the "all" analyzer, under which every dependency-eligible batch +// conflicts. +func newTestController(t *testing.T, ctrl *gomock.Controller, store storage.Storage, analyzer conflict.Analyzer, publisher *queuemock.MockPublisher) *Controller { + t.Helper() + + if analyzer == nil { + analyzer = all.New(analyzerCfg) + } + analyzerFactory := conflictmock.NewMockFactory(ctrl) + analyzerFactory.EXPECT().For(gomock.Any()).Return(analyzer, nil).AnyTimes() + + if publisher == nil { + publisher = queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + } + queue := queuemock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(publisher).AnyTimes() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: queue}, + {Key: topickey.TopicKeyLog, Name: "log", Queue: queue}, + }) + require.NoError(t, err) + + return NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, storageFactoryFor(ctrl, store), + analyzerFactory, registry, topickey.TopicKeyDependencyAnalysis, "orchestrator-dependency-analysis") +} + +func newDelivery(t *testing.T, ctrl *gomock.Controller, id, queue string) *consumermock.MockDelivery { + t.Helper() + + msg := entityqueue.NewMessage(id, batchIDPayload(t, id, queue), queue, nil) + delivery := consumermock.NewMockDelivery(ctrl) + delivery.EXPECT().Message().Return(msg).AnyTimes() + delivery.EXPECT().Attempt().Return(1).AnyTimes() + return delivery +} + +// requestStoreFor returns a request store that answers with the given requests, +// keyed by ID. +func requestStoreFor(ctrl *gomock.Controller, requests ...entity.Request) *storagemock.MockRequestStore { + s := storagemock.NewMockRequestStore(ctrl) + for _, request := range requests { + s.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil).AnyTimes() + } + s.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + return s +} + +// noPriorEnrollment answers the dedup lookup with nothing and accepts the +// association write, i.e. this batch is the first one through for its request. +func noPriorEnrollment(ctrl *gomock.Controller) *storagemock.MockRequestBatchStore { + s := storagemock.NewMockRequestBatchStore(ctrl) + s.EXPECT().GetByRequestID(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + s.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + return s +} + +func TestNewController(t *testing.T) { + ctrl := gomock.NewController(t) + store := storagemock.NewMockStorage(ctrl) + controller := newTestController(t, ctrl, store, nil, nil) + + require.NotNil(t, controller) + assert.Equal(t, topickey.TopicKeyDependencyAnalysis, controller.TopicKey()) + assert.Equal(t, "orchestrator-dependency-analysis", controller.ConsumerGroup()) + assert.Equal(t, "dependency-analysis", controller.Name()) + + var _ consumer.Controller = controller +} + +// The whole point of the stage: resolve what the batch must serialize behind, +// record it on both sides of the graph, and promote it to Created. +func TestController_Process_AnalyzesAndTransitionsToCreated(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + inFlight := []entity.Batch{ + {ID: "test-queue/batch/1", Queue: testQueue, State: entity.BatchStateCreated, Version: 1}, + {ID: "test-queue/batch/2", Queue: testQueue, State: entity.BatchStateSpeculating, Version: 2}, + } + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight[0].ID).Return(inFlight[0], nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight[1].ID).Return(inFlight[1], nil) + batchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: batch.ID, + Queue: testQueue, + Contains: []string{testRequestID}, + Dependencies: []string{inFlight[0].ID, inFlight[1].ID}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), entity.BatchDependent{ + BatchID: batch.ID, + Dependents: []string{}, + Version: 1, + }).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), inFlight[0].ID).Return(entity.BatchDependent{ + BatchID: inFlight[0].ID, + Version: 1, + }, nil) + dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ + BatchID: inFlight[0].ID, + Dependents: []string{batch.ID}, + Version: 1, + }, int32(1), int32(2)).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), inFlight[1].ID).Return(entity.BatchDependent{ + BatchID: inFlight[1].ID, + Dependents: []string{"test-queue/batch/99"}, + Version: 2, + }, nil) + dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ + BatchID: inFlight[1].ID, + Dependents: []string{"test-queue/batch/99", batch.ID}, + Version: 2, + }, int32(2), int32(3)).Return(nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, inFlight...)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// The analyzer may report one in-flight batch several times when more than one +// conflict type applies; the dependency graph only tracks the relation. +func TestController_Process_DedupesAnalyzerConflicts(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + inFlight := entity.Batch{ID: "test-queue/batch/2", Queue: testQueue, State: entity.BatchStateSpeculating, Version: 2} + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(inFlight, nil) + batchStore.EXPECT().Update(gomock.Any(), entity.Batch{ + ID: batch.ID, + Queue: testQueue, + Contains: []string{testRequestID}, + Dependencies: []string{inFlight.ID}, + State: entity.BatchStateCreated, + Version: 1, + }, int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(entity.BatchDependent{ + BatchID: inFlight.ID, + Version: 5, + }, nil) + dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ + BatchID: inFlight.ID, + Dependents: []string{batch.ID}, + Version: 5, + }, int32(5), int32(6)).Return(nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, inFlight)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + analyzer := conflictmock.NewMockAnalyzer(ctrl) + analyzer.EXPECT().Analyze(gomock.Any(), gomock.Any(), gomock.Any()).Return([]entity.Conflict{ + {BatchID: inFlight.ID, Type: entity.ConflictTypeConservative}, + {BatchID: inFlight.ID, Type: entity.ConflictTypeTargetOverlap}, + }, nil) + + controller := newTestController(t, ctrl, store, analyzer, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// A redelivery whose previous attempt transitioned but lost its announcement: +// re-announcing is the whole job, and re-analyzing would corrupt the graph. +func TestController_Process_RedeliveryAfterTransitionOnlyRepublishes(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + batch.State = entity.BatchStateCreated + batch.Dependencies = []string{"test-queue/batch/1"} + batch.Version = 2 + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + // Dependent store and request store with no EXPECTs — must not be touched. + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(storagemock.NewMockBatchDependentStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + publisher := queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), "log", gomock.Any()).Return(nil).AnyTimes() + publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).Return(nil) + + controller := newTestController(t, ctrl, store, nil, publisher) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// A failure part-way through the index loop leaves the batch in Creating, so +// the retry re-enters the loop over dependencies it may already have written. +func TestController_Process_RedeliveryMidIndexDoesNotDoubleAppend(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + inFlight := entity.Batch{ID: "test-queue/batch/1", Queue: testQueue, State: entity.BatchStateCreated, Version: 1} + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(inFlight, nil) + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists) + // The first pass already listed this batch; Update must not be called again. + dependentStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(entity.BatchDependent{ + BatchID: inFlight.ID, + Dependents: []string{batch.ID}, + Version: 2, + }, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, inFlight)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +func TestController_Process_HaltedRequestIsNotPromoted(t *testing.T) { + for _, state := range []entity.RequestState{ + entity.RequestStateCancelling, + entity.RequestStateCancelled, + entity.RequestStateLanded, + entity.RequestStateError, + } { + t.Run(string(state), func(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + request := liveRequest() + request.State = state + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + // Dependent store with no EXPECTs — must not be touched. + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(storagemock.NewMockBatchDependentStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, request)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + // Publisher with no EXPECTs — must not be called. + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + }) + } +} + +func TestController_Process_HaltedBatchAcksWithoutPublishing(t *testing.T) { + for _, state := range []entity.BatchState{ + entity.BatchStateCancelling, + entity.BatchStateCancelled, + entity.BatchStateFailed, + entity.BatchStateSucceeded, + } { + t.Run(string(state), func(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + batch.State = state + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + // Publisher with no EXPECTs — must not be called. + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + }) + } +} + +// A batch already admitted got its announcement; re-sending one would only buy +// a redundant re-plan. +func TestController_Process_AlreadyAdmittedAcksWithoutPublishing(t *testing.T) { + for _, state := range []entity.BatchState{entity.BatchStateSpeculating, entity.BatchStateMerging} { + t.Run(string(state), func(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + batch.State = state + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + }) + } +} + +func TestController_Process_QueueMismatchRejected(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, "some-other-queue"))) +} + +// The announced batch ID carries its queue, and the message is partitioned by +// queue so speculate stays serial per queue. +func TestController_Process_StampsQueueOnAnnouncement(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + var announced []entityqueue.Message + publisher := queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), "log", gomock.Any()).Return(nil).AnyTimes() + publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).DoAndReturn( + func(_ context.Context, _ string, msg entityqueue.Message) error { + announced = append(announced, msg) + return nil + }, + ) + + controller := newTestController(t, ctrl, store, nil, publisher) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + + require.Len(t, announced, 1) + bid, err := entity.BatchIDFromBytes(announced[0].Payload) + require.NoError(t, err) + assert.Equal(t, batch.ID, bid.ID) + assert.Equal(t, testQueue, bid.Queue) + assert.Equal(t, testQueue, announced[0].PartitionKey) +} + +func TestController_Process_AnalyzerFailure(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + analyzer := conflictmock.NewMockAnalyzer(ctrl) + analyzer.EXPECT().Analyze(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("analyzer down")) + + controller := newTestController(t, ctrl, store, analyzer, nil) + require.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// A lost Update must leave the caller's copy of Dependents untouched, or the +// next attempt would append onto a slice that already grew. +func TestController_Process_IndexUpdateFailureDoesNotMutateFetchedDependents(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + inFlight := entity.Batch{ID: "test-queue/batch/1", Queue: testQueue, State: entity.BatchStateCreated, Version: 1} + + dependents := make([]string, 1, 2) + dependents[0] = "test-queue/batch/98" + existing := entity.BatchDependent{BatchID: inFlight.ID, Dependents: dependents, Version: 4} + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(inFlight, nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(existing, nil) + dependentStore.EXPECT().Update(gomock.Any(), entity.BatchDependent{ + BatchID: inFlight.ID, + Dependents: []string{"test-queue/batch/98", batch.ID}, + Version: existing.Version, + }, existing.Version, existing.Version+1).Return(errors.New("update failed")) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, inFlight)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, nil) + require.Error(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + assert.Equal(t, "", dependents[:cap(dependents)][1]) + assert.Equal(t, int32(4), existing.Version) +} + +func TestController_Process_PromotionErrors(t *testing.T) { + batch := testBatch() + dependencyID := "test-queue/batch/1" + inFlight := entity.Batch{ID: dependencyID, Queue: testQueue, State: entity.BatchStateCreated, Version: 1} + storeErr := errors.New("storage failed") + + tests := map[string]struct { + setup func(*storagemock.MockBatchStore, *storagemock.MockBatchDependentStore) + errMsg string + }{ + "own reverse index create fails": { + setup: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storeErr) + }, + errMsg: "failed to create batch dependent index", + }, + "dependency get fails": { + setup: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), dependencyID).Return(entity.BatchDependent{}, storeErr) + }, + errMsg: "failed to get batch dependent", + }, + "dependency update fails": { + setup: func(_ *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), dependencyID).Return(entity.BatchDependent{ + BatchID: dependencyID, + Version: 2, + }, nil) + dependentStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(2), int32(3)).Return(storeErr) + }, + errMsg: "failed to update batch dependent index", + }, + "created transition fails": { + setup: func(batchStore *storagemock.MockBatchStore, dependentStore *storagemock.MockBatchDependentStore) { + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + dependentStore.EXPECT().Get(gomock.Any(), dependencyID).Return(entity.BatchDependent{ + BatchID: dependencyID, + Version: 2, + }, nil) + dependentStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(2), int32(3)).Return(nil) + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(storeErr) + }, + errMsg: "failed to mark batch", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), inFlight.ID).Return(inFlight, nil) + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + tt.setup(batchStore, dependentStore) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl, inFlight)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + err := controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue)) + assert.ErrorContains(t, err, tt.errMsg) + }) + } +} + +// "batched" is reported from here, not from batch creation: until the batch is +// Created it has no dependency set and nothing in the queue can see it. +func TestController_Process_PublishesBatchedLogOnPromotion(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + + var logs []entity.RequestLog + publisher := queuemock.NewMockPublisher(ctrl) + publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, topic string, msg entityqueue.Message) error { + if topic == "log" { + entry, err := entity.RequestLogFromBytes(msg.Payload) + require.NoError(t, err) + logs = append(logs, entry) + } + return nil + }, + ).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, publisher) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) + + require.Len(t, logs, 1) + assert.Equal(t, testRequestID, logs[0].RequestID) + assert.Equal(t, entity.RequestStatusBatched, logs[0].Status) + assert.Equal(t, batch.ID, logs[0].Metadata["batch_id"]) +} + +// The batch stage mints one batch per delivery, so a lost ack leaves two. The +// first through here enrols the request; the second must stop, or the same +// change ends up in two live batches. +func TestController_Process_AbandonsBatchWhoseRequestIsAlreadyEnrolled(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + winner := entity.Batch{ + ID: "test-queue/batch/2", Queue: testQueue, Contains: []string{testRequestID}, + State: entity.BatchStateSpeculating, Version: 3, + } + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Get(gomock.Any(), winner.ID).Return(winner, nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return([]entity.RequestBatch{ + {RequestID: testRequestID, BatchID: winner.ID, Version: 1}, + }, nil) + + // Dependent store and request store with no EXPECTs — neither may be touched. + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(storagemock.NewMockBatchDependentStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + + // Publisher with no EXPECTs — an abandoned batch announces nothing. + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// The batch's own association must not read as someone else's enrolment. +func TestController_Process_OwnAssociationDoesNotBlockPromotion(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil).AnyTimes() + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return([]entity.RequestBatch{ + {RequestID: testRequestID, BatchID: batch.ID, Version: 1}, + }, nil) + associations.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStoreFor(ctrl, liveRequest())).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// Promotion writes the association and claims the request. +func TestController_Process_EnrolsTheRequest(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + request := liveRequest() + request.State = entity.RequestStateValidated + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + batchStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return(nil, nil) + associations.EXPECT().Create(gomock.Any(), entity.RequestBatch{ + RequestID: testRequestID, BatchID: batch.ID, Version: 1, + }).Return(nil) + + claimed := request + claimed.State = entity.RequestStateBatched + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), testRequestID).Return(request, nil).AnyTimes() + requestStore.EXPECT().Update(gomock.Any(), claimed, request.Version, request.Version+1).Return(nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, nil) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// Cancel reached the request first. The batch is left in Creating and the +// delivery is acked: retrying would not change the answer. +func TestController_Process_ClaimLostToCancelAbandonsBatch(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + // Update must NOT be called — the batch stays in Creating. + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return(nil, nil) + associations.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), testRequestID).Return(liveRequest(), nil).AnyTimes() + requestStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fmt.Errorf("cas: %w", storage.ErrVersionMismatch)) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + // Publisher with no EXPECTs — an abandoned batch announces nothing. + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// Cancel ran to completion while analysis was resolving dependencies, so the +// claim's re-read finds the request halted at a version cancel itself wrote. +// Comparing versions alone would compare equal and write Batched straight over +// Cancelled, reviving a request the user gave up on. +func TestController_Process_RequestCancelledDuringAnalysisAbandonsBatch(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + + live := liveRequest() + cancelled := liveRequest() + cancelled.State = entity.RequestStateCancelled + // Two CASes: cancel records intent, then writes the terminal state. + cancelled.Version = live.Version + 2 + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + // Update must NOT be called — the batch stays in Creating. + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + requestStore := storagemock.NewMockRequestStore(ctrl) + gomock.InOrder( + requestStore.EXPECT().Get(gomock.Any(), testRequestID).Return(live, nil), + requestStore.EXPECT().Get(gomock.Any(), testRequestID).Return(cancelled, nil), + ) + // Update must NOT be called — claiming would overwrite the cancellation. + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noPriorEnrollment(ctrl)).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + // Publisher with no EXPECTs — an abandoned batch announces nothing. + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue))) +} + +// Any claim failure other than a lost race is retryable and must nack. +func TestController_Process_ClaimStorageErrorPropagates(t *testing.T) { + ctrl := gomock.NewController(t) + + batch := testBatch() + claimErr := errors.New("db connection lost") + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) + + dependentStore := storagemock.NewMockBatchDependentStore(ctrl) + dependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), testRequestID).Return(nil, nil) + associations.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), testRequestID).Return(liveRequest(), nil).AnyTimes() + requestStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(claimErr) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetBatchDependentStore().Return(dependentStore).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + controller := newTestController(t, ctrl, store, nil, queuemock.NewMockPublisher(ctrl)) + assert.ErrorIs(t, controller.Process(context.Background(), newDelivery(t, ctrl, batch.ID, testQueue)), claimErr) +} diff --git a/submitqueue/orchestrator/controller/dlq/dlq_test.go b/submitqueue/orchestrator/controller/dlq/dlq_test.go index c77a804a..8d7f63b9 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq_test.go +++ b/submitqueue/orchestrator/controller/dlq/dlq_test.go @@ -39,6 +39,14 @@ type staticStorageFactory struct{ store storage.Storage } // For returns the fixed store aggregate for any queue. func (f staticStorageFactory) For(storage.Config) (storage.Storage, error) { return f.store, nil } +// noBatchAssociations answers the owning-batch lookup with nothing, i.e. no +// batch ever enrolled the request. +func noBatchAssociations(ctrl *gomock.Controller) *storagemock.MockRequestBatchStore { + s := storagemock.NewMockRequestBatchStore(ctrl) + s.EXPECT().GetByRequestID(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + return s +} + func newQueueBatchStateStore(ctrl *gomock.Controller) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() diff --git a/submitqueue/orchestrator/controller/dlq/request.go b/submitqueue/orchestrator/controller/dlq/request.go index 46b68743..cae5ab41 100644 --- a/submitqueue/orchestrator/controller/dlq/request.go +++ b/submitqueue/orchestrator/controller/dlq/request.go @@ -16,11 +16,13 @@ package dlq import ( "context" + "errors" "fmt" "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/metrics" + corebatch "github.com/uber/submitqueue/submitqueue/core/batch" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" "go.uber.org/zap" @@ -63,8 +65,8 @@ func DecodeRequestID(payload []byte) (entity.RequestID, error) { // requestController is the DLQ reconciler for request-scoped pipeline stages. // It is registered once per primary request-scoped topic (start, cancel, // validate, batch) with the matching decoder. On each delivery it decodes the -// request ID and transitions the request to RequestStateError if it is not -// already halted. +// request ID and transitions the request to RequestStateError, unless a live +// batch already owns the request's outcome. type requestController struct { logger *zap.SugaredLogger metricsScope tally.Scope @@ -139,6 +141,26 @@ func (c *requestController) Process(ctx context.Context, delivery consumer.Deliv "dlq_last_error", dmeta["dlq.last_error"], ) + // A batch that got as far as enrolling the request owns its outcome, and + // conclude writes that outcome when the batch finishes. This dead letter can + // be a redundant attempt whose predecessor is already speculating — the batch + // stage mints one batch per delivery — and TerminateRequest does not guard + // against Batched, so failing the request here would overwrite a live batch's + // claim and leave it building for a request reported as failed. + owner, err := owningBatch(ctx, store, rid.ID) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "batch_lookup_errors", 1) + return err + } + if owner != "" { + metrics.NamedCounter(c.metricsScope, opName, "skipped_owned_by_batch", 1) + c.logger.Infow("dlq reconcile: request is carried by a live batch, leaving the outcome to conclude", + "request_id", rid.ID, + "batch_id", owner, + ) + return nil + } + if err := failRequest(ctx, store, c.registry, c.logger, rid.ID, lastError, failureMeta); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err @@ -147,6 +169,55 @@ func (c *requestController) Process(ctx context.Context, delivery consumer.Deliv return nil } +// owningBatch returns a batch that owns the request's outcome, or the empty +// string if none does. +// +// A non-terminal batch past Creating owns the request outright: conclude writes +// its outcome when the batch finishes. Creating is the ambiguous case, because +// the association is written before the claim that enrols the request, so it +// also survives an abandoned attempt — one whose claim lost to cancel, or found +// the request already halted. Such a batch is inert: nothing promotes it and no +// conclude will ever run for it, so letting it answer "owned" would suppress +// the reconcile forever and strand the request. +// +// The request's own state separates the two, because the claim is the only +// writer of Batched: a Creating batch alongside a Batched request is +// mid-promotion and owns it, and any other request state means the claim never +// landed. A request that has since disappeared is left to failRequest, which +// reports the missing row. +func owningBatch(ctx context.Context, store storage.Storage, requestID string) (string, error) { + batches, _, err := corebatch.FindByRequestID(ctx, store, requestID) + if err != nil { + return "", err + } + + creating := "" + for _, batch := range batches { + switch { + case batch.State.IsTerminal(): + case batch.State != entity.BatchStateCreating: + return batch.ID, nil + case creating == "": + creating = batch.ID + } + } + if creating == "" { + return "", nil + } + + request, err := store.GetRequestStore().Get(ctx, requestID) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + return "", nil + } + return "", err + } + if request.State != entity.RequestStateBatched { + return "", nil + } + return creating, nil +} + // Name returns the controller name for logging and metrics. func (c *requestController) Name() string { return string(c.topicKey) diff --git a/submitqueue/orchestrator/controller/dlq/request_test.go b/submitqueue/orchestrator/controller/dlq/request_test.go index 5f88c6bc..adc34d06 100644 --- a/submitqueue/orchestrator/controller/dlq/request_test.go +++ b/submitqueue/orchestrator/controller/dlq/request_test.go @@ -35,6 +35,7 @@ func TestDLQRequestController_InterfaceAndAccessors(t *testing.T) { ctrl := gomock.NewController(t) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") @@ -59,6 +60,7 @@ func TestDLQRequestController_Process_LandRequestPayload(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeLandRequestID, TopicKey(topickey.TopicKeyStart), "orchestrator-start-dlq") @@ -86,6 +88,7 @@ func TestDLQRequestController_Process_CancelRequestPayload(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeCancelRequestID, TopicKey(topickey.TopicKeyCancel), "orchestrator-cancel-dlq") @@ -114,6 +117,7 @@ func TestDLQRequestController_Process_RequestIDPayload(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") @@ -135,6 +139,7 @@ func TestDLQRequestController_Process_DifferentTerminalOutcomeSkips(t *testing.T store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") @@ -151,6 +156,7 @@ func TestDLQRequestController_Process_MalformedPayloadFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() // no store calls expected c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") @@ -165,6 +171,7 @@ func TestDLQRequestController_Process_EmptyIDFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(noBatchAssociations(ctrl)).AnyTimes() // no store calls expected c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") @@ -198,3 +205,163 @@ func newMockDeliveryWithFailure(ctrl *gomock.Controller, payload []byte, f failu d.EXPECT().Failure().Return(f, failed).AnyTimes() return d } + +// The dead letter can be a redundant batch attempt whose predecessor already +// enrolled the request. Failing it here would overwrite a live batch's claim +// and leave that batch building for a request reported as failed. +func TestDLQRequestController_Process_SkipsRequestOwnedByLiveBatch(t *testing.T) { + for _, state := range []entity.BatchState{ + entity.BatchStateCreated, + entity.BatchStateSpeculating, + entity.BatchStateMerging, + entity.BatchStateCancelling, + } { + t.Run(string(state), func(t *testing.T) { + ctrl := gomock.NewController(t) + + // Request store with no EXPECTs — the request must not be touched. + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), "q/1").Return([]entity.RequestBatch{ + {RequestID: "q/1", BatchID: "q/batch/1", Version: 1}, + }, nil) + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: state, Version: 1, + }, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() + + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, + consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") + + payload, err := entity.RequestID{ID: "q/1", Queue: "q"}.ToBytes() + require.NoError(t, err) + + require.NoError(t, c.Process(context.Background(), newMockDelivery(ctrl, payload))) + }) + } +} + +// A batch that already concluded does not own anything conclude has not written +// yet, so the request is still the dead letter's to fail. +func TestDLQRequestController_Process_FailsWhenEveryBatchIsTerminal(t *testing.T) { + ctrl := gomock.NewController(t) + + request := entity.Request{ID: "q/1", Version: 1, State: entity.RequestStateBatched} + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(request, nil) + requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), "q/1").Return([]entity.RequestBatch{ + {RequestID: "q/1", BatchID: "q/batch/1", Version: 1}, + }, nil) + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateFailed, Version: 1, + }, nil) + + registry := newTestLogRegistry(t, ctrl, 1, func(entity.RequestLog) error { return nil }) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, + registry, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") + + payload, err := entity.RequestID{ID: "q/1", Queue: "q"}.ToBytes() + require.NoError(t, err) + + require.NoError(t, c.Process(context.Background(), newMockDelivery(ctrl, payload))) +} + +// The association is written before the claim, so it outlives an attempt whose +// claim was abandoned. That batch stays in Creating forever with nothing to +// promote it, so it must not answer "owned" — doing so would suppress the +// reconcile and strand the request short of a terminal state. +func TestDLQRequestController_Process_FailsWhenCreatingBatchNeverClaimed(t *testing.T) { + for _, state := range []entity.RequestState{ + entity.RequestStateCancelling, + entity.RequestStateValidated, + } { + t.Run(string(state), func(t *testing.T) { + ctrl := gomock.NewController(t) + + request := entity.Request{ID: "q/1", Version: 1, State: state} + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(request, nil).Times(2) + requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), "q/1").Return([]entity.RequestBatch{ + {RequestID: "q/1", BatchID: "q/batch/1", Version: 1}, + }, nil) + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateCreating, Version: 1, + }, nil) + + registry := newTestLogRegistry(t, ctrl, 1, func(entity.RequestLog) error { return nil }) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, + registry, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") + + payload, err := entity.RequestID{ID: "q/1", Queue: "q"}.ToBytes() + require.NoError(t, err) + + require.NoError(t, c.Process(context.Background(), newMockDelivery(ctrl, payload))) + }) + } +} + +// The same Creating batch mid-promotion: the claim landed, so the batch is on +// its way to Created and owns the request. Batched is written only by that +// claim, which is what separates this from the abandoned case above. +func TestDLQRequestController_Process_SkipsWhenCreatingBatchAlreadyClaimed(t *testing.T) { + ctrl := gomock.NewController(t) + + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), "q/1"). + Return(entity.Request{ID: "q/1", Version: 2, State: entity.RequestStateBatched}, nil) + // Update must NOT be called — the batch owns the outcome. + + associations := storagemock.NewMockRequestBatchStore(ctrl) + associations.EXPECT().GetByRequestID(gomock.Any(), "q/1").Return([]entity.RequestBatch{ + {RequestID: "q/1", BatchID: "q/batch/1", Version: 1}, + }, nil) + + batchStore := storagemock.NewMockBatchStore(ctrl) + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{ + ID: "q/batch/1", Queue: "q", Contains: []string{"q/1"}, State: entity.BatchStateCreating, Version: 1, + }, nil) + + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() + store.EXPECT().GetRequestBatchStore().Return(associations).AnyTimes() + store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, + consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") + + payload, err := entity.RequestID{ID: "q/1", Queue: "q"}.ToBytes() + require.NoError(t, err) + + require.NoError(t, c.Process(context.Background(), newMockDelivery(ctrl, payload))) +} diff --git a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go index 2519c5b8..f4f4565f 100644 --- a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go @@ -154,12 +154,12 @@ func TestProcess_MergedAdvancesBatch(t *testing.T) { // The fan-out after a merge must not reuse the bare batch ID. // -// The batch controller announces a new batch to speculate under exactly that -// ID, and the queue deduplicates on (topic, partition key, message ID) against -// every row it has not collected yet, consumed ones included — a window with no -// upper bound on a busy partition. Reusing the ID here made the wake-up that -// lets dependents re-plan a silent no-op, acked as a success, with nothing to -// retry it. +// The batch's own announcement to speculate uses exactly that ID, and the +// queue deduplicates on (topic, partition key, message ID) against every row it +// has not collected yet, consumed ones included — a window with no upper bound +// on a busy partition. Reusing the ID here made the wake-up that lets +// dependents re-plan a silent no-op, acked as a success, with nothing to retry +// it. func TestProcess_FanoutDoesNotCollideWithTheBatchAnnouncement(t *testing.T) { ctrl := gomock.NewController(t) diff --git a/submitqueue/orchestrator/controller/speculate/doc.go b/submitqueue/orchestrator/controller/speculate/doc.go index 109269bf..1a5e8226 100644 --- a/submitqueue/orchestrator/controller/speculate/doc.go +++ b/submitqueue/orchestrator/controller/speculate/doc.go @@ -94,6 +94,11 @@ // user cancel (cancel stage): // ... ──► Cancelling ── every path stopped ──► Cancelled // +// A batch is admitted either by the message that names it or by the next run +// that finds it still in Created, whichever comes first. Created is +// dependency-eligible, so a batch left there would be a dependency nothing can +// resolve; admission cannot be left to rest on one message arriving. +// // Failed and Cancelled fan out to the conclude stage, which reconciles the // batch's requests. // @@ -105,12 +110,12 @@ // reordered signals are harmless, and a later run repairs whatever an // earlier one left half-done. // -// signal ──► read ──► finalize ──► ask ──► check ──► dispatch -// one enact the the filter save changes, -// read of outcomes Specu- its hand builds to -// queue + the facts lator proposals the build stage -// paths already -// decide +// signal ──► read ──► admit ──► finalize ──► ask ──► check ──► dispatch +// one every enact the the filter save changes, +// read of batch outcomes Specu- its hand builds to +// queue + still in the facts lator proposals the build stage +// paths Created already +// decide // // The Speculator is the extension that proposes which paths to fund or // preempt. It only ever proposes: check.go filters its answer, and outcomes diff --git a/submitqueue/orchestrator/controller/speculate/run.go b/submitqueue/orchestrator/controller/speculate/run.go index b5c10899..afc642cc 100644 --- a/submitqueue/orchestrator/controller/speculate/run.go +++ b/submitqueue/orchestrator/controller/speculate/run.go @@ -28,8 +28,8 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) -// run re-plans a whole queue from a single read of its state, in the five -// steps the package doc lays out: read, finalize, ask, check, dispatch. +// run re-plans a whole queue from a single read of its state, in the six +// steps the package doc lays out: read, admit, finalize, ask, check, dispatch. // // The batch on the triggering message only says which queue woke up; nothing // about the plan depends on which batch it was, or on any earlier run. Its @@ -41,6 +41,8 @@ func (c *Controller) run(ctx context.Context, store storage.Storage, trigger ent } snap.trigger = trigger.ID + c.admitCreated(ctx, &snap) + if err := c.finalize(ctx, &snap); err != nil { return err } @@ -70,6 +72,45 @@ func (c *Controller) run(ctx context.Context, store storage.Storage, trigger ent return c.dispatch(ctx, trigger.Queue, snap, kept) } +// admitCreated admits every batch the run found still in Created, folding each +// one into the snapshot as a head open to new work. +// +// Created is dependency-eligible, so a batch that stalls there is a dependency +// nothing can resolve and the whole queue wedges behind it. Tying admission to +// a message that names one specific batch makes that guarantee only as strong +// as one message; the read above already lists every batch in the queue, so the +// run can close the gap itself for the price of a compare-and-swap. +// +// A head admitted here has no paths yet, so finalize cannot reach an outcome on +// it this run — see decide. +// +// Best-effort: a failed admit is left to the next run rather than failing this +// one, because one batch must not stall the rest of the queue's planning. A +// lost compare-and-swap just means another writer admitted it first. +func (c *Controller) admitCreated(ctx context.Context, snap *snapshot) { + for i, batch := range snap.inFlight { + if batch.State != entity.BatchStateCreated { + continue + } + + updated, err := c.admit(ctx, snap.store, batch) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "unadmitted_repair_errors", 1) + c.logger.Warnw("failed to admit a batch found in created", + "batch_id", batch.ID, + "queue", batch.Queue, + "error", err, + ) + continue + } + + metrics.NamedCounter(c.metricsScope, opName, "unadmitted_repaired", 1) + snap.batches[updated.ID] = updated + snap.inFlight[i] = updated + snap.speculating = append(snap.speculating, updated) + } +} + // read builds the run's snapshot. Batches come first because their dependency // lists say which finalized batches still have to be resolved, and their IDs // say which path sets to load. diff --git a/submitqueue/orchestrator/controller/speculate/run_test.go b/submitqueue/orchestrator/controller/speculate/run_test.go index 1863b820..a2ad8298 100644 --- a/submitqueue/orchestrator/controller/speculate/run_test.go +++ b/submitqueue/orchestrator/controller/speculate/run_test.go @@ -1569,3 +1569,75 @@ func TestRun_SpeculatedIsReportedBeforeTheMergeDispatch(t *testing.T) { require.Len(t, h.logs, 1) assert.Equal(t, entity.RequestStatusSpeculated, h.logs[0].Status) } + +// A batch left in Created is dependency-eligible but has nothing driving it +// forward, so the run admits it rather than waiting for a message that names it. +func TestRun_AdmitsBatchLeftInCreated(t *testing.T) { + ctrl := gomock.NewController(t) + spec := &scriptedSpeculator{} + + trigger := entity.Batch{ID: head, Queue: "q", State: entity.BatchStateSpeculating, Version: 1} + straggler := entity.Batch{ + ID: "q/batch/9", Queue: "q", Contains: []string{"q/9"}, + State: entity.BatchStateCreated, Version: 1, + } + + h := newRunHarness(t, ctrl, spec, []entity.Batch{trigger, straggler}) + h.noBuildsDispatched() + h.pathSets.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.SpeculationPathSet{}, storage.ErrNotFound).AnyTimes() + + admitted := straggler + admitted.State = entity.BatchStateSpeculating + h.batches.EXPECT().Update(gomock.Any(), admitted, int32(1), int32(2)).Return(nil) + + require.NoError(t, h.run(head)) + + assert.Contains(t, h.filed, entity.QueueBatchState{Queue: "q", State: entity.BatchStateSpeculating, BatchID: straggler.ID}) + require.Len(t, h.logs, 1) + assert.Equal(t, entity.RequestStatusSpeculating, h.logs[0].Status) + assert.Empty(t, h.published, "admission needs no message of its own") +} + +// Another writer got there first. Nothing is left to redo, and the rest of the +// queue's planning must not be abandoned over it. +func TestRun_AdmitLostRaceDoesNotFailRun(t *testing.T) { + ctrl := gomock.NewController(t) + spec := &scriptedSpeculator{} + + trigger := entity.Batch{ID: head, Queue: "q", State: entity.BatchStateSpeculating, Version: 1} + straggler := entity.Batch{ + ID: "q/batch/9", Queue: "q", Contains: []string{"q/9"}, + State: entity.BatchStateCreated, Version: 1, + } + + h := newRunHarness(t, ctrl, spec, []entity.Batch{trigger, straggler}) + h.noBuildsDispatched() + h.pathSets.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.SpeculationPathSet{}, storage.ErrNotFound).AnyTimes() + h.batches.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(storage.ErrVersionMismatch) + + require.NoError(t, h.run(head)) + assert.NotContains(t, h.filed, entity.QueueBatchState{Queue: "q", State: entity.BatchStateSpeculating, BatchID: straggler.ID}) + assert.Equal(t, 1, spec.calls, "the rest of the queue is still planned") +} + +// One unrepairable batch must not stall every other batch in the queue; the +// next run tries again. +func TestRun_AdmitStorageErrorDoesNotFailRun(t *testing.T) { + ctrl := gomock.NewController(t) + spec := &scriptedSpeculator{} + + trigger := entity.Batch{ID: head, Queue: "q", State: entity.BatchStateSpeculating, Version: 1} + straggler := entity.Batch{ + ID: "q/batch/9", Queue: "q", Contains: []string{"q/9"}, + State: entity.BatchStateCreated, Version: 1, + } + + h := newRunHarness(t, ctrl, spec, []entity.Batch{trigger, straggler}) + h.noBuildsDispatched() + h.pathSets.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.SpeculationPathSet{}, storage.ErrNotFound).AnyTimes() + h.batches.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(assert.AnError) + + require.NoError(t, h.run(head)) + assert.NotContains(t, h.filed, entity.QueueBatchState{Queue: "q", State: entity.BatchStateSpeculating, BatchID: straggler.ID}) + assert.Equal(t, 1, spec.calls, "the rest of the queue is still planned") +} diff --git a/submitqueue/orchestrator/pipeline.go b/submitqueue/orchestrator/pipeline.go index 60cfef5e..795bfc29 100644 --- a/submitqueue/orchestrator/pipeline.go +++ b/submitqueue/orchestrator/pipeline.go @@ -36,6 +36,7 @@ import ( "github.com/uber/submitqueue/submitqueue/orchestrator/controller/buildsignal" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/cancel" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/conclude" + "github.com/uber/submitqueue/submitqueue/orchestrator/controller/dependencyanalysis" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/dlq" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/merge" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/mergeconflictsignal" @@ -139,12 +140,23 @@ var Stages = []pipeline.Stage[Deps]{ Name: "batch", ConsumerGroup: "orchestrator", New: func(d Deps, sc pipeline.StageContext) (consumer.Controller, error) { - return batch.NewController(d.Logger, d.Scope, sc.Registry, d.Counter, d.Storage, d.Analyzer, sc.TopicKey, sc.ConsumerGroup), nil + return batch.NewController(d.Logger, d.Scope, sc.Registry, d.Counter, d.Storage, sc.TopicKey, sc.ConsumerGroup), nil }, DLQ: func(d Deps, sc pipeline.StageContext) (consumer.Controller, error) { return dlq.NewDLQRequestController(d.Logger, d.Scope, d.Storage, sc.Registry, dlq.DecodeRequestID, sc.TopicKey, sc.ConsumerGroup), nil }, }, + { + Key: topickey.TopicKeyDependencyAnalysis, + Name: "dependency-analysis", + ConsumerGroup: "orchestrator", + New: func(d Deps, sc pipeline.StageContext) (consumer.Controller, error) { + return dependencyanalysis.NewController(d.Logger, d.Scope, d.Storage, d.Analyzer, sc.Registry, sc.TopicKey, sc.ConsumerGroup), nil + }, + DLQ: func(d Deps, sc pipeline.StageContext) (consumer.Controller, error) { + return dlq.NewDLQBatchController(d.Logger, d.Scope, d.Storage, sc.Registry, sc.TopicKey, sc.ConsumerGroup), nil + }, + }, { Key: topickey.TopicKeySpeculate, Name: "speculate", diff --git a/test/e2e/submitqueue/BUILD.bazel b/test/e2e/submitqueue/BUILD.bazel index f9b159ac..a2f1c713 100644 --- a/test/e2e/submitqueue/BUILD.bazel +++ b/test/e2e/submitqueue/BUILD.bazel @@ -39,8 +39,13 @@ go_test( "//api/runway/messagequeue:go_default_library", "//api/submitqueue/gateway/protopb:go_default_library", "//api/submitqueue/orchestrator/protopb:go_default_library", + "//platform/consumer:go_default_library", "//platform/extension/consumergate:go_default_library", "//platform/extension/consumergate/file:go_default_library", + "//platform/extension/messagequeue/mysql:go_default_library", + "//platform/publish:go_default_library", + "//submitqueue/core/batch:go_default_library", + "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", "//test/testutil:go_default_library", @@ -51,5 +56,6 @@ go_test( "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//codes:go_default_library", "@org_golang_google_grpc//status:go_default_library", + "@org_uber_go_zap//:go_default_library", ], ) diff --git a/test/e2e/submitqueue/harness_test.go b/test/e2e/submitqueue/harness_test.go index c3f229e4..cb149ae5 100644 --- a/test/e2e/submitqueue/harness_test.go +++ b/test/e2e/submitqueue/harness_test.go @@ -31,11 +31,18 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber-go/tally" changepb "github.com/uber/submitqueue/api/base/change/protopb" mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" gatewaypb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/extension/consumergate" + queuemysql "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" + "github.com/uber/submitqueue/platform/publish" + corebatch "github.com/uber/submitqueue/submitqueue/core/batch" + "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" + "go.uber.org/zap" ) func pollUntil(interval time.Duration, condition func() bool) { @@ -247,6 +254,90 @@ func (s *E2EIntegrationSuite) assertStatusCount(req request, status entity.Reque "GetRequestHistoryByID for %s should record %q %d time(s); got %v", req.sqid, status, want, got) } +// batchIDsFor returns every batch the request has ever been associated with, +// so a test can assert that a redelivery did not leave a second one behind. +func (s *E2EIntegrationSuite) batchIDsFor(req request) []string { + t := s.T() + store, err := s.appStorage.For(req.queue) + require.NoError(t, err, "failed to resolve operating store for queue %s", req.queue) + + associations, err := store.GetRequestBatchStore().GetByRequestID(s.ctx, req.sqid) + require.NoError(t, err, "failed to read batch associations for %s", req.sqid) + + ids := make([]string, 0, len(associations)) + for _, a := range associations { + ids = append(ids, a.BatchID) + } + return ids +} + +// redeliverBatchMessage re-publishes the request onto the batch topic, which is +// exactly what the pipeline sees when a batch delivery is retried after its ack +// was lost. +// +// The message ID has to be a fresh one: the queue deduplicates on (topic, +// partition key, message ID) against rows it has not collected yet, so reusing +// the original would make this publish a silent no-op. +func (s *E2EIntegrationSuite) redeliverBatchMessage(req request) { + t := s.T() + + queue, err := queuemysql.NewQueue(queuemysql.Params{ + DB: s.queueDB, + Logger: zap.NewNop(), + MetricsScope: tally.NoopScope, + }) + require.NoError(t, err, "failed to open the queue for a manual publish") + defer func() { require.NoError(t, queue.Close()) }() + + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: topickey.TopicKeyBatch, Name: topickey.TopicKeyBatch.String(), Queue: queue}, + }) + require.NoError(t, err) + + payload, err := entity.RequestID{ID: req.sqid, Queue: req.queue}.ToBytes() + require.NoError(t, err) + + require.NoError(t, publish.Message(s.ctx, registry, topickey.TopicKeyBatch, + publish.UniqueID(req.sqid), payload, req.queue), "failed to redeliver the batch message") + s.log.Logf("Redelivered the batch message for %s", req.sqid) +} + +// batchState reads a batch's current state from the operating store. +func (s *E2EIntegrationSuite) batchState(queue, batchID string) entity.BatchState { + t := s.T() + store, err := s.appStorage.For(queue) + require.NoError(t, err, "failed to resolve operating store for queue %s", queue) + batch, err := store.GetBatchStore().Get(s.ctx, batchID) + require.NoError(t, err, "failed to read batch %s", batchID) + return batch.State +} + +// awaitBatchState polls the operating store until the batch reaches want. +func (s *E2EIntegrationSuite) awaitBatchState(queue, batchID string, want entity.BatchState) { + pollUntil(persistPollInterval, func() bool { + got := s.batchState(queue, batchID) + s.log.Logf("Batch %s is %q (want %q)", batchID, got, want) + return got == want + }) +} + +// strandInCreated puts a batch back into Created, reproducing the state a batch +// is left in when it is promoted but its announcement never reaches speculate. +// Nothing will name it on the speculate topic again, so only a run that looks +// for it can move it on. +func (s *E2EIntegrationSuite) strandInCreated(queue, batchID string) { + t := s.T() + store, err := s.appStorage.For(queue) + require.NoError(t, err, "failed to resolve operating store for queue %s", queue) + + batch, err := store.GetBatchStore().Get(s.ctx, batchID) + require.NoError(t, err, "failed to read batch %s", batchID) + + _, err = corebatch.Transition(s.ctx, store, batch, entity.BatchStateCreated) + require.NoError(t, err, "failed to strand batch %s in created", batchID) + s.log.Logf("Stranded batch %s in created with no announcement in flight", batchID) +} + // closeGate closes the consumer gate for the consumer group, scoped to one // partition (the queue name for pipeline topics). The gate must be closed // before the message that must be caught is published — that makes the stop diff --git a/test/e2e/submitqueue/suite_test.go b/test/e2e/submitqueue/suite_test.go index 323fadb9..7097647d 100644 --- a/test/e2e/submitqueue/suite_test.go +++ b/test/e2e/submitqueue/suite_test.go @@ -575,3 +575,90 @@ func (s *E2EIntegrationSuite) TestCancel_CaughtPreBatch_NeverLands() { "request %s must stay terminal cancelled after its stale check signal is processed", req.sqid) s.assertStatusesNever(req, entity.RequestStatusBatched, entity.RequestStatusLanded) } + +// A batch delivery that is retried after its ack was lost must not enrol the +// request into a second batch. Both batches would be analyzed, promoted and +// admitted, and both would merge the same change. +// +// The build gate is what makes the redelivery land in the window that matters: +// with the build for the first batch held, the request cannot reach a terminal +// state, so the retry finds it exactly as a lost ack would — claimed, carried +// by a live batch, and still in flight. +// +// The second request is the settle signal. The batch topic is partitioned by +// queue and consumed in order, so its batch existing proves the redelivered +// message ahead of it has already been handled, and the association count is +// safe to assert. +func (s *E2EIntegrationSuite) TestBatchRedelivery_DoesNotEnrolTheRequestTwice() { + t := s.T() + + const queue = "e2e-redelivery-queue" + const gateGroup = "orchestrator" + // Nothing has landed on this queue, so the first batch is predictable, and + // the build topic partitions by batch ID. + const heldBatch = queue + "/batch/1" + + s.closeGate(gateGroup, heldBatch, "e2e: hold the build so the request stays in flight for the redelivery") + // Reopen even if an assertion below fails, so teardown does not stop the + // stack with a delivery still parked. Opening twice is a no-op. + defer s.openGate(gateGroup, heldBatch) + + req := s.land(queue, "github://github.example.com/uber/e2e-redelivery/pull/1/abcdef0123456789abcdef0123456789abcdef01") + require.Equal(t, heldBatch, s.awaitBatchID(req), "the first batch of a fresh queue must be batch/1") + s.awaitStatus(req, entity.RequestStatusSpeculating) + + s.redeliverBatchMessage(req) + + settle := s.land(queue, "github://github.example.com/uber/e2e-redelivery/pull/2/1234567890abcdef1234567890abcdef12345678") + settleBatch := s.awaitBatchID(settle) + require.NotEqual(t, heldBatch, settleBatch) + + assert.Equal(t, []string{heldBatch}, s.batchIDsFor(req), + "the redelivery must resume the existing batch, not mint another") + + s.openGate(gateGroup, heldBatch) + s.awaitStatus(req, entity.RequestStatusLanded) + s.awaitStatus(settle, entity.RequestStatusLanded) +} + +// A batch left in Created is dependency-eligible, so everything created after +// it serializes behind it — and nothing will name it on the speculate topic +// again. Without a run that looks for such batches the queue wedges here with +// no way out, which is how CODEM-444 stalled a 100-PR run. +// +// The strand is built from a real batch rather than seeded rows so the request, +// its association and its path set are all genuine: the only thing missing is +// the announcement, which is exactly what the bug destroyed. +// +// Against a pipeline without the repair this test does not fail fast — the +// batch simply never moves and the suite runs to Bazel's timeout, which is how +// this harness reports a stall. +func (s *E2EIntegrationSuite) TestStrandedBatch_IsAdmittedByALaterRun() { + t := s.T() + + const queue = "e2e-strand-queue" + const gateGroup = "orchestrator" + const heldBatch = queue + "/batch/1" + + s.closeGate(gateGroup, heldBatch, "e2e: hold the build so the batch can be stranded while still in flight") + defer s.openGate(gateGroup, heldBatch) + + req := s.land(queue, "github://github.example.com/uber/e2e-strand/pull/1/abcdef0123456789abcdef0123456789abcdef01") + require.Equal(t, heldBatch, s.awaitBatchID(req), "the first batch of a fresh queue must be batch/1") + s.awaitStatus(req, entity.RequestStatusSpeculating) + + // Its build is parked, so the queue is quiet and this write cannot race. + s.strandInCreated(queue, heldBatch) + require.Equal(t, entity.BatchStateCreated, s.batchState(queue, heldBatch)) + + // Any later request wakes the queue; the run it triggers is what has to + // notice the stranded batch. + trigger := s.land(queue, "github://github.example.com/uber/e2e-strand/pull/2/1234567890abcdef1234567890abcdef12345678") + s.awaitBatchID(trigger) + + s.awaitBatchState(queue, heldBatch, entity.BatchStateSpeculating) + + s.openGate(gateGroup, heldBatch) + s.awaitStatus(req, entity.RequestStatusLanded) + s.awaitStatus(trigger, entity.RequestStatusLanded) +}