feat(buildrunner): drop per-call queueName, add Factory

doc/rfc/build-runner.md

@@ -38,12 +38,27 @@ The build stage needs a vendor-agnostic abstraction for talking to a Build Runne
 
 `BuildRunner` exposes three verbs, all keyed by a build identifier (`entity.BuildID`):
 
-- **`Trigger`** — submit a build for a queue, given the ordered `base` and `head` change sets plus a free-form metadata map; returns the new build's ID. Runner-side work is asynchronous.
+- **`Trigger`** — submit a build given the ordered `base` and `head` change sets plus a free-form metadata map; returns the new build's ID. Runner-side work is asynchronous.
 - **`Status`** — fetch the current `BuildStatus` and runner-defined metadata for a build; MAY round-trip to the runner.
 - **`Cancel`** — request cancellation; returns once the request reaches the runner, not once the build stops.
 
 See `extension/buildrunner/build_runner.go` for the exact Go signatures. The sections below record why the contract is shaped this way.
 
+### Construction: a Factory, queue bound at build time
+
+A `BuildRunner` does not take a queue selector on any verb. The queue whose job configuration a runner uses is fixed when the runner is constructed, and runners are constructed by a `Factory`.
+
+- **`Factory`** — produces `BuildRunner` instances from a `Config`. A controller that drives builds for several queues holds one `Factory` and obtains one `BuildRunner` per queue.
+- **`Config`** — the configuration the factory binds in: a `QueueID` selecting the queue whose job definition the runner builds against, plus any backend-specific settings (endpoints, credentials, defaults) a concrete implementation adds.
+
+Why bind the queue at construction rather than pass it per call:
+
+- A runner's connection pool, caches, and job defaults are all keyed to one queue's configuration. Passing the queue per call would force every implementation to re-resolve that configuration on the hot path, or to maintain an internal queue→config map the factory already expresses cleanly.
+- It keeps the per-call verbs (`Trigger`, `Status`, `Cancel`) free of routing concerns — they speak only in builds and changes.
+- It matches the rest of the extension family, whose implementations are bound to their configuration at construction.
+
+Rejected: a `queueName` argument on `Trigger`. It put routing on the hot path and on a single verb, leaving `Status` and `Cancel` to rediscover the queue from the build ID. Carrying the selection in `Config` keeps each runner bound to a single queue.
+
 ### Trigger: base + head
 
 `Trigger` takes two ordered lists of changes and a free-form metadata map:
@@ -125,7 +140,7 @@ Rejected: long-polling on `Status`. Not every backend supports efficient server-
 
 ### Lifecycle
 
-Implementations are long-lived singletons bound to provider config at construction. Every method is concurrent-safe; connection pools and caches live inside the manager; anything that must survive a restart belongs in persistent storage, not the manager.
+Implementations are constructed by a `Factory` and bound to one queue's provider config at construction (see *Construction* above). They may be shared and called concurrently, so every method must be concurrent-safe; connection pools and caches live inside the manager; anything that must survive a restart belongs in persistent storage, not the manager.
 
 ### Transient failures
 

example/server/orchestrator/main.go

@@ -218,12 +218,13 @@ func run() error {
 		return fmt.Errorf("failed to create pusher: %w", err)
 	}
 
-	// Create build runner. The noop runner is the pass-through default
-	// (every build immediately succeeds) until a real backend is wired in.
-	br := buildnoop.New()
+	// Create the build runner factory. The noop factory is the pass-through
+	// default (every build immediately succeeds) until a real backend is
+	// wired in; controllers build a runner per queue from it.
+	runnerFactory := buildnoop.NewFactory()
 
 	// Register controllers
