Skip to content

CI: serialize Swift Testing inside app-host shards - #9717

Closed
lawrencecchen wants to merge 15 commits into
mainfrom
fix-ci-app-host-parallelism
Closed

lawrencecchen wants to merge 15 commits into
mainfrom
fix-ci-app-host-parallelism

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • disable in-process Swift Testing parallelism in the shared app-host xcodebuild wrapper
  • keep four app-host shards running on separate Macs
  • run a focused parameterized behavior test before every broad shard
  • exclude that focused policy test from ordinary shard allocation

Why

App-host tests share process-global AppKit, account, socket, and store state. Xcode's in-process concurrency let unrelated tests overlap inside one host and produced nondeterministic crashes and hangs. Job-level environment variables and scheme variables were disproven by real CI runs. The supported xcodebuild -parallel-testing-enabled NO option controls the test runner directly.

This is a principled CI ownership fix. One test host runs one case at a time, while independent machines retain shard-level throughput.

Validation

  • Exact head: fb589e7765c4a2cf86b3c1c2b01854e2993a2cb2
  • red control without the supported option: both parameterized cases overlapped and the focused test failed
  • strict determinism scan: zero findings
  • workflow routing, app-host retry, attempt-limit, sharder, and self-hosted guards: passed
  • canonical $autoreview: clean, confidence 0.9
  • CodeRabbit: success
  • exact speculative merge run: https://github.com/manaflow-ai/cmux/actions/runs/31128817575

Merge state

The exact speculative merge run is in progress. This PR changes CI only, so it does not require a tagged runtime build.

Dictionary

  • App-host test: A test loaded into the cmux application process, where UI and account state is shared.
  • Shard: One independent subset of app-host tests assigned to a separate CI machine.
  • Speculative merge: A temporary commit combining the PR head with current main before required checks run.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The shared cmux-unit scheme limits Swift Testing to one worker. Regression coverage parses the scheme and checks that the app-host job does not define equivalent environment variables.

Changes

App-host CI serialization

