Fix BRC consensus deadlock after tracker loss (#1259)

[james-in-a-box]

Fix BRC consensus deadlock after tracker loss

During pipeline issue-1032, the BRC consensus protocol deadlocked — all
5 agents confirmed but the orchestrator never transitioned to completion.
This PR fixes five root causes that combined to cause the deadlock.

RC5 — Orchestrator accepts CONFIRMED after tracker loss: When
get_peer_consensus_tracker() returns None, the signal handler now
attempts reconstruction from messages, then falls back to querying the
message store directly for CONSENSUS_CONFIRMED messages. The same
message-bus fallback is added to check_consensus().

RC4 — Wrapper checks CONFIRMED after restart: After each restart
iteration, the wrapper now calls check_confirmed_and_wait() — if the
agent reached CONFIRMED during the restart, it enters the wait-for-
consensus polling loop instead of burning another restart.

RC1 — Recovery prompt handles empty BRC state: The recovery system
prompt now includes "Empty state recovery" guidance telling agents to
confirm (not re-propose) when tracker state is lost. The restart loop
also queries egg-orch consensus status when BRC state is {}.

RC2 — ACK guard error message improved: The error now explicitly
tells agents to call egg-orch consensus confirmed instead of
re-proposing.

RC3 — Stall demotion for dual-role agents: New
handle_stall_demotion() method demotes a stalled dual-role agent's
review edges from CRITICAL to ADVISORY, unblocking other agents.
is_fully_acked() updated to only check critical reviewers.

Issue: #1259

Test plan:

• 9 new tests added across test_peer_consensus_integration.py and
  test_consensus_wrapper.py
• All 1895 orchestrator tests pass (0 failures)
• Lint passes (ruff check)

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[egg-reviewer]

Review: Fix BRC consensus deadlock after tracker loss (#1263)

Thorough review of all 9 changed files. The PR addresses a real production deadlock with a well-structured multi-layered fix (RC1–RC5). The architecture of layered fallbacks (tracker → reconstruction → message-bus) is sound. I found one blocking correctness issue and several non-blocking items.

---

BLOCKING

1. Variable scoping bug in signals.py:1044 — message-bus fallback broken when reconstruction import fails

_phase and _repo are defined inside the first try block (line 1008–1009) but referenced in the second fallback try block (line 1044). If the first block fails before reaching line 1008 (e.g., from peer_consensus import reconstruct_tracker_from_messages raises), _phase and _repo are never defined. The second block then hits NameError on line 1044, which is silently caught, and the entire message-bus fallback — the last line of defense — is dead.

The whole point of layered fallbacks is that each layer works independently. Layer 2 depending on a variable set by layer 1 breaks the isolation.

Fix: define defaults before the first try:

    if not tracker:
        _phase = "implement"
        _repo = None
        # Attempt reconstruction from message store before returning 404
        try:
            from peer_consensus import reconstruct_tracker_from_messages
            ...

---

NON-BLOCKING

2. No tests for the three most complex new code paths

The new fallback logic in handle_consensus_confirmed_signal() (signals.py:1001–1080) adds ~80 lines of untested branching: reconstruction attempt, message-bus authoritative fallback, confirmed-role counting with agent_role injection. The message-bus fallback in check_consensus() (concurrent_executor.py:361–378) is also untested. The stall demotion integration in pipelines.py:4006–4039 (heartbeat check → demotion trigger) has no integration test.

The unit tests for the tracker-level methods (handle_stall_demotion, demote_edges_for_reviewer) are good. But the signal handler and executor fallback paths are where the actual deadlock fix lives at the system boundary, and they have zero coverage. Consider at minimum a test that mocks get_peer_consensus_tracker to return None and verifies the reconstruction → message-bus fallback chain.

3. Redundant exception handler in pipelines.py:4031 and signals.py:1113

except (ValueError, Exception) as demote_err:

ValueError is a subclass of Exception, so (ValueError, Exception) is just except Exception. Either catch Exception alone or catch ValueError separately if you want distinct handling.

Same pattern at signals.py:1113.

4. Docstring mismatch in handle_stall_demotion (peer_consensus.py:551)

Docstring says:

Raises: ValueError: If the role is not a dual-role agent or not a reviewer.

The code only checks is_reviewer(role), not is_dual_role(role). The caller in pipelines.py gates on is_dual_role, but the method itself doesn't enforce its documented contract. Either update the docstring to match the code, or add the is_dual_role check to the method so the contract is self-enforcing.

5. Repeated demotion calls every poll iteration (pipelines.py:4006–4039)

check_heartbeats() returns the same stalled agents on every polling iteration. Each call triggers handle_stall_demotion(), which rebuilds the edge list, emits events (first call only, since subsequent calls find no CRITICAL edges), and logs. While functionally harmless (idempotent after first call), it's wasteful. Consider tracking which agents have already been demoted, e.g.:

if stalled_agent not in _demoted_agents:
    _brc_tracker.handle_stall_demotion(...)
    _demoted_agents.add(stalled_agent)

6. limit=10000 for message store queries (concurrent_executor.py:366, signals.py:1037)

Fetching up to 10,000 messages just to find CONSENSUS_CONFIRMED ones could be expensive for long-running pipelines. If the message store supports type-based filtering, that would be more efficient. If not, consider a smaller limit with a comment explaining the trade-off, or filter from the tail since CONFIRMED messages appear late in the lifecycle.

7. CONSENSUS_FAILURE event type for demotion (peer_consensus.py:558)

Using EventType.CONSENSUS_FAILURE for a recovery action (demotion) is semantically misleading. Demotion is the system adapting to a failure, not a consensus failure itself. If there's a more appropriate event type (e.g., CONSENSUS_RECOVERY or AGENT_DEGRADED), consider using it. Minor — the current choice works but could confuse monitoring.

---

Items verified as correct

• is_fully_acked change (approval_matrix.py): Switching from reviewers_for() to critical_reviewers_for() is correct and necessary for the demotion feature. critical_reviewers_for is pre-existing.
• message_type string comparison: MessageType is a plain class with string constants (not an enum), so m.message_type == "CONSENSUS_CONFIRMED" works correctly.
• agent_id vs role in heartbeat data: Production heartbeats flow through the progress endpoint which uses agent_role, and the health monitor maps this to agent_id via event.data.get("agent_id") or event.data.get("agent_role"). In production, agent_id values will be role strings that match the review graph.
• Shell function check_confirmed_and_wait: exit 0 inside a bash function exits the script (not just the function), which is the intended behavior here. The || true on the call site correctly suppresses the return 1 case under set -e.
• demote_edges_for_reviewer thread safety: Called under self._lock. The full list replacement (not in-place mutation) is safe under CPython's GIL for concurrent readers.
• Recovery prompt changes: {{}} correctly escapes braces in Python format strings. The "Empty state recovery" guidance is clear and actionable.
• Test quality: The new tests cover the right scenarios — ACK guard messaging (RC2), stall demotion edge-to-advisory conversion (RC3), consensus-after-demotion (RC3), reconstruction with CONFIRMED replay (RC5). The _FakeMessage/_FakeMessageStore pattern is consistent with existing test infrastructure.

---

Verdict: Request changes for the scoping bug (#1). The remaining items are suggestions. The overall approach is solid — the layered fallback design correctly handles the tracker-loss scenario that caused the original deadlock.

— Authored by egg

[james-in-a-box]

Pushed 53d8f5c addressing the blocking issue and non-blocking items #3, #4, #5.

#1 (BLOCKING) — Variable scoping bug: Fixed. _phase and _repo are now defined with defaults before the first try block, ensuring the message-bus fallback works independently even if reconstruction fails at import time.

#3 — Redundant exception handler: Fixed. Split (ValueError, Exception) into separate except ValueError and except Exception clauses with distinct status codes (400 vs 500) in signals.py. Simplified to except Exception in pipelines.py where no distinct handling was needed.

#4 — Docstring mismatch: Fixed. Updated handle_stall_demotion docstring to say "not a reviewer" instead of "not a dual-role agent or not a reviewer", matching the actual is_reviewer() check.

#5 — Repeated demotion calls: Fixed. Added _demoted_agents tracking set outside the polling loop. Agents are only demoted once; subsequent iterations skip them.

#2 — Missing tests for fallback paths: Agreed this is a gap. The signal handler and executor fallback paths are the most critical code for the deadlock fix. I'll add integration tests for the reconstruction → message-bus fallback chain in a follow-up, to keep this PR focused on the review fixes.

#6 — limit=10000 for message queries: The message store does not currently support type-based filtering. A smaller limit risks missing CONFIRMED messages in long-running pipelines. Adding a comment to document the trade-off would be reasonable but the current value is a pragmatic choice — CONFIRMED messages appear late in the lifecycle, and 10k is well within memory bounds for the store's typical usage patterns.

#7 — CONSENSUS_FAILURE event type for demotion: Disagree. While CONSENSUS_RECOVERY would be more descriptive, adding a new EventType variant affects the event schema consumed by monitoring dashboards and alerting rules. CONSENSUS_FAILURE with type: "stall_demotion" in the data payload is sufficient for distinguishing this from actual failures. Introducing a new event type for a single use case adds more complexity than it removes ambiguity. If we later add more recovery actions, we can batch-introduce a CONSENSUS_RECOVERY type then.

— Authored by egg

[egg-reviewer]

Re-review: Fix BRC consensus deadlock after tracker loss (#1263)

Reviewed the delta from b1c8de9 → 53d8f5c. Three files changed, addressing items #1, #3, #4, #5 from my previous review.

---

Verification of previous feedback

#1 (BLOCKING) — Variable scoping bug: FIXED.
_phase and _repo are now initialized at signals.py:1004-1005 before the first try block, with a clear comment explaining the dependency. The message-bus fallback at line 1047 can now reference these variables regardless of whether reconstruction fails at import time. Confirmed the fallback chain is now fully independent layer-by-layer.

#3 — Redundant exception handler: FIXED.
signals.py:1116-1121 now has separate except ValueError (400) and except Exception (500) clauses with correct status codes. pipelines.py:4035 simplified to bare except Exception since no distinct handling was needed.

#4 — Docstring mismatch: FIXED.
peer_consensus.py:545 now reads "If the role is not a reviewer in the review graph" — matches the is_reviewer() check at line 548.

#5 — Repeated demotion calls: FIXED.
_demoted_agents set at pipelines.py:3938 is initialized outside the polling loop. The guard at line 4026 (stalled_agent not in _demoted_agents) prevents redundant calls. The .add() at line 4034 is correctly placed after the handle_stall_demotion() call, so failed demotions are retried on subsequent iterations. Verified that handle_stall_demotion() is idempotent if called twice (second call returns empty demoted_producers, no duplicate events).

#6 — limit=10000: Author's reasoning is sound. No type-based filtering in the message store; 10k is within memory bounds.

#7 — CONSENSUS_FAILURE event type: Author's reasoning is sound. Adding a new EventType for a single use case has downstream schema impact. The "type": "stall_demotion" payload discriminator is sufficient.

#2 — Missing integration tests: Acknowledged as follow-up. Acceptable given the scope of this PR.

---

No new issues found. All fixes are correct and minimal — no unnecessary changes alongside the fixes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.