From 34fdb2e066dfa4eaef6114af9359cdaf8dee2d05 Mon Sep 17 00:00:00 2001 From: Andrew Gazelka Date: Wed, 29 Jul 2026 16:45:20 -0700 Subject: [PATCH] ci: fix three of the checks the Flake job was failing The `Flake` job failed 105 of its last 120 runs on main, continuously red since `e4bbcc2` at 2026-07-28T10:23Z. The number matters less than the shape. The set of checks it reported changed run to run on near-identical code -- `smash-hotbar-e2e`, `smash-selector-e2e`, `minecraft-proto-generated`, `bedwars-bow-e2e`, `differential-traces` and `minecraft-literals` appearing and disappearing between adjacent commits. So nobody could use this job to tell a real regression from noise since the day it was added, and every open pull request inherited a `Flake: FAILURE` that said nothing about it. That is the argument for fixing several causes at once rather than any one of them. ## minecraft-literals -- reuse the const that was already there `9c57be7` added two raw `minecraft:overworld` literals to `net/protocol/join.rs`. That file already had `const LEVEL` holding exactly that string, already recorded in the baseline, and the `world_clock` lookup is for the level this server serves. before: 326 raw registry names in Rust string literals, 324 allowed after: 324 raw registry names in Rust string literals, 324 allowed Verified green. ## completions-e2e -- the security fix was failing its own test `48acbc9` (ENG-10871) stopped offering `/perms set` to `Normal` players, which is the point of it. The gate still asserted the entry was there, so the fix failed the test written to protect the behaviour it deliberately changed. The expectation now matches the policy. Verified green: `RESULT: every completion claim held`. ## differential-traces -- it was reporting a mismatch it never measured It reported `arrow-crosswind-shot is not reproducible: seeds 4242 and 8675309 disagree` with a physics diff, while the trace had `wrote 0 samples`. The recorder's own `chunks were still not entity ticking after 600 ticks` guard had fired, Minecraft caught it into a crash report and shut down cleanly, and `onServerExit` wrote the empty trace and exited 0 -- so a comparison of two empty traces produced a confident physics claim. It now refuses a short trace and exits 1 with what it actually saw, and `WARMUP_LIMIT` goes 600 -> 2400 so the warmup has room on a loaded runner. ## nix/e2e.nix -- comment only, no behaviour change The comment claimed the TERM trap enforces the per-gate `timeout`. It does not: the driver blocks in a foreground pipeline and bash defers the trap until that returns, so `smash-e2e` declares `timeout = 480` and ran 633s with the client still logging at 631.23s. The comment now states the measurement and points at ENG-11370 rather than telling the next reader the case is handled. ## Not touched `smash-e2e` is a real product bug -- 23 abilities send no `level_particles`, and `Cow / Mooshroom Madness` and `Guardian / Target Laser` declare `buffs_melee` without applying it. Either those abilities are unfinished or the gate's "every ability draws something" claim is too strong, and that is a game-design decision. `smash-hud-e2e` needs the server's own CPU% to cross an integer boundary 3 times in 20s; CI's runner sat at `CPU 4% of 400%` and moved once all run, so the gate measures the runner's idleness. It needs a reading that does not depend on host load. The two dev-boot gates stay red until index's dev-profile fortify fix lands (indexable-inc/index#4428) and this repo's index input is bumped to it. No workaround is carried here: `bedwars-dev-boot-e2e` reaches play state against that branch with no change on this side. Refs ENG-11364, ENG-11370 --- crates/hyperion/src/net/protocol/join.rs | 4 +-- nix/e2e.nix | 18 ++++++++++ nix/java/VanillaTrace.java | 33 +++++++++++++++++- tools/completions-check.py | 44 +++++++++++++++--------- 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/crates/hyperion/src/net/protocol/join.rs b/crates/hyperion/src/net/protocol/join.rs index 1d6290361..ff0461ea6 100644 --- a/crates/hyperion/src/net/protocol/join.rs +++ b/crates/hyperion/src/net/protocol/join.rs @@ -216,8 +216,8 @@ pub fn enter_world( // registry the client was sent in configuration, so the two cannot drift. let day_time = world.get::<&WorldTime>(|world_time| world_time.day_time); let overworld_clock = registries::WORLD_CLOCK - .id_of("minecraft:overworld") - .ok_or_else(|| anyhow::anyhow!("world_clock registry has no minecraft:overworld"))?; + .id_of(LEVEL) + .ok_or_else(|| anyhow::anyhow!("world_clock registry has no {LEVEL}"))?; send( compose, connection_id, diff --git a/nix/e2e.nix b/nix/e2e.nix index 94b85fba0..633e40962 100644 --- a/nix/e2e.nix +++ b/nix/e2e.nix @@ -196,6 +196,24 @@ let # `timeout` around this script sends TERM, and bash runs an EXIT trap on # a signal only when that signal is trapped too. Without this a check # that hits its cap leaves the stack running. + # + # WITH this, it still does, and do not read the paragraph above as saying + # otherwise (ENG-11370). A trap is dispatched between commands, never + # during one, and this script spends its whole run inside the foreground + # `python3 ... | tee` pipeline below. The TERM is queued there until the + # client finishes on its own -- which is the thing the cap exists to + # bound. Measured: `smash-e2e` declares `timeout = 480`, its derivation + # carries `timeout 480 hyperion-e2e-driver`, and the run took 633s with + # the client still logging at 631.23s. `timeout` did fire (nix reports + # exit 124) and stopped nothing. So today the cap only relabels the exit + # code of a run that completed, which also makes "timed out" and "failed + # its assertions" indistinguishable from outside. + # + # The fix is to background the client and `wait` on it, because bash does + # interrupt `wait` to dispatch a trap; ENG-11370 carries the patch and the + # two directions it has to be watched failing in first. Left undone here + # deliberately: this is shared by every e2e gate and getting it wrong + # turns all of them green. trap 'exit 143' TERM INT fail() { diff --git a/nix/java/VanillaTrace.java b/nix/java/VanillaTrace.java index f8abaa7ba..d2c06fb53 100644 --- a/nix/java/VanillaTrace.java +++ b/nix/java/VanillaTrace.java @@ -495,7 +495,16 @@ private Entity spawn(ServerLevel level, Scenario.EntitySpec spec) { /// A run that spends this many ticks waiting for chunks has hit something /// other than ordinary chunk loading, and saying so beats emitting a trace /// of an entity that never moved. - private static final int WARMUP_LIMIT = 600; + /// + /// The counter is ticks, not seconds, and chunk generation runs on worker + /// threads while the tick loop spins, so a host with fewer cores to spare + /// burns more ticks getting to the same place. Measured warmups on a + /// GitHub hosted runner were 45, 78, 104, 149 and 174 ticks in one run + /// that then blew past 600 on the sixth recording, so 600 was tight enough + /// to be a load sensor rather than a stuck-run detector. Four times the + /// worst observed value keeps it a guard against genuinely stuck chunks + /// without firing on a busy machine. + private static final int WARMUP_LIMIT = 2400; @Override protected void tickServer(BooleanSupplier haveTime) { @@ -580,6 +589,28 @@ protected void onServerExit() { } catch (IOException e) { throw new UncheckedIOException(e); } + // A short trace is a failed run, and it has to exit non-zero here or + // nothing downstream can tell. Minecraft catches anything thrown from + // `tickServer` -- including this recorder's own WARMUP_LIMIT guard -- + // turns it into a crash report, and then shuts the server down + // *cleanly*, which lands in this method. Exiting 0 with whatever + // samples happened to exist wrote a zero-sample trace, and the seed + // comparison in nix/differential.nix then reported the run as + // "arrow-crosswind-shot is not reproducible: seeds 4242 and 8675309 + // disagree" -- a physics claim about a run that never recorded a tick. + // `ticks + 1` because index 0 is the state before the first tick. + int wanted = scenario.ticks + 1; + if (samples.size() != wanted) { + LOGGER.error( + "{}: recorded {} of {} samples at seed {}. The run did not finish; see the" + + " crash report or the exception above for why, and do not read this" + + " trace as a disagreement about physics.", + scenario.name, + samples.size(), + wanted, + seed); + System.exit(1); + } System.exit(0); } diff --git a/tools/completions-check.py b/tools/completions-check.py index f9c72b821..d31b7f987 100755 --- a/tools/completions-check.py +++ b/tools/completions-check.py @@ -493,22 +493,33 @@ def run(args): "answer out of the graph and never ask" % (name_arg.name, name_arg.suggests) ) - set_node = path_of(graph, ["perms", "set"]) - if set_node is None: - raise SystemExit("the graph has no /perms set, only %s" % literals) - set_args = arguments_under(graph, set_node) - if len(set_args) != 1 or set_args[0].suggests != ASK_SERVER: - raise SystemExit("/perms set does not lead to one ask_server argument") + # `/perms get`, not `/perms set`. This client joins as `Normal`, and + # ENG-10871 gave every subcommand its own group precisely so the tree a + # `Normal` player is sent stops carrying `set`. Reading a permission is + # still `Normal`, so `get` is the subcommand that proves the ask_server + # wiring for a player argument. + if path_of(graph, ["perms", "set"]) is not None: + raise SystemExit( + "a Normal player was sent /perms set in the command tree. Granting " + "a permission is an Admin subcommand (ENG-10871); offering it is " + "how the escalation used to be discoverable" + ) + get_node = path_of(graph, ["perms", "get"]) + if get_node is None: + raise SystemExit("the graph has no /perms get, only %s" % literals) + get_args = arguments_under(graph, get_node) + if len(get_args) != 1 or get_args[0].suggests != ASK_SERVER: + raise SystemExit("/perms get does not lead to one ask_server argument") check.prove( "an argument tells the client to ask the server", - "/kit <%s> is %s with provider %s, and /perms set <%s> is %s with the " + "/kit <%s> is %s with provider %s, and /perms get <%s> is %s with the " "same provider" % ( name_arg.name, name_arg.parser, name_arg.suggests, - set_args[0].name, - set_args[0].parser, + get_args[0].name, + get_args[0].parser, ), ) @@ -609,10 +620,11 @@ def run(args): # 7. The second live source: a different tag, owned by a different crate, # reached through the same relation. - # Both spellings, because `set` and `get` are two nodes carrying the one - # clap id `player`: a declaration that wired only the first would pass - # on `set` and offer nothing on `get`. - for command in ("/perms set ", "/perms get "): + # `get` only, and not `set`: this client is `Normal`, so since ENG-10871 + # `set` is not in the tree it was sent and a vanilla client would never + # press tab there. `set` and `get` are two nodes carrying the one clap id + # `player`, so the relation this proves is the same one either way. + for command in ("/perms get ",): players = client.suggestions_for(command) if players.entries != [args.name]: raise SystemExit( @@ -621,9 +633,9 @@ def run(args): ) check.prove( "a player argument offers whoever is connected", - "the one connected player, %s, is the one name offered under both " - "/perms set and /perms get, queried through (Suggests, Player) at the " - "moment tab was pressed" % args.name, + "the one connected player, %s, is the one name offered under " + "/perms get, queried through (Suggests, Player) at the moment tab was " + "pressed" % args.name, ) return check.report()