Skip to content

fix(network): send Cloud auth and Relay through the configured HTTP proxy - #20894

Open
mmarabel wants to merge 9 commits into
stablyai:mainfrom
mmarabel:mmarabel/19976-main-process-proxy
Open

mmarabel wants to merge 9 commits into
stablyai:mainfrom
mmarabel:mmarabel/19976-main-process-proxy

Conversation

@mmarabel

@mmarabel mmarabel commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

Orca has a Settings → Network → HTTP Proxy setting. It already worked for the embedded browser, but the two things the app itself does with the internet — signing in to Orca Cloud and pairing a phone through Orca Relay — went out over Node's stack and ignored it. On a network where the only exit is a proxy, sign-in died with fetch failed and pairing reported relay_control_not_active. Both now use the configured proxy.

What Changed

  • src/main/network/http-client.ts: new fetchWithConfiguredProxy. It applies the configured policy to the Chromium session (ensureElectronProxyFromEnvironment, the step src/main/jira/authenticated-request.ts already performed) and then sends through the main HTTP client port, which is Electron's net.fetch on the desktop — so Chromium's proxy applies. On a host without Chromium the port's Node fallback resolves the same policy and passes it as undici's per-request dispatcher.
  • src/main/network/outbound-proxy.ts (new): resolves the proxy for one Node-side target through Chromium's session.resolveProxy — Settings, proxy env vars, OS proxy and the bypass list all apply — and falls back to the env policy when there is no session. Returns a CONNECT-tunnelling https-proxy-agent for WebSockets and an undici ProxyAgent for fetch, memoized per proxy URL; the credentials the same settings produced are attached when host and port match. Loopback is never proxied, matching Chromium.
  • HTTP call sites: the Cloud clients (profile-cloud-client.ts, profile-cloud-org-members-client.ts), relay token exchange and assignment (relay-http-client.ts) and the relay region catalog/probes (relay-region-preference.ts) now use fetchWithConfiguredProxy. An injected fetch still wins, so callers and tests that pass their own transport are unaffected.
  • Sockets: relay-control-client.ts passes the resolved agent into the ws constructor, relay-control-origin.ts resolves it per control connection, and relay-transport.ts does the same for cell data sockets — both skipped when a socket factory is injected, which keeps injected factories synchronous exactly as before.
  • electron-proxy-credentials.ts exposes the applied credentials for that agent; the two fetch call-site audit tables are updated.
  • Adds undici and https-proxy-agent as production dependencies: Node's fetch accepts only an undici dispatcher, and ws accepts only an http.Agent.

Why

The reporter extracted app.asar and reproduced it outside the app: the code exchange posts to login.onorca.dev with Node's global fetch, so it leaves without the configured proxy and times out (UND_ERR_CONNECT_TIMEOUT), and the relay control channel is a raw WebSocket with no proxy support at all. Sending app-owned HTTP through the main HTTP client port is the pattern already used for Jira, so this removes the asymmetry instead of adding a second proxy mechanism.

Fixes #19976

Visual Proof

N/A — no UI or interaction change. The difference is whether sign-in and Relay pairing succeed behind a proxy.

