Sign up
View stargazersSign up

fix(ssh): tunnel readiness no longer counts ssh's connection time#33554

merged6
main
fix/issue-33531
opened1d agobyJJulius Marminge
2 files changed
View on GitHubTwo-way sync enabledPR last imported 14h ago

Problem

A desktop SSH environment fails with Timed out waiting 20000ms for backend readiness when its host name resolves to two or more unreachable addresses before the reachable one. ssh spends ConnectTimeout=10 on each dead address, and the tunnel's 20s readiness clock started when ssh was spawned, so the tunnel was abandoned while ssh was still connecting (processRunning: true, localPortListening: false).

Reproduced on bf469bb196 with real ssh against an isolated sshd, using a host name whose addresses are two blackholed link-local addresses followed by the reachable one (ssh -o ConnectTimeout=10 <host> true takes 20.27s), driving the desktop's SshEnvironmentManager.ensureEnvironment.

Change

startSshTunnel now waits for ssh's local forward to listen (NetService.hasListenerOnHost, polled every 100ms) before it starts the unchanged 20s backend readiness wait. The connect wait is capped at 90s, the same bound the node-script launch command already gets for its whole ssh run against that host, and fails with an SshReadinessError saying the SSH connection did not come up. Both phases still race the ssh process exit, so a connection that fails surfaces ssh's own error immediately.

Scope and approval

Fixes pingdotgg/t3code#33531

Verification

  • vp test run packages/ssh/src/tunnel.test.ts: 12 passed. waits for a tunnel whose ssh connection outlasts the readiness timeout (forward listens after 25s) and fails with a readiness error when ssh never connects (90s cap, tunnel process killed) both fail on main and pass here (before log, after log).
  • vp run check:changed: format, lint, knip, typecheck, tests pass.

Runtime evidence (real ssh)

Real OpenSSH/sshd 9.6p1 on Linux, each run in its own network namespace with a loopback-only sshd, throwaway keys and config, dropped addresses via namespace-private iptables, and ordered resolver answers injected for the fixture host only. The driver calls SshEnvironmentManager.ensureEnvironment from each side's own checkout; only the remote T3 launch is stubbed (it reports an external server on a local HTTP port the fixture controls). Same driver and fixtures on both sides; times are whole ensureEnvironment calls. Each cell links its timestamped transcript.

CaseBefore ee59ab22a8After e4868beb9d
F01 direct address, backend readyok, 0.6sok, 0.7s
F02 1 unreachable address firstok, 10.5sok, 10.7s
F03 3 unreachable addresses first (the report)Timed out waiting 20000ms for backend readiness, 20.3sok, 30.6s
F04 forward after 10s, backend ready 15s laterbackend readiness timeout, 20.4sok, 25.5s
F05 forward after ~85s, backend ready 10s laterbackend readiness timeout, 20.2sok, 95.5s
F06 ssh alive, forward never listensbackend readiness timeout, 20.4sSSH connection to fixture-never did not come up within 90000ms., 90.3s, 0 HTTP probes, child killed
F06b same, retried in one manager scope2× backend readiness timeout, ~20.2s each, fresh tunnel each2× SSH connection error, ~90.2s each, fresh tunnel (new PID and port) each
F07 forward listens, backend always 503backend readiness timeout, 20.4sbackend readiness timeout, 22.6s
F08 forward listens, HTTP headers never arrivebackend readiness timeout, 21.1sbackend readiness timeout, 21.1s
F09 forward after 30s, backend never readybackend readiness timeout, 20.3sbackend readiness timeout, 50.9s
F10 ssh refused before forwardingssh's Connection refused, 0.3sssh's Connection refused, 0.3s

ssh exiting while the HTTP wait runs (F11) still surfaces ssh's own error on both sides (3.6s). Cached-tunnel reuse, stale-tunnel replacement, target locking and preparation guards behave the same on both sides (full index with all rows and raw artifacts, F06b index). Rebuild: run-fixture.py, driver.mjs, ssh-wrapper.py, resolver.c, all rebuild scripts, F06b driver diff.

Add Environment error (F06's message as the user sees it)

Electron 44 under Xvfb, DPR 1, 100% font scaling, app theme set explicitly, reduced motion. Labelled harness: the real Settings → Add Environment flow, with the desktop bridge's SSH call rejected with each side's measured F06 error; the connection-layer wrapping, formatter and alert run unchanged. apps/web is identical on both sides.

Viewport · themeBefore
ee59ab22a8		After
e4868beb9d		Diff0DiffT (>16/255)
1440×900 · lightBefore: 1440×900 · lightAfter: 1440×900 · lightDiff0: 1440×900 · lightDiffT: 1440×900 · light
1440×900 · darkBefore: 1440×900 · darkAfter: 1440×900 · darkDiff0: 1440×900 · darkDiffT: 1440×900 · dark
390×900 · lightBefore: 390×900 · lightAfter: 390×900 · lightDiff0: 390×900 · lightDiffT: 390×900 · light
390×900 · darkBefore: 390×900 · darkAfter: 390×900 · darkDiff0: 390×900 · darkDiffT: 390×900 · dark

What else changed: both alerts keep the prefix Could not prepare the SSH environment: Error invoking remote method 'desktop:ensure-ssh-environment': SshReadinessError: . At 1440 the shorter text takes two lines instead of three, so the alert is 20px shorter and the dialog re-centres 10px lower. At 390 only the alert text pixels differ. Same-text control pairs differ by zero pixels. Scripts: capture-electron.cjs, bridge-harness.js, compare.py, reproduce.sh, source trace.

Not run: native macOS and Windows ssh; OpenSSH binds -L forwards only after authentication on every platform, which is what the wait relies on.

Verification log
  • Review round 1 on d9c8cd4520: two independent reviewers, both MERGE, no confirmed findings. Both noted the timeout comment named the wrong launch; fixed in e4868beb9d (comment only).
  • Proof audit: fixture matrix derived from the code (matrix); pass 2 checked every minimum row against its transcript or image, recomputed every diff, and rebuilt F03 after independently (30.53s vs 30.56s delivered). Verdict: sufficient (after an added same-scope retry row, F06b, closed its one gap).
  • Evidence head e4868beb9d; later commits merge main only, with no changes under packages/ssh.

AI-assisted; verified with the tests and tools listed above.

Checks

20 successful · 20 skipped
All checks passed
6f200cc

Added 3 commits

  • test(ssh): reproduce tunnel readiness timing out while ssh connects (#33531)57abbd4
  • Merge remote-tracking branch 'origin/main' into fix/issue-33531db31116
  • fix(ssh): tunnel readiness waits for ssh to connect before its 20s limite3bd14d
MmacroscopeappBot1d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — Tunnel startup now allows up to 90 seconds for SSH to connect before starting the 20-second backend readiness timer. A human needs to accept this longer default wait for stalled connections; process-exit monitoring and failure cleanup remain in place.

No code changes detected at 6f200cc. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

Ggithub-actionsBot1d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
CodexLive turn WebSocket decoded21.5 KiB21.5 KiB0 B (0.0%)24.4 KiB✅
CodexLive turn messages110 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.9 KiB21.9 KiB0 B (0.0%)24.4 KiB✅
ClaudeLive turn messages110 (0.0%)8✅

Baseline: bbdc7dc · PR result: 6f200cc · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Sshivam1d ago

[GPT-6.1 Sol][Codex] on behalf of shivam.

Reproduced through Settings → Environments → Add Environment in the Linux desktop app on corex, built from e85e0094dd4617a1eada1ad7961f2132a79e67b2. The fixture used real OpenSSH and an isolated real T3 backend. A launcher delayed forwarding startup by 21 seconds before executing SSH; it did not reproduce the reporter's DNS/address ordering. The UI failed with Timed out waiting 20000ms, with processRunning: true and localPortListening: false in the desktop log.

Separately, a regression test failed with the original 20s budget and passed with my alternative 60s-budget change at 7db63c0c45f04b36efe708e8d7743bfbfa781f78: 12 focused tunnel tests passed, including delayed readiness and a bounded never-ready case. These results are for that separate branch, not this PR. I did not test this PR's implementation, the fixed desktop UI, or macOS.

Linux desktop screenshot · Recording of Add environment through the timeout. Both contain only test data.

Stopping the separate work in favor of this PR. Test processes and temporary files have been cleaned up.

CcoderabbitaiBot1d ago
Synced from GitHub

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Tunnel startup now polls the local loopback port for the SSH forward every 100 ms, for up to 90 seconds. It runs the existing HTTP readiness check after the forward listens. Tests cover delayed readiness and timeout cleanup.

Changes: SSH tunnel readiness

Layer / File(s)Summary
Local forward polling
packages/ssh/src/tunnel.ts		The tunnel readiness check polls the local port every 100 ms for up to 90 seconds. It fails with SshReadinessError if no listener appears.
Startup ordering and readiness tests
packages/ssh/src/tunnel.ts, packages/ssh/src/tunnel.test.ts		Tunnel startup waits for the local forward before checking HTTP readiness. Tests cover a forward that listens after 25 seconds and a forward that never listens; the latter expects an error and verifies the tunnel is killed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant TunnelStartup
3 participant waitForTunnelForward
4 participant LocalNetworkService
5 participant HttpClient
6 TunnelStartup->>waitForTunnelForward: wait for local SSH forward
7 waitForTunnelForward->>LocalNetworkService: check listener on 127.0.0.1
8 LocalNetworkService-->>waitForTunnelForward: return listener status
9 waitForTunnelForward-->>TunnelStartup: report forward ready
10 TunnelStartup->>HttpClient: check HTTP readiness

Suggested reviewers: t3dotgg

Merge Risk: External image Minimal · up to e4868

SSH tunnel startup now waits for the local forward to listen before starting the 20-second backend readiness timer. This should fix timeouts caused by slow SSH connection attempts. No merge-blocking risk was found in the supplied context.

Pre-merge checks: Passed 4

Check nameStatusExplanation
Title checkPassedThe title clearly describes the main change: SSH connection time no longer consumes the backend readiness timeout.
Linked Issues checkPassedIssue #33531 requires SSH tunnel startup to tolerate delayed SSH connections before the local forward listens. tunnel.ts now waits for the loopback forward before starting the existing 20-second HTT…
Out of Scope Changes checkPassedThe whole-PR diff changes packages/ssh/src/tunnel.ts and packages/ssh/src/tunnel.test.ts. The implementation and tests directly support issue #33531. The incremental change only updates a related …
Description checkPassedThe description covers the problem, change, scope, and detailed verification, including tests and platform limitations. It identifies AI assistance but does not name the model and harness as the templ…
Full details: Linked Issues check

Explanation

Issue #33531 requires SSH tunnel startup to tolerate delayed SSH connections before the local forward listens. tunnel.ts now waits for the loopback forward before starting the existing 20-second HTTP readiness check. The connection wait is capped at 90 seconds and races SSH process exit. Tests cover a forward that listens after 25 seconds and a connection that never comes up, including process cleanup. The change since the prior review only updates a comment.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff changes packages/ssh/src/tunnel.ts and packages/ssh/src/tunnel.test.ts. The implementation and tests directly support issue #33531. The incremental change only updates a related timeout comment. No unrelated changes are present.

Full details: Description check

Explanation

The description covers the problem, change, scope, and detailed verification, including tests and platform limitations. It identifies AI assistance but does not name the model and harness as the template requests; this is a minor omission.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between d9c8cd4520ac1e4cc9c8141db9f215716e6d4fd8 and e4868beb9d922edc2dd0ebaf6db16c996b044d5b.

External image Files selected for processing: 1

  • packages/ssh/src/tunnel.ts
External image Files skipped from review as they are similar to previous changes: 1

  • packages/ssh/src/tunnel.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Added 3 commits

  • Merge remote-tracking branch 'origin/main' into fix/issue-33531d9c8cd4
  • docs(ssh): say which launch the tunnel connect wait mirrorse4868be
  • Merge remote-tracking branch 'origin/main' into fix/issue-335316f200cc