fix(relay): join invite claimers to open channels - #4196
Draft
brow wants to merge 1 commit into
Draft
Conversation
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Contributor
Author
|
@builderbot review |
Contributor
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
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
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.
What changed?
Accepting a community invite previously granted relay-level membership only. The claimer joined the community but was not a member of any channel, so clients that derive their channel list from per-channel membership showed an empty community. This adds channel membership to the claim path: on a genuine new join, the claimer is added to the community's active open, non-DM channels, and NIP-29 discovery is republished for each so kind 39002 advertises the new membership. Kind 39002 is the event mobile's channel list is built from, so the discovery republish is the load-bearing half, not the database write alone.
Idempotent reclaims (
AlreadyMember) run no channel-join side effects, and neither do exhausted, expired, invalid, policy-rejected, or rate-limited outcomes.This covers future claimers. Users who already claimed before this change are in
relay_membersand getalready_memberwith no membership write, so they are not healed by this PR. #4199 covers that population with client-side open-channel discovery. The two together cover both.Why?
Reported on Buzz mobile: accepting a community invite added the community to the list, but no channels or messages ever loaded. The claim endpoint wrote
relay_membersand emitted relay-level NIP-43 events, and never calledadd_memberfor any channel.Relay membership is already durable before these side effects run. A channel membership or discovery failure is therefore logged at
warnand does not turn a successful claim into an error, matching the existing NIP-43 partial-failure handling in this flow. That is a deliberate availability tradeoff with a known cost: a partial failure returns 200joinedwith an incomplete channel list, and because a retry becomesalready_memberit does not self-heal. No test covers that path.How is it tested?
The new assertions are
#[ignore = "requires Postgres"], so a plaincargo test -p buzz-relaydoes not execute them. They run in CI underBackend Integration (relay e2e), stepInvite security tests, which selects the invite module with--run-ignored ignored-only(18 of 18 passing at this head).Added tests, in
api::invites::tests:AlreadyMemberreclaim adds nothing.Mutation results, per call site rather than in aggregate:
emit_group_discovery_eventsfails the kind 39002 assertion. Forcing side effects onAlreadyMember, and removing the archived guard, each fail their assertions.Claims that hold by code reading but have no test behind them at this head, so a future regression would not be caught by CI:
breakafter the first join also passes. "Every" is not test-enforced.invalidate_membership; removing that call leaves the suite green.channel_type == "dm"guard; removing it leaves the suite green. It is not dead code:create_dmhardcodesvisibility='private', but the kind 9002 metadata-edit path changes visibility with no channel-type guard, so an open DM is reachable.add_memberrejecting private admission wheninvited_byisNone. Removing theSome("open")visibility filter still passes, so the list-level filter is not proven load-bearing.Full package suite at this head: 835 passed, 37 ignored, with
api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echofailing (504 instead of 200). That failure reproduces identically at the base commit and is additionally flaky, so it is pre-existing and not from this branch.cargo clippy -p buzz-relay --all-targets -- -D warningspasses.The uncovered-claim list and the per-call-site mutation split come from two independent blind reviews of this exact head, which converged on the
Some("open")finding separately.