Skip to content

feat: add Apple xctrace perf profiling#755

Open
thymikee wants to merge 1 commit into
mainfrom
feat/xctrace-perf
Open

feat: add Apple xctrace perf profiling#755
thymikee wants to merge 1 commit into
mainfrom
feat/xctrace-perf

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Adds Apple native xctrace collection under the consolidated perf family for #694.

  • Adds perf cpu profile start|stop|report --kind xctrace and perf trace start|stop --kind xctrace.
  • Wires contracts, CLI grammar, daemon projection/routing, typed client/MCP metadata, compact output, help/docs, and tests.
  • Reuses Apple process resolution and xctrace helper patterns while keeping .trace contents as artifacts only.

Refs #697
Refs #694

Touched files: 23.

Validation

Worker validation passed:

  • pnpm typecheck.
  • Focused perf/CLI tests: 5 files, 145 tests.
  • pnpm check:unit: 247 unit files, 2335 tests, 8 smoke tests.
  • pnpm format.
  • pnpm build.
  • pnpm clean:daemon.
  • git diff --check.

iOS simulator E2E:

  • Simulator: iPhone 17, UDID EE00C210-642F-4AAD-B4DE-94892624F5A4.
  • App/session: Safari com.apple.mobilesafari, session xctrace697-final.
  • Baseline perf metrics succeeded; perf frames reported the expected simulator support limitation.
  • perf cpu profile start/stop/report --kind xctrace succeeded, wrote .tmp/xctrace697/final-profile.trace, and honored --out for .tmp/xctrace697/final-profile.json.
  • Compact report included xctrace kind/mode/template/app bundle and bounded summary metadata.
  • perf trace start --kind xctrace --template "Animation Hitches" started and produced a .trace bundle; stop surfaced Apple’s simulator limitation: Hitches is not supported on this platform.

Physical iPhone 00008150-001849640CF8401C was visible and Safari opened, but baseline perf metrics timed out after the daemon 90s request budget before profile/trace verification could safely proceed. The worker cleaned the stale xctrace process afterward.

SkillGym case was added but not runnable in the sandbox because external runner approval was denied.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-755/

Built to branch gh-pages at 2026-06-11 10:23 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.2 MB 1.2 MB +13.6 kB
JS gzip 390.8 kB 394.4 kB +3.5 kB
npm tarball 504.6 kB 508.2 kB +3.6 kB
npm unpacked 1.7 MB 1.7 MB +14.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.3 ms 28.6 ms +0.3 ms
CLI --help 44.2 ms 43.7 ms -0.5 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/2415.js +22.6 kB +6.1 kB
dist/src/session.js -11.9 kB -3.3 kB
dist/src/args.js +860 B +243 B
dist/src/8699.js +882 B +230 B
dist/src/8173.js +631 B +225 B

Copy link
Copy Markdown
Member Author

Code review

Verdict: significant issues — the process-lifecycle gaps reproduce exactly the stale-xctrace failure hit during validation; security and grammar wiring are otherwise sound.

