fix: stream archives instead of holding them in memory, and close a leaked socket - #27
Merged
Merged
Conversation
…eaked socket Tar.pack_directory built the whole build context in a StringIO before sending a byte. A context is whatever the caller points at -- a Rails app with its assets, a monorepo subtree, a directory holding a model checkpoint -- so the archive's full size was charged to the heap on top of what the daemon was about to receive. It is written to a temporary file now, and streamed. Container#archive_in did `archive.respond_to?(:read) ? archive.read : archive`, which defeated the point of accepting an IO: a container filesystem is exactly the kind of archive nobody wants resident. The IO is handed over as it stands. Both of those depend on a third fix. attach_payload chose between body_stream and body with `when IO, StringIO`, which looks exhaustive and is not -- Tempfile is a delegator around File, so Tempfile.new.is_a?(IO) is false. A packed context would have fallen through to `body = payload` and gone out as the delegator's to_s: a string like "#<Tempfile:...>" where the daemon expected a tar. What Net::HTTP needs from a body stream is `read`, so that is what is asked for now. Separately, Transport::Tls leaked the TCP socket when the handshake failed. sync_close only ties the two together once an SSLSocket exists and owns it, so an expired certificate, a hostname mismatch or an untrusted CA left the descriptor open until GC -- and a retry loop waiting for a daemon to come up exhausts descriptors rather than failing cleanly. tempfile joins the allowlist in spec/zero_dependency_spec.rb. It is a default gem, not a bundled one, so it is safe for a gem that declares no runtime dependencies -- verified by loading it with no gems on the path. Signed-off-by: Tim Smith <tim@mondoo.com>
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.
1. The build context was assembled entirely in memory
Tar.pack_directorywrote the whole archive into aStringIObefore sending a byte. A build context is whatever the caller points at — a Rails app with its assets, a monorepo subtree, a directory holding a model checkpoint — so its full size was charged to the heap on top of what the daemon was about to receive.It now packs to a
Tempfileand streams. The connection layer already sends a readable body chunked, so the archive never needs to be resident.pack_dockerfilestill usesStringIO— a short generated Dockerfile has no reason to touch disk.2.
archive_inslurped the IO it was givenThat defeats the point of accepting an IO — a container filesystem is exactly the kind of archive nobody wants in memory. It is handed over as it stands now.
3. The fix both of those depend on
attach_payloadchose betweenbody_streamandbodywith a class check:which looks exhaustive and is not.
Tempfileis a delegator aroundFile, soTempfile.new.is_a?(IO)isfalse. A packed context would have fallen through tobody = payloadand gone out as the delegator'sto_s— a string like#<Tempfile:0x...>where the daemon expected a tar.What Net::HTTP actually needs from a body stream is
read, so that is what is asked for. There is a test asserting the request contains real archive bytes and the wordTempfileappears nowhere in it.4. A failed TLS handshake leaked the socket
sync_closeonly ties the SSLSocket to the socket underneath once the SSLSocket exists and owns it. An expired certificate, a hostname mismatch or an untrusted CA raised before that, anddialconverted it to aConnectionErrorwith nothing closing the descriptor — so a retry loop waiting for a daemon to come up exhausts file descriptors rather than failing cleanly.The test stands up a TCP server that never speaks TLS and asserts the socket the transport opened is closed after the failure.
Note on the allowlist
spec/zero_dependency_spec.rb(added in #19) caughtrequire "tempfile"immediately, which is the guard doing its job.tempfileis a default gem, not a bundled one — confirmed by loading it withGEM_HOME/GEM_PATHempty — so it is safe for a gem that declares no runtime dependencies, and it is allowlisted explicitly rather than by loosening the rule.Verification
bundle exec rake— 333 runs, 1128 assertions, 0 failuressteep checkclean, chefstyle cleanDOCKER_API_NG_INTEGRATION=1 rake integration— 19 runs, 49 assertions, 0 failures against a real daemon, which is the run that matters here: it puts a genuinely streamed tar through/buildand/containers/{id}/archive.spec/archive_streaming_spec.rb— 2 failures and 2 errors onmain.