test(desktop): make PTT HAL liveness deterministic - #10725
test(desktop): make PTT HAL liveness deterministic#10725tianmind-studio wants to merge 3 commits into
Conversation
Fixes BasedHardware#10724. Wait until the injected HAL probe is actually wedged, then require three main-actor snapshot reads to complete. This preserves the liveness contract without treating macOS runner throughput in a fixed 300 ms window as correctness. Tests: desktop/macos/scripts/check_desktop_test_quality.py (macOS Swift CI required; this Windows host has no Xcode toolchain). Failure-Class: none
|
Authoritative macOS verification is green on head |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the focused fix here. I reviewed the diff and the current head CI evidence.
Positive signal from my side: the test rewrite is a good direction. Waiting for the injected HAL probe to actually enter the wedged path removes the old race, and replacing scheduler-throughput thresholds with explicit main-actor and off-main snapshot-read completions better matches the production contract this regression is supposed to pin. I also like that the parked probe is still released and joined before teardown.
The changelog-gate update also looks narrowly scoped: Desktop/Tests/ is the real SwiftPM test location and the added unit coverage keeps that exemption from leaking into product source paths.
I’m not formally approving from automation because this touches CI/changelog policy code and the approval gate requires human maintainer sign-off for workflow-sensitive paths, even though this looks clean and the relevant Desktop Swift CI run is green.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
Superseded by canonical-repo PR #10742 (same fix rebased onto current main). |
|
Hey @tianmind-studio 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
Fixes #10724.
What changed and why
> 10timer/read throughput requirements in a 300 ms window with three explicit main-actor heartbeat completions and three explicitcurrentSnapshotcompletionsdesktop/macos/Desktop/Tests/, from the user-facing changelog gate and cover that path classification with regression casestestMainRunLoopKeepsPumpingWhileTheHALIsWedgedwas testing a binary liveness contract with host-throughput thresholds. Under macOS runner contention, the main actor and snapshot path still made progress, but the isolated suite completed too few iterations. The production contract is that the main actor and turn-start snapshot reads both keep completing while the HAL probe remains blocked off-main, so the new test observes those two completions independently instead of measuring scheduler throughput.The changelog gate already exempts internal test paths because its post-merge push run cannot see a PR-only
no-changelog-neededlabel. It covered the legacy lowercasedesktop/macos/tests/directory but not the Swift package tests underDesktop/Tests/, so this test-only patch exposed the same open failure class on the actual Swift test path.Failure evidence
main: run 30276084659,reads == 10ticker.count == 5,reads == 4ticker.count == 7,reads == 7All sibling
PTTInputDeviceProbeTestscases passed in those failures.Product invariants affected
none
How it was verified
python3 .github/scripts/test_desktop_changelog.pypassed: 4 testspython3 .github/scripts/check-desktop-changelog.py --base origin/main --head HEADpassed: no desktop changes require a changelog entrypython3 desktop/macos/scripts/check_desktop_test_quality.pypassedgit diff --check origin/main...HEADpassedstartedis fulfilled immediately beforerelease.wait(), main-actor heartbeat and snapshot-read expectations settle beforerelease.signal(), andfinishedis awaited afterwardThis contribution was prepared on Windows, so the local commit hook could not run the Xcode-only
swift-formatcommand (xcrunis unavailable). Desktop Swift CI is the authoritative compile, formatter, and XCTest verification for the Swift test change.Tests
ChangelogRequirementTestsnow covers both a top-level and nested Swift package test path.Failure class (fixes)
Failure-Class: FC-push-gate-internal-path-scope
Scoped cleanups
The changelog path exemption is a separate commit because the initial test-only head demonstrated that relying on
no-changelog-neededwould pass only the PR lane and could still redden the post-merge push lane.