From 477ff5f6ccc2eef9df311b0c54343e9c9b3ecece Mon Sep 17 00:00:00 2001 From: michael stack Date: Wed, 23 Sep 2026 16:38:56 -0700 Subject: [PATCH 1/2] Make fdbbackup query exit non-zero when it fails Every failure path printed a JSON error object and returned normally, so stopAfter() yielded a present Optional and main() kept FDB_EXIT_SUCCESS. Automation checking the exit code read failures as successes. queryBackup now reports its status through the fstatus future, which was already declared and inspected for this but never assigned. An absent value means stopAfter() absorbed a fatal error, so it maps to FDB_EXIT_ERROR. Throwing instead would emit SevError and a stderr line for an ordinary "no restorable files set" answer, breaking stdout as a single parseable document. --- fdbbackup/backup.cpp | 57 ++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/fdbbackup/backup.cpp b/fdbbackup/backup.cpp index c3bc4d8cd28..7d99d0b97ca 100644 --- a/fdbbackup/backup.cpp +++ b/fdbbackup/backup.cpp @@ -2529,16 +2529,16 @@ std::pair getMaxMinRestorableVersions(const BackupDescription& // If restoreVersion is invalidVersion or latestVersion, use the maximum or minimum restorable version respectively for // selected key ranges. If restoreTimestamp is specified, any specified restoreVersion will be overridden to the version // resolved to that timestamp. -Future queryBackup(const char* name, - std::string destinationContainer, - Optional proxy, - Standalone> keyRangesFilter, - Version restoreVersion, - Version snapshotVersion, - std::string originalClusterFile, - std::string restoreTimestamp, - Verbose verbose, - Optional cx) { +Future queryBackup(const char* name, + std::string destinationContainer, + Optional proxy, + Standalone> keyRangesFilter, + Version restoreVersion, + Version snapshotVersion, + std::string originalClusterFile, + std::string restoreTimestamp, + Verbose verbose, + Optional cx) { UID operationId = deterministicRandom()->randomUniqueID(); JsonBuilderObject result; std::string errorMessage; @@ -2562,7 +2562,7 @@ Future queryBackup(const char* name, result, format("a cluster file must be given in order to resolve restore target timestamp '%s'", restoreTimestamp.c_str())); - co_return; + co_return FDB_EXIT_ERROR; } if (!fileExists(originalClusterFile)) { @@ -2570,7 +2570,7 @@ Future queryBackup(const char* name, result, format("The specified original source database cluster file '%s' does not exist\n", originalClusterFile.c_str())); - co_return; + co_return FDB_EXIT_ERROR; } Database origDb = Database::createDatabase(originalClusterFile, ApiVersion::LATEST_VERSION); @@ -2607,7 +2607,7 @@ Future queryBackup(const char* name, result, errorMessage = format("the specified restorable version %lld is not valid", restoreVersion)); - co_return; + co_return FDB_EXIT_ERROR; } Optional fileSet; @@ -2655,7 +2655,7 @@ Future queryBackup(const char* name, result, format("no restorable files set found for specified key ranges from snapshotVersion %lld", snapshotVersion)); - co_return; + co_return FDB_EXIT_ERROR; } // We only need to know all the mutation logs from `snapshotVersion` to `restoreVersion`. @@ -2698,12 +2698,12 @@ Future queryBackup(const char* name, .detail("LogFilesBytes", totalLogFilesSize); } else if (snapshotVersion == invalidVersion) { reportBackupQueryError(operationId, result, "no restorable files set found for specified key ranges"); - co_return; + co_return FDB_EXIT_ERROR; } } catch (Error& e) { reportBackupQueryError(operationId, result, e.what()); - co_return; + co_return FDB_EXIT_ERROR; } result["total_range_files_size"] = totalRangeFilesSize; @@ -2715,6 +2715,7 @@ Future queryBackup(const char* name, } printf("%s\n", result.getJson().c_str()); + co_return FDB_EXIT_SUCCESS; } Future listBackup(std::string baseUrl, Optional proxy) { @@ -4279,16 +4280,16 @@ int main(int argc, char* argv[]) { case BackupType::QUERY: initTraceFile(); - f = stopAfter(queryBackup(newArgV[0], - destinationContainer, - proxy, - backupKeysFilter, - restoreVersion, - snapshotVersion, - clusterFile, - restoreTimestamp, - Verbose{ !quietDisplay }, - db)); + fstatus = stopAfter(queryBackup(newArgV[0], + destinationContainer, + proxy, + backupKeysFilter, + restoreVersion, + snapshotVersion, + clusterFile, + restoreTimestamp, + Verbose{ !quietDisplay }, + db)); break; case BackupType::DUMP: @@ -4432,8 +4433,8 @@ int main(int argc, char* argv[]) { status = FDB_EXIT_ERROR; } - if (fstatus.isValid() && fstatus.isReady() && !fstatus.isError() && fstatus.get().present()) { - status = fstatus.get().get(); + if (fstatus.isValid() && fstatus.isReady() && !fstatus.isError()) { + status = fstatus.get().orDefault(FDB_EXIT_ERROR); } #ifdef ALLOC_INSTRUMENTATION From 3f8db9626107299972e795f3fce32031d109ba8e Mon Sep 17 00:00:00 2001 From: michael stack Date: Thu, 22 Oct 2026 11:25:26 -0700 Subject: [PATCH 2/2] fdbbackup query: pin exit codes in dir_backup_tests, report timestamp-resolution errors dir_backup_tests now runs `fdbbackup query` against an existing backup (exit 0) and a missing one (non-zero). The success check is skipped for encrypted backups because query cannot be given an encryption key and fails with "Restore error" on the manifest. Move the restore-timestamp resolution inside queryBackup's try so a failure there is reported as a JSON error with FDB_EXIT_ERROR like every other failure, instead of escaping to stopAfter(). --- fdbbackup/backup.cpp | 53 +++++++++++++++--------------- fdbbackup/tests/dir_backup_test.sh | 44 +++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/fdbbackup/backup.cpp b/fdbbackup/backup.cpp index 7d99d0b97ca..df0a9d21cb1 100644 --- a/fdbbackup/backup.cpp +++ b/fdbbackup/backup.cpp @@ -2554,37 +2554,38 @@ Future queryBackup(const char* name, .detail("RestoreTimestamp", restoreTimestamp) .detail("BackupClusterFile", originalClusterFile); - // Resolve restoreTimestamp if given - if (!restoreTimestamp.empty()) { - if (originalClusterFile.empty()) { - reportBackupQueryError( - operationId, - result, - format("a cluster file must be given in order to resolve restore target timestamp '%s'", - restoreTimestamp.c_str())); - co_return FDB_EXIT_ERROR; - } - - if (!fileExists(originalClusterFile)) { - reportBackupQueryError(operationId, - result, - format("The specified original source database cluster file '%s' does not exist\n", - originalClusterFile.c_str())); - co_return FDB_EXIT_ERROR; - } - - Database origDb = Database::createDatabase(originalClusterFile, ApiVersion::LATEST_VERSION); - Version v = co_await timeKeeperVersionFromDatetime(restoreTimestamp, origDb); - result["restore_timestamp"] = restoreTimestamp; - result["restore_timestamp_resolved_version"] = v; - restoreVersion = v; - } - int64_t totalRangeFilesSize = 0; int64_t totalLogFilesSize = 0; JsonBuilderArray rangeFilesJson; JsonBuilderArray logFilesJson; try { + // Resolve restoreTimestamp if given + if (!restoreTimestamp.empty()) { + if (originalClusterFile.empty()) { + reportBackupQueryError( + operationId, + result, + format("a cluster file must be given in order to resolve restore target timestamp '%s'", + restoreTimestamp.c_str())); + co_return FDB_EXIT_ERROR; + } + + if (!fileExists(originalClusterFile)) { + reportBackupQueryError( + operationId, + result, + format("The specified original source database cluster file '%s' does not exist\n", + originalClusterFile.c_str())); + co_return FDB_EXIT_ERROR; + } + + Database origDb = Database::createDatabase(originalClusterFile, ApiVersion::LATEST_VERSION); + Version v = co_await timeKeeperVersionFromDatetime(restoreTimestamp, origDb); + result["restore_timestamp"] = restoreTimestamp; + result["restore_timestamp_resolved_version"] = v; + restoreVersion = v; + } + Reference bc = openBackupContainer(name, destinationContainer, proxy, {}, 0); BackupDescription desc = co_await bc->describeBackup(); // Use continuous log end version for the maximum restorable version for the key ranges when a restorable diff --git a/fdbbackup/tests/dir_backup_test.sh b/fdbbackup/tests/dir_backup_test.sh index a4e3b65f17f..241dd6fde7f 100755 --- a/fdbbackup/tests/dir_backup_test.sh +++ b/fdbbackup/tests/dir_backup_test.sh @@ -143,6 +143,40 @@ function restore { fi } +# fdbbackup query reports failure as a JSON object on stdout, so only the exit status distinguishes it from success. +# $1 build directory +# $2 a url that does not hold a backup +# $3 the url of an existing backup; empty to skip the success check, as query has no way to be given an +# encryption key +function test_query_exit_codes { + local local_build_dir="${1}" + local bad_url="${2}" + local good_url="${3:-}" + + local out + local rc + if [[ -n "${good_url}" ]]; then + set +e + out=$("${local_build_dir}"/bin/fdbbackup query -d "${good_url}" 2>/dev/null) + rc=$? + set -e + if (( rc != 0 )); then + err "fdbbackup query on an existing backup exited ${rc}; output: ${out}" + return 1 + fi + fi + + set +e + out=$("${local_build_dir}"/bin/fdbbackup query -d "${bad_url}" 2>/dev/null) + rc=$? + set -e + if (( rc == 0 )); then + err "fdbbackup query on a missing backup exited 0; output: ${out}" + return 1 + fi + log "fdbbackup query exit codes OK (missing backup exit: ${rc})" +} + # Run a backup to the fs and then a restore. # $1 build directory # $2 the scratch directory @@ -182,6 +216,16 @@ function test_dir_backup_and_restore { backup_name=$(basename "${backup}") test_encryption_mismatches "${local_build_dir}" "${scratch_dir}" "file://${scratch_dir}/backups/${backup_name}" "${TAG}" "${local_encryption_key_file}" + log "Testing fdbbackup query exit codes" + local query_url= + if [[ -z "${local_encryption_key_file}" ]]; then + query_url="file://${scratch_dir}/backups/${backup_name}" + fi + if ! test_query_exit_codes "${local_build_dir}" "file://${scratch_dir}/no_such_backup" "${query_url}"; then + err "Failed fdbbackup query exit code test" + return 1 + fi + log "Restore" if ! restore "${local_build_dir}" "${scratch_dir}" "${local_encryption_key_file}"; then err "Failed restore"