Testing

  • Three new assertions fail before the change and pass after it: the Cloud auth-code exchange goes through the main client (not the global), relay HTTP uses it by default, and the resolved agent reaches the ws constructor.
  • relay-transport.test.ts gains a case with a real local CONNECT proxy: the cell data socket is tunnelled to relay-cell.example:80, so the data plane is proven rather than only the option plumbing.
  • src/main/network/outbound-proxy.test.ts (new, 8 cases) covers the resolver: proxy rule, dispatcher, DIRECT, loopback bypass, unsupported rule, unusable rule, credentials, env fallback.
  • Red first: with only the source changes stashed, the three new assertions fail (3 failed | 42 passed).
  • Commands run on Linux (headless): pnpm tc clean; the 10 touched test files 107 passed; the src/main/** relay/network/profiles/jira/rpc/startup sweep 3516 passed; pnpm run check:code-quality:changed clean (ORCA_CODE_QUALITY_BASE=HEAD~1, because the gate otherwise compares against a main ref that is hundreds of commits behind this worktree's base and reports pre-existing findings from the whole delta).

Platform testing: Linux only. I did not run this against a real corporate proxy on macOS or Windows; the proof here is unit tests plus a local CONNECT proxy. The renderer/Chromium side is untouched, and the changed path is net.fetch plus session.resolveProxy, both platform-neutral.

Known limits, unchanged from before (not regressions): HTTPS-proxy and SOCKS* rules still resolve direct — they need an agent this change does not add — and loopback stays direct by design.

AI Disclosure

Written with an AI coding agent (deepseek-flash via opencode-go). All commands in Testing were run by the agent on Linux.

Review

AI-assisted code review summary, per CONTRIBUTING:

  • Cross-platform: no platform branches added. The Chromium-less path still calls globalThis.fetch, only adding a dispatcher when a proxy resolves, so macOS, Windows and Linux take the same code path.
  • SSH/remote/local: untouched. The Cloud and Relay clients run in the local main process; execution-host ownership and the SSH execution boundary are not involved.
  • Agents and integrations: no provider or agent path changes. The port is the existing main-process HTTP port already used by Jira; the Claude/Codex usage fetchers are untouched.
  • Mobile: the relay control and cell data channels are the mobile pairing path, so pairing gains the proxy with no wire-protocol change. Injected socket factories keep their previous behaviour and timing.
  • Performance: one resolveProxy call per relay socket plus memoized agents per proxy URL; HTTP calls add the same memoized proxy-ensure step Jira already performed.
  • UI quality: no UI change.
  • Security: the proxy URL and credentials come from the settings/session state Chromium already uses and are only attached when host and port match; an unsupported or unusable rule degrades to direct rather than to a wrong destination; nothing new is logged.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

Review rounds after the first commit: 6a0669535c (proxy credentials on the Chromium-less path, the stop() window while a proxy resolves) and 7e7b417203 (ordered resolver directives, the whole 127.0.0.0/8 loopback bypass, NO_PROXY on the Chromium-less path, Request inputs forwarded intact, duplicate open for one connId).

Other main-process fetch call sites still leave through globalThis.fetch and therefore still ignore the app proxy: main/artifacts/artifact-cloud-request.ts, main/azure-devops/azure-devops-api-request.ts, main/bitbucket/client.ts, main/bitbucket/user-request.ts, main/gitea/client.ts, main/rate-limits/codex-fetcher.ts, main/runtime/push/push-gateway-client.ts, main/source-control/hosted-review-api-request.ts and main/speech/openai-transcription-client.ts. They stay in AUDITED_GLOBAL_FETCH_LINES on purpose: this PR is scoped to the Cloud auth and Relay paths #19976 reports, and fetchWithConfiguredProxy is the drop-in for them in a follow-up.

Two production dependencies are added and justified above; pnpm-lock.yaml changes are limited to their importer entries. Injected fetch/createSocket still take precedence, so no caller or test loses control of its transport.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…roxy

Main-process Cloud and Relay traffic escaped Settings → Network → HTTP Proxy:
the Cloud clients used bare fetch, the relay token/assign calls fell back to
globalThis.fetch, and the relay control and cell data WebSockets dialled
directly. On a proxy-only network the desktop then failed the sign-in exchange
with UND_ERR_CONNECT_TIMEOUT and Relay pairing with relay_control_not_active.

- Cloud and Relay HTTP now go through the main HTTP client port after applying
  the configured policy to the Chromium session, so the desktop sends them
  through net.fetch and the app's proxy — the pattern Jira already uses. The
  relay region catalog and probes inherit the same default.
- Node-side relay sockets resolve the same policy (Chromium's resolver, with
  the proxy environment as the fallback on a host without a session) and
  tunnel through an HTTPS-CONNECT agent; the Chromium-less fetch fallback gets
  the equivalent undici dispatcher. Settings still win over the environment,
  loopback stays direct, and rules that cannot be tunnelled (HTTPS-proxy,
  SOCKS) stay direct as they already were.
- Adds undici and https-proxy-agent as dependencies: Node's fetch accepts only
  an undici dispatcher, and ws accepts only an http.Agent.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The Chromium-less fallback this PR adds drops proxy credentials that came from the environment, so an authenticated HTTPS_PROXY/HTTP_PROXY yields an agent/dispatcher with no Proxy-Authorization and every proxied WebSocket or fetch on that host fails with 407. Detail inline.

Reviewed changes

  • Proxy-aware fetch wrapper — src/main/network/http-client.ts adds fetchWithConfiguredProxy, which applies the configured policy to the Chromium session and then sends through the main HTTP client port, with an undici dispatcher on the Chromium-less Node fallback.
  • New proxy resolver — src/main/network/outbound-proxy.ts resolves the proxy per target through session.resolveProxy (or the env policy when there is no session), attaches session credentials on host/port match, bypasses loopback, and memoizes a CONNECT HttpsProxyAgent for sockets and an undici ProxyAgent for fetch per proxy URL.
  • HTTP call-site migration — the Cloud clients, relay token/assignment HTTP, and the relay region catalog/probes now default to fetchWithConfiguredProxy; an injected fetch still wins.
  • WebSocket migration — relay-control-client.ts, relay-control-origin.ts, and relay-transport.ts pass the resolved agent into the ws constructor, skipped when a socket factory is injected.
  • Credentials + audits + deps — getElectronProxyCredentialsForSession is exposed; the two fetch audit tables are updated; undici and https-proxy-agent are added as production dependencies.

ℹ️ Proxy coverage stays partial

This removes the asymmetry for Cloud auth and Relay, but several other app-owned outbound paths still leave through globalThis.fetch and therefore still ignore Settings → Network → HTTP Proxy: main/artifacts/artifact-cloud-request.ts, main/azure-devops/azure-devops-api-request.ts, main/bitbucket/client.ts, main/bitbucket/user-request.ts, main/gitea/client.ts, main/rate-limits/codex-fetcher.ts, main/runtime/push/push-gateway-client.ts, main/source-control/hosted-review-api-request.ts, and main/speech/openai-transcription-client.ts are all still listed in AUDITED_GLOBAL_FETCH_LINES. The reporter's failure mode (fetch failed / relay_control_not_active behind a proxy-only network) applies equally to these, so this is only a partial fix for the underlying class. Not a blocker for this PR — flagging so the remaining set is a conscious follow-up rather than an oversight.

Technical details
# Remaining proxy-bypassing call sites

## Context
`fetchWithConfiguredProxy` is the only path that consults the app proxy for
app-owned HTTP. Every other file still in `AUDITED_GLOBAL_FETCH_LINES`
(`src/main/global-fetch-call-site-audit.test.ts:16-48`) sends over Node's
`globalThis.fetch`, which does not read the app's configured proxy.

## Open questions for the human
- Is a follow-up planned to migrate the rest, or is each of these intentionally
  out of scope (e.g. already using `net.fetch` on a partition)?

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/network/outbound-proxy.test.ts Resolved
Comment thread src/main/runtime/rpc/relay-transport.ts Resolved
@coderabbitai

coderabbitai Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds outbound proxy resolution from Chromium proxy sessions and environment variables. Routes Cloud, Relay HTTP, and region-probing requests through proxy-aware fetches. Preserves Request inputs. Passes proxy agents to Relay control and data WebSocket connections. Adds credential handling, cached agents, integration tests, and fetch-call-site audit updates.

Priority: ⬆️ High

Merge Risk: 🟡 Moderate · up to 2d157

Relay connections can leak duplicate sockets after a restart or become unable to reconnect after proxy or socket creation fails. These lifecycle defects should be fixed before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: routing Cloud and Relay traffic through the configured HTTP proxy.
Description check ✅ Passed The description is complete and follows the repository template. It explains the change and rationale, links issue #19976, documents testing and platform coverage, includes security and compatibility …
Linked Issues check ✅ Passed Issue #19976 requires proxy-aware Cloud authentication and Relay director/control connections with bypass support. Cloud profile and Relay HTTP requests use fetchWithConfiguredProxy. Relay control a…
Out of Scope Changes check ✅ Passed The changes remain within issue #19976. The proxy resolver, HTTP client routing, Relay socket agents, dependency updates, audit updates, and regression tests support proxy-aware Cloud and Relay networ…

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 52918dcb-8535-4a80-a605-2d3b018f46b4

📥 Commits

Reviewing files that changed from the base of the PR and between 22ca862 and 49f7feb.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (19)
  • package.json
  • src/main/global-fetch-call-site-audit.test.ts
  • src/main/network/electron-proxy-credentials.ts
  • src/main/network/http-client.ts
  • src/main/network/outbound-proxy.test.ts
  • src/main/network/outbound-proxy.ts
  • src/main/orca-profiles/profile-cloud-client.test.ts
  • src/main/orca-profiles/profile-cloud-client.ts
  • src/main/orca-profiles/profile-cloud-org-members-client.ts
  • src/main/proxy-guarded-fetch-call-site-audit.test.ts
  • src/main/runtime/relay/relay-control-client-options.ts
  • src/main/runtime/relay/relay-control-client.test.ts
  • src/main/runtime/relay/relay-control-client.ts
  • src/main/runtime/relay/relay-control-origin.ts
  • src/main/runtime/relay/relay-http-client.test.ts
  • src/main/runtime/relay/relay-http-client.ts
  • src/main/runtime/relay/relay-region-preference.ts
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/runtime/rpc/relay-transport.ts
💤 Files with no reviewable changes (1)
  • src/main/global-fetch-call-site-audit.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

Comment thread src/main/network/http-client.ts Resolved
Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/runtime/rpc/relay-transport.ts Resolved
- The Chromium-less branch read the resolved policy, which strips the proxy
  userinfo (Chromium answers an auth challenge instead) and has no session
  credential store to fall back on, so an authenticated HTTPS_PROXY produced an
  agent with no Proxy-Authorization and every proxied request failed with 407.
  It now reads the proxy env URL directly, credentials included.
- A relay data socket whose proxy resolution outlived stop() was never
  terminated by it, because stop() snapshots the sockets it knows about. The
  socket is now dropped when stop() wins that race.
- Adds a test that drives a real request through the Node fetch fallback's
  dispatcher against a local CONNECT proxy — the seam a runtime undici change
  could silently break — plus the regression tests for both fixes above.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The delta (6a06695, harden the outbound proxy path after review) closes out the three findings from the prior review and adds real coverage for each.

  • Env-proxy credentials now travel — resolveOutboundProxyUrl reads getProxyUrlFromEnvironment directly on the Chromium-less path, so an authenticated HTTPS_PROXY/HTTP_PROXY reaches the agent/dispatcher instead of being stripped by separateElectronProxyCredentials.
  • Real dispatcher interop test — new http-client.test.ts drives nodeHttpClient.fetch through a local CONNECT proxy, proving the undici ProxyAgent per-request dispatcher tunnels an actual request rather than only asserting its type.
  • Relay socket stop race closed — openSocket re-checks stopped after proxy resolution and returns null; openConnection throws relay_transport_stopped, with a test that stops during a held resolveProxy.
  • Env credential assertion — the env-fallback test now uses a credentialed proxy URL and checks it reaches both resolveOutboundProxyUrl and the socket agent.

Verified locally: pnpm test on http-client.test.ts, outbound-proxy.test.ts, and relay-transport.test.ts — 19 passed.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

- Chromium's resolver returns an ordered failover list; its later entries are
  fallbacks for a failed attempt this layer cannot observe, so a route whose
  first directive is DIRECT (or one it cannot tunnel) now stays direct instead
  of being promoted to a fallback proxy.
- The implicit loopback bypass covers 127.0.0.0/8, not only 127.0.0.1.
- A host with no Chromium session reads the proxy environment directly, so it
  now honours NO_PROXY (host, leading-dot suffix, `*`, optional port) instead of
  sending a bypassed internal target through the proxy.
- fetchWithConfiguredProxy keeps a Request input intact: the port accepts
  RequestInfo and forwards it, using only its URL to select the proxy.
- A second openConnection for a connId whose first open is still resolving its
  proxy no longer creates a socket that overwrites the first one's mapping.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ab3d0533-e47e-41a7-9d9c-19d47d0b0d49

📥 Commits

Reviewing files that changed from the base of the PR and between 49f7feb and 6a06695.

📒 Files selected for processing (5)
  • src/main/network/http-client.test.ts
  • src/main/network/outbound-proxy.test.ts
  • src/main/network/outbound-proxy.ts
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/runtime/rpc/relay-transport.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/network/outbound-proxy.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

Comment thread src/main/network/outbound-proxy.ts Resolved
Comment thread src/main/runtime/rpc/relay-transport.ts Resolved
stop() then start() cleared the stopped flag, so an open whose proxy resolution
was still in flight could register and authenticate a stale socket in the new
lifecycle. The claim an open takes before that await now doubles as its
lifecycle marker: stop() drops every claim, so the open aborts instead.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run covers the delta since the prior pullfrog review at 6a06695 — 7e7b417 (address the remaining proxy review findings) and 94d0ffa (cancel an open whose proxy resolution outlived a restart).

  • Claim-based socket cancellation — openSocket now re-validates its claimedConnectionIds claim after awaiting the proxy instead of reading this.stopped, so an open that outlived stop() cannot adopt itself into a lifecycle restarted by a later start() (94d0ffa).
  • Stop→restart regression test — a new case holds resolveProxy across stop() + start() and asserts the open rejects with relay_transport_stopped and the cell sees no connection.
  • Ordered resolver directives — proxyUrlFromRules honours only the first Chromium directive, so a route that leads with DIRECT (or a directive it cannot tunnel) stays direct instead of being promoted to a fallback proxy.
  • Wider loopback bypass — the implicit bypass now covers all of 127.0.0.0/8, matching Chromium.
  • NO_PROXY on the Chromium-less path — isEnvBypassed implements host / leading-dot suffix / * / optional-port matching, closing the last open question from the initial review.
  • Request inputs forwarded intact — fetchWithConfiguredProxy keeps the Request (method, headers, body) and uses only its URL to select the proxy.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Outside the diff (2)

🟠 Major · Block generation changes while a connection claim is pending.

src/main/runtime/rpc/relay-transport.ts:97-98
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Block generation changes while a connection claim is pending.

openConnection() adds the connection ID to claimedConnectionIds before awaiting openSocket(), while socketsByConnectionId is still empty. setGeneration() can therefore update this.generation. When the socket opens, onOpen sends the earlier connTicket with the new generation. The relay accepts the ticket only when session.generation === generation, so it closes this connection as invalid host data authentication.

Treat claimedConnectionIds as active connections in this transition guard.

Proposed fix
     if (
       this.socketsByConnectionId.size > 0 ||
+      this.claimedConnectionIds.size > 0 ||
       !Number.isSafeInteger(generation) ||
       generation <= 0
🟠 Major · Fence proxy resolution against a closed origin.

src/main/runtime/relay/relay-control-origin.ts:207-211
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Fence proxy resolution against a closed origin.

openControl() can resume after RelayControlOrigin.close() sets closed, clears controls, and closes existing clients. The continuation then creates a RelayControlClient, adds it to the cleared set, and calls connect(), which creates the control socket. activate() checks only set membership and liveness, so this stale client can become active after shutdown. Add an origin lifecycle check or epoch fence after proxy resolution and before activation.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: a4ec2a73-ebd3-498b-8959-fd9851ed5982

📥 Commits

Reviewing files that changed from the base of the PR and between 6a06695 and 7e7b417.

📒 Files selected for processing (6)
  • src/main/network/http-client.test.ts
  • src/main/network/http-client.ts
  • src/main/network/outbound-proxy.test.ts
  • src/main/network/outbound-proxy.ts
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/runtime/rpc/relay-transport.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/network/http-client.test.ts
  • src/main/network/http-client.ts
  • src/main/network/outbound-proxy.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 768b4cc6-9958-40ba-ab10-ba2bde8c86d4

📥 Commits

Reviewing files that changed from the base of the PR and between 7e7b417 and 94d0ffa.

📒 Files selected for processing (2)
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/runtime/rpc/relay-transport.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment thread src/main/runtime/rpc/relay-transport.ts Resolved
Both windows come from awaiting proxy resolution before the socket exists.

- setGeneration() only guarded on registered sockets, so a generation change
  could land while a connection was still claimed and its open handshake would
  authenticate the ticket against the new generation. A claimed connection now
  blocks the transition, like a registered one.
- openControl() resolved the socket agent before the client existed, so a
  close() in that window cleared `controls` first and the client built
  afterwards was never included in the teardown, still able to activate after
  shutdown. The origin now refuses to build a control once closed.
@mmarabel

Copy link
Copy Markdown
Contributor Author

Both outside the diff findings are fixed in 2d157ebc7f:

  • Block generation changes while a connection claim is pending — setGeneration() now treats a claimed connection like a registered one, so a transition cannot land while a socket is still opening and have its auth frame carry the wrong generation. Test refuses a generation change while a connection is claimed holds proxy resolution, asserts the transition throws, then lets the open complete through a real CONNECT proxy; it fails without the guard.
  • Fence proxy resolution against a closed origin — the socket agent is resolved before the client is constructed, followed by a this.closed check, so a close() landing during that await can no longer leave a control the teardown never saw. Test refuses a control whose proxy resolution outlived the origin holds resolveProxy, closes the origin, then asserts the open rejects with relay_control_closed and that no control was constructed; it fails without the fence.

Verification on 2d157ebc7f: pnpm tc clean, the 3032-test src/main/** sweep green, check:code-quality:changed clean against the PR base.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Outside the diff (1)

🟠 Major · Bind the claim to one open attempt.

src/main/runtime/rpc/relay-transport.ts:130
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bind the claim to one open attempt.

If stop() clears the claim and start() opens the same connId before the old openSocket() continuation completes, the old continuation sees the new lifecycle's claimedConnectionIds.has(connId) result and creates a socket. The new open can also create a socket. The later socketsByConnectionId.set() replaces the earlier mapping, so the replaced socket is no longer reachable through normal connection cleanup.

Store a unique claim token for each open attempt. Check that the stored token still matches after proxy resolution. Delete the claim only when its token matches.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 980e0837-5404-4e8a-9fca-c533f1a2bc2d

📥 Commits

Reviewing files that changed from the base of the PR and between 94d0ffa and 2d157eb.

📒 Files selected for processing (4)
  • src/main/runtime/relay/relay-control-origin.test.ts
  • src/main/runtime/relay/relay-control-origin.ts
  • src/main/runtime/rpc/relay-transport.test.ts
  • src/main/runtime/rpc/relay-transport.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run covers the delta since the prior pullfrog review at 94d0ffa — 2d157eb (fence the generation change and the origin teardown), which closes the two findings raised against 7e7b417.

  • Generation transitions now wait for opening sockets — CloudRelayTransport.setGeneration also treats claimedConnectionIds as blocking, so a conn-open whose socket is still resolving its proxy cannot have its auth frame carry a generation assigned after the claim.
  • The origin fences proxy resolution against teardown — RelayControlOrigin.openControl resolves the socket agent before constructing the client and throws relay_control_closed if close() landed during that await, so no client can be created after controls was cleared.
  • Two regression tests — a held resolveProxy with a live claim asserts setGeneration throws; a held proxy across origin.close() asserts the open rejects with relay_control_closed and no client is constructed. Both fail against the pre-commit sources (verified locally).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

A claim identified its connId but not the attempt that made it, so a stale
continuation — one whose stop() already dropped its claim — saw a newer
attempt's claim on the same connId and created a second socket, letting the
later registration replace the earlier mapping and hide that socket from
normal cleanup. Each attempt now takes a unique claim and only proceeds when
its own claim is still the one recorded.
@mmarabel

Copy link
Copy Markdown
Contributor Author

Fixed in cf2efefe66: each open now takes a unique claim token instead of marking the connId alone, and both the pre-socket check and the release only act while that exact token is still the one recorded. A stale continuation whose stop() already dropped its claim therefore aborts instead of finding a newer attempt's claim on the same connId, and a stale finalize cannot delete a newer attempt's claim either.

Test keeps a stale open from taking the socket of a newer attempt for the same connId: attempt A opens conn-1 while resolveProxy is held, stop() then start(), attempt B opens the same connId, then the held resolution releases for both — A rejects with relay_transport_stopped and exactly one connection reaches the cell. It fails with the claim-as-connId-marker version.

Verification on cf2efefe66: pnpm tc clean, the 3033-test src/main/** sweep green, check:code-quality:changed clean against the PR base.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run covers the delta since the prior pullfrog review at 2d157eb — cf2efef (bind each connection claim to one open attempt), which closes CodeRabbit's finding that a stale open could adopt a newer attempt's claim.

  • Claim token per open attempt — claimedConnectionIds is now a Map<connId, symbol>; openSocket re-checks that the stored symbol is still its own after the proxy await, so an open that outlived stop() (even across a later start()) throws relay_transport_stopped instead of registering a socket the new lifecycle never asked for.
  • Conditional claim release — finalize deletes the claim only while it still owns it, so a late finalize cannot clear a newer attempt's claim for the same connId.
  • Stop drops in-flight claims — stop() clears the map, so a still-resolving open cannot survive into the next lifecycle.
  • Regression test — holds resolveProxy across a stop() + start(), reopens the same connId, and asserts the stale open rejects while the cell sees exactly one connection (two before the fix).

Verified locally: pnpm test src/main/runtime/rpc/relay-transport.test.ts — 12 passed.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

A failed dial — the proxy lookup rejecting or the socket constructor throwing —
left the claim behind, and every later open of that connId then returned
without dialling until stop() cleared the claims.

The proxy-aware dial moves to relay-cell-socket.ts, which is also where the
claim is re-checked after its await, so a superseded open is dropped before it
can produce a socket. The transport file was at its line budget, so this is the
split rather than a suppression.
@mmarabel

Copy link
Copy Markdown
Contributor Author

CodeRabbit's "Release the connection claim when socket creation fails" is fixed in e03378ed4f (the thread is already resolved; noting it here for the record):

  • The proxy-aware dial moved to src/main/runtime/rpc/relay-cell-socket.ts and re-checks isCurrent() after resolving the agent, so an attempt that stop() or a newer attempt superseded is dropped before it creates a socket.
  • openConnection releases the claim in a catch when the dial rejects, covering both the proxy lookup failing and a synchronous throw from an injected factory. A failed attempt no longer blocks the next open of that connId.
  • Test releases the claim when the dial fails: the proxy lookup rejects for the first open, then succeeds for the second, which must reach the cell through the CONNECT proxy. Without the release the second open returns without dialling and the test times out at the 30s limit.
  • The extraction is the max-lines remedy, not a suppression: relay-transport.ts sat exactly at its 300 counted-line budget.

Verification on e03378ed4f: pnpm tc clean, the 3034-test src/main/** sweep green, check:code-quality:changed clean against the PR base.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run covers the delta since the prior pullfrog review at cf2efef — e03378e (release a connection claim when the dial fails).

  • Failed dials release their claim — openConnection now wraps socket creation in try/catch and calls a new symbol-guarded releaseClaim, so a rejected outboundProxySocketAgent or a throwing injected factory no longer leaves a connId claimed and silently skipping every later open until stop() clears the map.
  • Proxy dial extracted to relay-cell-socket.ts — dialRelayCellSocket(url, isCurrent) owns the proxy-aware WebSocket construction and the post-await claim re-check; the transport stores its optional factory as a nullable injectedSocketFactory, which also keeps relay-transport.ts within its max-lines budget without a suppression.
  • Regression test — a rejecting resolveProxy makes the first openConnection reject, then a reopen of the same connId is asserted to tunnel to the cell; the leaked claim makes that reopen a no-op, so the case cannot pass against the pre-fix source.

Verified locally: pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/rpc/relay-transport.test.ts — 13 passed.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…onflicts

# Conflicts:
#	src/main/runtime/relay/relay-control-client.test.ts
#	src/main/runtime/relay/relay-control-client.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run covers the delta since the prior pullfrog review at e03378e — d527325 (Merge remote-tracking branch 'upstream/main' into mmarabel/pr-20894-conflicts), which integrates main (646e9a5) and resolves the two conflicts it reported.

  • Control-client conflict resolved additively — relay-control-client.ts keeps this PR's socketAgent plumbing (the createSocket(url, token, agent?) seam and the ...(agent ? { agent } : {}) socket option) alongside upstream's RelayControlLiveness wiring (livenessRandom, pong handling), so neither the proxy agent nor the half-open-recovery probes were dropped.
  • Options type carries both fields — relay-control-client-options.ts exposes socketAgent and upstream's livenessRandom; the base's silenceLimitMs is gone, as expected.
  • Test-file conflict resolved additively — relay-control-client.test.ts retains this PR's dials the control socket through the app proxy agent case and upstream's scripted-socket / half-open-recovery suites.
  • No other PR-touched file changed — the range-diff against the prior head isolates only the two conflicted relay-control files; the resolver, HTTP clients, transport, and audit tables are unchanged.

Verified the resolution locally: relay-control-client.test.ts (20) and relay-transport.test.ts (13) pass, and pnpm tc:node is clean. All prior pullfrog threads remain addressed and resolved.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
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.

[Bug]: Main-process network calls (Cloud auth + Relay control) don't honor the app's configured HTTP proxy

1 participant