fix(telemetry): bound network I/O so telemetry can't outlive the run envelope (BE-3403)#564
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTelemetry provider construction now uses bounded network settings, and shutdown flushing runs providers concurrently with a shared deadline. Regression tests cover provider configuration, PostHog handler removal, and completion while another provider remains blocked. ChangesTelemetry shutdown controls
Sequence Diagram(s)sequenceDiagram
participant Atexit
participant FlushAllProviders
participant MixpanelProvider
participant PostHogProvider
Atexit->>FlushAllProviders: invoke shutdown flush
FlushAllProviders->>MixpanelProvider: start daemon flush
FlushAllProviders->>PostHogProvider: start daemon flush
MixpanelProvider-->>FlushAllProviders: complete or remain blocked
PostHogProvider-->>FlushAllProviders: complete flush
FlushAllProviders->>FlushAllProviders: join until shared deadline
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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:
|
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Re-verified locally with main merged in: tracking suites (126) and full unit suite (2491) green. The bounded Mixpanel consumer (10s, retry_limit=1), tightened PostHog budget, shared 5s daemon-thread flush deadline, and the atexit.unregister of PostHog's own join are all correct — the unregister especially, since LIFO atexit ordering would otherwise run PostHog's unbounded join before the bounded flush. One nit before merging: the PR body's 'Judgment calls' section still says PostHog's internal atexit was left in place — the final code removes it, so please refresh the description to match.
2 participants
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