Thanks for the follow-up — the diff is much tighter than #224 (4 files, no scope creep) and the symptom is real. But I think the proposed shape (a parallel endgame path with redundant requests) is the wrong tool for what aMule already has, and the implementation underneath has issues independent of that. My recommendation is to redirect this PR toward improving the existing mechanism rather than adding a new one.

aMule already has the right shape — it's just narrowly triggered

The "stuck at 99% on a slow source" case has a working remedy today, in DownloadClient.cpp:646-700 and PartFile.cpp:4577. The wiring:

• When a remote peer (one that has the file we want) becomes ready to feed us data — either via OP_ACCEPTUPLOADREQ (ClientTCPSocket.cpp:581) or right after we successfully received a block from them (DownloadClient.cpp:1052) — we call SendBlockRequests on that peer to decide what to ask them for next.
• If we find that all remaining unfinished blocks of the file are already assigned to other peers, GetSlowerDownloadingClient scans the file's currently-downloading peers; if any is at least 2× slower (DROP_FACTOR = 2) than the peer that just became ready, we send OP_CANCELTRANSFER to the slow one, free its blocks, and reassign to the newly-ready fast peer.

That's the correct strategy: rotate work away from a slow source onto a fast one when one becomes available, with explicit cancellation, zero bandwidth waste. Same end result your endgame mode is after, achieved without redundant requests.

Failure modes that match the "stuck at 99%" symptom:

• thePrefs::GetDropSlowSources() is off → rotation disabled entirely.
• No new peer ever accepts us to upload, so SendBlockRequests is never re-invoked → no rotation, slow source plods on.
• A new peer accepts us but isn't ≥2× faster than the slow incumbent → GetSlowerDownloadingClient returns NULL.
• Faster known peers stay in their queue on our side (DS_ONQUEUE) and never transition to DS_DOWNLOADING → no trigger.

Each of those is a small, well-scoped tweak — relax DROP_FACTOR near completion, remove the pref gate when remaining < N parts, add a periodic stale-assignment scan, special-case "one chunk left + single source" to force a rotation when any new source for the file becomes known. Any one of those is a much safer change than a new redundant-request path.

Why the current PR shape is also problematic on its own merits

Even if we ended up wanting endgame-style redundant requests, this implementation has three issues that would block it:

1. The bandwidth-overhead claim isn't backed by the code. The PR says: "the core automatically cancels the remaining redundant transfers via OP_CANCELTRANSFER." In reality, CPartFile::WriteToBuffer silently discards duplicate data and logs a debug line — no cancellation is sent. OP_CANCELTRANSFER is only emitted at full-block boundaries from the slow-source-rotation path above, not when a duplicate block arrives mid-flight. So every redundant request your PR allows will download to completion (or until the remote loses interest) before the duplicate is discarded in memory. "Negligible and strictly controlled overhead" doesn't hold without an additional cancellation path that this PR doesn't add.

2. Concurrency race on unlocked lists. HasRequestedBlock iterates m_PendingBlocks_list and m_DownloadBlocks_list, but those have no mutex. Their siblings m_BlockRequests_queue and m_DoneBlocks_list are explicitly guarded by m_blockListLock (updownclient.h:718) precisely because they're touched from socket handlers. Mutations happen in SendBlockRequests and ProcessBlockPacket on the network-io path; your new read site reaches them from GetNextRequestedBlock via GetNextEmptyBlockInPart. Iterating concurrently is list-corruption territory.

3. Small-file bug. ENDGAME_TRIGGER_SIZE = 37 MB is compared against (GetFileSize() - completedsize). For a 30 MB file, that's true from byte 0 — the entire download runs in endgame, with every source racing every block, relying solely on (currently-absent) cancellation as the backstop. Also: 37 MB ≈ 4 × PARTSIZE; express it as N * PARTSIZE with a comment so the choice is reviewable.

Correction / additional context to my earlier comment: I undersold how narrow that existing rotation path actually is in practice. thePrefs::GetDropSlowSources() defaults to false (Preferences.cpp:1398), and there's no GUI control, no amulecmd command, no webserver template knob, no EC tag, and no setter anywhere in the tree — the only way to turn it on is to hand-edit amule.conf, set DropSlowSources=1 under [eMule], and restart. For practically every aMule user out of the box, the GetSlowerDownloadingClient path is dead code.

That makes your point about the "stuck at 99%" symptom more valid than my earlier comment gave it credit for — and it also opens a much smaller, safer fix than either your endgame design or the per-knob tweaks I listed.

Concrete suggestion for a follow-up PR — make DropSlowSources dynamic / auto near completion, instead of (or in addition to) a manual preference:

• Compute on every SendBlockRequests whether the file is in the "near completion" window — e.g. (remaining_parts ≤ 4) && (total_parts > 4). The total_parts > 4 half is the small-file guard: a 30 MB file with only ~3 parts total never auto-enables, so you don't get full-download endgame on small files. The remaining ≤ 4 half scopes activation to the genuine tail.
• When that condition holds, treat GetDropSlowSources() as true regardless of the stored preference. Leave the user-set preference alone for the "always-on rotation" power-user case, but don't require it.

Concrete site to put the gate: DownloadClient.cpp:650. Today it reads if (thePrefs::GetDropSlowSources()). With the dynamic shape it'd be something like:

bool nearCompletion =
    m_reqfile && m_reqfile->GetPartCount() > 4 &&
    m_reqfile->GetIncompletePartCount() <= 4;
if (thePrefs::GetDropSlowSources() || nearCompletion) {
    slower_client = m_reqfile->GetSlowerDownloadingClient(m_lastaverage, this);
}

(GetIncompletePartCount or equivalent — pick whatever the partfile already exposes; m_gaplist-derived bytes / PARTSIZE works too.)

That gives you the symptom fix you're after, with no concurrency rework, no redundant requests, no bandwidth-claim wiring, and a clean small-file guard. ~3 lines of substantive change plus a helper.

@Cflsft much better — the diff on the latest commit (3a4d279) is exactly what was needed: 7 lines, single file, no concurrency rework, no redundant requests, no magic constant. All three of the previous blockers (bandwidth-claim wiring, unlocked-list race, small-file bug) are gone. The dynamic gate at DownloadClient.cpp:647-655 reads cleanly, the GetPartCount() > 4 small-file guard is correctly placed, and the defensive underflow check (filesize > completedsize before the subtraction) is a nice touch.

That said — this isn't mergeable on inspection alone. The reason: GetSlowerDownloadingClient + the OP_CANCELTRANSFER/ClearDownloadBlockRequests/DS_NONEEDEDPARTS/GetNextRequestedBlock rotation loop has been effectively dead code for years (pref defaults off, no UI, no EC tag). Your change activates that path for every aMule user on every download as soon as it enters the last ≥4 parts. Even though the change itself is correct, it's a substantial behavioral expansion of a code path that has had minimal real-world exercise, and any latent bug in the rotation logic — UAF on the displaced client, state-machine mismatch when the cancelled source is concurrently sending a block, double-cancellation, or simply wrong block reassignment leading to corrupted writes — would now hit everyone, not just power users.

So before merging we need a test cycle from you that demonstrates:

1. Rotation actually fires in your stall scenario. Build with -DCMAKE_BUILD_TYPE=Debug — the debug log lines are #ifdef __DEBUG__-gated, so Release builds compile them out entirely. Then either tick the boxes in Preferences → Logging (Debug build has the tab) or set the following in amule.conf directly:

   [eMule]
   VerboseDebug=1
   VerboseDebugLogfile=1
   
   [Debug]
   Cat_Local Client Protocol=1
   Cat_Remote Client Protocol=1
   Cat_PartFiles=1
   

   VerboseDebugLogfile=1 routes verbose output to ~/.aMule/logfile so you can copy it back as text. Local Client Protocol (logLocalClient) is the essential one — that's where the rotation log line fires (Local Client: OP_CANCELTRANSFER (faster source eager to transfer), DownloadClient.cpp:673). Remote Client Protocol and PartFiles add context on the displaced peer and on block reassignment.

   Then run a real download you've previously seen stall at 99% on this branch, and capture the rotation log line firing inside the endgame window plus the download finishing in a reasonable time.

2. The completed file is correct. Verify the file finishes without Error or AICH-failure lines in the log, and (ideally) sha256 the result against a known-good copy.

3. No crashes / UAFs. Run several end-to-end downloads on the branch (mix of sizes — one ≤4 parts so the small-file guard is exercised, one ~10 parts, one ~100 parts) and confirm no crashes, no wxASSERT failures.

4. Spot-check the "all sources equally slow" case. When the rotation finds no slower client, the code at DownloadClient.cpp:655 sets slower_client = this and effectively drops the asking client itself. With the verbose log above, you should see Local Client: OP_CANCELTRANSFER (no free blocks) instead, and the asker going to DS_NONEEDEDPARTS — please confirm that still behaves sanely now that this branch can fire from nearCompletion=true even with the user pref off.

Paste the relevant log excerpts as text in code fences (not screenshots), along with the aMule rev + platform you tested on. If everything checks out and we don't see latent issues surface, this is ready to land.

@Cflsft this is a real improvement on 3a4d279 — three distinct fixes, all targeted at genuine bugs:

1. Iterator UAF in CPartFile::Process (PartFile.cpp:1515-1530) — snapshotting m_downloadingSourcesList and m_SrcList into a temp vector before iterating is the right shape. TickDownloadAndMeasure can re-enter the watcher/state-machine and trigger a removal mid-iteration; the snapshot keeps the elements alive (verified CClientRef ref-counts via Link() at ClientRef.h:79-81) and the if (cur_src && …) null guard handles freshly-detached refs. Minor cost: O(N) copy per timer tick on the hot path; acceptable.
2. HasUsefulBlocksFor guard in GetSlowerDownloadingClient (PartFile.cpp:4581-4585) — prevents the wasteful displacement-with-no-payoff case (slow source A holds blocks in parts B doesn't have; cancelling A and re-asking for blocks B can't claim is pure churn). Reasoning checks out, no concurrency hazard since m_DownloadBlocks_list / m_PendingBlocks_list are single-thread (event loop only).
3. Replacing wxFAIL_MSG with a graceful drop — correct in intent; the assert was based on the assumption that "we just freed blocks, so we'll find some" which the new guard at (2) shows is sometimes false.

That said, the fix-3 branch is incomplete and slightly leaky:

} else {
    AddDebugLogLineN(logLocalClient,
        "Local Client: OP_CANCELTRANSFER (freed blocks not available here) to " + GetFullIP());
    slower_client = this;
    slower_client->SetDownloadState(DS_NONEEDEDPARTS);
    return;
}

The log line says OP_CANCELTRANSFER, but no cancellation packet is actually sent here. The cancellation that happened in this scope was the one above for slower_client = A (the slow peer we picked). this (the asker) is going to DS_NONEEDEDPARTS without notifying its own remote, which means that remote will keep pumping bytes at us until something else tears the connection down. Compare with the symmetric branch a few lines below at DownloadClient.cpp:695 (slower_client == this, "no free blocks"): there the CANCEL is actually sent via the unconditional block at the top of the section, because slower_client == this causes the cancel-to-self path to fire.

Suggested shape — actually send the cancel so the log matches reality:

} else {
    if (!GetSentCancelTransfer()) {
        CPacket* packet = new CPacket(OP_CANCELTRANSFER, 0, OP_EDONKEYPROT);
        theStats::AddUpOverheadFileRequest(packet->GetPacketSize());
        ClearDownloadBlockRequests();
        SendPacket(packet, true, true);
        SetSentCancelTransfer(1);
    }
    AddDebugLogLineN(logLocalClient,
        "Local Client: OP_CANCELTRANSFER (freed blocks not available here) to " + GetFullIP());
    SetDownloadState(DS_NONEEDEDPARTS);
    return;
}

Two minor nits while you're in there:

• slower_client = this; slower_client->SetDownloadState(DS_NONEEDEDPARTS); is an indirect way to write SetDownloadState(DS_NONEEDEDPARTS);. The reassignment isn't used.
• HasUsefulBlocksFor walks m_DownloadBlocks_list and m_PendingBlocks_list looking for any block whose containing part is available on other. Correct, but worth a one-line comment that we're checking part-level availability, not block-level — block-level fits implicitly because a block can't straddle a part boundary in ed2k (180 KB block << ~9.28 MB part).

Testing still required before merge. The previous review's test cycle still applies and matters more now that the diff activates the rotation path widely. Per my earlier comment, please run a Debug build with [eMule] VerboseDebug=1 / [Debug] Cat_Local Client Protocol=1 / Cat_Remote Client Protocol=1 / Cat_PartFiles=1 and capture: a real 99% stall completing via the rotation, an AICH-clean completion, a few mixed-size end-to-end downloads with no crashes, and the "no useful blocks anywhere" path firing the new branch at least once.

Thanks for the review and the excellent suggestions!

When testing the new nearCompletion trigger on a real download, the code activated the rotation path much more aggressively. This stress-test exposed two edge cases in the DropSlowSources logic, which we have now addressed in this PR:

1. Iterator Invalidation (UAF) in CPartFile::Process

The Issue: A Segfault occurred because calling TickDownloadAndMeasure() could trigger the rotation logic, which synchronously removes clients from m_downloadingSourcesList while the loop was still iterating over it.
The Fix: As you noted, we snapshotted the lists into a temporary std::vector before iterating, keeping the elements alive and preventing the crash.
2. Assertion Failure in DownloadClient.cpp (SendBlockRequests)

The Issue: The code could hit the wxFAIL_MSG("No free blocks to request after freeing some blocks"). This happened because forcing a slower client to free its blocks does not guarantee those blocks can be assigned to the faster client, specifically if the faster client does not have the Part where those freed blocks reside.
The Fix: We introduced HasUsefulBlocksFor() in GetSlowerDownloadingClient to ensure we only displace a slow client if it holds blocks we can actually request. We also replaced the fatal assertion with a graceful drop if block reassignment cannot proceed.
I have applied your latest suggestions to the PR:

Added the actual OP_CANCELTRANSFER packet dispatch in the graceful drop fallback.
Cleaned up the SetDownloadState call.
Added the comment clarifying the part-level vs block-level check in HasUsefulBlocksFor().
I have run the requested test cycle (VerboseDebug enabled, local/remote/PartFiles logs on) and verified everything is solid. The endgame stall recovery works perfectly without crashes.

@Cflsft — the latest changes all landed correctly: the graceful-drop now actually dispatches OP_CANCELTRANSFER (so the log line matches reality), the SetDownloadState cleanup is in, and the part-level comment on HasUsefulBlocksFor is there. The drop guard itself checks out on inspection — a source is displaced only when the just-ready peer is >2× faster (DROP_FACTOR) and HasUsefulBlocksFor confirms it holds the part, so we never free blocks the asker can't claim.

Three things before this can land:

1. The test cycle needs to be shown, across several different runs. Your comment says it was verified, but no logs are attached — and since this activates a rotation path that's been effectively dead for years, "works perfectly" on a single download isn't enough. Please paste, as text in code fences, the Local Client Protocol excerpts from a few distinct runs: a real 99% stall recovering via rotation, an AICH-clean completion, and a mix of sizes/source counts — including one ≤4 parts (small-file guard) and an "all sources equally slow" case where the asker self-drops to DS_NONEEDEDPARTS. Because the speed test is heuristic (m_lastaverage last-block vs GetKBpsDown()×2 smoothed), the logs should also show it isn't churning sources by mis-flagging a transiently-fast peer.

2. Drop the leftover comments. The graceful-drop branch still carries the commented-out original // // WTF… / wxFAIL_MSG… block — please remove it; the new code speaks for itself.

3. This turns a years-dormant code path on for the entire user base — that's the heart of our caution, and it can't be overstated. Today GetSlowerDownloadingClient and its cancel/reassign loop are effectively dead code: DropSlowSources defaults to false, there's no GUI, amulecmd, EC, or webserver control for it, and the only way to enable it is hand-editing amule.conf and restarting. In practice essentially nobody runs it. This PR doesn't just tweak that path — it activates it automatically for every aMule user, on every download, the moment it enters the last ≥4 parts. So any latent defect in that dormant logic — a UAF on the displaced client, a state-machine mismatch when the cancelled source is mid-block, a double-cancellation, or a wrong reassignment that corrupts a write — goes from "affects the handful of people who hand-edited a pref" to "affects everyone, by default, right at the end of every download," which is exactly when a corruption or crash is most costly. That blast radius is why code inspection alone isn't enough here, and why we won't ship it on-by-default in the 3.0.x bugfix line. Once the evidence above is clean, this should target the next feature cycle (3.1.x), or stay gated/off in release builds until it's had real-world soak across platforms.

Although I have already successfully downloaded quite a few files of various types and sizes with this patch, I completely agree with your caution regarding the blast radius of activating this globally.

To properly address the concerns about heuristic and edge cases, I am going to test for the next few days to gather logs across a variety of downloads (different sizes, ≤4 parts, "all sources slow", etc.).

Thanks for the meticulous review!

Great, thank you for the contribution. 👍

I have been extensively testing the latest build on Linux running as a daemon (amuled), as well as testing the GUI version on Windows 11. I have downloaded dozens of very large files simultaneously across these environments to observe the behavior of the endgame block requests and the overall network stability.

Test Results & Stability

Based on the collected logs (with Local Client Protocol enabled), I checked that there are:

• Zero unexpected connection drops or exceptions.
• No network stalls or bottlenecks at the 99% mark.

I have gathered 6 test cases that cover different scenarios. For ease of review, I am providing both the complete logs and filtered extracts highlighting the most notable events:

• Cases 1 & 2: High concurrency and small file completions.
• Cases 3 & 5: Simultaneous multiple drops (up to 16 concurrent drops managed gracefully).
• Case 4: AICH corruption recovery under endgame conditions.
• Case 6: Healthy completion scenario where files finish smoothly and OP_CANCELTRANSFER is only sent cordially to release unused queue slots, without triggering forced source dropping.

Overall, the logic manages the block queues correctly without choking, stalling, or causing network starvation.

Moving Forward: Opt-in Endgame Option

Even though the baseline behavior is very stable and files are completing naturally, I understand the concerns about making aggressive changes to the default endgame behavior (OP_CANCELTRANSFER on slow sources).

