Repository navigation
fix(controller): refresh keep-alive on every authenticated request - #115
Merged
Merged
Conversation
Keep-alive only counted requests that finished without an error, so a failed call (NotFound, "core is not started yet"), a call the panel cancelled on its own timeout, or a slow call still in flight did not count. On a lossy link or with a 50 s poll against a 60 s keep_alive, one such call let the node kill its core and leave it down until the next Start. Refresh it when an authenticated request arrives and again when it ends, whatever the result, in the gRPC unary and stream interceptors and the REST middleware. Requests that fail the API key check still never count.
The upload is closed with an error, so the handler cannot read a body; say so next to the assertion.
|
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 ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Free-Guy-IR
added a commit
to Free-Guy-IR/node
that referenced
this pull request
Nov 3, 2026
…r panel (upstream PasarGuard#115) Upstream PasarGuard/node d309483 (PasarGuard#115, released in v0.5.5) ships the gRPC and REST tests below with its fix. Keep-alive counts only calls that succeed (the gRPC logging interceptors on err == nil, REST on a 2xx), so a failed, cancelled or still-running call from the panel does not count. In production (KG "Incident 2026-10-23 Instagram stalls", node 73) the node restarted a wedged xray itself, the panel's only calls were failing health checks, and the watchdog then tore the core down: about 52 s of extra outage until the panel's next Start. Failing on the parent: - gRPC: TestKeepAliveRefreshedWhenAuthenticatedCallArrives, TestKeepAliveRefreshedWhenBackendGateRejects, TestKeepAliveRefreshedWhenStreamOpensAndEnds; - REST: TestKeepAliveRefreshedWhenAuthenticatedRequestArrives, TestKeepAliveRefreshedForFailedRequests; - fork, gRPC, about 12 s: TestFailingCallsFromTheOwnerPanelKeepItsSession, the owner panel polls GetBackendStats every 300 ms and every call fails; on the parent the watchdog ends its session ("client is \"\" after 12s"). - fork, gRPC, up to 15 s: TestASecondPanelsCallsNeverKeepTheOwnerPanelsSessionAlive. This node binds the core to the client that started it, and a second client's Start is refused until keep-alive ends that session. A second panel's successful GetBaseInfo used to refresh keep-alive, so a silent owner's session never ended and the second panel could never take over. Taking upstream's "every authenticated request" literally would make even the second panel's refused calls keep it alive. TestKeepAliveNotRefreshedWithoutValidKey (gRPC and REST) passes on the parent and pins that an unauthenticated call never counts. Controller.LastRequest is added as the test seam upstream also adds; behaviour is unchanged. Upstream's tests are carried apart from their comments (no explanatory comments in this fork); the gRPC ones get a peer address, which this node's current-client check needs. Upstream's REST TestKeepAliveRefreshedWhenLongRequestEnds calls the new middleware by name and lands with the fix. Upstream: d309483
Free-Guy-IR
added a commit
to Free-Guy-IR/node
that referenced
this pull request
Nov 3, 2026
…l (upstream PasarGuard#115) Port of upstream PasarGuard/node d309483 (PasarGuard#115, released in v0.5.5), reconciled with this fork's ownership of the core. Keep-alive used to count only calls that succeeded, so a panel whose calls were failing, timing out on its side or still running looked gone, and the watchdog tore down the core it was waiting to recover. Like upstream, keep-alive is now refreshed when an authenticated call arrives and again when it ends, whatever the result, in the gRPC unary and stream interceptors and the REST middleware (trackRequest replaces trackSuccessfulRequest); the logging interceptors no longer refresh it, and a call that fails the API key check still never counts. Unlike upstream, only calls from the client that owns the core count (IsCurrentClient, which is true for any client while no one owns it). Upstream has no owner, so "every authenticated request" is enough there. Here the core is bound to the panel that started it, a second panel's Start is refused while that session lives, and keep-alive ending the session is how a second panel takes over. Counting the second panel's calls, even refused ones, would keep a silent owner's session alive for good. Before this change a second panel's successful GetBaseInfo did keep it alive; now it does not. TestASecondPanelsCallsNeverKeepTheOwnerPanelsSessionAlive pins this, and it fails with upstream's unconditional refresh. Upstream's code otherwise, without its comments (no explanatory comments in this fork). Upstream's REST TestKeepAliveRefreshedWhenLongRequestEnds, which calls trackRequest by name, lands here. Upstream: d309483
Free-Guy-IR
added a commit
to Free-Guy-IR/node
that referenced
this pull request
Nov 3, 2026
d309483 (PasarGuard#115) is now ported by c2e15f2 and its waiver goes. 47857d4 (the v0.5.5 version bump) is not applicable: the fork keeps its own NodeVersion line. bb40094 (PasarGuard#111) is superseded while the fork pins Xray v26.3.27 and checks the geo files by sha256. d359a8f (PasarGuard#117) is not applicable without StartOrAttach. bd745b5 (PasarGuard#118) is not applicable until PasarGuard#64 brings Xray WireGuard accounts and goes with it. 1549c96 (PasarGuard#116) and 8ad52fe (PasarGuard#112) stay pending to 2026-11-08 under NODE-LEAD, each with its exact remaining work.
Free-Guy-IR
added a commit
to Free-Guy-IR/node
that referenced
this pull request
Nov 3, 2026
The REST trackRequest refreshes keep-alive only for the panel that owns the core, a fork divergence from upstream PasarGuard#115 (d309483). The gRPC side is pinned by TestASecondPanelsCallsNeverKeepTheOwnerPanelsSessionAlive; nothing pinned the REST side (finding F-V7-V2 / R7B-1): dropping the guard left every REST and controller test green. These two tests, from VERIFY-V055-R7B's probes, fail on that mutant: a second panel's failed, unauthenticated and successful requests must leave the owner's LastRequest unchanged, and an owner request rejected before auth never refreshes it.
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.
Keep-alive only counted requests that finished without an error. These did not count:
With
keep_alive60 and the panel polling about every 50 s, one such call on a lossy link made the node kill Xray. The node then answeredbackend not initializeduntil the panel sent a new Start. This is the "nodes green, users cannot connect" report.Change:
Tests:
controller/rpcandcontroller/rest: failed calls, gated calls, in-flight requests, stream open and close, and unauthenticated requests.keep_alive10 and only NotFound calls for 30 s: dev kills Xray at about 15 s; this branch keeps it.No wire change. Old panels benefit without any change.
Known limitation: a long gRPC stream (
SyncUser,GetLogs) or REST log stream only refreshes keep-alive when it opens and when it ends. One that runs longer thankeep_alive+ 5 s with no other traffic could still let the tracker stop the core. The panel's health check and stats jobs run alongside in practice, and the panel's new per-node heartbeat covers the gap. A per-message refresh is a follow-up:NewRequesttakes the controller'sc.muwrite lock, which a Start holds for its whole duration (known issue N3), so refreshing on every message would stall streams during a Start.