Repository navigation
Conversation
github-actions
Bot
added
the
size:L
serabi
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| self._positions = fresh | ||
| self._last_fetch = time.time() | ||
| self._consecutive_failures = 0 |
There was a problem hiding this comment.
🟡 Medium sync_clients/libby_sync_client.py:104
Individual position-fetch outages are recorded as successful refreshes, so persistent failures never reach MAX_CONSECUTIVE_FAILURES and cannot trigger the intended auto-disable cooldown. When get_active_loans succeeds but every loan yields no position, fresh is empty and line 106 unconditionally resets _consecutive_failures; preserve/register the failure count for that case instead.
self._positions = fresh
self._last_fetch = time.time()
- self._consecutive_failures = 0
+ if loans and not fresh:
+ self._register_failure()
+ else:
+ self._consecutive_failures = 0🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/sync_clients/libby_sync_client.py around lines 104-106:
Individual position-fetch outages are recorded as successful refreshes, so persistent failures never reach `MAX_CONSECUTIVE_FAILURES` and cannot trigger the intended auto-disable cooldown. When `get_active_loans` succeeds but every loan yields no position, `fresh` is empty and line 106 unconditionally resets `_consecutive_failures`; preserve/register the failure count for that case instead.
| now = time.time() | ||
| if not self.is_configured(): | ||
| return None | ||
| if now < self._retry_after_until: |
There was a problem hiding this comment.
🟡 Medium sync_clients/libby_sync_client.py:80
The Retry-After cooldown is never honored: _retry_after_until remains 0.0, so the guard at this line is false for every normal request and a rate-limited get_active_loans() response does not prevent the next eligible fetch. Update _retry_after_until from the server's Retry-After value when handling that response (or remove the guard if the client cannot expose it).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/sync_clients/libby_sync_client.py around line 80:
The Retry-After cooldown is never honored: `_retry_after_until` remains `0.0`, so the guard at this line is false for every normal request and a rate-limited `get_active_loans()` response does not prevent the next eligible fetch. Update `_retry_after_until` from the server's `Retry-After` value when handling that response (or remove the guard if the client cannot expose it).
| """Fetch a single loan's position for manual per-book syncs. | ||
| Throttled to MIN_FETCH_GAP to stay rate-safe.""" | ||
| now = time.time() | ||
| if now < self._last_fetch + MIN_FETCH_GAP: |
There was a problem hiding this comment.
🟡 Medium sync_clients/libby_sync_client.py:222
A manual sync for an uncached book returns None without checking Libby when another book was fetched within the last 60 seconds, so a valid position is silently omitted. The global _last_fetch check runs before looking up the requested book and returns only self._positions.get(book.libby_psn_key); throttle only when this book already has a cached entry, or track the throttle per book.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/sync_clients/libby_sync_client.py around line 222:
A manual sync for an uncached book returns `None` without checking Libby when another book was fetched within the last 60 seconds, so a valid position is silently omitted. The global `_last_fetch` check runs before looking up the requested book and returns only `self._positions.get(book.libby_psn_key)`; throttle only when this book already has a cached entry, or track the throttle per book.
| state = self.get_sync_state() | ||
| if not state: | ||
| return [] | ||
| if state is None: |
There was a problem hiding this comment.
🟠 High api/libby_client.py:421
_targeted_position raises TypeError: 'NoneType' object is not iterable when /chip/sync fails, so a transient manual per-book sync crashes instead of reporting that the position is unavailable. Since get_active_loans() now returns None on fetch failure, guard that result before building the loan index (or otherwise normalize it to an empty iterable).
Also found in 1 other location(s)
src/sync_clients/libby_sync_client.py:225
_targeted_positioniterates directly overself.libby_client.get_active_loans(), but that method explicitly returnsNonewhen/chip/syncfails. A manual/per-book sync during a transient Libby failure therefore raisesTypeError: 'NoneType' object is not iterableinstead of degrading to unavailable state.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/api/libby_client.py around line 421:
`_targeted_position` raises `TypeError: 'NoneType' object is not iterable` when `/chip/sync` fails, so a transient manual per-book sync crashes instead of reporting that the position is unavailable. Since `get_active_loans()` now returns `None` on fetch failure, guard that result before building the loan index (or otherwise normalize it to an empty iterable).
Also found in 1 other location(s):
- src/sync_clients/libby_sync_client.py:225 -- `_targeted_position` iterates directly over `self.libby_client.get_active_loans()`, but that method explicitly returns `None` when `/chip/sync` fails. A manual/per-book sync during a transient Libby failure therefore raises `TypeError: 'NoneType' object is not iterable` instead of degrading to unavailable state.
| possession = self.libby_client.get_possession(passport["urls"]["possession"]) | ||
| return self._extract_position(possession) |
There was a problem hiding this comment.
🟠 High sync_clients/libby_sync_client.py:139
A partial passport containing urls without possession raises KeyError here, aborting the entire bulk refresh instead of skipping that unavailable loan. Validate the possession URL before calling get_possession.
| possession = self.libby_client.get_possession(passport["urls"]["possession"]) | |
| return self._extract_position(possession) | |
| possession_url = passport.get("urls", {}).get("possession") | |
| if not possession_url: | |
| return None, None | |
| possession = self.libby_client.get_possession(possession_url) | |
| return self._extract_position(possession) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/sync_clients/libby_sync_client.py around lines 139-140:
A partial passport containing `urls` without `possession` raises `KeyError` here, aborting the entire bulk refresh instead of skipping that unavailable loan. Validate the possession URL before calling `get_possession`.
| GrimmoryAudio=grimmory_audio_sync_client, | ||
| Grimmory2Audio=grimmory_audio_sync_client_2, | ||
| Hardcover=hardcover_sync_client, | ||
| Libby=libby_sync_client, |
There was a problem hiding this comment.
🟠 High utils/di_container.py:235
LibbySyncClient can be elected as the leader, but its get_text_from_current_state() always returns None; _execute_sync_update treats that as terminal and returns before updating any followers. Therefore a Libby position never propagates to ABS, KoSync, or the other clients. Handle Libby’s percentage-only state in _execute_sync_update (or prevent Libby from leading) so a valid Libby position still produces a locator and follower updates.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/utils/di_container.py around line 235:
`LibbySyncClient` can be elected as the leader, but its `get_text_from_current_state()` always returns `None`; `_execute_sync_update` treats that as terminal and returns before updating any followers. Therefore a Libby position never propagates to ABS, KoSync, or the other clients. Handle Libby’s percentage-only state in `_execute_sync_update` (or prevent Libby from leading) so a valid Libby position still produces a locator and follower updates.
| loans = {loan["psn_key"]: loan for loan in self.libby_client.get_active_loans()} | ||
| loan = loans.get(book.libby_psn_key) | ||
| if not loan: | ||
| return None | ||
| pct, reading_time = self._read_loan_position(loan) | ||
| self._last_fetch = now |
There was a problem hiding this comment.
🟡 Medium sync_clients/libby_sync_client.py:225
A stale libby_psn_key causes _targeted_position to return without updating _last_fetch, so each subsequent targeted sync refetches the full active-loan list instead of honoring MIN_FETCH_GAP; this can burst /chip/sync requests. Record the fetch time immediately after get_active_loans() returns, before checking whether the mapping exists.
- loans = {loan["psn_key"]: loan for loan in self.libby_client.get_active_loans()}
+ loans = {loan["psn_key"]: loan for loan in self.libby_client.get_active_loans()}
+ self._last_fetch = now
loan = loans.get(book.libby_psn_key)
if not loan:
return None
pct, reading_time = self._read_loan_position(loan)
- self._last_fetch = now🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/sync_clients/libby_sync_client.py around lines 225-230:
A stale `libby_psn_key` causes `_targeted_position` to return without updating `_last_fetch`, so each subsequent targeted sync refetches the full active-loan list instead of honoring `MIN_FETCH_GAP`; this can burst `/chip/sync` requests. Record the fetch time immediately after `get_active_loans()` returns, before checking whether the mapping exists.
| GrimmoryAudio=grimmory_audio_sync_client, | ||
| Grimmory2Audio=grimmory_audio_sync_client_2, | ||
| Hardcover=hardcover_sync_client, | ||
| Libby=libby_sync_client, |
There was a problem hiding this comment.
🟡 Medium utils/di_container.py:235
LibbySyncClient continues making requests after its auto-disable cooldown starts: when fetch_bulk_state() returns None, SyncManager still calls get_service_state() for each book, which falls through to _targeted_position() and calls get_active_loans() without checking is_configured() or auto_disabled. Add a disabled-state guard before targeted fetching so the cooldown actually prevents Libby requests.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/utils/di_container.py around line 235:
`LibbySyncClient` continues making requests after its auto-disable cooldown starts: when `fetch_bulk_state()` returns `None`, `SyncManager` still calls `get_service_state()` for each book, which falls through to `_targeted_position()` and calls `get_active_loans()` without checking `is_configured()` or `auto_disabled`. Add a disabled-state guard before targeted fetching so the cooldown actually prevents Libby requests.
| GrimmoryAudio=grimmory_audio_sync_client, | ||
| Grimmory2Audio=grimmory_audio_sync_client_2, | ||
| Hardcover=hardcover_sync_client, | ||
| Libby=libby_sync_client, |
There was a problem hiding this comment.
🟠 High utils/di_container.py:235
Libby is permanently omitted from SyncManager.sync_clients when the container is constructed before pairing, so storing LIBBY_*_TOKEN later does not enable Libby syncing until restart. _setup_sync_clients filters each client only once via is_configured(); refresh the client registry after pairing or make client selection evaluate configuration dynamically.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/utils/di_container.py around line 235:
`Libby` is permanently omitted from `SyncManager.sync_clients` when the container is constructed before pairing, so storing `LIBBY_*_TOKEN` later does not enable Libby syncing until restart. `_setup_sync_clients` filters each client only once via `is_configured()`; refresh the client registry after pairing or make client selection evaluate configuration dynamically.
| GrimmoryAudio=grimmory_audio_sync_client, | ||
| Grimmory2Audio=grimmory_audio_sync_client_2, | ||
| Hardcover=hardcover_sync_client, | ||
| Libby=libby_sync_client, |
There was a problem hiding this comment.
🟠 High utils/di_container.py:235
Libby is never used for audio-only books without a kosync_doc_id, so their libby_psn_key position is not read or synchronized. SyncManager._sync_single_book filters audio-only clients to a hard-coded set that excludes Libby; include Libby in that path so its declared audiobook support takes effect.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/utils/di_container.py around line 235:
`Libby` is never used for audio-only books without a `kosync_doc_id`, so their `libby_psn_key` position is not read or synchronized. `SyncManager._sync_single_book` filters audio-only clients to a hard-coded set that excludes `Libby`; include `Libby` in that path so its declared audiobook support takes effect.
Part 2/3 of the Libby integration (#131). Builds on #132.
What
LibbySyncClient: leader-capable, supports both sync types, percentage-only locator (percentageOfBook)expires − leeway; bulk refreshes throttled toLIBBY_POLL_MINS(60 default, 10 floor) with a 60s anti-hammer floorRetry-After; 5 consecutive failed cycles → auto-disable cooldown (degrades to offline, not noise)books.libby_psn_keycolumn + migration; registered in DIsync_clientsas LibbyTests
Bulk fetch/dual-store/throttle/disable matrix + service-state contract.
Note
Add read-only
LibbySyncClientwith throttled bulk position fetchinglibby_psn_keycolumn to theBookmodel via migration to link books to Libby loans; registers the new client in the DI container under the'Libby'keyLibbyClient.get_legacy_positionto fetch cross-device position data from the legacy store, returningNoneon failureGrimmoryClient.download_book_fileto refuse downloads when the selection record is missing or falsey, not only on ID mismatchLibbyClient.get_active_loansnow returnsNoneinstead of an empty list when/chip/synccannot be fetched; callers that do not handleNonemay break. The newlibby_psn_keycolumn is added via a conditional migration but any external readers of thebookstable will see a new column.📊 Macroscope summarized e53223b. 19 files reviewed, 15 issues evaluated, 1 issue filtered, 12 comments posted
🗂️ Filtered Issues
src/sync_clients/libby_sync_client.py — 6 comments posted, 8 evaluated, 1 filtered
_targeted_positioniterates directly overself.libby_client.get_active_loans(), but that method explicitly returnsNonewhen/chip/syncfails. A manual/per-book sync during a transient Libby failure therefore raisesTypeError: 'NoneType' object is not iterableinstead of degrading to unavailable state. [ Cross-file consolidated ]