Skip to content

feat(runway): classify invalid merge requests as terminal - #458

Merged
behinddwalls merged 1 commit into
mainfrom
preetam/runway-terminal-errors
Jul 30, 2026
Merged

feat(runway): classify invalid merge requests as terminal#458
behinddwalls merged 1 commit into
mainfrom
preetam/runway-terminal-errors

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

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

@behinddwalls
behinddwalls marked this pull request as ready for review July 30, 2026 14:45
@behinddwalls
behinddwalls requested review from a team and sbalabanov as code owners July 30, 2026 14:45
@behinddwalls
behinddwalls force-pushed the preetam/remove-dead-pusher branch from 8990caa to ed6f3a9 Compare July 30, 2026 16:12
@behinddwalls
behinddwalls force-pushed the preetam/runway-terminal-errors branch from 24ac344 to 8878096 Compare July 30, 2026 16:12
@behinddwalls
behinddwalls marked this pull request as draft July 30, 2026 16:14
@behinddwalls
behinddwalls force-pushed the preetam/remove-dead-pusher branch from ed6f3a9 to 06ef188 Compare July 30, 2026 16:15
@behinddwalls
behinddwalls force-pushed the preetam/runway-terminal-errors branch from 8878096 to 930df02 Compare July 30, 2026 16:15
@behinddwalls
behinddwalls marked this pull request as ready for review July 30, 2026 17:05
Base automatically changed from preetam/remove-dead-pusher to main July 30, 2026 17:13
@behinddwalls
behinddwalls force-pushed the preetam/runway-terminal-errors branch from 930df02 to 8ee23c0 Compare July 30, 2026 17:13
## 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`
@behinddwalls
behinddwalls force-pushed the preetam/runway-terminal-errors branch from 8ee23c0 to 133a045 Compare July 30, 2026 17:55
@behinddwalls
behinddwalls merged commit a8013f6 into main Jul 30, 2026
15 checks passed
@behinddwalls
behinddwalls deleted the preetam/runway-terminal-errors branch July 30, 2026 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants