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

@behinddwallsbehinddwalls 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 FAILEDMergeResult 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 ownersJuly 30, 2026 14:45
@behinddwalls
behinddwallsforce-pushed the preetam/remove-dead-pusher branch from 8990caa to ed6f3a9CompareJuly 30, 2026 16:12
@behinddwalls
behinddwallsforce-pushed the preetam/runway-terminal-errors branch from 24ac344 to 8878096CompareJuly 30, 2026 16:12
@behinddwalls
behinddwalls marked this pull request as draft July 30, 2026 16:14
@behinddwalls
behinddwallsforce-pushed the preetam/remove-dead-pusher branch from ed6f3a9 to 06ef188CompareJuly 30, 2026 16:15
@behinddwalls
behinddwallsforce-pushed the preetam/runway-terminal-errors branch from 8878096 to 930df02CompareJuly 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 mainJuly 30, 2026 17:13
@behinddwalls
behinddwallsforce-pushed the preetam/runway-terminal-errors branch from 930df02 to 8ee23c0CompareJuly 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
behinddwallsforce-pushed the preetam/runway-terminal-errors branch from 8ee23c0 to 133a045CompareJuly 30, 2026 17:55
@behinddwalls
behinddwalls merged commit a8013f6 into mainJul 30, 2026
15 checks passed
@behinddwalls
behinddwalls deleted the preetam/runway-terminal-errors branch July 30, 2026 17:58
Sign up for freeto 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

@behinddwalls@kevinlnew