[cpu][windows]: harden the cpu-total computation added in #2125

Follow-up to #2125, which stopped deriving cpu-total from GetSystemTimes
and accumulates the per-processor counters instead.

- Fall back to GetSystemTimes for cpu-total when perfInfo() fails.
  perfInfo() relies on the undocumented NtQuerySystemInformation, so
  Times(false) would now fail outright where that call is unavailable,
  even though the public API would still work. The processor-group
  problem #2125 fixes does not apply to the fallback, which is only
  taken when the group-aware query is unusable in the first place.
- Resolve every proc with Find before calling it in perfInfo().
  LazyProc.Call panics when it cannot resolve the proc, so on a host
  where NtQuerySystemInformation is missing the fallback above was
  unreachable: the process would go down instead. Only the Ex variant
  was probed before.
- Return an error instead of an all-zero cpu-total when perfInfo()
  reports no processors.
- Drop the accumulation of DpcTime and InterruptCount: TimesStat has no
  field for either, so the sums were never used. The struct fields stay,
  they are needed for the buffer layout passed to the Windows API.
- Restore the note from #2110 explaining why the tick counts are summed
  as integers and converted to float64 only once.
- Add TestTimesTotalMatchesPerCPUSum, asserting that cpu-total is the
  field-wise sum of the per-CPU stats. Its tolerance scales with the
  wall clock time actually measured between the two Times calls, so a
  descheduled test goroutine on a busy CI host does not make it flaky.
- Add TestSystemTimes, covering the fallback directly. TimesWithContext
  only reaches it when perfInfo() fails, so it would otherwise ship
  untested.
- Fix a stale comment in the psutil comparison test: Windows no longer
  derives cpu-total from GetSystemTimes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: shirou

cpu/cpu_psutil_test.go

