fix(shares): handle file rename / move-out / delete during hash by got3nks · Pull Request #858 · amule-project/amule

got3nks · 2026-06-05T20:08:11Z

Summary

Two complementary fixes for the same race: a file is renamed, moved out of the watched tree, or deleted while CHashingTask is mid-hash. The hash completes anyway (against its open fd), and depending on whether any later hash for the same content also lands, the file either stays in m_Files_map under a ghost name or leaves a stale entry whose underlying CKnownFile* may eventually get pruned.

Reproducer

Rapid renames of a large file in a shared dir. The watcher fires CREATE → hash queued → rename → CREATE for the new name → hash queued → and so on. None of the long-running hashes complete during the renaming; they then land one after the other under their respective stale names. After everything settles, show shared lists the file under whatever name won the hash race, while the real on-disk file has a different name and is invisible to peers.

Fix 1 — drop hashed-then-vanished file

amule.cpp: OnFinishedHashing re-checks FileExists on the path the hash ran against. If the file is no longer there at completion, the result is dropped. Catches move-out, delete-during-hash, and the common rename-during-hash case (the hash for the old name no longer points at a real file).

Fix 2 — swap stale entry on rename-during-hash

SharedFileList.cpp: when two hashes for the same content both complete and both attempt to add, the first lands cleanly. The second hits CKnownFileList::Append's rename-during-hash branch — which demotes the prior CKnownFile to m_duplicateFileList and installs the new Record as canonical in m_knownFileMap. But sharedfiles->AddFile silently fails on the duplicate hash, so the shared view keeps pointing at the demoted pointer (whose filename no longer matches disk, and which the duplicate-list prune may free later → dangling pointer in m_Files_map / m_pathIndex).

SafeAddKFile now detects this: when AddFile fails and the existing entry is a different CKnownFile*, detach the stale entry via RemoveFile and add the live one.

Coverage

Scenario	Caught by
Rename A → B, only one hash completes	Fix 1
Rename A → B, both hashes complete	Fix 1 + Fix 2
Move out of watched tree during hash	Fix 1
Delete during hash	Fix 1
Multi-rename burst	Fix 1 (all stale-path hashes dropped)

DIAGNOSTIC BRANCH — NOT for merge. Promotes the new hash-completion / rename-during-hash logs to AddLogLineN with [diag] prefix so the PR amule-project#858 fixes are observable without enabling KnownFileList debug. Covers: - Fix 1: "Hashed file vanished before share, dropping:" (file moved out / deleted / renamed mid-hash → hash result discarded) - "Safe adding file to sharedlist:" (hash completion landed cleanly in m_knownFileMap and forwarded to sharedfiles) - "File not added to sharedlist:" (knownfiles SafeAddKFile returned false — usually a same-name/mtime/size duplicate) - Fix 2: new "[diag] SafeAddKFile: rename-during-hash swap" line inside the SharedFileList::SafeAddKFile swap branch, naming the stale entry being detached and the live one taking its place

Two complementary fixes for the same race: a file gets renamed, moved out of the watched tree, or deleted while CHashingTask is mid-hash. The hash continues against its open fd and completes with the original path; what happens next depends on whether any later hash for the same content also lands. The reproducer is rapid renames of a large file in a shared dir: the watcher fires CREATE -> hash queued -> rename -> CREATE for the new name -> hash queued -> and so on. None of the long-running hashes complete during the renaming, then all fire one after another under their respective stale names. (1) OnFinishedHashing re-checks the path of the hashed file at completion. If the file is no longer there (moved out, deleted, renamed mid-hash), drop the result instead of adding it to shares under a name that does not exist on disk. Without this, the entry surfaces in `show shared` but peers cannot fetch it. (2) CSharedFileList::SafeAddKFile handles the residual case where two hashes for the same content both complete and both attempt to add: the first lands, the second hits CKnownFileList::Append's rename-during-hash branch (existing demoted to m_duplicateFileList, new Record installed as canonical), then sharedfiles->AddFile silently fails on the duplicate hash. The shared view stays pointing at the demoted pointer, whose filename no longer matches disk and whose duplicate-list prune may eventually free it (dangling pointer in m_Files_map / m_pathIndex). When AddFile fails on a different CKnownFile*, detach the stale entry and install the live one so the view mirrors knownfiles.

DIAGNOSTIC BRANCH — NOT for merge. Promotes the four hash-completion / rename-during-hash logs introduced by the fix in amule-project#858 to AddLogLineN with [diag] prefix so they are visible at default log verbosity: - "Hashed file vanished before share, dropping:" (Fix 1, file moved out / deleted / renamed mid-hash → result discarded) - "Safe adding file to sharedlist:" (hash completion landed in m_knownFileMap and forwarded to sharedfiles) - "File not added to sharedlist:" (knownfiles SafeAddKFile rejected, typically a same-name/mtime/size duplicate) - "SafeAddKFile: rename-during-hash swap" (Fix 2, m_Files_map stale entry detached and live one installed)

got3nks · 2026-06-05T21:52:06Z

Functional coverage matrix for #858, exercised end-to-end on Linux ARM64 (amule-dev-vm) against a 5 GB test file:

#	Scenario	Result
1	Happy path (drop file, hash completes)	PASS — `Safe adding file to sharedlist: scenario1.bin`
2	Rename A→B AFTER hash A lands	PASS — watcher's `NotifyPathRemoved` → `RemoveFile` → fresh hash B → AddFile clean
3	Rename A→B BEFORE A's hash completes	PASS — Fix 1: `Hashed file vanished before share, dropping: scenario3-A.bin` + `Safe adding scenario3-B.bin`
4	Move out of shared tree during hash	PASS — Fix 1: `Hashed file vanished before share, dropping: scenario4.bin`
5	Delete during hash	PASS — Fix 1: `Hashed file vanished before share, dropping: scenario5.bin`
6	Multi-rename burst (1→2→3→4→5 in <3s)	PASS — watcher coalesces the rename storm; only the surviving-name hash runs; final name lands under `scenario6-5.bin`

Fix 1 (vanish-on-completion guard) is the primary work-horse — it catches every case where the file moved / renamed / deleted while the hash was in flight (3, 4, 5). The watcher's own NotifyPathRemoved handles the post-hash rename path (2) before any second hash can collide. Fix 2 (rename-during-hash swap in SafeAddKFile) is belt-and-suspenders against races the watcher would miss (backend-overflow fallback Reload, or two physically distinct paths sharing identical content) — not reachable from natural rename-burst paths, but the code is still correct defence in depth.

Ready for merge.

…ule-project#912) Extends existing categories (preferring extensions over new lines): - Performance/Upload: amule-project#898 SlotAllocation default raised. - Networking & Discovery: wire-parser hardening list extended with amule-project#879/amule-project#882/amule-project#890/amule-project#886; new amuleweb security hardening bullet consolidating ngosang's amule-project#869-amule-project#874 triage (all landed in amule-project#875); amulegui list extended with amule-project#857; shared-folder watcher extended with amule-project#858. - Packaging: Windows installer i18n line extended with amule-project#899. - Internals & Refactoring: new docs-polish + code-quality bullets covering amule-project#851/amule-project#855/amule-project#862/amule-project#888/amule-project#900/amule-project#866/amule-project#867/amule-project#895 and amule-project#909/amule-project#910/amule-project#912. - Translations: new pre-release final-wave bullet covering amule-project#847/amule-project#856/ amule-project#891/amule-project#908/amule-project#860/amule-project#904/amule-project#859/amule-project#863/amule-project#861/amule-project#880/amule-project#911/amule-project#901/amule-project#902/amule-project#889/amule-project#868/amule-project#853. - Bug Fixes & Stability: amule-project#850/amule-project#854/amule-project#878/amule-project#906. - CI: ccache wiring (amule-project#892, amule-project#903) + CodeQL binutils-dev (amule-project#907). Contributors footer gains mifritscher and nguyenhoangminhhieu2004-gif (both first-time contributors). PR index extended through amule-project#912.

Extends existing categories (preferring extensions over new lines): - Performance/Upload: #898 SlotAllocation default raised. - Networking & Discovery: wire-parser hardening list extended with #879/#882/#890/#886; new amuleweb security hardening bullet consolidating ngosang's #869-#874 triage (all landed in #875); amulegui list extended with #857; shared-folder watcher extended with #858. - Packaging: Windows installer i18n line extended with #899. - Internals & Refactoring: new docs-polish + code-quality bullets covering #851/#855/#862/#888/#900/#866/#867/#895 and #909/#910/#912. - Translations: new pre-release final-wave bullet covering #847/#856/ #891/#908/#860/#904/#859/#863/#861/#880/#911/#901/#902/#889/#868/#853. - Bug Fixes & Stability: #850/#854/#878/#906. - CI: ccache wiring (#892, #903) + CodeQL binutils-dev (#907). Contributors footer gains mifritscher and nguyenhoangminhhieu2004-gif (both first-time contributors). PR index extended through #912.

DIAGNOSTIC BRANCH — NOT for merge. Promotes the four hash-completion / rename-during-hash logs introduced by the fix in amule-project#858 to AddLogLineN with [diag] prefix so they are visible at default log verbosity: - "Hashed file vanished before share, dropping:" (Fix 1, file moved out / deleted / renamed mid-hash → result discarded) - "Safe adding file to sharedlist:" (hash completion landed in m_knownFileMap and forwarded to sharedfiles) - "File not added to sharedlist:" (knownfiles SafeAddKFile rejected, typically a same-name/mtime/size duplicate) - "SafeAddKFile: rename-during-hash swap" (Fix 2, m_Files_map stale entry detached and live one installed)

DIAGNOSTIC BRANCH — NOT for merge. Promotes the four hash-completion / rename-during-hash logs introduced by the fix in amule-project#858 to AddLogLineN with [diag] prefix so they are visible at default log verbosity: - "Hashed file vanished before share, dropping:" (Fix 1, file moved out / deleted / renamed mid-hash → result discarded) - "Safe adding file to sharedlist:" (hash completion landed in m_knownFileMap and forwarded to sharedfiles) - "File not added to sharedlist:" (knownfiles SafeAddKFile rejected, typically a same-name/mtime/size duplicate) - "SafeAddKFile: rename-during-hash swap" (Fix 2, m_Files_map stale entry detached and live one installed) (cherry picked from commit 26f5f4c)

…oes (amule-project#858) The splash stayed up until the startup hash queue drained, because FinishStartupSplash() both ended the list batches and showed the window, and it only ran once CThreadScheduler reported nothing left to hash. A share with many new files therefore left the application unreachable for as long as hashing took -- half an hour in issue amule-project#853 -- and Preferences was among the things out of reach, so a user could not stop aMule hashing a folder they no longer wanted to share. The justification for waiting does not hold. It reasoned that each completion's main-thread work would leave the UI unusable, but that is only true once the batch is over: while it is open, BeginBatchUpdate() makes ShowFile() an O(1) append instead of a sorted insert that rebuilds the row index. The freeze it was avoiding came from ending the batch, which the same function did a few lines earlier. So the two are separated. When the scan ends the download list closes its batch as before, the shared list sorts what the scan found and thaws while its batch stays open, and the window goes up. Rows from the drain then arrive as appends, and the poll tick sorts them into place -- but only when one actually arrived. Hashing cost tracks bytes, so a large file is minutes of nothing followed by a single append; sorting per tick regardless would run a full std::sort, a row-index rebuild and a model reset once a second for no reordering at all. The tick also reports what is left on the shared-files label, so the count is visible in the window rather than on a splash nobody can see past. Hashing gets the whole of the bar's last band back as a result: it had kScanBandEnd = 99 and its own band from there, so the longest phase of a first run advanced the bar by one point and read as a hang. CSplashScreen::SetLoopRunning() went with it. It existed so updates would stop pumping the loop once the splash outlived OnInit, which it no longer does, and without it m_pumpsLoop could never be false -- leaving an unreachable branch in SetProgress().

got3nks force-pushed the fix/hash-rename-race branch from ee619e2 to 18f7222 Compare June 5, 2026 20:15

got3nks marked this pull request as draft June 5, 2026 21:28

got3nks marked this pull request as ready for review June 5, 2026 21:52

mrjimenez merged commit 7580cbd into amule-project:master Jun 5, 2026
7 checks passed

got3nks deleted the fix/hash-rename-race branch June 5, 2026 22:35

Navigation Menu

Uh oh!

fix(shares): handle file rename / move-out / delete during hash - #858

fix(shares): handle file rename / move-out / delete during hash#858
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/hash-rename-race

got3nks commented Jun 5, 2026

Uh oh!

got3nks commented Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

got3nks commented Jun 5, 2026

Summary

Reproducer

Fix 1 — drop hashed-then-vanished file

Fix 2 — swap stale entry on rename-during-hash

Coverage

Uh oh!

got3nks commented Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants