Skip to content

Commit 2ec412e

Browse files
ajtmccartyclaude
andcommitted
document failed-merge detection and align the spec with the shipped shape
Knowledge-base note for the MERGE_FAILED status and detection mechanism, and internal-api contract updated to the scan()/module-function shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 675b6c0 commit 2ec412e

5 files changed

Lines changed: 86 additions & 58 deletions

File tree

dev/knowledge/backend/branch-status.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,16 @@ How Infrahub enforces read-only constraints on branches based on their lifecycle
1515
| `NEED_UPGRADE_REBASE` | Schema version behind | `BranchDelete` |
1616
| `DELETING` | Deletion in progress (internal) | None |
1717
| `MERGING` | Merge in progress; branch and default branch write-blocked | `BranchCreate`, `BranchDelete` (of uninvolved branches) |
18+
| `MERGE_FAILED` | A merge died (worker killed mid-merge); branch and default branch stay write-blocked until recovery | `BranchCreate`, `BranchDelete` (of uninvolved branches) |
1819
| `MERGED` | Successfully merged; permanently read-only | `BranchDelete` only |
1920

20-
`MERGED` is terminal — there is no transition back to `OPEN`. `MERGING` is transient: it becomes `MERGED` on success, or reverts to `OPEN` if the merge rolls back.
21+
`MERGED` is terminal — there is no transition back to `OPEN`. `MERGING` is transient: it becomes `MERGED` on success, reverts to `OPEN` if the merge rolls back, or is flipped to `MERGE_FAILED` if the merge worker dies. `MERGE_FAILED` is cleared only by recovery (it returns the branch to `OPEN`).
22+
23+
### Failed merge detection
24+
25+
`backend/infrahub/core/merge/failure_recovery.py`
26+
27+
A merge holds the global `all_branches` merge lock for its whole `MERGING` window; the lock token encodes the holder's `worker_id`. When the holding worker dies, the lock stays held by a `worker_id` that is no longer in the active-worker set. `MergeFailureRecovery.detect_and_mark` flips such a branch `MERGING → MERGE_FAILED` (after a configurable grace period, `INFRAHUB_MERGE_FAILURE_GRACE_PERIOD_SECONDS`, that absorbs a transient heartbeat blip) and updates the `merge:protected` key to `"{branch}::MERGE_FAILED"`. It runs from the recurring `MERGE_WATCHER` workflow (one-minute cron, single-flighted), from a check at worker startup, and the recurring scan also reconciles the cache key against the durable branch status so protection self-heals after a restart or cache flush. A healthy in-progress merge (lock held by a live worker) is never flagged.
2128

2229
## BranchStatusChecker
2330

