Fix: status check reported ready before models finished loading (#4) - #12
Open
carochacs wants to merge 3 commits into
Open
Fix: status check reported ready before models finished loading (#4)#12carochacs wants to merge 3 commits into
carochacs wants to merge 3 commits into
Conversation
ls_status() treated any HTTP 200 from /health as "alignment server ready", but the demucs-server responds 200 with status:"ok" as soon as the process is up -- well before whisperx (needed for /align's VAD-anchored windows) finishes downloading/loading. During that window /health's own `warmup` block already reports e.g. `whisperx: "downloading"`. This showed a green "Alignment server ready" notice while a request would still hit the server mid-warmup, then fail with a confusing "Cannot connect to alignment server" the moment the user actually clicked Align -- despite the ready notice being visible seconds earlier. ls_status() now inspects /health's `warmup.whisperx` field and only reports available:true once it's "ready", surfacing an accurate "still loading" reason otherwise. Per-language aligners load lazily on demand and are not gated on here. Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe routes now restrict configured Demucs URLs to approved local or private HTTP(S) destinations. The status endpoint requires HTTP 200 and ChangesDemucs validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LyricsSync as Lyrics sync route
participant Demucs as Demucs server
participant WhisperX as WhisperX warmup
LyricsSync->>Demucs: Request /health without redirects
Demucs-->>LyricsSync: Return HTTP 200 and health JSON
LyricsSync->>WhisperX: Read warmup.whisperx
WhisperX-->>LyricsSync: Return readiness state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (1)
routes.py (1)
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
/healthreadiness contract.
README.md:22-26documents the/alignendpoint and the configured server URL, but not the required HTTP 200 response withwarmup.whisperx == "ready". Add this requirement and the loading behavior so operators can diagnose an unavailable status. Confirm that supported Demucs server versions expose this field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routes.py` around lines 99 - 113, Update the README section documenting the alignment server URL and /align endpoint to require HTTP 200 with warmup.whisperx equal to "ready" before the server is considered available. Document that the server may return 200 while WhisperX is still downloading/loading, and note the supported Demucs server versions that expose this health field so operators can diagnose unavailable status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@routes.py`:
- Around line 125-130: Update the unavailable-status reason in the whisperx
readiness check around whisperx_state so unknown and terminal states use the
neutral “Alignment model is not ready ({state})” wording, while preserving the
existing loading-specific message only for transient warmup states.
- Around line 114-119: Validate or restrict the URL returned by
_get_demucs_server_url before the outbound requests in the lyrics sync status
and align handlers, allowing only approved destinations and schemes; ensure
requests.get and requests.post do not follow redirects. Apply the same
protection to both endpoints while preserving the existing unavailable response
for missing or rejected configuration.
---
Nitpick comments:
In `@routes.py`:
- Around line 99-113: Update the README section documenting the alignment server
URL and /align endpoint to require HTTP 200 with warmup.whisperx equal to
"ready" before the server is considered available. Document that the server may
return 200 while WhisperX is still downloading/loading, and note the supported
Demucs server versions that expose this health field so operators can diagnose
unavailable status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Restrict configured Demucs server URLs to local/private HTTP(S) targets, rejecting credentials and non-local hosts. Health and align requests now disable redirects, and availability messaging better distinguishes known WhisperX warmup states from unknown readiness failures. README requirements were updated to clarify that the plugin only treats the server as available when `/health` is 200 and `warmup.whisperx` is exactly `"ready"`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@routes.py`:
- Around line 28-29: Update the hostname validation logic around the hostname
allowlist to remove the unconditional host.docker.internal exception, or resolve
it and require the resolved address to pass the existing local/private-network
validation before allowing the request; preserve localhost handling without
permitting requests to public addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Removes 'host.docker.internal' from the list of approved hostnames for demucs URLs, limiting approval to 'localhost' only.
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ls_status() treated any HTTP 200 from /health as "alignment server ready", but the demucs-server responds 200 with status:"ok" as soon as the process is up -- well before whisperx (needed for /align's VAD-anchored windows) finishes downloading/loading. During that window /health's own
warmupblock already reports e.g.whisperx: "downloading".This showed a green "Alignment server ready" notice while a request would still hit the server mid-warmup, then fail with a confusing "Cannot connect to alignment server" the moment the user actually clicked Align -- despite the ready notice being visible seconds earlier.
ls_status() now inspects /health's
warmup.whisperxfield and only reports available:true once it's "ready", surfacing an accurate "still loading" reason otherwise. Per-language aligners load lazily on demand and are not gated on here.Summary by CodeRabbit
Bug Fixes
Documentation