greater rollback coverage of merge logic#9116
Conversation
| db=db, | ||
| log=log, | ||
| obj=obj, | ||
| branch=obj, |
|
|
||
| branch.branched_from = pre_merge_branched_from | ||
| branch.status = BranchStatus.OPEN | ||
| await branch.save(db=db) |
There was a problem hiding this comment.
Just highlighting here that we don't set a user_id here, could be that we don't have it in this context, and it could technically be true that it was the system user who did this
| async with lock.registry.global_graph_lock(): | ||
| # Set to MERGING to lock the branch while merge proceeds | ||
| branch.status = BranchStatus.MERGING | ||
| await branch.save(db=db) |
There was a problem hiding this comment.
Here we would have the user from the context and could inject it to .save().
| await obj.save(db=db) | ||
| registry.branch[obj.name] = obj | ||
| branch.status = BranchStatus.MERGED | ||
| await branch.save(db=db) |
There was a problem hiding this comment.
Same as above as we have the user we might as well add it to branch.save here.
| ) | ||
|
|
||
| if config.SETTINGS.main.delete_branch_after_merge and not obj.is_default: | ||
| if config.SETTINGS.main.delete_branch_after_merge and not branch.is_default: |
There was a problem hiding this comment.
Not related to this PR, just highlighting that here we need a global config object to determine how the code should behave. I think we should consider to transition away from that. Basically when we initialize Infrahub we'd create a config object and instead tie it to the app and then we pass along the active config to other classes and functions. I think it could make various tests easier if we didn't have to go and modify a shared global object in various places.
There was a problem hiding this comment.
yes I agree. ideally, _do_merge_branch would be encapsulated in a component into which we can inject dependencies and settings, but I'm going to save that for a different PR
* fix(branch): add MERGING to BranchStatus enum to match server (#9293) The server-side BranchStatus enum gained MERGING in Infrahub 1.9.3 (PR opsmill/infrahub#9116) but the SDK enum was not updated. Any branch returned with status="MERGING" — including a branch stranded in that state by a task worker dying mid-merge — caused client.branch.all() to raise ValidationError, which crashloops the task worker on every sync_remote_repositories run. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(tests): use model_validate so ty accepts the parametrized status string ty rejects passing str where BranchStatus is annotated; switch to BranchData.model_validate({...}) which mirrors how the SDK actually parses server JSON in branch.py. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * review(branch): address PR feedback — drop test, rename changelog, shorten note - Drop test_branch_data_accepts_all_server_statuses: per review, the enum-coverage test belongs in the infrahub community repo where it can iterate over the actual server-side BranchStatus enum instead of a hardcoded list of strings. It will be added there as part of the PR that bumps the SDK commit pin. - Rename changelog +branchstatus-merging.fixed.md → 1037.fixed.md to link it to the newly-filed SDK-side issue #1037 (migrated from opsmill/infrahub#9293). - Shorten changelog note per review. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * chore(changelog): shorten 1037.fixed.md per review Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Phillip Simonds <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Why
DiffMerger.merge_graph()advances the source branch'sbranched_fromtomerge_at - 1µson success. When a downstream step in the wider merge flow then failed, the advance was never undone — making the next merge attempt's diff window exclude the branch's pre-merge_atchanges. The retry would then "succeed" while silently merging nothing, and any schema migrations would fail with confusing errors (Unable to find the attribute …). Surfaced via manual testing.Goal: any unexpected failure between the start of
_do_merge_branchand theMERGEDstatus save should leave the graph and registry as they were before the merge attempt. The branch should never be left in a half-merged state silently looking likeOPEN.Non-goals:
tasks.py:rebase_branch(same shape, same gap, deferred).Closes #9115
What changed
Behavioral changes
BranchStatus.MERGING. A branch is set toMERGINGfor the duration of_do_merge_branch; transitions toMERGEDon full success, back toOPENif rollback succeeds, or stays atMERGINGif rollback itself fails (signals "needs human intervention"). Mutations, rebases, re-merges, and proposed-change creation are blocked against aMERGINGbranch with the same semantics asMERGED.load_schema_from_db,calculate_migrations, andcoordinator.execute()are all wrapped.branched_fromto its pre-merge value, so a retry sees the same diff window the first attempt did.SchemaUpdateCoordinator.execute()acceptsmanage_rollback: bool = True. The merge call site passesFalseso the outer wrapper is the sole rollback authority. All other call sites are unchanged.Implementation notes
DiffMergeRollbackQueryandRollbackQueryare consolidated into a singleRollbackQuery. It now accepts an optionalnode_uuidslist to restoreupdated_at/updated_bymetadata for Node, Attribute, and Relationship vertices usingprevious_updated_at/by. It always resetsto_user_idalong withto, and always deletes orphan vertices. All write subqueries useCALL { ... } IN TRANSACTIONS._rollback_mergeintasks.pyruns three steps:merger.rollback(), registry restore, then a singlebranch.save()that flips status back toOPENand restoresbranched_from. Each step is independentlytry/except-wrapped; the helper does not raise. If rollback fails, the branch is left atMERGING. Ideally this would be reorganized to be more SOLID, but that would be a bigger change and this fix is for a specific bug._do_merge_branchcapturespre_merge_branched_from = branch.branched_frombefore the merge starts and threads it through to_rollback_merge. The advance itself remains insideDiffMerger.merge_graph. Moving it to the success path of_do_merge_branchwould arguably be cleaner —branched_fromandstatus=MERGEDare paired "this merge committed" facts — but that's a wider refactor (other direct callers ofmerge_graphwould have to change). Capture + restore on the rollback path is the smaller, more targeted fix for the bug at hand.What stayed the same
/schema/loadis unchanged (manage_rollbackdefaults toTrue).tasks.pyis unchanged.BranchMerger.merge()and its inner rollback are unchanged./schema/load, CLI init) keepsIN TRANSACTIONSbatching; behavior is identical except thatto_user_idis now reset alongsideto(a strictly more-correct fix).Suggested review order
backend/infrahub/core/query/rollback.py— the consolidated query.backend/infrahub/core/branch/tasks.py—_rollback_mergeand the restructured_do_merge_branch.backend/infrahub/core/schema/update_coordinator.py—manage_rollbackplumbing.backend/infrahub/core/branch/enums.pyand the audit changes (status_checker.py,permissions/report.py, GraphQL mutations).How to review
Focus areas
_rollback_merge: DB rollback first, then registry restore, then the finalbranch.save()that resetsstatusandbranched_fromtogether.branched_fromadvance inDiffMerger.merge_graph(versus moving it to the success path of_do_merge_branch). Both options are correct; the move would tie the advance to the success path more directly, at the cost of a wider refactor ofmerge_graph's direct callers. Worth a second opinion on whether the larger change is preferable.manage_rollback=Falsepath inSchemaUpdateCoordinator.execute()confirm it propagates the original exception (orMigrationErrorfromerror_msgs) without invoking_handle_failure_and_rollback.BaseException(notException) catch in_do_merge_branchso cancellation also triggers cleanup.How to test
I did manual testing using a local Infrahub instance. I inserted an error into the merge branch workflow; verified all changes were rolled back and branch was in the correct state; removed the error; ran the merge again; verified that it succeeded and merged branch was updated correctly. The retry-after-rollback scenario surfaced the
branched_frombug noted in Why above; the fix is covered by an extension totest_merge_branch_rollbackthat asserts bothstatusandbranched_fromare restored after a failed merge.Manual: trigger a merge through the UI on a branch with schema changes, confirm the success path is unchanged. To exercise rollback, the existing
BrokenBranchMergerpattern intest_merge_rollback.pyinjects aValueErroraftermerge_graphreturns; the branch returns toOPENandmaindata is restored.Impact & rollout
BranchStatusenum gainsMERGING. Any consumer that exhaustively switches onBranchStatus(clients, dashboards, reports) needs a case for it.is_terminaldoes not includeMERGING. The newmanage_rollbackparameter onSchemaUpdateCoordinator.execute()is keyword-only with a default that preserves existing behavior, so all current call sites are unaffected.Branch.save()at the start of every merge (for theMERGINGwrite) and one at the end of the rollback path. Negligible. The consolidatedRollbackQuerydoes the same DB work as the two queries it replaces, plus orphan vertex cleanup that the merge-rollback path was missing.OPENcontinue to behave as before; the newMERGINGstate only appears for newly initiated merges.Checklist
changelog/9115.fixed.md)