Fix #2068: BRC heartbeat refreshes gateway session liveness

[jwbron]

Summary

• Adds POST /api/v1/sessions/by-container/<id>/heartbeat (launcher-authed) on the gateway, mirroring the existing delete_session_by_container trust pattern.
• Adds matching SessionManager.heartbeat_session_by_container and GatewayClient.heartbeat_session_by_container helpers.
• Wires the orchestrator's /api/v1/pipelines/<id>/heartbeat route to fan out a best-effort refresh to the gateway on every non-deduped, non-rate-limited HEARTBEAT, so a producer in WAITING_FOR_EVENT no longer ages out of the gateway's 60-minute idle window during a long review cycle.

Why

A BRC producer in WAITING_FOR_EVENT heartbeats only through the orchestrator MCP (mcp__brc__send_heartbeat), so the gateway sees zero traffic from that container even though the agent is alive. After 60 minutes the gateway's idle pruner evicts the session, and the next git/gh op (e.g. committing a finished v2 plan, opening a PR) fails with 401. The agent has no in-sandbox refresh path. See #2068 for the full repro from pipeline issue-1962.

Test plan

• gateway/tests/test_session_manager.py::TestHeartbeatByContainer — refresh path advances last_seen, unknown container returns False, expired sessions are not silently revived
• gateway/tests/test_gateway.py::TestSessionHeartbeatByContainer — endpoint refreshes a session, returns 404 on miss, requires launcher auth
• orchestrator/tests/test_gateway_client.py::TestSessionManagement — new client method calls the endpoint, swallows 404
• orchestrator/tests/test_messages.py::TestHeartbeatRoute — HEARTBEAT fans out to the gateway with the right egg-agent-{pipeline}-{role} container_id; gateway failures don't fail the heartbeat
• Full focused suite (gateway/tests, orchestrator/tests/test_gateway_client.py, orchestrator/tests/test_messages.py) — 473 passed
• Verify in a live pipeline by leaving a producer in WAITING_FOR_EVENT for >60 min and confirming the next gateway op still authenticates

🤖 Generated with Claude Code

[egg-reviewer]

Reviewed the full diff and traced the heartbeat path end-to-end. The fix correctly addresses #2068's WAITING_FOR_EVENT scenario, container-id construction matches kubernetes_spawner.JOB_NAME_FORMAT, the launcher-auth pattern mirrors delete_session_by_container, and all 11 new tests pass locally. Good test coverage for the happy path, expired-session guard, missing-container, auth, and orchestrator fan-out failure modes.

A few non-blocking observations:

Non-blocking

1. Disk persistence is more aggressive than validate_session's in-flight extend
gateway/session_manager.py:944-967 calls _save_to_disk() after every extend_ttl(). The existing validate_session path (session_manager.py:683-684) extends TTL on every successful request without persisting — only token-cache repopulation happens, the disk write is deferred to deletion/expiry/idle-prune events. With ~5 agents in a pipeline beating every 60s through _default_emit_wait_loop_heartbeat (sandbox/egg_agent_tools/handlers/message.py:44), this is one full O(N) atomic file write every ~12s per pipeline. That's not catastrophic, but it's a ~10× increase in write frequency vs. the existing pattern for the same TTL-extension semantics. Consider matching validate_session's "extend in-memory, persist on lifecycle events" pattern — the worst case if the gateway dies between persistence is the session is evicted and the next gateway op re-registers, which is the same recovery path you already have for validate_session extends.

