test(tools): a wire assertion must be its own wait - #1082
Merged
Merged
Conversation
andrewgazelka
enabled auto-merge
July 30, 2026 02:42
Benchmark Results for generalComparing to dd8311f |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1082 +/- ##
=======================================
Coverage 54.65% 54.65%
=======================================
Files 361 361
Lines 33257 33257
Branches 1259 1259
=======================================
Hits 18178 18178
Misses 14793 14793
Partials 286 286 🚀 New features to boost your workflow:
|
andrewgazelka
force-pushed
the
test/wire-assertions-are-their-own-wait
branch
from
July 30, 2026 02:49
fddc5ee to
04030cf
Compare
Benchmark Results for generalComparing to 34178ea |
andrewgazelka
added a commit
that referenced
this pull request
Jul 30, 2026
…1083) `smash-selector-e2e` fails intermittently with FAILED S1 picked the Skeleton and the action bar never said so: [] and the line it says never arrived is in the same log **0.2 ms later**. The server is not at fault. `lobby::choose` sends two packets, a chat line and then a separate `Channel::ActionBar` line. The check waited on `client.kit`, which `on_chat` sets from the *first* of the two, then read the action bar with no further wait. The harness reads one packet per pump, so a burst split across two reads leaves the second packet still on the wire when the assertion runs. Draining harder would not help; the bytes had genuinely not arrived. Every other action bar assertion in the suite either settles first or waits on the action bar itself. This was the only one keying its wait on a different packet that merely precedes the one it asserts on. Proved in **both** directions on a Linux builder, because the unfixed tree also passes on a good run and one green run here proves nothing: fixed PASS ... the server answered ['You are the Skeleton.'] negative with the `Channel::ActionBar` send deleted from `lobby::choose`, rc=1, failing after the full 10 s wait rather than passing vacuously That the negative control was necessary rather than merely thorough is now measured: `smash-hud-e2e` and `smash-selector-e2e` each failed one CI run and passed the next on effectively the same tree. Two of five failures in that suite are coin flips. Split out of #1082 deliberately. That PR carries this fix plus a 322 line static analyzer that stops the whole class being written again, and the analyzer deserves an unhurried review. This half is costing a 35 to 90 minute CI run every time it fires and reads as a server bug to whoever sees it next, so it should not wait on the discretionary half. Diagnosed, fixed and controlled by a subagent; the negative control was its own idea. Split and landed by me. (sent by an AI agent via Claude Code)
`smash-selector-e2e` failed in CI with "S1 picked the Skeleton and the action bar never said so: []" while the server had sent that exact line. Run 30500640846 recorded the failure at 00:21:41.7509285 and logged the line it was waiting for at 00:21:41.7511018, a fifth of a millisecond later. The check waited on `client.kit`, which is set from the chat packet `lobby::choose` sends one ahead of the action bar, and `drain` takes a single `recv` per pump, so a burst split across two reads satisfies the wait with the asserted-on packet still in flight. Sweeping for other instances finds today's and not tomorrow's, so this adds the rule instead: an assertion about a field the packet handler fills must be preceded, in the same function, by a wait whose predicate reads that same field. `must_become` and `must_not_become` make the predicate the assertion, so waiting on one thing and asserting on another is not expressible through them. The rule is deliberately narrow. It only looks at functions that wait at all, because the bug is not "forgot to wait", it is "waited, and for the wrong packet", and flagging every synchronous assertion is how a check becomes noise and its escape hatch becomes a mute button. The observed field set is derived from what the packet handlers fill rather than listed, so a new handler brings its field under the rule the day it lands. Three sites, which is what the rule flags on this tree rather than a hand-picked set. The selector's kit announcement and the mid-match refusal are real: the first is the shipped bug, the second slept `settle(1.0)` where a second happened to be generous. The lobby-count assertion takes the escape hatch, which carries a required reason because a bare marker is a mute button. A single green run proves nothing about this class, which is why the fix was verified against a negative control that deletes the announcement rather than against one passing build. The unfixed tree passes on a good run: measured directly, and confirmed across the suite the same night, where `smash-hud-e2e` and `smash-selector-e2e` each failed one CI run and passed the next on effectively the same tree. Two of five failures in that suite are coin flips. This converts "you can wait on the wrong packet" into "you can write the wrong predicate", which is a real trade: the second is an ordinary bug, and the first reads as a server defect and sends someone into the wrong code for an afternoon.
andrewgazelka
force-pushed
the
test/wire-assertions-are-their-own-wait
branch
from
July 30, 2026 02:55
04030cf to
afa2fee
Compare
Benchmark Results for generalComparing to b3870e4 |
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.
smash-selector-e2efailed in CI with exactly one assertion:The server had sent that line. Run 30500640846 recorded the failure at
00:21:41.7509285and logged the line it was waiting for at00:21:41.7511018:The check waited on
client.kit, which is set from the chat packetlobby::choosesends one ahead of the action bar, anddraintakes a singlerecvper pump, so a burst split across two reads satisfies the wait with theasserted-on packet still in flight. Production code was not at fault.
That fix landed on its own in #1083. This is the rule that stops it recurring.
What this adds
nix/verify-wire-assertions.py, wired in as a flake check so the subtractivegate in
nix/ci/flake-gate.nixpicks it up without anybody adding it anywhere.The rule: an assertion about a field the packet handler fills must be preceded,
in the same function, by a wait whose predicate reads that same field.
must_becomeandmust_not_becomemake the predicate the assertion, so waitingon one thing and asserting on another is not expressible through them.
The rule is deliberately narrow. It only looks at functions that wait at all,
because the bug is not "forgot to wait", it is "waited, and for the wrong
packet". The observed field set is derived from what the packet handlers fill
rather than listed, so a new handler brings its field under the rule the day it
lands. Two limits are documented in the script: it does not follow a field
through a helper call, and it says nothing about functions that never wait.
What it flags today, and what I did with it
Three sites, which is what the rule reports on this tree rather than a set I
picked. I deliberately did not hand-convert the other 64 assertions: that diff
is mechanical, and a conversion with a subtly wrong predicate turns a real
assertion into one that cannot fail while the gate stays green.
a_click_picks_the_mobmust_becomesettle(1.0)where a second happened to be generous. Nowmust_not_becomefor the kit andmust_becomefor the refusalThe escape hatch is
# not-a-wire-assertion: <why>and the reason is part ofthe syntax: a marker with nothing after the colon is itself an error, because a
bare marker is a mute button.
Break-it results
The lint was broken three ways and watched, through the real flake check. Three
cases and not one, because a rule that rejects everything passes the
reject-the-bad-thing test perfectly: the first version of this script flagged 67
of 67 assertion sites.
smash-selector.py:1191: in a_click_picks_the_mob, this fails on action_bar, which the packet handler fills, and no wait before it reads it.must_become:14 client scripts checked, 0 problemssmash-selector.py:1349: not-a-wire-assertion with no reason after the colon. The reason is the point: say which non-wire thing this asserts.Because two live assertions changed, the gate they live in was re-run rather
than trusted:
nix build .#checks.x86_64-linux.smash-selector-e2e, rc=0, withboth converted claims passing.
Enforcement, stated plainly
The check runs in CI automatically via the
Flakejob and nobody has toremember it. It does not block a merge: ruleset 566717 on
maincarries norequired_status_checksrule, so no check in this repository is a requiredcontext and a red gate is enforced by somebody noticing. ENG-10827.