Skip to content

Commit

Permalink
Pidlock source and artifact installation. Registry pidlock fixes. (#4168
Browse files Browse the repository at this point in the history 
)

* protect against an interrupted mv

* pidlock download_source

* add tests

* try a proper package

* actually test ffmpeg works

* windows fix

* pidlock artifact downloads

* don't claim to have done work

* more tests

* debug hang

* debug: try a yield

* increase stale_age for downloads

* rm debugs

* use (and rename) mv_temp_artifact_dir

* disable modifying permissions in mv_temp_artifact_dir

* rm goto's

* rename as generic and move to utils

* reduce to 1 test. lock ffmpeg version.

* show info if test fails

* debug: turn on verbose testsets for timing

* tweaks

* fix error printing

* check which registries need installing after acquiring pidlock

* rm ispath check outside of pidlock

* add time print

* color process prints

* use early throw `Base.Experimental.@sync`

* narrow try

* reduce stale_age

* fix!

* remove print from happy path

* also pidlock git installs
  • Loading branch information
@IanButterworth
IanButterworth authored Mar 1, 2025
1 parent 2779d42 commit cf6191b
Show file tree
Hide file tree
Showing 6 changed files with 390 additions and 253 deletions.
213 changes: 90 additions & 123 deletions src/Artifacts.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ module Artifacts
using Artifacts, Base.BinaryPlatforms, SHA
using ..MiniProgressBars, ..PlatformEngines
using Tar: can_symlink
using FileWatching: FileWatching

import ..set_readonly, ..GitTools, ..TOML, ..pkg_server, ..can_fancyprint,
..stderr_f, ..printpkgstyle
..stderr_f, ..printpkgstyle, ..mv_temp_dir_retries

import Base: get, SHA1
import Artifacts: artifact_names, ARTIFACTS_DIR_OVERRIDE, ARTIFACT_OVERRIDES, artifact_paths,
Expand Down Expand Up @@ -49,7 +50,7 @@ function create_artifact(f::Function)
# as something that was foolishly overridden. This should be virtually impossible
# unless the user has been very unwise, but let's be cautious.
new_path = artifact_path(artifact_hash; honor_overrides=false)
_mv_temp_artifact_dir(temp_dir, new_path)
mv_temp_dir_retries(temp_dir, new_path)

# Give the people what they want
return artifact_hash
Expand All @@ -59,55 +60,6 @@ function create_artifact(f::Function)
end
end

"""
_mv_temp_artifact_dir(temp_dir::String, new_path::String)::Nothing
Either rename the directory at `temp_dir` to `new_path` and set it to read-only
or if `new_path` artifact already exists try to do nothing.
"""
function _mv_temp_artifact_dir(temp_dir::String, new_path::String)::Nothing
# Sometimes a rename can fail because the temp_dir is locked by
# anti-virus software scanning the new files.
# In this case we want to sleep and try again.
# I am using the list of error codes to retry from:
# https://github.com/isaacs/node-graceful-fs/blob/234379906b7d2f4c9cfeb412d2516f42b0fb4953/polyfills.js#L87
# Retry for up to about 60 seconds by retrying 20 times with exponential backoff.
retry = 0
max_num_retries = 20 # maybe this should be configurable?
sleep_amount = 0.01 # seconds
max_sleep_amount = 5.0 # seconds
while true
isdir(new_path) && return
# This next step is like
# `mv(temp_dir, new_path)`.
# However, `mv` defaults to `cp` if `rename` returns an error.
# `cp` is not atomic, so avoid the potential of calling it.
err = ccall(:jl_fs_rename, Int32, (Cstring, Cstring), temp_dir, new_path)
if err ≥ 0
# rename worked
new_path_mode = filemode(dirname(new_path))
if Sys.iswindows()
# If this is Windows, ensure the directory mode is executable,
# as `filemode()` is incomplete. Some day, that may not be the
# case, there exists a test that will fail if this is changes.
new_path_mode |= 0o111
end
chmod(new_path, new_path_mode)
set_readonly(new_path)
return
else
# Ignore rename error if `new_path` exists.
isdir(new_path) && return
if retry < max_num_retries && err ∈ (Base.UV_EACCES, Base.UV_EPERM, Base.UV_EBUSY)
sleep(sleep_amount)
sleep_amount = min(sleep_amount*2.0, max_sleep_amount)
retry += 1
else
Base.uv_error("rename of $(repr(temp_dir)) to $(repr(new_path))", err)
end
end
end
end

"""
remove_artifact(hash::SHA1; honor_overrides::Bool=false)
Expand Down Expand Up @@ -330,87 +282,102 @@ function download_artifact(
io::IO=stderr_f(),
progress::Union{Function, Nothing} = nothing,
)
if artifact_exists(tree_hash)
return true
_artifact_paths = Artifacts.artifact_paths(tree_hash)
pidfile = _artifact_paths[1] * ".pid"
mkpath(dirname(pidfile))
t_wait_msg = Timer(2) do t
if progress === nothing
@info "downloading $tarball_url ($hex) in another process"
else
progress(0, 0; status="downloading in another process")
end
end
ret = FileWatching.mkpidlock(pidfile, stale_age = 20) do
close(t_wait_msg)
if artifact_exists(tree_hash)
return true
end

# Ensure the `artifacts` directory exists in our default depot
artifacts_dir = first(artifacts_dirs())
mkpath(artifacts_dir)
# expected artifact path
dst = joinpath(artifacts_dir, bytes2hex(tree_hash.bytes))
# Ensure the `artifacts` directory exists in our default depot
artifacts_dir = first(artifacts_dirs())
mkpath(artifacts_dir)
# expected artifact path
dst = joinpath(artifacts_dir, bytes2hex(tree_hash.bytes))

# We download by using a temporary directory. We do this because the download may
# be corrupted or even malicious; we don't want to clobber someone else's artifact
# by trusting the tree hash that has been given to us; we will instead download it
# to a temporary directory, calculate the true tree hash, then move it to the proper
# location only after knowing what it is, and if something goes wrong in the process,
# everything should be cleaned up.
# We download by using a temporary directory. We do this because the download may
# be corrupted or even malicious; we don't want to clobber someone else's artifact
# by trusting the tree hash that has been given to us; we will instead download it
# to a temporary directory, calculate the true tree hash, then move it to the proper
# location only after knowing what it is, and if something goes wrong in the process,
# everything should be cleaned up.

# Temporary directory where we'll do our creation business
temp_dir = mktempdir(artifacts_dir)
# Temporary directory where we'll do our creation business
temp_dir = mktempdir(artifacts_dir)

try
download_verify_unpack(tarball_url, tarball_hash, temp_dir;
ignore_existence=true, verbose, quiet_download, io, progress)
isnothing(progress) || progress(10000, 10000; status="verifying")
calc_hash = SHA1(GitTools.tree_hash(temp_dir))

# Did we get what we expected? If not, freak out.
if calc_hash.bytes != tree_hash.bytes
msg = """
Tree Hash Mismatch!
Expected git-tree-sha1: $(bytes2hex(tree_hash.bytes))
Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))
"""
# Since tree hash calculation is rather fragile and file system dependent,
# we allow setting JULIA_PKG_IGNORE_HASHES=1 to ignore the error and move
# the artifact to the expected location and return true
ignore_hash_env_set = get(ENV, "JULIA_PKG_IGNORE_HASHES", "") != ""
if ignore_hash_env_set
ignore_hash = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
ignore_hash === nothing && @error(
"Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
ENV["JULIA_PKG_IGNORE_HASHES"],
)
ignore_hash = something(ignore_hash, false)
else
# default: false except Windows users who can't symlink
ignore_hash = Sys.iswindows() &&
!mktempdir(can_symlink, artifacts_dir)
try
download_verify_unpack(tarball_url, tarball_hash, temp_dir;
ignore_existence=true, verbose, quiet_download, io, progress)
isnothing(progress) || progress(10000, 10000; status="verifying")
calc_hash = SHA1(GitTools.tree_hash(temp_dir))

# Did we get what we expected? If not, freak out.
if calc_hash.bytes != tree_hash.bytes
msg = """
Tree Hash Mismatch!
Expected git-tree-sha1: $(bytes2hex(tree_hash.bytes))
Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))
"""
# Since tree hash calculation is rather fragile and file system dependent,
# we allow setting JULIA_PKG_IGNORE_HASHES=1 to ignore the error and move
# the artifact to the expected location and return true
ignore_hash_env_set = get(ENV, "JULIA_PKG_IGNORE_HASHES", "") != ""
if ignore_hash_env_set
ignore_hash = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
ignore_hash === nothing && @error(
"Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
ENV["JULIA_PKG_IGNORE_HASHES"],
)
ignore_hash = something(ignore_hash, false)
else
# default: false except Windows users who can't symlink
ignore_hash = Sys.iswindows() &&
!mktempdir(can_symlink, artifacts_dir)
end
if ignore_hash
desc = ignore_hash_env_set ?
"Environment variable \$JULIA_PKG_IGNORE_HASHES is true" :
"System is Windows and user cannot create symlinks"
msg *= "\n$desc: \
ignoring hash mismatch and moving \
artifact to the expected location"
@error(msg)
else
error(msg)
end
end
if ignore_hash
desc = ignore_hash_env_set ?
"Environment variable \$JULIA_PKG_IGNORE_HASHES is true" :
"System is Windows and user cannot create symlinks"
msg *= "\n$desc: \
ignoring hash mismatch and moving \
artifact to the expected location"
@error(msg)
else
error(msg)
# Move it to the location we expected
isnothing(progress) || progress(10000, 10000; status="moving to artifact store")
mv_temp_dir_retries(temp_dir, dst)
catch err
@debug "download_artifact error" tree_hash tarball_url tarball_hash err
if isa(err, InterruptException)
rethrow(err)
end
# If something went wrong during download, return the error
return err
finally
# Always attempt to cleanup
try
rm(temp_dir; recursive=true, force=true)
catch e
e isa InterruptException && rethrow()
@warn("Failed to clean up temporary directory $(repr(temp_dir))", exception=e)
end
end
# Move it to the location we expected
isnothing(progress) || progress(10000, 10000; status="moving to artifact store")
_mv_temp_artifact_dir(temp_dir, dst)
catch err
@debug "download_artifact error" tree_hash tarball_url tarball_hash err
if isa(err, InterruptException)
rethrow(err)
end
# If something went wrong during download, return the error
return err
finally
# Always attempt to cleanup
try
rm(temp_dir; recursive=true, force=true)
catch e
e isa InterruptException && rethrow()
@warn("Failed to clean up temporary directory $(repr(temp_dir))", exception=e)
end
return true
end
return true

return ret
end

"""
Expand Down
Loading

0 comments on commit cf6191b

Please sign in to comment.