fix(xray): make add_user idempotent instead of aborting on EmailExistsError#43
Open
lArtiquel wants to merge 1 commit into
Open
fix(xray): make add_user idempotent instead of aborting on EmailExistsError#43lArtiquel wants to merge 1 commit into
lArtiquel wants to merge 1 commit into
Conversation
…sError add_user re-raises EmailExistsError, but nothing up the call chain (_add_user / _update_user / SyncUsers / RepopulateUsers) catches it. When the panel opens overlapping streams — e.g. a RepopulateUsers running alongside a SyncUsers, or a reconnect that re-pushes users — a user can be added to an inbound twice. The second add raises EmailExistsError, which propagates out of the stream handler and aborts the whole iteration, so the users after it are never synced. In production this shows up as nodes silently dropping a subset of users after a reconnect until the next full resync. Adding a user that already exists is semantically a no-op, so log at debug and continue rather than raising. TagNotFoundError and OSError handling are unchanged.
|
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.
Problem
XrayBackend.add_userre-raisesEmailExistsError:Nothing up the call chain (
_add_user/_update_user/SyncUsers/RepopulateUsersinservice.py) catches it. When the panel opens overlapping streams — aRepopulateUsersrunning alongside aSyncUsers, or a reconnect that re-pushes the user set — a user can be added to an inbound that already contains it. The duplicate add raisesEmailExistsError, which propagates out of the stream handler and aborts the whole iteration, so every user after the offending one is skipped. In production this shows up as a node silently serving only a subset of its users after a reconnect, until the next clean full resync.Fix
Adding a user who already exists in the inbound is semantically a no-op, so log at debug and continue instead of raising.
TagNotFoundErrorandOSErrorhandling are unchanged.Testing
python -m py_compileclean; no new dependencies.RepopulateUsers) in production across several marznode instances since 2026-05. The periodic post-reconnect user-drops andEmailExistsErrorlog spam disappeared and have not recurred.Note
Best reviewed alongside the companion PR that guards
RepopulateUserswith a lock — that removes the interleaving which produces most of these duplicate adds, while this PR absorbs the residual cases (reconnects, SyncUsers/RepopulateUsers overlap). Each stands on its own, though.@khodedawsh @erfjab — would appreciate a review when you get a chance. Happy to move the handling one level up into
service.pyinstead if you'd prefer it there.