diff --git a/include/kpn/diagnostics.hpp b/include/kpn/diagnostics.hpp index 2bc1e4c..b94c8f2 100644 --- a/include/kpn/diagnostics.hpp +++ b/include/kpn/diagnostics.hpp @@ -157,6 +157,16 @@ struct NodeSnapshot { double cpu_util_pct; // exec_ms / (exec_ms + blocked_ms) * 100 double queue_wait_ms{0}; // PoolNode: cumulative time spent in pool queue + // Cumulative wall time inside fire_once. Unlike ema_exec_ms this is a true + // sum, so it is the field to use for "share of the run spent in this node". + // Note it still includes time parked pushing into a full output channel; + // total_cpu_ms is the part that backpressure cannot inflate. + // + // Declared before the two bools below because every node type initialises + // this aggregate positionally, and all of them supply total_exec_ms as the + // element after queue_wait_ms. + double total_exec_ms{0}; + // Live scheduling state, for observing the AR-004 invariant "a node never // sleeps with a wake outstanding". The invariant was previously asserted in // comments but invisible at runtime, so a lost wake could only be found in a @@ -169,11 +179,6 @@ struct NodeSnapshot { // queued=1 while nothing running -> submitted but never scheduled bool queued{false}; bool wake_pending{false}; - // Cumulative wall time inside fire_once. Unlike ema_exec_ms this is a true - // sum, so it is the field to use for "share of the run spent in this node". - // Note it still includes time parked pushing into a full output channel; - // total_cpu_ms is the part that backpressure cannot inflate. - double total_exec_ms{0}; }; // ── Pool statistics + snapshot ──────────────────────────────────────────────── diff --git a/tests/test_pool_node.cpp b/tests/test_pool_node.cpp index 779572b..a5b3f90 100644 --- a/tests/test_pool_node.cpp +++ b/tests/test_pool_node.cpp @@ -478,3 +478,49 @@ TEST_CASE("per-node and network overflow callbacks both fire independently", "[p REQUIRE(per_node.load() == 0); REQUIRE(network.load() == 0); } + +// Regression: NodeSnapshot's fields must line up with what nodes initialise. +// +// The snapshot is an aggregate that every node type fills positionally, and +// a8cfe73 appended queued/wake_pending/total_exec_ms to it in an order no call +// site used: each node supplies total_exec_ms as the element straight after +// queue_wait_ms, but the struct declared the two bools there. So the exec total +// landed in `queued`, `queued` landed in `wake_pending`, and `wake_pending` +// landed in total_exec_ms. The compiler said so (-Wnarrowing, bool to double, +// once per node instantiation) and the build carried on. +// +// It matters more than a cosmetic mix-up: these three fields exist to diagnose a +// wedge, and a wedged pipeline reported total_exec_ms as 0 or 1 and `queued` as +// "did this node ever run". Reading them would have pointed at the wrong node. +// +// Asserted against ema_exec_ms because that field is independently computed and +// was already correct: a true sum over several frames cannot be below the +// exponentially-weighted average of the same samples. +TEST_CASE("node snapshot fields line up with the values nodes supply", + "[pool_node][diagnostics]") { + auto pool = std::make_shared(1); + pool->start(); + + auto node = make_pool_node(pool, 64); + Channel out(64); + node.set_output_channel<0>(&out); + node.start(); + + for (int i = 0; i < 8; ++i) node.input_channel<0>().push(i); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + auto snap = node.node_snapshot("n", 1.0); + node.stop(); + pool->stop(); + + INFO("frames=" << snap.frames_processed + << " ema=" << snap.ema_exec_ms + << " total=" << snap.total_exec_ms); + REQUIRE(snap.frames_processed == 8); + // The mis-ordered aggregate put wake_pending here, so this was 0.0 or 1.0. + CHECK(snap.total_exec_ms >= snap.ema_exec_ms); + // ...and the exec total here, which is non-zero, so `queued` read true for + // any node that had ever run — including one asleep with nothing to do. + CHECK_FALSE(snap.queued); + CHECK_FALSE(snap.wake_pending); +}