Repository navigation
fix(xray): leave unchanged users alone during user sync - #93
ImMohammad20000 merged 2 commits into
Conversation
SyncUser and UpdateUsers removed and re-added every user on every inbound they stay on, even when nothing about the account changed. On a core that closes a user's connections when the user is removed, every such sync would disconnect the user. A user is now removed and added again only when their account on that inbound changed. Users who leave an inbound are removed as before. An account whose add fails is dropped from the local state, so the next sync retries it.
Runs the real core with one VLESS inbound: an open connection must keep flowing after SyncUser and UpdateUsers resend the same user, and a removed user must not open a new connection. The test also logs how much the removed user's already open connection still received.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughAccount synchronization now compares inbound accounts by email and deeply equal values. ChangesAccount synchronization
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each account twice, Comment |
…ing Xray, sing-box, sing-quic and mtg The node section now fetches every PasarGuard/node branch and tag into refs/upstream-drift/node/*, not only main and dev. New sections watch XTLS/Xray-core, SagerNet/sing-box (oldstable line), SagerNet/sing-quic and 9seconds/mtg. Each pin is read from the checked commit: the Dockerfile XRAY_COMMIT and SINGBOX_VERSION, the sing-quic require line in the sing-box fork go.mod at SINGBOX_COMMIT, and the node go.mod mtg require checked against its replace commit. For each upstream, the check fails on any of these that is neither ported nor waived: - a published GHSA of the repository or of its Go module; - a security-looking commit on any branch active since the pin; - a release tag after the pin. Ports are recognised by an Upstream line in the checked history or in a patches/*.patch header. Advisories also count as ported through the ported upstream commit that names them. The waivers are seeded with a reason for every item open today. Four items are left unwaived on purpose: e5a9fb7 and 8f15190 (Xray QUIC sniffer) and 52d5aab (grpc 1.84.0) are being ported in 0.6.21 with Upstream trailers, and be2c445 (PasarGuard#93) is awaiting a decision.
…ift rules - PasarGuard#93 (be2c445) is ported by 40006fe, and PasarGuard#90 (4a9d0cf) by f9c4f05, merged into release/v0.6.21 at 01eed71. Both rows and the node state now say so, and the upstream dev head in the branch table is 4a9d0cf. - The drift-check section describes the two new item kinds: fork commits with an upstream parent, which must be allowlisted as allowed-merge (the ours-merge hole), and upstream merges whose combined diff is not empty, which need a port or a waiver. - The waiver section describes the allowed-merge category and the until:/issue: requirement for pending waivers, what happens after the date, and DRIFT_TODAY.
Summary
SyncUserandUpdateUsersused to remove a user from every inbound they stay on and then add them again. They did this on every call, even when the account had not changed. With this PR, the node sends that remove and add only when the user's account on that inbound changed. Users who leave an inbound are removed exactly as before.Refs #92.
Why this matters now. The node takes access away with Xray's
RemoveUserOperation. A removed user keeps the connections they already have open (#92). XTLS/Xray-core#5844 makesRemoveUserclose those connections. With that core, the current remove and add would disconnect a user every time the panel sends them again without changes, for example after an admin edits their data limit or expiry date. With this PR, only two kinds of user have their connections closed: users who lose access to an inbound, and users whose credentials changed.On today's stock core users see no difference, and the node sends fewer gRPC calls. When a user who is on N inbounds is synced without changes, the node now sends 2N fewer calls.
Details
Inbound.hasAccountandInbound.changedAccountscompare the account the node is about to send with the one it stored for that inbound. They usereflect.DeepEqual.syncUsers,SyncUserandUpdateUsersbuild the stored account with the same constructors, including the Shadowsocks 2022 key normalisation, so a user loaded at startup counts as unchanged too.SyncUsersandUpdateUsersAndRestartare unchanged, since they restart the core with the full user list.Type of change
Checklist
Testing
New tests
backend/xray/user_sync_test.go(7 tests) runsSyncUserandUpdateUsersagainst a recording gRPCHandlerService. The config has one inbound for each protocol: vless, vmess, trojan, shadowsocks, shadowsocks 2022 and hysteria. The tests check exactly which operations reach Xray:backend/xray/user_session_test.goruns the real core with a VLESS TCP inbound and a minimal VLESS client:SyncUser, and thenUpdateUsers, send the same user again.Fails before, passes after
Go 1.26.3, linux/amd64, in an Alpine container limited to 1.5 CPUs:
dev(d44ca8e)f356a34)open connection stopped after SyncUser sent the same user again: received 0 bytes, err EOF.Full suite
The full suite was run as in
run-tests.yml: Go 1.26.3, the official Xray v26.3.27 in/usr/local/bin/xray, and certificates frommake generate_server_certandmake generate_client_cert.backend/wireguard,backend/xray,backend/xray/api,controller/rest,controller/rpc,pkg/statsandpkg/sysstatsall pass: 162 tests.go vet ./...reports nothing except thecontrollerproblem described in the notes below.Screenshots
Not applicable.
Notes for reviewers
controllertests do not compile ondev. Sinced44ca8e,controller/controller_test.gostill callskeepAliveStalewith three arguments. The "Test Modules" run for that push failed on this. This PR does not touch that file, so the regular test step fails the same way until it is fixed.SyncUserandUpdateUsers, and it also records an account only after a successful add. Whichever PR lands second needs a small rebase. The two changes do not conflict in behaviour.-race,TestUserSyncKeepsAnUnchangedUsersOpenConnectionreports the existing race betweenCore.Startedandcmd.Wait(core.go:120andcore.go:257).TestXrayBackendreports the same race on unmodifieddev, and fix(xray): synchronize process status checks #86 fixes it. The new recording tests are race-free.127.0.0.1in the freedom outbound'sfinalRules, because Xray 26.6 and later refuse private targets by default. Older cores ignore the field.Summary by CodeRabbit