Harden Desktop error telemetry and diagnostics#1271
Conversation
📝 WalkthroughWalkthroughThis PR standardizes telemetry sanitization and failure tracking, preserves bounded Python traceback context, adds operation-specific updater errors, instruments adoption and migration phases, and improves process termination and port-conflict handling during launches. ChangesTelemetry and diagnostics
Adoption, migration, and launch flows
Sequence Diagram(s)sequenceDiagram
participant Launch
participant ChildProcess
participant PortManager
participant ExecutionTap
Launch->>ChildProcess: spawn ComfyUI process
Launch->>PortManager: wait for or reserve launch port
ChildProcess->>ExecutionTap: stream stderr and stdout
PortManager-->>Launch: report listener or port conflict
ChildProcess-->>Launch: emit close or exit
Launch->>Launch: retry port or return structured failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/updater.ts (1)
408-417: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack the active updater operation explicitly.
This still derives the operation from cached state, so a revalidation check performed while state is
readyis reported as an installer failure; carry the initiatingcheck,download, orapply_restartoperation into the event handler instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/updater.ts` around lines 408 - 417, Update the updater event handling around the stage calculation and emitDesktopUpdateError call to use the initiating operation captured when the updater action starts, rather than deriving it from _appUpdateState.kind, _autoDownloadTriggeredFor, or wasUserInitiated. Propagate the active check, download, or apply_restart operation into this handler and emit that operation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/lib/executionTap.ts`:
- Around line 68-69: Update EXCEPTION_LINE and the traceback detection logic in
executionTap to recognize final exception lines structurally rather than
requiring class names to end with Error, Exception, Warning, or Interrupt.
Ensure names such as Boom, StopIteration, and ExceptionGroup trigger traceback
flushing through the existing some(...) checks, and add a regression test
covering a non-suffixed exception.
In `@src/main/lib/ipc/sessionActions/launch.test.ts`:
- Around line 81-86: Update the process event test around proc.emit and the
callback assertions to use distinct payloads for the exit and close events, then
assert that the callback is invoked once with the close event’s payload.
Preserve the multiple close emissions to verify the first close result is
preferred over exit and subsequent close events.
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 823-830: Compute the explicit-port policy once in the shared
launch flow, treating a present portOverride as explicit alongside the --port
launch argument. Reuse that shared value in both conflict checks, including the
branches around the existing late-conflict logic, and remove their duplicate
local declarations while preserving the current portConflict behavior.
- Around line 129-134: Update the finish callback in the launch termination flow
to handle rejected promises returned by callback instead of discarding them with
void callback(...). Preserve the existing single-execution guard and timer
cleanup, and ensure callback failures are caught so they do not produce
unhandled rejections or interrupt lifecycle cleanup.
- Around line 1069-1083: In the port-conflict retry flow surrounding tryLaunch,
await killProcessTree(spawned.proc) before returning tryLaunch(). Keep the
existing port release, reassignment, reservation, and retry behavior unchanged,
ensuring the previous process tree is fully terminated before launching the
replacement.
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 48-52: Update the options passed to mainTelemetry.captureException
in the process error handler to merge scrubbed.context alongside origin, source,
and level. Preserve the existing fields and allow captureException to normalize
the merged context so child/render process details are retained.
In `@src/main/lib/updater.ts`:
- Around line 302-317: Update the standard error-field construction in the
telemetry payload to use the original error value rather than only message.
Reuse errorObject when available, preserving Error.name and structured code for
updater failures, while retaining the existing message fallback when no Error
instance exists.
In `@src/renderer/src/lib/rendererBootstrap.ts`:
- Around line 167-174: Replace the renderer-local claimRendererTelemetryBudget
counter with a shared main-process IPC budget, ensuring each telemetry event is
emitted to Datadog only after main acknowledges acceptance. Update the related
renderer telemetry call sites, including the error path, so all
renderer-originated events use this centralized acceptance flow and cannot
bypass the global cap.
In `@src/shared/errorBucket.ts`:
- Around line 26-40: Update the detail extraction in the error-bucketing logic
around the raw input normalization so plain objects with a string record.message
use that value as their detail. Preserve existing Error and string handling,
allowing serialized errors such as { message: 'ECONNRESET' } to contribute to
raw instead of being classified as unknown.
In `@src/shared/errorEvent.ts`:
- Line 176: Update the missing-module replacement regex in error signature
normalization to preserve only explicitly allowlisted safe module identifiers;
replace non-allowlisted identifiers, including private package names, with <str>
to prevent unbounded grouping cardinality.
- Around line 83-93: Restrict structuredCode in src/shared/errorEvent.ts:83-93
to return only recognized Node, Chromium, or errno codes rather than arbitrary
uppercase tokens, including the nested cause path. In
src/shared/errorEvent.test.ts:102-105, add an uppercase private or
high-cardinality code case and assert it resolves to unknown.
---
Outside diff comments:
In `@src/main/lib/updater.ts`:
- Around line 408-417: Update the updater event handling around the stage
calculation and emitDesktopUpdateError call to use the initiating operation
captured when the updater action starts, rather than deriving it from
_appUpdateState.kind, _autoDownloadTriggeredFor, or wasUserInitiated. Propagate
the active check, download, or apply_restart operation into this handler and
emit that operation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10f0495a-0b35-4d22-b440-fa42c2e85238
📒 Files selected for processing (23)
src/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.tssrc/main/lib/executionTap.test.tssrc/main/lib/executionTap.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.test.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/standaloneMigration.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/shared/datadogMirroredEvents.test.tssrc/shared/datadogMirroredEvents.tssrc/shared/errorBucket.tssrc/shared/errorEvent.test.tssrc/shared/errorEvent.tssrc/shared/piiScrub.test.tssrc/shared/piiScrub.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/lib/updater.ts (1)
657-713: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated
sourcelogic into a shared variable.Repeated ternary logic makes maintenance a fright, so share that variable and keep it tight! As per coding guidelines, check for duplicated logic and extract it into shared variables to prevent divergence.
♻️ Proposed refactor
export function installUpdate(userInitiated = true): void { + const updateSource = userInitiated ? 'install_call' : 'startup_install' if (isSessionEnding()) { // The OS is shutting down / logging off. Spawning the installer now risks // it being force-killed mid-write, corrupting the install — the exact // failure this whole flow exists to avoid. The staged update stays put and // applies on the next launch instead. return } const updater = getAutoUpdater() if (!updater) { emitDesktopUpdateError('apply_restart', UPDATER_UNAVAILABLE_MESSAGE, { userInitiated, - source: userInitiated ? 'install_call' : 'startup_install' + source: updateSource }) _broadcastToRenderer('app-update:user-action-failed', { message: UPDATER_UNAVAILABLE_MESSAGE }) return } emitTelemetry('comfy.desktop.app_update.install_triggered', { version: _appUpdateState.version, auto_update_setting: isAutoInstallEnabled() ? 'on' : 'off' }) try { _activeUpdateOperation = { operation: 'apply_restart', - source: userInitiated ? 'install_call' : 'startup_install', + source: updateSource, targetVersion: _appUpdateState.version, userInitiated, startedAt: Date.now() } setQuitReason('update-install') // macOS Squirrel quirk: if requestSingleInstanceLock is still held by // the quitting process, ShipIt swaps the .app bundle correctly but // the new Squirrel.Mac process cannot acquire the lock and exits // silently — the user sees the app close and nothing relaunches. // Electron's own docs call this out: quitAndInstall + single-instance // lock is a known footgun on darwin. Releasing the lock immediately // before restartAndInstall lets the next process come up cleanly. // Windows / Linux update paths don't have this contention and don't // need the release. if (process.platform === 'darwin') { app.releaseSingleInstanceLock() } // `isSilent: false` shows the NSIS progress window during the install (see // `isInstallerUIEnabled` — Windows-only, default on). Forced silent on // macOS/Linux, where `isSilent` has no effect anyway. updater.restartAndInstall({ isSilent: !isInstallerUIEnabled() }) } catch (err) { _activeUpdateOperation = null clearQuitReason() emitDesktopUpdateError('apply_restart', err, { userInitiated, - source: userInitiated ? 'install_call' : 'startup_install' + source: updateSource }) _broadcastToRenderer('app-update:user-action-failed', { message: err instanceof Error ? err.message : String(err) }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/updater.ts` around lines 657 - 713, In installUpdate, extract the repeated userInitiated ? 'install_call' : 'startup_install' expression into one local source variable near the start of the function, then reuse it for _activeUpdateOperation.source and both emitDesktopUpdateError calls. Preserve the existing source values and behavior.Source: Coding guidelines
src/main/lib/telemetry.ts (1)
1013-1016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated stack-length cap — extract a shared constant. Both sinks truncate
error.stackto the same magic16 * 1024, instead of reusing a shared constant the wayERROR_MESSAGE_MAXis already shared between main and renderer. One divergence away from the two caps quietly drifting.
src/main/lib/telemetry.ts#L1013-L1016: replace the inline16 * 1024incaptureExceptionwith a sharedERROR_STACK_MAXconstant (defined alongsideERROR_MESSAGE_MAX).src/renderer/src/lib/rendererBootstrap.ts#L536-L539: replace the inline16 * 1024inreportRendererErrorwith the same sharedERROR_STACK_MAXconstant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/telemetry.ts` around lines 1013 - 1016, Define a shared ERROR_STACK_MAX constant alongside ERROR_MESSAGE_MAX, then update captureException in src/main/lib/telemetry.ts:1013-1016 and reportRendererError in src/renderer/src/lib/rendererBootstrap.ts:536-539 to use it instead of the inline 16 * 1024 cap.Source: Coding guidelines
src/renderer/src/lib/rendererBootstrap.ts (1)
201-212: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
scrubTelemetryContextsilently skips array values — Datadog-bound array fields bypass PII scrubbing.Unlike main-process
scrubProperties(which scrubs each string entry of array values too), this renderer-side helper doesif (typeof value !== 'string') continue, so any array-shaped context value sails through unscrubbed straight intodatadogRum.addAction/addError. Pre-existing gap, but this PR just widened what flows through this exact function (main-process events now round-trip here viaonTelemetryActionFromMain), so the blast radius grew. Bring it in line withscrubPropertiesto keep the two sinks' scrubbing symmetric — no need to let one scrub imp nap while the other one works overtime.🔒 Proposed fix
function scrubTelemetryContext(context: TelemetryContext): TelemetryContext { let mutated: TelemetryContext | null = null for (const key of Object.keys(context)) { const value = context[key] - if (typeof value !== 'string') continue - const cleaned = scrubAll(value) - if (cleaned === value) continue - if (!mutated) mutated = { ...context } - mutated[key] = cleaned + if (typeof value === 'string') { + const cleaned = scrubAll(value) + if (cleaned === value) continue + if (!mutated) mutated = { ...context } + mutated[key] = cleaned + } else if (Array.isArray(value)) { + const cleaned = value.map((entry) => (typeof entry === 'string' ? scrubAll(entry) : entry)) + if (cleaned.every((entry, index) => entry === value[index])) continue + if (!mutated) mutated = { ...context } + mutated[key] = cleaned + } } return mutated ?? context }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/lib/rendererBootstrap.ts` around lines 201 - 212, Update scrubTelemetryContext to scrub array-valued fields as well as direct string values, applying scrubAll to each string element and preserving non-string elements. Keep the existing copy-on-change behavior and return the original context when no values require modification, matching scrubProperties’ handling.
♻️ Duplicate comments (1)
src/main/lib/processErrorHandlers.ts (1)
47-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
scrubbed.contextstill doesn't reach PostHog — and now bypasses it entirely for Datadog.Same gap flagged previously:
mainTelemetry.captureException(...)only receivesorigin/source/level, soexitCode,reason,name,serviceNamefromchild-process-gone/render-process-gonenever land in PostHog. Worse, this diff now spreadsscrubbed.contextintoforwardExceptionToRendererinstead — so the same fields that are missing from the "rich" sink partially leak toward the intentionally-thin Datadog sink, inverting the intended sink priority.🩹 Proposed fix
const accepted = mainTelemetry.captureException(err, { + ...(scrubbed.context || {}), origin: 'main-process', source: scrubbed.source, level: scrubbed.level ?? null })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/processErrorHandlers.ts` around lines 47 - 59, Update the error handling flow around mainTelemetry.captureException and forwardExceptionToRenderer so scrubbed.context is included in the PostHog capture payload, while the renderer/Datadog forwarding payload remains intentionally limited to origin, source, and level. Remove the context spread from forwardExceptionToRenderer and preserve the existing scrubbed field handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/lib/telemetry.ts`:
- Around line 1013-1016: Define a shared ERROR_STACK_MAX constant alongside
ERROR_MESSAGE_MAX, then update captureException in
src/main/lib/telemetry.ts:1013-1016 and reportRendererError in
src/renderer/src/lib/rendererBootstrap.ts:536-539 to use it instead of the
inline 16 * 1024 cap.
In `@src/main/lib/updater.ts`:
- Around line 657-713: In installUpdate, extract the repeated userInitiated ?
'install_call' : 'startup_install' expression into one local source variable
near the start of the function, then reuse it for _activeUpdateOperation.source
and both emitDesktopUpdateError calls. Preserve the existing source values and
behavior.
In `@src/renderer/src/lib/rendererBootstrap.ts`:
- Around line 201-212: Update scrubTelemetryContext to scrub array-valued fields
as well as direct string values, applying scrubAll to each string element and
preserving non-string elements. Keep the existing copy-on-change behavior and
return the original context when no values require modification, matching
scrubProperties’ handling.
---
Duplicate comments:
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 47-59: Update the error handling flow around
mainTelemetry.captureException and forwardExceptionToRenderer so
scrubbed.context is included in the PostHog capture payload, while the
renderer/Datadog forwarding payload remains intentionally limited to origin,
source, and level. Remove the context spread from forwardExceptionToRenderer and
preserve the existing scrubbed field handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d7842d6-ef51-4301-9643-dca862c29c56
📒 Files selected for processing (11)
src/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/shared/datadogMirroredEvents.test.tssrc/shared/datadogMirroredEvents.ts
|
Addressed the actionable outside-diff review findings:
Validation: full typecheck, lint, build, and test suites pass. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/ipc/sessionActions/launch.ts (1)
771-809: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract duplicated process exit logic.
The termination handling logic here is identical to the logic in
attachExitHandlerat lines 1359-1395 (aside fromgetStderr()vscurrentGetStderr()). As per coding guidelines, extract this duplicated logic into a shared helper function to prevent divergence. Don't be a goblin hoarding duplicate code! Extracting it makes the codebase gleam and puts less stress on the team.♻️ Proposed helper extraction
Define a helper function in
handleLaunch(above theskipPortWaitblock):async function handleProcessExited( code: number | null, signal: NodeJS.Signals | null, getStderrFn: () => string, logStream: WriteStream, execTap: ReturnType<typeof createExecutionTap>, hwTap: ReturnType<typeof createHardwareTap> ) { logStream.end() const crashed = _runningSessions.has(installationId) && isCrashedExit(code, signal) const lastStderr = lastNLines(getStderrFn(), 100) execTap.flushSummary() hwTap.flushSummary() const crashDiagnosis = crashed ? await diagnoseCrash(code) : {} _removeSession(installationId) const exitedPayload = { installationId, crashed, exitCode: code ?? undefined, signal: signal ?? undefined, installationName: inst.name, lastStderr, ...crashDiagnosis } telemetry.emit('comfy.desktop.comfyui.exited', { installation_id: installationId, crashed, exit_code: code ?? null, last_stderr: lastStderr ?? null }) if (crashed) { recordCrash(exitedPayload) _broadcastToRenderer('instance-crashed', exitedPayload) } if (!sender.isDestroyed()) { sender.send('comfy-exited', exitedPayload) } if (_onComfyExited) _onComfyExited({ installationId, crashed }) }Then replace this block (and the matching block in
attachExitHandlerat lines 1359-1395) with a single call:onProcessTerminated(proc, async (code, signal) => { await handleProcessExited(code, signal, getStderr, logStream, execTap, hwTap) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/ipc/sessionActions/launch.ts` around lines 771 - 809, Extract the shared termination handling from the current exit callback and attachExitHandler into a local async handleProcessExited helper within handleLaunch, accepting the exit details, stderr getter, log stream, and execution/hardware taps. Move the common cleanup, crash diagnosis, payload creation, telemetry, crash recording, renderer notification, and _onComfyExited calls into that helper, preserving getStderr versus currentGetStderr and ending the corresponding log stream. Replace both duplicated blocks with calls to handleProcessExited through onProcessTerminated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/lib/executionTap.ts`:
- Around line 68-69: Update the EXCEPTION_LINE regular expression in
executionTap.ts to allow uppercase letters in dotted module-name prefixes, while
preserving the existing matching behavior for exception class names and trailing
colon/whitespace.
In `@src/main/lib/process.ts`:
- Around line 91-103: Bound the retry loop in waitForGroupExit by tracking poll
attempts or elapsed time and stopping after a defined maximum, including
persistent EPERM cases. When the limit is reached, call done() so the launch
lifecycle cannot hang indefinitely; preserve the existing polling behavior
before the cap.
- Around line 193-197: In src/main/lib/process.ts lines 193-197, update
waitForPort to use a per-attempt boolean guard so the error and timeout handlers
schedule poll only once, including when req.destroy() emits error synchronously,
and ignore late errors after success. Apply the identical guard in waitForUrl at
src/main/lib/process.ts lines 232-236.
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 54-58: Update the mainTelemetry.forwardExceptionToRenderer call in
process error handling to include scrubbed.context alongside origin, source, and
level, preserving the same merged context already used by captureException so
Datadog receives details such as exitCode and reason.
---
Outside diff comments:
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 771-809: Extract the shared termination handling from the current
exit callback and attachExitHandler into a local async handleProcessExited
helper within handleLaunch, accepting the exit details, stderr getter, log
stream, and execution/hardware taps. Move the common cleanup, crash diagnosis,
payload creation, telemetry, crash recording, renderer notification, and
_onComfyExited calls into that helper, preserving getStderr versus
currentGetStderr and ending the corresponding log stream. Replace both
duplicated blocks with calls to handleProcessExited through onProcessTerminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c223db9c-0f00-45eb-b06e-12400b645bd7
📒 Files selected for processing (15)
src/main/lib/executionTap.test.tssrc/main/lib/executionTap.tssrc/main/lib/ipc/sessionActions/launch.test.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/process.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/renderer/src/lib/telemetry.tssrc/shared/errorBucket.tssrc/shared/errorEvent.test.tssrc/shared/errorEvent.ts
| const EXCEPTION_LINE = | ||
| /^(?:[A-Z_][A-Za-z0-9_]*|(?:[a-z_][A-Za-z0-9_]*\.)+[A-Z_][A-Za-z0-9_]*)(?::(?:\s|$)|$)/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow uppercase module names in exception traceback matching.
Like a shy imp, uppercase modules hide from this regex! 😈
Restricting the module prefix matching to [a-z_] means valid exceptions originating from capitalized packages (like PIL.UnidentifiedImageError) will be silently ignored as exception lines. Consider relaxing the module prefix check to allow both upper and lowercase letters.
🛠️ Proposed fix to catch all the exceptions
- /^(?:[A-Z_][A-Za-z0-9_]*|(?:[a-z_][A-Za-z0-9_]*\.)+[A-Z_][A-Za-z0-9_]*)(?::(?:\s|$)|$)/
+ /^(?:[A-Za-z_][A-Za-z0-9_]*\.)*[A-Z_][A-Za-z0-9_]*(?::(?:\s|$)|$)/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const EXCEPTION_LINE = | |
| /^(?:[A-Z_][A-Za-z0-9_]*|(?:[a-z_][A-Za-z0-9_]*\.)+[A-Z_][A-Za-z0-9_]*)(?::(?:\s|$)|$)/ | |
| const EXCEPTION_LINE = | |
| /^(?:[A-Za-z_][A-Za-z0-9_]*\.)*[A-Z_][A-Za-z0-9_]*(?::(?:\s|$)|$)/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/lib/executionTap.ts` around lines 68 - 69, Update the EXCEPTION_LINE
regular expression in executionTap.ts to allow uppercase letters in dotted
module-name prefixes, while preserving the existing matching behavior for
exception class names and trailing colon/whitespace.
| const waitForGroupExit = (): void => { | ||
| try { | ||
| process.kill(processGroup, 0) | ||
| setTimeout(waitForGroupExit, 25).unref() | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === 'EPERM') { | ||
| setTimeout(waitForGroupExit, 25).unref() | ||
| } else { | ||
| done() | ||
| } | ||
| } | ||
| } | ||
| waitForGroupExit() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Bound the process group exit poll.
An unkillable process (e.g., stuck in uninterruptible sleep or returning persistent EPERM) will trap this poll in an infinite loop, permanently hanging the launch lifecycle. You don't want this loop haunting you forever! Cap the poll, take control, and let the launch achieve its goal.
🛡️ Proposed fix to add a retry cap
- const waitForGroupExit = (): void => {
+ let attempts = 0
+ const waitForGroupExit = (): void => {
+ if (++attempts > 40) return done() // Cap at ~1s
try {
process.kill(processGroup, 0)
setTimeout(waitForGroupExit, 25).unref()
} catch (err) {
if ((err as NodeJS.ErrnoException).code === 'EPERM') {
setTimeout(waitForGroupExit, 25).unref()
} else {
done()
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const waitForGroupExit = (): void => { | |
| try { | |
| process.kill(processGroup, 0) | |
| setTimeout(waitForGroupExit, 25).unref() | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === 'EPERM') { | |
| setTimeout(waitForGroupExit, 25).unref() | |
| } else { | |
| done() | |
| } | |
| } | |
| } | |
| waitForGroupExit() | |
| let attempts = 0 | |
| const waitForGroupExit = (): void => { | |
| if (++attempts > 40) return done() // Cap at ~1s | |
| try { | |
| process.kill(processGroup, 0) | |
| setTimeout(waitForGroupExit, 25).unref() | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === 'EPERM') { | |
| setTimeout(waitForGroupExit, 25).unref() | |
| } else { | |
| done() | |
| } | |
| } | |
| } | |
| waitForGroupExit() |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, execFile, type ChildProcess } from 'child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/lib/process.ts` around lines 91 - 103, Bound the retry loop in
waitForGroupExit by tracking poll attempts or elapsed time and stopping after a
defined maximum, including persistent EPERM cases. When the limit is reached,
call done() so the launch lifecycle cannot hang indefinitely; preserve the
existing polling behavior before the cap.
| req.on('error', () => setTimeout(poll, intervalMs)) | ||
| req.on('timeout', () => { | ||
| req.destroy() | ||
| setTimeout(poll, intervalMs) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard polling against double-fire on timeout.
When req.destroy() is called to abort a timed-out request, it synchronously emits an error event. Without an idempotency guard, both the timeout and error handlers schedule poll independently, creating an exponentially multiplying fork bomb of background polling requests. Let's cage that mischievous multiplying beast, before it turns your network traffic into a feast!
src/main/lib/process.ts#L193-L197: Use a boolean guard inwaitForPortto ensurepollis only scheduled once per attempt and blocks out late errors after success.src/main/lib/process.ts#L232-L236: Apply the identical guard inwaitForUrl.
🐛 Proposed fix for both locations
+ let handled = false
+ const retry = () => {
+ if (handled) return
+ handled = true
+ setTimeout(poll, intervalMs)
+ }
const req = http.get({ host, port, path: '/', timeout: 2000 }, (res) => {
res.resume()
+ if (handled) return
+ handled = true
resolve()
})
- req.on('error', () => setTimeout(poll, intervalMs))
- req.on('timeout', () => {
- req.destroy()
- setTimeout(poll, intervalMs)
- })
+ req.on('error', retry)
+ req.on('timeout', () => {
+ req.destroy()
+ retry()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| req.on('error', () => setTimeout(poll, intervalMs)) | |
| req.on('timeout', () => { | |
| req.destroy() | |
| setTimeout(poll, intervalMs) | |
| }) | |
| let handled = false | |
| const retry = () => { | |
| if (handled) return | |
| handled = true | |
| setTimeout(poll, intervalMs) | |
| } | |
| req.on('error', retry) | |
| req.on('timeout', () => { | |
| req.destroy() | |
| retry() | |
| }) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, execFile, type ChildProcess } from 'child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 1 file
src/main/lib/process.ts#L193-L197(this comment)src/main/lib/process.ts#L232-L236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/lib/process.ts` around lines 193 - 197, In src/main/lib/process.ts
lines 193-197, update waitForPort to use a per-attempt boolean guard so the
error and timeout handlers schedule poll only once, including when req.destroy()
emits error synchronously, and ignore late errors after success. Apply the
identical guard in waitForUrl at src/main/lib/process.ts lines 232-236.
| mainTelemetry.forwardExceptionToRenderer({ | ||
| origin: 'main-process', | ||
| source: scrubbed.source, | ||
| level: scrubbed.level ?? null | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve scrubbed.context when forwarding to Datadog.
Poof! The context vanishes like a ghost in the night! 👻
While you successfully merged scrubbed.context for PostHog in captureException, Datadog relies on forwardExceptionToRenderer to receive the same details. Omitting it here means Datadog loses crucial context like exitCode and reason for child/renderer process crashes.
✨ Proposed fix to keep the context intact
if (accepted) {
mainTelemetry.forwardExceptionToRenderer({
+ ...(scrubbed.context || {}),
origin: 'main-process',
source: scrubbed.source,
level: scrubbed.level ?? null
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mainTelemetry.forwardExceptionToRenderer({ | |
| origin: 'main-process', | |
| source: scrubbed.source, | |
| level: scrubbed.level ?? null | |
| }) | |
| mainTelemetry.forwardExceptionToRenderer({ | |
| ...(scrubbed.context || {}), | |
| origin: 'main-process', | |
| source: scrubbed.source, | |
| level: scrubbed.level ?? null | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/lib/processErrorHandlers.ts` around lines 54 - 58, Update the
mainTelemetry.forwardExceptionToRenderer call in process error handling to
include scrubbed.context alongside origin, source, and level, preserving the
same merged context already used by captureException so Datadog receives details
such as exitCode and reason.
1 participant
Summary
closeevents without hanging on inherited pipescheck,download, orapply_restart), running/target versions, provider, error source, stack, and Chinese-mirror cohortValidation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test(188 files, 2826 tests)Closes #1268
Closes #1269