fix(tracking): reconcile tracking groups on runs that save no nodes - #1278
Draft
ogenstad wants to merge 2 commits into
Draft
fix(tracking): reconcile tracking groups on runs that save no nodes#1278ogenstad wants to merge 2 commits into
ogenstad wants to merge 2 commits into
Conversation
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.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #1278 +/- ##
====================================================
- Coverage 84.16% 84.15% -0.01%
====================================================
Files 147 147
Lines 13047 13072 +25
Branches 1930 1933 +3
====================================================
+ Hits 10981 11001 +20
- Misses 1503 1507 +4
- Partials 563 564 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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.
4 issues 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:129">
P1: When tracking targets a non-default branch, this cleanup call uses `client.default_branch` and can delete the wrong object or report a false failure. Pass `branch=self.branch` to both async and sync member deletions.</violation>
<violation number="2" location="infrahub_sdk/query_groups.py:185">
P2: The upsert (`group.save()`) now runs after `delete_unused()`. If a delete raises any non-`GraphQLError` exception (e.g. `ServerNotReachableError`, `RateLimitError`, a timeout), `update_group()` aborts before the group is saved, so the current run's members are never persisted to the group. Previously the group was saved before deletes, so current members were always recorded even when cleanup failed. Move the group upsert ahead of the reap, or wrap the reap so a hard delete failure still persists/restores the group.</violation>
<violation number="3" 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>
<violation number="4" location="infrahub_sdk/query_groups.py:297">
P1: On a non-default branch, this new zero-member path can miss the branch group or reconcile a same-named default-branch group. Pass the configured branch through the sync group lookup.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if member.id not in self.unused_member_ids or not member.typename: | ||
| continue | ||
| try: | ||
| await self.client.delete(kind=member.typename, id=member.id) |
Contributor
There was a problem hiding this comment.
P1: When tracking targets a non-default branch, this cleanup call uses client.default_branch and can delete the wrong object or report a false failure. Pass branch=self.branch to both async and sync member deletions.
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 129:
<comment>When tracking targets a non-default branch, this cleanup call uses `client.default_branch` and can delete the wrong object or report a false failure. Pass `branch=self.branch` to both async and sync member deletions.</comment>
<file context>
@@ -108,17 +108,32 @@ async def get_group(self, store_peers: bool = False) -> InfrahubNode | None:
+ if member.id not in self.unused_member_ids or not member.typename:
+ continue
+ try:
+ await self.client.delete(kind=member.typename, id=member.id)
+ except GraphQLError as exc:
+ if exc.message and "Unable to find the node" in exc.message:
</file context>
| if not members: | ||
| existing_group = None | ||
| if self.delete_unused_nodes: | ||
| existing_group = self.get_group(store_peers=True) |
Contributor
There was a problem hiding this comment.
P1: On a non-default branch, this new zero-member path can miss the branch group or reconcile a same-named default-branch group. Pass the configured branch through the sync group lookup.
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 297:
<comment>On a non-default branch, this new zero-member path can miss the branch group or reconcile a same-named default-branch group. Pass the configured branch through the sync group lookup.</comment>
<file context>
@@ -239,40 +284,49 @@ def add_related_groups(self, ids: list[str], update_group_context: bool | None =
- if not members:
+ existing_group = None
+ if self.delete_unused_nodes:
+ existing_group = self.get_group(store_peers=True)
+
+ # A run that tracked nothing and has no group to reconcile must not create an empty one.
</file context>
|
|
||
| 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>
| if existing_group: | ||
| previous_member_ids: list[str] = existing_group.members.peer_ids # type: ignore[union-attr] | ||
| self.unused_member_ids = list(set(previous_member_ids) - set(members)) | ||
| failures = await self.delete_unused() |
Contributor
There was a problem hiding this comment.
P2: The upsert (group.save()) now runs after delete_unused(). If a delete raises any non-GraphQLError exception (e.g. ServerNotReachableError, RateLimitError, a timeout), update_group() aborts before the group is saved, so the current run's members are never persisted to the group. Previously the group was saved before deletes, so current members were always recorded even when cleanup failed. Move the group upsert ahead of the reap, or wrap the reap so a hard delete failure still persists/restores the group.
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 185:
<comment>The upsert (`group.save()`) now runs after `delete_unused()`. If a delete raises any non-`GraphQLError` exception (e.g. `ServerNotReachableError`, `RateLimitError`, a timeout), `update_group()` aborts before the group is saved, so the current run's members are never persisted to the group. Previously the group was saved before deletes, so current members were always recorded even when cleanup failed. Move the group upsert ahead of the reap, or wrap the reap so a hard delete failure still persists/restores the group.</comment>
<file context>
@@ -147,40 +162,49 @@ async def add_related_groups(self, ids: list[str], update_group_context: bool |
+ if existing_group:
+ previous_member_ids: list[str] = existing_group.members.peer_ids # type: ignore[union-attr]
+ self.unused_member_ids = list(set(previous_member_ids) - set(members))
+ failures = await self.delete_unused()
+
+ # An already-empty group that stays empty needs no upsert.
</file context>
…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.
Deploying infrahub-sdk-python with
|
| Latest commit: |
ae12445
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee8f083c.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://po-tracking-group-zero-membe.infrahub-sdk-python.pages.dev |
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 and keep undeletable ones in the group for retry, with failures reported together.
Bug Fixes
delete_unused()attempts all unused members, tolerates cascade-deleted nodes, and aggregates failures asTrackingGroupCleanupError.delete_unused()now matches async error handling.Migration
delete_unused()returnsdict[str, str]instead ofNone.TrackingGroupCleanupErrorto handle partial cleanup; failed member reasons are in.failures.Written for commit a6763a9. Summary will update on new commits.