From c6eca28ff8e551bd2e76f0c91b1a70ddee60b35e Mon Sep 17 00:00:00 2001 From: David Steele Date: Sun, 27 Sep 2026 10:54:50 +0300 Subject: [PATCH] Disable read retry when filters are passed to the driver. When a read is retried the driver is freed and recreated but the filter group is not set on the new driver. For drivers that require the filter group (i.e. remote) this causes a segfault when the read is opened because the filter group is NULL. This was introduced in a953aa7, which changed retry to recreate the driver rather than reopen it, added the filter group callback that is only set when the read is created, and replaced the driver retry flag with the storageFeatureReadRetry feature, which remote storage inherits from the repository host. Before that commit retry was never enabled for remote reads. Even with the filter group set the retry could not work because the filters are passed to the remote when the read is opened so they cannot be run again, and bytes read are counted after the filters have been applied so the offset to restart the read cannot be determined. Read retry is only enabled for object stores so this happened when the repository was on an object store accessed via a repository host. The repository host still retries the object store read, where the filters are applied above the driver and the retry works correctly, so the only retry lost is a redundant second attempt by the client. --- doc/xml/release/2020s/2026/2.59.2.xml | 13 +++++++++++++ doc/xml/release/contributor.xml | 5 +++++ src/storage/read.c | 12 ++++++++++++ test/src/module/storage/remoteTest.c | 23 +++++++++++++++++++++++ 4 files changed, 53 insertions(+) diff --git a/doc/xml/release/2020s/2026/2.59.2.xml b/doc/xml/release/2020s/2026/2.59.2.xml index 00f0e415f..71d4a15ff 100644 --- a/doc/xml/release/2020s/2026/2.59.2.xml +++ b/doc/xml/release/2020s/2026/2.59.2.xml @@ -14,6 +14,19 @@

Fix empty block map when file is truncated to zero during backup.

+ + + + + + + + + + +

Disable read retry when filters are passed to the driver.

+ + diff --git a/doc/xml/release/contributor.xml b/doc/xml/release/contributor.xml index 970bc4b56..aee1fa716 100644 --- a/doc/xml/release/contributor.xml +++ b/doc/xml/release/contributor.xml @@ -890,6 +890,11 @@ navaneswarareddy + + megamaced + megamaced + + mibiio mibiio diff --git a/src/storage/read.c b/src/storage/read.c index 0653bf777..337d9060a 100644 --- a/src/storage/read.c +++ b/src/storage/read.c @@ -119,6 +119,10 @@ storageRead(THIS_VOID, Buffer *const buffer, const bool block) .limit = this->pub.limit != NULL ? varNewUInt64(varUInt64(this->pub.limit) - this->bytesRead) : NULL, .versionId = this->pub.versionId); + // The filter group is never set on the recreated driver, so retry must already be disabled for any driver + // that requires one (see storageReadNew()) + ASSERT(storageReadDriverInterface(this->driver)->filterGroup == NULL); + // Error when the file is missing since it must have existed when the read began if (!storageReadDriverInterface(this->driver)->open(this->driver)) THROW_FMT(FileMissingError, STORAGE_ERROR_READ_MISSING, strZ(storageReadName(this))); @@ -259,7 +263,15 @@ storageReadNew( // Set filter group when interface function exists if (storageReadDriverInterface(this->driver)->filterGroup != NULL) + { storageReadDriverInterface(this->driver)->filterGroup(this->driver, ioReadFilterGroup(storageReadIo(this))); + + // Disable retry when the driver requires the filter group. The filters are passed to the driver when the read is + // opened, e.g. to be run on a remote host, so they cannot be run again for a retry. Also, bytes read are counted after + // the filters have been applied so the offset to restart the read cannot be determined. Retry in this case is expected + // to be handled by the driver, e.g. the remote host retries reads on object stores. + this->retry = false; + } } OBJ_NEW_END(); diff --git a/test/src/module/storage/remoteTest.c b/test/src/module/storage/remoteTest.c index f4797f404..f08b6c64f 100644 --- a/test/src/module/storage/remoteTest.c +++ b/test/src/module/storage/remoteTest.c @@ -415,6 +415,29 @@ testRun(void) " 7:strid:buffer", "filter results"); + // ------------------------------------------------------------------------------------------------------------------------- + TEST_TITLE("read error is not retried when filters are passed to the remote"); + + // Write an encrypted file larger than the buffer size so the error will happen after the first chunk has been read + HRN_STORAGE_PUT( + storageTest, TEST_PATH "/repo128/test.bin", contentBuf, .cipherType = cipherTypeAes256Cbc, .cipherPass = "x"); + + // Enable read retry to show that it is disabled when the filters are passed to the remote + uint64_t featureOld = storageRepo->pub.interface.feature; + storageRepo->pub.interface.feature |= 1 << storageFeatureReadRetry; + + // Limit the read so it ends in the middle of a cipher block and the decrypt filter errors on the remote + TEST_ASSIGN( + fileRead, storageNewReadP(storageRepo, STRDEF("test.bin"), .limit = VARUINT64(bufSize(contentBuf) - 5)), "new read"); + TEST_RESULT_BOOL(fileRead->retry, false, "retry is disabled"); + + ioFilterGroupAdd( + ioReadFilterGroup(storageReadIo(fileRead)), cipherBlockNewP(cipherModeDecrypt, cipherTypeAes256Cbc, BUFSTRDEF("x"))); + + TEST_ERROR(storageGetP(fileRead), CryptoError, "raised from remote-0 shim protocol: unable to flush"); + + storageRepo->pub.interface.feature = featureOld; + // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("error on invalid filter");