fix: validate materialized immutable_inputs paths to survive macOS te… - #23458
fix: validate materialized immutable_inputs paths to survive macOS te…#23458thiago-carbonera wants to merge 2 commits into
Conversation
7ae8cba to
495f847
Compare
|
@benjyw Could you review this? |
benjyw
left a comment
benjyw
left a comment
There was a problem hiding this comment.
Thanks for the fix. Just a small comment about use style in the test.
|
|
||
| #[tokio::test] | ||
| async fn recovers_from_deleted_directory_issue_23411() { | ||
| use crate::{ImmutableInputs, Store}; |
There was a problem hiding this comment.
Our style is to have use statements at the file level, other than in exceptional circumstances.
| ) -> Result<PathBuf, StoreError> { | ||
| let digest = directory_digest.as_digest(); | ||
| let cell = self.0.contents.lock().entry(digest).or_default().clone(); | ||
| // we must verify the memoized path still exists on disk. |
There was a problem hiding this comment.
There is still a race condition here: the OS might reap the tmpdir after it's read here and before it's used.
This is still a good change, as it also defends against a human deleting the dir, but to be truly robust against the macos shenanigans we would also want to not use $TMPDIR in the first place. We should probably write these under the pants cache dir.
But I think it's fine to do that in a followup.
There was a problem hiding this comment.
Thanks for the review, @benjyw!
I completely agree with the race condition. Moving these away from $TMPDIR and into the Pants cache dir seems like the most robust long-term solution against macOS shenanigans.
I've just pushed a commit moving the use statements to the file level as requested. Let me know if everything looks good to go now!
There was a problem hiding this comment.
I'm still not sure about this, the race condition is more complicated than I first thought. For example, what if the parent dir is reaped? Then the reinitialization will always fail, no? I think TempDir::new_in() requires the parent dir to exist.
The issue is really that the default value of BootstrapOptions.local_execution_root_dir is a tempdir. So maybe that is all we need to fix, and put it under pants_workdir instead? That will be a simpler and more comprehensive fix.
There was a problem hiding this comment.
Hey @benjyw, the CI caught an interesting side-effect of this architectural change.
The test test_mypyc_build failed. Because the sandboxes are now nested inside the workspace (.pants.d/workdir/local_execution/...), tools that recursively search parent directories for config files (like mypy) are breaking out of the sandbox.
Looking at the logs, mypy traversed up 9 directories (../../../../../../../../../pyproject.toml), hit the Pants repo's root pyproject.toml, and crashed trying to load mypy_typing_asserts.
Moving the execution root inside the workspace seems to break sandbox hermeticity for hierarchical config resolution. Should we reconsider the $TMPDIR cache invalidation in Rust, or is there another safe default outside the workspace we could use?
There was a problem hiding this comment.
Oh, right, local_execution_root_dir is used for sandboxes as well as immutable_inputs. And we do generally want old sandboxes to be reaped I suppose.
There was a problem hiding this comment.
OK, so probably the original solution here is a decent best-effort. It will fail if the parent tempdir is reaped, but for that to happen there would have had to be no new writes at all under it in three days, which seems unlikely in normal use since all the sandboxes go there.
OK, so let's go back to your original plan.
There was a problem hiding this comment.
I've just reverted the Python changes and restored the original Rust validation approach (checking path.exists() before using the memoized OnceCell). Everything is back to the Rust implementation! Let me know if it looks good now.
601a579 to
968ac95
Compare
|
Thanks for the contribution. We've just branched for 2.33.x, so merging this pull request now will come out in 2.34.x, please move the release notes updates to docs/notes/2.34.x.md if that's appropriate. |
5097ef1 to
e2890cd
Compare
|
Done @cburroughs. |
|
@thiago-carbonera please add an LLM disclosure, if relevant: https://www.pantsbuild.org/stable/docs/contributions#llm-assistance-notice |
|
There is still a merge conflict |
a3ab1ae to
4a7e191
Compare
4a7e191 to
7e04e5a
Compare
3 participants
Fixes #23411
Problem
On macOS, a long-running
pantsdinstance can outlive the OS periodic temp cleaner (which reaps unaccessed files in$TMPDIRafter ~3 days).ImmutableInputs::path_for_dirwas memoizing paths in memory and returning them without verifying if the underlying directory still existed on disk, causing tools like Docker to fail withexecutable file not found in $PATHbecause their$PATHpointed to a deleted directory.Solution
path_for_dirto checkpath.exists()on the cachedPathBuf.OnceCelland forces the directory to be re-materialized.local_tests.rsto simulate the exact OS reaper behavior.