Repository navigation
test(cli): fail the import EOF test on the publisher's exit, not a deadline - #5155
Conversation
…adline Each wait while `moq import` still holds stdin open now races the child's exit, so a publisher that dies reports its exit status at once instead of surfacing ten seconds later as "announce timed out". The announce wait expects the publisher's Start directly, since the `Live` marker it skipped past no longer exists. The reported 8.8 s run and the loaded "announce timed out" did not reproduce on main, on the quest's base, or on the base the failure was seen on, so the quest closes with this hardening. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome of the quest-start run: the 8.8 s run and the loaded "announce timed out" did not reproduce on (Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 24 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78c63821be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Reproduce the slowdown, fix its cause, and coordinate the fixture through | ||
| observable readiness and completion events rather than elapsed-time |
There was a problem hiding this comment.
Because the reported slowdown and loaded failure were not reproduced, the new running helper only improves failure diagnostics and does not establish or fix the original cause. Deleting this quest now removes the remaining record of a test that was observed failing under load; keep it open until the failure is reproduced and fixed at its source, or explicitly abandon it as a maintainer decision.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I disagree, and this is a maintainer decision: the maintainer chose option A, which closes the quest with this hardening. The test now fails right away with the publisher's exit status, so if the failure comes back it reports its real cause instead of "announce timed out". The related leads are still tracked in #5151 (ts-passthrough-jump, import-catalog-drop).
(Written by Claude Opus 5.5)
|
Merge summary:
(Written by Claude Opus 5.5) |
1 participant
Completes
quest/m1/test-flakes-2/import-catalog-finish.md. The reported slowness and the loaded failure do not reproduce, so this PR adds event-based hardening and closes the quest. The deletion is a maintainer call; see Decision below.Reproduction
import_delivers_the_catalog_finish_at_eofwas reported at 8.8 s alone, and at "announce timed out" under a loadedjust check. I could not reproduce either:main(8bc3e7f04era)moq-relay/moq-tokio/moq-clinextest run at load average ~50; 1.3 s with 12 copies pinned to one CPUedd671fff^48d89430dTest, Oct 6 and Oct 9)None of the attempts came close to 10 s. Two other environment guesses didn't reproduce it either: a network namespace with IPv6 disabled, and 100 ms of netem delay on loopback (2.4 s, which scales with RTT).
Change
moq importstill holds stdin open (the announce, the broadcast, the first catalog) now races the child's exit. If the publisher dies, the test fails at once with its exit status (checked: a bad subcommand now fails in 0.2 s withmoq exited with exit status: 2 before the announce). Before, the same failure showed up 10 s later as "announce timed out", and that message could cover the test(mux): skip truncated spliced groups as aborted #4707 failure.TIMEOUTstays as the hang guard and is unchanged.Startondemo. The loop that skippedLiveevents is gone because feat!: delete the announce Live marker #4916 deleted that marker.moqsubprocess EOF coverage is unchanged.Public API / wire impact
None. Test-only, plus the quest deletion.
Decision
The premise didn't reproduce, so what should happen to the quest?
Verification
cargo nextest run -p moq-cli --test import: pass (0.44 s).just check: lint and build pass, andmoq-cli::importpasses. The test pass fails on the unrelatedmoq-cliunit testpublish::tests::ts_passthrough_crosses_a_relay_through_a_flagged_jump("moq-transport-14: both copies crossed") in 2 of 3 runs. That test is untouched here, and fix(mux): keep the last good parameter sets when refusing a TS access unit #5153 reports the same failure on unmodifiedmain.Follow-ups
ts_passthrough_crosses_a_relay_through_a_flagged_jumpflake undertest-flakes-2(from feat(moq-mux): import ts --passthrough carries the multiplex whole #5003). It paces input with a 15 ms wall-clock sleep and has a 10 sWAIT.track::Producer dropped without finish() or abort() track=catalog.json, while the subscriber still sees a clean finish. Some catalog producer, probably the per-request copy, is dropped without a finish. It's worth a small look.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code