fix(agent): re-register the push service when a heartbeat gets HTTP 404 - #4332
Conversation
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ Approved 2 resolved / 2 findingsDetects HTTP 404 on heartbeat and re-registers the push service instead of escalating to persistent failure. After re-registration succeeds, heartbeats resume and the consecutive-failure counter resets. Re-registration failures still count toward escalation, and existing 4xx non-retry, 5xx retry, and teardown semantics are unchanged. All tests pass, lint and typecheck are clean. ✅ 2 resolved✅ Edge Case: Endless re-register loop never escalates if 404s persist
✅ Quality: Re-registration failure branch has no test coverage
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
![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: a986035b0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c09631474
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Codex independent review for the exact head Finding, P2 medium:
Assessment:
Verdict: REQUEST_CHANGES until the rollout and post-deploy Sentry verification contract is added to the PR body or linked operational documentation. Score: 84/100 Review-Gate: |
|
Codex independent review for the exact head Findings:
Verification:
Verdict: APPROVE for code review. No actionable findings remain; merge still requires the pending check and repository review-gate conditions to complete. Score: 95/100 Review-Gate: |
Claude Code Review —
|
| File | +/- |
|---|---|
src/agent/service/registration.ts |
+90 |
src/agent/service/registration.test.ts |
+299 |
The server handler and durable-run-event-sink diffs visible in the base..head comparison are merge-base artifacts from PRs #4325 and #4326 landing on main after this branch was cut. They disappear with a rebase and are not reviewed here.
Correctness and completeness (38/40)
Recovery logic is sound. Traced every control-flow path at registration.ts:572–619:
isLostRegistrationHeartbeatFailure(registration.ts:496–498) correctly gates onNETWORK_ERROR.slug+ HTTP 404 — no false positives from other 4xx codes.- Re-registration uses
retryWithBackoffwithmaxAttempts: 1andtimeoutMs: Math.max(heartbeatIntervalMs, HEARTBEAT_MIN_ATTEMPT_TIMEOUT_MS)(registration.ts:578–587), bounding the recovery attempt to a single try with a deadline matching the existing heartbeat timeout floor. VerifiedHEARTBEAT_MIN_ATTEMPT_TIMEOUT_MSis 5000ms. - Bounded exemption:
awaitingHeartbeatAfterReregistrationstartsfalse. First recovery sets ittrueand adds the current promise torecoveredHeartbeats. A second 404 before any heartbeat succeeds finds the flag alreadytrue, skips the WeakSet add, and the interval handler incrementsconsecutiveHeartbeatFailuresnormally → escalation at 3 ticks. A successful heartbeat resets the flag (registration.ts:565), granting one fresh recovery. - Promise-scoped state:
recoveredHeartbeats(WeakSet<Promise<void>>) atregistration.ts:544keys recovery status to the exact shared promise captured atregistration.ts:644(const scheduledHeartbeat = heartbeat()). This eliminates the race where a direct caller's rejection handler starts a new heartbeat that clears mutable state before the interval's.catchreads it. Verified the WeakSet lookup atregistration.ts:648matches the interval-captured reference. - Stop guards: Four
if (stopped) returnchecks at lines 569, 589, 604, 615 cover every await boundary (heartbeat, re-registration success, re-registration failure, post-recovery). Late-arriving re-registrations are not adopted. - Failed recovery rethrows
registrationError(not the original 404), so the third-tick escalation reports the actual recovery failure (e.g. HTTP 500), not the stale 404. Confirmed by test assertion atregistration.test.ts:1262. lifecycle.serviceIdandlifecycle.serviceare updated atregistration.ts:597–598on successful recovery. TheAgentServiceRegistrationLifecycletype (registration.ts:196–201) declares both as plain mutable fields. The lifecycle object is aconstbinding to a plain object (registration.ts:670), so property assignment works correctly under strict mode.
Minor note (non-blocking): After successful re-registration the heartbeat promise still rejects with the original 404 via throw error at line 619. Direct callers of lifecycle.heartbeat() see a rejection even though recovery worked; the interval handler treats it as recovered via the WeakSet check. The PR body documents this as intentional ("a direct caller sees the failed beat"). The contract is correct but subtle — worth a code comment if callers multiply.
Tests (20/20)
Six new test cases in a dedicated "heartbeat recovery" describe block, all verified locally (27 steps, 10s):
| Test | What it proves |
|---|---|
| re-registers on 404 | Happy-path: second POST, adopted ID exposed on lifecycle.serviceId and lifecycle.service, recovered heartbeats succeed, no error-level log |
| repeated loss escalates | Bounded exemption: 4+ registration attempts, escalation at consecutiveFailures: 3 |
| failed re-registration escalates | Dead control plane: 3 warn logs + escalation at 3, error metadata reports HTTP 500 |
| hung re-registration times out | FakeTime: 3 abort events, escalation at 3 — proves the retryWithBackoff timeout works |
| recovery scoped to scheduled promise | Race condition: direct caller starts next heartbeat during recovery, no false escalation |
| post-stop non-adoption | Deferred fetch: stopped lifecycle retains original ID, no recovery log |
scriptedHeartbeatFetch helper cleanly extended with heartbeatResponse and registrationResponse callbacks and registrationAttempts() counter. recordingLogger extended to capture info entries.
Codecov: 90.32% patch coverage. The 4 missing + 2 partial lines are defensive stopped return guards and optional logger calls — low-risk branches that are structurally protected by the surrounding test scenarios.
Reliability and security (14/15)
- Teardown
AbortSignalthreaded through recovery registration (registration.ts:582) WeakSetfor recovery state prevents memory leaks from long-running lifecycles- No new external inputs, no new network-facing surface, no credential handling changes
- Minor: SonarQube quality gate shows 71.4% coverage < 80% required, but this is against the
0e2d942base which includes merge-base artifact files. After rebase, the gate should reflect the actual ~90% patch coverage. Thesonarcheck was stillIN_PROGRESSat review time.
Maintainability (14/15)
isLostRegistrationHeartbeatFailureis a focused, well-documented predicate mirroring the existingisRetryableHeartbeatFailurepattern- Recovery logic is contained within the existing heartbeat catch block; no new public API surface
- The lifecycle variable hoisting (
const lifecyclebefore return at line 670) is a clean structural change enabling recovery to synchronize fields - Minor: Two recovery-tracking mechanisms (
awaitingHeartbeatAfterReregistrationflag for bounding exemptions,recoveredHeartbeatsWeakSet for promise-scoped state) serve complementary purposes but increase cognitive load for future maintainers. Both are necessary — the flag tracks "has a heartbeat succeeded since last recovery" while the WeakSet tracks "which promise instance was recovered" — but a brief comment explaining their interaction would help.
Scope, docs, and rollout (9/10)
- PR body is thorough: root cause, red→green evidence, revert check, acceptance criteria, deployment gates, post-deploy Sentry verification with 24-hour quiet-period contract
- All 8 prior review threads are resolved with follow-up commits (
a986035througheb2a493) - The Codex review finding about missing deployment documentation has been addressed — the PR body now includes comprehensive deployment gates and Sentry verification sections
- Rebase needed before merge: the branch is 2 commits behind main (
#4325fail-closed shared-runtime gates,#4326durable-run terminal handling). No conflict is expected since the PR only touchesregistration.tsandregistration.test.ts.
Verification
| Check | Result |
|---|---|
deno task test:file src/agent/service/registration.test.ts |
ok, 3 passed (27 steps), 0 failed (10s) |
deno check registration.ts registration.test.ts |
clean |
deno lint registration.ts |
Checked 1 file |
| CI matrix (at review time) | All functional checks pass; sonar pending |
Merge-base diff (32dac4af8..eb2a4934) |
2 files only, confirms clean scope |
Gaps
- SonarQube gate resolution depends on rebase (informational, not a code defect)
- No integration-level test against a real control-plane stub returning 404 (unit coverage is sufficient for this fix)
Verdict
The implementation is correct, well-bounded, and thoroughly tested. Every edge case identified during prior reviews (unbounded recovery loop, hung registration, promise-scoped state race, post-stop adoption, writable lifecycle fields) has been addressed with a corresponding regression test. The only pre-merge requirement is a rebase onto current main.
Score: 95/100
Review-Gate:
Reviewer: Claude
Reviewed-SHA: eb2a493
Score: 95/100
Actionable-Findings: 0
Verdict: APPROVE
…stration (issue-inbox#873)
A heartbeat answered with 404 means the control plane no longer knows the service id (registry row evicted, environment reset, redeploy). The 404 is correctly non-retryable within a tick, but nothing ever registered the service again, so every subsequent tick failed and the lifecycle logged "Agent service heartbeat failing persistently" forever (Sentry VERYFRONT-AGENT-E, issue-inbox#873). On a lost-registration failure the lifecycle now registers again (the service_key upsert makes it idempotent), adopts the returned id for later heartbeats, and keeps the recovered tick out of the consecutive-failure counter. A failed re-registration still counts toward the escalation so a genuinely dead control plane surfaces persistently, and the teardown abort signal is threaded through the re-registration request.
eb2a493 to
d5adcaa
Compare
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Claude Code Review —
|
| File | +/- | Purpose |
|---|---|---|
src/agent/service/registration.ts |
+74 | Detect heartbeat 404, re-register, adopt new service row, isolate failure counter for recovered heartbeats |
src/agent/service/registration.test.ts |
+257 | 6 new recovery tests + extended test infrastructure |
6 commits showing clean TDD discipline (red first, then green, then hardening: timeout bounding, exposure of adopted registration, recovery state isolation).
Correctness (38/40)
Detection — isLostRegistrationHeartbeatFailure (:496-498) correctly narrows to NETWORK_ERROR + httpStatus === 404, matching the error shape produced by readAgentPushRuntimeServiceResponse (:368-372). No risk of misfiring on other error types or slugs.
Recovery path (:572-618) — On 404, calls retryWithBackoff with maxAttempts: 1 (single attempt + timeout, no actual retry). Timeout is Math.max(heartbeatIntervalMs, HEARTBEAT_MIN_ATTEMPT_TIMEOUT_MS), matching the heartbeat timeout logic. teardown.signal threaded through so stop() aborts in-flight re-registration.
State machine — Two flags control failure-counter exemption:
awaitingHeartbeatAfterReregistration(:543): only the first recovery is exempted; a second 404 before any heartbeat succeeds means the control plane is repeatedly losing registrations → counts toward escalation.recoveredHeartbeatsWeakSet (:544): tags the in-flight promise so the interval handler resets the counter instead of incrementing it.
Traced the state transitions for all scenarios:
- Happy path: 404 → re-register → recovered promise tagged → counter reset → next heartbeat uses new ID → succeeds →
awaitingHeartbeatAfterReregistrationcleared. - Repeated loss: first recovery exempted, subsequent 404s increment counter → escalation on 3rd.
- Failed re-registration:
throw registrationErrorexits the function; originalthrow errorunreachable; interval handler increments counter; escalation reports the registration error (more actionable than the 404).
Post-recovery throw: after successful re-registration, the heartbeat still throws the original 404 (throw error at :619). This is intentional — direct callers see the failed beat, but the interval handler recognizes the recovered promise and resets the counter. Subtle but correct.
Lifecycle mutation (:596-598): service (closure let), lifecycle.serviceId, and lifecycle.service are all updated, so subsequent heartbeats use the new service ID and external callers see the adopted row.
stop() guards: three if (stopped) return; checks after each async boundary (:589, :603-606, :614-616) plus the teardown signal abort. Verified by the "does not adopt or publish a re-registration that finishes after stop" test.
Minor deduction: the two-phase recovery state (awaitingHeartbeatAfterReregistration + recoveredHeartbeats) is correct but its invariants are distributed across ~30 lines. An inline summary comment would help future readers. Not actionable — the individual comments are adequate.
Tests (19/20)
6 new tests in "heartbeat recovery" describe block:
| # | Test | Scenario |
|---|---|---|
| 1 | re-registers instead of failing persistently | Happy path: 404 → re-register → new ID heartbeats → no escalation |
| 2 | escalates repeated losses | 404 every tick, registration always succeeds → escalation after 3 non-exempted ticks |
| 3 | counts failed re-registration toward escalation | Registration 500 → counts, reports registration error not 404 |
| 4 | times out hung re-registration | FakeTime, hung registration → 3 aborts → escalation |
| 5 | recovery scoped to scheduled heartbeat | Concurrency: direct caller starts next beat while scheduled one recovers → no false escalation |
| 6 | stop during pending re-registration | stop() → no adoption, no log |
Test infrastructure extensions (:308-368): recordingLogger now captures infos; scriptedHeartbeatFetch accepts heartbeatResponse/registrationResponse callbacks and tracks registrationAttempts(). Clean, backwards-compatible additions.
Minor gap: no test for 404 on the very first heartbeat after initial registration (extreme edge case). Code handles it correctly by construction (awaitingHeartbeatAfterReregistration starts false, so first recovery is always exempted).
Reliability & Security (15/15)
- Abort signal threaded through re-registration;
stop()semantics unchanged. retryWithBackoffwithmaxAttempts: 1bounds recovery to one attempt + timeout.- Failed re-registrations count toward escalation — a dead control plane still surfaces.
- No new auth surface; re-registration reuses
registerAgentPushRuntimeServicewith existing bearer auth. service_keyupsert idempotency is a server-side guarantee, correctly relied upon.
Maintainability (14/15)
isLostRegistrationHeartbeatFailureis a standalone, well-documented predicate.- Lifecycle object refactored from inline return to named
const— minimal and necessary for closure mutation. - Commit history (6 commits) is clean and tells the story.
- Minor: the
awaitingHeartbeatAfterReregistration/recoveredHeartbeatsinteraction could benefit from a brief 3-line state-diagram comment near their declarations.
Scope & Docs (8/10)
- PR body is excellent: root cause, scope, red→green evidence, issue link.
- Two files touched, tightly scoped to the fix.
- Existing behavior unchanged: per-tick 4xx non-retry, transient 5xx retry,
stop()semantics, escalation threshold. - Gap (informational, noted in prior review feat: expose additional AI SDK core exports #12): PR body omits deployment gates and post-deploy Sentry verification plan per the issue acceptance criteria.
Local verification
$ PATH=/Users/kentarowakayama/.deno-2.7.7/bin:$PATH deno check src/agent/service/registration.ts
Check src/agent/service/registration.ts ✅
$ PATH=/Users/kentarowakayama/.deno-2.7.7/bin:$PATH deno task test:file src/agent/service/registration.test.ts
ok | 3 passed (27 steps) | 0 failed (10s) ✅
All 27 steps across 3 describe blocks pass. Typecheck clean.
CI status
Typecheck, format, test-layout, sentry runtime packages, rsc browser e2e, proxy binary, npm smoke — all pass. Coverage shards, lint, integration, binary e2e — still pending at review time.
Review-Gate:
Reviewer: Claude
Reviewed-SHA: d5adcaa
Score: 94/100
Actionable-Findings: 0
Verdict: APPROVE
|
Codex independent review for exact head Findings: None. Assessment:
Verification:
Verdict: APPROVE for code review. Merge still requires the pending exact-head checks and repository review-gate conditions to complete. Score: 95/100 Review-Gate: |
|
1 participant
When the control plane answers a heartbeat with HTTP 404 for a previously registered service id (registry row evicted, reset, or redeployed), the registration lifecycle had no recovery path:
registerAgentPushRuntimeServicewas called exactly once at lifecycle creation, so every subsequent tick failed and the "Agent service heartbeat failing persistently" escalation fired forever. This change detects a heartbeat failure classified asNETWORK_ERRORwith upstream HTTP status 404 and re-runs registration inside the heartbeat catch (theservice_keyupsert makes it idempotent), adopts the returned service row for subsequent heartbeats, and resets the consecutive-failure counter on a recovered tick so the persistent-failure escalation never fires when re-registration works. Re-registration failures still count toward the escalation so a genuinely dead control plane surfaces, the teardownAbortControllersignal is threaded through the re-registration request, andstop()semantics, per-tick 4xx non-retry behavior, and the transient 5xx/transport retry schedule are unchanged.Fixes veryfront/veryfront-issue-inbox#873
Red
Green
Full directory suite and static checks:
Revert check
With the fix commit reverted (
git revert --no-commit 6dc734cd3, onlysrc/agent/service/registration.tsmodified, test commit untouched), the new test fails for the issue's reason:FAILED | 2 passed (21 steps) | 1 failed (1 step) (9s)— the persistent-failure error is logged and no second registration occurs. Restored to HEAD (git reset --hard 6dc734cd3), the same command passes:ok | 3 passed (22 steps) | 0 failed (9s).Acceptance criteria
/agent-runtimes/push-services) instead of counting ticks toward the persistent-failure escalation.stop()teardown still cancels in-flight work without counting as a failure.src/agent/service/registration.test.tsstay green; lint and typecheck pass.The inbox issue is labeled
repo:veryfront-agent, but the erroring code lives in this repo's framework atsrc/agent/service/registration.ts(veryfront-agent only consumes the lifecycle viasrc/agent/hosted/cloud-agent-chat-execution.ts), so the branch was cut here.Deployment gates
Post-deploy Sentry verification