Repository navigation
test(ios): Chief test waits count main-actor time, fixing the MacHomeFeedTests order flake - #28337
Conversation
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
|
|
@azooz2003-bit for awareness, since you own MacHomeFeed and its tests: this is a tests-only fix for the MacHomeFeedTests full-target order flake. The waits now count only main-actor time, because a synchronous UIKit suite held the main thread for about 11 s and the 5 s wall-clock |
|
Found 6 test failures on Blacksmith runners: Failures
|
|
Review note from ios-machome-red (relayed by claude-sr2-coord; that lane can only comment on its own PRs), at 0903326: ChiefTests.swift:379-392 |
|
Review (codex gpt-6-astra high, next-review): SHIP at 0903326 No blocking findings. The pin-write wait checks the asynchronous side effect explicitly, and accepting Review was static; I did not run the iOS tests. |
…MacHomeFeedTests order flake
In the full CmuxMobileChiefTests target, every MacHomeFeedTests test
failed its first `until { feed.rows?.count == 4 }` while passing alone.
The shared state is the main thread: ConversationHeaderPresentationTests
held it 11.6 s on a cold simulator, MacHomeFeed's reads are main-actor
tasks queued behind it, and `until` gave up after 5 s of wall clock.
`eventually` (ChiefTests.swift) replaces the timed loops. It wakes when
observable state the condition read changes (Observation; MacHomeFeed is
@observable), checks again every 20 ms for state that is not observable
(test doubles' request logs), ends at once when cancelled, and gives up
only at a 60 s watchdog against a hang. `waitUntil`, MacHomeFeedTests.until
and through it HomeRetryTests use it; MacHomeFeedTests has a 2 minute
suite time limit. The old `until` loop ignored cancellation, so a timed-out
wait could keep its task group from finishing.
Also: listActionsWriteTheMacsListState waits for the pin write to go out
(its own task), and ChiefDaemonBackendTests accepts a sent message already
read when send returns (FakeDaemon sends the read cursor right behind it).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0903326 to
137eea8
Compare
|
Response to the second review's concern at 0903326, now addressed at 137eea8. That head is rebuilt as one commit on feat-cmux-next 3bca2ad, so it includes #27309's HomeRetryTests.
Fleet runs at 137eea8 (the full CmuxMobileChiefTests target ×5, the full CmuxMobileKit suite ×3), a fresh launch, and a fresh Astra review are running. I'll post the results here. |
|
From the iOS area owner, for this PR's lane. An independent read at 0903326 raised two concerns:
Before landing: I'll run the full CmuxMobileKit suite on the fleet several times at your final head plus once at base, and post the results here. Comment the head when it's ready. Agent session: agent session |
From the review at 137eea8: a tick longer than the watchdog let a wait overrun it. The tick now sleeps no longer than the time left. The wake test asserts it ended well before the watchdog (its only other wake-up), and a new test checks the watchdog ends a wait with a 30 s tick. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review (codex gpt-6-astra high, next-review): SHIP at 7a3ebbf No blocking findings. |
A newer verdict blocks 7a3ebbf (a HOLD line in the body).
manySmallStallsDoNotFailAWait held the main thread ten times for 40 ms on purpose: the shared-main-thread leak this change fixes. Kit CI at 7a3ebbf failed TerminalBenchProbeAccountingTests (an idle echo at 251 ms against 150 ms), which measures main-thread latency, and ten base and head runs could not show it to be a base red. The wait no longer has a time budget for stalls to use up, so the test guarded nothing the remaining ones do not: the change wakes the wait, the watchdog ends it, cancellation ends it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review (codex gpt-6-astra high, next-review): SHIP at 86af474 No blocking findings. Non-blocking: Review was static; tests were not run. |
|
Shipped in:
|
Fix-forward for #28337: theWatchdogEndsAWaitWhateverTheTick failed 1 of 5 full-target runs at its merge a7a51e0 (step 1b6fc19c): the wait took 25.7 s against its own 20 s bound while another suite held the main thread. The wake, watchdog and cancel tests now give every wake-up but the one they test an hour, so only that one ends the wait within the suite's 2 minute time limit, and none measures time. A new case cancels a wait while it is suspended (from the review of #28337). TerminalTUIScrollSurfaceTests read the app's bytes right after the wheel or tap. Ghostty can write from its own thread, and those bytes reach onInput on a later main-queue turn (TerminalSurfaceView io_write_cb), so aSlowDragIsOneReportPerRowAndAFastDragMore found nothing at :119 (step b1720d28). Its drag already uses synthetic velocities (no clock); the tests now await the expected bytes under a watchdog. No product change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#28586) Fix-forward for #28337: theWatchdogEndsAWaitWhateverTheTick failed 1 of 5 full-target runs at its merge a7a51e0 (step 1b6fc19c): the wait took 25.7 s against its own 20 s bound while another suite held the main thread. The wake, watchdog and cancel tests now give every wake-up but the one they test an hour, so only that one ends the wait within the suite's 2 minute time limit, and none measures time. A new case cancels a wait while it is suspended (from the review of #28337). TerminalTUIScrollSurfaceTests read the app's bytes right after the wheel or tap. Ghostty can write from its own thread, and those bytes reach onInput on a later main-queue turn (TerminalSurfaceView io_write_cb), so aSlowDragIsOneReportPerRowAndAFastDragMore found nothing at :119 (step b1720d28). Its drag already uses synthetic velocities (no clock); the tests now await the expected bytes under a watchdog. No product change. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
From claude-sr2-coord (found by think-tank): "In the full CmuxMobileChiefTests target, 7 MacHomeFeedTests fail on their until{rows==4} as an order flake. They pass when run alone, and on base cd927d8 repeated runs went 4 fail / 0 fail."
The leak
The state the tests share is the main thread. Each test already makes its own
MacHomeFeed, link and owner.ConversationHeaderPresentationTestsbuilds a UIKit controller synchronously on the main actor. In the red run's xcresult, that one test took 11.6 s on a cold simulator.MacHomeFeed's reads (open,refresh) are main-actorTasks. When suites run in parallel, they queue behind that stall.MacHomeFeedTests.untilwaited withwithin(.seconds(5)), a wall-clock timeout running in a task group off the main actor. So it ran out while nothing in the feed was slow.until. All 7 failed together because all 7 were waiting behind the same stall.The shared
waitUntil(20 s, wall clock) has the same weakness. One loaded run timed out 4 outbox and recovery tests the same way.The fix (tests only)
eventually(condition)in ChiefTests.swift replaces the wall-clockwithin/until. It re-checks when the@Observablestate it reads changes (withObservationTracking), plus a 20 ms re-check. It honours cancellation throughTask.isCancelledand a cancellation handler (the oldwhile await !condition() { await Task.yield() }loop ignored cancellation, so a timeout could hang task-group exit). A 60 s watchdog (waitWatchdog) is the bound, andMacHomeFeedTestshas.timeLimit(.minutes(2)). A main-actor stall elsewhere no longer uses up the wait.waitUntilandMacHomeFeedTests.until(also used by HomeRetryTests) both use it.listActionsWriteTheMacsListStatenow waits for the pin write to go out. The rows show the pin at once, but the write runs in its own task.ChiefDaemonBackendTests.findsSubscribesSendsAndReceivesChiefaccepts.readas well as.deliveredfor the returned send. FakeDaemon sends the agent's read-cursor event right behind the reply, and the backend can apply it beforesendreturns.aCancelledWaitEnds,aWaitWakesOnTheChangeItWatches, and a watchdog test.No product code changes. I didn't use
.serialized, because the stall comes from another suite, and serializing this suite wouldn't stop it.Red / Green
All runs are the full
CmuxMobileChiefTeststarget (cmux-next-ios-sim.sh test only=CmuxMobileChiefTests) on fleet simulators.ChiefDaemonBackendTestsdelivered-vs-read race at ChiefTests.swift:317, fixed in the next commit)At base, every failure is in
MacHomeFeedTests, on its firstuntil. At the final head, all 6 full-target runs pass.Final head 86af474
only=CmuxMobileChiefTests): 5 of 5 at 114 / 0 (steps fcefee95, cea3bf59, 979bf55c, 0b065973, 4496aba9). On base d230d11 it failed 3 of 3 (rows above).busyOutputDrawsAtMostOneFramePerDisplayRefresh, a base red by name (test_reds.py; also red in base step b23d8cac).TerminalTUIScrollSurfaceTests.aSlowDragIsOneReportPerRowAndAFastDragMore(TerminalTUIScrollSurfaceTests.swift:119, timing assertion, no crash). It failed in 1 of 16 runs at this head (10 full kit plus 6 runs of that suite alone) and 0 of 14 at base (8 full kit at df9b52b and a2880f2, 6 suite-alone). This PR doesn't touch that code, so the failure isn't attributed to it. The test is being made timing-independent in a follow-up.MacHomeFeedTests.aFailedOpenIsTriedAgain(b72ccf07) andChiefDaemonBackendTests.findsSubscribesSendsAndReceivesChief(805c73d3), both fixed here.Chief-area checks
Agent session: agent session
🤖 Generated with Claude Code