[KLC-2405] address PR #60 review: atomic snapshot + Unix guard + comment fix

- Add ClockSnapshot() (time.Duration, time.Time) to ntp.SyncTimer.
  Returns offset + lastSync under a single RLock so the polling code
  can publish them as a coherent pair. The previous two-call read had
  a microseconds-wide window where a sync could land between the two
  getter calls and mix offset/timestamp across two sync passes --
  vanishingly improbable but cleaner to remove entirely.

- Guard uint64(ts.Unix()) against negative seconds. A pre-1970 RTC
  after a successful sync would otherwise wrap to ~1.8e19 and silently
  defeat any time()-value>threshold stale-sync alarm. Now publishes 0.

- Rewrite the misleading "float64 round-trip" comment on SetInt64Value.
  The float64 conversion exists in cmd/connector/provider, not in the
  /node/metrics writer (statusHandler/statusMetricsProvider.go), which
  formats stored int64/uint64 directly with %v. Real reason for
  SetInt64Value is that uint64 cannot represent negative offsets.

Tests:
- ntp/syncTime_test.go: TestClockSnapshotReturnsConsistentPair asserts
  the snapshot matches individual getters post-sync.
- cmd/node/metrics/clockMetrics_test.go:
  TestBuildClockMetricsPollingFunc_UsesAtomicSnapshot fails the test
  if the polling code falls back to the legacy getters.
  TestBuildClockMetricsPollingFunc_NegativeUnixGuard verifies a 1969
  timestamp publishes 0, not the uint64 wrap-around.

Addresses review threads from @copilot and @coderabbitai on PR #60.

Author: fbsobreira

cmd/node/metrics/clockMetrics.go

