fix(batch): split dependency analysis out of batch creation

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: <what>` or `// TODO(topic): <what>`, 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.

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` |

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 "<queue>/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.

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",
     ],

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
+}

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)
+		})
+	}
+}

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.