chore(runner): connect tunnel before connecting pb ws #2872
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
This was referenced Sep 5, 2025
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
commit: |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+404
to
+410
| onDisconnected: () => { | ||
| if (!connected) { | ||
| // First connection attempt failed | ||
| reject(new Error("Tunnel connection failed")); | ||
| } | ||
| // If already connected, tunnel will handle reconnection automatically | ||
| }, |
There was a problem hiding this comment.
There's a potential race condition in the tunnel connection handling. If the tunnel disconnects immediately after connecting, the Promise will have already resolved successfully, but the system would be in a disconnected state.
Consider enhancing the connection state management by either:
- Using a timeout for the initial connection attempt
- Adding a state variable that tracks whether the connection is stable
- Implementing a more robust reconnection mechanism that handles the case where disconnection happens shortly after initial connection
This would prevent the code from proceeding with a tunnel that appears connected but actually isn't available for network requests, which seems to be the original issue this PR is trying to solve.
Suggested change
| onDisconnected: () => { | |
| if (!connected) { | |
| // First connection attempt failed | |
| reject(new Error("Tunnel connection failed")); | |
| } | |
| // If already connected, tunnel will handle reconnection automatically | |
| }, | |
| onDisconnected: () => { | |
| if (!connected) { | |
| // First connection attempt failed | |
| reject(new Error("Tunnel connection failed")); | |
| } else if (Date.now() - connectionEstablishedTime < 2000) { | |
| // Connection was established but disconnected within 2 seconds | |
| // This suggests an unstable connection that shouldn't be considered successful | |
| reject(new Error("Tunnel connection unstable - disconnected shortly after connecting")); | |
| } | |
| // If already connected for a significant time, tunnel will handle reconnection automatically | |
| }, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
+384
to
+420
| async #openTunnelAndWait(): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| const url = this.pegboardRelayUrl; | ||
| //console.log("[RUNNER] Opening tunnel to:", url); | ||
| //console.log("[RUNNER] Current runner ID:", this.runnerId || "none"); | ||
| //console.log("[RUNNER] Active actors count:", this.#actors.size); | ||
|
|
||
| let connected = false; | ||
|
|
||
| this.#tunnel = new Tunnel(url); | ||
| this.#tunnel.setCallbacks({ | ||
| fetch: this.#config.fetch, | ||
| websocket: this.#config.websocket, | ||
| onConnected: () => { | ||
| if (!connected) { | ||
| connected = true; | ||
| //console.log("[RUNNER] Tunnel connected"); | ||
| resolve(); | ||
| } | ||
| }, | ||
| onDisconnected: () => { | ||
| if (!connected) { | ||
| // First connection attempt failed | ||
| reject(new Error("Tunnel connection failed")); | ||
| } | ||
| // If already connected, tunnel will handle reconnection automatically | ||
| }, | ||
| }); | ||
| this.#tunnel.start(); | ||
|
|
||
| // Re-register all active actors with the new tunnel | ||
| for (const actorId of this.#actors.keys()) { | ||
| //console.log("[RUNNER] Re-registering actor with tunnel:", actorId); | ||
| this.#tunnel.registerActor(actorId); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation has a potential resource leak when the tunnel connection fails. If the Promise is rejected, the tunnel instance and its callbacks remain active without proper cleanup.
Consider adding cleanup code in the rejection path:
onDisconnected: () => {
if (!connected) {
// First connection attempt failed
this.#tunnel.shutdown();
this.#tunnel = null;
reject(new Error("Tunnel connection failed"));
}
// If already connected, tunnel will handle reconnection automatically
}This ensures the tunnel is properly shut down when the connection fails, preventing resource leaks and avoiding callback execution on a failed connection.
Suggested change
| async #openTunnelAndWait(): Promise<void> { | |
| return new Promise((resolve, reject) => { | |
| const url = this.pegboardRelayUrl; | |
| //console.log("[RUNNER] Opening tunnel to:", url); | |
| //console.log("[RUNNER] Current runner ID:", this.runnerId || "none"); | |
| //console.log("[RUNNER] Active actors count:", this.#actors.size); | |
| let connected = false; | |
| this.#tunnel = new Tunnel(url); | |
| this.#tunnel.setCallbacks({ | |
| fetch: this.#config.fetch, | |
| websocket: this.#config.websocket, | |
| onConnected: () => { | |
| if (!connected) { | |
| connected = true; | |
| //console.log("[RUNNER] Tunnel connected"); | |
| resolve(); | |
| } | |
| }, | |
| onDisconnected: () => { | |
| if (!connected) { | |
| // First connection attempt failed | |
| reject(new Error("Tunnel connection failed")); | |
| } | |
| // If already connected, tunnel will handle reconnection automatically | |
| }, | |
| }); | |
| this.#tunnel.start(); | |
| // Re-register all active actors with the new tunnel | |
| for (const actorId of this.#actors.keys()) { | |
| //console.log("[RUNNER] Re-registering actor with tunnel:", actorId); | |
| this.#tunnel.registerActor(actorId); | |
| } | |
| }); | |
| } | |
| async #openTunnelAndWait(): Promise<void> { | |
| return new Promise((resolve, reject) => { | |
| const url = this.pegboardRelayUrl; | |
| //console.log("[RUNNER] Opening tunnel to:", url); | |
| //console.log("[RUNNER] Current runner ID:", this.runnerId || "none"); | |
| //console.log("[RUNNER] Active actors count:", this.#actors.size); | |
| let connected = false; | |
| this.#tunnel = new Tunnel(url); | |
| this.#tunnel.setCallbacks({ | |
| fetch: this.#config.fetch, | |
| websocket: this.#config.websocket, | |
| onConnected: () => { | |
| if (!connected) { | |
| connected = true; | |
| //console.log("[RUNNER] Tunnel connected"); | |
| resolve(); | |
| } | |
| }, | |
| onDisconnected: () => { | |
| if (!connected) { | |
| // First connection attempt failed | |
| this.#tunnel.shutdown(); | |
| this.#tunnel = null; | |
| reject(new Error("Tunnel connection failed")); | |
| } | |
| // If already connected, tunnel will handle reconnection automatically | |
| }, | |
| }); | |
| this.#tunnel.start(); | |
| // Re-register all active actors with the new tunnel | |
| for (const actorId of this.#actors.keys()) { | |
| //console.log("[RUNNER] Re-registering actor with tunnel:", actorId); | |
| this.#tunnel.registerActor(actorId); | |
| } | |
| }); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This was referenced Sep 5, 2025
NathanFlurry
force-pushed
the
09-04-chore_runner_connect_tunnel_before_connecting_pb_ws
branch
from
cf0534f to
90ce950
Compare
NathanFlurry
force-pushed
the
09-04-chore_ups_remove_no_responders
branch
from
f957fbb to
0678b46
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
graphite-app
bot
changed the base branch from
09-04-chore_ups_remove_no_responders
to
graphite-base/2872
MasterPtato
reviewed
Sep 8, 2025
graphite-app
bot
force-pushed
the
graphite-base/2872
branch
from
0678b46 to
6b4c00b
Compare
graphite-app
bot
force-pushed
the
09-04-chore_runner_connect_tunnel_before_connecting_pb_ws
branch
from
90ce950 to
c140b96
Compare
graphite-app
bot
force-pushed
the
09-04-chore_runner_connect_tunnel_before_connecting_pb_ws
branch
from
c140b96 to
f452a88
Compare
This was referenced Sep 9, 2025
This was referenced Sep 9, 2025
Closed
MasterPtato
approved these changes
Sep 9, 2025
Merge activity
|
graphite-app
bot
deleted the
09-04-chore_runner_connect_tunnel_before_connecting_pb_ws
branch
This was referenced Sep 12, 2025
This was referenced Sep 12, 2025
This was referenced Sep 13, 2025
This was referenced Sep 15, 2025
Merged
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.
No description provided.