fix(analytics): guard polling start so concurrent callers don't leak intervals (SDK-81)#278
Open
tylerjroach wants to merge 2 commits into
Open
fix(analytics): guard polling start so concurrent callers don't leak intervals (SDK-81)#278tylerjroach wants to merge 2 commits into
tylerjroach wants to merge 2 commits into
Conversation
…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>
tylerjroach
changed the title
tylerjroach
changed the title
4 tasks
Author
Author
|
@greptileai review |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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