fix: report why a temp file could not be written - #480
Merged
Conversation
create_temp_file assigned the opened file back over its own file parameter and
closed it in an ensure:
file = ::File.open(file, "w")
file.write(contents)
rescue IOError => e
raise "Failed to write temp file. Error Details: #{e}"
ensure
file.close unless file.nil?
When opening failed, file was still the path String, so the ensure called
String#close and raised NoMethodError -- discarding the real error. The rescue
could not help: it caught IOError, while opening a file fails with Errno
classes, which are SystemCallError and not IOError.
Every failure produced the same useless message. Confirmed against a
read-only directory, a parent that is not a directory, and a target that is
itself a directory:
parent is a file -> NoMethodError: undefined method 'close' for an instance of String
directory read-only -> NoMethodError: undefined method 'close' for an instance of String
target is a directory -> NoMethodError: undefined method 'close' for an instance of String
Container::Linux#execute and Container::Windows#execute both write their
command through this, so it reached users as a Docker failure. With .kitchen
not writable, which is what a read-only checkout gives you, kitchen exec said:
Docker failed to execute command on container. Error Details: Failed to
execute command on Linux container. undefined method 'close' for an
instance of String
Written with File.write now, which opens, writes and closes in one call, so
there is no handle to leak and no reassignment to trip over. SystemCallError is
rescued alongside IOError, and the message names the path. The same run now
reports:
Failed to write temp file ./.kitchen/temp/docker-7fa49201-....sh.
Error Details: Permission denied @ dir_s_mkdir - ./.kitchen/temp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
The bug
create_temp_fileassigned the opened file back over its ownfileparameter, then closed it in anensure:Two things go wrong together:
fileis still the path String, so theensurecallsString#closeand raisesNoMethodError— which replaces the real error.rescuecatchesIOError, but opening a file fails withErrno::*, which areSystemCallError— notIOError. So the intended message never appeared either.Confirmed
Every failure path produced the same useless error:
Container::Linux#executeandContainer::Windows#executeboth write their command through this, so it reaches users as a Docker failure. With.kitchennot writable — which is what a read-only checkout, or a CI job with a read-only mount, gives you — a realkitchen execsaid:Nothing there points at a filesystem permission.
The fix
File.write, which opens, writes and closes in one call — no handle to leak, no reassignment to trip over, noensureneeded.SystemCallErroris rescued alongsideIOError, and the message names the path.The same run now reports:
And the normal path is unchanged —
kitchen exec … -c "echo hello"still printshello.Specs
New
spec/file_helper_spec.rb: writes contents, creates the parent directory (./.kitchen/tempdoes not exist on a fresh checkout), truncates an existing file, leaves no open handle, and — for each failure mode — reports the real cause, names the path, and never raisesNoMethodError.One note on the parent-is-a-file case: the errno differs by platform (macOS reports
EEXISTfrommkdir, LinuxENOTDIR), so that example asserts the underlying cause reaches the user rather than pinning an exact string. That is the thing that was lost.🤖 Generated with Claude Code