[mpatou-openai]

Partitioned restore currently queues every snapshot block in a version window in one dispatch transaction. Large windows can exceed the transaction size limit. Writer recovery also needs to reset transactions after size rejection, and partitioned log writers must retain a reduced write budget across subsequent batches.

This change:

• Bounds snapshot dispatch by task count and a new RESTORE_DISPATCH_TX_SIZE byte target (4 MiB by default), carrying a file/block cursor across continuation tasks.
• Keeps continuations on the same completion future so the next version window waits for all snapshot blocks and log work. Dispatch commits task creation, cursor advancement, and retirement together, handles unknown commit results, and retries size rejection with a smaller task limit. Reduced dispatch limits carry through continuations and requeues within the same version window; each new window starts with the configured task limit again.
• Resets range and legacy log writer transactions after size rejection, resets partitioned log transactions between commits, and preserves reduced partitioned writer budgets across batches.
• Loads log metadata only after snapshot dispatch finishes.
• Adds dispatch fault injection and a RestoreWriterRecovery simulation workload that gives the actual writers a 4 KiB transaction limit with a 16 KiB initial budget. It checks rejection, shrinking, multiple successful commits, exact output, and reduced-budget retention across partitioned batches. Task acquisition retries empty results under the test timeout, allowing recovery when an unknown commit result leaves the task leased. Verification and idempotent cleanup use separate transactions.

Validation: both changed C++ translation units compile with -Werror; symbol inspection and a relocatable object-link check confirm the workload resolves fileBackup::testRestoreWriterRecovery. CMake registers the new simulation test, the TOML parses, and git diff --check passes. The branch merges cleanly with the current upstream main.

Full fdbserver linking and simulation execution have not been run.

[tclinkenbeard-oai]

Generated by Codex.

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

Restoring a backup creates jobs for copying saved data back into the database. The old dispatcher can try to create too many jobs in one transaction.

For example, imagine a backup file with enough blocks that its job list exceeds the transaction limit. The database rejects the list, preventing restore progress. This is an illustrative scenario grounded in the previous unbounded dispatch loop. The data writers also need to discard failed transactions before retrying smaller writes.

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

It creates smaller groups of jobs and saves a bookmark identifying where to continue. The next restore stage still waits until every group finishes.

1. Claim and retry safely. dispatchWithRetry checks whether the task already finished, verifies ownership, and commits dispatch together with task retirement. This prevents duplicate dispatch after a successful commit whose reply was lost.
2. Stop at a bounded chunk. The block-dispatch loop limits task count and estimated transaction bytes. Its bookmark preserves the next block or advances beyond the completed file.
3. Preserve the completion dependency. Continuation tasks join the same batch. The next version window waits on that batch, and the successor dependency is installed before the current one is released.
4. Rebuild smaller writes. Writers reset rejected transactions. The partitioned writer also resets after successful commits and returns its reduced budget, which subsequent batches retain.

What is it trying to do?

Make partitioned restore dispatch bounded and resumable, repair oversized-write recovery, and add behavioral regression coverage.

Is it correct?

The production changes look sound on inspection, but the new test has a reachable false assertion described below.

I inspected cursor ordering, completion dependencies, retry atomicity, transaction resets, cancellation, ownership, and budget propagation. The persisted task changes appropriately bump the task version to 2, while older tasks receive default cursor behavior.

Reviewed head 3613fdc6769e631d1b5c8fd4d2f0b09b43f91f13 against base 7a4ca845c8cb78e15dc6c6d8e037144e40488c26; both were rechecked. No builds, tests, or simulations were run. GitHub reports no checks for this head.

Are there bugs?

[P2] Retry empty task acquisition in the new recovery test.

The single getOne(cx) call followed by ASSERT(task) assumes acquisition always returns a task. However, acquisition moves the task into a timeout lease. If that transaction commits but its reply is lost, the internal retry finds no available task and can return empty before the lease expires.

This can happen under the new test’s configuration: disabling failure-injection workloads only suppresses those workload factories; the tester still enables ordinary connection failures. The assertion can therefore fail before any writer runs.

Retry empty acquisitions with a delay so timeout reclamation can recover the task, or make this test’s acquisition explicitly idempotent.

Are there omissions?

No additional blocking omissions found. The writer test checks actual size rejection, multiple commits, exact output, and retained budgets. The highest remaining uncertainty is runtime behavior around dispatcher retries and task timeouts; the end-to-end coverage remains stochastic and was not executed for this review.

Are there better ways of doing things?

Keep the behavioral test and fix its acquisition assumption. No broader redesign appears necessary.

Should this CL be LGTMd?

Not yet. Fix the test’s empty-acquisition handling. I found no separate correctness bug in the production changes.

[saintstack]

Looks good. Question below.

Local LLM turned up the Trevor priority 2 getOne issue. Here is another:

One transaction_too_large halves dispatchTaskLimit. The halved value is saved on the task and inherited by every later batch, so it never resets. Slows dispatch only, doesn't break correctness. Hard to trigger outside fault injection, since the 1 MiB target is far below the transaction limit.

Simplifying plugin offered these suggestions if you are interested:

1. Redundant clamp when halving a limit: at FileBackupAgent.cpp:4927, :5299, :5617 and :6294 the line x = std::max(1, x / 2) sits inside a limit > 1 guard. Any integer of 2 or more halves to at least 1, so the clamp never fires and x /= 2 does the same thing.
2. Manual increment before break in the dispatch loop (:6115-6133): the code adds j += f.blockSize by hand and then breaks, with a comment explaining why. A txFull flag in the loop condition gives the same final j and lets that comment go.
3. Nested ternary for the test's task name (:5881-5883): the test already branches writer == 0/1/else when it creates the file around :5831. Setting the task name in that same branch removes the ternary.

[saintstack]

Is this too conservative?

[mpatou-openai]

I think I addressed the issue @saintstack can you have a second look and @tclinkenbeard-oai too ?

[tclinkenbeard-oai]

Generated by Codex.

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

Restoring a backup creates jobs to copy saved data back into the database. The dispatcher currently tries to put every snapshot-block job for a version window into one transaction—a group of changes that must succeed together.

Illustratively, a backup with many blocks can produce a job list larger than the database accepts, preventing restore progress. The previous dispatch loop has no effective task limit. The writers also need to rebuild rejected transactions before retrying smaller writes.

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

It creates smaller groups of jobs and saves a bookmark for the next group. The next restore stage still waits for all the work in the current window.

1. Claim and commit dispatch safely: check task ownership, create successors, and retire the current task in one transaction. Checking whether it already finished handles a successful commit whose reply was lost.
2. Bound the snapshot jobs and advance the bookmark: stop at the task or byte limit, retaining the next block or advancing past a completed file. The per-job wait here also introduces the latency issue below.
3. Keep continuations in the same completion group. The next window waits on that group, and the replacement dependency is installed before releasing the current one.
4. Reset and shrink writer transactions. The partitioned writer returns its reduced budget, which subsequent batches retain.

What is it trying to do?

Make partitioned restore dispatch bounded and resumable, repair oversized-write recovery, and add regression coverage using the actual writers.

Is it correct?

The cursor, completion dependencies, unknown-commit handling, writer resets, and budget propagation look sound. Persisted cursor tasks appropriately use task version 2; old tasks receive default cursor behavior. I found one conditional dispatch liveness regression below.

The earlier empty-task acquisition bug is fixed by retrying. The reviewer's concern about permanently inherited dispatch reductions is also resolved for new version windows.

Reviewed head c8c33bd4717987792fedbd4cf595ba3ab8e5e49f against base 7a4ca845c8cb78e15dc6c6d8e037144e40488c26; both were rechecked. No builds, tests, or simulations were run. GitHub exposes no check runs or status contexts for this head.

Are there bugs?

[P2] Reduce dispatch work after transaction-age failures.

The new per-task await serializes task creation. Each task creates a fresh completion future whose callback registration reads its block range. The locally written blocker does not cover the uncached gap before it, so these calls can require separate database round trips.

For an illustrative window of 150 small tasks, with the byte cutoff not reached and sustained 40 ms reads, those serial reads require about six seconds—longer than the normal five-second read-transaction lifetime. The transaction can expire before dispatch commits. However, only transaction_too_large reduces the task limit; transaction_too_old retries the same limit and cursor, allowing restore to stall while that latency persists. Previously these task-creation futures ran concurrently.

Reduce the task limit on transaction-age failures, or commit a partial chunk after an elapsed-time budget. This is a conditional failure derived from code inspection, not a reproduced incident.

Are there omissions?

The writer test meaningfully checks rejection, multiple commits, exact output, and retained budgets. The dispatch simulation uses two tasks per transaction, so it does not cover the latency accumulation described above. Extend dispatch coverage to require cursor progress after an age failure with a larger initial task limit.

Are there better ways of doing things?

Handling transaction-age failures in the existing adaptive retry loop is a narrow fix. A broader concurrency redesign is unnecessary for this finding.

Should this CL be LGTMd?

Not yet. Address the conditional dispatch stall. The earlier acquisition issue is fixed, and I found no separate correctness bug in the writer recovery or continuation bookkeeping.