Prevent collisions between attempts to materialize an immutable input digest. #15652
Conversation
… digest. [ci skip-build-wheels]
There was a problem hiding this comment.
😭🥳
@jyggen thank you so much for commenting that hint about remote caching
| @@ -94,58 +82,21 @@ impl ImmutableInputs { | |||
| digest, e | |||
| ) | |||
| })?; | |||
|
|
|||
| let dest = chroot.path().join(digest.hash.to_hex()); | |||
There was a problem hiding this comment.
I'm totally lost. How is this reproducible digest suffix any more anonymous than the tmpdir its being joined with?
There was a problem hiding this comment.
The creation of the tempdir is guaranteed not to collide (via OS facilities, presumably), so each materialization attempt is in a fresh temporary directory. The digest suffix is a convenience for readability: it's the temp dir that is avoiding collisions.
There was a problem hiding this comment.
On the LHS:
self.store..materialize_directory(
chroot.path().to_path_buf(),
directory_digest,
Permissions::ReadOnly,
)On the RHS - paraphrasing with an inline:
self.store..materialize_directory(
chroot.path().join(digest.hash.to_hex()),
directory_digest,
Permissions::ReadOnly,
)So the only change is to use the tempdir as-is instead of renaming it to a fixed self.workdir.path().join(digest.hash.to_hex()) dest - Got it.
Why was the self.workdir rooting important before and not now? Or was it never?
There was a problem hiding this comment.
Ok, so self.workdir is already a TempDir. So we create a TempDir in a TempDir, as we always did, but no longer rename from <tmpdir>/<tmpdir> -> <tmpdir>/<digest> we now just directly create <tmpdir>/<tmpdir>/<digest>. It would seem the outer could just be a normal dir now, but that's only mildly confusing as is.
… digest. (pantsbuild#15652) Fixes pantsbuild#13899. [ci skip-build-wheels]
… digest. (pantsbuild#15652) Fixes pantsbuild#13899. [ci skip-build-wheels]
Fixes #13899.
[ci skip-build-wheels]