fix(aci): don't fire action if there is a conflict when creating WAGS #104002
Conversation
github-actions
bot
added
the
Scope: Backend
| all_statuses = WorkflowActionGroupStatus.objects.bulk_create( | ||
| missing_statuses, | ||
| batch_size=1000, | ||
| ignore_conflicts=True, | ||
| ) |
There was a problem hiding this comment.
Unfortunately the best way to find the rows that were not created was to use a SQL query. Django's bulk_create doesn't have a nice way to only populate PKs in the return list for successful creates that didn't conflict
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| When(id=action_id, then=Value(list(workflow_ids)[0])) | ||
| for action_id, workflow_ids in action_to_workflows_ids.items() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| workflow_id_cases = [ | ||
| When(id=action_id, then=Value(workflow_id)) | ||
| for action_id, workflow_id in action_to_workflow_ids.items() | ||
| When(id=action_id, then=Value(list(workflow_ids)[0])) |
There was a problem hiding this comment.
maybe min to make it consistently choose the eldest workflow, then a comment above noting that the choice of 'min' is arbitrary? I think calling out nuances like this can help some future debugging effort.
| now: datetime, statuses_to_update: set[int], missing_statuses: list[WorkflowActionGroupStatus] | ||
| ) -> None: | ||
| WorkflowActionGroupStatus.objects.filter( | ||
| ) -> tuple[int, int, list[tuple[int, int]]]: |
There was a problem hiding this comment.
document what we're returning; this is not an intuitive type.
| with connection.cursor() as cursor: | ||
| # Build values for batch insert | ||
| values_placeholders = [] | ||
| values_data = [] |
There was a problem hiding this comment.
bulk_create has a batch size because there are statement size limits. I don't know that we expect to hit them, I assume our expected upper end is hundreds here, but seems worth noting.
| ) | ||
|
|
||
| # if statuses were not created for some reason, we should not fire for them | ||
| for workflow_id, action_id in uncreated_statuses: |
There was a problem hiding this comment.
if we believe them to be conflicting, maybe 'conflicted_statuses' and this all is a bit easier to understand. If there are reasons beyond conflict where we expect them to not be created, it's less clear to me why they shouldn't be fired.
| for workflow_id, action_id in uncreated_statuses: | ||
| action_to_workflows_ids[action_id].remove(workflow_id) | ||
| if not action_to_workflows_ids[action_id]: | ||
| action_to_workflows_ids.pop(action_id, None) |
If there is a conflict when we attempt to create a
WorkflowActionGroupStatusrow, that means another process has already successfully created the row and is going to fire a notification.We should not fire an additional notification with the workflow+action that conflicted because we would not be respecting the action interval.