fix(tracking): reconcile tracking groups on runs that save no nodes
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.
Removing the early return on its own would have turned that silent no-op into a run-killer. delete_unused() stopped at the first refused delete, and the group was saved before the reap, so a node whose delete was refused had already dropped out of the group and could never be retried.
Closes #572. Also fixes #737 (closed as a duplicate, code never changed) and is the SDK half of opsmill/infrahub#10134.
This is rebased onto the error catalogue (#1266) as a single commit and builds on its typed exceptions rather than on message text.
What changed
Behavioral changes:
- With
delete_unused_nodes=True, which generators and repository imports use, a run that tracks nothing now prunes the members of an existing tracking group instead of doing nothing. - A run that tracks nothing and has no existing group still creates no group, and an already-empty group is no longer re-upserted.
delete_unused()attempts every unused member instead of stopping at the first refusal. The members the server refused stay in the tracking group so a later run retries them, andupdate_group()reports them together as a newTrackingGroupCleanupError. A refusal that persists is raised again on every run until whatever blocks the deletion is removed; previously the member dropped out of the group after the first failure and stayed behind unnoticed.- The reap deletes members on the tracking context's branch. It used the client's default branch while the group lookup and upsert used the tracking branch, so on any other branch it deleted the wrong node or silently found nothing.
- Both context-manager exits reset the client mode in a
finallyblock.update_group()raising left the client inTRACKINGmode, silently enrolling every later save into the stale context.
What a failed cleanup does
The reap separates failures that are about the member from failures that are about the request, because only the first kind is a fact about that member:
- About the member - a server refusal such as a mandatory relationship (
UNDEFINED_ERRORor no code),PERMISSION_DENIED, or a schema that no longer has the member's kind. It is recorded against that member and the remaining candidates are still attempted. - About the request - a timeout, an unreachable server, an expired or missing token, or a branch that is gone, already merged, needs a rebase, or is locked by a merge. It stops the reap and is raised as itself rather than recorded against every remaining member in turn.
After #1266 the GraphQL path raises a coded failure as a GraphQLError even when the code describes the request (TOKEN_EXPIRED, MERGE_IN_PROGRESS, ...), so the class alone cannot separate the two. The reap reads the catalogue code instead, and falls back to the class's declared CODE for a lookup miss the SDK raised without a server behind it.
Either way the group write is attempted before the failure surfaces, listing the nodes this run created plus the members the reap refused or never reached. An interrupted reap used to abort ahead of the upsert: members already in the group self-heal on the next run's diff, but the nodes the run had just created were in no group at all, and no later run could reach them. When the write fails as well, which it usually does for the same reason, the error that stopped the reap is raised with the write's failure as its cause.
The member whose delete was interrupted is kept in the group only when the server answered and rejected it (an ApiError, or an HTTP 429 or 404 that the SDK raises as RateLimitError or URLNotFoundError), since a rejected delete removed nothing. When no answer came back, such as on a timeout, the delete may have gone through, and the server rejects a group write that names a node that no longer exists, which would lose this run's membership. So that member is left out, matching what happened before this PR.
A member already removed by another member's cascade is tolerated through #1266's NodeNotFoundError. The message check for "Unable to find the node" is kept only for servers that predate the catalogue and report no code.
Also changed
delete_unused()returns aReapResult(refused members, members never attempted, and the error that stopped the reap), importable frominfrahub_sdk.query_groups, instead of raising. Nothing in the SDK or in Infrahub calls it directly, but it is public, so the change has achangedfragment of its own.- A context reused without finding a group clears what an earlier lookup recorded, so it has nothing to reap. The old early return covered this; the reap now runs on every call.
TrackingGroupCleanupErroris exported frominfrahub_sdk.exceptionsand added to the public-names snapshot. It is not aGraphQLError, so a caller that caught a refused cleanup withexcept GraphQLErrormust catch it explicitly; thechangedfragment says so.TrackingGroupCleanupErrorsurvivespickle, so it survives the serialization a task orchestrator applies to a failed run.infrahubctlrendersTrackingGroupCleanupErroras a table of each member and the server's reason, escaped the way #1266 escapes every other error message, instead of falling through to a traceback.- The member diff, candidate selection, failure classification and the decisions around the group write live in shared helpers and in
ReapResult.failure(), so the async and sync contexts keep only their own calls.
What stayed the same: no change to when tracking is armed, to delete_unused_nodes defaults, or to the rollback-on-exception behavior.
Deliberately out of scope
- With
delete_unused_nodes=Falsethe existing group is not read, so a run that tracks nothing still leaves it unchanged, while a run that tracks one node replaces the whole membership. Reading the group on that path would add a lookup to every default tracking run. The changelog states the condition. - 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. - Known edge: a delete that times out without having gone through leaves that one member out of the group, so it stays behind as an orphan, as it did before this PR. Covering it needs a lookup after the timeout, which is the request most likely to fail next.
How to review
Suggested order:
infrahub_sdk/query_groups.py: the module-level classification helpers, then asyncupdate_group()anddelete_unused(), then confirm the sync twin matches.tests/unit/sdk/test_group_context.py, thentests/integration/test_tracking_zero_members.py.
Worth extra scrutiny: _REQUEST_FAILURE_CODES, the list of catalogue codes treated as about the request. A code missing from it is recorded against each member; a code wrongly in it stops the cleanup early.
Also deliberate: raising rather than warning on a refused delete. 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.
Raised by local cubic reviews and declined:
PERMISSION_DENIEDis counted as about the member. Object permissions are evaluated per kind and branch, so the denial is accurate for each member of that kind, and the members stay in the group either way.- A rate limit stops the reap without being in
_REQUEST_FAILURE_CODES: it arrives asRateLimitError, which is not aGraphQLError. - When a request failure stops the reap after some refusals, the raised error is the request failure alone. The refused members stay in the group, and the next run that gets through reports them; attaching them would need
add_note, which requires Python 3.11. ReapResultis not exported from the package root, matchingInfrahubGroupContext, which returns it and is not exported either.- The async and sync
update_group()still repeat their awaited calls and the four lines handling a failed write, whose bareraisehas to stay inside theexcept.
How to test
uv run pytest tests/unit/sdk/test_group_context.py tests/unit/ctl/test_utils.py tests/unit/sdk/test_exceptions_public_names.py
uv run pytest tests/integration/test_tracking_zero_members.py
The unit tests need no Docker daemon. They drive the reap end to end through httpx_mock with #1266's recorded catalogue responses, covering for both clients:
- a zero-member run with no group, and one with an already-empty group, each issue a single request and no mutation
- a coded
NODE_NOT_FOUNDduring the reap drops the member without a failure - a
MERGE_IN_PROGRESSduring the reap stops it, keeps every member in the group, and raisesMergeInProgressErrorrather than a cleanup error - a timeout during the reap still writes the group, and when the write fails as well, the error that stopped the reap is the one raised
- the member deleted before an interruption is dropped and the one never reached is kept
Regression guards checked against the pushed code: classifying every GraphQLError as a refusal, the rule before #1266 was taken into account, fails the classification cases, the locked-branch test and the failed-write test for both clients.
The integration tests need a Docker daemon and were not run locally; CI is their first run.