fixes breakage of Proposal A6: Retry policies #2577
Merged
Conversation
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
github-actions
bot
added
the
area/tooling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2577 +/- ##
==========================================
- Coverage 77.76% 77.72% -0.04%
==========================================
Files 440 440
Lines 54379 54378 -1
==========================================
- Hits 42284 42260 -24
- Misses 9481 9496 +15
- Partials 2614 2622 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vroldanbet
force-pushed
the
requestid-mw-bug
branch
from
2c8c8d3 to
3767950
Compare
vroldanbet
force-pushed
the
requestid-mw-bug
branch
3 times, most recently
from
43bd093 to
2ea3cd3
Compare
See https://github.com/grpc/proposal/blob/master/A6-client-retries.md#when-retries-are-valid As I was trying to implement an example client using retry policies, I found they didn't work. This was due to the fact SpiceDB response violated the expectations from that spec, notably: > In certain cases it is not valid to retry an RPC. These cases occur when the RPC has been committed, and thus it does not make sense to perform the retry. > The reasoning behind the first scenario is that the Response-Headers include initial metadata from the server. The metadata (or its absence) it is transmitted to the client application. This may fundamentally change the state of the client, so we cannot safely retry if a failure occurs later in the RPC’s life. The RequestID middleware was sending the requestID as a metadata header, which causes the stream to send a frame just to send the headers before sending the actual response. This in turn causes the stream to become committed, which as per A6 spec causes retry policies to be disabled. The TL;DR is: use trailers for metadata unless there is an explicit reason to use metadata headers, otherwise retry policies will become disabled.
just like with the requestID middleware, the solution is to turn it into a trailer instead of a header. This is a breaking change for zed.
vroldanbet
force-pushed
the
requestid-mw-bug
branch
from
55568d1 to
3f03e66
Compare
tstirrat15
approved these changes
Oct 1, 2025
Contributor
tstirrat15
left a comment
tstirrat15
left a comment
There was a problem hiding this comment.
I really appreciate the regression test, and I'm glad you were able to surface this. Thank you!
Contributor
|
I added a zed issue to update its |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See https://github.com/grpc/proposal/blob/master/A6-client-retries.md#when-retries-are-valid
As I attempted to implement an example client using retry policies, I found that they didn't work.
This was due to the fact SpiceDB's response violated the expectations from that spec, notably:
The RequestID middleware was sending the
requestIDas a metadata header, which causes the stream to send a frame just to send the headers before sending the actual response. This, in turn, causes the stream to become committed, which, according to the A6 spec, disables retry policies.The TL;DR is: use trailers for metadata unless there is an explicit reason to use metadata headers; otherwise, retry policies will become disabled.
This also fixes the version middleware, which was subject to the same problem.
zedmakes use of this.