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 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
fbclient/config.py (1)
176-184: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
deepcopycan raise for non-copyablessloptvalues.
WebSocketConfig.ssloptaccepts any mapping. If a caller stores a non-copyable object there, for example anssl.SSLContext,deepcopy(self.__websocket)raises andcopy_config_in_a_new_envfails. Both config objects only expose read-only properties, so a shallow copy of the wrapper plus an isolateddefaultscopy is sufficient. Consider passing the existing instances, or catching the copy failure and falling back to the original object.🤖 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/config.py` around lines 176 - 184, Update copy_config_in_a_new_env to avoid unconditionally deep-copying self.__http and self.__websocket, since WebSocketConfig.sslopt may contain non-copyable values. Reuse the existing configuration instances or shallow-copy only their wrappers, while retaining the isolated defaults copy and explicit data_storage behavior.fbclient/client.py (2)
169-185: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
stop()holds_stop_lockduring component shutdown.
stop()keeps_stop_lockwhile it callscomponent.stop().DefaultEventProcessor.stop()waits for the dispatcher for up to five seconds, andStreaming.stop()joins its thread. A concurrentstop()call from another thread therefore blocks for the whole shutdown instead of returning at the idempotency check. The behavior stays correct, so this is optional. To make the second caller return immediately, set and test_closedunder the lock, then run the shutdown loop outside the lock.🤖 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/client.py` around lines 169 - 185, Update the client stop flow around _stop_lock and the component shutdown loop: check and set _closed while holding _stop_lock, then release the lock before calling each component.stop(). Preserve the existing idempotency behavior and per-component exception isolation so concurrent stop() calls return immediately while the first caller completes shutdown.
201-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType inference runs before the
Nonecheck.
simple_type_inference(default_value)executes before thedefault_value is Nonebranch. The result is unused in that branch. Move the inference after theNonecheck to avoid an unnecessary call and to keep theNonepath independent of inference behavior.The static analysis hint about
jsonifyat Line 209 does not apply here. This module is not a web framework handler.♻️ Proposed reorder
default_value = self._config.get_default_value(key, default) try: - default_value_type = simple_type_inference(default_value) if default_value is None: return None, None - elif default_value_type == 'boolean': + default_value_type = simple_type_inference(default_value) + if default_value_type == 'boolean': return default_value_type, str(default_value).lower()🤖 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/client.py` around lines 201 - 215, Update __handle_default_value so it checks default_value is None and returns (None, None) before calling simple_type_inference; keep the existing boolean, JSON, and fallback conversions unchanged, with inference-related exception handling applying only to non-None values.Source: Linters/SAST tools
fbclient/event_processor.py (1)
189-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
self.__closed = Trueis assigned twice.Line 189 sets
__closedin the success path. Line 194 sets it again in thefinallyblock. Keep only the assignment infinally, because that path always runs.The Ruff BLE001 hints on the surrounding
except Exceptionblocks are intentional here. Shutdown isolates each cleanup step.🤖 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 189 - 196, Remove the redundant self.__closed = True assignment before the except block in the event dispatcher shutdown logic, keeping the assignment in the finally block so it executes for every cleanup outcome. Preserve the intentional broad exception handling and the existing flush worker shutdown behavior.Source: Linters/SAST tools
🤖 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 `@fbclient/data_storage.py`:
- Around line 37-48: Update InMemoryDataStorage.init so an initialized empty
version-zero snapshot with version equal to self.__version is accepted as an
idempotent no-op. Preserve rejection for older versions and non-empty snapshots,
and keep the existing initialization and version-advance behavior unchanged.
In `@fbclient/streaming.py`:
- Around line 125-134: Update __prepare_connection_timeout so
_WEBSOCKET_TIMEOUT_LOCK is held only while reading and setting the
process-global websocket timeout, then restore the previous timeout and release
the lock immediately after setdefaulttimeout completes. Ensure connection
establishment no longer retains the lock, and preserve exception cleanup if
setting the timeout fails.
In `@tests/test_runtime_safety.py`:
- Around line 165-176: Add an assertion in
test_config_copy_uses_independent_storage_and_exact_queue_capacity that
copied.events_max_in_queue equals 1, alongside the existing original capacity
assertion, so the copied configuration’s queue capacity is verified.
---
Nitpick comments:
In `@fbclient/client.py`:
- Around line 169-185: Update the client stop flow around _stop_lock and the
component shutdown loop: check and set _closed while holding _stop_lock, then
release the lock before calling each component.stop(). Preserve the existing
idempotency behavior and per-component exception isolation so concurrent stop()
calls return immediately while the first caller completes shutdown.
- Around line 201-215: Update __handle_default_value so it checks default_value
is None and returns (None, None) before calling simple_type_inference; keep the
existing boolean, JSON, and fallback conversions unchanged, with
inference-related exception handling applying only to non-None values.
In `@fbclient/config.py`:
- Around line 176-184: Update copy_config_in_a_new_env to avoid unconditionally
deep-copying self.__http and self.__websocket, since WebSocketConfig.sslopt may
contain non-copyable values. Reuse the existing configuration instances or
shallow-copy only their wrappers, while retaining the isolated defaults copy and
explicit data_storage behavior.
In `@fbclient/event_processor.py`:
- Around line 189-196: Remove the redundant self.__closed = True assignment
before the except block in the event dispatcher shutdown logic, keeping the
assignment in the finally block so it executes for every cleanup outcome.
Preserve the intentional broad exception handling and the existing flush worker
shutdown behavior.
🪄 Autofix
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: 3ea2b556-5326-4a5a-aa50-de0d79e58c3e
📒 Files selected for processing (14)
fbclient/client.pyfbclient/config.pyfbclient/data_storage.pyfbclient/event_processor.pyfbclient/interfaces.pyfbclient/status.pyfbclient/streaming.pyfbclient/update_processor.pyfbclient/utils/__init__.pyfbclient/utils/http_client.pytests/test_data_storage.pytests/test_event_processor.pytests/test_fbclient.pytests/test_runtime_safety.py
🚧 Files skipped from review as they are similar to previous changes (2)
- fbclient/utils/http_client.py
- fbclient/interfaces.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fbclient/client.py`:
- Around line 169-192: Update client.stop() around _stop_lock and the component
cleanup loop to track the shutdown owner thread and a completion event:
recursive calls from the owner must return immediately, while other concurrent
callers wait for shutdown completion. Ensure the completion event is set in a
finally block after all components have been attempted, including when cleanup
raises unexpectedly.
🪄 Autofix
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: 0caee1a0-e1d7-4cf7-9142-ce5869e8d991
📒 Files selected for processing (2)
fbclient/client.pytests/test_runtime_safety.py
|
我的和robot提的issue，处理过的要点resolve conversation |
3 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
Greptile Summary
The PR exposes stable variation IDs and SDK update status while hardening evaluation, event delivery, synchronization, and shutdown behavior.
EvalDetail.variation_idwhile preserving existing construction and serialization behavior.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant App participant Client as FBClient participant Evaluator participant Storage as DataStorage participant Events as EventProcessor participant Status as StatusProvider App->>Client: variation_detail(flag, user, default) Client->>Storage: read flag and segment data Client->>Evaluator: evaluate(flag, user) Evaluator-->>Client: value, reason, variation ID Client->>Events: enqueue evaluation event Client-->>App: EvalDetail Status-->>App: notify state listener App->>Client: stop() Client->>Events: stop and drain Client->>Status: transition to OFF Client-->>App: shutdown completeReviews (3): Last reviewed commit: "fix: address lifecycle and concurrency r..." | Re-trigger Greptile