fix: cannot download differentially because of space in range #9276
Conversation
🦋 Changeset detectedLatest commit: e35c4aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
beyondkmp
changed the title
|
@mmaietta differentialUpdateTest keeps failing, and it seems the test server no longer supports this configuration. |
What do you mean by this? If the current test server doesn't accept the new differential range format, doesn't that signify this change is not backward compatible? |
|
I tried to understand how differentialUpdateTest.ts works, but I have no idea how it works and where exactly it fails and why it is not compatible |
|
I haven't found where this update server is yet. I haven't found it. |
|
@mmaietta I've found the reason for the UT failure; it's because we used the 'ran' server（https://github.com/m3ng9i/ran/tree/master/server）, which doesn't support ranges without spaces. It seems the server didn't handle this case properly. According to RFC 7233, it should support both cases: with and without spaces. |
|
Do we need to update |
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #9270 related to differential downloads failing due to spaces in HTTP Range headers. The change removes the trailing space after commas when constructing multi-part range request headers.
Key Changes:
- Removed trailing space from range string construction (changing
", "to","in the loop that builds the ranges string)
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/electron-updater/src/differentialDownloader/multipleRangeDownloader.ts | Modified range header construction to remove trailing space after comma |
| .changeset/hungry-eagles-explain.md | Added changeset documenting the fix |
Critical Issue Found: The fix is incomplete and introduces a new bug. While line 49 correctly removes the trailing space, line 93 (not shown in the diff but visible in the surrounding code) still uses ranges.substring(0, ranges.length - 2) to trim the final comma and space. With this change, only the comma remains at the end, so the substring should use ranges.length - 1 instead. This will cause the last digit of the final range's end value to be truncated, resulting in malformed HTTP Range headers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const task = options.tasks[i] | ||
| if (task.kind === OperationKind.DOWNLOAD) { | ||
| ranges += `${task.start}-${task.end - 1}, ` | ||
| ranges += `${task.start}-${task.end - 1},` |
There was a problem hiding this comment.
The fix is incomplete. While the trailing space has been removed from line 49, line 93 still uses ranges.substring(0, ranges.length - 2) which assumes both a comma and a space need to be removed. With this change, only the comma remains, so line 93 should use ranges.length - 1 instead.
Currently, this will result in truncating the last digit of the final range's end value, leading to incorrect HTTP Range headers being sent to the server. For example, if the last range is "100-199", it would be sent as "bytes=100-19" instead of "bytes=100-199".
There was a problem hiding this comment.
This is a valid point and should be fixed, otherwise it will break
3 participants
fix #9270