Fix #2242: alive-signal gate on heartbeat/progress alerts; plan-phase post-ACK threshold - #2268
Conversation
… post-ACK threshold Heartbeat-stall and progress-stall alerts fired prematurely on plan-phase producers during long-form Anthropic completions: the agent is mid-draft, no tool calls in flight, so no `mcp__brc__send_heartbeat` arrives on the bus. On `issue-1557-v2` this escalated to a 3-of-3 producer-silence alert at 355s while every producer was simply composing its draft. Separately, the 180s post-ACK confirmation timeout was tight for plan-phase reconciliation (12 resolved decisions, 6 feedback bodies, slice-DAG sanity passes). Apply two fixes, both leaning on primitives shipped in #2254: 1. Alive-signal gate at the per-agent alert sites. Before firing `heartbeat_timeout` or `progress_stall`, consult `PeerConsensusTracker.get_latest_progress_timestamp()` plus peer-heartbeat snapshots; defer if either has fired within `orchestrator_alert_progress_gate_seconds` (default 300s, 0 disables). Self-excluded so a solo silent agent still escalates. The escalated flag is intentionally not set on defer, so the next poll re-checks. 2. Phase-aware post-ACK confirm timeout. Plan phase now uses `orchestrator_plan_post_ack_confirmation_timeout_seconds` (default 300s); refine/implement keep the existing 180s default. Out of scope (call out in #2059 follow-ups): - "Anthropic completion in flight" SDK telemetry — the cleanest fix for the heartbeat detector, but requires SDK work; the alive-signal gate covers the common case at far lower cost. - Voluntary "I'm finalizing" heartbeats that reset the post-ACK timer — same SDK-side dependency. - Auto-attaching log-tail evidence to OVERSEER_ALERT messages. Same-role cross-phase pollution caveat documented in the existing `_check_brc_progress_gate` TODO applies here too: a phase-stamped heartbeat key would close both at once.
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Test/Unit Tests": 2, "Lint/Custom Checks": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The 4 failing tests in TestMultipleAgentsStalling create multiple agents where one or both are deliberately stalled. The new alive-signal gate (#2242) defers per-agent stall alerts when peer agents have heartbeats within orchestrator_alert_progress_gate_seconds (default 300s), which caused these tests to observe zero escalations instead of the expected per-agent escalations. Set orchestrator_alert_progress_gate_seconds=0 in the four failing tests so they isolate the per-agent escalation behavior from the peer-progress deferral, matching the pattern already used in test_health_monitor.py::test_heartbeat_timeout_per_agent.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…/pipelines.py Both files grew past their recorded baselines in the allowlist, causing the file-sizes custom check to fail on PR #2268. Update baselines to the actual current sizes so CI passes. - orchestrator/peer_consensus.py: 1988→2003 lines, 85268→85965 bytes - orchestrator/routes/pipelines.py: 15356→15514 lines, 669950→677112 bytes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…pts/file-size-allowlist.yaml
Conflict Resolution SummaryResolved merge conflicts with
Side-effect commit: select_tests.py baselineAfter resolving, Please review:
Local verification
Two — Authored by egg |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The fix is sound: it directly addresses the false-positive agent-heartbeat-stall cited in #2242 and the tight plan-phase post-ACK threshold. The alive-signal gate correctly mirrors the existing _check_brc_progress_gate vocabulary, the lock discipline is clean, the self-exclusion logic for solo agents works, and the test suite is genuinely thorough — covering peer-fresh defer, BRC-bus-fresh defer, gate-window-elapsed proceed, self-exclusion, gate-disabled bypass, missing-tracker fallback, and progress-stall path. Existing multi-agent tests in test_health_monitor.py and test_two_tier_integration.py were correctly updated to disable the gate. No blocking issues.
A few non-blocking concerns worth flagging:
1. BRC-bus path is not self-excluded
_has_recent_peer_progress excludes the focal agent only from the peer-heartbeat signal. The BRC-bus path (tracker.get_latest_progress_timestamp()) aggregates CONSENSUS_PROPOSE and ACK/NACK timestamps without filtering by the focal agent. For a single-producer pipeline (BRC tracker registered, no peers), the producer's own recent propose/ACK timestamp will defer its own heartbeat alert until gate_seconds elapses past that timestamp.
This means the effective stall-detection window for an agent that just proposed and then hangs is heartbeat_threshold + gate_seconds (≈ 360s with defaults) rather than heartbeat_threshold (60s). For genuinely-dead containers this is fine — CONTAINER_STOPPED covers it — but for a hung process inside a live container, detection is delayed by 5 minutes.
The PR description says "Self-excluded gate so a solo silent agent still escalates"; that guarantee actually only holds when both peer-heartbeat and BRC-bus signals are absent. Worth either:
- Updating the docstring to clarify that BRC-bus self-deferral can occur within the gate window after a propose, or
- Filtering tracker timestamps by the focal agent (would require
tracker.get_latest_progress_timestamp(exclude_role=agent_id)or equivalent).
The trade-off is acceptable, but the discrepancy between description and behavior should be reconciled.
2. Cross-phase pollution TODO is real but not urgent
health_monitor.py:230-260 — the TODO acknowledges that _last_heartbeat is not phase-filtered, but unlike the existing _check_brc_progress_gate (which filters by active_role_names), this gate doesn't filter at all. In practice this is mitigated because:
- Sandbox heartbeat cadence (~30s) is well under
gate_seconds(300s), so stale heartbeats from prior-phase containers age out naturally. reset_agentis called on respawn, clearing_last_heartbeat[agent_id].
But a phase boundary that doesn't trigger respawn (e.g., overseer carrying across phases, or a refiner→planner transition where the refiner container is still spinning down) could leave a fresh heartbeat that defers a current-phase alert. Worth at least matching _check_brc_progress_gate's active_role_names filter — the symmetry argument cuts both ways: if cross-phase pollution warranted a filter there, it warrants one here.
3. Sliced pipelines: BRC-bus signal is intentionally partial
get_peer_consensus_tracker(self._pipeline_id) is called without slice_id, so per-slice trackers (#2137) aren't consulted. The docstring acknowledges this and points to peer-heartbeat fallback. Acceptable, but the consequence is that a sliced pipeline whose only signs of life are slice-scoped CONSENSUS_PROPOSEs without container heartbeats would not benefit from this gate at all. Worth a follow-up if sliced pipelines start tripping false positives.
4. File-size allowlist updates for unrelated files
scripts/file-size-allowlist.yaml bumps orchestrator/routes/pipelines.py (+158 lines), orchestrator/peer_consensus.py (+15 lines), and scripts/select_tests.py (+200 lines) — none of which are touched by this PR. These are presumably housekeeping after rebasing on main. Not blocking, but it muddies the diff and means a future bisect on this PR will reveal allowlist drift unrelated to the fix. Worth a separate housekeeping PR going forward.
5. Minor: unnecessary defensive override in test
test_check_brc_progress_uses_plan_phase_threshold sets orchestrator_alert_progress_gate_seconds=0, but check_brc_progress doesn't consult _has_recent_peer_progress. The override is harmless but the comment ("isolate the phase-aware threshold path") implies a coupling that doesn't exist.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…pts/file-size-allowlist.yaml
Conflict Resolution SummaryResolved merge conflicts with
Verification
Please review: the disjoint config additions in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conflict Resolution SummaryNo action taken — the branch is already up to date with
The previous merge commits ( If GitHub is still showing this PR as conflicting, refreshing the PR page should clear the stale state. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review
The PR-specific surface is byte-identical to my prior review at 6a29efa48. The new commits since then are all merge-from-main:
b5e0d21d4and389cb6ee0— merges oforigin/main, bringing in #2245 (post-consensus rebaseline), #2266 (sys.path scrub), #2271 (docs), #2274 (file-size allowlist cleanup).
git diff 6a29efa48..origin/egg/issue-2242-alive-signal-gate -- orchestrator/health_monitor.py orchestrator/tests/test_health_monitor.py orchestrator/tests/test_two_tier_integration.py returns zero lines. The PR-specific delta against origin/main shrank only because main absorbed the unrelated scripts/file-size-allowlist.yaml baselines I flagged in point 4 of my prior review — the orchestrator/* portion (441 insertions / 4 deletions) is unchanged.
Merge-integration sanity check
#2245 and this PR both touch BRC tracker timestamp surfaces in peer_consensus.py. They use distinct methods that coexist cleanly:
- This PR's alive-signal gate (
health_monitor.py:_has_recent_peer_progress,routes/pipelines.py:9637) callstracker.get_latest_progress_timestamp()— aggregates proposals + matrix ACK/NACK entries. - #2245's post-timeout rebaseline (
routes/pipelines.py:10816_latest_proposal_ts) callstracker.get_latest_proposal_timestamp()— proposals only.
get_latest_progress_timestamp() calls get_latest_proposal_timestamp() internally and then unions in the matrix entry timestamp; both are read-only with the tracker's lock held. No conflicts in models.py either — the two PRs added disjoint PipelineConfig fields in adjacent regions.
CI
Unit Tests, Custom Checks, Lint, Security Scan, Aggregate Test Results all green at b5e0d21d4.
Previous suggestions still stand (non-blocking)
The five non-blocking concerns from my prior review (BRC-bus self-deferral within gate window, cross-phase heartbeat pollution TODO, sliced-pipeline tracker scope, allowlist hygiene, harmless test override) are unchanged and remain non-blocking. Suggestion 4 (allowlist drift) is now moot — main absorbed the unrelated baselines.
No new blocking issues. Re-approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
heartbeats by active-agent set Reviewer flagged three actionable non-blocking concerns on #2268: 1. Docstring discrepancy. The PR description claims "self-excluded gate so a solo silent agent still escalates", but self-exclusion only applies to the peer-heartbeat path. ``get_latest_progress_timestamp`` aggregates proposals + matrix entries across the whole tracker, so on a single-producer pipeline the producer's own propose/ACK timestamp defers its own alert until ``gate_seconds`` elapses past that timestamp (effective stall window ≈ ``heartbeat_threshold + gate_seconds``). ``CONTAINER_STOPPED`` covers genuinely-dead containers; the delay only matters for hung processes inside live containers. Filtering by focal agent would require a new ``peer_consensus`` API — deferred. Docstring updated to call out the behavior explicitly. 2. Cross-phase heartbeat pollution. Reviewer noted the symmetry argument: ``_check_brc_progress_gate`` filters peer heartbeats by ``active_role_names`` to drop stale prior-phase heartbeats from the shared HealthMonitor; this gate did not. Added a snapshot of ``set(self._agents.keys())`` taken under the same lock as ``_last_heartbeat`` so prior-phase ghosts (agents whose containers were stopped without ``reset_agent``) cannot defer current-phase alerts. Same-role cross-phase pollution remains, tracked by the existing TODO. New regression test: ``test_gate_filters_inactive_agent_heartbeats``. 3. Misleading test override. ``test_check_brc_progress_uses_plan_phase _threshold`` set ``orchestrator_alert_progress_gate_seconds=0`` with a comment about isolating the phase-aware threshold path — but ``check_brc_progress`` doesn't consult ``_has_recent_peer_progress``, so the override implied a coupling that doesn't exist. Removed. Two other reviewer concerns left as-is: the sliced-pipeline tracker scope is acknowledged in the docstring (peer-heartbeat fallback covers it), and the file-size allowlist drift was absorbed by main and is now moot.
There was a problem hiding this comment.
Re-review of 3fcec48
PR-author surface is byte-identical to my prior review at 76c3aac5. The two new commits since then (b22ed8f0c, 3fcec480) are both Merge branch 'main' into egg/issue-2242-alive-signal-gate — no PR-author code changes.
Verified:
git diff 76c3aac5..pr-2268 -- \
orchestrator/health_monitor.py \
orchestrator/tests/test_health_monitor.py \
orchestrator/tests/test_two_tier_integration.py
# zero lines
git diff 76c3aac5..pr-2268 -- orchestrator/models.py
# only #2264-imported description text ("HITL decision" → "OVERSEER_ALERT",
# "(#2243)" → "(#2243, #2264)") plus a "PipelineConfig" string-→-bare
# annotation tweak on _validate_post_consensus_budgets — both from main.
The full PR delta against origin/main is unchanged from the prior re-review:
orchestrator/health_monitor.py(+138/-4)orchestrator/models.py(+20/-0)orchestrator/tests/test_health_monitor.py(+311/-1)orchestrator/tests/test_two_tier_integration.py(+18/-2)
Merge sanity
b22ed8f0cbrought in #2279 (Python 3.14 pin), #2257 (asyncio.run for 3.14), #2260 (tester scaffold telemetry), #2273 (post-consensus rebaseline doc), #2269 (replace consensus-timeout HITL decision with OVERSEER_ALERT). #2269 is the only one that touches PR-adjacent surfaces — it rewrote thebrc_consensus_progress_gate_secondsdocstring onmodels.py:406("HITL decision" → "OVERSEER_ALERT"). Auto-merge took main's wording cleanly; the field this PR added (orchestrator_alert_progress_gate_secondsatmodels.py:496-505) is in a different region ofPipelineConfigand was not touched.3fcec480brought in #2292 (pyyaml install in action composite). Pure CI/action surface; no overlap with this PR.
git merge-base origin/main pr-2268 is 50e16d99 (HEAD of main as of b22ed8f0c's merge); merge state still clean.
CI
All required checks green at 3fcec480:
- Lint (Python, Shell, YAML, Dockerfile, Actions, Custom Checks, Aggregate)
- Test (Unit Tests, Security Scan, Aggregate)
- Validate configuration (Code Review, Contract Verification)
Prior suggestions
The five non-blocking concerns from earlier reviews are unchanged:
- BRC-bus self-deferral within gate window — docstring at
health_monitor.py:233-258already reconciles this honestly. - Active-agent filter is a no-op in production (cross-phase pollution unaddressed) — flagged in the prior re-review at
76c3aac5. Still non-blocking; the docstring athealth_monitor.py:268-275overstates what the filter accomplishes, but the code is harmless. - Sliced-pipeline tracker scope — acknowledged in docstring; non-blocking.
- Allowlist drift — moot.
- Misleading test override — fixed at
76c3aac5.
No new blocking issues. Re-approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reviewer noted in 76c3aac that the active-agent filter in _has_recent_peer_progress is a no-op in production: every heartbeat write also populates _agents, so set(self._agents.keys()) is a static superset of _last_heartbeat.keys() and the filter never fires. Replace it with the tracker graph's all_roles() — the current-phase roster installed by concurrent_executor.spawn_active_phase_agents. The graph IS phase-scoped, so cross-phase ghosts in _last_heartbeat (which the singleton HealthMonitor doesn't reset on phase transition) are now actually dropped. When no tracker is registered (early startup, between phases, non-BRC phases) the filter is skipped — preserves the pre-#2242 peer-heartbeat fallback behavior. Update test_gate_filters_inactive_agent_heartbeats to register AGENT_ID_2 via _emit_heartbeat (production state shape) and mock a tracker whose graph excludes it, instead of injecting an impossible state into _last_heartbeat directly.
|
Addressed the active-agent-filter no-op flagged in the prior re-review at Change. Replaced Why this closes the cross-phase pollution case for real. The reviewer's invariant Test update. Docstring. Reconciled with reality — calls out that the filter consults the tracker graph (phase-scoped), why
Pushed as — Authored by egg |
- Lower orchestrator_activity_quiet_seconds Pydantic bound from ge=1 to ge=0 so the documented disabled-gate path is reachable through normal config (operator escape hatch). Remove the contradictory test that claimed to exercise the disabled path while actually exercising the stale-activity path; replace it with a real ge=0 test. - Drop the forward :func: reference to _has_recent_peer_progress from the _has_recent_activity docstring (the symbol lives in unmerged PR #2268). Reword to describe the gate as standalone and note the forward-looking peer-progress half explicitly. - Promote the 0.0 sentinel to a module-level _NEVER_SEEN_ACTIVITY constant so the magic-number compare in _has_recent_activity reads obviously. - Add a deterministic post-hoc guard in decide_corrective_action that downgrades restart_agent -> nudge on a first-occurrence stuck / needs_help classification. The decision-maker prompt now leads with inspect-logs guidance, but the prompt is advisory; the guard is the load-bearing enforcement that prevents the destructive recommendation from reaching agents mid-pytest. Threaded redirect_history through Monitor's two call sites. - Drop the no-op self-rebinding monkeypatch in test_register_publishes_container_activity. The lazy import inside _publish_container_activity means patching the events module is sufficient.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of 19f78b9 (PR-author delta = f371318)
The PR-author delta since my prior review at 3fcec480 is one commit, f371318 — addresses the no-op filter flagged in the prior re-review at 76c3aac. The other commit (19f78b9) is a clean merge from main; nothing in the merged main commits touches the gate path.
What f371318 changes
health_monitor.py:_has_recent_peer_progress— replacesset(self._agents.keys())withtracker.graph.all_roles(). When no tracker is registered the filter is skipped (active_role_setstaysNone→ preserves the pre-#2242 "any known peer heartbeat defers" fallback).- Active-role lookup is nested inside the tracker outer-try with its own inner try/except so a graph-access failure logs and falls open (no filter) without poisoning the bus-signal check.
- Test
test_gate_filters_inactive_agent_heartbeatsnow registers AGENT_ID_2 via_emit_heartbeat(real production state shape — both_agentsand_last_heartbeatget populated) and mocks a tracker whosegraph.all_roles()returns only{AGENT_ID}. The earlier directmonitor._lockinjection that constructed an impossible state is gone.
Verified correctness
- The graph IS phase-scoped.
concurrent_executor.spawn_all()(the actual method, notspawn_active_phase_agents— see suggestion below) constructs a freshReviewGraphper phase via_get_review_graph()→get_review_graph_for_phase(self.pipeline.current_phase.value, repo=...), thencreate_peer_consensus_tracker(pipeline_id, graph, ...)overwrites the registered tracker entry.tracker.graph.all_roles()is therefore the current-phase roster as claimed. - The
_last_heartbeat.keys() ⊆ _agents.keys()invariant flagged in the prior review still holds, but it no longer matters — the filter now consults a structure that is genuinely phase-scoped, not the singleton heartbeat ledger. - The test is a real regression test for the cross-phase pollution case. Without the new filter, AGENT_ID_2's 50s-old heartbeat (within the 300s gate) would defer AGENT_ID's stalled-heartbeat alert; with the filter, AGENT_ID_2 is dropped and the alert fires. Trace the timing: AGENT_ID_2 hb at
base+200, check atbase+250, gate window 300s, AGENT_ID elapsed 250s vs 60s threshold. - Test isolation works because
_emit_heartbeattriggers_on_progress→_get_or_create_agentwrites both_agentsand_last_heartbeat. Production state shape exercised. - The other gate tests (
test_heartbeat_alert_defers_when_peer_heartbeat_fresh,test_gate_disabled_when_seconds_zero,test_gate_handles_missing_tracker_gracefully,test_gate_defers_when_brc_bus_active) all stay green under the new code because:- The first three don't register a tracker →
active_role_setstays None → filter skipped → pre-#2242 fallback applies, which is the behavior they exercise. test_gate_defers_when_brc_bus_activetriggers the earlyreturn True, "BRC bus active …s ago"branch before the active-role lookup runs, so the mock'sgraph.all_roles()is never called and the unsetMagicMockreturn value is never coerced throughset(...).
- The first three don't register a tracker →
Non-blocking
1. Docstring/comment typo: spawn_active_phase_agents → spawn_all
health_monitor.py:271 and health_monitor.py:306 both reference concurrent_executor.spawn_active_phase_agents. That method does not exist — git log --all --grep="spawn_active" only finds this commit. The actual phase-spawn entry point is concurrent_executor.ConcurrentExecutor.spawn_all (concurrent_executor.py:349), which is what routes/pipelines.py:10710 calls per phase.
Suggested fix:
- installed by :func:`concurrent_executor.spawn_active_phase_agents`
+ installed by :func:`concurrent_executor.ConcurrentExecutor.spawn_all`- # ``concurrent_executor.spawn_active_phase_agents``), so
+ # ``ConcurrentExecutor.spawn_all``), soThe substantive claim (graph is phase-scoped) is correct; only the symbol name is wrong. This will mislead anyone trying to navigate from the docstring to the actual phase-spawn code.
2. tracker.graph.all_roles() is read without the tracker _lock
PeerConsensusTracker.excuse_reviewer / excuse_producer mutate self.graph._edges / _producer_roles / _reviewer_roles under self._lock. The new code reads tracker.graph.all_roles() without acquiring that lock, so a concurrent excuse path could see partially-rebuilt sets (remove_edge does self._producer_roles = {…} after popping the edge — atomic assignment, but the two-line _producer_roles = …; _reviewer_roles = … sequence is not atomic together). Worst case is a transient stale or mid-update view: one cycle late on dropping a heartbeat, or one cycle early on including one — won't crash, won't deadlock.
Worth flagging because _check_brc_progress_gate sidesteps this by taking the role list as a caller argument (the live-execution roster), not by reading the graph live. If you want symmetric semantics, either snapshot tracker.graph.all_roles() under tracker._lock or pass an active-role set into _has_recent_peer_progress from above. Not blocking — excuse paths are HITL-gated and rare.
3. Empty-graph edge case
If the tracker exists but the graph has no roles (e.g. a degenerate BRC pipeline, or a transient mid-construction state), active_role_set = set() filters out every peer heartbeat. The gate will not defer; alerts fire. Probably the right semantic ("no roles → nothing legitimate to defer for") but worth confirming you're OK with it — the previous no-op filter would have always deferred in this case.
4. Carry-over from prior reviews
Unchanged and still non-blocking:
- BRC-bus self-deferral docstring at
health_monitor.py:245-259honestly reconciles the trade-off. - Sliced-pipeline scope:
get_peer_consensus_tracker(self._pipeline_id)is called withoutslice_id, acknowledged in docstring athealth_monitor.py:290-292. - Same-role cross-phase pollution (
coderreappearing across implement / implement-fix) still tracked in the docstring's TODO; needs phase-stamped heartbeat keys.
CI / merge
Latest HEAD 19f78b90 — merge state clean against main. Author reports pytest orchestrator/tests/test_health_monitor.py orchestrator/tests/test_two_tier_integration.py orchestrator/tests/test_brc_confirmation_nudge.py orchestrator/tests/test_pipelines_routes.py → 156 passed; I haven't re-run locally but the test logic is sound on read-through.
No new blocking issues. Re-approving with the docstring-symbol-name fix as the only actionable suggestion.
— Authored by egg
|
egg review completed. View run logs 24 previous review(s) hidden. |
| egg is addressing review feedback... |
The reviewer flagged that concurrent_executor.spawn_active_phase_agents does not exist — the actual phase-spawn entry point is ConcurrentExecutor.spawn_all (concurrent_executor.py:349). Update both references in health_monitor._has_recent_peer_progress to point at the real method so docstring navigation lands at the right symbol.
There was a problem hiding this comment.
Addressed the actionable suggestion from the prior re-review.
Suggestion 1 (docstring symbol typo). Fixed in 9b485c3. Both health_monitor.py:271 and health_monitor.py:306 now point at ConcurrentExecutor.spawn_all — the actual phase-spawn entry point at concurrent_executor.py:349. The substantive claim (the graph is phase-scoped because the tracker is overwritten per phase) is unchanged; only the symbol name was wrong.
Suggestions 2 (read without tracker._lock) and 3 (empty-graph edge case). Acknowledged as non-blocking; leaving as-is.
- For (2): the excuse paths are HITL-gated and rare, and
_check_brc_progress_gatemakes the same choice (it takes the role list as a caller arg rather than reading the graph live). Snapshotting undertracker._lockwould add lock-ordering surface that's not justified by the worst case (one cycle late/early on a heartbeat). - For (3):
active_role_set = set()filters out every peer heartbeat, so on an empty graph the gate falls through to the BRC-bus path (already checked above) and then to theFalse, Nonefallback — which is the right semantic ("no roles registered → nothing legitimate to defer for"). A pipeline with a registered tracker but zero roles is a degenerate state; preferring "alert fires" over "alert defers indefinitely" matches the broader contract.
Suggestion 4 (carry-over). Unchanged and still non-blocking.
pytest orchestrator/tests/test_health_monitor.py → 105 passed. ruff check + ruff format --check clean.
— Authored by egg
|
egg feedback addressed. View run logs 24 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review of 9b485c3 (PR-author delta since 19f78b9)
The single new commit 9b485c3 addresses suggestion 1 from the prior re-review (docstring symbol typo). The other carry-over suggestions (2: tracker _lock for graph.all_roles(), 3: empty-graph edge case, 4: same-role cross-phase pollution) were acknowledged in the feedback-addressing comment with reasoning; leaving them as-is is fine.
Verified
health_monitor.py:271andhealth_monitor.py:306— both references updated.git grep spawn_active_phase_agentsclean across the repo. No other call sites.- The substantive claim (graph is phase-scoped because the tracker is overwritten per phase by
spawn_all) is unchanged and correct:concurrent_executor.py:349constructs a freshReviewGraphandcreate_peer_consensus_trackeroverwrites the registered tracker entry. - Code is identical to
19f78b9apart from the two comment lines; existing tests carry over green.
Non-blocking
1. The new symbol name is also wrong: ConcurrentExecutor does not exist
health_monitor.py:271 now reads :meth:concurrent_executor.ConcurrentExecutor.spawn_all``, and health_monitor.py:306 reads `ConcurrentExecutor.spawn_all`. There is no class named `ConcurrentExecutor` in the codebase.
$ grep -rn "^class ConcurrentExecutor\b\|^class ConcurrentPhaseExecutor\b" orchestrator/
orchestrator/concurrent_executor.py:113:class ConcurrentPhaseExecutor:
The class is ConcurrentPhaseExecutor. spawn_all is a method on it (concurrent_executor.py:349), called from routes/pipelines.py:10710 via an instance typed as ConcurrentPhaseExecutor. The only places that use the literal token ConcurrentExecutor are:
tests/test_concurrent_executor_staging_branch.py:47— a local aliasConcurrentPhaseExecutor as ConcurrentExecutor(the test file's own docstring at:9calls this out: "The spec calls the classConcurrentExecutor; the actual class in [the codebase is] ConcurrentPhaseExecutor").tests/test_removal_validation_1165.py:591— a test classTestConcurrentExecutorFailureHandling(unrelated, no symbol match).
So this fix swapped one fictional symbol for another. Sphinx :meth: will still fail to resolve, and a reader navigating from the docstring will still hit a wall.
Suggested fix:
- installed by :meth:`concurrent_executor.ConcurrentExecutor.spawn_all`
+ installed by :meth:`concurrent_executor.ConcurrentPhaseExecutor.spawn_all`- # ``ConcurrentExecutor.spawn_all``), so
+ # ``ConcurrentPhaseExecutor.spawn_all``), soCarry-over from prior reviews (unchanged, still non-blocking)
- BRC-bus self-deferral docstring at
health_monitor.py:245-259honestly reconciles the trade-off. - Sliced-pipeline scope:
get_peer_consensus_tracker(self._pipeline_id)is called withoutslice_id, acknowledged athealth_monitor.py:290-292. tracker.graph.all_roles()is read withouttracker._lock— symmetric with_check_brc_progress_gate's caller-supplied role list. Excuse paths are HITL-gated and rare.- Empty-graph edge case (
active_role_set = set()filters everything out) — documented as the desired semantic. - Same-role cross-phase pollution still tracked in the docstring TODO; needs phase-stamped heartbeat keys.
CI / merge
PR head 9b485c3. Merge state clean. Author reports pytest orchestrator/tests/test_health_monitor.py → 105 passed; the change is purely a comment edit so functional tests are unaffected.
No new blocking issues. Re-approving with the corrected symbol name as the only actionable suggestion.
— Authored by egg
|
egg review completed. View run logs 24 previous review(s) hidden. |
| egg is addressing review feedback... |
The actual class in orchestrator/concurrent_executor.py is ConcurrentPhaseExecutor (line 113); ConcurrentExecutor does not exist outside test-file local aliases. Both docstring references in health_monitor.py now point at the real symbol so Sphinx :meth: resolves and code-navigation works.
|
egg feedback addressed. View run logs 24 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review of cd48c87d (PR-author delta since 9b485c3)
The single new commit cd48c87d addresses the actionable suggestion from the prior re-review (the symbol name was still fictional after the previous fix swapped one wrong name for another). The other carry-over non-blockers (tracker _lock for graph.all_roles(), empty-graph edge case, same-role cross-phase pollution) were acknowledged in the feedback-addressing comment with reasoning; leaving them as-is is fine.
Verified
health_monitor.py:271andhealth_monitor.py:306— both references updated toConcurrentPhaseExecutor.spawn_all.- The class exists:
orchestrator/concurrent_executor.py:113definesclass ConcurrentPhaseExecutor:, withspawn_allat:349. - Repo-wide grep for
ConcurrentExecutor\breturns only test-local aliases intests/test_concurrent_executor_staging_branch.py(from concurrent_executor import ConcurrentPhaseExecutor as ConcurrentExecutor). No production references to the old/fictional name remain. - Substantive claim (graph is phase-scoped because
spawn_allconstructs a freshReviewGraphand overwrites the registered tracker per phase) is unchanged and correct. - Diff is a pure comment edit; functional tests carry over green from
9b485c3.
Carry-over from prior reviews (unchanged, still non-blocking)
- BRC-bus self-deferral docstring at
health_monitor.py:245-259honestly reconciles the trade-off. - Sliced-pipeline scope:
get_peer_consensus_tracker(self._pipeline_id)is called withoutslice_id, acknowledged athealth_monitor.py:290-292. tracker.graph.all_roles()is read withouttracker._lock— symmetric with_check_brc_progress_gate's caller-supplied role list. Excuse paths are HITL-gated and rare.- Empty-graph edge case (
active_role_set = set()filters everything out) — documented as the desired semantic (alert fires rather than defers indefinitely on a degenerate zero-roles tracker). - Same-role cross-phase pollution still tracked in the docstring TODO; needs phase-stamped heartbeat keys.
CI / merge
PR head cd48c87d. Pure comment edit; no functional impact. Author reports pytest orchestrator/tests/test_health_monitor.py → 105 passed on the prior commit; this delta cannot regress that.
No new blocking issues. Re-approving.
— Authored by egg
|
egg review completed. View run logs 24 previous review(s) hidden. |
OR'd the focal-agent activity gate (#2190, this PR) with the alive-signal peer-progress gate (#2242, just landed in main via #2268) at both alert sites. Either gate firing now defers heartbeat / progress alerts; the escalated flag is intentionally not set on defer so the next poll re-checks once the deferring signal goes stale. Also pulled in main's _get_post_ack_confirmation_timeout helper alongside _has_recent_peer_progress.
…r OR'd gates Test was written assuming peer signals belong solely to #2242 and would not interfere with the per-agent focal-agent gate. After merging #2268 (which OR'd the peer-progress gate into the alert sites), a peer heartbeat from AGENT_ID_2 now defers AGENT_ID's alert via the peer- progress gate. Disable that gate (orchestrator_alert_progress_gate_seconds=0) in this test to isolate the focal-agent gate's per-agent property under test.
* docs: document alive-signal gate and plan post-ACK timeout Update pipeline-health-monitoring.md to reflect changes from #2268: - Add plan-phase override note to Post-ACK Confirmation section - Add new "Alive-Signal Gate" section describing the per-agent alert deferral - Add orchestrator_plan_post_ack_confirmation_timeout_seconds (300s) to config table - Add orchestrator_alert_progress_gate_seconds (300s) to config table Authored-by: egg * Address review: alive-signal gate caveats and dedupe grace row Apply non-blocking suggestions from #2299 review: - Document gate-window-bounded deferral semantics (escalated flag is not set on defer, so each cycle re-evaluates and the alert fires once gate_seconds elapses past the most recent peer signal). - Add caveat for same-role cross-phase pollution (heartbeat keys are not phase-stamped, so a recurring role like coder across implement / implement-fix can pass the active-agent filter with a stale prior-phase heartbeat). - Add caveat for single-producer self-deferral on the BRC-bus path (get_latest_progress_timestamp is not focal-agent-filtered, so on a single-producer pipeline the producer's own propose/ACK defers its own heartbeat alert until gate_seconds elapses; effective stall window is heartbeat_threshold + gate_seconds for hung-process-in-live-container cases — CONTAINER_STOPPED still covers genuine container death). - Replace active-agent filter example (coder is exactly the case the filter does not help) with a non-recurring role (refiner). - Dedupe the post_proposal_grace_seconds config table row (the second occurrence pre-dated this PR but was already in the table being edited); consolidated description matches orchestrator/models.py:467-471. Authored-by: egg --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
…ts (#2283) * Fix #2190: focal-agent activity gate on heartbeat/progress alerts The `agent-heartbeat-stall` and `progress_stall` detectors fired against agents that were correctly executing long-blocking tool calls (e.g. a multi-minute background pytest via `Bash`+`TaskOutput`). The detectors sampled only message-bus `HEARTBEAT` traffic, so any window of tool-call activity without an explicit heartbeat was treated as silence — even when the agent was making git commits, calling `mcp__task__add_commit`, and exchanging tool results. The Tier-2 overseer then escalated to a destructive `restart_agent` recommendation, which would have wiped in-flight commits. Add a focal-agent activity signal to `HealthMonitor`: 1. New `EventType.CONTAINER_ACTIVITY`. Published by `routes/commit_authorship.register_commit` (the gateway commit observer's HTTP target) on every successful, pipeline-scoped commit registration. Best-effort — publish failures never affect the route's response. 2. `HealthMonitor` subscribes, tracks per-agent `last_activity`, and defers `heartbeat_timeout` / `progress_stall` alerts when activity has fired within `orchestrator_activity_quiet_seconds` (default 120s, ge=1). Mirrors #2242's `_has_recent_peer_progress` pattern; the two gates are OR'd — focal activity OR peer progress is enough to defer. The `escalated` flag is intentionally not set on defer so the next poll re-checks once activity goes stale. 3. Soften the overseer's `decide_corrective_action` prompt to direct the LLM toward `mcp__egg__get_container_logs` inspection before recommending `restart_agent` for stall classifications, and forbid embedding `egg-orch container restart <id>` as a first-line operator action. Restartable infrastructure errors (the fast-path in `_is_restartable`) are unaffected. Tests cover the suppression for both `check_heartbeats` and `check_progress`, stale-activity expiry, missing-activity behaviour, cross-pipeline isolation, peer-not-self semantics, and that `register_commit` publishes the event (with the orphan-shard case suppressed to avoid scopeless events). Out of scope (follow-ups): - Gateway → orchestrator webhook for `git_push` / raw `git_execute` activity (requires cross-process plumbing). - Container log harvester so per-tool-call `Read`/`Edit`/`Bash` activity drives the gate. - "Awaiting tool result" inference (last message was a `tool_use` with no matching `tool_result`). - Integration regression spawning a real coder running 5-min background pytest. * Address review feedback on PR #2283 (#2190 activity gate) - Lower orchestrator_activity_quiet_seconds Pydantic bound from ge=1 to ge=0 so the documented disabled-gate path is reachable through normal config (operator escape hatch). Remove the contradictory test that claimed to exercise the disabled path while actually exercising the stale-activity path; replace it with a real ge=0 test. - Drop the forward :func: reference to _has_recent_peer_progress from the _has_recent_activity docstring (the symbol lives in unmerged PR #2268). Reword to describe the gate as standalone and note the forward-looking peer-progress half explicitly. - Promote the 0.0 sentinel to a module-level _NEVER_SEEN_ACTIVITY constant so the magic-number compare in _has_recent_activity reads obviously. - Add a deterministic post-hoc guard in decide_corrective_action that downgrades restart_agent -> nudge on a first-occurrence stuck / needs_help classification. The decision-maker prompt now leads with inspect-logs guidance, but the prompt is advisory; the guard is the load-bearing enforcement that prevents the destructive recommendation from reaching agents mid-pytest. Threaded redirect_history through Monitor's two call sites. - Drop the no-op self-rebinding monkeypatch in test_register_publishes_container_activity. The lazy import inside _publish_container_activity means patching the events module is sufficient. * Address PR #2283 review feedback (egg-reviewer non-blocking items) - A. Drop dead `infrastructure_error` classification dict in `test_non_stall_classification_unchanged`; it was rebound on the next line and exercised nothing. Test now uses `working` only, with the comment trimmed to match. - B. Replace the `try/except TypeError` fallback in `OverseerMonitor._decide_corrective_action` with an `inspect.signature`-based `_accepts_kwarg` helper. A genuine `TypeError` raised inside a custom decision-maker double's body no longer silently re-runs with the legacy signature and drops `redirect_history`. `AsyncMock`-style doubles (signature `(*args, **kwargs)`) still take the new path. - C/D. Route the first-stall override through `hitl` instead of `nudge`. The override message is operator-targeted (it instructs `mcp__egg__get_container_logs` inspection — an operator-only tool), so emitting `nudge` delivered the body to the agent's inbox via `_send_message`. Routing through `hitl` puts the decision in the operator's surface and keeps the agent's inbox clean. The original recommendation is preserved as "Model's recommendation: …" only when non-empty (no dangling trailing colon). - E. Expand the first-occurrence history check to span every intervention type (`nudge`, `redirect`, `restart_agent`, `hitl`), not just non-destructive ones. A prior `restart_agent` no longer fast-tracks the next restart past the guard. The docstring now states the intent explicitly: "ensure at least one non-destructive intervention before destruction." Tests: targeted suites all pass (`test_overseer_decision_maker.py test_overseer_monitor.py test_health_monitor.py test_commit_authorship_routes.py test_restart_overseer.py test_infra_error_escalation.py`, 343 passed). New tests: - `test_first_stall_restart_overridden_to_hitl` - `test_first_stall_restart_with_empty_message_no_dangling_colon` - `test_restart_allowed_after_prior_restart` * Update test_activity_event_for_other_agent_does_not_suppress_focal for OR'd gates Test was written assuming peer signals belong solely to #2242 and would not interfere with the per-agent focal-agent gate. After merging #2268 (which OR'd the peer-progress gate into the alert sites), a peer heartbeat from AGENT_ID_2 now defers AGENT_ID's alert via the peer- progress gate. Disable that gate (orchestrator_alert_progress_gate_seconds=0) in this test to isolate the focal-agent gate's per-agent property under test. * Address non-blocking review items A & B on PR #2283 Item A: Expand _PRIOR_INTERVENTIONS to span every action in the decision-maker vocabulary — adds issue, slack, and restart_phase to the existing nudge / redirect / restart_agent / hitl set. Promoted to a module-level frozenset constant for testability and discoverability. Updated the docstring to align with the implemented behavior. New parametric test pins each of the three newly-covered action types. Item B: Add four-case test class TestAcceptsKwarg covering the explicit-kwarg path, the **kwargs catch-all path, the legacy no-kwarg path, and the introspection-failure fallback (using int, which raises ValueError under inspect.signature on CPython). The fallback branch was previously only covered by manual smoke. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
Summary
check_heartbeats,check_progress). Before firingheartbeat_timeout/progress_stall, consultPeerConsensusTracker.get_latest_progress_timestamp()plus peer-heartbeat snapshots; defer the alert if either has fired withinorchestrator_alert_progress_gate_seconds(default 300s, 0 disables).orchestrator_plan_post_ack_confirmation_timeout_seconds; refine / implement keep the existing 180s default.Why
On
issue-1557-v2the plan phase escalated a 3-of-3 producer-silence alert at 355s while every producer was simply composing its draft — the long Anthropic completion has no in-flight tool call, somcp__brc__send_heartbeatdoesn't arrive on the bus. Separately, the 180s post-ACK confirmation timeout was tight for plan-phase reconciliation (resolved decisions, feedback bodies, slice-DAG sanity).Both alerts are correct in their genuine-stall failure mode; the issue is calibration for heavy plan phases. The alive-signal gate reuses the vocabulary shipped in #2254 — a peer heartbeat or BRC-bus event (
CONSENSUS_PROPOSE/ ACK / NACK) within the gate window keeps the broader pipeline judged "alive" even when one producer is silent mid-completion.Out of scope (follow-ups under #2059)
OVERSEER_ALERTmessages.The same-role cross-phase pollution caveat documented in the existing
_check_brc_progress_gateTODO applies here too — a phase-stamped heartbeat key would close both at once.Test plan
pytest orchestrator/tests/test_health_monitor.py— 104 tests pass, including 11 new cases covering: peer-fresh defer, BRC-bus-fresh defer, gate-window-elapsed proceed, self-exclusion, gate-disabled bypass, progress-stall path, missing-tracker fallback, phase-aware post-ACK threshold default + override + behavior.pytest orchestrator/tests/test_brc_confirmation_nudge.py orchestrator/tests/test_pipelines_routes.py— 26 tests pass; the existing_check_brc_progress_gatecallers are unaffected.ruff check+ruff format --checkclean on the three changed files.