test: Add failing tests for LDClient.close() releasing all components - #498
Open
aviadr1 wants to merge 1 commit into
Open
test: Add failing tests for LDClient.close() releasing all components#498aviadr1 wants to merge 1 commit into
aviadr1 wants to merge 1 commit into
Conversation
close() is documented as releasing all threads and network connections, but it calls the event processor, the data system and the big segment store manager in sequence with no error handling and no closed-flag. Two failing tests (marked xfail strict, so they will start failing loudly once the behaviour is fixed and the markers can be removed): - test_close_releases_every_component_even_if_one_raises: if the first component's stop() raises, the data system and big segment store manager are never stopped. Two of the three reach code the SDK does not control - the eventsource client, and the application's own BigSegmentStore - so this is a realistic failure, not a contrived one. The caller is left with leaked threads and connections from a client it believes is closed. - test_close_is_idempotent: with no closed-flag, a second close() re-runs the whole sequence, calling stop() twice on the application's BigSegmentStore and store.close() twice under FDv2. The recorded call log shows ['update_processor', 'big_segment_store', 'update_processor', 'big_segment_store']. No fix is proposed here; these only pin the contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Failing tests for #495. Tests only — no fix.
Found while investigating #493, a production incident where
LDClient.close()hung and left worker pods alive for hours to days, each holding a concurrency slot. Auditing the shutdown path afterwards turned up two further defects inclose()itself. I have not observed these two in our own production, but both reproduce reliably.What the tests pin
close()is documented as "Releases all threads and network connections used by the LaunchDarkly client". Two ways it does not:test_close_releases_every_component_even_if_one_raises—client.py:355-358calls the event processor, data system and big segment store manager in sequence with no error handling. If the first raises, the other two are never stopped. Two of the three reach code the SDK does not control: the eventsource client, and the application's ownBigSegmentStoreimplementation. The caller is left with leaked threads and connections from a client it believes is closed.test_close_is_idempotent— there is no closed-flag, so a secondclose()re-runs the whole sequence. The recorded shutdown log is['update_processor', 'big_segment_store', 'update_processor', 'big_segment_store']— the application's own store is stopped twice, and under FDv2store.close()is called twice.Why xfail
Both are marked
@pytest.mark.xfail(strict=True)so CI stays green while the defect is documented in the suite. Because it isstrict, the moment the behaviour is fixed these turn into failures telling you to drop the marker.To see the actual failures:
Why no fix
Whether
close()should still raise after releasing everything is a semantics decision for the SDK team, so the tests pin the leak rather than the exception behaviour. Happy to add the fix if you tell me which way you want it.Note
Overview
Adds
ldclient/testing/test_ldclient_shutdown.py— contract tests for syncLDClient.close()with no production code changes. Both cases are@pytest.mark.xfail(strict=True)so CI stays green until a fix lands.test_close_releases_every_component_even_if_one_raisesdocuments thatclient.pystops the event processor, data system, and big-segment manager in sequence with no error handling; ifstop()on the first raises (e.g. eventsource or userBigSegmentStore), the others never run.test_close_is_idempotentdocuments that without a closed flag, a secondclose()runs shutdown again (e.g. doublestop()on the app’s big-segment store).Uses injectable
Recording*/FailingEventProcessorhelpers and unreachable URIs so tests don’t need network.Reviewed by Cursor Bugbot for commit a10751c. Bugbot is set up for automated code reviews on this repo. Configure here.