fix(udpscope): reconstruct lost accumulated bursts from the packet counter

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 <noreply@anthropic.com>

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
+ * How far a chained burst prediction may sit from where arrival time says it
- * where arrival time says it should be before the chain is abandoned.
+ * 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
+ * This is a backstop, not the primary mechanism: the packet counter normally
- * prediction a few hundred milliseconds ahead of arrival, so the threshold has
+ * accounts for lost datagrams exactly, so the prediction and arrival agree.
- * to be well clear of that. Anything larger is not delivery jitter: it is lost
+ * It catches what the counter cannot describe — a producer restart (the
- * packets or a declared sampling rate that does not match the producer's real
+ * counter returns to zero), a counter that never advances, and a declared
- * one, and both must resynchronise rather than accumulate forever. Same value
+ * sampling rate that does not match the producer's real one. A kernel draining
- * and same reasoning as ClockOffset::kRecalibThresholdS.
+ * 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<double>& 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<double>(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
+             * apart still yields contiguous timestamps. What a bare chain gets
-             * blind: one lost datagram, or a declared rate that does not match
+             * wrong is loss: it closes the hole a dropped datagram left, and
-             * the producer's real one, displaces every later sample and never
+             * every later sample is then dated early for the rest of the run.
-             * recovers. So the chain is a PREDICTION, checked each packet
+             *
-             * against arrival and abandoned when the two disagree by more than
+             * The wire says exactly how much is missing. counter increments
-             * a delivery backlog can explain. That bounds the error instead of
+             * once per update, so a gap of g means g-1 lost packets, each
-             * letting it accumulate. */
+             * 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<double>(gap - 1u) *
+                      static_cast<double>(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<double>(e) * dt;
             }
             st.lastEmittedEnd   = tsOut[nElems - 1u];
+            st.lastCounter      = f.counter;
+            st.prevAccCount     = nElems;
             st.lastEmittedValid = true;
             return true;
         }

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
+         *  of the most recently emitted burst, and the packet counter it came
-         *  start one sample period after it — immune to arrival-time jitter —
+         *  from. The next burst is chained onto that end, with the counter gap
-         *  but the prediction is discarded when arrival time disagrees with it
+         *  reinstating the exact duration of any lost datagrams. */
-         *  by more than a delivery backlog can explain, so packet loss cannot
+        double      lastEmittedEnd   = 0.0;
-         *  displace the trace permanently. */
+        uint32_t    lastCounter      = 0u;
-        double      lastEmittedEnd  = 0.0;
         bool        lastEmittedValid = false;
     };
 

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<uint32_t>(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<uint64_t>(producerSec * ticks),
-                                      arrival, 10);
+                                      arrival, 10, static_cast<uint32_t>(p + 1));
         dec.beginFrame(f);
         std::vector<double> 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
+namespace {
-// 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
+/** Ten contiguous 10-sample bursts at 1 kHz, counters 1..10, ending at 500.090. */
-// hole silently and dates every later sample a full second early — for the rest
+SignalMeta accSignal() {
-// 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;
     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<double> ts;
+void primeTenBursts(FrameDecoder& dec, std::vector<double>& ts, bool withCounter) {
     for (int p = 0; p < 10; p++) {
         FrameBuilder fb;
         fb.addSignal(std::vector<double>(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<uint32_t>(p + 1) : 0u);
         dec.beginFrame(f);
         ASSERT_TRUE(dec.timestamps(f, 0, ts));
     }
-    /* Contiguous so far: burst 9 ends at 500.090. */
+    ASSERT_NEAR(ts[9], 500.090, 1e-9);
-    EXPECT_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<double> 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<double>(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
+    /* Chaining blindly gives 500.091; anchoring on arrival gives 501.291. */
-     * the gap as though no data were missing. */
     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<double> ts;
+    primeTenBursts(dec, ts, /*withCounter=*/false);
+
+    FrameBuilder fb;
+    fb.addSignal(std::vector<double>(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<double> ts;
+    primeTenBursts(dec, ts, /*withCounter=*/true);
+    const double prevEnd = ts[9];
+
+    FrameBuilder fb;
+    fb.addSignal(std::vector<double>(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<double>(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<uint64_t>(producerSec * ticks),
-                                      700.0 + p * 0.010, 10);
+                                      700.0 + p * 0.010 + jitter[p % 4], 10,
+                                      static_cast<uint32_t>(p + 1));
         dec.beginFrame(f);
         std::vector<double> 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);
 }
 

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
+// ADDED in Task 4 review, together with two siblings. See the shipped
-// arrival jitter by chaining bursts is only safe while the chain is CHECKED. On
+// tests/FrameDecoderTest.cpp for the full set — accSignal()/primeTenBursts()
-// UDP packets are lost, and an unchecked chain closes the hole silently and
+// helpers plus:
-// dates every later sample early for the rest of the run.
+//   * AccumulatedScalarReinstatesLostPacketsFromTheCounterGap — counter 10 →
-TEST(FrameDecoder, AccumulatedScalarResynchronisesAfterLostPackets) {
+//     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;
+    dec.setSignals({accSignal()});
-    m.name         = "Acc";
-    m.typeCode     = 9;
-    m.numRows      = 1;
-    m.samplingRate = 1000.0;          /* 10 samples = 10 ms per packet */
-    dec.setSignals({m});
-
     std::vector<double> ts;
-    for (int p = 0; p < 10; p++) {
+    primeTenBursts(dec, ts, /*withCounter=*/true);
-        FrameBuilder fb;
-        fb.addSignal(std::vector<double>(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);
 
-    /* A full second of packets never arrives. The next one lands at 501.100. */
     FrameBuilder fb;
     fb.addSignal(std::vector<double>(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
+    /* Chaining blindly gives 500.091; anchoring on arrival gives 501.291. */
-     * data were missing. */
     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
+     * exact and bursts are contiguous, so the next burst chains onto the end of
-     * lastEmittedEnd + dt. The prediction must be checked, not trusted — a pure
+     * the previous one. The one thing a bare chain gets wrong is LOSS — it
-     * chain silently closes the hole left by a lost datagram and dates every
+     * closes the hole a dropped datagram left, dating every later sample early
-     * later sample early for the rest of the run. So each packet compares the
+     * for the rest of the run — and the wire already says exactly how much is
-     * prediction against the arrival anchor and abandons it beyond
+     * missing: FrameView::counter increments once per update, so a gap of g
-     * kBurstResyncThresholdS. The hrt path below remains for samplingRate == 0. */
+     * 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<double>(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<double>(gap - 1u) *
+                      static_cast<double>(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<double>(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**