[dengliming]

Fixes #6590.

Injects a Clock into InstanceCheckService while preserving the production system-clock constructor. The heartbeat test now supplies exact timestamps and no longer relies on Thread.sleep(1) or wall-clock resolution. Health-state checks also use one consistent time snapshot per run.

Tests:

• InstanceCheckServiceTest
• Checkstyle

[Aias00]

Good use of java.time.Clock — this is the standard way to make time-dependent logic testable, and it removes a Thread.sleep(1) from the suite.

What I checked:

• The public constructor delegates to a new package-private InstanceCheckService(instanceInfoService, Clock) with Clock.systemDefaultZone(), so Spring wiring and production behaviour are unchanged.
• All three time reads now share one injected clock: handleBeatInfo (both branches) and both comparisons in doCheck. Capturing currentTime once per doCheck pass is also slightly more correct than calling System.currentTimeMillis() per instance, since every instance in a sweep is now judged against the same instant.
• The test no longer relies on wall-clock deltas; it stubs millis() to return 1000L, 2000L and asserts exact values, which is deterministic.

Bonus cleanup: the duplicated instanceInfoVO.setLastHeartBeatTime(System.currentTimeMillis()) call in the "new instance" branch was removed. It was setting the same field twice to the same value, so dropping it is safe.

Non-blocking: Clock.millis() is used rather than instant(), which is fine here since only epoch millis are needed. If this class ever needs zone-aware logic, Clock is already the right abstraction to extend.

[Aias00]

Hi @dengliming, this PR currently has merge conflicts with the master base branch, so CI cannot validate it against the latest code. Please rebase/merge the latest master and resolve the conflicts, then push the updated branch. Once the conflicts are cleared, CI will re-run automatically and we can continue the review. Thanks!

[Aias00]

Re-approving. My earlier approval was auto-dismissed after a new commit, so I re-reviewed the current diff.

This is a clean de-flaking change for the instance heartbeat test. InstanceCheckService now accepts an injectable Clock (default Clock.systemDefaultZone()) via a package-private constructor, and all timestamp reads (handleBeatInfo, doCheck) use clock.millis() instead of System.currentTimeMillis(). The test swaps in a mocked Clock with fixed values (when(clock.millis()).thenReturn(1000L, 2000L)) and asserts deterministic values (assertEquals(1000L, ...), assertEquals(2000L, ...)) instead of the previous Thread.sleep(1) + greaterThan flakiness.

The production behavior is unchanged for the default clock, and the package-private constructor is only for tests. This is the correct way to make time-based logic deterministic.

No blocking issues. Approving.

[Aias00]

Direction is good — replacing Thread.sleep(1) and System.currentTimeMillis() with an injected Clock is the right way to make the heartbeat timeout deterministic. But this PR currently breaks shenyu-admin startup, which cascades through almost the whole pipeline.

Blocker: InstanceCheckService is no longer instantiable by Spring

pr_build fails in shenyu-admin:

[shenyu-admin] Tests run: ... Errors: ... <<< FAILURE!
java.lang.IllegalStateException: Failed to load ApplicationContext for
    [testClass = org.apache.shenyu.admin.discovery.ZookeeperClientDiscoveryIntegrationTest, ...]

Caused by: UnsatisfiedDependencyException: Error creating bean with name 'instanceController'
  ... through constructor parameter 1:
Caused by: BeanCreationException: Error creating bean with name 'instanceCheckService':
    Failed to instantiate [org.apache.shenyu.admin.service.impl.InstanceCheckService]: No default constructor found
Caused by: java.lang.NoSuchMethodException: org.apache.shenyu.admin.service.impl.InstanceCheckService.<init>()

The class now has two constructors:

public InstanceCheckService(final InstanceInfoService instanceInfoService) { ... }

InstanceCheckService(final InstanceInfoService instanceInfoService, final Clock clock) { ... }

Spring only performs implicit constructor injection when a bean has exactly one constructor. With two, it falls back to a no-arg constructor, which does not exist. The minimal fix is to mark the public one:

@Autowired
public InstanceCheckService(final InstanceInfoService instanceInfoService) {
    this(instanceInfoService, Clock.systemDefaultZone());
}

(Annotating the two-arg constructor instead also works, but the production path should be the one that carries the annotation.)

This failure is the root cause of the other red jobs

Once instanceCheckService cannot be created, every @SpringBootTest in shenyu-admin fails during context startup (the whole batch reports Time elapsed: 0.001 s errors right after ZookeeperClientDiscoveryIntegrationTest), and the fallout reaches:

• build (shenyu-integrated-test-http) → dependency failed to start: container shenyu-admin is unhealthy (admin starts, then is reported unhealthy 7s later because the app never came up).
• build (shenyu-integrated-test-apache-dubbo|combination|grpc|https|rewrite|sdk-*) — same, all in the 6m2x range.
• e2e (2s, 255) is only an aggregator; e2e-case (...) and e2e-storage fail downstream.
• list-contract, registry_dependencies, sql-matrix are very likely the same cascade — build (3s) is just the status aggregator.

So one @Autowired should turn the pipeline green; please re-run everything afterwards rather than only pr_build.

Review notes on the change itself

