fix(tracking): reconcile tracking groups on runs that save no nodes - #1278
Draft
ogenstad wants to merge 3 commits into
Draft
fix(tracking): reconcile tracking groups on runs that save no nodes#1278ogenstad wants to merge 3 commits into
ogenstad wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #1278 +/- ##
====================================================
+ Coverage 84.57% 85.37% +0.80%
====================================================
Files 148 148
Lines 13373 14112 +739
Branches 1953 1939 -14
====================================================
+ Hits 11310 12048 +738
- Misses 1496 1498 +2
+ Partials 567 566 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 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.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrahub_sdk/query_groups.py">
<violation number="1" location="infrahub_sdk/query_groups.py:207">
P2: When a member deletion is refused, this exception escapes `InfrahubClient.__aexit__`/`__exit__` before either method resets `self.mode` to `DEFAULT`. Reset the mode in a `finally` block so subsequent non-tracking saves do not append to the stale tracking context.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| await self.delete_unused() | ||
| if failures: | ||
| raise TrackingGroupCleanupError(failures=failures) |
Contributor
There was a problem hiding this comment.
P2: When a member deletion is refused, this exception escapes InfrahubClient.__aexit__/__exit__ before either method resets self.mode to DEFAULT. Reset the mode in a finally block so subsequent non-tracking saves do not append to the stale tracking context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/query_groups.py, line 207:
<comment>When a member deletion is refused, this exception escapes `InfrahubClient.__aexit__`/`__exit__` before either method resets `self.mode` to `DEFAULT`. Reset the mode in a `finally` block so subsequent non-tracking saves do not append to the stale tracking context.</comment>
<file context>
@@ -147,40 +162,49 @@ async def add_related_groups(self, ids: list[str], update_group_context: bool |
-
- await self.delete_unused()
+ if failures:
+ raise TrackingGroupCleanupError(failures=failures)
# TODO : create anoter "read" group. Could be based of the store items
# Need to filters the store items inherited from CoreGroup to add them as children
</file context>
Deploying infrahub-sdk-python with
|
| Latest commit: |
1dc8bad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80468da8.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://po-tracking-group-zero-membe.infrahub-sdk-python.pages.dev |
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.
ogenstad
force-pushed
the
po-tracking-group-zero-member-reap
branch
from
August 31, 2026 12:53
ae12445 to
1dc8bad
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 failures 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.Implementation notes:
delete_unused()returnsdict[str, str](member id to reason) instead ofNone. Additive for callers that ignore the return value.members=[]reaches the mutation payload, and the server replaces the relationship set.What stayed the same: no change to when tracking is armed, to
delete_unused_nodesdefaults, or to the rollback-on-exception behavior.How to review
Suggested order:
infrahub_sdk/query_groups.pyasyncupdate_group()for the new control flow, then confirm the sync twin mirrors it exactly.delete_unused()in both classes.tests/integration/test_tracking_zero_members.py.Worth extra scrutiny: raising versus warning on a refused delete. Today the code already raises, 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.
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 test
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_modeAll four new tests fail on the unfixed code, verified before the fix was written:
Eight tests, four per client. Reverting only
query_groups.pyto the unfixed version, keeping the rest, gives 6 failed / 2 passed:assert 2 == 0The two that pass in both columns are deliberate: they pin the invariant that a tracked run with nothing to do creates no group, so a future change cannot start creating empty ones.
Full integration suite on this branch: 133 passed, 2 xfailed.
ruff,mypy,tyandyamllintclean.Impact & rollout
delete_unused()'s return type changes fromNonetodict[str, str], andTrackingGroupCleanupErroris new public API, which is why this targetsinfrahub-developrather than a patch line.Checklist
Summary by cubic
Fixes tracking group reconciliation when a run saves no nodes. Previously zero-member runs were no-ops that left prior members orphaned; now they prune existing members, keep undeletable ones for retry, and report failures together as
TrackingGroupCleanupError.delete_unused()tolerates any SDKError, including transport failures likeServerNotReachableError, so a mid-sweep failure no longer skips remaining members.DEFAULTmode in afinallyblock, so a raisingupdate_group()can't leave later saves tracked.Migration
delete_unused()returnsdict[str, str]instead ofNone.TrackingGroupCleanupErrorto handle partial cleanup; failed member reasons are in.failures.Written for commit 1dc8bad. Summary will update on new commits.