Skip to content

Retry old-epoch backup mutation-log uploads - #13896

Merged
tclinkenbeard-oai merged 6 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/retry-old-epoch-backup-uploads
Oct 11, 2026
Merged

tclinkenbeard-oai merged 6 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/retry-old-epoch-backup-uploads

Conversation

@tclinkenbeard-oai

@tclinkenbeard-oai tclinkenbeard-oai commented Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

An I/O error while writing partitioned backup logs can terminate an old-epoch backup worker. Those failures deliberately do not restart transaction recovery, so the old log generation can remain retained and backup progress can stop indefinitely.

Retry transient file and blob-store request failures for old-epoch mutation-log uploads while retaining the same role and message prefix. Each attempt opens fresh files. Freeze the recipient ranges and encoding boundaries so an ambiguous finish produces byte-identical duplicates, and handle newly observed overlapping backups before discarding messages or advancing durable progress. Preserve current-epoch recovery, cancellation, and displacement behavior.

Add focused regression coverage for file creation, partial append, ambiguous finish, changing recipients, and stopping during an upload or retry.

Validation

  • fdbserver_backupworker_test: 8/8 in native mode and 8/8 in simulation mode after test consolidation.

Earlier validation of the unchanged production logic:

  • BackupCorrectnessPartitioned.toml with buggify and fault injection enabled: passed; backup and restore completed.
  • A matched control with only old-epoch retries disabled reproduces the stalled backup; enabling retries completes the same test.
  • The workload and fault settings are unchanged.

This change covers transient destination I/O failures. It does not add replacement of an already-terminated process or recovery from a permanently hung destination.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@saintstack saintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the local LLM:

The retry predicate likely misses the production destination. It retries only io_error/io_timeout, which originate in the IAsyncFile layer — file:// destinations and simulated disk faults. The blob-store upload path never throws them (BlobStoreCommon.cpp:985-1010 throws http_request_failed, connection_failed, timed_out; the one io_error in S3BlobStore.cpp:1140 is read-path). The PreserveFailurePolicy test deliberately asserts http_request_failed is not retried, so this was a conscious choice with a decent rationale — the blob store retries internally. But for S3-backed backups the stall the PR describes appears to remain, and the unit tests can't reveal that because they inject the codes directly into the mock.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@tclinkenbeard-oai tclinkenbeard-oai left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by Codex.

What problem is this PR trying to solve? (ELI10)

A backup worker copies the database’s change history into backup storage. An old-epoch worker handles history from an earlier database generation. If its upload fails, it can exit while unfinished backups keep waiting. Its failure deliberately does not restart database recovery.

For example, illustratively, a temporary storage outage can exhaust upload retries and stop that worker. Even after storage recovers, backup progress can remain stuck and the old logs stay retained.

How does this PR solve the problem? (ELI10 and review walkthrough)

Keep the old worker alive and retry the same chunk of history using fresh files. If storage accepted a file but its success reply was lost, the retry must preserve the same mutation contents so either copy is safe to restore.

  1. Snapshot the batch: retain the selected message prefix and freeze recipient ranges, versions, and block size. New recipients get another pass before that prefix is discarded.
  2. Retry destination failures: retry only an unstopped old-epoch worker. Current-epoch failures and cancellation still propagate. Each attempt opens fresh files.
  3. Preserve clear-range ordering: writes to each file are chained, retaining their bytes across waits. Frozen boundaries keep retries consistent when another backup stops. Existing restore code filters duplicate log intervals.
  4. Advance progress after saving: only after saving succeeds does the caller discard messages, record progress, and permit old log data to be released.

What is it trying to do?

Recover from transient destination failures during old-epoch partitioned-log uploads without restarting transaction recovery, losing mutations, or changing current-epoch failure handling.

Is it correct?

Yes, based on source inspection at head dbb02ca4eee1fb7af25c0fc7ed1c970320b98f47, against base a003925f03ef78d5270da134e47d2422985cc440.

I inspected error propagation, async ownership and cancellation, recipient changes, duplicate restore handling, progress ordering, hot-path costs, and regression tests. No persisted format or network schema changes were introduced. The tests exercise fresh-file retries, partial writes, ambiguous completion, changing recipients, and stopping/cancellation.

No builds, tests, or simulations were run for this review. Current-head CI has three passing checks and seven pending jobs, with no reported failures.

Are there bugs?

I did not find any correctness bugs.

The earlier review’s missing-blob-errors concern was valid for the older revision and is addressed here: the retry predicate now includes http_request_failed, connection_failed, timed_out, and lookup_failed, matching the blob request failure outputs.

Are there omissions?

None that I think block this. The main remaining coverage gap is the complete worker/progress path during a real blob-storage outage: the added tests exercise helpers, while the existing partitioned-backup workload uses a file destination. Replacing a dead process and interrupting a permanently hung destination remain explicitly outside this change’s scope.

Are there better ways of doing things?

No alternative warrants changing this approach. Per-file append chaining adds bookkeeping but preserves ordering and concurrent writes across different files.

Should this CL be LGTMd?

Yes, LGTM based on code inspection. Retry policy, ownership, duplicate handling, and durable-progress ordering look sound. The highest remaining risk is distributed integration behavior during destination outages; current-head CI is still pending.

@tclinkenbeard-oai
tclinkenbeard-oai marked this pull request as ready for review September 16, 2026 07:03
@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@spraza

spraza commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@saintstack @neethuhaneesha can you have a look?

@saintstack saintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry so late reviewing this)

LGTM. #14240 is related. I think they compliment. It adds the 'replacing a terminated process' you note in here (But not 'destination hangs permanently')

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: 589a147
  • Duration 0:21:10
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: 589a147
  • Duration 0:34:38
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: 589a147
  • Duration 0:46:43
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: 589a147
  • Duration 0:47:51
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: 589a147
  • Duration 0:52:45
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: 589a147
  • Duration 1:39:43
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@tclinkenbeard-oai
tclinkenbeard-oai merged commit 0e419be into apple:main Oct 11, 2026
9 of 10 checks passed
@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: 589a147
  • Duration 1:53:35
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants