proto-p10: BX P merge case + silent variant for probe lookups - #56
Draft
MrLenin wants to merge 3 commits into
Draft
proto-p10: BX P merge case + silent variant for probe lookups#56MrLenin wants to merge 3 commits into
MrLenin wants to merge 3 commits into
Conversation
Factor the GetUserN numeric-to-userNode lookup into a static GetUserN_impl(numeric, quiet) helper, expose both the original GetUserN (noisy — keeps WARNING for every failure mode) and a new GetUserN_silent (suppresses all warnings on lookup miss). Flip the BX P handler's two probe lookups (cmd_bouncer_transfer: both old_primary and new_node) to the silent variant. Probing is the whole point — the old client may already be gone, and new_node is normally absent in the swap-path case. Command handlers and protocol parsers (opserv command targets, FAKEHOST/MARK/etc. dispatchers, channel-mode victims) keep using the noisy GetUserN — a miss there is a real protocol bug or config issue worth surfacing. mod-snoop / mod-track stay on the noisy variant too (those modules are off by default and operators who enable them want the misses). Symptom this addresses: noisy networks running the fork's bouncer subsystem were seeing repeated x3 warning: GetUserN(GkAAp) couldn't find user! snotices to O3 channels from the BX P probe path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The original cmd_bouncer_transfer BX P handler logged a warning and bailed when both old_primary and new_node existed locally. Modern fork peers can emit BX P in that exact shape when an N-introduced client is being absorbed into an existing primary on the same account — typically because we received the would-be-alias's N before its BX C in a burst, so both userNodes landed in our tables. Add a merge branch gated on same-handle: if both nodes exist AND share handle_info, delete old_primary's userNode via DelUser(announce=0) — channels, dict entry, oper list, etc. all get cleaned up by the standard DelUser path without a network QUIT/KILL. The surviving identity is new_node. The mismatched-handle case keeps the original "log and ignore" behaviour — we don't silently merge across unrelated accounts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cmd_bouncer_transfer's same-handle merge branch (BX P where new_node already exists with old_primary's handle_info) called DelUser on old_primary without first moving its channel memberships to new_node. Both nefarious implementations of the both-exist case transfer memberships before killing the old node (upstream m_bouncer_transfer.c:88-113, fork bouncer_session.c:7070-7078); X3's merge branch skipped that step. Reachable in steady state via cross-server BOUNCER RESUME: old_primary is a ghost holding all of the session's channels, new_node is a fresh client with none. DelUser's own channel-removal loop calls DelChannelUser(..., NULL, 0), and DelChannelUser's tail destroys an unregistered channel when the removal empties it. With no prior transfer, any unregistered channel where old_primary was the only member from X3's point of view got destroyed mid-merge, even though the surviving identity (and the rest of the network) was still in it. Fix: before DelUser, walk old_primary's channel list the same way DelUser's own loop does (always pop the last element, since Add/DelChannelUser mutate both the channel's member list and the user's channel list). For each channel, add new_node with the same modes/oplevel if it isn't already a member, then remove old_primary with no announce. Doing this before DelUser means old_primary's channel list is already empty when DelUser runs, so its loop body never executes and never risks the auto-destroy path. AddChannelUser unconditionally fires call_join_funcs (chanserv join-time hooks) -- there's no hook-free "just add the membership" primitive, and the numeric-swap path above never touches channel membership at all so there's no hook-free both-exist precedent to follow instead. Accepted as a known side effect since membership state correctness is the priority; it does not emit anything on the wire (irc_join only fires for IsLocal users, i.e. X3's own service bots, never for a bounced network client). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MrLenin
added a commit
to MrLenin/x3
that referenced
this pull request
Jul 31, 2026
cmd_bouncer_transfer's same-handle merge branch (BX P where new_node already exists with old_primary's handle_info) called DelUser on old_primary without first moving its channel memberships to new_node. Both nefarious implementations of the both-exist case transfer memberships before killing the old node (upstream m_bouncer_transfer.c:88-113, fork bouncer_session.c:7070-7078); X3's merge branch skipped that step. Reachable in steady state via cross-server BOUNCER RESUME: old_primary is a ghost holding all of the session's channels, new_node is a fresh client with none. DelUser's own channel-removal loop calls DelChannelUser(..., NULL, 0), and DelChannelUser's tail destroys an unregistered channel when the removal empties it. With no prior transfer, any unregistered channel where old_primary was the only member from X3's point of view got destroyed mid-merge, even though the surviving identity (and the rest of the network) was still in it. Fix: before DelUser, walk old_primary's channel list the same way DelUser's own loop does (always pop the last element, since Add/DelChannelUser mutate both the channel's member list and the user's channel list). For each channel, add new_node with the same modes/oplevel if it isn't already a member, then remove old_primary with no announce. Doing this before DelUser means old_primary's channel list is already empty when DelUser runs, so its loop body never executes and never risks the auto-destroy path. AddChannelUser unconditionally fires call_join_funcs (chanserv join-time hooks) -- there's no hook-free "just add the membership" primitive, and the numeric-swap path above never touches channel membership at all so there's no hook-free both-exist precedent to follow instead. Accepted as a known side effect since membership state correctness is the priority; it does not emit anything on the wire (irc_join only fires for IsLocal users, i.e. X3's own service bots, never for a bounced network client). Applies the PR evilnet#56 review fix (fix/bx-p-merge-and-silent-probes) on this stacked branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MrLenin
added a commit
to MrLenin/x3
that referenced
this pull request
Aug 1, 2026
cmd_bouncer_transfer's same-handle merge branch (BX P where new_node already exists with old_primary's handle_info) called DelUser on old_primary without first moving its channel memberships to new_node. Both nefarious implementations of the both-exist case transfer memberships before killing the old node (upstream m_bouncer_transfer.c:88-113, fork bouncer_session.c:7070-7078); X3's merge branch skipped that step. Reachable in steady state via cross-server BOUNCER RESUME: old_primary is a ghost holding all of the session's channels, new_node is a fresh client with none. DelUser's own channel-removal loop calls DelChannelUser(..., NULL, 0), and DelChannelUser's tail destroys an unregistered channel when the removal empties it. With no prior transfer, any unregistered channel where old_primary was the only member from X3's point of view got destroyed mid-merge, even though the surviving identity (and the rest of the network) was still in it. Fix: before DelUser, walk old_primary's channel list the same way DelUser's own loop does (always pop the last element, since Add/DelChannelUser mutate both the channel's member list and the user's channel list). For each channel, add new_node with the same modes/oplevel if it isn't already a member, then remove old_primary with no announce. Doing this before DelUser means old_primary's channel list is already empty when DelUser runs, so its loop body never executes and never risks the auto-destroy path. AddChannelUser unconditionally fires call_join_funcs (chanserv join-time hooks) -- there's no hook-free "just add the membership" primitive, and the numeric-swap path above never touches channel membership at all so there's no hook-free both-exist precedent to follow instead. Accepted as a known side effect since membership state correctness is the priority; it does not emit anything on the wire (irc_join only fires for IsLocal users, i.e. X3's own service bots, never for a bounced network client). Applies the PR evilnet#56 review fix (fix/bx-p-merge-and-silent-probes) on this stacked branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MrLenin
marked this pull request as draft
August 1, 2026 07:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related fixes for cmd_bouncer_transfer's BX P handler in
src/proto-p10.c.1.
GetUserN_silent()variant — stop spamming "couldn't find user!" snotices from BX P probesFactor the existing
GetUserNlookup into a privateGetUserN_impl(numeric, quiet)helper. Expose two variants:GetUserN— original behaviour, every failure mode (numeric too long/short, server not found, user not found at valid slot) logsLOG_WARNING.GetUserN_silent— same lookup, all warnings suppressed on miss.Flip only the two probe lookups in
cmd_bouncer_transfer(old_primaryandnew_node) to the silent variant — the BX P handler is the probe path, so a lookup miss is expected, not bug-worthy. Everything else (mod-snoop / mod-track / opserv command targets / FAKEHOST / MARK / channel-mode victims) keeps the noisyGetUserN.Symptom this addresses: networks running the IRCv3 fork's bouncer subsystem were seeing repeated
snotices to oper channels for every BX P probe miss.
2. BX P merge case for in-place conversion
The original handler logged a warning and bailed when both
old_primaryandnew_nodeexisted locally. Modern fork peers can emit BX P in exactly that shape when an N-introduced client is being absorbed into an existing primary on the same account — typically because burst ordering caused us to receive the would-be-alias'sNbefore itsBX C, so both userNodes landed in our tables.Add a merge branch gated on same-handle:
DelUser(announce=0)suppresses both QUIT and KILL emission — this is internal cleanup, the network shouldn't see the alias's identity leave. The surviving identity isnew_node.The mismatched-handle case keeps the existing "log and ignore" guard — we don't silently merge across unrelated accounts.
Test plan
GetUserN(...) couldn't find user!for the alias/primary lookupsReal-world burst-ordering scenario (modern fork peer sending BX P in merge shape) needs network-level verification on the fork side once both halves of the patch are in.
🤖 Generated with Claude Code