Skip to content

fix(core):Skip pruned Set entries to prevent panic in unique predicate #9449

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

Closed
wants to merge 3 commits into from

Conversation

gooohgb
Copy link
Contributor

@gooohgb gooohgb commented Jun 20, 2025

We encountered a panic issue caused by the @unique directive and found it related to https://github.com/hypermodeinc/dgraph/issues/9437
After review, I determined that the root cause lies in the mismatch between the updateMutations pruning logic and the @unique validation. Specifically:

addQueryIfUnique executes before updateMutations, generating uniqueVars entries based on all mutation predicates.

Later, updateMutations may prune some of those mutations based on @if conditions, setting their .Set to nil.

However, the unique-check logic in verifyUniqueWithinMutation does not account for pruned mutations, and still attempts to access entries from .Set, leading to a nil dereference and index out-of-range panic.

But when a mutation has only one conditional clause, this line:

if len(qc.gmuList[gmuIndex].Set) == 0 { continue }
effectively bypasses the faulty access.

When there are multiple conditional mutations, due to uniqueVars being a map, the iteration order is random. This randomness causes the issue to appear intermittently.

For unit testing, I directly copied the test case provided by @matthewmcneely to reproduce the panic reliably.

@gooohgb gooohgb requested a review from a team as a code owner June 20, 2025 09:59
}
pred1 := qc.gmuList[gmuIndex].Set[rdfIndex]
pred1Value := dql.TypeValFrom(pred1.ObjectValue).Value
for j := range qc.uniqueVars {
gmuIndex2, rdfIndex2 := decodeIndex(j)
if len(qc.gmuList[gmuIndex2].Set) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the PR! The test fails, so please check. This is not the right solution.

@matthewmcneely
Copy link
Member

@gooohgb See #9450 for a more complete sanity check in verifyUniqueWithinMutation and a much simpler test that's a lot easier to grok. That PR builds upon your initial attempt, so would welcome your review.

@gooohgb
Copy link
Contributor Author

gooohgb commented Jun 23, 2025

@gooohgb See #9450 for a more complete sanity check in verifyUniqueWithinMutation and a much simpler test that's a lot easier to grok. That PR builds upon your initial attempt, so would welcome your review.

@matthewmcneely The new PR LGTM — closing mine now.

@gooohgb gooohgb closed this Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants