Remove GRIST_SKIP_REDIS_CHECKSUM_MISMATCH (#1098)

Skipping the redis checksum mismatch is now generalized. A warning is logged when we see a mismatch.
gristlabs · Jul 10, 2024 · 39eb042 · 39eb042
1 parent d336293
commit 39eb042
Show file tree

Hide file tree

Showing 3 changed files with 5 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -316,7 +316,6 @@ Grist can be configured in many ways. Here are the main environment variables it
 | HOME_PORT                          | port number to listen on for REST API server; if set to "share", add API endpoints to regular grist port.                                                                                                                                                                                                                                                     |
 | PORT                               | port number to listen on for Grist server                                                                                                                                                                                                                                                                                                                     |
 | REDIS_URL                          | optional redis server for browser sessions and db query caching                                                                                                                                                                                                                                                                                               |
-| GRIST_SKIP_REDIS_CHECKSUM_MISMATCH | Experimental. If set, only warn if the checksum in Redis differs with the one in your S3 backend storage. You may turn it on if your backend storage implements the [read-after-write consistency](https://aws.amazon.com/fr/blogs/aws/amazon-s3-update-strong-read-after-write-consistency/). Defaults to false.                                             |
 | GRIST_SNAPSHOT_TIME_CAP            | optional. Define the caps for tracking buckets. Usage: {"hour": 25, "day": 32, "isoWeek": 12, "month": 96, "year": 1000}                                                                                                                                                                                                                                      |
 | GRIST_SNAPSHOT_KEEP                | optional. Number of recent snapshots to retain unconditionally for a document, regardless of when they were made                                                                                                                                                                                                                                              |
 | GRIST_PROMCLIENT_PORT              | optional. If set, serve the Prometheus metrics on the specified port number. ⚠️ Be sure to use a port which is not publicly exposed ⚠️.                                                                                                                                                                                                                         |

diff --git a/app/server/lib/ExternalStorage.ts b/app/server/lib/ExternalStorage.ts
@@ -1,5 +1,4 @@
 import {ObjMetadata, ObjSnapshot, ObjSnapshotWithMetadata} from 'app/common/DocSnapshot';
-import {isAffirmative} from 'app/common/gutil';
 import log from 'app/server/lib/log';
 import {createTmpDir} from 'app/server/lib/uploads';
 
@@ -236,19 +235,8 @@ export class ChecksummedExternalStorage implements ExternalStorage {
           // We are confident this should not be the case anymore, though this has to be studied carefully.
           // If a snapshotId was specified, we can skip this check.
           if (expectedChecksum && expectedChecksum !== checksum) {
-            const message = `ext ${this.label} download: data for ${fromKey} has wrong checksum:` +
-              ` ${checksum} (expected ${expectedChecksum})`;
-
-            // If GRIST_SKIP_REDIS_CHECKSUM_MISMATCH is set, issue a warning only and continue,
-            // rather than issuing an error and failing.
-            // This flag is experimental and should be removed once we are
-            // confident that the checksums verification is useless.
-            if (isAffirmative(process.env.GRIST_SKIP_REDIS_CHECKSUM_MISMATCH)) {
-              log.warn(message);
-            } else {
-              log.error(message);
-              return undefined;
-            }
+            log.warn(`ext ${this.label} download: data for ${fromKey} has wrong checksum:` +
+              ` ${checksum} (expected ${expectedChecksum})`);
           }
         }
 

diff --git a/test/server/lib/HostedStorageManager.ts b/test/server/lib/HostedStorageManager.ts
@@ -494,18 +494,12 @@ describe('HostedStorageManager', function() {
         await setRedisChecksum(docId, 'nobble');
         await store.removeAll();
 
-        // With GRIST_SKIP_REDIS_CHECKSUM_MISMATCH set, the fetch should work
-        process.env.GRIST_SKIP_REDIS_CHECKSUM_MISMATCH = 'true';
+        const warnSpy = sandbox.spy(log, 'warn');
         await store.run(async () => {
           await assert.isFulfilled(store.docManager.fetchDoc(docSession, docId));
+          assert.isTrue(warnSpy.calledWithMatch('has wrong checksum'), 'a warning should have been logged');
         });
-
-        // By default, the fetch should eventually errors.
-        delete process.env.GRIST_SKIP_REDIS_CHECKSUM_MISMATCH;
-        await store.run(async () => {
-          await assert.isRejected(store.docManager.fetchDoc(docSession, docId),
-                                  /operation failed to become consistent/);
-        });
+        warnSpy.restore();
 
         // Check we get the document back on fresh start if checksum is correct.
         await setRedisChecksum(docId, checksum);