fix(ios): let a legacy untagged Mac row take pushed presence routes - #14012
Merged
Merged
Conversation
performPushedRouteSyncBatch has looked up the stored row by the exact pairing id (device + presence tag) since 5aedb51. A row paired before instance tags existed is untagged, and presence always reports a tag, so the lookup never found it. applyPushedRoutes was handed nil and never wrote. Its untagged branch, reconnectRouteAuthority(pairedMacInstanceTag: nil), which lets a legacy row adopt its device's sole route-advertising build, had become unreachable. A legacy Mac that moved stopped getting fresh routes over presence. Fall back to the device's untagged row when there is no exact tagged match. applyPushedRoutes still requires the presence instance to be the device's only route-advertising build, and the upsert is still conditioned on the row's own (nil) tag, so a Mac with two builds online is not written. presenceRoutesForHiddenDuplicateRefreshOnlyTheEmittingRow covers this. On main it awaits pairedStore.waitUntilUpsertCount(1) forever, which is what hangs the serial CmuxMobileShell package run until the job timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe pushed-route sync batch now falls back to an untagged legacy pairing when it cannot find a paired Mac for the primary pairing ID. Route application receives the primary paired Mac when available. ChangesLegacy pairing route sync
Estimated code review effort: 2 (Simple) | ~8 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The legacy pairing fallback retains the existing route-authority check. No actionable issue remains before normal merge checks. 🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, scope, and test results. However, it omits the template's Demo Video, Review Trigger, and Checklist sections, including the required deterministic soak coverage status for this iOS connectivity change.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
Collaborator
Author
|
Full serial package run on air-blue (Xcode 27.0), with this PR plus #14003, #14004 and #14005 applied on 19 tests still fail in that run. I reran all 19 with — Thimble g1 🔆 |
rustybret
pushed a commit
to rustybret/bmux
that referenced
this pull request
Sep 23, 2026
ccdbf30 fix(ios): let a legacy untagged Mac row take pushed presence routes (manaflow-ai#14012) 6332063 test(ios): give pool alias fixtures one build identity (manaflow-ai#14004) 82d49a3 fix(ios): keep an offline sibling build out of secondary aggregation (manaflow-ai#14005) 19a6ac1 test: read errno before the assertion that can overwrite it (manaflow-ai#13957) ef98b5e ci: name the macOS 26 runner variables after machines, not lanes (manaflow-ai#14009) 9697411 test(ios): expect the construction focus event in composer diagnostics (manaflow-ai#14003) ad7f9fe ci: run app-host unit tests for a cmuxTests/ diff without a label (manaflow-ai#14017) # Conflicts: # .github/workflows/ci-health-report.yml # .github/workflows/ci-macos.yml # .github/workflows/ci.yml # .github/workflows/nightly.yml
This was referenced Sep 23, 2026
Collaborator
Author
|
Checked under CI's Xcode 26.6: all four fixes (#14003, #14004, #14005, #14012) merged onto main
With the fixes, the suite finished all 1303 tests in 386 s. Main hung and was killed at the 25-minute step timeout, so its job shows "cancelled". 21 tests still fail with the fixes. 4 of them also fail on main here: — Nyan g1 🗝️ |
teamleaderleo
added a commit
that referenced
this pull request
Sep 25, 2026
Probes older than #14012 hang in the serial CmuxMobileShell run, so tests after the hang never execute. Counting their silence as a pass made 14 tests look broken by the commit that fixed the hang. - Record passes as well as failures; a test that never ran at a probe is unknown (`-`), and a probe with no "Test run with" summary is INCOMPLETE. - `start --patch <sha>` applies a known fix to every probe, and `--bisect <name>` keeps that experiment beside the first. - Cache finished job logs, so `status --refetch` re-parses without spending the shared REST budget, and skip a run the API refuses instead of aborting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 25, 2026
Probes older than #14012 hang in the serial CmuxMobileShell run, so tests after the hang never execute. Counting their silence as a pass made 14 tests look broken by the commit that fixed the hang. - Record passes as well as failures; a test that never ran at a probe is unknown (`-`), and a probe with no "Test run with" summary is INCOMPLETE. - `start --patch <sha>` applies a known fix to every probe, and `--bisect <name>` keeps that experiment beside the first. - Cache finished job logs, so `status --refetch` re-parses without spending the shared REST budget, and skip a run the API refuses instead of aborting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 25, 2026
Probes older than #14012 hang in the serial CmuxMobileShell run, so tests after the hang never execute. Counting their silence as a pass made 14 tests look broken by the commit that fixed the hang. - Record passes as well as failures; a test that never ran at a probe is unknown (`-`), and a probe with no "Test run with" summary is INCOMPLETE. - `start --patch <sha>` applies a known fix to every probe, and `--bisect <name>` keeps that experiment beside the first. - Cache finished job logs, so `status --refetch` re-parses without spending the shared REST budget, and skip a run the API refuses instead of aborting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 25, 2026
…14533) * ci: bisect package test failures across main's history, with a skill PR CI runs selected package tests, so the full CmuxMobileShell suite drifted red on main without anyone noticing. Finding which PR broke each test meant hand-building overlay branches (old commits carry CI scripts that no longer run), dispatching test-ios.yml, scraping logs and diffing failure sets. scripts/ci/package_bisect.py does that loop: `start` pushes probe branches (an old commit's tree with today's iOS CI files and no lint gate) and dispatches the package suite on runner `auto`; `status` prints a per-test matrix with break, fix and flaky verdicts; `next` dispatches midpoints that split each break window; `adopt` counts existing runs; `cleanup` deletes the branches. The cmux-test-bisect skill covers when to use it, how to read the matrix, and how to decide stale test vs regression. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect counts only tests that ran, and can patch probes Probes older than #14012 hang in the serial CmuxMobileShell run, so tests after the hang never execute. Counting their silence as a pass made 14 tests look broken by the commit that fixed the hang. - Record passes as well as failures; a test that never ran at a probe is unknown (`-`), and a probe with no "Test run with" summary is INCOMPLETE. - `start --patch <sha>` applies a known fix to every probe, and `--bisect <name>` keeps that experiment beside the first. - Cache finished job logs, so `status --refetch` re-parses without spending the shared REST budget, and skip a run the API refuses instead of aborting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect review fixes - Cache job logs per run attempt, and look the attempt up first, so a rerun of a compile-failed probe is read fresh instead of the cached failure. - Read a cancelled job's log: a job timeout reports as cancelled and still carries partial results. Only a skipped or never-started job is an error. - Save state after every dispatch and merge probes other invocations added, written atomically, so a failed dispatch or a long `status --wait` cannot orphan branches. - Check range endpoints against first-parent history, and space `--points` probes evenly instead of rounding the step down. - `next` steps past a midpoint that answered nothing for the test instead of reporting the window closed. - `adopt` keeps an existing probe's branch; `cleanup` keeps state when a branch deletion fails; `start --force` names the branches it leaves. - "broken by" only names a watched commit; counts say "watched commits". - Skill: #14510 for app-host bisects, branch naming, INCOMPLETE meaning, and `--patch` needs its own `--bisect` name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect waits on pending windows and keeps the newest probe Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: fix pending-window check in package bisect Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: run the package bisect tests in workflow-guard-tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect next --ways N probes a window N ways per round Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect probe subcommand adds chosen commits Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: package bisect review fixes - SKILL.md and the docstring put --package/--bisect before the subcommand; the old 'start --bisect ...' form fails in argparse. - cleanup deletes only the probe branches still on the remote, so a rerun after a partial cleanup finishes. - dispatch looks the run up when 'gh workflow run' prints no URL, instead of leaving a probe pending forever. - adopt checks the run's head and keeps the adopted run over a newer one. - start rejects a reversed GOOD..BAD range; drop_lint_gate exits cleanly when the job is missing. - workflow_guard_groups routes package_bisect.py to the ci guard group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
A Mac paired before instance tags existed has an untagged stored row, and presence always reports a tag. Since 5aedb51,
performPushedRouteSyncBatchlooks up the stored row by the exact pairing id (device + presence tag), so it never finds that row. As a result, a legacy Mac that changes address never gets the new routes pushed over presence. This PR falls back to the device's untagged row when no exact tagged row exists.The receiving side was already written for this case. For an untagged row,
applyPushedRoutescallsreconnectRouteAuthority(pairedMacInstanceTag: nil), which accepts only the device's sole route-advertising build. The upsert is still conditioned on.matchingInstanceTag(nil). So when two builds on one Mac both advertise routes, nothing is written;presenceRoutesDoNotFanOutWhenLogicalDuplicatesBothAdvertiseRoutescovers that and still passes.This is the
mobile-core-packagehangMobileShellCompositePairedMacCoalescingTests.presenceRoutesForHiddenDuplicateRefreshOnlyTheEmittingRow()covers this path. On main it awaitspairedStore.waitUntilUpsertCount(1), which has no bound, and the upsert never comes. The run below reproduced it.db5d212d86, running this one test alone hangs until killed (timeout 150exit 124). A serial full-package run parked on it at 0% CPU; asampleshowed every thread idle inmach_msg/workq.MobileShellAltScreenNoticeTests.sameActiveScreenRenderGridDoesNotNotifyAlternateScreenObservers()started, and the job then hits the 25-minute timeout. That line is a flush boundary, not the hung test. The job's output arrived in about 64 KB blocks, and the alt-screen test passed in both serial runs on air-blue. Suite order is the same on both machines, and this test runs a few suites later. I could not rerun the job with unbuffered output, so the claim that CI hung on this same test rests on this reasoning, not on a CI run.Testing
MobileShellCompositePairedMacCoalescingTestspass, including the formerly hanging test (0.03 s).waitUntilUpsertCountshould probably have a deadline so a regression here fails the test instead of the job. I left that out to keep this PR to the product fix.— Thimble g1 🔆
Run: run_ios_shell_mobile_tests_20260923_e464e5ba
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes pushed presence route sync for legacy Macs paired before instance tags existed.
applyPushedRoutesstill restricts untagged rows to the device's sole route-advertising build, so a Mac with two builds online is not written.presenceRoutesForHiddenDuplicateRefreshOnlyTheEmittingRownow passes; the hang was an unbounded wait for an upsert that never arrived.Written for commit 7dc36c2. Summary will update on new commits.
Summary by CodeRabbit