push(), try_push() and push_blocking() fired push_callback_ only on the
empty->non-empty edge, and computed that edge from a head_ sampled before
the item was published. A PoolNode consumer decides whether to run again
from the level (count_ready -> approx_size), so a pop landing in that
window left both sides standing down:
producer (push) consumer (PoolNode firing)
------------------------ ----------------------------
samples t=782, h=781
-> was_empty = false, no wake
pops idx 781, head_ = 782
count_ready(): head_==tail_==782
-> not ready, gate released to Idle
tail_.store(783)
The item is in the ring, the node is idle, and no wake is outstanding.
The failure is absorbing: every later push then sees a non-empty ring, so
the edge never fires again and the node sleeps while its backlog grows.
Observed as a hang in bench_pipeline at (chain, depth=4, work_us=10,
shared pool): all pool workers asleep in worker_loop, the reader blocked
in pop(), and 218 items stranded in one channel with head_ stopped at
exactly the index where the edge was dropped.
Re-reading head_ after the tail_ store does not fix this. That is the
store-buffer pattern, and under acquire/release both sides may legally
read stale; forbidding it needs seq_cst on the producer's tail_ store and
head_ load *and* on the consumer's head_ store and tail_ load, a fence on
both hot paths. Firing unconditionally is correct by construction: the
callback runs after the publishing store, so a consumer that observes the
level at all observes the item. The redundant wakes are cheap --
on_input_ready re-checks the level and SubmitGate::claim() collapses a
wake arriving mid-firing into the firing already in flight.
The stress test named this exact hazard and could not detect it: it
asserted only 1 <= callbacks <= N, which a *missed* callback satisfies.
It now requires one callback per successful push, and fails at 1325/10000
against the old code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
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.
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.
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.
a0c4bf5 closed the variant where the caller's emptiness check ran against a
stale tail_ snapshot. This is the one where the check is fresh and simply too
early.
take_sentinel now re-checks emptiness *after* observing has_eof_, which is
what makes it sound rather than merely narrower: the producer publishes the
sentinel with a release store after its ring pushes, so a consumer that has
observed has_eof_ has necessarily observed every tail_ advance before it. A
non-empty ring at that point means those values genuinely precede the
sentinel, and returning false hands them over first.
Rates, since this is a race and the numbers are the evidence. The existing
"sentinel is strictly last" stress cases fail about 1 run in 15 on the commit
before this one and 0 in 25 after; they did not fail in 25 runs of the
pre-series baseline, so something in this series widened the window rather
than opened it. I could not pin down which change, and it does not much
matter: the interleaving is reachable from the code as written, and the
narrower version was never correct.
No new test. The two existing stress cases already assert exactly this and
are what caught it; a deterministic reproduction would need a seam inside
pop() that the fix then makes unreachable.
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.
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.
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.
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.
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.
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.
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 4b6e498 sets up.
Verified in both directions under -DKPN_SANITIZER=thread: the new contended
case reports three data races and a heap-use-after-free against the old
overwrite, and is clean with the handshake. Full suite 137/137, TSan clean
across unit and stress suites.
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.
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 5628447 and
9c5ce5f were both picking at, and each fix had to be applied to every copy.
Pre-existing, not introduced by the gate rewrite: the old two-atomic version
cleared queued_ in the same place, with the same code after it.
Verified with -DKPN_SANITIZER=thread. The race is intermittent — roughly one
run in three before the fix — so five consecutive clean runs of the unit suite
plus the contended channel stress suite, all zero. Full suite 132/132.
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 a8cfe73 patched. That commit found nodes
missing their startup wake because "enable_inputs() opens the channel
several statements before register_callbacks() installs the push callback",
and fixed it by re-asking the question with on_input_ready(). The gap it
described is this race: the callback is not merely late, it is being written
while another thread reads it.
INode gains prepare(), which installs callbacks and starts nothing. Networks
call it on every node before starting any of them, so every write happens
while the pipeline is idle and the callbacks are read-only once it is live.
start() calls prepare() itself when a node is used standalone, and prepare()
is idempotent so both paths are safe. The flag is never cleared: the
callbacks capture `this` and stay valid across a restart, so re-registering
them would only add a pointless write to a live channel.
a8cfe73's on_input_ready() stays, and is still needed — a network starts
nodes one at a time, so an upstream node can still push into this one
between its prepare() and its start(), where on_input_ready() returns early
on stop_flag_ and the empty->non-empty edge is spent. It is now a
level-triggered check against a benign ordering rather than cover for a race.
Verified with -DKPN_SANITIZER=thread: ten races before, none of these after,
across the unit suite and the contended channel stress suite. One unrelated
race remains, on overlapping fire_once invocations; it is pre-existing and
is fixed separately.
9c5ce5f established "a node never sleeps with a wake outstanding" and
implemented it as two independent atomics: queued_ for "a firing is in
flight", wake_pending_ for "a wake arrived during one". Two variables cannot
express that invariant, because the release side has to read and write both
and a wake can land in between:
producer (try_submit) worker (release_and_recheck)
------------------------ ----------------------------
CAS reads queued_ == true, fails
queued_.store(false)
wake_pending_.exchange(false) -> false
wake_pending_.store(true)
queued_ false, wake_pending_ true, nothing running and nothing scheduled —
exactly the state the invariant forbids. This is not a memory-ordering
subtlety; the interleaving holds under seq_cst.
SubmitGate replaces both with one atomic over three states, so "idle" and
"wake outstanding" are the same variable and no interleaving can produce
both. A release that finds a recorded wake keeps the claim and hands it to
the next firing, so the node is never momentarily idle while a submission
for it is in flight.
What this does not do is fix a reproducible hang. Every current call site
follows release_and_recheck() with a level re-check — on_input_ready(), or
outputs_have_space() on the parked path — which rediscovers the state a lost
wake would have signalled. The bug is masked, and I could not write a
node-level test that fails before and passes after; claiming otherwise would
be dishonest. The masking is a property of the call sites, not the
mechanism: any future early return that forgets its re-check reintroduces a
silent hang, and the pipeline has already been round that loop twice
(28e0667, then 9c5ce5f, each of which moved the stall rather than removing
it).
So the tests are structural. The state machine is pinned by contract tests,
and the defect it replaces is pinned by demonstration: LegacyGate in the
test file is the old protocol with a seam between the failed CAS and the
wake record, which makes the loss deterministic rather than something to
wait for. It also keeps the defect on record now that the code implementing
it is gone.
Also ignores build-*/ so a sanitizer build tree cannot be committed by
accident, which this commit did on its first attempt.
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 6a4f45f the parked payload arrives with size
0; here it arrives intact. The test drives raw channels rather than consumer
nodes so each step is forced rather than raced, and both channels are
capacity 1 — Channel fires the space callback only on the full->not-full
edge, so a roomy channel A would never resubmit the node and the retry would
never happen at all.
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&) {}
6595e6e made node outputs lossless, 28e0667 stopped them parking a worker,
a8cfe73 did the same for FanoutNode. These two were in none of them.
For ordinary values that is the familiar silent-loss problem: a dropped item
does not degrade a downstream result, it silently changes one, and the
consumer cannot tell it happened.
For a sentinel it is a hang. EOF is what tells every downstream node to shut
down and there is nothing after it to retry, so a filter that passes EOF by
predicate but drops it by backpressure produces a pipeline that never
terminates. scene-actor-extraction's decimator is exactly that shape —
`if (f.eof) return true;` in the predicate, feeding a chain whose slowest
node is an ONNX embedder, so the output is reliably full when EOF arrives.
Everything downstream then waits forever for a token that was discarded, and
the run has to be killed.
Both now route sentinels out-of-band via push_sentinel, which consumes no
ring capacity and cannot overflow, and retry ordinary values until taken.
Like FanoutNode and unlike a pool node, these own a private thread, so
waiting costs no scheduler worker and needs no space-callback park;
stop_flag_ is rechecked every pass so teardown cannot hang on a full output.
Time spent parked is charged to blocked rather than exec, so a held-up node
does not report as busy. An out-of-range router selector still drops by
design — that item was routed nowhere, which is not the same as lost.
is_sentinel_value moves from pool_node.hpp to traits.hpp. Every node type
that forwards a value needs it; these two not having it is the bug.
Verified in both directions. On c73edff the new case delivers 6 of 40 values
and never sets saw_eof; here it delivers 40 and terminates. EOF is emitted
exactly once, as a real source does — a test source that re-offered it would
mask the bug, since a later attempt could find the channel drained.
Note for downstream: the decimator is now a backpressure point rather than a
relief valve, so the source throttles to the face branch instead of quietly
thinning it. That is the intended behaviour, but it changes the shape of a
loaded run and is worth a benchmark comparison on a known clip.
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.
a8cfe73 appended 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: on a8cfe73 the new case reports frames=8
total=0 queued=true; here total >= ema and both flags are false.
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.** 6595e6e made node outputs lossless and
28e0667 stopped them parking a worker; FanoutNode was in neither and kept
`catch (ChannelOverflowError&) {}` per output. Whichever branch fell behind
lost items, silently, by an amount that depended on timing — so two runs of
the same input could disagree. deliver() now retries each output
independently until it is taken, rechecking stop_flag_ every pass so
teardown cannot hang on a full output. A fanout owns a private thread, so
waiting costs no scheduler worker.
**A node could start with a wake already outstanding.** start() enables the
input channel several statements before it installs the push callback, and
StaticNetwork starts nodes sources-first, so an upstream node is already
firing into the gap. A push landing there is accepted by the ring but wakes
nobody: push_callback_ fires only on the empty→non-empty transition, and at
that instant the callback is null. Every later push sees a non-empty ring
and stays silent, so the node is never submitted. Asking on_input_ready()
once at the end of start() converts the missed edge into a state check.
The signature is distinctive — zero items delivered, not a stall partway.
Under `ctest -j4` on a loaded machine it reproduced 7 times in 24 and never
in 10 unloaded runs, which is almost certainly the "~1 run in 20" hang
28e0667 recorded as known-incomplete.
**NodeSnapshot now carries scheduling state and a true exec total.** queued
and wake_pending make the 9c5ce5f invariant observable at runtime; it could
previously only be inspected in a debugger, and the bug does not reproduce
under one. total_exec_us is a real sum — frames × ema_exec_us tracks the
tail of a run, not the whole of it, and diverges badly on a workload whose
per-frame cost varies. Both are exposed over the web debug JSON so a wedged
pipeline can be interrogated without attaching to it.
28e0667 stopped nodes blocking a worker on a full output, but replaced an
intermittent hang with a quieter one: the pipeline still wedged about 2
runs in 30, now with every worker idle in pthread_cond_wait rather than
asleep in a push. Nothing was blocked; nothing had been woken.
try_submit discarded any wake arriving while queued_ was up:
if (queued_.compare_exchange_strong(expected, true, ...))
scheduler_->submit(...);
// else: silently gone
Wakes are edge-triggered — a channel fires its space callback once, on the
transition — so a dropped one never returns. A node could park a value,
release its worker, and sleep forever holding exactly the output its
consumer was waiting for, while its producer parked on an input channel
that would never drain.
try_submit now records the drop in wake_pending_, and release_and_recheck()
consumes it at every site that releases a node, giving one invariant: a
node never sleeps with a wake outstanding. This subsumes the two ad-hoc
re-checks added for the parked-retry and normal push paths, which only
moved the stall (1211 items to 6472) because each new early return was a
fresh chance to drop a wake. self_stop keeps a plain store — honouring a
pending wake there would resubmit a dead node.
Adds "a saturated chain never stalls", which reproduces this in about a
second where the pipeline needed ~30 runs. It asserts *progress does not
freeze* rather than a completion total: capacity-1 channels are slow, and
slow must never be reported as wedged. Verified in both directions — it
stalls after 1211 items on 28e0667 and passes here.
The existing chain test could not catch it: 40 items drain before any
strand occurs, and its producer emits forever, so fresh input keeps
re-triggering on_input_ready() and flushing the stranded value.
Tests: 122/122.
push_blocking parked a scheduler worker inside the push. Nodes own a
private single-thread pool, so the parked thread was the only one that
could drain that node's own input — hold-and-wait, and under sustained
backpressure four nodes of a five-node chain slept in nanosleep at once.
channel.hpp already warned about this for sentinels; it applies just as
much to data pushes.
The scheduler was purely input-driven: on_input_ready() wakes a node when
input arrives, with no counterpart for "my output has room". Lacking that
signal, blocking the thread was the only way to handle a full output.
This adds the missing half.
- Channel::try_push + has_space + set_space_callback; the callback fires
from both pop() and try_pop_now().
- PoolNode/PoolObjectNode keep a one-slot pending_ buffer with per-element
done flags, so a retry cannot duplicate an already-accepted element. One
slot suffices because queued_ admits at most one fire_once per node.
- The re-check after clearing queued_ closes the lost-wakeup race where a
space callback fires while the flag is still up and is swallowed.
Two bugs surfaced once nodes actually parked, both fixed here:
- pop_one reports an *empty* channel as ChannelClosedError, which is also
the node's "upstream finished, self-stop" signal. A node woken by output
space with empty inputs therefore killed itself. fire_once now releases
the worker when its inputs are not ready rather than falling through.
- The drained-park path resubmitted unconditionally instead of via
on_input_ready(), firing nodes with nothing to read.
compute_priority is now output-aware: mean output fill is deducted from
mean input fill, mapped as 0.5·(1 + in - out). Input fill alone asks only
"how much work is waiting for me"; a node whose outputs are already full
cannot deliver, so running it just parks it again and wastes the slot
while the node that would drain that channel waits behind it. The
scheduler now favours whoever is furthest downstream of a bottleneck.
Also adds a network-level error listener. A node's exception was discarded
at the node boundary and survived only as a Closed event, which reports
that a node stopped but not why — that missing detail is what made the
above slow to diagnose. INode::set_network_error_callback plus
StaticNetwork::set_error_handler forward it to the application.
Tests: 121/121. test_backpressure_deadlock drives a five-node chain with
capacity-2 channels against a slow sink and fails on the old code. The
four test_pool_node overflow tests now assert parking rather than the
removed drop-and-report behaviour.
Known-incomplete: a rare hang remains, roughly 1 run in 20 against a 300s
timeout, down from every run failing. Committed because the fix is a large
strict improvement and the residual case needs its own reproduction.
Every node output used the throwing push(), so a consumer falling behind cost
values rather than time. push_blocking() already existed on Channel and
OutputPort — "wait for the consumer to drain instead of dropping; the producer
just runs slower" — but nothing called it.
A dropped frame does not degrade a downstream result, it silently changes one,
and the consumer has no way to tell it happened. For any pipeline whose output
is a claim about its input, that is corruption rather than degradation.
Safe because sentinels are already handled out-of-band, above this path: only
data blocks, so the EOF token that unwinds the network can always overtake a
stalled data path. That is exactly the hold-and-wait deadlock the push_sentinel
comment warns about, and the reason it is not reachable here.
Measured on a downstream consumer (face pipeline, 77s clip at 5 fps, expected
385 sampled frames):
before 65 frames written, 320 dropped at one node, 29s
after 385 frames written, 0 dropped, 17s
Faster, not slower — a dropped frame has already cost its decode, and the
overflow exception cost more. Two consecutive runs now produce byte-identical
output, which they did not before: what got dropped depended on timing, so the
same command could yield different results.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds three pieces needed to build one KPN network and reuse it across many
replays/configs instead of tearing down and rebuilding per run:
- Channel<T>::push_blocking (+ IVariantChannel/VariantChannel forwarding):
lossless backpressure push that waits for space instead of dropping when
the ring is full. PyNode's run_loop now uses it so a downstream consumer
lagging behind never silently drops a frame.
- PyNetwork::node_ptr / node_stats: raw node handle by name (for a binding
to dynamic_cast to a concrete wrapper and call functor-specific runtime
setters) and a per-node timing snapshot for profiling.
- ObjectVariantNodeWrapper: variant-node adapter for functors that need
runtime-constructed state (a Config, a loaded gallery), mirroring
VariantNodeWrapper's channel plumbing but backed by ObjectNode<Obj>.
Built and used downstream in scene-actor-extraction's sae_kpn Python replay
bindings for repeated threshold-sweep evaluation of the same pipeline.
Channel<T>::pop() surfaced the out-of-band sentinel from its empty branch
using the tail_ snapshot taken at the top of the loop. Under contention the
producer can push more values *and* the sentinel in the window between that
snapshot and take_sentinel(), so pop() could return the sentinel while real
values still sat in the ring — the sentinel jumping ahead of values pushed
before it. No value was lost (a consumer that keeps draining still receives
them, and approx_size() keeps counting them so a PoolNode reschedules), but a
consumer treating the sentinel as a hard "last message" barrier would act on
EOF early.
Re-confirm emptiness against a fresh tail_ load before taking the sentinel.
Costs one acquire-load on the empty-ring path only; never runs in steady
state. The spin and post-spin takes already reload tail_ on the line above
them; try_pop_now() already reads tail_ fresh in the same branch — both were
correct and are unchanged.
The two sentinel stress cases now assert the strict "sentinel is last, after
every value" ordering (previously relaxed to avoid the flake this fixes).
Verified TSan-clean (2606 assertions, no data races) over repeated runs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Python Network holds each PyNode's callable, forming an
uncollectable instance -> callable -> globals() -> instance cycle that
tripped nanobind's leak check at interpreter shutdown.
Implement tp_traverse/tp_clear type slots on the Network binding so
Python's cyclic collector can see through the C++-held callables and
break the cycle. PyNode exposes its callable; PyNetwork visits and
clears them. Wired into both binding sites (auto_bind and the legacy
register_py_network).
Channel::push() drops values on overflow (the intended backpressure
policy for data), and PoolNode swallows the resulting ChannelOverflowError.
For a control sentinel like EOF this is fatal: a single dropped EOF under
backpressure wedges every downstream pop() forever, so the pipeline never
tears down.
Deliver sentinels out-of-band instead. Channel::push_sentinel() stores the
token in a dedicated slot that does not consume ring capacity, so it can
never overflow and — crucially — never blocks the caller. That non-blocking
property is essential: each KPN node has a single worker thread, so a
*blocking* push would park that thread and stop it draining its own input,
cascading into a hold-and-wait deadlock under backpressure. The consumer's
pop()/try_pop_now() drain the ring first, then deliver the sentinel, so it
always arrives after every value pushed before it.
approx_size() (which node readiness checks call) counts a pending sentinel
as consumable work, so a channel carrying only a sentinel still schedules
its consumer's next fire — without this the token would sit undelivered and
the pipeline would still deadlock at teardown.
PoolNode/PoolObjectNode route values carrying an eof flag (direct .eof or
nested .source.eof) through push_sentinel via a SFINAE-safe is_sentinel_value
trait; all other values keep the existing lossy throwing push. The trait
compiles to false for types without an eof convention, so this is a no-op
for pipelines that don't use one.
Verified end-to-end: scene_analyze now reaches EOF, flushes its output, and
exits cleanly instead of hanging.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dtourolleandClaude Opus 5