From 39eb042ff166626a923bb07c9e8eb557d99b5e6e Mon Sep 17 00:00:00 2001 From: Florent Date: Wed, 10 Jul 2024 20:28:20 +0200 Subject: [PATCH] Remove GRIST_SKIP_REDIS_CHECKSUM_MISMATCH (#1098) Skipping the redis checksum mismatch is now generalized. A warning is logged when we see a mismatch. --- README.md | 1 - app/server/lib/ExternalStorage.ts | 16 ++-------------- test/server/lib/HostedStorageManager.ts | 12 +++--------- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index dd44a0113d..92de74e81e 100644 --- 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 index d56361091b..c92fb0bbc9 100644 --- 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 index 2fee78c208..aa164892f8 100644 --- 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);