Detect failed merges and hold write protection

[ajtmccarty]

Why

When a task worker is killed mid-merge (e.g. SIGKILL), the source branch is left stuck in MERGING with a partially-merged graph and the global merge lock still held by the dead worker. Nothing detected this state, so the default-branch write-protection either lingered indefinitely or — worse — writes could resume against a half-merged default branch, and users got no signal that anything was wrong.

Goal: deterministically detect a dead merge, record it durably as MERGE_FAILED, and keep the default branch write-blocked with a distinct, non-retryable error that tells the operator recovery is required — while never mis-flagging a healthy in-progress merge.

Non-goals (follow-ups):

• The recovery itself — infrahub recover / range rollback / branch + proposed-change reset (US3, separate PR).
• Structured detection/recovery logging and richer visibility (US4).

Part of the merge-failure-recovery epic IFC-2559; this is the US2 (detection) slice of umbrella bug IFC-2437 and advances IFC-2565 (surface a crashed-merge error to the operator). It does not close IFC-2437 — recovery lands in a follow-up.

Closes IFC-2565

What changed

Behavioral changes (what you'll observe differently):

• New durable branch status MERGE_FAILED, auto-exposed in the GraphQL BranchStatus enum. It is set only by detection and cleared only by recovery (not yet in this PR).
• A recurring MERGE_WATCHER workflow (1-minute cron, single-flighted across workers) plus a best-effort check at API-server startup flip a branch that is stuck in MERGING whose merge-lock holder is no longer a live worker (after a grace period) to MERGE_FAILED, and hold the shared write-protection.
• Writes blocked by a failed merge now return the distinct MERGE_RECOVERY_REQUIRED GraphQL error code (HTTP 423) with a message naming infrahub recover and "contact an administrator" — separate from the transient MERGE_IN_PROGRESS used during a healthy merge. Payload: {branch_name, merging_branch}.
• A healthy in-progress merge (live lock holder, or younger than the grace period) is never flagged.
• New config setting INFRAHUB_MERGE_FAILURE_GRACE_PERIOD_SECONDS (default 180).

Implementation notes:

• Liveness reuses the existing merge-lock token ({timestamp}::{worker_id}) compared against component.list_workers()' active set — the same pattern as clean_up_deadlocks. The decision is a side-effect-free predicate (is_failed_merge) that is unit-tested without a DB; the grace period is injected at construction.
• MergeFailureIdentifier.scan() runs detection then reconciliation in one pass. Reconciliation re-aligns the volatile merge:protected cache key with the durable MERGING/MERGE_FAILED branch status, so protection self-heals after a restart or cache flush.
• MergeRecoveryRequiredError is deliberately a sibling of MergeInProgressError, not a subclass: the two states demand opposite reactions (retry vs. run recovery), and subclassing would let any handler written for the retryable error silently absorb the durable one.
• Lock-token parsing now lives in lock.py beside the token format (a get_worker_id_from_lock_token helper), and a multi-status filter (BranchListFilters.statuses → n.status IN …) lets reconcile query protected branches in one round-trip.
• The python_sdk submodule is bumped so SDK clients can deserialize a MERGE_FAILED branch without a validation error.

What stayed the same: no database migration; the healthy-merge happy path is unchanged; the recovery flow is out of scope.

Suggested review order

1. 945f7a18c + 421f3828b — the MergeRecoveryRequiredError exception and regenerated error-catalogue bindings/tests for MERGE_RECOVERY_REQUIRED.
2. cbff3a97c — add the MERGE_FAILED status (+ GraphQL schema regen).
3. 426833218 — detection primitives (grace config, lock-token parser, merge-lock read, multi-status filter).
4. 4ce36eba2 — the detector, watcher cron, startup scan, and the write gate (the meat + tests).
5. 2db7d9548 — docs + spec alignment.
6. The remaining commits are generated-file/CI syncs for the new config setting, the python_sdk submodule bump, hardening of the SIGKILL integration test, and post-review refactors (rename to MergeFailureIdentifier/failure_identifier.py, grace-period constructor injection, docstring corrections).

How to review

• Focus: backend/infrahub/core/merge/failure_identifier.py (the predicate + scan), and backend/infrahub/branch/status_checker.py (the MERGE_FAILED gate raising MergeRecoveryRequiredError).
• Worth extra scrutiny: the failure predicate's conditions (why an absent lock is deliberately not auto-flagged), and the sibling-not-subclass choice for MergeRecoveryRequiredError.
• End-to-end coverage: backend/tests/integration_docker/test_merge_kill_recovery.py SIGKILLs a task worker mid-merge against a real docker stack (with a ~1s grace period in the test compose file) and asserts the idle scan flips the branch to MERGE_FAILED. The write-gate contract test lives in backend/tests/integration/merge/test_failed_merge_protection.py.
• Mechanical / generated (skim for drift only): schema/schema.graphql, schema/error-catalogue.json, schema/openapi.json, frontend/app/src/shared/api/**/generated, frontend/app/src/shared/api/rest/types.generated.ts, docs/docs/reference/error-catalogue.mdx, docs/docs/reference/configuration.mdx, regenerated docker-compose env vars.

Impact & rollout

• Backward compatibility: additive only — MERGE_FAILED is a new BranchStatus enum value and MERGE_RECOVERY_REQUIRED a new catalogue code (marked evolving). Clients that exhaustively switch on either enum should handle the new value. No migration.
• ⚠️ Operational caveat: recovery (infrahub recover) is not in this PR. Until the recovery PR lands, a branch that reaches MERGE_FAILED stays write-blocked with no in-product way to clear it (manual DB intervention only). Land the recovery follow-up close behind this.
• Performance: the write gate is unchanged (one cache GET per top-level mutation). Detection adds one parameterized branch query + one cache read per worker per minute.
• Config/env changes: new INFRAHUB_MERGE_FAILURE_GRACE_PERIOD_SECONDS (default 180).
• Deployment notes: safe to deploy; the MERGE_WATCHER cron deployment is created automatically once the workflow definition is present.

Checklist

• Tests added/updated
• Changelog entry — intentionally omitted; to be folded into the larger merge-failure-recovery docs update once recovery lands
• External docs updated (generated error-catalogue reference reflects MERGE_RECOVERY_REQUIRED; configuration reference reflects the new grace-period setting)
• Internal .md docs updated (dev/knowledge/backend/branch-status.md, dev/specs/ifc-2437-merge-failure-recovery/*)
• I have reviewed AI generated content

---

Summary by cubic

Detects and guards against failed merges to keep the default branch consistent. Adds MERGE_FAILED with a distinct MERGE_RECOVERY_REQUIRED (HTTP 423) response, runs a 1‑min single‑flight MERGE_WATCHER plus an API‑server startup scan, and exposes a grace‑period setting (IFC-2437).

• New Features

  • Detection: flip branches stuck in MERGING to MERGE_FAILED when the merge‑lock holder is dead and past grace; never flag a healthy merge. Hold shared write protection and reconcile the merge:protected key with durable status so it self‑heals after restarts or cache flushes. Runs via MERGE_WATCHER (every minute, single‑flight) and on API‑server startup.
  • Write gates: raise MergeInProgressError for MERGING and MergeRecoveryRequiredError (code MERGE_RECOVERY_REQUIRED, HTTP 423) for MERGE_FAILED with payload {branch_name, merging_branch}; applies to the source and default branches. Cache‑down fallback enforces from DB and prioritizes MERGE_FAILED.
  • Runtime/contracts: new BranchStatus.MERGE_FAILED (GraphQL/SDK); server‑side multi‑status filter (statuses) for safe DB fallback; lock‑token worker‑id parser and read‑only merge‑lock holder lookup. Error catalogue/formatter updated; frontend error bindings and GraphQL/OpenAPI/REST types regenerated; SDK submodule bumped. Tests include unit, component, functional, and a skipped-by-default SIGKILL integration.

• Migration

  • No breaking changes. Clients may see MERGE_FAILED and MERGE_RECOVERY_REQUIRED.
  • New setting merge_failure_grace_period_seconds (default 180), exposed as INFRAHUB_MERGE_FAILURE_GRACE_PERIOD_SECONDS.
  • Ensure workers run the internal MERGE_WATCHER workflow.

^{Written for commit f55ee89. Summary will update on new commits.}

[cubic-dev-ai]

3 issues found and verified against the latest diff

Confidence score: 3/5

• In schema/error-catalogue.json, the SDK BranchStatus enum missing MERGE_FAILED can leave clients unable to represent or handle a backend-reported merge state, which risks broken consumer logic when MERGE_RECOVERY_REQUIRED is returned — add MERGE_FAILED to the SDK enum before merging.
• In backend/infrahub/branch/status_checker.py, the cache-fallback path uses Branch.get_list without a status filter or explicit limit, so branches beyond the default page size may bypass merge gating and allow writes during active/failed merges — tighten the fallback query (status + pagination/limit handling) before merge.
• In backend/tests/functional/merge/test_failed_merge_protection.py, inconsistent assertions across the three error-response blocks make it easier for regressions in error contract fields to slip through unnoticed — align the assertions so each path verifies the same key fields (message, code, http_status, and data) for reliable protection.

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/tests/functional/merge/test_failed_merge_protection.py">

<violation number="1" location="backend/tests/functional/merge/test_failed_merge_protection.py:85">
P3: The three error-response assertion blocks in this test are inconsistent in what they verify. The default-branch gate checks `message`, `code` (`MERGE_RECOVERY_REQUIRED`), `http_status` (`423`), and `data`; the source-branch gate skips `http_status`; and the restart/post-recovery gate only checks `code`. A partial regression in `http_status`, `data`, or the error message after recovery would go undetected. Align all three gates to verify the same set of fields so the error contract is uniformly validated, especially after the `scan_for_failed_merges` recovery path.</violation>
</file>

<file name="schema/error-catalogue.json">

<violation number="1" location="schema/error-catalogue.json:304">
P1: SDK `BranchStatus` enum is missing `MERGE_FAILED`. The backend already has this value (`backend/infrahub/core/branch/enums.py:8`), and the new `MERGE_RECOVERY_REQUIRED` error code communicates this state to consumers. But the SDK's `BranchStatus` enum at `opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:5` is missing it, so any `branch.get()` or `branch.all()` call that encounters a branch with `status: "MERGE_FAILED"` will crash with a Pydantic validation error. Add `MERGE_FAILED = "MERGE_FAILED"` to the SDK's `BranchStatus` enum so SDK consumers can query and handle failed-merge branches.</violation>
</file>

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

<violation number="1" location="backend/infrahub/branch/status_checker.py:110">
P2: Cache-fallback merge gating can allow writes during an active/failed merge when branch count exceeds the default page size. The fallback query uses `Branch.get_list` without status filter or explicit limit, so it only inspects up to 1000 branches before deciding no block is needed.</violation>
</file>

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

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: SDK BranchStatus enum is missing MERGE_FAILED. The backend already has this value (backend/infrahub/core/branch/enums.py:8), and the new MERGE_RECOVERY_REQUIRED error code communicates this state to consumers. But the SDK's BranchStatus enum at opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:5 is missing it, so any branch.get() or branch.all() call that encounters a branch with status: "MERGE_FAILED" will crash with a Pydantic validation error. Add MERGE_FAILED = "MERGE_FAILED" to the SDK's BranchStatus enum so SDK consumers can query and handle failed-merge branches.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At schema/error-catalogue.json, line 304:

<comment>SDK `BranchStatus` enum is missing `MERGE_FAILED`. The backend already has this value (`backend/infrahub/core/branch/enums.py:8`), and the new `MERGE_RECOVERY_REQUIRED` error code communicates this state to consumers. But the SDK's `BranchStatus` enum at `opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:5` is missing it, so any `branch.get()` or `branch.all()` call that encounters a branch with `status: "MERGE_FAILED"` will crash with a Pydantic validation error. Add `MERGE_FAILED = "MERGE_FAILED"` to the SDK's `BranchStatus` enum so SDK consumers can query and handle failed-merge branches.</comment>

<file context>
@@ -238,6 +238,92 @@
+      }
+    },
+    "MERGE_RECOVERY_REQUIRED": {
+      "description": "The write was rejected because a previous branch merge failed and left the default branch protected. Recovery is required: an administrator must run `infrahub recover`. Unlike MERGE_IN_PROGRESS this is not retryable.",
+      "stability": "evolving",
+      "http_status": 423,
</file context>

[ajtmccarty]

added in the SDK and the submodule commit has been bumped to include it

[cubic-dev-ai]

2 issues found across 34 files

Confidence score: 4/5

• In docs/docs/reference/error-catalogue.mdx, the recovery guidance points operators to infrahub recover, which is not present here, so on-call responders could follow a non-existent command during an incident and lose time; update the catalogue to the actual supported recovery path before merging.
• In dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md, the predicate text still depends on merge_locker.get_merge_lock_holder_worker_id(cache) even though merge_locker is no longer injected, which leaves the contract internally inconsistent and can cause incorrect implementations; revise the spec to document the new source of lock-holder worker ID before merge.

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="schema/error-catalogue.json">

<violation number="1" location="schema/error-catalogue.json:304">
P1: SDK `BranchStatus` enum is missing `MERGE_FAILED`. The backend already has this value (`backend/infrahub/core/branch/enums.py:8`), and the new `MERGE_RECOVERY_REQUIRED` error code communicates this state to consumers. But the SDK's `BranchStatus` enum at `opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:5` is missing it, so any `branch.get()` or `branch.all()` call that encounters a branch with `status: "MERGE_FAILED"` will crash with a Pydantic validation error. Add `MERGE_FAILED = "MERGE_FAILED"` to the SDK's `BranchStatus` enum so SDK consumers can query and handle failed-merge branches.</violation>
</file>

<file name="docs/docs/reference/error-catalogue.mdx">

<violation number="1" location="docs/docs/reference/error-catalogue.mdx:289">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

The error catalogue claims that operators should run `infrahub recover`, but that CLI command does not exist in this PR. The PR description explicitly states that "No recovery path included in this PR; the recovery flow lands in a follow-up." Documentation that describes behavior not yet implemented is misleading and should be deferred until the recovery command actually exists, or at minimum should not prescribe a specific command that does not exist.</violation>
</file>

<file name="dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md">

<violation number="1" location="dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md:193">
P2: The constructor no longer injects `merge_locker`, but the predicate description still references `merge_locker.get_merge_lock_holder_worker_id(cache)` to read the lock-holder worker id. The spec needs to clarify how the lock-holder token is now parsed — either the reading/parsing logic is folded into `MergeWriteBlocker` (which currently owns the `merge:protected` key, not the merge-lock token) or is handled through a standalone helper. As written, an implementer following this spec has no way to get the lock-holder worker id.</violation>
</file>

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

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Custom agent: Flag AI Slop and Fabricated Changes

The error catalogue claims that operators should run infrahub recover, but that CLI command does not exist in this PR. The PR description explicitly states that "No recovery path included in this PR; the recovery flow lands in a follow-up." Documentation that describes behavior not yet implemented is misleading and should be deferred until the recovery command actually exists, or at minimum should not prescribe a specific command that does not exist.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/reference/error-catalogue.mdx, line 289:

<comment>The error catalogue claims that operators should run `infrahub recover`, but that CLI command does not exist in this PR. The PR description explicitly states that "No recovery path included in this PR; the recovery flow lands in a follow-up." Documentation that describes behavior not yet implemented is misleading and should be deferred until the recovery command actually exists, or at minimum should not prescribe a specific command that does not exist.</comment>

<file context>
@@ -284,6 +284,41 @@ The write was rejected because a branch merge is in progress. The block is trans
 
+### MERGE_RECOVERY_REQUIRED
+
+The write was rejected because a previous branch merge failed and left the default branch protected. Recovery is required: an administrator must run `infrahub recover`. Unlike MERGE_IN_PROGRESS this is not retryable.
+
+- **Stability**: `evolving`
</file context>

[ajtmccarty]

it will be added in this release

[cubic-dev-ai]

The parent comment still applies for this PR: the recovery command is still listed as a follow-up in the PR body, so the docs line is premature here.

[cubic-dev-ai]

P2: The constructor no longer injects merge_locker, but the predicate description still references merge_locker.get_merge_lock_holder_worker_id(cache) to read the lock-holder worker id. The spec needs to clarify how the lock-holder token is now parsed — either the reading/parsing logic is folded into MergeWriteBlocker (which currently owns the merge:protected key, not the merge-lock token) or is handled through a standalone helper. As written, an implementer following this spec has no way to get the lock-holder worker id.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md, line 193:

<comment>The constructor no longer injects `merge_locker`, but the predicate description still references `merge_locker.get_merge_lock_holder_worker_id(cache)` to read the lock-holder worker id. The spec needs to clarify how the lock-holder token is now parsed — either the reading/parsing logic is folded into `MergeWriteBlocker` (which currently owns the `merge:protected` key, not the merge-lock token) or is handled through a standalone helper. As written, an implementer following this spec has no way to get the lock-holder worker id.</comment>

<file context>
@@ -179,42 +179,48 @@ deleted/reopened; the `updated_at >= merge_at` filter excludes already-restored
-        merge_locker: MergeLocker,     # read-side helper: current merge-lock holder worker_id (or None)
+        cache: InfrahubCache,                    # read the merge-lock holder token
+        component: InfrahubComponent,            # list_workers() -> active-worker set for the liveness check
+        merge_write_blocker: MergeWriteBlocker,  # owns the merge:protected key (set/get/delete)
+        # recovery (PR-5) additionally injects: diff_merger: DiffMerger (the range rollback query)
     ) -> None: ...
</file context>

[ajtmccarty]

I'll clean the spec up for the next PR in the series

[cubic-dev-ai]

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

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

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

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

Re-trigger cubic</sub>

[cubic-dev-ai]

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

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

Re-trigger cubic</sub>

[codspeed-hq]

Merging this PR will not alter performance

✅ 12 untouched benchmarks

---

_{Comparing ifc-2437-merge-failure-recovery-detect-failed (f55ee89) with develop (e2e8744)}

[cubic-dev-ai]

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

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="schema/error-catalogue.json">

<violation number="1" location="schema/error-catalogue.json:304">
P1: SDK `BranchStatus` enum is missing `MERGE_FAILED`. The backend already has this value (`backend/infrahub/core/branch/enums.py:8`), and the new `MERGE_RECOVERY_REQUIRED` error code communicates this state to consumers. But the SDK's `BranchStatus` enum at `opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:5` is missing it, so any `branch.get()` or `branch.all()` call that encounters a branch with `status: "MERGE_FAILED"` will crash with a Pydantic validation error. Add `MERGE_FAILED = "MERGE_FAILED"` to the SDK's `BranchStatus` enum so SDK consumers can query and handle failed-merge branches.</violation>
</file>

<file name="docs/docs/reference/error-catalogue.mdx">

<violation number="1" location="docs/docs/reference/error-catalogue.mdx:289">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

The error catalogue claims that operators should run `infrahub recover`, but that CLI command does not exist in this PR. The PR description explicitly states that "No recovery path included in this PR; the recovery flow lands in a follow-up." Documentation that describes behavior not yet implemented is misleading and should be deferred until the recovery command actually exists, or at minimum should not prescribe a specific command that does not exist.</violation>
</file>

<file name="dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md">

<violation number="1" location="dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md:193">
P2: The constructor no longer injects `merge_locker`, but the predicate description still references `merge_locker.get_merge_lock_holder_worker_id(cache)` to read the lock-holder worker id. The spec needs to clarify how the lock-holder token is now parsed — either the reading/parsing logic is folded into `MergeWriteBlocker` (which currently owns the `merge:protected` key, not the merge-lock token) or is handled through a standalone helper. As written, an implementer following this spec has no way to get the lock-holder worker id.</violation>
</file>

<file name="backend/tests/integration_docker/test_merge_kill_recovery.py">

<violation number="1" location="backend/tests/integration_docker/test_merge_kill_recovery.py:60">
P2: The merge mutation fires as a background task via `asyncio.create_task`, but its potential exception is never collected. If the GraphQL call raises an error for any reason unrelated to the SIGKILL (API timeout, schema validation, transient network issue), Python will emit a "Task exception was never retrieved" warning at garbage-collection time and the root cause will be silently lost, potentially masking CI flakiness or real failures. Consider short-circuiting the exception retrieval in the `finally` block so the test makes the failure explicit rather than letting it go unobserved.</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]

0 issues found across 1 file (changes from recent commits).

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

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

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

Re-trigger cubic</sub>

[cubic-dev-ai]

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

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

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

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

Re-trigger cubic</sub>

[polmichel]

I am not sure to understand this comment. I think we just remove the constructor from MergeRecoveryRequiredError and if we make it inherit from MergeInProgressError, the unit test test_merge_recovery_required_resolves_to_distinct_code passes.
If a code exists inside the catalogue for the subtype, it returns it. If not, it looks for parent types.

[ajtmccarty]

yes I agree. this docstring has been updated. looks like you were reviewing older code

[polmichel]

Ah sorry, I missed that

[polmichel]

Is it worth to keep both of these filters? Does it make a difference in Cypher to use the IN for only asserting against one status?

[ajtmccarty]

I kept it for backwards compatibility. I didn't think it was worth it to search for and update uses of status vs statuses

[polmichel]

As this is a component isolated from the rest, this is not really important, but having a method named is_failed_merge which does not consider the case when a branch comes in with a status of MERGE_FAILED looks confusing at first glance.

Based on the current usage inside the code, a branch can only come in with MERGING status and this is the first condition of the method, so I guess it's fine.

I get that this is to determine if a MERGING branch should transition to MERGE_FAILED status but I think there should be a dedicated comment explaining why MERGE_FAILED comes out as false or maybe the method's name is not totally accurate.

Maybe I'm also missing something.

[ajtmccarty]

renamed to should_mark_as_failed_merge

[polmichel]

If the upper comment makes sense around is_failed_merge there would be an impact over here

[polmichel]

It looks like this fixture should be named differently since this is a merging branch that specifically started 10 minutes+ ago. In the test_within_grace_is_not_flagged, there is also a merging branch created.

[ajtmccarty]

updated to merging_branch_for_10m

[polmichel]

Nice test, it clearly illustrates the different steps of the workflow

[polmichel]

Just thinking out loud here, but shouldn't we keep such long tests from running in every pipeline? Maybe we should have a test suite that we run once per release, or something like that?

Based on my tests, this test takes approximately 90–100s, which impacts the integration_docker test suite. That's also the longest test suite in the current CI since the component tests were split out.

This gives a lof of confidence, but also costs a lot in my opinion.

[ajtmccarty]

yes this is a good point. I'll mark as skip for now and make a follow-up item to enable it for release testing only
https://opsmill.atlassian.net/browse/IFC-2878

[cubic-dev-ai]

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

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

Re-trigger cubic</sub>

[polmichel]

LGTM !