[konlue]

Fixes #7692.

Summary

InstanceCheckService#handleBeatInfo updates only lastHeartBeatTime for an already-known instance identity and drops the instanceInfo payload the beat carries. A restarted process on the same address reporting new metadata (or any periodic metadata change) is therefore invisible: the cached entry — and, via the scheduled syncDB(), the persisted row — keeps the payload captured at first sighting.

Modifications

• In the existing-identity branch of handleBeatInfo, refresh instanceInfo when the incoming beat carries a non-blank payload. Liveness-only beats (blank payload) keep the last known value rather than wiping it.
• Remove a duplicated setLastHeartBeatTime call in the create branch (it was invoked twice with the same value).

Verifying this change

• New InstanceCheckServiceTest#testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity: same identity reports version=1.0 then version=2.0; asserts the cached entry observes the refreshed value. Fails on the current code with expected: <version=2.0> but was: <version=1.0> and passes with the fix.

• New InstanceCheckServiceTest#testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank: a follow-up beat with a blank payload keeps the last known metadata.

• Targeted tests pass:

  ./mvnw -B -ntp -pl shenyu-admin test -Dtest=InstanceCheckServiceTest

  14/14 green.

• Checkstyle passes:

  ./mvnw -B -ntp -pl shenyu-admin checkstyle:check

  clean.

Notes

Identity (ip:port@type#namespace) is unchanged; only the metadata refresh behaviour within an existing entry changes. No schema or wire-format change.

[Aias00]

LGTM. The bug and the fix are both narrow, and dropping the duplicated setLastHeartBeatTime is a tidy bonus.

Evidence

• StringUtils is already imported in InstanceCheckService (used by getInstanceDataVisual), so the guard compiles without touching imports.
• The blank-payload guard is the right default: HttpLongPollingDataChangedListener and WebsocketCollector both report with instanceInfo derived from a header/map entry that can be empty, and a liveness-only beat must not wipe the metadata captured at first sighting. The two new tests pin both directions, and the first one is genuinely red on master.
• Identity is untouched (ip:port@type#namespace), so syncDB/createOrUpdate and the dashboard overlay behave exactly as before; only the payload carried by an existing entry changes.

Cross-PR note (worth acting on, it is cheap)

HttpLongPollingDataChangedListener#reportInstanceInfo builds instanceInfo as GsonUtils.toJson(bootstrapInfo) where bootstrapInfo is StringUtils.defaultString(header, ""). A client that does not send that header therefore publishes the JSON string "" — two characters, not blank — which now overwrites whatever instanceInfo the beat channel had stored for the same identity. Suggest either GsonUtils.toJson(StringUtils.defaultIfBlank(bootstrapInfo, null)) or skipping the payload when the header is absent, so a header-less client cannot erase real system info. This only shows up when #7721 and the long-polling listener are read together, which is why I am flagging it here.

Nit: the "keep last known value" rule is now an implicit contract between three producers. A one-line comment on the isNotBlank guard ("liveness-only beats must not wipe metadata") would stop someone from "simplifying" it later.

CI: pr_build green.

[Aias00]

Worth a comment here: liveness-only beats must not wipe metadata, which is now an implicit contract across three producers (beat channel, websocket collector, long polling). One related edge case: HttpLongPollingDataChangedListener builds the payload as GsonUtils.toJson(bootstrapInfo) where a missing header yields "" (the two-character JSON string), which is not blank and will therefore overwrite real metadata. Normalising that producer to toJson(defaultIfBlank(bootstrapInfo, null)) would close it.

[Aias00]

Heads-up: master is currently red because two assertions added in this PR contradict the instance-info contract that #7785 established an hour earlier.

Failure

test_group (admin-sync, shenyu-admin, ...) on master (de93952d) fails with:

InstanceCheckServiceTest.testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity:178
  expected: <version=1.0> but was: <{}>
InstanceCheckServiceTest.testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank:198
  expected: <version=1.0> but was: <{}>

This is not a flake and it is not caused by #7749 — that PR simply inherits the same two failures (it also runs 7 additional tests of its own, all of which pass).

Root cause

#7785 (merged as 7086afaf) changed the insert path in InstanceCheckService.handleBeatInfo to normalize what the beat reports:

instanceInfoVO.setInstanceInfo(InstanceInfoUtils.normalize(instanceBeatInfoDTO.getInstanceInfo()));

and pinned the contract in testInvalidOptionalInformationDoesNotStopHeartbeats:

@ValueSource(strings = {"{broken}", "[]", "true", "not-json"})
...
assertEquals("{}", actual.getInstanceInfo());

"version=1.0" in the two tests added here is exactly not-json, so normalize returns {} and both assertions fail. The two contracts cannot both hold.

Requested change

Please use a JSON object payload in the two tests, which keeps their actual intent (metadata is refreshed on repeated heartbeats) while conforming to the storage contract:

@Test
void testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity() {
    InstanceBeatInfoDTO first = buildDTO("127.0.0.1", "8080", "grpc", "ns");
    first.setInstanceInfo("{\"version\":\"1.0\"}");
    instanceCheckService.handleBeatInfo(first);
    InstanceInfoVO cached = instanceCheckService.getInstanceHealthBeatInfo(first);
    assertNotNull(cached);
    assertEquals("{\"version\":\"1.0\"}", cached.getInstanceInfo());

    InstanceBeatInfoDTO second = buildDTO("127.0.0.1", "8080", "grpc", "ns");
    second.setInstanceInfo("{\"version\":\"2.0\"}");
    instanceCheckService.handleBeatInfo(second);
    assertEquals("{\"version\":\"2.0\"}", cached.getInstanceInfo());
}

@Test
void testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank() {
    InstanceBeatInfoDTO first = buildDTO("127.0.0.1", "8080", "grpc", "ns");
    first.setInstanceInfo("{\"version\":\"1.0\"}");
    instanceCheckService.handleBeatInfo(first);
    InstanceInfoVO cached = instanceCheckService.getInstanceHealthBeatInfo(first);
    assertNotNull(cached);

    InstanceBeatInfoDTO blank = buildDTO("127.0.0.1", "8080", "grpc", "ns");
    blank.setInstanceInfo("");
    instanceCheckService.handleBeatInfo(blank);
    assertEquals("{\"version\":\"1.0\"}", cached.getInstanceInfo());
}

normalize returns the canonical form unchanged for an already-compact JSON object, so no production change is needed for this to pass.

One related inconsistency I will raise separately on #7785: the containsKey branch stores the reported value raw while the insert branch normalizes, so a non-JSON value flips between {} and the original text across heartbeats. That is #7785 territory, not yours.

If you believe raw pass-through of non-JSON text is the contract you want instead, say so here — then the right move is to revert the normalize call on the store path rather than change these assertions, and we should reconcile the two contracts in one place. Either way master needs to go green before the 2.7.2 cut.

[konlue]

@Aias00 PTAL. Fixed in #7846 — payloads switched to json objects per your comment.