chore: Support flag change listeners in contract tests#410
Merged
Conversation
Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
Contributor
Author
🤖 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:
|
Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
keelerm84
approved these changes
Mar 3, 2026
The test_fdv2_falls_back_to_fdv1_on_polling_success_with_header test was flaky on Windows CI because it waited only 1 second for the flag change listener to be called. The VALID->fallback->FDv1 init path takes longer on slower CI runners. Increased to 2 seconds to match the similar test_fdv2_falls_back_to_fdv1_with_initializer timeout. Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
Move add_flag_value_change_listener inside the lock block so the old listener is removed before the new one is registered, preventing brief duplicate firing when re-registering with the same listener ID. Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
Ensures both registration methods perform all tracker operations under the lock, preventing a race where unregister could miss a listener that hasn't been added to the tracker yet. Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
The test was flaky on Windows CI because it required exactly 2 listener calls before signaling success. On the VALID->fallback path, the first notification from FDv1 init isn't guaranteed to arrive before the explicit update. Simplified to match the pattern used by the error-path test: signal on the first listener call and verify the flag key. Co-Authored-By: mkeeler@launchdarkly.com <keelerm84@gmail.com>
jsonbailey
approved these changes
Mar 4, 2026
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
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.
Requirements
Related issues
Describe the solution you've provided
Implements test service support for the new flag change listener contract tests. No changes to the SDK itself — only the contract test service (
contract-tests/) and one flaky unit test are modified.New file:
contract-tests/flag_change_listener.pyA
ListenerRegistryclass that manages per-client listener subscriptions. It wraps the SDK'sFlagTrackerAPI and POSTsListenerNotificationJSON payloads to a callback URI when listeners fire. Supports:FlagTracker.add_listener()FlagTracker.add_flag_value_change_listener(), which internally handles context-specific evaluation and old/new value trackingclose_all(), called on client entity shutdownAll tracker registration and removal operations happen inside the lock to prevent races where old and new listeners could be briefly active simultaneously, or where unregister could miss a not-yet-added listener.
Updated files:
client_entity.py— InitializesListenerRegistryon client creation; addsregister_flag_change_listener,register_flag_value_change_listener,unregister_listenermethods; callsclose_all()before SDK closeservice.py— Advertisesflag-change-listenersandflag-value-change-listenerscapabilities; routes the three new commandsFlaky test fix:
test_fdv2_datasystem.py—test_fdv2_falls_back_to_fdv1_on_polling_success_with_headerwas flaky on Windows CI. The root cause was that it required exactly 2 listener calls before signaling success, but on theVALID→ fallback → FDv1 init path, the first notification from FDv1 initialization isn't guaranteed to arrive before the explicit update. Simplified the listener to match the pattern used bytest_fdv2_falls_back_to_fdv1_on_polling_error_with_header: signal on the first listener call and verify at least one change has the expected flag key. Also increased the wait timeout from 1s to 2s for additional margin.Items for reviewer attention:
default_valueis accepted but not forwarded to the SDK (flag_change_listener.pyline 47): The test harness sends adefaultValueinregisterFlagValueChangeListener, and the test service accepts it, but the Python SDK'sFlagTracker.add_flag_value_change_listener(key, context, fn)does not take a default value parameter (unlike Go's, which does). The SDK internally evaluates withNoneas the default. This could cause mismatches if a test relies on a non-Nonedefault affecting the initial evaluated value.Value change listener unregistration (
flag_change_listener.pylines 65–70): The SDK'sadd_flag_value_change_listenerreturns an underlying wrapper that is stored (not the user callback) soremove_listenerworks correctly — worth verifying this matches the SDK's API contract.Flaky test weakened assertion: The fix simplifies the listener to signal on the first call instead of requiring 2 calls. This still validates FDv1 is active (via the flag key check), but no longer asserts that the initial FDv1 data load also fires a notification. Worth confirming this is acceptable.
Suggested review checklist:
FlagTracker.add_flag_value_change_listenerreturn value is the correct reference forremove_listenerdefaultValuewill cause any contract test failuresDescribe alternatives you've considered
The implementation follows the same pattern as the Go SDK's contract test service (PR #349), adapted to Python's callback-based
FlagTrackerAPI (vs Go's channel-based approach).Additional context
Link to Devin session | Requested by @keelerm84
Note
Low Risk
Changes are limited to contract-test infrastructure and a unit test timing/assertion tweak; main risk is flakiness or listener cleanup issues affecting CI runs.
Overview
Adds contract-test service support for flag change and flag value change listener contract tests by introducing a per-client
ListenerRegistrythat registers/unregistersFlagTrackerlisteners and POSTs notifications to the harness callback URI.Updates the contract-test HTTP command surface to expose
registerFlagChangeListener,registerFlagValueChangeListener, andunregisterListener, advertises the new capabilities, and ensures listeners are cleaned up on client shutdown.Bumps the CI contract-tests v3 runner to
v3.0.0-alpha.4and adjusts an FDv2 fallback unit test to reduce Windows flakiness by waiting for a specific flag update rather than a fixed number of listener calls.Written by Cursor Bugbot for commit d1a7783. This will update automatically on new commits. Configure here.