Comfy-Org / Comfy-Org/ComfyUI_frontend
reportError: undeliverable cloud reports permanently occupy the pending buffer
- 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
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