Skip to content

fix(web): interrupted SSH pairing retries reuse unfinished setup - #19666

Merged
juliusmarminge merged 6 commits into
pingdotgg:mainfrom
im-kvijay:fix/ssh-retry-adopts-setup
Oct 20, 2026
Merged

juliusmarminge merged 6 commits into
pingdotgg:mainfrom
im-kvijay:fix/ssh-retry-adopts-setup

Conversation

@im-kvijay

@im-kvijay im-kvijay commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Interrupted SSH pairing attempts keep running in desktop IPC. On a slow host, retrying used to queue another setup and leave the first pairing credential unused. A retry now waits for the interrupted attempt’s unfinished setup and exchanges its credential once. Settlement and explicit disconnect remove the retained work.

Rebuilt from the original contribution by @im-kvijay in #19666, preserving the author’s commits and merging main. The takeover replaces permanent disconnect counters with state that exists only while setup is unfinished, and removes exported internal helpers. Saved-token reconnection continues to request no pairing credential; broader startup optimizations are outside this change.

Verification on head c7ec0d2572:

  • Existing platform.test.ts: original PR 22 tests → 21 tests, with the added coverage reduced from four cases to three behavioral tests. They cover repeated interruption and credential exchange, settlement in success/failure, and disconnect invalidation through the platform gateway.
  • Red/green: main 1 failed, 20 passed; rebuilt implementation 21 passed. Removing settlement cleanup or disconnect invalidation separately makes its regression test fail.
  • Scoped web lint, web typecheck, formatting and whitespace checks pass.
  • In headless Chromium, the Vite-served provisioning code with a controlled desktop IPC boundary produced one setup, one credential exchange, correct credential after two interrupted attempts. No visual component changed; the focused behavior tests and compiled-client probe are the proof.

Not verified: an actual slow SSH host, the Electron desktop process or packaged AppImage, macOS, Windows, iOS or Android. The browser probe controls IPC and is not a claim of end-to-end SSH verification. One independent full-diff review round (Claude and Sol) found no blockers. A narrow remaining case is interleaving onboarding and removal for the same host with unresolved versus resolved targets: a retry may adopt setup that the removal then tears down, requiring one further retry. Current-head CI passed after one rerun of the two unrelated browser failures. The earlier cancelled fingerprint run was superseded by a successful run.

Implementation: Codex gpt-6.1-sol (high), tests: Codex gpt-6-astra (high); harness: T3 Code with Codex. Independent review: Claude claude-opus-5-5 and Codex gpt-6-sol through T3 delegation.

On a slow host an SSH connection attempt can time out while the desktop is
still setting up the environment. The desktop runs one setup per target at a
time and cannot cancel it, so each retry queued another setup behind the lock,
and each one minted a pairing credential. The next attempt now awaits the
abandoned setup and spends its credential instead.

Fixes pingdotgg#18229

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 8f3d31f

Macroscope's review found this PR approvable — This is a contained SSH retry bug fix that coalesces abandoned setup operations in the web connection layer, with targeted coverage for timeout, disconnect, and completion cases. It does not alter contracts, defaults, deployment behavior, or static-analysis settings.

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

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1d6c87e3-53db-4135-b4b9-c1f42d3e57cf
📥 Commits

Reviewing files that changed from the base of the PR and between 19feeca and 9637569.

📒 Files selected for processing: 2
  • apps/web/src/connection/platform.test.ts
  • apps/web/src/connection/platform.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b7461e63-1f6e-4740-8537-e97fc3f2bff6
📥 Commits

Reviewing files that changed from the base of the PR and between 8f3d31f and 19feeca.

📒 Files selected for processing: 2
  • apps/web/src/connection/platform.test.ts
  • apps/web/src/connection/platform.ts

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

📝 Walkthrough

SSH setup attempts now retain in-flight setup work for a retry after interruption, unless a disconnect clears it. Provisioning uses the shared setup helper. Tests cover retries, disconnect cleanup, and setup completion.

Changes: SSH setup handoff

Layer / File(s) Summary
Coordinate SSH setup attempts
apps/web/src/connection/platform.ts
The shared helper tracks setup promises and disconnect counts by target. Provisioning uses the helper. Disconnect clears retained setup state before calling the desktop bridge.
Test SSH setup handoff
apps/web/src/connection/platform.test.ts
Tests control setup completion and verify pending setup reuse, disconnect cleanup, and new setup after an abandoned setup succeeds or fails.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 19fee

No actionable issue remains identified for this change; it is ready to merge after normal checks.

Security architecture risk: 🔵 Low · up to 19fee

The change is confined to SSH connection orchestration and preserves existing environment-identity checks. No introduced privilege expansion or authentication bypass was identified. Remaining uncertainty concerns simultaneous callers and end-to-end teardown behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The introduced handoff is scoped to matching SSH target keys within the renderer module. It changes which attempt receives an existing setup result, rather than adding a new remote destination or privilege-bearing operation.

Trust Boundaries and Controls

  • observed — Existing controls remain on the credential path: an expected-environment mismatch blocks pairing-token exchange, and saved-token preparation checks environment identity before sending the saved token. A saved session is reused only when authenticated with all standard client scopes.

Resilience and Maintainability Implications

  • inferred — Production lifecycle ordering supports the disconnect invalidation design: disabling signals supervisor disconnect before SSH teardown, removal closes the service scope before teardown, and connection establishment races interruption and timeout. This supports cancellation of normal preparation attempts but is not end-to-end verification of remote credential or server teardown.
  • observed — The helper retains abandoned setups, not all active setups, so simultaneous active callers can still request separate credential-producing setups. The base also issued separate setup requests per call; neither new attacker reachability nor worsened security exposure from this concurrency was established.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Linked Issues check Warning Issue [#18229] has three coding requests. The PR implements the retry coalescing request for token-issuing SSH setup in apps/web/src/connection/platform.ts, and platform.test.ts covers adoption, s… Implement the startup optimization and server-bundle lazy loading requested by [#18229], or narrow the linked issue scope before treating the issue requirements as complete. If prepare must share retry coalescing, route its direct setup c…
Passed Passed checks: 3 passed
Check name Status Explanation
Out of Scope Changes check Passed The changes are limited to SSH setup coordination in platform.ts and focused tests in platform.test.ts. The helper, settlement cleanup, disconnect guard, and tests support the retry-coalescing req…
Title check Passed The title is concise, specific, and accurately describes the main change: interrupted SSH pairing retries reuse unfinished setup.
Description check Passed The description clearly explains the problem, implementation, verification results, limitations, and agent usage. It does not explicitly link the triaged issue or maintainer approval required by the S…
Full details: Out of Scope Changes check

Explanation

The changes are limited to SSH setup coordination in platform.ts and focused tests in platform.test.ts. The helper, settlement cleanup, disconnect guard, and tests support the retry-coalescing request in [#18229]. No unrelated contract, server, desktop main-process, or mobile changes appear in the reviewed diff.

Full details: Description check

Explanation

The description clearly explains the problem, implementation, verification results, limitations, and agent usage. It does not explicitly link the triaged issue or maintainer approval required by the Scope and approval section, but the remaining required information is substantially covered.

Full details: Linked Issues check

Explanation

Issue [#18229] has three coding requests. The PR implements the retry coalescing request for token-issuing SSH setup in apps/web/src/connection/platform.ts, and platform.test.ts covers adoption, settlement cleanup, and disconnect handling. The PR does not implement the requested once-per-launch startup optimization or lazy loading of the server bundle. The direct prepare path with a saved bearer token also still calls bridge.ensureSshEnvironment directly, although the PR summary states that prepare uses the shared helper.

Resolution

Implement the startup optimization and server-bundle lazy loading requested by [#18229], or narrow the linked issue scope before treating the issue requirements as complete. If prepare must share retry coalescing, route its direct setup call through the shared helper and add coverage for that path.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

im-kvijay and others added 2 commits October 18, 2026 09:22
…sh-retry-adopts-setup

# Conflicts:
#	apps/web/src/connection/platform.test.ts
#	apps/web/src/connection/platform.ts
@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one as maintainer for batch ssh-relay-remote-08. I’ll keep the focused SSH retry fix on this PR, rebuild and verify it against current main, and preserve your commits and credit.

@juliusmarminge juliusmarminge changed the title fix(web): SSH retries wait for the setup already running fix(web): interrupted SSH pairing retries reuse unfinished setup Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Takeover update: the rebuilt head is c7ec0d2572. One full-diff review round with Claude Opus 5.5 and Codex GPT-6-Sol found no blockers. The description now limits the claim to interrupted credential-issuing setup; saved-bearer setup and the broader startup work are outside this change.

The focused SSH suite passes all 21 tests. Current-head build/typecheck, lint, package tests and all server shards passed. The first CI attempt failed in existing AttachmentFilePreview and WorkflowOpeningBlock browser tests; both files are unchanged by this PR and passed a targeted local Chromium run (6/6 tests). Requested one rerun of failed jobs. No merge performed.

@juliusmarminge
juliusmarminge merged commit eca0e05 into pingdotgg:main Oct 20, 2026
27 checks passed
Pipyakas pushed a commit to Pipyakas/t3code that referenced this pull request Oct 21, 2026
…gdotgg#19666)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
(cherry picked from commit eca0e05)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants