feat: expose variation ID, SDK status, and harden runtime safety - #14
feat: expose variation ID, SDK status, and harden runtime safety#14WuJiayi0307 wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughChangesThe SDK adds stable variation IDs and data-update status listeners, hardens client and background-thread shutdown, isolates event failures, adds runtime audits and integration checks, updates production packaging, and documents the 1.1.8 behavior and verification procedures. SDK behavior and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FBClient
participant Streaming
participant EventProcessor
participant NoticeBroadcater
FBClient->>Streaming: stop and join
FBClient->>EventProcessor: stop and join dispatcher
FBClient->>NoticeBroadcater: enqueue sentinel and join
FBClient-->>FBClient: mark closed and isolate component failures
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_fbclient.py (1)
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock down the unchanged JSON contract.
This verifies constructor compatibility but not the stated backward-compatible JSON output. Assert that
to_json_dict()remains unchanged even whenvariation_idis provided.Proposed test addition
assert detail_with_id.variation_id == "variation-id" + assert detail_with_id.to_json_dict() == legacy_detail.to_json_dict()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_fbclient.py` around lines 32 - 39, Extend test_eval_detail_variation_id_is_backward_compatible to call to_json_dict() on detail_with_id and assert its serialized dictionary remains identical to the established legacy JSON contract, excluding variation_id. Keep the existing constructor and variation_id assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@featbit_openfeature/provider.py`:
- Around line 84-94: Update shutdown so _flag_change_listener is cleared only
after _client.flag_tracker.remove_flag_change_notifier(listener) succeeds; if
removal raises, retain the listener reference so cleanup can be retried and
subsequent initialization cannot register a duplicate listener.
In `@README.md`:
- Line 21: Update the README installation sentence to say “Install the SDK using
pip” instead of “install the sdk in using pip,” while preserving the Python
compatibility statement.
In `@release/description.md`:
- Around line 4-6: Update the release-note section in description.md to use the
standard heading “Breaking changes” and the text “No breaking changes,” or
remove the empty section entirely.
---
Nitpick comments:
In `@tests/test_fbclient.py`:
- Around line 32-39: Extend test_eval_detail_variation_id_is_backward_compatible
to call to_json_dict() on detail_with_id and assert its serialized dictionary
remains identical to the established legacy JSON contract, excluding
variation_id. Keep the existing constructor and variation_id assertions
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 457f6a3b-e2e5-43ec-a493-af734f3ee36a
📒 Files selected for processing (22)
.github/workflows/code-quality.yamlMANIFEST.inREADME.mddocs/openfeature.mdfbclient/__init__.pyfbclient/common_types.pyfeatbit_openfeature/__init__.pyfeatbit_openfeature/impl/__init__.pyfeatbit_openfeature/impl/context_converter.pyfeatbit_openfeature/impl/details_converter.pyfeatbit_openfeature/provider.pyfeatbit_openfeature/py.typedrelease/description.mdrelease/package.jsonsetup.pytests/openfeature/__init__.pytests/openfeature/fixtures/bootstrap.jsontests/openfeature/test_context_converter.pytests/openfeature/test_details_converter.pytests/openfeature/test_offline_integration.pytests/openfeature/test_provider.pytests/test_fbclient.py
💤 Files with no reviewable changes (1)
- fbclient/init.py
|
Addressed all four review findings in
Validation after the fixes: |
WuJiayi0307
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fbclient/status.py (1)
59-84: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPreserve transition order when publishing callbacks.
Concurrent
update_statecalls can notify in reverse order: one thread can snapshotOK, release__lock, then another can publishINTERRUPTEDbefore the first invokes its callback. Serialize queued publications in transition order while keeping user callbacks outside__lock, and add a concurrent ordering regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fbclient/status.py` around lines 59 - 84, The update_state callback flow must preserve state-transition order under concurrent calls. Modify update_state and its listener-publication mechanism to queue committed states while holding the existing lock, then drain that queue serially outside __lock so callbacks execute in transition order without invoking user listeners under the lock; add a concurrent regression test verifying ordered OK/INTERRUPTED publications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@fbclient/status.py`:
- Around line 59-84: The update_state callback flow must preserve
state-transition order under concurrent calls. Modify update_state and its
listener-publication mechanism to queue committed states while holding the
existing lock, then drain that queue serially outside __lock so callbacks
execute in transition order without invoking user listeners under the lock; add
a concurrent regression test verifying ordered OK/INTERRUPTED publications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62070b14-1267-4839-993b-9a887e3830d5
📒 Files selected for processing (8)
MANIFEST.inREADME.mdfbclient/interfaces.pyfbclient/status.pyrelease/description.mdrelease/package.jsontests/test_data_update_status_provider.pytests/test_fbclient.py
💤 Files with no reviewable changes (1)
- MANIFEST.in
🚧 Files skipped from review as they are similar to previous changes (2)
- release/package.json
- tests/test_fbclient.py
WuJiayi0307
changed the title
|
Completed the Python SDK reliability review in bf4c842. Key fixes:
Validation:
FeatBit Cloud:
GitHub Actions still requires maintainer approval for this fork workflow. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/REVIEW.md`:
- Around line 82-87: Update the live integration check in
scripts/live_integration_check.py to assert that the recorded state_after_close
equals StateType.OFF, preserving the checklist’s documented OFF transition claim
alongside the existing readiness and thread-cleanup checks.
In `@fbclient/event_processor.py`:
- Around line 82-84: Guard the self.__dispatcher.join(5.0) call in the event
processor shutdown path by checking that the current thread is not
self.__dispatcher, mirroring the existing guards in sibling shutdown
implementations. Preserve the subsequent is_alive warning behavior for joins
that are performed.
In `@fbclient/notice_broadcaster.py`:
- Around line 51-55: Move self.__notice_queue.put(notice) inside the
self.__lock-protected section in broadcast(), immediately after the __closed
check, so enqueueing is serialized with stop() setting __closed and adding the
shutdown sentinel.
In `@scripts/live_integration_check.py`:
- Around line 22-26: Update sdk_threads() and its baseline/comparison logic to
capture each matching thread as an ident-to-name mapping, compare leaked threads
by thread identity rather than name, and retain the mapped names only for
reporting. Apply the same identity-based handling to the referenced lines 92-94
while preserving the existing featbit- thread filter.
In `@scripts/resource_audit.py`:
- Around line 29-31: Validate the --workers and --evaluations arguments before
the audit begins, rejecting values below 1 to prevent invalid worker-pool usage.
Update the workload partitioning near the worker execution logic to use divmod,
distributing the remainder across workers so every requested evaluation is
processed exactly once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19fccbb1-7b77-467d-ac44-c56f2be15355
📒 Files selected for processing (18)
.github/workflows/code-quality.yamlREADME.mddocs/REVIEW.mdfbclient/client.pyfbclient/config.pyfbclient/event_processor.pyfbclient/notice_broadcaster.pyfbclient/status.pyfbclient/streaming.pyfbclient/utils/http_client.pyfbclient/utils/repeatable_task.pyrelease/description.mdscripts/live_integration_check.pyscripts/resource_audit.pysetup.pytests/test_data_update_status_provider.pytests/test_fbclient.pytests/test_runtime_safety.py
🚧 Files skipped from review as they are similar to previous changes (1)
- fbclient/status.py
| 1. authenticated WebSocket connection and initial data synchronization; | ||
| 2. SDK status reaches `OK`; | ||
| 3. remote flag evaluation returns value, reason, and variation ID; | ||
| 4. feature-flag, identify, and custom metric events enter the delivery path; | ||
| 5. explicit flush and final synchronous drain complete; | ||
| 6. status changes to `OFF` and no SDK threads remain after close. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the live-check criterion enforce the documented OFF transition.
The documented script records state_after_close but does not assert that it equals StateType.OFF; it only checks pre-close readiness and leaked threads. Add that assertion to scripts/live_integration_check.py, or remove the OFF-state claim from this checklist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/REVIEW.md` around lines 82 - 87, Update the live integration check in
scripts/live_integration_check.py to assert that the recorded state_after_close
equals StateType.OFF, preserving the checklist’s documented OFF transition claim
alongside the existing readiness and thread-cleanup checks.
| self.__dispatcher.join(5.0) | ||
| if self.__dispatcher.is_alive(): | ||
| log.warning('FB Python SDK: event dispatcher did not stop in time') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing self-join guard on dispatcher join (inconsistent with sibling patterns).
self.__dispatcher.join(5.0) isn't guarded by a current_thread() is not self.__dispatcher check, unlike the equivalent pattern just introduced in streaming.py (Line 285), repeatable_task.py (Line 29), and notice_broadcaster.py (Line 64) in this same PR. If stop() is ever invoked from within the dispatcher's own thread (e.g., a custom event_processor_imp shutdown path), Thread.join() raises RuntimeError: cannot join current thread.
🔧 Proposed fix: mirror the guard used elsewhere
+from threading import current_thread
...
- self.__dispatcher.join(5.0)
- if self.__dispatcher.is_alive():
- log.warning('FB Python SDK: event dispatcher did not stop in time')
+ if current_thread() is not self.__dispatcher:
+ self.__dispatcher.join(5.0)
+ if self.__dispatcher.is_alive():
+ log.warning('FB Python SDK: event dispatcher did not stop in time')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.__dispatcher.join(5.0) | |
| if self.__dispatcher.is_alive(): | |
| log.warning('FB Python SDK: event dispatcher did not stop in time') | |
| from threading import current_thread | |
| ... | |
| if current_thread() is not self.__dispatcher: | |
| self.__dispatcher.join(5.0) | |
| if self.__dispatcher.is_alive(): | |
| log.warning('FB Python SDK: event dispatcher did not stop in time') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fbclient/event_processor.py` around lines 82 - 84, Guard the
self.__dispatcher.join(5.0) call in the event processor shutdown path by
checking that the current thread is not self.__dispatcher, mirroring the
existing guards in sibling shutdown implementations. Preserve the subsequent
is_alive warning behavior for joins that are performed.
| def broadcast(self, notice: Notice): | ||
| with self.__lock: | ||
| if self.__closed: | ||
| return | ||
| self.__notice_queue.put(notice) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Narrow race: a notice can be enqueued after the shutdown sentinel.
broadcast() checks __closed and calls self.__notice_queue.put(notice) outside the lock. If stop() sets __closed=True and enqueues the stop sentinel between those two steps, the real notice lands in the queue after the sentinel, so __run() exits on the sentinel and silently drops that trailing notice during shutdown.
🔒 Proposed fix: enqueue inside the same locked section
def broadcast(self, notice: Notice):
with self.__lock:
if self.__closed:
return
- self.__notice_queue.put(notice)
+ self.__notice_queue.put(notice)Since stop() also sets __closed=True under the same lock before enqueueing the sentinel, this fully serializes the ordering and removes the race.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def broadcast(self, notice: Notice): | |
| with self.__lock: | |
| if self.__closed: | |
| return | |
| self.__notice_queue.put(notice) | |
| def broadcast(self, notice: Notice): | |
| with self.__lock: | |
| if self.__closed: | |
| return | |
| self.__notice_queue.put(notice) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fbclient/notice_broadcaster.py` around lines 51 - 55, Move
self.__notice_queue.put(notice) inside the self.__lock-protected section in
broadcast(), immediately after the __closed check, so enqueueing is serialized
with stop() setting __closed and adding the shutdown sentinel.
| def sdk_threads(): | ||
| return sorted( | ||
| thread.name for thread in threading.enumerate() | ||
| if thread.name.startswith("featbit-") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect leaked threads by identity, not name.
A new leaked SDK thread with the same name as a baseline thread is currently ignored. Capture ident -> name and compare identities, retaining names only for reporting.
Proposed fix
def sdk_threads():
- return sorted(
- thread.name for thread in threading.enumerate()
+ return {
+ thread.ident: thread.name for thread in threading.enumerate()
if thread.name.startswith("featbit-")
- )
+ }
- leaked_threads = [
- name for name in sdk_threads() if name not in baseline_threads
- ]
+ leaked_threads = [
+ name for ident, name in sdk_threads().items()
+ if ident not in baseline_threads
+ ]Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/live_integration_check.py` around lines 22 - 26, Update sdk_threads()
and its baseline/comparison logic to capture each matching thread as an
ident-to-name mapping, compare leaked threads by thread identity rather than
name, and retain the mapped names only for reporting. Apply the same
identity-based handling to the referenced lines 92-94 while preserving the
existing featbit- thread filter.
| parser.add_argument("--evaluations", type=int, default=80000) | ||
| parser.add_argument("--workers", type=int, default=8) | ||
| parser.add_argument("--max-retained-bytes", type=int, default=2 * 1024 * 1024) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate inputs and distribute the requested workload exactly.
--workers 0 raises at Line 52, and non-divisible values under-run the audit (for example, 9 evaluations / 2 workers runs only 8). Reject values below 1 and distribute the remainder with divmod.
Proposed fix
args = parser.parse_args()
+ if args.evaluations < 1 or args.workers < 1:
+ parser.error("--evaluations and --workers must be positive")
- per_worker = max(1, args.evaluations // args.workers)
+ base, remainder = divmod(args.evaluations, args.workers)
+ workloads = [base + (worker < remainder) for worker in range(args.workers)]
- def evaluate(worker):
+ def evaluate(work_item):
+ worker, workload = work_item
completed = 0
try:
- for index in range(per_worker):
+ for index in range(workload):
...
with ThreadPoolExecutor(max_workers=args.workers) as executor:
- completed = sum(executor.map(evaluate, range(args.workers)))
+ completed = sum(executor.map(evaluate, enumerate(workloads)))Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/resource_audit.py` around lines 29 - 31, Validate the --workers and
--evaluations arguments before the audit begins, rejecting values below 1 to
prevent invalid worker-pool usage. Update the workload partitioning near the
worker execution logic to use divmod, distributing the remainder across workers
so every requested evaluation is processed exactly once.
2 participants
Summary
This PR now contains only the FeatBit Python SDK capabilities required by the standalone OpenFeature provider:
EvalDetail.variation_idadd_listener()/remove_listener()EvalDetailconstruction, JSON output, and custom status-provider implementations1.1.8The OpenFeature provider implementation has been moved to the dedicated repository:
https://github.com/featbit/openfeature-provider-python-server
Status mapping support
The provider repository maps FeatBit SDK states as follows:
OK→ OpenFeatureREADYINTERRUPTED→STALEOFF→FATALNOT_READYValidation
58 passed52 passedv2, variantvariation-v2, statusREADYThe SDK PR remains compatible with Python 3.6–3.12. The standalone provider requires Python 3.10+.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation