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

consolidate hook calling in flow engine #16596

Merged
merged 2 commits into from
Jan 3, 2025
Merged

consolidate hook calling in flow engine #16596

merged 2 commits into from
Jan 3, 2025

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Jan 3, 2025

closes #16427

(#16436 is not closed by this, as this is a separate issue)

@github-actions github-actions bot added the bug Something isn't working label Jan 3, 2025
@@ -3667,7 +3667,7 @@ def my_flow():
await my_flow(return_state=True)
assert my_mock.mock_calls == [call("crashed")]

async def test_on_crashed_hook_not_called_on_sigterm_from_flow_with_cancelling_state(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the hook actually should run here

@zzstoatzz zzstoatzz changed the title consolidate hook calling consolidate hook calling in flow engine Jan 3, 2025
@zzstoatzz zzstoatzz force-pushed the crash-hook branch 2 times, most recently from c2a9cd9 to 653bc7f Compare January 3, 2025 18:42
Copy link

codspeed-hq bot commented Jan 3, 2025

CodSpeed Performance Report

Merging #16596 will not alter performance

Comparing crash-hook (f678b46) with main (85f6e26)

Summary

✅ 3 untouched benchmarks

@zzstoatzz zzstoatzz added the fix A fix for a bug in an existing feature label Jan 3, 2025
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

🧼

@zzstoatzz zzstoatzz merged commit cecd3a1 into main Jan 3, 2025
38 checks passed
@zzstoatzz zzstoatzz deleted the crash-hook branch January 3, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix A fix for a bug in an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flow on_crashed hook does not run
2 participants