Skip to content

bugfix: Search results are mixed between tabs when doing Kad and ed2k searches - #530

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
danim7:fix-mixed-results-search-dialog
May 7, 2026
Merged

bugfix: Search results are mixed between tabs when doing Kad and ed2k searches#530
mrjimenez merged 1 commit into
amule-project:masterfrom
danim7:fix-mixed-results-search-dialog

Conversation

@danim7

@danim7 danim7 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Search results are mixed between tabs when doing Kad and ed2k searches.

Steps to reproduce:

  1. Open a fresh amule session
  2. In Search Dialog, using Search Type=Local/Global, search 'ubuntu' and wait for results to arrive
step2
  1. Change Search Type to Kad and search 'freebsd', you will hit this bug:
    3.a. The new tab 'freebsd' gets a copy of the results of 'ubuntu'. The search count is set to zero
step3a

3.b. The previous tab 'ubuntu' gets a mix of the original 'ubuntu' and the new 'freebsd' results

step3b

The rootcause seems to be the search ID. Actually, there are two variables used to define this value, both starting at 0.
The one for ed2k is defined in SearchDlg.cpp --> CSearchDlg::StartNewSearch() --> static uint32 m_nSearchID = 0;
The one for Kad is defined in SearchManager.cpp --> uint32_t CSearchManager::m_nextID = 0;
A given tab gets assigned a search ID to retrieve the results. Since these counters are managed separately for ed2k and kad, we can get a collision and the tabs will display wrong results...

The proposed fix is to initialize the Kad counter to a high integer, for example 1024.

The rationale for this is:

  • Kad performs background searches (via PrepareLookup), so it needs its own counter for the searchID.
  • This counter tends to increase faster than Local/Global searches, because it contains not only the user-initiated searches but also the background lookups from the previous point. Therefore, we could start it at a higher value than the Local/Global searches, to avoid catching up the other counter.
  • IMHO, starting at 1024 shall be enough for the average user/session, but I can change that value if you think otherwise. This gives the ed2k counter enough room for an average user/session, and it allows the kad counter plenty of space to grow autonomously without overlapping.
  • I also tried to retrieve the value of the Kad counter from SearchDlg.cpp to use it for all search types, but it broke amule-remote-gui compilation, and fixing it was turning into a much larger patch...

TL;DR Search results are mixed between tabs because Kad and ed2k searchIDs are both initialized at 0 and collide. Fix it by initializing Kad ID at a different value

@got3nks

got3nks commented May 7, 2026

Copy link
Copy Markdown
Contributor

Nice catch on the dual-counter root cause.

One thought on the chosen value: 1024 keeps the bug at bay for typical sessions but it's a delay rather than a structural fix — after ~1024 user-initiated Local/Global searches in one session, the ed2k counter catches up to where Kad started and the collision returns.

Same patch size with full structural safety: partition the 32-bit ID space by the top bit so the two allocators can never produce equal values, regardless of session length.

-uint32_t  CSearchManager::m_nextID = 0;
+// Top bit reserved for Kad-allocated IDs; ed2k Local/Global IDs
+// (CSearchDlg::StartNewSearch) live in the bottom half. Keeps the
+// two ID spaces from ever colliding regardless of session length.
+uint32_t  CSearchManager::m_nextID = 0x80000000;

Optionally also on the ed2k side at SearchDlg.cpp:509, for defensive symmetry:

-m_nSearchID++;
+m_nSearchID = (m_nSearchID + 1) & 0x7fffffff;

A couple of reasons this feels like the cleaner shape:

  • ed2k IDs span 0x00000000–0x7FFFFFFF (top bit 0); Kad IDs span 0x80000000–0xFFFFFEFF (top bit 1). The two sets are disjoint by construction — no collision is possible.
  • It fits the existing convention in this file. SearchManager.cpp:169 already reserves the top byte (0xffffff00..0xffffffff) for "use supplied ID as-is" related-search IDs, so high-bit tagging of the search-ID space is already the pattern; this just splits the remaining space cleanly between the two allocators.
  • Both counters stay where they are, so it sidesteps the amule-remote-gui link breakage you hit when trying a single shared counter.

Happy to open this as a counter-PR off your branch if it's easier than amending here — your call.

@got3nks

got3nks commented May 7, 2026

Copy link
Copy Markdown
Contributor

Posted #533 as an alternative — same bug, same dual-counter root cause you diagnosed (exactly right), but using a top-bit partition of the 32-bit ID space so the two counters can never alias regardless of session length. The +1024 offset here delays the collision; the partition prevents it structurally. Thanks for tracking this one down — your repro and analysis are what made the alternative possible.

@danim7

danim7 commented May 7, 2026 via email

Copy link
Copy Markdown
Contributor Author

@danim7
danim7 force-pushed the fix-mixed-results-search-dialog branch from a8e916b to fa7f0d3 Compare May 7, 2026 11:51
@danim7

danim7 commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

hi @got3nks

I amended my commit and implemented your idea, which prevented the ed2k counter from ever entering kad counter space.
However, there was still an edge case, were the kad counter could enter ed2k space and collide.
So instead of just initializing to 0x80000000, I turned it into a mask that is applied on each counter increment.
Now, the collision problem is definitely solved for all edge cases.

As an extra modif, I removed the unused function SetNextSearchID(), as it could have a misleading behaviour now because of the mask. If someone ever needs it, they will arrive to the actual variable and mask with the explanatory comment.

What are your thoughts?

@got3nks

got3nks commented May 7, 2026

Copy link
Copy Markdown
Contributor

Yep, that's better — the OR-mask on each emit closes the (theoretical) wrap-around edge case ours has: once m_nextID increments past 0xFFFFFFFF and wraps to 0x00000000, our naked ++m_nextID would briefly output values with the top bit clear, i.e. into ed2k territory. The mask on every emit forces the top bit back even after wrap. Practically irrelevant (2^31 searches in one session is implausible) but defensively correct, and reads cleaner with the SEARCH_ID_KAD_MASK named constant. Good catch.

Removing the unused SetNextSearchID() setter is also right — it'd silently get OR'd later and would surprise any future caller.

Going to close #533 in favor of yours; this is strictly a superset of what we proposed.

@mrjimenez

Copy link
Copy Markdown
Contributor

Great work, guys!

@mrjimenez
mrjimenez merged commit 3008ada into amule-project:master May 7, 2026
12 checks passed
@danim7
danim7 deleted the fix-mixed-results-search-dialog branch June 9, 2026 16:34
got3nks added a commit to got3nks/amule that referenced this pull request Jul 20, 2026
…ct#530)

The Directories preferences panel was inert in amuleGUI: its tree browses the wrong filesystem when the core is remote, and nothing it produced reached the core (the shared-folder lists are not in the EC preferences packet, and SaveSharedFolders() is compiled out under CLIENT_GUI), while the reload sent alongside made the core rescan its own unchanged config — so it looked applied.

Adds EC_OP_GET/SET_SHARED_DIRS carrying the explicit and recursive roots, negotiated via EC_TAG_CAN_SHAREDDIRS_CONFIG. The core validates each path, applies and persists the valid ones, rescans, and reports refused paths with a locale-neutral reason; the union file is refreshed alongside the intent lists so the rescan cannot trim the new roots back out. amuleGUI replaces the tree with a Path/Recursive list editor owned by CPreferencesRem, which outlives the dialog.

The monolithic tree now tracks the roots it was painted from and rebuilds only when they move, and its pending-edit flag is cleared at session end instead of latching for the dialog lifetime.

muuli_wdr.cpp moves from the muleappgui static library into GUI_SOURCES so each executable compiles it with its own defines; it was previously built once without CLIENT_GUI, making every such branch in it dead code. Closes amule-project#484.
got3nks added a commit to got3nks/amule that referenced this pull request Jul 20, 2026
…ect#533)

The Flatpak job regenerates ECCodes.h from ECCodes.abstract rather than using the committed header, and amule-project#530's comments in that file made the generated header unparseable ("invalid preprocessing directive #=" plus a truncated ECOpCodes enum), failing every Flatpak build on master.

The generator only treats a line as a comment when it matches "^#" with no leading whitespace, so the comments indented to line up with the tag entries fell through to the data path and had every word turned into a field. It also flattens the file into a CMake list by replacing newlines with ";", so a semicolon inside a comment splits the line and the remainder loses its leading "#".

Documentation for these codes lives in the hand-maintained header, so the abstract keeps to entries. Generating before and after: master produces 8 mangled lines and 11 compile errors, this produces none, with all seven new codes intact.
got3nks added a commit to got3nks/amule that referenced this pull request Jul 20, 2026
…-project#531)

Follow-up to amule-project#530, which made the core's share roots reachable over EC. amuleapi could list the files a share produced and ask the core to re-walk its roots, but had no way to see or change which roots those were.

Adds /shared/directories: GET returns the configured roots as {path, recursive} (GUEST, matching GET /shared and GET /preferences); PUT replaces the whole set, mirroring the core operation rather than hiding a read-modify-write; POST adds a single root and DELETE removes one, since the scripted case is a single folder. POST is idempotent so "ensure this is shared" repeats safely; DELETE 404s on an unconfigured path so a typo is visible.

The core validates each path — a REST client cannot stat the core filesystem — applying the ones that pass and returning the rest in `rejected` with a reason, so one bad path never discards the edit. Reasons arrive as codes and are rendered by the API, keeping the core locale out of responses.

POST and DELETE hold a mutex across their read and write, since SendRecvSerialized locks per roundtrip and two concurrent adds would otherwise lose one. Covered by a new curl smoke that snapshots and restores the operator configuration, 30/30 passing live.
got3nks added a commit to got3nks/amule that referenced this pull request Jul 21, 2026
…ale daemons

A headless amuled started without LANG / LC_* (systemd, Docker) runs under
the POSIX/C locale, whose ASCII codeset cannot represent accented or other
non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so
an accented shared directory silently becomes invisible to the file scan —
reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a
UTF-8 locale finds them. This newly bites the remote shared-folder config
(amule-project#530), whose whole point is configuring a headless core, but it affects all
of amuled's non-ASCII file handling.

aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to
en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the
promotion survives wx re-resolving the locale from the environment during
app init — a plain setlocale() alone is undone there. The guard is the
codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden,
and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt
(wide-char filesystem APIs).

Verified on an ARM64 glibc VM: with the fix an accented shared folder is
found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8
locales alike, with no change for a real locale (it_IT.UTF-8).
got3nks added a commit to got3nks/amule that referenced this pull request Jul 21, 2026
…ale daemons

A headless amuled started without LANG / LC_* (systemd, Docker) runs under
the POSIX/C locale, whose ASCII codeset cannot represent accented or other
non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so
an accented shared directory silently becomes invisible to the file scan —
reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a
UTF-8 locale finds them. This newly bites the remote shared-folder config
(amule-project#530), whose whole point is configuring a headless core, but it affects all
of amuled's non-ASCII file handling.

aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to
en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the
promotion survives wx re-resolving the locale from the environment during
app init — a plain setlocale() alone is undone there. The guard is the
codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden,
and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt
(wide-char filesystem APIs).

Verified on an ARM64 glibc VM: with the fix an accented shared folder is
found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8
locales alike, with no change for a real locale (it_IT.UTF-8).
got3nks added a commit to got3nks/amule that referenced this pull request Jul 21, 2026
…ale daemons (amule-project#542)

A headless amuled started without LANG / LC_* (systemd, Docker) runs under
the POSIX/C locale, whose ASCII codeset cannot represent accented or other
non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so
an accented shared directory silently becomes invisible to the file scan —
reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a
UTF-8 locale finds them. This newly bites the remote shared-folder config
(amule-project#530), whose whole point is configuring a headless core, but it affects all
of amuled's non-ASCII file handling.

aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to
en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the
promotion survives wx re-resolving the locale from the environment during
app init — a plain setlocale() alone is undone there. The guard is the
codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden,
and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt
(wide-char filesystem APIs).

Verified on an ARM64 glibc VM: with the fix an accented shared folder is
found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8
locales alike, with no change for a real locale (it_IT.UTF-8).
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.

3 participants