Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deduplication bug in reachable resources #1226

Merged

Conversation

josephschorr
Copy link
Member

We were previously deduplicating dispatching in reachable resources based solely on the subject found, but without being transformed for the parent resource type. This resulted in branches that produced the same subject, but with a different relation, being skipped occasionally due to a race in the dispatching.

We now do the duplication check at the dispatch point, to ensure we're checking the exact subject being dispatched.

Also adds a consistency test which reproduces the issue. Thanks to tr33 from Discord for the bug report and reproduction case!

@josephschorr josephschorr requested review from vroldanbet and a team March 24, 2023 17:00
@github-actions github-actions bot added area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Mar 24, 2023
@josephschorr josephschorr force-pushed the reachable-resources-duplicate-path branch 2 times, most recently from 0455091 to 94da686 Compare March 24, 2023 17:04
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit

@josephschorr josephschorr force-pushed the reachable-resources-duplicate-path branch from 94da686 to 6b5696a Compare April 10, 2023 15:52
We were previously deduplicating dispatching in reachable resources based solely on the subject found, but without being transformed for the parent resource type. This resulted in branches that produced the *same* subject, but with a different relation, being skipped occasionally due to a race in the dispatching.

We now do the duplication check at the dispatch point, to ensure we're checking the exact subject being dispatched.

Also adds a consistency test which reproduces the issue. Thanks to `tr33` from Discord for the bug report and reproduction case!
@josephschorr josephschorr force-pushed the reachable-resources-duplicate-path branch from 6b5696a to 1438cf2 Compare April 10, 2023 15:52
@josephschorr josephschorr enabled auto-merge April 10, 2023 15:57
@josephschorr josephschorr merged commit ed69028 into authzed:main Apr 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 10, 2023
@josephschorr josephschorr deleted the reachable-resources-duplicate-path branch May 16, 2023 00:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants