From f2b01eff64db5ef0c43fa81947aabde09481314b Mon Sep 17 00:00:00 2001 From: Will Washburn Date: Wed, 10 Jun 2026 00:25:00 -0400 Subject: [PATCH] Make swallowed errors visible: log or justify every silent catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit follow-up: every silently-swallowed error in src/main and src/renderer non-test code now either emits a categorized, context-bearing log (bracket-prefix style per AGENTS.md) or carries a one-line comment justifying why silence is correct. No control-flow, retry, or error-propagation behavior was changed — logging only. High-frequency catches (PTY/event/poll loops) are kept silent with a justification, and the polled burn fingerprint log is time-throttled so the telemetry stays low-noise. src/main/integrations.ts is intentionally untouched (edited in parallel on another branch); its findings are deferred to a follow-up in the PR. Co-Authored-By: Claude Opus 4.8 --- src/main/broker.ts | 56 ++++++++++++++++--- src/main/burn.ts | 20 ++++++- src/main/cloud-agent.ts | 30 ++++++++-- .../components/agents/CloudAgentPicker.tsx | 3 + .../components/broker/BrokerDetailsPage.tsx | 3 + .../components/settings/ProjectSettings.tsx | 5 ++ .../src/components/sidebar/ProjectSidebar.tsx | 5 ++ src/renderer/src/hooks/use-terminal.ts | 4 +- .../src/lib/terminal-runtime-registry.ts | 15 +++-- src/renderer/src/stores/git-store.ts | 3 + src/renderer/src/stores/issues-store.ts | 3 + 11 files changed, 127 insertions(+), 20 deletions(-) diff --git a/src/main/broker.ts b/src/main/broker.ts index a8e19238..e7b6e2db 100644 --- a/src/main/broker.ts +++ b/src/main/broker.ts @@ -412,6 +412,8 @@ child.on('exit', (code, signal) => { try { process.kill(process.pid, signal) } catch { + // Re-raising the child's signal failed; exit non-zero so the failure still + // propagates to the parent. Nothing to log from inside this shim process. process.exit(1) } return @@ -1011,6 +1013,7 @@ function parsePortFromUrl(url: string | undefined): number | undefined { } return parsed.protocol === 'https:' ? 443 : parsed.protocol === 'http:' ? 80 : undefined } catch { + // Unparseable URL -> undefined port; the parse error carries no extra signal. return undefined } } @@ -1073,6 +1076,8 @@ function readBrokerConnectionFileInfo( apiKey: parsed.data.apiKey } } catch { + // Read/JSON error -> treat as an invalid connection file; the 'invalid' + // status propagates to callers, so the underlying error adds nothing. return { path: connectionPath, status: 'invalid', hasApiKey: false } } } @@ -1768,6 +1773,8 @@ export class BrokerManager { const normalizedProjectId = projectId.trim() if (!normalizedProjectId) return undefined const startPromise = this.startPromises.get(normalizedProjectId) + // Only awaiting the in-flight start as a gate; its own owner surfaces start + // failures. Here we just fall through to read whatever session exists. if (startPromise) await startPromise.catch(() => undefined) const session = this.sessions.get(normalizedProjectId) if (!session) return undefined @@ -1810,6 +1817,8 @@ export class BrokerManager { // lets getOrAwaitSessionsForProject await an in-flight attach. const sessionKey = cloudSessionKey(normalizedProjectId) const inFlightAttach = this.startPromises.get(sessionKey) + // Gate only: serialize behind an in-flight attach. That attach's own caller + // surfaces its failure; we ignore the outcome and proceed to re-attach. if (inFlightAttach) await inFlightAttach.catch(() => undefined) let releaseAttachGate!: () => void const attachGate = new Promise((resolve) => { @@ -1824,8 +1833,10 @@ export class BrokerManager { if (previous) { try { await previous.client.shutdown() - } catch { - // Ignore shutdown errors. + } catch (err) { + // Non-fatal: we replace the session regardless, but a failed shutdown + // can leak the previous cloud sandbox, so make it visible. + console.warn(`[broker] Failed to shut down previous cloud session for project ${normalizedProjectId}:`, err) } this.dropSession(sessionKey, { disconnectOnly: false }) } @@ -1990,6 +2001,10 @@ export class BrokerManager { private async getOrAwaitSessionsForProject(projectId: string): Promise { const normalizedProjectId = projectId.trim() + // These three awaits are gates: we wait for any in-flight revive/start/attach + // to settle so the session map is populated, then read it below. Each + // operation's own owner surfaces its failure; ignoring it here is correct, + // and the `if (!sessions.length)` check turns a real failure into a throw. const revivePromise = this.revivePromises.get(normalizedProjectId) if (revivePromise) { await revivePromise.catch(() => undefined) @@ -2388,7 +2403,11 @@ export class BrokerManager { if (event.kind === 'agent_spawned' && event.name) { this.rememberAgentSession(event.name, sessionKey) if (event.parent) { - void this.handleSpawnedChildLineage(sessionKey, event) + void this.handleSpawnedChildLineage(sessionKey, event).catch((err) => { + // Lineage recording is best-effort (burn enrichment), but a silent + // rejection here loses parent/child attribution — log it. + console.warn(`[broker] Failed to record spawned-child lineage for ${event.name}:`, err) + }) } } else if (event.kind === 'agent_exit' && event.name) { this.closeInputStream(this.getInputStreamKey(sessionKey, event.name), 1000, 'agent closed') @@ -2719,7 +2738,12 @@ export class BrokerManager { ) for (const sibling of this.sessionsForProject(session.projectId)) { if (sibling === session) continue - const siblingAgents = await sibling.client.listAgents().catch(() => []) + const siblingAgents = await sibling.client.listAgents().catch((err) => { + // Best-effort: missing a sibling's names risks a relay-side collision, + // so log, but keep spawning rather than blocking on the sibling broker. + console.warn(`[broker] Failed to list sibling agents for project ${session.projectId} during spawn dedupe:`, err) + return [] + }) for (const agent of siblingAgents) existingNames.add(agent.name) } let nextInput = { @@ -2978,6 +3002,10 @@ export class BrokerManager { const deadline = Date.now() + parsePositiveIntegerEnv('PEAR_ATTACH_REGISTRATION_TIMEOUT_MS', 15_000) while (Date.now() < deadline) { for (const candidate of candidates) { + // Silent by design: this probes both brokers every 250ms until the + // deadline, and a not-yet-registered agent throws transiently here. + // Logging would flood; the `registered: false` return surfaces the + // real outcome to the caller. const agents = await candidate.client.listAgents().catch(() => null) if (agents?.some((agent) => agent.name === name)) { this.rememberAgentSession(name, sessionKeyFor(candidate)) @@ -3245,6 +3273,8 @@ export class BrokerManager { let idleSince: number | null = null while (Date.now() - startedAt < timeoutMs) { + // Silent by design: tight idle-poll loop; a transient getStatus failure + // is treated as "agent gone" (return) and logging each miss would flood. const agent = await session.client.getStatus() .then((status) => status.agents.find((entry) => entry.name === name)) .catch(() => undefined) @@ -3334,6 +3364,8 @@ export class BrokerManager { } } + // Pending count is a non-critical UI hint; 0 on failure is a safe default + // and the snapshot call right below surfaces any real attach failure. const pending = mode === 'manual_flush' ? await this.withAgentMissingRetry('getPending', name, () => client.getPending(name)).then((messages) => messages.length).catch(() => 0) : 0 @@ -3479,6 +3511,8 @@ export class BrokerManager { } throw err } + // Pending count is a non-critical UI hint; 0 on failure is a safe default + // (the setInboundDeliveryMode above already succeeded for this to run). const pending = result.mode === 'manual_flush' ? await session.client.getPending(trimmedName).then((messages) => messages.length).catch(() => 0) : 0 @@ -3933,6 +3967,8 @@ export class BrokerManager { const brokerKind = session.cloudSandboxId ? ('cloud' as const) : ('local' as const) return Promise.all( agents.map(async (agent) => { + // Best-effort enrichment on every listAgents poll: undefined just omits + // the delivery-mode badge. Logging per-agent per-poll would flood. const inboundDeliveryMode = await session.client.getInboundDeliveryMode(agent.name).catch(() => undefined) return { ...agent, projectId: session.projectId, inboundDeliveryMode, brokerKind } }) @@ -4070,8 +4106,10 @@ export class BrokerManager { for (const session of sessions) { try { await session.client.shutdown() - } catch { - // Ignore shutdown errors. + } catch (err) { + // Non-fatal during teardown, but a failed cloud-session shutdown can + // leak a sandbox — log so it is not invisible. + console.warn(`[broker] Failed to shut down session for project ${targetProjectId}:`, err) } this.dropSession(sessionKeyFor(session), { disconnectOnly: false }) } @@ -4122,8 +4160,10 @@ export class BrokerManager { if (!session) return try { await session.client.shutdown() - } catch { - // Ignore shutdown errors. + } catch (err) { + // Non-fatal: we drop the session anyway, but a failed shutdown can leak + // the detached cloud sandbox, so surface it. + console.warn(`[broker] Failed to shut down cloud session on detach for project ${normalizedProjectId}:`, err) } this.dropSession(sessionKey, { disconnectOnly: false }) // Only report the project as disconnected when nothing is left serving diff --git a/src/main/burn.ts b/src/main/burn.ts index b497e02b..74e6f64c 100644 --- a/src/main/burn.ts +++ b/src/main/burn.ts @@ -223,6 +223,10 @@ const MAX_SESSIONS_FOR_DETAILS = 12 const MAX_ROWS = 12 let warnedUnavailable = false +// getFingerprint is polled (~every 15s) by burn views, so throttle its failure +// log to stay low-noise when the ledger is persistently unavailable. +const FINGERPRINT_WARN_THROTTLE_MS = 60_000 +let lastFingerprintWarnAt = 0 const requireForResolve = createRequire(import.meta.url) const BURN_INGEST_TOOL_RESULT_WARNING_START = @@ -820,7 +824,12 @@ class BurnManager { ? await burn.fingerprint({ session: input.sessionId, ledgerHome }) : await burn.fingerprint({ ledgerHome }) return { fingerprint: res.fingerprint } - } catch { + } catch (err) { + const now = Date.now() + if (now - lastFingerprintWarnAt > FINGERPRINT_WARN_THROTTLE_MS) { + lastFingerprintWarnAt = now + console.warn(`[burn] Failed to compute ledger fingerprint${input.sessionId ? ` for session ${input.sessionId}` : ''}:`, toErrorMessage(err)) + } return { fingerprint: '' } } } @@ -953,7 +962,10 @@ class BurnManager { turnCount: totals.turnCount } }) - } catch { + } catch (err) { + // Recoverable: fall back to per-agent summaries below. Log so a + // consistently failing batch summary (the fast path) stays visible. + console.warn(`[burn] Batch agent summary failed for project ${projectId}; falling back to per-agent summaries:`, toErrorMessage(err)) return Promise.all(agents.map((agent) => this.getAgentSummary(agent))) } } @@ -1096,6 +1108,8 @@ class BurnManager { * view queries a warm, recently-ingested ledger instead of stalling on it. */ warmUp(): void { + // Fire-and-forget is safe: ingestRecent() catches its own errors and logs + // them once via the warnedUnavailable guard, so it never rejects unhandled. void this.ingestRecent() } @@ -1110,6 +1124,8 @@ class BurnManager { if (force) { await this.ingestRecent(true) } else { + // Fire-and-forget is safe: ingestRecent() self-handles and once-logs its + // own errors (warnedUnavailable guard), so it never rejects unhandled. void this.ingestRecent() } } diff --git a/src/main/cloud-agent.ts b/src/main/cloud-agent.ts index 2f711b7c..96e20031 100644 --- a/src/main/cloud-agent.ts +++ b/src/main/cloud-agent.ts @@ -426,6 +426,8 @@ function normalizeHttpsGitRemote(rawUrl: string): string | null { url.hash = '' return url.toString() } catch { + // Unparseable remote → null is the documented "not a usable git remote" + // sentinel the callers branch on; the parse error itself carries no extra info. return null } } @@ -888,7 +890,11 @@ export class CloudAgentManager { await this.throwIfAttachCanceled(projectId, cloudAgentId, sandbox) } catch (error) { await this.stopMount(projectId) - await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch(() => undefined) + await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch((cleanupError) => { + // Best-effort cleanup: the mount/connect error below is what we throw, + // but a failed box delete leaks a cloud sandbox, so surface it. + console.warn(`[cloud-agent] Failed to delete sandbox box after attach failure for project ${projectId} (cloud agent ${cloudAgentId}):`, toErrorMessage(cleanupError)) + }) throw error } @@ -919,7 +925,11 @@ export class CloudAgentManager { ): Promise { if (!this.canceledAttaches.has(projectId)) return this.canceledAttaches.delete(projectId) - await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch(() => undefined) + await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch((cleanupError) => { + // Cancel path: a failed box delete leaks a cloud sandbox; log it but + // still throw the cancellation so the original control flow is unchanged. + console.warn(`[cloud-agent] Failed to delete sandbox box after attach cancel for project ${projectId} (cloud agent ${cloudAgentId}):`, toErrorMessage(cleanupError)) + }) throw new Error('Cloud agent attach canceled') } @@ -1102,7 +1112,10 @@ export class CloudAgentManager { let sandbox = await this.fetchBox(url, auth.accessToken, 'POST', mountPaths, workspaceSource, relayBroker) options.onSandbox?.(sandbox) if (options.isCancelled?.()) { - await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch(() => undefined) + await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch((cleanupError) => { + // Warm-cancel cleanup: log a leaked sandbox without changing the throw. + console.warn(`[cloud-agent] Failed to delete sandbox box after warm cancel for project ${projectId} (cloud agent ${cloudAgentId}):`, toErrorMessage(cleanupError)) + }) throw new Error('Cloud agent sandbox warm canceled') } console.log(`[cloud-agent] warming sandbox ${sandbox.sandboxId} for project ${projectId}: initial status=${sandbox.status}`) @@ -1129,7 +1142,10 @@ export class CloudAgentManager { sandbox = await this.fetchBox(url, fetchAccessToken, 'GET') options.onSandbox?.(sandbox) if (options.isCancelled?.()) { - await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch(() => undefined) + await this.deleteBox(toBinding(projectId, cloudAgentId, sandbox)).catch((cleanupError) => { + // Warm-cancel cleanup: log a leaked sandbox without changing the throw. + console.warn(`[cloud-agent] Failed to delete sandbox box after warm cancel for project ${projectId} (cloud agent ${cloudAgentId}):`, toErrorMessage(cleanupError)) + }) throw new Error('Cloud agent sandbox warm canceled') } this.emitSandboxStatus(projectId, sandbox) @@ -1270,6 +1286,9 @@ export class CloudAgentManager { onEvent: (event) => { input.onEvent?.(event) if (event.type === 'reconcile' || event.type === 'status') { + // Fire-and-forget is safe: emitMountStatus swallows mount.status() + // failures internally (-> null) and emit() is synchronous, so it + // never rejects. This fires per mount event, so a log here would flood. void this.emitMountStatus(projectId) } } @@ -1409,6 +1428,9 @@ export class CloudAgentManager { private schedulePullAfterRun(projectId: string): void { this.clearPullTimer(projectId) const timer = setTimeout(() => { + // Fire-and-forget is safe: maybePullAfterRun wraps its git work in a + // try/catch that surfaces failures via emit({ type: 'error' }), so the + // promise never rejects unhandled. void this.maybePullAfterRun(projectId) }, RUN_END_PULL_DELAY_MS) this.pullTimers.set(projectId, timer) diff --git a/src/renderer/src/components/agents/CloudAgentPicker.tsx b/src/renderer/src/components/agents/CloudAgentPicker.tsx index 510352cd..95233e96 100644 --- a/src/renderer/src/components/agents/CloudAgentPicker.tsx +++ b/src/renderer/src/components/agents/CloudAgentPicker.tsx @@ -271,6 +271,9 @@ export default function CloudAgentPicker({ const previous = prewarmTargetRef.current if (previous === agentId) return if (previous) { + // Fire-and-forget cancel of the superseded prewarm target. A failed + // cancel is harmless here — the box self-expires — and the main process + // logs real deletion failures; surfacing it in the picker would be noise. void pear.cloudAgent.cancelPrewarm(projectId, previous).catch(() => undefined) } prewarmTargetRef.current = agentId diff --git a/src/renderer/src/components/broker/BrokerDetailsPage.tsx b/src/renderer/src/components/broker/BrokerDetailsPage.tsx index 87ec7316..3d2dfbd3 100644 --- a/src/renderer/src/components/broker/BrokerDetailsPage.tsx +++ b/src/renderer/src/components/broker/BrokerDetailsPage.tsx @@ -1024,6 +1024,9 @@ export function BrokerDetailsPage(): React.ReactNode { setDetailsError(null) try { if (typeof pear.broker.listEvents === 'function') { + // Best-effort backfill of historical events alongside the live stream; + // on failure the page still renders from listDetails below, and the + // main process logs the underlying broker error. pear.broker.listEvents() .then(hydrateBrokerEvents) .catch(() => undefined) diff --git a/src/renderer/src/components/settings/ProjectSettings.tsx b/src/renderer/src/components/settings/ProjectSettings.tsx index ec822455..6e9a3cb8 100644 --- a/src/renderer/src/components/settings/ProjectSettings.tsx +++ b/src/renderer/src/components/settings/ProjectSettings.tsx @@ -624,6 +624,9 @@ function IntegrationVisibilitySection({ if (optionChannels.length > 0) return optionChannels } catch (err) { console.warn('[integrations] Failed to list Slack channel options:', err) + // The primary failure (err) was just logged and is rethrown below if + // this fallback is empty; a failed mounted-channel read here is a + // secondary fallback, so [] without its own log is correct. const mountedChannels = await listMountedSlackChannels().catch(() => []) if (mountedChannels.length > 0) return mountedChannels const message = err instanceof Error ? err.message : String(err) @@ -637,6 +640,8 @@ function IntegrationVisibilitySection({ }) if (remoteChannels.length > 0) return remoteChannels + // Last-resort fallback: an empty channel list is the correct "nothing to + // show" UI state, and the remote-list failure above is already logged. return listMountedSlackChannels().catch(() => []) }) }, [cachedResources, projectId]) diff --git a/src/renderer/src/components/sidebar/ProjectSidebar.tsx b/src/renderer/src/components/sidebar/ProjectSidebar.tsx index 1e33c5ed..142a21b0 100644 --- a/src/renderer/src/components/sidebar/ProjectSidebar.tsx +++ b/src/renderer/src/components/sidebar/ProjectSidebar.tsx @@ -187,6 +187,9 @@ function AccountMenu({ compact = false }: { compact?: boolean }): React.ReactNod useEffect(() => { pear.auth.status().then(setAuth) + // Initial recovery-state probe is best-effort: on failure we leave the + // banner hidden, and the integration-auth event listener below will set it + // if recovery is actually needed. pear.integrations.authRecoveryState().then(setAuthRecovery).catch(() => undefined) }, []) @@ -212,6 +215,8 @@ function AccountMenu({ compact = false }: { compact?: boolean }): React.ReactNod try { const result = await pear.auth.login() setAuth(result.loggedIn ? await pear.auth.status() : result) + // Post-login recovery probe is best-effort: null just clears the banner, + // which is the correct post-login default if the probe itself fails. setAuthRecovery(await pear.integrations.authRecoveryState().catch(() => null)) } finally { setLoading(false) diff --git a/src/renderer/src/hooks/use-terminal.ts b/src/renderer/src/hooks/use-terminal.ts index b1562e7c..9d793017 100644 --- a/src/renderer/src/hooks/use-terminal.ts +++ b/src/renderer/src/hooks/use-terminal.ts @@ -300,7 +300,9 @@ export function useTerminal( runtime.refreshOnShow() if (wasPinned) runtime.term.scrollToBottom() } catch { - // ignore + // Visibility-change repaint/fit is best-effort: xterm can throw if the + // term was disposed between the event firing and this handler running. + // A failed redraw self-corrects on the next fit, so silence is correct. } // Intentionally do NOT call term.focus() on visibility change. // When the PTY application has enabled DECSET ?1004 (focus events) — diff --git a/src/renderer/src/lib/terminal-runtime-registry.ts b/src/renderer/src/lib/terminal-runtime-registry.ts index f6a09981..1ff66b3a 100644 --- a/src/renderer/src/lib/terminal-runtime-registry.ts +++ b/src/renderer/src/lib/terminal-runtime-registry.ts @@ -355,7 +355,8 @@ function createRuntime( try { addon.dispose() } catch { - // ignore + // Disposing an addon whose WebGL context is already lost can throw; + // we are tearing it down anyway, so there is nothing to recover or report. } if (webglAddon === addon) webglAddon = null }) @@ -547,7 +548,8 @@ function createRuntime( try { term.refresh(0, term.rows - 1) } catch { - // ignore + // A repaint can throw if the term was disposed between scheduling and + // running this fit; benign for a redraw, nothing to recover. } // Post-settle metrics may differ from the pre-settle ones the // predictor was constructed with. Sync it so column wraps and row @@ -615,13 +617,15 @@ function createRuntime( try { webglAddon?.dispose() } catch { - // ignore + // Teardown: an already-disposed or context-lost WebGL addon can throw + // on dispose; we null it out next regardless, so silence is correct. } webglAddon = null try { term?.dispose() } catch { - // ignore + // Teardown: disposing an xterm instance twice (or after its host was + // detached) can throw; we null it out next regardless, so silence is correct. } term = null if (host.parentElement) { @@ -667,7 +671,8 @@ function createRuntime( try { term.refresh(0, term.rows - 1) } catch { - // ignore + // A forced repaint can throw if the term was disposed; benign for a + // redraw request, nothing to recover. } }, setInputSrttGetter(getter: () => number | null): void { diff --git a/src/renderer/src/stores/git-store.ts b/src/renderer/src/stores/git-store.ts index bbd6a9e3..1f9164c7 100644 --- a/src/renderer/src/stores/git-store.ts +++ b/src/renderer/src/stores/git-store.ts @@ -184,6 +184,9 @@ export const useGitStore = create((set, get) => ({ projectStatusRequests.add(rootPathKey) try { const entries = await Promise.all(rootPaths.map(async (rootPath) => { + // Per-root git reads degrade independently: a non-repo or transiently + // locked root falls back to empty/null so one bad root can't blank the + // whole multi-root status sweep. The main process logs the real git error. const [files, summary] = await Promise.all([ pear.git.status(rootPath).catch(() => [] as FileStatus[]), pear.git.summary(rootPath).catch(() => null) diff --git a/src/renderer/src/stores/issues-store.ts b/src/renderer/src/stores/issues-store.ts index 401c21af..e572fc39 100644 --- a/src/renderer/src/stores/issues-store.ts +++ b/src/renderer/src/stores/issues-store.ts @@ -189,6 +189,9 @@ async function readGithubLink(projectId: string, sync: Record): const path = githubPathForSyncedRecord(sync) if (!path) return { owner, repo, number, type: fallbackType } + // Best-effort enrichment: if the remote file can't be read we fall back to + // the derived type below. null is also the "not text" path, so a read error + // needs no separate handling here. const preview = await pear.integrations.readRemoteFile(projectId, path).catch(() => null) if (!preview || preview.kind !== 'text') return { owner, repo, number, type: fallbackType } const parsed = parseJsonPreview(preview.content, path)