fix: remove the staged script when a command fails - #481
Merged
Conversation
Container#execute stages the command as a script under .kitchen/temp, uploads
it, and deletes the local copy. The delete sat between the upload and the exec,
so any failure before it left the script behind:
after a successful exec : 0 file(s) left
after a failing exec : 1 file(s) left
Confirmed by removing the container out from under a kitchen exec, which is
what a container that has exited looks like to the driver. One file is left per
failed attempt, and nothing ever clears them, so .kitchen/temp fills up over a
run of failing converges.
Deleting in an ensure instead, in both the Linux and Windows containers, which
share the shape of this method.
Not addressed here: the uploaded copy inside the container is never removed
either, so a converge leaves one script per command in the container's temp
directory. Those go when the container does, and removing them would add a
docker exec round trip per command, so it is left alone deliberately.
Verified after the change: nothing is left behind after either a successful or
a failing exec.
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
Container#executestages the command as a script under.kitchen/temp, uploads it to the container, and deletes the local copy. The delete sat between the upload and the exec:Any failure before that line leaves the script behind.
Confirmed
Removing the container out from under a
kitchen exec— which is what a container that has exited looks like to the driver:One file per failed attempt, and nothing ever clears them, so
.kitchen/tempfills up over a run of failing converges. Minor, but it is a leak the code plainly intends not to have.The fix
Delete in an
ensure, in both the Linux and Windows containers — they share the shape of this method and both had it.Deliberately not addressed
The uploaded copy inside the container is never removed either — three execs leave three scripts in the container's temp directory:
Those go when the container does, and removing them would add a
docker execround trip per command for no real benefit. Flagging it rather than changing it; happy to if you'd rather.Verified after the change
Specs
New
#executegroup inspec/linux_container_spec.rb: the staged script is removed after a successful run, after a failed upload, and after a failed command — plus one pinning that the command really is what gets written to the script.🤖 Generated with Claude Code