@@ -53,18 +53,26 @@ func StartClockMetricsPolling(
 
 func buildClockMetricsPollingFunc(syncTimer ntp.SyncTimer) func(core.AppStatusHandler) {
 	return func(appStatusHandler core.AppStatusHandler) {
+		// Read offset and last-sync timestamp under a single RLock so a scrape
+		// can't observe a fresh offset paired with a stale timestamp (or vice
+		// versa) if a sync lands between two separate getter calls.
+		offset, lastSync := syncTimer.ClockSnapshot()
+
 		// ClockOffset is signed: negative means OS clock is fast relative to NTP
-		// servers, positive means it is slow. Route through SetInt64Value so the
-		// sign survives the float64 round-trip done by the metrics provider.
-		appStatusHandler.SetInt64Value(core.MetricClockOffsetNs, syncTimer.ClockOffset().Nanoseconds())
+		// servers, positive means it is slow. SetInt64Value (not SetUInt64Value)
+		// is required because uint64 can't represent negative values.
+		appStatusHandler.SetInt64Value(core.MetricClockOffsetNs, offset.Nanoseconds())
 
 		// Zero before the first successful sync — publish 0 explicitly so
 		// `time() - value > threshold` alarms only fire once a sync has
-		// actually happened (avoids the "node booted but NTP is broken" alarm
-		// firing identically to "node has been up for years without resync").
+		// actually happened. The unix > 0 guard defends against a pathological
+		// pre-1970 RTC: uint64(negative-int64) wraps to a huge value that
+		// would silently keep "stale-sync" alarms quiet forever.
 		var lastSyncUnix uint64
-		if ts := syncTimer.LastSyncTimestamp(); !ts.IsZero() {
-			lastSyncUnix = uint64(ts.Unix()) // #nosec G115 -- unix seconds fit in uint64 well beyond year 2262
+		if !lastSync.IsZero() {
+			if unix := lastSync.Unix(); unix > 0 {
+				lastSyncUnix = uint64(unix)
+			}
 		}
 		appStatusHandler.SetUInt64Value(core.MetricClockLastSyncTimestamp, lastSyncUnix)
 	}

cmd/node/metrics/clockMetrics_test.go

@@ -167,6 +167,59 @@ func TestBuildClockMetricsPollingFunc_LastSyncTimestampPublished(t *testing.T) {
 	assert.Equal(t, uint64(syncTs.Unix()), cap.u64[core.MetricClockLastSyncTimestamp])
 }
 
+func TestBuildClockMetricsPollingFunc_NegativeUnixGuard(t *testing.T) {
+	// Pathological case: a non-zero timestamp whose Unix() is negative (pre-1970
+	// RTC after a successful sync). Without the guard, uint64(negative-int64)
+	// wraps to ~1.8e19, which would silently bypass any "stale sync" alarm
+	// expressed as `time() - value > threshold`. Must publish 0 instead.
+	preEpoch := time.Date(1969, 6, 1, 0, 0, 0, 0, time.UTC)
+	require.True(t, preEpoch.Unix() < 0, "test setup: timestamp must precede 1970")
+
+	cap, stub := newCaptureHandler()
+	syncTimer := &consensusMock.SyncTimerMock{
+		LastSyncTimestampCalled: func() time.Time { return preEpoch },
+	}
+
+	pollingFunc := buildClockMetricsPollingFunc(syncTimer)
+	pollingFunc(stub)
+
+	cap.mu.Lock()
+	defer cap.mu.Unlock()
+	assert.Equal(t, uint64(0), cap.u64[core.MetricClockLastSyncTimestamp],
+		"negative Unix seconds must publish 0, not the uint64 wrap-around")
+}
+
+func TestBuildClockMetricsPollingFunc_UsesAtomicSnapshot(t *testing.T) {
+	// Verify the polling func reads (offset, lastSync) via the single-RLock
+	// snapshot, not as two independent getter calls. Forcing the legacy getters
+	// to fail the assertion proves the snapshot path is what publishes the metrics.
+	const offset = 1234 * time.Nanosecond
+	syncTs := time.Unix(1_700_000_000, 0)
+
+	cap, stub := newCaptureHandler()
+	syncTimer := &consensusMock.SyncTimerMock{
+		ClockOffsetCalled: func() time.Duration {
+			t.Errorf("ClockOffset() must not be called — polling must use ClockSnapshot()")
+			return 0
+		},
+		LastSyncTimestampCalled: func() time.Time {
+			t.Errorf("LastSyncTimestamp() must not be called — polling must use ClockSnapshot()")
+			return time.Time{}
+		},
+		ClockSnapshotCalled: func() (time.Duration, time.Time) {
+			return offset, syncTs
+		},
+	}
+
+	pollingFunc := buildClockMetricsPollingFunc(syncTimer)
+	pollingFunc(stub)
+
+	cap.mu.Lock()
+	defer cap.mu.Unlock()
+	assert.Equal(t, offset.Nanoseconds(), cap.i64[core.MetricClockOffsetNs])
+	assert.Equal(t, uint64(syncTs.Unix()), cap.u64[core.MetricClockLastSyncTimestamp])
+}
+
 // TestClockMetricsAppearInPrometheusOutput is an end-to-end check that
 // exercises the real statusHandler + clockMetrics polling combination,
 // ensuring both metrics appear in the /node/metrics scrape output with the

core/consensus/mock/syncTimerMock.go

@@ -8,6 +8,7 @@ import (
 type SyncTimerMock struct {
 	ClockOffsetCalled       func() time.Duration
 	LastSyncTimestampCalled func() time.Time
+	ClockSnapshotCalled     func() (time.Duration, time.Time)
 	CurrentTimeCalled       func() time.Time
 }
 
@@ -34,6 +35,18 @@ func (stm *SyncTimerMock) LastSyncTimestamp() time.Time {
 	return time.Time{}
 }
 
+// ClockSnapshot returns the configured (offset, lastSync) pair under a single
+// callback so tests can assert atomic-pair semantics. Falls back to the
+// individual getters when no explicit snapshot stub is set, which preserves
+// the existing behavior for tests that only configure one or both fields.
+func (stm *SyncTimerMock) ClockSnapshot() (time.Duration, time.Time) {
+	if stm.ClockSnapshotCalled != nil {
+		return stm.ClockSnapshotCalled()
+	}
+
+	return stm.ClockOffset(), stm.LastSyncTimestamp()
+}
+
 // FormattedCurrentTime method gets the formatted current time on which is added a given offset
 func (stm *SyncTimerMock) FormattedCurrentTime() string {
 	return time.Unix(0, 0).String()

ntp/interface.go

@@ -10,6 +10,7 @@ type SyncTimer interface {
 	StartSyncingTime()
 	ClockOffset() time.Duration
 	LastSyncTimestamp() time.Time
+	ClockSnapshot() (offset time.Duration, lastSync time.Time)
 	FormattedCurrentTime() string
 	CurrentTime() time.Time
 	IsInterfaceNil() bool

ntp/syncTime.go

@@ -284,6 +284,19 @@ func (s *syncTime) LastSyncTimestamp() time.Time {
 	return ts
 }
 
+// ClockSnapshot returns the current offset and last-sync timestamp under a
+// single RLock so callers see a coherent pair. Without this, a sync landing
+// between two separate getter calls could mix a fresh offset with a stale
+// timestamp on the same scrape.
+func (s *syncTime) ClockSnapshot() (offset time.Duration, lastSync time.Time) {
+	s.mut.RLock()
+	offset = s.clockOffset
+	lastSync = s.lastSyncTimestamp
+	s.mut.RUnlock()
+
+	return
+}
+
 // recordSyncSuccess updates offset and timestamp under a single lock so
 // /node/metrics scrapes can't observe a fresh offset paired with a stale
 // timestamp (or vice versa) mid-update.

ntp/syncTime_test.go

@@ -398,3 +398,33 @@ func TestLastSyncTimestampStampedOnSuccess(t *testing.T) {
 	assert.False(t, ts.Before(before), "timestamp must be set on or after the sync started")
 	assert.False(t, ts.After(after), "timestamp must not be set in the future")
 }
+
+func TestClockSnapshotReturnsConsistentPair(t *testing.T) {
+	t.Parallel()
+
+	// After a successful sync, ClockSnapshot must return the offset and
+	// timestamp written by that same sync — they're set together under one
+	// lock in recordSyncSuccess and must be read together for callers
+	// publishing them as paired metrics.
+	st := ntp.NewSyncTime(
+		config.NTPConfig{
+			SyncPeriodSeconds: 1,
+			Hosts:             []string{"host1"},
+		},
+		func(options ntp.NTPOptions, hostIndex int) (*beevikNtp.Response, error) {
+			return &beevikNtp.Response{ClockOffset: 23456}, nil
+		},
+	)
+
+	offset, ts := st.ClockSnapshot()
+	assert.Equal(t, time.Duration(0), offset, "no sync yet, offset is zero")
+	assert.True(t, ts.IsZero(), "no sync yet, timestamp is zero")
+
+	st.Sync()
+
+	offset, ts = st.ClockSnapshot()
+	assert.Equal(t, time.Duration(23456), offset)
+	assert.False(t, ts.IsZero())
+	assert.Equal(t, st.ClockOffset(), offset, "snapshot offset must match individual getter")
+	assert.Equal(t, st.LastSyncTimestamp(), ts, "snapshot timestamp must match individual getter")
+}