fix(analytics): guard polling start so concurrent callers don't leak intervals (SDK-81) - #278
Conversation
tylerjroach
changed the title
tylerjroach
changed the title
|
@greptileai review |
There was a problem hiding this comment.
Pull request overview
This PR makes LocalFeatureFlagsProvider.startPollingForDefinitions() safe to call concurrently by caching the in-flight start operation, preventing redundant initial fetches and ensuring polling setup is only initiated once per lifecycle.
Changes:
- Add
_startPromiseto coalesce concurrentstartPollingForDefinitions()calls into a single async start/fetch. - Reset
_startPromiseonstopPollingForDefinitions()andshutdown()(and on initial fetch failure) to allow clean restarts/retries. - Add Vitest coverage for concurrent starts, restart-after-stop behavior, and retry-after-failure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/mixpanel/lib/flags/local_flags.js | Introduces _startPromise caching and lifecycle resets to dedupe concurrent starts and allow restart/retry. |
| packages/mixpanel/test/flags/local_flags.js | Adds tests validating start coalescing under concurrency and correct cache clearing on stop/failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rvals startPollingForDefinitions had a check-then-act race: the initial _fetchFlagDefinitions() was awaited *before* the `!this.pollingInterval` check, giving the event loop an interleaving point. Two concurrent callers both await the fetch, both observe pollingInterval == null, and both call setInterval — the first interval handle is overwritten and leaks (still scheduled to poll, no way to clear it). Cache the in-flight start as `_startPromise` and short-circuit subsequent calls to share the same promise. stopPollingForDefinitions and shutdown clear the cache so a later start can re-establish polling. Linear: SDK-85 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The initial startPollingForDefinitions guard had two dead-lock states where the cached _startPromise permanently blocked further starts: 1. When enable_polling=false, stop hits the else branch (warn log) without clearing _startPromise. Subsequent start becomes a no-op. Now unconditionally clear, matching shutdown(). 2. When the initial fetch throws, the outer try/catch logs and returns, leaving _startPromise resolved-with-undefined. Callers cannot retry via start() — they'd have to stop first, which logs a spurious warning. Now clear _startPromise in the catch so the next start attempts a fresh fetch. Also reworked the concurrent-start test to assert on _fetchFlagDefinitions call count rather than setInterval count. The !pollingInterval check-then-set is already atomic within a microtask in JS's single-threaded model, so setInterval-count would be 1 with or without the guard. What the guard actually prevents is N redundant fetches under N concurrent starts.
Two doc/behavior polish items surfaced by Copilot on the SDK-81 changes: - _startPromise comment described the wrong failure mode. JS's single-threaded model makes the !this.pollingInterval check + setInterval assignment atomic within a microtask, so setInterval itself was never the race. The guard prevents N concurrent starts each firing their own _fetchFlagDefinitions() and clobbering _initialFetchPromise. Comment now reflects that. - stopPollingForDefinitions warned "polling was not active" whenever pollingInterval was null, including the legitimate enable_polling=false lifecycle (start → stop) and the mid-start case (stop before setInterval fires). Only warn when neither an active interval nor an in-flight _startPromise existed — i.e. the caller truly stopped something that was never started.
8747a6f to
ec98138
Compare
|
Rebased onto master (pulled in service account support) and addressed the three Copilot review threads. Summary: Copilot #1 — Copilot #2 — noisy "polling was not active" warning ( Copilot #3 — real Tests: 48/48 green locally ( |
Per Ketan's review on #278: when a scheduled poll of _fetchFlagDefinitions fails, clear the interval and _startPromise by delegating to stopPollingForDefinitions(). Continuing to poll a failing endpoint just spammed the log without recovering — if the failure is transient (network blip), a follow-up startPollingForDefinitions() gets us back; if it's permanent (revoked token, project deleted), silent retries hide the real problem instead of letting the caller notice. Note: this is a deliberate divergence from the Ruby/Python/Java/Go providers, which log-and-continue. If we want cross-SDK parity in the future, revisit and align. New test "stops polling and resets state when a refresh fails" pins the behavior — mocks setInterval to capture the callback, fires it after seeding a 500, and asserts clearInterval was called with the handle, pollingInterval/_startPromise are cleared, and the error log mentions "stopping polling".
|
Pushed `a91549c` addressing your two threads. Both threads ( New test
Heads up — this is a deliberate divergence from the Ruby / Python / Java / Go providers, which log-and-continue on polling errors. Since your rationale (surface real failures, don't spam) applies equally to the other SDKs, worth opening a follow-up to align them if we want cross-SDK parity. Happy to file that. All 49 flag tests pass. Regarding the earlier three Copilot threads that were still open — those were addressed in |
Summary
startPollingForDefinitionshad a check-then-act race:await this._initialFetchPromisehappens before the!this.pollingIntervalcheck, giving the event loop an interleaving point. Two concurrent callers both await the fetch, both observepollingInterval == null, and both callsetInterval— the first interval handle is overwritten and orphaned (still scheduled to poll, no reference to clear it).Cache the in-flight start as
_startPromiseand short-circuit subsequent calls to share that promise.stopPollingForDefinitionsandshutdownclear the cache so a later start can re-establish polling cleanly.Context
Linear: SDK-81. Same cross-SDK push as the Java and Ruby fixes — three SDKs were affected (Node, Java, Ruby). Python uses a
not self._polling_taskcheck that's safe under the GIL; Go usesCompareAndSwap.Test plan
local_flags.jssuite (45 examples) passes"is idempotent under concurrent startPollingForDefinitions calls and does not leak intervals"test fires 8 concurrentstartPollingForDefinitions()calls and assertssetIntervalwas called exactly once. Before this fix the count would be 8.🤖 Generated with Claude Code