docs: define the URI as the unit of change - #477
Open
behinddwalls wants to merge 1 commit into
Open
Conversation
## Summary ### Why? The contract never said what a URI is relative to a change, and the two readings lead to different behavior. Under one, a change is the unit and its URIs are pieces of it; under the other, each URI is a change and a list is a stack of them. Nothing wrote the second one down, so the first kept getting assumed — most recently in SQUASH_REBASE, which collapsed every URI of a step into one commit and erased the boundary between stacked pull requests. The strategy fields had the same hole. Nothing said whether a strategy is picked once and repeated per URI, or picked once for the list as a whole. Both `LandRequest.strategy` and `MergeStep.strategy` are singular, which reads either way. ### What? States the rule once where a change is defined, in `uber.base.change.Change` and its `platform/base/change` entity: one URI is one unit of change, and a list is an ordered set of distinct changes applied each on top of the last, not one change described several ways. Carries it to the places a strategy is chosen. The `Strategy` enum now says a strategy applies to every URI the change carries, the same way to each, and its values are defined per URI — so `SQUASH_REBASE` says the squash unit is the individual change, and a stack of three URIs becomes three commits rather than one. `PROMOTE` notes that it is the one value constraining the list rather than repeating over it: advancing a ref to an exact revision admits a single URI, because a second could not also be the revision the target ends at. The two call sites say the same in their own terms — `MergeStep.strategy` that a step is never a mix of strategies, `LandRequest.strategy` that a land request cannot pick a different strategy per URI — as do the `LandStrategy` entity fields. The git merger's README gains the corresponding implementation statement: the URI is the unit of application, and a step's outputs are the concatenation of what each URI produced. Documentation only. The behavior described is what the code already does; this is the contract catching up with it, so that the next reader does not have to infer the rule from an implementation. ## Test Plan ✅ `make proto` — regenerated stubs carry the new comments ✅ `bazel test //runway/... //service/runway/... //submitqueue/...` — 47/47 ✅ `make lint`, `make check-tidy`, `make check-gazelle`, `make test` No behavior change, so no new tests. The rule stated here is already pinned by existing cases — `TestMerge_SquashRebase_OneCommitPerChange` for the per-URI squash unit, `TestMerge_RejectsInconsistentProvider` for one provider per request, and the PROMOTE composition cases for its single-URI constraint.
This was referenced Jul 30, 2026
behinddwalls
marked this pull request as ready for review
July 30, 2026 19:57
behinddwalls
added a commit
that referenced
this pull request
Jul 30, 2026
## Summary ### Why? SubmitQueue records in-flight merge work before publishing and then waits for exactly one `MergeResult` echoing its correlation id. Runway is stateless and the sole responder on that id, so every request must resolve to a result — or the client waits forever. The primary controllers resolve what they can name: conflicts and invalid requests become a `FAILED` result, infrastructure faults are nacked for retry. But a fault that never recovers exhausts the retry budget and dead-letters. Nothing consumed those dead-letter topics, so the request produced no signal at all and the client's correlation id hung indefinitely. ### What? Adds `runway/controller/dlq`, a reconciler that subscribes to an inbound topic's `_dlq` queue and, for each dead-lettered `MergeRequest`, republishes a `FAILED` `MergeResult` echoing the correlation id to the corresponding signal topic. `dlq.TopicKey` derives the DLQ topic key from the primary one so the two stay in lockstep. Unlike the SubmitQueue and Stovepipe DLQ reconcilers this one writes no entity state — Runway has none, and the signal *is* the resolution. A payload that cannot be decoded carries no correlation id and is dropped rather than retried forever. Wires two instances in the server (one per inbound topic) on a dedicated consumer running under `errs.AlwaysRetryableProcessor`, so a transient publish failure retries indefinitely rather than dead-lettering the dead-letter. The DLQ consumer is started alongside the primary one and stopped with the same 30s drain on shutdown; both stop errors are joined into the exit status. ## Test Plan ✅ `bazel test //runway/...` — 5/5 pass, including new `//runway/controller/dlq` coverage for republish-on-dead-letter, the drop-undecodable-payload path, and publish-failure propagation ✅ `bazel build //service/runway/...` — wiring compiles ✅ `make gazelle`, `make fmt` ## Stack 1. @ #459 1. #460 1. #461 1. #462 1. #463 1. #476 1. #477
kevinlnew
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
The contract never said what a URI is relative to a change, and the two readings lead to different behavior. Under one, a change is the unit and its URIs are pieces of it; under the other, each URI is a change and a list is a stack of them. Nothing wrote the second one down, so the first kept getting assumed — most recently in SQUASH_REBASE, which collapsed every URI of a step into one commit and erased the boundary between stacked pull requests.
The strategy fields had the same hole. Nothing said whether a strategy is picked once and repeated per URI, or picked once for the list as a whole. Both
LandRequest.strategyandMergeStep.strategyare singular, which reads either way.What?
States the rule once where a change is defined, in
uber.base.change.Changeand itsplatform/base/changeentity: one URI is one unit of change, and a list is an ordered set of distinct changes applied each on top of the last, not one change described several ways.Carries it to the places a strategy is chosen. The
Strategyenum now says a strategy applies to every URI the change carries, the same way to each, and its values are defined per URI — soSQUASH_REBASEsays the squash unit is the individual change, and a stack of three URIs becomes three commits rather than one.PROMOTEnotes that it is the one value constraining the list rather than repeating over it: advancing a ref to an exact revision admits a single URI, because a second could not also be the revision the target ends at.The two call sites say the same in their own terms —
MergeStep.strategythat a step is never a mix of strategies,LandRequest.strategythat a land request cannot pick a different strategy per URI — as do theLandStrategyentity fields.The git merger's README gains the corresponding implementation statement: the URI is the unit of application, and a step's outputs are the concatenation of what each URI produced.
Documentation only. The behavior described is what the code already does; this is the contract catching up with it, so that the next reader does not have to infer the rule from an implementation.
Test Plan
✅
make proto— regenerated stubs carry the new comments✅
bazel test //runway/... //service/runway/... //submitqueue/...— 47/47✅
make lint,make check-tidy,make check-gazelle,make testNo behavior change, so no new tests. The rule stated here is already pinned by existing cases —
TestMerge_SquashRebase_OneCommitPerChangefor the per-URI squash unit,TestMerge_RejectsInconsistentProviderfor one provider per request, and the PROMOTE composition cases for its single-URI constraint.Stack