fix(agent): retry a transient heartbeat failure before counting it - #3961
Conversation
The push-runtime-service heartbeat gave up on the first non-2xx response, so a transient 5xx from the control plane counted straight toward consecutiveHeartbeatFailures. Three of those escalate to the persistent-failure error, which meant a blip lasting a second spent a third of that budget. Retry the heartbeat up to three times within one tick, on a 5xx and on a request that never reached the control plane. A 4xx still fails on the first attempt: an unknown service id or a rejected token is a real error that retrying only delays. The backoff doubles and is scaled to a quarter of the configured heartbeat interval, so one tick's retry sequence always finishes before the next scheduled tick, however short that interval is. Refs veryfront/veryfront-issue-inbox#709
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reached
Next review available in: 56 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
stop() cleared the interval but left an in-flight retry running: the backoff timer stayed pending and woke to fire one more request after teardown, which can hold a process open during shutdown. Give the lifecycle an AbortController, abort it in stop(), and thread the signal through retryWithBackoff to both the pending backoff and the in-flight request. An abort during teardown is swallowed rather than counted, since it is a shutdown rather than a heartbeat failure. Also classify the failure by error slug plus an httpStatus recorded in the error context, instead of overwriting the registry error's own status with the upstream one. NETWORK_ERROR keeps its 502, and registration failures keep the status they had before this branch. Refs veryfront/veryfront-issue-inbox#709
Review — score: 85/100. fix-then-merge. Not merging yet.Independently reviewed by running and mutating the code. This repo currently gets no automated Finding 1 — CONFIRMED, medium. Blocking.The bounding claim is false, and the test named after it cannot detect that.
Overlapping ticks — the exact outcome the comment promises cannot occur. Worse, the test named Honest severity: at the 30 s production default you need ~10 s per attempt to overlap. Issue #709 Fix: pass Finding 2 — CONFIRMED, lowNo test covers the transport-rejection path. A fetch rejection does retry 3× as intended, but Finding 3 — minor, docThe body says the fetch rejection "is reported as a 503". The final commit changed that: the code Finding 4 — scope observation, not a defectThe identical unretried-heartbeat + consecutive-failure-count pattern lives at What is verified right — by mutation, not by reading
One correction to the PR body's red-green storyIt is thinner than written. Reverting To reach the barFinding 1. Findings 2-4 are follow-ups. |
…ning The backoff schedule bounds only the waits between attempts. A request has no deadline, so a slow control plane pushed a tick past its own interval and the next tick started on top of it. Measured at a 200ms interval with 120ms of request latency: 3 heartbeats concurrently in flight. Retries made this easier to reach by tripling the length of a tick, so guard the interval: while a tick is in flight the next beat is skipped and logged rather than started. That holds however slow a single attempt is, which a per-attempt timeout would not — three bounded attempts can still overrun a short interval. It also avoids newly aborting slow requests that do succeed. The trade is that a persistently slow control plane escalates more slowly, since a skipped beat is not a failure. The skip is logged so a wedged heartbeat stays visible. Correct the budget-ratio comment, which claimed the ratio kept two heartbeats from overlapping, and rewrite the test that was named for that property but asserted only schedule arithmetic against an instantly-resolving double. Refs veryfront/veryfront-issue-inbox#709
…ipped The in-flight guard introduced a beat that neither succeeds nor fails, and nothing asserted which. If a skipped beat reset consecutiveHeartbeatFailures, a slow-failing control plane would never escalate at all — the counter would be knocked back before it could reach three. Drive a lifecycle whose ticks outlive their interval, assert beats were actually skipped, and assert escalation still reports exactly three consecutive failures. Resetting the counter on the skip path makes it fail. Refs veryfront/veryfront-issue-inbox#709
Re-review after
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
1 participant
Description
The agent's push-runtime-service heartbeat gave up on the first non-2xx response. A
transient 5xx from the control plane therefore counted straight toward
consecutiveHeartbeatFailures, and three of those escalate to theAgent service heartbeat failing persistentlyerror — so a failure lasting about asecond spent a third of that budget with no attempt to recover.
heartbeatAgentPushRuntimeServicenow retries within one tick:retryWithBackoffhelper from#veryfront/errors.fetchrejection is reported as a 503 while keeping the original transport message,so the log line is unchanged).
that retrying only delays.
heartbeat interval (
heartbeatRetrySchedule). This bounds the waits between attemptsand nothing else — a request has no deadline, so backoff alone cannot keep a tick
inside its interval.
flight, and logs the skip. That holds however slow a single attempt is. A per-attempt
timeoutMswould not — three bounded attempts can still overrun a short interval —and it would newly abort slow requests that do succeed. The trade is that a
persistently slow control plane escalates more slowly, since a skipped beat is not a
failure; the skip log keeps a wedged heartbeat visible.
AbortControllerthatstop()aborts. Thesignal is threaded through
retryWithBackoffto both the pending backoff timer andthe in-flight request, so teardown leaves nothing pending that could hold a process
open. An abort during teardown is swallowed rather than counted as a failure.
Failures are classified by error slug plus an
httpStatusrecorded in the errorcontext, rather than by overwriting the registry error's own status with the upstreamone — so
NETWORK_ERRORkeeps its 502 and registration failures keep their prior status.consecutiveHeartbeatFailuresis untouched: it still increments only when the wholeattempt sequence fails, so persistent failures escalate exactly as before.
Related Issue(s)
Refs veryfront/veryfront-issue-inbox#709
Type of Change
Checklist
Tests
Red-then-green is not uniform across these, so stated precisely: two were genuinely red
before their fix (
retries a transient 500 …failed withVeryfrontError: … HTTP 500, andthe two below marked as such). The rest are regression guards that were green on both sides;
mutation testing, not a red run, is what establishes those have teeth.
retries a transient 500 so a one-second blip never counts as a failure— 500 then 200resolves in two attempts and logs no failure.
fails a client error immediately without retrying— 400/401/404 each reject afterexactly one attempt. This is the regression guard.
keeps one tick's backoff waits inside the heartbeat interval— schedule arithmetic only,and now named for what it actually asserts. Its previous name promised the no-overlap
property while testing none of it, against an instantly-resolving double.
never runs two heartbeat ticks at once, even when attempts outlast the interval— a fetchdouble with real latency, asserting max concurrent in-flight is 1. Genuinely red at 3
concurrent before the guard; neutering the guard to
if (false)reproduces that.still escalates persistent 500s, in bounded time— constant 500s still reachconsecutiveFailures=3, within a bounded wall-clock budget, which proves the retrysequence terminates.
cancels a pending retry backoff when the lifecycle stops—stop()during a backoffmust leave no timer pending and fire no further request. Genuinely red at 254 ms (the timer
ran to completion and retried after teardown), green at 1 ms.