fix(backend): delete branch data in bounded batches

[ajtmccarty]

Why

Deleting a large branch can fail with Neo.TransientError.General.MemoryPoolOutOfMemoryError. Because the delete commits its writes in batches while the surrounding query does not, the failure leaves the branch stranded in DELETING — filtered out of the branch list, so invisible to the user — with most of its data still in the graph, taking up space with no way to reclaim it.

Goal: make a branch delete complete regardless of how large the branch is, by bounding how much transaction memory it needs — and clean up the branches previous failures already stranded.

Non-goals: no change to what a branch delete removes.

Closes #9889

What changed

Behavioral changes:

• A branch delete no longer exhausts transaction memory on large branches. During a local testing, deleting a branch of 1,074,217 edges completed at a 512 MiB transaction limit, where the previous implementation ran out of memory.
• It is also faster: 27.8s vs 87.9s on that branch at the default limit.
• A new graph migration (075_finish_deleting_branches) finishes the job for branches an earlier failure abandoned. Upgrading finds every branch left in DELETING, runs it through the new deleter, and removes the branch node — reclaiming space that was otherwise unreachable, since such a branch cannot be looked up or deleted again through any normal path.
• Branch.delete() now raises NotImplementedError pointing at BranchDeleter. It is kept as an override rather than removed, because the inherited StandardNode.delete would drop the Branch vertex and silently orphan every edge and vertex on the branch — a worse failure than the one being fixed.
• A delete that failed part way through can now be retried. Both the BranchDelete mutation and the branch-delete flow load the branch with ignore_deleting=False, so the DELETING status the first attempt leaves behind no longer hides it from the lookup. Previously the retry failed with BranchNotFoundError and the migration was the only way out — and it runs once, on upgrade, so any later failure stranded the branch for good. Two consequences to be aware of: the mutation will now accept a delete for a branch whose delete is still running (every query involved is idempotent, but nothing serialises the two runs), and a retry re-emits BranchDeletedEvent. Making the surrounding flow steps idempotent is not addressed here.

Implementation notes:

The single mega-query became a set of bounded queries sequenced from Python by a new BranchDeleter component:

1. DeleteBranchAgnosticRelationshipsQuery / DeleteBranchAgnosticAttributesQuery — the agnostic peers of nodes that exist on no other branch.
2. DeleteBranchEdgesQuery — one batch of edges of a single relationship type, re-run until no edges of that type remain, for each member of DatabaseEdgeType.

Three things drove the design:

• Peak transaction memory is now a function of batch size, not branch size. The old query's dominant allocation was WITH DISTINCT elementId(s), elementId(d) followed by collect(s_id) + collect(d_id): an eager DISTINCT over pairs (so a node was stored once per edge it had), materialized as element-id strings, then two collects plus a concatenated copy held at once. The existing CALL {} IN TRANSACTIONS only bounded the inner writes — everything upstream lived in one outer transaction, which is why the earlier "use smaller transactions" fix didn't help. The replacement has no eager aggregation beyond a DISTINCT bounded by the batch.
• Naming the relationship type lets the branch range index serve the match. MATCH (s)-[r]->(d) WHERE r.branch = $branch_name can use none of the per-type indexes in core/graph/index.py, so it scanned every edge in the database. One query per type turns that into an index seek, and because deleted entries leave the index, each batch is O(batch) rather than a re-scan from the start.
• Each batch deletes the vertices its own edge deletions left bare, so no vertex bookkeeping is needed anywhere. This is sound because every branch edge is removed by the batch's DELETE and both endpoints are re-examined afterwards — a vertex is examined once per edge it had, so the batch that removes its last edge is the one that sees it at degree zero. The DISTINCT is load-bearing: it forces the batch's edge deletes to complete before the first vertex is examined. It must not become a DETACH DELETE, which would remove branch edges that then never reach a batch of their own, leaving the vertices on their far side unexamined and orphaned.

A fourth point applies to the agnostic cleanup specifically. Its batches count Nodes, and one Node can drag an unbounded number of peer vertices into the transaction with it, unlike the edge batches where one row is one edge. So it takes min(batch_size, 500) rather than the configured batch size: 500 is what this phase used before, and it is a ceiling rather than a fixed size so that an operator who lowers the configured limit to fit a constrained database is not handed a larger batch here than they asked for.

The migration reuses the delete path rather than reimplementing it: it loads each stranded branch with Branch.get_by_name(..., ignore_deleting=False) — the escape hatch that makes these branches reachable at all — and hands it to BranchDeleter.delete(), so it exercises exactly the code a normal delete does. Finding them needs its own query because the shared branch list query filters DELETING out unconditionally, so asking it for those branches returns nothing.

What stayed the same:

• No node-schema changes, no GraphQL or API changes. The only migration is the graph migration above; it adds and alters nothing, it only finishes deletes.
• The set of vertices removed is unchanged — 416,021 on the benchmark branch, identical to before, with pre-existing orphans left untouched.
• Phase 1 (agnostic cleanup) keeps its existing Cypher and must still run first: both queries find their candidate nodes through the branch's IS_PART_OF edges, which phase 2 destroys. That ordering used to be implicit in one query text and is now spread across Python control flow, so it is stated in the docstrings — and it is what makes an interrupted delete safe to re-run.
• Migration032 still works, via BranchDeleter.delete_branch_data(), which takes a branch name rather than a Branch because the branches it cleans up have no Branch vertex left.

Alternatives considered:

• Accumulating touched vertex ids client-side and sweeping at the end. Rejected: millions of ids held in Python.
• Marking candidates in the database with a label. Rejected: it writes to the store and the token index where there was only a read, and a crashed run leaves labels behind to poison the next delete.
• A final label-scoped orphan sweep. Implemented and measured, then removed once DETACH DELETE was dropped: it cost a full 9M-vertex scan per delete and also deleted pre-existing orphans, a scope change the current version doesn't make.
• Wrapping the per-type query in CALL {} IN TRANSACTIONS instead of a Python LIMIT loop. Not taken: it would stream a scan over the same index its committed batches are mutating, while deleting vertices mid-scan. The LIMIT loop gives each batch a fresh snapshot for the price of one round trip per batch.

One deliberate gap: IS_RESERVED is the only member of DatabaseEdgeType without a branch range index, so its pass is a type scan rather than a seek. In practice it is only ever written on the global branch, so a branch-scoped delete never matches it. Adding the index is a DB index change and was left out of this PR.

Local performance testing results

Results — branch of 1,074,217 edges in a 24M-edge database:

	before	after
4.11 GiB memory limit	87.9s	27.8s
512 MiB memory limit	OOM at 28.8s, branch stranded	31.3s, OK
vertices removed	416,021	416,021
new orphaned vertices	0	0
edges on main	unchanged	unchanged

Impact & rollout

• Backward compatibility: Branch.delete() now raises. The only production caller was the branch-delete flow, and every caller in this repo is updated; any out-of-tree caller (Infrahub Enterprise, scripts) must switch to BranchDeleter.
• Performance: 3.2× faster on the benchmark branch, and peak transaction memory no longer scales with branch size. Cost is one extra round trip per batch, negligible against the work each batch does.
• Config/env changes: none. Batch size comes from the existing database.query_size_limit setting, which the agnostic-cleanup phase caps at 500 for the reason above.
• Deployment notes: GRAPH_VERSION goes to 75, so this requires the usual infrahub db migrate on upgrade. The migration is a no-op on any database with no branch in DELETING; where there is one, it deletes that branch's data, so expect it to run for as long as a branch delete would — it reports each branch as it starts and the edge count when it finishes, since infrahub db migrate suppresses the deleter's own logging and silence would otherwise look like a hang. A branch that cannot be deleted is named in the migration's errors and does not stop the others. Safe to re-run: an interrupted delete simply resumes.

Checklist

• Tests added/updated
• Changelog entry added (uv run towncrier create ...)
• External docs updated (if user-facing or ops-facing change) — n/a, no user-facing change beyond the fix itself
• Internal .md docs updated (internal knowledge and AI code tools knowledge)
• I have reviewed AI generated content

[codspeed-hq]

Merging this PR will not alter performance

✅ 13 untouched benchmarks

---

_{Comparing ajtm-08042026-branch-delete-update (3fe3c3a) with stable (dccce1d)¹}

Footnotes

1. No successful run was found on stable (7a224bf) during the generation of this report, so dccce1d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[cubic-dev-ai]

5 issues found across 12 files

Confidence score: 2/5

• In backend/infrahub/core/branch/deleter.py, a failed deletion batch can leave a branch stuck in DELETING and invisible to retry selection, so cleanup may never resume and branches can remain in a broken limbo state — make the retry path explicitly re-queue/resume DELETING branches after partial failure.
• In backend/infrahub/core/query/branch.py, the agnostic cleanup transaction cap is applied per node, not per deleted relationship/attribute, so high-degree nodes can still blow past the intended transaction bound and cause unstable or expensive delete runs — batch directly on matched relationships/attributes instead of node count.
• In backend/infrahub/core/query/branch.py, the branch-deletion predicate can treat a historical active IS_PART_OF edge as proof that an agnostic peer is still referenced, which can leak peers that should be removed and leave stale graph data — tighten the predicate to evaluate branch/time-correct active references only.
• In backend/infrahub/core/query/branch.py, the _delete_edges loop relies on nullable relationships_deleted stats for termination, so missing counters can prematurely stop or mis-handle progress reporting in batched deletes — harden the stop condition against null stats and add a regression test around zero/null counter batches.

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="backend/infrahub/core/branch/models.py">

<violation number="1" location="backend/infrahub/core/branch/models.py:326">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

The changed behavior of `Branch.delete` (now unconditionally raising `NotImplementedError`) is not covered by tests, and a regression-style assertion would be practical. Consider adding a test that verifies `Branch.delete` raises `NotImplementedError` and, if callers previously relied on the `ValidationError` for default/global branches, confirm those callers were migrated to `BranchDeleter`.</violation>
</file>

<file name="backend/infrahub/core/branch/deleter.py">

<violation number="1" location="backend/infrahub/core/branch/deleter.py:47">
P1: A failed batch still leaves the branch undiscoverable to the deletion workflow because this status is saved before cleanup and the retry reload filters `DELETING` branches. The retry path should explicitly resume `DELETING` branches (or otherwise make cleanup idempotently reachable) so the failure mode described in the commit does not strand branches again.</violation>
</file>

<file name="backend/infrahub/core/query/branch.py">

<violation number="1" location="backend/infrahub/core/query/branch.py:47">
P2: A high-degree node can still exceed the intended transaction bound during agnostic cleanup: `IN TRANSACTIONS OF batch_size ROWS` limits nodes, not the relationships or attributes deleted per node. Batch the matched `rel`/`attr` vertices themselves so peer fan-out is bounded too.</violation>

<violation number="2" location="backend/infrahub/core/query/branch.py:77">
P2: Branch deletion can leak agnostic peers when the node has an active historical `IS_PART_OF` edge plus a later deleted edge on another branch, because this predicate treats the historical active edge as proof that the node still exists there. Resolve the latest edge per branch/time and require `status = "active"` with `to IS NULL` before preserving the peer.</violation>

<violation number="3" location="backend/infrahub/core/query/branch.py:140">
P2: The batched delete loop in `BranchDeleter._delete_edges` uses `deleted_edge_count()` as its only stop condition, and that method sums the nullable `query.stats.get_counter("relationships_deleted")`. If a batch's stats payload ever lacks `relationships-deleted`, `QueryStat.relationships_deleted` stays `None` and `get_counter` raises a `TypeError` mid-delete, which would abort branch deletion rather than finishing the remaining edges. Consider guarding against a `None` counter (e.g. treating it as 0) so the termination signal is always an integer, keeping the batching loop robust.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: A failed batch still leaves the branch undiscoverable to the deletion workflow because this status is saved before cleanup and the retry reload filters DELETING branches. The retry path should explicitly resume DELETING branches (or otherwise make cleanup idempotently reachable) so the failure mode described in the commit does not strand branches again.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/branch/deleter.py, line 47:

<comment>A failed batch still leaves the branch undiscoverable to the deletion workflow because this status is saved before cleanup and the retry reload filters `DELETING` branches. The retry path should explicitly resume `DELETING` branches (or otherwise make cleanup idempotently reachable) so the failure mode described in the commit does not strand branches again.</comment>

<file context>
@@ -0,0 +1,100 @@
+        if branch.is_global:
+            raise ValidationError(f"Unable to delete {branch.name} this is an internal branch.")
+
+        branch.status = BranchStatus.DELETING
+        await branch.save(db=self.db)
+
</file context>

[ajtmccarty]

Both lookups now pass ignore_deleting=False — the flow (core/branch/tasks.py) and the BranchDelete mutation (graphql/mutations/branch.py). Without the mutation change a user still could not retry, only an automated re-submission of the workflow.

Test added: test_branch_delete_retries_a_branch_left_deleting. It fails without the change with exactly BranchNotFoundError: Branch: stuck-deleting-branch not found.

Two consequences we accepted deliberately and recorded in the commit message: the mutation will now accept a delete for a branch whose delete is still running (each query is idempotent, but nothing serializes the two runs), and a retry re-emits BranchDeletedEvent. Making the surrounding flow steps idempotent is left for separate work.

[cubic-dev-ai]

P2: Branch deletion can leak agnostic peers when the node has an active historical IS_PART_OF edge plus a later deleted edge on another branch, because this predicate treats the historical active edge as proof that the node still exists there. Resolve the latest edge per branch/time and require status = "active" with to IS NULL before preserving the peer.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/query/branch.py, line 77:

<comment>Branch deletion can leak agnostic peers when the node has an active historical `IS_PART_OF` edge plus a later deleted edge on another branch, because this predicate treats the historical active edge as proof that the node still exists there. Resolve the latest edge per branch/time and require `status = "active"` with `to IS NULL` before preserving the peer.</comment>

<file context>
@@ -10,83 +10,137 @@
+WHERE e.branch = $branch_name
+AND NOT EXISTS {
+    MATCH (n)-[ipo:IS_PART_OF {status: "active"}]->(:Root)
+    WHERE ipo.branch <> $branch_name
+}
+CALL (n) {
</file context>

[ajtmccarty]

The observation is accurate — the predicate does accept status: "active" without checking to IS NULL — but I don't think the conclusion holds, and the suggested fix would lose data. Not changing it.

Two things first: this predicate is carried over character for character from before this PR (only the surrounding OPTIONAL MATCH … LIMIT 1 / RETURN ipo IS NOT NULL was rewritten as NOT EXISTS { }), so nothing here is new behaviour.

Second, NodeDeleteQuery (core/query/node.py:644-651) only closes the active edge when that edge is on the branch doing the deleting:

CREATE (n)-[delete_edge:IS_PART_OF { branch: $branch, status: "deleted", from: $at }]->(root)
WITH r
WHERE r.branch = $branch      // ← only then
SET r.to = $at

So reaching the case you describe takes: create n on branch X → merge X to main (n gains an active main edge) → delete n on main (same-branch, so main's active edge gets to set) → delete branch X.

At that point n is not garbage. It keeps its main edges, so it is never orphaned, and main's history still contains it — a query against main at a time before the deletion must render n, which needs those agnostic attributes to still exist. Requiring to IS NULL would delete the agnostic attributes of every node that was ever deleted on another branch and break historical (--at) reads of it.

The other reachable cases already behave correctly: a node created and deleted only on X has no other-branch active edge, so its peers are cleaned up; a node created on main and deleted on X never gets an active edge on X, so the outer MATCH does not select it at all.

[cubic-dev-ai]

P2: A high-degree node can still exceed the intended transaction bound during agnostic cleanup: IN TRANSACTIONS OF batch_size ROWS limits nodes, not the relationships or attributes deleted per node. Batch the matched rel/attr vertices themselves so peer fan-out is bounded too.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/query/branch.py, line 47:

<comment>A high-degree node can still exceed the intended transaction bound during agnostic cleanup: `IN TRANSACTIONS OF batch_size ROWS` limits nodes, not the relationships or attributes deleted per node. Batch the matched `rel`/`attr` vertices themselves so peer fan-out is bounded too.</comment>

<file context>
@@ -10,83 +10,137 @@
-    OPTIONAL MATCH (n)-[:HAS_ATTRIBUTE {branch: $global_branch_name}]-(attr:Attribute)
-    DETACH DELETE attr
-} IN TRANSACTIONS OF 500 ROWS
+} IN TRANSACTIONS OF %(batch_size)s ROWS
+        """ % {"batch_size": self.batch_size}
+        self.params["branch_name"] = self.branch_name
</file context>

[ajtmccarty]

Valid. Fixed in 7175415.

This PR had changed that batch from a hardcoded 500 to query_size_limit (5000), so the transactions were 10× larger than before, in a change whose whole purpose is bounding transaction memory. That was incidental rather than deliberate: query_size_limit was simply the value already in hand.

Reverted to 500 via a named AGNOSTIC_PEER_BATCH_SIZE constant, with the distinction recorded where it is easy to miss — phase 1 batches Nodes with unbounded peer fan-out, phase 2 batches edges where the work per row is fixed, so they must not share a number.

[cubic-dev-ai]

P2: The batched delete loop in BranchDeleter._delete_edges uses deleted_edge_count() as its only stop condition, and that method sums the nullable query.stats.get_counter("relationships_deleted"). If a batch's stats payload ever lacks relationships-deleted, QueryStat.relationships_deleted stays None and get_counter raises a TypeError mid-delete, which would abort branch deletion rather than finishing the remaining edges. Consider guarding against a None counter (e.g. treating it as 0) so the termination signal is always an integer, keeping the batching loop robust.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/query/branch.py, line 140:

<comment>The batched delete loop in `BranchDeleter._delete_edges` uses `deleted_edge_count()` as its only stop condition, and that method sums the nullable `query.stats.get_counter("relationships_deleted")`. If a batch's stats payload ever lacks `relationships-deleted`, `QueryStat.relationships_deleted` stays `None` and `get_counter` raises a `TypeError` mid-delete, which would abort branch deletion rather than finishing the remaining edges. Consider guarding against a `None` counter (e.g. treating it as 0) so the termination signal is always an integer, keeping the batching loop robust.</comment>

<file context>
@@ -10,83 +10,137 @@
+        self.params["batch_size"] = self.batch_size
+        self.add_to_query(query)
+
+    def deleted_edge_count(self) -> int:
+        return self.stats.get_counter("relationships_deleted")
+
</file context>

[ajtmccarty]

Not taking this one — the path is unreachable for this query, tested rather than reasoned about.

A batch that deletes nothing produces no stats entry at all, so get_counter sums an empty list and returns 0:

stats entries: []
deleted_edge_count(): 0

That is DeleteBranchEdgesQuery run against a branch name matching nothing, i.e. exactly the terminating batch of every edge type. Query.execute only appends a QueryStat when "stats" in metadata, and Neo4j omits the key entirely when a query makes no updates.

For relationships_deleted to be None you would need a stats payload that is present but missing relationships-deleted. This query cannot produce that: if any row matched, DELETE r removed at least one relationship, so the counter is present; if no row matched there are no updates and no payload. There is no third state.

Guarding QueryStats.get_counter would be a change to shared code (three other call sites) for a path this query cannot reach, so it seems better as its own hardening change if we want it

[cubic-dev-ai]

P2: Custom agent: Flag AI Slop and Fabricated Changes

The changed behavior of Branch.delete (now unconditionally raising NotImplementedError) is not covered by tests, and a regression-style assertion would be practical. Consider adding a test that verifies Branch.delete raises NotImplementedError and, if callers previously relied on the ValidationError for default/global branches, confirm those callers were migrated to BranchDeleter.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/branch/models.py, line 326:

<comment>The changed behavior of `Branch.delete` (now unconditionally raising `NotImplementedError`) is not covered by tests, and a regression-style assertion would be practical. Consider adding a test that verifies `Branch.delete` raises `NotImplementedError` and, if callers previously relied on the `ValidationError` for default/global branches, confirm those callers were migrated to `BranchDeleter`.</comment>

<file context>
@@ -315,17 +314,16 @@ async def create(self, db: InfrahubDatabase, user_id: str = SYSTEM_USER_ID) -> b
+            NotImplementedError: Always.
+
+        """
+        raise NotImplementedError("Unable to delete a Branch directly, use BranchDeleter instead.")
 
     def get_query_filter_relationships(
</file context>

[ajtmccarty]

Fair — both halves addressed in 7175415, in backend/tests/component/core/test_branch.py.

test_branch_delete_method_is_refused asserts the NotImplementedError with an anchored match, and additionally that the branch is left untouched (still resolvable, still OPEN) rather than half-deleted.

On your second point: the ValidationError guards for the default and global branches were not dropped, they moved onto BranchDeleter.delete(). test_branch_deleter_refuses_default_and_global_branches covers both and asserts the default branch is unaltered, since those guards fire before any write.

Context on why Branch.delete raises rather than being deleted outright: Branch inherits StandardNode.delete, so removing the override would make branch.delete(db=db) silently drop the Branch vertex and orphan every edge and vertex on the branch — a quieter and worse failure than the one this PR fixes.

[cubic-dev-ai]

2 issues found across 9 files (changes from recent commits).

Confidence score: 2/5

• In backend/infrahub/graphql/mutations/branch.py (BranchDelete), concurrent requests can both proceed when a branch is already DELETING, which risks duplicate cleanup, duplicate post-delete events, and repeated Git operations—add a branch-scoped lock or atomic state transition so only one delete workflow can run.
• In backend/tests/component/graphql/mutations/test_branch.py, the retry test only checks ok: True while workflow.execute_workflow is mocked, so it can miss whether a stranded DELETING branch is actually reclaimed—extend the test to assert state recovery/idempotent cleanup behavior, not just mutation success.

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="backend/infrahub/graphql/mutations/branch.py">

<violation number="1" location="backend/infrahub/graphql/mutations/branch.py:152">
P1: Concurrent `BranchDelete` requests can now both run deletion workflows for a branch already in `DELETING`, so branch cleanup and post-delete events/Git cleanup may be processed twice. A branch-scoped lock or atomic active-versus-stranded deletion claim would allow retries without admitting an active duplicate.</violation>
</file>

<file name="backend/tests/component/graphql/mutations/test_branch.py">

<violation number="1" location="backend/tests/component/graphql/mutations/test_branch.py:573">
P3: The retry test mocks `workflow.execute_workflow` and only asserts the mutation returns `ok: True`, so it never verifies the stranded DELETING branch is actually reclaimed. It validates the mutation's `ignore_deleting=False` resolution (which would fail this test if regressed), but a regression in the real retry path (`delete_branch` → `BranchDeleter.delete()` on a DELETING branch) would pass silently. Consider asserting the branch remains resolvable with `ignore_deleting=False`/is removed after retry, or adding a flow-level test that exercises the actual deletion rather than mocking it.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: Concurrent BranchDelete requests can now both run deletion workflows for a branch already in DELETING, so branch cleanup and post-delete events/Git cleanup may be processed twice. A branch-scoped lock or atomic active-versus-stranded deletion claim would allow retries without admitting an active duplicate.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/graphql/mutations/branch.py, line 152:

<comment>Concurrent `BranchDelete` requests can now both run deletion workflows for a branch already in `DELETING`, so branch cleanup and post-delete events/Git cleanup may be processed twice. A branch-scoped lock or atomic active-versus-stranded deletion claim would allow retries without admitting an active duplicate.</comment>

<file context>
@@ -148,7 +148,8 @@ async def mutate(
         graphql_context: GraphqlContext = info.context
-        obj = await Branch.get_by_name(db=graphql_context.db, name=str(data.name))
+        # ignore_deleting=False so a delete that failed part way through can be retried: the first
+        obj = await Branch.get_by_name(db=graphql_context.db, name=str(data.name), ignore_deleting=False)
         await apply_external_context(graphql_context=graphql_context, context_input=context)
 
</file context>

[ajtmccarty]

Accurate, but this is a consequence we accepted deliberately rather than an oversight — it is called out in the commit message for 7175415f5 and in the PR description, both of which note that the mutation will now accept a delete for a branch whose delete is still running, that nothing serialises the two runs, and that a retry re-emits BranchDeletedEvent.

The alternative was leaving a branch stranded permanently: before this change the DELETING status hid the branch from the mutation's own lookup, so a failed delete could not be retried at all, and the upgrade migration — which runs once — was the only route back. That was judged the worse failure.

[cubic-dev-ai]

P3: The retry test mocks workflow.execute_workflow and only asserts the mutation returns ok: True, so it never verifies the stranded DELETING branch is actually reclaimed. It validates the mutation's ignore_deleting=False resolution (which would fail this test if regressed), but a regression in the real retry path (delete_branch → BranchDeleter.delete() on a DELETING branch) would pass silently. Consider asserting the branch remains resolvable with ignore_deleting=False/is removed after retry, or adding a flow-level test that exercises the actual deletion rather than mocking it.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/component/graphql/mutations/test_branch.py, line 573:

<comment>The retry test mocks `workflow.execute_workflow` and only asserts the mutation returns `ok: True`, so it never verifies the stranded DELETING branch is actually reclaimed. It validates the mutation's `ignore_deleting=False` resolution (which would fail this test if regressed), but a regression in the real retry path (`delete_branch` → `BranchDeleter.delete()` on a DELETING branch) would pass silently. Consider asserting the branch remains resolvable with `ignore_deleting=False`/is removed after retry, or adding a flow-level test that exercises the actual deletion rather than mocking it.</comment>

<file context>
@@ -554,6 +554,36 @@ async def test_branch_delete_own_branch_succeeds(
+    branch.status = BranchStatus.DELETING
+    await branch.save(db=db)
+
+    with patch.object(local_services.workflow, "execute_workflow", new=AsyncMock(return_value=None)):
+        delete_result = await graphql_mutation(
+            query='mutation { BranchDelete(data: { name: "stuck-deleting-branch" }) { ok } }',
</file context>

[ajtmccarty]

Fair, and left as is — with the gap acknowledged rather than papered over.

What the test does guard is the thing the change actually altered: the mutation's branch lookup. It fails without ignore_deleting=False, with BranchNotFoundError: Branch: stuck-deleting-branch not found., and that failure occurs at the lookup, before any of the permission or workflow logic is reached.

What it does not cover, as you say, is delete_branch → BranchDeleter.delete() against a branch already in DELETING. That path is exercised indirectly by the migration tests, which delete a DELETING branch through BranchDeleter and assert the edge counts and branch node afterwards — but not through the flow. A flow-level test would be better and is not in this PR.

Note the mock here is the file's existing convention for branch-delete mutation tests (test_branch_delete_own_branch_succeeds does the same), not a new choice. The repo's testing rules discourage unittest.mock, so a flow-level test would want a proper workflow adapter rather than another patch.object.

[cubic-dev-ai]

0 issues found across 4 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.

Re-trigger cubic</sub>

[ogenstad]

I added a small comment regarding a ruff ignore line that would be good to add.

I'm not so fond around the requirement to have a NotImplemented exception for Branch.delete(), it seems like a Liskov violation for StandardNode.delete(). I'm not sure we need to deal with it now but it might be confusing moving forward if we start to see more types of StandardNode objects. We can go with this for now and later evaluate when we have other types. I'm also not sure if any special handling would be required for other types. It might be that we should remove StandardNode.delete() and have something else for other nodes as well, or if we can have some common API.

[ogenstad]

Here I think it could be good with an inline noqa. For reference check #10002 and how other migrations were changed. Otherwise we'll run into problems later. Ideally we should know all of the errors that the migrations can raise and catch those instead of a bare Exception but I think that's something that will come later.

[ajtmccarty]

ruff actually won't let me
RUF100 [*] Unused `noqa` directive (non-enabled: `BLE001`)

[cubic-dev-ai]

All reported issues were addressed across 5 files (changes from recent commits).

<sub>Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

1 issue found across 14 files (changes from recent commits).

Confidence score: 2/5

• In backend/infrahub/core/branch/delete_coordinator.py, the delete race handling can let a losing concurrent delete assume post-delete work is already done, so proposed-change cancellation and BranchDeletedEvent may be skipped entirely if the winner crashes at the wrong point. That can leave orphaned proposed changes and downstream consumers unaware of deletion—make post-delete steps idempotent and guaranteed to run/retry after branch removal (e.g., persisted completion marker or retryable outbox-driven follow-up).

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="backend/infrahub/core/branch/delete_coordinator.py">

<violation number="1" location="backend/infrahub/core/branch/delete_coordinator.py:64">
P1: A concurrent losing delete can permanently skip proposed-change cancellation and `BranchDeletedEvent`: if the winner exits after removing the branch but before post-delete work, the loser treats that work as complete even though no durable completion is recorded. Running these idempotent post-delete actions for every attempt, or recording them in a durable retryable job, would avoid this gap.

(Based on your team's feedback about retryable DELETING branch deletes.) .</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: A concurrent losing delete can permanently skip proposed-change cancellation and BranchDeletedEvent: if the winner exits after removing the branch but before post-delete work, the loser treats that work as complete even though no durable completion is recorded. Running these idempotent post-delete actions for every attempt, or recording them in a durable retryable job, would avoid this gap.

(Based on your team's feedback about retryable DELETING branch deletes.) .

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/branch/delete_coordinator.py, line 64:

<comment>A concurrent losing delete can permanently skip proposed-change cancellation and `BranchDeletedEvent`: if the winner exits after removing the branch but before post-delete work, the loser treats that work as complete even though no durable completion is recorded. Running these idempotent post-delete actions for every attempt, or recording them in a durable retryable job, would avoid this gap.

(Based on your team's feedback about retryable DELETING branch deletes.) .</comment>

<file context>
@@ -0,0 +1,87 @@
+
+        result = await self.data_deleter.delete(branch=branch)
+
+        if result.branch_deleted:
+            await self.workflow.submit_workflow(
+                workflow=BRANCH_CANCEL_PROPOSED_CHANGES, context=context, parameters={"branch_name": branch.name}
</file context>

[ajtmccarty]

this is very unlikely. there would have to be 2 branch-delete processes running at the same time and then the winning one would need to die before reaching submit_workflow() or send(), which are the next 2 calls

[cubic-dev-ai]

0 issues found across 2 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.

Re-trigger cubic</sub>