Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #1278 +/- ##
====================================================
- Coverage 84.88% 81.19% -3.69%
====================================================
Files 148 149 +1
Lines 13288 14398 +1110
Branches 1955 1964 +9
====================================================
+ Hits 11279 11690 +411
- Misses 1441 2134 +693
- Partials 568 574 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Deploying infrahub-sdk-python with
|
| Latest commit: |
1cbc949
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://950af926.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://po-tracking-group-zero-membe.infrahub-sdk-python.pages.dev |
ogenstad
force-pushed
the
po-tracking-group-zero-member-reap
branch
from
August 31, 2026 12:53
ae12445 to
1dc8bad
Compare
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
update_group() returned early whenever the current run tracked no members, so it never diffed the previous membership against the empty set. A run that saved nothing left every previously tracked node in place as an orphan, still listed in the group. The pruning path now runs when the member list is empty, provided a group already exists, so a run that tracks nothing still reconciles. A run that tracks nothing with no existing group continues to create no group, and an already-empty group is not re-upserted. delete_unused() no longer aborts on the first refused delete. It attempts every unused member, returns the ones that failed, and those are reported together as TrackingGroupCleanupError. Failed members are kept in the group so a later run retries them, which the previous ordering made impossible: the group was saved before the reap, so a refused node was already out of the group and could never be seen again. InfrahubGroupContextSync.delete_unused() had no error handling at all and is now at parity with the async variant.
…e sync client The sync variant of delete_unused() previously had no error handling at all, so the sync half of the fix was the least covered. Mirrors the four async tests against InfrahubClientSync.
Review of the reaper surfaced three defects around it, all reachable now that a zero-member run performs a real cleanup. The reap deleted members on the client's default branch while the group lookup and the group upsert both used the tracking context's branch. On a non-default branch that deletes the wrong node or reports a false failure, which matters for repository imports since those run per Infrahub branch. InfrahubGroupContextSync.get_group() dropped the branch that its async twin passes, so the sync client looked up a same-named group on the default branch instead of the tracked one. delete_unused() only tolerated GraphQLError. A transport failure such as ServerNotReachableError or a rate limit escaped mid-sweep, skipping the remaining members and aborting before the group upsert. It now records any SDK Error as a failure, so the sweep completes and the group is still written with the members that could not be deleted. Also reset the client mode in a finally block on both context-manager exits. update_group() raising left the client in TRACKING mode, silently enrolling every later save into the stale context.
The reap running against the client's default branch instead of the tracked one was invisible to the rest of the suite, because every other test runs on main. These two exercise a tracked run on a branch for both clients. Reverting either branch fix makes them fail: deleting on the default branch does not find a node that only exists on the branch, so the delete is swallowed as already-deleted and the node survives, and the sync group lookup misses the branch group entirely and skips the cleanup.
Widening the reap's except clause from GraphQLError to the SDK base Error went past the problem this branch solves. It also mislabelled outages: a server that is down or a token that expired would land in the failures map as a per-member "could not be deleted" entry for every unused member, which is not a fact about any of them. Back to GraphQLError, so only a refusal by the server is collected and kept in the group. Anything else propagates for the caller to handle. The tradeoff this accepts is that a transport failure mid-reap aborts before the group upsert, so that run's membership update is lost. The next run diffs against the unchanged group and retries, so it self-heals.
The container fixtures are class-scoped, so every test class boots its own Infrahub stack. Six classes meant six boots, which pushed integration-tests-latest-infrahub past its 40 minute timeout: the job passed in 38m46s before the branch tests were added and was cancelled at 40m28s after. The tests were already isolated from each other by distinct tracking params, which give distinct group names, and distinct object names, so they did not need separate classes. Two classes now, one per client, same ten tests. Locally the file drops from 395s to 131s.
An earlier commit on this branch narrowed the reap's except clause back to GraphQLError, to stop an outage being recorded as a per-member refusal, and accepted that a transport failure mid-reap aborts before the group upsert. That loss is not symmetric. Members already in the group do self-heal on the next run, because the next diff still sees them. The nodes this run just created are in no group at all, so no later diff can reach them and they orphan permanently. Keep the labelling rule and fix the loss. A refusal is a fact about the member and is still recorded against it, with the remaining candidates still attempted. Anything else stops the reap and is returned as itself, alongside the candidates it never reached, so an outage is never blamed on a member. update_group() then writes the group first, listing this run's nodes plus the refused and unreached members, and only then re-raises the reap's own error or TrackingGroupCleanupError. delete_unused() returns a ReapResult rather than a bare dict, which names the three outcomes a caller has to handle now that the method no longer raises. Also from the same review: the already-deleted check inspects each GraphQL error instead of substring-matching the whole blob, so a cascade-deleted peer reported alongside a genuine refusal is no longer swallowed; failure reasons carry the server's message rather than the mutation that caused them; TrackingGroupCleanupError is reconstructible so it survives serialization by a task orchestrator; infrahubctl renders the failures as a table instead of a traceback; and the branch-independent decision logic moves onto the base class, unifying the member ordering the two clients had drifted on.
The zero-member decision logic and the request counts this branch claims were only exercised by the docker-backed suite, so a regression in them could not fail a plain unit run. These pin the local branches directly: a zero-member run with no group, and one with an already-empty group, each issue a single request and no mutation, and an interrupted reap still writes the group with the member it never reached before the transport error surfaces. Dropping the group write makes that last one fail for both clients. Also pins what decides the meaning of a failure: an error list carrying a cascade-deleted peer alongside a refusal is not treated as already-deleted, a reason never contains the mutation, and TrackingGroupCleanupError survives a pickle round trip. The integration tests get the match= the repo's test rules require, and class-scoped teardown for the branch, nodes and groups they create.
ogenstad
force-pushed
the
po-tracking-group-zero-member-reap
branch
from
September 10, 2026 13:11
aae78f5 to
1cbc949
Compare
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
1 participant
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
update_group()returned early whenever a run tracked zero members, so it never diffed the previous membership against the empty set. Any run that saved nothing left every previously tracked node behind as an orphan, still listed in the tracking group. This bites two ways in the field: a generator that legitimately produces nothing (a decommissioning run) never cleans up, and a repository whose last object file is removed leaves its objects stranded.While fixing that, a second defect in the same code path had to be fixed first.
delete_unused()aborted on the first refused delete, and because the group was saved before the reap, a node whose delete was refused was already out of the group and could never be retried. Removing the early return without fixing that would have turned today's silent no-op into a run-killer: every zero-member run on a group containing an undeletable node would fail and silently skip the remaining members.Closes #572. Also fixes #737 (closed as a duplicate, code never changed) and is the SDK half of opsmill/infrahub#10134.
What changed
Behavioral changes:
delete_unused()attempts every unused member instead of stopping at the first refusal, and reports the refusals together as a newTrackingGroupCleanupError.InfrahubGroupContextSync.delete_unused()had no error handling at all. It is now at parity with the async variant, including the "already deleted by cascade" tolerance added for bug: SDK Tracking feature errors out when handling parent/component deletion sequence #265.What a failed cleanup does
The reap distinguishes two kinds of failure, because only one of them is a fact about a member:
GraphQLError) is attributed to that member, and the remaining candidates are still attempted.Either way the tracking group is written before the failure surfaces, listing the nodes this run created plus the members the reap refused or never reached. That ordering matters: an interrupted reap used to abort ahead of the upsert, and while members already in the group self-heal on the next run's diff, the nodes the run had just created were in no group at all and no later run could reach them.
Also fixed in the same path
All reachable only because a zero-member run now performs a real cleanup:
InfrahubGroupContextSync.get_group()dropped the branch its async twin passes, so the sync client reconciled a same-named group on the default branch.finallyblock.update_group()raising left the client inTRACKINGmode, silently enrolling every later save into the stale context.GraphQLErrorblob, which includes the mutation text. It now inspects each error in the response, so a cascade-deleted peer reported alongside a genuine refusal is no longer swallowed.TrackingGroupCleanupErroris reconstructible from its own state, so it survives the serialization a task orchestrator applies to a failed run. It previously raisedAttributeErroron unpickling.infrahubctlrenders the failures as a table instead of falling through to a raw traceback.Implementation notes:
delete_unused()returns aReapResult(refused members, members never attempted, and the error that stopped it) instead ofNone. It no longer raises, so a baredictwould have made it too easy for a direct caller to drop a failure silently; the result type names what has to be handled.members=[]reaches the mutation payload, and the server replaces the relationship set.InfrahubGroupContextBase, so the async and sync twins share it rather than carrying two copies. That also unified the member ordering the two had quietly drifted on.What stayed the same: no change to when tracking is armed, to
delete_unused_nodesdefaults, or to the rollback-on-exception behavior.Known limitation, deliberately not addressed here: the reap issues one sequential delete per unused member, and the zero-member fix makes that path reachable with an entire membership at once. A decommission of several thousand objects is several thousand sequential round trips.
InfrahubBatchalready has the right shape (return_exceptions=True) and is the natural follow-up; batching it here would have grown this PR past reviewability.Also deliberate: with
delete_unused_nodes=Falseand zero members, the group is still left stale. Fixing that would cost a lookup on the default path.How to review
Suggested order:
infrahub_sdk/query_groups.pyasyncupdate_group()for the new control flow, then confirm the sync twin mirrors it.delete_unused()in both classes, andReapResult.tests/unit/sdk/test_group_context.py, thentests/integration/test_tracking_zero_members.py.Worth extra scrutiny: raising versus warning on a refused delete. The code already raised, just prematurely and after a partial reap, so this keeps raising but only once everything has been attempted and the group has been saved. A silent warning was the alternative, but a decommission that quietly fails to decommission seemed worse than a loud one. The consumer decides what that means in context — opsmill/infrahub#10134 catches it per import phase and continues, because the refused objects stay group members and the next import retries them.
Also worth a look: the labelling rule is load-bearing. An earlier revision of this branch widened the reap's
exceptto the SDKErrorbase and had to be reverted, because it turned one outage into a per-member "could not be deleted" entry for every unused member. The current shape keeps that narrowness without paying the membership loss it previously implied.How to test
uv run pytest tests/unit/sdk/test_group_context.py uv run pytest tests/integration/test_tracking_zero_members.py uv run pytest tests/integration/test_infrahub_client.py::TestInfrahubNode::test_tracking_mode \ tests/integration/test_infrahub_client_sync.py::TestInfrahubClientSync::test_tracking_modeThe unit file covers the decision logic and the request counts without a Docker daemon, so a regression fails a fast run: a zero-member run with no group and one with an already-empty group each issue a single request and no mutation; an interrupted reap still writes the group carrying the member it never reached before the transport error surfaces; and the pieces that decide what a failure means (a cascade-deleted peer reported alongside a refusal is not "already deleted", a reason never contains the mutation, the exception survives a pickle round trip).
Regression guard shown to bite: dropping the group write from
update_group()failstest_failed_reap_still_records_the_membershipfor both clients. Revertingquery_groups.pyto the unfixed version fails the zero-member and refused-delete integration tests for both clients.Integration coverage runs in CI; it needs a Docker daemon and was not run locally.