fix: track recorder ownership so trace/profiler stop don't cross-stop#1457
Open
LukeTheoJohnson wants to merge 1 commit into
Open
fix: track recorder ownership so trace/profiler stop don't cross-stop#1457LukeTheoJohnson wants to merge 1 commit into
LukeTheoJohnson wants to merge 1 commit into
Conversation
`trace` and `profiler` share the CDP Tracing domain, but `TracingState` only tracked a single `active` flag. Neither stop command recorded which command owned the active recording, so a `profiler stop` would issue `Tracing.end` against a recording started by `trace start` (and vice versa): the wrong command consumed the recording and the owner's own stop then failed with "No tracing in progress". Track the owner on start and reject a stop from the other command before issuing `Tracing.end`, so the real recorder keeps running. Adds unit tests covering both cross-stop directions. Fixes vercel-labs#1313
Contributor
|
@LukeTheoJohnson is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
traceandprofilerboth drive Chrome's CDPTracingdomain. The daemon tracked only whether a recording was active, not which command started it, so astopissued by the other command ended the wrong recording. The owner's ownstopthen failed and its data was lost.Current behavior
The symmetric case (
profiler startthentrace stop) fails the same way:trace stopconsumes the profiler recording andprofiler stopthen reports "No profiling in progress". Both repros are in #1313.Root cause
TracingStateheld a singleactive: bool.trace_stopandprofiler_stopboth guarded only onactive, then issuedTracing.endunconditionally, so either command could stop a recording started by the other.Fix
Record which command owns the active recording (
TracingOwner::Trace/TracingOwner::Profiler) when it starts, and check it on stop through a smallensure_stoppablehelper. A stop from the other command returns a clear error beforeTracing.endis sent, so the real recorder keeps running:trace start+profiler stop->Cannot stop profiler: a trace recording is active. Use 'trace stop' instead.profiler start+trace stop->Cannot stop trace: a profiler recording is active. Use 'profiler stop' instead.The existing idle messages (
No tracing in progress/No profiling in progress) are unchanged for the genuine "nothing running" case.Tests
ensure_stoppableis a pure function, so the regression is pinned with deterministic unit tests that need no Chrome: both cross-stop directions, the owner stopping its own recording, and the idle case.cargo test --profile cipasses.Scope
Ownership tracking is the minimal change that fixes the reported bug. The single shared
TracingStatestill means trace and profiler cannot run at the same time (a secondstartis rejected); this PR preserves that and only corrects which command maystop. No CLI, MCP, or flag surface changes, so no docs updates are needed.CHANGELOG.mdis left to the maintainer per AGENTS.md.Fixes #1313