Repository navigation
fix(xray): bound the xray version probe with a timeout - #132
Open
Free-Guy-IR wants to merge 3 commits into
Open
Free-Guy-IR wants to merge 3 commits into
Free-Guy-IR wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
…lemt probe A CPU-starved xray version took up to 5.6 s on a fast machine, and a probe that needed 12 s made Start fail where it used to succeed. Use the same 30 s bound as telemtCommandTimeout and add a test that a probe answering after 11 s is still accepted.
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.
Fixes #127
Found by code review; never seen in production. Head
a927325e8662, based on main v0.5.7739eab94a934;git merge-tree --write-treeonto dev9ccd2638c7feis clean.What
refreshVersion(backend/xray/core.go:94-99at dev9ccd2638) ranxray versionwith no context, timeout orWaitDelay. This PR:exec.CommandContextunderversionProbeTimeout= 30 s, the same bound astelemtCommandTimeout(backend/mtproto/process.go:29on dev), and returnsxray version probe did not finish within 30swhen it does not finish;cmd.WaitDelay = time.Second, so a child that holds stdout cannot keepRunwaiting; a version printed before that is still accepted (exec.ErrWaitDelayis ignored).Why the scope is wider than the core
StartBackendholdsc.muwhile it builds the core (controller/controller.go:281-283on dev), and every REST request takesc.muin middleware (controller/rest/middleware.go:20,70). Real node, REST, macOS arm64, executable = a shell script that stands in for a hung binary (exec sleep 600onversion, real Xray 26.3.27 otherwise):9ccd2638Startxray version probe did not finish within 30s: context deadline exceededGetBaseInfo/GetSystemStats/Stopsent 2 s into Startstarted=false; 500backend not initialized; 500backend not initialized)sleepafter the runControl: with the real Xray 26.3.27 both builds start in about 2 s (
started=true,core_version=26.3.27).The panel's Start for Xray goes through node_bridge_py with the node's
default_timeout(10 s by default; panel devf92acaf51cecapp/operation/node.py:427-439, bridgec5632b65a10arest.py:174), so the panel still gives up at 10 s; what changes is that the node answers again after 30 s instead of never. This part was found by reading, not run against a real panel.Tests
backend/xray/core_version_probe_unix_test.go(four tests). go1.27.3,-race, macOS arm64.On dev
9ccd2638with the test file and one linevar versionProbeTimeout = 30 * time.Secondadded so it compiles (refreshVersionunchanged):On the previous head
c2ecd81c(10 s bound) with the same test file:On this PR merged onto dev: all four pass, and
TestCoreStarted*still pass (ok backend/xray 19.801s).Whole module,
go test -race -count=1 -p 1 ./..., withXRAY_EXECUTABLE_PATHset to the real Xray 26.3.27 and a self-signed cert incerts/(needed bycontroller/restandcontroller/rpc):backend/xrayfail, for the environment only: five run/usr/local/bin/xray, which is hard-coded and missing here (TestConvertLivePartsUsesTheInstalledXray,TestApplyConfigLiveKeepsOpenConnections,TestUserSyncKeepsAnUnchangedUsersOpenConnection,TestXrayBackend,TestGetOutboundsLatencyWithRealXray), and two fail withbind: invalid argumentbecause the macOS temp path is too long for a unix socket (TestRemoveUnixSocketFilesOnlyRemovesSockets,TestStopRemovesTrackedUnixSocketsAfterProcessExit).controller/rpcfailed once at load average 16-17 (TestMain's 5 s Start deadline:Failed to start backend: rpc error: code = DeadlineExceeded) and passed when re-run alone right after.controller(includingTestDisconnectAbortsAStartInProgress) passed on both.CI
This PR's Test Modules run 42404182300 (it tests the merge ref
pull/132/merge92761dc3b5b5) passedTEST_INTEGRATION=true go test ./... -v -p 1, including this PR's four tests (TestVersionProbeOfAHungBinaryIsBounded,TestVersionProbeSurvivesABinaryThatForksAndHoldsStdout,TestVersionProbeOfAWorkingBinaryStillParses,TestVersionProbeWaitsForASlowButHealthyBinary), and the-raceTestCoreStartedstep. It then failed insudo -E make test-integration-wireguard:TestIntegrationE2EStartupUserscould not bindfd10:210:1::1(listen tcp [fd10:210:1::1]:0: bind: cannot assign requested address). This PR changes onlybackend/xray/core.goandbackend/xray/core_version_probe_unix_test.go.Known limits
exec.CommandContextkills only the direct child. A helper that a wrapper script starts in the background is not killed;WaitDelayletsrefreshVersionreturn about 1 s after the wrapper exits, while the helper keeps running (seen in the 2026-11-05 lab; not re-run for this edit).Startinstead of waiting.