diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index a5b58600..11c526ab 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -40,9 +40,10 @@ Fetching by SHA guarantees the merger applies exactly the commit a URI names — | `REBASE` | Cherry-picks every commit each change introduces onto the tip, in order. A commit already present on the target is skipped (no output), as is one that was empty to begin with. | one revision per newly-created commit | | `SQUASH_REBASE` | Applies each change like `REBASE`, then collapses the commits it produced into a single commit (squash unit = the change, not the step). | one revision per change, or none for a change already present | | `MERGE` | Creates a `--no-ff` merge commit per change, keeping the change's original commits reachable through second-parent history. A commit already contained in the tip is skipped. | the merge-commit revision(s) | +| `PROMOTE` | Fast-forwards the target to an already-existing commit — no content transform, no new revision. Must be the entire request (one step, one change, one URI). | the exact named revision | | `DEFAULT` | Resolved to the instance's configured default strategy before any step runs. | per the resolved strategy | -`PROMOTE` is defined by the wire contract but not yet applied here — a step naming it is rejected as an invalid request. +`PROMOTE` is exclusive because a pre-existing commit cannot descend from commits an earlier transforming step produced, and it advances the ref to an exact SHA rather than to the locally-built HEAD. Mixing it with any other step is rejected as an invalid request. ## Importing an unrelated history @@ -58,11 +59,11 @@ Redelivery is safe: once imported, the source head is contained in the target, s `Merge` commits and reports outputs; `CheckMergeability` runs the identical apply but never pushes, then resets the checkout to discard the local commits and reports empty outputs. A multi-step check commits its intermediate steps locally so it sees the same conflict surface a real merge would. -For a committing merge nothing reaches the remote until the final push. A step that fails to apply aborts its in-progress git operation and returns without pushing. If the push fails because the remote tip moved between reset and push, the whole reset/apply/push cycle is retried up to a bounded number of attempts; detection re-fetches the tip and compares it to the SHA the cycle was based on. +For a committing merge nothing reaches the remote until the final push (a `PROMOTE` is itself a single atomic fast-forward ref update). A step that fails to apply aborts its in-progress git operation and returns without pushing. If the push fails because the remote tip moved between reset and push, the whole reset/apply/push cycle is retried up to a bounded number of attempts; detection re-fetches the tip and compares it to the SHA the cycle was based on. ## Failure classification -A merge conflict surfaces as `merger.ErrConflict`. An unusable request surfaces as `merger.ErrInvalidRequest`: an unsupported strategy or URI scheme, a malformed URI, a commit a reachable remote cannot supply, a change whose head has moved on, or a change sharing no history with the target under a picking strategy. Both are terminal — the controller publishes a `FAILED` result rather than retrying. Everything else (network/auth/push faults, and an unreachable remote) is returned as a plain error for the consumer to retry. +A merge conflict surfaces as `merger.ErrConflict`. An unusable request surfaces as `merger.ErrInvalidRequest`: an unsupported strategy or URI scheme, a malformed URI, an invalid `PROMOTE` composition, a commit a reachable remote cannot supply, a change whose head has moved on, or a change sharing no history with the target under a picking strategy. Both are terminal — the controller publishes a `FAILED` result rather than retrying. Everything else (network/auth/push faults, and an unreachable remote) is returned as a plain error for the consumer to retry. The distinction between the last two matters operationally: a commit that is missing while the remote answers is a property of the request, whereas a remote that will not answer is a property of the moment. diff --git a/runway/extension/merger/git/git_merger.go b/runway/extension/merger/git/git_merger.go index 8e1c41bf..1e64ad2e 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -23,14 +23,16 @@ // single commit (squash unit = the step). // - MERGE: create a --no-ff merge commit per URI, preserving the // original commit hashes in second-parent history. +// - PROMOTE: fast-forward the target to an already-existing commit with +// no content transform. PROMOTE must be the entire request (one step, one +// change, one URI) because a pre-existing commit cannot descend from +// commits produced by an earlier transforming step. // - DEFAULT: resolved to the instance's configured DefaultStrategy // before any step runs. // -// PROMOTE is not implemented yet; a step naming it is rejected as -// merger.ErrInvalidRequest until its apply path lands. -// // Atomicity: for a committing merge nothing reaches the remote until the final -// push. A step that fails to apply aborts the in-progress git operation and +// push (PROMOTE excepted, which is itself a single atomic fast-forward ref +// update). A step that fails to apply aborts the in-progress git operation and // returns without pushing. // // Contention: if the push fails because the remote tip moved between reset and @@ -116,7 +118,7 @@ type Params struct { // Target is the destination branch ref on the remote (e.g. "main"). Target string // DefaultStrategy resolves a step whose strategy is DEFAULT. Must be a - // concrete strategy (REBASE, SQUASH_REBASE, or MERGE). + // concrete strategy (REBASE, SQUASH_REBASE, MERGE, or PROMOTE). DefaultStrategy mergestrategypb.Strategy // Runtime is the pinned Git runtime used for every invocation. Runtime GitRuntime @@ -196,7 +198,7 @@ func NewMerger(params Params) (merger.Merger, error) { return nil, err } if !isConcreteStrategy(params.DefaultStrategy) { - return nil, fmt.Errorf("default strategy must be concrete (REBASE, SQUASH_REBASE, or MERGE), got %v", params.DefaultStrategy) + return nil, fmt.Errorf("default strategy must be concrete (REBASE, SQUASH_REBASE, MERGE, or PROMOTE), got %v", params.DefaultStrategy) } maxAttempts := params.MaxPushAttempts if maxAttempts <= 0 { @@ -284,18 +286,25 @@ func (m *gitMerger) process(ctx context.Context, req *runwaymq.MergeRequest, com "commit", commit, ) + // PROMOTE is exclusive: validated to be the entire request (one step). + if steps[0].strategy == mergestrategypb.Strategy_PROMOTE { + return m.promote(ctx, req, steps[0], commit) + } return m.applyTransforming(ctx, req, steps, commit) } -// resolveAndValidate normalizes DEFAULT strategies to the configured default -// and resolves every change URI, once, for the apply paths to work from. All failures here are terminal (merger.ErrInvalidRequest): retrying never -// succeeds, so the controller publishes a FAILED result rather than nacking. +// resolveAndValidate normalizes DEFAULT strategies to the configured default, +// resolves every change URI once for the apply paths to work from, and enforces +// the PROMOTE composition rule. All failures here are terminal +// (merger.ErrInvalidRequest): retrying never succeeds, so the controller +// publishes a FAILED result rather than nacking. func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedStep, error) { if len(req.GetSteps()) == 0 { return nil, fmt.Errorf("%w: request has no steps", merger.ErrInvalidRequest) } resolved := make([]resolvedStep, 0, len(req.GetSteps())) + promoteSeen := false for _, step := range req.GetSteps() { strategy := step.GetStrategy() if strategy == mergestrategypb.Strategy_DEFAULT { @@ -304,6 +313,10 @@ func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedSt if !isConcreteStrategy(strategy) { return nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, step.GetStrategy()) } + if strategy == mergestrategypb.Strategy_PROMOTE { + promoteSeen = true + } + ch := step.GetChange() if ch == nil || len(ch.GetUris()) == 0 { return nil, fmt.Errorf("%w: step %q has no change URIs", merger.ErrInvalidRequest, step.GetStepId()) @@ -319,6 +332,17 @@ func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedSt resolved = append(resolved, resolvedStep{step: step, strategy: strategy, refs: refs}) } + // PROMOTE must be the entire request: one step, one change, one URI. A + // pre-existing commit cannot descend from commits an earlier transforming + // step produced, and PROMOTE pushes :target directly rather than + // HEAD:target, so it cannot compose with any other step. + if promoteSeen { + if len(resolved) != 1 || + len(resolved[0].step.GetChange().GetUris()) != 1 { + return nil, fmt.Errorf("%w: PROMOTE must be the entire request (one step, one change, one URI)", merger.ErrInvalidRequest) + } + } + return resolved, nil } @@ -569,6 +593,77 @@ func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) ([]*runwaym return outputs, nil } +// promote fast-forwards the target to an already-existing commit. It is only +// reachable for a validated single-step/single-change/single-URI request. +func (m *gitMerger) promote(ctx context.Context, req *runwaymq.MergeRequest, rs resolvedStep, commit bool) (*runwaymq.MergeResult, error) { + // Validated to be one step with one change of one URI, so the request names + // exactly one commit. + ref := rs.refs[0] + sha := ref.SHA + + // PROMOTE does not go through tryApply, so it performs the same availability + // and freshness checks itself. Without them a commit the remote cannot + // supply turns every containment query into a plain error, which the + // consumer retries forever instead of reporting. They sit outside the retry + // loop because the commit under promotion is fixed for the whole request — + // only the target tip moves between attempts. + if err := m.ensureObjects(ctx, []changeRef{ref}); err != nil { + return nil, err + } + if err := m.checkStale(ctx, []changeRef{ref}); err != nil { + return nil, err + } + + var lastErr error + for attempt := 1; attempt <= m.maxPushAttempts; attempt++ { + if err := m.resetToRemote(ctx); err != nil { + return nil, err + } + tip, err := m.headSHA(ctx) + if err != nil { + return nil, err + } + + // Idempotent: the commit is already the tip or contained in it. + if sha == tip { + return promoteResult(req, rs, sha, commit), nil + } + contained, err := m.isAncestor(ctx, sha, tip) + if err != nil { + return nil, err + } + if contained { + return promoteResult(req, rs, sha, commit), nil + } + + // Only a true fast-forward is allowed; divergence is a terminal conflict. + fastForward, err := m.isAncestor(ctx, tip, sha) + if err != nil { + return nil, err + } + if !fastForward { + return nil, fmt.Errorf("%w: promote target %s is not a fast-forward of %s", merger.ErrConflict, sha, tip) + } + + if !commit { + return promoteResult(req, rs, sha, commit), nil + } + + refspec := sha + ":refs/heads/" + m.target + if _, err := m.run(ctx, nil, "push", m.remote, refspec); err != nil { + // The target may have moved under us; re-fetch and re-classify. + coremetrics.NamedCounter(m.metricsScope, "promote", "push_retries", 1) + m.logger.Warnw("promote push failed, re-classifying", + "attempt", attempt, "max_attempts", m.maxPushAttempts, "err", err) + lastErr = err + continue + } + return promoteResult(req, rs, sha, commit), nil + } + coremetrics.NamedCounter(m.metricsScope, "promote", "giveup", 1) + return nil, fmt.Errorf("exceeded %d promote attempts due to remote contention: %w", m.maxPushAttempts, lastErr) +} + // classifyMergeFailure decides what a failed `git merge` actually means, given // whether the index was left holding conflicted entries. // @@ -932,13 +1027,13 @@ func passthroughEnv(extra []string) []string { } // isConcreteStrategy reports whether s names a concrete integration strategy -// (i.e. not DEFAULT and not an unknown value). PROMOTE is not implemented yet -// and is rejected as an invalid request until its apply path lands. +// (i.e. not DEFAULT and not an unknown value). func isConcreteStrategy(s mergestrategypb.Strategy) bool { switch s { case mergestrategypb.Strategy_REBASE, mergestrategypb.Strategy_SQUASH_REBASE, - mergestrategypb.Strategy_MERGE: + mergestrategypb.Strategy_MERGE, + mergestrategypb.Strategy_PROMOTE: return true default: return false @@ -993,3 +1088,18 @@ func successResult(req *runwaymq.MergeRequest, steps []*runwaymq.StepResult) *ru Steps: steps, } } + +// promoteResult builds a SUCCEEDED MergeResult for a promote. A committing +// promote reports the promoted SHA as the step's single output; a dry-run check +// reports no output. +func promoteResult(req *runwaymq.MergeRequest, rs resolvedStep, sha string, commit bool) *runwaymq.MergeResult { + var outputs []*runwaymq.StepOutput + if commit { + outputs = []*runwaymq.StepOutput{{Id: sha}} + } + return &runwaymq.MergeResult{ + Id: req.GetId(), + Outcome: runwaypb.Outcome_SUCCEEDED, + Steps: []*runwaymq.StepResult{{StepId: rs.step.GetStepId(), Outputs: outputs}}, + } +} diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index 0b0c17ee..f1c53f16 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -497,6 +497,85 @@ func TestMerge_Merge_AlreadyAncestor(t *testing.T) { // --- PROMOTE --- +func TestMerge_Promote_MultiCommitChange(t *testing.T) { + // PROMOTE moves the ref to the named commit, so a change of any size + // arrives whole by construction — its ancestry comes along with it. + f := setupGitFixture(t) + head := f.pushMultiCommitPR(t, "feature/ff", + commitSpec{"a.txt", "a\n", "add a"}, + commitSpec{"b.txt", "b\n", "add b"}, + commitSpec{"c.txt", "c\n", "add c"}, + ) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(head)))) + require.NoError(t, err) + assert.Equal(t, head, f.remoteHEAD(t), "promote fast-forwards to the exact named commit") + assert.Equal(t, "a\n", f.remoteFile(t, "a.txt")) + assert.Equal(t, "c\n", f.remoteFile(t, "c.txt")) + require.Len(t, res.GetSteps(), 1) + require.Len(t, res.GetSteps()[0].GetOutputs(), 1) + assert.Equal(t, head, res.GetSteps()[0].GetOutputs()[0].GetId()) +} + +func TestMerge_Promote_UnavailableCommitIsInvalidNotRetryable(t *testing.T) { + // promote does not run through tryApply, so it needs its own availability + // check; otherwise the containment queries fail with a plain error and the + // consumer retries a request that can never succeed. + f := setupGitFixture(t) + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + + _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(fakeSHA)))) + require.Error(t, err) + assert.True(t, errors.Is(err, merger.ErrInvalidRequest)) + assert.False(t, errors.Is(err, merger.ErrConflict)) +} + +func TestMerge_Promote_FastForward(t *testing.T) { + f := setupGitFixture(t) + ffSHA := f.pushPRCommit(t, "feature/ff", "hello.txt", "hello\nearth\n", "ff") + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(ffSHA)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + require.Len(t, res.GetSteps(), 1) + require.Len(t, res.GetSteps()[0].GetOutputs(), 1) + assert.Equal(t, ffSHA, res.GetSteps()[0].GetOutputs()[0].GetId(), "promote reports the exact named SHA") + assert.Equal(t, ffSHA, f.remoteHEAD(t)) +} + +func TestMerge_Promote_AlreadyContained(t *testing.T) { + f := setupGitFixture(t) + seedSHA := f.remoteSHA(t, "main") + advSHA := f.pushPRCommit(t, "feature/adv", "adv.txt", "adv\n", "adv") + f.advanceMain(t, advSHA) + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(seedSHA)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + assert.Equal(t, mainBefore, f.remoteHEAD(t), "promoting an already-contained SHA does not move the tip") +} + +func TestMerge_Promote_Divergent(t *testing.T) { + f := setupGitFixture(t) + seedSHA := f.remoteSHA(t, "main") + divSHA := f.pushPRCommitFrom(t, seedSHA, "feature/div", "div.txt", "div\n", "div") + otherSHA := f.pushPRCommitFrom(t, seedSHA, "feature/other", "other.txt", "other\n", "other") + f.advanceMain(t, otherSHA) + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(divSHA)))) + require.Error(t, err) + assert.True(t, errors.Is(err, merger.ErrConflict)) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + +// --- DEFAULT --- + func TestMerge_Default_ResolvesToRebase(t *testing.T) { f := setupGitFixture(t) sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") @@ -966,8 +1045,15 @@ func TestMerge_InvalidRequests(t *testing.T) { req: req("b"), }, { - name: "unsupported strategy", - req: req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(fakeSHA))), + name: "promote with two steps", + req: req("b", + stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(fakeSHA)), + stepOf(mergestrategypb.Strategy_PROMOTE, "s2", uri(fakeSHA)), + ), + }, + { + name: "promote step with two URIs", + req: req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(fakeSHA), uri(fakeSHA))), }, { name: "malformed URI", @@ -1027,6 +1113,35 @@ func TestCheckMergeability_Conflict(t *testing.T) { assert.Equal(t, mainBefore, f.remoteHEAD(t)) } +func TestCheckMergeability_PromoteFastForward(t *testing.T) { + f := setupGitFixture(t) + ffSHA := f.pushPRCommit(t, "feature/ff", "hello.txt", "hello\nearth\n", "ff") + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.CheckMergeability(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(ffSHA)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + require.Len(t, res.GetSteps(), 1) + assert.Empty(t, res.GetSteps()[0].GetOutputs()) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + +func TestCheckMergeability_PromoteDivergent(t *testing.T) { + f := setupGitFixture(t) + seedSHA := f.remoteSHA(t, "main") + divSHA := f.pushPRCommitFrom(t, seedSHA, "feature/div", "div.txt", "div\n", "div") + otherSHA := f.pushPRCommitFrom(t, seedSHA, "feature/other", "other.txt", "other\n", "other") + f.advanceMain(t, otherSHA) + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + _, err := m.CheckMergeability(context.Background(), req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(divSHA)))) + require.Error(t, err) + assert.True(t, errors.Is(err, merger.ErrConflict)) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + func TestPinnedGitVersion(t *testing.T) { out := mustGitOutput(t, t.TempDir(), "--version") assert.Equal(t, "git version "+pinnedGitVersion, strings.TrimSpace(string(out)))