Conversation
The unwrap already told a reordered packet from a roll-over by how far backwards the counter stepped, but it sized that threshold as a fraction of the counter's range. The two quantities are unrelated: a reorder swaps packets that are adjacent in time, whereas a dropout that happens to span the wrap point is most of a modulo. At an eighth of the range the boundary between them lands in the middle of ordinary dropout territory - on the 16-bit counter every gap between 1.75 s and 2.0 s read as a reorder and the roll-over was silently lost. The window is now eight sample periods, derived from the rate the inquiry reports, which shrinks that misread band to about 16 ms. Both clients pass the rate at stream start; `timelineState.reorderWindowTicks` reports the window in force, which is what the two new client tests assert - the wiring is one line in each client and nothing else would notice it going missing. Also restated on the modular forward distance rather than on a comparison of unwrapped values. The two look equivalent and are not: a packet arriving late from just before a wrap boundary has a candidate ABOVE its predecessor, so a comparison accepts it as forward motion of nearly a whole modulo and then reads the next real sample as a second wrap. `[2^24 - 10, 5, 2^24 - 10, 70]` lands at 33554502, two modulos out, from one out-of-order packet. A rate the client does not know yet still falls back to an eighth of the modulo, as before, so a timeline nobody has told the rate to behaves exactly as it did. The other Shimmer host APIs disable the branch outright in that case; they import files, where there is no host clock to appeal to, and this SDK has the missed-wrap recovery as a second witness. One thing the fallback does not get: overruling the invalid-zero test. A reorder is judged before that test, which is right for a window of a few sample periods - a zero that close to an origin is genuinely ambiguous and the cost of choosing wrong is 16 ms - but at 64 s wide it would read an unstamped record as a packet 64 s late, place it 64 s early and call it valid, which is worse than either answer the rule is choosing between. tests/fixtures/timestamp_unwrap.json is the shared conformance file, 26 vectors and 11 window derivations, copied byte-identically from log-and-stream-common where the rule is specified and a reference implementation regenerates it in CI: git blob 367efd9 in both repositories. The Java, C# and Python APIs run the same file. Nine mutations were checked rather than assumed, each caught - including dropping the rate wiring from either client. Reorder and duplicate detection came out of a parallel fix for the same defect on the Java driver; the polarity and the window sizing here are different, but the observation that a backward step is not always a roll-over is hers. Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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.
…ector
Two things an adversarial review of this branch turned up.
The derivation clamps the window to an eighth of the counter's range, because a
window at or above the modulo leaves no backward step large enough to be a
roll-over - the unwrap stops counting them and a recording quietly runs short.
setReorderWindowTicks and the constructor option did not clamp, so a caller
could express the one thing the clamp exists to make impossible:
new StreamTimeline({ timestampBits: 16, reorderWindowTicks: 100000 })
stamp(65000); stamp(100) -> unwrapped = 100, wraps = 0
A plain 2-byte roll-over, silently lost. Clamped in _recomputeReorderWindow
rather than in the setter, so it follows a later setTimestampBits.
Nothing passes an override today except the conformance vectors, all of which
are at or below the clamp, so this changes no existing behaviour.
The second was a divergence rather than a defect here: hosts that keep an
unwrapped value and a cycle count encode "no sample yet" as (0, 0), which a
reorder landing exactly on the counter's origin can reach mid stream. This
timeline keeps lastRaw with a null sentinel and was already right, but the
sequence is now a shared vector - [520, 0, 2^24 - 16] - and this suite runs it.
It is the first vector whose final cycle is negative.
Also: setSamplingRateHz's doc said a client calls it whenever the rate changes.
Both clients call it once per stream, at _prepareStreamTimeline. Calling it mid
stream does work, and a test covers that, but the sentence described a
capability as though it were the wiring.
1944 tests pass; typecheck, lint and prettier clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #147, bringing this SDK onto the merged DEV-1023 rule. The spec and the vectors are
ShimmerResearch/log-and-stream-common#136.
What was still wrong after #147
#147 stopped an unstamped
0x000000record being read as a roll-over. It leftthe wider half of the problem in place: a reordered or duplicated packet costs
exactly the same 512 s, and the threshold that was supposed to tell the two
apart was sized as a fraction of the counter's range.
Those two quantities are unrelated. A reorder swaps packets that are adjacent in
time — a handful of sample periods. A dropout that happens to span the wrap point
is most of a modulo. Sizing the window by the modulo puts the boundary between
them in the middle of ordinary dropout territory:
A misread there loses the roll-over for the rest of the session.
The rule now
Three things in there are easy to get wrong, and each has a vector:
Forward is the default. A roll-over preceded by a long dropout is still a
roll-over, however much was lost. A rule that defaults the other way — "a
backward step is corrupt unless it clears some threshold" — fails exactly there.
Decide on the modular distance, not on unwrapped values. A packet arriving
late from just before a wrap boundary has a candidate above its predecessor,
so a comparison accepts it as forward motion of nearly a whole modulo and then
reads the next real sample as a second wrap.
[2²⁴ − 10, 5, 2²⁴ − 10, 70]landsat 33554502 — two modulos out, from one out-of-order packet.
An unknown rate is not an infinite window.
8 × 32768 / 0isInfinityinseveral of these languages, and an infinite window reads every backward step as
a reorder and loses every roll-over — the original bug, restored, with no symptom
until a recording comes out short.
API
reorderWindowTicks(hz, modulo), exported and pure.StreamTimelineOptionsgainssamplingRateHzandreorderWindowTicks;setSamplingRateHz(hz | null)andsetReorderWindowTicks(ticks | null).TimelineStategainsreorderWindowTicks, so a host can see that its ratereached the timeline.
Both clients pass the rate at
_prepareStreamTimeline(). That is one line each,and nothing else would notice it going missing — so there is a test per client
asserting the window is 5120 ticks and not the 2097152-tick fallback.
Nothing existing changes behaviour. A timeline nobody has told the rate to
still uses an eighth of the modulo. The other four APIs disable the branch
outright when the rate is unknown; they import files, where there is no host
clock to appeal to, whereas this SDK has the missed-wrap recovery as a second
witness. Follow-up: drop the fallback once every client always knows its rate.
One thing the fallback does not get. A reorder is judged before the
invalid-zero test, which is right for a window of a few sample periods — a zero
that close to an origin is genuinely ambiguous and choosing wrong costs 16 ms.
At 64 s wide the same order would read an unstamped record as a packet 64 s
late, place it 64 s early and mark it valid — worse than either answer the
rule is choosing between. So the fallback window classifies reorders but never
overrules the zero test.
Tests
tests/fixtures/timestamp_unwrap.json— 27 vectors and 11 window derivations,copied byte-identically from
log-and-stream-common, where the rule isspecified and a reference implementation regenerates and re-checks it in CI. Git
blob
de91de25accc7c74c0422f7e279a535da92579d9in both repositories. The Java, C#,Python and Swift APIs run the same file — five implementations of one wire format
drifted apart once already, with the same defect in all five, and reviewing them
against each other by hand is what let that happen.
1944 tests pass, typecheck, lint and prettier clean.
Nine mutations checked rather than assumed, each caught:
modulo / 8, ignoring the rateThe first harness I wrote reported nothing caught, because
--reporter=basicis not a vitest 5 reporter and every run was erroring out before it reached a
test. Worth saying out loud: a mutation run that catches nothing is a broken
harness until proven otherwise.
Since the first push — two review findings, both fixed here
An explicit reorder window was not clamped.
reorderWindowTicks()caps atmodulo / 8so a backward step large enough to be a roll-over always exists;setReorderWindowTicks()and the constructor option did not, so a caller couldexpress the one thing the clamp exists to prevent:
A plain 2-byte roll-over, silently lost. Clamped in
_recomputeReorderWindowsoit follows a later
setTimestampBitsrather than only the moment the setter ran.Nothing passes an override today except the vectors, all at or below the cap, so
no existing behaviour changes.
"No sample yet" was not distinguishable from a sample — in three of the five
APIs. Java, C# and Swift keep
(lastUnwrapped, cycle)and encoded the resetstate as
(0, 0); a reorder landing exactly on the counter's origin reaches thatstate mid stream. This timeline keeps
lastRawwith anullsentinel and wasalready right, but the divergence was real and unwatched:
[520, 0, 2²⁴ − 16][520, 0, -16][520, 0, 16777200]512 seconds apart, and no vector covered it. It is now
reorder-onto-origin-then-earlier-packet-24bitin the shared file, the otherthree are fixed, and reverting either fix reproduces it as a test failure. It is
also the first vector whose final cycle is negative.
Also corrected a doc comment:
setSamplingRateHzclaimed a client calls itwhenever the rate changes. Both clients call it once per stream. Calling it mid
stream does work and is now covered.
Related
Reorder and duplicate detection came out of @MAzalya's parallel fix for this
defect on the Java driver (DEV-1030). The polarity and the window sizing here
differ, but the observation that a backward step is not always a roll-over is
hers.
Release: patch bump via
cut-releaseafter merge; the vendor sync intoverisense-device-consoleandwebBLEDemosis a release step, not part of this.🤖 Generated with Claude Code