Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/572.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Tracking groups now reconcile correctly when a run saves no nodes at all. Previously `update_group()` returned early on an empty member list, so a generator that produced nothing (a decommissioning run) or a repository whose last object file was removed left every previously tracked node behind as an orphan, still listed in the group. A run that tracks nothing but has an existing group now prunes it; a run that tracks nothing and has no group still creates none.

Cleanup is also no longer aborted by a single refused delete. `delete_unused()` attempts every unused member and reports the failures together as `TrackingGroupCleanupError` instead of propagating the first `GraphQLError` and silently skipping the rest. Members that could not be deleted are kept in the tracking group so a later run retries them, and the sync client now has the same error tolerance as the async one.
22 changes: 14 additions & 8 deletions infrahub_sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2139,10 +2139,13 @@ async def __aexit__(
exc_value: BaseException | None,
traceback: TracebackType | None,
) -> None:
if exc_type is None and self.mode == InfrahubClientMode.TRACKING:
await self.group_context.update_group()

self.mode = InfrahubClientMode.DEFAULT
try:
if exc_type is None and self.mode == InfrahubClientMode.TRACKING:
await self.group_context.update_group()
finally:
# update_group() can raise, and leaving the client in tracking mode would
# silently enroll every later save into the stale context.
self.mode = InfrahubClientMode.DEFAULT

