fix(telemetry): bound network I/O so telemetry can't outlive the run envelope (BE-3403)#564
fix(telemetry): bound network I/O so telemetry can't outlive the run envelope (BE-3403)#564mattmillerai wants to merge 2 commits into
Conversation
…envelope (BE-3403) Mixpanel's default Consumer used request_timeout=None (unbounded, synchronous requests.post on the main thread) and _flush_all_providers had no deadline, so with consent on and a blackholed telemetry endpoint (accepts TCP, never responds) 'comfy run --wait' blocked indefinitely: execution_start hung before any output, and execution_success + the atexit flush lingered after the terminal envelope was already on stdout (BE-3354/BE-3329). - MixpanelProvider builds Mixpanel(token, consumer=Consumer(request_timeout=10)). - PostHogProvider passes max_retries=1, timeout=10 (was 3/15), dropping the worst-case consumer drain from ~50s to ~21s. - _flush_all_providers runs each provider's flush in a daemon thread with a 5s join deadline; a hung provider is abandoned (in-flight events dropped) rather than wedging every consumer of the CLI's stdout.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
the
size:M
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 3 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…shared flush deadline (BE-3403) Address cursor-review panel findings that partially defeated this PR's goal of keeping telemetry I/O from outliving the run envelope: - MixpanelConsumer defaults to retry_limit=4 with backoff, so request_timeout=10 bounded only a single attempt (~40s+ worst case). track() sends inline on the main thread and flush() is a no-op, so this send is never covered by the atexit daemon deadline. Pass retry_limit=1 so a blackholed send is a single ~10s try. - Posthog's constructor registers its own atexit.register(self.join), which flushes synchronously on the main thread at shutdown, unbounded by _flush_all_providers' daemon deadline (~21s against a blackholed endpoint). Unregister it so our bounded flush is the only shutdown drain path. - _flush_all_providers now starts all provider threads, then joins them against a single shared 5s deadline (was 5s per provider = 5s x N), and wraps start()/join() defensively so the atexit hook can't raise past the envelope. Tests: assert Mixpanel retry total == 1, and that PostHog unregisters its join. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI note: the two red checks — |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
1 participant
ELI-5
When you run
comfy run, the CLI quietly phones home telemetry. If the telemetry server accepts the connection but then goes silent (a firewall/proxy "blackhole"), the old code waited forever for a reply — socomfy run --waitcould hang with zero output for minutes, and even after the real result was printed the process kept lingering. This PR puts a stopwatch on every telemetry network call so the CLI can never wait more than a few seconds on a dead endpoint.What & why
BE-3354 reproduced (live, macOS) an indefinite hang: with telemetry consent ON and a telemetry endpoint that accepts TCP but never responds,
comfy --json-stream run --waitblocked with 0 bytes out for 103s, stuck inrequests.post(..., timeout=None). Two root causes, both incomfy_cli/tracking.py:MixpanelProvider.__init__builtMixpanel(token), whose defaultConsumerusesrequest_timeout=None→ an unbounded, synchronousrequests.poston the main thread.track_event("execution_start")fires this before the run emits anything, andtrack_event("execution_success")fires it after the terminal envelope is already on stdout (which comfy-local-mcp, reading stdout to EOF, experiences as a full tool timeout — BE-3329)._flush_all_providers(atexit) had no deadline; PostHog'sflush()→queue.join()with the defaultmax_retries=3 × timeout=15could drain for ~50s, post-envelope.Changes (all in
comfy_cli/tracking.py)Mixpanel(token, consumer=MixpanelConsumer(request_timeout=10))— every send bounded to 10s.Posthog(..., max_retries=1, timeout=10)(was defaults 3/15) — worst-case consumer drain ~50s → ~21s._flush_all_providersnow runs each provider'sflush()in a daemon thread andjoin(timeout=5.0); a still-alive thread past the deadline logs a warning and is abandoned (in-flight events dropped). Telemetry is best-effort by contract — dropping events beats wedging every consumer of the CLI's stdout. The per-provider try/except is preserved in_flush_one.Tests
test_flush_returns_before_deadline_when_a_provider_hangs: a stub provider whoseflush()blocks 60s is registered inPROVIDERS;_flush_all_providers()returns in well under the hang (assert < 8s), and a healthy provider after it is still drained.test_mixpanel_client_has_bounded_request_timeout: assertsMixpanelProvider().client._consumer._request_timeout == 10so the default-Noneregression can't silently return.pytest tests/ -k track→ 125 passed.ruff check+ruff formatclean.Empirical verification (real blackhole socket, not just unit tests)
Measured against a local socket that accepts TCP and never replies:
track()on the blackhole: 10.0s then raises (the Consumer's urllib3 retry adapter does not re-retry the read timeout — one attempt, bounded exactly atrequest_timeout).flush()drain withmax_retries=1, timeout=10: ~21.7s — but our daemon-threadjoin(timeout=5.0)abandons it at 5s, so_flush_all_providersreturns at ~5s.Before the fix these were unbounded (∞). The two documented hang points are now bounded.
Judgment calls / honest note on the envelope→exit gap
Per the ticket's non-goal, PostHog's own internal
atexit(Client.join) is left in place (withmax_retries=1, timeout=10it is self-bounded). Capping it further is not trivially achievable without removing it, which the ticket forbids — so I left it. Consequently the measured worst-case envelope→exit gap in the blackhole case is ~21s, dominated by (a) the specified 10s Mixpanelexecution_successsend and (b) PostHog's retained internal atexit drain. This is within the ticket's hard < ~30s total-penalty bound and is a night-and-day improvement over the pre-fix unbounded hang (and comfortably under comfy-local-mcp's 120s timeout), but it is slightly above the ticket's ~16s envelope→exit estimate — flagging that transparently rather than contorting the code (or removing PostHog's atexit) to hit the estimate. Typical (reachable-endpoint) penalty stays < 2s.Negative-claim falsification
Not applicable: this change denies no capability. It bounds best-effort I/O and logs a warning only on a blackhole; telemetry still sends normally when the network is healthy. No "not supported"/dead-end/deny path is added.
BE-3403