diff --git a/doc/rfc/submitqueue/modular-queue-wiring.md b/doc/rfc/submitqueue/modular-queue-wiring.md index 0e7e828f7..c63a59479 100644 --- a/doc/rfc/submitqueue/modular-queue-wiring.md +++ b/doc/rfc/submitqueue/modular-queue-wiring.md @@ -466,7 +466,7 @@ func run(ctx context.Context) error { ChangeProvider(github.New(cfg.GitHub)). BuildRunner(local.New()). Scorer(heuristic.New()). - ConflictAnalyzer(fileoverlap.New()), + ConflictAnalyzer(pathoverlap.New()), ). Option(pipeline.TopicNames(cfg.TopicNames)). Option(pipeline.Classifiers(backendClassifiers())). @@ -497,7 +497,7 @@ app, err := submitqueue.New(). ). Queue(base.Named("monorepo/exp"). BuildRunner(local.New()). - ConflictAnalyzer(fileoverlap.New()), + ConflictAnalyzer(pathoverlap.New()), ). Queue(base.Named("monorepo/test"). BuildRunner(noop.New()). diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 436453a2e..bd113c1e9 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -37,8 +37,8 @@ go_library( "//submitqueue/extension/conflict:go_default_library", "//submitqueue/extension/conflict/all:go_default_library", "//submitqueue/extension/conflict/fake:go_default_library", - "//submitqueue/extension/conflict/fileoverlap:go_default_library", "//submitqueue/extension/conflict/none:go_default_library", + "//submitqueue/extension/conflict/pathoverlap:go_default_library", "//submitqueue/extension/scorer:go_default_library", "//submitqueue/extension/scorer/composite:go_default_library", "//submitqueue/extension/scorer/fake:go_default_library", diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index a59d25459..02aeac713 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -27,8 +27,8 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/conflict/all" conflictfake "github.com/uber/submitqueue/submitqueue/extension/conflict/fake" - "github.com/uber/submitqueue/submitqueue/extension/conflict/fileoverlap" "github.com/uber/submitqueue/submitqueue/extension/conflict/none" + "github.com/uber/submitqueue/submitqueue/extension/conflict/pathoverlap" "github.com/uber/submitqueue/submitqueue/extension/scorer" "github.com/uber/submitqueue/submitqueue/extension/scorer/composite" scorerfake "github.com/uber/submitqueue/submitqueue/extension/scorer/fake" @@ -256,9 +256,10 @@ func newProfiles(logger *zap.Logger, scope tally.Scope, resolver changeset.Resol // file-overlap-queue: a real analyzer that serializes only batches sharing // a changed file, resolving each batch's files itself via the resolver. + // pathoverlap.ByDirectory would coarsen this to whole directories. fileOverlapQueue := base fileOverlapQueue.Analyzer = analyzerFunc(func(c conflict.Config) (conflict.Analyzer, error) { - return fileoverlap.New(c, resolver), nil + return pathoverlap.New(c, resolver, pathoverlap.ByFile), nil }) // e2e-test-queue: composite scorer; no conflicts (maximum parallelism). diff --git a/submitqueue/extension/conflict/fileoverlap/README.md b/submitqueue/extension/conflict/fileoverlap/README.md deleted file mode 100644 index d63cf6d39..000000000 --- a/submitqueue/extension/conflict/fileoverlap/README.md +++ /dev/null @@ -1,9 +0,0 @@ -# fileoverlap - -`fileoverlap` is a `conflict.Analyzer` that reports a conflict between two batches when they change one or more of the same files. - -## Behavior - -The files a batch changes are drawn from each change's provider-supplied details. The candidate batch conflicts with an in-flight batch when their changed-file sets intersect; each such in-flight batch is reported once, preserving the in-flight order. A shared file is the concrete notion of *target overlap*, so conflicts are classified as `ConflictTypeTargetOverlap`. A batch that changes no files conflicts with nothing, and an empty in-flight list yields no conflicts. A failure to resolve a batch's changes is returned as a (retryable) error. - -File-path intersection is a deliberately simple notion of overlap. A richer one (build targets, ownership boundaries) would be a separate analyzer rather than a change to this one. diff --git a/submitqueue/extension/conflict/fileoverlap/fileoverlap.go b/submitqueue/extension/conflict/fileoverlap/fileoverlap.go deleted file mode 100644 index cc19f1fbc..000000000 --- a/submitqueue/extension/conflict/fileoverlap/fileoverlap.go +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright (c) 2025 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 fileoverlap provides a conflict.Analyzer that reports a conflict -// between two batches when they change one or more of the same files. It is the -// first analyzer to use the capability the extension contract unblocks: it takes -// only batch identity and resolves each batch's changed files itself through an -// injected changeset resolver, rather than depending on the controller to -// pre-compute them. A shared file is the concrete notion of target overlap, so -// it reports entity.ConflictTypeTargetOverlap. -package fileoverlap - -import ( - "context" - "fmt" - - "github.com/uber/submitqueue/submitqueue/core/changeset" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/conflict" -) - -// analyzer reports a conflict between batches that change a common file. The -// files a batch changes are resolved from each batch's change details. -type analyzer struct { - // cfg is the per-queue identity this analyzer was built for. - cfg conflict.Config - resolver changeset.Resolver -} - -// New returns a conflict.Analyzer that flags an in-flight batch as conflicting -// when it changes a file the candidate batch also changes, bound to the queue -// named in cfg. The resolver resolves each batch's changed files. -func New(cfg conflict.Config, resolver changeset.Resolver) conflict.Analyzer { - return analyzer{cfg: cfg, resolver: resolver} -} - -// Analyze returns one ConflictTypeTargetOverlap Conflict per in-flight batch -// that shares a changed file with batch, preserving the in-flight order. A batch -// that changes no files conflicts with nothing. -func (a analyzer) Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]entity.Conflict, error) { - if len(inFlight) == 0 { - return nil, nil - } - - candidate, err := a.files(ctx, batch) - if err != nil { - return nil, fmt.Errorf("failed to resolve files for batch %s: %w", batch.ID, err) - } - if len(candidate) == 0 { - return nil, nil - } - - var conflicts []entity.Conflict - for _, other := range inFlight { - files, err := a.files(ctx, other) - if err != nil { - return nil, fmt.Errorf("failed to resolve files for batch %s: %w", other.ID, err) - } - if intersects(candidate, files) { - conflicts = append(conflicts, entity.Conflict{ - BatchID: other.ID, - Type: entity.ConflictTypeTargetOverlap, - }) - } - } - return conflicts, nil -} - -// files resolves the set of file paths the batch changes. -func (a analyzer) files(ctx context.Context, batch entity.Batch) (map[string]struct{}, error) { - changes, err := a.resolver.DetailedForBatch(ctx, batch) - if err != nil { - return nil, err - } - files := make(map[string]struct{}) - for _, change := range changes.Changes { - for _, file := range change.Details.ChangedFiles { - files[file.Path] = struct{}{} - } - } - return files, nil -} - -// intersects reports whether the two sets share any element. -func intersects(a, b map[string]struct{}) bool { - // Iterate the smaller set for fewer lookups. - if len(b) < len(a) { - a, b = b, a - } - for k := range a { - if _, ok := b[k]; ok { - return true - } - } - return false -} diff --git a/submitqueue/extension/conflict/fileoverlap/BUILD.bazel b/submitqueue/extension/conflict/pathoverlap/BUILD.bazel similarity index 89% rename from submitqueue/extension/conflict/fileoverlap/BUILD.bazel rename to submitqueue/extension/conflict/pathoverlap/BUILD.bazel index a32b277a5..be667b4cb 100644 --- a/submitqueue/extension/conflict/fileoverlap/BUILD.bazel +++ b/submitqueue/extension/conflict/pathoverlap/BUILD.bazel @@ -2,8 +2,8 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["fileoverlap.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/conflict/fileoverlap", + srcs = ["pathoverlap.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/conflict/pathoverlap", visibility = ["//visibility:public"], deps = [ "//submitqueue/core/changeset:go_default_library", @@ -14,7 +14,7 @@ go_library( go_test( name = "go_default_test", - srcs = ["fileoverlap_test.go"], + srcs = ["pathoverlap_test.go"], embed = [":go_default_library"], deps = [ "//submitqueue/core/changeset/fake:go_default_library", diff --git a/submitqueue/extension/conflict/pathoverlap/README.md b/submitqueue/extension/conflict/pathoverlap/README.md new file mode 100644 index 000000000..6f9086efa --- /dev/null +++ b/submitqueue/extension/conflict/pathoverlap/README.md @@ -0,0 +1,18 @@ +# pathoverlap + +`pathoverlap` is a `conflict.Analyzer` that reports a conflict between two batches when the paths they change share a key. + +## Behavior + +The files a batch changes are drawn from each change's provider-supplied details, and each path is projected onto a key by the `PathKey` chosen at construction. The candidate batch conflicts with an in-flight batch when their key sets intersect; each such in-flight batch is reported once, preserving the in-flight order. A shared path key is the concrete notion of *target overlap*, so conflicts are classified as `ConflictTypeTargetOverlap`. A batch that changes no files conflicts with nothing, and an empty in-flight list yields no conflicts. A failure to resolve a batch's changes is returned as a (retryable) error. + +## Granularity + +Two projections ship with the package, selected per queue in the wiring layer: + +- **`ByFile`** keys on the whole path, so only batches touching the same file conflict. +- **`ByDirectory`** keys on the path's immediate parent directory, so batches touching sibling files conflict too. Paths at the repository root share the key `.`, which serializes batches that touch any two root-level files. + +`ByDirectory` is strictly coarser: every file overlap is also a directory overlap. It trades parallelism for protection against semantic conflicts between neighbouring files — edits that break each other without touching the same file. Which trade is right is a per-queue judgement about how tightly coupled a directory's contents are, so the choice is a construction parameter rather than a property of the analyzer. + +Path-key intersection is a deliberately simple notion of overlap. A richer one that needs inputs beyond the changed paths — build targets, ownership boundaries — would be a separate analyzer rather than another `PathKey`. diff --git a/submitqueue/extension/conflict/pathoverlap/pathoverlap.go b/submitqueue/extension/conflict/pathoverlap/pathoverlap.go new file mode 100644 index 000000000..a206854e3 --- /dev/null +++ b/submitqueue/extension/conflict/pathoverlap/pathoverlap.go @@ -0,0 +1,138 @@ +// Copyright (c) 2025 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 pathoverlap provides a conflict.Analyzer that reports a conflict +// between two batches when the paths they change share a key. The key is a +// projection of the path chosen at construction: ByFile compares whole paths, +// so only batches touching the same file conflict; ByDirectory compares parent +// directories, so batches touching sibling files conflict too. It is the first +// analyzer to use the capability the extension contract unblocks: it takes only +// batch identity and resolves each batch's changed files itself through an +// injected changeset resolver, rather than depending on the controller to +// pre-compute them. A shared path key is the concrete notion of target overlap, +// so it reports entity.ConflictTypeTargetOverlap. +package pathoverlap + +import ( + "context" + "fmt" + "path" + + "github.com/uber/submitqueue/submitqueue/core/changeset" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/conflict" +) + +// PathKey projects a changed file path onto the key overlap is measured on. Two +// batches conflict when any of their changed paths yield the same key, so a +// coarser projection serializes more batches. +type PathKey func(path string) string + +// ByFile keys on the whole path: batches conflict only when they change the +// same file. +func ByFile(p string) string { + return path.Clean(p) +} + +// ByDirectory keys on the path's immediate parent directory: batches conflict +// when they change any files in the same directory, whether or not the files +// themselves are the same. Overlap by directory is strictly coarser than +// overlap by file — every file overlap is also a directory overlap. Paths at +// the repository root share the key ".". +func ByDirectory(p string) string { + return path.Dir(p) +} + +// analyzer reports a conflict between batches whose changed paths share a key. +// The paths a batch changes are resolved from each batch's change details. +type analyzer struct { + // cfg is the per-queue identity this analyzer was built for. + cfg conflict.Config + resolver changeset.Resolver + // key projects each changed path onto the key overlap is measured on. + key PathKey +} + +// New returns a conflict.Analyzer that flags an in-flight batch as conflicting +// when it changes a path whose key matches one the candidate batch changes, +// bound to the queue named in cfg. The resolver resolves each batch's changed +// files, and key selects the granularity of overlap. +// Panics if key is nil. +func New(cfg conflict.Config, resolver changeset.Resolver, key PathKey) conflict.Analyzer { + if key == nil { + panic("pathoverlap.New: key must not be nil") + } + return analyzer{cfg: cfg, resolver: resolver, key: key} +} + +// Analyze returns one ConflictTypeTargetOverlap Conflict per in-flight batch +// that shares a path key with batch, preserving the in-flight order. A batch +// that changes no files conflicts with nothing. +func (a analyzer) Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]entity.Conflict, error) { + if len(inFlight) == 0 { + return nil, nil + } + + candidate, err := a.keys(ctx, batch) + if err != nil { + return nil, fmt.Errorf("failed to resolve files for batch %s: %w", batch.ID, err) + } + if len(candidate) == 0 { + return nil, nil + } + + var conflicts []entity.Conflict + for _, other := range inFlight { + keys, err := a.keys(ctx, other) + if err != nil { + return nil, fmt.Errorf("failed to resolve files for batch %s: %w", other.ID, err) + } + if intersects(candidate, keys) { + conflicts = append(conflicts, entity.Conflict{ + BatchID: other.ID, + Type: entity.ConflictTypeTargetOverlap, + }) + } + } + return conflicts, nil +} + +// keys resolves the set of path keys the batch changes. +func (a analyzer) keys(ctx context.Context, batch entity.Batch) (map[string]struct{}, error) { + changes, err := a.resolver.DetailedForBatch(ctx, batch) + if err != nil { + return nil, err + } + keys := make(map[string]struct{}) + for _, change := range changes.Changes { + for _, file := range change.Details.ChangedFiles { + keys[a.key(file.Path)] = struct{}{} + } + } + return keys, nil +} + +// intersects reports whether the two sets share any element. +func intersects(a, b map[string]struct{}) bool { + // Iterate the smaller set for fewer lookups. + if len(b) < len(a) { + a, b = b, a + } + for k := range a { + if _, ok := b[k]; ok { + return true + } + } + return false +} diff --git a/submitqueue/extension/conflict/fileoverlap/fileoverlap_test.go b/submitqueue/extension/conflict/pathoverlap/pathoverlap_test.go similarity index 55% rename from submitqueue/extension/conflict/fileoverlap/fileoverlap_test.go rename to submitqueue/extension/conflict/pathoverlap/pathoverlap_test.go index 6363ac74d..28cb9d0a4 100644 --- a/submitqueue/extension/conflict/fileoverlap/fileoverlap_test.go +++ b/submitqueue/extension/conflict/pathoverlap/pathoverlap_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package fileoverlap +package pathoverlap import ( "context" @@ -39,9 +39,45 @@ func detailed(batchID string, files ...string) entity.BatchChanges { } } +func TestPathKey(t *testing.T) { + tests := []struct { + name string + path string + wantFile string + wantDirectory string + }{ + { + name: "nested path", + path: "src/pkg/a.go", + wantFile: "src/pkg/a.go", + wantDirectory: "src/pkg", + }, + { + name: "repository root file keys on the root directory", + path: "README.md", + wantFile: "README.md", + wantDirectory: ".", + }, + { + name: "unclean path is normalized", + path: "./src/pkg/../pkg/a.go", + wantFile: "src/pkg/a.go", + wantDirectory: "src/pkg", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantFile, ByFile(tt.path)) + assert.Equal(t, tt.wantDirectory, ByDirectory(tt.path)) + }) + } +} + func TestAnalyze(t *testing.T) { tests := []struct { name string + key PathKey candidate entity.BatchChanges inFlight map[string]entity.BatchChanges inFlightIDs []string @@ -49,6 +85,7 @@ func TestAnalyze(t *testing.T) { }{ { name: "overlap on a shared file conflicts", + key: ByFile, candidate: detailed("cand", "a.go", "b.go"), inFlight: map[string]entity.BatchChanges{ "x": detailed("x", "b.go", "c.go"), @@ -58,6 +95,7 @@ func TestAnalyze(t *testing.T) { }, { name: "disjoint files do not conflict", + key: ByFile, candidate: detailed("cand", "a.go"), inFlight: map[string]entity.BatchChanges{ "x": detailed("x", "z.go"), @@ -67,6 +105,7 @@ func TestAnalyze(t *testing.T) { }, { name: "only overlapping in-flight batches are reported, in order", + key: ByFile, candidate: detailed("cand", "a.go"), inFlight: map[string]entity.BatchChanges{ "x": detailed("x", "a.go"), @@ -78,6 +117,7 @@ func TestAnalyze(t *testing.T) { }, { name: "candidate with no targets conflicts with nothing", + key: ByFile, candidate: detailed("cand"), inFlight: map[string]entity.BatchChanges{ "x": detailed("x", "a.go"), @@ -85,6 +125,57 @@ func TestAnalyze(t *testing.T) { inFlightIDs: []string{"x"}, wantBatches: nil, }, + { + name: "sibling files in one directory do not conflict by file", + key: ByFile, + candidate: detailed("cand", "src/pkg/a.go"), + inFlight: map[string]entity.BatchChanges{ + "x": detailed("x", "src/pkg/b.go"), + }, + inFlightIDs: []string{"x"}, + wantBatches: nil, + }, + { + name: "sibling files in one directory conflict by directory", + key: ByDirectory, + candidate: detailed("cand", "src/pkg/a.go"), + inFlight: map[string]entity.BatchChanges{ + "x": detailed("x", "src/pkg/b.go"), + }, + inFlightIDs: []string{"x"}, + wantBatches: []string{"x"}, + }, + { + name: "files in sibling directories do not conflict by directory", + key: ByDirectory, + candidate: detailed("cand", "src/pkg/a.go"), + inFlight: map[string]entity.BatchChanges{ + "x": detailed("x", "src/other/a.go"), + }, + inFlightIDs: []string{"x"}, + wantBatches: nil, + }, + { + name: "the same file still conflicts by directory", + key: ByDirectory, + candidate: detailed("cand", "src/pkg/a.go"), + inFlight: map[string]entity.BatchChanges{ + "x": detailed("x", "src/pkg/a.go"), + }, + inFlightIDs: []string{"x"}, + wantBatches: []string{"x"}, + }, + { + name: "repository root files conflict with each other by directory", + key: ByDirectory, + candidate: detailed("cand", "README.md"), + inFlight: map[string]entity.BatchChanges{ + "x": detailed("x", "go.mod"), + "y": detailed("y", "src/pkg/a.go"), + }, + inFlightIDs: []string{"x", "y"}, + wantBatches: []string{"x"}, + }, } for _, tt := range tests { @@ -96,7 +187,7 @@ func TestAnalyze(t *testing.T) { inFlight = append(inFlight, entity.Batch{ID: id}) } - got, err := New(conflict.Config{QueueName: "test-queue"}, resolver).Analyze(context.Background(), entity.Batch{ID: "cand"}, inFlight) + got, err := New(conflict.Config{QueueName: "test-queue"}, resolver, tt.key).Analyze(context.Background(), entity.Batch{ID: "cand"}, inFlight) require.NoError(t, err) var ids []string @@ -110,7 +201,7 @@ func TestAnalyze(t *testing.T) { } func TestAnalyze_EmptyInFlight(t *testing.T) { - got, err := New(conflict.Config{QueueName: "test-queue"}, changesetfake.New()).Analyze(context.Background(), entity.Batch{ID: "cand"}, nil) + got, err := New(conflict.Config{QueueName: "test-queue"}, changesetfake.New(), ByFile).Analyze(context.Background(), entity.Batch{ID: "cand"}, nil) require.NoError(t, err) assert.Empty(t, got) } @@ -119,6 +210,12 @@ func TestAnalyze_ResolverError(t *testing.T) { sentinel := errors.New("resolve failed") resolver := changesetfake.New().FailWith(sentinel) - _, err := New(conflict.Config{QueueName: "test-queue"}, resolver).Analyze(context.Background(), entity.Batch{ID: "cand"}, []entity.Batch{{ID: "x"}}) + _, err := New(conflict.Config{QueueName: "test-queue"}, resolver, ByFile).Analyze(context.Background(), entity.Batch{ID: "cand"}, []entity.Batch{{ID: "x"}}) require.ErrorIs(t, err, sentinel) } + +func TestNew_NilKeyPanics(t *testing.T) { + assert.Panics(t, func() { + New(conflict.Config{QueueName: "test-queue"}, changesetfake.New(), nil) + }) +}