fix(agent): keep a heartbeat schema failure out of transport retries - #4008
Conversation
#3990 moved the response read inside the transport-error wrapper so a body read that fails after the headers arrive is retried. That read also runs the schema parse, which throws a raw ZodError, so it got wrapped as NETWORK_ERROR with no httpStatus and became retryable too. An HTTP 200 carrying a malformed body is a permanent protocol mismatch, and it now spent all three attempts of every tick. Measured against origin/main, driving lifecycle.heartbeat() with a 200 and valid JSON of the wrong shape: 3 requests and 2 retry warnings, where before #3990 it was 1 request and none. The read now does its own transport mapping, so only the fetch call and the body read are treated as transport failures. The schema parse sits outside the wrapper and stays permanent. #3990's body-read retry is unchanged. Refs veryfront/veryfront-issue-inbox#728
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
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 |
|
@codex review |
|
@coderabbitai review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
Tracking issue for this: veryfront/veryfront-issue-inbox#764. I filed it from the Codex thread on #4007 before I knew this PR was open, so it duplicates your work rather than adding to it. It carries an independent verification of the regression, in case it is useful as a before/after for this PR: Probe: |
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
Deep review — merge confidence 88/100Verdict: merge. Minimal correct narrowing; no blockers. What I verified (in an isolated worktree at 790af5b)
Non-blocking
Not re-verifiedFull Reviewed with Claude Code; findings spot-checked by running the tests and mutations above. |
Deep reviewMerge confidence: 91/100 FindingsNo merge-blocking correctness, spec, security, or standards issue was found.
StandardsPass. The change is narrow, preserves the public API, adds focused regression coverage, and introduces no actionable smell or security issue. SpecPass. A valid JSON body with the wrong schema is no longer retried, while body-read failures retain the existing retry behavior. Architecture
Verification
|
…try loop Narrowing the transport wrapper to the fetch call also dropped the isVeryfrontError guard that used to sit in front of it. That guard covered more than the response read: `fetch` is a public option on CreateAgentServiceRegistrationLifecycleOptions, so a caller-supplied transport can reject with an error of ours that already carries a slug and an httpStatus. Without the guard every such rejection was rewrapped as a bare NETWORK_ERROR with no status, and isRetryableHeartbeatFailure retries anything without a status. Measured against origin/main, driving lifecycle.heartbeat() with a custom fetch rejecting with NETWORK_ERROR httpStatus 404: origin/main 1 request httpStatus=404 0 retry warnings before this 3 requests httpStatus=undefined 2 retry warnings after this 1 request httpStatus=404 0 retry warnings The guard is back on the fetch catch only. The schema parse still sits outside the wrapper, so the fix this PR is for is unchanged. Refs veryfront/veryfront-issue-inbox#728 Refs veryfront/veryfront-issue-inbox#764
Independent re-derivation, plus one regression this PR introducedEverything below was measured on my own probe in a fresh worktree, not taken from the report above. No bot reviewed this PR: the red The defect reproduces on
|
| heartbeat answers | main: requests / thrown / httpStatus | 790af5b: requests / thrown / httpStatus |
|---|---|---|
| 200, valid JSON, wrong shape | 3 / VeryfrontError / undefined | 1 / ZodError / n/a |
| 200, body is not JSON at all | 3 / VeryfrontError / undefined | 3 / VeryfrontError / undefined |
| 200, body read rejects mid-stream | 3 / VeryfrontError / undefined | 3 / VeryfrontError / undefined |
| 503 | 3 / VeryfrontError / 503 | 3 / VeryfrontError / 503 |
| 404 | 1 / VeryfrontError / 404 | 1 / VeryfrontError / 404 |
| fetch rejects | 3 / VeryfrontError / undefined | 3 / VeryfrontError / undefined |
Confirmed: 3 requests and 2 retry warnings on main, 1 request and 0 warnings here. Exactly one row moves, which is the scoping proof. The read is correctly split: only fetch and await response.json() map to NETWORK_ERROR, and the non-ok branch still carries its httpStatus (the 503 and 404 rows show that intact).
What the change also did, which was not in the report
Narrowing sendHeartbeatRequest dropped if (isVeryfrontError(cause)) throw cause; from the catch. That guard was needed for the response read, which moved out, but it also covered fetchImpl itself. fetch is a public option on the exported CreateAgentServiceRegistrationLifecycleOptions, so a caller-supplied transport can reject with an error of ours that already carries a slug and an httpStatus. Without the guard that classification was thrown away:
custom fetch rejects with NETWORK_ERROR httpStatus 404
origin/main 1 request httpStatus=404 0 retry warnings
790af5b1c2 3 requests httpStatus=undefined 2 retry warnings
A caller's permanent 4xx became a retried transport failure. In-repo production callers never pass fetch, so nothing shipping today hits it, but it is a real behaviour change on a public surface and it is one line.
Fixed in e133407b45: the guard is back on the fetch catch only. The schema parse stays outside the wrapper, so this PR's own fix is untouched. Row 1 of the table above still reads 1 request. Added keeps a fetch rejection that already carries an HTTP status out of the retry loop to pin it.
Mutation matrix, re-run by me on the current head
Every mutation is of src/agent/service/registration.ts, each run against the whole registration.test.ts.
| mutation | red test | other tests |
|---|---|---|
| none | none, `2 passed (17 steps) | 0 failed` |
revert registration.ts to main |
fails a heartbeat whose body does not match the schema, without retrying |
16 pass |
| drop the body-read mapping | retries a heartbeat whose response body read fails after the headers arrive |
16 pass |
remove timeoutMs |
times out a permanently hung heartbeat and escalates in bounded time |
16 pass |
timeoutMs: 3_600_000 |
same hang test | 16 pass |
drop the restored isVeryfrontError guard |
keeps a fetch rejection that already carries an HTTP status out of the retry loop |
16 pass |
No mutation reddens more than one test, so each row is a positive control for the others. The strongest claim in the original report holds, and it now covers six rows.
Fail-first, separately from mutation: this branch's registration.test.ts applied alone on origin/main at 9bac878ea9 exits 1, with only the new schema test red and the other 15 steps green. The new guard test cannot fail on main, because main already has that behaviour; its capability to fail is the last mutation row.
Other callers, checked
readAgentPushRuntimeServiceResponsehas one other caller,registerAgentPushRuntimeService. Registration is not retried, so the new body-read mapping causes no retry loop there. Its only consumer (src/agent/hosted/cloud-agent-chat-execution.ts:477) branches on config, not on error type, so no branch moves. The register path's failed body read now surfaces asNETWORK_ERRORinstead of a rawError, which is strictly better messaging.- Nothing downstream of
heartbeat()branches onisVeryfrontError. The escalation catch takesunknownand only callsgetErrorMessage, so a rawZodErrorstill counts as a failed tick and still escalates on the third. Escalation timing is unchanged. - Third error shape: a 200 whose body is not JSON at all is still retried three times, because
response.json()cannot tell a truncated body from a complete non-JSON one. That ismain's behaviour and fix(agent): time out hung service heartbeats #3990's, unchanged here, and worth a separate issue rather than widening this PR.
Correction to the PR body
Repo-wide deno fmt --check exits 0 on both origin/main and this branch (5158 files). deno.json's fmt.include covers src, cli, templates and react only, so docs/ is never in scope for the bare command. docs/guides/head-and-seo.md is unformatted only when named explicitly on the command line. It is untouched here either way, but the note as written sends the next reader chasing a failure that does not exist.
Gates, re-run after the last edit
deno task typecheck 0, deno task lint:ci 0, deno task docs:api-reference:check 0, deno fmt --check on the touched files 0, deno fmt --check repo-wide 0, deno test --preload=src/testing/preload.ts --no-check --allow-all src/agent/service/registration.test.ts 0. Also merged origin/main into the branch locally as a trial: clean merge, typecheck 0, tests green.
veryfront-issue-inbox#764
Same defect and the same prescribed fix, down to the wording ("wrap only the fetch call and the body read as transport failures, leave .parse outside"). This PR implements it. #764 should be closed when this merges, not worked separately. No other open PR touches src/agent/service/registration.ts, so there is no second fix in flight.
|
@codex review |
|
@coderabbitai review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Independent review: verified, merge-ready.Re-derived rather than taken on report, because this is a production change to a file that merged forty minutes earlier, written by the agent that found the bug, and no bot has reviewed it: CodeRabbit and Codex are both hard rate-limited on this account tonight. The red Scoping is correct
The Four mutations, all run here, all red
Baseline green: The claim worth confirming was that no mutation reddens more than one test, and it holds. Each row names exactly one failing test, so each is the positive control for the others: the schema fix does not detach #3990's body-read retry, and neither touches the hang-escalation deadline. Rows 3 and 4 are the same property from both directions, which is the pair verified when #3990 was reviewed. Row 1 is also the fail-first: reverting to what is on Not foundNo other caller of VerdictMerge-ready. Zero unresolved threads, zero non-gate check failures, and every guard proven capable of failing.
|
|
@codex review |
![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: e133407b45
ℹ️ 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".
| // match the schema is a permanent protocol mismatch, not a transient one. | ||
| // Wrapping it as a transport error would make every tick spend all three | ||
| // attempts on a response that will never parse. | ||
| const parsed = agentPushRuntimeServiceResponseSchema.parse(payload); |
There was a problem hiding this comment.
When a heartbeat returns HTTP 200 with valid but wrong-shaped JSON, this parse now exposes a raw schema-library error through the public heartbeat() promise. Consumers that handle failures by Veryfront error slug or serialize them through the standard error boundary therefore lose the stable classification that the parent implementation provided. Wrap the parse failure in a registered, non-retryable VeryfrontError instead, which preserves the intended single attempt without leaking the validator's error type.
AGENTS.md reference: AGENTS.md:L256-L258
Useful? React with 👍 / 👎.
2 participants
Follow-up to #3990, which is already merged. Not a competing fix. #3990's two behaviours both survive here; this narrows only the error classification that came with them.
Filed independently as veryfront/veryfront-issue-inbox#764.
What went wrong
#3990 moved
readAgentPushRuntimeServiceResponse(response)inside the transport-errortryinsendHeartbeatRequest, so a body read that fails after the headers arrive is retried. That was right. But the same function also runsagentPushRuntimeServiceResponseSchema.parse(...), which throws a rawZodError.isVeryfrontError(cause)is false for it, so the catch wrapped it asNETWORK_ERRORwith nohttpStatus, andisRetryableHeartbeatFailurereturns true wheneverhttpStatus === undefined.An HTTP 200 carrying a malformed body is a permanent protocol mismatch. It was being retried.
Measured, not inferred. The same probe on both trees, driving
lifecycle.heartbeat()against a fetch answering 200 with valid JSON of the wrong shape:Impact
Bounded, which is why this is a follow-up rather than a revert. Escalation is unaffected: three failed ticks still escalate at the same moment. The cost is 3x heartbeat requests and two spurious retry warnings per tick, and only while the control plane is serving malformed 200s.
The change
Move the transport mapping down to where the transport actually is.
readAgentPushRuntimeServiceResponsenow wraps its ownawait response.json(), so a failed body read still becomes a retryableNETWORK_ERROR. The schema parse sits outside that wrapper and keeps its own error class.sendHeartbeatRequestgoes back to wrapping only thefetchcall.Mutation matrix
Every mutation is of
src/agent/service/registration.ts, run against the whole registration test file. The new test isfails a heartbeat whose body does not match the schema, without retrying.main's shapetimeoutMsentirelytimeoutMs: 3_600_000Row 2 is the fail-first proof on current
main. Row 3 proves the narrowing did not detach #3990's body-read retry. Rows 4 and 5 confirm the hang-escalation deadline is untouched: both still producehung heartbeat attempts never reached persistent-failure escalation (timeout: 1500ms), the same diagnostic #3990 was verified with.Each row also doubles as a positive control for the others: no mutation turns more than one test red, so every red is that test's own assertion rather than a broken harness.
Unmutated, the file is green:
2 passed (16 steps) | 0 failed.Gates
deno task typecheck,deno task lint:ci,deno task docs:api-reference:check,deno fmt --checkon every touched file, anddeno test --preload=src/testing/preload.ts --no-check --allow-all src/agent/service/registration.test.tsall exit 0, re-run after the last edit.Repo-wide
deno fmt --checkfails ondocs/guides/head-and-seo.md, which is unformatted onmainalready and untouched here.docs/api-reference/veryfront/agent.mdis a one-line source-link update fromdeno task docs, not hand-edited.Refs veryfront/veryfront-issue-inbox#728
Follow-up commit
e133407b45An independent re-derivation found that narrowing
sendHeartbeatRequestalso droppedif (isVeryfrontError(cause)) throw cause;. That guard was needed for the response read, which moved out of the wrapper, but it also coveredfetchImplitself.fetchis a public option on the exportedCreateAgentServiceRegistrationLifecycleOptions, so a caller-supplied transport can reject with an error that already carries a slug and anhttpStatus, and that classification was being thrown away:The guard is back on the fetch catch only. The schema parse stays outside the wrapper, so the fix above is unchanged. A sixth mutation row covers it: dropping the restored guard turns only
keeps a fetch rejection that already carries an HTTP status out of the retry loopred, and no mutation reddens more than one test. Unmutated:2 passed (17 steps) | 0 failed.Correction to the gates note above. Repo-wide
deno fmt --checkexits 0 on bothorigin/mainand this branch (5158 files).deno.json'sfmt.includecoverssrc,cli,templatesandreactonly, sodocs/is never in scope for the bare command.docs/guides/head-and-seo.mdis unformatted only when named explicitly on the command line, and is untouched here either way.Details and the full probe matrix: #4008 (comment)