-
Notifications
You must be signed in to change notification settings - Fork 13
fix(tracking): reconcile tracking groups on runs that save no nodes #1278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
ogenstad
wants to merge
3
commits into
infrahub-develop
Choose a base branch
from
po-tracking-group-zero-member-reap
base: infrahub-develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+405
−64
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
e13b500
fix(tracking): reconcile tracking groups on runs that save no nodes
ogenstad 65eb4b0
test(tracking): cover the zero-member and refused-delete cases for th…
ogenstad 1dc8bad
fix(tracking): honour the tracked branch and survive transport failures
ogenstad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| 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. |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
|
@@ -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. | ||
|
|
@@ -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() | ||
|
|
||
| # 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When a member deletion is refused, this exception escapes Prompt for AI agents |
||
| # 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 | ||
|
|
@@ -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 | ||
|
|
||
|
|
@@ -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. | ||
|
|
@@ -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) | ||
|
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 | ||
|
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.