From 012b64dd3e1e27b5636456900316f5617194d192 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Wed, 5 Aug 2026 16:11:20 +0200 Subject: [PATCH] fix: the sentinel must not be delivered ahead of a queued value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- include/kpn/channel.hpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/include/kpn/channel.hpp b/include/kpn/channel.hpp index 6ddad69..afaf9fe 100644 --- a/include/kpn/channel.hpp +++ b/include/kpn/channel.hpp @@ -444,6 +444,24 @@ private: // delivered after every value pushed before it. bool take_sentinel(T& out) { if (!has_eof_.load(std::memory_order_acquire)) return false; + // Re-check emptiness *after* observing has_eof_, not before. + // + // Callers check the ring is empty and then call this, but the producer + // can push a value and publish the sentinel in the window between those + // two steps — so the sentinel would be delivered with a real value still + // queued behind it, breaking the "sentinel is strictly last" contract + // that downstream teardown depends on. a0c4bf5 closed the variant where + // the caller's emptiness check used a stale tail_ snapshot; this is the + // one where the check is fresh but simply too early. + // + // Checking here is what makes it sound: the producer publishes the + // sentinel with a release store *after* its ring pushes, so a consumer + // that has observed has_eof_ has also observed every tail_ advance + // before it. If the ring is non-empty now, those values genuinely + // precede the sentinel and must be delivered first. + if (head_.load(std::memory_order_relaxed) + != tail_.load(std::memory_order_acquire)) + return false; out = extract(std::move(eof_value_)); has_eof_.store(false, std::memory_order_release); stats_.record_pop();