fix(analytics): Make local-flags polling loop shutdown promptly by tylerjroach · Pull Request #149 · mixpanel/mixpanel-ruby

tylerjroach · 2026-06-15T20:49:19Z

Summary

stop_polling_for_definitions! previously took up to polling_interval_in_seconds to return (3-minute teardown observed during a cross-SDK audit at the default 60s interval — three polling cycles before the test process gave up) because the polling thread blocked on an uninterruptible sleep before checking @stop_polling. Setting the flag couldn't preempt the sleep, so Thread#join had to wait.

Replace the sleep with Mutex#synchronize + ConditionVariable#wait, and have stop_polling_for_definitions! broadcast the condition so the polling thread wakes immediately and breaks out of the loop on the next iteration.

Context

Found during a cross-SDK audit of feature-flag / OpenFeature implementations. The Python and Go server SDKs already handle this correctly (Python via task cancellation, Go via context).

Test plan

All 44 existing local_flags_spec examples pass
Full Ruby spec suite passes (one pre-existing unrelated failure: tracker_spec.rb:23 calls CGI.parse which was extracted out of stdlib in Ruby 4)
Shutdown timing smoke test: stop_polling_for_definitions! returned in ~0.1ms after my change vs. up to 60_000ms before, with the polling thread actively waiting on the condition

🤖 Generated with Claude Code

stop_polling_for_definitions! previously took up to polling_interval_in_seconds to return (3 min teardown observed during cross-SDK audit at the default 60s interval) because the polling thread blocked on an uninterruptible sleep before checking @stop_polling. Replace the sleep with Mutex#synchronize + ConditionVariable#wait, and have stop_polling_for_definitions! broadcast the condition so the thread wakes immediately. Shutdown is now ~0.1ms in a smoke test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

codecov · 2026-06-15T20:54:36Z

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.73%. Comparing base (e5edf71) to head (2cd04ae).

Files with missing lines	Patch %	Lines
lib/mixpanel-ruby/flags/local_flags_provider.rb	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   96.64%   96.73%   +0.08%     
==========================================
  Files          14       14              
  Lines         656      673      +17     
==========================================
+ Hits          634      651      +17     
  Misses         22       22

Flag	Coverage Δ
openfeature	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The previous commit's polling loop called ConditionVariable#wait inside @polling_mutex but checked @stop_polling outside it: @polling_mutex.synchronize do @polling_condition.wait(@polling_mutex, interval) end break if @stop_polling If stop_polling_for_definitions! set @stop_polling = true and called broadcast while the polling thread was outside the mutex (typically mid fetch_flag_definitions), the broadcast had no waiter and was lost. The thread would then re-enter the synchronized region and call wait(interval) again, riding out the full polling interval before observing @stop_polling — reintroducing the original slow-shutdown bug for the duration of one extra tick. Move the predicate check inside the synchronized region so the polling thread sees @stop_polling consistently with the broadcast and skips the wait when shutdown has already been requested. Add two regression specs: - shutdown latency while the thread is parked on the CV (basic case) - shutdown latency when shutdown races an in-flight fetch (race case) Without the predicate-inside-mutex fix, the race spec fails with elapsed ~= polling_interval_in_seconds. With it, elapsed is sub-millisecond. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

tylerjroach · 2026-06-16T13:04:44Z

Pushed deac4ef to close a residual race I missed in the first commit.

What was wrong: The original commit moved the sleep to a ConditionVariable#wait but kept the break if @stop_polling check outside the mutex. If stop_polling_for_definitions! set @stop_polling = true and called broadcast while the polling thread was outside the synchronized region (e.g. mid-fetch_flag_definitions), the broadcast had no waiter and was lost. The thread would then re-enter the synchronized region and call wait(interval) again, riding out the full polling interval before observing the flag — reintroducing the original slow-shutdown bug for one extra tick.

Fix: Move the predicate check inside synchronize, before and after wait:

stopped = @polling_mutex.synchronize do
  next true if @stop_polling
  @polling_condition.wait(@polling_mutex, @config[:polling_interval_in_seconds])
  @stop_polling
end
break if stopped

This is the standard "always check the predicate under the mutex before/after wait" idiom for ConditionVariable.

Regression coverage: Added two specs:

shuts down promptly while the polling thread is waiting on the interval — basic case (thread parked on CV)
shuts down promptly when stop races an in-flight fetch — race case (thread mid-fetch)

I verified the race spec actually catches the bug by temporarily reverting just the predicate-inside-mutex change: the spec failed with expected < 0.5, got 1.005 (full polling interval rode out). With the fix in place, it passes with elapsed sub-millisecond.

All 46 local_flags_spec examples pass.

Copilot

Pull request overview

This PR improves Mixpanel::Flags::LocalFlagsProvider’s polling shutdown behavior by making the polling loop’s interval wait interruptible, so stop_polling_for_definitions! can return promptly instead of waiting out the full polling interval.

Changes:

Replace the polling thread’s sleep interval with a Mutex + ConditionVariable#wait so shutdown can wake the thread immediately.
Update stop_polling_for_definitions! to set the stop flag under the mutex and broadcast the condition before joining the thread.
Add specs that validate prompt shutdown both while waiting on the interval and when stop races an in-flight fetch.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`lib/mixpanel-ruby/flags/local_flags_provider.rb`	Makes polling interval waits interruptible via `ConditionVariable`, and broadcasts on stop to wake the polling thread promptly.
`spec/mixpanel-ruby/flags/local_flags_spec.rb`	Adds regression coverage for prompt shutdown in both “waiting” and “mid-fetch” scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Address Copilot review feedback on PR #149: - Wait for @polling_thread.status == 'sleep' instead of a fixed 0.1 s sleep, so the spec deterministically exercises the CV-wakeup path. - Wrap second_fetch_started.pop in Timeout.timeout(5) so a regression that keeps the polling thread from reaching the second fetch fails fast instead of hanging the suite. - Poll @stop_polling until set instead of a fixed 0.05 s sleep, so the fetch release deterministically lands in the lost-wakeup window the race spec targets. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

linear-code · 2026-06-25T17:25:46Z

SDK-77

Address Copilot review feedback on PR #149: - Wrap both new shutdown specs in begin/ensure so the polling thread is always torn down, even when a Timeout raises or an expectation fails before normal shutdown. - In the race spec, also push to second_fetch_release in ensure to unblock any in-flight stub fetch — otherwise the polling thread would stay parked at second_fetch_release.pop and the join inside stop_polling_for_definitions! would hang the suite. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…olling_thread Address Copilot review feedback on PR #149. start_polling_for_definitions! wrote @stop_polling = false outside the mutex, while stop_polling_for_definitions! wrote it inside. If start raced a concurrent stop after stop's broadcast but before the polling thread reacquired the mutex, start could flip the flag back to false — the polling thread would observe false, keep looping, and stop's join would hang forever. - Take @polling_mutex around the @stop_polling reset and @polling_thread assignment in start so the lifecycle writes are serialized with stop. - In stop, capture @polling_thread and clear it under the mutex (join outside) so a concurrent start can't see a stale reference and skip spawning a fresh thread. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…e mutex Address Copilot review feedback on PR #149. The prior fix moved @stop_polling and @polling_thread writes under @polling_mutex, but stop_polling_for_definitions! still released the mutex before joining. During the join window, a concurrent start_polling_for_definitions! could observe @polling_thread = nil, reset @stop_polling to false, and spawn a new polling thread — leaving the old thread to observe the reset flag, keep looping, and hang the join indefinitely. @polling_mutex can't be held across the join because the polling thread needs it to wake from the condition wait. Introduce @lifecycle_mutex that serializes the full start/stop sequence (including the join) without touching the polling thread's wakeup path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

