Repository navigation
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
Review of #8363 (head 1787879) is INCONCLUSIVE: the versioned diff was not fully re-materialized in this review window (returned as archived observations), and the frozen CI evidence shows passed=false with several e2e-case jobs still in_progress. Partial reads of key files (InstanceLeaseService, RegistrationLeaseProperties, HeartbeatListener, InstanceController) indicate a coherent opt-in server-issued registration-lease feature, but complete cross-file contract verification was not achieved.
Evidence
- Frozen PR evidence: 18 files changed (added RegistrationLeaseProperties, InstanceLeaseService, RegistrationLeaseProtocol, REGISTRATION_LEASE.md, and tests; modified InstanceController, InstanceCheckService, RegistrationProtocolInterceptor, HeartbeatListener, InstanceBeatInfoDTO, RegisterUtils).
- Frozen CI: check_count=67, passed=false, several e2e-case jobs status=in_progress with conclusion=null; cannot assert passing CI.
- Partial file reads of InstanceLeaseService.onBeat, RegistrationLeaseProperties (enabled/interval/min/max/expiryFactor), and HeartbeatListener.handleBeatResponse show an opt-in lease protocol with server-issued lease ids, but exact diff line coverage not confirmed.
Recommended next steps
- Re-materialize the full versioned diff via resource_diff across all offsets until has_more=false to confirm all 18 changed files are reviewed
- Wait for the in-progress e2e-case CI checks to finish and confirm ci.passed=true before merging
- Verify InstanceLeaseService.onBeat state machine and HeartbeatListener.handleBeatResponse against the REGISTRATION_LEASE.md contract for expiry/renew/stale edge cases
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: f51dd01c-b803-4505-aa07-f159393698cc · Commit: 17878798efa88b21a2967c2811a0514a1eaf2c44
Review verdict: INCONCLUSIVE
Full diff coverage not confirmed (versioned diff pages returned as archived observations, not materialized in this window) and CI not passing (ci.passed=false with e2e-case checks in_progress/conclusion null). Per policy, PASS requires both full diff coverage and passing CI; NEEDS_CHANGES requires concrete anchored HIGH/CRITICAL findings, none verified. Therefore the verdict is INCONCLUSIVE with no confirmed blocking defects.
Publication: COMMENT (REVIEW_EVIDENCE_MISSING)
Review details
I could not complete a full review of PR #8363. The versioned diff pages were not materialized in this session (returned as archived observations), and the frozen CI evidence shows passed=false with several e2e-case jobs still in_progress. Because PASS requires full diff coverage and passing CI, and no concrete HIGH/CRITICAL defect could be anchored to verified diff lines, the verdict is INCONCLUSIVE. Please re-run the complete diff read and wait for CI to turn green; the added lease service, properties, controller, interceptor and client heartbeat handling appear coherent but need full coverage confirmation.
|
/retest |
Part of #8038. Resolves #8039.
Problem
The
/instance/beatendpoint proves instance liveness by timestamping the last received beat, but nothing about that liveness is explicit: the endpoint answers with a baresuccessmarker, the offline and deletion thresholds are hard-coded constants inside the liveness check, no lease identity ties a beat to a negotiated agreement, and nothing tells the client when the instance will be considered gone or at which cadence it should renew. The registration fencing contract (boot-session-fencing-v1) explicitly leaves automatic lease expiry out of its scope, and the acceptance criteria of this issue call for lease identity, a negotiated heartbeat interval, server-side receipt time, an expiry deadline, idempotent renewal and pinned scenarios for duplicate, delayed and lost beats, clock skew, restart and final removal.Approach
This introduces the opt-in
registration-lease-v1capability on the existing beat endpoint. Whenshenyu.register.lease.enabledis false, no capability is advertised and every legacy byte of the v1 behavior is unchanged: the response stays the plainsuccessmarker, the thresholds stay the hard-coded constants, and the payload fields the client now may send are ignored by old Admins like any unknown JSON field.When enabled, the Admin advertises
registration-lease-v1next to the existing capabilities and answers a capability-presenting beat with a structured document:{code, leaseId, intervalSeconds, expiryDeadlineMs, message}. The server decides the interval — clamping any client proposal into the configured bounds — and the deadline isreceipt + expiryFactor * interval. Renewals are idempotent: the new deadline is the maximum of the previous deadline and the freshly computed one, so duplicate, delayed or replayed beats can never push the deadline past the last server receipt. A beat presenting an unknown lease id is rejected withSTALE_LEASE; renewing an already expired lease answersEXPIRED; both are terminal for the presented lease and never refresh the liveness timestamp, so a client that lost its lease cannot keep its instance alive without re-registering. A re-registration beat with a blank lease id attaches to the current lease of that instance without resetting its deadline, and once the lease has expired it grants a fresh one — the only path that recovers an expired instance. Fenced business addresses (boot-session-fencing-v1) stay out of lease scope and keep their short-circuit; external registries keep owning membership and expiry for their own registrations.The Java reporter keeps its fixed 5-second scheduler and gates each Admin by the server-issued interval — the smallest negotiable interval (2s) with a three-interval deadline can never be missed by the tick granularity. A renewal is compact: identity plus lease id, with the full instance observation attached only on (re-)registration and once every 12 consecutive renewals. Transport failures defer the next attempt by the current interval plus bounded jitter; there is no failback loop. Client-side the feature is separately opt-in (
registrationLease=true), so an upgraded client changes its wire behavior only when operators turn it on, and a mixed Admin cluster negotiates independently per server url. The lease state lives in the memory of the Admin that owns the liveness checks (the cluster master, which receives forwarded beats and rebuilds from the database on failover); an Admin restart invalidates outstanding leases and clients recover through the re-registration path, which is documented together with the fenced-owner release follow-up in the newREGISTRATION_LEASE.md.Testing
All timing-dependent scenarios run against a controlled clock or injected nanotime supplier — no test sleeps. The server tests cover grant and renewal with exact deadline arithmetic, monotonic deadlines under delayed or replayed renewals, idempotent duplicates, stale and expired rejections that leave the deadline untouched, re-registration recovery, proposal clamping, offline exactly at the lease deadline (and not before), compact renewals preserving the last observation, the fenced short-circuit, the exact legacy response without the capability header, and the capability advertisement. The client tests cover the legacy wire remaining byte-identical when the feature is off, the capability header and lease payload when on, honoring the server-issued interval, compact renewals, re-registration after
EXPIRED, the legacy cadence for a server answering the v1 marker, and bounded deferral after transport failures.shenyu-register-protocol2 tests,shenyu-register-client-beat36 tests (31 existing + 5 new),shenyu-admin2735 tests (2718 existing + 21 new; the singleShenyuClusterServiceTestfailure is the pre-existing flaky, green 9/9 when re-run alone), Checkstyle clean on all four touched modules.