From 002f80f4d84db79296af4f9afc7881769ff15157 Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 10:30:26 +0200 Subject: [PATCH] fix(profiler): resolve a Metro logicalDeviceId instead of calling it an Android device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `debugger-connect` returns a Metro logicalDeviceId, and when several devices share one Metro the debugger tools instruct the user to pass it. Profiler sessions, though, are keyed by the list-devices id, and platform is decided purely from an id's shape — an opaque logical id matches nothing, so it fell through to "android". The reported symptom was the mildest part. Because the id named no existing session, the registry minted a brand-new one on demand, frozen to android and holding no capture. So an iOS user was not merely told the wrong word: the tool reported on a session that had never recorded anything. Correcting only the platform string would still have been wrong, since the real data sits under the UDID. It reached further than the message, too. The same misclassification sends native-profiler-start down the Android path, where the id is handed to adb as a serial, and three react-profiler tools build session URNs with no canonicalization at all — opening a second CDP connection to a device that already has one, which is what the alias map was introduced to prevent. The device_id parameter now resolves through the alias map in the schema itself. That placement is the point: the registry parses params once and hands the same object to services() and to execute(), so one transform covers URN construction, the platform branch, and every id forwarded to a platform impl — including the ones passed to adb and simctl. Canonicalizing inside services() alone would let those disagree with each other. classifyDevice is deliberately untouched. Its android fallback is load-bearing: a physical Android serial is an arbitrary manufacturer string with no matchable shape, so any rule strict enough to reject a logicalDeviceId rejects real hardware too. There is a test pinning that. Canonicalization alone would not have been enough. Aliases live only as long as the debugger connection — they are dropped on dispose — so a user who profiles, lets the app reload, and then asks for a report has nothing to resolve through. That is the ordinary case, not an edge one. So a session with no capture state at all now reports that without naming a platform, and points at list-devices as the id to use. The platform-specific messages are kept for the states that actually prove a platform, so an Android user who started but never stopped still gets the Android instruction. An empty device_id is also rejected rather than resolving to a plausible-looking Android device. Scoped to the eight profiler tools that route to a native session, which is what closes the reported bug. The remaining Metro-family tools and the tool and skill docs that tell the agent to reuse a logicalDeviceId as device_id are a separate change, since that one rewrites agent-facing semantics rather than fixing a defect. Fixes #618 --- .../combined/profiler-combined-report.ts | 26 ++- .../native-profiler-analyze.ts | 7 +- .../native-profiler/native-profiler-start.ts | 7 +- .../native-profiler/native-profiler-stop.ts | 7 +- .../profiler/query/profiler-commit-query.ts | 9 +- .../profiler/query/profiler-cpu-query.ts | 9 +- .../src/tools/profiler/query/profiler-load.ts | 9 +- .../profiler/query/profiler-stack-query.ts | 26 ++- .../src/utils/debugger/device-id-param.ts | 40 +++++ .../metro/device-id-canonicalization.test.ts | 166 ++++++++++++++++++ 10 files changed, 280 insertions(+), 26 deletions(-) create mode 100644 packages/tool-server/src/utils/debugger/device-id-param.ts create mode 100644 packages/tool-server/test/metro/device-id-canonicalization.test.ts 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/); + }); +});