test(agent): pin the heartbeat deadline at the production interval - #4007
Conversation
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.
The existing false-positive guard runs at a 500ms interval and a 200ms answer, so it catches a deadline set as a fraction of the interval but not one pinned to a fixed number of milliseconds. This adds the other half: a 3s answer at the 30s production interval, sized against a heartbeat measured at 2704ms while the control plane was degraded. Proven complementary. With the deadline replaced by a fixed `timeoutMs: 1_000`, `leaves healthy and intermittently slow heartbeats alone` still passes while this test fails on `The operation was aborted`. Carried over from #4004, closed as a duplicate of #3990. Refs veryfront/veryfront-issue-inbox#728
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe registration tests add a regression case for a heartbeat that completes after three seconds at the production 30-second interval. The test verifies completion without aborting, retries, or retry warning logs. ChangesHeartbeat lifecycle validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds focused test coverage for heartbeat deadlines without changing production behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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: f85bafb360
ℹ️ 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".
# Conflicts: # src/agent/service/registration.test.ts
The slow-success test drives lifecycle.heartbeat() directly and runs at a 30s interval, so the interval tick never fires inside it. Escalation is counted in that tick, which made `log.errors.length === 0` true by construction rather than by behaviour. Proven: an unconditional `logger.error` at the top of the tick callback turns four other tests in this file red and leaves this one green. Removing it, and saying in the comment why escalation is not asserted here. The 500ms test above covers the tick path. The two remaining assertions are live: capping the deadline below the simulated latency still turns this test red, and answering the first attempt with a 500 still trips the request-count assertion.
|
@coderabbitai review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Deep review — merge confidence 82/100Verdict: merge with nits. The test does exactly what it says and is the only step in the file that kills the What I verified (isolated worktree at c36a010)
Non-blocking
Spec#728's own requirement (hung heartbeat escalates in bounded time) is covered by #3990's hang test; this adds the complementary fixed-cap guard. No creep. The in-test comment that the 500ms test covers escalation is accurate. Not re-verifiedThe Reviewed with Claude Code; mutation and FakeTime probes were run, not inferred. |
Deep reviewMerge confidence: 95/100 FindingsNo introduced correctness, spec, security, or standards issue was found.
StandardsPass. This is a focused one-file regression test with intentional production-scale timing. SpecPass. The test exercises Architecture
Verification
|
2 participants
Carries one test over from #4004, which was closed as a duplicate of #3990.
#3990 has now merged, so the diff against the merge base is one file, 54 lines,
test only.
What it adds
lets a slow-but-successful heartbeat finish at the production interval: a 3sanswer at the 30s production interval, asserted to be answered on the first
attempt with no retry notice.
Why the existing guard is not enough
#3990 already has a false-positive guard,
leaves healthy and intermittently slow heartbeats alone. It runs at a 500ms interval with a 200ms answer, so theanswer sits at 40% of the interval. That catches a deadline expressed as a
fraction of the interval. It does not catch a deadline capped at a fixed number
of milliseconds, because 200ms clears almost any fixed value someone would
plausibly write.
This test sits at the other end: 3s against a 30s interval, 10% of it. The two
together cover both shapes of regression.
The 3s figure is not a round number. A heartbeat was measured at
duration_ms=2704during a degraded period (veryfront/veryfront-issue-inbox#709).That is slow, not dead, and a deadline that fails it turns a degraded control
plane into a dead one.
Mutation evidence
Every run is the whole file.
deno test --filtermatches only the two top-leveldescribenames in this file, so filtering by anitname runs nothing andexits 0. Verified:
--filter "lets a slow-but-successful"reports0 passed | 0 failed | 2 filtered out.timeoutMs: input.heartbeatIntervalMsleaves healthy and intermittently slowtimes out a permanently hung heartbeatMath.min(interval, 2_500)1_000interval * 0.25The first mutation row is the one that justifies this PR. Capping the deadline
at 2500ms is survived by all 15 of #3990's tests, because every one of them runs
at an interval of 500ms or less where
Math.minchanges nothing, or answersinstantly. Only this test kills it. That is coverage #3990 does not have.
The last row is the same argument in reverse: shrinking the deadline to a
quarter of the interval is caught only by the 500ms test and survived by this
one. The two are complementary, not redundant.
A flat
timeoutMs: 1_000is caught by #3990's hang test as well, so thatmutation on its own does not establish the gap.
What this test does not cover
It drives
lifecycle.heartbeat()directly at a 30s interval, so the intervaltick never fires inside it. Escalation is counted in that tick, so nothing here
can assert on escalation. An earlier revision asserted
log.errors.length === 0anyway; that assertion was true by construction. Proven by adding an
unconditional
logger.errorto the top of the tick callback, which turns fourother tests in this file red and leaves this one green. It has been removed.
Cost
3s of wall clock, measured. This file goes from 5s to 8s. The cost is intrinsic:
the test's whole point is a real latency measured against a real deadline at the
real interval, so it cannot be scaled down without giving up what it catches.
VF_TEST_TIME_SCALEonly stretches durations for slow runners, it does not fakea clock.
Not covered here
veryfront/veryfront-issue-inbox#758 is the opposite end of the same design: the
interval-sized deadline has no floor, so a short configured interval makes a
healthy control plane escalate. This test cannot catch that, and #758's fix
(
Math.max(interval, FLOOR)) leaves this test green at 30s. They areindependent.
Gates
deno fmt --check,deno task lint:ci,deno task typecheck, anddeno test --preload=src/testing/preload.ts --no-check --allow-all src/agent/service/registration.test.tsall exit 0.
Refs veryfront/veryfront-issue-inbox#728
Summary by CodeRabbit