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(aci): set workflow in job when evaluating action filters #87353

Closed
wants to merge 1 commit into from

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Mar 18, 2025

I wrote a test and found that

  1. In workflow.evaluate_trigger_conditions(job) we attach the workflow to the job
  2. If we have a set of workflows we're evaluating, the last workflow we evaluate remains attached to the job
  3. Next in evaluate_workflows_action_filters we end up passing that last workflow to every single filter/IF data condition group we evaluate, even if the filter/IF DCG is not actually attached to that last workflow

We should be correctly setting the workflow for each data condition group we evaluate.

(Wrote a test with a condition that should not be an allowable filter condition for demonstration purposes only)

@cathteng cathteng requested a review from a team as a code owner March 18, 2025 22:22
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2025
@@ -109,6 +123,8 @@ def evaluate_workflows_action_filters(
if evaluation:
filtered_action_groups.add(action_condition)

job.pop("workflow", None)
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe i don't need to pop, but not popping caused this problem in the first place

@cathteng
Copy link
Member Author

TIL dictionaries are pass by reference in Python which is why this happens

Copy link
Member

@iamrajjoshi iamrajjoshi left a comment

Choose a reason for hiding this comment

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

lgtm, good find
The Rock Nice LGTM

@saponifi3d
Copy link
Contributor

pls hold on merging -- i'd like to take a quick look.

@cathteng cathteng requested a review from saponifi3d March 18, 2025 22:29
Copy link

codecov bot commented Mar 18, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
24416 1 24415 299
View the top 1 failed test(s) by shortest run time
tests.sentry.workflow_engine.processors.test_workflow.TestProcessWorkflows::test_error_event__logger
Stack Traces | 4.32s run time
#x1B[1m#x1B[.../workflow_engine/processors/test_workflow.py#x1B[0m:100: in test_error_event__logger
    mock_logger.info.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:977: in assert_called_with
    raise AssertionError(_error_message()) from cause
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: info('workflow_engine.process_workflows.fired_workflow', extra={'workflow_id': 17, 'rule_id': 698, 'payload': {'event': <sentry.eventstore.models.GroupEvent object at 0x7f91419232f0>, 'group_state': {'id': 1, 'is_new': False, 'is_regression': True, 'is_new_group_environment': False}, 'workflow': <Workflow at 0x7f914025f350: id=17, name='error_workflow', organization_id=4555749194989572>}, 'group_id': 931, 'event_id': '550e7bf3e1da42f28b322f8bdecd2936'})#x1B[0m
#x1B[1m#x1B[31mE     Actual: info('workflow_engine.process_workflows.fired_workflow', extra={'workflow_id': 17, 'rule_id': 698, 'payload': {'event': <sentry.eventstore.models.GroupEvent object at 0x7f91419232f0>, 'group_state': {'id': 1, 'is_new': False, 'is_regression': True, 'is_new_group_environment': False}}, 'group_id': 931, 'event_id': '550e7bf3e1da42f28b322f8bdecd2936'})#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@saponifi3d
Copy link
Contributor

saponifi3d commented Mar 19, 2025

we chatted a little in slack, but i'm not 100% sold on this as the fix. it feels like we're fixing a symptom rather than treating the issue.

i'd like to take a closer look at how we're using the job here (maybe we can just grab the environment? 🤔) and think a bit more about why we need to do the pop() -- that is a smell that there's likely a bug higher up in the code and it's causing issues here.

hopefully i can get some keyboard time tomorrow and i might be able to take a look around to see if there's a more holistic fix.

@cathteng cathteng marked this pull request as draft March 19, 2025 17:08
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

imo the problem here is that we're mutating WorkflowJob. I think it should actually be a frozen dataclass. If we need to copy it for multiple workflows we can do that so that the original object isn't modified

@cathteng
Copy link
Member Author

i will try this again

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.

4 participants