Count the packets that left in a different order, and let throughput be read in Mbit/s - #176
Merged
Merged
Conversation
Jitter and the latency spike change the order packets go out in, and nothing measured whether that actually happened. It is not derivable from the settings: whether a delayed packet is overtaken depends on the gap between packets, so at two packets a second a 50 ms spike reorders nothing. Without the number, a run where reordering never occurred reads exactly like a run the application survived - the hole loss_bursts was added to close for runs of loss. - engine: read the arrival sequence the heap entry already carried and compare it against a per-direction high-water mark, after send() rather than before, so a packet the driver refused cannot make the next one look overtaken by one that never reached the stack - counter surfaces on the Statistics tile and session panel, in the stats CSV as packets_reordered, and in the repro report; the NDJSON sample schema is left alone (frozen contract) - carve damage.py out of engine.py: adding the counter pushed that module past the file-size crowd band (817 logic lines against 816.2), and the ratchet is answered by moving code out, not by raising the number. The five names moved read a stats dict and touch no thread, handle or packet - tests/test_reordering.py: an overtaken packet counts, kept order does not, the directions are judged separately, a refused send does not move the mark, and a restarted session does not inherit the old one Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…that could not fail Five entries in the mutation registry, so "these tests catch it" is a claim CI repeats instead of a sentence in a changelog. The fifth mutant SURVIVED the first run, and that was information about the test: the arrival counter was built in __init__ and never reset, so a second session's numbers were always higher than a stale high-water mark and the restart assertion could not fail in either direction. Fixed at the source rather than in the test - arrival numbering is now per session and resets alongside the mark it is judged against, since the mark only means anything against numbers from the same numbering. stop() clears the heap, so a reused number cannot meet a queued entry that still holds it as a tie-breaker. - repoint "engine: the Internet-only drop loses its own counter" at damage.py: the carve-out moved its pattern, so it reported SKIP, and a skip reads like a pass. Re-run from its new home: caught - reordering mutations, re-run after the fix: 5 of 5 caught Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The knob for out-of-order delivery already existed - a latency spike is "delay X% of packets by Y ms", which reorders traffic without smearing every other packet's delay the way jitter does. Nothing said so. It sat in a card called "Latency (ping)" under a tooltip about momentary lag, and the Control page search matches names rather than tooltip bodies, so a person looking for reordering found nothing. - rename the card to "Latency (ping) and packet order" in all three languages. The section title is folded into the haystack of every field inside it, so one string makes the card and all four of its fields findable. MEASURED: "order" and "kolejnosc" went from 0 hits to 5 - rewrite tips.spike so its second sentence carries the consequence that matters instead of restating the first - README: a paragraph on testing out-of-order delivery, and what a zero in the new Reordered figure does and does not mean - guard the discoverability itself, since a later reword would take it away silently Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A speed limit is typed in KB/s and a link is sold in Mbit/s, and the tool made you do that conversion in your head - in the one place where getting it wrong is invisible, because a wrong limit still runs. The switch is DISPLAY only, and that is forced rather than chosen: KB/s is what a saved config file, the throughput schedule, the shipped scenarios, --down/--up and the NDJSON down_kbps/up_kbps fields all carry, and several of those are frozen contracts. So it converts on the way to the screen and never on the way to a file. - new CHOICE kind in the Pref registry, rendered as a readonly combobox. The language box beside it is hand-rendered and is a named exception, so copying that would have copied the exception rather than the rule - gui/rates.py gains the unit table and the conversion; the rate fields are a VIEW over the field registry (unit == "KB/s"), not a list of names - wired into both Statistics tiles and their captions, the copied text, the session peak and average, the chart axis and caption, and a live grey readout beside Download and Upload. The chart converts labels only: the series and the ceiling stay in KB/s - the readout is pushed from set_pref rather than polled - the dropdown is in a different window from the label it moves - 1024 KB/s is 8.39 Mbit/s and not 8: K is 1024 here, a megabit is a decimal million, and the two do not cancel. The factor is derived in the open and the comfortable wrong answer is asserted against, because that version looks more right than this one Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eiling The reaction to the speed-unit preference was in App.set_pref, which put five logic lines into the one module that sits ON the file-size ratchet with zero headroom by construction - it went 1166 to 1171 and the full suite said so. The answer to a ceiling here is to move code out, not to raise the number, and this project already has the right home for it: pages.pref_changed broadcasts to whichever page cares, and its own docstring gives this exact reason for existing. - move the refresh to ControlPage.on_pref_changed, beside the one that shows and hides the search bar - point the test at the production path (set_pref then pref_changed) rather than calling the form directly: it was green through the move while the wiring was broken, which is the half worth guarding - one more mutation entry for that wiring, caught - annotate the three new rates.py helpers - that module is on the mypy strictness ratchet, so an unannotated def there is a red job - trim the user-facing changelog entry under the 100-word ceiling Verified: 1373 passed, ruff and mypy clean, GUI smoke OK, and the crash log is EMPTY after a full run - no fake drifted from a real interface and no guarded path went silent. 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
1 participant
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.
Two asks, and the first one turned out to be half built already
Reordering as its own knob. It exists: a latency spike is "delay X% of packets by Y ms", it
reorders traffic without smearing every other packet's delay the way jitter does, and the release
heap is keyed by release time so packets genuinely come out in a different order. Nothing said so.
It sat in a card called "Latency (ping)" under a tooltip about momentary lag, and the Control page
search matches names rather than tooltip bodies - so a person looking for reordering found nothing.
So this adds the two things that genuinely did not exist rather than a second field doing what
--spike-probalready does:Reorderedcounter, because "20% got +50 ms" is NOT "20% arrived out of order" - whetheranything overtakes anything depends on the gap between packets, which is a property of the
traffic. Without the number, a run where reordering never happened reads exactly like a run the
application coped with. Same hole
loss_burstswas added to close for runs of loss.order". Measured before and after:
orderandkolejnoscwent from 0 hits to 5 in bothlanguages (the section plus all four fields in it).
Speed units. A limit is typed in KB/s and a link is sold in Mbit/s. The switch is DISPLAY only,
and that is forced rather than chosen: KB/s is what a saved config file, the throughput schedule,
the shipped scenarios,
--down/--upand the NDJSONdown_kbps/up_kbpsfields all carry, andseveral of those are frozen contracts. It converts on the way to the screen and never on the way to
a file, and a grey
(= 8.39 Mbit/s)appears beside the field you type into.Measured, not asserted
loopback, arrival order read from the socket, so the engine's number and the application's come
from different ends of the same event. Two runs, elevated, real driver: control 0 out of order
and 0 lost both times, and the two agreed exactly in all six comparisons (788/788, 826/826,
1974/1974, 1989/1989, 0/0, 0/0). The rig itself is outside git, so only its effect is visible
here.
too_sparsecase is the one worth having: a spike smaller than the gap between datagramscannot make anything overtake anything, so a configured spike reports zero on both sides.
That sentence is now in the tooltip, the README and the counter's docstring, and nothing else
measured it.
than claimed here. One SURVIVED the first run and that was information about the TEST: the
arrival counter never reset, so a restart assertion could not fail in either direction. Fixed at
the source - arrival numbering is now per session, alongside the mark it is judged against.
measures buttons and checkboxes only, so the new labels needed their own pass; the converted
readout asks for 122 px and gets 122.
Shape
damage.pyis carved out ofengine.py: adding the counter pushed that module past the file-sizecrowd band (817 logic lines against 816.2), and the ratchet is answered by moving code out. The
five names moved read a stats dict and touch no thread, handle or packet. The unit preference
needed a
CHOICEkind in thePrefregistry - the language combobox beside it is hand-renderedand is an explicitly named exception, so copying that would have copied the exception rather than
the rule.
The NDJSON
sampleschema is deliberately untouched; new information goes down the text channel,as the F17 ADR settled. The stats CSV gains
packets_reordered, and an existing file is rotated toa dated backup by the header check that already exists for this.
Verification
1373 passed, ruff and mypy clean, GUI smoke OK, CI render check OK in en/pl/zh - and the crashlog is empty after a full run, so no fake drifted from a real interface and no guarded path
went quiet.
Not done, and deliberate: the rig's baseline reference is not recorded, because
baseline.pyrefuses to record off
origin/master- a reference names the commit that produced it and a squashmerge throws the branch commit away. It wants recording on master after this lands.
🤖 Generated with Claude Code