ServerSocket: disconnect explicitly when server assigns no client ID by got3nks · Pull Request #788 · amule-project/amule

got3nks · 2026-05-31T14:21:18Z

Summary

Follow-up to #787 on the UX of OP_IDCHANGE's new_id == 0 branch. When the ed2k server gives up on its HighID-callback verification and assigns no client ID, the handler at ServerSocket.cpp:300-311 silently breaks without ever calling SetConnectionState(CS_CONNECTED). The socket sits in CS_NOTCONNECTED limbo until the 15 s client-side timeout fires and tears the connection down with no visible indication of what happened. mifritscher2 surfaced this on #778 while diagnosing the load-correlated race that #787 fixes.

After #787 lands, this code path becomes rarer (most LowIDs were the throttler-induced verification timeout, not actual server rejections) but it still fires when a server has anti-abuse policy or a tight verification budget that the throttler bypass doesn't help with. So the silent failure mode is worth fixing on its own.

Fix

Surface the rejection explicitly:

AddLogLineC a clear "Server <name> (<ip:port>) rejected our login (no client ID assigned). Disconnecting." line
Disconnect via the same teardown path OnError already uses for IO failures (m_bIsDeleting, SetConnectionState(CS_DISCONNECTED), serverconnect->DestroySocket(this))

Subsequent auto-connect logic picks the next server immediately rather than waiting out the silent 15 s.

The SmartID state-machine bookkeeping (incrementing state, capping at 3) is preserved before the disconnect so the cross-server retry policy still tracks the rejection.

Out of scope

The neighbouring SmartID-retry break at line ~324 has the same fall-into-limbo shape but is intentionally part of the "keep trying servers until one gives HighID" loop — the silent wait is load-bearing for that flow's pacing. Left alone here.

Test plan

amule + amuled build clean on macOS
Field verification by @mifritscher2 — when the server-rejection case fires, the failure should now appear immediately in the log instead of after a 15 s silent timeout

Pairs with #787; either order of landing works since this PR only changes the visible behaviour of a code path the other PR makes rarer.

…mule-project#778) When the ed2k server's OP_IDCHANGE returns new_id == 0 -- meaning the server gave up on its HighID-callback verification before we could complete it -- the handler silently `break`s out of the switch without ever calling SetConnectionState(CS_CONNECTED). The socket sits in CS_NOTCONNECTED limbo until the 15 s client-side timeout fires and the connection is torn down with no user-visible indication of what happened. This has been the shape since the SmartID retry was added in 2004; nothing recently regressed it, the failure mode just stayed invisible because no one looked. mifritscher2 surfaced it on amule-project#778 while diagnosing a separate race-condition LowID issue. After PR fixing the inbound-server- probe race lands, the new_id == 0 path becomes rarer but still hits when the server has anti-abuse policy or a hard verification timeout that the throttler-bypass doesn't help with. Surface the failure explicitly: log a clear "server rejected our login (no client ID assigned)" line citing the server's name and address, then disconnect via the same teardown path OnError already uses for IO failures (m_bIsDeleting / SetConnectionState CS_DISCONNECTED / serverconnect->DestroySocket). Subsequent auto-connect logic picks the next server immediately rather than waiting out the silent 15 s. The SmartID state-machine bookkeeping (incrementing state, capping at 3, etc.) is preserved before the disconnect so the cross-server retry policy still tracks the rejection. The neighbouring SmartID-retry break at line ~324 has the same fall-into-limbo shape but is intentionally part of the "keep trying servers until one gives HighID" loop and the silent wait is load-bearing for that flow. Left alone here.

mifritscher2 · 2026-05-31T18:11:11Z

Problem: With #787 applied, I have an hard time reproducing the problem this PR is trying to fix (which is good ;) ^^
But I have at least no regression with this change applied.

got3nks · 2026-05-31T18:13:16Z

Problem: With #787 applied, I have an hard time reproducing the problem this PR is trying to fix (which is good ;) ^^ But I have at least no regression with this change applied.

You could try without #787 if you got some spare time. :)

…ndex Adds 55+ merged PRs to the 3.0.0 changelog since the last update (amule-project#747, 2026-05-27). Narrative additions cover: - Packaging: expanded the top list to include the macOS per-arch .app bundles and the Windows NSIS installer alongside the existing AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785 (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796 (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths). - Bug Fixes & Stability: post-amule-project#744 fixes including EC notification leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728), amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760), PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788, amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756), Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833, amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774). - Translations: late-cycle wave covering French/Turkish manpages (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for date+version drift (amule-project#802). - Contributors: added ngosang for UX feedback on the late-3.0 cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the user-facing manual at amule-org.github.io. - Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.

Adds 55+ merged PRs to the 3.0.0 changelog since the last update (#747, 2026-05-27). Narrative additions cover: - Packaging: expanded the top list to include the macOS per-arch .app bundles and the Windows NSIS installer alongside the existing AppImage / Flatpak / .dmg / .zip entries. New bullets for #785 (alc/alcc/cas/wxcas everywhere + Windows amuleweb), #794 (.dmg amuleweb path), #789 (<OS>-<arch> artifact naming), #780 / #796 (Windows DPI + comctl32 manifest), #784 (FHS share/amule paths). - Bug Fixes & Stability: post-#744 fixes including EC notification leak (#797), big-library scaling (#736, #840 superseding #728), amulegui ghost entries (#810, #819, #841, #824, #830, #760), PartFile early hash (#762), server protocol fixes (#835, #788, #721, #787), crypto stream UB (#779), UAF prevention (#756), Kad rotation (#795, #799/#805), GTK warning silencing (#833, #826/#836), and the clang-tidy worklist (#770, #772-#774). - Translations: late-cycle wave covering French/Turkish manpages (#753/#754/#776), Galician (#763), Slovenian (#771), pt-BR (#768/#775/#812), French (#811), plus man-page tooling for date+version drift (#802). - Contributors: added ngosang for UX feedback on the late-3.0 cycle (#817/#818/#821/#828/#844) and ongoing work on the user-facing manual at amule-org.github.io. - Merged PRs flat index: extended with #746-#845 + #841.

The tray offered fifths of the configured line capacity, whose defaults were 300 and 100 kB/s. On any current connection the top entry was already a heavy throttle, which is what amule-project#788 reports. Two changes, and the first helps everyone without touching a config. The ladder is no longer even. Fifths span a single factor of five, so on a fast line every entry lands high and there is no way to throttle hard from the tray, while on a slow one they bunch near the top. Dividing by 1, 2, 4, 10 and 50 instead covers full speed down to a heavy throttle from the same capacity -- at the old default of 300 that is 300/150/75/30/6 rather than 300/240/180/120/60, better reach downward with the same ceiling. The capacity defaults move to 100/20 Mbit, converted at 1024. They are not limits: capacity is what the line can do, and the traffic graph scales from it too. Only new configurations see this -- aMule writes every key on save, so an existing amule.conf already carries the old value and wxConfig applies a default only when the key is absent. That is also why the ladder change matters on its own. Deriving the presets from the configured limit instead was considered and rejected: the menu could then only ever lower it, since every entry would be at or below the cap in force, and the shipped default of unlimited leaves nothing to scale from at all.

… defaults (amule-project#790) * refactor(gui): build the tray speed presets in one place The ladder of limit presets was written out three times -- once in the appindicator backend's submenu builder and once each for upload and download in the wx backend -- and the two backends had drifted. The appindicator one passes its labels straight to GTK, so "Unlimited" and "kB/s" were never translated there, while the wx one ran both through _(). One of them also formatted an unsigned with %d. One helper produces the speeds and one produces the labels, so both backends show the same strings in the user's language. The clamp moves into the helper with them. The appindicator backend was clamping in its two callers and then handing the result to a builder that has to cope with any value anyway; there is now a single place that decides what an unusable capacity turns into. No change to the numbers offered. Raising the stale defaults they are derived from is a separate question. * fix(gui): spread the tray limit presets, and raise the stale capacity The tray offered fifths of the configured line capacity, whose defaults were 300 and 100 kB/s. On any current connection the top entry was already a heavy throttle, which is what amule-project#788 reports. Two changes, and the first helps everyone without touching a config. The ladder is no longer even. Fifths span a single factor of five, so on a fast line every entry lands high and there is no way to throttle hard from the tray, while on a slow one they bunch near the top. Dividing by 1, 2, 4, 10 and 50 instead covers full speed down to a heavy throttle from the same capacity -- at the old default of 300 that is 300/150/75/30/6 rather than 300/240/180/120/60, better reach downward with the same ceiling. The capacity defaults move to 100/20 Mbit, converted at 1024. They are not limits: capacity is what the line can do, and the traffic graph scales from it too. Only new configurations see this -- aMule writes every key on save, so an existing amule.conf already carries the old value and wxConfig applies a default only when the key is absent. That is also why the ladder change matters on its own. Deriving the presets from the configured limit instead was considered and rejected: the menu could then only ever lower it, since every entry would be at or below the cap in force, and the shipped default of unlimited leaves nothing to scale from at all. * chore(i18n): regenerate catalogs after the tray menu refactor

got3nks mentioned this pull request May 31, 2026

assert on EncryptedStreamSocket.cpp(437): assert "m_nReceiveBytesWanted > 0" failed in Negotiate(). #778

Closed

mrjimenez merged commit afcc7cd into amule-project:master May 31, 2026
7 checks passed

got3nks deleted the fix/server-no-id-explicit-disconnect branch June 3, 2026 14:16

got3nks mentioned this pull request Jun 4, 2026

pre-release(3.0.0): refresh CHANGELOG with post-#747 wave + regenerate .po translations #846

Merged

1 task

Navigation Menu

Uh oh!

ServerSocket: disconnect explicitly when server assigns no client ID - #788

ServerSocket: disconnect explicitly when server assigns no client ID#788
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/server-no-id-explicit-disconnect

got3nks commented May 31, 2026 •

edited

Loading

Uh oh!

mifritscher2 commented May 31, 2026

Uh oh!

got3nks commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

got3nks commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Out of scope

Test plan

Uh oh!

mifritscher2 commented May 31, 2026

Uh oh!

got3nks commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

got3nks commented May 31, 2026 •

edited

Loading