⚡ Bolt: Short-circuit Qobuz valid secret check#64
⚡ Bolt: Short-circuit Qobuz valid secret check#64davidjuarezdev wants to merge 1 commit intomainfrom
Conversation
Replaces asyncio.gather with asyncio.as_completed in streamrip/client/qobuz.py to short-circuit upon finding the first valid secret, improving startup login latency. Co-authored-by: davidjuarezdev <[email protected]>
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors Qobuz app secret validation to use asyncio.as_completed with explicit task management, returning as soon as the first valid secret is found and canceling the remaining tests to reduce latency and unnecessary network I/O. Sequence diagram for Qobuz secret validation with asyncio.as_completedsequenceDiagram
actor User
participant QobuzClient
participant asyncio
participant QobuzAPI
User->>QobuzClient: login()
QobuzClient->>QobuzClient: _get_valid_secret(secrets)
QobuzClient->>QobuzClient: create_task(_test_secret(secret_i)) for each secret
QobuzClient->>asyncio: as_completed(tasks)
loop for each future in as_completed
asyncio-->>QobuzClient: next completed future
QobuzClient->>QobuzClient: await future
QobuzClient->>QobuzAPI: _test_secret(secret)
QobuzAPI-->>QobuzClient: valid or invalid
alt secret is valid
QobuzClient-->>User: return first_valid_secret
QobuzClient->>QobuzClient: cancel remaining pending tasks and break loop
else secret is invalid
QobuzClient->>QobuzClient: continue loop
end
end
alt no secret is valid
QobuzClient-->>User: raise InvalidAppSecretError
QobuzClient->>QobuzClient: cancel remaining pending tasks
end
Class diagram for updated QobuzClient secret validation methodsclassDiagram
class QobuzClient {
+_get_valid_secret(secrets list~str~) str
+_test_secret(secret str) str
}
class InvalidAppSecretError {
+InvalidAppSecretError(secrets list~str~)
}
class asyncio {
+create_task(coro) Task
+as_completed(tasks list~Task~) AsyncIterator
}
class Task {
+cancel() void
+done() bool
}
QobuzClient ..> InvalidAppSecretError : raises
QobuzClient ..> asyncio : uses
asyncio --> Task : creates
QobuzClient ..> Task : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@streamrip/client/qobuz.py`:
- Around line 422-426: The finally block in _get_valid_secret() currently calls
task.cancel() on items in tasks but doesn't await them, so cancelled
_test_secret() coroutines may keep running against the shared aiohttp session;
update the cleanup to collect the pending tasks, call cancel() on each, then
await their completion (e.g., via asyncio.gather(pending_tasks,
return_exceptions=True)) to drain/unwind the coroutines before returning,
referencing the tasks list, _test_secret(), and _get_valid_secret() to locate
where to add the await.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c33b6938-6508-43cf-b300-1a9fd7ae34bf
📒 Files selected for processing (1)
streamrip/client/qobuz.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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
🔇 Additional comments (1)
streamrip/client/qobuz.py (1)
417-420: Confirm whether the secret priority should intentionally be ignored.The code explicitly reorders secrets to prioritize the second seed/timezone pair (via
move_to_end), butasyncio.as_completed()returns the first valid probe to finish, not the first in the configured order. When multiple secrets can pass_test_secret(), selection becomes timing-dependent rather than respecting the configured priority.If this change is intentional, document the rationale. If not, preserve the original order by retaining
asyncio.gather()or by checking results in the original order.
| finally: | ||
| # Cancel remaining tasks to prevent background task leakage | ||
| for task in tasks: | ||
| if not task.done(): | ||
| task.cancel() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'asyncio.as_completed|task.cancel|_test_secret' streamrip/client/qobuz.py
python - <<'PY'
import asyncio
async def worker():
try:
await asyncio.sleep(10)
finally:
await asyncio.sleep(0.05)
async def main():
tasks = [asyncio.create_task(worker()) for _ in range(2)]
await asyncio.sleep(0)
for task in tasks:
task.cancel()
print("after cancel:", [task.done() for task in tasks])
await asyncio.sleep(0)
print("next loop turn:", [task.done() for task in tasks])
await asyncio.gather(*tasks, return_exceptions=True)
print("after gather:", [task.done() for task in tasks])
asyncio.run(main())
PYRepository: davidjuarezdev/streamrip_RipDL
Length of output: 1344
Drain cancelled tasks before returning.
task.cancel() only requests cancellation. Without awaiting the pending tasks, the cancelled _test_secret() coroutines continue unwinding against the shared aiohttp session after _get_valid_secret() returns, causing background task leakage.
Collect pending tasks, cancel them, and await their completion:
Suggested fix
finally:
- # Cancel remaining tasks to prevent background task leakage
- for task in tasks:
- if not task.done():
- task.cancel()
+ pending = [task for task in tasks if not task.done()]
+ for task in pending:
+ task.cancel()
+ if pending:
+ await asyncio.gather(*pending, return_exceptions=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@streamrip/client/qobuz.py` around lines 422 - 426, The finally block in
_get_valid_secret() currently calls task.cancel() on items in tasks but doesn't
await them, so cancelled _test_secret() coroutines may keep running against the
shared aiohttp session; update the cleanup to collect the pending tasks, call
cancel() on each, then await their completion (e.g., via
asyncio.gather(pending_tasks, return_exceptions=True)) to drain/unwind the
coroutines before returning, referencing the tasks list, _test_secret(), and
_get_valid_secret() to locate where to add the await.
![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 1 file
Confidence score: 3/5
- There is a concrete async cleanup issue in
streamrip/client/qobuz.py: cancelled tasks aren’t awaited, which can leave aiohttp connections open and emit 'Task was destroyed but it is pending!' warnings. - This introduces some runtime risk around resource cleanup, so the change isn’t a no-brainer despite being localized.
- Pay close attention to
streamrip/client/qobuz.py- ensure cancelled tasks are awaited to close connections cleanly.
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:424">
P2: Cancelled tasks must be awaited to avoid 'Task was destroyed but it is pending!' warnings and to ensure aiohttp connections are properly closed. Add `await asyncio.gather(*tasks, return_exceptions=True)` after cancelling.</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 must be awaited to avoid 'Task was destroyed but it is pending!' warnings and to ensure aiohttp connections are properly closed. Add await asyncio.gather(*tasks, return_exceptions=True) after cancelling.
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 424:
<comment>Cancelled tasks must be awaited to avoid 'Task was destroyed but it is pending!' warnings and to ensure aiohttp connections are properly closed. Add `await asyncio.gather(*tasks, return_exceptions=True)` after cancelling.</comment>
<file context>
@@ -409,14 +409,21 @@ async def _test_secret(self, secret: str) -> Optional[str]:
- return working_secrets[0]
+ finally:
+ # Cancel remaining tasks to prevent background task leakage
+ 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: Replace asyncio.gather with asyncio.as_completed for Qobuz secret validation.
🎯 Why: To immediately return the first valid secret, avoiding slow-tail latency and unnecessary network requests.
📊 Impact: Significantly speeds up Qobuz login and saves network bandwidth by canceling remaining requests.
🔬 Measurement: The tests still pass and the login duration is visibly shorter when the first secret tested happens to be valid.
PR created automatically by Jules for task 1032822583719394639 started by @davidjuarezdev
Summary by Sourcery
Enhancements:
Summary by cubic
Short-circuit Qobuz secret validation to return the first valid secret and cancel the rest, reducing login latency and network I/O.
Written for commit abfb3f9. Summary will update on new commits.