Open compact index cache in binary mode when appending#9679
Open
hsbt wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Windows-specific corruption when incrementally appending to Compact Index cache files by ensuring the append path uses binary mode (matching the existing full-write path). This prevents LF→CRLF translation during append, which could previously bypass digest verification and lead to persistent cache invalidation/redownload loops on Windows.
Changes:
- Switch
CompactIndexClient::CacheFile#appendfrom"a"to binary append"ab"in both RubyGems and Bundler copies. - Add a RubyGems minitest covering binary-safe append behavior (ensuring no
"\r"reaches disk). - Add a Bundler RSpec covering the same binary append behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/rubygems/compact_index_client/cache_file.rb | Use binary append mode ("ab") so appended bytes are written verbatim on Windows. |
| lib/bundler/compact_index_client/cache_file.rb | Mirror the RubyGems fix in Bundler’s CacheFile implementation. |
| test/rubygems/test_gem_compact_index_client_cache_file.rb | Add regression test asserting append preserves LF-only line endings on disk. |
| spec/bundler/compact_index_client/cache_file_spec.rb | Add Bundler-side regression spec for binary append behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+5
| require "bundler/compact_index_client" | ||
| require "bundler/compact_index_client/cache_file" | ||
|
|
CacheFile#append opened the temp file with mode "a", which on Windows is a text-mode stream that rewrites every "\n" as "\r\n". The corruption goes undetected because DigestIO hashes the logical bytes handed to write, not the bytes the OS actually lands on disk, so verify passes and the mangled file is committed. The damage compounds on the next update: the extra carriage returns shift file.size, so the ranged request restarts at the wrong offset and the versions index no longer matches its expected digest, forcing a full re-download every time. The info files keep failing their MD5 check against the raw index bytes, so they are refetched on every resolve. Open the append stream with "ab" so the appended range is written verbatim, matching the binary "wb" used for full writes.
hsbt
force-pushed
the
compact-index-binary-append
branch
from
6808889 to
9568d96
Compare
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
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.
CompactIndexClient::CacheFile#appendopened the temporary cache file with mode"a". On Windows that is a text-mode stream, so every"\n"written into the appended range is silently rewritten as"\r\n". Full writes already use the binary"wb"mode, so only the incremental append path is affected.The corruption is invisible to the integrity check.
appendwrites throughGem::Package::DigestIO, which hashes the logical bytes handed towrite, not the bytes the operating system actually lands on disk. The digest therefore matches,verifyreturns true, and the mangled file is committed to the cache as if it were correct.The failure compounds on the next update. The stray carriage returns inflate
file.size, so the following ranged request (file.size - 1) restarts at the wrong offset and the reassembledversionsindex no longer matches the digest the server advertises, forcing a full multi-megabyte re-download. The per-geminfofiles keep failing their MD5 comparison against the raw index bytes, so they are refetched on every resolve. A full replace with"wb"heals the file momentarily, but the next incremental append corrupts it again, leaving Windows users in a permanent re-download loop.The fix opens the append stream with
"ab"so the appended range is written verbatim, matching the binary mode already used for full writes. The change is applied to both the Bundler and RubyGems copies ofCacheFile.Tests
test/rubygems/test_gem_compact_index_client_cache_file.rbgainstest_append_writes_in_binary_mode, which appends content containing an"\n"and asserts viabinreadthat no"\r"reaches disk. On the unfixed code this fails on Windows with"rake 13.0.0\r\n"; with the fix it passes. A matchingspec/bundler/compact_index_client/cache_file_spec.rbcovers the Bundler copy. The existing append specs used newline-free payloads, which is why the corruption went unnoticed.