fix(analytics): Infinite unbound loop in analytics processor - #57
Conversation
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe analytics worker now waits for incoming events or the next flush deadline instead of polling. It updates analytics data when events arrive, flushes on timer expiry or channel disconnect, clears flushed data, and exits after shutdown. Tests cover flushing pending events when the processor is dropped and handling an unreachable endpoint without panicking. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The shutdown path may return before pending analytics are flushed, so applications that exit immediately can lose recent analytics data; merge should wait for the flush to complete or obtain explicit owner acceptance of this bounded risk. Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a68a8d6b-8440-4680-9eea-a3d4ef6bf1ba
📒 Files selected for processing (1)
src/flagsmith/analytics.rs
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Closes #55.
In this PR, we:
recv_timeout.Measured on an idle processor with no features tracked, over one second of wall clock:
The loop now blocks for whatever remains of the flush window, so it wakes only when a feature is tracked or a flush is due. Flush cadence is unchanged — a
Timeoutjust falls through to the existing flush check. The write lock is taken only when there is data to record or a flush to perform.Disconnectedpreviously broke out of the loop immediately. With the default 10s timer, dropping a client could silently lose up to 10s of analytics. Shutdown now flushes unconditionally before exiting, on a best-effort guarantee (we don't want to add a handle as part of this PR — tracking in #58).The remaining wait uses
timer.saturating_sub(elapsed). Without a guard, a flush that overran its own window (a slow HTTP POST, say) makestimer - elapsedunderflow: a panic in debug, and in release a ~584M year timeout, which would wedge the analytics thread permanently with no further flushes. That was reachable on the original code path too.Added two tests to cover new code.