• Hoisting clock.millis() out of the forEach loop in doCheck() is a real improvement: every instance in one sweep is now judged against the same instant instead of drifting by a few microseconds.
• In the test, when(clock.millis()).thenReturn(1000L, 2000L) relies on consecutive-stub semantics; any third call silently returns 2000L. That is fine today because the test never triggers doCheck(), but a following test that does would inherit a frozen clock. thenReturn(1000L, 2000L) plus an explicit verify(clock, times(2)).millis() would pin the contract down.
• The other tests in InstanceCheckServiceTest still go through @InjectMocks, i.e. the real Clock.systemDefaultZone(). If the intent is full determinism for this service, consider a field default plus ReflectionTestUtils.setField in the tests that care about time — you already import ReflectionTestUtils in this file.
• Minor: the added package-private constructor has no Javadoc, which is fine for checkstyle, but a short // visible for testing note would help future readers understand why Clock is injectable while nothing else is.

[Aias00]

Re-review after 6627a188

The NoSuchMethodException: InstanceCheckService.<init>() startup failure is fixed, and the fix is verified by a test that would have caught it.

Verified

1. @Autowired on the production constructor

@Autowired
public InstanceCheckService(final InstanceInfoService instanceInfoService) {
    this(instanceInfoService, Clock.systemDefaultZone());
}

// Visible for tests that need a controlled clock.
InstanceCheckService(final InstanceInfoService instanceInfoService, final Clock clock) {

This is the correct fix. Spring's implicit single-constructor injection stops applying as soon as a second constructor exists, so the container fell back to looking for a no-argument constructor and threw. Annotating the production constructor restores explicit selection while the package-private clock constructor stays test-only.

2. The new test actually exercises container assembly

testSpringCreatesServiceWithProductionClock registers the bean in a real AnnotationConfigApplicationContext and refreshes it, rather than calling the constructor directly. That is exactly the path that broke — a unit test calling new InstanceCheckService(mock) would never have failed. Good call.

I checked that the test is safe to run: setup() carries no @PostConstruct, so refresh() does not start the ScheduledThreadPoolExecutor. context.close() does trigger @PreDestroy close(), which calls syncDB() → instanceInfoService.createOrUpdate(vo) on the mock (a void call, no NPE) and then guards the null executor with Objects.nonNull(executor). No leaked threads, no failing teardown.

3. verify(clock, times(2)).millis() matches the implementation

handleBeatInfo samples the clock exactly once per call, and the test performs two calls, so two invocations is right. It also pins the useful property that a single round of checks uses one time snapshot instead of re-reading the clock per instance.

CI

All 11 checks are still QUEUED for d33ce570, so I cannot confirm the shenyu-admin context starts cleanly yet — that was the whole point of this change. Please let the run finish; I will merge once it is green.

One thing to be aware of when the run lands: the branch merged master after #7903, which introduced an unrelated regression in shenyu-integrated-test-http (CryptorResponsePluginTest.testWhenDoesntExistFiled, Expected: is "10001" but: was null). I am fixing that on master separately. If this PR's build (shenyu-integrated-test-http) fails on that same test, it is not caused by your change.

Approving on the code; the merge is gated on the pending run.

[Aias00]

Ping on state: this PR is approved on the code, but pr_build fails and it cannot be merged yet.

Failing test in shenyu-admin:

org.apache.shenyu.admin.controller.RegistrationProtocolCompatibilityTest.acceptsV1Fixtures

All four parameterized variants fail. Since this PR controls the instance heartbeat time, it is likely the test fixture cadence rather than the registration contract. Please check whether the fixture needs to be adjusted to the new heartbeat interval, and run the class locally to confirm.

[Aias00]

@dengliming This PR is approved, but the pr_build check is red. I looked at that job on several PRs today and it fails in shared test setup rather than in the diff: the MQTT SubscribeTest fixture throws NullPointerException: Cannot invoke "Object.hashCode()" because "key" is null at SubscribeTest.java:75/90 because the repositories are not registered in Singleton.INST (#8075 fixes exactly that), and another variant dies on a stale shenyu-register-client-beat artifact.

Please rebase onto master and push so CI re-runs. If pr_build is still red afterwards, paste the failing test and I'll take it from there.

[Aias00]

Hi @dengliming, this PR is already approved, but it currently cannot be merged. Here is the blocker and what is needed:

Blocker
This PR is unmergeable (conflict) against master, and the build / pr_build / registry_dependencies failures are from that stale base.

What we need
Please rebase onto the latest master, resolve conflicts, and re-run CI.

Once the branch is up to date and CI is green, we will squash-merge it. Thanks!

[Aias00]

Re-reviewed the current head. The focused feature tests pass, but an existing test affected by the changed production contract still fails in both CI and an exact-head local reproduction. See the inline finding.

[Aias00]

The registration compatibility fixture now explicitly creates the instance-check spy with the required dependencies, addressing the previous Mockito constructor-injection failure. Re-reviewed Clock-based liveness behavior and retention integration; no remaining actionable finding. Focused instance-check, registration compatibility and long-polling tests: 164 tests passed.

Checkstyle and a separate Apache RAT check passed locally. This review covers source and the stated targeted verification; full Docker integration/E2E suites were not run locally.