[BugFix] fix backend node lastStartTime updated by hbTime unexpectedly (backport #52704)

fe/fe-core/src/main/java/com/starrocks/system/ComputeNode.java

@@ -496,17 +496,7 @@ public boolean handleHbResponse(BackendHbResponse hbResponse, boolean isReplay)
             }
 
             this.lastUpdateMs = hbResponse.getHbTime();
-            if (!isAlive.get()) {
-                isChanged = true;
-                // From version 2.5 we not use isAlive to determine whether to update the lastStartTime 
-                // This line to set 'lastStartTime' will be removed in due time
-                this.lastStartTime = hbResponse.getHbTime();
-                LOG.info("{} is alive, last start time: {}", this.toString(), hbResponse.getHbTime());
-                this.isAlive.set(true);
-            } else if (this.lastStartTime <= 0) {
-                this.lastStartTime = hbResponse.getHbTime();
-            }
-
+            // RebootTime will be `-1` if not set from backend.
             if (hbResponse.getRebootTime() > this.lastStartTime) {
                 this.lastStartTime = hbResponse.getRebootTime();
                 isChanged = true;
@@ -516,6 +506,18 @@ public boolean handleHbResponse(BackendHbResponse hbResponse, boolean isReplay)
                 becomeDead = true;
             }
 
+            if (!isAlive.get()) {
+                isChanged = true;
+                if (hbResponse.getRebootTime() == -1) {
+                    // Only update lastStartTime by hbResponse.hbTime if the RebootTime is not set from an OK-response.
+                    // Just for backwards compatibility purpose in case the response is from an ancient version
+                    this.lastStartTime = hbResponse.getHbTime();
+                }
+                LOG.info("{} is alive, last start time: {}, hbTime: {}", this.toString(), this.lastStartTime,
+                        hbResponse.getHbTime());
+                this.isAlive.set(true);
+            }
+
             if (this.cpuCores != hbResponse.getCpuCores()) {
                 isChanged = true;
                 this.cpuCores = hbResponse.getCpuCores();

fe/fe-core/src/test/java/com/starrocks/system/ComputeNodeTest.java

@@ -18,6 +18,7 @@
 import com.starrocks.common.Config;
 import com.starrocks.qe.CoordinatorMonitor;
 import com.starrocks.system.HeartbeatResponse.HbStatus;
+import com.starrocks.thrift.TStatusCode;
 import mockit.Expectations;
 import org.junit.Assert;
 import org.junit.Test;
@@ -118,4 +119,55 @@ public void testUpdateStartTime() {
         Assert.assertTrue(node.getLastStartTime() == 1000000L);
         Assert.assertTrue(needSync);
     }
+
+    BackendHbResponse generateHbResponse(ComputeNode node, TStatusCode tStatusCode, long rebootTimeSecs) {
+        if (tStatusCode != TStatusCode.OK) {
+            return new BackendHbResponse(node.getId(), "Unknown Error");
+        }
+        BackendHbResponse hbResponse =
+                new BackendHbResponse(node.getId(), node.getBePort(), node.getHttpPort(), node.getBrpcPort(),
+                        node.getStarletPort(), System.currentTimeMillis(), node.getVersion(), node.getCpuCores());
+        hbResponse.setRebootTime(rebootTimeSecs);
+        return hbResponse;
+    }
+
+    @Test
+    public void testComputeNodeLastStartTimeUpdate() {
+        int previousRetryTimes = Config.heartbeat_retry_times;
+        // no retry, set to not_alive for the first hb failure
+        Config.heartbeat_retry_times = 0;
+
+        long nodeId = 1000;
+        ComputeNode node = new ComputeNode(nodeId, "127.0.0.1", 9050);
+        long rebootTimeA = System.currentTimeMillis() / 1000 - 60;
+        BackendHbResponse hbResponse;
+
+        hbResponse = generateHbResponse(node, TStatusCode.OK, rebootTimeA);
+        Assert.assertTrue(node.handleHbResponse(hbResponse, false));
+        Assert.assertEquals(rebootTimeA * 1000, node.getLastStartTime());
+        Assert.assertTrue(node.isAlive());
+
+        // simulate that a few intermittent heartbeat probing failures due to high load or unstable network.
+        // FE marks the BE as dead and then the node is back alive
+        hbResponse = generateHbResponse(node, TStatusCode.THRIFT_RPC_ERROR, 0);
+        Assert.assertTrue(node.handleHbResponse(hbResponse, false));
+        Assert.assertFalse(node.isAlive());
+        // BE reports heartbeat with the same rebootTime
+        hbResponse = generateHbResponse(node, TStatusCode.OK, rebootTimeA);
+        Assert.assertTrue(node.handleHbResponse(hbResponse, false));
+        Assert.assertTrue(node.isAlive());
+        // reboot time should not change
+        Assert.assertEquals(rebootTimeA * 1000, node.getLastStartTime());
+
+        // response an OK status, but the rebootTime changed.
+        // This simulates the case that the BE is restarted quickly in between the two heartbeat probes.
+        long rebootTimeB = System.currentTimeMillis() / 1000;
+        hbResponse = generateHbResponse(node, TStatusCode.OK, rebootTimeB);
+        Assert.assertTrue(node.handleHbResponse(hbResponse, false));
+        Assert.assertTrue(node.isAlive());
+        // reboot time changed
+        Assert.assertEquals(rebootTimeB * 1000, node.getLastStartTime());
+
+        Config.heartbeat_retry_times = previousRetryTimes;
+    }
 }