Repository navigation
feat(local): Add daemon-managed telemetry sessions and viewer - #1492
MathurAditya724 wants to merge 3 commits into
Conversation
Port getsentry/cli#1578 to Toolkit with scoped capabilities, portable daemon lifecycle, and native/npm E2E coverage. Co-Authored-By: GPT-6 Codex <noreply@openai.com>
|
Validation of the migrated feature:
The complete workspace test command was run in UTC with isolated temporary storage and four workers. Its CLI result was 10,644 passed, 14 existing skips, and two failures:
These existing test-isolation failures are documented rather than changed, as requested. The installer and debug-files tests have no changes in this PR. Linux, macOS and Windows npm/native Local coverage is configured in the new Local E2E workflow. The existing Local hosting project also needs to point to Toolkit with |
|
Hosted Local E2E passed on Linux, macOS, and Windows at commit 82cacca. Each platform ran viewer integration tests plus the Local process suite against both the npm bundle and a freshly built native CLI. Typecheck, lint, and docs checks also passed after rebasing onto current main. The original getsentry/cli#1578 draft is now closed as superseded. |
…URLs Carry the ingest token in the pathname for Spotlight SDK transports that omit URL query parameters. Preserve scoped read URLs and legacy ingestion, cover real SDK capture, and scrub Local capabilities at telemetry delivery. Co-Authored-By: GPT-6 Codex <noreply@openai.com>
|
Validation for
The full workspace test gate retains exactly two existing macOS-dependent failures, with 10,653 CLI tests passing and 14 skipped: the musl installer case does not simulate Linux, and the DerivedData case scans the host Xcode directory and times out. The user requested that this PR remain scoped to Local, so those unrelated test files are unchanged. Other tested workspace packages pass. The actual Node SDK regression exposed transports dropping query parameters; the ingest capability now lives in the pathname. Scoped capability URLs are also redacted at final CLI telemetry delivery. Fresh Linux/macOS/Windows npm and native E2E checks are running on this head. BYK is requested for review; the PR is now ready for review. |
|
Hosted readiness update for The PR risk (Jev) check failed with Warden and the broad CLI test job are still running. BYK has not yet reviewed. The PR should not be merged with these gates unresolved. Unrelated workflow/provider changes remain outside this Local migration. |
|
Final platform validation at The independent review has no remaining actionable findings and this PR has no unresolved review threads. Merge readiness is still blocked: PR risk (Jev) failed with a non-retryable provider HTTP 400, Warden and the broad CLI check remain in progress, and BYK review is requested but not yet submitted. The two documented local macOS baseline failures remain unchanged as requested. |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| app.use(`${CONTROL_PLANE_PREFIX}/sessions/:id/stream`, async (c, next) => { | ||
| const origin = c.req.header("origin") ?? ""; | ||
| if (!isHostedUiOrigin(origin)) { | ||
| return loopbackCors(c, next); | ||
| } | ||
| if (c.req.method === "OPTIONS") { | ||
| if (c.req.header("access-control-request-method") !== "GET") { | ||
| return c.body(null, 403); | ||
| } | ||
| c.header("Access-Control-Allow-Origin", origin); | ||
| c.header("Access-Control-Allow-Methods", "GET, OPTIONS"); | ||
| c.header("Access-Control-Allow-Headers", "Last-Event-ID"); | ||
| if (c.req.header("access-control-request-private-network") === "true") { | ||
| c.header("Access-Control-Allow-Private-Network", "true"); | ||
| } | ||
| c.header( | ||
| "Vary", | ||
| "Origin, Access-Control-Request-Headers, Access-Control-Request-Private-Network", | ||
| ); | ||
| return c.body(null, 204); | ||
| } | ||
| if (c.req.method !== "GET") { | ||
| return c.body(null, 403); | ||
| } | ||
| return hostedCors(c, next); | ||
| }); |
There was a problem hiding this comment.
POST /stream bypasses remote-origin ingest write protection
Non-loopback browsers blocked on /sessions/:id/ingest/:cap can still write envelopes via POST /sessions/:id/stream?cap=<ingest>, so a page that knows the client-injected ingest capability can forge telemetry into the session. Apply the same isLoopbackBrowserRequest (or non-GET) rejection used on the ingest route to stream POSTs.
Evidence
- Ingest middleware (lines 122–128) returns 403 unless
isLoopbackBrowserRequestpasses, and tests cover rejectinghttps://local.sentry.dev/https://example.comPOSTs to/ingest/:cap. - Stream middleware (lines 135–143) only special-cases hosted UI origins; any other origin, including remote sites, falls through to
loopbackCorsand reaches the sharedingesthandler for POST. ingestaccepts the ingest secret fromc.req.query("cap")(line 230) and is mounted onPOST .../stream, so/stream?cap=<ingestCapability>is an alternate write path.sentry local run --sessioninjectssession.ingestUrlintoVITE_/NEXT_PUBLIC_/ relatedSENTRY_SPOTLIGHTvars, so the ingest capability is present in browser-exposed client env during normal use.- A simple
text/plainPOST (no CORS preflight) from a non-loopback origin therefore performs the write even though the dedicated ingest route would have returned 403.
Identified by Warden · security-review · LEJ-9TY
| const clearItems = () => { | ||
| telemetryStore.clear(); | ||
| setLastViewedItemIds({}); | ||
| void workspaceQuery.clearWorkspace(); | ||
|
|
||
| const endpoint = parseStreamEndpoint(streamUrl); | ||
| if ( | ||
| endpoint?.kind !== "loopback" || | ||
| !isLoopbackHost(window.location.hostname) | ||
| ) { | ||
| return; | ||
| } | ||
| const clearUrl = new URL(endpoint.url); | ||
| const scopedSession = isScopedSessionStream(clearUrl.pathname); | ||
| if (scopedSession) { | ||
| clearUrl.pathname = clearUrl.pathname.replace(/\/stream$/, "/ui/reset"); | ||
| } else { | ||
| clearUrl.pathname = "/clear"; | ||
| clearUrl.search = ""; | ||
| } | ||
| void fetch(clearUrl, { method: scopedSession ? "POST" : "DELETE" }) | ||
| .then((response) => { | ||
| if (!response.ok) | ||
| throw new Error(`Session reset failed with HTTP ${response.status}`); | ||
| }) | ||
| .catch(() => { | ||
| setMessage( | ||
| "Cleared this viewer, but the receiver could not clear its retained session.", | ||
| ); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Clear events immediately restores the previous search query
After Clear, clearWorkspace() nulls q, but the search-sync effect writes the still-stale commandSearchQuery back into the URL, so later events stay filtered by the old term.
Evidence
clearItems()callsworkspaceQuery.clearWorkspace(), which setsqto null.- It never clears
commandSearchQueryor setsisClearingCommandSearch. - The effect at lines 787–802 runs whenever
searchQuerychanges and, whendebouncedCommandSearchQuery !== searchQuery, callssetSearchQuery(debouncedCommandSearchQuery). selectCommandItemavoids this by clearingcommandSearchQueryunderisClearingCommandSearch;clearItemsdoes not.
Identified by Warden · code-review · D6F-KU9
| const selectedItem = | ||
| workspaceItems.find((item) => item.id === workspaceQuery.eventId) ?? | ||
| workspaceItems[0]; |
There was a problem hiding this comment.
Detail pane can show an event outside the active search filter
Selected event resolution uses unfiltered workspaceItems, so with an active search the detail pane can show a non-matching event while the list only shows matches.
Evidence
searchedItemsisworkspaceItemsfiltered bymatchesSearch(lines 756–758).selectedItemis resolved fromworkspaceItems(byeventIdorworkspaceItems[0]), notsearchedItems.- The detail pane renders whenever
searchedItems.length > 0, using that unfilteredselectedItem(lines 1280–1289). - Repro: oldest event A, newer event B; search matches only B → list shows B, detail still shows A.
Also found at 1 additional location
apps/local/src/App.tsx:1280-1289
Identified by Warden · code-review · X4J-M6J
| let disposed = false; | ||
|
|
||
| void highlighter | ||
| .then((instance) => | ||
| instance.codeToHtml(formattedCode, { | ||
| lang: "json", | ||
| themes: { | ||
| light: "github-light", | ||
| dark: "github-dark", | ||
| }, | ||
| defaultColor: false, | ||
| }), | ||
| ) | ||
| .then((result) => { | ||
| if (!disposed) { | ||
| setHtml(result); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| // Keep the readable, escaped source fallback if syntax highlighting cannot load. | ||
| }); | ||
|
|
||
| return () => { | ||
| disposed = true; | ||
| }; | ||
| }, [formattedCode]); |
There was a problem hiding this comment.
JsonView keeps previous envelope HTML when code changes
When code changes on an already-mounted JsonView, stale highlighted HTML from the previous value stays on screen until the new highlight finishes, and remains permanently if highlighting fails.
Evidence
htmlis React state and is only written inside the asynccodeToHtmlsuccess path; the effect never resets it whenformattedCodechanges.RawEnvelopeDetailrenders<JsonView code={item.text} />without akey, and unlikeEventDetailit is not keyed byselectedItem.id, so switching envelopes reuses the same JsonView instance.- While the new highlight is in flight—or if the
.catchpath runs after a prior success—the render still takes thehtmlbranch and paints the previous envelope viadangerouslySetInnerHTML.
Identified by Warden · code-review · 84A-UEE
| } | ||
| if ( | ||
| state.version !== LOCAL_DAEMON_PROTOCOL_VERSION || | ||
| !(await daemonResponds(state)) | ||
| ) { | ||
| await removeLocalDaemonState(directory); | ||
| return; |
There was a problem hiding this comment.
getRunningLocalDaemon wipes discovery while owner PID can still be alive
A failed HTTP probe deletes the daemon state file without checking state.pid, so a still-running owner becomes undiscoverable and unreplacable until the process is killed manually.
Evidence
getRunningLocalDaemon()callsremoveLocalDaemonState()wheneverdaemonResponds()fails; it never inspectsstate.pidorisProcessRunning.daemonResponds()is only a 500ms authenticated GET/sessions; any timeout, connection failure, or non-OK body counts as dead.runLocalDaemonForeground()keepslocal-control-plane.owneruntil aftershutdownServer()and state cleanup, so the process can still own the lock when probes already fail.- Callers such as
ensureLocalDaemon()/daemon restarttreatundefinedas safe to spawn, but the child losesclaimDaemonOwnership()to the live owner and the parent spawns only once before timing out. - The running daemon never rewrites discovery metadata after startup, so stop/status/session lose the bearer token for the life of that process.
Identified by Warden · code-review · 4P9-5DU
| child.unref(); | ||
| for (let attempt = 0; attempt < 40; attempt += 1) { | ||
| await sleep(125); | ||
| if (startupError) throw startupError; | ||
| const state = await getRunningLocalDaemon(); | ||
| if (state) { | ||
| return state; | ||
| } | ||
| } | ||
| } finally { | ||
| await lock.close(); | ||
| await rm(startupLockPath(directory), { force: true }); | ||
| } | ||
| throw new Error("Timed out while starting the Sentry Local control plane."); |
There was a problem hiding this comment.
ensureLocalDaemon times out before tryListen EADDRINUSE retry can succeed
The post-spawn readiness loop only waits ~5s, but the daemon child uses tryListen which sleeps 5s before its first EADDRINUSE retry, so a busy port makes ensureLocalDaemon throw even while the child is still starting and may bind successfully afterward.
Evidence
- After spawn,
ensureLocalDaemonpolls only 40 × 125ms (~5000ms) then throwsTimed out while starting the Sentry Local control plane. - The child runs
runLocalDaemonForeground→tryListen, which onEADDRINUSEwaitsPORT_RETRY_DELAY_MS(5000ms) before the first retry and may retry three times. - Parent only observes
childerror(spawn failure), notexitor listen progress, so a delayed successful bind still surfaces as a hard failure and can leave a daemon running after the caller already aborted.
Also found at 1 additional location
packages/cli/src/commands/local/daemon.ts:278-287
Identified by Warden · code-review · RA2-V7Q
| {isProbing | ||
| ? "Checking the default receiver on localhost:8969." | ||
| : "Connect this read-only viewer to an event stream."} | ||
| </p> |
There was a problem hiding this comment.
Probing subtitle always claims default localhost:8969 receiver
When phase is "probing", the subtitle always says the default localhost:8969 receiver is being checked even if endpoint is a custom port, session stream, or remote URL, so the landing misstates the active target after Connect or a non-default initial stream.
Evidence
ConnectionLandingreceives the liveendpointprop but the probing subtitle is hardcoded tolocalhost:8969.- In
App.tsx,phaseis set to"probing"wheneverconnection === "connecting", including afterconnectToDraft()sets a parsed customdraftEndpoint/streamUrl. - Initial
draftEndpointalso comes fromresolveInitialStreamUrl(), which can return saved loopback/session streams rather thanDEFAULT_STREAM_URL. showConnectionLandingstays true while connecting with no retained items, so this wrong subtitle is shown for those non-default targets.
Identified by Warden · code-review · MSC-45D
| logger.info( | ||
| `SENTRY_SPOTLIGHT=${daemonSessionId ? `${new URL(spotlightUrl).origin}/_local/v1/sessions/${daemonSessionId}/stream` : spotlightUrl}`, | ||
| ); |
There was a problem hiding this comment.
--session logs a non-ingestible SENTRY_SPOTLIGHT URL
With --session, the status log prints .../sessions/<id>/stream without an ingest capability, while the child actually receives session.ingestUrl. Copying the logged value yields HTTP 401 on envelope POST.
Evidence
spotlightUrlis set todaemonIngestUrl(.../sessions/<id>/ingest/<cap>), and that value is whatbuildChildEnv()/ Wrangler injection use.- The log line replaces it with
${origin}/_local/v1/sessions/${daemonSessionId}/streamwheneverdaemonSessionIdis set. - Control-plane ingest requires
(param|query).cap === capabilities.ingest, so POST to that logged/streampath without the ingest cap returns 401. - Non-session mode logs the same URL that is injected; only the session branch diverges.
Identified by Warden · code-review · U4W-9GZ
Protect both ingest routes, keep live and replacement discovery intact, wait through listen retries, and clean up failed startup. Keep viewer selection, search, JSON and receiver state consistent, and guide legacy commands when the managed daemon owns their port. Integrate current main and verify scoped regressions, browser behavior, full npm E2E and native Local E2E. Existing macOS installer/DerivedData test failures remain documented outside this migration. Co-Authored-By: GPT-6 Codex <noreply@openai.com>
This branch has not been deployed
1 participant
Add isolated Sentry Local telemetry sessions owned by a loopback daemon.
local run --session <name>andlocal serve --session <name>retain captured data after their foreground lease ends; session, event, envelope, and daemon commands let developers and agents inspect it later. Each session has separate ingest/read capabilities and a 500-envelope buffer, with three-hour retention after close and subsequent ingestion.Move the Local browser viewer into
apps/localso the complete feature lives in Toolkit. A loopback-served viewer can reset or close its scoped session; the hosted viewer has read-only access. The existing Vercel Local project must be pointed at Toolkit withapps/localas its root to publish the migrated viewer.The daemon enforces singleton ownership, authenticates discovery, preserves SDK compression and browser stream compatibility, and starts correctly from source, npm bundles, and native binaries. The ingest capability lives in the URL path for SDK transports that omit query parameters; Local capability URLs are scrubbed from diagnostics and outgoing CLI telemetry. Process E2E coverage exercises real Sentry Node SDK capture, concurrent startup, scoped telemetry, retained queries, foreground leases, restart, and shutdown; the Local CI workflow runs npm/native cases on Linux, macOS, and Windows.
Supersedes getsentry/cli#1578.
The full local workspace test gate fails two existing macOS-dependent tests: the musl installer case and the DerivedData scanner case. They remain outside this Local migration. Typecheck, lint, Local regressions, viewer tests, full npm CLI E2E, and native macOS Local E2E pass; hosted platform results will be recorded in the PR.