fix(tracking): reconcile tracking groups on runs that save no nodes - #1278
Draft
ogenstad wants to merge 6 commits into
Draft
fix(tracking): reconcile tracking groups on runs that save no nodes#1278ogenstad wants to merge 6 commits into
ogenstad wants to merge 6 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 6 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: |
aae78f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b795bc5a.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
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.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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:131">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description claims `delete_unused()` collects every SDK `Error`, but both implementations now catch only `GraphQLError`. Transport errors such as `ServerNotReachableError` therefore abort the sweep before remaining members and the group upsert run; catch the SDK `Error` base class if that behavior is intended.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| continue | ||
| try: | ||
| await self.client.delete(kind=member.typename, id=member.id, branch=self.branch) | ||
| except GraphQLError as exc: |
Contributor
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
The PR description claims delete_unused() collects every SDK Error, but both implementations now catch only GraphQLError. Transport errors such as ServerNotReachableError therefore abort the sweep before remaining members and the group upsert run; catch the SDK Error base class if that behavior is intended.
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 131:
<comment>The PR description claims `delete_unused()` collects every SDK `Error`, but both implementations now catch only `GraphQLError`. Transport errors such as `ServerNotReachableError` therefore abort the sweep before remaining members and the group upsert run; catch the SDK `Error` base class if that behavior is intended.</comment>
<file context>
@@ -127,7 +128,7 @@ async def delete_unused(self) -> dict[str, str]:
try:
await self.client.delete(kind=member.typename, id=member.id, branch=self.branch)
- except Error as exc:
+ except GraphQLError as exc:
if exc.message and "Unable to find the node" in exc.message:
# The node was already removed by the cascade delete of another node
</file context>
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.
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.Three further defects in the same code path were found in review and fixed here, 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.Deliberately out of scope:
delete_unused()still collects only server refusals (GraphQLError). A transport failure mid-reap propagates and that run's group update is lost, which self-heals because the next run diffs against the unchanged group. Widening the catch was tried and reverted: it relabelled outages as per-member "could not be deleted" failures, which is not a fact about any member. A caller that needs to handle transport errors should catch them itself.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 # 10 passed uv run pytest tests/integration/test_infrahub_client.py::TestInfrahubNode::test_tracking_mode \ tests/integration/test_infrahub_client_sync.py::TestInfrahubClientSync::test_tracking_modeTen tests, five per client. Reverting only
query_groups.pyto the unfixed version, keeping everything else, shows which behaviour each one pins:assert 2 == 0The last row passes in both columns on purpose: it pins the invariant that a tracked run with nothing to do creates no group, so a future change cannot start creating empty ones.
The branch tests are worth a note. Deleting on the default branch does not find a node that only exists on the branch, so the server answers "Unable to find the node", the cleanup treats that as already-deleted-by-cascade and skips it, and the reap reports complete success while deleting nothing. Every other test in the suite runs on
main, which is why this was invisible until it was tested directly.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()collects only server refusals (GraphQLError); a transport failure aborts before the group upsert, and the next run retries against the unchanged group.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 aae78f5. Summary will update on new commits.