-
Notifications
You must be signed in to change notification settings - Fork 56
fix(backend): retire branch-agnostic field edges on object deletion (… #10312
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
base: retire-agnostic-edges-ifc-2843
Are you sure you want to change the base?
Changes from 1 commit
ae43c2e
43cabda
6a86225
446b432
e6fa96a
0e27d7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| """The one predicate that decides whether a branch-agnostic field is still retained by any branch. | ||
|
|
||
| A branch-agnostic field keeps its value on edges carrying the global branch name, which every branch | ||
| reads. Those edges may only be closed once **NO** branch can still reach a live owner over a live field | ||
| edge. Cypher for this is consolidated here to ensure consistency. | ||
|
|
||
| Retention is decided per branch **and** per linked vertex: under that branch's view, the vertex's | ||
| existence edge and its edge to the field must both resolve to `active`. Surviving edges are summed | ||
| per branch, and only then is the maximum taken across branches. Retention is a disjunction of what | ||
| each branch holds live on its own, never a pool the branches contribute to jointly. | ||
|
|
||
| A `:Relationship` needs two qualifying field edges rather than one, because a relationship missing a | ||
| peer is not a relationship. A `:Relationship` must also have two distinct active peers to be | ||
| considered active. | ||
|
|
||
| Assumption: every branch forks from the default branch. A branch-of-branch feature would not extend | ||
| this logic. | ||
| """ | ||
|
|
||
| # Expects `field` in scope, one row per candidate vertex, plus the `$global_branch_name` and `$at` | ||
| # parameters. Emits the candidates no branch retains, with `field` as the only variable in scope. | ||
| UNRETAINED_AGNOSTIC_FIELD_PREDICATE = """ | ||
| WITH collect(field) AS candidates | ||
|
|
||
| // ---------------------- | ||
| // The branches are read once for the whole run and carried as a list. | ||
| // ---------------------- | ||
| MATCH (branch:Branch) | ||
| WHERE branch.name <> $global_branch_name | ||
| WITH | ||
| candidates, | ||
| collect({ | ||
| name: branch.name, | ||
| origin_name: CASE WHEN branch.is_default THEN NULL ELSE branch.origin_branch END, | ||
| origin_at: CASE | ||
| WHEN branch.is_default THEN NULL | ||
| WHEN branch.branched_from < $at THEN branch.branched_from | ||
|
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 non-isolated branch exists, this window treats its origin snapshot as retained even though non-isolated reads use the current default branch. After the default branch deletes an object, the branch can no longer read that owner, but this keeps its agnostic fields open; include Prompt for AI agents
Contributor
Author
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. non-isolated branches do not exist anymore. the references to them are dead code |
||
| ELSE $at | ||
| END | ||
| }) AS branch_windows | ||
|
|
||
| UNWIND candidates AS field | ||
| WITH field, branch_windows, CASE WHEN field:Relationship THEN 2 ELSE 1 END AS required_live_peers | ||
|
|
||
| CALL (field, branch_windows) { | ||
| UNWIND branch_windows AS branch_window | ||
| WITH | ||
| field, | ||
| branch_window.name AS branch_name, | ||
| branch_window.origin_name AS origin_name, | ||
| branch_window.origin_at AS origin_at | ||
|
|
||
| // ---------------------- | ||
| // Count this `field`'s active links to :Node vertices on this branch | ||
| // ---------------------- | ||
| MATCH (node:Node)-[field_edge:HAS_ATTRIBUTE|IS_RELATED]-(field) | ||
| WHERE (field_edge.branch IN [$global_branch_name, branch_name] | ||
| AND field_edge.from <= $at | ||
| AND (field_edge.to IS NULL OR field_edge.to > $at)) | ||
| OR (field_edge.branch = origin_name | ||
| AND field_edge.from <= origin_at | ||
| AND (field_edge.to IS NULL OR field_edge.to > origin_at)) | ||
| WITH | ||
| branch_name, | ||
| origin_name, | ||
| origin_at, | ||
| node, | ||
| field_edge.branch_level AS field_edge_level, | ||
| field_edge.from AS field_edge_from, | ||
| field_edge.status AS field_edge_status | ||
| ORDER BY field_edge_level DESC, field_edge_from DESC, field_edge_status ASC | ||
| WITH | ||
| branch_name, | ||
| origin_name, | ||
| origin_at, | ||
| node, | ||
| collect(field_edge_status)[0] AS winning_field_edge_status | ||
| WHERE winning_field_edge_status = "active" | ||
|
|
||
| // ---------------------- | ||
| // Check that each linked :Node vertex is active on this branch. | ||
| // The global branch stays in the existence match so that an owner which is itself branch-agnostic | ||
| // reads as live on every branch and is therefore retained. | ||
| // ---------------------- | ||
| MATCH (node)-[existence:IS_PART_OF]->(:Root) | ||
| WHERE (existence.branch IN [$global_branch_name, branch_name] | ||
| AND existence.from <= $at | ||
| AND (existence.to IS NULL OR existence.to > $at)) | ||
| OR (existence.branch = origin_name | ||
| AND existence.from <= origin_at | ||
| AND (existence.to IS NULL OR existence.to > origin_at)) | ||
| WITH | ||
| branch_name, | ||
| node, | ||
| existence.branch_level AS existence_level, | ||
| existence.from AS existence_from, | ||
| existence.status AS existence_status | ||
| ORDER BY existence_level DESC, existence_from DESC, existence_status ASC | ||
| WITH branch_name, node, collect(existence_status)[0] AS winning_existence | ||
| WHERE winning_existence = "active" | ||
|
|
||
| // ---------------------- | ||
| // Peers are counted by uuid. Kind/inheritance migration leaves multiple Node vertices with | ||
| // the same uuid for a single entity | ||
| // ---------------------- | ||
| WITH branch_name, count(DISTINCT node.uuid) AS live_peer_count | ||
|
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: For a self-referencing relationship (a node related to itself) both Prompt for AI agents
Contributor
Author
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. we don't support an object referencing itself in a relationship |
||
| RETURN max(live_peer_count) AS most_live_peers | ||
| } | ||
|
|
||
| WITH field, required_live_peers, coalesce(most_live_peers, 0) AS live_peers | ||
| WHERE live_peers < required_live_peers | ||
| """ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from infrahub.core.constants import GLOBAL_BRANCH_NAME | ||
| from infrahub.core.query import Query, QueryType | ||
| from infrahub.core.query.agnostic_retention import UNRETAINED_AGNOSTIC_FIELD_PREDICATE | ||
|
|
||
| if TYPE_CHECKING: | ||
| from infrahub.core.timestamp import Timestamp | ||
| from infrahub.database import InfrahubDatabase | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class NodeAgnosticRetirementResult: | ||
| """What retiring one node's branch-agnostic fields changed.""" | ||
|
|
||
| edges_closed: int | ||
| """Global edges given a `to` timestamp. Zero means every field is still retained somewhere.""" | ||
|
|
||
|
|
||
| _RETIRE_UNRETAINED_FIELDS_OF_NODE = """ | ||
| // ----------------- | ||
| // MATCH on the branch-agnostic edges we care about to start with. | ||
| // ----------------- | ||
| MATCH (anchor_node:Node {uuid: $node_uuid})-[anchor:HAS_ATTRIBUTE|IS_RELATED]-(field:Attribute|Relationship) | ||
| WHERE anchor.branch = $global_branch_name | ||
| AND anchor.status = "active" | ||
| AND anchor.to IS NULL | ||
| WITH DISTINCT field | ||
| %(unretained_predicate)s | ||
|
|
||
| MATCH (field)-[e]-() | ||
| WHERE e.branch = $global_branch_name | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| AND e.status = "active" | ||
| AND e.to IS NULL | ||
| SET e.to = $at | ||
| RETURN count(e) AS edges_closed | ||
| """ % {"unretained_predicate": UNRETAINED_AGNOSTIC_FIELD_PREDICATE} | ||
|
|
||
|
|
||
| class RetireNodeAgnosticFieldsQuery(Query): | ||
| """Close the open global edges of one node's branch-agnostic fields that no branch retains. | ||
|
|
||
| Checks if the field is reachable from ANY branch. It is only deleted if it is completely | ||
| unreachable. | ||
| """ | ||
|
|
||
| name: str = "retire_node_agnostic_fields" | ||
| type: QueryType = QueryType.WRITE | ||
|
|
||
| insert_return: bool = False | ||
| insert_limit: bool = False | ||
|
|
||
| def __init__(self, node_uuid: str, at: Timestamp, **kwargs: Any) -> None: | ||
| self.node_uuid = node_uuid | ||
| super().__init__(at=at, **kwargs) | ||
|
|
||
| async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: # noqa: ARG002 | ||
| self.params["global_branch_name"] = GLOBAL_BRANCH_NAME | ||
| self.params["node_uuid"] = self.node_uuid | ||
| self.params["at"] = self.at.to_string() | ||
|
|
||
| self.add_to_query(_RETIRE_UNRETAINED_FIELDS_OF_NODE) | ||
| self.update_return_labels(["edges_closed"]) | ||
|
|
||
| def get_data(self) -> NodeAgnosticRetirementResult: | ||
| """Return what the run closed.""" | ||
| result = self.get_result() | ||
| if result: | ||
| return NodeAgnosticRetirementResult(edges_closed=result.get_as_type("edges_closed", int)) | ||
| return NodeAgnosticRetirementResult(edges_closed=0) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: When
Node.delete()receives a session-backeddb, a retirement failure leaves the earlier delete tombstones committed.git/tasks.pyalready calls this path fromstart_session(), so run the whole deletion in an explicit transaction or enforce that requirement before propagating this exception.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
run_check_merge_conflicts()ingit/tasks.pyto use a transaction instead of a session. this is a pre-existing bug b/cNodeManager.delete()will run multipleNode.delete()calls in sequence, so it is possible for one to fail partway through, leaving the group semi-deleted. the move to use a transaction should fix this