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 worker not releasing limit slot on failed propose pending state #16012

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jdjfisher
Copy link

Fixes a bug where workers aborting a flow run submission doesn't release the concurrency slot that was claimed. I've based this on what has been done in the runner as this issue doesn't seem to exist over there

def _release_limit_slot(self, flow_run_id: str) -> None:
. I've had a quick look at putting together a test case for this but I might need a hand here. Cheers.

Closes #15952

@github-actions github-actions bot added the bug Something isn't working label Nov 14, 2024
Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #16012 will not alter performance

Comparing jdjfisher:fix-worker-limiter-release-15952 (75d87f4) with main (377505b)

Summary

✅ 3 untouched benchmarks

@desertaxle
Copy link
Member

Thanks for the PR @jdjfisher!

I think there's one other place where your new release_on_behalf_of method could be used, which would be good for consistency.

For a test, I think you could write a test similar to this one:

async def test_worker_with_work_pool_and_limit(
and mock _propose_pending_state to return False so that you can ensure that the concurrency limiter slot is correctly released.

Let me know if you have any questions about either of those points!

@jdjfisher jdjfisher force-pushed the fix-worker-limiter-release-15952 branch from d2be850 to 75d87f4 Compare November 15, 2024 08:59
@jdjfisher
Copy link
Author

Thanks for the review @desertaxle, super helpful. I'm managed to put a case together based on your suggestions that should cover this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Worker concurrency count getting stuck when aborting flow run submission
2 participants