To ensure safety and allow for wider community testing, I propose adding a hidden configuration option to amule.conf:

• EnableEndGame = 0 (Disabled by default)

By default, this will remain off, so the standard aMule behavior remains completely untouched for regular users. Advanced users or testers who experience the "99% stall" issue can opt-in to enable the Endgame rotation logic.

Addressing the "Aggressive Drop" Flaw: An Experimental Approach

While I haven't observed this flaw trigger negatively in the recent stress tests, there is a known theoretical issue with the traditional Endgame approach of dropping slow sources:

When we send an OP_CANCELTRANSFER to a slow source, we aggressively sever the connection. This destroys any in-flight data the slow source had already downloaded for us, wasting bandwidth. Furthermore, if the fast source we reassign the block to unexpectedly disconnects, we lose the block entirely and have to start over.

To address this without destroying connections, we could develop a smarter, non-destructive rotation strategy. The proposed logic would work as follows:

1. Partial Reallocation: Instead of aggressively clearing all block requests from the slow source, we only "steal" a limited amount (e.g., up to 2 pending blocks) to feed the fast source.
2. Preserving In-Flight Data: This allows the slow source to maintain its connection and finish downloading its current block, so we don't waste the bandwidth already spent on it.
3. The "Useless" Threshold: I must distinguish between a "useful" slow source and a completely "useless" one. If the slow source's speed drops below a fixed minimum threshold (I believe 10 KB/s is a good baseline for modern connections), it is deemed "useless". Only in this extreme case is the classic destructive drop (OP_CANCELTRANSFER) applied, because the source is too slow to even finish its current block in a reasonable time.

Do you think it would be worth implementing and testing this new rotation method in the future?

Endgame rotation: graceful block eviction to prevent part corruption

Context

While testing the endgame client rotation on high-concurrency downloads, I identified
a data corruption vector that can cause part hash failures and unnecessary re-downloads
after endgame activity.

The bug

When the endgame logic evicts a slow client to make room for a faster one, the current
code calls ClearDownloadBlockRequests() immediately on the slow client:

// DownloadClient.cpp – current eviction sequence
slower_client->ClearDownloadBlockRequests();  // frees ALL pending blocks
slower_client->SendPacket(OP_CANCELTRANSFER);
slower_client->SetDownloadState(DS_NONEEDEDPARTS);

ClearDownloadBlockRequests() correctly returns unstarted blocks to the gap list
via RemoveBlockFromList(). However, it has no way to undo bytes that were already
written to disk:

Timeline:
  1. Slow client writes bytes [0 – 90 KB]   → FillGap(0, 90KB) ← permanent on disk
  2. Endgame fires, slow client is evicted
  3. ClearDownloadBlockRequests() frees [90 – 180KB] back to pool ← OK
  4. Fast client downloads [90 – 180KB]     ← clean data
  5. HashSinglePart() → FAIL
     ↑ bytes [0–90KB] from the evicted slow client may be corrupt/incomplete
  6. AICH recovery + re-download of the failed blocks

The evicted client's partial write is "sealed" into the gap list with no re-verification
path until the full 9.28 MB part hash runs. A single eviction mid-block can cause the
entire 9.28 MB part to re-download.

Two corruption scenarios

It is important to distinguish two different situations that can arise when multiple
clients contribute to the same 9.28 MB part during endgame rotation:

Scenario 1 — Complete blocks, two clients (no inherent bug)

Slow client:  [block 1: 0–180 KB]  [block 2: 180–360 KB]   ← writes complete blocks
Fast client:  [block 3: 360–540 KB] … [block 52: …–9.28 MB] ← writes complete blocks

→ Part hash covers all blocks together.
→ If any client sent corrupt data for their block(s), the part hash fails.
→ AICH can pinpoint exactly which 180 KB block failed and from whom.
→ Only that 180 KB block needs re-downloading. Attribution is clean.

This is normal network-level corruption, not an endgame bug. It is handled correctly
by the existing CorruptionBlackBox + AICH recovery pipeline.

Scenario 2 — Split block, two clients (the actual bug)

Slow client:  writes [0–90 KB] of block 1   → FillGap(0, 90KB) — permanent on disk
  ↓ evicted mid-block
Fast client:  writes [90–180 KB] of block 1 ← clean data from a different source

→ Block 1's 180 KB comes from two different clients.
→ If the slow client's 90 KB was corrupt or incomplete, block 1's AICH hash fails.
→ CorruptionBlackBox sees BOTH clients wrote into that range — attribution is ambiguous.
→ The entire block (and potentially the full 9.28 MB part) must be re-downloaded.

This is the bug introduced by the current hard eviction. ClearDownloadBlockRequests()
correctly frees the unstarted blocks back to the gap list, but it cannot undo
bytes already written to disk via FillGap(). That partial write becomes invisible
to the hash system until the part-level MD4 or the final file hash runs.

A secondary effect: endgame rotation statistically increases the number of distinct
clients that contribute to each part. Even in Scenario 1 (complete blocks), more
clients per part means higher exposure to any single bad actor. Graceful eviction does
not solve this secondary effect, but it eliminates the Scenario 2 window entirely.

Proposed fix: graceful eviction

Instead of canceling all blocks at once, I propose canceling all pending blocks
except the first (the one currently being received). The freed blocks go back to
the pool immediately for the fast client to pick up, while the slow client is given
time to complete its current block cleanly.

Graceful eviction:
  1. Slow client has pending blocks: [A (in flight), B, C]
  2. ClearDownloadBlockRequests(bKeepFirst=true):
       → B, C freed to pool   ← fast client picks these up
       → A kept               ← slow client finishes it
  3. Slow client completes block A → data verified by AICH block hash
  4. Slow client requests more blocks → none available → natural disconnect

This eliminates the partial-write window entirely. The slow client's data is always
complete before it leaves, so the CorruptionBlackBox can correctly attribute
responsibility if needed, and AICH block verification has a full block to work with.

Implementation sketch

1. BaseClient.cpp — add bKeepFirst to ClearDownloadBlockRequests()

