[tools] Add a tool to launch processes disclaming responsibility. - #24249
Open
rolfbjarne wants to merge 16 commits into
Open
[tools] Add a tool to launch processes disclaming responsibility.#24249rolfbjarne wants to merge 16 commits into
rolfbjarne wants to merge 16 commits into
Conversation
TODO: improve description.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new spawner tool has correctness/robustness issues (missing required C headers and unguarded use of a non-public symbol) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a small native spawner utility to launch test processes while disclaiming macOS “responsible process” attribution, then routes macOS test app execution through it to avoid TCC privacy checks being evaluated against the launcher (e.g., an IDE host) instead of the test app.
Changes:
- Introduce
tools/spawner(native executable + docs + Makefile) and include it in thetoolsbuild. - Update xharness macOS execution (
MacExecuteTask) and dotnet test makefiles to launch apps viaspawner. - Ensure CI builds
spawneras part of test run dependencies.
File summaries
| File | Description |
|---|---|
| tools/spawner/spawner.c | New native launcher that uses posix_spawn* with responsibility disclaiming. |
| tools/spawner/Makefile | Builds per-arch spawner binaries and lipo’s into a universal spawner. |
| tools/spawner/README.md | Documents the macOS TCC “Responsible process” problem and intended mitigation. |
| tools/spawner/.gitignore | Ignores build outputs for the new tool. |
| tools/Makefile | Adds spawner to tool subdirs when Xcode is available. |
| tools/devops/automation/templates/tests/run-tests.yml | Builds tools/spawner in the test pipeline dependency step. |
| tests/xharness/Jenkins/TestTasks/MacExecuteTask.cs | Switches macOS test execution to go through Harness.UseSpawner. |
| tests/xharness/Harness.cs | Adds SpawnerPath + UseSpawner helper to rewrite ProcessStartInfo to launch via spawner. |
| tests/common/shared-dotnet.mk | Routes run-bare through $(SPAWNER) instead of executing the app directly. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 6
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- spawner.c: include the correct standard headers (errno.h, stdbool.h, string.h) instead of the unused dispatch/dlfcn headers, fixing implicit-declaration compile errors for bool/errno_t/strerror. - Harness.UseSpawner: also reject a non-empty ArgumentList, and fail fast with a clear error if the spawner executable is missing. - README.md: fix PID in the Electron example and correct a typo (reponsibility -> responsibility). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2f4c5e94-acb7-47c3-9555-8f952effb493
This comment has been minimized.
This comment has been minimized.
The spawner's Makefile relied on the generic per-RID compilation rules
from mk/rules.mk (.libs/osx-arm64/spawner.o etc.). Those rules are only
generated for the platforms that are enabled, via DOTNET_RUNTIME_IDENTIFIERS.
Most test jobs in CI run ./configure --disable-all-platforms --enable-<platform>
for a single non-macOS platform, which leaves INCLUDE_MAC empty. In that
configuration the osx-* RIDs don't exist, so no rule is generated for
.libs/osx-arm64/spawner.o and the build fails with:
make: *** No rule to make target '.libs/osx-arm64/spawner.o',
needed by '.libs/osx-arm64/spawner'. Stop.
This is why the 'Generate / compile dependencies' step failed for the
ios/tvos/maccatalyst jobs but succeeded for the macos ones.
The spawner is a host tool that always runs on the macOS build machine and
is needed no matter which platforms are enabled, so define its own
compilation rules keyed off the architecture instead, and query the macOS
SDK path directly rather than using $(macos_SDK) (which is likewise only
meaningful when macOS is enabled).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2f4c5e94-acb7-47c3-9555-8f952effb493
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The entire job's timeout is 3h.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… in CI. Harness.GetIncludeSystemPermissionTests() had a switch over TestPlatform that only handled iOS and Mac explicitly. Mac Catalyst fell through to the 'default' branch, which assumes any non-device platform is a simulator whose TCC database can be freely pre-seeded. Mac Catalyst apps run natively on the host Mac (there's no Mac Catalyst simulator), so that assumption doesn't hold, and DISABLE_SYSTEM_PERMISSION_TESTS was never set for Mac Catalyst test runs. This caused monotouch-test's Contacts.ContactStoreTest to run for real (instead of being ignored), which hits a still-NotDetermined TCC authorization status now that the new 'spawner' tool disclaims process responsibility (so the child app is now checked on its own TCC status instead of inheriting an already-approved responsible process). With no interactive session available to answer the resulting system permission dialog, the test process hangs until it's killed after the 20 minute timeout, exactly matching the CI hangs seen on #24249 for the monotouch_maccatalyst job (and by extension monotouch_macos, whose overall job timeout was also exceeded as a result). Add TestPlatform.MacCatalyst next to TestPlatform.Mac in the switch so Mac Catalyst gets the same '!InCI' treatment as native macOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…etSkipBackupAttribute. Across every one of the 12 monotouch-test macOS configurations in CI run 15213209 (PR #24249), the test suite hung 100% reproducibly right after NSFileManagerTest.GetHomeDirectoryTest, which is exactly where GetSkipBackupAttribute runs next (alphabetically). lldb backtraces of the hung process show the main thread stuck in a getxattr syscall, which is how macOS's sandbox/TCC subsystem checks extended attributes (e.g. com.apple.macl) when a process is granted or denied access to a protected user folder. GetSkipBackupAttribute writes a scratch file into the user's Documents directory (NSSearchPathDirectory.DocumentDirectory), which is one of the folders macOS protects behind a 'Files and Folders' TCC prompt. Now that tests are launched through the new spawner tool (which disclaims process responsibility so the test app is evaluated on its own TCC status instead of inheriting an already-approved ancestor's), the ephemeral test app has no existing grant for this folder, so the write triggers a fresh authorization prompt that hangs forever with no interactive session available to answer it, exactly matching the observed CI hangs. Use NSFileManager.TemporaryDirectory instead, which isn't behind a TCC prompt (and is already the established convention for scratch files elsewhere in this test suite, e.g. AudioConverterTest.cs and FSEventStreamTest.cs). The test only needs a writable file to exercise GetSkipBackupAttribute/SetSkipBackupAttribute, not specifically a file in the Documents directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ConverterTest. CI run 15215315 (PR #24249) showed the exact same class of hang as the previous fix (ccc70fb), just one test further down the alphabetical list: after the Documents-directory write in FileManagerTest.GetSkipBackupAttribute was fixed, macOS/Mac Catalyst runs progressed further into Foundation.UrlTest, then hung again with the same getxattr-on-main-thread signature right after UrlTest.Invalid_29510 - i.e. at UrlTest.IsExcludedFromBackupKey, the next test alphabetically. IsExcludedFromBackupKey also writes a scratch file into the user's Documents directory (NSSearchPathDirectory.DocumentDirectory) before manipulating NSUrl.IsExcludedFromBackupKey on it. Now that tests run through the new spawner tool (which disclaims process responsibility, so the ephemeral test app is checked on its own TCC status instead of inheriting an already-approved ancestor's), this write triggers a fresh 'Files and Folders' authorization prompt that hangs forever with no interactive session to answer it. Fix it the same way as the previous commit: use NSFileManager.TemporaryDirectory instead, which isn't behind a TCC prompt and is already the established convention for scratch files elsewhere in this test suite. While auditing the rest of the test suite for the same anti-pattern (a search for NSSearchPathDirectory.DocumentDirectory across tests/monotouch-test), also fix AudioConverterTest.ConvertWithPacketDependencies, which wrote its output audio file into the Documents directory instead of using the DoWithTemporaryDirectory helper that its sibling tests in the same file (Convert, CreateWithOptions) already use for exactly this reason. This wasn't yet confirmed as an observed CI hang (it likely wasn't reached before the UrlTest hang), but it has the identical risk and would very likely have been the next hang once UrlTest was fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ca98fc3c-135b-4bad-80dc-50262be55c90
Collaborator
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
Collaborator
🔥 [CI Build #bb4002f] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 1 tests failed, 239 tests passed. Failures❌ monotouch tests (MacCatalyst)# Test run in progress: Building: 1, BuildQueued: 11, Running: 1, Ignored: 342, TimedOut: 12Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a new
spawnertool and wires test execution through it so launched test apps disclaim process responsibility, avoiding macOS TCC attribution issues where a parent/responsible process (for example, an IDE host) is checked for usage-description keys instead of the test app itself.Why
Some macOS privacy checks (TCC) can attribute responsibility to the launcher process instead of the child app, producing false failures that claim missing
Info.plistusage-description keys even when the test app includes them. Launching throughspawnerdisclaims responsibility so the app is evaluated against its own metadata.