Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.google.protobuf.ByteString;
import com.google.protobuf.ExtensionRegistryLite;
import java.io.FileNotFoundException;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -270,9 +271,7 @@ public void close() {
public ListenableFuture<Void> uploadFile(Digest digest, Path file) {
return executorService.submit(
() -> {
try (InputStream in = file.getInputStream()) {
saveFile(digest, Store.CAS, in);
}
saveFile(digest, Store.CAS, file);
return null;
});
}
Expand Down Expand Up @@ -313,6 +312,51 @@ public Path toPath(String hash, Store store) {
}

public void saveFile(Digest digest, Store store, InputStream in) throws IOException {
save(
digest,
store,
temp -> {
try (OutputStream out = temp.getOutputStream()) {
ByteStreams.copy(in, out);
// Fsync temp before we rename it to avoid data loss in the case of machine
// crashes (the OS may reorder the writes and the rename).
if (out instanceof FileOutputStream fos) {
fos.getFD().sync();
}
}
});
}

/**
* Saves an existing file into the cache.
*
* <p>The contents are copied through {@link FileSystemUtils#copyFile}, so a filesystem with
* copy-on-write support (clonefile on macOS, copy_file_range on Linux) can serve the copy as a
* clone, leaving the entry sharing its blocks with the file it was saved from.
*/
private void saveFile(Digest digest, Store store, Path file) throws IOException {
save(
digest,
store,
temp -> {
FileSystemUtils.copyFile(file, temp);
// copyFile preserves the source's permissions and mtime, neither of which suits a cache
// entry: an entry must remain readable by every user of a shared cache, and its mtime
// records when it was last stored or retrieved.
temp.chmod(0644);
temp.setLastModifiedTime(Path.NOW_SENTINEL_TIME);
// Fsync temp before we rename it to avoid data loss in the case of machine
// crashes (the OS may reorder the writes and the rename).
syncFile(temp);
});
}

/** Writes the contents of a cache entry into a temporary file. */
private interface TempFileWriter {
void write(Path temp) throws IOException;
}

private void save(Digest digest, Store store, TempFileWriter writer) throws IOException {
Path path = toPath(digest, store);

// CAS entries are content-addressed and thus automatically have the correct content if they
Expand All @@ -325,14 +369,7 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept
Path temp = getTempPath();

try {
try (OutputStream out = temp.getOutputStream()) {
ByteStreams.copy(in, out);
// Fsync temp before we rename it to avoid data loss in the case of machine
// crashes (the OS may reorder the writes and the rename).
if (out instanceof FileOutputStream fos) {
fos.getFD().sync();
}
}
writer.write(temp);
path.getParentDirectory().createDirectoryAndParents();
FileSystemUtils.renameToleratingConcurrentCreation(temp, path);
} catch (IOException e) {
Expand All @@ -344,4 +381,13 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept
throw e;
}
}

/** Flushes a file's contents to stable storage, where the filesystem supports it. */
private static void syncFile(Path path) throws IOException {

@fmeum fmeum Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the same mechanism as in the fallback path (e.g. FileInputStream).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like the revised version? I'm not entirely clear on the importance of the sync semantics here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither, which is why I like this better since the semantics agree ;⁠-⁠)

try (InputStream in = path.getInputStream()) {
if (in instanceof FileInputStream fileInputStream) {
fileInputStream.getFD().sync();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,26 @@ public void uploadFile_whenPresent_updatesMtime() throws Exception {
assertThat(path.getLastModifiedTime()).isNotEqualTo(0);
}

@Test
public void uploadFile_whenMissing_doesNotInheritSourceMtimeOrPermissions() throws Exception {
Path file = fs.getPath("/file");
FileSystemUtils.writeContent(file, UTF_8, "contents");
// A build output is read-only, and may have been written long before it is uploaded. Neither
// property may leak into the cache entry: the mtime records when the entry was last stored or
// retrieved, and the entry must remain readable by every user of a shared cache.
file.chmod(0555);
file.setLastModifiedTime(1000);
Digest digest = getDigest("contents");

var unused = getFromFuture(client.uploadFile(digest, file));

Path path = getCasPath(digest);
assertThat(FileSystemUtils.readContent(path, UTF_8)).isEqualTo("contents");
assertThat(path.getLastModifiedTime()).isNotEqualTo(1000);
assertThat(path.isReadable()).isTrue();
assertThat(path.isWritable()).isTrue();
}

@Test
public void uploadBlob_whenMissing_populatesCas() throws Exception {
ByteString blob = ByteString.copyFromUtf8("contents");
Expand Down
Loading