fix(tsa): gate on the clock's post-correction error, not the raw offset (#369)

Engine.succeed() stored LastOffset as the offset measured before any
correction, and the TSA clock gate treated that as the clock's current
error. A step corrects the offset down to roughly its root distance plus
half the round-trip delay, but the gate kept judging it by the size of
the correction that was just applied, so a boot step larger than
max_clock_error_ms left every TSA request refused with
timeNotAvailable until the next poll, even though the clock was
already correct.

Add last_sync_error to TimeSyncStatus: for a step, the residual
actually passed to Clock.MarkSynced; for a slew, the full offset
measured, since a slew has not caught up to it yet. ClockGate and
EstimatedClockError now read that field instead of last_offset.
last_offset keeps its old meaning (the pre-correction measurement) for
display.

Refs #364

Signed-off-by: Bugs5382 <12115015+Bugs5382@users.noreply.github.com>
Co-authored-by: Bugs5382 <12115015+Bugs5382@users.noreply.github.com>

Author: Bugs5382

docs/tsa.md

@@ -102,11 +102,17 @@ so one unanswered poll does not stop it. Every request is answered
 | `max_sync_age` | the last good sync is older than `max_sync_age_seconds` | 3600 (1 hour) |
 | `max_clock_error` | the estimated clock error is larger than `max_clock_error_ms` | `accuracy_ms` (1 second) |
 
-The estimated clock error is the offset measured at the last good sync plus
-`max_drift_ppm` parts per million of the time since it. At the defaults, a
-sync that measured 20 ms gives an estimate of 20 ms + 360 ms = 380 ms an hour
-later, inside the 1 second bound. `max_drift_ppm` defaults to 100, a
-conservative bound for a server crystal; 500 is the NTP frequency tolerance.
+The estimated clock error is the clock's error right after the last good
+sync, plus `max_drift_ppm` parts per million of the time since it. That
+right-after-sync figure is not the offset the sync corrected: once a sync
+steps the clock, what remains is bounded by the server's root distance and
+half the round-trip delay, typically a few milliseconds, not the (possibly
+much larger) step that was just applied; a slew has not caught up to its
+offset yet, so its figure is the full measured offset until the next sync. At
+the defaults, a sync left with a 20 ms error gives an estimate of
+20 ms + 360 ms = 380 ms an hour later, inside the 1 second bound.
+`max_drift_ppm` defaults to 100, a conservative bound for a server crystal;
+500 is the NTP frequency tolerance.
 
 `max_clock_error_ms` may not be larger than the effective `accuracy_ms`:
 `config apply` refuses it, because a token must never claim more accuracy than

gen/go/cryptos/node/v1/status.pb.go

@@ -70,9 +70,10 @@ func newTSAFixture(t *testing.T) *tsaFixture {
 				return &nodev1.TimeSyncStatus{State: nodev1.TimeSyncState_TIME_SYNC_STATE_PENDING}
 			}
 			return &nodev1.TimeSyncStatus{
-				State:      nodev1.TimeSyncState_TIME_SYNC_STATE_SYNCED,
-				LastOffset: durationpb.New(3 * time.Millisecond),
-				LastSync:   timestamppb.Now(),
+				State:         nodev1.TimeSyncState_TIME_SYNC_STATE_SYNCED,
+				LastOffset:    durationpb.New(3 * time.Millisecond),
+				LastSyncError: durationpb.New(3 * time.Millisecond),
+				LastSync:      timestamppb.Now(),
 			}
 		},
 	}

internal/init/tsa_test.go

@@ -128,17 +128,18 @@ type Engine struct {
 	// round serialises sync rounds.
 	round sync.Mutex
 
-	mu         sync.Mutex
-	state      nodev1.TimeSyncState
-	syncedOnce bool
-	lastServer string
-	lastOffset time.Duration
-	hasOffset  bool
-	stratum    uint32
-	lastSync   time.Time
-	stepped    bool
-	lastErr    string
-	refused    bool
+	mu            sync.Mutex
+	state         nodev1.TimeSyncState
+	syncedOnce    bool
+	lastServer    string
+	lastOffset    time.Duration
+	lastSyncError time.Duration
+	hasOffset     bool
+	stratum       uint32
+	lastSync      time.Time
+	stepped       bool
+	lastErr       string
+	refused       bool
 }
 
 // New builds an Engine. It does no I/O.
@@ -223,6 +224,7 @@ func (e *Engine) Status() *nodev1.TimeSyncStatus {
 	}
 	if e.hasOffset {
 		st.LastOffset = durationpb.New(e.lastOffset)
+		st.LastSyncError = durationpb.New(e.lastSyncError)
 	}
 	if !e.lastSync.IsZero() {
 		st.LastSync = timestamppb.New(e.lastSync)
@@ -613,7 +615,7 @@ func (e *Engine) apply(samples []Sample, errs map[string]error, boot bool) {
 	if err := e.cfg.Floor.Advance(synced); err != nil {
 		e.log.Error("clock floor: could not record the synced time", "err", err)
 	}
-	e.succeed(s, synced, boot && step)
+	e.succeed(s, synced, boot && step, step)
 }
 
 // refuse records a round in which servers answered but the clock was not
@@ -638,20 +640,21 @@ func (e *Engine) fail(msg string, level slog.Level) {
 	e.log.Log(context.Background(), level, "time sync round did not adjust the clock", "reason", msg)
 }
 
-func (e *Engine) succeed(s Sample, at time.Time, stepped bool) {
+func (e *Engine) succeed(s Sample, at time.Time, bootStep, stepped bool) {
 	e.mu.Lock()
 	prev := e.state
 	first := !e.syncedOnce
 	e.state = nodev1.TimeSyncState_TIME_SYNC_STATE_SYNCED
 	e.syncedOnce = true
 	e.lastServer = s.Server
 	e.lastOffset = s.Offset
+	e.lastSyncError = syncError(s, stepped)
 	e.hasOffset = true
 	e.stratum = uint32(s.Stratum)
 	e.lastSync = at
 	e.lastErr = ""
 	e.refused = false
-	if stepped {
+	if bootStep {
 		e.stepped = true
 	}
 	e.mu.Unlock()
@@ -663,6 +666,20 @@ func (e *Engine) succeed(s Sample, at time.Time, stepped bool) {
 	}
 }
 
+// syncError is the clock's estimated error right after this round, for the
+// gate TSA uses to decide whether to trust the clock: a step corrects the
+// clock down to about its root distance plus half the round-trip delay (the
+// same figure passed to Clock.MarkSynced), so that residual is what remains,
+// not the much larger offset the step was asked to correct; a slew has not
+// caught up to the offset yet, so its estimate is still the full offset
+// measured, as before.
+func syncError(s Sample, stepped bool) time.Duration {
+	if stepped {
+		return s.RootDistance + s.Delay/2
+	}
+	return absDuration(s.Offset)
+}
+
 // fmtFloorError is the reason a step below the floor was refused.
 func fmtFloorError(target, floor time.Time) string {
 	return fmt.Sprintf("server time %s is behind already-issued certificates or the last good sync (floor %s)",

internal/timesync/engine.go

@@ -96,12 +96,14 @@ type fakeServer struct {
 	clock    *fakeClock
 	requests atomic.Int32
 
-	mu      sync.Mutex
-	offset  time.Duration
-	stratum uint8
-	leap    uint8
-	kiss    string
-	drop    bool
+	mu             sync.Mutex
+	offset         time.Duration
+	stratum        uint8
+	leap           uint8
+	kiss           string
+	drop           bool
+	rootDelay      Short
+	rootDispersion Short
 }
 
 func newFakeServer(t *testing.T, clock *fakeClock, offset time.Duration) *fakeServer {
@@ -110,12 +112,18 @@ func newFakeServer(t *testing.T, clock *fakeClock, offset time.Duration) *fakeSe
 	if err != nil {
 		t.Fatalf("listen: %v", err)
 	}
-	s := &fakeServer{conn: conn, clock: clock, offset: offset, stratum: 2}
+	s := &fakeServer{conn: conn, clock: clock, offset: offset, stratum: 2, rootDelay: 0x00000100, rootDispersion: 0x00000100}
 	t.Cleanup(func() { _ = conn.Close() })
 	go s.serve()
 	return s
 }
 
+// shortFrom converts d to the NTP 16.16 fixed-point form used for root delay
+// and root dispersion.
+func shortFrom(d time.Duration) Short {
+	return Short(uint64(d) * 65536 / uint64(time.Second))
+}
+
 func (s *fakeServer) set(fn func(*fakeServer)) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -136,14 +144,15 @@ func (s *fakeServer) serve() {
 		}
 		s.mu.Lock()
 		drop, off, stratum, leap, kiss := s.drop, s.offset, s.stratum, s.leap, s.kiss
+		rootDelay, rootDispersion := s.rootDelay, s.rootDispersion
 		s.mu.Unlock()
 		if drop {
 			continue
 		}
 		now := TimestampFromTime(s.clock.Now().Add(off))
 		reply := Header{
 			Leap: leap, Version: 4, Mode: ModeServer, Stratum: stratum,
-			RootDelay: 0x00000100, RootDispersion: 0x00000100,
+			RootDelay: rootDelay, RootDispersion: rootDispersion,
 			Origin: req.Transmit, Receive: now, Transmit: now,
 		}
 		if kiss != "" {
@@ -289,6 +298,44 @@ func TestBootSyncStepsALargeOffset(t *testing.T) {
 	}
 }
 
+// A boot step corrects the clock down to roughly its root distance, so the
+// TSA's clock gate (which reads GetLastSyncError, not the raw offset the step
+// was asked to correct) sees it as trustworthy at once, not still 5s off.
+func TestBootSyncStepLeavesASmallPostStepSyncError(t *testing.T) {
+	h := newHarness(t)
+	h.add("192.0.2.1", 5*time.Second)
+	e := h.engine()
+	e.BootSync(context.Background())
+
+	st := e.Status()
+	if !approx(st.GetLastOffset().AsDuration(), 5*time.Second) {
+		t.Fatalf("last offset = %v, want it to keep the raw measured offset", st.GetLastOffset().AsDuration())
+	}
+	if got := st.GetLastSyncError().AsDuration(); got >= time.Second {
+		t.Fatalf("last sync error = %v, want well under the 5s step (the clock was already corrected)", got)
+	}
+}
+
+// A server with a large root distance leaves the clock genuinely less
+// trustworthy even after a step corrects the bulk of the offset; the post-step
+// sync error estimate must still reflect that, not collapse to near zero.
+func TestBootSyncStepRecordsSyncErrorFromRootDistance(t *testing.T) {
+	h := newHarness(t)
+	s := h.add("192.0.2.1", 5*time.Second)
+	s.set(func(s *fakeServer) { s.rootDelay = 0; s.rootDispersion = shortFrom(600 * time.Millisecond) })
+	e := h.engine()
+	e.BootSync(context.Background())
+
+	st := e.Status()
+	if !approx(st.GetLastOffset().AsDuration(), 5*time.Second) {
+		t.Fatalf("last offset = %v, want it to keep the raw measured offset", st.GetLastOffset().AsDuration())
+	}
+	got := st.GetLastSyncError().AsDuration()
+	if got < 550*time.Millisecond || got > 750*time.Millisecond {
+		t.Fatalf("last sync error = %v, want it to track the 600ms root distance, not the 5s offset or ~0", got)
+	}
+}
+
 func TestBootSyncSlewsASmallOffset(t *testing.T) {
 	h := newHarness(t)
 	h.add("192.0.2.1", 60*time.Millisecond)
@@ -303,6 +350,22 @@ func TestBootSyncSlewsASmallOffset(t *testing.T) {
 	}
 }
 
+// A slew has not caught up to the offset yet when the round finishes, so its
+// post-sync error estimate is still the full offset measured, unchanged from
+// before this estimate existed.
+func TestBootSyncSlewSyncErrorIsTheFullOffset(t *testing.T) {
+	h := newHarness(t)
+	h.add("192.0.2.1", 60*time.Millisecond)
+	e := h.engine()
+	e.BootSync(context.Background())
+
+	st := e.Status()
+	if !approx(st.GetLastSyncError().AsDuration(), st.GetLastOffset().AsDuration()) {
+		t.Fatalf("last sync error = %v, last offset = %v; want them equal for a slew",
+			st.GetLastSyncError().AsDuration(), st.GetLastOffset().AsDuration())
+	}
+}
+
 // A node with no time source runs on its hardware clock and is never gated.
 func TestNoSourceIsNotConfiguredAndNeverGated(t *testing.T) {
 	h := newHarness(t)

internal/timesync/engine_test.go

@@ -84,29 +84,30 @@ func (r *ClockRefusal) Public() string {
 }
 
 // EstimatedClockError is how far the clock may be off: the magnitude of the
-// offset measured at the last good sync plus ppm parts per million of the
-// time since it.
-func EstimatedClockError(offset, age time.Duration, ppm uint32) time.Duration {
-	return offset.Abs() + age/1_000_000*time.Duration(ppm) + age%1_000_000*time.Duration(ppm)/1_000_000
+// clock's error estimate at the last good sync plus ppm parts per million of
+// the time since it.
+func EstimatedClockError(syncErr, age time.Duration, ppm uint32) time.Duration {
+	return syncErr.Abs() + age/1_000_000*time.Duration(ppm) + age%1_000_000*time.Duration(ppm)/1_000_000
 }
 
 // ClockGate returns the TSA's clock check over the node's time-sync status.
 // The clock may be stamped while the last good sync of this boot is no
 // older than MaxSyncAge and the estimated clock error (EstimatedClockError
-// over the offset it measured and the time since) is no larger than
-// MaxClockError. A poll that no server answered does not refuse on its own,
-// so one lost packet does not stop the TSA; a round that was answered but
-// not applied (adjustment_refused: the sources disagree, or a step was
-// refused) does, until the next good sync. A node with no time source never
-// syncs and is always refused. Unlike certificate signing there is no
-// override: a timestamp is nothing but a claim about the time.
+// over the clock's post-correction error estimate at that sync and the time
+// since) is no larger than MaxClockError. A poll that no server answered does
+// not refuse on its own, so one lost packet does not stop the TSA; a round
+// that was answered but not applied (adjustment_refused: the sources
+// disagree, or a step was refused) does, until the next good sync. A node
+// with no time source never syncs and is always refused. Unlike certificate
+// signing there is no override: a timestamp is nothing but a claim about the
+// time.
 func ClockGate(status func() *nodev1.TimeSyncStatus, limits ClockLimits, now func() time.Time) func() error {
 	return func() error {
 		st := status()
 		if st == nil {
 			return &ClockRefusal{Limit: LimitNoGoodSync, Detail: "the time sync status is unavailable"}
 		}
-		if st.GetLastSync() == nil || st.GetLastOffset() == nil {
+		if st.GetLastSync() == nil || st.GetLastSyncError() == nil {
 			return &ClockRefusal{Limit: LimitNoGoodSync, Detail: st.GetLastError()}
 		}
 		if st.GetAdjustmentRefused() {
@@ -116,7 +117,7 @@ func ClockGate(status func() *nodev1.TimeSyncStatus, limits ClockLimits, now fun
 		if age > limits.MaxSyncAge {
 			return &ClockRefusal{Limit: LimitMaxSyncAge, Value: age.Round(time.Millisecond).String(), Bound: limits.MaxSyncAge.String()}
 		}
-		if est := EstimatedClockError(st.GetLastOffset().AsDuration(), age, limits.MaxDriftPPM); est > limits.MaxClockError {
+		if est := EstimatedClockError(st.GetLastSyncError().AsDuration(), age, limits.MaxDriftPPM); est > limits.MaxClockError {
 			return &ClockRefusal{Limit: LimitMaxClockError, Value: est.String(), Bound: limits.MaxClockError.String()}
 		}
 		return nil

internal/tsa/clock.go

@@ -30,13 +30,17 @@ import (
 
 var gateNow = time.Date(2026, 6, 1, 12, 0, 0, 0, time.UTC)
 
-// goodSync is a time-sync status whose last good sync was age ago and
-// measured offset. state is what the latest round left behind.
-func goodSync(state nodev1.TimeSyncState, offset, age time.Duration) *nodev1.TimeSyncStatus {
+// goodSync is a time-sync status whose last good sync was age ago, with
+// syncErr as both the raw measured offset and the post-correction error
+// estimate (the two coincide for a slew, or a step whose residual happens to
+// match what was measured; see the gate tests below for a step where they
+// differ). state is what the latest round left behind.
+func goodSync(state nodev1.TimeSyncState, syncErr, age time.Duration) *nodev1.TimeSyncStatus {
 	return &nodev1.TimeSyncStatus{
-		State:      state,
-		LastOffset: durationpb.New(offset),
-		LastSync:   timestamppb.New(gateNow.Add(-age)),
+		State:         state,
+		LastOffset:    durationpb.New(syncErr),
+		LastSyncError: durationpb.New(syncErr),
+		LastSync:      timestamppb.New(gateNow.Add(-age)),
 	}
 }
 
@@ -93,6 +97,18 @@ func TestClockGate(t *testing.T) {
 			return st
 		}(), limit: LimitAdjustmentRefused},
 		"last sync after now counts as fresh": {status: goodSync(synced, 0, -time.Minute)},
+		"a boot step's large raw offset does not by itself refuse": {status: &nodev1.TimeSyncStatus{
+			State:         synced,
+			LastOffset:    durationpb.New(5 * time.Second),
+			LastSyncError: durationpb.New(10 * time.Millisecond),
+			LastSync:      timestamppb.New(gateNow),
+		}},
+		"a genuinely large post-correction error still refuses despite a small raw offset": {status: &nodev1.TimeSyncStatus{
+			State:         synced,
+			LastOffset:    durationpb.New(20 * time.Millisecond),
+			LastSyncError: durationpb.New(1500 * time.Millisecond),
+			LastSync:      timestamppb.New(gateNow),
+		}, limit: LimitMaxClockError},
 	} {
 		t.Run(name, func(t *testing.T) {
 			l := tc.limits

internal/tsa/clock_test.go

@@ -31,9 +31,9 @@
 // refuses before the first good sync of the boot, after a round the servers
 // answered but that was not applied (adjustment_refused), once the last good
 // sync is older than ClockLimits.MaxSyncAge, and once the estimated clock
-// error (EstimatedClockError: the offset at the last good sync plus
-// MaxDriftPPM of the time since) is above MaxClockError, which is never
-// larger than the accuracy tokens claim.
+// error (EstimatedClockError: the clock's post-correction error estimate at
+// the last good sync plus MaxDriftPPM of the time since) is above
+// MaxClockError, which is never larger than the accuracy tokens claim.
 //
 // # Keys and certificates
 //

internal/tsa/doc.go

@@ -147,8 +147,10 @@ message TimeSyncStatus {
   // sync, as queried (a hostname or an IPv4 literal). Empty until the first
   // good sync.
   string last_server = 4;
-  // last_offset is the clock offset measured at the latest good sync: the
-  // server's time minus the node's. Unset until the first good sync.
+  // last_offset is the clock offset measured at the latest good sync, before
+  // any correction: the server's time minus the node's. A step corrects most
+  // of it at once, so after a step this is not how far off the clock still
+  // is; see last_sync_error for that. Unset until the first good sync.
   google.protobuf.Duration last_offset = 5;
   // stratum is the stratum of last_server at the latest good sync. Zero until
   // the first good sync.
@@ -168,6 +170,15 @@ message TimeSyncStatus {
   // in which no server answered leaves it as it was, and the next good sync
   // clears it. The TSA refuses to stamp while it is set.
   bool adjustment_refused = 10;
+  // last_sync_error is the clock's estimated error right after the latest
+  // good sync: for a step, the residual the correction left behind (about
+  // root distance plus half the round-trip delay, the same figure passed to
+  // the kernel's MarkSynced); for a slew, the full measured offset, since a
+  // slew has not caught up to it yet. Unlike last_offset, which keeps the
+  // offset as measured before any correction, this is what the clock is
+  // actually still off by, and is what gates time-stamping. Unset until the
+  // first good sync.
+  google.protobuf.Duration last_sync_error = 11;
 }
 
 // TimeSyncState is the node's clock synchronisation state.