Make +CARGO output copy atomic #61
Conversation
| @@ -60,10 +63,9 @@ CARGO: | |||
| set -e; \ | |||
| cargo $args; \ | |||
| cargo sweep -r -t $EARTHLY_SWEEP_DAYS; \ | |||
| cargo sweep -r -i; | |||
| IF [ "$output" != "" ] | |||
| DO +COPY_OUTPUT --output=$output | |||
There was a problem hiding this comment.
Running this outside of the previous RUN command was the culprit
| ENV EARTHLY_FUNCTIONS_HOME="/tmp/earthly/functions" | ||
| RUN mkdir -p $EARTHLY_FUNCTIONS_HOME | ||
| RUN if [ ! -f $EARTHLY_FUNCTIONS_HOME/copy-output.sh ]; then \ | ||
| OUTPUT_TMP_FOLDER="/tmp/earthly/lib/rust"; \ |
There was a problem hiding this comment.
Thanks Nacho for quick fix. I think there is a race here:
Let's say there are two jobs A and B running parallel producing output.
A completes first by holding the cargo cache mount point and it copies the output to /tmp/earthly/lib/rust
And then B starts even before A run or completes rename-output.sh at line 69.
Now, B also starts copying its output to /tmp/earthly/lib/rust since the tmp folder is not in lock IIUC
But at the end of A running script rename-output.sh, it will delete the tmp folder /tmp/earthly/lib/rust along with B's artifacts.
IOW, using same temp folder /tmp/earthly/lib/rust without lock for multiple jobs, feels like a race to me.
I think we might use mktemp -d etc to create unique temp dir for each job outputs
Please let me know if i miss something
There was a problem hiding this comment.
I think you are missing this part: /tmp/earthly/lib is not shared between builds (it's in the layers fs).
The only shared paths (belonging to a shared cache) are:
/tmp/earthly/.cargo./target
There was a problem hiding this comment.
so, rename-output.sh just changes the location of the files, in the layers filesystem. I will add some comments to make this clearer
There was a problem hiding this comment.
Yeah that makes sense. Thanks
Concurrent builds of the same target may use the output of the other, since the cache lock is released between the cargo build and the output copy.
This PR fixes that