Layer / File(s) Summary
Configure and verify shard serialization
cmux.xcodeproj/xcshareddata/xcschemes/cmux-unit.xcscheme, tests/test_ci_change_areas.py
The shared scheme sets SWT_EXPERIMENTAL_MAXIMUM_PARALLELIZATION_WIDTH to 1 and documents shared process-global test state. The regression test parses the scheme and rejects equivalent job-level configuration.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: azooz2003-bit, austinywang

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The PR range changes only tests/test_ci_change_areas.py; it contains no production Swift changes, so the Swift actor-isolation check does not apply.
Cmux Swift Blocking Runtime ✅ Passed The PR diff changes only tests/test_ci_change_areas.py and contains no Swift files; it introduces no blocking or timing primitive in production Swift code.
Cmux Browser Automation Off-Main ✅ Passed The PR changes only the Xcode scheme and CI regression tests; no browser/WebKit/socket automation files or policy-sensitive routing lines changed.
Cmux Expensive Synchronous Load ✅ Passed The complete PR diff changes only the Xcode scheme and a Python test; it adds no production Swift or expensive synchronous agent-history load.
Cmux Cache Substitution Correctness ✅ Passed The PR changes only an Xcode scheme and Python CI tests; it contains no production Swift, TypeScript, or JavaScript cache substitution.
Cmux No Hacky Sleeps ✅ Passed The PR changes only Python test coverage and an Xcode scheme XML file; it adds no covered TypeScript, JavaScript, shell, or non-Swift runtime delay.
Cmux Algorithmic Complexity ✅ Passed The diff changes only CI configuration, a static Xcode scheme, and test-only Python. It adds no production collection-processing path or scalable algorithm covered by the complexity rule.
Cmux Swift Concurrency ✅ Passed The PR changes only an Xcode scheme and Python regression tests; it adds no Swift source or legacy async, Combine, completion-handler, or fire-and-forget Task patterns.
Cmux Swift @Concurrent ✅ Passed The complete PR diff changes only an Xcode scheme and a Python test; it adds no Swift files, async declarations, @concurrent annotations, or changed Swift call sites.
Cmux Swift Package Boundaries ✅ Passed The PR-wide diff contains only a Python regression test and Xcode scheme XML; it introduces no production Swift source or SwiftPM boundary change.
Cmux Swiftpm Lockfiles ✅ Passed The PR diff contains only an Xcode scheme environment change and tests; no Package.swift, Package.resolved, .gitignore, workflow, or Xcode package-reference change requires a lockfile update.
Cmux Swift Logging ✅ Passed The PR changes only the Xcode scheme and Python CI tests; the diff contains no Swift files or added production logging statements.
Cmux User-Facing Error Privacy ✅ Passed The diff only changes CI/test code and a shared Xcode test-scheme setting/comment; it adds no production user-facing error, alert, command output, or recovery text.
Cmux Full Internationalization ✅ Passed The diff only adds CI test code, a developer comment, and an Xcode test-scheme environment token; it adds no user-facing text, localization keys, catalogs, or web messages.
Cmux Swiftui State Layout ✅ Passed The PR changes only an Xcode scheme and Python CI tests; it adds no Swift or SwiftUI code, so the SwiftUI state/layout rules are not triggered.
Cmux Architecture Rethink ✅ Passed The PR changes only a shared Xcode test-scheme setting and a Python guard; it adds no Swift source or prohibited lifecycle constructs. The serialization is test-only synchronization.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR diff changes only one Xcode scheme and one Python test file; it adds no Swift source or auxiliary window code, so the close-shortcut rule is not applicable.
Cmux Source Artifacts ✅ Passed The PR changes only a tracked Xcode test scheme and a hand-written Python regression test; neither path is a local, generated, cache, build, download, or scratch artifact.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The complete PR diff changes only one Python test file and one Xcode scheme; it contains no Swift files or edits under production Sources paths.
Cmux No Ambient Global State ✅ Passed The full diff changes only an Xcode scheme and a Python test; it adds no production Swift code or ambient global state.
Title check ✅ Passed The title clearly identifies the main change: serializing Swift Testing within app-host shards.
Description check ✅ Passed The description explains the change, rationale, validation, merge state, and key terms; it omits the template checklist and review-trigger block.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-ci-app-host-parallelism

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_ci_change_areas.py`:
- Around line 730-734: Replace the raw substring assertion in the relevant test
with XML parsing scoped to the scheme’s TestAction/EnvironmentVariables node.
Assert that exactly one enabled SWT_EXPERIMENTAL_MAXIMUM_PARALLELIZATION_WIDTH
entry exists there and that its value is 1, ignoring comments and entries from
other actions.
- Around line 723-725: Update
test_app_host_tests_configure_swift_testing_serialization_in_the_xcode_scheme so
it inspects the complete workflow_job_block("app-host-unit-tests") rather than
truncating at "steps:". Parse or match the job-level env mapping with
indentation awareness, ensuring forbidden variables are detected even when env
appears after steps while ignoring step-level environments.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 30ed336e-9dca-4b99-9a99-405889bb1718

📥 Commits

Reviewing files that changed from the base of the PR and between 3715188 and 4af0402.

📒 Files selected for processing (2)
  • cmux.xcodeproj/xcshareddata/xcschemes/cmux-unit.xcscheme
  • tests/test_ci_change_areas.py

Comment thread tests/test_ci_change_areas.py Outdated
Comment thread tests/test_ci_change_areas.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_ci_change_areas.py`:
- Line 12: Update the XML parser import used by the test around ET.parse to
defusedxml.ElementTree, preserving the existing scheme parsing behavior; if that
parser cannot be used, add a narrowly scoped S314 suppression at ET.parse with
the trusted checked-in scheme rationale, without changing global Ruff
configuration.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b52c0ee1-eeee-4dc8-bccd-6ed8440805da

📥 Commits

Reviewing files that changed from the base of the PR and between 4af0402 and 6f9094a.

📒 Files selected for processing (1)
  • tests/test_ci_change_areas.py

Comment thread tests/test_ci_change_areas.py
@cursor

cursor Bot commented Aug 6, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Closing because the exact speculative merge disproved the approach: disabling Xcode parallel testing collapsed each shard into one long-lived app host and exposed process-order dependence instead of stabilizing the suite. Shard 2 reported 127 failures and shard 1 reported 55 failures in https://github.com/manaflow-ai/cmux/actions/runs/31128817575. The failures span account state, shell configuration, session persistence, hibernation, and WebKit lifecycle, so the focused two-case probe was insufficient. I am keeping process isolation and validating the Sentry stack against the existing sharded runner instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant