chore(trace): use a single id for calls and test runner steps - #42430
chore(trace): use a single id for calls and test runner steps#42430Dmitry Gozman (dgozman) wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
🔴 The one failure looks caused by this PRHi, I'm the Playwright bot and I took a first look at the CI failures.
DetailsCaused by this PR
Pre-existing flakes / infra (not triaged)
Worth a look before merge: run the trace test locally against the parallel-request scenario and confirm the merged-by-call-id actions still resolve for concurrent API calls. Triaged by the Playwright bot - agent run |
This comment has been minimized.
This comment has been minimized.
c237a77 to
7c20c33
Compare
The library minted `call@<wire-id>` while the test runner minted `<category>@<ordinal>`, and the two were reconciled through a `stepId` side-channel on every trace event. Instead, let the client instrumentation assign the protocol message id, so a call and its step share one id. - `Metadata.stepId` is gone; the message's own `id` is the call id, minted in the client connection when the instrumentation did not assign one. - `ClientSideCallMetadata.id` is a string, so `trace.stacks` is keyed by the same id as the action. - `stepId` is no longer written to the trace. `TraceModernizer` rewrites the ids of older traces on the fly and owns the stacks through `appendStacks()`. - `TraceModel` merges actions by call id, dropping `nonPrimaryIdToPrimaryId`.
Step ids double as protocol call ids now, but the counter restarted for every test while the client connection that correlates responses by id is shared by all tests in the worker. A call still in flight when a test ended could have its id reused by the next test, overwriting the pending callback and failing with "Cannot find command to respond".
7c20c33 to
0898882
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"4 failed 8271 passed, 1371 skipped Merge workflow run. |
Test results for "tests 1"8 flaky51309 passed, 1246 skipped Merge workflow run. |
🟢 The failures are all pre-existing flakes — this PR looks clearHi, I'm the Playwright bot and I took a first look at the CI failures. The four reds are all MCP tests, each on a single browser, and none of them touch the trace call-id path this PR rewrites (client instrumentation, connection, dispatcher, DetailsPre-existing flakes / infra Verdict flips across unrelated runs for all four, on the same
None of these exercise trace call-id merging, and each has failed on SHAs this PR can't be responsible for — the signature is flake, not regression. Triaged by the Playwright bot. Triaged by the Playwright bot - agent run |
2 participants
Summary
Metadata.stepIdis gone;call@<n>is minted in one place, the client connection, only when nothing assigned an id.stepIdis no longer written to the trace.TraceModernizerrewrites the ids of older traces on the fly and owns client-side stacks viaappendStacks().TraceModelmerges actions by call id, sononPrimaryIdToPrimaryIdgoes away.ClientSideCallMetadata.idbecomes a string — the language ports need to send the message id there instead of an int.Design choice
This is done to unify ids between steps and actions. Alternatively, we could use the
callIdas a step id, except for expect calls where the step is created much earlier than the is made, and rewriting the id is too late.