Skip to content

fix(xray): bound the xray version probe with a timeout - #132

Open
Free-Guy-IR wants to merge 3 commits into
PasarGuard:devfrom
Free-Guy-IR:upn/version-probe-w
Open

Free-Guy-IR wants to merge 3 commits into
PasarGuard:devfrom
Free-Guy-IR:upn/version-probe-w

Conversation

@Free-Guy-IR

@Free-Guy-IR Free-Guy-IR commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #127

Found by code review; never seen in production. Head a927325e8662, based on main v0.5.7 739eab94a934; git merge-tree --write-tree onto dev 9ccd2638c7fe is clean.

What

refreshVersion (backend/xray/core.go:94-99 at dev 9ccd2638) ran xray version with no context, timeout or WaitDelay. This PR:

  • runs it with exec.CommandContext under versionProbeTimeout = 30 s, the same bound as telemtCommandTimeout (backend/mtproto/process.go:29 on dev), and returns xray version probe did not finish within 30s when it does not finish;
  • sets cmd.WaitDelay = time.Second, so a child that holds stdout cannot keep Run waiting; a version printed before that is still accepted (exec.ErrWaitDelay is ignored).

Why the scope is wider than the core

StartBackend holds c.mu while it builds the core (controller/controller.go:281-283 on dev), and every REST request takes c.mu in 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 600 on version, real Xray 26.3.27 otherwise):

dev 9ccd2638 this PR merged onto dev
Start no answer in 60 s HTTP 503 after 30.01 s: xray version probe did not finish within 30s: context deadline exceeded
GetBaseInfo / GetSystemStats / Stop sent 2 s into Start no answer in 45 s answered at 30.01 s (200 started=false; 500 backend not initialized; 500 backend not initialized)
the same three afterwards no answer in 5 s answered in under 0.01 s
the stand-in's sleep after the run still running (child of the node) gone

Control: 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 dev f92acaf51cec app/operation/node.py:427-439, bridge c5632b65a10a rest.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 9ccd2638 with the test file and one line var versionProbeTimeout = 30 * time.Second added so it compiles (refreshVersion unchanged):

core_version_probe_unix_test.go:42: NewXRayCore did not return within 5s while the version probe hung
--- FAIL: TestVersionProbeOfAHungBinaryIsBounded (5.00s)
core_version_probe_unix_test.go:49: NewXRayCore did not return within 15s while the version probe hung
--- FAIL: TestVersionProbeSurvivesABinaryThatForksAndHoldsStdout (15.01s)
--- PASS: TestVersionProbeOfAWorkingBinaryStillParses (1.31s)
--- PASS: TestVersionProbeWaitsForASlowButHealthyBinary (12.85s)

On the previous head c2ecd81c (10 s bound) with the same test file:

core_version_probe_unix_test.go:70: a version probe that answers after 11 s must still be accepted, got xray version probe did not finish within 10s: context deadline exceeded
--- FAIL: TestVersionProbeWaitsForASlowButHealthyBinary (11.00s)

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 ./..., with XRAY_EXECUTABLE_PATH set to the real Xray 26.3.27 and a self-signed cert in certs/ (needed by controller/rest and controller/rpc):

  • On dev and on this PR the same seven tests in backend/xray fail, 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 with bind: invalid argument because the macOS temp path is too long for a unix socket (TestRemoveUnixSocketFilesOnlyRemovesSockets, TestStopRemovesTrackedUnixSocketsAfterProcessExit).
  • Every other package passed on dev. On this PR controller/rpc failed 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 (including TestDisconnectAbortsAStartInProgress) passed on both.

CI

This PR's Test Modules run 42404182300 (it tests the merge ref pull/132/merge 92761dc3b5b5) passed TEST_INTEGRATION=true go test ./... -v -p 1, including this PR's four tests (TestVersionProbeOfAHungBinaryIsBounded, TestVersionProbeSurvivesABinaryThatForksAndHoldsStdout, TestVersionProbeOfAWorkingBinaryStillParses, TestVersionProbeWaitsForASlowButHealthyBinary), and the -race TestCoreStarted step. It then failed in sudo -E make test-integration-wireguard: TestIntegrationE2EStartupUsers could not bind fd10:210:1::1 (listen tcp [fd10:210:1::1]:0: bind: cannot assign requested address). This PR changes only backend/xray/core.go and backend/xray/core_version_probe_unix_test.go.

Known limits

  • exec.CommandContext kills only the direct child. A helper that a wrapper script starts in the background is not killed; WaitDelay lets refreshVersion return 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).
  • 30 s is fixed. A probe slower than that now fails Start instead of waiting.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e6940e35-c386-431c-9c43-ff50b250207c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant