From 31c05683ef0560916357d5c37fd5bf7d86bfde68 Mon Sep 17 00:00:00 2001
From: redsolver <info@redsolver.net>
Date: Fri, 28 May 2021 17:26:24 +0200
Subject: [PATCH 01/31] [BREAKING] Added a revision number cache to make
 setJSON safer

---
 src/client.ts |  3 +++
 src/skydb.ts  | 33 ++++++++++++++++++++++-----------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/client.ts b/src/client.ts
index db565f88..13c7bfa3 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -78,6 +78,9 @@ export class SkynetClient {
   // The given portal URL, if one was passed in to `new SkynetClient()`.
   protected givenPortalUrl?: string;
 
+  // Holds the cached revision numbers
+  revisionNumberCache: { [key: string]: bigint } = {};
+
   // Set methods (defined in other files).
 
   // Upload
diff --git a/src/skydb.ts b/src/skydb.ts
index 0b53374c..016bcc52 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -101,6 +101,19 @@ export async function getJSON(
   // Lookup the registry entry.
   const getEntryOpts = extractOptions(opts, defaultGetEntryOptions);
   const { entry }: { entry: RegistryEntry | null } = await this.registry.getEntry(publicKey, dataKey, getEntryOpts);
+
+  const cacheKey = `${publicKey}/${dataKey}`;
+  const newRevision = entry?.revision ?? BigInt(-1);
+
+  if (
+    Object.prototype.hasOwnProperty.call(this.revisionNumberCache, cacheKey) &&
+    this.revisionNumberCache[cacheKey] > newRevision
+  ) {
+    throw new Error("A higher revision number for this userID and path is already cached");
+  }
+
+  this.revisionNumberCache[cacheKey] = newRevision;
+
   if (entry === null || areEqualUint8Arrays(entry.data, EMPTY_SKYLINK)) {
     return { data: null, dataLink: null };
   }
@@ -353,19 +366,17 @@ export async function getOrCreateRegistryEntry(
   const uploadOpts = extractOptions(opts, defaultUploadOptions);
   const skyfilePromise: Promise<UploadRequestResponse> = client.uploadFile(file, uploadOpts);
 
-  // Fetch the current value to find out the revision.
-  //
-  // Start getEntry, do not block.
-  const getEntryOpts = extractOptions(opts, defaultGetEntryOptions);
-  const entryPromise: Promise<SignedRegistryEntry> = client.registry.getEntry(publicKey, dataKey, getEntryOpts);
+  // Block until uploadFile is finished.
+  const [skyfile] = await Promise.all<UploadRequestResponse>([skyfilePromise]);
 
-  // Block until both getEntry and uploadFile are finished.
-  const [signedEntry, skyfile] = await Promise.all<SignedRegistryEntry, UploadRequestResponse>([
-    entryPromise,
-    skyfilePromise,
-  ]);
+  const revision: bigint = (client.revisionNumberCache[`${publicKey}/${dataKey}`] ?? BigInt(-1)) + BigInt(1);
 
-  const revision = getRevisionFromEntry(signedEntry.entry);
+  assertUint64(revision);
+
+  // Throw if the revision is already the maximum value.
+  if (revision > MAX_REVISION) {
+    throw new Error("Current entry already has maximum allowed revision, could not update the entry");
+  }
 
   // Build the registry entry.
   const dataLink = skyfile.skylink;

From e6aebff3cecbbb83bfdf0c96359f741da4e1df74 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 28 May 2021 14:08:30 -0400
Subject: [PATCH 02/31] Small revision cache refactor

---
 src/skydb.ts | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/src/skydb.ts b/src/skydb.ts
index 2b9889c5..d015aa62 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -101,18 +101,20 @@ export async function getJSON(
   const getEntryOpts = extractOptions(opts, defaultGetEntryOptions);
   const { entry }: { entry: RegistryEntry | null } = await this.registry.getEntry(publicKey, dataKey, getEntryOpts);
 
-  const cacheKey = `${publicKey}/${dataKey}`;
-  const newRevision = entry?.revision ?? BigInt(-1);
+  // Calculate the new revision and get the cached revision.
+  const revision = entry?.revision ?? BigInt(-1);
+  const newRevision = incrementRevision(revision);
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  const cachedRevision = this.revisionNumberCache[cacheKey];
 
-  if (
-    Object.prototype.hasOwnProperty.call(this.revisionNumberCache, cacheKey) &&
-    this.revisionNumberCache[cacheKey] > newRevision
-  ) {
+  if (cachedRevision && cachedRevision > newRevision) {
     throw new Error("A higher revision number for this userID and path is already cached");
   }
 
+  // Update the cached revision.
   this.revisionNumberCache[cacheKey] = newRevision;
 
+  // Return null if the entry was not found or if it contained a sentinel value indicating deletion.
   if (entry === null || areEqualUint8Arrays(entry.data, EMPTY_SKYLINK)) {
     return { data: null, dataLink: null };
   }
@@ -359,21 +361,13 @@ export async function getOrCreateRegistryEntry(
   }
   const file = new File([JSON.stringify(fullData)], `dk:${dataKeyHex}`, { type: "application/json" });
 
-  // Start file upload, do not block.
+  // Do file upload.
   const uploadOpts = extractOptions(opts, defaultUploadOptions);
-  const skyfilePromise: Promise<UploadRequestResponse> = client.uploadFile(file, uploadOpts);
+  const skyfile = await client.uploadFile(file, uploadOpts);
 
-  // Block until uploadFile is finished.
-  const [skyfile] = await Promise.all<UploadRequestResponse>([skyfilePromise]);
-
-  const revision: bigint = (client.revisionNumberCache[`${publicKey}/${dataKey}`] ?? BigInt(-1)) + BigInt(1);
-
-  assertUint64(revision);
-
-  // Throw if the revision is already the maximum value.
-  if (revision > MAX_REVISION) {
-    throw new Error("Current entry already has maximum allowed revision, could not update the entry");
-  }
+  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
+  let revision: bigint = client.revisionNumberCache[getCacheKey(publicKey, dataKey)] ?? BigInt(-1);
+  revision = incrementRevision(revision);
 
   // Build the registry entry.
   const dataLink = trimUriPrefix(skyfile.skylink, uriSkynetPrefix);
@@ -387,14 +381,22 @@ export async function getOrCreateRegistryEntry(
   return [entry, formatSkylink(dataLink)];
 }
 
+function getCacheKey(publicKey: string, dataKey: string): string {
+  return `${publicKey}/${dataKey}`;
+}
+
 export function getRevisionFromEntry(entry: RegistryEntry | null): bigint {
-  let revision: bigint;
-  if (entry === null) {
-    revision = BigInt(0);
-  } else {
-    revision = entry.revision + BigInt(1);
+  let revision = BigInt(-1);
+  if (entry !== null) {
+    revision = entry.revision;
   }
 
+  return incrementRevision(revision);
+}
+
+function incrementRevision(revision: bigint): bigint {
+  revision = revision + BigInt(1);
+
   // Throw if the revision is already the maximum value.
   if (revision > MAX_REVISION) {
     throw new Error("Current entry already has maximum allowed revision, could not update the entry");

From 64bec9e157102971b6968f7bc4c94c9581f3e75c Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 13 Oct 2021 20:23:50 -0500
Subject: [PATCH 03/31] Address review comments

---
 src/skydb.ts | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/skydb.ts b/src/skydb.ts
index d6a4cffc..7a1ef51b 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -49,6 +49,8 @@ export const DELETION_ENTRY_DATA = new Uint8Array(RAW_SKYLINK_SIZE);
 
 const JSON_RESPONSE_VERSION = 2;
 
+const UNCACHED_REVISION_NUMBER = BigInt(-1);
+
 /**
  * Custom get JSON options. Includes the options for get entry, to get the
  * skylink; and download, to download the file from the skylink.
@@ -151,12 +153,12 @@ export async function getJSON(
   const entry: RegistryEntry | null = await getSkyDBRegistryEntry(this, publicKey, dataKey, getEntryOpts);
 
   // Calculate the new revision and get the cached revision.
-  const revision = entry?.revision ?? BigInt(-1);
+  const revision = entry?.revision ?? UNCACHED_REVISION_NUMBER;
   const newRevision = incrementRevision(revision);
   const cacheKey = getCacheKey(publicKey, dataKey);
   const cachedRevision = this.revisionNumberCache[cacheKey];
 
-  if (cachedRevision && cachedRevision > newRevision) {
+  if (cachedRevision && cachedRevision >= newRevision) {
     throw new Error("A higher revision number for this userID and path is already cached");
   }
 
@@ -598,7 +600,8 @@ export async function getOrCreateRegistryEntry(
   const skyfile = await client.uploadFile(file, uploadOpts);
 
   // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
-  let revision: bigint = client.revisionNumberCache[getCacheKey(publicKey, dataKey)] ?? BigInt(-1);
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  let revision: bigint = client.revisionNumberCache[cacheKey] ?? UNCACHED_REVISION_NUMBER;
   revision = incrementRevision(revision);
 
   // Build the registry entry.
@@ -613,12 +616,20 @@ export async function getOrCreateRegistryEntry(
   return [entry, formatSkylink(dataLink)];
 }
 
+/**
+ * Gets the revision cache key for the given public key and data key.
+ *
+ * @param publicKey - The given public key.
+ * @param dataKey - The given data key.
+ * @returns - The revision cache key.
+ */
 function getCacheKey(publicKey: string, dataKey: string): string {
   return `${publicKey}/${dataKey}`;
 }
 
 /**
- * Gets the next revision from a returned entry (or 0 if the entry was not found).
+ * Gets the next revision from a returned entry (or 0 if the entry was not
+ * found).
  *
  * @param entry - The returned registry entry.
  * @returns - The revision.
@@ -631,6 +642,14 @@ export function getNextRevisionFromEntry(entry: RegistryEntry | null): bigint {
   return incrementRevision(entry.revision);
 }
 
+/**
+ * Increments the given revision number and checks to make sure it is not
+ * greater than the maximum revision.
+ *
+ * @param revision - The given revision number.
+ * @returns - The incremented revision number.
+ * @throws - Will throw if the incremented revision number is greater than the maximum revision.
+ */
 function incrementRevision(revision: bigint): bigint {
   revision = revision + BigInt(1);
 
@@ -685,7 +704,7 @@ export function validateEntryData(data: Uint8Array, allowDeletionEntryData: bool
 }
 
 /**
- * Gets the registry entry, returning null if the entry contains an empty skylink (the deletion sentinel).
+ * Gets the registry entry, returning null if the entry was not found or if it contained a sentinel value indicating deletion.
  *
  * @param client - The Skynet Client
  * @param publicKey - The user public key.
@@ -700,6 +719,8 @@ async function getSkyDBRegistryEntry(
   opts: CustomGetEntryOptions
 ): Promise<RegistryEntry | null> {
   const { entry } = await client.registry.getEntry(publicKey, dataKey, opts);
+
+  // Return null if the entry was not found or if it contained a sentinel value indicating deletion.
   if (entry === null || areEqualUint8Arrays(entry.data, EMPTY_SKYLINK)) {
     return null;
   }

From 1c7553f0e1056c9b769a8b0dc95b8d048f1bcc7f Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Tue, 2 Nov 2021 18:04:12 -0500
Subject: [PATCH 04/31] Finish up implementation for registry entry revision
 number cache

- Refactor SkyDB helpers to use revision cache and rename them
- Make all SkyDB methods use revision cache
- Make MySky DB methods call SkyDB methods or helpers
- Update SkyDB tests
---
 src/mysky/index.ts |  26 +++--
 src/skydb.test.ts  |  33 ++-----
 src/skydb.ts       | 237 ++++++++++++++++++++++-----------------------
 3 files changed, 146 insertions(+), 150 deletions(-)

diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 48ffdad3..74e60d0b 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -29,10 +29,9 @@ import {
   DEFAULT_SET_JSON_OPTIONS,
   CustomGetJSONOptions,
   CustomSetJSONOptions,
-  getOrCreateRegistryEntry,
+  getOrCreateRegistryEntryFromCache,
   JSONResponse,
-  getNextRegistryEntry,
-  getOrCreateRawBytesRegistryEntry,
+  getNextRegistryEntryFromCache,
   validateEntryData,
   CustomSetEntryDataOptions,
   DEFAULT_SET_ENTRY_DATA_OPTIONS,
@@ -395,7 +394,16 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    const [entry, dataLink] = await getOrCreateRegistryEntry(this.connector.client, publicKey, dataKey, json, opts);
+    // Call SkyDB helper to create the registry entry. We can't call SkyDB's
+    // setJSON here directly because we need MySky to sign the entry, instead of
+    // signing it ourselves with a given private key.
+    const [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
+      this.connector.client,
+      publicKey,
+      dataKey,
+      json,
+      opts
+    );
 
     const signature = await this.signRegistryEntry(entry, path);
 
@@ -424,6 +432,8 @@ export class MySky {
       ...customOptions,
     };
 
+    // We re-implement deleteJSON instead of calling SkyDB's deleteJSON so that
+    // MySky can do the signing.
     await this.setEntryData(path, DELETION_ENTRY_DATA, { ...opts, allowDeletionEntryData: true });
   }
 
@@ -493,14 +503,16 @@ export class MySky {
       ...customOptions,
     };
 
+    // We re-implement setEntryData instead of calling SkyDB's setEntryData so
+    // that MySky can do the signing.
+
     validateEntryData(data, opts.allowDeletionEntryData);
 
     const publicKey = await this.userID();
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-    const entry = await getNextRegistryEntry(this.connector.client, publicKey, dataKey, data, getEntryOpts);
+    const entry = await getNextRegistryEntryFromCache(this.connector.client, publicKey, dataKey, data);
 
     const signature = await this.signRegistryEntry(entry, path);
 
@@ -629,7 +641,7 @@ export class MySky {
     // Pad and encrypt json file.
     const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
 
-    const entry = await getOrCreateRawBytesRegistryEntry(this.connector.client, publicKey, dataKey, data, opts);
+    const [entry] = await getOrCreateRegistryEntryFromCache(this.connector.client, publicKey, dataKey, data, opts);
 
     // Call MySky which checks for write permissions on the path.
     const signature = await this.signEncryptedRegistryEntry(entry, path);
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 9e257702..3ebafb33 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -5,8 +5,8 @@ import { getSkylinkUrlForPortal } from "./download";
 import { MAX_REVISION } from "./utils/number";
 import { DEFAULT_SKYNET_PORTAL_URL, URI_SKYNET_PREFIX } from "./utils/url";
 import { SkynetClient } from "./index";
-import { getEntryUrlForPortal, REGEX_REVISION_NO_QUOTES } from "./registry";
-import { checkCachedDataLink, DELETION_ENTRY_DATA } from "./skydb";
+import { getEntryUrlForPortal } from "./registry";
+import { checkCachedDataLink, DELETION_ENTRY_DATA, getCacheKey } from "./skydb";
 import { MAX_ENTRY_LENGTH } from "./mysky";
 
 // Generated with genKeyPairFromSeed("insecure test seed")
@@ -160,9 +160,6 @@ describe("setJSON", () => {
   });
 
   it("should perform an upload, lookup and registry update", async () => {
-    // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
-
     // mock a successful registry update
     mock.onPost(registryUrl).replyOnce(204);
 
@@ -172,25 +169,23 @@ describe("setJSON", () => {
     expect(returnedSkylink).toEqual(sialink);
 
     // assert our request history contains the expected amount of requests
-    expect(mock.history.get.length).toBe(1);
+    expect(mock.history.get.length).toBe(0);
     expect(mock.history.post.length).toBe(2);
 
     const data = JSON.parse(mock.history.post[1].data);
     expect(data).toBeDefined();
-    expect(data.revision).toEqual(revision + 1);
+    expect(data.revision).toBeGreaterThanOrEqual(revision + 1);
   });
 
-  it("should use a revision number of 0 if the lookup failed", async () => {
-    mock.onGet(registryLookupUrl).reply(404);
-
+  it("should use a revision number of 0 if the entry is not cached", async () => {
     // mock a successful registry update
     mock.onPost(registryUrl).reply(204);
 
     // call `setJSON` on the client
-    await client.db.setJSON(privateKey, dataKey, jsonData);
+    await client.db.setJSON(privateKey, "inexistent entry", jsonData);
 
     // assert our request history contains the expected amount of requests
-    expect(mock.history.get.length).toBe(1);
+    expect(mock.history.get.length).toBe(0);
     expect(mock.history.post.length).toBe(2);
 
     const data = JSON.parse(mock.history.post[1].data);
@@ -199,17 +194,9 @@ describe("setJSON", () => {
   });
 
   it("should fail if the entry has the maximum allowed revision", async () => {
-    // mock a successful registry lookup
-    const entryData = {
-      data,
-      // String the bigint since JS doesn't support 64-bit numbers.
-      revision: MAX_REVISION.toString(),
-      signature:
-        "18c76e88141c7cc76d8a77abcd91b5d64d8fc3833eae407ab8a5339e5fcf7940e3fa5830a8ad9439a0c0cc72236ed7b096ae05772f81eee120cbd173bfd6600e",
-    };
-    // Replace the quotes around the stringed bigint.
-    const json = JSON.stringify(entryData).replace(REGEX_REVISION_NO_QUOTES, '"revision":"$1"');
-    mock.onGet(registryLookupUrl).reply(200, json);
+    const dataKey = "maximum revision";
+    const cacheKey = getCacheKey(publicKey, dataKey);
+    client.revisionNumberCache[cacheKey] = MAX_REVISION;
 
     // mock a successful registry update
     mock.onPost(registryUrl).reply(204);
diff --git a/src/skydb.ts b/src/skydb.ts
index 7a1ef51b..ea9dc7ac 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -7,7 +7,6 @@ import {
   DEFAULT_SET_ENTRY_OPTIONS,
   CustomGetEntryOptions,
   RegistryEntry,
-  SignedRegistryEntry,
   CustomSetEntryOptions,
   validatePublicKey,
 } from "./registry";
@@ -22,7 +21,7 @@ import {
   uint8ArrayToStringUtf8,
 } from "./utils/string";
 import { formatSkylink } from "./skylink/format";
-import { DEFAULT_UPLOAD_OPTIONS, CustomUploadOptions, UploadRequestResponse } from "./upload";
+import { DEFAULT_UPLOAD_OPTIONS, CustomUploadOptions } from "./upload";
 import { areEqualUint8Arrays } from "./utils/array";
 import { decodeSkylinkBase64, encodeSkylinkBase64 } from "./utils/encoding";
 import { DEFAULT_BASE_OPTIONS, extractOptions } from "./utils/options";
@@ -123,7 +122,21 @@ export type RawBytesResponse = {
 // ====
 
 /**
- * Gets the JSON object corresponding to the publicKey and dataKey.
+ * Gets the JSON object corresponding to the publicKey and dataKey. If the data
+ * was found, we update the cached revision number for the entry.
+ *
+ * NOTE: The cached revision number will be updated only if the data is found
+ * (including deleted data). If there is a 404 or the entry contains deleted
+ * data, null will be returned. If there is an error, the error is returned
+ * without updating the cached revision number.
+ *
+ * Summary:
+ *   - Data found: update cached revision
+ *   - Parse error: don't update cached revision
+ *   - Network error: don't update cached revision
+ *   - Too high version error: don't update the cached revision
+ *   - 404 (data not found): don't update the cached revision
+ *   - Data deleted: update cached revision
  *
  * @param this - SkynetClient
  * @param publicKey - The user public key.
@@ -150,22 +163,7 @@ export async function getJSON(
 
   // Lookup the registry entry.
   const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entry: RegistryEntry | null = await getSkyDBRegistryEntry(this, publicKey, dataKey, getEntryOpts);
-
-  // Calculate the new revision and get the cached revision.
-  const revision = entry?.revision ?? UNCACHED_REVISION_NUMBER;
-  const newRevision = incrementRevision(revision);
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  const cachedRevision = this.revisionNumberCache[cacheKey];
-
-  if (cachedRevision && cachedRevision >= newRevision) {
-    throw new Error("A higher revision number for this userID and path is already cached");
-  }
-
-  // Update the cached revision.
-  this.revisionNumberCache[cacheKey] = newRevision;
-
-  // Return null if the entry was not found or if it contained a sentinel value indicating deletion.
+  const entry: RegistryEntry | null = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, getEntryOpts);
   if (entry === null) {
     return { data: null, dataLink: null };
   }
@@ -200,7 +198,11 @@ export async function getJSON(
 }
 
 /**
- * Sets a JSON object at the registry entry corresponding to the publicKey and dataKey.
+ * Sets a JSON object at the registry entry corresponding to the publicKey and
+ * dataKey.
+ *
+ * This will use the entry revision number from the cache, so getJSON must
+ * always be called first for existing entries.
  *
  * @param this - SkynetClient
  * @param privateKey - The user private key.
@@ -230,7 +232,13 @@ export async function setJSON(
 
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
 
-  const [entry, dataLink] = await getOrCreateRegistryEntry(this, toHexString(publicKeyArray), dataKey, json, opts);
+  const [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
+    this,
+    toHexString(publicKeyArray),
+    dataKey,
+    json,
+    opts
+  );
 
   // Update the registry.
   const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
@@ -240,7 +248,11 @@ export async function setJSON(
 }
 
 /**
- * Deletes a JSON object at the registry entry corresponding to the publicKey and dataKey.
+ * Deletes a JSON object at the registry entry corresponding to the publicKey
+ * and dataKey.
+ *
+ * This will use the entry revision number from the cache, so getJSON must
+ * always be called first.
  *
  * @param this - SkynetClient
  * @param privateKey - The user private key.
@@ -297,6 +309,9 @@ export async function setDataLink(
 /**
  * Gets the raw registry entry data at the given public key and data key.
  *
+ * If the data was found, we update the cached revision number for the entry.
+ * See getJSON for behavior in other cases.
+ *
  * @param this - SkynetClient
  * @param publicKey - The user public key.
  * @param dataKey - The data key.
@@ -319,7 +334,7 @@ export async function getEntryData(
     ...customOptions,
   };
 
-  const entry = await getSkyDBRegistryEntry(this, publicKey, dataKey, opts);
+  const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, opts);
   if (entry === null) {
     return { data: null };
   }
@@ -329,6 +344,9 @@ export async function getEntryData(
 /**
  * Sets the raw entry data at the given private key and data key.
  *
+ * This will use the entry revision number from the cache, so getEntryData must
+ * always be called first for existing entries.
+ *
  * @param this - SkynetClient
  * @param privateKey - The user private key.
  * @param dataKey - The data key.
@@ -359,8 +377,7 @@ export async function setEntryData(
 
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
 
-  const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entry = await getNextRegistryEntry(this, toHexString(publicKeyArray), dataKey, data, getEntryOpts);
+  const entry = await getNextRegistryEntryFromCache(this, toHexString(publicKeyArray), dataKey, data);
 
   const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
   await this.registry.setEntry(privateKey, entry, setEntryOpts);
@@ -372,6 +389,9 @@ export async function setEntryData(
  * Deletes the entry data at the given private key and data key. Trying to
  * access the data again with e.g. getEntryData will result in null.
  *
+ * This will use the entry revision number from the cache, so getEntryData must
+ * always be called first.
+ *
  * @param this - SkynetClient
  * @param privateKey - The user private key.
  * @param dataKey - The data key.
@@ -399,6 +419,9 @@ export async function deleteEntryData(
 /**
  * Gets the raw bytes corresponding to the publicKey and dataKey. The caller is responsible for setting any metadata in the bytes.
  *
+ * If the data was found, we update the cached revision number for the entry.
+ * See getJSON for behavior in other cases.
+ *
  * @param this - SkynetClient
  * @param publicKey - The user public key.
  * @param dataKey - The key of the data to fetch for the given user.
@@ -425,7 +448,7 @@ export async function getRawBytes(
 
   // Lookup the registry entry.
   const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entry = await getSkyDBRegistryEntry(this, publicKey, dataKey, getEntryOpts);
+  const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, getEntryOpts);
   if (entry === null) {
     return { data: null, dataLink: null };
   }
@@ -449,71 +472,6 @@ export async function getRawBytes(
   return { data: new Uint8Array(buffer), dataLink };
 }
 
-/* istanbul ignore next */
-/**
- * Gets the registry entry for the given raw bytes or creates the entry if it doesn't exist.
- *
- * @param client - The Skynet client.
- * @param publicKey - The user public key.
- * @param dataKey - The dat akey.
- * @param data - The raw byte data to set.
- * @param [customOptions] - Additional settings that can optionally be set.
- * @returns - The registry entry and corresponding data link.
- * @throws - Will throw if the revision is already the maximum value.
- */
-// TODO: Rename & refactor after the SkyDB caching refactor.
-export async function getOrCreateRawBytesRegistryEntry(
-  client: SkynetClient,
-  publicKey: string,
-  dataKey: string,
-  data: Uint8Array,
-  customOptions?: CustomSetJSONOptions
-): Promise<RegistryEntry> {
-  // Not publicly available, don't validate input.
-
-  const opts = {
-    ...DEFAULT_SET_JSON_OPTIONS,
-    ...client.customOptions,
-    ...customOptions,
-  };
-
-  // Create the data to upload to acquire its skylink.
-  let dataKeyHex = dataKey;
-  if (!opts.hashedDataKeyHex) {
-    dataKeyHex = toHexString(stringToUint8ArrayUtf8(dataKey));
-  }
-  const file = new File([data], `dk:${dataKeyHex}`, { type: "application/octet-stream" });
-
-  // Start file upload, do not block.
-  const uploadOpts = extractOptions(opts, DEFAULT_UPLOAD_OPTIONS);
-  const skyfilePromise: Promise<UploadRequestResponse> = client.uploadFile(file, uploadOpts);
-
-  // Fetch the current value to find out the revision.
-  //
-  // Start getEntry, do not block.
-  const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entryPromise: Promise<SignedRegistryEntry> = client.registry.getEntry(publicKey, dataKey, getEntryOpts);
-
-  // Block until both getEntry and uploadFile are finished.
-  const [signedEntry, skyfile] = await Promise.all<SignedRegistryEntry, UploadRequestResponse>([
-    entryPromise,
-    skyfilePromise,
-  ]);
-
-  const revision = getNextRevisionFromEntry(signedEntry.entry);
-
-  // Build the registry entry.
-  const dataLink = trimUriPrefix(skyfile.skylink, URI_SKYNET_PREFIX);
-  const rawDataLink = decodeSkylinkBase64(dataLink);
-  validateUint8ArrayLen("rawDataLink", rawDataLink, "skylink byte array", RAW_SKYLINK_SIZE);
-  const entry: RegistryEntry = {
-    dataKey,
-    data: rawDataLink,
-    revision,
-  };
-  return entry;
-}
-
 // =======
 // Helpers
 // =======
@@ -525,29 +483,21 @@ export async function getOrCreateRawBytesRegistryEntry(
  * @param publicKey - The user public key.
  * @param dataKey - The dat akey.
  * @param data - The data to set.
- * @param [customOptions] - Additional settings that can optionally be set.
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
  */
-export async function getNextRegistryEntry(
+export async function getNextRegistryEntryFromCache(
   client: SkynetClient,
   publicKey: string,
   dataKey: string,
-  data: Uint8Array,
-  customOptions?: CustomGetEntryOptions
+  data: Uint8Array
 ): Promise<RegistryEntry> {
   // Not publicly available, don't validate input.
 
-  const opts = {
-    ...DEFAULT_GET_ENTRY_OPTIONS,
-    ...client.customOptions,
-    ...customOptions,
-  };
-
-  // Get the latest entry.
-  // TODO: Can remove this once we start caching the latest revision.
-  const signedEntry = await client.registry.getEntry(publicKey, dataKey, opts);
-  const revision = getNextRevisionFromEntry(signedEntry.entry);
+  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  let revision: bigint = client.revisionNumberCache[cacheKey] ?? UNCACHED_REVISION_NUMBER;
+  revision = incrementRevision(revision);
 
   // Build the registry entry.
   const entry: RegistryEntry = {
@@ -560,21 +510,23 @@ export async function getNextRegistryEntry(
 }
 
 /**
- * Gets the registry entry and data link or creates the entry if it doesn't exist.
+ * Gets the registry entry and data link or creates the entry if it doesn't
+ * exist. Uses the cached revision number for the entry, or 0 if the entry has
+ * not been cached.
  *
  * @param client - The Skynet client.
  * @param publicKey - The user public key.
  * @param dataKey - The dat akey.
- * @param json - The JSON to set.
+ * @param data - The JSON or raw byte data to set.
  * @param [customOptions] - Additional settings that can optionally be set.
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
  */
-export async function getOrCreateRegistryEntry(
+export async function getOrCreateRegistryEntryFromCache(
   client: SkynetClient,
   publicKey: string,
   dataKey: string,
-  json: JsonData,
+  data: JsonData | Uint8Array,
   customOptions?: CustomSetJSONOptions
 ): Promise<[RegistryEntry, string]> {
   // Not publicly available, don't validate input.
@@ -585,15 +537,21 @@ export async function getOrCreateRegistryEntry(
     ...customOptions,
   };
 
-  // Set the hidden _data and _v fields.
-  const fullData: JsonFullData = { _data: json, _v: JSON_RESPONSE_VERSION };
+  let fullData: string | Uint8Array;
+  if (!(data instanceof Uint8Array)) {
+    // Set the hidden _data and _v fields.
+    const jsonFullData: JsonFullData = { _data: data, _v: JSON_RESPONSE_VERSION };
+    fullData = JSON.stringify(jsonFullData);
+  } else {
+    fullData = data;
+  }
 
   // Create the data to upload to acquire its skylink.
   let dataKeyHex = dataKey;
   if (!opts.hashedDataKeyHex) {
     dataKeyHex = toHexString(stringToUint8ArrayUtf8(dataKey));
   }
-  const file = new File([JSON.stringify(fullData)], `dk:${dataKeyHex}`, { type: "application/json" });
+  const file = new File([fullData], `dk:${dataKeyHex}`, { type: "application/json" });
 
   // Do file upload.
   const uploadOpts = extractOptions(opts, DEFAULT_UPLOAD_OPTIONS);
@@ -606,11 +564,11 @@ export async function getOrCreateRegistryEntry(
 
   // Build the registry entry.
   const dataLink = trimUriPrefix(skyfile.skylink, URI_SKYNET_PREFIX);
-  const data = decodeSkylinkBase64(dataLink);
-  validateUint8ArrayLen("data", data, "skylink byte array", RAW_SKYLINK_SIZE);
+  const rawDataLink = decodeSkylinkBase64(dataLink);
+  validateUint8ArrayLen("rawDataLink", rawDataLink, "skylink byte array", RAW_SKYLINK_SIZE);
   const entry: RegistryEntry = {
     dataKey,
-    data,
+    data: rawDataLink,
     revision,
   };
   return [entry, formatSkylink(dataLink)];
@@ -623,7 +581,7 @@ export async function getOrCreateRegistryEntry(
  * @param dataKey - The given data key.
  * @returns - The revision cache key.
  */
-function getCacheKey(publicKey: string, dataKey: string): string {
+export function getCacheKey(publicKey: string, dataKey: string): string {
   return `${publicKey}/${dataKey}`;
 }
 
@@ -704,7 +662,11 @@ export function validateEntryData(data: Uint8Array, allowDeletionEntryData: bool
 }
 
 /**
- * Gets the registry entry, returning null if the entry was not found or if it contained a sentinel value indicating deletion.
+ * Gets the registry entry, returning null if the entry was not found or if it
+ * contained a sentinel value indicating deletion.
+ *
+ * If the data was found, we update the cached revision number for the entry.
+ * See getJSON for behavior in other cases.
  *
  * @param client - The Skynet Client
  * @param publicKey - The user public key.
@@ -712,21 +674,56 @@ export function validateEntryData(data: Uint8Array, allowDeletionEntryData: bool
  * @param opts - Additional settings.
  * @returns - The registry entry, or null if not found or deleted.
  */
-async function getSkyDBRegistryEntry(
+async function getSkyDBRegistryEntryAndUpdateCache(
   client: SkynetClient,
   publicKey: string,
   dataKey: string,
   opts: CustomGetEntryOptions
 ): Promise<RegistryEntry | null> {
+  // Calculate the cached revision number before doing anything else.
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  const cachedRevision = client.revisionNumberCache[cacheKey];
+
+  // If this throws due to a parse error or network error, exit early and do not
+  // update the cached revision number.
   const { entry } = await client.registry.getEntry(publicKey, dataKey, opts);
 
-  // Return null if the entry was not found or if it contained a sentinel value indicating deletion.
-  if (entry === null || areEqualUint8Arrays(entry.data, EMPTY_SKYLINK)) {
+  // Don't update the cached revision number if the data was not found (404). Return null.
+  if (entry === null) {
     return null;
   }
+
+  // Calculate the new revision and get the cached revision.
+  const revision = entry?.revision ?? UNCACHED_REVISION_NUMBER;
+  const newRevision = incrementRevision(revision);
+
+  // Don't update the cached revision number if the received version is too low. Throw error.
+  if (cachedRevision && cachedRevision > newRevision) {
+    throw new Error("A higher revision number for this userID and path is already cached");
+  }
+
+  // Update the cached revision.
+  client.revisionNumberCache[cacheKey] = newRevision;
+
+  // Return null if the entry contained a sentinel value indicating deletion.
+  // We do this after updating the revision number cache.
+  if (entry !== null && wasRegistryEntryDeleted(entry)) {
+    return null;
+  }
+
   return entry;
 }
 
+/**
+ * Returns whether the given registry entry indicates a past deletion.
+ *
+ * @param entry - The registry entry.
+ * @returns - Whether the registry entry data indicated a past deletion.
+ */
+function wasRegistryEntryDeleted(entry: RegistryEntry): boolean {
+  return areEqualUint8Arrays(entry.data, EMPTY_SKYLINK);
+}
+
 /**
  * Parses a data link out of the given registry entry data.
  *

From 1fe1d9a2e1178f901bce7fcab77e0534e544d34f Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 3 Nov 2021 14:04:23 -0500
Subject: [PATCH 05/31] Fix data race bug and add data race integration test

---
 integration/registry.test.ts | 16 +++++++
 integration/skydb.test.ts    | 39 ++++++++++++++++
 src/mysky/index.ts           | 65 ++++++++++++++++++---------
 src/skydb.ts                 | 87 +++++++++++++++++++++++++-----------
 4 files changed, 159 insertions(+), 48 deletions(-)

diff --git a/integration/registry.test.ts b/integration/registry.test.ts
index 8f1be556..cb8129cd 100644
--- a/integration/registry.test.ts
+++ b/integration/registry.test.ts
@@ -65,4 +65,20 @@ describe(`Registry end to end integration tests for portal '${portal}'`, () => {
 
     expect(returnedEntry).toEqual(entry);
   });
+
+  it("Should fail to set an entry with a revision number that's too low", async () => {
+    const { privateKey } = genKeyPairAndSeed();
+
+    const entry = {
+      dataKey,
+      data: new Uint8Array(),
+      revision: BigInt(1),
+    };
+
+    await client.registry.setEntry(privateKey, entry);
+    entry.revision--;
+    await expect(client.registry.setEntry(privateKey, entry)).rejects.toThrowError(
+      "Request failed with status code 500"
+    );
+  });
 });
diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 82c5ceea..78f107a0 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -1,4 +1,5 @@
 import { AxiosError } from "axios";
+
 import { client, dataKey, portal } from ".";
 import { genKeyPairAndSeed, getEntryLink, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
@@ -240,4 +241,42 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
 
     expect(data).toEqual(json);
   });
+
+  // REGRESSION TEST: By creating a gap between setJSON and getJSON, a user
+  // could call getJSON, get outdated data, then call setJSON, and overwrite
+  // more up to date data with outdated data, but still use a high enough
+  // revision number.
+  //
+  // The fix is that you cannot retrieve the revision number while calling
+  // setJSON. You have to use the same revision number that you had when you
+  // called getJSON
+  it("Should avoid data race bugs where getJSON is not called", async () => {
+    const { publicKey, privateKey } = genKeyPairAndSeed();
+    const json1 = { message: 1 };
+    const json2 = { message: 2 };
+
+    // Set the data.
+    await client.db.setJSON(privateKey, dataKey, json1);
+
+    // Try to invoke the data race.
+    let receivedJson;
+    try {
+      // Get the data while also calling setJSON.
+      [{ data: receivedJson }] = await Promise.all([
+        client.db.getJSON(publicKey, dataKey),
+        client.db.setJSON(privateKey, dataKey, json2),
+      ]);
+    } catch (e) {
+      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+        // The data race condition has been prevented and we received the expected error. Return from test early.
+        return;
+      }
+
+      // Unexpected error, throw.
+      throw e;
+    }
+
+    // Data race did not occur, getJSON should have latest JSON.
+    expect(receivedJson).toEqual(json2);
+  });
 });
diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 74e60d0b..6edc30de 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -36,6 +36,8 @@ import {
   CustomSetEntryDataOptions,
   DEFAULT_SET_ENTRY_DATA_OPTIONS,
   DELETION_ENTRY_DATA,
+  incrementCachedRevision,
+  decrementCachedRevision,
 } from "../skydb";
 import { Signature } from "../crypto";
 import { deriveDiscoverableFileTweak } from "./tweak";
@@ -394,21 +396,31 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Call SkyDB helper to create the registry entry. We can't call SkyDB's
-    // setJSON here directly because we need MySky to sign the entry, instead of
-    // signing it ourselves with a given private key.
-    const [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
-      this.connector.client,
-      publicKey,
-      dataKey,
-      json,
-      opts
-    );
+    // Get the cached revision number before doing anything else.
+    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
+
+    let entry, dataLink;
+    try {
+      // Call SkyDB helper to create the registry entry. We can't call SkyDB's
+      // setJSON here directly because we need MySky to sign the entry, instead of
+      // signing it ourselves with a given private key.
+      [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
+        this.connector.client,
+        dataKey,
+        json,
+        newRevision,
+        opts
+      );
 
-    const signature = await this.signRegistryEntry(entry, path);
+      const signature = await this.signRegistryEntry(entry, path);
 
-    const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-    await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+    } catch (e) {
+      // Something failed, revert the cached revision number increment.
+      decrementCachedRevision(this.connector.client, publicKey, dataKey);
+      throw e;
+    }
 
     return { data: json, dataLink };
   }
@@ -636,18 +648,29 @@ export class MySky {
     const [publicKey, pathSeed] = await Promise.all([this.userID(), this.getEncryptedPathSeed(path, false)]);
     const dataKey = deriveEncryptedFileTweak(pathSeed);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
-    const encryptionKey = deriveEncryptedFileKeyEntropy(pathSeed);
 
-    // Pad and encrypt json file.
-    const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
+    // Get the cached revision number before doing anything else.
+    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
 
-    const [entry] = await getOrCreateRegistryEntryFromCache(this.connector.client, publicKey, dataKey, data, opts);
+    try {
+      // Derive the key.
+      const encryptionKey = deriveEncryptedFileKeyEntropy(pathSeed);
 
-    // Call MySky which checks for write permissions on the path.
-    const signature = await this.signEncryptedRegistryEntry(entry, path);
+      // Pad and encrypt json file.
+      const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
 
-    const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-    await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+      const [entry] = await getOrCreateRegistryEntryFromCache(this.connector.client, dataKey, data, newRevision, opts);
+
+      // Call MySky which checks for write permissions on the path.
+      const signature = await this.signEncryptedRegistryEntry(entry, path);
+
+      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+    } catch (e) {
+      // Something failed, revert the cached revision number increment.
+      decrementCachedRevision(this.connector.client, publicKey, dataKey);
+      throw e;
+    }
 
     return { data: json };
   }
diff --git a/src/skydb.ts b/src/skydb.ts
index ea9dc7ac..bcf204d1 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -231,18 +231,23 @@ export async function setJSON(
   };
 
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
-
-  const [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
-    this,
-    toHexString(publicKeyArray),
-    dataKey,
-    json,
-    opts
-  );
-
-  // Update the registry.
-  const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-  await this.registry.setEntry(privateKey, entry, setEntryOpts);
+  const publicKey = toHexString(publicKeyArray);
+
+  // Get the cached revision number before doing anything else.
+  const newRevision = incrementCachedRevision(this, publicKey, dataKey);
+
+  let entry, dataLink;
+  try {
+    [entry, dataLink] = await getOrCreateRegistryEntryFromCache(this, dataKey, json, newRevision, opts);
+
+    // Update the registry.
+    const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+    await this.registry.setEntry(privateKey, entry, setEntryOpts);
+  } catch (e) {
+    // Something failed, revert the cached revision number increment.
+    decrementCachedRevision(this, publicKey, dataKey);
+    throw e;
+  }
 
   return { data: json, dataLink: formatSkylink(dataLink) };
 }
@@ -481,7 +486,7 @@ export async function getRawBytes(
  *
  * @param client - The Skynet client.
  * @param publicKey - The user public key.
- * @param dataKey - The dat akey.
+ * @param dataKey - The data key.
  * @param data - The data to set.
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
@@ -515,18 +520,18 @@ export async function getNextRegistryEntryFromCache(
  * not been cached.
  *
  * @param client - The Skynet client.
- * @param publicKey - The user public key.
- * @param dataKey - The dat akey.
+ * @param dataKey - The data key.
  * @param data - The JSON or raw byte data to set.
+ * @param revision - The revision number to set.
  * @param [customOptions] - Additional settings that can optionally be set.
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
  */
 export async function getOrCreateRegistryEntryFromCache(
   client: SkynetClient,
-  publicKey: string,
   dataKey: string,
   data: JsonData | Uint8Array,
+  revision: bigint,
   customOptions?: CustomSetJSONOptions
 ): Promise<[RegistryEntry, string]> {
   // Not publicly available, don't validate input.
@@ -557,11 +562,6 @@ export async function getOrCreateRegistryEntryFromCache(
   const uploadOpts = extractOptions(opts, DEFAULT_UPLOAD_OPTIONS);
   const skyfile = await client.uploadFile(file, uploadOpts);
 
-  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  let revision: bigint = client.revisionNumberCache[cacheKey] ?? UNCACHED_REVISION_NUMBER;
-  revision = incrementRevision(revision);
-
   // Build the registry entry.
   const dataLink = trimUriPrefix(skyfile.skylink, URI_SKYNET_PREFIX);
   const rawDataLink = decodeSkylinkBase64(dataLink);
@@ -600,6 +600,42 @@ export function getNextRevisionFromEntry(entry: RegistryEntry | null): bigint {
   return incrementRevision(entry.revision);
 }
 
+/**
+ * Decrements the revision number in the cache for the given entry.
+ *
+ * @param client - The Skynet client.
+ * @param publicKey - The user public key.
+ * @param dataKey - The data key.
+ */
+export function decrementCachedRevision(client: SkynetClient, publicKey: string, dataKey: string): void {
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  client.revisionNumberCache[cacheKey] -= BigInt(1);
+}
+
+/**
+ * Increments the revision number in the cache for the given entry and returns
+ * the new revision number.
+ *
+ * @param client - The Skynet client.
+ * @param publicKey - The user public key.
+ * @param dataKey - The data key.
+ * @returns - The new revision number.
+ * @throws - Will throw if the revision is already the maximum value.
+ */
+export function incrementCachedRevision(client: SkynetClient, publicKey: string, dataKey: string): bigint {
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  const cachedRevision = client.revisionNumberCache[cacheKey];
+
+  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
+  const revision: bigint = cachedRevision ?? UNCACHED_REVISION_NUMBER;
+  const newRevision = incrementRevision(revision);
+
+  // Update the cached revision number.
+  client.revisionNumberCache[cacheKey] = newRevision;
+
+  return newRevision;
+}
+
 /**
  * Increments the given revision number and checks to make sure it is not
  * greater than the maximum revision.
@@ -680,10 +716,6 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   dataKey: string,
   opts: CustomGetEntryOptions
 ): Promise<RegistryEntry | null> {
-  // Calculate the cached revision number before doing anything else.
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  const cachedRevision = client.revisionNumberCache[cacheKey];
-
   // If this throws due to a parse error or network error, exit early and do not
   // update the cached revision number.
   const { entry } = await client.registry.getEntry(publicKey, dataKey, opts);
@@ -694,8 +726,9 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   }
 
   // Calculate the new revision and get the cached revision.
-  const revision = entry?.revision ?? UNCACHED_REVISION_NUMBER;
-  const newRevision = incrementRevision(revision);
+  const cacheKey = getCacheKey(publicKey, dataKey);
+  const cachedRevision = client.revisionNumberCache[cacheKey];
+  const newRevision = entry?.revision ?? UNCACHED_REVISION_NUMBER + BigInt(1);
 
   // Don't update the cached revision number if the received version is too low. Throw error.
   if (cachedRevision && cachedRevision > newRevision) {

From 7ff99d7798edfb33e4a72d0c7f2ad5b0f377d9c2 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 3 Nov 2021 14:40:34 -0500
Subject: [PATCH 06/31] Add test coverage

---
 integration/skydb.test.ts | 22 ++++++++++++++++++++++
 src/client.ts             |  2 +-
 src/skydb.test.ts         | 38 +++++++++++++++++++++++++++++++-------
 src/skydb.ts              | 16 +---------------
 4 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 78f107a0..9635eed5 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -3,6 +3,7 @@ import { AxiosError } from "axios";
 import { client, dataKey, portal } from ".";
 import { genKeyPairAndSeed, getEntryLink, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
+import { getCacheKey } from "../src/skydb";
 import { decodeSkylinkBase64 } from "../src/utils/encoding";
 import { toHexString } from "../src/utils/string";
 
@@ -242,6 +243,27 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     expect(data).toEqual(json);
   });
 
+  it("Should update the revision number cache", async () => {
+    const { publicKey, privateKey } = genKeyPairAndSeed();
+    const json = { message: 1 };
+    const cacheKey = getCacheKey(publicKey, dataKey);
+
+    await client.db.setJSON(privateKey, dataKey, json);
+
+    let revisionNumber = client.revisionNumberCache[cacheKey];
+    expect(revisionNumber.toString()).toEqual("0");
+
+    await client.db.setJSON(privateKey, dataKey, json);
+
+    revisionNumber = client.revisionNumberCache[cacheKey];
+    expect(revisionNumber.toString()).toEqual("1");
+
+    await client.db.getJSON(publicKey, dataKey);
+
+    revisionNumber = client.revisionNumberCache[cacheKey];
+    expect(revisionNumber.toString()).toEqual("1");
+  });
+
   // REGRESSION TEST: By creating a gap between setJSON and getJSON, a user
   // could call getJSON, get outdated data, then call setJSON, and overwrite
   // more up to date data with outdated data, but still use a high enough
diff --git a/src/client.ts b/src/client.ts
index 0d971e73..347d6a75 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -104,7 +104,7 @@ export class SkynetClient {
   // The custom portal URL, if one was passed in to `new SkynetClient()`.
   protected customPortalUrl?: string;
 
-  // Holds the cached revision numbers
+  // Holds the cached revision numbers.
   revisionNumberCache: { [key: string]: bigint } = {};
 
   // Set methods (defined in other files).
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 3ebafb33..86f0c389 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -110,7 +110,7 @@ describe("getJSON", () => {
   });
 
   it("should return null if no entry is found", async () => {
-    mock.onGet(registryLookupUrl).reply(404);
+    mock.onGet(registryLookupUrl).replyOnce(404);
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey);
     expect(data).toBeNull();
@@ -119,8 +119,8 @@ describe("getJSON", () => {
 
   it("should throw if the returned file data is not JSON", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).reply(200, JSON.stringify(entryData));
-    mock.onGet(skylinkUrl).reply(200, "thisistext", { ...headers, "content-type": "text/plain" });
+    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(skylinkUrl).replyOnce(200, "thisistext", { ...headers, "content-type": "text/plain" });
 
     await expect(client.db.getJSON(publicKey, dataKey)).rejects.toThrowError(
       `File data for the entry at data key '${dataKey}' is not JSON.`
@@ -129,8 +129,8 @@ describe("getJSON", () => {
 
   it("should throw if the returned _data field in the file data is not JSON", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).reply(200, JSON.stringify(entryData));
-    mock.onGet(skylinkUrl).reply(200, { _data: "thisistext", _v: 1 }, headers);
+    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(skylinkUrl).replyOnce(200, { _data: "thisistext", _v: 1 }, headers);
 
     await expect(client.db.getJSON(publicKey, dataKey)).rejects.toThrowError(
       "File data '_data' for the entry at data key 'app' is not JSON."
@@ -179,7 +179,7 @@ describe("setJSON", () => {
 
   it("should use a revision number of 0 if the entry is not cached", async () => {
     // mock a successful registry update
-    mock.onPost(registryUrl).reply(204);
+    mock.onPost(registryUrl).replyOnce(204);
 
     // call `setJSON` on the client
     await client.db.setJSON(privateKey, "inexistent entry", jsonData);
@@ -199,7 +199,7 @@ describe("setJSON", () => {
     client.revisionNumberCache[cacheKey] = MAX_REVISION;
 
     // mock a successful registry update
-    mock.onPost(registryUrl).reply(204);
+    mock.onPost(registryUrl).replyOnce(204);
 
     // Try to set data, should fail.
     await expect(client.db.setJSON(privateKey, dataKey, entryData)).rejects.toThrowError(
@@ -226,6 +226,30 @@ describe("setJSON", () => {
       "Expected parameter 'json' to be type 'object', was type 'undefined'"
     );
   });
+
+  it("Should not update the cached revision if the registry update fails.", async () => {
+    const dataKey = "registry failure";
+    const cacheKey = getCacheKey(publicKey, dataKey);
+    const json = { foo: "bar" };
+
+    // mock a successful registry update
+    mock.onPost(registryUrl).replyOnce(204);
+
+    await client.db.setJSON(privateKey, dataKey, json);
+
+    const revision1 = client.revisionNumberCache[cacheKey];
+
+    // mock a failed registry update
+    mock.onPost(registryUrl).replyOnce(404);
+
+    await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toThrowError(
+      "Request failed with status code 404"
+    );
+
+    const revision2 = client.revisionNumberCache[cacheKey];
+
+    expect(revision1.toString()).toEqual(revision2.toString());
+  });
 });
 
 describe("setEntryData", () => {
diff --git a/src/skydb.ts b/src/skydb.ts
index bcf204d1..38da8bde 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -548,6 +548,7 @@ export async function getOrCreateRegistryEntryFromCache(
     const jsonFullData: JsonFullData = { _data: data, _v: JSON_RESPONSE_VERSION };
     fullData = JSON.stringify(jsonFullData);
   } else {
+    /* istanbul ignore next - This case is only called by setJSONEncrypted which is not tested in this repo */
     fullData = data;
   }
 
@@ -585,21 +586,6 @@ export function getCacheKey(publicKey: string, dataKey: string): string {
   return `${publicKey}/${dataKey}`;
 }
 
-/**
- * Gets the next revision from a returned entry (or 0 if the entry was not
- * found).
- *
- * @param entry - The returned registry entry.
- * @returns - The revision.
- * @throws - Will throw if the next revision would be beyond the maximum allowed value.
- */
-export function getNextRevisionFromEntry(entry: RegistryEntry | null): bigint {
-  if (entry === null) {
-    return BigInt(0);
-  }
-  return incrementRevision(entry.revision);
-}
-
 /**
  * Decrements the revision number in the cache for the given entry.
  *

From c6101ffe3549e90d86ca056d2c37f16698813bc3 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 8 Nov 2021 15:07:15 -0600
Subject: [PATCH 07/31] Return errors from skyd

---
 integration/registry.test.ts |  2 +-
 src/registry.test.ts         |  9 ++---
 src/registry.ts              | 75 ++++++++++++++++++++++++++++--------
 src/skydb.test.ts            |  6 +--
 4 files changed, 65 insertions(+), 27 deletions(-)

diff --git a/integration/registry.test.ts b/integration/registry.test.ts
index cb8129cd..6812e9d7 100644
--- a/integration/registry.test.ts
+++ b/integration/registry.test.ts
@@ -78,7 +78,7 @@ describe(`Registry end to end integration tests for portal '${portal}'`, () => {
     await client.registry.setEntry(privateKey, entry);
     entry.revision--;
     await expect(client.registry.setEntry(privateKey, entry)).rejects.toThrowError(
-      "Request failed with status code 500"
+      "Unable to update the registry: provided revision number is invalid"
     );
   });
 });
diff --git a/src/registry.test.ts b/src/registry.test.ts
index 712d937f..1ed10102 100644
--- a/src/registry.test.ts
+++ b/src/registry.test.ts
@@ -25,11 +25,10 @@ describe("getEntry", () => {
   it("should throw if the response status is not in the 200s and not 404 and JSON is returned", async () => {
     mock.onGet(registryLookupUrl).replyOnce(400, JSON.stringify({ message: "foo error" }));
 
-    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toThrowError(
-      "Request failed with status code 400"
-    );
+    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(new Error("foo error"));
   });
 
+  // In the case of a 429 error due to rate limiting all we get is HTML.
   it("should throw if the response status is not in the 200s and not 404 and HTML is returned", async () => {
     const responseHTML = `
 <head><title>429 Too Many Requests</title></head>
@@ -41,8 +40,8 @@ describe("getEntry", () => {
 
     mock.onGet(registryLookupUrl).replyOnce(429, responseHTML);
 
-    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toThrowError(
-      "Request failed with status code 429"
+    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(
+      new Error("Request failed with status code 429")
     );
   });
 
diff --git a/src/registry.ts b/src/registry.ts
index 98026a07..4c3a197b 100644
--- a/src/registry.ts
+++ b/src/registry.ts
@@ -482,20 +482,24 @@ export async function postSignedEntry(
     signature: Array.from(signature),
   };
 
-  await this.executeRequest({
-    ...opts,
-    endpointPath: opts.endpointSetEntry,
-    method: "post",
-    data,
-    // Transform the request to remove quotes, since the revision needs to be
-    // parsed as a uint64 on the Go side.
-    transformRequest: function (data: unknown) {
-      // Convert the object data to JSON.
-      const json = JSON.stringify(data);
-      // Change the revision value from a string to a JSON integer.
-      return json.replace(REGEX_REVISION_WITH_QUOTES, '"revision":$1');
-    },
-  });
+  try {
+    await this.executeRequest({
+      ...opts,
+      endpointPath: opts.endpointSetEntry,
+      method: "post",
+      data,
+      // Transform the request to remove quotes, since the revision needs to be
+      // parsed as a uint64 on the Go side.
+      transformRequest: function (data: unknown) {
+        // Convert the object data to JSON.
+        const json = JSON.stringify(data);
+        // Change the revision value from a string to a JSON integer.
+        return json.replace(REGEX_REVISION_WITH_QUOTES, '"revision":$1');
+      },
+    });
+  } catch (err) {
+    handleSetEntryErrResponse(err as AxiosError);
+  }
 }
 
 /**
@@ -575,17 +579,18 @@ export function validateRegistryProof(
 }
 
 /**
- * Handles error responses returned in getEntry.
+ * Handles error responses returned from getEntry endpoint.
  *
  * @param err - The Axios error.
  * @returns - An empty signed registry entry if the status code is 404.
- * @throws - Will throw if the status code is not 404.
+ * @throws - Will throw if the error response is malformed, or the error message otherwise if the error status code is not 404.
  */
 function handleGetEntryErrResponse(err: AxiosError): SignedRegistryEntry {
   /* istanbul ignore next */
   if (!err.response) {
     throw new Error(`Error response field not found, incomplete Axios error. Full error: ${err}`);
   }
+
   /* istanbul ignore next */
   if (!err.response.status) {
     throw new Error(`Error response did not contain expected field 'status'. Full error: ${err}`);
@@ -595,7 +600,43 @@ function handleGetEntryErrResponse(err: AxiosError): SignedRegistryEntry {
     return { entry: null, signature: null };
   }
 
-  throw err;
+  // If we don't get an error message from skyd, just return the Axios error.
+  /* istanbul ignore next */
+  if (!err.response.data) {
+    throw err;
+  }
+  if (!err.response.data.message) {
+    throw err;
+  }
+
+  // Return the error message from skyd.
+  throw new Error(err.response.data.message);
+}
+
+/**
+ * Handles error responses returned from setEntry endpoint.
+ *
+ * @param err - The Axios error.
+ * @throws - Will throw if the error response is malformed, or the error message otherwise.
+ */
+function handleSetEntryErrResponse(err: AxiosError) {
+  /* istanbul ignore next */
+  if (!err.response) {
+    throw new Error(`Error response field not found, incomplete Axios error. Full error: ${err}`);
+  }
+
+  // If we don't get an error message from skyd, just return the Axios error.
+  /* istanbul ignore next */
+  if (!err.response.data) {
+    throw err;
+  }
+  /* istanbul ignore next */
+  if (!err.response.data.message) {
+    throw err;
+  }
+
+  // Return the error message from skyd.
+  throw new Error(err.response.data.message);
 }
 
 /**
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 86f0c389..e9c021a2 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -240,11 +240,9 @@ describe("setJSON", () => {
     const revision1 = client.revisionNumberCache[cacheKey];
 
     // mock a failed registry update
-    mock.onPost(registryUrl).replyOnce(404);
+    mock.onPost(registryUrl).replyOnce(400, JSON.stringify({ message: "foo" }));
 
-    await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toThrowError(
-      "Request failed with status code 404"
-    );
+    await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toEqual(new Error("foo"));
 
     const revision2 = client.revisionNumberCache[cacheKey];
 

From 6dad595ed8d18fdf354f727d3e433fe74a3724d5 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 8 Nov 2021 15:23:15 -0600
Subject: [PATCH 08/31] Fix missing cache updates in `setEntryData` methods +
 refactor

---
 src/mysky/index.ts | 30 ++++++++++++++------------
 src/skydb.ts       | 53 +++++++++++++---------------------------------
 2 files changed, 31 insertions(+), 52 deletions(-)

diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 6edc30de..247311c7 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -29,9 +29,8 @@ import {
   DEFAULT_SET_JSON_OPTIONS,
   CustomGetJSONOptions,
   CustomSetJSONOptions,
-  getOrCreateRegistryEntryFromCache,
+  getOrCreateRegistryEntry,
   JSONResponse,
-  getNextRegistryEntryFromCache,
   validateEntryData,
   CustomSetEntryDataOptions,
   DEFAULT_SET_ENTRY_DATA_OPTIONS,
@@ -404,13 +403,7 @@ export class MySky {
       // Call SkyDB helper to create the registry entry. We can't call SkyDB's
       // setJSON here directly because we need MySky to sign the entry, instead of
       // signing it ourselves with a given private key.
-      [entry, dataLink] = await getOrCreateRegistryEntryFromCache(
-        this.connector.client,
-        dataKey,
-        json,
-        newRevision,
-        opts
-      );
+      [entry, dataLink] = await getOrCreateRegistryEntry(this.connector.client, dataKey, json, newRevision, opts);
 
       const signature = await this.signRegistryEntry(entry, path);
 
@@ -524,12 +517,21 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    const entry = await getNextRegistryEntryFromCache(this.connector.client, publicKey, dataKey, data);
+    // Get the cached revision number before doing anything else.
+    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
 
-    const signature = await this.signRegistryEntry(entry, path);
+    const entry = { dataKey, data, revision: newRevision };
 
-    const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-    await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+    try {
+      const signature = await this.signRegistryEntry(entry, path);
+
+      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+    } catch (e) {
+      // Something failed, revert the cached revision number increment.
+      decrementCachedRevision(this.connector.client, publicKey, dataKey);
+      throw e;
+    }
 
     return { data: entry.data };
   }
@@ -659,7 +661,7 @@ export class MySky {
       // Pad and encrypt json file.
       const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
 
-      const [entry] = await getOrCreateRegistryEntryFromCache(this.connector.client, dataKey, data, newRevision, opts);
+      const [entry] = await getOrCreateRegistryEntry(this.connector.client, dataKey, data, newRevision, opts);
 
       // Call MySky which checks for write permissions on the path.
       const signature = await this.signEncryptedRegistryEntry(entry, path);
diff --git a/src/skydb.ts b/src/skydb.ts
index 38da8bde..a5207f27 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -238,7 +238,7 @@ export async function setJSON(
 
   let entry, dataLink;
   try {
-    [entry, dataLink] = await getOrCreateRegistryEntryFromCache(this, dataKey, json, newRevision, opts);
+    [entry, dataLink] = await getOrCreateRegistryEntry(this, dataKey, json, newRevision, opts);
 
     // Update the registry.
     const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
@@ -381,11 +381,21 @@ export async function setEntryData(
   validateEntryData(data, opts.allowDeletionEntryData);
 
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
+  const publicKey = toHexString(publicKeyArray);
 
-  const entry = await getNextRegistryEntryFromCache(this, toHexString(publicKeyArray), dataKey, data);
+  // Get the cached revision number before doing anything else.
+  const newRevision = incrementCachedRevision(this, publicKey, dataKey);
 
-  const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-  await this.registry.setEntry(privateKey, entry, setEntryOpts);
+  const entry = { dataKey, data, revision: newRevision };
+
+  try {
+    const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+    await this.registry.setEntry(privateKey, entry, setEntryOpts);
+  } catch (e) {
+    // Something failed, revert the cached revision number increment.
+    decrementCachedRevision(this, publicKey, dataKey);
+    throw e;
+  }
 
   return { data: entry.data };
 }
@@ -481,39 +491,6 @@ export async function getRawBytes(
 // Helpers
 // =======
 
-/**
- * Gets the next entry for the given public key and data key, setting the data to be the given data and the revision number accordingly.
- *
- * @param client - The Skynet client.
- * @param publicKey - The user public key.
- * @param dataKey - The data key.
- * @param data - The data to set.
- * @returns - The registry entry and corresponding data link.
- * @throws - Will throw if the revision is already the maximum value.
- */
-export async function getNextRegistryEntryFromCache(
-  client: SkynetClient,
-  publicKey: string,
-  dataKey: string,
-  data: Uint8Array
-): Promise<RegistryEntry> {
-  // Not publicly available, don't validate input.
-
-  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  let revision: bigint = client.revisionNumberCache[cacheKey] ?? UNCACHED_REVISION_NUMBER;
-  revision = incrementRevision(revision);
-
-  // Build the registry entry.
-  const entry: RegistryEntry = {
-    dataKey,
-    data,
-    revision,
-  };
-
-  return entry;
-}
-
 /**
  * Gets the registry entry and data link or creates the entry if it doesn't
  * exist. Uses the cached revision number for the entry, or 0 if the entry has
@@ -527,7 +504,7 @@ export async function getNextRegistryEntryFromCache(
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
  */
-export async function getOrCreateRegistryEntryFromCache(
+export async function getOrCreateRegistryEntry(
   client: SkynetClient,
   dataKey: string,
   data: JsonData | Uint8Array,

From aca91decdfd3158d6d633f7dd9604a96654f827c Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 8 Nov 2021 16:51:42 -0600
Subject: [PATCH 09/31] Minor refactor for clarity and to more closely match
 spec

---
 src/mysky/index.ts |  6 +++---
 src/skydb.ts       | 34 +++++++++++++++++++++++-----------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 247311c7..356a5ffe 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -29,7 +29,7 @@ import {
   DEFAULT_SET_JSON_OPTIONS,
   CustomGetJSONOptions,
   CustomSetJSONOptions,
-  getOrCreateRegistryEntry,
+  getOrCreateSkyDBRegistryEntry,
   JSONResponse,
   validateEntryData,
   CustomSetEntryDataOptions,
@@ -403,7 +403,7 @@ export class MySky {
       // Call SkyDB helper to create the registry entry. We can't call SkyDB's
       // setJSON here directly because we need MySky to sign the entry, instead of
       // signing it ourselves with a given private key.
-      [entry, dataLink] = await getOrCreateRegistryEntry(this.connector.client, dataKey, json, newRevision, opts);
+      [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this.connector.client, dataKey, json, newRevision, opts);
 
       const signature = await this.signRegistryEntry(entry, path);
 
@@ -661,7 +661,7 @@ export class MySky {
       // Pad and encrypt json file.
       const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
 
-      const [entry] = await getOrCreateRegistryEntry(this.connector.client, dataKey, data, newRevision, opts);
+      const [entry] = await getOrCreateSkyDBRegistryEntry(this.connector.client, dataKey, data, newRevision, opts);
 
       // Call MySky which checks for write permissions on the path.
       const signature = await this.signEncryptedRegistryEntry(entry, path);
diff --git a/src/skydb.ts b/src/skydb.ts
index a5207f27..b8f69135 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -39,7 +39,7 @@ import {
 import { ResponseType } from "axios";
 import { EntryData, MAX_ENTRY_LENGTH } from "./mysky";
 
-export type JsonFullData = {
+type SkynetJson = {
   _data: JsonData;
   _v: number;
 };
@@ -179,8 +179,9 @@ export async function getJSON(
 
   // Download the data in the returned data link.
   const downloadOpts = extractOptions(opts, DEFAULT_DOWNLOAD_OPTIONS);
-  const { data } = await this.getFileContent<JsonData>(dataLink, downloadOpts);
+  const { data }: { data: JsonData | SkynetJson } = await this.getFileContent<JsonData>(dataLink, downloadOpts);
 
+  // Validate that the returned data is JSON.
   if (typeof data !== "object" || data === null) {
     throw new Error(`File data for the entry at data key '${dataKey}' is not JSON.`);
   }
@@ -190,6 +191,7 @@ export async function getJSON(
     return { data, dataLink };
   }
 
+  // Extract the JSON from the returned SkynetJson.
   const actualData = data["_data"];
   if (typeof actualData !== "object" || data === null) {
     throw new Error(`File data '_data' for the entry at data key '${dataKey}' is not JSON.`);
@@ -238,7 +240,7 @@ export async function setJSON(
 
   let entry, dataLink;
   try {
-    [entry, dataLink] = await getOrCreateRegistryEntry(this, dataKey, json, newRevision, opts);
+    [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this, dataKey, json, newRevision, opts);
 
     // Update the registry.
     const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
@@ -504,7 +506,7 @@ export async function getRawBytes(
  * @returns - The registry entry and corresponding data link.
  * @throws - Will throw if the revision is already the maximum value.
  */
-export async function getOrCreateRegistryEntry(
+export async function getOrCreateSkyDBRegistryEntry(
   client: SkynetClient,
   dataKey: string,
   data: JsonData | Uint8Array,
@@ -522,8 +524,8 @@ export async function getOrCreateRegistryEntry(
   let fullData: string | Uint8Array;
   if (!(data instanceof Uint8Array)) {
     // Set the hidden _data and _v fields.
-    const jsonFullData: JsonFullData = { _data: data, _v: JSON_RESPONSE_VERSION };
-    fullData = JSON.stringify(jsonFullData);
+    const skynetJson = buildSkynetJsonObject(data);
+    fullData = JSON.stringify(skynetJson);
   } else {
     /* istanbul ignore next - This case is only called by setJSONEncrypted which is not tested in this repo */
     fullData = data;
@@ -711,13 +713,13 @@ async function getSkyDBRegistryEntryAndUpdateCache(
 }
 
 /**
- * Returns whether the given registry entry indicates a past deletion.
+ * Sets the hidden _data and _v fields on the given raw JSON data.
  *
- * @param entry - The registry entry.
- * @returns - Whether the registry entry data indicated a past deletion.
+ * @param data - The given JSON data.
+ * @returns - The Skynet JSON data.
  */
-function wasRegistryEntryDeleted(entry: RegistryEntry): boolean {
-  return areEqualUint8Arrays(entry.data, EMPTY_SKYLINK);
+function buildSkynetJsonObject(data: JsonData): SkynetJson {
+  return { _data: data, _v: JSON_RESPONSE_VERSION };
 }
 
 /**
@@ -741,3 +743,13 @@ function parseDataLink(data: Uint8Array, legacy: boolean): { rawDataLink: string
   }
   return { rawDataLink, dataLink: formatSkylink(rawDataLink) };
 }
+
+/**
+ * Returns whether the given registry entry indicates a past deletion.
+ *
+ * @param entry - The registry entry.
+ * @returns - Whether the registry entry data indicated a past deletion.
+ */
+function wasRegistryEntryDeleted(entry: RegistryEntry): boolean {
+  return areEqualUint8Arrays(entry.data, EMPTY_SKYLINK);
+}

From 3b64f2ef3170941c016aed90856f596b6dc69519 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 10 Nov 2021 12:41:03 -0600
Subject: [PATCH 10/31] Add integration and unit regression tests for SkyDB
 data race bugs

- [X] Integration test: setJSON and getJSON called simultaneously
- [X] Unit test: setJSON and getJSON called simultaneously
- [X] Integration test: setJSON and getJSON called with a delay
- [X] Integration test: setJSON and getJSON called with two different clients (different local caches)
- [X] Unit test: setJSON and getJSON called with two different clients (different local caches)
- [X] Unit test: multiple setJSON calls where one fails
---
 integration/skydb.test.ts | 110 +++++++++++++-----
 src/registry.test.ts      |  38 ++++++-
 src/skydb.test.ts         | 228 ++++++++++++++++++++++++++++++++++----
 src/utils/encoding.ts     |   4 +
 tsconfig.json             |   1 +
 5 files changed, 324 insertions(+), 57 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 9635eed5..14f32c58 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -1,7 +1,7 @@
 import { AxiosError } from "axios";
 
 import { client, dataKey, portal } from ".";
-import { genKeyPairAndSeed, getEntryLink, URI_SKYNET_PREFIX } from "../src";
+import { genKeyPairAndSeed, getEntryLink, SkynetClient, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
 import { getCacheKey } from "../src/skydb";
 import { decodeSkylinkBase64 } from "../src/utils/encoding";
@@ -264,41 +264,91 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     expect(revisionNumber.toString()).toEqual("1");
   });
 
-  // REGRESSION TEST: By creating a gap between setJSON and getJSON, a user
+  // REGRESSION TESTS: By creating a gap between setJSON and getJSON, a user
   // could call getJSON, get outdated data, then call setJSON, and overwrite
   // more up to date data with outdated data, but still use a high enough
   // revision number.
   //
   // The fix is that you cannot retrieve the revision number while calling
   // setJSON. You have to use the same revision number that you had when you
-  // called getJSON
-  it("Should avoid data race bugs where getJSON is not called", async () => {
-    const { publicKey, privateKey } = genKeyPairAndSeed();
-    const json1 = { message: 1 };
-    const json2 = { message: 2 };
-
-    // Set the data.
-    await client.db.setJSON(privateKey, dataKey, json1);
-
-    // Try to invoke the data race.
-    let receivedJson;
-    try {
-      // Get the data while also calling setJSON.
-      [{ data: receivedJson }] = await Promise.all([
-        client.db.getJSON(publicKey, dataKey),
-        client.db.setJSON(privateKey, dataKey, json2),
-      ]);
-    } catch (e) {
-      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
-        // The data race condition has been prevented and we received the expected error. Return from test early.
-        return;
+  // called getJSON.
+  describe("getJSON/setJSON data race regression integration tests", () => {
+    const jsonOld = { message: 1 };
+    const jsonNew = { message: 2 };
+
+    const delays = [0, 100, 200, 300, 500, 1000];
+
+    it.each(delays)(
+      "should not get old data when getJSON is called after setJSON on a single client with a '%s' ms delay and getJSON doesn't fail",
+      async (delay) => {
+        const { publicKey, privateKey } = genKeyPairAndSeed();
+
+        // Set the data.
+        await client.db.setJSON(privateKey, dataKey, jsonOld);
+
+        // Try to invoke the data race.
+        let receivedJson;
+        try {
+          // Get the data while also calling setJSON.
+          const getJSONFn = async function () {
+            // Sleep.
+            await new Promise((r) => setTimeout(r, delay));
+            return await client.db.getJSON(publicKey, dataKey);
+          };
+          [{ data: receivedJson }] = await Promise.all([getJSONFn(), client.db.setJSON(privateKey, dataKey, jsonNew)]);
+        } catch (e) {
+          if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+            // The data race condition has been prevented and we received the expected error. Return from test early.
+            return;
+          }
+
+          // Unexpected error, throw.
+          throw e;
+        }
+
+        // Data race did not occur, getJSON should have latest JSON.
+        expect(receivedJson).toEqual(jsonNew);
       }
-
-      // Unexpected error, throw.
-      throw e;
-    }
-
-    // Data race did not occur, getJSON should have latest JSON.
-    expect(receivedJson).toEqual(json2);
+    );
+
+    it.each(delays)(
+      "should not get old data when getJSON is called after setJSON on two different clients with a '%s' ms delay and getJSON doesn't fail",
+      async (delay) => {
+        // Create two new clients with a fresh revision cache.
+        const client1 = new SkynetClient(portal);
+        const client2 = new SkynetClient(portal);
+        const { publicKey, privateKey } = genKeyPairAndSeed();
+
+        // Use a random client to set the initial data.
+        if (Math.random() < 0.5) {
+          await client1.db.setJSON(privateKey, dataKey, jsonOld);
+        } else {
+          await client2.db.setJSON(privateKey, dataKey, jsonOld);
+        }
+
+        // Try to invoke the data race.
+        let receivedJson;
+        try {
+          // Get the data while also calling setJSON.
+          const getJSONFn = async function () {
+            // Sleep.
+            await new Promise((r) => setTimeout(r, delay));
+            return await client2.db.getJSON(publicKey, dataKey);
+          };
+          [{ data: receivedJson }] = await Promise.all([getJSONFn(), client1.db.setJSON(privateKey, dataKey, jsonNew)]);
+        } catch (e) {
+          if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+            // The data race condition has been prevented and we received the expected error. Return from test early.
+            return;
+          }
+
+          // Unexpected error, throw.
+          throw e;
+        }
+
+        // Data race did not occur, getJSON should have latest JSON.
+        expect(receivedJson).toEqual(jsonNew);
+      }
+    );
   });
 });
diff --git a/src/registry.test.ts b/src/registry.test.ts
index 1ed10102..680cee09 100644
--- a/src/registry.test.ts
+++ b/src/registry.test.ts
@@ -5,14 +5,15 @@ import { genKeyPairAndSeed } from "./crypto";
 import { SkynetClient, defaultSkynetPortalUrl, genKeyPairFromSeed } from "./index";
 import { getEntryLink, getEntryUrlForPortal, signEntry, validateRegistryProof } from "./registry";
 import { uriSkynetPrefix } from "./utils/url";
-import { stringToUint8ArrayUtf8 } from "./utils/string";
+import { hexToUint8Array, stringToUint8ArrayUtf8 } from "./utils/string";
 
 const { publicKey, privateKey } = genKeyPairFromSeed("insecure test seed");
 const portalUrl = defaultSkynetPortalUrl;
 const client = new SkynetClient(portalUrl);
 const dataKey = "app";
 
-const registryLookupUrl = getEntryUrlForPortal(portalUrl, publicKey, dataKey);
+const registryPostUrl = `${portalUrl}/skynet/registry`;
+const registryGetUrl = getEntryUrlForPortal(portalUrl, publicKey, dataKey);
 
 describe("getEntry", () => {
   let mock: MockAdapter;
@@ -23,7 +24,7 @@ describe("getEntry", () => {
   });
 
   it("should throw if the response status is not in the 200s and not 404 and JSON is returned", async () => {
-    mock.onGet(registryLookupUrl).replyOnce(400, JSON.stringify({ message: "foo error" }));
+    mock.onGet(registryGetUrl).replyOnce(400, JSON.stringify({ message: "foo error" }));
 
     await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(new Error("foo error"));
   });
@@ -38,7 +39,7 @@ describe("getEntry", () => {
 </body>
 </html>`;
 
-    mock.onGet(registryLookupUrl).replyOnce(429, responseHTML);
+    mock.onGet(registryGetUrl).replyOnce(429, responseHTML);
 
     await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(
       new Error("Request failed with status code 429")
@@ -54,7 +55,7 @@ describe("getEntry", () => {
         "33d14d2889cb292142614da0e0ff13a205c4867961276001471d13b779fc9032568ddd292d9e0dff69d7b1f28be07972cc9d86da3cecf3adecb6f9b7311af808",
     };
 
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
 
     await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toThrow();
   });
@@ -66,7 +67,7 @@ describe("getEntry", () => {
   });
 
   it("Should throw on incomplete response from registry GET", async () => {
-    mock.onGet(registryLookupUrl).replyOnce(200, "{}");
+    mock.onGet(registryGetUrl).replyOnce(200, "{}");
 
     await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toThrowError(
       "Did not get a complete entry response"
@@ -124,6 +125,31 @@ describe("getEntryUrl", () => {
 });
 
 describe("setEntry", () => {
+  let mock: MockAdapter;
+
+  beforeEach(() => {
+    mock = new MockAdapter(axios);
+    mock.resetHistory();
+  });
+
+  it("Should sign and set a valid registry entry", async () => {
+    // mock a successful registry update
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Hex-encoded skylink.
+    const data = hexToUint8Array(
+      "43414241425f31447430464a73787173755f4a34546f644e4362434776744666315579735f3345677a4f6c546367"
+    );
+
+    const entry = {
+      data,
+      dataKey,
+      revision: BigInt(11),
+    };
+
+    await client.registry.setEntry(privateKey, entry);
+  });
+
   it("Should throw an error if the private key is not hex-encoded", async () => {
     // @ts-expect-error We pass an invalid private key on purpose.
     await expect(client.registry.setEntry("foo", {})).rejects.toThrowError(
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index e9c021a2..2c932758 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -3,6 +3,7 @@ import MockAdapter from "axios-mock-adapter";
 
 import { getSkylinkUrlForPortal } from "./download";
 import { MAX_REVISION } from "./utils/number";
+import { stringToUint8ArrayUtf8, toHexString } from "./utils/string";
 import { DEFAULT_SKYNET_PORTAL_URL, URI_SKYNET_PREFIX } from "./utils/url";
 import { SkynetClient } from "./index";
 import { getEntryUrlForPortal } from "./registry";
@@ -25,14 +26,15 @@ const bitfield = 2048;
 
 const portalUrl = DEFAULT_SKYNET_PORTAL_URL;
 const client = new SkynetClient(portalUrl);
-const registryUrl = `${portalUrl}/skynet/registry`;
-const registryLookupUrl = getEntryUrlForPortal(portalUrl, publicKey, dataKey);
+const registryPostUrl = `${portalUrl}/skynet/registry`;
+const registryGetUrl = getEntryUrlForPortal(portalUrl, publicKey, dataKey);
 const uploadUrl = `${portalUrl}/skynet/skyfile`;
 const skylinkUrl = getSkylinkUrlForPortal(portalUrl, skylink);
 
 // Hex-encoded skylink.
 const data = "43414241425f31447430464a73787173755f4a34546f644e4362434776744666315579735f3345677a4f6c546367";
 const revision = 11;
+// Entry data for the data and revision.
 const entryData = {
   data,
   revision,
@@ -40,6 +42,12 @@ const entryData = {
     "33d14d2889cb292142614da0e0ff13a205c4867961276001471d13b779fc9032568ddd292d9e0dff69d7b1f28be07972cc9d86da3cecf3adecb6f9b7311af809",
 };
 
+const headers = {
+  "skynet-portal-api": portalUrl,
+  "skynet-skylink": skylink,
+  "content-type": "application/json",
+};
+
 describe("getJSON", () => {
   let mock: MockAdapter;
 
@@ -49,15 +57,10 @@ describe("getJSON", () => {
     mock.resetHistory();
   });
 
-  const headers = {
-    "skynet-portal-api": portalUrl,
-    "skynet-skylink": skylink,
-    "content-type": "application/json",
-  };
-
   it("should perform a lookup and skylink GET", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
+    // mock a successful data download
     mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, headers);
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey);
@@ -68,7 +71,7 @@ describe("getJSON", () => {
 
   it("should perform a lookup but not a skylink GET if the cachedDataLink is a hit", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey, { cachedDataLink: skylink });
     expect(data).toBeNull();
@@ -80,7 +83,7 @@ describe("getJSON", () => {
     const skylinkNoHit = "XABvi7JtJbQSMAcDwnUnmp2FKDPjg8_tTTFP4BwMSxVdEg";
 
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
     mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, headers);
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey, { cachedDataLink: skylinkNoHit });
@@ -91,7 +94,7 @@ describe("getJSON", () => {
 
   it("should throw if the cachedDataLink is not a valid skylink", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
     mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, {});
 
     await expect(client.db.getJSON(publicKey, dataKey, { cachedDataLink: "asdf" })).rejects.toThrowError(
@@ -101,7 +104,7 @@ describe("getJSON", () => {
 
   it("should perform a lookup and skylink GET on legacy pre-v4 data", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
     mock.onGet(skylinkUrl).replyOnce(200, legacyJsonData, headers);
 
     const jsonReturned = await client.db.getJSON(publicKey, dataKey);
@@ -110,7 +113,7 @@ describe("getJSON", () => {
   });
 
   it("should return null if no entry is found", async () => {
-    mock.onGet(registryLookupUrl).replyOnce(404);
+    mock.onGet(registryGetUrl).replyOnce(404);
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey);
     expect(data).toBeNull();
@@ -119,7 +122,7 @@ describe("getJSON", () => {
 
   it("should throw if the returned file data is not JSON", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
     mock.onGet(skylinkUrl).replyOnce(200, "thisistext", { ...headers, "content-type": "text/plain" });
 
     await expect(client.db.getJSON(publicKey, dataKey)).rejects.toThrowError(
@@ -129,7 +132,7 @@ describe("getJSON", () => {
 
   it("should throw if the returned _data field in the file data is not JSON", async () => {
     // mock a successful registry lookup
-    mock.onGet(registryLookupUrl).replyOnce(200, JSON.stringify(entryData));
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
     mock.onGet(skylinkUrl).replyOnce(200, { _data: "thisistext", _v: 1 }, headers);
 
     await expect(client.db.getJSON(publicKey, dataKey)).rejects.toThrowError(
@@ -161,7 +164,7 @@ describe("setJSON", () => {
 
   it("should perform an upload, lookup and registry update", async () => {
     // mock a successful registry update
-    mock.onPost(registryUrl).replyOnce(204);
+    mock.onPost(registryPostUrl).replyOnce(204);
 
     // set data
     const { data: returnedData, dataLink: returnedSkylink } = await client.db.setJSON(privateKey, dataKey, jsonData);
@@ -179,7 +182,7 @@ describe("setJSON", () => {
 
   it("should use a revision number of 0 if the entry is not cached", async () => {
     // mock a successful registry update
-    mock.onPost(registryUrl).replyOnce(204);
+    mock.onPost(registryPostUrl).replyOnce(204);
 
     // call `setJSON` on the client
     await client.db.setJSON(privateKey, "inexistent entry", jsonData);
@@ -199,7 +202,7 @@ describe("setJSON", () => {
     client.revisionNumberCache[cacheKey] = MAX_REVISION;
 
     // mock a successful registry update
-    mock.onPost(registryUrl).replyOnce(204);
+    mock.onPost(registryPostUrl).replyOnce(204);
 
     // Try to set data, should fail.
     await expect(client.db.setJSON(privateKey, dataKey, entryData)).rejects.toThrowError(
@@ -233,14 +236,14 @@ describe("setJSON", () => {
     const json = { foo: "bar" };
 
     // mock a successful registry update
-    mock.onPost(registryUrl).replyOnce(204);
+    mock.onPost(registryPostUrl).replyOnce(204);
 
     await client.db.setJSON(privateKey, dataKey, json);
 
     const revision1 = client.revisionNumberCache[cacheKey];
 
     // mock a failed registry update
-    mock.onPost(registryUrl).replyOnce(400, JSON.stringify({ message: "foo" }));
+    mock.onPost(registryPostUrl).replyOnce(400, JSON.stringify({ message: "foo" }));
 
     await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toEqual(new Error("foo"));
 
@@ -285,3 +288,186 @@ describe("checkCachedDataLink", () => {
     );
   });
 });
+
+// REGRESSION TESTS: By creating a gap between setJSON and getJSON, a user
+// could call getJSON, get outdated data, then call setJSON, and overwrite
+// more up to date data with outdated data, but still use a high enough
+// revision number.
+//
+// The fix is that you cannot retrieve the revision number while calling
+// setJSON. You have to use the same revision number that you had when you
+// called getJSON.
+describe("getJSON/setJSON data race regression unit tests", () => {
+  let mock: MockAdapter;
+
+  beforeEach(() => {
+    mock = new MockAdapter(axios);
+    mock.onHead(portalUrl).replyOnce(200, {}, { "skynet-portal-api": portalUrl });
+    mock.resetHistory();
+  });
+
+  const skylinkOld = "XABvi7JtJbQSMAcDwnUnmp2FKDPjg8_tTTFP4BwMSxVdEg";
+  const skylinkOldUrl = getSkylinkUrlForPortal(portalUrl, skylinkOld);
+  const dataOld = toHexString(stringToUint8ArrayUtf8(skylinkOld)); // hex-encoded skylink
+  const revisionOld = 0;
+  const entryDataOld = {
+    data: dataOld,
+    revision: revisionOld,
+    signature:
+      "921d30e860d51f13d1065ea221b29fc8d11cfe7fa0e32b5d5b8e13bee6f91cfa86fe6b12ca4cef7a90ba52d2c50efb62b241f383e9d7bb264558280e564faa0f",
+  };
+  const headersOld = { ...headers, "skynet-skylink": skylinkOld };
+
+  const skylinkNew = skylink;
+  const skylinkNewUrl = skylinkUrl;
+  const dataNew = data; // hex-encoded skylink
+  const revisionNew = 1;
+  const entryDataNew = {
+    data: dataNew,
+    revision: revisionNew,
+    signature:
+      "2a9889915f06d414e8cde51eb17db565410d20b2b50214e8297f7f4a0cb5c77e0edc62a319607dfaa042e0cc16ed0d7e549cca2abd11c2f86a335009936f150d",
+  };
+  const headersNew = { ...headers, "skynet-skylink": skylinkNew };
+
+  const jsonOld = { message: 1 };
+  const jsonNew = { message: 2 };
+  const skynetJsonOld = { _data: jsonOld, _v: 2 };
+  const skynetJsonNew = { _data: jsonNew, _v: 2 };
+
+  it("should not get old data when getJSON and setJSON are called simultaneously on the same client and getJSON doesn't fail", async () => {
+    // Create a new client with a fresh revision cache.
+    const client = new SkynetClient(portalUrl);
+
+    // Mock setJSON with the old skylink.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Set the data.
+    await client.db.setJSON(privateKey, dataKey, jsonOld);
+
+    // Mock getJSON with the new entry data and the new skylink.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
+    mock.onGet(skylinkNewUrl).replyOnce(200, skynetJsonNew, headers);
+
+    // Mock setJSON with the new skylink.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkNew, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Try to invoke the data race.
+    let receivedJson;
+    try {
+      // Get the data while also calling setJSON.
+      [{ data: receivedJson }] = await Promise.all([
+        client.db.getJSON(publicKey, dataKey),
+        client.db.setJSON(privateKey, dataKey, jsonNew),
+      ]);
+    } catch (e) {
+      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+        // The data race condition has been prevented and we received the expected error. Return from test early.
+        return;
+      }
+
+      // Unexpected error, throw.
+      throw e;
+    }
+
+    // Data race did not occur, getJSON should have latest JSON.
+    expect(receivedJson).toEqual(jsonNew);
+
+    // assert our request history contains the expected amount of requests
+    expect(mock.history.get.length).toBe(2);
+    expect(mock.history.post.length).toBe(4);
+  });
+
+  it("should not get old data when getJSON and setJSON are called simultaneously on different clients and getJSON doesn't fail", async () => {
+    // Create two new clients with a fresh revision cache.
+    const client1 = new SkynetClient(portalUrl);
+    const client2 = new SkynetClient(portalUrl);
+
+    // Mock setJSON with the old skylink.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Set the data.
+    await client1.db.setJSON(privateKey, dataKey, jsonOld);
+
+    // Mock getJSON with the new entry data and the new skylink.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
+    mock.onGet(skylinkNewUrl).replyOnce(200, skynetJsonNew, headersNew);
+
+    // Mock setJSON with the new skylink.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkNew, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Try to invoke the data race.
+    let receivedJson;
+    try {
+      // Get the data while also calling setJSON.
+      [{ data: receivedJson }] = await Promise.all([
+        client2.db.getJSON(publicKey, dataKey),
+        client1.db.setJSON(privateKey, dataKey, jsonNew),
+      ]);
+    } catch (e) {
+      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+        // The data race condition has been prevented and we received the expected error. Return from test early.
+        return;
+      }
+
+      // Unexpected error, throw.
+      throw e;
+    }
+
+    // Data race did not occur, getJSON should have latest JSON.
+    expect(receivedJson).toEqual(jsonNew);
+
+    // assert our request history contains the expected amount of requests.
+    expect(mock.history.get.length).toBe(2);
+    expect(mock.history.post.length).toBe(4);
+  });
+
+  it("should not mess up cache when two setJSON calls are made simultaneously and one fails", async () => {
+    // Create a new client with a fresh revision cache.
+    const client = new SkynetClient(portalUrl);
+
+    // Mock a successful setJSON.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+
+    // Mock setJSON that fails to upload.
+    mock.onPost(uploadUrl).replyOnce(400);
+
+    await Promise.allSettled([
+      client.db.setJSON(privateKey, dataKey, jsonOld),
+      client.db.setJSON(privateKey, dataKey, jsonOld),
+    ]);
+
+    const cacheKey1 = getCacheKey(publicKey, dataKey);
+    expect(client.revisionNumberCache[cacheKey1].toString()).toEqual("0");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataOld));
+    mock.onGet(skylinkOldUrl).replyOnce(200, skynetJsonOld, headersOld);
+    const { data: receivedJson1 } = await client.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJson1).toEqual(jsonOld);
+
+    // Make another setJSON call - it should still work.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkNew, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+    await client.db.setJSON(privateKey, dataKey, jsonNew);
+
+    const cacheKey2 = getCacheKey(publicKey, dataKey);
+    expect(client.revisionNumberCache[cacheKey2].toString()).toEqual("1");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
+    mock.onGet(skylinkNewUrl).replyOnce(200, skynetJsonNew, headersNew);
+    const { data: receivedJson2 } = await client.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJson2).toEqual(jsonNew);
+
+    expect(mock.history.get.length).toBe(4);
+    expect(mock.history.post.length).toBe(5);
+  });
+});
diff --git a/src/utils/encoding.ts b/src/utils/encoding.ts
index 44ee0d92..08662e25 100644
--- a/src/utils/encoding.ts
+++ b/src/utils/encoding.ts
@@ -3,7 +3,9 @@ import base32Encode from "base32-encode";
 import { fromByteArray, toByteArray } from "base64-js";
 
 import { assertUint64 } from "./number";
+import { BASE32_ENCODED_SKYLINK_SIZE, BASE64_ENCODED_SKYLINK_SIZE } from "../skylink/sia";
 import { stringToUint8ArrayUtf8 } from "./string";
+import { validateStringLen } from "./validation";
 
 const BASE32_ENCODING_VARIANT = "RFC4648-HEX";
 
@@ -14,6 +16,7 @@ const BASE32_ENCODING_VARIANT = "RFC4648-HEX";
  * @returns - The decoded bytes.
  */
 export function decodeSkylinkBase32(skylink: string): Uint8Array {
+  validateStringLen("skylink", skylink, "parameter", BASE32_ENCODED_SKYLINK_SIZE);
   skylink = skylink.toUpperCase();
   const bytes = base32Decode(skylink, BASE32_ENCODING_VARIANT);
   return new Uint8Array(bytes);
@@ -36,6 +39,7 @@ export function encodeSkylinkBase32(bytes: Uint8Array): string {
  * @returns - The decoded bytes.
  */
 export function decodeSkylinkBase64(skylink: string): Uint8Array {
+  validateStringLen("skylink", skylink, "parameter", BASE64_ENCODED_SKYLINK_SIZE);
   // Add padding.
   skylink = `${skylink}==`;
   // Convert from URL encoding.
diff --git a/tsconfig.json b/tsconfig.json
index b5bc863b..c1be2556 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,6 +1,7 @@
 {
   "compilerOptions": {
     "downlevelIteration": true,
+    "lib": ["dom", "es2020"],
     "noEmit": true,
     "esModuleInterop": true,
     "strict": true,

From 3bebcad7dda14fbc201ce2f291c5cd88e09374bf Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 10 Nov 2021 16:38:38 -0600
Subject: [PATCH 11/31] Protect concurrent SkyDB method calls on the same entry
 with mutexes

---
 package.json       |   1 +
 src/client.ts      |  50 ++++++++-
 src/mysky/index.ts |  86 +++++++++------
 src/skydb.ts       | 268 ++++++++++++++++++++++-----------------------
 yarn.lock          |  12 ++
 5 files changed, 244 insertions(+), 173 deletions(-)

diff --git a/package.json b/package.json
index 9e736ba6..964f60ca 100644
--- a/package.json
+++ b/package.json
@@ -52,6 +52,7 @@
   },
   "homepage": "https://github.com/SkynetLabs/skynet-js",
   "dependencies": {
+    "async-mutex": "^0.3.2",
     "axios": "^0.24.0",
     "base32-decode": "^1.0.0",
     "base32-encode": "^1.1.1",
diff --git a/src/client.ts b/src/client.ts
index 347d6a75..f98dec62 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -1,3 +1,4 @@
+import { Mutex } from "async-mutex";
 import axios from "axios";
 import type { AxiosResponse, ResponseType, Method } from "axios";
 
@@ -104,8 +105,9 @@ export class SkynetClient {
   // The custom portal URL, if one was passed in to `new SkynetClient()`.
   protected customPortalUrl?: string;
 
-  // Holds the cached revision numbers.
-  revisionNumberCache: { [key: string]: bigint } = {};
+  // Holds the cached revision numbers, protected by mutexes to prevent
+  // concurrent access.
+  revisionNumberCache = new RevisionNumberCache();
 
   // Set methods (defined in other files).
 
@@ -312,6 +314,50 @@ export class SkynetClient {
   }
 }
 
+// =====================
+// Revision Number Cache
+// =====================
+
+class RevisionNumberCache {
+  mutex = new Mutex();
+  cache: { [key: string]: CachedRevisionEntry } = {};
+
+  async getRevisionAndMutexForEntry(publicKey: string, dataKey: string): Promise<CachedRevisionEntry> {
+    const cacheKey = getCacheKey(publicKey, dataKey);
+
+    // Block until the mutex is available for the cache.
+    return await this.mutex.runExclusive(async () => {
+      const cachedValue = this.cache[cacheKey];
+
+      if (!cachedValue) {
+        // Initialize a new cached entry and return that.
+        const newValue = new CachedRevisionEntry();
+        this.cache[cacheKey] = newValue;
+        return newValue;
+      } else {
+        // Return the cached entry.
+        return cachedValue;
+      }
+    });
+  }
+}
+
+export class CachedRevisionEntry {
+  mutex = new Mutex();
+  revision = BigInt(-1);
+}
+
+/**
+ * Gets the revision cache key for the given public key and data key.
+ *
+ * @param publicKey - The given public key.
+ * @param dataKey - The given data key.
+ * @returns - The revision cache key.
+ */
+function getCacheKey(publicKey: string, dataKey: string): string {
+  return `${publicKey}/${dataKey}`;
+}
+
 // =======
 // Helpers
 // =======
diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 356a5ffe..f2100c48 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -3,6 +3,7 @@
 export type { CustomConnectorOptions } from "./connector";
 export { DacLibrary } from "./dac";
 
+import { tryAcquire } from "async-mutex";
 import { Connection, ParentHandshake, WindowMessenger } from "post-me";
 import {
   CheckPermissionsResponse,
@@ -35,8 +36,7 @@ import {
   CustomSetEntryDataOptions,
   DEFAULT_SET_ENTRY_DATA_OPTIONS,
   DELETION_ENTRY_DATA,
-  incrementCachedRevision,
-  decrementCachedRevision,
+  incrementRevision,
 } from "../skydb";
 import { Signature } from "../crypto";
 import { deriveDiscoverableFileTweak } from "./tweak";
@@ -326,6 +326,10 @@ export class MySky {
     return await this.connector.connection.remoteHandle().call("userID");
   }
 
+  // =============
+  // SkyDB methods
+  // =============
+
   /**
    * Gets Discoverable JSON at the given path through MySky, if the user has
    * given Discoverable Read permissions to do so.
@@ -395,27 +399,35 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Get the cached revision number before doing anything else.
-    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
+    // Safely get or create mutex for the requested entry.
+    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+      publicKey,
+      dataKey
+    );
+
+    // Immediately fail if the mutex is not available
+    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+      // Get the cached revision number before doing anything else.
+      const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
-    let entry, dataLink;
-    try {
       // Call SkyDB helper to create the registry entry. We can't call SkyDB's
       // setJSON here directly because we need MySky to sign the entry, instead of
       // signing it ourselves with a given private key.
-      [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this.connector.client, dataKey, json, newRevision, opts);
+      const [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(
+        this.connector.client,
+        dataKey,
+        json,
+        newRevision,
+        opts
+      );
 
       const signature = await this.signRegistryEntry(entry, path);
 
       const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
       await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
-    } catch (e) {
-      // Something failed, revert the cached revision number increment.
-      decrementCachedRevision(this.connector.client, publicKey, dataKey);
-      throw e;
-    }
 
-    return { data: json, dataLink };
+      return { data: json, dataLink };
+    });
   }
 
   /**
@@ -442,6 +454,10 @@ export class MySky {
     await this.setEntryData(path, DELETION_ENTRY_DATA, { ...opts, allowDeletionEntryData: true });
   }
 
+  // ==================
+  // Entry Data Methods
+  // ==================
+
   /**
    * Sets entry at the given path to point to the data link. Like setJSON, but it doesn't upload a file.
    *
@@ -517,23 +533,26 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Get the cached revision number before doing anything else.
-    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
+    // Safely get or create mutex for the requested entry.
+    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+      publicKey,
+      dataKey
+    );
+
+    // Immediately fail if the mutex is not available
+    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+      // Get the cached revision number before doing anything else.
+      const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
-    const entry = { dataKey, data, revision: newRevision };
+      const entry = { dataKey, data, revision: newRevision };
 
-    try {
       const signature = await this.signRegistryEntry(entry, path);
 
       const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
       await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
-    } catch (e) {
-      // Something failed, revert the cached revision number increment.
-      decrementCachedRevision(this.connector.client, publicKey, dataKey);
-      throw e;
-    }
 
-    return { data: entry.data };
+      return { data: entry.data };
+    });
   }
 
   /**
@@ -651,10 +670,17 @@ export class MySky {
     const dataKey = deriveEncryptedFileTweak(pathSeed);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Get the cached revision number before doing anything else.
-    const newRevision = incrementCachedRevision(this.connector.client, publicKey, dataKey);
+    // Safely get or create mutex for the requested entry.
+    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+      publicKey,
+      dataKey
+    );
+
+    // Immediately fail if the mutex is not available
+    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+      // Get the cached revision number before doing anything else.
+      const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
-    try {
       // Derive the key.
       const encryptionKey = deriveEncryptedFileKeyEntropy(pathSeed);
 
@@ -668,13 +694,9 @@ export class MySky {
 
       const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
       await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
-    } catch (e) {
-      // Something failed, revert the cached revision number increment.
-      decrementCachedRevision(this.connector.client, publicKey, dataKey);
-      throw e;
-    }
 
-    return { data: json };
+      return { data: json };
+    });
   }
 
   // ================
diff --git a/src/skydb.ts b/src/skydb.ts
index b8f69135..e6ee23f4 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -1,6 +1,7 @@
+import { tryAcquire } from "async-mutex";
 import { sign } from "tweetnacl";
 
-import { SkynetClient } from "./client";
+import { CachedRevisionEntry, SkynetClient } from "./client";
 import { DEFAULT_DOWNLOAD_OPTIONS, CustomDownloadOptions } from "./download";
 import {
   DEFAULT_GET_ENTRY_OPTIONS,
@@ -161,42 +162,54 @@ export async function getJSON(
     ...customOptions,
   };
 
-  // Lookup the registry entry.
-  const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entry: RegistryEntry | null = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, getEntryOpts);
-  if (entry === null) {
-    return { data: null, dataLink: null };
-  }
-
-  // Determine the data link.
-  // TODO: Can this still be an entry link which hasn't yet resolved to a data link?
-  const { rawDataLink, dataLink } = parseDataLink(entry.data, true);
-
-  // If a cached data link is provided and the data link hasn't changed, return.
-  if (checkCachedDataLink(rawDataLink, opts.cachedDataLink)) {
-    return { data: null, dataLink };
-  }
-
-  // Download the data in the returned data link.
-  const downloadOpts = extractOptions(opts, DEFAULT_DOWNLOAD_OPTIONS);
-  const { data }: { data: JsonData | SkynetJson } = await this.getFileContent<JsonData>(dataLink, downloadOpts);
-
-  // Validate that the returned data is JSON.
-  if (typeof data !== "object" || data === null) {
-    throw new Error(`File data for the entry at data key '${dataKey}' is not JSON.`);
-  }
-
-  if (!(data["_data"] && data["_v"])) {
-    // Legacy data prior to skynet-js v4, return as-is.
-    return { data, dataLink };
-  }
-
-  // Extract the JSON from the returned SkynetJson.
-  const actualData = data["_data"];
-  if (typeof actualData !== "object" || data === null) {
-    throw new Error(`File data '_data' for the entry at data key '${dataKey}' is not JSON.`);
-  }
-  return { data: actualData as JsonData, dataLink };
+  // Safely get or create mutex for the requested entry.
+  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+
+  // Immediately fail if the mutex is not available
+  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+    // Lookup the registry entry.
+    const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
+    const entry: RegistryEntry | null = await getSkyDBRegistryEntryAndUpdateCache(
+      this,
+      publicKey,
+      dataKey,
+      cachedRevisionEntry,
+      getEntryOpts
+    );
+    if (entry === null) {
+      return { data: null, dataLink: null };
+    }
+
+    // Determine the data link.
+    // TODO: Can this still be an entry link which hasn't yet resolved to a data link?
+    const { rawDataLink, dataLink } = parseDataLink(entry.data, true);
+
+    // If a cached data link is provided and the data link hasn't changed, return.
+    if (checkCachedDataLink(rawDataLink, opts.cachedDataLink)) {
+      return { data: null, dataLink };
+    }
+
+    // Download the data in the returned data link.
+    const downloadOpts = extractOptions(opts, DEFAULT_DOWNLOAD_OPTIONS);
+    const { data }: { data: JsonData | SkynetJson } = await this.getFileContent<JsonData>(dataLink, downloadOpts);
+
+    // Validate that the returned data is JSON.
+    if (typeof data !== "object" || data === null) {
+      throw new Error(`File data for the entry at data key '${dataKey}' is not JSON.`);
+    }
+
+    if (!(data["_data"] && data["_v"])) {
+      // Legacy data prior to skynet-js v4, return as-is.
+      return { data, dataLink };
+    }
+
+    // Extract the JSON from the returned SkynetJson.
+    const actualData = data["_data"];
+    if (typeof actualData !== "object" || data === null) {
+      throw new Error(`File data '_data' for the entry at data key '${dataKey}' is not JSON.`);
+    }
+    return { data: actualData as JsonData, dataLink };
+  });
 }
 
 /**
@@ -235,23 +248,25 @@ export async function setJSON(
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
   const publicKey = toHexString(publicKeyArray);
 
-  // Get the cached revision number before doing anything else.
-  const newRevision = incrementCachedRevision(this, publicKey, dataKey);
+  // Safely get or create mutex for the requested entry.
+  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
 
-  let entry, dataLink;
-  try {
-    [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this, dataKey, json, newRevision, opts);
+  // Immediately fail if the mutex is not available
+  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+    // Get the cached revision number before doing anything else.
+    const newRevision = incrementRevision(cachedRevisionEntry.revision);
+
+    const [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this, dataKey, json, newRevision, opts);
 
     // Update the registry.
     const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
     await this.registry.setEntry(privateKey, entry, setEntryOpts);
-  } catch (e) {
-    // Something failed, revert the cached revision number increment.
-    decrementCachedRevision(this, publicKey, dataKey);
-    throw e;
-  }
 
-  return { data: json, dataLink: formatSkylink(dataLink) };
+    // Update the cached revision number.
+    cachedRevisionEntry.revision = newRevision;
+
+    return { data: json, dataLink: formatSkylink(dataLink) };
+  });
 }
 
 /**
@@ -341,11 +356,17 @@ export async function getEntryData(
     ...customOptions,
   };
 
-  const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, opts);
-  if (entry === null) {
-    return { data: null };
-  }
-  return { data: entry.data };
+  // Safely get or create mutex for the requested entry.
+  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+
+  // Immediately fail if the mutex is not available
+  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+    const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, cachedRevisionEntry, opts);
+    if (entry === null) {
+      return { data: null };
+    }
+    return { data: entry.data };
+  });
 }
 
 /**
@@ -385,21 +406,24 @@ export async function setEntryData(
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
   const publicKey = toHexString(publicKeyArray);
 
-  // Get the cached revision number before doing anything else.
-  const newRevision = incrementCachedRevision(this, publicKey, dataKey);
+  // Safely get or create mutex for the requested entry.
+  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
 
-  const entry = { dataKey, data, revision: newRevision };
+  // Immediately fail if the mutex is not available
+  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+    // Get the cached revision number.
+    const newRevision = incrementRevision(cachedRevisionEntry.revision);
+
+    const entry = { dataKey, data, revision: newRevision };
 
-  try {
     const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
     await this.registry.setEntry(privateKey, entry, setEntryOpts);
-  } catch (e) {
-    // Something failed, revert the cached revision number increment.
-    decrementCachedRevision(this, publicKey, dataKey);
-    throw e;
-  }
 
-  return { data: entry.data };
+    // Update the cached revision number.
+    cachedRevisionEntry.revision = newRevision;
+
+    return { data: entry.data };
+  });
 }
 
 /**
@@ -463,30 +487,42 @@ export async function getRawBytes(
     ...customOptions,
   };
 
-  // Lookup the registry entry.
-  const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
-  const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, getEntryOpts);
-  if (entry === null) {
-    return { data: null, dataLink: null };
-  }
-
-  // Determine the data link.
-  // TODO: Can this still be an entry link which hasn't yet resolved to a data link?
-  const { rawDataLink, dataLink } = parseDataLink(entry.data, false);
-
-  // If a cached data link is provided and the data link hasn't changed, return.
-  if (checkCachedDataLink(rawDataLink, opts.cachedDataLink)) {
-    return { data: null, dataLink };
-  }
-
-  // Download the data in the returned data link.
-  const downloadOpts = {
-    ...extractOptions(opts, DEFAULT_DOWNLOAD_OPTIONS),
-    responseType: "arraybuffer" as ResponseType,
-  };
-  const { data: buffer } = await this.getFileContent<ArrayBuffer>(dataLink, downloadOpts);
-
-  return { data: new Uint8Array(buffer), dataLink };
+  // Safely get or create mutex for the requested entry.
+  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+
+  // Immediately fail if the mutex is not available
+  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+    // Lookup the registry entry.
+    const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
+    const entry = await getSkyDBRegistryEntryAndUpdateCache(
+      this,
+      publicKey,
+      dataKey,
+      cachedRevisionEntry,
+      getEntryOpts
+    );
+    if (entry === null) {
+      return { data: null, dataLink: null };
+    }
+
+    // Determine the data link.
+    // TODO: Can this still be an entry link which hasn't yet resolved to a data link?
+    const { rawDataLink, dataLink } = parseDataLink(entry.data, false);
+
+    // If a cached data link is provided and the data link hasn't changed, return.
+    if (checkCachedDataLink(rawDataLink, opts.cachedDataLink)) {
+      return { data: null, dataLink };
+    }
+
+    // Download the data in the returned data link.
+    const downloadOpts = {
+      ...extractOptions(opts, DEFAULT_DOWNLOAD_OPTIONS),
+      responseType: "arraybuffer" as ResponseType,
+    };
+    const { data: buffer } = await this.getFileContent<ArrayBuffer>(dataLink, downloadOpts);
+
+    return { data: new Uint8Array(buffer), dataLink };
+  });
 }
 
 // =======
@@ -554,53 +590,6 @@ export async function getOrCreateSkyDBRegistryEntry(
   return [entry, formatSkylink(dataLink)];
 }
 
-/**
- * Gets the revision cache key for the given public key and data key.
- *
- * @param publicKey - The given public key.
- * @param dataKey - The given data key.
- * @returns - The revision cache key.
- */
-export function getCacheKey(publicKey: string, dataKey: string): string {
-  return `${publicKey}/${dataKey}`;
-}
-
-/**
- * Decrements the revision number in the cache for the given entry.
- *
- * @param client - The Skynet client.
- * @param publicKey - The user public key.
- * @param dataKey - The data key.
- */
-export function decrementCachedRevision(client: SkynetClient, publicKey: string, dataKey: string): void {
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  client.revisionNumberCache[cacheKey] -= BigInt(1);
-}
-
-/**
- * Increments the revision number in the cache for the given entry and returns
- * the new revision number.
- *
- * @param client - The Skynet client.
- * @param publicKey - The user public key.
- * @param dataKey - The data key.
- * @returns - The new revision number.
- * @throws - Will throw if the revision is already the maximum value.
- */
-export function incrementCachedRevision(client: SkynetClient, publicKey: string, dataKey: string): bigint {
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  const cachedRevision = client.revisionNumberCache[cacheKey];
-
-  // Get the new revision by incrementing the one in the cache, or use 0 if not cached.
-  const revision: bigint = cachedRevision ?? UNCACHED_REVISION_NUMBER;
-  const newRevision = incrementRevision(revision);
-
-  // Update the cached revision number.
-  client.revisionNumberCache[cacheKey] = newRevision;
-
-  return newRevision;
-}
-
 /**
  * Increments the given revision number and checks to make sure it is not
  * greater than the maximum revision.
@@ -609,7 +598,7 @@ export function incrementCachedRevision(client: SkynetClient, publicKey: string,
  * @returns - The incremented revision number.
  * @throws - Will throw if the incremented revision number is greater than the maximum revision.
  */
-function incrementRevision(revision: bigint): bigint {
+export function incrementRevision(revision: bigint): bigint {
   revision = revision + BigInt(1);
 
   // Throw if the revision is already the maximum value.
@@ -672,6 +661,7 @@ export function validateEntryData(data: Uint8Array, allowDeletionEntryData: bool
  * @param client - The Skynet Client
  * @param publicKey - The user public key.
  * @param dataKey - The key of the data to fetch for the given user.
+ * @param cachedRevisionEntry - The cached revision entry object containing the revision number and the mutex.
  * @param opts - Additional settings.
  * @returns - The registry entry, or null if not found or deleted.
  */
@@ -679,6 +669,7 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   client: SkynetClient,
   publicKey: string,
   dataKey: string,
+  cachedRevisionEntry: CachedRevisionEntry,
   opts: CustomGetEntryOptions
 ): Promise<RegistryEntry | null> {
   // If this throws due to a parse error or network error, exit early and do not
@@ -690,18 +681,17 @@ async function getSkyDBRegistryEntryAndUpdateCache(
     return null;
   }
 
-  // Calculate the new revision and get the cached revision.
-  const cacheKey = getCacheKey(publicKey, dataKey);
-  const cachedRevision = client.revisionNumberCache[cacheKey];
+  // Calculate the new revision.
   const newRevision = entry?.revision ?? UNCACHED_REVISION_NUMBER + BigInt(1);
 
   // Don't update the cached revision number if the received version is too low. Throw error.
+  const cachedRevision = cachedRevisionEntry.revision;
   if (cachedRevision && cachedRevision > newRevision) {
     throw new Error("A higher revision number for this userID and path is already cached");
   }
 
   // Update the cached revision.
-  client.revisionNumberCache[cacheKey] = newRevision;
+  cachedRevisionEntry.revision = newRevision;
 
   // Return null if the entry contained a sentinel value indicating deletion.
   // We do this after updating the revision number cache.
diff --git a/yarn.lock b/yarn.lock
index 44801bfd..a44257c4 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1006,6 +1006,13 @@ astral-regex@^2.0.0:
   resolved "https://registry.yarnpkg.com/astral-regex/-/astral-regex-2.0.0.tgz#483143c567aeed4785759c0865786dc77d7d2e31"
   integrity sha512-Z7tMw1ytTXt5jqMcOP+OQteU1VuNK9Y02uuJtKQ1Sv69jXQKKg5cibLwGJow8yzZP+eAc18EmLGPal0bp36rvQ==
 
+async-mutex@^0.3.2:
+  version "0.3.2"
+  resolved "https://registry.yarnpkg.com/async-mutex/-/async-mutex-0.3.2.tgz#1485eda5bda1b0ec7c8df1ac2e815757ad1831df"
+  integrity sha512-HuTK7E7MT7jZEh1P9GtRW9+aTWiDWWi9InbZ5hjxrnRa39KS4BW04+xLBhYNS2aXhHUIKZSw3gj4Pn1pj+qGAA==
+  dependencies:
+    tslib "^2.3.1"
+
 asynckit@^0.4.0:
   version "0.4.0"
   resolved "https://registry.yarnpkg.com/asynckit/-/asynckit-0.4.0.tgz#c79ed97f7f34cb8f2ba1bc9790bcc366474b4b79"
@@ -4572,6 +4579,11 @@ tslib@^1.8.1, tslib@^1.9.0:
   resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.14.1.tgz#cf2d38bdc34a134bcaf1091c41f6619e2f672d00"
   integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==
 
+tslib@^2.3.1:
+  version "2.3.1"
+  resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.3.1.tgz#e8a335add5ceae51aa261d32a490158ef042ef01"
+  integrity sha512-77EbyPPpMz+FRFRuAFlWMtmgUWGe9UOG2Z25NqCwiIjRhOf5iKGuzSe5P2w1laq+FkRy4p+PCuVkJSGkzTEKVw==
+
 tsutils@^3.21.0:
   version "3.21.0"
   resolved "https://registry.yarnpkg.com/tsutils/-/tsutils-3.21.0.tgz#b48717d394cea6c1e096983eed58e9d61715b623"

From 2ab776e840d27a7a626b1a998cf794c12c9fb2e4 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 11 Nov 2021 10:42:07 -0600
Subject: [PATCH 12/31] Update tests

---
 integration/skydb.test.ts |  87 ++++++++-------
 src/skydb.test.ts         | 224 ++++++++++++++++++++++++++++++--------
 2 files changed, 225 insertions(+), 86 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 14f32c58..e4902ad4 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -3,7 +3,6 @@ import { AxiosError } from "axios";
 import { client, dataKey, portal } from ".";
 import { genKeyPairAndSeed, getEntryLink, SkynetClient, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
-import { getCacheKey } from "../src/skydb";
 import { decodeSkylinkBase64 } from "../src/utils/encoding";
 import { toHexString } from "../src/utils/string";
 
@@ -246,22 +245,19 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
   it("Should update the revision number cache", async () => {
     const { publicKey, privateKey } = genKeyPairAndSeed();
     const json = { message: 1 };
-    const cacheKey = getCacheKey(publicKey, dataKey);
 
     await client.db.setJSON(privateKey, dataKey, json);
 
-    let revisionNumber = client.revisionNumberCache[cacheKey];
-    expect(revisionNumber.toString()).toEqual("0");
+    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    expect(cachedRevisionEntry.revision.toString()).toEqual("0");
 
     await client.db.setJSON(privateKey, dataKey, json);
 
-    revisionNumber = client.revisionNumberCache[cacheKey];
-    expect(revisionNumber.toString()).toEqual("1");
+    expect(cachedRevisionEntry.revision.toString()).toEqual("1");
 
     await client.db.getJSON(publicKey, dataKey);
 
-    revisionNumber = client.revisionNumberCache[cacheKey];
-    expect(revisionNumber.toString()).toEqual("1");
+    expect(cachedRevisionEntry.revision.toString()).toEqual("1");
   });
 
   // REGRESSION TESTS: By creating a gap between setJSON and getJSON, a user
@@ -276,7 +272,8 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     const jsonOld = { message: 1 };
     const jsonNew = { message: 2 };
 
-    const delays = [0, 100, 200, 300, 500, 1000];
+    // const delays = [0, 100, 200, 300, 500, 1000];
+    const delays = [100];
 
     it.each(delays)(
       "should not get old data when getJSON is called after setJSON on a single client with a '%s' ms delay and getJSON doesn't fail",
@@ -297,7 +294,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           };
           [{ data: receivedJson }] = await Promise.all([getJSONFn(), client.db.setJSON(privateKey, dataKey, jsonNew)]);
         } catch (e) {
-          if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
+          if ((e as Error).message.includes("mutex already locked")) {
             // The data race condition has been prevented and we received the expected error. Return from test early.
             return;
           }
@@ -312,42 +309,54 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     );
 
     it.each(delays)(
-      "should not get old data when getJSON is called after setJSON on two different clients with a '%s' ms delay and getJSON doesn't fail",
+      "should not be able to use old data when getJSON is called after setJSON on two different clients with a '%s' ms delay",
       async (delay) => {
         // Create two new clients with a fresh revision cache.
         const client1 = new SkynetClient(portal);
         const client2 = new SkynetClient(portal);
         const { publicKey, privateKey } = genKeyPairAndSeed();
 
-        // Use a random client to set the initial data.
-        if (Math.random() < 0.5) {
-          await client1.db.setJSON(privateKey, dataKey, jsonOld);
-        } else {
-          await client2.db.setJSON(privateKey, dataKey, jsonOld);
+        // Set the initial data.
+        await client1.db.setJSON(privateKey, dataKey, jsonOld);
+
+        // Call getJSON and setJSON concurrently on different clients -- both
+        // should succeeed.
+
+        // Get the data while also calling setJSON.
+        const getJSONFn = async function () {
+          // Sleep.
+          await new Promise((r) => setTimeout(r, delay));
+          return await client2.db.getJSON(publicKey, dataKey);
+        };
+        const [{ data: receivedJson }] = await Promise.all([
+          getJSONFn(),
+          client1.db.setJSON(privateKey, dataKey, jsonNew),
+        ]);
+
+        // If we got old data from getJSON, make sure that that client is not
+        // able to update that JSON.
+
+        expect(receivedJson).not.toBeNull();
+        if (receivedJson === jsonNew) {
+          return;
         }
-
-        // Try to invoke the data race.
-        let receivedJson;
-        try {
-          // Get the data while also calling setJSON.
-          const getJSONFn = async function () {
-            // Sleep.
-            await new Promise((r) => setTimeout(r, delay));
-            return await client2.db.getJSON(publicKey, dataKey);
-          };
-          [{ data: receivedJson }] = await Promise.all([getJSONFn(), client1.db.setJSON(privateKey, dataKey, jsonNew)]);
-        } catch (e) {
-          if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
-            // The data race condition has been prevented and we received the expected error. Return from test early.
-            return;
-          }
-
-          // Unexpected error, throw.
-          throw e;
-        }
-
-        // Data race did not occur, getJSON should have latest JSON.
-        expect(receivedJson).toEqual(jsonNew);
+        expect(receivedJson).toEqual(jsonOld);
+
+        const updatedJson = receivedJson as { message: number };
+        updatedJson.message--;
+        // Catches both "doesn't have enough pow" and "provided revision number
+        // is already registered" errors.
+        await expect(client2.db.setJSON(privateKey, dataKey, updatedJson)).rejects.toThrowError(
+          "Unable to update the registry"
+        );
+
+        // Should work on that client again after calling getJSON.
+
+        const { data: receivedJson2 } = await client2.db.getJSON(publicKey, dataKey);
+        expect(receivedJson2).toEqual(jsonNew);
+        const updatedJson2 = receivedJson2 as { message: number };
+        updatedJson2.message++;
+        await client2.db.setJSON(privateKey, dataKey, updatedJson2);
       }
     );
   });
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 2c932758..ed82b2f1 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -7,7 +7,7 @@ import { stringToUint8ArrayUtf8, toHexString } from "./utils/string";
 import { DEFAULT_SKYNET_PORTAL_URL, URI_SKYNET_PREFIX } from "./utils/url";
 import { SkynetClient } from "./index";
 import { getEntryUrlForPortal } from "./registry";
-import { checkCachedDataLink, DELETION_ENTRY_DATA, getCacheKey } from "./skydb";
+import { checkCachedDataLink, DELETION_ENTRY_DATA, JSONResponse } from "./skydb";
 import { MAX_ENTRY_LENGTH } from "./mysky";
 
 // Generated with genKeyPairFromSeed("insecure test seed")
@@ -198,8 +198,8 @@ describe("setJSON", () => {
 
   it("should fail if the entry has the maximum allowed revision", async () => {
     const dataKey = "maximum revision";
-    const cacheKey = getCacheKey(publicKey, dataKey);
-    client.revisionNumberCache[cacheKey] = MAX_REVISION;
+    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    cachedRevisionEntry.revision = MAX_REVISION;
 
     // mock a successful registry update
     mock.onPost(registryPostUrl).replyOnce(204);
@@ -232,7 +232,6 @@ describe("setJSON", () => {
 
   it("Should not update the cached revision if the registry update fails.", async () => {
     const dataKey = "registry failure";
-    const cacheKey = getCacheKey(publicKey, dataKey);
     const json = { foo: "bar" };
 
     // mock a successful registry update
@@ -240,14 +239,15 @@ describe("setJSON", () => {
 
     await client.db.setJSON(privateKey, dataKey, json);
 
-    const revision1 = client.revisionNumberCache[cacheKey];
+    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    const revision1 = cachedRevisionEntry.revision;
 
     // mock a failed registry update
     mock.onPost(registryPostUrl).replyOnce(400, JSON.stringify({ message: "foo" }));
 
     await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toEqual(new Error("foo"));
 
-    const revision2 = client.revisionNumberCache[cacheKey];
+    const revision2 = cachedRevisionEntry.revision;
 
     expect(revision1.toString()).toEqual(revision2.toString());
   });
@@ -301,9 +301,10 @@ describe("getJSON/setJSON data race regression unit tests", () => {
   let mock: MockAdapter;
 
   beforeEach(() => {
-    mock = new MockAdapter(axios);
+    // Add a delay to responses to simulate actual calls that use the network.
+    mock = new MockAdapter(axios, { delayResponse: 100 });
+    mock.reset();
     mock.onHead(portalUrl).replyOnce(200, {}, { "skynet-portal-api": portalUrl });
-    mock.resetHistory();
   });
 
   const skylinkOld = "XABvi7JtJbQSMAcDwnUnmp2FKDPjg8_tTTFP4BwMSxVdEg";
@@ -335,6 +336,10 @@ describe("getJSON/setJSON data race regression unit tests", () => {
   const skynetJsonOld = { _data: jsonOld, _v: 2 };
   const skynetJsonNew = { _data: jsonNew, _v: 2 };
 
+  // TODO: Improve this error message.
+  const concurrentAccessError = "mutex already locked";
+  const higherRevisionError = "A higher revision number for this userID and path is already cached";
+
   it("should not get old data when getJSON and setJSON are called simultaneously on the same client and getJSON doesn't fail", async () => {
     // Create a new client with a fresh revision cache.
     const client = new SkynetClient(portalUrl);
@@ -355,25 +360,23 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(registryPostUrl).replyOnce(204);
 
     // Try to invoke the data race.
-    let receivedJson;
-    try {
-      // Get the data while also calling setJSON.
-      [{ data: receivedJson }] = await Promise.all([
-        client.db.getJSON(publicKey, dataKey),
-        client.db.setJSON(privateKey, dataKey, jsonNew),
-      ]);
-    } catch (e) {
-      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
-        // The data race condition has been prevented and we received the expected error. Return from test early.
-        return;
-      }
+    // Get the data while also calling setJSON.
+    // Use Promise.allSettled to wait for all promises to finish, or some mocked requests will hang around and interfere with the later tests.
+    const values = await Promise.allSettled([
+      client.db.getJSON(publicKey, dataKey),
+      client.db.setJSON(privateKey, dataKey, jsonNew),
+    ]);
 
-      // Unexpected error, throw.
-      throw e;
+    // If any promises were rejected, check the error message.
+    const data = checkSettledValuesForErrorOrValue<JSONResponse>(values, concurrentAccessError);
+    if (!data) {
+      // The data race condition was avoided and we received the expected
+      // error. Return from test early.
+      return;
     }
 
     // Data race did not occur, getJSON should have latest JSON.
-    expect(receivedJson).toEqual(jsonNew);
+    expect(data.data).toEqual(jsonNew);
 
     // assert our request history contains the expected amount of requests
     expect(mock.history.get.length).toBe(2);
@@ -401,25 +404,23 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(registryPostUrl).replyOnce(204);
 
     // Try to invoke the data race.
-    let receivedJson;
-    try {
-      // Get the data while also calling setJSON.
-      [{ data: receivedJson }] = await Promise.all([
-        client2.db.getJSON(publicKey, dataKey),
-        client1.db.setJSON(privateKey, dataKey, jsonNew),
-      ]);
-    } catch (e) {
-      if ((e as Error).message.includes("A higher revision number for this userID and path is already cached")) {
-        // The data race condition has been prevented and we received the expected error. Return from test early.
-        return;
-      }
+    // Get the data while also calling setJSON.
+    // Use Promise.allSettled to wait for all promises to finish, or some mocked requests will hang around and interfere with the later tests.
+    const values = await Promise.allSettled([
+      client1.db.getJSON(publicKey, dataKey),
+      client2.db.setJSON(privateKey, dataKey, jsonNew),
+    ]);
 
-      // Unexpected error, throw.
-      throw e;
+    // If any promises were rejected, check the error message.
+    const data = checkSettledValuesForErrorOrValue<JSONResponse>(values, higherRevisionError);
+    if (!data) {
+      // The data race condition was avoided and we received the expected
+      // error. Return from test early.
+      return;
     }
 
     // Data race did not occur, getJSON should have latest JSON.
-    expect(receivedJson).toEqual(jsonNew);
+    expect(data.data).toEqual(jsonNew);
 
     // assert our request history contains the expected amount of requests.
     expect(mock.history.get.length).toBe(2);
@@ -434,16 +435,15 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
     mock.onPost(registryPostUrl).replyOnce(204);
 
-    // Mock setJSON that fails to upload.
-    mock.onPost(uploadUrl).replyOnce(400);
-
-    await Promise.allSettled([
+    const values = await Promise.allSettled([
       client.db.setJSON(privateKey, dataKey, jsonOld),
       client.db.setJSON(privateKey, dataKey, jsonOld),
     ]);
 
-    const cacheKey1 = getCacheKey(publicKey, dataKey);
-    expect(client.revisionNumberCache[cacheKey1].toString()).toEqual("0");
+    checkSettledValuesForErrorOrValue<JSONResponse>(values, concurrentAccessError);
+
+    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    expect(cachedRevisionEntry.revision.toString()).toEqual("0");
 
     // Make a getJSON call.
     mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataOld));
@@ -457,8 +457,7 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(registryPostUrl).replyOnce(204);
     await client.db.setJSON(privateKey, dataKey, jsonNew);
 
-    const cacheKey2 = getCacheKey(publicKey, dataKey);
-    expect(client.revisionNumberCache[cacheKey2].toString()).toEqual("1");
+    expect(cachedRevisionEntry.revision.toString()).toEqual("1");
 
     // Make a getJSON call.
     mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
@@ -468,6 +467,137 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     expect(receivedJson2).toEqual(jsonNew);
 
     expect(mock.history.get.length).toBe(4);
-    expect(mock.history.post.length).toBe(5);
+    expect(mock.history.post.length).toBe(4);
   });
+
+  it("should not mess up cache when two setJSON calls are made simultaneously on different clients and one fails", async () => {
+    // Create two new clients with a fresh revision cache.
+    const client1 = new SkynetClient(portalUrl);
+    const client2 = new SkynetClient(portalUrl);
+
+    // Run two simultaneous setJSONs on two different clients - one should work,
+    // one should fail due to bad revision number.
+
+    // Mock a successful setJSON.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+    // Mock a failed setJSON (bad revision number).
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(400);
+
+    const values = await Promise.allSettled([
+      client1.db.setJSON(privateKey, dataKey, jsonOld),
+      client2.db.setJSON(privateKey, dataKey, jsonOld),
+    ]);
+
+    let successClient;
+    let failClient;
+    if (values[0].status === "rejected") {
+      successClient = client2;
+      failClient = client1;
+    } else {
+      successClient = client1;
+      failClient = client2;
+    }
+
+    // Test that the client that succeeded has a consistent cache.
+
+    const cachedRevisionEntrySuccess = await successClient.revisionNumberCache.getRevisionAndMutexForEntry(
+      publicKey,
+      dataKey
+    );
+    expect(cachedRevisionEntrySuccess.revision.toString()).toEqual("0");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataOld));
+    mock.onGet(skylinkOldUrl).replyOnce(200, skynetJsonOld, headersOld);
+    const { data: receivedJson1 } = await successClient.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJson1).toEqual(jsonOld);
+
+    // Make another setJSON call - it should still work.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkNew, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+    await successClient.db.setJSON(privateKey, dataKey, jsonNew);
+
+    expect(cachedRevisionEntrySuccess.revision.toString()).toEqual("1");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
+    mock.onGet(skylinkNewUrl).replyOnce(200, skynetJsonNew, headersNew);
+    const { data: receivedJson2 } = await successClient.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJson2).toEqual(jsonNew);
+
+    // Test that the client that failed has a consistent cache.
+
+    const cachedRevisionEntryFail = await failClient.revisionNumberCache.getRevisionAndMutexForEntry(
+      publicKey,
+      dataKey
+    );
+    expect(cachedRevisionEntryFail.revision.toString()).toEqual("-1");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataOld));
+    mock.onGet(skylinkOldUrl).replyOnce(200, skynetJsonOld, headersOld);
+    const { data: receivedJsonFail1 } = await failClient.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJsonFail1).toEqual(jsonOld);
+
+    // Make another setJSON call - it should still work.
+    mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkNew, merkleroot, bitfield });
+    mock.onPost(registryPostUrl).replyOnce(204);
+    await failClient.db.setJSON(privateKey, dataKey, jsonNew);
+
+    expect(cachedRevisionEntrySuccess.revision.toString()).toEqual("1");
+
+    // Make a getJSON call.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataNew));
+    mock.onGet(skylinkNewUrl).replyOnce(200, skynetJsonNew, headersNew);
+    const { data: receivedJsonFail2 } = await failClient.db.getJSON(publicKey, dataKey);
+
+    expect(receivedJsonFail2).toEqual(jsonNew);
+
+    // Check final request counts.
+
+    expect(mock.history.get.length).toBe(8);
+    expect(mock.history.post.length).toBe(8);
+  });
+
+  /**
+   * Checks the settled values from Promise.allSettled for the given error.
+   * Throws if an unexpected error is found. Returns settled value if no errors
+   * were found.
+   *
+   * @param values - The settled values.
+   * @param err - The err to check for.
+   * @returns - The settled value if no errors were found, or null if the expected error was found.
+   * @throws - Will throw if an unexpected error occurred.
+   */
+  function checkSettledValuesForErrorOrValue<T>(values: PromiseSettledResult<T>[], err: string): T | null {
+    let rejected = false;
+    let reason;
+    let receivedValue: T | null = null;
+    for (const value of values) {
+      if (value.status === "rejected") {
+        rejected = true;
+        reason = value.reason;
+      } else if (value.value) {
+        receivedValue = value.value;
+      }
+    }
+    if (rejected) {
+      // TODO: Don't return early.
+      if ((reason as Error).message.includes(err)) {
+        // The data race condition was avoided and we received the expected
+        // error. Return from test early.
+        return null;
+      } else {
+        // Unexpected error, throw.
+        throw reason as Error;
+      }
+    }
+
+    return receivedValue;
+  }
 });

From a6a3dd8747e361f8b85adf6196cc94d4e5a30625 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 11 Nov 2021 12:00:47 -0600
Subject: [PATCH 13/31] Update returned error to be more descriptive (+
 refactor)

---
 integration/skydb.test.ts |   4 +-
 src/client.ts             |  51 +++++++++++++++-
 src/mysky/index.ts        | 119 ++++++++++++++++++--------------------
 src/skydb.test.ts         |   3 +-
 src/skydb.ts              |  37 ++++--------
 5 files changed, 121 insertions(+), 93 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index e4902ad4..2559a54c 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -275,6 +275,8 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     // const delays = [0, 100, 200, 300, 500, 1000];
     const delays = [100];
 
+    const concurrentAccessError = "Concurrent access prevented in SkyDB";
+
     it.each(delays)(
       "should not get old data when getJSON is called after setJSON on a single client with a '%s' ms delay and getJSON doesn't fail",
       async (delay) => {
@@ -294,7 +296,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           };
           [{ data: receivedJson }] = await Promise.all([getJSONFn(), client.db.setJSON(privateKey, dataKey, jsonNew)]);
         } catch (e) {
-          if ((e as Error).message.includes("mutex already locked")) {
+          if ((e as Error).message.includes(concurrentAccessError)) {
             // The data race condition has been prevented and we received the expected error. Return from test early.
             return;
           }
diff --git a/src/client.ts b/src/client.ts
index f98dec62..1867be96 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -1,4 +1,4 @@
-import { Mutex } from "async-mutex";
+import { Mutex, tryAcquire } from "async-mutex";
 import axios from "axios";
 import type { AxiosResponse, ResponseType, Method } from "axios";
 
@@ -318,10 +318,24 @@ export class SkynetClient {
 // Revision Number Cache
 // =====================
 
+/**
+ * An abstraction over the client's revision number cache. Provides a cache,
+ * keyed by public key and data key and protected by a mutex to guard against
+ * concurrent access to the cache. Each cache entry also has its own mutex, to
+ * protect against concurrent access to that entry.
+ */
 class RevisionNumberCache {
   mutex = new Mutex();
   cache: { [key: string]: CachedRevisionEntry } = {};
 
+  /**
+   * Gets an object containing the cached revision and the mutex for the entry.
+   * The revision and mutex will be initialized if the entry is not yet cached.
+   *
+   * @param publicKey - The given public key.
+   * @param dataKey - The given data key.
+   * @returns - The cached revision entry object.
+   */
   async getRevisionAndMutexForEntry(publicKey: string, dataKey: string): Promise<CachedRevisionEntry> {
     const cacheKey = getCacheKey(publicKey, dataKey);
 
@@ -340,8 +354,43 @@ class RevisionNumberCache {
       }
     });
   }
+
+  /**
+   * Calls `exclusiveFn` with exclusive access to the given cached entry. The
+   * revision number of the entry can be safely updated in `exclusiveFn`.
+   *
+   * @param publicKey - The given public key.
+   * @param dataKey - The given data key.
+   * @param exclusiveFn - A function to call with exclusive access to the given cached entry.
+   * @returns - A promise containing the result of calling `exclusiveFn`.
+   */
+  async withCachedEntryLock<T>(
+    publicKey: string,
+    dataKey: string,
+    exclusiveFn: (cachedRevisionEntry: CachedRevisionEntry) => Promise<T>
+  ): Promise<T> {
+    // Safely get or create mutex for the requested entry.
+    const cachedRevisionEntry = await this.getRevisionAndMutexForEntry(publicKey, dataKey);
+
+    try {
+      return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => exclusiveFn(cachedRevisionEntry));
+    } catch (e) {
+      // Change mutex error to be more descriptive and user-friendly.
+      if ((e as Error).message.includes("mutex already locked")) {
+        throw new Error(
+          `Concurrent access prevented in SkyDB for entry { publicKey: ${publicKey}, dataKey: ${dataKey} }`
+        );
+      } else {
+        throw e;
+      }
+    }
+  }
 }
 
+/**
+ * An object containing a cached revision and a corresponding mutex. The
+ * revision can be internally updated and it will reflect in the client's cache.
+ */
 export class CachedRevisionEntry {
   mutex = new Mutex();
   revision = BigInt(-1);
diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index f2100c48..9bbce9b7 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -3,7 +3,6 @@
 export type { CustomConnectorOptions } from "./connector";
 export { DacLibrary } from "./dac";
 
-import { tryAcquire } from "async-mutex";
 import { Connection, ParentHandshake, WindowMessenger } from "post-me";
 import {
   CheckPermissionsResponse,
@@ -399,35 +398,33 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Safely get or create mutex for the requested entry.
-    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+    // Immediately fail if the mutex is not available.
+    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
       publicKey,
-      dataKey
+      dataKey,
+      async (cachedRevisionEntry) => {
+        // Get the cached revision number before doing anything else.
+        const newRevision = incrementRevision(cachedRevisionEntry.revision);
+
+        // Call SkyDB helper to create the registry entry. We can't call SkyDB's
+        // setJSON here directly because we need MySky to sign the entry, instead of
+        // signing it ourselves with a given private key.
+        const [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(
+          this.connector.client,
+          dataKey,
+          json,
+          newRevision,
+          opts
+        );
+
+        const signature = await this.signRegistryEntry(entry, path);
+
+        const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+        await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+
+        return { data: json, dataLink };
+      }
     );
-
-    // Immediately fail if the mutex is not available
-    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
-      // Get the cached revision number before doing anything else.
-      const newRevision = incrementRevision(cachedRevisionEntry.revision);
-
-      // Call SkyDB helper to create the registry entry. We can't call SkyDB's
-      // setJSON here directly because we need MySky to sign the entry, instead of
-      // signing it ourselves with a given private key.
-      const [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(
-        this.connector.client,
-        dataKey,
-        json,
-        newRevision,
-        opts
-      );
-
-      const signature = await this.signRegistryEntry(entry, path);
-
-      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
-
-      return { data: json, dataLink };
-    });
   }
 
   /**
@@ -533,26 +530,24 @@ export class MySky {
     const dataKey = deriveDiscoverableFileTweak(path);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Safely get or create mutex for the requested entry.
-    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+    // Immediately fail if the mutex is not available.
+    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
       publicKey,
-      dataKey
-    );
-
-    // Immediately fail if the mutex is not available
-    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
-      // Get the cached revision number before doing anything else.
-      const newRevision = incrementRevision(cachedRevisionEntry.revision);
+      dataKey,
+      async (cachedRevisionEntry) => {
+        // Get the cached revision number before doing anything else.
+        const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
-      const entry = { dataKey, data, revision: newRevision };
+        const entry = { dataKey, data, revision: newRevision };
 
-      const signature = await this.signRegistryEntry(entry, path);
+        const signature = await this.signRegistryEntry(entry, path);
 
-      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
+        const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+        await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
 
-      return { data: entry.data };
-    });
+        return { data: entry.data };
+      }
+    );
   }
 
   /**
@@ -670,33 +665,31 @@ export class MySky {
     const dataKey = deriveEncryptedFileTweak(pathSeed);
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
-    // Safely get or create mutex for the requested entry.
-    const cachedRevisionEntry = await this.connector.client.revisionNumberCache.getRevisionAndMutexForEntry(
+    // Immediately fail if the mutex is not available.
+    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
       publicKey,
-      dataKey
-    );
+      dataKey,
+      async (cachedRevisionEntry) => {
+        // Get the cached revision number before doing anything else.
+        const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
-    // Immediately fail if the mutex is not available
-    return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
-      // Get the cached revision number before doing anything else.
-      const newRevision = incrementRevision(cachedRevisionEntry.revision);
+        // Derive the key.
+        const encryptionKey = deriveEncryptedFileKeyEntropy(pathSeed);
 
-      // Derive the key.
-      const encryptionKey = deriveEncryptedFileKeyEntropy(pathSeed);
+        // Pad and encrypt json file.
+        const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
 
-      // Pad and encrypt json file.
-      const data = encryptJSONFile(json, { version: ENCRYPTED_JSON_RESPONSE_VERSION }, encryptionKey);
+        const [entry] = await getOrCreateSkyDBRegistryEntry(this.connector.client, dataKey, data, newRevision, opts);
 
-      const [entry] = await getOrCreateSkyDBRegistryEntry(this.connector.client, dataKey, data, newRevision, opts);
+        // Call MySky which checks for write permissions on the path.
+        const signature = await this.signEncryptedRegistryEntry(entry, path);
 
-      // Call MySky which checks for write permissions on the path.
-      const signature = await this.signEncryptedRegistryEntry(entry, path);
+        const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
+        await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
 
-      const setEntryOpts = extractOptions(opts, DEFAULT_SET_ENTRY_OPTIONS);
-      await this.connector.client.registry.postSignedEntry(publicKey, entry, signature, setEntryOpts);
-
-      return { data: json };
-    });
+        return { data: json };
+      }
+    );
   }
 
   // ================
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index ed82b2f1..1f738fbd 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -336,8 +336,7 @@ describe("getJSON/setJSON data race regression unit tests", () => {
   const skynetJsonOld = { _data: jsonOld, _v: 2 };
   const skynetJsonNew = { _data: jsonNew, _v: 2 };
 
-  // TODO: Improve this error message.
-  const concurrentAccessError = "mutex already locked";
+  const concurrentAccessError = "Concurrent access prevented in SkyDB";
   const higherRevisionError = "A higher revision number for this userID and path is already cached";
 
   it("should not get old data when getJSON and setJSON are called simultaneously on the same client and getJSON doesn't fail", async () => {
diff --git a/src/skydb.ts b/src/skydb.ts
index e6ee23f4..65b20301 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -1,4 +1,3 @@
-import { tryAcquire } from "async-mutex";
 import { sign } from "tweetnacl";
 
 import { CachedRevisionEntry, SkynetClient } from "./client";
@@ -162,11 +161,8 @@ export async function getJSON(
     ...customOptions,
   };
 
-  // Safely get or create mutex for the requested entry.
-  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-  // Immediately fail if the mutex is not available
-  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+  // Immediately fail if the mutex is not available.
+  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Lookup the registry entry.
     const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
     const entry: RegistryEntry | null = await getSkyDBRegistryEntryAndUpdateCache(
@@ -248,11 +244,8 @@ export async function setJSON(
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
   const publicKey = toHexString(publicKeyArray);
 
-  // Safely get or create mutex for the requested entry.
-  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-  // Immediately fail if the mutex is not available
-  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+  // Immediately fail if the mutex is not available.
+  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Get the cached revision number before doing anything else.
     const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
@@ -356,11 +349,8 @@ export async function getEntryData(
     ...customOptions,
   };
 
-  // Safely get or create mutex for the requested entry.
-  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-  // Immediately fail if the mutex is not available
-  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+  // Immediately fail if the mutex is not available.
+  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, cachedRevisionEntry, opts);
     if (entry === null) {
       return { data: null };
@@ -406,11 +396,8 @@ export async function setEntryData(
   const { publicKey: publicKeyArray } = sign.keyPair.fromSecretKey(hexToUint8Array(privateKey));
   const publicKey = toHexString(publicKeyArray);
 
-  // Safely get or create mutex for the requested entry.
-  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-  // Immediately fail if the mutex is not available
-  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+  // Immediately fail if the mutex is not available.
+  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Get the cached revision number.
     const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
@@ -487,11 +474,8 @@ export async function getRawBytes(
     ...customOptions,
   };
 
-  // Safely get or create mutex for the requested entry.
-  const cachedRevisionEntry = await this.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-  // Immediately fail if the mutex is not available
-  return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => {
+  // Immediately fail if the mutex is not available.
+  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Lookup the registry entry.
     const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
     const entry = await getSkyDBRegistryEntryAndUpdateCache(
@@ -687,6 +671,7 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   // Don't update the cached revision number if the received version is too low. Throw error.
   const cachedRevision = cachedRevisionEntry.revision;
   if (cachedRevision && cachedRevision > newRevision) {
+    /* istanbul ignore next - this shouldn't come up in practice */
     throw new Error("A higher revision number for this userID and path is already cached");
   }
 

From f575b83ff9dc395722460cedcd8bb9f9c1a96adc Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 11 Nov 2021 12:32:24 -0600
Subject: [PATCH 14/31] Finish adding tests

Final test suite:

- [X] Integration:
  - [X] setJSON and getJSON called simultaneously (should error)
  - [X] setJSON and getJSON with delays (should error, or getJSON gets new data)
  - [X] two setJSON calls (should error)
  - [X] setJSON and getJSON with two different clients (different local caches)
  - [X] two setJSON calls with two different clients (different local caches)
- [X] Unit:
  - [X] setJSON and getJSON called simultaneously (should error)
  - [X] two setJSON calls (should error)
  - [X] setJSON and getJSON with two different clients (different local caches)
  - [X] two setJSON calls with two different clients (different local caches)
---
 integration/skydb.test.ts | 75 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 5 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 2559a54c..af9c7ebd 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -272,10 +272,10 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     const jsonOld = { message: 1 };
     const jsonNew = { message: 2 };
 
-    // const delays = [0, 100, 200, 300, 500, 1000];
-    const delays = [100];
+    const delays = [0, 10, 100, 500, 1000];
 
     const concurrentAccessError = "Concurrent access prevented in SkyDB";
+    const registryUpdateError = "Unable to update the registry";
 
     it.each(delays)(
       "should not get old data when getJSON is called after setJSON on a single client with a '%s' ms delay and getJSON doesn't fail",
@@ -348,9 +348,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         updatedJson.message--;
         // Catches both "doesn't have enough pow" and "provided revision number
         // is already registered" errors.
-        await expect(client2.db.setJSON(privateKey, dataKey, updatedJson)).rejects.toThrowError(
-          "Unable to update the registry"
-        );
+        await expect(client2.db.setJSON(privateKey, dataKey, updatedJson)).rejects.toThrowError(registryUpdateError);
 
         // Should work on that client again after calling getJSON.
 
@@ -361,5 +359,72 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         await client2.db.setJSON(privateKey, dataKey, updatedJson2);
       }
     );
+
+    it.each(delays)(
+      "should make sure that two concurrent setJSON calls on a single client with a '%s' ms delay either fail with the right error or succeed ",
+      async (delay) => {
+        const { publicKey, privateKey } = genKeyPairAndSeed();
+
+        // Try to invoke two concurrent setJSON calls.
+        try {
+          const setJSONFn = async function () {
+            // Sleep.
+            await new Promise((r) => setTimeout(r, delay));
+            await client.db.setJSON(privateKey, dataKey, jsonNew);
+          };
+          await Promise.all([setJSONFn(), client.db.setJSON(privateKey, dataKey, jsonOld)]);
+        } catch (e) {
+          if ((e as Error).message.includes(concurrentAccessError)) {
+            // The data race condition has been prevented and we received the expected error. Return from test early.
+            return;
+          }
+
+          // Unexpected error, throw.
+          throw e;
+        }
+
+        // Data race did not occur, getJSON should get latest JSON.
+        const { data: receivedJson } = await client.db.getJSON(publicKey, dataKey);
+        expect(receivedJson).toEqual(jsonNew);
+      }
+    );
+
+    it.each(delays)(
+      "should make sure that two concurrent setJSON calls on different clients with a '%s' ms delay fail with the right error or succeed",
+      async (delay) => {
+        // Create two new clients with a fresh revision cache.
+        const client1 = new SkynetClient(portal);
+        const client2 = new SkynetClient(portal);
+        const { publicKey, privateKey } = genKeyPairAndSeed();
+
+        // Try to invoke two concurrent setJSON calls.
+        try {
+          const setJSONFn = async function () {
+            // Sleep.
+            await new Promise((r) => setTimeout(r, delay));
+            await client2.db.setJSON(privateKey, dataKey, jsonNew);
+          };
+          await Promise.all([setJSONFn(), client1.db.setJSON(privateKey, dataKey, jsonOld)]);
+        } catch (e) {
+          if ((e as Error).message.includes(registryUpdateError)) {
+            // The data race condition has been prevented and we received the expected error. Return from test early.
+            return;
+          }
+
+          // Unexpected error, throw.
+          throw e;
+        }
+
+        // Data race did not occur, getJSON should get one of the JSON values.
+        let client3;
+        if (Math.random() < 0.5) {
+          client3 = client1;
+        } else {
+          client3 = client2;
+        }
+        const { data: receivedJson } = await client3.db.getJSON(publicKey, dataKey);
+        expect([jsonOld, jsonNew]).toContainEqual(receivedJson);
+      }
+    );
   });
 });

From 83c6a674e699128dd8b3f74b7eb750c4028e7e9b Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 11 Nov 2021 12:47:33 -0600
Subject: [PATCH 15/31] Try to avoid rate limiter in SkyDB tests

---
 integration/skydb.test.ts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index af9c7ebd..f3969486 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -7,6 +7,11 @@ import { decodeSkylinkBase64 } from "../src/utils/encoding";
 import { toHexString } from "../src/utils/string";
 
 describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
+  // Sleep for a second before each test to try to avoid rate limiter.
+  beforeEach(async () => {
+    await new Promise((r) => setTimeout(r, 1000));
+  });
+
   it("Should get existing SkyDB data", async () => {
     const publicKey = "89e5147864297b80f5ddf29711ba8c093e724213b0dcbefbc3860cc6d598cc35";
     const dataKey = "dataKey1";
@@ -272,7 +277,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     const jsonOld = { message: 1 };
     const jsonNew = { message: 2 };
 
-    const delays = [0, 10, 100, 500, 1000];
+    const delays = [0, 10, 100, 500];
 
     const concurrentAccessError = "Concurrent access prevented in SkyDB";
     const registryUpdateError = "Unable to update the registry";

From 5b268417fae9b3745fade7169521324ee23c7629 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 26 Nov 2021 16:21:38 -0600
Subject: [PATCH 16/31] Address some review comments

---
 integration/skydb.test.ts |   2 +-
 src/client.ts             | 103 ++------------------------------------
 src/mysky/index.ts        |   6 +--
 src/revision_cache.ts     |  93 ++++++++++++++++++++++++++++++++++
 src/skydb.test.ts         |  10 ++--
 src/skydb.ts              |  27 +++++-----
 6 files changed, 122 insertions(+), 119 deletions(-)
 create mode 100644 src/revision_cache.ts

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index f3969486..ee9af90f 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -253,7 +253,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
 
     await client.db.setJSON(privateKey, dataKey, json);
 
-    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
     expect(cachedRevisionEntry.revision.toString()).toEqual("0");
 
     await client.db.setJSON(privateKey, dataKey, json);
diff --git a/src/client.ts b/src/client.ts
index c983273f..2ae486dd 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -1,4 +1,3 @@
-import { Mutex, tryAcquire } from "async-mutex";
 import axios from "axios";
 import type { AxiosResponse, ResponseType, Method } from "axios";
 
@@ -33,6 +32,7 @@ import {
 } from "./file";
 import { pinSkylink } from "./pin";
 import { getEntry, getEntryLinkAsync, getEntryUrl, setEntry, postSignedEntry } from "./registry";
+import { RevisionNumberCache } from "./revision_cache";
 import {
   deleteJSON,
   getJSON,
@@ -106,10 +106,6 @@ export class SkynetClient {
   // The custom portal URL, if one was passed in to `new SkynetClient()`.
   protected customPortalUrl?: string;
 
-  // Holds the cached revision numbers, protected by mutexes to prevent
-  // concurrent access.
-  revisionNumberCache = new RevisionNumberCache();
-
   // Set methods (defined in other files).
 
   // Upload
@@ -167,6 +163,10 @@ export class SkynetClient {
     getEntryData: getEntryData.bind(this),
     setEntryData: setEntryData.bind(this),
     deleteEntryData: deleteEntryData.bind(this),
+
+    // Holds the cached revision numbers, protected by mutexes to prevent
+    // concurrent access.
+    revisionNumberCache: new RevisionNumberCache(),
   };
 
   // Registry
@@ -329,99 +329,6 @@ export class SkynetClient {
   }
 }
 
-// =====================
-// Revision Number Cache
-// =====================
-
-/**
- * An abstraction over the client's revision number cache. Provides a cache,
- * keyed by public key and data key and protected by a mutex to guard against
- * concurrent access to the cache. Each cache entry also has its own mutex, to
- * protect against concurrent access to that entry.
- */
-class RevisionNumberCache {
-  mutex = new Mutex();
-  cache: { [key: string]: CachedRevisionEntry } = {};
-
-  /**
-   * Gets an object containing the cached revision and the mutex for the entry.
-   * The revision and mutex will be initialized if the entry is not yet cached.
-   *
-   * @param publicKey - The given public key.
-   * @param dataKey - The given data key.
-   * @returns - The cached revision entry object.
-   */
-  async getRevisionAndMutexForEntry(publicKey: string, dataKey: string): Promise<CachedRevisionEntry> {
-    const cacheKey = getCacheKey(publicKey, dataKey);
-
-    // Block until the mutex is available for the cache.
-    return await this.mutex.runExclusive(async () => {
-      const cachedValue = this.cache[cacheKey];
-
-      if (!cachedValue) {
-        // Initialize a new cached entry and return that.
-        const newValue = new CachedRevisionEntry();
-        this.cache[cacheKey] = newValue;
-        return newValue;
-      } else {
-        // Return the cached entry.
-        return cachedValue;
-      }
-    });
-  }
-
-  /**
-   * Calls `exclusiveFn` with exclusive access to the given cached entry. The
-   * revision number of the entry can be safely updated in `exclusiveFn`.
-   *
-   * @param publicKey - The given public key.
-   * @param dataKey - The given data key.
-   * @param exclusiveFn - A function to call with exclusive access to the given cached entry.
-   * @returns - A promise containing the result of calling `exclusiveFn`.
-   */
-  async withCachedEntryLock<T>(
-    publicKey: string,
-    dataKey: string,
-    exclusiveFn: (cachedRevisionEntry: CachedRevisionEntry) => Promise<T>
-  ): Promise<T> {
-    // Safely get or create mutex for the requested entry.
-    const cachedRevisionEntry = await this.getRevisionAndMutexForEntry(publicKey, dataKey);
-
-    try {
-      return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => exclusiveFn(cachedRevisionEntry));
-    } catch (e) {
-      // Change mutex error to be more descriptive and user-friendly.
-      if ((e as Error).message.includes("mutex already locked")) {
-        throw new Error(
-          `Concurrent access prevented in SkyDB for entry { publicKey: ${publicKey}, dataKey: ${dataKey} }`
-        );
-      } else {
-        throw e;
-      }
-    }
-  }
-}
-
-/**
- * An object containing a cached revision and a corresponding mutex. The
- * revision can be internally updated and it will reflect in the client's cache.
- */
-export class CachedRevisionEntry {
-  mutex = new Mutex();
-  revision = BigInt(-1);
-}
-
-/**
- * Gets the revision cache key for the given public key and data key.
- *
- * @param publicKey - The given public key.
- * @param dataKey - The given data key.
- * @returns - The revision cache key.
- */
-function getCacheKey(publicKey: string, dataKey: string): string {
-  return `${publicKey}/${dataKey}`;
-}
-
 // =======
 // Helpers
 // =======
diff --git a/src/mysky/index.ts b/src/mysky/index.ts
index 9bbce9b7..bda409b8 100644
--- a/src/mysky/index.ts
+++ b/src/mysky/index.ts
@@ -399,7 +399,7 @@ export class MySky {
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
     // Immediately fail if the mutex is not available.
-    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
+    return await this.connector.client.db.revisionNumberCache.withCachedEntryLock(
       publicKey,
       dataKey,
       async (cachedRevisionEntry) => {
@@ -531,7 +531,7 @@ export class MySky {
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
     // Immediately fail if the mutex is not available.
-    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
+    return await this.connector.client.db.revisionNumberCache.withCachedEntryLock(
       publicKey,
       dataKey,
       async (cachedRevisionEntry) => {
@@ -666,7 +666,7 @@ export class MySky {
     opts.hashedDataKeyHex = true; // Do not hash the tweak anymore.
 
     // Immediately fail if the mutex is not available.
-    return await this.connector.client.revisionNumberCache.withCachedEntryLock(
+    return await this.connector.client.db.revisionNumberCache.withCachedEntryLock(
       publicKey,
       dataKey,
       async (cachedRevisionEntry) => {
diff --git a/src/revision_cache.ts b/src/revision_cache.ts
new file mode 100644
index 00000000..6971889c
--- /dev/null
+++ b/src/revision_cache.ts
@@ -0,0 +1,93 @@
+import { Mutex, tryAcquire } from "async-mutex";
+
+/**
+ * An abstraction over the client's revision number cache. Provides a cache,
+ * keyed by public key and data key and protected by a mutex to guard against
+ * concurrent access to the cache. Each cache entry also has its own mutex, to
+ * protect against concurrent access to that entry.
+ */
+export class RevisionNumberCache {
+  private mutex: Mutex;
+  private cache: { [key: string]: CachedRevisionNumber };
+
+  constructor() {
+    this.mutex = new Mutex();
+    this.cache = {};
+  }
+
+  /**
+   * Gets the revision cache key for the given public key and data key.
+   *
+   * @param publicKey - The given public key.
+   * @param dataKey - The given data key.
+   * @returns - The revision cache key.
+   */
+  static getCacheKey(publicKey: string, dataKey: string): string {
+    return `${publicKey}/${dataKey}`;
+  }
+
+  /**
+   * Gets an object containing the cached revision and the mutex for the entry.
+   * The revision and mutex will be initialized if the entry is not yet cached.
+   *
+   * @param publicKey - The given public key.
+   * @param dataKey - The given data key.
+   * @returns - The cached revision entry object.
+   */
+  async getRevisionAndMutexForEntry(publicKey: string, dataKey: string): Promise<CachedRevisionNumber> {
+    const cacheKey = RevisionNumberCache.getCacheKey(publicKey, dataKey);
+
+    // Block until the mutex is available for the cache.
+    return await this.mutex.runExclusive(async () => {
+      if (!this.cache[cacheKey]) {
+        this.cache[cacheKey] = new CachedRevisionNumber();
+      }
+      return this.cache[cacheKey];
+    });
+  }
+
+  /**
+   * Calls `exclusiveFn` with exclusive access to the given cached entry. The
+   * revision number of the entry can be safely updated in `exclusiveFn`.
+   *
+   * @param publicKey - The given public key.
+   * @param dataKey - The given data key.
+   * @param exclusiveFn - A function to call with exclusive access to the given cached entry.
+   * @returns - A promise containing the result of calling `exclusiveFn`.
+   */
+  async withCachedEntryLock<T>(
+    publicKey: string,
+    dataKey: string,
+    exclusiveFn: (cachedRevisionEntry: CachedRevisionNumber) => Promise<T>
+  ): Promise<T> {
+    // Safely get or create mutex for the requested entry.
+    const cachedRevisionEntry = await this.getRevisionAndMutexForEntry(publicKey, dataKey);
+
+    try {
+      return await tryAcquire(cachedRevisionEntry.mutex).runExclusive(async () => exclusiveFn(cachedRevisionEntry));
+    } catch (e) {
+      // Change mutex error to be more descriptive and user-friendly.
+      if ((e as Error).message.includes("mutex already locked")) {
+        throw new Error(
+          `Concurrent access prevented in SkyDB for entry { publicKey: ${publicKey}, dataKey: ${dataKey} }`
+        );
+      } else {
+        throw e;
+      }
+    }
+  }
+}
+
+/**
+ * An object containing a cached revision and a corresponding mutex. The
+ * revision can be internally updated and it will reflect in the client's cache.
+ */
+export class CachedRevisionNumber {
+  mutex: Mutex;
+  revision: bigint;
+
+  constructor() {
+    this.mutex = new Mutex();
+    this.revision = BigInt(-1);
+  }
+}
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 1f738fbd..9a4918ca 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -198,7 +198,7 @@ describe("setJSON", () => {
 
   it("should fail if the entry has the maximum allowed revision", async () => {
     const dataKey = "maximum revision";
-    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
     cachedRevisionEntry.revision = MAX_REVISION;
 
     // mock a successful registry update
@@ -239,7 +239,7 @@ describe("setJSON", () => {
 
     await client.db.setJSON(privateKey, dataKey, json);
 
-    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
     const revision1 = cachedRevisionEntry.revision;
 
     // mock a failed registry update
@@ -441,7 +441,7 @@ describe("getJSON/setJSON data race regression unit tests", () => {
 
     checkSettledValuesForErrorOrValue<JSONResponse>(values, concurrentAccessError);
 
-    const cachedRevisionEntry = await client.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
     expect(cachedRevisionEntry.revision.toString()).toEqual("0");
 
     // Make a getJSON call.
@@ -501,7 +501,7 @@ describe("getJSON/setJSON data race regression unit tests", () => {
 
     // Test that the client that succeeded has a consistent cache.
 
-    const cachedRevisionEntrySuccess = await successClient.revisionNumberCache.getRevisionAndMutexForEntry(
+    const cachedRevisionEntrySuccess = await successClient.db.revisionNumberCache.getRevisionAndMutexForEntry(
       publicKey,
       dataKey
     );
@@ -530,7 +530,7 @@ describe("getJSON/setJSON data race regression unit tests", () => {
 
     // Test that the client that failed has a consistent cache.
 
-    const cachedRevisionEntryFail = await failClient.revisionNumberCache.getRevisionAndMutexForEntry(
+    const cachedRevisionEntryFail = await failClient.db.revisionNumberCache.getRevisionAndMutexForEntry(
       publicKey,
       dataKey
     );
diff --git a/src/skydb.ts b/src/skydb.ts
index 65b20301..c3a2f0b5 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -1,6 +1,6 @@
 import { sign } from "tweetnacl";
 
-import { CachedRevisionEntry, SkynetClient } from "./client";
+import { SkynetClient } from "./client";
 import { DEFAULT_DOWNLOAD_OPTIONS, CustomDownloadOptions } from "./download";
 import {
   DEFAULT_GET_ENTRY_OPTIONS,
@@ -10,6 +10,7 @@ import {
   CustomSetEntryOptions,
   validatePublicKey,
 } from "./registry";
+import { CachedRevisionNumber } from "./revision_cache";
 import { BASE64_ENCODED_SKYLINK_SIZE, decodeSkylink, EMPTY_SKYLINK, RAW_SKYLINK_SIZE } from "./skylink/sia";
 import { MAX_REVISION } from "./utils/number";
 import { URI_SKYNET_PREFIX } from "./utils/url";
@@ -134,7 +135,7 @@ export type RawBytesResponse = {
  *   - Data found: update cached revision
  *   - Parse error: don't update cached revision
  *   - Network error: don't update cached revision
- *   - Too high version error: don't update the cached revision
+ *   - Too low version error: don't update the cached revision
  *   - 404 (data not found): don't update the cached revision
  *   - Data deleted: update cached revision
  *
@@ -162,7 +163,7 @@ export async function getJSON(
   };
 
   // Immediately fail if the mutex is not available.
-  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
+  return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Lookup the registry entry.
     const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
     const entry: RegistryEntry | null = await getSkyDBRegistryEntryAndUpdateCache(
@@ -245,7 +246,7 @@ export async function setJSON(
   const publicKey = toHexString(publicKeyArray);
 
   // Immediately fail if the mutex is not available.
-  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
+  return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Get the cached revision number before doing anything else.
     const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
@@ -350,7 +351,7 @@ export async function getEntryData(
   };
 
   // Immediately fail if the mutex is not available.
-  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
+  return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     const entry = await getSkyDBRegistryEntryAndUpdateCache(this, publicKey, dataKey, cachedRevisionEntry, opts);
     if (entry === null) {
       return { data: null };
@@ -397,7 +398,7 @@ export async function setEntryData(
   const publicKey = toHexString(publicKeyArray);
 
   // Immediately fail if the mutex is not available.
-  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
+  return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Get the cached revision number.
     const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
@@ -475,7 +476,7 @@ export async function getRawBytes(
   };
 
   // Immediately fail if the mutex is not available.
-  return await this.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
+  return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
     // Lookup the registry entry.
     const getEntryOpts = extractOptions(opts, DEFAULT_GET_ENTRY_OPTIONS);
     const entry = await getSkyDBRegistryEntryAndUpdateCache(
@@ -653,7 +654,7 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   client: SkynetClient,
   publicKey: string,
   dataKey: string,
-  cachedRevisionEntry: CachedRevisionEntry,
+  cachedRevisionEntry: CachedRevisionNumber,
   opts: CustomGetEntryOptions
 ): Promise<RegistryEntry | null> {
   // If this throws due to a parse error or network error, exit early and do not
@@ -668,11 +669,13 @@ async function getSkyDBRegistryEntryAndUpdateCache(
   // Calculate the new revision.
   const newRevision = entry?.revision ?? UNCACHED_REVISION_NUMBER + BigInt(1);
 
-  // Don't update the cached revision number if the received version is too low. Throw error.
+  // Don't update the cached revision number if the received version is too low.
+  // Throw error.
   const cachedRevision = cachedRevisionEntry.revision;
   if (cachedRevision && cachedRevision > newRevision) {
-    /* istanbul ignore next - this shouldn't come up in practice */
-    throw new Error("A higher revision number for this userID and path is already cached");
+    throw new Error(
+      "Returned revision number too low. A higher revision number for this userID and path is already cached"
+    );
   }
 
   // Update the cached revision.
@@ -680,7 +683,7 @@ async function getSkyDBRegistryEntryAndUpdateCache(
 
   // Return null if the entry contained a sentinel value indicating deletion.
   // We do this after updating the revision number cache.
-  if (entry !== null && wasRegistryEntryDeleted(entry)) {
+  if (wasRegistryEntryDeleted(entry)) {
     return null;
   }
 

From 7420dd51e691408a96fa3304ec0259995bcad2ec Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 26 Nov 2021 16:21:54 -0600
Subject: [PATCH 17/31] Add unit test for "too low revision" error

---
 src/skydb.test.ts | 52 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 9a4918ca..3771732f 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -9,6 +9,7 @@ import { SkynetClient } from "./index";
 import { getEntryUrlForPortal } from "./registry";
 import { checkCachedDataLink, DELETION_ENTRY_DATA, JSONResponse } from "./skydb";
 import { MAX_ENTRY_LENGTH } from "./mysky";
+import { decodeSkylink } from "./skylink/sia";
 
 // Generated with genKeyPairFromSeed("insecure test seed")
 const [publicKey, privateKey] = [
@@ -58,9 +59,9 @@ describe("getJSON", () => {
   });
 
   it("should perform a lookup and skylink GET", async () => {
-    // mock a successful registry lookup
+    // Mock a successful registry lookup.
     mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
-    // mock a successful data download
+    // Mock a successful data download.
     mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, headers);
 
     const { data, dataLink } = await client.db.getJSON(publicKey, dataKey);
@@ -69,6 +70,53 @@ describe("getJSON", () => {
     expect(mock.history.get.length).toBe(2);
   });
 
+  it("should fail properly with a too low error", async () => {
+    // Use a custom data key for this test to get a fresh cache.
+    const dataKey = "testTooLowError";
+    const registryGetUrl = getEntryUrlForPortal(portalUrl, publicKey, dataKey);
+    const skylinkData = toHexString(decodeSkylink(skylink));
+    const entryData = {
+      data: skylinkData,
+      revision: 1,
+      signature:
+        "18d2b5f64042db39c4c591c21bd93015f7839eefab487ef8e27086cdb95b190732211b9a23d38c33f4f9a4e5219de55a80f75ff7e437713732ecdb4ccddb0804",
+    };
+    const entryDataTooLow = {
+      data: skylinkData,
+      revision: 0,
+      signature:
+        "4d7b26923f4211794eaf5c13230e62618ea3bebcb3fa6511ec8772b1f1e1a675b5244e7c33f89daf31999aeabe46c3a1e324a04d2f35c6ba902c75d35ceba00d",
+    };
+
+    // Cache the revision.
+
+    // Mock a successful registry lookup.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));
+    // Mock a successful data download.
+    mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, headers);
+
+    const { data } = await client.db.getJSON(publicKey, dataKey);
+    expect(data).toEqual(jsonData);
+
+    // The cache should contain revision 1.
+    const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
+    expect(cachedRevisionEntry.revision.toString()).toEqual("1");
+
+    // Return a revision that's too low.
+
+    // Mock a successful registry lookup.
+    mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryDataTooLow));
+    // Mock a successful data download.
+    mock.onGet(skylinkUrl).replyOnce(200, fullJsonData, headers);
+
+    await expect(client.db.getJSON(publicKey, dataKey)).rejects.toThrowError(
+      "Returned revision number too low. A higher revision number for this userID and path is already cached"
+    );
+
+    // The cache should still contain revision 1.
+    expect(cachedRevisionEntry.revision.toString()).toEqual("1");
+  });
+
   it("should perform a lookup but not a skylink GET if the cachedDataLink is a hit", async () => {
     // mock a successful registry lookup
     mock.onGet(registryGetUrl).replyOnce(200, JSON.stringify(entryData));

From 3eada7e6c9f340b522c47b481a297729849d894e Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 29 Nov 2021 20:50:17 -0600
Subject: [PATCH 18/31] Refactor delay functions in get/set JSON data race
 tests

---
 integration/skydb.test.ts | 61 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index ee9af90f..f5e9db61 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -1,7 +1,7 @@
 import { AxiosError } from "axios";
 
 import { client, dataKey, portal } from ".";
-import { genKeyPairAndSeed, getEntryLink, SkynetClient, URI_SKYNET_PREFIX } from "../src";
+import { genKeyPairAndSeed, getEntryLink, JsonData, JSONResponse, SkynetClient, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
 import { decodeSkylinkBase64 } from "../src/utils/encoding";
 import { toHexString } from "../src/utils/string";
@@ -282,6 +282,26 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     const concurrentAccessError = "Concurrent access prevented in SkyDB";
     const registryUpdateError = "Unable to update the registry";
 
+    const getJSONWithDelay = async function (
+      client: SkynetClient,
+      delay: number,
+      publicKey: string,
+      dataKey: string
+    ): Promise<JSONResponse> {
+      await new Promise((r) => setTimeout(r, delay));
+      return await client.db.getJSON(publicKey, dataKey);
+    };
+    const setJSONWithDelay = async function (
+      client: SkynetClient,
+      delay: number,
+      privateKey: string,
+      dataKey: string,
+      data: JsonData
+    ) {
+      await new Promise((r) => setTimeout(r, delay));
+      return await client.db.setJSON(privateKey, dataKey, data);
+    };
+
     it.each(delays)(
       "should not get old data when getJSON is called after setJSON on a single client with a '%s' ms delay and getJSON doesn't fail",
       async (delay) => {
@@ -294,12 +314,10 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         let receivedJson;
         try {
           // Get the data while also calling setJSON.
-          const getJSONFn = async function () {
-            // Sleep.
-            await new Promise((r) => setTimeout(r, delay));
-            return await client.db.getJSON(publicKey, dataKey);
-          };
-          [{ data: receivedJson }] = await Promise.all([getJSONFn(), client.db.setJSON(privateKey, dataKey, jsonNew)]);
+          [{ data: receivedJson }] = await Promise.all([
+            getJSONWithDelay(client, delay, publicKey, dataKey),
+            setJSONWithDelay(client, 0, privateKey, dataKey, jsonNew),
+          ]);
         } catch (e) {
           if ((e as Error).message.includes(concurrentAccessError)) {
             // The data race condition has been prevented and we received the expected error. Return from test early.
@@ -330,14 +348,9 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         // should succeeed.
 
         // Get the data while also calling setJSON.
-        const getJSONFn = async function () {
-          // Sleep.
-          await new Promise((r) => setTimeout(r, delay));
-          return await client2.db.getJSON(publicKey, dataKey);
-        };
         const [{ data: receivedJson }] = await Promise.all([
-          getJSONFn(),
-          client1.db.setJSON(privateKey, dataKey, jsonNew),
+          getJSONWithDelay(client2, delay, publicKey, dataKey),
+          setJSONWithDelay(client1, 0, privateKey, dataKey, jsonNew),
         ]);
 
         // If we got old data from getJSON, make sure that that client is not
@@ -372,12 +385,10 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
 
         // Try to invoke two concurrent setJSON calls.
         try {
-          const setJSONFn = async function () {
-            // Sleep.
-            await new Promise((r) => setTimeout(r, delay));
-            await client.db.setJSON(privateKey, dataKey, jsonNew);
-          };
-          await Promise.all([setJSONFn(), client.db.setJSON(privateKey, dataKey, jsonOld)]);
+          await Promise.all([
+            setJSONWithDelay(client, delay, privateKey, dataKey, jsonNew),
+            setJSONWithDelay(client, 0, privateKey, dataKey, jsonOld),
+          ]);
         } catch (e) {
           if ((e as Error).message.includes(concurrentAccessError)) {
             // The data race condition has been prevented and we received the expected error. Return from test early.
@@ -404,12 +415,10 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
 
         // Try to invoke two concurrent setJSON calls.
         try {
-          const setJSONFn = async function () {
-            // Sleep.
-            await new Promise((r) => setTimeout(r, delay));
-            await client2.db.setJSON(privateKey, dataKey, jsonNew);
-          };
-          await Promise.all([setJSONFn(), client1.db.setJSON(privateKey, dataKey, jsonOld)]);
+          await Promise.all([
+            setJSONWithDelay(client2, delay, privateKey, dataKey, jsonNew),
+            setJSONWithDelay(client1, 0, privateKey, dataKey, jsonOld),
+          ]);
         } catch (e) {
           if ((e as Error).message.includes(registryUpdateError)) {
             // The data race condition has been prevented and we received the expected error. Return from test early.

From f55bdaf426877c869ef8448aee05d60ab5901ae5 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 29 Nov 2021 22:21:19 -0600
Subject: [PATCH 19/31] Refactor settled values check

---
 src/skydb.test.ts | 106 ++++++++++++++++++++++------------------------
 utils/testing.ts  |  22 ++++++++++
 2 files changed, 73 insertions(+), 55 deletions(-)

diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 3771732f..2d56ac66 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -10,6 +10,8 @@ import { getEntryUrlForPortal } from "./registry";
 import { checkCachedDataLink, DELETION_ENTRY_DATA, JSONResponse } from "./skydb";
 import { MAX_ENTRY_LENGTH } from "./mysky";
 import { decodeSkylink } from "./skylink/sia";
+import { getSettledValues } from "../utils/testing";
+import { JsonData } from "./utils/types";
 
 // Generated with genKeyPairFromSeed("insecure test seed")
 const [publicKey, privateKey] = [
@@ -408,22 +410,31 @@ describe("getJSON/setJSON data race regression unit tests", () => {
 
     // Try to invoke the data race.
     // Get the data while also calling setJSON.
-    // Use Promise.allSettled to wait for all promises to finish, or some mocked requests will hang around and interfere with the later tests.
-    const values = await Promise.allSettled([
+    //
+    // Use Promise.allSettled to wait for all promises to finish, or some mocked
+    // requests will hang around and interfere with the later tests.
+    const settledResults = await Promise.allSettled([
       client.db.getJSON(publicKey, dataKey),
       client.db.setJSON(privateKey, dataKey, jsonNew),
     ]);
 
-    // If any promises were rejected, check the error message.
-    const data = checkSettledValuesForErrorOrValue<JSONResponse>(values, concurrentAccessError);
-    if (!data) {
-      // The data race condition was avoided and we received the expected
-      // error. Return from test early.
-      return;
+    let data: JsonData | null;
+    try {
+      const values = getSettledValues<JSONResponse>(settledResults);
+      data = values[0].data;
+    } catch (e) {
+      // If any promises were rejected, check the error message.
+      if ((e as Error).message.includes(concurrentAccessError)) {
+        // The data race condition was avoided and we received the expected
+        // error. Return from test early.
+        return;
+      }
+
+      throw e;
     }
 
     // Data race did not occur, getJSON should have latest JSON.
-    expect(data.data).toEqual(jsonNew);
+    expect(data).toEqual(jsonNew);
 
     // assert our request history contains the expected amount of requests
     expect(mock.history.get.length).toBe(2);
@@ -452,22 +463,30 @@ describe("getJSON/setJSON data race regression unit tests", () => {
 
     // Try to invoke the data race.
     // Get the data while also calling setJSON.
+    //
     // Use Promise.allSettled to wait for all promises to finish, or some mocked requests will hang around and interfere with the later tests.
-    const values = await Promise.allSettled([
+    const settledResults = await Promise.allSettled([
       client1.db.getJSON(publicKey, dataKey),
       client2.db.setJSON(privateKey, dataKey, jsonNew),
     ]);
 
-    // If any promises were rejected, check the error message.
-    const data = checkSettledValuesForErrorOrValue<JSONResponse>(values, higherRevisionError);
-    if (!data) {
-      // The data race condition was avoided and we received the expected
-      // error. Return from test early.
-      return;
+    let data: JsonData | null;
+    try {
+      const values = getSettledValues<JSONResponse>(settledResults);
+      data = values[0].data;
+    } catch (e) {
+      // If any promises were rejected, check the error message.
+      if ((e as Error).message.includes(higherRevisionError)) {
+        // The data race condition was avoided and we received the expected
+        // error. Return from test early.
+        return;
+      }
+
+      throw e;
     }
 
     // Data race did not occur, getJSON should have latest JSON.
-    expect(data.data).toEqual(jsonNew);
+    expect(data).toEqual(jsonNew);
 
     // assert our request history contains the expected amount of requests.
     expect(mock.history.get.length).toBe(2);
@@ -482,12 +501,24 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
     mock.onPost(registryPostUrl).replyOnce(204);
 
+    // Use Promise.allSettled to wait for all promises to finish, or some mocked
+    // requests will hang around and interfere with the later tests.
     const values = await Promise.allSettled([
       client.db.setJSON(privateKey, dataKey, jsonOld),
       client.db.setJSON(privateKey, dataKey, jsonOld),
     ]);
 
-    checkSettledValuesForErrorOrValue<JSONResponse>(values, concurrentAccessError);
+    try {
+      getSettledValues<JSONResponse>(values);
+    } catch (e) {
+      if ((e as Error).message.includes(concurrentAccessError)) {
+        // The data race condition was avoided and we received the expected
+        // error. Return from test early.
+        return;
+      }
+
+      throw e;
+    }
 
     const cachedRevisionEntry = await client.db.revisionNumberCache.getRevisionAndMutexForEntry(publicKey, dataKey);
     expect(cachedRevisionEntry.revision.toString()).toEqual("0");
@@ -532,6 +563,8 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     mock.onPost(uploadUrl).replyOnce(200, { skylink: skylinkOld, merkleroot, bitfield });
     mock.onPost(registryPostUrl).replyOnce(400);
 
+    // Use Promise.allSettled to wait for all promises to finish, or some mocked
+    // requests will hang around and interfere with the later tests.
     const values = await Promise.allSettled([
       client1.db.setJSON(privateKey, dataKey, jsonOld),
       client2.db.setJSON(privateKey, dataKey, jsonOld),
@@ -610,41 +643,4 @@ describe("getJSON/setJSON data race regression unit tests", () => {
     expect(mock.history.get.length).toBe(8);
     expect(mock.history.post.length).toBe(8);
   });
-
-  /**
-   * Checks the settled values from Promise.allSettled for the given error.
-   * Throws if an unexpected error is found. Returns settled value if no errors
-   * were found.
-   *
-   * @param values - The settled values.
-   * @param err - The err to check for.
-   * @returns - The settled value if no errors were found, or null if the expected error was found.
-   * @throws - Will throw if an unexpected error occurred.
-   */
-  function checkSettledValuesForErrorOrValue<T>(values: PromiseSettledResult<T>[], err: string): T | null {
-    let rejected = false;
-    let reason;
-    let receivedValue: T | null = null;
-    for (const value of values) {
-      if (value.status === "rejected") {
-        rejected = true;
-        reason = value.reason;
-      } else if (value.value) {
-        receivedValue = value.value;
-      }
-    }
-    if (rejected) {
-      // TODO: Don't return early.
-      if ((reason as Error).message.includes(err)) {
-        // The data race condition was avoided and we received the expected
-        // error. Return from test early.
-        return null;
-      } else {
-        // Unexpected error, throw.
-        throw reason as Error;
-      }
-    }
-
-    return receivedValue;
-  }
 });
diff --git a/utils/testing.ts b/utils/testing.ts
index 3d062e4b..651133bf 100644
--- a/utils/testing.ts
+++ b/utils/testing.ts
@@ -73,6 +73,28 @@ export function extractNonSkylinkPath(url: string, skylink: string): string {
   return path;
 }
 
+/**
+ * Gets the settled values from `Promise.allSettled`. Throws if an error is
+ * found. Returns all settled values if no errors were found.
+ *
+ * @param values - The settled values.
+ * @returns - The settled value if no errors were found.
+ * @throws - Will throw if an unexpected error occurred.
+ */
+export function getSettledValues<T>(values: PromiseSettledResult<T>[]): T[] {
+  const receivedValues = [];
+
+  for (const value of values) {
+    if (value.status === "rejected") {
+      throw value.reason;
+    } else if (value.value) {
+      receivedValues.push(value.value);
+    }
+  }
+
+  return receivedValues;
+}
+
 /**
  * Generates a random Unicode string using the code points between 0 and 65536.
  *

From 114c4c0d58160203cea7fe2fd1fbff061a83582c Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 10 Dec 2021 16:23:18 -0600
Subject: [PATCH 20/31] Fix NDF in test, clean up, add more checks

---
 .eslintrc.json            |  1 +
 integration/skydb.test.ts | 25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/.eslintrc.json b/.eslintrc.json
index 5b0cebc2..41df1eab 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -13,6 +13,7 @@
   "extends": ["eslint:recommended", "plugin:jsdoc/recommended", "plugin:@typescript-eslint/recommended"],
   "rules": {
     "@typescript-eslint/no-floating-promises": 1,
+    "@typescript-eslint/no-unused-vars": ["warn", { "argsIgnorePattern": "^_", "varsIgnorePattern": "^_" }],
 
     "jsdoc/require-description": 1,
     "jsdoc/require-throws": 1,
diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index f5e9db61..cced4837 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -341,29 +341,41 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         const client2 = new SkynetClient(portal);
         const { publicKey, privateKey } = genKeyPairAndSeed();
 
+        const cachedRevisionEntry1 = await client1.db.revisionNumberCache.getRevisionAndMutexForEntry(
+          publicKey,
+          dataKey
+        );
+        const cachedRevisionEntry2 = await client2.db.revisionNumberCache.getRevisionAndMutexForEntry(
+          publicKey,
+          dataKey
+        );
+
         // Set the initial data.
         await client1.db.setJSON(privateKey, dataKey, jsonOld);
+        expect(cachedRevisionEntry1.revision.toString()).toEqual("0");
 
         // Call getJSON and setJSON concurrently on different clients -- both
         // should succeeed.
 
         // Get the data while also calling setJSON.
-        const [{ data: receivedJson }] = await Promise.all([
-          getJSONWithDelay(client2, delay, publicKey, dataKey),
+        const [_, { data: receivedJson }] = await Promise.all([
           setJSONWithDelay(client1, 0, privateKey, dataKey, jsonNew),
+          getJSONWithDelay(client2, delay, publicKey, dataKey),
         ]);
 
         // If we got old data from getJSON, make sure that that client is not
         // able to update that JSON.
 
         expect(receivedJson).not.toBeNull();
-        if (receivedJson === jsonNew) {
+        expect(cachedRevisionEntry1.revision.toString()).toEqual("1");
+        if (receivedJson?.message === jsonNew.message) {
+          expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
           return;
         }
         expect(receivedJson).toEqual(jsonOld);
+        expect(cachedRevisionEntry2.revision.toString()).toEqual("0");
 
-        const updatedJson = receivedJson as { message: number };
-        updatedJson.message--;
+        const updatedJson = { message: 3 };
         // Catches both "doesn't have enough pow" and "provided revision number
         // is already registered" errors.
         await expect(client2.db.setJSON(privateKey, dataKey, updatedJson)).rejects.toThrowError(registryUpdateError);
@@ -371,10 +383,13 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         // Should work on that client again after calling getJSON.
 
         const { data: receivedJson2 } = await client2.db.getJSON(publicKey, dataKey);
+        expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
         expect(receivedJson2).toEqual(jsonNew);
+
         const updatedJson2 = receivedJson2 as { message: number };
         updatedJson2.message++;
         await client2.db.setJSON(privateKey, dataKey, updatedJson2);
+        expect(cachedRevisionEntry2.revision.toString()).toEqual("2");
       }
     );
 

From 1dc51470b6b3e99a1cf3deb00555d8b5b82a2220 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Tue, 16 Nov 2021 14:43:31 -0600
Subject: [PATCH 21/31] Add some tests for files and subfiles with spaces in
 their names

---
 integration/upload_download.test.ts | 56 +++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/integration/upload_download.test.ts b/integration/upload_download.test.ts
index 1e89a469..eb74673c 100644
--- a/integration/upload_download.test.ts
+++ b/integration/upload_download.test.ts
@@ -34,9 +34,10 @@ describe(`Upload and download end-to-end tests for portal '${portal}'`, () => {
 
   it("Should upload and download directories", async () => {
     const directory = {
-      "i-am-not/file1.jpeg": new File(["foo1"], "i-am-not/file1.jpeg"),
-      "i-am-not/file2.jpeg": new File(["foo2"], "i-am-not/file2.jpeg"),
-      "i-am-not/me-neither/file3.jpeg": new File(["foo3"], "i-am-not/me-neither/file3.jpeg"),
+      "file1.jpeg": new File(["foo1"], "file1.jpeg"),
+      // Test a space in the subfile name.
+      "file 2.jpeg": new File(["foo2"], "file 2.jpeg"),
+      "subdir/file3.jpeg": new File(["foo3"], "subdir/file3.jpeg"),
     };
     const dirname = "dirname";
     const dirType = "application/zip";
@@ -44,14 +45,33 @@ describe(`Upload and download end-to-end tests for portal '${portal}'`, () => {
     const { skylink } = await client.uploadDirectory(directory, dirname);
     expect(skylink).not.toEqual("");
 
-    // Get file content and check returned values.
-
-    const resp = await client.getFileContent(skylink);
-    const { data, contentType, portalUrl, skylink: returnedSkylink } = resp;
-    expect(data).toEqual(expect.any(String));
-    expect(contentType).toEqual(dirType);
-    expect(portalUrl).toEqualPortalUrl(portal);
-    expect(skylink).toEqual(returnedSkylink);
+    // Get content for the skylink and check returned values.
+
+    {
+      const resp = await client.getFileContent(skylink);
+      const { data, contentType, portalUrl, skylink: returnedSkylink } = resp;
+      expect(data).toEqual(expect.any(String));
+      expect(contentType).toEqual(dirType);
+      expect(portalUrl).toEqualPortalUrl(portal);
+      expect(skylink).toEqual(returnedSkylink);
+    }
+
+    // Get file content for each subfile.
+
+    {
+      const { data } = await client.getFileContent(`${skylink}/file1.jpeg`);
+      expect(data).toEqual("foo1");
+    }
+
+    {
+      const { data } = await client.getFileContent(`${skylink}/file 2.jpeg`);
+      expect(data).toEqual("foo2");
+    }
+
+    {
+      const { data } = await client.getFileContent(`${skylink}/subdir:file3.jpeg`);
+      expect(data).toEqual("foo3");
+    }
   });
 
   it("Custom filenames should take effect", async () => {
@@ -144,6 +164,20 @@ describe(`Upload and download end-to-end tests for portal '${portal}'`, () => {
     expect(contentType).toEqual("application/octet-stream");
   });
 
+  it("Should upload and download a file with spaces in the filename", async () => {
+    const filename = " foo bar ";
+
+    const file = new File(["asdf"], filename);
+    expect(file.size).toEqual(4);
+    const { skylink } = await client.uploadFile(file);
+    expect(skylink).not.toEqual("");
+
+    // Get file content and check returned values.
+    const { data } = await client.getFileContent(skylink);
+
+    expect(data).toEqual("asdf");
+  });
+
   it("Should upload and download a 0-byte file", async () => {
     const onProgress = (progress: number) => {
       expect(progress).toEqual(1);

From e137a54318acf230e62ab561b2c33edaf5d15d5b Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Mon, 13 Dec 2021 10:07:31 +0000
Subject: [PATCH 22/31] Bump typescript from 4.5.2 to 4.5.3

Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.2 to 4.5.3.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](https://github.com/Microsoft/TypeScript/compare/v4.5.2...v4.5.3)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
---
 yarn.lock | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/yarn.lock b/yarn.lock
index c01cfa47..e6af38f7 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -4697,9 +4697,9 @@ typedarray-to-buffer@^3.1.5:
     is-typedarray "^1.0.0"
 
 typescript@^4.2.4:
-  version "4.5.2"
-  resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.5.2.tgz#8ac1fba9f52256fdb06fb89e4122fa6a346c2998"
-  integrity sha512-5BlMof9H1yGt0P8/WF+wPNw6GfctgGjXp5hkblpyT+8rkASSmkUKMXrxR0Xg8ThVCi/JnHQiKXeBaEwCeQwMFw==
+  version "4.5.3"
+  resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.5.3.tgz#afaa858e68c7103317d89eb90c5d8906268d353c"
+  integrity sha512-eVYaEHALSt+s9LbvgEv4Ef+Tdq7hBiIZgii12xXJnukryt3pMgJf6aKhoCZ3FWQsu6sydEnkg11fYXLzhLBjeQ==
 
 union-value@^1.0.0:
   version "1.0.1"

From b7e9761aceae8b50cf9aa0d8db490397b37a9b1a Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Mon, 13 Dec 2021 10:08:44 +0000
Subject: [PATCH 23/31] Bump eslint-plugin-jsdoc from 37.1.0 to 37.2.0

Bumps [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) from 37.1.0 to 37.2.0.
- [Release notes](https://github.com/gajus/eslint-plugin-jsdoc/releases)
- [Commits](https://github.com/gajus/eslint-plugin-jsdoc/compare/v37.1.0...v37.2.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-jsdoc
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
---
 yarn.lock | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/yarn.lock b/yarn.lock
index e6af38f7..c1d0d6fd 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -315,12 +315,12 @@
   dependencies:
     "@cspotcode/source-map-consumer" "0.8.0"
 
-"@es-joy/jsdoccomment@0.12.0":
-  version "0.12.0"
-  resolved "https://registry.yarnpkg.com/@es-joy/jsdoccomment/-/jsdoccomment-0.12.0.tgz#47de05d86e9728ae3a5f1c57d6e9b63b07c6dc98"
-  integrity sha512-Gw4/j9v36IKY8ET+W0GoOzrRw17xjf21EIFFRL3zx21fF5MnqmeNpNi+PU/LKjqLpPb2Pw2XdlJbYM31VVo/PQ==
+"@es-joy/jsdoccomment@0.13.0":
+  version "0.13.0"
+  resolved "https://registry.yarnpkg.com/@es-joy/jsdoccomment/-/jsdoccomment-0.13.0.tgz#90ffe2006981ff66d3f4fd2f05949487f0fca089"
+  integrity sha512-APVqbVPGOprb4BmjEnwbSzV+V2e/6DVIUnZG3zdW5uWXWkN0DKMCpiIy2TdBauoANKYO7RQpO8cTjIYNVSKwUA==
   dependencies:
-    comment-parser "1.2.4"
+    comment-parser "1.3.0"
     esquery "^1.4.0"
     jsdoc-type-pratt-parser "2.0.0"
 
@@ -1444,11 +1444,6 @@ commander@^8.3.0:
   resolved "https://registry.yarnpkg.com/commander/-/commander-8.3.0.tgz#4837ea1b2da67b9c616a67afbb0fafee567bca66"
   integrity sha512-OkTL9umf+He2DZkUq8f8J9of7yL6RJKI24dVITBmNfZBmri9zYZQrKkuXiKhyfPSu8tUhnVBB1iKXevvnlR4Ww==
 
-comment-parser@1.2.4:
-  version "1.2.4"
-  resolved "https://registry.yarnpkg.com/comment-parser/-/comment-parser-1.2.4.tgz#489f3ee55dfd184a6e4bffb31baba284453cb760"
-  integrity sha512-pm0b+qv+CkWNriSTMsfnjChF9kH0kxz55y44Wo5le9qLxMj5xDQAaEd9ZN1ovSuk9CsrncWaFwgpOMg7ClJwkw==
-
 comment-parser@1.3.0:
   version "1.3.0"
   resolved "https://registry.yarnpkg.com/comment-parser/-/comment-parser-1.3.0.tgz#68beb7dbe0849295309b376406730cd16c719c44"
@@ -1733,11 +1728,11 @@ escodegen@^1.14.1:
     source-map "~0.6.1"
 
 eslint-plugin-jsdoc@^37.0.3:
-  version "37.1.0"
-  resolved "https://registry.yarnpkg.com/eslint-plugin-jsdoc/-/eslint-plugin-jsdoc-37.1.0.tgz#88ccd4a70453f0660f0e14e9a67b91a324c32cfd"
-  integrity sha512-DpkFzX5Sqkqzy4MCgowhDXmusWcF1Gn7wYnphdGfWmIkoQr6SwL0jEtltGAVyF5Rj6ACi6ydw0oCCI5hF3yz6w==
+  version "37.2.0"
+  resolved "https://registry.yarnpkg.com/eslint-plugin-jsdoc/-/eslint-plugin-jsdoc-37.2.0.tgz#233690a37fa2e2abbe1f4f28ab676641dc8b3710"
+  integrity sha512-ca7s/DD1mMObZQ2Y0n0DO/KnFV+FqCX6ztir8pcSuylg3GGCREIisn36P/0cRySuWW/7Y7MNCuUDqtKdgLPU7Q==
   dependencies:
-    "@es-joy/jsdoccomment" "0.12.0"
+    "@es-joy/jsdoccomment" "0.13.0"
     comment-parser "1.3.0"
     debug "^4.3.3"
     escape-string-regexp "^4.0.0"

From 0d0ca3fe675b976b5a31aa3a841b954f4d0e5a7a Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 2 Dec 2021 21:56:21 -0600
Subject: [PATCH 24/31] Get the full, descriptive error response returned from
 skyd

Previously, methods were returning error messages that said e.g. "Request failed
with status code 429" and we had to drill down into the actual response to see
what the skyd error was.

All methods that make a request now return a descriptive error message based on
the skyd response message.

- Added ExecuteRequestError which contains the original Axios error

- Added request.ts file for ExecuteRequestError, where it was causing circular
  dependencies and stuff not being defined at runtime.

- Also, I fixed a bug where a promise in `uploadLargeFile` was not being
  rejected on error.
---
 integration/skydb.test.ts |  8 +++---
 src/client.ts             | 42 +++++++++++++++++-------------
 src/index.ts              |  1 +
 src/registry.ts           | 21 +++++----------
 src/request.ts            | 54 +++++++++++++++++++++++++++++++++++++++
 src/upload.ts             | 20 +++++++++------
 6 files changed, 101 insertions(+), 45 deletions(-)
 create mode 100644 src/request.ts

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index cced4837..8bf9c291 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -1,7 +1,5 @@
-import { AxiosError } from "axios";
-
 import { client, dataKey, portal } from ".";
-import { genKeyPairAndSeed, getEntryLink, JsonData, JSONResponse, SkynetClient, URI_SKYNET_PREFIX } from "../src";
+import { ExecuteRequestError, genKeyPairAndSeed, getEntryLink, JsonData, JSONResponse, SkynetClient, URI_SKYNET_PREFIX } from "../src";
 import { hashDataKey } from "../src/crypto";
 import { decodeSkylinkBase64 } from "../src/utils/encoding";
 import { toHexString } from "../src/utils/string";
@@ -143,9 +141,9 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
     // TODO: Should getFileContent return `null` on 404?
     try {
       await client.getFileContent(entryLink);
-      throw new Error("getFileContent should not have succeeded");
+      throw new Error("'getFileContent' should not have succeeded");
     } catch (err) {
-      expect((err as AxiosError).response?.status).toEqual(404);
+      expect((err as ExecuteRequestError).responseStatus).toEqual(404);
     }
 
     // The SkyDB entry should be null.
diff --git a/src/client.ts b/src/client.ts
index 2ae486dd..5fde77f2 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -1,4 +1,4 @@
-import axios from "axios";
+import axios, { AxiosError } from "axios";
 import type { AxiosResponse, ResponseType, Method } from "axios";
 
 import {
@@ -46,6 +46,7 @@ import {
 import { addSubdomain, addUrlQuery, defaultPortalUrl, ensureUrlPrefix, makeUrl } from "./utils/url";
 import { loadMySky } from "./mysky";
 import { extractDomain, getFullDomainUrl } from "./mysky/utils";
+import { getPortalErrResponse } from "./request";
 
 /**
  * Custom client options.
@@ -249,6 +250,7 @@ export class SkynetClient {
    *
    * @param config - Configuration for the request.
    * @returns - The response from axios.
+   * @throws - Will throw if the request fails.
    */
   async executeRequest(config: RequestConfig): Promise<AxiosResponse> {
     const url = await buildRequestUrl(this, {
@@ -285,23 +287,27 @@ export class SkynetClient {
       };
     }
 
-    return axios({
-      url,
-      method: config.method,
-      data: config.data,
-      headers,
-      auth,
-      onDownloadProgress,
-      onUploadProgress,
-      responseType: config.responseType,
-      transformRequest: config.transformRequest,
-      transformResponse: config.transformResponse,
-
-      maxContentLength: Infinity,
-      maxBodyLength: Infinity,
-      // Allow cross-site cookies.
-      withCredentials: true,
-    });
+    try {
+      return await axios({
+        url,
+        method: config.method,
+        data: config.data,
+        headers,
+        auth,
+        onDownloadProgress,
+        onUploadProgress,
+        responseType: config.responseType,
+        transformRequest: config.transformRequest,
+        transformResponse: config.transformResponse,
+
+        maxContentLength: Infinity,
+        maxBodyLength: Infinity,
+        // Allow cross-site cookies.
+        withCredentials: true,
+      });
+    } catch (e) {
+      throw getPortalErrResponse(e as AxiosError);
+    }
   }
 
   // ===============
diff --git a/src/index.ts b/src/index.ts
index e768334e..b884e2b7 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -82,6 +82,7 @@ export type {
   RegistryEntry,
   RegistryProofEntry,
 } from "./registry";
+export type { ExecuteRequestError } from "./request";
 export type {
   CustomGetJSONOptions,
   CustomSetJSONOptions,
diff --git a/src/registry.ts b/src/registry.ts
index 859e0d63..a4518f78 100644
--- a/src/registry.ts
+++ b/src/registry.ts
@@ -1,8 +1,9 @@
-import type { AxiosError, AxiosResponse } from "axios";
+import type { AxiosResponse } from "axios";
 import { Buffer } from "buffer";
 import { sign } from "tweetnacl";
 
 import { SkynetClient } from "./client";
+import { ExecuteRequestError } from "./request";
 import { assertUint64 } from "./utils/number";
 import { BaseCustomOptions, DEFAULT_BASE_OPTIONS } from "./utils/options";
 import { ensurePrefix, hexToUint8Array, isHexString, toHexString, trimPrefix, trimUriPrefix } from "./utils/string";
@@ -185,7 +186,8 @@ export async function getEntry(
       },
     });
   } catch (err) {
-    return handleGetEntryErrResponse(err as AxiosError);
+    // Check the executeRequest error to see if a 404 status was returned.
+    return handleGetEntryErrResponse(err as ExecuteRequestError);
   }
 
   // Sanity check.
@@ -580,22 +582,13 @@ export function validateRegistryProof(
 /**
  * Handles error responses returned from getEntry endpoint.
  *
- * @param err - The Axios error.
+ * @param err - The error.
  * @returns - An empty signed registry entry if the status code is 404.
  * @throws - Will throw if the error response is malformed, or the error message otherwise if the error status code is not 404.
  */
-function handleGetEntryErrResponse(err: AxiosError): SignedRegistryEntry {
-  /* istanbul ignore next */
-  if (!err.response) {
-    throw new Error(`Error response field not found, incomplete Axios error. Full error: ${err}`);
-  }
-
-  /* istanbul ignore next */
-  if (!err.response.status) {
-    throw new Error(`Error response did not contain expected field 'status'. Full error: ${err}`);
-  }
+function handleGetEntryErrResponse(err: ExecuteRequestError): SignedRegistryEntry {
   // Check if status was 404 "not found" and return null if so.
-  if (err.response.status === 404) {
+  if (err.responseStatus === 404) {
     return { entry: null, signature: null };
   }
 
diff --git a/src/request.ts b/src/request.ts
new file mode 100644
index 00000000..0538ae93
--- /dev/null
+++ b/src/request.ts
@@ -0,0 +1,54 @@
+import { AxiosError } from "axios";
+
+export class ExecuteRequestError extends Error {
+  originalError: AxiosError;
+  responseStatus: number | null;
+  responseMessage: string | null;
+
+  constructor(message: string, axiosError: AxiosError, responseStatus: number | null, responseMessage: string | null) {
+    super(message);
+    this.originalError = axiosError;
+    this.responseStatus = responseStatus;
+    this.responseMessage = responseMessage;
+
+    // Required for `instanceof` to work.
+    Object.setPrototypeOf(this, ExecuteRequestError.prototype);
+  }
+}
+
+/**
+ * Gets the full, descriptive error response returned from skyd on the portal.
+ *
+ * @param err - The Axios error.
+ * @returns - A new error if the error response is malformed, or the skyd error message otherwise.
+ */
+export function getPortalErrResponse(err: AxiosError): ExecuteRequestError {
+  /* istanbul ignore next */
+  if (!err.response) {
+    return new ExecuteRequestError(`Error repsonse did not contain expected field 'response'.`, err, null, null);
+  }
+  /* istanbul ignore next */
+  if (!err.response.status) {
+    return new ExecuteRequestError(`Error response did not contain expected field 'response.status'.`, err, null, null);
+  }
+
+  const status = err.response.status;
+
+  // If we don't get an error message from skyd, just return the status code.
+  /* istanbul ignore next */
+  if (!err.response.data) {
+    return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+  }
+  /* istanbul ignore next */
+  if (!err.response.data.message) {
+    return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+  }
+
+  // Return the error message from skyd. Pass along the original Axios error.
+  return new ExecuteRequestError(
+    `Request failed with status code ${err.response.status}: ${err.response.data.message}`,
+    err,
+    status,
+    err.response.data.message
+  );
+}
diff --git a/src/upload.ts b/src/upload.ts
index 0f8a5e4c..d6ce2865 100644
--- a/src/upload.ts
+++ b/src/upload.ts
@@ -280,14 +280,18 @@ export async function uploadLargeFileRequest(
         }
 
         // Call HEAD to get the metadata, including the skylink.
-        const resp = await this.executeRequest({
-          ...opts,
-          url: upload.url,
-          endpointPath: opts.endpointLargeUpload,
-          method: "head",
-          headers: { ...headers, "Tus-Resumable": "1.0.0" },
-        });
-        resolve(resp);
+        try {
+          const resp = await this.executeRequest({
+            ...opts,
+            url: upload.url,
+            endpointPath: opts.endpointLargeUpload,
+            method: "head",
+            headers: { ...headers, "Tus-Resumable": "1.0.0" },
+          });
+          resolve(resp);
+        } catch (err) {
+          reject(err);
+        }
       },
     };
 

From c400b17e4eb4d07f6fb1614c700336a09c21c079 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 3 Dec 2021 10:38:10 -0600
Subject: [PATCH 25/31] Add test to prove we now get skyd error messages

---
 integration/upload_download.test.ts | 7 +++++++
 src/client.ts                       | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/integration/upload_download.test.ts b/integration/upload_download.test.ts
index eb74673c..b15d9952 100644
--- a/integration/upload_download.test.ts
+++ b/integration/upload_download.test.ts
@@ -280,6 +280,13 @@ describe(`Upload and download end-to-end tests for portal '${portal}'`, () => {
     expect(etag2).toBeTruthy();
     expect(etag2).not.toEqual(etag1);
   });
+
+  it("should fail to download a non-existent skylink", async () => {
+    // Use a resolver skylink as it will time out faster.
+    const skylink = "AQDwh1jnoZas9LaLHC_D4-2yO9XYDdZzNtz62H4Dww1jDB";
+
+    await expect(client.getFileContent(skylink)).rejects.toThrowError("Failed to resolve skylink");
+  });
 });
 
 /**
diff --git a/src/client.ts b/src/client.ts
index 5fde77f2..893091c3 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -250,7 +250,7 @@ export class SkynetClient {
    *
    * @param config - Configuration for the request.
    * @returns - The response from axios.
-   * @throws - Will throw if the request fails.
+   * @throws - Will throw `ExecuteRequestError` if the request fails. This error contains the original Axios error.
    */
   async executeRequest(config: RequestConfig): Promise<AxiosResponse> {
     const url = await buildRequestUrl(this, {

From 663e344ef5dc860e5638df32584d442ed80f02e5 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Fri, 10 Dec 2021 13:52:12 -0600
Subject: [PATCH 26/31] Address review comments

---
 integration/skydb.test.ts |  2 ++
 src/client.ts             | 57 +++++++++++++++++++------------
 src/index.ts              |  2 +-
 src/request.ts            | 71 +++++++++++++++++++++------------------
 yarn.lock                 |  6 ++--
 5 files changed, 79 insertions(+), 59 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 8bf9c291..2870a3ae 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -143,6 +143,8 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
       await client.getFileContent(entryLink);
       throw new Error("'getFileContent' should not have succeeded");
     } catch (err) {
+      // Assert the type and that instanceof behaves as expected.
+      expect(err).toBeInstanceOf(ExecuteRequestError);
       expect((err as ExecuteRequestError).responseStatus).toEqual(404);
     }
 
diff --git a/src/client.ts b/src/client.ts
index 893091c3..820da3b9 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -46,7 +46,7 @@ import {
 import { addSubdomain, addUrlQuery, defaultPortalUrl, ensureUrlPrefix, makeUrl } from "./utils/url";
 import { loadMySky } from "./mysky";
 import { extractDomain, getFullDomainUrl } from "./mysky/utils";
-import { getPortalErrResponse } from "./request";
+import { ExecuteRequestError } from "./request";
 
 /**
  * Custom client options.
@@ -94,6 +94,21 @@ export type RequestConfig = CustomClientOptions & {
   transformResponse?: (data: string) => Record<string, unknown>;
 };
 
+// Add a response interceptor so that we always return an error of type
+// `ExecuteResponseError`.
+axios.interceptors.response.use(
+  function (response) {
+    // Any status code that lie within the range of 2xx cause this function to trigger.
+    // Do something with response data.
+    return response;
+  },
+  function (error) {
+    // Any status codes that falls outside the range of 2xx cause this function to trigger
+    // Do something with response error.
+    return Promise.reject(ExecuteRequestError.From(error as AxiosError));
+  }
+);
+
 /**
  * The Skynet Client which can be used to access Skynet.
  */
@@ -287,27 +302,25 @@ export class SkynetClient {
       };
     }
 
-    try {
-      return await axios({
-        url,
-        method: config.method,
-        data: config.data,
-        headers,
-        auth,
-        onDownloadProgress,
-        onUploadProgress,
-        responseType: config.responseType,
-        transformRequest: config.transformRequest,
-        transformResponse: config.transformResponse,
-
-        maxContentLength: Infinity,
-        maxBodyLength: Infinity,
-        // Allow cross-site cookies.
-        withCredentials: true,
-      });
-    } catch (e) {
-      throw getPortalErrResponse(e as AxiosError);
-    }
+    // NOTE: The error type will be ExecuteRequestError as we set up a response
+    // interceptor above.
+    return await axios({
+      url,
+      method: config.method,
+      data: config.data,
+      headers,
+      auth,
+      onDownloadProgress,
+      onUploadProgress,
+      responseType: config.responseType,
+      transformRequest: config.transformRequest,
+      transformResponse: config.transformResponse,
+
+      maxContentLength: Infinity,
+      maxBodyLength: Infinity,
+      // Allow cross-site cookies.
+      withCredentials: true,
+    });
   }
 
   // ===============
diff --git a/src/index.ts b/src/index.ts
index b884e2b7..6a93772a 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -34,6 +34,7 @@ export {
   deriveEncryptedFileSeed,
 } from "./mysky/encrypted_files";
 export { deriveDiscoverableFileTweak } from "./mysky/tweak";
+export { ExecuteRequestError } from "./request";
 export { DELETION_ENTRY_DATA } from "./skydb";
 export { convertSkylinkToBase32, convertSkylinkToBase64 } from "./skylink/format";
 export { parseSkylink } from "./skylink/parse";
@@ -82,7 +83,6 @@ export type {
   RegistryEntry,
   RegistryProofEntry,
 } from "./registry";
-export type { ExecuteRequestError } from "./request";
 export type {
   CustomGetJSONOptions,
   CustomSetJSONOptions,
diff --git a/src/request.ts b/src/request.ts
index 0538ae93..e5c73380 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -14,41 +14,46 @@ export class ExecuteRequestError extends Error {
     // Required for `instanceof` to work.
     Object.setPrototypeOf(this, ExecuteRequestError.prototype);
   }
-}
 
-/**
- * Gets the full, descriptive error response returned from skyd on the portal.
- *
- * @param err - The Axios error.
- * @returns - A new error if the error response is malformed, or the skyd error message otherwise.
- */
-export function getPortalErrResponse(err: AxiosError): ExecuteRequestError {
-  /* istanbul ignore next */
-  if (!err.response) {
-    return new ExecuteRequestError(`Error repsonse did not contain expected field 'response'.`, err, null, null);
-  }
-  /* istanbul ignore next */
-  if (!err.response.status) {
-    return new ExecuteRequestError(`Error response did not contain expected field 'response.status'.`, err, null, null);
-  }
+  /**
+   * Gets the full, descriptive error response returned from skyd on the portal.
+   *
+   * @param err - The Axios error.
+   * @returns - A new error if the error response is malformed, or the skyd error message otherwise.
+   */
+  static From(err: AxiosError): ExecuteRequestError {
+    /* istanbul ignore next */
+    if (!err.response) {
+      return new ExecuteRequestError(`Error repsonse did not contain expected field 'response'.`, err, null, null);
+    }
+    /* istanbul ignore next */
+    if (!err.response.status) {
+      return new ExecuteRequestError(
+        `Error response did not contain expected field 'response.status'.`,
+        err,
+        null,
+        null
+      );
+    }
 
-  const status = err.response.status;
+    const status = err.response.status;
 
-  // If we don't get an error message from skyd, just return the status code.
-  /* istanbul ignore next */
-  if (!err.response.data) {
-    return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
-  }
-  /* istanbul ignore next */
-  if (!err.response.data.message) {
-    return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
-  }
+    // If we don't get an error message from skyd, just return the status code.
+    /* istanbul ignore next */
+    if (!err.response.data) {
+      return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+    }
+    /* istanbul ignore next */
+    if (!err.response.data.message) {
+      return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+    }
 
-  // Return the error message from skyd. Pass along the original Axios error.
-  return new ExecuteRequestError(
-    `Request failed with status code ${err.response.status}: ${err.response.data.message}`,
-    err,
-    status,
-    err.response.data.message
-  );
+    // Return the error message from skyd. Pass along the original Axios error.
+    return new ExecuteRequestError(
+      `Request failed with status code ${err.response.status}: ${err.response.data.message}`,
+      err,
+      status,
+      err.response.data.message
+    );
+  }
 }
diff --git a/yarn.lock b/yarn.lock
index c1d0d6fd..f3585ca4 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1272,9 +1272,9 @@ camelcase@^6.0.0:
   integrity sha512-c7wVvbw3f37nuobQNtgsgG9POC9qMbNuMQmTCqZv23b6MIz0fcYpBiOlv9gEN/hdLdnZTDQhg6e9Dq5M1vKvfg==
 
 caniuse-lite@^1.0.30001219:
-  version "1.0.30001235"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001235.tgz#ad5ca75bc5a1f7b12df79ad806d715a43a5ac4ed"
-  integrity sha512-zWEwIVqnzPkSAXOUlQnPW2oKoYb2aLQ4Q5ejdjBcnH63rfypaW34CxaeBn1VMya2XaEU3P/R2qHpWyj+l0BT1A==
+  version "1.0.30001286"
+  resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001286.tgz"
+  integrity sha512-zaEMRH6xg8ESMi2eQ3R4eZ5qw/hJiVsO/HlLwniIwErij0JDr9P+8V4dtx1l+kLq6j3yy8l8W4fst1lBnat5wQ==
 
 capture-exit@^2.0.0:
   version "2.0.0"

From dce69c37b202d99ef1819339bb8d685e6b38e04f Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Thu, 25 Nov 2021 13:34:15 -0600
Subject: [PATCH 27/31] [Breaking] Use `sign.detached` instead of `sign` to get
 signature

Previously, the returned signature was longer than `SIGNATURE_LENGTH`. This is
because `sign` returns the signed message (including the original message) which
was not necessary -- we only need it to return the signature.

In addition to being unnecessary, it was also confusing to have a signature that
was longer than expected.

We should have been using `sign.detached`. We already use `verify.detached`.
---
 src/registry.test.ts | 23 +++++++++++++++++++++--
 src/registry.ts      |  2 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/registry.test.ts b/src/registry.test.ts
index 680cee09..56dfe053 100644
--- a/src/registry.test.ts
+++ b/src/registry.test.ts
@@ -1,7 +1,7 @@
 import axios from "axios";
 import MockAdapter from "axios-mock-adapter";
 
-import { genKeyPairAndSeed } from "./crypto";
+import { genKeyPairAndSeed, SIGNATURE_LENGTH } from "./crypto";
 import { SkynetClient, defaultSkynetPortalUrl, genKeyPairFromSeed } from "./index";
 import { getEntryLink, getEntryUrlForPortal, signEntry, validateRegistryProof } from "./registry";
 import { uriSkynetPrefix } from "./utils/url";
@@ -166,7 +166,26 @@ describe("setEntry", () => {
 });
 
 describe("signEntry", () => {
-  it("Should throw if we try to sign an entry with a prehashed data key that is not in hex format", async () => {
+  it("should derive the correct signature", async () => {
+    // Hard-code expected value to catch breaking changes.
+    const expectedSignature = [
+      133, 154, 188, 25, 22, 198, 83, 227, 64, 89, 92, 137, 232, 240, 27, 215, 31, 207, 179, 160, 142, 4, 136, 12, 137,
+      119, 163, 150, 210, 114, 50, 94, 157, 128, 80, 18, 54, 125, 230, 233, 44, 109, 167, 40, 132, 33, 134, 13, 75, 48,
+      130, 200, 120, 173, 141, 196, 238, 235, 178, 186, 48, 208, 241, 13,
+    ];
+    const entry = {
+      data: new Uint8Array([1, 2, 3]),
+      dataKey: "foo",
+      revision: BigInt(11),
+    };
+
+    const signature = await signEntry(privateKey, entry, false);
+
+    expect(signature.length).toEqual(SIGNATURE_LENGTH);
+    expect(signature).toEqual(new Uint8Array(expectedSignature));
+  });
+
+  it("should throw if we try to sign an entry with a prehashed data key that is not in hex format", async () => {
     const entry = { data: stringToUint8ArrayUtf8("test"), dataKey: "test", revision: BigInt(0) };
     await expect(signEntry(privateKey, entry, true)).rejects.toThrowError(
       "Expected parameter 'str' to be a hex-encoded string, was type 'string', value 'test'"
diff --git a/src/registry.ts b/src/registry.ts
index a4518f78..62d5ffed 100644
--- a/src/registry.ts
+++ b/src/registry.ts
@@ -432,7 +432,7 @@ export async function signEntry(
 
   // Sign the entry.
   // TODO: signature type should be Signature?
-  return sign(hashRegistryEntry(entry, hashedDataKeyHex), privateKeyArray);
+  return sign.detached(hashRegistryEntry(entry, hashedDataKeyHex), privateKeyArray);
 }
 
 /**

From b840ef102c3df6b21a14a74a2190b2985d88f7e2 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 13 Dec 2021 11:07:21 -0600
Subject: [PATCH 28/31] Fix tests failing after merge

---
 integration/skydb.test.ts | 20 +++++++++++++++++---
 src/registry.test.ts      |  6 ++++--
 src/request.ts            |  4 ++--
 src/skydb.test.ts         |  4 +++-
 4 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 2870a3ae..211cb399 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -320,7 +320,11 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           ]);
         } catch (e) {
           if ((e as Error).message.includes(concurrentAccessError)) {
-            // The data race condition has been prevented and we received the expected error. Return from test early.
+            // The data race condition has been prevented and we received the
+            // expected error. Return from test early.
+            //
+            // NOTE: I've manually confirmed that both code paths (no error, and
+            // return on expected error) are hit.
             return;
           }
 
@@ -370,6 +374,8 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         expect(cachedRevisionEntry1.revision.toString()).toEqual("1");
         if (receivedJson?.message === jsonNew.message) {
           expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
+          // NOTE: I've manually confirmed that both code paths (no error, and
+          // return on expected error) are hit.
           return;
         }
         expect(receivedJson).toEqual(jsonOld);
@@ -406,7 +412,11 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           ]);
         } catch (e) {
           if ((e as Error).message.includes(concurrentAccessError)) {
-            // The data race condition has been prevented and we received the expected error. Return from test early.
+            // The data race condition has been prevented and we received the
+            // expected error. Return from test early.
+            //
+            // NOTE: I've manually confirmed that both code paths (no error, and
+            // return on expected error) are hit.
             return;
           }
 
@@ -436,7 +446,11 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           ]);
         } catch (e) {
           if ((e as Error).message.includes(registryUpdateError)) {
-            // The data race condition has been prevented and we received the expected error. Return from test early.
+            // The data race condition has been prevented and we received the
+            // expected error. Return from test early.
+            //
+            // NOTE: I've manually confirmed that both code paths (no error, and
+            // return on expected error) are hit.
             return;
           }
 
diff --git a/src/registry.test.ts b/src/registry.test.ts
index 56dfe053..e4385ad9 100644
--- a/src/registry.test.ts
+++ b/src/registry.test.ts
@@ -26,10 +26,12 @@ describe("getEntry", () => {
   it("should throw if the response status is not in the 200s and not 404 and JSON is returned", async () => {
     mock.onGet(registryGetUrl).replyOnce(400, JSON.stringify({ message: "foo error" }));
 
-    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(new Error("foo error"));
+    await expect(client.registry.getEntry(publicKey, dataKey)).rejects.toEqual(
+      new Error("Request failed with status code 400: foo error")
+    );
   });
 
-  // In the case of a 429 error due to rate limiting all we get is HTML.
+  // In the case of a 429 error due to rate limiting, all we get is HTML.
   it("should throw if the response status is not in the 200s and not 404 and HTML is returned", async () => {
     const responseHTML = `
 <head><title>429 Too Many Requests</title></head>
diff --git a/src/request.ts b/src/request.ts
index e5c73380..281e5c3b 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -41,11 +41,11 @@ export class ExecuteRequestError extends Error {
     // If we don't get an error message from skyd, just return the status code.
     /* istanbul ignore next */
     if (!err.response.data) {
-      return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+      return new ExecuteRequestError(`Request failed with status code ${status}`, err, status, null);
     }
     /* istanbul ignore next */
     if (!err.response.data.message) {
-      return new ExecuteRequestError(`Request failed with status code ${status}.`, err, status, null);
+      return new ExecuteRequestError(`Request failed with status code ${status}`, err, status, null);
     }
 
     // Return the error message from skyd. Pass along the original Axios error.
diff --git a/src/skydb.test.ts b/src/skydb.test.ts
index 2d56ac66..851d5ca0 100644
--- a/src/skydb.test.ts
+++ b/src/skydb.test.ts
@@ -295,7 +295,9 @@ describe("setJSON", () => {
     // mock a failed registry update
     mock.onPost(registryPostUrl).replyOnce(400, JSON.stringify({ message: "foo" }));
 
-    await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toEqual(new Error("foo"));
+    await expect(client.db.setJSON(privateKey, dataKey, json)).rejects.toEqual(
+      new Error("Request failed with status code 400: foo")
+    );
 
     const revision2 = cachedRevisionEntry.revision;
 

From f57bae0c9571bb83b502a354f637a18fba408797 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Mon, 13 Dec 2021 11:46:33 -0600
Subject: [PATCH 29/31] Fix typo

---
 src/request.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/request.ts b/src/request.ts
index 281e5c3b..e20d3fc2 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -24,7 +24,7 @@ export class ExecuteRequestError extends Error {
   static From(err: AxiosError): ExecuteRequestError {
     /* istanbul ignore next */
     if (!err.response) {
-      return new ExecuteRequestError(`Error repsonse did not contain expected field 'response'.`, err, null, null);
+      return new ExecuteRequestError(`Error response did not contain expected field 'response'.`, err, null, null);
     }
     /* istanbul ignore next */
     if (!err.response.status) {

From 7d394d4c054a9e7bfb9ecd1645c49f6de5c0fab7 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 15 Dec 2021 09:25:13 -0600
Subject: [PATCH 30/31] Increase timeout

---
 integration/skydb.test.ts | 3 +++
 jest.config.ts            | 2 +-
 src/skydb.ts              | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index f29792ea..4542f280 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -365,6 +365,7 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         // Set the initial data.
         await client1.db.setJSON(privateKey, dataKey, jsonOld);
         expect(cachedRevisionEntry1.revision.toString()).toEqual("0");
+        expect(cachedRevisionEntry2.revision.toString()).toEqual("-1");
 
         // Call getJSON and setJSON concurrently on different clients -- both
         // should succeeed.
@@ -386,9 +387,11 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
           // return on expected error) are hit.
           return;
         }
+        // client2 should have old data and cached revision at this point.
         expect(receivedJson).toEqual(jsonOld);
         expect(cachedRevisionEntry2.revision.toString()).toEqual("0");
 
+        // Try to update the entry with client2 which has the old revision.
         const updatedJson = { message: 3 };
         // Catches both "doesn't have enough pow" and "provided revision number
         // is already registered" errors.
diff --git a/jest.config.ts b/jest.config.ts
index c1a69876..8ce02cf2 100644
--- a/jest.config.ts
+++ b/jest.config.ts
@@ -11,7 +11,7 @@ const config: Config.InitialOptions = {
   preset: "ts-jest",
 
   // From old package.json.
-  testTimeout: 90000,
+  testTimeout: 120000,
   // Automatically clear mock calls and instances between every test
   clearMocks: true,
   // An array of glob patterns indicating a set of files for which coverage information should be collected
diff --git a/src/skydb.ts b/src/skydb.ts
index c3a2f0b5..213b4442 100644
--- a/src/skydb.ts
+++ b/src/skydb.ts
@@ -247,7 +247,7 @@ export async function setJSON(
 
   // Immediately fail if the mutex is not available.
   return await this.db.revisionNumberCache.withCachedEntryLock(publicKey, dataKey, async (cachedRevisionEntry) => {
-    // Get the cached revision number before doing anything else.
+    // Get the cached revision number before doing anything else. Increment it.
     const newRevision = incrementRevision(cachedRevisionEntry.revision);
 
     const [entry, dataLink] = await getOrCreateSkyDBRegistryEntry(this, dataKey, json, newRevision, opts);

From 9a76d0ab40c8418fb6142ed19b33c172722742ff Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@bytedude.com>
Date: Wed, 15 Dec 2021 15:31:05 -0600
Subject: [PATCH 31/31] Revise failing test

---
 integration/skydb.test.ts | 101 ++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 36 deletions(-)

diff --git a/integration/skydb.test.ts b/integration/skydb.test.ts
index 4542f280..fbc04b5b 100644
--- a/integration/skydb.test.ts
+++ b/integration/skydb.test.ts
@@ -345,14 +345,21 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
       }
     );
 
+    // NOTE: We can't guarantee that data won't be lost if two (or more) actors
+    // write to the registry at the same time, but we can guarantee that the
+    // final state will be the desired final state by at least one of the
+    // actors. One of the two clients will lose, but the other will win and be
+    // consistent, so the data won't be corrupt, it'll just be missing one
+    // update.
     it.each(delays)(
-      "should not be able to use old data when getJSON is called after setJSON on two different clients with a '%s' ms delay",
+      "should get either old or new data when getJSON is called after setJSON on two different clients with a '%s' ms delay",
       async (delay) => {
         // Create two new clients with a fresh revision cache.
         const client1 = new SkynetClient(portal);
         const client2 = new SkynetClient(portal);
         const { publicKey, privateKey } = genKeyPairAndSeed();
 
+        // Get revision entry cache handles.
         const cachedRevisionEntry1 = await client1.db.revisionNumberCache.getRevisionAndMutexForEntry(
           publicKey,
           dataKey
@@ -363,50 +370,72 @@ describe(`SkyDB end to end integration tests for portal '${portal}'`, () => {
         );
 
         // Set the initial data.
-        await client1.db.setJSON(privateKey, dataKey, jsonOld);
-        expect(cachedRevisionEntry1.revision.toString()).toEqual("0");
-        expect(cachedRevisionEntry2.revision.toString()).toEqual("-1");
+        {
+          await client1.db.setJSON(privateKey, dataKey, jsonOld);
+          expect(cachedRevisionEntry1.revision.toString()).toEqual("0");
+          expect(cachedRevisionEntry2.revision.toString()).toEqual("-1");
+        }
 
         // Call getJSON and setJSON concurrently on different clients -- both
         // should succeeed.
+        {
+          // Get the data while also calling setJSON.
+          const [_, { data: receivedJson }] = await Promise.all([
+            setJSONWithDelay(client1, 0, privateKey, dataKey, jsonNew),
+            getJSONWithDelay(client2, delay, publicKey, dataKey),
+          ]);
 
-        // Get the data while also calling setJSON.
-        const [_, { data: receivedJson }] = await Promise.all([
-          setJSONWithDelay(client1, 0, privateKey, dataKey, jsonNew),
-          getJSONWithDelay(client2, delay, publicKey, dataKey),
-        ]);
-
-        // If we got old data from getJSON, make sure that that client is not
-        // able to update that JSON.
-
-        expect(receivedJson).not.toBeNull();
-        expect(cachedRevisionEntry1.revision.toString()).toEqual("1");
-        if (receivedJson?.message === jsonNew.message) {
-          expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
-          // NOTE: I've manually confirmed that both code paths (no error, and
-          // return on expected error) are hit.
-          return;
+          // See if we got the new or old data.
+          expect(receivedJson).not.toBeNull();
+          expect(cachedRevisionEntry1.revision.toString()).toEqual("1");
+          if (receivedJson?.message === jsonNew.message) {
+            expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
+            // Return if we got the new data -- both clients are in sync.
+            //
+            // NOTE: I've manually confirmed that both code paths (old data and
+            // new data) are hit.
+            return;
+          }
+          // client2 should have old data and cached revision at this point.
+          expect(receivedJson).toEqual(jsonOld);
+          expect(cachedRevisionEntry2.revision.toString()).toEqual("0");
         }
-        // client2 should have old data and cached revision at this point.
-        expect(receivedJson).toEqual(jsonOld);
-        expect(cachedRevisionEntry2.revision.toString()).toEqual("0");
 
+        // If we got old data and an old revision from getJSON, the client may
+        // still be able to write to that entry, overwriting the new data.
+        //
         // Try to update the entry with client2 which has the old revision.
         const updatedJson = { message: 3 };
-        // Catches both "doesn't have enough pow" and "provided revision number
-        // is already registered" errors.
-        await expect(client2.db.setJSON(privateKey, dataKey, updatedJson)).rejects.toThrowError(registryUpdateError);
-
-        // Should work on that client again after calling getJSON.
-
-        const { data: receivedJson2 } = await client2.db.getJSON(publicKey, dataKey);
-        expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
-        expect(receivedJson2).toEqual(jsonNew);
+        let expectedJson: JsonData;
+        try {
+          await client2.db.setJSON(privateKey, dataKey, updatedJson);
+          expectedJson = updatedJson;
+        } catch (e) {
+          // Catches both "doesn't have enough pow" and "provided revision number
+          // is already registered" errors.
+          if ((e as Error).message.includes(registryUpdateError)) {
+            // NOTE: I've manually confirmed that both code paths (no error, and
+            // return on expected error) are hit.
+            expectedJson = jsonNew;
+          } else {
+            // Unexpected error, throw.
+            throw e;
+          }
+        }
 
-        const updatedJson2 = receivedJson2 as { message: number };
-        updatedJson2.message++;
-        await client2.db.setJSON(privateKey, dataKey, updatedJson2);
-        expect(cachedRevisionEntry2.revision.toString()).toEqual("2");
+        // The entry should have the overriden, updated data at this point.
+        await Promise.all([
+          async () => {
+            const { data: receivedJson } = await client1.db.getJSON(publicKey, dataKey);
+            expect(cachedRevisionEntry1.revision.toString()).toEqual("1");
+            expect(receivedJson).toEqual(expectedJson);
+          },
+          async () => {
+            const { data: receivedJson } = await client2.db.getJSON(publicKey, dataKey);
+            expect(cachedRevisionEntry2.revision.toString()).toEqual("1");
+            expect(receivedJson).toEqual(expectedJson);
+          },
+        ]);
       }
     );