Skip to content

Commit 8ee23c0

Browse files
committed
feat(runway): classify invalid merge requests as terminal
## Summary ### Why? Runway is stateless and the sole responder on the client's correlation id: SubmitQueue records the in-flight work before publishing and waits for exactly one `MergeResult` echoing that id. A request that resolves to neither a success nor a failure leaves the client waiting forever. Today the controllers treat exactly one error as a nameable outcome — `merger.ErrConflict` — and nack everything else for retry. But some requests are unusable as written: an unknown or unsupported merge strategy, a malformed change URI, an invalid strategy composition. Retrying those can never succeed. Nacking them burns the retry budget and then dead-letters, which — with nothing draining the dead-letter topics — is precisely the silent hang described above. ### What? Adds a second terminal sentinel to the merger contract, `merger.ErrInvalidRequest`, for requests that can never be applied as written, and `merger.IsTerminal` as the single classification point over both sentinels. Switches both the `merge` and `mergeconflictcheck` controllers from an `errors.Is(err, merger.ErrConflict)` test to `merger.IsTerminal(err)`, so an invalid request now publishes a `FAILED` `MergeResult` and acks instead of nacking. The two cases stay distinguishable in observability: invalid requests increment an `invalid_requests` counter and log the underlying error, conflicts keep the existing `merge_conflicts` counter. No implementation returns the new sentinel yet — the git-backed merger later in this stack is its first producer. This lands the contract and the controller behavior first so that change is only about merge mechanics. Also corrects the Runway README, which still claimed the controllers merely deserialize and log the request. ## Test Plan ✅ `bazel test //runway/...` — 4/4 pass, including new `TestProcess_InvalidRequest` cases on both controllers asserting a FAILED result is published and the delivery is acked ✅ `make gazelle`, `make fmt`
1 parent e0bf5c1 commit 8ee23c0

6 files changed

Lines changed: 139 additions & 14 deletions

File tree

runway/README.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,8 @@ Runway is a single service (the domain *is* the service); its controllers live d
1111
- `merge-conflict-check` — dry-run check that an ordered sequence of merge steps applies cleanly, without committing.
1212
- `merge` — committing merge: apply and commit the ordered steps.
1313

14-
Both controllers currently deserialize the `MergeRequest` off the queue and log it; performing the
15-
merge and publishing a `MergeResult` to the corresponding signal queue is not wired yet.
14+
Each controller deserializes the `MergeRequest`, obtains a `Merger` for the request's queue from the [`merger`](extension/merger) extension, applies the ordered steps, and publishes a `MergeResult` to the corresponding signal queue (`merge-conflict-check-signal` / `merge-signal`). `merge` commits and reports the produced revisions; `merge-conflict-check` is a dry run that reports mergeability with empty outputs.
15+
16+
## Failure handling
17+
18+
A merge outcome the controller can name is published as a `FAILED` result and acked, not retried: a merge conflict (`merger.ErrConflict`) or an invalid request (`merger.ErrInvalidRequest` — unknown strategy, malformed change URI, invalid PROMOTE composition). The `merger.IsTerminal` helper draws that line. Any other error is an infrastructure fault and is nacked for retry.

runway/controller/merge/merge.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,15 +103,24 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
103103

