[clangd] Change disk-backed storage to be atomic

[kadircet]

Summary

There was a chance that multiple clangd instances could try to write
same shard, in which case we would get a malformed file most likely. This patch
changes the writing mechanism to first write to a temporary file and then rename
it to fit real destination. Which is guaranteed to be atomic by POSIX.

[kadircet]

• Fix returns

[ilya-biryukov]

NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.

clangd/index/BackgroundIndexStorage.cpp

103

Maybe extract this into a helper function?
Something like

std::error_code writeAtomically(StringRef OutPath, function_ref<void(raw_ostream&)> Writer) {
 /// create temp file, open output stream.
 Writer(OS):
 /// Move the temp file into OutPath.
}

This would keep the storeShard function as readable as it is now

105

Maybe create these temporary files in the same dir? That would ensure the move operations do not require transferring data between physical devices.
If the output file is /some/path/foo.bar, we could create files named /some/path/foo.bar.tmp#####

[kadircet]

• Address comments

[ilya-biryukov]

clangd/index/BackgroundIndexStorage.cpp

42

There's a helper in LLVM that will do this, llvm::createUniqueFile(), I believe it also tries multiple times in case of clashes.

55

We should not remove if no error occurred, as the same name at this point can be taken by a different action.

[kadircet]

• Use creauteUniqueFile.
• Don't remove on success.

[ilya-biryukov]

LGTM with a few nits

clangd/index/BackgroundIndexStorage.cpp

39

NIT: use llvm::function_ref here. It forces to clearly state the signature of the function and allows to avoid templates.

44

NIT: I believe there's an overloaded operator +(StringRef, const char*) that would produce a Twine, so maybe simplify to OutPath + ".tmp.%%%%%"?
Just a suggestion, up to you.

60

NIT: s/it/the name/

[kadircet]

• Address comments

clangd/index/BackgroundIndexStorage.cpp

 //== BackgroundIndexStorage.cpp - Provide caching support to BackgroundIndex ==/
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Logger.h"
 #include "index/Background.h"
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using FileDigest = decltype(llvm::SHA1::hash({}));
 
 static FileDigest digest(StringRef Content) {
   return llvm::SHA1::hash({(const uint8_t *)Content.data(), Content.size()});
 }
 
 std::string getShardPathFromFilePath(llvm::StringRef ShardRoot,
                                      llvm::StringRef FilePath) {
   llvm::SmallString<128> ShardRootSS(ShardRoot);
   llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) +
                                            "." + llvm::toHex(digest(FilePath)) +
                                            ".idx");
   return ShardRootSS.str();
 }
 
+llvm::Error
+writeAtomically(llvm::StringRef OutPath,
+                llvm::function_ref<void(llvm::raw_ostream &)> Writer) {
+  // Write to a temporary file first.
+  llvm::SmallString<128> TempPath;
+  int FD;
+  auto EC =
+      llvm::sys::fs::createUniqueFile(OutPath + ".tmp.%%%%%%%%", FD, TempPath);
+  if (EC)
+    return llvm::errorCodeToError(EC);
+  // Make sure temp file is destroyed on failure.
+  auto RemoveOnFail =
+      llvm::make_scope_exit([TempPath] { llvm::sys::fs::remove(TempPath); });
+  llvm::raw_fd_ostream OS(FD, /*shouldClose=*/true);
+  Writer(OS);
+  OS.close();
+  if (OS.has_error())
+    return llvm::errorCodeToError(OS.error());
+  // Then move to real location.
+  EC = llvm::sys::fs::rename(TempPath, OutPath);
+  if (EC)
+    return llvm::errorCodeToError(EC);
+  // If everything went well, we already moved the file to another name. So
+  // don't delete the file, as the name might be taken by another file.
+  RemoveOnFail.release();
+  return llvm::ErrorSuccess();
+}
+
 // Uses disk as a storage for index shards. Creates a directory called
 // ".clangd-index/" under the path provided during construction.
 class DiskBackedIndexStorage : public BackgroundIndexStorage {
   std::string DiskShardRoot;
 
 public:
   // Sets DiskShardRoot to (Directory + ".clangd-index/") which is the base
   // directory for all shard files.
loadShard(llvm::StringRef ShardIdentifier) const override {
     else
       elog("Error while reading shard {0}: {1}", ShardIdentifier,
            I.takeError());
     return nullptr;
   }
 
   llvm::Error storeShard(llvm::StringRef ShardIdentifier,
                          IndexFileOut Shard) const override {
-    auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
-    std::error_code EC;
-    llvm::raw_fd_ostream OS(ShardPath, EC);
-    if (EC)
-      return llvm::errorCodeToError(EC);
-    OS << Shard;
-    OS.close();
-    return llvm::errorCodeToError(OS.error());
+    return writeAtomically(
+        getShardPathFromFilePath(DiskShardRoot, ShardIdentifier),
+        [&Shard](llvm::raw_ostream &OS) { OS << Shard; });
   }
 };
 
 // Doesn't persist index shards anywhere (used when the CDB dir is unknown).
 // We could consider indexing into ~/.clangd/ or so instead.
 class NullStorage : public BackgroundIndexStorage {
 public:
   std::unique_ptr<IndexFileIn>