From 7102412a9fc768edc12324ddf31eeed129e56fa9 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Thu, 27 Aug 2026 20:16:52 +0200 Subject: [PATCH] fix(udpscope): reconstruct lost accumulated bursts from the packet counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the decoder was estimating something the wire states exactly. FrameView::counter increments once per update, so a gap of g means g-1 lost datagrams; reinstating their duration restores the hole precisely, with no threshold and no dependence on arrival time. The arrival-anchor comparison survives only as a backstop for what the counter cannot express — a producer restart, a counter stuck at zero, a wrong declared rate — and can no longer step a signal's timestamps backwards, which the ring and trigger forbid. Also from review: guard the time-signal lookup against a frame carrying more signals than the installed table, and give FrameBuilder a counter parameter. Leaving it at zero had hidden the counter rules from every test, and made the hrt-gap test vacuous — under uniform arrivals the hrt path and packetBurst agree by construction, so it could not tell which branch answered. Its arrivals now carry zero-mean jitter. Each new assertion was proven non-vacuous by sabotage: dropping the gap term, the backward guard, or the hrt branch fails exactly its own test. Co-Authored-By: Claude Opus 4.6 --- Client/udpscope/FrameDecoder.cpp | 69 ++++++++--- Client/udpscope/FrameDecoder.h | 11 +- Client/udpscope/tests/FrameDecoderTest.cpp | 109 ++++++++++++++---- docs/superpowers/plans/2026-08-27-udpscope.md | 75 +++++++----- 4 files changed, 186 insertions(+), 78 deletions(-) diff --git a/Client/udpscope/FrameDecoder.cpp b/Client/udpscope/FrameDecoder.cpp index 482a714..b217420 100644 --- a/Client/udpscope/FrameDecoder.cpp +++ b/Client/udpscope/FrameDecoder.cpp @@ -8,15 +8,17 @@ namespace udpscope { static constexpr double kDefaultDt = 1.0e-3; /** - * How far the forward-chained prediction for an accumulated burst may sit from - * where arrival time says it should be before the chain is abandoned. + * How far a chained burst prediction may sit from where arrival time says it + * should be before the chain is abandoned and time is re-anchored on arrival. * - * A kernel draining a backlog of queued datagrams can legitimately put the - * prediction a few hundred milliseconds ahead of arrival, so the threshold has - * to be well clear of that. Anything larger is not delivery jitter: it is lost - * packets or a declared sampling rate that does not match the producer's real - * one, and both must resynchronise rather than accumulate forever. Same value - * and same reasoning as ClockOffset::kRecalibThresholdS. + * This is a backstop, not the primary mechanism: the packet counter normally + * accounts for lost datagrams exactly, so the prediction and arrival agree. + * It catches what the counter cannot describe — a producer restart (the + * counter returns to zero), a counter that never advances, and a declared + * sampling rate that does not match the producer's real one. A kernel draining + * a backlog of queued datagrams can legitimately put the prediction a couple of + * hundred milliseconds from arrival, so the threshold sits well clear of that. + * Same value and same reasoning as ClockOffset::kRecalibThresholdS. */ static constexpr double kBurstResyncThresholdS = 0.5; @@ -59,7 +61,8 @@ bool FrameDecoder::packetBurst(uint32_t idx, uint32_t nElems, double wallNow, bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx, std::vector& tsOut) { tsOut.clear(); - if (idx >= signals_.size() || idx >= f.numSignals || f.counts == nullptr) { + if (idx >= signals_.size() || idx >= f.numSignals || + f.counts == nullptr || f.values == nullptr) { return false; } @@ -70,7 +73,12 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx, const double wallNow = f.recvTime; SigState& st = state_[idx]; - const bool hasTimeSig = d.hasTimeSignal(f.numSignals); + /* hasTimeSignal() bounds the index against the FRAME's signal count, but + * the time signal's type code is read from our own table, whose size is + * independent — a frame carrying more signals than the installed table + * (briefly possible after a CONFIG change) would otherwise read past it. */ + const bool hasTimeSig = d.hasTimeSignal(f.numSignals) && + d.timeSignalIdx < signals_.size(); const uint32_t tIdx = hasTimeSig ? d.timeSignalIdx : 0u; const double tScale = hasTimeSig ? TimeSignalScale(signals_[tIdx].typeCode) @@ -127,27 +135,50 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx, const double arrivalAnchor = wallNow - static_cast(nElems - 1u) * dt; - /* Chaining from the end of the previous burst is immune to arrival + /* Chaining onto the end of the previous burst is immune to arrival * jitter — a kernel draining several queued datagrams microseconds - * apart still yields contiguous timestamps. But a pure chain is - * blind: one lost datagram, or a declared rate that does not match - * the producer's real one, displaces every later sample and never - * recovers. So the chain is a PREDICTION, checked each packet - * against arrival and abandoned when the two disagree by more than - * a delivery backlog can explain. That bounds the error instead of - * letting it accumulate. */ + * apart still yields contiguous timestamps. What a bare chain gets + * wrong is loss: it closes the hole a dropped datagram left, and + * every later sample is then dated early for the rest of the run. + * + * The wire says exactly how much is missing. counter increments + * once per update, so a gap of g means g-1 lost packets, each + * carrying (as far as we can tell) as many samples as the last one + * we saw. Reinstating that duration keeps the chain honest without + * consulting arrival time at all. */ double base = arrivalAnchor; if (st.lastEmittedValid) { - const double predicted = st.lastEmittedEnd + dt; + /* Unsigned subtraction wraps, so this stays right across the + * counter's own 2^32 rollover. */ + const uint32_t gap = f.counter - st.lastCounter; + const double lost = (gap > 1u) + ? static_cast(gap - 1u) * + static_cast(st.prevAccCount) + : 0.0; + const double predicted = st.lastEmittedEnd + dt * (1.0 + lost); + + /* Backstop for what the counter cannot express: a producer + * restart, a counter stuck at zero, or a declared rate that is + * simply wrong. Beyond this the chain is not recoverable and + * arrival time is the better of two bad answers. */ if (std::fabs(predicted - arrivalAnchor) <= kBurstResyncThresholdS) { base = predicted; } + /* Re-anchoring must never move time backwards: the ring, the + * trigger and the exporter all assume a signal's timestamps + * increase. A backward resync would be indistinguishable from + * corruption downstream, so give up the correction instead. */ + if (base <= st.lastEmittedEnd) { + base = st.lastEmittedEnd + dt; + } } tsOut.resize(nElems); for (uint32_t e = 0; e < nElems; e++) { tsOut[e] = base + static_cast(e) * dt; } st.lastEmittedEnd = tsOut[nElems - 1u]; + st.lastCounter = f.counter; + st.prevAccCount = nElems; st.lastEmittedValid = true; return true; } diff --git a/Client/udpscope/FrameDecoder.h b/Client/udpscope/FrameDecoder.h index 9f912c5..a00e8b4 100644 --- a/Client/udpscope/FrameDecoder.h +++ b/Client/udpscope/FrameDecoder.h @@ -54,12 +54,11 @@ private: bool lastAccValid = false; uint32_t prevAccCount = 0; /** For accumulated scalars with a declared sampling rate: end timestamp - * of the most recently emitted burst. The next burst is PREDICTED to - * start one sample period after it — immune to arrival-time jitter — - * but the prediction is discarded when arrival time disagrees with it - * by more than a delivery backlog can explain, so packet loss cannot - * displace the trace permanently. */ - double lastEmittedEnd = 0.0; + * of the most recently emitted burst, and the packet counter it came + * from. The next burst is chained onto that end, with the counter gap + * reinstating the exact duration of any lost datagrams. */ + double lastEmittedEnd = 0.0; + uint32_t lastCounter = 0u; bool lastEmittedValid = false; }; diff --git a/Client/udpscope/tests/FrameDecoderTest.cpp b/Client/udpscope/tests/FrameDecoderTest.cpp index b58e39d..63245ad 100644 --- a/Client/udpscope/tests/FrameDecoderTest.cpp +++ b/Client/udpscope/tests/FrameDecoderTest.cpp @@ -19,13 +19,17 @@ struct FrameBuilder { storage.push_back(std::move(vals)); } - const FrameView& build(uint64_t hrt, double recvTime, uint32_t numSamples = 1) { + /* Real frames carry a per-update counter; leaving it at zero would hide + * whichever rules depend on it, so it must be passed explicitly. */ + const FrameView& build(uint64_t hrt, double recvTime, uint32_t numSamples = 1, + uint32_t counter = 0) { ptrs.clear(); counts.clear(); for (const auto& s : storage) { ptrs.push_back(s.data()); counts.push_back(static_cast(s.size())); } + view.counter = counter; view.hrt = hrt; view.recvTime = recvTime; view.numSamples = numSamples; @@ -161,7 +165,7 @@ TEST(FrameDecoder, AccumulatedScalarSurvivesBurstyDelivery) { const double arrival = (p < 20) ? (500.0 + p * 0.010) : (500.2 + (p - 20) * 0.00005); const FrameView& f = fb.build(static_cast(producerSec * ticks), - arrival, 10); + arrival, 10, static_cast(p + 1)); dec.beginFrame(f); std::vector ts; if (dec.timestamps(f, 0, ts)) { @@ -177,45 +181,100 @@ TEST(FrameDecoder, AccumulatedScalarSurvivesBurstyDelivery) { } } -// The counterweight to the test above. Suppressing arrival jitter by chaining -// each burst onto the previous one is only safe while the chain is checked: on -// UDP, packets are lost, and a chain that ignores arrival entirely closes the -// hole silently and dates every later sample a full second early — for the rest -// of the run, because nothing ever pulls it back. The prediction has to be -// abandoned once arrival contradicts it by more than a delivery backlog could. -TEST(FrameDecoder, AccumulatedScalarResynchronisesAfterLostPackets) { - FrameDecoder dec; +namespace { + +/** Ten contiguous 10-sample bursts at 1 kHz, counters 1..10, ending at 500.090. */ +SignalMeta accSignal() { SignalMeta m; m.name = "Acc"; m.typeCode = 9; m.numRows = 1; m.samplingRate = 1000.0; /* 10 samples = 10 ms per packet */ - dec.setSignals({m}); + return m; +} - std::vector ts; +void primeTenBursts(FrameDecoder& dec, std::vector& ts, bool withCounter) { for (int p = 0; p < 10; p++) { FrameBuilder fb; fb.addSignal(std::vector(10, 1.0)); - const FrameView& f = fb.build(0, 500.0 + p * 0.010, 10); + const FrameView& f = fb.build(0, 500.0 + p * 0.010, 10, + withCounter ? static_cast(p + 1) : 0u); dec.beginFrame(f); ASSERT_TRUE(dec.timestamps(f, 0, ts)); } - /* Contiguous so far: burst 9 ends at 500.090. */ - EXPECT_NEAR(ts[9], 500.090, 1e-9); + ASSERT_NEAR(ts[9], 500.090, 1e-9); +} - /* A full second of packets never arrives. The next one lands at 501.100. */ +} /* namespace */ + +// The counterweight to the test above. Chaining bursts to suppress arrival +// jitter is only safe if loss is accounted for: a bare chain closes the hole a +// dropped datagram left and dates every later sample early for the rest of the +// run. The wire says exactly how much is missing, so no estimate is needed — +// and this test deliberately makes arrival time a LIAR (200 ms off) to prove +// the reconstruction comes from the counter and not from when the packet landed. +TEST(FrameDecoder, AccumulatedScalarReinstatesLostPacketsFromTheCounterGap) { + FrameDecoder dec; + dec.setSignals({accSignal()}); + std::vector ts; + primeTenBursts(dec, ts, /*withCounter=*/true); + + /* Counter 111 after 10: 100 packets lost, 1000 samples, exactly 1 s. The + * packet lands 200 ms later than that truth would predict. */ FrameBuilder fb; fb.addSignal(std::vector(10, 1.0)); - const FrameView& f = fb.build(0, 501.100, 10); + const FrameView& f = fb.build(0, 501.300, 10, 111u); dec.beginFrame(f); ASSERT_TRUE(dec.timestamps(f, 0, ts)); - /* Chaining blindly would put this burst at 500.091..500.100, overlapping - * the gap as though no data were missing. */ + /* Chaining blindly gives 500.091; anchoring on arrival gives 501.291. */ EXPECT_NEAR(ts[0], 501.091, 1e-9); EXPECT_NEAR(ts[9], 501.100, 1e-9); } +// A producer that never advances the counter, or restarts it, leaves nothing to +// reconstruct from. Arrival time is then the better of two bad answers, and the +// chain has to be abandoned rather than left to drift forever. +TEST(FrameDecoder, AccumulatedScalarResyncsOnArrivalWhenTheCounterSaysNothing) { + FrameDecoder dec; + dec.setSignals({accSignal()}); + std::vector ts; + primeTenBursts(dec, ts, /*withCounter=*/false); + + FrameBuilder fb; + fb.addSignal(std::vector(10, 1.0)); + const FrameView& f = fb.build(0, 501.100, 10, 0u); + dec.beginFrame(f); + ASSERT_TRUE(dec.timestamps(f, 0, ts)); + + EXPECT_NEAR(ts[0], 501.091, 1e-9); + EXPECT_NEAR(ts[9], 501.100, 1e-9); +} + +// Re-anchoring must never move a signal's timestamps backwards: the ring, the +// trigger and the exporter all assume they increase, and a backward step is +// indistinguishable from corruption downstream. Here the counter claims a +// 20 s hole while the packet arrives 10 ms after the last one, so the +// prediction and arrival disagree wildly and arrival points into the past. +TEST(FrameDecoder, AccumulatedScalarNeverStepsBackwardsWhenResyncing) { + FrameDecoder dec; + dec.setSignals({accSignal()}); + std::vector ts; + primeTenBursts(dec, ts, /*withCounter=*/true); + const double prevEnd = ts[9]; + + FrameBuilder fb; + fb.addSignal(std::vector(10, 1.0)); + const FrameView& f = fb.build(0, 500.000, 10, 2010u); + dec.beginFrame(f); + ASSERT_TRUE(dec.timestamps(f, 0, ts)); + + EXPECT_GT(ts[0], prevEnd) << "resync stepped backwards over the previous burst"; + for (size_t i = 1; i < ts.size(); i++) { + EXPECT_GT(ts[i], ts[i - 1]); + } +} + TEST(FrameDecoder, AccumulatedScalarDerivesDtFromTheHrtGapWhenNoRateIsDeclared) { FrameDecoder dec; SignalMeta m; @@ -230,15 +289,23 @@ TEST(FrameDecoder, AccumulatedScalarDerivesDtFromTheHrtGapWhenNoRateIsDeclared) FrameBuilder fb; fb.addSignal(std::vector(10, 1.0)); const double producerSec = 100.0 + p * 0.010; /* 10 ms per packet */ + /* Zero-mean arrival jitter, so the rate fit still converges but any + * single arrival GAP is wrong. Without it, uniform arrivals make + * packetBurst and the hrt path return the same number and the test + * cannot tell which branch produced it. The last packet's gap is + * 7 ms, which arrival-spanning would render as a 0.7 ms period. */ + const double jitter[4] = {0.0, 0.003, 0.0, -0.003}; const FrameView& f = fb.build(static_cast(producerSec * ticks), - 700.0 + p * 0.010, 10); + 700.0 + p * 0.010 + jitter[p % 4], 10, + static_cast(p + 1)); dec.beginFrame(f); std::vector ts; if (dec.timestamps(f, 0, ts)) { last = ts; } } ASSERT_EQ(last.size(), 10u); - /* 10 ms of producer time across 10 samples is a 1 ms period. */ + /* 10 ms of producer time across 10 samples is a 1 ms period, whatever the + * datagrams did on the way over. */ EXPECT_NEAR(last[1] - last[0], 0.001, 1e-5); } diff --git a/docs/superpowers/plans/2026-08-27-udpscope.md b/docs/superpowers/plans/2026-08-27-udpscope.md index 11c4296..849fa6f 100644 --- a/docs/superpowers/plans/2026-08-27-udpscope.md +++ b/docs/superpowers/plans/2026-08-27-udpscope.md @@ -1609,38 +1609,31 @@ TEST(FrameDecoder, AccumulatedScalarSurvivesBurstyDelivery) { } } -// ADDED in Task 4 review. The counterweight to the test above: suppressing -// arrival jitter by chaining bursts is only safe while the chain is CHECKED. On -// UDP packets are lost, and an unchecked chain closes the hole silently and -// dates every later sample early for the rest of the run. -TEST(FrameDecoder, AccumulatedScalarResynchronisesAfterLostPackets) { +// ADDED in Task 4 review, together with two siblings. See the shipped +// tests/FrameDecoderTest.cpp for the full set — accSignal()/primeTenBursts() +// helpers plus: +// * AccumulatedScalarReinstatesLostPacketsFromTheCounterGap — counter 10 → +// 111 means 100 lost packets = exactly 1 s; the packet deliberately lands +// 200 ms off that truth so the test fails if the answer comes from arrival. +// * AccumulatedScalarResyncsOnArrivalWhenTheCounterSaysNothing — counter +// stuck at 0, so only the arrival backstop can recover. +// * AccumulatedScalarNeverStepsBackwardsWhenResyncing — a resync that would +// move a signal's timestamps into the past must be given up instead. +// FrameBuilder::build() gained a `counter` parameter for these; leaving it at +// zero, as the original harness did, hides the counter rules entirely. +TEST(FrameDecoder, AccumulatedScalarReinstatesLostPacketsFromTheCounterGap) { FrameDecoder dec; - SignalMeta m; - m.name = "Acc"; - m.typeCode = 9; - m.numRows = 1; - m.samplingRate = 1000.0; /* 10 samples = 10 ms per packet */ - dec.setSignals({m}); - + dec.setSignals({accSignal()}); std::vector ts; - for (int p = 0; p < 10; p++) { - FrameBuilder fb; - fb.addSignal(std::vector(10, 1.0)); - const FrameView& f = fb.build(0, 500.0 + p * 0.010, 10); - dec.beginFrame(f); - ASSERT_TRUE(dec.timestamps(f, 0, ts)); - } - EXPECT_NEAR(ts[9], 500.090, 1e-9); + primeTenBursts(dec, ts, /*withCounter=*/true); - /* A full second of packets never arrives. The next one lands at 501.100. */ FrameBuilder fb; fb.addSignal(std::vector(10, 1.0)); - const FrameView& f = fb.build(0, 501.100, 10); + const FrameView& f = fb.build(0, 501.300, 10, 111u); dec.beginFrame(f); ASSERT_TRUE(dec.timestamps(f, 0, ts)); - /* Chaining blindly would put this burst at 500.091..500.100, as though no - * data were missing. */ + /* Chaining blindly gives 500.091; anchoring on arrival gives 501.291. */ EXPECT_NEAR(ts[0], 501.091, 1e-9); EXPECT_NEAR(ts[9], 501.100, 1e-9); } @@ -1930,12 +1923,16 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx, * fallback is waiting on. * * With a declared rate none of that is needed: the intra-packet step is - * exact, and bursts are contiguous, so the next burst is PREDICTED at - * lastEmittedEnd + dt. The prediction must be checked, not trusted — a pure - * chain silently closes the hole left by a lost datagram and dates every - * later sample early for the rest of the run. So each packet compares the - * prediction against the arrival anchor and abandons it beyond - * kBurstResyncThresholdS. The hrt path below remains for samplingRate == 0. */ + * exact and bursts are contiguous, so the next burst chains onto the end of + * the previous one. The one thing a bare chain gets wrong is LOSS — it + * closes the hole a dropped datagram left, dating every later sample early + * for the rest of the run — and the wire already says exactly how much is + * missing: FrameView::counter increments once per update, so a gap of g + * means g-1 lost packets. Reinstating that duration needs no estimate and + * no threshold. The arrival-anchor comparison is only a BACKSTOP for what + * the counter cannot express (producer restart, counter stuck at zero, a + * declared rate that is simply wrong), and it must never move time + * backwards. The hrt path below remains for samplingRate == 0. */ if (d.numElements() == 1u && nElems > 1u) { const double dtDeclared = (d.samplingRate > 0.0) ? (1.0 / d.samplingRate) : 0.0; if (d.samplingRate > 0.0) { @@ -1943,16 +1940,28 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx, wallNow - static_cast(nElems - 1u) * dtDeclared; double base = arrivalAnchor; if (st.lastEmittedValid) { - const double predicted = st.lastEmittedEnd + dtDeclared; + /* Unsigned subtraction wraps, so this is right across the + * counter's own 2^32 rollover. */ + const uint32_t gap = f.counter - st.lastCounter; + const double lost = (gap > 1u) + ? static_cast(gap - 1u) * + static_cast(st.prevAccCount) + : 0.0; + const double predicted = st.lastEmittedEnd + dtDeclared * (1.0 + lost); if (std::fabs(predicted - arrivalAnchor) <= kBurstResyncThresholdS) { base = predicted; } + if (base <= st.lastEmittedEnd) { + base = st.lastEmittedEnd + dtDeclared; + } } tsOut.resize(nElems); for (uint32_t e = 0; e < nElems; e++) { tsOut[e] = base + static_cast(e) * dtDeclared; } st.lastEmittedEnd = tsOut[nElems - 1u]; + st.lastCounter = f.counter; + st.prevAccCount = nElems; st.lastEmittedValid = true; return true; } @@ -2015,7 +2024,9 @@ cd Client/udpscope && cmake --build build -j && ./build/udpscope_tests --gtest_f Expected: PASS, 9 tests. -If `AccumulatedScalarSurvivesBurstyDelivery` fails, do NOT reach for the hrt fit: with a declared `samplingRate` rule 3 never consults it, precisely because the fit is not ready for the first 32 packets and is itself corrupted by bursty arrivals. Check instead that `lastEmittedEnd`/`lastEmittedValid` are being updated on every emitted burst. The only test that may legitimately fall through to rule 4 early is `AccumulatedScalarDerivesDtFromTheHrtGapWhenNoRateIsDeclared`, whose arrivals are uniform, so `packetBurst` is accurate there. +If `AccumulatedScalarSurvivesBurstyDelivery` fails, do NOT reach for the hrt fit: with a declared `samplingRate` rule 3 never consults it, precisely because the fit is not ready for the first 32 packets and — since `HrtRateFit` regresses `hrt` against ARRIVAL time — is itself corrupted by the very bursts it would be asked to survive. Check instead that `lastEmittedEnd`, `lastCounter`, `prevAccCount` and `lastEmittedValid` are updated on every emitted burst. + +Note for `AccumulatedScalarDerivesDtFromTheHrtGapWhenNoRateIsDeclared`: its arrivals carry zero-mean jitter on purpose. Under UNIFORM arrivals the hrt path and `packetBurst` return the same number by construction (the fit expresses `hrt` in arrival-clock seconds), so the test could not tell which branch answered. - [ ] **Step 8: Commit**