Skip to content

feat: handle concurrent refreshes and improve graceful refreshing#3895

Merged
aeneasr merged 5 commits into
masterfrom
fix-concurrent-retry
Dec 17, 2024
Merged

feat: handle concurrent refreshes and improve graceful refreshing#3895
aeneasr merged 5 commits into
masterfrom
fix-concurrent-retry

Conversation

@aeneasr

@aeneasr aeneasr commented Nov 25, 2024

Copy link
Copy Markdown
Member

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

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@alnr alnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

difficult to test programatically I guess

@aeneasr

aeneasr commented Nov 25, 2024

Copy link
Copy Markdown
Member Author

We do have tests in network for this so if it passes there it's fine here too

@aeneasr

aeneasr commented Nov 28, 2024

Copy link
Copy Markdown
Member Author

difficult to test programatically I guess

It actually wasn't that hard other than that the test tooling isn't great :) I've added a test case

@aeneasr
aeneasr force-pushed the fix-concurrent-retry branch 3 times, most recently from 34f492e to e6534c2 Compare December 4, 2024 11:30
@aeneasr
aeneasr force-pushed the fix-concurrent-retry branch from e6534c2 to 812b199 Compare December 4, 2024 12:25
@aeneasr aeneasr changed the title feat: retry crdb serializable errors feat: handle concurrent refreshes and improve graceful refreshing Dec 4, 2024
Comment thread .schema/config.schema.json
Comment thread aead/aead_test.go
Comment thread consent/test/manager_test_helpers.go
Comment thread cypress/integration/oauth2/refresh_token.js
Comment thread driver/config/provider.go
Comment thread persistence/sql/persister_oauth2.go Outdated
Comment thread persistence/sql/persister_oauth2.go
Comment thread persistence/sql/persister_oauth2.go
Comment thread persistence/sql/persister_oauth2.go
Comment thread spec/config.json
Comment thread oauth2/oauth2_auth_code_test.go
@aeneasr
aeneasr requested a review from alnr December 4, 2024 13:10
@aeneasr

aeneasr commented Dec 4, 2024

Copy link
Copy Markdown
Member Author

ory/fosite#838

@aeneasr
aeneasr force-pushed the fix-concurrent-retry branch 10 times, most recently from 269fbb6 to cdf6c19 Compare December 9, 2024 10:25

@alnr alnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dense change. I did not see anything wrong with it.

@aeneasr
aeneasr requested a review from zepatrik December 10, 2024 13:49

@zepatrik zepatrik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

@aeneasr

aeneasr commented Dec 11, 2024

Copy link
Copy Markdown
Member Author

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 😅

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

@aeneasr

aeneasr commented Dec 11, 2024

Copy link
Copy Markdown
Member Author

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.

@zepatrik zepatrik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM, especially test improvements 👍

@hperl hperl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The review comments were quite helpful in understanding the change, thanks!

@aeneasr
aeneasr force-pushed the fix-concurrent-retry branch from 6aac1b3 to 1e9420d Compare December 17, 2024 08:24
@aeneasr

aeneasr commented Dec 17, 2024

Copy link
Copy Markdown
Member Author

I confirmed that a refresh token created before this patch also works after this patch.

@aeneasr
aeneasr merged commit 0a6c966 into master Dec 17, 2024
@aeneasr
aeneasr deleted the fix-concurrent-retry branch December 17, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants