fix(smartmirror): use GET instead of HEAD for mirror connectivity testing - #480
fix(smartmirror): use GET instead of HEAD for mirror connectivity testing#480qiuzhiqian wants to merge 1 commit into
Conversation
testing PMS: BUG-369443
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates all smart-mirror connectivity tests from HTTP HEAD to GET, covering Release routing, parallel mirror selection, and final URL validation to support mirrors that do not handle HEAD correctly. Sequence diagram for smart-mirror GET connectivity testingsequenceDiagram
participant Client
participant SmartMirror
participant Mirror
Client->>SmartMirror: route(original, officialMirror)
SmartMirror->>Mirror: GET original Release URL
Mirror-->>SmartMirror: URL and status
SmartMirror-->>Client: routed URL
SmartMirror->>SmartMirror: makeChoice(original, officialMirror)
par Test mirror candidates
SmartMirror->>Mirror: GET candidate mirror URL
Mirror-->>SmartMirror: URL and status
and Test mirror candidates
SmartMirror->>Mirror: GET candidate mirror URL
Mirror-->>SmartMirror: URL and status
end
SmartMirror->>Mirror: GET original URL for final validation
Mirror-->>SmartMirror: validation response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
CLA Assistant Lite bot: |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qiuzhiqian The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/lastore-smartmirror-daemon/smartmirror.go" line_range="157-158" />
<code_context>
return s.makeChoice(original, officialMirror)
} else if strings.Contains(original, "/dists/") && strings.HasSuffix(original, "Release") {
// Get Release from Release
- url, _ := handleRequest(buildRequest(makeHeader(), "HEAD", original))
+ url, _ := handleRequest(buildRequest(makeHeader(), "GET", original))
return url
} else if strings.Contains(original, "/dists/") && strings.Contains(original, "/by-hash/") {
return s.makeChoice(original, officialMirror)
</code_context>
<issue_to_address>
**issue (performance):** The connectivity checks now issue unrestricted GET requests for Release and package URLs. `handleRequest` closes each response body without consuming it, so every probe still starts a real payload transfer and causes unnecessary mirror bandwidth and server load; `makeChoice` additionally performs another GET against the original package URL when reporting results.
**Triggers:** When routing pool files or Release files, especially when the requested artifact is large.
**Suggested fix:** Use a GET request with an appropriate small `Range` header, or otherwise limit the response body while retaining the GET method required by affected mirrors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| url, _ := handleRequest(buildRequest(makeHeader(), "HEAD", original)) | ||
| url, _ := handleRequest(buildRequest(makeHeader(), "GET", original)) | ||
| return url |
There was a problem hiding this comment.
issue (performance): The connectivity checks now issue unrestricted GET requests for Release and package URLs. handleRequest closes each response body without consuming it, so every probe still starts a real payload transfer and causes unnecessary mirror bandwidth and server load; makeChoice additionally performs another GET against the original package URL when reporting results.
Triggers: When routing pool files or Release files, especially when the requested artifact is large.
Suggested fix: Use a GET request with an appropriate small Range header, or otherwise limit the response body while retaining the GET method required by affected mirrors.
|
TAG Bot New tag: 6.2.68 |
PMS: BUG-369443
Summary by Sourcery
Bug Fixes: