Verify packed PEX cache entries before reuse. - #3262
Conversation
`_build_packedapp` admitted an existing bootstrap-zip or packed-wheel cache entry on `atomic_directory(...).is_finalized()` alone, which is just `os.path.exists` of the target dir, and then copied the cached zip into the PEX being built without inspecting it. An entry finalized while incomplete was therefore reused verbatim by every later build sharing the `PEX_ROOT`, turning a single bad write into a permanent failure: the resulting PEX dies at `PEX_TOOLS=1 ... venv --scope=deps` with `MetadataError: Failed to determine project name and version` for a missing `.deps/` wheel, or `ModuleNotFoundError: No module named 'pex.version'` for a short `.bootstrap`. Because the entry is stable, retries reproduce it exactly. `cache_zip` records the digest of the zip inside the `atomic_directory` work dir, so it lands through the same atomic rename as the zip it describes, and checks it before reuse. A mismatched entry is discarded and re-created once. An entry written before this existed carries no digest and is treated as unverified, so caches poisoned prior to this change heal on first contact. This addresses reuse of a bad entry, not its creation.
|
@apetti1920 please provide more context on your Pex use. Is it direct or indirect, say via Pants? The current scheme is very deliberate. Pex uses atomic_directory to ensure bad cache dirs can never be written in the 1st place and relies on this for speed of reads. Absent bugs, a bad cache dir can only be created by another entity deleting cache dir contents or feeding Pex bad inputs for the cache dir. Pex considers the former outside the bounds of what it guards against and purposefully trusts its own cache. The latter is a possibility with, for example, For example of (likely) external bad, see: pantsbuild/pants#23657 Here Pex is being fed corrupt uv venvs as the input source of pre-installed wheels. |
|
@apetti1920 your profile links klaviyo via Linked In; so I assume this is Pants. If that's true, are you using the uv resolver and what Pants version is this? |
|
We're on pants 2.33.0 Checking on UV |
|
It would be this option in pants.toml: https://www.pantsbuild.org/dev/reference/subsystems/python#resolver: [python]
resolver = "uv" |
|
Ok yep we're not using that. |
@jsirois Via Pants 2.33.0, PEX version: 2.97.3, pinned by us (Pants 2.33's own floor is 2.97.1). The UV resolver is unset so we're on the default (Resolver.pex, pip via PEX). We have no Our environment is alittle unusual in that PEX_ROOT is a Pants append only named cache which is shared by every concurrently executing pex process on the machine and never pruned or invalidated by Pants. These are long-lived CI agents that serve many jobs in parallel and builds are routinely SIGKILLed mid-flight by the merge-queue. So high concurrency against one PEX_ROOT plus abrupt termination. Nothing external should write to or deletes from PEX_ROOT so it shouldnt be the case that another entity deleted cache dir contents. We also see PEX's own warning in the logs immediately before these: If you'd rather fix the root cause than the defensive check I'm happy to close it. I wrote it to stop a reuse loop with one bad write becoming a permanent failure for every subsequent build on that machine but it does not prevent creation of a bad entry. If the creation path gets fixed most of its value goes away, though it might still be useful for the case of SIGKILLS |
That is actually the typical Pants scenario! On your local machine your named cache directory is never pruned unless you do so manually and a single Pants invocation can run many Pex processes in parallel. As a result, there were many Pex bug fixes in the early days of Pants use of Pex as it exercised parallel Pex invocations.
The
So, if the SIGKILL happens at any point before 4, there is never a rename and so partial population of the work dir will never be seen at the final directory rename location. So all a SIGKILL can do is prevent a rename of a work dir to the final dir, in which case the work dir will be left around for the next try, and you'd see what you report: After obtaining an exclusive lock on .../packed_wheels/1/<hash>/.defN.atomic_directory.lck,
failed to establish a work directory at .../packed_wheels/1/<hash>/defN.lck.work
due to: [Errno 17] File exists
Continuing to forcibly re-create the work directory at ...This is as noted in the comment here: Lines 259 to 285 in e0d238b I.E.: workdir cleanup indicates a forceable kill of a prior attempt to populate the atomic directory (OK), or else a failed flock lock scenario where two processes hold the same lock (not OK!). Its the latter that I suspect in this scenario because I've reviewed this locking code so many times now over the last many years. The known cases where flock fails are network filesystems like NFS and other exotic storage configuration scenarios. I'm currently focused on your use of docker overlayfs2 for the Pants named-caches directory mount from the host to confirm that style of mount does or does not allow flock to be implemented faithfully. |
Problem
PEXBuilder._build_packedappadmits an existing bootstrap-zip or packed-wheel cache entry onatomic_directory(...).is_finalized()alone, which is justos.path.existsof the target dir, and thensafe_copys the cached zip into the PEX being built — outside the atomic block, without inspecting it.If an entry is ever finalized while incomplete, it is reused verbatim by every later build sharing that
PEX_ROOT. A single bad write becomes a permanent, deterministic failure for every subsequent build:or, when it is the bootstrap zip that is short,
ModuleNotFoundError: No module named 'pex.version'.We hit this across a large CI fleet. The tell that it is not a bad dependency is that the missing artifact differs every time — we saw
botocore,grpcio,rpds_py,ConfigArgParse,aiobotocore, and in one casepex.versionitself. The victim is positional, not packaged. Because the cache entry is stable, retries reproduce the failure byte-for-byte; one of our builds was retried 11 times and failed identically each time.Note the corrupt artifacts are structurally valid zips with correct central directories, so
testzip()andnamelist()both pass on them. They are simply missing members.Fix
cache_ziprecords the digest of the zip inside theatomic_directorywork dir, so it lands through the same atomic rename as the zip it describes, and verifies it before reuse. A mismatched entry is discarded and re-created once; a second failure raises rather than shipping a bad PEX.An entry written before this existed carries no digest and is treated as unverified, so caches already poisoned heal on first contact. That costs one re-pack per stale entry.
Scope
This addresses reuse of a bad entry, which is what turns one bad write into a permanent failure for everyone sharing the cache. It deliberately does not claim to prevent creation of one: the digest is taken from whatever
create_zipwrote, so a short write that still yields a structurally valid zip is only caught by the emptiness guard. The creation-side race appears to live inatomic_directory's EEXIST recovery, wheresafe_mkdir(work_dir, clean=True)cannot distinguish a dead writer from a live one — its own comment says as much. That felt like a maintainer call rather than something to fold in here, so I have left it alone.Verification
test_cache_zip_rejects_incomplete_entrycovers cold build, warm reuse without rebuild, a poisoned-but-structurally-valid entry being discarded and re-created, and an entry with no recorded digest being re-created.test_cache_zip_raises_when_no_zip_producedcovers the loud-failure path.--layout packedPEX, replace the cached packed wheel with a valid zip whose.dist-infois gone, rebuild. Before this change the build exits 0 and silently ships a corrupt PEX whosevenv --scope=depsthen fails with theMetadataErrorabove; after it, the entry is discarded, re-created, and the PEX is correct.tests/test_pex_builder.pyandtests/test_atomic_directory.pypass (46 passed). Twotest_build_compressioncases error withSystemExit: Pex tests must be run via testing/bin/runtests.pyboth with and without this change, so they are pre-existing to my environment rather than caused here.blackandisortclean. I could not getuv run dev-cmd format lint typecheckto bootstrap its venv locally (itspip install -U pipstep fails in my environment), so please treat CI as the authority on the full gate.Happy to adjust the approach — including moving the check inside
atomic_directoryinstead, if you would rather it be generic across all cache users.