infrahub recover merge CLI tool#9942
Conversation
github-actions
Bot
added
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. Adds a new infrahub recover merge CLI to reverse partial graph merges and reset statuses. Updates recovery procedures to require human review, though the implementation is sound.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 3/5
client.branch.all()is currently blocked byBranchDatavalidation rejectingMERGE_FAILED, so SDK users can hit runtime validation errors and be unable to see or recover affected branches during merge-recovery workflows — updateBranchDatato acceptMERGE_FAILEDand add a regression test for listing recoverable branches before merging.
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="changelog/+merge-recover-command.added.md">
<violation number="1" location="changelog/+merge-recover-command.added.md:1">
P1: SDK consumers cannot list branches while a merge is awaiting recovery: `BranchData` rejects `MERGE_FAILED`, so `client.branch.all()` raises validation instead of returning the recoverable branch (opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:11-25; opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:182). Add `MERGE_FAILED` to the SDK enum in a coordinated SDK change.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| @@ -0,0 +1 @@ | |||
| Added the `infrahub recover merge` CLI command to recover from a failed branch merge. It rolls back the partial merge on the default branch, resets the branch and any associated proposed change to `OPEN`, and lifts the write protection so the default branch is writable again. The command is operator-confirmed (skip the prompt with `--yes`) and idempotent: a run with nothing to recover reports so and makes no changes. While a branch is in the failed state, all mutations against it, including its deletion, are refused until recovery has run. | |||
There was a problem hiding this comment.
P1: SDK consumers cannot list branches while a merge is awaiting recovery: BranchData rejects MERGE_FAILED, so client.branch.all() raises validation instead of returning the recoverable branch (opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:11-25; opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:182). Add MERGE_FAILED to the SDK enum in a coordinated SDK change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At changelog/+merge-recover-command.added.md, line 1:
<comment>SDK consumers cannot list branches while a merge is awaiting recovery: `BranchData` rejects `MERGE_FAILED`, so `client.branch.all()` raises validation instead of returning the recoverable branch (opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:11-25; opsmill/infrahub-sdk-python/infrahub_sdk/branch.py:182). Add `MERGE_FAILED` to the SDK enum in a coordinated SDK change.</comment>
<file context>
@@ -0,0 +1 @@
+Added the `infrahub recover merge` CLI command to recover from a failed branch merge. It rolls back the partial merge on the default branch, resets the branch and any associated proposed change to `OPEN`, and lifts the write protection so the default branch is writable again. The command is operator-confirmed (skip the prompt with `--yes`) and idempotent: a run with nothing to recover reports so and makes no changes. While a branch is in the failed state, all mutations against it, including its deletion, are refused until recovery has run.
</file context>
There was a problem hiding this comment.
sdk commit has been bumped to include the MERGE_FAILED branch status
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
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
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
185a588 to
925139f
Compare
polmichel
left a comment
polmichel
left a comment
There was a problem hiding this comment.
LGTM! The test suite coverage is really solid and the feature is super clean. This looks like a great improvement that will save the team a meaningful amount of time.
A comment about the SDK commit update I've already done in another PR.
Optional comments about fixtures encapsulation and a docstring.
There was a problem hiding this comment.
Since I missed this PR on Monday, I've already updated the SDK commit in another PR: #9981. That PR points to a more recent commit, so I think the best way forward is to drop the change this comment refers to.
| def should_mark_as_failed_merge( | ||
| self, | ||
| *, | ||
| status: BranchStatus, | ||
| lock_holder_worker_id: str | None, | ||
| active_worker_ids: set[str], | ||
| merge_started_at: Timestamp | None, | ||
| now: Timestamp, | ||
| ) -> bool: | ||
| """Decide whether a branch represents a dead merge that must be flagged ``MERGE_FAILED``. | ||
| async def should_mark_as_failed_merge(self, *, branch: Branch, now: Timestamp) -> bool: | ||
| """Decide whether a branch stuck in ``MERGING`` is a dead merge whose worker died mid-flight. |
There was a problem hiding this comment.
Nice change, this cleaner to just pass the branch as a parameter
| """Clear a stale write-protection cache key when no merge still owns it. | ||
|
|
||
| Reached when no branch needs recovering. A cache key naming a branch that is still ``MERGING`` | ||
| is left in place — that is a healthy in-progress merge, or an ambiguous absent-lock merge that | ||
| was not auto-recovered. A key is stale and safe to drop when its branch was removed out-of-band, | ||
| when it is malformed with no identifiable branch, or when the branch is already ``OPEN`` — the | ||
| latter happens when a prior recovery reopened the branch but the key delete then failed, so a | ||
| re-run finishes the cleanup instead of leaving writes blocked until the watcher reconciles. | ||
| Transient cache errors are allowed to propagate so the key is never dropped on an | ||
| unreadable-but-present value. | ||
| """ |
There was a problem hiding this comment.
The docstring is quite long, and mentions expectations about the caller's context when it calls the method.
Could it be trimmed, or is this information required in your opinion?
For instance it could be
The key is left in place while a merge still owns it and dropped once none does. A cache read error propagates rather than dropping a key whose value could not be read.
| class _FailAtBranchResetRecoverer(MergeFailureRecoverer): | ||
| """Real recoverer that raises while resetting the branch, after the graph rollback has run. | ||
|
|
||
| Reproduces a recovery interrupted before the branch is reopened and the protection lifted, so the | ||
| branch stays flagged and protected for a re-run. | ||
| """ | ||
|
|
||
| async def _reset_branch(self, branch: Branch) -> None: | ||
| raise RuntimeError("branch reset failed") | ||
|
|
||
|
|
||
| class _FailAtLockReleaseRecoverer(MergeFailureRecoverer): | ||
| """Real recoverer whose merge-lock release fails, before the branch is reopened. | ||
|
|
||
| Reproduces a cache backend that cannot drop the stale lock key. The failure must not be swallowed: | ||
| a held lock blocks every future merge, so recovery must report failure and leave the branch flagged, | ||
| the protection held and the lock in place for a re-run. | ||
| """ |
| class _FailAtBranchResetRecoverer(MergeFailureRecoverer): | ||
| """Real recoverer that raises while resetting the branch, after the graph rollback has run. | ||
|
|
||
| Reproduces a recovery interrupted after the rollback lands but before the branch is reopened, so | ||
| the branch stays flagged and protected for a re-run. | ||
| """ | ||
|
|
||
| async def _reset_branch(self, branch: Branch) -> None: | ||
| raise RuntimeError("branch reset failed") | ||
|
|
||
|
|
||
| class _MidMergeFailingDiffMerger(DiffMerger): | ||
| """A real DiffMerger that commits the earlier bulk edges then raises before finishing. | ||
|
|
||
| Reproduces a worker dying partway through the graph write phase: the earlier bulk queries have | ||
| already committed on the default branch when a later one fails, leaving partially merged data with | ||
| no in-process rollback for recovery to clean up. | ||
| """ |
There was a problem hiding this comment.
Can all these fakes being encapsulated inside a conftest.py file at the package level? It would expose the "fake component library" in an unique place and prevent duplication.
Or at least _FailAtBranchResetRecoverer which is the only one duplicated today?
| def _build_identifier( | ||
| self, db: InfrahubDatabase, cache: MemoryCache, component: InfrahubComponent, default_branch: Branch | ||
| ) -> MergeFailureIdentifier: | ||
| return MergeFailureIdentifier( | ||
| db=db, | ||
| cache=cache, | ||
| component=component, | ||
| merge_write_blocker=MergeWriteBlocker(cache=cache), | ||
| default_branch=default_branch, | ||
| grace_period_seconds=GRACE_PERIOD_SECONDS, | ||
| ) | ||
|
|
||
| def _build_recovery( | ||
| self, db: InfrahubDatabase, cache: MemoryCache, component: InfrahubComponent, default_branch: Branch | ||
| ) -> MergeFailureRecoverer: | ||
| return MergeFailureRecoverer( | ||
| db=db, | ||
| merge_write_blocker=MergeWriteBlocker(cache=cache), | ||
| identifier=self._build_identifier(db=db, cache=cache, component=component, default_branch=default_branch), | ||
| default_branch=default_branch, | ||
| cache=cache, | ||
| ) |
There was a problem hiding this comment.
Same comment, still an optional one: could this be encapsulated inside a common conftest.py file?
update dependencies, tests, and signature so the method can be used from outside of this class
The @pytest.mark.skip decorator was commented out, leaving `import pytest` unused and failing python-lint (ruff F401). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
925139f to
78927b4
Compare
2 participants
Why
The detection PR flags a merge whose worker died mid-flight as
MERGE_FAILEDand holds the write block, and the rollback-core PR (#9878) gave us an idempotent, state-free range rollback keyed on(default branch, merge_started_at). But there was still no way to actually use any of it: a failed merge left the default branch write-blocked and the branch stuck, with no operator-facing path back to a writable state.Goal: the operator-facing recovery entry point — an
infrahub recover mergeCLI backed by aMergeFailureRecoverer— that reverses the partial merge, resets the branch and any associated proposed change toOPEN, releases the stale merge lock, and lifts the write protection so the default branch is writable again. Idempotent and operator-confirmed.Why it's safe: recovery reuses the range rollback from #9878, which is safe precisely because the write block owned every default-branch write in the merge window. The recoverer only orders the surrounding state transitions (proposed change, branch status, merge lock, write protection) so that any mid-sequence failure leaves the branch flagged and protected for a clean re-run.
Non-goals:
MERGE_FAILED— prior detection PR.This closes IFC-2566 (reset the app to its pre-merge state, operator-facing).
Closes IFC-2566
What changed
Behavioral changes (what you'll observe differently):
infrahub recover merge [branch]. With no argument it auto-detects the failed merge; given a branch name it targets that branch. It previews first and asks for confirmation (--yes/-yto skip). By default it only recovers a merge whose worker is confirmed dead;--force/-falso recovers a merge stuck with an absent/ambiguous lock. Exits non-zero when recovery fails.MERGE_FAILEDcan no longer be deleted until it has been recovered —BranchDeleteraisesMergeRecoveryRequiredErrorrather than deleting a half-merged branch.Internals:
MergeFailureRecovererexposespreview()(read-only: reportsRECOVERABLE/NOTHING_TO_RECOVER, makes no changes) andrecover(). Outcomes:NOTHING_TO_RECOVER,RECOVERABLE,RECOVERED,ORPHANED_CLEARED,FAILED.FAILEDand leaves the branch flagged + protected for a re-run: rollback → reset proposed change → release merge lock → reset branch toOPEN→ lift write protection. The proposed change is reset before the branch, and the write protection lifted last, so a partial run never reopens the branch or unblocks writes prematurely. The merge lock is released while protection is still up (so this unconditional delete can only hit the dead worker's lock, never a fresh merge's), and a release failure surfaces asFAILEDrather than being swallowed — a held lock would otherwise block every future merge silently.recover()clears an orphaned write-protection key whose branch was removed out-of-band, is malformed, or is alreadyOPEN(ORPHANED_CLEARED), but leaves a key naming a healthy in-progressMERGINGbranch in place. A transient/unreadable cache value is never treated as absent.MergeFailureIdentifierreshaped so its dead-merge decision is reusable by recovery:should_mark_as_failed_merge(branch, now)andmerge_lock_holder()are now async instance methods that read the cache themselves, anddefault_branchis constructor-injected.RecoveryOutcome/RecoveryReportmoved out of the identifier into the recoverer.SchemaManager(internal + core models) and attaches it to the db, so the proposed-change lookup during recovery resolves its schema without depending on — or mutating — the global registry or any user-defined schema.What stayed the same: the healthy-merge happy path; the range rollback semantics from #9878; no database migration, schema, or API-contract change.
Suggested review order
backend/infrahub/core/merge/failure_recoverer.py—preview/recover, the recovery step ordering and failure semantics, the merge-lock release, and orphaned/stale-key clearing.backend/infrahub/cli/recover.py— the CLI: preview→confirm→recover flow, exit codes, registry-independent schema wiring.backend/infrahub/core/merge/failure_identifier.py— the asyncshould_mark_as_failed_merge/merge_lock_holderrefactor and the moved report types.backend/infrahub/graphql/mutations/branch.py— theMERGE_FAILEDdelete guard.test_recovery.py(outcomes, force gating, stale-key clearing, delete gate, merge-lock release + retry),test_recovery_rollback.py(end-to-end rollback through recovery),test_failure_identifier.py(replacestest_failed_merge_predicate.py), andintegration_docker/test_merge_kill_recovery.py(kill a merge mid-flight, then recover).Impact & rollout
MERGE_FAILEDbranches. No schema, API, enum, or config change; no migration.Checklist
changelog/+merge-recover-command.added.md--help)Summary by cubic
Adds a new
infrahub recover mergeCLI to roll back partial default-branch writes, reset the failed branch and any linked proposed change to OPEN, and restore write access by clearing protections and any stale merge lock. MERGE_FAILED branches are read-only and undeletable until recovery runs, closing IFC-2566.New Features
infrahub recover merge [branch]: previews, asks for confirmation (--yesto skip), and is idempotent.--forcecan recover when the lock state is ambiguous.MergeRecoveryRequiredErrorwithMERGE_RECOVERY_REQUIRED_MESSAGE.Bug Fixes
branched_fromafter merge.Written for commit 78927b4. Summary will update on new commits.