2. Client method silently swallows failures, unlike its sibling
orchestrator/gateway_client.py:591-599 catches GatewayError with no log. Compare delete_session_by_container (lines 568-574) which logs a warning with the container_id and error. When this fan-out fails (which is the exact symptom of #2068), the only log will be the orchestrator route's _refresh_gateway_session warning, but that only fires for non-GatewayError exceptions because the client swallows those. So a 500/timeout from the gateway leaves no breadcrumb at all. Add a logger.warning(...) in the except GatewayError to match the sibling — debuggability matters for a "best-effort" path.

3. Coverage gap for non-WAITING_FOR_EVENT long compute
The fan-out runs after the dedup gate (routes/messages.py:518), and only WAITING_FOR_EVENT is in _DEDUP_EXEMPT_HEARTBEAT_STATES. An agent in WORKING for >60min that doesn't make gateway calls (e.g. a long pure-CPU test suite) emits identical WORKING heartbeats that get deduped after the first, so the gateway session ages out the same way. This is exactly out of scope for #2068's bug, but worth flagging — the fix is asymmetric and the next #2068-style report will probably be WORKING during a slow make test. Either document the gap or move the fan-out above the dedup check (the gateway can absorb 1 cheap POST per beat far more easily than it can absorb a bogus 401-cascade after eviction).

4. test_heartbeat_extends_ttl is essentially tautological
gateway/tests/test_session_manager.py:809-822 registers the session, captures original_expiry = now + 24h, then sets expires_at = (now-30min) + 24h = now + 23.5h, then heartbeats which sets expires_at = now + 24h, then asserts refreshed > original - 1s. The original was now + 24h and the refreshed is now + 24h so the assertion passes by ≈0s margin regardless of whether extend_ttl fired. Stronger: backdate both last_seen and expires_at to now - 30min and now + 23.5h, then assert refreshed_session.expires_at > now + 23.9h — that actually proves TTL moved forward.

5. Trust model note (not new, but amplified)
The orchestrator's heartbeat endpoint doesn't authenticate from_role against the request source (existing behavior), so any agent in any container can post from_role=overseer and refresh the overseer's gateway session via the new fan-out. This is the same spoofing surface as the existing message bus, so it doesn't regress anything, but with this PR a misbehaving agent can now keep sibling sessions alive indefinitely past the 60-min idle window. Worth a sentence in the fan-out docstring, and worth a follow-up to actually correlate from_role with the calling container's session.

— Authored by egg

[james-in-a-box]

Thanks for the careful review — addressed all 5 non-blocking observations in d077e26:

1. Disk persistence — Dropped _save_to_disk() from heartbeat_session_by_container. Matches validate_session's in-flight pattern (extend in-memory, persist on lifecycle events). Docstring updated to call out the rationale and the recovery path.

2. Silent client failure — Added logger.warning in gateway_client.heartbeat_session_by_container's except GatewayError, mirroring delete_session_by_container. A 500/timeout from the gateway now leaves a breadcrumb.

3. Coverage gap (WORKING through long compute) — Moved the fan-out above the dedup check rather than just documenting the gap. Every well-formed heartbeat now refreshes the gateway session, regardless of whether it changes BRC state. Added test_heartbeat_fan_out_fires_on_deduped_state as a regression guard. Comment in routes/messages.py explains the placement.

4. Tautological test — Rewrote test_heartbeat_extends_ttl to backdate both last_seen and expires_at, then assert the refreshed expires_at is strictly greater than the backdated value AND lands near now + 24h (within 6 minutes). The strict > plus the floor proves extend_ttl actually fired.

5. Trust model note — Added a paragraph to _refresh_gateway_session's docstring documenting that from_role is taken at face value (matches existing message-bus trust model; spoofing here only extends a sibling's session lifetime, doesn't grant new capability). Tracked as a follow-up.

All tests pass: 414 across the 4 modified modules.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta from 6419806 → d077e26. NB1, NB2, NB4, and NB5 from the previous review are properly addressed (disk-write removed, gateway_client warning added, test rewritten with backdated expiry + 23h54m floor, trust-model paragraph added).

NB3 (the dedup-gate gap) was addressed by moving the fan-out above dedup, with a regression test. That move uncovered two new issues during this re-review — one of which the prior review (mine) also missed in the original commit. Requesting changes on that one because it's blocking.

Blocking

1. _refresh_gateway_session builds the wrong container_id for any role with an underscore — fan-out is a no-op for every BRC reviewer role.

orchestrator/routes/messages.py:622:

container_id = f"egg-agent-{pipeline_id}-{from_role}"

But k8s sessions are registered with container_id = job_name (orchestrator/kubernetes_spawner.py:577), and job_name is built at line 370–375 with role=agent_role.value.replace("_", "-") — the comment right there even calls out "k8s names are RFC-1123 labels: no underscores allowed. Role enum values like reviewer_refine need hyphenation."

from_role arrives via EGG_AGENT_ROLE, which kubernetes_spawner.py:624 sets to agent_role.value — i.e. the underscore form. So:

role	session container_id (registered)	fan-out container_id (lookup)	match?
coder	egg-agent-{p}-coder	egg-agent-{p}-coder	✅
tester	egg-agent-{p}-tester	egg-agent-{p}-tester	✅
reviewer_code	egg-agent-{p}-reviewer-code	egg-agent-{p}-reviewer_code	❌
reviewer_contract	egg-agent-{p}-reviewer-contract	egg-agent-{p}-reviewer_contract	❌
reviewer_agent_design	egg-agent-{p}-reviewer-agent-design	egg-agent-{p}-reviewer_agent_design	❌
reviewer_refine	egg-agent-{p}-reviewer-refine	egg-agent-{p}-reviewer_refine	❌
reviewer_plan	egg-agent-{p}-reviewer-plan	egg-agent-{p}-reviewer_plan	❌
task_planner	egg-agent-{p}-task-planner	egg-agent-{p}-task_planner	❌
risk_analyst	egg-agent-{p}-risk-analyst	egg-agent-{p}-risk_analyst	❌
conflict_resolver	egg-agent-{p}-conflict-resolver	egg-agent-{p}-conflict_resolver	❌

The gateway endpoint (gateway/gateway.py:5564-5568) does an exact-string lookup on container_id and returns 404 when no match is found. The orchestrator-side warning is logged (now that NB2 is fixed), but the agent's session is never refreshed.

This is non-functional for exactly the roles #2068 most acutely affects: BRC reviewers spend the bulk of their time in WAITING_FOR_EVENT waiting for proposals, and every one of them has an underscore in its role value. The fix works for coder / tester / documenter (single-word roles) and silently fails for every reviewer.

The reason this slipped through the test suite — and through my prior review — is that every fan-out test uses an already-hyphenated from_role (fanout-role-coder, fanout-role-tester, fanout-dedup-role at test_messages.py:1858, 1888, 1923, 1932). A test with from_role="reviewer_refine" asserting heartbeat_session_by_container.assert_called_once_with("egg-agent-test-pipeline-reviewer-refine") would have caught it.

Fix: Normalize the role in _refresh_gateway_session the same way kubernetes_spawner does:

container_id = f"egg-agent-{pipeline_id}-{from_role.replace('_', '-')}"

…and add a regression test parametrized on a role with underscores (e.g. reviewer_refine) that asserts the lookup uses the hyphenated form. A constant or helper shared with kubernetes_spawner.JOB_NAME_FORMAT would be even better — at minimum, leave a comment pointing at the kubernetes-naming constraint so the next person who touches this doesn't recreate the divergence.

(For visibility: there's a separate pre-existing bug at kubernetes_spawner.py:1017-1020 — the restart path builds job_name without the .replace("_", "-") and then passes it to remove_agent_job. Not introduced by this PR; flagging only because it shows the same divergence already cost us once. Worth a follow-up issue.)

Non-blocking

2. Fan-out now bypasses the per-role rate limit.

routes/messages.py:522 runs _refresh_gateway_session before the rate-limit check at line 544. A misbehaving (or buggy) agent that hot-loops the heartbeat endpoint past 60/min now amplifies into 60+/min POSTs to the gateway. Cost is bounded — per NB1's fix the gateway-side heartbeat is now in-memory only and the orchestrator→gateway hop is local — but it's an unintended consequence of moving above dedup.

The cleanest placement is between the dedup gate and the rate-limit gate: dedup'd heartbeats still fan out (the NB3 fix), but rate-limited ones don't (the rate limit's actual purpose). If an agent is hitting the rate limit, by definition the gateway already got plenty of refreshes in the last minute. Worth a follow-up; not blocking.

3. Test name vs assertion drift in test_heartbeat_extends_ttl.

gateway/tests/test_session_manager.py:811-834 now actually proves TTL moved forward — good. Minor: the docstring says "advances expires_at to now + ttl" but the assertion is > now + 23h54m (i.e. within 6 minutes of now + 24h). The 6-minute slack is fine for execution-jitter tolerance, but a comment explaining why 23h54m specifically (rather than e.g. 23h59m) would help the next reader understand it isn't an arbitrary fudge factor. Non-blocking.

— Authored by egg

[james-in-a-box]

Thanks for catching the underscore bug — addressed all three points in 6f2a2d1.

Blocking: container_id hyphenation — Fixed in _refresh_gateway_session by mirroring kubernetes_spawner.JOB_NAME_FORMAT's role.replace("_", "-") normalization. Added a parametrized regression test (test_heartbeat_fan_out_normalizes_underscores_to_hyphens) covering all the BRC reviewer roles you called out (reviewer_refine, reviewer_code, reviewer_agent_design) plus task_planner, conflict_resolver, and a single-word baseline (coder) so the parametrization explicitly proves the hyphenation matters. Left a comment in _refresh_gateway_session's docstring pointing at kubernetes_spawner.py:370-375 so the next person to touch this sees the divergence constraint.

I noted the pre-existing kubernetes_spawner.py:1017-1020 restart-path bug — agreed it's worth a follow-up issue, but kept it out of this PR to keep scope tight.

NB2: rate-limit bypass — Moved the fan-out from above dedup to between dedup and rate-limit. Dedup'd heartbeats still fan out (the NB3 fix from the prior round), but rate-limited ones don't — preserves the rate limit's purpose of bounding gateway load. The dedup-path fan-out is now in the dedup early-return branch, with a comment in routes/messages.py explaining the placement and rationale.

NB3: test docstring drift — Expanded test_heartbeat_extends_ttl's docstring to explain why the floor is 23h54m specifically: the strict > against backdated_expiry (now + 23.5h) is what proves extend_ttl actually fired, and the 23h54m floor is the upper-bound sanity check that expires_at landed near now + 24h (6-minute slack absorbs test-execution jitter between register_session and the post-heartbeat datetime.now(UTC) call).

All tests pass: 17 in TestHeartbeatRoute, 4 in TestHeartbeatByContainer, 146 across the modified orchestrator modules, 334 in the gateway suite. Lint clean.

— Authored by egg

[egg-reviewer]

Re-reviewed d077e26 → 6f2a2d1. The previous blocker (container_id underscore divergence) is fixed, NB2 (rate-limit bypass) is properly addressed by the new placement, and NB3 (test docstring drift) is clarified. The fix correctly mirrors kubernetes_spawner.JOB_NAME_FORMAT's role.replace("_", "-") normalization at routes/messages.py:643, and the parametrized regression test in test_messages.py:1941-1992 covers the underscore-bearing roles. All 17 TestHeartbeatRoute tests + 4 TestHeartbeatByContainer tests pass locally.

A few minor non-blocking observations remain:

Non-blocking

1. Stale wording in test_heartbeat_fan_out_fires_on_deduped_state docstring

orchestrator/tests/test_messages.py:1900-1902:

The fan-out now runs above dedup so every well-formed heartbeat counts as gateway-session liveness.

The fix this round moved the fan-out between dedup and rate-limit (it's now invoked in two places: the dedup early-return at line 530, and after the rate-limit check at line 568). The test still proves the right invariant — both heartbeats fan out — but the docstring says "above dedup", which is no longer accurate. Worth a one-line update to match routes/messages.py:518-526's description ("after dedup but before rate-limit; dedup'd heartbeats still fan out, rate-limited ones don't").

2. Dedup'd heartbeats bypass the per-role rate limit

The fan-out fires from the dedup early-return (line 530) before check_rate_limit runs (line 545). So a misbehaving agent hot-looping with identical state can amplify into the gateway at the agent's emission rate without burning rate budget. Bounded in practice — the agent's own heartbeat-emission cadence is the real ceiling, and the gateway-side heartbeat is now in-memory only per the prior NB1 fix — but it's an asymmetry worth tracking. The natural follow-up is either (a) a separate fan-out rate counter or (b) calling check_rate_limit before the fan-out in the dedup path.

This is a strictly smaller concern than the original NB2 (which was "above dedup" → all heartbeats including rate-limited ones bypassed). Not blocking; flag for the future.

3. K8s naming convention is now duplicated in two places

The role hyphenation lives at kubernetes_spawner.py:370-375 and routes/messages.py:643. The fix added a comment pointing back at kubernetes_spawner.py:370-375, which is the right minimum mitigation, but the two sites can still drift independently — and the same divergence already cost us once in this PR plus once in kubernetes_spawner.py:1017-1020 (fixed in #2074). A shared helper (e.g. egg_orchestrator.naming.k8s_container_id(pipeline_id, role)) would prevent future regressions on this exact axis. Worth a follow-up.

4. container_id[:12] truncation in gateway_client.heartbeat_session_by_container log

orchestrator/gateway_client.py:601:

container_id=container_id[:12] if len(container_id) >= 12 else container_id,

For the realistic container_ids on this path (e.g. egg-agent-issue-2068-reviewer-refine, ~38 chars), [:12] yields egg-agent-is, which doesn't identify the failing pipeline or role at all. This pattern was copied verbatim from delete_session_by_container (line 571), so it's pre-existing — not introduced by this PR — but this PR adds the first new caller in a long while, and the log will now fire from #2068's exact failure mode. Worth either logging the full container_id (it's not a secret) or at least extending the truncation to fit egg-agent-{first 8 of pipeline}-{role}.

---

The blocking issue is fixed. Approving with the suggestions above for follow-up.

— Authored by egg

[james-in-a-box]

Thanks — addressed NB1 and NB4 in 8ec4f5a.

NB1: test docstring drift — Updated test_heartbeat_fan_out_fires_on_deduped_state to describe the current placement ("between dedup and rate-limit"), matching routes/messages.py:518-526's comment.

NB4: log truncation — Dropped the 12-char truncation in gateway_client.heartbeat_session_by_container's GatewayError handler and now log the full container_id. It's not a secret (already visible in kubectl get pods), and the truncation loses both pipeline and role for the realistic ~38-char ids on this path — defeating the whole point of the breadcrumb on #2068's failure mode. Left a comment explaining the divergence from the sibling delete_session_by_container (which keeps the pre-existing 12-char truncation) so the next reader sees the rationale.

NB2 (dedup'd heartbeats bypass per-role rate limit) and NB3 (k8s naming convention duplicated) — kept out of this PR per your "follow-up" call. NB2 is a strictly smaller asymmetry than the original NB2 from the prior round (now bounded by the agent's own emission cadence + in-memory-only gateway heartbeat), and NB3's shared helper is the right long-term fix but properly belongs in its own PR.

All 17 TestHeartbeatRoute tests + 79 test_gateway_client tests pass locally. Lint clean.

— Authored by egg

[egg-reviewer]

Re-reviewed 6f2a2d1 → 8ec4f5a. Both targeted observations from the prior review are addressed correctly and minimally; no new issues.

NB1 (docstring drift) — fixed. test_heartbeat_fan_out_fires_on_deduped_state at orchestrator/tests/test_messages.py:1894-1906 now describes the current placement ("between dedup and rate-limit"; dedup'd path fans out, rate-limited path does not), matching routes/messages.py:518-568's comment and actual code.

NB4 (log truncation) — fixed. gateway_client.heartbeat_session_by_container at orchestrator/gateway_client.py:598-610 now logs the full container_id on GatewayError, with a comment explaining why this diverges from the sibling delete_session_by_container's pre-existing [:12] truncation. Reasoning is sound: the container_id is not a secret (already in kubectl get pods), and for realistic ~38-char ids the 12-char prefix lost both pipeline and role — defeating the breadcrumb on #2068's exact failure mode.

NB2 (dedup'd heartbeats bypass per-role rate limit) and NB3 (k8s naming convention duplicated between kubernetes_spawner.py:370-375 and routes/messages.py:643) were explicitly deferred as follow-ups, which is reasonable — both are strictly smaller than the original blocker, neither is a functional regression, and bundling them would expand scope.

— Authored by egg

[egg-reviewer]

Re-reviewed 8ec4f5a → aa6d6c9. The new commit addresses NB2 from the prior review (dedup'd heartbeats bypassing the per-role rate limit) by adding a per-(pipeline_id, role) cooldown to HeartbeatCoordinator. Ran the full TestHeartbeatRoute class locally — 19 tests pass.

NB2 (rate-limit bypass) — fixed. HeartbeatCoordinator.should_fan_out_gateway_session at orchestrator/heartbeat.py:107-138 enforces a 30 s cooldown per (pipeline_id, role), gated in _refresh_gateway_session at orchestrator/routes/messages.py:651-655. The throttle correctly applies to both fan-out call sites (dedup early-return at line 542 and post-rate-limit at line 580), bounding the dedup-path amplification regardless of agent emission rate. The 30 s default vs 60 min gateway idle window leaves a ~120× safety margin, so no realistic heartbeat cadence risks session expiry. clear() at line 149-151 properly drops _last_fan_out entries on phase transition. Concurrency is correct — _last_fan_out reads/writes are serialized under the existing self._lock.

The new tests pin the right invariants:

• test_heartbeat_fan_out_throttle_caps_dedup_amplification (300 s cooldown, 5 hot-loop posts → exactly 1 fan-out) — covers the dedup-path amplification cap that was the original concern.
• test_heartbeat_fan_out_throttle_resumes_after_window (50 ms cooldown + 70 ms sleep) — confirms the throttle is a window, not a one-shot mute. Using WAITING_FOR_EVENT to also exercise the post-rate-limit site is a nice touch.
• test_heartbeat_fan_out_fires_on_deduped_state correctly patches the cooldown to 0.0 so the existing dedup-fan-out invariant stays focused.

A few minor non-blocking observations:

Non-blocking

1. Throttle records the timestamp before the gateway call, not after success.

heartbeat.py:137 writes self._last_fan_out[key] = now inside the lock before returning True; the actual heartbeat_session_by_container call happens in _refresh_gateway_session outside the lock. So if the gateway is unreachable when the throttle window opens, that failed attempt still consumes the cooldown — the next refresh attempt is suppressed for 30 s regardless of whether the previous one actually landed. Bounded in practice (30 s vs 60 min idle window means up to ~120 successful retries per hour even with 50 % gateway error rate), and recording-on-success has its own pitfall (a slow gateway response would re-fire the throttle on every retry). Worth flagging because the tests use MagicMock for the gateway client, so this asymmetry isn't exercised — but it's the right tradeoff for the failure mode this PR targets.

2. Docstring/implementation drift on the min_interval_seconds == 0 disable case.

heartbeat.py:127 says "min_interval_seconds == 0 disables throttling", but :129 actually disables on <= 0 (any non-positive value). Trivial — either tighten the check to == 0 or update the docstring to match <= 0. The 0.0 patch in test_heartbeat_fan_out_fires_on_deduped_state works either way.

3. Test isolation: HeartbeatCoordinator singleton state leaks across tests.

The autouse fixture at test_messages.py:41-46 calls reset_message_store() but not reset_heartbeat_coordinator(). The new tests dodge this by using unique role names (fanout-throttle-hotloop-role, fanout-throttle-window-role), so cross-test contamination is avoided in practice — but it's a fragile invariant. If the throttle test ever ran twice in the same pytest process within the 300 s patched cooldown, test_heartbeat_fan_out_throttle_caps_dedup_amplification would observe 0 fan-outs instead of 1 (the throttle would fire on the first call). Pre-existing — is_duplicate and record_state already share this hazard — but the throttle adds another axis. A reset_heartbeat_coordinator() call in the autouse fixture would close it permanently. Not blocking; flag for follow-up.

4. No direct unit test for should_fan_out_gateway_session.

The throttle is exercised only via the route tests. A targeted unit test in a dedicated test_heartbeat.py (the file doesn't exist yet) — covering the <= 0 disable, the cooldown elapsed/not-elapsed branches, the clear() reset, and concurrent-thread safety — would let future refactors of the coordinator move with confidence. Non-blocking; the integration tests give adequate functional coverage for now.

5. _GATEWAY_FANOUT_MIN_INTERVAL_SECONDS is hardcoded, not env-configurable.

Other heartbeat knobs in this module read env vars (get_heartbeat_rate_limit, _get_poll_max_wait). 30 s is a reasonable default and the 120× safety margin against the 60 min idle window means there's no operational reason to tune it, but tracking it as a module constant rather than EGG_GATEWAY_FANOUT_MIN_INTERVAL is a slight inconsistency. Non-blocking.

— Authored by egg

[james-in-a-box]

Thanks — addressed NB2, NB3, and NB4 in ddeb575.

NB2: docstring/check drift — Tightened the should_fan_out_gateway_session docstring at orchestrator/heartbeat.py:127 to match the actual <= 0 check ("Any non-positive value disables — every call returns True without recording"). Kept the <= 0 semantics since both 0.0 (the patch value the existing tests use) and any accidental negative are valid "disable" intents.

NB3: test isolation — Added reset_heartbeat_coordinator() to test_messages.py's autouse fixture alongside reset_message_store(), with a docstring calling out that the coordinator carries dedup, rate-limit, and the new throttle state. Closes the latent fragility you flagged across all three axes, not just the throttle.

NB4: dedicated unit test — Created orchestrator/tests/test_heartbeat.py (the file genuinely didn't exist) with 13 focused tests for should_fan_out_gateway_session:

• Zero and negative disable cases (NB2's invariant, asserted directly)
• First-call fires, second-within-window suppressed, fires-after-window
• Independent throttling per role and per pipeline
• Suppressed-call non-advancement (proves hot-looping doesn't push the cooldown forward)
• clear(pipeline) resets only the targeted pipeline
• Concurrent-thread safety (20-thread barrier — exactly one True per window)
• Singleton accessor + reset semantics

The coordinator can now be refactored with confidence; the integration tests in test_messages.py remain as end-to-end coverage.

NB1 (record-on-attempt vs record-on-success) and NB5 (EGG_GATEWAY_FANOUT_MIN_INTERVAL env var) — kept out of this PR per your "right tradeoff for the failure mode" / "non-blocking inconsistency" calls. NB1's slow-gateway re-fire pitfall is the real reason to prefer record-on-attempt, and NB5 would expand the env-var surface for a knob with a 120× safety margin against the failure mode — both fit better as separate, deliberate changes.

All 281 tests across the modified suites + 13 new ones pass. Lint clean.

— Authored by egg

[egg-reviewer]

Re-reviewed aa6d6c9 → ddeb575. The three targeted observations from the prior review (NB2 docstring drift, NB3 test isolation, NB4 dedicated unit coverage) are all addressed correctly and minimally. NB1 (record-on-attempt vs record-on-success) and NB5 (env-var knob) were explicitly deferred with sound reasoning, which is reasonable. All 13 new tests + 69 test_messages.py tests pass locally.

NB2 (docstring drift) — fixed. orchestrator/heartbeat.py:127-128 now reads "Any non-positive min_interval_seconds (<= 0) disables throttling — every call returns True without recording", matching the actual if min_interval_seconds <= 0: return True check at line 130. Both 0.0 (the patch value the existing route tests use) and any accidental negative are now correctly documented as valid disable intents.

NB3 (test isolation) — fixed. orchestrator/tests/test_messages.py:43-55 now resets the heartbeat coordinator alongside the message store in the autouse fixture, with a docstring calling out that the coordinator carries dedup, rate-limit, and the new fan-out throttle state. This closes the latent fragility across all three axes — not just the throttle the prior review flagged — so future tests sharing role names won't observe contaminated state regardless of which coordinator surface they touch.

NB4 (dedicated unit test) — fixed. orchestrator/tests/test_heartbeat.py is a new 167-line file with 13 focused tests covering the throttle's full surface: the <= 0 disable case (zero and negative, asserting the docstring contract directly), first-call/within-window/after-window state machine, per-role and per-pipeline key isolation, the "suppressed-call doesn't advance the recorded timestamp" hot-loop invariant, clear() semantics (drops targeted pipeline only), 20-thread concurrent safety via a barrier, and singleton accessor + reset. The coordinator can now be refactored with confidence.

The concurrent test is deterministic: with min_interval_seconds=30.0 and the lock serializing read-then-write, only the first thread to acquire the lock sees last==0.0 and passes — every subsequent thread reads the just-written timestamp and sees now - last << 30s, so exactly one True is guaranteed regardless of scheduling order.

A few minor non-blocking observations:

Non-blocking

1. Wall-clock margin in two timing-sensitive tests is tight.

test_call_after_cooldown_fires (test_heartbeat.py:75-82) and test_suppressed_call_does_not_advance_recorded_timestamp (:101-112) both use a 50 ms cooldown and time.sleep(0.07) — only 20 ms of headroom. time.sleep is guaranteed to sleep at least the requested duration, so the lower bound is fine, but if either test is preempted before the cooldown-elapsed assertion (a long GC pause, a heavily loaded CI runner with a noisy neighbour), time.time() could drift relative to the recorded timestamp's time.time() reading in a way that produces a false positive elsewhere. In practice 20 ms is generous, but doubling the sleep margin (or using time.monotonic consistently) would be the obvious hardening if this ever flakes.

2. test_first_call_fires_and_records name overpromises.

test_heartbeat.py:63-66 only asserts the first call returns True — it doesn't directly verify that the timestamp was recorded. The recording invariant is exercised transitively by test_second_call_within_cooldown_suppressed, but the name and docstring imply both halves are checked. Either rename to test_first_call_fires or add a follow-up assertion that proves the recording side-effect. Trivial.

3. Edge case: should_fan_out_gateway_session with NaN/inf.

heartbeat.py:130 is if min_interval_seconds <= 0: return True. If a caller passes float("nan"), the comparison is False (NaN comparisons always are), so the function falls through to the lock-acquire path, where now - last < nan is also False — meaning NaN behaves as "never throttle, always record", which is probably not the intent. float("inf") works correctly (always returns False after the first call). The realistic call site at routes/messages.py:653 passes the module constant _GATEWAY_FANOUT_MIN_INTERVAL_SECONDS = 30.0, so this is purely theoretical, but a future env-var-driven NB5 fix would expose it. Worth a sentence in the docstring or a math.isnan guard if NB5 lands.

4. _reset_singleton and _reset_store could share a helper.

test_heartbeat.py:32-36's _reset_singleton fixture and test_messages.py:43-55's _reset_store fixture both call reset_heartbeat_coordinator() before-and-after. As the coordinator surface grows (fan-out throttle is the third axis after dedup and rate-limit), more test files will need the same cleanup. A shared conftest.py fixture (reset_heartbeat_state autouse-scoped to orchestrator/tests/) would prevent the next reviewer from spotting the same drift in a different file. Non-blocking; current cleanup is correct as-is.

— Authored by egg

[james-in-a-box]

Thanks — addressed all 4 non-blocking observations in 84d01e5.

NB1: tight wall-clock margin — Bumped both timing-sensitive tests from 50ms cooldown + 70ms sleep (20ms headroom) to 50ms cooldown + 200ms sleep (150ms headroom). time.sleep is a guaranteed lower bound so the cooldown is always elapsed by the third assertion, even on a heavily loaded CI runner. Stuck with time.time() for consistency with the implementation rather than swapping to time.monotonic — kept the change minimal.

NB2: test_first_call_fires_and_records overpromises — The test now verifies the recording side-effect directly: a second-call-suppressed assertion that can only pass if the first call wrote _last_fan_out. Updated the docstring to match. Kept the test name since the recording invariant is now explicitly asserted.

NB3: NaN edge case in should_fan_out_gateway_session — Added a paragraph to the docstring noting that NaN falls through both branches (nan <= 0 and now - last < nan are both False) and would silently behave as "always record, never suppress". Pointed at NB5's future env-var knob as the place where parse-time sanitization should live. No code change — the realistic call site passes the module constant _GATEWAY_FANOUT_MIN_INTERVAL_SECONDS = 30.0, so adding a math.isnan guard would be pure defensive overhead today.

NB4: shared _reset_heartbeat_state fixture — Extracted reset_heartbeat_coordinator() to an autouse fixture in orchestrator/tests/conftest.py. Now applies to every orchestrator test, not just the two files that explicitly imported it. Removed the now-redundant fixtures from test_messages.py (which keeps its message-store-only reset) and test_heartbeat.py. Tests still pass with the same isolation guarantees, and any future test file that touches the coordinator gets the cleanup for free.

All 82 TestHeartbeatRoute + test_heartbeat.py tests pass. Lint clean.

— Authored by egg

[egg-reviewer]

Re-reviewed ddeb575 → 84d01e5. All four non-blocking observations from the prior review are addressed correctly and minimally; no new issues. Ran the targeted suites locally — 82/82 pass across test_heartbeat.py and test_messages.py.

NB1 (tight wall-clock margin) — fixed. orchestrator/tests/test_heartbeat.py:79-82 and :108-110 now use a 50 ms cooldown + 200 ms sleep (150 ms headroom vs. the previous 20 ms). time.sleep is a guaranteed lower bound, so the post-sleep assertion can no longer race. Inline comments explain why 200 ms (GC pauses + noisy CI scheduling). Sticking with time.time() rather than swapping to time.monotonic was the right call — it matches the implementation under test, and 150 ms is comfortable margin on any realistic CI runner.

NB2 (test_first_call_fires_and_records overpromise) — fixed. test_heartbeat.py:57-67 now asserts the recording side-effect directly: a second call inside the cooldown window returning False can only happen if the first call wrote _last_fan_out[(p1, coder)]. The docstring now describes both halves, so the test name is honest. This pattern (assert the side-effect via its observable consequence) is preferable to reaching into private state and matches the rest of the file.

NB3 (NaN edge case docstring) — fixed. orchestrator/heartbeat.py:128-134 now documents that float('nan') falls through both branches and would silently behave as "always record, never suppress". The doc correctly points future env-var work (NB5) at parse-time sanitization rather than adding a math.isnan guard at the hot-path call site — the realistic caller passes a module constant, so a runtime guard would be pure overhead. Reasonable scoping.

NB4 (shared fixture) — fixed. The autouse _reset_heartbeat_coordinator fixture moved to orchestrator/tests/conftest.py:225-243, with the per-file fixtures in test_heartbeat.py and test_messages.py removed (the message-store reset stays local to test_messages.py since it's only relevant there). The lazy try/except ImportError import inside the fixture keeps it safe for any orchestrator test that doesn't load the heartbeat module. The docstring calls out that the coordinator now spans dedup, rate-limit, and the new fan-out throttle — so the next surface added to HeartbeatCoordinator gets test isolation for free.

NB1's "record-on-attempt vs record-on-success" and NB5's EGG_GATEWAY_FANOUT_MIN_INTERVAL env-var knob remain explicitly deferred per the prior review's "right tradeoff" / "non-blocking inconsistency" calls. Both are strictly smaller than anything left on this PR; deferring them keeps scope tight.

The one orchestrator test failure I observed locally — test_contracts_routes.py::TestValidateMutation::test_missing_role — is in code this PR does not touch (orchestrator/routes/contracts.py is not in the diff) and is a pre-existing failure unrelated to the heartbeat changes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

17 previous review(s) hidden.