void CUpDownClient::ClearDownloadBlockRequests(bool bKeepFirst /*= false*/)
{
    // m_DownloadBlocks_list: queued but not yet sent — always free entirely.
    for (auto* block : m_DownloadBlocks_list) {
        if (m_reqfile)
            m_reqfile->RemoveBlockFromList(block->StartOffset, block->EndOffset);
        delete block;
    }
    m_DownloadBlocks_list.clear();

    // m_PendingBlocks_list: in-flight blocks.
    // When bKeepFirst is set, skip the first entry so the client can finish
    // the block it is currently receiving before disconnecting.
    auto it = m_PendingBlocks_list.begin();
    if (bKeepFirst && it != m_PendingBlocks_list.end())
        ++it;  // preserve the currently-receiving block

    while (it != m_PendingBlocks_list.end()) {
        auto* pending = *it;
        if (m_reqfile)
            m_reqfile->RemoveBlockFromList(
                pending->block->StartOffset, pending->block->EndOffset);
        delete pending->block;
        if (pending->zStream) { inflateEnd(pending->zStream); delete pending->zStream; }
        delete pending;
        it = m_PendingBlocks_list.erase(it);
    }
}

2. DownloadClient.cpp — restructure the eviction block

The original code calls SetDownloadState(DS_NONEEDEDPARTS) unconditionally at
line 671, before the slower_client != this branch. SetDownloadState itself
calls ClearDownloadBlockRequests() (without bKeepFirst), which would immediately
clear the first block I just preserved — defeating the purpose.

The fix separates the two eviction paths so that SetDownloadState is only called
for the hard-eviction (self-drop) case:

// ── Was: lines 659-671 (merged, unconditional) ────────────────────────────

if (slower_client != this) {
    // Graceful eviction of the slow peer.
    // Keep its first in-flight block so it can finish cleanly.
    // Do NOT send OP_CANCELTRANSFER and do NOT call SetDownloadState here:
    //   • the remote peer keeps sending data for the current block.
    //   • when the block completes, the slow client calls RequestPackets();
    //     finds no available blocks; reaches DS_NONEEDEDPARTS on its own.
    if (!slower_client->GetSentCancelTransfer()) {
        slower_client->ClearDownloadBlockRequests(/*bKeepFirst=*/true);
        slower_client->SetSentCancelTransfer(1); // no new requests after this block
    }
    // fall through → fast client (this) picks up the freed blocks below

} else {
    // Hard eviction: no slower peer found, drop ourselves.
    if (!GetSentCancelTransfer()) {
        CPacket *packet = new CPacket(OP_CANCELTRANSFER, 0, OP_EDONKEYPROT);
        theStats::AddUpOverheadFileRequest(packet->GetPacketSize());
        ClearDownloadBlockRequests();
        SendPacket(packet, true, true);
        SetSentCancelTransfer(1);
    }
    SetDownloadState(DS_NONEEDEDPARTS);
    return;
}

// ── Was: lines 673-715 (unchanged) ───────────────────────────────────────
// Re-request freed blocks for THIS (fast) client.
AddDebugLogLineN(logLocalClient,
    "Local Client: graceful eviction to " + slower_client->GetFullIP());
wxASSERT(m_DownloadBlocks_list.empty());
wxASSERT(m_PendingBlocks_list.empty());
uint16 count = m_MaxBlockRequests;
std::vector<Requested_Block_Struct *> toadd;
if (m_reqfile->GetNextRequestedBlock(this, toadd, count)) {
    // ... add blocks to m_PendingBlocks_list (unchanged) ...
} else {
    // freed blocks not available on this source — drop self
    // ... existing fallback (unchanged) ...
}

Summary of changes

No other files need to change. All existing call sites pass no argument and get the
current (hard-clear) behaviour unchanged.

Trade-offs

The worst case wait is one 180 KB block at the slow client's speed. For a client at
5 KB/s that is ~36 seconds, during which the fast client is busy downloading the
other freed blocks — so the net throughput impact is minimal.

Related

• CCorruptionBlackBox::TransferredData() / VerifiedData() — attribution is more
  accurate when the slow client always writes complete blocks.
• CPartFile::AICHRecoveryDataAvailable() — AICH block-level recovery works correctly
  regardless, but is invoked less often with this fix.

Design evolution: superseding the earlier RFC

An earlier version of this work proposed a time-bounded partial reallocation strategy
based on a minimum throughput threshold of 10.0 KB/s (derived from the 18-second
Maximum Tolerable Delay for a 180 KB block at that speed). The idea was to classify
slow clients into two tiers:

• < 10.0 KB/s → destructive reallocation (current hard eviction behaviour)
• ≥ 10.0 KB/s → protected reallocation: steal all blocks except one, let the
  client finish its current block cleanly

After testing and further analysis, I concluded that this threshold-based approach
solves the right problem but draws the wrong line:

1. The corruption risk is speed-independent. A client writing a partial block at
   9 KB/s creates the same split-block situation as one writing at 1 KB/s. The
   corruption window exists whenever a client is mid-block at eviction time, regardless
   of its throughput. Applying the protection only above 10 KB/s leaves the bug intact
   for the majority of evictions.

2. The threshold introduces a fragile edge case. A client oscillating around
   10 KB/s would alternate between protected and destructive eviction non-deterministically,
   making the corruption window intermittent and hard to reproduce or reason about.

3. The graceful eviction generalises the "keep one block" insight universally.
   The protected path of the RFC — preserve one block, let the client finish it — is
   exactly correct. The fix here simply removes the threshold gate and applies that
   logic unconditionally. The cost (waiting for one block to finish) is the same in
   both cases; the benefit (no partial write on disk) is now guaranteed for every
   eviction, not just those above the speed cutoff.

The graceful eviction (bKeepFirst = true) is therefore a strict improvement over the
RFC: simpler, more correct, and without speed-dependent edge cases.

Proposal: Safe Hard Eviction with GAP Recovery (Alternative to Graceful Eviction)

First of all, I would like to sincerely apologize for the multiple changes in design direction over the course of this PR. I am a bit embarrassed by the back-and-forth, but as I ran more real-world tests and analyzed different edge cases, it became clear that the previous solutions either introduced new bottlenecks or didn't fully solve the core issue. I truly appreciate your patience.

After a lot of reflection and tracing actual transfer behaviors in a test environment, I believe I have finally found the cleanest, most robust, and bottleneck-free approach: Safe Hard Eviction with GAP Recovery.

I would like to ask if you think it is appropriate to update this PR to implement this new design instead of the original one.

The New Design: Safe Hard Eviction with GAP Recovery

Instead of keeping the first block in flight and waiting for the slow source to finish (which bottlenecks the end of the download), we disconnect the slow source immediately. However, to prevent split-block corruption, we revert the entire range of the in-flight block back to a GAP in the temporary file.

Timeline:
  1. Slow client writes bytes [0 – 90 KB] of Block 1   → FillGap(0, 90KB)
  2. Fast client is ready. Endgame logic evicts the slow client immediately:
       → Sends OP_CANCELTRANSFER.
       → Calls ClearDownloadBlockRequests(bAddGaps=true).
       → Reverts Block 1 range [0 - 180 KB] to a GAP: AddGap(0, 180KB).
  3. Fast client immediately requests the whole Block 1 [0 - 180 KB].
  4. Fast client downloads the block cleanly from scratch, overwriting the partial data.

Why it is better:

1. Zero Bottleneck: The fast source takes over the block immediately. The download finishes at the maximum speed of the fast source, without waiting for the slow peer to finish its block.
2. 100% Corruption-Free: Because the range is reset to a GAP, the fast client is guaranteed to download the entire block from scratch. There is no mixing of data from multiple sources in a single block, eliminating split-block AICH failures.

Implementation Details

The changes are compact and self-contained:

1. PartFile.h — Class Friendship

We declare CUpDownClient as a friend of CPartFile to allow it to invoke the private AddGap method:

class CPartFile : public CKnownFile
{
    friend class CPartFileWriteThread;
    friend class CPartFileHashThread;
    friend class CUpDownClient; // Allows client to revert ranges to GAPs on eviction
    ...

2. updownclient.h & BaseClient.cpp — bAddGaps Parameter

We modify ClearDownloadBlockRequests to accept bool bAddGaps = false instead of bKeepFirst. When bAddGaps is true, we add the range of any cancelled pending block back to the gap list:

void CUpDownClient::ClearDownloadBlockRequests(bool bAddGaps)
{
    ...
    // m_PendingBlocks_list:
    while (it != m_PendingBlocks_list.end()) {
        Pending_Block_Struct *pending = *it;

        if (m_reqfile) {
            m_reqfile->RemoveBlockFromList(
                pending->block->StartOffset, pending->block->EndOffset);
            if (bAddGaps) {
                m_reqfile->AddGap(pending->block->StartOffset, pending->block->EndOffset);
            }
        }
        ...

3. DownloadClient.cpp — Immediate Eviction

We restore the immediate eviction path for slow sources, passing true to ClearDownloadBlockRequests:

        if (slower_client != this) {
            if (!slower_client->GetSentCancelTransfer()) {
                CPacket *packet = new CPacket(OP_CANCELTRANSFER, 0, OP_EDONKEYPROT);
                theStats::AddUpOverheadFileRequest(packet->GetPacketSize());
                slower_client->ClearDownloadBlockRequests(true); // bAddGaps = true
                slower_client->SendPacket(packet, true, true);
                slower_client->SetSentCancelTransfer(1);
            }
            slower_client->SetDownloadState(DS_NONEEDEDPARTS);
        }

Trade-offs

Given that a block is only 180 KB, discarding the partial progress of a slow client is extremely cheap in terms of bandwidth, while the benefits in speed and safety are absolute.

Would you see fit that I update the PR to implement this new design? Let me know your thoughts.

Hi @Cflsft don't worry about iterating on this PR multiple times, it will ultimately reach the good solution. And I think many users will appreciate a solution to the slow download speed when file reaches 99%.

On your last question, IMHO wasting 180KB of transfer seems acceptable to me in 2026 Internet.

However, I have a question concerning your previous comment:

Scenario 1 — Complete blocks, two clients (no inherent bug)

Slow client:  [block 1: 0–180 KB]  [block 2: 180–360 KB]   ← writes complete blocks
Fast client:  [block 3: 360–540 KB] … [block 52: …–9.28 MB] ← writes complete blocks

→ Part hash covers all blocks together.
→ If any client sent corrupt data for their block(s), the part hash fails.
→ AICH can pinpoint exactly which 180 KB block failed and from whom.
→ Only that 180 KB block needs re-downloading. Attribution is clean.

This is normal network-level corruption, not an endgame bug. It is handled correctly by the existing CorruptionBlackBox + AICH recovery pipeline.

Scenario 2 — Split block, two clients (the actual bug)

Slow client:  writes [0–90 KB] of block 1   → FillGap(0, 90KB) — permanent on disk
  ↓ evicted mid-block
Fast client:  writes [90–180 KB] of block 1 ← clean data from a different source

→ Block 1's 180 KB comes from two different clients.
→ If the slow client's 90 KB was corrupt or incomplete, block 1's AICH hash fails.
→ CorruptionBlackBox sees BOTH clients wrote into that range — attribution is ambiguous.
→ The entire block (and potentially the full 9.28 MB part) must be re-downloaded.

This is the bug introduced by the current hard eviction. ClearDownloadBlockRequests() correctly frees the unstarted blocks back to the gap list, but it cannot undo bytes already written to disk via FillGap(). That partial write becomes invisible to the hash system until the part-level MD4 or the final file hash runs.

If I'm understanding correctly, the difference between the 2 scenarios is that one 180KB corrupt block is downloaded from a single peer in the first scenario, and from 2 different peers in the second.

However, I don't understand why the first scenario would be recovered by AICH downloading only the wrong block, but on the second scenario you talk about a potential need to download the 9.28MB part.

I mean, if we ask a peer for the needed AICH hash set to identify the wrong block, verify the packet and find the wrong block, why would corruption attribution be an issue and requiere a larger redownload to fix it?

If that is the case, that would be a latent bug worth fixing independent of your endgame PR.

Hi @danim7, you are correct about the 9.28 MB fallback. Both scenarios recover only the 180 KB block if AICH is available, and both fall back to the full 9.28 MB part if it is not. My previous comment was wrong to attribute that difference specifically to Scenario 2.

That said, during my testing I noticed an unjustified increase in corruption events specifically during the endgame phase with the original eviction code. The original code systematically creates split blocks at scale — during a single endgame phase, dozens of evictions can occur, each leaving partial writes from one peer to be completed by another. In normal downloads, split blocks are rare and accidental; in the endgame, they become the rule rather than the exception.

When I switched to the GAP Recovery approach — reverting the in-flight block to a GAP and letting the fast peer download it entirely from scratch — I did not encounter a single corruption error during the endgame phase across all my latest downloads.

That said, given that I was already wrong about how AICH recovery works in this context, I do not want to rush into a new design change. Before formally proposing this as a code change I want to do more testing with a wider variety of files, to rule out that the corruption increase I observed was specific to the files I used for testing.

Hi @Cflsft ,

Think of the PR area as a workplace. You can polish the code until it shines, no need to hurry.

The problem you are trying to solve is complicated and poorly understood in general.

No hurries, take as much time as you need. Change the code as much as you need.

Best regards.

@danim7 is right that the recovery mechanism is symmetric across the two scenarios — both recover a single 180 KB block when AICH is usable, and both fall back to the full 9.28 MB part when it isn't. What decides which path runs is whether the file's AICH master hash is trusted, and that's a property of the file's source population, not of the block or the download phase: when a part fails its MD4 hash, RequestAICHRecovery() returns immediately unless the AICH set is AICH_TRUSTED/AICH_VERIFIED. AICH_TRUSTED needs a quorum — MINUNIQUEIPS_TOTRUST = 10 unique IPs reporting the same root hash with MINPERCENTAGE_TOTRUST = 92 % agreement (SHAHashSet.cpp:44-46, in UntrustedHashReceived()); AICH_VERIFIED only when we already hold the full hashset (e.g. the AICH hash in the ed2k link). So a rarely-sourced file may never reach the quorum and re-downloads the whole 9.28 MB part on any single bad block, while a well-sourced file repairs just the 180 KB. That's the latent behaviour worth being aware of, independent of endgame.

On the corruption itself, it's worth pinning down the mechanism before redesigning around it, because the committed code doesn't obviously create one. The current path is the existing drop-slow-source-and-reassign: GetSlowerDownloadingClient → ClearDownloadBlockRequests() on the slow peer → the fast peer re-requests the freed gaps. GetNextEmptyBlockInPart() only hands out a range when !IsAlreadyRequested(...), so two sources never download the same un-filled range at once — there's no concurrent-write race. The only multi-source case is sequential: the slow peer's already-written prefix plus the fast peer's suffix, and the slow peer's late in-flight data is discarded once its block leaves the pending list. A block touched by a bad source fails its hash and re-downloads the same whether or not it was split, so I don't see how eviction raises the corruption rate — what it does change is that nearCompletion now exercises this reassignment far more often at the tail.

@Cflsft — could you share the before/after corruption counts from the runs that motivated this (the committed build vs. your local GAP-recovery build, ideally same files/sources)? If the increase holds up across a wider set, that isolates whether it's the reassignment frequency or something subtler in the partial-write/gap accounting, and it would justify the change on its own merits.

For the design direction, GAP-recovery is a reasonable refinement regardless: reverting the in-flight block to a gap keeps the reassigned block single-sourced and avoids the slow-tail wait that the keep-first variant reintroduces (the 99 % stall this PR exists to remove), at a negligible ≤180 KB cost. If you take it, a small CPartFile helper (a public RevertBlockToGap(start, end)) is cleaner than friend class CUpDownClient, and keeping it behind the opt-in EnableEndGame = 0 default while it gets wider testing makes sense.

Hi again,

To better understand this issue, I ran some tests with full debug logging enabled. I wanted to trace exactly what happens during the endgame phase when blocks are split, specifically testing the hypothesis that AICH_TRUSTED would safely recover those corrupted blocks.

In one specific session, I was downloading 4 files simultaneously. I am attaching the annotated log (logfile_popular_files_annotated.txt) showing what happened to the 2 largest ones.

The log confirms aMule easily reached AICH_TRUSTED (100% quorum) for both files (one had 56 trusted sources, the other 10).
During the endgame, multiple OP_CANCELTRANSFER events were triggered to slow clients across these downloads.
Immediately after this source rotation finished filling the final blocks, the HashThread verified the files and reported 9 corrupted parts simultaneously (1 part in the first file, and 8 parts in the second).
Despite having AICH_TRUSTED active, aMule was unable to recover these split 180 KB blocks. The recovery process completely failed for both files, discarding 9 full parts (83 MB lost in total) and aborting the final hashset creation.
Summary: The logs consistently show that when the endgame splits a 180 KB block via OP_CANCELTRANSFER during source rotation, it frequently results in a corrupted 9.28 MB part. In this session alone, 2 out of the 4 downloading files suffered massive corruptions due to this.

I am not entirely sure exactly why this happens at the code level—specifically, why the boundary corruption occurs during the splice, or why AICH fails to recover the 180 KB blocks despite having a full trusted hashset—but the behavior is clearly documented in the logs.

What is clear is the outcome: AICH appears unable to recover these specific split-block corruptions, resulting in discarded parts and massive data loss even with full quorum.

Alternatively, I could test the approach of cancelling the transfer but completely "stealing" the current gap (discarding the slow client's partial block and re-assigning the entire gap to the fast source). This would avoid the split-block splice entirely and theoretically prevent these corruptions. Let me know if you would like me to implement and test that approach instead.

I've attached the log for your review. Let me know your thoughts.

Thanks for the rigorous logging — I went through the full log and it's very informative. A few clarifications, then a concrete next step.

On AICH: the log actually shows recovery was never invoked — there are no RequestAICHRecovery calls in it. The corruption is caught by PartFileHashFinished (the final full-file re-hash on completion), and that path only re-gaps whole parts (AddGap); it never requests the 180 KB AICH sub-block recovery, which lives on the mid-download path. So "Failed to store new AICH Hashset" is just the downstream effect of a part failing here, not an AICH recovery attempt failing — the quorum you reached was simply never used on this path.

On the mechanism: the corruption comes from two things acting together — the endgame splitting a block across two sources and those transfers being compressed. When the faster source evicts the slower one mid-block (OP_CANCELTRANSFER), the 180 KB block is filled partly by each; and because the transfers are packed, one source's partial decompressed output doesn't align cleanly with the other's at the seam, so the splice corrupts the part. Your log shows both halves: for part 202 the CorruptionBlackBox records multiple sources delivering overlapping ranges of the same part (≈2× the part size across 2+ clients — the split), and OP_COMPRESSEDPART is essentially universal (the compression). So it's split-block and compression together, not either alone — and the existing log already has what we need; no new categories required.

Next step (#270): I opened #270, which makes PartFileHashFinished request AICH recovery for parts that fail the final re-hash (it's a no-op unless a trusted AICH hashset is available, so nothing changes for non-AICH files). On a file that reached AICH_TRUSTED like your test, this should recover the split blocks at 180 KB granularity instead of re-downloading whole parts — and it lets the blackbox pinpoint the exact bad block.

Since #270 isn't merged yet, you can pull it straight from my fork and test it together with your endgame branch:

# add my fork as a remote (once) and fetch the AICH-recovery branch
git remote add got3nks https://github.com/got3nks/amule.git
git fetch got3nks fix/aich-recover-on-completion-rehash

# throwaway test branch off your endgame work, with the fix merged in
git checkout -b endgame-aich-test feature/endgame-mode
git merge got3nks/fix/aich-recover-on-completion-rehash
# build, then re-run the popular-file scenario on endgame-aich-test

That keeps your feature/endgame-mode PR branch untouched. (If you'd rather rebase your branch onto it directly, git rebase got3nks/fix/aich-recover-on-completion-rehash while on feature/endgame-mode — that rewrites history and needs a force-push.) The question we're after: does AICH recovery clear the endgame split-block corruption once it's actually asked?

Once that's confirmed, your "discard the slow source's partial and re-request the whole block from one source" idea is the right root-cause fix — a block should come wholly from a single source, never spliced. Let's land the AICH recovery piece first, then do that.

On the mechanism: the corruption comes from two things acting together — the endgame splitting a block across two sources and those transfers being compressed. When the faster source evicts the slower one mid-block (OP_CANCELTRANSFER), the 180 KB block is filled partly by each; and because the transfers are packed, one source's partial decompressed output doesn't align cleanly with the other's at the seam, so the splice corrupts the part. Your log shows both halves: for part 202 the CorruptionBlackBox records multiple sources delivering overlapping ranges of the same part (≈2× the part size across 2+ clients — the split), and OP_COMPRESSEDPART is essentially universal (the compression). So it's split-block and compression together, not either alone — and the existing log already has what we need; no new categories required.

Thank you for your analysis, got3nks!

Independently of the endgame feature, could this mechanism be already causing corruption in the production version? What happens with a 180KB block when a peer suddenly disconnects leaving the block half completed, and we finally fill it from another peer?

Hi @got3nks,

Thank you for the analysis.

To test this, I created a local experimental branch merging my endgame branch with your #270 PR (fix/aich-recover-on-completion-rehash). I compiled the debug build and ran a similar scenario to trigger the endgame source rotation.

The results confirm your analysis. The fix works as intended.

This confirms definitively that the endgame rotation corrupted precisely a single 180 KB chunk, and your PR successfully intercepted the final hash failure,, and restored the remaining 9.10 MB of the part. This resolves the data loss issue.

The endgame fix and the clang-tidy pass both look right — DS_ONQUEUE on the two near-completion self-drop paths with case 2 left as DS_NONEEDEDPARTS, and the five modernize fixes are all in. I re-ran the Release endgame stress test (8 concurrent downloads × 5 cycles) on this tip and got 5/5 clean: no freeze, no complete-stall, and the self-drop paths requeue without thrash (worst any single peer hit was 2× in one cycle). Nice.

One thing to pull out before merge though: the style(core): fix clang-tidy warnings… commit (f6eea4d2) accidentally added a pupnp submodule gitlink:

new file mode 160000
+Subproject commit c540ce2431bdeac73359029d4592b45790e1d154

That's unrelated to the endgame work — it looks like a broad git add picked up your local pupnp/ build checkout while you were regenerating the catalogs. It isn't tracked on master (aMule builds against the system libupnp via cmake/upnp.cmake), and there's no .gitmodules, so it's a dangling reference. CI won't flag it since git submodule update ignores gitlinks with no .gitmodules entry, but it'd land a phantom submodule in the tree. Dropping it:

git rm --cached pupnp
git commit --amend --no-edit
git push --force-with-lease

After that this is good to go from my side.

Thank you so much to everyone for the guidance, reviews, and invaluable help throughout this PR!

There is something wrong with the clang-tidy Tier-1 job, it is taking too long.

There is something wrong with the clang-tidy Tier-1 job, it is taking too long.

I'm monitoring it. It was just a VERY SLOW dependencies download from Ubuntu servers.

The analysis seems compromised, some files get an usage message with Error: no checks enabled.

The no checks enabled errors are confined to files under src/extern/ and src/webserver/src/, which each carry a local .clang-tidy with Checks: '-*' — the vendored (wxWidgets) and generated (php_lexer / php_parser / php_syntree / etc.) code we deliberately keep out of the lint. clang-tidy 21 turned an empty enabled-check set into a hard Error: no checks enabled where older versions silently no-op'd, so it's new noise rather than a new gap.

All real source was analyzed normally — the run linted 219/249 files and the endgame changes went through it (DownloadClient.cpp, PartFile.cpp and TextClient.cpp are all in the list with diagnostics enabled). So no project file is being skipped that wasn't already intentionally excluded; the only change is that the excluded ones now print an error instead of quietly doing nothing.

If we want the log clean, run-clang-tidy takes --allow-no-checks, which is the documented switch for exactly this "all checks disabled for some files" case. I can send a one-line workflow PR for it if that's useful.

Merged, congratulations! 🎉 Thanks for seeing this all the way through, @Cflsft. This one had a genuinely tricky failure mode (the 99.9% self-banishment cascade) and you nailed both the mechanism and the fix, plus wired the toggle cleanly through every surface. Validated 5/5 clean endgame cycles on the final tip with no thrash. Great contribution.

Congratulations, all of you! That must have been one of the most tricky bugs I have seen in aMule development.