fix: NodeSnapshot fields must line up with what nodes supply
a8cfe73appended queued/wake_pending/total_exec_ms to the NodeSnapshot aggregate in an order no call site used. Every node type fills the aggregate positionally and all of them supply 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. GCC reported it as -Wnarrowing (bool to double), 88 times, once per node instantiation across the test build. The build carried on. This is worse than a cosmetic mix-up, because all three fields were added specifically to diagnose a wedge. A wedged pipeline reported total_exec_ms as 0.0 or 1.0, and `queued` as "has this node ever run" — true for every node that had, including ones asleep with nothing to do. The web debug JSON served the same values. Anyone reading them to find the stalled node would have been pointed at the wrong one. Moves total_exec_ms above the two bools to match every call site, and notes in the struct why the order is load-bearing. Verified in both directions: ona8cfe73the new case reports frames=8 total=0 queued=true; here total >= ema and both flags are false.
This commit is contained in:
@@ -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<ThreadPool>(1);
|
||||
pool->start();
|
||||
|
||||
auto node = make_pool_node<double_it>(pool, 64);
|
||||
Channel<int> 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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user