perf(tracking): make MixpanelProvider dispatch non-blocking — bounded queue + daemon worker + real flush() (BE-5868)

[mattmillerai]

ELI-5

Every time you ran a comfy command with analytics on, the CLI stopped and waited for an analytics event to be delivered over the network before it started doing the thing you asked for. If the analytics endpoint was slow or blackholed, you waited up to ~10 seconds staring at nothing. Now the event goes into an in-memory outbox and a background thread mails it while your command runs. If the outbox somehow fills up, extra events are quietly dropped — analytics is best-effort, your command is not.

What changed

MixpanelProvider is rewritten from send-inline to queue-and-drain, all inside comfy_cli/tracking.py:

• track() does a put_nowait onto a bounded queue.Queue(maxsize=256) and returns immediately. On queue.Full it logs at debug and drops (never WARNING — a telemetry failure must not surface on the user's stderr, same policy as the urllib3/posthog logger silencing at the top of the module).
• A single daemon=True worker thread (mixpanel-telemetry) drains the queue and does the actual client.track(...), with try/except Exception → logging.debug and self._queue.task_done() in a finally: so a raising send can never wedge flush().
• flush() is no longer a no-op: it is a real self._queue.join(). It is deliberately unbounded here — boundedness comes from the caller, _flush_all_providers, which already runs each provider's flush in a daemon thread joined against the shared 5s _FLUSH_DEADLINE_SECONDS deadline and abandons it on timeout. That is exactly how PostHog's internally-unbounded client.flush() is already handled.
• No atexit hook of our own; the worker just dies with the process (_flush_all_providers stays the single shutdown drain path).
• The lazy-SDK-import structure is untouched: queue is stdlib and joins threading at module scope; the mixpanel import stays inside __init__. comfy --help and shell completion remain thread-free and SDK-free, because providers are still built lazily on the first dispatched event.

The request_timeout=10 / retry_limit=1 consumer bound stays (the mixpanel<5 pin is untouched — 4.x has no async consumer, hence the hand-rolled worker). Its comment was rewritten: the bound no longer guards the hot path, it now caps how long one worker send can occupy the queue and how much of the exit drain's 5s budget a single in-flight event can eat.

Accepted semantic shift

mixpanel-python stamps an event's time when client.track() runs, which moves from call time to dequeue time — sub-second skew in practice, no dashboard impact. Ordering is preserved: one FIFO, one worker, so per-process event order is unchanged.

Tests

New TestMixpanelNonBlockingDispatch in tests/comfy_cli/test_tracking_providers.py (7 cases): the hot path returns in well under 1s while a send is provably still in flight; flush() drains 20 events in submission order; overflow (with _QUEUE_MAX monkeypatched to 2) drops without blocking or raising and leaves a DEBUG-level breadcrumb; a raising send still lets flush() return; _flush_all_providers() returns inside the 5s deadline against a send that hangs forever; the worker is a daemon and the provider registers no atexit hook; a token-less provider's track()/flush() stay inert.

Existing assertions on the mocked Mixpanel client were swept — they now drain first, via a shared _mixpanel_track_kwargs() helper or an explicit mp_provider.flush(). The negative assertion (assert_not_called on opt-out) also flushes, so it proves nothing was enqueued rather than racing the worker.

Verification (all green): ruff check + ruff format --check at the CI-pinned 0.15.15, the four tracking suites (174 passed), and the full pytest run (3748 passed, 37 skipped). The new async tests were run 3× to check for flakiness — stable.

Judgment calls

• Drop-log formatting. The spec sketched logging.debug("mixpanel queue full; dropping event %s", event_name), but comfy_cli.logging.debug(message) is a single-argument wrapper — the %-arg form would raise TypeError. Used an f-string, matching the rest of the module.
• dict(properties) at enqueue time. _dispatch already hands each provider a fresh dict, so this copy is redundant today; kept it anyway so a caller mutating its dict between enqueue and send can't corrupt an in-flight event.
• Added a coverage case beyond the spec's list (test_worker_is_a_daemon_and_the_provider_registers_no_atexit_hook) to fence the two settled design constraints that are otherwise only enforced by a comment — mirroring the existing test_posthog_unregisters_its_own_atexit_join guard.
• Exit-path warning under a blackholed endpoint. A hung Mixpanel send can now make _flush_all_providers log telemetry flush timed out for MixpanelProvider. That is not new user-visible noise: it is a logging.warning, and the CLI's default LOG_LEVEL is ERROR, so it is filtered unless the user is explicitly debugging — and PostHog already reaches the same line under the same conditions.
• No capability is denied by this diff (no deny/dead-end path, no "unsupported" string, no test flipped to assert absence), so the negative-claim falsification gate does not apply. The one thing the diff can drop — events past the 256-item queue cap — is a settled design constraint in the ticket and is consistent with the module's existing best-effort-telemetry contract.

Out of scope

No change to PostHogProvider, _dispatch, _flush_all_providers, or any of the ~60 call sites. The mixpanel<5 pin is untouched.

---

Update — review round 1 (7b31a37)

Six of the ten Cursor panel findings landed as code. Three claims in the section above are now stale and are superseded here.

Superseded:

• "flush() is a real self._queue.join(), deliberately unbounded here" — join()/task_done() are gone. flush() waits on a _pending counter guarded by a threading.Condition, bounded by _FLUSH_DEADLINE_SECONDS and re-checking self._worker.is_alive() on each short wait. join() is unconditional, so a worker that never came back blocked every later flush() forever — and flush() is public, so a direct caller had no deadline of its own to fall back on.
• "dict(properties) … is redundant today" — it was not redundant, it was insufficient. _dispatch's fresh dict is shallow, so nested values (Typer multi-value options, feedback score dicts) stayed aliased to the caller's objects. Since @track_command fires before the wrapped body runs and serialization moved to the worker, the body could mutate a value out from under an in-flight send. Now copy.deepcopy, with a guarded fallback to the shallow copy.
• "a logging.warning … is filtered unless the user is explicitly debugging" — accurate as far as it goes (cmdline.py calls setup_logging() at import, default LOG_LEVEL=ERROR), but the line still surfaces at LOG_LEVEL=WARNING, after the terminal envelope, for what is a slow endpoint rather than a defect. Dropped to logging.debug.

Also fixed:

• os._exit bypassed the drain — the one real regression in the original diff. @track_command fires its event before the command body runs, so inline sends were already delivered by the time comfy launch --background reached os._exit; with queue-and-drain the event was still queued and dropped on every backgrounded launch. Adds tracking.flush_for_hard_exit(), and routes launch.py's seven exits through _hard_exit(), which drains (bounded, best-effort) and then exits with the same code. __main__.py's broken-pipe exit still skips the drain, as its comment already declares.
• Worker survivability. The whole _run loop body is guarded, on BaseException — get(), the unpack, and the send. Nothing detects or restarts this thread, so any escape wedged telemetry for the rest of the process. The failure log itself goes through _log_telemetry_debug(), which swallows, so a raise from inside the except handler can't escape either. task_done() became _mark_done(), keyed on having actually dequeued something.
• Thread.start() RuntimeError ("can't start new thread") degrades to an inert provider instead of escaping into _get_providers' logging.warning.

Declined, with reasoning on the threads: the 🔴 Critical logging.debug NameError is a false positive — tracking.py:17 rebinds logging to comfy_cli.logging, which is why the stdlib is aliased to logginglib. Two Lows (per-provider thread leak; a log racing logging.shutdown) and the Nit (payload-size clamp) have no reachable impact today and are answered in-thread rather than deferred to tickets.

New tests: TestMixpanelWorkerSurvivability (4), TestMixpanelPropertySnapshot (2), TestHardExitDrain (3). The snapshot test is a verified regression test — reverting the deepcopy to dict() fails it with the post-mutation payload.

Verification: ruff check clean, full pytest green — 3758 passed, 37 skipped.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 25dc6d70-c44d-4b79-9592-16255a12b49c

📥 Commits

Reviewing files that changed from the base of the PR and between 5704b55 and 7b31a37.

📒 Files selected for processing (3)

• comfy_cli/command/launch.py
• comfy_cli/tracking.py
• tests/comfy_cli/test_tracking_providers.py

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity	Count
🔴 Critical	1
🟠 High	2
🟡 Medium	3
🟢 Low	3
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[bigcat88]

Approving. I didn't take the latency claim on trust — I stood up a local CONNECT proxy that terminates TLS for api.mixpanel.com and t.comfy.org with a scratch CA, so every event this CLI emits was captured on the wire and the endpoint's latency was mine to set.

1. The central claim, measured

comfy stop against a blackholed endpoint (accepts TCP, never responds):

	time to first output byte	total
main	20.3s	25.3s
this branch	0.26s	5.3s

Two notes. The 20s is two attempts, not one — retry_limit=1 is one retry, so a blackholed send is ~2 × 10s. The comment still says "a single ~10s attempt"; that wording predates this PR, but since you rewrote the block around it, worth correcting. And the branch's 5.3s is the atexit deadline doing exactly what it is supposed to.

2. Nothing is lost on the normal path

With a reachable endpoint, comfy stop delivers the stop Mixpanel event and the cli:stop PostHog batch on both, and wall time is indistinguishable (0.26s/0.80s branch vs 0.27s/0.81s main).

3. _hard_exit is load-bearing — and fixes more than the description claims

comfy launch --background, endpoint lagging 2s inside the tunnel:

variant	Mixpanel	PostHog
main	2 delivered	0 — dropped
this branch + main's launch.py (fix removed)	0 — dropped	0 — dropped
this branch as-is	2 delivered	2 delivered

The row worth calling out is PostHog on main: os._exit was already dropping every PostHog event on this path before this PR. Your drain rescues them, which is a strict improvement the description doesn't claim.

The description is a little strong in the other direction, though: with a fast endpoint the fix-removed variant still delivered both Mixpanel events — the worker won the race. So it's "drops whenever the send hasn't landed yet", not "drops on every backgrounded launch". Doesn't change the verdict; the drain is correct either way, and on the success path (where ComfyUI takes seconds to boot) Mixpanel would usually have gone out already, which makes PostHog the real beneficiary.

Bounded, too: launch --background under a full blackhole is 10.6s here (5s × parent + child) against 40.7s on main.

4. Direct probes of the machinery (20 assertions, all pass)

50 track() calls return in 0.1ms with a send provably in flight; FIFO order holds across 1600 events from 8 concurrent producers with _pending settling to 0; overflow at maxsize=2 drops 498 of 500 without blocking or raising; flush() bounds at exactly 5.00s against a wedged worker, and worst-case track() latency while another thread sits inside that blocked flush is 0.0 ms (the Condition.wait releases as intended); a raising send leaves the worker alive and still serving later events; the deepcopy snapshot holds for both a nested dict and a nested list; an un-deepcopyable value falls back; a token-less provider is inert; cap is 256, daemon, named mixpanel-telemetry.

Lazy-import guarantee intact: comfy --help produces zero telemetry requests, and importing comfy_cli.tracking leaves mixpanel unimported, PROVIDERS None, and MainThread the only thread.

5. Tests are non-vacuous

With tracking.py and launch.py reverted to main and your test file kept, 10 of 16 fail. The 6 that pass are invariant guards (submission order, bounded exit, inert-when-disabled, flush-gives-up) whose property holds trivially under inline sends — right to keep as guards.

6. Suite

Full pytest . on your branch merged with current main: 3763 passed, 31 skipped (this box has a GPU, so 6 tests run that skip in your env — the totals reconcile exactly). ruff check and ruff format --diff clean under the CI-pinned 0.15.15.

Findings you resolved that I re-checked independently

• The 🔴 NameError is indeed a false positive: tracking.py:17 binds logging to comfy_cli.logging, whose debug(message) is single-argument — so the f-string isn't a style choice, the %-arg form would genuinely TypeError.
• Your correction on the LOG_LEVEL thread is the right one: cmdline.py:60 calls setup_logging() at import, so a warning there really would have been filtered on the default path. Good catch on yourself.
• All six os._exit call sites in launch.py are converted; the only remaining one in that module is inside _hard_exit itself. (The description says seven.)
• MixpanelProvider is constructed exactly once in production, memoized behind _get_providers, and provider.flush() has exactly one production caller — so both "thread leak" and "public method has no deadline" are correctly judged as theoretical.

Nits, none blocking

1. retry_limit=1 comment: measured 2 attempts / ~20s, not one ~10s attempt.
2. Two test docstrings still describe the abandoned design — test_flush_returns_even_when_the_send_raises credits finally: task_done() for saving queue.join(), and test_flush_gives_up_promptly_when_the_worker_is_gone opens on "queue.join() would block forever here". Both are gone from the implementation.
3. launch --background costs +0.4s on the happy path (two processes each doing a real PostHog upload now). Fair trade for events that were being dropped, and invisible next to server startup — just noting it's a measured delta, not zero.
4. Ctrl-C during a foreground comfy launch can now wait up to 5s for the drain. Consistent with every other command's atexit path, so I'd leave it.

Merge sequencing

#647 also touches comfy_cli/tracking.py; expect it to want a rebase once this lands.