diff --git a/packages/tool-server/src/tools/profiler/combined/profiler-combined-report.ts b/packages/tool-server/src/tools/profiler/combined/profiler-combined-report.ts index f181b835a..03c49d825 100644 --- a/packages/tool-server/src/tools/profiler/combined/profiler-combined-report.ts +++ b/packages/tool-server/src/tools/profiler/combined/profiler-combined-report.ts @@ -27,10 +27,11 @@ import { loadAndroidCombinedData } from "../../../utils/android-profiler/pipelin import { buildHotCommitSummaries } from "../../../utils/react-profiler/pipeline/00-hot-commits"; import { preprocess } from "../../../utils/react-profiler/pipeline/00-preprocess"; import { readCpuProfile, readCommitTree } from "../../../utils/react-profiler/debug/dump"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const zodSchema = z.object({ port: z.coerce.number().default(8081).describe("Metro server port"), - device_id: z.string().describe("iOS Simulator/device UDID or Android serial"), + device_id: metroDeviceIdParam("iOS Simulator/device UDID or Android serial"), }); interface HangCommitCorrelation { @@ -102,6 +103,29 @@ Fails if either react-profiler-analyze or native-profiler-analyze has not been c // For iOS, the analyze step cached uiHangs + memoryLeaks in parsedData. // For Android, drill-down re-queries the .pftrace, so we load the same // shape on demand here. + // A session with no capture state at all was minted by THIS call: the + // device_id matched no existing session, so nothing is known about the + // device. Say so without naming a platform — classification is shape-based + // and falls back to "android" for any opaque id (utils/device-info.ts:52), + // so an id this tool cannot place would otherwise be reported as an Android + // device (#618). That happens routinely: a forwarded Metro logicalDeviceId + // resolves only while a debugger connection is live, and the alias is + // dropped when it disposes. + if (!nativeApi.traceFile && !nativeApi.exportedFiles && !nativeApi.parsedData) { + throw new FailureError( + `No native profiler capture is loaded for device \`${params.device_id}\`. Run ` + + "native-profiler-start → native-profiler-stop → native-profiler-analyze on this device " + + "first. (If that id came from debugger-connect, pass the id from list-devices instead — " + + "the simulator UDID or adb serial — since profiler sessions are keyed by that one.)", + { + error_code: FAILURE_CODES.PROFILER_DATA_NOT_LOADED, + failure_stage: "profiler_combined_report_load_native_data", + failure_area: "tool_server", + error_kind: "not_found", + } + ); + } + let uiHangs: UiHang[]; let memoryLeaks: MemoryLeak[]; if (nativeApi.platform === "android") { diff --git a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-analyze.ts b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-analyze.ts index 0adab6ef8..553012761 100644 --- a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-analyze.ts +++ b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-analyze.ts @@ -11,11 +11,12 @@ import type { NativeProfilerAnalyzeResult } from "../../../utils/ios-profiler/ty import { analyzeNativeProfilerIos } from "./platforms/ios"; import { analyzeNativeProfilerAndroid } from "./platforms/android"; import { requireArtifacts, type ArtifactHandle } from "../../../artifacts"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const zodSchema = z.object({ - device_id: z - .string() - .describe("Target device id from `list-devices` (iOS UDID or Android serial)."), + device_id: metroDeviceIdParam( + "Target device id from `list-devices` (iOS UDID or Android serial)." + ), }); const capability = { diff --git a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-start.ts b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-start.ts index 5d4975175..9a2ffde87 100644 --- a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-start.ts +++ b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-start.ts @@ -9,11 +9,12 @@ import { assertSupported } from "../../../utils/capability"; import { ensureDeps } from "../../../utils/check-deps"; import { startNativeProfilerIos } from "./platforms/ios"; import { startNativeProfilerAndroid } from "./platforms/android"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const zodSchema = z.object({ - device_id: z - .string() - .describe("Target device id from `list-devices` (iOS UDID or Android serial)."), + device_id: metroDeviceIdParam( + "Target device id from `list-devices` (iOS UDID or Android serial)." + ), app_process: z .string() .optional() diff --git a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-stop.ts b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-stop.ts index 9b1576145..47520a6e5 100644 --- a/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-stop.ts +++ b/packages/tool-server/src/tools/profiler/native-profiler/native-profiler-stop.ts @@ -12,11 +12,12 @@ import { stopNativeProfilerAndroid, type AndroidStopResult } from "./platforms/a import type { ExportDiagnostics } from "../../../utils/ios-profiler/export"; import { requireArtifacts, type ArtifactHandle } from "../../../artifacts"; import type { ArtifactStore } from "@argent/registry"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const zodSchema = z.object({ - device_id: z - .string() - .describe("Target device id from `list-devices` (iOS UDID or Android serial)."), + device_id: metroDeviceIdParam( + "Target device id from `list-devices` (iOS UDID or Android serial)." + ), }); /** diff --git a/packages/tool-server/src/tools/profiler/query/profiler-commit-query.ts b/packages/tool-server/src/tools/profiler/query/profiler-commit-query.ts index 1fa3caff5..37bc81c37 100644 --- a/packages/tool-server/src/tools/profiler/query/profiler-commit-query.ts +++ b/packages/tool-server/src/tools/profiler/query/profiler-commit-query.ts @@ -13,6 +13,7 @@ import { renderComponentNameMiss, describeResolution, } from "../../../utils/react-profiler/component-names"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const timeRangeSchema = z.object({ start: z.coerce.number().describe("Start of range in ms (performance.now clock)"), @@ -21,11 +22,9 @@ const timeRangeSchema = z.object({ const zodSchema = z.object({ port: z.coerce.number().default(8081).describe("Metro server port"), - device_id: z - .string() - .describe( - "Device logicalDeviceId from debugger-connect (iOS simulator UDID or Android logicalDeviceId)." - ), + device_id: metroDeviceIdParam( + "Device logicalDeviceId from debugger-connect (iOS simulator UDID or Android logicalDeviceId)." + ), mode: z .enum(["by_component", "by_time_range", "by_index", "cascade_tree"]) .describe( diff --git a/packages/tool-server/src/tools/profiler/query/profiler-cpu-query.ts b/packages/tool-server/src/tools/profiler/query/profiler-cpu-query.ts index fba56a1ac..4f683a07a 100644 --- a/packages/tool-server/src/tools/profiler/query/profiler-cpu-query.ts +++ b/packages/tool-server/src/tools/profiler/query/profiler-cpu-query.ts @@ -20,6 +20,7 @@ import { describeResolution, } from "../../../utils/react-profiler/component-names"; import { promises as fs } from "fs"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const timeWindowSchema = z.object({ start: z.coerce.number().describe("Start of window in ms (performance.now clock)"), @@ -28,11 +29,9 @@ const timeWindowSchema = z.object({ const zodSchema = z.object({ port: z.coerce.number().default(8081).describe("Metro server port"), - device_id: z - .string() - .describe( - "Device logicalDeviceId from debugger-connect (iOS simulator UDID or Android logicalDeviceId)." - ), + device_id: metroDeviceIdParam( + "Device logicalDeviceId from debugger-connect (iOS simulator UDID or Android logicalDeviceId)." + ), mode: z .enum(["top_functions", "time_window", "call_tree", "component_cpu"]) .describe( diff --git a/packages/tool-server/src/tools/profiler/query/profiler-load.ts b/packages/tool-server/src/tools/profiler/query/profiler-load.ts index 143afcb47..a8bc165f6 100644 --- a/packages/tool-server/src/tools/profiler/query/profiler-load.ts +++ b/packages/tool-server/src/tools/profiler/query/profiler-load.ts @@ -25,6 +25,7 @@ import { isCaptureInFlight, inFlightGuardMessage, } from "../../../utils/profiler-shared/capture-guard"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; // session_id is interpolated into on-disk file paths // (`react-profiler-${id}_cpu.json`, `native-profiler-${id}_raw_cpu.xml`, …). @@ -68,11 +69,9 @@ const zodSchema = z.object({ .describe( "Metro port — the loaded React data is cached under this port for query tools (default 8081)" ), - device_id: z - .string() - .describe( - "Target device id from `list-devices`. Used to cache the loaded React session under the correct port+device key, and required to resolve the native profiler session for load_native." - ), + device_id: metroDeviceIdParam( + "Target device id from `list-devices`. Used to cache the loaded React session under the correct port+device key, and required to resolve the native profiler session for load_native." + ), app_process: z .string() .optional() diff --git a/packages/tool-server/src/tools/profiler/query/profiler-stack-query.ts b/packages/tool-server/src/tools/profiler/query/profiler-stack-query.ts index aed322b57..1fe7265d5 100644 --- a/packages/tool-server/src/tools/profiler/query/profiler-stack-query.ts +++ b/packages/tool-server/src/tools/profiler/query/profiler-stack-query.ts @@ -17,9 +17,10 @@ import { import { normalizeThreadName } from "../../../utils/profiler-shared/thread"; import { formatBytes, escapeMarkdownTableCell } from "../../../utils/profiler-shared/format"; import { demangleSymbol } from "../../../utils/profiler-shared/demangle"; +import { metroDeviceIdParam } from "../../../utils/debugger/device-id-param"; const zodSchema = z.object({ - device_id: z.string().describe("iOS Simulator UDID or Android serial."), + device_id: metroDeviceIdParam("iOS Simulator UDID or Android serial."), mode: z .enum(["hang_stacks", "function_callers", "thread_breakdown", "leak_stacks"]) .describe( @@ -455,6 +456,29 @@ Fails if native-profiler-analyze has not been run or no parsed trace data is in }), async execute(services, params) { const api = services.session as NativeProfilerSessionApi; + // A session with no capture state at all was minted by THIS call: the + // device_id matched no existing session, so nothing is known about the + // device. Say so without naming a platform — classification is shape-based + // and falls back to "android" for any opaque id (utils/device-info.ts:52), + // so an id this tool cannot place would otherwise be reported as an Android + // device (#618). That happens routinely: a forwarded Metro logicalDeviceId + // resolves only while a debugger connection is live, and the alias is + // dropped when it disposes. + if (!api.traceFile && !api.exportedFiles && !api.parsedData) { + throw new FailureError( + `No native profiler capture is loaded for device \`${params.device_id}\`. Run ` + + "native-profiler-start → native-profiler-stop → native-profiler-analyze on this device " + + "first. (If that id came from debugger-connect, pass the id from list-devices instead — " + + "the simulator UDID or adb serial — since profiler sessions are keyed by that one.)", + { + error_code: FAILURE_CODES.PROFILER_DATA_NOT_LOADED, + failure_stage: "profiler_stack_query_load_native_data", + failure_area: "tool_server", + error_kind: "not_found", + } + ); + } + if (api.platform === "android") { return executeAndroid(api, params); } diff --git a/packages/tool-server/src/utils/debugger/device-id-param.ts b/packages/tool-server/src/utils/debugger/device-id-param.ts new file mode 100644 index 000000000..9061c2c7f --- /dev/null +++ b/packages/tool-server/src/utils/debugger/device-id-param.ts @@ -0,0 +1,40 @@ +import { z } from "zod"; +import { canonicalDeviceId } from "./device-alias"; + +/** + * A `device_id` parameter that accepts either id namespace the Metro-family + * tools deal in. + * + * `debugger-connect` returns a Metro `logicalDeviceId`, and when several devices + * share one Metro the debugger tools instruct the user to pass it. But profiler + * sessions are keyed by the list-devices id (the simulator UDID or adb serial), + * and `classifyDevice` decides platform purely from an id's shape — an opaque + * logicalDeviceId matches no known shape, so it falls through to "android" + * (utils/device-info.ts:52). Following the debugger tools' own advice therefore + * built an Android session for an iOS device: the wrong service, not merely the + * wrong word in a message (#618). + * + * Canonicalizing here rather than inside each tool is what makes it stick. The + * registry parses params exactly once and hands the SAME object to `services()` + * and to `execute()` (packages/registry/src/registry.ts:125-153), so one + * transform covers URN construction, the platform branch, and every id the tool + * forwards to a platform impl — including the ones passed straight to `adb` and + * `simctl` as a serial. Canonicalizing in `services()` alone would leave those + * disagreeing with each other. + * + * Deliberately NOT done inside `resolveDevice`: that is the lowest-level + * identity primitive, used by ~60 call sites that can only ever receive a + * list-devices id, and making it consult a mutable alias map would make the same + * input resolve differently depending on whether a debugger had connected. + * + * The transform is invisible on the wire — `zodObjectToJsonSchema` derives the + * published schema with `io: "input"`, which keeps the `string`, the + * `minLength` and the description and drops the transform. + */ +export function metroDeviceIdParam(description: string) { + return z + .string() + .min(1, "device_id must not be empty") + .describe(description) + .transform((id: string) => canonicalDeviceId(id) ?? id); +} diff --git a/packages/tool-server/test/metro/device-id-canonicalization.test.ts b/packages/tool-server/test/metro/device-id-canonicalization.test.ts new file mode 100644 index 000000000..db7b23bdd --- /dev/null +++ b/packages/tool-server/test/metro/device-id-canonicalization.test.ts @@ -0,0 +1,166 @@ +/** + * Issue #618. `debugger-connect` hands back a Metro `logicalDeviceId`, and when + * several devices share one Metro the debugger tools tell the user to pass it. + * But `classifyDevice` decides platform purely from an id's shape, and an opaque + * logical id matches nothing, so it falls through to "android" + * (utils/device-info.ts:52). + * + * Following the tools' own advice therefore built a `NativeProfilerSession` + * under an id no session was ever stored under. The registry mints services on + * demand, so that resolved to a brand-new session frozen to "android" — and an + * iOS user was told "No Android trace loaded". The wrong service, not just the + * wrong word. + */ +import { describe, it, expect, beforeEach } from "vitest"; +import { getFailureSignal } from "@argent/registry"; +import { + rememberDeviceAlias, + forgetDeviceAlias, + resetDeviceAliases, +} from "../../src/utils/debugger/device-alias"; +import { resolveDevice } from "../../src/utils/device-info"; +import { zodObjectToJsonSchema } from "@argent/registry"; +import { profilerCombinedReportTool } from "../../src/tools/profiler/combined/profiler-combined-report"; +import { nativeProfilerSessionBlueprint } from "../../src/blueprints/native-profiler-session"; + +const LOGICAL_ID = "8a44101d"; +const IOS_UDID = "1E273101-2926-4A76-88D0-544C7EA5C2FD"; + +const schema = profilerCombinedReportTool.zodSchema!; + +function parseDeviceId(id: string): string { + return (schema.parse({ port: 8081, device_id: id }) as { device_id: string }).device_id; +} + +beforeEach(() => { + resetDeviceAliases(); +}); + +describe("device_id accepts either id namespace", () => { + it("resolves a Metro logicalDeviceId to the device it was connected with", () => { + rememberDeviceAlias(LOGICAL_ID, IOS_UDID); + + expect(parseDeviceId(LOGICAL_ID)).toBe(IOS_UDID); + }); + + it("routes the resolved id to the iOS session, not a minted Android one", () => { + // The reported symptom, at the layer that caused it: services() builds the + // URN, so if canonicalization does not happen before that, the tool opens a + // session that never existed. + rememberDeviceAlias(LOGICAL_ID, IOS_UDID); + + const params = schema.parse({ port: 8081, device_id: LOGICAL_ID }); + const refs = profilerCombinedReportTool.services!(params as never) as { + nativeSession: { urn: string; options: { device: { platform: string } } }; + }; + + expect(refs.nativeSession.urn).toBe(`NativeProfilerSession:${IOS_UDID}`); + expect(refs.nativeSession.options.device.platform).toBe("ios"); + }); + + it("passes an id through untouched once its alias is gone", () => { + // Aliases live only as long as the debugger connection: they are dropped on + // dispose, so this is the ordinary state a while after profiling. + rememberDeviceAlias(LOGICAL_ID, IOS_UDID); + forgetDeviceAlias(LOGICAL_ID); + + expect(parseDeviceId(LOGICAL_ID)).toBe(LOGICAL_ID); + }); +}); + +describe("shape-based classification is left alone", () => { + it("still treats an opaque serial as a physical Android device", () => { + // The load-bearing fallback. A physical Android serial is an arbitrary + // manufacturer string with no matchable shape, which is exactly why + // classifyDevice ends in "android" — so the fix must not tighten it, and + // must not rewrite an id that was never aliased. + expect(parseDeviceId("HT82A0203045")).toBe("HT82A0203045"); + expect(resolveDevice("HT82A0203045")).toMatchObject({ platform: "android", kind: "device" }); + }); + + it("still treats a wireless-debugging address as a physical device", () => { + expect(resolveDevice("192.168.1.5:5555")).toMatchObject({ + platform: "android", + kind: "device", + }); + }); + + it("still treats an emulator serial as an emulator", () => { + expect(parseDeviceId("emulator-5554")).toBe("emulator-5554"); + expect(resolveDevice("emulator-5554")).toMatchObject({ platform: "android", kind: "emulator" }); + }); + + it("leaves a Chromium id alone", () => { + expect(parseDeviceId("chromium-cdp-19222")).toBe("chromium-cdp-19222"); + }); +}); + +describe("the published schema", () => { + it("does not leak the transform to callers", () => { + // The wire contract must stay a plain string: agents read this schema, and a + // transform in it would be neither meaningful nor representable. + const json = zodObjectToJsonSchema(schema) as { + properties: { device_id: Record }; + required?: string[]; + }; + + expect(json.properties.device_id).toMatchObject({ type: "string", minLength: 1 }); + expect(json.properties.device_id.description).toEqual(expect.any(String)); + expect(json.required).toContain("device_id"); + }); + + it("rejects an empty device_id instead of resolving it to an Android device", () => { + // classifyDevice("") is "android" too, so an empty id used to produce a + // plausible-looking device rather than an error. + expect(schema.safeParse({ port: 8081, device_id: "" }).success).toBe(false); + }); +}); + +describe("an unrecognised device is not reported as an Android device", () => { + async function sessionWithPlatform(platform: "ios" | "android") { + const device = { id: "whatever", platform, kind: "simulator" as const }; + const instance = await nativeProfilerSessionBlueprint.factory({}, device, { device }); + return instance.api; + } + + for (const platform of ["ios", "android"] as const) { + it(`says nothing about the platform for a session that never captured (${platform})`, async () => { + // A session with no capture state at all was minted by this very call, so + // its platform is whatever the id's shape guessed — it is not evidence + // about the device and must not be reported as if it were. + const nativeSession = await sessionWithPlatform(platform); + + const err = await profilerCombinedReportTool.execute!( + { nativeSession } as never, + { port: 8081, device_id: LOGICAL_ID } as never + ) + .then(() => null) + .catch((e: Error) => e); + + expect(err).toBeInstanceOf(Error); + expect(err!.message).toMatch(/No native profiler capture is loaded/); + expect(err!.message).not.toMatch(/android/i); + // And it names the way out of the id mix-up that causes this. + expect(err!.message).toMatch(/list-devices/); + expect(getFailureSignal(err!)?.error_code).toBe("PROFILER_DATA_NOT_LOADED"); + }); + } + + it("still gives the Android-specific message once a trace proves the platform", async () => { + // Started but not stopped: traceFile is set, so the platform IS known and + // the specific guidance is correct. The neutral gate must not swallow it. + const device = { id: "emulator-5554", platform: "android" as const, kind: "emulator" as const }; + const instance = await nativeProfilerSessionBlueprint.factory({}, device, { device }); + const nativeSession = instance.api; + nativeSession.traceFile = "/tmp/fake.pftrace"; + + const err = await profilerCombinedReportTool.execute!( + { nativeSession } as never, + { port: 8081, device_id: "emulator-5554" } as never + ) + .then(() => null) + .catch((e: Error) => e); + + expect(err!.message).toMatch(/No Android trace loaded/); + }); +});