Repository navigation
test: fix two app-host flakes (starved process waits, fixed-delay key wait) - #14056
teamleaderleo wants to merge 3 commits into
Conversation
24 test helpers waited for a child process by queueing a blocking waitUntilExit() on DispatchQueue.global() and waiting on a semaphore with a timeout. Every queued waiter holds a pool thread until its child exits. Waiters for children that never exit accumulate across a batch; once the pool is exhausted a new waiter never starts, and a child that exited normally is reported as a timeout. testAgentTurnDiffBaselineStoresUntrackedSnapshotsOutsideGit hit this: it passes in ~0.6 s in eight of the nine runs examined and, in one, a read-only `git for-each-ref` reported status 124 after 30 s with empty stderr. cd7d25c fixed the same failure in one Claude hook helper. waitForProcessExit(_:timeout:) polls isRunning on the calling thread and returns DispatchTimeoutResult, so each `sem.wait(timeout: .now() + t)` becomes `waitForProcessExit(process, timeout: t)` with its surrounding logic unchanged. No test in cmuxTests queues a waitUntilExit() anymore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
added
the
full-ci
|
Warning Review limit reachedNext included review available in 33 seconds. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (20)
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 ✍️ ✅ |
testSenderRelativeSidebarActionKeysItsOriginatingWindowBeforeMutation gave AppKit one fixed 50 ms run-loop spin to move key status after makeKeyAndOrderFront. On a loaded runner the change can take several turns, so the same code passed 3 times and failed 6 across full-suite runs. Wait for the transition with a 5 s deadline instead. Also normalize project.pbxproj ordering for ProcessExitWait.swift. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
changed the title
teamleaderleo
added
no-full-ci
|
Status for 6b6c0be:
Once this run finishes, I'll report the three targets here: — Yorick g1 🍂 |
|
Independent diagnosis of Status of this PR's current run: shards 2/3/4/6 are red. Shard 3 is the icon test (#14062). The others include — Ibex g1 🌿 |
Tracing every name a changed file declares, transitively, reached 798 suites for a one-helper edit, because nearly every file names a suite that names another. And a second review found two holes: suites used as helper holders (MachineCreateCoordinatorTests.newMachineRequest) were never traced, and files marked as added skipped the search. scripts/ci/test_impact.py now reads `git diff -U0` and starts only from the declarations whose lines changed. Each place that names a changed helper marks the declaration around it: a test method ends the trail at its suite, a helper continues it. A member of a type is searched only in files that also name that type; a member an extension adds to another type is searched everywhere, since values call it without naming the type. A change inside a conformance, or an init, subscript or operator an extension adds to another type, has no name to search for and still runs every suite. Every changed file is searched, added or not, so the --added-from plumbing is gone. On the last 40 main commits that touched cmuxTests/, the median run is 2 suites (p90 6), and 35 fit one worker. manaflow-ai#14056 selects 24 suites and its one strict step; manaflow-ai#14062 selects 1 suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Results for 6b6c0be, run 35940971858 (7 shards; the diff alone selects the unit suite under #14017):
— Yorick g1 🍂 |
* ci: run only the edited suites when a diff edits cmuxTests/ Since #14017 a cmuxTests/ diff selects `app-host unit tests` by itself, and that job runs every suite on seven workers. Editing one test file cost the whole suite, and the only label that ran the edited tests ran everything else too. choose_ci_suite.py now also emits unit_selectors: the suites the changed cmuxTests/ files declare or extend. When set, the app-host job runs one worker (shard 8, which owns no strict step) with exactly those suites. It widens back to every suite when the answer could be incomplete: an unreadable diff, a non-Swift file, an existing helper that declares no suite (it can change any suite), or a set whose measured time exceeds ten minutes serial. A helper the diff adds is the exception, since only files this diff changes can call it. `unit-ci` and `full-ci` still run every suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: widen a changed-suites run for shared helpers and strict suites Review of the first cut found three ways it could run too little: - A suite file that also declares a helper other files can see (47 of them, e.g. BundledCLITestSupport, used by 54 files) narrowed to its own suite. Any non-private top-level declaration other than a suite or suite extension now counts as a helper and widens to every suite. - A moved helper appeared as added under --no-renames and was treated as new. The added list now detects renames. - A suite a strict step owns ran in the shared batch, without the app host of its own the step gives it. Selecting one now widens. Changed-suites runs also upload the test inventory, so they can be replayed offline like shard 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: keep changed-suites runs narrow for helpers and strict suites Widening to all seven workers was the lazy answer to both cases the review found: - A changed helper affects only the files that use it. Find them by name, follow them transitively, and run their suites. Extensions of other types are traced through the member names they add; only a conformance, init, subscript or operator, which have no name to search for, still widens. Editing BundledCLITestSupport now runs its 83 dependent suites. - A suite a strict step owns needs that step's app host and settings, not seven workers. The single worker now runs the owning step, named in unit_strict_steps and derived from the workflow itself, and leaves the suite out of its shared batch. In a sample of 150 cmuxTests/ files edited alone, 145 now narrow, with a median of one suite. The home-isolation guard now rejects only a top-level `||` in the Cloud ordering gate; a parenthesized worker choice keeps both gates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: trace a test diff through the declarations it changed Tracing every name a changed file declares, transitively, reached 798 suites for a one-helper edit, because nearly every file names a suite that names another. And a second review found two holes: suites used as helper holders (MachineCreateCoordinatorTests.newMachineRequest) were never traced, and files marked as added skipped the search. scripts/ci/test_impact.py now reads `git diff -U0` and starts only from the declarations whose lines changed. Each place that names a changed helper marks the declaration around it: a test method ends the trail at its suite, a helper continues it. A member of a type is searched only in files that also name that type; a member an extension adds to another type is searched everywhere, since values call it without naming the type. A change inside a conformance, or an init, subscript or operator an extension adds to another type, has no name to search for and still runs every suite. Every changed file is searched, added or not, so the --added-from plumbing is gone. On the last 40 main commits that touched cmuxTests/, the median run is 2 suites (p90 6), and 35 fit one worker. #14056 selects 24 suites and its one strict step; #14062 selects 1 suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: close the tracer gaps a third review reproduced - A helper type's changed member now traces the type, not the member name. A mock app code calls (AgentChatResumeIntentRecorder.record) reaches every suite that builds one, and an instance a factory returns (VaultPaneTestDrag via beginVaultDrag) reaches its callers through the factory, which names the type. - Only a suite's hooks and test methods count as runner-only; a helper type's tearDown() is traced like any helper. - An attribute, directive or doc comment line also marks the declaration below it, so `@MainActor` or `#if` edits are credited to what they modify. - A non-suite declaration that holds tests (nested Swift Testing suites in a container) cannot be named as selectors, so an edit there runs every suite. - An import or other file-level line changes the whole file. Both real reproductions from the review now select the missed suites. On the last 40 cmuxTests/ commits on main, 33 still fit one worker (median 2 suites, p90 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
24 test helpers in
cmuxTests/, across 17 files, waited for a child process the same way: they queued a blockingwaitUntilExit()onDispatchQueue.global()and waited on a semaphore with a timeout. Each queued waiter holds a pool thread until its child exits. Waiters for children that never exit pile up over a test batch. Once the pool runs out of threads, a new waiter never starts, and a child that exited normally is reported as timed out.This is the flake in
CMUXOpenCommandTests/testAgentTurnDiffBaselineStoresUntrackedSnapshotsOutsideGit():35849325644, a read-onlygit for-each-refcame back as status 124 (timed out) after 30 s, with empty stderr.cd7d25c13cfixed the same failure in one Claude-hook helper; this PR fixes the other 24.waitForProcessExit(_:timeout:)incmuxTests/ProcessExitWait.swiftpollsisRunningon the calling thread, so it never waits on a pool thread. It returnsDispatchTimeoutResult, so eachsem.wait(timeout: .now() + t)becomeswaitForProcessExit(process, timeout: t)and the code around it stays the same. After this change no helper incmuxTests/queues awaitUntilExit()on a dispatch queue.Also: the key-window flake in
testSenderRelativeSidebarActionKeysItsOriginatingWindowBeforeMutationAppDelegateShortcutRoutingTestsgave AppKit one fixed 50 ms run-loop spin to move key status aftermakeKeyAndOrderFront. On a loaded runner that change can take several turns. The same test passed in 3 and failed in 6 of the full-suite runs I checked, on both Blacksmith and Warp. It now waits for the change with the file's existingwaitForCondition(timeout: 5).Testing
Reproduced the failure mode on Linux Foundation (Swift 6.1.3), with the helper compiled into a small driver. With the global pool pinned by 200 blocked blocks, a child that exits in 0.1 s gave:
waitUntilExit()+ semaphoretimedOutafter 5 swaitForProcessExitsuccessin 0.11 sNormal cases on the same driver:
successin 0.20 s.timedOut.terminate(), the wait returnedsuccess.Checks:
scripts/check-pbxproj.shpasses. The first push failed "Fast static checks" on entry ordering, fixed inbe5fa50.swiftc -parsepasses on all 18 changed Swift files.TimeIntervalparameter or a numeric literal.scripts/lint-pbxproj-test-wiring.shpasses.test_ghostty_zig_version_sync.shneeds the ghostty submodule, andtest_ci_app_host_xcodebuild_retry.shfails under parallel load and passes on its own.Not yet verified: that it compiles in the app-host target and the affected suites pass. This PR's
full-cirun checks both. One green run can't prove the flake is gone; it happened once in nine runs.Checklist
🤖 Generated with Claude Code