docs: a design for resequencing late packets into a reserved hole, and for relative timing

The two were asked for together and are one design: the packet counter is what makes a hole addressable, so relative timing is the coordinate system in which resequencing is arithmetic rather than inference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-10-08 16:38:18 +02:00
1 parent aa8ec1794a
commit d5d1295b26
@@ -0,0 +1,319 @@
 <!-- SPDX-License-Identifier: MIT -->
 <!-- SPDX-FileCopyrightText: 2026 Martino Ferrari -->
 # A late packet leaves a hole it could have filled, and the clock it would be filed under is the wrong one
 Two requests, and they are one design.
 The first is that a reordered packet currently reads as permanent damage. `udps_client.c:504-514`
 orders the stream by the wrapping `uint32` counter and discards anything that arrives behind the
 newest counter — `stats.stale_packets++; return 0` — *before* spending anything on the payload.
 The samples are gone, the span they should have filled stays empty, and udpscope never learns
 there was a span at all. The asked-for remedy is to leave a hole of
 `<N samples> × <LAST_PACKET_ID − NEW_PACKET_ID − 1>`, mark it invalid, and fill it when the
 missing packets arrive.
 The second is `todo.md` item 2: a relative-time mode in which the plot's x comes only from the
 declared sample rate and the packet number, with absolute stream time kept for export.
 They are one design because **the counter is what makes a hole addressable**. A reserved hole has
 to be a known number of samples at a known position, and a late packet has to compute which
 position it belongs to. Absolute stamps cannot do either — a lost packet's stamps were never
 received, so its extent in time is a guess, and the guess is what the glitch bridge at
 `src/acquisition.cpp:274-292` is already making. The counter gives the extent exactly. Relative
 timing is not a cosmetic preference that happens to be requested at the same time; it is the
 coordinate system in which resequencing is arithmetic rather than inference.
 Two facts from the existing code shape almost every decision below, and both say the work is
 smaller than it looks:
 - **`frame->counter` and `frame->lost` are read nowhere in udpscope.** `frame.lost` is set by the
  client at `udps_client.c:560` and appears in `src/acquisition.cpp` only inside comments
  (`:309`, `:331`, `:336`). The client has been reporting the exact gap width for the life of the
  project and nothing has ever consumed it. Neither input needs inventing.
 - **`clamp_deadline()` already exists** (`udps_client.c:1102`), documented as *"Clamps `budget` so
  a pending deadline is not slept through"* and already used for the keepalive and the silence
  timeout. The reorder deadline is a third call to it.
 ## What was decided, and the constraint that decided it
 The latency budget is **20 ms**, maximum. That one number settles the shape of the whole receive
 path, because it is a *time* budget and not a packet count: at 1000 pkt/s it is twenty packet
 spans of reorder depth, at 10 pkt/s it is a fifth of one. A hold policy counted in packets would
 either be useless at high rates or add 400 ms at low ones. A policy counted in wall time works at
 every rate, because network reorder is measured in milliseconds regardless of how fast the
 producer sends.
 It also means a *contiguous* arrival must be released immediately. The 20 ms is spent only when
 there is a gap to wait on, never on the common case.
 Past the window, a committed hole is still **backfilled for values only**. Math channels and the
 trigger are not replayed. A math channel takes NaN in and gives NaN out, so after a repair the
 raw trace is whole and the math trace derived from it keeps its gap. That is a deliberate,
 documented state, not an oversight: the display then says *"this region arrived too late to be
 part of the computation"*, which is true and is cheaper than any mechanism that could make it
 false.
 Resequencing and hole reservation run in **both** time modes. In relative mode a hole's stamps
 are exact; in stream mode they are the declared cadence continued from the last good stamp, which
 is the existing bridge mechanism reused. Relative timing is therefore an independent toggle a user
 flips for jitter, not a prerequisite for recovery.
 A hole is surfaced two ways: **status-bar counters** and a **shaded band** over the invalid span.
 A NaN run already breaks the polyline, but a one-packet hole at a 10 s window is sub-pixel, and a
 gap nobody can see is not a report.
 ## Relative timing
 `build_stamps()` today picks its clock *by inspection*: `time_source_` latches 1 for the declared
 time signal or 2 for the arrival clock and never flips back (`src/acquisition.cpp:357-367`).
 Relative timing is not a third thing to infer. It is a user-selected mode that bypasses the
 inference: `--relative-time` on the command line, a toggle in the UI, persisted in the session
 file.
 ### The stamp
 ```
 stamps_[k] = double(slot_base_ + k) / rate
 ```
 `slot_base_` is a running `u64` of samples seen. Each packet the counter reports missing advances
 it by `batch`, **before** the frame's own samples are stamped; the delivered frame then advances it
 by its own `n`. The order is what puts the hole ahead of the data that revealed it.
 This is the requested `counter × batch + sample_index` with the origin left at counter zero, but
 **accumulated rather than multiplied**, and the difference earns three things:
 - `batch` is then only ever needed to size what is *absent*. That is exactly the
  `<n_samples_per_frame>` of the original formula, and it is the only place the value is load
  bearing.
 - The `uint32` wrap costs nothing. The counter is consumed only as a signed delta — the same
  arithmetic `udps_client.c:505` already performs — so no unwrapping bookkeeping is needed.
 - A frame whose `n` disagrees with the established `batch` cannot silently shift the whole
  timeline, because the timeline is built from the `n` values actually seen.
 `batch` is established from the first DATA frame and held for the life of the stream. A later
 frame disagreeing is a genuine inconsistency in the producer, not a loss, so it warns once rather
 than re-deriving.
 ### The rate is the declared one, and only the declared one
 `build_stamps()` currently overrides a declared cadence with the arrival-measured one when the two
 disagree by more than ±25% (`src/acquisition.cpp:152-158`), because a CONFIG declaring 1 MS/s
 while delivering a hundred samples every fifth of a second reads as a staircase in the x column.
 Relative mode must **not** do this. Discarding arrival timing is the entire point of the mode. The
 override stays live in stream mode and is skipped in relative mode.
 ### The constant offset is already absorbed
 `src/acquisition.cpp:555` stamps `rel = stamps_[k] - epoch`, and the `t0 =` caption names the
 epoch. A counter that happens to read 500 000 on connect therefore shows up as an epoch value, not
 as a 50 000 s axis. No origin subtraction is needed in the formula itself.
 ### The x axis needs no change at all
 `RowSet::update_time_labels()` already goes to a relative label form on the tick step against the
 label resolution at the axis' magnitude, with the `t0 =` caption naming the absolute anchor
 (`docs/claude/invariants-time-axis-and-window.md:34`). "Counting backwards, like it is now" is
 what that already does. Relative mode changes *where a stamp comes from* and nothing in the label
 layer.
 ### What relative mode makes unreachable
 The degeneracy guard, the implausible-jump test, `kFailureLimit`, the bridge and the monotonic
 clamp are all dead in relative mode: the stamps are monotone by construction, so there is no bad
 stamp to detect and nothing to clamp. (`kGlitchPackets` itself survives — not as the threshold of
 that test, which is unreachable, but as the reservation cap below, where its value and its
 reasoning carry over unchanged.)
 That is not tidiness, it is a fix. Measured last session: a loss burst past `kGlitchPackets` (256)
 is misread as a producer clock glitch; each bridge then advances the anchor by *one* packet span
 while the true timeline is 300 ahead, so the next four packets fail too, `kFailureLimit` trips,
 and the whole stream is thrown onto the arrival clock — the exact failure the bridge was added to
 prevent (`--drop-burst 300` reads
 `jumped implausibly far ahead ... (1.299..1.29999 against a previous 0.99899)` then
 `timing samples by arrival instead`). In relative mode a 300-packet gap is a known quantity and
 cannot be misdiagnosed.
 ### Absolute time for export
 `todo.md` requires absolute stream time to remain available for export. There is exactly one f64
 time ring per `BufferStore`, shared by every channel of that buffer, and relative mode takes it.
 Export therefore needs a **second** f64 ring carrying the absolute stamp under the same group-mean
 rule, written in relative mode only and read by export and by nothing else — eight bytes per
 stored slot.
 This cost is paid rather than avoided. Without it, selecting relative mode silently degrades the
 one output `todo.md` says must keep absolute time.
 ## The receive path
 ### In the client: a reorder ring of reassembled payloads
 Every `udps_frame_t` sets `frame.signals = c->sigs; frame.values = c->vals`
 (`udps_client.c:566-567`) — one client-owned decode buffer, reused for every packet. A frame is
 therefore valid only inside the callback, and holding one requires a copy *wherever* the holding
 happens. The client has no advantage there.
 It has a different one: it still holds the **raw datagram**. Resequencing undecoded wire bytes is
 strictly cheaper than resequencing decoded doubles — an `f32` or `i16` payload is two to four
 times smaller than what `c->valbuf` would hold — and the bytes exist nowhere else. That is why
 this sits in the client and not in `Acquisition`.
 The hook is the `decode_data()` call site at `udps_client.c:599`: after fragment reassembly, which
 already tolerates arrival order through the slot table, and before decode. Each held entry carries
 the payload bytes, their length, the counter and `recv_time`.
 The policy, against `next_counter = last_delivered + 1`:
 | arrival | action | added latency |
 | --- | --- | --- |
 | `== next_counter` | decode and deliver now, then drain held entries that became contiguous | **none** |
 | `> next_counter` | copy into the ring; set the gap deadline to `now + 20 ms` if not already set | the newer packet waits |
 | `< next_counter` | the gap is already committed — decode and deliver, flagged **late** | n/a |
 | deadline fires | give up on the missing counters, advance `next_counter` to the lowest held entry, drain in order | capped at 20 ms |
 The last row is where `stats.stale_packets++; return 0` at `:506-509` is replaced: a late packet
 is delivered flagged instead of discarded, which is the single change that makes backfill possible
 at all.
 The deadline is served by a third `clamp_deadline()` call on the `select()` budget
 (`udps_client.c:1145-1152`). Without it, a held packet on a quiet stream would wait for the next
 arrival — 100 ms at 10 pkt/s — and blow the budget.
 Two bounds on the ring, because 20 ms is a different number of packets at every rate: a fixed
 entry count and a fixed byte budget. Overflow commits the gap early rather than growing, and
 increments its own counter, so an undersized window is visible rather than silent.
 The window itself is `udps_config_t::reorder_window_s`, defaulting to `0.020`, along
 `recv_buffer_bytes` and `silence_timeout_s` (`udps_client.c:982`). Zero disables resequencing and
 restores today's discard-on-late behaviour exactly, which is what makes the change bisectable
 against a reported regression. The policy value is udpscope's; the mechanism is the client's.
 ### In `Acquisition`: reservation
 On a frame with `lost > 0`, before pushing the frame's own samples, run `lost × batch` NaN samples
 through the **existing** per-sample loop at `src/acquisition.cpp:553`.
 That reuse is the design. `tick_` advances, so `tick % g == g - 1` keeps firing on the same phase
 and the decimation grid does not shift; the group mean still lands on the mean stamp; and
 `MinMaxPyramid::append` sees NaN, which its existing non-finite contract already handles as
 *advances the positional count, excluded from every aggregate*. Nothing bespoke is needed for
 decimation, stamping or the pyramid.
 Stamps across the hole come from the active time source — exact in relative mode, the declared
 cadence continued from the last good stamp in stream mode, which is the bridge at `:274-292`.
 Math channels take NaN and give NaN, so a math trace keeps its gap. The trigger engine's
 comparisons are false on NaN, so it cannot latch inside a hole; this is asserted by the probe
 rather than assumed.
 **Reservation is capped at 256 missing packets**, reusing `kGlitchPackets` and its existing
 reasoning verbatim — *256 packet spans in one go is an outage, not loss*. Past the cap there is no
 reservation: log once and continue as today. That cap is also what stops a ten-minute outage from
 pushing billions of NaN samples through the ring and wiping all history to make room for a gap.
 ## Backfill
 `plot::data::RingBuffer` and `MinMaxPyramid` are both strictly append-only today. Backfill needs
 an amend in each, and the two belong on different threads.
 `plot_host.hpp:183` calls `acquire(ScopeModel&)` from the frame path, so the **pyramid is mutated
 only on the GTK main thread**, once per frame: the producer pushes into the ring and the main
 thread folds what was pushed into the pyramid. That split carries straight over to the amend.
 - **Ring amend — producer thread.** `RingBuffer<T>::amend(absolute, v)`, bounds-checked against
  `[produced_ - capacity, produced_)`, a single relaxed store. The same race class as a push, and
  an `f32` cannot tear. Measurements and export read the ring directly, so they see a repair
  immediately.
 - **Pyramid amend — main thread, inside `acquire()`.** The producer enqueues `(slot, value)` on an
  SPSC queue; `ChannelBuffer::acquire()` drains it **after** its append pass, so every amended
  index is already published.
 The thread split is not a stylistic choice. A bucket is 32 bytes and is not atomic, so amending it
 from the producer would let a reader see a half-updated bucket whose `sum` does not match its
 `count` — a wrong mean from a structure whose readers trust `count == 0` to mean *absent*. On the
 main thread that cannot happen and no seqlock is needed.
 `MinMaxPyramid::amend(index, v)` folds `v` into every covering bucket: at `count == 0`,
 `min = max = v`, `sum = v`, `sumsq = v²`, `count = 1`; otherwise widen `min`/`max` and accumulate
 `sum`/`sumsq`, `++count`. Its documented precondition is **non-finite → finite only**, which is
 what makes it purely additive and O(log n) with no rebuild. An index already dropped by
 `drop_before` is a no-op.
 ### The time ring is never amended
 The hole was pre-stamped at reservation, so a backfill changes values only. `neighbours()` keeps
 the monotonic array its binary search requires and `window_x0_` stays valid. This is the reason
 reservation pre-stamps rather than leaving the ring short.
 ### Addressing a late packet
 Reservation records `(counter_first, counter_last, tick_start, slot_start, slot_count)` in a
 bounded side table. A late frame's counter indexes straight into it:
 ```
 tick = tick_start + (counter - counter_first) * batch
 ```
 Only slots lying **wholly** inside the arrived packet's tick range are amended. A slot straddling
 a hole edge went NaN when its accumulator took a NaN, and its real members were never retained, so
 it stays NaN. A displayed hole is therefore at most one slot wider per edge than the true loss —
 invisible at any hole size worth repairing, and it makes a boundary slot need no special case.
 The side table is bounded by the 256-packet reservation cap and a maximum entry count; it is also
 the source the shaded band reads.
 ## Surfacing
 `StreamStats` gains `late_resequenced`, `late_committed` (gave up past 20 ms, or overflowed the
 reorder ring) and `late_backfilled`, and finally exposes `stale_packets` — which the client has
 counted since the beginning and which `src/status_text.cpp:70` has never shown, so an inversion
 today has to be inferred from `packets` and `frames` diverging. All four sit in the status bar's
 second tier.
 The band is a new `Plot::set_invalid_spans()` in `deps/plotter`, drawn as a tinted quad behind the
 trace. Unlike the `t0` caption this *is* data-anchored — it must pan and zoom with the data it
 marks — so it is annotation-class, not chrome. The invariant it must respect is that it reserves
 no layout space and cannot be picked, so a hole cannot move the layout the labels were measured
 into.
 ## Verification
 udpscope has no unit test suite; the only automated check is the application's own headless
 self-check (`CLAUDE.md`). This adds exit-dump lines and a `UDPSCOPE_PROBE_RESEQ_MS` probe,
 asserting: a reorder inside the window is resequenced with no hole; a reorder past the window
 leaves a hole and then backfills it; the trigger does not latch inside a hole; a 300-packet burst
 hits the cap and does not throw the stream onto the arrival clock; and relative mode's stamps are
 monotone with the declared spacing.
 Runs are serial and short, against the two standing baselines: four build warnings on a clean
 build (two on an incremental build) and zero `Gtk-CRITICAL`. Batching several `xvfb` runs back to
 back starves the mock past its 2 s `silence_timeout_s` and reads as a reconnect, which is a
 measurement artefact and not a defect (`docs/claude/verification.md`).
 **One gap in the mock has to be closed first.** `--reorder-every` releases the held packet
 immediately after the one that overtook it, so it is ~0 ms late and would always land inside the
 20 ms window. That exercises resequencing but leaves the **backfill path untestable**. The mock
 needs `--reorder-delay-ms` so a packet can be released deliberately past the window.
 ## Documentation this lands against
 - `docs/claude/invariants-data-path.md` — append-only stops being the whole truth; the amend's
  precondition and its thread belong here.
 - `docs/claude/invariants-plot-and-layout.md` — the band, and the layout space it must not take.
 - `docs/claude/invariants-trigger-and-pinning.md` — the trigger does not latch inside a hole.
 - `docs/claude/verification.md` — the new probe and dump lines, and `--reorder-delay-ms`.
 - `docs/claude/build-and-run.md` — the vendored `udpsclient` diff: the reorder ring, the late
  delivery replacing the discard, and the third `clamp_deadline()`.
 - `README.md` and a `SPECS.md` deviation note for relative-time mode.
 ## Deliberately not in scope
 - **Replaying math channels or the trigger over a backfilled region.** Decided against above.
 - **Interpolating across an unrepaired hole.** A hole is reported, not invented. There is no
  interpolation anywhere in the data path today and this does not add the first.
 - **Making relative mode the default.** It is a toggle. The default stays the declared time
  signal, so no existing session or run changes what it shows.
 - **The `(min, max, mean, rms)` gather mode** that would fix the separate solid-band degeneration
  at wide time windows. Related to the pyramid, unrelated to this; still open.