Make MixpanelClient durable with an on-disk event spool

[johnml1135]

Summary

• MixpanelClient fired each event over HTTP immediately and silently dropped it when offline; the Segment path already had durability via Segment.Analytics.CSharp, so this closes that gap for Mixpanel.
• Adds a DiskQueue-backed event spool (EventSpool) and a background flush loop (Polly retry + circuit breaker) that drains it — events survive offline stretches and process restarts, delivered at-least-once via a stamped $insert_id.
• Spool is bounded (cap + drop-oldest) so an extended offline period can't grow it without limit.
• All spooled string values are path-scrubbed (PathScrubber) before being persisted, normalizing user home paths so stack traces and usage events don't leak OS account names.
• AllowTracking → false purges the spool immediately and pauses the flush loop; re-enabling resumes it (IClient.ResumeSending() / PurgeQueuedEvents()).
• Flush()/ShutDown() now trigger a bounded drain instead of being no-ops, without blocking an offline shutdown.
• SampleApp gains an optional c=false flag to skip its built-in AllowTracking on/off demo (see "Manual smoke test" below for why).

Test plan

• dotnet build DesktopAnalytics.sln — succeeds, 0 warnings/errors
• Full test suite via vstest — 68/68 passing, including reliability property tests (no-loss retry, crash-window dedup via $insert_id, spool bounding, consent purge, cross-process lock) and poison-vs-retryable HTTP status classification
• Manual offline smoke test against the live Mixpanel endpoint — see below

Manual smoke test (2026-07-09)

