Skip to content

greater rollback coverage of merge logic#9116

Merged
ajtmccarty merged 10 commits into
stablefrom
ajtm-04302026-wider-merge-rollback
May 5, 2026
Merged

greater rollback coverage of merge logic#9116
ajtmccarty merged 10 commits into
stablefrom
ajtm-04302026-wider-merge-rollback

Conversation

@ajtmccarty

@ajtmccarty ajtmccarty commented May 1, 2026

Copy link
Copy Markdown
Contributor

Why

  1. It was possible for a merge operation to fail in such a way that the merge changes had already been applied and were not rolled back. This PR adds greater rollback coverage to the merge logic.
  2. Both SchemaUpdateCoordinator and DiffMerger had rollback logic, but they were slightly different. The rollback logic is now consolidated in a single query.
  3. DiffMerger.merge_graph() advances the source branch's branched_from to merge_at - 1µs on 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_at changes. 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_branch and the MERGED status 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 like OPEN.

Non-goals:

  • Does not change the rebase flow at tasks.py:rebase_branch (same shape, same gap, deferred).
  • Does not add any method for recovering from a failure during a rollback. This would probably require restoring from a backup or some sort of manual cypher intervention

Closes #9115

What changed

Behavioral changes

  • New BranchStatus.MERGING. A branch is set to MERGING for the duration of _do_merge_branch; transitions to MERGED on full success, back to OPEN if rollback succeeds, or stays at MERGING if rollback itself fails (signals "needs human intervention"). Mutations, rebases, re-merges, and proposed-change creation are blocked against a MERGING branch with the same semantics as MERGED.
  • A failed merge now restores the destination branch's graph and registry, regardless of exactly where the failure occurs. Failures in load_schema_from_db, calculate_migrations, and coordinator.execute() are all wrapped.
  • A failed merge now also restores the source branch's branched_from to its pre-merge value, so a retry sees the same diff window the first attempt did.
  • SchemaUpdateCoordinator.execute() accepts manage_rollback: bool = True. The merge call site passes False so the outer wrapper is the sole rollback authority. All other call sites are unchanged.

Implementation notes

  • DiffMergeRollbackQuery and RollbackQuery are consolidated into a single RollbackQuery. It now accepts an optional node_uuids list to restore updated_at/updated_by metadata for Node, Attribute, and Relationship vertices using previous_updated_at/by. It always resets to_user_id along with to, and always deletes orphan vertices. All write subqueries use CALL { ... } IN TRANSACTIONS.
  • New private helper _rollback_merge in tasks.py runs three steps: merger.rollback(), registry restore, then a single branch.save() that flips status back to OPEN and restores branched_from. Each step is independently try/except-wrapped; the helper does not raise. If rollback fails, the branch is left at MERGING. 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_branch captures pre_merge_branched_from = branch.branched_from before the merge starts and threads it through to _rollback_merge. The advance itself remains inside DiffMerger.merge_graph. Moving it to the success path of _do_merge_branch would arguably be cleaner — branched_from and status=MERGED are paired "this merge committed" facts — but that's a wider refactor (other direct callers of merge_graph would have to change). Capture + restore on the rollback path is the smaller, more targeted fix for the bug at hand.

What stayed the same

  • API contract for /schema/load is unchanged (manage_rollback defaults to True).
  • Rebase flow in tasks.py is unchanged.
  • BranchMerger.merge() and its inner rollback are unchanged.
  • Schema-only rollback path (/schema/load, CLI init) keeps IN TRANSACTIONS batching; behavior is identical except that to_user_id is now reset alongside to (a strictly more-correct fix).

Suggested review order

  1. backend/infrahub/core/query/rollback.py — the consolidated query.
  2. backend/infrahub/core/branch/tasks.py_rollback_merge and the restructured _do_merge_branch.
  3. backend/infrahub/core/schema/update_coordinator.pymanage_rollback plumbing.
  4. backend/infrahub/core/branch/enums.py and the audit changes (status_checker.py, permissions/report.py, GraphQL mutations).
  5. Tests last.

How to review

Focus areas

  • The order of operations in _rollback_merge: DB rollback first, then registry restore, then the final branch.save() that resets status and branched_from together.
  • The decision to keep the branched_from advance in DiffMerger.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 of merge_graph's direct callers. Worth a second opinion on whether the larger change is preferable.
  • The manage_rollback=False path in SchemaUpdateCoordinator.execute() confirm it propagates the original exception (or MigrationError from error_msgs) without invoking _handle_failure_and_rollback.
  • The BaseException (not Exception) catch in _do_merge_branch so 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_from bug noted in Why above; the fix is covered by an extension to test_merge_branch_rollback that asserts both status and branched_from are restored after a failed merge.

# Targeted component / integration tests for rollback query and merge flow:
uv run pytest -x backend/tests/component/core/diff/query/test_rollback.py
uv run pytest -x backend/tests/component/core/diff/test_diff_merger.py
uv run pytest -x backend/tests/component/core/diff/test_diff_and_merge.py
uv run pytest -x backend/tests/component/core/schema_manager/test_schema_rollback.py
uv run pytest -x backend/tests/integration/diff/test_merge_rollback.py

# Unit tests for the BranchStatus.MERGING audit:
uv run pytest backend/tests/unit/core/branch/test_merged_status.py

Manual: trigger a merge through the UI on a branch with schema changes, confirm the success path is unchanged. To exercise rollback, the existing BrokenBranchMerger pattern in test_merge_rollback.py injects a ValueError after merge_graph returns; the branch returns to OPEN and main data is restored.

Impact & rollout

  • Backward compatibility: BranchStatus enum gains MERGING. Any consumer that exhaustively switches on BranchStatus (clients, dashboards, reports) needs a case for it. is_terminal does not include MERGING. The new manage_rollback parameter on SchemaUpdateCoordinator.execute() is keyword-only with a default that preserves existing behavior, so all current call sites are unaffected.
  • Performance: Adds one extra Branch.save() at the start of every merge (for the MERGING write) and one at the end of the rollback path. Negligible. The consolidated RollbackQuery does the same DB work as the two queries it replaces, plus orphan vertex cleanup that the merge-rollback path was missing.
  • Config / env changes: None.
  • Deployment notes: Safe to deploy. No data migration needed. Branches currently at OPEN continue to behave as before; the new MERGING state only appears for newly initiated merges.

Checklist

  • Tests added/updated
  • Changelog entry added (changelog/9115.fixed.md)
  • External docs updated (if user-facing or ops-facing change)
  • Internal .md docs updated (internal knowledge and AI code tools knowledge)
  • I have reviewed AI generated content

@github-actions github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 1, 2026
@codspeed-hq

codspeed-hq Bot commented May 1, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing ajtm-04302026-wider-merge-rollback (5936396) with stable (5e9a835)

Open in CodSpeed

@github-actions github-actions Bot added the group/frontend Issue related to the frontend (React) label May 1, 2026
@ajtmccarty ajtmccarty marked this pull request as ready for review May 1, 2026 16:49
@ajtmccarty ajtmccarty requested review from a team as code owners May 1, 2026 16:49
db=db,
log=log,
obj=obj,
branch=obj,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread backend/infrahub/core/branch/tasks.py Outdated

branch.branched_from = pre_merge_branched_from
branch.status = BranchStatus.OPEN
await branch.save(db=db)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread backend/infrahub/core/branch/tasks.py Outdated
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we would have the user from the context and could inject it to .save().

Comment thread backend/infrahub/core/branch/tasks.py Outdated
await obj.save(db=db)
registry.branch[obj.name] = obj
branch.status = BranchStatus.MERGED
await branch.save(db=db)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ajtmccarty ajtmccarty merged commit c6bf872 into stable May 5, 2026
51 checks passed
@ajtmccarty ajtmccarty deleted the ajtm-04302026-wider-merge-rollback branch May 5, 2026 00:53
ogenstad pushed a commit to opsmill/infrahub-sdk-python that referenced this pull request May 20, 2026
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent) group/frontend Issue related to the frontend (React)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: failed merge can leave database in partially-merged state

2 participants