Skip to content

Conversation

wedamija
Copy link
Member

@wedamija wedamija commented Oct 3, 2025

We found that for other deletions, using joins wasn't performant. Switching this over to use exists instead.

We found that for other deletions, using joins wasn't performant. Switching this over to use exists instead.
@wedamija wedamija requested review from yuvmen and a team October 3, 2025 22:49
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 3, 2025
Comment on lines +160 to +161
commit_in_release = Exists(ReleaseCommit.objects.filter(commit_id=OuterRef("commit_id")))
commit_in_head = Exists(ReleaseHeadCommit.objects.filter(commit_id=OuterRef("commit_id")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: The Exists subqueries use OuterRef("commit_id"), but the outer PullRequest model does not have a commit_id field, which will cause a FieldError.
  • Description: The get_unused_filter class method constructs a query where Exists subqueries for ReleaseCommit and ReleaseHeadCommit use OuterRef("commit_id"). This attempts to reference a commit_id field from the outer PullRequest model's context. However, the PullRequest model does not have a commit_id field. When Django's ORM attempts to build the SQL for this query, it will fail to resolve this reference and raise a FieldError. This will cause the PullRequestDeletionTask, which uses this filter, to crash, preventing the cleanup of old pull request data.

  • Suggested fix: The OuterRef("commit_id") incorrectly refers to the outer PullRequest model. The subqueries for commit_in_release and commit_in_head should be constructed to reference the commit_id from the intermediate PullRequestCommit model within the commit_exists subquery. This can be achieved by using a nested OuterRef or by restructuring the query to correctly scope the reference.
    severity: 0.8, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant