diff --git a/docs/notes/2.34.x.md b/docs/notes/2.34.x.md index 161da74f163..978eb7364b0 100644 --- a/docs/notes/2.34.x.md +++ b/docs/notes/2.34.x.md @@ -61,6 +61,8 @@ Fixed `system_binary` / PATH lookup crashing with `IntrinsicError: Operation not Fixed pantsd logging remote Authentication bearer tokens in plaintext when `remote_store_headers` / `remote_execution_headers` / `remote_oauth_bearer_token` (or the matching `DynamicRemoteOptions` header fields) changed and the scheduler reinitialized. Those sensitive values are now shown as `` in the reinitialization diff ([#23685](https://github.com/pantsbuild/pants/issues/23685)). +Fixed the `experimental-file` remote cache provider (`remote_store_address = "file://..."`) leaving truncated blobs behind when a write was interrupted, for example by killing Pants mid-upload. Blobs were written directly to their final path, so later runs found a truncated file under the right name and failed with `Remote CAS gave wrong digest`. Each blob is now written to a temporary file in a `.tmp` directory under the store's root and renamed into place once complete. Existing truncated blobs are not repaired; delete the affected files, or the store directory, to clear them. + Pants now logs a warning when it cannot delete a local process execution sandbox. Cleanup failures were previously ignored, so sandboxes containing read-only directories leaked in the temporary directory without any message. An idle `pantsd` no longer re-expands and rewrites the lease of every store entry its graph references every 72 seconds. A lease with more than half of its time left is no longer rewritten, and a digest leased within the last quarter of the lease time (30 minutes) is not expanded again, which removes most of the CPU time and disk writes an idle daemon spent extending leases ([#23694](https://github.com/pantsbuild/pants/issues/23694)). diff --git a/src/rust/remote_provider/remote_provider_opendal/src/byte_store_tests.rs b/src/rust/remote_provider/remote_provider_opendal/src/byte_store_tests.rs index 092f900a990..dc24425e42b 100644 --- a/src/rust/remote_provider/remote_provider_opendal/src/byte_store_tests.rs +++ b/src/rust/remote_provider/remote_provider_opendal/src/byte_store_tests.rs @@ -297,3 +297,86 @@ async fn list_missing_digests_empty_never_missing() { Ok(HashSet::new()) ) } + +fn new_fs_provider(root: &std::path::Path) -> Provider { + Provider::fs(root.to_str().unwrap(), BASE.to_owned(), remote_options()).unwrap() +} + +#[tokio::test] +async fn fs_store_file_round_trip() { + let root = tempfile::TempDir::new().unwrap(); + let testdata = TestData::all_the_henries(); + let provider = new_fs_provider(root.path()); + + provider + .store_file( + testdata.digest(), + mk_tempfile(Some(&testdata.bytes())).await, + ) + .await + .unwrap(); + assert_store(&provider, &testdata).await; + + // The temporary file was renamed into place, not left behind. + let leftovers = std::fs::read_dir(root.path().join(".tmp")).unwrap().count(); + assert_eq!(leftovers, 0); +} + +#[tokio::test] +async fn fs_partial_write_is_not_visible() { + let root = tempfile::TempDir::new().unwrap(); + let testdata = TestData::all_the_henries(); + let provider = new_fs_provider(root.path()); + let path = test_path(&testdata); + + // Start a write and stop partway, as happens when the process is killed mid-upload. + let mut writer = provider.operator.writer(&path).await.unwrap(); + writer.write(testdata.bytes().slice(..1024)).await.unwrap(); + + // A truncated blob must never be visible under the digest's name. + assert!(!root.path().join(&path).exists()); + let mut destination = Vec::new(); + assert!( + !provider + .load(testdata.digest(), &mut destination) + .await + .unwrap() + ); + assert_eq!( + provider + .list_missing_digests(&mut vec![testdata.digest()].into_iter()) + .await, + Ok(HashSet::from([testdata.digest()])) + ); + + // Once the write completes, the whole blob appears at once. + writer.write(testdata.bytes().slice(1024..)).await.unwrap(); + writer.close().await.unwrap(); + assert_store(&provider, &testdata).await; +} + +#[tokio::test] +async fn fs_concurrent_writes_do_not_collide() { + let root = tempfile::TempDir::new().unwrap(); + let testdata = TestData::all_the_henries(); + let provider = new_fs_provider(root.path()); + let path = test_path(&testdata); + + // Two writers of the same blob, interleaved, each get their own temporary file. + let mut first = provider.operator.writer(&path).await.unwrap(); + let mut second = provider.operator.writer(&path).await.unwrap(); + first.write(testdata.bytes().slice(..1024)).await.unwrap(); + second.write(testdata.bytes().slice(..1024)).await.unwrap(); + let in_flight = std::fs::read_dir(root.path().join(".tmp")).unwrap().count(); + assert_eq!(in_flight, 2); + + first.write(testdata.bytes().slice(1024..)).await.unwrap(); + second.write(testdata.bytes().slice(1024..)).await.unwrap(); + first.close().await.unwrap(); + assert_store(&provider, &testdata).await; + second.close().await.unwrap(); + assert_store(&provider, &testdata).await; + + let leftovers = std::fs::read_dir(root.path().join(".tmp")).unwrap().count(); + assert_eq!(leftovers, 0); +} diff --git a/src/rust/remote_provider/remote_provider_opendal/src/lib.rs b/src/rust/remote_provider/remote_provider_opendal/src/lib.rs index beaa3c5147b..ce15101cae3 100644 --- a/src/rust/remote_provider/remote_provider_opendal/src/lib.rs +++ b/src/rust/remote_provider/remote_provider_opendal/src/lib.rs @@ -4,6 +4,7 @@ #![recursion_limit = "256"] use std::collections::{HashMap, HashSet}; +use std::path::Path; use std::time::Instant; use async_trait::async_trait; @@ -76,7 +77,15 @@ impl Provider { } pub fn fs(path: &str, scope: String, options: RemoteStoreOptions) -> Result { - let builder = opendal::services::Fs::default().root(path); + // Write each blob to a temporary file and rename it into place once complete. Without + // this, opendal writes directly to the final path, so an interrupted write (e.g. a killed + // process) leaves a truncated blob under its digest's name. The temporary directory is + // inside the root so that the rename never crosses a filesystem boundary. opendal gives + // each write its own randomly named temporary file, so concurrent writers don't collide. + let atomic_write_dir = Path::new(path).join(".tmp"); + let builder = opendal::services::Fs::default() + .root(path) + .atomic_write_dir(&atomic_write_dir.to_string_lossy()); Provider::new(builder, opendal::services::FS_SCHEME, scope, options) }