From 06ef188e68ce573bb056f6e03711dc04cb2897bb Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 28 Jul 2026 14:45:27 -0700 Subject: [PATCH] chore(submitqueue): remove dead pusher extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The `pusher` extension is dead code. Merge execution moved to Runway, where the pluggable contract is `runway/extension/merger` — a Runway-owned interface that takes a `MergeRequest` off the queue rather than SubmitQueue `entity.Batch` values. Nothing in the tree imports `submitqueue/extension/pusher` any more: the only remaining references were the package's own files and the `mocks` target in the Makefile. Removing it now keeps a second, divergent merge contract from drifting alongside the live one. ### What? Deletes the whole `submitqueue/extension/pusher` tree — the `Pusher`/`Factory` contract, the git-backed implementation, the fake, and the generated mock. Drops `./submitqueue/extension/pusher/...` from the `go generate` invocation in the `mocks` Makefile target. Left in place it would point `make mocks` at a path that no longer exists. Drops the stale `pusher` mention from the `changeset` package docs, which listed it as an example of a decision/action extension resolving content through a `Resolver`. ## Test Plan ✅ `make gazelle` — no BUILD churn beyond the deleted packages ✅ `make fmt` ✅ `make test` — 82/82 pass ✅ `grep -rn "extension/pusher"` over the tree returns nothing --- Makefile | 2 +- submitqueue/core/changeset/changeset.go | 2 +- submitqueue/extension/pusher/BUILD.bazel | 9 - submitqueue/extension/pusher/README.md | 57 -- submitqueue/extension/pusher/fake/BUILD.bazel | 29 - submitqueue/extension/pusher/fake/fake.go | 100 --- .../extension/pusher/fake/fake_test.go | 68 -- submitqueue/extension/pusher/git/BUILD.bazel | 48 -- .../extension/pusher/git/git_pusher.go | 532 --------------- .../extension/pusher/git/git_pusher_test.go | 642 ------------------ submitqueue/extension/pusher/mock/BUILD.bazel | 13 - .../extension/pusher/mock/pusher_mock.go | 97 --- submitqueue/extension/pusher/pusher.go | 70 -- 13 files changed, 2 insertions(+), 1667 deletions(-) delete mode 100644 submitqueue/extension/pusher/BUILD.bazel delete mode 100644 submitqueue/extension/pusher/README.md delete mode 100644 submitqueue/extension/pusher/fake/BUILD.bazel delete mode 100644 submitqueue/extension/pusher/fake/fake.go delete mode 100644 submitqueue/extension/pusher/fake/fake_test.go delete mode 100644 submitqueue/extension/pusher/git/BUILD.bazel delete mode 100644 submitqueue/extension/pusher/git/git_pusher.go delete mode 100644 submitqueue/extension/pusher/git/git_pusher_test.go delete mode 100644 submitqueue/extension/pusher/mock/BUILD.bazel delete mode 100644 submitqueue/extension/pusher/mock/pusher_mock.go delete mode 100644 submitqueue/extension/pusher/pusher.go diff --git a/Makefile b/Makefile index bcc9e765..d4dfed59 100644 --- a/Makefile +++ b/Makefile @@ -366,7 +366,7 @@ local-stovepipe-stop: ## Stop the Stovepipe service mocks: ## Generate mock files using mockgen @echo "Generating mocks..." - @$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/consumergate/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/pusher/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./submitqueue/extension/speculation/... ./submitqueue/extension/validator/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/... + @$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/consumergate/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./submitqueue/extension/speculation/... ./submitqueue/extension/validator/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/... @echo "Mocks generated successfully!" proto: ## Generate protobuf files from .proto definitions diff --git a/submitqueue/core/changeset/changeset.go b/submitqueue/core/changeset/changeset.go index 29aeb4b9..3edf21a8 100644 --- a/submitqueue/core/changeset/changeset.go +++ b/submitqueue/core/changeset/changeset.go @@ -15,7 +15,7 @@ // Package changeset resolves batch identity into the changes a batch contains. // It is the single place the orchestrator walks batch -> requests -> changes, // consolidating what the build and merge controllers each did privately. -// Decision/action extensions (scorer, buildrunner, pusher, and future +// Decision/action extensions (scorer, buildrunner, and future // detail-aware conflict analyzers) take thin identity entities and resolve their // granular content through an injected Resolver instead of being handed // pre-resolved data by a controller. diff --git a/submitqueue/extension/pusher/BUILD.bazel b/submitqueue/extension/pusher/BUILD.bazel deleted file mode 100644 index d6f3ffb0..00000000 --- a/submitqueue/extension/pusher/BUILD.bazel +++ /dev/null @@ -1,9 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library") - -go_library( - name = "go_default_library", - srcs = ["pusher.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/pusher", - visibility = ["//visibility:public"], - deps = ["//submitqueue/entity:go_default_library"], -) diff --git a/submitqueue/extension/pusher/README.md b/submitqueue/extension/pusher/README.md deleted file mode 100644 index f1a666db..00000000 --- a/submitqueue/extension/pusher/README.md +++ /dev/null @@ -1,57 +0,0 @@ -# Pusher - -Pluggable abstraction for landing the changes of one or more batches onto a -target branch and pushing the result to a source-control remote. - -## Interface - -`Pusher` exposes a single `Push` method that accepts an ordered list of -batches and resolves each batch's changes itself through an injected changeset -resolver. Implementations are bound to a specific `(checkout, remote, target)` -tuple at construction time, so the interface itself stays vendor- and -configuration-agnostic. The batch list designs for a merge-train (landing -several ready batches in one atomic push); today the merge stage passes a -single batch. - -The interface enforces an **all-or-nothing atomicity contract**: when `Push` -returns an error, no change has reached the remote — neither partially nor -fully. Callers can treat a non-nil error as "the remote is exactly as it was -before the call". The `ErrConflict` sentinel marks user-caused failures so -callers can route them to a non-retry path. - -A successful `Push` returns one `entity.BatchOutcome` per input batch, in input -order, each carrying one `entity.ChangeOutcome` per change in that batch. Each -change outcome reports either: - -- `entity.OutcomeStatusCommitted` with the list of `CommitSHAs` produced on the - target branch (one change can land as multiple commits, e.g. a stack of - PRs); or -- `entity.OutcomeStatusAlreadyExisted` with no commits, when the change is - already present on the target branch (previously landed via another path, or - subsumed by an earlier change in the same push). Git surfaces this as - "rebased out" during a cherry-pick. - -There is no per-batch status: the push is all-or-nothing across the whole call, -so a per-batch pass/fail would be uniformly redundant. - -## Implementations - -- [`git/`](git/) — applies changes against a local checkout via `git - cherry-pick`, then `git push`. Construction takes the path to the - checkout, the remote name, and the target branch; the implementation - owns that working tree and serializes concurrent invocations. -- [`fake/`](fake/) — test/example stub. Reports every change as committed - unless a change URI carries a failure marker (`sq-fake=conflict` → - `ErrConflict`, `sq-fake=push-error` → error), letting a single running - stack exercise negative paths from request payloads. Not for production. - -## Adding a new backend - -1. Create `extension/pusher/{backend}/` with a `Pusher` implementation. -2. Bind the implementation to its checkout/remote/target at construction. -3. Resolve each batch's changes via the injected resolver, then map each - `entity.Change` to the backend's commit/push primitives. -4. Honour the atomicity contract: never publish partial state. Return - `ErrConflict` (wrapped) for user-caused apply failures and a plain error - for transient infra failures. - diff --git a/submitqueue/extension/pusher/fake/BUILD.bazel b/submitqueue/extension/pusher/fake/BUILD.bazel deleted file mode 100644 index d8df8300..00000000 --- a/submitqueue/extension/pusher/fake/BUILD.bazel +++ /dev/null @@ -1,29 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "go_default_library", - srcs = ["fake.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/pusher/fake", - visibility = ["//visibility:public"], - deps = [ - "//platform/base/change:go_default_library", - "//submitqueue/core/changeset:go_default_library", - "//submitqueue/core/fakemarker:go_default_library", - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/pusher:go_default_library", - ], -) - -go_test( - name = "go_default_test", - srcs = ["fake_test.go"], - embed = [":go_default_library"], - deps = [ - "//platform/base/change:go_default_library", - "//submitqueue/core/changeset/fake:go_default_library", - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/pusher:go_default_library", - "@com_github_stretchr_testify//assert:go_default_library", - "@com_github_stretchr_testify//require:go_default_library", - ], -) diff --git a/submitqueue/extension/pusher/fake/fake.go b/submitqueue/extension/pusher/fake/fake.go deleted file mode 100644 index 8c96e01b..00000000 --- a/submitqueue/extension/pusher/fake/fake.go +++ /dev/null @@ -1,100 +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 fake provides a pusher.Pusher whose outcome is driven by the input -// changes. With no marker every change is reported as committed with a synthetic -// commit SHA, behaving as a best-case stub for wiring and baselines. A failure -// can be injected end-to-end (e.g. from an e2e land request) by embedding a -// marker token in a change URI of the form "sq-fake=": -// -// sq-fake=conflict -> pusher.ErrConflict -// sq-fake=push-error -> non-nil error -// -// Both failure markers honor the atomicity contract: on error nothing is -// "pushed". This lets a single running stack exercise negative paths purely by -// varying request payloads. It is intended for examples and tests only, never -// production. -package fake - -import ( - "context" - "fmt" - "sync/atomic" - - "github.com/uber/submitqueue/platform/base/change" - "github.com/uber/submitqueue/submitqueue/core/changeset" - "github.com/uber/submitqueue/submitqueue/core/fakemarker" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/pusher" -) - -// Recognized marker tokens. See the package doc for the convention. -const ( - tokenConflict = "conflict" - tokenError = "push-error" -) - -// fakePusher is a pusher.Pusher that reports every change as committed unless a -// marker token in a change URI requests a failure. The atomic counter hands out -// unique synthetic commit SHAs and makes the type safe for concurrent use. It -// resolves each batch's changes through the injected resolver. -type fakePusher struct { - resolver changeset.Resolver - counter atomic.Uint64 -} - -// New returns a pusher.Pusher that defaults to committing every change and -// honors marker tokens embedded in change URIs. The resolver resolves each -// batch's changes. -func New(resolver changeset.Resolver) pusher.Pusher { - return &fakePusher{resolver: resolver} -} - -// Push resolves each batch's changes and reports every change as committed with -// a synthetic commit SHA, grouped per batch, unless a recognized marker token in -// one of the changes requests a failure. -func (p *fakePusher) Push(ctx context.Context, batches []entity.Batch) (entity.PushResult, error) { - perBatch := make([][]change.Change, len(batches)) - var all []change.Change - for i, b := range batches { - cs, err := p.resolver.ChangesForBatch(ctx, b) - if err != nil { - return entity.PushResult{}, fmt.Errorf("fake: resolve batch %s: %w", b.ID, err) - } - perBatch[i] = cs - all = append(all, cs...) - } - - switch fakemarker.TokenInChanges(all) { - case tokenConflict: - return entity.PushResult{}, pusher.ErrConflict - case tokenError: - return entity.PushResult{}, fmt.Errorf("fake: marked push error") - } - - result := make([]entity.BatchOutcome, len(batches)) - for i, b := range batches { - outcomes := make([]entity.ChangeOutcome, 0, len(perBatch[i])) - for _, change := range perBatch[i] { - sha := fmt.Sprintf("fake-%d", p.counter.Add(1)) - outcomes = append(outcomes, entity.ChangeOutcome{ - Change: change, - Status: entity.OutcomeStatusCommitted, - CommitSHAs: []string{sha}, - }) - } - result[i] = entity.BatchOutcome{BatchID: b.ID, Outcomes: outcomes} - } - return entity.PushResult{Batches: result}, nil -} diff --git a/submitqueue/extension/pusher/fake/fake_test.go b/submitqueue/extension/pusher/fake/fake_test.go deleted file mode 100644 index d91b2727..00000000 --- a/submitqueue/extension/pusher/fake/fake_test.go +++ /dev/null @@ -1,68 +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 fake - -import ( - "context" - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/platform/base/change" - changesetfake "github.com/uber/submitqueue/submitqueue/core/changeset/fake" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/pusher" -) - -func TestNew_ImplementsInterface(t *testing.T) { - var _ pusher.Pusher = New(changesetfake.New()) -} - -func TestPusher_Push_Committed(t *testing.T) { - changes := []change.Change{ - {URIs: []string{"github://github.example.com/owner/repo/pull/1/abc"}}, - {URIs: []string{"github://github.example.com/owner/repo/pull/2/def"}}, - } - p := New(changesetfake.New().Set("b", changes...)) - - res, err := p.Push(context.Background(), []entity.Batch{{ID: "b"}}) - require.NoError(t, err) - require.Len(t, res.Batches, 1) - require.Len(t, res.Batches[0].Outcomes, len(changes)) - - seen := map[string]bool{} - for i, out := range res.Batches[0].Outcomes { - assert.Equal(t, changes[i], out.Change) - assert.Equal(t, entity.OutcomeStatusCommitted, out.Status) - require.Len(t, out.CommitSHAs, 1) - assert.False(t, seen[out.CommitSHAs[0]], "commit SHAs must be unique") - seen[out.CommitSHAs[0]] = true - } -} - -func TestPusher_Push_ConflictMarker(t *testing.T) { - p := New(changesetfake.New().Set("b", change.Change{URIs: []string{"github://github.example.com/owner/repo/pull/1/abc?sq-fake=conflict"}})) - _, err := p.Push(context.Background(), []entity.Batch{{ID: "b"}}) - assert.True(t, errors.Is(err, pusher.ErrConflict)) -} - -func TestPusher_Push_ErrorMarker(t *testing.T) { - p := New(changesetfake.New().Set("b", change.Change{URIs: []string{"github://github.example.com/owner/repo/pull/1/abc?sq-fake=push-error"}})) - res, err := p.Push(context.Background(), []entity.Batch{{ID: "b"}}) - require.Error(t, err) - // Atomicity: on error no outcomes are reported. - assert.Empty(t, res.Batches) -} diff --git a/submitqueue/extension/pusher/git/BUILD.bazel b/submitqueue/extension/pusher/git/BUILD.bazel deleted file mode 100644 index d1891bd6..00000000 --- a/submitqueue/extension/pusher/git/BUILD.bazel +++ /dev/null @@ -1,48 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "go_default_library", - srcs = ["git_pusher.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/pusher/git", - visibility = ["//visibility:public"], - deps = [ - "//platform/base/change:go_default_library", - "//platform/base/change/github:go_default_library", - "//platform/metrics:go_default_library", - "//submitqueue/core/changeset:go_default_library", - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/pusher: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 = ["git_pusher_test.go"], - data = [ - "@git", - "@git//:git-remote-http", - "@git//:git_receive_pack", - "@git//:git_remote_https", - "@git//:git_upload_archive", - "@git//:git_upload_pack", - "@git//:templates", - "@git//:templates/description", - ], - embed = [":go_default_library"], - env = { - "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", - "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION": "$(location @git//:templates/description)", - }, - deps = [ - "//platform/base/change:go_default_library", - "//submitqueue/core/changeset/fake:go_default_library", - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/pusher: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_zap//zaptest:go_default_library", - ], -) diff --git a/submitqueue/extension/pusher/git/git_pusher.go b/submitqueue/extension/pusher/git/git_pusher.go deleted file mode 100644 index 78e99e2e..00000000 --- a/submitqueue/extension/pusher/git/git_pusher.go +++ /dev/null @@ -1,532 +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 git is a simple Pusher implementation backed by a local git -// checkout. -// -// On every Push call the implementation: -// -// 1. Fetches the configured remote. -// 2. Resets the checkout's HEAD to refs/remotes//. -// 3. Cherry-picks the head SHA of every URI of every Change, in order. -// A pick that produces no new content (because the change is already -// present on the target branch) is what git surfaces as "rebased out" -// and is recorded as OutcomeStatusAlreadyExisted. -// A pick that fails to apply cleanly is treated as a conflict. -// 4. Pushes HEAD to refs/heads/ on the remote. -// -// Atomicity: nothing is published to the remote until step 4 succeeds. If -// any cherry-pick fails the in-progress pick is aborted and Push returns an -// error without ever invoking step 4. -// -// Contention: if step 4 fails because the remote tip moved between step 2 -// and step 4 (typically a concurrent push from another writer), the whole -// fetch/reset/cherry-pick/push cycle is retried. Detection works by -// re-fetching the remote tip after a push failure and comparing it to the -// SHA we reset to in step 2: if it advanced, the failure is treated as -// contention. Other push failures (network, auth, hook reject without ref -// change) propagate immediately. Retries are capped at -// Params.MaxPushAttempts (default 10) to bound the worst case. -// -// Construction takes the path to an existing checkout, the remote name, and -// the target branch — the implementation owns the working tree at that path -// for the duration of any in-flight Push call and serializes concurrent -// invocations. -// -// Change URIs are parsed using the github-family URI format (see -// entity/github), so each URI's last segment is interpreted as the head -// commit SHA. The SHA must be reachable from a ref on the remote so that -// `git fetch` makes it available locally. -package git - -import ( - "bytes" - "context" - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "sync" - - "github.com/uber-go/tally" - "go.uber.org/zap" - - "github.com/uber/submitqueue/platform/base/change" - entitygithub "github.com/uber/submitqueue/platform/base/change/github" - coremetrics "github.com/uber/submitqueue/platform/metrics" - "github.com/uber/submitqueue/submitqueue/core/changeset" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/pusher" -) - -// defaultMaxPushAttempts caps the retry loop in Push when the remote tip -// keeps moving under us. Bounded to prevent an infinite loop against a -// pathologically busy remote. -const defaultMaxPushAttempts = 10 - -// GitRuntime identifies the explicitly provided Git runtime used by the Pusher. -type GitRuntime struct { - // Executable is the absolute path to the Git executable. - Executable string - // ExecPath is the absolute directory containing Git's helper executables. - ExecPath string - // TemplateDir is the absolute directory containing Git's repository - // templates. - TemplateDir string -} - -// Params holds the dependencies for the git Pusher. -type Params struct { - // CheckoutPath is the absolute path to an existing git checkout that the - // Pusher will operate against. The Pusher owns this working tree. - CheckoutPath string - // Remote is the name of the git remote to fetch from and push to - // (e.g. "origin"). - Remote string - // Target is the destination branch ref on the remote (e.g. "main"). - Target string - // Resolver resolves each batch's changes. - Resolver changeset.Resolver - // Logger is the structured logger. - Logger *zap.SugaredLogger - // MetricsScope is the metrics scope for instrumentation. - MetricsScope tally.Scope - // Runtime is the pinned Git runtime used for every invocation. - Runtime GitRuntime - // MaxPushAttempts caps how many times Push retries the full - // fetch/reset/cherry-pick/push cycle when the remote tip moves under - // it. Defaults to defaultMaxPushAttempts when zero or negative. - MaxPushAttempts int -} - -// gitPusher implements pusher.Pusher by shelling out to the `git` CLI -// against a local checkout. -type gitPusher struct { - checkoutPath string - remote string - target string - resolver changeset.Resolver - logger *zap.SugaredLogger - metricsScope tally.Scope - runtime GitRuntime - maxPushAttempts int - - // mu serializes concurrent Push calls — the underlying checkout cannot - // be safely shared between operations. - mu sync.Mutex -} - -// Verify gitPusher implements pusher.Pusher at compile time. -var _ pusher.Pusher = (*gitPusher)(nil) - -// NewPusher constructs a new git-backed Pusher operating against the given -// checkout. The checkout must already exist and have the configured remote. -// Runtime paths must be absolute. -func NewPusher(params Params) (pusher.Pusher, error) { - if err := params.Runtime.validate(); err != nil { - return nil, err - } - maxAttempts := params.MaxPushAttempts - if maxAttempts <= 0 { - maxAttempts = defaultMaxPushAttempts - } - return &gitPusher{ - checkoutPath: params.CheckoutPath, - remote: params.Remote, - target: params.Target, - resolver: params.Resolver, - logger: params.Logger.Named("git_pusher"), - metricsScope: params.MetricsScope.SubScope("git_pusher"), - runtime: params.Runtime, - maxPushAttempts: maxAttempts, - }, nil -} - -func (r GitRuntime) validate() error { - for name, path := range map[string]string{ - "executable": r.Executable, - "exec path": r.ExecPath, - "template dir": r.TemplateDir, - } { - if path == "" { - return fmt.Errorf("git runtime %s is required", name) - } - if !filepath.IsAbs(path) { - return fmt.Errorf("git runtime %s must be absolute: %q", name, path) - } - } - return nil -} - -// Push fulfils the pusher.Pusher contract. -func (p *gitPusher) Push(ctx context.Context, batches []entity.Batch) (ret entity.PushResult, retErr error) { - op := coremetrics.Begin(p.metricsScope, "push", coremetrics.LongLatencyBuckets) - defer func() { op.Complete(retErr) }() - - // Resolve each batch's changes, keeping per-batch counts so the flat - // outcomes can be regrouped per batch on success. - perBatch := make([][]change.Change, len(batches)) - var changes []change.Change - for i, b := range batches { - cs, err := p.resolver.ChangesForBatch(ctx, b) - if err != nil { - return entity.PushResult{}, fmt.Errorf("resolve batch %s: %w", b.ID, err) - } - perBatch[i] = cs - changes = append(changes, cs...) - } - - p.mu.Lock() - defer p.mu.Unlock() - - if len(changes) == 0 { - coremetrics.NamedCounter(p.metricsScope, "push", "empty_changes", 1) - return entity.PushResult{}, fmt.Errorf("push called with no changes") - } - - p.logger.Debugw("starting push", - "target", p.target, - "remote", p.remote, - "change_count", len(changes), - ) - - var lastErr error - for attempt := 1; attempt <= p.maxPushAttempts; attempt++ { - baseSHA, outcomes, err := p.tryPush(ctx, changes) - if err == nil { - p.logger.Debugw("push complete", - "target", p.target, - "outcomes", outcomes, - ) - return entity.PushResult{Batches: groupByBatch(batches, perBatch, outcomes)}, nil - } - - // Was the failure caused by the remote tip moving under us between - // reset and push (concurrent-push contention)? That's the only push - // failure worth retrying; everything else (network, auth, hook - // reject without ref change) is fatal. Detection is by re-fetching - // the remote tip and comparing to baseSHA — robust against varying - // git error message formats. baseSHA is empty when the failure - // happened before reset captured a base; treat those as fatal too. - if baseSHA == "" { - return entity.PushResult{}, err - } - currentSHA, refetchErr := p.refetchTipSHA(ctx) - if refetchErr != nil { - return entity.PushResult{}, fmt.Errorf("refetch after push failure failed: %v (original push error: %w)", refetchErr, err) - } - if currentSHA == baseSHA { - return entity.PushResult{}, err - } - - coremetrics.NamedCounter(p.metricsScope, "push", "stale_base_retries", 1) - p.logger.Warnw("remote tip moved during push, retrying", - "attempt", attempt, - "max_attempts", p.maxPushAttempts, - "base_sha", baseSHA, - "current_sha", currentSHA, - "err", err, - ) - lastErr = err - } - - coremetrics.NamedCounter(p.metricsScope, "push", "stale_base_giveup", 1) - return entity.PushResult{}, fmt.Errorf("exceeded %d push attempts due to remote contention: %w", p.maxPushAttempts, lastErr) -} - -// groupByBatch splits the flat, apply-ordered outcomes back into one -// BatchOutcome per input batch, using each batch's resolved change count. -func groupByBatch(batches []entity.Batch, perBatch [][]change.Change, outcomes []entity.ChangeOutcome) []entity.BatchOutcome { - result := make([]entity.BatchOutcome, len(batches)) - pos := 0 - for i, b := range batches { - n := len(perBatch[i]) - result[i] = entity.BatchOutcome{BatchID: b.ID, Outcomes: outcomes[pos : pos+n]} - pos += n - } - return result -} - -// tryPush runs one full reset+cherry-pick+push cycle. The returned baseSHA -// is the SHA the cycle was based on (set as soon as resetToRemote completes) -// so the caller can distinguish concurrent-push contention from other push -// failures. baseSHA is empty when the failure happened before reset -// produced a base. -func (p *gitPusher) tryPush(ctx context.Context, changes []change.Change) (string, []entity.ChangeOutcome, error) { - if err := p.resetToRemote(ctx); err != nil { - coremetrics.NamedCounter(p.metricsScope, "push", "reset_errors", 1) - return "", nil, err - } - baseSHA, err := p.headSHA(ctx) - if err != nil { - return "", nil, err - } - - outcomes, err := p.cherryPickAll(ctx, changes) - if err != nil { - // Best-effort cleanup so the next attempt starts from a known state. - // The next attempt starts with resetToRemote regardless, so we don't - // care if --abort itself fails (e.g., no pick is in progress). - _, _ = p.run(ctx, nil, "cherry-pick", "--abort") - return baseSHA, nil, err - } - - if err := p.push(ctx); err != nil { - coremetrics.NamedCounter(p.metricsScope, "push", "git_push_errors", 1) - return baseSHA, nil, err - } - return baseSHA, outcomes, nil -} - -// headSHA returns the SHA at HEAD in the local checkout. -func (p *gitPusher) headSHA(ctx context.Context) (string, error) { - out, err := p.run(ctx, nil, "rev-parse", "HEAD") - if err != nil { - return "", fmt.Errorf("git rev-parse HEAD: %w", err) - } - return strings.TrimSpace(string(out)), nil -} - -// refetchTipSHA fetches the remote and returns the current SHA at -// refs/remotes//. Used after a push failure to detect -// whether the remote tip moved under us. -func (p *gitPusher) refetchTipSHA(ctx context.Context) (string, error) { - if _, err := p.run(ctx, nil, "fetch", p.remote); err != nil { - return "", fmt.Errorf("git fetch %s: %w", p.remote, err) - } - remoteRef := p.remote + "/" + p.target - out, err := p.run(ctx, nil, "rev-parse", remoteRef) - if err != nil { - return "", fmt.Errorf("git rev-parse %s: %w", remoteRef, err) - } - return strings.TrimSpace(string(out)), nil -} - -// resetToRemote fetches the configured remote and hard-resets the checkout's -// HEAD to refs/remotes//, producing a clean working tree -// pinned to the latest remote tip. -func (p *gitPusher) resetToRemote(ctx context.Context) error { - if _, err := p.run(ctx, nil, "fetch", p.remote); err != nil { - return fmt.Errorf("git fetch %s: %w", p.remote, err) - } - remoteRef := p.remote + "/" + p.target - if _, err := p.run(ctx, nil, "reset", "--hard", remoteRef); err != nil { - return fmt.Errorf("git reset --hard %s: %w", remoteRef, err) - } - if _, err := p.run(ctx, nil, "clean", "-fdx"); err != nil { - return fmt.Errorf("git clean: %w", err) - } - return nil -} - -// cherryPickAll walks the changes in order, cherry-picking every URI's head -// SHA, and returns one ChangeOutcome per Change in the same order. -func (p *gitPusher) cherryPickAll(ctx context.Context, changes []change.Change) ([]entity.ChangeOutcome, error) { - outcomes := make([]entity.ChangeOutcome, 0, len(changes)) - for _, change := range changes { - commits, err := p.cherryPickChange(ctx, change) - if err != nil { - return nil, err - } - status := entity.OutcomeStatusCommitted - if len(commits) == 0 { - status = entity.OutcomeStatusAlreadyExisted - } - outcomes = append(outcomes, entity.ChangeOutcome{ - Change: change, - Status: status, - CommitSHAs: commits, - }) - } - return outcomes, nil -} - -// cherryPickChange parses each URI in the change, fetches the referenced -// SHA, and cherry-picks it. It returns the list of new commit SHAs -// produced for this change (empty if every pick was a no-op because the -// content was already on the target branch). -func (p *gitPusher) cherryPickChange(ctx context.Context, change change.Change) ([]string, error) { - var commits []string - for _, uri := range change.URIs { - cid, err := entitygithub.ParseChangeID(uri) - if err != nil { - return nil, fmt.Errorf("invalid change URI %q: %w", uri, err) - } - - sha, picked, err := p.cherryPickSHA(ctx, cid.HeadCommitSHA) - if err != nil { - return nil, fmt.Errorf("cherry-pick %s (uri %q): %w", cid.HeadCommitSHA, uri, err) - } - if picked { - commits = append(commits, sha) - } - } - return commits, nil -} - -// cherryPickSHA cherry-picks a single SHA. It returns: -// - the new commit SHA and picked=true on a successful pick that produced -// a non-empty commit; -// - "" and picked=false when the pick is a no-op (the change is already -// on the target branch); the empty commit is rolled back so it doesn't -// get pushed; -// - an error: pusher.ErrConflict when the pick fails to apply because of a conflict (generally a user-caused failure), -// or any other error when the pick fails for any other reason (potentially recoverable). -// -// `--allow-empty` covers the case where the source commit itself was -// originally empty. For redundant picks (the change is already on target, -// so applying it produces no new diff) git refuses with "previous -// cherry-pick is now empty"; we recover by running `cherry-pick --skip` -// and reporting the change as already-existed (what git would call -// "rebased out"). -func (p *gitPusher) cherryPickSHA(ctx context.Context, sha string) (string, bool, error) { - out, err := p.runCombined(ctx, nil, "cherry-pick", "--allow-empty", sha) - if err != nil { - if isRedundantCherryPick(out) { - if _, skipErr := p.run(ctx, nil, "cherry-pick", "--skip"); skipErr != nil { - return "", false, fmt.Errorf("git cherry-pick --skip after redundant pick: %w", skipErr) - } - return "", false, nil - } - coremetrics.NamedCounter(p.metricsScope, "push", "cherry_pick_conflicts", 1) - return "", false, fmt.Errorf("%w: git cherry-pick %s: %s", pusher.ErrConflict, sha, strings.TrimSpace(string(out))) - } - - // `--allow-empty` lets a genuinely empty source commit through as an - // empty commit on target. Detect and roll that back so it doesn't get - // pushed, and report it as already-existed. - empty, err := p.isEmptyHEADCommit(ctx) - if err != nil { - return "", false, err - } - if empty { - if _, err := p.run(ctx, nil, "reset", "--hard", "HEAD^"); err != nil { - return "", false, fmt.Errorf("git reset --hard HEAD^ after empty pick: %w", err) - } - return "", false, nil - } - - headOut, err := p.run(ctx, nil, "rev-parse", "HEAD") - if err != nil { - return "", false, fmt.Errorf("git rev-parse HEAD: %w", err) - } - return strings.TrimSpace(string(headOut)), true, nil -} - -// isEmptyHEADCommit returns true when HEAD's tree matches HEAD^'s tree — -// i.e. the most recent commit introduces no changes. -func (p *gitPusher) isEmptyHEADCommit(ctx context.Context) (bool, error) { - headTree, err := p.commitTreeSHA(ctx, "HEAD") - if err != nil { - return false, err - } - parentTree, err := p.commitTreeSHA(ctx, "HEAD^") - if err != nil { - return false, err - } - return headTree == parentTree, nil -} - -// commitTreeSHA returns the tree SHA recorded in the commit object at ref. -// It reads the raw commit via `git cat-file commit` and parses the leading -// `tree ` line — the same data `rev-parse ^{tree}` would peel to, -// but without depending on revision-syntax magic. -func (p *gitPusher) commitTreeSHA(ctx context.Context, ref string) (string, error) { - out, err := p.run(ctx, nil, "cat-file", "commit", ref) - if err != nil { - return "", fmt.Errorf("git cat-file commit %s: %w", ref, err) - } - firstLine, _, _ := strings.Cut(string(out), "\n") - const prefix = "tree " - if !strings.HasPrefix(firstLine, prefix) { - return "", fmt.Errorf("git cat-file commit %s: unexpected first line %q", ref, firstLine) - } - return strings.TrimSpace(firstLine[len(prefix):]), nil -} - -// push pushes the current HEAD to refs/heads/ on the remote. -func (p *gitPusher) push(ctx context.Context) error { - refspec := "HEAD:refs/heads/" + p.target - if _, err := p.run(ctx, nil, "push", p.remote, refspec); err != nil { - return fmt.Errorf("git push %s %s: %w", p.remote, refspec, err) - } - return nil -} - -// run executes a `git` command in the checkout. Returns captured stdout and -// an error that includes captured stderr for diagnostics. -func (p *gitPusher) run(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := newGitCommand(ctx, p.runtime, p.checkoutPath, args...) - if stdin != nil { - cmd.Stdin = bytes.NewReader(stdin) - } - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { - return nil, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String())) - } - return stdout.Bytes(), nil -} - -// runCombined is like run but returns combined stdout+stderr both on success -// and failure. Used when the caller needs to inspect git's diagnostic -// output (e.g., to detect "previous cherry-pick is now empty"). -func (p *gitPusher) runCombined(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := newGitCommand(ctx, p.runtime, p.checkoutPath, args...) - if stdin != nil { - cmd.Stdin = bytes.NewReader(stdin) - } - return cmd.CombinedOutput() -} - -// newGitCommand constructs a Git command without inheriting the caller's -// environment. The executable and helper paths come from the pinned runtime; -// repository-local configuration remains an intentional input. -func newGitCommand(ctx context.Context, runtime GitRuntime, dir string, args ...string) *exec.Cmd { - gitArgs := make([]string, 0, len(args)+3) - gitArgs = append(gitArgs, - "--exec-path="+runtime.ExecPath, - "-c", "init.templateDir="+runtime.TemplateDir, - ) - gitArgs = append(gitArgs, args...) - - cmd := exec.CommandContext(ctx, runtime.Executable, gitArgs...) - cmd.Dir = dir - cmd.Env = []string{ - "HOME=" + filepath.Join(dir, ".submitqueue-git-home"), - "XDG_CONFIG_HOME=" + filepath.Join(dir, ".submitqueue-git-home", "xdg"), - "GIT_CONFIG_NOSYSTEM=1", - "GIT_CONFIG_GLOBAL=" + os.DevNull, - "GIT_ATTR_NOSYSTEM=1", - "GIT_TERMINAL_PROMPT=0", - "GIT_PAGER=cat", - "GIT_EDITOR=:", - "GIT_EXEC_PATH=" + runtime.ExecPath, - "GIT_TEMPLATE_DIR=" + runtime.TemplateDir, - "LC_ALL=C", - "LANG=C", - } - return cmd -} - -// isRedundantCherryPick reports whether git's cherry-pick output indicates -// the pick was rejected because the change is already present on target -// (i.e. applying it would produce no diff). -func isRedundantCherryPick(out []byte) bool { - s := string(out) - return strings.Contains(s, "previous cherry-pick is now empty") || - strings.Contains(s, "nothing to commit") -} diff --git a/submitqueue/extension/pusher/git/git_pusher_test.go b/submitqueue/extension/pusher/git/git_pusher_test.go deleted file mode 100644 index 27791298..00000000 --- a/submitqueue/extension/pusher/git/git_pusher_test.go +++ /dev/null @@ -1,642 +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 git - -import ( - "bytes" - "context" - "errors" - "fmt" - "os" - "path/filepath" - "strconv" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber-go/tally" - "go.uber.org/zap/zaptest" - - "github.com/uber/submitqueue/platform/base/change" - "github.com/uber/submitqueue/submitqueue/core/changeset/fake" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/pusher" -) - -const pinnedGitVersion = "2.55.0" - -// gitFixture provides a bare "remote" repository plus a working checkout -// that pushes to it. Tests run real `git` commands so we exercise the same -// code path as production. -// -// The fixture also exposes helpers for pushing additional commits to side -// branches on the remote, so each test can build the SHAs it needs the -// Pusher to cherry-pick. -type gitFixture struct { - root string - remoteDir string - checkoutDir string - authorDir string // a separate working clone used to author "PR" commits - resolver *fake.Resolver -} - -func setupGitFixture(t *testing.T) gitFixture { - t.Helper() - - root := t.TempDir() - remoteDir := filepath.Join(root, "remote.git") - checkoutDir := filepath.Join(root, "checkout") - authorDir := filepath.Join(root, "author") - - mustGit(t, root, "init", "--bare", "-b", "main", remoteDir) - - // Seed main with one initial commit so the Pusher's reset/fetch flow has - // something to land on. - mustGit(t, root, "init", "-b", "main", authorDir) - configRepo(t, authorDir, "author", "author@example.com") - require.NoError(t, writeFile(filepath.Join(authorDir, "hello.txt"), "hello\nworld\n")) - mustGit(t, authorDir, "add", ".") - mustGit(t, authorDir, "commit", "-m", "seed") - mustGit(t, authorDir, "remote", "add", "origin", remoteDir) - mustGit(t, authorDir, "push", "origin", "main") - - mustGit(t, root, "clone", remoteDir, checkoutDir) - configRepo(t, checkoutDir, "checkout", "checkout@example.com") - - return gitFixture{ - root: root, - remoteDir: remoteDir, - checkoutDir: checkoutDir, - authorDir: authorDir, - resolver: fake.New(), - } -} - -func TestGitRuntimeValidate(t *testing.T) { - abs, err := filepath.Abs("git") - require.NoError(t, err) - tests := []struct { - name string - runtime GitRuntime - wantErr bool - }{ - { - name: "valid", - runtime: GitRuntime{ - Executable: abs, - ExecPath: abs, - TemplateDir: abs, - }, - }, - { - name: "missing executable", - runtime: GitRuntime{ - ExecPath: abs, - TemplateDir: abs, - }, - wantErr: true, - }, - { - name: "relative exec path", - runtime: GitRuntime{ - Executable: abs, - ExecPath: "git-core", - TemplateDir: abs, - }, - wantErr: true, - }, - { - name: "relative template dir", - runtime: GitRuntime{ - Executable: abs, - ExecPath: abs, - TemplateDir: "templates", - }, - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.runtime.validate() - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - }) - } -} - -func TestNewGitCommandDoesNotInheritEnvironment(t *testing.T) { - t.Setenv("SUBMITQUEUE_GIT_AMBIENT", "ambient") - runtime := testGitRuntime(t) - - cmd := newGitCommand(context.Background(), runtime, t.TempDir(), "--version") - - assert.Equal(t, runtime.Executable, cmd.Path) - assert.NotContains(t, cmd.Env, "SUBMITQUEUE_GIT_AMBIENT=ambient") - assert.Contains(t, cmd.Env, "GIT_CONFIG_NOSYSTEM=1") - assert.Contains(t, cmd.Env, "GIT_EXEC_PATH="+runtime.ExecPath) -} - -// batchFor seeds the fixture resolver so batch id resolves to the given changes, -// and returns the batch to pass to Push. -func (f gitFixture) batchFor(id string, changes ...change.Change) entity.Batch { - f.resolver.Set(id, changes...) - return entity.Batch{ID: id} -} - -// configRepo applies the test-only config that lets git commit work in a -// sandbox without GPG signing, system identity, or system git hooks. The -// devpod environment installs hooks via the system git config (core.hooksPath -// = /etc/git-hooks) that interfere with test commits — point each test repo -// at an empty hooks dir to disarm them without resorting to --no-verify. -func configRepo(t *testing.T, dir, name, email string) { - mustGit(t, dir, "config", "user.name", name) - mustGit(t, dir, "config", "user.email", email) - mustGit(t, dir, "config", "commit.gpgsign", "false") - mustGit(t, dir, "config", "tag.gpgsign", "false") - - hooksDir := filepath.Join(dir, ".no-hooks") - require.NoError(t, os.MkdirAll(hooksDir, 0o755)) - mustGit(t, dir, "config", "core.hooksPath", hooksDir) -} - -// pushPRCommit creates a single commit on a feature branch on the remote -// branched off the *current* origin/main, returning the resulting SHA. -func (f gitFixture) pushPRCommit(t *testing.T, branch, path, contents, message string) string { - t.Helper() - mustGit(t, f.authorDir, "fetch", "origin") - return f.pushPRCommitFrom(t, "origin/main", branch, path, contents, message) -} - -// pushPRCommitFrom creates a single commit on `branch` based on the given -// base ref, returning the resulting SHA. Use this to create branches that -// diverge from a specific ancestor (so they conflict when cherry-picked -// onto a newer main). -func (f gitFixture) pushPRCommitFrom(t *testing.T, base, branch, path, contents, message string) string { - t.Helper() - mustGit(t, f.authorDir, "checkout", "-B", branch, base) - require.NoError(t, writeFile(filepath.Join(f.authorDir, path), contents)) - mustGit(t, f.authorDir, "add", ".") - mustGit(t, f.authorDir, "commit", "-m", message) - mustGit(t, f.authorDir, "push", "-f", "origin", branch) - out := mustGitOutput(t, f.authorDir, "rev-parse", "HEAD") - return strings.TrimSpace(string(out)) -} - -// remoteSHA returns the SHA at refs/heads/ on the bare remote. -func (f gitFixture) remoteSHA(t *testing.T, branch string) string { - t.Helper() - out := mustGitOutput(t, f.remoteDir, "rev-parse", branch) - return strings.TrimSpace(string(out)) -} - -// landOnMain cherry-picks an arbitrary SHA directly onto main on the -// remote, simulating "this content is already on the target branch" for -// rebased-out tests. -func (f gitFixture) landOnMain(t *testing.T, sha string) { - t.Helper() - mustGit(t, f.authorDir, "fetch", "origin") - mustGit(t, f.authorDir, "checkout", "-B", "land", "origin/main") - mustGit(t, f.authorDir, "cherry-pick", sha) - mustGit(t, f.authorDir, "push", "origin", "land:main") -} - -// uri builds a github-format URI ending in `sha` so the Pusher's parser -// resolves it to that SHA. -func uri(sha string) string { - return fmt.Sprintf("github://github.example.com/uber/submitqueue/pull/1/%s", sha) -} - -func (f gitFixture) newPusher(t *testing.T) pusher.Pusher { - p, err := NewPusher(Params{ - CheckoutPath: f.checkoutDir, - Remote: "origin", - Target: "main", - Resolver: f.resolver, - Logger: zaptest.NewLogger(t).Sugar(), - MetricsScope: tally.NoopScope, - Runtime: testGitRuntime(t), - }) - require.NoError(t, err) - return p -} - -// installRaceHook writes a pre-receive hook on the bare remote that -// simulates concurrent pushes. On its Nth invocation it reads the Nth line -// of race-shas, points refs/heads/main at that SHA via update-ref, and -// exits 1 (rejecting the current push). Once race-shas is exhausted it -// exits 0 and the push goes through. -// -// Combined with the gitPusher's contention-detection (refetch + compare -// base SHA), this lets a test drive the full retry loop using only real -// git mechanics — the second-attempt's reset picks up the moved tip and -// proceeds from there. -func (f gitFixture) installRaceHook(t *testing.T, raceSHAs []string) { - t.Helper() - hookDir := filepath.Join(f.remoteDir, "hooks") - require.NoError(t, os.MkdirAll(hookDir, 0o755)) - // Override the system-wide core.hooksPath so the hook we just wrote - // actually fires on the bare remote (the devpod sets a global - // /etc/git-hooks directory that would otherwise win). - mustGit(t, f.remoteDir, "config", "core.hooksPath", hookDir) - require.NoError(t, writeFile( - filepath.Join(hookDir, "race-shas"), - strings.Join(raceSHAs, "\n")+"\n", - )) - const script = `#!/bin/sh -counter_file="$GIT_DIR/hooks/race-counter" -race_sha_file="$GIT_DIR/hooks/race-shas" -count=$(cat "$counter_file" 2>/dev/null || echo 0) -count=$((count + 1)) -echo "$count" > "$counter_file" -next_sha=$(sed -n "${count}p" "$race_sha_file") -if [ -z "$next_sha" ]; then - exit 0 -fi -# Pre-receive runs in git's quarantine env; unset its markers so update-ref -# is allowed to mutate the live ref store. -unset GIT_QUARANTINE_PATH GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES -"$GIT_EXEC_PATH/git" update-ref refs/heads/main "$next_sha" -echo "race hook moved main to $next_sha and rejected push" >&2 -exit 1 -` - hookPath := filepath.Join(hookDir, "pre-receive") - require.NoError(t, os.WriteFile(hookPath, []byte(script), 0o755)) -} - -// hookInvocations returns the number of times the pre-receive race hook -// has fired. Used by retry tests to verify the loop ran the expected -// number of attempts. -func (f gitFixture) hookInvocations(t *testing.T) int { - t.Helper() - data, err := os.ReadFile(filepath.Join(f.remoteDir, "hooks", "race-counter")) - if errors.Is(err, os.ErrNotExist) { - return 0 - } - require.NoError(t, err) - n, err := strconv.Atoi(strings.TrimSpace(string(data))) - require.NoError(t, err) - return n -} - -// remoteHEAD returns the SHA that origin/main currently points to. -func (f gitFixture) remoteHEAD(t *testing.T) string { - out := mustGitOutput(t, f.remoteDir, "rev-parse", "main") - return strings.TrimSpace(string(out)) -} - -// remoteFile returns the contents of `path` at origin/main on the bare remote. -func (f gitFixture) remoteFile(t *testing.T, path string) string { - out := mustGitOutput(t, f.remoteDir, "show", "main:"+path) - return string(out) -} - -// remoteCommitsSinceSeed returns commit SHAs on origin/main newer than the -// first (seed) commit, in chronological order. -func (f gitFixture) remoteCommitsSinceSeed(t *testing.T) []string { - out := mustGitOutput(t, f.remoteDir, "log", "--reverse", "--format=%H", "main") - all := strings.Split(strings.TrimSpace(string(out)), "\n") - if len(all) <= 1 { - return nil - } - return all[1:] -} - -func TestPusher_Push_SingleChangeSingleURIProducesOneCommit(t *testing.T) { - f := setupGitFixture(t) - sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - p := f.newPusher(t) - - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(sha)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes, 1) - - out := res.Batches[0].Outcomes[0] - assert.Equal(t, entity.OutcomeStatusCommitted, out.Status) - require.Len(t, out.CommitSHAs, 1) - assert.Equal(t, []string{out.CommitSHAs[0]}, f.remoteCommitsSinceSeed(t)) - assert.Equal(t, "hello\nearth\n", f.remoteFile(t, "hello.txt")) -} - -func TestPusher_Push_StackedURIsProduceMultipleCommitsForOneChange(t *testing.T) { - f := setupGitFixture(t) - // Build a stack on a single branch so the second SHA's parent is the first SHA. - mustGit(t, f.authorDir, "fetch", "origin") - mustGit(t, f.authorDir, "checkout", "-B", "feature/stack", "origin/main") - - require.NoError(t, writeFile(filepath.Join(f.authorDir, "hello.txt"), "hello\nearth\n")) - mustGit(t, f.authorDir, "add", ".") - mustGit(t, f.authorDir, "commit", "-m", "step 1") - sha1 := strings.TrimSpace(string(mustGitOutput(t, f.authorDir, "rev-parse", "HEAD"))) - - require.NoError(t, writeFile(filepath.Join(f.authorDir, "hello.txt"), "hello\nearth\ngoodbye\n")) - mustGit(t, f.authorDir, "add", ".") - mustGit(t, f.authorDir, "commit", "-m", "step 2") - sha2 := strings.TrimSpace(string(mustGitOutput(t, f.authorDir, "rev-parse", "HEAD"))) - - mustGit(t, f.authorDir, "push", "-f", "origin", "feature/stack") - - p := f.newPusher(t) - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(sha1), uri(sha2)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes, 1) - - out := res.Batches[0].Outcomes[0] - assert.Equal(t, entity.OutcomeStatusCommitted, out.Status) - require.Len(t, out.CommitSHAs, 2) - assert.Equal(t, out.CommitSHAs, f.remoteCommitsSinceSeed(t)) - assert.Equal(t, "hello\nearth\ngoodbye\n", f.remoteFile(t, "hello.txt")) -} - -func TestPusher_Push_AlreadyLandedChangeIsRebasedOut(t *testing.T) { - f := setupGitFixture(t) - sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - - // Land the same content on main outside the Pusher so the cherry-pick - // finds nothing new to add. - f.landOnMain(t, sha) - mainBeforePush := f.remoteHEAD(t) - - p := f.newPusher(t) - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(sha)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes, 1) - - out := res.Batches[0].Outcomes[0] - assert.Equal(t, entity.OutcomeStatusAlreadyExisted, out.Status) - assert.Empty(t, out.CommitSHAs) - assert.Equal(t, mainBeforePush, f.remoteHEAD(t), - "rebased-out push should not advance the remote tip") -} - -func TestPusher_Push_MixedChangesPartiallyRebasedOut(t *testing.T) { - f := setupGitFixture(t) - subsumedSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - f.landOnMain(t, subsumedSHA) - - freshSHA := f.pushPRCommit(t, "feature/b", "extra.txt", "extra\n", "add extra") - - p := f.newPusher(t) - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(subsumedSHA)}}, change.Change{URIs: []string{uri(freshSHA)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes, 2) - - assert.Equal(t, entity.OutcomeStatusAlreadyExisted, res.Batches[0].Outcomes[0].Status) - assert.Empty(t, res.Batches[0].Outcomes[0].CommitSHAs) - - assert.Equal(t, entity.OutcomeStatusCommitted, res.Batches[0].Outcomes[1].Status) - require.Len(t, res.Batches[0].Outcomes[1].CommitSHAs, 1) - - assert.Equal(t, "extra\n", f.remoteFile(t, "extra.txt")) -} - -func TestPusher_Push_ConflictReturnsErrConflictAndDoesNotPush(t *testing.T) { - f := setupGitFixture(t) - seedSHA := f.remoteSHA(t, "main") - - // Both branches start from the same seed and change the same line in - // different ways, then "earth" lands first directly on main. The - // Pusher's attempt to land "mars" must conflict. - earthSHA := f.pushPRCommitFrom(t, seedSHA, "feature/a", "hello.txt", "hello\nearth\n", "earth") - f.landOnMain(t, earthSHA) - mainBefore := f.remoteHEAD(t) - - conflictingSHA := f.pushPRCommitFrom(t, seedSHA, "feature/b", "hello.txt", "hello\nmars\n", "mars") - - p := f.newPusher(t) - _, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(conflictingSHA)}}), - }) - require.Error(t, err) - assert.True(t, errors.Is(err, pusher.ErrConflict)) - - assert.Equal(t, mainBefore, f.remoteHEAD(t), - "on conflict the remote tip must not move") -} - -func TestPusher_Push_ResetsBetweenCalls(t *testing.T) { - f := setupGitFixture(t) - sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - p := f.newPusher(t) - - // Dirty the checkout so that, without a reset, subsequent operations - // would fail or include unrelated changes. - require.NoError(t, writeFile(filepath.Join(f.checkoutDir, "stray.txt"), "leftover\n")) - - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(sha)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes[0].CommitSHAs, 1) - - out := mustGitOutput(t, f.remoteDir, "ls-tree", "--name-only", res.Batches[0].Outcomes[0].CommitSHAs[0]) - assert.NotContains(t, string(out), "stray.txt", "unrelated file should not have landed") - assert.Contains(t, string(out), "hello.txt") -} - -func TestPusher_Push_RecoversAfterPriorConflict(t *testing.T) { - f := setupGitFixture(t) - seedSHA := f.remoteSHA(t, "main") - - first := f.pushPRCommitFrom(t, seedSHA, "feature/a", "hello.txt", "hello\nearth\n", "earth") - f.landOnMain(t, first) - conflictingSHA := f.pushPRCommitFrom(t, seedSHA, "feature/b", "hello.txt", "hello\nmars\n", "mars") - - p := f.newPusher(t) - _, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(conflictingSHA)}}), - }) - require.Error(t, err) - - // A subsequent, clean push must succeed even though the prior call left - // a cherry-pick in progress before its rollback. - freshSHA := f.pushPRCommit(t, "feature/c", "extra.txt", "extra\n", "add extra") - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b2", change.Change{URIs: []string{uri(freshSHA)}}), - }) - require.NoError(t, err) - assert.Equal(t, entity.OutcomeStatusCommitted, res.Batches[0].Outcomes[0].Status) - assert.Equal(t, "extra\n", f.remoteFile(t, "extra.txt")) -} - -func TestPusher_Push_RejectsEmptyChanges(t *testing.T) { - f := setupGitFixture(t) - p := f.newPusher(t) - - _, err := p.Push(context.Background(), nil) - require.Error(t, err) - assert.False(t, errors.Is(err, pusher.ErrConflict)) -} - -func TestPusher_Push_InvalidURIErrors(t *testing.T) { - f := setupGitFixture(t) - p := f.newPusher(t) - - _, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{"not a uri"}}), - }) - require.Error(t, err) -} - -func TestPusher_Push_RetriesWhenRemoteMovesUnderUs(t *testing.T) { - f := setupGitFixture(t) - // Pre-stage one race commit, install the hook, then build the feature - // commit. Order matters: pushPRCommit also goes through the hook, so - // race + feature must be on the remote before the hook is armed. - raceSHA := f.pushPRCommit(t, "race", "race.txt", "race\n", "race commit") - featureSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - f.installRaceHook(t, []string{raceSHA}) - - p := f.newPusher(t) - res, err := p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(featureSHA)}}), - }) - require.NoError(t, err) - require.Len(t, res.Batches[0].Outcomes, 1) - require.Len(t, res.Batches[0].Outcomes[0].CommitSHAs, 1) - - assert.Equal(t, 2, f.hookInvocations(t), - "first attempt rejected by hook, second attempt allowed through") - - commits := f.remoteCommitsSinceSeed(t) - require.Len(t, commits, 2) - assert.Equal(t, raceSHA, commits[0], - "race commit landed first via the hook") - assert.Equal(t, res.Batches[0].Outcomes[0].CommitSHAs[0], commits[1], - "our cherry-pick landed on top after the retry") - assert.Equal(t, "hello\nearth\n", f.remoteFile(t, "hello.txt")) -} - -func TestPusher_Push_GivesUpAfterMaxAttempts(t *testing.T) { - f := setupGitFixture(t) - raceSHAs := []string{ - f.pushPRCommit(t, "race1", "r1.txt", "1\n", "r1"), - f.pushPRCommit(t, "race2", "r2.txt", "2\n", "r2"), - } - featureSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") - f.installRaceHook(t, raceSHAs) - - p, err := NewPusher(Params{ - CheckoutPath: f.checkoutDir, - Remote: "origin", - Target: "main", - Resolver: f.resolver, - Logger: zaptest.NewLogger(t).Sugar(), - MetricsScope: tally.NoopScope, - Runtime: testGitRuntime(t), - MaxPushAttempts: 2, - }) - require.NoError(t, err) - _, err = p.Push(context.Background(), []entity.Batch{ - f.batchFor("b", change.Change{URIs: []string{uri(featureSHA)}}), - }) - require.Error(t, err) - assert.Equal(t, 2, f.hookInvocations(t), - "both attempts hit the hook before the cap kicked in") - // The remote ended up at race2 (the last hook injection), and our - // feature commit never landed. - assert.Equal(t, raceSHAs[1], f.remoteHEAD(t)) -} - -// --- helpers --- - -func TestPinnedGitVersion(t *testing.T) { - out := mustGitOutput(t, t.TempDir(), "--version") - assert.Equal(t, "git version "+pinnedGitVersion, strings.TrimSpace(string(out))) -} - -func mustGit(t *testing.T, dir string, args ...string) { - t.Helper() - cmd := newGitCommand(context.Background(), testGitRuntime(t), dir, args...) - var stderr bytes.Buffer - cmd.Stderr = &stderr - require.NoError(t, cmd.Run(), "git %s: %s", strings.Join(args, " "), stderr.String()) -} - -func mustGitOutput(t *testing.T, dir string, args ...string) []byte { - t.Helper() - cmd := newGitCommand(context.Background(), testGitRuntime(t), dir, args...) - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - require.NoError(t, cmd.Run(), "git %s: %s", strings.Join(args, " "), stderr.String()) - return stdout.Bytes() -} - -func testGitRuntime(t *testing.T) GitRuntime { - t.Helper() - executable := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT") - templateDescription := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") - return GitRuntime{ - Executable: executable, - ExecPath: filepath.Dir(executable), - TemplateDir: filepath.Dir(templateDescription), - } -} - -func absoluteTestPath(t *testing.T, name string) string { - t.Helper() - path := os.Getenv(name) - require.NotEmpty(t, path) - if filepath.IsAbs(path) { - _, err := os.Stat(path) - require.NoError(t, err) - return path - } - if absolute, err := filepath.Abs(path); err == nil { - if _, err := os.Stat(absolute); err == nil { - return absolute - } - } - - // rules_go expands $(location) to an execroot-relative path. Tests run - // from their main-repository runfiles directory, so translate an external - // output to its canonical repository path under TEST_SRCDIR. - const externalMarker = "/external/" - slashed := filepath.ToSlash(path) - externalPath := "" - if strings.HasPrefix(slashed, "external/") { - externalPath = strings.TrimPrefix(slashed, "external/") - } else if i := strings.Index(slashed, externalMarker); i >= 0 { - externalPath = slashed[i+len(externalMarker):] - } - if externalPath != "" { - runfilesRoot := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, runfilesRoot) - candidate := filepath.Join(runfilesRoot, filepath.FromSlash(externalPath)) - _, err := os.Stat(candidate) - require.NoError(t, err) - return candidate - } - - require.FailNowf(t, "resolve test path", "%s=%q is not a runfile", name, path) - return "" -} - -func writeFile(path, contents string) error { - return os.WriteFile(path, []byte(contents), 0o644) -} diff --git a/submitqueue/extension/pusher/mock/BUILD.bazel b/submitqueue/extension/pusher/mock/BUILD.bazel deleted file mode 100644 index d548027e..00000000 --- a/submitqueue/extension/pusher/mock/BUILD.bazel +++ /dev/null @@ -1,13 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library") - -go_library( - name = "go_default_library", - srcs = ["pusher_mock.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/pusher/mock", - visibility = ["//visibility:public"], - deps = [ - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/pusher:go_default_library", - "@org_uber_go_mock//gomock:go_default_library", - ], -) diff --git a/submitqueue/extension/pusher/mock/pusher_mock.go b/submitqueue/extension/pusher/mock/pusher_mock.go deleted file mode 100644 index 0101e0c7..00000000 --- a/submitqueue/extension/pusher/mock/pusher_mock.go +++ /dev/null @@ -1,97 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: pusher.go -// -// Generated by this command: -// -// mockgen -source=pusher.go -destination=mock/pusher_mock.go -package=mock -// - -// Package mock is a generated GoMock package. -package mock - -import ( - context "context" - reflect "reflect" - - entity "github.com/uber/submitqueue/submitqueue/entity" - pusher "github.com/uber/submitqueue/submitqueue/extension/pusher" - gomock "go.uber.org/mock/gomock" -) - -// MockPusher is a mock of Pusher interface. -type MockPusher struct { - ctrl *gomock.Controller - recorder *MockPusherMockRecorder - isgomock struct{} -} - -// MockPusherMockRecorder is the mock recorder for MockPusher. -type MockPusherMockRecorder struct { - mock *MockPusher -} - -// NewMockPusher creates a new mock instance. -func NewMockPusher(ctrl *gomock.Controller) *MockPusher { - mock := &MockPusher{ctrl: ctrl} - mock.recorder = &MockPusherMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockPusher) EXPECT() *MockPusherMockRecorder { - return m.recorder -} - -// Push mocks base method. -func (m *MockPusher) Push(ctx context.Context, batches []entity.Batch) (entity.PushResult, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Push", ctx, batches) - ret0, _ := ret[0].(entity.PushResult) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Push indicates an expected call of Push. -func (mr *MockPusherMockRecorder) Push(ctx, batches any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Push", reflect.TypeOf((*MockPusher)(nil).Push), ctx, batches) -} - -// MockFactory is a mock of Factory interface. -type MockFactory struct { - ctrl *gomock.Controller - recorder *MockFactoryMockRecorder - isgomock struct{} -} - -// MockFactoryMockRecorder is the mock recorder for MockFactory. -type MockFactoryMockRecorder struct { - mock *MockFactory -} - -// NewMockFactory creates a new mock instance. -func NewMockFactory(ctrl *gomock.Controller) *MockFactory { - mock := &MockFactory{ctrl: ctrl} - mock.recorder = &MockFactoryMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockFactory) EXPECT() *MockFactoryMockRecorder { - return m.recorder -} - -// For mocks base method. -func (m *MockFactory) For(cfg pusher.Config) (pusher.Pusher, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "For", cfg) - ret0, _ := ret[0].(pusher.Pusher) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// For indicates an expected call of For. -func (mr *MockFactoryMockRecorder) For(cfg any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "For", reflect.TypeOf((*MockFactory)(nil).For), cfg) -} diff --git a/submitqueue/extension/pusher/pusher.go b/submitqueue/extension/pusher/pusher.go deleted file mode 100644 index b26b74f0..00000000 --- a/submitqueue/extension/pusher/pusher.go +++ /dev/null @@ -1,70 +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 pusher - -//go:generate mockgen -source=pusher.go -destination=mock/pusher_mock.go -package=mock - -import ( - "context" - "errors" - - "github.com/uber/submitqueue/submitqueue/entity" -) - -// ErrConflict is returned by a Pusher when one of the changes fails to apply -// cleanly on top of the current tip of the target branch. Callers should -// treat conflicts as user-caused and non-retryable. -var ErrConflict = errors.New("change conflict") - -// Pusher applies the changes of one or more batches on top of a target branch -// and pushes the result to the source-control remote. Each implementation is -// bound to a specific (checkout, remote, target) at construction time and -// resolves each batch's changes itself through an injected changeset resolver. -// -// Atomicity contract: when Push returns a non-nil error, NO change has been -// pushed to the remote — neither partially nor fully. Implementations must -// either roll back any local state or arrange for the push to never happen -// when any change fails to apply. Callers can treat a non-nil error as -// "the remote is exactly as it was before the call". -// -// On success, len(entity.PushResult.Batches) == len(batches) and Batches[i] -// describes what happened to batches[i], with one entity.ChangeOutcome per -// change in that batch in apply order. A change can produce multiple commits -// (entity.OutcomeStatusCommitted, CommitSHAs populated in apply order) or none -// at all (entity.OutcomeStatusAlreadyExisted, CommitSHAs empty) — the latter -// happens when the change's content is already present on the target branch. -type Pusher interface { - // Push resolves and applies the changes of the given batches, in order, - // onto the target branch and pushes the resulting commits. The batch list - // designs for a merge-train (land several ready batches in one atomic push); - // today merge passes a single batch. See the type-level docs for the - // atomicity contract. - Push(ctx context.Context, batches []entity.Batch) (entity.PushResult, error) -} - -// Config carries the per-queue identity handed to a Factory. The system knows -// only the queue name; everything an implementation needs (checkout, remote, -// target) is injected at construction by the integrator. -type Config struct { - // QueueName identifies the queue this Pusher serves. - QueueName string -} - -// Factory builds the Pusher for a queue. Implementations are provided by -// integrators (and tests) and inject whatever they need at construction. -type Factory interface { - // For returns the Pusher for the given queue. - For(cfg Config) (Pusher, error) -}