fix(transfer): re-delta the phase-2 redo against the retained basis

[oferchen]

Stacked on #7229

Base is perf/receiver-owned-inflight-window (#7229), not master. That PR
converts run_pipeline_loop_decoupled from &'a self to &mut self with an
owned in-flight window and rewrites the same batch loop this change edits, in all
three files touched here. Rebasing that borrow-model change underneath this edit
later would be far more disruptive than stacking now. Merge #7229 first; this PR
retargets to master cleanly afterwards.

The bug

The receiver requested every phase-2 redo index with a null sum head, so the
sender fell into match.c:403-409's literal branch and re-sent the whole file.
The bytes still landed correctly, which is why the existing redo-recovery test
passed throughout - the cost is invisible unless the delta split is asserted.

Measured on a forced --append-verify redo (200 KiB source, 100 KiB wrong-prefix
destination), oc as the pulling client against an upstream 3.4.4 daemon:

	transfers	Literal	Matched
oc before	2	307,200	0
oc after	2	205,300	101,900
upstream 3.4.4	2	205,300	101,900

307,200 is exactly 102,400 (phase-1 append) + 204,800 (the whole file again).

Why upstream re-deltas

The redo re-enters the ordinary recv_generator() for the redo index
(generator.c:2200) - not a reduced retry path. That re-stats the destination and
re-sends a block signature built from it (generator.c:1967). The destination
still holds the phase-1 update: --append implies --inplace
(options.c:2411), and receiver.c:1029 ((recv_ok && ...) || inplace) finishes
the transfer in place even when recv_ok == 0 - which is also why the warning
reads retained (receiver.c:1074-1079). So the retained partial is the
redo's basis.

Only two things differ from phase 1, and both were already applied on this side:
csum_length = SUM_LENGTH (generator.c:2178) and a negated append_mode
(:2186). The latter is load-bearing beyond the signature: without it the redo
would trip the append short-circuit at generator.c:1842
(append_mode > 0 && sx.st.st_size >= F_LENGTH(file)), where the destination now
equals F_LENGTH exactly.

The change

Route the redo batch through the same basis lookup as phase 1. The former
no-basis arm is then unreachable - its only other selector was the empty batch,
whose loop body never ran - so it is deleted rather than left as dead code.

The comment that licensed this bug claimed "the basis comparison is not re-run
for the retry" and cited generator.c:1939, which is refuted by :2200 and
:1967 inside the very function it points at. It is replaced with a citation
that actually holds. A wrong upstream citation is worse than none.

Testing

crates/transfer/src/receiver/basis.rs already asserted "phase-2 redo must send
a checksum-based delta, not a whole-file literal transfer" - but it called
find_basis_file_with_config directly, and the production redo path never called
that function, so it passed for the whole life of the bug. The new test drives a
real daemon pull end to end and asserts the split:

• the redo must match every whole basis block inside the retained tail
  (derived from signature::calculate_signature_layout, not hardcoded);
• it must not count the wrong prefix as matched (upstream's append pass never
  calls matched() for the retained prefix - match.c:389-390);
• literal + matched must account for exactly the phase-1 tail plus the phase-2
  resend, so a redo that skipped bytes cannot satisfy the lower bound alone;
• the file must be transferred twice, or there was no redo to measure.

Verified red before / green after by restoring pipeline.rs to its base revision:
fails with matched 0 < 101500, passes after.

The test payload is index-hashed rather than reusing the sibling's i % 251.
That sequence repeats with period 251, so a source block matches a basis block
whenever the offsets are congruent mod 251 - the first run of this test reported
matched 204,400, matching blocks the retained tail never covered. A guard now
asserts no two basis blocks are equal, so the ceiling cannot go vacuous.

Verification

• cargo fmt --all -- --check clean
• cargo clippy --locked --workspace --all-targets --all-features --no-deps -- -D warnings clean
• cargo nextest run --workspace --all-features -E 'package(transfer) or package(engine)':
  7627/7629 passed. The 2 failures are engine local_copy::tests::{execute_preserves_read_only_permissions, archive_preserves_mixed_permissions_across_files}, both failing with
  remove extended attribute ... Permission denied on a local macOS temp dir.
  Reproduced identically on the untouched base commit f01596f, and this
  change touches only crates/transfer, so they are pre-existing and
  environment-specific.

Scope

This fixes the receiver's redo request only. Two neighbouring divergences the same
fixture exposes are deliberately not in this PR, because each reproduces
independently of the redo:

• The daemon push cell still reports 205,700 / 101,500 - 400 bytes out. That
  is oc's sender never matching the basis's trailing short block, and it
  reproduces with no --append, no --append-verify and no phase 2 at all
  (plain --no-whole-file push against the same basis: 103,300 / 101,500 vs
  upstream's 102,900 / 101,900). Separate PR against crates/matching.
• The local cell still reports 1 transfer / 204,800 / 0, because the local-copy
  executor has no phase-2 model at all and implements --append-verify as an
  a-priori prefix comparison. Separate work in crates/engine.

[oferchen]

Rebased onto master (d54f6e5) after #7229 and #7236 landed.

Conflict resolution. #7236 did not just touch tests/daemon_pull_verify_redo_recovers.rs - it deleted it and replaced it with the broader tests/verify_redo_recovers_network_paths.rs, so git reported a modify/delete rather than a text conflict. I took master's deletion and re-homed my delta-split test into the new file, reusing its harness (DaemonScratch, spawn_oc_daemon, run_oc_rsync_deadlined, assert_recovered). The three source files auto-merged; git diff origin/master on them is exactly my change and nothing else, so #7236's emit-site fix is untouched.

Both concerns verified together, re-measured on the rebased binary (daemon pull, oc client against an upstream 3.4.4 daemon):

Number of regular files transferred: 2
Literal data: 205,300 bytes      <- was 307,200
Matched data: 101,900 bytes      <- was 0
stderr: WARNING: payload.bin failed verification -- update retained (will try again).
stdout occurrences of the warning: 0
destination: identical to source

Byte-identical to upstream 3.4.4 on the same fixture, and the warning is now on stderr with the bare flist-relative name - #7236's behaviour, which my re-homed test asserts through the shared assert_recovered rather than re-stating.

On the payload: my test seeds distinct_block_payload() instead of the file's shared authoritative_payload(). i % 251 repeats with period 251, so a source block matches a basis block whenever the offsets are congruent mod 251 - it measured 204,400 matched bytes on a first run, which makes any bound on which bytes were matched meaningless. A guard asserts no two basis blocks are equal, so the ceiling cannot go vacuous.

package(transfer) or package(engine): 7649/7651 pass. The 2 failures are the pre-existing macOS-local engine local_copy::tests::{execute_preserves_read_only_permissions, archive_preserves_mixed_permissions_across_files} (remove extended attribute ... Permission denied), reproduced on an untouched base.

One open item I am still chasing rather than asserting: rsh_pull_forced_verification_failure_recovers_via_redo (a #7236 cell, not mine) hits its 60s deadline when I run it alone on this host, while passing in the full parallel run. Reproducing the same cell by hand with the same binaries and flags succeeds in under a second (rc=0, 2 transfers, destination identical, warning on stderr), so it is not the transport. I am running the same single test on untouched master to confirm it is pre-existing; this host is currently at load ~40 from parallel builds, which is a plausible cause on its own. Will report the verdict.

[oferchen]

Follow-up to the rebase, all re-run rather than re-read.

1. Red-check after the rebase. Reverting only pipeline.rs to its base revision (test file untouched) makes the re-homed test fail with exactly the diagnosed message:

FAIL transfer::verify_redo_recovers_network_paths daemon_pull_redo_redeltas_against_the_retained_basis
  the phase-2 redo did not re-delta against the retained basis: matched 0 < 101500
  (the 145 whole basis blocks inside the retained tail). A null sum head on the
  redo request reports matched 0.

Restoring the change: 1 passed. So the test still bites post-rebase.

2. Daemon pull, re-measured on the rebased binary - 2 transfers, literal 205,300 / matched 101,900, destination identical, warning on stderr with the bare name and zero occurrences on stdout. Byte-identical to upstream 3.4.4.

3. The rsh cell I flagged is NOT a regression - it was my host. rsh_pull_forced_verification_failure_recovers_via_redo hits its 60s wall-clock deadline only when this machine is saturated by other builds:

condition	result
fix applied, load ~40	FAIL at 60.03s (2/2)
fix applied, load ~21	PASS 2.1-2.6s (4/4)
fix reverted, load ~40	PASS
fix applied, by hand (same binaries, flags, transport)	PASS (4/4, even under load)

In the same loaded runs my own daemon-pull test took 30.8s where it now takes 2.1s - a ~15x slowdown, which puts a normally-2s cell within reach of a 60s deadline. I chased this as a possible pipe deadlock from the redo now carrying ~5.8 KB of block sums instead of a null sum head; the manual runs over the identical lsh-stub pipe transport rule that out.

Worth recording separately: run_oc_rsync_deadlined polls try_wait() without draining the child's piped stdout/stderr, and only calls wait_with_output() after exit. A child that writes more than the pipe buffer would block forever, and the deadline would not fire because it is only checked in the try_wait loop. This fixture's output is far too small to reach it, so it is latent, not the cause here - but it is a real hang waiting for a noisier cell.

4. fmt. CI's cargo fmt --check caught two lines in my test that my local cargo fmt --all -- --check reported clean - the known false-clean invocation on this host. Fixed and re-verified by calling the toolchain's rustfmt --check on the files directly rather than trusting the cargo wrapper. Pushed as an amend.