Comfy-Org / Comfy-Org/ComfyUI_frontend

reportError: undeliverable cloud reports permanently occupy the pending buffer

Open
#17,429 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

On cloud builds, a report that can never reach Sentry is re-enqueued on every flush and permanently occupies one of the 25 pending slots. Enough of them and `reportError()` silently stops buffering early-boot failures — the exact case the buffer exists for.

### Mechanism

`src/platform/telemetry/reportError.ts:142` re-enqueues whenever a required sink has not confirmed delivery:

```ts
const sentryDelivered = report.sentryDelivered || delivered.sentry
const datadogDelivered = report.datadogDelivered || delivered.datadog
if (isCloud && (!sentryDelivered || !datadogDelivered)) {
enqueuePendingReport({ error, options, sentryDelivered, datadogDelivered })
}
```

Required-ness is assumed, never computed. But Sentry can legitimately be off on cloud — `src/main.ts:97`:

```ts
const sentryEnabled = !import.meta.env.DEV && !!sentryDsn
```

With an empty cloud DSN, `sentryEnabled` is `false`, `sentryInit({ enabled: false })`, and `isSentryEnabled()` returns `false` forever. `dispatch()` never sets `sentryDelivered`, so the condition stays true on every flush and the report is re-enqueued indefinitely.

`enqueuePendingReport` caps the queue at `MAX_PENDING_REPORTS = 25` but never evicts, so the slots are held, not rotated:

```ts
function enqueuePendingReport(report: PendingReport): void {
if (pendingReports.length < MAX_PENDING_REPORTS) {
pendingReports.push(report)
}
}
```

### Consequence

Steady-state on such a deployment: 25 undeliverable reports pinned in the buffer, every subsequent early-boot report dropped at the `length < 25` check. Datadog still receives reports dispatched while it is live, so this is not total blindness — it is the loss of the pre-sink window specifically, which is where splash-screen and boot failures land.

### Suggested fix

Compute required-ness when the report is first enqueued rather than assuming both sinks are required — a sink that is not enabled is not pending:

```ts
const sentryRequired = isSentryEnabled()
```

Alternatively, confirm Sentry is always enabled on cloud and drop the conditional. Either way the invariant worth asserting is that a report is retained only while some sink could still accept it.

### Provenance

Raised during review of #15671 (https://github.com/Comfy-Org/ComfyUI_frontend/pull/15671#discussion_r3961880110) and not addressed before that PR merged as `3d7c5d4df3`. DrJKL's related per-sink-retry blocker on the same file *was* addressed pre-merge; this one was not.

Not yet reproduced against a live cloud deployment with an empty DSN — the code path is established by reading `main.ts` and `reportError.ts` on `main`. Worth confirming whether any cloud environment actually ships without a Sentry DSN before prioritising.

Contributor guide

Open the contributing guide

Research direction

Start with src/platform/telemetry/reportError.ts around line 142 and trace the pending queue through enqueuePendingReport, then compare sink initialization in src/main.ts around line 97. Confirm the empty-cloud-DSN behavior and verify that reports are retained only while an enabled sink could accept them; the pending buffer should continue admitting early-boot reports instead of pinning 25 undeliverable entries.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
observability-sre
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.