fix/kpn-wedging-audit #3
Merged
dtourolle
merged 20 commits from 2026-08-05 16:24:02 +00:00
fix/kpn-wedging-audit into master
20
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 dtourolle
|
00245f5760 |
fix: Network::set_error_handler must actually deliver the handler
The handler was stored in a member and never read. A node's exception was discarded at the node boundary and the only surviving evidence was a Closed event, which reports that a node stopped but not why — the difference between a diagnosis and a guess. StaticNetwork has always wired this; Network accepted the handler and silently dropped it, which is worse than not offering the setter, because the caller believes they have a listener. start() now delivers it to each node, exactly as StaticNetwork does. The type changes with it. It was void(name, exception_ptr), which cannot express the keep-running decision the node side needs — so it is now NodeErrorHandler, the same alias StaticNetwork uses. That is a breaking change in principle; in practice nothing in the tree called this setter, which is how it stayed dead long enough to be worth finding. Verified by the new case: the node throws, the handler receives the name and the exception, returns true, and the node goes on to process the next value. 148/148. |
||
|
dtourolle
|
7a3e96cc99 |
fix: the watchdog must be interruptible, or stop() waits for its next tick
start_watchdog looped on std::this_thread::sleep_for(watchdog_interval_), and request_stop() cannot wake a sleeping thread. stop_watchdog()'s join therefore blocked until the current sleep expired: three seconds on every teardown at the default interval, and unbounded for anyone who set a long one to keep the periodic report quiet. Now a condition_variable_any waited on with the stop token, so request_stop() ends the wait immediately. Found while writing the next commit's test, which sets a one-hour interval to silence the report and consequently hung for an hour in stop(). The test needs one non-obvious thing, and says so: a pause between start() and stop(). Without it the test races the watchdog — stop_watchdog() runs before the thread has entered its loop, the token is already set when it does, and it exits without ever waiting. That passes against the bug as well as the fix, which is exactly what the first version of this test did. Verified in both directions: without the fix the case is killed at a 25 s timeout; with it, stop() returns in 0 ms. |
||
|
dtourolle
|
80c2b1fb2f |
fix: try_push must distinguish delivered from discarded
try_push returned bool, and returned *true* for a closed channel — so "the
value arrived" and "the value was thrown away because nobody is listening"
were the same answer.
Every caller was nonetheless correct, because both cases mean "stop trying,
do not park and retry". But nothing above the channel could tell the two
apart: a node counting successful pushes counted discards among them, and the
only record of the loss was the channel's own drop counter, visible solely to
whoever read the diagnostics table.
Now a three-way PushResult { Taken, Full, Closed }, matching the shape
SentinelResult already uses. Behaviour is unchanged at every call site —
each treats Closed the same as Taken, and only Full parks — but the
distinction is now available to anyone who needs it, and a scoped enum means
a future caller cannot silently reintroduce the conflation with `if (push)`.
deliver_one benefits immediately: it no longer reaches its teardown path for
a closed channel, only for one that is still full, so the last-ditch throwing
push it does there to record the loss now records an overflow rather than a
drop the channel had already counted.
|
||
|
dtourolle
|
012b64dd3e |
fix: the sentinel must not be delivered ahead of a queued value
pop() and try_pop_now() observe the ring empty and then call take_sentinel().
The producer can push a value *and* publish the sentinel in the window between
those two steps, so the sentinel was delivered with a real value still queued
behind it — breaking the "sentinel is strictly last" contract that downstream
teardown depends on, and losing that value to any consumer which, like the
stress cases here, treats the sentinel as EOF and stops draining.
|
||
|
dtourolle
|
7b7f631e6d |
fix: a shared resource must be able to release its waiters
SharedResource::acquire() blocks on a condition variable whose predicate only becomes true when release() hands over ownership. No timeout, no stop condition. A node parked there was not observing stop flags, so teardown had no way to reach it: the worker never returned, the pool's join never completed, and shutdown waited on a resource nobody was going to release — which is precisely the situation when the holder is being stopped too. close() wakes every waiter and refuses further acquisitions, and the waiters leave through ResourceClosedError, which is an exception the node error path already handles rather than a new mechanism. StaticNetwork calls it on registered resources at the top of halt() and shutdown(), before stopping any node, since a node stopped while parked cannot respond to being stopped. The handover needed care in two places. A waiter woken by close() has not been given ownership, so it takes no Guard and leaves held_ exactly as it found it; and release() now skips handing over to waiters when closed, because handing ownership to a thread that is on its way out would leave held_ true with nobody holding it. reopen() is there for reuse across runs, which the persistent-pipeline work will want; teardown does not need it. Verified in both directions: without close() the waiter thread never returns and the test's join blocks; with it the waiter leaves through ResourceClosedError while the holder still has the resource. 145/145. |
||
|
dtourolle
|
97670d8ba3 |
fix: stop() must not return while a firing is still running
stop() set the flag, disabled the inputs and returned, leaving an executing fire_once touching input_channels_, stats_ and pending_ while the caller went on to destroy them. The comment was explicit about it: callers wanting the guarantee should call scheduler_->drain() first. But ~PoolNode calls stop(), and a destructor cannot ask its caller to have done that. A node with a private pool survived by accident, because Node::stop() calls pool->stop() and that joins the worker. A node sharing a pool — which make_pool_node exists to create — had nothing joining it, so its own destructor raced the firing. stop() now waits on the submit gate, which is claimed for the whole of a firing and released as its last act. A queued but unstarted firing also holds it and will run, observe stop_flag_ and release, so the pool must still be running when stop() is called; that is already the documented order and what Node/ObjectNode do. Two ways it declines to wait. It is bounded at five seconds, because a node function that never returns must not convert teardown into a hang — it warns and continues. And it returns immediately when called from the firing thread itself, since an error handler that stops its own node would otherwise wait for a firing that is waiting for it. Verified in both directions: with the wait removed, stop() returns while the node function is still sleeping and the flag it sets on the way out is still false. 143/143. |
||
|
dtourolle
|
b9698fae60 |
fix: idle workers must sleep while another worker is busy
The wait predicate was `stopped_ || total_ > 0`, and total_ counts queued *plus executing*. So while any one task ran, every other worker's predicate was true: wait() returned immediately and the worker spun through try_pop, try_steal and back to wait at full speed, try_lock-ing every peer queue on each pass. Measured on this tree, 8 workers and one 300 ms task: 1991 ms of CPU and 19205 voluntary context switches, against 0.4 ms and 10 with the fix. Call it six and a half cores burned for the duration of one sleeping task. Sleeping requires "no work is *waiting*", which total_ cannot express, so queued_ is now tracked separately: incremented on submit, decremented when a task leaves a queue, and adjusted for the tasks stop() discards. total_ stays as it was for drain(), which genuinely does need to know about executing work. The worker exit condition moves to queued_ for the same reason — waiting for total_ to reach zero meant waiting for someone else's task to finish, which a worker cannot help with and would spin through until it did. PoolSnapshot's queue depth stops being an estimate as a side effect. Latent for this pipeline, where each node owns a private single-thread pool and there is no idle peer to spin. Any use of a shared pool, which make_pool_node exists for, hits it immediately. Reproducing it needs the right trigger, and the test says so, because my first attempt got it wrong and passed against the bug: a worker that has never been woken stays blocked in wait() and never re-evaluates the predicate. The spin only appears once a worker *finishes* something and re-enters the loop while a peer is still busy, so the case submits one long task plus a trivial one per remaining worker. Submitting only the long task measures nothing. Verified in both directions: 1969 ms of CPU before, 0.35 ms after, against a 300 ms threshold. Full suite 142/142. |
||
|
dtourolle
|
87c5f98d04 |
fix: start and stop in the topological order that was computed
make_network computes Topo for the cycle check and then discarded it. The node vector was filled in edge-declaration order — and then named user_nodes_topo_ and relied upon as if it were sorted. halt() stops in its reverse, and shutdown() walks it forwards stopping each node and draining its outputs before the next, which is a graceful drain only if the order really is sources-first. It held for every network in this tree because edges happen to be declared in pipeline order, so the two coincided. Declared any other way — which is legal and which make_network otherwise accepts in silence — shutdown stops a consumer before its producer and discards whatever was queued in front of it. The order now comes from Topo::topo, which is already sources-first. Fanout nodes appear there too and are skipped, since they are owned separately in fanout_storage; they are still started after the user nodes and stopped before them, so a fanout sitting between two user nodes is not staged precisely during a drain. That is a smaller gap than the one being closed and is left alone rather than restructured on the way past. The test declares the sink edge first and the source edge last, and asserts through shutdown() rather than by reading the order back — the order is private, and what it buys is the point. A sources-first shutdown lets the backlog queued in front of the slow relay reach the sink; stopping the relay first discards all of it. Worth recording how this went, because it is the more useful half: the new test segfaulted, 12 runs in 20. Not a fault in the ordering change — it was stopping sources first that finally put a live consumer behind a dead producer, which is the condition the previous commit's crash needs. The ordering fix did not introduce that bug, it made it reachable. Verified in both directions: with declaration order the sink receives nothing after shutdown begins; with topological order it receives the whole backlog. |
||
|
dtourolle
|
abbb2d4770 |
fix: submitting to a stopped pool must be refused, not fatal
ThreadPool::stop() ends with queues_.clear(), and submit() went straight to
queues_[target] with no check. A submission arriving after stop indexed an
empty vector and segfaulted.
This is not a contrived teardown ordering. A node's space callback fires from
whichever thread drained the channel, and that thread belongs to the
*consumer*; the callback it runs belongs to the *producer*. Stop the producer
first — which a sources-first shutdown does by design — and the consumer keeps
draining its backlog, firing the producer's space callback into a pool that
has already been torn down:
ThreadPool::submit
<- source node's space_callback
<- Channel<int>::try_pop_now (relay draining its input)
<- relay fire_once
The static-network shutdown case in the next commit crashed about 12 runs in
20 on this. It survived until now because halt() stops in reverse topological
order — consumers first — so the producer whose callback might fire is always
still alive. shutdown() stops sources first and does not have that protection.
Reading stopped_ without a lock would not fix it: the window between the read
and the indexing is exactly where clear() runs. submit() takes a shared lock
and stop() an exclusive one, so submissions still proceed in parallel with
each other while being serialised against teardown. stop() sets the flag under
the lock, releases it to join — a worker's task may itself call submit, and
holding the lock across the join would deadlock against that — then retakes it
to destroy the queues.
Refusals are counted rather than silent. A teardown race is expected, but a
node repeatedly trying to run after its pool is gone is worth being able to
see. try_submit also checks stop_flag_ first, so a stopped node cannot claim
the submit gate and leave it held.
Not a smart-pointer problem, for anyone reading the crash: nothing here is
owned by a raw pointer. It is std::vector::operator[] on a vector that was
emptied by another thread.
Verified in both directions: with the guard removed the new scheduler cases
segfault; with it they pass.
|
||
|
dtourolle
|
0f277c0f98 |
fix: the drain loops must terminate, and must drain the right channels
shutdown()'s drain step was an unbounded
while (anything, anywhere, is non-empty) poll every channel
Three defects in one loop.
It drained the wrong thing. Stopping a node should wait for that node's own
outputs before moving to the next layer; this waited for the entire graph to
fall idle each time. The dynamic Network's version made it explicit — it took
a node name and ignored it. Both now track which node feeds each probe and
wait only on those.
It had no deadline, so anything wedged downstream turned a graceful shutdown
into the hang it exists to avoid. Now bounded two ways, because a stalled
consumer and a slow one fail differently: a deadline for fill that never
changes, and a no-progress counter that keeps waiting as long as the queue is
shrinking, so a slow drain is not cut short merely for taking a while.
And it could fail to terminate with nothing wedged at all. current_fill came
from a snapshot that loaded tail_ before head_. A concurrent pop between the
two reads yields a head_ past the sampled tail_, and the unsigned difference
wraps to ~2^64 — so a poll for "is it empty yet" runs forever on a channel
that is in fact empty. Both indices only ever increase, so loading head_
first can at worst under-report a concurrent push, which this loop tolerates
and a wrap does not. size() and snapshot() are both corrected; size() feeds
approx_size(), which is what node readiness checks call.
Giving up is now reported rather than silent, because undrained data at that
point is about to be discarded by the stop that follows, and a graceful
shutdown quietly dropping values is the thing worth knowing about.
Verified in both directions: with the old loop the new case is killed at a
30 s timeout; with this it returns in under a second, having reported four
items its wedged consumer never took. Full suite 138/138.
Note the drain timeout is per node and defaults to 5 s, so a graph of N
stalled nodes can still take N x 5 s to shut down. That is a deliberate
trade against cutting off legitimate slow drains, and set_drain_timeout()
exists for callers who want it tighter.
|
||
|
dtourolle
|
139bfbb794 |
fix: the sentinel slot holds one token and refuses a second
push_sentinel wrote eof_value_ unconditionally. Offering a second token
before the first was taken did two wrong things at once.
It lost the first silently, and a lost EOF is not a lost frame — it is the
token every downstream node is waiting for in order to shut down, so losing
it wedges the pipeline.
And it wrote the storage while the consumer could be moving the previous
value out of it. I expected that to be a stale read; ThreadSanitizer shows
it is worse. On the shared_ptr storage that non-trivial types use, the
racing write tears the refcount, and the stress case added here reports
heap-use-after-free in extract() alongside the data race.
try_push_sentinel now refuses when the slot is occupied, which turns the
slot into a correct SPSC handshake: the producer is the only writer of
eof_value_ and the only one that sets has_eof_, the consumer is the only one
that clears it, so observing it false is what licenses the write. Refusal is
recorded as a drop, and PoolNode reports it through the overflow event
callback, because a refused control token going unnoticed is the failure
this commit exists to stop.
Refusing rather than queueing is deliberate. Two control tokens on one
channel means the stream ended twice, which is a caller protocol error and
not backpressure; parking and retrying would spin against a slot only the
consumer can free, and there is no sensible second value to deliver after
the end of a stream. The non-consuming try_push_sentinel exists so a refused
token is still the caller's to report — the consuming push_sentinel cannot
offer that, since the value has already been moved into its parameter.
Single-shot EOF is what every current caller does, so this is latent for
them today. It stops being latent the moment a pipeline is reused for a
second input, which is what the persistent-pipeline work in
|
||
|
dtourolle
|
8d319eeb88 |
fix: an empty channel is not a closed one
pop_one reported an empty channel the same way it reported a closed one, by throwing ChannelClosedError, and fire_once treats that as "upstream is finished" and calls self_stop(). self_stop disables the node's own inputs *and* outputs, so a benign empty read does not merely skip a frame — it kills the node and, through the disabled channels, whatever depended on it. A node genuinely does get woken with empty inputs: a space callback fires when its output drains, which has nothing to do with input arrival. fire_once already guards against it by checking readiness before popping. That guard is the live protection and it works; this commit makes the thing it is guarding non-lethal. So the pop_one path changed here is unreachable today, and I would rather say that than imply a fixed hang. Its value is that the readiness check is now a performance detail rather than the only thing standing between a routine wake and a dead pipeline. Three separate comment blocks in fire_once exist to warn about exactly this hazard; they were added because it had already been hit during development, and the conflation they warn about is what this removes. Verified in three directions. With the guard and the distinction: passes. With the guard removed but the distinction present: still passes, which is the point — the new ChannelEmptyError path catches what the guard used to. With both removed, reproducing the original code: the node self-stops on the firing that has nothing to read, the next value throws "channel closed" out of its own output channel, and the relay handles one item instead of two. |
||
|
dtourolle
|
15e993f6ca |
fix: two firings of the same node must not overlap
fire_once released the submit gate and then kept working:
release_and_recheck(); // gate is now free
if (stop_flag_) return;
if (pending_) { ... } // still reading node state
on_input_ready();
The moment the gate is free another worker may enter fire_once for the same
node, so this invocation's reads of pending_ raced with the next one's writes
to pending_done_. ThreadSanitizer caught exactly that, between a firing
submitted by release_and_recheck and one submitted by try_submit.
The race is the visible half. The real damage is to the one-slot park, which
is sound only because "at most one fire_once runs per node at a time" — the
comment on pending_ says so explicitly. With two firings live, one can park a
value into the slot the other is about to overwrite, and the overwritten value
is gone with no drop recorded anywhere. That is silent data loss under
backpressure, from a node that reports itself healthy.
finish_firing() replaces release_and_recheck() at every exit: it evaluates the
follow-up decision — parked and waiting on output space, or drained and
waiting on input — while the claim is still held, and releases the gate as the
last thing the firing does. Nothing touches node state afterwards.
This also collapses three near-identical resubmit tails into one, which is
worth something on its own: the divergence between them is what
|
||
|
dtourolle
|
a5c016833d |
fix: install channel callbacks before any node runs
ThreadSanitizer reported ten data races on a plain multi-node network, all
the same one:
Read in Channel::try_push -> push_callback_() (worker thread)
Write in Channel::set_push_callback -> push_callback_ = ... (main thread)
A node's push and space callbacks are std::function members living on
channels it shares with its neighbours. register_callbacks() wrote them from
inside start(), and a network starts its nodes one at a time — so by the
time node N is being started, nodes 1..N-1 are already running and pushing
into N's input channel, reading the very std::function that start() is
assigning. Concurrent read and write of a std::function is a data race on
its vtable pointer and buffer, not a benign one.
This is the cause of the symptom
|
||
|
dtourolle
|
f53af260a2 |
fix: make the submit gate a single atomic
|
||
|
dtourolle
|
5628447ea8 |
fix: never self-move the parked output tuple
push_outputs ended with
else pending_ = std::move(result);
and the retry path calls it as push_outputs(std::move(*pending_), …), so on
that path `result` is the parked tuple itself. The assignment was a
self-move-assignment. std::tuple's is elementwise, and libstdc++'s
std::vector does not guard against self-move: _M_move_assign swaps its data
into a temporary, which is then destroyed. The vector ends up empty.
So the first park was clean — the argument there is a local temporary — and
the second erased the payload. The value was still delivered, still in
order, still counted, just empty. Downstream cannot distinguish that from a
frame on which the node genuinely found nothing, which is why it would never
surface as an error: in scene-actor-extraction it reads as "no faces in this
frame" and the run completes with a quietly wrong answer.
Scope, stated precisely because I first got it wrong: this needs a node with
*two or more* outputs. With one output the only thing that resubmits a
parked node is that output's own space callback, which by definition fires
when there is room, so the retry always succeeds and never reassigns. With
two, output A draining resubmits the node while output B is still full — the
retry skips A (already delivered, tracked in pending_done_) and fails on B,
and that is the reassignment that eats B's payload.
Every node in the scene-actor-extraction pipeline currently has exactly one
output, and the fanout is a separate class that does not use pending_, so
this is latent there rather than active. It is reachable by any multi-output
node under backpressure, which the library supports and documents.
Verified in both directions: on
|
||
|
dtourolle
|
6a4f45f111 |
fix: a filter or router must not drop on a full output
RouterNode and FilterNode were the last nodes on a data path still using the
throwing push() and swallowing the result:
try { out_ch_->push(val); } catch (const ChannelOverflowError&) {}
|
||
|
dtourolle
|
c73edffe5c |
chore: delete the unreachable duplicate of the parked-retry block
PoolNode::fire_once carried the pending_ retry block twice, verbatim. The first copy returns on every path through it — parked, drained, or not pending at all — so the second was dead code from the moment it appeared. PoolObjectNode, which is otherwise a line-for-line twin of PoolNode, has it once. No behaviour change; the deleted 25 lines were unreachable. Worth doing before the fixes queued behind it, each of which has to be applied once per copy of this function. The duplication is a symptom: PoolNode and PoolObjectNode are ~400 lines of near-identical code maintained by parallel edit, and a block getting pasted twice into one of them is exactly the failure that arrangement invites. Factoring the shared body out is a larger change and wants its own review. |
||
|
dtourolle
|
091211cb19 |
fix: NodeSnapshot fields must line up with what nodes supply
|
||
|
dtourolle
|
a8cfe7300a |
fix: a lossless fanout, a node that starts awake, and the instrumentation that found them
Three changes from one debugging session on the intermittent wedge, kept together because the instrumentation is what made the other two findable. **Fanout was never made lossless.** |