[Free-Guy-IR]

Fixes #128

Found by code review and reproduced in a lab; never seen in production. Rebased onto dev 689f266; git merge-tree onto the current dev 9ccd263 (#124) is clean, and the tests below pass on that merge result too.

Keeps the health-check restart's syncMu serialization added in #113. SyncUser and UpdateUsers run their core API calls under a cancellable context registered on the backend (beginSync, backend/xray/xray.go:40 at f9a2f8e). Restart() (backend/xray/xray.go:201) and the health-check restart (backend/xray/jobs.go:100) go through lockSyncForRestart (backend/xray/xray.go:58): it marks a restart as pending, cancels the running sync, then takes syncMu. While a restart is pending, a sync that reaches beginSync is cancelled at once, so a second sync queued on syncMu cannot take the lock after the abort and block the restart again.

An aborted sync never reports success. It returns backend.RetryLater(... user sync aborted by a core restart) (backend/xray/user.go:298, :426), which the node answers as gRPC Unavailable or REST 503. Before this, the same abort was answered Internal / 500, and node_bridge_py treats Internal with the prefix failed to update users as a rejection by the backend and splits the batch (grpclib.py:345-348, controller.py:740 at c5632b65). The aborted sync keeps the accounts it was applying in the in-memory config, so the rebuilt core has them.

If the health-check restart had to abort a sync, it no longer skips the restart when the core answers the re-probe (backend/xray/jobs.go:105), so the core and the in-memory config cannot be left disagreeing about the aborted users.

Tests

All local runs: macOS arm64, go1.27.3 -race, real Xray 26.3.27 through XRAY_EXECUTABLE_PATH / XRAY_ASSETS_PATH for the controller tests. The backend/xray tests use a fake core (a gRPC server whose AlterInbound blocks until its context ends). Base = dev 9ccd263 with the new test files copied in; the two xray tests that name errSyncAborted need a one-line test-only declaration of it to compile on the base.

Test	dev 9ccd263	this PR, and merged onto 9ccd263
TestRestartIsNotBlockedByAUserSyncOnAnUnresponsiveCore	FAIL: Restart did not start a new core within 10 s	PASS
TestHealthLoopRestartsAnUnresponsiveCoreWhileAUserSyncIsStuckOnIt	FAIL: the health loop did not replace the core within 60 s	PASS
TestHealthLoopRestartsAnUnresponsiveCoreWithNoUserSyncInFlight (control)	PASS	PASS
TestRestartIsNotBlockedByAUserSyncQueuedBehindAStuckOne	FAIL: Restart did not start a new core within 10 s (the base has no abort, so the first sync still holds syncMu)	PASS
TestHealthLoopReplacesTheCoreAfterAbortingAUserSyncEvenWhenTheCoreAnswersAgain	FAIL at :302: the core was not replaced within 45 s	PASS
TestRestartDuringAUserSyncKeepsTheUserForTheRebuiltCore	FAIL: the restart never returned	PASS
TestRestartDuringAUserUpdateAbortsItAsRetryLater	FAIL: the restart never returned	PASS
TestGRPC_SyncUsersChunkedAbortedByACoreRestartIsUnavailable (real Xray, SIGSTOP)	FAIL: no abort; the stream ended about 20 s after it opened, when the node's keep-alive timeout stopped the backend, and answered Internal	PASS
TestREST_SyncUserAbortedByACoreRestartIsServiceUnavailable (real Xray, SIGSTOP)	FAIL: the test's HTTP client timed out with no answer	PASS

The two controller tests also fail with user.go from d814391, the commit before the last one (abort without RetryLater): gRPC answered Internal (... user sync aborted by a core restart), REST answered 500. Two tests also fail when only their own mechanism is removed: the queued-sync test with 0dfc854's xray.go, user.go and jobs.go (abort, but no pending-restart guard), and the recovered-core test with only the !aborted check removed from jobs.go:105.

Linux CI on this head: the Test Modules run 42406639575 (job test; it tests the merge ref pull/133/merge 72274b47013c, i.e. f9a2f8e merged with dev; linux/amd64, go1.27.3, step TEST_INTEGRATION=true go test ./... -v -p 1 without -race, Xray 26.10.31) passes all nine:

--- PASS: TestRestartIsNotBlockedByAUserSyncOnAnUnresponsiveCore (1.01s)
--- PASS: TestRestartIsNotBlockedByAUserSyncQueuedBehindAStuckOne (1.30s)
--- PASS: TestRestartDuringAUserSyncKeepsTheUserForTheRebuiltCore (1.00s)
--- PASS: TestRestartDuringAUserUpdateAbortsItAsRetryLater (1.00s)
--- PASS: TestHealthLoopRestartsAnUnresponsiveCoreWhileAUserSyncIsStuckOnIt (21.05s)
--- PASS: TestHealthLoopRestartsAnUnresponsiveCoreWithNoUserSyncInFlight (21.05s)
--- PASS: TestHealthLoopReplacesTheCoreAfterAbortingAUserSyncEvenWhenTheCoreAnswersAgain (21.05s)
--- PASS: TestGRPC_SyncUsersChunkedAbortedByACoreRestartIsUnavailable (3.01s)
--- PASS: TestREST_SyncUserAbortedByACoreRestartIsServiceUnavailable (4.03s)

With the real bridge

Real node, real Xray 26.3.27 frozen with SIGSTOP, node_bridge_py c5632b65 calling _sync_isolating_rejections with 8 users and its default internal_timeout of 15 s; the sync starts 40 s after the freeze, so the health loop reaches its 10th failed probe during the sync:

Node	Answer to the bridge	Bridge result
this PR f9a2f8e	Unavailable ... user sync aborted by a core restart after 9.9 s, in the same second as restarting Xray core...; a new xray process was running 2 s later	1 send, not a rejection, all 8 users returned for retry
d814391, the commit before the last one in this PR (abort without RetryLater)	Internal ... user sync aborted by a core restart after 10.0 s	treated as a rejection: batch split, 3 sends (8, then 4 and 4, both accepted by the restarted core)
dev 9ccd263	none; the bridge timed out after 15.0 s, the node logged context canceled per user, and the health loop replaced the core 3 s later, after its re-probe	1 send, not a rejection, all 8 users returned for retry

The rows for this PR and for dev were each run twice with the same result.

Known limits

• With the bridge's 15 s timeout, the abort answer should reach the bridge only for a sync that started in the last 15 s before the 10th failed probe; an older sync is ended by the bridge's own timeout first, as on dev (derived from the timings above; only a sync started 40 s after the freeze was run).
• If the health loop aborted a sync, it now restarts the core even when the core answers the re-probe. A core that was only slow is restarted.
• ApplyConfigLive (backend/xray/live.go:51) still holds syncMu with the caller's context and does not register with beginSync (read from the code).
• In a gRPC SyncUser stream, the abort ends the stream, so users sent later on the same stream are not applied; the caller has to resend them (read from the code).

[coderabbitai]

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: ab58faec-dc49-46f8-832b-19ecb659d0a0

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

• X
• Mastodon
• Reddit
• LinkedIn