diff --git a/client/internal/auth/sessionwatch/watcher.go b/client/internal/auth/sessionwatch/watcher.go index e75a7022e..e685c28d0 100644 --- a/client/internal/auth/sessionwatch/watcher.go +++ b/client/internal/auth/sessionwatch/watcher.go @@ -24,11 +24,7 @@ import ( ) const ( - // Skew tolerates a small clock difference between the management - // server and this peer before treating a deadline as "in the past". - // Slightly above typical NTP drift; tight enough that the UI doesn't - // paint a stale expiry as if it were valid. - Skew = 30 * time.Second + maxPastHorizon = 30 * 24 * time.Hour // maxDeadlineHorizon caps how far in the future an accepted deadline // can sit. A timestamp beyond this is almost certainly a protocol @@ -57,7 +53,7 @@ var ( ErrDeadlineTooFarFuture = errors.New("session deadline too far in the future") // ErrDeadlineInPast is returned by Update when the supplied deadline - // is more than Skew in the past. + // is more than maxPastHorizon in the past. ErrDeadlineInPast = errors.New("session deadline in the past") ) @@ -66,15 +62,14 @@ var ( // for deadline change/clear, PublishEvent for the two warnings); tests pass // a fake recorder so the same surface is observable without an engine. // -// The watcher is the single owner of the deadline propagated to the -// recorder: every set, clear, sanity-check rejection and Close routes the -// value through SetSessionExpiresAt, so the SubscribeStatus snapshot the UI -// reads can never drift from the watcher's timer state. (SetSessionExpiresAt -// fans out its own state-change notification, so no separate notify is -// needed.) The recorder is server-scoped and outlives this engine-scoped -// watcher — without the Close-time clear a teardown (Down, or the Down+Up of -// a profile switch) would leave the next session showing the previous one's -// stale "expires in" value. +// While the watcher runs, it owns the deadline propagated to the recorder: +// every set, clear and sanity-check rejection routes the value through +// SetSessionExpiresAt, so the SubscribeStatus snapshot the UI reads can +// never drift from the watcher's timer state. (SetSessionExpiresAt fans +// out its own state-change notification, so no separate notify is needed.) +// The recorder is server-scoped and outlives this engine-scoped watcher; +// Close deliberately leaves the recorder value in place so transient engine +// restarts don't blank it — the client run loop clears it on real teardown. // // PublishEvent's signature mirrors peer.Status.PublishEvent: the watcher // composes the metadata internally so the wire format (MetaSession*) is @@ -135,10 +130,13 @@ func NewWithLeads(lead, final time.Duration, recorder StatusRecorder) *Watcher { // was disabled). // // Same-value updates are no-ops. A different non-zero value cancels any -// pending timer, resets the "already fired" guard, and arms a new one. +// pending timer, resets the "already fired" guards, and — when the +// deadline lies in the future — arms fresh warning timers. A deadline +// already in the past (within maxPastHorizon) is recorded as-is with no +// timers: the session has expired and consumers render it that way. // // Returns one of the sentinel Err* values when the deadline fails the -// sanity checks (pre-epoch, far future, or in the past beyond Skew). +// sanity checks (pre-epoch, far future, or past beyond maxPastHorizon). // In every error case the watcher first clears its state so it stays // consistent with what the caller will push into its other sinks (e.g. // applySessionDeadline forces a zero deadline into the status recorder @@ -163,7 +161,7 @@ func (w *Watcher) Update(deadline time.Time) error { case deadline.After(now.Add(maxDeadlineHorizon)): w.clearLocked() return fmt.Errorf("%w: %v", ErrDeadlineTooFarFuture, deadline) - case deadline.Before(now.Add(-Skew)): + case deadline.Before(now.Add(-maxPastHorizon)): w.clearLocked() return fmt.Errorf("%w: %v (now=%v)", ErrDeadlineInPast, deadline, now) } @@ -183,7 +181,9 @@ func (w *Watcher) Update(deadline time.Time) error { w.finalFiredAt = time.Time{} w.dismissedAt = time.Time{} - w.armTimerLocked(deadline) + if deadline.After(now) { + w.armTimerLocked(deadline) + } recorder := w.recorder w.mu.Unlock() if recorder != nil { @@ -227,30 +227,25 @@ func (w *Watcher) Dismiss() { log.Infof("auth session final-warning dismissed for deadline %s", w.current.Format(time.RFC3339)) } -// Close stops any pending timer and drops the deadline on the status -// recorder. Update calls after Close are ignored. Clearing the recorder -// here is what keeps a teardown (Down, or the Down+Up of a profile switch) -// from leaving the next session showing this one's stale "expires in" -// value — the recorder is server-scoped and outlives this engine-scoped -// watcher, so nothing else drops the anchor on teardown. +// Close stops any pending timer. Update calls after Close are ignored. +// The recorder keeps its deadline: the watcher is engine-scoped and closes +// on every engine restart (network change, sleep/wake, stream errors) +// while the SSO deadline stays valid across those, so clearing here would +// blank the UI's "expires in" row on every transient reconnect. The +// client run loop clears the server-scoped recorder when it exits for +// real (Down, profile switch, permanent login failure). func (w *Watcher) Close() { w.mu.Lock() + defer w.mu.Unlock() if w.closed { - w.mu.Unlock() return } w.closed = true w.stopTimerLocked() - hadDeadline := !w.current.IsZero() w.current = time.Time{} w.firedAt = time.Time{} w.finalFiredAt = time.Time{} w.dismissedAt = time.Time{} - recorder := w.recorder - w.mu.Unlock() - if recorder != nil && hadDeadline { - recorder.SetSessionExpiresAt(time.Time{}) - } } // clearLocked drops the tracked deadline and notifies the recorder so diff --git a/client/internal/auth/sessionwatch/watcher_test.go b/client/internal/auth/sessionwatch/watcher_test.go index da2b6add6..4b49a94b6 100644 --- a/client/internal/auth/sessionwatch/watcher_test.go +++ b/client/internal/auth/sessionwatch/watcher_test.go @@ -224,11 +224,13 @@ func TestNewDeadlineCancelsPriorTimer(t *testing.T) { func TestRefreshAfterFireArmsNewWarning(t *testing.T) { r := &fakeRecorder{} - lead := 30 * time.Millisecond + lead := 150 * time.Millisecond w := newWatcher(lead, r) defer w.Close() - first := time.Now().Add(50 * time.Millisecond) + // Warning fires ~20ms in; the deadline itself stays 150ms away so the + // replacement below lands well before it. + first := time.Now().Add(170 * time.Millisecond) _ = w.Update(first) // Wait for stateChange + warning of the first cycle. @@ -306,7 +308,29 @@ func TestUpdateRejectsTooFarFuture(t *testing.T) { } } -func TestUpdateInPastClearsDeadline(t *testing.T) { +func TestUpdateRecentPastRecordedAsExpired(t *testing.T) { + r := &fakeRecorder{} + w := newWatcher(50*time.Millisecond, r) + defer w.Close() + + d := time.Now().Add(-1 * time.Hour) + if err := w.Update(d); err != nil { + t.Fatalf("recent-past Update should succeed, got %v", err) + } + if !w.Deadline().Equal(d) { + t.Fatalf("expected deadline to be recorded, got %v want %v", w.Deadline(), d) + } + if got := r.deadline(); !got.Equal(d) { + t.Fatalf("recorder deadline = %v, want %v", got, d) + } + + time.Sleep(80 * time.Millisecond) + if n := countWhere(r.snapshot(), func(e event) bool { return e.kind == publish }); n != 0 { + t.Fatalf("no warning events may fire for an already-past deadline, got %+v", r.snapshot()) + } +} + +func TestUpdateAncientPastRejected(t *testing.T) { r := &fakeRecorder{} w := newWatcher(50*time.Millisecond, r) defer w.Close() @@ -318,12 +342,12 @@ func TestUpdateInPastClearsDeadline(t *testing.T) { // Drain the stateChange from the seed. waitForEvents(t, r, 1) - err := w.Update(time.Now().Add(-1 * time.Hour)) + err := w.Update(time.Now().Add(-31 * 24 * time.Hour)) if !errors.Is(err, ErrDeadlineInPast) { t.Fatalf("want ErrDeadlineInPast, got %v", err) } if !w.Deadline().IsZero() { - t.Fatalf("in-past update must clear the deadline, got %v", w.Deadline()) + t.Fatalf("rejected ancient-past update must clear the deadline, got %v", w.Deadline()) } events := waitForEvents(t, r, 2) if events[1].kind != stateChange { @@ -331,39 +355,25 @@ func TestUpdateInPastClearsDeadline(t *testing.T) { } } -func TestUpdateWithinSkewAccepted(t *testing.T) { - r := &fakeRecorder{} - w := newWatcher(50*time.Millisecond, r) - defer w.Close() - - // 5 seconds in the past is within the 30s Skew tolerance — accept it. - d := time.Now().Add(-5 * time.Second) - if err := w.Update(d); err != nil { - t.Fatalf("within-skew Update should succeed, got %v", err) - } - if !w.Deadline().Equal(d) { - t.Fatalf("expected deadline to be applied, got %v want %v", w.Deadline(), d) - } -} - func TestCloseSilencesUpdates(t *testing.T) { r := &fakeRecorder{} w := newWatcher(50*time.Millisecond, r) w.Close() - _ = w.Update(time.Now().Add(time.Hour)) - - time.Sleep(20 * time.Millisecond) + if err := w.Update(time.Now().Add(time.Hour)); err != nil { + t.Fatalf("Update after Close: want nil, got %v", err) + } if got := r.snapshot(); len(got) != 0 { t.Fatalf("expected no events after Close, got %+v", got) } } -// TestCloseClearsRecorderDeadline pins the profile-switch fix: a watcher -// holding a live deadline must zero the recorder on Close so the next -// engine's watcher (and the UI reading the shared server-scoped recorder) -// doesn't start out showing the previous session's stale "expires in". -func TestCloseClearsRecorderDeadline(t *testing.T) { +// TestCloseKeepsRecorderDeadline pins the reconnect-flap fix: the watcher +// closes on every engine restart (network change, sleep/wake) while the +// SSO deadline stays valid across those, so Close must leave the +// server-scoped recorder's value in place. The client run loop clears the +// recorder when it exits for real. +func TestCloseKeepsRecorderDeadline(t *testing.T) { r := &fakeRecorder{} w := newWatcher(time.Hour, r) @@ -377,8 +387,8 @@ func TestCloseClearsRecorderDeadline(t *testing.T) { w.Close() - if got := r.deadline(); !got.IsZero() { - t.Fatalf("recorder deadline after Close = %v, want zero", got) + if got := r.deadline(); !got.Equal(d) { + t.Fatalf("recorder deadline after Close = %v, want %v", got, d) } } diff --git a/client/internal/connect.go b/client/internal/connect.go index c2fc2fd73..ae5971a85 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -257,7 +257,10 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan log.Errorf("failed to clean up temporary installer file: %v", err) } - defer c.statusRecorder.ClientStop() + defer func() { + c.statusRecorder.SetSessionExpiresAt(time.Time{}) + c.statusRecorder.ClientStop() + }() operation := func() error { // if context cancelled we not start new backoff cycle if c.ctx.Err() != nil { diff --git a/client/internal/engine_session_deadline_test.go b/client/internal/engine_session_deadline_test.go index 6127e5bb0..5a67f103a 100644 --- a/client/internal/engine_session_deadline_test.go +++ b/client/internal/engine_session_deadline_test.go @@ -75,4 +75,14 @@ func TestApplySessionDeadline_ThreeState(t *testing.T) { require.True(t, e.statusRecorder.GetSessionExpiresAt().IsZero(), "invalid timestamp must clear the deadline") }) + + t.Run("recently expired timestamp stays visible as expired", func(t *testing.T) { + e := newEngine() + expired := time.Now().Add(-5 * time.Minute).UTC().Truncate(time.Second) + + e.ApplySessionDeadline(timestamppb.New(expired)) + + require.True(t, e.statusRecorder.GetSessionExpiresAt().Equal(expired), + "recently-expired deadline must stay on the recorder so consumers render it as expired") + }) } diff --git a/client/internal/peer/status.go b/client/internal/peer/status.go index a987482fe..423ce9b23 100644 --- a/client/internal/peer/status.go +++ b/client/internal/peer/status.go @@ -813,19 +813,14 @@ func (d *Status) SetSessionExpiresAt(deadline time.Time) { } // GetSessionExpiresAt returns the most recently recorded SSO session deadline, -// or the zero value when no deadline is tracked. A deadline that has already -// slipped into the past reports as "none": once the session has expired it is -// no longer a meaningful countdown, and the sessionwatch.Watcher does not -// arm a timer at the deadline itself to clear it (only the two pre-expiry -// warnings). Without this guard the UI would keep painting a stale -// "expires in …" against a moment that has passed until the next login, -// extend, or teardown rewrote the value. +// or the zero value when no deadline is tracked. A deadline in the past is +// returned as-is: it means the session has expired, and consumers (tray row, +// CLI status) render it as "expired" rather than hiding it — masking it as +// "none" would blank the UI at the exact moment it should say the session +// ended. func (d *Status) GetSessionExpiresAt() time.Time { d.mux.Lock() defer d.mux.Unlock() - if !d.sessionExpiresAt.IsZero() && d.sessionExpiresAt.Before(time.Now()) { - return time.Time{} - } return d.sessionExpiresAt } diff --git a/client/ui/tray.go b/client/ui/tray.go index c4918825f..63b6a46ec 100644 --- a/client/ui/tray.go +++ b/client/ui/tray.go @@ -317,8 +317,7 @@ func (t *Tray) relayoutMenu() { if sessionDeadline.IsZero() { t.sessionExpiresItem.SetHidden(true) } else { - remaining := t.formatSessionRemaining(time.Until(sessionDeadline)) - t.sessionExpiresItem.SetLabel(t.loc.T("tray.session.expiresIn", "remaining", remaining)) + t.sessionExpiresItem.SetLabel(t.sessionRowLabel(sessionDeadline)) t.sessionExpiresItem.SetHidden(false) } } diff --git a/client/ui/tray_session.go b/client/ui/tray_session.go index 6b73ddb49..885fdb348 100644 --- a/client/ui/tray_session.go +++ b/client/ui/tray_session.go @@ -64,11 +64,42 @@ func (t *Tray) applySessionExpiry(deadline *time.Time, connected bool) bool { return changed } -// runSessionExpiryTicker recomputes the "Expires in …" row label every 30s. Runs until process exit. +// runSessionExpiryTicker recomputes the "Expires in …" row label until process exit. +// The interval scales with the remaining time: coarse when the deadline is far off, +// down to 10s in the final two minutes so the label doesn't lag the ceiling-rounded +// countdown near expiry. The cached deadline is re-read every iteration, so an extend +// or reconnect that moves it is picked up on the next tick. func (t *Tray) runSessionExpiryTicker() { - tk := time.NewTicker(30 * time.Second) - for range tk.C { + tm := time.NewTimer(sessionRefreshInterval(t.sessionRemaining())) + defer tm.Stop() + for range tm.C { t.refreshSessionExpiresLabel() + tm.Reset(sessionRefreshInterval(t.sessionRemaining())) + } +} + +// sessionRemaining returns the time left on the cached SSO deadline, or 0 when unknown. +func (t *Tray) sessionRemaining() time.Duration { + t.sessionMu.Lock() + deadline := t.sessionExpiresAt + t.sessionMu.Unlock() + if deadline.IsZero() { + return 0 + } + return time.Until(deadline) +} + +// sessionRefreshInterval picks how long to wait before the next label recompute. +func sessionRefreshInterval(remaining time.Duration) time.Duration { + switch { + case remaining <= 0: + return 30 * time.Second + case remaining <= 2*time.Minute: + return 10 * time.Second + case remaining <= time.Hour: + return 30 * time.Second + default: + return time.Minute } } @@ -87,30 +118,39 @@ func (t *Tray) refreshSessionExpiresLabel() { if deadline.IsZero() { return } - remaining := t.formatSessionRemaining(time.Until(deadline)) - item.SetLabel(t.loc.T("tray.session.expiresIn", "remaining", remaining)) + item.SetLabel(t.sessionRowLabel(deadline)) +} + +func (t *Tray) sessionRowLabel(deadline time.Time) string { + remaining := time.Until(deadline) + if remaining <= 0 { + return t.loc.T("tray.status.sessionExpired") + } + return t.loc.T("tray.session.expiresIn", "remaining", t.formatSessionRemaining(remaining)) } // formatSessionRemaining renders d as a localised long-form string picking the largest non-zero unit. +// Each unit is rounded up so the label never claims less time than actually remains, matching the +// upper-bound sense of the sub-minute "less than a minute" fragment. // Singular/plural keys are split per language for proper translation. func (t *Tray) formatSessionRemaining(d time.Duration) string { switch { case d < time.Minute: return t.loc.T("tray.session.unit.lessThanMinute") - case d < time.Hour: - m := int(d / time.Minute) + case d <= 59*time.Minute: + m := ceilDiv(d, time.Minute) if m == 1 { return t.loc.T("tray.session.unit.minute") } return t.loc.T("tray.session.unit.minutes", "count", strconv.Itoa(m)) - case d < 24*time.Hour: - h := int((d + 30*time.Minute) / time.Hour) + case d <= 23*time.Hour: + h := ceilDiv(d, time.Hour) if h == 1 { return t.loc.T("tray.session.unit.hour") } return t.loc.T("tray.session.unit.hours", "count", strconv.Itoa(h)) default: - days := int((d + 12*time.Hour) / (24 * time.Hour)) + days := ceilDiv(d, 24*time.Hour) if days == 1 { return t.loc.T("tray.session.unit.day") } @@ -118,6 +158,11 @@ func (t *Tray) formatSessionRemaining(d time.Duration) string { } } +// ceilDiv divides d by unit rounding up, assuming d > 0. +func ceilDiv(d, unit time.Duration) int { + return int((d + unit - time.Nanosecond) / unit) +} + // registerSessionWarningCategory wires the OS notification category and response handler for the expiry warning. // Errors are swallowed since the worst case is a plain notification without buttons. func (t *Tray) registerSessionWarningCategory() { @@ -252,11 +297,9 @@ func (t *Tray) openSessionExpiration() { } // openSessionExtendFlow opens the SessionExpiration window seeded with the cached deadline's remaining time, -// for the "Expires in …" tray row. No-ops when the deadline is unknown or elapsed. +// for the "Expires in …" tray row. Once the deadline has elapsed the row reads "Session expired" and the +// click routes to the login flow instead. No-op when the deadline is unknown. func (t *Tray) openSessionExtendFlow() { - if t.svc.WindowManager == nil { - return - } t.sessionMu.Lock() deadline := t.sessionExpiresAt t.sessionMu.Unlock() @@ -265,6 +308,14 @@ func (t *Tray) openSessionExtendFlow() { } seconds := int(time.Until(deadline).Seconds()) if seconds <= 0 { + if t.window != nil { + t.window.SetURL("/#/login") + t.window.Show() + t.window.Focus() + } + return + } + if t.svc.WindowManager == nil { return } t.svc.WindowManager.OpenSessionExpiration(seconds)