Ran two PowerShell harnesses (in the FieldWorks repo, scripts/Test-AnalyticsOfflineDrain.ps1 and scripts/Test-AnalyticsOfflineSandbox.ps1 — see that repo's Docs/architecture/local-library-debugging.md) against this branch's build, hitting the real Mixpanel /track endpoint with FieldWorks' own DEBUG-build project token:

• Per-process firewall block (Test-AnalyticsOfflineDrain.ps1): blocks SampleApp.exe's outbound traffic with a scoped Windows Firewall rule, runs it once blocked, removes the rule, runs it again. Result: Phase 1 (blocked) — Submitted=3 Succeeded=0 Failed=0, confirming events accumulate in the spool rather than being dropped. Phase 2 (unblocked) — Submitted=3 Succeeded=6 Failed=0: this run's 3 new events plus all 3 leftover from Phase 1 were delivered. PASS.
• Windows Sandbox, no guest NIC (Test-AnalyticsOfflineSandbox.ps1): runs SampleApp inside a disposable Windows Sandbox guest with <Networking>Disable</Networking> (no virtual NIC at all, host networking untouched) — a "born offline" check complementary to the firewall test. Result: Submitted=4 Succeeded=0 Failed=0 — events submitted, none falsely reported delivered. PASS.

Both confirm MixpanelClient no longer depends on NetworkInterface.GetIsNetworkAvailable() (the old gate a firewall block couldn't fool) — it now classifies offline from real HTTP send failures, so a per-process block is an accurate simulation of "network down" for delivery purposes.

Found and fixed along the way (this commit): SampleApp's built-in AllowTracking on/off demo calls PurgeQueuedEvents(), which is working as designed (consent revocation empties the spool immediately) but made SampleApp unusable as a driver for a cross-process durability test — the purge always fires before the next scheduled flush tick, deleting Phase 1's leftover events before they could ever be observed draining. Not a MixpanelClient bug; a test-driver limitation. Added SampleApp's new c=false flag to skip that demo block, used by both phases above.

Network-failure hardening (follow-up commit)

A review of the bad-network-condition handling found four issues, all fixed in a follow-up commit:

1. Captive portals could silently lose events. MixpanelEventSender's HttpClient followed redirects by default, so a captive portal (hotel/coffee-shop Wi-Fi login page) intercepting the POST would get followed to the portal's login page, return a 200, and be misclassified as Delivered even though Mixpanel never received the event. Fixed by disabling AllowAutoRedirect and classifying any 3xx response as RetryableFailure (Mixpanel's /track never legitimately redirects).
2. ShutDown()/Flush() could hang up to ~45s against a "black hole" server (one that accepts the TCP connection but never responds — captive portals and some firewalls do this), because the bounded-drain deadline was only checked between attempts, not during one. Fixed by threading a CancellationToken through IEventSender.Send down to HttpClient.PostAsync, tying a CancellationTokenSource to the remaining deadline on every bounded-drain attempt, and bypassing the Polly retry pipeline during bounded drains (retrying during shutdown has little value and only multiplies the hang). Undelivered events remain correctly rolled back in the spool.
3. The circuit breaker could never open. MinimumThroughput was 4, but a fully failing drain tick only ever produces 3 outcomes (1 attempt + 2 retries), and ticks are 30s apart — well outside the 10s sampling window — so 4 outcomes could never land in the same window during a real outage. Fixed by lowering MinimumThroughput to 3 to match.
4. Mixpanel's /track returns HTTP 200 with body "0" for a rejected payload, which was previously counted as Delivered. Fixed to classify a trimmed "0" body on a 2xx as PoisonDrop (a rejected payload will never succeed on retry); a body-read failure on an otherwise-2xx response still counts as Delivered (at-least-once + $insert_id dedup makes that safe).

Covered by new NUnit tests (WireMock-based redirect/body/cancellation tests in MixpanelEventSenderTests, and bounded-drain-against-a-hanging-sender / circuit-breaker-threshold tests in MixpanelClientTests). Full suite: 76/76 passing.

Review feedback (2026-07-09, commit 256eebc)

All 14 of @hahn-kev's inline comments are addressed — each thread has a specific reply (from Claude, on John's behalf), and a top-level comment summarizes the dispositions. Highlights that changed behavior:

• Track() on a never-initialized client now throws InvalidOperationException (restoring the original contract) instead of silently dropping events. An initialized client whose spool couldn't be created still degrades to a no-op by design — analytics must never crash the host.
• Purge() (consent revocation) now actually removes event data from disk — dispose + delete the spool directory + reopen. Empirically, DiskQueue's HardDelete(reset: true) leaves the live instance's count estimate stale, and the previous dequeue-and-flush loop left the purged events' bytes in the data files; neither is right for a consent purge. A test scans the raw on-disk files to pin this.
• Corruption behavior pinned by tests: garbage appended to a DiskQueue data file is recovered (committed events survive a reopen); garbage in the transaction log makes DiskQueue fail fast on open rather than serve corrupt data (MixpanelClient.Initialize catches that and degrades to non-durable).
• Cleanups per review: flush interval stored as a clamped TimeSpan; AnalyticsEvent.Create takes plain insertId/time values instead of factory funcs; BoundedDrain shares one CancellationTokenSource (created via the injected TimeProvider) across attempts; _timerDraining comment corrected; concurrency test uses dedicated threads; new ProcessBatch max-items test; the scrub test now proves scrub-before-disk by scanning raw spool bytes.
• Declined with reasoning (see threads): /import (requires server-side credentials a desktop binary can't ship), send-first-then-spool (the write-ahead is the durability guarantee, and "online?" is only knowable from a send outcome), and a per-event retry counter (would poison-drop good head-of-queue events during long outages).

Async pipeline (2026-07-09, commit bd49103)

Per @hahn-kev's request (FW Lite prefers async), the internal delivery pipeline is now async end-to-end: IEventSender.SendAsync, EventSpool.ProcessBatchAsync (monitor lock → SemaphoreSlim, held across awaits with the same serialization guarantee), MixpanelClient.DrainOnceAsync/BoundedDrainAsync via Polly ExecuteAsync, and MixpanelEventSender awaiting HttpClient directly (no more sync-over-async). ConfigureAwait(false) throughout. Sync Flush/ShutDown block only at the boundary on the ~5s-bounded drain, which is UI-thread-safe given ConfigureAwait(false).

Public async surface (543db87, additive — no existing signature changed): Analytics.FlushClientAsync() and analytics.ShutDownAsync(), an async alternative to Dispose (shutdown is idempotent, so a using block around the object stays correct). IClient gains FlushAsync/ShutDownAsync; MixpanelClient implements them natively, SegmentClient completes synchronously (Segment.Analytics.CSharp's Flush() exposes nothing awaitable).

Per-event size cap (cad6b07, resolving the retry-forever thread): events over Mixpanel's documented 1MB-uncompressed-JSON limit are refused at enqueue time and counted in Statistics.Failed — a rejected-but-deliverable payload was already caught as poison via the 200-with-body-"0" path, but a payload large enough to time out the request classifies as retryable (indistinguishable from being offline) and would have wedged the head of the queue forever. Now it can't enter the queue at all.

Constrained bandwidth (02ea2bc): the "internet café" scenario

Many users of SIL software are on slow, shared, or per-MB metered connections. The scenario to design for: a user's machine accumulates an offline backlog, then reconnects on a slow shared link (e.g. a café in Kenya) — the library must not hog that link or silently spend meaningful metered data.

What the design already guaranteed (before this commit): sends are strictly sequential — one connection, one request in flight, never parallel; a drain tick is at most 20 events every 30s; typical events are ~2–3KB on the wire, so a normal tick is a ~50–60KB burst (<2KB/s averaged, ~5% duty cycle on a 256kbps line); the circuit breaker opens after one fully-failing tick, so a flaky link gets less traffic, not retry-hammering; there is no reconnect "dump" — the first tick after reconnect sends one batch and waits.

What this commit adds (the two dimensions that were unbounded):

• Per-tick byte budget (256KB, soft): a tick stops once its sent bytes reach the budget, so a run of large exception events is spread across ticks instead of saturating a slow link in one burst. Soft = checked after each event; a single event over the whole budget still goes, so the budget can never starve the queue.
• Total backlog cap (20MB, drop-oldest) alongside the 5,000-item cap: bounds both disk footprint and the total upload liability of a long-offline machine (the item cap alone allowed a theoretical 5,000 × 1MB ≈ 5GB). Byte accounting tracks live entries and survives restarts (re-measured from the spool at open).

Residual, accepted risks (deliberate, with reasoning):

• Worst-case tick can exceed 256KB by up to one event (≤1MB): the alternative — refusing to send an over-budget event — would wedge the queue; forward progress wins.
• Metered data cost is not a consent question the library answers: a full 20MB backlog drain still happens silently. If a host app (FieldWorks/FW Lite) wants "only drain on Wi-Fi / on explicit flush", that's a product decision to build on the existing flushAt/flushInterval knobs and FlushClientAsync — not a default this library should assume.
• No adaptive "slow connection" detection: bandwidth estimation from inside the client is unreliable (this PR's testing showed even "am I online" can only be learned from send outcomes). Fixed conservative bounds are preferred over adaptive guesses.
• Events dropped by the caps are not individually reported (same as the pre-existing item cap): drop-oldest is silent by design; Statistics still reflects overall submitted/succeeded/failed counts.

Full suite after the changes: 90/90 passing, 0 warnings. Production init/timer/purge/resume path re-verified end-to-end by driving SampleApp against the live endpoint (identical statistics to the sync build).

🤖 Generated with Claude Code

---

This change is

[hahn-kev]

I left some suggestions, I'm very interested in this change because I want to use it in FW Lite, however FW Lite uses async Tasks where possible, so I'd love to see this be async where possible. But I'd be happy to do that in a follow up PR if that's out of scope here.

[hahn-kev]

I suggest based on the docs we should use import instead of track, see here: https://docs.mixpanel.com/reference/track-event#when-to-use-/track-vs-/import
it's designed for importing data, and supports importing old events, track will only accept events from up to 5 days ago. Track is best for live tracking, but we don't care about it in this case. Also using import would allow us to send up to 2000 events in a single http request.

[johnml1135]

The auth model rules /import out for this library, unfortunately: per that same doc page, /import requires a Project Secret or Service Account credential ("intended for server-side integration"), while /track authenticates with just the project token ("intended for untrusted clients"). DesktopAnalytics ships inside desktop apps, so the only credential we can embed is the token — shipping a project secret in a client binary would hand project-level API access to anyone with the executable.

Two real points in here worth acting on, though:

1. The 5-day limit is a genuine gap: a machine offline longer than that will replay events /track no longer accepts. That's now a documented limitation of this design rather than something we can fix client-side.
2. Batching doesn't require /import — the same page says /track also accepts up to 2,000 events per request. We deliberately post one event at a time for now because the whole spool design keys off per-event outcome classification (Delivered / retryable / poison → commit or roll back exactly one spool entry), and a batched response is one verdict for the whole batch. At desktop-analytics volume (a handful of events per session, drained in 20-event batches every 30s) the per-request overhead is negligible, but batch-over-/track is a reasonable future optimization if volume ever grows.

— Claude (Fable 5), replying at John's request

[hahn-kev]

since this is going over the network can we have an async Task version? I realize this would bubble up to other places, but that's kinda the point.

[johnml1135]

Taking you up on the follow-up-PR offer from your review summary rather than folding it in here. The refactor is mechanical but invasive — EventSpool's lock can't span an await so it becomes a SemaphoreSlim, ProcessBatch takes an async callback, and the whole flush pipeline changes shape — with no observable behavior change, since IEventSender is internal, the public IClient surface stays sync either way, and the flush loop already runs off the UI thread (timer threads, never blocking a caller). Keeping this PR reviewable and doing async as its own focused change seems like the better split; happy to have FW Lite's needs drive that PR.

— Claude (Fable 5), replying at John's request

[johnml1135]

Update: John asked to include it in this PR after all — done in bd49103, and it was indeed not too hard. The whole delivery pipeline is now async end-to-end: IEventSender.SendAsync, EventSpool.ProcessBatchAsync (its monitor lock became a SemaphoreSlim so it can be held across awaits — same serialization guarantee), MixpanelClient.DrainOnceAsync/BoundedDrainAsync with Polly's ExecuteAsync, and MixpanelEventSender awaiting HttpClient directly instead of the previous sync-over-async block. ConfigureAwait(false) throughout.

The public IClient surface is unchanged (sync Track/Flush/ShutDown — Flush/ShutDown block on the ~5s-bounded drain at the boundary only, which is UI-thread-safe given ConfigureAwait(false)). If FW Lite wants an async public surface too (FlushAsync/ShutDownAsync on IClient or the Analytics facade), that's now a thin addition — say the word and it can go in here or in your follow-up.

81/81 tests passing, 0 warnings; production path re-verified end-to-end via SampleApp.

— Claude (Fable 5), replying at John's request

[johnml1135]

The public async surface is in as well (543db87): Analytics.FlushClientAsync() (static, mirrors FlushClient()) and analytics.ShutDownAsync() — an async alternative to Dispose for hosts that shut down asynchronously; a using block around the object stays correct because shutdown is idempotent and the subsequent Dispose is a no-op. On IClient, FlushAsync/ShutDownAsync are the primary implementations in MixpanelClient (the sync methods are now thin blocking wrappers); SegmentClient's complete synchronously since Segment.Analytics.CSharp's Flush() only signals its own background delivery and exposes nothing awaitable. Everything is additive — no existing signature changed.

84/84 tests passing, including bounded ShutDownAsync/FlushAsync against a black-hole sender awaited directly (no Task.Run), and a ShutDownAsync-then-Dispose idempotence test.

— Claude (Fable 5), replying at John's request

[hahn-kev]

Is there a reason the caller doesn't just provide a DateTimeOffset here? what's the benefit of using a function if we're just going to call it once and throw it away. If we do still want to do it this way then shouldn't we just use a TimeProvider?

[johnml1135]

No benefit — the factory was invoked exactly once at the single call site, so it was indirection for nothing. Fixed in 256eebc: Create now takes plain string insertId = null, DateTimeOffset? time = null values, and Track passes _timeProvider.GetUtcNow() directly (the TimeProvider already lives in MixpanelClient; AnalyticsEvent stays a dumb data envelope that doesn't need to know about it).

— Claude (Fable 5), replying at John's request

[hahn-kev]

we can move the cts outside of the loop and just create it once with new CancellationTokenSource(s_boundedDrainDuration), the same cts.Token can be used many times, you can even use it to check if the deadline is up without needing to do any extra date time math inside the loop.

[johnml1135]

Done in 256eebc, with one adjustment to the suggestion: new CancellationTokenSource(delay) only knows the system clock, and BoundedDrain deliberately measures its deadline through the injected TimeProvider so tests keep deterministic control of it. Microsoft.Bcl.TimeProvider (already referenced) provides timeProvider.CreateCancellationTokenSource(delay), so the loop now creates that single CTS once outside, shares cts.Token across all attempts, and checks cts.IsCancellationRequested instead of doing date math per iteration.

— Claude (Fable 5), replying at John's request

[hahn-kev]

this is a behavior change. If Initialize was not called, previously _client would be null and calling Track would throw. Now if Initialize is not called this will silently fail, we should throw an explicit error.

[johnml1135]

Confirmed it was an unintended behavior change — fixed in 256eebc. Track on a never-initialized client now throws InvalidOperationException (checked before the catch-all, which would otherwise swallow it), with a test pinning it.

One deliberate distinction kept: _spool == null on an initialized client means Initialize ran but the spool couldn't be created (e.g. another instance of the same app holds the cross-process lock on the spool directory). That case still degrades to a silent no-op — analytics failing to be durable must never crash the host app. So: never initialized → throw (programming error); initialized but spool unavailable → no-op (environmental failure).

— Claude (Fable 5), replying at John's request

[hahn-kev]

Why not use _queue.HardDelete? Per https://github.com/i-e-b/DiskQueue#removing-or-resetting-queues

[johnml1135]

Tried it empirically (DiskQueue 1.7.2): HardDelete(reset: true) recreates the files but leaves the live instance's EstimatedCountOfItemsInQueue stale, so the spool keeps reporting the purged events as present — the purge tests fail with "Expected: 0 But was: 5". That's consistent with its own doc warning ("not thread safe, multi-process safe, or safe in any other way"), so I didn't fight it.

But testing it surfaced something better than the original loop: the dequeue-and-flush approach leaves the purged events' bytes sitting in the data files (DiskQueue doesn't trim consumed entries), which isn't great for a consent purge. So 256eebc reimplements Purge as what HardDelete should have been: dispose the queue, delete the spool directory, reopen fresh — under the spool's own lock. Data is actually gone from disk (a new test scans the raw files to prove it), the count is coherent, and the spool remains usable/reopenable afterward.

— Claude (Fable 5), replying at John's request

[hahn-kev]

Tasks are not one to one with threads. Starting a task like this will just queue it to run on the thread pool and will not guarantee that each task is running at the same time. For a test like this I would explicitly use new Thread, then start them all in a loop, and then join in a loop to ensure they're done before you resume the test.

[johnml1135]

Fixed in 256eebc — dedicated Threads, started in a loop and joined in a loop, so the intended parallelism is explicit instead of at the mercy of thread-pool scheduling.

— Claude (Fable 5), replying at John's request

[hahn-kev]

There are no tests around ProcessBatch max items, only around the queue max size, probably not important but I thought I'd mention it as the tests are otherwise quite thorough

[johnml1135]

Added in 256eebc: 5 events spooled, ProcessBatch(2, …) twice — asserts exactly 2 attempted per call, in order, with the remainder intact between calls.

— Claude (Fable 5), replying at John's request

[hahn-kev]

rather than hand crafting a truncated log couldn't you just put garbage data in there to test this case?

[johnml1135]

Tried it in 256eebc, and the experiment split into two findings, both now pinned as tests:

• Garbage appended to a data file: DiskQueue ignores bytes beyond the extents committed transactions reference — the spool reopens and every committed event survives. Test added.
• Garbage appended to the transaction log: DiskQueue refuses to open (it recovers a truncated log — the realistic torn-write shape — but treats unrecognized bytes after a committed entry as a conflict and throws, surfacing as a lock-wait timeout via PersistentQueue.WaitFor). So "recovers gracefully" isn't testable there because it isn't the behavior; the test instead pins the fail-fast: the constructor throws rather than serving corrupt data, and MixpanelClient.Initialize already catches that and degrades to a non-durable no-op instead of crashing the host.

The fixture NOTE now documents this split instead of declining the test outright.

— Claude (Fable 5), replying at John's request

[hahn-kev]

this doesn't actually prove that the data was scrubbed before hitting the disk because all events are round tripped through the disk before being sent.

[johnml1135]

Correct — asserting on what the sender is handed can't distinguish scrub-at-enqueue from scrub-at-send, so it proved the pipeline output was scrubbed but not the on-disk property the comment claimed. Fixed in 256eebc: before anything drains, the test now scans the raw spool files on disk and asserts the username never appears — and, to avoid passing vacuously, that the scrubbed %USER% form IS found in the persisted bytes. (DiskQueue's exclusively-held lock file is skipped; it only ever contains a PID.)

— Claude (Fable 5), replying at John's request

[johnml1135]

(This reply — and the threaded replies above — are from Claude (Fable 5), acting on John's behalf.)

All 14 inline comments are addressed in 256eebc; each thread has a specific reply. Summary of dispositions:

Fixed as suggested: uninitialized Track now throws; flush interval stored as a TimeSpan; AnalyticsEvent.Create takes plain values instead of factories; single CancellationTokenSource hoisted out of the BoundedDrain loop; _timerDraining comment corrected; concurrency test uses dedicated threads; ProcessBatch maxItems test added; on-disk scrub-before-spool assertion added.

Investigated, landed differently (details in-thread): HardDelete turns out to leave the live queue's count estimate stale, but testing it exposed that the old dequeue-loop purge left purged bytes on disk — Purge is now dispose + delete-directory + reopen, so a consent purge genuinely removes data. The garbage-data corruption test split into two pinned behaviors: data-file garbage is recovered; transaction-log garbage fails fast (and Initialize already degrades gracefully).

Declined with reasoning (details in-thread): /import requires server-side credentials we can't ship in a desktop binary, so /track stays (5-day limit acknowledged as a documented limitation; batching is available on /track itself if volume ever warrants); spool-first stays because the write-ahead is the durability guarantee and "am I online" is only knowable from a send outcome; the per-event retry counter would poison-drop good head-of-queue events during long outages — an enqueue-time size cap is the offer on the table for the pathological-event case.

On async: taking you up on the follow-up-PR offer — IEventSender is internal and the refactor (async ProcessBatch callback, SemaphoreSlim for the spool) deserves its own reviewable change, ideally shaped by FW Lite's actual needs.

Full suite after the changes: 81/81 passing; the production init/timer/purge/resume path re-verified end-to-end via SampleApp.