Repository navigation
test: Avoid hang in big segment status polling specs - #440
Merged
Merged
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
|
@cursor review |
kinyoklion
marked this pull request as ready for review
September 15, 2026 15:44
jsonbailey
approved these changes
Sep 16, 2026
2 of 3 tasks
kinyoklion
added a commit
that referenced
this pull request
Nov 2, 2026
#473) Fixes an intermittent JRuby failure in `big_segment_store_status_provider sends status updates` (`spec/ldclient_listeners_spec.rb:34`, `expected false, got true`), seen on #472. - `spec/mock_components.rb`: adds `GatedBigSegmentStore`, a Big Segment store whose metadata queries wait for the test to supply each result - `spec/ldclient_listeners_spec.rb`: the example adds its observer and checks what it received only while the poller is waiting inside a query **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [ ] I have validated my changes against all supported platform versions <details> <summary>Implementation details</summary> **Root cause** The status poller caches each status inside `@poll_lock` and notifies observers after it releases the lock. `status_provider.status` returns the cached status without waiting for that notification. When the poller's first poll runs before the example reads the status, as it usually does on JRuby, the example can do all of the following before the poller delivers its first notification: 1. Read the healthy status. 2. Add its observer. 3. Inject the store error. The provider has not notified anyone yet, so that first notification goes to the new observer, carrying the healthy status. `statuses.pop` returns it instead of the unavailable status. #440 fixed the same race in the `status polling` examples in `spec/impl/big_segments_spec.rb`. There it caused a hang; here it delivers the wrong first status. **Solution** `GatedBigSegmentStore#get_metadata` counts the query, then waits until the test queues a result. The poller notifies observers of one result before it starts its next query. So once query N has started, every earlier result has been delivered, and nothing else happens until the test queues another result. The example now runs in three steps: 1. It queues a healthy result and reads it through `status_provider.status`. 2. It waits for query 2, then adds the observer. Only the poller can make that query, because later status reads come from the cache. 3. It queues an error and waits for query 3. Then it checks that the observer received exactly one status, that the status is unavailable, and that `status_provider.status` returns it. None of these steps depends on how long a poll or notification takes: - `status_poll_interval: 0.01` only controls how quickly the poller moves on to its next query. - The 5-second wait timeouts only bound how long a failure takes to report. The example releases the store in `ensure`. The client can't stop its poller while a query is waiting. **Alternatives considered** - Skipping statuses until an unavailable one arrives, as `next_status_matching` does in `big_segments_spec.rb`. That would pass, but it would hide an extra or out-of-order notification, and it waits in a sleep loop. - Changing the manager so the first status reaches observers before `status` returns. That changes SDK behavior to suit a test. The manager also deliberately notifies outside `@poll_lock` (see `query_store_status`). **Testing** With no code changes, the failure reproduces on JRuby 9.4.6 pinned to one or two cores. Every failure was the CI one: `expected false, got true` at line 34. | Unmodified SDK, JRuby 9.4.6 | Old example | New example | |---|---|---| | Pinned to 1 core (`taskset -c 0`) | 6/1,000 failed; 8/1,000 on a rerun | 1,000/1,000 passed | | Pinned to 2 cores | 4/1,000 failed | 1,000/1,000 passed | | 8 cores, not pinned | 1,500/1,500 passed | 1,500/1,500 passed | The next rows inject delays, gated by environment variables, into a local copy of `big_segments.rb` and the spec. None of that code is in this PR. The random delays apply at 5 points in the poll and notify path, plus the example's status read and `add_observer`. | Injected delays | Old example | New example | |---|---|---| | Poller polls first, notify delayed 0.3s (MRI 3.2) | 10/10 failed | 10/10 passed | | Same, JRuby 9.4.6 | 10/10 failed | 10/10 passed | | Random delay up to 5ms at each point (MRI 3.2) | 121/300 failed | 300/300 passed | | Same, JRuby 9.4.6 | 141/300 failed | 300/300 passed | | Random delay up to 50ms at each point (MRI 3.2) | 23/60 failed | 60/60 passed | | Same, JRuby 9.4.6 | 23/60 failed | 60/60 passed | The new example still fails when the SDK is broken in each of these ways: - The poller never notifies. - A store error is reported as available. - A change is notified twice. - Polls never update the cached status. - The store is queried only once. The full suite passes on MRI 3.2 (`bundle exec rspec spec`) and on JRuby 9.4.6 (`--tag '~flaky'`, as in CI), with one exception. `launchdarkly-server-sdk_spec.rb:5` fails on `main` too: its unbundled subprocess can't find the gems in my local bundle path. RuboCop reports no offenses on the changed files. **Not addressed here** The same timing can affect an application. If it reads the status and then adds a listener while the client is starting, the listener can receive the status that was just read. That's a redundant notification rather than a wrong status, so this PR doesn't change it. </details> [SDK-3424](https://launchdarkly.atlassian.net/browse/SDK-3424) [SDK-3424]: https://launchdarkly.atlassian.net/browse/SDK-3424?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes a flaky JRuby failure in **`big_segment_store_status_provider sends status updates`** where the observer could receive the initial healthy status instead of the post-error unavailable status, because the status poller notifies observers after caching and the old mock returned results immediately. > > Adds **`GatedBigSegmentStore`** in `spec/mock_components.rb`: metadata **`get_metadata`** blocks until the test **`queue_metadata`** / **`queue_metadata_error`**, exposes **`wait_for_queries`** for poll ordering, and **`release`** so the client can shut down without hanging on a blocked query. > > The listener example switches from **`MockBigSegmentStore`** to the gated store: queue healthy metadata, wait for the second poll before registering the observer, then queue an error and wait for the third poll before asserting exactly one unavailable notification and matching **`status`**. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1ecf5d0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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
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.
Fixes the intermittent JRuby CI hang by removing a race in the big segment status polling specs, and adds a job timeout so a future hang fails fast instead of running until GitHub's limit.
spec/impl/big_segments_spec.rb: the twostatus pollingexamples no longer assume the first status change is observed afteradd_observer.github/workflows/build-gem.yml:timeout-minutes: 20on the gem build jobRequirements
Implementation details
Root cause
BigSegmentStoreManager.newstarts its status poll worker immediately (start_delayof 0), andBigSegmentStoreStatusProviderImpl#update_statusonly notifies observers when the status differs from the previous one. Bothstatus pollingexamples constructed the manager, then added an observer, then blocked onQueue#popfor the initial status. If the worker's first poll ran beforeadd_observer, the initial status was already recorded, no notification was ever sent, and the unboundedQueue#popblocked forever — taking the whole rspec process with it, so RuboCop and the gem build never ran and the job sat until it was cancelled at the 6 hour limit.MRI generally loses that race (a newly spawned thread does not run until the main thread yields), which is why this only showed up on the JRuby job, where threads run truly concurrently.
Solution
The examples now read the initial status directly from
status_provider.statusand use a helper that waits, with a timeout, for the next status matching a predicate. That makes them independent of whether a status was queued before the observer was registered, and a broken expectation now fails instead of hanging.The job timeout is defense in depth: any other future hang in tests, rubocop, or contract tests now fails the job in minutes.
Alternatives considered
Changing the manager to delay the first poll or to always notify on the first status would make the specs pass, but that changes SDK behavior to accommodate a test, and the current behavior (no notification unless the status changes) is intended.
Testing
Reproduced the hang locally on JRuby 9.4.15.0 (2 pinned cores), where the
status pollinggroup hung every attempt before the change and passed repeatedly after it. The full suite (bundle exec rspec spec --tag '~flaky') passes on JRuby with the persistent stores running.Known remaining issue (not addressed here)
ld-eventsourceconnection threads can still outlive an example and call into an expired rspec double viaStreamProcessor#log_connection_result, which prints anExpiredTestDoubleErrorfrom a terminating thread. It is noisy but does not hang or fail the suite; worth a separate look.Link to Devin session: https://app.devin.ai/sessions/700251d712274fcfaabc41ae8e479b3f
Open in Devin Desktop: https://app.devin.ai/desktop/session/700251d712274fcfaabc41ae8e479b3f?variant=devin
Requested by: @kinyoklion
Note
Overview
Fixes intermittent JRuby CI hangs in big segment status polling tests and caps gem build job runtime so future stalls fail fast.
The two
status pollingexamples no longer block on the firstQueue#popafter registering an observer—a race on JRuby where the poll worker can finish before the observer is attached, so no notification fires and the test waits forever. They now assert the initial state viastatus_provider.statusand use a newnext_status_matchinghelper (5s timeout, predicate filter) for subsequent transitions.The build-gem workflow job gets
timeout-minutes: 20as defense in depth.Reviewed by Cursor Bugbot for commit c9aac8f. Bugbot is set up for automated code reviews on this repo. Configure here.