@@ -82,8 +82,10 @@ func TestTimes_Against_Psutil(t *testing.T) {
 	require.NoError(t, err)
 
 	// user/system/idle exist on all supported platforms and both sides
-	// derive them identically (linux: /proc/stat; windows: GetSystemTimes
-	// with system = kernel - idle; darwin: mach host statistics).
+	// derive them from the same counters (linux: /proc/stat; darwin: mach host
+	// statistics; windows: system = kernel - idle, which psutil reads via
+	// GetSystemTimes while gopsutil sums the per-CPU counters — equivalent as
+	// long as the host has a single processor group, see #2125).
 	pt.AssertBracketedDelta(t, "User", before.User, got.User, after.User, cpuTimesSlack)
 	pt.AssertBracketedDelta(t, "System", before.System, got.System, after.System, cpuTimesSlack)
 	pt.AssertBracketedDelta(t, "Idle", before.Idle, got.Idle, after.Idle, cpuTimesSlack)

cpu/cpu_windows.go

@@ -106,8 +106,17 @@ func Times(percpu bool) ([]TimesStat, error) {
 func TimesWithContext(_ context.Context, percpu bool) ([]TimesStat, error) {
 	// Get the CPU performance counters per processor via Windows API
 	stats, err := perfInfo()
+	if err == nil && len(stats) == 0 {
+		err = errors.New("no processor performance information returned")
+	}
 	if err != nil {
-		return nil, err
+		if percpu {
+			return nil, err
+		}
+		// perfInfo relies on the undocumented NtQuerySystemInformation, which
+		// may be unavailable in restricted environments. Fall back to the
+		// public GetSystemTimes for the total rather than failing outright.
+		return systemTimes()
 	}
 
 	if percpu {
@@ -130,23 +139,64 @@ func TimesWithContext(_ context.Context, percpu bool) ([]TimesStat, error) {
 	// machines with more than 64 logical processors when the current thread is
 	// switched to another processor group as then the counters are not
 	// monotonic anymore.
-	var total win32_SystemProcessorPerformanceInformation
+	//
+	// Do all arithmetic on the integer tick counts and convert to float64 only
+	// once. Converting each counter first loses precision on large counters and
+	// can make the returned values non-monotonic. See issue #2110.
+	var idle, kernel, user, interrupt int64
 	for _, v := range stats {
-		total.IdleTime += v.IdleTime
-		total.KernelTime += v.KernelTime
-		total.UserTime += v.UserTime
-		total.DpcTime += v.DpcTime
-		total.InterruptTime += v.InterruptTime
-		total.InterruptCount += v.InterruptCount
+		idle += v.IdleTime
+		kernel += v.KernelTime
+		user += v.UserTime
+		interrupt += v.InterruptTime
 	}
 
 	return []TimesStat{
 		{
 			CPU:    "cpu-total",
-			User:   float64(total.UserTime) / ClocksPerSec,
-			System: float64(total.KernelTime-total.IdleTime) / ClocksPerSec,
-			Idle:   float64(total.IdleTime) / ClocksPerSec,
-			Irq:    float64(total.InterruptTime) / ClocksPerSec,
+			User:   float64(user) / ClocksPerSec,
+			System: float64(kernel-idle) / ClocksPerSec, // kernel time includes idle time
+			Idle:   float64(idle) / ClocksPerSec,
+			Irq:    float64(interrupt) / ClocksPerSec,
+		},
+	}, nil
+}
+
+// systemTimes returns the combined CPU times reported by GetSystemTimes.
+//
+// This is only a fallback for TimesWithContext(ctx, false): GetSystemTimes is a
+// documented public API but returns the counters of the calling thread's
+// processor group only, so on hosts with more than 64 logical processors the
+// values are not monotonic once the thread is migrated to another group. It
+// also cannot report Irq, which is left at zero here.
+//
+// Whether perfInfo works is a property of the environment, so in practice a
+// process stays on one path for its whole lifetime. Should perfInfo fail only
+// intermittently on a multi-group host, the two paths cover a different number
+// of processors, and Percent may report one bogus sample before recovering.
+func systemTimes() ([]TimesStat, error) {
+	var lpIdleTime, lpKernelTime, lpUserTime common.FILETIME
+	// GetSystemTimes returns 0 for error, in which case we check err,
+	// see https://pkg.go.dev/golang.org/x/sys/windows#LazyProc.Call
+	r, _, err := common.ProcGetSystemTimes.Call(
+		uintptr(unsafe.Pointer(&lpIdleTime)),
+		uintptr(unsafe.Pointer(&lpKernelTime)),
+		uintptr(unsafe.Pointer(&lpUserTime)))
+	if r == 0 {
+		return nil, err
+	}
+
+	// See the note on integer arithmetic in TimesWithContext and issue #2110.
+	idle := uint64(lpIdleTime.DwHighDateTime)<<32 | uint64(lpIdleTime.DwLowDateTime)
+	user := uint64(lpUserTime.DwHighDateTime)<<32 | uint64(lpUserTime.DwLowDateTime)
+	kernel := uint64(lpKernelTime.DwHighDateTime)<<32 | uint64(lpKernelTime.DwLowDateTime)
+
+	return []TimesStat{
+		{
+			CPU:    "cpu-total",
+			User:   float64(user) / ClocksPerSec,
+			System: float64(kernel-idle) / ClocksPerSec, // kernel time includes idle time
+			Idle:   float64(idle) / ClocksPerSec,
 		},
 	}, nil
 }
@@ -261,9 +311,16 @@ func perfInfo() ([]win32_SystemProcessorPerformanceInformation, error) {
 	// (up to 64 logical CPUs per group). The non-Ex NtQuerySystemInformation only returns
 	// data for the calling thread's group, so whenever the Ex variant is available we
 	// iterate every active group and concatenate the results. See issue #887.
-	if common.ProcNtQuerySystemInformationEx.Find() == nil {
+	//
+	// Every proc is resolved with Find before it is used: LazyProc.Call panics when it
+	// cannot resolve, and a panic would take the caller's process down instead of
+	// letting TimesWithContext fall back to GetSystemTimes.
+	if common.ProcNtQuerySystemInformationEx.Find() == nil && procGetActiveProcessorGroupCount.Find() == nil {
 		return perfInfoAllGroups()
 	}
+	if err := common.ProcNtQuerySystemInformation.Find(); err != nil {
+		return nil, err
+	}
 	return perfInfoSingleGroup()
 }
 

cpu/cpu_windows_test.go

@@ -6,10 +6,18 @@ package cpu
 import (
 	"context"
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// timesTotalSlack is the per-logical-CPU margin added on top of the measured
+// wall clock time when comparing cpu-total against the sum of the per-CPU stats.
+// It only has to absorb the counter granularity, since the time the two Times
+// calls are actually apart is measured rather than assumed.
+const timesTotalSlack = 0.1 // seconds per logical CPU
+
 // TestPerfInfoMatchesLogicalCount ensures perfInfo() returns one entry per logical
 // CPU on the host. This guards against regressions like issue #887 where only the
 // calling thread's processor group was reported on hosts with more than 64 CPUs.
@@ -22,3 +30,73 @@ func TestPerfInfoMatchesLogicalCount(t *testing.T) {
 
 	require.Len(t, info, n, "perfInfo must return one entry per logical CPU across all processor groups")
 }
+
+// TestTimesTotalMatchesPerCPUSum ensures the cpu-total entry is the field-wise sum
+// of the per-CPU entries. Both are derived from the same perfInfo() counters, so
+// they must agree apart from the counters advancing between the two calls. It
+// guards the accumulation loop in TimesWithContext: a mis-mapped field, a dropped
+// counter or a skipped processor group shows up as a mismatch.
+//
+// Note this does not by itself detect a revert to a GetSystemTimes-based total:
+// that call reports the sum over the calling thread's processor group, which is
+// the whole machine on a single-group host, so User/System/Idle would still match.
+// Only the Irq assertion would catch it, and only once enough interrupt time has
+// accumulated, since GetSystemTimes cannot report it at all.
+func TestTimesTotalMatchesPerCPUSum(t *testing.T) {
+	start := time.Now()
+
+	perCPU, err := Times(true)
+	require.NoError(t, err)
+	require.NotEmpty(t, perCPU)
+
+	total, err := Times(false)
+	require.NoError(t, err)
+	elapsed := time.Since(start)
+	require.Len(t, total, 1)
+	require.Equal(t, "cpu-total", total[0].CPU)
+
+	var want TimesStat
+	for _, c := range perCPU {
+		want.User += c.User
+		want.System += c.System
+		want.Idle += c.Idle
+		want.Irq += c.Irq
+	}
+
+	// Each logical CPU can advance by at most the wall clock time between the two
+	// calls, so scale the tolerance by what actually elapsed. Assuming the calls
+	// are close together would make this flaky whenever the test goroutine is
+	// descheduled on a busy CI host.
+	slack := (elapsed.Seconds() + timesTotalSlack) * float64(len(perCPU))
+	assert.InDelta(t, want.User, total[0].User, slack)
+	assert.InDelta(t, want.System, total[0].System, slack)
+	assert.InDelta(t, want.Idle, total[0].Idle, slack)
+	assert.InDelta(t, want.Irq, total[0].Irq, slack)
+}
+
+// TestSystemTimes exercises the GetSystemTimes fallback directly. TimesWithContext
+// only reaches it when perfInfo() fails, which never happens on a healthy host, so
+// without this test the fallback would ship untested.
+func TestSystemTimes(t *testing.T) {
+	total, err := systemTimes()
+	require.NoError(t, err)
+	require.Len(t, total, 1)
+
+	assert.Equal(t, "cpu-total", total[0].CPU)
+	assert.Positive(t, total[0].User)
+	assert.Positive(t, total[0].Idle)
+	assert.GreaterOrEqual(t, total[0].System, 0.0)
+	// GetSystemTimes reports no interrupt time, so Irq stays unset here. This is
+	// the one field that differs from the perfInfo-based total.
+	assert.Zero(t, total[0].Irq)
+
+	// The counters are cumulative since boot, so a second call must not go
+	// backwards. This catches a broken FILETIME recombination, which would
+	// otherwise only show up as an implausible absolute value.
+	again, err := systemTimes()
+	require.NoError(t, err)
+	require.Len(t, again, 1)
+	assert.GreaterOrEqual(t, again[0].User, total[0].User)
+	assert.GreaterOrEqual(t, again[0].System, total[0].System)
+	assert.GreaterOrEqual(t, again[0].Idle, total[0].Idle)
+}