104104
result, err := m.Merge(ctx, request)
105105
if err != nil {
106-
if !errors.Is(err, merger.ErrConflict) {
106+
if !merger.IsTerminal(err) {
107107
metrics.NamedCounter(c.metricsScope, opName, "merge_errors", 1)
108108
return fmt.Errorf("failed to merge for %s: %w", request.GetId(), err)
109109
}
110-
metrics.NamedCounter(c.metricsScope, opName, "merge_conflicts", 1)
111-
c.logger.Infow("merge conflict detected",
112-
"id", request.GetId(),
113-
"queue_name", request.GetQueueName(),
114-
)
110+
if errors.Is(err, merger.ErrInvalidRequest) {
111+
metrics.NamedCounter(c.metricsScope, opName, "invalid_requests", 1)
112+
c.logger.Infow("invalid merge request",
113+
"id", request.GetId(),
114+
"queue_name", request.GetQueueName(),
115+
"err", err,
116+
)
117+
} else {
118+
metrics.NamedCounter(c.metricsScope, opName, "merge_conflicts", 1)
119+
c.logger.Infow("merge conflict detected",
120+
"id", request.GetId(),
121+
"queue_name", request.GetQueueName(),
122+
)
123+
}
115124
result = &runwaymq.MergeResult{
116125
Id: request.GetId(),
117126
Outcome: runwaypb.Outcome_FAILED,

runway/controller/merge/merge_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,51 @@ func TestProcess_MergeConflict(t *testing.T) {
198198
assert.NotEmpty(t, result.Reason)
199199
}
200200

201+
func TestProcess_InvalidRequest(t *testing.T) {
202+
ctrl := gomock.NewController(t)
203+
204+
m := mergermock.NewMockMerger(ctrl)
205+
m.EXPECT().Merge(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("bad strategy: %w", merger.ErrInvalidRequest))
206+
207+
factory := mergermock.NewMockFactory(ctrl)
208+
factory.EXPECT().For(merger.Config{QueueName: testQueue}).Return(m, nil)
209+
210+
var gotTopic string
211+
var gotPayload []byte
212+
pub := queuemock.NewMockPublisher(ctrl)
213+
pub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
214+
func(_ context.Context, topic string, msg entityqueue.Message) error {
215+
gotTopic = topic
216+
gotPayload = msg.Payload
217+
return nil
218+
},
219+
)
220+
q := queuemock.NewMockQueue(ctrl)
221+
q.EXPECT().Publisher().Return(pub).AnyTimes()
222+
registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{
223+
{Key: runwaymq.TopicKeyMergeSignal, Name: "merge-signal", Queue: q},
224+
})
225+
require.NoError(t, err)
226+
227+
controller := newController(t, factory, registry)
228+
229+
req := &runwaymq.MergeRequest{
230+
Id: testID,
231+
QueueName: testQueue,
232+
Steps: []*runwaymq.MergeStep{{StepId: "step-1"}},
233+
}
234+
delivery := newDelivery(t, ctrl, requestPayload(t, req))
235+
236+
require.NoError(t, controller.Process(context.Background(), delivery))
237+
238+
assert.Equal(t, "merge-signal", gotTopic)
239+
result := &runwaymq.MergeResult{}
240+
require.NoError(t, runwaymq.Unmarshal(gotPayload, result))
241+
assert.Equal(t, testID, result.Id)
242+
assert.Equal(t, runwaypb.Outcome_FAILED, result.Outcome)
243+
assert.NotEmpty(t, result.Reason)
244+
}
245+
201246
func TestProcess_MergerInfraError(t *testing.T) {
202247
ctrl := gomock.NewController(t)
203248

runway/controller/mergeconflictcheck/mergeconflictcheck.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,15 +103,24 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
103103

104104
result, err := m.CheckMergeability(ctx, request)
105105
if err != nil {
106-
if !errors.Is(err, merger.ErrConflict) {
106+
if !merger.IsTerminal(err) {
107107
metrics.NamedCounter(c.metricsScope, opName, "check_errors", 1)
108108
return fmt.Errorf("failed to check mergeability for %s: %w", request.GetId(), err)
109109
}
110-
metrics.NamedCounter(c.metricsScope, opName, "merge_conflicts", 1)
111-
c.logger.Infow("merge conflict detected",
112-
"id", request.GetId(),
113-
"queue_name", request.GetQueueName(),
114-
)
110+
if errors.Is(err, merger.ErrInvalidRequest) {
111+
metrics.NamedCounter(c.metricsScope, opName, "invalid_requests", 1)
112+
c.logger.Infow("invalid merge request",
113+
"id", request.GetId(),
114+
"queue_name", request.GetQueueName(),
115+
"err", err,
116+
)
117+
} else {
118+
metrics.NamedCounter(c.metricsScope, opName, "merge_conflicts", 1)
119+
c.logger.Infow("merge conflict detected",
120+
"id", request.GetId(),
121+
"queue_name", request.GetQueueName(),
122+
)
123+
}
115124
result = &runwaymq.MergeResult{
116125
Id: request.GetId(),
117126
Outcome: runwaypb.Outcome_FAILED,

runway/controller/mergeconflictcheck/mergeconflictcheck_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,51 @@ func TestProcess_MergeConflict(t *testing.T) {
195195
assert.NotEmpty(t, result.Reason)
196196
}
197197

198+
func TestProcess_InvalidRequest(t *testing.T) {
199+
ctrl := gomock.NewController(t)
200+
201+
m := mergermock.NewMockMerger(ctrl)
202+
m.EXPECT().CheckMergeability(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("bad strategy: %w", merger.ErrInvalidRequest))
203+
204+
factory := mergermock.NewMockFactory(ctrl)
205+
factory.EXPECT().For(merger.Config{QueueName: testQueue}).Return(m, nil)
206+
207+
var gotTopic string
208+
var gotPayload []byte
209+
pub := queuemock.NewMockPublisher(ctrl)
210+
pub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
211+
func(_ context.Context, topic string, msg entityqueue.Message) error {
212+
gotTopic = topic
213+
gotPayload = msg.Payload
214+
return nil
215+
},
216+
)
217+
q := queuemock.NewMockQueue(ctrl)
218+
q.EXPECT().Publisher().Return(pub).AnyTimes()
219+
registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{
220+
{Key: runwaymq.TopicKeyMergeConflictCheckSignal, Name: "merge-conflict-check-signal", Queue: q},
221+
})
222+
require.NoError(t, err)
223+
224+
controller := newController(t, factory, registry)
225+
226+
req := &runwaymq.MergeRequest{
227+
Id: testID,
228+
QueueName: testQueue,
229+
Steps: []*runwaymq.MergeStep{{StepId: "step-1"}},
230+
}
231+
delivery := newDelivery(t, ctrl, requestPayload(t, req))
232+
233+
require.NoError(t, controller.Process(context.Background(), delivery))
234+
235+
assert.Equal(t, "merge-conflict-check-signal", gotTopic)
236+
result := &runwaymq.MergeResult{}
237+
require.NoError(t, runwaymq.Unmarshal(gotPayload, result))
238+
assert.Equal(t, testID, result.Id)
239+
assert.Equal(t, runwaypb.Outcome_FAILED, result.Outcome)
240+
assert.NotEmpty(t, result.Reason)
241+
}
242+
198243
func TestProcess_MergerInfraError(t *testing.T) {
199244
ctrl := gomock.NewController(t)
200245

runway/extension/merger/merger.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,20 @@ import (
3232
// result), not an infrastructure error.
3333
var ErrConflict = errors.New("merge conflict")
3434

35+
// ErrInvalidRequest signals that the request can never be applied as written —
36+
// an unknown merge strategy, a malformed change URI, or an invalid strategy
37+
// composition (e.g. PROMOTE mixed with other steps). Like ErrConflict it is a
38+
// terminal outcome: retrying never succeeds, so controllers ack and publish a
39+
// failure result rather than nacking into an infinite retry / dead-letter.
40+
var ErrInvalidRequest = errors.New("invalid merge request")
41+
42+
// IsTerminal reports whether err is a terminal merge outcome — one the client
43+
// must be told about via a FAILED result rather than retried. Controllers use
44+
// it to decide between publishing a FAILED result (ack) and nacking for retry.
45+
func IsTerminal(err error) bool {
46+
return errors.Is(err, ErrConflict) || errors.Is(err, ErrInvalidRequest)
47+
}
48+
3549
// Merger performs version-control operations against a single merge target.
3650
// Both methods accept the same MergeRequest payload; the behavioral difference
3751
// is whether the result is committed to the remote.

0 commit comments

Comments
 (0)