-	if err := registerControllers(c, logger.Sugar(), scope, registry, mc, cp, psh, br, cnt, store, changeStore); err != nil {
+	if err := registerControllers(c, logger.Sugar(), scope, registry, mc, cp, psh, runnerFactory, cnt, store, changeStore); err != nil {
 		return err
 	}
 
@@ -408,7 +409,7 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe
 //                                        │        │                       │
 //                                        └────────┴───────────────────────┘
 
-func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, mc mergechecker.MergeChecker, cp changeprovider.ChangeProvider, psh pusher.Pusher, br buildrunner.BuildRunner, cnt counter.Counter, store storage.Storage, changeStore changestore.ChangeStore) error {
+func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, mc mergechecker.MergeChecker, cp changeprovider.ChangeProvider, psh pusher.Pusher, runnerFactory buildrunner.Factory, cnt counter.Counter, store storage.Storage, changeStore changestore.ChangeStore) error {
 	requestController := start.NewController(
 		logger,
 		scope,
@@ -494,7 +495,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t
 		logger,
 		scope,
 		store,
-		br,
+		runnerFactory,
 		registry,
 		consumer.TopicKeyBuild,
 		"orchestrator-build",
@@ -507,7 +508,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t
 		logger,
 		scope,
 		store,
-		br,
+		runnerFactory,
 		registry,
 		consumer.TopicKeyBuildSignal,
 		"orchestrator-buildsignal",

extension/buildrunner/README.md

@@ -6,7 +6,7 @@ See [`doc/rfc/build-runner.md`](../../doc/rfc/build-runner.md) for the contract
 
 ## Adding a new backend
 
-1. Create `extension/buildrunner/{backend}/` with a `BuildRunner` implementation bound to its runner configuration at construction.
+1. Create `extension/buildrunner/{backend}/` with a `Factory` whose `New` returns a `BuildRunner` bound to one queue's job configuration. The runner verbs carry no queue selector — that selection lives in the `Config` passed to the factory.
 2. Map the `base` and `head` change slices onto the backend's build primitives (apply `base`, apply `head`, validate the result).
 3. Map the runner's lifecycle states down to the `BuildStatus` values: `Accepted` (accepted for execution), `Running` (executing), and the terminal `Succeeded` / `Failed` / `Cancelled`.
 4. Implement internal reconnect / retry so transient failures surface as plain errors without blocking the caller.

extension/buildrunner/build_runner.go

@@ -22,10 +22,34 @@ import (
 	"github.com/uber/submitqueue/entity"
 )
 
+// Config carries the configuration a Factory binds into a BuildRunner: the
+// queue whose job definition the runner builds against, plus any
+// backend-specific settings (endpoints, credentials, defaults) a concrete
+// implementation adds.
+type Config struct {
+	// QueueID identifies the queue whose job configuration this runner
+	// builds against.
+	QueueID string
+}
+
+// Factory constructs BuildRunner instances from a Config. A BuildRunner is
+// bound to its Config at construction; the per-build verbs take no queue
+// selector. A controller that serves multiple queues holds a Factory and
+// calls New with each batch's queue (see Config.QueueID).
+//
+// Implementations must be safe for concurrent use by multiple goroutines.
+type Factory interface {
+	// New returns a BuildRunner for cfg, ready to trigger builds. Returns an
+	// error if a runner cannot be constructed from cfg (e.g. invalid
+	// configuration or an unreachable backend).
+	New(cfg Config) (BuildRunner, error)
+}
+
 // BuildRunner triggers builds against an external Build Runner, queries
-// their status, and cancels them.
+// their status, and cancels them. A BuildRunner is bound to its Config at
+// construction (see Factory); the verbs below take no queue selector.
 //
-// Implementations are long-lived singletons and must:
+// Implementations may be shared and called concurrently, and must:
 //   - make every method safe for concurrent use by multiple goroutines;
 //   - recover from transient connectivity failures internally, returning
 //     plain errors during the recovery window rather than blocking the
@@ -55,11 +79,9 @@ type BuildRunner interface {
 	// asynchronously. Callers learn the build's progress via Status, not
 	// via Trigger.
 	//
-	// queueName selects the runner-specific job configuration.
 	// Returns an error if the request is invalid.
 	Trigger(
 		ctx context.Context,
-		queueName string,
 		base []entity.Change,
 		head []entity.Change,
 		metadata entity.BuildMetadata,

extension/buildrunner/mock/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//entity",
+        "//extension/buildrunner",
         "@org_uber_go_mock//gomock",
     ],
 )

extension/buildrunner/noop/noop.go

@@ -34,14 +34,27 @@ type runner struct {
 	counter atomic.Uint64
 }
 
+// factory builds no-op runners. It ignores the supplied Config.
+type factory struct{}
+
+// NewFactory returns a buildrunner.Factory that produces no-op runners.
+func NewFactory() buildrunner.Factory {
+	return factory{}
+}
+
+// New returns a no-op buildrunner.BuildRunner. The Config is ignored.
+func (factory) New(_ buildrunner.Config) (buildrunner.BuildRunner, error) {
+	return New(), nil
+}
+
 // New returns a buildrunner.BuildRunner that performs no real work.
 func New() buildrunner.BuildRunner {
 	return &runner{}
 }
 
 // Trigger returns a unique build ID without contacting any runner.
 // Inputs are ignored.
-func (r *runner) Trigger(_ context.Context, _ string, _ []entity.Change, _ []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
+func (r *runner) Trigger(_ context.Context, _ []entity.Change, _ []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
 	return entity.BuildID{ID: fmt.Sprintf("noop-%d", r.counter.Add(1))}, nil
 }
 

extension/buildrunner/noop/noop_test.go

@@ -28,11 +28,18 @@ func TestNew_ImplementsInterface(t *testing.T) {
 	var _ buildrunner.BuildRunner = New()
 }
 
+func TestNewFactory_ImplementsInterface(t *testing.T) {
+	var f buildrunner.Factory = NewFactory()
+	r, err := f.New(buildrunner.Config{QueueID: "queueA"})
+	require.NoError(t, err)
+	assert.NotNil(t, r)
+}
+
 func TestRunner_Trigger(t *testing.T) {
 	r := New()
 	ctx := context.Background()
 
-	id1, err := r.Trigger(ctx, "queueA",
+	id1, err := r.Trigger(ctx,
 		[]entity.Change{{URIs: []string{"github://owner/repo/pull/1"}}},
 		[]entity.Change{{URIs: []string{"github://owner/repo/pull/2"}}},
 		entity.BuildMetadata{"requester": "alice"},
@@ -41,7 +48,7 @@ func TestRunner_Trigger(t *testing.T) {
 	assert.NotEmpty(t, id1.ID)
 
 	// IDs are unique across calls, even with empty inputs.
-	id2, err := r.Trigger(ctx, "queueA", nil, nil, nil)
+	id2, err := r.Trigger(ctx, nil, nil, nil)
 	require.NoError(t, err)
 	assert.NotEqual(t, id1, id2)
 }

orchestrator/controller/build/build.go

@@ -36,7 +36,7 @@ type Controller struct {
 	logger        *zap.SugaredLogger
 	metricsScope  tally.Scope
 	store         storage.Storage
-	buildRunner   buildrunner.BuildRunner
+	runnerFactory buildrunner.Factory
 	registry      consumer.TopicRegistry
 	topicKey      consumer.TopicKey
 	consumerGroup string
@@ -50,7 +50,7 @@ func NewController(
 	logger *zap.SugaredLogger,
 	scope tally.Scope,
 	store storage.Storage,
-	buildRunner buildrunner.BuildRunner,
+	runnerFactory buildrunner.Factory,
 	registry consumer.TopicRegistry,
 	topicKey consumer.TopicKey,
 	consumerGroup string,
@@ -59,7 +59,7 @@ func NewController(
 		logger:        logger.Named("build_controller"),
 		metricsScope:  scope.SubScope("build_controller"),
 		store:         store,
-		buildRunner:   buildRunner,
+		runnerFactory: runnerFactory,
 		registry:      registry,
 		topicKey:      topicKey,
 		consumerGroup: consumerGroup,
@@ -112,10 +112,16 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
 		return fmt.Errorf("failed to assemble head changes for batch %s: %w", batch.ID, err)
 	}
 
-	// Trigger the build with the configured build manager. metadata is nil
-	// until a caller-supplied source materializes (e.g. requester / ticket
-	// pulled off the originating LandRequest).
-	buildID, err := c.buildRunner.Trigger(ctx, batch.Queue, base, head, nil)
+	// Resolve the BuildRunner for this batch's queue and trigger the build.
+	// metadata is nil until a caller-supplied source materializes (e.g.
+	// requester / ticket pulled off the originating LandRequest).
+	runner, err := c.runnerFactory.New(buildrunner.Config{QueueID: batch.Queue})
+	if err != nil {
+		metrics.NamedCounter(c.metricsScope, opName, "runner_errors", 1)
+		return fmt.Errorf("failed to create build runner for queue %s: %w", batch.Queue, err)
+	}
+
+	buildID, err := runner.Trigger(ctx, base, head, nil)
 	if err != nil {
 		metrics.NamedCounter(c.metricsScope, opName, "trigger_errors", 1)
 		return fmt.Errorf("failed to trigger build for batch %s: %w", batch.ID, err)