fix(apple): scope perf processes to the resolved app executable - #2406
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped with targeted regression tests, and the only remaining feedback is a minor CLI wording tweak.
Pull request overview
This PR tightens Apple perf process selection so iOS simulator/macOS profiling targets only the resolved app executable path (including /private/var ↔ /var aliases), preventing accidental sampling of another simulator’s app instance with the same basename.
Changes:
- Extracted executable/process identity matching into a dedicated helper (
perf-process-identity.ts) and updated Apple perf code to use it. - Adjusted matching behavior to avoid falling back to basename matching when an executable path is known but doesn’t match.
- Added regression tests for the new matching rules and updated CLI/docs text to clarify profiling scope.
File summaries
| File | Description |
|---|---|
| website/docs/docs/debugging-profiling.md | Documents that simulator/macOS perf targets the resolved executable to avoid cross-simulator name collisions. |
| src/commands/perf/index.ts | Updates CLI help text to describe the new Apple process targeting behavior. |
| packages/platform-apple/src/core/perf.ts | Uses the extracted process identity helper and removes the in-file implementation. |
| packages/platform-apple/src/core/perf-process-identity.ts | New helper implementing path-scoped matching (with /private/var aliasing) plus token parsing. |
| packages/platform-apple/src/core/tests/perf-process-identity.test.ts | Regression coverage for path-scoped matching, alias handling, and name-only fallback when no path exists. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
readProcessCommandToken truncates commands when the executable path contains spaces, which can corrupt processName values and undermine robustness for apps whose executable names include spaces.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| export function readProcessCommandToken(command: string): string { | ||
| const [token = ''] = command.trim().split(/\s+/, 1); | ||
| return token; |
There was a problem hiding this comment.
The display-name limitation is pre-existing: readProcessCommandToken and its memgraph callers have the same implementation at the merge base. At 33fbe8d the parser remains private to perf.ts; the extracted matcher checks the full resolved path before using a token, and its regression covers paths with spaces and arguments. I am keeping this change scoped to excluding other app instances. Plumbing the resolved executableName into memgraph display fields is a separate improvement; quoted/escaped argv handling would need a concrete ps output case.
|
Fixed the coverage failure in All 549 startup-guard tests plus the 28 performance tests pass, as does |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, removes the incorrect basename fallback when an executable path is known, includes targeted regression tests, and updates user-facing docs/help accordingly.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
All PR checks now pass at |
|
Code review is clean at 33fbe8d. The resolved-path match excludes other copies of the app, with reported live two-simulator selection and exact-head memory sampling; checks pass and there are no conflicts. Ready for human review; the separately reproduced xctrace stop limitation still prevents claiming a completed trace/report. |
thymikee
added
the
ready-for-human
Summary
Apple performance sampling could include another simulator’s copy of an app: after resolving the selected app’s executable path, a failed path match still fell back to its basename. Keep matching scoped to the resolved executable (including
/private/varaliases); use name matching only when no path is available.Five files: extract the process identity helper, add regression coverage, and clarify profiling scope in CLI help and docs. This is a separate diagnostic fix discovered while investigating #2362; it does not change touch delivery.
Validation
Head
33fbe8dc806c23152c2824177e34dbc6ab4cdae1:pnpm check:affected --runpasses, including 2,141 related tests, build, lint, formatting, type and architecture checks.726eeffd2: ran the same Scenario Lab app on two owned simulators (PIDs 4297 and 4495).perf cpu profile start --kind xctrace --template "Time Profiler"selected only PID 4495 on the requested simulator; the drag and committed-event check passed.xcrun xctrace record --time-limit 10scontrol also exceeded 90 seconds. Process selection is proven; successful trace/report generation remains unverified. Owned sessions, captures and simulators were cleaned up.CI caught eager import growth. Load the helper on demand; all 549 startup-guard tests and a live
perf memory samplepass at the new head. CI and iOS smoke pass; all PR checks are green.