Introduce capabilities instead of version numbers for takeover

Summary:
This commit is introducing capabilities for the takeover code. This will make it
easier to evolve the protocol. Which I am about to do to support NFS.

Why is capabilities worth adding?
Currently takeover uses a versioning based approach to evolve the takeover
protocol. This works and is pretty simple. The downside is that it leads us to
making lots of checks in the code base like: if this version then do x.

There are two downsides of such checks:
1. reading the code is not self contained. You have to know or have already
read which version introduces which features for the checks to make sense. This
generally makes the code harder to read and harder to check in code review.

2. introducing new versions sometimes mean we need to update those checks we
previously added. For example we make this check in the server on wether to
send a ping message before sending data:
```
  shouldPing_ =
      (protocolVersion_ == TakeoverData::kTakeoverProtocolVersionFour);
```

if we add a kTakeoverProtocolVersionFive where we still want to send the ping (
which we likely do because the ping is a part of the protocol correctness under
failures), then we need to update this time to

```
  shouldPing_ =
      (protocolVersion_ >= TakeoverData::kTakeoverProtocolVersionFour);
```

then if we replace the ping with something else we would have to again update
the code to
```
  shouldPing_ =
      (protocolVersion_ >= TakeoverData::kTakeoverProtocolVersionFour
      && protocolVersion_ <= lastTakeoverVersionWithPing);
```

The first of these code updates is very easy to skip accidentally if you are
making an update to the protocol that has nothing to do with ping. This would
silently introduce a regression.

Using capabilities instead makes this check
```
  shouldPing_ =
      (protocolVersionCapabilities_ & TakeoverCapabilities::PING);
```
This is more readable. and this code never has to be updated; we just don't use
the ping capability any more.

Eventually, if we go down the capabilities route we will want to kill off
versions completely. This will mean bumping the version of the protocol, where
we agree on capabilities after agreeing to that version. After this version is
stable (released to production and a good track record indicating we don't need
to rollback prior to the change), we can delete the older versions of the
protocol and all the version based stuff in the code. This stabilizing process
will take a while, so I do not want this to block the NFS evolution I am doing
in the next change. I will comeback to this after we get NFS support in graceful
restart.

Reviewed By: xavierd

Differential Revision: D31965080

fbshipit-source-id: f8413d07da42a92125d46aa3cfbd3fa458b87b46

eden/fs/takeover/TakeoverClient.cpp

@@ -97,25 +97,11 @@ TakeoverData takeoverMounts(
     expectedMessage.error().throw_exception();
   }
   auto& message = expectedMessage.value();
-
-  auto data = TakeoverData::deserialize(&message.data);
-  // Add 2 here for the lock file and the thrift socket
-  if (data.mountPoints.size() + 2 != message.files.size()) {
-    throw std::runtime_error(folly::to<string>(
-        "received ",
-        data.mountPoints.size(),
-        " mount paths, but ",
-        message.files.size(),
-        " FDs (including the lock file FD)"));
-  }
-  data.lockFile = std::move(message.files[0]);
-  data.thriftSocket = std::move(message.files[1]);
-  for (size_t n = 0; n < data.mountPoints.size(); ++n) {
-    auto& mountInfo = data.mountPoints[n];
-    mountInfo.fuseFD = std::move(message.files[n + 2]);
+  for (auto& file : message.files) {
+    XLOG(DBG7) << "received fd for takeover: " << file.fd();
   }
 
-  return data;
+  return TakeoverData::deserialize(message);
 }
 } // namespace eden
 } // namespace facebook

eden/fs/takeover/TakeoverData.cpp

@@ -9,6 +9,8 @@
 
 #include "eden/fs/takeover/TakeoverData.h"
 
+#include <stdexcept>
+
 #include <folly/Format.h>
 #include <folly/io/Cursor.h>
 #include <folly/io/IOBuf.h>
@@ -47,39 +49,98 @@ std::optional<int32_t> TakeoverData::computeCompatibleVersion(
   return best;
 }
 
