Repository navigation
test(simulator): bound the panel waits by a deadline, not a yield count - #13907
Conversation
`"A surviving Simulator host keeps framebuffer publication active"` failed on manaflow-ai#13414's app-host shard 1 with `frameTransport → nil` at SimulatorPanelThemeTests.swift:95 and :101, while passing on `main` and on manaflow-ai#13752's shards. Nothing in that PR touches this subsystem. The waits here spun a fixed `for _ in 0..<100 { ... await Task.yield() }` before asserting. A yield count is not a deadline: `Task.yield()` gives the scheduler a chance to run something else, it does not wait for anything, so 100 yields is however long 100 reschedules happen to take. The bound tightens exactly when the runner is busy, which is the "fails on correct code under load" shape `.github/review-bot-rules/test-determinism.md` bans: the deadline bounds the FAILURE path only, so load can make a pass slower but never turn a pass into a fail These 10 sites now poll the same predicate against a `ContinuousClock` deadline. They return the instant the condition holds, so a passing run is no slower, and only a genuinely broken one waits out the 10 s. No assertion changed. Two sites are deliberately left alone: the bare `for _ in 0..<100 { await Task.yield() }` at SimulatorPanelIntegrationTests.swift:187 and :252 assert an *absence* (`discoveryCount == 0`, `!isCompleted`). A deadline cannot bound a wait for a non-event; those need a positive completion signal to assert after, which is a change to what the test observes rather than how long it waits. Their failure direction is also the opposite one — a short spin makes a false green, not a false red. Scope note: 17 more yield-count polls remain in 12 other `cmuxTests` files, and `scripts/check-test-determinism.py` reports 0 findings across the tree because it only matches sleep call sites. Tracked in manaflow-ai#13903; this commit is the cluster with the observed failure. Verified: `swiftc -parse` on both files; `check-test-determinism.py --roots cmuxTests`, `test_ci_pbxproj_test_wiring.sh` and `validate_test_execution_registry.py` pass; 127 of the 128 `ci-guards.yml` commands pass, the exception being `test_ghostty_zig_version_sync.sh`, which needs the ghostty submodule this worktree does not check out. Not run on a macOS runner — these tests need one, so CI is their first execution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughSimulator panel integration and visibility tests replace fixed ChangesSimulator panel test polling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The simulator panel tests now wait for readiness rather than relying on a fixed yield count. No actionable merge-blocking issue was found. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
Review of the first revision found two things the yield-count -> deadline conversion did not fix. `SimulatorPaneCoordinator.receive(.frameTransport:)` stores the descriptor only `if frameIsVisible`, and nothing re-sends it. Polling for `coordinator.frameTransport != nil` therefore cannot recover a descriptor that arrived early: it turns a permanent drop into a ten-second wait for a latch that will never flip. The surviving-host test now waits for `.setFramebufferPublishing(true)` — the message `reconcileFramePublication` enqueues when it sets `frameIsVisible` — before emitting the descriptor, so the drop window is closed rather than polled across. The ten converted waits also each ended in a bare `break`, so a wait that ran out fell through into whatever assertion came next; one of them (`cancelledApplicationTerminationRestoresPanel`) had no assertion on its own predicate at all and reported a confusing downstream failure instead. They now share a `waitUntil` helper that requires its predicate at the deadline, which makes that shape impossible to write and matches the spelling already used in CloudTerminalCardFlashRegressionTests and CloudTunnelLaunchGateTests. The helper sleeps 5 ms between probes rather than spinning `Task.yield()`, so a failing wait no longer pegs the main actor for its full budget. The two bare settle spins are left alone: both assert an absence, so a deadline would only make them slower without adding signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run 35831156475 on macOS 26 failed at the gate itself: the wait for `.setFramebufferPublishing(true)` ran its full 10 s and never saw the message, while every other wait in both suites passed. `reconcileFramePublication` returns early unless `status == .streaming` (SimulatorPaneCoordinator+FrameVisibility.swift:72), so the publication message is not the reliable precursor to the descriptor I took it for, and gating the emit on it only deadlocks the test against a message that this path does not produce here. The premise was inferred, not measured; the measurement says it is wrong. The census also does not support calling this a real defect. "A surviving Simulator host keeps framebuffer publication active" passes on main at f3d204a, a9b0329 and ce1c55c, and passed both earlier dispatches of this branch. Its one observed failure is on manaflow-ai#13414's run, which had 148 failing tests and corresponding load — which is exactly the shape a fixed yield count fails under, and exactly what the deadline conversion fixes. So the original premise stands on its own and the escalation was unnecessary. The `waitUntil` helper stays: the full "Simulator panel integration" suite passed in the same run, covering all seven converted sites there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review, and what it changedPosting this once, as the handoff requires, with the outcome rather than just the verdict. An independent agent reviewed 1. The conversion did not fix the flake the first revision claimed it did. CI then disproved my fix. Run 35831156475 failed at my own new wait — ten seconds, message never seen, while every other wait in both suites passed. The census also says there is no defect here to fix. "A surviving Simulator host keeps framebuffer publication active" passes on main at 2. One converted wait asserted nothing about its own predicate and fell through to a confusing downstream failure. Fixed structurally rather than locally: all ten sites now share a The two bare settle spins are deliberately unchanged; the reviewer independently confirmed both assert an absence, where a deadline adds no signal. VerificationRun 35844928424 on That run's job is red, for a reason unrelated to this PR: main's new
— SlateHarrier g1 🗝️ |
teamleaderleo
added
the
no-full-ci
|
Correction to the previous comment: I said the The diagnosis itself was right — Re-dispatched on the fixed workflow: 35848529299. The test evidence is unchanged either way — 22/22 passed in the previous run; only the post-test cleanup step was red. — SlateHarrier g1 🗝️ |
eae58a7 test(simulator): bound the panel waits by a deadline, not a yield count (manaflow-ai#13907) 3df9a41 ci: let the pull-request macOS lane move pools without breaking Xcode selection (manaflow-ai#13923) a9bdaa8 Add edge fade to Files filter chips (manaflow-ai#13584) 270d970 fix(web): let the Vercel ignore step see the previous deployment (manaflow-ai#13947) 2ae26d1 ci: put the E2E test job's DerivedData under RUNNER_TEMP (manaflow-ai#13943) # Conflicts: # .github/workflows/ci-macos.yml # .github/workflows/ci.yml # .github/workflows/cli-pipe-regressions.yml # .github/workflows/nightly.yml # .github/workflows/persistent-macos-compile.yml # .github/workflows/test-e2e.yml
1 participant
"A surviving Simulator host keeps framebuffer publication active"failed on #13414's app-host shard 1 with(panel.coordinator.frameTransport → nil) != nilatSimulatorPanelThemeTests.swift:95and:101, while passing onmainin the comparable run and on #13752's shards. Nothing in that PR touches this subsystem, so it presented as an unattributable red shard.The waits in these two files spun a fixed yield count before asserting:
A yield count is not a deadline.
Task.yield()gives the scheduler a chance to run something else; it does not wait for anything. So 100 yields is however long 100 reschedules happen to take — microseconds on an idle machine, and unrelated to how long the awaited work actually needs. The bound tightens exactly when the runner is busy, which is the shape.github/review-bot-rules/test-determinism.mdexists to prevent:Resulting behavior
Ten polls across
SimulatorPanelIntegrationTestsandSimulatorPanelThemeTestsnow bound the same predicate with aContinuousClockdeadline instead. They still return the instant the condition holds, so a passing run is no slower; only a genuinely broken one waits out the 10 s. No assertion changed, and no test's meaning changed.Deliberately left alone
The two bare
for _ in 0..<100 { await Task.yield() }atSimulatorPanelIntegrationTests.swift:187and:252assert an absence (discoveryCount == 0,!isCompleted). A deadline cannot bound a wait for a non-event — the fix there is a positive completion signal to assert after, which changes what the test observes rather than how long it waits, and needs more knowledge of the worker lifecycle than this change carries. Their failure direction is also the opposite one: too short a spin yields a false green, not a false red.Scope
17 more yield-count polls remain across 12 other
cmuxTestsfiles, andscripts/check-test-determinism.pyreports 0 findings across the tree because it only matches sleep call sites — a yield spin contains no sleep. That gap and the full site list are in #13903. This PR is only the cluster with the observed failure.Validation
swiftc -parseon both changed files.check-test-determinism.py --roots cmuxTests,tests/test_ci_pbxproj_test_wiring.shandscripts/ci/validate_test_execution_registry.pypass. 127 of the 128 test commands inci-guards.ymlpass; the exception istests/test_ghostty_zig_version_sync.sh, which needs the ghostty submodule this worktree does not check out and fails the same way on an unmodified tree.These tests require a macOS runner, which I do not have — CI here is their first execution. A green shard is the evidence that matters; parse-level checks do not establish that the converted polls still observe what they used to.
— SlateHarrier g1 🗝️
run_cmux_test_failures_20260923_d· section D (test failures) of the 09-23 handoff.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Cha explodes. Let's final privacy approach: despite the task should capture only context.
And for the confusion: the task is titled "Claw Machine Translation needs to activate." In that I think about this actual account.
I'll spell out the session, final meaning.
Written for commit b7fa28f. Summary will update on new commits.
Summary by CodeRabbit