diff --git a/src/worker/src/xcecd_cgroups.c b/src/worker/src/xcecd_cgroups.c index f09ff12a..326f54ef 100644 --- a/src/worker/src/xcecd_cgroups.c +++ b/src/worker/src/xcecd_cgroups.c @@ -1318,6 +1318,26 @@ static int get_unit_property(sd_bus* bus, const char* unit_path, return r; } +/* + * sd_resource_counter_is_available - systemd represents a cgroup resource + * counter (CPUUsageNSec/MemoryCurrent/MemorySwapCurrent/TasksCurrent) that + * isn't actually being tracked yet for a unit -- accounting not yet active, + * or queried before the unit's cgroup has been fully set up -- as + * UINT64_MAX, NOT as an error return or a 0 reading. get_unit_property()'s + * own success/failure (`r < 0`) only reflects whether the D-Bus round trip + * itself worked; it says nothing about whether systemd actually had a real + * measurement to report, so every caller must check this sentinel itself + * before trusting the value. Bug found live: an unguarded MemoryCurrent read + * of this sentinel, divided down for unit conversion, produced a ~1.76e13 + * "memory usage" per job that the scheduler's rusage reconciliation then + * summed across every RUN job on the host, landing on exactly the + * "used=334251534843904.000000 (304.00 TB)" figure a live replay reported. + */ +static int sd_resource_counter_is_available(uint64_t val) +{ + return val != UINT64_MAX; +} + /** * @brief From cgroup.procs read all PIDs * @param control_group for example "/system.slice/xcecd-worker-123.service" @@ -1440,14 +1460,25 @@ xce_job_rusages_t* collect_job_rusages(uint64_t job_id) // CPU Time (CPUUsageNSec) if (get_unit_property(bus, unit_path, "org.freedesktop.systemd1.Service", "CPUUsageNSec", 't', &u64_val) - >= 0) { + >= 0 + && sd_resource_counter_is_available(u64_val)) { rusages->usages[0]->value = (float)u64_val / 1e9; // 从纳秒转换为秒 } - // Memory RSS (MemoryCurrent) + // Memory RSS (MemoryCurrent). Kept in MB here (bug-1735's established + // wire convention for "memory_rss", also relied on by the controller's + // own passthrough parsing/display, update_host_runusage()/misc.c) -- + // NOT changed by this fix, to avoid moving that value out from under + // the controller's aggregate mem_usage/max_mem/avg_mem/used_mem + // pipeline without also auditing every consumer of it (a separate, + // broader question -- see this fix's bug-log entry for the open + // question of whether the scheduler's own "mem" resource reconciliation, + // sched_reconcile_job_resource_usage()/resource.c, actually expects + // this same MB unit or bytes; not resolved here). if (get_unit_property(bus, unit_path, "org.freedesktop.systemd1.Service", "MemoryCurrent", 't', &u64_val) - >= 0) { + >= 0 + && sd_resource_counter_is_available(u64_val)) { rusages->usages[1]->value = (float)u64_val / (1024 * 1024); // 从字节转换为MB } @@ -1455,7 +1486,8 @@ xce_job_rusages_t* collect_job_rusages(uint64_t job_id) // Memory Swap (MemorySwapCurrent) if (get_unit_property(bus, unit_path, "org.freedesktop.systemd1.Service", "MemorySwapCurrent", 't', &u64_val) - >= 0) { + >= 0 + && sd_resource_counter_is_available(u64_val)) { rusages->usages[2]->value = (float)u64_val / (1024 * 1024); // 从字节转换为MB } @@ -1463,7 +1495,8 @@ xce_job_rusages_t* collect_job_rusages(uint64_t job_id) // Tasks/Threads (TasksCurrent) if (get_unit_property(bus, unit_path, "org.freedesktop.systemd1.Service", "TasksCurrent", 't', &u64_val) - >= 0) { + >= 0 + && sd_resource_counter_is_available(u64_val)) { rusages->usages[3]->value = (float)u64_val; } diff --git a/src/worker/testsuite/meson.build b/src/worker/testsuite/meson.build index 0fcff1bf..75c9eb36 100644 --- a/src/worker/testsuite/meson.build +++ b/src/worker/testsuite/meson.build @@ -161,3 +161,22 @@ test('worker launch_type_dispatch test', launch_type_dispatch_test, suite: 'worker', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# Live-reported bug: a replay session's `info worker` showed +# "resource mem: ... used=334251534843904.000000 (304.00 TB)". Root-caused +# to collect_job_rusages() (xcecd_cgroups.c) trusting a systemd +# UINT64_MAX "resource counter not yet available" sentinel as a real +# reading. White-box #include of xcecd_cgroups.c, same convention as +# hardware_monitor_test/resource_sync_test above -- see +# test_xcecd_cgroups_rusage.c's own header comment for the exact +# reproduced arithmetic and the D-Bus/systemd sandbox limitation. +xcecd_cgroups_rusage_test = executable('xcecd_cgroups_rusage_test', + 'test_xcecd_cgroups_rusage.c', + include_directories: [worker_test_incdir], + dependencies: worker_test_deps, + install: false) + +test('worker xcecd_cgroups rusage test', xcecd_cgroups_rusage_test, + suite: 'worker', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/worker/testsuite/test_xcecd_cgroups_rusage.c b/src/worker/testsuite/test_xcecd_cgroups_rusage.c new file mode 100644 index 00000000..241a2524 --- /dev/null +++ b/src/worker/testsuite/test_xcecd_cgroups_rusage.c @@ -0,0 +1,124 @@ +/* + * test_xcecd_cgroups_rusage.c - regression test for a live-reported bug: a + * replay session showed `info worker pc13v` reporting + * "resource mem: ... used=334251534843904.000000 (304.00 TB)" -- a physical + * impossibility for a 20.81GB worker. Root-caused to collect_job_rusages() + * (xcecd_cgroups.c): get_unit_property()'s success return (`r >= 0`) only + * means the D-Bus round trip to systemd worked -- it says nothing about + * whether systemd actually had a real measurement for the requested + * property. systemd represents "not yet tracked" for a cgroup resource + * counter (CPUUsageNSec/MemoryCurrent/MemorySwapCurrent/TasksCurrent) as + * UINT64_MAX, not as an error or a 0 reading -- a convention the pre-fix + * code never checked for. An unguarded MemoryCurrent read of this sentinel, + * divided by (1024*1024) for MB conversion, produces exactly + * 17592186044416.0 (2^44, UINT64_MAX/2^20 rounded to the nearest + * representable float32) as a single job's "memory usage" -- and once the + * scheduler's rusage reconciliation (sched_reconcile_job_resource_usage(), + * resource.c) sums this same garbage figure across every RUN job on the + * host that hit the same sentinel, N jobs land on exactly N * 2^44, matching + * the reported 334251534843904.0 for N=19 to the bit. + * + * The actual D-Bus/systemd round trip (collect_job_rusages() itself) cannot + * be exercised in this sandbox -- no D-Bus/systemd daemon is reachable here + * (sd_bus_open_system() fails immediately, same environment limitation + * bug-1735 already established for this same file). This test instead + * covers the pure sentinel-detection predicate the fix introduces, + * sd_resource_counter_is_available(), in isolation -- white-box #include of + * the real xcecd_cgroups.c, same convention as test_hardware_monitor.c/ + * test_resource_sync.c. + */ +#include +#include + +#include + +#include "../src/xcecd_cgroups.c" + +/* xcecd_cgroups.c's start_systemd_job() (not exercised by this file -- no + * D-Bus/systemd is reachable in this sandbox, same environment limitation + * bug-1735 already established) calls make_job_sd_unit_name(), the real + * definition of which lives in xcecd_worker_utils.c. Pulling that whole file + * in (and its own further dependencies) just to satisfy the linker for a + * function this test never calls isn't worth it -- a trivial stub matching + * the real declaration (xcecd_utils.h) is enough. */ +int make_job_sd_unit_name(char* buf, size_t bufsize, const char* prefix, + uint64_t job_id) +{ + (void)prefix; + (void)job_id; + if ((buf != NULL) && (bufsize > 0)) { + buf[0] = '\0'; + } + return 0; +} + +void setUp(void) { return; } +void tearDown(void) { return; } + +static void test_sentinel_uint64_max_is_not_available(void) +{ + TEST_ASSERT_FALSE(sd_resource_counter_is_available(UINT64_MAX)); + + printf("test_sentinel_uint64_max_is_not_available: PASS\n"); +} + +static void test_zero_is_a_genuinely_available_reading(void) +{ + /* A freshly-started unit legitimately reads 0 bytes of MemoryCurrent -- + * must NOT be confused with "unavailable" (that would silently drop a + * real, valid zero reading). */ + TEST_ASSERT_TRUE(sd_resource_counter_is_available(0)); + + printf("test_zero_is_a_genuinely_available_reading: PASS\n"); +} + +static void test_ordinary_reading_is_available(void) +{ + /* A realistic MemoryCurrent reading, e.g. ~200MB in bytes. */ + TEST_ASSERT_TRUE(sd_resource_counter_is_available(209715200ULL)); + + printf("test_ordinary_reading_is_available: PASS\n"); +} + +static void test_one_below_sentinel_is_still_available(void) +{ + /* Exact boundary: only UINT64_MAX itself is the sentinel, not merely a + * very large value. */ + TEST_ASSERT_TRUE(sd_resource_counter_is_available(UINT64_MAX - 1)); + + printf("test_one_below_sentinel_is_still_available: PASS\n"); +} + +static void test_sentinel_mb_conversion_matches_live_reported_figure(void) +{ + /* Reproduces the exact arithmetic the pre-fix bug performed on a + * MemoryCurrent sentinel read, and confirms it lands on the exact + * per-job garbage figure the live replay reported (2^44), and that 19 + * such jobs summed match the exact reported total + * (334251534843904.0 = 19 * 2^44). This documents the bug's magnitude; + * the actual fix (guarding on sd_resource_counter_is_available() before + * ever computing this division) is proven by the tests above, which + * assert UINT64_MAX must never reach this arithmetic in the first + * place. */ + float per_job = (float)UINT64_MAX / (1024 * 1024); + + TEST_ASSERT_EQUAL_FLOAT(17592186044416.0f, per_job); + TEST_ASSERT_EQUAL_FLOAT(334251534843904.0f, per_job * 19.0f); + TEST_ASSERT_FALSE(sd_resource_counter_is_available(UINT64_MAX)); + + printf("test_sentinel_mb_conversion_matches_live_reported_figure: " + "PASS\n"); +} + +int main(void) +{ + UNITY_BEGIN(); + + RUN_TEST(test_sentinel_uint64_max_is_not_available); + RUN_TEST(test_zero_is_a_genuinely_available_reading); + RUN_TEST(test_ordinary_reading_is_available); + RUN_TEST(test_one_below_sentinel_is_still_available); + RUN_TEST(test_sentinel_mb_conversion_matches_live_reported_figure); + + return UNITY_END(); +}