… defaults Address Copilot review feedback on PR #149. The polling loop now waits on ConditionVariable#wait instead of sleep, and CV#wait treats a nil timeout as "wait forever". Previously, sleep(polling_interval_in_seconds) raised TypeError if a caller passed polling_interval_in_seconds: nil; the new wait silently parks the polling thread forever, disabling polling without any signal. Compact the caller-supplied config before merging into DEFAULT_CONFIG so explicit nils don't override sane defaults. Protects every config key from the same class of bug. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…ecycle on initial fetch Proactive sweep after several rounds of Copilot review on PR #149. - Validate polling_interval_in_seconds in initialize: must be a positive number. Raise ArgumentError loudly. Previously 0 or negative would silently tight-loop in CV#wait (hammering the endpoint), and a non-numeric value would kill the polling thread on first iteration. - Move the initial fetch_flag_definitions in start_polling_for_definitions! outside @lifecycle_mutex. The previous commit moved it inside the mutex, which meant a concurrent stop_polling_for_definitions! would block for the full HTTP request timeout (default 10 s) waiting for start's initial fetch. - Wrap @error_handler.handle in safe_handle_error so a misbehaving handler can't propagate out of the polling loop and kill the thread. Pair with an `.alive?` check in start so a thread that died for any reason can be restarted (otherwise @polling_thread would be non-nil but dead and start would silently refuse to spawn a new one). - Add specs covering the new ArgumentError validation and the nil-fallback path via .compact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Address Copilot review feedback on PR #149. Kernel#sleep(nil) behaves like sleep with no argument — it blocks indefinitely, it doesn't raise. The previous comment claimed the old sleep(nil) would "fail loudly", which was wrong. The .compact fix is still correct (both the old sleep and the new CV#wait silently hang on nil, and falling back to the default avoids that), but the rationale needed to reflect reality. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…h is in flight Address Copilot review feedback on PR #149. The initial fetch in start_polling_for_definitions! is intentionally outside @lifecycle_mutex (so a slow HTTP call can't block a concurrent stop for the full request timeout), but that opened a race: thread A calls start and blocks in the initial fetch; thread B calls stop, sets @stop_polling=true, sees @polling_thread=nil and returns; thread A finishes the fetch, takes @lifecycle_mutex, unconditionally resets @stop_polling=false (inside the spawn branch) and spawns a polling thread. Stop's postcondition ("polling is off when stop returns") was silently violated. Fix per Copilot's suggestion: - Clear @stop_polling at the *top* of start_polling_for_definitions!, before the fetch (under @polling_mutex). - Inside the lifecycle lock after the fetch, re-read @stop_polling and refuse to spawn if a stop arrived in between. Because all writes to @stop_polling go through @polling_mutex, the last-writer-wins semantics guarantee: whichever of {start's clear, stop's set} happened latest is what we observe under the lifecycle lock. Stop's intent is preserved without holding the lifecycle lock across the HTTP request. Add a deterministic regression test that controls the fetch via Queues: verified it fails on the pre-fix code (polling thread non-nil after stop returned) and passes with the fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

tylerjroach requested review from a team and rahul-mixpanel June 15, 2026 20:49

tylerjroach changed the title ~~Make local-flags polling loop shutdown promptly~~ fix(analytics): Make local-flags polling loop shutdown promptly Jun 16, 2026

tylerjroach requested a review from Copilot June 16, 2026 13:06

Copilot started reviewing on behalf of tylerjroach June 16, 2026 13:06 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

Comment thread spec/mixpanel-ruby/flags/local_flags_spec.rb Outdated

Comment thread spec/mixpanel-ruby/flags/local_flags_spec.rb Outdated

Comment thread spec/mixpanel-ruby/flags/local_flags_spec.rb Outdated

tylerjroach requested a review from Copilot June 25, 2026 17:22

Copilot started reviewing on behalf of tylerjroach June 25, 2026 17:23 View session