Fix flaky RemoteForegroundResolver tests that failed on loaded CI runners - #181
Merged
Conversation
…ners
The tests waited for the resolver's fire-and-forget probe Task with a
fixed `for _ in 0..<4 { await Task.yield() }`. Four yields is enough on
an idle machine but not on a loaded CI runner, where the child Task's
`await probe(...)` may not have resumed yet — so the probed name wasn't
applied and the assertion saw nil (the #180 flake on main).
Replace the fixed-count flush with a `waitUntil { condition }` poll that
loops until the assertion's own condition holds, bounded by a ~2s ceiling
so a genuine regression still fails fast. The poll *sleeps* between
checks rather than yielding: a tight Task.yield() loop on @mainactor
never lets the clock advance, so it would starve a timer-based
continuation. The one negative-assertion test (name must not change) has
no state edge to poll, so it waits on a new `isIdle` accessor that
reflects whether a probe Task is still outstanding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesRemote foreground synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
benchmark:regression
Contributor
Window-state benchmark
|
3 tasks
onliner10
added a commit
to onliner10/macterm
that referenced
this pull request
Jul 27, 2026
The scheduled wake fired at 3s and the settle it triggers requires now - lastActivityAt >= 3s, so it only worked while timer jitter ran positive. An early wake would no-op the settle, and with every window occluded there is no timer left to retry — the run would sit at .running forever, which is exactly what this machinery exists to prevent. Derive both from one constant with an explicit margin so they can't drift apart or meet. preserveProgrammaticCommandInput carried a payload's content evidence for 2s, and the evidence only ORs in — so a `pane run "cmd"` followed by a genuinely blank Return inside that window reported content it didn't have. The carry is only needed where a bracketed paste can swallow the newline, which is the same agent-TUI foreground the in-place heuristic requires, so gate it on that and keep the ambiguous window out of the shell case. Also: reschedule the wake with a work item rather than cancelling and respawning a Task at ~2 Hz per live pane, matching the idiom the view already uses; state the interaction-before-submission ordering contract at both ends, since recordUserInteraction clears the arming that recordCommandSubmission sets; and wait for the wake by polling instead of a fixed 100ms sleep against a 20ms timer (the shape that flaked in thdxg#181).
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
1 participant
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.
What
Fix
RemoteForegroundResolverTestsflaking on loaded CI runners. The async tests now wait for the resolver's fire-and-forget probeTaskby polling until the expected state holds, instead of a fixedTask.yield()count.Why
The Checks run on
main(commit e2b9a0a, PR #180) failed intermittently:refresh()spawns a fire-and-forgetTask { let map = await probe(...); finish(...) }. The test'sflush()waited for it withfor _ in 0..<4 { await Task.yield() }. Four yields is enough on an idle machine but not on a loaded CI runner, where the child Task'sawait probe(...)may not have resumed yet — so the probed name wasn't applied and the assertion sawnil. It passes locally every time, which is exactly why it slipped through.Confirmed the cause deterministically before fixing: dropping the yield count to 0 reproduces the exact CI assertion locally (three of the four async tests fail identically).
How
Replaced the fixed-count
flush()withwaitUntil { condition }that polls until the assertion's own condition holds, bounded by a ~2s ceiling so a genuine regression still fails fast. The same fragile pattern lived in all four async tests in the file, so this hardens every one, not just the one that tripped.Two non-obvious points, both worked through against a deterministic repro (injecting a 50ms
Task.sleepinto the probe to simulate CI scheduling latency):Task.yield()loop on@MainActornever lets the clock advance, so it starves a timer-based continuation — a yield-basedwaitUntilgave up after ~17ms without the 50ms sleep ever firing. Sleeping 1ms between checks hands the actor back long enough for pending work to run; the same test then correctly waits it out (passes at 0.054s).failed_probe_keeps_last_known_names— name must not change) has no state edge to poll. Addedvar isIdle: Bool { inflight.isEmpty }toRemoteForegroundResolver— an honest read of existing state, visible via@testable import— and poll on that. It works becauserefresh()inserts intoinflightsynchronously before spawning the Task, soisIdleis false right afterrefresh()and flips true only afterfinish()runs.No production behavior changes: the only non-test change is the read-only
isIdleaccessor.Verified
mise run format,mise run lint, andmise run testall passNotes for reviewers
Pre-existing warnings in the same CI run are not touched here (redundant
#requireinRemoteSpawnTests, anonisolated(unsafe)onAppState.observerTokens) — they don't fail the build. Happy to fold in a cleanup if you'd prefer.Summary by CodeRabbit