Skip to content

smokeng v0.17.0

Choose a tag to compare

@Timdebruijn Timdebruijn released this 01 Sep 23:17
· 20 commits to main since this release

v0.17.0 — a review of v0.16.0, and what it found

Seven changes, all from reviewing the release before it. Two of them are the
kind of defect this project exists to prevent, and one is a monitor that
quietly stops monitoring.

Two irtt targets probing at once could wedge one of them indefinitely. Every
irtt client shared one package-level timer and averager whose fields are
written without synchronisation, and each target is probed on its own
goroutine. One session completes, the other never returns, and the probe's own
deadline does not free it because what is stuck is not waiting on that context.
The target stops delivering measurements with nothing in the log. Two irtt
targets is what a SmokePing import produces, since its IRTT probe graphs one
figure per target.

server_processing recorded fabricated zeros against a server started with
--tstamp=single. At a midpoint stamp the two server timestamps hold the same
value, so the difference between them is not unavailable but exactly zero — a
distribution of zeros kept forever under a heading that says how long the far
end held each packet. The same stamp also leaks half the server's hold time
into both jitter figures, so a loaded peer graphs its own scheduling as network
jitter; measured, not argued. Every extra series now requires a stamp at both
ends, and a wall-clock fallback silences them too, because inter-packet delay
variation only cancels the clock offset while it is a monotonic difference.

A signed agent could exhaust the master with a sub-megabyte request: Arrow IPC
allocates whatever uncompressed size a message header claims, with no ceiling.
629 KB grew the heap by 5.6 GiB. Unvalidated list offsets were used as a slice
capacity, so 408 bytes reserved 8 GiB. Both are bounded now, and an unsorted
distribution is normalised rather than failing a write that would have wedged
that agent's outbox forever.

Measurements record why a send failed, not only that it did. An irtt target
losing a probe an interval could not say whether the far end refused the
traffic or this prober never got it out — a network fault and a bug here,
stored identically, which cost three wrong theories and an hour of a
production SmokePing.

The rest: a corrupt optional series no longer takes the whole window with it,
the outbox query no longer scans the table (or overruns SQLite's expression
depth, which the first fix did), and the crosshair reads the interval it is
over rather than the nearest one — wrong for half of every bucket.

Schema v21, additive. Both wire formats gained optional columns, so master and
agents may be upgraded in either order.

The tests are half of this release. Mutating the code found nine checks that
could be deleted with the suite still green, including the orphan invariant the
storage design rests on and the version byte of the samples blob. go test -race
did not work for the probe package at all.

Every finding here came from review, and most were not in what the code did but
in what its comments claimed it did.