feat: handle concurrent refreshes and improve graceful refreshing#3895
feat: handle concurrent refreshes and improve graceful refreshing#3895
Conversation
alnr
left a comment
alnr
left a comment
There was a problem hiding this comment.
difficult to test programatically I guess
|
We do have tests in network for this so if it passes there it's fine here too |
It actually wasn't that hard other than that the test tooling isn't great :) I've added a test case |
aeneasr
force-pushed
the
fix-concurrent-retry
branch
3 times, most recently
from
34f492e to
e6534c2
Compare
aeneasr
force-pushed
the
fix-concurrent-retry
branch
from
e6534c2 to
812b199
Compare
aeneasr
changed the title
aeneasr
force-pushed
the
fix-concurrent-retry
branch
10 times, most recently
from
269fbb6 to
cdf6c19
Compare
alnr
left a comment
alnr
left a comment
There was a problem hiding this comment.
This is a dense change. I did not see anything wrong with it.
zepatrik
left a comment
zepatrik
left a comment
There was a problem hiding this comment.
It's super hard to review with all the parallel refactoring. I know that you already put in a lot of work, but maybe you could still pull out the refactoring into a separate PR? Especially refactoring and changing of relevant test cases at the same time gets super hard to review 😅
...ce/sql/migrations/20241129111700000000_add_refresh_token_access_token_link.autocommit.up.sql
Show resolved
Hide resolved
I had to change the tests because I was not able to test parallel refresh grants in SQLite. So I had to change the test suite to use cockroach etc. If you review it in IntelliJ and hide whitespace changes it should be much easier to review |
|
And the graceful refresh token rotation tests I had to anyways rewrite because the test suite was not effective. |
This patch improves Ory Hydra's ability to deal with refresh flows which, for example, concurrently refresh the same token. Furthermore, graceful token refresh has been improved to handle a variety of edge cases and scenarios.
aeneasr
force-pushed
the
fix-concurrent-retry
branch
from
1faf44d to
62a1e6f
Compare
zepatrik
left a comment
zepatrik
left a comment
There was a problem hiding this comment.
All LGTM, especially test improvements 👍
hperl
left a comment
hperl
left a comment
There was a problem hiding this comment.
LGTM! The review comments were quite helpful in understanding the change, thanks!
aeneasr
force-pushed
the
fix-concurrent-retry
branch
from
6aac1b3 to
1e9420d
Compare
|
I confirmed that a refresh token created before this patch also works after this patch. |
4 participants
This patch improves Ory Hydra's ability to deal with refresh flows which, for example, concurrently refresh the same token. Furthermore, graceful token refresh has been improved to handle a variety of edge cases and scenarios.
Additionally, serializability errors in CockroachDB are now correctly retried.
Related issue(s)
See https://github.com/ory-corp/cloud/issues/7311
Closes #3895
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments