Skip to content

Library zk reconnect noise#744

Open
mithunbharadwaj wants to merge 2 commits intomasterfrom
fix/library-zk-reconnect-noise
Open

Library zk reconnect noise#744
mithunbharadwaj wants to merge 2 commits intomasterfrom
fix/library-zk-reconnect-noise

Conversation

@mithunbharadwaj
Copy link
Copy Markdown
Collaborator

@mithunbharadwaj mithunbharadwaj commented Apr 9, 2026

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced error handling to gracefully manage connection loss during library provider operations
    • Added initialization checks to prevent operations from executing when services aren't ready
    • Improved exception handling and resource cleanup for file operations
  • Refactor

    • Refined control flow and resource management across library service operations

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

📝 Walkthrough

Walkthrough

Two library provider modules are enhanced with resilience guards. The ZooKeeper provider adds early-return checks and connection-loss exception handling to version tracking and library change notification methods. The service module adds readiness checks, early returns, and explicit resource cleanup to favorite and disabled file loading.

Changes

Cohort / File(s) Summary
ZooKeeper Connection Resilience
asab/library/providers/zookeeper.py
Added early-return guards when ZooKeeper client is uninitialized or not ready in _get_version_counter and _on_library_changed. Wraps ZooKeeper operations with kazoo.exceptions.ConnectionClosedError exception handling to gracefully handle connection loss during version checks and subscription updates across global, tenant, and personal targets.
Service Readiness & File Handling
asab/library/service.py
Added early-return guards in tick and file-loading methods when libraries are empty or not ready. Refined exception handling for favorites and disabled file reads: read failures now warn only when first provider is ready, parse failures reset state to empty. Introduced explicit file handle cleanup via close() and separated read/parse error paths into distinct try/except blocks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • New 'personal' targets #712 — Modifies ZooKeeper provider's _on_library_changed method to handle additional "personal" target types, directly overlapping with this PR's connection resilience enhancements in the same method.
  • Read favourites #709 — Introduces favorites loading functionality in asab/library/service.py that this PR subsequently hardens with readiness checks, exception handling, and resource cleanup.

Poem

🐰 Hops through the warren with guards held high,
When connections falter, we gracefully sigh,
Readiness checked, resources cleaned with care,
Resilient paths blooming everywhere! 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Library zk reconnect noise' directly describes the main changes: handling ZooKeeper reconnection scenarios more gracefully by catching connection errors and returning early to reduce noise/logging.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/library-zk-reconnect-noise

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
asab/library/service.py (1)

538-574: Consider logging the suppressed close() exception.

The control flow correctly separates read vs parse failures: read failures preserve existing state (appropriate for transient errors), while parse failures reset to empty (appropriate for malformed config).

However, the try/except/pass at lines 570-573 silently swallows any close exception. While this is defensive cleanup code, logging at DEBUG level would aid troubleshooting.

♻️ Optional: Log suppressed close exception
 		finally:
 			if hasattr(disabled_file, "close"):
 				try:
 					disabled_file.close()
 				except Exception:
-					pass
+					L.debug("Failed to close disabled_file handle")

The same pattern applies to _read_favorites at lines 471-474.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@asab/library/service.py` around lines 538 - 574, The finally block that
closes disabled_file currently swallows exceptions silently; update it to log
any exception at DEBUG (or similar) so suppressed close errors are visible
during troubleshooting—capture the exception when calling disabled_file.close()
and call the module logger (e.g. L.debug or L.exception with a brief message)
instead of a bare pass; apply the same change to the analogous cleanup in
_read_favorites so both close failures are logged while preserving existing
control flow and behavior for Disabled and DisabledPaths.
🤖 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/service.py`:
- Around line 538-574: The finally block that closes disabled_file currently
swallows exceptions silently; update it to log any exception at DEBUG (or
similar) so suppressed close errors are visible during troubleshooting—capture
the exception when calling disabled_file.close() and call the module logger
(e.g. L.debug or L.exception with a brief message) instead of a bare pass; apply
the same change to the analogous cleanup in _read_favorites so both close
failures are logged while preserving existing control flow and behavior for
Disabled and DisabledPaths.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6b818412-8126-43fd-8bfb-04f47a1823e3

📥 Commits

Reviewing files that changed from the base of the PR and between 4e1e467 and 02322f2.

📒 Files selected for processing (2)
  • asab/library/providers/zookeeper.py
  • asab/library/service.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant