diff --git a/backend/internal/service/ops_metrics_collector.go b/backend/internal/service/ops_metrics_collector.go index d4f5c3292..4fc957604 100644 --- a/backend/internal/service/ops_metrics_collector.go +++ b/backend/internal/service/ops_metrics_collector.go @@ -601,24 +601,11 @@ func (c *OpsMetricsCollector) collectSystemStats(ctx context.Context) (*opsColle sampleAt := time.Now().UTC() - // Prefer cgroup (container) metrics when available. + // CPU: prefer cgroup (container) metrics, falling back to host metrics when + // cgroup CPU accounting is unavailable. if cpuPct := c.tryCgroupCPUPercent(sampleAt); cpuPct != nil { out.cpuUsagePercent = cpuPct } - - cgroupUsed, cgroupTotal, cgroupOK := readCgroupMemoryBytes() - if cgroupOK { - usedMB := int64(cgroupUsed / bytesPerMB) - out.memoryUsedMB = &usedMB - if cgroupTotal > 0 { - totalMB := int64(cgroupTotal / bytesPerMB) - out.memoryTotalMB = &totalMB - pct := roundTo1DP(float64(cgroupUsed) / float64(cgroupTotal) * 100) - out.memoryUsagePercent = &pct - } - } - - // Fallback to host metrics if cgroup metrics are unavailable (or incomplete). if out.cpuUsagePercent == nil { if cpuPercents, err := cpu.PercentWithContext(ctx, 0, false); err == nil && len(cpuPercents) > 0 { v := roundTo1DP(cpuPercents[0]) @@ -626,32 +613,60 @@ func (c *OpsMetricsCollector) collectSystemStats(ctx context.Context) (*opsColle } } - // If total memory isn't available from cgroup (e.g. memory.max = "max"), fill total from host. - if out.memoryUsedMB == nil || out.memoryTotalMB == nil || out.memoryUsagePercent == nil { - if vm, err := mem.VirtualMemoryWithContext(ctx); err == nil && vm != nil { - if out.memoryUsedMB == nil { - usedMB := int64(vm.Used / bytesPerMB) - out.memoryUsedMB = &usedMB - } - if out.memoryTotalMB == nil { - totalMB := int64(vm.Total / bytesPerMB) - out.memoryTotalMB = &totalMB - } - if out.memoryUsagePercent == nil { - if out.memoryUsedMB != nil && out.memoryTotalMB != nil && *out.memoryTotalMB > 0 { - pct := roundTo1DP(float64(*out.memoryUsedMB) / float64(*out.memoryTotalMB) * 100) - out.memoryUsagePercent = &pct - } else { - pct := roundTo1DP(vm.UsedPercent) - out.memoryUsagePercent = &pct - } - } + // Memory: prefer cgroup (container) metrics, but only when the cgroup exposes + // BOTH a current usage and a concrete limit (memory.max != "max"). When the + // limit is missing or the cgroup data is otherwise incomplete, fall back + // ENTIRELY to host metrics. Never mix a container "used" with a host "total": + // doing so reports a misleadingly tiny percentage (e.g. 60MB / 23GB ~= 0.3%). + cgroupUsed, cgroupTotal, cgroupOK := readCgroupMemoryBytes() + var host *mem.VirtualMemoryStat + if !cgroupOK || cgroupTotal == 0 { + if vm, err := mem.VirtualMemoryWithContext(ctx); err == nil { + host = vm } } + out.memoryUsedMB, out.memoryTotalMB, out.memoryUsagePercent = resolveMemoryStats(cgroupUsed, cgroupTotal, cgroupOK, host) return out, nil } +// resolveMemoryStats picks a single, self-consistent (used, total, percent) +// memory trio from either cgroup (container) or host metrics — never a mix of +// the two. +// +// cgroup metrics are preferred, but only when the cgroup reports BOTH a current +// usage and a concrete limit (i.e. memory.max is a number, not "max", so +// cgroupTotal > 0). If the limit is absent or the cgroup data is otherwise +// incomplete, all three values fall back to the host reading. This avoids the +// classic pitfall of dividing a container "used" by a host "total", which +// wildly understates memory usage (e.g. 60MB / 23GB ~= 0.3%). +func resolveMemoryStats(cgroupUsed, cgroupTotal uint64, cgroupOK bool, host *mem.VirtualMemoryStat) (usedMB *int64, totalMB *int64, usagePercent *float64) { + if cgroupOK && cgroupTotal > 0 { + u := int64(cgroupUsed / bytesPerMB) + t := int64(cgroupTotal / bytesPerMB) + p := roundTo1DP(float64(cgroupUsed) / float64(cgroupTotal) * 100) + return &u, &t, &p + } + + if host == nil { + return nil, nil, nil + } + + u := int64(host.Used / bytesPerMB) + usedMB = &u + if host.Total > 0 { + t := int64(host.Total / bytesPerMB) + totalMB = &t + p := roundTo1DP(float64(host.Used) / float64(host.Total) * 100) + usagePercent = &p + } else { + // Degenerate: host reported no total. Preserve gopsutil's own percentage. + p := roundTo1DP(host.UsedPercent) + usagePercent = &p + } + return usedMB, totalMB, usagePercent +} + func (c *OpsMetricsCollector) tryCgroupCPUPercent(now time.Time) *float64 { usageNanos, ok := readCgroupCPUUsageNanos() if !ok { diff --git a/backend/internal/service/ops_metrics_collector_memory_test.go b/backend/internal/service/ops_metrics_collector_memory_test.go new file mode 100644 index 000000000..0a1e8873a --- /dev/null +++ b/backend/internal/service/ops_metrics_collector_memory_test.go @@ -0,0 +1,110 @@ +package service + +import ( + "testing" + + "github.com/shirou/gopsutil/v4/mem" + "github.com/stretchr/testify/require" +) + +const ( + testMiB = 1024 * 1024 + testGiB = 1024 * testMiB +) + +// TestResolveMemoryStatsCgroupUsageButUnlimitedFallsBackToHost is the core +// regression: in Docker + cgroup v2 with NO memory limit, memory.current is a +// small container number while memory.max = "max" (so cgroupTotal == 0). The +// old code reported the container "used" against the host "total", producing a +// misleadingly tiny percentage (~0.3%). The fix must fall back ENTIRELY to host +// metrics instead of mixing the two sources. +func TestResolveMemoryStatsCgroupUsageButUnlimitedFallsBackToHost(t *testing.T) { + const cgroupUsed = uint64(64573440) // memory.current, ~61 MiB + host := &mem.VirtualMemoryStat{ + Used: 16 * testGiB, + Total: 24 * testGiB, + UsedPercent: 66.7, + } + + usedMB, totalMB, pct := resolveMemoryStats(cgroupUsed, 0 /* memory.max = "max" */, true, host) + + require.NotNil(t, usedMB) + require.NotNil(t, totalMB) + require.NotNil(t, pct) + + // Everything must come from the host, not from the cgroup container value. + require.Equal(t, int64(16*1024), *usedMB, "used must be host used, not container used") + require.Equal(t, int64(24*1024), *totalMB, "total must be host total") + require.InDelta(t, 66.7, *pct, 0.05, "percent must be host-derived, not container/host mix") + + // Guard against the specific bug: container used (~61 MiB) vs host total (~0.3%). + require.NotEqual(t, int64(cgroupUsed/testMiB), *usedMB, "must not report the container used value") + require.Greater(t, *pct, 1.0, "percent must not collapse to the ~0.3%% mixed value") +} + +// TestResolveMemoryStatsExplicitContainerLimitUsesCgroup covers the case where +// a real container memory limit is set: memory.current = 512 MiB and +// memory.max = 2 GiB must yield ~25% entirely from cgroup, ignoring the host. +func TestResolveMemoryStatsExplicitContainerLimitUsesCgroup(t *testing.T) { + host := &mem.VirtualMemoryStat{ + Used: 16 * testGiB, // deliberately different; must be ignored + Total: 24 * testGiB, + UsedPercent: 66.7, + } + + usedMB, totalMB, pct := resolveMemoryStats(512*testMiB, 2*testGiB, true, host) + + require.NotNil(t, usedMB) + require.NotNil(t, totalMB) + require.NotNil(t, pct) + + require.Equal(t, int64(512), *usedMB) + require.Equal(t, int64(2048), *totalMB) + require.InDelta(t, 25.0, *pct, 0.05) +} + +// TestResolveMemoryStatsNoCgroupUsesHost covers bare-metal / no-cgroup hosts: +// all three values come from the host reading. +func TestResolveMemoryStatsNoCgroupUsesHost(t *testing.T) { + host := &mem.VirtualMemoryStat{ + Used: 16 * testGiB, + Total: 24 * testGiB, + UsedPercent: 66.7, + } + + usedMB, totalMB, pct := resolveMemoryStats(0, 0, false, host) + + require.NotNil(t, usedMB) + require.NotNil(t, totalMB) + require.NotNil(t, pct) + require.Equal(t, int64(16*1024), *usedMB) + require.Equal(t, int64(24*1024), *totalMB) + require.InDelta(t, 66.7, *pct, 0.05) +} + +// TestResolveMemoryStatsNoDataReturnsNil: when neither cgroup nor host data is +// available, all outputs are nil (best-effort, nothing persisted). +func TestResolveMemoryStatsNoDataReturnsNil(t *testing.T) { + usedMB, totalMB, pct := resolveMemoryStats(0, 0, false, nil) + require.Nil(t, usedMB) + require.Nil(t, totalMB) + require.Nil(t, pct) +} + +// TestResolveMemoryStatsHostWithoutTotalKeepsGopsutilPercent: degenerate host +// reading with no total still yields a used value and gopsutil's own percentage. +func TestResolveMemoryStatsHostWithoutTotalKeepsGopsutilPercent(t *testing.T) { + host := &mem.VirtualMemoryStat{ + Used: 8 * testGiB, + Total: 0, + UsedPercent: 42.5, + } + + usedMB, totalMB, pct := resolveMemoryStats(0, 0, false, host) + + require.NotNil(t, usedMB) + require.Nil(t, totalMB) + require.NotNil(t, pct) + require.Equal(t, int64(8*1024), *usedMB) + require.InDelta(t, 42.5, *pct, 0.05) +}