async def convert_object_type(
self,
Expand Down Expand Up @@ -3883,10 +3886,13 @@ def __exit__(
exc_value: BaseException | None,
traceback: TracebackType | None,
) -> None:
if exc_type is None and self.mode == InfrahubClientMode.TRACKING:
self.group_context.update_group()

self.mode = InfrahubClientMode.DEFAULT
try:
if exc_type is None and self.mode == InfrahubClientMode.TRACKING:
self.group_context.update_group()
finally:
# update_group() can raise, and leaving the client in tracking mode would
# silently enroll every later save into the stale context.
self.mode = InfrahubClientMode.DEFAULT

def convert_object_type(
self,
Expand Down
13 changes: 13 additions & 0 deletions infrahub_sdk/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@ def __init__(self, errors: list[dict[str, Any]], query: str | None = None, varia
super().__init__(self.message)


class TrackingGroupCleanupError(Error):
"""Raised when unused members of a tracking group could not be deleted.

Every unused member is attempted before this is raised, and the ones that failed are
kept in the tracking group so a later run retries them.
"""

def __init__(self, failures: dict[str, str]) -> None:
self.failures = failures
details = "; ".join(f"{node_id} ({reason})" for node_id, reason in failures.items())
super().__init__(f"Unable to delete {len(failures)} unused member(s) of the tracking group: {details}")


class VersionNotSupportedError(Error):
"""Raised when a feature is used against an Infrahub server version that does not support it."""

Expand Down
164 changes: 108 additions & 56 deletions infrahub_sdk/query_groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from typing import TYPE_CHECKING, Any

from .constants import InfrahubClientMode
from .exceptions import GraphQLError, NodeNotFoundError
from .exceptions import Error, NodeNotFoundError, TrackingGroupCleanupError
from .utils import dict_hash

if TYPE_CHECKING:
Expand Down Expand Up @@ -108,17 +108,32 @@ async def get_group(self, store_peers: bool = False) -> InfrahubNode | None:
self.previous_members = group._get_relationship_many(name="members").peers
return group

async def delete_unused(self) -> None:
if self.previous_members and self.unused_member_ids:
for member in self.previous_members:
if member.id in self.unused_member_ids and member.typename:
try:
await self.client.delete(kind=member.typename, id=member.id)
except GraphQLError as exc:
if not exc.message or "Unable to find the node" not in exc.message:
# If the node already has been deleted, skip the error as it would have been deleted
# by the cascade delete of another node
raise
async def delete_unused(self) -> dict[str, str]:
"""Delete the members that this run no longer uses.

Every candidate is attempted even when some deletes are refused, so one refusal
cannot leave the rest of the unused members behind.

Returns:
The id of each member that could not be deleted, mapped to the reason.

"""
failures: dict[str, str] = {}
if not self.previous_members or not self.unused_member_ids:
return failures

for member in self.previous_members:
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, branch=self.branch)
except Error 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
continue
failures[member.id] = exc.message or str(exc)

return failures

async def add_related_nodes(self, ids: list[str], update_group_context: bool | None = None) -> None:
"""Add related Nodes IDs to the context.
Expand Down Expand Up @@ -147,42 +162,49 @@ async def add_related_groups(self, ids: list[str], update_group_context: bool |
self.related_group_ids.extend(ids)

async def update_group(self) -> None:
"""Create or update (using upsert) a CoreStandardGroup to store all the Nodes and Groups used during an execution."""
"""Create or update (using upsert) a CoreStandardGroup to store all the Nodes and Groups used during an execution.

Raises:
TrackingGroupCleanupError: When one or more unused members could not be deleted.

"""
members: list[str] = self.related_group_ids + self.related_node_ids

if not members:
existing_group = None
if self.delete_unused_nodes:
existing_group = await self.get_group(store_peers=True)

# A run that tracked nothing and has no group to reconcile must not create an empty one.
if not members and existing_group is None:
return

failures: dict[str, str] = {}
if existing_group:
previous_member_ids: list[str] = existing_group._get_relationship_many(name="members").peer_ids
self.unused_member_ids = list(set(previous_member_ids) - set(members))
failures = await self.delete_unused()
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

# An already-empty group that stays empty needs no upsert.
if not members and not previous_member_ids:
return

group_name = self._generate_group_name()
schema = await self.client.schema.get(kind=self.group_type)
description = self._generate_group_description(schema=schema)

existing_group = None
if self.delete_unused_nodes:
existing_group = await self.get_group(store_peers=True)

# Members that could not be deleted stay in the group so a later run retries them.
group = await self.client.create(
kind=self.group_type,
name=group_name,
description=description,
members=members,
members=members + list(failures),
branch=self.branch,
**self.group_params,
)
await group.save(allow_upsert=True, update_group_context=False)

if not existing_group:
return

# Calculate how many nodes should be deleted
self.unused_member_ids = list(
set(existing_group._get_relationship_many(name="members").peer_ids) - set(members)
)

if not self.delete_unused_nodes:
return

await self.delete_unused()
if failures:
raise TrackingGroupCleanupError(failures=failures)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

# 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
# Need to validate that it's UUIDas "key" if we want to implement other methods to store item
Expand All @@ -198,7 +220,9 @@ def __init__(self, client: InfrahubClientSync) -> None:
def get_group(self, store_peers: bool = False) -> InfrahubNodeSync | None:
group_name = self._generate_group_name()
try:
group = self.client.get(kind=self.group_type, name__value=group_name, include=["members"])
group = self.client.get(
kind=self.group_type, name__value=group_name, include=["members"], branch=self.branch
)
except NodeNotFoundError:
return None

Expand All @@ -208,11 +232,32 @@ def get_group(self, store_peers: bool = False) -> InfrahubNodeSync | None:
self.previous_members = group._get_relationship_many(name="members").peers
return group

def delete_unused(self) -> None:
if self.previous_members and self.unused_member_ids:
for member in self.previous_members:
if member.id in self.unused_member_ids and member.typename:
self.client.delete(kind=member.typename, id=member.id)
def delete_unused(self) -> dict[str, str]:
"""Delete the members that this run no longer uses.

Every candidate is attempted even when some deletes are refused, so one refusal
cannot leave the rest of the unused members behind.

Returns:
The id of each member that could not be deleted, mapped to the reason.

"""
failures: dict[str, str] = {}
if not self.previous_members or not self.unused_member_ids:
return failures

for member in self.previous_members:
if member.id not in self.unused_member_ids or not member.typename:
continue
try:
self.client.delete(kind=member.typename, id=member.id, branch=self.branch)
except Error 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
continue
failures[member.id] = exc.message or str(exc)

return failures

def add_related_nodes(self, ids: list[str], update_group_context: bool | None = None) -> None:
"""Add related Nodes IDs to the context.
Expand Down Expand Up @@ -241,42 +286,49 @@ def add_related_groups(self, ids: list[str], update_group_context: bool | None =
self.related_group_ids.extend(ids)

def update_group(self) -> None:
"""Create or update (using upsert) a CoreStandardGroup to store all the Nodes and Groups used during an execution."""
"""Create or update (using upsert) a CoreStandardGroup to store all the Nodes and Groups used during an execution.

Raises:
TrackingGroupCleanupError: When one or more unused members could not be deleted.

"""
members: list[str] = self.related_node_ids + self.related_group_ids

if not members:
existing_group = None
if self.delete_unused_nodes:
existing_group = self.get_group(store_peers=True)
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

# A run that tracked nothing and has no group to reconcile must not create an empty one.
if not members and existing_group is None:
return

failures: dict[str, str] = {}
if existing_group:
previous_member_ids: list[str] = existing_group._get_relationship_many(name="members").peer_ids
self.unused_member_ids = list(set(previous_member_ids) - set(members))
failures = self.delete_unused()

# An already-empty group that stays empty needs no upsert.
if not members and not previous_member_ids:
return

group_name = self._generate_group_name()
schema = self.client.schema.get(kind=self.group_type)
description = self._generate_group_description(schema=schema)

existing_group = None
if self.delete_unused_nodes:
existing_group = self.get_group(store_peers=True)

# Members that could not be deleted stay in the group so a later run retries them.
group = self.client.create(
kind=self.group_type,
name=group_name,
description=description,
members=members,
members=members + list(failures),
branch=self.branch,
**self.group_params,
)
group.save(allow_upsert=True, update_group_context=False)

if not existing_group:
return

# Calculate how many nodes should be deleted
self.unused_member_ids = list(
set(existing_group._get_relationship_many(name="members").peer_ids) - set(members)
)

if not self.delete_unused_nodes:
return

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
Expand Down
Loading