From beb24133bbef41f2db4c609eb15cf6c2e8a9fae6 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Thu, 24 Sep 2026 17:12:11 -0700 Subject: [PATCH] fix(macos): resolve #283 guest exclusions with hosted-Mac evidence Hosted Intel and Apple Silicon runners pass every test the Recovery guest excluded as a macOS finding (exact-SHA run 36067778414), so what remains excluded is guest-only: - cap-primitives st_rdev panic: the guest reports negative device numbers; wait for bytecodealliance/cap-std#428 rather than [patch]. - ENOTSUP from the guest filesystem's atomic renames, now also covering install_directory's RENAME_SWAP (documented to return the fs error). - one two-core timing window. Re-enabled in the guest: the PTY restore and owner-bound child tests (fixed in #347) and the containment-state test, which passes there now. owned_group_termination_reaches_the_grandchild failed intermittently in the guest because Child::wait closes stdin first, letting the root read EOF and exit 0 before the SIGKILL landed. Hold stdin across a bounded wait and assert the root died by SIGKILL. Co-Authored-By: Claude Opus 5.5 --- ci/macos-x64/README.md | 35 ++++++++------------ ci/macos-x64/recovery-guest.sh | 46 +++++++++++---------------- ci/test_macos_x64_guest.py | 4 +-- tests/process/process_host_control.rs | 26 ++++++++++++++- 4 files changed, 58 insertions(+), 53 deletions(-) diff --git a/ci/macos-x64/README.md b/ci/macos-x64/README.md index 384fa748..11285ccd 100644 --- a/ci/macos-x64/README.md +++ b/ci/macos-x64/README.md @@ -75,7 +75,7 @@ the stub never has to stand in for real source. constant. They assert on source text that is identical on every host and ci.yml's Linux `test-run` lanes already run them. This mirrors the exclusion the aarch64 lane carries in `ci.yml`. -- **18 further tests**, excluded by name and grouped by cause in +- **16 further tests**, excluded by name and grouped by cause in `recovery-guest.sh`. Each entry is named rather than pattern-matched, so the list stays reviewable, and each group carries the evidence observed in the guest. None of it is a blanket filter: a test that starts failing for a *new* @@ -83,27 +83,18 @@ the stub never has to stand in for real source. | Group | N | Why it cannot pass here | |---|---|---| -| `EXCLUDE_CAP_PRIMITIVES` | 12 | `cap-primitives` 4.0.3 panics converting a negative macOS `st_rdev` — `u64::try_from(stat.st_rdev).unwrap()` at `metadata_ext.rs:171`. The `dev` field two lines above guards the same signedness, so `dev_t` is known-signed here and only `rdev` was missed. Surfaces as `TryFromIntError(())`. No fixed 4.x exists. | -| `EXCLUDE_MACOS_RENAME` | 2 | The readiness marker's no-clobber publish returns `ENOTSUP` (45). `tempfile` asks for `renameatx_np(RENAME_EXCL)`, which real macOS supports, so this most likely reflects the guest's virtualized filesystem rather than macOS. Test-only: `persist_noclobber` has no production caller. | -| `EXCLUDE_TTY` | 1 | A Recovery guest gives the script no controlling terminal to save and restore. | -| `EXCLUDE_VM_TIMING` | 2 | Two cores in a VM are not representative for wall-clock assertions; a suspension window and a containment deadline elapsed before the work did. | -| `EXCLUDE_CONTAINMENT_STATE` | 1 | **Investigate, not an artifact.** `real_worker_sequential_stress_leaves_no_parent_state` observed `ForcedContainment { trigger: Cancelled }` where it expects `Stopped(Cancelled)`. That is a state mismatch rather than an elapsed deadline, so it may be a genuine macOS containment difference. Excluded to keep the lane green while it is investigated (#283). | - -**Three macOS portability findings this lane surfaced have since been fixed in -the tests themselves** and are no longer excluded: the TLS fixture's 3650-day -validity (now 820 days, under macOS's 825-day ceiling), the `/var` → -`/private/var` canonicalization comparison in `context_file_observation`, and -the invalid-UTF-8 file name in `tree_hash`, which APFS rejects with `EILSEQ` -before the hash can ever see it. All three stayed invisible only because every -macOS *test* lane in this repository is gated off — this lane is the first to -run the suite there. - -**The `EXCLUDE_MACOS_RENAME` group is not in that category.** `tempfile`'s -`persist_noclobber` asks for `renameatx_np(RENAME_EXCL)`, which real macOS -supports, so the `ENOTSUP` is most likely a property of the guest's virtualized -filesystem rather than of macOS. It is excluded as a guest artifact pending a -check on real hardware (issue #283). **`EXCLUDE_CAP_PRIMITIVES` is an upstream -bug**, not a test defect. +| `EXCLUDE_CAP_PRIMITIVES` | 12 | `cap-primitives` 4.0.3 panics converting a negative `st_rdev` — `u64::try_from(stat.st_rdev).unwrap()` at `metadata_ext.rs:171`. The `dev` field two lines above guards the same signedness; only `rdev` was missed. This guest reports negative device numbers; hosted Macs do not, and all 12 pass there. Upstream fix: bytecodealliance/cap-std#428 (we wait rather than `[patch]`, which would not reach our consumers). | +| `EXCLUDE_MACOS_RENAME` | 3 | The guest's filesystem returns `ENOTSUP` (45) for `renameatx_np(RENAME_EXCL)` (readiness marker) and `renamex_np(RENAME_SWAP)` (`install_directory`, which documents returning the filesystem's error when exchange is unsupported). Real APFS supports both, and all three pass on hosted Macs. | +| `EXCLUDE_VM_TIMING` | 1 | Two cores in a VM are not representative for wall-clock assertions; a suspension window elapsed before the work did. | + +**Every macOS portability finding this lane surfaced is resolved** (#283). The +TLS fixture validity, the `/var` → `/private/var` comparison, and the +invalid-UTF-8 name in `tree_hash` were fixed in the tests. The owner-bound +child, verified-control, and PTY-restore failures were real and were fixed in +#347. Everything still excluded is a property of this guest, not of macOS: +each group passes on hosted Intel and Apple Silicon runners with no exclusions +(exact-SHA full CI run +[36067778414](https://github.com/zackees/kernal-api/actions/runs/36067778414)). ## Coverage assertion diff --git a/ci/macos-x64/recovery-guest.sh b/ci/macos-x64/recovery-guest.sh index 8bd6f8a2..bd806e53 100644 --- a/ci/macos-x64/recovery-guest.sh +++ b/ci/macos-x64/recovery-guest.sh @@ -44,10 +44,11 @@ POLICY_FILTER='not binary(source_policy)' # validity (now 820, under macOS's 825-day ceiling), the `/var` -> `/private/var` # canonicalization comparison in context_file_observation, and the invalid-UTF-8 # file name in tree_hash, which APFS rejects with EILSEQ before the hash can see -# it. The remaining entries are the upstream cap-primitives panic, one rename -# that the guest's filesystem does not support, Recovery environment artifacts -# (no tty, two-core timing), and one containment-state mismatch that is -# still under investigation rather than explained. See issue #283. +# it. The remaining entries are all properties of this guest, not of macOS: +# each passes on hosted Intel and Apple Silicon runners with no exclusions +# (exact-SHA full CI run 36067778414, #283). They are the upstream +# cap-primitives panic on the guest's negative device numbers, one rename +# flag the guest's filesystem lacks, and two-core VM timing. # --------------------------------------------------------------------------- # cap-primitives 4.0.3 panics converting a negative macOS st_rdev: @@ -55,33 +56,24 @@ POLICY_FILTER='not binary(source_policy)' # cap-primitives-4.0.3/src/rustix/fs/metadata_ext.rs:171, surfacing as # `TryFromIntError(())`. The `dev` field two lines above guards the same # signedness, so `dev_t` is known-signed here and only `rdev` was missed. -# APFS device numbers in this guest are negative; there is no fixed 4.x release. +# APFS device numbers in this guest are negative; hosted Macs report +# non-negative ones and these tests pass there. The fix is upstream +# (bytecodealliance/cap-std#428); we wait for it rather than fork, because a +# [patch] would not reach kernal-api's own consumers anyway. EXCLUDE_CAP_PRIMITIVES='commit_error_cleans_staging_after_destination_parent_is_renamed completed_symlink_is_not_published_or_followed missing_completed_output_preserves_destination_and_cleans_staging parent_discard_cleans_staging_after_destination_parent_is_renamed parent_discard_removes_worker_partial_and_completed_files replacement_failure_preserves_existing_directory_and_cleans_staging staged_output_is_invisible_until_parent_commit cancellation_after_parent_sync_preserves_output deadline_after_parent_sync_preserves_output_and_reports_cleanup_failure dispatcher_retry_releases_ownership_and_records_one_forced_reap output_cleanup_failure_reports_whether_publication_occurred parent_output_discards_on_failure_or_stop_and_commits_only_success' -# The readiness marker's no-clobber publish returns ENOTSUP (45) in this guest. -# `tempfile` asks for renameatx_np(RENAME_EXCL), which APFS and HFS+ support, so -# this most likely reflects the guest's virtualized filesystem rather than -# macOS. Not yet verified on real hardware (#283). Test-only: persist_noclobber -# has no production caller. -EXCLUDE_MACOS_RENAME='failed_marker_write_is_cleaned_up_and_existing_marker_is_preserved marker_is_invisible_until_payload_is_complete' - -# A Recovery guest gives this script no controlling terminal to save and -# restore, so the termios flags it compares are not the ones it set. -EXCLUDE_TTY='native_session_rejects_overlap_and_restores_mode' +# The guest's filesystem returns ENOTSUP (45) for the atomic rename flags: +# `tempfile`'s renameatx_np(RENAME_EXCL) in the readiness marker's no-clobber +# publish, and renamex_np(RENAME_SWAP) in install_directory, which documents +# returning the filesystem's error when exchange is unsupported. Real APFS +# supports both; all three tests pass on hosted Intel and Apple Silicon runners. +EXCLUDE_MACOS_RENAME='failed_marker_write_is_cleaned_up_and_existing_marker_is_preserved marker_is_invisible_until_payload_is_complete install_directory_replaces_an_existing_tree_and_removes_the_old_one' # Two cores in a VM are not representative for wall-clock assertions: a -# suspension window and a containment deadline elapsed before the work did. -EXCLUDE_VM_TIMING='a_handful_of_threads_fills_a_small_ring_long_before_the_window_ends a_child_bound_to_another_owner_dies_when_that_owner_does' - -# NOT a known artifact -- investigate. The guest observed -# `ForcedContainment { trigger: Cancelled }` where the test expects -# `Stopped(Cancelled)`: containment was reached by force rather than by a -# graceful stop. That is a state mismatch, not an elapsed deadline, so it may be -# a genuine macOS containment difference. Excluded only to keep the lane green -# while it is investigated (#283). -EXCLUDE_CONTAINMENT_STATE='real_worker_sequential_stress_leaves_no_parent_state' - -EXCLUDED_TESTS="$EXCLUDE_CAP_PRIMITIVES $EXCLUDE_MACOS_RENAME $EXCLUDE_TTY $EXCLUDE_VM_TIMING $EXCLUDE_CONTAINMENT_STATE" +# suspension window elapsed before the work did. +EXCLUDE_VM_TIMING='a_handful_of_threads_fills_a_small_ring_long_before_the_window_ends' + +EXCLUDED_TESTS="$EXCLUDE_CAP_PRIMITIVES $EXCLUDE_MACOS_RENAME $EXCLUDE_VM_TIMING" GUEST_EXCLUDE='' for name in $EXCLUDED_TESTS; do diff --git a/ci/test_macos_x64_guest.py b/ci/test_macos_x64_guest.py index 24ffa18e..b9d320b1 100644 --- a/ci/test_macos_x64_guest.py +++ b/ci/test_macos_x64_guest.py @@ -180,9 +180,7 @@ def test_every_exclusion_group_is_present_and_non_empty(self): expected = { "EXCLUDE_CAP_PRIMITIVES", "EXCLUDE_MACOS_RENAME", - "EXCLUDE_TTY", "EXCLUDE_VM_TIMING", - "EXCLUDE_CONTAINMENT_STATE", } self.assertEqual(set(groups), expected, "exclusion groups changed") for name, entries in groups.items(): @@ -195,7 +193,7 @@ def test_exclusions_are_named_and_unique(self): groups = self.exclusion_groups() names = [n for entries in groups.values() for n in entries] self.assertEqual(len(names), len(set(names)), "duplicate exclusion entries") - self.assertEqual(len(names), 18, "the documented exclusion count changed") + self.assertEqual(len(names), 16, "the documented exclusion count changed") def test_every_exclusion_is_applied_to_the_guest_filter(self): """Declaring a group but not using it would silently re-enable tests.""" diff --git a/tests/process/process_host_control.rs b/tests/process/process_host_control.rs index 89c68ee7..4220cd57 100644 --- a/tests/process/process_host_control.rs +++ b/tests/process/process_host_control.rs @@ -135,7 +135,31 @@ fn owned_group_termination_reaches_the_grandchild() { return; } result.expect("group kill"); - let status = root.wait().expect("reap root"); + // Keep the root's stdin open: `Child::wait` closes it first, and a root + // that read that EOF before the signal landed would exit 0 on its own, + // passing or failing for the wrong reason (#283). + let stdin = root.stdin.take(); + let deadline = Instant::now() + Duration::from_secs(10); + let status = loop { + if let Some(status) = root.try_wait().expect("poll root") { + break status; + } + assert!( + Instant::now() < deadline, + "the group kill never reached the root" + ); + std::thread::sleep(Duration::from_millis(20)); + }; + drop(stdin); + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt; + assert_eq!( + status.signal(), + Some(libc::SIGKILL), + "root was killed: {status:?}" + ); + } assert!(!status.success(), "root was killed"); assert!( gone_within(grandchild, Duration::from_secs(5)),