Repository navigation
fix(host-service): force tunnel reconnect on inbound silence without waiting for onclose - #6236
andyst-dev wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe tunnel watchdog now uses ChangesTunnel watchdog recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change forces a stalled tunnel to reconnect instead of waiting indefinitely for socket closure and ignores events from the stale connection. No actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/tunnel/tunnel-client.test.ts`:
- Around line 105-116: Update makeClient to expose the TunnelClient.close
method, then change the “forceReconnect is a no-op after close()” test to invoke
close() before forceReconnect instead of directly setting the private closed
flag. Preserve the existing scheduled-reconnect assertion and socket setup.
In `@packages/host-service/src/tunnel/tunnel-client.ts`:
- Around line 417-421: Update the socket event handlers in the tunnel client to
return immediately when the handler’s captured socket no longer matches
this.socket, applying the guard before state updates, message dispatch, or sends
in both onmessage and onopen. Add a regression test covering stale.onmessage
after socket replacement and verify lastInboundAt remains unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a83c99c6-03b1-4cf8-a428-2152549038e0
📒 Files selected for processing (2)
packages/host-service/src/tunnel/tunnel-client.test.tspackages/host-service/src/tunnel/tunnel-client.ts
Greptile SummaryThe PR makes the v1 host tunnel recover from inbound silence without relying on a potentially stuck WebSocket closing handshake.
Confidence Score: 4/5The PR appears safe to merge, with only a non-blocking test-isolation cleanup recommended. The forced-reconnect path consistently orphans the stale socket, clears tunnel-owned state, and schedules one reconnect under existing identity and timer guards; the remaining concern is an unnecessary process-wide WebSocket test stub. Files Needing Attention: packages/host-service/src/tunnel/tunnel-client.test.ts
|
| Filename | Overview |
|---|---|
| packages/host-service/src/tunnel/tunnel-client.ts | Introduces immediate, identity-guarded recovery from a blackholed tunnel while preventing stale onclose events from scheduling twice. |
| packages/host-service/src/tunnel/tunnel-client.test.ts | Covers the intended forced-reconnect behavior, but unnecessarily mutates globalThis.WebSocket during module evaluation. |
Sequence Diagram
sequenceDiagram
participant W as Watchdog
participant C as TunnelClient
participant S as Stale WebSocket
participant R as Reconnect Timer
W->>C: inbound silence timeout
C->>S: close(4002)
C->>C: "socket = null"
C->>C: stop watchdog and cleanup channels
C->>R: scheduleReconnect()
Note over S,C: Late onclose is ignored by socket identity guard
R->>C: connect()
Prompt To Fix All With AI
### Issue 1
packages/host-service/src/tunnel/tunnel-client.test.ts:23-27
**Avoid the unnecessary global stub**
These tests instantiate `FakeWebSocket` directly and never use `globalThis.WebSocket`, so replacing the process-wide implementation during module evaluation creates avoidable cross-test coupling until the first `afterEach` restores it.
```suggestion
const { TunnelClient } = await import("./tunnel-client");
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(host-service): force tunnel reconnec..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Stub global WebSocket with the fake. | ||
| const originalWebSocket = globalThis.WebSocket; | ||
| (globalThis as Record<string, unknown>).WebSocket = FakeWebSocket; | ||
|
|
||
| const { TunnelClient } = await import("./tunnel-client"); |
There was a problem hiding this comment.
These tests instantiate FakeWebSocket directly and never use globalThis.WebSocket, so replacing the process-wide implementation during module evaluation creates avoidable cross-test coupling until the first afterEach restores it.
| // Stub global WebSocket with the fake. | |
| const originalWebSocket = globalThis.WebSocket; | |
| (globalThis as Record<string, unknown>).WebSocket = FakeWebSocket; | |
| const { TunnelClient } = await import("./tunnel-client"); | |
| const { TunnelClient } = await import("./tunnel-client"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/tunnel/tunnel-client.test.ts
Line: 23-27
Comment:
**Avoid the unnecessary global stub**
These tests instantiate `FakeWebSocket` directly and never use `globalThis.WebSocket`, so replacing the process-wide implementation during module evaluation creates avoidable cross-test coupling until the first `afterEach` restores it.
```suggestion
const { TunnelClient } = await import("./tunnel-client");
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/tunnel/tunnel-client.test.ts">
<violation number="1" location="packages/host-service/src/tunnel/tunnel-client.test.ts:29">
P3: The comment here claims the tests drive the watchdog timer manually ("we drive the timer manually by stubbing timers"), but none of the added tests actually start, observe, or stop the watchdog — they all assign `socket` directly and call `forceReconnect(...)` on it. Consequently the `startWatchdog`/`stopWatchdog`/`watchdogTimer`/`connect` members exposed by `makeClient` are unused dead scaffolding, and the comment is misleading for future maintainers. Consider either trimming the exposed surface and correcting the comment to reflect that the watchdog path is exercised by calling `forceReconnect` directly, or actually driving the timer if the goal was to test the interval-triggered path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Host as Linux Host
participant Process as host-service Process
participant WS as WebSocket Connection
participant Relay as Relay Server
participant Superset as Superset Control Plane
Note over Host,Superset: Normal Operation (happy path)
Process->>WS: open connection
WS->>Relay: establish WebSocket
Relay->>Superset: host online
loop Watchdog Interval
Process->>Process: check lastInboundAt
alt Inbound traffic received
Process->>WS: reset watchdog timer
else Inbound silence detected
Process->>WS: close(4002, "Inbound silence timeout")
WS->>Relay: closing handshake
Relay-->>WS: close handshake ack
WS-->>Process: onclose event
Process->>Process: scheduleReconnect()
Process->>WS: new connection attempt
end
end
Note over Host,Superset: Failure Mode - Blackholed TCP Path (#6229)
loop Watchdog Interval
Process->>Process: check lastInboundAt
alt Inbound silence detected
Process->>WS: close(4002, "Inbound silence timeout")
Note over WS,Relay: close frame NEVER reaches relay<br/>packets dropped, TCP stuck in ESTAB
WS-->>Process: onclose NEVER fires<br/>client parks in CLOSING forever
Note over Process: No reconnect scheduled<br/>Host appears offline
end
end
Note over Host,Superset: Fixed Flow - forceReconnect()
loop Watchdog Interval
Process->>Process: check lastInboundAt
alt Inbound silence detected
Process->>Process: forceReconnect(socket, 4002, "Inbound silence timeout")
Process->>WS: socket.close(code, reason)
Process->>Process: this.socket = null
Process->>Process: this.connecting = false
Process->>Process: stopWatchdog()
Process->>Process: cleanupChannels()
Process->>Process: scheduleReconnect()
Note over Process: Reconnect scheduled IMMEDIATELY<br/>without waiting for onclose
Process->>WS: new connection attempt
alt Late onclose from stale socket arrives
WS-->>Process: onclose event
Process->>Process: identity guard: this.socket !== socket<br/>→ ignore (no double-schedule)
end
end
end
opt forceReconnect with stale socket already replaced
Note over Process: stale socket from previous watchdog cycle
Process->>Process: forceReconnect(staleSocket, 4002, ...)
Process->>Process: identity guard: this.socket !== staleSocket<br/>→ immediate return (no-op)
Note over Process: Current socket and connection<br/>remain unchanged
end
opt forceReconnect after client close()
Note over Process: client.closed = true
Process->>Process: forceReconnect(socket, 4002, ...)
Process->>Process: this.closed check → immediate return
Note over Process: No action taken
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const { TunnelClient } = await import("./tunnel-client"); | ||
|
|
||
| // Speed up the watchdog for tests: reach into the module via the constants |
There was a problem hiding this comment.
P3: The comment here claims the tests drive the watchdog timer manually ("we drive the timer manually by stubbing timers"), but none of the added tests actually start, observe, or stop the watchdog — they all assign socket directly and call forceReconnect(...) on it. Consequently the startWatchdog/stopWatchdog/watchdogTimer/connect members exposed by makeClient are unused dead scaffolding, and the comment is misleading for future maintainers. Consider either trimming the exposed surface and correcting the comment to reflect that the watchdog path is exercised by calling forceReconnect directly, or actually driving the timer if the goal was to test the interval-triggered path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/tunnel/tunnel-client.test.ts, line 29:
<comment>The comment here claims the tests drive the watchdog timer manually ("we drive the timer manually by stubbing timers"), but none of the added tests actually start, observe, or stop the watchdog — they all assign `socket` directly and call `forceReconnect(...)` on it. Consequently the `startWatchdog`/`stopWatchdog`/`watchdogTimer`/`connect` members exposed by `makeClient` are unused dead scaffolding, and the comment is misleading for future maintainers. Consider either trimming the exposed surface and correcting the comment to reflect that the watchdog path is exercised by calling `forceReconnect` directly, or actually driving the timer if the goal was to test the interval-triggered path.</comment>
<file context>
@@ -0,0 +1,119 @@
+
+const { TunnelClient } = await import("./tunnel-client");
+
+// Speed up the watchdog for tests: reach into the module via the constants
+// is not possible (not exported), so we drive the timer manually by
+// stubbing timers.
</file context>
|
Thanks — all round-2 feedback is addressed in
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks — both points are addressed in
|
|
Thanks — addressed in
|
|
Thanks — this comment is stale: it points at commit
|
3a60b61 to
15fe31a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…waiting for onclose The watchdog detected a silent relay connection and called socket.close(4002). On a blackholed TCP path (packets dropped, not reset) the WebSocket closing handshake never completes, onclose never fires, and the only reconnect path (scheduled from onclose) never ran — the client parked in CLOSING forever while the process stayed alive but the host showed offline (ParadevAI#6229). forceReconnect() now orphans the stale socket (null + stopWatchdog + cleanupChannels) and schedules the reconnect immediately, mirroring the existing connect-timeout path. The onclose identity guard already ignores the stale socket's late close, so no double-scheduling. 3 unit tests with a fake WebSocket whose close() never fires onclose.
forceReconnect nulls this.socket so the stale socket's late onclose is ignored, but onopen/onmessage/onerror could still run on the orphaned socket and touch state (lastInboundAt, watchdog, handleMessage dispatch) or use a replacement socket. Add the identity guard to all three handlers, mirroring the existing onclose guard. Tests: drop the unnecessary globalThis.WebSocket stub (tests instantiate FakeWebSocket directly), drive close() instead of setting closed by hand, and add a regression test that late messages from an orphaned socket are ignored. 4/4 pass.
The late-messages test previously assigned socket state by hand, so onmessage stayed null and the assertion was a no-op that passed regardless of the fix. The fake WebSocket is now registered before the module import (TunnelClient resolves WebSocket at eval time) and the test calls connect() to get the REAL guarded handlers, then verifies a late inbound from an orphaned socket is ignored and that reconnect is scheduled exactly once. scheduleReconnect is stubbed so no real timer leaks; the global stub is restored in afterEach.
connect() leaves a 20s deadline pending on the fake socket (which never opens); closing the client after the assertions marks it closed so the deadline becomes a no-op instead of firing post-test (cubic P2).
15fe31a to
1b873b0
Compare
|
Closing as a duplicate: main already ships this via |
Fixes #6229
Bug
A persistent Linux host repeatedly goes offline in the Superset control plane while the VPS, network, host-service process, and terminals all stay running. The host logs
no inbound traffic for 77s, forcing reconnect— and then never reconnects untilsystemctl --user restart superset-host.Root cause (confirmed by the triage bot and a contributor in the issue thread):
tunnel-client.tsdetects relay silence via a watchdog, but its only recovery mechanism was a gracefulWebSocket.close(4002, …)— and every reconnect after an established connection was scheduled exclusively from theonclosehandler. On a blackholed TCP path (packets dropped rather than reset — the stuckESTABwith a ~39 KB unacked send queue seen in the report), the close frame can never be flushed, the closing handshake never completes,onclosenever fires, andscheduleReconnect()is never called. The client parks inCLOSINGforever.Fix
forceReconnect(socket, code, reason)intunnel-client.ts:this.socket = null,connecting = false,stopWatchdog(),cleanupChannels()— and callsscheduleReconnect()directly, without waiting foronclose;connect()'s deadline already did close + null + scheduleReconnect);socket.close();onclose(if (this.socket !== socket) return) means the stale socket's late close (if it ever arrives) is ignored — no double-scheduling, no teardown of a newer socket.Validation
bun testontunnel-client.test.ts: 3/3 pass using a fakeWebSocketwhoseclose()never firesonclose(the Remote Linux host stays alive but becomes offline after relay WebSocket stalls #6229 failure mode) — forceReconnect schedules the reconnect anyway; stale-socket and post-close no-op cases covered.tsc --noEmiton packages/host-service: 0 errorsNote:
tunnel-client-v2.tsmay share the pattern; this PR scopes to the v1 client the report and triage identified.Summary by cubic
Force reconnects the tunnel on inbound silence and ignores events from orphaned sockets. Previously the watchdog waited for onclose after close(4002); on blackholed TCP, onclose never fired and the client stayed in CLOSING.
Written for commit 1b873b0. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests