Repository navigation
fix(macos): attach to a running app instead of launching a duplicate on open - #3707
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The code change in 746cfa8 is not ready yet. The new probe fails the eager-closure check, and the live evidence for the changed Coverage fails in The change touches the device-facing macOS Could the probe live in its own os/macos module that Not blocking, and fine to take or leave: the doc block at host-provider.ts:125 says the 5 s budget is longer than the On the open inline threads, three still apply: the CHANGELOG overstates the wait, the first launch pays an uncounted I did not run the tests or any live macOS run. I also did not verify that The next step is to move the probe behind the lazy import so eager-closure-budgets passes. Then post the live transcript on that final head, showing one pid with the caller's arguments where base shows two. |
3122789 to
1a0d1b0
Compare
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The earlier concern about the static import is fixed at 1a0d1b0: the lazy import removes that edge, and the stale CHANGELOG and docs wording is corrected. The code now looks right to me, but the live evidence asked for earlier is still missing. The change affects the real macOS Not blocking, take or leave: the doc block on On the open threads, I checked each earlier thread against 1a0d1b0. The three P2 threads on the CHANGELOG count and wording, on the docs wording and on the Smoke Tests failed in "Preflight iOS runner through public CLI" with |
1a0d1b0 to
15c8c96
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Live head-vs-base evidence on the public route (fixture Base full CLI The seam-level comparison ( Probe cost and the budget question from review (measured, same host):
Reproduction notes: the window must be asserted unlisted before dispatch — with warm LaunchServices state, |
96b43c5 to
b6fbc2e
Compare
|
Final-head live CLI proof at Gate at this head: |
|
The earlier fixes in 1a0d1b0 now hold at 42228df, and the code looks right. I did not run the tests or a live macOS open, so the author's live transcripts are checked only against the route and the production-identical trees by diff. The cold-open and close-then-open timings come from 15c8c96 (production-identical to this head), not from a run at 42228df. The test fixes are in: the real-plutil stub now registers only for the paired pid, and a 3-plist batch covers the multi-file plutil output, so the two inline threads can be resolved: #3707 (comment) (stub pairing, fixed at head) and #3707 (comment) (multi-file plutil rows, benign because readAppBundleIds refuses mismatched counts). Should the process-table read go through the same injected AppleToolCommandExecutor as the lsappinfo, plutil and open calls, using listHostProcesses({ runCommand, timeoutMs })? Not blocking: the The macOS Smoke Tests job is still queued, so CI has no result yet. It runs the macOS command coverage contract, which may reach the changed macOS openBundle route, so a failure there would not be clearly unrelated. Required evidence is satisfied. There are no conflicts. Before merge, Smoke Tests must finish green, and I will check the macOS command coverage contract lane when it does. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…on open A macOS app started directly — by a supervisor or a dev script — is not listed by LaunchServices until half a second or so after it spawns. During that window `open -b` sees no instance and starts a second copy with no arguments, which the session then drives instead of the copy the caller owns (#3406). The bundle open now reads the same view `open -b` consults and, when it lists nothing while a process of that bundle is running, waits until LaunchServices names the running copy before running `open` unchanged. An already-listed bundle and a bundle with nothing running reach `open` on the first read; the wait, every read, and the process scan share one budget, and anything the probe cannot answer fails open so an open is never withheld.
The probe's 5 s ceiling exceeded the healthy open it precedes by an order of magnitude while the case it waits out registers within ~0.75 s. The ceiling now covers the full poll plus the reads ahead of it with measured room (reads cost well under 300 ms together on a healthy host), so a host too loaded to answer within the wait is dropped at the remaining-budget floor instead of holding the open for seconds the race never needs. The batched bundle-identifier read gains a hermetic invariant against the real plutil: one -extract answers for every plist it is named, in argument order, so one call can pair every candidate bundle; the unit fakes cannot carry that proof because they script the rows.
The registration stub answered any pid-pinned read, so a batch that paired an identifier with the wrong bundle's process could still end the wait green. Only pid 4242's query now reports an ASN and the test asserts the exact read transcript, so a rotation of the row-to-path pairing (verified by a disposable mutation: 3 failed, 17 passed) fails this test on the target's pid rather than passing on another bundle's.
The final review caught that the comment in the routing test named host-provider-open.test.ts, which does not exist. The probe assertions live in launch-services-registration.test.ts, the file the comment should route a reader to.
The probe ran lsappinfo, plutil and the open through the injected Apple
tool executor but read ps on the daemon's own host through the host-kit
default. A scoped Apple provider could then pair pids found on one host
with its lsappinfo and plutil on another. listHostProcesses({runCommand,
timeoutMs}) routes the scan through the same seam and deadline the other
reads already share, so the probe answers about one host end to end. The
owning tests now script ps through fakeMacHost instead of mocking the
process module.
… read fakeMacHost recorded the ps call and returned the table before consulting the responder, so the scan could not be made to throw, fail, or time out even though the probe now runs it through the same executor seam the Error-answering stub exists for. The responder is consulted first and the table is only the default answer. Three owning scenarios pin what the probe does when the scan itself fails: a tool that cannot spawn and a read killed at its budget end the probe with a typed diagnostic and the original open, and a non-zero ps exit is the unreadable table the empty answer already was — never 'nothing runs'.
b857831 to
ee2a12e
Compare
|
Rebased onto Gate at this head: |
CI blocker at
|
|
I reviewed ee2a12e and found no code problems. The earlier fixes are in: the Smoke Tests failed in "Preflight iOS runner through public CLI" with For evidence, I did not run the live macOS transcript or the unit suites at ee2a12e myself. The 2-vs-1 copy result and the snapshot and deep-link checks come from your comment (#3707 (comment)). Two small notes that need no change here. The timeout test uses a hand-written |
thymikee
added
the
ready-for-human
1 participant
Summary
Part of #3406. macOS
openchecks LaunchServices registration when an app-shaped process is already running, preserving the selected instance and arguments. The 2500 ms probe remains best-effort; unconfirmed registration retains normalopenbehavior and may duplicate. Process-table reads use the same provider executor as the other tools.Validation
Head:
ee2a12ef551548e133bb78c5ef72104240c4db3e.run-YjbGPzpassed with exit 0,exactCommit: trueand matching clean fingerprints. Fresh isolated r9 review found no findings, reproduced 973 tests, and killed restored process-error, plist-pairing and budget-guard mutations.prepare ios-runner: daemon startup exceeded 15000 ms, the child was gracefully retired, and no owner metadata appeared. Native runner compilation passed; no smoke scenario or XCTest assertion ran. Daemon log empty; cause unestablished; owner investigating startup and transitive imports. No CI retry taken.CI: 17 successes, one failure, one skip and one neutral; macOS Smoke passed. The same preflight signature hit
cac1247ac(#3345) and9c7d2602cin the same window while9203a7a40reported success at 17:50:12Z — evidence pinned in comment 6221775611; cause unestablished, not classified as infra. The42228dfb6masked-field assertion is separate and historical; this preflight failure does not establish whether it passes here — atb85783148the suite ran 117/0 with it passing.