Introduce item and directory subscriptions for library providers#741
Introduce item and directory subscriptions for library providers#741mithunbharadwaj wants to merge 9 commits intomasterfrom
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:
📝 WalkthroughWalkthroughProvider subscribe docstrings adjusted to allow directory-or-item subscriptions; library readiness wait short-circuits when already ready and re-checks readiness after waiting; filesystem provider refactored to per-watch multiple subscriptions, target-aware subscription path resolution, and item-vs-directory handling; ZooKeeper provider replaced polling with persistent recursive watches and normalized subscription descriptors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LibraryService
participant Provider
participant ZooKeeper
Client->>LibraryService: subscribe(paths, target)
LibraryService->>Provider: resolve paths (target-aware)
Provider->>Provider: create subscription descriptors (dir/item)
Provider->>ZooKeeper: register persistent recursive watch (zk_path)
ZooKeeper-->>Provider: deliver watch events
Provider->>LibraryService: publish Library.change! (publish_path)
LibraryService->>Client: deliver change notifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/service.py`:
- Around line 189-190: The except block currently catches asyncio.TimeoutError
and raises LibraryNotReadyError without chaining the original exception; update
the handler that catches asyncio.TimeoutError (the except asyncio.TimeoutError:
block) to re-raise LibraryNotReadyError using "from err" (i.e., raise
LibraryNotReadyError("Library is not ready yet.") from err) so the original
TimeoutError traceback is preserved when LibraryNotReadyError is raised.
- Line 933: Update the subscribe() docstring to state that paths may point to
directories or items and explicitly note that item support is provider-dependent
(e.g., "Paths can point to directories or items; item support depends on the
provider"). Also change the validation error message used when checking absolute
paths (the check invoked from subscribe() and mirrored in the filesystem
provider) from "Directory path must start with '/'" to a provider-agnostic "Path
must start with '/'" so the text correctly applies to all paths; ensure the
subscribe() docstring and the error thrown by the path validation (and any
related checks in the filesystem provider class) are updated to match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| except asyncio.TimeoutError: | ||
| raise LibraryNotReadyError("Library is not ready yet.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout re-raise patterns lacking explicit chaining in this file
rg -nP 'except\s+asyncio\.TimeoutError(?:\s+as\s+\w+)?\s*:\s*\n\s*raise\s+LibraryNotReadyError' asab/library/service.py -URepository: TeskaLabs/asab
Length of output: 155
Chain the caught asyncio.TimeoutError to preserve traceback context (Line 190).
Use from err syntax when re-raising to maintain the root-cause exception chain.
🔧 Proposed fix
- except asyncio.TimeoutError:
- raise LibraryNotReadyError("Library is not ready yet.")
+ except asyncio.TimeoutError as err:
+ raise LibraryNotReadyError("Library is not ready yet.") from err📝 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.
| except asyncio.TimeoutError: | |
| raise LibraryNotReadyError("Library is not ready yet.") | |
| except asyncio.TimeoutError as err: | |
| raise LibraryNotReadyError("Library is not ready yet.") from err |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 190-190: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/service.py` around lines 189 - 190, The except block currently
catches asyncio.TimeoutError and raises LibraryNotReadyError without chaining
the original exception; update the handler that catches asyncio.TimeoutError
(the except asyncio.TimeoutError: block) to re-raise LibraryNotReadyError using
"from err" (i.e., raise LibraryNotReadyError("Library is not ready yet.") from
err) so the original TimeoutError traceback is preserved when
LibraryNotReadyError is raised.
|
|
||
| Args: | ||
| paths (str | list[str]): Either single path or list of paths to be subscribed. All the paths must be absolute (start with '/'). | ||
| paths (str | list[str]): Either a single path or list of paths to be subscribed. Paths can point to directories or items, and all paths must be absolute (start with '/'). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify provider subscribe implementations for directory-only checks / item support parity
rg -n -C2 --type=py 'async def subscribe\(|os\.path\.isdir\(|_subscribe_recursive\(' asab/library/providersRepository: TeskaLabs/asab
Length of output: 6162
🏁 Script executed:
#!/bin/bash
# Check lines 930-970 in service.py to see current docstring state
sed -n '930,970p' asab/library/service.pyRepository: TeskaLabs/asab
Length of output: 1593
🏁 Script executed:
#!/bin/bash
# Get full subscribe method implementations from each provider to check for item support
for provider in zookeeper libsreg git filesystem; do
echo "=== $provider.py ==="
rg -A 20 'async def subscribe\(' asab/library/providers/$provider.py
doneRepository: TeskaLabs/asab
Length of output: 1933
Update API contract to clarify provider-dependent item support, and fix error message.
The subscribe() docstring promises "directories or items" support, but the filesystem provider silently ignores non-directory paths (line 391 in asab/library/providers/filesystem.py). The error message also incorrectly refers to "Directory path" when the check applies to all paths. Update both to reflect that item support is provider-dependent:
Required changes
- paths (str | list[str]): Either a single path or list of paths to be subscribed. Paths can point to directories or items, and all paths must be absolute (start with '/').
+ paths (str | list[str]): Either a single path or list of paths to be subscribed. Paths must be absolute (start with '/'); directory vs item support is provider-dependent.
- message="Directory path must start with '/' when subscribing to Library changes.",
+ message="Path must start with '/' when subscribing to Library changes.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/service.py` at line 933, Update the subscribe() docstring to
state that paths may point to directories or items and explicitly note that item
support is provider-dependent (e.g., "Paths can point to directories or items;
item support depends on the provider"). Also change the validation error message
used when checking absolute paths (the check invoked from subscribe() and
mirrored in the filesystem provider) from "Directory path must start with '/'"
to a provider-agnostic "Path must start with '/'" so the text correctly applies
to all paths; ensure the subscribe() docstring and the error thrown by the path
validation (and any related checks in the filesystem provider class) are updated
to match.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
asab/library/providers/filesystem.py (2)
171-174: Consider extracting_is_item_subscriptionto a shared base class or utility.This method is duplicated identically in both
ZooKeeperLibraryProviderandFileSystemLibraryProvider. While the duplication is minor, extracting it toLibraryProviderABCwould ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/providers/filesystem.py` around lines 171 - 174, The _is_item_subscription static method is duplicated in ZooKeeperLibraryProvider and FileSystemLibraryProvider; extract it into the shared abstract base (LibraryProviderABC) as a single `@staticmethod` named _is_item_subscription and remove the duplicate implementations. Update both ZooKeeperLibraryProvider and FileSystemLibraryProvider to call LibraryProviderABC._is_item_subscription (or inherit it) so they use the centralized implementation and ensure imports/exports and tests still pass.
616-619: Consider clearing subscription state in finalize for explicitness.Unlike the ZooKeeper provider which clears all subscription-related state in
finalize, this provider only closes the file descriptor. While functionally correct (the object is being destroyed anyway), explicitly clearingWDs,WDSubscriptions, andWatchedPathswould be more consistent with the ZooKeeper implementation.♻️ Proposed fix for consistency
async def finalize(self, app): if self.FD is not None: self.App.Loop.remove_reader(self.FD) os.close(self.FD) + self.WDs.clear() + self.WDSubscriptions.clear() + self.WatchedPaths.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/providers/filesystem.py` around lines 616 - 619, The finalize method currently only removes the reader and closes self.FD; update finalize (async def finalize(self, app)) to also clear subscription-related state for consistency with the ZooKeeper provider by resetting self.WDs, self.WDSubscriptions, and self.WatchedPaths to empty containers after removing the reader and closing the FD (and still guard on self.FD being not None), ensuring no stale subscription state remains referenced when the provider is finalized.asab/library/providers/zookeeper.py (2)
465-473: Consider removing redundant hasattr checks.These attributes are already initialized in
__init__(lines 178-185). The defensivehasattrchecks suggest concern about initialization order, but they add unnecessary complexity ifsubscribe()is only called after__init__completes.♻️ Proposed simplification
async def subscribe(self, path, target: typing.Union[str, tuple, None] = None): - if not hasattr(self, "SubscriptionDescriptors"): - self.SubscriptionDescriptors = {} - if not hasattr(self, "WatchSubscriptions"): - self.WatchSubscriptions = {} - if not hasattr(self, "PersistentWatches"): - self.PersistentWatches = set() - if not hasattr(self, "SubscriptionActualPaths"): - self.SubscriptionActualPaths = {} - self._persistent_recursive_watch_mode()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/providers/zookeeper.py` around lines 465 - 473, The hasattr defensive checks in subscribe should be removed because those attributes are initialized in __init__; edit the subscribe method to delete the hasattr branches that set SubscriptionDescriptors, WatchSubscriptions, PersistentWatches, and SubscriptionActualPaths and assume they exist (or replace with a single assertion if you want a runtime check), referencing the subscribe method and the attributes SubscriptionDescriptors, WatchSubscriptions, PersistentWatches, SubscriptionActualPaths and the class __init__ for context.
500-512: Same redundant hasattr check pattern.Similar to the
subscribe()method, this hasattr check is unnecessary if__init__always runs first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/providers/zookeeper.py` around lines 500 - 512, The redundant hasattr check in _get_subscription_actual_paths should be removed; instead ensure SubscriptionActualPaths is initialized in the class constructor (so __init__ always sets self.SubscriptionActualPaths = {}) and then simplify _get_subscription_actual_paths by assuming self.SubscriptionActualPaths exists (remove the if hasattr(self, "SubscriptionActualPaths") block and rely on key = (target, path), self.SubscriptionActualPaths.get(...), and assignment). This mirrors the fix used for subscribe() and keeps the method concise and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@asab/library/providers/filesystem.py`:
- Around line 171-174: The _is_item_subscription static method is duplicated in
ZooKeeperLibraryProvider and FileSystemLibraryProvider; extract it into the
shared abstract base (LibraryProviderABC) as a single `@staticmethod` named
_is_item_subscription and remove the duplicate implementations. Update both
ZooKeeperLibraryProvider and FileSystemLibraryProvider to call
LibraryProviderABC._is_item_subscription (or inherit it) so they use the
centralized implementation and ensure imports/exports and tests still pass.
- Around line 616-619: The finalize method currently only removes the reader and
closes self.FD; update finalize (async def finalize(self, app)) to also clear
subscription-related state for consistency with the ZooKeeper provider by
resetting self.WDs, self.WDSubscriptions, and self.WatchedPaths to empty
containers after removing the reader and closing the FD (and still guard on
self.FD being not None), ensuring no stale subscription state remains referenced
when the provider is finalized.
In `@asab/library/providers/zookeeper.py`:
- Around line 465-473: The hasattr defensive checks in subscribe should be
removed because those attributes are initialized in __init__; edit the subscribe
method to delete the hasattr branches that set SubscriptionDescriptors,
WatchSubscriptions, PersistentWatches, and SubscriptionActualPaths and assume
they exist (or replace with a single assertion if you want a runtime check),
referencing the subscribe method and the attributes SubscriptionDescriptors,
WatchSubscriptions, PersistentWatches, SubscriptionActualPaths and the class
__init__ for context.
- Around line 500-512: The redundant hasattr check in
_get_subscription_actual_paths should be removed; instead ensure
SubscriptionActualPaths is initialized in the class constructor (so __init__
always sets self.SubscriptionActualPaths = {}) and then simplify
_get_subscription_actual_paths by assuming self.SubscriptionActualPaths exists
(remove the if hasattr(self, "SubscriptionActualPaths") block and rely on key =
(target, path), self.SubscriptionActualPaths.get(...), and assignment). This
mirrors the fix used for subscribe() and keeps the method concise and
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84d9e6fc-2c29-4e2d-ad1a-628a0bbdd980
📒 Files selected for processing (2)
asab/library/providers/filesystem.pyasab/library/providers/zookeeper.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/providers/zookeeper.py`:
- Around line 219-222: The provider is restoring watches on the CONNECTED event
too early (ZooKeeperContainer.state/CONNECTED fires before its ensure_path()
completes), so change the CONNECTED handler to wait for the container's
path-setup to finish before calling _restore_subscription_watches() and then
_set_ready(); specifically, ensure you await the container.ensure_path() (or
loop/wait until ensure_path task completes) before invoking
self._restore_subscription_watches(), and if self._ensure_subscription_watch()
encounters a NoNodeError make it retry (with a short backoff) or surface the
error so the restore is retried instead of silently dropped; keep _set_ready()
only after successful watch restoration.
- Around line 475-478: The code is caching expanded aggregate targets into
SubscriptionDescriptors and short-circuiting future subscribes, so aggregate
targets like "personal" become a one-time snapshot; in subscribe(), detect
aggregate/wildcard targets (e.g., target == "personal" or other aggregate forms)
and do NOT store their resolved concrete (Tenant, CredentialsId) pairs into
SubscriptionDescriptors; instead store the original aggregate key (or a special
marker) in Subscriptions/SubscriptionDescriptors and ensure subscribe() always
re-resolves aggregate expansions (scanning tenants/credentials) on each call
rather than returning early when a cached concrete descriptor exists; update any
related logic that adds to SubscriptionDescriptors (including where resolved
paths are cached) to preserve an aggregate sentinel so new tenants/scopes added
later will be discovered.
- Around line 175-176: Change the ZooKeeper state handling so only the LOST
state triggers clearing and rebuilding of persistent watches: unsubscribe the
SUSPENDED subscription from _on_zk_lost and instead subscribe SUSPENDED to a new
or existing handler that does not clear PersistentWatches. Update the logic in
the handlers referenced around the code that manipulates PersistentWatches (the
block that currently clears/rebuilds watches in the _on_zk_lost/_on_zk_suspended
area) so that clearing and re-registration of watches happens only in the
_on_zk_lost path, while the _on_zk_suspended path simply logs or marks transient
suspension without wiping PersistentWatches (adjust the methods _on_zk_lost and
the SUSPENDED handler accordingly).
🪄 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: 5fb30283-4208-44da-accb-eca87d97f7dc
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py
| self.App.PubSub.subscribe("ZooKeeperContainer.state/LOST!", self._on_zk_lost) | ||
| self.App.PubSub.subscribe("ZooKeeperContainer.state/SUSPENDED!", self._on_zk_lost) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific lines mentioned
wc -l asab/library/providers/zookeeper.pyRepository: TeskaLabs/asab
Length of output: 1868
🏁 Script executed:
# Read the relevant sections of the file
sed -n '170,230p' asab/library/providers/zookeeper.pyRepository: TeskaLabs/asab
Length of output: 1867
🏁 Script executed:
# Search for the method implementations mentioned in the comment
rg -n "_on_zk_lost|_on_zk_connected" asab/library/providers/zookeeper.py -A 5Repository: TeskaLabs/asab
Length of output: 1059
🏁 Script executed:
# Get more details on the _on_zk_connected method and surrounding code
sed -n '201,250p' asab/library/providers/zookeeper.pyRepository: TeskaLabs/asab
Length of output: 1485
🏁 Script executed:
# Search for where PersistentWatches is used/referenced
rg -n "PersistentWatches" asab/library/providers/zookeeper.py -B 2 -A 2Repository: TeskaLabs/asab
Length of output: 1559
🏁 Script executed:
# Find _restore_subscription_watches implementation
rg -n "_restore_subscription_watches" asab/library/providers/zookeeper.py -A 15Repository: TeskaLabs/asab
Length of output: 1280
🏁 Script executed:
# Check if there's a _ensure_subscription_watch that adds watches
rg -n "_ensure_subscription_watch" asab/library/providers/zookeeper.py -B 2 -A 12Repository: TeskaLabs/asab
Length of output: 1958
🏁 Script executed:
# Find where add_watch is actually called
rg -n "add_watch" asab/library/providers/zookeeper.py -B 3 -A 5Repository: TeskaLabs/asab
Length of output: 834
🏁 Script executed:
# Check the _persistent_recursive_watch_mode method
rg -n "_persistent_recursive_watch_mode" asab/library/providers/zookeeper.py -A 10Repository: TeskaLabs/asab
Length of output: 1963
🏁 Script executed:
# Search for Kazoo library usage and version info
find . -name "*.py" -o -name "*.txt" -o -name "*.cfg" -o -name "*.toml" | head -20Repository: TeskaLabs/asab
Length of output: 496
🏁 Script executed:
# Search for requirements, setup.py, or pyproject.toml files
fd -t f "(requirements|setup|pyproject)" . | head -20Repository: TeskaLabs/asab
Length of output: 80
🏁 Script executed:
# Check setup files for kazoo dependency
find . -maxdepth 2 \( -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" -o -name "requirements*.txt" \) -exec cat {} \;Repository: TeskaLabs/asab
Length of output: 3546
🏁 Script executed:
# Check Kazoo version and look for any vendored kazoo in the repo
find . -type d -name "kazoo" 2>/dev/nullRepository: TeskaLabs/asab
Length of output: 40
🏁 Script executed:
# Look at imports and kazoo usage more carefully
grep -r "from kazoo" asab/library/providers/zookeeper.py | head -10Repository: TeskaLabs/asab
Length of output: 40
🌐 Web query:
Kazoo SUSPENDED vs LOST state semantics ZooKeeper persistent watches
💡 Result:
Kazoo connection states
KazooState.SUSPENDED: the TCP connection to the ensemble is currently down (or the server you were on dropped out of quorum). The ZooKeeper session might still be valid and could recover on reconnect. Kazoo treats this as a temporary disconnect and (newer Kazoo) will queue requests expecting recovery. [1][2]KazooState.LOST: Kazoo has determined the session is dead/expired (or you stopped the client). All ephemerals from that session are gone, so locks/ephemeral-based recipes must be treated as lost and recreated after reconnect. Requests fail immediately in this state. [1][2][3]
What this means for ZooKeeper persistent watches (addWatch())
- ZooKeeper standard watches are normally re-registered by the client library after reconnect (if the session survives), and you don’t receive watch notifications while disconnected. [4][5]
- ZooKeeper persistent watches (
addWatchwithPERSISTENT/PERSISTENT_RECURSIVE) do not auto-remove when triggered; they stay active until explicitly removed, and otherwise have the same triggering semantics/guarantees as standard watches. [5][6]
Putting it together:
- If you go
SUSPENDED→CONNECTED(session survived): your persistent watches should still be in effect, but you may have missed events during the disconnect, so you typically re-list/re-read state and reconcile. (ZooKeeper explicitly warns you won’t get watches while disconnected.) [5] - If you go to
LOST(session expired): you must assume everything tied to the session is gone (ephemerals, locks, and also server-side registrations such as watches). Create a new session/client and re-register persistent watches (and recreate ephemerals/locks) after you’re connected again. ZooKeeper also treats a session-expired handle as invalid. [1][3][4]
Rule of thumb:
- SUSPENDED = temporary connection loss; pause/act conservatively; reconcile on reconnect. [1][5]
- LOST = definitive session loss; rebuild session-scoped state (ephemerals, locks, persistent watches). [1][3][4]
Sources
[1] Kazoo “Basic Usage” – Understanding Kazoo States / LOST vs SUSPENDED semantics
[2] KazooClient docs – queuing requests in SUSPENDED; failing in LOST
[3] kazoo.protocol.states – LOST: ephemerals must be recreated
[4] ZooKeeper Programmer’s Guide – session expiration; handle invalid after SESSION_EXPIRED
[5] ZooKeeper Programmer’s Guide (3.8.x) – watch delivery during disconnect; re-registration; persistent watches overview
[6] ZooKeeper AddWatchMode API – PERSISTENT / PERSISTENT_RECURSIVE semantics
🌐 Web query:
Kazoo add_watch PERSISTENT_RECURSIVE repeated calls behavior
💡 Result:
With ZooKeeper persistent recursive watches (AddWatchMode.PERSISTENT_RECURSIVE), repeated add_watch() calls behave like this:
- The watch is persistent: it is not removed after it fires; it stays active until explicitly removed (ZooKeeper: remove via
removeWatches(..., WatcherType.Any)). (zookeeper.apache.org) - If you repeatedly add the same watcher/callback for the same path, it is generally treated as non-stacking / de-duplicated (i.e., you should not expect the callback to run N times just because you registered it N times).
- ZooKeeper’s client semantics: if the same watch object is registered more than once for the same znode/event, it is only invoked once for a given event. (svn.apache.org)
- Kazoo specifically fixed “stacking” so the same watch function isn’t registered multiple times for the same path. (kazoo.readthedocs.io)
- In implementations that add persistent recursive watch support by storing watchers in a
set, repeated registrations of the same callback for the same path become inherently idempotent (only one entry). (opendev.org)
- If you call
add_watch()with different callable objects (e.g., new lambdas/closures each time), those are distinct watchers and can accumulate, meaning multiple callbacks may fire.
So, repeated calls are usually “safe but redundant” when the callback identity is the same; they only “stack” if you keep passing new watcher callables or you’re using a client implementation that doesn’t de-duplicate.
Citations:
- 1: https://zookeeper.apache.org/doc/current/apidocs/zookeeper-server/org/apache/zookeeper/AddWatchMode.html
- 2: https://svn.apache.org/repos/asf/zookeeper/branches/branch-3.0/docs/zookeeperProgrammers.html?utm_source=openai
- 3: https://kazoo.readthedocs.io/en/2.0/changelog.html?utm_source=openai
- 4: https://opendev.org/zuul/zuul/commit/bbb4fa7ff0f300f74617b13ef5f87d929f3b8d81
Handle SUSPENDED and LOST states differently for persistent watch bookkeeping.
Kazoo documents SUSPENDED as a recoverable state where the session may survive, while LOST means the session is confirmed dead. ZooKeeper persistent recursive watches remain active on the server until explicitly removed. Clearing PersistentWatches on SUSPENDED forces unnecessary re-registration of watches that are still live—Kazoo will de-duplicate the add_watch() calls with the same callback, making the churn redundant. Only clear and rebuild watches on LOST.
Apply this to lines 175–176, 219–220, and 224–230.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/zookeeper.py` around lines 175 - 176, Change the
ZooKeeper state handling so only the LOST state triggers clearing and rebuilding
of persistent watches: unsubscribe the SUSPENDED subscription from _on_zk_lost
and instead subscribe SUSPENDED to a new or existing handler that does not clear
PersistentWatches. Update the logic in the handlers referenced around the code
that manipulates PersistentWatches (the block that currently clears/rebuilds
watches in the _on_zk_lost/_on_zk_suspended area) so that clearing and
re-registration of watches happens only in the _on_zk_lost path, while the
_on_zk_suspended path simply logs or marks transient suspension without wiping
PersistentWatches (adjust the methods _on_zk_lost and the SUSPENDED handler
accordingly).
| if len(self.WatchSubscriptions) > 0: | ||
| await self._restore_subscription_watches() | ||
|
|
||
| await self._set_ready() |
There was a problem hiding this comment.
Delay watch restoration until the container’s CONNECTED setup is complete.
This callback assumes ZooKeeperContainer.state/CONNECTED! means the container path is ready, but asab/zookeeper/container.py:132-186 publishes that event before its scheduled ensure_path() work is guaranteed to finish. If _restore_subscription_watches() hits NoNodeError, _ensure_subscription_watch() drops the reinstall silently and the provider still transitions to ready, so a reconnect can leave subscriptions inactive until the next successful restore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/zookeeper.py` around lines 219 - 222, The provider is
restoring watches on the CONNECTED event too early
(ZooKeeperContainer.state/CONNECTED fires before its ensure_path() completes),
so change the CONNECTED handler to wait for the container's path-setup to finish
before calling _restore_subscription_watches() and then _set_ready();
specifically, ensure you await the container.ensure_path() (or loop/wait until
ensure_path task completes) before invoking
self._restore_subscription_watches(), and if self._ensure_subscription_watch()
encounters a NoNodeError make it retry (with a short backoff) or surface the
error so the restore is retried instead of silently dropped; keep _set_ready()
only after successful watch restoration.
| key = (target, path) | ||
| self.Subscriptions.add(key) | ||
| if key in self.SubscriptionDescriptors: | ||
| return |
There was a problem hiding this comment.
Aggregate targets are being narrowed to a one-time concrete snapshot.
target="personal" is documented in asab/library/service.py:922-940 as “all personal credential IDs”, but this implementation only resolves the current (Tenant, CredentialsId) pair and returns no paths outside that context. subscribe() then caches that empty result and short-circuits future calls for the same key, so a startup-time personal subscription can become permanently inert. The same one-shot expansion means aggregate targets only ever watch the scopes that exist at subscribe time; new tenants or personal scopes created later will never get a watch.
Also applies to: 495-496, 512-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/zookeeper.py` around lines 475 - 478, The code is
caching expanded aggregate targets into SubscriptionDescriptors and
short-circuiting future subscribes, so aggregate targets like "personal" become
a one-time snapshot; in subscribe(), detect aggregate/wildcard targets (e.g.,
target == "personal" or other aggregate forms) and do NOT store their resolved
concrete (Tenant, CredentialsId) pairs into SubscriptionDescriptors; instead
store the original aggregate key (or a special marker) in
Subscriptions/SubscriptionDescriptors and ensure subscribe() always re-resolves
aggregate expansions (scanning tenants/credentials) on each call rather than
returning early when a cached concrete descriptor exists; update any related
logic that adds to SubscriptionDescriptors (including where resolved paths are
cached) to preserve an aggregate sentinel so new tenants/scopes added later will
be discovered.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/providers/abc.py`:
- Around line 69-71: _is_item_subscription currently strips trailing slashes
then classifies the final path segment, causing directory paths like
"/.tenants/" or "/foo.bar/" to be misclassified as items; update
_is_item_subscription to first check for a trailing "/" and immediately return
False for directory paths, then extract the final segment with rsplit("/",
1)[-1] (without rstrip) and apply the existing dot-and-suffix checks (the "."
presence and not ending with (".io", ".d")) so only real item names are
classified as items.
In `@asab/library/providers/filesystem.py`:
- Around line 409-410: The current target=="tenant" branch only subscribes to
tenants returned by _get_tenants() at call time, missing tenants created later;
change it to also subscribe to the tenants directory itself and dynamically add
per-tenant subscriptions on new entries. Specifically, update the logic around
the target=="tenant" branch (referencing build_path and _get_tenants) to: 1)
subscribe to the parent tenants path (e.g. build_path("/.tenants",
tenant_specific=False)) so you receive create/delete events for tenant entries,
and 2) on receiving a new-tenant create event, call build_path(new_tenant_path,
tenant_specific=True, tenant=new_tenant) and add that tenant to the active
subscription set (extract this into a helper like _add_tenant_subscription if
helpful). Ensure removal on tenant deletion and avoid duplicating subscriptions
for already-known tenants.
- Around line 415-420: The current branch for target == "personal" only resolves
the single current tenant+credentials via _current_tenant_id() and
_current_credentials_id() and returns one _personal_path, but "personal" should
subscribe all personal scopes; update subscribe() so that when target ==
"personal" it enumerates all tenant IDs and credential IDs (e.g., via existing
helpers or by adding helpers like _all_tenant_ids()/_all_credentials_ids()),
call _personal_path(path, tenant_id, cred_id) for each pair, collect non-None
results into the returned list, and remove the single-current-context shortcut
so all personal scopes are included.
- Around line 412-413: The code passes target[1] directly into self.build_path
as tenant; validate and normalize that tenant ID before calling build_path:
ensure it's non-empty and does not contain path separators ('/', '\\'),
traversal tokens ('..'), or other unsafe characters (e.g. require a whitelist
like /^[A-Za-z0-9._-]+$/), and either raise a ValueError on invalid input or
sanitize/encode the value and then call self.build_path(path,
tenant_specific=True, tenant=<validated_or_sanitized_value>); refer to the tuple
check for target and the self.build_path(..., tenant=target[1]) call to locate
where to add this validation.
In `@docs/reference/services/library.md`:
- Around line 120-132: Fix the example by correcting indentation and the missing
closing parenthesis in the subscription calls inside class MyApplication: make
initialize an async method with properly indented body and call
self.PubSub.subscribe("Library.ready!", self.on_library_ready) and
self.PubSub.subscribe("Library.change!", self.on_library_change) (add the
missing “)”). Also ensure the callback contracts are consistent by making
on_library_change async (async def on_library_change(self, message, provider,
path)) to match on_library_ready and the expected coroutine handler for
LibraryService.subscribe and PubSub events; keep await
self.LibraryService.subscribe(["/asab"]) inside on_library_ready as shown.
- Around line 111-114: The message/provider matrix was rendered as plain
tab-separated text; restore proper Markdown table syntax by converting the block
that lists "Message", "Published when" and the rows containing
"Library.not_ready!", "Library.ready!", and "Library.change!" into a Markdown
table (include the header row and the separator line with dashes and pipes), and
do the same for the other occurrence covering lines referenced (the block at the
second instance with the same entries). Locate the blocks by the unique message
names (Library.not_ready!, Library.ready!, Library.change!) and replace the
tab-separated lines with well-formed Markdown table rows so MkDocs will render
them correctly.
- Around line 35-37: The admonition blocks starting with the marker "!!! tip"
(and other "!!!" admonitions) are malformed because their body text is not
indented beneath the marker; fix each by indenting the body lines under the
corresponding "!!! <kind>" marker (e.g., indent the paragraph after "!!! tip" by
at least one level—commonly 4 spaces or a tab) so the content is recognized and
rendered as the admonition; apply the same change to the other occurrences of
"!!!" admonitions in the file.
- Line 1: Several section titles are written as plain paragraphs (e.g., "Library
Service") instead of Markdown headings, which breaks TOC and navigation; update
each plain-title line to use the appropriate Markdown heading level (prepend one
or more '#' as needed, for example change "Library Service" to "# Library
Service" or "## Library Service" depending on its hierarchy) and ensure other
similar title lines in the document are converted the same way so the headings
are recognized for TOC/in-page anchors.
- Line 350: Fix the typo at the start of the sentence "Is is assumed that the
content archives are uploaded to the distribution point using CI/CD." by
changing "Is is" to "It is" (update the sentence in
docs/reference/services/library.md so it reads "It is assumed that the content
archives are uploaded to the distribution point using CI/CD.").
- Around line 13-21: The fenced code block that begins with triple backticks and
contains the directory listing (the block showing /MyDirectory/,
/AnotherDirectory/, etc.) lacks a language identifier; update the opening fence
to include a language (e.g., add "text" so it becomes ```text) so markdown
linters and syntax highlighters treat it correctly and preserve formatting.
🪄 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: fd5383c9-7547-46fe-9746-3e324a16d25d
📒 Files selected for processing (4)
asab/library/providers/abc.pyasab/library/providers/filesystem.pyasab/library/providers/zookeeper.pydocs/reference/services/library.md
🚧 Files skipped from review as they are similar to previous changes (1)
- asab/library/providers/zookeeper.py
| def _is_item_subscription(path: str) -> bool: | ||
| name = path.rstrip("/").rsplit("/", 1)[-1] | ||
| return "." in name and not name.endswith((".io", ".d")) |
There was a problem hiding this comment.
_is_item_subscription() can misclassify dotted directories as items
Line 70 removes trailing / before classification, so paths like /.tenants/ or /foo.bar/ are treated as items. That can route directory subscriptions into wrong handling.
Suggested fix
`@staticmethod`
def _is_item_subscription(path: str) -> bool:
- name = path.rstrip("/").rsplit("/", 1)[-1]
- return "." in name and not name.endswith((".io", ".d"))
+ # Explicit trailing slash means directory subscription.
+ if path.endswith("/"):
+ return False
+ name = path.rstrip("/").rsplit("/", 1)[-1]
+ # Hidden/internal dotted directories should not be treated as items.
+ if name.startswith(".") and not name.endswith((".io", ".d")):
+ return False
+ return "." in name and not name.endswith((".io", ".d"))📝 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 _is_item_subscription(path: str) -> bool: | |
| name = path.rstrip("/").rsplit("/", 1)[-1] | |
| return "." in name and not name.endswith((".io", ".d")) | |
| def _is_item_subscription(path: str) -> bool: | |
| # Explicit trailing slash means directory subscription. | |
| if path.endswith("/"): | |
| return False | |
| name = path.rstrip("/").rsplit("/", 1)[-1] | |
| # Hidden/internal dotted directories should not be treated as items. | |
| if name.startswith(".") and not name.endswith((".io", ".d")): | |
| return False | |
| return "." in name and not name.endswith((".io", ".d")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/abc.py` around lines 69 - 71, _is_item_subscription
currently strips trailing slashes then classifies the final path segment,
causing directory paths like "/.tenants/" or "/foo.bar/" to be misclassified as
items; update _is_item_subscription to first check for a trailing "/" and
immediately return False for directory paths, then extract the final segment
with rsplit("/", 1)[-1] (without rstrip) and apply the existing dot-and-suffix
checks (the "." presence and not ending with (".io", ".d")) so only real item
names are classified as items.
| if target == "tenant": | ||
| return [self.build_path(path, tenant_specific=True, tenant=tenant) for tenant in self._get_tenants()] |
There was a problem hiding this comment.
target="tenant" subscription is a one-time snapshot
Line 410 subscribes only tenants present at subscription time. New tenants created later under /.tenants won’t be watched, so change events can be missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/filesystem.py` around lines 409 - 410, The current
target=="tenant" branch only subscribes to tenants returned by _get_tenants() at
call time, missing tenants created later; change it to also subscribe to the
tenants directory itself and dynamically add per-tenant subscriptions on new
entries. Specifically, update the logic around the target=="tenant" branch
(referencing build_path and _get_tenants) to: 1) subscribe to the parent tenants
path (e.g. build_path("/.tenants", tenant_specific=False)) so you receive
create/delete events for tenant entries, and 2) on receiving a new-tenant create
event, call build_path(new_tenant_path, tenant_specific=True, tenant=new_tenant)
and add that tenant to the active subscription set (extract this into a helper
like _add_tenant_subscription if helpful). Ensure removal on tenant deletion and
avoid duplicating subscriptions for already-known tenants.
| if isinstance(target, tuple) and len(target) == 2 and target[0] == "tenant": | ||
| return [self.build_path(path, tenant_specific=True, tenant=target[1])] |
There was a problem hiding this comment.
Validate tenant ID before path construction
Line 413 passes target[1] directly into tenant path construction. Values with path separators or traversal tokens can escape the intended .tenants/<tenant> scope.
Suggested fix
+ def _validate_scope_component(self, value: str, name: str) -> str:
+ if not isinstance(value, str) or value in {"", ".", ".."}:
+ raise ValueError("Invalid {} identifier".format(name))
+ if os.sep in value or (os.altsep and os.altsep in value):
+ raise ValueError("Invalid {} identifier".format(name))
+ return value
@@
if isinstance(target, tuple) and len(target) == 2 and target[0] == "tenant":
- return [self.build_path(path, tenant_specific=True, tenant=target[1])]
+ tenant_id = self._validate_scope_component(target[1], "tenant")
+ return [self.build_path(path, tenant_specific=True, tenant=tenant_id)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/filesystem.py` around lines 412 - 413, The code passes
target[1] directly into self.build_path as tenant; validate and normalize that
tenant ID before calling build_path: ensure it's non-empty and does not contain
path separators ('/', '\\'), traversal tokens ('..'), or other unsafe characters
(e.g. require a whitelist like /^[A-Za-z0-9._-]+$/), and either raise a
ValueError on invalid input or sanitize/encode the value and then call
self.build_path(path, tenant_specific=True,
tenant=<validated_or_sanitized_value>); refer to the tuple check for target and
the self.build_path(..., tenant=target[1]) call to locate where to add this
validation.
| if target == "personal": | ||
| tenant_id = self._current_tenant_id() | ||
| cred_id = self._current_credentials_id() | ||
| personal_path = self._personal_path(path, tenant_id, cred_id) | ||
| return [personal_path] if personal_path is not None else [] | ||
|
|
There was a problem hiding this comment.
target="personal" currently subscribes only one context scope
Line 416–419 resolves only current tenant+credentials, but API semantics describe "personal" as all personal scopes. This misses updates outside current context.
Suggested direction
- def _iter_subscription_paths(self, path, target: typing.Union[str, tuple, None]):
+ async def _iter_subscription_paths(self, path, target: typing.Union[str, tuple, None]):
@@
- if target == "personal":
- tenant_id = self._current_tenant_id()
- cred_id = self._current_credentials_id()
- personal_path = self._personal_path(path, tenant_id, cred_id)
- return [personal_path] if personal_path is not None else []
+ if target == "personal":
+ paths = []
+ for tenant_id, cred_id in await self._get_personal_scopes():
+ personal_path = self._personal_path(path, tenant_id, cred_id)
+ if personal_path is not None:
+ paths.append(personal_path)
+ return pathsAnd in subscribe():
- for actual_path in self._iter_subscription_paths(path, target):
+ for actual_path in await self._iter_subscription_paths(path, target):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/filesystem.py` around lines 415 - 420, The current
branch for target == "personal" only resolves the single current
tenant+credentials via _current_tenant_id() and _current_credentials_id() and
returns one _personal_path, but "personal" should subscribe all personal scopes;
update subscribe() so that when target == "personal" it enumerates all tenant
IDs and credential IDs (e.g., via existing helpers or by adding helpers like
_all_tenant_ids()/_all_credentials_ids()), call _personal_path(path, tenant_id,
cred_id) for each pair, collect non-None results into the returned list, and
remove the single-current-context shortcut so all personal scopes are included.
| @@ -1,299 +1,226 @@ | |||
| # Library Service | |||
| Library Service | |||
There was a problem hiding this comment.
Section headings appear downgraded to plain paragraphs.
These lines look like intended section headers but lack Markdown heading markers, which can break ToC generation and in-page navigation.
Also applies to: 8-8, 38-38, 52-52, 104-104, 139-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/services/library.md` at line 1, Several section titles are
written as plain paragraphs (e.g., "Library Service") instead of Markdown
headings, which breaks TOC and navigation; update each plain-title line to use
the appropriate Markdown heading level (prepend one or more '#' as needed, for
example change "Library Service" to "# Library Service" or "## Library Service"
depending on its hierarchy) and ensure other similar title lines in the document
are converted the same way so the headings are recognized for TOC/in-page
anchors.
| ``` | ||
| + /MyDirectory/ | ||
| - /MyDirectory/item1.yaml | ||
| - /MyDirectory/item2.json | ||
| + /AnotherDirectory/ | ||
| - /AnotherDirectory/item3.yaml | ||
| + /AnotherDirectory/Folder/ | ||
| - /folder2/Folder/item4.json | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
Line 13 starts a fence without a language, which fails common markdown linting and hurts syntax highlighting.
Proposed fix
-```
+```text
+ /MyDirectory/
- /MyDirectory/item1.yaml
- /MyDirectory/item2.json
+ /AnotherDirectory/
- /AnotherDirectory/item3.yaml
+ /AnotherDirectory/Folder/
- /folder2/Folder/item4.json</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/reference/services/library.md around lines 13 - 21, The fenced code
block that begins with triple backticks and contains the directory listing (the
block showing /MyDirectory/, /AnotherDirectory/, etc.) lacks a language
identifier; update the opening fence to include a language (e.g., add "text" so
it becomes ```text) so markdown linters and syntax highlighters treat it
correctly and preserve formatting.
</details>
<!-- fingerprinting:phantom:triton:hawk:90b21a14-e14c-48d6-bb21-22c7736a34b4 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| !!! tip | ||
|
|
||
| In the most applications, it is common to create the first layer with *Zookeeper* provider and the layers beneath with *git* or *libsreg* provider. This allows you to quickly modify items of the Library on the first layer. | ||
|
|
||
| ## Disabling files | ||
|
|
||
| In the most applications, it is common to create the first layer with *Zookeeper* provider and the layers beneath with *git* or *libsreg* provider. This allows you to quickly modify items of the Library on the first layer. |
There was a problem hiding this comment.
Admonition blocks are malformed (body text should be indented under !!!).
Several admonitions are written as standalone markers with non-indented body text, so the content may render as normal paragraphs instead of admonitions.
Proposed pattern
-!!! tip
-
-In the most applications, it is common to create the first layer ...
+!!! tip
+ In most applications, it is common to create the first layer ...Also applies to: 50-50, 136-139, 264-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/services/library.md` around lines 35 - 37, The admonition
blocks starting with the marker "!!! tip" (and other "!!!" admonitions) are
malformed because their body text is not indented beneath the marker; fix each
by indenting the body lines under the corresponding "!!! <kind>" marker (e.g.,
indent the paragraph after "!!! tip" by at least one level—commonly 4 spaces or
a tab) so the content is recognized and rendered as the admonition; apply the
same change to the other occurrences of "!!!" admonitions in the file.
| Message Published when... | ||
| Library.not_ready! at least one provider is not ready. | ||
| Library.ready! all of the providers are ready. | ||
| Library.change! the content of the Library has changed. |
There was a problem hiding this comment.
Restore Markdown table syntax for message/provider matrices.
These lines are now plain tab-separated text, so they won’t render as tables in MkDocs.
Proposed fix
-Message Published when...
-Library.not_ready! at least one provider is not ready.
-Library.ready! all of the providers are ready.
-Library.change! the content of the Library has changed.
+| Message | Published when... |
+|---|---|
+| `Library.not_ready!` | At least one provider is not ready. |
+| `Library.ready!` | All providers are ready. |
+| `Library.change!` | The content of the Library has changed. |
-Provider Read the content Notify on changes
-Filesystem :material-check: :material-check:
-Apache Zookeeper :material-check: :material-check:
-Microsoft Azure Storage :material-check: :material-close:
-Git :material-check: :material-check:
-Libraries repository :material-check: :material-close:
+| Provider | Read the content | Notify on changes |
+|---|---:|---:|
+| Filesystem | :material-check: | :material-check: |
+| Apache Zookeeper | :material-check: | :material-check: |
+| Microsoft Azure Storage | :material-check: | :material-close: |
+| Git | :material-check: | :material-check: |
+| Libraries repository | :material-check: | :material-close: |Also applies to: 142-147
🧰 Tools
🪛 LanguageTool
[style] ~113-~113: Consider removing “of” to be more concise
Context: ...e provider is not ready. Library.ready! all of the providers are ready. Library.change! th...
(ALL_OF_THE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/services/library.md` around lines 111 - 114, The
message/provider matrix was rendered as plain tab-separated text; restore proper
Markdown table syntax by converting the block that lists "Message", "Published
when" and the rows containing "Library.not_ready!", "Library.ready!", and
"Library.change!" into a Markdown table (include the header row and the
separator line with dashes and pipes), and do the same for the other occurrence
covering lines referenced (the block at the second instance with the same
entries). Locate the blocks by the unique message names (Library.not_ready!,
Library.ready!, Library.change!) and replace the tab-separated lines with
well-formed Markdown table rows so MkDocs will render them correctly.
| ```python | ||
| class MyApplication(asab.Application): | ||
|
|
||
| async def initialize(self): | ||
| self.PubSub.subscribe("Library.ready!", self.on_library_ready | ||
| self.PubSub.subscribe("Library.change!", self.on_library_change) | ||
| async def initialize(self): | ||
| self.PubSub.subscribe("Library.ready!", self.on_library_ready | ||
| self.PubSub.subscribe("Library.change!", self.on_library_change) | ||
|
|
||
| async def on_library_ready(self, event_name, library=None): | ||
| await self.LibraryService.subscribe(["/asab"]) #(1)! | ||
| async def on_library_ready(self, event_name, library=None): | ||
| await self.LibraryService.subscribe(["/asab"]) #(1)! | ||
|
|
||
| def on_library_change(self, message, provider, path): #(2)! | ||
| print("New changes in the library found by provider: '{}'".format(provider)) | ||
| ``` | ||
|
|
||
| 1. `self.LibraryService.subscribe()` method takes either a single path as a string or multiple paths in list and watches for changes in them. | ||
| 2. This coroutine takes three arguments: `message` (`Library.change!` in this case), `provider` (name of the provider that has detected changes) and `path` (the path where changes were made). | ||
| def on_library_change(self, message, provider, path): #(2)! | ||
| print("New changes in the library found by provider: '{}'".format(provider)) | ||
| ``` |
There was a problem hiding this comment.
Fix the subscription example (currently invalid Python and inconsistent callback contract).
Line 124 is missing a closing ), and Lines 123/127/130 break class-method indentation, so the example won’t run. Also, Line 135 says the handler is a coroutine, but on_library_change is declared as def.
Proposed fix
```python
class MyApplication(asab.Application):
-async def initialize(self):
- self.PubSub.subscribe("Library.ready!", self.on_library_ready
- self.PubSub.subscribe("Library.change!", self.on_library_change)
+ async def initialize(self):
+ self.PubSub.subscribe("Library.ready!", self.on_library_ready)
+ self.PubSub.subscribe("Library.change!", self.on_library_change)
-async def on_library_ready(self, event_name, library=None):
- await self.LibraryService.subscribe(["/asab"]) #(1)!
+ async def on_library_ready(self, event_name, library=None):
+ await self.LibraryService.subscribe(["/asab"]) #(1)!
-def on_library_change(self, message, provider, path): #(2)!
- print("New changes in the library found by provider: '{}'".format(provider))
+ async def on_library_change(self, message, provider, path): #(2)!
+ print("New changes in the library found by provider: '{}'".format(provider))</details>
Also applies to: 135-135
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/reference/services/library.md around lines 120 - 132, Fix the example
by correcting indentation and the missing closing parenthesis in the
subscription calls inside class MyApplication: make initialize an async method
with properly indented body and call self.PubSub.subscribe("Library.ready!",
self.on_library_ready) and self.PubSub.subscribe("Library.change!",
self.on_library_change) (add the missing “)”). Also ensure the callback
contracts are consistent by making on_library_change async (async def
on_library_change(self, message, provider, path)) to match on_library_ready and
the expected coroutine handler for LibraryService.subscribe and PubSub events;
keep await self.LibraryService.subscribe(["/asab"]) inside on_library_ready as
shown.
</details>
<!-- fingerprinting:phantom:triton:hawk:90b21a14-e14c-48d6-bb21-22c7736a34b4 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ## Reference | ||
|
|
||
| This provider is designed to use Microsoft Azure Storage as a distribution point. | ||
| Is is assumed that the content archives are uploaded to the distribution point using CI/CD. |
There was a problem hiding this comment.
Typo in sentence start.
Line 350 says “Is is assumed…”.
Proposed fix
-Is is assumed that the content archives are uploaded to the distribution point using CI/CD.
+It is assumed that the content archives are uploaded to the distribution point using CI/CD.📝 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.
| Is is assumed that the content archives are uploaded to the distribution point using CI/CD. | |
| It is assumed that the content archives are uploaded to the distribution point using CI/CD. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/services/library.md` at line 350, Fix the typo at the start of
the sentence "Is is assumed that the content archives are uploaded to the
distribution point using CI/CD." by changing "Is is" to "It is" (update the
sentence in docs/reference/services/library.md so it reads "It is assumed that
the content archives are uploaded to the distribution point using CI/CD.").
1 participant
Summary by CodeRabbit
Documentation
Bug Fixes / Behavior
Refactor / Reliability