fix(agent): time out hung service heartbeats - #3990
Conversation
|
Warning Review limit reached
Next review available in: 6 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d794d8dbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The per-attempt deadline can fire after the response headers arrive, while the JSON body is still being read. That abort surfaced from the body read, outside the transport-error wrapper in sendHeartbeatRequest, so it reached isRetryableHeartbeatFailure as a raw AbortError and was classified as permanent. The tick stopped after one attempt instead of using its remaining two, so a transient body stall counted as a full heartbeat failure. Wrap the response read in the same NETWORK_ERROR mapping as the fetch call. An error that is already ours is rethrown untouched, so a non-ok response keeps its httpStatus and a 4xx still fails on the first attempt with no retry. Also add the false-positive guard the deadline needs: a heartbeat that answers inside its own interval, fast or slow, must never retry, skip, or escalate. Its double honours the abort signal the way a real fetch does, so an over-eager deadline shows up as a failure instead of being answered late anyway.
The stalled-body-read test timed out on a CI coverage shard. Its 20ms interval left a full escalation, nine attempts across three ticks, to finish inside 1.5 seconds, and a contended runner stretches timers of that size well past their nominal values. What the test pins is the classification of a body-read abort, not the clock, so it now runs on a 50ms interval and waits with the same 10 second budget the other lifecycle tests here use. The bounded-time property stays pinned by the hung-heartbeat test, which is where it belongs. The false-positive test had the same fragility in the other direction: a 140ms answer against a 200ms deadline needed only a 60ms overshoot to report a false timeout. The slow answer now sits at 200ms against a 500ms deadline, still above the quarter-interval mark, so an over-eager deadline is still caught.
The previous version of this test built a Response over a ReadableStream that only errored once the per-attempt deadline aborted its signal. That passed locally and failed on the CI coverage shard, where escalation never arrived inside a ten second budget, so the double was not portable to the instrumented runner. The classification it pins does not need any of that. A body read that fails after the headers arrive is transient whatever caused it, so the double now just rejects the read directly with the raw DOMException an aborted or reset read throws. The test drives one heartbeat, asserts all three attempts are used, and asserts both retry notices are logged. No timers, no streams, and it fails in milliseconds against the old code instead of waiting out a budget.
Review — score: 86/100. fix-then-merge, one line.Reviewed against It closes #728 — measured, not readBefore this change, the same probe gave 1 request and 0 escalations, ever — that measurement is No regression on the slow-but-healthy case, which was the risk in picking a timeout: the real 4xx still fails immediately — 400/401/404 each take one attempt. And the refactor moving The #3961 in-flight guard still holds — re-measured at interval 200ms / latency 100ms: Retrying a heartbeat whose response body read fails after headers arrive is a case neither the Finding — CONFIRMED, medium. A healthy service can declare itself failing.
A healthy service raises the exact alarm this subsystem exists for. That is worse than the silence Reachability, stated fairly: the default is 30s against an observed worst latency of 2704ms — a One line: timeoutMs: Math.max(input.heartbeatIntervalMs, HEARTBEAT_MIN_ATTEMPT_TIMEOUT_MS)with a floor of a few seconds. That keeps the property worth having — raise the interval for a slow Secondary, non-blocking and correctly documentedWith A methodology note worth recordingThe reviewer's first run reported the hung heartbeat still not escalating — the fix appearing not to CI32 pass, 6 skipping, zero failing, zero pending. Fully green. DispositionThis is in the merge queue. The floor is cheaper to add now than to diagnose later from a false |
|
#4004 fixed the same defect (veryfront/veryfront-issue-inbox#728) in the same file and is now closed as a duplicate of this PR. I compared them by running each one's tests against the other's code, with the reasoning and numbers in the closing comment on #4004. Short version, since it bears on this PR:
Nothing to change here. One test from #4004 covers a case this PR's guard does not: a deadline pinned to a fixed number of milliseconds rather than a fraction of the interval. It is carried over in #4007 rather than lost, stacked on this branch so it collapses to a single test once this merges. This branch is untouched. |
|
One finding against this PR, raised while comparing it with #4004. Recording it here so it is not lost, whichever way it goes. It does not touch the hang fix, which is sound. A schema failure is now retriedMoving An HTTP 200 with a malformed body is a permanent protocol mismatch, and it now gets all three attempts on every tick. Probed directly, same probe on both trees, driving Codex reached the same conclusion independently on #4007, which is stacked on this branch. How much it mattersNot much, which is why this is a note rather than a request to pull the PR from the queue:
It is still a behaviour change from The fixSplit the two concerns. Wrap the I have not touched this branch. Happy to make the change here or as a follow-up, whichever the merge owner prefers. |
Follow-up to #3990, which is merged. This is not a competing fix: it keeps both behaviours #3990 added and narrows only the classification that came with them. Four mutations of src/agent/service/registration.ts, each run against the full registration test file: revert to main's shape schema test RED (3 requests, not 1) drop the body-read mapping #3990's body-read test RED (1, not 3) remove timeoutMs entirely #3990's hang test RED (no escalation in 1500ms) timeoutMs: 3_600_000 #3990's hang test RED (same) The first two show each half of this change is load-bearing and that the narrowing did not detach #3990's body-read retry. The last two show the hang-escalation deadline is untouched: both still produce the diagnostic "hung heartbeat attempts never reached persistent-failure escalation". Unmutated: 2 passed (16 steps), 0 failed. Refs veryfront/veryfront-issue-inbox#728 Refs veryfront/veryfront-issue-inbox#764
2 participants
Summary
Why one interval
The deadline is
heartbeatIntervalMs, so an attempt is cut off only once thenext heartbeat is already due. A request that slow is gone, not slow. The
deadline is not a separate setting: it moves with
VERYFRONT_AGENT_SERVICE_HEARTBEAT_INTERVAL_MS, so raising the interval for aslow link raises the deadline with it.
Worst case, a tick is three attempts plus backoff, about 3.25 intervals, and
escalation needs three failed ticks. At the 30s default a permanent hang now
escalates in roughly five minutes. Before, it never escalated at all.
The body-read failure
A body read can fail after the headers land: the deadline fires while the JSON
is still arriving, or the connection resets mid-body. That error came out of the
read, which sat outside the transport-error wrapper in
sendHeartbeatRequest,so it reached
isRetryableHeartbeatFailureas a rawAbortErrorand wasclassified as permanent: the tick stopped after one attempt instead of using its
remaining two. The response read now shares the fetch call's
NETWORK_ERRORmapping. Anerror that is already ours is rethrown untouched, so a non-ok response keeps its
httpStatusand a 4xx still fails on the first attempt with no retry.Red and green
Every new test was proven capable of failing:
timeoutMstimeoutMs: 3_600_000httpStatustimeoutMs: interval / 4The hang test holds every request open on a promise that never settles until its
signal aborts, so it exercises a real hang rather than a rejection. A rejecting
stub already advanced the counter before this change and would have proven
nothing.
Verification
deno test --preload=src/testing/preload.ts --no-check --allow-all src/agent/service/registration.test.tsdeno fmt --checkdeno task lint:cideno task typecheckdeno task docsdeno task docs:api-reference:checkRefs veryfront/veryfront-issue-inbox#728