fix(batch): cancel in-flight tasks when InfrahubBatch.execute() exits#1002
Open
PhillSimonds wants to merge 1 commit into
Open
fix(batch): cancel in-flight tasks when InfrahubBatch.execute() exits#1002PhillSimonds wants to merge 1 commit into
PhillSimonds wants to merge 1 commit into
Conversation
InfrahubBatch.execute() created one asyncio.Task per BatchTask but did not clean them up when the generator stopped iterating. With return_exceptions=False, the first failing task caused execute() to re-raise while siblings kept running on the event loop with no caller awaiting them. Successful sibling work was discarded silently; failing siblings surfaced only as "Task exception was never retrieved" GC warnings. Wrap the as_completed loop in try/finally that cancels any still- running task and gather()s with return_exceptions=True. Covers all generator exit paths: explicit raise, caller break, generator close. Closes #1001
Deploying infrahub-sdk-python with
|
| Latest commit: |
2d17f96
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3a664adc.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://phill-fix-batch-orphan-tasks.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #1002 +/- ##
===========================================
- Coverage 81.43% 81.42% -0.02%
===========================================
Files 134 134
Lines 11359 11352 -7
Branches 1703 1705 +2
===========================================
- Hits 9250 9243 -7
Misses 1566 1566
Partials 543 543
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
ajtmccarty
reviewed
May 11, 2026
Contributor
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
The functional code change looks good. comments on a the code comment and the second test
Comment on lines
+90
to
+92
| # Ensure no task created here outlives execute(). Cancel any still | ||
| # running, then drain so their exceptions are retrieved instead of | ||
| # surfacing later as "Task exception was never retrieved". |
Contributor
There was a problem hiding this comment.
I'd say that you get, at most, 2 lines for a comment
When raising an error, all tasks not done are canceled and/or drained
Comment on lines
+171
to
+175
| Failures are yielded as ``(node, ExceptionInstance)`` using the same tuple | ||
| shape as successes ``(node, result)``. The yielded ``node`` is whatever the | ||
| caller passed via ``batch.add(..., node=...)`` regardless of outcome, so a | ||
| consumer that does not ``isinstance``-check ``result`` cannot tell a failed | ||
| task from a successful one and will silently treat both as "created". |
Contributor
There was a problem hiding this comment.
I think this is just the expected behavior when return_exceptions=True
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
InfrahubBatch.execute()orphans in-flight tasks when one task raises withreturn_exceptions=False. Sibling work continues silently on the event loop; the caller has been told the batch failed and cannot retry; failed siblings surface only asTask exception was never retrievedGC warnings. Under a transient server-slowness window, this can yield build exit status0against a partial graph.Closes #1001
What changed
infrahub_sdk/batch.py: wrapped theasyncio.as_completediteration intry/finallythat cancels any still-running task andgather(..., return_exceptions=True)to drain. Covers all generator exit paths (raise, callerbreak, generator close), not only the original raise path.test_execute_does_not_orphan_inflight_tasks_when_raising— regression guard for the orphan behavior. Asserts that sibling task side effects do not run to completion afterexecute()raises.test_return_exceptions_yields_exceptions_indistinguishably_from_successes— pins the currentreturn_exceptions=Truecontract (failures yield as(node, ExceptionInstance)in the same tuple shape as successes). Expected to change when that API is reworked; tracked separately.How to review
Start with
infrahub_sdk/batch.py. The diff is small and self-contained. The two new tests intests/unit/sdk/test_batch.pyrun without HTTPX mocks (~0.4s total).How to test
Expected: 8 passed.
Impact & rollout
Checklist
changelog/1001.fixed.md)