Skip to content

DEV-1023: reject an isolated zero timestamp instead of reading it as a wrap - #147

Merged
marknolan merged 1 commit into
mainfrom
DEV-1023_reject_isolated_zero_timestamp
Sep 16, 2026
Merged

marknolan merged 1 commit into
mainfrom
DEV-1023_reject_isolated_zero_timestamp

Conversation

@marknolan

Copy link
Copy Markdown
Member

The fault

Firmware stamps a packet when the sample tick starts it and does not publish a packet
it never stamped, so a counter field of exactly 0x000000 means the record is
invalid — not that the counter reached its origin. LogAndStream v1.00.x–v1.01.003
could emit one under SD write back-pressure.

StreamTimeline read it as a roll-over. Because this SDK uses a reorder-window rule
rather than the naive "any backward step is a wrap", the damage arrived in two pieces
instead of one. With values taken from a real recording — predecessor 7406506, then
0, on the 24-bit counter:

step arithmetic effect
the zero forward = 9,370,710, backwards = 7,406,506 — backwards exceeds the reorder window (2²¹), so it reads as forward motion +285.97 s
the sample after it 7406571 read forward from a predecessor of 0 +226.03 s
16,777,281 ticks 512.002 s

The same net error as every other Shimmer host API, just split across two steps. The
host-clock missed-wrap branch does not engage — elapsed time between the two packets
is ~2 ms, far below half a modulo.

The fix

_unwrap returns null for a 24-bit counter reading exactly zero whose predecessor
was more than INVALID_ZERO_WINDOW_TICKS (32768 — one second) below the maximum.

stamp() then holds every piece of timeline state:

  • _lastRaw, so the next sample is compared against the last value the firmware
    actually stamped and reads as the ordinary step forward it is, rather than as a
    second wrap;
  • _lastHostMs, so the recovery for wraps that went by unseen still measures the
    whole gap rather than only the part after the bad packet;
  • _wraps;
  • the pending anchor — a packet with no timestamp must not become an anchor's origin.

The frame is still emitted, with ObjectCluster.timestampValid false, exactly the way
a CRC failure is: its sensor values are real, and dropping it silently would hide the
fault from anything counting it. Its TIMESTAMP fields repeat the previous frame's.
Consumers that need a true time axis should drop those frames.

The exemption is deliberately narrow:

  • a genuine wrap onto zero has a predecessor at the top of the range and is accepted;
  • the 16-bit counter older firmware uses, whose whole range is 2 s and which a stall
    really can cross, is untouched;
  • any other backward step is still a reorder or a wrap, as before.

Testing

12 new cases in tests/core/stream-timeline.test.ts. Full suite 1873 passed / 95
files
; typecheck, lint and prettier clean.

Both defences were mutation-checked rather than trusted green:

  • disabling the rejection fails 7 of the new cases;
  • letting a rejected packet advance _lastHostMs fails the missed-wrap-recovery case —
    which is why that test uses a 600 s gap with the bad packet landing late inside it,
    the only shape where holding and not holding diverge.

Scope

This is one of four parallel host implementations of the same wire format; the
identical rule is going into the Java driver, the C# API and pyshimmer, so a
recording reads the same whichever reads it. Cross-referenced in the source comments.

No version bump and no dist/ vendor sync here — that is a release step, not part of
the source fix.

DEV-1023.

🤖 Generated with Claude Code

…a wrap

Firmware stamps a packet when the sample tick starts it and does not publish
a packet it never stamped, so a counter field of exactly 0x000000 means the
record is invalid rather than that the counter reached its origin. LogAndStream
v1.00.x-v1.01.003 could emit one under SD write back-pressure.

StreamTimeline read it as a 24-bit roll-over and, uniquely among the host
implementations, split the damage across two samples: the zero advanced the
timeline by the distance to the top of the range and the sample after it
advanced by the rest, summing to exactly one modulo. Same net error as
everywhere else - 512.000 s added to every later sample in the session.

_unwrap now returns null for a 24-bit counter reading exactly zero whose
predecessor was more than a second below the maximum, and stamp() holds every
piece of timeline state where it was: _lastRaw so the next sample is compared
against the last value the firmware actually stamped, _lastHostMs so the
missed-wrap recovery still measures the whole gap, _wraps, and the pending
anchor, which must not bind to a packet with no timestamp. The frame is still
emitted, with ObjectCluster.timestampValid false, the way a CRC failure is:
its sensor values are real and dropping it silently would hide the fault.

The exemption is narrow. A genuine wrap onto zero has a predecessor at the top
of the range and is accepted; the 16-bit counter, whose whole range is 2 s, is
untouched; any other backward step is still a reorder or a wrap.

Mirrors TimestampUnwrap in the Java driver and the C# API, and
find_invalid_timestamps in pyshimmer. Five host implementations read the same
wire format and have to agree on it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@marknolan
marknolan merged commit f175662 into main Sep 16, 2026
2 checks passed
@marknolan

Copy link
Copy Markdown
Member Author

Adversarial review — no code changes needed, two notes for the reviewer

Went looking for defects rather than confirmation. Nothing here needs changing, but two
things are worth knowing before this is released.

1. StreamStamp.invalid is a breaking change to a public type

It is a required field added to an exported interface. Anything outside this repo
that constructs a StreamStamp — a mock, a fixture, a re-implementation — stops
compiling. Reading one is unaffected.

Checked both vendored consumers: neither verisense-device-console nor webBLEDemos
references StreamStamp or unwrappedTicks at all, so there is no practical impact.
Raising it only so the release version bump is a deliberate choice rather than an
accident. The package is pre-1.0, so a minor bump is defensible either way.

2. Two consecutive raw zeros now read as one good sample and one invalid

If a stream somehow delivers 0 twice in a row on the 24-bit counter, the second is
rejected (its predecessor, 0, is below the window). Previously it was a duplicate:
forward === 0, timeline unmoved.

Both outcomes leave the timeline in the same place, so this is a labelling difference
rather than a behavioural one, and "the firmware never stamped it" is the better reading
of two identical zeros. Noted for completeness, not as a concern.

Attacked and found sound

  • A rejection before the counter width is known. The default is 24 bits, and a
    legitimate 16-bit wrap onto zero would be rejected under it. Unreachable in practice:
    _prepareStreamTimeline() runs at the top of startStreaming() and both sets the
    width and resets the timeline before any sample arrives.
  • A rejection as the first sample. _lastRaw === null returns before the test, so
    the first sample of a stream is always accepted, zero or not.
  • Cascading. The held _lastRaw is the last value the firmware actually stamped, so
    the next sample is an ordinary forward step. Covered by a test.
  • A missed wrap spanning a rejection. This is the one that needed the _lastHostMs
    hold, and the test for it uses a 600 s gap with the bad packet landing late inside it
    — the only shape where holding and not holding diverge. Mutation-checked: advancing
    _lastHostMs on a rejected packet fails that case and nothing else.
  • Anchor binding. A pending anchor is not resolved against a packet with no
    timestamp; the next real sample takes it, and the anchor's origin is that sample's
    tick value.
  • _wraps regression. Held on a rejection, and still monotonic across one, because
    the running maximum is only updated on accepted samples.

Rejection was also mutation-checked as a whole: disabling it fails 7 of the 12 new
cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant