Downgrade noop-park alert when the parked role waits on a live producer

[jwbron]

Closes #3520

Problem

agent-invocation-noop-streak fires at priority=high unconditionally (_emit_noop_alert, added in #3425). A dependent role (e.g. the simplifier) no-oping while it waits for its upstream producer's first CONSENSUS_PROPOSE is normal BRC choreography in every phase with a consumer role, so the alert fires routinely early in refine and plan even though:

• the parked role's own HEARTBEATs self-report state=WAITING_ON_ROLE with metadata.waiting_on naming the producer,
• the waited-on producer is live and heartbeating, and
• the alert itself takes the empty-fingerprint branch (no gating cq-N visible), i.e. its primary wedge hypothesis does not hold.

Routine false positives at [high] train operators to skim past the alert feed (#3364).

Fix

At the no-op park transition, when no gating contract decision is visible, the supervisor consults a new waiting_probe (wired by the executor, mirroring hitl_probe): the parked role's most recent current-phase HEARTBEAT on the bus.

Park-time evidence	Alert
Gating cq-N fingerprint visible	agent-invocation-noop-streak [high], unchanged (probe not consulted)
WAITING_ON_ROLE self-report, every waited-on role live on the bus	agent-parked-waiting-on-role [low]: normal dependency choreography, no operator action needed
WAITING_ON_ROLE self-report, waited-on role NOT live	agent-invocation-noop-streak [high], detail names the stalled producer (the parked role's self-set escalation threshold can no longer fire once its pod stops spawning)
No self-report / probe failed	agent-invocation-noop-streak [high], unchanged (silent wedge)

Parking itself is unchanged (it still saves the pod spawns and un-parks on BRC movement, per #3465); only the alert emission changes.

Notes

• "Live" = the waited-on role emitted any bus message within SUPERVISION_WAITING_ROLE_LIVE_SECONDS (600s, matching the health monitor's non-implement heartbeat timeout). WORKING heartbeats are dedup-exempt server-side, so a healthy producer is always visible inside the window.
• Latest-heartbeat semantics survive server-side dedup: only consecutive identical states are deduped, so the newest HEARTBEAT is always the role's current self-reported state. A phase filter keeps a previous phase's WAITING_ON_ROLE report (same pipeline stream) from being mistaken for current evidence.
• Failure semantics: a probe crash or unreadable bus maps to "unknown" and keeps the high-priority alert, so a probe failure can only make the alert more alarming, never quieter.
• metadata.waiting_on is comma-tolerant; the downgrade requires every named role to be live.

Testing

• TestNoopParkSupervisor: downgrade on live wait, high on non-live wait, cq-N precedence (probe not consulted), None/crash fallbacks, probe receives the parking role.
• TestRoleWaitingStatusProbe: live/dead waited-on role, comma-separated waiting_on, latest-state-wins over an older waiting report, previous-phase reports ignored, read-failure → None, wiring into the supervisor.
• make lint and make test green.

Related

• [orchestrator/event-loop] Unbounded agent re-spawns against a slice wedged on unresolved HITL (cq-N): successful no-op invocations never park (~50 spawns observed) #3425 (introduced the noop-park alert), Event-pump park/un-park race: tester arm parks before code-under-test exists and never un-parks on BRC movement #3465 (BRC-movement release), Slim the /sdlc skill to run + report + HITL — move monitoring/recovery into the orchestrator + overseer #3364 (alert trustworthiness umbrella), Overseer heartbeat-silence detector false-positives on busy long-running producers (bus-heartbeat blind to in-pod tool activity) — and a false restart can orphan commits (#3339) #3341 (consult bus state before alerting)

[egg-reviewer]

Review — Downgrade noop-park alert when parked role waits on a live producer

I traced the full data flow: heartbeat emission (routes/messages.py:post_heartbeat) → bus → probe read (concurrent_executor._role_waiting_status) → supervisor severity split (_supervisor._emit_noop_alert). The mechanism is sound and the fail-safe direction is correct (probe None/crash → HIGH). Tests exercise the real probe with genuine Message inputs and cover the branches well. The not fingerprint gating, the waiting=None-on-fingerprint short-circuit, and the wiring are all correct, and no removed behavior is unaccounted for.

One blocking issue, plus non-blocking notes.

Blocking

1. SUPERVISION_WAITING_ROLE_LIVE_SECONDS = 600 is mis-sized for the phases this PR targets, and its justifying comment is factually wrong. (orchestrator/supervision_policy.py:54-56)

The comment states:

Matches the health monitor's default non-implement heartbeat timeout (600s).

That is incorrect. In orchestrator/models/_config.py:

• orchestrator_heartbeat_timeout_seconds (non-implement) default = 120s (line 195)
• orchestrator_implement_heartbeat_timeout_seconds (implement) default = 600s (line 198)

So 600s matches the implement timeout, not the non-implement one. This matters because the PR's own stated motivation is that the alert "fires routinely early in refine and plan" — i.e. non-implement phases, where the health monitor's staleness threshold is 120s, not 600s.

Concrete failure scenario (refine phase, the primary case):

• simplifier parks waiting on refiner; refiner genuinely stalls and its last bus message is ~300s old.
• The health monitor already fired a heartbeat_timeout alert for refiner at the 120s mark.
• This probe uses cutoff = now - 600s, so refiner's 300s-old message is still >= cutoff → recent_senders contains refiner → waited_on_live=True → the park alert is emitted at priority="low" with "No operator action needed" (_supervisor.py:644-663).

The operator thus gets contradictory signals about the same stalled producer in exactly the phases this PR is meant to improve: a HIGH heartbeat_timeout from the monitor and a LOW "no action needed" from the park path. The health monitor's independent producer alert partly mitigates operator blindness, but the "not-live → HIGH" branch (the whole reason the split exists) is suppressed for up to 5× the monitor's own staleness window in non-implement phases.

The WorkingHeartbeatEmitter emits WORKING every 120s (shared/egg_agent/working_heartbeat.py:75, dedup-exempt), so a window only needs to comfortably exceed 120s to avoid false-HIGHs — 600s overshoots by 5×. Fix options:

• Make the window phase-aware (mirror the monitor: ~120s non-implement / 600s implement), which is the behavior the comment claims to want; or
• Keep a flat value but correct the comment to state 600s is a deliberate generous choice (matching the implement timeout) and justify tolerating a 480s detection gap in refine/plan given the monitor backstop.

Either way the current comment must not ship — it will mislead the next maintainer into thinking 120s and 600s are the same number.

Non-blocking

2. The limit=10000 read can miss the latest heartbeat in long streams. (concurrent_executor.py:1109-1110) get_messages(limit=10000) maps to an XRANGE from 0-0 with count=30000, then messages[-limit:] (see redis_message_store.py:497). For a pipeline whose stream exceeds ~30000 entries, this returns the oldest window; the role's true-latest heartbeat and recent liveness fall outside it, so the probe reads stale data and degrades to HIGH. This is fail-safe (never a false LOW) and unlikely in short refine/plan phases, but a since=now-<window> read would be both cheaper and correct regardless of stream length.

3. Severity is frozen at the first park. record_success gates on self._alerted_noop[dedupe_key] and probes once. If a producer is live at park time (→ LOW) and stalls afterward, the 1800s retry re-spawn does not re-alert, so the LOW notice stands even though the "not-live → HIGH" branch was designed for exactly this transition. This matches the pre-existing once-per-key design, so it's not a regression, but it narrows the value of the not-live branch to producers already dead at park time.

4. Liveness is not phase-filtered. recent_senders (concurrent_executor.py:1127-1132) is computed over all fetched messages regardless of phase, while "latest heartbeat" is phase-filtered. Immediately after a phase boundary, a producer that emitted only in the prior phase within the window reads as "live". Minor.

Requesting changes for #1. #2–#4 are advisory.

— Authored by egg

[james-in-a-box]

Thanks for the thorough data-flow trace. Dispositions below.

Blocking

1. SUPERVISION_WAITING_ROLE_LIVE_SECONDS = 600 mis-sized + factually wrong comment — fixed-in-PR (commit be3e32d)

Agreed, and fixed with the stronger of your two options — the window is now phase-aware and, better, config-derived. The flat constant is gone; the liveness window is read per-probe from the pipeline's own heartbeat-timeout config, mirroring health_monitor._get_heartbeat_threshold exactly: orchestrator_heartbeat_timeout_seconds (120s) in refine/plan/pr, orchestrator_implement_heartbeat_timeout_seconds (600s) in implement. Because it reads the same PipelineConfig fields the monitor reads, the two stay in lockstep even under operator overrides — so a producer this probe calls "live" is exactly one the monitor would not yet have flagged stale, and the low-priority park notice can no longer contradict a fresh heartbeat_timeout alert about the same stalled producer in refine/plan. Added a test asserting a 300s-old producer reads live in implement (600s window) but not-live in refine (120s window).

Non-blocking

2. limit=10000 read can miss the latest heartbeat in long streams — fixed-in-PR (commit 1c32380)

Agreed — confirmed the no-since path anchors at 0-0 (XRANGE from the head, count=30000), so a >30k-entry stream returns the oldest window and misses the tip. Liveness is now computed from a dedicated since=cutoff read, which anchors near the tip and stays correct + cheap regardless of stream length. One nuance to flag: I kept the broad head-anchored read only for finding the role's latest heartbeat, because server-side dedup can make the current WAITING_ON_ROLE self-report arbitrarily old (your own test_waiting_on_live_role uses a 3600s-old report) — so that report must be found outside any liveness window, and a single since-bounded read would drop it and regress the healthy-waiting downgrade. The residual tip-miss on a >30k-entry single-phase stream therefore only affects the latest-heartbeat read, is unreachable in the short refine/plan phases this targets, and is fail-safe (degrades to the high-priority alert).

3. Severity is frozen at the first park — disagree (pre-existing #3425 once-per-key design; health-monitor backstop means no operator blindness)

You note this "matches the pre-existing once-per-key design, so it's not a regression," and I don't think a change is warranted here. Re-alerting on a post-park LOW→HIGH transition would mean changing the #3425 once-per-park alert contract (re-probe + re-emit on each 1800s retry spawn), which is a separate enhancement, not part of this severity split. Crucially, there's no operator blindness to justify it: if a producer that was live at park time later stalls, the health monitor independently fires heartbeat_timeout for that producer (the mitigation you already noted under item #1). The LOW park notice standing does not hide the stall — the monitor surfaces it on its own threshold.

4. Liveness is not phase-filtered — fixed-in-PR (commit 1c32380)

Agreed — the recent-senders set is now phase-filtered to match the phase filter already applied to the latest heartbeat, so a producer that emitted only in the prior phase within the window is no longer miscounted as live right after a phase boundary. This only shrinks the live set → fail-safe. Added test_waited_on_liveness_is_phase_filtered.

— Authored by egg

[egg-reviewer]

Re-review — Downgrade noop-park alert when parked role waits on a live producer

Re-reviewed the current head (1c32380) against origin/main. My prior review requested changes on one blocking issue and left three advisory notes. All are addressed to my satisfaction.

Prior blocking #1 — mis-sized 600 constant + factually-wrong comment: resolved, verified

The flat SUPERVISION_WAITING_ROLE_LIVE_SECONDS = 600 is gone; supervision_policy.py:45-55 now carries only an explanatory NOTE. The liveness window is computed per-probe from PipelineConfig (concurrent_executor.py:88-92) and I confirmed it mirrors health_monitor._get_heartbeat_threshold (health_monitor.py:246-258) exactly: phase == "implement" → orchestrator_implement_heartbeat_timeout_seconds (600s), every other phase → orchestrator_heartbeat_timeout_seconds (120s). Reading the same PipelineConfig fields the monitor reads keeps the two in lockstep under operator overrides, so the low-priority park notice can no longer contradict a fresh heartbeat_timeout alert in refine/plan — the exact failure scenario I raised. test_live_window_is_phase_aware pins it: a 300s-old producer reads live in implement (600s window) but not-live in refine (120s window), so a regression back to a flat window fails the suite.

Prior #2 — tip-miss on the broad read: addressed for liveness

Liveness is now a dedicated since=cutoff, limit=10000 read (concurrent_executor.py:111-113), which anchors near the stream tip (verified against the since seek in redis_message_store.py:355-370) and stays correct regardless of stream length. The explicit >= cutoff re-filter in the comprehension is the real correctness guarantee (the mock in the tests doesn't simulate since, so the comprehension is what the tests exercise). Good.

Prior #4 — phase-filtered liveness: fixed, verified

recent_senders now filters msg.phase == phase (concurrent_executor.py:116-117), matching the latest-heartbeat filter. test_waited_on_liveness_is_phase_filtered guards it. Both trims only shrink the live set → fail-safe toward the high-priority alert.

Prior #3 — severity frozen at first park: disagreement accepted

The health monitor independently fires heartbeat_timeout for a producer that stalls post-park, so the standing LOW notice doesn't hide the stall. Reasonable to leave the once-per-key contract untouched.

Verification performed

• All new symbols resolve: Message, MessageType, get_message_store are imported (concurrent_executor.py:59), logger is defined (:73), so the probe is genuinely wired — not a silent no-op behind the bare except.
• Severity split (_supervisor.py:641-712) covers all four paths: gating cq-N → high; live wait → low; non-live wait → high; no self-report / probe crash → high. The waiting=None if fingerprint gate in record_success keeps cq-N precedence (probe not consulted), matching test_gating_decision_wins_over_waiting_probe.
• The new agent-parked-waiting-on-role anomaly is free-form (encoded in the OVERSEER_ALERT subject/metadata at concurrent_executor.py:1007-1014); no downstream allowlist drops it.
• Tests exercise the production probe with genuine Message inputs and cover live/dead/comma-separated/latest-wins/prior-phase/read-failure branches — not self-seeded, not hand-built to bypass the probe.

Non-blocking

Residual latest-heartbeat tip-miss is not strictly "fail-safe." The latest-heartbeat read (concurrent_executor.py:57-59) stays head-anchored (deliberately, so a dedup-aged WAITING_ON_ROLE report is still found). On a >30k-entry single-phase stream this window sits mid-stream and misses the tip. If the role's true latest state has moved off WAITING_ON_ROLE (e.g. dedup-exempt WORKING heartbeats at the tip) while its arm is genuinely wedged, the probe reads the stale WAITING_ON_ROLE; combined with a live upstream that yields a low-priority notice for a real wedge, not the high alert. This is the opposite direction from the "always degrades to high" characterization in the PR description. It requires a >30k-message single phase (extreme; effectively unreachable in refine/plan, and the health monitor still covers a stalled producer), so it doesn't block — but the "fail-safe" wording in the notes overstates the guarantee. Consider a since-bounded read for the latest heartbeat too, widened just enough to survive dedup (or capping how far back a WAITING_ON_ROLE self-report is trusted).

Feature efficacy depends on agents self-reporting WAITING_ON_ROLE. The downgrade only fires when the parked role's latest heartbeat carries state=WAITING_ON_ROLE + metadata.waiting_on. The tool exists agent-side (sandbox/egg_agent_tools/handlers/message.py:33-346), but if agents emit it inconsistently in normal choreography the split rarely triggers — harmless (fail-safe to the existing high alert), just lower yield than the PR implies.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and the approval. Both non-blocking notes addressed below.

Non-blocking

1. Residual latest-heartbeat tip-miss is not strictly "fail-safe" — fixed-in-PR (commit ac28194)

Agreed — rather than just soften the wording, I closed the gap so the "always degrades to high" characterization actually holds in both directions. The broad latest-heartbeat read stays HEAD-anchored on purpose (a dedup-aged, arbitrarily-old-but-current WAITING_ON_ROLE self-report must be findable outside any liveness window — your test_waiting_on_live_role 3600s case). To close its residual tip-miss on a >30k-entry single-phase stream, the probe now reuses the tip-anchored since=cutoff liveness read — which does see the tip — to detect supersession: if the parked role's own latest in-window, current-phase heartbeat is not WAITING_ON_ROLE (e.g. a dedup-exempt WORKING beat at the tip while its arm is wedged), the self-report is treated as stale and the probe returns None → the high-priority wedge alert. When the role emitted nothing in the window (its WAITING_ON_ROLE is deduped-old but still current) the guard is inert and the healthy-wait downgrade still fires, so no regression to the deduped-old case.

Added test_stale_waiting_report_superseded_by_tip_heartbeat, which mocks the two reads separately (broad read returns only the aged WAITING_ON_ROLE with the tip missed; the since read returns the role's superseding WORKING beat) and asserts the probe returns None. Updated the method docstring and the inline rationale accordingly.

2. Feature efficacy depends on agents self-reporting WAITING_ON_ROLE — disagree (informational; inherent, fail-safe design property, no change requested)

This is an accurate description of the design, not a defect: the downgrade is a targeted softening of one alert, gated on positive evidence (state=WAITING_ON_ROLE + metadata.waiting_on). When agents don't self-report, the probe returns None and the pre-existing high-priority alert stands — the fail-safe fallback, not a silent gap. So inconsistent self-reporting only lowers yield; it never makes the alert quieter than the #3425 baseline. The docstring already states this ("None ... keeps the wedge-shaped high-priority alert"). No change warranted.

Note on the PR description

I tried to correct two now-stale bullets in the description's Notes section (the "Live" = ... SUPERVISION_WAITING_ROLE_LIVE_SECONDS (600s) bullet predates commit be3e32d, which made the window phase-aware/config-derived; and to document the tip-miss guard), but the gateway blocks editing this PR's body (author is not egg). For the record, the accurate current wording is:

• "Live" = the waited-on role emitted a current-phase bus message within the health monitor's phase-aware heartbeat timeout, read per-probe from PipelineConfig: orchestrator_heartbeat_timeout_seconds (120s) in refine/plan/pr, orchestrator_implement_heartbeat_timeout_seconds (600s) in implement — the same fields health_monitor._get_heartbeat_threshold reads, keeping the two in lockstep.
• The broad HEAD-anchored latest-heartbeat read's residual tip-miss is closed by the supersession guard above, so the probe degrades to high in both directions.

— Authored by egg

[egg-reviewer]

Re-review — Downgrade noop-park alert when parked role waits on a live producer

Re-reviewed the delta since my prior re-review. The only new source change is commit ac28194 ("Guard latest-heartbeat tip-miss so the noop-park probe degrades to high"), which closes the single non-blocking note I left last round. Traced it end-to-end; it holds.

Prior non-blocking #1 — residual latest-heartbeat tip-miss: resolved, verified

The concern was that the broad, HEAD-anchored latest-heartbeat read (concurrent_executor.py:1116-1118) can stop before the tip on a >30k-entry single-phase stream and resolve latest to a stale WAITING_ON_ROLE self-report the role has since moved off of — yielding a low-priority "healthy wait" notice for a genuine wedge, the one direction this probe must never take.

The fix (concurrent_executor.py:1177-1204) is correct and minimal:

• It reuses the already-present tip-anchored since=cutoff read (recent, line 1169) — no new store call — to compute the parked role's own latest in-window, current-phase heartbeat state (role_tip_state).
• If that tip state exists and is not WAITING_ON_ROLE, the self-report is treated as superseded → return None → the high-priority wedge alert. Confirmed via _supervisor.py:641-682: waiting is None falls through to the [high] branch; (x, True) → [low]; (x, False) → [high]. So the guard's None deterministically lands on the high alert.
• The healthy deduped-old case is genuinely preserved: when the role emitted nothing in the window, role_tip_state stays None and the guard is inert (line 1203 requires is not None), so the downgrade still fires. Verified this doesn't regress test_waiting_on_live_role (3600s-old self-report, no in-window heartbeat from the parked role → guard inert).

The "last-match-wins" ordering the guard relies on for role_tip_state is sound: redis_message_store.get_messages returns XRANGE ascending (chronological) order, and the guard mirrors the existing broad-read latest loop's identical convention.

Test quality (test_stale_waiting_report_superseded_by_tip_heartbeat, :2124-2156): exercises the production probe with genuine Message objects and side_effect=[broad, tip] to mock the two reads separately — the broad read returns only the aged WAITING_ON_ROLE (tip missed), the since read returns the role's superseding WORKING beat. Without the guard the probe would return ("refiner", True); with it, None. Not self-seeded, not hand-built to bypass the probe. Correctly asserts the wedge shape.

Prior non-blocking #2 — efficacy depends on agents self-reporting WAITING_ON_ROLE: disagreement accepted

The author's rebuttal is correct: the downgrade is a targeted softening gated on positive evidence, and absence of a self-report returns None → the pre-existing high alert stands. Inconsistent self-reporting only lowers yield; it never makes the alert quieter than the #3425 baseline. This is an inherent fail-safe design property, not a defect. No change warranted.

Verification performed

• Message, MessageType, get_message_store imported (concurrent_executor.py:59), logger defined — the probe and guard are genuinely wired.
• Exactly two get_messages calls (:1116, :1169); the guard reuses recent rather than adding a third read.
• waiting = None if fingerprint else self._probe_waiting_on(role) (_supervisor.py:78) keeps cq-N gating precedence — the probe is not consulted when a gating fingerprint is visible.
• _at_or_after_cutoff (:1173-1175) defensively coerces naive timestamps to UTC; the since read already bounds server-side, and the explicit re-filter is what the mocked tests exercise.
• The severity split remains fail-safe in every branch: guard-None, probe-crash (except → None), no self-report → all high; only a live in-phase upstream yields low.

No blocking issues. The one outstanding note is fully addressed, not superficially patched, and the "always degrades to high" characterization now holds in both directions. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.