⚡ Bolt: Fast return on Qobuz secret checking via asyncio.as_completed#65
⚡ Bolt: Fast return on Qobuz secret checking via asyncio.as_completed#65davidjuarezdev wants to merge 1 commit intomainfrom
Conversation
Use `asyncio.as_completed` instead of `asyncio.gather` when testing qobuz app secrets. This short-circuits the requests as soon as the first valid secret is found and cancels the remaining tests, avoiding slow tail latency from waiting for all secrets to be tested. Co-authored-by: davidjuarezdev <230496599+davidjuarezdev@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThese changes optimize the Qobuz client's secret validation mechanism by replacing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReworks Qobuz app-secret probing to short‑circuit on the first valid secret using asyncio.as_completed with explicit task cancellation, and documents the pattern in the project’s Bolt learning log. Class diagram for QobuzClient secret validation changesclassDiagram
class QobuzClient {
+async _test_secret(secret str) Optional~str~
+async _get_valid_secret(secrets list_str) str
}
class InvalidAppSecretError {
+InvalidAppSecretError(secrets list_str)
}
QobuzClient ..> InvalidAppSecretError : raises
class asyncio {
+create_task(coro) Task
+as_completed(tasks Iterable_Task) Iterable_Future
}
QobuzClient ..> asyncio : uses
class Task {
+done() bool
+cancel() None
}
class Future {
+result() Any
+__await__() Any
}
asyncio ..> Task : creates
asyncio ..> Future : yields as completed
Flow diagram for short-circuiting Qobuz secret validationflowchart TD
A[Start _get_valid_secret with list of secrets] --> B[Create asyncio tasks for each _test_secret]
B --> C[Iterate over tasks using asyncio.as_completed]
C --> D[Await next completed future]
D --> E{Result is not None?}
E -->|Yes| F[Return first valid secret]
F --> G[finally: cancel all unfinished tasks]
G --> H[End]
E -->|No| I{More futures?}
I -->|Yes| C
I -->|No| J[Raise InvalidAppSecretError]
J --> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
streamrip/client/qobuz.py (1)
411-423: Good optimization usingasyncio.as_completedfor early return.The implementation correctly achieves the goal of returning as soon as the first valid secret is found. Two observations:
Task cancellation cleanup: After calling
task.cancel(), the tasks are not awaited. While not strictly required, awaiting cancelled tasks ensures clean resource cleanup and avoids potential "task was destroyed but pending" warnings.Exception propagation: If
_test_secretraises an unexpected exception (e.g., network error from aiohttp), it propagates immediately without trying remaining secrets. This may be acceptable if such errors indicate a broader connectivity issue.♻️ Optional: Await cancelled tasks for cleaner shutdown
async def _get_valid_secret(self, secrets: list[str]) -> str: tasks = [asyncio.create_task(self._test_secret(secret)) for secret in secrets] try: for future in asyncio.as_completed(tasks): result = await future if result is not None: return result finally: for task in tasks: if not task.done(): task.cancel() + # Await cancellation to ensure clean resource cleanup + await asyncio.gather(*tasks, return_exceptions=True) raise InvalidAppSecretError(secrets)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@streamrip/client/qobuz.py` around lines 411 - 423, In _get_valid_secret, ensure cancelled background tasks are awaited for clean shutdown and avoid warnings: after cancelling any task in the finally block, await each cancelled task and suppress asyncio.CancelledError (and ignore other non-fatal exceptions). Also make the as_completed loop resilient by catching and ignoring non-fatal exceptions raised by await future (from _test_secret) so a single network error doesn't abort trying other secrets; still raise InvalidAppSecretError(secrets) if no secret returns a valid result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@streamrip/client/qobuz.py`:
- Around line 411-423: In _get_valid_secret, ensure cancelled background tasks
are awaited for clean shutdown and avoid warnings: after cancelling any task in
the finally block, await each cancelled task and suppress asyncio.CancelledError
(and ignore other non-fatal exceptions). Also make the as_completed loop
resilient by catching and ignoring non-fatal exceptions raised by await future
(from _test_secret) so a single network error doesn't abort trying other
secrets; still raise InvalidAppSecretError(secrets) if no secret returns a valid
result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a387872-ad4a-4b3c-9e25-be6dcd4de07c
📒 Files selected for processing (2)
.jules/bolt.mdstreamrip/client/qobuz.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
streamrip/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
streamrip/**/*.py: Use Black-compatible code formatting with double quotes and spaces via ruff format
Lint Python code with ruff using rules: E4, E7, E9, F, I, ASYNC, N, RUF, ERA001
Use async/await for asynchronous operations instead of blocking I/O
Implement Windows compatibility by using WindowsSelectorEventLoopPolicy on Windows and the pick library instead of simple-term-menu
Files:
streamrip/client/qobuz.py
🧠 Learnings (2)
📚 Learning: 2026-03-18T03:25:38.238Z
Learnt from: CR
Repo: davidjuarezdev/streamrip_RipDL PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T03:25:38.238Z
Learning: Applies to streamrip/**/*.py : Use async/await for asynchronous operations instead of blocking I/O
Applied to files:
.jules/bolt.md
📚 Learning: 2026-03-18T03:25:38.238Z
Learnt from: CR
Repo: davidjuarezdev/streamrip_RipDL PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T03:25:38.238Z
Learning: Use abstract base classes for Client implementations, with each streaming service (Qobuz, Tidal, Deezer, SoundCloud) having its own concrete implementation handling login/auth, metadata fetching, search, and Downloadable object production
Applied to files:
.jules/bolt.md
🔇 Additional comments (1)
.jules/bolt.md (1)
8-11: LGTM!Clear and accurate documentation of the optimization. The learning correctly captures why
asyncio.gatherwas suboptimal (waiting for all requests including slow/failing ones) and the action accurately describes the solution usingasyncio.as_completedwith task cancellation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- There is a concrete async cleanup risk in
streamrip/client/qobuz.py: cancelled tasks are not awaited, which can leaveaiohttpresources open and trigger runtime warnings. - Given the severity (6/10) and high confidence (9/10), this is more than a cosmetic issue and could cause user-facing instability under cancellation paths, so merge risk is moderate.
- Pay close attention to
streamrip/client/qobuz.py- ensure cancelled tasks are awaited so coroutine cleanup runs and HTTP connections are properly released.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="streamrip/client/qobuz.py">
<violation number="1" location="streamrip/client/qobuz.py:419">
P2: Cancelled tasks are not awaited, which can leak HTTP connections and produce runtime warnings. After cancelling, await the tasks to ensure their coroutines run cleanup code (e.g., closing `aiohttp` response context managers).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| for task in tasks: | ||
| if not task.done(): | ||
| task.cancel() |
There was a problem hiding this comment.
P2: Cancelled tasks are not awaited, which can leak HTTP connections and produce runtime warnings. After cancelling, await the tasks to ensure their coroutines run cleanup code (e.g., closing aiohttp response context managers).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At streamrip/client/qobuz.py, line 419:
<comment>Cancelled tasks are not awaited, which can leak HTTP connections and produce runtime warnings. After cancelling, await the tasks to ensure their coroutines run cleanup code (e.g., closing `aiohttp` response context managers).</comment>
<file context>
@@ -409,14 +409,18 @@ async def _test_secret(self, secret: str) -> Optional[str]:
+ if result is not None:
+ return result
+ finally:
+ for task in tasks:
+ if not task.done():
+ task.cancel()
</file context>
| for task in tasks: | |
| if not task.done(): | |
| task.cancel() | |
| for task in tasks: | |
| if not task.done(): | |
| task.cancel() | |
| await asyncio.gather(*tasks, return_exceptions=True) |
1 participant
💡 What: Replaced
asyncio.gatherwithasyncio.as_completedin_get_valid_secretofQobuzClient. This allows returning immediately upon finding the first valid secret and cleanly cancels the pending tasks.🎯 Why: Qobuz secret verification requires testing multiple potential app secrets. The previous implementation waited for all secrets to be tested, which could result in slow execution times (especially if some tests time out or fail).
📊 Impact: Considerably faster login/initialization times by eliminating the wait time for trailing/failing request checks.
🔬 Measurement: Can be verified by running the tests to ensure login behaves the same, or dynamically running the
QobuzClient.login()and checking execution time compared tomaster.PR created automatically by Jules for task 7445357538402336570 started by @davidjuarezdev
Summary by Sourcery
Optimize Qobuz client secret validation to return on the first successful result and document the concurrency pattern used.
Enhancements:
Documentation:
Summary by cubic
Make Qobuz secret discovery return as soon as the first valid secret is found, cutting login/initialization latency and avoiding waits on slow or timed‑out checks.
asyncio.gatherwithasyncio.as_completedinQobuzClient._get_valid_secret, returning on the first valid result.finallyblock and keptInvalidAppSecretErrorwhen none succeed.Written for commit 34406fc. Summary will update on new commits.