Repository navigation
fix(native): keep UI fixture ownership through a missing handshake - #8646
kaoru0822-kitauji wants to merge 9 commits into
Conversation
LaunchServices starts the fixture outside the launcher's process tree, and the verifier learned its PID only from the ready record. A missing handshake therefore stopped `open`, removed the fixture root and left the app running, reparented to PID 1. Own the launch from before it starts: the run token reaches the app through the launch environment, so the shared sweep finds it with or without a ready record. Stop the launcher through its retained handle, retire the application path so an in-flight launch cannot start, sweep, and only then remove the root. A refused cleanup retains the root and run for retry. The ready PID must prove the token before it is used, and an interrupted verifier releases its fixture. Fixes #8559 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kaoru0822-kitauji
added
bug
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d9021913c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fixture = await scenario.started(); | ||
| verifier.child.kill(signal); | ||
| expect((await verifier.exit).code, verifier.stderr()).toBe(exitCode); | ||
| expect(await isLive(fixture.pid)).toBe(false); | ||
| expect(await verifier.roots()).toEqual([]); |
There was a problem hiding this comment.
Both signal tests interrupt the handshake, before final cleanup begins. They do not exercise a signal arriving while withVerifierCleanup is already awaiting fixture.close(), when the new signal handler joins that release and schedules process.exit(). Add a regression that gates an in-progress release, delivers the signal, and verifies that exit waits for cleanup settlement, including retained-root diagnostics when cleanup fails. This finalization coverage is explicitly required for lifecycle changes; handshake cancellation does not substitute for it.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was a code defect as well as missing coverage. Fixed in f2e0525; the branch head is f9160aa, which merges latest main.
Root cause. The signal handlers in verify-native-ui.mjs raced the cleanup they were meant to protect:
process.onceremoved the handler after the first signal, so a repeated signal took its default action and killed the process mid-release. That orphans the fixture and leaves the root. It is an ordinary case, sincenpmforwarding Ctrl-C can deliver the same signal twice.- The handler's own
process.exitbeat the main path, so a single signal during cleanup exited 143 instead of keeping an existing failure's 1.
Exit did wait for a joined release, and a failed release did keep its root and retry ability, so those parts were already right.
Structural fix. runNativeUiFixtureLane now settles the process outcome in one place, and the verifier runs inside it:
- A signal only records the interruption and ends the operation's race. The handler no longer exits the process.
- The release always runs to settlement through
withVerifierCleanup, and every signal stays handled until it settles.SIGKILLremains the override. - The first failure selects the status, and every failure is reported. A signal after a failed operation keeps exit 1 and the primary failure. A signal after a completed operation keeps the report on stdout and exits 130 or 143. If the release then fails, the report is lost and the retained root and run are reported instead, still with 130 or 143.
Validation.
- New tests park the real release at a barrier inside its retire step, then deliver the signals, with both successful and failed release outcomes. They cover repeated and mixed signals, a hung operation, and joined closes followed by a retry.
- This PR's first handler fails the 4 held-release tests on the actual verifier: a repeated signal left
signalCodeSIGTERMorSIGINT, and a single signal gave 143 instead of 1. - Mutations also fail:
process.once(6), exit from the listener (11), dropping the late status (2), losing primary precedence (4), no forced exit (5). test:focusedon the new file and 6 sibling files: 493 passed, in each of 2 runs after merging main.check:fast,format:checkanddocs:checkpass.- Real
/usr/bin/openand LaunchServices with the release held:SIGTERM,SIGTERMandSIGINTleft the verifier alive with its root present. After the gate opened it exited 1 with the primary failure, and no root or fixture remained.
Remaining.
- The barrier sits at the retire step only. Signals during launcher stop, sweep or root removal reach the same settlement point but are not separately gated.
- A release that hangs can now only be ended with
SIGKILL. Its steps have their own bounds (source-derived); that bound was not tested. - The real lane's success path through the new wrapper is unobserved, because a pre-existing
window-mismatchstopped all 3 attempts since the change. It also stopped all 3 runs on unmodified main. An authored operation covers that path in the tests. Details are in the PR body.
Leaving this thread open for review.
The verifier's signal handlers raced its own cleanup. `process.once` left a repeated signal to its default action, which killed the process mid-release and orphaned the fixture; npm forwarding Ctrl-C makes that repeat ordinary. The handler's own `process.exit` also beat the main path, so a signal during cleanup replaced an existing failure with an interrupted status. Settle the process outcome in one place. A signal now only records the interruption and ends the operation's race, the release always runs to settlement, and every signal stays handled until then. The first failure selects the status and every failure is reported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 [Low impact] The reviewed changes appear safe to merge, with all three previous findings addressed. SummaryThe verifier now owns its fixture before launch and keeps the root until cleanup succeeds. The latest change also brings both Swift compiles under that owner.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Create fixture owner and root] --> B[Compile through owner]
B --> C[Launch with run token]
C --> D[Check ready PID and run token]
D --> E[Run verifier checks]
B --> F[Close owner]
C --> F
E --> F
F --> G[Abort and join admitted work]
G --> H[Stop launcher]
H --> I[Retire application path]
I --> J[Sweep token-owned processes]
J --> K{All cleanup steps succeeded?}
K -->|Yes| L[Remove root]
K -->|No| M[Report failure and retain root for retry]
Reviews (6) · Last reviewed commit: "fix(native): stop and join verifier comp..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The application path was gone while the root still existed. | ||
| expect( | ||
| (await scenario.events()).find(({ kind }) => kind === "launch-refused"), | ||
| ).toMatchObject({ rootPresent: true }); |
There was a problem hiding this comment.
The rootPresent: true assertion relies on the launch-service process running between application retirement and root removal. Its 2 ms timer does not guarantee that. If that process is delayed, owner.close() can finish the sweep and remove the root before the timer runs, so correct cleanup fails this test.
Add an explicit acknowledgment before allowing root removal, or check the ordering at a held retirement step.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/boundary/process/nativeUiVerifierFixtureOwnership.test.ts
Line: 415-418
Comment:
**Correct cleanup can fail CI**
The `rootPresent: true` assertion relies on the launch-service process running between application retirement and root removal. Its 2 ms timer does not guarantee that. If that process is delayed, `owner.close()` can finish the sweep and remove the root before the timer runs, so correct cleanup fails this test.
Add an explicit acknowledgment before allowing root removal, or check the ordering at a held retirement step.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid; this was a flake in a test I added, and it reproduced. With the stand-in launch service's poll stretched from 2 ms to 2 s, the old test failed on rootPresent even though cleanup was correct. Fixed in 48bb585; the branch head is 9a8733e, which merges latest main.
Root cause. The test read the ordering from a separate process polling on a timer. Nothing kept the root in place until that process had looked, so its scheduling decided the assertion.
Fix.
- The test now runs in a child process with a barrier in the real release, just before root removal. Removal stays held until the test has seen the launch refused. At that point it checks directly that the app path is gone, its
.retiredpath exists and the root is present, then opens the gate. - The barrier is a preload wrapper around
fs/promises.rm, in the same form as the retire barrier. The release itself is not mocked. - Moving to a child process exposed a second dependency: the 300 ms handshake budget could stop the launcher before it had started the launch service. The operation now waits until the launch is admitted, then fails, so release cannot start early.
Others of the same kind in this file and the lifecycle module.
- Removed the elapsed-time bound in the launcher-refusal test. Its exit message already proves the exit path rather than the deadline path.
- Raised the stand-ins' 25 s safety expiries to 60 s, because they were shorter than the 30 to 40 s test timeouts and a slow but correct run could lose a fixture the test expects alive.
- Raised the launcher diagnostics grace from 1 s to 5 s, so a stalled event loop cannot drop stderr from a startup failure. This is the one production constant changed, and it only applies once a launcher has exited before the handshake.
- Left alone: the
delay(150)between signals and theoutlived-rootpoll, because lateness there can only miss a defect and cannot fail correct cleanup. The 300 ms budgets in the module-level tests are also fine, because those fixtures never answer and the tests wait for the fixture before closing.
Validation.
- With the same 2 s poll, the new test passes.
- Removing the retire step fails it with
Timed out waiting for the in-flight launch to lose its application. It also fails the 8 tests whose barrier sits on that step. - The test file ran 5 consecutive times with 10 busy loops on this 10-core host (load average up to about 19): 34 passed each time.
- After merging main: the file plus 6 sibling files, 493 passed;
check:fastandformat:checkpass.
Remaining. Those loaded runs were on macOS only, so these tests have not been run on Linux locally. CI covers that.
Leaving this thread open for review.
The in-flight launch test read `rootPresent` from a stand-in polling every 2 ms, so a delayed stand-in could see the root already gone and fail correct cleanup. Hold the real release just before root removal until the test has seen the launch refused, and start release only once the launch is admitted, which the 300 ms handshake budget did not ensure in a separate process. Remove the same kind of dependency elsewhere in the file: an elapsed-time bound the exit message already proves, and safety expiries shorter than the test timeouts. The launcher diagnostics grace also becomes 5 s, so a stalled event loop cannot drop the launcher's stderr from a startup failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if (this.#ownership === undefined || this.#familyReleased) return; | ||
| const result = await cleanupOwnedProcessGroup(this.#ownership); | ||
| if (!result.cleaned) | ||
| throw new Error(`Native UI fixture processes: ${result.reason}`, { | ||
| cause: result, | ||
| }); | ||
| this.#familyReleased = true; |
There was a problem hiding this comment.
#sweepFixture() sets #familyReleased even when application retirement failed earlier in the same cleanup attempt. If rename() is refused while LaunchServices still has a launch pending, the sweep can find nothing and cache success. The app can then start before cleanup is retried. That retry retires the path but skips the sweep and deletes the root beneath the running fixture.
Only cache family release after the launcher has stopped and the application path has retired, or sweep again on each failed-release retry. The current behavior also violates the repository requirement to retain ownership when cleanup fails.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/lib/native-ui-fixture-lifecycle.mjs
Line: 221-227
Comment:
**Retry misses a late fixture**
`#sweepFixture()` sets `#familyReleased` even when application retirement failed earlier in the same cleanup attempt. If `rename()` is refused while LaunchServices still has a launch pending, the sweep can find nothing and cache success. The app can then start before cleanup is retried. That retry retires the path but skips the sweep and deletes the root beneath the running fixture.
Only cache family release after the launcher has stopped and the application path has retired, or sweep again on each failed-release retry. The current behavior also violates the repository requirement to retain ownership when cleanup fails.
**Context Used:** AGENTS.md ([source](https://github.com/morluto/rea/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
This held: it was a logic bug in the code I added. Fixed in 61e8653.
Root cause. #sweepFixture cached a clean result in #familyReleased, even when retiring the application path had failed earlier in the same attempt. A clean sweep says nothing while a launch can still start. So a refused rename could cache success, the app could then start, and the retry skipped the sweep and removed the root beneath the running fixture.
Structural fix. I removed the flag rather than adding a condition to it. The invariant is now one sentence: every release attempt stops the launcher, retires the path and sweeps again, and only an attempt whose steps all succeeded removes the root.
Other cached state in the class.
ProviderProcessSupervisor.stop()keeps a completed result, which is safe because an observed launcher exit cannot be undone.- Retirement has no flag. Retrying it after success sees
ENOENT, which is accepted. #releasedis set only after root removal succeeds.- Nothing else can be recorded as done while an earlier step failed.
A retry after only a root removal failure now repeats a sweep that finds nothing. That is the cost of removing the flag, and it applies only on a retry.
Validation.
- New regression at the real owner, with no mock: a read-only root gives a real
EACCESon the rename while the launch is pending. The first sweep finds nothing, then the launch starts and the root is confirmed still present. The retry must kill the fixture and only then remove the root, and nooutlived-rootevent may appear. - With the previous head's lifecycle module restored, that test fails with the fixture still alive (
expected true to be false). - Mutations, re-measured on the final tests, still fail: no retire step (9), sweep before stopping the launcher (26), no ready PID check (1), ignoring a refused cleanup (7),
process.once(6), exit from the listener (11), late status dropped (2), primary precedence lost (4), no forced exit (5), this PR's first signal handler (4). - The test file ran 5 consecutive times with 10 busy loops on this 10-core host (load average up to about 29): 35 passed each time.
- After merging main: the file plus 6 sibling files, 494 passed;
check:fastandformat:checkpass.
Remaining. The refusal here is a read-only directory standing in for whatever might refuse the rename, and the launch service is an authored stand-in, so this does not establish LaunchServices timing. These tests have not been run on Linux locally; CI covers that.
Leaving this thread open for review.
A clean sweep was cached even when retiring the application path had failed in the same attempt. If the rename was refused while a launch was pending, the sweep found nothing, the app then started, and the retry skipped the sweep and removed the root beneath the running fixture. Remove the cached flag. A sweep proves nothing while a launch can still start, so every attempt sweeps again, and only an attempt whose steps all succeeded removes the root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A signal during either Swift compile ended the operation's race while the compiler kept running, so release removed the root beneath it and the verifier exited without stopping it. On base, the same signal left the root behind instead. Run both compiles through the fixture owner with the existing `execFileOutput` and `SWIFTC_INTERRUPT` path. Closing the owner now stops them as swiftc requires, and release joins them before it touches the root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What broke:
verify-native-ui.mjslearned its fixture's PID only from the ready record, so a missing handshake stoppedopen, deleted the fixture root and left the app running, reparented to PID 1. Reproduced on latest main with real LaunchServices.The fix: A new owner holds the run token from before launch, stops the launcher by its handle, retires the app path, sweeps by token, and removes the root only after that succeeds. A refused cleanup keeps the root and run for retry.
The risk: Limited to this macOS verifier and its CI scope. The fixture now ends by
SIGKILLfrom the sweep instead ofSIGTERM, repeated Ctrl-C no longer force-quits during release, and positive capture and actions could not be run here because Screen Recording is not granted.Summary
Fixes #8559.
The verifier could orphan its fixture app whenever the ready handshake was late, missing or invalid, because nothing identified the app until the record arrived. The verifier now owns the launch from before it starts, so cleanup does not depend on the handshake, and it does not delete the root unless cleanup is proven.
Problem and expected behavior
Smallest trigger: a fixture that starts and never writes its ready record.
openis signaled and exits, and the app stays alive with no recorded identity. The violated invariant is that the root must outlive any owned process. The cleanup must also not signal on a PID alone, yet a ready record could name any PID, and that PID was then signaled.Change and scope
scripts/lib/native-ui-fixture-lifecycle.mjsowns the root, launcher and fixture. It reusesspawnOwnedProviderProcess,ProviderProcessSupervisor,cleanupOwnedProcessGroupandobserveProcessStartIdentity; it adds no ownership primitive.NativeUiFixtureCleanupFailure, which names the root and run and keeps the owner retryable. A clean sweep proves nothing while a launch can still start, so every attempt sweeps again, and only an attempt whose steps all succeeded removes the root./usr/bin/openis a platform binary whose token macOS withholds, so the sweep cannot prove it. That is why the handle stops it first.execFileOutputpath withSWIFTC_INTERRUPT. Release stops them as swiftc requires and joins them before it touches the root.runNativeUiFixtureLanewraps the verifier's operation and settles the process outcome in one place, throughwithVerifierCleanup, so primary and cleanup failures both surface.open(1), not--env. My first version used--envand it made unrelated sweeps fail: the shared reader treats an argv entry with the token prefix asambiguous_environment_boundary, so a readable launcher holding one failedverifierTerminalOwnertests running beside it.apple-artifactsjob and scope, because the entrypoint tests can only run on macOS.Added beyond the literal issue, with the failure mode each one addresses:
open10 or 20 ms after spawn, then sweeping immediately, the app appeared after the sweep in 6 of 6 rounds. With the rename, the app did not launch in 15 of 15 rounds at +0 to +30 ms, and at +40 ms the immediate sweep saw it in 3 of 3. Likelihood is low in the normal 20-second path and higher for an early interrupt or a stalled launch. No new on-screen window owner, such as a LaunchServices alert, appeared during the probe.openas well as the fixture. A signal only records the interruption and ends the operation's race. The release always runs to settlement, and every signal stays handled until then, becausenpmforwarding Ctrl-C can deliver the same signal twice.SIGKILLremains the override.Exit status when a signal is involved (the first failure selects the status, and every failure is reported):
Not included: the sibling verifier issues, and the two unrelated lane failures listed under proof gaps.
Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.
Contract and boundary impact
rmare removed.open(1)documents; if that stops, the ready check fails loudly.Expected effects (check known effects; use documentation-only on its own).
The launched process is the verifier's own fixture, as before.
Evidence and regression coverage
tests/boundary/process/nativeUiVerifierFixtureOwnership.test.ts(38 tests), plus three rows inciChangeScope.test.ts. The launcher and fixture are authored stand-ins; the owner, spawn, token sweep, signals and root removal are real. 28 tests run on POSIX hosts, and 10 drive the actual verifier entrypoint on macOS only. Final-cleanup tests park the real release at a barrier inside its retire step, then deliver the signals. The in-flight launch test holds the real release just before root removal, so its ordering does not depend on scheduling.8250bf232): the actual verifier with only the compiler injected, real/usr/bin/openand LaunchServices, and a never-ready AppKit binary. It exited 1 after 20 seconds with the root deleted, and the fixture was still alive:NOT_ENOUGH_INFO: the app is reparented to PID 1, in its own process group, and survivesSIGTERMtoopen.SIGINT. With the previous verifier,SIGTERMduring either compile exits with it never stopped, and both verifier tests fail. Removing the join fails 3 tests, and usingSIGTERMas the stop signal fails 3. The other mutation counts here predate these 3 tests.signalCodeSIGTERMorSIGINT), and a single signal exited 143 instead of keeping the primary failure's 1. Mutations of the lane also fail, re-measured on the final tests:process.once(6), exit from the signal listener (11), dropping the late status (2), losing primary precedence (4), no forced exit (5).Lifecycle regressions covered: handshake late, absent and invalid; launch failure with and without a live process; launcher that ignores
SIGTERM; cleanup refusal followed by retry; a retirement refused while a launch is pending, with the fixture starting before the retry; mixed outcome where the processes are released and root removal is denied, then retried; close racing an admitted launch; interruption during the handshake and during either compile; interruption during final cleanup with successful and failed release, including repeated signals; a hung operation ended by a signal; joined closes sharing one failed release, then a retry; foreign PID in the ready record.Remaining proof gaps:
window-mismatchfrom the helper stopped all 3 full runs on unmodified main and 4 of 6 on this branch. A control using only the owner, with no lane, reproduced it: the fixture's window was listed before the helper call and gone fromCGWindowListCopyWindowInfo(count 0) after the helper's 26-second run. The root cause is not diagnosed.error.messagefortarget-mismatch, but sincecc90771afthat code sits inerror.issues(source-derived, and observed failing here). With only that line changed in a local copy, the lane exited 0 withpermission-boundary-onlyandpositive_e2e: false, leaving no root or process. That run predatesrunNativeUiFixtureLane; all 3 attempts since then stopped atwindow-mismatch, so the real lane's success path through the new wrapper is unobserved. An authored operation covers it in the tests.SIGKILLor a crash still leaves its fixture, because no owner survives it. A release that hangs can now only be ended that way. Its steps have their own bounds (source-derived); that bound was not tested.For evidence-bearing changes: not applicable; this changes no Evidence.
Validation performed
npm run test:focused --the new test,verifierCleanup,verifierTerminalOwner,darwinProcessRunTokenReader,ciChangeScope,ciRequiredLanes,ciQualification— 497 passed, before addinglifetimePipe; the new test file was re-run after it.npm run check:fast— passed.npm run format:check— passed.npm run docs:check— passed./usr/bin/openand LaunchServices, final code: never-ready fixture as above; 4 of 4SIGTERMs sent as soon asopenappeared exited 143 with no root and no survivors;SIGTERMduring each realswiftccompile exited 143 with no root and noswiftprocess left; with the release held,SIGTERM,SIGTERMandSIGINTleft the verifier alive with its root present, and after the gate opened it exited 1 with the primary failure and no root or survivor. Owner code unchanged since these: the realui.swiftfixture reached ready in 1.1 s and closed in 435 ms, with no root or process left, and no crash report named the fixture.Compatibility, safety, and release
Author readiness
Human owner: @kaoru0822-kitauji
Declarations entered by Claude Code on the owner's standing written instruction.
Before merge
type(scope): outcome.🤖 Generated with Claude Code