@@ -45,7 +52,7 @@ While a merge runs, writes are blocked on two branches: the **source branch** (i
4552

4653
The gate is driven by `MergeWriteBlocker`, which manages the shared `merge:protected` cache key with value `"{branch}::{state}"` (`MergeProtectionState.MERGING` or `MERGE_FAILED`). The merge flow in `backend/infrahub/core/branch/tasks.py` sets the key before any graph write and deletes it when the merge completes or rolls back. Every worker sees the same state with a single cache lookup per top-level mutation.
4754

48-
`check_merging_status()` reads the key and raises `MergeInProgressError` when the write target is the merging branch or the default branch. If the cache is unreachable, it falls back to the durable branch status in the database (`_check_merging_status_from_db`), so a cache outage only blocks writes when a merge is genuinely in progress.
55+
`check_merging_status()` reads the key and raises when the write target is the merging branch or the default branch. The key's state decides which error: a `MERGING` key raises the transient, retryable `MergeInProgressError` (code `MERGE_IN_PROGRESS`), while a `MERGE_FAILED` key raises the durable `MergeRecoveryRequiredError` (code `MERGE_RECOVERY_REQUIRED`, HTTP 423) whose message directs an administrator to run `infrahub recover`. `MergeRecoveryRequiredError` is a sibling of `MergeInProgressError`, not a subclass, so the error-catalogue resolver cannot collapse it onto the retryable code. If the cache is unreachable, it falls back to the durable branch status in the database (`_check_merging_status_from_db`), so a cache outage only blocks writes when a merge is genuinely in progress or failed.
4956

5057

5158
## Enforcement Points

dev/specs/ifc-2437-merge-failure-recovery/contracts/internal-api.md

Lines changed: 53 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -179,42 +179,48 @@ deleted/reopened; the `updated_at >= merge_at` filter excludes already-restored
179179

180180
**New file**: `backend/infrahub/core/merge/failure_recovery.py`
181181

182-
A DI component (constructor-injected collaborators) with two entry points:
182+
A DI component (constructor-injected collaborators). Detection (this increment) exposes a single
183+
entry point `scan`; recovery (PR-5) adds `recover` and injects a `diff_merger` for the range
184+
rollback.
183185

184186
```python
185187
class MergeFailureRecovery:
186188
def __init__(
187189
self,
188190
db: InfrahubDatabase,
189-
diff_merger: DiffMerger, # owns the range rollback query (no get_affected_node_uuids needed)
190-
cache: InfrahubCache, # merge:protected key set/update/delete; read the lock-holder token
191-
component: InfrahubComponent, # list_workers() -> active-worker set for the liveness predicate
192-
merge_locker: MergeLocker, # read-side helper: current merge-lock holder worker_id (or None)
191+
cache: InfrahubCache, # read the merge-lock holder token
192+
component: InfrahubComponent, # list_workers() -> active-worker set for the liveness check
193+
merge_write_blocker: MergeWriteBlocker, # owns the merge:protected key (set/get/delete)
194+
# recovery (PR-5) additionally injects: diff_merger: DiffMerger (the range rollback query)
193195
) -> None: ...
194196

195-
async def detect_and_mark(self) -> str | None:
196-
"""Evaluate the failed-merge predicate and, if matched, transition the
197-
branch MERGING -> MERGE_FAILED (preserving merge_started_at) and update
198-
the merge:protected cache key to '{branch}::MERGE_FAILED'.
199-
Idempotent. Returns the branch name marked failed, or None.
200-
201-
Predicate: status == MERGING AND the MergeLocker 'all_branches' lock is
202-
PRESENT AND its token worker_id is not in the active-worker set AND
203-
(now - merge_started_at) > grace_period. The lock must be present (a dead
204-
worker cannot release it, so a real failure leaves it present); an ABSENT
205-
lock is not auto-flagged, because absence is ambiguous (a cache flush
206-
during a live merge would otherwise false-positive). A branch whose lock
207-
holder is active, or whose merge is younger than the grace period, is
208-
healthy in-progress and left untouched. The grace period (small,
209-
configurable, default ~2-3 min) absorbs a transient heartbeat-write blip.
197+
async def scan(self) -> str | None:
198+
"""Detect a dead merge and reconcile the protection key, in one pass. Idempotent.
199+
Returns the branch flagged failed this pass, or None.
200+
201+
_detect_and_mark: if a MERGING branch's merge is dead, transition it
202+
MERGING -> MERGE_FAILED (preserving merge_started_at) and update the
203+
merge:protected key to '{branch}::MERGE_FAILED'.
204+
_reconcile_protection_key: re-align the merge:protected key with the durable
205+
MERGING/MERGE_FAILED branch status (a single multi-status query) so it
206+
self-heals after a restart or cache flush.
207+
208+
A merge is dead when: status == MERGING AND the 'all_branches' merge lock is
209+
HELD by a worker_id not in the active-worker set AND (now - merge_started_at)
210+
> grace_period. The lock must be held (a dead worker cannot release it, so a
211+
real failure leaves it held); an ABSENT lock is not auto-flagged, because
212+
absence is ambiguous (a cache flush during a live merge would otherwise
213+
false-positive). A branch whose lock holder is active, or whose merge is
214+
younger than the grace period, is healthy in-progress and left untouched. The
215+
grace period (configurable, default ~3 min) absorbs a transient heartbeat blip.
210216
"""
211217

212-
async def recover(self, *, confirmed: bool) -> RecoveryReport:
218+
async def recover(self, *, confirmed: bool) -> RecoveryReport: # PR-5 — not yet implemented
213219
"""Find the failed merge and (when confirmed) recover it.
214220
215221
Detection covers BOTH (FR-016): (i) a branch recorded MERGE_FAILED, and
216222
(ii) a branch stuck in MERGING whose merge-lock holder is not a live
217-
worker (the ambiguous case detect_and_mark deliberately does not flag).
223+
worker (the ambiguous case scan deliberately does not flag).
218224
The human confirmation is what makes acting on (ii) safe. Then:
219225
220226
1. Run the range rollback (§3) over the default branch: reopen edges
@@ -252,18 +258,30 @@ The `outcome` enum (not a free-text `note`) lets the CLI format its own human-re
252258
lets tests assert on a structured value; the remaining fields carry what to report (branch name, the
253259
persisted merge timestamp, the associated proposed change if any).
254260

255-
The failed-merge **predicate** is a pure helper (unit-testable without DB), taking
256-
`(status, lock_token: str | None, active_worker_ids: set[str], merge_started_at: Timestamp | None,
257-
now: Timestamp, grace_period: Duration)``bool`. Both `detect_and_mark` and the on-write/on-merge
258-
fast paths call it.
261+
The failed-merge **predicate** `is_failed_merge` is a side-effect-free method on the component
262+
(unit-testable without a DB), taking `(status, lock_holder_worker_id: str | None,
263+
active_worker_ids: set[str], merge_started_at: Timestamp | None, now: Timestamp,
264+
grace_period_seconds: int)``bool`. It does no parsing: the lock-holder worker id is read via
265+
`merge_locker.get_merge_lock_holder_worker_id(cache)`, which parses the token with
266+
`lock.get_worker_id_from_lock_token` (the token format lives beside the lock). An optional on-write/
267+
on-merge escalation could call the same predicate, but the block does not depend on it and it is not
268+
implemented in this increment.
259269

260270
**Detection callers**:
261271

262-
- Recurring scan flow (new) — see §8.
263-
- `backend/infrahub/core/initialization.py`, after `initialize_registry()` (runs on both API server
264-
and git-agent startup).
265-
- Write gate (§5) when a default-branch write finds a branch in `MERGING`.
266-
- Merge/rebase gate (§5) before starting a new operation.
272+
- Recurring scan flow (new, the `MERGE_WATCHER` Prefect cron that runs on the task workers) — see §8.
273+
This is the continuous, authoritative detector.
274+
- **API server startup**: a best-effort `detect_failed_merge_on_startup` call from the API server
275+
entry point (`backend/infrahub/server.py`), right after `initialization()`, for immediate
276+
detection on a full-stack restart rather than waiting up to one scan interval. It is *not* wired
277+
into `initialization()` itself (which is also called by offline CLI/migration paths that have no
278+
`service`) nor into the `git-agent` entry point: the git-agent is a hidden/legacy command that
279+
runs no flows and requires the API server to be up anyway, so the API-server startup check plus the
280+
recurring cron already cover it.
281+
- Write gate (§5) when a default-branch write finds a branch in `MERGING` — optional fast-path
282+
escalation (FR-011b); the block does not depend on it, so it can be added later without changing
283+
behavior.
284+
- Merge/rebase gate (§5) before starting a new operation — same optional escalation.
267285

268286
**Recovery caller**: the `infrahub recover` CLI command (§7).
269287

@@ -400,8 +418,9 @@ MERGE_WATCHER = WorkflowDefinition(
400418

401419
`setup_deployments()` (`backend/infrahub/workflows/initialization.py`) iterates `get_workflows()`
402420
and creates the cron deployment automatically once `MERGE_WATCHER` is in `WORKFLOWS`. The flow calls
403-
`MergeFailureRecovery.detect_and_mark()`. `concurrency_limit=1` + `CANCEL_NEW` single-flights it
404-
across workers (same guarantee as `clean-up-deadlocks`).
421+
`scan_for_failed_merges(db, service)`, which runs `MergeFailureRecovery.scan()` (detect + reconcile).
422+
`concurrency_limit=1` + `CANCEL_NEW` single-flights it across workers (same guarantee as
423+
`clean-up-deadlocks`).
405424

406425
## 9. Logging events
407426

@@ -415,7 +434,7 @@ downstream automation, but that is out of scope here and recovery must not depen
415434

416435
| Log event | Where | Fields |
417436
|-------|-------|--------|
418-
| `merge.failure.detected` | `MergeFailureRecovery.detect_and_mark` (task worker) | `branch`, `merge_started_at`, `proposed_change`, `worker_id`, `source` |
437+
| `merge.failure.detected` | `MergeFailureRecovery` detection (task worker) | `branch`, `merge_started_at`, `proposed_change`, `worker_id`, `source` |
419438
| `merge.recovery.started` | `MergeFailureRecovery.recover` (CLI) | `branch`, `merge_started_at`, `proposed_change` |
420439
| `merge.recovery.completed` | `MergeFailureRecovery.recover` (CLI) | `branch`, `proposed_change`, `duration_ms` |
421440
| `merge.recovery.failed` | `MergeFailureRecovery.recover` (CLI) | `branch`, `error` |

dev/specs/ifc-2437-merge-failure-recovery/plan.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ backend/infrahub/
146146
│ └── recurring.py / merge_watcher # NEW recurring scan flow (module for the INTERNAL workflow)
147147
├── workflows/
148148
│ └── catalogue.py # ADD MERGE_WATCHER WorkflowDefinition to WORKFLOWS
149-
├── core/initialization.py # ADD startup detection call (API + git-agent both run initialization())
149+
├── server.py # ADD startup detection call after initialization() (API server entry point only)
150150
├── config.py # ADD merge-failure grace-period setting (default ~2-3 min)
151151
├── cli/
152152
│ ├── __init__.py # register `recover` command/typer

dev/specs/ifc-2437-merge-failure-recovery/research.md

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,13 @@ recovery needs no UUID list under R8); replay a transaction log (Neo4j exposes n
214214
(`backend/infrahub/workflows/initialization.py`) iterates `get_workflows()` and creates the
215215
cron deployment automatically once the definition is in `WORKFLOWS`.
216216

217-
2. **Startup fast path (FR-011a)** — call the detector once from
218-
`backend/infrahub/core/initialization.py` after `initialize_registry()` has loaded branches.
219-
Both the API server (`server.py:app_initialization`) and the git-agent
220-
(`cli/git_agent.py:start`) call `initialization(db)`, so a restart of either records the
221-
failure immediately.
217+
2. **Startup fast path (FR-011a)** — call the detector once from the API server entry point
218+
(`server.py:app_initialization`), right after `initialization()` has loaded branches, so a
219+
full-stack restart records the failure immediately instead of waiting up to one scan interval.
220+
It is kept out of `initialization()` itself (also called by offline CLI/migration paths with no
221+
`service`) and out of the git-agent (`cli/git_agent.py:start`), which is a hidden/legacy command
222+
that runs no flows and depends on the API server being up — so the API-server startup check plus
223+
the recurring cron (run on the task workers) already cover it.
222224

223225
3. **On-demand fast paths (FR-011b/c, FR-012)** — when a write to the default branch finds a branch
224226
in `MERGING`, escalate by evaluating the predicate so the write returns the recovery message

0 commit comments

Comments
 (0)