feat: add sampling profiler launch modes - #148
Conversation
b701b39 to
da0d237
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed da0d2371b6dc22286573185d82f33940435e7e29 against ef1e297cb79cfb4dbc1ad39a2a17f24c53321894 (also the merge base). The profiling command is useful, but its privacy and lifecycle contracts are not safe to rely on yet.
Findings
[P1] Do not write an unsanitized trace under the sanitized capture contract
scripts/profile-dev.mjs:447-450
--network writes the raw Tracing stream directly to chrome-performance.json; only the separate network.json receives URL/header filtering. With this PR's exact tracing categories/options, installed Chrome 153.0.8010.53 retained synthetic query and fragment markers in ResourceSendRequest/loading events, and an arbitrary X-Review-Secret response-header value in ResourceReceiveResponse.args.data.headers. The documented “sanitized browser network and Chrome performance events” capture can therefore expose sensitive URLs/headers when shared. Network.enable.maxPostDataSize does not sanitize the Tracing domain. Enforce the advertised contract across the trace before writing it, or omit that raw artifact from the sanitized mode. Use an allowlisted output/schema rather than assuming only args.data.url contains URLs: this probe also found args.url and args.snapshot.documentLoaderURL. Add an actual-CDP synthetic-secret regression. This reproduction did not retain the planted Set-Cookie value; I am not claiming that cookies or authorization headers were observed.
[P2] Use the launched Vite server URL instead of assuming the requested port
scripts/profile-dev.mjs:382-386
The profiler fixes url to the requested port, but launches Vite with its existing strictPort: false behavior. When that port already hosts another dev server, waitForServer immediately accepts the old server while the new Vite moves to the next port. page.goto(url) then profiles the old app's renderer while the Node inspector profiles the new Vite/broker, producing a misleading combined capture. Reproduced with actual pinned Vite 8.3.0 and two synthetic localhost pages: requested 59970, Vite reported 59971, profiler readiness/open URL still returned the unrelated page on 59970. Bind readiness/navigation to the actual launched endpoint, or explicitly require an exclusive port and fail before accepting an unrelated listener.
[P2] Serialize stop intent with resource acquisition and always clean up startup failures
scripts/profile-dev.mjs:464-475
A signal runs stop() concurrently with the remaining startup awaits. Holding chromium.launch() at an explicit gate, then delivering SIGINT, produced “Profile saved” with only manifest/broker data; after releasing the launch, the unchanged function started renderer profiling and navigated anyway. The Browser was never closed, and the exit listener at lines 504–506 was attached after Vite's exit, leaving profileWeb() pending. Errors after Browser creation also escape to the outer catch, which stops children but never closes Browser/inspector. Make cancellation stop forward startup progress, clean late-acquired resources, attach exit observation before it can be missed, and run Browser/inspector/child cleanup in an idempotent finally on every exit. Report cancellation/partial output rather than full success. Add a gated launch-interruption regression and a post-launch failure case.
[P2] Reject inspector requests when the transport closes
scripts/profile-dev.mjs:103-109
send() only settles on a matching response; socket closure neither rejects pending requests nor prevents a new request. Reproduced the unchanged inspectorClient against actual Node 24.18.0: start profiling, kill the synthetic target, wait for both child exit and WebSocket close, then send("Profiler.stop") remains pending. When Vite dies during capture, the path at lines 507–508 calls stop(), which can hang here before reaching Browser cleanup. A second Ctrl-C only kills tracked children and does not release this wait. Reject outstanding/new requests on terminal socket state and ensure finalization can fail/abort into cleanup; add an inspector-loss regression.
[P2] Disclose system-wide desktop capture before suppressing privacy prompts
scripts/profile-dev.mjs:519-527
This command records --all-processes --no-prompt, not just the desktop process tree advertised in contributing.md and the console. Installed xcrun xctrace help record confirms those flags record every process and suppress prompts including privacy warnings. The accurate all-native-processes value exists only inside the generated manifest, after capture has started. A developer opting into profiling Buzz can unknowingly include unrelated applications' native stacks/process metadata. The bounded fix is to explicitly disclose system-wide scope and artifact sensitivity before recording and in the usage docs, rather than claim a Buzz-only process tree. Narrowing the capture is an alternative, not a requirement to design a new process-tree profiler.
Validation and bounded exit criteria
The checks in the current hosted CI run, including CI required, pass. They do not exercise these new profiler modes: the PR adds no profiler test, and the existing dev-command integration test only invokes ordinary web/desktop commands. I did not duplicate the broad suite.
Independent review lanes covered privacy, web lifecycle, and desktop behavior; I traced and reconciled every finding above. Focused evidence: actual Vite 8.3.0 port fallback; actual Chrome/CDP trace with synthetic local markers; unchanged startup/signal functions with an explicitly gated fake Chrome acquisition; unchanged inspector client against actual Node 24.18.0. No live identity traffic or system-wide native recording was captured. The native path was source-reviewed and checked against installed xctrace help only.
Resolve the five contracts above and add focused regressions for the failing boundaries. No production-app changes or generalized profiling framework are needed. Non-blocking notes: state that web profiling is also macOS-only and requires installed Google Chrome; do not imply full cold-start coverage because desktop launch is not gated on xctrace readiness. --notify-tracing-started exists if launch-interval coverage becomes a requirement. Broad CI is green, but no approval or merge is implied.
🤖 Superseded by fixes through ecaca4a. Independent exact-head re-review found no actionable web or desktop findings; hosted checks pass, and the owner accepted the documented native-parent-only desktop scope.
ecaca4a to
bb4354e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one existing P2 remains at bb4354eb against base/merge-base 877ae2a6749221dd6850d5695b5c8de023a239e3.
- Required fix: finish the startup-cancellation fence. Ctrl-C during page creation still permits profiling/navigation afterward. Exit criterion: gated cancellation reaches cleanup without late navigation, while late-launch and failure cleanup remain correct.
- Resolved: raw Chrome trace leakage, wrong Vite endpoint, dead-inspector waits, and undisclosed system-wide capture. No new blocker is being added for those contracts.
- Validation: hosted CI passed on merge
d8cd35126db3bf565a731e6fbcfde558adda175b, including all six profiler helper tests. Focused checks passed against actual Vite 8.3.0 and Node 24.18.0; the gated startup probe reproduces the remaining defect. The committed tests lack this ordering regression and a network-artifact regression. No broad suites, live-account capture, or native Instruments run repeated locally; native capture scope is source/CLI-contract reviewed, with manual runtime evidence reported by the author.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Previous blocker resolved; no remaining code findings. Re-reviewed 2ad7e88bd3d8f79cf2dae7a53253d250bbf83c31 against base/merge-base 877ae2a6749221dd6850d5695b5c8de023a239e3, concentrating on the delta from bb4354eb and the agreed cancellation contract. This is a comment review, not approval or unconditional merge clearance.
- Resolution: the remaining startup-cancellation P2 meets its exit criterion. The other four findings remain resolved; this delta does not change those paths.
- Evidence and limits: all 32 committed profiler tests pass in the pinned archive, and my independent held-page/late-launch/failure probe passes against unchanged current functions. The fixture ordering was independently reviewed. Prior real Vite ownership and Node inspector-loss evidence still applies to unchanged paths. No broad suite, real Chrome/live-account capture, or native Instruments rerun; the browser double does not establish real Playwright cancellation behavior.
- Remaining merge gate: required CI is red. Chromium 2/2 and WebKit 2/2 time out in the two quota-recovery journeys waiting for “Conversation options”; WebKit also fails the settings switch's strict viewport assertion. These appear unrelated to the profiler-only delta, but their causes are unproven and the required gate is not waived. Resolve or separately disposition those failures before merge.
Carl, an automated reviewer, commenting via Wes’s GitHub account. Superseded by re-review at 2ad7e88: the sole remaining cancellation blocker is verified fixed. See #148 (review). Dismissing only my stale code-blocking verdict, not approving the PR; required CI remains red.
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Signed-off-by: cyberpunk <d6839277d8e2b34d4f50da40d4de079dc36df4c282e87147aaaa2674c070cf5a@buzz.block.builderlab.xyz>
2ad7e88 to
afe0bf6
Compare
…search-send * origin/main: Connect attachments to existing message delivery (#176) perf: preserve unchanged thread row identities (#171) perf: cache markdown preparation by content (#172) Add safe attachment upload groundwork (#150) feat: add sampling profiler launch modes (#148) feat(channels): remove DMs from the sidebar (#157) Distinguish namesake agents and selected recipients (#142) feat(channels): move diagnostics into Channel Settings (#163) Replace warning banners with shared Base UI toasts (#164) feat(shortcuts): add keyboard shortcut settings (#155) fix(channels): give floating unread cue an opaque panel surface (#153) feat(communities): add BUZZ_DEV_OPEN_RELAY to open the default relay on fresh dev ports (#151) Restore recipient avatars beside the composer mention tool (#162) Fix startup inventory duplication and late panel scroll shifts (#160) feat(channels): add channel creation (#138) Standardize Button and IconButton with Buzz design tokens (#145) Signed-off-by: Zach Marley <zmarley@squareup.com>
Summary
Usage
just web profilerecords Chromium renderer and Vite/broker.cpuprofilefiles after owned-Vite readiness.just web profile --networkadditionally writes sanitizednetwork.json. It omits payloads, cookies, authorization headers, query strings, fragments, opaque/data URL contents, and WebSocket frame data. No raw Chrome trace is written.just desktop profilerecords a macOS Instruments Time Profiler.tracefor the launched Buzz native parent process. It does not use system-wide--all-processescapture. WebKit subprocesses and the Vite broker are outside this native trace; use web profiling for those CPU lanes. Native watching is disabled during capture..profiles/...path and exit successfully after handled interruption..cpuprofilein Chromium DevTools via Performance → Load profile. Open.tracein Instruments.just profile-cleanremoves all generated captures.Browser network capture covers browser-visible requests only; the broker-to-upstream-relay WebSocket remains outside the browser CDP boundary.
Test plan
pnpm checknode --test tests/integration/profile-dev.test.mjs(3/3)git diff --checkjust web profile --network --port 15997through Ctrl-C: finalized renderer/broker/network artifacts and exited 0 in ~6sjust desktop profile --port 15999through Ctrl-C: finalized a parent-scoped Instruments trace and exited 0buzz-foundation, not a system-wide process list333aaeb