-IOBuf TakeoverData::serialize(int32_t protocolVersion) {
-  switch (protocolVersion) {
+uint64_t TakeoverData::versionToCapabilites(int32_t version) {
+  switch (version) {
+    case kTakeoverProtocolVersionNeverSupported:
+      return 0;
     case kTakeoverProtocolVersionOne:
-      return serializeVersion1();
+      return TakeoverCapabilities::CUSTOM_SERIALIZATION |
+          TakeoverCapabilities::FUSE;
     case kTakeoverProtocolVersionThree:
+      return TakeoverCapabilities::FUSE |
+          TakeoverCapabilities::THRIFT_SERIALIZATION;
     case kTakeoverProtocolVersionFour:
-      // versions 3 and 4 use the same data serialization
-      return serializeVersion3();
-    default: {
-      EDEN_BUG() << "asked to serialize takeover data in unsupported format "
-                 << protocolVersion;
+      return TakeoverCapabilities::FUSE |
+          TakeoverCapabilities::THRIFT_SERIALIZATION |
+          TakeoverCapabilities::PING;
   }
+  throw std::runtime_error(fmt::format("Unsupported version: {}", version));
+}
+
+int32_t TakeoverData::capabilitesToVersion(uint64_t capabilities) {
+  if (capabilities == 0) {
+    return kTakeoverProtocolVersionNeverSupported;
+  }
+  if (capabilities ==
+      (TakeoverCapabilities::CUSTOM_SERIALIZATION |
+       TakeoverCapabilities::FUSE)) {
+    return kTakeoverProtocolVersionOne;
+  }
+  if (capabilities ==
+      (TakeoverCapabilities::FUSE |
+       TakeoverCapabilities::THRIFT_SERIALIZATION)) {
+    return kTakeoverProtocolVersionThree;
+  }
+  if (capabilities ==
+      (TakeoverCapabilities::FUSE | TakeoverCapabilities::THRIFT_SERIALIZATION |
+       TakeoverCapabilities::PING)) {
+    return kTakeoverProtocolVersionFour;
+  }
+
+  throw std::runtime_error(
+      fmt::format("Unsupported combination of capabilities: {}", capabilities));
+}
+
+void TakeoverData::serialize(
+    uint64_t protocolCapabilities,
+    UnixSocket::Message& msg) {
+  msg.data = serialize(protocolCapabilities);
+  msg.files.push_back(std::move(lockFile));
+  msg.files.push_back(std::move(thriftSocket));
+  for (auto& mount : mountPoints) {
+    msg.files.push_back(std::move(mount.fuseFD));
+  }
+}
+
+IOBuf TakeoverData::serialize(uint64_t protocolCapabilities) {
+  uint64_t serializationMethod = protocolCapabilities &
+      (TakeoverCapabilities::CUSTOM_SERIALIZATION |
+       TakeoverCapabilities::THRIFT_SERIALIZATION);
+
+  if (serializationMethod == TakeoverCapabilities::CUSTOM_SERIALIZATION) {
+    return serializeCustom();
+  } else if (
+      serializationMethod == TakeoverCapabilities::THRIFT_SERIALIZATION) {
+    return serializeThrift(protocolCapabilities);
+  } else {
+    throw std::runtime_error(fmt::format(
+        "Asked to serialize takeover data in unsupported format. "
+        "Cababilities: {}",
+        protocolCapabilities));
   }
 }
 
 folly::IOBuf TakeoverData::serializeError(
-    int32_t protocolVersion,
+    uint64_t protocolCapabilities,
     const folly::exception_wrapper& ew) {
-  switch (protocolVersion) {
+  uint64_t serializationMethod = protocolCapabilities &
+      (TakeoverCapabilities::CUSTOM_SERIALIZATION |
+       TakeoverCapabilities::THRIFT_SERIALIZATION);
+
   // We allow NeverSupported in the error case so that we don't
-    // end up EDEN_BUG'ing out in the version mismatch error
+  // end up erroring out in the version mismatch error
   // reporting case.
-    case kTakeoverProtocolVersionNeverSupported:
-    case kTakeoverProtocolVersionOne:
-      return serializeErrorVersion1(ew);
-    case kTakeoverProtocolVersionThree:
-    case kTakeoverProtocolVersionFour:
-      // versions 3 and 4 use the same data serialization
-      return serializeErrorVersion3(ew);
-    default: {
-      EDEN_BUG() << "asked to serialize takeover error in unsupported format "
-                 << protocolVersion;
-    }
+  if (serializationMethod == TakeoverCapabilities::CUSTOM_SERIALIZATION ||
+      protocolCapabilities == 0) {
+    return serializeErrorCustom(ew);
+  } else if (
+      serializationMethod == TakeoverCapabilities::THRIFT_SERIALIZATION) {
+    return serializeErrorThrift(ew);
+  } else {
+    throw std::runtime_error(fmt::format(
+        "Asked to serialize takeover error in unsupported format. "
+        "Capabilities: {}",
+        protocolCapabilities));
   }
 }
 
@@ -99,7 +160,32 @@ folly::IOBuf TakeoverData::serializePing() {
   return buf;
 }
 
-TakeoverData TakeoverData::deserialize(IOBuf* buf) {
+TakeoverData TakeoverData::deserialize(UnixSocket::Message& msg) {
+  auto protocolVersion = TakeoverData::getProtocolVersion(&msg.data);
+  auto capabilities = TakeoverData::versionToCapabilites(protocolVersion);
+
+  auto data = TakeoverData::deserialize(capabilities, &msg.data);
+  constexpr auto mountPointFilesOffset = 2;
+
+  // Add 2 here for the lock file and the thrift socket
+  if (data.mountPoints.size() + mountPointFilesOffset != msg.files.size()) {
+    throw std::runtime_error(folly::to<string>(
+        "received ",
+        data.mountPoints.size(),
+        " mount paths, but ",
+        msg.files.size(),
+        " FDs (including the lock file FD)"));
+  }
+  data.lockFile = std::move(msg.files[0]);
+  data.thriftSocket = std::move(msg.files[1]);
+  for (size_t n = 0; n < data.mountPoints.size(); ++n) {
+    auto& mountInfo = data.mountPoints[n];
+    mountInfo.fuseFD = std::move(msg.files[n + mountPointFilesOffset]);
+  }
+  return data;
+}
+
+int32_t TakeoverData::getProtocolVersion(IOBuf* buf) {
   // We need to probe the data to see which version we have
   folly::io::Cursor cursor(buf);
 
@@ -109,13 +195,14 @@ TakeoverData TakeoverData::deserialize(IOBuf* buf) {
     case MessageType::MOUNTS:
       // A version 1 response.  We don't advance the buffer that we pass down
       // because it the messageType is needed to decode the response.
-      return deserializeVersion1(buf);
+      return kTakeoverProtocolVersionOne;
     case kTakeoverProtocolVersionThree:
+    case kTakeoverProtocolVersionFour:
       // Version 3 (there was no 2 because of how Version 1 used word values
       // 1 and 2) doesn't care about this version byte, so we skip past it
       // and let the underlying code decode the data
       buf->trimStart(sizeof(uint32_t));
-      return deserializeVersion3(buf);
+      return messageType;
     default:
       throw std::runtime_error(fmt::format(
           "Unrecognized TakeoverData response starting with {:x}",
@@ -123,7 +210,25 @@ TakeoverData TakeoverData::deserialize(IOBuf* buf) {
   }
 }
 
-IOBuf TakeoverData::serializeVersion1() {
+TakeoverData TakeoverData::deserialize(
+    uint64_t protocolCapabilities,
+    IOBuf* buf) {
+  uint64_t serializationMethod = protocolCapabilities &
+      (TakeoverCapabilities::CUSTOM_SERIALIZATION |
+       TakeoverCapabilities::THRIFT_SERIALIZATION);
+  if (serializationMethod == TakeoverCapabilities::CUSTOM_SERIALIZATION) {
+    return deserializeCustom(buf);
+  }
+  if (serializationMethod == TakeoverCapabilities::THRIFT_SERIALIZATION) {
+    return deserializeThrift(buf);
+  }
+
+  throw std::runtime_error(fmt::format(
+      "Unrecognized TakeoverData serialization capability {:x}",
+      protocolCapabilities));
+}
+
+IOBuf TakeoverData::serializeCustom() {
   // Compute the body data length
   uint64_t bodyLength = sizeof(uint32_t);
   for (const auto& mount : mountPoints) {
@@ -193,7 +298,7 @@ IOBuf TakeoverData::serializeVersion1() {
   return buf;
 }
 
-folly::IOBuf TakeoverData::serializeErrorVersion1(
+folly::IOBuf TakeoverData::serializeErrorCustom(
     const folly::exception_wrapper& ew) {
   // Compute the body data length
   auto exceptionClassName = ew.class_name();
@@ -218,7 +323,7 @@ folly::IOBuf TakeoverData::serializeErrorVersion1(
   return buf;
 }
 
-TakeoverData TakeoverData::deserializeVersion1(IOBuf* buf) {
+TakeoverData TakeoverData::deserializeCustom(IOBuf* buf) {
   folly::io::Cursor cursor(buf);
 
   auto messageType = cursor.readBE<uint32_t>();
@@ -284,14 +389,25 @@ TakeoverData TakeoverData::deserializeVersion1(IOBuf* buf) {
   return data;
 }
 
-IOBuf TakeoverData::serializeVersion3() {
+IOBuf TakeoverData::serializeThrift(uint64_t protocolCapabilities) {
   SerializedTakeoverData serialized;
 
   folly::IOBufQueue bufQ;
   folly::io::QueueAppender app(&bufQ, 0);
 
-  // First word is the protocol version
-  app.writeBE<uint32_t>(kTakeoverProtocolVersionThree);
+  { // we scope this to avoid using the version any further in the code.
+    // Ideally we would only use capabilities, but we need to send version
+    // numbers to be compatible with older version.
+    int32_t versionToAdvertize = capabilitesToVersion(protocolCapabilities);
+    // first word is the protocol version. previous versions of EdenFS do not
+    // know how to deserialize version 4 because they assume that protocol 4
+    // uses protocol 3 serialization. We need to do this funkiness for rollback
+    // safety.
+    if (versionToAdvertize == kTakeoverProtocolVersionFour) {
+      versionToAdvertize = kTakeoverProtocolVersionThree;
+    }
+    app.writeBE<uint32_t>(versionToAdvertize);
+  }
 
   std::vector<SerializedMountInfo> serializedMounts;
   for (const auto& mount : mountPoints) {
@@ -325,7 +441,7 @@ IOBuf TakeoverData::serializeVersion3() {
   return std::move(*bufQ.move());
 }
 
-folly::IOBuf TakeoverData::serializeErrorVersion3(
+folly::IOBuf TakeoverData::serializeErrorThrift(
     const folly::exception_wrapper& ew) {
   SerializedTakeoverData serialized;
   auto exceptionClassName = ew.class_name();
@@ -343,7 +459,7 @@ folly::IOBuf TakeoverData::serializeErrorVersion3(
   return std::move(*bufQ.move());
 }
 
-TakeoverData TakeoverData::deserializeVersion3(IOBuf* buf) {
+TakeoverData TakeoverData::deserializeThrift(IOBuf* buf) {
   auto serialized = CompactSerializer::deserialize<SerializedTakeoverData>(buf);
 
   switch (serialized.getType()) {

eden/fs/takeover/TakeoverData.h

@@ -15,6 +15,7 @@
 
 #include "eden/fs/takeover/gen-cpp2/takeover_types.h"
 #include "eden/fs/utils/FsChannelTypes.h"
+#include "eden/fs/utils/FutureUnixSocket.h"
 #include "eden/fs/utils/PathFuncs.h"
 
 namespace folly {
@@ -26,8 +27,55 @@ namespace facebook {
 namespace eden {
 
 // Holds the versions supported by this build.
+// TODO(T104382350): The code is being migrated to use capabilities bits instead
+// of versions numbers as the former makes it less error prone to check for
+// supported features by both the client and server. Currently, the protocol
+// works by agreeing on a mutually supported version which is then
+// deterministically mapped to a set of capabilities. We would like to instead
+// agree on a shared set of capabilities. To completely migrate to capabilities
+// this we need to bump the version number and introduce
+// kSupportedTakeoverCapabilities (Should use a lower overhead representation
+// than std::set, probably a uint). We also need to teach the server and client
+// to find the common supported capabilities of the client and server. After
+// this capability matching version of takeover has made it into a stable build,
+// we can delete the version related code (and all code for early versions).
+// Note "stable build" means we should never need to rollback before the change,
+// so the capability matching build should be out for at least a month before we
+// delete all the version code.
 extern const std::set<int32_t> kSupportedTakeoverVersions;
 
+// TODO (T104724681): use a nicer representation for a bit set to combine these
+// flags like watchman's OptionSet.
+// Note that capabilities must be things that a new version is able to do, not
+// things that that version can not do. Capabilities must be all positive so
+// that we can find the intersection of the capabilities of the server and
+// client to find all the capabilities they both support.
+class TakeoverCapabilities {
+ public:
+  enum : uint64_t {
+    // This indicates we use our own invented format for sending takeover data
+    // between the client and server. This was used in early versions of the
+    // protocol.
+    CUSTOM_SERIALIZATION = 1 << 0,
+
+    // Indicates this version of the protocol is able to serialize FUSE mount
+    // points.
+    FUSE = 1 << 1,
+
+    // Indicates this version of the protocol uses thrift based serialization.
+    // This means we use thrift to serialize our takeover data when sending it
+    // between client and server. See the types defined in takeover.thrift.
+    // This is used in all the modern takeover versions.
+    THRIFT_SERIALIZATION = 1 << 2,
+
+    // Indicates a ping will be sent by the server to the client before sending
+    // takeover data. This handles client failure cases more gracefully.
+    // This should be used in all modern takeover versions.
+    PING = 1 << 3,
+
+  };
+};
+
 /**
  * TakeoverData contains the data exchanged between processes during
  * graceful mount point takeover.
@@ -72,9 +120,28 @@ class TakeoverData {
     kTakeoverProtocolVersionFour = 4,
   };
 
-  // Given a set of versions provided by a client, find the largest
-  // version that is also present in the provided set of supported
-  // versions.
+  /**
+   * Converts a supported version to the capabilities that version of the
+   * protocol supports. This is used as a bridge between the version based
+   * protocol we use now and the capability based protocol we would like to use
+   * in future versions. See T104382350.
+   */
+  static uint64_t versionToCapabilites(int32_t version);
+
+  /**
+   * Converts a valid set of capabilities into the takeover version that
+   * supports exactly those capabilities. This is used to "serialize" the
+   * capabilities. Older versions of the protocol were version based instead of
+   * capability based. So we "serialize" the capabilities as a version number.
+   * Eventually we will migrate off versions, then we can get rid of this.
+   */
+  static int32_t capabilitesToVersion(uint64_t capabilities);
+
+  /**
+   * Given a set of versions provided by a client, find the largest
+   * version that is also present in the provided set of supported
+   * versions.
+   */
   static std::optional<int32_t> computeCompatibleVersion(
       const std::set<int32_t>& versions,
       const std::set<int32_t>& supported = kSupportedTakeoverVersions);
@@ -116,13 +183,13 @@ class TakeoverData {
    * This includes all data except for file descriptors.  The file descriptors
    * must be sent separately.
    */
-  folly::IOBuf serialize(int32_t protocolVersion);
+  void serialize(uint64_t protocolCapabilities, UnixSocket::Message& msg);
 
   /**
    * Serialize an exception.
    */
   static folly::IOBuf serializeError(
-      int32_t protocolVersion,
+      uint64_t protocolCapabilities,
       const folly::exception_wrapper& ew);
 
   /**
@@ -131,9 +198,18 @@ class TakeoverData {
   static folly::IOBuf serializePing();
 
   /**
-   * Deserialize the TakeoverData from a buffer.
+   * Determine the protocol version of the serialized message in buf.
+   *
+   * Note this should only be called once. This will advance the buffer past
+   * the version byte so that the data to deserialize is at the beginning of the
+   * buffer.
    */
-  static TakeoverData deserialize(folly::IOBuf* buf);
+  static int32_t getProtocolVersion(folly::IOBuf* buf);
+
+  /**
+   * Deserialize the TakeoverData from a UnixSocket msg.
+   */
+  static TakeoverData deserialize(UnixSocket::Message& msg);
 
   /**
    * Checks to see if a message is of type PING
@@ -163,39 +239,54 @@ class TakeoverData {
   folly::Promise<std::optional<TakeoverData>> takeoverComplete;
 
  private:
+  /**
+   * Serialize the TakeoverData using the specified protocol version into a
+   * buffer that can be sent to a remote process.
+   *
+   * This includes all data except for file descriptors; these must be sent
+   * separately.
+   */
+  folly::IOBuf serialize(uint64_t protocolCapabilities);
+
   /**
    * Serialize data using version 1 of the takeover protocol.
    */
-  folly::IOBuf serializeVersion1();
+  folly::IOBuf serializeCustom();
 
   /**
    * Serialize an exception using version 1 of the takeover protocol.
    */
-  static folly::IOBuf serializeErrorVersion1(
-      const folly::exception_wrapper& ew);
+  static folly::IOBuf serializeErrorCustom(const folly::exception_wrapper& ew);
+
+  /**
+   * Serialize data using version 2 of the takeover protocol.
+   */
+  folly::IOBuf serializeThrift(uint64_t protocolCapabilities);
+
+  /**
+   * Serialize an exception using version 2 of the takeover protocol.
+   */
+  static folly::IOBuf serializeErrorThrift(const folly::exception_wrapper& ew);
+
+  /**
+   * Deserialize the TakeoverData from a buffer. We assume that we are only sent
+   * mounts with mount protocols that we are able to parse.
+   */
+  static TakeoverData deserialize(
+      uint64_t protocolCapabilities,
+      folly::IOBuf* buf);
+
+  /**
+   * Deserialize the TakeoverData from a buffer using version 3 (also known as
+   * 2), 4 or 5 of the takeover protocol.
+   */
+  static TakeoverData deserializeThrift(folly::IOBuf* buf);
 
   /**
    * Deserialize the TakeoverData from a buffer using version 1 of the takeover
    * protocol.
    */
-  static TakeoverData deserializeVersion1(folly::IOBuf* buf);
-
-  /**
-   * Serialize data using version 2 of the takeover protocol.
-   */
-  folly::IOBuf serializeVersion3();
-
-  /**
-   * Serialize an exception using version 2 of the takeover protocol.
-   */
-  static folly::IOBuf serializeErrorVersion3(
-      const folly::exception_wrapper& ew);
-
-  /**
-   * Deserialize the TakeoverData from a buffer using version 2 of the takeover
-   * protocol.
-   */
-  static TakeoverData deserializeVersion3(folly::IOBuf* buf);
+  static TakeoverData deserializeCustom(folly::IOBuf* buf);
 
   /**
    * Message type values.

eden/fs/takeover/TakeoverServer.cpp

@@ -81,6 +81,7 @@ class TakeoverServer::ConnHandler {
   FutureUnixSocket socket_;
   int32_t protocolVersion_{
       TakeoverData::kTakeoverProtocolVersionNeverSupported};
+  uint64_t protocolCapabilities_{0};
 };
 
 Future<Unit> TakeoverServer::ConnHandler::start() noexcept {
@@ -148,8 +149,9 @@ Future<Unit> TakeoverServer::ConnHandler::start() noexcept {
           }
           // Initiate the takeover shutdown.
           protocolVersion_ = supported.value();
-          shouldPing_ =
-              (protocolVersion_ == TakeoverData::kTakeoverProtocolVersionFour);
+          protocolCapabilities_ =
+              TakeoverData::versionToCapabilites(protocolVersion_);
+          shouldPing_ = (protocolCapabilities_ & TakeoverCapabilities::PING);
           return server_->getTakeoverHandler()->startTakeoverShutdown();
         })
         .thenTryInline(folly::makeAsyncTask(
@@ -176,7 +178,8 @@ Future<Unit> TakeoverServer::ConnHandler::sendError(
   XLOG(ERR) << "error while performing takeover shutdown: " << error;
   if (socket_) {
     // Send the error to the client.
-    return socket_.send(TakeoverData::serializeError(protocolVersion_, error));
+    return socket_.send(
+        TakeoverData::serializeError(protocolCapabilities_, error));
   }
   // Socket was closed (likely by a receive timeout above), so don't
   // try to send again in here lest we break; instead just pass up
@@ -234,16 +237,15 @@ Future<Unit> TakeoverServer::ConnHandler::sendTakeoverData(
 
   UnixSocket::Message msg;
   try {
-    msg.data = data.serialize(protocolVersion_);
-    msg.files.push_back(std::move(data.lockFile));
-    msg.files.push_back(std::move(data.thriftSocket));
-    for (auto& mount : data.mountPoints) {
-      msg.files.push_back(std::move(mount.fuseFD));
+    data.serialize(protocolCapabilities_, msg);
+    for (auto& file : msg.files) {
+      XLOG(DBG7) << "sending fd for takeover: " << file.fd();
     }
   } catch (const std::exception& ex) {
     auto ew = folly::exception_wrapper{std::current_exception(), ex};
     data.takeoverComplete.setException(ew);
-    return socket_.send(TakeoverData::serializeError(protocolVersion_, ew));
+    return socket_.send(
+        TakeoverData::serializeError(protocolCapabilities_, ew));
   }
 
   XLOG(INFO) << "Sending takeover data to new process: "

eden/fs/takeover/test/TakeoverTest.cpp

@@ -154,6 +154,32 @@ void checkExpectedFile(int fd, AbsolutePathPiece path) {
 }
 } // namespace
 
+TEST(Takeover, roundTripVersionCapabilities) {
+  for (auto& version : kSupportedTakeoverVersions) {
+    EXPECT_EQ(
+        TakeoverData::capabilitesToVersion(
+            TakeoverData::versionToCapabilites(version)),
+        version);
+  }
+}
+
+TEST(Takeover, unsupportedVersionCapabilites) {
+  EXPECT_EQ(
+      TakeoverData::versionToCapabilites(
+          TakeoverData::kTakeoverProtocolVersionNeverSupported),
+      0);
+
+  EXPECT_EQ(
+      TakeoverData::capabilitesToVersion(0),
+      TakeoverData::kTakeoverProtocolVersionNeverSupported);
+}
+
+TEST(Takeover, invalidComboCapabilites) {
+  EXPECT_THROW(
+      TakeoverData::capabilitesToVersion(TakeoverCapabilities::FUSE),
+      std::runtime_error);
+}
+
 TEST(Takeover, simple) {
   TemporaryDirectory tmpDir("eden_takeover_test");
   AbsolutePathPiece tmpDirPath{tmpDir.path().string()};