From ef0af011b6c46235d2bb6994e0f5993233400161 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Fri, 7 Aug 2026 14:08:28 -0700 Subject: [PATCH] fix(orchestrator): stamp the queue on path sets and path-build links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? Speculation never funded a path, so no build was ever dispatched and the submitqueue e2e hung until Bazel killed it at 300s, having spilled ~11MB of retry logs. Two producers build an entity without naming its queue, and both of their stores are bound to one queue and reject a write that disagrees: - `speculate/dispatch.go` created a head's first `SpeculationPathSet` as `{Head: batch.ID}`, so `Create` failed with `queue "" does not match the store's bound queue`. That is the first write of every newly funded head, so no head was ever funded. - `build/build.go` created the `PathBuild` link as `{PathID, Attempt, BuildID}`, so the link write failed the same way. Without the link a path is never observed moving to `building`, so it stays `pending` and every later run re-dispatches it. Both stores gained their queue-bound guard when their tables were re-keyed to lead with the queue (#543 for `speculation_path_set`, #502 for `path_build`); the producers were not updated to match. Unit tests missed it because they use the gomock stores, which do not enforce the binding, and the storage contract suite missed it because it builds its own fixtures with the queue set. Only the e2e exercises a real producer against a real queue-bound store. ### What? Stamps the queue at both construction sites: the path set takes the run's queue, the link takes its batch's. Turns both into regression tests rather than leaving them to the e2e: the build controller's link expectations are exact struct matches that now require the queue, and the speculate run test asserts the created set names it. Each fails against the unfixed code. ## Test Plan ✅ `go test ./submitqueue/... ./service/...` ✅ Verified each new assertion fails when its fix is reverted ✅ `make lint` / `make check-gazelle` --- submitqueue/orchestrator/controller/build/build.go | 2 +- submitqueue/orchestrator/controller/build/build_test.go | 6 +++--- submitqueue/orchestrator/controller/speculate/dispatch.go | 2 +- submitqueue/orchestrator/controller/speculate/run_test.go | 4 ++++ 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/submitqueue/orchestrator/controller/build/build.go b/submitqueue/orchestrator/controller/build/build.go index 4a0880d6..ce770ad7 100644 --- a/submitqueue/orchestrator/controller/build/build.go +++ b/submitqueue/orchestrator/controller/build/build.go @@ -260,7 +260,7 @@ func (c *Controller) startPath(ctx context.Context, store storage.Storage, batch return fmt.Errorf("failed to record build %s: %w", buildID.ID, err) } - link := entity.PathBuild{PathID: entry.ID, Attempt: entry.Attempt, BuildID: buildID.ID} + link := entity.PathBuild{Queue: batch.Queue, PathID: entry.ID, Attempt: entry.Attempt, BuildID: buildID.ID} if err := store.GetPathBuildStore().Create(ctx, link); err != nil { if errors.Is(err, storage.ErrAlreadyExists) { // Another dispatch named this attempt first. Its build is the one diff --git a/submitqueue/orchestrator/controller/build/build_test.go b/submitqueue/orchestrator/controller/build/build_test.go index 44d4f4a3..5ade579d 100644 --- a/submitqueue/orchestrator/controller/build/build_test.go +++ b/submitqueue/orchestrator/controller/build/build_test.go @@ -231,7 +231,7 @@ func TestProcess_TriggersWithThePathsBase(t *testing.T) { return nil }), deps.pathBuilds.EXPECT().Create(gomock.Any(), entity.PathBuild{ - PathID: entry.ID, Attempt: 1, BuildID: "build-1", + Queue: "test-queue", PathID: entry.ID, Attempt: 1, BuildID: "build-1", }).Return(nil), deps.publisher.EXPECT().Publish(gomock.Any(), "buildsignal", gomock.Any()). DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { @@ -270,7 +270,7 @@ func TestProcess_NeverWritesThePathSetAndIgnoresCancelling(t *testing.T) { Return(entity.BuildID{ID: "build-1"}, nil) deps.builds.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) deps.pathBuilds.EXPECT().Create(gomock.Any(), entity.PathBuild{ - PathID: pending.ID, Attempt: 1, BuildID: "build-1", + Queue: "test-queue", PathID: pending.ID, Attempt: 1, BuildID: "build-1", }).Return(nil) expectSignal(t, deps, "build-1") @@ -336,7 +336,7 @@ func TestProcess_LostDispatchRaceHandsBothBuildsToThePollLoop(t *testing.T) { deps.pathBuilds.EXPECT().Get(gomock.Any(), entry.ID, 1). Return(entity.PathBuild{}, storage.ErrNotFound), deps.pathBuilds.EXPECT().Create(gomock.Any(), entity.PathBuild{ - PathID: entry.ID, Attempt: 1, BuildID: "build-surplus", + Queue: "test-queue", PathID: entry.ID, Attempt: 1, BuildID: "build-surplus", }).Return(storage.ErrAlreadyExists), deps.pathBuilds.EXPECT().Get(gomock.Any(), entry.ID, 1). Return(entity.PathBuild{PathID: entry.ID, Attempt: 1, BuildID: "build-winner"}, nil), diff --git a/submitqueue/orchestrator/controller/speculate/dispatch.go b/submitqueue/orchestrator/controller/speculate/dispatch.go index 2d061ff7..f802b7ed 100644 --- a/submitqueue/orchestrator/controller/speculate/dispatch.go +++ b/submitqueue/orchestrator/controller/speculate/dispatch.go @@ -46,7 +46,7 @@ func (c *Controller) dispatch(ctx context.Context, store storage.Storage, queue // at all, because nothing marked it changed. set, exists := snap.pathSets[batch.ID] if !exists { - set = entity.SpeculationPathSet{Head: batch.ID} + set = entity.SpeculationPathSet{Queue: queue, Head: batch.ID} } changed := snap.isDirty(batch.ID) diff --git a/submitqueue/orchestrator/controller/speculate/run_test.go b/submitqueue/orchestrator/controller/speculate/run_test.go index d4245b84..ed3f3f8f 100644 --- a/submitqueue/orchestrator/controller/speculate/run_test.go +++ b/submitqueue/orchestrator/controller/speculate/run_test.go @@ -159,6 +159,10 @@ func TestRun_FundsProposedPath(t *testing.T) { h.pathSets.EXPECT().Create(gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, set entity.SpeculationPathSet) error { require.Len(t, set.Paths, 1) + // The set must name the queue it belongs to: the store is bound to + // one queue and refuses a write that disagrees, so an unstamped set + // means no head is ever funded. + assert.Equal(t, "q", set.Queue) assert.Equal(t, path.ID(), set.Paths[0].ID) assert.Equal(t, entity.SpeculationPathStatusPending, set.Paths[0].Status) assert.Equal(t, 1, set.Paths[0].Attempt)