Skip to content

[ISSUE #8039] Add the opt-in server-issued registration lease - #8363

Open
konlue wants to merge 1 commit into
apache:masterfrom
konlue:feat/8039-registration-lease
Open

konlue wants to merge 1 commit into
apache:masterfrom
konlue:feat/8039-registration-lease

Conversation

@konlue

@konlue konlue commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Part of #8038. Resolves #8039.

Problem

The /instance/beat endpoint proves instance liveness by timestamping the last received beat, but nothing about that liveness is explicit: the endpoint answers with a bare success marker, 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-v1 capability on the existing beat endpoint. When shenyu.register.lease.enabled is false, no capability is advertised and every legacy byte of the v1 behavior is unchanged: the response stays the plain success marker, 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-v1 next 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 is receipt + 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 with STALE_LEASE; renewing an already expired lease answers EXPIRED; 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 new REGISTRATION_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-protocol 2 tests, shenyu-register-client-beat 36 tests (31 existing + 5 new), shenyu-admin 2735 tests (2718 existing + 21 new; the single ShenyuClusterServiceTest failure is the pre-existing flaky, green 9/9 when re-run alone), Checkstyle clean on all four touched modules.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@konlue

konlue commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

/retest

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] Define server-issued registration leases and heartbeat expiry semantics for business instances

2 participants