merge failure recovery improvements - #10031
Conversation
Add a `pre_merge_destination_schema_changed_at` field to Branch and stamp it onto the source branch when a merge starts, alongside `merge_started_at`. The destination branch's schema metadata (hash, changed-at) is written in place on the Branch vertex mid-merge and is not versioned, so the range rollback cannot revert it; persisting the pre-merge value here gives an out-of-process recovery the source it needs to restore it. The field is optional with a default, so it is backward compatible for existing Branch vertices and needs no migration. The merge-start bookkeeping is extracted into `_record_merge_start` so it can be verified without standing up the full merge machinery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a merge crashes after the schema graph is written, the CLI recovery rolls the graph back but left the destination branch's persisted schema_hash and schema_changed_at at their failed-merge values: those are plain Branch-vertex properties written in place mid-merge, so the versioned range rollback cannot revert them. The stale metadata disagreed with the rolled-back graph, so every worker kept detecting a "new" schema hash and churning through reconciles. Recovery now recomputes the schema hash from the rolled-back graph and restores schema_changed_at from pre_merge_destination_schema_changed_at (captured at merge start), reloading the destination branch first so the write lands on the current persisted record. The schema load is isolated behind an override point so tests can substitute an in-memory schema and skip the slow database round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…covery A hard-killed merge leaks no-TTL distributed locks (lock.global.graph, lock.global.schema, and the per-branch lock.diff-update.* locks) that silently block the next merge until the 15-minute deadlock reaper clears them. Merge recovery now clears these during recovery via a new StaleLockCleaner, deleting only locks whose holder is no longer an active worker so a lock a live flow still holds is left alone. The shared active-worker lookup is consolidated onto InfrahubComponent.list_active_worker_ids. The blocking merge-lock and global-graph-lock acquisitions on the merge path now log before they block, so a leaked-lock hang is diagnosable from the logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
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.
3 issues found across 16 files
Confidence score: 2/5
- In
backend/infrahub/locks/cleaner.py, the unconditional delete can race with recovery/reaper flow and remove a lock that has already been legitimately re-acquired, which could allow concurrent workers into the same critical section. Gate deletion on owner/version re-validation (compare-and-delete semantics) before removing the key. - In
backend/infrahub/core/merge/orchestrator.py, persistingNonefor a destination with no priorschema_changed_atmakes crash recovery skip restoration, leaving merge state unrecovered for that path. Capture and persist an explicit pre-merge sentinel/state so recovery can always distinguish “missing” from “recorded empty.” - In
backend/infrahub/locks/tasks.py, the updated call path no longer hits the mockedcomponent.list_workers, so the cleanup test can fail for the wrong reason and stop protecting this behavior. Update the test to mock/assert the newlist_active_workerspath to restore reliable regression coverage.
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/locks/cleaner.py">
<violation number="1" location="backend/infrahub/locks/cleaner.py:42">
P1: A live lock can be deleted after this cleaner observes its previous dead holder: a concurrent recovery/reaper can remove the key and let another worker acquire it before this unconditional delete, admitting concurrent graph/schema or diff-update work. An atomic compare-and-delete keyed by the token would preserve the lock acquired after the stale read.</violation>
</file>
<file name="backend/infrahub/locks/tasks.py">
<violation number="1" location="backend/infrahub/locks/tasks.py:25">
P2: The lock cleanup test now fails because this call bypasses the configured `component.list_workers` mock; the test still configures and asserts that old method. Updating the test to configure and assert `list_active_worker_ids` would keep coverage aligned with the new path.</violation>
</file>
<file name="backend/infrahub/core/merge/orchestrator.py">
<violation number="1" location="backend/infrahub/core/merge/orchestrator.py:195">
P2: A destination with no prior `schema_changed_at` cannot be restored after a crash: this line persists `None`, which recovery treats as an older record with no marker and skips. Recovery needs an explicit captured-state marker (or equivalent) so it can restore `schema_changed_at = None` rather than retaining the failed merge value.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| worker_id = get_worker_id_from_lock_token(token) | ||
| if worker_id is None or worker_id in active_worker_ids: | ||
| continue | ||
| await self.cache.delete(key=key) |
There was a problem hiding this comment.
P1: A live lock can be deleted after this cleaner observes its previous dead holder: a concurrent recovery/reaper can remove the key and let another worker acquire it before this unconditional delete, admitting concurrent graph/schema or diff-update work. An atomic compare-and-delete keyed by the token would preserve the lock acquired after the stale read.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/locks/cleaner.py, line 42:
<comment>A live lock can be deleted after this cleaner observes its previous dead holder: a concurrent recovery/reaper can remove the key and let another worker acquire it before this unconditional delete, admitting concurrent graph/schema or diff-update work. An atomic compare-and-delete keyed by the token would preserve the lock acquired after the stale read.</comment>
<file context>
@@ -0,0 +1,48 @@
+ worker_id = get_worker_id_from_lock_token(token)
+ if worker_id is None or worker_id in active_worker_ids:
+ continue
+ await self.cache.delete(key=key)
+ deleted.append(key)
+ log.warning("lock.stale.cleared", key=key, worker_id=worker_id)
</file context>
There was a problem hiding this comment.
technically true, but incredibly unlikely. the gap in here is 5 lines of Python
| """ | ||
| self.source_branch.status = BranchStatus.MERGING | ||
| self.source_branch.merge_started_at = merge_at.to_string() | ||
| self.source_branch.pre_merge_destination_schema_changed_at = self.destination_branch.schema_changed_at |
There was a problem hiding this comment.
P2: A destination with no prior schema_changed_at cannot be restored after a crash: this line persists None, which recovery treats as an older record with no marker and skips. Recovery needs an explicit captured-state marker (or equivalent) so it can restore schema_changed_at = None rather than retaining the failed merge value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/merge/orchestrator.py, line 195:
<comment>A destination with no prior `schema_changed_at` cannot be restored after a crash: this line persists `None`, which recovery treats as an older record with no marker and skips. Recovery needs an explicit captured-state marker (or equivalent) so it can restore `schema_changed_at = None` rather than retaining the failed merge value.</comment>
<file context>
@@ -184,3 +182,16 @@ async def merge(self, *, context: InfrahubContext, proposed_change_id: str | Non
+ """
+ self.source_branch.status = BranchStatus.MERGING
+ self.source_branch.merge_started_at = merge_at.to_string()
+ self.source_branch.pre_merge_destination_schema_changed_at = self.destination_branch.schema_changed_at
+ await self.source_branch.save(db=self.db, user_id=user_id)
+ registry.branch[self.source_branch.name] = self.source_branch
</file context>
There was a problem hiding this comment.
this is resolved
| values = await service.cache.get_values(keys=keys) | ||
| workers = await service.component.list_workers(branch=registry.default_branch, schema_hash=False) | ||
| workers_active = {worker.id for worker in workers if worker.active} | ||
| workers_active = await service.component.list_active_worker_ids(branch=registry.default_branch) |
There was a problem hiding this comment.
P2: The lock cleanup test now fails because this call bypasses the configured component.list_workers mock; the test still configures and asserts that old method. Updating the test to configure and assert list_active_worker_ids would keep coverage aligned with the new path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/locks/tasks.py, line 25:
<comment>The lock cleanup test now fails because this call bypasses the configured `component.list_workers` mock; the test still configures and asserts that old method. Updating the test to configure and assert `list_active_worker_ids` would keep coverage aligned with the new path.</comment>
<file context>
@@ -22,8 +22,7 @@ async def clean_up_deadlocks(service: InfrahubServices) -> None:
values = await service.cache.get_values(keys=keys)
- workers = await service.component.list_workers(branch=registry.default_branch, schema_hash=False)
- workers_active = {worker.id for worker in workers if worker.active}
+ workers_active = await service.component.list_active_worker_ids(branch=registry.default_branch)
for key, value in zip(keys, values, strict=False):
</file context>
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Confidence score: 4/5
- In
backend/infrahub/services/component.py,list_active_worker_ids()now touchesregistry.default_brancheven on cache-only liveness checks, which can raiseInitializationErrorbefore the registry is initialized and break worker health reporting/startup flows; decouple the cache-only path from branch resolution (or make branch lookup lazy/optional) so liveness checks still work pre-initialization.
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/services/component.py">
<violation number="1" location="backend/infrahub/services/component.py:84">
P2: Cache-only liveness checks now depend on `registry.default_branch`, so `list_active_worker_ids()` raises `InitializationError` whenever the registry has not been initialized even though no branch is needed. Passing an inert constant (or making `list_workers`' branch optional when `schema_hash=False`) would preserve the method's global-liveness behavior without this lifecycle dependency.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| Liveness is global: the active set is the same for every branch, so this takes no branch. | ||
| """ | ||
| # ``branch`` only scopes the schema-hash lookup, which is skipped here, so its value is inert. | ||
| workers = await self.list_workers(branch=registry.default_branch, schema_hash=False) |
There was a problem hiding this comment.
P2: Cache-only liveness checks now depend on registry.default_branch, so list_active_worker_ids() raises InitializationError whenever the registry has not been initialized even though no branch is needed. Passing an inert constant (or making list_workers' branch optional when schema_hash=False) would preserve the method's global-liveness behavior without this lifecycle dependency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/services/component.py, line 84:
<comment>Cache-only liveness checks now depend on `registry.default_branch`, so `list_active_worker_ids()` raises `InitializationError` whenever the registry has not been initialized even though no branch is needed. Passing an inert constant (or making `list_workers`' branch optional when `schema_hash=False`) would preserve the method's global-liveness behavior without this lifecycle dependency.</comment>
<file context>
@@ -75,9 +75,13 @@ async def list_workers(self, branch: str, schema_hash: bool) -> list[WorkerInfo]
+ Liveness is global: the active set is the same for every branch, so this takes no branch.
+ """
+ # ``branch`` only scopes the schema-hash lookup, which is skipped here, so its value is inert.
+ workers = await self.list_workers(branch=registry.default_branch, schema_hash=False)
return {worker.id for worker in workers if worker.active}
</file context>
| workers = await self.list_workers(branch=registry.default_branch, schema_hash=False) | |
| workers = await self.list_workers(branch=GLOBAL_BRANCH_NAME, schema_hash=False) |
|
|
||
| # Captured before the merge so it can be compared against what the merge stamps on the source branch. | ||
| pre_merge_destination_changed_at = default_branch.schema_changed_at | ||
| assert pre_merge_destination_changed_at is not None |
There was a problem hiding this comment.
Is this assertion useful?
There was a problem hiding this comment.
it's mostly to ensure that the test is running correctly. so that it is easier to troubleshoot if this is not set as expected. if just the new assertion on line 81 failed, that would look like a functional problem, but if default_branch.schema_changed_at is never set, then it is more a test configuration issue
| class InMemorySchemaRecoverer(MergeFailureRecoverer): | ||
| """Recoverer that recomputes the schema hash from the in-memory registry instead of the database. | ||
|
|
||
| Loading the schema from the database is a slow round-trip; the recovery logic under test does not | ||
| depend on that round-trip, so the schema is read from the registry to keep these tests fast. | ||
| """ | ||
|
|
||
| async def _load_destination_schema(self, branch: Branch) -> SchemaBranch: | ||
| return registry.schema.get_schema_branch(name=branch.name) |
| self.cache = cache | ||
| self.component = component | ||
|
|
||
| async def clear_if_holder_dead(self, keys: list[str]) -> list[str]: |
There was a problem hiding this comment.
This is an optional comment, which may be subjective:
I assume the return value is there for testing purposes, but in my opinion a method should either act or return something and not both. A method that does both is usually harder to maintain, and bending that rule just for testing is probably not a good enough reason.
There was a problem hiding this comment.
I think you are right. I've removed the return and updated the test to actually check the cache for existing locks
|
|
||
| async def _build_cleaner(cache: MemoryCache) -> StaleLockCleaner: | ||
| # list_active_worker_ids reads only the cache heartbeats, so a cache-backed component needs no database. | ||
| db = cast("InfrahubDatabase", None) |
There was a problem hiding this comment.
I'm not sure my idea is better, but since the only reason for injecting component into StaleLockCleaner is to call list_active_worker_ids, would a dedicated Protocol be a better solution, making the object easier to fake in tests?
There was a problem hiding this comment.
good idea. I've added a protocol to cover this and this test is much clearer now
| assert deleted == [DEAD_KEY] | ||
| assert await cache.get(DEAD_KEY) is None | ||
| assert await cache.get(LIVE_KEY) == live_token |
There was a problem hiding this comment.
If my comment above about a method not doing both is valid, then the second and third assertions should be enough here
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
2 participants
Why
Live failure-mode testing of the merge-failure-recovery feature surfaced two ways a crashed branch merge leaves the system in a degraded state that
infrahub recover mergedid not clean up:schema_hash/schema_changed_atat their failed-merge values. Those are plainBranch-vertex properties written in place mid-merge, so the versioned range rollback cannot revert them. The persisted metadata then disagrees with the rolled-back graph, so every worker keeps detecting a "new" schema hash and churning through reconciles.lock.global.graph,lock.global.schema, and the per-branchlock.diff-update.*locks). Recovery cleared only the merge lock, so the leaked locks silently blocked the next merge until the 15-minute deadlock reaper cleared them — with no log line before the blocking acquire to diagnose the hang.Goal: make
infrahub recover mergeleave the destination branch's schema metadata and the distributed locks in the same state a healthy merge would, and make a leaked-lock hang diagnosable from logs.Non-goals: no change to the merge happy path, the rollback range logic, or the failure-detection/
MERGE_FAILEDflagging.What changed
Behavioral changes
schema_hash(recomputed from the rolled-back graph) andschema_changed_at(restored from the value captured at merge start), so a recovered merge no longer triggers spurious "New hash detected" reconcile churn.Implementation notes
Branch.pre_merge_destination_schema_changed_at(optional, defaultNone) is stamped onto the source branch at merge start (extracted intoBranchMergeOrchestrator._record_merge_start) and is the source recovery restores from.StaleLockCleanercomponent (liveness-guarded deletes) and the shared active-worker lookup consolidated ontoInfrahubComponent.list_active_worker_ids.MergeFailureRecoverer._load_destination_schema) so tests can substitute an in-memory schema and skip the slow database round-trip.Impact & rollout
Branchfield, no migration; existing in-flight/olderMERGE_FAILEDrecords without the field are handled (schema hash still recomputed,schema_changed_atleft untouched).Checklist
Summary by cubic
Improves merge failure recovery by restoring destination schema metadata and clearing stale distributed locks so subsequent merges and workers don’t get stuck or recompute endlessly. Adds merge-start bookkeeping and better lock diagnostics for easier debugging.
New Features
schema_changed_atat merge start on the source branch (newBranchfield:pre_merge_destination_schema_changed_at).StaleLockCleaner(deletes only locks held by dead workers).WorkerLivenessprotocol and expose globalInfrahubComponent.list_active_worker_ids; log before/after acquiring global merge/graph locks; CLI recovery wires inSchemaManager,StaleLockCleaner, andDiffLocker.Bug Fixes
schema_changed_atfrom the captured pre-merge value, including when it wasNone.Written for commit 56b44ca. Summary will update on new commits.