fix(test): two flakes in the tests added today, found by stress.yml - #359
Merged
Merged
Conversation
The test I added in #311 waits for `contains("panicked")` and then asserts the message. The Rust runtime writes the location line and the message as separate writes, so the predicate is true one line before the message exists. It went red on the `features` job with the grid holding TUI drawing here thread 'main' (7206) panicked at fixtures/emit/src/main.rs:281:37: and nothing under it — the assertion's own output is the evidence. Every wait now names the message. That is rule 3 of the wait-semantics contract in docs/DESIGN.md §2: name the last thing the application paints, so that its truth implies the rest arrived. "panicked" is asserted instead of waited on, since it cannot be absent once the line after it is there. Not reproducible here: 60 runs of the pre-fix test at --test-threads 8 all passed on this machine, which is why it reached main. The fixed test is 60/60 too, so the local runs prove nothing either way; the CI grid above is the measurement, and stress.yml is the instrument. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`with_stdin` wrote the input with `.expect("write to the child's stdin")`.
`diff - -` refuses its arguments before reading anything, so the child can
exit and close the pipe before that write lands, and the helper panicked on
a broken pipe — the faster the refusal, the likelier it is.
write to the child's stdin: Os { code: 32, kind: BrokenPipe, … }
Found by stress.yml on main, on both the 4- and 8-thread Linux shards, at
iterations 13 and 15 of 25. It never reproduced on an idle machine, where
the bytes reach the pipe buffer before the child can exit.
A broken pipe is a result, not a failure: the callers assert the child's
exit code and stderr, and whether it chose to read the input is the child's
business. Proven both ways by forcing the race with a sleep before the
write — the old helper fails with exactly the message above, the new one
passes.
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
3 tasks
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.
Two races in tests I added today, both found by
stress.ymland neitherreproducible on an idle machine. No library, CLI or fixture code changes.
1. The panic test waited for the wrong thing (from #357)
Each case waited for
s.contains("panicked")and then asserted the message.The Rust runtime writes the location line and the message as separate
writes, so the predicate goes true one line before the message exists. The
featuresjob caught it, and the assertion's own output is the evidence:Every wait now names the message — rule 3 of the wait-semantics contract
(
docs/DESIGN.md§2): name the last thing the application paints, so itstruth implies the rest arrived.
"panicked"is asserted rather thanwaited on, since it cannot be absent once the line after it is on the grid,
so the README's actual promise is still checked.
2. A refused stdin operand may close the pipe first (from #356)
with_stdinwrote the input with.expect("write to the child's stdin").diff - -refuses its arguments before reading anything, so the childcan exit and close the pipe before the write lands:
Found by
stress.ymlonmain, on both the 4- and 8-thread Linuxshards, at iterations 13 and 15 of 25. The faster the refusal, the likelier
it is — the feature working well is what broke the test.
A broken pipe is a result, not a failure. The callers assert the child's
exit code and stderr; whether it chose to read the input is the child's
business.
Evidence, and its limits
#2 is proven deterministically. Forcing the race with a sleep before the
write: the old helper fails with exactly the CI message above, the new one
passes. Both directions, locally.
#1 is not. 60 runs of the pre-fix test at
--test-threads 8passed onthis machine, which is why it reached
main; the fixed version is 60/60too, so neither number is evidence and I am not offering them as
verification. What stands up is the CI grid above showing the two writes
split, and the fact that the new predicate cannot be true before the message
is on the screen.
On the failing branch shard, the panic test itself passed 21/21; on
main's two failing shards, 15/15 and 13/13.The third failure, which is not mine
main's stress run and this branch's earlier one each turned up one moreshard, and the Windows one is a different animal: four
queries.rstestsfailed on
windows-latest, 16 threadswith an empty screen at a 500mstimeout — the child had not written its query bytes yet.
That is a pre-existing class, not something this PR introduces: the same
shape failed on
docs/stability-candidateon 2026-09-10 (windows-latest, 8 threads,wait_idle_for_overrides_the_builder_default), before any oftoday's work. It is filed separately rather than absorbed here.
stress.ymlis running again on this branch.