Findings

  1. Blockersrc/platforms/ios/perf-xctrace.ts:299-308: waitForAppleXctraceStop races the child's exit against a 10 s timer and throws on timeout without ever escalating to SIGKILL, so a hung xctrace keeps running with no automated cleanup — this is the stale process that had to be killed manually during validation. 10 s is also optimistic; xctrace routinely needs longer to finalize a physical-device .trace after SIGINT.

  2. Blockersrc/daemon/handlers/session-perf-xctrace.ts:168-174: the stop handler's catch unconditionally clears session.applePerf.active before rethrowing, so after a stop timeout the daemon drops its only handle to the still-running child; the error's own hint ("Retry perf stop") is then impossible because the next stop returns "no active Apple xctrace perf capture" (line 160). On timeout the handler should SIGKILL (or retain the capture for retry) rather than orphan the process.

  3. Majorsrc/daemon/handlers/session-close.ts:69-80: teardownSessionResources stops appLog and the Apple runner but never stops session.applePerf.active, so closing a session (or daemon shutdown — Node doesn't kill children on exit) mid-capture leaks a running xctrace that will block subsequent xctrace-based commands, including the existing perf metrics Activity Monitor path — the likely mechanism behind the reported 90 s physical-device timeout.

  4. Minorsrc/platforms/ios/perf-xctrace.ts:293-297: the 500 ms settle window only catches immediate launch failures; on physical devices xctrace commonly errors 1-5 s in (device prep, kperf lock), in which case start reports success and the failure only surfaces at stop.

  5. Minorsrc/daemon/handlers/session-perf-xctrace.ts:204-207: report falls back to session.applePerf?.active?.outPath — a .trace bundle still being written by a live xctrace; export from an in-progress trace will fail or produce partial metadata. Better to reject report while a capture is active.

  6. Minorsrc/utils/cli-flags.ts:389-396 + src/utils/args.ts:113-118: two FlagDefinitions now share the name --kind (perfKind, metroKind); correctness silently depends on declaration order plus a command === 'perf' special case in the parser — works today but fragile, and feat: add Android native perf profiling #757 adds a third --kind enum to the same mechanism.

Verified clean

No command injection (arg arrays with shell: false — spaced template names, bundle ids, paths are safe); missing xcrun/xctrace surfaces as MISSING_TOOL; double-start and stop-without-start are rejected; out-path expansion and the rename-with-cp fallback are correct; the start/stop model itself keeps each request well under the 90 s budget. Tests cover forwarding, lifecycle, retry, report defaults, Android rejection — but not the stop-timeout/stale-process path or close-with-active-capture (the two real gaps above).

Conflict analysis with siblings

#757 hard-conflicts both textually and semantically — it edits the same hunks of src/contracts/perf.ts and cli-grammar/observability.ts with an incompatible design: PerfCpuMode/mode + PERF_NATIVE_AREA_VALUES vs this PR's PerfSubject/subject + extended PERF_AREA_VALUES, disjoint PERF_KIND_VALUES, a second conflicting perfKind flag enum, and a different CLI→daemon protocol (#757 requires --kind and passes options as flags; this PR makes kind optional and passes template/out/tracePath as padded positionals). Whichever lands second needs real reconciliation — union the kind enum, pick one of subject/mode, agree on one daemon protocol — not a textual merge. #756 and #759 also overlap on grammar/flags/overrides/docs (and #759 on contracts/perf.ts).

Overall

The feature wiring, contracts, and security posture are solid, and the start/stop model is the right answer to the 90 s budget. But the PR ships the exact failure mode its own validation uncovered — findings 1-3 should be fixed before merge, and the #757 grammar collision needs an explicit coordination decision.


Generated by Claude Code

@thymikee thymikee force-pushed the feat/xctrace-perf branch from e68d49e to 83c5107 Compare June 11, 2026 10:21
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the xctrace lifecycle review feedback in commit 83c5107.

What changed:

  • perf stop now sends SIGINT, waits up to 45s for xctrace finalization, then SIGKILLs and waits for confirmed process exit.
  • Stop errors now distinguish confirmed cleanup from unconfirmed cleanup; daemon state only clears session.applePerf.active when cleanup is confirmed.
  • perf cpu profile report now rejects while a capture is active instead of exporting a live .trace bundle.
  • Session close and daemon session teardown now stop active Apple xctrace captures before deleting session state.
  • Added focused tests for force-kill timeout cleanup, confirmed forced cleanup, retained active state on unconfirmed cleanup, live-report rejection, close teardown, and daemon teardown.

Validation passed locally:

  • Focused tests: pnpm exec vitest run src/platforms/ios/__tests__/perf.test.ts src/daemon/handlers/__tests__/session-observability.test.ts src/daemon/handlers/__tests__/session-close-shutdown.test.ts src/__tests__/cli-perf.test.ts src/utils/__tests__/perf-args.test.ts src/utils/__tests__/args.test.ts (6 files, 157 tests)
  • pnpm check:fallow --base origin/main
  • pnpm check:unit (247 unit files / 2378 tests + 8 smoke tests)
  • pnpm format
  • pnpm build
  • pnpm check:quick
  • git diff --check

No raw .trace content is emitted; output remains compact and artifact-path based.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant