Skip to content

fix: cancel and clean up endpoint connection attempts - #5247

Merged
JJLiebig merged 3 commits into
masterfrom
issue/4721-attempt-cleanup
Oct 13, 2026
Merged

JJLiebig merged 3 commits into
masterfrom
issue/4721-attempt-cleanup

Conversation

@JJLiebig

@JJLiebig JJLiebig commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Cancelled reconnects could hang while another process kept their pipes open. Cleanup now finishes and stops its own SSH process before retrying the same machine. Final output and useful SSH errors are preserved.

Merge order: after #5242 (already merged).

Tests cover stalled reads/writes, cancellation, and replacement connections; just check passed.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d5847fda-4bf1-4c81-9374-ceed10604530
📥 Commits

Reviewing files that changed from the base of the PR and between d0297a0 and ec1b0b7.

📒 Files selected for processing: 4
  • src/client/endpoint/supervisor.rs
  • src/remote/attach.rs
  • src/remote/process.rs
  • src/remote/saved.rs
💤 Files with no reviewable changes: 2
  • src/client/endpoint/supervisor.rs
  • src/remote/process.rs

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

The change adds cancellation and completion state to endpoint connection attempts. Local connections, handshakes, SSH subprocesses, and bridge I/O now support cancellation. Replaced endpoints wait for unfinished attempts, and transports retain completion state until they are dropped. Tests cover cancellation, timeouts, endpoint replacement, stream closure, and subprocess output handling.

Suggested reviewers: ogulcancelik

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to ec1b0

This change makes endpoint and SSH connection attempts cancellable and cleans up their connections before a retry. The review found no remaining issue. The suspected Windows hang during cancellation does not occur with the dependency version in use.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 91 functions across 12 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: cancelling and cleaning up endpoint connection attempts.
Description check Passed The description directly explains the cancellation, cleanup, SSH process handling, preserved output, tests, and merge-order requirement.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no blocking issues were found.

Summary

This PR makes endpoint connection attempts cancellable and waits for their cleanup before retrying the same endpoint.

  • Replaced endpoint connections wait for the old attempt to close.
  • SSH commands stop their own pipe work and keep useful output.
  • SSH bridges stop stalled forwarding and deliver final output.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Start connection attempt] --> B[Connect and complete handshake]
    B --> C[Writer owns bridge and completion flag]
    C --> D[Endpoint retires or disconnects]
    D --> E[Stop writer and cancel SSH work]
    E --> F[Close stream and drop bridge]
    F --> G[Mark attempt complete]
    G --> H[Allow replacement attempt]
Loading

Reviews (2) · Last reviewed commit: "fix: preserve nonblocking bridge output ..." · Reviewed by Greptile

greptile-apps[bot]
greptile-apps Bot previously approved these changes Oct 13, 2026
@JJLiebig

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@JJLiebig
JJLiebig merged commit 87e5c13 into master Oct 13, 2026
11 of 12 checks passed
aceitw added a commit to aceitw/herdr that referenced this pull request Oct 18, 2026
Keep our fork features (federation, gram, api-bridge, guest, push,
notifications, agent archive/forget/transfer, accounts, staged updates)
alongside upstream's v0.9.4, v0.9.5 and unreleased fixes. Most source
conflicts keep our fork's Method enum (upstream's define_operations!
macro cannot express our cfg-gated and serde-skipped variants). Detection
manifests keep our extra rules; Codex integration scripts switch to
upstream's canonical version.

Upstream highlights:
- Persistent macOS/Linux server (SIGHUP no longer stops it)
- Session saves on system shutdown/restart
- Plugin update command
- Keyboard/IME fixes (non-Latin layouts, Ctrl+Shift, accents)
- Windows improvements (PowerShell 7 default, clipboard, notifications)
- SSH compression and bandwidth improvements
- Performance fixes (CPU, memory, hidden pane improvements)
- Codex, Claude Code, Pi, Antigravity, Grok detection updates
- New platform modules (launchd spawn, power/shutdown)
- Vendor patches (libghostty-vt cursor hyperlink, portable-pty Windows dir)

refs herdrdev#5310, herdrdev#5278, herdrdev#5236, herdrdev#5303, herdrdev#5308, herdrdev#5275, herdrdev#5280, herdrdev#5282,
     herdrdev#5277, herdrdev#5276, herdrdev#5270, herdrdev#5269, herdrdev#5252, herdrdev#5249, herdrdev#5247, herdrdev#5241,
     herdrdev#5242, herdrdev#5192, herdrdev#5232, herdrdev#5212, herdrdev#5174, herdrdev#5173, herdrdev#5171, herdrdev#5092
maciejul pushed a commit to maciejul/herdr-supercharged that referenced this pull request Oct 28, 2026
* fix: cancel and clean up endpoint connection attempts

refs herdrdev#4721

* fix: keep probe teardown scoped to its bridge

refs herdrdev#4721

* fix: preserve nonblocking bridge output and simplify cleanup

refs herdrdev#4721
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.

2 participants